Brian questioned the unnecessary alarms in v1 when "git diff" or "git
index-pack" run in no-repository mode. I had the same feeling after
sending v1. But instead of suppresing alarms, we could do better.

v2 breaks those "fall back to SHA-1" code into separate patches and
handles it properly (I hope) instead of blind fall back like v1:

- for index-pack, we can determine the needed algorithm from the pack
  file. I'm making an assumption here that pack files with new hash
  algo must step up file format version. But I think it's a reasonable

- for diff --no-index, I still fall back to SHA-1 to generate the
  hashes like before. We could probably introduce a new command line
  option to use a different hash. But that work could be done later
  when an actual new hash has come

Note, I didn't test but this series could potentially break 'pu' (in a
good way). I initially was puzzled why the test suite didn't fail when
the_repository->hash_algo was NULL (i.e. before Brian's fix). Then I
found out that more work to abstract away SHA-1 hashing functions have
been done since then. But because the_hash_algo is pre-initialized
with SHA-1, these special cases did not show up until now. If there
are more the_hash_algo conversion on 'pu', more "no-repo" cases
could be spotted by the test suite.

I think this makes pre-initializing the_hash_algo to NULL a very good
point: during the abstraction work, we at least must identify the use
cases like this (code running without repo). We don't have to fix it
right away, but we can start thinking about how to deal with it. If we
ignore it and switch to a new hash, these code will keep using SHA-1
and cause more headache in future.

Nguyễn Thái Ngọc Duy (5):
  setup.c: initialize the_repository correctly in all cases
  sha1_file.c: keep a good name for "unknown" hash_algos[UNKNOWN]
  index-pack: check (and optionally set) hash algo based on input file
  diff.c: initialize hash algo when running in --no-index mode
  Revert "repository: pre-initialize hash algo pointer"

 builtin/index-pack.c             | 26 +++++++++++++++++++++++++-
 builtin/init-db.c                |  3 ++-
 cache.h                          |  3 ++-
 common-main.c                    | 10 ++++++++++
 diff.c                           | 12 ++++++++++++
 path.c                           |  2 +-
 repository.c                     |  2 +-
 setup.c                          |  5 ++++-
 sha1_file.c                      |  2 +-
 t/helper/test-dump-split-index.c |  2 ++
 10 files changed, 60 insertions(+), 7 deletions(-)


Reply via email to