[PATCH 1/2] commit: convert lookup_commit_graft to struct object_id

[PATCH 1/2] commit: convert lookup_commit_graft to struct object_id

[Stefan Beller]

With this patch, commit.h doesn't contain the string 'sha1' any more.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Before diving into the "RFC object store" series further, I want to get
rid of the final sha1s in {commit,tag}.{c,h}.

 commit.c  | 6 +++---
 commit.h  | 2 +-
 fsck.c    | 2 +-
 shallow.c | 4 ++--
 4 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/commit.c b/commit.c
index cbfd689939..e0888cf0f7 100644
--- a/commit.c
+++ b/commit.c
@@ -199,11 +199,11 @@ static void prepare_commit_graft(void)
 	commit_graft_prepared = 1;
 }
 
-struct commit_graft *lookup_commit_graft(const unsigned char *sha1)
+struct commit_graft *lookup_commit_graft(const struct object_id *oid)
 {
 	int pos;
 	prepare_commit_graft();
-	pos = commit_graft_pos(sha1);
+	pos = commit_graft_pos(oid->hash);
 	if (pos < 0)
 		return NULL;
 	return commit_graft[pos];
@@ -335,7 +335,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
 	bufptr += tree_entry_len + 1; /* "tree " + "hex sha1" + "\n" */
 	pptr = &item->parents;
 
-	graft = lookup_commit_graft(item->object.oid.hash);
+	graft = lookup_commit_graft(&item->object.oid);
 	while (bufptr + parent_entry_len < tail && !memcmp(bufptr, "parent ", 7)) {
 		struct commit *new_parent;
 
diff --git a/commit.h b/commit.h
index 4127c298cb..6d857f06c1 100644
--- a/commit.h
+++ b/commit.h
@@ -249,7 +249,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
 
 struct commit_graft *read_graft_line(char *buf, int len);
 int register_commit_graft(struct commit_graft *, int);
-struct commit_graft *lookup_commit_graft(const unsigned char *sha1);
+struct commit_graft *lookup_commit_graft(const struct object_id *oid);
 
 extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
 extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos);
diff --git a/fsck.c b/fsck.c
index b4204d772b..2d2d2e9432 100644
--- a/fsck.c
+++ b/fsck.c
@@ -736,7 +736,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
 		buffer += 41;
 		parent_line_count++;
 	}
