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

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

[Michael Henry]

All,

I've encountered some unexpected behavior with `git bundle create -`.

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.

What did you do before the bug happened? (Steps to reproduce your issue)
========================================================================

The below steps are using Git's `next` branch (cloned today) to demonstrate.

- Bundle creation to `stdout` works in the repository root:

      $ ./git bundle create -q - HEAD ^HEAD~ > head.bundle
      $ ./git bundle list-heads head.bundle
      a93b1961839a603a8ac2df08fd80c48bd140fe02 HEAD

- From the `Documentation` directory, `-` is treated like a filename:

      $ cd Documentation/
      $ ../git bundle create -q - HEAD ^HEAD~ > head.bundle
      $ ../git bundle list-heads head.bundle
      error: 'Documentation/head.bundle' does not look like a v2 or v3
bundle file
      $ ../git bundle list-heads ./-
      a93b1961839a603a8ac2df08fd80c48bd140fe02 HEAD

- Consider this patch to display the bundle file path:

      diff --git a/bundle.c b/bundle.c
      index 6ab6cd7378..05be3ed520 100644
      --- a/bundle.c
      +++ b/bundle.c
      @@ -524,6 +524,7 @@ int create_bundle(struct repository *r, const
char *path,
                      goto err;
              }
       
      +    fprintf(stderr, "create_bundle(): path=\"%s\"\n", path);
              bundle_to_stdout = !strcmp(path, "-");
              if (bundle_to_stdout)
                      bundle_fd = 1;

  When in a subdirectory, it seems that the current working directory is
being
  changed to the root, and the bundle file's path is being adjusted
accordingly:

      $ ../git bundle create -q - HEAD ^HEAD~ > head.bundle
      create_bundle(): path="Documentation/-"

What did you expect to happen? (Expected behavior)
==================================================

I expected `-` to be treated as `stdout` in subdirectories as well as in the
repository root directory.

What happened instead? (Actual behavior)
========================================

When in a subdirectory `some/subdir`, `-` is converted to
`some/subdir/-` and
treated like a regular file instead of `stdout`.

What's different between what you expected and what actually happened?
======================================================================

I expected the bundle file to appear on `stdout`; instead, it was
written to a
file named `-` in the current directory.

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?

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.

Thanks,
Michael Henry

==============================================================================

[System Info]
git version:
git version 2.40.0.rc0.245.ga93b196183
cpu: x86_64
built from commit: a93b1961839a603a8ac2df08fd80c48bd140fe02
sizeof-long: 8
sizeof-size_t: 8
shell-path: /bin/sh
uname: Linux 5.15.0-52-generic #58-Ubuntu SMP Thu Oct 13 08:03:55 UTC
2022 x86_64
compiler info: gnuc: 11.3
libc info: glibc: 2.35
$SHELL (typically, interactive shell): /bin/bash


[Enabled Hooks]

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

[Jeff King]

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.

Hmm, yeah. The problem is that we call prefix_filename() to accommodate
the fact that we'll have changed directory to the root of the working
tree, and it has no knowledge that "-" is special for bundle creation.

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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;
>  }

This fell thru cracks, it seems.

OPT_FILENAME() needs to do exactly this, and has its own helper
function in parse-options.c::fix_filename(), but its memory
ownership rules is somewhat screwed (it was perfectly fine in the
original context of "we parse the bounded number of options the user
would give us from the command line, and giving more than one
instance of these last-one-wins option may leak but we do not
care---if you do not like leaking, don't give duplicates", but with
automated leak checking that does not care about such nuances, it
will trigger unnecessary errors), and cannot be reused here.

Here is your "most directed fix" packaged into a call to a helper
function.  Given that we may want to slim the cache.h header, it may
not want to be declared there, but for now, its declaration sits
next to prefix_filename().


diff --git c/abspath.c w/abspath.c
index 39e06b5848..b2fd9ff321 100644
--- c/abspath.c
+++ w/abspath.c
@@ -280,3 +280,10 @@ char *prefix_filename(const char *pfx, const char *arg)
 #endif
 	return strbuf_detach(&path, NULL);
 }
+
+char *prefix_filename_except_for_dash(const char *pfx, const char *arg)
+{
+	if (!strcmp(arg, "-"))
+		return xstrdup(arg);
+	return prefix_filename(pfx, arg);
+}
diff --git c/builtin/bundle.c w/builtin/bundle.c
index acceef6200..3056dbeee3 100644
--- c/builtin/bundle.c
+++ w/builtin/bundle.c
@@ -59,7 +59,7 @@ 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 = prefix_filename_except_for_dash(prefix, argv[0]);
 	return argc;
 }
 
diff --git c/cache.h w/cache.h
index 12789903e8..38867de41a 100644
--- c/cache.h
+++ w/cache.h
@@ -638,6 +638,9 @@ char *prefix_path_gently(const char *prefix, int len, int *remaining, const char
  */
 char *prefix_filename(const char *prefix, const char *path);
 
+/* Likewise, but path=="-" always yields "-" */
+char *prefix_filename_except_for_dash(const char *prefix, const char *path);
+
 int check_filename(const char *prefix, const char *name);
 void verify_filename(const char *prefix,
 		     const char *name,
diff --git c/parse-options.c w/parse-options.c
index fd4743293f..bd1ed906a8 100644
--- c/parse-options.c
+++ w/parse-options.c
@@ -59,6 +59,10 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,
 	return 0;
 }
 
+/*
+ * NEEDSWORK: fix memory ownership rules around here and then use
+ * prefix_filename_except_for_dash() helper.
+ */
 static void fix_filename(const char *prefix, const char **file)
 {
 	if (!file || !*file || !prefix || is_absolute_path(*file)
diff --git c/t/t6020-bundle-misc.sh w/t/t6020-bundle-misc.sh
index 7d40994991..d14f7cea91 100755
--- c/t/t6020-bundle-misc.sh
+++ w/t/t6020-bundle-misc.sh
@@ -606,4 +606,15 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
 	)
 '
 
+test_expect_success 'send a bundle to standard output' '
+	git bundle create - --all HEAD >bundle-one &&
+	mkdir -p down &&
+	git -C down bundle create - --all HEAD >bundle-two &&
+	git bundle verify bundle-one &&
+	git bundle verify bundle-two &&
+	git ls-remote bundle-one >expect &&
+	git ls-remote bundle-two >actual &&
+	test_cmp expect actual
+'
+
 test_done

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

[Jeff King]

On Fri, Mar 03, 2023 at 02:31:05PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > 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;
> >  }
> 
> This fell thru cracks, it seems.

I was waiting to give Michael a chance to respond to my offer. :)

