On Wed, Jul 20 2022, Junio C Hamano wrote:
> "ZheNing Hu via GitGitGadget" <gitgitgadget@xxxxxxxxx> 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...
