Re: [PATCH] Give better 'pull' advice when pushing non-ff updates to current branch

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 24, 2012, 02:29 UTC
Message-ID: <xmqqr4vdhnfh.fsf@junio.mtv.corp.google.com>
In-Reply-To: <1335221121-36664-1-git-send-email-christiwald@gmail.com>

Christopher Tiwald <christiwald@gmail.com> writes:

Show 14 quoted lines

> @@ -177,6 +192,16 @@ static int push_with_options(struct transport *transport, int flags)
>  {
>  	int err;
>  	int nonfastforward;
> +	struct branch *branch;
> +	struct strbuf buf = STRBUF_INIT;
> +
> +	branch = branch_get(NULL);
> +
> +	if (branch) {
> +		strbuf_addstr(&buf, transport->remote->name);
> +		strbuf_addstr(&buf, "/");
> +		strbuf_addstr(&buf, branch->name);
> +	}

The "buf" is a horrible name for a variable that is used to hold states in a long haul that has to span multiple hunks in a patch. Please name it after what the value means.

Show 13 quoted lines

> @@ -201,7 +226,18 @@ static int push_with_options(struct transport *transport, int flags)
>  	default:
>  		break;
>  	case NON_FF_HEAD:
> -		advise_pull_before_push();
> +		/* Branches configured for octopus merges should advise
> +		 * just 'git pull' */
> +		if (branch->remote_name &&
> +		    branch->merge &&
> +		    branch->merge_nr == 1 &&
> +		    !strcmp(transport->remote->name, branch->remote_name) &&
> +		    !strcmp(strbuf_detach(&buf, NULL),
> +			    prettify_refname(branch->merge[0]->dst))) {

Why detach?  buf_to_be_renamed_more_sanely.buf, perhaps?

Is comparison between whatever buf has and the result of prettify safe and sane? After all, prettify is a random abbreviation that is meant for human consumption, assuming that the reader is intelligent enough to guess "Ah, it must be a tag" when she sees "v1.0" which is the result of stripping the leading "refs/tags/" out.

This part should be using straight strcmp against branch->merge[0]->dst, which means buf_to_be_renamed_more_sanely in the earlier hunk needs to be computing what's tracked and merged correctly using the configured refspec mapping, perhaps?