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

["Bence Ferdinandy" <bence@ferdinandy.com>]

On Fri Sep 20, 2024 at 01:07, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

I'll take another look at this.

>
> 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 ;-).

:D

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

I copied this verbatim from builtin/remote.c
https://github.com/git/git/blob/master/builtin/remote.c#L267 so it looked like
a safe bet T_T I'm not 100% sure, but I think there's no use case where you
pass a ref directly to remote except for set_head, but there it's not used, so
it looks safe?

That being said, I can change the name(s) to not use abbrev here, especially if
you have a good suggestion. Maybe strip_prefix_ref/branch? Again in fetch, we
(should) know what type of refs we're calling it on, so should be safe?

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

Ack.

>
> > +{
> > +	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" */
> 			...


Ah, yeah terribly sorry about this, I've gotten so used to automatic
formatters, that I just didn't take a second look. I'll be more careful in the
next round.

-- 
bence.ferdinandy.com