| Submitter | via Mercurial-devel |
|---|---|
| Date | Feb. 14, 2017, 1:07 a.m. |
| Message ID | <75e5a492d7f69420a554.1487034442@martinvonz.mtv.corp.google.com> |
| Download | mbox | patch |
| Permalink | /patch/18464/ |
| State | Superseded |
| Headers | show |
Comments
(+mercurial-devel) On Tue, Feb 14, 2017 at 10:48 AM, Gábor STEFANIK <Gabor.STEFANIK@nng.com> wrote: >> > > > -------------------------------------------------------------------------- > This message, including its attachments, is confidential. For more information please read NNG's email policy here: > http://www.nng.com/emailpolicy/ > By responding to this email you accept the email policy. > > > -----Original Message----- >> From: Mercurial-devel [mailto:mercurial-devel-bounces@mercurial-scm.org] >> On Behalf Of Martin von Zweigbergk via Mercurial-devel >> Sent: Tuesday, February 14, 2017 2:07 AM >> To: mercurial-devel@mercurial-scm.org >> Subject: [PATCH 5 of 6] update: learn --merge to allow merging across >> topological branches (issue5125) >> >> # HG changeset patch >> # User Martin von Zweigbergk <martinvonz@google.com> >> # Date 1487019517 28800 >> # Mon Feb 13 12:58:37 2017 -0800 >> # Node ID 75e5a492d7f69420a554fe498ae24060c755b09f >> # Parent 59e2d3da607cc09ebe133ab199fc4f343d74e1e6 >> update: learn --merge to allow merging across topological branches >> (issue5125) >> >> diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/commands.py >> --- a/mercurial/commands.py Mon Feb 13 15:04:46 2017 -0800 >> +++ b/mercurial/commands.py Mon Feb 13 12:58:37 2017 -0800 >> @@ -6471,12 +6471,13 @@ >> @command('^update|up|checkout|co', >> [('C', 'clean', None, _('discard uncommitted changes (no backup)')), >> ('c', 'check', None, _('require clean working directory')), >> + ('m', 'merge', None, _('merge local changes')), >> ('d', 'date', '', _('tipmost revision matching date'), _('DATE')), >> ('r', 'rev', '', _('revision'), _('REV')) >> ] + mergetoolopts, >> - _('[-C|-c] [-d DATE] [[-r] REV]')) >> + _('[-C|-c|-m] [-d DATE] [[-r] REV]')) >> def update(ui, repo, node=None, rev=None, clean=False, date=None, >> check=False, >> - tool=None): >> + merge=None, tool=None): >> """update working directory (or switch revisions) >> >> Update the repository's working directory to the specified >> @@ -6498,7 +6499,7 @@ >> The following rules apply when the working directory contains >> uncommitted changes: >> >> - 1. If neither -c/--check nor -C/--clean is specified, and if >> + 1. If none of -c/--check, -C/--clean, or -m/--merge is specified, and if >> the requested changeset is an ancestor or descendant of >> the working directory's parent, the uncommitted changes >> are merged into the requested changeset and the merged >> @@ -6507,10 +6508,14 @@ >> branch), the update is aborted and the uncommitted changes >> are preserved. >> >> - 2. With the -c/--check option, the update is aborted and the >> + 2. With the -m/--merge option, the update is allowed even if the >> + requested changeset is not an ancestor or descendant of >> + the working directory's parent. >> + >> + 3. With the -c/--check option, the update is aborted and the >> uncommitted changes are preserved. >> >> - 3. With the -C/--clean option, uncommitted changes are discarded and >> + 4. With the -C/--clean option, uncommitted changes are discarded and >> the working directory is updated to the requested changeset. >> >> To cancel an uncommitted merge (and lose your changes), use >> @@ -6535,8 +6540,15 @@ >> if date and rev is not None: >> raise error.Abort(_("you can't specify a revision and a date")) >> >> - if check and clean: >> - raise error.Abort(_("cannot specify both -c/--check and -C/--clean")) >> + if len([x for x in (check, clean , merge) if x]) > 1: >> + raise error.Abort(_("can only specify one of -c/--check, -C/--clean, " >> + "and -m/merge")) >> + >> + updatecheck = 'linear' >> + if check: >> + updatecheck = 'abort' >> + elif merge: >> + updatecheck = None > > I don't really like the use of None to indicate "always merge". > I would strongly prefer updatecheck = 'merge', with None instead > representing a default or "unset" case. Makes sense. Since the check that's done is not about merging, I'll call it 'none' instead, if you don't mind. > > This makes setting the default via a config option (as in your 6th patch) > a lot safer and easier - see below. > >> >> with repo.wlock(): >> cmdutil.clearunfinished(repo) >> @@ -6550,7 +6562,8 @@ >> >> repo.ui.setconfig('ui', 'forcemerge', tool, 'update') >> >> - return hg.updatetotally(ui, repo, rev, brev, clean=clean, check=check) >> + return hg.updatetotally(ui, repo, rev, brev, clean=clean, >> + updatecheck=updatecheck) >> >> @command('verify', []) >> def verify(ui, repo): >> diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/hg.py >> --- a/mercurial/hg.py Mon Feb 13 15:04:46 2017 -0800 >> +++ b/mercurial/hg.py Mon Feb 13 12:58:37 2017 -0800 >> @@ -681,18 +681,19 @@ >> repo.ui.status(_("%d files updated, %d files merged, " >> "%d files removed, %d files unresolved\n") % stats) >> >> -def updaterepo(repo, node, overwrite): >> +def updaterepo(repo, node, overwrite, updatecheck=None): >> """Update the working directory to node. >> >> When overwrite is set, changes are clobbered, merged else >> >> returns stats (see pydoc mercurial.merge.applyupdates)""" >> return mergemod.update(repo, node, False, overwrite, >> - labels=['working copy', 'destination']) >> + labels=['working copy', 'destination'], >> + updatecheck=updatecheck) >> >> -def update(repo, node, quietempty=False): >> - """update the working directory to node, merging linear changes""" >> - stats = updaterepo(repo, node, False) >> +def update(repo, node, quietempty=False, updatecheck=None): >> + """update the working directory to node""" >> + stats = updaterepo(repo, node, False, updatecheck=updatecheck) >> _showstats(repo, stats, quietempty) >> if stats[3]: >> repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n")) >> @@ -712,7 +713,8 @@ >> # naming conflict in updatetotally() >> _clean = clean >> >> -def updatetotally(ui, repo, checkout, brev, clean=False, check=False): >> +def updatetotally(ui, repo, checkout, brev, clean=False, >> + updatecheck='linear'): > > Please make this updatecheck=None, and set it to 'linear' inside merge.update() > explicitly if None was passed to that function. Will do. > > This way, in patch 6, you can red the config option inside merge.update(), and let > other callers (primarily extensions) also automatically follow the config option. I don't think it's a good idea for the low-level merge.update() to read the config. That function is called e.g. by rebase and graft and they should not have their behavior changed by this config. > >> """Update the working directory with extra care for non-file components >> >> This takes care of non-file components below: >> @@ -724,7 +726,14 @@ >> :checkout: to which revision the working directory is updated >> :brev: a name, which might be a bookmark to be activated after updating >> :clean: whether changes in the working directory can be discarded >> - :check: whether changes in the working directory should be checked >> + :updatecheck: how to deal with a dirty working directory >> + >> + Valid values for updatecheck are: >> + >> + * abort: abort if the working directory is dirty >> + * None: don't check (merge working directory changes into destination) >> + * linear: check that update is linear before merging working directory >> + changes into destination >> >> This returns whether conflict is detected at updating or not. >> """ >> @@ -739,9 +748,10 @@ >> if clean: >> ret = _clean(repo, checkout) >> else: >> - if check: >> + if updatecheck == 'abort': >> cmdutil.bailifchanged(repo, merge=False) >> - ret = _update(repo, checkout) >> + updatecheck = None >> + ret = _update(repo, checkout, updatecheck=updatecheck) >> >> if not ret and movemarkfrom: >> if movemarkfrom == repo['.'].node(): >> diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/merge.py >> --- a/mercurial/merge.py Mon Feb 13 15:04:46 2017 -0800 >> +++ b/mercurial/merge.py Mon Feb 13 12:58:37 2017 -0800 >> @@ -1444,7 +1444,8 @@ >> repo.dirstate.normal(f) >> >> def update(repo, node, branchmerge, force, ancestor=None, >> - mergeancestor=False, labels=None, matcher=None, >> mergeforce=False): >> + mergeancestor=False, labels=None, matcher=None, >> mergeforce=False, >> + updatecheck='linear'): > > Again, default to None here, and explicitly set to 'linear' > (and later set it based on config) if it's None. Will do. > >> """ >> Perform a merge between the working directory and the given node >> >> @@ -1494,6 +1495,7 @@ >> # This functon used to find the default destination if node was None, but >> # that's now in destutil.py. >> assert node is not None >> + assert updatecheck in (None, 'linear') >> # If we're doing a partial update, we need to skip updating >> # the dirstate, so make a note of any partial-ness to the >> # update here. >> @@ -1550,7 +1552,8 @@ >> repo.hook('update', parent1=xp2, parent2='', error=0) >> return 0, 0, 0, 0 >> >> - if pas not in ([p1], [p2]): # nonlinear >> + if (updatecheck == 'linear' and >> + pas not in ([p1], [p2])): # nonlinear >> dirty = wc.dirty(missing=True) >> if dirty: >> # Branching is a bit strange to ensure we do the minimal >> diff -r 59e2d3da607c -r 75e5a492d7f6 tests/test-completion.t >> --- a/tests/test-completion.t Mon Feb 13 15:04:46 2017 -0800 >> +++ b/tests/test-completion.t Mon Feb 13 12:58:37 2017 -0800 >> @@ -223,7 +223,7 @@ >> serve: accesslog, daemon, daemon-postexec, errorlog, port, address, >> prefix, name, web-conf, webdir-conf, pid-file, stdio, cmdserver, templates, >> style, ipv6, certificate >> status: all, modified, added, removed, deleted, clean, unknown, ignored, >> no-status, copies, print0, rev, change, include, exclude, subrepos, template >> summary: remote >> - update: clean, check, date, rev, tool >> + update: clean, check, merge, date, rev, tool >> addremove: similarity, subrepos, include, exclude, dry-run >> archive: no-decode, prefix, rev, type, subrepos, include, exclude >> backout: merge, commit, no-commit, parent, rev, edit, tool, include, >> exclude, message, logfile, date, user >> diff -r 59e2d3da607c -r 75e5a492d7f6 tests/test-update-branches.t >> --- a/tests/test-update-branches.t Mon Feb 13 15:04:46 2017 -0800 >> +++ b/tests/test-update-branches.t Mon Feb 13 12:58:37 2017 -0800 >> @@ -160,6 +160,16 @@ >> parent=1 >> M foo >> >> + $ revtest '-m dirty linear' dirty 1 2 -m >> + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved >> + parent=2 >> + M foo >> + >> + $ revtest '-m dirty cross' dirty 3 4 -m >> + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved >> + parent=4 >> + M foo >> + >> $ revtest '-c dirtysub linear' dirtysub 1 2 -c >> abort: uncommitted changes in subrepository 'sub' >> parent=1 >> @@ -171,7 +181,17 @@ >> parent=2 >> >> $ revtest '-cC dirty linear' dirty 1 2 -cC >> - abort: cannot specify both -c/--check and -C/--clean >> + abort: can only specify one of -c/--check, -C/--clean, and -m/merge >> + parent=1 >> + M foo >> + >> + $ revtest '-mc dirty linear' dirty 1 2 -mc >> + abort: can only specify one of -c/--check, -C/--clean, and -m/merge >> + parent=1 >> + M foo >> + >> + $ revtest '-mC dirty linear' dirty 1 2 -mC >> + abort: can only specify one of -c/--check, -C/--clean, and -m/merge >> parent=1 >> M foo >> >> _______________________________________________ >> Mercurial-devel mailing list >> Mercurial-devel@mercurial-scm.org >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
On Wed, 15 Feb 2017 13:29:30 +0000, Gábor STEFANIK wrote: > > >> -def updatetotally(ui, repo, checkout, brev, clean=False, check=False): > > >> +def updatetotally(ui, repo, checkout, brev, clean=False, > > >> + updatecheck='linear'): > > > > > > Please make this updatecheck=None, and set it to 'linear' inside > > > merge.update() explicitly if None was passed to that function. > > > > Will do. > > > > > > > > This way, in patch 6, you can red the config option inside > > > merge.update(), and let other callers (primarily extensions) also > > automatically follow the config option. > > > > I don't think it's a good idea for the low-level merge.update() to read the > > config. That function is called e.g. by rebase and graft and they should not > > have their behavior changed by this config. I tend to agree with Martin, low-level functions should have less dependency on config. Instead, maybe we could add a function that builds options from ui (like patch.diffopts), but I don't know if that's appropriate here. > I forgot to add, I'm particularly concerned about the "guestrepo" extension, > which we use extensively at NNG. IIRC in the "grupdate" command, it doesn't > use commands.update, and so changing the config option causes "update" > and "grupdate" to behave differently. > > Also, care must be taken about subrepositories. Not sure if our subrepo update > code calls command.update - if not, we may even end up with a situation > where "hg update" in a repository with subrepos with ui.updatecheck="merge" > will do nonlinear updates with a dirty main repo, but not with a dirty subrepo. IIRC, updating including subrepos isn't a simple cascading operation. Perhaps users would be asked how to process changes in subrepos? > In fact, if we introduce a config option, but only use it in commands.update(), > we should make the lower functions error out or at least warn if called without > explicit updatecheck, and without other options (e.g. branchmerge) that override > linearity checks. This way, we at least catch extensions that don't follow the > config option, but try to rely on merge.update()'s linearity checks. Good point.
On Wed, Feb 15, 2017 at 6:22 AM, Yuya Nishihara <yuya@tcha.org> wrote: > On Wed, 15 Feb 2017 13:29:30 +0000, Gábor STEFANIK wrote: >> > >> -def updatetotally(ui, repo, checkout, brev, clean=False, check=False): >> > >> +def updatetotally(ui, repo, checkout, brev, clean=False, >> > >> + updatecheck='linear'): >> > > >> > > Please make this updatecheck=None, and set it to 'linear' inside >> > > merge.update() explicitly if None was passed to that function. >> > >> > Will do. >> > >> > > >> > > This way, in patch 6, you can red the config option inside >> > > merge.update(), and let other callers (primarily extensions) also >> > automatically follow the config option. >> > >> > I don't think it's a good idea for the low-level merge.update() to read the >> > config. That function is called e.g. by rebase and graft and they should not >> > have their behavior changed by this config. > > I tend to agree with Martin, low-level functions should have less dependency > on config. Instead, maybe we could add a function that builds options from > ui (like patch.diffopts), but I don't know if that's appropriate here. > >> I forgot to add, I'm particularly concerned about the "guestrepo" extension, >> which we use extensively at NNG. IIRC in the "grupdate" command, it doesn't >> use commands.update, and so changing the config option causes "update" >> and "grupdate" to behave differently. >> >> Also, care must be taken about subrepositories. Not sure if our subrepo update >> code calls command.update - if not, we may even end up with a situation >> where "hg update" in a repository with subrepos with ui.updatecheck="merge" >> will do nonlinear updates with a dirty main repo, but not with a dirty subrepo. > > IIRC, updating including subrepos isn't a simple cascading operation. Perhaps > users would be asked how to process changes in subrepos? > >> In fact, if we introduce a config option, but only use it in commands.update(), >> we should make the lower functions error out or at least warn if called without >> explicit updatecheck, and without other options (e.g. branchmerge) that override >> linearity checks. This way, we at least catch extensions that don't follow the >> config option, but try to rely on merge.update()'s linearity checks. > > Good point. Yeah, I agree with that last point. I tried it and there are several places that fail. For example, clone, rebase, and mq all call hg.update[repo](). AFAICT, there should never be any changes where it's being called, so maybe that should be setting force=True to overwrite. Or maybe we should teach merge.update() to accept updatecheck='abort' and make these callers pass that value. Regardless, since what I'm adding is an experimental config, I'd prefer to make merge.update() default to 'linear' for now and I'll add a TODO to add the check you mention. I don't have time right now to clean up all the other callers, and I'd still like to be able to make progress on this topic.
On Wed, Feb 15, 2017 at 10:35 AM, Gábor STEFANIK <Gabor.STEFANIK@nng.com> wrote: >> > > > -------------------------------------------------------------------------- > This message, including its attachments, is confidential. For more information please read NNG's email policy here: > http://www.nng.com/emailpolicy/ > By responding to this email you accept the email policy. > > > -----Original Message----- >> From: Martin von Zweigbergk [mailto:martinvonz@google.com] >> Sent: Wednesday, February 15, 2017 7:26 PM >> To: Yuya Nishihara <yuya@tcha.org> >> Cc: Gábor STEFANIK <Gabor.STEFANIK@nng.com>; Mercurial-devel >> <mercurial-devel@mercurial-scm.org> >> Subject: Re: [PATCH 5 of 6] update: learn --merge to allow merging across >> topological branches (issue5125) >> >> On Wed, Feb 15, 2017 at 6:22 AM, Yuya Nishihara <yuya@tcha.org> wrote: >> > On Wed, 15 Feb 2017 13:29:30 +0000, Gábor STEFANIK wrote: >> >> > >> -def updatetotally(ui, repo, checkout, brev, clean=False, >> check=False): >> >> > >> +def updatetotally(ui, repo, checkout, brev, clean=False, >> >> > >> + updatecheck='linear'): >> >> > > >> >> > > Please make this updatecheck=None, and set it to 'linear' inside >> >> > > merge.update() explicitly if None was passed to that function. >> >> > >> >> > Will do. >> >> > >> >> > > >> >> > > This way, in patch 6, you can red the config option inside >> >> > > merge.update(), and let other callers (primarily extensions) also >> >> > automatically follow the config option. >> >> > >> >> > I don't think it's a good idea for the low-level merge.update() to >> >> > read the config. That function is called e.g. by rebase and graft >> >> > and they should not have their behavior changed by this config. >> > >> > I tend to agree with Martin, low-level functions should have less >> > dependency on config. Instead, maybe we could add a function that >> > builds options from ui (like patch.diffopts), but I don't know if that's >> appropriate here. >> > >> >> I forgot to add, I'm particularly concerned about the "guestrepo" >> >> extension, which we use extensively at NNG. IIRC in the "grupdate" >> >> command, it doesn't use commands.update, and so changing the config >> option causes "update" >> >> and "grupdate" to behave differently. >> >> >> >> Also, care must be taken about subrepositories. Not sure if our >> >> subrepo update code calls command.update - if not, we may even end up >> >> with a situation where "hg update" in a repository with subrepos with >> ui.updatecheck="merge" >> >> will do nonlinear updates with a dirty main repo, but not with a dirty >> subrepo. >> > >> > IIRC, updating including subrepos isn't a simple cascading operation. >> > Perhaps users would be asked how to process changes in subrepos? >> > >> >> In fact, if we introduce a config option, but only use it in >> >> commands.update(), we should make the lower functions error out or at >> >> least warn if called without explicit updatecheck, and without other >> >> options (e.g. branchmerge) that override linearity checks. This way, >> >> we at least catch extensions that don't follow the config option, but try to >> rely on merge.update()'s linearity checks. >> > >> > Good point. >> >> Yeah, I agree with that last point. I tried it and there are several places that >> fail. For example, clone, rebase, and mq all call hg.update[repo](). AFAICT, >> there should never be any changes where it's being called, so maybe that >> should be setting force=True to overwrite. Or maybe we should teach >> merge.update() to accept updatecheck='abort' and make these callers pass >> that value. >> Regardless, since what I'm adding is an experimental config, I'd prefer to >> make merge.update() default to 'linear' for now and I'll add a TODO to add >> the check you mention. I don't have time right now to clean up all the other >> callers, and I'd still like to be able to make progress on this topic. > > These other callers IIRC call with options that already disable linearity checks. > > We should only fail if we are in a normal update scenario with linearity checks > enabled, but no check strategy was provided. The ones I mention do not disable linearity checks (not in all the places anyway). Here are some examples: https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/hg.py#l667 https://www.mercurial-scm.org/repo/hg/file/tip/hgext/rebase.py#l485 https://www.mercurial-scm.org/repo/hg/file/tip/hgext/mq.py#l1423
On Wed, Feb 15, 2017 at 10:40 AM, Martin von Zweigbergk <martinvonz@google.com> wrote: > On Wed, Feb 15, 2017 at 10:35 AM, Gábor STEFANIK <Gabor.STEFANIK@nng.com> wrote: >>> >> >> >> -------------------------------------------------------------------------- >> This message, including its attachments, is confidential. For more information please read NNG's email policy here: >> http://www.nng.com/emailpolicy/ >> By responding to this email you accept the email policy. >> >> >> -----Original Message----- >>> From: Martin von Zweigbergk [mailto:martinvonz@google.com] >>> Sent: Wednesday, February 15, 2017 7:26 PM >>> To: Yuya Nishihara <yuya@tcha.org> >>> Cc: Gábor STEFANIK <Gabor.STEFANIK@nng.com>; Mercurial-devel >>> <mercurial-devel@mercurial-scm.org> >>> Subject: Re: [PATCH 5 of 6] update: learn --merge to allow merging across >>> topological branches (issue5125) >>> >>> On Wed, Feb 15, 2017 at 6:22 AM, Yuya Nishihara <yuya@tcha.org> wrote: >>> > On Wed, 15 Feb 2017 13:29:30 +0000, Gábor STEFANIK wrote: >>> >> > >> -def updatetotally(ui, repo, checkout, brev, clean=False, >>> check=False): >>> >> > >> +def updatetotally(ui, repo, checkout, brev, clean=False, >>> >> > >> + updatecheck='linear'): >>> >> > > >>> >> > > Please make this updatecheck=None, and set it to 'linear' inside >>> >> > > merge.update() explicitly if None was passed to that function. >>> >> > >>> >> > Will do. >>> >> > >>> >> > > >>> >> > > This way, in patch 6, you can red the config option inside >>> >> > > merge.update(), and let other callers (primarily extensions) also >>> >> > automatically follow the config option. >>> >> > >>> >> > I don't think it's a good idea for the low-level merge.update() to >>> >> > read the config. That function is called e.g. by rebase and graft >>> >> > and they should not have their behavior changed by this config. >>> > >>> > I tend to agree with Martin, low-level functions should have less >>> > dependency on config. Instead, maybe we could add a function that >>> > builds options from ui (like patch.diffopts), but I don't know if that's >>> appropriate here. >>> > >>> >> I forgot to add, I'm particularly concerned about the "guestrepo" >>> >> extension, which we use extensively at NNG. IIRC in the "grupdate" >>> >> command, it doesn't use commands.update, and so changing the config >>> option causes "update" >>> >> and "grupdate" to behave differently. >>> >> >>> >> Also, care must be taken about subrepositories. Not sure if our >>> >> subrepo update code calls command.update - if not, we may even end up >>> >> with a situation where "hg update" in a repository with subrepos with >>> ui.updatecheck="merge" >>> >> will do nonlinear updates with a dirty main repo, but not with a dirty >>> subrepo. >>> > >>> > IIRC, updating including subrepos isn't a simple cascading operation. >>> > Perhaps users would be asked how to process changes in subrepos? >>> > >>> >> In fact, if we introduce a config option, but only use it in >>> >> commands.update(), we should make the lower functions error out or at >>> >> least warn if called without explicit updatecheck, and without other >>> >> options (e.g. branchmerge) that override linearity checks. This way, >>> >> we at least catch extensions that don't follow the config option, but try to >>> rely on merge.update()'s linearity checks. >>> > >>> > Good point. >>> >>> Yeah, I agree with that last point. I tried it and there are several places that >>> fail. For example, clone, rebase, and mq all call hg.update[repo](). AFAICT, >>> there should never be any changes where it's being called, so maybe that >>> should be setting force=True to overwrite. Or maybe we should teach >>> merge.update() to accept updatecheck='abort' and make these callers pass >>> that value. >>> Regardless, since what I'm adding is an experimental config, I'd prefer to >>> make merge.update() default to 'linear' for now and I'll add a TODO to add >>> the check you mention. I don't have time right now to clean up all the other >>> callers, and I'd still like to be able to make progress on this topic. >> >> These other callers IIRC call with options that already disable linearity checks. >> >> We should only fail if we are in a normal update scenario with linearity checks >> enabled, but no check strategy was provided. > > The ones I mention do not disable linearity checks (not in all the > places anyway). Here are some examples: > > https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/hg.py#l667 > https://www.mercurial-scm.org/repo/hg/file/tip/hgext/rebase.py#l485 > https://www.mercurial-scm.org/repo/hg/file/tip/hgext/mq.py#l1423 And the reason they don't is probably that they have a reason to assume that the working directory is clean (e.g. a fresh clone should normally be clean).
On Wed, Feb 15, 2017 at 11:00 AM, Gábor STEFANIK <Gabor.STEFANIK@nng.com> wrote: >> > > > -------------------------------------------------------------------------- > This message, including its attachments, is confidential. For more information please read NNG's email policy here: > http://www.nng.com/emailpolicy/ > By responding to this email you accept the email policy. > > > -----Original Message----- >> From: Martin von Zweigbergk [mailto:martinvonz@google.com] >> Sent: Wednesday, February 15, 2017 7:41 PM >> To: Gábor STEFANIK <Gabor.STEFANIK@nng.com> >> Cc: Yuya Nishihara <yuya@tcha.org>; Mercurial-devel <mercurial- >> devel@mercurial-scm.org> >> Subject: Re: [PATCH 5 of 6] update: learn --merge to allow merging across >> topological branches (issue5125) >> >> On Wed, Feb 15, 2017 at 10:35 AM, Gábor STEFANIK >> <Gabor.STEFANIK@nng.com> wrote: >> >> >> > >> > >> > ---------------------------------------------------------------------- >> > ---- This message, including its attachments, is confidential. For >> > more information please read NNG's email policy here: >> > http://www.nng.com/emailpolicy/ >> > By responding to this email you accept the email policy. >> > >> > >> > -----Original Message----- >> >> From: Martin von Zweigbergk [mailto:martinvonz@google.com] >> >> Sent: Wednesday, February 15, 2017 7:26 PM >> >> To: Yuya Nishihara <yuya@tcha.org> >> >> Cc: Gábor STEFANIK <Gabor.STEFANIK@nng.com>; Mercurial-devel >> >> <mercurial-devel@mercurial-scm.org> >> >> Subject: Re: [PATCH 5 of 6] update: learn --merge to allow merging >> >> across topological branches (issue5125) >> >> >> >> On Wed, Feb 15, 2017 at 6:22 AM, Yuya Nishihara <yuya@tcha.org> wrote: >> >> > On Wed, 15 Feb 2017 13:29:30 +0000, Gábor STEFANIK wrote: >> >> >> > >> -def updatetotally(ui, repo, checkout, brev, clean=False, >> >> check=False): >> >> >> > >> +def updatetotally(ui, repo, checkout, brev, clean=False, >> >> >> > >> + updatecheck='linear'): >> >> >> > > >> >> >> > > Please make this updatecheck=None, and set it to 'linear' >> >> >> > > inside >> >> >> > > merge.update() explicitly if None was passed to that function. >> >> >> > >> >> >> > Will do. >> >> >> > >> >> >> > > >> >> >> > > This way, in patch 6, you can red the config option inside >> >> >> > > merge.update(), and let other callers (primarily extensions) >> >> >> > > also >> >> >> > automatically follow the config option. >> >> >> > >> >> >> > I don't think it's a good idea for the low-level merge.update() >> >> >> > to read the config. That function is called e.g. by rebase and >> >> >> > graft and they should not have their behavior changed by this config. >> >> > >> >> > I tend to agree with Martin, low-level functions should have less >> >> > dependency on config. Instead, maybe we could add a function that >> >> > builds options from ui (like patch.diffopts), but I don't know if >> >> > that's >> >> appropriate here. >> >> > >> >> >> I forgot to add, I'm particularly concerned about the "guestrepo" >> >> >> extension, which we use extensively at NNG. IIRC in the "grupdate" >> >> >> command, it doesn't use commands.update, and so changing the >> >> >> config >> >> option causes "update" >> >> >> and "grupdate" to behave differently. >> >> >> >> >> >> Also, care must be taken about subrepositories. Not sure if our >> >> >> subrepo update code calls command.update - if not, we may even end >> >> >> up with a situation where "hg update" in a repository with >> >> >> subrepos with >> >> ui.updatecheck="merge" >> >> >> will do nonlinear updates with a dirty main repo, but not with a >> >> >> dirty >> >> subrepo. >> >> > >> >> > IIRC, updating including subrepos isn't a simple cascading operation. >> >> > Perhaps users would be asked how to process changes in subrepos? >> >> > >> >> >> In fact, if we introduce a config option, but only use it in >> >> >> commands.update(), we should make the lower functions error out or >> >> >> at least warn if called without explicit updatecheck, and without >> >> >> other options (e.g. branchmerge) that override linearity checks. >> >> >> This way, we at least catch extensions that don't follow the >> >> >> config option, but try to >> >> rely on merge.update()'s linearity checks. >> >> > >> >> > Good point. >> >> >> >> Yeah, I agree with that last point. I tried it and there are several >> >> places that fail. For example, clone, rebase, and mq all call >> >> hg.update[repo](). AFAICT, there should never be any changes where >> >> it's being called, so maybe that should be setting force=True to >> >> overwrite. Or maybe we should teach >> >> merge.update() to accept updatecheck='abort' and make these callers >> >> pass that value. >> >> Regardless, since what I'm adding is an experimental config, I'd >> >> prefer to make merge.update() default to 'linear' for now and I'll >> >> add a TODO to add the check you mention. I don't have time right now >> >> to clean up all the other callers, and I'd still like to be able to make >> progress on this topic. >> > >> > These other callers IIRC call with options that already disable linearity >> checks. >> > >> > We should only fail if we are in a normal update scenario with >> > linearity checks enabled, but no check strategy was provided. >> >> The ones I mention do not disable linearity checks (not in all the places >> anyway). > > These are exactly the kinds of calls we'd like to catch. They ask for a linearity check, > but ignore the config option, which is wrong. > >> Here are some examples: >> >> https://www.mercurial-scm.org/repo/hg/file/tip/mercurial/hg.py#l667 > > This is updating a newly created repository, and should therefore update with 'abort'. > >> https://www.mercurial-scm.org/repo/hg/file/tip/hgext/rebase.py#l485 > > If we intend to allow rebasing with a dirty working copy, this should follow the > pref, otherwise it should be 'abort'. > >> https://www.mercurial-scm.org/repo/hg/file/tip/hgext/mq.py#l1423 > > Same as above, if "hg qpush" with a dirty working copy is meaningful, follow > the pref, otherwise 'abort'. Sure, that seems to be in line with what I said. I'm happy to review followup patches after my series is in if you feel like working on it.
Patch
diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/commands.py --- a/mercurial/commands.py Mon Feb 13 15:04:46 2017 -0800 +++ b/mercurial/commands.py Mon Feb 13 12:58:37 2017 -0800 @@ -6471,12 +6471,13 @@ @command('^update|up|checkout|co', [('C', 'clean', None, _('discard uncommitted changes (no backup)')), ('c', 'check', None, _('require clean working directory')), + ('m', 'merge', None, _('merge local changes')), ('d', 'date', '', _('tipmost revision matching date'), _('DATE')), ('r', 'rev', '', _('revision'), _('REV')) ] + mergetoolopts, - _('[-C|-c] [-d DATE] [[-r] REV]')) + _('[-C|-c|-m] [-d DATE] [[-r] REV]')) def update(ui, repo, node=None, rev=None, clean=False, date=None, check=False, - tool=None): + merge=None, tool=None): """update working directory (or switch revisions) Update the repository's working directory to the specified @@ -6498,7 +6499,7 @@ The following rules apply when the working directory contains uncommitted changes: - 1. If neither -c/--check nor -C/--clean is specified, and if + 1. If none of -c/--check, -C/--clean, or -m/--merge is specified, and if the requested changeset is an ancestor or descendant of the working directory's parent, the uncommitted changes are merged into the requested changeset and the merged @@ -6507,10 +6508,14 @@ branch), the update is aborted and the uncommitted changes are preserved. - 2. With the -c/--check option, the update is aborted and the + 2. With the -m/--merge option, the update is allowed even if the + requested changeset is not an ancestor or descendant of + the working directory's parent. + + 3. With the -c/--check option, the update is aborted and the uncommitted changes are preserved. - 3. With the -C/--clean option, uncommitted changes are discarded and + 4. With the -C/--clean option, uncommitted changes are discarded and the working directory is updated to the requested changeset. To cancel an uncommitted merge (and lose your changes), use @@ -6535,8 +6540,15 @@ if date and rev is not None: raise error.Abort(_("you can't specify a revision and a date")) - if check and clean: - raise error.Abort(_("cannot specify both -c/--check and -C/--clean")) + if len([x for x in (check, clean , merge) if x]) > 1: + raise error.Abort(_("can only specify one of -c/--check, -C/--clean, " + "and -m/merge")) + + updatecheck = 'linear' + if check: + updatecheck = 'abort' + elif merge: + updatecheck = None with repo.wlock(): cmdutil.clearunfinished(repo) @@ -6550,7 +6562,8 @@ repo.ui.setconfig('ui', 'forcemerge', tool, 'update') - return hg.updatetotally(ui, repo, rev, brev, clean=clean, check=check) + return hg.updatetotally(ui, repo, rev, brev, clean=clean, + updatecheck=updatecheck) @command('verify', []) def verify(ui, repo): diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/hg.py --- a/mercurial/hg.py Mon Feb 13 15:04:46 2017 -0800 +++ b/mercurial/hg.py Mon Feb 13 12:58:37 2017 -0800 @@ -681,18 +681,19 @@ repo.ui.status(_("%d files updated, %d files merged, " "%d files removed, %d files unresolved\n") % stats) -def updaterepo(repo, node, overwrite): +def updaterepo(repo, node, overwrite, updatecheck=None): """Update the working directory to node. When overwrite is set, changes are clobbered, merged else returns stats (see pydoc mercurial.merge.applyupdates)""" return mergemod.update(repo, node, False, overwrite, - labels=['working copy', 'destination']) + labels=['working copy', 'destination'], + updatecheck=updatecheck) -def update(repo, node, quietempty=False): - """update the working directory to node, merging linear changes""" - stats = updaterepo(repo, node, False) +def update(repo, node, quietempty=False, updatecheck=None): + """update the working directory to node""" + stats = updaterepo(repo, node, False, updatecheck=updatecheck) _showstats(repo, stats, quietempty) if stats[3]: repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n")) @@ -712,7 +713,8 @@ # naming conflict in updatetotally() _clean = clean -def updatetotally(ui, repo, checkout, brev, clean=False, check=False): +def updatetotally(ui, repo, checkout, brev, clean=False, + updatecheck='linear'): """Update the working directory with extra care for non-file components This takes care of non-file components below: @@ -724,7 +726,14 @@ :checkout: to which revision the working directory is updated :brev: a name, which might be a bookmark to be activated after updating :clean: whether changes in the working directory can be discarded - :check: whether changes in the working directory should be checked + :updatecheck: how to deal with a dirty working directory + + Valid values for updatecheck are: + + * abort: abort if the working directory is dirty + * None: don't check (merge working directory changes into destination) + * linear: check that update is linear before merging working directory + changes into destination This returns whether conflict is detected at updating or not. """ @@ -739,9 +748,10 @@ if clean: ret = _clean(repo, checkout) else: - if check: + if updatecheck == 'abort': cmdutil.bailifchanged(repo, merge=False) - ret = _update(repo, checkout) + updatecheck = None + ret = _update(repo, checkout, updatecheck=updatecheck) if not ret and movemarkfrom: if movemarkfrom == repo['.'].node(): diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/merge.py --- a/mercurial/merge.py Mon Feb 13 15:04:46 2017 -0800 +++ b/mercurial/merge.py Mon Feb 13 12:58:37 2017 -0800 @@ -1444,7 +1444,8 @@ repo.dirstate.normal(f) def update(repo, node, branchmerge, force, ancestor=None, - mergeancestor=False, labels=None, matcher=None, mergeforce=False): + mergeancestor=False, labels=None, matcher=None, mergeforce=False, + updatecheck='linear'): """ Perform a merge between the working directory and the given node @@ -1494,6 +1495,7 @@ # This functon used to find the default destination if node was None, but # that's now in destutil.py. assert node is not None + assert updatecheck in (None, 'linear') # If we're doing a partial update, we need to skip updating # the dirstate, so make a note of any partial-ness to the # update here. @@ -1550,7 +1552,8 @@ repo.hook('update', parent1=xp2, parent2='', error=0) return 0, 0, 0, 0 - if pas not in ([p1], [p2]): # nonlinear + if (updatecheck == 'linear' and + pas not in ([p1], [p2])): # nonlinear dirty = wc.dirty(missing=True) if dirty: # Branching is a bit strange to ensure we do the minimal diff -r 59e2d3da607c -r 75e5a492d7f6 tests/test-completion.t --- a/tests/test-completion.t Mon Feb 13 15:04:46 2017 -0800 +++ b/tests/test-completion.t Mon Feb 13 12:58:37 2017 -0800 @@ -223,7 +223,7 @@ serve: accesslog, daemon, daemon-postexec, errorlog, port, address, prefix, name, web-conf, webdir-conf, pid-file, stdio, cmdserver, templates, style, ipv6, certificate status: all, modified, added, removed, deleted, clean, unknown, ignored, no-status, copies, print0, rev, change, include, exclude, subrepos, template summary: remote - update: clean, check, date, rev, tool + update: clean, check, merge, date, rev, tool addremove: similarity, subrepos, include, exclude, dry-run archive: no-decode, prefix, rev, type, subrepos, include, exclude backout: merge, commit, no-commit, parent, rev, edit, tool, include, exclude, message, logfile, date, user diff -r 59e2d3da607c -r 75e5a492d7f6 tests/test-update-branches.t --- a/tests/test-update-branches.t Mon Feb 13 15:04:46 2017 -0800 +++ b/tests/test-update-branches.t Mon Feb 13 12:58:37 2017 -0800 @@ -160,6 +160,16 @@ parent=1 M foo + $ revtest '-m dirty linear' dirty 1 2 -m + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved + parent=2 + M foo + + $ revtest '-m dirty cross' dirty 3 4 -m + 1 files updated, 0 files merged, 0 files removed, 0 files unresolved + parent=4 + M foo + $ revtest '-c dirtysub linear' dirtysub 1 2 -c abort: uncommitted changes in subrepository 'sub' parent=1 @@ -171,7 +181,17 @@ parent=2 $ revtest '-cC dirty linear' dirty 1 2 -cC - abort: cannot specify both -c/--check and -C/--clean + abort: can only specify one of -c/--check, -C/--clean, and -m/merge + parent=1 + M foo + + $ revtest '-mc dirty linear' dirty 1 2 -mc + abort: can only specify one of -c/--check, -C/--clean, and -m/merge + parent=1 + M foo + + $ revtest '-mC dirty linear' dirty 1 2 -mC + abort: can only specify one of -c/--check, -C/--clean, and -m/merge parent=1 M foo