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

[Martin von Zweigbergk]

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