Re: [PATCH v13 5/9] remote set-head: better output for --auto

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

Bence Ferdinandy <bence@ferdinandy.com> writes:

> +static void report_set_head_auto(const char *remote, const char *head_name,
> +			struct strbuf *b_local_head, int updateres) {

"updateres" was too mysterious a name.  "res" stands for what,
"resource"?

Looking at the way the parameter is used by the code, it seems to
indicate that the remote HEAD originally was in a detached state, so
"was_detached" may be a better name, perhaps?

> +	else if (!!updateres && b_local_head->len)
> +		printf(_("'%s/HEAD' was detached at '%s' and now points to '%s'\n"),
> +			remote, b_local_head->buf, head_name);

There is no need for !!; any non-zero integer is true.  !! is useful
only in a context that takes only 0 and 1 (like when you are making
an assignment to a variable or a structure member that takes only 0
or 1).

>  static int set_head(int argc, const char **argv, const char *prefix)
>  {
> -	int i, opt_a = 0, opt_d = 0, result = 0;
> -	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT;
> +	int i, opt_a = 0, opt_d = 0, result = 0, updateres;
> +	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
> +		b_local_head = STRBUF_INIT;

> @@ -1440,20 +1468,27 @@ static int set_head(int argc, const char **argv, const char *prefix)
>  	} else
>  		usage_with_options(builtin_remote_sethead_usage, options);
>  
> -	if (head_name) {
> -		strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name);
> -		/* make sure it's valid */
> -		if (!refs_ref_exists(refs, b_remote_head.buf))
> -			result |= error(_("Not a valid ref: %s"), b_remote_head.buf);
> -		else if (refs_update_symref(refs, b_head.buf, b_remote_head.buf, "remote set-head"))
> -			result |= error(_("Could not setup %s"), b_head.buf);
> -		else if (opt_a)
> -			printf("%s/HEAD set to %s\n", argv[0], head_name);
> -		free(head_name);
> +	if (!head_name)
> +		goto cleanup;
> +	strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", argv[0], head_name);
> +	if (!refs_ref_exists(refs, b_remote_head.buf)) {
> +		result |= error(_("Not a valid ref: %s"), b_remote_head.buf);
> +		goto cleanup;
> +	}

OK, we refuse to allow a manual "remote set-head" to create a
dangling symref, which is a faithful rewrite from the original.

> +	updateres = refs_update_symref_extended(refs, b_head.buf, b_remote_head.buf,
> +			"remote set-head", &b_local_head);

> +	if (updateres == -2) {

Where does this -2 come from?  It is not the "you asked to read it
as a symref but it wasn't a symref" thing, which was mapped to -1
with [PATCH 3/9].

It is an unusual way to construct an extensible API function to say
"all different kinds of errors we happen to know when this
particular caller was written return -2, but some special cases are
not -2".

Rather, "all negatives, other than these selected few values we
special-case and handle, are errors" is more natural, isn't it?

Maybe I am misreading the code and missing where the -2 comes from
or the significance of the value?  I dunno.