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

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

I presume that it is "from root first while you have a checkout of version2, so .gcov files for version2 exist in the working tree"?

As you know you are reading from diff output, you do not have to be so strict in this if condition. It's not like you try to carefully reject a line that began with "@@" but did not match this pattern in the corresponding else block.

"read line" does funny things to backslashes and whitespaces in the payload ("read -r line" sometimes helps), and echoing $line without quoting will destroy its contents here and in the line below (but you do not care because all you care about is if it begins with a dash).

If a patch removes a line with a solitary 'n' on it, possibly followed by a SP and something else, such a line would say "-n something else", and some implementation of "echo -n something else" would do what this line does not expect. A safer way to do this, as you only care if it is a deletion line, is to do

	case "$line" in -*) ;; *) line_num=$(( $line_num + 1 ));; esac

Also you can make the echoing of "$line_num:$line" above conditional, which will allow you to lose "grep ':+'" in the pipeline that consumes output from this thing, e.g.

	case "$line" in +*) echo "$line_num:$line";; esac

iff you must write this in shell (but see below).

I have a feeling that a single Perl script instead of a shell loop that runs many grep and awk as subprocesses performs better even on Windows, and it would be more readable and maintainable.

perl -e '
	my $line_num;
	while (<>) {
		# Hunk header?  Grab the beginning in postimage.
		if (/^@@ -\d+(?:,\d+)? \+(\d+)(?:,\d+)? @@/) {
			$line_num = $1;
			next;
		}

		# Have we seen a hunk?  Ignore "diff --git" etc.
		next unless defined $line_num;

		# Deleted line? Ignore.
		if (/^-/) {
			next;
		}

		# Show only the line number of added lines.
		if (/^\+/) {
			print "$line_num\n";
		}
		# Either common context or added line appear in
		# the postimage.  Count it.
		$line_num++;
	}
'

or something like that, given that you seem to only need line numbers in new_lines.txt anyway?

Do we need these other commands in the pipe? How is this different from, say,

	git diff --name-only "$V1" "$V2" -- \*.c

?

I think you meant to match "^<linenum>:+<added contents>" you are echoing out in the above helper function, but this will find a removed line that happens to have colon followed by a plus (which is not all that uncommon in a patch to a shell script).

Turning "<linenum>:+<added contents>" to "<linenum> +<added contents>" with this, so that the next "awk" can show <linenum> only? Then you do not need 'g' at the end. I see you use 'g' in many sed scripts for uncovered_lines.txt, and I suspect most of 'g's you do not mean.

Then we get a list of $line_num here?

This is sorted textually, which goes against intuition because these lines are all line numbers, but it is done so that you can run "comm" on it later and "comm" requires us to feed lines sorted textually.

If I were writing this part, I'd probably write a small Perl script that takes output from 'git diff "$V1" "$V2" -- "$file"' and then reduces it to a list of line numbers for newly introduced lines. Then pipe it to "sort >new_lines.txt" to match what we see below.

Do not cat a single regular file into a pipe. Write pipe at the end of the line, not the beginning of next line. Do not grep into sed. Do not feed sed into sed unless needed.

Without knowing what the above is trying to do, but just mechanically translating, I'd get something like this:

	sed -ne '/#####/{
		s/    #####: //g
		s/:.*//
                p
	}' "$hash_file.gcov" |
	sort >uncovered_lines.txt

;-)  This creates something like

	1)
	2)
	3)

out of a list of numbers.  Cute.