Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
Hi Andrei, On Thu, 26 Jul 2018, Andrei Rybak wrote: > On 2018-05-30 10:03, Eric Sunshine wrote: > > Dscho recently implemented a 'tbdiff' replacement as a Git builtin named > > git-branch-diff[1] which computes differences between two versions of a > > patch series. Such a diff can be a useful aid for reviewers when > > inserted into a cover letter. However, doing so requires manual > > generation (invoking git-branch-diff) and copy/pasting the result into > > the cover letter. > > > > This series fully automates the process by adding a --range-diff option > > to git-format-patch. > > [...] > > > > > Eric Sunshine (5): > > format-patch: allow additional generated content in > > make_cover_letter() > > format-patch: add --range-diff option to embed diff in cover letter > > format-patch: extend --range-diff to accept revision range > > format-patch: teach --range-diff to respect -v/--reroll-count > > format-patch: add --creation-weight tweak for --range-diff > > > > Documentation/git-format-patch.txt | 18 + > > builtin/log.c | 119 - > > t/t7910-branch-diff.sh | 16 > > 3 files changed, 132 insertions(+), 21 deletions(-) > > Would it make sense to mention new option in the cover letter section of > Documentation/SubmittingPatches? I would be hesitant to add it there already. Let's prove first that these options are really as useful as we hope they are. It might turn out that in many cases, the range-diff is just stupidly unreadable, for example. Personally, I already miss the color coding when looking at range-diffs sent via mails. Ciao, Johannes
Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
On 2018-05-30 10:03, Eric Sunshine wrote: > Dscho recently implemented a 'tbdiff' replacement as a Git builtin named > git-branch-diff[1] which computes differences between two versions of a > patch series. Such a diff can be a useful aid for reviewers when > inserted into a cover letter. However, doing so requires manual > generation (invoking git-branch-diff) and copy/pasting the result into > the cover letter. > > This series fully automates the process by adding a --range-diff option > to git-format-patch. [...] > > Eric Sunshine (5): > format-patch: allow additional generated content in > make_cover_letter() > format-patch: add --range-diff option to embed diff in cover letter > format-patch: extend --range-diff to accept revision range > format-patch: teach --range-diff to respect -v/--reroll-count > format-patch: add --creation-weight tweak for --range-diff > > Documentation/git-format-patch.txt | 18 + > builtin/log.c | 119 - > t/t7910-branch-diff.sh | 16 > 3 files changed, 132 insertions(+), 21 deletions(-) Would it make sense to mention new option in the cover letter section of Documentation/SubmittingPatches? -- Best regards, Andrei Rybak
Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
Hi Eric, On Wed, 30 May 2018, Eric Sunshine wrote: > Dscho recently implemented a 'tbdiff' replacement as a Git builtin named > git-branch-diff[1] which computes differences between two versions of a > patch series. Such a diff can be a useful aid for reviewers when > inserted into a cover letter. However, doing so requires manual > generation (invoking git-branch-diff) and copy/pasting the result into > the cover letter. > > This series fully automates the process by adding a --range-diff option > to git-format-patch. Nice! > It is RFC for a couple reasons: > > * The final name for the 'tbdiff' replacement has not yet been nailed > down. The name git-branch-diff is moribund[2]; Dscho favors merging > the functionality into git-branch as a new --diff option[3]; others > prefer a standalone command named git-range-diff or > git-series-diff[4] or similar. I think this has been settled now: range-diff it is. > * I have some ideas for future enhancements and want to be careful not > to lock in a UI which doesn't mesh well with them (though I think the > current UI turned out reasonably well). First, I foresee a > complementary --interdiff option for inserting an interdiff-style diff > for cases when that style is easier to read or simply more > appropriate. Second, although the current patch series only supports > --range-diff for the cover letter, some people insert interdiff- or > tbdiff-style diffs (indented) into the commentary section of > standalone patches. Although this often makes for a noisy mess, it is > periodically useful. Sure. > This series is built atop js/branch-diff in 'pu'. > > [1]: > https://public-inbox.org/git/cover.1525448066.git.johannes.schinde...@gmx.de/ > [2]: > https://public-inbox.org/git/nycvar.qro.7.76.6.1805061401260...@tvgsbejvaqbjf.bet/ > [3]: > https://public-inbox.org/git/nycvar.qro.7.76.6.1805062155120...@tvgsbejvaqbjf.bet/ > [4]: https://public-inbox.org/git/xmqqin7gzbkb@gitster-ct.c.googlers.com/ > > Eric Sunshine (5): > format-patch: allow additional generated content in > make_cover_letter() > format-patch: add --range-diff option to embed diff in cover letter > format-patch: extend --range-diff to accept revision range > format-patch: teach --range-diff to respect -v/--reroll-count > format-patch: add --creation-weight tweak for --range-diff > > Documentation/git-format-patch.txt | 18 + > builtin/log.c | 119 - > t/t7910-branch-diff.sh | 16 > 3 files changed, 132 insertions(+), 21 deletions(-) > > -- > 2.17.1.1235.ge295dfb56e Thanks! Dscho
Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
On Thu, Jun 7, 2018 at 10:34 AM, Eric Sunshine wrote: > On Wed, Jun 6, 2018 at 3:16 PM, Duy Nguyen wrote: >> On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine >> wrote: >>> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named >>> git-branch-diff[1] which computes differences between two versions of a >>> patch series. Such a diff can be a useful aid for reviewers when >>> inserted into a cover letter. However, doing so requires manual >>> generation (invoking git-branch-diff) and copy/pasting the result into >>> the cover letter. >> >> Another option which I wanted to go is delegate part of cover letter >> generation to a hook (or just a config key that contains a shell >> command). This way it's easier to customize cover letters. We could >> still have a good fallback that does shortlog, diffstat and tbdiff. > > It is common on this mailing list to turn down requests for new hooks > when the requested functionality could just as easily be implemented > via a wrapper script. So, my knee-jerk reaction is that a hook to > customize the cover letter may be overkill when the same functionality > could likely be implemented relatively easily by a shell script which > invokes git-format-patch and customizes the cover letter > after-the-fact. Same argument regarding a config key holding a shell > command. But, perhaps there are cases which don't occur to me which > could be helped by a config variable or such. I think format-patch --cover-letter nowadays does more stuff that's not so easy to simply rewrite it in a shell script. My original problem with format-patch is it hard codes shortlog settings and you can't list patches with patch number (e.g. "[1/2] foo bar"). The simplest way is let format-patch does it stuff as usual and "outsource" some cover letter's body generation to a script. But it's ok. I could try to code the patch numbering thing in format-patch and maybe submit a patch or two for that later. > Of course, by the same reasoning, the --range-diff functionality > implemented by this patch series, which is just a convenience, could > be handled by a wrapper script, thus is not strictly needed. On the > other hand, given that interdiffs and range-diffs are so regularly > used in re-rolls on this list (and perhaps other mailing list-based > projects) may be argument enough in favor of having such an option > built into git-format-patch. -- Duy
Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
On Wed, Jun 6, 2018 at 3:16 PM, Duy Nguyen wrote: > On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine > wrote: >> Dscho recently implemented a 'tbdiff' replacement as a Git builtin named >> git-branch-diff[1] which computes differences between two versions of a >> patch series. Such a diff can be a useful aid for reviewers when >> inserted into a cover letter. However, doing so requires manual >> generation (invoking git-branch-diff) and copy/pasting the result into >> the cover letter. > > Another option which I wanted to go is delegate part of cover letter > generation to a hook (or just a config key that contains a shell > command). This way it's easier to customize cover letters. We could > still have a good fallback that does shortlog, diffstat and tbdiff. It is common on this mailing list to turn down requests for new hooks when the requested functionality could just as easily be implemented via a wrapper script. So, my knee-jerk reaction is that a hook to customize the cover letter may be overkill when the same functionality could likely be implemented relatively easily by a shell script which invokes git-format-patch and customizes the cover letter after-the-fact. Same argument regarding a config key holding a shell command. But, perhaps there are cases which don't occur to me which could be helped by a config variable or such. Of course, by the same reasoning, the --range-diff functionality implemented by this patch series, which is just a convenience, could be handled by a wrapper script, thus is not strictly needed. On the other hand, given that interdiffs and range-diffs are so regularly used in re-rolls on this list (and perhaps other mailing list-based projects) may be argument enough in favor of having such an option built into git-format-patch.
Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
On Wed, May 30, 2018 at 10:03 AM, Eric Sunshine wrote: > Dscho recently implemented a 'tbdiff' replacement as a Git builtin named > git-branch-diff[1] which computes differences between two versions of a > patch series. Such a diff can be a useful aid for reviewers when > inserted into a cover letter. However, doing so requires manual > generation (invoking git-branch-diff) and copy/pasting the result into > the cover letter. Another option which I wanted to go is delegate part of cover letter generation to a hook (or just a config key that contains a shell command). This way it's easier to customize cover letters. We could still have a good fallback that does shortlog, diffstat and tbdiff. -- Duy
Re: [RFC PATCH 0/5] format-patch: automate cover letter range-diff
On Wed, May 30 2018, Eric Sunshine wrote: > * The final name for the 'tbdiff' replacement has not yet been nailed > down. The name git-branch-diff is moribund[2]; Dscho favors merging > the functionality into git-branch as a new --diff option[3]; others > prefer a standalone command named git-range-diff or > git-series-diff[4] or similar. FWIW Dscho in an IRC conversation on May 25th seems to have settled on calling it "git range-diff ". He just hasn't gotten around to submitting a new version.
[RFC PATCH 0/5] format-patch: automate cover letter range-diff
Dscho recently implemented a 'tbdiff' replacement as a Git builtin named git-branch-diff[1] which computes differences between two versions of a patch series. Such a diff can be a useful aid for reviewers when inserted into a cover letter. However, doing so requires manual generation (invoking git-branch-diff) and copy/pasting the result into the cover letter. This series fully automates the process by adding a --range-diff option to git-format-patch. It is RFC for a couple reasons: * The final name for the 'tbdiff' replacement has not yet been nailed down. The name git-branch-diff is moribund[2]; Dscho favors merging the functionality into git-branch as a new --diff option[3]; others prefer a standalone command named git-range-diff or git-series-diff[4] or similar. * I have some ideas for future enhancements and want to be careful not to lock in a UI which doesn't mesh well with them (though I think the current UI turned out reasonably well). First, I foresee a complementary --interdiff option for inserting an interdiff-style diff for cases when that style is easier to read or simply more appropriate. Second, although the current patch series only supports --range-diff for the cover letter, some people insert interdiff- or tbdiff-style diffs (indented) into the commentary section of standalone patches. Although this often makes for a noisy mess, it is periodically useful. This series is built atop js/branch-diff in 'pu'. [1]: https://public-inbox.org/git/cover.1525448066.git.johannes.schinde...@gmx.de/ [2]: https://public-inbox.org/git/nycvar.qro.7.76.6.1805061401260...@tvgsbejvaqbjf.bet/ [3]: https://public-inbox.org/git/nycvar.qro.7.76.6.1805062155120...@tvgsbejvaqbjf.bet/ [4]: https://public-inbox.org/git/xmqqin7gzbkb@gitster-ct.c.googlers.com/ Eric Sunshine (5): format-patch: allow additional generated content in make_cover_letter() format-patch: add --range-diff option to embed diff in cover letter format-patch: extend --range-diff to accept revision range format-patch: teach --range-diff to respect -v/--reroll-count format-patch: add --creation-weight tweak for --range-diff Documentation/git-format-patch.txt | 18 + builtin/log.c | 119 - t/t7910-branch-diff.sh | 16 3 files changed, 132 insertions(+), 21 deletions(-) -- 2.17.1.1235.ge295dfb56e
