Re: [PATCH 3/5] checkout --to: no auto-detach if the ref is already checked out

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 23, 2014, 21:16 UTC
Message-ID: <xmqqzjfzdbds.fsf@gitster.dls.corp.google.com>
In-Reply-To: <1406115795-24082-4-git-send-email-pclouds@gmail.com>

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

Show 35 quoted lines

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index c83f476..d35245a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1006,31 +1006,52 @@ static const char *unique_tracking_name(const char *name, unsigned char *sha1)
>  	return NULL;
>  }
>  
> -static int check_linked_checkout(struct branch_info *new,
> -				  const char *name, const char *path)
> +static void check_linked_checkout(struct branch_info *new, const char *id)
>  {
>  	struct strbuf sb = STRBUF_INIT;
> +	struct strbuf path = STRBUF_INIT;
> +	struct strbuf gitdir = STRBUF_INIT;
>  	const char *start, *end;
> -	if (strbuf_read_file(&sb, path, 0) < 0 ||
> -	    !skip_prefix(sb.buf, "ref:", &start)) {
> -		strbuf_release(&sb);
> -		return 0;
> -	}
>  
> +	if (id)
> +		strbuf_addf(&path, "%s/repos/%s/HEAD", get_git_common_dir(), id);
> +	else
> +		strbuf_addf(&path, "%s/HEAD", get_git_common_dir());
> +
> +	if (strbuf_read_file(&sb, path.buf, 0) <= 0 ||
> +	    !skip_prefix(sb.buf, "ref:", &start))
> +		goto done;
>  	while (isspace(*start))
>  		start++;
>  	end = start;
>  	while (*end && !isspace(*end))
>  		end++;

Not new in this round of update, and may not even be an issue, but:

 - Earlier, the code returned early on a negative return value from
   read-file (i.e., an error), but this round it also does so for
   zero.  Intended?

 - The above duplicates the logic in resolve_ref_unsafe() and
   resolve_gitlink_ref_recursive(); three places now knows what a
   textual symref looks like (i.e. begins with "ref:", zero or more
   whitespaces, the target ref and then zero or more trailing
   whitespaces).  Perhaps we need to consolidate the code further,
   so that this knowledge does not leak out of refs.c?

Show 12 quoted lines

> +	if (strncmp(start, new->path, end - start) ||
> +	    new->path[end - start] != '\0')
> +		goto done;
> +	if (id) {
> +		strbuf_reset(&path);
> +		strbuf_addf(&path, "%s/repos/%s/gitdir",
> +			    get_git_common_dir(), id);
> +		if (strbuf_read_file(&gitdir, path.buf, 0) <= 0)
> +			goto done;
> +		while (gitdir.len && (gitdir.buf[gitdir.len - 1] == '\n' ||
> +				      gitdir.buf[gitdir.len - 1] == '\r'))
> +			gitdir.buf[--gitdir.len] = '\0';

Accepting arbitrary numbers of '\r' and '\n' sounds as if the code is allowing it, but text editors would not end their files with a nonsense sequence like "\r\r\n\r" unless the end-user works to do so, and if you are prepared to be lenient to noisy human input, not trimming trailing whitespaces does not look it is going far enough to help them.

I do not see a good reason to allow random text editors to edit this file in the first place, so my preference is:

	if (strbuf_read_file(...) < 0 ||
	    gitdir.len == 0 ||
            gitdir.buf[gitdir.len - 1] != '\n')
            goto error_return;
	gitdir.buf[--gitdir.len] = '\0';

Alternatively, if you are trying to be lenient to human input, I would understand:

	if (strbuf_read_file(...) < 0)
        	goto error_return;
	strbuf_rtrim(&gitdir);

The code in the patch, which is something in between, does not make much sense to me.