[v2,00/13] format-patch: clean up tests and documentation
mbox series

Message ID cover.1566878373.git.liu.denton@gmail.com
Headers show
Series
  • format-patch: clean up tests and documentation
Related show

Message

Denton Liu Aug. 27, 2019, 4:04 a.m. UTC
In this reroll, I squashed Junio's suggestion into the correct patch.
Also, I took Eric's suggestion and removed the weak justification (i.e.
better error messages) from the sed patch since it doesn't really
contribute.


As one of the older parts of the Git, the tests and documentation for
format-patch have been needing cleanup for a while. Let's do that in
this patchset!

This patchset is based on v3 of "format-patch: learn
--infer-cover-subject option (also t4014 cleanup)"[1].

Changes since v1:

* Squash Junio's patch into the correct patch ;)

* Remove weak justification (better error messages) in 8/13

Changes since v3 of "format-patch: learn --infer-cover-subject option (also
t4014 cleanup)":

* Squash in Junio's and Hannes' suggestions

* Add 't4014: let sed open its own files'

[1]: https://public-inbox.org/git/xmqqwof3ljcz.fsf@gitster-ct.c.googlers.com/T/#m19570aff4828dfbd65d57cacf231c2938af1dc9f


Denton Liu (13):
  t4014: drop unnecessary blank lines from test cases
  t4014: s/expected/expect/
  t4014: move closing sq onto its own line
  t4014: use sq for test case names
  t4014: remove spaces after redirect operators
  t4014: use indentable here-docs
  t4014: drop redirections to /dev/null
  t4014: let sed open its own files
  t4014: use test_line_count() where possible
  t4014: remove confusing pipe in check_threading()
  t4014: stop losing return codes of git commands
  Doc: add more detail for git-format-patch
  config/format.txt: specify default value of format.coverLetter

 Documentation/config/format.txt    |   1 +
 Documentation/git-format-patch.txt |  23 +-
 t/t4014-format-patch.sh            | 814 ++++++++++++++---------------
 3 files changed, 421 insertions(+), 417 deletions(-)

Range-diff against v1:
 1:  fb000bfca2 =  1:  fb000bfca2 t4014: drop unnecessary blank lines from test cases
 2:  0a5ce9b95f =  2:  0a5ce9b95f t4014: s/expected/expect/
 3:  5c49703aa4 =  3:  5c49703aa4 t4014: move closing sq onto its own line
 4:  02a11147fd =  4:  02a11147fd t4014: use sq for test case names
 5:  8d9791c061 =  5:  8d9791c061 t4014: remove spaces after redirect operators
 6:  90ad0fcf70 =  6:  90ad0fcf70 t4014: use indentable here-docs
 7:  804b3163f8 =  7:  804b3163f8 t4014: drop redirections to /dev/null
 8:  7d9a24a979 !  8:  967e624bb4 t4014: let sed open its own files
    @@ Commit message
         t4014: let sed open its own files
     
         In some cases, we were using a redirection operator to feed input into
    -    sed. However, since sed is capable of opening its own files and provides
    -    better error messages on IO failure, make sed open its own files instead
    -    of redirecting input into it.
    +    sed. However, since sed is capable of opening its own files, make sed
    +    open its own files instead of redirecting input into it.
     
         Signed-off-by: Denton Liu <liu.denton@gmail.com>
     
 9:  d068d42098 =  9:  9a42ec2b7e t4014: use test_line_count() where possible
