This is an archive of the discontinued Mercurial Phabricator instance.

graft: add no-commit mode (issue5631)
ClosedPublic

Authored by khanchi97 on Feb 24 2018, 3:05 AM.

Details

Summary

This patch adds a new flag --no-commit in graft command. This feature
grafts the changes but do not create commits for those changes, grafted
changes will be added in the working directory. Also added tests to reflect
the expected behavior.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

khanchi97 created this revision.Feb 24 2018, 3:05 AM
pulkit added a subscriber: pulkit.Feb 24 2018, 4:13 AM

Can you test about the --no-commit flag being respected when we do --continue in case of conflicts? That part is not handled by this patch yet.

To elaborate:

$ hg graft --no-commit
  warning: conflicts while merging ...
$ resolve the conflicts
$ hg graft --continue
  ^^ this should respect --no-commit by not creating a commit
tests/test-graft.t
1390

We don't want this message while using hg graft --no-commit.

Hello Pulkit,
After spending some time with graft implementation, I hope now I have a good understanding of how graft is working.
Let me tell you what I have understood and how I want to implement --no-commit mode. So when we hit a merge conflict what we do is save the current state in graftstate by storing
rev for rev in revs[current_pos: ] And after marking resolved, IIUC when we run $ hg graft --continue we only left with graftstate and we have no idea what are the previous flags passed to graft.
My solution for --no-commit mode:
As in case of continue, we are only left with graftstate, So I am thinking to make a no_commit_flag file similarly as we create graftstate file and when we get --no-commit flag we can create this file and after completion of the command we can unlink this no_commit_flag file.
Please let me know if I am on the right path or not?
Thanks!

khanchi97 updated this revision to Diff 6132.Feb 26 2018, 3:27 PM

Hello there, Can I get a little review on this :)

pulkit added a comment.Mar 1 2018, 7:52 AM

Sorry for getting a bit late for reviewing this. I was mostly traveling for last few days.

Hello Pulkit,
After spending some time with graft implementation, I hope now I have a good understanding of how graft is working.
Let me tell you what I have understood and how I want to implement --no-commit mode. So when we hit a merge conflict what we do is save the current state in graftstate by storing
rev for rev in revs[current_pos: ] And after marking resolved, IIUC when we run $ hg graft --continue we only left with graftstate and we have no idea what are the previous flags passed to graft.
My solution for --no-commit mode:
As in case of continue, we are only left with graftstate, So I am thinking to make a no_commit_flag file similarly as we create graftstate file and when we get --no-commit flag we can create this file and after completion of the command we can unlink this no_commit_flag file.

We can store that information in graftstate and we are done, no need to have an extra file. If you look at rebasestate and histeditstate, they do the same thing. About the format in which the new flag should be stored in graft state, I am going to send a series which will ease writing data to state files.

khanchi97 updated this revision to Diff 6325.Mar 2 2018, 5:36 AM

What I did to store --no-commit flag in graftstate is:
Add a flag True/False in 1st line of graftstate and keep it updated from previous value in graftstate for every iteration in revs.

pulkit added inline comments.Mar 3 2018, 2:41 PM
mercurial/commands.py
2382

To be honest I am not fan of storing data in such format, but that's how we store data right now. I have finally a series in review where we can use cbor to serialize data while writing to state files. Look at D2591. I think if that goes in, we should use that format to store about no-commit flag.

2394

What about if we do --continue and --no-commit? Also, a lot of existing flags should error out while using with --no-commit. That's also missing.

khanchi97 updated this revision to Diff 6538.Mar 4 2018, 5:12 AM

I have added tests to show the behavior of --no-commit with other flags.

mercurial/commands.py
2382

okay, when your series will be pushed in, I will use cbor to write in statefile.

pulkit: Can you please review it? I have made the requested changes.

pulkit: Can you please review it? I have made the requested changes.

I am sorry but I will like this to wait before we land the new state format thing. I don't want to put more information in old state files which don't have good format. But yes, if someone else feels we can go with this, I am fine with that too.

In D2409#44682, @pulkit wrote:

pulkit: Can you please review it? I have made the requested changes.

I am sorry but I will like this to wait before we land the new state format thing. I don't want to put more information in old state files which don't have good format. But yes, if someone else feels we can go with this, I am fine with that too.

@pulkit do we have new state format pushed in?

In D2409#44682, @pulkit wrote:

pulkit: Can you please review it? I have made the requested changes.

I am sorry but I will like this to wait before we land the new state format thing. I don't want to put more information in old state files which don't have good format. But yes, if someone else feels we can go with this, I am fine with that too.

@pulkit do we have new state format pushed in?

Not yet. I have started reiterating over the series and hopefully we will be in a state to take this patch in few weeks or maybe a month.

pulkit added a comment.Jun 1 2018, 5:19 AM

@pulkit do we have new state format pushed in?

Yep, you can go ahead and rebase this patch to make it use the new statefile format.

khanchi97 updated this revision to Diff 8953.Jun 2 2018, 1:21 PM
In D2409#57814, @pulkit wrote:

@pulkit do we have new state format pushed in?

Yep, you can go ahead and rebase this patch to make it use the new statefile format.

I have updated the patch and now it is using the new statefile format.

pulkit requested changes to this revision.Jun 2 2018, 3:13 PM
pulkit added inline comments.
mercurial/commands.py
2247

Here we are reading data from graftstate, this is run when --continue is passed, we read the graftstate and then continue the graft.

2378

Why can't we do this outside of the for-loop above? Also there is no need to store the value if it's False. We can use statedata.get('no_commit') which will return None if the value is not present.

2391

This reading of statefile should happen above where we are reading other values too. Also logic can be simplified if we read the value from statefile and store it in opts['no_commit'], similar to how we are reading date and user and storing them in opts.

This revision now requires changes to proceed.Jun 2 2018, 3:13 PM
khanchi97 updated this revision to Diff 8955.Jun 3 2018, 4:38 AM
pulkit requested changes to this revision.Jun 3 2018, 8:07 AM

Please add tests where we are grafting multiple revisions using the --no-commit flag.

mercurial/commands.py
2225

the error messages should be cannot specify --no-commit and --edit together. This applies to rest two also.

tests/test-graft.t
1541

We are already in a repository here. cd back before creating a new repo.

1550

add a hg log call here, it will be helpful in understanding what's happening.

1553

maybe test with couple of other flags too.

1561

replace this with hg log

1579

add a hg log call after that

1599

replace this one also with hg log

1609

I am not sure we need this behavior. Let's have this test so that others can chime in if we don't want to have this behavior.

This revision now requires changes to proceed.Jun 3 2018, 8:07 AM
khanchi97 updated this revision to Diff 8958.Jun 3 2018, 9:17 AM
khanchi97 updated this revision to Diff 8961.Jun 3 2018, 10:13 AM

The patch mostly looks good to me. Added some minor nits. It will be great if you can add test where we graft multiple revs with --no-commit flag.

tests/test-graft.t
1551

We should print a graphical version of log so that test can be understand better. Maybe replace this with hg log -GT "{rev}:{node|short} {desc}\n". Comment applies to other instances of hg log in test too.

1677

hg log will serve the purpose better here.

khanchi97 edited the summary of this revision. (Show Details)Jun 26 2018, 6:46 AM
khanchi97 updated this revision to Diff 9301.
This revision was automatically updated to reflect the committed changes.