This is an archive of the discontinued Mercurial Phabricator instance.

Differential D2409

graft: add no-commit mode (issue5631)
ClosedPublic

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

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG622f79e3a1cb: graft: add no-commit mode (issue5631)
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.

khanchi97 added a comment.Feb 26 2018, 12:47 PM

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
khanchi97 added a comment.Feb 28 2018, 9:56 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.

In D2409#40102, @khanchi97 wrote:

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
khanchi97 added a comment.Mar 3 2018, 3:17 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.

2399

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
khanchi97 added a comment.Mar 4 2018, 5:24 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.

khanchi97 added a comment.Mar 8 2018, 9:55 AM

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

pulkit added a comment.Mar 10 2018, 4:05 AM
In D2409#43891, @khanchi97 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.

khanchi97 added a comment.May 13 2018, 11:00 AM
In D2409#44682, @pulkit wrote:
In D2409#43891, @khanchi97 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?

pulkit added a comment.May 20 2018, 3:56 PM
In D2409#56200, @khanchi97 wrote:
In D2409#44682, @pulkit wrote:
In D2409#43891, @khanchi97 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
In D2409#56200, @khanchi97 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.

khanchi97 updated this revision to Diff 8953.Jun 2 2018, 1:21 PM
khanchi97 added a comment.Jun 2 2018, 1:25 PM
In D2409#57814, @pulkit wrote:
In D2409#56200, @khanchi97 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
2245

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.

2396

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
pulkit added a comment.Jun 26 2018, 4:04 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.
Closed by commit rHG622f79e3a1cb: graft: add no-commit mode (issue5631) (authored by khanchi97). · Explain WhyJun 26 2018, 1:51 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/commands.py (43 lines)
Mtests/test-graft.t (103 lines)

Diff 8953

mercurial/commands.py

return rejected and 1 or 0 return rejected and 1 or 0
@command( @command(
'graft', 'graft',
[('r', 'rev', [], _('revisions to graft'), _('REV')), [('r', 'rev', [], _('revisions to graft'), _('REV')),
('c', 'continue', False, _('resume interrupted graft')), ('c', 'continue', False, _('resume interrupted graft')),
('e', 'edit', False, _('invoke editor on commit messages')), ('e', 'edit', False, _('invoke editor on commit messages')),
('', 'log', None, _('append graft info to log message')), ('', 'log', None, _('append graft info to log message')),
('', 'no-commit', None,
_("don't commit, just apply the changes in working directory")),
('f', 'force', False, _('force graft')), ('f', 'force', False, _('force graft')),
('D', 'currentdate', False, ('D', 'currentdate', False,
_('record the current date as commit date')), _('record the current date as commit date')),
('U', 'currentuser', False, ('U', 'currentuser', False,
_('record the current user as committer'), _('DATE'))] _('record the current user as committer'), _('DATE'))]
+ commitopts2 + mergetoolopts + dryrunopts, + commitopts2 + mergetoolopts + dryrunopts,
_('[OPTION]... [-r REV]... REV...')) _('[OPTION]... [-r REV]... REV...'))
def graft(ui, repo, *revs, **opts): def graft(ui, repo, *revs, **opts):
If a graft merge results in conflicts, the graft process is If a graft merge results in conflicts, the graft process is
interrupted so that the current merge can be manually resolved. interrupted so that the current merge can be manually resolved.
Once all conflicts are addressed, the graft process can be Once all conflicts are addressed, the graft process can be
continued with the -c/--continue option. continued with the -c/--continue option.
.. note:: .. note::
The -c/--continue option does not reapply earlier options, except The -c/--continue option does not reapply earlier options, except
for --force, --user and --date. for --force, --user, --date and --no-commit.
.. container:: verbose .. container:: verbose
Examples: Examples:
- copy a single change to the stable branch and edit its description:: - copy a single change to the stable branch and edit its description::
hg update stable hg update stable
opts['user'] = ui.username() opts['user'] = ui.username()
if not opts.get('date') and opts.get('currentdate'): if not opts.get('date') and opts.get('currentdate'):
opts['date'] = "%d %d" % dateutil.makedate() opts['date'] = "%d %d" % dateutil.makedate()
editor = cmdutil.getcommiteditor(editform='graft', editor = cmdutil.getcommiteditor(editform='graft',
**pycompat.strkwargs(opts)) **pycompat.strkwargs(opts))
cont = False cont = False
if opts.get('no_commit'):
if opts.get('edit'):
raise error.Abort(_("can't specify --no-commit and --edit"))
pulkitUnsubmitted
Not Done

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

