Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

rebase: make --ignore-whitespace available for the interactive machinery #2

Closed
wants to merge 1 commit into from

Conversation

@r1walz
Copy link
Owner

commented Jun 5, 2019

There are two backends available for rebasing, viz, the am and the interactive. Naturally, there shall be some features that are implemented in one but not in the other. One such flag is --ignore-whitespace which indicated merge mechanism to treat lines with only whitespace changes as unchanged.

NB: am's --ignore-space-change (also aliased --ignore-whitespace; see git-apply's description) not only share the same name with diff's --ignore-space-change and merge's -Xignore-space-change, but the similarity in naming appears to have been intentional with am's --ignore-space-change being designed to have the same functionality (see e.g. the commit messages for f008cef ("Merge branch 'jc/apply-ignore-whitespace'", 2014-06-04) and 4e5dd04 ("merge-recursive: options to ignore whitespace changes", 2010-08-26)).

For the most part, these options do provide the same behaviour. However, there are some edge cases where both apply's --ignore-space-change and merge's -Xignore-space-change fall short of optimal behaviour, and that too in different ways. Fixing these differences in edge cases is left for future work.

builtin/rebase.c Show resolved Hide resolved
builtin/rebase.c Outdated Show resolved Hide resolved
@tgummerer
Copy link

left a comment

Mainly trying to address your questions here. I'll try to do a more in-depth review tomorrow

builtin/rebase--interactive.c Outdated Show resolved Hide resolved
Documentation/git-rebase.txt Outdated Show resolved Hide resolved
Documentation/git-rebase.txt Show resolved Hide resolved
Documentation/git-rebase.txt Show resolved Hide resolved
builtin/rebase--interactive.c Outdated Show resolved Hide resolved
builtin/rebase--interactive.c Outdated Show resolved Hide resolved
builtin/rebase--interactive.c Outdated Show resolved Hide resolved
builtin/rebase.c Show resolved Hide resolved
builtin/rebase.c Outdated Show resolved Hide resolved
t/t3422-rebase-incompatible-options.sh Show resolved Hide resolved

@r1walz r1walz force-pushed the rebase-i--ignore-whitespace branch from f7a8057 to dedf19c Jun 6, 2019

@tgummerer
Copy link

left a comment

Just a couple more questions from my side. I think it would still be nice if we could come up with an automated test for this.

builtin/rebase.c Outdated Show resolved Hide resolved
builtin/rebase.c Outdated Show resolved Hide resolved
builtin/rebase.c Show resolved Hide resolved
t/t3422-rebase-incompatible-options.sh Show resolved Hide resolved

@r1walz r1walz force-pushed the rebase-i--ignore-whitespace branch 2 times, most recently from 0de4e82 to ab129af Jun 7, 2019

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jun 7, 2019

@tgummerer @newren Just so you know, using DEVOPTS = pedantic in config.mak spits out this error:

In file included from remote.h:5:0,
                 from walker.h:4,
                 from walker.c:2:
parse-options.h:53:14: error: ISO C forbids forward references to ‘enum’ types [-Werror=pedantic]
 typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
              ^~~~~~~~~~~~~~~~
In file included from remote.h:5:0,
                 from wt-status.h:8,
                 from wt-status.c:2:
parse-options.h:53:14: error: ISO C forbids forward references to ‘enum’ types [-Werror=pedantic]
 typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
              ^~~~~~~~~~~~~~~~
In file included from remote.h:5:0,
                 from wt-status.h:8,
                 from worktree.c:7:
parse-options.h:53:14: error: ISO C forbids forward references to ‘enum’ types [-Werror=pedantic]
 typedef enum parse_opt_result parse_opt_ll_cb(struct parse_opt_ctx_t *ctx,
              ^~~~~~~~~~~~~~~~

(cf. v2.22.0-rc3)

@tgummerer

This comment has been minimized.

Copy link

commented Jun 7, 2019

Yeah I did run into that before, but didn't have time to look into it, and wasn't curious enough to try and fix it. It's probably something that would be good to fix eventually, but I guess not too many people are running with DEVOPTS = pedantic.

This flag is either passed to the 'git apply' program
(see linkgit:git-apply[1]), or to 'git merge' program
(see linkgit:git-merge[1]) as `-Xignore-space-change`,
depending on which backend is selected by other options.

This comment has been minimized.

Copy link
@tgummerer

tgummerer Jun 7, 2019

nit: I think "by other options" can be dropped, as that would be implied. I don't mind it as it is either though.

builtin/rebase.c Outdated Show resolved Hide resolved
t/t3431-rebase-ignore-ws.sh Outdated Show resolved Hide resolved
t/t3431-rebase-ignore-ws.sh Outdated Show resolved Hide resolved
t/t3431-rebase-ignore-ws.sh Outdated Show resolved Hide resolved
t/t3431-rebase-ignore-ws.sh Outdated Show resolved Hide resolved
t/t3431-rebase-ignore-ws.sh Outdated Show resolved Hide resolved

@r1walz r1walz force-pushed the rebase-i--ignore-whitespace branch 2 times, most recently from c46190b to c0d0753 Jun 7, 2019

@tgummerer
Copy link

left a comment

Sorry, I failed to notice the force push again here. I think the code here looks good.

I would order the patches in the reverse order though. First adding the new test, adding test coverage for the option which we're going to touch in the next commit, so we can make sure it works the same way before and after. That has the added benefit of then having the second commit focused only on the code changes, and the new test case, without any test setup to review, as that has been done in the first patch already.

'

test_expect_success '--ignore-whitespace works with am backend' '
test_must_fail git rebase main side &&

This comment has been minimized.

Copy link
@tgummerer

tgummerer Jun 12, 2019

Is there any output we can check to make sure this failed the way we expected?

This comment has been minimized.

Copy link
@r1walz

r1walz Jun 12, 2019

Author Owner

🤔 yes, we can.

This comment has been minimized.

Copy link
@r1walz

r1walz Jun 18, 2019

Author Owner

I checked it and git rebase main side does not uniquely produce anything which relates it to whitespace errors.

test_expect_success '--ignore-whitespace works with am backend' '
test_must_fail git rebase main side &&
git rebase --abort &&
git rebase --ignore-whitespace main side

This comment has been minimized.

Copy link
@tgummerer

tgummerer Jun 12, 2019

Maybe check that this had the right result?

This comment has been minimized.

Copy link
@r1walz

r1walz Jun 12, 2019

Author Owner

🤔 we can, but --ignore-whitespace in both cases will produce slightly different results. (but no error ofc)

This comment has been minimized.

Copy link
@tgummerer

tgummerer Jun 12, 2019

Ah right, and that's something we want to fix eventually, so it's better not to encode it in a test. Just testing for success makes sense to me then

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jun 12, 2019

@tgummerer wrote:

I would order the patches in the reverse order though. [...] That has the added benefit of then having the second commit focused only on the code changes, and the new test case, without any test setup to review, as that has been done in the first patch already.

Makes sense. I'll rearrange them 👍

@r1walz r1walz force-pushed the rebase-i--ignore-whitespace branch from c0d0753 to 41514b3 Jun 18, 2019

@tgummerer
Copy link

left a comment

Just a bit in the commit message. Code wise this looks good to me (although as usual you might get some more feedback on the mailing list from experts in this area). 99d8cc7 seems to only be the commit adding the flag to the builtin rebase, during the conversion from a shell script. However I'd assume the flag existed before that. Probably worth digging a bit deeper into the history.

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jun 21, 2019

However I'd assume the flag existed before that. Probably worth digging a bit deeper into the history.

Ok, I'll find that commit and reference it here.

@newren

This comment has been minimized.

Copy link

commented Jun 21, 2019

One minor comment: I find it more typical for people to refer to "Commit abc123def456" rather than "The commit abc123def456" (in one of your commit messages).

Also, I'm curious whether you tested to verify whether --ignore-whitespace and -Xignore-space-change had the suboptimal behavior we inferred from their documentation. Did you do that? If so then your changes look good to me, but if you haven't yet then I'm worried about the assertion in the commit message -- it's possible the implementation actually does the right thing and the documentation is what is wrong, after all.

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jun 24, 2019

Commit 86c91f9 ("git apply: option to ignore whitespace differences", 2009-08-04) actually introduced --ignore--whitespace to git-apply and git-rebase--am passed the flag to git-am which carry-forwards it to git-apply. Furthermore, the test for git apply --ignore-whitespace was added in the patch which kinda makes adding for git (rebase | am) --ignore-whitespace moot. Do we actually need test for rebase--am --ignore-whitespace? Perhaps as a proof of concept?

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jun 24, 2019

@newren wrote:

Also, I'm curious whether you tested to verify whether --ignore-whitespace and -Xignore-space-change had the suboptimal behavior we inferred from their documentation. Did you do that?

I believe that I looked into the code to find the same. Please confirm yourself, I might have missed something.

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jun 24, 2019

Also, I am actually a bit confused, there is no such concept of context lines in the merge mechanism, is there?

For the most part, these options do provide the same behaviour.

They are fundamentally different since we are ignoring whitespaces only in context line with --ignore-whitespace (am's) and ignoring whitespaces in all modified lines with -Xignore-space-change (merge's).

@newren

This comment has been minimized.

Copy link

commented Jun 24, 2019

Also, I am actually a bit confused, there is no such concept of context lines in the merge mechanism, is there?

For the most part, these options do provide the same behaviour.

They are fundamentally different since we are ignoring whitespaces only in context line with --ignore-whitespace (am's) and ignoring whitespaces in all modified lines with -Xignore-space-change (merge's).

Yes, but as I noted before ignoring whitespaces only in the context lines is a bug (or at least an ugly shortcoming of apply's --ignore-whitespace). If the user has a patch:
@@ -<old:starting line,num lines> +<new:starting line, num lines>
B
-C
+E
D

(where each capital letter represents multiple lines), and the existing file is of the form
B
C*
D

where C* differs from C only in whitespace, then it's super lame that apply's --ignore-whitespace won't apply the patch (or so I assume from the documentation; that's one of the two things I wanted you to test).

Likewise, assuming the documentation is correct, the -Xignore-space-change option is also really lame in that if our version made no changes and their version only made whitespace changes, our version is used anyway making this option delete whitespace-fixes even when it doesn't need to. (That would be really annoying if someone created a commit whose entire purpose was cleaning up the whitespace in all files to match some local coding standards.)

I think both these shortcomings should be viewed as bugs -- not just in the implementation but also in the documentation; neither are what the user would actually want, they are simply an artifact of a developer attempting to implement what was wanted, not noticing the difference between their algorithm and what is wanted, and exposing the algorithm in the documentation. (But again, I haven't tested myself that the implementation matches the documentation in these two cases; since you say you have but want me to check, I'll try to find some time to do so.)

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jun 25, 2019

We can apply:

diff --git a/main.c b/main.c
--- a/main.c
+++ b/main.c
@@ -3,4 +3,4 @@
 int main() {
-^Iprintf("Hello World\n");
+^Iprintf("Hello Git\n");
 ^Ireturn 0;
 }

on:

#include <stdio.h>

int main() {
       printf("Hello World\n");
^Ireturn 0;
}

using --ignore-whitespace. But, it does not work for multiline. Here is a gist. Hence, apply's ignore-whitespace is flawed.

@tgummerer

This comment has been minimized.

Copy link

commented Jun 27, 2019

Do we actually need test for rebase--am --ignore-whitespace? Perhaps as a proof of concept?

Sorry this question slipped by me. I think it would be nice to test rebase--am --ignore-whitespace as after this patch series it doesn't just pass through the argument directly (although it almost still does). I don't think it's absolutely necessary, but rather a nice to have in this case.

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jul 5, 2019

@tgummerer @newren: now that my first patch is going to merge in 'next'. Let's review this for one last time and send to the mailing list. Particularly check if the anomaly in -Xignore-space-change is indeed what the documentation says.

@tgummerer

This comment has been minimized.

Copy link

commented Jul 5, 2019

On IRC we had discussed moving the tests to t3402. Did you decide against that?

I'll let @newren comment on the -Xignore-space-change vs. --ignore-whitespace differences (though if you don't have time to check it out, I can give it a go).

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jul 5, 2019

