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

From: Junio C Hamano <gitster@pobox.com>
Date: Sep 22, 2015, 17:44 UTC
Message-ID: <xmqqmvwe4adl.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1442583027-47653-3-git-send-email-rappazzo@gmail.com>

Michael Rappazzo <rappazzo@gmail.com> writes:

Show 19 quoted lines

> 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).

Show 14 quoted lines

> + *
> + * $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.

Show 8 quoted lines

> +	} 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?

Show 13 quoted lines

> +	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;

Show 22 quoted lines

> +	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.

Show 5 quoted lines

>  	/*
>  	 * $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?

Show 17 quoted lines

> -	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.