From: Junio C Hamano <firstname.lastname@example.org> To: Jeff King <email@example.com> Cc: Felipe Contreras <firstname.lastname@example.org>, email@example.com, Jonathan Nieder <firstname.lastname@example.org>, Dominik Salvet <email@example.com> Subject: Re: [RFC/PATCH] Add fetch.updateHead option Date: Fri, 20 Nov 2020 16:28:45 -0800 Message-ID: <firstname.lastname@example.org> (raw) In-Reply-To: <20201120235203.GA353076@coredump.intra.peff.net> (Jeff King's message of "Fri, 20 Nov 2020 18:52:03 -0500") Jeff King <email@example.com> 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.
next prev parent reply other threads:[~2020-11-21 0:29 UTC|newest] Thread overview: 13+ messages / expand[flat|nested] mbox.gz Atom feed top 2020-11-18 9:12 Felipe Contreras 2020-11-18 9:30 ` Ævar Arnfjörð Bjarmason 2020-11-18 9:43 ` Felipe Contreras 2020-11-18 15:53 ` Junio C Hamano 2020-11-18 19:04 ` Felipe Contreras 2020-11-20 23:52 ` Jeff King 2020-11-21 0:28 ` Junio C Hamano [this message] 2020-11-21 0:40 ` Jeff King 2020-11-21 1:18 ` Felipe Contreras 2020-11-24 6:58 ` Jeff King 2020-11-21 1:53 ` Felipe Contreras 2020-11-21 1:41 ` Felipe Contreras 2020-11-24 7:09 ` Jeff King
Reply instructions: You may reply publicly to this message via plain-text email using any one of the following methods: * Save the following mbox file, import it into your mail client, and reply-to-all from there: mbox Avoid top-posting and favor interleaved quoting: https://en.wikipedia.org/wiki/Posting_style#Interleaved_style List information: http://vger.kernel.org/majordomo-info.html * Reply using the --to, --cc, and --in-reply-to switches of git-send-email(1): git send-email \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ --email@example.com \ --firstname.lastname@example.org \ /path/to/YOUR_REPLY https://kernel.org/pub/software/scm/git/docs/git-send-email.html * If your mail client supports setting the In-Reply-To header via mailto: links, try the mailto: link
email@example.com list mirror (unofficial, one of many) This inbox may be cloned and mirrored by anyone: git clone --mirror https://public-inbox.org/git git clone --mirror http://ou63pmih66umazou.onion/git git clone --mirror http://czquwvybam4bgbro.onion/git git clone --mirror http://hjrcffqmbrq6wope.onion/git # If you have public-inbox 1.1+ installed, you may # initialize and index your mirror using the following commands: public-inbox-init -V1 git git/ https://public-inbox.org/git \ firstname.lastname@example.org public-inbox-index git Example config snippet for mirrors. Newsgroups are available over NNTP: nntp://news.public-inbox.org/inbox.comp.version-control.git nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git nntp://news.gmane.io/gmane.comp.version-control.git note: .onion URLs require Tor: https://www.torproject.org/ code repositories for the project(s) associated with this inbox: https://80x24.org/mirrors/git.git AGPL code for this site: git clone https://public-inbox.org/public-inbox.git