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

From: Elijah Newren <newren@gmail.com>
Date: Jan 10, 2022, 20:03 UTC
Message-ID: <CABPp-BEgOi-UqH9bP0_OU6G7wCaE7g1GJN-RU5oaWPvwUW+imA@mail.gmail.com>
In-Reply-To: <bec659b2-8d69-c69c-73e5-a56ae415874b@github.com>

On Mon, Jan 10, 2022 at 10:01 AM Victoria Dye <vdye@github.com> wrote:

Show 10 quoted lines

>
> 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>
> >>>>

...

Show 6 quoted lines

>
> 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.

Show 13 quoted lines

> 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.

...

Show 20 quoted lines

> >> 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!