Page MenuHomePhabricator

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

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



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

rHG Mercurial
Automatic diff as part of commit; lint not applicable.
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

khanchi97 created this revision.Mar 3 2019, 5:37 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

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.

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

#if ..
#if ..

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.

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

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
This revision was automatically updated to reflect the committed changes.