From: Jeff King <peff@peff.net>
Date: Wed, 10 Aug 2016 13:37:46 GMT
Subject: Re: [PATCH v5 03/15] pkt-line: add `gentle` parameter to format_packet()
Message-ID: <20160810133745.wagccvvf35o3pbwb@sigill.intra.peff.net>
In-Reply-To: <F4D9C42A-5B7F-47B4-B334-704D5F6210F5@gmail.com>

On Wed, Aug 10, 2016 at 03:29:26PM +0200, Lars Schneider wrote:

> 
> > On 10 Aug 2016, at 15:15, Jeff King <peff@peff.net> wrote:
> > 
> > On Wed, Aug 10, 2016 at 03:03:59PM +0200, larsxschneider@gmail.com wrote:
> > 
> >> From: Lars Schneider <larsxschneider@gmail.com>
> >> 
> >> format_packet() dies if the caller wants to format a packet larger than
> >> LARGE_PACKET_MAX. Certain callers might prefer an error response instead.
> > 
> > I am not sure I agree here. Certainly I see the usefulness of gently
> > handling a failure to write(). But if you are passing in too-large
> > buffers, isn't that a bug in the program?
> > 
> > How would you recover, except by splitting up the content? That might
> > not be possible depending on how you are using the pkt-lines. And even
> > if it is, wouldn't it be simpler to split it up before sending it to
> > format_packet()?
> 
> Good argument. I agree - this patch should be dropped.

Actually, after reading further, one thought did occur to me. Let's say
you are writing to a smudge filter, and one of the header packets you
send has the filename in it. So you might do something like:

  if (packet_write_fmt_gently(fd, "filename=%s", filename) < 0) {
	if (filter_required)
		die(...);
	else
		return -1; /* we tried our best; skip smudge */
  }

The "recovery" there is not to try sending again, but rather to give up.
And that is presumably a sane outcome for somebody who tries to checkout
a filename larger than 64K.

It does still feel a little weird that you cannot tell the difference
between a write() error and bad input. Because you really might want to
do something different between the two. Like:

  #define MAX_FILENAME (PKTLINE_DATA_MAXLEN - strlen("filename"))

  if (filename > MAX_FILENAME) {
	warning("woah, that name is ridiculous; truncating");
	ret = packet_write_fmt_gently(fd, "%.*s", MAX_FILENAME, filename);
  } else
	ret = packet_write_fmt_gently(fd, "%s", filename);

-Peff