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

From: Jeff King <peff@peff.net>
Date: Mar 4, 2023, 01:43 UTC
Message-ID: <ZAKiV9nAet4wlEEu@coredump.intra.peff.net>
In-Reply-To: <xmqqa60tco76.fsf@gitster.g>

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

Show 12 quoted lines

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

Show 15 quoted lines

> 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