userdiff: support Rust macros

PATCH In review Konrad Borowski via GitGitGadget 4 participants 6 messages Oct 6, 2020 – Oct 7, 2020

lore

Share & tools

lore

Konrad Borowski via GitGitGadget

author

2020-10-06 12:13 UTC #0 lore raw

PATCH 1/1 userdiff: support Rust macros

From: Konrad Borowski <[email protected]>

This adds a support for macro_rules! keyword which declares a macro. It also includes a test case.

Signed-off-by Konrad Borowski <[email protected]>

---
userdiff: support Rust macros

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-865%2Fxfix%2Fuserdiff-macro-rules-rust-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-865/xfix/userdiff-macro-rules-rust-v1
Pull-Request: https://github.com/git/git/pull/865

 t/t4018/rust-macro-rules | 6 ++++++
 userdiff.c               | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 t/t4018/rust-macro-rules

t/t4018/rust-macro-rules new file +6 -0

new file mode 100644
index 0000000000..ec610c5b62
@@ -0,0 +1,6 @@
+macro_rules! RIGHT {
+    () => {
+        // a comment
+        let x = ChangeMe;
+    };
+}

userdiff.c +1 -1

index fde02f225b..f13a913697 100644
@@ -165,7 +165,7 @@ PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
 	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\?(\\\\C-)?(\\\\M-)?."
 	 "|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
 PATTERNS("rust",
-	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
+	 "^[\t ]*(((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+|macro_rules[\t ]*!)[^;]*)$",
 	 /* -- */
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"

base-commit: d98273ba77e1ab9ec755576bc86c716a97bf59d7

Signature · 1 line

gitgitgadget

Phillip Wood

2020-10-06 16:44 UTC #1 lore raw

Hi Konrad

Thanks for the patch, this would be a useful addition

Quoted 35 lines · Konrad Borowski via GitGitGadget

From: Konrad Borowski <[email protected]>
This adds a support for macro_rules! keyword which declares a macro. It also includes a test case.
Signed-off-by Konrad Borowski <[email protected]>
---
userdiff: support Rust macros
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-865%2Fxfix%2Fuserdiff-macro-rules-rust-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-865/xfix/userdiff-macro-rules-rust-v1
Pull-Request: https://github.com/git/git/pull/865
  t/t4018/rust-macro-rules | 6 ++++++
  userdiff.c               | 2 +-
  2 files changed, 7 insertions(+), 1 deletion(-)
create mode 100644 t/t4018/rust-macro-rules
t/t4018/rust-macro-rules new file +6 -0
new file mode 100644
index 0000000000..ec610c5b62
@@ -0,0 +1,6 @@
+macro_rules! RIGHT {
+    () => {
+        // a comment
+        let x = ChangeMe;
+    };
+}
userdiff.c +0 -0
index fde02f225b..f13a913697 100644

userdiff.c PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$", · line 168

Quoted 5 lines

userdiff.c +1 -1

@@ -165,7 +165,7 @@ PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
  	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\?(\\\\C-)?(\\\\M-)?."
  	 "|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
  PATTERNS("rust",
-	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
+	 "^[\t ]*(((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+|macro_rules[\t ]*!)

I think you want the ! before the [\t ]. We could probably get away with just adding macro-rules! as another alternative in the group starting with struct

Best Wishes

Phillip

[^;]*)$",

Quoted 6 lines

userdiff.c +0 -0
  	 /* -- */
  	 "[a-zA-Z_][a-zA-Z0-9_]*"
  	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"
base-commit: d98273ba77e1ab9ec755576bc86c716a97bf59d7

Johannes Sixt

Reviewed-by on v1 1/1

2020-10-06 18:44 UTC #2 lore raw

In reply to Konrad Borowski via GitGitGadget

Am 06.10.20 um 14:13 schrieb Konrad Borowski via GitGitGadget:

Quoted 4 lines

From: Konrad Borowski <[email protected]>
This adds a support for macro_rules! keyword which declares a macro. It also includes a test case.

We would write this sentence in imperative mood, i.e., give the codebase the order to become so. That a test case is included can be seen from the patch text and need not be mentioned.

In this case, if I were the only one to judge, I would accept this commit without a message if the commit summary were:

userdiff: permit Rust's macro_rules! keyword in hunk headers

Quoted 31 lines

Signed-off-by Konrad Borowski <[email protected]>
---
userdiff: support Rust macros
Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-865%2Fxfix%2Fuserdiff-macro-rules-rust-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-865/xfix/userdiff-macro-rules-rust-v1
Pull-Request: https://github.com/git/git/pull/865
 t/t4018/rust-macro-rules | 6 ++++++
 userdiff.c               | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 t/t4018/rust-macro-rules
t/t4018/rust-macro-rules new file +6 -0
new file mode 100644
index 0000000000..ec610c5b62
@@ -0,0 +1,6 @@
+macro_rules! RIGHT {
+    () => {
+        // a comment
+        let x = ChangeMe;
+    };
+}
userdiff.c +0 -0
index fde02f225b..f13a913697 100644

userdiff.c PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$", · line 171

Quoted 8 lines

userdiff.c +1 -1

@@ -165,7 +165,7 @@ PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
 	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\?(\\\\C-)?(\\\\M-)?."
 	 "|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
 PATTERNS("rust",
-	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
+	 "^[\t ]*(((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+|macro_rules[\t ]*!)[^;]*)$",
 	 /* -- */
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"

Technically, this looks fine. Originally I thought that the new keyword could just be thrown into the mix of struct|enum etc, but judging from the (existing) test cases, this new case looks fairly different (I am not a Rust expert, so I cannot really tell). Therefore, it is OK to put it in an alternative branch in the regexp.

Reviewed-by Johannes Sixt <[email protected]>

-- Hannes

Konrad Borowski via GitGitGadget

author

2020-10-07 05:27 UTC #3 lore raw

PATCH v2 1/1 userdiff: permit Rust's macro_rules! keyword in hunk headers

In reply to Konrad Borowski via GitGitGadget

From: Konrad Borowski <[email protected]>

Changes since v1:
- Changed macro_rules! to be considered to use the same rule as rest of keywords to reduce the size of a change as suggested by Phillip Wood. This means that 'pub macro_rules!` (a syntax error) is considered to be a hunk header.

- Written commit message in imperative mood as suggested by Johannes Sixt.

Signed-off-by Konrad Borowski <[email protected]>

---
userdiff: support Rust macros

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-865%2Fxfix%2Fuserdiff-macro-rules-rust-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-865/xfix/userdiff-macro-rules-rust-v2
Pull-Request: https://github.com/git/git/pull/865

Range-diff vs v1:

1: 9e071ddf63 ! 1: a3fd67d84f userdiff: support Rust macros
@@ Metadata
Author: Konrad Borowski <[email protected]>

## Commit message ##
- userdiff: support Rust macros
+ userdiff: permit Rust's macro_rules! keyword in hunk headers

- This adds a support for macro_rules! keyword which declares
- a macro. It also includes a test case.
+ Changes since v1:
+ - Changed macro_rules! to be considered to use the same
+ rule as rest of keywords to reduce the size of a change
+ as suggested by Phillip Wood. This means that
+ 'pub macro_rules!` (a syntax error) is considered to be
+ a hunk header.
+
+ - Written commit message in imperative mood as suggested by
+ Johannes Sixt.

Signed-off-by Konrad Borowski <[email protected]>

@@ userdiff.c: PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
"|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
PATTERNS("rust",
- "^[\t ]*((pub(\$[^\$]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
-+ "^[\t ]*(((pub(\$[^\$]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+|macro_rules[\t ]*!)[^;]*)$",
++ "^[\t ]*((pub(\$[^\$]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl|macro_rules[\t ]*!)[< \t]+[^;]*)$",
/* -- */
"[a-zA-Z_][a-zA-Z0-9_]*" "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"

 t/t4018/rust-macro-rules | 6 ++++++
 userdiff.c               | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 t/t4018/rust-macro-rules

t/t4018/rust-macro-rules new file +6 -0

new file mode 100644
index 0000000000..ec610c5b62
@@ -0,0 +1,6 @@
+macro_rules! RIGHT {
+    () => {
+        // a comment
+        let x = ChangeMe;
+    };
+}

userdiff.c +1 -1

index fde02f225b..c0f63c57b2 100644
@@ -165,7 +165,7 @@ PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
 	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\?(\\\\C-)?(\\\\M-)?."
 	 "|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
 PATTERNS("rust",
-	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
+	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl|macro_rules[\t ]*!)[< \t]+[^;]*)$",
 	 /* -- */
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"

base-commit: d98273ba77e1ab9ec755576bc86c716a97bf59d7

Signature · 1 line

gitgitgadget

Junio C Hamano

2020-10-07 06:10 UTC #4 lore raw

Re: [PATCH v2] userdiff: permit Rust's macro_rules! keyword in hunk headers

Quoted 11 lines · Konrad Borowski via GitGitGadget

From: Konrad Borowski <[email protected]>
Changes since v1:
- Changed macro_rules! to be considered to use the same rule as rest of keywords to reduce the size of a change as suggested by Phillip Wood. This means that 'pub macro_rules!` (a syntax error) is considered to be a hunk header.
- Written commit message in imperative mood as suggested by Johannes Sixt.

The above is where you write the proposed log message, not where you write the differences between v1 and v2 (which, if exists, would be below the three-dash line after your sign off). After all, people who are reading the final history of the project in "git log" will not even know or care that what they are reading is the second iteration and/or there was a first iteration that was different from the final version.

Quoted 1 line

Subject: [PATCH v2] userdiff: permit Rust's macro_rules! keyword in hunk headers

Somehow I find the "permit" a bit odd---it is not like we are disallowing Rust programs written in certain way.

userdiff: recognize 'macro_rules!' as starting a Rust function block

or something?

Quoted 3 lines

Signed-off-by Konrad Borowski <[email protected]>
---
...

As to the simplified rule that matches 'pub macro_rules!'.

We assume what is written by the end-user, in which these patterns are used to find function headers, is a syntactically correct program. This allows us to write a pattern that may match with a syntactic nonsense (i.e. false positive is perfectly fine). What we care about is to avoid false negatives---if our pattern does not recognize what users would consider a valid and common function header line, that is a problem.

So, from that point of view, you do not have to worry too much about 'pub macro_rules!' matching your pattern.

Quoted 17 lines

t/t4018/rust-macro-rules new file +6 -0

new file mode 100644
index 0000000000..ec610c5b62
@@ -0,0 +1,6 @@
+macro_rules! RIGHT {
+    () => {
+        // a comment
+        let x = ChangeMe;
+    };
+}

userdiff.c +0 -0

index fde02f225b..c0f63c57b2 100644

userdiff.c PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$", · line 168

Quoted 5 lines

userdiff.c +1 -1

@@ -165,7 +165,7 @@ PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
 	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\?(\\\\C-)?(\\\\M-)?."
 	 "|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
 PATTERNS("rust",
-	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
+	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl|macro_rules[\t ]*!)[< \t]+[^;]*)$",

I notice that you still allow "macro_rules !" as Phillip pointed
out. Is that intended?

Thanks.

Konrad Borowski via GitGitGadget

author

2020-10-07 13:26 UTC #5 lore raw

PATCH v3 1/1 userdiff: recognize 'macro_rules!' as starting a Rust function block

In reply to Konrad Borowski via GitGitGadget

From: Konrad Borowski <[email protected]>

Signed-off-by Konrad Borowski <[email protected]>

---
userdiff: support Rust macros

Changes since v1:

* Changed macro_rules! to be considered to use the same rule as rest of keywords to reduce the size of a change as suggested by Phillip Wood. This means that 'pub macro_rules!` (a syntax error) is considered to be a hunk header.
* Written commit message in imperative mood as suggested by Johannes Sixt.

Changes since v2:

* Updated the commit message as suggested by Junio C Hamano.
* Removed handling for 'macro_rules !' with a space in-between tokens. While it is allowed by the compiler to have a space between 'macro_rules' and '!' it's pretty much never done.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-865%2Fxfix%2Fuserdiff-macro-rules-rust-v3
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-865/xfix/userdiff-macro-rules-rust-v3
Pull-Request: https://github.com/git/git/pull/865

Range-diff vs v2:

1: a3fd67d84f ! 1: 0d8e6e06d4 userdiff: permit Rust's macro_rules! keyword in hunk headers
@@ Metadata
Author: Konrad Borowski <[email protected]>

## Commit message ##
- userdiff: permit Rust's macro_rules! keyword in hunk headers
-
- Changes since v1:
- - Changed macro_rules! to be considered to use the same
- rule as rest of keywords to reduce the size of a change
- as suggested by Phillip Wood. This means that
- 'pub macro_rules!` (a syntax error) is considered to be
- a hunk header.
-
- - Written commit message in imperative mood as suggested by
- Johannes Sixt.
+ userdiff: recognize 'macro_rules!' as starting a Rust function block

Signed-off-by Konrad Borowski <[email protected]>

@@ userdiff.c: PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
"|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
PATTERNS("rust",
- "^[\t ]*((pub(\$[^\$]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
-+ "^[\t ]*((pub(\$[^\$]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl|macro_rules[\t ]*!)[< \t]+[^;]*)$",
++ "^[\t ]*((pub(\$[^\$]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl|macro_rules!)[< \t]+[^;]*)$",
/* -- */
"[a-zA-Z_][a-zA-Z0-9_]*" "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"

 t/t4018/rust-macro-rules | 6 ++++++
 userdiff.c               | 2 +-
 2 files changed, 7 insertions(+), 1 deletion(-)
 create mode 100644 t/t4018/rust-macro-rules

t/t4018/rust-macro-rules new file +6 -0

new file mode 100644
index 0000000000..ec610c5b62
@@ -0,0 +1,6 @@
+macro_rules! RIGHT {
+    () => {
+        // a comment
+        let x = ChangeMe;
+    };
+}

userdiff.c +1 -1

index fde02f225b..2903037724 100644
@@ -165,7 +165,7 @@ PATTERNS("ruby", "^[ \t]*((class|module|def)[ \t].*)$",
 	 "|[-+0-9.e]+|0[xXbB]?[0-9a-fA-F]+|\\?(\\\\C-)?(\\\\M-)?."
 	 "|//=?|[-+*/<>%&^|=!]=|<<=?|>>=?|===|\\.{1,3}|::|[!=]~"),
 PATTERNS("rust",
-	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl)[< \t]+[^;]*)$",
+	 "^[\t ]*((pub(\\([^\\)]+\\))?[\t ]+)?((async|const|unsafe|extern([\t ]+\"[^\"]+\"))[\t ]+)?(struct|enum|union|mod|trait|fn|impl|macro_rules!)[< \t]+[^;]*)$",
 	 /* -- */
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[0-9][0-9_a-fA-Fiosuxz]*(\\.([0-9]*[eE][+-]?)?[0-9_fF]*)?"

base-commit: d98273ba77e1ab9ec755576bc86c716a97bf59d7

Signature · 1 line

gitgitgadget