Re: [PATCH 0/5] Sparse Index: Integrate with 'git add'

>
> Hi,
>
> I haven't read the series yet (I'm going to try to read them all
> today), but wanted to comment on a couple things in your cover
> letter...
>
> On Wed, Jul 21, 2021 at 2:07 PM Derrick Stolee via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
> >
> > This patch series re-submits the 'git add' integration with sparse-index.
> > The performance gains are the same as before.
> >
> > It is based on ds/commit-and-checkout-with-sparse-index.
> >
> > This series was delayed from its initial submission for a couple reasons.
> >
> > The first was because it was colliding with some changes in
> > mt/add-rm-in-sparse-checkout, so now we are far enough along that that
> > branch is in our history and we can work forwards.
> >
> > The other concern was about how 'git add ' should respond when a path
> > outside of the sparse-checkout cone exists. One recommendation (that I am
> > failing to find a link to the message, sorry) was to disallow adding files
> > that would become index entries with SKIP_WORKTREE on. However, as I worked
>
> I think the recommendation was:
>   * Permitted: running git add on tracked files with the SKIP_WORKTREE
> bit *clear*
>   * Disallowed: running git add on tracked files with the
> SKIP_WORKTREE bit *set*
>   * Disallowed: running git add on untracked files that would become
> index entries with the SKIP_WORKTREE bit set
>
> where the latter two exit with an error message that suggests changing
> the sparsity specification first.  I think this is what has existed
> for some time, other than Matheus adding some error messages to help
> the user when their add command doesn't match anything otherwise.
>
> > towards that goal I found that change would cause problems for a realistic
> > scenario: merge conflicts outside of the sparse-checkout cone.
>
> ...which wouldn't be a problem because these are tracked files whose
> SKIP_WORKTREE bit was cleared by the merge machinery (when it marked
> them as conflicted and wrote them to the working tree).
>
> > The first patch of this series adds tests that create merge conflicts
> > outside of the sparse cone and then presents different ways a user could
> > resolve the situation. We want all of them to be feasible, and this
> > includes:
> >
> >  1. Reverting the file to a known version in history.
> >  2. Adding the file with its contents on disk.
> >  3. Moving the file to a new location in the sparse directory.
> >
> > Without maintaining the behavior of adding files outside of the
> > sparse-checkout cone, we risk confusing users who get into this state. The
> > only workaround they would have is to modify their sparse-checkout
> > definition which might lead to expanding much more data than they need to
> > resolve the conflicts.
> >
> > For that reason, I stopped trying to limit 'git add' to be within the cone.
> > I'm open to suggestions here, but we need an approach that works for
> > out-of-cone conflicts.
>
> I believe my above suggestion works for out-of-cone conflicts.  Some
> important other details to keep in mind in regards to how we make add
> behave:
>
> * We don't want "git add -A [GLOB_OR_DIRECTORY]" to accidentally be
> treated as a directive to remove files from the repository (and
> naively noticing that SKIP_WORKTREE files are missing but attempting
> to operate on them anyway would give this problematic result).
> * We don't want "git rm [GLOB_OR_DIRECTORY]" to nuke SKIP_WORKTREE
> files; it should only operate on files that are present.
> * We need add and rm to be consistent with each other in terms of how
> SKIP_WORKTREE files and the sparsity cone are treated
>
> and more generally about not-SKIP_WORKTREE-despite-not-matching-sparsity-paths:
>
> * These files that aren't SKIP_WORKTREE but normally would be are
> prone to "disappear" at some random later time after they are made to
> match the index.  The disappearing can happen either with an explicit
> "git sparse-checkout reapply" (which is fine since it was explicit) or
> as a side-effect of various other commands that run through
> unpack_trees() since it attempts to heed the sparsity rules.  Users
> tend to get confused by the latter case; they'll understand at some
> point that it was because the file was outside the sparsity paths, but
> the randomness in when it's pulled out as a side-effect of other
> commands can be slightly jarring.  So, I'd like to avoid that where we
> can easily do so, which I think the recommendation above does.  (As a
> side note to these kinds of cases, maybe we want to consider having
> the sparsification logic in unpack_trees() first check whether paths
> being removed from the working tree and having their SKIP_WORKTREE bit
> set not only match the index but also match HEAD?  That'd be a bit
> more expensive to check in the sparsification paths, and I'm not sure
> they all have the relevant information, but it's an idea...)
>
> > The one place I did continue to update is 'git add --refresh ' to match the
> > behavior added by mt/add-rm-in-sparse-checkout which outputs an error
> > message. This happens even when the file exists in the working directory,
> > but that seems appropriate enough.