[also forwarding this to the list since it's relevant to the ongoing discussion, and I hadn't noticed when replying that Pranit had (presumably) accidentally dropped the git list as a recipient] On Sun, Mar 27, 2016 at 3:10 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: > On Sun, Mar 27, 2016 at 1:42 AM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: >> On Sun, Mar 27, 2016 at 8:37 AM, Eric Sunshine <sunshine@xxxxxxxxxxxxxx> wrote: >>> On Sat, Mar 26, 2016 at 3:48 PM, Pranit Bauva <pranit.bauva@xxxxxxxxx> wrote: >>> It is important to explain *why* you want to be able to count how many >>> diffs were in the editor message. In particular, you will be adding >>> new tests in a subsequent patch which will expect a specific number of >>> diffs (rather than any number of diffs) >>> >>> Also, you need to explain why you're changing the existing tests to >>> count the number of diffs. Is it simply "because you can" or is it >>> because you suspect that a change you're making in a subsequent patch >>> might accidentally cause too many diffs to show up in the existing >>> test cases? >> >> Sorry for not writing commit messages properly. It is a part I am working on. >> How does this paragraph sound to you in addition to the previous commit message? >> "A subsequent commit will introduce a scenario where it is important >> to be able to exactly determine how many diffs were present." > > That's fine for explaining why 'check-for-diff' is being updated to > store the output, but you still need to explain why you're changing > the existing tests. > >>>> -cat >check-for-diff <<EOF >>>> -#!$SHELL_PATH >>>> -exec grep '^diff --git' "\$1" >>>> +write_script "check-for-diff" <<'EOF' && >>>> +! test -s out || >>>> +rm out && >>> >>> Why not just "rm -f out"? But, more importantly, why do you need to >>> remove the file at all? The '>' redirection operator (used below) will >>> overwrite the file, so no need to remove it beforehand. >> >> I wasn't aware about '-f' option. It is important to remove the file. >> I initially tried without removing the file and it caused problems. >> This is because let's say the previous test had 1 diff which was >> stored in out. Now, if in the next test, it is expected to have 0 >> diffs then grep would fail to execute and nothing will be re-written >> to out and out will contain the 1 diff from previous test. An >> explanation for this should be mentioned in the commit message? > > No. Rather than trying to explain all this in the commit message, it > would be better to retain a simple implementation of 'check-for-diff' > rather than adding several levels of complication. More about this > below... > >>>> +! grep '^diff --git' "$1" || >>>> +grep '^diff --git' "$1" >out >>> >>> Um, what? Why two greps? I would have expected you to simply re-use >>> the existing grep (minus the backslash) while adding the redirection: >>> >>> -exec grep '^diff --git' "\$1" >>> +exec grep '^diff --git' "$1" >out >> >> The reason of two greps in that if in some test it fails to find any >> diffs, the editor will return an error. Of course we could test this >> scenario by using test_must_fail as in previous patches, but I think >> it would be more clearer if we could test for the existence of out >> which is done in patch 3/3. I will explanation for this in commit >> message. > > This sounds unnecessarily complicated. It's not too difficult to > reason about a script named 'check-for-diff' actually "checking for > presence of diffs" and failing if no diff is present, from which it > follows naturally that new tests which don't expect any diffs would > use test_must_fail. Keeping it simple like this also makes this patch > much less noisy since it doesn't require changing existing tests. > > Likewise, it keeps most of the new tests in 3/3 small because the bulk > of them only want to know that a diff was present, but don't care > about the number of diffs. However, it's not necessarily a bad thing > to ensure that you got the number of diffs you expected (for instance, > that a single -v really behaved as -v, and not as -v -v), so that can > be used as an argument for overhauling the old tests, as well as using > counting in all new tests. > > However, even if you take the approach of making 'check-for-diff' > succeed unconditionally and always count diffs, the current > implementation is still overly complicated. It would be much simpler > to let 'check-for-diff' always create the output file, and then use > "test_line_count = 0 out" when expecting no diffs than to sometimes > create the output file and sometimes not. The shell '>' operator will > truncate the file to zero size even before grep is invoked, so you > don't need to worry that results from an earlier test will pollute > 'out' for a subsequent test, even if grep finds no matches. Thus, > 'check-for-diff' collapses to this tiny implementation: > > grep '^diff --git' "$1" >out > exit 0 > > Or, if you want to be terse: > > grep '^diff --git' "$1" || exit 0 >out > > A couple notes: First, "out" isn't a great name for this file; perhaps > come up with a better name. Second, under this scenario, > "check-for-diff" isn't the most accurate name; it's now simply > collecting diffs rather than checking for presence. > > An alternative would be for the output file to contain the actual > count of diffs, rather than the diff lines themselves. For instance: > > write_script count-diffs <<\EOF && > grep -c '^diff --git' "$1" >out > exit 0 > EOF > > Whether that's actually better is subjective. > > So, what's my final word? I quite like the idea of keeping everything > as simple as possible, thus not altering existing tests and only doing > line count in the two or three new tests which actually care about the > number of diffs. However, I can also be sold on retrofitting existing > tests and having new tests do line counting as a way to improve test > coverage by catching cases where "-v" incorrectly behaves as "-v -v". > If you go that route, then the commit message of this patch needs to > sell it as such (an improvement in test coverage). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html