Re: [PATCH 1/2] completion: refactor existence checks for special refs

[Junio C Hamano <gitster@pobox.com>]

Stan Hu <stanhu@gmail.com> writes:

> In preparation for the reftable backend, this commit introduces a
> '__git_ref_exists' function that continues to use 'test -f' to
> determine whether a given ref exists in the local filesystem.

I do not think git_ref_exists is a good name for what this one does.
The name adds undue cognitive load on readers.  As far as I can
tell, with this helper function, you are interested in handling only
pseudorefs like $GIT_DIR/FOOBAR_HEAD (oh, retitle the patch to call
them "pseudorefs", not "special refs", by the way), and that is why
you can get away with a simple

    [ -f "$__git_repo_path/$ref" ]

without bothering to check the packed-refs file.  The checks this
patch replace to calls to this helper functions in the original make
it clear, simply because they spell out what they are checking, like
"CHERRY_PICK_HEAD", why a mere filesystem check was sufficient, but
once you give an overly generic name like "ref-exists", it becomes
tempting to (ab|mis)use it to check for branches and tags, which is
not your intention at all, and the implementation does not work well
for that purpose.

> Each caller of '__git_ref_exists' must call '__git_find_repo_path`
> first. '_git_restore' already used 'git rev-parse HEAD', but to use
> '__git_ref_exists' insert a '__git_find_repo_path' at the start.

To whom do you think the above piece of information is essential for
them to work?  Whoever updates the completion script, finds existing
use of __git_ref_exists, and thinks the helper would be useful for
their own use.  To them, the above needs be in the in-code comment
to make it discoverable.  It is OK to have it also in the proposed
log message, but it is not as essential, especially if you have it
in-code anyway.

Another thing you would need to make sure that the potential users
of this helpers understand is of course this is meant to be used
only on pseudorefs.  You can of course do so with an additional
in-code comment, but giving a more appropriate name to the helper
would be the easiest and simplest, e.g. __git_pseudoref_exists or
something.

> Reviewed-by: Patrick Steinhardt <ps@pks.im>
> Reviewed-by: Christian Couder <christian.couder@gmail.com>
> Signed-off-by: Stan Hu <stanhu@gmail.com>
> ---
>  contrib/completion/git-completion.bash | 20 +++++++++++++++-----
>  1 file changed, 15 insertions(+), 5 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 13a39ebd2e..9fbdc13f9a 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -122,6 +122,15 @@ __git ()
>  		${__git_dir:+--git-dir="$__git_dir"} "$@" 2>/dev/null
>  }
>  
> +# Runs git in $__git_repo_path to determine whether a ref exists.
> +# 1: The ref to search
> +__git_ref_exists ()
> +{
> +	local ref=$1
> +
> +	[ -f "$__git_repo_path/$ref" ]
> +}