Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Jul 28, 2010, 14:00 UTC
Message-ID: <vpq39v3670o.fsf@bauges.imag.fr>
In-Reply-To: <AANLkTi=TRzn-QWduEYH7qO-4=a-nGqCGSMkZCGn2iB=W@mail.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> DISCUSSION in git-commit(1) and gittutorial(7) mention 50 characters
> explicitly though, and a lot of tools that present commits in short
> form use that as a soft limit, e.g. gitweb and github.

Where does github have this limitation?

On http://github.com/git/git/commits/master, I can see messages like this:

git-read-tree.txt: acknowledge the directory matching bug in sparse checkout

76 chars-wide, untruncated.

> I'll submit a patch to SubmittingPatches citing the 50 char soft
> limit.

git$ git log --format='%s' | wc -l 22700 git$ git log --format='%s' | grep '..................................................' | wc -l 9392

Almost half of the commits already there do not comply with this supposed rule.

I'm fine with rewriting the subject, but the claim that commit messages should be <50 chars is just not reasonable and has never been applied to git.git.

> I only saw two test_expect_success additions in your v2 series, maybe
> I missed one.

I'm putting the diff for the t/ directory below to sum up the tests added.

> Not for everything it seems, but the coverage is pretty good:
>
>     http://v.nix.is/~avar/cover_db_html/parse-options-c.html
>     http://v.nix.is/~avar/cover_db_html/test-parse-options-c.html

You're precisely illustrating my point: these coverage results are about parse-option, not about the places where it is used. For example:

git/t$ git grep -e '--message' git/t$

=> git commit --message is not tested, at all (but git commit -m obviously is).

In my case, I didn't gcov it, but I think I've got 100% coverage on the helper functions (short_opt and diff_long_opt), just not 100% on the places where they're used.

Show 12 quoted lines

> Anyway, here's how you can easily get almost complete coverage at
> almost no cost for this series, first make a t/lib-log.sh like this:
>
>     #!/bin/sh
>
>     log_expect_success() {
>         desc=$1
>         text=$2
>         test_expect_success "Phony test with attached options: $1" "$2"
>         mocked=$(echo "$2" | sed -r 's/([A-Za-z-]+)=/\1 /')
>         test_expect_success "Phony test with detached options: $1" "$mocked"
>     }

You should check whether "$mocked" == "$2" to avoid useless re-run.

> I can submit something like that later if I get around to it,

Go ahead, I've exhausted my git time budget ;-).

diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index dae6358..8036e00 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -208,6 +208,7 @@ log -p --first-parent master
 log -m -p --first-parent master
 log -m -p master
 log -SF master
+log -S F master
 log -SF -p master
 log --decorate --all
 log --decorate=full --all
diff --git a/t/t4013/diff.log_-S_F_master b/t/t4013/diff.log_-S_F_master
new file mode 100644
index 0000000..978d2b4
--- /dev/null
+++ b/t/t4013/diff.log_-S_F_master
@@ -0,0 +1,7 @@
+$ git log -S F master
+commit 9a6d4949b6b76956d9d5e26f2791ec2ceff5fdc0
+Author: A U Thor <author@example.com>
+Date:   Mon Jun 26 00:02:00 2006 +0000
+
+    Third
+$
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 2230e60..f912589 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -101,11 +101,16 @@ test_expect_success 'oneline' '
 test_expect_success 'diff-filter=A' '
 
 	actual=$(git log --pretty="format:%s" --diff-filter=A HEAD) &&
+	actual_detached=$(git log --pretty="format:%s" --diff-filter A HEAD) &&
 	expect=$(echo fifth ; echo fourth ; echo third ; echo initial) &&
 	test "$actual" = "$expect" || {
 		echo Oops
 		echo "Actual: $actual"
 		false
+	} &&
+	test "$actual" = "$actual_detached" || {
+		echo Oops. Detached form broken
+		echo "Actual_detached: $actual_detached"
 	}
 
 '
@@ -203,6 +208,13 @@ test_expect_success 'log --grep' '
 	test_cmp expect actual
 '
 
+test_expect_success 'log --grep option parsing' '
+	echo second >expect &&
+	git log -1 --pretty="tformat:%s" --grep sec >actual &&
+	test_cmp expect actual &&
+	test_must_fail git log -1 --pretty="tformat:%s" --grep
+'
+
 test_expect_success 'log -i --grep' '
 	echo Second >expect &&
 	git log -1 --pretty="tformat:%s" -i --grep=sec >actual &&
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 58428d9..fb8291c 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -123,6 +123,12 @@ test_expect_success 'rev-list --glob=refs/heads/subspace/*' '
 
 '
 
+test_expect_success 'rev-list --glob refs/heads/subspace/*' '
+
+	compare rev-list "subspace/one subspace/two" "--glob refs/heads/subspace/*"
+
+'
+
 test_expect_success 'rev-list --glob=heads/subspace/*' '
 
 	compare rev-list "subspace/one subspace/two" "--glob=heads/subspace/*"

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/