Here follows a rant about "--show-c-function". Please don't flame me for it.
It's probably not my final stance on the matter. Constructive and reasoned
refutations are most welcome.
Peter N. Lundblad wrote:
>
> I've been spending some time in libsvn_diff/diff_file.c implementing
> options to ignore whitespace, eol-style
That's excellent, probably. Despite the separate email I've just sent about
being uncomfortable about adding those options because they are "crude" tools,
I expect that will want to have them.
> and the -p option of GNU diff (--show-c-function).
I have a problem with this. Actually, it's very similar to my concerns about
the ignore-spaces options, but a bit different.
I know how useful the "--show-c-function" feature is, and I use it myself when
preparing patches for you most of the time, but I'm sorry, I really can't
accept it being built in to Subversion. I hate it being built in to GNU "diff"
(where it is) and I have been among the developers refusing requests to add it
to GNU "grep".
There are two ways in which it is wrong:
1) It's a feature that's specific to a very particular (though quite common)
sub-set of uses (and users) of Subversion, and indeed to a particular (though
common) style of writing C functions. It's not exactly specific to C - the
same implementation will work to varing degrees for some other languages - but
nor is it generic.
2) It's only a crude approximation to what you really want. It works for a
reasonable proportion of typical use cases, but fails in most of the less
common cases. For instance, it fails if the source code isn't laid out as
expected, e.g. I sometimes put temporary "printf" or similar debugging aids
aligned with the left margin so they're easy to spot and remove later. It
names the previous function if the change begins in the doc string on the lines
before a function. The GNU diff implementation is particularly bad, naming the
previous function if even the context lines begin before the changed function,
but I trust yours won't do that. The GNU diff implementation also truncates
moderately long function names; I trust yours won't do that.
I really dislike the idea of adding such a crude and yet narrowly targetted
feature to core Subversion.
As an aside, I think the same about having a built-in default for
"global-ignores" that matches some particular file extensions that we
developers like to ignore ("*.o *.lo *.la #*# .*.rej *.rej .*~ *~ .#*
.DS_Store"). In version 2 we should remove the built-in default and let it be
configured entirely by the external configuration facility (~/.subversion/config).
Concern (1) could be alleviated by making it a generic "find this pattern"
feature rather than a hard-coded "find a certain style of C function". There
is a practical-level objection that "we'd probably have to incorporate a regex
engine" but that's not an objection in principle. Powerful text-processing
tools need regex support and that is why there are regex libraries. (We have
other reasons for wanting it anyway.)
Concern (2) could only be alleviated by incorporating a full C preprocessor and
parser, which is plainly silly, or by using an external tool that does so,
which is plainly the correct solution.
Let's step back a little and ask why a "diff" is built in to Subversion. I
think the history is roughly:
* We wanted to supply Subversion with a "diff" feature that worked for everyone
"out of the box", and we couldn't find suitable source code with an acceptable
licence, so we wrote our own.
* It was easier and more efficient to build it in than to build and call a
separate program.
Now, while the built-in diff is a light-weight library that does just what
Subversion needs, that's fine, but if we're going to let it grow specialised
features dedicated to particular programming languages or whatever (even though
the implementation of "-p" may currently be lightweight) then that's not fine.
The fact that the feature is commonly requested is not sufficient justification.
If we want to build and supply a separate program that does this, and that
integrates nicely with Subversion, that's fine, let's do so.
I don't know. Again, it's a matter of how we look at the situation. I think
that if we add this then we should be willing to add "--show-pascal-function",
"--show-xml-element", "--show-python-function", "--show-docbook-lite-section",
and so on whenever people want them. I'm serious, and maybe that's OK.
- Julian
---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org
Received on Thu Feb 16 20:05:06 2006