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

[Junio C Hamano <gitster@pobox.com>]

"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.

>      @@ t/t3013-ls-files-format.sh (new)
>       +	printf "LINEONE\nLINETWO\nLINETHREE\n" >o1.txt &&
>       +	printf "LINEONE\r\nLINETWO\r\nLINETHREE\r\n" >o2.txt &&
>       +	printf "LINEONE\r\nLINETWO\nLINETHREE\n" >o3.txt &&
>      -+	ln -s o3.txt o4.txt &&
>      -+	git add "*.txt" &&
>      -+	git add --chmod +x o1.txt &&
>      -+	git update-index --add --cacheinfo 160000 $(git hash-object o1.txt) o5.txt &&
>      ++	git add . &&

We may want to be a bit more strict (like "o?.txt") but because we
know this is the first 'setup' step, let's let it pass.

>      ++	oid=$(git hash-object o1.txt) &&
>      ++	git update-index --add --cacheinfo 120000 $oid o4.txt &&
>      ++	git update-index --add --cacheinfo 160000 $oid o5.txt &&
>      ++	git update-index --add --cacheinfo 100755 $oid o6.txt &&

It is a bit inconvenient that --cacheinfo takes only fully-spelled
raw object name that we need to use $oid like this (otherwise we
would be able to write ":o1.txt" instead), but (1) it is not a fault
of this patch, and (2) update-index is a plumbing command meant for
scripts, so it is not too big a problem.

>       +	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.

But is this testing the right thing?

> +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.

Thanks.