git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
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

  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).