v2, 4 partsHash Abstraction

> We enumerate several different items as part of struct
> repository_format, but then actually set up those values using the
> global variables we've initialized from them.  Instead, let's pass a
> pointer to the structure down to the code where we enumerate these
> values, so we can later on use those values directly to perform setup.
>
> This technique makes it easier for us to determine additional items
> about the repository format (such as the hash algorithm) and then use
> them for setup later on, without needing to add additional global
> variables.  We can't avoid using the existing global variables since
> they're intricately intertwined with how things work at the moment, but
> this improves things for the future.

> In future versions of Git, we plan to support an additional hash
> algorithm.  Integrate the enumeration of hash algorithms with repository
> setup, and store a pointer to the enumerated data in struct repository.
> Of course, we currently only support SHA-1, so hard-code this value in
> read_repository_format.  In the future, we'll enumerate this value from
> the configuration.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/cache.h b/cache.h
> @@ -132,6 +133,8 @@ struct git_hash_algo {
>  extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
>
> +#define current_hash the_repository->hash_algo

> diff --git a/repository.c b/repository.c
> @@ -64,6 +64,11 @@ void repo_set_gitdir(struct repository *repo, const char *path)
> +void repo_set_hash_algo(struct repository *repo, int hash_algo)
> +{
> +       repo->hash_algo = &hash_algos[hash_algo];
> +}
> +
>  /*
>   * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.
>   * Return 0 upon success and a non-zero value upon failure.
> @@ -136,6 +141,8 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
>         if (read_and_verify_repository_format(&format, repo->commondir))
>                 goto error;
>
> +       repo->hash_algo = &hash_algos[format.hash_algo];

> > diff --git a/cache.h b/cache.h
> > @@ -132,6 +133,8 @@ struct git_hash_algo {
> >  extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
> >
> > +#define current_hash the_repository->hash_algo
> 
> The all-lowercase name "current_hash" seems likely to conflict with a
> variable name some day; the fact that it is also a #define makes such
> a collision even more worrisome. Although it is retrieving the "hash
> algorithm", when reading the terse name "current_hash", one may
> instead intuitively think it is referring to a hash _value_ (not an
> algorithm).

> >   * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.
> >   * Return 0 upon success and a non-zero value upon failure.
> > @@ -136,6 +141,8 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)
> >         if (read_and_verify_repository_format(&format, repo->commondir))
> >                 goto error;
> >
> > +       repo->hash_algo = &hash_algos[format.hash_algo];
> 
> Should this be instead invoking repo_set_hash_algo()?

> On Sat, Oct 28, 2017 at 09:44:07PM -0400, Eric Sunshine wrote:
>> > +#define current_hash the_repository->hash_algo
>>
>> The all-lowercase name "current_hash" seems likely to conflict with a
>> variable name some day; the fact that it is also a #define makes such
>> a collision even more worrisome. Although it is retrieving the "hash
>> algorithm", when reading the terse name "current_hash", one may
>> instead intuitively think it is referring to a hash _value_ (not an
>> algorithm).
>
> I can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's
> an improvement.  I originally omitted the "algo" portion to keep it
> short.

> On Sun, Oct 29, 2017 at 1:57 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > I can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's
> > an improvement.  I originally omitted the "algo" portion to keep it
> > short.
> 
> I don't have strong feelings about it aside from worrying about a
> "current_hash" name clash or a reader misunderstanding what it
> represents.
> 
> Does "current" need to be in the name? What about HASH_ALGO or REPO_HASH_ALGO?
> 
> > Alternatively, we could have a current_hash() (or current_hash_algo())
> > inline function if people like that better.
> 
> hash_algo() or repo_hash_algo()?

> On Sun, Oct 29, 2017 at 03:02:20PM -0400, Eric Sunshine wrote:
>> On Sun, Oct 29, 2017 at 1:57 PM, brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>> > I can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's
>> > an improvement.  I originally omitted the "algo" portion to keep it
>> > short.
>> 
>> I don't have strong feelings about it aside from worrying about a
>> "current_hash" name clash or a reader misunderstanding what it
>> represents.
>> 
>> Does "current" need to be in the name? What about HASH_ALGO or REPO_HASH_ALGO?
>> 
>> > Alternatively, we could have a current_hash() (or current_hash_algo())
>> > inline function if people like that better.
>> 
>> hash_algo() or repo_hash_algo()?
>
> Those are also fine, and shorter to boot.  I'll wait to see if anyone
> has strong opinions on the direction we should go before making a
> change.

> Is the plan to allow running with multiple hash algorithms in
> parallel?  I thought what we want to see in the future codebase was
> to have the default hash algorithm used for everything except for a
> select few codepaths, and assumed that the way we achieve it is to
> 
>  - allow very low level helper functions (e.g. read_sha1_file(),
>    write_sha1_file_prepare(), etc.) to take a pointer to the hash
>    algorithm structure;
> 
>  - have higher level helper functions to call these low level
>    helpers with a fixed singleton instance of hash algorithm
>    structure that represents that default one (SHA2-something?); and
> 
>  - a few selected codepaths (e.g. index-pack that reads SHA-1 stream
>    and converts it into NewHash pack/index while building the object
>    name mapping) use an additional singleton instance of hash
>    algorithm structure that represents the SHA-1 hash, and the way
>    they use it is *NOT* by replacing "the current" one with SHA-1,
>    but by explicitly passing the instance to the low level helpers
>    as parameter.

> So, "current" does indeed sound quite wrong, as it makes it sound as
> if you can swap it anytime and ask "which one is in effect now?".
> If we do not want to call the default instance "SHA-2" because we
> want to prepare for migrating again by not having the name of a
> concrete hash algorithm sprinkled in the codebase all over like we
> currently do, why not call the instance "the_hash_algo"?

>  };
>  extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
>
> +#define current_hash the_repository->hash_algo
> +
>  #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
>  #define DTYPE(de)      ((de)->d_type)
>  #else
> @@ -920,6 +923,7 @@ struct repository_format {
>         int version;
>         int precious_objects;
>         int is_bare;
> +       int hash_algo;

> Since in the future we want to support an additional hash algorithm, add
> a structure that represents a hash algorithm and all the data that must
> go along with it.  Add a constant to allow easy enumeration of hash
> algorithms.  Implement function typedefs to create an abstract API that
> can be used by any hash algorithm, and wrappers for the existing SHA1
> functions that conform to this API.
> [...]
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> diff --git a/cache.h b/cache.h
> @@ -77,6 +77,61 @@ struct object_id {
> +typedef void (*git_hash_init_fn)(void *ctx);
> +typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);
> +typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);
> +
> +struct git_hash_algo {
> +       [...]
> +       /* The hash initialization function. */
> +       git_hash_init_fn init_fn;
> +
> +       /* The hash update function. */
> +       git_hash_update_fn update_fn;
> +
> +       /* The hash finalization function. */
> +       git_hash_final_fn final_fn;
> +       [...]
> +};
> diff --git a/sha1_file.c b/sha1_file.c
> @@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {
> +static inline void git_hash_sha1_init(void *ctx)
> +{
> +       git_SHA1_Init((git_SHA_CTX *)ctx);
> +}
> +
> +static inline void git_hash_sha1_update(void *ctx, const void *data, size_t len)
> +{
> +       git_SHA1_Update((git_SHA_CTX *)ctx, data, len);
> +}
> +
> +static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)
> +{
> +       git_SHA1_Final(hash, (git_SHA_CTX *)ctx);
> +}

> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
> +       [...]
> +       {
> +               "sha-1",
> +               /* "sha1", big-endian */
> +               0x73686131,
> +               sizeof(git_SHA_CTX),
> +               GIT_SHA1_RAWSZ,
> +               GIT_SHA1_HEXSZ,
> +               git_hash_sha1_init,
> +               git_hash_sha1_update,
> +               git_hash_sha1_final,
> +               &empty_tree_oid,
> +               &empty_blob_oid,
> +       },
> +};

> On Sat, Oct 28, 2017 at 2:12 PM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > diff --git a/sha1_file.c b/sha1_file.c
> > @@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {
> > +static inline void git_hash_sha1_init(void *ctx)
> > +{
> > +       git_SHA1_Init((git_SHA_CTX *)ctx);
> > +}
> > +
> > +static inline void git_hash_sha1_update(void *ctx, const void *data, size_t len)
> > +{
> > +       git_SHA1_Update((git_SHA_CTX *)ctx, data, len);
> > +}
> > +
> > +static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)
> > +{
> > +       git_SHA1_Final(hash, (git_SHA_CTX *)ctx);
> > +}
> 
> Why 'inline' if you only ever take the addresses of these functions?

> Since in the future we want to support an additional hash algorithm, add
> a structure that represents a hash algorithm and all the data that must
> go along with it.  Add a constant to allow easy enumeration of hash
> algorithms.  Implement function typedefs to create an abstract API that
> can be used by any hash algorithm, and wrappers for the existing SHA1
> functions that conform to this API.
>
> Expose a value for hex size as well as binary size.  While one will
> always be twice the other, the two values are both used extremely
> commonly throughout the codebase and providing both leads to improved
> readability.
>
> Don't include an entry in the hash algorithm structure for the null
> object ID.  As this value is all zeros, any suitably sized all-zero
> object ID can be used, and there's no need to store a given one on a
> per-hash basis.
>
> The current hash function transition plan envisions a time when we will
> accept input from the user that might be in SHA-1 or in the NewHash
> format.  Since we cannot know which the user has provided, add a
> constant representing the unknown algorithm to allow us to indicate that
> we must look the correct value up.

