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

From: "Bence Ferdinandy" <[email-masked]>
Date: Tue, 19 Nov 2024 11:29:34 +0100

On Tue Nov 19, 2024 at 03:27, Junio C Hamano <[email-masked]> wrote:
> Bence Ferdinandy <[email-masked]> 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?

"res" wanted to be short for result, but "was_detached" is definitely more readable.

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

No, it is not, but it's also a mistake. It should be `updateres == 1`.
refs_update_symref_extended outputs -1 for "not a symref" and 1 for any other
error currently. Before I touched the code it was 1 for any error, so I left
that as is. So we want to error out on set_head if we get a 1 and continue if
we get 0 or -1 (and handle the difference in the report_set_head_auto).

Thanks for noticing, I'll get that fixed in v14.