> OPT_FILENAME() needs to do exactly this, and has its own helper
> function in parse-options.c::fix_filename(), but its memory
> ownership rules is somewhat screwed (it was perfectly fine in the
> original context of "we parse the bounded number of options the user
> would give us from the command line, and giving more than one
> instance of these last-one-wins option may leak but we do not
> care---if you do not like leaking, don't give duplicates", but with
> automated leak checking that does not care about such nuances, it
> will trigger unnecessary errors), and cannot be reused here.

Heh, I was worried about it kicking in for spots that "-" was not
meaningful, but I checked only prefix_filename() itself, and didn't
think to check OPT_FILENAME()'s full code path.

(I do still think we don't want to push it down into prefix_filename(),
because it gets used for paths and pathspecs given raw on the command
line. It does make me wonder if there are places where OPT_FILENAME() is
doing the wrong thing).

> Here is your "most directed fix" packaged into a call to a helper
> function.  Given that we may want to slim the cache.h header, it may
> not want to be declared there, but for now, its declaration sits
> next to prefix_filename().

Yeah, a helper may be nice, though if this is the only spot, I'd be
tempted not to even bother until fix_filename() is fixed. The obvious
fix is for it to always allocate, but callers will need to be adjusted.
I suspect it will trigger a bunch of complaints from the leak-checking
tests (because the caller does not expect it to be allocated, so it's a
sometimes-leak now, but it will become an always-leak).

> diff --git c/t/t6020-bundle-misc.sh w/t/t6020-bundle-misc.sh
> index 7d40994991..d14f7cea91 100755
> --- c/t/t6020-bundle-misc.sh
> +++ w/t/t6020-bundle-misc.sh
> @@ -606,4 +606,15 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
>  	)
>  '
>  
> +test_expect_success 'send a bundle to standard output' '
> +	git bundle create - --all HEAD >bundle-one &&
> +	mkdir -p down &&
> +	git -C down bundle create - --all HEAD >bundle-two &&
> +	git bundle verify bundle-one &&
> +	git bundle verify bundle-two &&
> +	git ls-remote bundle-one >expect &&
> +	git ls-remote bundle-two >actual &&
> +	test_cmp expect actual
> +'

This test looks good to me. Let's also not forget about the doc fixes. I
don't think there's much urgency to get this into v2.40, but I can put
it together in the next day or three.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> This test looks good to me. Let's also not forget about the doc fixes. I
> don't think there's much urgency to get this into v2.40,

Doc?  Meaning 

	<file> can be "-" to mean the standard output (for writing)
	or the standard input (for reading)

or something?

Given that the other three subcommands also take <file>

    'git bundle' create [-q | --quiet | --progress | --all-progress] ...
                        [--version=<version>] <file> <git-rev-list-args>
    'git bundle' verify [-q | --quiet] <file>
    'git bundle' list-heads <file> [<refname>...]
    'git bundle' unbundle [--progress] <file> [<refname>...]

but read_bundle_header() function all three calls begins like so:

    int read_bundle_header(const char *path, struct bundle_header *header)
    {
            int fd = open(path, O_RDONLY);

            if (fd < 0)
                    return error(_("could not open '%s'"), path);
            return read_bundle_header_fd(fd, header, path);
    }

this function needs to be fixed first ;-)

Of course none of these is urgent for the upcoming 2.40 ;-).

> but I can put
> it together in the next day or three.

Thanks.  Just for reference, here is what I have (just a log
message, the patch is the same and does not support input yet).

----- >8 -----
Subject: [PATCH] bundle: don't blindly apply prefix_filename() to "-"

A user can specify a filename to a command from the command line,
either as the value given to a command line option, or a command
line argument.  When it is given as a relative filename, in the
user's mind, it is relative to the directory "git" was started from,
but by the time the filename is used, "git" would almost always have
chdir()'ed up to the root level of the working tree.

The given filename, if it is relative, needs to be prefixed with the
path to the current directory, and it typically is done by calling
prefix_filename() helper function.  For commands that can also take
"-" to use the standard input or the standard output, however, this
needs to be done with care.

"git bundle create" uses the next word on the command line as the
output filename, and can take "-" to mean "write to the standard
output".  It blindly called prefix_filename(), so running it in a
subdirectory did not quite work as expected.

Introduce a new helper, prefix_filename_except_for_dash(), and use
it to help "git bundle create" codepath.

Reported-by: Michael Henry
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 abspath.c              |  7 +++++++
 builtin/bundle.c       |  2 +-
 cache.h                |  3 +++
 parse-options.c        |  4 ++++
 t/t6020-bundle-misc.sh | 11 +++++++++++
 5 files changed, 26 insertions(+), 1 deletion(-)

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

[Jeff King]

On Fri, Mar 03, 2023 at 03:05:14PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > This test looks good to me. Let's also not forget about the doc fixes. I
> > don't think there's much urgency to get this into v2.40,
> 
> Doc?  Meaning 
> 
> 	<file> can be "-" to mean the standard output (for writing)
> 	or the standard input (for reading)
> 
> or something?

Yeah, I was referring to my earlier mail in the thread, which said:

>> 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 "-".

Your patch is (1), but we'd want (2) and (3) still.

> Given that the other three subcommands also take <file>
> 
>     'git bundle' create [-q | --quiet | --progress | --all-progress] ...
>                         [--version=<version>] <file> <git-rev-list-args>
>     'git bundle' verify [-q | --quiet] <file>
>     'git bundle' list-heads <file> [<refname>...]
>     'git bundle' unbundle [--progress] <file> [<refname>...]
> 
> but read_bundle_header() function all three calls begins like so:
> 
>     int read_bundle_header(const char *path, struct bundle_header *header)
>     {
>             int fd = open(path, O_RDONLY);
> 
>             if (fd < 0)
>                     return error(_("could not open '%s'"), path);
>             return read_bundle_header_fd(fd, header, path);
>     }
> 
> this function needs to be fixed first ;-)

I wasn't thinking of changing the behavior for input, but just focusing
the docs in the right spot (the "create" option), like:

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index 18a022b4b4..ea6b5c24d1 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt
@@ -65,9 +65,9 @@ OPTIONS
 create [options] <file> <git-rev-list-args>::
 	Used to create a bundle named 'file'.  This requires the
 	'<git-rev-list-args>' arguments to define the bundle contents.
 	'options' contains the options specific to the 'git bundle create'
-	subcommand.
+	subcommand. If 'file' is `-`, the bundle is written to stdout.
 
 verify <file>::
 	Used to check that a bundle file is valid and will apply
 	cleanly to the current repository.  This includes checks on the

> > but I can put
> > it together in the next day or three.
> 
> Thanks.  Just for reference, here is what I have (just a log
> message, the patch is the same and does not support input yet).

I don't mind supporting "-" for input, but I don't think it's strictly
necessary and nobody is really asking for it. I'm also not sure it won't
run afoul of problems in the lower-level code. I seem to recall that the
bundle code may want to seek() on read, but a quick grep doesn't seem to
turn anything up (so I'm not sure if I'm mis-remembering or just didn't
look hard enough).

> ----- >8 -----
> Subject: [PATCH] bundle: don't blindly apply prefix_filename() to "-"

Thanks.

-Peff

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

[Jeff King]

On Fri, Mar 03, 2023 at 08:28:36PM -0500, Jeff King wrote:

> > Given that the other three subcommands also take <file>
> > 
> >     'git bundle' create [-q | --quiet | --progress | --all-progress] ...
> >                         [--version=<version>] <file> <git-rev-list-args>
> >     'git bundle' verify [-q | --quiet] <file>
> >     'git bundle' list-heads <file> [<refname>...]
> >     'git bundle' unbundle [--progress] <file> [<refname>...]
> > 
> > but read_bundle_header() function all three calls begins like so:
> > 
> >     int read_bundle_header(const char *path, struct bundle_header *header)
> >     {
> >             int fd = open(path, O_RDONLY);
> > 
> >             if (fd < 0)
> >                     return error(_("could not open '%s'"), path);
> >             return read_bundle_header_fd(fd, header, path);
> >     }
> > 
> > this function needs to be fixed first ;-)
> 
> I wasn't thinking of changing the behavior for input, but just focusing
> the docs in the right spot (the "create" option), like:

Oh, hmph. I didn't realize that both my patch and yours are touching a
shared options-parser that affects both reading and writing. So the
patch by itself is fixing "git bundle create -" but breaking "git bundle
verify -". We either need to teach the reading side to handle "-", or we
have to teach parse_options_cmd_bundle() to handle the two cases
differently.

-Peff

[PATCH 0/5] handling "-" as stdin/stdout in git bundle

[Jeff King]

On Fri, Mar 03, 2023 at 08:46:24PM -0500, Jeff King wrote:

> > I wasn't thinking of changing the behavior for input, but just focusing
> > the docs in the right spot (the "create" option), like:
> 
> Oh, hmph. I didn't realize that both my patch and yours are touching a
> shared options-parser that affects both reading and writing. So the
> patch by itself is fixing "git bundle create -" but breaking "git bundle
> verify -". We either need to teach the reading side to handle "-", or we
> have to teach parse_options_cmd_bundle() to handle the two cases
> differently.

So here's a potential series. It does teach "-" to the reading side.
Which is technically a behavior change, but one that I hope people will
find pretty reasonable.

The patches are:

  [1/5]: bundle: let "-" mean stdin for reading operations

    Does what it says. I did it as a preparatory patch before yours, so
    that there's never a state where we are treating "-" as special for
    prefixing but it doesn't actually work yet. :-/

  [2/5]: bundle: document handling of "-" as stdin

    One half of the doc-fixes discussed earlier, but of course covering
    all operations now.

  [3/5]: bundle: don't blindly apply prefix_filename() to "-"

    Your patch, but rebased and with the comment above fix_filename()
    dropped (since it's handled below). I saw you prepared yours on
    "maint". I did this one on "master", because some of the
    leak-handling in the other patches needs it. Since this has been
    broken for ages, and since "master" is about to become "maint" when
    2.40 release in a few days, it seemed simpler to just wait. But if
    we really want to, I think we could reorder and split it into two
    topics.

  [4/5]: parse-options: consistently allocate memory in fix_filename()

    Leak-fixes that also get us ready for using your new helper. :)

  [5/5]: parse-options: use prefix_filename_except_for_dash() helper

    And using the new helper. This could arguably be squashed into 4,
    but I wasn't sure at the outset what order to do it in (another
    option is putting 4 earlier, and then just converting it over in
    your patch 3).

 Documentation/git-bundle.txt        |  8 +++++---
 abspath.c                           |  7 +++++++
 builtin/archive.c                   |  3 ++-
 builtin/bundle.c                    | 28 +++++++++++++++++++++++-----
 builtin/checkout.c                  |  3 ++-
 builtin/reset.c                     |  4 +++-
 builtin/tag.c                       |  4 +++-
 cache.h                             |  3 +++
 parse-options.c                     | 12 ++++++------
 t/helper/test-parse-pathspec-file.c |  3 ++-
 t/t6020-bundle-misc.sh              | 26 ++++++++++++++++++++++++++
 11 files changed, 82 insertions(+), 19 deletions(-)

-Peff

[PATCH 1/5] bundle: let "-" mean stdin for reading operations

[Jeff King]

For writing, "bundle create -" indicates that the bundle should be
written to stdout. But there's no matching handling of "-" for reading
operations. This is inconsistent, and a little inflexible (though one
can always use "/dev/stdin" on systems that support it).

However, it's easy to change. Once upon a time, the bundle-reading code
required a seekable descriptor, but that was fixed long ago in
e9ee84cf28b (bundle: allowing to read from an unseekable fd,
2011-10-13). So we just need to handle "-" explicitly when opening the
file.

We _could_ do this by handling "-" in read_bundle_header(), which the
reading functions all call already. But that is probably a bad idea.
It's also used by low-level code like the transport functions, and we
may want to be more careful there. We do not know that stdin is even
available to us, and certainly we would not want to get confused by a
configured URL that happens to point to "-".

So instead, let's add a helper to builtin/bundle.c. Since both the
bundle code and some of the callers refer to the bundle by name for
error messages, let's use the string "<stdin>" to make the output a bit
nicer to read.

Signed-off-by: Jeff King <peff@peff.net>
---
It occurs to me while sending this that even though the new tests check
stdin, they don't confirm that it works with an unseekable descriptor.
It does. I don't know if the lack of fseek/lseek calls gives us enough
confidence, or if we want to protect this with a test that does:

  cat some.bundle | git bundle unbundle -

 builtin/bundle.c       | 26 ++++++++++++++++++++++----
 t/t6020-bundle-misc.sh | 15 +++++++++++++++
 2 files changed, 37 insertions(+), 4 deletions(-)

diff --git a/builtin/bundle.c b/builtin/bundle.c
index acceef62001..02dab1cfa02 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -107,6 +107,23 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 	return ret;
 }
 
+/*
+ * Similar to read_bundle_header(), but handle "-" as stdin.
+ */
+static int open_bundle(const char *path, struct bundle_header *header,
+		       const char **name)
+{
+	if (!strcmp(path, "-")) {
+		if (name)
+			*name = "<stdin>";
+		return read_bundle_header_fd(0, header, "<stdin>");
+	}
+
+	if (name)
+		*name = path;
+	return read_bundle_header(path, header);
+}
+
 static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 	struct bundle_header header = BUNDLE_HEADER_INIT;
 	int bundle_fd = -1;
@@ -118,12 +135,13 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 		OPT_END()
 	};
 	char *bundle_file;
+	const char *name;
 
 	argc = parse_options_cmd_bundle(argc, argv, prefix,
 			builtin_bundle_verify_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+	if ((bundle_fd = open_bundle(bundle_file, &header, &name)) < 0) {
 		ret = 1;
 		goto cleanup;
 	}
@@ -134,7 +152,7 @@ static int cmd_bundle_verify(int argc, const char **argv, const char *prefix) {
 		goto cleanup;
 	}
 
-	fprintf(stderr, _("%s is okay\n"), bundle_file);
+	fprintf(stderr, _("%s is okay\n"), name);
 	ret = 0;
 cleanup:
 	free(bundle_file);
@@ -155,7 +173,7 @@ static int cmd_bundle_list_heads(int argc, const char **argv, const char *prefix
 			builtin_bundle_list_heads_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+	if ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) {
 		ret = 1;
 		goto cleanup;
 	}
@@ -185,7 +203,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix)
 			builtin_bundle_unbundle_usage, options, &bundle_file);
 	/* bundle internals use argv[1] as further parameters */
 
-	if ((bundle_fd = read_bundle_header(bundle_file, &header)) < 0) {
+	if ((bundle_fd = open_bundle(bundle_file, &header, NULL)) < 0) {
 		ret = 1;
 		goto cleanup;
 	}
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 7d40994991e..45d93588c91 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -606,4 +606,19 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
 	)
 '
 
+test_expect_success 'read bundle over stdin' '
+	git bundle create some.bundle HEAD &&
+
+	git bundle verify - <some.bundle 2>err &&
+	grep "<stdin> is okay" err &&
+
+	git bundle list-heads some.bundle >expect &&
+	git bundle list-heads - <some.bundle >actual &&
+	test_cmp expect actual &&
+
+	git bundle unbundle some.bundle >expect &&
+	git bundle unbundle - <some.bundle >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.40.0.rc1.500.g967c04631e

[PATCH 2/5] bundle: document handling of "-" as stdin

[Jeff King]

We have always allowed "bundle create -" to write to stdout, but it was
never documented. And a recent patch let reading operations like "bundle
list-heads -" read from stdin.

Let's document all of these cases.

Signed-off-by: Jeff King <peff@peff.net>
---
Arguably could be squashed into the previous patch, but the bit for
"create" is weird then.

 Documentation/git-bundle.txt | 8 +++++---
 1 file changed, 5 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index 18a022b4b40..d19f4cf2b3c 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt
@@ -66,7 +66,7 @@ create [options] <file> <git-rev-list-args>::
 	Used to create a bundle named 'file'.  This requires the
 	'<git-rev-list-args>' arguments to define the bundle contents.
 	'options' contains the options specific to the 'git bundle create'
-	subcommand.
+	subcommand. If 'file' is `-`, the bundle is written to stdout.
 
 verify <file>::
 	Used to check that a bundle file is valid and will apply
@@ -77,19 +77,21 @@ verify <file>::
 	Finally, information about additional capabilities, such as "object
 	filter", is printed. See "Capabilities" in linkgit:gitformat-bundle[5]
 	for more information. The exit code is zero for success, but will
-	be nonzero if the bundle file is invalid.
+	be nonzero if the bundle file is invalid. If 'file' is `-`, the
+	bundle is read from stdin.
 
 list-heads <file>::
 	Lists the references defined in the bundle.  If followed by a
 	list of references, only references matching those given are
-	printed out.
+	printed out. If 'file' is `-`, the bundle is read from stdin.
 
 unbundle <file>::
 	Passes the objects in the bundle to 'git index-pack'
 	for storage in the repository, then prints the names of all
 	defined references. If a list of references is given, only
 	references matching those in the list are printed. This command is
 	really plumbing, intended to be called only by 'git fetch'.
+	If 'file' is `-`, the bundle is read from stdin.
 
 <git-rev-list-args>::
 	A list of arguments, acceptable to 'git rev-parse' and
-- 
2.40.0.rc1.500.g967c04631e

[PATCH 3/5] bundle: don't blindly apply prefix_filename() to "-"

[Jeff King]

From: Junio C Hamano <gitster@pobox.com>

A user can specify a filename to a command from the command line,
either as the value given to a command line option, or a command
line argument.  When it is given as a relative filename, in the
user's mind, it is relative to the directory "git" was started from,
but by the time the filename is used, "git" would almost always have
chdir()'ed up to the root level of the working tree.

The given filename, if it is relative, needs to be prefixed with the
path to the current directory, and it typically is done by calling
prefix_filename() helper function.  For commands that can also take
"-" to use the standard input or the standard output, however, this
needs to be done with care.

"git bundle create" uses the next word on the command line as the
output filename, and can take "-" to mean "write to the standard
output".  It blindly called prefix_filename(), so running it in a
subdirectory did not quite work as expected.

Introduce a new helper, prefix_filename_except_for_dash(), and use
it to help "git bundle create" codepath.

Reported-by: Michael Henry
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jeff King <peff@peff.net>
---
I didn't modify the commit message. I guess we could mention that it
works for files that read, too (but that we didn't bother to test every
command).

 abspath.c              |  7 +++++++
 builtin/bundle.c       |  2 +-
 cache.h                |  3 +++
 t/t6020-bundle-misc.sh | 11 +++++++++++
 4 files changed, 22 insertions(+), 1 deletion(-)

diff --git a/abspath.c b/abspath.c
index 39e06b58486..9a81c5525be 100644
--- a/abspath.c
+++ b/abspath.c
@@ -280,3 +280,10 @@ char *prefix_filename(const char *pfx, const char *arg)
 #endif
 	return strbuf_detach(&path, NULL);
 }
+
+char *prefix_filename_except_for_dash(const char *pfx, const char *arg)
+{
+	if (!strcmp(arg, "-"))
+		return xstrdup(arg);
+	return prefix_filename(pfx, arg);
+}
diff --git a/builtin/bundle.c b/builtin/bundle.c
index 02dab1cfa02..eca39b64bf9 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -59,7 +59,7 @@ 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 = prefix_filename_except_for_dash(prefix, argv[0]);
 	return argc;
 }
 
diff --git a/cache.h b/cache.h
index 12789903e88..38867de41af 100644
--- a/cache.h
+++ b/cache.h
@@ -638,6 +638,9 @@ char *prefix_path_gently(const char *prefix, int len, int *remaining, const char
  */
 char *prefix_filename(const char *prefix, const char *path);
 
+/* Likewise, but path=="-" always yields "-" */
+char *prefix_filename_except_for_dash(const char *prefix, const char *path);
+
 int check_filename(const char *prefix, const char *name);
 void verify_filename(const char *prefix,
 		     const char *name,
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 45d93588c91..6a5877a09ef 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -621,4 +621,15 @@ test_expect_success 'read bundle over stdin' '
 	test_cmp expect actual
 '
 
+test_expect_success 'send a bundle to standard output' '
+	git bundle create - --all HEAD >bundle-one &&
+	mkdir -p down &&
+	git -C down bundle create - --all HEAD >bundle-two &&
+	git bundle verify bundle-one &&
+	git bundle verify bundle-two &&
+	git ls-remote bundle-one >expect &&
+	git ls-remote bundle-two >actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.40.0.rc1.500.g967c04631e

[PATCH 4/5] parse-options: consistently allocate memory in fix_filename()

[Jeff King]

When handling OPT_FILENAME(), we have to stick the "prefix" (if any) in
front of the filename to make up for the fact that Git has chdir()'d to
the top of the repository. We can do this with prefix_filename(), but
there are a few special cases we handle ourselves.

Unfortunately the memory allocation is inconsistent here; if we do make
it to prefix_filename(), we'll allocate a string which the caller must
free to avoid a leak. But if we hit our special cases, we'll return the
string as-is, and a caller which tries to free it will crash. So there's
no way to win.

Let's consistently allocate, so that callers can do the right thing.

There are now three cases to care about in the function (and hence a
three-armed if/else):

  1. we got a NULL input (and should leave it as NULL, though arguably
     this is the sign of a bug; let's keep the status quo for now and we
     can pick at that scab later)

  2. we hit a special case that means we leave the name intact; we
     should duplicate the string. This includes our special "-"
     matching. Prior to this patch, it also included empty prefixes and
     absolute filenames. But we can observe that prefix_filename()
     already handles these, so we don't need to detect them.

  3. everything else goes to prefix_filename()

I've dropped the "const" from the "char **file" parameter to indicate
that we're allocating, though in practice it's not really important.
This is all being shuffled through a void pointer via opt->value before
it hits code which ever looks at the string. And it's even a bit weird,
because we are really taking _in_ a const string and using the same
out-parameter for a non-const string. A better function signature would
be:

  static char *fix_filename(const char *prefix, const char *file);

but that would mean the caller dereferences the double-pointer (and the
NULL check is currently handled inside this function). So I took the
path of least-change here.

Note that we have to fix several callers in this commit, too, or we'll
break the leak-checking tests. These are "new" leaks in the sense that
they are now triggered by the test suite, but these spots have always
been leaky when Git is run in a subdirectory of the repository. I fixed
all of the cases that trigger with GIT_TEST_PASSING_SANITIZE_LEAK. There
may be others in scripts that have other leaks, but we can fix them
later along with those other leaks (and again, you _couldn't_ fix them
before this patch, so this is the necessary first step).

Signed-off-by: Jeff King <peff@peff.net>
---
I'm not sure if UNLEAK() has fallen out of favor, but I did use it in
one spot here. It's the exact case it was designed for: we don't care
about free-ing because we're returning from a builtin's main function,
and doing a free() requires an awkward "save the return value, then
free, then return" dance.

But I'm happy to change it if people are too offended by it. :)

 builtin/archive.c                   |  3 ++-
 builtin/checkout.c                  |  3 ++-
 builtin/reset.c                     |  4 +++-
 builtin/tag.c                       |  4 +++-
 parse-options.c                     | 14 ++++++++------
 t/helper/test-parse-pathspec-file.c |  3 ++-
 6 files changed, 20 insertions(+), 11 deletions(-)

diff --git a/builtin/archive.c b/builtin/archive.c
index f094390ee01..d0a583ea958 100644
--- a/builtin/archive.c
+++ b/builtin/archive.c
@@ -81,7 +81,7 @@ static int run_remote_archiver(int argc, const char **argv,
 int cmd_archive(int argc, const char **argv, const char *prefix)
 {
 	const char *exec = "git-upload-archive";
-	const char *output = NULL;
+	char *output = NULL;
 	const char *remote = NULL;
 	struct option local_opts[] = {
 		OPT_FILENAME('o', "output", &output,
@@ -106,5 +106,6 @@ int cmd_archive(int argc, const char **argv, const char *prefix)
 
 	setvbuf(stderr, NULL, _IOLBF, BUFSIZ);
 
+	UNLEAK(output);
 	return write_archive(argc, argv, prefix, the_repository, output, 0);
 }
diff --git a/builtin/checkout.c b/builtin/checkout.c
index a5155cf55c1..85f591c85f3 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -75,7 +75,7 @@ struct checkout_opts {
 	const char *ignore_unmerged_opt;
 	int ignore_unmerged;
 	int pathspec_file_nul;
-	const char *pathspec_from_file;
+	char *pathspec_from_file;
 
 	const char *new_branch;
 	const char *new_branch_force;
@@ -1876,6 +1876,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 			    options, checkout_usage, &new_branch_info);
 	branch_info_release(&new_branch_info);
 	clear_pathspec(&opts.pathspec);
+	free(opts.pathspec_from_file);
 	FREE_AND_NULL(options);
 	return ret;
 }
diff --git a/builtin/reset.c b/builtin/reset.c
index 0697fa89de2..45a3143567c 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -317,7 +317,8 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 	int reset_type = NONE, update_ref_status = 0, quiet = 0;
 	int no_refresh = 0;
 	int patch_mode = 0, pathspec_file_nul = 0, unborn;
-	const char *rev, *pathspec_from_file = NULL;
+	const char *rev;
+	char *pathspec_from_file = NULL;
 	struct object_id oid;
 	struct pathspec pathspec;
 	int intent_to_add = 0;
@@ -495,5 +496,6 @@ int cmd_reset(int argc, const char **argv, const char *prefix)
 
 cleanup:
 	clear_pathspec(&pathspec);
+	free(pathspec_from_file);
 	return update_ref_status;
 }
diff --git a/builtin/tag.c b/builtin/tag.c
index d428c45dc8d..ccefcbd716f 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -433,7 +433,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	int create_reflog = 0;
 	int annotate = 0, force = 0;
 	int cmdmode = 0, create_tag_object = 0;
-	const char *msgfile = NULL, *keyid = NULL;
+	char *msgfile = NULL;
+	const char *keyid = NULL;
 	struct msg_arg msg = { .buf = STRBUF_INIT };
 	struct ref_transaction *transaction;
 	struct strbuf err = STRBUF_INIT;
@@ -643,5 +644,6 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
 	strbuf_release(&reflog_msg);
 	strbuf_release(&msg.buf);
 	strbuf_release(&err);
+	free(msgfile);
 	return ret;
 }
diff --git a/parse-options.c b/parse-options.c
index fd4743293fc..25bae8b585b 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -59,12 +59,14 @@ static enum parse_opt_result get_arg(struct parse_opt_ctx_t *p,
 	return 0;
 }
 
-static void fix_filename(const char *prefix, const char **file)
+static void fix_filename(const char *prefix, char **file)
 {
-	if (!file || !*file || !prefix || is_absolute_path(*file)
-	    || !strcmp("-", *file))
-		return;
-	*file = prefix_filename(prefix, *file);
+	if (!file || !*file)
+		; /* leave as NULL */
+	else if (!strcmp("-", *file))
+		*file = xstrdup(*file);
+	else
+		*file = prefix_filename(prefix, *file);
 }
 
 static enum parse_opt_result opt_command_mode_error(
@@ -177,7 +179,7 @@ static enum parse_opt_result get_value(struct parse_opt_ctx_t *p,
 			err = get_arg(p, opt, flags, (const char **)opt->value);
 
 		if (!err)
-			fix_filename(p->prefix, (const char **)opt->value);
+			fix_filename(p->prefix, (char **)opt->value);
 		return err;
 
 	case OPTION_CALLBACK:
diff --git a/t/helper/test-parse-pathspec-file.c b/t/helper/test-parse-pathspec-file.c
index b3e08cef4b3..71d2131fbad 100644
--- a/t/helper/test-parse-pathspec-file.c
+++ b/t/helper/test-parse-pathspec-file.c
@@ -6,7 +6,7 @@
 int cmd__parse_pathspec_file(int argc, const char **argv)
 {
 	struct pathspec pathspec;
-	const char *pathspec_from_file = NULL;
+	char *pathspec_from_file = NULL;
 	int pathspec_file_nul = 0, i;
 
 	static const char *const usage[] = {
@@ -29,5 +29,6 @@ int cmd__parse_pathspec_file(int argc, const char **argv)
 		printf("%s\n", pathspec.items[i].original);
 
 	clear_pathspec(&pathspec);
+	free(pathspec_from_file);
 	return 0;
 }
-- 
2.40.0.rc1.500.g967c04631e

[PATCH 5/5] parse-options: use prefix_filename_except_for_dash() helper

[Jeff King]

Since our fix_filename()'s only remaining special case is handling "-",
we can use the newly-minted helper function that handles this already.

Signed-off-by: Jeff King <peff@peff.net>
---
If we do eventually find that "!file || !*file" is a sign of a bug, we
could perhaps just simplify this whole function away. :)

 parse-options.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/parse-options.c b/parse-options.c
index 25bae8b585b..6dd4c090e03 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -63,10 +63,8 @@ static void fix_filename(const char *prefix, char **file)
 {
 	if (!file || !*file)
 		; /* leave as NULL */
-	else if (!strcmp("-", *file))
-		*file = xstrdup(*file);
 	else
-		*file = prefix_filename(prefix, *file);
+		*file = prefix_filename_except_for_dash(prefix, *file);
 }
 
 static enum parse_opt_result opt_command_mode_error(
-- 
2.40.0.rc1.500.g967c04631e

[RFC/PATCH] bundle: turn on --all-progress-implied by default

[Jeff King]

On Sat, Mar 04, 2023 at 05:22:40AM -0500, Jeff King wrote:

>   [2/5]: bundle: document handling of "-" as stdin
> 
>     One half of the doc-fixes discussed earlier, but of course covering
>     all operations now.

And here's the other one. I split it out into its own series because
it's quite a bit more complicated, and there's no textual dependency.

I'm marking it as "RFC" because it's undoing some of what Robin (cc'd)
did in the commit referenced below. But I remain unconvinced that it's a
useful direction.

-- >8 --
Subject: bundle: turn on --all-progress-implied by default

In 79862b6b77c (bundle-create: progress output control, 2019-11-10),
"bundle create" learned about the --all-progress and
--all-progress-implied options, which were copied from pack-objects.
I think these were a mistake.

In pack-objects, "all-progress-implied" is about switching the behavior
between a regular on-disk "git repack" and the use of pack-objects for
push/fetch (where a fetch does not want progress from the server during
the write stage; the client will print progress as it receives the
data). But there's no such distinction for bundles. Prior to
79862b6b77c, we always printed the write stage. Afterwards, a vanilla:

  git bundle create foo.bundle

omits the write progress, appearing to hang (especially if your
repository is large or your disk is slow). That seems like a regression.

It's possible that the flexibility to disable the write-phase progress
_could_ be useful for bundle. E.g., if you did something like:

  ssh some-host git bundle create foo.bundle |
  git bundle unbundle

But if you are running both in real-time, why are you using bundles in
the first place? You're better off doing a real fetch.

But even if we did want to support that, it should be the exception, and
vanilla "bundle create" should display the full progress. So we'd want
to name the option "--no-write-progress" or something.

The "--all-progress" option itself is even worse. It exists in
pack-objects only for historical reasons. It's a mistake because it
implies "--progress", and we added "--all-progress-implied" to fix that.
There is no reason to propagate that mistake to new commands.

Likewise, the documentation for these options was pulled from
pack-objects. But it doesn't make any sense in this context. It talks
about "--stdout", but that is not even an option that git-bundle
supports.

This patch flips the default for "--all-progress-implied" back to
"true", fixing the regression in 79862b6b77c. This turns that option
into a noop, and means that "--all-progress" is really the same as
"--progress". We _could_ drop them completely, but since they've been
shipped with Git since v2.25.0, it's polite to continue accepting them.

I didn't implement any sort of "--no-write-progress" here. I'm not at
all convinced it's necessary, and the discussion from the original
thread:

  https://lore.kernel.org/git/20191110204126.30553-2-robbat2@gentoo.org/

shows that that the main focus was on getting --progress and --quiet
support, and not any kind of clever "real-time bundle over the network"
feature. But technically this patch is making it impossible to do
something that you _could_ do post-79862b6b77c.

Signed-off-by: Jeff King <peff@peff.net>
---
I'm actually kind of confused about what happened with 79862b6b77c. If
you go to the subthread linked above, you can see that I made similar
arguments there, and Junio seemed to agree. Then the next thing I could
find is the series appearing in What's cooking:

  https://lore.kernel.org/git/xmqqftikxs4z.fsf@gitster-ct.c.googlers.com/

marked as "will merge to master". Is there more discussion I couldn't
find? Was it accidentally graduated?

 Documentation/git-bundle.txt | 18 +-----------------
 builtin/bundle.c             | 15 ++++++++-------
 t/t6020-bundle-misc.sh       |  6 ++++++
 3 files changed, 15 insertions(+), 24 deletions(-)

diff --git a/Documentation/git-bundle.txt b/Documentation/git-bundle.txt
index 18a022b4b40..a4227b6561d 100644
--- a/Documentation/git-bundle.txt
+++ b/Documentation/git-bundle.txt
@@ -9,7 +9,7 @@ git-bundle - Move objects and refs by archive
 SYNOPSIS
 --------
 [verse]
-'git bundle' create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]
+'git bundle' create [-q | --quiet | --progress]
 		    [--version=<version>] <file> <git-rev-list-args>
 'git bundle' verify [-q | --quiet] <file>
 'git bundle' list-heads <file> [<refname>...]
@@ -115,22 +115,6 @@ unbundle <file>::
 	is specified. This flag forces progress status even if
 	the standard error stream is not directed to a terminal.
 
---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.
-
---all-progress-implied::
-	This is used to imply --all-progress whenever progress display
-	is activated.  Unlike --all-progress this flag doesn't actually
-	force any progress display by itself.
-
 --version=<version>::
 	Specify the bundle version.  Version 2 is the older format and can only be
 	used with SHA-1 repositories; the newer version 3 contains capabilities that
diff --git a/builtin/bundle.c b/builtin/bundle.c
index acceef62001..c2b916e027a 100644
--- a/builtin/bundle.c
+++ b/builtin/bundle.c
@@ -12,7 +12,7 @@
  */
 
 #define BUILTIN_BUNDLE_CREATE_USAGE \
-	N_("git bundle create [-q | --quiet | --progress | --all-progress] [--all-progress-implied]\n" \
+	N_("git bundle create [-q | --quiet | --progress]\n" \
 	   "                  [--version=<version>] <file> <git-rev-list-args>")
 #define BUILTIN_BUNDLE_VERIFY_USAGE \
 	N_("git bundle verify [-q | --quiet] <file>")
@@ -64,7 +64,7 @@ static int parse_options_cmd_bundle(int argc,
 }
 
 static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
-	int all_progress_implied = 0;
+	int all_progress_implied = 1;
 	int progress = isatty(STDERR_FILENO);
 	struct strvec pack_opts;
 	int version = -1;
@@ -74,11 +74,12 @@ static int cmd_bundle_create(int argc, const char **argv, const char *prefix) {
 			    N_("do not show progress meter"), 0),
 		OPT_SET_INT(0, "progress", &progress,
 			    N_("show progress meter"), 1),
-		OPT_SET_INT(0, "all-progress", &progress,
-			    N_("show progress meter during object writing phase"), 2),
-		OPT_BOOL(0, "all-progress-implied",
-			 &all_progress_implied,
-			 N_("similar to --all-progress when progress meter is shown")),
+		OPT_SET_INT_F(0, "all-progress", &progress,
+			      N_("historical; same as --progress"), 2,
+			      PARSE_OPT_HIDDEN),
+		OPT_HIDDEN_BOOL(0, "all-progress-implied",
+				&all_progress_implied,
+				N_("historical; does nothing")),
 		OPT_INTEGER(0, "version", &version,
 			    N_("specify bundle format version")),
 		OPT_END()
diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 7d40994991e..978c5b17ba5 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -606,4 +606,10 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
 	)
 '
 
+test_expect_success 'bundle progress includes write phase' '
+	GIT_PROGRESS_DELAY=0 \
+		git bundle create --progress out.bundle --all 2>err &&
+	grep 'Writing' err
+'
+
 test_done
-- 
2.40.0.rc1.500.g967c04631e

Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default

[Robin H. Johnson]

[-- Attachment #1: Type: text/plain, Size: 1335 bytes --]

On Sat, Mar 04, 2023 at 05:55:13AM -0500, Jeff King wrote:
...
> I'm marking it as "RFC" because it's undoing some of what Robin (cc'd)
> did in the commit referenced below. But I remain unconvinced that it's a
> useful direction.
> -- >8 --
> Subject: bundle: turn on --all-progress-implied by default
(snip)

Signed-off-by: Robin H. Johnson <robbat2@gentoo.org>

+1 to at least roll it into a RC.

IIRC this was a mechanical mapping to expose the pack options into the
bundle command; and a cleanup is worthwhile.

(snip)
> diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
> index 7d40994991e..978c5b17ba5 100755
> --- a/t/t6020-bundle-misc.sh
> +++ b/t/t6020-bundle-misc.sh
> @@ -606,4 +606,10 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
>  	)
>  '
>  
> +test_expect_success 'bundle progress includes write phase' '
> +	GIT_PROGRESS_DELAY=0 \
> +		git bundle create --progress out.bundle --all 2>err &&
> +	grep 'Writing' err
> +'
> +
>  test_done

Suggestion: How about adding a test for --quiet that ensures no other
output?

-- 
Robin Hugh Johnson
Gentoo Linux: Dev, Infra Lead, Foundation Treasurer
E-Mail   : robbat2@gentoo.org
GnuPG FP : 11ACBA4F 4778E3F6 E4EDF38E B27B944E 34884E85
GnuPG FP : 7D0B3CEB E9B85B1F 825BCECF EE05E6F6 A48F6136

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 1113 bytes --]

Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default

[Jeff King]

On Mon, Mar 06, 2023 at 03:44:11AM +0000, Robin H. Johnson wrote:

> > diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
> > index 7d40994991e..978c5b17ba5 100755
> > --- a/t/t6020-bundle-misc.sh
> > +++ b/t/t6020-bundle-misc.sh
> > @@ -606,4 +606,10 @@ test_expect_success 'verify catches unreachable, broken prerequisites' '
> >  	)
> >  '
> >  
> > +test_expect_success 'bundle progress includes write phase' '
> > +	GIT_PROGRESS_DELAY=0 \
> > +		git bundle create --progress out.bundle --all 2>err &&
> > +	grep 'Writing' err
> > +'
> > +
> >  test_done
> 
> Suggestion: How about adding a test for --quiet that ensures no other
> output?

I had sort of assumed that we had those already, from the earlier work,
but it looks like we don't. Squashing this in would do it, I think (we
need test_terminal since otherwise we'd be quiet anyway):

diff --git a/t/t6020-bundle-misc.sh b/t/t6020-bundle-misc.sh
index 978c5b17ba5..7bbb351b7be 100755
--- a/t/t6020-bundle-misc.sh
+++ b/t/t6020-bundle-misc.sh
@@ -10,6 +10,7 @@ export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
 . ./test-lib.sh
 . "$TEST_DIRECTORY"/lib-bundle.sh
+. "$TEST_DIRECTORY"/lib-terminal.sh
 
 for cmd in create verify list-heads unbundle
 do
@@ -612,4 +613,10 @@ test_expect_success 'bundle progress includes write phase' '
 	grep 'Writing' err
 '
 
+test_expect_success TTY '--quiet disables all bundle progress' '
+	test_terminal env GIT_PROGRESS_DELAY=0 \
+		git bundle --quiet create out.bundle --all 2>err &&
+	test_must_be_empty err
+'
+
 test_done

Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default

[Jeff King]

On Mon, Mar 06, 2023 at 12:38:38AM -0500, Jeff King wrote:

> @@ -612,4 +613,10 @@ test_expect_success 'bundle progress includes write phase' '
>  	grep 'Writing' err
>  '
>  
> +test_expect_success TTY '--quiet disables all bundle progress' '
> +	test_terminal env GIT_PROGRESS_DELAY=0 \
> +		git bundle --quiet create out.bundle --all 2>err &&
> +	test_must_be_empty err
> +'

Err, this should be "create --quiet", of course, not the other way
around. I did run it before sending, but I removed the "--quiet" to
double-check that it generates progress with test_terminal. But then
added it back in to the wrong spot. :-/

-Peff

Re: [RFC/PATCH] bundle: turn on --all-progress-implied by default

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm actually kind of confused about what happened with 79862b6b77c. If
> you go to the subthread linked above, you can see that I made similar
> arguments there, and Junio seemed to agree. Then the next thing I could
> find is the series appearing in What's cooking:
>
>   https://lore.kernel.org/git/xmqqftikxs4z.fsf@gitster-ct.c.googlers.com/
>
> marked as "will merge to master". Is there more discussion I couldn't
> find? Was it accidentally graduated?

I am stumped as well X-<.  It does look like it was merged down by
accident.  Sorry for the confusion.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Oh, hmph. I didn't realize that both my patch and yours are touching a
> shared options-parser that affects both reading and writing. So the
> patch by itself is fixing "git bundle create -" but breaking "git bundle
> verify -". We either need to teach the reading side to handle "-", or we
> have to teach parse_options_cmd_bundle() to handle the two cases
> differently.

Yes.  Teaching that "-" is to read from the standard input to
consumers would be a natural thing to do, with the well-understood
escape hatch to use "./-" if the user really means a file.

Thanks.

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> (I do still think we don't want to push it down into prefix_filename(),
> because it gets used for paths and pathspecs given raw on the command
> line. It does make me wonder if there are places where OPT_FILENAME() is
> doing the wrong thing).

To be quite honest, I had the opposite reaction ;-)  At least for
OPT_FILENAME() thing, I think it is well known that you should work
around with "git cmd --opt ./-" if you do mean a file whose name
happens to be a single dash.  Teaching prefix_filename() the same
trick does not look _too_ bad.

There is a problem that commands that use prefix_filename() may not
be prepared to read from the standard input or write to the standard
output.  For some such callers it may be just the matter of
replacing an unconditional open() with

	-	fd = open(filename, O_RDONLY);
	+	if (!strcmp(filename, "-"))
	+		fd = 0;
	+	else
	+		fd = open(filename, O_RDONLY);

or something, but if some callers have fundamental reasons why they
do not want to work with the standard input, it may make sense to
treat "-" as a normal filename, and for them, blindly prefixing the
leading directory name would be much better than special casing "-".

So, I dunno.

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

[Jeff King]

On Fri, Mar 03, 2023 at 05:14:37PM -0800, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > (I do still think we don't want to push it down into prefix_filename(),
> > because it gets used for paths and pathspecs given raw on the command
> > line. It does make me wonder if there are places where OPT_FILENAME() is
> > doing the wrong thing).
> 
> To be quite honest, I had the opposite reaction ;-)  At least for
> OPT_FILENAME() thing, I think it is well known that you should work
> around with "git cmd --opt ./-" if you do mean a file whose name
> happens to be a single dash.  Teaching prefix_filename() the same
> trick does not look _too_ bad.

For OPT_FILENAME() I agree you are generally thinking about "-" as
special anyway. But if it's the argument to an option, you do not
generally need to prefix it.

After a little grepping, here's a real world example where we wouldn't
want prefix_filename() to treat "-" specially:

  git archive --add-virtual-file=-:1234abcd

Using "stdin" there doesn't make sense. It's OK because it's not using
OPT_FILENAME(), but it is using prefix_filename().

But I'd worry much more about non-option arguments. There you _do_ need
to say "./-" if you are intermingled with options. But a "--" separator
splits that. So something like:

  git mv -- - to

should move the literal file "-", and it _should_ be prefixed with the
subdirectory if there is one. Having it mean stdin makes no sense there,
as we are not reading or writing the file, but rather operating on the
path.

Now this particular case isn't a problem, because it uses
internal_prefix_pathspec(), and not prefix_filename(). And maybe most
things are in that boat. Grepping around, most spots I see using
prefix_filename() could reasonably handle "-" as stdin/stdout, though
there are definitely some outliers.  E.g., "git format-patch -o" wants a
directory, and "git merge-file" probably doesn't want to handle stdin.

So I think we probably do want to stick with "-" as being not-special by
default for prefix_filename(), and individual sites would opt into it.

> There is a problem that commands that use prefix_filename() may not
> be prepared to read from the standard input or write to the standard
> output.  For some such callers it may be just the matter of
> replacing an unconditional open() with
> 
> 	-	fd = open(filename, O_RDONLY);
> 	+	if (!strcmp(filename, "-"))
> 	+		fd = 0;
> 	+	else
> 	+		fd = open(filename, O_RDONLY);
> 
> or something, but if some callers have fundamental reasons why they
> do not want to work with the standard input, it may make sense to
> treat "-" as a normal filename, and for them, blindly prefixing the
> leading directory name would be much better than special casing "-".

Right. I think we should stick with the status quo and change over
individual callers if people want that effect (but I do not really hear
anybody clamoring for it, so it seems very non-urgent).

-Peff

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

[Michael Henry]

On 3/3/23 17:31, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
Jeff,

I'm not subscribed to the git mailing list (I thought it might
be too high volume for me), but I thought I'd be getting any
replies regarding my bug report.  For some reason, though,  I'm
not getting copied on any of the emails you've sent (from
peff@peff.net).  I am getting Junio's emails, but today was the
first time I'd seen a response.  I am able to see the
conversation via the archive at
https://marc.info/?l=git&m=167733032407411&w=2, so I've read
your responses there.

It looks like Junio is also making fast progress on this, and
I'm happy stand back and watch the magic happen this time.

Thanks,
Michael Henry

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

[Jeff King]

On Fri, Mar 03, 2023 at 06:59:14PM -0500, Michael Henry wrote:

> I'm not subscribed to the git mailing list (I thought it might
> be too high volume for me), but I thought I'd be getting any
> replies regarding my bug report.  For some reason, though,  I'm
> not getting copied on any of the emails you've sent (from
> peff@peff.net).  I am getting Junio's emails, but today was the
> first time I'd seen a response.  I am able to see the
> conversation via the archive at
> https://marc.info/?l=git&m=167733032407411&w=2, so I've read
> your responses there.

Ah, thanks for letting me know. They seem to be stuck in my mail
server's queue. The problem is that your server supports TLS, but only
TLS v1.0. Recent versions of openssl (at least packaged for Debian) bump
the default to TLS v1.2 (because older versions are considered less
secure).

I relaxed the settings on my server, so you should have gotten a flurry
of deliveries (including this one). But you might run into delivery
problems from other people at some point.

-Peff

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

[Michael Henry]

On 3/3/23 21:22, Jeff King wrote:
> The problem is that your server supports TLS, but only TLS
> v1.0.

That's good information; I hadn't realized that about my email
provider.

> I relaxed the settings on my server, so you should have gotten
> a flurry of deliveries (including this one). But you might run
> into delivery problems from other people at some point.

Thanks very much for the pointer; I'll definitely look into
getting this fixed.  This is an unexpected side benefit of
posting to the Git mailing list :-)

Michael Henry