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

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