[PATCH 1 of 4] rebase: rename variable "keepbranch" to "keepbranchchange"

classic Classic list List threaded Threaded
20 messages Options
Reply | Threaded
Open this post in threaded view
|

[PATCH 1 of 4] rebase: rename variable "keepbranch" to "keepbranchchange"

Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1593579865 -7200
#      Wed Jul 01 07:04:25 2020 +0200
# Node ID ee204ca1c4d68d004d4f90b5fc9edf66c9db6bae
# Parent  24b1a8eb73aa8203c94f0a168cb7ec27ea26cef9
# EXP-Topic keepbranchchange
rebase: rename variable "keepbranch" to "keepbranchchange"

When looking at the use of the "keepbranch" variable, I was confused because it
sounded like the variable is True iff --keepbranches is passed. Only after
looking at the variable definition, I realized that the variable is True iff
the new changeset will change the branch name. The new name "keepbranchchange"
clarifies that.

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -529,10 +529,12 @@
         extra = {b'rebase_source': ctx.hex()}
         for c in self.extrafns:
             c(ctx, extra)
-        keepbranch = self.keepbranchesf and repo[p1].branch() != ctx.branch()
+        keepbranchchange = (
+            self.keepbranchesf and repo[p1].branch() != ctx.branch()
+        )
         destphase = max(ctx.phase(), phases.draft)
         overrides = {(b'phases', b'new-commit'): destphase}
-        if keepbranch:
+        if keepbranchchange:
             overrides[(b'ui', b'allowemptycommit')] = True
         with repo.ui.configoverride(overrides, b'rebase'):
             if self.inmemory:

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# 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.

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'):
             overrides[(b'ui', b'allowemptycommit')] = True
         with repo.ui.configoverride(overrides, b'rebase'):
             if self.inmemory:
@@ -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()
+                ):
+                    ui.warn(
+                        _(
+                            b'note: created empty changeset for %s, its '
+                            b'destination already has all its changes\n'
+                        )
+                        % desc
+                    )
             else:
                 if not self.collapsef:
                     ui.warn(
diff --git a/mercurial/configitems.py b/mercurial/configitems.py
--- a/mercurial/configitems.py
+++ b/mercurial/configitems.py
@@ -1573,3 +1573,6 @@
 coreconfigitem(
     b'rebase', b'experimental.inmemory', default=False,
 )
+coreconfigitem(
+    b'rebase', b'createempty', default=False,
+)
diff --git a/tests/test-rebase-createempty.t b/tests/test-rebase-createempty.t
new file mode 100644
--- /dev/null
+++ b/tests/test-rebase-createempty.t
@@ -0,0 +1,44 @@
+  $ cat << EOF >> $HGRCPATH
+  > [extensions]
+  > rebase=
+  > [alias]
+  > tglog = log -G -T "{rev} '{desc}'\n"
+  > EOF
+
+  $ hg init
+
+  $ echo a > a; hg add a; hg ci -m a
+  $ echo b > b; hg add b; hg ci -m b1
+  $ hg up 0 -q
+  $ echo b > b; hg add b; hg ci -m b2 -q
+
+  $ hg tglog
+  @  2 'b2'
+  |
+  | o  1 'b1'
+  |/
+  o  0 'a'
+  
+
+Without the rebase.createempty config, b2 is dropped because it would become empty.
+
+  $ hg rebase -s 2 -d 1 --dry-run
+  starting dry-run rebase; repository will not be changed
+  rebasing 2:6e2aad5e0f3c "b2" (tip)
+  note: not rebasing 2:6e2aad5e0f3c "b2" (tip), its destination already has all its changes
+  dry-run rebase completed successfully; run without -n/--dry-run to perform this rebase
+
+With the rebase.createempty config, b2 will be recreated although it became empty.
+
+  $ hg rebase -s 2 -d 1 --config rebase.createempty=True
+  rebasing 2:6e2aad5e0f3c "b2" (tip)
+  note: created empty changeset for 2:6e2aad5e0f3c "b2" (tip), its destination already has all its changes
+  saved backup bundle to $TESTTMP/.hg/strip-backup/6e2aad5e0f3c-7d7c8801-rebase.hg
+
+  $ hg tglog
+  @  2 'b2'
+  |
+  o  1 'b1'
+  |
+  o  0 'a'
+  
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 3 of 4] absorb: add config that causes empty changesets to be created

