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