Page MenuHomePhabricator

absorb: make the absorbed changeset be automatically "evolved"
Needs RevisionPublic

Authored by rdamazio on Dec 13 2019, 12:55 AM.

Details

Reviewers
baymax
Group Reviewers
hg-reviewers
Summary

When a committed changeset is absorbed, this will make it so it's not used for
computing the absorption, but is still recreated at the destination.

Diff Detail

Repository
rHG Mercurial
Branch
default
Lint
No Linters Available
Unit
No Unit Test Coverage

Event Timeline

rdamazio created this revision.Dec 13 2019, 12:55 AM
martinvonz added inline comments.
tests/test-absorb-rev.t
67–68

hg absorb has a -r flag? Did you forgot to upload some ancestors of this commit?

martinvonz added inline comments.Dec 13 2019, 1:47 AM
tests/test-absorb-rev.t
67–68

Oh, it's added in D7631, which is marked as a *child* of this review. How did that happen? hg phabsend should set the relationships automatically. How did you run that command? Just hg phabsend .^::. or similar? Or did you manually mark D7631 as a child?

(on mobile) I think it was hg phabsend -r .+.^

(on mobile) I think it was hg phabsend -r .+.^

Ohh, that makes some sense that phabsend might do it backwards if you pass the revisions backwards, but that's still clearly a bug. I'll report it in bugzilla if it's not already there. I'll manually update the parent/child relationship on these reviews for now.

rdamazio marked 2 inline comments as done.Dec 14 2019, 1:02 AM
rdamazio retitled this revision from RFC: absorb: make the absorbed changeset be automatically "evolved" to absorb: make the absorbed changeset be automatically "evolved".
rdamazio updated this revision to Diff 18727.

Sorry for the upload mess (though it's arguably a phabsend bug :) ). Tried uploading the "right" way now.

rdamazio updated this revision to Diff 18729.Dec 14 2019, 1:25 AM
rdamazio updated this revision to Diff 18842.Dec 17 2019, 11:42 PM
rdamazio updated this revision to Diff 18844.Dec 17 2019, 11:46 PM
rdamazio updated this revision to Diff 18874.Dec 19 2019, 2:36 AM
rdamazio updated this revision to Diff 19049.Jan 6 2020, 7:46 PM
pulkit added a subscriber: pulkit.Jan 13 2020, 10:57 AM

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

Sorry for the upload mess (though it's arguably a phabsend bug :) ). Tried uploading the "right" way now.

I reported that as https://bz.mercurial-scm.org/show_bug.cgi?id=6241.

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?

pulkit added a comment.EditedJan 13 2020, 12:37 PM

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?

Yep, that sounds good.

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?

Yep, that sounds good.

This means generate split+fold markers. That is going to be fun to deal with :-). I dont' really have any much better idea however (the alternative seems to use simple prune markers, which is meh).

rdamazio updated this revision to Diff 19812.Feb 1 2020, 2:10 AM

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

It's not *always* empty - if there had been lines that it failed absorb (because it couldn't find where to absorb into), those would be left in the commit.

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?

Yep, that sounds good.

I'm fine with doing this, but is there an efficient way to detect that it became empty?

This means generate split+fold markers. That is going to be fun to deal with :-). I dont' really have any much better idea however (the alternative seems to use simple prune markers, which is meh).

I'm not following this part - why do you need markers at all?
(you can argue whether you want them - but I don't see why you *need* them)

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?

Yep, that sounds good.

I'm fine with doing this, but is there an efficient way to detect that it became empty?

And by "this" I meant I'm fine with making it disappear if allowemptycommit is False. I don't fully understand how markers help accomplish that.

rdamazio updated this revision to Diff 19982.Feb 6 2020, 10:11 PM

This results in an empty commit which is not similar to what rebase or evolve will generally result in after D7631 unless ui.allowemptycommit=True is set. I think good behavior is to obsolete the absorbed changeset in favour of either it's parent or one of the revs in which it was absorbed.

I made a related comment on the parent patch before I realized that this patch adds obsmarker handling. My suggestion there was to mark all the commits that got absorbed into as successors, and if there's anything left in the absorbed commit, that would be yet another successor. Would that work?

Yep, that sounds good.

I'm fine with doing this, but is there an efficient way to detect that it became empty?

And by "this" I meant I'm fine with making it disappear if allowemptycommit is False. I don't fully understand how markers help accomplish that.

When you try to create an empty commit, you'll get a None back for the nodeid (from repo.commitctx(), IIRC), so check for that.

It looks like this series is introducing UI change of the same kind as the one @martinvonz is looking into for hg copy. I'll try to have a look at both of them tomorrow.

baymax requested changes to this revision.Mon, Jun 8, 1:34 PM

There seems to have been no activities on this Diff for the past 3 Months.

By policy, we are automatically moving it out of the need-review state.

Please, move it back to need-review without hesitation if this diff should still be discussed.

:baymax:need-review-idle:

This revision now requires changes to proceed.Mon, Jun 8, 1:34 PM