Bence Ferdinandy <bence@xxxxxxxxxxxxxx> writes:
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 80a64d0d26..c3d3c05950 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -1578,6 +1578,82 @@ static int backfill_tags(struct display_state *display_state,
> return retcode;
> }
>
> +static void report_set_head(const char *remote, const char *head_name,
> + struct strbuf *buf_prev) {
> + struct strbuf buf_prefix = STRBUF_INIT;
> + const char *prev_head;
> +
> + strbuf_addf(&buf_prefix, "refs/remotes/%s/", remote);
> + skip_prefix(buf_prev->buf, buf_prefix.buf, &prev_head);
The same "uninitialized prev_head" problem I discussed earlier for a
separate patch is here, I think.
> + if (prev_head && !strcmp(prev_head, head_name))
> + ;
> + else if (prev_head) {
> + printf("'HEAD' at '%s' has changed from '%s' to '%s'\n",
> + remote, prev_head, head_name);
> + printf("Run 'git remote set-head %s %s' to follow the change.\n",
> + remote, head_name);
> + }
If the condition is only for one arm, can't we do this without
else/if? Would the condition become too contorted to read if we did
so? Let's see.
if (prev_head && strcmp(prev_head, head_name)) {
HEAD has changed from prev_head to head_name;
}
I guess it is a bit subjective, but as long as you are used to read
"strcmp(a,b)" as "a and b are different", this should be easier to
read than the original.
> +static const char *strip_refshead(const char *name){
> + skip_prefix(name, "refs/heads/", &name);
> + return name;
> +}
> +
> +static int set_head(const struct ref *remote_refs)
> +{
> + int result = 0;
> + struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
> + b_local_head = STRBUF_INIT;
> + const char *remote = gtransport->remote->name;
> + char *head_name = NULL;
> + struct ref *ref, *matches;
> + struct ref *fetch_map = NULL, **fetch_map_tail = &fetch_map;
> + struct refspec_item refspec = {
> + .force = 0,
> + .pattern = 1,
> + .src = (char *) "refs/heads/*",
> + .dst = (char *) "refs/heads/*",
> + };
> + struct string_list heads = STRING_LIST_INIT_DUP;
> + struct ref_store *refs = get_main_ref_store(the_repository);
> +
> + get_fetch_map(remote_refs, &refspec, &fetch_map_tail, 0);
> + matches = guess_remote_head(find_ref_by_name(remote_refs, "HEAD"),
> + fetch_map, 1);
> + for (ref = matches; ref; ref = ref->next) {
> + string_list_append(&heads, strip_refshead(ref->name));
> + }
> +
> +
> + if (!heads.nr)
> + result = 1;
> + else if (heads.nr > 1) {
Curious use of extra {braces}. In this case we can just lose them.
> + result = 1;
> + } else
> + head_name = xstrdup(heads.items[0].string);
> + if (head_name) {
> + strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote);
> + strbuf_addf(&b_remote_head, "refs/remotes/%s/%s", remote, 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", &b_local_head, true))
> + result |= error(_("Could not setup %s"), b_head.buf);
Two problems.
* we do not capitalize the beginning of the sentence in an error()
message, i.e. "Could not" -> "could not" (or "cannot").
* "setup" is not a verb.
I know both were inherited from builtin/remote.c but existing
breakage is not a good excuse to make new code worse.
> + else {
Curious use of extra {braces}. In this case we can just lose them.
> + report_set_head(remote, head_name, &b_local_head);
> + }
> + free(head_name);
> + }
> +
> + strbuf_release(&b_head);
> + strbuf_release(&b_local_head);
> + strbuf_release(&b_remote_head);
> + return result;
> +}
> +
> static int do_fetch(struct transport *transport,
> struct refspec *rs,
> const struct fetch_config *config)
> @@ -1647,6 +1723,8 @@ static int do_fetch(struct transport *transport,
> "refs/tags/");
> }
>
> + strvec_push(&transport_ls_refs_options.ref_prefixes,"HEAD");
Missing SP after ",".
> if (must_list_refs) {
> trace2_region_enter("fetch", "remote_refs", the_repository);
> remote_refs = transport_get_remote_refs(transport,
> @@ -1791,6 +1869,9 @@ static int do_fetch(struct transport *transport,
> "you need to specify exactly one branch with the --set-upstream option"));
> }
> }
> + if (set_head(remote_refs))
> + ; // Way too many cases where this can go wrong.
> + // Just fail silently.
/*
* our multi-line comments
* look like this.
*/
More importantly, we are not failing silently. As we saw in the
implementation of set_head() above, a non-zero return came from ret
that was assigned the return value of error() in the function, so an
error message is already given, no?
> @@ -2021,6 +2102,7 @@ static int fetch_multiple(struct string_list *list, int max_children,
> return !!result;
> }
>
> +
> /*
> * Fetching from the promisor remote should use the given filter-spec
> * or inherit the default filter-spec from the config.
An unintended change?
Thanks.
