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

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Mon, Mar 29 2021, Derrick Stolee wrote:

> On 3/28/2021 11:31 AM, Ævar Arnfjörð Bjarmason wrote:> It seems to me that the reason for that state is based on a
>> misunderstanding about what we would and wouldn't add to builtin/*.c,
>> i.e. that we wouldn't have something like a --debug option, but as
>> ls-files shows that's not a problem.

At the risk of going in circles here...

> I feel _strongly_ that a change to the user-facing CLI should come
> with a good reason and care about how it locks-in behavior for the
> future.

And I agree with you. Where we disagree is whether lives in builtin/*.c
== user-facing. I think --debug options are != that. It seems Junio
downthread agrees with that.

> Any adjustment to 'git ls-files' deserves its own series and
> attention[...]

A user-facing change to it yes, but I don't see how use of an (existing
even) --debug option would warrant any more attention than a new test
helper, less actually, it's less new code.

> [...] not in an already-too-large series like this one.

The alternative way of doing it at the end of
https://lore.kernel.org/git/874kgzq4qi.fsf@evledraar.gmail.com would
make this series smaller.

Anyway. As I noted in the E-Mail you're replying to
(https://lore.kernel.org/git/87eeg0ng78.fsf@evledraar.gmail.com/) I
really don't care that much.

I'm just still perplexed at how you keep bringing up use of an
internal-only --debug option as "user-facing", and here "already too
large" when we're talking about a proposed alternate direction that
would reduce the size.

> I'm not happy that this series and the next are so long, but that's
> the best I can do to make them reviewable and still capture a
> complete scenario. Hopefully the remaining series after these first
> two are smaller. Things like "what should 'git ls-files' do with a
> sparse index?" can fit cleanly on top once the core functionality
> of the internals are stable.

Sure. I'm fully on board with just moving forward with this in some
manner.

I'm not on board with the part of this that seems like it could just be
rephrased/understood as "...and we're not touching ls-files even with a
--debug option now because that would be user-facing[...]".

> I have an _opinion_ that the ls-files output is not well-suited to
> testing because the --debug output splits details across multiple
> lines. This is a minor point that could probably be corrected by
> a complicated script method, but that's why I list this as an
> opinion.

If the --debug it's spewing now isn't handy we can just change the
output format. The docs say:

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

And we don't have existing in-tree users, something like this would make
it rather trivial:
    
    diff --git a/builtin/ls-files.c b/builtin/ls-files.c
    index f6f9e483b27..7596edc9f9d 100644
    --- a/builtin/ls-files.c
    +++ b/builtin/ls-files.c
    @@ -113,11 +113,11 @@ static void print_debug(const struct cache_entry *ce)
     	if (debug_mode) {
     		const struct stat_data *sd = &ce->ce_stat_data;
     
    -		printf("  ctime: %u:%u\n", sd->sd_ctime.sec, sd->sd_ctime.nsec);
    -		printf("  mtime: %u:%u\n", sd->sd_mtime.sec, sd->sd_mtime.nsec);
    -		printf("  dev: %u\tino: %u\n", sd->sd_dev, sd->sd_ino);
    -		printf("  uid: %u\tgid: %u\n", sd->sd_uid, sd->sd_gid);
    -		printf("  size: %u\tflags: %x\n", sd->sd_size, ce->ce_flags);
    +		printf("  ctime: %u:%u%c", sd->sd_ctime.sec, sd->sd_ctime.nsec, line_terminator);
    +		printf("  mtime: %u:%u%c", sd->sd_mtime.sec, sd->sd_mtime.nsec, line_terminator);
    +		printf("  dev: %u\tino: %u%c", sd->sd_dev, sd->sd_ino, line_terminator);
    +		printf("  uid: %u\tgid: %u%c", sd->sd_uid, sd->sd_gid, line_terminator);
    +		printf("  size: %u\tflags: %x%c", sd->sd_size, ce->ce_flags, line_terminator);
     	}
     }

But even without that it wouldn't be some complicated post-processing,
just a pipe to a small perl or awk process.
     
>> I mean it's fine if it's just a "I don't think this is important and
>> don't want to spend time on it, but it seems like a good idea", in which
>> case others have the option of re-rolling some of these patches if they
>> care (at this point I wouldn't).
>> 
>> Or "this is just a bad idea for XYZ reason", which is also fine, and
>> even more valuable to document for future work in the area.
>> 
>> But to have another series built on this with refactorings back and
>> forth before code's landed on master just seems like needless churn.
>
> I think changing 'ls-files' before the sparse index has stabilized is
> premature. I said that a series like the RFC you sent would be
> appropriate after this concept is more stable. I do _not_ recommend
> trying to juggle it on top of the work while the patches are in flight.

Just to clarify, upthread in [1] you said:

    And I recommend that you continue to pursue [these RFC patches] as
    an independent series, but I'm not going to incorporate them into
    this one[...]

So do I understand it right that you're referring to phase IV in your
opinion being the first point where we'd consider piggy-backing on
anything in builtin (that "user-facing" dilemma again...).

But at that point wouldn't you have your own ideas about some
user-facing ls-files or other porcelain for this, so I'm not sure where
to place the encouragement that I continue to pursue that RFC series,
other than setting a reminder in my calendar for 6-12 months in the
future :)

1. https://lore.kernel.org/git/ca8a96a4-5897-2484-b195-57e5b3820576@gmail.com/