{"thread":{"id":"47059","subject":"[PATCH v2 0/4] Hash Abstraction","startedAt":"2017-10-28T18:16:07Z","lastAt":"2017-11-01T01:36:01Z","messageCount":19,"participants":["brian m. carlson","Eric Sunshine","Junio C Hamano","Stefan Beller","Brandon Williams"],"isPatch":true,"patchVersion":2,"patchTotal":4},"messages":[{"id":"331238","messageId":"20171028181239.59458-1-sandals@crustytoothpaste.net","threadId":"47059","inReplyTo":null,"subject":"[PATCH v2 0/4] Hash Abstraction","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-28T18:12:35Z","receivedAt":"2017-10-28T18:16:07Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"This is a series proposing a basic abstraction for hash functions.\n\nAs we get closer to converting the remainder of the codebase to use\nstruct object_id, we should think about the design we want our hash\nfunction abstraction to take. This series is a proposal for one idea.\nInput on any aspect of this proposal is welcome.\n\nThis series exposes a struct git_hash_algo that contains basic\ninformation about a given hash algorithm that distinguishes it from\nother algorithms: name, identifiers, lengths, implementing functions,\nand empty tree and blob constants. It also exposes an array of hash\nalgorithms, and a constant for indexing them.\n\nThe series also demonstrates a simple conversion using the abstraction\nover empty blob and tree values.\n\nIn order to avoid conflicting with the struct repository work and with\nthe goal of avoiding global variables as much as possible, I've pushed\nthe hash algorithm into struct repository and exposed it via a #define.\n\nI propose this series now as it will inform the way we go about\nconverting other parts of the codebase, especially some of the pack\nalgorithms. Because we share some hash computation code between pack\nchecksums and object hashing, we need to decide whether to expose pack\nchecksums as struct object_id, even though they are technically not\nobject IDs. Furthermore, if we end up needing to stuff an algorithm\nvalue into struct object_id, we'll no longer be able to directly\nreference object IDs in a pack without a copy.\n\nI've updated this series in some significant ways to reflect and better\nimplement the transition plan as it's developed. If there are ways\nin which this series (or future series) can converge better on the\ntransition plan, that input would be valuable.\n\nThis series is available from the usual places as branch hash-struct,\nbased against master as of 2.15-rc2.\n\nChanges from v1:\n* Rebase onto 2.15-rc2.\n* Fix the uninitialized value that Peff pointed out. This fixes the\n error, but leaves the code in the same place, since I think it's where\n it should be.\n* Improve commit message to explain the meaning of current_hash WRT the\n transition plan.\n* Added an unknown hash algorithm constant and value to better implement\n the transition plan.\n* Explain in the commit message why hex size and binary size are both\n provided.\n* Add a format_id field to the struct, in coordination with the\n transition plan.\n* Improve comments for struct fields and constants.\n\nbrian m. carlson (4):\n setup: expose enumerated repo info\n Add structure representing hash algorithm\n Integrate hash algorithm support with repo setup\n Switch empty tree and blob lookups to use hash abstraction\n\n builtin/am.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/diff.c | 2 +-\n builtin/pull.c | 2 +-\n cache.h | 67 ++++++++++++++++++++++++++++++++++++++++++++++++++----\n diff-lib.c | 2 +-\n merge-recursive.c | 2 +-\n notes-merge.c | 2 +-\n repository.c | 7 ++++++\n repository.h | 5 ++++\n sequencer.c | 6 ++---\n setup.c | 49 ++++++++++++++++++++++-----------------\n sha1_file.c | 43 +++++++++++++++++++++++++++++++++++\n submodule.c | 2 +-\n 14 files changed, 157 insertions(+), 36 deletions(-)\n\n-- \n2.15.0.rc2\n\n"},{"id":"331239","messageId":"20171028181239.59458-2-sandals@crustytoothpaste.net","threadId":"47059","inReplyTo":"20171028181239.59458-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 1/4] setup: expose enumerated repo info","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-28T18:12:36Z","receivedAt":"2017-10-28T18:16:09Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"We enumerate several different items as part of struct\nrepository_format, but then actually set up those values using the\nglobal variables we've initialized from them. Instead, let's pass a\npointer to the structure down to the code where we enumerate these\nvalues, so we can later on use those values directly to perform setup.\n\nThis technique makes it easier for us to determine additional items\nabout the repository format (such as the hash algorithm) and then use\nthem for setup later on, without needing to add additional global\nvariables. We can't avoid using the existing global variables since\nthey're intricately intertwined with how things work at the moment, but\nthis improves things for the future.\n\nSigned-off-by: brian m. carlson \n---\n setup.c | 46 +++++++++++++++++++++++++---------------------\n 1 file changed, 25 insertions(+), 21 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex 03f51e056c..f0f509fe85 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -432,16 +432,15 @@ static int check_repo_format(const char *var, const char *value, void *vdata)\n \treturn 0;\n }\n \n-static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n+static int check_repository_format_gently(const char *gitdir, struct repository_format *candidate, int *nongit_ok)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \tstruct strbuf err = STRBUF_INIT;\n-\tstruct repository_format candidate;\n \tint has_common;\n \n \thas_common = get_common_dir(&sb, gitdir);\n \tstrbuf_addstr(&sb, \"/config\");\n-\tread_repository_format(&candidate, sb.buf);\n+\tread_repository_format(candidate, sb.buf);\n \tstrbuf_release(&sb);\n \n \t/*\n@@ -449,10 +448,10 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n \t * we treat a missing config as a silent \"ok\", even when nongit_ok\n \t * is unset.\n \t */\n-\tif (candidate.version < 0)\n+\tif (candidate->version < 0)\n \t\treturn 0;\n \n-\tif (verify_repository_format(&candidate, &err) < 0) {\n+\tif (verify_repository_format(candidate, &err) < 0) {\n \t\tif (nongit_ok) {\n \t\t\twarning(\"%s\", err.buf);\n \t\t\tstrbuf_release(&err);\n@@ -462,21 +461,21 @@ static int check_repository_format_gently(const char *gitdir, int *nongit_ok)\n \t\tdie(\"%s\", err.buf);\n \t}\n \n-\trepository_format_precious_objects = candidate.precious_objects;\n-\tstring_list_clear(&candidate.unknown_extensions, 0);\n+\trepository_format_precious_objects = candidate->precious_objects;\n+\tstring_list_clear(&candidate->unknown_extensions, 0);\n \tif (!has_common) {\n-\t\tif (candidate.is_bare != -1) {\n-\t\t\tis_bare_repository_cfg = candidate.is_bare;\n+\t\tif (candidate->is_bare != -1) {\n+\t\t\tis_bare_repository_cfg = candidate->is_bare;\n \t\t\tif (is_bare_repository_cfg == 1)\n \t\t\t\tinside_work_tree = -1;\n \t\t}\n-\t\tif (candidate.work_tree) {\n+\t\tif (candidate->work_tree) {\n \t\t\tfree(git_work_tree_cfg);\n-\t\t\tgit_work_tree_cfg = candidate.work_tree;\n+\t\t\tgit_work_tree_cfg = candidate->work_tree;\n \t\t\tinside_work_tree = -1;\n \t\t}\n \t} else {\n-\t\tfree(candidate.work_tree);\n+\t\tfree(candidate->work_tree);\n \t}\n \n \treturn 0;\n@@ -623,6 +622,7 @@ const char *read_gitfile_gently(const char *path, int *return_error_code)\n \n static const char *setup_explicit_git_dir(const char *gitdirenv,\n \t\t\t\t\t struct strbuf *cwd,\n+\t\t\t\t\t struct repository_format *repo_fmt,\n \t\t\t\t\t int *nongit_ok)\n {\n \tconst char *work_tree_env = getenv(GIT_WORK_TREE_ENVIRONMENT);\n@@ -648,7 +648,7 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \t\tdie(\"Not a git repository: '%s'\", gitdirenv);\n \t}\n \n-\tif (check_repository_format_gently(gitdirenv, nongit_ok)) {\n+\tif (check_repository_format_gently(gitdirenv, repo_fmt, nongit_ok)) {\n \t\tfree(gitfile);\n \t\treturn NULL;\n \t}\n@@ -721,9 +721,10 @@ static const char *setup_explicit_git_dir(const char *gitdirenv,\n \n static const char *setup_discovered_git_dir(const char *gitdir,\n \t\t\t\t\t struct strbuf *cwd, int offset,\n+\t\t\t\t\t struct repository_format *repo_fmt,\n \t\t\t\t\t int *nongit_ok)\n {\n-\tif (check_repository_format_gently(gitdir, nongit_ok))\n+\tif (check_repository_format_gently(gitdir, repo_fmt, nongit_ok))\n \t\treturn NULL;\n \n \t/* --work-tree is set without --git-dir; use discovered one */\n@@ -735,7 +736,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n \t\t\tgitdir = to_free = real_pathdup(gitdir, 1);\n \t\tif (chdir(cwd->buf))\n \t\t\tdie_errno(\"Could not come back to cwd\");\n-\t\tret = setup_explicit_git_dir(gitdir, cwd, nongit_ok);\n+\t\tret = setup_explicit_git_dir(gitdir, cwd, repo_fmt, nongit_ok);\n \t\tfree(to_free);\n \t\treturn ret;\n \t}\n@@ -767,11 +768,12 @@ static const char *setup_discovered_git_dir(const char *gitdir,\n \n /* #16.1, #17.1, #20.1, #21.1, #22.1 (see t1510) */\n static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n+\t\t\t\t struct repository_format *repo_fmt,\n \t\t\t\t int *nongit_ok)\n {\n \tint root_len;\n \n-\tif (check_repository_format_gently(\".\", nongit_ok))\n+\tif (check_repository_format_gently(\".\", repo_fmt, nongit_ok))\n \t\treturn NULL;\n \n \tsetenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, \"0\", 1);\n@@ -783,7 +785,7 @@ static const char *setup_bare_git_dir(struct strbuf *cwd, int offset,\n \t\tgitdir = offset == cwd->len ? \".\" : xmemdupz(cwd->buf, offset);\n \t\tif (chdir(cwd->buf))\n \t\t\tdie_errno(\"Could not come back to cwd\");\n-\t\treturn setup_explicit_git_dir(gitdir, cwd, nongit_ok);\n+\t\treturn setup_explicit_git_dir(gitdir, cwd, repo_fmt, nongit_ok);\n \t}\n \n \tinside_git_dir = 1;\n@@ -1024,6 +1026,7 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tstatic struct strbuf cwd = STRBUF_INIT;\n \tstruct strbuf dir = STRBUF_INIT, gitdir = STRBUF_INIT;\n \tconst char *prefix;\n+\tstruct repository_format repo_fmt;\n \n \t/*\n \t * We may have read an incomplete configuration before\n@@ -1051,18 +1054,18 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\tprefix = NULL;\n \t\tbreak;\n \tcase GIT_DIR_EXPLICIT:\n-\t\tprefix = setup_explicit_git_dir(gitdir.buf, &cwd, nongit_ok);\n+\t\tprefix = setup_explicit_git_dir(gitdir.buf, &cwd, &repo_fmt, nongit_ok);\n \t\tbreak;\n \tcase GIT_DIR_DISCOVERED:\n \t\tif (dir.len < cwd.len && chdir(dir.buf))\n \t\t\tdie(_(\"Cannot change to '%s'\"), dir.buf);\n \t\tprefix = setup_discovered_git_dir(gitdir.buf, &cwd, dir.len,\n-\t\t\t\t\t\t nongit_ok);\n+\t\t\t\t\t\t &repo_fmt, nongit_ok);\n \t\tbreak;\n \tcase GIT_DIR_BARE:\n \t\tif (dir.len < cwd.len && chdir(dir.buf))\n \t\t\tdie(_(\"Cannot change to '%s'\"), dir.buf);\n-\t\tprefix = setup_bare_git_dir(&cwd, dir.len, nongit_ok);\n+\t\tprefix = setup_bare_git_dir(&cwd, dir.len, &repo_fmt, nongit_ok);\n \t\tbreak;\n \tcase GIT_DIR_HIT_CEILING:\n \t\tprefix = setup_nongit(cwd.buf, nongit_ok);\n@@ -1169,7 +1172,8 @@ int git_config_perm(const char *var, const char *value)\n \n void check_repository_format(void)\n {\n-\tcheck_repository_format_gently(get_git_dir(), NULL);\n+\tstruct repository_format repo_fmt;\n+\tcheck_repository_format_gently(get_git_dir(), &repo_fmt, NULL);\n \tstartup_info->have_repository = 1;\n }\n \n-- \n2.15.0.rc2\n\n"},{"id":"331240","messageId":"20171028181239.59458-4-sandals@crustytoothpaste.net","threadId":"47059","inReplyTo":"20171028181239.59458-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-28T18:12:38Z","receivedAt":"2017-10-28T18:16:12Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"In future versions of Git, we plan to support an additional hash\nalgorithm. Integrate the enumeration of hash algorithms with repository\nsetup, and store a pointer to the enumerated data in struct repository.\nOf course, we currently only support SHA-1, so hard-code this value in\nread_repository_format. In the future, we'll enumerate this value from\nthe configuration.\n\nAdd a constant, current_hash, which points to the hash_algo structure\npointer in the repository global. Note that this is the hash which is\nused to serialize data to disk, not the hash which is used to display\nitems to the user. The transition plan anticipates that these may be\ndifferent. We can add an additional element in the future (say,\nui_hash_algo) to provide for this case.\n\nInclude repository.h in cache.h since we now need to have access to\nthese struct and variable definitions.\n\nSigned-off-by: brian m. carlson \n---\n cache.h | 4 ++++\n repository.c | 7 +++++++\n repository.h | 5 +++++\n setup.c | 3 +++\n 4 files changed, 19 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 9e9eb08f05..bce57c74c4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -14,6 +14,7 @@\n #include \"hash.h\"\n #include \"path.h\"\n #include \"sha1-array.h\"\n+#include \"repository.h\"\n \n #ifndef platform_SHA_CTX\n /*\n@@ -132,6 +133,8 @@ struct git_hash_algo {\n };\n extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];\n \n+#define current_hash the_repository->hash_algo\n+\n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\n #else\n@@ -920,6 +923,7 @@ struct repository_format {\n \tint version;\n \tint precious_objects;\n \tint is_bare;\n+\tint hash_algo;\n \tchar *work_tree;\n \tstruct string_list unknown_extensions;\n };\ndiff --git a/repository.c b/repository.c\nindex bb2fae5446..c63bff05a5 100644\n--- a/repository.c\n+++ b/repository.c\n@@ -64,6 +64,11 @@ void repo_set_gitdir(struct repository *repo, const char *path)\n \tfree(old_gitdir);\n }\n \n+void repo_set_hash_algo(struct repository *repo, int hash_algo)\n+{\n+\trepo->hash_algo = &hash_algos[hash_algo];\n+}\n+\n /*\n * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.\n * Return 0 upon success and a non-zero value upon failure.\n@@ -136,6 +141,8 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)\n \tif (read_and_verify_repository_format(&format, repo->commondir))\n \t\tgoto error;\n \n+\trepo->hash_algo = &hash_algos[format.hash_algo];\n+\n \tif (worktree)\n \t\trepo_set_worktree(repo, worktree);\n \ndiff --git a/repository.h b/repository.h\nindex 7f5e24a0a2..0329e40c7f 100644\n--- a/repository.h\n+++ b/repository.h\n@@ -4,6 +4,7 @@\n struct config_set;\n struct index_state;\n struct submodule_cache;\n+struct git_hash_algo;\n \n struct repository {\n \t/* Environment */\n@@ -67,6 +68,9 @@ struct repository {\n \t */\n \tstruct index_state *index;\n \n+\t/* Repository's current hash algorithm, as serialized on disk. */\n+\tconst struct git_hash_algo *hash_algo;\n+\n \t/* Configurations */\n \t/*\n \t * Bit used during initialization to indicate if repository state (like\n@@ -86,6 +90,7 @@ extern struct repository *the_repository;\n \n extern void repo_set_gitdir(struct repository *repo, const char *path);\n extern void repo_set_worktree(struct repository *repo, const char *path);\n+extern void repo_set_hash_algo(struct repository *repo, int algo);\n extern int repo_init(struct repository *repo, const char *gitdir, const char *worktree);\n extern int repo_submodule_init(struct repository *submodule,\n \t\t\t struct repository *superproject,\ndiff --git a/setup.c b/setup.c\nindex f0f509fe85..d99f8a82ff 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -486,6 +486,7 @@ int read_repository_format(struct repository_format *format, const char *path)\n \tmemset(format, 0, sizeof(*format));\n \tformat->version = -1;\n \tformat->is_bare = -1;\n+\tformat->hash_algo = GIT_HASH_SHA1;\n \tstring_list_init(&format->unknown_extensions, 1);\n \tgit_config_from_file(check_repo_format, path, format);\n \treturn format->version;\n@@ -1111,6 +1112,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \t\t\trepo_set_gitdir(the_repository, gitdir);\n \t\t\tsetup_git_env();\n \t\t}\n+\t\tif (startup_info->have_repository)\n+\t\t\trepo_set_hash_algo(the_repository, repo_fmt.hash_algo);\n \t}\n \n \tstrbuf_release(&dir);\n-- \n2.15.0.rc2\n\n"},{"id":"331241","messageId":"20171028181239.59458-3-sandals@crustytoothpaste.net","threadId":"47059","inReplyTo":"20171028181239.59458-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 2/4] Add structure representing hash algorithm","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-28T18:12:37Z","receivedAt":"2017-10-28T18:16:14Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Since in the future we want to support an additional hash algorithm, add\na structure that represents a hash algorithm and all the data that must\ngo along with it. Add a constant to allow easy enumeration of hash\nalgorithms. Implement function typedefs to create an abstract API that\ncan be used by any hash algorithm, and wrappers for the existing SHA1\nfunctions that conform to this API.\n\nExpose a value for hex size as well as binary size. While one will\nalways be twice the other, the two values are both used extremely\ncommonly throughout the codebase and providing both leads to improved\nreadability.\n\nDon't include an entry in the hash algorithm structure for the null\nobject ID. As this value is all zeros, any suitably sized all-zero\nobject ID can be used, and there's no need to store a given one on a\nper-hash basis.\n\nThe current hash function transition plan envisions a time when we will\naccept input from the user that might be in SHA-1 or in the NewHash\nformat. Since we cannot know which the user has provided, add a\nconstant representing the unknown algorithm to allow us to indicate that\nwe must look the correct value up.\n\nSigned-off-by: brian m. carlson \n---\nI believe I did get the format_id constant for SHA-1 right, but\nsanity-checking would be appreciated. We don't want another Referer\nmess.\n\n cache.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n sha1_file.c | 43 +++++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 98 insertions(+)\n\ndiff --git a/cache.h b/cache.h\nindex 6440e2bf21..9e9eb08f05 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -77,6 +77,61 @@ struct object_id {\n \tunsigned char hash[GIT_MAX_RAWSZ];\n };\n \n+/*\n+ * Note that these constants are suitable for indexing the hash_algos array and\n+ * comparing against each other, but are otherwise arbitrary, so they should not\n+ * be exposed to the user or serialized to disk. To know whether a\n+ * git_hash_algo struct points to some usable hash function, test the format_id\n+ * field for being non-zero. Use the name field for user-visible situations and\n+ * the format_id field for fixed-length fields on disk.\n+ */\n+/* An unknown hash function. */\n+#define GIT_HASH_UNKNOWN 0\n+/* SHA-1 */\n+#define GIT_HASH_SHA1 1\n+/* Number of algorithms supported (including unknown). */\n+#define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)\n+\n+typedef void (*git_hash_init_fn)(void *ctx);\n+typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);\n+typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);\n+\n+struct git_hash_algo {\n+\t/*\n+\t * The name of the algorithm, as appears in the config file and in\n+\t * messages.\n+\t */\n+\tconst char *name;\n+\n+\t/* A four-byte version identifier, used in pack indices. */\n+\tuint32_t format_id;\n+\n+\t/* The size of a hash context (e.g. git_SHA_CTX). */\n+\tsize_t ctxsz;\n+\n+\t/* The length of the hash in binary. */\n+\tsize_t rawsz;\n+\n+\t/* The length of the hash in hex characters. */\n+\tsize_t hexsz;\n+\n+\t/* The hash initialization function. */\n+\tgit_hash_init_fn init_fn;\n+\n+\t/* The hash update function. */\n+\tgit_hash_update_fn update_fn;\n+\n+\t/* The hash finalization function. */\n+\tgit_hash_final_fn final_fn;\n+\n+\t/* The OID of the empty tree. */\n+\tconst struct object_id *empty_tree;\n+\n+\t/* The OID of the empty blob. */\n+\tconst struct object_id *empty_blob;\n+};\n+extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];\n+\n #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n #define DTYPE(de)\t((de)->d_type)\n #else\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 10c3a0083d..77b320126a 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {\n \tEMPTY_BLOB_SHA1_BIN_LITERAL\n };\n \n+static inline void git_hash_sha1_init(void *ctx)\n+{\n+\tgit_SHA1_Init((git_SHA_CTX *)ctx);\n+}\n+\n+static inline void git_hash_sha1_update(void *ctx, const void *data, size_t len)\n+{\n+\tgit_SHA1_Update((git_SHA_CTX *)ctx, data, len);\n+}\n+\n+static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)\n+{\n+\tgit_SHA1_Final(hash, (git_SHA_CTX *)ctx);\n+}\n+\n+const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n+\t{\n+\t\tNULL,\n+\t\t0x00000000,\n+\t\t0,\n+\t\t0,\n+\t\t0,\n+\t\tNULL,\n+\t\tNULL,\n+\t\tNULL,\n+\t\tNULL,\n+\t\tNULL,\n+\t},\n+\t{\n+\t\t\"sha-1\",\n+\t\t/* \"sha1\", big-endian */\n+\t\t0x73686131,\n+\t\tsizeof(git_SHA_CTX),\n+\t\tGIT_SHA1_RAWSZ,\n+\t\tGIT_SHA1_HEXSZ,\n+\t\tgit_hash_sha1_init,\n+\t\tgit_hash_sha1_update,\n+\t\tgit_hash_sha1_final,\n+\t\t&empty_tree_oid,\n+\t\t&empty_blob_oid,\n+\t},\n+};\n+\n /*\n * This is meant to hold a *small* number of objects that you would\n * want read_sha1_file() to be able to return, but yet you do not want\n-- \n2.15.0.rc2\n\n"},{"id":"331242","messageId":"20171028181239.59458-5-sandals@crustytoothpaste.net","threadId":"47059","inReplyTo":"20171028181239.59458-1-sandals@crustytoothpaste.net","subject":"[PATCH v2 4/4] Switch empty tree and blob lookups to use hash abstraction","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-28T18:12:39Z","receivedAt":"2017-10-28T18:16:18Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"Switch the uses of empty_tree_oid and empty_blob_oid to use the\ncurrent_hash abstraction that represents the current hash algorithm in\nuse.\n\nSigned-off-by: brian m. carlson \n---\n builtin/am.c | 2 +-\n builtin/checkout.c | 2 +-\n builtin/diff.c | 2 +-\n builtin/pull.c | 2 +-\n cache.h | 8 ++++----\n diff-lib.c | 2 +-\n merge-recursive.c | 2 +-\n notes-merge.c | 2 +-\n sequencer.c | 6 +++---\n submodule.c | 2 +-\n 10 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/am.c b/builtin/am.c\nindex d7513f5375..7f71974b79 100644\n--- a/builtin/am.c\n+++ b/builtin/am.c\n@@ -1433,7 +1433,7 @@ static void write_index_patch(const struct am_state *state)\n \tif (!get_oid_tree(\"HEAD\", &head))\n \t\ttree = lookup_tree(&head);\n \telse\n-\t\ttree = lookup_tree(&empty_tree_oid);\n+\t\ttree = lookup_tree(current_hash->empty_tree);\n \n \tfp = xfopen(am_path(state, \"patch\"), \"w\");\n \tinit_revisions(&rev_info, NULL);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex fc4f8fd2ea..e7878fa62e 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -516,7 +516,7 @@ static int merge_working_tree(const struct checkout_opts *opts,\n \t\t}\n \t\ttree = parse_tree_indirect(old->commit ?\n \t\t\t\t\t &old->commit->object.oid :\n-\t\t\t\t\t &empty_tree_oid);\n+\t\t\t\t\t current_hash->empty_tree);\n \t\tinit_tree_desc(&trees[0], tree->buffer, tree->size);\n \t\ttree = parse_tree_indirect(&new->commit->object.oid);\n \t\tinit_tree_desc(&trees[1], tree->buffer, tree->size);\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex f5bbd4d757..2419de1770 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -380,7 +380,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t\t\tadd_head_to_pending(&rev);\n \t\t\t\tif (!rev.pending.nr) {\n \t\t\t\t\tstruct tree *tree;\n-\t\t\t\t\ttree = lookup_tree(&empty_tree_oid);\n+\t\t\t\t\ttree = lookup_tree(current_hash->empty_tree);\n \t\t\t\t\tadd_pending_object(&rev, &tree->object, \"HEAD\");\n \t\t\t\t}\n \t\t\t\tbreak;\ndiff --git a/builtin/pull.c b/builtin/pull.c\nindex 6f772e8a22..7bb96cb5a0 100644\n--- a/builtin/pull.c\n+++ b/builtin/pull.c\n@@ -541,7 +541,7 @@ static int pull_into_void(const struct object_id *merge_head,\n \t * index/worktree changes that the user already made on the unborn\n \t * branch.\n \t */\n-\tif (checkout_fast_forward(&empty_tree_oid, merge_head, 0))\n+\tif (checkout_fast_forward(current_hash->empty_tree, merge_head, 0))\n \t\treturn 1;\n \n \tif (update_ref(\"initial pull\", \"HEAD\", merge_head->hash, curr_head->hash, 0, UPDATE_REFS_DIE_ON_ERR))\ndiff --git a/cache.h b/cache.h\nindex bce57c74c4..28906468fa 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1056,22 +1056,22 @@ extern const struct object_id empty_blob_oid;\n \n static inline int is_empty_blob_sha1(const unsigned char *sha1)\n {\n-\treturn !hashcmp(sha1, EMPTY_BLOB_SHA1_BIN);\n+\treturn !hashcmp(sha1, current_hash->empty_blob->hash);\n }\n \n static inline int is_empty_blob_oid(const struct object_id *oid)\n {\n-\treturn !hashcmp(oid->hash, EMPTY_BLOB_SHA1_BIN);\n+\treturn !oidcmp(oid, current_hash->empty_blob);\n }\n \n static inline int is_empty_tree_sha1(const unsigned char *sha1)\n {\n-\treturn !hashcmp(sha1, EMPTY_TREE_SHA1_BIN);\n+\treturn !hashcmp(sha1, current_hash->empty_tree->hash);\n }\n \n static inline int is_empty_tree_oid(const struct object_id *oid)\n {\n-\treturn !hashcmp(oid->hash, EMPTY_TREE_SHA1_BIN);\n+\treturn !oidcmp(oid, current_hash->empty_tree);\n }\n \n /* set default permissions by passing mode arguments to open(2) */\ndiff --git a/diff-lib.c b/diff-lib.c\nindex 4e0980caa8..b5d95ea5c3 100644\n--- a/diff-lib.c\n+++ b/diff-lib.c\n@@ -216,7 +216,7 @@ int run_diff_files(struct rev_info *revs, unsigned int option)\n \t\t\t} else if (revs->diffopt.ita_invisible_in_index &&\n \t\t\t\t ce_intent_to_add(ce)) {\n \t\t\t\tdiff_addremove(&revs->diffopt, '+', ce->ce_mode,\n-\t\t\t\t\t &empty_tree_oid, 0,\n+\t\t\t\t\t current_hash->empty_tree, 0,\n \t\t\t\t\t ce->name, 0);\n \t\t\t\tcontinue;\n \t\t\t}\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex 1d3f8f0d22..4547e15b8c 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -2081,7 +2081,7 @@ int merge_recursive(struct merge_options *o,\n \t\t/* if there is no common ancestor, use an empty tree */\n \t\tstruct tree *tree;\n \n-\t\ttree = lookup_tree(&empty_tree_oid);\n+\t\ttree = lookup_tree(current_hash->empty_tree);\n \t\tmerged_common_ancestors = make_virtual_commit(tree, \"ancestor\");\n \t}\n \ndiff --git a/notes-merge.c b/notes-merge.c\nindex 4352c34a6e..aab27121cb 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -595,7 +595,7 @@ int notes_merge(struct notes_merge_options *o,\n \tbases = get_merge_bases(local, remote);\n \tif (!bases) {\n \t\tbase_oid = &null_oid;\n-\t\tbase_tree_oid = &empty_tree_oid;\n+\t\tbase_tree_oid = current_hash->empty_tree;\n \t\tif (o->verbosity >= 4)\n \t\t\tprintf(\"No merge base found; doing history-less merge\\n\");\n \t} else if (!bases->next) {\ndiff --git a/sequencer.c b/sequencer.c\nindex f2a10cc4f2..b910165a82 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -347,7 +347,7 @@ static int read_oneliner(struct strbuf *buf,\n \n static struct tree *empty_tree(void)\n {\n-\treturn lookup_tree(&empty_tree_oid);\n+\treturn lookup_tree(current_hash->empty_tree);\n }\n \n static int error_dirty_index(struct replay_opts *opts)\n@@ -705,7 +705,7 @@ static int is_original_commit_empty(struct commit *commit)\n \t\t\t\toid_to_hex(&parent->object.oid));\n \t\tptree_oid = &parent->tree->object.oid;\n \t} else {\n-\t\tptree_oid = &empty_tree_oid; /* commit is root */\n+\t\tptree_oid = current_hash->empty_tree; /* commit is root */\n \t}\n \n \treturn !oidcmp(ptree_oid, &commit->tree->object.oid);\n@@ -958,7 +958,7 @@ static int do_pick_commit(enum todo_command command, struct commit *commit,\n \t} else {\n \t\tunborn = get_oid(\"HEAD\", &head);\n \t\tif (unborn)\n-\t\t\toidcpy(&head, &empty_tree_oid);\n+\t\t\toidcpy(&head, current_hash->empty_tree);\n \t\tif (index_differs_from(unborn ? EMPTY_TREE_SHA1_HEX : \"HEAD\", 0, 0))\n \t\t\treturn error_dirty_index(opts);\n \t}\ndiff --git a/submodule.c b/submodule.c\nindex 63e7094e16..218cbf6227 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -587,7 +587,7 @@ void show_submodule_inline_diff(struct diff_options *o, const char *path,\n \t\tstruct object_id *one, struct object_id *two,\n \t\tunsigned dirty_submodule)\n {\n-\tconst struct object_id *old = &empty_tree_oid, *new = &empty_tree_oid;\n+\tconst struct object_id *old = current_hash->empty_tree, *new = current_hash->empty_tree;\n \tstruct commit *left = NULL, *right = NULL;\n \tstruct commit_list *merge_bases = NULL;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-- \n2.15.0.rc2\n\n"},{"id":"331253","messageId":"CAPig+cSX_+mN8Pz=0k3xnHRa-Teoq2JrSHe0zQhzJ1NZafXPoQ@mail.gmail.com","threadId":"47059","inReplyTo":"20171028181239.59458-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/4] Add structure representing hash algorithm","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-29T01:36:05Z","receivedAt":"2017-10-29T01:37:53Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 28, 2017 at 2:12 PM, brian m. carlson\n wrote:\n> Since in the future we want to support an additional hash algorithm, add\n> a structure that represents a hash algorithm and all the data that must\n> go along with it. Add a constant to allow easy enumeration of hash\n> algorithms. Implement function typedefs to create an abstract API that\n> can be used by any hash algorithm, and wrappers for the existing SHA1\n> functions that conform to this API.\n> [...]\n> Signed-off-by: brian m. carlson \n> ---\n> diff --git a/cache.h b/cache.h\n> @@ -77,6 +77,61 @@ struct object_id {\n> +typedef void (*git_hash_init_fn)(void *ctx);\n> +typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);\n> +typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);\n> +\n> +struct git_hash_algo {\n> + [...]\n> + /* The hash initialization function. */\n> + git_hash_init_fn init_fn;\n> +\n> + /* The hash update function. */\n> + git_hash_update_fn update_fn;\n> +\n> + /* The hash finalization function. */\n> + git_hash_final_fn final_fn;\n> + [...]\n> +};\n> diff --git a/sha1_file.c b/sha1_file.c\n> @@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {\n> +static inline void git_hash_sha1_init(void *ctx)\n> +{\n> + git_SHA1_Init((git_SHA_CTX *)ctx);\n> +}\n> +\n> +static inline void git_hash_sha1_update(void *ctx, const void *data, size_t len)\n> +{\n> + git_SHA1_Update((git_SHA_CTX *)ctx, data, len);\n> +}\n> +\n> +static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)\n> +{\n> + git_SHA1_Final(hash, (git_SHA_CTX *)ctx);\n> +}\n\nWhy 'inline' if you only ever take the addresses of these functions?\n\n> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n> + [...]\n> + {\n> + \"sha-1\",\n> + /* \"sha1\", big-endian */\n> + 0x73686131,\n> + sizeof(git_SHA_CTX),\n> + GIT_SHA1_RAWSZ,\n> + GIT_SHA1_HEXSZ,\n> + git_hash_sha1_init,\n> + git_hash_sha1_update,\n> + git_hash_sha1_final,\n> + &empty_tree_oid,\n> + &empty_blob_oid,\n> + },\n> +};\n"},{"id":"331254","messageId":"CAPig+cTrAxWczJ5qX2qE-hqoo6hA2QCom25HYB7v48rVnPgTow@mail.gmail.com","threadId":"47059","inReplyTo":"20171028181239.59458-4-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-29T01:44:07Z","receivedAt":"2017-10-29T01:44:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Oct 28, 2017 at 2:12 PM, brian m. carlson\n wrote:\n> In future versions of Git, we plan to support an additional hash\n> algorithm. Integrate the enumeration of hash algorithms with repository\n> setup, and store a pointer to the enumerated data in struct repository.\n> Of course, we currently only support SHA-1, so hard-code this value in\n> read_repository_format. In the future, we'll enumerate this value from\n> the configuration.\n> [...]\n> Signed-off-by: brian m. carlson \n> ---\n> diff --git a/cache.h b/cache.h\n> @@ -132,6 +133,8 @@ struct git_hash_algo {\n> extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];\n>\n> +#define current_hash the_repository->hash_algo\n\nThe all-lowercase name \"current_hash\" seems likely to conflict with a\nvariable name some day; the fact that it is also a #define makes such\na collision even more worrisome. Although it is retrieving the \"hash\nalgorithm\", when reading the terse name \"current_hash\", one may\ninstead intuitively think it is referring to a hash _value_ (not an\nalgorithm).\n\n> diff --git a/repository.c b/repository.c\n> @@ -64,6 +64,11 @@ void repo_set_gitdir(struct repository *repo, const char *path)\n> +void repo_set_hash_algo(struct repository *repo, int hash_algo)\n> +{\n> + repo->hash_algo = &hash_algos[hash_algo];\n> +}\n> +\n> /*\n> * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.\n> * Return 0 upon success and a non-zero value upon failure.\n> @@ -136,6 +141,8 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)\n> if (read_and_verify_repository_format(&format, repo->commondir))\n> goto error;\n>\n> + repo->hash_algo = &hash_algos[format.hash_algo];\n\nShould this be instead invoking repo_set_hash_algo()?\n\n> if (worktree)\n> repo_set_worktree(repo, worktree);\n"},{"id":"331281","messageId":"20171029170025.3z2mohqgcwhycq43@genre.crustytoothpaste.net","threadId":"47059","inReplyTo":"CAPig+cSX_+mN8Pz=0k3xnHRa-Teoq2JrSHe0zQhzJ1NZafXPoQ@mail.gmail.com","subject":"Re: [PATCH v2 2/4] Add structure representing hash algorithm","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-29T17:00:25Z","receivedAt":"2017-10-29T17:00:36Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Oct 28, 2017 at 09:36:05PM -0400, Eric Sunshine wrote:\n> On Sat, Oct 28, 2017 at 2:12 PM, brian m. carlson\n> wrote:\n> > diff --git a/sha1_file.c b/sha1_file.c\n> > @@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {\n> > +static inline void git_hash_sha1_init(void *ctx)\n> > +{\n> > + git_SHA1_Init((git_SHA_CTX *)ctx);\n> > +}\n> > +\n> > +static inline void git_hash_sha1_update(void *ctx, const void *data, size_t len)\n> > +{\n> > + git_SHA1_Update((git_SHA_CTX *)ctx, data, len);\n> > +}\n> > +\n> > +static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)\n> > +{\n> > + git_SHA1_Final(hash, (git_SHA_CTX *)ctx);\n> > +}\n> \n> Why 'inline' if you only ever take the addresses of these functions?\n\nGood point. I wanted to avoid the overhead of a needless intermediate\nfunction call when using this code path and have the compiler\nessentially emit a call to the underlying functions directly, but inline\nprobably won't affect whether that happens.\n\nI'll remove them.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"331284","messageId":"20171029175712.ijqirnghcgeufqae@genre.crustytoothpaste.net","threadId":"47059","inReplyTo":"CAPig+cTrAxWczJ5qX2qE-hqoo6hA2QCom25HYB7v48rVnPgTow@mail.gmail.com","subject":"Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-29T17:57:13Z","receivedAt":"2017-10-29T17:57:25Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sat, Oct 28, 2017 at 09:44:07PM -0400, Eric Sunshine wrote:\n> > diff --git a/cache.h b/cache.h\n> > @@ -132,6 +133,8 @@ struct git_hash_algo {\n> > extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];\n> >\n> > +#define current_hash the_repository->hash_algo\n> \n> The all-lowercase name \"current_hash\" seems likely to conflict with a\n> variable name some day; the fact that it is also a #define makes such\n> a collision even more worrisome. Although it is retrieving the \"hash\n> algorithm\", when reading the terse name \"current_hash\", one may\n> instead intuitively think it is referring to a hash _value_ (not an\n> algorithm).\n\nI can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's\nan improvement. I originally omitted the \"algo\" portion to keep it\nshort.\n\nAlternatively, we could have a current_hash() (or current_hash_algo())\ninline function if people like that better.\n\n> > * Attempt to resolve and set the provided 'gitdir' for repository 'repo'.\n> > * Return 0 upon success and a non-zero value upon failure.\n> > @@ -136,6 +141,8 @@ int repo_init(struct repository *repo, const char *gitdir, const char *worktree)\n> > if (read_and_verify_repository_format(&format, repo->commondir))\n> > goto error;\n> >\n> > + repo->hash_algo = &hash_algos[format.hash_algo];\n> \n> Should this be instead invoking repo_set_hash_algo()?\n\nYes, it should. Will fix.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"331291","messageId":"CAPig+cRtexDJYMCUR1rGZjJOBRuDbhxP==US2VdNPhSWzeL6LA@mail.gmail.com","threadId":"47059","inReplyTo":"20171029175712.ijqirnghcgeufqae@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2017-10-29T19:02:20Z","receivedAt":"2017-10-29T19:02:26Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Oct 29, 2017 at 1:57 PM, brian m. carlson\n wrote:\n> On Sat, Oct 28, 2017 at 09:44:07PM -0400, Eric Sunshine wrote:\n>> > +#define current_hash the_repository->hash_algo\n>>\n>> The all-lowercase name \"current_hash\" seems likely to conflict with a\n>> variable name some day; the fact that it is also a #define makes such\n>> a collision even more worrisome. Although it is retrieving the \"hash\n>> algorithm\", when reading the terse name \"current_hash\", one may\n>> instead intuitively think it is referring to a hash _value_ (not an\n>> algorithm).\n>\n> I can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's\n> an improvement. I originally omitted the \"algo\" portion to keep it\n> short.\n\nI don't have strong feelings about it aside from worrying about a\n\"current_hash\" name clash or a reader misunderstanding what it\nrepresents.\n\nDoes \"current\" need to be in the name? What about HASH_ALGO or REPO_HASH_ALGO?\n\n> Alternatively, we could have a current_hash() (or current_hash_algo())\n> inline function if people like that better.\n\nhash_algo() or repo_hash_algo()?\n"},{"id":"331292","messageId":"20171029193327.uqkj6w3ypfwqwm7b@genre.crustytoothpaste.net","threadId":"47059","inReplyTo":"CAPig+cRtexDJYMCUR1rGZjJOBRuDbhxP==US2VdNPhSWzeL6LA@mail.gmail.com","subject":"Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-29T19:33:28Z","receivedAt":"2017-10-29T19:33:39Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Sun, Oct 29, 2017 at 03:02:20PM -0400, Eric Sunshine wrote:\n> On Sun, Oct 29, 2017 at 1:57 PM, brian m. carlson\n> wrote:\n> > I can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's\n> > an improvement. I originally omitted the \"algo\" portion to keep it\n> > short.\n> \n> I don't have strong feelings about it aside from worrying about a\n> \"current_hash\" name clash or a reader misunderstanding what it\n> represents.\n> \n> Does \"current\" need to be in the name? What about HASH_ALGO or REPO_HASH_ALGO?\n> \n> > Alternatively, we could have a current_hash() (or current_hash_algo())\n> > inline function if people like that better.\n> \n> hash_algo() or repo_hash_algo()?\n\nThose are also fine, and shorter to boot. I'll wait to see if anyone\nhas strong opinions on the direction we should go before making a\nchange.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"331306","messageId":"xmqq7evd8jsa.fsf@gitster.mtv.corp.google.com","threadId":"47059","inReplyTo":"20171029193327.uqkj6w3ypfwqwm7b@genre.crustytoothpaste.net","subject":"Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-10-30T02:13:41Z","receivedAt":"2017-10-30T02:13:48Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"brian m. carlson\" writes:\n\n> On Sun, Oct 29, 2017 at 03:02:20PM -0400, Eric Sunshine wrote:\n>> On Sun, Oct 29, 2017 at 1:57 PM, brian m. carlson\n>> wrote:\n>> > I can do CURRENT_HASH_ALGO or CURRENT_HASH instead if you think that's\n>> > an improvement. I originally omitted the \"algo\" portion to keep it\n>> > short.\n>> \n>> I don't have strong feelings about it aside from worrying about a\n>> \"current_hash\" name clash or a reader misunderstanding what it\n>> represents.\n>> \n>> Does \"current\" need to be in the name? What about HASH_ALGO or REPO_HASH_ALGO?\n>> \n>> > Alternatively, we could have a current_hash() (or current_hash_algo())\n>> > inline function if people like that better.\n>> \n>> hash_algo() or repo_hash_algo()?\n>\n> Those are also fine, and shorter to boot. I'll wait to see if anyone\n> has strong opinions on the direction we should go before making a\n> change.\n\nIs the plan to allow running with multiple hash algorithms in\nparallel? I thought what we want to see in the future codebase was\nto have the default hash algorithm used for everything except for a\nselect few codepaths, and assumed that the way we achieve it is to\n\n - allow very low level helper functions (e.g. read_sha1_file(),\n write_sha1_file_prepare(), etc.) to take a pointer to the hash\n algorithm structure;\n\n - have higher level helper functions to call these low level\n helpers with a fixed singleton instance of hash algorithm\n structure that represents that default one (SHA2-something?); and\n\n - a few selected codepaths (e.g. index-pack that reads SHA-1 stream\n and converts it into NewHash pack/index while building the object\n name mapping) use an additional singleton instance of hash\n algorithm structure that represents the SHA-1 hash, and the way\n they use it is *NOT* by replacing \"the current\" one with SHA-1,\n but by explicitly passing the instance to the low level helpers\n as parameter.\n\nSo, \"current\" does indeed sound quite wrong, as it makes it sound as\nif you can swap it anytime and ask \"which one is in effect now?\".\nIf we do not want to call the default instance \"SHA-2\" because we\nwant to prepare for migrating again by not having the name of a\nconcrete hash algorithm sprinkled in the codebase all over like we\ncurrently do, why not call the instance \"the_hash_algo\"?\n\n"},{"id":"331320","messageId":"20171030025405.53f4sqxlc53vqake@genre.crustytoothpaste.net","threadId":"47059","inReplyTo":"xmqq7evd8jsa.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-10-30T02:54:05Z","receivedAt":"2017-10-30T02:54:17Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Oct 30, 2017 at 11:13:41AM +0900, Junio C Hamano wrote:\n> Is the plan to allow running with multiple hash algorithms in\n> parallel? I thought what we want to see in the future codebase was\n> to have the default hash algorithm used for everything except for a\n> select few codepaths, and assumed that the way we achieve it is to\n> \n> - allow very low level helper functions (e.g. read_sha1_file(),\n> write_sha1_file_prepare(), etc.) to take a pointer to the hash\n> algorithm structure;\n> \n> - have higher level helper functions to call these low level\n> helpers with a fixed singleton instance of hash algorithm\n> structure that represents that default one (SHA2-something?); and\n> \n> - a few selected codepaths (e.g. index-pack that reads SHA-1 stream\n> and converts it into NewHash pack/index while building the object\n> name mapping) use an additional singleton instance of hash\n> algorithm structure that represents the SHA-1 hash, and the way\n> they use it is *NOT* by replacing \"the current\" one with SHA-1,\n> but by explicitly passing the instance to the low level helpers\n> as parameter.\n\nThis is consistent with my understanding as well.\n\n> So, \"current\" does indeed sound quite wrong, as it makes it sound as\n> if you can swap it anytime and ask \"which one is in effect now?\".\n> If we do not want to call the default instance \"SHA-2\" because we\n> want to prepare for migrating again by not having the name of a\n> concrete hash algorithm sprinkled in the codebase all over like we\n> currently do, why not call the instance \"the_hash_algo\"?\n\nMy original plan was to handle different algorithms in different\nsubmodules, but I think with the current transition plan, that becomes\nunnecessary, since we're going to paper over any differences that might\nshow up. the_hash_algo should be fine, and it's consistent with\nthe_repository and the_index as well.\n\nThanks for a sensible suggestion.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"},{"id":"331358","messageId":"CAGZ79kZdAvP5HWk4XqqUYdkorvEvoWYR32T3frrmnTXetU45Xg@mail.gmail.com","threadId":"47059","inReplyTo":"20171028181239.59458-2-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 1/4] setup: expose enumerated repo info","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-30T16:08:20Z","receivedAt":"2017-10-30T16:08:27Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson\n wrote:\n> We enumerate several different items as part of struct\n> repository_format, but then actually set up those values using the\n> global variables we've initialized from them. Instead, let's pass a\n> pointer to the structure down to the code where we enumerate these\n> values, so we can later on use those values directly to perform setup.\n>\n> This technique makes it easier for us to determine additional items\n> about the repository format (such as the hash algorithm) and then use\n> them for setup later on, without needing to add additional global\n> variables. We can't avoid using the existing global variables since\n> they're intricately intertwined with how things work at the moment, but\n> this improves things for the future.\n\nyup. looks good to me.\n\nThanks,\nStefan\n"},{"id":"331359","messageId":"CAGZ79kb-+DFz2m-8arh+RDJibBMc_9vjTC5ZoXbojBBcThrBdQ@mail.gmail.com","threadId":"47059","inReplyTo":"20171028181239.59458-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/4] Add structure representing hash algorithm","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-30T16:14:07Z","receivedAt":"2017-10-30T16:14:20Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson\n wrote:\n> Since in the future we want to support an additional hash algorithm, add\n> a structure that represents a hash algorithm and all the data that must\n> go along with it. Add a constant to allow easy enumeration of hash\n> algorithms. Implement function typedefs to create an abstract API that\n> can be used by any hash algorithm, and wrappers for the existing SHA1\n> functions that conform to this API.\n>\n> Expose a value for hex size as well as binary size. While one will\n> always be twice the other, the two values are both used extremely\n> commonly throughout the codebase and providing both leads to improved\n> readability.\n>\n> Don't include an entry in the hash algorithm structure for the null\n> object ID. As this value is all zeros, any suitably sized all-zero\n> object ID can be used, and there's no need to store a given one on a\n> per-hash basis.\n>\n> The current hash function transition plan envisions a time when we will\n> accept input from the user that might be in SHA-1 or in the NewHash\n> format. Since we cannot know which the user has provided, add a\n> constant representing the unknown algorithm to allow us to indicate that\n> we must look the correct value up.\n\nCool.\n\n\n> +\n> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n> + {\n> + NULL,\n> + 0x00000000,\n> + 0,\n> + 0,\n> + 0,\n> + NULL,\n> + NULL,\n> + NULL,\n> + NULL,\n> + NULL,\n\nIf we are fancy we could provide an appropriate die() call\nas the function pointers. That way if you call these functions\nby accident, you get a well worded warning instead of a segfault.\n"},{"id":"331361","messageId":"CAGZ79kZZpO6SzGrN=2WKbKijwVCv4Kpj=yHBKN0vkP1UibhE9Q@mail.gmail.com","threadId":"47059","inReplyTo":"20171028181239.59458-4-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 3/4] Integrate hash algorithm support with repo setup","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-30T16:27:27Z","receivedAt":"2017-10-30T16:27:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson\n wrote:\n>\n> Include repository.h in cache.h since we now need to have access to\n> these struct and variable definitions.\n\nLet's see how that works out. I remember having include issues\nin the repository struct series.\n\n> };\n> extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];\n>\n> +#define current_hash the_repository->hash_algo\n> +\n> #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n> #define DTYPE(de) ((de)->d_type)\n> #else\n> @@ -920,6 +923,7 @@ struct repository_format {\n> int version;\n> int precious_objects;\n> int is_bare;\n> + int hash_algo;\n\nI wonder if this (as well as the #defines in patch 2),\nought to be an enum { unknown-hash=0, sha1=1}.\n"},{"id":"331365","messageId":"CAGZ79kbX_6ONL1RK3xPopE8Rg15AinvzVZv3rAwLCXgX15NQ_Q@mail.gmail.com","threadId":"47059","inReplyTo":"20171028181239.59458-1-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 0/4] Hash Abstraction","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-10-30T16:45:38Z","receivedAt":"2017-10-30T16:45:44Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Sat, Oct 28, 2017 at 11:12 AM, brian m. carlson\n wrote:\n> This is a series proposing a basic abstraction for hash functions.\n>\n> As we get closer to converting the remainder of the codebase to use\n> struct object_id, we should think about the design we want our hash\n> function abstraction to take. This series is a proposal for one idea.\n> Input on any aspect of this proposal is welcome.\n\nI like the series/idea.\n\n> This series exposes a struct git_hash_algo that contains basic\n> information about a given hash algorithm that distinguishes it from\n> other algorithms: name, identifiers, lengths, implementing functions,\n> and empty tree and blob constants. It also exposes an array of hash\n> algorithms, and a constant for indexing them.\n>\n> The series also demonstrates a simple conversion using the abstraction\n> over empty blob and tree values.\n\nThat looks good, though I wonder if we'll have to give a way a little\nperformance during the transition period (or even indefinitely, as the\nhash abstraction won't go away; we'd rather want to keep it in place\nlong term). I guess we can measure after all is said and done,\nno big deal.\n\n>\n> In order to avoid conflicting with the struct repository work and with\n> the goal of avoiding global variables as much as possible, I've pushed\n> the hash algorithm into struct repository and exposed it via a #define.\n\nIf you are referring to that long series[1] that Jonathan and I were working\non, then no worries. Unfortunately that series got some delays, but I rebased\nits prepared successors (of over 100 patches) on Friday and it was surprisingly\neasy to do so; just tedious.\n\n[1] https://public-inbox.org/git/20170830064634.GA153983@aiede.mtv.corp.google.com/\n\n>\n> I propose this series now as it will inform the way we go about\n> converting other parts of the codebase, especially some of the pack\n> algorithms.\n\nThanks for doing this now.\n\n> Because we share some hash computation code between pack\n> checksums and object hashing, we need to decide whether to expose pack\n> checksums as struct object_id, even though they are technically not\n> object IDs.\n\nI think we used sha1 as checksums, because it was a hash function easily\naccessible, not because its other properties were the best to do its job.\nSo I am undecided if we want to just \"keep the same hash function for\nchecksum as well as object hashing\" or pickup another checksum, that\nactually *only* does checksumming (we don't need the cryptographic\nproperties, such that an easy hash [such as crc, djb, fnv or murmur]\nwould do; raw speed, barely protecting the pack file against bit flips).\n\nFor the sake of symmetry I tend to go with the former, i.e use the object\nhash function for pack files, too.\n\n> Furthermore, if we end up needing to stuff an algorithm\n> value into struct object_id, we'll no longer be able to directly\n> reference object IDs in a pack without a copy.\n>\n> I've updated this series in some significant ways to reflect and better\n> implement the transition plan as it's developed. If there are ways\n> in which this series (or future series) can converge better on the\n> transition plan, that input would be valuable.\n>\n> This series is available from the usual places as branch hash-struct,\n> based against master as of 2.15-rc2.\n\nThanks,\nStefan\n"},{"id":"331425","messageId":"20171030233615.GA94048@google.com","threadId":"47059","inReplyTo":"20171028181239.59458-3-sandals@crustytoothpaste.net","subject":"Re: [PATCH v2 2/4] Add structure representing hash algorithm","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-10-30T23:36:15Z","receivedAt":"2017-10-30T23:36:24Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 10/28, brian m. carlson wrote:\n> Since in the future we want to support an additional hash algorithm, add\n> a structure that represents a hash algorithm and all the data that must\n> go along with it. Add a constant to allow easy enumeration of hash\n> algorithms. Implement function typedefs to create an abstract API that\n> can be used by any hash algorithm, and wrappers for the existing SHA1\n> functions that conform to this API.\n> \n> Expose a value for hex size as well as binary size. While one will\n> always be twice the other, the two values are both used extremely\n> commonly throughout the codebase and providing both leads to improved\n> readability.\n> \n> Don't include an entry in the hash algorithm structure for the null\n> object ID. As this value is all zeros, any suitably sized all-zero\n> object ID can be used, and there's no need to store a given one on a\n> per-hash basis.\n> \n> The current hash function transition plan envisions a time when we will\n> accept input from the user that might be in SHA-1 or in the NewHash\n> format. Since we cannot know which the user has provided, add a\n> constant representing the unknown algorithm to allow us to indicate that\n> we must look the correct value up.\n> \n> Signed-off-by: brian m. carlson \n> ---\n> I believe I did get the format_id constant for SHA-1 right, but\n> sanity-checking would be appreciated. We don't want another Referer\n> mess.\n> \n> cache.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> sha1_file.c | 43 +++++++++++++++++++++++++++++++++++++++++++\n> 2 files changed, 98 insertions(+)\n> \n> diff --git a/cache.h b/cache.h\n> index 6440e2bf21..9e9eb08f05 100644\n> --- a/cache.h\n> +++ b/cache.h\n\nMaybe it would be good to place this interface in its own header file\nthat way we can avoid cluttering cache.h with more stuff?\n\n> @@ -77,6 +77,61 @@ struct object_id {\n> \tunsigned char hash[GIT_MAX_RAWSZ];\n> };\n> \n> +/*\n> + * Note that these constants are suitable for indexing the hash_algos array and\n> + * comparing against each other, but are otherwise arbitrary, so they should not\n> + * be exposed to the user or serialized to disk. To know whether a\n> + * git_hash_algo struct points to some usable hash function, test the format_id\n> + * field for being non-zero. Use the name field for user-visible situations and\n> + * the format_id field for fixed-length fields on disk.\n> + */\n> +/* An unknown hash function. */\n> +#define GIT_HASH_UNKNOWN 0\n> +/* SHA-1 */\n> +#define GIT_HASH_SHA1 1\n> +/* Number of algorithms supported (including unknown). */\n> +#define GIT_HASH_NALGOS (GIT_HASH_SHA1 + 1)\n> +\n> +typedef void (*git_hash_init_fn)(void *ctx);\n> +typedef void (*git_hash_update_fn)(void *ctx, const void *in, size_t len);\n> +typedef void (*git_hash_final_fn)(unsigned char *hash, void *ctx);\n> +\n> +struct git_hash_algo {\n> +\t/*\n> +\t * The name of the algorithm, as appears in the config file and in\n> +\t * messages.\n> +\t */\n> +\tconst char *name;\n> +\n> +\t/* A four-byte version identifier, used in pack indices. */\n> +\tuint32_t format_id;\n> +\n> +\t/* The size of a hash context (e.g. git_SHA_CTX). */\n> +\tsize_t ctxsz;\n> +\n> +\t/* The length of the hash in binary. */\n> +\tsize_t rawsz;\n> +\n> +\t/* The length of the hash in hex characters. */\n> +\tsize_t hexsz;\n> +\n> +\t/* The hash initialization function. */\n> +\tgit_hash_init_fn init_fn;\n> +\n> +\t/* The hash update function. */\n> +\tgit_hash_update_fn update_fn;\n> +\n> +\t/* The hash finalization function. */\n> +\tgit_hash_final_fn final_fn;\n> +\n> +\t/* The OID of the empty tree. */\n> +\tconst struct object_id *empty_tree;\n> +\n> +\t/* The OID of the empty blob. */\n> +\tconst struct object_id *empty_blob;\n> +};\n> +extern const struct git_hash_algo hash_algos[GIT_HASH_NALGOS];\n> +\n> #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)\n> #define DTYPE(de)\t((de)->d_type)\n> #else\n> diff --git a/sha1_file.c b/sha1_file.c\n> index 10c3a0083d..77b320126a 100644\n> --- a/sha1_file.c\n> +++ b/sha1_file.c\n> @@ -39,6 +39,49 @@ const struct object_id empty_blob_oid = {\n> \tEMPTY_BLOB_SHA1_BIN_LITERAL\n> };\n> \n> +static inline void git_hash_sha1_init(void *ctx)\n> +{\n> +\tgit_SHA1_Init((git_SHA_CTX *)ctx);\n> +}\n> +\n> +static inline void git_hash_sha1_update(void *ctx, const void *data, size_t len)\n> +{\n> +\tgit_SHA1_Update((git_SHA_CTX *)ctx, data, len);\n> +}\n> +\n> +static inline void git_hash_sha1_final(unsigned char *hash, void *ctx)\n> +{\n> +\tgit_SHA1_Final(hash, (git_SHA_CTX *)ctx);\n> +}\n> +\n> +const struct git_hash_algo hash_algos[GIT_HASH_NALGOS] = {\n> +\t{\n> +\t\tNULL,\n> +\t\t0x00000000,\n> +\t\t0,\n> +\t\t0,\n> +\t\t0,\n> +\t\tNULL,\n> +\t\tNULL,\n> +\t\tNULL,\n> +\t\tNULL,\n> +\t\tNULL,\n> +\t},\n> +\t{\n> +\t\t\"sha-1\",\n> +\t\t/* \"sha1\", big-endian */\n> +\t\t0x73686131,\n> +\t\tsizeof(git_SHA_CTX),\n> +\t\tGIT_SHA1_RAWSZ,\n> +\t\tGIT_SHA1_HEXSZ,\n> +\t\tgit_hash_sha1_init,\n> +\t\tgit_hash_sha1_update,\n> +\t\tgit_hash_sha1_final,\n> +\t\t&empty_tree_oid,\n> +\t\t&empty_blob_oid,\n> +\t},\n> +};\n> +\n> /*\n> * This is meant to hold a *small* number of objects that you would\n> * want read_sha1_file() to be able to return, but yet you do not want\n> -- \n> 2.15.0.rc2\n> \n\n-- \nBrandon Williams\n"},{"id":"331531","messageId":"20171101013549.np2zr7kq3uqvtel4@genre.crustytoothpaste.net","threadId":"47059","inReplyTo":"20171030233615.GA94048@google.com","subject":"Re: [PATCH v2 2/4] Add structure representing hash algorithm","fromName":"brian m. carlson","fromEmail":"sandals@crustytoothpaste.net","sentAt":"2017-11-01T01:35:49Z","receivedAt":"2017-11-01T01:36:01Z","isPatch":true,"sender":{"key":"sandals@crustytoothpaste.net","avatar":"https://avatars.githubusercontent.com/u/497054?v=4"},"body":"On Mon, Oct 30, 2017 at 04:36:15PM -0700, Brandon Williams wrote:\n> On 10/28, brian m. carlson wrote:\n> > Since in the future we want to support an additional hash algorithm, add\n> > a structure that represents a hash algorithm and all the data that must\n> > go along with it. Add a constant to allow easy enumeration of hash\n> > algorithms. Implement function typedefs to create an abstract API that\n> > can be used by any hash algorithm, and wrappers for the existing SHA1\n> > functions that conform to this API.\n> > \n> > Expose a value for hex size as well as binary size. While one will\n> > always be twice the other, the two values are both used extremely\n> > commonly throughout the codebase and providing both leads to improved\n> > readability.\n> > \n> > Don't include an entry in the hash algorithm structure for the null\n> > object ID. As this value is all zeros, any suitably sized all-zero\n> > object ID can be used, and there's no need to store a given one on a\n> > per-hash basis.\n> > \n> > The current hash function transition plan envisions a time when we will\n> > accept input from the user that might be in SHA-1 or in the NewHash\n> > format. Since we cannot know which the user has provided, add a\n> > constant representing the unknown algorithm to allow us to indicate that\n> > we must look the correct value up.\n> > \n> > Signed-off-by: brian m. carlson \n> > ---\n> > I believe I did get the format_id constant for SHA-1 right, but\n> > sanity-checking would be appreciated. We don't want another Referer\n> > mess.\n> > \n> > cache.h | 55 +++++++++++++++++++++++++++++++++++++++++++++++++++++++\n> > sha1_file.c | 43 +++++++++++++++++++++++++++++++++++++++++++\n> > 2 files changed, 98 insertions(+)\n> > \n> > diff --git a/cache.h b/cache.h\n> > index 6440e2bf21..9e9eb08f05 100644\n> > --- a/cache.h\n> > +++ b/cache.h\n> \n> Maybe it would be good to place this interface in its own header file\n> that way we can avoid cluttering cache.h with more stuff?\n\nSure, if you like. It will end up needing to be included in cache.h\nbecause of the blob and tree code, but I'm happy to split it out.\n-- \nbrian m. carlson / brian with sandals: Houston, Texas, US\nhttps://www.crustytoothpaste.net/~bmc | My opinion only\nOpenPGP: https://keybase.io/bk2204\n"}]}