Re: [PATCH v4 07/20] test-read-cache: print cache entries with --table

On Wed, Mar 24 2021, Derrick Stolee wrote:

So a dump of ce->ce_stat_data, i.e. the same thing ls-files --debug prints out now, or...?

This series is 20 patches. Your current derrickstolee/sparse-index/wip is another 36, and from skimming those patches & your design doc those 56 seem to be partway into Phase I of IV.

So at the rate things tend to get reviewed / re-rolled & land in git.git it seems exceedingly likely that we'll have some part-way implementation of this for at least a major release or two. No?

Which is why I'm suggesting/asking if we shouldn't have something like this debugging helper as part of installed tooling, because people are going to try it, it's probably going to have bugs and do other weird things, and I'd rather not have to manually build some test-tool to debug some local sparse checkout somewhere.

That's just not the case, we have plenty of unstable debug-esque options in various built-in commands, in fact ls-files already has a --debug option whose docs say:

    This is intended to show as much information as possible for manual
    inspection; the exact format may change at any time.

It was added in 84974217151 (ls-files: learn a debugging dump format, 2010-07-31) and "just tacks all available data from the cache onto each file's line" so in a way not adjusting it and using it would be a regression, after all this is new data in the cache, so it should print it :)

There's also PARSE_OPT_HIDDEN for other such in-tree use. Whatever the sanity/merits of me suggesting that this specific thing be in ls-files instead of a test-helper, it seems far fetched that something like that hidden behind a GIT_TEST_* env var (or hidden option, --debug etc.) is something we'd need to worry about backwards compatibility for.

So, whatever you think about the merits of including this functionality in ls-files I think your stance of this being a no-go for adding to the builtin is based on a false premise. It's fine to have unstable/transitory/debug output in the builtins. We just name & document them as such.

I also had some feedback in that series and on the earlier iteration that I think is appropriate to be incorporated into a re-roll of this one, which doesn't have anything to do with the question of whether we use ls-files or the helper in the tests. Such as us showing more stuff into the read-cache.c test-tool v.s. splitting it up making that code needlessly convoluted.

I don't see how recommending that I pursue that as an independent series is productive for anyone. So as you re-roll this I should submit another series on top to refactor your in-flight code & tests?

Either my suggestions are just bad, and we shouldn't do them at all, or it makes sense to incorporate relevant feedback in re-rolls. I'll let other reviewers draw their own conclusions on that.

That's not a snarky "I'm right" b.t.w., I may honestly be full of it on this particular topic.

But if those suggested changes are worth doing at all, then doing them in that way seems like a massive waste of time for everyone involved, or maybe I'm not getting what you're suggesting by pursuing them as an independent series.

At which point you'd be adjusting your tests that expect ls-tree format output to using ls-files output, instead of using ls-files-like output from the beginning?

At the end of this E-Mail is a patch on top that adds an undocumented
--debug-sparse in addition to the existing --debug. Running that in the
middle of one of your tests:
    
    $ ~/g/git/git ls-files --debug -- a folder1
    a
      ctime: 1616641434:474004002
      mtime: 1616641434:474004002
      dev: 2306     ino: 28576528
      uid: 1001     gid: 1001
      size: 8       flags: 0
    folder1/a
      ctime: 0:0
      mtime: 0:0
      dev: 0        ino: 0
      uid: 0        gid: 0
      size: 0       flags: 40000000
    $ ~/g/git/git ls-files --debug --debug-sparse -- a folder1
    a 
      ctime: 1616641434:474004002
      mtime: 1616641434:474004002
      dev: 2306     ino: 28576528
      uid: 1001     gid: 1001
      size: 8       flags: 0
    folder1/
      ctime: 0:0
      mtime: 0:0
      dev: 0        ino: 0
      uid: 0        gid: 0
      size: 0       flags: 40004000
    $ ~/g/git/git ls-files --stage -- a folder1
    100644 e79c5e8f964493290a409888d5413a737e8e5dd5 0       a
    100644 e79c5e8f964493290a409888d5413a737e8e5dd5 0       folder1/a
    $ ~/g/git/git ls-files --stage --debug-sparse -- a folder1
    100644 e79c5e8f964493290a409888d5413a737e8e5dd5 0       a
    040000 f203181537ff55dcf7896bf8c5b5c35af1514421 0       folder1/

I.e. it gives you everything your helper does and more with a trivial addition of a --debug-sparse (which we can later just remove, it's a debug option...).

See e.g. my recent 15c9649730d (grep/log: remove hidden --debug and --grep-debug options, 2021-01-26) which is already in a release, and AFAICT nobody has noticed or cared.

I don't know if that's the stat() information you wanted (your WIP branch doesn't have such a change), but presumably it either is the info you want, or ls-files's --debug would want to emit any such such info that's now missing too.