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
