( )⚙ D6655 continue: added support for graft

This is an archive of the discontinued Mercurial Phabricator instance.

continue: added support for graft
ClosedPublic

Authored by taapas1128 on Jul 18 2019, 1:45 PM.

Details

Summary

This adds support of graft to hg continue plan.

The patch creates a seperate function cmdutil.continuegraft
so that continue logic for graft can be called independently.
This logic is registered to the statedetection API as continuefunc.

Results are shown as tests.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

taapas1128 created this revision.Jul 18 2019, 1:45 PM

I'm landing this, but in the future I suspect we should make sure we just have a mix of hg graft --continue and hg graft rather than run the whole test twice, as it'll severely bloat the already-slow testsuite to do that for many tests.

This revision was not accepted when it landed; it landed in state Needs Review.
This revision was automatically updated to reflect the committed changes.

There is a lot of duplication here. Can you try to extract common parts?

@martinvonz I will send a patch doing that.

mharbison72 added inline comments.
tests/test-graft.t
1

This has the same problem D6579 had where there are 4 separate test cases, instead of 2 with sub cases.

@martinvonz I will send a patch doing that.

tests/test-graft.t
1

I tried to fix that but the test fails if I do that.

martinvonz added inline comments.Jul 19 2019, 6:21 PM
mercurial/cmdutil.py
3482–3498

It looks like pretty much this entire function is copies commands.graft() and then all this code is left dead here (since cont = True above). I'd prefer to de-queue this commit and let you clean this up and then submit a new patch, but maybe other reviewers are less picky or stubborn than I am, so I'll leave it queued until I hear from someone else.

taapas1128 added inline comments.Jul 19 2019, 11:48 PM
mercurial/cmdutil.py
3482–3498

@martinvonz That code is not dead but it takes care not to merge the first commit when continuing graft. cont=True is just the first iteration of the for loop at line 3455 after that it becomes False. Although it may seem like all the code has been copied from commands.graft but only that part is copied but only that part is imported that works when --continue flag is called. Regarding cleaning I do agree spliting code from commands.graft() would be nice. So have a look at D6659.

yuja added a subscriber: yuja.Jul 20 2019, 3:36 AM

It looks like pretty much this entire function is copies commands.graft()
and then all this code is left dead here (since cont = True above).
I'd prefer to de-queue this commit and let you clean this up and then
submit a new patch

That's my feeling. Perhaps the core for loop can be extracted to a helper
function. Or alternatively, continuegraft() can be implemented on top of
commands.graft()?

@yuja have a look at D6659 and suggest changes there. Extracting the core for loop as a helper function sounds like a nice idea. I will update the patch for the same.

tests/test-graft.t
1

I will have a look.

taapas1128 added inline comments.Jul 20 2019, 10:06 AM
tests/test-graft.t
1

@mharbison72 I have generated a patch for that in D6659. I minimized it to 3 cases as @durin42 stated about bloating of the test sets.

Sorry, but since both Yuya and I are uncomfortable with this patch, I'll de-queue it so it doesn't block other patches from getting queued. I think it would be good if you can fold D6659 into this one and then split it up so have only the refactoring in a first patch and the new support for continue in a second patch. Thanks.

@martinvonz No problem, I will update patch D6659.