On Wed, Apr 26, 2023 at 02:33:30PM -0700, Junio C Hamano wrote:
> "brian m. carlson" <sandals@xxxxxxxxxxxxxxxxxxxx> 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
