[PATCH] diff: add ruby funcname pattern

[PATCH] diff: add ruby funcname pattern

[Giuseppe Bilotta]

Provide a regexp that catches class, module and method definitions in
Ruby scripts, since the built-in default only finds classes.

Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
---
 diff.c |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index cbf2547..c253015 100644
--- a/diff.c
+++ b/diff.c
@@ -1381,6 +1381,7 @@ static struct builtin_funcname_pattern {
 			"[A-Za-z_][A-Za-z_0-9]*\\)\\{2,\\}"
 			"[ 	]*([^;]*\\)$" },
 	{ "tex", "^\\(\\\\\\(sub\\)*section{.*\\)$" },
+	{ "ruby", "^\\s*\\(\\(class\\|module\\|def\\)\\s.*\\)$" },
 };
 
 static const char *diff_funcname_pattern(struct diff_filespec *one)
-- 
1.5.6.3

Re: [PATCH] diff: add ruby funcname pattern

[Junio C Hamano]

Giuseppe Bilotta <giuseppe.bilotta@gmail.com> writes:

> Provide a regexp that catches class, module and method definitions in
> Ruby scripts, since the built-in default only finds classes.
>
> Signed-off-by: Giuseppe Bilotta <giuseppe.bilotta@gmail.com>
> ---
>  diff.c |    1 +
>  1 files changed, 1 insertions(+), 0 deletions(-)
>
> diff --git a/diff.c b/diff.c
> index cbf2547..c253015 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1381,6 +1381,7 @@ static struct builtin_funcname_pattern {
>  			"[A-Za-z_][A-Za-z_0-9]*\\)\\{2,\\}"
>  			"[ 	]*([^;]*\\)$" },
>  	{ "tex", "^\\(\\\\\\(sub\\)*section{.*\\)$" },
> +	{ "ruby", "^\\s*\\(\\(class\\|module\\|def\\)\\s.*\\)$" },
>  };
>  
>  static const char *diff_funcname_pattern(struct diff_filespec *one)

Thanks.

I do not talk Ruby, so I'll wait for a few days to hear any one of the
following happen before deciding what to do with this patch:

 (1) Yeah, this is a sufficient and necessary set of keywords, and it
     would make my Ruby life so much better;

 (2) This might be a good start but you need to cover this and that
     keywords as well;

 (3) This will misidentify a line that is not the beginning of a
     definition, and should not be applied;

Needless to say, "Here is a better patch" is appreciated if somebody says
(2) or (3).

Re: [PATCH] diff: add ruby funcname pattern

[Giuseppe Bilotta]

On Fri, Aug 1, 2008 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Thanks.
>
> I do not talk Ruby,

You're really missing into something ;)

> so I'll wait for a few days to hear any one of the
> following happen before deciding what to do with this patch:
>
>  (1) Yeah, this is a sufficient and necessary set of keywords, and it
>     would make my Ruby life so much better;
>
>  (2) This might be a good start but you need to cover this and that
>     keywords as well;
>
>  (3) This will misidentify a line that is not the beginning of a
>     definition, and should not be applied;
>
> Needless to say, "Here is a better patch" is appreciated if somebody says
> (2) or (3).

I wasn't sure about the completeness of the regexp myself, which is
why I asked in #ruby on freenode for additional suggestions. The only
reply I got was an idea to add a number of block-based coding sections
such as Procs an Threads by matching .*\.new ({|do) as well.

This is something I had been thinking about myself, but I decided to
discard the idea because the current chunk text detection in git does
not have a way to say 'this is the end of a function, so get back to
the previous chunk (con)text' instead of using the last chunk text: so
if I have methodx followed by methody and make a change near the top
of methody I get methodx in the chunk text.

This limitation, that affects all funcname parsers, has kept me from
adding Proc, Thread and other anonymous code blocks to the Ruby
funcname, because having the proper context for changed code after an
anonymous block is IMO better than having specific context in the
anonymous block itself.

OTOH, if the funcname capability was expand to a full stack
manipulation (push chunk text, pop chunk text) ... (and yes, if I ever
find a sane way to do it myself I *will* provide a patch)




-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] diff: add ruby funcname pattern

