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

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

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

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

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