Re: [RFC] refer to post-patch lines in whitespace warnings

Previous thread: [PATCH] gitk: Fix the Makefile to cope with systems lacking msgfmt by Charles Bailey on Wednesday, January 9, 2008 - 10:31 am. (1 message)

Next thread: [PATCH] Change git-gc documentation to reflect gc.packrefs implementation. by Florian La Roche on Wednesday, January 9, 2008 - 12:05 pm. (1 message)
To: <git@...>
Date: Wednesday, January 9, 2008 - 11:57 am

When I rebase series with bad whitespace, I end up with unhelpful messages
like:

.dotest/patch:412: trailing whitespace.
--
.dotest/patch:446: trailing whitespace.
--

These line numbers obviously refer to lines in a file that's been removed
by the time I can do anything about it. It seems to me like the message
would be more useful if, in the case where it leaves the working tree
modified with the non-compliant whitespace, it gave this location rather
than the patch's location (because, even if you have the patch still,
you'd need to revert it first in order to be able to apply a fixed version
anyway). Anybody see any problems with this theory?

-Daniel
*This .sig left intentionally blank*
-

To: Daniel Barkalow <barkalow@...>
Cc: <git@...>
Date: Wednesday, January 9, 2008 - 4:42 pm

I realize that I did not answer your primary question in the
previous response.

I think it is fine if you are thinking about _adding_ line
number of postimage (or preimage for that matter) to the warning
output, but I do not think we would want to remove the in-patch
line numbers we currently have and replace them with something
else. I often very much appreciate the fact that these messages
precisely identify the problematic spots in the patch so that I
can go in and fix them in place before applying.
-

To: Daniel Barkalow <barkalow@...>
Cc: <git@...>
Date: Wednesday, January 9, 2008 - 4:22 pm

The message is more appropriate for a workflow to "git apply --check"

In such a case, "git diff" will highlight the non-compliant whitespace.

More problematic is if you used whitespace=warn to let it commit anyway.
You can use "git diff $beginning_of_series..HEAD" the same way
to locate the breakages, but you then need to do "rebase -i" to
fix it up (I personally would run "format-patch", fix the problems in
the patch text, and run "am", instead of "rebase -i", mostly
because I am used to working that way).

-

To: Junio C Hamano <gitster@...>
Cc: <git@...>
Date: Wednesday, January 9, 2008 - 5:35 pm

Right; I'd keep it the same for all cases in which the patch is not
applied: "git apply --check" (which doesn't apply it regardless) or when
whitespace errors prevent application; so the message would be, in either
case, appropriate to the workflow.

In fact, in your workflow, it wouldn't make any sense to give the
resulting location of the whitespace, because that version of the file
hasn't been created. I think that only one of the possible location
reports is actually helpful: if it wasn't applied, the line in the patch

I should point out that, in this particular series, the non-compliant
whitespace is in the "expected result" file for tests for email message
generation, and it actually has to be like that. The annoying thing is
that it's not clear what file has the trailing whitespace, so it's not
clear that it is supposed to be that way from the message.

-Daniel
*This .sig left intentionally blank*
-

Previous thread: [PATCH] gitk: Fix the Makefile to cope with systems lacking msgfmt by Charles Bailey on Wednesday, January 9, 2008 - 10:31 am. (1 message)

Next thread: [PATCH] Change git-gc documentation to reflect gc.packrefs implementation. by Florian La Roche on Wednesday, January 9, 2008 - 12:05 pm. (1 message)