Re: [PATCH 1/5] fast-import: Let importers retrieve blobs
From: Jonathan Nieder <hidden>
Date: 2016-06-15 22:49:48
(+cc: Sam) Hi, David Barr wrote:
So introduce another way: a "cat-blob" command introduced in the command stream requests for fast-import to print a blob to stdout or a file descriptor specified by the argument --cat-blob-fd.
Yes, please!
Cc: Shawn O. Pearce <redacted> Cc: Ramkumar Ramachandra <redacted>
It turns out these Cc tags are not supposed to be used except in some very weird circumstances. See [1] if curious. [...]
quoted hunk ↗ jump to hunk
--- a/Documentation/git-fast-import.txt +++ b/Documentation/git-fast-import.txt@@ -92,6 +92,17 @@ OPTIONS --(no-)-relative-marks= with the --(import|export)-marks= options. +--cat-blob-fd=<fd>:: + Specify the file descriptor that will be written to + when the `cat-blob` command is encountered in the stream. + The default behaviour is to write to `stdout`.
Sounds good.
++ +The described objects are not necessarily accessible +using standard git plumbing tools until a little while +after the next checkpoint. To request access to the +blobs before then, use `cat-blob` lines in the command +stream.
This is stale explanation from --report-fd, I think, to explain why the commit ids it printed were not very useful. It would be possible to reword it to describe cat-blob-fd but since the frontend does not have easy access to blob names as it is, I think cat-blob motivates itself on its own. [...]
quoted hunk ↗ jump to hunk
@@ -876,6 +892,23 @@ Placing a `progress` command immediately after a `checkpoint` will inform the reader when the `checkpoint` has been completed and it can safely access the refs that fast-import updated. +`cat-blob` +~~~~~
~~~~~~~~~~
quoted hunk ↗ jump to hunk
@@ -896,6 +929,7 @@ The following features are currently supported: * date-format * import-marks * export-marks +* cat-blob
The explanation says (paraphrased) "Features work identically to their option counterparts, with the exception of import-marks as described below". Maybe ought to be reworded? date-format:: export-marks:: relative-marks:: no-relative-marks:: force:: See the corresponding command-line option. import-marks:: Like --import-marks, except in two respects. First, only one "feature import-marks" command is allowed per stream. Second, an --import-marks= specified on the command line will override it. cat-blob:: No-op to check that the importer supports the cat-blob command. By the way, it might be nice to make cat-blob not just check the importer but the environment in which it was invoked, like this: exporter says: feature this feature that feature cat-blob feature another ... importer says: feature cat-blob That is, after writing "feature cat-blob\n", an exporter could tell if the backchannel was set up correctly by reading for "feature cat-blob\n" from the importer.
quoted hunk ↗ jump to hunk
--- a/fast-import.c +++ b/fast-import.c@@ -2680,6 +2685,77 @@ static void parse_reset_branch(void) unread_command_buf = 1; } +static void cat_blob_write(const char *buf, unsigned long size) +{ + if (write_in_full(cat_blob_fd, buf, size) != size) + die_errno("Write to frontend failed"); +}
An odd operation, since if the pipe_buf gets filled then it blocks until the exporter finds time to read. Maybe in some future version this would write to a private ring buffer and there would be an event loop or seperate thread to flush it out when the exporter is ready. Upshot: I am happy with this as a separate function. [...]
quoted hunk ↗ jump to hunk
@@ -2808,6 +2892,8 @@ static int parse_one_feature(const char *feature, int from_stream) option_import_marks(feature + 13, from_stream); } else if (!prefixcmp(feature, "export-marks=")) { option_export_marks(feature + 13); + } else if (!prefixcmp(feature, "cat-blob")) { + /* Don't die - this feature is supported */
Maybe if (!strcmp(...? [...]
quoted hunk ↗ jump to hunk
@@ -2896,6 +2982,10 @@ static void parse_argv(void) if (*a != '-' || !strcmp(a, "--")) break; + if (!prefixcmp(a + 2, "cat-blob-fd=")) { + option_cat_blob_fd(a + 2 + 12); + } + if (parse_one_option(a + 2)) continue;
Probably worth mentioning in the manual, under the option command: The following command-line option describes the environment in which fast-import was executed and may not be passed to 'option': * cat-blob-fd
quoted hunk ↗ jump to hunk
@@ -2953,6 +3043,8 @@ int main(int argc, const char **argv) parse_new_tag(); else if (!prefixcmp(command_buf.buf, "reset ")) parse_reset_branch(); + else if (!prefixcmp(command_buf.buf, "cat-blob ")) + parse_cat_blob();
Thanks. That was simple. :) Tests? [1] http://thread.gmane.org/gmane.comp.version-control.git/157711