| Message ID | 07a7c34615ec68fa42c725fd34d6144b6b191f03.1685710884.git.ps@pks.im (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | cat-file: introduce NUL-terminated output format | expand |
Hi Patrick On 02/06/2023 14:02, Patrick Steinhardt wrote: > In db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with > `-z`, 2022-07-22), we have introduced a new mode to read the input via > NUL-delimited records instead of newline-delimited records. This allows > the user to query for revisions that have newlines in their path > component. While unusual, such queries are perfectly valid and thus it > is clear that we should be able to support them properly. > > Unfortunately, the commit only changed the input to be NUL-delimited, > but didn't change the output at the same time. While this is fine for > queries that are processed successfully, it is less so for queries that > aren't. In the case of missing commits for example the result can become > entirely unparsable: > > ``` > $ printf "7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\n1234567890\n\n\commit000" | > git cat-file --batch -z > 7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10 > 1234567890 > > commit missing > ``` > > This is of course a crafted query that is intentionally gaming the > deficiency, but more benign queries that contain newlines would have > similar problems. > > Ideally, we should have also changed the output to be NUL-delimited when > `-z` is specified to avoid this problem. As the input is NUL-delimited, > it is clear that the output in this case cannot ever contain NUL > characters by itself. Furthermore, Git does not allow NUL characters in > revisions anyway, further stressing the point that using NUL-delimited > output is safe. The only exception is of course the object data itself, > but as git-cat-file(1) prints the size of the object data clients should > read until that specified size has been consumed. > > But even though `-z` has only been introduced a few releases ago in Git > v2.38.0, changing the output format retroactively to also NUL-delimit > output would be a backwards incompatible change. And while one could > make the argument that the output is inherently broken already, we need > to assume that there are existing users out there that use it just fine > given that revisions containing newlines are quite exotic. > > Instead, introduce a new option `-Z` that switches to NUL-delimited > input and output. The old `-z` option is marked as deprecated with a > hint that its output may become unparsable. The commit message explains the problem well, I agree adding a new option is the cleanest solution. > Co-authored-by: Toon Claes <toon@iotcl.com> > Signed-off-by: Patrick Steinhardt <ps@pks.im> > --- > Documentation/git-cat-file.txt | 13 +++- > builtin/cat-file.c | 55 +++++++++------ > t/t1006-cat-file.sh | 123 ++++++++++++++++++++++++--------- > 3 files changed, 137 insertions(+), 54 deletions(-) > > diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt > index 411de2e27d..b1f48fdfb1 100644 > --- a/Documentation/git-cat-file.txt > +++ b/Documentation/git-cat-file.txt > @@ -14,7 +14,7 @@ SYNOPSIS > 'git cat-file' (-t | -s) [--allow-unknown-type] <object> > 'git cat-file' (--batch | --batch-check | --batch-command) [--batch-all-objects] > [--buffer] [--follow-symlinks] [--unordered] > - [--textconv | --filters] [-z] > + [--textconv | --filters] [-z] [-Z] > 'git cat-file' (--textconv | --filters) > [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>] > > @@ -246,6 +246,12 @@ respectively print: > -z:: > Only meaningful with `--batch`, `--batch-check`, or > `--batch-command`; input is NUL-delimited instead of > + newline-delimited. This option is deprecated in favor of > + `-Z` as the output can otherwise be ambiguous. > + > +-Z:: > + Only meaningful with `--batch`, `--batch-check`, or > + `--batch-command`; input and output is NUL-delimited instead of > newline-delimited. The documentation changes look good. I wonder if we should put the documentation for "-Z" above "-z" so users see the preferred option first. > > @@ -384,6 +390,11 @@ notdir SP <size> LF > is printed when, during symlink resolution, a file is used as a > directory name. > > +Alternatively, when `-Z` is passed, the line feeds in any of the above examples > +are replaced with NUL terminators. This ensures that output will be parsable if > +the output itself would contain a linefeed and is thus recommended for > +scripting purposes. > + > CAVEATS > ------- > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > index 001dcb24d6..90ef407d30 100644 > --- a/builtin/cat-file.c > +++ b/builtin/cat-file.c > @@ -492,17 +494,18 @@ static void batch_object_write(const char *obj_name, > strbuf_reset(scratch); > > if (!opt->format) { > - print_default_format(scratch, data); > + print_default_format(scratch, data, opt); > } else { > strbuf_expand(scratch, opt->format, expand_format, data); > - strbuf_addch(scratch, '\n'); > + strbuf_addch(scratch, opt->output_delim); > } > > batch_write(opt, scratch->buf, scratch->len); > > if (opt->batch_mode == BATCH_MODE_CONTENTS) { > + char buf[] = {opt->output_delim}; I found this a bit confusing, I think it would be clearer just to do batch_write(opt, &opt->output_delim, 1); > print_object_or_die(opt, data); > - batch_write(opt, "\n", 1); > + batch_write(opt, buf, 1); > } > } > @@ -920,7 +927,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) > N_("git cat-file (-t | -s) [--allow-unknown-type] <object>"), > N_("git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\n" > " [--buffer] [--follow-symlinks] [--unordered]\n" > - " [--textconv | --filters] [-z]"), > + " [--textconv | --filters] [-z] [-Z]"), If we're recommending that people don't use '-z' then maybe we should remove it from the synopsis and add OPT_HIDDEN to it below. > N_("git cat-file (--textconv | --filters)\n" > " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"), > NULL > @@ -950,6 +957,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) > PARSE_OPT_OPTARG | PARSE_OPT_NONEG, > batch_option_callback), > OPT_BOOL('z', NULL, &input_nul_terminated, N_("stdin is NUL-terminated")), > + OPT_BOOL('Z', NULL, &nul_terminated, N_("stdin and stdout is NUL-terminated")), > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh > index 7b985cfded..d73a0be1b9 100755 > --- a/t/t1006-cat-file.sh > +++ b/t/t1006-cat-file.sh > @@ -392,17 +393,18 @@ deadbeef > > " > > -batch_output="$hello_sha1 blob $hello_size > -$hello_content > -$commit_sha1 commit $commit_size > -$commit_content > -$tag_sha1 tag $tag_size > -$tag_content > -deadbeef missing > - missing" > +printf "%s\0" \ > + "$hello_sha1 blob $hello_size" \ > + "$hello_content" \ > + "$commit_sha1 commit $commit_size" \ > + "$commit_content" \ > + "$tag_sha1 tag $tag_size" \ > + "$tag_content" \ > + "deadbeef missing" \ > + " missing" >batch_output I think writing the expected output to a file is a good change as we always use it with test_cmp. As "-z" is deprecated I think it makes sense to model the expected output for "-Z" and use tr for the "-z" tests as you have done here. It looks like we have good coverage of the new option. Thanks for working on this Phillip > test_expect_success '--batch with multiple sha1s gives correct format' ' > - echo "$batch_output" >expect && > + tr "\0" "\n" <batch_output >expect && > echo_without_newline "$batch_input" >in && > git cat-file --batch <in >actual && > test_cmp expect actual > @@ -410,11 +412,17 @@ test_expect_success '--batch with multiple sha1s gives correct format' ' > > test_expect_success '--batch, -z with multiple sha1s gives correct format' ' > echo_without_newline_nul "$batch_input" >in && > - echo "$batch_output" >expect && > + tr "\0" "\n" <batch_output >expect && > git cat-file --batch -z <in >actual && > test_cmp expect actual > ' > > +test_expect_success '--batch, -Z with multiple sha1s gives correct format' ' > + echo_without_newline_nul "$batch_input" >in && > + git cat-file --batch -Z <in >actual && > + test_cmp batch_output actual > +' > + > batch_check_input="$hello_sha1 > $tree_sha1 > $commit_sha1 > @@ -423,40 +431,55 @@ deadbeef > > " > > -batch_check_output="$hello_sha1 blob $hello_size > -$tree_sha1 tree $tree_size > -$commit_sha1 commit $commit_size > -$tag_sha1 tag $tag_size > -deadbeef missing > - missing" > +printf "%s\0" \ > + "$hello_sha1 blob $hello_size" \ > + "$tree_sha1 tree $tree_size" \ > + "$commit_sha1 commit $commit_size" \ > + "$tag_sha1 tag $tag_size" \ > + "deadbeef missing" \ > + " missing" >batch_check_output > > test_expect_success "--batch-check with multiple sha1s gives correct format" ' > - echo "$batch_check_output" >expect && > + tr "\0" "\n" <batch_check_output >expect && > echo_without_newline "$batch_check_input" >in && > git cat-file --batch-check <in >actual && > test_cmp expect actual > ' > > test_expect_success "--batch-check, -z with multiple sha1s gives correct format" ' > - echo "$batch_check_output" >expect && > + tr "\0" "\n" <batch_check_output >expect && > echo_without_newline_nul "$batch_check_input" >in && > git cat-file --batch-check -z <in >actual && > test_cmp expect actual > ' > > -test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' ' > +test_expect_success "--batch-check, -Z with multiple sha1s gives correct format" ' > + echo_without_newline_nul "$batch_check_input" >in && > + git cat-file --batch-check -Z <in >actual && > + test_cmp batch_check_output actual > +' > + > +test_expect_success FUNNYNAMES 'setup with newline in input' ' > touch -- "newline${LF}embedded" && > git add -- "newline${LF}embedded" && > git commit -m "file with newline embedded" && > test_tick && > > - printf "HEAD:newline${LF}embedded" >in && > - git cat-file --batch-check -z <in >actual && > + printf "HEAD:newline${LF}embedded" >in > +' > > +test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' ' > + git cat-file --batch-check -z <in >actual && > echo "$(git rev-parse "HEAD:newline${LF}embedded") blob 0" >expect && > test_cmp expect actual > ' > > +test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' ' > + git cat-file --batch-check -Z <in >actual && > + printf "%s\0" "$(git rev-parse "HEAD:newline${LF}embedded") blob 0" >expect && > + test_cmp expect actual > +' > + > batch_command_multiple_info="info $hello_sha1 > info $tree_sha1 > info $commit_sha1 > @@ -480,7 +503,13 @@ test_expect_success '--batch-command with multiple info calls gives correct form > echo "$batch_command_multiple_info" | tr "\n" "\0" >in && > git cat-file --batch-command --buffer -z <in >actual && > > - test_cmp expect actual > + test_cmp expect actual && > + > + echo "$batch_command_multiple_info" | tr "\n" "\0" >in && > + tr "\n" "\0" <expect >expect_nul && > + git cat-file --batch-command --buffer -Z <in >actual && > + > + test_cmp expect_nul actual > ' > > batch_command_multiple_contents="contents $hello_sha1 > @@ -490,15 +519,15 @@ contents deadbeef > flush" > > test_expect_success '--batch-command with multiple command calls gives correct format' ' > - cat >expect <<-EOF && > - $hello_sha1 blob $hello_size > - $hello_content > - $commit_sha1 commit $commit_size > - $commit_content > - $tag_sha1 tag $tag_size > - $tag_content > - deadbeef missing > - EOF > + printf "%s\0" \ > + "$hello_sha1 blob $hello_size" \ > + "$hello_content" \ > + "$commit_sha1 commit $commit_size" \ > + "$commit_content" \ > + "$tag_sha1 tag $tag_size" \ > + "$tag_content" \ > + "deadbeef missing" >expect_nul && > + tr "\0" "\n" <expect_nul >expect && > > echo "$batch_command_multiple_contents" >in && > git cat-file --batch-command --buffer <in >actual && > @@ -508,7 +537,12 @@ test_expect_success '--batch-command with multiple command calls gives correct f > echo "$batch_command_multiple_contents" | tr "\n" "\0" >in && > git cat-file --batch-command --buffer -z <in >actual && > > - test_cmp expect actual > + test_cmp expect actual && > + > + echo "$batch_command_multiple_contents" | tr "\n" "\0" >in && > + git cat-file --batch-command --buffer -Z <in >actual && > + > + test_cmp expect_nul actual > ' > > test_expect_success 'setup blobs which are likely to delta' ' > @@ -848,6 +882,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for brok > test_cmp expect actual > ' > > +test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for broken in-repo, same-dir links' ' > + printf "HEAD:broken-same-dir-link\0" >in && > + printf "dangling 25\0HEAD:broken-same-dir-link\0" >expect && > + git cat-file --batch-check --follow-symlinks -Z <in >actual && > + test_cmp expect actual > +' > + > test_expect_success 'git cat-file --batch-check --follow-symlinks works for same-dir links-to-links' ' > echo HEAD:link-to-link | git cat-file --batch-check --follow-symlinks >actual && > test_cmp found actual > @@ -862,6 +903,15 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for pare > test_cmp expect actual > ' > > +test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for parent-dir links' ' > + echo HEAD:dir/parent-dir-link | git cat-file --batch-check --follow-symlinks >actual && > + test_cmp found actual && > + printf "notdir 29\0HEAD:dir/parent-dir-link/nope\0" >expect && > + printf "HEAD:dir/parent-dir-link/nope\0" >in && > + git cat-file --batch-check --follow-symlinks -Z <in >actual && > + test_cmp expect actual > +' > + > test_expect_success 'git cat-file --batch-check --follow-symlinks works for .. links' ' > echo dangling 22 >expect && > echo HEAD:dir/link-dir/nope >>expect && > @@ -976,6 +1026,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlink breaks loops' ' > test_cmp expect actual > ' > > +test_expect_success 'git cat-file --batch-check --follow-symlink -Z breaks loops' ' > + printf "loop 10\0HEAD:loop1\0" >expect && > + printf "HEAD:loop1\0" >in && > + git cat-file --batch-check --follow-symlinks -Z <in >actual && > + test_cmp expect actual > +' > + > test_expect_success 'git cat-file --batch --follow-symlink returns correct sha and mode' ' > echo HEAD:morx | git cat-file --batch >expect && > echo HEAD:morx | git cat-file --batch --follow-symlinks >actual &&
Phillip Wood <phillip.wood123@gmail.com> writes: >> Instead, introduce a new option `-Z` that switches to NUL-delimited >> input and output. The old `-z` option is marked as deprecated with a >> hint that its output may become unparsable. > > The commit message explains the problem well, I agree adding a new > option is the cleanest solution. > >... >> @@ -246,6 +246,12 @@ respectively print: >> -z:: >> Only meaningful with `--batch`, `--batch-check`, or >> `--batch-command`; input is NUL-delimited instead of >> + newline-delimited. This option is deprecated in favor of >> + `-Z` as the output can otherwise be ambiguous. >> + >> +-Z:: >> + Only meaningful with `--batch`, `--batch-check`, or >> + `--batch-command`; input and output is NUL-delimited instead of >> newline-delimited. > > The documentation changes look good. I wonder if we should put the > documentation for "-Z" above "-z" so users see the preferred option > first. Hmph, I expected "-z" and "-Z" to be orthogonal, the former controlling how input records are delimited, the latter controlling how output records are delimited, as it usually is a good idea to keep things that could be orthogonal to be orthogonal to avoid unnecessarily robbing users flexibility. "-Z is a new way that is preferred over -z" was something I did not expect, actually. I am not outright rejecting such a deliberately limiting design, but I'll have to think about it a bit.
Patrick Steinhardt <ps@pks.im> writes:
> Subject: Re: [PATCH 5/5] cat-file: Introduce new option to delimit output with NUL characters
If you are going to make "-Z" and "-z" not orthogonal, then this
should say
cat-file: introduce -Z option to delimit both input and output with ...
or something.
On Tue, Jun 06, 2023 at 08:54:16AM +0900, Junio C Hamano wrote: > Phillip Wood <phillip.wood123@gmail.com> writes: > > >> Instead, introduce a new option `-Z` that switches to NUL-delimited > >> input and output. The old `-z` option is marked as deprecated with a > >> hint that its output may become unparsable. > > > > The commit message explains the problem well, I agree adding a new > > option is the cleanest solution. > > > >... > >> @@ -246,6 +246,12 @@ respectively print: > >> -z:: > >> Only meaningful with `--batch`, `--batch-check`, or > >> `--batch-command`; input is NUL-delimited instead of > >> + newline-delimited. This option is deprecated in favor of > >> + `-Z` as the output can otherwise be ambiguous. > >> + > >> +-Z:: > >> + Only meaningful with `--batch`, `--batch-check`, or > >> + `--batch-command`; input and output is NUL-delimited instead of > >> newline-delimited. > > > > The documentation changes look good. I wonder if we should put the > > documentation for "-Z" above "-z" so users see the preferred option > > first. > > Hmph, I expected "-z" and "-Z" to be orthogonal, the former > controlling how input records are delimited, the latter controlling > how output records are delimited, as it usually is a good idea to > keep things that could be orthogonal to be orthogonal to avoid > unnecessarily robbing users flexibility. "-Z is a new way that is > preferred over -z" was something I did not expect, actually. > > I am not outright rejecting such a deliberately limiting design, but > I'll have to think about it a bit. Well, the way I see it we shouldn't have ever decoupled the input and output format, and I consider it a bug that `-z` did as it makes it unusable with arbitrary input. So I'd rather be helping the user of these modes to do the right thing and NUL-delimit both input and output than running into these edge cases later down the road. That being said I'd be fine to change this series to mean "-Z changes stdout" if you insist. In that case we should be pointing out in our documentation that "You should never use `-z` without `-Z` when you process arbitrary input". Patrick
On Mon, Jun 05, 2023 at 04:47:14PM +0100, Phillip Wood wrote: > > @@ -384,6 +390,11 @@ notdir SP <size> LF > > is printed when, during symlink resolution, a file is used as a > > directory name. > > > > +Alternatively, when `-Z` is passed, the line feeds in any of the above examples > > +are replaced with NUL terminators. This ensures that output will be parsable if > > +the output itself would contain a linefeed and is thus recommended for > > +scripting purposes. > > + > > CAVEATS > > ------- > > > > diff --git a/builtin/cat-file.c b/builtin/cat-file.c > > index 001dcb24d6..90ef407d30 100644 > > --- a/builtin/cat-file.c > > +++ b/builtin/cat-file.c > > @@ -492,17 +494,18 @@ static void batch_object_write(const char *obj_name, > > strbuf_reset(scratch); > > > > if (!opt->format) { > > - print_default_format(scratch, data); > > + print_default_format(scratch, data, opt); > > } else { > > strbuf_expand(scratch, opt->format, expand_format, data); > > - strbuf_addch(scratch, '\n'); > > + strbuf_addch(scratch, opt->output_delim); > > } > > > > batch_write(opt, scratch->buf, scratch->len); > > > > if (opt->batch_mode == BATCH_MODE_CONTENTS) { > > + char buf[] = {opt->output_delim}; > > I found this a bit confusing, I think it would be clearer just to do > > batch_write(opt, &opt->output_delim, 1); Agreed, that's cleaner. > > print_object_or_die(opt, data); > > - batch_write(opt, "\n", 1); > > + batch_write(opt, buf, 1); > > } > > } > > > @@ -920,7 +927,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) > > N_("git cat-file (-t | -s) [--allow-unknown-type] <object>"), > > N_("git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\n" > > " [--buffer] [--follow-symlinks] [--unordered]\n" > > - " [--textconv | --filters] [-z]"), > > + " [--textconv | --filters] [-z] [-Z]"), > > If we're recommending that people don't use '-z' then maybe we should > remove it from the synopsis and add OPT_HIDDEN to it below. I might still change this depending on the conclusion Junio and I will arrive at, but for now I agree that this makes sense. > > N_("git cat-file (--textconv | --filters)\n" > > " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"), > > NULL > > @@ -950,6 +957,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) > > PARSE_OPT_OPTARG | PARSE_OPT_NONEG, > > batch_option_callback), > > OPT_BOOL('z', NULL, &input_nul_terminated, N_("stdin is NUL-terminated")), > > + OPT_BOOL('Z', NULL, &nul_terminated, N_("stdin and stdout is NUL-terminated")), > > > diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh > > index 7b985cfded..d73a0be1b9 100755 > > --- a/t/t1006-cat-file.sh > > +++ b/t/t1006-cat-file.sh > > @@ -392,17 +393,18 @@ deadbeef > > > > " > > > > -batch_output="$hello_sha1 blob $hello_size > > -$hello_content > > -$commit_sha1 commit $commit_size > > -$commit_content > > -$tag_sha1 tag $tag_size > > -$tag_content > > -deadbeef missing > > - missing" > > +printf "%s\0" \ > > + "$hello_sha1 blob $hello_size" \ > > + "$hello_content" \ > > + "$commit_sha1 commit $commit_size" \ > > + "$commit_content" \ > > + "$tag_sha1 tag $tag_size" \ > > + "$tag_content" \ > > + "deadbeef missing" \ > > + " missing" >batch_output > > I think writing the expected output to a file is a good change as we > always use it with test_cmp. As "-z" is deprecated I think it makes > sense to model the expected output for "-Z" and use tr for the "-z" > tests as you have done here. It looks like we have good coverage of the > new option. It's actually also required in order to not have to specify the expected output twice. While we could leave this as-is, translating it to be NUL terminated via `tr \n \0` doesn't work as the output contains newlines in places where we don't want to translate them to NUL delimiters. And storing the NUL-delimited string in a variable doesn't work either as shells will truncate the C strings. Thanks for your review! Patrick
Patrick Steinhardt <ps@pks.im> writes: >> Hmph, I expected "-z" and "-Z" to be orthogonal, the former >> controlling how input records are delimited, the latter controlling >> how output records are delimited, as it usually is a good idea to >> keep things that could be orthogonal to be orthogonal to avoid >> unnecessarily robbing users flexibility. "-Z is a new way that is >> preferred over -z" was something I did not expect, actually. >> >> I am not outright rejecting such a deliberately limiting design, but >> I'll have to think about it a bit. > > Well, the way I see it we shouldn't have ever decoupled the input and > output format, and I consider it a bug that `-z` did as it makes it > unusable with arbitrary input. So I'd rather be helping the user of > these modes to do the right thing and NUL-delimit both input and output > than running into these edge cases later down the road. If that is the direction we want to go in, then the title of this step must be updated to say "-Z" does both output and input. Also with the same number of bytes it takes to say "new option", you can say what that new option is, so do that. > That being said I'd be fine to change this series to mean "-Z changes > stdout" if you insist. In that case we should be pointing out in our > documentation that "You should never use `-z` without `-Z` when you > process arbitrary input". You are not making sense. If we were to leave them orthogonal to keep flexibility, it is because there can be cases where using '-Z' without using '-z' (and vice versa) makes sense; "you should never" has no place to live in such a world.
On Tue, Jun 06, 2023 at 02:22:15PM +0900, Junio C Hamano wrote: > Patrick Steinhardt <ps@pks.im> writes: > > >> Hmph, I expected "-z" and "-Z" to be orthogonal, the former > >> controlling how input records are delimited, the latter controlling > >> how output records are delimited, as it usually is a good idea to > >> keep things that could be orthogonal to be orthogonal to avoid > >> unnecessarily robbing users flexibility. "-Z is a new way that is > >> preferred over -z" was something I did not expect, actually. > >> > >> I am not outright rejecting such a deliberately limiting design, but > >> I'll have to think about it a bit. > > > > Well, the way I see it we shouldn't have ever decoupled the input and > > output format, and I consider it a bug that `-z` did as it makes it > > unusable with arbitrary input. So I'd rather be helping the user of > > these modes to do the right thing and NUL-delimit both input and output > > than running into these edge cases later down the road. > > If that is the direction we want to go in, then the title of this > step must be updated to say "-Z" does both output and input. Also > with the same number of bytes it takes to say "new option", you can > say what that new option is, so do that. Sorry, my v2 and your mail crossed, so the new title doesn't yet mention `-Z`. I agree it would make sense to do so, and if there's a v3 I'll change it accordingly. Otherwise, please feel free to change: cat-file: introduce option to delimit input and output with NUL to cat-file: introduce `-Z` to delimit input and output with NUL > > That being said I'd be fine to change this series to mean "-Z changes > > stdout" if you insist. In that case we should be pointing out in our > > documentation that "You should never use `-z` without `-Z` when you > > process arbitrary input". > > You are not making sense. If we were to leave them orthogonal to > keep flexibility, it is because there can be cases where using '-Z' > without using '-z' (and vice versa) makes sense; "you should never" > has no place to live in such a world. Well, that's exactly what I'm arguing: I don't think it does make sense to keep them orthogonal. I cannot think of usecases where you'd want to only change either of those options once you start processing arbitrary input. If you exactly know that your input doesn't contain newlines then you don't need either `-z` nor `-Z`. On the other hand, if it does, then you must be using both. I only proposed the alternative with the intent of being diplomatic and not fully close the door on your remark in case you felt strongly about it. I've thus sent v2 to double down on my current approach where `-Z` changes both stdin and stdout and tried to make a better case for it in the commit message. Patrick
Patrick Steinhardt <ps@pks.im> writes: >> > That being said I'd be fine to change this series to mean "-Z changes >> > stdout" if you insist. In that case we should be pointing out in our >> > documentation that "You should never use `-z` without `-Z` when you >> > process arbitrary input". >> >> You are not making sense. If we were to leave them orthogonal to >> keep flexibility, it is because there can be cases where using '-Z' >> without using '-z' (and vice versa) makes sense; "you should never" >> has no place to live in such a world. > > Well, that's exactly what I'm arguing: I don't think it does make sense > to keep them orthogonal. I was not commenting on that. My "you are not making sense" was because you said "I'd be fine to make -Z to mean 'stdout is NUL delimited'". If somebody thinks it is a good idea to make '-Z' mean 'stdout is NUL delimited without affecting how the input is delimited', it can only be because that somebody thinks that it makes sense to make input and output orthogonal. It is perfectly OK if you are not fine to make -Z to affect only the standard output stream. That stance is at least self consistent.
diff --git a/Documentation/git-cat-file.txt b/Documentation/git-cat-file.txt index 411de2e27d..b1f48fdfb1 100644 --- a/Documentation/git-cat-file.txt +++ b/Documentation/git-cat-file.txt @@ -14,7 +14,7 @@ SYNOPSIS 'git cat-file' (-t | -s) [--allow-unknown-type] <object> 'git cat-file' (--batch | --batch-check | --batch-command) [--batch-all-objects] [--buffer] [--follow-symlinks] [--unordered] - [--textconv | --filters] [-z] + [--textconv | --filters] [-z] [-Z] 'git cat-file' (--textconv | --filters) [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>] @@ -246,6 +246,12 @@ respectively print: -z:: Only meaningful with `--batch`, `--batch-check`, or `--batch-command`; input is NUL-delimited instead of + newline-delimited. This option is deprecated in favor of + `-Z` as the output can otherwise be ambiguous. + +-Z:: + Only meaningful with `--batch`, `--batch-check`, or + `--batch-command`; input and output is NUL-delimited instead of newline-delimited. @@ -384,6 +390,11 @@ notdir SP <size> LF is printed when, during symlink resolution, a file is used as a directory name. +Alternatively, when `-Z` is passed, the line feeds in any of the above examples +are replaced with NUL terminators. This ensures that output will be parsable if +the output itself would contain a linefeed and is thus recommended for +scripting purposes. + CAVEATS ------- diff --git a/builtin/cat-file.c b/builtin/cat-file.c index 001dcb24d6..90ef407d30 100644 --- a/builtin/cat-file.c +++ b/builtin/cat-file.c @@ -43,6 +43,7 @@ struct batch_options { int unordered; int transform_mode; /* may be 'w' or 'c' for --filters or --textconv */ char input_delim; + char output_delim; const char *format; }; @@ -437,11 +438,12 @@ static void print_object_or_die(struct batch_options *opt, struct expand_data *d } } -static void print_default_format(struct strbuf *scratch, struct expand_data *data) +static void print_default_format(struct strbuf *scratch, struct expand_data *data, + struct batch_options *opt) { - strbuf_addf(scratch, "%s %s %"PRIuMAX"\n", oid_to_hex(&data->oid), + strbuf_addf(scratch, "%s %s %"PRIuMAX"%c", oid_to_hex(&data->oid), type_name(data->type), - (uintmax_t)data->size); + (uintmax_t)data->size, opt->output_delim); } /* @@ -470,8 +472,8 @@ static void batch_object_write(const char *obj_name, &data->oid, &data->info, OBJECT_INFO_LOOKUP_REPLACE); if (ret < 0) { - printf("%s missing\n", - obj_name ? obj_name : oid_to_hex(&data->oid)); + printf("%s missing%c", + obj_name ? obj_name : oid_to_hex(&data->oid), opt->output_delim); fflush(stdout); return; } @@ -492,17 +494,18 @@ static void batch_object_write(const char *obj_name, strbuf_reset(scratch); if (!opt->format) { - print_default_format(scratch, data); + print_default_format(scratch, data, opt); } else { strbuf_expand(scratch, opt->format, expand_format, data); - strbuf_addch(scratch, '\n'); + strbuf_addch(scratch, opt->output_delim); } batch_write(opt, scratch->buf, scratch->len); if (opt->batch_mode == BATCH_MODE_CONTENTS) { + char buf[] = {opt->output_delim}; print_object_or_die(opt, data); - batch_write(opt, "\n", 1); + batch_write(opt, buf, 1); } } @@ -520,22 +523,25 @@ static void batch_one_object(const char *obj_name, if (result != FOUND) { switch (result) { case MISSING_OBJECT: - printf("%s missing\n", obj_name); + printf("%s missing%c", obj_name, opt->output_delim); break; case SHORT_NAME_AMBIGUOUS: - printf("%s ambiguous\n", obj_name); + printf("%s ambiguous%c", obj_name, opt->output_delim); break; case DANGLING_SYMLINK: - printf("dangling %"PRIuMAX"\n%s\n", - (uintmax_t)strlen(obj_name), obj_name); + printf("dangling %"PRIuMAX"%c%s%c", + (uintmax_t)strlen(obj_name), + opt->output_delim, obj_name, opt->output_delim); break; case SYMLINK_LOOP: - printf("loop %"PRIuMAX"\n%s\n", - (uintmax_t)strlen(obj_name), obj_name); + printf("loop %"PRIuMAX"%c%s%c", + (uintmax_t)strlen(obj_name), + opt->output_delim, obj_name, opt->output_delim); break; case NOT_DIR: - printf("notdir %"PRIuMAX"\n%s\n", - (uintmax_t)strlen(obj_name), obj_name); + printf("notdir %"PRIuMAX"%c%s%c", + (uintmax_t)strlen(obj_name), + opt->output_delim, obj_name, opt->output_delim); break; default: BUG("unknown get_sha1_with_context result %d\n", @@ -547,9 +553,9 @@ static void batch_one_object(const char *obj_name, } if (ctx.mode == 0) { - printf("symlink %"PRIuMAX"\n%s\n", + printf("symlink %"PRIuMAX"%c%s%c", (uintmax_t)ctx.symlink_path.len, - ctx.symlink_path.buf); + opt->output_delim, ctx.symlink_path.buf, opt->output_delim); fflush(stdout); return; } @@ -913,6 +919,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) struct batch_options batch = {0}; int unknown_type = 0; int input_nul_terminated = 0; + int nul_terminated = 0; const char * const usage[] = { N_("git cat-file <type> <object>"), @@ -920,7 +927,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) N_("git cat-file (-t | -s) [--allow-unknown-type] <object>"), N_("git cat-file (--batch | --batch-check | --batch-command) [--batch-all-objects]\n" " [--buffer] [--follow-symlinks] [--unordered]\n" - " [--textconv | --filters] [-z]"), + " [--textconv | --filters] [-z] [-Z]"), N_("git cat-file (--textconv | --filters)\n" " [<rev>:<path|tree-ish> | --path=<path|tree-ish> <rev>]"), NULL @@ -950,6 +957,7 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) PARSE_OPT_OPTARG | PARSE_OPT_NONEG, batch_option_callback), OPT_BOOL('z', NULL, &input_nul_terminated, N_("stdin is NUL-terminated")), + OPT_BOOL('Z', NULL, &nul_terminated, N_("stdin and stdout is NUL-terminated")), OPT_CALLBACK_F(0, "batch-command", &batch, N_("format"), N_("read commands from stdin"), PARSE_OPT_OPTARG | PARSE_OPT_NONEG, @@ -1011,8 +1019,15 @@ int cmd_cat_file(int argc, const char **argv, const char *prefix) else if (input_nul_terminated) usage_msg_optf(_("'%s' requires a batch mode"), usage, options, "-z"); + else if (nul_terminated) + usage_msg_optf(_("'%s' requires a batch mode"), usage, options, + "-Z"); - batch.input_delim = input_nul_terminated ? '\0' : '\n'; + batch.input_delim = batch.output_delim = '\n'; + if (input_nul_terminated) + batch.input_delim = '\0'; + if (nul_terminated) + batch.input_delim = batch.output_delim = '\0'; /* Batch defaults */ if (batch.buffer_output < 0) diff --git a/t/t1006-cat-file.sh b/t/t1006-cat-file.sh index 7b985cfded..d73a0be1b9 100755 --- a/t/t1006-cat-file.sh +++ b/t/t1006-cat-file.sh @@ -89,7 +89,8 @@ done for opt in --buffer \ --follow-symlinks \ --batch-all-objects \ - -z + -z \ + -Z do test_expect_success "usage: bad option combination: $opt without batch mode" ' test_incompatible_usage git cat-file $opt && @@ -392,17 +393,18 @@ deadbeef " -batch_output="$hello_sha1 blob $hello_size -$hello_content -$commit_sha1 commit $commit_size -$commit_content -$tag_sha1 tag $tag_size -$tag_content -deadbeef missing - missing" +printf "%s\0" \ + "$hello_sha1 blob $hello_size" \ + "$hello_content" \ + "$commit_sha1 commit $commit_size" \ + "$commit_content" \ + "$tag_sha1 tag $tag_size" \ + "$tag_content" \ + "deadbeef missing" \ + " missing" >batch_output test_expect_success '--batch with multiple sha1s gives correct format' ' - echo "$batch_output" >expect && + tr "\0" "\n" <batch_output >expect && echo_without_newline "$batch_input" >in && git cat-file --batch <in >actual && test_cmp expect actual @@ -410,11 +412,17 @@ test_expect_success '--batch with multiple sha1s gives correct format' ' test_expect_success '--batch, -z with multiple sha1s gives correct format' ' echo_without_newline_nul "$batch_input" >in && - echo "$batch_output" >expect && + tr "\0" "\n" <batch_output >expect && git cat-file --batch -z <in >actual && test_cmp expect actual ' +test_expect_success '--batch, -Z with multiple sha1s gives correct format' ' + echo_without_newline_nul "$batch_input" >in && + git cat-file --batch -Z <in >actual && + test_cmp batch_output actual +' + batch_check_input="$hello_sha1 $tree_sha1 $commit_sha1 @@ -423,40 +431,55 @@ deadbeef " -batch_check_output="$hello_sha1 blob $hello_size -$tree_sha1 tree $tree_size -$commit_sha1 commit $commit_size -$tag_sha1 tag $tag_size -deadbeef missing - missing" +printf "%s\0" \ + "$hello_sha1 blob $hello_size" \ + "$tree_sha1 tree $tree_size" \ + "$commit_sha1 commit $commit_size" \ + "$tag_sha1 tag $tag_size" \ + "deadbeef missing" \ + " missing" >batch_check_output test_expect_success "--batch-check with multiple sha1s gives correct format" ' - echo "$batch_check_output" >expect && + tr "\0" "\n" <batch_check_output >expect && echo_without_newline "$batch_check_input" >in && git cat-file --batch-check <in >actual && test_cmp expect actual ' test_expect_success "--batch-check, -z with multiple sha1s gives correct format" ' - echo "$batch_check_output" >expect && + tr "\0" "\n" <batch_check_output >expect && echo_without_newline_nul "$batch_check_input" >in && git cat-file --batch-check -z <in >actual && test_cmp expect actual ' -test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' ' +test_expect_success "--batch-check, -Z with multiple sha1s gives correct format" ' + echo_without_newline_nul "$batch_check_input" >in && + git cat-file --batch-check -Z <in >actual && + test_cmp batch_check_output actual +' + +test_expect_success FUNNYNAMES 'setup with newline in input' ' touch -- "newline${LF}embedded" && git add -- "newline${LF}embedded" && git commit -m "file with newline embedded" && test_tick && - printf "HEAD:newline${LF}embedded" >in && - git cat-file --batch-check -z <in >actual && + printf "HEAD:newline${LF}embedded" >in +' +test_expect_success FUNNYNAMES '--batch-check, -z with newline in input' ' + git cat-file --batch-check -z <in >actual && echo "$(git rev-parse "HEAD:newline${LF}embedded") blob 0" >expect && test_cmp expect actual ' +test_expect_success FUNNYNAMES '--batch-check, -Z with newline in input' ' + git cat-file --batch-check -Z <in >actual && + printf "%s\0" "$(git rev-parse "HEAD:newline${LF}embedded") blob 0" >expect && + test_cmp expect actual +' + batch_command_multiple_info="info $hello_sha1 info $tree_sha1 info $commit_sha1 @@ -480,7 +503,13 @@ test_expect_success '--batch-command with multiple info calls gives correct form echo "$batch_command_multiple_info" | tr "\n" "\0" >in && git cat-file --batch-command --buffer -z <in >actual && - test_cmp expect actual + test_cmp expect actual && + + echo "$batch_command_multiple_info" | tr "\n" "\0" >in && + tr "\n" "\0" <expect >expect_nul && + git cat-file --batch-command --buffer -Z <in >actual && + + test_cmp expect_nul actual ' batch_command_multiple_contents="contents $hello_sha1 @@ -490,15 +519,15 @@ contents deadbeef flush" test_expect_success '--batch-command with multiple command calls gives correct format' ' - cat >expect <<-EOF && - $hello_sha1 blob $hello_size - $hello_content - $commit_sha1 commit $commit_size - $commit_content - $tag_sha1 tag $tag_size - $tag_content - deadbeef missing - EOF + printf "%s\0" \ + "$hello_sha1 blob $hello_size" \ + "$hello_content" \ + "$commit_sha1 commit $commit_size" \ + "$commit_content" \ + "$tag_sha1 tag $tag_size" \ + "$tag_content" \ + "deadbeef missing" >expect_nul && + tr "\0" "\n" <expect_nul >expect && echo "$batch_command_multiple_contents" >in && git cat-file --batch-command --buffer <in >actual && @@ -508,7 +537,12 @@ test_expect_success '--batch-command with multiple command calls gives correct f echo "$batch_command_multiple_contents" | tr "\n" "\0" >in && git cat-file --batch-command --buffer -z <in >actual && - test_cmp expect actual + test_cmp expect actual && + + echo "$batch_command_multiple_contents" | tr "\n" "\0" >in && + git cat-file --batch-command --buffer -Z <in >actual && + + test_cmp expect_nul actual ' test_expect_success 'setup blobs which are likely to delta' ' @@ -848,6 +882,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for brok test_cmp expect actual ' +test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for broken in-repo, same-dir links' ' + printf "HEAD:broken-same-dir-link\0" >in && + printf "dangling 25\0HEAD:broken-same-dir-link\0" >expect && + git cat-file --batch-check --follow-symlinks -Z <in >actual && + test_cmp expect actual +' + test_expect_success 'git cat-file --batch-check --follow-symlinks works for same-dir links-to-links' ' echo HEAD:link-to-link | git cat-file --batch-check --follow-symlinks >actual && test_cmp found actual @@ -862,6 +903,15 @@ test_expect_success 'git cat-file --batch-check --follow-symlinks works for pare test_cmp expect actual ' +test_expect_success 'git cat-file --batch-check --follow-symlinks -Z works for parent-dir links' ' + echo HEAD:dir/parent-dir-link | git cat-file --batch-check --follow-symlinks >actual && + test_cmp found actual && + printf "notdir 29\0HEAD:dir/parent-dir-link/nope\0" >expect && + printf "HEAD:dir/parent-dir-link/nope\0" >in && + git cat-file --batch-check --follow-symlinks -Z <in >actual && + test_cmp expect actual +' + test_expect_success 'git cat-file --batch-check --follow-symlinks works for .. links' ' echo dangling 22 >expect && echo HEAD:dir/link-dir/nope >>expect && @@ -976,6 +1026,13 @@ test_expect_success 'git cat-file --batch-check --follow-symlink breaks loops' ' test_cmp expect actual ' +test_expect_success 'git cat-file --batch-check --follow-symlink -Z breaks loops' ' + printf "loop 10\0HEAD:loop1\0" >expect && + printf "HEAD:loop1\0" >in && + git cat-file --batch-check --follow-symlinks -Z <in >actual && + test_cmp expect actual +' + test_expect_success 'git cat-file --batch --follow-symlink returns correct sha and mode' ' echo HEAD:morx | git cat-file --batch >expect && echo HEAD:morx | git cat-file --batch --follow-symlinks >actual &&
In db9d67f2e9 (builtin/cat-file.c: support NUL-delimited input with `-z`, 2022-07-22), we have introduced a new mode to read the input via NUL-delimited records instead of newline-delimited records. This allows the user to query for revisions that have newlines in their path component. While unusual, such queries are perfectly valid and thus it is clear that we should be able to support them properly. Unfortunately, the commit only changed the input to be NUL-delimited, but didn't change the output at the same time. While this is fine for queries that are processed successfully, it is less so for queries that aren't. In the case of missing commits for example the result can become entirely unparsable: ``` $ printf "7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10\n1234567890\n\n\commit000" | git cat-file --batch -z 7ce4f05bae8120d9fa258e854a8669f6ea9cb7b1 blob 10 1234567890 commit missing ``` This is of course a crafted query that is intentionally gaming the deficiency, but more benign queries that contain newlines would have similar problems. Ideally, we should have also changed the output to be NUL-delimited when `-z` is specified to avoid this problem. As the input is NUL-delimited, it is clear that the output in this case cannot ever contain NUL characters by itself. Furthermore, Git does not allow NUL characters in revisions anyway, further stressing the point that using NUL-delimited output is safe. The only exception is of course the object data itself, but as git-cat-file(1) prints the size of the object data clients should read until that specified size has been consumed. But even though `-z` has only been introduced a few releases ago in Git v2.38.0, changing the output format retroactively to also NUL-delimit output would be a backwards incompatible change. And while one could make the argument that the output is inherently broken already, we need to assume that there are existing users out there that use it just fine given that revisions containing newlines are quite exotic. Instead, introduce a new option `-Z` that switches to NUL-delimited input and output. The old `-z` option is marked as deprecated with a hint that its output may become unparsable. Co-authored-by: Toon Claes <toon@iotcl.com> Signed-off-by: Patrick Steinhardt <ps@pks.im> --- Documentation/git-cat-file.txt | 13 +++- builtin/cat-file.c | 55 +++++++++------ t/t1006-cat-file.sh | 123 ++++++++++++++++++++++++--------- 3 files changed, 137 insertions(+), 54 deletions(-)