| Message ID | 00fd829833cae1d192d6c42237aa13427156e3ea.1727881164.git.ps@pks.im (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Modernize the build system | expand |
On Wed, Oct 2, 2024 at 11:17 AM Patrick Steinhardt <ps@pks.im> wrote: > Two of our tests in t3404 use indented HERE docs where leading tabs on > some of the lines are actually relevant. The tabs do get removed though, > and we try to fix this up by using sed(1) to replace leading tabs in the > actual output, as well. But on macOS 10.15 this doesn't work as expected > and we somehow keep the tabs around in the actual output. I presume this nebulous explanation is due to the fact that the reason why macOS 10.15 exhibits this anomalous behavior is not yet known? > Work around this issue by retaining the tabs. > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > @@ -1917,18 +1917,17 @@ test_expect_success '--update-refs updates refs correctly' ' > - cat >expect <<-\EOF && > - Successfully rebased and updated refs/heads/update-refs. > - Updated the following refs with --update-refs: > - refs/heads/first > - refs/heads/no-conflict-branch > - refs/heads/second > - refs/heads/third > - EOF > + cat >expect <<\EOF && > +Successfully rebased and updated refs/heads/update-refs. > +Updated the following refs with --update-refs: > + refs/heads/first > + refs/heads/no-conflict-branch > + refs/heads/second > + refs/heads/third > +EOF Although this works, the problem with this change (and its sibling later in the patch) is that someday someone is going to come along (say, for instance, a GSoC applicant doing a microproject) who submits a patch to (re-)"modernize" this test by using `<<-` to (re-)indent the heredoc body. A better approach would probably be to retain `<<-` and use q_to_tab(): q_to_tab >expect <<-\EOF && Qrefs/heads/first Q... Qrefs/heads/third EOF
On 02/10/2024 22:43, Eric Sunshine wrote: > On Wed, Oct 2, 2024 at 11:17 AM Patrick Steinhardt <ps@pks.im> wrote: >> Two of our tests in t3404 use indented HERE docs where leading tabs on >> some of the lines are actually relevant. The tabs do get removed though, >> and we try to fix this up by using sed(1) to replace leading tabs in the >> actual output, as well. But on macOS 10.15 this doesn't work as expected >> and we somehow keep the tabs around in the actual output. > > I presume this nebulous explanation is due to the fact that the reason > why macOS 10.15 exhibits this anomalous behavior is not yet known? I suspect that the problem is that we use "\t" which is non-standard rather than a literal tab character in the sed expression. > Although this works, the problem with this change (and its sibling > later in the patch) is that someday someone is going to come along > (say, for instance, a GSoC applicant doing a microproject) who submits > a patch to (re-)"modernize" this test by using `<<-` to (re-)indent > the heredoc body. A better approach would probably be to retain `<<-` > and use q_to_tab(): > > q_to_tab >expect <<-\EOF && > Qrefs/heads/first > Q... > Qrefs/heads/third > EOF I agree that using q_to_tab is a better approach here. These first few patches all look like useful fixes in their own right. I might be worth splitting them out into a separate series so they can progress independently of the build system changes. Best Wishes Phillip
On Thu, Oct 3, 2024 at 11:16 AM Phillip Wood <phillip.wood123@gmail.com> wrote: > On 02/10/2024 22:43, Eric Sunshine wrote: > > On Wed, Oct 2, 2024 at 11:17 AM Patrick Steinhardt <ps@pks.im> wrote: > >> Two of our tests in t3404 use indented HERE docs where leading tabs on > >> some of the lines are actually relevant. The tabs do get removed though, > >> and we try to fix this up by using sed(1) to replace leading tabs in the > >> actual output, as well. But on macOS 10.15 this doesn't work as expected > >> and we somehow keep the tabs around in the actual output. > > > > I presume this nebulous explanation is due to the fact that the reason > > why macOS 10.15 exhibits this anomalous behavior is not yet known? > > I suspect that the problem is that we use "\t" which is non-standard > rather than a literal tab character in the sed expression. Ah yes. The `sed` on macOS 10.15 would have been of an older BSD-lineage than the more modern macOS versions, so that makes sense. It wouldn't be a bad idea for the commit message to mention something along those lines. (I always use literal TAB with `sed` for this precise reason, which may explain why my eyes skipped right over the non-standard use of "\t" or I just wasn't paying close enough attention, which is equally likely.)
Eric Sunshine <sunshine@sunshineco.com> writes: >> I suspect that the problem is that we use "\t" which is non-standard >> rather than a literal tab character in the sed expression. > > Ah yes. The `sed` on macOS 10.15 would have been of an older > BSD-lineage than the more modern macOS versions, so that makes sense. > It wouldn't be a bad idea for the commit message to mention something > along those lines. > > (I always use literal TAB with `sed` for this precise reason, which > may explain why my eyes skipped right over the non-standard use of > "\t" or I just wasn't paying close enough attention, which is equally > likely.) I also learned sed with old BSD behaviour to be portable (I somehow thought it is not just "old BSD" but outside POSIX if you used "\t" and friends). Checking with $ git grep 'sed.*\\t' t/\*.sh shows that t3305 also has this problem. The ones in t3404 are from 4611884e (sequencer: notify user of --update-refs activity, 2022-07-19), while the other one is from e1c52539 (t3305: check notes fanout more carefully and robustly, 2020-02-03), both are relatively old. If people are not reporting issues, it may be an indication that sed implementations of BSD origin may have died off.
On Wed, Oct 02, 2024 at 05:43:45PM -0400, Eric Sunshine wrote: > On Wed, Oct 2, 2024 at 11:17 AM Patrick Steinhardt <ps@pks.im> wrote: > > Two of our tests in t3404 use indented HERE docs where leading tabs on > > some of the lines are actually relevant. The tabs do get removed though, > > and we try to fix this up by using sed(1) to replace leading tabs in the > > actual output, as well. But on macOS 10.15 this doesn't work as expected > > and we somehow keep the tabs around in the actual output. > > I presume this nebulous explanation is due to the fact that the reason > why macOS 10.15 exhibits this anomalous behavior is not yet known? Yeah, I didn't thoroughly investigate this one but just wanted to have it fixed. I was hitting so many platform-dependent issues left and right that at some point I started to feel a bit tired. > > Work around this issue by retaining the tabs. > > > > Signed-off-by: Patrick Steinhardt <ps@pks.im> > > --- > > diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh > > @@ -1917,18 +1917,17 @@ test_expect_success '--update-refs updates refs correctly' ' > > - cat >expect <<-\EOF && > > - Successfully rebased and updated refs/heads/update-refs. > > - Updated the following refs with --update-refs: > > - refs/heads/first > > - refs/heads/no-conflict-branch > > - refs/heads/second > > - refs/heads/third > > - EOF > > + cat >expect <<\EOF && > > +Successfully rebased and updated refs/heads/update-refs. > > +Updated the following refs with --update-refs: > > + refs/heads/first > > + refs/heads/no-conflict-branch > > + refs/heads/second > > + refs/heads/third > > +EOF > > Although this works, the problem with this change (and its sibling > later in the patch) is that someday someone is going to come along > (say, for instance, a GSoC applicant doing a microproject) who submits > a patch to (re-)"modernize" this test by using `<<-` to (re-)indent > the heredoc body. A better approach would probably be to retain `<<-` > and use q_to_tab(): > > q_to_tab >expect <<-\EOF && > Qrefs/heads/first > Q... > Qrefs/heads/third > EOF Cute! Didn't know we even had this helper. Patrick
On Thu, Oct 03, 2024 at 04:15:50PM +0100, Phillip Wood wrote: > On 02/10/2024 22:43, Eric Sunshine wrote: > > On Wed, Oct 2, 2024 at 11:17 AM Patrick Steinhardt <ps@pks.im> wrote: > > > Two of our tests in t3404 use indented HERE docs where leading tabs on > > > some of the lines are actually relevant. The tabs do get removed though, > > > and we try to fix this up by using sed(1) to replace leading tabs in the > > > actual output, as well. But on macOS 10.15 this doesn't work as expected > > > and we somehow keep the tabs around in the actual output. > > > > I presume this nebulous explanation is due to the fact that the reason > > why macOS 10.15 exhibits this anomalous behavior is not yet known? > > I suspect that the problem is that we use "\t" which is non-standard rather > than a literal tab character in the sed expression. Ah, that makes sense. > > Although this works, the problem with this change (and its sibling > > later in the patch) is that someday someone is going to come along > > (say, for instance, a GSoC applicant doing a microproject) who submits > > a patch to (re-)"modernize" this test by using `<<-` to (re-)indent > > the heredoc body. A better approach would probably be to retain `<<-` > > and use q_to_tab(): > > > > q_to_tab >expect <<-\EOF && > > Qrefs/heads/first > > Q... > > Qrefs/heads/third > > EOF > > I agree that using q_to_tab is a better approach here. > > These first few patches all look like useful fixes in their own right. I > might be worth splitting them out into a separate series so they can > progress independently of the build system changes. Yeah, I'm certainly happy to split these out into a separate series. For now I'll keep things in one, but will do so depending on how this series progresses. Patrick
On Thu, Oct 03, 2024 at 04:19:07PM -0700, Junio C Hamano wrote: > Eric Sunshine <sunshine@sunshineco.com> writes: > > >> I suspect that the problem is that we use "\t" which is non-standard > >> rather than a literal tab character in the sed expression. > > > > Ah yes. The `sed` on macOS 10.15 would have been of an older > > BSD-lineage than the more modern macOS versions, so that makes sense. > > It wouldn't be a bad idea for the commit message to mention something > > along those lines. > > > > (I always use literal TAB with `sed` for this precise reason, which > > may explain why my eyes skipped right over the non-standard use of > > "\t" or I just wasn't paying close enough attention, which is equally > > likely.) > > I also learned sed with old BSD behaviour to be portable (I somehow > thought it is not just "old BSD" but outside POSIX if you used "\t" > and friends). Checking with > > $ git grep 'sed.*\\t' t/\*.sh > > shows that t3305 also has this problem. The ones in t3404 are from > 4611884e (sequencer: notify user of --update-refs activity, > 2022-07-19), while the other one is from e1c52539 (t3305: check > notes fanout more carefully and robustly, 2020-02-03), both are > relatively old. If people are not reporting issues, it may be an > indication that sed implementations of BSD origin may have died off. Probably. Curious that I didn't see issues in t3305. Patrick
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh index f171af3061..da4f3e6caf 100755 --- a/t/t3404-rebase-interactive.sh +++ b/t/t3404-rebase-interactive.sh @@ -1917,18 +1917,17 @@ test_expect_success '--update-refs updates refs correctly' ' test_cmp_rev HEAD~1 refs/heads/third && test_cmp_rev HEAD refs/heads/no-conflict-branch && - cat >expect <<-\EOF && - Successfully rebased and updated refs/heads/update-refs. - Updated the following refs with --update-refs: - refs/heads/first - refs/heads/no-conflict-branch - refs/heads/second - refs/heads/third - EOF + cat >expect <<\EOF && +Successfully rebased and updated refs/heads/update-refs. +Updated the following refs with --update-refs: + refs/heads/first + refs/heads/no-conflict-branch + refs/heads/second + refs/heads/third +EOF # Clear "Rebasing (X/Y)" progress lines and drop leading tabs. - sed -e "s/Rebasing.*Successfully/Successfully/g" -e "s/^\t//g" \ - <err >err.trimmed && + sed "s/Rebasing.*Successfully/Successfully/g" <err >err.trimmed && test_cmp expect err.trimmed ' @@ -2178,19 +2177,18 @@ test_expect_success '--update-refs: check failed ref update' ' test_must_fail git rebase --continue 2>err && grep "update_ref failed for ref '\''refs/heads/second'\''" err && - cat >expect <<-\EOF && - Updated the following refs with --update-refs: - refs/heads/first - refs/heads/no-conflict-branch - refs/heads/third - Failed to update the following refs with --update-refs: - refs/heads/second - EOF + cat >expect <<\EOF && +Updated the following refs with --update-refs: + refs/heads/first + refs/heads/no-conflict-branch + refs/heads/third +Failed to update the following refs with --update-refs: + refs/heads/second +EOF # Clear "Rebasing (X/Y)" progress lines and drop leading tabs. tail -n 6 err >err.last && - sed -e "s/Rebasing.*Successfully/Successfully/g" -e "s/^\t//g" \ - <err.last >err.trimmed && + sed "s/Rebasing.*Successfully/Successfully/g" <err.last >err.trimmed && test_cmp expect err.trimmed '
Two of our tests in t3404 use indented HERE docs where leading tabs on some of the lines are actually relevant. The tabs do get removed though, and we try to fix this up by using sed(1) to replace leading tabs in the actual output, as well. But on macOS 10.15 this doesn't work as expected and we somehow keep the tabs around in the actual output. Work around this issue by retaining the tabs. Signed-off-by: Patrick Steinhardt <ps@pks.im> --- t/t3404-rebase-interactive.sh | 38 +++++++++++++++++------------------ 1 file changed, 18 insertions(+), 20 deletions(-)