Re: [PATCH v8] ls-files: introduce "--format" option

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

On Wed, Jul 20 2022, Junio C Hamano wrote:

> "ZheNing Hu via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> It's been quite many iterations, so I'll just comment on the range-diff.
>
>>      -+			usage_msg_opt("--format cannot used with -s, -o, -k, -t"
>>      ++			usage_msg_opt("--format cannot used with -s, -o, -k, -t, "
>>       +				      "--resolve-undo, --deduplicate, --eol",
>>       +				      ls_files_usage, builtin_ls_files_options);
>
> Looks good.

Although a nit I didn't spot before: missing _() & this should be marked
for translation, surely...

>>       +	git commit -m base
>>       +'
>>       +
>>       +test_expect_success 'git ls-files --format objectmode v.s. -s' '
>>      -+	git ls-files -s | awk "{print \$1}" >expect &&
>>      ++	git ls-files -s >files &&
>>      ++	cut -d" " -f1 files >expect &&
>
> Either "awk" or "cut" is fine and flipping between them is a bit
> distracting.  Cutting the pipe into two is a good move.

That "cut" suggestion saw mine, sorry about the churn...

> But is this testing the right thing?

On this...

>> +test_expect_success 'git ls-files --format objectmode v.s. -s' '
>> +	git ls-files -s >files &&
>> +	cut -d" " -f1 files >expect &&
>> +	git ls-files --format="%(objectmode)" >actual &&
>> +	test_cmp expect actual
>> +'
>
> It only looks at the first column of the "-s" output, and we are
> implicitly assuming that the order of output does not change between
> the "-s" output and "--format=<format>" output.  I wonder if it is
> more useful and less error prone to come up with a format string
> that 100% reproduces the "ls-files -s" output and compare the two,
> e.g. 
>
> 	format="%(objectmode) %(objectname) %(stage)	%(path)" &&
> 	git ls-files -s >expect &&
> 	git ls-files --format="$format" >actual &&
> 	test_cmp expect actual
>
> I do not know if the $format I wrote without looking at the doc is
> correct, but you get the idea.

Past rounds moved some tests towards that, maybe that's a good thing
here too I didn't look deeply this time around...