Re: [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values
"Philip Oakley"writes: >> We do not know until this change is released to the wild, at which >> time we will hear noises about the lack of expected ellipses their >> (poorly written) scripts rely on and tell them to set the workaround >> environment variable. We may not hear from such people at all, in >> which case we may be able to remove it within a year or so, but it >> is too early to tell. > > I was wondering if there should be a small documentation change for > the env variable and states that it is a temporary measure for short > term compatibility. Though I'm not sure where the 'right' place would > be for it. We list environment variables that applies git-wide in git(1), I think. If this is going to be only applicable for the "diff" family, Documentation/diff-format.txt may be a better place, and in that case the name of the environment variable may need to include a word DIFF somewhere.
Re: [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values
From: "Junio C Hamano""Philip Oakley" writes: From: "Junio C Hamano" Ann T Ropea writes: *1* We are being overly generous in t4013-diff-various.sh because we do not want to destroy/take apart the here-document. Given that all this a temporary measure, we should get away with it. So, the need to reformat the test for the future post-deprecation period is being deferred to the time that the PRINT_SHA1_ELLIPSIS env variable, and all ellipis, is removed - is that the case? Maybe it just needs saying plainly. And if we say it that way, it is clear that with this series, we are shipping a new feature with a test that does not protect the output format we claim to be the improved and preferred one. That sounds quite bad. Having said that, I have already queued this to 'pu' and I do not terribly mind to merge it down to 'next', leaving the test updates to cover the new output format as well as the backward compatible one at the same time for a later follow-up patch. I'd agree. I just wanted to ensure that I had the right understanding. I'd however hate it if I have to carry the topic in the current shape in 'next' forever, waiting for such an update to come, that may never materialize, and be forced to do it myself without being explicitly asked by (and thanked for) anybody, especially because this is not exactly my itch X-<. True. Or is the env variable being retained as a fallback 'forever'? I'm half guessing that it may tend toward the latter as it's an easier backward compatibility decision. We do not know until this change is released to the wild, at which time we will hear noises about the lack of expected ellipses their (poorly written) scripts rely on and tell them to set the workaround environment variable. We may not hear from such people at all, in which case we may be able to remove it within a year or so, but it is too early to tell. I was wondering if there should be a small documentation change for the env variable and states that it is a temporary measure for short term compatibility. Though I'm not sure where the 'right' place would be for it.
Re: [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values
"Philip Oakley"writes: > From: "Junio C Hamano" >> Ann T Ropea writes: >> >>> *1* We are being overly generous in t4013-diff-various.sh because we do >>> not want to destroy/take apart the here-document. Given that all this a >>> temporary measure, we should get away with it. > > So, the need to reformat the test for the future post-deprecation > period is being deferred to the time that the PRINT_SHA1_ELLIPSIS env > variable, and all ellipis, is removed - is that the case? Maybe it > just needs saying plainly. And if we say it that way, it is clear that with this series, we are shipping a new feature with a test that does not protect the output format we claim to be the improved and preferred one. That sounds quite bad. Having said that, I have already queued this to 'pu' and I do not terribly mind to merge it down to 'next', leaving the test updates to cover the new output format as well as the backward compatible one at the same time for a later follow-up patch. I'd however hate it if I have to carry the topic in the current shape in 'next' forever, waiting for such an update to come, that may never materialize, and be forced to do it myself without being explicitly asked by (and thanked for) anybody, especially because this is not exactly my itch X-<. > Or is the env variable being retained as a fallback 'forever'? I'm > half guessing that it may tend toward the latter as it's an easier > backward compatibility decision. We do not know until this change is released to the wild, at which time we will hear noises about the lack of expected ellipses their (poorly written) scripts rely on and tell them to set the workaround environment variable. We may not hear from such people at all, in which case we may be able to remove it within a year or so, but it is too early to tell.
Re: [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values
From: "Junio C Hamano"Ann T Ropea writes: *1* We are being overly generous in t4013-diff-various.sh because we do not want to destroy/take apart the here-document. Given that all this a temporary measure, we should get away with it. So, the need to reformat the test for the future post-deprecation period is being deferred to the time that the PRINT_SHA1_ELLIPSIS env variable, and all ellipis, is removed - is that the case? Maybe it just needs saying plainly. Or is the env variable being retained as a fallback 'forever'? I'm half guessing that it may tend toward the latter as it's an easier backward compatibility decision. [apologioes this is mid thread, I'm catching up on 2 weeks of emails] I do not think the patch is being particularly generous. If anything, it is being unnecessarily sloppy by not adding new checks to verify the updated behaviour. The above comment mentions "destroy/take apart" the here-document, but I do see no need to destroy anything. All you need to do is to enhance and extend. For example, you could do it like so (this is written in my e-mail client, and not an output of diff, so the indentation etc. may be all off, but should be sufficient to illustrate the idea): while read cmd do case "$cmd" in '' | '#'*) continue ;; esac test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g') pfx=$(printf "%04d" $test_count) expect="$TEST_DIRECTORY/t4013/diff.$test" actual="$pfx-diff.$test" +case "$cmd" in +X*) cmd=${cmd#X}; no_ellipses=" (no ellipses)" ;; +*) no_ellipses= ;; +esac -test_expect_success "git $cmd" ' +test_expect_success "git $cmd$no_ellipses" ' { echo "\$ git $cmd" -git $cmd | +if test -n "$no_ellipses" +then +git $cmd +else +PRINT_SHA1_ELLIPSES=yes git $cmd +fi | sed -e ... done <<\EOF diff-tree initial diff-tree -r initial diff-tree -r --abbrev initial diff-tree -r --abbrev=4 initial +Xdiff-tree -r --abbrev=4 initial ... EOF There is a new and duplicated line with a prefix X for one existing test in the above. The idea is that the ones marked as such will test and verify the effect of this new behaviour by not setting the environment variable. The expected and actual test output for the new test will have X prefixed to it. t4013 is arranged in such a way that it is easy to add a new test like this---you only need to add an expected output in a new file in t/t4013/. directory. And the output with these ellipses removed will be something we would expect see in the new world (without the escape hatch environment variable), we would need to add a new file there to record what the expected output from the command is. I singled out the diff-tree invocation with --abbrev=4 as an example in the above, but in a more thorough final version, we'd need to cover both "abbreviation with ellipses" and "abbreviation without ellipses" output for other lines in the test case listed in the here-document.
Re: [PATCH v3 5/5] Testing: provide tests requiring them with ellipses after SHA-1 values
Ann T Ropeawrites: > *1* We are being overly generous in t4013-diff-various.sh because we do > not want to destroy/take apart the here-document. Given that all this a > temporary measure, we should get away with it. I do not think the patch is being particularly generous. If anything, it is being unnecessarily sloppy by not adding new checks to verify the updated behaviour. The above comment mentions "destroy/take apart" the here-document, but I do see no need to destroy anything. All you need to do is to enhance and extend. For example, you could do it like so (this is written in my e-mail client, and not an output of diff, so the indentation etc. may be all off, but should be sufficient to illustrate the idea): while read cmd do case "$cmd" in '' | '#'*) continue ;; esac test=$(echo "$cmd" | sed -e 's|[/ ][/ ]*|_|g') pfx=$(printf "%04d" $test_count) expect="$TEST_DIRECTORY/t4013/diff.$test" actual="$pfx-diff.$test" +case "$cmd" in +X*) cmd=${cmd#X}; no_ellipses=" (no ellipses)" ;; +*) no_ellipses= ;; +esac -test_expect_success "git $cmd" ' +test_expect_success "git $cmd$no_ellipses" ' { echo "\$ git $cmd" -git $cmd | +if test -n "$no_ellipses" +then +git $cmd +else +PRINT_SHA1_ELLIPSES=yes git $cmd +fi | sed -e ... done <<\EOF diff-tree initial diff-tree -r initial diff-tree -r --abbrev initial diff-tree -r --abbrev=4 initial +Xdiff-tree -r --abbrev=4 initial ... EOF There is a new and duplicated line with a prefix X for one existing test in the above. The idea is that the ones marked as such will test and verify the effect of this new behaviour by not setting the environment variable. The expected and actual test output for the new test will have X prefixed to it. t4013 is arranged in such a way that it is easy to add a new test like this---you only need to add an expected output in a new file in t/t4013/. directory. And the output with these ellipses removed will be something we would expect see in the new world (without the escape hatch environment variable), we would need to add a new file there to record what the expected output from the command is. I singled out the diff-tree invocation with --abbrev=4 as an example in the above, but in a more thorough final version, we'd need to cover both "abbreviation with ellipses" and "abbreviation without ellipses" output for other lines in the test case listed in the here-document.