10:  6a9409cee0 = 10:  8acc90f74d t4014: remove confusing pipe in check_threading()
11:  c580ce447b = 11:  bc7355485f t4014: stop losing return codes of git commands
12:  a97f861e6a ! 12:  fd343b99c5 Doc: add more detail for git-format-patch
    @@ Commit message
         In git-format-patch.txt, we were missing some key user information.
         First of all, document the special value of `--base=auto`.
     
    -    Next, while we're at it, surround option arguments with <>.
    +    Next, while we're at it, surround option arguments with <> and change
    +    existing names such as "Message-Id" to "message id", which conforms with
    +    how existing documentation is written.
     
         Finally, document the `format.outputDirectory` config and change
         `format.coverletter` to use camel case.
    @@ Documentation/git-format-patch.txt: SYNOPSIS
      		   [-n | --numbered | -N | --no-numbered]
      		   [--start-number <n>] [--numbered-files]
     -		   [--in-reply-to=Message-Id] [--suffix=.<sfx>]
    -+		   [--in-reply-to=<Message-Id>] [--suffix=.<sfx>]
    ++		   [--in-reply-to=<message id>] [--suffix=.<sfx>]
      		   [--ignore-if-in-upstream]
     -		   [--rfc] [--subject-prefix=Subject-Prefix]
    -+		   [--rfc] [--subject-prefix=<Subject-Prefix>]
    ++		   [--rfc] [--subject-prefix=<subject prefix>]
      		   [(--reroll-count|-v) <n>]
      		   [--to=<email>] [--cc=<email>]
      		   [--[no-]cover-letter] [--quiet]
    @@ Documentation/git-format-patch.txt: Beware that the default for 'git send-email'
      will want to ensure that threading is disabled for `git send-email`.
      
     ---in-reply-to=Message-Id::
    -+--in-reply-to=<Message-Id>::
    ++--in-reply-to=<message id>::
      	Make the first mail (or all the mails with `--no-thread`) appear as a
    - 	reply to the given Message-Id, which avoids breaking threads to
    +-	reply to the given Message-Id, which avoids breaking threads to
    ++	reply to the given <message id>, which avoids breaking threads to
      	provide a new patch series.
    + 
    + --ignore-if-in-upstream::
    +@@ Documentation/git-format-patch.txt: will want to ensure that threading is disabled for `git send-email`.
    + 	patches being generated, and any patch that matches is
    + 	ignored.
    + 
    +---subject-prefix=<Subject-Prefix>::
    ++--subject-prefix=<subject prefix>::
    + 	Instead of the standard '[PATCH]' prefix in the subject
    +-	line, instead use '[<Subject-Prefix>]'. This
    ++	line, instead use '[<subject prefix>]'. This
    + 	allows for useful naming of a patch series, and can be
    + 	combined with the `--numbered` option.
    + 
     @@ Documentation/git-format-patch.txt: you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
      --base=<commit>::
      	Record the base tree information to identify the state the
13:  7c8522abf2 <  -:  ---------- config/format.txt: specify default value of format.coverLetter
 -:  ---------- > 13:  4e429e1989 config/format.txt: specify default value of format.coverLetter

Comments

Denton Liu Sept. 4, 2019, 11:21 a.m. UTC | #1
Hi Junio,

I see that "dl/format-patch-doc-test-cleanup" currently has the comment
"Expecting a reroll." This should be the reroll that you're expecting ;)

Also, since there haven't been any comments on the topic in a while, I
propose that it should be ready for inclusion.

Thanks,

Denton

