Re: `git bundle create -` may not write to `stdout`

From

Jeff King <peff@peff.net>

Date

Feb 26, 2023, 23:16 UTC

Message-ID

<Y/voNv1OQ1Cf/N5a@coredump.intra.peff.net>

In-Reply-To

<80beb487-cd93-06ed-88cf-87a96a829ff6@drmikehenry.com>

On Sat, Feb 25, 2023 at 07:58:33AM -0500, Michael Henry wrote:

> Summary
> =======
> 
> Using `-` to create a bundle file on `stdout` works only when the
> current working directory is at the root of the repository; when in a
> subdirectory, `-` is treated as the name of a file instead.

The most directed fix is this:

diff --git a/builtin/bundle.c b/builtin/bundle.c
index acceef6200..145b814f48 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -59,7 +59,9 @@ static int parse_options_cmd_bundle(int argc,
 			     PARSE_OPT_STOP_AT_NON_OPTION);
 	if (!argc)
 		usage_msg_opt(_("need a <file> argument"), usagestr, options);
-	*bundle_file = prefix_filename(prefix, argv[0]);
+	*bundle_file = strcmp(argv[0], "-") ?
+		       prefix_filename(prefix, argv[0]) :
+		       xstrdup(argv[0]);
 	return argc;
 }
 

though I wonder if there are similar cases for other commands,
especially ones where the call to prefix_filename() is lurking behind an
OPT_FILENAME() option.

It's tempting to treat this name as special at that level, like:

diff --git a/abspath.c b/abspath.c
index 39e06b5848..e89697c85f 100644
--- a/abspath.c
+++ b/abspath.c
@@ -269,7 +269,7 @@ char *prefix_filename(const char *pfx, const char *arg)
 
 	if (!pfx_len)
 		; /* nothing to prefix */
-	else if (is_absolute_path(arg))
+	else if (is_absolute_path(arg) || !strcmp(arg, "-"))
 		pfx_len = 0;
 	else
 		strbuf_add(&path, pfx, pfx_len);

but I suspect that may bite us in the long run. Something like:

  git mv -- foo -

should treat "-" literally, and not as some special token (stdout does
not even make sense in this context).

> Anything else you want to add:
> ==============================
> 
> It's unclear to me whether creating a bundle file to `stdout` is documented
> behavior.  I can't find direct mention of it in
> `Documentation/git-bundle.txt`,
> though that document does have this text:
> 
>     --all-progress::
>             When --stdout is specified then progress report is
>             displayed during the object count and compression phases
>             but inhibited during the write-out phase. The reason is
>             that in some cases the output stream is directly linked
>             to another command which may wish to display progress
>             status of its own as it processes incoming pack data.
>             This flag is like --progress except that it forces progress
>             report for the write-out phase as well even if --stdout is
>             used.
> 
> The switch `--stdout` doesn't seem to exist, though; perhaps it was a past
> feature that got removed but the documentation hung around?

It comes from 79862b6b77 (bundle-create: progress output control,
2019-11-10), which I think was trying to copy the explanation from the
similar options in pack-objects (which underlies git-bundle, and we just
forward those options to it).

Every reference to "--stdout" there should probably be replaced with
"when writing a bundle to stdout" (or alternatively, these should
perhaps just be pointers to the pack-objects equivalents).

And yes, we should document "-" (probably when discussing <file> in the
"create" section of OPTIONS), as I don't see any mention of it. Its
behavior is intentional (it goes all the way back to 2e0afafebd (Add
git-bundle: move objects and references by archive, 2007-02-22)).

I think having "--stdout" would have been a nicer choice to avoid this
ambiguity, but at this point it is probably not worth going through
deprecation dance to get rid of it.

> I find the ability to create a bundle to `stdout` a useful feature,
> though a niche use case: I'm post-processing the bundle file's
> contents before writing to a file, and bundling to `stdout` saves the
> creation of a potentially large temporary file.  I'm currently using
> the temporary file approach, however, because I'm not sure that
> bundling to `stdout` is intended as a supported feature; I'll leave
> that for you to determine.

I think bundling to stdout is perfectly reasonable. Even without
post-processing, if you need to use a command to get it to its ultimate
storage spot (e.g., by piping into a network command like "nc" or
something), it's nice to avoid the temporary file because it could
potentially be huge.

So it seems like we'd want a three-patch series:

  1. The first hunk I showed above, along with a test to demonstrate the
     fix.

  2. Remove bogus references to --stdout in the docs.

  3. Document "-".

Do you want to try your hand at that? (It's OK if not, but I like to
trick^W give opportunities to new folks to contribute).

-Peff