>
-------------------------------------------------------------------------- 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:martinv...@google.com] > Sent: Wednesday, February 15, 2017 7:26 PM > To: Yuya Nishihara <y...@tcha.org> > Cc: Gábor STEFANIK <gabor.stefa...@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 <y...@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. _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
