merge-ort: only do pointer arithmetic for non-empty lists #930

ahunt · 2021-04-10T07:41:11Z

V2 removes relevant_entries as per René's suggestion, and adds an assertion as per Junio's question.

CC: Elijah Newren newren@gmail.com
cc: René Scharfe l.s.r@web.de

ahunt · 2021-04-10T07:41:54Z

/preview

gitgitgadget · 2021-04-10T07:43:06Z

Preview email sent as pull.930.git.1618040584669.gitgitgadget@gmail.com

ahunt · 2021-04-10T08:23:35Z

/preview

gitgitgadget · 2021-04-10T08:24:45Z

Preview email sent as pull.930.git.1618043083141.gitgitgadget@gmail.com

ahunt · 2021-04-10T08:28:54Z

/submit

gitgitgadget · 2021-04-10T08:31:00Z

Submitted as pull.930.git.1618043449249.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-930/ahunt/merge-ort-ubsan-v1

To fetch this version to local tag pr-930/ahunt/merge-ort-ubsan-v1:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-930/ahunt/merge-ort-ubsan-v1

gitgitgadget · 2021-04-10T11:56:25Z

On the Git mailing list, René Scharfe wrote (reply to this):

Am 10.04.21 um 10:30 schrieb Andrzej Hunt via GitGitGadget:
> From: Andrzej Hunt <ajrhunt@google.com>
>
> versions could be an empty string_list. In that case, versions->items is
> NULL, and we shouldn't be trying to perform pointer arithmetic with it (as
> that results in undefined behaviour).
>
> This issue has probably existed since:
>   ee4012dcf9 (merge-ort: step 2 of tree writing -- function to create tree object, 2020-12-13)
> But it only started occurring during tests since tests started using
> merge-ort:
>   f3b964a07e (Add testing with merge-ort merge strategy, 2021-03-20)
>
> For reference - here's the original UBSAN commit that implemented this
> check, it sounds like this behaviour isn't actually likely to cause any
> issues (but we might as well fix it regardless):
> https://reviews.llvm.org/D67122
>
> UBSAN output from t3404 or t5601:
>
> merge-ort.c:2669:43: runtime error: applying zero offset to null pointer
>     #0 0x78bb53 in write_tree merge-ort.c:2669:43
>     #1 0x7856c9 in process_entries merge-ort.c:3303:2
>     #2 0x782317 in merge_ort_nonrecursive_internal merge-ort.c:3744:2
>     #3 0x77feef in merge_incore_nonrecursive merge-ort.c:3853:2
>     #4 0x6f6a5c in do_recursive_merge sequencer.c:640:3
>     #5 0x6f6a5c in do_pick_commit sequencer.c:2221:9
>     #6 0x6ef055 in single_pick sequencer.c:4814:9
>     #7 0x6ef055 in sequencer_pick_revisions sequencer.c:4867:10
>     #8 0x4fb392 in run_sequencer revert.c:225:9
>     #9 0x4fa5b0 in cmd_revert revert.c:235:8
>     #10 0x42abd7 in run_builtin git.c:453:11
>     #11 0x429531 in handle_builtin git.c:704:3
>     #12 0x4282fb in run_argv git.c:771:4
>     #13 0x4282fb in cmd_main git.c:902:19
>     #14 0x524b63 in main common-main.c:52:11
>     #15 0x7fc2ca340349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>     #16 0x4072b9 in _start start.S:120
>
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior merge-ort.c:2669:43 in
>
> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
> ---
>     merge-ort: only do pointer arithmetic for non-empty lists
>
>     Here's a small and inconsequential UBSAN issue that started happening
>     when running tests on next since yesterday (since the merge of
>     en/ort-readiness).
>
>     It can be reproduced with something like this (t3404 also triggers the
>     same issue):
>
>     make SANITIZE=undefined COPTS="-Og -g" T="$(wildcard t5601-*.sh)"
>     GIT_TEST_OPTS="-v" UBSAN_OPTIONS=print_stacktrace=1 test
>
>     ATB, Andrzej
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-930%2Fahunt%2Fmerge-ort-ubsan-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-930/ahunt/merge-ort-ubsan-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/930
>
>  merge-ort.c | 6 ++++--
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 5e118a85ee04..4da4b4688336 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2504,8 +2504,10 @@ static void write_tree(struct object_id *result_oid,
>  	 * We won't use relevant_entries again and will let it just pop off the
>  	 * stack, so there won't be allocation worries or anything.
>  	 */
> -	relevant_entries.items = versions->items + offset;
> -	relevant_entries.nr = versions->nr - offset;
> +	if (versions->nr) {
> +		relevant_entries.items = versions->items + offset;
> +		relevant_entries.nr = versions->nr - offset;
> +	}
>  	QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);

