Patchwork [4,of,5,changedelete] merge.mergestate: update docstrings for preresolve and resolve

login
register
mail settings
Submitter Siddharth Agarwal
Date Nov. 5, 2015, 8:12 a.m.
Message ID <7514e2e35e70661a7783.1446711121@dev666.prn1.facebook.com>
Download mbox | patch
Permalink /patch/11295/
State Accepted
Headers show

Comments

Siddharth Agarwal - Nov. 5, 2015, 8:12 a.m.
# HG changeset patch
# User Siddharth Agarwal <sid0@fb.com>
# Date 1446709491 28800
#      Wed Nov 04 23:44:51 2015 -0800
# Node ID 7514e2e35e70661a7783668cf9cddd949ab9cd76
# Parent  14056ae3782ddc3fee4b4bb0b9557aa463e08f3c
merge.mergestate: update docstrings for preresolve and resolve

Add a docstring for preresolve, and update the one for resolve.
Martin von Zweigbergk - Nov. 6, 2015, 1:34 a.m.
On Thu, Nov 5, 2015 at 12:14 AM Siddharth Agarwal <sid0@fb.com> wrote:

> # HG changeset patch
> # User Siddharth Agarwal <sid0@fb.com>
> # Date 1446709491 28800
> #      Wed Nov 04 23:44:51 2015 -0800
> # Node ID 7514e2e35e70661a7783668cf9cddd949ab9cd76
> # Parent  14056ae3782ddc3fee4b4bb0b9557aa463e08f3c
> merge.mergestate: update docstrings for preresolve and resolve
>
> Add a docstring for preresolve, and update the one for resolve.
>

This seems to make sense without the three patches before it, so I've
pushed it to the clowncopter.

I'd prefer to see the other four patches squashed (they do very similar
things and it seems unlikely that only one of them would be reverted) and
be sent out with later patches that use the added return value. Looking at
only these four patches, they just add dead code and I can't say whether it
seems like that code will be useful later.


>
> diff --git a/mercurial/merge.py b/mercurial/merge.py
> --- a/mercurial/merge.py
> +++ b/mercurial/merge.py
> @@ -397,10 +397,15 @@ class mergestate(object):
>          return complete, r
>
>      def preresolve(self, dfile, wctx, labels=None):
> +        """run premerge process for dfile
> +
> +        Returns whether the merge is complete, and the exit code."""
>          return self._resolve(True, dfile, wctx, labels=labels)
>
>      def resolve(self, dfile, wctx, labels=None):
> -        """rerun merge process for file path `dfile`"""
> +        """run merge process (assuming premerge was run) for dfile
> +
> +        Returns the exit code of the merge."""
>          return self._resolve(False, dfile, wctx, labels=labels)[1]
>
>  def _checkunknownfile(repo, wctx, mctx, f, f2=None):
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
>
Siddharth Agarwal - Nov. 6, 2015, 2:55 a.m.
On 11/5/15 17:34, Martin von Zweigbergk wrote:
>
> On Thu, Nov 5, 2015 at 12:14 AM Siddharth Agarwal <sid0@fb.com 
> <mailto:sid0@fb.com>> wrote:
>
>     # HG changeset patch
>     # User Siddharth Agarwal <sid0@fb.com <mailto:sid0@fb.com>>
>     # Date 1446709491 28800
>     #      Wed Nov 04 23:44:51 2015 -0800
>     # Node ID 7514e2e35e70661a7783668cf9cddd949ab9cd76
>     # Parent  14056ae3782ddc3fee4b4bb0b9557aa463e08f3c
>     merge.mergestate: update docstrings for preresolve and resolve
>
>     Add a docstring for preresolve, and update the one for resolve.
>
>
> This seems to make sense without the three patches before it, so I've 
> pushed it to the clowncopter.
>
> I'd prefer to see the other four patches squashed (they do very 
> similar things and it seems unlikely that only one of them would be 
> reverted) and be sent out with later patches that use the added return 
> value. Looking at only these four patches, they just add dead code and 
> I can't say whether it seems like that code will be useful later.

All four squashed into one, you mean?

These are preparatory patches, but the series that actually does things 
is long and complicated enough that I'd like to get all this landed first.

- Siddharth


>
>     diff --git a/mercurial/merge.py b/mercurial/merge.py
>     --- a/mercurial/merge.py
>     +++ b/mercurial/merge.py
>     @@ -397,10 +397,15 @@ class mergestate(object):
>              return complete, r
>
>          def preresolve(self, dfile, wctx, labels=None):
>     +        """run premerge process for dfile
>     +
>     +        Returns whether the merge is complete, and the exit code."""
>              return self._resolve(True, dfile, wctx, labels=labels)
>
>          def resolve(self, dfile, wctx, labels=None):
>     -        """rerun merge process for file path `dfile`"""
>     +        """run merge process (assuming premerge was run) for dfile
>     +
>     +        Returns the exit code of the merge."""
>              return self._resolve(False, dfile, wctx, labels=labels)[1]
>
>      def _checkunknownfile(repo, wctx, mctx, f, f2=None):
>     _______________________________________________
>     Mercurial-devel mailing list
>     Mercurial-devel@selenic.com <mailto:Mercurial-devel@selenic.com>
>     https://selenic.com/mailman/listinfo/mercurial-devel
>
>
>
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@selenic.com
> https://selenic.com/mailman/listinfo/mercurial-devel
Martin von Zweigbergk - Nov. 6, 2015, 2:58 a.m.
Yes, all four squashed. If you squash those four, will that plus the next
four-or-so patches give enough context for me to understand what the added
return value is for? If not, could you send a link to a repo to pull from
so I can see?

