Re: [PATCH 2/2] Honor GIT_DEFAULT_HASH for empty clones without remote algo

From: Jeff King <peff@peff.net>
Date: Apr 27, 2023, 05:43 UTC
Message-ID: <20230427054343.GE982277@coredump.intra.peff.net>
In-Reply-To: <xmqqbkjaqqfp.fsf@gitster.g>

On Wed, Apr 26, 2023 at 02:33:30PM -0700, Junio C Hamano wrote:

Show 24 quoted lines

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> >  `GIT_DEFAULT_HASH`::
> >  	If this variable is set, the default hash algorithm for new
> >  	repositories will be set to this value. This value is currently
> > +	ignored when cloning if the remote value can be definitively
> > +	determined; the setting of the remote repository is used
> > +	instead. The value is honored if the remote repository's
> > +	algorithm cannot be determined, such as some cases when
> > +	the remote repository is empty. The default is "sha1".
> > +	THIS VARIABLE IS EXPERIMENTAL! See `--object-format`
> > +	in linkgit:git-init[1].
> 
> We'd need to evantually cover all the transports (and non-transport
> like the "--local" optimization) so that the object-format and other
> choices are communicated from the origin to a new clone anyway, so
> this extra complexity "until X is fixed, it behaves this way, but
> otherwise the variable is read in the meantime" may be a disservice
> to the end users, even though it may make it easier in the shorter
> term for maintainers of programs that rely on the buggy "git clone"
> that partially honored this environment variable.
> 
> In short, I am still not convinced that the above is a good design
> choice in the longer term.

I also think it is working against the backwards-compatible design of the hash function transition. If we do not see an object-format line from the remote, then either:

  1. They sent us capabilities, but it did not include object-format. So
     if we are in GIT_DEFAULT_HASH=sha256 mode locally, but the other
     side is an older version of Git (or even a current version of other
     implementations, like Dulwich) that do not send object-format at
     all, then we will not correctly fall back to assuming they are
     sha1. In a non-empty repo, this means we'll fail to parse their ref
     advertisement (we'll expect sha256 hashes but get sha1), and
     cloning will be broken.

  2. They did not send us capabilities, because the repo is empty (and
     the server does not have brian's patch 1). The hash transition doc
     says we're supposed to assume they're sha1. It's _sort of_
     academic, in that they also are not telling us about any refs on
     their side. But we may end up mis-matched with the server (again,
     this is the 50/50 thing; we don't know what their format is).
     Presumably that bites us later when we try to push up new objects
     (but would eventually work when we support interop).

I think handling (2) is iffy as a goal, but the collateral damage of (1) is a complete show-stopper for this patch. If we wanted to do (2) by itself, we'd have to distinguish "did they even send us a capabilities line" as a separate case (but I tend to agree with you that it is not worth doing for now).

-Peff