Reading the diff I was wondering if QSORT now gets handed uninitialized
values if version-nr is 0.  The answer is no -- relevant_entries is
initialized at declaration.  Otherwise the compiler would have probably
warned, but sometimes it gets confused.

I wonder why relevant_entries is introduced at all, though.  It's not
referenced later.  So how about this instead?

	if (versions->nr)
		QSORT(versions->items + offset, nr, tree_entry_order);

The intent to sort the last versions->nr-offset entries of versions,
but only if it's not empty, is easier to see like this, I think.

>
>  	/* Pre-allocate some space in buf */
>
> base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4
>

gitgitgadget · 2021-04-10T11:56:27Z

User René Scharfe <l.s.r@web.de> has been added to the cc: list.

gitgitgadget · 2021-04-10T22:59:50Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

René Scharfe <l.s.r@web.de> writes:

>> diff --git a/merge-ort.c b/merge-ort.c
>> index 5e118a85ee04..4da4b4688336 100644
>> --- a/merge-ort.c
>> +++ b/merge-ort.c
>> @@ -2504,8 +2504,10 @@ static void write_tree(struct object_id *result_oid,
>>  	 * We won't use relevant_entries again and will let it just pop off the
>>  	 * stack, so there won't be allocation worries or anything.
>>  	 */
>> -	relevant_entries.items = versions->items + offset;
>> -	relevant_entries.nr = versions->nr - offset;
>> +	if (versions->nr) {
>> +		relevant_entries.items = versions->items + offset;
>> +		relevant_entries.nr = versions->nr - offset;
>> +	}
>>  	QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);
>
> Reading the diff I was wondering if QSORT now gets handed uninitialized
> values if version-nr is 0.  The answer is no -- relevant_entries is
> initialized at declaration.  Otherwise the compiler would have probably
> warned, but sometimes it gets confused.
>
> I wonder why relevant_entries is introduced at all, though.  It's not
> referenced later.  So how about this instead?
>
> 	if (versions->nr)
> 		QSORT(versions->items + offset, nr, tree_entry_order);
>
> The intent to sort the last versions->nr-offset entries of versions,
> but only if it's not empty, is easier to see like this, I think.

That does make sense.  I wonder if there needs some assertion to
ensure that "offset" does not exceed "versions.nr", though.

ahunt · 2021-04-11T09:10:02Z

/preview

gitgitgadget · 2021-04-11T09:11:10Z

Preview email sent as pull.930.v2.git.1618132269021.gitgitgadget@gmail.com

gitgitgadget · 2021-04-11T09:15:10Z

On the Git mailing list, Andrzej Hunt wrote (reply to this):


