7:43 a.m.
Dave Leskovec <dlesko(a)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
8:40 a.m.
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
9:37 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
9:42 a.m.
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 :|
10:56 a.m.
"Daniel P. Berrange" <berrange(a)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.
1:42 p.m.
"Daniel P. Berrange" <berrange(a)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(a)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(a)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);
...
1:46 p.m.
On Wed, Apr 09, 2008 at 08:42:01PM +0200, Jim Meyering wrote:
"Daniel P. Berrange" <berrange(a)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 :|
2:18 a.m.
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.
2:29 a.m.
Hi Mark,
Mark McLoughlin <markmc(a)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 ...
3:14 a.m.
On Thu, 2008-04-10 at 09:29 +0200, Jim Meyering wrote:
Mark McLoughlin <markmc(a)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.
3:49 a.m.
Mark McLoughlin <markmc(a)redhat.com> wrote:
On Thu, 2008-04-10 at 09:29 +0200, Jim Meyering wrote:
> Mark McLoughlin <markmc(a)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.
2:42 a.m.
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
3:29 a.m.
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
3:53 a.m.
Daniel Veillard <veillard(a)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(a)redhat.com>
rather than the ones we use:
Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard(a)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.
4:27 a.m.
On Thu, Apr 10, 2008 at 10:53:44AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard(a)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(a)redhat.com>
rather than the ones we use:
Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard(a)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
4:39 a.m.
"Richard W.M. Jones" <rjones(a)redhat.com> wrote:
On Thu, Apr 10, 2008 at 10:53:44AM +0200, Jim Meyering wrote:
> Daniel Veillard <veillard(a)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(a)redhat.com>
>
> rather than the ones we use:
>
> Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard(a)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(a)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.
6:41 a.m.
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(a)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(a)redhat.com>
>
> rather than the ones we use:
>
> Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
7:18 a.m.
Daniel Veillard <veillard(a)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(a)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(a)redhat.com>
> >
> > rather than the ones we use:
> >
> > Tue Apr 8 18:14:50 CET 2008 Daniel Veillard <veillard(a)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(a)redhat.com>'
12:38 p.m.
On Thu, Apr 10, 2008 at 02:18:49PM +0200, Jim Meyering wrote:
Daniel Veillard <veillard(a)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(a)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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
3:22 a.m.
Daniel Veillard <veillard(a)redhat.com> wrote:
On Thu, Apr 10, 2008 at 02:18:49PM +0200, Jim Meyering wrote:
> Daniel Veillard <veillard(a)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(a)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
4:22 a.m.
On Mon, Apr 21, 2008 at 10:22:11AM +0200, Jim Meyering wrote:
Daniel Veillard <veillard(a)redhat.com> wrote:
> On Thu, Apr 10, 2008 at 02:18:49PM +0200, Jim Meyering wrote:
>> Daniel Veillard <veillard(a)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(a)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
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(a)redhat.com | libxml GNOME XML XSLT toolkit http://xmlsoft.org/
http://veillard.com/ | Rpmfind RPM search engine http://rpmfind.net/
5:06 a.m.
New subject: [Libvir] revised proposal: remove all indentation directives
Daniel Veillard <veillard(a)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.
5:24 a.m.
New subject: [Libvir] Re: revised proposal: remove all indentation directives
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
10:38 a.m.
Jim Meyering <jim(a)meyering.net> wrote:
"Daniel P. Berrange" <berrange(a)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,'
6059
days inactive
6071
days old
23 comments
5 participants
participants (5)
-
Daniel P. Berrange -
Daniel Veillard -
Jim Meyering -
Mark McLoughlin -
Richard W.M. Jones