{"thread":{"id":"43838","subject":"[PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix","startedAt":"2016-08-14T08:56:45Z","lastAt":"2016-08-16T15:39:50Z","messageCount":5,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"299259","messageId":"5ca415f99718b1792cd0a9c31606b45d27befc48.1471164824.git.johannes.schindelin@gmx.de","threadId":"43838","inReplyTo":null,"subject":"[PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-14T08:56:25Z","receivedAt":"2016-08-14T08:56:45Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"The '>' character is not a legal part of filenames on Windows. So let's\njust not use it in Git's source code. This poses a challenge in the test\nscript t4013 which distills command-lines into file names (so that the\nexpected outcome can be stored in files with said names).\n\nWe have to take particular care not to confound the existing conversion\nof unwanted characters to underscores with the new substitution of '>':\nthe existing conversion chose to collapse runs of multiple unwanted\ncharacters into a single underscore. If we allowed '>' to be collapsed,\ntoo, the file name generated from the command \"diff [...]=-- [...]\"\nwould be identical to the one generated from \"diff [...]=--> [...]\".\n\nPlease squash this patch into\n3c90ffd2f01e2d0d080c8e42df2ee89709b324de\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\nPublished-As: https://github.com/dscho/git/releases/tag/mingw-t4013-v1\nFetch-It-Via: git fetch https://github.com/dscho/git mingw-t4013-v1\n\n\tFor the record: this prevented my beautiful CI jobs from even\n\tchecking out the source code for `pu` in the last days.\n\n\tJunio, please let me know if you would prefer this as a separate\n\tpatch.\n\n t/t4013-diff-various.sh                                                 | 2 +-\n ...aster^_side => diff.diff_--diff-line-prefix=--__master_master^_side} | 0\n 2 files changed, 1 insertion(+), 1 deletion(-)\n rename t/t4013/{diff.diff_--diff-line-prefix=-->_master_master^_side => diff.diff_--diff-line-prefix=--__master_master^_side} (100%)\n\ndiff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh\nindex 5204645..84e2ee0 100755\n--- a/t/t4013-diff-various.sh\n+++ b/t/t4013-diff-various.sh\n@@ -115,7 +115,7 @@ do\n \tcase \"$cmd\" in\n \t'' | '#'*) continue ;;\n \tesac\n-\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g')\n+\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g' -e 'y/>/_/')\n \tpfx=$(printf \"%04d\" $test_count)\n \texpect=\"$TEST_DIRECTORY/t4013/diff.$test\"\n \tactual=\"$pfx-diff.$test\"\ndiff --git a/t/t4013/diff.diff_--diff-line-prefix=-->_master_master^_side b/t/t4013/diff.diff_--diff-line-prefix=--__master_master^_side\nsimilarity index 100%\nrename from t/t4013/diff.diff_--diff-line-prefix=-->_master_master^_side\nrename to t/t4013/diff.diff_--diff-line-prefix=--__master_master^_side\n-- \n2.9.2.691.g78954f3\n\nbase-commit: 945e149951a152207b56d5e49ff5167d151a4c89\n"},{"id":"299286","messageId":"xmqqinv3krpe.fsf@gitster.mtv.corp.google.com","threadId":"43838","inReplyTo":"5ca415f99718b1792cd0a9c31606b45d27befc48.1471164824.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-14T20:42:37Z","receivedAt":"2016-08-14T20:59:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> -\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g')\n> +\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g' -e 'y/>/_/')\n\nThe existing sed scriptlet says \"we cannot have slash and do not\nwant to have space in filename, so we squash runs of them to a\nsingle underscore\".  If you have more characters that you do not\nwant, you should add that to the existing set instead.\n\nWhile you are at it, it may be sensible to add a colon to that set,\ntoo, no?\n\nSomething like this, perhaps?\n\n> -\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g')\n> +\ttest=$(echo \"$cmd\" | sed -e 's|[/ <:>][/ <:>]*|_|g')\n"},{"id":"299345","messageId":"alpine.DEB.2.20.1608151519480.4924@virtualbox","threadId":"43838","inReplyTo":"xmqqinv3krpe.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-15T14:07:21Z","receivedAt":"2016-08-15T14:08:28Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Sun, 14 Aug 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> \n> > -\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g')\n> > +\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g' -e 'y/>/_/')\n> \n> The existing sed scriptlet says \"we cannot have slash and do not\n> want to have space in filename, so we squash runs of them to a\n> single underscore\".  If you have more characters that you do not\n> want, you should add that to the existing set instead.\n\nNo, we cannot do that. I even mentioned it in my commit message why:\n\n\tWe have to take particular care not to confound the existing\n\tconversion of unwanted characters to underscores with the new\n\tsubstitution of '>': the existing conversion chose to collapse\n\truns of multiple unwanted characters into a single underscore. If\n\twe allowed '>' to be collapsed, too, the file name generated from\n\tthe command \"diff [...]=-- [...]\" would be identical to the one\n\tgenerated from \"diff [...]=--> [...]\".\n\nAnd as there is exactly this case (two command-lines, differing only in\nthe '>' character), doing what you suggested would *break* the test since\nit would now look at the wrong file.\n\nI know that this is so because my first iteration of the patch did exactly\nwhat you suggested.\n\n> While you are at it, it may be sensible to add a colon to that set, too,\n> no?\n> \n> Something like this, perhaps?\n> \n> > -\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g')\n> > +\ttest=$(echo \"$cmd\" | sed -e 's|[/ <:>][/ <:>]*|_|g')\n\nMaybe. But what other characters are missing? Those are not the only ones\nillegal on Windows: [/ \\0-\\x1f\"*:<>?\\\\|] would be a more complete version\nof what you suggested. Except that it is not a valid basic regex.\n\nBut is that really necessary?\n\nI think not, for the following reasons:\n\n- my patch was specifically designed to stop my CI from pestering me about\n  a totally broken revision that cannot even be checked out (and causes\n  subsequent problems because of it),\n\n- as such, my patch was meant not to be an all-encompassing solution to\n  the problem of filenames that are illegal on Windows, but really a tiny\n  patch that you could apply as quickly as possible so that my CI jobs\n  would stop pestering me (which they did not, because `pu` is still\n  broken),\n\n- I even meant this little patch to be so small that it can be easily\n  squashed into Jacob's patch,\n\n- I do not want to complicate regular expressions unless *really* needed\n  (and you have to admit that we do not need to address any more\n  characters than the '>' *right now*), as\n\n\t- regular expressions are not exactly an easy meal, so it makes\n\t  sense to keep them as simple as possible both for contributors'\n\t  as well as for maintainers' sake, and\n\n\t- when trying to come up with a super-complete solution, it is\n\t  really easy not only to spend waaaaay too much time on it, but\n\t  also to introduce bugs that are not spotted for a loooong time\n\t  because nothing actually exercises the newly introduced code, and\n\n\t- If It Ain't Broke, Don't Fix It.\n\n- the broader solution must come separately, and not as a mere add-on to\n  one test case: we really need to ensure that such file names do not\n  enter Git's source code.\n\nI will send my proposal to address the larger issue in a moment, in the\nmeantime I *beg* you to add this here patch as a quick fix to my CI woes,\neither by squashing it into the indicated commit, or by appending it to\nthe topic branch. I do not care which one, as long as `pu` gets fixed.\n\nThanks,\nDscho\n\n\n"},{"id":"299355","messageId":"xmqqfuq6humf.fsf@gitster.mtv.corp.google.com","threadId":"43838","inReplyTo":"alpine.DEB.2.20.1608151519480.4924@virtualbox","subject":"Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-15T16:20:08Z","receivedAt":"2016-08-15T16:21:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> On Sun, 14 Aug 2016, Junio C Hamano wrote:\n>\n>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>> \n>> > -\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g')\n>> > +\ttest=$(echo \"$cmd\" | sed -e 's|[/ ][/ ]*|_|g' -e 'y/>/_/')\n>> ...\n> I know that this is so because my first iteration of the patch did exactly\n> what you suggested.\n\nYou probably should have stepped back and taken a deep breath before\nwriting the second round.  Doing so after writing it before sending\nwould also have been OK.\n\nAmong three characters that are special cased here, the problem \"if\nwe squish a run of problematic characters into one underscore, we\nrisk making the result ambiguous\" is NOT limited to '>'; it is not a\nnew problem with '>', either.  I can think of two other possible\nsolutions offhand:\n\n (1) drop \"squishing a run\", i.e. [/ ]*, from the regexp, and rename\n     existing test vectors that would be affected;\n\n (2) change the string used in the offending test so that squishing\n     will not make the result ambiguous.\n\nbefore special casing \"y/>/_/\"; as there is nothing in \">\" that\ninherently causes the ambiguity that won't be caused by \"/\" or \" \",\nadding second clause to the sed expression that does things\ndifferently only for \">\" is simply wrong.  After all, you only\nwanted to affect the \"prefix=-->\" test and not the whole set of the\ntests in the script.\n\nObviously (1) is a lot of impact with little gain, and as Jacob\nalready offered to do, I think (2) is a lot more sensible solution\nand it also is more in line with your \"If it isn't broken, do not\nfix it\", I would say.\n\n\n"},{"id":"299465","messageId":"alpine.DEB.2.20.1608161738440.4924@virtualbox","threadId":"43838","inReplyTo":"xmqqfuq6humf.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] squash! diff: add --diff-line-prefix option for passing in a prefix","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-08-16T15:39:04Z","receivedAt":"2016-08-16T15:39:50Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Mon, 15 Aug 2016, Junio C Hamano wrote:\n\n> Obviously (1) is a lot of impact with little gain, and as Jacob\n> already offered to do, I think (2) is a lot more sensible solution\n> and it also is more in line with your \"If it isn't broken, do not\n> fix it\", I would say.\n\nYep, that makes most sense to me, too.\n\nCiao,\nDscho\n"}]}