Re: [PATCH v3 0/6] Fix initializing the_hash_algo

2018-02-26 Thread Stefan Beller 
On Sun, Feb 25, 2018 at 12:34 PM, brian m. carlson
 wrote:
> On Sun, Feb 25, 2018 at 06:18:34PM +0700, Nguyễn Thái Ngọc Duy wrote:
>> v3 refines v2 a bit more:
>>
>> - fix configure typo (and stray words in commit message)
>> - use repo_set_hash_algo() instead of reassigning the_hash_algo
>> - compare hash algos by format_id
>> - catch NULL hash algo, report nicely and suggest GIT_HASH_FIXUP
>>
>> The last point makes me much happier about keeping this workaround
>> around until we are confident we can live without it. Interdiff
>
> This looks sane to me.
>
> Reviewed-by: brian m. carlson 

I agree with this version as well.

Thanks,
Stefan

Re: [PATCH v3 0/6] Fix initializing the_hash_algo

2018-02-25 Thread brian m. carlson 
On Sun, Feb 25, 2018 at 06:18:34PM +0700, Nguyễn Thái Ngọc Duy wrote:
> v3 refines v2 a bit more:
> 
> - fix configure typo (and stray words in commit message)
> - use repo_set_hash_algo() instead of reassigning the_hash_algo
> - compare hash algos by format_id
> - catch NULL hash algo, report nicely and suggest GIT_HASH_FIXUP
> 
> The last point makes me much happier about keeping this workaround
> around until we are confident we can live without it. Interdiff

This looks sane to me.

Reviewed-by: brian m. carlson 
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

[PATCH v3 0/6] Fix initializing the_hash_algo

2018-02-25 Thread Nguyễn Thái Ngọc Duy 
v3 refines v2 a bit more:

- fix configure typo (and stray words in commit message)
- use repo_set_hash_algo() instead of reassigning the_hash_algo
- compare hash algos by format_id
- catch NULL hash algo, report nicely and suggest GIT_HASH_FIXUP

The last point makes me much happier about keeping this workaround
around until we are confident we can live without it. Interdiff

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 1144458140..2c75f28b41 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -331,21 +331,24 @@ static const char *open_pack_file(const char *pack_name)
 
 static void prepare_hash_algo(uint32_t pack_version)
 {
+   int pack_algo_id;
const struct git_hash_algo *pack_algo;
 
switch (pack_version) {
case 2:
case 3:
-   pack_algo = _algos[GIT_HASH_SHA1];
+   pack_algo_id = GIT_HASH_SHA1;
break;
default:
-   die("BUG: how to determine hash algo for new version?");
+   die("BUG: how to determine hash algo for version %d?",
+   pack_version);
}
 
-   if (!the_hash_algo) /* running without repo */
-   the_hash_algo = pack_algo;
+   if (!repo_has_valid_hash_algo(the_repository)) /* running without repo 
*/
+   repo_set_hash_algo(the_repository, pack_algo_id);
 
-   if (the_hash_algo != pack_algo)
+   pack_algo = _algos[pack_algo_id];
+   if (the_hash_algo->format_id != pack_algo->format_id)
die(_("incompatible hash algorithm, "
  "configured for %s but the pack file needs %s"),
the_hash_algo->name, pack_algo->name);
diff --git a/cache.h b/cache.h
index 6b97138264..55b31e9756 100644
--- a/cache.h
+++ b/cache.h
@@ -53,7 +53,7 @@ struct object_id {
unsigned char hash[GIT_MAX_RAWSZ];
 };
 
-#define the_hash_algo the_repository->hash_algo
+#define the_hash_algo repo_get_hash_algo(the_repository)
 
 #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
 #define DTYPE(de)  ((de)->d_type)
diff --git a/diff.c b/diff.c
index f5755425b6..5f28d84b87 100644
--- a/diff.c
+++ b/diff.c
@@ -3997,15 +3997,15 @@ static void run_diff(struct diff_filepair *p, struct 
diff_options *o)
 
/*
 * NEEDSWORK: When running in no-index mode (and no repo is
-* found, thus no hash algo conifugred), fall back to SHA-1
+* found, thus no hash algo configured), fall back to SHA-1
 * hashing (which is used by diff_fill_oid_info below) to
 * avoid regression in diff output.
 *
 * In future, perhaps we can allow the user to specify their
 * hash algorithm from command line in this mode.
 */
-   if (o->flags.no_index && !the_hash_algo)
-   the_hash_algo = _algos[GIT_HASH_SHA1];
+   if (o->flags.no_index && !repo_has_valid_hash_algo(the_repository))
+   repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
 
diff_fill_oid_info(one);
diff_fill_oid_info(two);
diff --git a/repository.h b/repository.h
index 0329e40c7f..40bd49611f 100644
--- a/repository.h
+++ b/repository.h
@@ -107,4 +107,20 @@ extern void repo_clear(struct repository *repo);
  */
 extern int repo_read_index(struct repository *repo);
 
+static inline const struct git_hash_algo *repo_get_hash_algo(
+   const struct repository *repo)
+{
+   if (!repo->hash_algo)
+   die("BUG: hash_algo is not initialized!\n%s",
+   _("You can work around this by setting environment"
+ " variable GIT_HASH_FIXUP=1.\n"
+ "Please report this to git@vger.kernel.org"));
+   return repo->hash_algo;
+}
+
+static inline int repo_has_valid_hash_algo(const struct repository *repo)
+{
+   return repo->hash_algo != NULL;
+}
+
 #endif /* REPOSITORY_H */

Nguyễn Thái Ngọc Duy (6):
  setup.c: initialize the_repository correctly in all cases
  sha1_file.c: keep a good name for "unknown" hash_algos[UNKNOWN]
  cache.h: make the_hash_algo read-only
  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 | 29 -
 builtin/init-db.c|  3 ++-
 cache.h  |  5 +++--
 common-main.c| 10 ++
 diff.c   | 12 
 path.c   |  2 +-
 repository.c |  2 +-
 repository.h | 16 
 setup.c  |  5 -
 sha1_file.c  |  2 +-
 t/helper/test-dump-split-index.c |  2 ++
 11 files changed, 80 insertions(+), 8 deletions(-)

-- 
2.16.1.435.g8f24da2e1a