Patchwork [2,of,4] rebase: add config that causes empty changesets to be created

# HG changeset patch
# User Manuel Jacob <me@manueljacob.de>
# Date 1590993522 -7200
#      Mon Jun 01 08:38:42 2020 +0200
# Node ID 45fa02da6fd1c3b40350329d80d6fe03c83f9a30
# Parent  ee204ca1c4d68d004d4f90b5fc9edf66c9db6bae
# EXP-Topic createempty
rebase: add config that causes empty changesets to be created

The default in rebase is that changesets which would become empty are not
created in the target branch. This makes sense if the source branch consists of
small fix-up changes. For more advanced workflows that make heavy use of
history-editing to create curated patch series, dropping empty changesets is
not as important or even undesirable.

Some users want to keep the meta-history, e.g. to make finding comments in a
code review tool easier or to avoid that divergent bookmarks are created. For
that, obsmarkers from the (to-be) empty changeset to the changeset(s) that
already made the changes should be added. If a to-be empty changeset is pruned
without a successor, adding the obsmarkers is hard because the changeset has to
be found within the hidden part of the history.

If rebasing in TortoiseHg, it’s easy to miss the fact that the to-be empty
changeset was pruned. An empty changeset will function as a reminder that
obsmarkers should be added.

On Wed, 01 Jul 2020 23:39:01 +0200, Manuel Jacob wrote:
> # HG changeset patch
> # User Manuel Jacob <me@manueljacob.de>
> # Date 1590993522 -7200
> #      Mon Jun 01 08:38:42 2020 +0200
> # Node ID 45fa02da6fd1c3b40350329d80d6fe03c83f9a30
> # Parent  ee204ca1c4d68d004d4f90b5fc9edf66c9db6bae
> # EXP-Topic createempty
> rebase: add config that causes empty changesets to be created
> 
> The default in rebase is that changesets which would become empty are not
> created in the target branch. This makes sense if the source branch consists of
> small fix-up changes. For more advanced workflows that make heavy use of
> history-editing to create curated patch series, dropping empty changesets is
> not as important or even undesirable.
> 
> Some users want to keep the meta-history, e.g. to make finding comments in a
> code review tool easier or to avoid that divergent bookmarks are created. For
> that, obsmarkers from the (to-be) empty changeset to the changeset(s) that
> already made the changes should be added. If a to-be empty changeset is pruned
> without a successor, adding the obsmarkers is hard because the changeset has to
> be found within the hidden part of the history.

I think it's fine to add such configuration.

> diff --git a/hgext/rebase.py b/hgext/rebase.py
> --- a/hgext/rebase.py
> +++ b/hgext/rebase.py
> @@ -534,7 +534,7 @@
>          )
>          destphase = max(ctx.phase(), phases.draft)
>          overrides = {(b'phases', b'new-commit'): destphase}
> -        if keepbranchchange:
> +        if keepbranchchange or repo.ui.configbool(b'rebase', b'createempty'):

Nit: According to the current naming convention, it should be named as
create-empty, and I slightly prefer allow-empty-commit than createempty.

https://www.mercurial-scm.org/wiki/ConfigConsolidationPlan

Since we have rebase/absorb.<opt>, adding rewrite.<opt> might be better.
Please also update the help text.

> @@ -655,6 +655,18 @@
>              if newnode is not None:
>                  self.state[rev] = repo[newnode].rev()
>                  ui.debug(b'rebased as %s\n' % short(newnode))
> +                if not (
> +                    repo[newnode].files()
> +                    or len(repo[newnode].parents()) > 1
> +                    or repo[p1].branch() != repo[newnode].branch()
> +                ):

I think I've seen this pattern before, and it had close-branch handling.
Maybe it's time to add a named function to scmutil.py or rewriteutil.py?

On 2020-07-03 12:45, Yuya Nishihara wrote:
> On Wed, 01 Jul 2020 23:39:01 +0200, Manuel Jacob wrote:
>> # HG changeset patch
>> # User Manuel Jacob <me@manueljacob.de>
>> # Date 1590993522 -7200
>> #      Mon Jun 01 08:38:42 2020 +0200
>> # Node ID 45fa02da6fd1c3b40350329d80d6fe03c83f9a30
>> # Parent  ee204ca1c4d68d004d4f90b5fc9edf66c9db6bae
>> # EXP-Topic createempty
>> rebase: add config that causes empty changesets to be created
>> 
>> The default in rebase is that changesets which would become empty are 
>> not
>> created in the target branch. This makes sense if the source branch 
>> consists of
>> small fix-up changes. For more advanced workflows that make heavy use 
>> of
>> history-editing to create curated patch series, dropping empty 
>> changesets is
>> not as important or even undesirable.
>> 
>> Some users want to keep the meta-history, e.g. to make finding 
>> comments in a
>> code review tool easier or to avoid that divergent bookmarks are 
>> created. For
>> that, obsmarkers from the (to-be) empty changeset to the changeset(s) 
>> that
>> already made the changes should be added. If a to-be empty changeset 
>> is pruned
>> without a successor, adding the obsmarkers is hard because the 
>> changeset has to
>> be found within the hidden part of the history.
> 
> I think it's fine to add such configuration.
> 
>> diff --git a/hgext/rebase.py b/hgext/rebase.py
>> --- a/hgext/rebase.py
>> +++ b/hgext/rebase.py
>> @@ -534,7 +534,7 @@
>>          )
>>          destphase = max(ctx.phase(), phases.draft)
>>          overrides = {(b'phases', b'new-commit'): destphase}
>> -        if keepbranchchange:
>> +        if keepbranchchange or repo.ui.configbool(b'rebase', 
>> b'createempty'):
> 
> Nit: According to the current naming convention, it should be named as
> create-empty, and I slightly prefer allow-empty-commit than 
> createempty.
> 
> https://www.mercurial-scm.org/wiki/ConfigConsolidationPlan

While I don’t understand why hyphens instead of underscores were taken, 
I accept that hyphens should be used here (it’s in any case better than 
no separator).

I’m not sure whether "allow" is better than "create". It makes sense 
that "ui.allowemptycommit" is called like this because without the 
option, `hg commit` actually disallows empty commits. From the user 
perspective, for rebase, it’s not about whether it’s allowed or not. 
It’s about whether they are created or the creation is skipped.

> Since we have rebase/absorb.<opt>, adding rewrite.<opt> might be 
> better.

I considered adding a unified option, but thought it might be helpful to 
make it separately configurable. For me personally, that configurability 
is not needed (I would enable both).

I’d like a second opinion from Pierre-Yves on this (but also the 
previous) points, as I already briefly discussed the topic with him 
(although while going up a very steep hill if I remember correctly).

> Please also update the help text.

Should I just add another paragraph under "Configuration Options"? I was 
concerned that if we document it, we can’t change the name or semantics 
later. Still, documenting the as-is state seems good.

>> @@ -655,6 +655,18 @@
>>              if newnode is not None:
>>                  self.state[rev] = repo[newnode].rev()
>>                  ui.debug(b'rebased as %s\n' % short(newnode))
>> +                if not (
>> +                    repo[newnode].files()
>> +                    or len(repo[newnode].parents()) > 1
>> +                    or repo[p1].branch() != repo[newnode].branch()
>> +                ):
> 
> I think I've seen this pattern before, and it had close-branch 
> handling.
> Maybe it's time to add a named function to scmutil.py or 
> rewriteutil.py?

Yes, you probably saw it in my recent patch series about preserving 
branch changes in absorb (function _willbecomenoop()). If it’s just the 
one-line pattern, I think we would not gain much from having it in a 
function. I will check if a larger piece of logic can be factored out.

On Fri, 03 Jul 2020 13:36:24 +0200, Manuel Jacob wrote:
> I’m not sure whether "allow" is better than "create". It makes sense 
> that "ui.allowemptycommit" is called like this because without the 
> option, `hg commit` actually disallows empty commits. From the user 
> perspective, for rebase, it’s not about whether it’s allowed or not. 
> It’s about whether they are created or the creation is skipped.

"create empty" sounded to me that it would always create something
empty. If "allow" doesn't make sense, how about "keep empty commit"?

