threads / patch / 48842

patch, 17 partsobject_id part 14

Subject: [PATCH 00/17] object_id part 14

## tl;dr

37 messages between Jul 8, 2018 and Jul 16, 2018. Diffs are folded; open one to read it.

replies: 36people: 9as markdown or json

brian m. carlson· Jul 8, 2018, 23:36 UTC · lore

This is the fourteenth series of patches to switch to using struct object_id and the_hash_algo. This series converts several core pieces to use struct object_id, including the oid* and hex functions.

All of these patches have been tested with both SHA-1 and a 256-bit hash.

brian m. carlson (17):
  cache: update object ID functions for the_hash_algo
  tree-walk: replace hard-coded constants with the_hash_algo
  hex: switch to using the_hash_algo
  commit: express tree entry constants in terms of the_hash_algo
  strbuf: allocate space with GIT_MAX_HEXSZ
  sha1-name: use the_hash_algo when parsing object names
  commit: increase commit message buffer size
  refs/files-backend: use the_hash_algo for writing refs
  builtin/update-index: convert to using the_hash_algo
  builtin/update-index: simplify parsing of cacheinfo
  builtin/fmt-merge-msg: make hash independent
  builtin/merge: switch to use the_hash_algo
  builtin/merge-recursive: make hash independent
  diff: switch GIT_SHA1_HEXSZ to use the_hash_algo
  log-tree: switch GIT_SHA1_HEXSZ to the_hash_algo->hexsz
  sha1-file: convert constants to uses of the_hash_algo
  pretty: switch hard-coded constants to the_hash_algo
 builtin/fmt-merge-msg.c   | 19 ++++++++++---------
 builtin/merge-recursive.c |  4 ++--
 builtin/merge.c           | 11 ++++++-----
 builtin/update-index.c    | 14 ++++++++------
 cache.h                   |  6 +++---
 commit.c                  |  4 ++--
 diff.c                    |  6 +++---
 hex.c                     |  6 +++---
 log-tree.c                |  2 +-
 pretty.c                  |  4 ++--
 refs/files-backend.c      |  6 +++---
 sha1-file.c               |  8 ++++----
 sha1-name.c               | 12 +++++++-----
 strbuf.c                  |  2 +-
 tree-walk.c               |  3 ++-
 15 files changed, 57 insertions(+), 50 deletions(-)
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 01/17] cache: update object ID functions for the_hash_algo

Update the hashcpy and hashclr functions to use the_hash_algo, since they are used in a variety of places to copy and manipulate buffers that need to move data into or out of struct object_id. Update oidcmp so that it is implemented on its own and similarly uses the_hash_algo.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 cache.h | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to cache.h +3 −3
diff --git a/cache.h b/cache.h
index d49092d94d..c4a64278a1 100644
--- a/cache.h
+++ b/cache.h
@@ -977,7 +977,7 @@ static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
 
 static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
 {
-	return hashcmp(oid1->hash, oid2->hash);
+	return memcmp(oid1->hash, oid2->hash, the_hash_algo->rawsz);
 }
 
 static inline int is_null_sha1(const unsigned char *sha1)
@@ -992,7 +992,7 @@ static inline int is_null_oid(const struct object_id *oid)
 
 static inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src)
 {
-	memcpy(sha_dst, sha_src, GIT_SHA1_RAWSZ);
+	memcpy(sha_dst, sha_src, the_hash_algo->rawsz);
 }
 
 static inline void oidcpy(struct object_id *dst, const struct object_id *src)
@@ -1009,7 +1009,7 @@ static inline struct object_id *oiddup(const struct object_id *src)
 
 static inline void hashclr(unsigned char *hash)
 {
-	memset(hash, 0, GIT_SHA1_RAWSZ);
+	memset(hash, 0, the_hash_algo->rawsz);
 }
 
 static inline void oidclr(struct object_id *oid)
Jacob Keller· Jul 9, 2018, 02:38 UTC · re: brian m. carlson · lore

Re: [PATCH 01/17] cache: update object ID functions for the_hash_algo

On Sun, Jul 8, 2018 at 4:39 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 6 quoted lines
>  static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
>  {
> -       return hashcmp(oid1->hash, oid2->hash);
> +       return memcmp(oid1->hash, oid2->hash, the_hash_algo->rawsz);
>  }
>
Just curious, what's the reasoning for not using the hashcmp anymore?

Thanks, Jake

Eric Sunshine· Jul 9, 2018, 04:05 UTC · re: Jacob Keller · lore

Re: [PATCH 01/17] cache: update object ID functions for the_hash_algo

On Sun, Jul 8, 2018 at 10:38 PM Jacob Keller <jacob.keller@gmail.com> wrote:
Show 9 quoted lines
> On Sun, Jul 8, 2018 at 4:39 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> >  static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
> >  {
> > -       return hashcmp(oid1->hash, oid2->hash);
> > +       return memcmp(oid1->hash, oid2->hash, the_hash_algo->rawsz);
> >  }
>
> Just curious, what's the reasoning for not using the hashcmp anymore?

hashcmp() is specific to SHA-1 (for instance, it hardocdes GIT_SHA1_RAWSZ). oidcmp() is meant as the hash-agnostic replacement for hashcmp(), so it doesn't make sense to continue implementing oidcmp() in terms of hashcmp() (the latter of which will eventually be retired, presumably).

Jacob Keller· Jul 9, 2018, 04:31 UTC · re: Eric Sunshine · lore

Re: [PATCH 01/17] cache: update object ID functions for the_hash_algo

On Sun, Jul 8, 2018 at 9:05 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 17 quoted lines
>
> On Sun, Jul 8, 2018 at 10:38 PM Jacob Keller <jacob.keller@gmail.com> wrote:
> > On Sun, Jul 8, 2018 at 4:39 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> > >  static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
> > >  {
> > > -       return hashcmp(oid1->hash, oid2->hash);
> > > +       return memcmp(oid1->hash, oid2->hash, the_hash_algo->rawsz);
> > >  }
> >
> > Just curious, what's the reasoning for not using the hashcmp anymore?
>
> hashcmp() is specific to SHA-1 (for instance, it hardocdes
> GIT_SHA1_RAWSZ). oidcmp() is meant as the hash-agnostic replacement
> for hashcmp(), so it doesn't make sense to continue implementing
> oidcmp() in terms of hashcmp() (the latter of which will eventually be
> retired, presumably).

Fair. I just saw that hashcmp was also updated to use the_hash_algo, but if we're going to drop it eventually, then there's zero reason to keep implementing oidcmp in terms of it, so... makes sense to me!

Thanks, Jake

brian m. carlson· Jul 9, 2018, 23:26 UTC · re: Jacob Keller · lore

Re: [PATCH 01/17] cache: update object ID functions for the_hash_algo

