git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: git@vger.kernel.org, Johannes Sixt <j6t@kdbg.org>
Subject: Re: [RFC] allowing hooks to ignore input?
Date: Thu, 18 Sep 2014 07:14:12 -0400	[thread overview]
Message-ID: <20140918111412.GB13481@peff.net> (raw)
In-Reply-To: <xmqq7g1319rz.fsf@gitster.dls.corp.google.com>

On Tue, Sep 16, 2014 at 03:27:12PM -0700, Junio C Hamano wrote:

> Johannes Sixt <j6t@kdbg.org> writes:
> 
> > I think this is a good move. Hooks are written by users, who sometimes
> > are not clueful enough.
> 
> Thanks for a sanity check.  I do not think it is about cluefulness
> in this particular case.  A rule that is not meaningfully enforced
> by reliably failing offenders is a rule that is hard to follow if a
> clueful user wanted to.

This looks good to me. Here's a real-world example of somebody getting
bitten by this (I _thought_ we had dealt with it then, but apparently
not):

  http://article.gmane.org/gmane.comp.version-control.git/186287

> This round comes with a test, but depending on the size of your pipe
> buffer and context switching race, an unpatched Git may pass it
> (reducing the test_seq number down to say 199 would certainly make
> it pass without the fix most of the time).

I took a look at your test. Having worked on racy sigpipe tests
recently, I think the only way to guarantee failure is to make sure you
fill up the pipe buffer. My experiments showed this was typically 64K on
a variety of Linux systems, but I think can be bumped up at runtime.

When working on the sanitized_signals() test, we decided to just pick an
arbitrarily high number like 1MB and use that (ideally you would send
infinite data until you get SIGPIPE, but the failure mode for your test
is then that it doesn't finish :-/).

You have things much harder and much easier here. Harder, in that you
can only convince git to send ~100 bytes per ref, so you would need a
lot of refs (and thus it's expensive to over-compensate). But easier, in
that you do not need to _reliably_ catch SIGPIPE. You only need to catch
it often enough that somebody notices if the rest is broken, not catch
it every time to ensure success.

So I think it is OK as-is. You should be generating ~91K of ref data
(refname + two sha1s + spaces and newline), and I can't imagine many
systems have a pipe buffer bigger than 64K. If they do, the only
downside is that those systems might only intermittently catch a
regression.

-Peff

      reply	other threads:[~2014-09-18 11:14 UTC|newest]

Thread overview: 5+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-09-12 22:48 [RFC] allowing hooks to ignore input? Junio C Hamano
2014-09-13  8:19 ` Johannes Sixt
2014-09-15 17:51   ` Junio C Hamano
2014-09-16 22:27   ` Junio C Hamano
2014-09-18 11:14     ` Jeff King [this message]

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=20140918111412.GB13481@peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=j6t@kdbg.org \
    /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).