{"thread":{"id":"55442","subject":"[PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully","startedAt":"2021-04-06T18:13:04Z","lastAt":"2021-04-08T06:53:04Z","messageCount":3,"participants":["Ville Skyttä","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"421080","messageId":"20210406181247.250046-1-ville.skytta@iki.fi","threadId":"55442","inReplyTo":null,"subject":"[PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully","fromName":"Ville Skyttä","fromEmail":"ville.skytta@iki.fi","sentAt":"2021-04-06T18:12:47Z","receivedAt":"2021-04-06T18:13:04Z","isPatch":true,"sender":{"key":"ville.skytta@iki.fi","avatar":"https://avatars.githubusercontent.com/u/109152?v=4"},"body":"If not set, referencing it in nounset (set -u) mode unguarded produces\nan error.\n\nSigned-off-by: Ville Skyttä <ville.skytta@iki.fi>\n---\n contrib/completion/git-completion.bash | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\nindex e1a66954fe..6d77f56f92 100644\n--- a/contrib/completion/git-completion.bash\n+++ b/contrib/completion/git-completion.bash\n@@ -427,7 +427,7 @@ __gitcomp_builtin ()\n \n \tif [ -z \"$options\" ]; then\n \t\tlocal completion_helper\n-\t\tif [ \"$GIT_COMPLETION_SHOW_ALL\" = \"1\" ]; then\n+\t\tif [ \"${GIT_COMPLETION_SHOW_ALL-}\" = \"1\" ]; then\n \t\t\tcompletion_helper=\"--git-completion-helper-all\"\n \t\telse\n \t\t\tcompletion_helper=\"--git-completion-helper\"\n-- \n2.25.1\n\n"},{"id":"421098","messageId":"xmqqo8er12kq.fsf@gitster.g","threadId":"55442","inReplyTo":"20210406181247.250046-1-ville.skytta@iki.fi","subject":"Re: [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-04-06T22:16:21Z","receivedAt":"2021-04-06T22:16:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ville Skyttä <ville.skytta@iki.fi> writes:\n\n> If not set, referencing it in nounset (set -u) mode unguarded produces\n> an error.\n>\n> Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>\n> ---\n>  contrib/completion/git-completion.bash | 2 +-\n>  1 file changed, 1 insertion(+), 1 deletion(-)\n\nThanks.\n\n$ git grep -h -o -e '\\$GIT_[A-Z_]*' master -- contrib/completion/git-completion.bash\n\ngives a few other hits.\n\n$GIT_DIR\n$GIT_COMPLETION_SHOW_ALL\n$GIT_TESTING_ALL_COMMAND_LIST\n$GIT_TESTING_ALL_COMMAND_LIST\n$GIT_TESTING_PORCELAIN_COMMAND_LIST\n\nHave you gone through all of these hits?\n\nI just checked that the reference to $GIT_DIR is safe.\n\n        elif [ -n \"${GIT_DIR-}\" ]; then\n                test -d \"${GIT_DIR-}\" &&\n                __git_repo_path=\"$GIT_DIR\"\n\nIn fact, after checking \"is this non-empty?\", the form used to see\n\"is this a directory\" does not even need to be \"${VAR-}\".\n\nAmong the two references to GIT_TESTING_ALL_COMMAND_LIST, the first\none does not look safe to me, and that is what made me take a look\nmyself.  It probably wants to follow the same pattern as above,\ndoesn't it?  Or am I reading the code incorrectly and the use there\nis safe?\n\nReference to $GIT_TESTING_PORCELAIN_COMMAND_LIST is safe, as it\nfollows the same \"if test -n \"${VAR-}\"; then use \"$VAR\"; fi\"\npattern.\n\nSo, this patch definitely looks like an improvement, but if there\nare so few remaining issues, I'd prefer to see that (1) the proposed\nlog message explain that the patch author audited all usages of\nvariables and updated all \"-u\"-unsafe ones, and (2) the patch\nactually does update all remaining problematic ones.\n\nIf I am wrong about TESTING_ALL_COMMAND_LIST, then (2) may already\nbe true, but then we want to describe that fact in the log message\neven more.  It would ensure that future developers understand that\n${VAR-} constructs are no accident and we are striving to make the\nscript \"-u\"-safe.\n\nThanks.\n\n> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash\n> index e1a66954fe..6d77f56f92 100644\n> --- a/contrib/completion/git-completion.bash\n> +++ b/contrib/completion/git-completion.bash\n> @@ -427,7 +427,7 @@ __gitcomp_builtin ()\n>  \n>  \tif [ -z \"$options\" ]; then\n>  \t\tlocal completion_helper\n> -\t\tif [ \"$GIT_COMPLETION_SHOW_ALL\" = \"1\" ]; then\n> +\t\tif [ \"${GIT_COMPLETION_SHOW_ALL-}\" = \"1\" ]; then\n>  \t\t\tcompletion_helper=\"--git-completion-helper-all\"\n>  \t\telse\n>  \t\t\tcompletion_helper=\"--git-completion-helper\"\n"},{"id":"421195","messageId":"CABr9L5C_+bdiv=hhbx4h1cXcOcW-9su45kNMo0i0i0zBO0j8QA@mail.gmail.com","threadId":"55442","inReplyTo":"xmqqo8er12kq.fsf@gitster.g","subject":"Re: [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully","fromName":"Ville Skyttä","fromEmail":"ville.skytta@iki.fi","sentAt":"2021-04-08T06:52:44Z","receivedAt":"2021-04-08T06:53:04Z","isPatch":true,"sender":{"key":"ville.skytta@iki.fi","avatar":"https://avatars.githubusercontent.com/u/109152?v=4"},"body":"On Wed, 7 Apr 2021 at 01:16, Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Ville Skyttä <ville.skytta@iki.fi> writes:\n>\n> > If not set, referencing it in nounset (set -u) mode unguarded produces\n> > an error.\n> >\n> > Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>\n> > ---\n> >  contrib/completion/git-completion.bash | 2 +-\n> >  1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> Thanks.\n>\n> $ git grep -h -o -e '\\$GIT_[A-Z_]*' master -- contrib/completion/git-completion.bash\n>\n> gives a few other hits.\n\nThanks for checking. If we want to do what was proposed below:\n\n> I'd prefer to see that (1) the proposed\n> log message explain that the patch author audited all usages of\n> variables\n\n...we need to go through not only ones starting with GIT_, but as is\nwritten above, _all_ variables, which is a larger task.\n\nTo be clear, I haven't checked anything besides what was the subject\nof and change in the patch. I can go through all GIT_* while at it,\nbut I'm not promising going through all variables at this point.\nHopefully that's enough to get the resulting changes merged.\n\nWould be great if there were some automated tests to catch these\nissues, as they tend to crop up over time.\n"}]}