| Message ID | pull.1339.git.1661663879.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fix failing t4301 test and &&-chain breakage | expand |
"Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes: > Eric Sunshine (3): > t4301: account for behavior differences between sed implementations > t4031: fix broken &&-chains and add missing loop termination > t4301: emit blank line in more idiomatic fashion > > t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++------------ > 1 file changed, 12 insertions(+), 12 deletions(-) The second one is off by 270.
On Sun, Aug 28, 2022 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote: > "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes: > > t4301: account for behavior differences between sed implementations > > t4031: fix broken &&-chains and add missing loop termination > > t4301: emit blank line in more idiomatic fashion > > The second one is off by 270. Shall I re-roll or will you fix it while queuing (assuming you queue it)?
Eric Sunshine <sunshine@sunshineco.com> writes: > On Sun, Aug 28, 2022 at 4:05 PM Junio C Hamano <gitster@pobox.com> wrote: >> "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes: >> > t4301: account for behavior differences between sed implementations >> > t4031: fix broken &&-chains and add missing loop termination >> > t4301: emit blank line in more idiomatic fashion >> >> The second one is off by 270. > > Shall I re-roll or will you fix it while queuing (assuming you queue it)? I plan to fix it up when I queue.
On Sat, Aug 27, 2022 at 10:18 PM Eric Sunshine via GitGitGadget <gitgitgadget@gmail.com> wrote: > > This series fixes a failing test in t4301 due to 'sed' behavioral > differences between implementations. It also fixes a couple broken &&-chains > and adds missing explicit loop termination. > > The third patch is entirely subjective and can be dropped if unwanted. I > spent more than a few minutes puzzling over the script's use of 'printf > "\\n"' rather than the more typical 'printf "\n"' or even a simple 'echo', > wondering if there was some subtlety I was missing or whether Elijah had > encountered an unusual situation in which '\\n' was needed over '\n'. The > third patch chooses to replace 'printf "\\n"' with 'echo' which I find more > idiomatic, but I can see value in using 'printf "\n"' as perhaps being > clearer that it is adding a newline where one is missing. I can't actually provide the reasoning for it; I took Dscho's testcase from [1] and used it as a basis for adding several other testcases. When I was copying & pasting and adjusting, I just didn't notice the 'printf "\\n"'. But using a simple echo makes sense. [1] https://lore.kernel.org/git/3b4ed8bb1bb615277ee51a7b2af5fc53bae0a6e4.1660892256.git.gitgitgadget@gmail.com/ Anyway, I've read through the patches and your series looks good to me.
On Sun, Aug 28, 2022 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote: > > "Eric Sunshine via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > Eric Sunshine (3): > > t4301: account for behavior differences between sed implementations > > t4031: fix broken &&-chains and add missing loop termination > > t4301: emit blank line in more idiomatic fashion > > > > t/t4301-merge-tree-write-tree.sh | 24 ++++++++++++------------ > > 1 file changed, 12 insertions(+), 12 deletions(-) > > The second one is off by 270. Apparently Eric knew what you meant, but I'm perplexed by this statement and what it means. What am I missing?
On Mon, Aug 29, 2022 at 10:53 PM Elijah Newren <newren@gmail.com> wrote: > On Sun, Aug 28, 2022 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote: > > > t4301: account for behavior differences between sed implementations > > > t4031: fix broken &&-chains and add missing loop termination > > > t4301: emit blank line in more idiomatic fashion > > > > The second one is off by 270. > > Apparently Eric knew what you meant, but I'm perplexed by this > statement and what it means. What am I missing? Junio's comment was opaque to me, as well, and it took several minutes to figure it out (especially since I authored the patches, thus I read what I expected to read, not what was really there). Taking a look at just prefixes on the patch subject lines... t4301 t4031 t4301 there's a transposition in there which, mathematically speaking, makes one of the test script numbers 270 less than the others.
On Mon, Aug 29, 2022 at 7:56 PM Eric Sunshine <sunshine@sunshineco.com> wrote: > > On Mon, Aug 29, 2022 at 10:53 PM Elijah Newren <newren@gmail.com> wrote: > > On Sun, Aug 28, 2022 at 1:05 PM Junio C Hamano <gitster@pobox.com> wrote: > > > > t4301: account for behavior differences between sed implementations > > > > t4031: fix broken &&-chains and add missing loop termination > > > > t4301: emit blank line in more idiomatic fashion > > > > > > The second one is off by 270. > > > > Apparently Eric knew what you meant, but I'm perplexed by this > > statement and what it means. What am I missing? > > Junio's comment was opaque to me, as well, and it took several minutes > to figure it out (especially since I authored the patches, thus I read > what I expected to read, not what was really there). Taking a look at > just prefixes on the patch subject lines... > > t4301 > t4031 > t4301 > > there's a transposition in there which, mathematically speaking, makes > one of the test script numbers 270 less than the others. Ah, gotcha. Yeah, I totally missed both the digit transposition and the attempt to highlight it. Thanks for the explanation.
Hi Elijah & Eric, On Mon, 29 Aug 2022, Elijah Newren wrote: > On Sat, Aug 27, 2022 at 10:18 PM Eric Sunshine via GitGitGadget > <gitgitgadget@gmail.com> wrote: > > > > This series fixes a failing test in t4301 due to 'sed' behavioral > > differences between implementations. It also fixes a couple broken &&-chains > > and adds missing explicit loop termination. > > > > The third patch is entirely subjective and can be dropped if unwanted. I > > spent more than a few minutes puzzling over the script's use of 'printf > > "\\n"' rather than the more typical 'printf "\n"' or even a simple 'echo', > > wondering if there was some subtlety I was missing or whether Elijah had > > encountered an unusual situation in which '\\n' was needed over '\n'. The > > third patch chooses to replace 'printf "\\n"' with 'echo' which I find more > > idiomatic, but I can see value in using 'printf "\n"' as perhaps being > > clearer that it is adding a newline where one is missing. > > I can't actually provide the reasoning for it; I took Dscho's testcase > from [1] and used it as a basis for adding several other testcases. > When I was copying & pasting and adjusting, I just didn't notice the > 'printf "\\n"'. But using a simple echo makes sense. > > [1] https://lore.kernel.org/git/3b4ed8bb1bb615277ee51a7b2af5fc53bae0a6e4.1660892256.git.gitgitgadget@gmail.com/ No other reason than that I _seem_ to recall having run into some issues where _some_ POSIX shell (was it BusyBox' ash?) did not like the single-escape form "\n". I have no firm recollection, though, and am fine with converting all of the double backslashes to single backslashes (read: I am very indifferent to this issue). Ciao, Dscho