What's cooking (draft for #4's issue this month)

What's cooking (draft for #4's issue this month)

[Junio C Hamano]

A handful of topics have been merged to 'master' as the 9th batch of
this cycle.

    54a3917115 The ninth batch
    e0d4a63c09 Merge branch 'vs/completion-with-set-u'
    e6545201ad Merge branch 'ab/detox-config-gettext'
    a9414b86ac Merge branch 'gk/gitweb-redacted-email'
    8446b388b1 Merge branch 'cc/test-helper-bloom-usage-fix'
    2279289e95 Merge branch 'ab/send-email-validate-errors'
    4c6ac2da2c Merge branch 'tb/precompose-prefix-simplify'
    1d5fbd45c4 Merge branch 'fm/user-manual-use-preface'
    7b55441db1 Merge branch 'ab/perl-do-not-abuse-map'
    0623669fc6 Merge branch 'tb/pack-preferred-tips-to-give-bitmap'
    f63add4aa8 Merge branch 'jk/ref-filter-segfault-fix'

And a few topics have been merged to 'next'.

    cdadb2c621 Merge branch 'hn/reftable-tables-doc-update' into next
    bbe18a7b3a Merge branch 'jk/pack-objects-bitmap-progress-fix' into next
    41713a32bd Merge branch 'ah/merge-ort-ubsan-fix' into next
    35fb0e853d Merge branch 'ab/userdiff-tests' into next
    eb80d55a8c Merge branch 'ar/userdiff-scheme' into next

On 'seen' there are many topics that have not seen adequately
reviews; some tests are broken near its tip (I think they pass the
selftests by themselves).

    0e76df05ca Merge branch 'ps/rev-list-object-type-filter' into seen
    956fbceb2e ### breaks 6112 6113
    c007303ad4 Merge branch 'bc/hash-transition-interop-part-1' into seen
    4813f16161 ### breaks 0031

Re: What's cooking (draft for #4's issue this month)

[Patrick Steinhardt]

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

On Tue, Apr 13, 2021 at 06:11:39PM -0700, Junio C Hamano wrote:
> On 'seen' there are many topics that have not seen adequately
> reviews; some tests are broken near its tip (I think they pass the
> selftests by themselves).
> 
>     0e76df05ca Merge branch 'ps/rev-list-object-type-filter' into seen
>     956fbceb2e ### breaks 6112 6113
>     c007303ad4 Merge branch 'bc/hash-transition-interop-part-1' into seen
>     4813f16161 ### breaks 0031

Test breakage for the rev-list filter series has been a bad interaction
of d8883ed540 (object.c: stop supporting len == -1 in
type_from_string_gently(), 2021-03-28) and
ps/rev-list-object-type-filter. The following patch fixes this, which
I'll include in my next reroll of this series.

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 19e128ef11..33c7718a7a 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -100,7 +100,7 @@ static int gently_parse_list_objects_filter(
 		return 1;

 	} else if (skip_prefix(arg, "object:type=", &v0)) {
-		int type = type_from_string_gently(v0, -1);
+		int type = type_from_string_gently(v0, strlen(v0));
 		if (type < 0) {
 			strbuf_addstr(errbuf, _("expected 'object:type=<type>'"));
 			return 1;


The other breakages I see are caused by hn/reftable, where all tests in
t0031-reftables.sh cause segfaults. The root cause seems to be that
reading refs via the reftable backend doesn't initialize the `algo`
field of the OID, which is fixed via the following patch.

diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
index 130fd90e45..35fb7dd0a2 100644
--- a/refs/reftable-backend.c
+++ b/refs/reftable-backend.c
@@ -251,10 +251,10 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
 		ri->base.flags = 0;
 		switch (ri->ref.value_type) {
 		case REFTABLE_REF_VAL1:
-			hashcpy(ri->oid.hash, ri->ref.value.val1);
+			oidread(&ri->oid, ri->ref.value.val1);
 			break;
 		case REFTABLE_REF_VAL2:
-			hashcpy(ri->oid.hash, ri->ref.value.val2.value);
+			oidread(&ri->oid, ri->ref.value.val2.value);
 			break;
 		case REFTABLE_REF_SYMREF: {
 			int out_flags = 0;
@@ -299,7 +299,7 @@ static int reftable_ref_iterator_peel(struct ref_iterator *ref_iterator,
 	struct git_reftable_iterator *ri =
 		(struct git_reftable_iterator *)ref_iterator;
 	if (ri->ref.value_type == REFTABLE_REF_VAL2) {
-		hashcpy(peeled->hash, ri->ref.value.val2.target_value);
+		oidread(peeled, ri->ref.value.val2.target_value);
 		return 0;
 	}

@@ -977,7 +977,7 @@ git_reftable_reflog_ref_iterator_advance(struct ref_iterator *ref_iterator)

 		free(ri->last_name);
 		ri->last_name = xstrdup(ri->log.refname);
-		hashcpy(ri->oid.hash, ri->log.update.new_hash);
+		oidread(&ri->oid, ri->log.update.new_hash);
 		return ITER_OK;
 	}
 }
@@ -1090,8 +1090,8 @@ static int git_reftable_for_each_reflog_ent_newest_first(
 			break;
 		}

-		hashcpy(old_oid.hash, log.update.old_hash);
-		hashcpy(new_oid.hash, log.update.new_hash);
+		oidread(&old_oid, log.update.old_hash);
+		oidread(&new_oid, log.update.new_hash);

 		full_committer = fmt_ident(log.update.name, log.update.email,
 					   WANT_COMMITTER_IDENT,
@@ -1157,8 +1157,8 @@ static int git_reftable_for_each_reflog_ent_oldest_first(
 		struct object_id new_oid;
 		const char *full_committer = "";

-		hashcpy(old_oid.hash, log->update.old_hash);
-		hashcpy(new_oid.hash, log->update.new_hash);
+		oidread(&old_oid, log->update.old_hash);
+		oidread(&new_oid, log->update.new_hash);

 		full_committer = fmt_ident(log->update.name, log->update.email,
 					   WANT_COMMITTER_IDENT, NULL,
@@ -1330,8 +1330,8 @@ git_reftable_reflog_expire(struct ref_store *ref_store, const char *refname,
 		if (err > 0 || strcmp(log.refname, refname)) {
 			break;
 		}
-		hashcpy(ooid.hash, log.update.old_hash);
-		hashcpy(noid.hash, log.update.new_hash);
+		oidread(&ooid, log.update.old_hash);
+		oidread(&noid, log.update.new_hash);

 		if (should_prune_fn(&ooid, &noid, log.update.email,
 				    (timestamp_t)log.update.time,
@@ -1410,7 +1410,7 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
 		strbuf_addstr(referent, ref.value.symref);
 		*type |= REF_ISSYMREF;
 	} else if (reftable_ref_record_val1(&ref) != NULL) {
-		hashcpy(oid->hash, reftable_ref_record_val1(&ref));
+		oidread(oid, reftable_ref_record_val1(&ref));
 	} else {
 		*type |= REF_ISBROKEN;
 		errno = EINVAL;


Patrick

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

Re: What's cooking (draft for #4's issue this month)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> On Tue, Apr 13, 2021 at 06:11:39PM -0700, Junio C Hamano wrote:
>> On 'seen' there are many topics that have not seen adequately
>> reviews; some tests are broken near its tip (I think they pass the
>> selftests by themselves).
>> 
>>     0e76df05ca Merge branch 'ps/rev-list-object-type-filter' into seen
>>     956fbceb2e ### breaks 6112 6113
>>     c007303ad4 Merge branch 'bc/hash-transition-interop-part-1' into seen
>>     4813f16161 ### breaks 0031
>
> Test breakage for the rev-list filter series has been a bad interaction
> of d8883ed540 (object.c: stop supporting len == -1 in
> type_from_string_gently(), 2021-03-28) and
> ps/rev-list-object-type-filter.

Yuck.  The commit d8883ed5 (object.c: stop supporting len == -1 in
type_from_string_gently(), 2021-03-28) by itself may have meant
well, but in an environment where concurrent group development is
the norm, it is a change that can unnecessarily break other topics
easily, like it just did.

Perhaps a band-aid like this may be necessary.

diff --git i/object.c w/object.c
index 68de229f1a..b4bde9e444 100644
--- i/object.c
+++ w/object.c
@@ -39,6 +39,9 @@ int type_from_string_gently(const char *str, ssize_t len, int gentle)
 {
 	int i;
 
+	if (len < 0)
+		BUG("type-from-string-gently no longer allows unspecified length");
+
 	for (i = 1; i < ARRAY_SIZE(object_type_strings); i++)
 		if (!strncmp(str, object_type_strings[i], len) &&
 		    object_type_strings[i][len] == '\0')

> The following patch fixes this, which
> I'll include in my next reroll of this series.

Thanks.

> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
> index 19e128ef11..33c7718a7a 100644
> --- a/list-objects-filter-options.c
> +++ b/list-objects-filter-options.c
> @@ -100,7 +100,7 @@ static int gently_parse_list_objects_filter(
>  		return 1;
>
>  	} else if (skip_prefix(arg, "object:type=", &v0)) {
> -		int type = type_from_string_gently(v0, -1);
> +		int type = type_from_string_gently(v0, strlen(v0));
>  		if (type < 0) {
>  			strbuf_addstr(errbuf, _("expected 'object:type=<type>'"));
>  			return 1;

Re: What's cooking (draft for #4's issue this month)

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> The other breakages I see are caused by hn/reftable, where all tests in
> t0031-reftables.sh cause segfaults. The root cause seems to be that
> reading refs via the reftable backend doesn't initialize the `algo`
> field of the OID, which is fixed via the following patch.

OK.  On 'master' without bc/hash-transition-interop-part-1,
hashcpy() and oidread() both use the size from the_hash_algo
but with the topic, oidread() becomes the way to declare that
the given oid uses the_hash_algo.

static inline void hashcpy(unsigned char *sha_dst, const unsigned char *sha_src)
{
	memcpy(sha_dst, sha_src, the_hash_algo->rawsz);
}

static inline void oidread(struct object_id *oid, const unsigned char *hash)
{
	size_t rawsz = the_hash_algo->rawsz;
	memcpy(oid->hash, hash, rawsz);
	memset(oid->hash + rawsz, 0, GIT_MAX_RAWSZ - rawsz);
	oid->algo = hash_algo_by_ptr(the_hash_algo);
}

So the patch makes sense to directly squashed into hn/reftable topic
to (1) be a benign no-op in the master and to (2) fix an expected
semantic conflict with bc/hash-transition-interop-part-1 topic.

Thanks.



> diff --git a/refs/reftable-backend.c b/refs/reftable-backend.c
> index 130fd90e45..35fb7dd0a2 100644
> --- a/refs/reftable-backend.c
> +++ b/refs/reftable-backend.c
> @@ -251,10 +251,10 @@ static int reftable_ref_iterator_advance(struct ref_iterator *ref_iterator)
>  		ri->base.flags = 0;
>  		switch (ri->ref.value_type) {
>  		case REFTABLE_REF_VAL1:
> -			hashcpy(ri->oid.hash, ri->ref.value.val1);
> +			oidread(&ri->oid, ri->ref.value.val1);
>  			break;
>  		case REFTABLE_REF_VAL2:
> -			hashcpy(ri->oid.hash, ri->ref.value.val2.value);
> +			oidread(&ri->oid, ri->ref.value.val2.value);
>  			break;
>  		case REFTABLE_REF_SYMREF: {
>  			int out_flags = 0;
> @@ -299,7 +299,7 @@ static int reftable_ref_iterator_peel(struct ref_iterator *ref_iterator,
>  	struct git_reftable_iterator *ri =
>  		(struct git_reftable_iterator *)ref_iterator;
>  	if (ri->ref.value_type == REFTABLE_REF_VAL2) {
> -		hashcpy(peeled->hash, ri->ref.value.val2.target_value);
> +		oidread(peeled, ri->ref.value.val2.target_value);
>  		return 0;
>  	}
>
> @@ -977,7 +977,7 @@ git_reftable_reflog_ref_iterator_advance(struct ref_iterator *ref_iterator)
>
>  		free(ri->last_name);
>  		ri->last_name = xstrdup(ri->log.refname);
> -		hashcpy(ri->oid.hash, ri->log.update.new_hash);
> +		oidread(&ri->oid, ri->log.update.new_hash);
>  		return ITER_OK;
>  	}
>  }
> @@ -1090,8 +1090,8 @@ static int git_reftable_for_each_reflog_ent_newest_first(
>  			break;
>  		}
>
> -		hashcpy(old_oid.hash, log.update.old_hash);
> -		hashcpy(new_oid.hash, log.update.new_hash);
> +		oidread(&old_oid, log.update.old_hash);
> +		oidread(&new_oid, log.update.new_hash);
>
>  		full_committer = fmt_ident(log.update.name, log.update.email,
>  					   WANT_COMMITTER_IDENT,
> @@ -1157,8 +1157,8 @@ static int git_reftable_for_each_reflog_ent_oldest_first(
>  		struct object_id new_oid;
>  		const char *full_committer = "";
>
> -		hashcpy(old_oid.hash, log->update.old_hash);
> -		hashcpy(new_oid.hash, log->update.new_hash);
> +		oidread(&old_oid, log->update.old_hash);
> +		oidread(&new_oid, log->update.new_hash);
>
>  		full_committer = fmt_ident(log->update.name, log->update.email,
>  					   WANT_COMMITTER_IDENT, NULL,
> @@ -1330,8 +1330,8 @@ git_reftable_reflog_expire(struct ref_store *ref_store, const char *refname,
>  		if (err > 0 || strcmp(log.refname, refname)) {
>  			break;
>  		}
> -		hashcpy(ooid.hash, log.update.old_hash);
> -		hashcpy(noid.hash, log.update.new_hash);
> +		oidread(&ooid, log.update.old_hash);
> +		oidread(&noid, log.update.new_hash);
>
>  		if (should_prune_fn(&ooid, &noid, log.update.email,
>  				    (timestamp_t)log.update.time,
> @@ -1410,7 +1410,7 @@ static int git_reftable_read_raw_ref(struct ref_store *ref_store,
>  		strbuf_addstr(referent, ref.value.symref);
>  		*type |= REF_ISSYMREF;
>  	} else if (reftable_ref_record_val1(&ref) != NULL) {
> -		hashcpy(oid->hash, reftable_ref_record_val1(&ref));
> +		oidread(oid, reftable_ref_record_val1(&ref));
>  	} else {
>  		*type |= REF_ISBROKEN;
>  		errno = EINVAL;
>
>
> Patrick

Re: What's cooking (draft for #4's issue this month)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>> The following patch fixes this, which
>> I'll include in my next reroll of this series.

Hmph, I am not sure if that is wise.  The offending "-1 no longer is
accepted" comes from a topic that is not even in 'next', so you may
not want to depend on it.

It turns out that the two-argument form that passes -1 to the
function we see below as the preimage in your fix-up patch comes
from my conflict resolution.  Your original has

	type_from_string_gently(v0, -1, 1);

and the other topic wants to make two unrelated changes to the API,
i.e. making -1 no longer a valid "please count, as it is pointless
to force callers to always count" option, and drops "is this asking
to be gentle?" parameter.  It may be better to just update what is
recorded in my conflict resolution machinery, without making it

	type_from_string_gently(v0, strlen(v0), 1);

as it would be necessary to adjust when both topics are merged
anyway.

What's cooking (draft for #4's issue this month)

[Junio C Hamano]

Here is the (local) test status near the tip of 'seen', relative to
the integration result last night.

 * hn/reftable has a preparatory change to use oidread() instead of
   hashcpy() in places queued at its tip.  This is essentially a
   no-op in the codebase without bc/hash-transition-interop-part-1
   and would be a bugfix with that topic.  Please squash it into an
   appropriate step in the series when updating the topic in the
   future.

 * ab/unexpected-object-type topic has an assertion to catch
   semantic conflicts with topics in-flight queued at its tip.  It
   would probably be safe to carry it until the topioc is merged to
   'master' and then remove it after the dust settles.  Please
   squash it into an appropriate step in the series when updating
   the topic in the future.

 * The tip of 'seen' passes all the tests locally, except that t5540
   fails when compiled with CC=clang (http-push exits with signal
   11).  bc/hash-transition-interop-part-1, which is at the tip of
   'seen', seems to have this issue standalone.  FYI, here is what
   "clang --version" gives me:

    Debian clang version 11.0.1-2
    Target: x86_64-pc-linux-gnu
    Thread model: posix
    InstalledDir: /usr/bin

Credits for the corrections cited above goes to Patrick.

The attached is a summary of various topics.  Their status may be a
bit stale (I haven't re-reviewed them with respect to the latest
discussion on the list).

Thanks.

----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----

Expecting a (hopefully final) reroll.
 - ag/merge-strategies-in-c                                     03-17         #15

Expecting a reroll.
 - sg/bugreport-fixes                                           04-08          #1
 - ds/maintenance-prefetch-fix                                  04-10          #3
 - ma/t0091-bugreport-fix                                       04-12          #1

On hold, waiting for the "protections" topic to stablize.
 + ds/sparse-index                                              03-30/04-07   #21

On hold.  Probably needs either a reroll or incremental refinements.
 + dl/complete-stash                                            03-24/03-24    #3

Stalled
 - es/config-hooks                                              03-10         #36
 - ab/describe-tests-fix                                        04-12          #5
 - ab/pickaxe-pcre2                                             04-12         #22
 - ab/unexpected-object-type                                    04-14         #11

Undecided
 - mt/add-rm-in-sparse-checkout                                 04-08          #7
 - jh/rfc-builtin-fsmonitor                                     04-08         #23
 - mt/parallel-checkout-part-2                                  04-08          #5
 - bc/hash-transition-interop-part-1                            04-10         #16
 - mr/bisect-in-c-4                                             04-11          #4
 - ps/rev-list-object-type-filter                               04-12          #8
 - ab/fsck-unexpected-type                                      04-13          #6
 - rs/repack-without-loosening-promised-objects                 04-14          #2
 - ds/sparse-index-protections                                  04-14         #26

Waiting for reroll.
 - ah/plugleaks                                                 04-10          #9

Waiting for reviews to conclude.
 - ab/doc-lint                                                  04-10          #7
 - ab/rebase-no-reschedule-failed-exec                          04-10          #2
 - hn/refs-trace-errno                                          04-12          #1

Waiting for reviews.
 - jt/push-negotiation                                          04-08          #6
 - tb/multi-pack-bitmaps                                        04-10         #23
 - ab/svn-tests-set-e-fix                                       04-12          #2
 - ab/test-lib-updates                                          04-12         #16
 - ao/p4-avoid-decoding                                         04-12          #2
 - zh/trailer-cmd                                               04-12          #2
 - hn/reftable                                                  04-14         #21

Will merge to 'master'.
 + en/ort-perf-batch-10                                         03-18/04-07    #8
 + en/ort-readiness                                             03-20/04-08   #13
 + jz/apply-run-3way-first                                      04-06/04-08    #1
 + ab/complete-cherry-pick-head                                 04-07/04-09    #1
 + jz/apply-3way-cached                                         04-07/04-09    #1
 + ab/userdiff-tests                                            04-08/04-13    #9
 + ar/userdiff-scheme                                           04-08/04-13    #1
 + ah/merge-ort-ubsan-fix                                       04-12/04-13    #1
 + hn/reftable-tables-doc-update                                04-12/04-13    #1
 + jk/pack-objects-bitmap-progress-fix                          04-12/04-13    #1

Will merge to 'next'.
 - jt/fetch-pack-request-fix                                    04-08          #1
 - ab/detox-gettext-tests                                       04-13          #1
 - ab/usage-error-docs                                          04-13          #3
 - jk/promisor-optim                                            04-13          #3
 - so/log-diff-merge                                            04-13          #5
 - ps/config-global-override                                    04-13          #3

Re: What's cooking (draft for #4's issue this month)

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

>  * The tip of 'seen' passes all the tests locally, except that t5540
>    fails when compiled with CC=clang (http-push exits with signal
>    11).  bc/hash-transition-interop-part-1, which is at the tip of
>    'seen', seems to have this issue standalone.

BTW https://github.com/git/git/runs/2347905320?check_suite_focus=true#step:5:203
shows this.

Re: What's cooking (draft for #4's issue this month)

[brian m. carlson]

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

On 2021-04-14 at 23:22:19, Junio C Hamano wrote:
> Here is the (local) test status near the tip of 'seen', relative to
> the integration result last night.
> 
>  * hn/reftable has a preparatory change to use oidread() instead of
>    hashcpy() in places queued at its tip.  This is essentially a
>    no-op in the codebase without bc/hash-transition-interop-part-1
>    and would be a bugfix with that topic.  Please squash it into an
>    appropriate step in the series when updating the topic in the
>    future.
> 
>  * ab/unexpected-object-type topic has an assertion to catch
>    semantic conflicts with topics in-flight queued at its tip.  It
>    would probably be safe to carry it until the topioc is merged to
>    'master' and then remove it after the dust settles.  Please
>    squash it into an appropriate step in the series when updating
>    the topic in the future.
> 
>  * The tip of 'seen' passes all the tests locally, except that t5540
>    fails when compiled with CC=clang (http-push exits with signal
>    11).  bc/hash-transition-interop-part-1, which is at the tip of
>    'seen', seems to have this issue standalone.  FYI, here is what
>    "clang --version" gives me:
> 
>     Debian clang version 11.0.1-2
>     Target: x86_64-pc-linux-gnu
>     Thread model: posix
>     InstalledDir: /usr/bin

You should expect a reroll, so feel free to drop this if it breaks
things for now and I'll figure out where things are going wrong.
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

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

Re: What's cooking (draft for #4's issue this month)

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> On 2021-04-14 at 23:22:19, Junio C Hamano wrote:
>> Here is the (local) test status near the tip of 'seen', relative to
>> the integration result last night.
>> 
>>  * hn/reftable has a preparatory change to use oidread() instead of
>>    hashcpy() in places queued at its tip.  This is essentially a
>>    no-op in the codebase without bc/hash-transition-interop-part-1
>>    and would be a bugfix with that topic.  Please squash it into an
>>    appropriate step in the series when updating the topic in the
>>    future.
>> 
>>  * ab/unexpected-object-type topic has an assertion to catch
>>    semantic conflicts with topics in-flight queued at its tip.  It
>>    would probably be safe to carry it until the topioc is merged to
>>    'master' and then remove it after the dust settles.  Please
>>    squash it into an appropriate step in the series when updating
>>    the topic in the future.
>> 
>>  * The tip of 'seen' passes all the tests locally, except that t5540
>>    fails when compiled with CC=clang (http-push exits with signal
>>    11).  bc/hash-transition-interop-part-1, which is at the tip of
>>    'seen', seems to have this issue standalone.  FYI, here is what
>>    "clang --version" gives me:
>> 
>>     Debian clang version 11.0.1-2
>>     Target: x86_64-pc-linux-gnu
>>     Thread model: posix
>>     InstalledDir: /usr/bin
>
> You should expect a reroll, so feel free to drop this if it breaks
> things for now and I'll figure out where things are going wrong.

I actually do appreciate the topic to be in 'seen', as these
integration exercises tend to serve as an early warning for
impending messy conflicts I'll need to be worried about.

I do worry about the memory requirement bloat of the object_id
structure, as we do need to keep one instance per object in-core,
but the squashable fix for the reftable topic given by Patrick
to replace use of hashcpy() with oidread() is still a good idea even
if we are going to use a different mechanism to keep track of which
object_id instance uses what hash algorithm, so again I am happy to
have seen your bc/hash-transition-interop-part-1 topic and had an
early chance to make it collide with others ;-)

Thanks.

Re: What's cooking (draft for #4's issue this month)

[Jeff King]

On Wed, Apr 14, 2021 at 02:53:19PM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> >> The following patch fixes this, which
> >> I'll include in my next reroll of this series.
> 
> Hmph, I am not sure if that is wise.  The offending "-1 no longer is
> accepted" comes from a topic that is not even in 'next', so you may
> not want to depend on it.
> 
> It turns out that the two-argument form that passes -1 to the
> function we see below as the preimage in your fix-up patch comes
> from my conflict resolution.  Your original has
> 
> 	type_from_string_gently(v0, -1, 1);
> 
> and the other topic wants to make two unrelated changes to the API,
> i.e. making -1 no longer a valid "please count, as it is pointless
> to force callers to always count" option, and drops "is this asking
> to be gentle?" parameter.  It may be better to just update what is
> recorded in my conflict resolution machinery, without making it
> 
> 	type_from_string_gently(v0, strlen(v0), 1);
> 
> as it would be necessary to adjust when both topics are merged
> anyway.

Heh. I wonder if I was too optimistic in my review at:

  https://lore.kernel.org/git/YHCYfeRsHU34ZF%2Fl@coredump.intra.peff.net/

-Peff

Re: What's cooking (draft for #4's issue this month)

[Han-Wen Nienhuys]

On Thu, Apr 15, 2021 at 1:22 AM Junio C Hamano <gitster@pobox.com> wrote:
>  * hn/reftable has a preparatory change to use oidread() instead of
>    hashcpy() in places queued at its tip.  This is essentially a
>    no-op in the codebase without bc/hash-transition-interop-part-1
>    and would be a bugfix with that topic.  Please squash it into an
>    appropriate step in the series when updating the topic in the
>    future.

Done. Thanks for the fix.

-- 
Han-Wen Nienhuys - Google Munich
I work 80%. Don't expect answers from me on Fridays.
--

Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich

Registergericht und -nummer: Hamburg, HRB 86891

Sitz der Gesellschaft: Hamburg

Geschäftsführer: Paul Manicle, Halimah DeLaine Prado

Re: What's cooking (draft for #4's issue this month)

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Heh. I wonder if I was too optimistic in my review at:
>
>   https://lore.kernel.org/git/YHCYfeRsHU34ZF%2Fl@coredump.intra.peff.net/

Yes, merging is hard.  When merging a topic X into 'seen', I can
review "log -p ..MERGE_HEAD" and keep the core changes the topic
wants to do in my head while reviewing the conflicting parts, but
this is the other "log -p MERGE_HEAD.." direction where all the
little cuts _other_ topics inflicted on the codebase before an
attempted merge, and something seemingly benign and well intended
change like "well nobody passes -1 to ask us to count _now_, so
let's drop the feature without even bothering to check nobody does
so in the future" can easily be missed.

Re: What's cooking (draft for #4's issue this month)

[brian m. carlson]

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

On 2021-04-15 at 06:37:11, Junio C Hamano wrote:
> I actually do appreciate the topic to be in 'seen', as these
> integration exercises tend to serve as an early warning for
> impending messy conflicts I'll need to be worried about.
> 
> I do worry about the memory requirement bloat of the object_id
> structure, as we do need to keep one instance per object in-core,
> but the squashable fix for the reftable topic given by Patrick
> to replace use of hashcpy() with oidread() is still a good idea even
> if we are going to use a different mechanism to keep track of which
> object_id instance uses what hash algorithm, so again I am happy to
> have seen your bc/hash-transition-interop-part-1 topic and had an
> early chance to make it collide with others ;-)

I'm still working on a full reroll for the series including performance
measurements (since this took me much longer than I expected it would),
but I wanted to include a patch for the segfault below to keep things
tidy in the mean time.  I should point out that this doesn't appear to
crash when running the testsuite in SHA-256 mode for reasons I'm not
sure about, which explains why I didn't see it originally.

---- %< ----
From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
From: "brian m. carlson" <sandals@crustytoothpaste.net>
Date: Sun, 18 Apr 2021 22:56:04 +0000
Subject: [PATCH] http-push: set algorithm when reading object ID

In most places in the codebase, we use oidread to properly read an
object ID into a struct object_id.  However, in the HTTP code, we end up
needing to parse a loose object path with a slash in it, so we can't do
that.  Let's instead explicitly set the algorithm in this function so we
can rely on it in the future.

Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
---
 http-push.c | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/http-push.c b/http-push.c
index b60d5fcc85..5675cd7708 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1022,6 +1022,8 @@ static void remote_ls(const char *path, int flags,
 /* extract hex from sharded "xx/x{38}" filename */
 static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
 {
+	oid->algo = hash_algo_by_ptr(the_hash_algo);
+
 	if (strlen(path) != the_hash_algo->hexsz + 1)
 		return -1;
 
---- %< ----
-- 
brian m. carlson (he/him or they/them)
Houston, Texas, US

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

Re: What's cooking (draft for #4's issue this month)

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I'm still working on a full reroll for the series including performance
> measurements (since this took me much longer than I expected it would),
> but I wanted to include a patch for the segfault below to keep things
> tidy in the mean time.  I should point out that this doesn't appear to
> crash when running the testsuite in SHA-256 mode for reasons I'm not
> sure about, which explains why I didn't see it originally.

Thanks; will apply and keep it with the latest round for now.  

>
> ---- %< ----
> From 0000000000000000000000000000000000000000 Mon Sep 17 00:00:00 2001
> From: "brian m. carlson" <sandals@crustytoothpaste.net>
> Date: Sun, 18 Apr 2021 22:56:04 +0000
> Subject: [PATCH] http-push: set algorithm when reading object ID
>
> In most places in the codebase, we use oidread to properly read an
> object ID into a struct object_id.  However, in the HTTP code, we end up
> needing to parse a loose object path with a slash in it, so we can't do
> that.  Let's instead explicitly set the algorithm in this function so we
> can rely on it in the future.
>
> Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net>
> ---
>  http-push.c | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/http-push.c b/http-push.c
> index b60d5fcc85..5675cd7708 100644
> --- a/http-push.c
> +++ b/http-push.c
> @@ -1022,6 +1022,8 @@ static void remote_ls(const char *path, int flags,
>  /* extract hex from sharded "xx/x{38}" filename */
>  static int get_oid_hex_from_objpath(const char *path, struct object_id *oid)
>  {
> +	oid->algo = hash_algo_by_ptr(the_hash_algo);
> +
>  	if (strlen(path) != the_hash_algo->hexsz + 1)
>  		return -1;
>  
> ---- %< ----