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

2017-02-15 Thread Martin von Zweigbergk via Mercurial-devel 
On Wed, Feb 15, 2017 at 11:00 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:41 PM
>> To: Gábor STEFANIK <gabor.stefa...@nng.com>
>> Cc: Yuya Nishihara <y...@tcha.org>; Mercurial-devel > 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 +, 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.
>> >

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

2017-02-15 Thread Gábor STEFANIK 
> 


--
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  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 +, 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 c

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

2017-02-15 Thread Martin von Zweigbergk via Mercurial-devel 
On Wed, Feb 15, 2017 at 10:40 AM, Martin von Zweigbergk
<martinv...@google.com> wrote:
> 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 +, 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
>

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

2017-02-15 Thread Martin von Zweigbergk via Mercurial-devel 
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 +, 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

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

2017-02-15 Thread Gábor STEFANIK 
> 


--
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 +, 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

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

2017-02-15 Thread Martin von Zweigbergk via Mercurial-devel 
On Wed, Feb 15, 2017 at 6:22 AM, Yuya Nishihara  wrote:
> On Wed, 15 Feb 2017 13:29:30 +, 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-02-15 Thread Yuya Nishihara 
On Wed, 15 Feb 2017 13:29:30 +, 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.
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

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

2017-02-15 Thread Gábor STEFANIK 
> 