On 10/04/2021 13:48, René Scharfe wrote:
> Am 10.04.21 um 10:30 schrieb Andrzej Hunt via GitGitGadget:
>> [...]
>> diff --git a/merge-ort.c b/merge-ort.c
>> index 5e118a85ee04..4da4b4688336 100644
>> --- a/merge-ort.c
>> +++ b/merge-ort.c
>> @@ -2504,8 +2504,10 @@ static void write_tree(struct object_id *result_oid,
>>   	 * We won't use relevant_entries again and will let it just pop off the
>>   	 * stack, so there won't be allocation worries or anything.
>>   	 */
>> -	relevant_entries.items = versions->items + offset;
>> -	relevant_entries.nr = versions->nr - offset;
>> +	if (versions->nr) {
>> +		relevant_entries.items = versions->items + offset;
>> +		relevant_entries.nr = versions->nr - offset;
>> +	}
>>   	QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);
> 
> Reading the diff I was wondering if QSORT now gets handed uninitialized
> values if version-nr is 0.  The answer is no -- relevant_entries is
> initialized at declaration.  Otherwise the compiler would have probably
> warned, but sometimes it gets confused.
> 
> I wonder why relevant_entries is introduced at all, though.  It's not
> referenced later.  So how about this instead?
> 
> 	if (versions->nr)
> 		QSORT(versions->items + offset, nr, tree_entry_order);
> 
> The intent to sort the last versions->nr-offset entries of versions,
> but only if it's not empty, is easier to see like this, I think.
> 

That is much more elegant, I will follow this approach. Thank you for 
the suggestion!

An alternative might be to keep relevant_entries, and replace all later 
usages of versions->items[offset+i] with relevant_entries.items[i], but 
that's more invasive and I don't see any good reason for doing so given 
that the existing pattern works fine.

gitgitgadget · 2021-04-11T09:15:12Z

On the Git mailing list, Andrzej Hunt wrote (reply to this):


On 11/04/2021 00:56, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
> [...] >> Reading the diff I was wondering if QSORT now gets handed uninitialized
>> values if version-nr is 0.  The answer is no -- relevant_entries is
>> initialized at declaration.  Otherwise the compiler would have probably
>> warned, but sometimes it gets confused.
>>
>> I wonder why relevant_entries is introduced at all, though.  It's not
>> referenced later.  So how about this instead?
>>
>> 	if (versions->nr)
>> 		QSORT(versions->items + offset, nr, tree_entry_order);
>>
>> The intent to sort the last versions->nr-offset entries of versions,
>> but only if it's not empty, is easier to see like this, I think.
> 
> That does make sense.  I wonder if there needs some assertion to
> ensure that "offset" does not exceed "versions.nr", though.
> 

I think so - I will add the assertion. Following the original offset 
calculations is not trivial- which seems like a good enough reason for 
the assert.

ahunt · 2021-04-11T09:18:10Z

/preview

gitgitgadget · 2021-04-11T09:19:25Z

Preview email sent as pull.930.v2.git.1618132763405.gitgitgadget@gmail.com

versions could be an empty string_list. In that case, versions->items is NULL, and we shouldn't be trying to perform pointer arithmetic with it (as that results in undefined behaviour). Moreover we only use the results of this calculation once when calling QSORT. Therefore we choose to skip creating relevant_entries and call QSORT directly with our manipulated pointers (but only if there's data requiring sorting). This lets us avoid abusing the string_list API, and saves us from having to explain why this abuse is OK. Finally, an assertion is added to make sure that write_tree() is called with a valid offset. This issue has probably existed since: ee4012d (merge-ort: step 2 of tree writing -- function to create tree object, 2020-12-13) But it only started occurring during tests since tests started using merge-ort: f3b964a (Add testing with merge-ort merge strategy, 2021-03-20) For reference - here's the original UBSAN commit that implemented this check, it sounds like this behaviour isn't actually likely to cause any issues (but we might as well fix it regardless): https://reviews.llvm.org/D67122 UBSAN output from t3404 or t5601: merge-ort.c:2669:43: runtime error: applying zero offset to null pointer #0 0x78bb53 in write_tree merge-ort.c:2669:43 #1 0x7856c9 in process_entries merge-ort.c:3303:2 #2 0x782317 in merge_ort_nonrecursive_internal merge-ort.c:3744:2 #3 0x77feef in merge_incore_nonrecursive merge-ort.c:3853:2 #4 0x6f6a5c in do_recursive_merge sequencer.c:640:3 #5 0x6f6a5c in do_pick_commit sequencer.c:2221:9 #6 0x6ef055 in single_pick sequencer.c:4814:9 #7 0x6ef055 in sequencer_pick_revisions sequencer.c:4867:10 git#8 0x4fb392 in run_sequencer revert.c:225:9 git#9 0x4fa5b0 in cmd_revert revert.c:235:8 git#10 0x42abd7 in run_builtin git.c:453:11 git#11 0x429531 in handle_builtin git.c:704:3 git#12 0x4282fb in run_argv git.c:771:4 git#13 0x4282fb in cmd_main git.c:902:19 git#14 0x524b63 in main common-main.c:52:11 git#15 0x7fc2ca340349 in __libc_start_main (/lib64/libc.so.6+0x24349) git#16 0x4072b9 in _start start.S:120 SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior merge-ort.c:2669:43 in Signed-off-by: Andrzej Hunt <ajrhunt@google.com>

