From: Junio C Hamano <gitster@pobox.com>
To: Johannes Sixt <j6t@kdbg.org>
Cc: Victor Engmark <victor@engmark.name>, git@vger.kernel.org
Subject: Re: [PATCH v2] userdiff: support Bash
Date: Wed, 21 Oct 2020 11:39:04 -0700 [thread overview]
Message-ID: <xmqq8sbz8lvb.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <a07042af-d16c-1975-c0d1-f22f4fec5827@kdbg.org> (Johannes Sixt's message of "Wed, 21 Oct 2020 09:07:12 +0200")
Johannes Sixt <j6t@kdbg.org> writes:
>> diff --git a/t/t4018/bash-missing-parentheses b/t/t4018/bash-missing-parentheses
>> new file mode 100644
>> index 0000000000..d112761300
>> --- /dev/null
>> +++ b/t/t4018/bash-missing-parentheses
>> @@ -0,0 +1,4 @@
>> +functionRIGHT { # non-match
>> + :
>> + echo 'ChangeMe'
>> +}
>
> To check for a non-match, you write the test like this:
>
> function RIGHT () {
> }
> # the following must not be picked up:
> functionWrong {
> :
> ChangeMe
> }
>
> That is, you present a positive match, and then expect that the
> subsequent negative match is not picked up.
All good suggestions, but I especially appreciate this one ;-).
>> diff --git a/userdiff.c b/userdiff.c
>> index fde02f225b..8830019f05 100644
>> --- a/userdiff.c
>> +++ b/userdiff.c
>> @@ -23,6 +23,28 @@ IPATTERN("ada",
>> "[a-zA-Z][a-zA-Z0-9_]*"
>> "|[-+]?[0-9][0-9#_.aAbBcCdDeEfF]*([eE][+-]?[0-9_]+)?"
>> "|=>|\\.\\.|\\*\\*|:=|/=|>=|<=|<<|>>|<>"),
>> +PATTERNS("bash",
>> + /* Optional leading indentation */
>> + "^[ \t]*"
>> + /* Start of function definition */
>> + "("
>
> The purpose of this outer-most pair of parentheses is actually to mark
> the captured text, not so much "the function definition".
This, too (I called it "here comes the whole thing" in my suggested
version ).
>> + /* Start of POSIX/Bashism grouping */
>> + "("
>
> You could omit the comment if you indent the parts that are inside the
> parentheses:
>
> "("
> "..."
> "|"
> "..."
> ")"
>
An excellent readability suggestion.
Thanks for a review. Especially the parts that mine didn't touch
(i.e. the proposed log message).
> (But perhaps don't indent between the outer-most parentheses; it would
> get us too far to the right. But judge yourself.)
>
>> + /* POSIX identifier with mandatory parentheses */
>> + "[a-zA-Z_][a-zA-Z0-9_]*[ \t]*\\([ \t]*\\))"
>> + /* Bashism identifier with optional parentheses */
>> + "|(function[ \t]+[a-zA-Z_][a-zA-Z0-9_]*(([ \t]*\\([ \t]*\\))|([ \t]+))"
>> + /* End of POSIX/Bashism grouping */
>> + ")"
>> + /* Optional whitespace */
>> + "[ \t]*"
>> + /* Compound command starting with `{`, `(`, `((` or `[[` */
>> + "(\\{|\\(\\(?|\\[\\[)"
>> + /* End of function definition */
>> + ")",
>> + /* -- */
>> + /* Characters not in the default $IFS value */
>> + "[^ \t]+"),
next prev parent reply other threads:[~2020-10-21 18:39 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-20 7:10 [PATCH] userdiff: support Bash Victor Engmark
2020-10-20 23:39 ` Junio C Hamano
2020-10-21 3:00 ` [PATCH v2] " Victor Engmark
2020-10-21 7:07 ` Johannes Sixt
2020-10-21 18:39 ` Junio C Hamano [this message]
2020-10-21 22:48 ` Victor Engmark
2020-10-21 23:45 ` [PATCH v3] " Victor Engmark
2020-10-22 6:08 ` Johannes Sixt
2020-10-22 17:30 ` Junio C Hamano
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
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqq8sbz8lvb.fsf@gitster.c.googlers.com \
--to=gitster@pobox.com \
--cc=git@vger.kernel.org \
--cc=j6t@kdbg.org \
--cc=victor@engmark.name \
/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
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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 read-only IMAP folder(s) and NNTP newsgroup(s).