Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to remote-helper.

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Jul 21, 2012, 14:48 UTC
Message-ID: <20120721144834.GB19860@burratino>
In-Reply-To: <2448876.O3MA5kWbuX@flobuntu>

Hi,

Florian Achleitner wrote:

> [Subject: Re: [RFC 4/4 v3] Add cat-blob report fifo from fast-import to
> remote-helper.]

Is this on top of patches 1, 2, and 3 from v2 of the series?

*checks* Looks like it doesn't overlap with any of the files from those patches, so I don't have to understand them first. Phew. My suggestion for next time would be to submit patches that can be understood on their own independently instead of as part of a series.

Show 5 quoted lines

> For some fast-import commands (e.g. cat-blob) an answer-channel
> is required. For this purpose a fifo (aka named pipe) (mkfifo)
> is created (.git/fast-import-report-fifo) by the transport-helper
> when fetch via import is requested. The remote-helper and
> fast-import open the ends of the pipe.

Motivation described! But it's odd --- it seems like this is doing at least two things:

 1) adding to the fast-import interface
 2) using the new fast-import feature in some in-tree callers

Those really want to be separate patches. That way, the fast-import change can be studied by other implementers of the fast-import interface (hg-fast-import, bzr-fast-import). As a side-benefit, it gives an easy check that any changes to fast-import were at least roughly backward-compatible ("did all the in-tree users still work?").

I'll focus on the new fast-import change below, since it's the most important part.

> The filename of the fifo is passed to the remote-helper via
> it's environment, helpers that don't use fast-import can
> simply ignore it.

My first impression is that I'd rather there be a command to request the filename instead of using the environment for the first time, since when debugging people would already be monitoring the command stream and responses.

> Add a new command line option --cat-blob-pipe to fast-import,
> for this purpose.

This is completely redundant next to --cat-blob-fd, right? That's really problematic --- adding new interfaces means new code and gratuitous incompatibility with all existing fast-import backends, with no benefit in return.

I imagine that there was some portability reason you were thinking about, but the above doesn't mention it at all. Future readers scratching their heads at the changelog can't read your mind! Please please please explain what you're trying to do.

Since if we're lucky fixing that could mean not having to change fast-import at all, I'm stopping here.

Another quick thought: any finished patch adding a new fast-import feature should also include

 - documentation in the manpage (Documentation/fast-import.txt)
 - testcases to make sure your careful work does not get broken
   by later changes (somewhere in t/*fast-import*.sh)

But don't worry too much about that now --- sending incomplete patches for review before then to make sure the direction is sane is a very good idea, as long as they are marked as such (as you've already done by marking this as RFC).

To sum up: I think we should just stick to pipes --- why all this fifo complication?

Hope that helps, Jonathan