--
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: Tuesday, February 14, 2017 11:36 PM
> To: Gábor STEFANIK <gabor.stefa...@nng.com>
> Cc: Mercurial-devel <mercurial-devel@mercurial-scm.org>
> Subject: Re: [PATCH 5 of 6] update: learn --merge to allow merging across
> topological branches (issue5125)
> 
> (+mercurial-devel)
> 
> On Tue, Feb 14, 2017 at 10:48 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: Mercurial-devel
> >> [mailto:mercurial-devel-boun...@mercurial-scm.org]
> >> On Behalf Of Martin von Zweigbergk via Mercurial-devel
> >> Sent: Tuesday, February 14, 2017 2:07 AM
> >> To: mercurial-devel@mercurial-scm.org
> >> Subject: [PATCH 5 of 6] update: learn --merge to allow merging across
> >> topological branches (issue5125)
> >>
> >> # HG changeset patch
> >> # User Martin von Zweigbergk <martinv...@google.com> # Date
> >> 1487019517 28800
> >> #  Mon Feb 13 12:58:37 2017 -0800
> >> # Node ID 75e5a492d7f69420a554fe498ae24060c755b09f
> >> # Parent  59e2d3da607cc09ebe133ab199fc4f343d74e1e6
> >> update: learn --merge to allow merging across topological branches
> >> (issue5125)
> >>
> >> diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/commands.py
> >> --- a/mercurial/commands.py   Mon Feb 13 15:04:46 2017 -0800
> >> +++ b/mercurial/commands.py   Mon Feb 13 12:58:37 2017 -0800
> >> @@ -6471,12 +6471,13 @@
> >>  @command('^update|up|checkout|co',
> >>  [('C', 'clean', None, _('discard uncommitted changes (no backup)')),
> >>  ('c', 'check', None, _('require clean working directory')),
> >> +('m', 'merge', None, _('merge local changes')),
> >>  ('d', 'date', '', _('tipmost revision matching date'), _('DATE')),
> >>  ('r', 'rev', '', _('revision'), _('REV'))
> >>   ] + mergetoolopts,
> >> -_('[-C|-c] [-d DATE] [[-r] REV]'))
> >> +_('[-C|-c|-m] [-d DATE] [[-r] REV]'))
> >>  def update(ui, repo, node=None, rev=None, clean=False, date=None,
> >> check=False,
> >> -   tool=None):
> >> +   merge=None, tool=None):
> >>  """update working directory (or switch revisions)
> >>
> >>  Update the repository's working directory to the specified @@
> >> -6498,7 +6499,7 @@
> >>The following rules apply when the working directory contains
> >>uncommitted changes:
> >>
> >> -  1. If neither -c/--check nor -C/--clean is specified, and if
> >> +  1. If none of -c/--check, -C/--clean, or -m/--merge is
> >> + specified, and if
> >>   the requested changeset is an ancestor or descendant of
> >>   the working directory's parent, the uncommitted changes
> >>   are merged into the requested changeset and the merged @@
> >> -6507,10 +6508,14 @@
> >>   branch), the update is aborted and the uncommitted changes
> >>   are preserved.
> >>
> >> -  2. With the -c/--check option, the update is aborted and the
> >> +  2. With the -m/--merge option, the update is allowed even if the
> >> + requested changeset is not an ancestor or descendant of
> >> + the working directory's parent.
> >> +
> >> +  3. With the -c/--check option, the update is aborted and the
> >>   uncommitted changes are preserved.
> >>
> >> -  3. With the -C/--clean option, uncommitted changes are discarded and
> >> +  4. With the -C/--clean option, uncommitted changes are
> >> + discarded and
> >>   the working directory is updated to the requested changeset.
> >>
> >>  To cancel an uncommitted merge (and lose your changes), use @@
> >> -6535,8 +6540,15 @@
> >>  if d

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

2017-02-14 Thread Martin von Zweigbergk via Mercurial-devel 
(+mercurial-devel)

On Tue, Feb 14, 2017 at 10:48 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: Mercurial-devel [mailto:mercurial-devel-boun...@mercurial-scm.org]
>> On Behalf Of Martin von Zweigbergk via Mercurial-devel
>> Sent: Tuesday, February 14, 2017 2:07 AM
>> To: mercurial-devel@mercurial-scm.org
>> Subject: [PATCH 5 of 6] update: learn --merge to allow merging across
>> topological branches (issue5125)
>>
>> # HG changeset patch
>> # User Martin von Zweigbergk <martinv...@google.com>
>> # Date 1487019517 28800
>> #  Mon Feb 13 12:58:37 2017 -0800
>> # Node ID 75e5a492d7f69420a554fe498ae24060c755b09f
>> # Parent  59e2d3da607cc09ebe133ab199fc4f343d74e1e6
>> update: learn --merge to allow merging across topological branches
>> (issue5125)
>>
>> diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/commands.py
>> --- a/mercurial/commands.py   Mon Feb 13 15:04:46 2017 -0800
>> +++ b/mercurial/commands.py   Mon Feb 13 12:58:37 2017 -0800
>> @@ -6471,12 +6471,13 @@
>>  @command('^update|up|checkout|co',
>>  [('C', 'clean', None, _('discard uncommitted changes (no backup)')),
>>  ('c', 'check', None, _('require clean working directory')),
>> +('m', 'merge', None, _('merge local changes')),
>>  ('d', 'date', '', _('tipmost revision matching date'), _('DATE')),
>>  ('r', 'rev', '', _('revision'), _('REV'))
>>   ] + mergetoolopts,
>> -_('[-C|-c] [-d DATE] [[-r] REV]'))
>> +_('[-C|-c|-m] [-d DATE] [[-r] REV]'))
>>  def update(ui, repo, node=None, rev=None, clean=False, date=None,
>> check=False,
>> -   tool=None):
>> +   merge=None, tool=None):
>>  """update working directory (or switch revisions)
>>
>>  Update the repository's working directory to the specified
>> @@ -6498,7 +6499,7 @@
>>The following rules apply when the working directory contains
>>uncommitted changes:
>>
>> -  1. If neither -c/--check nor -C/--clean is specified, and if
>> +  1. If none of -c/--check, -C/--clean, or -m/--merge is specified, and 
>> if
>>   the requested changeset is an ancestor or descendant of
>>   the working directory's parent, the uncommitted changes
>>   are merged into the requested changeset and the merged
>> @@ -6507,10 +6508,14 @@
>>   branch), the update is aborted and the uncommitted changes
>>   are preserved.
>>
>> -  2. With the -c/--check option, the update is aborted and the
>> +  2. With the -m/--merge option, the update is allowed even if the
>> + requested changeset is not an ancestor or descendant of
>> + the working directory's parent.
>> +
>> +  3. With the -c/--check option, the update is aborted and the
>>   uncommitted changes are preserved.
>>
>> -  3. With the -C/--clean option, uncommitted changes are discarded and
>> +  4. With the -C/--clean option, uncommitted changes are discarded and
>>   the working directory is updated to the requested changeset.
>>
>>  To cancel an uncommitted merge (and lose your changes), use
>> @@ -6535,8 +6540,15 @@
>>  if date and rev is not None:
>>  raise error.Abort(_("you can't specify a revision and a date"))
>>
>> -if check and clean:
>> -raise error.Abort(_("cannot specify both -c/--check and 
>> -C/--clean"))
>> +if len([x for x in (check, clean , merge) if x]) > 1:
>> +raise error.Abort(_("can only specify one of -c/--check, 
>> -C/--clean, "
>> +"and -m/merge"))
>> +
>> +updatecheck = 'linear'
>> +if check:
>> +updatecheck = 'abort'
>> +elif merge:
>> +updatecheck = None
>
> I don't really like the use of None to indicate "always merge".
> I would strongly prefer updatecheck = 'merge', with None instead
> representing a default or "unset" case.

Makes sense. Since the check that's done is not about merging, I'll
call it 'none' instead, if you don't mind.

>
> This makes setting the default via a config option (as in your 6th patch)
> a lot s

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

2017-02-13 Thread Martin von Zweigbergk via Mercurial-devel 
# HG changeset patch
# User Martin von Zweigbergk 
# Date 1487019517 28800
#  Mon Feb 13 12:58:37 2017 -0800
# Node ID 75e5a492d7f69420a554fe498ae24060c755b09f
# Parent  59e2d3da607cc09ebe133ab199fc4f343d74e1e6
update: learn --merge to allow merging across topological branches (issue5125)

diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/commands.py
--- a/mercurial/commands.py Mon Feb 13 15:04:46 2017 -0800
+++ b/mercurial/commands.py Mon Feb 13 12:58:37 2017 -0800
@@ -6471,12 +6471,13 @@
 @command('^update|up|checkout|co',
 [('C', 'clean', None, _('discard uncommitted changes (no backup)')),
 ('c', 'check', None, _('require clean working directory')),
+('m', 'merge', None, _('merge local changes')),
 ('d', 'date', '', _('tipmost revision matching date'), _('DATE')),
 ('r', 'rev', '', _('revision'), _('REV'))
  ] + mergetoolopts,
-_('[-C|-c] [-d DATE] [[-r] REV]'))
+_('[-C|-c|-m] [-d DATE] [[-r] REV]'))
 def update(ui, repo, node=None, rev=None, clean=False, date=None, check=False,
-   tool=None):
+   merge=None, tool=None):
 """update working directory (or switch revisions)
 
 Update the repository's working directory to the specified
@@ -6498,7 +6499,7 @@
   The following rules apply when the working directory contains
   uncommitted changes:
 
-  1. If neither -c/--check nor -C/--clean is specified, and if
+  1. If none of -c/--check, -C/--clean, or -m/--merge is specified, and if
  the requested changeset is an ancestor or descendant of
  the working directory's parent, the uncommitted changes
  are merged into the requested changeset and the merged
@@ -6507,10 +6508,14 @@
  branch), the update is aborted and the uncommitted changes
  are preserved.
 
-  2. With the -c/--check option, the update is aborted and the
+  2. With the -m/--merge option, the update is allowed even if the
+ requested changeset is not an ancestor or descendant of
+ the working directory's parent.
+
+  3. With the -c/--check option, the update is aborted and the
  uncommitted changes are preserved.
 
-  3. With the -C/--clean option, uncommitted changes are discarded and
+  4. With the -C/--clean option, uncommitted changes are discarded and
  the working directory is updated to the requested changeset.
 
 To cancel an uncommitted merge (and lose your changes), use
@@ -6535,8 +6540,15 @@
 if date and rev is not None:
 raise error.Abort(_("you can't specify a revision and a date"))
 
-if check and clean:
-raise error.Abort(_("cannot specify both -c/--check and -C/--clean"))
+if len([x for x in (check, clean , merge) if x]) > 1:
+raise error.Abort(_("can only specify one of -c/--check, -C/--clean, "
+"and -m/merge"))
+
+updatecheck = 'linear'
+if check:
+updatecheck = 'abort'
+elif merge:
+updatecheck = None
 
 with repo.wlock():
 cmdutil.clearunfinished(repo)
@@ -6550,7 +6562,8 @@
 
 repo.ui.setconfig('ui', 'forcemerge', tool, 'update')
 
-return hg.updatetotally(ui, repo, rev, brev, clean=clean, check=check)
+return hg.updatetotally(ui, repo, rev, brev, clean=clean,
+updatecheck=updatecheck)
 
 @command('verify', [])
 def verify(ui, repo):
diff -r 59e2d3da607c -r 75e5a492d7f6 mercurial/hg.py
--- a/mercurial/hg.py   Mon Feb 13 15:04:46 2017 -0800
+++ b/mercurial/hg.py   Mon Feb 13 12:58:37 2017 -0800
@@ -681,18 +681,19 @@
 repo.ui.status(_("%d files updated, %d files merged, "
  "%d files removed, %d files unresolved\n") % stats)
 
-def updaterepo(repo, node, overwrite):
+def updaterepo(repo, node, overwrite, updatecheck=None):
 """Update the working directory to node.
 
 When overwrite is set, changes are clobbered, merged else
 
 returns stats (see pydoc mercurial.merge.applyupdates)"""
 return mergemod.update(repo, node, False, overwrite,
-   labels=['working copy', 'destination'])
+   labels=['working copy', 'destination'],
+   updatecheck=updatecheck)
 
-def update(repo, node, quietempty=False):
-"""update the working directory to node, merging linear changes"""
-stats = updaterepo(repo, node, False)
+def update(repo, node, quietempty=False, updatecheck=None):
+"""update the working directory to node"""
+stats = updaterepo(repo, node, False, updatecheck=updatecheck)
 _showstats(repo, stats, quietempty)
 if stats[3]:
 repo.ui.status(_("use 'hg resolve' to retry unresolved file merges\n"))
@@ -712,7 +713,8 @@
 # naming conflict in updatetotally()
 _clean = clean
 
-def updatetotally(ui, repo, checkout, brev, clean=False, check=False):
+def updatetotally(ui, repo, checkout, brev, clean=False,
+