| Message ID | cover.1566878373.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | format-patch: clean up tests and documentation | expand |
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 >
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).
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
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