| Message ID | 20240513192112.866021-2-gitster@pobox.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Fix use of uninitialized hash algorithms | expand |
On Mon, May 13, 2024 at 12:21 PM Junio C Hamano <gitster@pobox.com> wrote: > > Partially revert c8aed5e8 (repository: stop setting SHA1 as the > default object hash, 2024-05-07), to keep end-user systems still > broken when we have gap in our test coverage but yet give them an > escape hatch to set the GIT_TEST_DEFAULT_HASH_IS_SHA1=1 environment > variable to revert to the previous behaviour. > > Signed-off-by: Junio C Hamano <gitster@pobox.com> > --- > repository.c | 24 ++++++++++++++++++++++++ Nice idea, this concept LG. > t/test-lib.sh | 4 ++++ > 2 files changed, 28 insertions(+) > > diff --git a/repository.c b/repository.c > index 15c10015b0..6f67966e35 100644 > --- a/repository.c > +++ b/repository.c > @@ -26,6 +26,30 @@ void initialize_repository(struct repository *repo) > repo->parsed_objects = parsed_object_pool_new(); > ALLOC_ARRAY(repo->index, 1); > index_state_init(repo->index, repo); > + > + /* > + * Unfortunately, we need to keep this hack around for the time being: Nit: the comment says we need to keep it around, but it's actually defaulted to off. We may want to add clarification of the current status to the comment (or replace the entire comment with a comment describing that we added a hack to the hack, and that it should be removed "soon"). > + * > + * - Not setting up the hash algorithm for `the_repository` leads to > + * crashes because `the_hash_algo` is a macro that expands to > + * `the_repository->hash_algo`. So if Git commands try to access > + * `the_hash_algo` without a Git directory we crash. > + * > + * - Setting up the hash algorithm to be SHA1 by default breaks other > + * commands when running with SHA256. > + * > + * This is another point in case why having global state is a bad idea. > + * Eventually, we should remove this hack and stop setting the hash > + * algorithm in this function altogether. Instead, it should only ever > + * be set via our repository setup procedures. But that requires more > + * work. > + * > + * As we discover the code paths that need fixing, we may remove this > + * code completely, but we are not there yet. > + */ > + if (repo == the_repository && > + git_env_bool("GIT_TEST_DEFAULT_HASH_IS_SHA1", 0)) > + repo_set_hash_algo(repo, GIT_HASH_SHA1); > } > > static void expand_base_dir(char **out, const char *in, > diff --git a/t/test-lib.sh b/t/test-lib.sh > index 79d3e0e7d9..36d311fb4a 100644 > --- a/t/test-lib.sh > +++ b/t/test-lib.sh > @@ -542,6 +542,10 @@ export EDITOR > > GIT_DEFAULT_HASH="${GIT_TEST_DEFAULT_HASH:-sha1}" > export GIT_DEFAULT_HASH > + > +GIT_TEST_DEFAULT_HASH_IS_SHA1=0 This is the default as being established in this commit, correct? Should we add a comment saying this is the default and describe why we have it here anyway? > +export GIT_TEST_DEFAULT_HASH_IS_SHA1 > + > GIT_DEFAULT_REF_FORMAT="${GIT_TEST_DEFAULT_REF_FORMAT:-files}" > export GIT_DEFAULT_REF_FORMAT > GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}" > -- > 2.45.0-145-g3e4a232f6e > >
diff --git a/repository.c b/repository.c index 15c10015b0..6f67966e35 100644 --- a/repository.c +++ b/repository.c @@ -26,6 +26,30 @@ void initialize_repository(struct repository *repo) repo->parsed_objects = parsed_object_pool_new(); ALLOC_ARRAY(repo->index, 1); index_state_init(repo->index, repo); + + /* + * Unfortunately, we need to keep this hack around for the time being: + * + * - Not setting up the hash algorithm for `the_repository` leads to + * crashes because `the_hash_algo` is a macro that expands to + * `the_repository->hash_algo`. So if Git commands try to access + * `the_hash_algo` without a Git directory we crash. + * + * - Setting up the hash algorithm to be SHA1 by default breaks other + * commands when running with SHA256. + * + * This is another point in case why having global state is a bad idea. + * Eventually, we should remove this hack and stop setting the hash + * algorithm in this function altogether. Instead, it should only ever + * be set via our repository setup procedures. But that requires more + * work. + * + * As we discover the code paths that need fixing, we may remove this + * code completely, but we are not there yet. + */ + if (repo == the_repository && + git_env_bool("GIT_TEST_DEFAULT_HASH_IS_SHA1", 0)) + repo_set_hash_algo(repo, GIT_HASH_SHA1); } static void expand_base_dir(char **out, const char *in, diff --git a/t/test-lib.sh b/t/test-lib.sh index 79d3e0e7d9..36d311fb4a 100644 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -542,6 +542,10 @@ export EDITOR GIT_DEFAULT_HASH="${GIT_TEST_DEFAULT_HASH:-sha1}" export GIT_DEFAULT_HASH + +GIT_TEST_DEFAULT_HASH_IS_SHA1=0 +export GIT_TEST_DEFAULT_HASH_IS_SHA1 + GIT_DEFAULT_REF_FORMAT="${GIT_TEST_DEFAULT_REF_FORMAT:-files}" export GIT_DEFAULT_REF_FORMAT GIT_TEST_MERGE_ALGORITHM="${GIT_TEST_MERGE_ALGORITHM:-ort}"
Partially revert c8aed5e8 (repository: stop setting SHA1 as the default object hash, 2024-05-07), to keep end-user systems still broken when we have gap in our test coverage but yet give them an escape hatch to set the GIT_TEST_DEFAULT_HASH_IS_SHA1=1 environment variable to revert to the previous behaviour. Signed-off-by: Junio C Hamano <gitster@pobox.com> --- repository.c | 24 ++++++++++++++++++++++++ t/test-lib.sh | 4 ++++ 2 files changed, 28 insertions(+)