git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jeff King <peff@peff.net>
Cc: Felipe Contreras <felipe.contreras@gmail.com>,
	git@vger.kernel.org, Jonathan Nieder <jrnieder@gmail.com>,
	Dominik Salvet <dominik.salvet@gmail.com>
Subject: Re: [RFC/PATCH] Add fetch.updateHead option
Date: Fri, 20 Nov 2020 16:28:45 -0800
Message-ID: <xmqq3613tuwy.fsf@gitster.c.googlers.com> (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 <peff@peff.net> 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.

  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 \
    --in-reply-to=xmqq3613tuwy.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=dominik.salvet@gmail.com \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=jrnieder@gmail.com \
    --cc=peff@peff.net \
    /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

git@vger.kernel.org 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 \
		git@vger.kernel.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