-	graft = lookup_commit_graft(commit->object.oid.hash);
+	graft = lookup_commit_graft(&commit->object.oid);
 	parent_count = commit_list_count(commit->parents);
 	if (graft) {
 		if (graft->nr_parent == -1 && !parent_count)
diff --git a/shallow.c b/shallow.c
index 54359d5490..f5591e56da 100644
--- a/shallow.c
+++ b/shallow.c
@@ -107,7 +107,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
 		cur_depth++;
 		if ((depth != INFINITE_DEPTH && cur_depth >= depth) ||
 		    (is_repository_shallow() && !commit->parents &&
-		     (graft = lookup_commit_graft(commit->object.oid.hash)) != NULL &&
+		     (graft = lookup_commit_graft(&commit->object.oid)) != NULL &&
 		     graft->nr_parent < 0)) {
 			commit_list_insert(commit, &result);
 			commit->object.flags |= shallow_flag;
@@ -398,7 +398,7 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)
 	for (i = 0; i < sa->nr; i++) {
 		if (has_object_file(sa->oid + i)) {
 			struct commit_graft *graft;
-			graft = lookup_commit_graft(sa->oid[i].hash);
+			graft = lookup_commit_graft(&sa->oid[i]);
 			if (graft && graft->nr_parent < 0)
 				continue;
 			info->ours[info->nr_ours++] = i;
-- 
2.13.3.650.g5ebc568d5d

[PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[Stefan Beller]

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 builtin/tag.c        |  2 +-
 builtin/verify-tag.c |  9 +++++----
 tag.c                | 10 +++++-----
 tag.h                |  2 +-
 4 files changed, 12 insertions(+), 11 deletions(-)

diff --git a/builtin/tag.c b/builtin/tag.c
index 01154ea8dc..b25bf8daa2 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -111,7 +111,7 @@ static int verify_tag(const char *name, const char *ref,
 	if (fmt_pretty)
 		flags = GPG_VERIFY_OMIT_STATUS;
 
-	if (gpg_verify_tag(oid->hash, name, flags))
+	if (gpg_verify_tag(oid, name, flags))
 		return -1;
 
 	if (fmt_pretty)
diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
index f9a5f7535a..ed8329340f 100644
--- a/builtin/verify-tag.c
+++ b/builtin/verify-tag.c
@@ -56,20 +56,21 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
 	}
 
 	while (i < argc) {
-		unsigned char sha1[20];
+		struct object_id oid;
 		const char *name = argv[i++];
-		if (get_sha1(name, sha1)) {
+
+		if (get_oid(name, &oid)) {
 			had_error = !!error("tag '%s' not found.", name);
 			continue;
 		}
 
-		if (gpg_verify_tag(sha1, name, flags)) {
+		if (gpg_verify_tag(&oid, name, flags)) {
 			had_error = 1;
 			continue;
 		}
 
 		if (fmt_pretty)
-			pretty_print_ref(name, sha1, fmt_pretty);
+			pretty_print_ref(name, oid.hash, fmt_pretty);
 	}
 	return had_error;
 }
diff --git a/tag.c b/tag.c
index 47f60ae151..7e10acfb6e 100644
--- a/tag.c
+++ b/tag.c
@@ -33,7 +33,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
 	return ret;
 }
 
-int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
+int gpg_verify_tag(const struct object_id *oid, const char *name_to_report,
 		unsigned flags)
 {
 	enum object_type type;
@@ -41,20 +41,20 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
 	unsigned long size;
 	int ret;
 
-	type = sha1_object_info(sha1, NULL);
+	type = sha1_object_info(oid->hash, NULL);
 	if (type != OBJ_TAG)
 		return error("%s: cannot verify a non-tag object of type %s.",
 				name_to_report ?
 				name_to_report :
-				find_unique_abbrev(sha1, DEFAULT_ABBREV),
+				find_unique_abbrev(oid->hash, DEFAULT_ABBREV),
 				typename(type));
 
-	buf = read_sha1_file(sha1, &type, &size);
+	buf = read_sha1_file(oid->hash, &type, &size);
 	if (!buf)
 		return error("%s: unable to read file.",
 				name_to_report ?
 				name_to_report :
-				find_unique_abbrev(sha1, DEFAULT_ABBREV));
+				find_unique_abbrev(oid->hash, DEFAULT_ABBREV));
 
 	ret = run_gpg_verify(buf, size, flags);
 
diff --git a/tag.h b/tag.h
index fdfcb4a84a..d469534e82 100644
--- a/tag.h
+++ b/tag.h
@@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
 extern int parse_tag(struct tag *item);
 extern struct object *deref_tag(struct object *, const char *, int);
 extern struct object *deref_tag_noverify(struct object *);
-extern int gpg_verify_tag(const unsigned char *sha1,
+extern int gpg_verify_tag(const struct object_id *oid,
 		const char *name_to_report, unsigned flags);
 
 #endif /* TAG_H */
-- 
2.13.3.650.g5ebc568d5d

Re: [PATCH 1/2] commit: convert lookup_commit_graft to struct object_id

[brian m. carlson]

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

On Wed, Jul 12, 2017 at 05:44:14PM -0700, Stefan Beller wrote:
> With this patch, commit.h doesn't contain the string 'sha1' any more.

From a relatively quick look, these look sane to me.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

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

Re: [PATCH 1/2] commit: convert lookup_commit_graft to struct object_id

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> With this patch, commit.h doesn't contain the string 'sha1' any more.

;-)  Nice.

commit_graft_pos() still thinks we only deal with SHA-1, but that
needs to wait for oid_pos().  The function has only two callers that
do not pass X->oid.hash so it may be a good candidate to convert.

>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Before diving into the "RFC object store" series further, I want to get
> rid of the final sha1s in {commit,tag}.{c,h}.
>
>  commit.c  | 6 +++---
>  commit.h  | 2 +-
>  fsck.c    | 2 +-
>  shallow.c | 4 ++--
>  4 files changed, 7 insertions(+), 7 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index cbfd689939..e0888cf0f7 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -199,11 +199,11 @@ static void prepare_commit_graft(void)
>  	commit_graft_prepared = 1;
>  }
>  
> -struct commit_graft *lookup_commit_graft(const unsigned char *sha1)
> +struct commit_graft *lookup_commit_graft(const struct object_id *oid)
>  {
>  	int pos;
>  	prepare_commit_graft();
> -	pos = commit_graft_pos(sha1);
> +	pos = commit_graft_pos(oid->hash);
>  	if (pos < 0)
>  		return NULL;
>  	return commit_graft[pos];
> @@ -335,7 +335,7 @@ int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long s
>  	bufptr += tree_entry_len + 1; /* "tree " + "hex sha1" + "\n" */
>  	pptr = &item->parents;
>  
> -	graft = lookup_commit_graft(item->object.oid.hash);
> +	graft = lookup_commit_graft(&item->object.oid);
>  	while (bufptr + parent_entry_len < tail && !memcmp(bufptr, "parent ", 7)) {
>  		struct commit *new_parent;
>  
> diff --git a/commit.h b/commit.h
> index 4127c298cb..6d857f06c1 100644
> --- a/commit.h
> +++ b/commit.h
> @@ -249,7 +249,7 @@ typedef int (*each_commit_graft_fn)(const struct commit_graft *, void *);
>  
>  struct commit_graft *read_graft_line(char *buf, int len);
>  int register_commit_graft(struct commit_graft *, int);
> -struct commit_graft *lookup_commit_graft(const unsigned char *sha1);
> +struct commit_graft *lookup_commit_graft(const struct object_id *oid);
>  
>  extern struct commit_list *get_merge_bases(struct commit *rev1, struct commit *rev2);
>  extern struct commit_list *get_merge_bases_many(struct commit *one, int n, struct commit **twos);
> diff --git a/fsck.c b/fsck.c
> index b4204d772b..2d2d2e9432 100644
> --- a/fsck.c
> +++ b/fsck.c
> @@ -736,7 +736,7 @@ static int fsck_commit_buffer(struct commit *commit, const char *buffer,
>  		buffer += 41;
>  		parent_line_count++;
>  	}
> -	graft = lookup_commit_graft(commit->object.oid.hash);
> +	graft = lookup_commit_graft(&commit->object.oid);
>  	parent_count = commit_list_count(commit->parents);
>  	if (graft) {
>  		if (graft->nr_parent == -1 && !parent_count)
> diff --git a/shallow.c b/shallow.c
> index 54359d5490..f5591e56da 100644
> --- a/shallow.c
> +++ b/shallow.c
> @@ -107,7 +107,7 @@ struct commit_list *get_shallow_commits(struct object_array *heads, int depth,
>  		cur_depth++;
>  		if ((depth != INFINITE_DEPTH && cur_depth >= depth) ||
>  		    (is_repository_shallow() && !commit->parents &&
> -		     (graft = lookup_commit_graft(commit->object.oid.hash)) != NULL &&
> +		     (graft = lookup_commit_graft(&commit->object.oid)) != NULL &&
>  		     graft->nr_parent < 0)) {
>  			commit_list_insert(commit, &result);
>  			commit->object.flags |= shallow_flag;
> @@ -398,7 +398,7 @@ void prepare_shallow_info(struct shallow_info *info, struct oid_array *sa)
>  	for (i = 0; i < sa->nr; i++) {
>  		if (has_object_file(sa->oid + i)) {
>  			struct commit_graft *graft;
> -			graft = lookup_commit_graft(sa->oid[i].hash);
> +			graft = lookup_commit_graft(&sa->oid[i]);
>  			if (graft && graft->nr_parent < 0)
>  				continue;
>  			info->ours[info->nr_ours++] = i;

Re: [PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

>  		if (fmt_pretty)
> -			pretty_print_ref(name, sha1, fmt_pretty);
> +			pretty_print_ref(name, oid.hash, fmt_pretty);

The next step would be to have pretty_print_ref() to take an oid; as
there are only two callers to it, both of which pass oid->hash to it,
that should be a small and conflict-free conversion.

>  
> -	type = sha1_object_info(sha1, NULL);
> +	type = sha1_object_info(oid->hash, NULL);

sha1_object_info() has a handful of callers that do not pass the
pointer to the hash field in an existing oid object, but it does not
look too bad as a candidate for conversion.

>  	if (!buf)
>  		return error("%s: unable to read file.",
>  				name_to_report ?
>  				name_to_report :
> -				find_unique_abbrev(sha1, DEFAULT_ABBREV));
> +				find_unique_abbrev(oid->hash, DEFAULT_ABBREV));

So does find_unique_abbrev().

Overall both patches look good.  Thanks.

Re: [PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
> index f9a5f7535a..ed8329340f 100644
> --- a/builtin/verify-tag.c
> +++ b/builtin/verify-tag.c
> @@ -56,20 +56,21 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>  	}
>  
>  	while (i < argc) {
> -		unsigned char sha1[20];
> +		struct object_id oid;
>  		const char *name = argv[i++];
> -		if (get_sha1(name, sha1)) {
> +
> +		if (get_oid(name, &oid)) {
>  			had_error = !!error("tag '%s' not found.", name);
>  			continue;
>  		}

This part is already done, it seems, in bc/object-id topic, even
though other parts are not yet done?

>  
> -		if (gpg_verify_tag(sha1, name, flags)) {
> +		if (gpg_verify_tag(&oid, name, flags)) {
>  			had_error = 1;
>  			continue;
>  		}
>  
>  		if (fmt_pretty)
> -			pretty_print_ref(name, sha1, fmt_pretty);
> +			pretty_print_ref(name, oid.hash, fmt_pretty);
>  	}
>  	return had_error;
>  }
> diff --git a/tag.c b/tag.c
> index 47f60ae151..7e10acfb6e 100644
> --- a/tag.c
> +++ b/tag.c
> @@ -33,7 +33,7 @@ static int run_gpg_verify(const char *buf, unsigned long size, unsigned flags)
>  	return ret;
>  }
>  
> -int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
> +int gpg_verify_tag(const struct object_id *oid, const char *name_to_report,
>  		unsigned flags)
>  {
>  	enum object_type type;
> @@ -41,20 +41,20 @@ int gpg_verify_tag(const unsigned char *sha1, const char *name_to_report,
>  	unsigned long size;
>  	int ret;
>  
> -	type = sha1_object_info(sha1, NULL);
> +	type = sha1_object_info(oid->hash, NULL);
>  	if (type != OBJ_TAG)
>  		return error("%s: cannot verify a non-tag object of type %s.",
>  				name_to_report ?
>  				name_to_report :
> -				find_unique_abbrev(sha1, DEFAULT_ABBREV),
> +				find_unique_abbrev(oid->hash, DEFAULT_ABBREV),
>  				typename(type));
>  
> -	buf = read_sha1_file(sha1, &type, &size);
> +	buf = read_sha1_file(oid->hash, &type, &size);
>  	if (!buf)
>  		return error("%s: unable to read file.",
>  				name_to_report ?
>  				name_to_report :
> -				find_unique_abbrev(sha1, DEFAULT_ABBREV));
> +				find_unique_abbrev(oid->hash, DEFAULT_ABBREV));
>  
>  	ret = run_gpg_verify(buf, size, flags);
>  
> diff --git a/tag.h b/tag.h
> index fdfcb4a84a..d469534e82 100644
> --- a/tag.h
> +++ b/tag.h
> @@ -17,7 +17,7 @@ extern int parse_tag_buffer(struct tag *item, const void *data, unsigned long si
>  extern int parse_tag(struct tag *item);
>  extern struct object *deref_tag(struct object *, const char *, int);
>  extern struct object *deref_tag_noverify(struct object *);
> -extern int gpg_verify_tag(const unsigned char *sha1,
> +extern int gpg_verify_tag(const struct object_id *oid,
>  		const char *name_to_report, unsigned flags);
>  
>  #endif /* TAG_H */

Re: [PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[Stefan Beller]

On Thu, Jul 13, 2017 at 1:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>> index f9a5f7535a..ed8329340f 100644
>> --- a/builtin/verify-tag.c
>> +++ b/builtin/verify-tag.c
>> @@ -56,20 +56,21 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>>       }
>>
>>       while (i < argc) {
>> -             unsigned char sha1[20];
>> +             struct object_id oid;
>>               const char *name = argv[i++];
>> -             if (get_sha1(name, sha1)) {
>> +
>> +             if (get_oid(name, &oid)) {
>>                       had_error = !!error("tag '%s' not found.", name);
>>                       continue;
>>               }
>
> This part is already done, it seems, in bc/object-id topic, even
> though other parts are not yet done?

Oops. I assumed the latest bc/object-id would have been in master
already, but after checking it is not. 967635dc3c2
(builtin/verify-tag: convert to struct object_id)
converts this part, although there are 2 differences:
* I added a stray newline before get_oid
* The argument to gpg_verify_tag is a sha1 or oid

So yes, this produces a merge conflict. :/

There rest (tag.{c,h}, builtin/tag.c) is not found in brians series.

Re: [PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> On Thu, Jul 13, 2017 at 1:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Stefan Beller <sbeller@google.com> writes:
>>
>>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>>> index f9a5f7535a..ed8329340f 100644
>>> --- a/builtin/verify-tag.c
>>> +++ b/builtin/verify-tag.c
>>> @@ -56,20 +56,21 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>>>       }
>>>
>>>       while (i < argc) {
>>> -             unsigned char sha1[20];
>>> +             struct object_id oid;
>>>               const char *name = argv[i++];
>>> -             if (get_sha1(name, sha1)) {
>>> +
>>> +             if (get_oid(name, &oid)) {
>>>                       had_error = !!error("tag '%s' not found.", name);
>>>                       continue;
>>>               }
>>
>> This part is already done, it seems, in bc/object-id topic, even
>> though other parts are not yet done?
>
> Oops. I assumed the latest bc/object-id would have been in master
> already, but after checking it is not. 967635dc3c2
> (builtin/verify-tag: convert to struct object_id)
> converts this part, although there are 2 differences:
> * I added a stray newline before get_oid
> * The argument to gpg_verify_tag is a sha1 or oid
>
> So yes, this produces a merge conflict. :/

That is OK.  This actually shouldn't create any meaningful conflict.
Both try to do the same code, with only a blank-line difference.

As Brian said bc/object-id would be rerolled, I was wondering if I
should queue these two patches (even though I already queued them)
myself, or it would be better for you to send them to Brian to make
it part of his series.

Re: [PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[Stefan Beller]

On Thu, Jul 13, 2017 at 2:23 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Stefan Beller <sbeller@google.com> writes:
>
>> On Thu, Jul 13, 2017 at 1:52 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Stefan Beller <sbeller@google.com> writes:
>>>
>>>> diff --git a/builtin/verify-tag.c b/builtin/verify-tag.c
>>>> index f9a5f7535a..ed8329340f 100644
>>>> --- a/builtin/verify-tag.c
>>>> +++ b/builtin/verify-tag.c
>>>> @@ -56,20 +56,21 @@ int cmd_verify_tag(int argc, const char **argv, const char *prefix)
>>>>       }
>>>>
>>>>       while (i < argc) {
>>>> -             unsigned char sha1[20];
>>>> +             struct object_id oid;
>>>>               const char *name = argv[i++];
>>>> -             if (get_sha1(name, sha1)) {
>>>> +
>>>> +             if (get_oid(name, &oid)) {
>>>>                       had_error = !!error("tag '%s' not found.", name);
>>>>                       continue;
>>>>               }
>>>
>>> This part is already done, it seems, in bc/object-id topic, even
>>> though other parts are not yet done?
>>
>> Oops. I assumed the latest bc/object-id would have been in master
>> already, but after checking it is not. 967635dc3c2
>> (builtin/verify-tag: convert to struct object_id)
>> converts this part, although there are 2 differences:
>> * I added a stray newline before get_oid
>> * The argument to gpg_verify_tag is a sha1 or oid
>>
>> So yes, this produces a merge conflict. :/
>
> That is OK.  This actually shouldn't create any meaningful conflict.
> Both try to do the same code, with only a blank-line difference.
>
> As Brian said bc/object-id would be rerolled, I was wondering if I
> should queue these two patches (even though I already queued them)
> myself, or it would be better for you to send them to Brian to make
> it part of his series.

+cc Brian

Snarkily I wanted to link to an essay "large patch series
considered harmful"[1], but could not find any. So a couple
of bullet points:
(a) humans dislike larger series, hence fewer reviewers
(b) the likelihood of a mistake in new code is proportional to its size
  We can use the number of patches as a proxy for size
(c) If a mistake is found, the whole series needs rerolling.
  The effort of rerolling a series can be approximated with
  its size as well.

From combining (b) and (c), we conclude that the effort to
land a patch series is O(n^2) with n as the number of patches.
Also from (a) we conclude that two smaller series containing
the same output as one large series, has better code quality.
So with that, we conclude that all series shall be as small
as possible.

So I'd ask to queue these 2 separately, asking Brian to drop
"builtin/verify-tag: convert to struct object_id"

[1] https://www.cs.princeton.edu/~dpw/papers/hotos.pdf, 2005
    seems interesting to me in hindsight.

I can also send my patches to Brian, as you (both) like.

Thanks,
Stefan

Re: [PATCH 2/2] tag: convert gpg_verify_tag to use struct object_id

[brian m. carlson]

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

On Thu, Jul 13, 2017 at 02:49:28PM -0700, Stefan Beller wrote:
> Snarkily I wanted to link to an essay "large patch series
> considered harmful"[1], but could not find any. So a couple
> of bullet points:
> (a) humans dislike larger series, hence fewer reviewers
> (b) the likelihood of a mistake in new code is proportional to its size
>   We can use the number of patches as a proxy for size
> (c) If a mistake is found, the whole series needs rerolling.
>   The effort of rerolling a series can be approximated with
>   its size as well.
> 
> From combining (b) and (c), we conclude that the effort to
> land a patch series is O(n^2) with n as the number of patches.
> Also from (a) we conclude that two smaller series containing
> the same output as one large series, has better code quality.
> So with that, we conclude that all series shall be as small
> as possible.
> 
> So I'd ask to queue these 2 separately, asking Brian to drop
> "builtin/verify-tag: convert to struct object_id"
> 
> [1] https://www.cs.princeton.edu/~dpw/papers/hotos.pdf, 2005
>     seems interesting to me in hindsight.
> 
> I can also send my patches to Brian, as you (both) like.

I'm literally about to send my series out; I rebased and tested it last
night.  I don't care whose patch gets applied, but to avoid needing to
rebase and retest my series, I'm going to send it out as it is.  Junio
can apply my series on top of yours, in which case he can drop the
relevant patch from my series.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204

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