Jeff King <peff@xxxxxxxx> writes: > Thanks for working on this. Regardless of whether we change the default > behavior, this seems like an obvious improvement (and I do think it > makes sense to eventually change the default; I'd even be OK switching > it to "missing" in the near term). I agree that "missing" would be an easy thing to take, and I do not mind seeing it made the default in the near term. It won't break existing expectations too much, and can even be seen as a bugfix for the current behaviour by making "init && fetch" a step closer to "clone". Beyond that to modify what the end user already has is a much harder sell. For some it may be an improvement, but for others it would be a breaking change. > The implementation looks pretty straight-forward, but I have a few > comments below: > ... >> + strbuf_addf(&ref, "refs/remotes/%s/HEAD", remote->name); >> + skip_prefix(head->symref, "refs/heads/", &head_name); > > Should we bail or complain if this skip_prefix() doesn't match? I think > it would be a sign of weirdness on the remote side, ... Yes, we should notice the weirdness and stop doing any further harm to the local side. >> + strbuf_addf(&target, "refs/remotes/%s/%s", remote->name, head_name); >> + >> + r = refs_resolve_ref_unsafe(get_main_ref_store(the_repository), >> + ref.buf, RESOLVE_REF_READING, >> + NULL, NULL); > > This won't resolve a symref pointing to an unborn branch, so it would > count as "missing". I.e.: > > git symbolic-ref refs/remotes/origin/HEAD refs/remotes/origin/nope > git -c fetch.updatehead=missing fetch > > will update it based on the remote HEAD. I guess I could see some > argument for defining "missing" in that way, but I suspect it is not > what somebody in this situation would expect. What do we do in "git clone" of an empty repository with the current branch not yet to be born? Modern Git tells where the HEAD points at even for unborn branch, so using that would be a natural thing to do. > If we do update the symref, we should probably tell the user. Better > still if we can print it as part of the usual fetch output table. True. >> + if (need_update_head) >> + strvec_push(&ref_prefixes, "HEAD"); >> refspec_ref_prefixes(&transport->remote->fetch, &ref_prefixes); >> + } > > Good catch. We need this for: > > git fetch origin > > since otherwise it doesn't ask about HEAD in a v2 exchange. What about: > > git fetch origin master > > That won't report on HEAD either, even with your patch, because it hits > the part of the conditional before your "else if". What should it do? I > can see an argument for "nothing, we only update head on full configured > fetches", but if so we should definitely make that clear in the > documentation. I can also see an argument for "always, if we happen to > have heard about it" (just like we opportunistically update tracking > refs even if they are fetched by name into FETCH_HEAD). After seeing that all users got accustomed to seeing an explicit fetch into FETCH_HEAD still update the remote-tracking branches, it is more consistent and easier to understand by users if the "if we happen to have heard about it" is used, I would think. Documenting the behaviour certainly is needed in a real version after this RFC. > In most cases the distinction would not matter anyway. The "guess" part > comes in only for ancient pre-symref-capability versions of Git (which > we are unlikely to see and it's OK if they just don't make use of the > feature), I agree with you that at this point the versions of Git that does not know about advertising symref can be left to rot. > or for remotes with detached HEADs (in which case not setting > our local origin/HEAD is probably the best thing, as a bad guess would > foul up a later use of the "missing" mode). Yes. That would help the "missing" mode. With a mode stronger than "missing" that modifies what the local side has, detaching origin/HEAD in a similar way as the remote has would be OK (although I do not see me using that mode personally). Thanks for a detailed review.