ahunt · 2021-04-11T11:03:56Z

/submit

gitgitgadget · 2021-04-11T11:05:19Z

Submitted as pull.930.v2.git.1618139107203.gitgitgadget@gmail.com

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git pr-930/ahunt/merge-ort-ubsan-v2

To fetch this version to local tag pr-930/ahunt/merge-ort-ubsan-v2:

git fetch --no-tags https://github.com/gitgitgadget/git tag pr-930/ahunt/merge-ort-ubsan-v2

gitgitgadget · 2021-04-12T15:54:06Z

On the Git mailing list, Elijah Newren wrote (reply to this):

On Sun, Apr 11, 2021 at 4:05 AM Andrzej Hunt via GitGitGadget
<gitgitgadget@gmail.com> wrote:
>
> From: Andrzej Hunt <ajrhunt@google.com>
>
> versions could be an empty string_list. In that case, versions->items is
> NULL, and we shouldn't be trying to perform pointer arithmetic with it (as
> that results in undefined behaviour).
>
> Moreover we only use the results of this calculation once when calling
> QSORT. Therefore we choose to skip creating relevant_entries and call
> QSORT directly with our manipulated pointers (but only if there's data
> requiring sorting). This lets us avoid abusing the string_list API,
> and saves us from having to explain why this abuse is OK.

Oh, nice, I like that.

>
> Finally, an assertion is added to make sure that write_tree() is called
> with a valid offset.
>
> This issue has probably existed since:
>   ee4012dcf9 (merge-ort: step 2 of tree writing -- function to create tree object, 2020-12-13)
> But it only started occurring during tests since tests started using
> merge-ort:
>   f3b964a07e (Add testing with merge-ort merge strategy, 2021-03-20)
>
> For reference - here's the original UBSAN commit that implemented this
> check, it sounds like this behaviour isn't actually likely to cause any
> issues (but we might as well fix it regardless):
> https://reviews.llvm.org/D67122
>
> UBSAN output from t3404 or t5601:
>
> merge-ort.c:2669:43: runtime error: applying zero offset to null pointer
>     #0 0x78bb53 in write_tree merge-ort.c:2669:43
>     #1 0x7856c9 in process_entries merge-ort.c:3303:2
>     #2 0x782317 in merge_ort_nonrecursive_internal merge-ort.c:3744:2
>     #3 0x77feef in merge_incore_nonrecursive merge-ort.c:3853:2
>     #4 0x6f6a5c in do_recursive_merge sequencer.c:640:3
>     #5 0x6f6a5c in do_pick_commit sequencer.c:2221:9
>     #6 0x6ef055 in single_pick sequencer.c:4814:9
>     #7 0x6ef055 in sequencer_pick_revisions sequencer.c:4867:10
>     #8 0x4fb392 in run_sequencer revert.c:225:9
>     #9 0x4fa5b0 in cmd_revert revert.c:235:8
>     #10 0x42abd7 in run_builtin git.c:453:11
>     #11 0x429531 in handle_builtin git.c:704:3
>     #12 0x4282fb in run_argv git.c:771:4
>     #13 0x4282fb in cmd_main git.c:902:19
>     #14 0x524b63 in main common-main.c:52:11
>     #15 0x7fc2ca340349 in __libc_start_main (/lib64/libc.so.6+0x24349)
>     #16 0x4072b9 in _start start.S:120
>
> SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior merge-ort.c:2669:43 in
>
> Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
> ---
>     merge-ort: only do pointer arithmetic for non-empty lists
>
>     V2 removes relevant_entries as per René's suggestion, and adds an
>     assertion as per Junio's question.
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-930%2Fahunt%2Fmerge-ort-ubsan-v2
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-930/ahunt/merge-ort-ubsan-v2
> Pull-Request: https://github.com/gitgitgadget/git/pull/930
>
> Range-diff vs v1:
>
>  1:  d52ce7110446 ! 1:  48c1a0ad6f46 merge-ort: only do pointer arithmetic for non-empty lists
>      @@ Commit message
>           NULL, and we shouldn't be trying to perform pointer arithmetic with it (as
>           that results in undefined behaviour).
>
>      +    Moreover we only use the results of this calculation once when calling
>      +    QSORT. Therefore we choose to skip creating relevant_entries and call
>      +    QSORT directly with our manipulated pointers (but only if there's data
>      +    requiring sorting). This lets us avoid abusing the string_list API,
>      +    and saves us from having to explain why this abuse is OK.
>      +
>      +    Finally, an assertion is added to make sure that write_tree() is called
>      +    with a valid offset.
>      +
>           This issue has probably existed since:
>             ee4012dcf9 (merge-ort: step 2 of tree writing -- function to create tree object, 2020-12-13)
>           But it only started occurring during tests since tests started using
>      @@ Commit message
>
>        ## merge-ort.c ##
>       @@ merge-ort.c: static void write_tree(struct object_id *result_oid,
>      -   * We won't use relevant_entries again and will let it just pop off the
>      -   * stack, so there won't be allocation worries or anything.
>      -   */
>      +  size_t maxlen = 0, extra;
>      +  unsigned int nr = versions->nr - offset;

Should we remove the initialization here and just define nr only, and then...

>      +  struct strbuf buf = STRBUF_INIT;
>      +- struct string_list relevant_entries = STRING_LIST_INIT_NODUP;
>      +  int i;
>      +
>      +- /*
>      +-  * We want to sort the last (versions->nr-offset) entries in versions.
>      +-  * Do so by abusing the string_list API a bit: make another string_list
>      +-  * that contains just those entries and then sort them.
>      +-  *
>      +-  * We won't use relevant_entries again and will let it just pop off the
>      +-  * stack, so there won't be allocation worries or anything.
>      +-  */
>       - relevant_entries.items = versions->items + offset;
>       - relevant_entries.nr = versions->nr - offset;
>      -+ if (versions->nr) {
>      -+         relevant_entries.items = versions->items + offset;
>      -+         relevant_entries.nr = versions->nr - offset;
>      -+ }
>      -  QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);
>      +- QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);
>      ++ assert(offset <= versions->nr);

Define nr here after this assert?  The definition of nr also depended
implicitly on this requirement, so if we're making this requirement
explicit, it would make sense to define nr after stating the
requirement.

