On Fri Sep 20, 2024 at 01:07, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Bence Ferdinandy <bence@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > > --- > > > > 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, ¤t)) { > /* 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
