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

From: Jeff King <peff@peff.net>
Date: Jan 15, 2019, 19:31 UTC
Message-ID: <20190115193112.GE4886@sigill.intra.peff.net>
In-Reply-To: <a7e58385290e6789105d2d5b794f4bf8607285dd.1547488709.git.martin.agren@gmail.com>

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

Show 20 quoted lines

> 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