[20/27] merge-ort: ensure full index

Commit Message

Derrick Stolee March 16, 2021, 9:17 p.m. UTC

From: Derrick Stolee <[e-mail]>

Before iterating over all cache entries, ensure that a sparse index is
expanded to a full one to avoid unexpected behavior.

This is a top candidate for updating later with a proper integration
with the sparse index, since we will likely enable the ORT strategy by
default when the sparse index is enabled. This appears to be the only
place where the ORT strategy interacts with the index in such a global
way, so that integration should be clear once the ORT strategy and the
sparse index topics stabilize.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 merge-ort.c | 1 +
 1 file changed, 1 insertion(+)

Comments

Elijah Newren March 18, 2021, 5:31 a.m. UTC | #1

On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> From: Derrick Stolee <[e-mail]>
>
> Before iterating over all cache entries, ensure that a sparse index is
> expanded to a full one to avoid unexpected behavior.
>
> This is a top candidate for updating later with a proper integration
> with the sparse index, since we will likely enable the ORT strategy by
> default when the sparse index is enabled. This appears to be the only

s/appears to be/is/   :-)

> place where the ORT strategy interacts with the index in such a global
> way, so that integration should be clear once the ORT strategy and the
> sparse index topics stabilize.

Right, there is one more patch that will touch this function -- patch
7 from the series that marks merge-ort stable over here:
https://lore.kernel.org/git/[e-mail]/

While I have more optimizations for merge-ort, none of them will touch
this function.

> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  merge-ort.c | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/merge-ort.c b/merge-ort.c
> index 603d30c52170..9f737212555d 100644
> --- a/merge-ort.c
> +++ b/merge-ort.c
> @@ -3112,6 +3112,7 @@ static int record_conflicted_index_entries(struct merge_options *opt,
>         if (strmap_empty(conflicted))
>                 return 0;
>
> +       ensure_full_index(index);
>         original_cache_nr = index->cache_nr;
>
>         /* Put every entry from paths into plist, then sort */
> --
> gitgitgadget

Derrick Stolee March 23, 2021, 1:26 p.m. UTC | #2

On 3/18/2021 1:31 AM, Elijah Newren wrote:
> On Tue, Mar 16, 2021 at 2:17 PM Derrick Stolee via GitGitGadget
> <[e-mail]> wrote:
>>
>> From: Derrick Stolee <[e-mail]>
>>
>> Before iterating over all cache entries, ensure that a sparse index is
>> expanded to a full one to avoid unexpected behavior.
>>
>> This is a top candidate for updating later with a proper integration
>> with the sparse index, since we will likely enable the ORT strategy by
>> default when the sparse index is enabled. This appears to be the only
> 
> s/appears to be/is/   :-)
> 
>> place where the ORT strategy interacts with the index in such a global
>> way, so that integration should be clear once the ORT strategy and the
>> sparse index topics stabilize.
> 
> Right, there is one more patch that will touch this function -- patch
> 7 from the series that marks merge-ort stable over here:
> https://lore.kernel.org/git/[e-mail]/
> 
> While I have more optimizations for merge-ort, none of them will touch
> this function.

Yes, I noticed that that was one of the conflicts introduced with
this series.

I took a closer look at that patch and method, and it seems like
it only cares about paths with SKIP_WORKTREE _and_ is conflicted.
If it has a conflict, then I believe we won't collapse those
entries to a sparse-directory entry.

I think I will drop this patch for now, and rely on the fact that
I plan to enable merge-ORT directly if sparse-index is enabled.
At that time, I will carefully test this logic.

For now, the 'command_requires_full_index' guard will be
sufficient to preserve behavior.

Thanks,
-Stolee

Patch

diff --git a/merge-ort.c b/merge-ort.c
index 603d30c52170..9f737212555d 100644
--- a/merge-ort.c
+++ b/merge-ort.c
@@ -3112,6 +3112,7 @@  static int record_conflicted_index_entries(struct merge_options *opt,
 	if (strmap_empty(conflicted))
 		return 0;
 
+	ensure_full_index(index);
 	original_cache_nr = index->cache_nr;
 
 	/* Put every entry from paths into plist, then sort */