Re: [PATCH v3 0/5] Sparse index: fetch, pull, ls-files
- From
Elijah Newren <newren@gmail.com>
- Date
- Dec 10, 2021, 18:45 UTC
- Message-ID
- <CABPp-BHLRUQmjEoO_eEvZWTrVFK_xdMoQH5Q9DAj7y=JrKDzig@mail.gmail.com>
- In-Reply-To
- <211210.86v8zwbev9.gmgdl@evledraar.gmail.com>
On Fri, Dec 10, 2021 at 8:31 AM Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
Show 16 quoted lines
> > On Fri, Dec 10 2021, Derrick Stolee via GitGitGadget wrote: > > > Updates in v3 > > ============= > > > > * Fixed typo in commit message. > > * Added comments around doing strange things in an ls-files test. > > * Fixed adjacent typo in a test comment. > > Yay, I'm happy to see 5/5. Not because I didn't like the helper, but > that sparse is getting mature enough that we're getting ls-files to emit > information about it. Thanks. > > There's the small "diff -u" portability issue noted in my just-sent > <211210.86zgp8bi48.gmgdl@evledraar.gmail.com>.
Yeah, that one is an important point.
Show 9 quoted lines
> Other than that 2/5 adds this documentation about ls-files --sparse: > > If the index is sparse, show the sparse directories without expanding > to the contained files. > > Shouldn't we at least add: > > Sparse directories will be shown with a trailing slash, > e.g. "x/" for a sparse directory "x".q
Makes sense. Except I don't understand the trailing 'q' -- typo?
Show 10 quoted lines
> > In addition to that I think this may have a buggy/unexpected interaction > with the --eol option: > > 040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0 i/ w/ attr/ x/ > > I.e. should we be saying anything about the EOL state of these? OTOHO I > tried adding a submodule and it says the same, which seems similarly > odd, so maybe it's either correct, or this isn't updated for those > either.
If it matches what we do for submodules, for which eol values are also non-sensical, then I think we're good enough for this series. Perhaps we just shouldn't print anything eol related for directories with --eol, but that sounds like an orthogonal series rather than something that should go in this one.
Show 8 quoted lines
> Is the behavior of: > > $ git -C sparse-index ls-files --stage --sparse -- 'folder2/a' > $ echo $? > 0 > > Expected? I.e. accepting /a when we'd just print "folder2/" and not > e.g. erroring (probably, just asking)?
Fair question. I think it's fine; by way of comparison:
$ git rm --cached removed-and-no-longer-tracked-file $ git ls-files --stage -- non-existent-file removed-and-no-longer-tracked-file untracked-file $ echo $? 0
So it also shows nothing and displays nothing when asked for file(s) that are not in the index.
Yes, there is a slight semantic difference in that in your example we have a "folder2/" entry which *could be* expanded, but I am quite happy with the literal interpretation of the command that there is no "folder2/a" in the index. Said another way, I'm happy with ls-files showing what is in the index right now, rather than what could be in it, or listing things that HEAD contains that we don't for whatever reason.
Show 13 quoted lines
> How about: > > $ ls -l sparse-index/x > ls: cannot access 'sparse-index/x': No such file or directory > $ git -C sparse-index ls-files --stage 'x/*' > 100644 78981922613b2afb6025042ff6bd878ac1994e85 0 x/a > $ git -C sparse-index ls-files --stage --no-empty-directory 'x/*' > 100644 78981922613b2afb6025042ff6bd878ac1994e85 0 x/a > $ git -C sparse-index ls-files --stage --no-empty-directory --sparse 'x/*' > 040000 aaff74984cccd156a469afa7d9ab10e4777beb24 0 x/ > > The answer is probably "yes that's fine" because I've got no idea how > sparse really works, but just checking..
You should read the docs for this option you are trying: "Do not list empty directories. Has no effect without --directory." (Also, --directory only takes effect with --other, which you are also missing.)
So yeah, that flag is irrelevant. Perhaps ls-files should print a warning when flags are passed but ignored due to other flags not being passed, but that would belong in an orthogonal series rather than this one.
> So it's very nice to have the new diff test in 2/5, but would be much > nicer/assuring to have that split into a trivial function followed by > seeing how the diff looked in combination with each of the other option > that "ls-files" accepts.
There's no point testing in combination with flags that only affect untracked files. And I'm very dubious of adding testing for a case where we would need to add an explicit disclaimer that "We have no idea what the output should be but we are testing it anyway". So the options you suggest at least are things I'd rather not see us trying to add to the testing here.