[PATCH 1/3] notes: move hex_to_bytes() to hex.c and export it

Make the function for converting pairs of hexadecimal digits to binary
available to other call sites.

Signed-off-by: Rene Scharfe <redacted>
---
 cache.h |  7 +++++++
 hex.c   | 12 ++++++++++++
 notes.c | 17 -----------------
 3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/cache.h b/cache.h
index 6440e2bf21..f06bfbaf32 100644
--- a/cache.h
+++ b/cache.h

@@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char *var, const char *value);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern int get_oid_hex(const char *hex, struct object_id *sha1);
 
+/*
+ * Read `len` pairs of hexadecimal digits from `hex` and write the
+ * values to `binary` as `len` bytes. Return 0 on success, or -1 if
+ * the input does not consist of hex digits).
+ */
+extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);
+
 /*
  * Convert a binary sha1 to its hex equivalent. The `_r` variant is reentrant,
  * and writes the NUL-terminated output to the buffer `out`, which must be at

diff --git a/hex.c b/hex.c
index 28b44118cb..8df2d63728 100644
--- a/hex.c
+++ b/hex.c

@@ -35,6 +35,18 @@ const signed char hexval_table[256] = {
 	 -1, -1, -1, -1, -1, -1, -1, -1,		/* f8-ff */
 };
 
+int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
+{
+	for (; len; len--, hex += 2) {
+		unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
+
+		if (val & ~0xff)
+			return -1;
+		*binary++ = val;
+	}
+	return 0;
+}
+
 int get_sha1_hex(const char *hex, unsigned char *sha1)
 {
 	int i;

diff --git a/notes.c b/notes.c
index 5c62862574..04f8c8613c 100644
--- a/notes.c
+++ b/notes.c

@@ -334,23 +334,6 @@ static void note_tree_free(struct int_node *tree)
 	}
 }
 
-/*
- * Read `len` pairs of hexadecimal digits from `hex` and write the
- * values to `binary` as `len` bytes. Return 0 on success, or -1 if
- * the input does not consist of hex digits).
- */
-static int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
-{
-	for (; len; len--, hex += 2) {
-		unsigned int val = (hexval(hex[0]) << 4) | hexval(hex[1]);
-
-		if (val & ~0xff)
-			return -1;
-		*binary++ = val;
-	}
-	return 0;
-}
-
 static int non_note_cmp(const struct non_note *a, const struct non_note *b)
 {
 	return strcmp(a->path, b->path);

-- 
2.15.0

The path of a loose object contains its hash value encoded into two
substrings of hexadecimal digits, separated by a slash.  The current
code copies the pieces into a temporary buffer to get rid of the slash
and then uses get_oid_hex() to decode the hash value.

Avoid the copy by using hex_to_bytes() directly on the substrings.
That's shorter and easier.

While at it correct the length of the second substring in a comment.

Signed-off-by: Rene Scharfe <redacted>
---
 http-push.c | 10 ++++------
 1 file changed, 4 insertions(+), 6 deletions(-)

diff --git a/http-push.c b/http-push.c
index 493ee7d719..14435ab65d 100644
--- a/http-push.c
+++ b/http-push.c

@@ -1007,20 +1007,18 @@ static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData);
 
-/* extract hex from sharded "xx/x{40}" filename */
+/* extract hex from sharded "xx/x{38}" filename */
 static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
 {
-	char hex[GIT_MAX_HEXSZ];
-
 	if (strlen(path) != GIT_SHA1_HEXSZ + 1)
 		return -1;
 
-	memcpy(hex, path, 2);
+	if (hex_to_bytes(oid->hash, path, 1))
+		return -1;
 	path += 2;
 	path++; /* skip '/' */
-	memcpy(hex + 2, path, GIT_SHA1_HEXSZ - 2);
 
-	return get_oid_hex(hex, oid);
+	return hex_to_bytes(oid->hash + 1, path, GIT_SHA1_RAWSZ - 1);
 }
 
 static void process_ls_object(struct remote_ls_ctx *ls)

-- 
2.15.0

On Tue, Oct 31, 2017 at 02:49:56PM +0100, René Scharfe wrote:

The path of a loose object contains its hash value encoded into two
substrings of hexadecimal digits, separated by a slash.  The current
code copies the pieces into a temporary buffer to get rid of the slash
and then uses get_oid_hex() to decode the hash value.