>      ++ if (versions->nr)
>      ++         QSORT(versions->items + offset, nr, tree_entry_order);
>
>         /* Pre-allocate some space in buf */
>      +  extra = hash_size + 8; /* 8: 6 for mode, 1 for space, 1 for NUL char */
>
>
>  merge-ort.c | 15 +++------------
>  1 file changed, 3 insertions(+), 12 deletions(-)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 5e118a85ee04..702eb5bf362d 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -2493,20 +2493,11 @@ static void write_tree(struct object_id *result_oid,
>         size_t maxlen = 0, extra;
>         unsigned int nr = versions->nr - offset;
>         struct strbuf buf = STRBUF_INIT;
> -       struct string_list relevant_entries = STRING_LIST_INIT_NODUP;
>         int i;
>
> -       /*
> -        * We want to sort the last (versions->nr-offset) entries in versions.
> -        * Do so by abusing the string_list API a bit: make another string_list
> -        * that contains just those entries and then sort them.
> -        *
> -        * We won't use relevant_entries again and will let it just pop off the
> -        * stack, so there won't be allocation worries or anything.
> -        */
> -       relevant_entries.items = versions->items + offset;
> -       relevant_entries.nr = versions->nr - offset;
> -       QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);
> +       assert(offset <= versions->nr);
> +       if (versions->nr)
> +               QSORT(versions->items + offset, nr, tree_entry_order);
>
>         /* Pre-allocate some space in buf */
>         extra = hash_size + 8; /* 8: 6 for mode, 1 for space, 1 for NUL char */
>
> base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4
> --
> gitgitgadget

Otherwise, this patch looks good to me; thanks!

gitgitgadget · 2021-04-12T17:42:42Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes:

> Otherwise, this patch looks good to me; thanks!

Queued, manually amended, and here is what I have.

