Re: [PATCH v2 2/3] setup: do not use invalid `repository_format`

[Jeff King <peff@peff.net>]

On Mon, Jan 14, 2019 at 07:34:56PM +0100, Martin Ågren wrote:

> If `read_repository_format()` encounters an error, `format->version`
> will be -1 and all other fields of `format` will be undefined. However,
> in `setup_git_directory_gently()`, we use `repo_fmt.hash_algo`
> regardless of the value of `repo_fmt.version`.
> 
> This can be observed by adding this to the end of
> `read_repository_format()`:
> 
> 	if (format->version == -1)
> 		format->hash_algo = 0; /* no-one should peek at this! */
> 
> This causes, e.g., "git branch -m q q2 without config should succeed" in
> t3200 to fail with "fatal: Failed to resolve HEAD as a valid ref."
> because it has moved .git/config out of the way and is now trying to use
> a bad hash algorithm.
> 
> Check that `version` is non-negative before using `hash_algo`.
> 
> This patch adds no tests, but do note that if we skip this patch, the
> next patch would cause existing tests to fail as outlined above.

I'm still somewhat confused about how this breaks. If we move
".git/config" out of the way, then we have no version indicator and
presumably we should guess GIT_HASH_SHA1. Which is what's happening if
we fail to call repo_set_hash_algo(), no?  In other words, wouldn't
repo_set_hash_algo() always be a noop in that case?

I get why adding the code snippet above would cause that assumption to
break, but I am just not sure why we would add that code snippet. ;)

I also get why read_repository_format() doing this in patch 3 would be a
problem:

  +       if (format->version == -1) {
  +               clear_repository_format(format);
  +               format->version = -1;
  +       }

but doesn't that point out that clear_repository_format() should be
setting hash_algo to GIT_HASH_SHA1 as the default (and likewise "bare =
-1", etc, that is done in that function)?

-Peff