This is an archive of the discontinued Mercurial Phabricator instance.

Differential D6058

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

Authored by khanchi97 on Mar 3 2019, 5:37 PM.

Details

Reviewers
mharbison72
martinvonz
Group Reviewers
hg-reviewers
Commits
rHGf8c5225b9054: patch: include flag-only file changes in "special" when filtering (issue5864)
rHG6308aa828cf4: patch: include flag-only file changes in "special" while filtering patch…
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.

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

khanchi97 created this revision.Mar 3 2019, 5:37 PM
khanchi97 added a comment.Mar 8 2019, 11:52 PM

Ping for review.

khanchi97 updated this revision to Diff 14485.Mar 13 2019, 3:21 PM
khanchi97 added inline comments.Mar 13 2019, 3:27 PM
tests/test-split.t
734

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

mharbison72 requested changes to this revision.Mar 14 2019, 12:47 PM
mharbison72 added a subscriber: mharbison72.
mharbison72 added inline comments.
tests/test-split.t
741

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.

This revision now requires changes to proceed.Mar 14 2019, 12:47 PM
khanchi97 updated this revision to Diff 14524.Mar 16 2019, 10:34 AM
khanchi97 marked an inline comment as done.Mar 16 2019, 10:45 AM
khanchi97 added inline comments.
tests/test-split.t
741

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.

mharbison72 added inline comments.Mar 16 2019, 6:31 PM
tests/test-split.t
741

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.

khanchi97 marked an inline comment as done.Mar 16 2019, 11:22 PM

Thanks, it is more clear now.

martinvonz accepted this revision.Mar 18 2019, 7:32 PM
Closed by commit rHG6308aa828cf4: patch: include flag-only file changes in "special" while filtering patch… (authored by khanchi97). · Explain WhyMar 18 2019, 7:35 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mmercurial/patch.py (2 lines)
Mtests/test-split.t (39 lines)

Diff 14551

mercurial/patch.py

class header(object): class header(object):
"""patch header """patch header
""" """
diffgit_re = re.compile('diff --git a/(.*) b/(.*)$') diffgit_re = re.compile('diff --git a/(.*) b/(.*)$')
diff_re = re.compile('diff -r .* (.*)$') diff_re = re.compile('diff -r .* (.*)$')
allhunks_re = re.compile('(?:index|deleted file) ') allhunks_re = re.compile('(?:index|deleted file) ')
pretty_re = re.compile('(?:new file|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)') newfile_re = re.compile('(?:new file)')
def __init__(self, header): def __init__(self, header):
self.header = header self.header = header
self.hunks = [] self.hunks = []
def binary(self): def binary(self):
return any(h.startswith('index ') for h in self.header) return any(h.startswith('index ') for h in self.header)

tests/test-split.t

$ hg up -q $ hg up -q
$ hg glog $ hg glog
@ 1:3a2125f0f4cb make executable @ 1:3a2125f0f4cb make executable
| |
o 0:51f273a58d82 initial o 0:51f273a58d82 initial
#if no-windows #if no-windows
khanchi97AuthorUnsubmitted
Not Done

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

khanchi97: I moved the part when on windows (#if windows) at the top because I found it easy to include…
$ printf 'y\ny\ny\n' | hg split $ cat > $TESTTMP/messages <<EOF
> split 1
> EOF
$ printf 'y\n' | hg split
diff --git a/foo b/foo diff --git a/foo b/foo
old mode 100644 old mode 100644
new mode 100755 new mode 100755
mharbison72Unsubmitted
Done

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.

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

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.

khanchi97: Thanks, I have updated the patch. I though it is required to a test in this file to run with…
mharbison72Unsubmitted
Not Done

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.

mharbison72: Correct, you do want to test both cases. But that happens because of the `#testcases`…
examine changes to 'foo'? [Ynesfdaq?] y examine changes to 'foo'? [Ynesfdaq?] y
no changes to record EDITOR: HG: Splitting 3a2125f0f4cb. Write commit message for the first split changeset.
diff --git a/foo b/foo EDITOR: make executable
old mode 100644 EDITOR:
new mode 100755 EDITOR:
examine changes to 'foo'? [Ynesfdaq?] y 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 (obsstore-off !)
no changes to record $ hg log -G -T "{node|short} {desc}\n"
diff --git a/foo b/foo @ b154670c87da split 1
old mode 100644 |
new mode 100755 o 51f273a58d82 initial
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]
#else #else
TODO: Fix this on Windows. See issue 2020 and 5883 TODO: Fix this on Windows. See issue 2020 and 5883
$ printf 'y\ny\ny\n' | hg split $ printf 'y\ny\ny\n' | hg split
abort: cannot split an empty revision abort: cannot split an empty revision
[255] [255]
#endif #endif