Re: [PATCH v9 2/5] worktree: refactor find_linked_symref function

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 2, 2015, 21:16 UTC
Message-ID: <xmqq612p3rab.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1443786935-81131-3-git-send-email-rappazzo@gmail.com>

Michael Rappazzo <rappazzo@gmail.com> writes:

Show 9 quoted lines

> +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))
> +			return -1;
> +	else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {

Which "if" does this "else if" pair with? I think you would want to clarify not with indentation but with a brace here.

	if (!strbuf_readlink(...)) {
		/* HEAD is a symlink */
                if (!starts_with(...) || check_refname_format(...))
			return -1; /* malformed */
	} else if (strbuf_read_file(...) >= 0) {
        	/* textual symref */
                ...

Alternatively, you could do it this way.

	if (!strbuf_readlink(...) &&
	    (!starts_with(...) || check_refname_format(...)))
		return -1;
	else if (strbuf_read_file(...) >= 0) {
		...

I do not know which one is more readable, though. Probably former, even though it is a bit more verbose, is easier to follow.

> +		if (!starts_with(ref->buf, "ref:"))
> +			if (is_detached)
> +				*is_detached = 1;
> +		else {

Same here.

 worktree.c | 13 ++++++++-----
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/worktree.c b/worktree.c
index c049947..f16c082 100644
--- a/worktree.c
+++ b/worktree.c
@@ -17,22 +17,25 @@ 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 (!strbuf_readlink(ref, path_to_ref, 0)) {
+		/* HEAD is symbolic link */
 		if (!starts_with(ref->buf, "refs/") ||
 				check_refname_format(ref->buf, 0))
 			return -1;
-	else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
-		if (!starts_with(ref->buf, "ref:"))
+	} else if (strbuf_read_file(ref, path_to_ref, 0) >= 0) {
+		/* textual symref or detached */
+		if (!starts_with(ref->buf, "ref:")) {
 			if (is_detached)
 				*is_detached = 1;
-		else {
+		} else {
 			strbuf_remove(ref, 0, strlen("ref:"));
 			strbuf_trim(ref);
 			if (check_refname_format(ref->buf, 0))
 				return -1;
 		}
-	} else
+	} else {
 		return -1;
+	}
 	return 0;
 }