Re: [PATCH v8 2/4] worktree: refactor find_linked_symref function

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

Michael Rappazzo <rappazzo@gmail.com> writes:

> Refactoring will help transition this code to provide additional useful
> worktree functions.
>
> Signed-off-by: Michael Rappazzo <rappazzo@gmail.com>
> ---
>  worktree.c | 86 ++++++++++++++++++++++++++++++++++++++++++++++----------------
>  1 file changed, 64 insertions(+), 22 deletions(-)
>
> diff --git a/worktree.c b/worktree.c
> index 10e1496..5c75875 100644
> --- a/worktree.c
> +++ b/worktree.c
> @@ -3,6 +3,60 @@
>  #include "strbuf.h"
>  #include "worktree.h"
>  
> +/*
> + * read 'path_to_ref' into 'ref'.  Also if is_detached is not NULL,
> + * set is_detached to 1 if the ref is detatched.

set is_detached to 1 (0) if the ref is detatched (is not detached).

> + *
> + * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside $GIT_DIR so
> + * for linked worktrees, `resolve_ref_unsafe()` won't work (it uses
> + * git_path). Parse the ref ourselves.
> + *
> + * return -1 if the ref is not a proper ref, 0 otherwise (success)
> + */
> +static int parse_ref(char *path_to_ref, struct strbuf *ref, int *is_detached)
> +{
> +	if (is_detached)
> +		*is_detached = 0;
> +	if (!strbuf_readlink(ref, path_to_ref, 0)) {
> +		if (!starts_with(ref->buf, "refs/")
> +				|| check_refname_format(ref->buf, 0))

Don't try to be creative with the format and stick to the original.

	if (!starts_with(ref->buf, "refs/") ||
	    check_refname_format(ref->buf, 0))

> +			return -1;
> +

This blank makes a strange code by making the "return -1" have no
blank above and one blank below.

> +	} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
> +		if (starts_with(ref->buf, "ref:")) {
> +			strbuf_remove(ref, 0, strlen("ref:"));
> +			strbuf_trim(ref);
> +			if (check_refname_format(ref->buf, 0))
> +				return -1;
> +		} else if (is_detached)
> +			*is_detached = 1;

Minor: I have a suspicion that this would be easier to follow:

		if (!starts_with(...)) {
			if (is_detached)
				*is_detached = 1;
		} else {
                	strbuf_remove(...);
                        strbuf_trim(...);
                        if (check_refname_format(...))
				return -1;
		}

> +	}

What should happen when strbuf_read_file() above fails?  Would it be
a bug (i.e. the caller shouldn't have called us in the first place
with such a broken ref), would it be a repository inconsistency
(i.e. it is worth warning and stop the caller from doing further
damage), or is it OK to silently succeed?

> +	return 0;
> +}
> +
> +static char *find_main_symref(const char *symref, const char *branch)
> +{
> +	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf gitdir = STRBUF_INIT;
> +	char *existing = NULL;
> +
> +	strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
> +	if (parse_ref(path.buf, &sb, NULL) == -1)
> +		goto done;

I know you described it to "return -1 on an error", but as a general
style, for a function that signals a success by returning 0 and
negative on error (or on various kinds of errors), it is easier to
follow if you followed a more common pattern:

	if (parse_ref(...) < 0)
        	goto done;

> +	if (strcmp(sb.buf, branch))
> +		goto done;
> +	strbuf_addstr(&gitdir, get_git_common_dir());
> +	strbuf_strip_suffix(&gitdir, ".git");
> +	existing = strbuf_detach(&gitdir, NULL);
> +done:
> +	strbuf_release(&path);
> +	strbuf_release(&sb);
> +	strbuf_release(&gitdir);
> +
> +	return existing;
> +}
> +
>  static char *find_linked_symref(const char *symref, const char *branch,
>  				const char *id)
>  {
> @@ -11,36 +65,24 @@ static char *find_linked_symref(const char *symref, const char *branch,
>  	struct strbuf gitdir = STRBUF_INIT;
>  	char *existing = NULL;
>  
> +	if (!id)
> +		goto done;

A caller that calls this function with id==NULL would always get a
no-op.  Is that what the caller intended, or should it have called
the new find_main_symref() instead?  I'd imagine it is the latter,
in which case

	if (!id)
        	die("BUG");

is more appropriate.  On the other hand, if you are trying to keep
the old interface to allow id==NULL to mean "get the information for
the primary one", I'd expect this to call find_main_symref() for the
(old-style) callers.

In either case, this "no id? no-op" looks funny.

>  	/*
>  	 * $GIT_COMMON_DIR/$symref (e.g. HEAD) is practically outside
>  	 * $GIT_DIR so resolve_ref_unsafe() won't work (it uses
>  	 * git_path). Parse the ref ourselves.
>  	 */

You moved this comment to parse_ref(), which is a more appropriate
home for it.  Perhaps you want to remove this copy from here?

> -	if (id)
> -		strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
> -	else
> -		strbuf_addf(&path, "%s/%s", get_git_common_dir(), symref);
> +	strbuf_addf(&path, "%s/worktrees/%s/%s", get_git_common_dir(), id, symref);
>  
> -	if (!strbuf_readlink(&sb, path.buf, 0)) {
> -		if (!starts_with(sb.buf, "refs/") ||
> -		    check_refname_format(sb.buf, 0))
> -			goto done;
> -	} else if (strbuf_read_file(&sb, path.buf, 0) >= 0 &&
> -	    starts_with(sb.buf, "ref:")) {
> -		strbuf_remove(&sb, 0, strlen("ref:"));
> -		strbuf_trim(&sb);
> -	} else
> +	if (parse_ref(path.buf, &sb, NULL) == -1)
>  		goto done;

Same comment as in find_main_symref() applies here.

I can see the value of splitting out parse_ref() into a separate
function, but it is not immediately obvious how it would be an
overall win to duplicate major parts of the logic that used to be
shared for "primary" and "linked" cases in a single function into
two separate, simpler and similar functions at this point in the
series (note that I did not say "it clearly made it worse").
Perhaps reviews on the callers will help us answer that.

Thanks.