Re: [PATCH 2/2] Revert "repository: pre-initialize hash algo pointer"

On 02/23, brian m. carlson wrote:

On Fri, Feb 23, 2018 at 04:56:40PM +0700, Nguyễn Thái Ngọc Duy wrote:

quoted

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 7e3e1a461c..8ee935504e 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c

@@ -1673,6 +1673,11 @@ int cmd_index_pack(int argc, const char **argv, const char *prefix)
 	if (prefix && chdir(prefix))
 		die(_("Cannot come back to cwd"));
 
+	if (!the_hash_algo) {
+		warning(_("Running without a repository, assuming SHA-1 hash"));
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+	}

Is this warning going to be visible to users in the normal course of
operation?  If so, people are probably going to find this bothersome or
alarming.

quoted

 	for (i = 1; i < argc; i++) {
 		const char *arg = argv[i];
 

diff --git a/common-main.c b/common-main.c
index 6a689007e7..12aec36794 100644
--- a/common-main.c
+++ b/common-main.c

@@ -1,6 +1,7 @@
 #include "cache.h"
 #include "exec_cmd.h"
 #include "attr.h"
+#include "repository.h"
 
 /*
  * Many parts of Git have subprograms communicate via pipe, expect the

@@ -40,5 +41,8 @@ int main(int argc, const char **argv)
 
 	restore_sigpipe_to_default();
 
+	if (getenv("GIT_HASH_FIXUP"))
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);

I'm lukewarm on adding this environment variable, but considering our
history here, we had probably better.  We can always remove it after a
few releases.

Yeah I don't know what the best thing to do here would be.  I mean we
could always just init the hash_algo for the_repository here so that we
don't ever have to worry about it, but I don't know if even that is the
right approach.

quoted

 	return cmd_main(argc, argv);
 }

diff --git a/diff-no-index.c b/diff-no-index.c
index 0ed5f0f496..f038f665bc 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c

@@ -241,6 +241,11 @@ void diff_no_index(struct rev_info *revs,
 	struct strbuf replacement = STRBUF_INIT;
 	const char *prefix = revs->prefix;
 
+	if (!the_hash_algo) {
+		warning(_("Running without a repository, assuming SHA-1 hash"));
+		repo_set_hash_algo(the_repository, GIT_HASH_SHA1);
+	}

Again, same concern.  I can imagine scripts that will blow up loudly if
git diff --no-index spews things to standard error.

I'm not opposed to making this more visible, but I wonder if maybe it
should only be visible to developers or such.  The only way I can think
of doing is that is with an advice options, but maybe there's a better
way.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

-- 
Brandon Williams