Patchwork D6058: patch: include flag-only file changes in "special" while filtering patch (issue5864)

login
register
mail settings
Submitter phabricator
Date March 3, 2019, 10:38 p.m.
Message ID <differential-rev-PHID-DREV-szuxsesvjooj77nldog6-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39025/
State Superseded
Headers show

Comments

phabricator - March 3, 2019, 10:38 p.m.
khanchi97 created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  This patch fix the issue5864 (or maybe issue5865 too) which occurs during
  split (or I should say at the time of filtering the hunks in interactive
  mode) where user hits a not ending loop of "no changes to record".
  And it's not only the case for split it will happen in every interactive
  case for e.g. `hg commit -i` or `hg uncommit -i`
  
  After looking into code I found that when filtering we have some
  notation called "special" for the file headers which doesn't contain
  any hunk and just contain the header (for e.g. newly added empty file
  or deleted file) where the user cannot change the content of operation.
  
  And I think we can put this "flag-only" file change in that same bucket
  of "special". But I doubt a bit about the case when a file have flag change
  and atleast one hunk then user won't be able to separate the flag change
  from hunks.
  Changed test file reflect the fixed behaviour.

REPOSITORY
  rHG Mercurial

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

AFFECTED FILES
  mercurial/patch.py
  tests/test-split.t

CHANGE DETAILS




To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - March 9, 2019, 4:52 a.m.
khanchi97 added a comment.


  Ping for review.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - March 13, 2019, 7:27 p.m.
khanchi97 added inline comments.

INLINE COMMENTS

> test-split.t:734
>  
> -#if no-windows
> +#if windows
> +TODO: Fix this on Windows. See issue 2020 and 5883