Manuel Jacob
In reply to this post by Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1590997361 -7200
#      Mon Jun 01 09:42:41 2020 +0200
# Node ID b54d68403a5435161e294b9227bfb5059a25b3f0
# Parent  45fa02da6fd1c3b40350329d80d6fe03c83f9a30
# EXP-Topic createempty
absorb: add config that causes empty changesets to be created

The design and the motivation for the new absorb.createempty config is the same
as for the rebase.createempty config.

diff --git a/hgext/absorb.py b/hgext/absorb.py
--- a/hgext/absorb.py
+++ b/hgext/absorb.py
@@ -70,6 +70,7 @@
 configitem(b'absorb', b'add-noise', default=True)
 configitem(b'absorb', b'amend-flag', default=None)
 configitem(b'absorb', b'max-stack-size', default=50)
+configitem(b'absorb', b'createempty', default=False)
 
 colortable = {
     b'absorb.description': b'yellow',
@@ -782,8 +783,10 @@
                 # nothing changed, nothing commited
                 nextp1 = ctx
                 continue
-            if ctx.files() and self._willbecomenoop(
-                memworkingcopy, ctx, nextp1
+            if (
+                not self.ui.configbool(b'absorb', b'createempty')
+                and ctx.files()
+                and self._willbecomenoop(memworkingcopy, ctx, nextp1)
             ):
                 # changeset is no longer necessary
                 self.replacemap[ctx.node()] = None
diff --git a/tests/test-absorb.t b/tests/test-absorb.t
--- a/tests/test-absorb.t
+++ b/tests/test-absorb.t
@@ -490,6 +490,71 @@
      +3
   
 
+The absorb.createempty config causes empty changesets to get committed:
+
+  $ cd ..
+  $ hg init repo4a
+  $ cd repo4a
+  $ cat > a <<EOF
+  > 1
+  > 2
+  > EOF
+  $ hg commit -m a12 -A a
+  $ cat > b <<EOF
+  > 1
+  > 2
+  > EOF
+  $ hg commit -m b12 -A b
+  $ echo 3 >> b
+  $ hg commit -m b3
+  $ echo 4 >> b
+  $ hg commit -m b4
+  $ echo 1 > b
+  $ echo 3 >> a
+  $ hg absorb -pn
+  showing changes for a
+          @@ -2,0 +2,1 @@
+  bfafb49 +3
+  showing changes for b
+          @@ -1,3 +1,0 @@
+  1154859 -2
+  30970db -3
+  a393a58 -4
+  
+  4 changesets affected
+  a393a58 b4
+  30970db b3
+  1154859 b12
+  bfafb49 a12
+  $ hg absorb -av --config absorb.createempty=True | grep became
+  0:bfafb49242db: 1 file(s) changed, became 4:1a2de97fc652
+  1:115485984805: 2 file(s) changed, became 5:0c930dfab74c
+  2:30970dbf7b40: 2 file(s) changed, became 6:df6574ae635c
+  3:a393a58b9a85: 2 file(s) changed, became 7:ad4bd3462c9e
+  $ hg log -T '{rev} {desc}\n' -Gp
+  @  7 b4
+  |
+  o  6 b3
+  |
+  o  5 b12
+  |  diff --git a/b b/b
+  |  new file mode 100644
+  |  --- /dev/null
+  |  +++ b/b
+  |  @@ -0,0 +1,1 @@
+  |  +1
+  |
+  o  4 a12
+     diff --git a/a b/a
+     new file mode 100644
+     --- /dev/null
+     +++ b/a
+     @@ -0,0 +1,3 @@
+     +1
+     +2
+     +3
+  
+
 Use revert to make the current change and its parent disappear.
 This should move us to the non-obsolete ancestor.
 

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

[PATCH 4 of 4] absorb: make it explicit if empty changeset was created

Manuel Jacob
In reply to this post by Manuel Jacob
# HG changeset patch
# User Manuel Jacob <[hidden email]>
# Date 1590998131 -7200
#      Mon Jun 01 09:55:31 2020 +0200
# Node ID 1328059c6ff3a68346429579a2920ffb959ec3e4
# Parent  b54d68403a5435161e294b9227bfb5059a25b3f0
# EXP-Topic createempty
absorb: make it explicit if empty changeset was created

If the absorb.createempty config is disabled, a message "became empty and was
dropped" is shown if the changeset became empty. If the absorb.createempty
config is enabled, absorb may create changesets even if they became empty. It’s
probably a good idea to make that explicit. Therefore the message is changed to
be a combination of both: "became empty and became ...".

Repeating the word "became" is not very elegant. This results from the fact
that "became" was and is overloaded to indicate both the change from non-empty
to empty and the successor relation. In the combinated message, both meanings
are used in one sentence.

diff --git a/hgext/absorb.py b/hgext/absorb.py
--- a/hgext/absorb.py
+++ b/hgext/absorb.py
@@ -783,10 +783,12 @@
                 # nothing changed, nothing commited
                 nextp1 = ctx
                 continue
+            willbecomenoop = ctx.files() and self._willbecomenoop(
+                memworkingcopy, ctx, nextp1
+            )
             if (
                 not self.ui.configbool(b'absorb', b'createempty')
-                and ctx.files()
-                and self._willbecomenoop(memworkingcopy, ctx, nextp1)
+                and willbecomenoop
             ):
                 # changeset is no longer necessary
                 self.replacemap[ctx.node()] = None
@@ -798,7 +800,14 @@
                 nextp1 = lastcommitted
                 self.replacemap[ctx.node()] = lastcommitted.node()
                 if memworkingcopy:
-                    msg = _(b'%d file(s) changed, became %s') % (
+                    if willbecomenoop:
+                        msg = _(
+                            b'%d file(s) changed, became empty '
+                            b'and became %s'
+                        )
+                    else:
+                        msg = _(b'%d file(s) changed, became %s')
+                    msg = msg % (
                         len(memworkingcopy),
                         self._ctx2str(lastcommitted),
                     )
diff --git a/tests/test-absorb.t b/tests/test-absorb.t
--- a/tests/test-absorb.t
+++ b/tests/test-absorb.t
@@ -509,6 +509,7 @@
   $ hg commit -m b3
   $ echo 4 >> b
   $ hg commit -m b4
+  $ hg commit -m empty --config ui.allowemptycommit=True
   $ echo 1 > b
   $ echo 3 >> a
   $ hg absorb -pn
@@ -527,16 +528,19 @@
   1154859 b12
   bfafb49 a12
   $ hg absorb -av --config absorb.createempty=True | grep became
-  0:bfafb49242db: 1 file(s) changed, became 4:1a2de97fc652
-  1:115485984805: 2 file(s) changed, became 5:0c930dfab74c
-  2:30970dbf7b40: 2 file(s) changed, became 6:df6574ae635c
-  3:a393a58b9a85: 2 file(s) changed, became 7:ad4bd3462c9e
+  0:bfafb49242db: 1 file(s) changed, became 5:1a2de97fc652
+  1:115485984805: 2 file(s) changed, became 6:0c930dfab74c
+  2:30970dbf7b40: 2 file(s) changed, became empty and became 7:df6574ae635c
+  3:a393a58b9a85: 2 file(s) changed, became empty and became 8:ad4bd3462c9e
+  4:1bb0e8cff87a: 2 file(s) changed, became 9:2dbed75af996
   $ hg log -T '{rev} {desc}\n' -Gp
-  @  7 b4
+  @  9 empty
+  |
+  o  8 b4
   |
-  o  6 b3
+  o  7 b3
   |
-  o  5 b12
+  o  6 b12
   |  diff --git a/b b/b
   |  new file mode 100644
   |  --- /dev/null
@@ -544,7 +548,7 @@
   |  @@ -0,0 +1,1 @@
   |  +1
   |
-  o  4 a12
+  o  5 a12
      diff --git a/a b/a
      new file mode 100644
      --- /dev/null
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Yuya Nishihara
In reply to this post by Manuel Jacob
On Wed, 01 Jul 2020 23:39:01 +0200, Manuel Jacob wrote:

> # HG changeset patch
> # User Manuel Jacob <[hidden email]>
> # 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?
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

Re: [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 <[hidden email]>
>> # 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.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Yuya Nishihara
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
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Pierre-Yves David-2
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
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>

--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Yuya Nishihara
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.
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Pierre-Yves David-5
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.

--
Pierre-Yves David

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Pierre-Yves David-2
In reply to this post by Yuya Nishihara
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.
>

--
Pierre-Yves David
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Augie Fackler-2
In reply to this post by Yuya Nishihara
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
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Martin von Zweigbergk via 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 <[hidden email]> 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
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Manuel Jacob
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 <[hidden email]> 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
>> > [hidden email]
>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> _______________________________________________
>> Mercurial-devel mailing list
>> [hidden email]
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Martin von Zweigbergk via Mercurial-devel


On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <[hidden email]> 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 <[hidden email]> 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
>> > [hidden email]
>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> _______________________________________________
>> Mercurial-devel mailing list
>> [hidden email]
>> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
>
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Manuel Jacob
On 2020-07-09 18:48, Martin von Zweigbergk wrote:

> On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <[hidden email]> 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 <[hidden email]> 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
>> >> > [hidden email]
>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >> _______________________________________________
>> >> Mercurial-devel mailing list
>> >> [hidden email]
>> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >>
>> >
>> > _______________________________________________
>> > Mercurial-devel mailing list
>> > [hidden email]
>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Martin von Zweigbergk via Mercurial-devel


On Thu, Jul 9, 2020 at 2:36 PM Manuel Jacob <[hidden email]> wrote:
On 2020-07-09 18:48, Martin von Zweigbergk wrote:
> On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <[hidden email]> 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 <[hidden email]> 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
>> >> > [hidden email]
>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >> _______________________________________________
>> >> Mercurial-devel mailing list
>> >> [hidden email]
>> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >>
>> >
>> > _______________________________________________
>> > Mercurial-devel mailing list
>> > [hidden email]
>> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Manuel Jacob
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 <[hidden email]> wrote:
>
>> On 2020-07-09 18:48, Martin von Zweigbergk wrote:
>> > On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <[hidden email]> 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 <[hidden email]> 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
>> >> >> > [hidden email]
>> >> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >> >> _______________________________________________
>> >> >> Mercurial-devel mailing list
>> >> >> [hidden email]
>> >> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >> >>
>> >> >
>> >> > _______________________________________________
>> >> > Mercurial-devel mailing list
>> >> > [hidden email]
>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>> >>
>>
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Manuel Jacob
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 <[hidden email]> wrote:
>>
>>> On 2020-07-09 18:48, Martin von Zweigbergk wrote:
>>> > On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <[hidden email]> 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 <[hidden email]> 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
>>> >> >> > [hidden email]
>>> >> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >> _______________________________________________
>>> >> >> Mercurial-devel mailing list
>>> >> >> [hidden email]
>>> >> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >>
>>> >> >
>>> >> > _______________________________________________
>>> >> > Mercurial-devel mailing list
>>> >> > [hidden email]
>>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >>
>>>
> _______________________________________________
> Mercurial-devel mailing list
> [hidden email]
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

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

Pierre-Yves David-2
In reply to this post by Manuel Jacob
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 <[hidden email]> wrote:
>>
>>> On 2020-07-09 18:48, Martin von Zweigbergk wrote:
>>> > On Thu, Jul 9, 2020 at 12:35 AM Manuel Jacob <[hidden email]>
>>> 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 <[hidden email]>
>>> 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
>>> >> >> > [hidden email]
>>> >> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >> _______________________________________________
>>> >> >> Mercurial-devel mailing list
>>> >> >> [hidden email]
>>> >> >> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >> >>
>>> >> >
>>> >> > _______________________________________________
>>> >> > Mercurial-devel mailing list
>>> >> > [hidden email]
>>> >> > https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
>>> >>
>>>
--
Pierre-Yves David

_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel