git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Felipe Contreras <felipe.contreras@gmail.com>
Cc: Junio C Hamano <gitster@pobox.com>, git@vger.kernel.org
Subject: Re: [RFC/PATCH] tests: add initial bash completion tests
Date: Fri, 6 Apr 2012 17:42:19 -0400	[thread overview]
Message-ID: <20120406214219.GB5436@sigill.intra.peff.net> (raw)
In-Reply-To: <CAMP44s0xAvc9aTeBP81tXhX6Q67+7cQ-51C1AWrKPN7bc7=JXQ@mail.gmail.com>

On Sat, Apr 07, 2012 at 12:28:05AM +0300, Felipe Contreras wrote:

> On Fri, Apr 6, 2012 at 11:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> > Jeff King <peff@peff.net> writes:
> >
> >> On Fri, Apr 06, 2012 at 10:28:39PM +0300, Felipe Contreras wrote:
> >>
> >>> Something is better than nothing.
> >>
> >> Yes, but...
> >
> > ;-)
> >
> > This is a good example that sometimes something is worse than nothing,
> > unless watched carefully by a competent reviewer.
> 
> And this is a good example of why you shouldn't blindly trust what a
> 'competent reviewer' says, as I'm pretty sure the comment is wrong.
> 
> But hey, if you prefer nothing, fine, drop this patch; let's continue
> to blindly modify the completion and fix regressions as they come. I
> guess I should drop my other tests as well.

Sorry, but I think you are wrong, and this is a perfect example of why
you are sometimes frustrating to work with. Your patch is definitely a
move in the right direction, and we would love to have something like it
as part of git. And I'm sure it runs fine under _your_ setup. But the
git community is much larger than just your setup, and your patch is a
regression for other people, as it breaks "make test".

Did I say "let's throw away this patch"? No. I said "here is a problem
with your patch", and I even sketched out a fix.

And nor do I think Junio was saying "let's throw it out". I think he was
responding specifically to "something is better than nothing". It's
_not_ better if it regresses other cases. So the patch as-is is not
acceptable, but it could be made so.

But instead of taking the constructive criticism and iterating on your
patch, you are ready to withdraw your patch. That seems silly when you
have already done the hard part, and the fix to make the patch
acceptable is the easy part.

But maybe I am wrong. Maybe there is no problem at all with your patch,
and my analysis is wrong, and yours is right. I am willing to admit that
as a possibility. But let's discuss it in the other part of the thread
and find out, shall we?

-Peff

  reply	other threads:[~2012-04-06 21:42 UTC|newest]

Thread overview: 18+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2012-04-06 19:28 [RFC/PATCH] tests: add initial bash completion tests Felipe Contreras
2012-04-06 20:19 ` Jeff King
2012-04-06 20:24   ` Junio C Hamano
2012-04-06 21:28     ` Felipe Contreras
2012-04-06 21:42       ` Jeff King [this message]
2012-04-06 22:22         ` Felipe Contreras
2012-04-06 21:42       ` Junio C Hamano
2012-04-06 21:21   ` Felipe Contreras
2012-04-06 21:34     ` Jeff King
2012-04-06 22:05       ` Felipe Contreras
2012-04-06 22:46         ` Junio C Hamano
2012-04-06 23:30           ` Felipe Contreras
2012-04-06 23:45         ` Jeff King
2012-04-06 23:12   ` Felipe Contreras
2012-04-06 23:22     ` Junio C Hamano
2012-04-06 23:33       ` Junio C Hamano
2012-04-07  0:02         ` Felipe Contreras
2012-04-06 23:52     ` Jeff King

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=20120406214219.GB5436@sigill.intra.peff.net \
    --to=peff@peff.net \
    --cc=felipe.contreras@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.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).