| Message ID | 20210711215510.191626-1-sandals@crustytoothpaste.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 1ceed60061e646ab368ee141fc85eafcea0601a4 |
| Headers | show |
| Series | [v2] rev-list: add option for --pretty=format without header | expand |
On Sun, Jul 11, 2021 at 11:55 PM brian m. carlson <sandals@crustytoothpaste.net> wrote: > > In general, we encourage users to use plumbing commands, like git > rev-list, over porcelain commands, like git log, when scripting. > However, git rev-list has one glaring problem that prevents it from > being used in certain cases: when --pretty is used with a custom format, > it always prints out a line containing "commit" and the object ID. This > makes it unsuitable for many scripting needs, and forces users to use > git log instead. > > While we can't change this behavior for backwards compatibility, we can > add an option to suppress this behavior, so let's do so, and call it > "--no-commit-header". Additionally, add the corresponding positive > option to switch it back on. > > Note that this option doesn't affect the built-in formats, only custom > formats. This is exactly the same behavior as users already have from > git log and is what most users will be used to. > > Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> > --- > Changes from v1: > * Ensure this applies only to custom formats. > * Fix the issue with --abbrev-commit mentioned by Junio. > * Add tests for additional cases, including built-in formats. > * Update documentation to reflect this data. Thanks! This version mostly looks good to me! > diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh > index 35a2f62392..41d0ca00b1 100755 > --- a/t/t6006-rev-list-format.sh > +++ b/t/t6006-rev-list-format.sh > @@ -41,22 +41,59 @@ test_expect_success 'setup' ' > echo "$added_iso88591" | git commit -F - && > head1=$(git rev-parse --verify HEAD) && > head1_short=$(git rev-parse --verify --short $head1) && > + head1_short4=$(git rev-parse --verify --short=4 $head1) && > tree1=$(git rev-parse --verify HEAD:) && > tree1_short=$(git rev-parse --verify --short $tree1) && > echo "$changed" > foo && > echo "$changed_iso88591" | git commit -a -F - && > head2=$(git rev-parse --verify HEAD) && > head2_short=$(git rev-parse --verify --short $head2) && > + head2_short4=$(git rev-parse --verify --short=4 $head2) && > tree2=$(git rev-parse --verify HEAD:) && > tree2_short=$(git rev-parse --verify --short $tree2) && > git config --unset i18n.commitEncoding > ' > > -# usage: test_format name format_string [failure] <expected_output > +# usage: test_format [argument...] name format_string [failure] <expected_output Usually we use "option" instead of "argument" for the flags starting with "-" or "--" before the required parameter. For example: $ git rev-list -h usage: git rev-list [OPTION] <commit-id>... [ -- paths... ] ... (Yeah, I agree that [OPTION] is not very consistent with what other commands show, which is usually "[<options>]".) > test_format () { > + local args= Here... > + while true > + do > + case "$1" in > + --*) > + args="$args $1" ...here... > + shift;; > + *) > + break;; > + esac > + done > cat >expect.$1 > test_expect_${3:-success} "format $1" " > - git rev-list --pretty=format:'$2' main >output.$1 && > + git rev-list $args --pretty=format:'$2' main >output.$1 && ...and here "opts" might be better than "args". > + test_cmp expect.$1 output.$1 > + " > +} > + > +# usage: test_pretty [argument...] name format_name [failure] <expected_output Here... > +test_pretty () { > + local args= ...and in this function too. > + while true > + do > + case "$1" in > + --*) > + args="$args $1" > + shift;; > + *) > + break;; > + esac > + done > + cat >expect.$1 > + test_expect_${3:-success} "pretty $1 (without --no-commit-header)" " > + git rev-list $args --pretty='$2' main >output.$1 && > + test_cmp expect.$1 output.$1 > + " > + test_expect_${3:-success} "pretty $1 (with --no-commit-header)" " > + git rev-list $args --no-commit-header --pretty='$2' main >output.$1 && > test_cmp expect.$1 output.$1 > " > } [...] > +test_pretty oneline oneline <<EOF > +$head2 $changed > +$head1 $added > +EOF > + > +test_pretty short short <<EOF test_pretty() accepts options like --no-commit-header, but it's only used without any option. So I wonder if you forgot to add a few tests with some options. > +commit $head2 > +Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> > + > + $changed > + > +commit $head1 > +Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> > + > + $added > + > +EOF > + > test_expect_success 'basic colors' ' > cat >expect <<-EOF && > commit $head2
On Sun, Jul 11, 2021 at 09:55:10PM +0000, brian m. carlson wrote: > In general, we encourage users to use plumbing commands, like git > rev-list, over porcelain commands, like git log, when scripting. > However, git rev-list has one glaring problem that prevents it from > being used in certain cases: when --pretty is used with a custom format, > it always prints out a line containing "commit" and the object ID. This > makes it unsuitable for many scripting needs, and forces users to use > git log instead. > > While we can't change this behavior for backwards compatibility, we can > add an option to suppress this behavior, so let's do so, and call it > "--no-commit-header". Additionally, add the corresponding positive > option to switch it back on. > > Note that this option doesn't affect the built-in formats, only custom > formats. This is exactly the same behavior as users already have from > git log and is what most users will be used to. Thanks for working on this. It has bugged me for at least a decade. :) I do wish this had been made the default when we introduced --pretty=format, but I agree we can't just change it now. This could be something to keep in mind for future deprecation (or a large breaking version). People would have to start saying --commit-header now to future-proof themselves if they really want the current behavior. I'm OK to leave any plans / warnings like that for future work. The patch looks correct to me. I did have one small nit: > diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt > index 5bf2a85f69..23388f36c3 100644 > --- a/Documentation/rev-list-options.txt > +++ b/Documentation/rev-list-options.txt > @@ -1064,6 +1064,14 @@ ifdef::git-rev-list[] > --header:: > Print the contents of the commit in raw-format; each record is > separated with a NUL character. > + > +--no-commit-header:: > + Suppress the header line containing "commit" and the object ID printed before > + the specified format. This has no effect on the built-in formats; only custom > + formats are affected. It wasn't immediately obvious to me what "custom formats" meant here. I don't think we use that term elsewhere, nor do we seem to have a succinct phrase for the concept. Maybe something like: only custom formats (i.e., `--pretty=format:`) are affected. helps without making it too clunky? -Peff
On 2021-07-12 at 07:30:33, Christian Couder wrote: > Usually we use "option" instead of "argument" for the flags starting > with "-" or "--" before the required parameter. For example: > > $ git rev-list -h > usage: git rev-list [OPTION] <commit-id>... [ -- paths... ] > ... > > (Yeah, I agree that [OPTION] is not very consistent with what other > commands show, which is usually "[<options>]".) I can do that rename. > test_pretty() accepts options like --no-commit-header, but it's only > used without any option. So I wonder if you forgot to add a few tests > with some options. I originally intended to add some, but decided to remove them because the --abbrev tests fit better elsewhere. I'll remove the options.
On 2021-07-12 at 18:13:54, Jeff King wrote: > Thanks for working on this. It has bugged me for at least a decade. :) I figured this would be something you'd like to see fixed. I'm just surprised that I'm the first to send a patch. > It wasn't immediately obvious to me what "custom formats" meant here. I > don't think we use that term elsewhere, nor do we seem to have a > succinct phrase for the concept. Maybe something like: > > only custom formats (i.e., `--pretty=format:`) are affected. > > helps without making it too clunky? I can add that.
diff --git a/Documentation/rev-list-options.txt b/Documentation/rev-list-options.txt index 5bf2a85f69..23388f36c3 100644 --- a/Documentation/rev-list-options.txt +++ b/Documentation/rev-list-options.txt @@ -1064,6 +1064,14 @@ ifdef::git-rev-list[] --header:: Print the contents of the commit in raw-format; each record is separated with a NUL character. + +--no-commit-header:: + Suppress the header line containing "commit" and the object ID printed before + the specified format. This has no effect on the built-in formats; only custom + formats are affected. + +--commit-header:: + Overrides a previous `--no-commit-header`. endif::git-rev-list[] --parents:: diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 7677b1af5a..36cb909eba 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -127,13 +127,15 @@ static void show_commit(struct commit *commit, void *data) if (info->header_prefix) fputs(info->header_prefix, stdout); - if (!revs->graph) - fputs(get_revision_mark(revs, commit), stdout); - if (revs->abbrev_commit && revs->abbrev) - fputs(find_unique_abbrev(&commit->object.oid, revs->abbrev), - stdout); - else - fputs(oid_to_hex(&commit->object.oid), stdout); + if (revs->include_header) { + if (!revs->graph) + fputs(get_revision_mark(revs, commit), stdout); + if (revs->abbrev_commit && revs->abbrev) + fputs(find_unique_abbrev(&commit->object.oid, revs->abbrev), + stdout); + else + fputs(oid_to_hex(&commit->object.oid), stdout); + } if (revs->print_parents) { struct commit_list *parents = commit->parents; while (parents) { @@ -153,7 +155,7 @@ static void show_commit(struct commit *commit, void *data) show_decorations(revs, commit); if (revs->commit_format == CMIT_FMT_ONELINE) putchar(' '); - else + else if (revs->include_header) putchar('\n'); if (revs->verbose_header) { @@ -512,6 +514,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) repo_init_revisions(the_repository, &revs, prefix); revs.abbrev = DEFAULT_ABBREV; revs.commit_format = CMIT_FMT_UNSPECIFIED; + revs.include_header = 1; /* * Scan the argument list before invoking setup_revisions(), so that we @@ -627,6 +630,16 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) continue; } + if (!strcmp(arg, ("--commit-header"))) { + revs.include_header = 1; + continue; + } + + if (!strcmp(arg, ("--no-commit-header"))) { + revs.include_header = 0; + continue; + } + if (!strcmp(arg, "--disk-usage")) { show_disk_usage = 1; info.flags |= REV_LIST_QUIET; @@ -636,10 +649,12 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) usage(rev_list_usage); } + if (revs.commit_format != CMIT_FMT_USERFORMAT) + revs.include_header = 1; if (revs.commit_format != CMIT_FMT_UNSPECIFIED) { /* The command line has a --pretty */ info.hdr_termination = '\n'; - if (revs.commit_format == CMIT_FMT_ONELINE) + if (revs.commit_format == CMIT_FMT_ONELINE || !revs.include_header) info.header_prefix = ""; else info.header_prefix = "commit "; diff --git a/revision.h b/revision.h index 17698cb51a..7464434f60 100644 --- a/revision.h +++ b/revision.h @@ -215,7 +215,8 @@ struct rev_info { missing_newline:1, date_mode_explicit:1, preserve_subject:1, - encode_email_headers:1; + encode_email_headers:1, + include_header:1; unsigned int disable_stdin:1; /* --show-linear-break */ unsigned int track_linear:1, diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh index 35a2f62392..41d0ca00b1 100755 --- a/t/t6006-rev-list-format.sh +++ b/t/t6006-rev-list-format.sh @@ -41,22 +41,59 @@ test_expect_success 'setup' ' echo "$added_iso88591" | git commit -F - && head1=$(git rev-parse --verify HEAD) && head1_short=$(git rev-parse --verify --short $head1) && + head1_short4=$(git rev-parse --verify --short=4 $head1) && tree1=$(git rev-parse --verify HEAD:) && tree1_short=$(git rev-parse --verify --short $tree1) && echo "$changed" > foo && echo "$changed_iso88591" | git commit -a -F - && head2=$(git rev-parse --verify HEAD) && head2_short=$(git rev-parse --verify --short $head2) && + head2_short4=$(git rev-parse --verify --short=4 $head2) && tree2=$(git rev-parse --verify HEAD:) && tree2_short=$(git rev-parse --verify --short $tree2) && git config --unset i18n.commitEncoding ' -# usage: test_format name format_string [failure] <expected_output +# usage: test_format [argument...] name format_string [failure] <expected_output test_format () { + local args= + while true + do + case "$1" in + --*) + args="$args $1" + shift;; + *) + break;; + esac + done cat >expect.$1 test_expect_${3:-success} "format $1" " - git rev-list --pretty=format:'$2' main >output.$1 && + git rev-list $args --pretty=format:'$2' main >output.$1 && + test_cmp expect.$1 output.$1 + " +} + +# usage: test_pretty [argument...] name format_name [failure] <expected_output +test_pretty () { + local args= + while true + do + case "$1" in + --*) + args="$args $1" + shift;; + *) + break;; + esac + done + cat >expect.$1 + test_expect_${3:-success} "pretty $1 (without --no-commit-header)" " + git rev-list $args --pretty='$2' main >output.$1 && + test_cmp expect.$1 output.$1 + " + test_expect_${3:-success} "pretty $1 (with --no-commit-header)" " + git rev-list $args --no-commit-header --pretty='$2' main >output.$1 && test_cmp expect.$1 output.$1 " } @@ -93,6 +130,20 @@ $head1 $head1_short EOF +test_format --no-commit-header hash-no-header %H%n%h <<EOF +$head2 +$head2_short +$head1 +$head1_short +EOF + +test_format --abbrev-commit --abbrev=0 --no-commit-header hash-no-header-abbrev %H%n%h <<EOF +$head2 +$head2_short4 +$head1 +$head1_short4 +EOF + test_format tree %T%n%t <<EOF commit $head2 $tree2 @@ -181,6 +232,31 @@ $added EOF +test_format --no-commit-header raw-body-no-header %B <<EOF +$changed + +$added + +EOF + +test_pretty oneline oneline <<EOF +$head2 $changed +$head1 $added +EOF + +test_pretty short short <<EOF +commit $head2 +Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> + + $changed + +commit $head1 +Author: $GIT_AUTHOR_NAME <$GIT_AUTHOR_EMAIL> + + $added + +EOF + test_expect_success 'basic colors' ' cat >expect <<-EOF && commit $head2
In general, we encourage users to use plumbing commands, like git rev-list, over porcelain commands, like git log, when scripting. However, git rev-list has one glaring problem that prevents it from being used in certain cases: when --pretty is used with a custom format, it always prints out a line containing "commit" and the object ID. This makes it unsuitable for many scripting needs, and forces users to use git log instead. While we can't change this behavior for backwards compatibility, we can add an option to suppress this behavior, so let's do so, and call it "--no-commit-header". Additionally, add the corresponding positive option to switch it back on. Note that this option doesn't affect the built-in formats, only custom formats. This is exactly the same behavior as users already have from git log and is what most users will be used to. Signed-off-by: brian m. carlson <sandals@crustytoothpaste.net> --- Changes from v1: * Ensure this applies only to custom formats. * Fix the issue with --abbrev-commit mentioned by Junio. * Add tests for additional cases, including built-in formats. * Update documentation to reflect this data. Documentation/rev-list-options.txt | 8 +++ builtin/rev-list.c | 33 ++++++++---- revision.h | 3 +- t/t6006-rev-list-format.sh | 80 +++++++++++++++++++++++++++++- 4 files changed, 112 insertions(+), 12 deletions(-)