On Wed, Jul 28, 2010 at 11:29, Matthieu Moy
<Matthieu.Moy@xxxxxxxxxxxxxxx> wrote:
> Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
>
>> On Wed, Jul 28, 2010 at 09:41, Matthieu Moy <Matthieu.Moy@xxxxxxx> 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.
>>> +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.
>> 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.html
Anyway. 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.
--
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