Avoid the copy by using hex_to_bytes() directly on the substrings.
That's shorter and easier.

While at it correct the length of the second substring in a comment.

I think the patch is correct, but I wonder if this approach will bite us
later on when we start dealing with multiple lengths of hashes.

The hex_to_bytes() function requires that the caller make sure they have
the right number of bytes. But for many callers, I think they'd want to
say "parse this oid, which might be truncated; I can't tell what the
length is supposed to be".

I.e., I wonder if the right primitive is really something like
parse_oid_hex(), but with a flag to say "skip over interior slashes".

We don't need to deal with that eventuality yet, but I'm on the fence on
whether this patch is making that harder down the road or not. The
current strategy of "stuff it into a buffer without slashes" would be
easier to convert, I think.

-Peff

Am 01.11.2017 um 20:55 schrieb Jeff King:

On Tue, Oct 31, 2017 at 02:49:56PM +0100, René Scharfe wrote:

quoted

The path of a loose object contains its hash value encoded into two
substrings of hexadecimal digits, separated by a slash.  The current
code copies the pieces into a temporary buffer to get rid of the slash
and then uses get_oid_hex() to decode the hash value.

Avoid the copy by using hex_to_bytes() directly on the substrings.
That's shorter and easier.

While at it correct the length of the second substring in a comment.

I think the patch is correct, but I wonder if this approach will bite us
later on when we start dealing with multiple lengths of hashes.

The hex_to_bytes() function requires that the caller make sure they have
the right number of bytes. But for many callers, I think they'd want to
say "parse this oid, which might be truncated; I can't tell what the
length is supposed to be".

I'm confused by the word "many".  After this series there are three
callers of hex_to_bytes() and I don't expect that number to grow.

Would loose objects be stored at paths containing only a subset of their
new hash value?  If they won't then there will be two acceptable lengths
instead of the one we have today, which should be easy to handle.

I.e., I wonder if the right primitive is really something like
parse_oid_hex(), but with a flag to say "skip over interior slashes".

Hmm, ignoring any slashes is an interesting idea.  Is that too loose, I
wonder?  And I don't think it makes for a good primitive, as slashes
only need to be ignored in a single place so far (here in http-push.c).
Collecting special cases in a central place, guarded by flags, doesn't
reduce the overall complexity.

We don't need to deal with that eventuality yet, but I'm on the fence on
whether this patch is making that harder down the road or not. The
current strategy of "stuff it into a buffer without slashes" would be
easier to convert, I think.

How so?  If you have a buffer then you need to know the size of the
data to copy into it as well, or you'll learn it in the process.

The call sites of hex_to_bytes() have to be modified along with the
functions in hex.c to support longer hashes, with or without this
series.

René

On Wed, Nov 01, 2017 at 10:59:49PM +0100, René Scharfe wrote:

quoted

The hex_to_bytes() function requires that the caller make sure they have
the right number of bytes. But for many callers, I think they'd want to
say "parse this oid, which might be truncated; I can't tell what the
length is supposed to be".

I'm confused by the word "many".  After this series there are three
callers of hex_to_bytes() and I don't expect that number to grow.

I meant only that most callers that parse oids, both in-file and not,
would want to stop knowing about the length ahead of time. I think
parse_oid_hex() solves that problem for most callers.

Would loose objects be stored at paths containing only a subset of their
new hash value?  If they won't then there will be two acceptable lengths
instead of the one we have today, which should be easy to handle.

I don't know. TBH, I'm not sure anyone has much interest in making
http-push work with new hashes. I'd be OK if it simply doesn't until
somebody interested shows up to change that.

quoted

We don't need to deal with that eventuality yet, but I'm on the fence on
whether this patch is making that harder down the road or not. The
current strategy of "stuff it into a buffer without slashes" would be
easier to convert, I think.

How so?  If you have a buffer then you need to know the size of the
data to copy into it as well, or you'll learn it in the process.

The call sites of hex_to_bytes() have to be modified along with the
functions in hex.c to support longer hashes, with or without this
series.

You have to know how big the data you have is, but you don't necessarily
know whether that makes a complete hash or not. With a "remove slashes
and then parse" strategy, you can do the removing without worrying about
how big things are _supposed_ to be, and then the parser can tell you if
you have a valid oid or not. The logic for what a hash looks like _and_
how big it must be are both in the parser.