On Thu, Nov 5, 2015, 18:55 Siddharth Agarwal <sid@less-broken.com> wrote:

> On 11/5/15 17:34, Martin von Zweigbergk wrote:
> >
> > On Thu, Nov 5, 2015 at 12:14 AM Siddharth Agarwal <sid0@fb.com
> > <mailto:sid0@fb.com>> wrote:
> >
> >     # HG changeset patch
> >     # User Siddharth Agarwal <sid0@fb.com <mailto:sid0@fb.com>>
> >     # Date 1446709491 28800
> >     #      Wed Nov 04 23:44:51 2015 -0800
> >     # Node ID 7514e2e35e70661a7783668cf9cddd949ab9cd76
> >     # Parent  14056ae3782ddc3fee4b4bb0b9557aa463e08f3c
> >     merge.mergestate: update docstrings for preresolve and resolve
> >
> >     Add a docstring for preresolve, and update the one for resolve.
> >
> >
> > This seems to make sense without the three patches before it, so I've
> > pushed it to the clowncopter.
> >
> > I'd prefer to see the other four patches squashed (they do very
> > similar things and it seems unlikely that only one of them would be
> > reverted) and be sent out with later patches that use the added return
> > value. Looking at only these four patches, they just add dead code and
> > I can't say whether it seems like that code will be useful later.
>
> All four squashed into one, you mean?
>
> These are preparatory patches, but the series that actually does things
> is long and complicated enough that I'd like to get all this landed first.
>
> - Siddharth
>
>
> >
> >     diff --git a/mercurial/merge.py b/mercurial/merge.py
> >     --- a/mercurial/merge.py
> >     +++ b/mercurial/merge.py
> >     @@ -397,10 +397,15 @@ class mergestate(object):
> >              return complete, r
> >
> >          def preresolve(self, dfile, wctx, labels=None):
> >     +        """run premerge process for dfile
> >     +
> >     +        Returns whether the merge is complete, and the exit code."""
> >              return self._resolve(True, dfile, wctx, labels=labels)
> >
> >          def resolve(self, dfile, wctx, labels=None):
> >     -        """rerun merge process for file path `dfile`"""
> >     +        """run merge process (assuming premerge was run) for dfile
> >     +
> >     +        Returns the exit code of the merge."""
> >              return self._resolve(False, dfile, wctx, labels=labels)[1]
> >
> >      def _checkunknownfile(repo, wctx, mctx, f, f2=None):
> >     _______________________________________________
> >     Mercurial-devel mailing list
> >     Mercurial-devel@selenic.com <mailto:Mercurial-devel@selenic.com>
> >     https://selenic.com/mailman/listinfo/mercurial-devel
> >
> >
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@selenic.com
> > https://selenic.com/mailman/listinfo/mercurial-devel
>
>
Pierre-Yves David - Nov. 6, 2015, 11:03 p.m.
On 11/05/2015 09:58 PM, Martin von Zweigbergk wrote:
> Yes, all four squashed. If you squash those four, will that plus the
> next four-or-so patches give enough context for me to understand what
> the added return value is for? If not, could you send a link to a repo
> to pull from so I can see?

Not super excited to see them squashed, the patches are not big, but 
large enought and repetitive enough that it would be easy to typo 
something. Fine grained change will help bisecting. If sid0 went through 
the effort to make them distinct, lets keep it that way and take 
advantage of it if necessary.

+1 for seeing the whole series somewhere

Patch

diff --git a/mercurial/merge.py b/mercurial/merge.py
--- a/mercurial/merge.py
+++ b/mercurial/merge.py
@@ -397,10 +397,15 @@  class mergestate(object):
         return complete, r
 
     def preresolve(self, dfile, wctx, labels=None):
+        """run premerge process for dfile
+
+        Returns whether the merge is complete, and the exit code."""
         return self._resolve(True, dfile, wctx, labels=labels)
 
     def resolve(self, dfile, wctx, labels=None):
-        """rerun merge process for file path `dfile`"""
+        """run merge process (assuming premerge was run) for dfile
+
+        Returns the exit code of the merge."""
         return self._resolve(False, dfile, wctx, labels=labels)[1]
 
 def _checkunknownfile(repo, wctx, mctx, f, f2=None):