Re: [PATCH v3 2/2] fetch: set remote/HEAD if it does not exist

[Junio C Hamano <gitster@pobox.com>]

Bence Ferdinandy <bence@ferdinandy.com> writes:

> If the user has remote/HEAD set already and it looks like it has changed
> on the server, then print a message, otherwise set it if we can.
>
> Signed-off-by: Bence Ferdinandy <bence@ferdinandy.com>
> ---
>
> Notes:
>     v3: - does not rely on remote set-head anymore so it only authenticates
>         once
>         - uses the new REF_CREATE_ONLY to atomically check if the ref exists
>           and only write it if it doesn't
>         - in all other cases the maximum it does is print a warning

Except for one thing, I can tell from the above that the design is
sensible without looking at the actual patch.  Nicely described.

The "one thing" is what to do if you normally do *not* keep track of
the remote-tracking refs for this particular remote.

If I am only fetching refs (or HEAD) in FETCH_HEAD for immediate
consumtion by doing "git pull bence HEAD" with something like

    [remote "bence"]
	 URL = http://github.com/bence/git

(with no other configuration under remote.bence.* hierarchy), I do
not think I want the code to create refs/remotes/bence/HEAD, with
no other remote-tracking refs in the same hierarchy, even if it finds
no existing refs/remotes/bence/HEAD there.  For that, I suspect that 
you'd need to see if the pointee of refs/remotes/bence/HEAD either
already exists or this fetch is about to create it, or something
like that, before deciding to create a ref with the REF_CREATE_ONLY
flag.

If you are doing so already in the code (I haven't started reading
it yet at this moment), you should mention it in your proposed log
message, so discount my earlier "Nicely described" a bit ;-).

Let's see what actually happens in the code now.

> +static const char *abbrev_ref(const char *name, const char *prefix)
> +{
> +	skip_prefix(name, prefix, &name);
> +	return name;
> +}
> +#define abbrev_branch(name) abbrev_ref((name), "refs/heads/")

We do not call the act of optionally skipping prefix "abbreviate" in
this project (object names are abbreviated but that is done by
chomping the later bytes of a long name).

I suspect that a caller of either of these two functions are
inherently buggy in that they _optionally_ skip the prefix so they
do the same thing for "refs/heads/main" and "main" but not
"heads/main".  The callsites may need to be inspected to see how
they should deal with cases where skip_prefix() did *not* see
anything to skip.

> +static inline int set_head(const struct ref *remote_refs)

Drop "inline".  You do not want to inline this much code if there
are many callers, and if you have only one caller, the compiler
would inline it into the sole caller if it is the more efficient
thing to do.

> +{
> +	int result, ref_changed = 0;
> +	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT, b_local_head = STRBUF_INIT, b_prefix = 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;
> +
> +	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, abbrev_branch(ref->name));
> +
> +
> +	if (!heads.nr)
> +		result = 1;
> +	else if (heads.nr > 1) {
> +		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);
> +		strbuf_addf(&b_prefix, "refs/remotes/%s/", remote);
> +		if (!refs_read_symbolic_ref(get_main_ref_store(the_repository),b_head.buf,&b_local_head)) {

Overly long lines around here cannot be sanely reviewed on my
terminal, so I'll stop here for now.  

Do not omit SP after ",", as that is the least effective way to
shorten overly long lines.  Instead, consider introducing a
one-time-use temporary variables with meaningful name, e.g.

		struct ref_store *store = get_main_ref_store(the_repository);
                
                strbuf_addf(&b_head, "refs/remotes/%s/HEAD", remote);
		...
		if (!refs_read_symbolic_ref(store, b_head.buf, &current)) {
    			/* it's current value is in "current" */
			...