{"thread":{"id":"11554","subject":"[RFC] refer to post-patch lines in whitespace warnings","startedAt":"2008-01-09T15:57:40Z","lastAt":"2008-01-09T21:35:37Z","messageCount":4,"participants":["Daniel Barkalow","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"64858","messageId":"alpine.LNX.1.00.0801091046100.13593@iabervon.org","threadId":"11554","inReplyTo":null,"subject":"[RFC] refer to post-patch lines in whitespace warnings","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-09T15:57:40Z","receivedAt":"2008-01-09T15:57:40Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"When I rebase series with bad whitespace, I end up with unhelpful messages \nlike:\n\n.dotest/patch:412: trailing whitespace.\n-- \n.dotest/patch:446: trailing whitespace.\n-- \n\nThese line numbers obviously refer to lines in a file that's been removed \nby the time I can do anything about it. It seems to me like the message \nwould be more useful if, in the case where it leaves the working tree \nmodified with the non-compliant whitespace, it gave this location rather \nthan the patch's location (because, even if you have the patch still, \nyou'd need to revert it first in order to be able to apply a fixed version \nanyway). Anybody see any problems with this theory?\n\n\t-Daniel\n*This .sig left intentionally blank*\n"},{"id":"64866","messageId":"7vy7ayhhpj.fsf@gitster.siamese.dyndns.org","threadId":"11554","inReplyTo":"alpine.LNX.1.00.0801091046100.13593@iabervon.org","subject":"Re: [RFC] refer to post-patch lines in whitespace warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-09T20:22:48Z","receivedAt":"2008-01-09T20:22:48Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> When I rebase series with bad whitespace, I end up with unhelpful messages \n> like:\n>\n> .dotest/patch:412: trailing whitespace.\n> -- \n> .dotest/patch:446: trailing whitespace.\n> -- \n>\n> These line numbers obviously refer to lines in a file that's been removed \n> by the time I can do anything about it.\n\nThe message is more appropriate for a workflow to \"git apply --check\"\nfirst, fix the patchfile and then applying for real.\n\n> ... if, in the case where it leaves the working tree \n> modified with the non-compliant whitespace, it gave this location rather \n> than the patch's location (because, even if you have the patch still, \n> you'd need to revert it first in order to be able to apply a fixed version \n> anyway).\n\nIn such a case, \"git diff\" will highlight the non-compliant whitespace.\n\nMore problematic is if you used whitespace=warn to let it commit anyway.\nYou can use \"git diff $beginning_of_series..HEAD\" the same way\nto locate the breakages, but you then need to do \"rebase -i\" to\nfix it up (I personally would run \"format-patch\", fix the problems in\nthe patch text, and run \"am\", instead of \"rebase -i\", mostly\nbecause I am used to working that way).\n"},{"id":"64870","messageId":"7vabnehgt3.fsf@gitster.siamese.dyndns.org","threadId":"11554","inReplyTo":"alpine.LNX.1.00.0801091046100.13593@iabervon.org","subject":"Re: [RFC] refer to post-patch lines in whitespace warnings","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-09T20:42:16Z","receivedAt":"2008-01-09T20:42:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Daniel Barkalow <barkalow@iabervon.org> writes:\n\n> When I rebase series with bad whitespace, I end up with unhelpful messages \n> like:\n>\n> .dotest/patch:412: trailing whitespace.\n> -- \n> .dotest/patch:446: trailing whitespace.\n> -- \n>\n> These line numbers obviously refer to lines in a file that's been removed \n> by the time I can do anything about it. It seems to me like the message \n> would be more useful if, in the case where it leaves the working tree \n> modified with the non-compliant whitespace, it gave this location rather \n> than the patch's location (because, even if you have the patch still, \n> you'd need to revert it first in order to be able to apply a fixed version \n> anyway). Anybody see any problems with this theory?\n\nI realize that I did not answer your primary question in the\nprevious response.\n\nI think it is fine if you are thinking about _adding_ line\nnumber of postimage (or preimage for that matter) to the warning\noutput, but I do not think we would want to remove the in-patch\nline numbers we currently have and replace them with something\nelse.  I often very much appreciate the fact that these messages\nprecisely identify the problematic spots in the patch so that I\ncan go in and fix them in place before applying.\n"},{"id":"64877","messageId":"alpine.LNX.1.00.0801091525030.13593@iabervon.org","threadId":"11554","inReplyTo":"7vy7ayhhpj.fsf@gitster.siamese.dyndns.org","subject":"Re: [RFC] refer to post-patch lines in whitespace warnings","fromName":"Daniel Barkalow","fromEmail":"barkalow@iabervon.org","sentAt":"2008-01-09T21:35:37Z","receivedAt":"2008-01-09T21:35:37Z","isPatch":false,"sender":{"key":"barkalow@iabervon.org","avatar":"https://avatars.githubusercontent.com/u/55364219?v=4"},"body":"On Wed, 9 Jan 2008, Junio C Hamano wrote:\n\n> Daniel Barkalow <barkalow@iabervon.org> writes:\n> \n> > When I rebase series with bad whitespace, I end up with unhelpful messages \n> > like:\n> >\n> > .dotest/patch:412: trailing whitespace.\n> > -- \n> > .dotest/patch:446: trailing whitespace.\n> > -- \n> >\n> > These line numbers obviously refer to lines in a file that's been removed \n> > by the time I can do anything about it.\n> \n> The message is more appropriate for a workflow to \"git apply --check\"\n> first, fix the patchfile and then applying for real.\n\nRight; I'd keep it the same for all cases in which the patch is not \napplied: \"git apply --check\" (which doesn't apply it regardless) or when \nwhitespace errors prevent application; so the message would be, in either \ncase, appropriate to the workflow.\n\nIn fact, in your workflow, it wouldn't make any sense to give the \nresulting location of the whitespace, because that version of the file \nhasn't been created. I think that only one of the possible location \nreports is actually helpful: if it wasn't applied, the line in the patch \nfile; if it was, the line in the working tree.\n\n> > ... if, in the case where it leaves the working tree \n> > modified with the non-compliant whitespace, it gave this location rather \n> > than the patch's location (because, even if you have the patch still, \n> > you'd need to revert it first in order to be able to apply a fixed version \n> > anyway).\n> \n> In such a case, \"git diff\" will highlight the non-compliant whitespace.\n> \n> More problematic is if you used whitespace=warn to let it commit anyway.\n> You can use \"git diff $beginning_of_series..HEAD\" the same way\n> to locate the breakages, but you then need to do \"rebase -i\" to\n> fix it up (I personally would run \"format-patch\", fix the problems in\n> the patch text, and run \"am\", instead of \"rebase -i\", mostly\n> because I am used to working that way).\n\nI should point out that, in this particular series, the non-compliant \nwhitespace is in the \"expected result\" file for tests for email message \ngeneration, and it actually has to be like that. The annoying thing is \nthat it's not clear what file has the trailing whitespace, so it's not \nclear that it is supposed to be that way from the message.\n\n\t-Daniel\n*This .sig left intentionally blank*\n"}]}