git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* [GSOC][PATCH] userdiff: add support for Scheme
@ 2021-03-27 17:39 Atharva Raykar
  2021-03-27 22:50 ` Junio C Hamano
                   ` (2 more replies)
  0 siblings, 3 replies; 35+ messages in thread
From: Atharva Raykar @ 2021-03-27 17:39 UTC (permalink / raw)
  To: git; +Cc: Atharva Raykar

Add a diff driver for Scheme (R5RS and R6RS) which
recognizes top level and local `define` forms,
whether it is a function definition, binding, syntax
definition or a user-defined `define-xyzzy` form.

The rationale for picking `define` forms for the
hunk headers is because it is usually the only
significant form for defining the structure of the
program, and it is a common pattern for schemers to
have local function definitions to hide their
visibility, so it is not only the top level
`define`'s that are of interest. Schemers also
extend the language with macros to provide their
own define forms (for example, something like a
`define-test-suite`) which is also captured in the
hunk header.

The word regex is a best-effort attempt to conform
to R6RS[1] valid identifiers, symbols and numbers.

[1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
---

Hi, first-time contributor here, I wanted to have a go at this as
a microproject.

A few things I had to consider:
 - Going through the mailing list, there have already been two other
   patches that are for lispy languages that have taken slightly
   different approaches: Elisp[1] and Clojure[2]. Would it make any
   sense to have a single userdiff driver for lisp that just captures
   all top level forms in the hunk? I personally felt it's better to
   differentiate the drivers for each language, as they have different
   constructs.

 - It was hard to decide exactly which forms should appear on the hunk
   headers. Having programmed in a Scheme before, I went with the forms I
   would have liked to see when looking at git diffs, which would be the
   nearest `define` along with `define-syntax` and other define forms that
   are created as user-defined macros. I am willing to ask around in
   certain active scheme communities for some kind of consensus, but there
   is no single large consolidated group of schemers (the closest is
   probably comp.lang.scheme?).

 - By best-effort attempt at the wordregex, I mean that it is a little
   more permissive than it has to be, as it accepts a few words that are
   technically invalid in Scheme.
   Making it handle all cases like numbers and identifiers with separate
   regexen would be greatly complicated (Eg: #x#e10.2f3 is a valid number
   but #x#f10.2e3 is not; 10t1 is a valid identifier, but 10s1 is a number
   -- my wordregex just clubs all of these into a generic 'word match' which
   trades of granularity for simplicity, and it usually does the right thing).

[1] http://public-inbox.org/git/20210213192447.6114-1-git@adamspiers.org/
[2] http://public-inbox.org/git/pull.902.git.1615667191368.gitgitgadget@gmail.com/

 Documentation/gitattributes.txt    | 2 ++
 t/t4018-diff-funcname.sh           | 1 +
 t/t4018/scheme-define-syntax       | 8 ++++++++
 t/t4018/scheme-local-define        | 4 ++++
 t/t4018/scheme-top-level-define    | 4 ++++
 t/t4018/scheme-user-defined-define | 6 ++++++
 t/t4034-diff-words.sh              | 1 +
 t/t4034/scheme/expect              | 9 +++++++++
 t/t4034/scheme/post                | 4 ++++
 t/t4034/scheme/pre                 | 4 ++++
 userdiff.c                         | 8 ++++++++
 11 files changed, 51 insertions(+)
 create mode 100644 t/t4018/scheme-define-syntax
 create mode 100644 t/t4018/scheme-local-define
 create mode 100644 t/t4018/scheme-top-level-define
 create mode 100644 t/t4018/scheme-user-defined-define
 create mode 100644 t/t4034/scheme/expect
 create mode 100644 t/t4034/scheme/post
 create mode 100644 t/t4034/scheme/pre

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 0a60472bb5..cfcfa800c2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -845,6 +845,8 @@ patterns are available:
 
 - `rust` suitable for source code in the Rust language.
 
+- `scheme` suitable for source code in the Scheme language.
+
 - `tex` suitable for source code for LaTeX documents.
 
 
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 9675bc17db..823ea96acb 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -48,6 +48,7 @@ diffpatterns="
 	python
 	ruby
 	rust
+	scheme
 	tex
 	custom1
 	custom2
diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
new file mode 100644
index 0000000000..603b99cea4
--- /dev/null
+++ b/t/t4018/scheme-define-syntax
@@ -0,0 +1,8 @@
+(define-syntax define-test-suite RIGHT
+  (syntax-rules ()
+    ((_ suite-name (name test) ChangeMe ...)
+     (define suite-name
+       (let ((tests
+              `((name . ,test) ...)))
+         (lambda ()
+           (ChangeMe 'suite-name tests)))))))
\ No newline at end of file
diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
new file mode 100644
index 0000000000..90e75dcce8
--- /dev/null
+++ b/t/t4018/scheme-local-define
@@ -0,0 +1,4 @@
+(define (higher-order)
+  (define local-function RIGHT
+    (lambda (x)
+     (car "this is" "ChangeMe"))))
\ No newline at end of file
diff --git a/t/t4018/scheme-top-level-define b/t/t4018/scheme-top-level-define
new file mode 100644
index 0000000000..03acdc628d
--- /dev/null
+++ b/t/t4018/scheme-top-level-define
@@ -0,0 +1,4 @@
+(define (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
\ No newline at end of file
diff --git a/t/t4018/scheme-user-defined-define b/t/t4018/scheme-user-defined-define
new file mode 100644
index 0000000000..401093bac3
--- /dev/null
+++ b/t/t4018/scheme-user-defined-define
@@ -0,0 +1,6 @@
+(define-test-suite record-case-tests RIGHT
+  (record-case-1 (lambda (fail)
+                   (let ((a (make-foo 1 2)))
+                     (record-case a
+                       ((bar x) (ChangeMe))
+                       ((foo a b) (+ a b)))))))
\ No newline at end of file
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 56f1e62a97..ee7721ab91 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -325,6 +325,7 @@ test_language_driver perl
 test_language_driver php
 test_language_driver python
 test_language_driver ruby
+test_language_driver scheme
 test_language_driver tex
 
 test_expect_success 'word-diff with diff.sbe' '
diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect
new file mode 100644
index 0000000000..eed21e803c
--- /dev/null
+++ b/t/t4034/scheme/expect
@@ -0,0 +1,9 @@
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 6a5efba..7c4a6b4 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
+<CYAN>@@ -1,4 +1,4 @@<RESET>
+(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)
+  ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.
+  (let ((c (<RED>+ a b<RESET><GREEN>add1 first<RESET>)))
+    (format "one more than the total is %d" (<RED>add1<RESET><GREEN>+<RESET> c <GREEN>second<RESET>))))
diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post
new file mode 100644
index 0000000000..7c4a6b4f3d
--- /dev/null
+++ b/t/t4034/scheme/post
@@ -0,0 +1,4 @@
+(define (my-func first second)
+  ; This is a (moderately) cool function.
+  (let ((c (add1 first)))
+    (format "one more than the total is %d" (+ c second))))
\ No newline at end of file
diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre
new file mode 100644
index 0000000000..6a5efbae61
--- /dev/null
+++ b/t/t4034/scheme/pre
@@ -0,0 +1,4 @@
+(define (myfunc a b)
+  ; This is a really cool function.
+  (let ((c (+ a b)))
+    (format "one more than the total is %d" (add1 c))))
\ No newline at end of file
diff --git a/userdiff.c b/userdiff.c
index 3f81a2261c..c51a8c98ba 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -191,6 +191,14 @@ PATTERNS("rust",
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
+PATTERNS("scheme",
+         "^[\t ]*(\\(define-?.*)$",
+         /* 
+          * Scheme allows symbol names to have any character,
+          * as long as it is not a form of a parenthesis.
+          * The spaces must be escaped.
+          */
+         "(\\.|[^][)(\\}\\{ ])+"),
 PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
 	 "[={}\"]|[^={}\" \t]+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
-- 
2.31.0


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-27 17:39 [GSOC][PATCH] userdiff: add support for Scheme Atharva Raykar
@ 2021-03-27 22:50 ` Junio C Hamano
  2021-03-27 23:09   ` Junio C Hamano
  2021-03-28 11:51   ` Atharva Raykar
  2021-03-27 23:46 ` Johannes Sixt
  2021-04-03 13:16 ` [GSoC][PATCH v2 0/1] userdiff: add support for scheme Atharva Raykar
  2 siblings, 2 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-03-27 22:50 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: git

Atharva Raykar <raykar.ath@gmail.com> writes:

> diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
> new file mode 100644
> index 0000000000..603b99cea4
> --- /dev/null
> +++ b/t/t4018/scheme-define-syntax
> @@ -0,0 +1,8 @@
> +(define-syntax define-test-suite RIGHT
> +  (syntax-rules ()
> +    ((_ suite-name (name test) ChangeMe ...)
> +     (define suite-name
> +       (let ((tests
> +              `((name . ,test) ...)))
> +         (lambda ()
> +           (ChangeMe 'suite-name tests)))))))
> \ No newline at end of file

Is there a good reason to leave the final line incomplete?  If there
isn't, complete it (applies to other newly-created files in the patch).

> diff --git a/userdiff.c b/userdiff.c
> index 3f81a2261c..c51a8c98ba 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -191,6 +191,14 @@ PATTERNS("rust",
>  	 "[a-zA-Z_][a-zA-Z0-9_]*"
>  	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>  	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
> +PATTERNS("scheme",
> +         "^[\t ]*(\\(define-?.*)$",

Didn't "git diff HEAD" before committing (or "git show") highlighted
these whitespace errors?

.git/rebase-apply/patch:183: indent with spaces.
         "^[\t ]*(\\(define-?.*)$",
.git/rebase-apply/patch:184: trailing whitespace, indent with spaces.
         /* 
.git/rebase-apply/patch:185: indent with spaces.
          * Scheme allows symbol names to have any character,
.git/rebase-apply/patch:186: indent with spaces.
          * as long as it is not a form of a parenthesis.
.git/rebase-apply/patch:187: indent with spaces.
          * The spaces must be escaped.
warning: squelched 2 whitespace errors
warning: 7 lines applied after fixing whitespace errors.


> +         /* 
> +          * Scheme allows symbol names to have any character,
> +          * as long as it is not a form of a parenthesis.
> +          * The spaces must be escaped.
> +          */
> +         "(\\.|[^][)(\\}\\{ ])+"),

One or more "dot or anything other than SP or parentheses"?  But
a dot "." is neither a space or any {bra-ce} letter, so would the
above be equivalent to

	"[^][()\\{\\} \t]+"

I wonder...

I am also trying to figure out what you wanted to achieve by
mentioning "The spaces must be escaped.".  Did you mean something
like (string->symbol "a symbol with SP in it") is a symbol?  Even
so, I cannot quite guess the significance of that fact wrt the
regexp you added here?

As we are trying to catch program identifiers (symbols in scheme)
and numeric literals, treating any group of non-whitespace letters
that is delimited by one or more whitespaces as a "word" would be a
good first-order approximation, but in addition, as can be seen in
an example like (a(b(c))), parentheses can also serve as such "word
delimiters" in addition to whitespaces.  So the regexp given above
makes sense to me from that angle, especially if you do not limit
the whitespace to only SP, but include HT (\t) as well.  But was
that how you came up with the regexp?

Thanks.

>  PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>  	 "[={}\"]|[^={}\" \t]+"),
>  PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-27 22:50 ` Junio C Hamano
@ 2021-03-27 23:09   ` Junio C Hamano
  2021-03-28  3:16     ` Ævar Arnfjörð Bjarmason
  2021-03-28 12:45     ` Atharva Raykar
  2021-03-28 11:51   ` Atharva Raykar
  1 sibling, 2 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-03-27 23:09 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: git

Junio C Hamano <gitster@pobox.com> writes:

> Atharva Raykar <raykar.ath@gmail.com> writes:
> ...
>> +           (ChangeMe 'suite-name tests)))))))
>> \ No newline at end of file
>
> Is there a good reason to leave the final line incomplete?  ...
> I am also trying to figure out what you wanted to achieve ...

Taking all of them together, here is what I hope you may agree as
its improved version.  The only differences from what you posted are
corrections to all the "\ No newline at end of file" and the simplification
of the pattern (remove "a dot" from the alternative and add \t next
to SP).  Without changes, the new tests still pass so ... ;-)

    diff --git c/userdiff.c w/userdiff.c
    index 5fd0eb31ec..685fe712aa 100644
    --- c/userdiff.c
    +++ w/userdiff.c
    @@ -193,12 +193,8 @@ PATTERNS("rust",
             "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
     PATTERNS("scheme",
             "^[\t ]*(\\(define-?.*)$",
    -	 /*
    -	  * Scheme allows symbol names to have any character,
    -	  * as long as it is not a form of a parenthesis.
    -	  * The spaces must be escaped.
    -	  */
    -	 "(\\.|[^][)(\\}\\{ ])+"),
    +	 /* whitespace separated tokens, but parentheses also can delimit words */
    +	 "([^][)(\\}\\{ \t])+"),
     PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
             "[={}\"]|[^={}\" \t]+"),
     PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",

----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----
From: Atharva Raykar <raykar.ath@gmail.com>
Date: Sat, 27 Mar 2021 23:09:38 +0530
Subject: [PATCH] userdiff: add support for Scheme

Add a diff driver for Scheme (R5RS and R6RS) which
recognizes top level and local `define` forms,
whether it is a function definition, binding, syntax
definition or a user-defined `define-xyzzy` form.

The rationale for picking `define` forms for the
hunk headers is because it is usually the only
significant form for defining the structure of the
program, and it is a common pattern for schemers to
have local function definitions to hide their
visibility, so it is not only the top level
`define`'s that are of interest. Schemers also
extend the language with macros to provide their
own define forms (for example, something like a
`define-test-suite`) which is also captured in the
hunk header.

Since the identifier syntax is quite forgiving, we start
our word regexp from "words delimited by whitespaces" and
then loosen to include various forms of parentheses characters
to word-delimiters.

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
[jc: simplified word regex and its explanation; fixed whitespace errors]
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/gitattributes.txt    | 2 ++
 t/t4018-diff-funcname.sh           | 1 +
 t/t4018/scheme-define-syntax       | 8 ++++++++
 t/t4018/scheme-local-define        | 4 ++++
 t/t4018/scheme-top-level-define    | 4 ++++
 t/t4018/scheme-user-defined-define | 6 ++++++
 t/t4034-diff-words.sh              | 1 +
 t/t4034/scheme/expect              | 9 +++++++++
 t/t4034/scheme/post                | 4 ++++
 t/t4034/scheme/pre                 | 4 ++++
 userdiff.c                         | 4 ++++
 11 files changed, 47 insertions(+)
 create mode 100644 t/t4018/scheme-define-syntax
 create mode 100644 t/t4018/scheme-local-define
 create mode 100644 t/t4018/scheme-top-level-define
 create mode 100644 t/t4018/scheme-user-defined-define
 create mode 100644 t/t4034/scheme/expect
 create mode 100644 t/t4034/scheme/post
 create mode 100644 t/t4034/scheme/pre

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 0a60472bb5..cfcfa800c2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -845,6 +845,8 @@ patterns are available:
 
 - `rust` suitable for source code in the Rust language.
 
+- `scheme` suitable for source code in the Scheme language.
+
 - `tex` suitable for source code for LaTeX documents.
 
 
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 9675bc17db..823ea96acb 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -48,6 +48,7 @@ diffpatterns="
 	python
 	ruby
 	rust
+	scheme
 	tex
 	custom1
 	custom2
diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
new file mode 100644
index 0000000000..33fa50c844
--- /dev/null
+++ b/t/t4018/scheme-define-syntax
@@ -0,0 +1,8 @@
+(define-syntax define-test-suite RIGHT
+  (syntax-rules ()
+    ((_ suite-name (name test) ChangeMe ...)
+     (define suite-name
+       (let ((tests
+              `((name . ,test) ...)))
+         (lambda ()
+           (ChangeMe 'suite-name tests)))))))
diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
new file mode 100644
index 0000000000..bc6d8aebbe
--- /dev/null
+++ b/t/t4018/scheme-local-define
@@ -0,0 +1,4 @@
+(define (higher-order)
+  (define local-function RIGHT
+    (lambda (x)
+     (car "this is" "ChangeMe"))))
diff --git a/t/t4018/scheme-top-level-define b/t/t4018/scheme-top-level-define
new file mode 100644
index 0000000000..624743c22b
--- /dev/null
+++ b/t/t4018/scheme-top-level-define
@@ -0,0 +1,4 @@
+(define (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
diff --git a/t/t4018/scheme-user-defined-define b/t/t4018/scheme-user-defined-define
new file mode 100644
index 0000000000..70e403c5e2
--- /dev/null
+++ b/t/t4018/scheme-user-defined-define
@@ -0,0 +1,6 @@
+(define-test-suite record-case-tests RIGHT
+  (record-case-1 (lambda (fail)
+                   (let ((a (make-foo 1 2)))
+                     (record-case a
+                       ((bar x) (ChangeMe))
+                       ((foo a b) (+ a b)))))))
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 56f1e62a97..ee7721ab91 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -325,6 +325,7 @@ test_language_driver perl
 test_language_driver php
 test_language_driver python
 test_language_driver ruby
+test_language_driver scheme
 test_language_driver tex
 
 test_expect_success 'word-diff with diff.sbe' '
diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect
new file mode 100644
index 0000000000..eed21e803c
--- /dev/null
+++ b/t/t4034/scheme/expect
@@ -0,0 +1,9 @@
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 6a5efba..7c4a6b4 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
+<CYAN>@@ -1,4 +1,4 @@<RESET>
+(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)
+  ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.
+  (let ((c (<RED>+ a b<RESET><GREEN>add1 first<RESET>)))
+    (format "one more than the total is %d" (<RED>add1<RESET><GREEN>+<RESET> c <GREEN>second<RESET>))))
diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post
new file mode 100644
index 0000000000..28f59c6584
--- /dev/null
+++ b/t/t4034/scheme/post
@@ -0,0 +1,4 @@
+(define (my-func first second)
+  ; This is a (moderately) cool function.
+  (let ((c (add1 first)))
+    (format "one more than the total is %d" (+ c second))))
diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre
new file mode 100644
index 0000000000..4bd0069493
--- /dev/null
+++ b/t/t4034/scheme/pre
@@ -0,0 +1,4 @@
+(define (myfunc a b)
+  ; This is a really cool function.
+  (let ((c (+ a b)))
+    (format "one more than the total is %d" (add1 c))))
diff --git a/userdiff.c b/userdiff.c
index 3f81a2261c..685fe712aa 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -191,6 +191,10 @@ PATTERNS("rust",
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
+PATTERNS("scheme",
+	 "^[\t ]*(\\(define-?.*)$",
+	 /* whitespace separated tokens, but parentheses also can delimit words */
+	 "([^][)(\\}\\{ \t])+"),
 PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
 	 "[={}\"]|[^={}\" \t]+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
-- 
2.31.1-255-g3df2b433e7


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-27 17:39 [GSOC][PATCH] userdiff: add support for Scheme Atharva Raykar
  2021-03-27 22:50 ` Junio C Hamano
@ 2021-03-27 23:46 ` Johannes Sixt
  2021-03-28 12:23   ` Atharva Raykar
  2021-04-03 13:16 ` [GSoC][PATCH v2 0/1] userdiff: add support for scheme Atharva Raykar
  2 siblings, 1 reply; 35+ messages in thread
From: Johannes Sixt @ 2021-03-27 23:46 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: git, Junio C Hamano

Am 27.03.21 um 18:39 schrieb Atharva Raykar:
>  - By best-effort attempt at the wordregex, I mean that it is a little
>    more permissive than it has to be, as it accepts a few words that are
>    technically invalid in Scheme.
>    Making it handle all cases like numbers and identifiers with separate
>    regexen would be greatly complicated (Eg: #x#e10.2f3 is a valid number
>    but #x#f10.2e3 is not; 10t1 is a valid identifier, but 10s1 is a number
>    -- my wordregex just clubs all of these into a generic 'word match' which
>    trades of granularity for simplicity, and it usually does the right thing).

It is ok to have regex that capture tokens that are not valid. A
userdiff driver can assume that it operates only text that is valid in
the language.

> diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
> new file mode 100644
> index 0000000000..603b99cea4
> --- /dev/null
> +++ b/t/t4018/scheme-define-syntax
> @@ -0,0 +1,8 @@
> +(define-syntax define-test-suite RIGHT
> +  (syntax-rules ()
> +    ((_ suite-name (name test) ChangeMe ...)
> +     (define suite-name

This test is suspicious. Notice the "ChangeMe" above? That is sufficient
to let the test case succeed. The "ChangeMe" in the last line below
should be the only one.

But then there is this indented '(define' that is not marked as RIGHT,
and I wonder how is it different from...

> +       (let ((tests
> +              `((name . ,test) ...)))
> +         (lambda ()
> +           (ChangeMe 'suite-name tests)))))))
> \ No newline at end of file
> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
> new file mode 100644
> index 0000000000..90e75dcce8
> --- /dev/null
> +++ b/t/t4018/scheme-local-define
> @@ -0,0 +1,4 @@
> +(define (higher-order)
> +  (define local-function RIGHT

... this one, which is also indented and *is* marked as RIGHT.

BTW, it's good to see test cases for both indented and not-indented
trigger lines.

> +    (lambda (x)
> +     (car "this is" "ChangeMe"))))
> \ No newline at end of file

> diff --git a/userdiff.c b/userdiff.c
> index 3f81a2261c..c51a8c98ba 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -191,6 +191,14 @@ PATTERNS("rust",
>  	 "[a-zA-Z_][a-zA-Z0-9_]*"
>  	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>  	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
> +PATTERNS("scheme",
> +         "^[\t ]*(\\(define-?.*)$",

This "optional hyphen followed by anything" in the regex is strange.
Wouldn't that also capture a line that looks like, e.g.,

    (defined-foo bar)

Perhaps we want "define[- \t].*" in the regex?

> +         /* 
> +          * Scheme allows symbol names to have any character,
> +          * as long as it is not a form of a parenthesis.
> +          * The spaces must be escaped.
> +          */
> +         "(\\.|[^][)(\\}\\{ ])+"),
>  PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>  	 "[={}\"]|[^={}\" \t]+"),
>  PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
> 

-- Hannes

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-27 23:09   ` Junio C Hamano
@ 2021-03-28  3:16     ` Ævar Arnfjörð Bjarmason
  2021-03-28  5:37       ` Junio C Hamano
  2021-03-28 12:40       ` Atharva Raykar
  2021-03-28 12:45     ` Atharva Raykar
  1 sibling, 2 replies; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-28  3:16 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Atharva Raykar, git


On Sun, Mar 28 2021, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Atharva Raykar <raykar.ath@gmail.com> writes:
>> ...
>>> +           (ChangeMe 'suite-name tests)))))))
>>> \ No newline at end of file
>>
>> Is there a good reason to leave the final line incomplete?  ...
>> I am also trying to figure out what you wanted to achieve ...
>
> Taking all of them together, here is what I hope you may agree as
> its improved version.  The only differences from what you posted are
> corrections to all the "\ No newline at end of file" and the simplification
> of the pattern (remove "a dot" from the alternative and add \t next
> to SP).  Without changes, the new tests still pass so ... ;-)
>
>     diff --git c/userdiff.c w/userdiff.c
>     index 5fd0eb31ec..685fe712aa 100644
>     --- c/userdiff.c
>     +++ w/userdiff.c
>     @@ -193,12 +193,8 @@ PATTERNS("rust",
>              "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>      PATTERNS("scheme",
>              "^[\t ]*(\\(define-?.*)$",
>     -	 /*
>     -	  * Scheme allows symbol names to have any character,
>     -	  * as long as it is not a form of a parenthesis.
>     -	  * The spaces must be escaped.
>     -	  */
>     -	 "(\\.|[^][)(\\}\\{ ])+"),
>     +	 /* whitespace separated tokens, but parentheses also can delimit words */
>     +	 "([^][)(\\}\\{ \t])+"),
>      PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>              "[={}\"]|[^={}\" \t]+"),
>      PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
>
> ----- >8 ---------- >8 ---------- >8 ---------- >8 ---------- >8 -----
> From: Atharva Raykar <raykar.ath@gmail.com>
> Date: Sat, 27 Mar 2021 23:09:38 +0530
> Subject: [PATCH] userdiff: add support for Scheme
>
> Add a diff driver for Scheme (R5RS and R6RS) which
> recognizes top level and local `define` forms,
> whether it is a function definition, binding, syntax
> definition or a user-defined `define-xyzzy` form.
>
> The rationale for picking `define` forms for the
> hunk headers is because it is usually the only
> significant form for defining the structure of the
> program, and it is a common pattern for schemers to
> have local function definitions to hide their
> visibility, so it is not only the top level
> `define`'s that are of interest. Schemers also
> extend the language with macros to provide their
> own define forms (for example, something like a
> `define-test-suite`) which is also captured in the
> hunk header.
>
> Since the identifier syntax is quite forgiving, we start
> our word regexp from "words delimited by whitespaces" and
> then loosen to include various forms of parentheses characters
> to word-delimiters.
>
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> [jc: simplified word regex and its explanation; fixed whitespace errors]
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/gitattributes.txt    | 2 ++
>  t/t4018-diff-funcname.sh           | 1 +
>  t/t4018/scheme-define-syntax       | 8 ++++++++
>  t/t4018/scheme-local-define        | 4 ++++
>  t/t4018/scheme-top-level-define    | 4 ++++
>  t/t4018/scheme-user-defined-define | 6 ++++++
>  t/t4034-diff-words.sh              | 1 +
>  t/t4034/scheme/expect              | 9 +++++++++
>  t/t4034/scheme/post                | 4 ++++
>  t/t4034/scheme/pre                 | 4 ++++
>  userdiff.c                         | 4 ++++
>  11 files changed, 47 insertions(+)
>  create mode 100644 t/t4018/scheme-define-syntax
>  create mode 100644 t/t4018/scheme-local-define
>  create mode 100644 t/t4018/scheme-top-level-define
>  create mode 100644 t/t4018/scheme-user-defined-define
>  create mode 100644 t/t4034/scheme/expect
>  create mode 100644 t/t4034/scheme/post
>  create mode 100644 t/t4034/scheme/pre
>
> diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
> index 0a60472bb5..cfcfa800c2 100644
> --- a/Documentation/gitattributes.txt
> +++ b/Documentation/gitattributes.txt
> @@ -845,6 +845,8 @@ patterns are available:
>  
>  - `rust` suitable for source code in the Rust language.
>  
> +- `scheme` suitable for source code in the Scheme language.
> +
>  - `tex` suitable for source code for LaTeX documents.
>  
>  
> diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
> index 9675bc17db..823ea96acb 100755
> --- a/t/t4018-diff-funcname.sh
> +++ b/t/t4018-diff-funcname.sh
> @@ -48,6 +48,7 @@ diffpatterns="
>  	python
>  	ruby
>  	rust
> +	scheme
>  	tex
>  	custom1
>  	custom2
> diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
> new file mode 100644
> index 0000000000..33fa50c844
> --- /dev/null
> +++ b/t/t4018/scheme-define-syntax
> @@ -0,0 +1,8 @@
> +(define-syntax define-test-suite RIGHT
> +  (syntax-rules ()
> +    ((_ suite-name (name test) ChangeMe ...)
> +     (define suite-name
> +       (let ((tests
> +              `((name . ,test) ...)))
> +         (lambda ()
> +           (ChangeMe 'suite-name tests)))))))
> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
> new file mode 100644
> index 0000000000..bc6d8aebbe
> --- /dev/null
> +++ b/t/t4018/scheme-local-define
> @@ -0,0 +1,4 @@
> +(define (higher-order)
> +  (define local-function RIGHT
> +    (lambda (x)
> +     (car "this is" "ChangeMe"))))
> diff --git a/t/t4018/scheme-top-level-define b/t/t4018/scheme-top-level-define
> new file mode 100644
> index 0000000000..624743c22b
> --- /dev/null
> +++ b/t/t4018/scheme-top-level-define
> @@ -0,0 +1,4 @@
> +(define (some-func x y z) RIGHT
> +  (let ((a x)
> +        (b y))
> +        (ChangeMe a b)))
> diff --git a/t/t4018/scheme-user-defined-define b/t/t4018/scheme-user-defined-define
> new file mode 100644
> index 0000000000..70e403c5e2
> --- /dev/null
> +++ b/t/t4018/scheme-user-defined-define
> @@ -0,0 +1,6 @@
> +(define-test-suite record-case-tests RIGHT
> +  (record-case-1 (lambda (fail)
> +                   (let ((a (make-foo 1 2)))
> +                     (record-case a
> +                       ((bar x) (ChangeMe))
> +                       ((foo a b) (+ a b)))))))
> diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
> index 56f1e62a97..ee7721ab91 100755
> --- a/t/t4034-diff-words.sh
> +++ b/t/t4034-diff-words.sh
> @@ -325,6 +325,7 @@ test_language_driver perl
>  test_language_driver php
>  test_language_driver python
>  test_language_driver ruby
> +test_language_driver scheme
>  test_language_driver tex
>  
>  test_expect_success 'word-diff with diff.sbe' '
> diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect
> new file mode 100644
> index 0000000000..eed21e803c
> --- /dev/null
> +++ b/t/t4034/scheme/expect
> @@ -0,0 +1,9 @@
> +<BOLD>diff --git a/pre b/post<RESET>
> +<BOLD>index 6a5efba..7c4a6b4 100644<RESET>
> +<BOLD>--- a/pre<RESET>
> +<BOLD>+++ b/post<RESET>
> +<CYAN>@@ -1,4 +1,4 @@<RESET>
> +(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)
> +  ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.
> +  (let ((c (<RED>+ a b<RESET><GREEN>add1 first<RESET>)))
> +    (format "one more than the total is %d" (<RED>add1<RESET><GREEN>+<RESET> c <GREEN>second<RESET>))))
> diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post
> new file mode 100644
> index 0000000000..28f59c6584
> --- /dev/null
> +++ b/t/t4034/scheme/post
> @@ -0,0 +1,4 @@
> +(define (my-func first second)
> +  ; This is a (moderately) cool function.
> +  (let ((c (add1 first)))
> +    (format "one more than the total is %d" (+ c second))))
> diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre
> new file mode 100644
> index 0000000000..4bd0069493
> --- /dev/null
> +++ b/t/t4034/scheme/pre
> @@ -0,0 +1,4 @@
> +(define (myfunc a b)
> +  ; This is a really cool function.
> +  (let ((c (+ a b)))
> +    (format "one more than the total is %d" (add1 c))))
> diff --git a/userdiff.c b/userdiff.c
> index 3f81a2261c..685fe712aa 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -191,6 +191,10 @@ PATTERNS("rust",
>  	 "[a-zA-Z_][a-zA-Z0-9_]*"
>  	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>  	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
> +PATTERNS("scheme",
> +	 "^[\t ]*(\\(define-?.*)$",

The "define-?.*" can be simplified to just "define.*", but looking at
the tests is that the intent? From the tests it looks like "define[- ]"
is what the author wants, unless this is meant to also match
"(definements".

Has this been tested on some real-world scheme code? E.g. I have guile
installed locally, and it has really large top-level eval-when
blocks. These rules would jump over those to whatever the function above
them is.

> +	 /* whitespace separated tokens, but parentheses also can delimit words */
> +	 "([^][)(\\}\\{ \t])+"),
>  PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>  	 "[={}\"]|[^={}\" \t]+"),
>  PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-28  3:16     ` Ævar Arnfjörð Bjarmason
@ 2021-03-28  5:37       ` Junio C Hamano
  2021-03-28 12:40       ` Atharva Raykar
  1 sibling, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-03-28  5:37 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: Ævar Arnfjörð Bjarmason, git

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

[jc: redirecting the question to patch author---I am just a messenger]

>> diff --git a/userdiff.c b/userdiff.c
>> index 3f81a2261c..685fe712aa 100644
>> --- a/userdiff.c
>> +++ b/userdiff.c
>> @@ -191,6 +191,10 @@ PATTERNS("rust",
>>  	 "[a-zA-Z_][a-zA-Z0-9_]*"
>>  	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>>  	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>> +PATTERNS("scheme",
>> +	 "^[\t ]*(\\(define-?.*)$",
>
> The "define-?.*" can be simplified to just "define.*", but looking at
> the tests is that the intent? From the tests it looks like "define[- ]"
> is what the author wants, unless this is meant to also match
> "(definements".
>
> Has this been tested on some real-world scheme code? E.g. I have guile
> installed locally, and it has really large top-level eval-when
> blocks. These rules would jump over those to whatever the function above
> them is.
>
>> +	 /* whitespace separated tokens, but parentheses also can delimit words */
>> +	 "([^][)(\\}\\{ \t])+"),
>>  PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>>  	 "[={}\"]|[^={}\" \t]+"),
>>  PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-27 22:50 ` Junio C Hamano
  2021-03-27 23:09   ` Junio C Hamano
@ 2021-03-28 11:51   ` Atharva Raykar
  2021-03-28 18:06     ` Junio C Hamano
  2021-03-29 10:12     ` Phillip Wood
  1 sibling, 2 replies; 35+ messages in thread
From: Atharva Raykar @ 2021-03-28 11:51 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On 28-Mar-2021, at 04:20, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Atharva Raykar <raykar.ath@gmail.com> writes:
> 
>> diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
>> new file mode 100644
>> index 0000000000..603b99cea4
>> --- /dev/null
>> +++ b/t/t4018/scheme-define-syntax
>> @@ -0,0 +1,8 @@
>> +(define-syntax define-test-suite RIGHT
>> +  (syntax-rules ()
>> +    ((_ suite-name (name test) ChangeMe ...)
>> +     (define suite-name
>> +       (let ((tests
>> +              `((name . ,test) ...)))
>> +         (lambda ()
>> +           (ChangeMe 'suite-name tests)))))))
>> \ No newline at end of file
> 
> Is there a good reason to leave the final line incomplete?  If there
> isn't, complete it (applies to other newly-created files in the patch).

Will do.

>> diff --git a/userdiff.c b/userdiff.c
>> index 3f81a2261c..c51a8c98ba 100644
>> --- a/userdiff.c
>> +++ b/userdiff.c
>> @@ -191,6 +191,14 @@ PATTERNS("rust",
>> 	 "[a-zA-Z_][a-zA-Z0-9_]*"
>> 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>> 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>> +PATTERNS("scheme",
>> +         "^[\t ]*(\\(define-?.*)$",
> 
> Didn't "git diff HEAD" before committing (or "git show") highlighted
> these whitespace errors?
> 
> .git/rebase-apply/patch:183: indent with spaces.
>         "^[\t ]*(\\(define-?.*)$",
> .git/rebase-apply/patch:184: trailing whitespace, indent with spaces.
>         /* 
> .git/rebase-apply/patch:185: indent with spaces.
>          * Scheme allows symbol names to have any character,
> .git/rebase-apply/patch:186: indent with spaces.
>          * as long as it is not a form of a parenthesis.
> .git/rebase-apply/patch:187: indent with spaces.
>          * The spaces must be escaped.
> warning: squelched 2 whitespace errors
> warning: 7 lines applied after fixing whitespace errors.

It did highlight the spaces (which I accidentally overlooked), but I
didn’t receive these warnings. It shows up with the --check flag though.
I'll recheck my configuration. Thanks for pointing this out.

> 
>> +         /* 
>> +          * Scheme allows symbol names to have any character,
>> +          * as long as it is not a form of a parenthesis.
>> +          * The spaces must be escaped.
>> +          */
>> +         "(\\.|[^][)(\\}\\{ ])+"),
> 
> One or more "dot or anything other than SP or parentheses"?  But
> a dot "." is neither a space or any {bra-ce} letter, so would the
> above be equivalent to
> 
> 	"[^][()\\{\\} \t]+"
> 
> I wonder...

A backslash is allowed in scheme identifiers, and I erroneously thought that
the first part handles the case for identifiers such as `component\new` or 
`\"id-with-quotes\"`. (I tested it with a regex engine that behaves differently
than the one git is using, my bad.)

> I am also trying to figure out what you wanted to achieve by
> mentioning "The spaces must be escaped.".  Did you mean something
> like (string->symbol "a symbol with SP in it") is a symbol?  Even
> so, I cannot quite guess the significance of that fact wrt the
> regexp you added here?

I initially tried using identifiers like `space\ separated` and they
seemed to work in my REPL, but turns out space separated identifiers in
scheme do not work with backslashes, and it was working because of the way
my terminal handled escaping. Space separated identifiers are declared like
`|space separated|` and this too only seems to work with Racket, not
the other Scheme implementations. So I stand corrected here, and it's better
to drop this feature altogether.

But somehow, the regexp you suggested, ie:

	"[^][()\\{\\} \t]+"

does not handle the case of make\foo -> make\bar (it will only diff on foo).
I am not too sure why it treats backslashes as delimiters.

This seems to actually do what I was going for:

	"(\\\\|[^][)(\\}\\{ ])+"

> As we are trying to catch program identifiers (symbols in scheme)
> and numeric literals, treating any group of non-whitespace letters
> that is delimited by one or more whitespaces as a "word" would be a
> good first-order approximation, but in addition, as can be seen in
> an example like (a(b(c))), parentheses can also serve as such "word
> delimiters" in addition to whitespaces.  So the regexp given above
> makes sense to me from that angle, especially if you do not limit
> the whitespace to only SP, but include HT (\t) as well.  But was
> that how you came up with the regexp?

Yes, this is exactly what I was trying to express. All words should be
delimited by either whitespace or a parenthesis, and all other special
characters should be accepted as part of the word.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-27 23:46 ` Johannes Sixt
@ 2021-03-28 12:23   ` Atharva Raykar
  2021-03-29 10:18     ` Phillip Wood
  0 siblings, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-03-28 12:23 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: git, Junio C Hamano

On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
>> diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
>> new file mode 100644
>> index 0000000000..603b99cea4
>> --- /dev/null
>> +++ b/t/t4018/scheme-define-syntax
>> @@ -0,0 +1,8 @@
>> +(define-syntax define-test-suite RIGHT
>> +  (syntax-rules ()
>> +    ((_ suite-name (name test) ChangeMe ...)
>> +     (define suite-name
> 
> This test is suspicious. Notice the "ChangeMe" above? That is sufficient
> to let the test case succeed. The "ChangeMe" in the last line below
> should be the only one.

Thanks for pointing this out. The second "ChangeMe" was not supposed to be
there.

What I wanted to test was the hunk header showing the line for
'(define-syntax ...' and not the internal '(define ...' below it. Thus the
ChangeMe should be located above the internal define so that the hunk header
would show define-syntax and not the local define.

> But then there is this indented '(define' that is not marked as RIGHT,
> and I wonder how is it different from...
> 
>> +       (let ((tests
>> +              `((name . ,test) ...)))
>> +         (lambda ()
>> +           (ChangeMe 'suite-name tests)))))))
>> \ No newline at end of file
>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>> new file mode 100644
>> index 0000000000..90e75dcce8
>> --- /dev/null
>> +++ b/t/t4018/scheme-local-define
>> @@ -0,0 +1,4 @@
>> +(define (higher-order)
>> +  (define local-function RIGHT
> 
> ... this one, which is also indented and *is* marked as RIGHT.

In this test case, I was explicitly testing for an indented '(define'
whereas in the former, I was testing for the top-level '(define-syntax',
which happened to have an internal define (which will inevitably show up
in a lot of scheme code).

>> +    (lambda (x)
>> +     (car "this is" "ChangeMe"))))
>> \ No newline at end of file
> 
>> diff --git a/userdiff.c b/userdiff.c
>> index 3f81a2261c..c51a8c98ba 100644
>> --- a/userdiff.c
>> +++ b/userdiff.c
>> @@ -191,6 +191,14 @@ PATTERNS("rust",
>> 	 "[a-zA-Z_][a-zA-Z0-9_]*"
>> 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>> 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>> +PATTERNS("scheme",
>> +         "^[\t ]*(\\(define-?.*)$",
> 
> This "optional hyphen followed by anything" in the regex is strange.
> Wouldn't that also capture a line that looks like, e.g.,
> 
>    (defined-foo bar)
> 
> Perhaps we want "define[- \t].*" in the regex?

Yes, this is what I intended to do, thanks for correcting it.


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-28  3:16     ` Ævar Arnfjörð Bjarmason
  2021-03-28  5:37       ` Junio C Hamano
@ 2021-03-28 12:40       ` Atharva Raykar
  2021-03-29 10:08         ` Phillip Wood
  1 sibling, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-03-28 12:40 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Junio C Hamano, git

On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> The "define-?.*" can be simplified to just "define.*", but looking at
> the tests is that the intent? From the tests it looks like "define[- ]"
> is what the author wants, unless this is meant to also match
> "(definements".

Yes, you captured my intent correctly. Will fix it.

> Has this been tested on some real-world scheme code? E.g. I have guile
> installed locally, and it has really large top-level eval-when
> blocks. These rules would jump over those to whatever the function above
> them is.

I do not have a large scheme codebase on my own, I usually use Racket,
which is a much larger language with many more forms. Other Schemes like
Guile also extend the language a lot, like in your example, eval-when is
an extension provided by Guile (and Chicken and Chez), but not a part of
the R6RS document when I searched its index.

So the 'define' forms are the only one that I know would reliably be present
across all schemes. But one can also make a case where some of these non-standard
forms may be common enough that they are worth adding in. In that case which
forms to include? Should we consider everything in the SRFI's[1]? Should the
various module definitions of Racket be included? It's a little tricky to know
where to stop.

That being said, I will try to run this through more Scheme codebases that I can
find and see if there are any forms that seem to show up commonly enough that they
are worth including.

[1] https://en.wikipedia.org/wiki/Scheme_Requests_for_Implementation

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-27 23:09   ` Junio C Hamano
  2021-03-28  3:16     ` Ævar Arnfjörð Bjarmason
@ 2021-03-28 12:45     ` Atharva Raykar
  1 sibling, 0 replies; 35+ messages in thread
From: Atharva Raykar @ 2021-03-28 12:45 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, j6t, avarab

On 28-Mar-2021, at 04:39, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Junio C Hamano <gitster@pobox.com> writes:
> 
>> Atharva Raykar <raykar.ath@gmail.com> writes:
>> ...
>>> +           (ChangeMe 'suite-name tests)))))))
>>> \ No newline at end of file
>> 
>> Is there a good reason to leave the final line incomplete?  ...
>> I am also trying to figure out what you wanted to achieve ...
> 
> Taking all of them together, here is what I hope you may agree as
> its improved version.  The only differences from what you posted are
> corrections to all the "\ No newline at end of file" and the simplification
> of the pattern (remove "a dot" from the alternative and add \t next
> to SP).  Without changes, the new tests still pass so ... ;-)
> 
>    diff --git c/userdiff.c w/userdiff.c
>    index 5fd0eb31ec..685fe712aa 100644
>    --- c/userdiff.c
>    +++ w/userdiff.c
>    @@ -193,12 +193,8 @@ PATTERNS("rust",
>             "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>     PATTERNS("scheme",
>             "^[\t ]*(\\(define-?.*)$",
>    -	 /*
>    -	  * Scheme allows symbol names to have any character,
>    -	  * as long as it is not a form of a parenthesis.
>    -	  * The spaces must be escaped.
>    -	  */
>    -	 "(\\.|[^][)(\\}\\{ ])+"),
>    +	 /* whitespace separated tokens, but parentheses also can delimit words */
>    +	 "([^][)(\\}\\{ \t])+"),
>     PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>             "[={}\"]|[^={}\" \t]+"),
>     PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",

Thanks for these. I will eventually send another patch with the whitespaces corrected,
and try to see if there is a better way to handle backslashes, other than the regex I
suggested. I will also be writing another test case to check that case properly.

I will also incorporate the other changes suggested by Johannes and Ævar as well,
My regex was not supposed to capture forms like `defined-thing`. And there are a
few rough edges with some of my test cases, which I will correct as well in the next
patch. It is also worth spending some more time and see if there is any other form other
than definitions that a Scheme programmer other than myself may be interested in. I will
consult a few Scheme communities and mailing lists and see what more experienced
programmers have to say.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-28 11:51   ` Atharva Raykar
@ 2021-03-28 18:06     ` Junio C Hamano
  2021-03-29  8:12       ` Atharva Raykar
  2021-03-29 10:12     ` Phillip Wood
  1 sibling, 1 reply; 35+ messages in thread
From: Junio C Hamano @ 2021-03-28 18:06 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: git

Atharva Raykar <raykar.ath@gmail.com> writes:

>>> +         "(\\.|[^][)(\\}\\{ ])+"),
>> 
>> One or more "dot or anything other than SP or parentheses"?  But
>> a dot "." is neither a space or any {bra-ce} letter, so would the
>> above be equivalent to
>> 
>> 	"[^][()\\{\\} \t]+"
>> 
>> I wonder...
>
> A backslash is allowed in scheme identifiers, and I erroneously thought that
> the first part handles the case for identifiers such as `component\new` or 
> `\"id-with-quotes\"`. (I tested it with a regex engine that behaves differently
> than the one git is using, my bad.)

Ah, perhaps you didn't have enough backslashes.  A half of the
doubled one before the dot is eaten by the C compiler, so the regexp
engine is seeing only a single backslash before the dot, which means
"literally a single dot".  If you meant "literally a single
backslash, followed by any single char", you probably would write 4
backslashes and a dot---half of the backslashes would be eaten by
the compiler, so you'd be passing two backslashes and a dot, which
is probably what you meant.

Having said that, two further points.

 - the "anything but whitespaces and various forms of parentheses"
   set would include backslash, so 'component\new' would be taken as
   a single word with "[^][()\\{\\} \t]+", wouldn't it?

 - how common is the use of backslashes in identifiers?  I am trying
   to see if the additional complexity needed to support them is
   worth the benefit.

> But somehow, the regexp you suggested, ie:
>
> 	"[^][()\\{\\} \t]+"
>
> does not handle the case of make\foo -> make\bar (it will only diff on foo).
> I am not too sure why it treats backslashes as delimiters.

Perhaps because you have included two backslashes inside [] to say
"backslash is not a word character" in the original, and I blindly
copied that?  IOW, do you need to quote {} inside []?

> Yes, this is exactly what I was trying to express. All words should be
> delimited by either whitespace or a parenthesis, and all other special
> characters should be accepted as part of the word.

That sentence after "All words should be..." would be a good comment
to replace what you wrote in the original, then ;-).

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-28 18:06     ` Junio C Hamano
@ 2021-03-29  8:12       ` Atharva Raykar
  2021-03-29 20:47         ` Junio C Hamano
  0 siblings, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-03-29  8:12 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git

On 28-Mar-2021, at 23:36, Junio C Hamano <gitster@pobox.com> wrote:
> 
> Atharva Raykar <raykar.ath@gmail.com> writes:
> 
>>>> +         "(\\.|[^][)(\\}\\{ ])+"),
>>> 
>>> One or more "dot or anything other than SP or parentheses"?  But
>>> a dot "." is neither a space or any {bra-ce} letter, so would the
>>> above be equivalent to
>>> 
>>> 	"[^][()\\{\\} \t]+"
>>> 
>>> I wonder...
>> 
>> A backslash is allowed in scheme identifiers, and I erroneously thought that
>> the first part handles the case for identifiers such as `component\new` or 
>> `\"id-with-quotes\"`. (I tested it with a regex engine that behaves differently
>> than the one git is using, my bad.)
> 
> Ah, perhaps you didn't have enough backslashes.  A half of the
> doubled one before the dot is eaten by the C compiler, so the regexp
> engine is seeing only a single backslash before the dot, which means
> "literally a single dot".  If you meant "literally a single
> backslash, followed by any single char", you probably would write 4
> backslashes and a dot---half of the backslashes would be eaten by
> the compiler, so you'd be passing two backslashes and a dot, which
> is probably what you meant.
> 
> Having said that, two further points.
> 
> - the "anything but whitespaces and various forms of parentheses"
>   set would include backslash, so 'component\new' would be taken as
>   a single word with "[^][()\\{\\} \t]+", wouldn't it?
> 
> - how common is the use of backslashes in identifiers?  I am trying
>   to see if the additional complexity needed to support them is
>   worth the benefit.

I have refined the regex, and now it is much simpler and does all of what
I want it to:

	"([^][)(}{[:space:]])+"

I did not have to escape the various parentheses, so I avoided the need to
handle backslashes separately. The "\\t" was causing problems as well because
it took it as a '\' followed by a 't' (Thanks to j416 on #git-devel for
helping me out on this).

>> Yes, this is exactly what I was trying to express. All words should be
>> delimited by either whitespace or a parenthesis, and all other special
>> characters should be accepted as part of the word.
> 
> That sentence after "All words should be..." would be a good comment
> to replace what you wrote in the original, then ;-).

Yes, that should make it a lot more clear.


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-28 12:40       ` Atharva Raykar
@ 2021-03-29 10:08         ` Phillip Wood
  2021-03-30  6:41           ` Atharva Raykar
  0 siblings, 1 reply; 35+ messages in thread
From: Phillip Wood @ 2021-03-29 10:08 UTC (permalink / raw)
  To: Atharva Raykar, Ævar Arnfjörð Bjarmason
  Cc: Junio C Hamano, git

Hi Atharva

On 28/03/2021 13:40, Atharva Raykar wrote:
> On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>> The "define-?.*" can be simplified to just "define.*", but looking at
>> the tests is that the intent? From the tests it looks like "define[- ]"
>> is what the author wants, unless this is meant to also match
>> "(definements".
> 
> Yes, you captured my intent correctly. Will fix it.
> 
>> Has this been tested on some real-world scheme code? E.g. I have guile
>> installed locally, and it has really large top-level eval-when
>> blocks. These rules would jump over those to whatever the function above
>> them is.
> 
> I do not have a large scheme codebase on my own, I usually use Racket,
> which is a much larger language with many more forms. Other Schemes like
> Guile also extend the language a lot, like in your example, eval-when is
> an extension provided by Guile (and Chicken and Chez), but not a part of
> the R6RS document when I searched its index.
> 
> So the 'define' forms are the only one that I know would reliably be present
> across all schemes. But one can also make a case where some of these non-standard
> forms may be common enough that they are worth adding in. In that case which
> forms to include? Should we consider everything in the SRFI's[1]? Should the
> various module definitions of Racket be included? It's a little tricky to know
> where to stop.

If there are some common forms such as eval-when then it would be good 
to include them, otherwise we end up needing a different rule for each 
scheme implementation as they all seem to tweak something. Gerbil uses 
'def...' e.g def, defsyntax, defstruct, defrules rather than define, 
define-syntax, define-record etc. I'm not user if we want to accommodate 
that or not.

Best Wishes

Phillip


> That being said, I will try to run this through more Scheme codebases that I can
> find and see if there are any forms that seem to show up commonly enough that they
> are worth including.
> 
> [1] https://en.wikipedia.org/wiki/Scheme_Requests_for_Implementation
> 


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-28 11:51   ` Atharva Raykar
  2021-03-28 18:06     ` Junio C Hamano
@ 2021-03-29 10:12     ` Phillip Wood
  1 sibling, 0 replies; 35+ messages in thread
From: Phillip Wood @ 2021-03-29 10:12 UTC (permalink / raw)
  To: Atharva Raykar, Junio C Hamano; +Cc: git

Hi Atharva

On 28/03/2021 12:51, Atharva Raykar wrote:
> On 28-Mar-2021, at 04:20, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> Atharva Raykar <raykar.ath@gmail.com> writes:
>>
>>> +         /*
>>> +          * Scheme allows symbol names to have any character,
>>> +          * as long as it is not a form of a parenthesis.
>>> +          * The spaces must be escaped.
>>> +          */
>>> +         "(\\.|[^][)(\\}\\{ ])+"),
>>
>> One or more "dot or anything other than SP or parentheses"?  But
>> a dot "." is neither a space or any {bra-ce} letter, so would the
>> above be equivalent to
>>
>> 	"[^][()\\{\\} \t]+"
>>
>> I wonder...
> 
> A backslash is allowed in scheme identifiers, and I erroneously thought that
> the first part handles the case for identifiers such as `component\new` or
> `\"id-with-quotes\"`. (I tested it with a regex engine that behaves differently
> than the one git is using, my bad.)
> 
>> I am also trying to figure out what you wanted to achieve by
>> mentioning "The spaces must be escaped.".  Did you mean something
>> like (string->symbol "a symbol with SP in it") is a symbol?  Even
>> so, I cannot quite guess the significance of that fact wrt the
>> regexp you added here?
> 
> I initially tried using identifiers like `space\ separated` and they
> seemed to work in my REPL, but turns out space separated identifiers in
> scheme do not work with backslashes, and it was working because of the way
> my terminal handled escaping. Space separated identifiers are declared like
> `|space separated|` and this too only seems to work with Racket, not
> the other Scheme implementations.

I think the bar notation works with some other such as gambit and 
possibly guile (it's a while since I used the latter)

Best wishes

Phillip

  So I stand corrected here, and it's better
> to drop this feature altogether.
> 
> But somehow, the regexp you suggested, ie:
> 
> 	"[^][()\\{\\} \t]+"
> 
> does not handle the case of make\foo -> make\bar (it will only diff on foo).
> I am not too sure why it treats backslashes as delimiters.
> 
> This seems to actually do what I was going for:
> 
> 	"(\\\\|[^][)(\\}\\{ ])+"
> 
>> As we are trying to catch program identifiers (symbols in scheme)
>> and numeric literals, treating any group of non-whitespace letters
>> that is delimited by one or more whitespaces as a "word" would be a
>> good first-order approximation, but in addition, as can be seen in
>> an example like (a(b(c))), parentheses can also serve as such "word
>> delimiters" in addition to whitespaces.  So the regexp given above
>> makes sense to me from that angle, especially if you do not limit
>> the whitespace to only SP, but include HT (\t) as well.  But was
>> that how you came up with the regexp?
> 
> Yes, this is exactly what I was trying to express. All words should be
> delimited by either whitespace or a parenthesis, and all other special
> characters should be accepted as part of the word.
> 


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-28 12:23   ` Atharva Raykar
@ 2021-03-29 10:18     ` Phillip Wood
  2021-03-29 10:48       ` Johannes Sixt
  2021-03-30  7:04       ` Atharva Raykar
  0 siblings, 2 replies; 35+ messages in thread
From: Phillip Wood @ 2021-03-29 10:18 UTC (permalink / raw)
  To: Atharva Raykar, Johannes Sixt; +Cc: git, Junio C Hamano

Hi Atharva

On 28/03/2021 13:23, Atharva Raykar wrote:
> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
 > [...]
>>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>>> new file mode 100644
>>> index 0000000000..90e75dcce8
>>> --- /dev/null
>>> +++ b/t/t4018/scheme-local-define
>>> @@ -0,0 +1,4 @@
>>> +(define (higher-order)
>>> +  (define local-function RIGHT
>>
>> ... this one, which is also indented and *is* marked as RIGHT.
> 
> In this test case, I was explicitly testing for an indented '(define'
> whereas in the former, I was testing for the top-level '(define-syntax',
> which happened to have an internal define (which will inevitably show up
> in a lot of scheme code).

It would be nice to include indented define forms but including them 
means that any change to the body of a function is attributed to the 
last internal definition rather than the actual function. For example

(define (f arg)
   (define (g x)
     (+ 1 x))

   (some-func ...)
   ;;any change here will have '(define (g x)' in the hunk header, not 
'(define (f arg)'

I don't think this can be avoided as we rely on regexs rather than 
parsing the source so it is probably best to only match toplevel defines.

Best Wishes

Phillip

> 
>>> +    (lambda (x)
>>> +     (car "this is" "ChangeMe"))))
>>> \ No newline at end of file
>>
>>> diff --git a/userdiff.c b/userdiff.c
>>> index 3f81a2261c..c51a8c98ba 100644
>>> --- a/userdiff.c
>>> +++ b/userdiff.c
>>> @@ -191,6 +191,14 @@ PATTERNS("rust",
>>> 	 "[a-zA-Z_][a-zA-Z0-9_]*"
>>> 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>>> 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>>> +PATTERNS("scheme",
>>> +         "^[\t ]*(\\(define-?.*)$",
>>
>> This "optional hyphen followed by anything" in the regex is strange.
>> Wouldn't that also capture a line that looks like, e.g.,
>>
>>     (defined-foo bar)
>>
>> Perhaps we want "define[- \t].*" in the regex?
> 
> Yes, this is what I intended to do, thanks for correcting it.
> 


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-29 10:18     ` Phillip Wood
@ 2021-03-29 10:48       ` Johannes Sixt
  2021-03-29 13:12         ` Ævar Arnfjörð Bjarmason
  2021-03-30  7:04       ` Atharva Raykar
  1 sibling, 1 reply; 35+ messages in thread
From: Johannes Sixt @ 2021-03-29 10:48 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Atharva Raykar, git, Junio C Hamano

Am 29.03.21 um 12:18 schrieb Phillip Wood:
> It would be nice to include indented define forms but including them
> means that any change to the body of a function is attributed to the
> last internal definition rather than the actual function. For example
> 
> (define (f arg)
>   (define (g x)
>     (+ 1 x))
> 
>   (some-func ...)
>   ;;any change here will have '(define (g x)' in the hunk header, not
> '(define (f arg)'
> 
> I don't think this can be avoided as we rely on regexs rather than
> parsing the source so it is probably best to only match toplevel defines.

There can be two rules, one that matches '(define-' that is indented,
and another one that matches all non-indented forms of definitions. If
that is what you mean.

-- Hannes

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-29 10:48       ` Johannes Sixt
@ 2021-03-29 13:12         ` Ævar Arnfjörð Bjarmason
  2021-03-29 14:06           ` Phillip Wood
  0 siblings, 1 reply; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-29 13:12 UTC (permalink / raw)
  To: Johannes Sixt; +Cc: Phillip Wood, Atharva Raykar, git, Junio C Hamano


On Mon, Mar 29 2021, Johannes Sixt wrote:

> Am 29.03.21 um 12:18 schrieb Phillip Wood:
>> It would be nice to include indented define forms but including them
>> means that any change to the body of a function is attributed to the
>> last internal definition rather than the actual function. For example
>> 
>> (define (f arg)
>>   (define (g x)
>>     (+ 1 x))
>> 
>>   (some-func ...)
>>   ;;any change here will have '(define (g x)' in the hunk header, not
>> '(define (f arg)'
>> 
>> I don't think this can be avoided as we rely on regexs rather than
>> parsing the source so it is probably best to only match toplevel defines.
>
> There can be two rules, one that matches '(define-' that is indented,
> and another one that matches all non-indented forms of definitions. If
> that is what you mean.

Yes, but that doesn't help in these sorts of cases because what a rule
like that really wants is some version of "don't match this line, but
only if you can reasonably match this other rule".

We can only do rule precedence on a per-line basis via the inverted
matches.

So for languages like cl/elisp/scheme and others where it's common to
have nested function definitions (then -W would like the top-level) *OR*
similarly looking nested function definitions, but the top-level isn't a
function but a (setq) or whatever we're basically stuck with picking one
or the other.

I've pondered how to get around this problem in my userdiff.c hacking
without resorting to supporting some general-purpose Turing machine, and
have so far come up with nothing.

You can see lots of prior art by grepping Emacs's source code for
beginning-of-defun, it solves this problem by exposing a Turing machine
:)

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-29 13:12         ` Ævar Arnfjörð Bjarmason
@ 2021-03-29 14:06           ` Phillip Wood
  0 siblings, 0 replies; 35+ messages in thread
From: Phillip Wood @ 2021-03-29 14:06 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason, Johannes Sixt
  Cc: Atharva Raykar, git, Junio C Hamano

On 29/03/2021 14:12, Ævar Arnfjörð Bjarmason wrote:
> 
> On Mon, Mar 29 2021, Johannes Sixt wrote:
> 
>> Am 29.03.21 um 12:18 schrieb Phillip Wood:
>>> It would be nice to include indented define forms but including them
>>> means that any change to the body of a function is attributed to the
>>> last internal definition rather than the actual function. For example
>>>
>>> (define (f arg)
>>>    (define (g x)
>>>      (+ 1 x))
>>>
>>>    (some-func ...)
>>>    ;;any change here will have '(define (g x)' in the hunk header, not
>>> '(define (f arg)'
>>>
>>> I don't think this can be avoided as we rely on regexs rather than
>>> parsing the source so it is probably best to only match toplevel defines.
>>
>> There can be two rules, one that matches '(define-' that is indented,
>> and another one that matches all non-indented forms of definitions. If
>> that is what you mean.
> 
> Yes, but that doesn't help in these sorts of cases because what a rule
> like that really wants is some version of "don't match this line, but
> only if you can reasonably match this other rule".
> 
> We can only do rule precedence on a per-line basis via the inverted
> matches.
> 
> So for languages like cl/elisp/scheme and others where it's common to
> have nested function definitions (then -W would like the top-level) *OR*
> similarly looking nested function definitions, but the top-level isn't a
> function but a (setq) or whatever we're basically stuck with picking one
> or the other.

Exactly

> I've pondered how to get around this problem in my userdiff.c hacking
> without resorting to supporting some general-purpose Turing machine, and
> have so far come up with nothing.

I think using an indentation heuristic would probably work quite well 
for most languages - see 
https://public-inbox.org/git/20200923215859.102981-1-rtzoeller@rtzoeller.com/ 
for a discussion from last year (from memory there were some problems 
with the approach in those patches but I think there are some suggestion 
from Peff and me later in the thread on how they could be overcome)

Best Wishes

Phillip


> You can see lots of prior art by grepping Emacs's source code for
> beginning-of-defun, it solves this problem by exposing a Turing machine
> :)
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-29  8:12       ` Atharva Raykar
@ 2021-03-29 20:47         ` Junio C Hamano
  0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-03-29 20:47 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: git

Atharva Raykar <raykar.ath@gmail.com> writes:

>> Having said that, two further points.
>> 
>> - the "anything but whitespaces and various forms of parentheses"
>>   set would include backslash, so 'component\new' would be taken as
>>   a single word with "[^][()\\{\\} \t]+", wouldn't it?
>> 
>> - how common is the use of backslashes in identifiers?  I am trying
>>   to see if the additional complexity needed to support them is
>>   worth the benefit.
>
> I have refined the regex, and now it is much simpler and does all of what
> I want it to:
>
> 	"([^][)(}{[:space:]])+"

OK, [:space:] is already used elsewhere, so it would be OK.

In practice, the only difference from "[ \t]" (which is used in many
other patterns in the same file) is that [:space:] class includes
form-feed (\Ctrl-L); nobody would write vertical-tab in the code,
and the matching is done one line at a time, so the fact that LF (or
CRLF) is in the [:space:] class does not make a difference anyway.

> I did not have to escape the various parentheses, so I avoided the need to
> handle backslashes separately. The "\\t" was causing problems as well because

If you spelled "\\t" that would have caused a problem of your own
making ;-)

I think what I gave in the message you are responding to was a
single backslash followed by a 't', to let the compiler turn them
into a single HT character, and that wouldn't have had such a
problem---in fact "[ \t]" is used in many other existing rules in
the same file.

Thanks.


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-29 10:08         ` Phillip Wood
@ 2021-03-30  6:41           ` Atharva Raykar
  2021-03-30 12:56             ` Ævar Arnfjörð Bjarmason
  0 siblings, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-03-30  6:41 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Ævar Arnfjörð Bjarmason, Junio C Hamano, git


On 29-Mar-2021, at 15:38, Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 28/03/2021 13:40, Atharva Raykar wrote:
>> On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>> The "define-?.*" can be simplified to just "define.*", but looking at
>>> the tests is that the intent? From the tests it looks like "define[- ]"
>>> is what the author wants, unless this is meant to also match
>>> "(definements".
>> Yes, you captured my intent correctly. Will fix it.
>>> Has this been tested on some real-world scheme code? E.g. I have guile
>>> installed locally, and it has really large top-level eval-when
>>> blocks. These rules would jump over those to whatever the function above
>>> them is.
>> I do not have a large scheme codebase on my own, I usually use Racket,
>> which is a much larger language with many more forms. Other Schemes like
>> Guile also extend the language a lot, like in your example, eval-when is
>> an extension provided by Guile (and Chicken and Chez), but not a part of
>> the R6RS document when I searched its index.
>> So the 'define' forms are the only one that I know would reliably be present
>> across all schemes. But one can also make a case where some of these non-standard
>> forms may be common enough that they are worth adding in. In that case which
>> forms to include? Should we consider everything in the SRFI's[1]? Should the
>> various module definitions of Racket be included? It's a little tricky to know
>> where to stop.
> 
> If there are some common forms such as eval-when then it would be good to include them, otherwise we end up needing a different rule for each scheme implementation as they all seem to tweak something. Gerbil uses 'def...' e.g def, defsyntax, defstruct, defrules rather than define, define-syntax, define-record etc. I'm not user if we want to accommodate that or not.

Yes, this is the part that is hard for me to figure out. I am going by
two heuristics: what Scheme communities in other places would generally
prefer, and what patterns I see happen more often in scheme code.

The former is tricky to do. I posted to a few mailing lists about this,
but they don't seem active enough to garner any responses.

The latter is a little easier to measure quickly. I did a GitHub search,
where I filtered results to only consider Scheme files (language:scheme).

Some armchair stats, just for a broad understanding:

  Total number of scheme files: 529,339
  No. of times a construct is used in those files:
    define and its variants : 431,090 (81.4%)
    def and its variants    :  18,466 ( 3.5%)
    eval-when               :   3,375 ( 0.6%)

There was no way for me to quickly know which of these uses are at the top
level, but either way of the more structural forms that do show up in Scheme
code, define and its variants seem like a clear winner. I am not sure if
it's worth adding more rules to check for def and its variants, given that
they are not nearly as common.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-29 10:18     ` Phillip Wood
  2021-03-29 10:48       ` Johannes Sixt
@ 2021-03-30  7:04       ` Atharva Raykar
  2021-03-30 10:22         ` Atharva Raykar
  1 sibling, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-03-30  7:04 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Johannes Sixt, git, Junio C Hamano



> On 29-Mar-2021, at 15:48, Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
> Hi Atharva
> 
> On 28/03/2021 13:23, Atharva Raykar wrote:
>> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
> > [...]
>>>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>>>> new file mode 100644
>>>> index 0000000000..90e75dcce8
>>>> --- /dev/null
>>>> +++ b/t/t4018/scheme-local-define
>>>> @@ -0,0 +1,4 @@
>>>> +(define (higher-order)
>>>> +  (define local-function RIGHT
>>> 
>>> ... this one, which is also indented and *is* marked as RIGHT.
>> In this test case, I was explicitly testing for an indented '(define'
>> whereas in the former, I was testing for the top-level '(define-syntax',
>> which happened to have an internal define (which will inevitably show up
>> in a lot of scheme code).
> 
> It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
> 
> (define (f arg)
>  (define (g x)
>    (+ 1 x))
> 
>  (some-func ...)
>  ;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'

The reason I went for this over the top level forms, is because
I felt it was useful to see the nearest definition for internal
functions that often have a lot of the actual business logic of
the program (at least a lot of SICP seems to follow this pattern).
The disadvantage is as you said, it might also catch trivial inner
functions and the developer might lose context.

Another problem is it may match more trivial bindings, like:

(define (some-func things)
  ...
  (define items '(eggs
                  ham
                  peanut-butter))
  ...)

What I have noticed *anecdotally* is that this is not common enough
to be too much of a problem, and local define bindings seem to be more
favoured in Racket than other Schemes, that use 'let' more often.

> I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.

The other issue with only matching top level defines is that a
lot of scheme programs are library definitions, something like

(library
    (foo bar)
  (export ...)
  (define ...)
  (define ...)
  ;; and a bunch of other definitions...
)

Only matching top level defines will completely ignore matching all
the definitions in these files.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-30  7:04       ` Atharva Raykar
@ 2021-03-30 10:22         ` Atharva Raykar
  2021-04-05 10:04           ` Phillip Wood
  0 siblings, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-03-30 10:22 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Johannes Sixt, git, Junio C Hamano



> On 30-Mar-2021, at 12:34, Atharva Raykar <raykar.ath@gmail.com> wrote:
> 
> 
> 
>> On 29-Mar-2021, at 15:48, Phillip Wood <phillip.wood123@gmail.com> wrote:
>> 
>> Hi Atharva
>> 
>> On 28/03/2021 13:23, Atharva Raykar wrote:
>>> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
>>> [...]
>>>>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>>>>> new file mode 100644
>>>>> index 0000000000..90e75dcce8
>>>>> --- /dev/null
>>>>> +++ b/t/t4018/scheme-local-define
>>>>> @@ -0,0 +1,4 @@
>>>>> +(define (higher-order)
>>>>> +  (define local-function RIGHT
>>>> 
>>>> ... this one, which is also indented and *is* marked as RIGHT.
>>> In this test case, I was explicitly testing for an indented '(define'
>>> whereas in the former, I was testing for the top-level '(define-syntax',
>>> which happened to have an internal define (which will inevitably show up
>>> in a lot of scheme code).
>> 
>> It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
>> 
>> (define (f arg)
>> (define (g x)
>>   (+ 1 x))
>> 
>> (some-func ...)
>> ;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
> 
> The reason I went for this over the top level forms, is because
> I felt it was useful to see the nearest definition for internal
> functions that often have a lot of the actual business logic of
> the program (at least a lot of SICP seems to follow this pattern).
> The disadvantage is as you said, it might also catch trivial inner
> functions and the developer might lose context.

Never mind this message, I had misunderstood the problem you were trying to
demonstrate. I wholeheartedly agree with what you are trying to say, and
the indentation heuristic discussed does look interesting. I shall have a
glance at the RFC you linked in the other reply.

> The disadvantage is as you said, it might also catch trivial inner
> functions and the developer might lose context.

Feel free to disregard me misquoting you here. You did not say that (:

> Another problem is it may match more trivial bindings, like:
> 
> (define (some-func things)
>  ...
>  (define items '(eggs
>                  ham
>                  peanut-butter))
>  ...)
> 
> What I have noticed *anecdotally* is that this is not common enough
> to be too much of a problem, and local define bindings seem to be more
> favoured in Racket than other Schemes, that use 'let' more often.
> 
>> I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
> 
> The other issue with only matching top level defines is that a
> lot of scheme programs are library definitions, something like
> 
> (library
>    (foo bar)
>  (export ...)
>  (define ...)
>  (define ...)
>  ;; and a bunch of other definitions...
> )
> 
> Only matching top level defines will completely ignore matching all
> the definitions in these files.

That said, I still stand by the fact that only catching top level defines
will lead to a lot of definitions being ignored. Maybe the occasional
mismatch may be worth the gain in the number of function contexts being
detected?



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-30  6:41           ` Atharva Raykar
@ 2021-03-30 12:56             ` Ævar Arnfjörð Bjarmason
  2021-03-30 13:48               ` Atharva Raykar
  0 siblings, 1 reply; 35+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-03-30 12:56 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: Phillip Wood, Junio C Hamano, git


On Tue, Mar 30 2021, Atharva Raykar wrote:

> On 29-Mar-2021, at 15:38, Phillip Wood <phillip.wood123@gmail.com> wrote:
>> On 28/03/2021 13:40, Atharva Raykar wrote:
>>> On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>> The "define-?.*" can be simplified to just "define.*", but looking at
>>>> the tests is that the intent? From the tests it looks like "define[- ]"
>>>> is what the author wants, unless this is meant to also match
>>>> "(definements".
>>> Yes, you captured my intent correctly. Will fix it.
>>>> Has this been tested on some real-world scheme code? E.g. I have guile
>>>> installed locally, and it has really large top-level eval-when
>>>> blocks. These rules would jump over those to whatever the function above
>>>> them is.
>>> I do not have a large scheme codebase on my own, I usually use Racket,
>>> which is a much larger language with many more forms. Other Schemes like
>>> Guile also extend the language a lot, like in your example, eval-when is
>>> an extension provided by Guile (and Chicken and Chez), but not a part of
>>> the R6RS document when I searched its index.
>>> So the 'define' forms are the only one that I know would reliably be present
>>> across all schemes. But one can also make a case where some of these non-standard
>>> forms may be common enough that they are worth adding in. In that case which
>>> forms to include? Should we consider everything in the SRFI's[1]? Should the
>>> various module definitions of Racket be included? It's a little tricky to know
>>> where to stop.
>> 
>> If there are some common forms such as eval-when then it would be good to include them, otherwise we end up needing a different rule for each scheme implementation as they all seem to tweak something. Gerbil uses 'def...' e.g def, defsyntax, defstruct, defrules rather than define, define-syntax, define-record etc. I'm not user if we want to accommodate that or not.
>
> Yes, this is the part that is hard for me to figure out. I am going by
> two heuristics: what Scheme communities in other places would generally
> prefer, and what patterns I see happen more often in scheme code.
>
> The former is tricky to do. I posted to a few mailing lists about this,
> but they don't seem active enough to garner any responses.
>
> The latter is a little easier to measure quickly. I did a GitHub search,
> where I filtered results to only consider Scheme files (language:scheme).
>
> Some armchair stats, just for a broad understanding:
>
>   Total number of scheme files: 529,339
>   No. of times a construct is used in those files:
>     define and its variants : 431,090 (81.4%)
>     def and its variants    :  18,466 ( 3.5%)
>     eval-when               :   3,375 ( 0.6%)
>
> There was no way for me to quickly know which of these uses are at the top
> level, but either way of the more structural forms that do show up in Scheme
> code, define and its variants seem like a clear winner. I am not sure if
> it's worth adding more rules to check for def and its variants, given that
> they are not nearly as common.

In those cases we should veer on the side of inclusion. The only problem
we'll have is if "eval-when" is a "setq"-like function top-level form in
some other scheme dialect, so we'll have a conflict.

Otherwise it's fine, programs that only use "define" won't be bothered
by an eval-when rule.

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-30 12:56             ` Ævar Arnfjörð Bjarmason
@ 2021-03-30 13:48               ` Atharva Raykar
  0 siblings, 0 replies; 35+ messages in thread
From: Atharva Raykar @ 2021-03-30 13:48 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason; +Cc: Phillip Wood, Junio C Hamano, git

On 30-Mar-2021, at 18:26, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
> 
> 
> On Tue, Mar 30 2021, Atharva Raykar wrote:
> 
>> On 29-Mar-2021, at 15:38, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> On 28/03/2021 13:40, Atharva Raykar wrote:
>>>> On 28-Mar-2021, at 08:46, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:
>>>>> The "define-?.*" can be simplified to just "define.*", but looking at
>>>>> the tests is that the intent? From the tests it looks like "define[- ]"
>>>>> is what the author wants, unless this is meant to also match
>>>>> "(definements".
>>>> Yes, you captured my intent correctly. Will fix it.
>>>>> Has this been tested on some real-world scheme code? E.g. I have guile
>>>>> installed locally, and it has really large top-level eval-when
>>>>> blocks. These rules would jump over those to whatever the function above
>>>>> them is.
>>>> I do not have a large scheme codebase on my own, I usually use Racket,
>>>> which is a much larger language with many more forms. Other Schemes like
>>>> Guile also extend the language a lot, like in your example, eval-when is
>>>> an extension provided by Guile (and Chicken and Chez), but not a part of
>>>> the R6RS document when I searched its index.
>>>> So the 'define' forms are the only one that I know would reliably be present
>>>> across all schemes. But one can also make a case where some of these non-standard
>>>> forms may be common enough that they are worth adding in. In that case which
>>>> forms to include? Should we consider everything in the SRFI's[1]? Should the
>>>> various module definitions of Racket be included? It's a little tricky to know
>>>> where to stop.
>>> 
>>> If there are some common forms such as eval-when then it would be good to include them, otherwise we end up needing a different rule for each scheme implementation as they all seem to tweak something. Gerbil uses 'def...' e.g def, defsyntax, defstruct, defrules rather than define, define-syntax, define-record etc. I'm not user if we want to accommodate that or not.
>> 
>> Yes, this is the part that is hard for me to figure out. I am going by
>> two heuristics: what Scheme communities in other places would generally
>> prefer, and what patterns I see happen more often in scheme code.
>> 
>> The former is tricky to do. I posted to a few mailing lists about this,
>> but they don't seem active enough to garner any responses.
>> 
>> The latter is a little easier to measure quickly. I did a GitHub search,
>> where I filtered results to only consider Scheme files (language:scheme).
>> 
>> Some armchair stats, just for a broad understanding:
>> 
>>  Total number of scheme files: 529,339
>>  No. of times a construct is used in those files:
>>    define and its variants : 431,090 (81.4%)
>>    def and its variants    :  18,466 ( 3.5%)
>>    eval-when               :   3,375 ( 0.6%)
>> 
>> There was no way for me to quickly know which of these uses are at the top
>> level, but either way of the more structural forms that do show up in Scheme
>> code, define and its variants seem like a clear winner. I am not sure if
>> it's worth adding more rules to check for def and its variants, given that
>> they are not nearly as common.
> 
> In those cases we should veer on the side of inclusion. The only problem
> we'll have is if "eval-when" is a "setq"-like function top-level form in
> some other scheme dialect, so we'll have a conflict.
> 
> Otherwise it's fine, programs that only use "define" won't be bothered
> by an eval-when rule.

I would like some clarification, since my knowledge of Common Lisp's setq
and Guile's/Other's eval-when is pretty surface level.

> The only problem we'll have is if "eval-when" is a "setq"-like function
> top-level form in some other scheme dialect, so we'll have a conflict.

I am not sure what you mean when you say if "eval-when" is a "setq"-like
top level form, and exactly what kind of problem it may cause.

I also realized from my understanding of the Guile Documentation[1],
that "eval-when" is used to tell the compiler which expressions should be
made available during the expansion phase.

It does not seem to have anything that may help identify the location of the
hunk, which I understand is the primary purpose of these hunk headers.
All uses of "eval-when" would be some variation of:

	(eval-when (expand load eval) ; no identifier in this form
	  ...)

unlike a "define" which will always name the nearest function, which helps as
a landmark.

Would that be a valid reason to exclude "eval-when"?


^ permalink raw reply	[flat|nested] 35+ messages in thread

* [GSoC][PATCH v2 0/1] userdiff: add support for scheme
  2021-03-27 17:39 [GSOC][PATCH] userdiff: add support for Scheme Atharva Raykar
  2021-03-27 22:50 ` Junio C Hamano
  2021-03-27 23:46 ` Johannes Sixt
@ 2021-04-03 13:16 ` Atharva Raykar
  2021-04-03 13:16   ` [GSoC][PATCH v2 1/1] " Atharva Raykar
                     ` (2 more replies)
  2 siblings, 3 replies; 35+ messages in thread
From: Atharva Raykar @ 2021-04-03 13:16 UTC (permalink / raw)
  To: git; +Cc: Atharva Raykar, gitster, avarab, j6t, phillip.wood123

Hello all,

This is v2 of the patch I sent to add userdiff support to Scheme. I have
modified my approach to be more inclusive of as many Schemes as possible and
thus added many more forms. Since Ævar suggested we veer on the side of
inclusion when talking about the Gerbil scheme syntax, I felt it made sense to
extend the driver to include common, but non-standard extensions of Scheme,
including the forms of Racket and Guile.

Forms added since last patch:

 - Variants of define such as def, defsyntax etc as suggested by Phillip and the
   other reviewers. - The library form of R6RS, as well as module definitions of
   Racket, Gerbil and Guile. * These forms were added on the recommendation of
   Göran Weinholt, creator of the Akku scheme package manager and the Loko
   Scheme implementation:
   https://groups.google.com/g/comp.lang.scheme/c/Aczn0TNEr5g/m/Jq3AlKvZBgAJ -
   The Racket forms for defining structs and classes.

I have restricted the "def" forms to only certain keywords, so that it does not
over-match to words like "deflate", "deform", "defer" etc.

I have also allowed the use of "/" after a define or def form, as some scheme
code uses it as a convention for defines in a certain context, such as Racket's
"define/public".

I have also fixed some a test case which had a redundant "ChangeMe".

Finally in the word regex, which has been simplified a lot, while retaining the
same functioning after taking into account Junio's suggestions.

Atharva Raykar (1):
  userdiff: add support for Scheme

 Documentation/gitattributes.txt    |  2 ++
 t/t4018-diff-funcname.sh           |  1 +
 t/t4018/scheme-class               |  7 +++++++
 t/t4018/scheme-def                 |  4 ++++
 t/t4018/scheme-def-variant         |  4 ++++
 t/t4018/scheme-define-slash-public |  7 +++++++
 t/t4018/scheme-define-syntax       |  8 ++++++++
 t/t4018/scheme-define-variant      |  4 ++++
 t/t4018/scheme-library             | 11 +++++++++++
 t/t4018/scheme-local-define        |  4 ++++
 t/t4018/scheme-module              |  6 ++++++
 t/t4018/scheme-top-level-define    |  4 ++++
 t/t4018/scheme-user-defined-define |  6 ++++++
 t/t4034-diff-words.sh              |  1 +
 t/t4034/scheme/expect              | 10 ++++++++++
 t/t4034/scheme/post                |  5 +++++
 t/t4034/scheme/pre                 |  5 +++++
 userdiff.c                         |  4 ++++
 18 files changed, 93 insertions(+)
 create mode 100644 t/t4018/scheme-class
 create mode 100644 t/t4018/scheme-def
 create mode 100644 t/t4018/scheme-def-variant
 create mode 100644 t/t4018/scheme-define-slash-public
 create mode 100644 t/t4018/scheme-define-syntax
 create mode 100644 t/t4018/scheme-define-variant
 create mode 100644 t/t4018/scheme-library
 create mode 100644 t/t4018/scheme-local-define
 create mode 100644 t/t4018/scheme-module
 create mode 100644 t/t4018/scheme-top-level-define
 create mode 100644 t/t4018/scheme-user-defined-define
 create mode 100644 t/t4034/scheme/expect
 create mode 100644 t/t4034/scheme/post
 create mode 100644 t/t4034/scheme/pre

-- 
2.31.1


^ permalink raw reply	[flat|nested] 35+ messages in thread

* [GSoC][PATCH v2 1/1] userdiff: add support for scheme
  2021-04-03 13:16 ` [GSoC][PATCH v2 0/1] userdiff: add support for scheme Atharva Raykar
@ 2021-04-03 13:16   ` Atharva Raykar
  2021-04-05 10:21     ` Phillip Wood
  2021-04-08  9:14   ` [GSoC][PATCH v3 0/1] " Atharva Raykar
  2021-04-08  9:14   ` [GSoC][PATCH v3 1/1] userdiff: add support for Scheme Atharva Raykar
  2 siblings, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-04-03 13:16 UTC (permalink / raw)
  To: git; +Cc: Atharva Raykar

Add a diff driver for Scheme-like languages which recognizes top level
and local `define` forms, whether it is a function definition, binding,
syntax definition or a user-defined `define-xyzzy` form.

Also supports R6RS `library` forms, `module` forms along with class and
struct declarations used in Racket (PLT Scheme).

Alternate "def" syntax such as those in Gerbil Scheme are also
supported, like defstruct, defsyntax and so on.

The rationale for picking `define` forms for the hunk headers is because
it is usually the only significant form for defining the structure of
the program, and it is a common pattern for schemers to have local
function definitions to hide their visibility, so it is not only the top
level `define`'s that are of interest. Schemers also extend the language
with macros to provide their own define forms (for example, something
like a `define-test-suite`) which is also captured in the hunk header.

Since it is common practice to extend syntax with variants of a form
like `module+`, `class*` etc, those have been supported as well.

The word regex is a best-effort attempt to conform to R6RS[1] valid
identifiers, symbols and numbers.

[1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
---
 Documentation/gitattributes.txt    |  2 ++
 t/t4018-diff-funcname.sh           |  1 +
 t/t4018/scheme-class               |  7 +++++++
 t/t4018/scheme-def                 |  4 ++++
 t/t4018/scheme-def-variant         |  4 ++++
 t/t4018/scheme-define-slash-public |  7 +++++++
 t/t4018/scheme-define-syntax       |  8 ++++++++
 t/t4018/scheme-define-variant      |  4 ++++
 t/t4018/scheme-library             | 11 +++++++++++
 t/t4018/scheme-local-define        |  4 ++++
 t/t4018/scheme-module              |  6 ++++++
 t/t4018/scheme-top-level-define    |  4 ++++
 t/t4018/scheme-user-defined-define |  6 ++++++
 t/t4034-diff-words.sh              |  1 +
 t/t4034/scheme/expect              | 10 ++++++++++
 t/t4034/scheme/post                |  5 +++++
 t/t4034/scheme/pre                 |  5 +++++
 userdiff.c                         |  4 ++++
 18 files changed, 93 insertions(+)
 create mode 100644 t/t4018/scheme-class
 create mode 100644 t/t4018/scheme-def
 create mode 100644 t/t4018/scheme-def-variant
 create mode 100644 t/t4018/scheme-define-slash-public
 create mode 100644 t/t4018/scheme-define-syntax
 create mode 100644 t/t4018/scheme-define-variant
 create mode 100644 t/t4018/scheme-library
 create mode 100644 t/t4018/scheme-local-define
 create mode 100644 t/t4018/scheme-module
 create mode 100644 t/t4018/scheme-top-level-define
 create mode 100644 t/t4018/scheme-user-defined-define
 create mode 100644 t/t4034/scheme/expect
 create mode 100644 t/t4034/scheme/post
 create mode 100644 t/t4034/scheme/pre

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 0a60472bb5..cfcfa800c2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -845,6 +845,8 @@ patterns are available:
 
 - `rust` suitable for source code in the Rust language.
 
+- `scheme` suitable for source code in the Scheme language.
+
 - `tex` suitable for source code for LaTeX documents.
 
 
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 9675bc17db..823ea96acb 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -48,6 +48,7 @@ diffpatterns="
 	python
 	ruby
 	rust
+	scheme
 	tex
 	custom1
 	custom2
diff --git a/t/t4018/scheme-class b/t/t4018/scheme-class
new file mode 100644
index 0000000000..e5e07b43fb
--- /dev/null
+++ b/t/t4018/scheme-class
@@ -0,0 +1,7 @@
+(define book-class%
+  (class* () object% RIGHT
+    (field (pages 5))
+    (field (ChangeMe 5))
+    (define/public (letters)
+      (* pages 500))
+    (super-new)))
diff --git a/t/t4018/scheme-def b/t/t4018/scheme-def
new file mode 100644
index 0000000000..1e2673da96
--- /dev/null
+++ b/t/t4018/scheme-def
@@ -0,0 +1,4 @@
+(def (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
diff --git a/t/t4018/scheme-def-variant b/t/t4018/scheme-def-variant
new file mode 100644
index 0000000000..d857a61d64
--- /dev/null
+++ b/t/t4018/scheme-def-variant
@@ -0,0 +1,4 @@
+(defmethod {print point} RIGHT
+  (lambda (self)
+    (with ((point x y) self)
+      (printf "{ChangeMe x:~a y:~a}~n" x y))))
diff --git a/t/t4018/scheme-define-slash-public b/t/t4018/scheme-define-slash-public
new file mode 100644
index 0000000000..39a93a1600
--- /dev/null
+++ b/t/t4018/scheme-define-slash-public
@@ -0,0 +1,7 @@
+(define bar-class%
+  (class object%
+    (field (info 5))
+    (define/public (foo) RIGHT
+      (+ info 42)
+      (* info ChangeMe))
+    (super-new)))
diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
new file mode 100644
index 0000000000..7d5e99e0fc
--- /dev/null
+++ b/t/t4018/scheme-define-syntax
@@ -0,0 +1,8 @@
+(define-syntax define-test-suite RIGHT
+  (syntax-rules ()
+    ((_ suite-name (name test) ChangeMe ...)
+     (define suite-name
+       (let ((tests
+              `((name . ,test) ...)))
+         (lambda ()
+           (run-suite 'suite-name tests)))))))
diff --git a/t/t4018/scheme-define-variant b/t/t4018/scheme-define-variant
new file mode 100644
index 0000000000..911708854d
--- /dev/null
+++ b/t/t4018/scheme-define-variant
@@ -0,0 +1,4 @@
+(define* (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
diff --git a/t/t4018/scheme-library b/t/t4018/scheme-library
new file mode 100644
index 0000000000..82ea3df510
--- /dev/null
+++ b/t/t4018/scheme-library
@@ -0,0 +1,11 @@
+(library (my-helpers id-stuff) RIGHT
+  (export find-dup)
+  (import (ChangeMe))
+  (define (find-dup l)
+    (and (pair? l)
+         (let loop ((rest (cdr l)))
+           (cond
+            [(null? rest) (find-dup (cdr l))]
+            [(bound-identifier=? (car l) (car rest))
+             (car rest)]
+            [else (loop (cdr rest))])))))
diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
new file mode 100644
index 0000000000..bc6d8aebbe
--- /dev/null
+++ b/t/t4018/scheme-local-define
@@ -0,0 +1,4 @@
+(define (higher-order)
+  (define local-function RIGHT
+    (lambda (x)
+     (car "this is" "ChangeMe"))))
diff --git a/t/t4018/scheme-module b/t/t4018/scheme-module
new file mode 100644
index 0000000000..edfae0ebf7
--- /dev/null
+++ b/t/t4018/scheme-module
@@ -0,0 +1,6 @@
+(module A RIGHT
+  (export with-display-exception)
+  (extern (display-exception display-exception ChangeMe))
+  (def (with-display-exception thunk)
+    (with-catch (lambda (e) (display-exception e (current-error-port)) e)
+      thunk)))
diff --git a/t/t4018/scheme-top-level-define b/t/t4018/scheme-top-level-define
new file mode 100644
index 0000000000..624743c22b
--- /dev/null
+++ b/t/t4018/scheme-top-level-define
@@ -0,0 +1,4 @@
+(define (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
diff --git a/t/t4018/scheme-user-defined-define b/t/t4018/scheme-user-defined-define
new file mode 100644
index 0000000000..35fe7cc9bf
--- /dev/null
+++ b/t/t4018/scheme-user-defined-define
@@ -0,0 +1,6 @@
+(define-test-suite record\ case-tests RIGHT
+  (record-case-1 (lambda (fail)
+                   (let ((a (make-foo 1 2)))
+                     (record-case a
+                       ((bar x) (ChangeMe))
+                       ((foo a b) (+ a b)))))))
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 56f1e62a97..ee7721ab91 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -325,6 +325,7 @@ test_language_driver perl
 test_language_driver php
 test_language_driver python
 test_language_driver ruby
+test_language_driver scheme
 test_language_driver tex
 
 test_expect_success 'word-diff with diff.sbe' '
diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect
new file mode 100644
index 0000000000..d9bb82225b
--- /dev/null
+++ b/t/t4034/scheme/expect
@@ -0,0 +1,10 @@
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index cb56df5..09d9506 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
+<CYAN>@@ -1,5 +1,5 @@<RESET>
+(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)
+  ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.
+  (<RED>this\place<RESET><GREEN>that\place<RESET> (+ 3 4))
+  (let ((c (<RED>+ a b<RESET><GREEN>add1 first<RESET>)))
+    (format "one more than the total is %d" (<RED>add1<RESET><GREEN>+<RESET> c <GREEN>second<RESET>))))
diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post
new file mode 100644
index 0000000000..06bf0b34f9
--- /dev/null
+++ b/t/t4034/scheme/post
@@ -0,0 +1,5 @@
+(define (my-func first second)
+  ; This is a (moderately) cool function.
+  (that\place (+ 3 4))
+  (let ((c (add1 first)))
+    (format "one more than the total is %d" (+ c second))))
diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre
new file mode 100644
index 0000000000..270a2d0cc5
--- /dev/null
+++ b/t/t4034/scheme/pre
@@ -0,0 +1,5 @@
+(define (myfunc a b)
+  ; This is a really cool function.
+  (this\place (+ 3 4))
+  (let ((c (+ a b)))
+    (format "one more than the total is %d" (add1 c))))
diff --git a/userdiff.c b/userdiff.c
index 3f81a2261c..ac1999bbc5 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -191,6 +191,10 @@ PATTERNS("rust",
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
+PATTERNS("scheme",
+	 "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",
+	 /* All words should be delimited by spaces or parentheses */
+	 "([^][)(}{[ \t])+"),
 PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
 	 "[={}\"]|[^={}\" \t]+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-03-30 10:22         ` Atharva Raykar
@ 2021-04-05 10:04           ` Phillip Wood
  2021-04-05 17:58             ` Johannes Sixt
  2021-04-06 12:29             ` Atharva Raykar
  0 siblings, 2 replies; 35+ messages in thread
From: Phillip Wood @ 2021-04-05 10:04 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: Johannes Sixt, git, Junio C Hamano

Hi Atharva

On 30/03/2021 11:22, Atharva Raykar wrote:
> 
> 
>> On 30-Mar-2021, at 12:34, Atharva Raykar <raykar.ath@gmail.com> wrote:
>>
>>
>>
>>> On 29-Mar-2021, at 15:48, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>
>>> Hi Atharva
>>>
>>> On 28/03/2021 13:23, Atharva Raykar wrote:
>>>> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
>>>> [...]
>>>>>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>>>>>> new file mode 100644
>>>>>> index 0000000000..90e75dcce8
>>>>>> --- /dev/null
>>>>>> +++ b/t/t4018/scheme-local-define
>>>>>> @@ -0,0 +1,4 @@
>>>>>> +(define (higher-order)
>>>>>> +  (define local-function RIGHT
>>>>>
>>>>> ... this one, which is also indented and *is* marked as RIGHT.
>>>> In this test case, I was explicitly testing for an indented '(define'
>>>> whereas in the former, I was testing for the top-level '(define-syntax',
>>>> which happened to have an internal define (which will inevitably show up
>>>> in a lot of scheme code).
>>>
>>> It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
>>>
>>> (define (f arg)
>>> (define (g x)
>>>    (+ 1 x))
>>>
>>> (some-func ...)
>>> ;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
>>
>> The reason I went for this over the top level forms, is because
>> I felt it was useful to see the nearest definition for internal
>> functions that often have a lot of the actual business logic of
>> the program (at least a lot of SICP seems to follow this pattern).
>> The disadvantage is as you said, it might also catch trivial inner
>> functions and the developer might lose context.
> 
> Never mind this message, I had misunderstood the problem you were trying to
> demonstrate. I wholeheartedly agree with what you are trying to say, and
> the indentation heuristic discussed does look interesting. I shall have a
> glance at the RFC you linked in the other reply.
> 
>> The disadvantage is as you said, it might also catch trivial inner
>> functions and the developer might lose context.
> 
> Feel free to disregard me misquoting you here. You did not say that (:
> 
>> Another problem is it may match more trivial bindings, like:
>>
>> (define (some-func things)
>>   ...
>>   (define items '(eggs
>>                   ham
>>                   peanut-butter))
>>   ...)
>>
>> What I have noticed *anecdotally* is that this is not common enough
>> to be too much of a problem, and local define bindings seem to be more
>> favoured in Racket than other Schemes, that use 'let' more often.
>>
>>> I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
>>
>> The other issue with only matching top level defines is that a
>> lot of scheme programs are library definitions, something like
>>
>> (library
>>     (foo bar)
>>   (export ...)
>>   (define ...)
>>   (define ...)
>>   ;; and a bunch of other definitions...
>> )
>>
>> Only matching top level defines will completely ignore matching all
>> the definitions in these files.
> 
> That said, I still stand by the fact that only catching top level defines
> will lead to a lot of definitions being ignored. Maybe the occasional
> mismatch may be worth the gain in the number of function contexts being
> detected?

I'm not sure that the mismatches will be occasional - every time you 
have an internal definition in a function the hunk header will be wrong 
when you change the main body of the function. This will affect grep 
--function-context and diff -W as well as the normal hunk headers. The 
problem is there is no way to avoid that and provide something useful in 
the library example you have above. It would be useful to find some code 
bases and diff the output of 'git log --patch' with and without the 
leading whitespace match in the function pattern to see how often this 
is a problem (i.e. when the funcnames do not match see which one is 
correct).

Best Wishes

Phillip



^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSoC][PATCH v2 1/1] userdiff: add support for scheme
  2021-04-03 13:16   ` [GSoC][PATCH v2 1/1] " Atharva Raykar
@ 2021-04-05 10:21     ` Phillip Wood
  2021-04-06 10:32       ` Atharva Raykar
  0 siblings, 1 reply; 35+ messages in thread
From: Phillip Wood @ 2021-04-05 10:21 UTC (permalink / raw)
  To: Atharva Raykar, git

Hi Atharva
On 03/04/2021 14:16, Atharva Raykar wrote:
> Add a diff driver for Scheme-like languages which recognizes top level
> and local `define` forms, whether it is a function definition, binding,
> syntax definition or a user-defined `define-xyzzy` form.
> 
> Also supports R6RS `library` forms, `module` forms along with class and
> struct declarations used in Racket (PLT Scheme).
> 
> Alternate "def" syntax such as those in Gerbil Scheme are also
> supported, like defstruct, defsyntax and so on.
> 
> The rationale for picking `define` forms for the hunk headers is because
> it is usually the only significant form for defining the structure of
> the program, and it is a common pattern for schemers to have local
> function definitions to hide their visibility, so it is not only the top
> level `define`'s that are of interest. Schemers also extend the language
> with macros to provide their own define forms (for example, something
> like a `define-test-suite`) which is also captured in the hunk header.
> 
> Since it is common practice to extend syntax with variants of a form
> like `module+`, `class*` etc, those have been supported as well.
> 
> The word regex is a best-effort attempt to conform to R6RS[1] valid
> identifiers, symbols and numbers.
> 
> [1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4
> 
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
>[...]
> diff --git a/userdiff.c b/userdiff.c
> index 3f81a2261c..ac1999bbc5 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -191,6 +191,10 @@ PATTERNS("rust",
>   	 "[a-zA-Z_][a-zA-Z0-9_]*"
>   	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>   	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
> +PATTERNS("scheme",
> +	 "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",
> +	 /* All words should be delimited by spaces or parentheses */
> +	 "([^][)(}{[ \t])+"),

I think it would be nice to match single '(' and '[' to highlight when 
they have been added or deleted - I find this useful when I get a syntax 
error. Also it would be nice to handle r7rs identifiers like | this is a 
symbol |. Maybe something like
"(\\|([^\\\\|]*(\\\\|)*)*\\||[^][}{)( \t]|[][(){}])"

Best Wishes

Phillip

>   PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>   	 "[={}\"]|[^={}\" \t]+"),
>   PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
> 


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-04-05 10:04           ` Phillip Wood
@ 2021-04-05 17:58             ` Johannes Sixt
  2021-04-06 12:29             ` Atharva Raykar
  1 sibling, 0 replies; 35+ messages in thread
From: Johannes Sixt @ 2021-04-05 17:58 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Atharva Raykar, git, Junio C Hamano

Am 05.04.21 um 12:04 schrieb Phillip Wood:
> Hi Atharva
> 
> On 30/03/2021 11:22, Atharva Raykar wrote:
>>
>>
>>> On 30-Mar-2021, at 12:34, Atharva Raykar <raykar.ath@gmail.com> wrote:
>>>
>>>
>>>
>>>> On 29-Mar-2021, at 15:48, Phillip Wood <phillip.wood123@gmail.com>
>>>> wrote:
>>>>
>>>> Hi Atharva
>>>>
>>>> On 28/03/2021 13:23, Atharva Raykar wrote:
>>>>> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
>>>>> [...]
>>>>>>> diff --git a/t/t4018/scheme-local-define
>>>>>>> b/t/t4018/scheme-local-define
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..90e75dcce8
>>>>>>> --- /dev/null
>>>>>>> +++ b/t/t4018/scheme-local-define
>>>>>>> @@ -0,0 +1,4 @@
>>>>>>> +(define (higher-order)
>>>>>>> +  (define local-function RIGHT
>>>>>>
>>>>>> ... this one, which is also indented and *is* marked as RIGHT.
>>>>> In this test case, I was explicitly testing for an indented '(define'
>>>>> whereas in the former, I was testing for the top-level
>>>>> '(define-syntax',
>>>>> which happened to have an internal define (which will inevitably
>>>>> show up
>>>>> in a lot of scheme code).
>>>>
>>>> It would be nice to include indented define forms but including them
>>>> means that any change to the body of a function is attributed to the
>>>> last internal definition rather than the actual function. For example
>>>>
>>>> (define (f arg)
>>>> (define (g x)
>>>>    (+ 1 x))
>>>>
>>>> (some-func ...)
>>>> ;;any change here will have '(define (g x)' in the hunk header, not
>>>> '(define (f arg)'
>>>
>>> The reason I went for this over the top level forms, is because
>>> I felt it was useful to see the nearest definition for internal
>>> functions that often have a lot of the actual business logic of
>>> the program (at least a lot of SICP seems to follow this pattern).
>>> The disadvantage is as you said, it might also catch trivial inner
>>> functions and the developer might lose context.
>>
>> Never mind this message, I had misunderstood the problem you were
>> trying to
>> demonstrate. I wholeheartedly agree with what you are trying to say, and
>> the indentation heuristic discussed does look interesting. I shall have a
>> glance at the RFC you linked in the other reply.
>>
>>> The disadvantage is as you said, it might also catch trivial inner
>>> functions and the developer might lose context.
>>
>> Feel free to disregard me misquoting you here. You did not say that (:
>>
>>> Another problem is it may match more trivial bindings, like:
>>>
>>> (define (some-func things)
>>>   ...
>>>   (define items '(eggs
>>>                   ham
>>>                   peanut-butter))
>>>   ...)
>>>
>>> What I have noticed *anecdotally* is that this is not common enough
>>> to be too much of a problem, and local define bindings seem to be more
>>> favoured in Racket than other Schemes, that use 'let' more often.
>>>
>>>> I don't think this can be avoided as we rely on regexs rather than
>>>> parsing the source so it is probably best to only match toplevel
>>>> defines.
>>>
>>> The other issue with only matching top level defines is that a
>>> lot of scheme programs are library definitions, something like
>>>
>>> (library
>>>     (foo bar)
>>>   (export ...)
>>>   (define ...)
>>>   (define ...)
>>>   ;; and a bunch of other definitions...
>>> )
>>>
>>> Only matching top level defines will completely ignore matching all
>>> the definitions in these files.
>>
>> That said, I still stand by the fact that only catching top level defines
>> will lead to a lot of definitions being ignored. Maybe the occasional
>> mismatch may be worth the gain in the number of function contexts being
>> detected?
> 
> I'm not sure that the mismatches will be occasional - every time you
> have an internal definition in a function the hunk header will be wrong
> when you change the main body of the function. This will affect grep
> --function-context and diff -W as well as the normal hunk headers. The
> problem is there is no way to avoid that and provide something useful in
> the library example you have above. It would be useful to find some code
> bases and diff the output of 'git log --patch' with and without the
> leading whitespace match in the function pattern to see how often this
> is a problem (i.e. when the funcnames do not match see which one is
> correct).

--function-context is just one application of the function matcher. To
work properly with nested function definitions, it would have to
understand the nesting. But it does not; there is nothing that we can do
about it without a proper language parser. Therefore, the argument that
the matcher does not work well with --function-context for nested
functions is of little relevance.

IMO, the primary concern should be whether the matcher decorates hunk
contexts sufficiently well.

-- Hannes

^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSoC][PATCH v2 1/1] userdiff: add support for scheme
  2021-04-05 10:21     ` Phillip Wood
@ 2021-04-06 10:32       ` Atharva Raykar
  0 siblings, 0 replies; 35+ messages in thread
From: Atharva Raykar @ 2021-04-06 10:32 UTC (permalink / raw)
  To: Phillip Wood; +Cc: git

On 05-Apr-2021, at 15:51, Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
> Hi Atharva
> On 03/04/2021 14:16, Atharva Raykar wrote:
>> Add a diff driver for Scheme-like languages which recognizes top level
>> and local `define` forms, whether it is a function definition, binding,
>> syntax definition or a user-defined `define-xyzzy` form.
>> Also supports R6RS `library` forms, `module` forms along with class and
>> struct declarations used in Racket (PLT Scheme).
>> Alternate "def" syntax such as those in Gerbil Scheme are also
>> supported, like defstruct, defsyntax and so on.
>> The rationale for picking `define` forms for the hunk headers is because
>> it is usually the only significant form for defining the structure of
>> the program, and it is a common pattern for schemers to have local
>> function definitions to hide their visibility, so it is not only the top
>> level `define`'s that are of interest. Schemers also extend the language
>> with macros to provide their own define forms (for example, something
>> like a `define-test-suite`) which is also captured in the hunk header.
>> Since it is common practice to extend syntax with variants of a form
>> like `module+`, `class*` etc, those have been supported as well.
>> The word regex is a best-effort attempt to conform to R6RS[1] valid
>> identifiers, symbols and numbers.
>> [1] http://www.r6rs.org/final/html/r6rs/r6rs-Z-H-7.html#node_chap_4
>> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
>> [...]
>> diff --git a/userdiff.c b/userdiff.c
>> index 3f81a2261c..ac1999bbc5 100644
>> --- a/userdiff.c
>> +++ b/userdiff.c
>> @@ -191,6 +191,10 @@ PATTERNS("rust",
>>  	 "[a-zA-Z_][a-zA-Z0-9_]*"
>>  	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
>>  	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
>> +PATTERNS("scheme",
>> +	 "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",
>> +	 /* All words should be delimited by spaces or parentheses */
>> +	 "([^][)(}{[ \t])+"),
> 
> I think it would be nice to match single '(' and '[' to highlight when they have been added or deleted - I find this useful when I get a syntax error. Also it would be nice to handle r7rs identifiers like | this is a symbol |. Maybe something like
> "(\\|([^\\\\|]*(\\\\|)*)*\\||[^][}{)( \t]|[][(){}])"

My patch seems to detect additions and removals of singular parentheses
already -- I am not sure why it works, but my suspicion is that the
userdiff code seems to fall back to some default rules for additions and
removals that do not match the current word regex? Either way that seems
to work.

As for the R7RS identifiers, I can definitely add that, thanks for
pointing that out!

> Best Wishes
> 
> Phillip
> 
>>  PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
>>  	 "[={}\"]|[^={}\" \t]+"),
>>  PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-04-05 10:04           ` Phillip Wood
  2021-04-05 17:58             ` Johannes Sixt
@ 2021-04-06 12:29             ` Atharva Raykar
  2021-04-06 19:10               ` Phillip Wood
  1 sibling, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-04-06 12:29 UTC (permalink / raw)
  To: Phillip Wood; +Cc: Johannes Sixt, git, Junio C Hamano

On 05-Apr-2021, at 15:34, Phillip Wood <phillip.wood123@gmail.com> wrote:
> 
> Hi Atharva
> 
> On 30/03/2021 11:22, Atharva Raykar wrote:
>>> On 30-Mar-2021, at 12:34, Atharva Raykar <raykar.ath@gmail.com> wrote:
>>> 
>>> 
>>> 
>>>> On 29-Mar-2021, at 15:48, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>> 
>>>> Hi Atharva
>>>> 
>>>> On 28/03/2021 13:23, Atharva Raykar wrote:
>>>>> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
>>>>> [...]
>>>>>>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>>>>>>> new file mode 100644
>>>>>>> index 0000000000..90e75dcce8
>>>>>>> --- /dev/null
>>>>>>> +++ b/t/t4018/scheme-local-define
>>>>>>> @@ -0,0 +1,4 @@
>>>>>>> +(define (higher-order)
>>>>>>> +  (define local-function RIGHT
>>>>>> 
>>>>>> ... this one, which is also indented and *is* marked as RIGHT.
>>>>> In this test case, I was explicitly testing for an indented '(define'
>>>>> whereas in the former, I was testing for the top-level '(define-syntax',
>>>>> which happened to have an internal define (which will inevitably show up
>>>>> in a lot of scheme code).
>>>> 
>>>> It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
>>>> 
>>>> (define (f arg)
>>>> (define (g x)
>>>>   (+ 1 x))
>>>> 
>>>> (some-func ...)
>>>> ;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
>>> 
>>> The reason I went for this over the top level forms, is because
>>> I felt it was useful to see the nearest definition for internal
>>> functions that often have a lot of the actual business logic of
>>> the program (at least a lot of SICP seems to follow this pattern).
>>> The disadvantage is as you said, it might also catch trivial inner
>>> functions and the developer might lose context.
>> Never mind this message, I had misunderstood the problem you were trying to
>> demonstrate. I wholeheartedly agree with what you are trying to say, and
>> the indentation heuristic discussed does look interesting. I shall have a
>> glance at the RFC you linked in the other reply.
>>> The disadvantage is as you said, it might also catch trivial inner
>>> functions and the developer might lose context.
>> Feel free to disregard me misquoting you here. You did not say that (:
>>> Another problem is it may match more trivial bindings, like:
>>> 
>>> (define (some-func things)
>>>  ...
>>>  (define items '(eggs
>>>                  ham
>>>                  peanut-butter))
>>>  ...)
>>> 
>>> What I have noticed *anecdotally* is that this is not common enough
>>> to be too much of a problem, and local define bindings seem to be more
>>> favoured in Racket than other Schemes, that use 'let' more often.
>>> 
>>>> I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
>>> 
>>> The other issue with only matching top level defines is that a
>>> lot of scheme programs are library definitions, something like
>>> 
>>> (library
>>>    (foo bar)
>>>  (export ...)
>>>  (define ...)
>>>  (define ...)
>>>  ;; and a bunch of other definitions...
>>> )
>>> 
>>> Only matching top level defines will completely ignore matching all
>>> the definitions in these files.
>> That said, I still stand by the fact that only catching top level defines
>> will lead to a lot of definitions being ignored. Maybe the occasional
>> mismatch may be worth the gain in the number of function contexts being
>> detected?
> 
> I'm not sure that the mismatches will be occasional - every time you have an internal definition in a function the hunk header will be wrong when you change the main body of the function. This will affect grep --function-context and diff -W as well as the normal hunk headers. The problem is there is no way to avoid that and provide something useful in the library example you have above. It would be useful to find some code bases and diff the output of 'git log --patch' with and without the leading whitespace match in the function pattern to see how often this is a problem (i.e. when the funcnames do not match see which one is correct).

You are right -- on trying out the function on a two other scheme
codebases, I noticed that there are a lot more wrongly matched functions
than I initially thought. About half of them identify the wrong function
in one of the repositories I tried. However, removing the leading
whitespace in the pattern did not lead to better matching; it just led
to a lot of the hunk headers going blank. I am not sure what causes this
behaviour, but my guess is that the function contexts are shown only if
it is within a certain distance from the function definition?

Even if it did match only the top level defines correctly, the functions
matched would still often be technically wrong -- it will show the outer
function as the context when the user has edited an internal function
(and in Scheme, there is heavy usage of internal functions).

After running 'git grep --function-context' with the leading whitespace
removed, it seems to match too aggressively, as it captures a huge
region to match all the way upto the top level. Especially for files
where all the definitions are in a 'library'.

Overall, I personally felt that there were more downsides to matching
only at the top level. I'd rather the hunk header have the nearest
function to provide the context, than have no function displayed at all.
Even when the match is wrong, it at least helps me locate where the
change was made more easily.


> Best Wishes
> 
> Phillip
> 
> 


^ permalink raw reply	[flat|nested] 35+ messages in thread

* Re: [GSOC][PATCH] userdiff: add support for Scheme
  2021-04-06 12:29             ` Atharva Raykar
@ 2021-04-06 19:10               ` Phillip Wood
  0 siblings, 0 replies; 35+ messages in thread
From: Phillip Wood @ 2021-04-06 19:10 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: Johannes Sixt, git, Junio C Hamano

Hi Atharva

On 06/04/2021 13:29, Atharva Raykar wrote:
> On 05-Apr-2021, at 15:34, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>
>> Hi Atharva
>>
>> On 30/03/2021 11:22, Atharva Raykar wrote:
>>>> On 30-Mar-2021, at 12:34, Atharva Raykar <raykar.ath@gmail.com> wrote:
>>>>
>>>>
>>>>
>>>>> On 29-Mar-2021, at 15:48, Phillip Wood <phillip.wood123@gmail.com> wrote:
>>>>>
>>>>> Hi Atharva
>>>>>
>>>>> On 28/03/2021 13:23, Atharva Raykar wrote:
>>>>>> On 28-Mar-2021, at 05:16, Johannes Sixt <j6t@kdbg.org> wrote:
>>>>>> [...]
>>>>>>>> diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
>>>>>>>> new file mode 100644
>>>>>>>> index 0000000000..90e75dcce8
>>>>>>>> --- /dev/null
>>>>>>>> +++ b/t/t4018/scheme-local-define
>>>>>>>> @@ -0,0 +1,4 @@
>>>>>>>> +(define (higher-order)
>>>>>>>> +  (define local-function RIGHT
>>>>>>>
>>>>>>> ... this one, which is also indented and *is* marked as RIGHT.
>>>>>> In this test case, I was explicitly testing for an indented '(define'
>>>>>> whereas in the former, I was testing for the top-level '(define-syntax',
>>>>>> which happened to have an internal define (which will inevitably show up
>>>>>> in a lot of scheme code).
>>>>>
>>>>> It would be nice to include indented define forms but including them means that any change to the body of a function is attributed to the last internal definition rather than the actual function. For example
>>>>>
>>>>> (define (f arg)
>>>>> (define (g x)
>>>>>    (+ 1 x))
>>>>>
>>>>> (some-func ...)
>>>>> ;;any change here will have '(define (g x)' in the hunk header, not '(define (f arg)'
>>>>
>>>> The reason I went for this over the top level forms, is because
>>>> I felt it was useful to see the nearest definition for internal
>>>> functions that often have a lot of the actual business logic of
>>>> the program (at least a lot of SICP seems to follow this pattern).
>>>> The disadvantage is as you said, it might also catch trivial inner
>>>> functions and the developer might lose context.
>>> Never mind this message, I had misunderstood the problem you were trying to
>>> demonstrate. I wholeheartedly agree with what you are trying to say, and
>>> the indentation heuristic discussed does look interesting. I shall have a
>>> glance at the RFC you linked in the other reply.
>>>> The disadvantage is as you said, it might also catch trivial inner
>>>> functions and the developer might lose context.
>>> Feel free to disregard me misquoting you here. You did not say that (:
>>>> Another problem is it may match more trivial bindings, like:
>>>>
>>>> (define (some-func things)
>>>>   ...
>>>>   (define items '(eggs
>>>>                   ham
>>>>                   peanut-butter))
>>>>   ...)
>>>>
>>>> What I have noticed *anecdotally* is that this is not common enough
>>>> to be too much of a problem, and local define bindings seem to be more
>>>> favoured in Racket than other Schemes, that use 'let' more often.
>>>>
>>>>> I don't think this can be avoided as we rely on regexs rather than parsing the source so it is probably best to only match toplevel defines.
>>>>
>>>> The other issue with only matching top level defines is that a
>>>> lot of scheme programs are library definitions, something like
>>>>
>>>> (library
>>>>     (foo bar)
>>>>   (export ...)
>>>>   (define ...)
>>>>   (define ...)
>>>>   ;; and a bunch of other definitions...
>>>> )
>>>>
>>>> Only matching top level defines will completely ignore matching all
>>>> the definitions in these files.
>>> That said, I still stand by the fact that only catching top level defines
>>> will lead to a lot of definitions being ignored. Maybe the occasional
>>> mismatch may be worth the gain in the number of function contexts being
>>> detected?
>>
>> I'm not sure that the mismatches will be occasional - every time you have an internal definition in a function the hunk header will be wrong when you change the main body of the function. This will affect grep --function-context and diff -W as well as the normal hunk headers. The problem is there is no way to avoid that and provide something useful in the library example you have above. It would be useful to find some code bases and diff the output of 'git log --patch' with and without the leading whitespace match in the function pattern to see how often this is a problem (i.e. when the funcnames do not match see which one is correct).
> 
> You are right -- on trying out the function on a two other scheme
> codebases, I noticed that there are a lot more wrongly matched functions
> than I initially thought. About half of them identify the wrong function
> in one of the repositories I tried. However, removing the leading
> whitespace in the pattern did not lead to better matching; it just led
> to a lot of the hunk headers going blank. I am not sure what causes this
> behaviour, but my guess is that the function contexts are shown only if
> it is within a certain distance from the function definition?
> 
> Even if it did match only the top level defines correctly, the functions
> matched would still often be technically wrong -- it will show the outer
> function as the context when the user has edited an internal function
> (and in Scheme, there is heavy usage of internal functions).
> 
> After running 'git grep --function-context' with the leading whitespace
> removed, it seems to match too aggressively, as it captures a huge
> region to match all the way upto the top level. Especially for files
> where all the definitions are in a 'library'.
> 
> Overall, I personally felt that there were more downsides to matching
> only at the top level. I'd rather the hunk header have the nearest
> function to provide the context, than have no function displayed at all.
> Even when the match is wrong, it at least helps me locate where the
> change was made more easily.

Thanks for taking the time to check the differences between the two 
approaches, as there is no perfect solution I'm happy to go with the one 
that seemed to be best in your investigations

Best Wishes

Phillip

>> Best Wishes
>>
>> Phillip
>>
>>
> 

^ permalink raw reply	[flat|nested] 35+ messages in thread

* [GSoC][PATCH v3 0/1] userdiff: add support for scheme
  2021-04-03 13:16 ` [GSoC][PATCH v2 0/1] userdiff: add support for scheme Atharva Raykar
  2021-04-03 13:16   ` [GSoC][PATCH v2 1/1] " Atharva Raykar
@ 2021-04-08  9:14   ` Atharva Raykar
  2021-04-08  9:14   ` [GSoC][PATCH v3 1/1] userdiff: add support for Scheme Atharva Raykar
  2 siblings, 0 replies; 35+ messages in thread
From: Atharva Raykar @ 2021-04-08  9:14 UTC (permalink / raw)
  To: git; +Cc: Atharva Raykar, gitster, avarab, j6t, phillip.wood123

Changes since v2:

 - Add word regex to support for R7RS identifiers that allow for special
   characters between vertical lines, such as |foo bar baz|.
   - Change t4034 to include the above case.

Atharva Raykar (1):
  userdiff: add support for Scheme

 Documentation/gitattributes.txt    |  2 ++
 t/t4018-diff-funcname.sh           |  1 +
 t/t4018/scheme-class               |  7 +++++++
 t/t4018/scheme-def                 |  4 ++++
 t/t4018/scheme-def-variant         |  4 ++++
 t/t4018/scheme-define-slash-public |  7 +++++++
 t/t4018/scheme-define-syntax       |  8 ++++++++
 t/t4018/scheme-define-variant      |  4 ++++
 t/t4018/scheme-library             | 11 +++++++++++
 t/t4018/scheme-local-define        |  4 ++++
 t/t4018/scheme-module              |  6 ++++++
 t/t4018/scheme-top-level-define    |  4 ++++
 t/t4018/scheme-user-defined-define |  6 ++++++
 t/t4034-diff-words.sh              |  1 +
 t/t4034/scheme/expect              | 11 +++++++++++
 t/t4034/scheme/post                |  6 ++++++
 t/t4034/scheme/pre                 |  6 ++++++
 userdiff.c                         |  9 +++++++++
 18 files changed, 101 insertions(+)
 create mode 100644 t/t4018/scheme-class
 create mode 100644 t/t4018/scheme-def
 create mode 100644 t/t4018/scheme-def-variant
 create mode 100644 t/t4018/scheme-define-slash-public
 create mode 100644 t/t4018/scheme-define-syntax
 create mode 100644 t/t4018/scheme-define-variant
 create mode 100644 t/t4018/scheme-library
 create mode 100644 t/t4018/scheme-local-define
 create mode 100644 t/t4018/scheme-module
 create mode 100644 t/t4018/scheme-top-level-define
 create mode 100644 t/t4018/scheme-user-defined-define
 create mode 100644 t/t4034/scheme/expect
 create mode 100644 t/t4034/scheme/post
 create mode 100644 t/t4034/scheme/pre

-- 
2.31.1


^ permalink raw reply	[flat|nested] 35+ messages in thread

* [GSoC][PATCH v3 1/1] userdiff: add support for Scheme
  2021-04-03 13:16 ` [GSoC][PATCH v2 0/1] userdiff: add support for scheme Atharva Raykar
  2021-04-03 13:16   ` [GSoC][PATCH v2 1/1] " Atharva Raykar
  2021-04-08  9:14   ` [GSoC][PATCH v3 0/1] " Atharva Raykar
@ 2021-04-08  9:14   ` Atharva Raykar
  2021-04-12 23:04     ` Junio C Hamano
  2 siblings, 1 reply; 35+ messages in thread
From: Atharva Raykar @ 2021-04-08  9:14 UTC (permalink / raw)
  To: git; +Cc: Atharva Raykar

Add a diff driver for Scheme-like languages which recognizes top level
and local `define` forms, whether it is a function definition, binding,
syntax definition or a user-defined `define-xyzzy` form.

Also supports R6RS `library` forms, `module` forms along with class and
struct declarations used in Racket (PLT Scheme).

Alternate "def" syntax such as those in Gerbil Scheme are also
supported, like defstruct, defsyntax and so on.

The rationale for picking `define` forms for the hunk headers is because
it is usually the only significant form for defining the structure of
the program, and it is a common pattern for schemers to have local
function definitions to hide their visibility, so it is not only the top
level `define`'s that are of interest. Schemers also extend the language
with macros to provide their own define forms (for example, something
like a `define-test-suite`) which is also captured in the hunk header.

Since it is common practice to extend syntax with variants of a form
like `module+`, `class*` etc, those have been supported as well.

The word regex is a best-effort attempt to conform to R7RS[1] valid
identifiers, symbols and numbers.

[1] https://small.r7rs.org/attachment/r7rs.pdf (section 2.1)

Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
---
 Documentation/gitattributes.txt    |  2 ++
 t/t4018-diff-funcname.sh           |  1 +
 t/t4018/scheme-class               |  7 +++++++
 t/t4018/scheme-def                 |  4 ++++
 t/t4018/scheme-def-variant         |  4 ++++
 t/t4018/scheme-define-slash-public |  7 +++++++
 t/t4018/scheme-define-syntax       |  8 ++++++++
 t/t4018/scheme-define-variant      |  4 ++++
 t/t4018/scheme-library             | 11 +++++++++++
 t/t4018/scheme-local-define        |  4 ++++
 t/t4018/scheme-module              |  6 ++++++
 t/t4018/scheme-top-level-define    |  4 ++++
 t/t4018/scheme-user-defined-define |  6 ++++++
 t/t4034-diff-words.sh              |  1 +
 t/t4034/scheme/expect              | 11 +++++++++++
 t/t4034/scheme/post                |  6 ++++++
 t/t4034/scheme/pre                 |  6 ++++++
 userdiff.c                         |  9 +++++++++
 18 files changed, 101 insertions(+)
 create mode 100644 t/t4018/scheme-class
 create mode 100644 t/t4018/scheme-def
 create mode 100644 t/t4018/scheme-def-variant
 create mode 100644 t/t4018/scheme-define-slash-public
 create mode 100644 t/t4018/scheme-define-syntax
 create mode 100644 t/t4018/scheme-define-variant
 create mode 100644 t/t4018/scheme-library
 create mode 100644 t/t4018/scheme-local-define
 create mode 100644 t/t4018/scheme-module
 create mode 100644 t/t4018/scheme-top-level-define
 create mode 100644 t/t4018/scheme-user-defined-define
 create mode 100644 t/t4034/scheme/expect
 create mode 100644 t/t4034/scheme/post
 create mode 100644 t/t4034/scheme/pre

diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index 0a60472bb5..cfcfa800c2 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -845,6 +845,8 @@ patterns are available:
 
 - `rust` suitable for source code in the Rust language.
 
+- `scheme` suitable for source code in the Scheme language.
+
 - `tex` suitable for source code for LaTeX documents.
 
 
diff --git a/t/t4018-diff-funcname.sh b/t/t4018-diff-funcname.sh
index 9675bc17db..823ea96acb 100755
--- a/t/t4018-diff-funcname.sh
+++ b/t/t4018-diff-funcname.sh
@@ -48,6 +48,7 @@ diffpatterns="
 	python
 	ruby
 	rust
+	scheme
 	tex
 	custom1
 	custom2
diff --git a/t/t4018/scheme-class b/t/t4018/scheme-class
new file mode 100644
index 0000000000..e5e07b43fb
--- /dev/null
+++ b/t/t4018/scheme-class
@@ -0,0 +1,7 @@
+(define book-class%
+  (class* () object% RIGHT
+    (field (pages 5))
+    (field (ChangeMe 5))
+    (define/public (letters)
+      (* pages 500))
+    (super-new)))
diff --git a/t/t4018/scheme-def b/t/t4018/scheme-def
new file mode 100644
index 0000000000..1e2673da96
--- /dev/null
+++ b/t/t4018/scheme-def
@@ -0,0 +1,4 @@
+(def (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
diff --git a/t/t4018/scheme-def-variant b/t/t4018/scheme-def-variant
new file mode 100644
index 0000000000..d857a61d64
--- /dev/null
+++ b/t/t4018/scheme-def-variant
@@ -0,0 +1,4 @@
+(defmethod {print point} RIGHT
+  (lambda (self)
+    (with ((point x y) self)
+      (printf "{ChangeMe x:~a y:~a}~n" x y))))
diff --git a/t/t4018/scheme-define-slash-public b/t/t4018/scheme-define-slash-public
new file mode 100644
index 0000000000..39a93a1600
--- /dev/null
+++ b/t/t4018/scheme-define-slash-public
@@ -0,0 +1,7 @@
+(define bar-class%
+  (class object%
+    (field (info 5))
+    (define/public (foo) RIGHT
+      (+ info 42)
+      (* info ChangeMe))
+    (super-new)))
diff --git a/t/t4018/scheme-define-syntax b/t/t4018/scheme-define-syntax
new file mode 100644
index 0000000000..7d5e99e0fc
--- /dev/null
+++ b/t/t4018/scheme-define-syntax
@@ -0,0 +1,8 @@
+(define-syntax define-test-suite RIGHT
+  (syntax-rules ()
+    ((_ suite-name (name test) ChangeMe ...)
+     (define suite-name
+       (let ((tests
+              `((name . ,test) ...)))
+         (lambda ()
+           (run-suite 'suite-name tests)))))))
diff --git a/t/t4018/scheme-define-variant b/t/t4018/scheme-define-variant
new file mode 100644
index 0000000000..911708854d
--- /dev/null
+++ b/t/t4018/scheme-define-variant
@@ -0,0 +1,4 @@
+(define* (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
diff --git a/t/t4018/scheme-library b/t/t4018/scheme-library
new file mode 100644
index 0000000000..82ea3df510
--- /dev/null
+++ b/t/t4018/scheme-library
@@ -0,0 +1,11 @@
+(library (my-helpers id-stuff) RIGHT
+  (export find-dup)
+  (import (ChangeMe))
+  (define (find-dup l)
+    (and (pair? l)
+         (let loop ((rest (cdr l)))
+           (cond
+            [(null? rest) (find-dup (cdr l))]
+            [(bound-identifier=? (car l) (car rest))
+             (car rest)]
+            [else (loop (cdr rest))])))))
diff --git a/t/t4018/scheme-local-define b/t/t4018/scheme-local-define
new file mode 100644
index 0000000000..bc6d8aebbe
--- /dev/null
+++ b/t/t4018/scheme-local-define
@@ -0,0 +1,4 @@
+(define (higher-order)
+  (define local-function RIGHT
+    (lambda (x)
+     (car "this is" "ChangeMe"))))
diff --git a/t/t4018/scheme-module b/t/t4018/scheme-module
new file mode 100644
index 0000000000..edfae0ebf7
--- /dev/null
+++ b/t/t4018/scheme-module
@@ -0,0 +1,6 @@
+(module A RIGHT
+  (export with-display-exception)
+  (extern (display-exception display-exception ChangeMe))
+  (def (with-display-exception thunk)
+    (with-catch (lambda (e) (display-exception e (current-error-port)) e)
+      thunk)))
diff --git a/t/t4018/scheme-top-level-define b/t/t4018/scheme-top-level-define
new file mode 100644
index 0000000000..624743c22b
--- /dev/null
+++ b/t/t4018/scheme-top-level-define
@@ -0,0 +1,4 @@
+(define (some-func x y z) RIGHT
+  (let ((a x)
+        (b y))
+        (ChangeMe a b)))
diff --git a/t/t4018/scheme-user-defined-define b/t/t4018/scheme-user-defined-define
new file mode 100644
index 0000000000..35fe7cc9bf
--- /dev/null
+++ b/t/t4018/scheme-user-defined-define
@@ -0,0 +1,6 @@
+(define-test-suite record\ case-tests RIGHT
+  (record-case-1 (lambda (fail)
+                   (let ((a (make-foo 1 2)))
+                     (record-case a
+                       ((bar x) (ChangeMe))
+                       ((foo a b) (+ a b)))))))
diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 56f1e62a97..ee7721ab91 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -325,6 +325,7 @@ test_language_driver perl
 test_language_driver php
 test_language_driver python
 test_language_driver ruby
+test_language_driver scheme
 test_language_driver tex
 
 test_expect_success 'word-diff with diff.sbe' '
diff --git a/t/t4034/scheme/expect b/t/t4034/scheme/expect
new file mode 100644
index 0000000000..496cd5de8c
--- /dev/null
+++ b/t/t4034/scheme/expect
@@ -0,0 +1,11 @@
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 74b6605..63b6ac4 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
+<CYAN>@@ -1,6 +1,6 @@<RESET>
+(define (<RED>myfunc a b<RESET><GREEN>my-func first second<RESET>)
+  ; This is a <RED>really<RESET><GREEN>(moderately)<RESET> cool function.
+  (<RED>this\place<RESET><GREEN>that\place<RESET> (+ 3 4))
+  (define <RED>some-text<RESET><GREEN>|a greeting|<RESET> "hello")
+  (let ((c (<RED>+ a b<RESET><GREEN>add1 first<RESET>)))
+    (format "one more than the total is %d" (<RED>add1<RESET><GREEN>+<RESET> c <GREEN>second<RESET>))))
diff --git a/t/t4034/scheme/post b/t/t4034/scheme/post
new file mode 100644
index 0000000000..63b6ac4f87
--- /dev/null
+++ b/t/t4034/scheme/post
@@ -0,0 +1,6 @@
+(define (my-func first second)
+  ; This is a (moderately) cool function.
+  (that\place (+ 3 4))
+  (define |a greeting| "hello")
+  (let ((c (add1 first)))
+    (format "one more than the total is %d" (+ c second))))
diff --git a/t/t4034/scheme/pre b/t/t4034/scheme/pre
new file mode 100644
index 0000000000..74b6605357
--- /dev/null
+++ b/t/t4034/scheme/pre
@@ -0,0 +1,6 @@
+(define (myfunc a b)
+  ; This is a really cool function.
+  (this\place (+ 3 4))
+  (define some-text "hello")
+  (let ((c (+ a b)))
+    (format "one more than the total is %d" (add1 c))))
diff --git a/userdiff.c b/userdiff.c
index 3f81a2261c..3897317aff 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -191,6 +191,15 @@ PATTERNS("rust",
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
 	 "|[-+*\\/<>%&^|=!:]=|<<=?|>>=?|&&|\\|\\||->|=>|\\.{2}=|\\.{3}|::"),
+PATTERNS("scheme",
+	 "^[\t ]*(\\(((define|def(struct|syntax|class|method|rules|record|proto|alias)?)[-*/ \t]|(library|module|struct|class)[*+ \t]).*)$",
+	 /*
+	  * R7RS valid identifiers include any sequence enclosed
+	  * within vertical lines having no backslashes
+	  */
+	 "\\|([^\\\\]*)\\|"
+	 /* All other words should be delimited by spaces or parentheses */
+	 "|([^][)(}{[ \t])+"),
 PATTERNS("bibtex", "(@[a-zA-Z]{1,}[ \t]*\\{{0,1}[ \t]*[^ \t\"@',\\#}{~%]*).*$",
 	 "[={}\"]|[^={}\" \t]+"),
 PATTERNS("tex", "^(\\\\((sub)*section|chapter|part)\\*{0,1}\\{.*)$",
-- 
2.31.1


^ permalink raw reply related	[flat|nested] 35+ messages in thread

* Re: [GSoC][PATCH v3 1/1] userdiff: add support for Scheme
  2021-04-08  9:14   ` [GSoC][PATCH v3 1/1] userdiff: add support for Scheme Atharva Raykar
@ 2021-04-12 23:04     ` Junio C Hamano
  0 siblings, 0 replies; 35+ messages in thread
From: Junio C Hamano @ 2021-04-12 23:04 UTC (permalink / raw)
  To: Atharva Raykar; +Cc: git

Atharva Raykar <raykar.ath@gmail.com> writes:

> Add a diff driver for Scheme-like languages which recognizes top level
> and local `define` forms, whether it is a function definition, binding,
> syntax definition or a user-defined `define-xyzzy` form.
>
> Also supports R6RS `library` forms, `module` forms along with class and
> struct declarations used in Racket (PLT Scheme).
>
> Alternate "def" syntax such as those in Gerbil Scheme are also
> supported, like defstruct, defsyntax and so on.
>
> The rationale for picking `define` forms for the hunk headers is because
> it is usually the only significant form for defining the structure of
> the program, and it is a common pattern for schemers to have local
> function definitions to hide their visibility, so it is not only the top
> level `define`'s that are of interest. Schemers also extend the language
> with macros to provide their own define forms (for example, something
> like a `define-test-suite`) which is also captured in the hunk header.
>
> Since it is common practice to extend syntax with variants of a form
> like `module+`, `class*` etc, those have been supported as well.
>
> The word regex is a best-effort attempt to conform to R7RS[1] valid
> identifiers, symbols and numbers.
>
> [1] https://small.r7rs.org/attachment/r7rs.pdf (section 2.1)
>
> Signed-off-by: Atharva Raykar <raykar.ath@gmail.com>
> ---
>  Documentation/gitattributes.txt    |  2 ++
>  t/t4018-diff-funcname.sh           |  1 +
>  t/t4018/scheme-class               |  7 +++++++
>  t/t4018/scheme-def                 |  4 ++++
>  t/t4018/scheme-def-variant         |  4 ++++
>  t/t4018/scheme-define-slash-public |  7 +++++++
>  t/t4018/scheme-define-syntax       |  8 ++++++++
>  t/t4018/scheme-define-variant      |  4 ++++
>  t/t4018/scheme-library             | 11 +++++++++++
>  t/t4018/scheme-local-define        |  4 ++++
>  t/t4018/scheme-module              |  6 ++++++
>  t/t4018/scheme-top-level-define    |  4 ++++
>  t/t4018/scheme-user-defined-define |  6 ++++++
>  t/t4034-diff-words.sh              |  1 +
>  t/t4034/scheme/expect              | 11 +++++++++++
>  t/t4034/scheme/post                |  6 ++++++
>  t/t4034/scheme/pre                 |  6 ++++++
>  userdiff.c                         |  9 +++++++++
>  18 files changed, 101 insertions(+)
>  create mode 100644 t/t4018/scheme-class
>  create mode 100644 t/t4018/scheme-def
>  create mode 100644 t/t4018/scheme-def-variant
>  create mode 100644 t/t4018/scheme-define-slash-public
>  create mode 100644 t/t4018/scheme-define-syntax
>  create mode 100644 t/t4018/scheme-define-variant
>  create mode 100644 t/t4018/scheme-library
>  create mode 100644 t/t4018/scheme-local-define
>  create mode 100644 t/t4018/scheme-module
>  create mode 100644 t/t4018/scheme-top-level-define
>  create mode 100644 t/t4018/scheme-user-defined-define
>  create mode 100644 t/t4034/scheme/expect
>  create mode 100644 t/t4034/scheme/post
>  create mode 100644 t/t4034/scheme/pre

We have seen reviews on previous rounds, and haven't heard anything
on this round yet.

If I do not hear from anybody in a few days, let's merge it to
'next'.

^ permalink raw reply	[flat|nested] 35+ messages in thread

end of thread, other threads:[~2021-04-12 23:04 UTC | newest]

Thread overview: 35+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-03-27 17:39 [GSOC][PATCH] userdiff: add support for Scheme Atharva Raykar
2021-03-27 22:50 ` Junio C Hamano
2021-03-27 23:09   ` Junio C Hamano
2021-03-28  3:16     ` Ævar Arnfjörð Bjarmason
2021-03-28  5:37       ` Junio C Hamano
2021-03-28 12:40       ` Atharva Raykar
2021-03-29 10:08         ` Phillip Wood
2021-03-30  6:41           ` Atharva Raykar
2021-03-30 12:56             ` Ævar Arnfjörð Bjarmason
2021-03-30 13:48               ` Atharva Raykar
2021-03-28 12:45     ` Atharva Raykar
2021-03-28 11:51   ` Atharva Raykar
2021-03-28 18:06     ` Junio C Hamano
2021-03-29  8:12       ` Atharva Raykar
2021-03-29 20:47         ` Junio C Hamano
2021-03-29 10:12     ` Phillip Wood
2021-03-27 23:46 ` Johannes Sixt
2021-03-28 12:23   ` Atharva Raykar
2021-03-29 10:18     ` Phillip Wood
2021-03-29 10:48       ` Johannes Sixt
2021-03-29 13:12         ` Ævar Arnfjörð Bjarmason
2021-03-29 14:06           ` Phillip Wood
2021-03-30  7:04       ` Atharva Raykar
2021-03-30 10:22         ` Atharva Raykar
2021-04-05 10:04           ` Phillip Wood
2021-04-05 17:58             ` Johannes Sixt
2021-04-06 12:29             ` Atharva Raykar
2021-04-06 19:10               ` Phillip Wood
2021-04-03 13:16 ` [GSoC][PATCH v2 0/1] userdiff: add support for scheme Atharva Raykar
2021-04-03 13:16   ` [GSoC][PATCH v2 1/1] " Atharva Raykar
2021-04-05 10:21     ` Phillip Wood
2021-04-06 10:32       ` Atharva Raykar
2021-04-08  9:14   ` [GSoC][PATCH v3 0/1] " Atharva Raykar
2021-04-08  9:14   ` [GSoC][PATCH v3 1/1] userdiff: add support for Scheme Atharva Raykar
2021-04-12 23:04     ` Junio C Hamano

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).