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, 15:03 UTC
- Message-ID
- <AANLkTikdy9W=mQ0RqGFRPXktTeqvFNrhipqgbVEbJzBC@mail.gmail.com>
- In-Reply-To
- <vpq39v3670o.fsf@bauges.imag.fr>
On Wed, Jul 28, 2010 at 14:00, Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> wrote:
Show 14 quoted lines
> Ę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.
A lot of applications do it in their public timeline, e.g. at GitHub:
http://github.com/mirrors
Although their limit seems to be closer to 65 characters now, but I'm fairly sure that it was 50 before. It's also overflowing my screen now as a result.
Show 10 quoted lines
>> 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.
It's not a rule, just a soft limit. Anyway, I just mentioned it because the subject ended with a full stop. Which indicated that you hadn't spotted that bit in SubmittingPatches, and might want to know for future submissions.
There's also a quickly decreasing long tail after 50 characters.
$ for i in {50..70}; do git log --format='%s' | egrep ".{$i}" | wc
-l | tr '\n' ' '; done && echo
9391 8847 8341 7838 7338 6840 6379 5915 5456 5018 4585 4184 3779
3418 3045 2751 2434 2153 1871 1655 1438(Use Extended Regular Expressions, your "." key will thank you >:)
Show 9 quoted lines
> 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.
Thanks. I missed 2/4 because I only grep-ed for the test harness functions.
Show 11 quoted lines
>> 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$
*added to my list of stuff to test*
Show 6 quoted lines
> => 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.
I just mentioned the coverage as an aside (and probably shouldn't have. What I mainly meant is that it would be easy to regress on option parsing in revision.c in the future because e.g. nothing tests both "--abbrev foo' and "--abbrev=foo".
Show 14 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.Yeah, it's just pseudocode. Assigning to desc and text and not using them is also pretty useless. I didn't even run it, but I'm delighted to find that it at least compiles and executes.
>> I can submit something like that later if I get around to it, > > Go ahead, I've exhausted my git time budget ;-).
Maybe I will. Anyway, like I said this patch looks just fine. I mainly just wanted to note that it was under-tested (but that it should go in anyway, IMO), if for no other reason than as a future note to myself. Or anyone else wanting to improve our test coverage.