>
-------------------------------------------------------------------------- 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:41 PM > To: Gábor STEFANIK <gabor.stefa...@nng.com> > Cc: Yuya Nishihara <y...@tcha.org>; Mercurial-devel <mercurial- > de...@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.stefa...@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: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. > > 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'. _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel