Re: [PATCH 3/4 v2] Allow detached form (e.g. "git log --grep foo") in log options.
- From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
- Date
- Jul 28, 2010, 12:56 UTC
- Message-ID
- <AANLkTi=TRzn-QWduEYH7qO-4=a-nGqCGSMkZCGn2iB=W@mail.gmail.com>
- In-Reply-To
- <vpq8w4vhmkj.fsf@bauges.imag.fr>
On Wed, Jul 28, 2010 at 11:29, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
Show 10 quoted lines
> Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes: > >> On Wed, Jul 28, 2010 at 09:41, Matthieu Moy <Matthieu.Moy@imag.fr> wrote: >> >> Here's a better commit message, the subject was >50 chars> (see >> Documentation/SubmittingPatches): > > I don't see a mention of 50 chars there. Conventions usually limit > lines to <80 chars (sometimes 72), and my subject line is 64 > chars-wide if you don't count [PATCH], which won't end up in git.git.
Hrm, I thought it was explicitly mentioned there, but it says:
- the first line of the commit message should be a short description and should skip the full stop
Since your patch subject had a full stop I thought I'd mention it.
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.
I'll submit a patch to SubmittingPatches citing the 50 char soft limit.
Show 11 quoted lines
>>> +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 >>> +' >> >> There's a lot of behavior change in this series, but only two small >> tests that I can see. > > 4, not 2.
I only saw two test_expect_success additions in your v2 series, maybe I missed one.
Show 7 quoted lines
>> It would be easy to change the parsing code back without triggering >> a regression test. > > I've introduced a test for each pattern, but deliberately did not do > exhaustive testing: I'm testing the patterns, not the way they are > applied. In the same way, I don't think there's a testcase for each > use of parse-option in the testsuite.
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.htmlAnyway. Regression tests can be added later, and I think this series is good enough for inclusion as-is. But option parsing has been a moving target in Git for a while now, and it's quite likely that this code will get rewritten down the line to use a new library that can parse all of Git's option syntax.
More tests coverage would be valuable for catching regressions in such future rewrites. In general that's the main value, not making sure *your* code works, but that someone else doesn't break it.
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"
}Then just change the t4*log*.sh tests that aren't doing setup work (like git init) to use it:
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index cb9f2bd..7ccc4bb 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -20 +20 @@ test_expect_success 'set up basic repos' '
-test_expect_success 'alias builtin format' '
+log_expect_success 'alias builtin format' 'We'll be running tests twice, but it'll catch these at least:
$ ack -h -i '(--[a-z-]+=)' --output '$1' t4*log*.sh | sort | uniq
--abbrev=
--author=
--color=
--decorate=
--diff-filter=
--format=
--grep=
--pretty=I can submit something like that later if I get around to it, or you can include it in your series if you want.