On Tue Nov 19, 2024 at 03:27, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Bence Ferdinandy <bence@xxxxxxxxxxxxxx> 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.
