git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
* [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully
@ 2021-04-06 18:12 Ville Skyttä
  2021-04-06 22:16 ` Junio C Hamano
  0 siblings, 1 reply; 3+ messages in thread
From: Ville Skyttä @ 2021-04-06 18:12 UTC (permalink / raw)
  To: git

If not set, referencing it in nounset (set -u) mode unguarded produces
an error.

Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e1a66954fe..6d77f56f92 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -427,7 +427,7 @@ __gitcomp_builtin ()
 
 	if [ -z "$options" ]; then
 		local completion_helper
-		if [ "$GIT_COMPLETION_SHOW_ALL" = "1" ]; then
+		if [ "${GIT_COMPLETION_SHOW_ALL-}" = "1" ]; then
 			completion_helper="--git-completion-helper-all"
 		else
 			completion_helper="--git-completion-helper"
-- 
2.25.1


^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully
  2021-04-06 18:12 [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully Ville Skyttä
@ 2021-04-06 22:16 ` Junio C Hamano
  2021-04-08  6:52   ` Ville Skyttä
  0 siblings, 1 reply; 3+ messages in thread
From: Junio C Hamano @ 2021-04-06 22:16 UTC (permalink / raw)
  To: Ville Skyttä; +Cc: git

Ville Skyttä <ville.skytta@iki.fi> writes:

> If not set, referencing it in nounset (set -u) mode unguarded produces
> an error.
>
> Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
> ---
>  contrib/completion/git-completion.bash | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)

Thanks.

$ git grep -h -o -e '\$GIT_[A-Z_]*' master -- contrib/completion/git-completion.bash

gives a few other hits.

$GIT_DIR
$GIT_COMPLETION_SHOW_ALL
$GIT_TESTING_ALL_COMMAND_LIST
$GIT_TESTING_ALL_COMMAND_LIST
$GIT_TESTING_PORCELAIN_COMMAND_LIST

Have you gone through all of these hits?

I just checked that the reference to $GIT_DIR is safe.

        elif [ -n "${GIT_DIR-}" ]; then
                test -d "${GIT_DIR-}" &&
                __git_repo_path="$GIT_DIR"

In fact, after checking "is this non-empty?", the form used to see
"is this a directory" does not even need to be "${VAR-}".

Among the two references to GIT_TESTING_ALL_COMMAND_LIST, the first
one does not look safe to me, and that is what made me take a look
myself.  It probably wants to follow the same pattern as above,
doesn't it?  Or am I reading the code incorrectly and the use there
is safe?

Reference to $GIT_TESTING_PORCELAIN_COMMAND_LIST is safe, as it
follows the same "if test -n "${VAR-}"; then use "$VAR"; fi"
pattern.

So, this patch definitely looks like an improvement, but if there
are so few remaining issues, I'd prefer to see that (1) the proposed
log message explain that the patch author audited all usages of
variables and updated all "-u"-unsafe ones, and (2) the patch
actually does update all remaining problematic ones.

If I am wrong about TESTING_ALL_COMMAND_LIST, then (2) may already
be true, but then we want to describe that fact in the log message
even more.  It would ensure that future developers understand that
${VAR-} constructs are no accident and we are striving to make the
script "-u"-safe.

Thanks.

> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index e1a66954fe..6d77f56f92 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -427,7 +427,7 @@ __gitcomp_builtin ()
>  
>  	if [ -z "$options" ]; then
>  		local completion_helper
> -		if [ "$GIT_COMPLETION_SHOW_ALL" = "1" ]; then
> +		if [ "${GIT_COMPLETION_SHOW_ALL-}" = "1" ]; then
>  			completion_helper="--git-completion-helper-all"
>  		else
>  			completion_helper="--git-completion-helper"

^ permalink raw reply	[flat|nested] 3+ messages in thread

* Re: [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully
  2021-04-06 22:16 ` Junio C Hamano
@ 2021-04-08  6:52   ` Ville Skyttä
  0 siblings, 0 replies; 3+ messages in thread
From: Ville Skyttä @ 2021-04-08  6:52 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On Wed, 7 Apr 2021 at 01:16, Junio C Hamano <gitster@pobox.com> wrote:
>
> Ville Skyttä <ville.skytta@iki.fi> writes:
>
> > If not set, referencing it in nounset (set -u) mode unguarded produces
> > an error.
> >
> > Signed-off-by: Ville Skyttä <ville.skytta@iki.fi>
> > ---
> >  contrib/completion/git-completion.bash | 2 +-
> >  1 file changed, 1 insertion(+), 1 deletion(-)
>
> Thanks.
>
> $ git grep -h -o -e '\$GIT_[A-Z_]*' master -- contrib/completion/git-completion.bash
>
> gives a few other hits.

Thanks for checking. If we want to do what was proposed below:

> I'd prefer to see that (1) the proposed
> log message explain that the patch author audited all usages of
> variables

...we need to go through not only ones starting with GIT_, but as is
written above, _all_ variables, which is a larger task.

To be clear, I haven't checked anything besides what was the subject
of and change in the patch. I can go through all GIT_* while at it,
but I'm not promising going through all variables at this point.
Hopefully that's enough to get the resulting changes merged.

Would be great if there were some automated tests to catch these
issues, as they tend to crop up over time.

^ permalink raw reply	[flat|nested] 3+ messages in thread

end of thread, other threads:[~2021-04-08  6:53 UTC | newest]

Thread overview: 3+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-04-06 18:12 [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully Ville Skyttä
2021-04-06 22:16 ` Junio C Hamano
2021-04-08  6:52   ` Ville Skyttä

git@vger.kernel.org list mirror (unofficial, one of many)

This inbox may be cloned and mirrored by anyone:

	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

	# If you have public-inbox 1.1+ installed, you may
	# initialize and index your mirror using the following commands:
	public-inbox-init -V1 git git/ https://public-inbox.org/git \
		git@vger.kernel.org
	public-inbox-index git

Example config snippet for mirrors.
Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://7fh6tueqddpjyxjmgtdiueylzoqt6pt7hec3pukyptlmohoowvhde4yd.onion/inbox.comp.version-control.git
	nntp://ie5yzdi7fg72h7s4sdcztq5evakq23rdt33mfyfcddc5u3ndnw24ogqd.onion/inbox.comp.version-control.git
	nntp://4uok3hntl7oi7b4uf4rtfwefqeexfzil2w6kgk2jn5z2f764irre7byd.onion/inbox.comp.version-control.git
	nntp://news.gmane.io/gmane.comp.version-control.git
 note: .onion URLs require Tor: https://www.torproject.org/

code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

AGPL code for this site: git clone https://public-inbox.org/public-inbox.git