On Sun, Jul 08, 2018 at 09:31:42PM -0700, Jacob Keller wrote:
Show 22 quoted lines
> On Sun, Jul 8, 2018 at 9:05 PM Eric Sunshine <sunshine@sunshineco.com> wrote:
> >
> > On Sun, Jul 8, 2018 at 10:38 PM Jacob Keller <jacob.keller@gmail.com> wrote:
> > > On Sun, Jul 8, 2018 at 4:39 PM brian m. carlson
> > > <sandals@crustytoothpaste.net> wrote:
> > > >  static inline int oidcmp(const struct object_id *oid1, const struct object_id *oid2)
> > > >  {
> > > > -       return hashcmp(oid1->hash, oid2->hash);
> > > > +       return memcmp(oid1->hash, oid2->hash, the_hash_algo->rawsz);
> > > >  }
> > >
> > > Just curious, what's the reasoning for not using the hashcmp anymore?
> >
> > hashcmp() is specific to SHA-1 (for instance, it hardocdes
> > GIT_SHA1_RAWSZ). oidcmp() is meant as the hash-agnostic replacement
> > for hashcmp(), so it doesn't make sense to continue implementing
> > oidcmp() in terms of hashcmp() (the latter of which will eventually be
> > retired, presumably).
> 
> Fair. I just saw that hashcmp was also updated to use the_hash_algo,
> but if we're going to drop it eventually, then there's zero reason to
> keep implementing oidcmp in terms of it, so... makes sense to me!

Actually, this reminded me that I have a patch that I had forgotten about in my next series that updates hashcmp. I'll squash that in in my reroll, and undo this change.

As a bonus, it also has a nicer commit message which I will include explaining why this is necessary.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 04/17] commit: express tree entry constants in terms of the_hash_algo

Specify these constants in terms of the size of the hash algorithm currently in use.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 commit.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to commit.c +2 −2
diff --git a/commit.c b/commit.c
index 0c3b75aeff..ff05d04570 100644
--- a/commit.c
+++ b/commit.c
@@ -364,8 +364,8 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
 	struct object_id parent;
 	struct commit_list **pptr;
 	struct commit_graft *graft;
