Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

t5563: prevent "ambiguous redirect" #1507

Conversation

dscho
Copy link
Member

@dscho dscho commented Mar 30, 2023

I ran into this issue while running the tests with TEST_SHELL_PATH=/bin/bash.

Cc: Matthew John Cheetham mjcheetham@outlook.com

When I ran this test using `TEST_SHELL_PATH=/bin/bash` in my Ubuntu
setup (where Bash is at version 5.0.17(1)-release), I was greeted with
this error message:

	./test-lib.sh: line 1072: $CHALLENGE: ambiguous redirect

This commit fixes that error by quoting the `CHALLENGE` variable (which
has as value a path containing spaces), and by avoiding to cuddle the
empty string parameter in the `printf` call with the redirect character
(in fact, the `printf ''>$CHALLENGE` is removed because the next line
overwrites the file anyway because it _also_ uses a single `>` to
redirect the output).

Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
@dscho
Copy link
Member Author

dscho commented Mar 31, 2023

/submit

@dscho dscho self-assigned this Mar 31, 2023
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 31, 2023

Submitted as pull.1507.git.1680245525637.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1507/dscho/fix-ambiguous-redirect-in-t5563-v1

To fetch this version to local tag pr-1507/dscho/fix-ambiguous-redirect-in-t5563-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1507/dscho/fix-ambiguous-redirect-in-t5563-v1

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 31, 2023

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> (in fact, the `printf ''>$CHALLENGE` is removed because the next line
> overwrites the file anyway because it _also_ uses a single `>` to
> redirect the output).

Good eyes. I also wondered what that empty printf was doing.  While
I suspect the original intention was to start from an empty file and
keep appending contents with any meaning so that the redirection on
subsequent lines would look identical, I do not think it is
necessary in this case, primarily because it is unlikely that any
future change will swap the first line with any subsequent lines.

Thanks for spotting.  I was hoping that we could soon retire the
"quote the redirection target if it has parameter substitution to
help older bash" rule in our coding guidelines, but this example
shows that the world is not quite ready yet.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 31, 2023

This branch is now known as js/t5563-portability-fix.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 31, 2023

This patch series was integrated into seen via git@4dd6f52.

@gitgitgadget gitgitgadget bot added the seen label Mar 31, 2023
@gitgitgadget
Copy link

gitgitgadget bot commented Mar 31, 2023

This patch series was integrated into seen via git@6f33c2e.

@gitgitgadget
Copy link

gitgitgadget bot commented Mar 31, 2023

This patch series was integrated into next via git@12f95a9.

@gitgitgadget gitgitgadget bot added the next label Mar 31, 2023
@gitgitgadget
Copy link

gitgitgadget bot commented Apr 1, 2023

This patch series was integrated into seen via git@dd88a1a.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 1, 2023

This patch series was integrated into master via git@dd88a1a.

@gitgitgadget
Copy link

gitgitgadget bot commented Apr 1, 2023

This patch series was integrated into next via git@dd88a1a.

@gitgitgadget gitgitgadget bot added the master label Apr 1, 2023
@gitgitgadget gitgitgadget bot closed this Apr 1, 2023
@gitgitgadget
Copy link

gitgitgadget bot commented Apr 1, 2023

Closed via dd88a1a.

@dscho dscho deleted the fix-ambiguous-redirect-in-t5563 branch April 6, 2023 10:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

1 participant