git@vger.kernel.org mailing list mirror (one of many)
 help / Atom feed
* [PATCH 00/20] object_id part 7
@ 2017-03-18 21:19 brian m. carlson
  2017-03-18 21:19 ` [PATCH 01/20] Define new hash-size constants for allocating memory brian m. carlson
                   ` (11 more replies)
  0 siblings, 12 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

This is part 7 in the continuing transition to use struct object_id.

This series focuses on two main areas: adding two constants for the
maximum hash size we'll be using (which will be suitable for allocating
memory) and converting struct sha1_array to struct oid_array.

The rationale for adding separate constants for allocating memory is
that with a new 256-bit hash function, we're going to need two different
items: a constant for allocating memory that's as large as the largest
hash, and a global variable telling us size the current hash is.  I've
opted to provide GIT_MAX_RAWSZ and GIT_MAX_HEXSZ for allocating memory,
and leave GIT_SHA1_RAWSZ and GIT_SHA1_HEXSZ as values that can be later
replaced by the aforementioned global.

Replacing struct sha1_array with struct oid_array necessarily involves
converting the shallow code, so I did that.  The structure now handles
objects of struct object_id.  While I renamed the documentation (since
people will search for that), I chose not to rename the sha1-array.[ch]
files or the test helper because I didn't think it was worth the hassle,
especially for people who don't have rename support turned on by
default.  Of course, if the consensus is that they should be, I'll do so
in v2.

I chose to use Coccinelle quite a bit in this series, as it automates a
lot of the manual work and aides in review.  There is also some use of
Perl one-liners.

This series is available at https://github.com/bk2204/git under
object-id-part7; it may be rebased.  Future series are available in
various states as the object-id-part8, object-id-part9, and
object-id-part10 series.

brian m. carlson (20):
  Define new hash-size constants for allocating memory
  Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ
  Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ
  builtin/diff: convert to struct object_id
  builtin/pull: convert portions to struct object_id
  builtin/receive-pack: convert portions to struct object_id
  fsck: convert init_skiplist to struct object_id
  parse-options-cb: convert sha1_array_append caller to struct object_id
  test-sha1-array: convert most code to struct object_id
  sha1_name: convert struct disambiguate_state to object_id
  sha1_name: convert disambiguate_hint_fn to take object_id
  submodule: convert check_for_new_submodule_commits to object_id
  builtin/pull: convert to struct object_id
  sha1-array: convert internal storage for struct sha1_array to
    object_id
  Make sha1_array_append take a struct object_id *
  Convert remaining callers of sha1_array_lookup to object_id
  Convert sha1_array_lookup to take struct object_id
  Convert sha1_array_for_each_unique and for_each_abbrev to object_id
  Rename sha1_array to oid_array
  Documentation: update and rename api-sha1-array.txt

 .../{api-sha1-array.txt => api-oid-array.txt}      |  44 +++----
 bisect.c                                           |  43 ++++---
 builtin/blame.c                                    |   4 +-
 builtin/cat-file.c                                 |  14 +--
 builtin/diff.c                                     |  40 +++----
 builtin/fetch-pack.c                               |   2 +-
 builtin/fetch.c                                    |   6 +-
 builtin/merge-index.c                              |   2 +-
 builtin/merge.c                                    |   2 +-
 builtin/pack-objects.c                             |  24 ++--
 builtin/patch-id.c                                 |   2 +-
 builtin/pull.c                                     |  98 +++++++--------
 builtin/receive-pack.c                             | 133 +++++++++++----------
 builtin/rev-list.c                                 |   2 +-
 builtin/rev-parse.c                                |   4 +-
 builtin/send-pack.c                                |   4 +-
 cache.h                                            |  10 +-
 combine-diff.c                                     |  18 +--
 commit.h                                           |  14 +--
 connect.c                                          |   8 +-
 diff.c                                             |   4 +-
 diff.h                                             |   4 +-
 fetch-pack.c                                       |  32 ++---
 fetch-pack.h                                       |   4 +-
 fsck.c                                             |  17 +--
 fsck.h                                             |   2 +-
 hex.c                                              |   2 +-
 parse-options-cb.c                                 |   8 +-
 patch-ids.c                                        |   2 +-
 patch-ids.h                                        |   2 +-
 ref-filter.c                                       |  22 ++--
 ref-filter.h                                       |   2 +-
 remote-curl.c                                      |   4 +-
 remote.h                                           |   6 +-
 send-pack.c                                        |   6 +-
 send-pack.h                                        |   2 +-
 sha1-array.c                                       |  38 +++---
 sha1-array.h                                       |  20 ++--
 sha1_file.c                                        |   6 +-
 sha1_name.c                                        |  94 ++++++++-------
 shallow.c                                          |  38 +++---
 submodule.c                                        |  66 +++++-----
 submodule.h                                        |   8 +-
 t/helper/test-sha1-array.c                         |  20 ++--
 transport.c                                        |  24 ++--
 wt-status.h                                        |   2 +-
 46 files changed, 459 insertions(+), 450 deletions(-)
 rename Documentation/technical/{api-sha1-array.txt => api-oid-array.txt} (61%)


^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 01/20] Define new hash-size constants for allocating memory
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
@ 2017-03-18 21:19 ` brian m. carlson
  2017-03-18 21:19 ` [PATCH 02/20] Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ brian m. carlson
                   ` (10 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Since we will want to transition to a new hash at some point in the
future, and that hash may be larger in size than 160 bits, introduce two
constants that can be used for allocating a sufficient amount of memory.
They can be increased to reflect the largest supported hash size.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 cache.h | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 9b2157f591..cb301d8d7d 100644
--- a/cache.h
+++ b/cache.h
@@ -66,8 +66,12 @@ unsigned long git_deflate_bound(git_zstream *, unsigned long);
 #define GIT_SHA1_RAWSZ 20
 #define GIT_SHA1_HEXSZ (2 * GIT_SHA1_RAWSZ)
 
+/* The length in byte and in hex digits of the largest possible hash value. */
+#define GIT_MAX_RAWSZ GIT_SHA1_RAWSZ
+#define GIT_MAX_HEXSZ GIT_SHA1_HEXSZ
+
 struct object_id {
-	unsigned char hash[GIT_SHA1_RAWSZ];
+	unsigned char hash[GIT_MAX_RAWSZ];
 };
 
 #if defined(DT_UNKNOWN) && !defined(NO_D_TYPE_IN_DIRENT)

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 02/20] Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
  2017-03-18 21:19 ` [PATCH 01/20] Define new hash-size constants for allocating memory brian m. carlson
