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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 3, 2023, 13:15 UTC
Message-ID: <xmqqedg34ecc.fsf@gitster.g>
In-Reply-To: <20231130202404.89791-2-stanhu@gmail.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.

Show 23 quoted lines

> 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" ]
> +}