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

[Manuel Jacob]

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 at 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.