> > Please also update the help text.
> 
> Should I just add another paragraph under "Configuration Options"? I was 
> concerned that if we document it, we can’t change the name or semantics 
> later. Still, documenting the as-is state seems good.

If the option is still unstable, mark it as experimental. I think there's
a check-code rule, but maybe it wouldn't work anymore.

> >> +                if not (
> >> +                    repo[newnode].files()
> >> +                    or len(repo[newnode].parents()) > 1
> >> +                    or repo[p1].branch() != repo[newnode].branch()
> >> +                ):
> > 
> > I think I've seen this pattern before, and it had close-branch 
> > handling.
> > Maybe it's time to add a named function to scmutil.py or 
> > rewriteutil.py?
> 
> Yes, you probably saw it in my recent patch series about preserving 
> branch changes in absorb (function _willbecomenoop()). If it’s just the 
> one-line pattern, I think we would not gain much from having it in a 
> function. I will check if a larger piece of logic can be factored out.

As a reviewer, a named function gives me confidence that we're doing
things correct. Otherwise I would always have to scan the codebase to
see if the expression is necessary and sufficient.

I did not had time to follow the details of this discussion. However a 
discussion I had with Martin on another command is relevant for this work.

We decided that -in-all-case- creating and empty commit at destination 
was useful, even if it was to prune it automatically in the same go. 
Because it allow for a better evolution-history. obslog display a better 
history that make the "prune" clearer and automatic evolution could 
behave in a better way.

So for example if rebase A - B -C resulted in B being empty, we could create

A - C
   \ B (pruned).


Sicne you are touching this area (adding the option to have)

A - B (empty) - C,

it would be nice if you could move the alternative to what I just describe.

What do you think ?

On 7/3/20 2:54 PM, Yuya Nishihara wrote:
> On Fri, 03 Jul 2020 13:36:24 +0200, Manuel Jacob wrote:
>> I’m not sure whether "allow" is better than "create". It makes sense
>> that "ui.allowemptycommit" is called like this because without the
>> option, `hg commit` actually disallows empty commits. From the user
>> perspective, for rebase, it’s not about whether it’s allowed or not.
>> It’s about whether they are created or the creation is skipped.
> 
> "create empty" sounded to me that it would always create something
> empty. If "allow" doesn't make sense, how about "keep empty commit"?
> 
>>> Please also update the help text.
>>
>> Should I just add another paragraph under "Configuration Options"? I was
>> concerned that if we document it, we can’t change the name or semantics
>> later. Still, documenting the as-is state seems good.
> 
> If the option is still unstable, mark it as experimental. I think there's
> a check-code rule, but maybe it wouldn't work anymore.
> 
>>>> +                if not (
>>>> +                    repo[newnode].files()
>>>> +                    or len(repo[newnode].parents()) > 1
>>>> +                    or repo[p1].branch() != repo[newnode].branch()
>>>> +                ):
>>>
>>> I think I've seen this pattern before, and it had close-branch
>>> handling.
>>> Maybe it's time to add a named function to scmutil.py or
>>> rewriteutil.py?
>>
>> Yes, you probably saw it in my recent patch series about preserving
>> branch changes in absorb (function _willbecomenoop()). If it’s just the
>> one-line pattern, I think we would not gain much from having it in a
>> function. I will check if a larger piece of logic can be factored out.
> 
> As a reviewer, a named function gives me confidence that we're doing
> things correct. Otherwise I would always have to scan the codebase to
> see if the expression is necessary and sufficient.
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
> discussion I had with Martin on another command is relevant for this work.
> 
> We decided that -in-all-case- creating and empty commit at destination 
> was useful, even if it was to prune it automatically in the same go.

So you second the global "rewrite.<opt>" config knob than "<command>.<opt>"?
I'm not against adding this feature.

FWIW, it's helpful to include this kind of information in commit message since
I don't follow IRC discussion or Phabricator. If you and Martin stamped this
feature, only thing I would have to do is nitpicking.

IIRC, this was while discussing thing for part on evolution. Either on 
#hg-evolve or on a heptapod merge request.

On 7/3/20 3:32 PM, Yuya Nishihara wrote:
> On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
>> discussion I had with Martin on another command is relevant for this work.
>>
>> We decided that -in-all-case- creating and empty commit at destination
>> was useful, even if it was to prune it automatically in the same go.
> So you second the global "rewrite.<opt>" config knob than "<command>.<opt>"?
> I'm not against adding this feature.
>
> FWIW, it's helpful to include this kind of information in commit message since
> I don't follow IRC discussion or Phabricator. If you and Martin stamped this
> feature, only thing I would have to do is nitpicking.

IIRC, this was while discussing thing for part on evolution. Either on 
#hg-evolve or on a heptapod merge request.

On 7/3/20 3:32 PM, Yuya Nishihara wrote:
> On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
>> discussion I had with Martin on another command is relevant for this work.
>>
>> We decided that -in-all-case- creating and empty commit at destination
>> was useful, even if it was to prune it automatically in the same go.
> 
> So you second the global "rewrite.<opt>" config knob than "<command>.<opt>"?
> I'm not against adding this feature.
> 
> FWIW, it's helpful to include this kind of information in commit message since
> I don't follow IRC discussion or Phabricator. If you and Martin stamped this
> feature, only thing I would have to do is nitpicking.
>

On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
> On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
> > discussion I had with Martin on another command is relevant for this work.
> >
> > We decided that -in-all-case- creating and empty commit at destination
> > was useful, even if it was to prune it automatically in the same go.
>
> So you second the global "rewrite.<opt>" config knob than "<command>.<opt>"?
> I'm not against adding this feature.
>
> FWIW, it's helpful to include this kind of information in commit message since
> I don't follow IRC discussion or Phabricator. If you and Martin stamped this
> feature, only thing I would have to do is nitpicking.

Another point on commit messages: in general, we should try and make
sure that the _why_ is encoded to the maximum extent possible so
future programmer-archaeologists have an easier time of things. :)

> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

Other reasons to always create the empty successor (in addition to just
having useful information in `hg obslog`):

* orphans on the predecessor would be evolved onto the successor instead of
being evolved onto the predecessor's parent. `hg evolve` could be taught to
evolve not evolve the orphan onto the empty commit but instead onto its
parent. (This may be what Pierre-Yves meant by "automatic evolution could
behave in a better way".)
* `hg strip <root of rebased commits>` will correctly de-obsolete the
predecessor

You could add this to the commit message if you end up creating the
obsmarkers that way.

On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> wrote:

> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
> > > discussion I had with Martin on another command is relevant for this
> work.
> > >
> > > We decided that -in-all-case- creating and empty commit at destination
> > > was useful, even if it was to prune it automatically in the same go.
> >
> > So you second the global "rewrite.<opt>" config knob than
> "<command>.<opt>"?
> > I'm not against adding this feature.
> >
> > FWIW, it's helpful to include this kind of information in commit message
> since
> > I don't follow IRC discussion or Phabricator. If you and Martin stamped
> this
> > feature, only thing I would have to do is nitpicking.
>
> Another point on commit messages: in general, we should try and make
> sure that the _why_ is encoded to the maximum extent possible so
> future programmer-archaeologists have an easier time of things. :)
>
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

On 2020-07-08 01:29, Martin von Zweigbergk via Mercurial-devel wrote:
> Other reasons to always create the empty successor (in addition to just
> having useful information in `hg obslog`):
> 
> * orphans on the predecessor would be evolved onto the successor 
> instead of
> being evolved onto the predecessor's parent. `hg evolve` could be 
> taught to
> evolve not evolve the orphan onto the empty commit but instead onto its
> parent. (This may be what Pierre-Yves meant by "automatic evolution 
> could
> behave in a better way".)
> * `hg strip <root of rebased commits>` will correctly de-obsolete the
> predecessor

I see four possibilities:

1) We don’t create an empty successor and prune the predecessor (without 
successor).
2) We create an empty successor and
2a) prune the successor immediately (without successor), and rebase the 
children of the predecessor onto the parent of the empty, pruned 
successor.
2b) leave the successor, and rebase the children of the predecessor onto 
the parent of the empty (but not pruned) successor.
2c) leave the successor, and rebase the children of the predecessor onto 
the empty (but not pruned) successor.

The patch implements (2c). What Pierre-Yves described is (2a). Can you 
clarify which possibility you referred to?

The `hg strip` point is true for (2a), (2b) and (2c). With the current 
evolve command, both (1) and (2a) cause the orphan to be relocated onto 
the predecessor’s parent, and both (2b) and (2c) cause the orphan to be 
relocated onto the empty successor. With (2c), the behavior of rebase 
and evolve would be consistent. With (2b), the behavior of rebase and 
evolve would be inconsistent, unless the behavior of evolve is changed 
like you described.

> You could add this to the commit message if you end up creating the
> obsmarkers that way.

Should we change the default from (1) to (2a)? If I understand 
Pierre-Yves correctly, he proposed that. Without obsmarkers enabled, the 
end result would be exactly the same. With obsmarkers enabled, the 
filtered repo would be the same (except revision numbers and 
obsmarkers), and the unfiltered repo would have some extra empty 
changesets.

For my use case, either (2b) and (2c) behind a configuration would be 
fine. If it’s not clear whether we should implement (2b) or (2c), I can 
implement both (so the user can opt-in to any of them).

Do you have an opinion on whether there should be one combined config 
for rebase and absorb (in the [rewrite] section) or separate configs for 
rebase and absorb (in their respective section)?

> On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> wrote:
> 
>> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
>> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
>> > > discussion I had with Martin on another command is relevant for this
>> work.
>> > >
>> > > We decided that -in-all-case- creating and empty commit at destination
>> > > was useful, even if it was to prune it automatically in the same go.
>> >
>> > So you second the global "rewrite.<opt>" config knob than
>> "<command>.<opt>"?
>> > I'm not against adding this feature.
>> >
>> > FWIW, it's helpful to include this kind of information in commit message
>> since
>> > I don't follow IRC discussion or Phabricator. If you and Martin stamped
>> this
>> > feature, only thing I would have to do is nitpicking.
>> 
>> Another point on commit messages: in general, we should try and make
>> sure that the _why_ is encoded to the maximum extent possible so
>> future programmer-archaeologists have an easier time of things. :)
>> 
>> > _______________________________________________
>> > Mercurial-devel mailing list
>> > Mercurial-devel@mercurial-scm.org
>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> _______________________________________________
>> Mercurial-devel mailing list
>> Mercurial-devel@mercurial-scm.org
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> 
> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <me@manueljacob.de> wrote:

> On 2020-07-08 01:29, Martin von Zweigbergk via Mercurial-devel wrote:
> > Other reasons to always create the empty successor (in addition to just
> > having useful information in `hg obslog`):
> >
> > * orphans on the predecessor would be evolved onto the successor
> > instead of
> > being evolved onto the predecessor's parent. `hg evolve` could be
> > taught to
> > evolve not evolve the orphan onto the empty commit but instead onto its
> > parent. (This may be what Pierre-Yves meant by "automatic evolution
> > could
> > behave in a better way".)
> > * `hg strip <root of rebased commits>` will correctly de-obsolete the
> > predecessor
>
> I see four possibilities:
>
> 1) We don’t create an empty successor and prune the predecessor (without
> successor).
> 2) We create an empty successor and
> 2a) prune the successor immediately (without successor), and rebase the
> children of the predecessor onto the parent of the empty, pruned
> successor.
> 2b) leave the successor, and rebase the children of the predecessor onto
> the parent of the empty (but not pruned) successor.
> 2c) leave the successor, and rebase the children of the predecessor onto
> the empty (but not pruned) successor.
>
> The patch implements (2c). What Pierre-Yves described is (2a). Can you
> clarify which possibility you referred to?
>

I'm also for (2a).


>
> The `hg strip` point is true for (2a), (2b) and (2c).


It's actually not true any of (2*) if the successor is the first commit in
the stack; you'd have to separately strip any empty successors (I omitted
this detail in my previous email). That's still better than manually
deleting obsmarkers, though. For (2a), you'd have to also `hg log --hidden`
to find the pruned successor(s).

With the current
> evolve command, both (1) and (2a) cause the orphan to be relocated onto
> the predecessor’s parent,


It depends. If the predecessor's parent was also rebased to the same
destination, then the current evolve command will actually rebase it to
there.

and both (2b) and (2c) cause the orphan to be
> relocated onto the empty successor. With (2c), the behavior of rebase
> and evolve would be consistent. With (2b), the behavior of rebase and
> evolve would be inconsistent, unless the behavior of evolve is changed
> like you described.
>

I meant that we might want to make that change for the (2a) case. I suspect
that would be generally better, even though there might be false positives
where a pruned, empty successor exists for some reason even though the user
would not want the orphan to be rebased onto that successor's parent.


>
> > You could add this to the commit message if you end up creating the
> > obsmarkers that way.
>
> Should we change the default from (1) to (2a)? If I understand
> Pierre-Yves correctly, he proposed that. Without obsmarkers enabled, the
> end result would be exactly the same. With obsmarkers enabled, the
> filtered repo would be the same (except revision numbers and
> obsmarkers), and the unfiltered repo would have some extra empty
> changesets.
>

I think it would be an interesting experiment, but it should probably be
behind an [experimental] config.


> For my use case, either (2b) and (2c) behind a configuration would be
> fine. If it’s not clear whether we should implement (2b) or (2c), I can
> implement both (so the user can opt-in to any of them).
>

IMO, the empty commit would be there mostly to guide evolve, so I'm not
sure how much value there is in (2b).

I also understand if (2a) is too much scope bloat for you.


>
> Do you have an opinion on whether there should be one combined config
> for rebase and absorb (in the [rewrite] section) or separate configs for
> rebase and absorb (in their respective section)?
>

I'd prefer one section. Maybe two separate configs as follows would make
sense?

* rewrite.empty-commit={strip,prune,keep} decides what to do when a commit
becomes empty
* rewrite.use-parent-if-empty=true/false decides whether to rebase/evolve
onto the parent instead if the usual destination is empty

In terms of those configs, we have:

                            |  (1)  | (2a)  | (2b)  | (2c)
rewrite.empty-commit        | strip | prune | keep  | keep
rewrite.use-parent-if-empty | false | true  | true  | false



> > On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> wrote:
> >
> >> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
> >> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
> >> > > discussion I had with Martin on another command is relevant for this
> >> work.
> >> > >
> >> > > We decided that -in-all-case- creating and empty commit at
> destination
> >> > > was useful, even if it was to prune it automatically in the same go.
> >> >
> >> > So you second the global "rewrite.<opt>" config knob than
> >> "<command>.<opt>"?
> >> > I'm not against adding this feature.
> >> >
> >> > FWIW, it's helpful to include this kind of information in commit
> message
> >> since
> >> > I don't follow IRC discussion or Phabricator. If you and Martin
> stamped
> >> this
> >> > feature, only thing I would have to do is nitpicking.
> >>
> >> Another point on commit messages: in general, we should try and make
> >> sure that the _why_ is encoded to the maximum extent possible so
> >> future programmer-archaeologists have an easier time of things. :)
> >>
> >> > _______________________________________________
> >> > Mercurial-devel mailing list
> >> > Mercurial-devel@mercurial-scm.org
> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >> _______________________________________________
> >> Mercurial-devel mailing list
> >> Mercurial-devel@mercurial-scm.org
> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >>
> >
> > _______________________________________________
> > Mercurial-devel mailing list
> > Mercurial-devel@mercurial-scm.org
> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

On 2020-07-09 18:48, Martin von Zweigbergk wrote:
> On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <me@manueljacob.de> wrote:
> 
>> On 2020-07-08 01:29, Martin von Zweigbergk via Mercurial-devel wrote:
>> > Other reasons to always create the empty successor (in addition to just
>> > having useful information in `hg obslog`):
>> >
>> > * orphans on the predecessor would be evolved onto the successor
>> > instead of
>> > being evolved onto the predecessor's parent. `hg evolve` could be
>> > taught to
>> > evolve not evolve the orphan onto the empty commit but instead onto its
>> > parent. (This may be what Pierre-Yves meant by "automatic evolution
>> > could
>> > behave in a better way".)
>> > * `hg strip <root of rebased commits>` will correctly de-obsolete the
>> > predecessor
>> 
>> I see four possibilities:
>> 
>> 1) We don’t create an empty successor and prune the predecessor 
>> (without
>> successor).
>> 2) We create an empty successor and
>> 2a) prune the successor immediately (without successor), and rebase 
>> the
>> children of the predecessor onto the parent of the empty, pruned
>> successor.
>> 2b) leave the successor, and rebase the children of the predecessor 
>> onto
>> the parent of the empty (but not pruned) successor.
>> 2c) leave the successor, and rebase the children of the predecessor 
>> onto
>> the empty (but not pruned) successor.
>> 
>> The patch implements (2c). What Pierre-Yves described is (2a). Can you
>> clarify which possibility you referred to?
>> 
> 
> I'm also for (2a).
> 
> 
>> 
>> The `hg strip` point is true for (2a), (2b) and (2c).
> 
> 
> It's actually not true any of (2*) if the successor is the first commit 
> in
> the stack; you'd have to separately strip any empty successors (I 
> omitted
> this detail in my previous email). That's still better than manually
> deleting obsmarkers, though. For (2a), you'd have to also `hg log 
> --hidden`
> to find the pruned successor(s).
> 
> With the current
>> evolve command, both (1) and (2a) cause the orphan to be relocated 
>> onto
>> the predecessor’s parent,
> 
> 
> It depends. If the predecessor's parent was also rebased to the same
> destination, then the current evolve command will actually rebase it to
> there.

Right, I think it would be the successor of the predecessor’s parent 
(thinking in two dimensions can be hard at times).

> and both (2b) and (2c) cause the orphan to be
>> relocated onto the empty successor. With (2c), the behavior of rebase
>> and evolve would be consistent. With (2b), the behavior of rebase and
>> evolve would be inconsistent, unless the behavior of evolve is changed
>> like you described.
>> 
> 
> I meant that we might want to make that change for the (2a) case. I 
> suspect
> that would be generally better, even though there might be false 
> positives
> where a pruned, empty successor exists for some reason even though the 
> user
> would not want the orphan to be rebased onto that successor's parent.
> 
> 
>> 
>> > You could add this to the commit message if you end up creating the
>> > obsmarkers that way.
>> 
>> Should we change the default from (1) to (2a)? If I understand
>> Pierre-Yves correctly, he proposed that. Without obsmarkers enabled, 
>> the
>> end result would be exactly the same. With obsmarkers enabled, the
>> filtered repo would be the same (except revision numbers and
>> obsmarkers), and the unfiltered repo would have some extra empty
>> changesets.
>> 
> 
> I think it would be an interesting experiment, but it should probably 
> be
> behind an [experimental] config.

What exactly do you want to have behind [experimental]? If I understand 
correctly, you didn’t propose to put the configs described below into 
[experimental].

>> For my use case, either (2b) and (2c) behind a configuration would be
>> fine. If it’s not clear whether we should implement (2b) or (2c), I 
>> can
>> implement both (so the user can opt-in to any of them).
>> 
> 
> IMO, the empty commit would be there mostly to guide evolve, so I'm not
> sure how much value there is in (2b).
> 
> I also understand if (2a) is too much scope bloat for you.

Right, it would probably be wiser if I restrict myself to (2c) for now.

>> 
>> Do you have an opinion on whether there should be one combined config
>> for rebase and absorb (in the [rewrite] section) or separate configs 
>> for
>> rebase and absorb (in their respective section)?
>> 
> 
> I'd prefer one section. Maybe two separate configs as follows would 
> make
> sense?
> 
> * rewrite.empty-commit={strip,prune,keep} decides what to do when a 
> commit
> becomes empty

I’d prefer "empty-changeset" over "empty-commit", as Mercurial uses 
"commit" only for the process of creating a new changeset. For "keep", 
"empty-changeset" makes more sense. We could make it even more explicit 
by naming it "empty-successor".

I think that "skip" would be better than "strip", as the empty changeset 
will not be created at all in this case.

> * rewrite.use-parent-if-empty=true/false decides whether to 
> rebase/evolve
> onto the parent instead if the usual destination is empty
> 
> In terms of those configs, we have:
> 
>                             |  (1)  | (2a)  | (2b)  | (2c)
> rewrite.empty-commit        | strip | prune | keep  | keep
> rewrite.use-parent-if-empty | false | true  | true  | false
> 
> 
> 
>> > On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> wrote:
>> >
>> >> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
>> >> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
>> >> > > discussion I had with Martin on another command is relevant for this
>> >> work.
>> >> > >
>> >> > > We decided that -in-all-case- creating and empty commit at
>> destination
>> >> > > was useful, even if it was to prune it automatically in the same go.
>> >> >
>> >> > So you second the global "rewrite.<opt>" config knob than
>> >> "<command>.<opt>"?
>> >> > I'm not against adding this feature.
>> >> >
>> >> > FWIW, it's helpful to include this kind of information in commit
>> message
>> >> since
>> >> > I don't follow IRC discussion or Phabricator. If you and Martin
>> stamped
>> >> this
>> >> > feature, only thing I would have to do is nitpicking.
>> >>
>> >> Another point on commit messages: in general, we should try and make
>> >> sure that the _why_ is encoded to the maximum extent possible so
>> >> future programmer-archaeologists have an easier time of things. :)
>> >>
>> >> > _______________________________________________
>> >> > Mercurial-devel mailing list
>> >> > Mercurial-devel@mercurial-scm.org
>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >> _______________________________________________
>> >> Mercurial-devel mailing list
>> >> Mercurial-devel@mercurial-scm.org
>> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >>
>> >
>> > _______________________________________________
>> > Mercurial-devel mailing list
>> > Mercurial-devel@mercurial-scm.org
>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>

On Thu, Jul 9, 2020 at 2:36 PM Manuel Jacob <me@manueljacob.de> wrote:

> On 2020-07-09 18:48, Martin von Zweigbergk wrote:
> > On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <me@manueljacob.de> wrote:
> >
> >> On 2020-07-08 01:29, Martin von Zweigbergk via Mercurial-devel wrote:
> >> > Other reasons to always create the empty successor (in addition to
> just
> >> > having useful information in `hg obslog`):
> >> >
> >> > * orphans on the predecessor would be evolved onto the successor
> >> > instead of
> >> > being evolved onto the predecessor's parent. `hg evolve` could be
> >> > taught to
> >> > evolve not evolve the orphan onto the empty commit but instead onto
> its
> >> > parent. (This may be what Pierre-Yves meant by "automatic evolution
> >> > could
> >> > behave in a better way".)
> >> > * `hg strip <root of rebased commits>` will correctly de-obsolete the
> >> > predecessor
> >>
> >> I see four possibilities:
> >>
> >> 1) We don’t create an empty successor and prune the predecessor
> >> (without
> >> successor).
> >> 2) We create an empty successor and
> >> 2a) prune the successor immediately (without successor), and rebase
> >> the
> >> children of the predecessor onto the parent of the empty, pruned
> >> successor.
> >> 2b) leave the successor, and rebase the children of the predecessor
> >> onto
> >> the parent of the empty (but not pruned) successor.
> >> 2c) leave the successor, and rebase the children of the predecessor
> >> onto
> >> the empty (but not pruned) successor.
> >>
> >> The patch implements (2c). What Pierre-Yves described is (2a). Can you
> >> clarify which possibility you referred to?
> >>
> >
> > I'm also for (2a).
> >
> >
> >>
> >> The `hg strip` point is true for (2a), (2b) and (2c).
> >
> >
> > It's actually not true any of (2*) if the successor is the first commit
> > in
> > the stack; you'd have to separately strip any empty successors (I
> > omitted
> > this detail in my previous email). That's still better than manually
> > deleting obsmarkers, though. For (2a), you'd have to also `hg log
> > --hidden`
> > to find the pruned successor(s).
> >
> > With the current
> >> evolve command, both (1) and (2a) cause the orphan to be relocated
> >> onto
> >> the predecessor’s parent,
> >
> >
> > It depends. If the predecessor's parent was also rebased to the same
> > destination, then the current evolve command will actually rebase it to
> > there.
>
> Right, I think it would be the successor of the predecessor’s parent
> (thinking in two dimensions can be hard at times).
>
> > and both (2b) and (2c) cause the orphan to be
> >> relocated onto the empty successor. With (2c), the behavior of rebase
> >> and evolve would be consistent. With (2b), the behavior of rebase and
> >> evolve would be inconsistent, unless the behavior of evolve is changed
> >> like you described.
> >>
> >
> > I meant that we might want to make that change for the (2a) case. I
> > suspect
> > that would be generally better, even though there might be false
> > positives
> > where a pruned, empty successor exists for some reason even though the
> > user
> > would not want the orphan to be rebased onto that successor's parent.
> >
> >
> >>
> >> > You could add this to the commit message if you end up creating the
> >> > obsmarkers that way.
> >>
> >> Should we change the default from (1) to (2a)? If I understand
> >> Pierre-Yves correctly, he proposed that. Without obsmarkers enabled,
> >> the
> >> end result would be exactly the same. With obsmarkers enabled, the
> >> filtered repo would be the same (except revision numbers and
> >> obsmarkers), and the unfiltered repo would have some extra empty
> >> changesets.
> >>
> >
> > I think it would be an interesting experiment, but it should probably
> > be
> > behind an [experimental] config.
>
> What exactly do you want to have behind [experimental]? If I understand
> correctly, you didn’t propose to put the configs described below into
> [experimental].
>

Hmm, I'm actually not sure. Maybe we should put those configs under
[experimental].


> >> For my use case, either (2b) and (2c) behind a configuration would be
> >> fine. If it’s not clear whether we should implement (2b) or (2c), I
> >> can
> >> implement both (so the user can opt-in to any of them).
> >>
> >
> > IMO, the empty commit would be there mostly to guide evolve, so I'm not
> > sure how much value there is in (2b).
> >
> > I also understand if (2a) is too much scope bloat for you.
>
> Right, it would probably be wiser if I restrict myself to (2c) for now.
>
> >>
> >> Do you have an opinion on whether there should be one combined config
> >> for rebase and absorb (in the [rewrite] section) or separate configs
> >> for
> >> rebase and absorb (in their respective section)?
> >>
> >
> > I'd prefer one section. Maybe two separate configs as follows would
> > make
> > sense?
> >
> > * rewrite.empty-commit={strip,prune,keep} decides what to do when a
> > commit
> > becomes empty
>
> I’d prefer "empty-changeset" over "empty-commit", as Mercurial uses
> "commit" only for the process of creating a new changeset. For "keep",
> "empty-changeset" makes more sense. We could make it even more explicit
> by naming it "empty-successor".
>
> I think that "skip" would be better than "strip", as the empty changeset
> will not be created at all in this case.
>

Those names sound good to me.


>
> > * rewrite.use-parent-if-empty=true/false decides whether to
> > rebase/evolve
> > onto the parent instead if the usual destination is empty
> >
> > In terms of those configs, we have:
> >
> >                             |  (1)  | (2a)  | (2b)  | (2c)
> > rewrite.empty-commit        | strip | prune | keep  | keep
> > rewrite.use-parent-if-empty | false | true  | true  | false
> >
> >
> >
> >> > On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> wrote:
> >> >
> >> >> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
> >> >> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
> >> >> > > discussion I had with Martin on another command is relevant for
> this
> >> >> work.
> >> >> > >
> >> >> > > We decided that -in-all-case- creating and empty commit at
> >> destination
> >> >> > > was useful, even if it was to prune it automatically in the same
> go.
> >> >> >
> >> >> > So you second the global "rewrite.<opt>" config knob than
> >> >> "<command>.<opt>"?
> >> >> > I'm not against adding this feature.
> >> >> >
> >> >> > FWIW, it's helpful to include this kind of information in commit
> >> message
> >> >> since
> >> >> > I don't follow IRC discussion or Phabricator. If you and Martin
> >> stamped
> >> >> this
> >> >> > feature, only thing I would have to do is nitpicking.
> >> >>
> >> >> Another point on commit messages: in general, we should try and make
> >> >> sure that the _why_ is encoded to the maximum extent possible so
> >> >> future programmer-archaeologists have an easier time of things. :)
> >> >>
> >> >> > _______________________________________________
> >> >> > Mercurial-devel mailing list
> >> >> > Mercurial-devel@mercurial-scm.org
> >> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >> >> _______________________________________________
> >> >> Mercurial-devel mailing list
> >> >> Mercurial-devel@mercurial-scm.org
> >> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >> >>
> >> >
> >> > _______________________________________________
> >> > Mercurial-devel mailing list
> >> > Mercurial-devel@mercurial-scm.org
> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
> >>
>

I’ll send another patch series on the mailing list.

In the meantime, I sent more rebase-related fixes, this time using 
Phabricator because Martin said he prefers it. The new patch series will 
require at least the following from Phabricator:

https://phab.mercurial-scm.org/D8724
https://phab.mercurial-scm.org/D8725
https://phab.mercurial-scm.org/D8729

Since I sent V1 of this patch series to the mailing list, it seemed 
wrong to send V2 of this patch series to Phabricator. I’m sorry that 
because of this, we end in a situation where people have to use both 
tools, but I felt like I’m excluding someone one way or another.

On 2020-07-10 06:41, Martin von Zweigbergk wrote:
> On Thu, Jul 9, 2020 at 2:36 PM Manuel Jacob <me@manueljacob.de> wrote:
> 
>> On 2020-07-09 18:48, Martin von Zweigbergk wrote:
>> > On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <me@manueljacob.de> wrote:
>> >
>> >> On 2020-07-08 01:29, Martin von Zweigbergk via Mercurial-devel wrote:
>> >> > Other reasons to always create the empty successor (in addition to
>> just
>> >> > having useful information in `hg obslog`):
>> >> >
>> >> > * orphans on the predecessor would be evolved onto the successor
>> >> > instead of
>> >> > being evolved onto the predecessor's parent. `hg evolve` could be
>> >> > taught to
>> >> > evolve not evolve the orphan onto the empty commit but instead onto
>> its
>> >> > parent. (This may be what Pierre-Yves meant by "automatic evolution
>> >> > could
>> >> > behave in a better way".)
>> >> > * `hg strip <root of rebased commits>` will correctly de-obsolete the
>> >> > predecessor
>> >>
>> >> I see four possibilities:
>> >>
>> >> 1) We don’t create an empty successor and prune the predecessor
>> >> (without
>> >> successor).
>> >> 2) We create an empty successor and
>> >> 2a) prune the successor immediately (without successor), and rebase
>> >> the
>> >> children of the predecessor onto the parent of the empty, pruned
>> >> successor.
>> >> 2b) leave the successor, and rebase the children of the predecessor
>> >> onto
>> >> the parent of the empty (but not pruned) successor.
>> >> 2c) leave the successor, and rebase the children of the predecessor
>> >> onto
>> >> the empty (but not pruned) successor.
>> >>
>> >> The patch implements (2c). What Pierre-Yves described is (2a). Can you
>> >> clarify which possibility you referred to?
>> >>
>> >
>> > I'm also for (2a).
>> >
>> >
>> >>
>> >> The `hg strip` point is true for (2a), (2b) and (2c).
>> >
>> >
>> > It's actually not true any of (2*) if the successor is the first commit
>> > in
>> > the stack; you'd have to separately strip any empty successors (I
>> > omitted
>> > this detail in my previous email). That's still better than manually
>> > deleting obsmarkers, though. For (2a), you'd have to also `hg log
>> > --hidden`
>> > to find the pruned successor(s).
>> >
>> > With the current
>> >> evolve command, both (1) and (2a) cause the orphan to be relocated
>> >> onto
>> >> the predecessor’s parent,
>> >
>> >
>> > It depends. If the predecessor's parent was also rebased to the same
>> > destination, then the current evolve command will actually rebase it to
>> > there.
>> 
>> Right, I think it would be the successor of the predecessor’s parent
>> (thinking in two dimensions can be hard at times).
>> 
>> > and both (2b) and (2c) cause the orphan to be
>> >> relocated onto the empty successor. With (2c), the behavior of rebase
>> >> and evolve would be consistent. With (2b), the behavior of rebase and
>> >> evolve would be inconsistent, unless the behavior of evolve is changed
>> >> like you described.
>> >>
>> >
>> > I meant that we might want to make that change for the (2a) case. I
>> > suspect
>> > that would be generally better, even though there might be false
>> > positives
>> > where a pruned, empty successor exists for some reason even though the
>> > user
>> > would not want the orphan to be rebased onto that successor's parent.
>> >
>> >
>> >>
>> >> > You could add this to the commit message if you end up creating the
>> >> > obsmarkers that way.
>> >>
>> >> Should we change the default from (1) to (2a)? If I understand
>> >> Pierre-Yves correctly, he proposed that. Without obsmarkers enabled,
>> >> the
>> >> end result would be exactly the same. With obsmarkers enabled, the
>> >> filtered repo would be the same (except revision numbers and
>> >> obsmarkers), and the unfiltered repo would have some extra empty
>> >> changesets.
>> >>
>> >
>> > I think it would be an interesting experiment, but it should probably
>> > be
>> > behind an [experimental] config.
>> 
>> What exactly do you want to have behind [experimental]? If I 
>> understand
>> correctly, you didn’t propose to put the configs described below into
>> [experimental].
>> 
> 
> Hmm, I'm actually not sure. Maybe we should put those configs under
> [experimental].
> 
> 
>> >> For my use case, either (2b) and (2c) behind a configuration would be
>> >> fine. If it’s not clear whether we should implement (2b) or (2c), I
>> >> can
>> >> implement both (so the user can opt-in to any of them).
>> >>
>> >
>> > IMO, the empty commit would be there mostly to guide evolve, so I'm not
>> > sure how much value there is in (2b).
>> >
>> > I also understand if (2a) is too much scope bloat for you.
>> 
>> Right, it would probably be wiser if I restrict myself to (2c) for 
>> now.
>> 
>> >>
>> >> Do you have an opinion on whether there should be one combined config
>> >> for rebase and absorb (in the [rewrite] section) or separate configs
>> >> for
>> >> rebase and absorb (in their respective section)?
>> >>
>> >
>> > I'd prefer one section. Maybe two separate configs as follows would
>> > make
>> > sense?
>> >
>> > * rewrite.empty-commit={strip,prune,keep} decides what to do when a
>> > commit
>> > becomes empty
>> 
>> I’d prefer "empty-changeset" over "empty-commit", as Mercurial uses
>> "commit" only for the process of creating a new changeset. For "keep",
>> "empty-changeset" makes more sense. We could make it even more 
>> explicit
>> by naming it "empty-successor".
>> 
>> I think that "skip" would be better than "strip", as the empty 
>> changeset
>> will not be created at all in this case.
>> 
> 
> Those names sound good to me.
> 
> 
>> 
>> > * rewrite.use-parent-if-empty=true/false decides whether to
>> > rebase/evolve
>> > onto the parent instead if the usual destination is empty
>> >
>> > In terms of those configs, we have:
>> >
>> >                             |  (1)  | (2a)  | (2b)  | (2c)
>> > rewrite.empty-commit        | strip | prune | keep  | keep
>> > rewrite.use-parent-if-empty | false | true  | true  | false
>> >
>> >
>> >
>> >> > On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> wrote:
>> >> >
>> >> >> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
>> >> >> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
>> >> >> > > discussion I had with Martin on another command is relevant for
>> this
>> >> >> work.
>> >> >> > >
>> >> >> > > We decided that -in-all-case- creating and empty commit at
>> >> destination
>> >> >> > > was useful, even if it was to prune it automatically in the same
>> go.
>> >> >> >
>> >> >> > So you second the global "rewrite.<opt>" config knob than
>> >> >> "<command>.<opt>"?
>> >> >> > I'm not against adding this feature.
>> >> >> >
>> >> >> > FWIW, it's helpful to include this kind of information in commit
>> >> message
>> >> >> since
>> >> >> > I don't follow IRC discussion or Phabricator. If you and Martin
>> >> stamped
>> >> >> this
>> >> >> > feature, only thing I would have to do is nitpicking.
>> >> >>
>> >> >> Another point on commit messages: in general, we should try and make
>> >> >> sure that the _why_ is encoded to the maximum extent possible so
>> >> >> future programmer-archaeologists have an easier time of things. :)
>> >> >>
>> >> >> > _______________________________________________
>> >> >> > Mercurial-devel mailing list
>> >> >> > Mercurial-devel@mercurial-scm.org
>> >> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >> >> _______________________________________________
>> >> >> Mercurial-devel mailing list
>> >> >> Mercurial-devel@mercurial-scm.org
>> >> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >> >>
>> >> >
>> >> > _______________________________________________
>> >> > Mercurial-devel mailing list
>> >> > Mercurial-devel@mercurial-scm.org
>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >>
>>

The patches in Phabricator were landed (thank you, Martin!), i.e. the 
patch series in 
https://www.mercurial-scm.org/pipermail/mercurial-devel/2020-July/142422.html 
now applies onto `@`. Yuya, since you reviewed V1, feel free to review 
V2 as well. If you want to leave it to Martin, I can submit them to 
Phabricator.

On 2020-07-12 07:13, Manuel Jacob wrote:
> I’ll send another patch series on the mailing list.
> 
> In the meantime, I sent more rebase-related fixes, this time using
> Phabricator because Martin said he prefers it. The new patch series
> will require at least the following from Phabricator:
> 
> https://phab.mercurial-scm.org/D8724
> https://phab.mercurial-scm.org/D8725
> https://phab.mercurial-scm.org/D8729
> 
> Since I sent V1 of this patch series to the mailing list, it seemed
> wrong to send V2 of this patch series to Phabricator. I’m sorry that
> because of this, we end in a situation where people have to use both
> tools, but I felt like I’m excluding someone one way or another.
> 
> On 2020-07-10 06:41, Martin von Zweigbergk wrote:
>> On Thu, Jul 9, 2020 at 2:36 PM Manuel Jacob <me@manueljacob.de> wrote:
>> 
>>> On 2020-07-09 18:48, Martin von Zweigbergk wrote:
>>> > On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <me@manueljacob.de> wrote:
>>> >
>>> >> On 2020-07-08 01:29, Martin von Zweigbergk via Mercurial-devel wrote:
>>> >> > Other reasons to always create the empty successor (in addition to
>>> just
>>> >> > having useful information in `hg obslog`):
>>> >> >
>>> >> > * orphans on the predecessor would be evolved onto the successor
>>> >> > instead of
>>> >> > being evolved onto the predecessor's parent. `hg evolve` could be
>>> >> > taught to
>>> >> > evolve not evolve the orphan onto the empty commit but instead onto
>>> its
>>> >> > parent. (This may be what Pierre-Yves meant by "automatic evolution
>>> >> > could
>>> >> > behave in a better way".)
>>> >> > * `hg strip <root of rebased commits>` will correctly de-obsolete the
>>> >> > predecessor
>>> >>
>>> >> I see four possibilities:
>>> >>
>>> >> 1) We don’t create an empty successor and prune the predecessor
>>> >> (without
>>> >> successor).
>>> >> 2) We create an empty successor and
>>> >> 2a) prune the successor immediately (without successor), and rebase
>>> >> the
>>> >> children of the predecessor onto the parent of the empty, pruned
>>> >> successor.
>>> >> 2b) leave the successor, and rebase the children of the predecessor
>>> >> onto
>>> >> the parent of the empty (but not pruned) successor.
>>> >> 2c) leave the successor, and rebase the children of the predecessor
>>> >> onto
>>> >> the empty (but not pruned) successor.
>>> >>
>>> >> The patch implements (2c). What Pierre-Yves described is (2a). Can you
>>> >> clarify which possibility you referred to?
>>> >>
>>> >
>>> > I'm also for (2a).
>>> >
>>> >
>>> >>
>>> >> The `hg strip` point is true for (2a), (2b) and (2c).
>>> >
>>> >
>>> > It's actually not true any of (2*) if the successor is the first commit
>>> > in
>>> > the stack; you'd have to separately strip any empty successors (I
>>> > omitted
>>> > this detail in my previous email). That's still better than manually
>>> > deleting obsmarkers, though. For (2a), you'd have to also `hg log
>>> > --hidden`
>>> > to find the pruned successor(s).
>>> >
>>> > With the current
>>> >> evolve command, both (1) and (2a) cause the orphan to be relocated
>>> >> onto
>>> >> the predecessor’s parent,
>>> >
>>> >
>>> > It depends. If the predecessor's parent was also rebased to the same
>>> > destination, then the current evolve command will actually rebase it to
>>> > there.
>>> 
>>> Right, I think it would be the successor of the predecessor’s parent
>>> (thinking in two dimensions can be hard at times).
>>> 
>>> > and both (2b) and (2c) cause the orphan to be
>>> >> relocated onto the empty successor. With (2c), the behavior of rebase
>>> >> and evolve would be consistent. With (2b), the behavior of rebase and
>>> >> evolve would be inconsistent, unless the behavior of evolve is changed
>>> >> like you described.
>>> >>
>>> >
>>> > I meant that we might want to make that change for the (2a) case. I
>>> > suspect
>>> > that would be generally better, even though there might be false
>>> > positives
>>> > where a pruned, empty successor exists for some reason even though the
>>> > user
>>> > would not want the orphan to be rebased onto that successor's parent.
>>> >
>>> >
>>> >>
>>> >> > You could add this to the commit message if you end up creating the
>>> >> > obsmarkers that way.
>>> >>
>>> >> Should we change the default from (1) to (2a)? If I understand
>>> >> Pierre-Yves correctly, he proposed that. Without obsmarkers enabled,
>>> >> the
>>> >> end result would be exactly the same. With obsmarkers enabled, the
>>> >> filtered repo would be the same (except revision numbers and
>>> >> obsmarkers), and the unfiltered repo would have some extra empty
>>> >> changesets.
>>> >>
>>> >
>>> > I think it would be an interesting experiment, but it should probably
>>> > be
>>> > behind an [experimental] config.
>>> 
>>> What exactly do you want to have behind [experimental]? If I 
>>> understand
>>> correctly, you didn’t propose to put the configs described below into
>>> [experimental].
>>> 
>> 
>> Hmm, I'm actually not sure. Maybe we should put those configs under
>> [experimental].
>> 
>> 
>>> >> For my use case, either (2b) and (2c) behind a configuration would be
>>> >> fine. If it’s not clear whether we should implement (2b) or (2c), I
>>> >> can
>>> >> implement both (so the user can opt-in to any of them).
>>> >>
>>> >
>>> > IMO, the empty commit would be there mostly to guide evolve, so I'm not
>>> > sure how much value there is in (2b).
>>> >
>>> > I also understand if (2a) is too much scope bloat for you.
>>> 
>>> Right, it would probably be wiser if I restrict myself to (2c) for 
>>> now.
>>> 
>>> >>
>>> >> Do you have an opinion on whether there should be one combined config
>>> >> for rebase and absorb (in the [rewrite] section) or separate configs
>>> >> for
>>> >> rebase and absorb (in their respective section)?
>>> >>
>>> >
>>> > I'd prefer one section. Maybe two separate configs as follows would
>>> > make
>>> > sense?
>>> >
>>> > * rewrite.empty-commit={strip,prune,keep} decides what to do when a
>>> > commit
>>> > becomes empty
>>> 
>>> I’d prefer "empty-changeset" over "empty-commit", as Mercurial uses
>>> "commit" only for the process of creating a new changeset. For 
>>> "keep",
>>> "empty-changeset" makes more sense. We could make it even more 
>>> explicit
>>> by naming it "empty-successor".
>>> 
>>> I think that "skip" would be better than "strip", as the empty 
>>> changeset
>>> will not be created at all in this case.
>>> 
>> 
>> Those names sound good to me.
>> 
>> 
>>> 
>>> > * rewrite.use-parent-if-empty=true/false decides whether to
>>> > rebase/evolve
>>> > onto the parent instead if the usual destination is empty
>>> >
>>> > In terms of those configs, we have:
>>> >
>>> >                             |  (1)  | (2a)  | (2b)  | (2c)
>>> > rewrite.empty-commit        | strip | prune | keep  | keep
>>> > rewrite.use-parent-if-empty | false | true  | true  | false
>>> >
>>> >
>>> >
>>> >> > On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> wrote:
>>> >> >
>>> >> >> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
>>> >> >> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
>>> >> >> > > discussion I had with Martin on another command is relevant for
>>> this
>>> >> >> work.
>>> >> >> > >
>>> >> >> > > We decided that -in-all-case- creating and empty commit at
>>> >> destination
>>> >> >> > > was useful, even if it was to prune it automatically in the same
>>> go.
>>> >> >> >
>>> >> >> > So you second the global "rewrite.<opt>" config knob than
>>> >> >> "<command>.<opt>"?
>>> >> >> > I'm not against adding this feature.
>>> >> >> >
>>> >> >> > FWIW, it's helpful to include this kind of information in commit
>>> >> message
>>> >> >> since
>>> >> >> > I don't follow IRC discussion or Phabricator. If you and Martin
>>> >> stamped
>>> >> >> this
>>> >> >> > feature, only thing I would have to do is nitpicking.
>>> >> >>
>>> >> >> Another point on commit messages: in general, we should try and make
>>> >> >> sure that the _why_ is encoded to the maximum extent possible so
>>> >> >> future programmer-archaeologists have an easier time of things. :)
>>> >> >>
>>> >> >> > _______________________________________________
>>> >> >> > Mercurial-devel mailing list
>>> >> >> > Mercurial-devel@mercurial-scm.org
>>> >> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >> _______________________________________________
>>> >> >> Mercurial-devel mailing list
>>> >> >> Mercurial-devel@mercurial-scm.org
>>> >> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >>
>>> >> >
>>> >> > _______________________________________________
>>> >> > Mercurial-devel mailing list
>>> >> > Mercurial-devel@mercurial-scm.org
>>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >>
>>> 
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

On 7/12/20 7:13 AM, Manuel Jacob wrote:
> I’ll send another patch series on the mailing list.
>
> In the meantime, I sent more rebase-related fixes, this time using 
> Phabricator because Martin said he prefers it. The new patch series 
> will require at least the following from Phabricator:
>
> https://phab.mercurial-scm.org/D8724
> https://phab.mercurial-scm.org/D8725
> https://phab.mercurial-scm.org/D8729
>
> Since I sent V1 of this patch series to the mailing list, it seemed 
> wrong to send V2 of this patch series to Phabricator. I’m sorry that 
> because of this, we end in a situation where people have to use both 
> tools, but I felt like I’m excluding someone one way or another.
>
> On 2020-07-10 06:41, Martin von Zweigbergk wrote:
>> On Thu, Jul 9, 2020 at 2:36 PM Manuel Jacob <me@manueljacob.de> wrote:
>>
>>> On 2020-07-09 18:48, Martin von Zweigbergk wrote:
>>> > On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <me@manueljacob.de> 
>>> wrote:
>>> >
>>> >> On 2020-07-08 01:29, Martin von Zweigbergk via Mercurial-devel 
>>> wrote:
>>> >> > Other reasons to always create the empty successor (in addition to
>>> just
>>> >> > having useful information in `hg obslog`):
>>> >> >
>>> >> > * orphans on the predecessor would be evolved onto the successor
>>> >> > instead of
>>> >> > being evolved onto the predecessor's parent. `hg evolve` could be
>>> >> > taught to
>>> >> > evolve not evolve the orphan onto the empty commit but instead 
>>> onto
>>> its
>>> >> > parent. (This may be what Pierre-Yves meant by "automatic 
>>> evolution
>>> >> > could
>>> >> > behave in a better way".)
>>> >> > * `hg strip <root of rebased commits>` will correctly 
>>> de-obsolete the
>>> >> > predecessor
>>> >>
>>> >> I see four possibilities:
>>> >>
>>> >> 1) We don’t create an empty successor and prune the predecessor
>>> >> (without
>>> >> successor).
>>> >> 2) We create an empty successor and
>>> >> 2a) prune the successor immediately (without successor), and 
>>> rebase the
>>> >> children of the predecessor onto the parent of the empty, pruned 
>>> successor.
>>> >> 2b) leave the successor, and rebase the children of the 
>>> predecessor onto
>>> >> the parent of the empty (but not pruned) successor.
>>> >> 2c) leave the successor, and rebase the children of the 
>>> predecessor onto
>>> >> the empty (but not pruned) successor.
>>> >>
>>> >> The patch implements (2c). What Pierre-Yves described is (2a). 
>>> Can you
>>> >> clarify which possibility you referred to?
>>> >>
>>> >
>>> > I'm also for (2a).
>>> >
>>> >
>>> >>
>>> >> The `hg strip` point is true for (2a), (2b) and (2c).
>>> >
>>> >
>>> > It's actually not true any of (2*) if the successor is the first 
>>> commit
>>> > in
>>> > the stack; you'd have to separately strip any empty successors (I
>>> > omitted
>>> > this detail in my previous email). That's still better than manually
>>> > deleting obsmarkers, though. For (2a), you'd have to also `hg log
>>> > --hidden`
>>> > to find the pruned successor(s).


Actually, case 2cpass the bar here. Since the base of the stack is still 
the topological base of the stack.

However,  strip is probably not a UX we should optimize since it has 
many know short coming (and the whole evolve effort is here to fix some 
of theses shortcoming.


>>> > With the current
>>> >> evolve command, both (1) and (2a) cause the orphan to be relocated
>>> >> onto
>>> >> the predecessor’s parent,
>>> >
>>> >
>>> > It depends. If the predecessor's parent was also rebased to the same
>>> > destination, then the current evolve command will actually rebase 
>>> it to
>>> > there.
>>>
>>> Right, I think it would be the successor of the predecessor’s parent
>>> (thinking in two dimensions can be hard at times).
>>>
>>> > and both (2b) and (2c) cause the orphan to be
>>> >> relocated onto the empty successor. With (2c), the behavior of 
>>> rebase
>>> >> and evolve would be consistent. With (2b), the behavior of rebase 
>>> and
>>> >> evolve would be inconsistent, unless the behavior of evolve is 
>>> changed
>>> >> like you described.
>>> >>
>>> >
>>> > I meant that we might want to make that change for the (2a) case. I
>>> > suspect
>>> > that would be generally better, even though there might be false
>>> > positives
>>> > where a pruned, empty successor exists for some reason even though 
>>> the
>>> > user
>>> > would not want the orphan to be rebased onto that successor's parent.


Yeah I think playing with the idea would be useful. Something else I 
have in mind for a while is to have some explicite "extract" marker for 
when an operation "extract" a changeset from a stack. (to signal 
descendant that they should evolve on the parent, recursively).

This is unrelated to this series, but could help with the case we 
discuss. In this case it would:

rebase, create an empty changeset and prune it with an extract flag on 
the marker. As a result the orphan descendant can follow the prune as 
"meaningful".


>>> >
>>> >
>>> >>
>>> >> > You could add this to the commit message if you end up creating 
>>> the
>>> >> > obsmarkers that way.
>>> >>
>>> >> Should we change the default from (1) to (2a)? If I understand
>>> >> Pierre-Yves correctly, he proposed that. Without obsmarkers enabled,
>>> >> the
>>> >> end result would be exactly the same. With obsmarkers enabled, the
>>> >> filtered repo would be the same (except revision numbers and
>>> >> obsmarkers), and the unfiltered repo would have some extra empty
>>> >> changesets.
>>> >>
>>> >
>>> > I think it would be an interesting experiment, but it should probably
>>> > be
>>> > behind an [experimental] config.
>>>
>>> What exactly do you want to have behind [experimental]? If I understand
>>> correctly, you didn’t propose to put the configs described below into
>>> [experimental].
>>>
>>
>> Hmm, I'm actually not sure. Maybe we should put those configs under
>> [experimental].
>>
>>
>>> >> For my use case, either (2b) and (2c) behind a configuration 
>>> would be
>>> >> fine. If it’s not clear whether we should implement (2b) or (2c), I
>>> >> can
>>> >> implement both (so the user can opt-in to any of them).
>>> >>
>>> >
>>> > IMO, the empty commit would be there mostly to guide evolve, so 
>>> I'm not
>>> > sure how much value there is in (2b).
>>> >
>>> > I also understand if (2a) is too much scope bloat for you.
>>>
>>> Right, it would probably be wiser if I restrict myself to (2c) for now.
>>>
>>> >>
>>> >> Do you have an opinion on whether there should be one combined 
>>> config
>>> >> for rebase and absorb (in the [rewrite] section) or separate configs
>>> >> for
>>> >> rebase and absorb (in their respective section)?
>>> >>
>>> >
>>> > I'd prefer one section. Maybe two separate configs as follows would
>>> > make
>>> > sense?
>>> >
>>> > * rewrite.empty-commit={strip,prune,keep} decides what to do when a
>>> > commit
>>> > becomes empty
>>>
>>> I’d prefer "empty-changeset" over "empty-commit", as Mercurial uses
>>> "commit" only for the process of creating a new changeset. For "keep",
>>> "empty-changeset" makes more sense. We could make it even more explicit
>>> by naming it "empty-successor".
>>>
>>> I think that "skip" would be better than "strip", as the empty 
>>> changeset
>>> will not be created at all in this case.
>>>
>>
>> Those names sound good to me.
>>
>>
>>>
>>> > * rewrite.use-parent-if-empty=true/false decides whether to
>>> > rebase/evolve
>>> > onto the parent instead if the usual destination is empty
>>> >
>>> > In terms of those configs, we have:
>>> >
>>> >                             |  (1)  | (2a)  | (2b)  | (2c)
>>> > rewrite.empty-commit        | strip | prune | keep  | keep
>>> > rewrite.use-parent-if-empty | false | true  | true  | false


I don't think 2b bring much value, so I would drop it, focusing on 
single config to select 2a and 2c. We can probably drop (1) in favor of 
(2a) too.


>>> >> > On Tue, Jul 7, 2020 at 3:11 PM Augie Fackler <raf@durin42.com> 
>>> wrote:
>>> >> >
>>> >> >> On Fri, Jul 03, 2020 at 10:32:56PM +0900, Yuya Nishihara wrote:
>>> >> >> > On Fri, 3 Jul 2020 15:08:17 +0200, Pierre-Yves David wrote:
>>> >> >> > > discussion I had with Martin on another command is 
>>> relevant for
>>> this
>>> >> >> work.
>>> >> >> > >
>>> >> >> > > We decided that -in-all-case- creating and empty commit at
>>> >> destination
>>> >> >> > > was useful, even if it was to prune it automatically in 
>>> the same
>>> go.
>>> >> >> >
>>> >> >> > So you second the global "rewrite.<opt>" config knob than
>>> >> >> "<command>.<opt>"?
>>> >> >> > I'm not against adding this feature.
>>> >> >> >
>>> >> >> > FWIW, it's helpful to include this kind of information in 
>>> commit
>>> >> message
>>> >> >> since
>>> >> >> > I don't follow IRC discussion or Phabricator. If you and Martin
>>> >> stamped
>>> >> >> this
>>> >> >> > feature, only thing I would have to do is nitpicking.
>>> >> >>
>>> >> >> Another point on commit messages: in general, we should try 
>>> and make
>>> >> >> sure that the _why_ is encoded to the maximum extent possible so
>>> >> >> future programmer-archaeologists have an easier time of 
>>> things. :)
>>> >> >>
>>> >> >> > _______________________________________________
>>> >> >> > Mercurial-devel mailing list
>>> >> >> > Mercurial-devel@mercurial-scm.org
>>> >> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >> _______________________________________________
>>> >> >> Mercurial-devel mailing list
>>> >> >> Mercurial-devel@mercurial-scm.org
>>> >> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >>
>>> >> >
>>> >> > _______________________________________________
>>> >> > Mercurial-devel mailing list
>>> >> > Mercurial-devel@mercurial-scm.org
>>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >>
>>>