pulkit: the error messages should be `cannot specify --no-commit and --edit together`. This applies to…
if opts.get('currentuser'):
raise error.Abort(_("can't specify --no-commit and --currentuser"))
if opts.get('currentdate'):
raise error.Abort(_("can't specify --no-commit and --currentdate"))
graftstate = statemod.cmdstate(repo, 'graftstate') graftstate = statemod.cmdstate(repo, 'graftstate')
if opts.get('continue'): if opts.get('continue'):
cont = True cont = True
if revs: if revs:
raise error.Abort(_("can't specify --continue and revisions")) raise error.Abort(_("can't specify --continue and revisions"))
# read in unfinished revisions # read in unfinished revisions
if graftstate.exists(): if graftstate.exists():
statedata = _readgraftstate(repo, graftstate) statedata = _readgraftstate(repo, graftstate)
if statedata.get('date'): if statedata.get('date'):
opts['date'] = statedata['date'] opts['date'] = statedata['date']
if statedata.get('user'): if statedata.get('user'):
opts['user'] = statedata['user'] opts['user'] = statedata['user']
nodes = statedata['nodes'] nodes = statedata['nodes']
revs = [repo[node].rev() for node in nodes] revs = [repo[node].rev() for node in nodes]
if opts.get('no_commit'):
pulkitUnsubmitted
Not Done

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

pulkit: Here we are reading data from graftstate, this is run when `--continue` is passed, we read the…
statedata['no_commit'] = True
stateversion = 1
graftstate.save(stateversion, statedata)
else: else:
cmdutil.wrongtooltocontinue(repo, _('graft')) cmdutil.wrongtooltocontinue(repo, _('graft'))
else: else:
if not revs: if not revs:
raise error.Abort(_('no revisions specified')) raise error.Abort(_('no revisions specified'))
cmdutil.checkunfinished(repo) cmdutil.checkunfinished(repo)
cmdutil.bailifchanged(repo) cmdutil.bailifchanged(repo)
revs = scmutil.revrange(repo, revs) revs = scmutil.revrange(repo, revs)
['local', 'graft']) ['local', 'graft'])
finally: finally:
repo.ui.setconfig('ui', 'forcemerge', '', 'graft') repo.ui.setconfig('ui', 'forcemerge', '', 'graft')
# report any conflicts # report any conflicts
if stats.unresolvedcount > 0: if stats.unresolvedcount > 0:
# write out state for --continue # write out state for --continue
nodes = [repo[rev].hex() for rev in revs[pos:]] nodes = [repo[rev].hex() for rev in revs[pos:]]
statedata['nodes'] = nodes statedata['nodes'] = nodes
if pos == 0:
pulkitUnsubmitted
Not Done

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.

pulkit: Why can't we do this outside of the for-loop above? Also there is no need to store the value if…
if opts.get('no_commit'):
statedata['no_commit'] = True
else:
statedata['no_commit'] = False
pulkitUnsubmitted
Not Done

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.

pulkit: To be honest I am not fan of storing data in such format, but that's how we store data right…
khanchi97AuthorUnsubmitted
Not Done

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

khanchi97: okay, when your series will be pushed in, I will use cbor to write in statefile.
stateversion = 1 stateversion = 1
graftstate.save(stateversion, statedata) graftstate.save(stateversion, statedata)
extra = '' extra = ''
if opts.get('log'): if opts.get('log'):
extra += ' --log' extra += ' --log'
hint=_("use 'hg resolve' and 'hg graft --continue%s'") % extra hint=_("use 'hg resolve' and 'hg graft --continue%s'") % extra
raise error.Abort( raise error.Abort(
_("unresolved conflicts, can't continue"), _("unresolved conflicts, can't continue"),
hint=hint) hint=hint)
else: else:
cont = False cont = False
# commit # commit if --no-commit is False
try:
pulkitUnsubmitted
Not Done

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.

pulkit: This reading of statefile should happen above where we are reading other values too. Also logic…
flagfromstate = statedata['no_commit']
except KeyError:
flagfromstate = False
pulkitUnsubmitted
Not Done

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.

pulkit: What about if we do `--continue` and `--no-commit`? Also, a lot of existing flags should error…
nocommitflag = opts.get('no_commit') or (opts.get('continue') and
flagfromstate)
if not nocommitflag:
node = repo.commit(text=message, user=user, node = repo.commit(text=message, user=user,
date=date, extra=extra, editor=editor) date=date, extra=extra, editor=editor)
if node is None: if node is None:
ui.warn( ui.warn(
_('note: graft of %d:%s created no changes to commit\n') % _('note: graft of %d:%s created no changes to commit\n') %
(ctx.rev(), ctx)) (ctx.rev(), ctx))
# remove state when we complete successfully # remove state when we complete successfully
if not opts.get('dry_run'): if not opts.get('dry_run'):
graftstate.delete() graftstate.delete()
return 0 return 0
def _readgraftstate(repo, graftstate): def _readgraftstate(repo, graftstate):

tests/test-graft.t

