Submitter | Manuel Jacob |
---|---|
Date | July 1, 2020, 9:39 p.m. |
Message ID | <45fa02da6fd1c3b40350.1593639541@tmp> |
Download | mbox | patch |
Permalink | /patch/46607/ |
State | New |
Headers | show |
Comments
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 >>> >> >>>
Patch
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' +