git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Ryan Zoeller <rtzoeller@rtzoeller.com>, git@vger.kernel.org
Subject: Re: [RFC 0/1] Leading whitespace as a function identification heuristic?
Date: Thu, 24 Sep 2020 17:17:25 -0400	[thread overview]
Message-ID: <20200924211725.GA3103003@coredump.intra.peff.net> (raw)
In-Reply-To: <xmqqzh5fhduk.fsf@gitster.c.googlers.com>

On Wed, Sep 23, 2020 at 11:45:55PM -0700, Junio C Hamano wrote:

> Ryan Zoeller <rtzoeller@rtzoeller.com> writes:
> 
> > 1. Is this indentation-aware function identification useful, and
> >     generally worth pursuing further?
> 
> I cannot shake the feeling off that this is being overly generous
> and inviting for misidentification for languages whose usual
> convention is not to nest and indent the definitions freely.
> 
> IOW, why can't the "we allow leading whitespaces" a per-language
> thing?  IOW, why do we even need any new code---shouldn't it be just
> the matter of defining xfuncname patterns for such a language with
> nested and indented definitions?

If I understand the problem correctly, I don't think a static regex can
accomplish this, because you need context from the original line. E.g.,
consider something like this:

  void foo() {
	void bar() {
		...code 1...
	}
	...code 2...
  }

If we change one of the lines of code, then we find the function header
by walking backwards up the lines, evaluating a regex for each line. But
for "code 2", how do we know to keep walking past bar() and up to foo()?
Or more specifically, what is different when evaluating a change from
"code 2" that is different than when we would evaluate "code 1"?

If the only input to the question of "is this line a function header" is
the regex from the config, then changes to either line of code must
produce the same answer (either bar() if we allow leading whitespace, or
foo() if we do not).

So I think Ryan's proposal is to give that search an extra piece of
information: the indentation of the original changed line. Which is
enough to differentiate the two cases.

If I understand the patch correctly, it is always picking the first line
where indentation is lessened (and which matches the funcname pattern).
That works out of the box with existing funcname patterns, which is
nice. Though I wonder if there are cases where the funcname regex could
use the information more flexibly (i.e., some marker in the regex that
means "match less than this many spaces" or something).

I do agree that this should not be on automatically for all languages.
Some file formats may want to show a header that's at the same
indentation as the change. Adding a diff.foo.funcnameIndentation config
option would be one solution. But requiring the funcname pattern to make
explicit use of the information is another (and would allow a format to
match some things at one indentation level and some at another; but
again, I'm hand-waving a bit on whether this level of flexibility is
even useful).

-Peff

  reply	other threads:[~2020-09-24 21:17 UTC|newest]

Thread overview: 10+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-09-23 21:59 Ryan Zoeller
2020-09-23 21:59 ` [RFC 1/1] xdiff: use leading whitespace in function heuristic Ryan Zoeller
2020-09-24  6:45 ` [RFC 0/1] Leading whitespace as a function identification heuristic? Junio C Hamano
2020-09-24 21:17   ` Jeff King [this message]
2020-09-24 22:01     ` Ryan Zoeller
2020-09-25  9:11       ` Phillip Wood
2020-09-25 18:43         ` Jeff King
2020-09-25 19:01           ` Phillip Wood
2020-09-25 19:05             ` Jeff King
2020-09-25 18:12 ` Johannes Sixt

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=20200924211725.GA3103003@coredump.intra.peff.net \
    --to=peff@peff.net \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=rtzoeller@rtzoeller.com \
    --subject='Re: [RFC 0/1] Leading whitespace as a function identification heuristic?' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).