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

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]

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

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:

Jeff King <peff@peff.net> writes:

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().

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

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

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

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

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

Jeff King <peff@peff.net> writes:

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 ;-).

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(-)

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

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

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

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

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

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

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

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

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(-)

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(-)

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(-)

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(-)

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(-)

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

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(-)

On Sat, Mar 04, 2023 at 05:55:13AM -0500, Jeff King wrote: ...

(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)

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

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

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):

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

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

Jeff King <peff@peff.net> writes:

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

Jeff King <peff@peff.net> writes:

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.

Jeff King <peff@peff.net> writes:

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.

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

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.

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

On 3/3/23 17:31, Junio C Hamano wrote:

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

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

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

On 3/3/23 21:22, Jeff King wrote:

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

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