With the new code you have here, we have to be a bit more intimate with
SHA1_HEXSZ in the calling code. It knows that the hash consists of a
certain number of hex bytes.

I'm perfectly willing to punt on it for now. I'm not sure we know 100%
yet what "new"-style hashes will look like, nor how their loose-object
filenames would look.

-Peff

Am 01.11.2017 um 23:15 schrieb Jeff King:

On Wed, Nov 01, 2017 at 10:59:49PM +0100, René Scharfe wrote:

quoted

quoted

The hex_to_bytes() function requires that the caller make sure they have
the right number of bytes. But for many callers, I think they'd want to
say "parse this oid, which might be truncated; I can't tell what the
length is supposed to be".

I'm confused by the word "many".  After this series there are three
callers of hex_to_bytes() and I don't expect that number to grow.

I meant only that most callers that parse oids, both in-file and not,
would want to stop knowing about the length ahead of time.

That's a good idea.

I'm not sure we know 100%
yet what "new"-style hashes will look like, nor how their loose-object
filenames would look.

So it's too early to implement a solution, but here's how a patch for
reducing dependency on hash lengths could look like today:

---
 cache.h     |  2 ++
 hex.c       | 13 +++++++++++++
 http-push.c |  7 +++----
 notes.c     |  6 +-----
 sha1_file.c |  4 +---
 5 files changed, 20 insertions(+), 12 deletions(-)

diff --git a/cache.h b/cache.h
index f06bfbaf32..acd3804c21 100644
--- a/cache.h
+++ b/cache.h

@@ -1317,6 +1317,8 @@ extern int set_disambiguate_hint_config(const char *var, const char *value);
 extern int get_sha1_hex(const char *hex, unsigned char *sha1);
 extern int get_oid_hex(const char *hex, struct object_id *sha1);
 
+extern int get_oid_hex_tail(const char *hex, struct object_id *oid, size_t offset);
+
 /*
  * Read `len` pairs of hexadecimal digits from `hex` and write the
  * values to `binary` as `len` bytes. Return 0 on success, or -1 if

diff --git a/hex.c b/hex.c
index 8df2d63728..3e6abe4d5e 100644
--- a/hex.c
+++ b/hex.c

@@ -47,6 +47,19 @@ int hex_to_bytes(unsigned char *binary, const char *hex, size_t len)
 	return 0;
 }
 
+int get_oid_hex_tail(const char *hex, struct object_id *oid, size_t offset)
+{
+	for (; offset < GIT_SHA1_RAWSZ; offset++, hex += 2) {
+		int val = hex2chr(hex);
+		if (val < 0)
+			return -1;
+		oid->hash[offset] = val;
+	}
+	if (*hex)
+		return -1;
+	return 0;
+}
+
 int get_sha1_hex(const char *hex, unsigned char *sha1)
 {
 	int i;

diff --git a/http-push.c b/http-push.c
index 14435ab65d..a5512616b9 100644
--- a/http-push.c
+++ b/http-push.c

@@ -1007,18 +1007,17 @@ static void remote_ls(const char *path, int flags,
 		      void (*userFunc)(struct remote_ls_ctx *ls),
 		      void *userData);
 
-/* extract hex from sharded "xx/x{38}" filename */
+/* extract hex from sharded "xx/x{N}" filename */
 static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
 {
-	if (strlen(path) != GIT_SHA1_HEXSZ + 1)
+	if (strnlen(path, 3) < 3)
 		return -1;
-
 	if (hex_to_bytes(oid->hash, path, 1))
 		return -1;
 	path += 2;
 	path++; /* skip '/' */
 
-	return hex_to_bytes(oid->hash + 1, path, GIT_SHA1_RAWSZ - 1);
+	return get_oid_hex_tail(path, oid, 1);
 }
 
 static void process_ls_object(struct remote_ls_ctx *ls)

diff --git a/notes.c b/notes.c
index 04f8c8613c..ff6ce57022 100644
--- a/notes.c
+++ b/notes.c