-	const int tree_entry_len = GIT_SHA1_HEXSZ + 5;
-	const int parent_entry_len = GIT_SHA1_HEXSZ + 7;
+	const int tree_entry_len = the_hash_algo->hexsz + 5;
+	const int parent_entry_len = the_hash_algo->hexsz + 7;
 
 	if (item->object.parsed)
 		return 0;
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 06/17] sha1-name: use the_hash_algo when parsing object names

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 sha1-name.c | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)
Show changes to sha1-name.c +7 −5
diff --git a/sha1-name.c b/sha1-name.c
index 60d9ef3c7e..ba6a5a689f 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -310,7 +310,7 @@ static int init_object_disambiguation(const char *name, int len,
 {
 	int i;
 
-	if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
+	if (len < MINIMUM_ABBREV || len > the_hash_algo->hexsz)
 		return -1;
 
 	memset(ds, 0, sizeof(*ds));
@@ -576,6 +576,8 @@ int find_unique_abbrev_r(char *hex, const struct object_id *oid, int len)
 	struct disambiguate_state ds;
 	struct min_abbrev_data mad;
 	struct object_id oid_ret;
+	const unsigned hexsz = the_hash_algo->hexsz;
+
 	if (len < 0) {
 		unsigned long count = approximate_object_count();
 		/*
@@ -599,8 +601,8 @@ int find_unique_abbrev_r(char *hex, const struct object_id *oid, int len)
 	}
 
 	oid_to_hex_r(hex, oid);
-	if (len == GIT_SHA1_HEXSZ || !len)
-		return GIT_SHA1_HEXSZ;
+	if (len == hexsz || !len)
+		return hexsz;
 
 	mad.init_len = len;
 	mad.cur_len = len;
@@ -706,7 +708,7 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,
 	int refs_found = 0;
 	int at, reflog_len, nth_prior = 0;
 
-	if (len == GIT_SHA1_HEXSZ && !get_oid_hex(str, oid)) {
+	if (len == the_hash_algo->hexsz && !get_oid_hex(str, oid)) {
 		if (warn_ambiguous_refs && warn_on_object_refname_ambiguity) {
 			refs_found = dwim_ref(str, len, &tmp_oid, &real_ref);
 			if (refs_found > 0) {
@@ -750,7 +752,7 @@ static int get_oid_basic(const char *str, int len, struct object_id *oid,
 		int detached;
 
 		if (interpret_nth_prior_checkout(str, len, &buf) > 0) {
-			detached = (buf.len == GIT_SHA1_HEXSZ && !get_oid_hex(buf.buf, oid));
+			detached = (buf.len == the_hash_algo->hexsz && !get_oid_hex(buf.buf, oid));
 			strbuf_release(&buf);
 			if (detached)
 				return 0;
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 05/17] strbuf: allocate space with GIT_MAX_HEXSZ

In order to be sure we have enough space to use with any hash algorithm, use GIT_MAX_HEXSZ to allocate space.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 strbuf.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to strbuf.c +1 −1
diff --git a/strbuf.c b/strbuf.c
index b0716ac585..030556111d 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -921,7 +921,7 @@ void strbuf_add_unique_abbrev(struct strbuf *sb, const struct object_id *oid,
 			      int abbrev_len)
 {
 	int r;
-	strbuf_grow(sb, GIT_SHA1_HEXSZ + 1);
+	strbuf_grow(sb, GIT_MAX_HEXSZ + 1);
 	r = find_unique_abbrev_r(sb->buf + sb->len, oid, abbrev_len);
 	strbuf_setlen(sb, sb->len + r);
 }
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 10/17] builtin/update-index: simplify parsing of cacheinfo

Switch from using get_oid_hex to parse_oid_hex to simplify pointer operations and avoid the need for a hash-related constant.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/update-index.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)
Show changes to builtin/update-index.c +3 −2
diff --git a/builtin/update-index.c b/builtin/update-index.c
index 031cef5229..3206c5ad45 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -827,6 +827,7 @@ static int parse_new_style_cacheinfo(const char *arg,
 {
 	unsigned long ul;
 	char *endp;
+	const char *p;
 
 	if (!arg)
 		return -1;
@@ -837,9 +838,9 @@ static int parse_new_style_cacheinfo(const char *arg,
 		return -1; /* not a new-style cacheinfo */
 	*mode = ul;
 	endp++;
-	if (get_oid_hex(endp, oid) || endp[GIT_SHA1_HEXSZ] != ',')
+	if (parse_oid_hex(endp, oid, &p) || *p != ',')
 		return -1;
-	*path = endp + GIT_SHA1_HEXSZ + 1;
+	*path = p + 1;
 	return 0;
 }
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 11/17] builtin/fmt-merge-msg: make hash independent

Convert several uses of GIT_SHA1_HEXSZ into references to the_hash_algo. Switch other uses into a use of parse_oid_hex and uses of its computed pointer.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/fmt-merge-msg.c | 19 ++++++++++---------
 1 file changed, 10 insertions(+), 9 deletions(-)
Show changes to builtin/fmt-merge-msg.c +10 −9
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index bd680be687..e8c13a2c03 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -108,14 +108,15 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
 	struct string_list_item *item;
 	int pulling_head = 0;
 	struct object_id oid;
+	const unsigned hexsz = the_hash_algo->hexsz;
 
-	if (len < GIT_SHA1_HEXSZ + 3 || line[GIT_SHA1_HEXSZ] != '\t')
+	if (len < hexsz + 3 || line[hexsz] != '\t')
 		return 1;
 
-	if (starts_with(line + GIT_SHA1_HEXSZ + 1, "not-for-merge"))
+	if (starts_with(line + hexsz + 1, "not-for-merge"))
 		return 0;
 
-	if (line[GIT_SHA1_HEXSZ + 1] != '\t')
+	if (line[hexsz + 1] != '\t')
 		return 2;
 
 	i = get_oid_hex(line, &oid);
@@ -130,7 +131,7 @@ static int handle_line(char *line, struct merge_parents *merge_parents)
 
 	if (line[len - 1] == '\n')
 		line[len - 1] = 0;
-	line += GIT_SHA1_HEXSZ + 2;
+	line += hexsz + 2;
 
 	/*
 	 * At this point, line points at the beginning of comment e.g.
@@ -342,7 +343,7 @@ static void shortlog(const char *name,
 	const struct object_id *oid = &origin_data->oid;
 	int limit = opts->shortlog_len;
 
-	branch = deref_tag(parse_object(oid), oid_to_hex(oid), GIT_SHA1_HEXSZ);
+	branch = deref_tag(parse_object(oid), oid_to_hex(oid), the_hash_algo->hexsz);
 	if (!branch || branch->type != OBJ_COMMIT)
 		return;
 
@@ -545,6 +546,7 @@ static void find_merge_parents(struct merge_parents *result,
 		int len;
 		char *p = in->buf + pos;
 		char *newline = strchr(p, '\n');
+		const char *q;
 		struct object_id oid;
 		struct commit *parent;
 		struct object *obj;
@@ -552,10 +554,9 @@ static void find_merge_parents(struct merge_parents *result,
 		len = newline ? newline - p : strlen(p);
 		pos += len + !!newline;
 
-		if (len < GIT_SHA1_HEXSZ + 3 ||
-		    get_oid_hex(p, &oid) ||
-		    p[GIT_SHA1_HEXSZ] != '\t' ||
-		    p[GIT_SHA1_HEXSZ + 1] != '\t')
+		if (parse_oid_hex(p, &oid, &q) ||
+		    q[0] != '\t' ||
+		    q[1] != '\t')
 			continue; /* skip not-for-merge */
 		/*
 		 * Do not use get_merge_parent() here; we do not have
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 14/17] diff: switch GIT_SHA1_HEXSZ to use the_hash_algo

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 diff.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to diff.c +3 −3
diff --git a/diff.c b/diff.c
index 639eb646b9..485ff6c264 100644
--- a/diff.c
+++ b/diff.c
@@ -3832,7 +3832,7 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
 		char *hex = oid_to_hex(oid);
 		if (abbrev < 0)
 			abbrev = FALLBACK_DEFAULT_ABBREV;
-		if (abbrev > GIT_SHA1_HEXSZ)
+		if (abbrev > the_hash_algo->hexsz)
 			BUG("oid abbreviation out of range: %d", abbrev);
 		if (abbrev)
 			hex[abbrev] = '\0';
@@ -4947,7 +4947,7 @@ const char *diff_aligned_abbrev(const struct object_id *oid, int len)
 	const char *abbrev;
 
 	/* Do we want all 40 hex characters? */
-	if (len == GIT_SHA1_HEXSZ)
+	if (len == the_hash_algo->hexsz)
 		return oid_to_hex(oid);
 
 	/* An abbreviated value is fine, possibly followed by an ellipsis. */
@@ -4977,7 +4977,7 @@ const char *diff_aligned_abbrev(const struct object_id *oid, int len)
 	 * the automatic sizing is supposed to give abblen that ensures
 	 * uniqueness across all objects (statistically speaking).
 	 */
-	if (abblen < GIT_SHA1_HEXSZ - 3) {
+	if (abblen < the_hash_algo->hexsz - 3) {
 		static char hex[GIT_MAX_HEXSZ + 1];
 		if (len < abblen && abblen <= len + 2)
 			xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, "..");
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 13/17] builtin/merge-recursive: make hash independent

Use GIT_MAX_HEXSZ instead of GIT_SHA1_HEXSZ for an allocation so that it is sufficiently large. Switch a comparison to use the_hash_algo to determine the length of a hex object ID.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/merge-recursive.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to builtin/merge-recursive.c +2 −2
diff --git a/builtin/merge-recursive.c b/builtin/merge-recursive.c
index 0dd9021958..9b2f707c29 100644
--- a/builtin/merge-recursive.c
+++ b/builtin/merge-recursive.c
@@ -9,10 +9,10 @@ static const char builtin_merge_recursive_usage[] =
 
 static const char *better_branch_name(const char *branch)
 {
-	static char githead_env[8 + GIT_SHA1_HEXSZ + 1];
+	static char githead_env[8 + GIT_MAX_HEXSZ + 1];
 	char *name;
 
-	if (strlen(branch) != GIT_SHA1_HEXSZ)
+	if (strlen(branch) != the_hash_algo->hexsz)
 		return branch;
 	xsnprintf(githead_env, sizeof(githead_env), "GITHEAD_%s", branch);
 	name = getenv(githead_env);
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 17/17] pretty: switch hard-coded constants to the_hash_algo

Switch several hard-coded constants into expressions based either on GIT_MAX_HEXSZ or the_hash_algo.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 pretty.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to pretty.c +2 −2
diff --git a/pretty.c b/pretty.c
index 703fa6ff7b..b0e653ff25 100644
--- a/pretty.c
+++ b/pretty.c
@@ -1575,7 +1575,7 @@ static void pp_header(struct pretty_print_context *pp,
 		}
 
 		if (starts_with(line, "parent ")) {
-			if (linelen != 48)
+			if (linelen != the_hash_algo->hexsz + 8)
 				die("bad parent line in commit");
 			continue;
 		}
@@ -1583,7 +1583,7 @@ static void pp_header(struct pretty_print_context *pp,
 		if (!parents_shown) {
 			unsigned num = commit_list_count(commit->parents);
 			/* with enough slop */
-			strbuf_grow(sb, num * 50 + 20);
+			strbuf_grow(sb, num * (GIT_MAX_HEXSZ + 10) + 20);
 			add_merge_info(pp, sb, commit);
 			parents_shown = 1;
 		}
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 15/17] log-tree: switch GIT_SHA1_HEXSZ to the_hash_algo->hexsz

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 log-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to log-tree.c +1 −1
diff --git a/log-tree.c b/log-tree.c
index d3a43e29cd..9655de8ad7 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -545,7 +545,7 @@ void show_log(struct rev_info *opt)
 	struct strbuf msgbuf = STRBUF_INIT;
 	struct log_info *log = opt->loginfo;
 	struct commit *commit = log->commit, *parent = log->parent;
-	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : GIT_SHA1_HEXSZ;
+	int abbrev_commit = opt->abbrev_commit ? opt->abbrev : the_hash_algo->hexsz;
 	const char *extra_headers = opt->extra_headers;
 	struct pretty_print_context ctx = {0};
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 16/17] sha1-file: convert constants to uses of the_hash_algo

