Re: [PATCH v5 6/6] fetch: set remote/HEAD if it does not exist
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Oct 9, 2024, 22:01 UTC
- Message-ID
- <xmqqzfndndfm.fsf@gitster.g>
- In-Reply-To
- <20241009135747.3563204-6-bence@ferdinandy.com>
Bence Ferdinandy <bence@ferdinandy.com> writes:
Show 15 quoted lines
> 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.
Show 8 quoted lines
> + 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.
Show 34 quoted lines
> +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);
Show 9 quoted lines
> + 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.Show 19 quoted lines
> + 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 ",".
Show 10 quoted lines
> 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?
Show 8 quoted lines
> @@ -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.