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

Gábor STEFANIK Gabor.STEFANIK at nng.com
Wed Feb 15 14:00:20 EST 2017


> 


--------------------------------------------------------------------------
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 at google.com]
> Sent: Wednesday, February 15, 2017 7:41 PM
> To: Gábor STEFANIK <Gabor.STEFANIK at nng.com>
> Cc: Yuya Nishihara <yuya at tcha.org>; Mercurial-devel <mercurial-
> devel at 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 at 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 at google.com]
> >> Sent: Wednesday, February 15, 2017 7:26 PM
> >> To: Yuya Nishihara <yuya at tcha.org>
> >> Cc: Gábor STEFANIK <Gabor.STEFANIK at nng.com>; Mercurial-devel
> >> <mercurial-devel at 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 at 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'.


More information about the Mercurial-devel mailing list