Convert one use of 20 and several uses of GIT_SHA1_HEXSZ into references to the_hash_algo.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 sha1-file.c | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)
Show changes to sha1-file.c +4 −4
diff --git a/sha1-file.c b/sha1-file.c
index de4839e634..1f66b9594f 100644
--- a/sha1-file.c
+++ b/sha1-file.c
@@ -336,7 +336,7 @@ int raceproof_create_file(const char *path, create_file_fn fn, void *cb)
 static void fill_sha1_path(struct strbuf *buf, const unsigned char *sha1)
 {
 	int i;
-	for (i = 0; i < 20; i++) {
+	for (i = 0; i < the_hash_algo->rawsz; i++) {
 		static char hex[] = "0123456789abcdef";
 		unsigned int val = sha1[i];
 		strbuf_addch(buf, hex[val >> 4]);
@@ -1473,7 +1473,7 @@ void *read_object_with_reference(const struct object_id *oid,
 		}
 		ref_length = strlen(ref_type);
 
-		if (ref_length + GIT_SHA1_HEXSZ > isize ||
+		if (ref_length + the_hash_algo->hexsz > isize ||
 		    memcmp(buffer, ref_type, ref_length) ||
 		    get_oid_hex((char *) buffer + ref_length, &actual_oid)) {
 			free(buffer);
@@ -2062,9 +2062,9 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		namelen = strlen(de->d_name);
 		strbuf_setlen(path, baselen);
 		strbuf_add(path, de->d_name, namelen);
-		if (namelen == GIT_SHA1_HEXSZ - 2 &&
+		if (namelen == the_hash_algo->hexsz - 2 &&
 		    !hex_to_bytes(oid.hash + 1, de->d_name,
-				  GIT_SHA1_RAWSZ - 1)) {
+				  the_hash_algo->rawsz - 1)) {
 			if (obj_cb) {
 				r = obj_cb(&oid, path->buf, data);
 				if (r)
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 12/17] builtin/merge: switch to use the_hash_algo

Switch uses of GIT_SHA1_HEXSZ to use the_hash_algo instead.
Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/merge.c | 11 ++++++-----
 1 file changed, 6 insertions(+), 5 deletions(-)
Show changes to builtin/merge.c +6 −5
diff --git a/builtin/merge.c b/builtin/merge.c
index 4a4c09496c..916c9f0569 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1034,6 +1034,7 @@ static void handle_fetch_head(struct commit_list **remotes, struct strbuf *merge
 	const char *filename;
 	int fd, pos, npos;
 	struct strbuf fetch_head_file = STRBUF_INIT;
+	const unsigned hexsz = the_hash_algo->hexsz;
 
 	if (!merge_names)
 		merge_names = &fetch_head_file;
@@ -1059,16 +1060,16 @@ static void handle_fetch_head(struct commit_list **remotes, struct strbuf *merge
 		else
 			npos = merge_names->len;
 
-		if (npos - pos < GIT_SHA1_HEXSZ + 2 ||
+		if (npos - pos < hexsz + 2 ||
 		    get_oid_hex(merge_names->buf + pos, &oid))
 			commit = NULL; /* bad */
-		else if (memcmp(merge_names->buf + pos + GIT_SHA1_HEXSZ, "\t\t", 2))
+		else if (memcmp(merge_names->buf + pos + hexsz, "\t\t", 2))
 			continue; /* not-for-merge */
 		else {
-			char saved = merge_names->buf[pos + GIT_SHA1_HEXSZ];
-			merge_names->buf[pos + GIT_SHA1_HEXSZ] = '\0';
+			char saved = merge_names->buf[pos + hexsz];
+			merge_names->buf[pos + hexsz] = '\0';
 			commit = get_merge_parent(merge_names->buf + pos);
-			merge_names->buf[pos + GIT_SHA1_HEXSZ] = saved;
+			merge_names->buf[pos + hexsz] = saved;
 		}
 		if (!commit) {
 			if (ptr)
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 09/17] builtin/update-index: convert to using the_hash_algo

Switch from using GIT_SHA1_HEXSZ to the_hash_algo to make the parsing of the index information hash independent.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/update-index.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)
Show changes to builtin/update-index.c +5 −4
diff --git a/builtin/update-index.c b/builtin/update-index.c
index a8709a26ec..031cef5229 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -492,6 +492,7 @@ static void update_one(const char *path)
 
 static void read_index_info(int nul_term_line)
 {
+	const int hexsz = the_hash_algo->hexsz;
 	struct strbuf buf = STRBUF_INIT;
 	struct strbuf uq = STRBUF_INIT;
 	strbuf_getline_fn getline_fn;
@@ -529,7 +530,7 @@ static void read_index_info(int nul_term_line)
 		mode = ul;
 
 		tab = strchr(ptr, '\t');
-		if (!tab || tab - ptr < GIT_SHA1_HEXSZ + 1)
+		if (!tab || tab - ptr < hexsz + 1)
 			goto bad_line;
 
 		if (tab[-2] == ' ' && '0' <= tab[-1] && tab[-1] <= '3') {
@@ -542,8 +543,8 @@ static void read_index_info(int nul_term_line)
 			ptr = tab + 1; /* point at the head of path */
 		}
 
-		if (get_oid_hex(tab - GIT_SHA1_HEXSZ, &oid) ||
-			tab[-(GIT_SHA1_HEXSZ + 1)] != ' ')
+		if (get_oid_hex(tab - hexsz, &oid) ||
+			tab[-(hexsz + 1)] != ' ')
 			goto bad_line;
 
 		path_name = ptr;
@@ -571,7 +572,7 @@ static void read_index_info(int nul_term_line)
 			 * ptr[-1] points at tab,
 			 * ptr[-41] is at the beginning of sha1
 			 */
-			ptr[-(GIT_SHA1_HEXSZ + 2)] = ptr[-1] = 0;
+			ptr[-(hexsz + 2)] = ptr[-1] = 0;
 			if (add_cacheinfo(mode, &oid, path_name, stage))
 				die("git update-index: unable to update %s",
 				    path_name);
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 07/17] commit: increase commit message buffer size

100 bytes is not sufficient to ensure we can write a commit message buffer when using a 32-byte hash algorithm. Increase the buffer size to ensure we have sufficient space.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 refs/files-backend.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
Show changes to refs/files-backend.c +1 −1
diff --git a/refs/files-backend.c b/refs/files-backend.c
index a9a066dcfb..252f835bae 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
 	char *logrec;
 
 	msglen = msg ? strlen(msg) : 0;
-	maxlen = strlen(committer) + msglen + 100;
+	maxlen = strlen(committer) + msglen + 200;
 	logrec = xmalloc(maxlen);
 	len = xsnprintf(logrec, maxlen, "%s %s %s\n",
 			oid_to_hex(old_oid),
Derrick Stolee· Jul 9, 2018, 13:09 UTC · re: brian m. carlson · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

On 7/8/2018 7:36 PM, brian m. carlson wrote:
Show 22 quoted lines
> 100 bytes is not sufficient to ensure we can write a commit message
> buffer when using a 32-byte hash algorithm.  Increase the buffer size to
> ensure we have sufficient space.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>   refs/files-backend.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index a9a066dcfb..252f835bae 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
>   	char *logrec;
>   
>   	msglen = msg ? strlen(msg) : 0;
> -	maxlen = strlen(committer) + msglen + 100;
> +	maxlen = strlen(committer) + msglen + 200;
>   	logrec = xmalloc(maxlen);
>   	len = xsnprintf(logrec, maxlen, "%s %s %s\n",
>   			oid_to_hex(old_oid),

nit: 100 is not enough anymore, but wasn't a very descriptive value. 200 may be enough now, but I'm not sure why.

Thanks, -Stolee

Stefan Beller· Jul 9, 2018, 17:24 UTC · re: Derrick Stolee · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

On Mon, Jul 9, 2018 at 6:09 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 27 quoted lines
>
> On 7/8/2018 7:36 PM, brian m. carlson wrote:
> > 100 bytes is not sufficient to ensure we can write a commit message
> > buffer when using a 32-byte hash algorithm.  Increase the buffer size to
> > ensure we have sufficient space.
> >
> > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > ---
> >   refs/files-backend.c | 2 +-
> >   1 file changed, 1 insertion(+), 1 deletion(-)
> >
> > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > index a9a066dcfb..252f835bae 100644
> > --- a/refs/files-backend.c
> > +++ b/refs/files-backend.c
> > @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
> >       char *logrec;
> >
> >       msglen = msg ? strlen(msg) : 0;
> > -     maxlen = strlen(committer) + msglen + 100;
> > +     maxlen = strlen(committer) + msglen + 200;
> >       logrec = xmalloc(maxlen);
> >       len = xsnprintf(logrec, maxlen, "%s %s %s\n",
> >                       oid_to_hex(old_oid),
>
> nit: 100 is not enough anymore, but wasn't a very descriptive value. 200
> may be enough now, but I'm not sure why.

That line was touched in by Michael in 7bd9bcf372d (refs: split filesystem-based refs code into a new file, 2015-11-09) and before that by Ronnie in 2c6207abbd6 (refs.c: add a function to append a reflog entry to a fd, 2014-12-12) and introduced by Junio in 8ac65937d03 (Make sure we do not write bogus reflog entries., 2007-01-26) and it appears to me that 2*40 + 5 ought to be sufficient, but no comments or commit messages are found as to why we rather choose 100.

Brandon Williams· Jul 9, 2018, 17:27 UTC · re: Stefan Beller · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

On 07/09, Stefan Beller wrote:
Show 38 quoted lines
> On Mon, Jul 9, 2018 at 6:09 AM Derrick Stolee <stolee@gmail.com> wrote:
> >
> > On 7/8/2018 7:36 PM, brian m. carlson wrote:
> > > 100 bytes is not sufficient to ensure we can write a commit message
> > > buffer when using a 32-byte hash algorithm.  Increase the buffer size to
> > > ensure we have sufficient space.
> > >
> > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> > > ---
> > >   refs/files-backend.c | 2 +-
> > >   1 file changed, 1 insertion(+), 1 deletion(-)
> > >
> > > diff --git a/refs/files-backend.c b/refs/files-backend.c
> > > index a9a066dcfb..252f835bae 100644
> > > --- a/refs/files-backend.c
> > > +++ b/refs/files-backend.c
> > > @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
> > >       char *logrec;
> > >
> > >       msglen = msg ? strlen(msg) : 0;
> > > -     maxlen = strlen(committer) + msglen + 100;
> > > +     maxlen = strlen(committer) + msglen + 200;
> > >       logrec = xmalloc(maxlen);
> > >       len = xsnprintf(logrec, maxlen, "%s %s %s\n",
> > >                       oid_to_hex(old_oid),
> >
> > nit: 100 is not enough anymore, but wasn't a very descriptive value. 200
> > may be enough now, but I'm not sure why.
> 
> That line was touched in by Michael in 7bd9bcf372d (refs: split filesystem-based
> refs code into a new file, 2015-11-09) and before that by Ronnie in 2c6207abbd6
> (refs.c: add a function to append a reflog entry to a fd, 2014-12-12)
> and introduced
> by Junio in 8ac65937d03 (Make sure we do not write bogus reflog
> entries., 2007-01-26)
> and it appears to me that 2*40 + 5 ought to be sufficient, but no
> comments or commit
> messages are found as to why we rather choose 100.

Whats the reason for not using a strbuf here so that we don't have to play with magic numbers?

-- 
Brandon Williams
Junio C Hamano· Jul 9, 2018, 17:34 UTC · re: Brandon Williams · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

Brandon Williams <bmwill@google.com> writes:
Show 29 quoted lines
>> > > diff --git a/refs/files-backend.c b/refs/files-backend.c
>> > > index a9a066dcfb..252f835bae 100644
>> > > --- a/refs/files-backend.c
>> > > +++ b/refs/files-backend.c
>> > > @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
>> > >       char *logrec;
>> > >
>> > >       msglen = msg ? strlen(msg) : 0;
>> > > -     maxlen = strlen(committer) + msglen + 100;
>> > > +     maxlen = strlen(committer) + msglen + 200;
>> > >       logrec = xmalloc(maxlen);
>> > >       len = xsnprintf(logrec, maxlen, "%s %s %s\n",
>> > >                       oid_to_hex(old_oid),
>> >
>> > nit: 100 is not enough anymore, but wasn't a very descriptive value. 200
>> > may be enough now, but I'm not sure why.
>> 
>> That line was touched in by Michael in 7bd9bcf372d (refs: split filesystem-based
>> refs code into a new file, 2015-11-09) and before that by Ronnie in 2c6207abbd6
>> (refs.c: add a function to append a reflog entry to a fd, 2014-12-12)
>> and introduced
>> by Junio in 8ac65937d03 (Make sure we do not write bogus reflog
>> entries., 2007-01-26)
>> and it appears to me that 2*40 + 5 ought to be sufficient, but no
>> comments or commit
>> messages are found as to why we rather choose 100.
>
> Whats the reason for not using a strbuf here so that we don't have to
> play with magic numbers?
Quite a legitimate question.

I suspect that the reason is because the code (even though it now sits in a file that was relatively recently creted) predates either the introduction or wide adoption of strbuf.

Back when 6de08ae6 ("Log ref updates to logs/refs/<ref>", 2006-05-17) was done, we already had strbuf.c, but it only had read_line() and nothing else back then, so it wouldn't have been possible to use a strbuf there.

Brandon Williams· Jul 9, 2018, 17:36 UTC · re: Junio C Hamano · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

On 07/09, Junio C Hamano wrote:
Show 42 quoted lines
> Brandon Williams <bmwill@google.com> writes:
> 
> >> > > diff --git a/refs/files-backend.c b/refs/files-backend.c
> >> > > index a9a066dcfb..252f835bae 100644
> >> > > --- a/refs/files-backend.c
> >> > > +++ b/refs/files-backend.c
> >> > > @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
> >> > >       char *logrec;
> >> > >
> >> > >       msglen = msg ? strlen(msg) : 0;
> >> > > -     maxlen = strlen(committer) + msglen + 100;
> >> > > +     maxlen = strlen(committer) + msglen + 200;
> >> > >       logrec = xmalloc(maxlen);
> >> > >       len = xsnprintf(logrec, maxlen, "%s %s %s\n",
> >> > >                       oid_to_hex(old_oid),
> >> >
> >> > nit: 100 is not enough anymore, but wasn't a very descriptive value. 200
> >> > may be enough now, but I'm not sure why.
> >> 
> >> That line was touched in by Michael in 7bd9bcf372d (refs: split filesystem-based
> >> refs code into a new file, 2015-11-09) and before that by Ronnie in 2c6207abbd6
> >> (refs.c: add a function to append a reflog entry to a fd, 2014-12-12)
> >> and introduced
> >> by Junio in 8ac65937d03 (Make sure we do not write bogus reflog
> >> entries., 2007-01-26)
> >> and it appears to me that 2*40 + 5 ought to be sufficient, but no
> >> comments or commit
> >> messages are found as to why we rather choose 100.
> >
> > Whats the reason for not using a strbuf here so that we don't have to
> > play with magic numbers?
> 
> Quite a legitimate question.  
> 
> I suspect that the reason is because the code (even though it now
> sits in a file that was relatively recently creted) predates either
> the introduction or wide adoption of strbuf.
> 
> Back when 6de08ae6 ("Log ref updates to logs/refs/<ref>",
> 2006-05-17) was done, we already had strbuf.c, but it only had
> read_line() and nothing else back then, so it wouldn't have been
> possible to use a strbuf there.

Fair enough, having never working in the code back then I don't know what life was like without strbuf.

-- 
Brandon Williams
Junio C Hamano· Jul 9, 2018, 17:45 UTC · re: Derrick Stolee · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

Derrick Stolee <stolee@gmail.com> writes:
Show 25 quoted lines
> On 7/8/2018 7:36 PM, brian m. carlson wrote:
>> 100 bytes is not sufficient to ensure we can write a commit message
>> buffer when using a 32-byte hash algorithm.  Increase the buffer size to
>> ensure we have sufficient space.
>>
>> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
>> ---
>>   refs/files-backend.c | 2 +-
>>   1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>> index a9a066dcfb..252f835bae 100644
>> --- a/refs/files-backend.c
>> +++ b/refs/files-backend.c
>> @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
>>   	char *logrec;
>>     	msglen = msg ? strlen(msg) : 0;
>> -	maxlen = strlen(committer) + msglen + 100;
>> +	maxlen = strlen(committer) + msglen + 200;
>>   	logrec = xmalloc(maxlen);
>>   	len = xsnprintf(logrec, maxlen, "%s %s %s\n",
>>   			oid_to_hex(old_oid),
>
> nit: 100 is not enough anymore, but wasn't a very descriptive
> value. 200 may be enough now, but I'm not sure why.

As Brandon alludes to downthread, we probably should use strbuf for things like this these days, so a preliminary clean-up to do so is probably a welcome change to sneak in and rebase this series on top of.

"%s %s %s\n" with old and new commit object name and the message will be "2 * len(hash_in_hex) + 4" bytes long (counting the three whitespaces and the terminating NUL), and Shawn's original in 6de08ae6 ("Log ref updates to logs/refs/<ref>", 2006-05-17) actually computed this one as "strlen(...) + 2*40+4".

100 was merely me being sloppier than Shawn at 8ac65937 ("Make sure we do not write bogus reflog entries.", 2007-01-26), preferring being sufficient over not wasting even a single byte.

brian m. carlson· Jul 9, 2018, 23:39 UTC · re: Junio C Hamano · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote:
Show 18 quoted lines
> Derrick Stolee <stolee@gmail.com> writes:
> 
> > On 7/8/2018 7:36 PM, brian m. carlson wrote:
> >> diff --git a/refs/files-backend.c b/refs/files-backend.c
> >> index a9a066dcfb..252f835bae 100644
> >> --- a/refs/files-backend.c
> >> +++ b/refs/files-backend.c
> >> @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
> >>   	char *logrec;
> >>     	msglen = msg ? strlen(msg) : 0;
> >> -	maxlen = strlen(committer) + msglen + 100;
> >> +	maxlen = strlen(committer) + msglen + 200;
> >>   	logrec = xmalloc(maxlen);
> >>   	len = xsnprintf(logrec, maxlen, "%s %s %s\n",
> >>   			oid_to_hex(old_oid),
> >
> > nit: 100 is not enough anymore, but wasn't a very descriptive
> > value. 200 may be enough now, but I'm not sure why.

200 is definitely enough. Suppose we had a message consisting entirely of SHA-1 hashes (5, at 20 bytes a piece). If our new hash is 32 bytes long, then it would require at most 160 bytes.

I only noticed this because the old code segfaulted. My approach to using a 32-byte hash was to set it up, do some basic tests, find out what crashed, and fix it. Most of this series is the basics necessary to get the most rudimentary functionality out of a 32-byte Git, excluding the index pieces, which are necessarily inelegant.

I didn't include them because there are other ways to implement the changes which are more elegant in some ways and less elegant in other ways, and I want to think more about it before I send them in.

> As Brandon alludes to downthread, we probably should use strbuf for
> things like this these days, so a preliminary clean-up to do so is
> probably a welcome change to sneak in and rebase this series on top
> of.

Sure, I agree that would be a better change, and I'm happy to reroll with that.

-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Junio C Hamano· Jul 10, 2018, 16:35 UTC · re: brian m. carlson · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 7 quoted lines
>> As Brandon alludes to downthread, we probably should use strbuf for
>> things like this these days, so a preliminary clean-up to do so is
>> probably a welcome change to sneak in and rebase this series on top
>> of.
>
> Sure, I agree that would be a better change, and I'm happy to reroll
> with that.

Or we can do the clean-up after this 17-patch series settles, which probably is a better order to do things. It is usually better to make a longer but more mechanical topic like this pass through the system rather quickly, which tends to minimize disruption on other topics.

Ben Peart· Jul 10, 2018, 18:08 UTC · re: brian m. carlson · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

On 7/9/2018 7:39 PM, brian m. carlson wrote:
Show 42 quoted lines
> On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote:
>> Derrick Stolee <stolee@gmail.com> writes:
>>
>>> On 7/8/2018 7:36 PM, brian m. carlson wrote:
>>>> diff --git a/refs/files-backend.c b/refs/files-backend.c
>>>> index a9a066dcfb..252f835bae 100644
>>>> --- a/refs/files-backend.c
>>>> +++ b/refs/files-backend.c
>>>> @@ -1587,7 +1587,7 @@ static int log_ref_write_fd(int fd, const struct object_id *old_oid,
>>>>    	char *logrec;
>>>>      	msglen = msg ? strlen(msg) : 0;
>>>> -	maxlen = strlen(committer) + msglen + 100;
>>>> +	maxlen = strlen(committer) + msglen + 200;
>>>>    	logrec = xmalloc(maxlen);
>>>>    	len = xsnprintf(logrec, maxlen, "%s %s %s\n",
>>>>    			oid_to_hex(old_oid),
>>>
>>> nit: 100 is not enough anymore, but wasn't a very descriptive
>>> value. 200 may be enough now, but I'm not sure why.
> 
> 200 is definitely enough.  Suppose we had a message consisting entirely
> of SHA-1 hashes (5, at 20 bytes a piece).  If our new hash is 32 bytes
> long, then it would require at most 160 bytes.
> 
> I only noticed this because the old code segfaulted.  My approach to
> using a 32-byte hash was to set it up, do some basic tests, find out
> what crashed, and fix it.  Most of this series is the basics necessary
> to get the most rudimentary functionality out of a 32-byte Git,
> excluding the index pieces, which are necessarily inelegant.
> 
> I didn't include them because there are other ways to implement the
> changes which are more elegant in some ways and less elegant in other
> ways, and I want to think more about it before I send them in.
> 
>> As Brandon alludes to downthread, we probably should use strbuf for
>> things like this these days, so a preliminary clean-up to do so is
>> probably a welcome change to sneak in and rebase this series on top
>> of.
> 
> Sure, I agree that would be a better change, and I'm happy to reroll
> with that.
>

I've put together a patch to update log_ref_write_fd() to use strbuf and will submit it shortly.

brian m. carlson· Jul 11, 2018, 01:43 UTC · re: Ben Peart · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

On Tue, Jul 10, 2018 at 02:08:28PM -0400, Ben Peart wrote:
Show 15 quoted lines
> 
> 
> On 7/9/2018 7:39 PM, brian m. carlson wrote:
> > On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote:
> > > As Brandon alludes to downthread, we probably should use strbuf for
> > > things like this these days, so a preliminary clean-up to do so is
> > > probably a welcome change to sneak in and rebase this series on top
> > > of.
> > 
> > Sure, I agree that would be a better change, and I'm happy to reroll
> > with that.
> > 
> 
> I've put together a patch to update log_ref_write_fd() to use strbuf and
> will submit it shortly.
Excellent.  I'll drop this patch in that case.
-- 
brian m. carlson: Houston, Texas, US
OpenPGP: https://keybase.io/bk2204
Junio C Hamano· Jul 11, 2018, 15:36 UTC · re: brian m. carlson · lore

Re: [PATCH 07/17] commit: increase commit message buffer size

"brian m. carlson" <sandals@crustytoothpaste.net> writes:
Show 18 quoted lines
> On Tue, Jul 10, 2018 at 02:08:28PM -0400, Ben Peart wrote:
>> 
>> 
>> On 7/9/2018 7:39 PM, brian m. carlson wrote:
>> > On Mon, Jul 09, 2018 at 10:45:33AM -0700, Junio C Hamano wrote:
>> > > As Brandon alludes to downthread, we probably should use strbuf for
>> > > things like this these days, so a preliminary clean-up to do so is
>> > > probably a welcome change to sneak in and rebase this series on top
>> > > of.
>> > 
>> > Sure, I agree that would be a better change, and I'm happy to reroll
>> > with that.
>> > 
>> 
>> I've put together a patch to update log_ref_write_fd() to use strbuf and
>> will submit it shortly.
>
> Excellent.  I'll drop this patch in that case.

OK, thanks for working well together. In the integration run I did yesterday evening, this part was resolved by taking what Ben did, dropping the s/100/200/ change from this series, and the result looked reasonable, of course.

brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 08/17] refs/files-backend: use the_hash_algo for writing refs

In order to ensure we write the correct amount, use the_hash_algo to find the correct number of bytes for the current hash.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 refs/files-backend.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)
Show changes to refs/files-backend.c +2 −2
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 252f835bae..4a724f20a9 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -1676,7 +1676,7 @@ static int write_ref_to_lockfile(struct ref_lock *lock,
 		return -1;
 	}
 	fd = get_lock_file_fd(&lock->lk);
-	if (write_in_full(fd, oid_to_hex(oid), GIT_SHA1_HEXSZ) < 0 ||
+	if (write_in_full(fd, oid_to_hex(oid), the_hash_algo->hexsz) < 0 ||
 	    write_in_full(fd, &term, 1) < 0 ||
 	    close_ref_gently(lock) < 0) {
 		strbuf_addf(err,
@@ -3070,7 +3070,7 @@ static int files_reflog_expire(struct ref_store *ref_store,
 			rollback_lock_file(&reflog_lock);
 		} else if (update &&
 			   (write_in_full(get_lock_file_fd(&lock->lk),
-				oid_to_hex(&cb.last_kept_oid), GIT_SHA1_HEXSZ) < 0 ||
+				oid_to_hex(&cb.last_kept_oid), the_hash_algo->hexsz) < 0 ||
 			    write_str_in_full(get_lock_file_fd(&lock->lk), "\n") < 0 ||
 			    close_ref_gently(lock) < 0)) {
 			status |= error("couldn't write %s",
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 02/17] tree-walk: replace hard-coded constants with the_hash_algo

Remove the hard-coded 20-based values and replace them with uses of the_hash_algo.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 tree-walk.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)
Show changes to tree-walk.c +2 −1
diff --git a/tree-walk.c b/tree-walk.c
index 8f5090862b..c1f27086a9 100644
--- a/tree-walk.c
+++ b/tree-walk.c
@@ -26,8 +26,9 @@ static int decode_tree_entry(struct tree_desc *desc, const char *buf, unsigned l
 {
 	const char *path;
 	unsigned int mode, len;
+	const unsigned hashsz = the_hash_algo->rawsz;
 
-	if (size < 23 || buf[size - 21]) {
+	if (size < hashsz + 3 || buf[size - (hashsz + 1)]) {
 		strbuf_addstr(err, _("too-short tree object"));
 		return -1;
 	}
brian m. carlson· Jul 8, 2018, 23:36 UTC · re: brian m. carlson · lore

[PATCH 03/17] hex: switch to using the_hash_algo

Instead of using the GIT_SHA1_* constants, switch to using the_hash_algo to convert object IDs to and from hex format.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 hex.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)
Show changes to hex.c +3 −3
diff --git a/hex.c b/hex.c
index 8df2d63728..10af1a29e8 100644
--- a/hex.c
+++ b/hex.c
@@ -50,7 +50,7 @@ int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
 int get_sha1_hex(const char *hex, unsigned char *sha1)
 {
 	int i;
-	for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
+	for (i = 0; i < the_hash_algo->rawsz; i++) {
 		int val = hex2chr(hex);
 		if (val < 0)
 			return -1;
@@ -69,7 +69,7 @@ int parse_oid_hex(const char *hex, struct object_id *oid, const char **end)
 {
 	int ret = get_oid_hex(hex, oid);
 	if (!ret)
-		*end = hex + GIT_SHA1_HEXSZ;
+		*end = hex + the_hash_algo->hexsz;
 	return ret;
 }
 
@@ -79,7 +79,7 @@ char *sha1_to_hex_r(char *buffer, const unsigned char *sha1)
 	char *buf = buffer;
 	int i;
 
-	for (i = 0; i < GIT_SHA1_RAWSZ; i++) {
+	for (i = 0; i < the_hash_algo->rawsz; i++) {
 		unsigned int val = *sha1++;
 		*buf++ = hex[val >> 4];
 		*buf++ = hex[val & 0xf];
Jacob Keller· Jul 9, 2018, 03:12 UTC · re: brian m. carlson · lore

Re: [PATCH 00/17] object_id part 14

On Sun, Jul 8, 2018 at 4:39 PM brian m. carlson <sandals@crustytoothpaste.net> wrote:

Show 8 quoted lines
>
> This is the fourteenth series of patches to switch to using struct
> object_id and the_hash_algo.  This series converts several core pieces
> to use struct object_id, including the oid* and hex functions.
>
> All of these patches have been tested with both SHA-1 and a 256-bit
> hash.
>

I read through the series, and didn't spot anything odd, except for the question about reasoning for why we use memcmp directly over using hashcmp. I don't think that's any sort of blocker, it just seemed an odd decision to me.

Thanks, Jake

Derrick Stolee· Jul 9, 2018, 13:12 UTC · re: Jacob Keller · lore

Re: [PATCH 00/17] object_id part 14

On 7/8/2018 11:12 PM, Jacob Keller wrote:
Show 13 quoted lines
> On Sun, Jul 8, 2018 at 4:39 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
>> This is the fourteenth series of patches to switch to using struct
>> object_id and the_hash_algo.  This series converts several core pieces
>> to use struct object_id, including the oid* and hex functions.
>>
>> All of these patches have been tested with both SHA-1 and a 256-bit
>> hash.
>>
> I read through the series, and didn't spot anything odd, except for
> the question about reasoning for why we use memcmp directly over using
> hashcmp. I don't think that's any sort of blocker, it just seemed an
> odd decision to me.

I also read through the series and only found the 100/200 constants confusing. Not worth blocking on, but I'm CC'ing Michael Haggerty to comment if he knows how the magic 100 was computed.

Thanks, -Stolee

Michael Haggerty· Jul 14, 2018, 23:38 UTC · re: Derrick Stolee · lore

Re: [PATCH 00/17] object_id part 14

On Mon, Jul 9, 2018 at 6:15 AM Derrick Stolee <stolee@gmail.com> wrote:
Show 18 quoted lines
> On 7/8/2018 11:12 PM, Jacob Keller wrote:
> > On Sun, Jul 8, 2018 at 4:39 PM brian m. carlson
> > <sandals@crustytoothpaste.net> wrote:
> >> This is the fourteenth series of patches to switch to using struct
> >> object_id and the_hash_algo.  This series converts several core pieces
> >> to use struct object_id, including the oid* and hex functions.
> >>
> >> All of these patches have been tested with both SHA-1 and a 256-bit
> >> hash.
> >>
> > I read through the series, and didn't spot anything odd, except for
> > the question about reasoning for why we use memcmp directly over using
> > hashcmp. I don't think that's any sort of blocker, it just seemed an
> > odd decision to me.
>
> I also read through the series and only found the 100/200 constants
> confusing. Not worth blocking on, but I'm CC'ing Michael Haggerty to
> comment if he knows how the magic 100 was computed.
The magic 100 blames back to our chief magician, Junio:
    8ac65937d0 Make sure we do not write bogus reflog entries. (2007-01-26)

Since then, as far as I can tell, it's just been copy-pasted forward. It would be easy to compute it precisely based on the length of the two OIDs, represented as hex strings, plus the few extra characters in the format string.

Michael
Junio C Hamano· Jul 16, 2018, 21:06 UTC · re: Michael Haggerty · lore

Re: [PATCH 00/17] object_id part 14

Michael Haggerty <mhagger@alum.mit.edu> writes:
> The magic 100 blames back to our chief magician, Junio:
>
>     8ac65937d0 Make sure we do not write bogus reflog entries. (2007-01-26)
Yup, guilty as charged.
cf. <xmqqva9oe20y.fsf@gitster-ct.c.googlers.com>
    "%s %s %s\n" with old and new commit object name and the message
    will be "2 * len(hash_in_hex) + 4" bytes long (counting the three
    whitespaces and the terminating NUL), and Shawn's original in
    6de08ae6 ("Log ref updates to logs/refs/<ref>", 2006-05-17) actually
    computed this one as "strlen(...) + 2*40+4".
    100 was merely me being sloppier than Shawn at 8ac65937 ("Make sure
    we do not write bogus reflog entries.", 2007-01-26), preferring
    being sufficient over not wasting even a single byte.

← back to recent threads