$ git range-diff @{1}...
1:  ca4cc8d182 ! 1:  c1ea48a8f7 merge-ort: only do pointer arithmetic for non-empty lists
    @@ Commit message
         SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior merge-ort.c:2669:43 in
     
         Signed-off-by: Andrzej Hunt <ajrhunt@google.com>
    +    Reviewed-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Junio C Hamano <gitster@pobox.com>
     
      ## merge-ort.c ##
     @@ merge-ort.c: static void write_tree(struct object_id *result_oid,
    + 		       size_t hash_size)
    + {
      	size_t maxlen = 0, extra;
    - 	unsigned int nr = versions->nr - offset;
    +-	unsigned int nr = versions->nr - offset;
    ++	unsigned int nr;
      	struct strbuf buf = STRBUF_INIT;
     -	struct string_list relevant_entries = STRING_LIST_INIT_NODUP;
      	int i;
    @@ merge-ort.c: static void write_tree(struct object_id *result_oid,
     -	relevant_entries.nr = versions->nr - offset;
     -	QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);
     +	assert(offset <= versions->nr);
    ++	nr = versions->nr - offset;
     +	if (versions->nr)
     +		QSORT(versions->items + offset, nr, tree_entry_order);

gitgitgadget · 2021-04-12T17:48:42Z

On the Git mailing list, Junio C Hamano wrote (reply to this):

Elijah Newren <newren@gmail.com> writes:

>>         /* Pre-allocate some space in buf */
>>         extra = hash_size + 8; /* 8: 6 for mode, 1 for space, 1 for NUL char */
>>
>> base-commit: 89b43f80a514aee58b662ad606e6352e03eaeee4
>> --
>> gitgitgadget
>
> Otherwise, this patch looks good to me; thanks!

By the way, I noticed the post-context comment and got curious.

	/* Pre-allocate some space in buf */
	extra = hash_size + 8; /* 8: 6 for mode, 1 for space, 1 for NUL char */
	for (i = 0; i < nr; i++) {
		maxlen += strlen(versions->items[offset+i].string) + extra;
	}
	strbuf_grow(&buf, maxlen);

Because "6 for mode" is wrong if it means "%06o", but the format
used in the code is "%o" so there is no correctness issues in the
code (Phew).  This "grow" is to avoid having to repeatedly
reallocate "nr" times in the loop that comes, but

 (1) s/6 for mode/up to 6 for mode/ to make it less misleading.

 (2) does this preallocation really help performance?

 (3) it is really disturbing to find a custom tree-writing code that
     is exercised only by ort here.  Tree-writing has been one major
     source of broken objects in various reimplementation of Git---
     all known ones made a mistake sometime in the past---and I'd
     strongly prefer to see a single helper that formats a tree
     object, given a set of <mode, object name, path> in the longer
     code health.

gitgitgadget · 2021-04-12T23:34:54Z

This branch is now known as ah/merge-ort-ubsan-fix.

gitgitgadget · 2021-04-12T23:34:55Z

This patch series was integrated into seen via git@8a64e23.

gitgitgadget · 2021-04-13T00:06:22Z

There was a status update in the "New Topics" section about the branch ah/merge-ort-ubsan-fix on the Git mailing list:

Code clean-up for merge-ort backend.

Will merge to 'next'?

gitgitgadget · 2021-04-14T01:14:56Z

This patch series was integrated into seen via git@8849ece.

gitgitgadget · 2021-04-14T01:14:56Z

This patch series was integrated into next via git@41713a3.

gitgitgadget · 2021-04-15T22:06:07Z

This patch series was integrated into seen via git@d53f509.

gitgitgadget · 2021-04-16T00:06:15Z

There was a status update in the "Cooking" section about the branch ah/merge-ort-ubsan-fix on the Git mailing list:

Code clean-up for merge-ort backend.

Will merge to 'master'.

gitgitgadget · 2021-04-16T22:26:52Z

This patch series was integrated into seen via git@257ae76.

gitgitgadget · 2021-04-16T22:26:53Z

This patch series was integrated into next via git@257ae76.

gitgitgadget · 2021-04-16T22:26:53Z

This patch series was integrated into master via git@257ae76.

gitgitgadget · 2021-04-16T22:26:56Z

Closed via 257ae76.

ahunt force-pushed the merge-ort-ubsan branch from d52ce71 to 8f0f7b4 Compare April 11, 2021 09:08

ahunt force-pushed the merge-ort-ubsan branch from 8f0f7b4 to 61329d8 Compare April 11, 2021 09:11

ahunt force-pushed the merge-ort-ubsan branch from 61329d8 to db803df Compare April 11, 2021 09:16

ahunt force-pushed the merge-ort-ubsan branch 2 times, most recently from cb3bb7e to e702e69 Compare April 11, 2021 10:07

ahunt force-pushed the merge-ort-ubsan branch from e702e69 to 48c1a0a Compare April 11, 2021 10:29

gitgitgadget bot added the seen label Apr 12, 2021

gitgitgadget bot added the next label Apr 14, 2021

gitgitgadget bot added the master label Apr 16, 2021

gitgitgadget bot closed this Apr 16, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

merge-ort: only do pointer arithmetic for non-empty lists #930

merge-ort: only do pointer arithmetic for non-empty lists #930

ahunt commented Apr 10, 2021 •

edited

ahunt commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

ahunt commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

ahunt commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

ahunt commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

ahunt commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

ahunt commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 13, 2021

gitgitgadget bot commented Apr 14, 2021

gitgitgadget bot commented Apr 14, 2021

gitgitgadget bot commented Apr 15, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

merge-ort: only do pointer arithmetic for non-empty lists #930

merge-ort: only do pointer arithmetic for non-empty lists #930

Conversation

ahunt commented Apr 10, 2021 • edited

ahunt commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

ahunt commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

ahunt commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

gitgitgadget bot commented Apr 10, 2021

ahunt commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

ahunt commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

ahunt commented Apr 11, 2021

gitgitgadget bot commented Apr 11, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 12, 2021

gitgitgadget bot commented Apr 13, 2021

gitgitgadget bot commented Apr 14, 2021

gitgitgadget bot commented Apr 14, 2021

gitgitgadget bot commented Apr 15, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

gitgitgadget bot commented Apr 16, 2021

ahunt commented Apr 10, 2021 •

edited