| 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
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 >
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
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 > >
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):