Patchwork D2409: graft: add no-commit mode (issue5631)

login
register
mail settings
Submitter phabricator
Date Feb. 24, 2018, 8:06 a.m.
Message ID <differential-rev-PHID-DREV-w6an4vnjwdwh4d2mycxl-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/28302/
State Superseded
Headers show

Comments

phabricator - Feb. 24, 2018, 8:06 a.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

AFFECTED FILES
  mercurial/commands.py
  tests/test-graft.t

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - Feb. 24, 2018, 9:13 a.m.
pulkit added a comment.


  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

INLINE COMMENTS

> test-graft.t:1390
> +  grafting 1:925d80f479bb "1"
> +  note: graft of 1:925d80f479bb created no changes to commit
> +

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - Feb. 26, 2018, 5:47 p.m.
khanchi97 added a comment.


  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!

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 1, 2018, 12:52 p.m.
pulkit added a comment.


  Sorry for getting a bit late for reviewing this. I was mostly traveling for last few days.
  
  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 3, 2018, 8:17 a.m.
khanchi97 added a comment.


  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`.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 3, 2018, 7:42 p.m.
pulkit added inline comments.

INLINE COMMENTS

> commands.py:2309
> +                else:
> +                    repo.vfs.write('graftstate', 'False\n')
>                  nodelines = [repo[rev].hex() + "\n" for rev in revs[pos:]]

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 https://phab.mercurial-scm.org/D2591. I think if that goes in, we should use that format to store about `no-commit` flag.

> commands.py:2331
> +            nocommitflag = lines[0]
> +        if not (opts.get('no_commit') or nocommitflag == 'True'):
> +            node = repo.commit(text=message, user=user,

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 4, 2018, 10:24 a.m.
khanchi97 added a comment.


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

INLINE COMMENTS

> pulkit wrote in commands.py:2309
> 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 https://phab.mercurial-scm.org/D2591. I think if that goes in, we should use that format to store about `no-commit` flag.

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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 8, 2018, 2:55 p.m.
khanchi97 added a comment.


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

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - March 10, 2018, 9:05 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - May 13, 2018, 3 p.m.
khanchi97 added a comment.


  In https://phab.mercurial-scm.org/D2409#44682, @pulkit wrote:
  
  > In https://phab.mercurial-scm.org/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?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - May 20, 2018, 7:56 p.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/D2409#56200, @khanchi97 wrote:
  
  > In https://phab.mercurial-scm.org/D2409#44682, @pulkit wrote:
  >
  > > In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 1, 2018, 9:19 a.m.
pulkit added a comment.


  In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 2, 2018, 5:25 p.m.
khanchi97 added a comment.


  In https://phab.mercurial-scm.org/D2409#57814, @pulkit wrote:
  
  > In https://phab.mercurial-scm.org/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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers
Cc: pulkit, mercurial-devel
phabricator - June 2, 2018, 7:14 p.m.
pulkit requested changes to this revision.
pulkit added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> commands.py:2245
>              revs = [repo[node].rev() for node in nodes]
> +            if opts.get('no_commit'):
> +                statedata['no_commit'] = True

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

> commands.py:2378
>                  statedata['nodes'] = nodes
> +                if pos == 0:
> +                    if opts.get('no_commit'):

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.

> commands.py:2396
> +        # commit if --no-commit is False
> +        try:
> +            flagfromstate = statedata['no_commit']

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - June 3, 2018, 12:07 p.m.
pulkit requested changes to this revision.
pulkit added a comment.
This revision now requires changes to proceed.


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

INLINE COMMENTS

> commands.py:2225
> +        if opts.get('edit'):
> +            raise error.Abort(_("can't specify --no-commit and --edit"))
> +        if opts.get('currentuser'):

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

> test-graft.t:1541
> +
> +  $ hg init nocommit
> +  $ cd nocommit

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

> test-graft.t:1550
> +  $ hg ci -qAm2
> +
> +Check reporting when --no-commit used with non-applicable options:

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

> test-graft.t:1553
> +
> +  $ hg graft 1 --no-commit -e
> +  abort: can't specify --no-commit and --edit

maybe test with couple of other flags too.

> test-graft.t:1561
> +
> +  $ hg tip -T "rev: {rev}\n"
> +  rev: 2

replace this with `hg log`

> test-graft.t:1579
> +  $ echo B>a
> +  $ hg ci -qm5
> +  $ hg graft 4 --no-commit

add a `hg log` call after that

> test-graft.t:1599
> +  grafting 4:a08bb3910e7c "4"
> +  $ hg tip -T "rev: {rev}\n"
> +  rev: 5

replace this one also with `hg log`

> test-graft.t:1609
> +
> +Prepare wrdir to check --no-commit is resepected when passed with --continue:
> +

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.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - June 26, 2018, 8:04 a.m.
pulkit added a comment.


  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.

INLINE COMMENTS

> test-graft.t:1551
> +  $ hg ci -qAm2
> +  $ hg log
> +  changeset:   2:db815d6d32e6

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.

> test-graft.t:1677
> +  grafting 4:a08bb3910e7c "4"
> +  $ hg tip -T "rev: {rev}\n"
> +  rev: 5

`hg log` will serve the purpose better here.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel
phabricator - June 26, 2018, 5:25 p.m.
pulkit accepted this revision.
pulkit added a comment.


  landed <https://phab.mercurial-scm.org/macro/meme/?macro=landed>

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D2409

To: khanchi97, #hg-reviewers, pulkit
Cc: pulkit, mercurial-devel

Patch

diff --git a/tests/test-graft.t b/tests/test-graft.t
--- a/tests/test-graft.t
+++ b/tests/test-graft.t
@@ -1373,3 +1373,30 @@ 
   note: graft of 7:d3c3f2b38ecc created no changes to commit
 
   $ cd ..
+
+Graft a change from a branch without making any commit using --no-commit option
+
+  $ hg init dirtochecknocommit
+  $ cd dirtochecknocommit
+  $ echo a > a
+  $ hg ci -qAm0
+  $ echo b > b
+  $ hg ci -qAm1
+  $ hg up -q 0
+  $ echo c > c
+  $ hg ci -qAm2
+  $ hg graft 1 --no-commit
+  grafting 1:925d80f479bb "1"
+  note: graft of 1:925d80f479bb created no changes to commit
+
+  $ hg tip -T "rev : {rev}\n"
+  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
+
+  $ cd ..
diff --git a/mercurial/commands.py b/mercurial/commands.py
--- a/mercurial/commands.py
+++ b/mercurial/commands.py
@@ -2074,6 +2074,8 @@ 
      ('c', 'continue', False, _('resume interrupted graft')),
      ('e', 'edit', False, _('invoke editor on commit messages')),
      ('', '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')),
      ('D', 'currentdate', False,
       _('record the current date as commit date')),
@@ -2313,9 +2315,11 @@ 
         else:
             cont = False
 
+        node = None
         # commit
-        node = repo.commit(text=message, user=user,
-                    date=date, extra=extra, editor=editor)
+        if not opts.get('no_commit'):
+            node = repo.commit(text=message, user=user,
+                        date=date, extra=extra, editor=editor)
         if node is None:
             ui.warn(
                 _('note: graft of %d:%s created no changes to commit\n') %