[Junio C Hamano]

"Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:

> On Fri, Aug 1, 2008 at 9:30 AM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> so I'll wait for a few days to hear any one of the
>> following happen before deciding what to do with this patch:
>>
>>  (1) Yeah, this is a sufficient and necessary set of keywords, and it
>>     would make my Ruby life so much better;
>>
>>  (2) This might be a good start but you need to cover this and that
>>     keywords as well;
>>
>>  (3) This will misidentify a line that is not the beginning of a
>>     definition, and should not be applied;
>>
>> Needless to say, "Here is a better patch" is appreciated if somebody says
>> (2) or (3).
>
> I wasn't sure about the completeness of the regexp myself, which is

Well, I forgot to say but the above was soliciting third party review;
original submitter does not count ;-)

... nah, I am just joking.

All of the things you said in the message I am responding to are good
background information.  It would have been nicer if it were part of the
initial message, perhaps below the three dash lines, which would have
avoided this extra exchange.

Thanks again for the patch.  Somewhere I heard that there are 10 Rubyista
git users for every non Rubyista git user, so I am sure somebody would
comment on your patch in a day or two.  Perhaps we might even get Python
and Perl hunk patterns (although I suspect Perl people are happy with the
default one we stole from GNU diff) to go with it ;-).

Re: [PATCH] diff: add ruby funcname pattern

[Giuseppe Bilotta]

On Fri, Aug 1, 2008 at 10:20 AM, Junio C Hamano <gitster@pobox.com> wrote:

> Well, I forgot to say but the above was soliciting third party review;
> original submitter does not count ;-)
>
> ... nah, I am just joking.
>
> All of the things you said in the message I am responding to are good
> background information.  It would have been nicer if it were part of the
> initial message, perhaps below the three dash lines, which would have
> avoided this extra exchange.

Yeah, I know, I'm sorry, I realized I should have written it there as
I was replying to your email. I'll try to keep it in mind the next
time I submit a patch :)

> Thanks again for the patch.  Somewhere I heard that there are 10 Rubyista
> git users for every non Rubyista git user, so I am sure somebody would
> comment on your patch in a day or two.  Perhaps we might even get Python
> and Perl hunk patterns (although I suspect Perl people are happy with the
> default one we stole from GNU diff) to go with it ;-).

You know, I think the Ruby funcname would work pretty well with Python
as well. Or something very similar. I'll try cooking up a patch for
that too. (But I thought Python users used mercurial? 8-D)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] diff: add ruby funcname pattern

[Jeff King]

On Fri, Aug 01, 2008 at 01:20:10AM -0700, Junio C Hamano wrote:

> Thanks again for the patch.  Somewhere I heard that there are 10 Rubyista
> git users for every non Rubyista git user, so I am sure somebody would
> comment on your patch in a day or two.  Perhaps we might even get Python
> and Perl hunk patterns (although I suspect Perl people are happy with the
> default one we stole from GNU diff) to go with it ;-).

I keep a lot of Perl in git, and yes, I am quite happy with the default
regex.

-Peff

Re: [PATCH] diff: add ruby funcname pattern

[Kevin Ballard]

On Aug 1, 2008, at 1:20 AM, Junio C Hamano wrote:

> "Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
>
>> On Fri, Aug 1, 2008 at 9:30 AM, Junio C Hamano <gitster@pobox.com>  
>> wrote:
>> ...
>>> so I'll wait for a few days to hear any one of the
>>> following happen before deciding what to do with this patch:
>>>
>>> (1) Yeah, this is a sufficient and necessary set of keywords, and it
>>>    would make my Ruby life so much better;
>>>
>>> (2) This might be a good start but you need to cover this and that
>>>    keywords as well;
>>>
>>> (3) This will misidentify a line that is not the beginning of a
>>>    definition, and should not be applied;
>>>
>>> Needless to say, "Here is a better patch" is appreciated if  
>>> somebody says
>>> (2) or (3).
>>
>> I wasn't sure about the completeness of the regexp myself, which is
>
> Well, I forgot to say but the above was soliciting third party review;
> original submitter does not count ;-)
>
> ... nah, I am just joking.
>
> All of the things you said in the message I am responding to are good
> background information.  It would have been nicer if it were part of  
> the
> initial message, perhaps below the three dash lines, which would have
> avoided this extra exchange.

