[dpdk-dev] [Process] Git Fixline Suggestion

[dpdk-dev] [Process] Git Fixline Suggestion

[Van Haaren, Harry]

Hi All,

I propose to add CC: <author> to the output of the git fixline command[1].

The logic here is that if it is a fix to the previous code, the original author of that code should be informed. Currently the author must read the mailing-list carefully in order to ensure that no patches are missed, that fix code that they are the author of.

The new workflow is the same as before: copy the output of "git fixline" into the commit message. The output would now be two lines as follows:

Fixes: 17b8320a3e11 ("vhost: remove index parameter")
cc: author@domain.com


The following git fixline achieves this:

git log -1 --abbrev=12 --format="Fixes: %h (\"%s\")%nCC:%ae"


Opinions? -Harry


[1] http://dpdk.org/doc/guides/contributing/patches.html#commit-messages-body

Re: [dpdk-dev] [Process] Git Fixline Suggestion

[Thomas Monjalon]

12/07/2017 10:40, Van Haaren, Harry:
> Hi All,
> 
> I propose to add CC: <author> to the output of the git fixline command[1].
> 
> The logic here is that if it is a fix to the previous code, the original author of that code should be informed. Currently the author must read the mailing-list carefully in order to ensure that no patches are missed, that fix code that they are the author of.
> 
> The new workflow is the same as before: copy the output of "git fixline" into the commit message. The output would now be two lines as follows:
> 
> Fixes: 17b8320a3e11 ("vhost: remove index parameter")
> cc: author@domain.com
> 
> 
> The following git fixline achieves this:
> 
> git log -1 --abbrev=12 --format="Fixes: %h (\"%s\")%nCC:%ae"
> 
> 
> Opinions? -Harry

Good idea.
Two comments:

- minor nit: CC should be Cc to be consistent with what is suggested
by check-git-log.sh for stable@dpdk.org.

- Do we want to keep this Cc in the git history or is it just
to notify people on the mailing list?
I think it is good to keep in history the tags Acked-by: or Reviewed-by:.
But if the author does not review the fix, I do not see the benefit of
saving his name in the history.

Re: [dpdk-dev] [Process] Git Fixline Suggestion

[Van Haaren, Harry]

> From: Thomas Monjalon [mailto:thomas@monjalon.net]
> Sent: Wednesday, July 12, 2017 11:50 AM
> To: Van Haaren, Harry <harry.van.haaren@intel.com>
> Cc: dev@dpdk.org
> Subject: Re: [dpdk-dev] [Process] Git Fixline Suggestion
> 
> 12/07/2017 10:40, Van Haaren, Harry:
> > Hi All,
> >
> > I propose to add CC: <author> to the output of the git fixline command[1].
> >
> > The logic here is that if it is a fix to the previous code, the original author of that
> code should be informed. Currently the author must read the mailing-list carefully in
> order to ensure that no patches are missed, that fix code that they are the author of.
> >
> > The new workflow is the same as before: copy the output of "git fixline" into the commit
> message. The output would now be two lines as follows:
> >
> > Fixes: 17b8320a3e11 ("vhost: remove index parameter")
> > cc: author@domain.com
> >
> >
> > The following git fixline achieves this:
> >
> > git log -1 --abbrev=12 --format="Fixes: %h (\"%s\")%nCC:%ae"
> >
> >
> > Opinions? -Harry
> 
> Good idea.
> Two comments:
> 
> - minor nit: CC should be Cc to be consistent with what is suggested
> by check-git-log.sh for stable@dpdk.org.

Sure.

> - Do we want to keep this Cc in the git history or is it just
> to notify people on the mailing list?
> I think it is good to keep in history the tags Acked-by: or Reviewed-by:.
> But if the author does not review the fix, I do not see the benefit of
> saving his name in the history.

Agreed.


I'll send a patch in-reply-to this message to update contributing guidelines.

[dpdk-dev] [PATCH] doc: add author on cc to git fixline alias

[Harry van Haaren]

With this commit, the correct method to use git fixline to indicate
a fix of a previous commit has changed. The new rules require the
author of the original code that is being fixed to be on CC.

The logic behind this improvement is that if there is a genuine bug,
one of the ideal people to review is the author of the original code
being fixed. Adding them on Cc makes them aware of the patch, avoiding
it from being passed by accidentally while reading the mailing-list.

Given that the original author (now on Cc:) might not actually review,
there is no value in keeping the Cc: in git commit history. If the
original author performs a review, their Reviewed-by: or Acked-by: is
stored in git history (same as now).

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

---

This fixline suggestion was proposed here:
http://dpdk.org/ml/archives/dev/2017-July/071107.html

Based on Thomas' feedback, improved CC: to Cc, and added note about
removing the Cc: from git history.
http://dpdk.org/ml/archives/dev/2017-July/071119.html

---
 doc/guides/contributing/patches.rst | 17 ++++++++++-------
 1 file changed, 10 insertions(+), 7 deletions(-)

diff --git a/doc/guides/contributing/patches.rst b/doc/guides/contributing/patches.rst
index 7926c96..27e218b 100644
--- a/doc/guides/contributing/patches.rst
+++ b/doc/guides/contributing/patches.rst
@@ -207,18 +207,21 @@ Here are some guidelines for the body of a commit message:
 
 * The text of the commit message should be wrapped at 72 characters.
 
