Re: [PATCH] completion: treat unset GIT_COMPLETION_SHOW_ALL gracefully

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

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.