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

[Jonathan Nieder <jrnieder@gmail.com>]

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.

> 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