On Tue, Aug 27, 2019 at 12:04:47AM -0400, Denton Liu wrote:
> In this reroll, I squashed Junio's suggestion into the correct patch.
> Also, I took Eric's suggestion and removed the weak justification (i.e.
> better error messages) from the sed patch since it doesn't really
> contribute.
> 
> 
> As one of the older parts of the Git, the tests and documentation for
> format-patch have been needing cleanup for a while. Let's do that in
> this patchset!
> 
> This patchset is based on v3 of "format-patch: learn
> --infer-cover-subject option (also t4014 cleanup)"[1].
> 
> Changes since v1:
> 
> * Squash Junio's patch into the correct patch ;)
> 
> * Remove weak justification (better error messages) in 8/13
> 
> Changes since v3 of "format-patch: learn --infer-cover-subject option (also
> t4014 cleanup)":
> 
> * Squash in Junio's and Hannes' suggestions
> 
> * Add 't4014: let sed open its own files'
> 
> [1]: https://public-inbox.org/git/xmqqwof3ljcz.fsf@gitster-ct.c.googlers.com/T/#m19570aff4828dfbd65d57cacf231c2938af1dc9f
> 
> 
> Denton Liu (13):
>   t4014: drop unnecessary blank lines from test cases
>   t4014: s/expected/expect/
>   t4014: move closing sq onto its own line
>   t4014: use sq for test case names
>   t4014: remove spaces after redirect operators
>   t4014: use indentable here-docs
>   t4014: drop redirections to /dev/null
>   t4014: let sed open its own files
>   t4014: use test_line_count() where possible
>   t4014: remove confusing pipe in check_threading()
>   t4014: stop losing return codes of git commands
>   Doc: add more detail for git-format-patch
>   config/format.txt: specify default value of format.coverLetter
> 
>  Documentation/config/format.txt    |   1 +
>  Documentation/git-format-patch.txt |  23 +-
>  t/t4014-format-patch.sh            | 814 ++++++++++++++---------------
>  3 files changed, 421 insertions(+), 417 deletions(-)
> 
> Range-diff against v1:
>  1:  fb000bfca2 =  1:  fb000bfca2 t4014: drop unnecessary blank lines from test cases
>  2:  0a5ce9b95f =  2:  0a5ce9b95f t4014: s/expected/expect/
>  3:  5c49703aa4 =  3:  5c49703aa4 t4014: move closing sq onto its own line
>  4:  02a11147fd =  4:  02a11147fd t4014: use sq for test case names
>  5:  8d9791c061 =  5:  8d9791c061 t4014: remove spaces after redirect operators
>  6:  90ad0fcf70 =  6:  90ad0fcf70 t4014: use indentable here-docs
>  7:  804b3163f8 =  7:  804b3163f8 t4014: drop redirections to /dev/null
>  8:  7d9a24a979 !  8:  967e624bb4 t4014: let sed open its own files
>     @@ Commit message
>          t4014: let sed open its own files
>      
>          In some cases, we were using a redirection operator to feed input into
>     -    sed. However, since sed is capable of opening its own files and provides
>     -    better error messages on IO failure, make sed open its own files instead
>     -    of redirecting input into it.
>     +    sed. However, since sed is capable of opening its own files, make sed
>     +    open its own files instead of redirecting input into it.
>      
>          Signed-off-by: Denton Liu <liu.denton@gmail.com>
>      
>  9:  d068d42098 =  9:  9a42ec2b7e t4014: use test_line_count() where possible
> 10:  6a9409cee0 = 10:  8acc90f74d t4014: remove confusing pipe in check_threading()
> 11:  c580ce447b = 11:  bc7355485f t4014: stop losing return codes of git commands
> 12:  a97f861e6a ! 12:  fd343b99c5 Doc: add more detail for git-format-patch
>     @@ Commit message
>          In git-format-patch.txt, we were missing some key user information.
>          First of all, document the special value of `--base=auto`.
>      
>     -    Next, while we're at it, surround option arguments with <>.
>     +    Next, while we're at it, surround option arguments with <> and change
>     +    existing names such as "Message-Id" to "message id", which conforms with
>     +    how existing documentation is written.
>      
>          Finally, document the `format.outputDirectory` config and change
>          `format.coverletter` to use camel case.
>     @@ Documentation/git-format-patch.txt: SYNOPSIS
>       		   [-n | --numbered | -N | --no-numbered]
>       		   [--start-number <n>] [--numbered-files]
>      -		   [--in-reply-to=Message-Id] [--suffix=.<sfx>]
>     -+		   [--in-reply-to=<Message-Id>] [--suffix=.<sfx>]
>     ++		   [--in-reply-to=<message id>] [--suffix=.<sfx>]
>       		   [--ignore-if-in-upstream]
>      -		   [--rfc] [--subject-prefix=Subject-Prefix]
>     -+		   [--rfc] [--subject-prefix=<Subject-Prefix>]
>     ++		   [--rfc] [--subject-prefix=<subject prefix>]
>       		   [(--reroll-count|-v) <n>]
>       		   [--to=<email>] [--cc=<email>]
>       		   [--[no-]cover-letter] [--quiet]
>     @@ Documentation/git-format-patch.txt: Beware that the default for 'git send-email'
>       will want to ensure that threading is disabled for `git send-email`.
>       
>      ---in-reply-to=Message-Id::
>     -+--in-reply-to=<Message-Id>::
>     ++--in-reply-to=<message id>::
>       	Make the first mail (or all the mails with `--no-thread`) appear as a
>     - 	reply to the given Message-Id, which avoids breaking threads to
>     +-	reply to the given Message-Id, which avoids breaking threads to
>     ++	reply to the given <message id>, which avoids breaking threads to
>       	provide a new patch series.
>     + 
>     + --ignore-if-in-upstream::
>     +@@ Documentation/git-format-patch.txt: will want to ensure that threading is disabled for `git send-email`.
>     + 	patches being generated, and any patch that matches is
>     + 	ignored.
>     + 
>     +---subject-prefix=<Subject-Prefix>::
>     ++--subject-prefix=<subject prefix>::
>     + 	Instead of the standard '[PATCH]' prefix in the subject
>     +-	line, instead use '[<Subject-Prefix>]'. This
>     ++	line, instead use '[<subject prefix>]'. This
>     + 	allows for useful naming of a patch series, and can be
>     + 	combined with the `--numbered` option.
>     + 
>      @@ Documentation/git-format-patch.txt: you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
>       --base=<commit>::
>       	Record the base tree information to identify the state the
> 13:  7c8522abf2 <  -:  ---------- config/format.txt: specify default value of format.coverLetter
>  -:  ---------- > 13:  4e429e1989 config/format.txt: specify default value of format.coverLetter
> -- 
> 2.23.0.248.g3a9dd8fb08
>
Junio C Hamano Sept. 5, 2019, 7:56 p.m. UTC | #2
Denton Liu <liu.denton@gmail.com> writes:

> Hi Junio,
>
> I see that "dl/format-patch-doc-test-cleanup" currently has the comment
> "Expecting a reroll." This should be the reroll that you're expecting ;)
>
> Also, since there haven't been any comments on the topic in a while, I
> propose that it should be ready for inclusion.

I may be the only person who had issues applying that series from
the list, with mixtures of iso-8859-1 and utf-8 causing troubles,
but if I am not alone, I suspect that the reason why nobody gave a
comment is because the patches did not even apply so there is
nothing to base their comments on.

I wiggled them and compared the result.  The range diff against what
has been queued seems a bit different from what you gave below
(e.g. I see log message got modified on patch #2 and the dropping of
the comma made it harder to read), but the endpoint diff looks not
too bad (IOW, the alloted time for the topic ran out before I
started looking at each individual patches in more depth).
Denton Liu Sept. 5, 2019, 9:40 p.m. UTC | #3
On Thu, Sep 05, 2019 at 12:56:06PM -0700, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
> 
> > Hi Junio,
> >
> > I see that "dl/format-patch-doc-test-cleanup" currently has the comment
> > "Expecting a reroll." This should be the reroll that you're expecting ;)
> >
> > Also, since there haven't been any comments on the topic in a while, I
> > propose that it should be ready for inclusion.
> 
> I may be the only person who had issues applying that series from
> the list, with mixtures of iso-8859-1 and utf-8 causing troubles,
> but if I am not alone, I suspect that the reason why nobody gave a
> comment is because the patches did not even apply so there is
> nothing to base their comments on.

Which patches weren't applying properly? I managed to apply both the
patchset I had locally and a fresh one I downloaded from public-inbox
and both applied cleanly.

> 
> I wiggled them and compared the result.  The range diff against what
> has been queued seems a bit different from what you gave below
> (e.g. I see log message got modified on patch #2 and the dropping of
> the comma made it harder to read), but the endpoint diff looks not
> too bad (IOW, the alloted time for the topic ran out before I
> started looking at each individual patches in more depth).

Hmmm, I don't think my workflow uses your topic branches properly. I've
been range-diffing against the previously submitted patchsets but it
seems like you expect a range-diff against the actual topic branch.

What should the ideal workflow be? I've been avoiding working directly
from the topic branch since that would require me to manually remove
your SOB line whenever I generate new patchsets. I guess I could
manually remove your SOB line from each patch manually. I dunno. Any
ideas?

Thanks,

Denton