From: Jeff King <peff@peff.net>
To: Jonathan Nieder <jrnieder@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
Michael Grubb <devel@dailyvoid.com>,
git@vger.kernel.org
Subject: Re: [PATCH 1/2] tests: eliminate unnecessary setup test assertions
Date: Fri, 6 May 2011 17:48:01 -0400 [thread overview]
Message-ID: <20110506214801.GA17848@sigill.intra.peff.net> (raw)
In-Reply-To: <20110506205851.GB20182@elie>
On Fri, May 06, 2011 at 03:58:52PM -0500, Jonathan Nieder wrote:
> Two scripts use a different style with this kind of trivial code
> enclosed by a test assertion; fix them. The usual style is easier to
> read since there is less indentation to keep track of and no need to
> worry about nested quotes; and on the other hand, because the commands
> in question are trivial, it should not make the test suite any worse
> at catching future bugs in git.
Thanks. Glancing at the first few hunks, this change didn't seem like a
big cleanup, but then I got to the hunk with all the ugly '\'' bits. :)
The patch looks correct to me (reviewing with "git diff -b" was a big
help).
> While at it, make some other small tweaks:
>
> - spell function definitions with a space before () for consistency
> with other scripts;
Hmm.
$ cd t
$ git grep ' ()' *.sh | wc -l
271
$ git grep '[^ ]()' *.sh | wc -l
247
I'm not sure you are making things any more consistent. But I don't
really care either way.
Just for giggles, I was curious who introduced the styles:
pattern_authors() {
git grep -n "$@" |
while IFS=: read file line match; do
git blame -L "$line,$line" "$file"
done |
perl -lpe '/\((.*?) \d+-\d+-\d+/; $_=$1'
}
$ pattern_authors ' ()' t/*.sh | sort | uniq -c | sort -rn
84 Junio C Hamano
32 Jonathan Nieder
16 Thomas Rast
16 Johan Herland
12 Johannes Sixt
12 Johannes Schindelin
...
$ pattern_authors '[^ ]()' t/*.sh | sort | uniq -c | sort -rn
52 Jeff King
26 Jonathan Nieder
18 Nguyễn Thái Ngọc Duy
16 Wincent Colaiuta
14 Jon Seymour
10 Tay Ray Chuan
...
So there are definitely particular people who prefer different styles
(and I recalled that Junio and I differed on this style point, which is
confirmed here). Interestingly, you are the only person to fall right in
the middle. I guess that means you are good at emulating surrounding
code. :)
-Peff
next prev parent reply other threads:[~2011-05-06 21:48 UTC|newest]
Thread overview: 46+ messages / expand[flat|nested] mbox.gz Atom feed top
2011-05-02 19:23 [PATCH 2] Add default merge options for all branches Michael Grubb
2011-05-02 22:47 ` Miklos Vajna
2011-05-02 23:36 ` Junio C Hamano
2011-05-03 5:35 ` Michael Grubb
2011-05-03 5:38 ` [PATCH v3] " Michael Grubb
2011-05-03 9:03 ` Jonathan Nieder
2011-05-03 9:49 ` Jonathan Nieder
2011-05-03 16:46 ` Michael Grubb
2011-05-03 18:16 ` Junio C Hamano
2011-05-03 20:22 ` Michael Grubb
2011-05-03 20:50 ` Jonathan Nieder
2011-05-03 20:37 ` Jens Lehmann
2011-05-03 20:07 ` [PATCH v4] " Michael Grubb
2011-05-03 20:36 ` Michael Grubb
2011-05-03 20:44 ` Jonathan Nieder
2011-05-03 22:51 ` Junio C Hamano
2011-05-04 4:25 ` Junio C Hamano
2011-05-04 4:28 ` Michael Grubb
2011-05-04 4:58 ` Jonathan Nieder
2011-05-04 18:58 ` Michael Grubb
2011-05-04 21:35 ` Junio C Hamano
2011-05-04 10:58 ` John Szakmeister
2011-05-03 22:03 ` Junio C Hamano
2011-05-03 20:37 ` [PATCH v4.1] " Michael Grubb
2011-05-04 22:07 ` [PATCH v5] " Michael Grubb
2011-05-05 0:42 ` Junio C Hamano
2011-05-06 20:36 ` Junio C Hamano
2011-05-06 21:59 ` Jonathan Nieder
2011-05-06 20:54 ` [PATCH 0/2] tests: make verify_merge check that the number of parents is right Jonathan Nieder
2011-05-06 20:58 ` [PATCH 1/2] tests: eliminate unnecessary setup test assertions Jonathan Nieder
2011-05-06 21:48 ` Jeff King [this message]
2011-05-06 22:13 ` Jeff King
2011-05-06 22:27 ` Junio C Hamano
2011-05-06 22:29 ` Jeff King
2011-05-07 22:05 ` Junio C Hamano
2011-05-09 13:31 ` [PATCH 0/3] blame --line-porcelain Jeff King
2011-05-09 13:33 ` [PATCH 1/3] add tests for various blame formats Jeff King
2011-05-09 13:34 ` [PATCH 2/3] blame: refactor porcelain output Jeff King
2011-05-09 15:39 ` Thiago Farina
2011-05-09 13:34 ` [PATCH 3/3] blame: add --line-porcelain output format Jeff King
2011-05-06 22:26 ` [PATCH 1/2] tests: eliminate unnecessary setup test assertions Jonathan Nieder
2011-05-06 21:00 ` [PATCH 2/2] tests: teach verify_parents to check for extra parents Jonathan Nieder
2011-05-06 21:34 ` Junio C Hamano
2011-05-06 21:42 ` Jonathan Nieder
2011-05-06 21:32 ` [PATCH v5] Add default merge options for all branches Jonathan Nieder
2011-05-06 22:01 ` Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20110506214801.GA17848@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=devel@dailyvoid.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jrnieder@gmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).