Patchwork [5,of,6] update: learn --merge to allow merging across topological branches (issue5125)

login
register
mail settings
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

via Mercurial-devel - Feb. 14, 2017, 1:07 a.m.
# 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)
via Mercurial-devel - Feb. 14, 2017, 10:36 p.m.
(+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
Yuya Nishihara - Feb. 15, 2017, 2:22 p.m.
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.
via Mercurial-devel - Feb. 15, 2017, 6:25 p.m.
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.
via Mercurial-devel - Feb. 15, 2017, 6:40 p.m.
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
via Mercurial-devel - Feb. 15, 2017, 6:42 p.m.
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).
via Mercurial-devel - Feb. 15, 2017, 7:02 p.m.
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