@tgummerer wrote:

On IRC we had discussed moving the tests to t3402. Did you decide against that?

Not exactly. I am thinking of changing the name to t3431-rebase-options.sh.

(though if you don't have time to check it out, I can give it a go).

I'd love if you can give it a try.
Thanks!

@tgummerer

This comment has been minimized.

Copy link

commented Jul 7, 2019

Not exactly. I am thinking of changing the name to t3431-rebase-options.sh.

Hmm, I'm not sure I like that name. It's very generic, so I would wonder which options are tested in that file, and why only some of the options are tested in the file (we probably don't want to write new tests for all the existing options). We could name it t3431-rebase-option-compatibility.sh, but then I would expect that all options are fully compatible, and do the exact same thing, which is probably not true at least for the --ignore-whitespace option, if my understanding of that is correct.

I'd love if you can give it a try.

Ok, I'll try to set some time aside tomorrow to have a closer look at that. Just so I don't duplicate any work, did you investigate anything more than trying git apply --ignore-whitespace?

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jul 7, 2019

Ok, I'll try to set some time aside tomorrow to have a closer look at that. Just so I don't duplicate any work, did you investigate anything more than trying git apply --ignore-whitespace?

Thank you very much. I don't think I have checked any further of verifying am's --ignore-whitespace.

@tgummerer

This comment has been minimized.

Copy link

commented Jul 8, 2019

So I gave this a try.

Likewise, assuming the documentation is correct, the -Xignore-space-change option is also really lame in that if our version made no changes and their version only made whitespace changes, our version is used anyway making this option delete whitespace-fixes even when it doesn't need to. (That would be really annoying if someone created a commit whose entire purpose was cleaning up the whitespace in all files to match some local coding standards.)

This bit seems to be true, and easy to reproduce. If there are only space changes in "their" version of the file, and some unrelated changes in our version, these space changes get ignored completely in the merge, which does seem a bit lame.

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jul 8, 2019

So, I guess we are clear? Let's move tests to t3402? Also, if we are moving it to t3402, I don't think the test for "am" suits the test file.

@tgummerer

This comment has been minimized.

Copy link

commented Jul 8, 2019

I think so. Moving the tests to t3402 sounds good to me, and yeah we should only have the test for the merge backend there. I think it would be nice assert on the contents of main.c after the test as well (or whatever other file we might be using in that test). I think we should be able to come up with a test case where the am backend and the merge backend have the same outcome with --ignore-whitespace (does your test already)?

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jul 9, 2019

I think we should be able to come up with a test case where the am backend and the merge backend have the same outcome with --ignore-whitespace (does your test already)?

Right now, it doesn't. But we can easily change it to match the outputs (see this comment). Are you implying to add my setup to t3402? I mean the main.c one.

@tgummerer

This comment has been minimized.

Copy link

commented Jul 9, 2019

But we can easily change it to match the outputs (see this comment).

Ah right, what I meant wasn't the output of the command to stdout/stderr, but instead the contents of whatever the file that conflicts on whitespace is.

Are you implying to add my setup to t3402? I mean the main.c one.

You might have to. I haven't looked closely at the test case, but there might not be any files that are useful for the ignore whitespace test. In such a case you can potentially also do the setup combined with the test case, or as a separate test, or in the actual setup step. It all depends on how involved the setup is.

@newren

This comment has been minimized.

Copy link

commented Jul 9, 2019

This round looks pretty good to me. I'm curious if it'd be better to move the comments about edge and corner cases out of the second commit message and in to the cover letter. In other words, copy this text (or something similar) to the cover letter:

"am's --ignore-space-change (also aliased --ignore-whitespace; see
git-apply's description) not only share the same name with diff's
--ignore-space-change and merge's -Xignore-space-change, but the
similarity in naming appears to have been intentional with am's
--ignore-space-change being designed to have the same functionality
(see e.g. the commit messages for f008cef ("Merge branch
'jc/apply-ignore-whitespace'", 2014-06-04) and 4e5dd04
("merge-recursive: options to ignore whitespace chagnes", 2010-08-26))."

...and move this text to the cover letter only:

"For the most part, these options do provide the same behaviour. However,
there are some edge cases where both apply's --ignore-space-change and
merge's -Xignore-space-change fall short of optimal behaviour, and that
too in different ways. Fixing these differences in edge cases is left
for future work;"

This seems like one of those things where either way I choose others on the list might disagree and ask that it be moved to the other location, so it might just mean being flexible and being ready to change it. My thinking, though, is the following: if people ask for more details about how things fall short of being suboptimal, you can explain in response to a cover letter without being expected to revise a commit message, and further details about the edge and corner cases really don't belong in a commit that isn't addressing them.

@r1walz r1walz force-pushed the rebase-i--ignore-whitespace branch from 41514b3 to a60fa9c Jul 10, 2019

t/t3402-rebase-merge.sh Outdated Show resolved Hide resolved
t/t3402-rebase-merge.sh Outdated Show resolved Hide resolved

@r1walz r1walz force-pushed the rebase-i--ignore-whitespace branch 4 times, most recently from 57b42d7 to 6a34e77 Jul 11, 2019

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jul 11, 2019

Let's have a final review? I've updated the description that will be sent to the list as cover-letter.

@newren

This comment has been minimized.

Copy link

commented Jul 12, 2019

Looks good to me for sending on to the list.

@tgummerer
Copy link

left a comment

I agree with @newren, this is ready for the list. I found on minor nit, but feel free to just address that and send the series to the list without another round of review from us.

test_must_fail git rebase --merge main side &&
git rebase --abort &&
git rebase --merge --ignore-whitespace main side &&
test_cmp file output

This comment has been minimized.

Copy link
@tgummerer

tgummerer Jul 12, 2019

minor nit: maybe s/output/expected, that's closer to the usual convention we're using in tests. Also the expected bit should be the first argument in test_cmp to match the convention of other tests.

This comment has been minimized.

Copy link
@r1walz

r1walz Jul 12, 2019

Author Owner

Thanks for approval. I'll send it to the list in the evening.

rebase -i: add --ignore-whitespace flag
There are two backends available for rebasing, viz, the am and the
interactive. Naturally, there shall be some features that are
implemented in one but not in the other. One such flag is
--ignore-whitespace which indicates merge mechanism to treat lines
with only whitespace changes as unchanged. Wire the interactive
rebase to also understand the --ignore-whitespace flag by
translating it to -Xignore-space-change.

Signed-off-by: Rohit Ashiwal <rohit.ashiwal265@gmail.com>

@r1walz r1walz force-pushed the rebase-i--ignore-whitespace branch from 6a34e77 to 4404339 Jul 12, 2019

@r1walz

This comment has been minimized.

Copy link
Owner Author

commented Jul 12, 2019

--- sent to the Mailing List ---

@r1walz r1walz closed this Jul 12, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.