git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Eric Sunshine <sunshine@sunshineco.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
	"Pratik Karki" <predatoramigo@gmail.com>,
	"Git List" <git@vger.kernel.org>
Subject: Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite
Date: Thu, 15 Mar 2018 10:04:01 -0700	[thread overview]
Message-ID: <xmqqsh915kzi.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <CAPig+cTLCswg_=q5ybnyN3As4Au05q5eAcA7Prr643KCgZ0OAw@mail.gmail.com> (Eric Sunshine's message of "Wed, 14 Mar 2018 14:22:45 -0400")

Eric Sunshine <sunshine@sunshineco.com> writes:

> Thanks for presenting an opposing opinion. While I understand your
> position, the reason for my suggested transformation is that if the
> patch already transformed the code in the way suggested, it would
> increase my confidence, as a reviewer, that the patch author had
> _studied_ and _understood_ the code. Increased confidence is
> especially important for mechanical transformations since -- as seen
> in the unsnipped review comment below -- blindly-applied mechanical
> transformations can be suboptimal or outright incorrect.
>
> It's also the sort of review comment I would make even to very
> seasoned project participants[1].
>
> [1]: https://public-inbox.org/git/CAPig+cQLmYQeRhPxvZHmY7gApnbE25H_KoSWs-ZjuBo4BruimQ@mail.gmail.com/

Yes, it is a good example that mechanical conversions are often
mind-numbing and make even seasoned participants miss trivially
obvious improvement opportunities ;-)

It however is OK to be more lenient to newer participants and allow
deferring such "while at it, make it right" on top of "minimally
required for correctness", in order to encourage them by getting
something to the tree early ;-)

  reply	other threads:[~2018-03-15 17:04 UTC|newest]

Thread overview: 16+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-03-13 20:19 Pratik Karki
2018-03-14  7:30 ` Eric Sunshine
2018-03-14  9:57   ` Ævar Arnfjörð Bjarmason
2018-03-14 18:22     ` Eric Sunshine
2018-03-15 17:04       ` Junio C Hamano [this message]
2018-03-19 17:32         ` [GSoC][PATCH] " Pratik Karki
2018-03-21 11:02           ` Eric Sunshine
2018-03-21 15:23             ` [GSoC][PATCH v3] test: avoid pipes in git related commands for test Pratik Karki
2018-03-21 18:11               ` Junio C Hamano
2018-03-21 18:45                 ` Eric Sunshine
2018-03-21 18:58                 ` Eric Sunshine
2018-03-23 15:01                   ` [GSoC][PATCH v4] " Pratik Karki
2018-03-25  8:37                     ` Eric Sunshine
2018-03-27 17:31                       ` [GSoC][PATCH v5] " Pratik Karki
2018-03-30 21:45                         ` Eric Sunshine
2018-03-30 22:08                           ` 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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqsh915kzi.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=predatoramigo@gmail.com \
    --cc=sunshine@sunshineco.com \
    --subject='Re: [GSoC] [PATCH] test: avoid pipes in git related commands for test suite' \
    /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

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

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 read-only IMAP folder(s) and NNTP newsgroup(s).