Re: [PATCH v3 1/1] contrib: add coverage-diff script

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:

You'd want to quote that *.c from the shell, i.e. either one of these

	files=$(git diff --name-only $V1 $V2 -- \*.c)
	files=$(git diff --name-only $V1 $V2 -- '*.c')

otherwise you'd lose things like "builtin/am.c", I'd think.

I know this is only for running in _our_ source tree, and we do not have a source with $IFS in it, so I'd declare that this is OK. It would be good to document that assumption in red capital letters at the beginning of this loop, though ;-)

	# Note: this script is only for our codebase and we rely on
	# the fact that the pathnames of our source files do not
	# have any funny characters---letting the shell split $files
	# list at $IFS boundary is very much intentional, and not
	# quoting "$file" in the code below also is.  Don't imitate
	# this in scripts that are meant to handle random end-user
	# repositories!
	for file in $files
	do
		...

I do not see a strong reason why we would want to limit $V1 and $V2 to branch names and raw commit object names, and quoting them in dq pair is a cheap fix to allow things like

	$ contrib/coverage-diff.sh master 'pu^{/^### match next}'

so let's do so.

Could you cut lines _after_ typing a pipe and omit backslashes, i.e.

	git diff "$V1" "$V2" -- $file |
	diff_lines |
	sort >new_lines.txt

It seems to be personal taste whether to indent the second and subsequent lines; I do not care if you indent or if you align too much either way (but I have moderate perference to align).

But I do not want to see people type unnecessary backslashes. This is not limited to just this pipeline but elsewhere in the script.

Do you need two sed invocations for this, or would

	sed -e 's/$/\)/' -e '/^/	/'

work as well? By the way """The meaning of an unescaped <backslash> immediately followed by any character other than '&', <backslash>, a digit, <newline>, or the delimiter character used for this command, is unspecified."""[*1*] so '\t' on the replacement side is a no-no in the portability department.

	Reference. *1*
	http://pubs.opengroup.org/onlinepubs/9699919799/utilities/sed.html

Lose the backslash at the end. The shell knows that you haven't finished your sentence when it sees a line that ends with &&, || or a pipe |, so there is no need to tell it redundantly that you have more things to say with the backslash.

Near the begininng (like just before the "for file in $files" loop), you can probably have a trap to make sure these are removed upon exit, e.g.

    trap 'rm -f new_lines.txt uncovered_lines.txt uncovered_new_lines.txt' 0