Re: [PATCH 7/9] update-index: add tests for sparse-checkout compatibility

[Elijah Newren <newren@gmail.com>]

On Mon, Jan 10, 2022 at 10:01 AM Victoria Dye <vdye@github.com> wrote:
>
> Elijah Newren wrote:
> > On Mon, Jan 10, 2022 at 7:47 AM Victoria Dye <vdye@github.com> wrote:
> >>
> >> Elijah Newren wrote:
> >>> On Tue, Jan 4, 2022 at 9:37 AM Victoria Dye via GitGitGadget
> >>> <gitgitgadget@gmail.com> wrote:
> >>>>
> >>>> From: Victoria Dye <vdye@github.com>
> >>>>
...
>
> Sorry about that - when I wrote the first version of this series in the
> `microsoft/git` fork, `git add` hadn't been updated to reject out-of-cone
> untracked files as it is in [1]. It's my mistake for not double-checking
> that it was still the case, apologies for wasting your time on re-explaining
> this.

No worries; there's lots of moving parts in the sparse world.

> In any case, I'll update the test comment and commit message per your
> suggestion:
>
> >>> I might buy that `git update-index` is lower level and should be
> >>> considered the same as `git add --sparse`, but the comment should
> >>> mention that and try to sell why update-index should be equivalent to
> >>> that instead of to `git add`.
>
> I'm leaning only slightly towards the current behavior (and will update the
> comment accordingly), but I'm happy to change it if the reasoning isn't as
> strong as that of another approach.
>
> [1] 105e8b014b (add: fail when adding an untracked sparse file, 2021-09-24)

I'm also leaning slightly towards keeping the current behavior and
just updating the comment.

...
> >> I understand why you find it buggy, but I am not making baseless assumptions
> >> about the correctness (or lack thereof) of the current behavior.
> >
> > To be clear, the fact that the behavior was there for a decade would
> > typically be basis enough for an assumption (in my opinion), and I
> > wouldn't have faulted folks for making it.  I might well have done so
> > myself.  My reasoning was just that I was getting confused by the
> > negations and trying to understand the testcase, and when I started to
> > unravel it, I found what looked like a possible inconsistency.
> >
> > Anyway, it's clear here you've actually dug a lot deeper and know the
> > history here.  In contrast, I was making assumptions about the history
> > (and ones that weren't correct, though I'd argue my assumptions
> > weren't baseless)...
> >
>
> Your assumptions were completely valid, I apologize if my response came off
> as implying otherwise. I'll try to use the comments on the tests to clarify
> their behavior as much as possible, hopefully reducing some confusion around
> all the multiple-negative flags & options.

Oh, no, not at all.  I was just worried that my earlier response might
have come across poorly and could be read as criticizing assumptions I
(incorrectly) _thought_ you were making.  I wasn't sure if that was
actually implied in your wording, but just to be careful I was just
trying to specify that I think the level of effort of your submissions
is totally appropriate -- even if you had been making the assumptions
I thought you had been.  And I was pointing out that I, after all, had
made and will continue to make assumptions too.  The point of review
is getting a second pair of eyes, because they can help catch issues;
and finding those issues often includes double-checking unspoken
assumptions.

Anyway, I think we're on the same page.  Thanks for your hard work
here; looking forward to seeing your reroll!