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). 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 _______________________________________________ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel