mbox series

[v2,00/13] Fixes to trailer test script, help text, and documentation

Message ID pull.1564.v2.git.1691702283.gitgitgadget@gmail.com (mailing list archive)
Headers show
Series Fixes to trailer test script, help text, and documentation | expand

Message

Philippe Blain via GitGitGadget Aug. 10, 2023, 9:17 p.m. UTC
This series contains various fixes to the trailer code. They pertain to
fixes to the test script, the command line help text for the
interpret-trailers builtin, and the documentation.

Patch 1 is the most important as it does cleanups in the tests where we used
'git config' in a test case without cleaning up that state for the next
test. This makes the tests self-contained, making it easier to add new tests
anywhere along the script, without worrying about previously-set implicit
state. These test cleanups exposed lots of cases where the test cases are
mutating more configuration state than is necessary to test the specific
behavior in the test; however such extraneous configurations were not
cleaned up to make these patches easier to review (again, we are not
changing any behavior and we are also not changing what the test cases
themselves purport to do).

Note that Patch 1 was originally a 22-commit series, but was squashed
together to make it easier to see the final diff for each test case. You can
see the 22-commit breakdown at
https://github.com/listx/git/tree/backup-trailer-22-commit-breakdown

Patch 3 adds some tests to check the behavior of '--no-if-exists' and
'--no-if-missing', which weren't previously tested. It also adds
similarly-themed test cases for '--no-where' which only had 1 test case for
it.

The other patches aren't as important, but are included here because I think
they are too small to include in a separate series.


Updates in v2
=============

 * Many additional patches to fix the help text and docs. No changes to any
   of the patches touching the actual tests (that is, Patch 1 and 3 have
   stayed the same, other than a rewording of the commit message for Patch
   1).
 * Of these new patches, I think the last one () is the most important as it
   resolves a longtime ambiguity about what a can be.

Linus Arver (13):
  trailer tests: make test cases self-contained
  trailer test description: this tests --where=after, not --where=before
  trailer: add tests to check defaulting behavior with --no-* flags
  trailer doc: narrow down scope of --where and related flags
  trailer: trailer location is a place, not an action
  trailer --no-divider help: describe usual "---" meaning
  trailer --parse help: expose aliased options
  trailer --only-input: prefer "configuration variables" over "rules"
  trailer --parse docs: add explanation for its usefulness
  trailer --unfold help: prefer "reformat" over "join"
  trailer doc: emphasize the effect of configuration variables
  trailer doc: separator within key suppresses default separator
  trailer doc: <token> is a <key> or <keyAlias>, not both

 Documentation/git-interpret-trailers.txt | 183 ++++----
 builtin/interpret-trailers.c             |  10 +-
 t/t7513-interpret-trailers.sh            | 506 +++++++++++++++++++----
 3 files changed, 544 insertions(+), 155 deletions(-)


base-commit: 1b0a5129563ebe720330fdc8f5c6843d27641137
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1564%2Flistx%2Ftrailer-fixes-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1564/listx/trailer-fixes-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/1564

Range-diff vs v1:

  1:  6d67ae6b1f6 !  1:  1623dd000dd trailer tests: make test cases self-contained
     @@ Commit message
          This commit was created mechanically as follows: we changed the first
          occurrence of a particular "git config trailer.*" option, then ran the
          tests repeatedly to see which ones broke, adding in the extra
     -    "test_config" equivalents to make them pass again. This was done until
     -    there were no more unbridled "git config" invocations. Some "git config"
     -    invocations still do exist in the script, but they were already cleaned
     -    up properly with
     +    "test_config" equivalents to make them pass again. In addition, in some
     +    test cases we removed "git config --unset ..." lines because they were
     +    no longer necessary (as the --unset was being used to clean up leaked
     +    configuration state from earlier test cases).
     +
     +    The process described above was done repeatedly until there were no more
     +    unbridled "git config" invocations. Some "git config" invocations still
     +    do exist in the script, but they were already cleaned up properly with
      
              test_when_finished "git config --remove-section ..."
      
  2:  100a2297fa3 =  2:  f680e76de84 trailer test description: this tests --where=after, not --where=before
  3:  6b427b4b1e8 =  3:  4b5c458ef43 trailer: add tests to check defaulting behavior with --no-* flags
  -:  ----------- >  4:  0df12c5c2dd trailer doc: narrow down scope of --where and related flags
  4:  53adcd9bf14 =  5:  040766861e2 trailer: trailer location is a place, not an action
  5:  68bc89beb2a =  6:  3e58b6f5ea2 trailer --no-divider help: describe usual "---" meaning
  -:  ----------- >  7:  d1780a0127a trailer --parse help: expose aliased options
  -:  ----------- >  8:  5cfff52da8f trailer --only-input: prefer "configuration variables" over "rules"
  -:  ----------- >  9:  ef6b77016cd trailer --parse docs: add explanation for its usefulness
  -:  ----------- > 10:  a08d78618ba trailer --unfold help: prefer "reformat" over "join"
  -:  ----------- > 11:  4db823ac354 trailer doc: emphasize the effect of configuration variables
  -:  ----------- > 12:  66087eaf5bd trailer doc: separator within key suppresses default separator
  -:  ----------- > 13:  7b66cf29d29 trailer doc: <token> is a <key> or <keyAlias>, not both

Comments

Junio C Hamano Aug. 11, 2023, 1:41 a.m. UTC | #1
t0450 dies like the attached, probably because the documentation was
updated to say "<key> or <keyAlias>" but a matching update to the
output of "interpret-trailers -h" command help is missing?

A possible trivial fix to 13/13 will be queued on top as "SQUASH???"
patch for now.

 builtin/interpret-trailers.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git c/builtin/interpret-trailers.c w/builtin/interpret-trailers.c
index 832f86a770..d2d78fd961 100644
--- c/builtin/interpret-trailers.c
+++ w/builtin/interpret-trailers.c
@@ -14,7 +14,7 @@
 
 static const char * const git_interpret_trailers_usage[] = {
 	N_("git interpret-trailers [--in-place] [--trim-empty]\n"
-	   "                       [(--trailer <token>[(=|:)<value>])...]\n"
+	   "                       [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]\n"
 	   "                       [--parse] [<file>...]"),
 	NULL
 };





--- txt 2023-08-10 22:31:54.328609532 +0000
+++ help        2023-08-10 22:31:54.332609929 +0000
@@ -1,3 +1,3 @@
 git interpret-trailers [--in-place] [--trim-empty]
-                       [(--trailer (<key>|<keyAlias>)[(=|:)<value>])...]
+                       [(--trailer <token>[(=|:)<value>])...]
                        [--parse] [<file>...]
not ok 302 - interpret-trailers -h output and SYNOPSIS agree
#
#                       t2s="$(txt_to_synopsis "$builtin")" &&
#                       if test "$builtin" = "merge-tree"
#                       then
#                               test_when_finished "rm -f t2s.new" &&
#                               sed -e 's/ (deprecated)$//g' <"$t2s" >t2s.new
#                               t2s=t2s.new
#                       fi &&
#                       h2s="$(help_to_synopsis "$builtin")" &&
#
#                       # The *.txt and -h use different spacing for the
#                       # alignment of continued usage output, normalize it.
#                       align_after_nl "$builtin" <"$t2s" >txt &&
#                       align_after_nl "$builtin" <"$h2s" >help &&
#                       test_cmp txt help
#
1
Linus Arver Aug. 11, 2023, 5:38 p.m. UTC | #2
Junio C Hamano <gitster@pobox.com> writes:

> t0450 dies like the attached, probably because the documentation was
> updated to say "<key> or <keyAlias>" but a matching update to the
> output of "interpret-trailers -h" command help is missing?

Yes, sorry I missed this. I already have made the fix locally.

> A possible trivial fix to 13/13 will be queued on top as "SQUASH???"
> patch for now.

Thanks.