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

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 20, 2022, 17:37 UTC
Message-ID: <xmqqbktj3ct7.fsf@gitster.g>
In-Reply-To: <pull.1262.v8.git.1658334983053.gitgitgadget@gmail.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.

Show 9 quoted lines

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

Show 7 quoted lines

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

Show 6 quoted lines

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