$ hg log -GT "{rev}:{node|short} {desc}\n" $ hg log -GT "{rev}:{node|short} {desc}\n"
@ 2:8be98ac1a569 added c @ 2:8be98ac1a569 added c
| |
o 1:80e6d2c47cfe added b o 1:80e6d2c47cfe added b
| |
o 0:f7ad41964313 added a o 0:f7ad41964313 added a
$ hg up 0 $ hg up 0
0 files updated, 0 files merged, 2 files removed, 0 files unresolved 0 files updated, 0 files merged, 2 files removed, 0 files unresolved
pulkitUnsubmitted
Not Done

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

pulkit: We don't want this message while using `hg graft --no-commit`.
$ echo bar > b $ echo bar > b
$ hg add b $ hg add b
$ hg ci -m "bar to b" $ hg ci -m "bar to b"
created new head created new head
$ hg graft -r 1 -r 2 $ hg graft -r 1 -r 2
grafting 1:80e6d2c47cfe "added b" grafting 1:80e6d2c47cfe "added b"
merging b merging b
warning: conflicts while merging b! (edit, then use 'hg resolve --mark') warning: conflicts while merging b! (edit, then use 'hg resolve --mark')
| summary: added b | summary: added b
| |
o changeset: 3:9e887f7a939c o changeset: 3:9e887f7a939c
| parent: 0:f7ad41964313 | parent: 0:f7ad41964313
~ user: test ~ user: test
date: Thu Jan 01 00:00:00 1970 +0000 date: Thu Jan 01 00:00:00 1970 +0000
summary: bar to b summary: bar to b
Test for --no-commit option:
$ hg init nocommit
pulkitUnsubmitted
Not Done

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

pulkit: We are already in a repository here. `cd` back before creating a new repo.
$ cd nocommit
$ echo a > a
$ hg ci -qAm0
$ echo b > b
$ hg ci -qAm1
$ hg up -q 0
$ echo c > c
$ hg ci -qAm2
pulkitUnsubmitted
Not Done

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

pulkit: add a `hg log` call here, it will be helpful in understanding what's happening.
Check reporting when --no-commit used with non-applicable options:
pulkitUnsubmitted
Not Done

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.

pulkit: We should print a graphical version of log so that test can be understand better. Maybe replace…
$ hg graft 1 --no-commit -e
pulkitUnsubmitted
Not Done

maybe test with couple of other flags too.

pulkit: maybe test with couple of other flags too.
abort: can't specify --no-commit and --edit
[255]
Test --no-commit is working:
$ hg graft 1 --no-commit
grafting 1:925d80f479bb "1"
$ hg tip -T "rev: {rev}\n"
pulkitUnsubmitted
Not Done

replace this with hg log

pulkit: replace this with `hg log`
rev: 2
$ hg diff
diff -r db815d6d32e6 b
--- /dev/null Thu Jan 01 00:00:00 1970 +0000
+++ b/b Thu Jan 01 00:00:00 1970 +0000
@@ -0,0 +1,1 @@
+b
$ hg ci -qm3
Prepare wrdir to check --no-commit is resepected after --continue:
$ echo A>a
$ hg ci -qm4
$ hg up -q 1
$ echo B>a
$ hg ci -qm5
pulkitUnsubmitted
Not Done

add a hg log call after that

pulkit: add a `hg log` call after that
$ hg graft 4 --no-commit
grafting 4:a08bb3910e7c "4"
merging a
warning: conflicts while merging a! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
[255]
Resolving conflict:
$ echo A>a
$ hg resolve --mark
(no more unresolved files)
continue: hg graft --continue
Test --no-commit is respected after --continue:
$ hg graft --continue
grafting 4:a08bb3910e7c "4"
$ hg tip -T "rev: {rev}\n"
pulkitUnsubmitted
Not Done

replace this one also with hg log

pulkit: replace this one also with `hg log`
rev: 5
$ hg diff
diff -r b1d5b5056844 a
--- a/a Thu Jan 01 00:00:00 1970 +0000
+++ b/a Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-B
+A
Prepare wrdir to check --no-commit is resepected when passed with --continue:
pulkitUnsubmitted
Not Done

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.

pulkit: I am not sure we need this behavior. Let's have this test so that others can chime in if we…
$ echo B>a
$ hg graft 4
grafting 4:a08bb3910e7c "4"
merging a
warning: conflicts while merging a! (edit, then use 'hg resolve --mark')
abort: unresolved conflicts, can't continue
(use 'hg resolve' and 'hg graft --continue')
[255]
Resolving conflict:
$ echo A>a
$ hg resolve --mark
(no more unresolved files)
continue: hg graft --continue
Test --no-commit is respected with --continue:
$ hg graft --continue --no-commit
grafting 4:a08bb3910e7c "4"
$ hg diff
diff -r b1d5b5056844 a
--- a/a Thu Jan 01 00:00:00 1970 +0000
+++ b/a Thu Jan 01 00:00:00 1970 +0000
@@ -1,1 +1,1 @@
-B
+A
$ hg tip -T "rev: {rev}\n"
rev: 5
$ cd ..
pulkitUnsubmitted
Not Done

hg log will serve the purpose better here.

pulkit: `hg log` will serve the purpose better here.