Re: [PATCH 0/7] Sparse index: integrate with 'git stash'

From: Derrick Stolee <derrickstolee@github.com>
Date: Apr 26, 2022, 13:09 UTC
Message-ID: <b7752094-1f70-20a4-3217-502803817264@github.com>
In-Reply-To: <9ff9f199-baec-80f5-41c2-17034744e527@github.com>

On 4/26/2022 8:49 AM, Derrick Stolee wrote:

Show 7 quoted lines

> On 4/25/2022 1:49 PM, Victoria Dye via GitGitGadget wrote:
>> This series, in combination with the sparse index integrations of reset [1],
>> update-index [2], checkout-index [2], clean [2], and read-tree [3], allows
>> most subcommands of 'git stash' to use the sparse index end-to-end without
>> index expansion.
> 
> I'm really excited to see this culmination of your work in this area!

After taking a careful read of this series, I'm really pleased with how simple the actual code changes are at this point. This is a testament to the hard work you've done on the child processes and also some luck in how we've added protections to different portions of the lower-level APIs.

The biggest interesting points from my review and Junio's comments are:

1. The merge_recursive_generic() change could maybe be done by modifying
   the options struct instead of changing the prototype.

2. Using merge ORT for applying stashes could maybe be changed by config,
   allowing users to select recursive if they want. This change could
   also use some testing to show the merits of ORT even without a sparse
   index.

And then I had some nitpicks:

* A script variable could use quoting, probably.

* You moved the checkout-index tests to be next to the stash tests, but
  I don't see the value of that. (Perhaps I just need to be enlightened.)

Thanks! -Stolee