| Submitter | Gregory Szorc |
|---|---|
| Date | May 9, 2014, 1:46 a.m. |
| Message ID | <a5e623de6557ea8854ce.1399599982@vm-ubuntu-main.gateway.sonic.net> |
| Download | mbox | patch |
| Permalink | /patch/4692/ |
| State | Accepted |
| Headers | show |
Comments
On 05/08/2014 06:46 PM, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1397873312 25200 > # Fri Apr 18 19:08:32 2014 -0700 > # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a > # Parent 6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3 > resolve: abort when not applicable > > The resolve command is only relevant when mergestate is present. > This patch will make resolve abort when no mergestate is present. > > This change will let people know when they are using resolve when they > shouldn't be. This change will let people know when their use of resolve > doesn't do anything. > > Previously, |hg resolve -m| would allow mergestate to be created. This > patch now forbids that. Strictly speaking, this is backwards > incompatible. The author of this patch believes creating mergestate via > resolve doesn't make much sense and this side-effect was unintended. This change make sense. I added a (BC) flag in the title to flag the backward compatibility breackage. I'm not super fan of the message. but I'll let native speaker bikeshed it. I've queued this series to the clowncopter repo. I expect message fixup in follow up patches.
On 05/09/2014 03:46 AM, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1397873312 25200 > # Fri Apr 18 19:08:32 2014 -0700 > # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a > # Parent 6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3 > resolve: abort when not applicable > > The resolve command is only relevant when mergestate is present. > This patch will make resolve abort when no mergestate is present. > > This change will let people know when they are using resolve when they > shouldn't be. This change will let people know when their use of resolve > doesn't do anything. > > Previously, |hg resolve -m| would allow mergestate to be created. This > patch now forbids that. Strictly speaking, this is backwards > incompatible. The author of this patch believes creating mergestate via > resolve doesn't make much sense and this side-effect was unintended. > > diff --git a/mercurial/commands.py b/mercurial/commands.py > --- a/mercurial/commands.py > +++ b/mercurial/commands.py > @@ -4926,8 +4926,12 @@ def resolve(ui, repo, *pats, **opts): > raise util.Abort(_('no files or directories specified; ' > 'use --all to remerge all files')) > > ms = mergemod.mergestate(repo) > + > + if not ms.active(): > + raise util.Abort(_('resolve command not applicable when not merging')) > + > m = scmutil.match(repo[None], pats, opts) > ret = 0 > > for f in ms: > diff --git a/mercurial/merge.py b/mercurial/merge.py > --- a/mercurial/merge.py > +++ b/mercurial/merge.py > @@ -175,8 +175,20 @@ class mergestate(object): > if err.errno != errno.ENOENT: > raise > return records > > + def active(self): > + """Whether mergestate is active. > + > + Returns True if there appears to be mergestate. This is a rough proxy > + for "is a merge in progress." > + """ > + # Check local variables before looking at filesystem for performance > + # reasons. > + return bool(self._local) or bool(self._state) or \ > + self._repo.opener.exists(self.statepathv1) or \ > + self._repo.opener.exists(self.statepathv2) Did you have a response to my question on http://selenic.com/pipermail/mercurial-devel/2014-May/058585.html : Will there ever be a (relevant) case where _local doesn't give the full answer? A mergestate without a _local (and _other) is no mergestate. It seems to me like this code is more complex than necessary. /Mads
On 5/9/2014 2:54 AM, Mads Kiilerich wrote: > On 05/09/2014 03:46 AM, Gregory Szorc wrote: >> # HG changeset patch >> # User Gregory Szorc <gregory.szorc@gmail.com> >> # Date 1397873312 25200 >> # Fri Apr 18 19:08:32 2014 -0700 >> # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a >> # Parent 6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3 >> resolve: abort when not applicable >> >> The resolve command is only relevant when mergestate is present. >> This patch will make resolve abort when no mergestate is present. >> >> This change will let people know when they are using resolve when they >> shouldn't be. This change will let people know when their use of resolve >> doesn't do anything. >> >> Previously, |hg resolve -m| would allow mergestate to be created. This >> patch now forbids that. Strictly speaking, this is backwards >> incompatible. The author of this patch believes creating mergestate via >> resolve doesn't make much sense and this side-effect was unintended. >> >> diff --git a/mercurial/commands.py b/mercurial/commands.py >> --- a/mercurial/commands.py >> +++ b/mercurial/commands.py >> @@ -4926,8 +4926,12 @@ def resolve(ui, repo, *pats, **opts): >> raise util.Abort(_('no files or directories specified; ' >> 'use --all to remerge all files')) >> ms = mergemod.mergestate(repo) >> + >> + if not ms.active(): >> + raise util.Abort(_('resolve command not applicable when not >> merging')) >> + >> m = scmutil.match(repo[None], pats, opts) >> ret = 0 >> for f in ms: >> diff --git a/mercurial/merge.py b/mercurial/merge.py >> --- a/mercurial/merge.py >> +++ b/mercurial/merge.py >> @@ -175,8 +175,20 @@ class mergestate(object): >> if err.errno != errno.ENOENT: >> raise >> return records >> + def active(self): >> + """Whether mergestate is active. >> + >> + Returns True if there appears to be mergestate. This is a >> rough proxy >> + for "is a merge in progress." >> + """ >> + # Check local variables before looking at filesystem for >> performance >> + # reasons. >> + return bool(self._local) or bool(self._state) or \ >> + self._repo.opener.exists(self.statepathv1) or \ >> + self._repo.opener.exists(self.statepathv2) > > Did you have a response to my question on > http://selenic.com/pipermail/mercurial-devel/2014-May/058585.html : > > Will there ever be a (relevant) case where _local doesn't give the full > answer? A mergestate without a _local (and _other) is no mergestate. > > It seems to me like this code is more complex than necessary. I don't know! I concede my knowledge of everything merge code is somewhat shallow. There is a single test that touches a mergestate file and puts no content in it (simulating a process crash or some such). Not changing behavior of that test is why I put the opener.exists() bits in there. I'm not sure if that was the proper course.
On 05/09/2014 02:54 AM, Mads Kiilerich wrote: > On 05/09/2014 03:46 AM, Gregory Szorc wrote: >> # HG changeset patch >> # User Gregory Szorc <gregory.szorc@gmail.com> >> # Date 1397873312 25200 >> # Fri Apr 18 19:08:32 2014 -0700 >> # Node ID a5e623de6557ea8854cefc108d5b069adb0f737a >> # Parent 6d5b402aae2d697b93b19ccf8f4d0245c4fc29d3 >> resolve: abort when not applicable >> >> The resolve command is only relevant when mergestate is present. >> This patch will make resolve abort when no mergestate is present. >> >> This change will let people know when they are using resolve when they >> shouldn't be. This change will let people know when their use of resolve >> doesn't do anything. >> >> Previously, |hg resolve -m| would allow mergestate to be created. This >> patch now forbids that. Strictly speaking, this is backwards >> incompatible. The author of this patch believes creating mergestate via >> resolve doesn't make much sense and this side-effect was unintended. >> >> diff --git a/mercurial/commands.py b/mercurial/commands.py >> --- a/mercurial/commands.py >> +++ b/mercurial/commands.py >> @@ -4926,8 +4926,12 @@ def resolve(ui, repo, *pats, **opts): >> raise util.Abort(_('no files or directories specified; ' >> 'use --all to remerge all files')) >> ms = mergemod.mergestate(repo) >> + >> + if not ms.active(): >> + raise util.Abort(_('resolve command not applicable when not >> merging')) >> + >> m = scmutil.match(repo[None], pats, opts) >> ret = 0 >> for f in ms: >> diff --git a/mercurial/merge.py b/mercurial/merge.py >> --- a/mercurial/merge.py >> +++ b/mercurial/merge.py >> @@ -175,8 +175,20 @@ class mergestate(object): >> if err.errno != errno.ENOENT: >> raise >> return records >> + def active(self): >> + """Whether mergestate is active. >> + >> + Returns True if there appears to be mergestate. This is a >> rough proxy >> + for "is a merge in progress." >> + """ >> + # Check local variables before looking at filesystem for >> performance >> + # reasons. >> + return bool(self._local) or bool(self._state) or \ >> + self._repo.opener.exists(self.statepathv1) or \ >> + self._repo.opener.exists(self.statepathv2) > > Did you have a response to my question on > http://selenic.com/pipermail/mercurial-devel/2014-May/058585.html : > > Will there ever be a (relevant) case where _local doesn't give the full > answer? A mergestate without a _local (and _other) is no mergestate. > > It seems to me like this code is more complex than necessary. Woops, I missed the fact your started reviewing this series. This change is already crewed. so please choose one of 1. request unqueuing 2. request followup comit
On 05/09/2014 10:23 AM, Pierre-Yves David wrote: > On 05/09/2014 02:54 AM, Mads Kiilerich wrote: >> On 05/09/2014 03:46 AM, Gregory Szorc wrote: […] >>> --- a/mercurial/merge.py >>> +++ b/mercurial/merge.py >>> @@ -175,8 +175,20 @@ class mergestate(object): >>> if err.errno != errno.ENOENT: >>> raise >>> return records >>> + def active(self): >>> + """Whether mergestate is active. >>> + >>> + Returns True if there appears to be mergestate. This is a >>> rough proxy >>> + for "is a merge in progress." >>> + """ >>> + # Check local variables before looking at filesystem for >>> performance >>> + # reasons. >>> + return bool(self._local) or bool(self._state) or \ >>> + self._repo.opener.exists(self.statepathv1) or \ >>> + self._repo.opener.exists(self.statepathv2) >> >> Did you have a response to my question on >> http://selenic.com/pipermail/mercurial-devel/2014-May/058585.html : >> >> Will there ever be a (relevant) case where _local doesn't give the full >> answer? A mergestate without a _local (and _other) is no mergestate. >> >> It seems to me like this code is more complex than necessary. > > Woops, I missed the fact your started reviewing this series. > > This change is already crewed. so please choose one of > > 1. request unqueuing > 2. request followup commit So this changeset is now public. A followup commit would be nice.
On 5/15/14, 12:31 AM, Pierre-Yves David wrote: > > > On 05/09/2014 10:23 AM, Pierre-Yves David wrote: >> On 05/09/2014 02:54 AM, Mads Kiilerich wrote: >>> On 05/09/2014 03:46 AM, Gregory Szorc wrote: > […] >>>> --- a/mercurial/merge.py >>>> +++ b/mercurial/merge.py >>>> @@ -175,8 +175,20 @@ class mergestate(object): >>>> if err.errno != errno.ENOENT: >>>> raise >>>> return records >>>> + def active(self): >>>> + """Whether mergestate is active. >>>> + >>>> + Returns True if there appears to be mergestate. This is a >>>> rough proxy >>>> + for "is a merge in progress." >>>> + """ >>>> + # Check local variables before looking at filesystem for >>>> performance >>>> + # reasons. >>>> + return bool(self._local) or bool(self._state) or \ >>>> + self._repo.opener.exists(self.statepathv1) or \ >>>> + self._repo.opener.exists(self.statepathv2) >>> >>> Did you have a response to my question on >>> http://selenic.com/pipermail/mercurial-devel/2014-May/058585.html : >>> >>> Will there ever be a (relevant) case where _local doesn't give the full >>> answer? A mergestate without a _local (and _other) is no mergestate. >>> >>> It seems to me like this code is more complex than necessary. >> >> Woops, I missed the fact your started reviewing this series. >> >> This change is already crewed. so please choose one of >> >> 1. request unqueuing >> 2. request followup commit > > So this changeset is now public. A followup commit would be nice. > I don't think we'll ever have _local without _other. The patch as committed reflects this.
On 05/16/2014 11:07 PM, Gregory Szorc wrote: > On 5/15/14, 12:31 AM, Pierre-Yves David wrote: >> >> >> On 05/09/2014 10:23 AM, Pierre-Yves David wrote: >>> On 05/09/2014 02:54 AM, Mads Kiilerich wrote: >>>> On 05/09/2014 03:46 AM, Gregory Szorc wrote: >> […] >>>>> --- a/mercurial/merge.py >>>>> +++ b/mercurial/merge.py >>>>> @@ -175,8 +175,20 @@ class mergestate(object): >>>>> if err.errno != errno.ENOENT: >>>>> raise >>>>> return records >>>>> + def active(self): >>>>> + """Whether mergestate is active. >>>>> + >>>>> + Returns True if there appears to be mergestate. This is a >>>>> rough proxy >>>>> + for "is a merge in progress." >>>>> + """ >>>>> + # Check local variables before looking at filesystem for >>>>> performance >>>>> + # reasons. >>>>> + return bool(self._local) or bool(self._state) or \ >>>>> + self._repo.opener.exists(self.statepathv1) or \ >>>>> + self._repo.opener.exists(self.statepathv2) >>>> >>>> Did you have a response to my question on >>>> http://selenic.com/pipermail/mercurial-devel/2014-May/058585.html : >>>> >>>> Will there ever be a (relevant) case where _local doesn't give the >>>> full >>>> answer? A mergestate without a _local (and _other) is no mergestate. >>>> >>>> It seems to me like this code is more complex than necessary. >>> >>> Woops, I missed the fact your started reviewing this series. >>> >>> This change is already crewed. so please choose one of >>> >>> 1. request unqueuing >>> 2. request followup commit >> >> So this changeset is now public. A followup commit would be nice. >> > > I don't think we'll ever have _local without _other. The patch as > committed reflects this. That do not answer the question, "Will there ever be a (relevant) case where _local doesn't give the full answer?" Checking "everything" makes it look like we have no idea what is going on in other code in the same class and seems like something similar to a layering violation. I do not buy the performance argument in this place. If file existence is what you want, checking file existence is very cheap. It is not worth the complexity of looking at variables that doesn't give exactly the same information anyway. But I will argue that you have to define "ongoing merge that can be resolved" as either something that has a local parent or as something that has things to resolve and thus has a non-empty state. Checking more than that will be unmaintainable code. /Mads
Patch
diff --git a/mercurial/commands.py b/mercurial/commands.py --- a/mercurial/commands.py +++ b/mercurial/commands.py @@ -4926,8 +4926,12 @@ def resolve(ui, repo, *pats, **opts): raise util.Abort(_('no files or directories specified; ' 'use --all to remerge all files')) ms = mergemod.mergestate(repo) + + if not ms.active(): + raise util.Abort(_('resolve command not applicable when not merging')) + m = scmutil.match(repo[None], pats, opts) ret = 0 for f in ms: diff --git a/mercurial/merge.py b/mercurial/merge.py --- a/mercurial/merge.py +++ b/mercurial/merge.py @@ -175,8 +175,20 @@ class mergestate(object): if err.errno != errno.ENOENT: raise return records + def active(self): + """Whether mergestate is active. + + Returns True if there appears to be mergestate. This is a rough proxy + for "is a merge in progress." + """ + # Check local variables before looking at filesystem for performance + # reasons. + return bool(self._local) or bool(self._state) or \ + self._repo.opener.exists(self.statepathv1) or \ + self._repo.opener.exists(self.statepathv2) + def commit(self): """Write current state on disk (if necessary)""" if self._dirty: records = [] diff --git a/tests/test-histedit-non-commute-abort.t b/tests/test-histedit-non-commute-abort.t --- a/tests/test-histedit-non-commute-abort.t +++ b/tests/test-histedit-non-commute-abort.t @@ -84,8 +84,10 @@ abort the edit 2 files updated, 0 files merged, 0 files removed, 0 files unresolved log after abort $ hg resolve -l + abort: resolve command not applicable when not merging + [255] $ hg log --graph @ changeset: 6:bfa474341cc9 | tag: tip | user: test diff --git a/tests/test-resolve.t b/tests/test-resolve.t --- a/tests/test-resolve.t +++ b/tests/test-resolve.t @@ -36,11 +36,13 @@ resolve the failure $ echo resolved > file $ hg resolve -m file $ hg commit -m 'resolved' -resolve -l, should be empty +resolve -l should error since no merge in progress $ hg resolve -l + abort: resolve command not applicable when not merging + [255] test crashed merge with empty mergestate $ mkdir .hg/merge diff --git a/tests/test-shelve.t b/tests/test-shelve.t --- a/tests/test-shelve.t +++ b/tests/test-shelve.t @@ -261,8 +261,10 @@ abort the unshelve and be happy date: Thu Jan 01 00:00:00 1970 +0000 summary: second $ hg resolve -l + abort: resolve command not applicable when not merging + [255] $ hg status A foo/foo ? a/a.orig