As a Ruby user, the regex for the funcname looks fine to me.

> Thanks again for the patch.  Somewhere I heard that there are 10  
> Rubyista
> git users for every non Rubyista git user, so I am sure somebody would
> comment on your patch in a day or two.  Perhaps we might even get  
> Python
> and Perl hunk patterns (although I suspect Perl people are happy  
> with the
> default one we stole from GNU diff) to go with it ;-).

I'd like to point out that Python users are called Pythonistas, Ruby  
users are called... uh, Ruby users, I guess.

-Kevin Ballard

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Re: [PATCH] diff: add ruby funcname pattern

[Junio C Hamano]

Kevin Ballard <kevin@sb.org> writes:

>> "Giuseppe Bilotta" <giuseppe.bilotta@gmail.com> writes:
>>> ...
>>> I wasn't sure about the completeness of the regexp myself, which is
>> ...
> As a Ruby user, the regex for the funcname looks fine to me.

Thanks, will apply.

Re: [PATCH] diff: add ruby funcname pattern

[Giuseppe Bilotta]

On Sat, Aug 2, 2008 at 2:31 AM, Kevin Ballard <kevin@sb.org> wrote:
>
> As a Ruby user, the regex for the funcname looks fine to me.

What is your opinion about the anonymous code blocks?

I've been thinking that another possibility could be to have two ruby
funcnames, a simpler one (the one I presented) that only has 'named'
blocks, and a more thorough one that also matches up anonymous blocks.
User could then choose which one to use in their code by having
gitattributes such as *.rb diff=ruby or *.rb diff=ruby-full (or
whatever else).

I'm not sure this would be a sensible policy though.

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] diff: add ruby funcname pattern

[Kevin Ballard]

On Aug 1, 2008, at 10:41 PM, Giuseppe Bilotta wrote:

> On Sat, Aug 2, 2008 at 2:31 AM, Kevin Ballard <kevin@sb.org> wrote:
>>
>> As a Ruby user, the regex for the funcname looks fine to me.
>
> What is your opinion about the anonymous code blocks?
>
> I've been thinking that another possibility could be to have two ruby
> funcnames, a simpler one (the one I presented) that only has 'named'
> blocks, and a more thorough one that also matches up anonymous blocks.
> User could then choose which one to use in their code by having
> gitattributes such as *.rb diff=ruby or *.rb diff=ruby-full (or
> whatever else).
>
> I'm not sure this would be a sensible policy though.

If you're going to get into anonymous code blocks, you're going to  
have a really tough time deciding which blocks are interesting and  
which aren't. And as you stated before, without a stack-based  
approach, this could really fall apart, as anonymous blocks are  
(almost) always going to be inside a method.

I think it's far simpler to stick with class/module/def.

-Kevin Ballard

-- 
Kevin Ballard
http://kevin.sb.org
kevin@sb.org
http://www.tildesoft.com

Re: [PATCH] diff: add ruby funcname pattern

[Giuseppe Bilotta]

On Sat, Aug 2, 2008 at 7:47 AM, Kevin Ballard <kevin@sb.org> wrote:
>
> If you're going to get into anonymous code blocks, you're going to have a
> really tough time deciding which blocks are interesting and which aren't.
> And as you stated before, without a stack-based approach, this could really
> fall apart, as anonymous blocks are (almost) always going to be inside a
> method.

