Re: [PATCH] Add -p to show C function names was Re: [PATCH] Extra options for libsvn_diff

[Julian Foad <ju...@btopenworld.com>]

Justin Erenkrantz wrote:
> On Dec 27, 2007 2:44 PM, C. Michael Pilato <cm...@collab.net> wrote:
>>Justin Erenkrantz wrote:
>>
>>>I'd like to resurrect this discussion - IOW, I want Subversion to
>>>support -p out-of-the-box to print C-style function names.
>>[...]
>>
>>>Patch against trunk below.  I plan to commit this unless someone
>>>intends to veto it.
>>
>>No veto here.  We now have precedent in the diff option space that we didn't
>>have before.  This won't look like a weird wart now.
> 
> Thanks, C-Mike.  I realize that Julian might still veto this when he
> ostensibly comes back from vacation, but enough other committers
> stated that they are in favor of the concept now that I went ahead and
> committed it in r28700.
> 
> Enjoy!  -- justin

Heh! I'm back. And fear not, for I can't really veto it in good conscience now 
that so many have agreed to it. Anyway, I'll want to use it.

In fact, I use "diff-cmd = /usr/bin/diff" and "svn diff -x -p" all the time: 
it's really useful for me because I'm a C programmer. Having it built in is 
fractionally more convenient, especially for those who don't have a suitable 
external diff program already installed.

But, AARGH! There goes another blow to the spirit of free software being a 
force for bringing together the best tools for each job.

Understanding C syntax is not Subversion's job. (I'm sure you realised your 
comment about how this being a specialist feature is in some way comparable to 
merge tracking being a specialist feature was completely off-base. Merging is 
in the domain of version control; understanding programming languages is not.)

The GNU diff implementation of "-p" is rather poor. It's enough to give a 
useful clue most of the time, and it's conveniently available to most people, 
and there isn't a better tool so readily to hand, so we like to use it. But I 
use it for automatically constructing Subversion log message templates (file 
names and function names formatted as required) and I find that a large 
proportion of its output is wrong. One big cause is that it stops looking for a 
function name when it starts emitting pre-context lines, which is usually too 
soon to pick up any change to a function prototype in a header file. There are 
many other problems with it which I could start to list if you want.

One thing that's galling is that we have now diminished people's incentive to 
improve that tool that is best suited to that job, by adding another mediocre 
implementation to the world. (Sorry if that sounds rude, but without having 
tried it I can guarantee it's mediocre.) I want to improve GNU diff's 
show-function support. Do I want to improve Subversion's implementation as well 
or instead? No, not really; I want it to be improved, but I don't want me or 
anyone else to have to consider two separate implementations.

I don't like having mediocre features in Subversion. It gives a poor impression 
to users. The argument that it's no worse than the common alternative is not a 
good one. That argument only works for you and those of us who were previously 
using the alternative external tool. New users will see it differently.

Another thing is that this focuses on one particular domain of usage in *the 
core* of Subversion client libraries, and if this trend continues it *will* 
over a long time lead to other groups of users gradually finding Subversion 
less and less suitable for their needs.

The correct solution to the genuine desire (or even need) for this feature is 
to recognise that it is a feature specific to certain file types and certain 
users, and implement it as part of the pluggable diff mechanism, in a way that 
is unbiased and equally able to support InterWeb-9.9 file formats and 
everything else that other groups of (especially non-programmer) users will want.

By "unbiased" I include the desire for similar features for other file formats 
to be supportable in a way that is at least as easy as invoking "svn diff -x -p".

Let us enjoy it now we've got it, but let's also bear in mind that we should be 
untangling it from the core code, making it an auxiliary feature via a 
domain-neutral extension mechanism. This could be the incentive we need to get 
the "pluggable diff" support moving forward. In the short term, don't you think 
it might be a good idea to move the C code for this out of the central "diff" 
functions in such a way that other such features can be added alongside it 
without cluttering the basic diff functionality?

Well, you knew I'd have to comment!

- Julian

p.s. I know some might see this as a bit of an extreme angle on it. Yes, it is 
useful for Subversion to support some commonly wanted domain-specific options. 
Yes, any harm it may do long term isn't measurable yet. Yes, I could argue the 
same way about the "diff" feature as a whole.


---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Add -p to show C function names was Re: [PATCH] Extra options for libsvn_diff

[Vincent Lefevre <vi...@vinc17.org>]

On 2008-01-26 18:11:06 -0800, Justin Erenkrantz wrote:
> Also, I think we do have pluggable diff - via the diff-cmd - or are

Isn't it a bit limited? For instance, how can it know the file type
(svn:mime-type property) to apply the diff corresponding to this file
type? This is useful as the -p option should only be used for some
file types (e.g. C files)?

-- 
Vincent Lefèvre <vi...@vinc17.org> - Web: <http://www.vinc17.org/>
100% accessible validated (X)HTML - Blog: <http://www.vinc17.org/blog/>
Work: CR INRIA - computer arithmetic / Arenaire project (LIP, ENS-Lyon)

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org

Re: [PATCH] Add -p to show C function names was Re: [PATCH] Extra options for libsvn_diff

[Justin Erenkrantz <ju...@erenkrantz.com>]

On Jan 26, 2008 5:28 PM, Julian Foad <ju...@btopenworld.com> wrote:
> Understanding C syntax is not Subversion's job. (I'm sure you realised your
> comment about how this being a specialist feature is in some way comparable to
> merge tracking being a specialist feature was completely off-base. Merging is
> in the domain of version control; understanding programming languages is not.)

I guess our difference stems from the fact that I view producing
useful diffs as an essential part of any version control tool.  Why
did we go to all of the hassle of writing libsvn_diff if differencing
isn't an intrinsic part of a good SCM tool?  We could have just always
invoked GNU diff and called it a day.  And, we actually did that for a
long time until we realized just how bad invoking GNU diff is.  =)

> Let us enjoy it now we've got it, but let's also bear in mind that we should be
> untangling it from the core code, making it an auxiliary feature via a
> domain-neutral extension mechanism. This could be the incentive we need to get
> the "pluggable diff" support moving forward. In the short term, don't you think
> it might be a good idea to move the C code for this out of the central "diff"
> functions in such a way that other such features can be added alongside it
> without cluttering the basic diff functionality?

Well, I think it's exactly where it belongs - inside of libsvn_diff.
If the logic for finding the functions and emitting the extra context
were in libsvn_wc or something, I'd think you'd have a case - but it
doesn't seem odd to me to have it inside libsvn_diff at all.

Also, I think we do have pluggable diff - via the diff-cmd - or are
you thinking of alternate libsvn_diff implementations?  I'm not sure
how I feel about that...but there's nothing that says you couldn't
just replace libsvn_diff with libsvn_cool_diff as long as it exposed
the same interfaces...

BTW, thanks for not vetoing.  =P  -- justin

---------------------------------------------------------------------
To unsubscribe, e-mail: dev-unsubscribe@subversion.tigris.org
For additional commands, e-mail: dev-help@subversion.tigris.org