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

From: Brandon Williams <[email-masked]>
Date: Fri, 23 Feb 2018 15:16:08 -0800

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:
> > 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.
> 
> >  	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.

> 
> >  	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