I was just looking for a libxdiff issue tracker but I couldn't find
one, so I guess I'll ask the author directly about the possibility to
implement such a thing. The matchit plugin for vim seems to manage
(even user-defined) code blocks very well, even for multi-state blocks
(if ... else ... end), using regexps; so maybe a reimplementation in C
for libxdiff could be a solution. Of course, one wonders how much
slower such an approach would be as opposed to the current "look back
until the first matching line" solution ...

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] diff: add ruby funcname pattern

[Johannes Schindelin]

Hi,

On Sat, 2 Aug 2008, Giuseppe Bilotta wrote:

> On Sat, Aug 2, 2008 at 7:47 AM, Kevin Ballard <kevin@sb.org> wrote:
> >
> > If you're going to get into anonymous code blocks, you're going to have a
> > really tough time deciding which blocks are interesting and which aren't.
> > And as you stated before, without a stack-based approach, this could really
> > fall apart, as anonymous blocks are (almost) always going to be inside a
> > method.
> 
> I was just looking for a libxdiff issue tracker but I couldn't find one, 
> so I guess I'll ask the author directly about the possibility to 
> implement such a thing.

The funcname thing was introduced by us, in Git.  I do not know if Davide 
picked the changes up; at least for the merge stuff he seemed to be pretty 
reluctant.

> The matchit plugin for vim seems to manage (even user-defined) code 
> blocks very well, even for multi-state blocks (if ... else ... end), 
> using regexps; so maybe a reimplementation in C for libxdiff could be a 
> solution.

Bzzt.  You say vi manages it with regexps, and then you go on and say that 
you therefore do _not_ want to use a regexp?

BTW having funcname calculation in C was shot down by Junio as being too 
inflexible, as the user cannot add new languages without recompiling.  
That's why we have regexps now.

Ciao,
Dscho

Re: [PATCH] diff: add ruby funcname pattern

[Giuseppe Bilotta]

Hello,

On Sat, Aug 2, 2008 at 1:39 PM, Johannes Schindelin
<Johannes.Schindelin@gmx.de> wrote:
> The funcname thing was introduced by us, in Git.  I do not know if Davide
> picked the changes up; at least for the merge stuff he seemed to be pretty
> reluctant.

Ah, it's a Git-only thing? didn't know about that. I guess that
knowing that git's libxdiff is already hacked up from the original
means I can think more freely about hacking it up some more, if I can
come up with a proper solution 8-)

>> The matchit plugin for vim seems to manage (even user-defined) code
>> blocks very well, even for multi-state blocks (if ... else ... end),
>> using regexps; so maybe a reimplementation in C for libxdiff could be a
>> solution.
>
> Bzzt.  You say vi manages it with regexps, and then you go on and say that
> you therefore do _not_ want to use a regexp?
>
> BTW having funcname calculation in C was shot down by Junio as being too
> inflexible, as the user cannot add new languages without recompiling.
> That's why we have regexps now.

Sorry, I think there's a misunderstanding here ... I have all
intentions to use regexps. What I was planning on reimplementing in C
was the way matchit uses the regexps to determine the blocks (because,
obviously, it being a vim script means it's coded in vim's own
langauge instead of C). The actual definitions for the start and end
of the block would still be done via regexps. 8-)

-- 
Giuseppe "Oblomov" Bilotta

Re: [PATCH] diff: add ruby funcname pattern

[Lee Marlow]

On Fri, Aug 1, 2008 at 11:47 PM, Kevin Ballard <kevin@sb.org> wrote:
> On Aug 1, 2008, at 10:41 PM, Giuseppe Bilotta wrote:
>
>> On Sat, Aug 2, 2008 at 2:31 AM, Kevin Ballard <kevin@sb.org> wrote:
>
> I think it's far simpler to stick with class/module/def.
>

I agree with Kevin.

It's simpler and I'm not sure what value it would bring to show
anonymous code blocks.  The funcname is just to provide some
additional context around the diff hunk, so you know where you are.
What would be shown for an anonymous code block or proc in the diff?
The module/class/method seems to provide the most useful context.

-Lee