@ 2017-03-18 21:19 ` brian m. carlson
  2017-03-18 21:19 ` [PATCH 03/20] Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ brian m. carlson
                   ` (9 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Since we will likely be introducing a new hash function at some point,
and that hash function might be longer than 40 hex characters, use the
constant GIT_MAX_HEXSZ, which is designed to be suitable for
allocations, instead of GIT_SHA1_HEXSZ.  This will ease the transition
down the line by distinguishing between places where we need to allocate
memory suitable for the largest hash from those where we need to handle
the current hash.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 bisect.c              | 2 +-
 builtin/blame.c       | 4 ++--
 builtin/merge-index.c | 2 +-
 builtin/merge.c       | 2 +-
 builtin/rev-list.c    | 2 +-
 diff.c                | 4 ++--
 hex.c                 | 2 +-
 sha1_file.c           | 2 +-
 sha1_name.c           | 6 +++---
 transport.c           | 2 +-
 10 files changed, 14 insertions(+), 14 deletions(-)

diff --git a/bisect.c b/bisect.c
index 30808cadf7..21c3e34636 100644
--- a/bisect.c
+++ b/bisect.c
@@ -682,7 +682,7 @@ static int is_expected_rev(const struct object_id *oid)
 
 static int bisect_checkout(const unsigned char *bisect_rev, int no_checkout)
 {
-	char bisect_rev_hex[GIT_SHA1_HEXSZ + 1];
+	char bisect_rev_hex[GIT_MAX_HEXSZ + 1];
 
 	memcpy(bisect_rev_hex, sha1_to_hex(bisect_rev), GIT_SHA1_HEXSZ + 1);
 	update_ref(NULL, "BISECT_EXPECTED_REV", bisect_rev, NULL, 0, UPDATE_REFS_DIE_ON_ERR);
diff --git a/builtin/blame.c b/builtin/blame.c
index f7aa95f4ba..07506a3e45 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1890,7 +1890,7 @@ static void emit_porcelain(struct scoreboard *sb, struct blame_entry *ent,
 	int cnt;
 	const char *cp;
 	struct origin *suspect = ent->suspect;
-	char hex[GIT_SHA1_HEXSZ + 1];
+	char hex[GIT_MAX_HEXSZ + 1];
 
 	oid_to_hex_r(hex, &suspect->commit->object.oid);
 	printf("%s %d %d %d\n",
@@ -1928,7 +1928,7 @@ static void emit_other(struct scoreboard *sb, struct blame_entry *ent, int opt)
 	const char *cp;
 	struct origin *suspect = ent->suspect;
 	struct commit_info ci;
-	char hex[GIT_SHA1_HEXSZ + 1];
+	char hex[GIT_MAX_HEXSZ + 1];
 	int show_raw_time = !!(opt & OUTPUT_RAW_TIMESTAMP);
 
 	get_commit_info(suspect->commit, &ci, 1);
diff --git a/builtin/merge-index.c b/builtin/merge-index.c
index 2d1b6db6bd..c99443b095 100644
--- a/builtin/merge-index.c
+++ b/builtin/merge-index.c
@@ -9,7 +9,7 @@ static int merge_entry(int pos, const char *path)
 {
 	int found;
 	const char *arguments[] = { pgm, "", "", "", path, "", "", "", NULL };
-	char hexbuf[4][GIT_SHA1_HEXSZ + 1];
+	char hexbuf[4][GIT_MAX_HEXSZ + 1];
 	char ownbuf[4][60];
 
 	if (pos >= active_nr)
diff --git a/builtin/merge.c b/builtin/merge.c
index 7554b8d412..a2cceea3fb 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1296,7 +1296,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (verify_signatures) {
 		for (p = remoteheads; p; p = p->next) {
 			struct commit *commit = p->item;
-			char hex[GIT_SHA1_HEXSZ + 1];
+			char hex[GIT_MAX_HEXSZ + 1];
 			struct signature_check signature_check;
 			memset(&signature_check, 0, sizeof(signature_check));
 
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0aa93d5891..bcf77f0b8a 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -212,7 +212,7 @@ static void print_var_int(const char *var, int val)
 static int show_bisect_vars(struct rev_list_info *info, int reaches, int all)
 {
 	int cnt, flags = info->flags;
-	char hex[GIT_SHA1_HEXSZ + 1] = "";
+	char hex[GIT_MAX_HEXSZ + 1] = "";
 	struct commit_list *tried;
 	struct rev_info *revs = info->revs;
 
diff --git a/diff.c b/diff.c
index a628ac3a95..330b640c68 100644
--- a/diff.c
+++ b/diff.c
@@ -398,7 +398,7 @@ static struct diff_tempfile {
 	 */
 	const char *name;
 
-	char hex[GIT_SHA1_HEXSZ + 1];
+	char hex[GIT_MAX_HEXSZ + 1];
 	char mode[10];
 
 	/*
@@ -4219,7 +4219,7 @@ const char *diff_aligned_abbrev(const struct object_id *oid, int len)
 	 * uniqueness across all objects (statistically speaking).
 	 */
 	if (abblen < GIT_SHA1_HEXSZ - 3) {
-		static char hex[GIT_SHA1_HEXSZ + 1];
+		static char hex[GIT_MAX_HEXSZ + 1];
 		if (len < abblen && abblen <= len + 2)
 			xsnprintf(hex, sizeof(hex), "%s%.*s", abbrev, len+3-abblen, "..");
 		else
diff --git a/hex.c b/hex.c
index eab7b626ee..28b44118cb 100644
--- a/hex.c
+++ b/hex.c
@@ -85,7 +85,7 @@ char *oid_to_hex_r(char *buffer, const struct object_id *oid)
 char *sha1_to_hex(const unsigned char *sha1)
 {
 	static int bufno;
-	static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
+	static char hexbuffer[4][GIT_MAX_HEXSZ + 1];
 	bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
 	return sha1_to_hex_r(hexbuffer[bufno], sha1);
 }
diff --git a/sha1_file.c b/sha1_file.c
index 29bbc5f427..cc6b93c8a9 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3764,7 +3764,7 @@ static int for_each_file_in_obj_subdir(int subdir_nr,
 		strbuf_addf(path, "/%s", de->d_name);
 
 		if (strlen(de->d_name) == GIT_SHA1_HEXSZ - 2)  {
-			char hex[GIT_SHA1_HEXSZ+1];
+			char hex[GIT_MAX_HEXSZ+1];
 			struct object_id oid;
 
 			snprintf(hex, sizeof(hex), "%02x%s",
diff --git a/sha1_name.c b/sha1_name.c
index cda9e49b12..964201bc26 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -15,7 +15,7 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
 
 struct disambiguate_state {
 	int len; /* length of prefix in hex chars */
-	char hex_pfx[GIT_SHA1_HEXSZ + 1];
+	char hex_pfx[GIT_MAX_HEXSZ + 1];
 	unsigned char bin_pfx[GIT_SHA1_RAWSZ];
 
 	disambiguate_hint_fn fn;
@@ -80,7 +80,7 @@ static void update_candidates(struct disambiguate_state *ds, const unsigned char
 static void find_short_object_filename(struct disambiguate_state *ds)
 {
 	struct alternate_object_database *alt;
-	char hex[GIT_SHA1_HEXSZ];
+	char hex[GIT_MAX_HEXSZ];
 	static struct alternate_object_database *fakeent;
 
 	if (!fakeent) {
@@ -509,7 +509,7 @@ int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len)
 const char *find_unique_abbrev(const unsigned char *sha1, int len)
 {
 	static int bufno;
-	static char hexbuffer[4][GIT_SHA1_HEXSZ + 1];
+	static char hexbuffer[4][GIT_MAX_HEXSZ + 1];
 	char *hex = hexbuffer[bufno];
 	bufno = (bufno + 1) % ARRAY_SIZE(hexbuffer);
 	find_unique_abbrev_r(hex, sha1, len);
diff --git a/transport.c b/transport.c
index 417ed7f19f..8a90b0c29b 100644
--- a/transport.c
+++ b/transport.c
@@ -447,7 +447,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
 
 static int measure_abbrev(const struct object_id *oid, int sofar)
 {
-	char hex[GIT_SHA1_HEXSZ + 1];
+	char hex[GIT_MAX_HEXSZ + 1];
 	int w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV);
 
 	return (w < sofar) ? sofar : w;

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 03/20] Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
  2017-03-18 21:19 ` [PATCH 01/20] Define new hash-size constants for allocating memory brian m. carlson
  2017-03-18 21:19 ` [PATCH 02/20] Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ brian m. carlson
@ 2017-03-18 21:19 ` brian m. carlson
  2017-03-18 21:19 ` [PATCH 05/20] builtin/pull: convert portions to struct object_id brian m. carlson
                   ` (8 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Since we will likely be introducing a new hash function at some point,
and that hash function might be longer than 20 bytes, use the constant
GIT_MAX_RAWSZ, which is designed to be suitable for allocations, instead
of GIT_SHA1_RAWSZ.  This will ease the transition down the line by
distinguishing between places where we need to allocate memory suitable
for the largest hash from those where we need to handle the current
hash.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/patch-id.c     | 2 +-
 builtin/receive-pack.c | 2 +-
 cache.h                | 2 +-
 patch-ids.c            | 2 +-
 patch-ids.h            | 2 +-
 sha1_file.c            | 4 ++--
 sha1_name.c            | 4 ++--
 wt-status.h            | 2 +-
 8 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/builtin/patch-id.c b/builtin/patch-id.c
index a84d0003a3..81552e02e4 100644
--- a/builtin/patch-id.c
+++ b/builtin/patch-id.c
@@ -55,7 +55,7 @@ static int scan_hunk_header(const char *p, int *p_before, int *p_after)
 
 static void flush_one_hunk(struct object_id *result, git_SHA_CTX *ctx)
 {
-	unsigned char hash[GIT_SHA1_RAWSZ];
+	unsigned char hash[GIT_MAX_RAWSZ];
 	unsigned short carry = 0;
 	int i;
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 83492af05f..f61efd5eed 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1165,7 +1165,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 	const char *dst_name;
 	struct string_list_item *item;
 	struct command *dst_cmd;
-	unsigned char sha1[GIT_SHA1_RAWSZ];
+	unsigned char sha1[GIT_MAX_RAWSZ];
 	int flag;
 
 	strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
diff --git a/cache.h b/cache.h
index cb301d8d7d..5cdd9cd229 100644
--- a/cache.h
+++ b/cache.h
@@ -968,7 +968,7 @@ extern char *sha1_pack_index_name(const unsigned char *sha1);
 extern const char *find_unique_abbrev(const unsigned char *sha1, int len);
 extern int find_unique_abbrev_r(char *hex, const unsigned char *sha1, int len);
 
-extern const unsigned char null_sha1[GIT_SHA1_RAWSZ];
+extern const unsigned char null_sha1[GIT_MAX_RAWSZ];
 extern const struct object_id null_oid;
 
 static inline int hashcmp(const unsigned char *sha1, const unsigned char *sha2)
diff --git a/patch-ids.c b/patch-ids.c
index ce285c2e0c..fa8f11de82 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -71,7 +71,7 @@ static int init_patch_id_entry(struct patch_id *patch,
 			       struct commit *commit,
 			       struct patch_ids *ids)
 {
-	unsigned char header_only_patch_id[GIT_SHA1_RAWSZ];
+	unsigned char header_only_patch_id[GIT_MAX_RAWSZ];
 
 	patch->commit = commit;
 	if (commit_patch_id(commit, &ids->diffopts, header_only_patch_id, 1))
diff --git a/patch-ids.h b/patch-ids.h
index 0f34ea11ea..b9e5751f8e 100644
--- a/patch-ids.h
+++ b/patch-ids.h
@@ -3,7 +3,7 @@
 
 struct patch_id {
 	struct hashmap_entry ent;
-	unsigned char patch_id[GIT_SHA1_RAWSZ];
+	unsigned char patch_id[GIT_MAX_RAWSZ];
 	struct commit *commit;
 };
 
diff --git a/sha1_file.c b/sha1_file.c
index cc6b93c8a9..657666b815 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1611,7 +1611,7 @@ static void mark_bad_packed_object(struct packed_git *p,
 		if (!hashcmp(sha1, p->bad_object_sha1 + GIT_SHA1_RAWSZ * i))
 			return;
 	p->bad_object_sha1 = xrealloc(p->bad_object_sha1,
-				      st_mult(GIT_SHA1_RAWSZ,
+				      st_mult(GIT_MAX_RAWSZ,
 					      st_add(p->num_bad_objects, 1)));
 	hashcpy(p->bad_object_sha1 + GIT_SHA1_RAWSZ * p->num_bad_objects, sha1);
 	p->num_bad_objects++;
@@ -3918,7 +3918,7 @@ static int check_stream_sha1(git_zstream *stream,
 			     const unsigned char *expected_sha1)
 {
 	git_SHA_CTX c;
-	unsigned char real_sha1[GIT_SHA1_RAWSZ];
+	unsigned char real_sha1[GIT_MAX_RAWSZ];
 	unsigned char buf[4096];
 	unsigned long total_read;
 	int status = Z_OK;
diff --git a/sha1_name.c b/sha1_name.c
index 964201bc26..3db166b40b 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -16,11 +16,11 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
 struct disambiguate_state {
 	int len; /* length of prefix in hex chars */
 	char hex_pfx[GIT_MAX_HEXSZ + 1];
-	unsigned char bin_pfx[GIT_SHA1_RAWSZ];
+	unsigned char bin_pfx[GIT_MAX_RAWSZ];
 
 	disambiguate_hint_fn fn;
 	void *cb_data;
-	unsigned char candidate[GIT_SHA1_RAWSZ];
+	unsigned char candidate[GIT_MAX_RAWSZ];
 	unsigned candidate_exists:1;
 	unsigned candidate_checked:1;
 	unsigned candidate_ok:1;
diff --git a/wt-status.h b/wt-status.h
index 54fec77032..6018c627b1 100644
--- a/wt-status.h
+++ b/wt-status.h
@@ -80,7 +80,7 @@ struct wt_status {
 	int hints;
 
 	enum wt_status_format status_format;
-	unsigned char sha1_commit[GIT_SHA1_RAWSZ]; /* when not Initial */
+	unsigned char sha1_commit[GIT_MAX_RAWSZ]; /* when not Initial */
 
 	/* These are computed during processing of the individual sections */
 	int commitable;

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 05/20] builtin/pull: convert portions to struct object_id
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (2 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 03/20] Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ brian m. carlson
@ 2017-03-18 21:19 ` brian m. carlson
  2017-03-18 21:19 ` [PATCH 06/20] builtin/receive-pack: " brian m. carlson
                   ` (7 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Convert the caller of sha1_array_append to struct object_id.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/pull.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 3ecb881b0b..a9f7553f30 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -335,16 +335,16 @@ static void get_merge_heads(struct sha1_array *merge_heads)
 	const char *filename = git_path("FETCH_HEAD");
 	FILE *fp;
 	struct strbuf sb = STRBUF_INIT;
-	unsigned char sha1[GIT_SHA1_RAWSZ];
+	struct object_id oid;
 
 	if (!(fp = fopen(filename, "r")))
 		die_errno(_("could not open '%s' for reading"), filename);
 	while (strbuf_getline_lf(&sb, fp) != EOF) {
-		if (get_sha1_hex(sb.buf, sha1))
+		if (get_oid_hex(sb.buf, &oid))
 			continue;  /* invalid line: does not start with SHA1 */
 		if (starts_with(sb.buf + GIT_SHA1_HEXSZ, "\tnot-for-merge\t"))
 			continue;  /* ref is not-for-merge */
-		sha1_array_append(merge_heads, sha1);
+		sha1_array_append(merge_heads, oid.hash);
 	}
 	fclose(fp);
 	strbuf_release(&sb);

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 06/20] builtin/receive-pack: convert portions to struct object_id
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (3 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 05/20] builtin/pull: convert portions to struct object_id brian m. carlson
@ 2017-03-18 21:19 ` " brian m. carlson
  2017-03-20 12:56   ` Duy Nguyen
  2017-03-18 21:19 ` [PATCH 08/20] parse-options-cb: convert sha1_array_append caller " brian m. carlson
                   ` (6 subsequent siblings)
  11 siblings, 1 reply; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Convert some hardcoded constants into uses of parse_oid_hex.
Additionally, convert all uses of struct command, and miscellaneous
other functions necessary for that.  This work is necessary to be able
to convert sha1_array_append later on.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/receive-pack.c | 97 +++++++++++++++++++++++++-------------------------
 1 file changed, 49 insertions(+), 48 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index f61efd5eed..b1aef26443 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -309,8 +309,8 @@ struct command {
 	unsigned int skip_update:1,
 		     did_not_exist:1;
 	int index;
-	unsigned char old_sha1[20];
-	unsigned char new_sha1[20];
+	struct object_id old_oid;
+	struct object_id new_oid;
 	char ref_name[FLEX_ARRAY]; /* more */
 };
 
@@ -723,7 +723,7 @@ static int feed_receive_hook(void *state_, const char **bufp, size_t *sizep)
 		return -1; /* EOF */
 	strbuf_reset(&state->buf);
 	strbuf_addf(&state->buf, "%s %s %s\n",
-		    sha1_to_hex(cmd->old_sha1), sha1_to_hex(cmd->new_sha1),
+		    oid_to_hex(&cmd->old_oid), oid_to_hex(&cmd->new_oid),
 		    cmd->ref_name);
 	state->cmd = cmd->next;
 	if (bufp) {
@@ -764,8 +764,8 @@ static int run_update_hook(struct command *cmd)
 		return 0;
 
 	argv[1] = cmd->ref_name;
-	argv[2] = sha1_to_hex(cmd->old_sha1);
-	argv[3] = sha1_to_hex(cmd->new_sha1);
+	argv[2] = oid_to_hex(&cmd->old_oid);
+	argv[3] = oid_to_hex(&cmd->new_oid);
 	argv[4] = NULL;
 
 	proc.no_stdin = 1;
@@ -988,8 +988,8 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 	const char *name = cmd->ref_name;
 	struct strbuf namespaced_name_buf = STRBUF_INIT;
 	const char *namespaced_name, *ret;
-	unsigned char *old_sha1 = cmd->old_sha1;
-	unsigned char *new_sha1 = cmd->new_sha1;
+	struct object_id *old_oid = &cmd->old_oid;
+	struct object_id *new_oid = &cmd->new_oid;
 
 	/* only refs/... are allowed */
 	if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1014,20 +1014,20 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 				refuse_unconfigured_deny();
 			return "branch is currently checked out";
 		case DENY_UPDATE_INSTEAD:
-			ret = update_worktree(new_sha1);
+			ret = update_worktree(new_oid->hash);
 			if (ret)
 				return ret;
 			break;
 		}
 	}
 
-	if (!is_null_sha1(new_sha1) && !has_sha1_file(new_sha1)) {
+	if (!is_null_oid(new_oid) && !has_object_file(new_oid)) {
 		error("unpack should have generated %s, "
-		      "but I can't find it!", sha1_to_hex(new_sha1));
+		      "but I can't find it!", oid_to_hex(new_oid));
 		return "bad pack";
 	}
 
-	if (!is_null_sha1(old_sha1) && is_null_sha1(new_sha1)) {
+	if (!is_null_oid(old_oid) && is_null_oid(new_oid)) {
 		if (deny_deletes && starts_with(name, "refs/heads/")) {
 			rp_error("denying ref deletion for %s", name);
 			return "deletion prohibited";
@@ -1053,14 +1053,14 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		}
 	}
 
-	if (deny_non_fast_forwards && !is_null_sha1(new_sha1) &&
-	    !is_null_sha1(old_sha1) &&
+	if (deny_non_fast_forwards && !is_null_oid(new_oid) &&
+	    !is_null_oid(old_oid) &&
 	    starts_with(name, "refs/heads/")) {
 		struct object *old_object, *new_object;
 		struct commit *old_commit, *new_commit;
 
-		old_object = parse_object(old_sha1);
-		new_object = parse_object(new_sha1);
+		old_object = parse_object(old_oid->hash);
+		new_object = parse_object(new_oid->hash);
 
 		if (!old_object || !new_object ||
 		    old_object->type != OBJ_COMMIT ||
@@ -1081,10 +1081,10 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		return "hook declined";
 	}
 
-	if (is_null_sha1(new_sha1)) {
+	if (is_null_oid(new_oid)) {
 		struct strbuf err = STRBUF_INIT;
-		if (!parse_object(old_sha1)) {
-			old_sha1 = NULL;
+		if (!parse_object(old_oid->hash)) {
+			old_oid = NULL;
 			if (ref_exists(name)) {
 				rp_warning("Allowing deletion of corrupt ref.");
 			} else {
@@ -1094,7 +1094,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 		}
 		if (ref_transaction_delete(transaction,
 					   namespaced_name,
-					   old_sha1,
+					   old_oid->hash,
 					   0, "push", &err)) {
 			rp_error("%s", err.buf);
 			strbuf_release(&err);
@@ -1111,7 +1111,7 @@ static const char *update(struct command *cmd, struct shallow_info *si)
 
 		if (ref_transaction_update(transaction,
 					   namespaced_name,
-					   new_sha1, old_sha1,
+					   new_oid->hash, old_oid->hash,
 					   0, "push",
 					   &err)) {
 			rp_error("%s", err.buf);
@@ -1190,8 +1190,8 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 
 	dst_cmd = (struct command *) item->util;
 
-	if (!hashcmp(cmd->old_sha1, dst_cmd->old_sha1) &&
-	    !hashcmp(cmd->new_sha1, dst_cmd->new_sha1))
+	if (!oidcmp(&cmd->old_oid, &dst_cmd->old_oid) &&
+	    !oidcmp(&cmd->new_oid, &dst_cmd->new_oid))
 		return;
 
 	dst_cmd->skip_update = 1;
@@ -1199,11 +1199,11 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 	rp_error("refusing inconsistent update between symref '%s' (%s..%s) and"
 		 " its target '%s' (%s..%s)",
 		 cmd->ref_name,
-		 find_unique_abbrev(cmd->old_sha1, DEFAULT_ABBREV),
-		 find_unique_abbrev(cmd->new_sha1, DEFAULT_ABBREV),
+		 find_unique_abbrev(cmd->old_oid.hash, DEFAULT_ABBREV),
+		 find_unique_abbrev(cmd->new_oid.hash, DEFAULT_ABBREV),
 		 dst_cmd->ref_name,
-		 find_unique_abbrev(dst_cmd->old_sha1, DEFAULT_ABBREV),
-		 find_unique_abbrev(dst_cmd->new_sha1, DEFAULT_ABBREV));
+		 find_unique_abbrev(dst_cmd->old_oid.hash, DEFAULT_ABBREV),
+		 find_unique_abbrev(dst_cmd->new_oid.hash, DEFAULT_ABBREV));
 
 	cmd->error_string = dst_cmd->error_string =
 		"inconsistent aliased update";
@@ -1234,10 +1234,10 @@ static int command_singleton_iterator(void *cb_data, unsigned char sha1[20])
 	struct command **cmd_list = cb_data;
 	struct command *cmd = *cmd_list;
 
-	if (!cmd || is_null_sha1(cmd->new_sha1))
+	if (!cmd || is_null_oid(&cmd->new_oid))
 		return -1; /* end of list */
 	*cmd_list = NULL; /* this returns only one */
-	hashcpy(sha1, cmd->new_sha1);
+	hashcpy(sha1, cmd->new_oid.hash);
 	return 0;
 }
 
@@ -1278,8 +1278,8 @@ static int iterate_receive_command_list(void *cb_data, unsigned char sha1[20])
 		if (shallow_update && data->si->shallow_ref[cmd->index])
 			/* to be checked in update_shallow_ref() */
 			continue;
-		if (!is_null_sha1(cmd->new_sha1) && !cmd->skip_update) {
-			hashcpy(sha1, cmd->new_sha1);
+		if (!is_null_oid(&cmd->new_oid) && !cmd->skip_update) {
+			hashcpy(sha1, cmd->new_oid.hash);
 			*cmd_list = cmd->next;
 			return 0;
 		}
@@ -1306,7 +1306,7 @@ static void reject_updates_to_hidden(struct command *commands)
 
 		if (!ref_is_hidden(cmd->ref_name, refname_full.buf))
 			continue;
-		if (is_null_sha1(cmd->new_sha1))
+		if (is_null_oid(&cmd->new_oid))
 			cmd->error_string = "deny deleting a hidden ref";
 		else
 			cmd->error_string = "deny updating a hidden ref";
@@ -1489,23 +1489,24 @@ static struct command **queue_command(struct command **tail,
 				      const char *line,
 				      int linelen)
 {
-	unsigned char old_sha1[20], new_sha1[20];
+	struct object_id old_oid, new_oid;
 	struct command *cmd;
 	const char *refname;
 	int reflen;
+	const char *p;
 
-	if (linelen < 83 ||
-	    line[40] != ' ' ||
-	    line[81] != ' ' ||
-	    get_sha1_hex(line, old_sha1) ||
-	    get_sha1_hex(line + 41, new_sha1))
+	if (!linelen ||
+	    parse_oid_hex(line, &old_oid, &p) ||
+	    *p++ != ' ' ||
+	    parse_oid_hex(p, &new_oid, &p) ||
+	    *p++ != ' ')
 		die("protocol error: expected old/new/ref, got '%s'", line);
 
-	refname = line + 82;
-	reflen = linelen - 82;
+	refname = p;
+	reflen = linelen - (p - line);
 	FLEX_ALLOC_MEM(cmd, ref_name, refname, reflen);
-	hashcpy(cmd->old_sha1, old_sha1);
-	hashcpy(cmd->new_sha1, new_sha1);
+	oidcpy(&cmd->old_oid, &old_oid);
+	oidcpy(&cmd->new_oid, &new_oid);
 	*tail = cmd;
 	return &cmd->next;
 }
@@ -1545,11 +1546,11 @@ static struct command *read_head_info(struct sha1_array *shallow)
 			break;
 
 		if (len == 48 && starts_with(line, "shallow ")) {
-			unsigned char sha1[20];
-			if (get_sha1_hex(line + 8, sha1))
+			struct object_id oid;
+			if (get_oid_hex(line + 8, &oid))
 				die("protocol error: expected shallow sha, got '%s'",
 				    line + 8);
-			sha1_array_append(shallow, sha1);
+			sha1_array_append(shallow, oid.hash);
 			continue;
 		}
 
@@ -1818,9 +1819,9 @@ static void update_shallow_info(struct command *commands,
 	}
 
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (is_null_sha1(cmd->new_sha1))
+		if (is_null_oid(&cmd->new_oid))
 			continue;
-		sha1_array_append(ref, cmd->new_sha1);
+		sha1_array_append(ref, cmd->new_oid.hash);
 		cmd->index = ref->nr - 1;
 	}
 	si->ref = ref;
@@ -1833,7 +1834,7 @@ static void update_shallow_info(struct command *commands,
 	ALLOC_ARRAY(ref_status, ref->nr);
 	assign_shallow_commits_to_refs(si, NULL, ref_status);
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (is_null_sha1(cmd->new_sha1))
+		if (is_null_oid(&cmd->new_oid))
 			continue;
 		if (ref_status[cmd->index]) {
 			cmd->error_string = "shallow update not allowed";
@@ -1871,7 +1872,7 @@ static int delete_only(struct command *commands)
 {
 	struct command *cmd;
 	for (cmd = commands; cmd; cmd = cmd->next) {
-		if (!is_null_sha1(cmd->new_sha1))
+		if (!is_null_oid(&cmd->new_oid))
 			return 0;
 	}
 	return 1;

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 08/20] parse-options-cb: convert sha1_array_append caller to struct object_id
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (4 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 06/20] builtin/receive-pack: " brian m. carlson
@ 2017-03-18 21:19 ` " brian m. carlson
  2017-03-18 21:19 ` [PATCH 09/20] test-sha1-array: convert most code " brian m. carlson
                   ` (5 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 parse-options-cb.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/parse-options-cb.c b/parse-options-cb.c
index b7d8f7dcb2..40ece4d8c2 100644
--- a/parse-options-cb.c
+++ b/parse-options-cb.c
@@ -96,7 +96,7 @@ int parse_opt_commits(const struct option *opt, const char *arg, int unset)
 
 int parse_opt_object_name(const struct option *opt, const char *arg, int unset)
 {
-	unsigned char sha1[20];
+	struct object_id oid;
 
 	if (unset) {
 		sha1_array_clear(opt->value);
@@ -104,9 +104,9 @@ int parse_opt_object_name(const struct option *opt, const char *arg, int unset)
 	}
 	if (!arg)
 		return -1;
-	if (get_sha1(arg, sha1))
+	if (get_oid(arg, &oid))
 		return error(_("malformed object name '%s'"), arg);
-	sha1_array_append(opt->value, sha1);
+	sha1_array_append(opt->value, oid.hash);
 	return 0;
 }
 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 09/20] test-sha1-array: convert most code to struct object_id
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (5 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 08/20] parse-options-cb: convert sha1_array_append caller " brian m. carlson
@ 2017-03-18 21:19 ` " brian m. carlson
  2017-03-18 21:19 ` [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id brian m. carlson
                   ` (4 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

This helper is very small, so convert the entire thing.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 t/helper/test-sha1-array.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/helper/test-sha1-array.c b/t/helper/test-sha1-array.c
index f7a53c4ad6..b4bb97fccc 100644
--- a/t/helper/test-sha1-array.c
+++ b/t/helper/test-sha1-array.c
@@ -14,16 +14,16 @@ int cmd_main(int argc, const char **argv)
 
 	while (strbuf_getline(&line, stdin) != EOF) {
 		const char *arg;
-		unsigned char sha1[20];
+		struct object_id oid;
 
 		if (skip_prefix(line.buf, "append ", &arg)) {
-			if (get_sha1_hex(arg, sha1))
+			if (get_oid_hex(arg, &oid))
 				die("not a hexadecimal SHA1: %s", arg);
-			sha1_array_append(&array, sha1);
+			sha1_array_append(&array, oid.hash);
 		} else if (skip_prefix(line.buf, "lookup ", &arg)) {
-			if (get_sha1_hex(arg, sha1))
+			if (get_oid_hex(arg, &oid))
 				die("not a hexadecimal SHA1: %s", arg);
-			printf("%d\n", sha1_array_lookup(&array, sha1));
+			printf("%d\n", sha1_array_lookup(&array, oid.hash));
 		} else if (!strcmp(line.buf, "clear"))
 			sha1_array_clear(&array);
 		else if (!strcmp(line.buf, "for_each_unique"))

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (6 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 09/20] test-sha1-array: convert most code " brian m. carlson
@ 2017-03-18 21:19 ` brian m. carlson
  2017-03-20 13:07   ` Duy Nguyen
  2017-03-18 21:19 ` [PATCH 11/20] sha1_name: convert disambiguate_hint_fn to take object_id brian m. carlson
                   ` (3 subsequent siblings)
  11 siblings, 1 reply; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Convert struct disambiguate_state to use struct object_id by changing
the structure definition and applying the following semantic patch:

@@
struct disambiguate_state E1;
@@
- E1.bin_pfx
+ E1.bin_pfx.hash

@@
struct disambiguate_state *E1;
@@
- E1->bin_pfx
+ E1->bin_pfx.hash

@@
struct disambiguate_state E1;
@@
- E1.candidate
+ E1.candidate.hash

@@
struct disambiguate_state *E1;
@@
- E1->candidate
+ E1->candidate.hash

This conversion is needed so we can convert disambiguate_hint_fn later.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 sha1_name.c | 22 +++++++++++-----------
 1 file changed, 11 insertions(+), 11 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index 3db166b40b..cf6f4be0c6 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -16,11 +16,11 @@ typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
 struct disambiguate_state {
 	int len; /* length of prefix in hex chars */
 	char hex_pfx[GIT_MAX_HEXSZ + 1];
-	unsigned char bin_pfx[GIT_MAX_RAWSZ];
+	struct object_id bin_pfx;
 
 	disambiguate_hint_fn fn;
 	void *cb_data;
-	unsigned char candidate[GIT_MAX_RAWSZ];
+	struct object_id candidate;
 	unsigned candidate_exists:1;
 	unsigned candidate_checked:1;
 	unsigned candidate_ok:1;
@@ -37,10 +37,10 @@ static void update_candidates(struct disambiguate_state *ds, const unsigned char
 	}
 	if (!ds->candidate_exists) {
 		/* this is the first candidate */
-		hashcpy(ds->candidate, current);
+		hashcpy(ds->candidate.hash, current);
 		ds->candidate_exists = 1;
 		return;
-	} else if (!hashcmp(ds->candidate, current)) {
+	} else if (!hashcmp(ds->candidate.hash, current)) {
 		/* the same as what we already have seen */
 		return;
 	}
@@ -52,14 +52,14 @@ static void update_candidates(struct disambiguate_state *ds, const unsigned char
 	}
 
 	if (!ds->candidate_checked) {
-		ds->candidate_ok = ds->fn(ds->candidate, ds->cb_data);
+		ds->candidate_ok = ds->fn(ds->candidate.hash, ds->cb_data);
 		ds->disambiguate_fn_used = 1;
 		ds->candidate_checked = 1;
 	}
 
 	if (!ds->candidate_ok) {
 		/* discard the candidate; we know it does not satisfy fn */
-		hashcpy(ds->candidate, current);
+		hashcpy(ds->candidate.hash, current);
 		ds->candidate_checked = 0;
 		return;
 	}
@@ -151,7 +151,7 @@ static void unique_in_pack(struct packed_git *p,
 		int cmp;
 
 		current = nth_packed_object_sha1(p, mid);
-		cmp = hashcmp(ds->bin_pfx, current);
+		cmp = hashcmp(ds->bin_pfx.hash, current);
 		if (!cmp) {
 			first = mid;
 			break;
@@ -170,7 +170,7 @@ static void unique_in_pack(struct packed_git *p,
 	 */
 	for (i = first; i < num && !ds->ambiguous; i++) {
 		current = nth_packed_object_sha1(p, i);
-		if (!match_sha(ds->len, ds->bin_pfx, current))
+		if (!match_sha(ds->len, ds->bin_pfx.hash, current))
 			break;
 		update_candidates(ds, current);
 	}
@@ -213,12 +213,12 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 		 * same repository!
 		 */
 		ds->candidate_ok = (!ds->disambiguate_fn_used ||
-				    ds->fn(ds->candidate, ds->cb_data));
+				    ds->fn(ds->candidate.hash, ds->cb_data));
 
 	if (!ds->candidate_ok)
 		return SHORT_NAME_AMBIGUOUS;
 
-	hashcpy(sha1, ds->candidate);
+	hashcpy(sha1, ds->candidate.hash);
 	return 0;
 }
 
@@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int len,
 		ds->hex_pfx[i] = c;
 		if (!(i & 1))
 			val <<= 4;
-		ds->bin_pfx[i >> 1] |= val;
+		ds->bin_pfx.hash[i >> 1] |= val;
 	}
 
 	ds->len = len;

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 11/20] sha1_name: convert disambiguate_hint_fn to take object_id
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (7 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id brian m. carlson
@ 2017-03-18 21:19 ` brian m. carlson
  2017-03-18 21:19 ` [PATCH 12/20] submodule: convert check_for_new_submodule_commits to object_id brian m. carlson
                   ` (2 subsequent siblings)
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

Convert this function pointer type and the functions that implement it
to take a struct object_id.  Introduce a temporary in
show_ambiguous_object to avoid having to convert for_each_abbrev at this
point.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 sha1_name.c | 64 ++++++++++++++++++++++++++++++++-----------------------------
 1 file changed, 34 insertions(+), 30 deletions(-)

diff --git a/sha1_name.c b/sha1_name.c
index cf6f4be0c6..2e38aedfa5 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -11,7 +11,7 @@
 
 static int get_sha1_oneline(const char *, unsigned char *, struct commit_list *);
 
-typedef int (*disambiguate_hint_fn)(const unsigned char *, void *);
+typedef int (*disambiguate_hint_fn)(const struct object_id *, void *);
 
 struct disambiguate_state {
 	int len; /* length of prefix in hex chars */
@@ -29,7 +29,7 @@ struct disambiguate_state {
 	unsigned always_call_fn:1;
 };
 
-static void update_candidates(struct disambiguate_state *ds, const unsigned char *current)
+static void update_candidates(struct disambiguate_state *ds, const struct object_id *current)
 {
 	if (ds->always_call_fn) {
 		ds->ambiguous = ds->fn(current, ds->cb_data) ? 1 : 0;
@@ -37,10 +37,10 @@ static void update_candidates(struct disambiguate_state *ds, const unsigned char
 	}
 	if (!ds->candidate_exists) {
 		/* this is the first candidate */
-		hashcpy(ds->candidate.hash, current);
+		oidcpy(&ds->candidate, current);
 		ds->candidate_exists = 1;
 		return;
-	} else if (!hashcmp(ds->candidate.hash, current)) {
+	} else if (!oidcmp(&ds->candidate, current)) {
 		/* the same as what we already have seen */
 		return;
 	}
@@ -52,14 +52,14 @@ static void update_candidates(struct disambiguate_state *ds, const unsigned char
 	}
 
 	if (!ds->candidate_checked) {
-		ds->candidate_ok = ds->fn(ds->candidate.hash, ds->cb_data);
+		ds->candidate_ok = ds->fn(&ds->candidate, ds->cb_data);
 		ds->disambiguate_fn_used = 1;
 		ds->candidate_checked = 1;
 	}
 
 	if (!ds->candidate_ok) {
 		/* discard the candidate; we know it does not satisfy fn */
-		hashcpy(ds->candidate.hash, current);
+		oidcpy(&ds->candidate, current);
 		ds->candidate_checked = 0;
 		return;
 	}
@@ -107,15 +107,15 @@ static void find_short_object_filename(struct disambiguate_state *ds)
 			continue;
 
 		while (!ds->ambiguous && (de = readdir(dir)) != NULL) {
-			unsigned char sha1[20];
+			struct object_id oid;
 
-			if (strlen(de->d_name) != 38)
+			if (strlen(de->d_name) != GIT_SHA1_HEXSZ - 2)
 				continue;
 			if (memcmp(de->d_name, ds->hex_pfx + 2, ds->len - 2))
 				continue;
-			memcpy(hex + 2, de->d_name, 38);
-			if (!get_sha1_hex(hex, sha1))
-				update_candidates(ds, sha1);
+			memcpy(hex + 2, de->d_name, GIT_SHA1_HEXSZ - 2);
+			if (!get_oid_hex(hex, &oid))
+				update_candidates(ds, &oid);
 		}
 		closedir(dir);
 	}
@@ -140,7 +140,7 @@ static void unique_in_pack(struct packed_git *p,
 			   struct disambiguate_state *ds)
 {
 	uint32_t num, last, i, first = 0;
-	const unsigned char *current = NULL;
+	const struct object_id *current = NULL;
 
 	open_pack_index(p);
 	num = p->num_objects;
@@ -169,8 +169,9 @@ static void unique_in_pack(struct packed_git *p,
 	 * 0, 1 or more objects that actually match(es).
 	 */
 	for (i = first; i < num && !ds->ambiguous; i++) {
-		current = nth_packed_object_sha1(p, i);
-		if (!match_sha(ds->len, ds->bin_pfx.hash, current))
+		struct object_id oid;
+		current = nth_packed_object_oid(&oid, p, i);
+		if (!match_sha(ds->len, ds->bin_pfx.hash, current->hash))
 			break;
 		update_candidates(ds, current);
 	}
@@ -213,7 +214,7 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 		 * same repository!
 		 */
 		ds->candidate_ok = (!ds->disambiguate_fn_used ||
-				    ds->fn(ds->candidate.hash, ds->cb_data));
+				    ds->fn(&ds->candidate, ds->cb_data));
 
 	if (!ds->candidate_ok)
 		return SHORT_NAME_AMBIGUOUS;
@@ -222,57 +223,57 @@ static int finish_object_disambiguation(struct disambiguate_state *ds,
 	return 0;
 }
 
-static int disambiguate_commit_only(const unsigned char *sha1, void *cb_data_unused)
+static int disambiguate_commit_only(const struct object_id *oid, void *cb_data_unused)
 {
-	int kind = sha1_object_info(sha1, NULL);
+	int kind = sha1_object_info(oid->hash, NULL);
 	return kind == OBJ_COMMIT;
 }
 
-static int disambiguate_committish_only(const unsigned char *sha1, void *cb_data_unused)
+static int disambiguate_committish_only(const struct object_id *oid, void *cb_data_unused)
 {
 	struct object *obj;
 	int kind;
 
-	kind = sha1_object_info(sha1, NULL);
+	kind = sha1_object_info(oid->hash, NULL);
 	if (kind == OBJ_COMMIT)
 		return 1;
 	if (kind != OBJ_TAG)
 		return 0;
 
 	/* We need to do this the hard way... */
-	obj = deref_tag(parse_object(sha1), NULL, 0);
+	obj = deref_tag(parse_object(oid->hash), NULL, 0);
 	if (obj && obj->type == OBJ_COMMIT)
 		return 1;
 	return 0;
 }
 
-static int disambiguate_tree_only(const unsigned char *sha1, void *cb_data_unused)
+static int disambiguate_tree_only(const struct object_id *oid, void *cb_data_unused)
 {
-	int kind = sha1_object_info(sha1, NULL);
+	int kind = sha1_object_info(oid->hash, NULL);
 	return kind == OBJ_TREE;
 }
 
-static int disambiguate_treeish_only(const unsigned char *sha1, void *cb_data_unused)
+static int disambiguate_treeish_only(const struct object_id *oid, void *cb_data_unused)
 {
 	struct object *obj;
 	int kind;
 
-	kind = sha1_object_info(sha1, NULL);
+	kind = sha1_object_info(oid->hash, NULL);
 	if (kind == OBJ_TREE || kind == OBJ_COMMIT)
 		return 1;
 	if (kind != OBJ_TAG)
 		return 0;
 
 	/* We need to do this the hard way... */
-	obj = deref_tag(parse_object(sha1), NULL, 0);
+	obj = deref_tag(parse_object(oid->hash), NULL, 0);
 	if (obj && (obj->type == OBJ_TREE || obj->type == OBJ_COMMIT))
 		return 1;
 	return 0;
 }
 
-static int disambiguate_blob_only(const unsigned char *sha1, void *cb_data_unused)
+static int disambiguate_blob_only(const struct object_id *oid, void *cb_data_unused)
 {
-	int kind = sha1_object_info(sha1, NULL);
+	int kind = sha1_object_info(oid->hash, NULL);
 	return kind == OBJ_BLOB;
 }
 
@@ -344,10 +345,13 @@ static int init_object_disambiguation(const char *name, int len,
 static int show_ambiguous_object(const unsigned char *sha1, void *data)
 {
 	const struct disambiguate_state *ds = data;
+	struct object_id oid;
 	struct strbuf desc = STRBUF_INIT;
 	int type;
 
-	if (ds->fn && !ds->fn(sha1, ds->cb_data))
+
+	hashcpy(oid.hash, sha1);
+	if (ds->fn && !ds->fn(&oid, ds->cb_data))
 		return 0;
 
 	type = sha1_object_info(sha1, NULL);
@@ -422,9 +426,9 @@ static int get_short_sha1(const char *name, int len, unsigned char *sha1,
 	return status;
 }
 
-static int collect_ambiguous(const unsigned char *sha1, void *data)
+static int collect_ambiguous(const struct object_id *oid, void *data)
 {
-	sha1_array_append(data, sha1);
+	sha1_array_append(data, oid->hash);
 	return 0;
 }
 

^ permalink raw reply	[flat|nested] 21+ messages in thread

* [PATCH 12/20] submodule: convert check_for_new_submodule_commits to object_id
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (8 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 11/20] sha1_name: convert disambiguate_hint_fn to take object_id brian m. carlson
@ 2017-03-18 21:19 ` brian m. carlson
  2017-03-18 21:19 ` [PATCH 19/20] Rename sha1_array to oid_array brian m. carlson
  2017-03-20 13:14 ` [PATCH 00/20] object_id part 7 Duy Nguyen
  11 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-18 21:19 UTC (permalink / raw)
  To: git; +Cc: Jeff King, Nguyễn Thái Ngọc Duy

All of the callers of this function have been converted, so convert this
function and update the callers.  This function also calls
sha1_array_append, which we'll convert shortly.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 builtin/fetch.c | 6 +++---
 submodule.c     | 4 ++--
 submodule.h     | 2 +-
 3 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/fetch.c b/builtin/fetch.c
index b5ad09d046..a41b892dcc 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -659,7 +659,7 @@ static int update_local_ref(struct ref *ref,
 
 		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
 		    (recurse_submodules != RECURSE_SUBMODULES_ON))
-			check_for_new_submodule_commits(ref->new_oid.hash);
+			check_for_new_submodule_commits(&ref->new_oid);
 		r = s_update_ref(msg, ref, 0);
 		format_display(display, r ? '!' : '*', what,
 			       r ? _("unable to update local ref") : NULL,
@@ -675,7 +675,7 @@ static int update_local_ref(struct ref *ref,
 		strbuf_add_unique_abbrev(&quickref, ref->new_oid.hash, DEFAULT_ABBREV);
 		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
 		    (recurse_submodules != RECURSE_SUBMODULES_ON))
-			check_for_new_submodule_commits(ref->new_oid.hash);
+			check_for_new_submodule_commits(&ref->new_oid);
 		r = s_update_ref("fast-forward", ref, 1);
 		format_display(display, r ? '!' : ' ', quickref.buf,
 			       r ? _("unable to update local ref") : NULL,
@@ -690,7 +690,7 @@ static int update_local_ref(struct ref *ref,
 		strbuf_add_unique_abbrev(&quickref, ref->new_oid.hash, DEFAULT_ABBREV);
 		if ((recurse_submodules != RECURSE_SUBMODULES_OFF) &&
 		    (recurse_submodules != RECURSE_SUBMODULES_ON))
-			check_for_new_submodule_commits(ref->new_oid.hash);
+			check_for_new_submodule_commits(&ref->new_oid);
 		r = s_update_ref("forced-update", ref, 1);
 		format_display(display, r ? '!' : '+', quickref.buf,
 			       r ? _("unable to update local ref") : _("forced update"),
diff --git a/submodule.c b/submodule.c
index 3200b7bb2b..5c5c18ec3d 100644
--- a/submodule.c
+++ b/submodule.c
@@ -821,14 +821,14 @@ static int add_sha1_to_array(const char *ref, const struct object_id *oid,
 	return 0;
 }
 
-void check_for_new_submodule_commits(unsigned char new_sha1[20])
+void check_for_new_submodule_commits(struct object_id *oid)
 {
 	if (!initialized_fetch_ref_tips) {
 		for_each_ref(add_sha1_to_array, &ref_tips_before_fetch);
 		initialized_fetch_ref_tips = 1;
 	}
 
-	sha1_array_append(&ref_tips_after_fetch, new_sha1);
+	sha1_array_append(&ref_tips_after_fetch, oid->hash);
 }
 
 static int add_sha1_to_argv(const unsigned char sha1[20], void *data)
diff --git a/submodule.h b/submodule.h
index c8a0c9cb29..9c32b28b12 100644
--- a/submodule.h
+++ b/submodule.h
@@ -58,7 +58,7 @@ extern void show_submodule_inline_diff(FILE *f, const char *path,
 		const char *del, const char *add, const char *reset,
 		const struct diff_options *opt);
 extern void set_config_fetch_recurse_submodules(int value);
-extern void check_for_new_submodule_commits(unsigned char new_sha1[20]);
+extern void check_for_new_submodule_commits(struct object_id *oid);
 extern int fetch_populated_submodules(const struct argv_array *options,
 			       const char *prefix, int command_line_option,
 			       int quiet, int max_parallel_jobs);

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 19/20] Rename sha1_array to oid_array
  2017-03-18 21:19 ` [PATCH 19/20] Rename sha1_array to oid_array brian m. carlson
@ 2017-03-20 12:25   ` Duy Nguyen
  2017-03-21  0:54     ` brian m. carlson
  0 siblings, 1 reply; 21+ messages in thread
From: Duy Nguyen @ 2017-03-20 12:25 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Git Mailing List, Jeff King

On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> Since this structure handles an array of object IDs, rename it to struct
> oid_array.  Also rename the accessor functions and the initialization
> constant.
>
> This commit was produced mechanically by providing non-Documentation
> files to the following Perl one-liners:
>
>     perl -pi -E 's/struct sha1_array/struct oid_array/g'
>     perl -pi -E 's/\bsha1_array_/oid_array_/g'
>     perl -pi -E 's/SHA1_ARRAY_INIT/OID_ARRAY_INIT/g'
>

I see a few multi-line function calls become unaligned because
oid_array is one character shorter than sha1_array. But I'm ok with
that, no need to manually align them. We can fix those when we touch
neighbor code.
-- 
Duy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 06/20] builtin/receive-pack: convert portions to struct object_id
  2017-03-18 21:19 ` [PATCH 06/20] builtin/receive-pack: " brian m. carlson
@ 2017-03-20 12:56   ` Duy Nguyen
  2017-03-20 23:17     ` brian m. carlson
  0 siblings, 1 reply; 21+ messages in thread
From: Duy Nguyen @ 2017-03-20 12:56 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Git Mailing List, Jeff King

On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> @@ -1489,23 +1489,24 @@ static struct command **queue_command(struct command **tail,
>                                       const char *line,
>                                       int linelen)
>  {
> -       unsigned char old_sha1[20], new_sha1[20];
> +       struct object_id old_oid, new_oid;
>         struct command *cmd;
>         const char *refname;
>         int reflen;
> +       const char *p;
>
> -       if (linelen < 83 ||
> -           line[40] != ' ' ||
> -           line[81] != ' ' ||
> -           get_sha1_hex(line, old_sha1) ||
> -           get_sha1_hex(line + 41, new_sha1))
> +       if (!linelen ||

I think you can skip this. The old code needed "< 83" because of the
random accesses to [40] and [81] but you don't do that anymore.
parse_oid_hex() can handle empty hex strings fine.

> +           parse_oid_hex(line, &old_oid, &p) ||
> +           *p++ != ' ' ||
> +           parse_oid_hex(p, &new_oid, &p) ||
> +           *p++ != ' ')

maybe "|| *p)" as well? I think the old code, with "linelen < 83",
makes sure reflen is at least one. Not sure what FLEX_ALLOC_MEM would
do if reflen is zero.

>                 die("protocol error: expected old/new/ref, got '%s'", line);
>
> -       refname = line + 82;
> -       reflen = linelen - 82;
> +       refname = p;
> +       reflen = linelen - (p - line);
>         FLEX_ALLOC_MEM(cmd, ref_name, refname, reflen);
> -       hashcpy(cmd->old_sha1, old_sha1);
> -       hashcpy(cmd->new_sha1, new_sha1);
> +       oidcpy(&cmd->old_oid, &old_oid);
> +       oidcpy(&cmd->new_oid, &new_oid);
>         *tail = cmd;
>         return &cmd->next;
>  }
-- 
Duy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id
  2017-03-18 21:19 ` [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id brian m. carlson
@ 2017-03-20 13:07   ` Duy Nguyen
  2017-03-20 22:32     ` brian m. carlson
  0 siblings, 1 reply; 21+ messages in thread
From: Duy Nguyen @ 2017-03-20 13:07 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Git Mailing List, Jeff King

On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> @@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int len,
>                 ds->hex_pfx[i] = c;
>                 if (!(i & 1))
>                         val <<= 4;
> -               ds->bin_pfx[i >> 1] |= val;
> +               ds->bin_pfx.hash[i >> 1] |= val;

The indexing makes me a bit nervous, especially since diff context
here is too narrow to see. It would be nice if this code (at the
beginning of init_object_disambiguation) is converted here too

        if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
                return -1;
-- 
Duy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 00/20] object_id part 7
  2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
                   ` (10 preceding siblings ...)
  2017-03-18 21:19 ` [PATCH 19/20] Rename sha1_array to oid_array brian m. carlson
@ 2017-03-20 13:14 ` Duy Nguyen
  2017-03-21  1:16   ` brian m. carlson
  11 siblings, 1 reply; 21+ messages in thread
From: Duy Nguyen @ 2017-03-20 13:14 UTC (permalink / raw)
  To: brian m. carlson; +Cc: Git Mailing List, Jeff King

On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> This is part 7 in the continuing transition to use struct object_id.

It feels very nice to see many ".hash" and "->hash" go away. Looking
forward to seeing you kill sha1 in sha1_file.c, object.c and friends.
That one (and read-cache) must be a battle.

I'm not familiar with many parts that this series touches so I may
have missed something subtle, but overall it looks good to me. Thank
you for working on this.

> I chose not to rename the sha1-array.[ch] files

You'll have to rename sha1_file.c, sha1_name.c too and might start a
code reorganization revolution by renaming sha1-array.[ch] ;-)
-- 
Duy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id
  2017-03-20 13:07   ` Duy Nguyen
@ 2017-03-20 22:32     ` brian m. carlson
  2017-03-21 10:17       ` Duy Nguyen
  0 siblings, 1 reply; 21+ messages in thread
From: brian m. carlson @ 2017-03-20 22:32 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List, Jeff King

[-- Attachment #1: Type: text/plain, Size: 1580 bytes --]

On Mon, Mar 20, 2017 at 08:07:09PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > @@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int len,
> >                 ds->hex_pfx[i] = c;
> >                 if (!(i & 1))
> >                         val <<= 4;
> > -               ds->bin_pfx[i >> 1] |= val;
> > +               ds->bin_pfx.hash[i >> 1] |= val;
> 
> The indexing makes me a bit nervous, especially since diff context
> here is too narrow to see. It would be nice if this code (at the
> beginning of init_object_disambiguation) is converted here too
> 
>         if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
>                 return -1;

Well, I think that's the way I would have written that text at the top
of the function.  I expect that we'll end up turning GIT_SHA1_HEXSZ into
a global named something like current_hash_len via global
search-and-replace, so it will always be the right length.

The indexing should be safe because len is guaranteed to be sufficiently
small, and I feel like it we would have seen it break by now if it had
had an overflow.  i will always be in the range [0, 40) (for SHA-1), so
i >> 1 should always be in [0, 20).

Am I understanding you correctly and if so, does that assuage your
concerns, or did you mean something else?
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 06/20] builtin/receive-pack: convert portions to struct object_id
  2017-03-20 12:56   ` Duy Nguyen
@ 2017-03-20 23:17     ` brian m. carlson
  2017-03-21 10:26       ` Duy Nguyen
  0 siblings, 1 reply; 21+ messages in thread
From: brian m. carlson @ 2017-03-20 23:17 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List, Jeff King

[-- Attachment #1: Type: text/plain, Size: 2666 bytes --]

On Mon, Mar 20, 2017 at 07:56:17PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > @@ -1489,23 +1489,24 @@ static struct command **queue_command(struct command **tail,
> >                                       const char *line,
> >                                       int linelen)
> >  {
> > -       unsigned char old_sha1[20], new_sha1[20];
> > +       struct object_id old_oid, new_oid;
> >         struct command *cmd;
> >         const char *refname;
> >         int reflen;
> > +       const char *p;
> >
> > -       if (linelen < 83 ||
> > -           line[40] != ' ' ||
> > -           line[81] != ' ' ||
> > -           get_sha1_hex(line, old_sha1) ||
> > -           get_sha1_hex(line + 41, new_sha1))
> > +       if (!linelen ||
> 
> I think you can skip this. The old code needed "< 83" because of the
> random accesses to [40] and [81] but you don't do that anymore.
> parse_oid_hex() can handle empty hex strings fine.

I just realized this looks fishy:

	while (boc < eoc) {
		const char *eol = memchr(boc, '\n', eoc - boc);
		tail = queue_command(tail, boc, eol ? eol - boc : eoc - eol);
		boc = eol ? eol + 1 : eoc;
	}

If eol is NULL, we subtract it from eoc?  I mean, eol will be zero, so
we get eoc, which seems like what we want.  I think I'm going to send in
a separate patch to fix that, because clearly that's bizarre and not at
all compliant with the C standard.

Going back to linelen, I think it's probably safe to remove, since even
if *boc is a newline (and we get an empty linelen), we're still
guaranteed to have another character, since this is a strbuf.  The
amount of double-checking I had to do there makes me nervous, though.

I'll squash in a change.

> > +           parse_oid_hex(line, &old_oid, &p) ||
> > +           *p++ != ' ' ||
> > +           parse_oid_hex(p, &new_oid, &p) ||
> > +           *p++ != ' ')
> 
> maybe "|| *p)" as well? I think the old code, with "linelen < 83",
> makes sure reflen is at least one. Not sure what FLEX_ALLOC_MEM would
> do if reflen is zero.

I don't think that line is actually guaranteed to be NUL-terminated.  It
may be terminated instead by a newline, such as by
queue_commands_from_cert.

If we did get an empty reflen, we'd call xcalloc, where we will allocate
exactly the size of the struct otherwise.  We'd then try to memcpy zero
bytes into that location, and succeed.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 19/20] Rename sha1_array to oid_array
  2017-03-20 12:25   ` Duy Nguyen
@ 2017-03-21  0:54     ` brian m. carlson
  0 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-21  0:54 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List, Jeff King

[-- Attachment #1: Type: text/plain, Size: 1163 bytes --]

On Mon, Mar 20, 2017 at 07:25:25PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > Since this structure handles an array of object IDs, rename it to struct
> > oid_array.  Also rename the accessor functions and the initialization
> > constant.
> >
> > This commit was produced mechanically by providing non-Documentation
> > files to the following Perl one-liners:
> >
> >     perl -pi -E 's/struct sha1_array/struct oid_array/g'
> >     perl -pi -E 's/\bsha1_array_/oid_array_/g'
> >     perl -pi -E 's/SHA1_ARRAY_INIT/OID_ARRAY_INIT/g'
> >
> 
> I see a few multi-line function calls become unaligned because
> oid_array is one character shorter than sha1_array. But I'm ok with
> that, no need to manually align them. We can fix those when we touch
> neighbor code.

That's how I felt about the situation: basically, people would
appreciate the ease of review over the tidiness of the code.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 00/20] object_id part 7
  2017-03-20 13:14 ` [PATCH 00/20] object_id part 7 Duy Nguyen
@ 2017-03-21  1:16   ` brian m. carlson
  0 siblings, 0 replies; 21+ messages in thread
From: brian m. carlson @ 2017-03-21  1:16 UTC (permalink / raw)
  To: Duy Nguyen; +Cc: Git Mailing List, Jeff King

[-- Attachment #1: Type: text/plain, Size: 1940 bytes --]

On Mon, Mar 20, 2017 at 08:14:20PM +0700, Duy Nguyen wrote:
> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > This is part 7 in the continuing transition to use struct object_id.
> 
> It feels very nice to see many ".hash" and "->hash" go away. Looking
> forward to seeing you kill sha1 in sha1_file.c, object.c and friends.
> That one (and read-cache) must be a battle.

Basically, the next few series look like this:

Part 7 (this one): sha1_array → oid_array
Part 8: lookup_{commit,tree,blob,tag}* and parse_object* [0]
Part 9: get_sha1* → get_oid*
Part 10: refs.c

As I go, I make note of which function calls I tend to modify a lot,
since those tend to be good places to look for future conversions.
sha1_file.c is looking like a good candidate for part 11.

> I'm not familiar with many parts that this series touches so I may
> have missed something subtle, but overall it looks good to me. Thank
> you for working on this.

I think git-contacts picked you because I touched a lot of the shallow
code.

> > I chose not to rename the sha1-array.[ch] files
> 
> You'll have to rename sha1_file.c, sha1_name.c too and might start a
> code reorganization revolution by renaming sha1-array.[ch] ;-)

I tend to prefer dashes over underscores, so I fear other people may not
like my renaming decisions very much.  I think I'm going to stay out of
it and let someone who cares more make the controversial decisions.

Anyway, I've squashed in some changes and will wait another day or two
for other people to send in feedback before v2.

[0] Looking at it now, I can probably handle lookup_object, too, so I
might stuff that in.  That series is already 53 patches, though….
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id
  2017-03-20 22:32     ` brian m. carlson
@ 2017-03-21 10:17       ` Duy Nguyen
  0 siblings, 0 replies; 21+ messages in thread
From: Duy Nguyen @ 2017-03-21 10:17 UTC (permalink / raw)
  To: brian m. carlson, Duy Nguyen, Git Mailing List, Jeff King

On Tue, Mar 21, 2017 at 5:32 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Mon, Mar 20, 2017 at 08:07:09PM +0700, Duy Nguyen wrote:
>> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>> > @@ -332,7 +332,7 @@ static int init_object_disambiguation(const char *name, int len,
>> >                 ds->hex_pfx[i] = c;
>> >                 if (!(i & 1))
>> >                         val <<= 4;
>> > -               ds->bin_pfx[i >> 1] |= val;
>> > +               ds->bin_pfx.hash[i >> 1] |= val;
>>
>> The indexing makes me a bit nervous, especially since diff context
>> here is too narrow to see. It would be nice if this code (at the
>> beginning of init_object_disambiguation) is converted here too
>>
>>         if (len < MINIMUM_ABBREV || len > GIT_SHA1_HEXSZ)
>>                 return -1;
>
> Well, I think that's the way I would have written that text at the top
> of the function.  I expect that we'll end up turning GIT_SHA1_HEXSZ into
> a global named something like current_hash_len via global
> search-and-replace, so it will always be the right length.
>
> The indexing should be safe because len is guaranteed to be sufficiently
> small, and I feel like it we would have seen it break by now if it had
> had an overflow.  i will always be in the range [0, 40) (for SHA-1), so
> i >> 1 should always be in [0, 20).
>
> Am I understanding you correctly and if so, does that assuage your
> concerns, or did you mean something else?

There's a disconnect between object_id (which goes with GIT_MAX_RAWSZ)
and the code here which still checks upper bound as GIT_SHA1_HEXSZ.
But I guess eventually GIT_SHA1_HEXSZ will be undefined and gone. This
is just a temporary state. So forget about my paranoid comment. All
good.
-- 
Duy

^ permalink raw reply	[flat|nested] 21+ messages in thread

* Re: [PATCH 06/20] builtin/receive-pack: convert portions to struct object_id
  2017-03-20 23:17     ` brian m. carlson
@ 2017-03-21 10:26       ` Duy Nguyen
  0 siblings, 0 replies; 21+ messages in thread
From: Duy Nguyen @ 2017-03-21 10:26 UTC (permalink / raw)
  To: brian m. carlson, Duy Nguyen, Git Mailing List, Jeff King

On Tue, Mar 21, 2017 at 6:17 AM, brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On Mon, Mar 20, 2017 at 07:56:17PM +0700, Duy Nguyen wrote:
>> On Sun, Mar 19, 2017 at 4:19 AM, brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>> > @@ -1489,23 +1489,24 @@ static struct command **queue_command(struct command **tail,
>> >                                       const char *line,
>> >                                       int linelen)
>> >  {
>> > -       unsigned char old_sha1[20], new_sha1[20];
>> > +       struct object_id old_oid, new_oid;
>> >         struct command *cmd;
>> >         const char *refname;
>> >         int reflen;
>> > +       const char *p;
>> >
>> > -       if (linelen < 83 ||
>> > -           line[40] != ' ' ||
>> > -           line[81] != ' ' ||
>> > -           get_sha1_hex(line, old_sha1) ||
>> > -           get_sha1_hex(line + 41, new_sha1))
>> > +       if (!linelen ||
>>
>> I think you can skip this. The old code needed "< 83" because of the
>> random accesses to [40] and [81] but you don't do that anymore.
>> parse_oid_hex() can handle empty hex strings fine.
>
> I just realized this looks fishy:
>
>         while (boc < eoc) {
>                 const char *eol = memchr(boc, '\n', eoc - boc);
>                 tail = queue_command(tail, boc, eol ? eol - boc : eoc - eol);
>                 boc = eol ? eol + 1 : eoc;
>         }
>
> If eol is NULL, we subtract it from eoc?  I mean, eol will be zero, so
> we get eoc, which seems like what we want.  I think I'm going to send in
> a separate patch to fix that, because clearly that's bizarre and not at
> all compliant with the C standard.

Eck.. Good eyes!

>> > +           parse_oid_hex(line, &old_oid, &p) ||
>> > +           *p++ != ' ' ||
>> > +           parse_oid_hex(p, &new_oid, &p) ||
>> > +           *p++ != ' ')
>>
>> maybe "|| *p)" as well? I think the old code, with "linelen < 83",
>> makes sure reflen is at least one. Not sure what FLEX_ALLOC_MEM would
>> do if reflen is zero.
>
> I don't think that line is actually guaranteed to be NUL-terminated.  It
> may be terminated instead by a newline, such as by
> queue_commands_from_cert.
>
> If we did get an empty reflen, we'd call xcalloc, where we will allocate
> exactly the size of the struct otherwise.  We'd then try to memcpy zero
> bytes into that location, and succeed.

Actually I think we allocate an extra byte for NUL as well, so
cmd->ref_name is still valid (as an empty string). Yes we should be
fine.
-- 
Duy

^ permalink raw reply	[flat|nested] 21+ messages in thread

end of thread, back to index

Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-03-18 21:19 [PATCH 00/20] object_id part 7 brian m. carlson
2017-03-18 21:19 ` [PATCH 01/20] Define new hash-size constants for allocating memory brian m. carlson
2017-03-18 21:19 ` [PATCH 02/20] Convert GIT_SHA1_HEXSZ used for allocation to GIT_MAX_HEXSZ brian m. carlson
2017-03-18 21:19 ` [PATCH 03/20] Convert GIT_SHA1_RAWSZ used for allocation to GIT_MAX_RAWSZ brian m. carlson
2017-03-18 21:19 ` [PATCH 05/20] builtin/pull: convert portions to struct object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 06/20] builtin/receive-pack: " brian m. carlson
2017-03-20 12:56   ` Duy Nguyen
2017-03-20 23:17     ` brian m. carlson
2017-03-21 10:26       ` Duy Nguyen
2017-03-18 21:19 ` [PATCH 08/20] parse-options-cb: convert sha1_array_append caller " brian m. carlson
2017-03-18 21:19 ` [PATCH 09/20] test-sha1-array: convert most code " brian m. carlson
2017-03-18 21:19 ` [PATCH 10/20] sha1_name: convert struct disambiguate_state to object_id brian m. carlson
2017-03-20 13:07   ` Duy Nguyen
2017-03-20 22:32     ` brian m. carlson
2017-03-21 10:17       ` Duy Nguyen
2017-03-18 21:19 ` [PATCH 11/20] sha1_name: convert disambiguate_hint_fn to take object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 12/20] submodule: convert check_for_new_submodule_commits to object_id brian m. carlson
2017-03-18 21:19 ` [PATCH 19/20] Rename sha1_array to oid_array brian m. carlson
2017-03-20 12:25   ` Duy Nguyen
2017-03-21  0:54     ` brian m. carlson
2017-03-20 13:14 ` [PATCH 00/20] object_id part 7 Duy Nguyen
2017-03-21  1:16   ` brian m. carlson

git@vger.kernel.org mailing list mirror (one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/
       or Tor2web: https://www.tor2web.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox