Re: [PATCH v2 5/5] format-patch: pick up branch description when no ref is specified

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 3, 2013, 15:49 UTC
Message-ID: <7v7gnuxo4q.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1357221791-7496-6-git-send-email-pclouds@gmail.com>

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

Show 38 quoted lines

> We only try to get branch name in "format-patch origin" case or
> similar and not "format-patch -22" where HEAD is automatically
> added. Without correct branch name, branch description cannot be
> added. Make sure we always get branch name.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  builtin/log.c           | 16 +++++++++++++---
>  t/t4014-format-patch.sh |  7 +++++++
>  2 files changed, 20 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/log.c b/builtin/log.c
> index 039bf67..81683f6 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1027,12 +1027,22 @@ static char *find_branch_name(struct rev_info *rev)
>  		else
>  			return NULL;
>  	}
> -	if (positive < 0)
> +	if (positive >= 0)
> +		ref = rev->cmdline.rev[positive].name;
> +	else if (!rev->cmdline.nr && rev->pending.nr == 1 &&
> +		 !strcmp(rev->pending.objects[0].name, "HEAD"))
> +		/*
> +		 * No actual ref from command line, but "HEAD" from
> +		 * rev->def was added in setup_revisions()
> +		 * e.g. format-patch --cover-letter -12
> +		 */
> +		ref = "HEAD";
> +	else
>  		return NULL;
> -	ref = rev->cmdline.rev[positive].name;
>  	if (dwim_ref(ref, strlen(ref), branch_sha1, &full_ref) &&
>  	    !prefixcmp(full_ref, "refs/heads/") &&
> -	    !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1))
> +	    (positive < 0 ||
> +	     !hashcmp(rev->cmdline.rev[positive].item->sha1, branch_sha1)))

This hashcmp() is to make sure that the tip we _guessed_ with dwim_ref() that the user meant matches what we are going to use, so I do not think disabling the check is a good idea.

You could (and I think you should) do something like this:

	if (0 <= positive) {
        	ref = rev->cmdline.rev[positive].name;
                tip_sha1 = rev->cmdline.rev[positive].item->sha1;
	} else if (... defaulted to implied HEAD? ...) {
		ref = "HEAD";
                tip_sha1 = rev->pending.objects[0].item->sha1;
	} else {
		return NULL;
	}

        if (dwim_ref(...) && !prefixcmp(full_ref, "refs/heads/") &&
            !hashcmp(tip_sha1, branch_sha1))

to preserve that safety instead.