@@ -410,17 +410,13 @@ static void load_subtree(struct notes_tree *t, struct leaf_node *subtree,
 		struct leaf_node *l;
 		size_t path_len = strlen(entry.path);
 
-		if (path_len == 2 * (GIT_SHA1_RAWSZ - prefix_len)) {
+		if (!get_oid_hex_tail(entry.path, &object_oid, prefix_len)) {
 			/* This is potentially the remainder of the SHA-1 */
 
 			if (!S_ISREG(entry.mode))
 				/* notes must be blobs */
 				goto handle_non_note;
 
-			if (hex_to_bytes(object_oid.hash + prefix_len, entry.path,
-					 GIT_SHA1_RAWSZ - prefix_len))
-				goto handle_non_note; /* entry.path is not a SHA1 */
-
 			type = PTR_TYPE_NOTE;
 		} else if (path_len == 2) {
 			/* This is potentially an internal node */

diff --git a/sha1_file.c b/sha1_file.c
index a3c32d91d1..0486696b0b 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -1911,9 +1911,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		strbuf_setlen(path, baselen);
 		strbuf_addf(path, "/%s", de->d_name);
 
-		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&
-		    !hex_to_bytes(oid.hash + 1, de->d_name,
-				  GIT_SHA1_RAWSZ - 1)) {
+		if (!get_oid_hex_tail(de->d_name, &oid, 1)) {
 			if (obj_cb) {
 				r = obj_cb(&oid, path->buf, data);
 				if (r)

-- 
2.15.0

The path of a loose object contains its hash value encoded into two
substrings of 2 and 38 hexadecimal digits separated by a slash.  The
first part is handed to for_each_file_in_obj_subdir() in decoded form as
subdir_nr.  The current code builds a full hexadecimal representation of
the hash in a temporary buffer, then uses get_oid_hex() to decode it.

Avoid the intermediate step by taking subdir_nr as-is and using
hex_to_bytes() directly on the second substring.  That's shorter and
easier.

Signed-off-by: Rene Scharfe <redacted>
---
 sha1_file.c | 24 +++++++++++-------------
 1 file changed, 11 insertions(+), 13 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 10c3a0083d..a3c32d91d1 100644
--- a/sha1_file.c
+++ b/sha1_file.c

@@ -1884,6 +1884,7 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 	DIR *dir;
 	struct dirent *de;
 	int r = 0;
+	struct object_id oid;
 
 	if (subdir_nr > 0xff)
 		BUG("invalid loose object subdirectory: %x", subdir_nr);

@@ -1901,6 +1902,8 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		return r;
 	}
 
+	oid.hash[0] = subdir_nr;
+
 	while ((de = readdir(dir))) {
 		if (is_dot_or_dotdot(de->d_name))
 			continue;

@@ -1908,20 +1911,15 @@ int for_each_file_in_obj_subdir(unsigned int subdir_nr,
 		strbuf_setlen(path, baselen);
 		strbuf_addf(path, "/%s", de->d_name);
 
-		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {
-			char hex[GIT_MAX_HEXSZ+1];
-			struct object_id oid;
-
-			xsnprintf(hex, sizeof(hex), "%02x%s",
-				  subdir_nr, de->d_name);
-			if (!get_oid_hex(hex, &oid)) {
-				if (obj_cb) {
-					r = obj_cb(&oid, path->buf, data);
-					if (r)
-						break;
-				}
-				continue;
+		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2 &&
+		    !hex_to_bytes(oid.hash + 1, de->d_name,
+				  GIT_SHA1_RAWSZ - 1)) {
+			if (obj_cb) {
+				r = obj_cb(&oid, path->buf, data);
+				if (r)
+					break;
 			}
+			continue;
 		}
 
 		if (cruft_cb) {

-- 
2.15.0

On Tue, Oct 31, 2017 at 02:50:06PM +0100, René Scharfe wrote:

The path of a loose object contains its hash value encoded into two
substrings of 2 and 38 hexadecimal digits separated by a slash.  The
first part is handed to for_each_file_in_obj_subdir() in decoded form as
subdir_nr.  The current code builds a full hexadecimal representation of
the hash in a temporary buffer, then uses get_oid_hex() to decode it.

Avoid the intermediate step by taking subdir_nr as-is and using
hex_to_bytes() directly on the second substring.  That's shorter and
easier.

This raises some of the same questions as the previous one on whether
hex_to_bytes() is the ideal abstraction. But as before, I'm on the
fence.

Now that this is one big conditional for "is this a valid object
filename", I think we could get rid of the "continue" in favor of:

  if (...looks like an object...)
          ...call obj_cb...
  else if (cruft_cb)
          ...call cruft_cb...

Not a big deal, but it may make the flow more clear (the original had to
use a continue because there were multiple independent steps to
determining it was an object file, so we had to "break out" from the
inner conditional).

-Peff

On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:

Is it correct to call the result binary? I would say that it's the value
that gets stored. To me, this value does not really have a base.

Kevin

Am 05.11.2017 um 03:56 schrieb Kevin Daudt:

On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:

quoted

Make the function for converting pairs of hexadecimal digits to binary
available to other call sites.

Signed-off-by: Rene Scharfe <redacted>
---
  cache.h |  7 +++++++
  hex.c   | 12 ++++++++++++
  notes.c | 17 -----------------
  3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/cache.h b/cache.h
index 6440e2bf21..f06bfbaf32 100644
--- a/cache.h
+++ b/cache.h

@@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char *var, const char *value);
  extern int get_sha1_hex(const char *hex, unsigned char *sha1);
  extern int get_oid_hex(const char *hex, struct object_id *sha1);
  
+/*
+ * Read `len` pairs of hexadecimal digits from `hex` and write the
+ * values to `binary` as `len` bytes. Return 0 on success, or -1 if

Is it correct to call the result binary? I would say that it's the value
that gets stored. To me, this value does not really have a base.

Here's the full context:

  /*
   * Read `len` pairs of hexadecimal digits from `hex` and write the
   * values to `binary` as `len` bytes. Return 0 on success, or -1 if
   * the input does not consist of hex digits).
   */
  extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);

The patch moves the comment verbatim.  Words in backticks (`binary`,
`hex`, `len`) are parameter names.

The function converts pairs of hexadecimal digits (base 16, ASCII
encoded) to bytes (base 256).  A byte can be seen as an array of bits;
thus the output is also binary (base 2) without requiring further
conversion.

Calling the variable "binary" may seem unspecific, but makes sense in
the context of this function.

Does any of that help?

Thanks,
René

On Sun, Nov 05, 2017 at 05:47:47PM +0100, René Scharfe wrote:

Am 05.11.2017 um 03:56 schrieb Kevin Daudt:

quoted

On Tue, Oct 31, 2017 at 02:46:49PM +0100, René Scharfe wrote:

quoted

Make the function for converting pairs of hexadecimal digits to binary
available to other call sites.

Signed-off-by: Rene Scharfe <redacted>
---
  cache.h |  7 +++++++
  hex.c   | 12 ++++++++++++
  notes.c | 17 -----------------
  3 files changed, 19 insertions(+), 17 deletions(-)

diff --git a/cache.h b/cache.h
index 6440e2bf21..f06bfbaf32 100644
--- a/cache.h
+++ b/cache.h

@@ -1317,6 +1317,13 @@ extern int set_disambiguate_hint_config(const char *var, const char *value);
  extern int get_sha1_hex(const char *hex, unsigned char *sha1);
  extern int get_oid_hex(const char *hex, struct object_id *sha1);
  
+/*
+ * Read `len` pairs of hexadecimal digits from `hex` and write the
+ * values to `binary` as `len` bytes. Return 0 on success, or -1 if

Is it correct to call the result binary? I would say that it's the value
that gets stored. To me, this value does not really have a base.

Here's the full context:

  /*
   * Read `len` pairs of hexadecimal digits from `hex` and write the
   * values to `binary` as `len` bytes. Return 0 on success, or -1 if
   * the input does not consist of hex digits).
   */
  extern int hex_to_bytes(unsigned char *binary, const char *hex, size_t len);

The patch moves the comment verbatim.  Words in backticks (`binary`,
`hex`, `len`) are parameter names.

The function converts pairs of hexadecimal digits (base 16, ASCII
encoded) to bytes (base 256).  A byte can be seen as an array of bits;
thus the output is also binary (base 2) without requiring further
conversion.

Calling the variable "binary" may seem unspecific, but makes sense in
the context of this function.

Does any of that help?

Thanks,
René

Thanks, I have been thinking about it more, and I agree, it does make
sense. 

I had a binary representation in mind, but this is refering to binary
data (just like you can have binary files).

Kevin