[libvirt] [PATCH] docs: add some more hacking tips
Based on a suggestion by John Ferlan: https://www.redhat.com/archives/libvir-list/2013-January/msg00158.html * docs/hacking.html.in: Add some commit message instructions. Mention the ./run script. * HACKING: Regenerate. --- I don't know how to make docs/hacking2.xsl delay the line wrapping until after the '(3) ' prefix due to the <li> is inserted; as a result, there is a line longer than 80 columns. Suggestions on improving the template are welcome. HACKING | 26 ++++++++++++++++++++++---- docs/hacking.html.in | 19 ++++++++++++++++++- 2 files changed, 40 insertions(+), 5 deletions(-) diff --git a/HACKING b/HACKING index 0ee988a..1b4dea3 100644 --- a/HACKING +++ b/HACKING @@ -58,7 +58,19 @@ though). -(3) Split large changes into a series of smaller patches, self-contained if +(3) In your commit message, make the summary line reasonably short (60 characters +is typical), followed by a blank line, followed by any longer description of +why your patch makes sense. If the patch fixes a regression, and you know what +commit introduced the problem, mentioning that is useful. If the patch +resolves a bugzilla report, mentioning the URL of the bug number is useful; +but also summarize the issue rather than making all readers follow the link. +You can use 'git shortlog -30' to get an idea of typical summary lines. +Libvirt does not currently attach any meaning to Signed-off-by: lines, so it +is up to you if you want to include or omit them in the commit message. + + + +(4) Split large changes into a series of smaller patches, self-contained if possible, with an explanation of each patch and an explanation of how the sequence of patches fits together. Moreover, please keep in mind that it's required to be able to compile cleanly (*including* "make check" and "make @@ -69,10 +81,10 @@ things). -(4) Make sure your patches apply against libvirt GIT. Developers only follow GIT +(5) Make sure your patches apply against libvirt GIT. Developers only follow GIT and don't care much about released versions. -(5) Run the automated tests on your code before submitting any changes. In +(6) Run the automated tests on your code before submitting any changes. In particular, configure with compile warnings set to -Werror. This is done automatically for a git checkout; from a tarball, use: @@ -97,7 +109,13 @@ Also, individual tests can be run from inside the "tests/" directory, like: ./qemuxml2xmltest -(6) Update tests and/or documentation, particularly if you are adding a new +There is also a "./run" script at the top level, to make it easier to run +uninstalled programs, as well as to wrap invocations of various tests under +gdb or valgrind. + + + +(7) Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program. diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 40acdbb..17136f0 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -59,6 +59,21 @@ version if needed though).</p> </li> + <li><p>In your commit message, make the summary line reasonably + short (60 characters is typical), followed by a blank line, + followed by any longer description of why your patch makes + sense. If the patch fixes a regression, and you know what + commit introduced the problem, mentioning that is useful. + If the patch resolves a bugzilla report, mentioning the URL + of the bug number is useful; but also summarize the issue + rather than making all readers follow the link. You can use + 'git shortlog -30' to get an idea of typical summary lines. + Libvirt does not currently attach any meaning to + Signed-off-by: lines, so it is up to you if you want to + include or omit them in the commit message. + </p> + </li> + <li><p>Split large changes into a series of smaller patches, self-contained if possible, with an explanation of each patch and an explanation of how the sequence of patches fits @@ -110,7 +125,9 @@ <pre> ./qemuxml2xmltest </pre> - + <p>There is also a <code>./run</code> script at the top level, + to make it easier to run uninstalled programs, as well as to + wrap invocations of various tests under gdb or valgrind.</p> </li> <li>Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program.</li> -- 1.8.0.2
I've noticed a number of people sending patches with file renames not compressed, so we might as well document how to set this up. (Git won't do it by default, for back-compat reasons) * docs/hacking.html.in: Add git config tip. * HACKING: Regenerate. --- HACKING | 7 ++++++- docs/hacking.html.in | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-) diff --git a/HACKING b/HACKING index 1b4dea3..11f08ff 100644 --- a/HACKING +++ b/HACKING @@ -14,7 +14,12 @@ General tips for contributing patches (1) Discuss any large changes on the mailing list first. Post patches early and listen to feedback. -(2) Post patches in unified diff format. A command similar to this should work: +(2) Post patches in unified diff format, with git rename detection enabled. You +need a one-time setup of: + + git config diff.renames true + +After that, a command similar to this should work: diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 17136f0..69520f0 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -9,8 +9,12 @@ <li>Discuss any large changes on the mailing list first. Post patches early and listen to feedback.</li> - <li><p>Post patches in unified diff format. A command similar to this - should work:</p> + <li><p>Post patches in unified diff format, with git rename + detection enabled. You need a one-time setup of:</p> +<pre> + git config diff.renames true +</pre> + <p>After that, a command similar to this should work:</p> <pre> diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch </pre> -- 1.8.0.2
On 01/11/13 01:40, Eric Blake wrote:
I've noticed a number of people sending patches with file renames not compressed, so we might as well document how to set this up. (Git won't do it by default, for back-compat reasons)
* docs/hacking.html.in: Add git config tip. * HACKING: Regenerate. --- HACKING | 7 ++++++- docs/hacking.html.in | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-)
[...]
diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 17136f0..69520f0 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -9,8 +9,12 @@ <li>Discuss any large changes on the mailing list first. Post patches early and listen to feedback.</li>
- <li><p>Post patches in unified diff format. A command similar to this - should work:</p> + <li><p>Post patches in unified diff format, with git rename + detection enabled. You need a one-time setup of:</p> +<pre> + git config diff.renames true +</pre> + <p>After that, a command similar to this should work:</p> <pre> diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch </pre>
It might also be beneficial to instruct the users to use the patience algorithm for diffs (especially when moving chunks of code around). But the issue with that is that it can't be enabled in the config (well apart from using an alias). ACK as is. Peter
On 11.01.2013 09:34, Peter Krempa wrote:
On 01/11/13 01:40, Eric Blake wrote:
I've noticed a number of people sending patches with file renames not compressed, so we might as well document how to set this up. (Git won't do it by default, for back-compat reasons)
* docs/hacking.html.in: Add git config tip. * HACKING: Regenerate. --- HACKING | 7 ++++++- docs/hacking.html.in | 8 ++++++-- 2 files changed, 12 insertions(+), 3 deletions(-)
[...]
diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 17136f0..69520f0 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -9,8 +9,12 @@ <li>Discuss any large changes on the mailing list first. Post patches early and listen to feedback.</li>
- <li><p>Post patches in unified diff format. A command similar to this - should work:</p> + <li><p>Post patches in unified diff format, with git rename + detection enabled. You need a one-time setup of:</p> +<pre> + git config diff.renames true +</pre> + <p>After that, a command similar to this should work:</p> <pre> diff -urp libvirt.orig/ libvirt.modified/ > libvirt-myfeature.patch </pre>
It might also be beneficial to instruct the users to use the patience algorithm for diffs (especially when moving chunks of code around). But the issue with that is that it can't be enabled in the config (well apart from using an alias).
OFF TOPIC I was trying to get patch in the git that does just this. I wonder where it got stuck. Gonna resend - thanks for reminding. Michal
On 01/11/2013 01:34 AM, Peter Krempa wrote:
It might also be beneficial to instruct the users to use the patience algorithm for diffs (especially when moving chunks of code around).
Indeed.
ACK as is.
I tweaked before pushing: diff --git i/docs/hacking.html.in w/docs/hacking.html.in index 1216f42..6485676 100644 --- i/docs/hacking.html.in +++ w/docs/hacking.html.in @@ -24,7 +24,9 @@ <pre> git diff > libvirt-myfeature.patch </pre> - <p>However, the usual workflow of libvirt developer is:</p> + <p>Also, for code motion patches, you may find that <code>git + diff --patience</code> provides an easier-to-read patch. + However, the usual workflow of libvirt developer is:</p> <pre> git checkout master git pull -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
On 01/04/13 18:29, Eric Blake wrote:
Based on a suggestion by John Ferlan: https://www.redhat.com/archives/libvir-list/2013-January/msg00158.html
* docs/hacking.html.in: Add some commit message instructions. Mention the ./run script. * HACKING: Regenerate. ---
I don't know how to make docs/hacking2.xsl delay the line wrapping until after the '(3) ' prefix due to the <li> is inserted; as a result, there is a line longer than 80 columns. Suggestions on improving the template are welcome.
Hm, I think of the 80 column boundary as a nice limit to start to think how to break the line. Today almost everybody has the fancy widescreen monitors so it's not that of an issue. Having a few characters more in a line isn't in my opinion that bad as breaking a line due to that. (It bothers me the most mostl if a line has 81 columns.). Well but rules are rules.
HACKING | 26 ++++++++++++++++++++++---- docs/hacking.html.in | 19 ++++++++++++++++++- 2 files changed, 40 insertions(+), 5 deletions(-)
[...]
diff --git a/docs/hacking.html.in b/docs/hacking.html.in index 40acdbb..17136f0 100644 --- a/docs/hacking.html.in +++ b/docs/hacking.html.in @@ -59,6 +59,21 @@ version if needed though).</p> </li>
+ <li><p>In your commit message, make the summary line reasonably + short (60 characters is typical), followed by a blank line, + followed by any longer description of why your patch makes + sense. If the patch fixes a regression, and you know what + commit introduced the problem, mentioning that is useful. + If the patch resolves a bugzilla report, mentioning the URL + of the bug number is useful; but also summarize the issue + rather than making all readers follow the link. You can use + 'git shortlog -30' to get an idea of typical summary lines. + Libvirt does not currently attach any meaning to + Signed-off-by: lines, so it is up to you if you want to + include or omit them in the commit message. + </p> + </li> + <li><p>Split large changes into a series of smaller patches, self-contained if possible, with an explanation of each patch and an explanation of how the sequence of patches fits @@ -110,7 +125,9 @@ <pre> ./qemuxml2xmltest </pre> - + <p>There is also a <code>./run</code> script at the top level, + to make it easier to run uninstalled programs, as well as to
"uninstalled" sounds to me as if I'd just remove them from my system. I'd change that to "programs that have not been installed in the system" or something with similar explanation
+ wrap invocations of various tests under gdb or valgrind.</p> </li> <li>Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program.</li>
ACK with the comment in the last hunk addressed (as the first one is just a meaningless rant :) ). Peter
On 01/11/2013 01:31 AM, Peter Krempa wrote:
+ <p>There is also a <code>./run</code> script at the top level, + to make it easier to run uninstalled programs, as well as to
"uninstalled" sounds to me as if I'd just remove them from my system. I'd change that to "programs that have not been installed in the system" or something with similar explanation
+ wrap invocations of various tests under gdb or valgrind.</p> </li> <li>Update tests and/or documentation, particularly if you are adding a new feature or changing the output of a program.</li>
ACK with the comment in the last hunk addressed
Done, and pushed. -- Eric Blake eblake redhat com +1-919-301-3266 Libvirt virtualization library http://libvirt.org
participants (3)
-
Eric Blake -
Michal Privoznik -
Peter Krempa