I moved the part when on windows (#if windows) at the top because I found it easy to include nested conditions then.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers
Cc: mercurial-devel
phabricator - March 14, 2019, 4:47 p.m.
mharbison72 requested changes to this revision.
mharbison72 added inline comments.
This revision now requires changes to proceed.

INLINE COMMENTS

> test-split.t:744
> +  > EOF
> +  !!! nested #if
> +#if obsstore-on

This happens because you can't have nested `#if`.  i.e. this doesn't work:

  #if ..
  ...
  #if ..
  ...
  #endif
  #endif

You can list multiple requirements on a line, and it is effectively `&&` IIRC.

I don't see any obvious differences in the output though, other that the bundle saving in the `obsstore-off` case.  I think what you can do here is conditionalize just that line by appending ` (obsstore-off !)` to it the way you would a `(!)`, `(glob)`, or whatever.  That means it must be there for `obsstore-off`.  Conditionalizing the output also makes it easier to see the differences between the difference cases in general.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - March 16, 2019, 2:45 p.m.
khanchi97 marked an inline comment as done.
khanchi97 added inline comments.

INLINE COMMENTS

> mharbison72 wrote in test-split.t:744
> This happens because you can't have nested `#if`.  i.e. this doesn't work:
> 
>   #if ..
>   ...
>   #if ..
>   ...
>   #endif
>   #endif
> 
> You can list multiple requirements on a line, and it is effectively `&&` IIRC.
> 
> I don't see any obvious differences in the output though, other that the bundle saving in the `obsstore-off` case.  I think what you can do here is conditionalize just that line by appending ` (obsstore-off !)` to it the way you would a `(!)`, `(glob)`, or whatever.  That means it must be there for `obsstore-off`.  Conditionalizing the output also makes it easier to see the differences between the difference cases in general.

Thanks, I have updated the patch. I though it is required to a test in this file to run with both the cases `(obsstore-[on|off])`. I have also removed rev no. from graph log as output differed because of stripping.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - March 16, 2019, 10:31 p.m.
mharbison72 added inline comments.

INLINE COMMENTS

> khanchi97 wrote in test-split.t:744
> Thanks, I have updated the patch. I though it is required to a test in this file to run with both the cases `(obsstore-[on|off])`. I have also removed rev no. from graph log as output differed because of stripping.

Correct, you do want to test both cases.  But that happens because of the `#testcases` directive at the top of the file.  Basically, it runs the file once for each declared case.  Note that if you run test-split.t by itself, the test runner says it is running 2 tests.  That's why the apparent discrepancy.

REPOSITORY
  rHG Mercurial

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

To: khanchi97, #hg-reviewers, mharbison72
Cc: mharbison72, mercurial-devel
phabricator - March 17, 2019, 3:22 a.m.
khanchi97 marked an inline comment as done.
khanchi97 added a comment.


  Thanks, it is more clear now.

REPOSITORY
  rHG Mercurial

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

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

Patch

diff --git a/tests/test-split.t b/tests/test-split.t
--- a/tests/test-split.t
+++ b/tests/test-split.t
@@ -727,27 +727,57 @@ 
   o  0:51f273a58d82 initial
   
 
-  $ printf 'y\ny\ny\n' | hg split
-  diff --git a/foo b/foo
-  old mode 100644
-  new mode 100755
-  examine changes to 'foo'? [Ynesfdaq?] y
-  
-  no changes to record
+  $ cat > $TESTTMP/messages <<EOF
+  > split 1
+  > EOF
+
+#if obsstore-on
+  $ printf 'y\n' | hg split
   diff --git a/foo b/foo
   old mode 100644
   new mode 100755
   examine changes to 'foo'? [Ynesfdaq?] y
   
-  no changes to record
+  EDITOR: HG: Splitting 3a2125f0f4cb. Write commit message for the first split changeset.
+  EDITOR: make executable
+  EDITOR: 
+  EDITOR: 
+  EDITOR: HG: Enter commit message.  Lines beginning with 'HG:' are removed.
+  EDITOR: HG: Leave message empty to abort commit.
+  EDITOR: HG: --
+  EDITOR: HG: user: test
+  EDITOR: HG: branch 'default'
+  EDITOR: HG: changed foo
+  created new head
+
+  $ hg glog
+  @  2:b154670c87da split 1
+  |
+  o  0:51f273a58d82 initial
+  
+#else
+  $ printf 'y\n' | hg split
   diff --git a/foo b/foo
   old mode 100644
   new mode 100755
   examine changes to 'foo'? [Ynesfdaq?] y
   
-  no changes to record
-  diff --git a/foo b/foo
-  old mode 100644
-  new mode 100755
-  examine changes to 'foo'? [Ynesfdaq?] abort: response expected
-  [255]
+  EDITOR: HG: Splitting 3a2125f0f4cb. Write commit message for the first split changeset.
+  EDITOR: make executable
+  EDITOR: 
+  EDITOR: 
+  EDITOR: HG: Enter commit message.  Lines beginning with 'HG:' are removed.
+  EDITOR: HG: Leave message empty to abort commit.
+  EDITOR: HG: --
+  EDITOR: HG: user: test
+  EDITOR: HG: branch 'default'
+  EDITOR: HG: changed foo
+  created new head
+  saved backup bundle to $TESTTMP/issue5864/.hg/strip-backup/3a2125f0f4cb-629e4432-split.hg
+
+  $ hg glog
+  @  1:b154670c87da split 1
+  |
+  o  0:51f273a58d82 initial
+  
+#endif
diff --git a/mercurial/patch.py b/mercurial/patch.py
--- a/mercurial/patch.py
+++ b/mercurial/patch.py
@@ -863,7 +863,7 @@ 
     diff_re = re.compile('diff -r .* (.*)$')
     allhunks_re = re.compile('(?:index|deleted file) ')
     pretty_re = re.compile('(?:new file|deleted file) ')
-    special_re = re.compile('(?:index|deleted|copy|rename) ')
+    special_re = re.compile('(?:index|deleted|copy|rename|new mode) ')
     newfile_re = re.compile('(?:new file)')
 
     def __init__(self, header):