[Libvir] proposal: remove contradictory indentation directive
Dave Leskovec <dlesko@linux.vnet.ibm.com> wrote:
This is a repost of the start container support. Changes from the last version: ... +/* + * Local variables: + * indent-tabs-mode: nil + * c-indent-level: 4 + * c-basic-offset: 4 + * tab-width: 4 + * End: + */
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces. Just yesterday I experienced first hand how adding code with existing 8-byte tab stops to a file with settings like the above mangles the indentation of the new code. FYI, a better approach would be to ensure that TAB is never used for indentation in a file with "indent-tabs-mode: nil". Both directives are in over 70 files, and I propose to straighten things out. There are many instances of invalid and misleading indentation, in part because of this sort of mix-up. To give an idea of the scope, while 77 files have those settings, removing 'tab-width: 4' would only affect those that use TABs for indentation in spite of "indent-tabs-mode: nil" (perhaps added via copy/paste, or added with a non-emacs editor). Many distro-provided versions of Vim do not honor :se-style settings, due to security concerns. Here are counts of lines with TAB indentation in those 77 files. Each TAB would have to be changed to 4 or 8 spaces, depending on context. That'd be the minimal change. An alternative is simply to filter each of these files through indent with appropriate options. $ git grep -l tab-width:|xargs grep -c '^ '|grep -v :0 proxy/libvirt_proxy.c:316 python/libvir.c:17 src/buf.c:27 src/conf.c:161 src/conf.h:7 src/driver.h:130 src/internal.h:9 src/libvirt.c:81 src/lxc_conf.c:17 src/openvz_driver.h:6 src/proxy_internal.c:122 src/proxy_internal.h:13 src/qemu_conf.c:1 src/qemu_driver.c:5 src/qparams.h:1 src/remote_internal.c:4 src/stats_linux.h:3 src/virsh.c:33 src/virterror.c:166 src/xen_internal.c:105 src/xen_unified.c:25 src/xen_unified.h:37 src/xend_internal.c:43 src/xm_internal.c:25 src/xml.c:6 src/xs_internal.c:45 tests/nodeinfotest.c:1 tests/sexpr2xmltest.c:83
On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces.
Agreed, good idea. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
On Wed, Apr 09, 2008 at 02:40:13PM +0100, Richard W.M. Jones wrote:
On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces.
Agreed, good idea.
my .vimrc defines tabs to be aligned on 8 characters boundaries. i guess i never saw the problem myself, agreed, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
On Wed, Apr 09, 2008 at 02:40:13PM +0100, Richard W.M. Jones wrote:
On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces.
Agreed, good idea.
ACK. Could we get a make syntax-check test to look for these bogus tabs too Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 09, 2008 at 02:40:13PM +0100, Richard W.M. Jones wrote:
On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces.
Agreed, good idea.
ACK.
Could we get a make syntax-check test to look for these bogus tabs too
Of course. These days, any time I make a global change like this, I also make time to ensure that new violations can't sneak back in.
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 09, 2008 at 02:40:13PM +0100, Richard W.M. Jones wrote:
On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces.
Agreed, good idea.
ACK.
Could we get a make syntax-check test to look for these bogus tabs too
I've done that. Abbreviated patches below. However, there are some "issues" to consider. Any global space-changing delta like these is going to cause trouble (conflicts) for people with pending changes and on branches. This one isn't too bad (as these things go), since the TAB-to-space change affects fewer than 1500 lines in 37 files. However, the new rule enforces the coding standard only in files with an existing "indent-tabs-mode: nil" directive. There are currently 42 .[ch] files that don't have such a directive (excluding gnulib/). $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|wc -l 42 If I were to do the same for those remaining files, the TAB-to-space change would modify an additional 2817 lines in 28 files: $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|xargs grep -E '^ * '|wc -l 2817 $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|xargs grep -El '^ * '|wc -l 28 My opinion is that if it's worth doing the first, it's also worth finishing the job, ... but then I don't have any huge re-architecting changes in my queue. However, I don't want to overstate the case either. The cost of a few space-change-provoked merges is pretty low in the grand scheme of things. What say you? Convert 'em all? ----------------------------------------------------------------- Here are the interesting parts of the two changes. First, remove the 'tab-width: 4' directives and make sure they stay gone: remove all 'tab-width: 4' directives Makefile.maint (sc_prohibit_tab_width_4): New rule. Remove all of them, using this command: git grep -l 'tab-width: 4' \ | xargs perl -ni -e '/^\s*\*\s*tab-width:\s*4\s*$/ or print' Signed-off-by: Jim Meyering <meyering@redhat.com> --- Makefile.maint | 9 +++++++++ proxy/libvirt_proxy.c | 1 - ... tests/xmconfigtest.c | 1 - 78 files changed, 9 insertions(+), 77 deletions(-) diff --git a/Makefile.maint b/Makefile.maint index a357d74..fc21bf4 100644 --- a/Makefile.maint +++ b/Makefile.maint @@ -287,6 +287,15 @@ sc_trailing_blank: { echo '$(ME): found trailing blank(s)' \ 1>&2; exit 1; } || : +sp=[ ] +# Some files used to containe a line like this: " * tab-width: 4", +# contradicting an adjacent "indent-tabs-mode: nil" directive. +# Make sure no more are introduced. +sc_prohibit_tab_width_4: + @grep -E '^$(sp)*\*$(sp)*tab-width: 4$$' $$($(VC_LIST_EXCEPT)) && \ + { echo '$(ME): found bogus "tab-width:" directive(s)' \ + 1>&2; exit 1; } || : + # Match lines like the following, but where there is only one space # between the options and the description: # -D, --all-repeated[=delimit-method] print all duplicate lines\n diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c index bed3ae8..b28a3bc 100644 --- a/proxy/libvirt_proxy.c +++ b/proxy/libvirt_proxy.c @@ -857,6 +857,5 @@ int main(void) { * indent-tabs-mode: nil * c-indent-level: 4 * c-basic-offset: 4 - * tab-width: 4 * End: */ diff --git a/python/libvir.c b/python/libvir.c index fe650e8..8ad1122 100644 --- a/python/libvir.c +++ b/python/libvir.c @@ -1464,6 +1464,5 @@ initcygvirtmod * indent-tabs-mode: nil * c-indent-level: 4 * c-basic-offset: 4 - * tab-width: 4 * End: */ ... --------------------------------------------------------------------- Then remove the offending in-indentation-only TABs from files with "indent-tabs-mode: nil": In a file with "indent-tabs-mode: nil", no TABs for indentation. Convert offenders via "expand --initial". * Makefile.maint (sc_TAB_in_indentation): New rule to prevent regression. * proxy/libvirt_proxy.c: Convert indentation TABs to spaces. * python/libvir.c, src/buf.c: src/conf.c, src/conf.h: Likewise. * src/driver.h, src/internal.h, src/libvirt.c, src/lxc_conf.c: Likewise. * src/openvz_driver.h, src/proxy_internal.c: Likewise. * src/proxy_internal.h, src/qemu_conf.c, src/qemu_driver.c: Likewise. * src/qparams.h, src/remote_internal.c, src/stats_linux.h: Likewise. * src/util.c, src/virsh.c, src/virterror.c, src/xen_internal.c: Likewise. * src/xen_unified.c, src/xen_unified.h, src/xend_internal.c: Likewise. * src/xm_internal.c, src/xml.c, src/xs_internal.c: Likewise. * tests/nodeinfotest.c, tests/sexpr2xmltest.c: Likewise. Signed-off-by: Jim Meyering <meyering@redhat.com> --- Makefile.maint | 9 + proxy/libvirt_proxy.c | 652 ++++++++++++++++++++++++------------------------ python/libvir.c | 34 ++-- src/buf.c | 56 ++-- src/conf.c | 324 ++++++++++++------------ src/conf.h | 16 +- src/driver.h | 260 ++++++++++---------- src/internal.h | 18 +- src/libvirt.c | 174 +++++++------- src/lxc_conf.c | 34 ++-- src/openvz_driver.h | 12 +- src/proxy_internal.c | 244 +++++++++--------- src/proxy_internal.h | 26 +- src/qemu_conf.c | 2 +- src/qemu_driver.c | 10 +- src/qparams.h | 2 +- src/remote_internal.c | 8 +- src/stats_linux.h | 6 +- src/util.c | 8 +- src/virsh.c | 66 +++--- src/virterror.c | 356 ++++++++++++++-------------- src/xen_internal.c | 212 ++++++++-------- src/xen_unified.c | 52 ++-- src/xen_unified.h | 74 +++--- src/xend_internal.c | 88 ++++---- src/xm_internal.c | 50 ++-- src/xml.c | 12 +- src/xs_internal.c | 90 ++++---- tests/nodeinfotest.c | 2 +- tests/sexpr2xmltest.c | 166 +++++++------- 30 files changed, 1536 insertions(+), 1527 deletions(-) diff --git a/Makefile.maint b/Makefile.maint index fc21bf4..a22c0f5 100644 --- a/Makefile.maint +++ b/Makefile.maint @@ -296,6 +296,15 @@ sc_prohibit_tab_width_4: { echo '$(ME): found bogus "tab-width:" directive(s)' \ 1>&2; exit 1; } || : +# Ensure that each file containing an "indent-tabs-mode: nil" +# directive uses no TABs for indentation. +sc_TAB_in_indentation: + @grep -lE '^ * ' /dev/null \ + $$(grep -lE '^$(sp)*\*$(sp)*indent-tabs-mode: nil$$' \ + $$($(VC_LIST_EXCEPT))) && \ + { echo '$(ME): found TAB(s) used for indentation' \ + 1>&2; exit 1; } || : + # Match lines like the following, but where there is only one space # between the options and the description: # -D, --all-repeated[=delimit-method] print all duplicate lines\n diff --git a/proxy/libvirt_proxy.c b/proxy/libvirt_proxy.c index b28a3bc..6be3ad8 100644 --- a/proxy/libvirt_proxy.c +++ b/proxy/libvirt_proxy.c @@ -93,10 +93,10 @@ proxyInitXen(void) { return(-1); } else { ret = xenHypervisorGetVersion(conn, &xenVersion); - if (ret != 0) { - fprintf(stderr, "Failed to get Xen hypervisor version\n"); - return(-1); - } + if (ret != 0) { + fprintf(stderr, "Failed to get Xen hypervisor version\n"); + return(-1); + } } ret = xenDaemonOpen_unix(conn, "/var/lib/xend/xend-socket"); if (ret < 0) { @@ -110,12 +110,12 @@ proxyInitXen(void) { } ret = xenDaemonGetVersion(conn, &xenVersion2); if (ret != 0) { - fprintf(stderr, "Failed to get Xen daemon version\n"); - return(-1); + fprintf(stderr, "Failed to get Xen daemon version\n"); + return(-1); } if (debug) fprintf(stderr, "Connected to hypervisor %lu and daemon %lu\n", - xenVersion, xenVersion2); + xenVersion, xenVersion2); if (xenVersion2 > xenVersion) xenVersion = xenVersion2; return(0); ...
On Wed, Apr 09, 2008 at 08:42:01PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 09, 2008 at 02:40:13PM +0100, Richard W.M. Jones wrote:
On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces.
Agreed, good idea.
ACK.
Could we get a make syntax-check test to look for these bogus tabs too
I've done that. Abbreviated patches below.
However, there are some "issues" to consider. Any global space-changing delta like these is going to cause trouble (conflicts) for people with pending changes and on branches. This one isn't too bad (as these things go), since the TAB-to-space change affects fewer than 1500 lines in 37 files. However, the new rule enforces the coding standard only in files with an existing "indent-tabs-mode: nil" directive. There are currently 42 .[ch] files that don't have such a directive (excluding gnulib/).
$ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|wc -l 42
If I were to do the same for those remaining files, the TAB-to-space change would modify an additional 2817 lines in 28 files:
$ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|xargs grep -E '^ * '|wc -l 2817 $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|xargs grep -El '^ * '|wc -l 28
My opinion is that if it's worth doing the first, it's also worth finishing the job, ... but then I don't have any huge re-architecting changes in my queue.
The only changes I have pending are the host device enumeration patches which are all new code, so unaffected, and the serial device support for QEMU which is easy enough to fix-up since its fairly isolated chunks of code. So I think its a net win to fix up everything. Dan. -- |: Red Hat, Engineering, Boston -o- http://people.redhat.com/berrange/ :| |: http://libvirt.org -o- http://virt-manager.org -o- http://ovirt.org :| |: http://autobuild.org -o- http://search.cpan.org/~danberr/ :| |: GnuPG: 7D3B9505 -o- F3C9 553F A1DA 4AC2 5648 23C1 B3DF F742 7D3B 9505 :|
On Wed, 2008-04-09 at 20:42 +0200, Jim Meyering wrote:
Any global space-changing delta like these is going to cause trouble (conflicts) for people with pending changes and on branches.
IMHO, the bigger issue with patches like this is that it makes digging into the history with "cvs annotate" a pain. But then, mixed tabs vs. spaces does annoy the crap out me too ... Cheers, Mark.
Hi Mark, Mark McLoughlin <markmc@redhat.com> wrote:
On Wed, 2008-04-09 at 20:42 +0200, Jim Meyering wrote:
Any global space-changing delta like these is going to cause trouble (conflicts) for people with pending changes and on branches.
IMHO, the bigger issue with patches like this is that it makes digging into the history with "cvs annotate" a pain.
In that case, just use git. [ sorry, I couldn't resist ;-) There is a git mirror of libvirt: git://et.redhat.com/libvirt ] True, a global change like this does obscure the most basic annotate output. Though, as I said, what I'm doing isn't that big -- and nowhere near as bad as filtering all of the code through indent. You probably know, and this is what you mean by "pain", but you can peel off the offending layer by specifying a revision (-r REV) or date (-D date) option.
But then, mixed tabs vs. spaces does annoy the crap out me too ...
On Thu, 2008-04-10 at 09:29 +0200, Jim Meyering wrote:
Mark McLoughlin <markmc@redhat.com> wrote:
On Wed, 2008-04-09 at 20:42 +0200, Jim Meyering wrote:
Any global space-changing delta like these is going to cause trouble (conflicts) for people with pending changes and on branches.
IMHO, the bigger issue with patches like this is that it makes digging into the history with "cvs annotate" a pain.
In that case, just use git.
Fully on-board with that, but it would still be an annoying artifact when poking through history with git too. Less so, sure.
True, a global change like this does obscure the most basic annotate output. Though, as I said, what I'm doing isn't that big -- and nowhere near as bad as filtering all of the code through indent.
Yep, my point was not so much an objection to this particular change, but that if one consistently dismisses the concern about reducing the usefulness of a codebase's history and continue cleanups like this ad-nauseum, then one ends up with near-useless history. (But you must know this - core-utils has plenty of history :-)
You probably know, and this is what you mean by "pain", but you can peel off the offending layer by specifying a revision (-r REV) or date (-D date) option.
Sure. Cheers, Mark.
Mark McLoughlin <markmc@redhat.com> wrote:
On Thu, 2008-04-10 at 09:29 +0200, Jim Meyering wrote:
Mark McLoughlin <markmc@redhat.com> wrote:
On Wed, 2008-04-09 at 20:42 +0200, Jim Meyering wrote:
Any global space-changing delta like these is going to cause trouble (conflicts) for people with pending changes and on branches.
IMHO, the bigger issue with patches like this is that it makes digging into the history with "cvs annotate" a pain.
In that case, just use git.
Fully on-board with that, but it would still be an annoying artifact when poking through history with git too. Less so, sure.
True, a global change like this does obscure the most basic annotate output. Though, as I said, what I'm doing isn't that big -- and nowhere near as bad as filtering all of the code through indent.
Yep, my point was not so much an objection to this particular change, but that if one consistently dismisses the concern about reducing the usefulness of a codebase's history and continue cleanups like this ad-nauseum, then one ends up with near-useless history.
No risk of that, imho. Such clean-ups tend to make the code converge on a well-defined style pretty quickly -- either that or they just stop.
(But you must know this - core-utils has plenty of history :-)
Not too many grey hairs, yet ;-) I've inflicted far worse on a few large, non-public code bases, and in retrospect, these changes have been universally appreciated, mostly because everyone sees and uses the code on the trunk all the time, yet only a few "special" souls end up doing VCS archeology. And even those lucky few learn quickly that it's not hard to skip past any global-changes. Of course, if you have a few active branches, the value of making global changes goes down very quickly. Appearance counts for a _lot_. If the code looks sloppy, or renders strangely, that can discourage people who might otherwise take good care of it, making the sorts of tiny (sometimes nearly pointless) changes that eventually lead to that ideal, perfect-looking project. Most people are surprised to see how many "real" bugs end up being exposed (or avoided, but we can't count *those*) by changes like this.
I also say you should go for the full patch. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-top is 'top' for virtual machines. Tiny program with many powerful monitoring features, net stats, disk stats, logging, etc. http://et.redhat.com/~rjones/virt-top
On Thu, Apr 10, 2008 at 08:42:08AM +0100, Richard W.M. Jones wrote:
I also say you should go for the full patch.
But please keep tabks in ChangeLog for indenting, thanks, Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 08:42:08AM +0100, Richard W.M. Jones wrote:
I also say you should go for the full patch.
But please keep tabks in ChangeLog for indenting,
Of course ;-) There's actually a "make syntax-check" that would barf if I tried expanding *those* TABs. It's in Makefile.maint, and I see that it's disabled because I added sc_changelog to the list of excluded checks in Makefile.cfg. I disabled it because the check requires ChangeLog date/author lines like this: 2008-04-08 Daniel Veillard <veillard@redhat.com> rather than the ones we use: Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard@redhat.com> If you want the added check, it's trivial to adjust the regexp in that test to permit the existing style, too. Just say the word and I'll do it.
On Thu, Apr 10, 2008 at 10:53:44AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 08:42:08AM +0100, Richard W.M. Jones wrote:
I also say you should go for the full patch.
But please keep tabks in ChangeLog for indenting,
Of course ;-) There's actually a "make syntax-check" that would barf if I tried expanding *those* TABs.
It's in Makefile.maint, and I see that it's disabled because I added sc_changelog to the list of excluded checks in Makefile.cfg. I disabled it because the check requires ChangeLog date/author lines like this:
2008-04-08 Daniel Veillard <veillard@redhat.com>
rather than the ones we use:
Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard@redhat.com>
The former are the style which emacs changelog mode uses, so switching over would be useful (for me). Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
"Richard W.M. Jones" <rjones@redhat.com> wrote:
On Thu, Apr 10, 2008 at 10:53:44AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 08:42:08AM +0100, Richard W.M. Jones wrote:
I also say you should go for the full patch.
But please keep tabks in ChangeLog for indenting,
Of course ;-) There's actually a "make syntax-check" that would barf if I tried expanding *those* TABs.
It's in Makefile.maint, and I see that it's disabled because I added sc_changelog to the list of excluded checks in Makefile.cfg. I disabled it because the check requires ChangeLog date/author lines like this:
2008-04-08 Daniel Veillard <veillard@redhat.com>
rather than the ones we use:
Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard@redhat.com>
The former are the style which emacs changelog mode uses, so switching over would be useful (for me).
I'd prefer it, too, for the same reason. In case there is consensus, here's that Emacs documentation has to say on the subject: Versions of Emacs before 20.1 used a different format for the time of the change log entry: Fri May 25 11:23:23 1993 Richard Stallman <rms@gnu.org> The `M-x change-log-redate' command converts all the old-style date entries in the change log file visited in the current buffer to the new format, to make the file uniform in style. This is handy when entries are contributed by many different people, some of whom use old versions of Emacs.
On Thu, Apr 10, 2008 at 10:27:19AM +0100, Richard W.M. Jones wrote:
On Thu, Apr 10, 2008 at 10:53:44AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 08:42:08AM +0100, Richard W.M. Jones wrote:
I also say you should go for the full patch.
But please keep tabks in ChangeLog for indenting,
Of course ;-) There's actually a "make syntax-check" that would barf if I tried expanding *those* TABs.
It's in Makefile.maint, and I see that it's disabled because I added sc_changelog to the list of excluded checks in Makefile.cfg. I disabled it because the check requires ChangeLog date/author lines like this:
2008-04-08 Daniel Veillard <veillard@redhat.com>
rather than the ones we use:
Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard@redhat.com>
The former are the style which emacs changelog mode uses, so switching over would be useful (for me).
current is :r!date for me in vi, the fingers type it by themselves without even asking the brain, that would be painful to change. Also i like the idea of having a more precise time, really, it's not like we do one commit every few days... Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 10:27:19AM +0100, Richard W.M. Jones wrote:
On Thu, Apr 10, 2008 at 10:53:44AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 08:42:08AM +0100, Richard W.M. Jones wrote:
I also say you should go for the full patch.
But please keep tabks in ChangeLog for indenting,
Of course ;-) There's actually a "make syntax-check" that would barf if I tried expanding *those* TABs.
It's in Makefile.maint, and I see that it's disabled because I added sc_changelog to the list of excluded checks in Makefile.cfg. I disabled it because the check requires ChangeLog date/author lines like this:
2008-04-08 Daniel Veillard <veillard@redhat.com>
rather than the ones we use:
Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard@redhat.com>
The former are the style which emacs changelog mode uses, so switching over would be useful (for me).
current is
:r!date
for me in vi, the fingers type it by themselves without even asking the brain, that would be painful to change. Also i like the idea of having a more precise time, really, it's not like we do one commit every few days...
If you do that, you still have to manually append your name and email address. How about teaching your fingers to type the shorter "#d" (or whatever you want to call it), which would add the entire line? Just add this to your .vimrc file (last byte is ctrl-M, aka C-vC-m): map #d :r!date '+\%F Daniel Veillard <veillard@redhat.com>'
On Thu, Apr 10, 2008 at 02:18:49PM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
for me in vi, the fingers type it by themselves without even asking the brain, that would be painful to change. Also i like the idea of having a more precise time, really, it's not like we do one commit every few days...
If you do that, you still have to manually append your name and email address. How about teaching your fingers to type the shorter "#d" (or whatever you want to call it), which would add the entire line?
Just add this to your .vimrc file (last byte is ctrl-M, aka C-vC-m):
map #d :r!date '+\%F Daniel Veillard <veillard@redhat.com>'
Heh, one more map for my .vimrc (I already have a bunch to of map and map! to mimick Turbo Pascal/Turbo C editor keystrokes, and i still use them all the time :-) Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 02:18:49PM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
for me in vi, the fingers type it by themselves without even asking the brain, that would be painful to change. Also i like the idea of having a more precise time, really, it's not like we do one commit every few days...
If you do that, you still have to manually append your name and email address. How about teaching your fingers to type the shorter "#d" (or whatever you want to call it), which would add the entire line?
Just add this to your .vimrc file (last byte is ctrl-M, aka C-vC-m):
map #d :r!date '+\%F Daniel Veillard <veillard@redhat.com>'
Heh, one more map for my .vimrc (I already have a bunch to of map and map! to mimick Turbo Pascal/Turbo C editor keystrokes, and i still use them all the time :-)
Good! If you're actually going to use that let me know, then I can adjust (make more strict) the patch I proposed here: http://thread.gmane.org/gmane.comp.emulators.libvirt/5929
On Mon, Apr 21, 2008 at 10:22:11AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 02:18:49PM +0200, Jim Meyering wrote:
Daniel Veillard <veillard@redhat.com> wrote:
for me in vi, the fingers type it by themselves without even asking the brain, that would be painful to change. Also i like the idea of having a more precise time, really, it's not like we do one commit every few days...
If you do that, you still have to manually append your name and email address. How about teaching your fingers to type the shorter "#d" (or whatever you want to call it), which would add the entire line?
Just add this to your .vimrc file (last byte is ctrl-M, aka C-vC-m):
map #d :r!date '+\%F Daniel Veillard <veillard@redhat.com>'
Heh, one more map for my .vimrc (I already have a bunch to of map and map! to mimick Turbo Pascal/Turbo C editor keystrokes, and i still use them all the time :-)
Good! If you're actually going to use that let me know, then I can adjust (make more strict) the patch I proposed here:
Well except I still prefer the ChangeLog long form for dates. Daniel -- Red Hat Virtualization group http://redhat.com/virtualization/ Daniel Veillard | virtualization library http://libvirt.org/ veillard@redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/ http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
Daniel Veillard <veillard@redhat.com> wrote:
On Thu, Apr 10, 2008 at 08:42:08AM +0100, Richard W.M. Jones wrote:
I also say you should go for the full patch.
But please keep tabks in ChangeLog for indenting,
Glad we agree. There's one more detail. Some files (e.g. libvirt.c) end with these lines: /* * vim: set tabstop=4: * vim: set shiftwidth=4: * vim: set expandtab: */ /* * Local variables: * indent-tabs-mode: nil * c-indent-level: 4 * c-basic-offset: 4 * tab-width: 4 * End: */ while others (e.g., conf.c) only have the latter block. I'm removing all of these lines regardless: * tab-width: 4 * vim: set tabstop=4: The question is what directives to add to files that currently have none, and how to normalize the existing things. First of all, regarding the vim: directives, they don't serve any purpose, by default. It helps to know that they're interpreted only when Vim's modeline option is enabled. Some distros disable that option because honoring those directives poses a security risk. (Vim's default is to disable modeline for root, and Debian's Vim does that for all users). Even if you turn on the modeline option, Vim searches only the first and last 5 lines by default, when looking for directives, and so the ones above aren't ever honored. So, unless someone objects soon, I'll remove all vim: directives. ------------------------------------------------------- As for the emacs directives, I'm divided. On one hand, it's nice to record project-wide guidelines in a way that's hard to miss. On the other, it's a shame to require this mark-up in every single .c and .h file. While I was planning to add 7 lines to each of the remaining 24 files, consider this alternative: Remove all such directives and instead instruct (via HACKING or some such file) developers to use a small .emacs snippet that defines the desired style for code in a libvirt/ subdir. Then, the style would be defined in just one place, in case we ever change it. For example, add this to your Emacs start-up file: ;;; When editing C in libvirt, indent using spaces, not TABs. (add-hook 'c-mode-hook '(lambda () (if (string-match "/libvirt/" (buffer-file-name)) (setq indent-tabs-mode nil)))) and it has the same effect as inserting the following comment at the end of every file (assuming your working directory name matches): /* * Local variables: * indent-tabs-mode: nil * End: */ I'm sure you can do something similar in Vim.
On Thu, Apr 10, 2008 at 12:06:19PM +0200, Jim Meyering wrote:
As for the emacs directives, I'm divided. On one hand, it's nice to record project-wide guidelines in a way that's hard to miss. On the other, it's a shame to require this mark-up in every single .c and .h file.
While I was planning to add 7 lines to each of the remaining 24 files, consider this alternative:
Remove all such directives and instead instruct (via HACKING or some such file) developers to use a small .emacs snippet that defines the desired style for code in a libvirt/ subdir. Then, the style would be defined in just one place, in case we ever change it.
For example, add this to your Emacs start-up file:
;;; When editing C in libvirt, indent using spaces, not TABs. (add-hook 'c-mode-hook '(lambda () (if (string-match "/libvirt/" (buffer-file-name)) (setq indent-tabs-mode nil))))
and it has the same effect as inserting the following comment at the end of every file (assuming your working directory name matches):
/* * Local variables: * indent-tabs-mode: nil * End: */
I was going to suggest exactly the above alternatives, so whichever you think is best. Rich. -- Richard Jones, Emerging Technologies, Red Hat http://et.redhat.com/~rjones virt-p2v converts physical machines to virtual machines. Boot with a live CD or over the network (PXE) and turn machines into Xen guests. http://et.redhat.com/~rjones/virt-p2v
Jim Meyering <jim@meyering.net> wrote:
"Daniel P. Berrange" <berrange@redhat.com> wrote:
On Wed, Apr 09, 2008 at 02:40:13PM +0100, Richard W.M. Jones wrote:
On Wed, Apr 09, 2008 at 02:43:22PM +0200, Jim Meyering wrote:
Please don't add the "tab-width: 4" specifier. Specifying a tab-width at all in a new file with "indent-tabs-mode: nil" is a contradiction. The latter says there should be no TABs, yet the former says "when there are, give them width 4." Coding style guidelines are universal in their recommendations to stick with 8-byte TAB stops, independent of whether you actually use TAB or spaces.
Agreed, good idea.
ACK.
Could we get a make syntax-check test to look for these bogus tabs too
I've done that. Abbreviated patches below.
However, there are some "issues" to consider. Any global space-changing delta like these is going to cause trouble (conflicts) for people with pending changes and on branches. This one isn't too bad (as these things go), since the TAB-to-space change affects fewer than 1500 lines in 37 files. However, the new rule enforces the coding standard only in files with an existing "indent-tabs-mode: nil" directive. There are currently 42 .[ch] files that don't have such a directive (excluding gnulib/).
$ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|wc -l 42
If I were to do the same for those remaining files, the TAB-to-space change would modify an additional 2817 lines in 28 files:
$ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|xargs grep -E '^ * '|wc -l 2817 $ git ls-files|grep -E '\.[ch]$'|xargs grep -L indent-tabs-mode|grep -v \ gnulib|xargs grep -El '^ * '|wc -l 28
My opinion is that if it's worth doing the first, it's also worth finishing the job, ... but then I don't have any huge re-architecting changes in my queue.
However, I don't want to overstate the case either. The cost of a few space-change-provoked merges is pretty low in the grand scheme of things.
What say you? Convert 'em all?
Sounds like we all agree, so here goes: four change-sets: Here are their ChangeLog entries. I'll post them separately. * HACKING: New file: begin to describe contributor/coding guidelines. Ensure that no C source file uses TABs for indentation. * Makefile.maint (sc_TAB_in_indentation): New rule. Convert TAB-based indentation in .[ch] files to use only spaces. Done using this command (also includes .c.in and .h.in files): for i in $(g ls-files|grep -E '\.[ch](\.in)?$'|grep -v gnulib); do expand -i $i > j && mv j $i;done Remove all vim: and emacs local variable settings in .c and .h files. Done with these commands: git grep -l Local.variab|xargs \ perl -0x3b -pi -e 's,\n+/\*\n \* vim:(.|\n)*,\n,' git grep -l Local.variab|xargs \ perl -0x3b -pi -e 's,\n+/\*\n \* Local variables:\n(.|\n)*,\n,'
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones