| Message ID | 5353B7F9.4060803@gmail.com |
|---|---|
| State | New |
| Headers | show |
On 04/20/2014 06:05 AM, Chen Gang wrote: > After finish 'writeconfig' to stdout (with '-'), we want to copy/past > the related information mannually, not for redirection ('readconfig' > does not support '-'). > > So we can not close the stdout, or next options which may use stdout > will not be displayed. Grammar, and awkward to read. May I suggest: vl: avoid closing stdout with 'writeconfig' 'writeconfig' supports output to stdout (with '-'); when that happens, we must not close stdout, or further command line options that also use stdout will be impacted. (Although 'writeconfig' was copied from 'readconfig', the latter does not have the problem because it does not support reading from '-') > > > Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> > --- > vl.c | 4 +++- > 1 file changed, 3 insertions(+), 1 deletion(-) With the improved commit message, Reviewed-by: Eric Blake <eblake@redhat.com>
On 04/21/2014 11:09 PM, Eric Blake wrote: > On 04/20/2014 06:05 AM, Chen Gang wrote: >> After finish 'writeconfig' to stdout (with '-'), we want to copy/past >> the related information mannually, not for redirection ('readconfig' >> does not support '-'). >> >> So we can not close the stdout, or next options which may use stdout >> will not be displayed. > > Grammar, and awkward to read. May I suggest: > > vl: avoid closing stdout with 'writeconfig' > > 'writeconfig' supports output to stdout (with '-'); when that happens, > we must not close stdout, or further command line options that also use > stdout will be impacted. (Although 'writeconfig' was copied from > 'readconfig', the latter does not have the problem because it does not > support reading from '-') > >> >> >> Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> >> --- >> vl.c | 4 +++- >> 1 file changed, 3 insertions(+), 1 deletion(-) > > With the improved commit message, > > Reviewed-by: Eric Blake <eblake@redhat.com> > > OK, it sounds fine to me, I shall send patch v2 for it. Thanks.
-----BEGIN PGP SIGNED MESSAGE----- Hash: SHA1 Am 21.04.2014 17:09, schrieb Eric Blake: > On 04/20/2014 06:05 AM, Chen Gang wrote: >> After finish 'writeconfig' to stdout (with '-'), we want to >> copy/past the related information mannually, not for redirection >> ('readconfig' does not support '-'). >> >> So we can not close the stdout, or next options which may use >> stdout will not be displayed. > > Grammar, and awkward to read. May I suggest: > > vl: avoid closing stdout with 'writeconfig' > > 'writeconfig' supports output to stdout (with '-'); when that > happens, we must not close stdout, or further command line options > that also use stdout will be impacted. (Although 'writeconfig' was > copied from 'readconfig', the latter does not have the problem > because it does not support reading from '-') Eric, did you intentionally request to change "Avoid" to lowercase? I was once pointed to https://wiki.gnome.org/Git/CommitMessages as a template, and starting an English-language heading with an uppercase letter seems natural, with anything else being too lazy to press Shift. Personally I find -writeconfig the most straightforward way to indicate it's an option, just like () makes clear something is a function. The original subject had an explicit "option", that got lost for v2. Regards, Andreas - -- SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg -----BEGIN PGP SIGNATURE----- Version: GnuPG v2.0.22 (GNU/Linux) iQIcBAEBAgAGBQJTVpNxAAoJEPou0S0+fgE/NBEP/RbA40p75QIWuDBM31PXXZNo lrIxfLkb9QVvN7ALUsGOEn6Y1bNQWuItfq5YBN0oDDBxWKSHaU2ou8M3TUWIX6S8 ENLPFJzsVGbzsb4pkP0yMMWZdbs+8AChDJ+oZxgtz6eBf90ERYLarL/t8UjnZos4 wKU3alo2j9xf2OfMpqEHvtxcFc9URDZWKjElPLkZDPn5rFEwcaVGu78rDtjNQIU4 z7Ebc7li29Yppz9MrQ6QoXtU5ZGygPlzZ90G5UZc4v0H0u4ObyYInrWbxqryVp2H k+X173VVpJtPzlhzDkBJPdnwqhoExq7x/n2ujZicRyL95+8ZeoH2kteMRP0eHZYV TDLOkIq2FXklccn91FVVCk6GfLKLdcgZMotaxYD1EIxp7I9NlzpB0d1EVCgClWMx AgZ6StOhYt4+72BaLDOT602FHdsVSEnCqsQeBxoDhKWLwKLklKi59vzDGn+wH0Ap fY33zIW4+YAiXDv3vQg+1k8iG7BauO3cHDVQfbRMzU5zgWORMsm1lsnEv0b3x9YH PhuG9Ve9J2zrbBQEBe0zx98CyB0tp8L/8w5TUVD3EEmQA9YZP5y7I8WU8mOnvgRa Rpu3JF6U3KBO99VF0Yyd3K1mTcG4yJzGfxdLdiTIkSDbEV/jc5aoJFltldZWWpsl DigKmE0VxCFolXJsIsiX =810z -----END PGP SIGNATURE-----
On 04/22/2014 10:06 AM, Andreas Färber wrote: >> Grammar, and awkward to read. May I suggest: > >> vl: avoid closing stdout with 'writeconfig' > > Eric, did you intentionally request to change "Avoid" to lowercase? Yes, out of habit, and in comparison to many recent commits. In fact, looking at the current qemu.git head 'git shortlog -100 2d03b49c' shows: 3 commits matching '\.$', 97 without 75 commits matching ': [A-Z]', 24 matching ': [a-z]' (some overlap here, and merge commits don't match either) extending further to 1000 commits: 21 commits with trailing dot, 979 without 644 matching ': [A-Z]', 260 matching ': [a-z]' at 10000, things are a bit more even: 5187 matching ': [A-Z]', 3904 matching ': [a-z]' but still a distinct leaning towards capital at the start of the message > I was once pointed to https://wiki.gnome.org/Git/CommitMessages as a > template, and starting an English-language heading with an uppercase > letter seems natural, with anything else being too lazy to press Shift. Hmm, maybe we should update the qemu wiki? http://wiki.qemu.org/Contribute/SubmitAPatch doesn't mention the qemu preferred style (you pointed to the GNOME style, but at least GNU coreutils explicitly prefers lowercase). At any rate, I'm not going to reject a patch based on capitalization, and can retrain my fingers to use a capital if that is the documented preference. > Personally I find -writeconfig the most straightforward way to > indicate it's an option, just like () makes clear something is a > function. The original subject had an explicit "option", that got lost > for v2. Good call, so maybe: vl: Avoid closing stdout with '-writeconfig'
On 22 April 2014 17:31, Eric Blake <eblake@redhat.com> wrote: > Hmm, maybe we should update the qemu wiki? > http://wiki.qemu.org/Contribute/SubmitAPatch doesn't mention the qemu > preferred style (you pointed to the GNOME style, but at least GNU > coreutils explicitly prefers lowercase). > > At any rate, I'm not going to reject a patch based on capitalization, Yeah, it doesn't really seem worth trying to enforce niceties of style here -- the effort required doesn't seem to really match the benefit, and we already have a lot of hoops we make new submitters jump through. "[file or area being patched]: [reasonably descriptive short summary]" is about the limit I personally feel comfortable insisting on. thanks -- PMM
On 04/22/2014 01:18 PM, Peter Maydell wrote: > On 22 April 2014 17:31, Eric Blake <eblake@redhat.com> wrote: >> Hmm, maybe we should update the qemu wiki? >> http://wiki.qemu.org/Contribute/SubmitAPatch doesn't mention the qemu >> preferred style (you pointed to the GNOME style, but at least GNU >> coreutils explicitly prefers lowercase). >> >> At any rate, I'm not going to reject a patch based on capitalization, > > Yeah, it doesn't really seem worth trying to enforce niceties > of style here -- the effort required doesn't seem to really match > the benefit, and we already have a lot of hoops we make new > submitters jump through. "[file or area being patched]: [reasonably > descriptive short summary]" is about the limit I personally feel > comfortable insisting on. So, given this discussion, I enhanced the existing paragraph in the wiki page mentioned above to include a couple more sentences: '''Write a good commit message'''. QEMU follows the usual standard for git commit messages: the first line (which becomes the email subject line) is "subsystem: single line summary of change". Whether the "single line summary of change" starts with a capital is a matter of taste, but we prefer that the summary does not end in ".". Look at <code>git short-log 30</code> for an idea of sample subject lines. Then there is a blank line and a more detailed description of the patch, another blank and your Signed-off-by: line. The body of the commit message is a good place to document why your change is important. Don't include comments like "This is a suggestion for fixing this bug" (they can go below the "---" line in the email so they don't go into the final commit message).
diff --git a/vl.c b/vl.c index 9975e5a..215467f 100644 --- a/vl.c +++ b/vl.c @@ -3855,7 +3855,9 @@ int main(int argc, char **argv, char **envp) } } qemu_config_write(fp); - fclose(fp); + if (fp != stdout) { + fclose(fp); + } break; } case QEMU_OPTION_qtest:
After finish 'writeconfig' to stdout (with '-'), we want to copy/past the related information mannually, not for redirection ('readconfig' does not support '-'). So we can not close the stdout, or next options which may use stdout will not be displayed. Signed-off-by: Chen Gang <gang.chen.5i5j@gmail.com> --- vl.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)