-* When fixing a regression, it is a good idea to reference the id of the commit which introduced the bug.
-  You can generate the required text using the following git alias::
+* When fixing a regression, it is required to reference the id of the commit
+  which introduced the bug, and put the original author of that commit on CC.
+  You can generate the required lines using the following git alias, which prints
+  the commit SHA and the author of the original code::
 
-     git config alias.fixline "log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'"
+     git config alias.fixline "log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'"
 
-  The ``Fixes:`` line can then be added to the commit message::
+  The output of ``git fixline <SHA>`` must then be added to the commit message::
 
-     doc: fix vhost sample parameter
+     doc: fix some parameter description
 
-     Update the docs to reflect removed dev-index.
+     Update the docs, fixing description of some parameter.
 
-     Fixes: 17b8320a3e11 ("vhost: remove index parameter")
+     Fixes: abcdefgh1234 ("doc: add some parameter")
+     Cc: author@example.com
 
      Signed-off-by: Alex Smith <alex.smith@example.com>
 
-- 
2.7.4

Re: [dpdk-dev] [PATCH] doc: add author on cc to git fixline alias

[Adrien Mazarguil]

On Wed, Jul 12, 2017 at 02:38:52PM +0100, Harry van Haaren wrote:
> With this commit, the correct method to use git fixline to indicate
> a fix of a previous commit has changed. The new rules require the
> author of the original code that is being fixed to be on CC.
> 
> The logic behind this improvement is that if there is a genuine bug,
> one of the ideal people to review is the author of the original code
> being fixed. Adding them on Cc makes them aware of the patch, avoiding
> it from being passed by accidentally while reading the mailing-list.
> 
> Given that the original author (now on Cc:) might not actually review,
> there is no value in keeping the Cc: in git commit history. If the
> original author performs a review, their Reviewed-by: or Acked-by: is
> stored in git history (same as now).
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

Acked-by: Adrien Mazarguil <adrien.mazarguil@6wind.com>

-- 
Adrien Mazarguil
6WIND

Re: [dpdk-dev] [PATCH] doc: add author on cc to git fixline alias

[Mcnamara, John]

> -----Original Message-----
> From: dev [mailto:dev-bounces@dpdk.org] On Behalf Of Harry van Haaren
> Sent: Wednesday, July 12, 2017 2:39 PM
> To: dev@dpdk.org
> Cc: thomas@monjalon.net; Van Haaren, Harry <harry.van.haaren@intel.com>
> Subject: [dpdk-dev] [PATCH] doc: add author on cc to git fixline alias
> 
> With this commit, the correct method to use git fixline to indicate a fix
> of a previous commit has changed. The new rules require the author of the
> original code that is being fixed to be on CC.
> 
> The logic behind this improvement is that if there is a genuine bug, one
> of the ideal people to review is the author of the original code being
> fixed. Adding them on Cc makes them aware of the patch, avoiding it from
> being passed by accidentally while reading the mailing-list.
> 
> Given that the original author (now on Cc:) might not actually review,
> there is no value in keeping the Cc: in git commit history. If the
> original author performs a review, their Reviewed-by: or Acked-by: is
> stored in git history (same as now).
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

Acked-by: John McNamara <john.mcnamara@intel.com>

Re: [dpdk-dev] [PATCH] doc: add author on cc to git fixline alias

[Thomas Monjalon]

> > With this commit, the correct method to use git fixline to indicate a fix
> > of a previous commit has changed. The new rules require the author of the
> > original code that is being fixed to be on CC.
> > 
> > The logic behind this improvement is that if there is a genuine bug, one
> > of the ideal people to review is the author of the original code being
> > fixed. Adding them on Cc makes them aware of the patch, avoiding it from
> > being passed by accidentally while reading the mailing-list.
> > 
> > Given that the original author (now on Cc:) might not actually review,
> > there is no value in keeping the Cc: in git commit history. If the
> > original author performs a review, their Reviewed-by: or Acked-by: is
> > stored in git history (same as now).
> > 
> > Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
> 
> Acked-by: John McNamara <john.mcnamara@intel.com>

Acked-by: Thomas Monjalon <thomas@monjalon.net>

Applied, thanks

Please update also the alias on the website.

[dpdk-dev] [PATCH] dev: update git fixline alias

[Harry van Haaren]

The git fixline was updated in the dpdk.org repository to add the original
author of the line being "fixed", using Cc:

This patch updates the fixline here in the website. See patch for details:
http://dpdk.org/dev/patchwork/patch/26860/

Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>
---
 dev.html | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/dev.html b/dev.html
index c0e64d0..34f24cf 100644
--- a/dev.html
+++ b/dev.html
@@ -110,7 +110,7 @@
 	smtpserverport = 465
 	smtpencryption = ssl
 [alias]
-	fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")'</pre>
+	fixline = log -1 --abbrev=12 --format='Fixes: %h (\"%s\")%nCc: %ae'</pre>
 
 	<h3 id="review">Contribute by testing or reviewing patches</h3>
 	<p>Patches are applied in the git repository when it becomes clear that
-- 
2.7.4

Re: [dpdk-dev] [PATCH] dev: update git fixline alias

[Thomas Monjalon]

03/08/2017 15:42, Harry van Haaren:
> The git fixline was updated in the dpdk.org repository to add the original
> author of the line being "fixed", using Cc:
> 
> This patch updates the fixline here in the website. See patch for details:
> http://dpdk.org/dev/patchwork/patch/26860/
> 
> Signed-off-by: Harry van Haaren <harry.van.haaren@intel.com>

Applied, thanks