> +
> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
> +       {
> +               NULL,
> +               0x00000000,
> +               0,
> +               0,
> +               0,
> +               NULL,
> +               NULL,
> +               NULL,
> +               NULL,
> +               NULL,

> Since in the future we want to support an additional hash algorithm, add
> a structure that represents a hash algorithm and all the data that must
> go along with it.  Add a constant to allow easy enumeration of hash
> algorithms.  Implement function typedefs to create an abstract API that
> can be used by any hash algorithm, and wrappers for the existing SHA1
> functions that conform to this API.
> 
> Expose a value for hex size as well as binary size.  While one will
> always be twice the other, the two values are both used extremely
> commonly throughout the codebase and providing both leads to improved
> readability.
> 
> Don't include an entry in the hash algorithm structure for the null
> object ID.  As this value is all zeros, any suitably sized all-zero
> object ID can be used, and there's no need to store a given one on a
> per-hash basis.
> 
> The current hash function transition plan envisions a time when we will
> accept input from the user that might be in SHA-1 or in the NewHash
> format.  Since we cannot know which the user has provided, add a
> constant representing the unknown algorithm to allow us to indicate that
> we must look the correct value up.
> 
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
> I believe I did get the format_id constant for SHA-1 right, but
> sanity-checking would be appreciated.  We don't want another Referer
> mess.
> 
>  cache.h     | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  sha1_file.c | 43 +++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 98 insertions(+)
> 
> diff --git a/cache.h b/cache.h
> index 6440e2bf21..9e9eb08f05 100644
> --- a/cache.h
> +++ b/cache.h

> @@ -77,6 +77,61 @@ struct object_id {
>  	unsigned char hash[GIT_MAX_RAWSZ];
>  };
>  
> +/*
> + * Note that these constants are suitable for indexing the hash_algos array and
> + * comparing against each other, but are otherwise arbitrary, so they should not
> + * be exposed to the user or serialized to disk.  To know whether a
> + * git_hash_algo struct points to some usable hash function, test the format_id
> + * field for being non-zero.  Use the name field for user-visible situations and
> + * the format_id field for fixed-length fields on disk.
> + */
> +/* An unknown hash function. */
> +#define GIT_HASH_UNKNOWN 0
> +/* SHA-1 */
> +#define GIT_HASH_SHA1 1
> +/* Number of algorithms supported (including unknown). */
> +#define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)
> +
> +typedef void (*git_hash_init_fn)(void *ctx);
> +typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);
> +typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);
> +
> +struct git_hash_algo {
> +	/*
> +	 * The name of the algorithm, as appears in the config file and in
> +	 * messages.
> +	 */
> +	const char *name;
> +
> +	/* A four-byte version identifier, used in pack indices. */
> +	uint32_t format_id;
> +
> +	/* The size of a hash context (e.g. git_SHA_CTX). */
> +	size_t ctxsz;
> +
> +	/* The length of the hash in binary. */
> +	size_t rawsz;
> +
> +	/* The length of the hash in hex characters. */
> +	size_t hexsz;
> +
> +	/* The hash initialization function. */
> +	git_hash_init_fn init_fn;
> +
> +	/* The hash update function. */
> +	git_hash_update_fn update_fn;
> +
> +	/* The hash finalization function. */
> +	git_hash_final_fn final_fn;
> +
> +	/* The OID of the empty tree. */
> +	const struct object_id *empty_tree;
> +
> +	/* The OID of the empty blob. */
> +	const struct object_id *empty_blob;
> +};
> +extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];
> +
>  #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)
>  #define DTYPE(de)	((de)->d_type)
>  #else
> diff --git a/sha1_file.c b/sha1_file.c
> index 10c3a0083d..77b320126a 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {
>  	EMPTY_BLOB_SHA1_BIN_LITERAL
>  };
>  
> +static inline void git_hash_sha1_init(void *ctx)
> +{
> +	git_SHA1_Init((git_SHA_CTX *)ctx);
> +}
> +
> +static inline void git_hash_sha1_update(void *ctx, const void *data, size_t len)
> +{
> +	git_SHA1_Update((git_SHA_CTX *)ctx, data, len);
> +}
> +
> +static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)
> +{
> +	git_SHA1_Final(hash, (git_SHA_CTX *)ctx);
> +}
> +
> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {
> +	{
> +		NULL,
> +		0x00000000,
> +		0,
> +		0,
> +		0,
> +		NULL,
> +		NULL,
> +		NULL,
> +		NULL,
> +		NULL,
> +	},
> +	{
> +		"sha-1",
> +		/* "sha1", big-endian */
> +		0x73686131,
> +		sizeof(git_SHA_CTX),
> +		GIT_SHA1_RAWSZ,
> +		GIT_SHA1_HEXSZ,
> +		git_hash_sha1_init,
> +		git_hash_sha1_update,
> +		git_hash_sha1_final,
> +		&empty_tree_oid,
> +		&empty_blob_oid,
> +	},
> +};
> +
>  /*
>   * This is meant to hold a *small* number of objects that you would
>   * want read_sha1_file() to be able to return, but yet you do not want
> -- 
> 2.15.0.rc2
> 

> On 10/28, brian m. carlson wrote:
> > Since in the future we want to support an additional hash algorithm, add
> > a structure that represents a hash algorithm and all the data that must
> > go along with it.  Add a constant to allow easy enumeration of hash
> > algorithms.  Implement function typedefs to create an abstract API that
> > can be used by any hash algorithm, and wrappers for the existing SHA1
> > functions that conform to this API.
> > 
> > Expose a value for hex size as well as binary size.  While one will
> > always be twice the other, the two values are both used extremely
> > commonly throughout the codebase and providing both leads to improved
> > readability.
> > 
> > Don't include an entry in the hash algorithm structure for the null
> > object ID.  As this value is all zeros, any suitably sized all-zero
> > object ID can be used, and there's no need to store a given one on a
> > per-hash basis.
> > 
> > The current hash function transition plan envisions a time when we will
> > accept input from the user that might be in SHA-1 or in the NewHash
> > format.  Since we cannot know which the user has provided, add a
> > constant representing the unknown algorithm to allow us to indicate that
> > we must look the correct value up.
> > 
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> > I believe I did get the format_id constant for SHA-1 right, but
> > sanity-checking would be appreciated.  We don't want another Referer
> > mess.
> > 
> >  cache.h     | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++
> >  sha1_file.c | 43 +++++++++++++++++++++++++++++++++++++++++++
> >  2 files changed, 98 insertions(+)
> > 
> > diff --git a/cache.h b/cache.h
> > index 6440e2bf21..9e9eb08f05 100644
> > --- a/cache.h
> > +++ b/cache.h
> 
> Maybe it would be good to place this interface in its own header file
> that way we can avoid cluttering cache.h with more stuff?

> This is a series proposing a basic abstraction for hash functions.
>
> As we get closer to converting the remainder of the codebase to use
> struct object_id, we should think about the design we want our hash
> function abstraction to take.  This series is a proposal for one idea.
> Input on any aspect of this proposal is welcome.

> This series exposes a struct git_hash_algo that contains basic
> information about a given hash algorithm that distinguishes it from
> other algorithms: name, identifiers, lengths, implementing functions,
> and empty tree and blob constants.  It also exposes an array of hash
> algorithms, and a constant for indexing them.
>
> The series also demonstrates a simple conversion using the abstraction
> over empty blob and tree values.

>  Furthermore, if we end up needing to stuff an algorithm
> value into struct object_id, we'll no longer be able to directly
> reference object IDs in a pack without a copy.
>
> I've updated this series in some significant ways to reflect and better
> implement the transition plan as it's developed.  If there are ways
> in which this series (or future series) can converge better on the
> transition plan, that input would be valuable.
>
> This series is available from the usual places as branch hash-struct,
> based against master as of 2.15-rc2.