On Tue, Oct 16, 2018 at 11:00:19AM +0900, Junio C Hamano wrote:
> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> 
> > Since the commit-graph code wants to serialize the hash algorithm into
> > the data store, specify a version number for each supported algorithm.
> > Note that we don't use the values of the constants themselves, as they
> > are internal and could change in the future.
> >
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> >  commit-graph.c | 9 ++++++++-
> >  1 file changed, 8 insertions(+), 1 deletion(-)
> >
> > diff --git a/commit-graph.c b/commit-graph.c
> > index 7a28fbb03f..e587c21bb6 100644
> > --- a/commit-graph.c
> > +++ b/commit-graph.c
> > @@ -45,7 +45,14 @@ char *get_commit_graph_filename(const char *obj_dir)
> >  
> >  static uint8_t oid_version(void)
> >  {
> > -	return 1;
> > +	switch (hash_algo_by_ptr(the_hash_algo)) {
> > +		case GIT_HASH_SHA1:
> > +			return 1;
> > +		case GIT_HASH_SHA256:
> > +			return 2;
> > +		default:
> > +			BUG("unknown hash algorithm");
> > +	}
> 
> Style: align switch/case.

Will fix.

> Shouldn't this be just using GIT_HASH_* constants instead?  Are we
> expecting that it would be benefitial to allow more than one "oid
> version" even within a same GIT_HASH_*?

I really would like to have us rely not rely explicitly on those values.
We don't serialize them anywhere else, and they're explicitly documented
as not being suitable for serialization.  If we were writing this fresh
today, we'd probably use the format_version field, which is designed for
this purpose, or simply omit the field altogether.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204