Re: [PATCH] userdiff: support Rust macros
- From
Johannes Sixt <j6t@kdbg.org>
- Date
- Oct 6, 2020, 18:44 UTC
- Message-ID
- <42808d58-6472-5959-a471-3d09d06d8fd3@kdbg.org>
- In-Reply-To
- <pull.865.git.git.1601986430388.gitgitgadget@gmail.com>
Am 06.10.20 um 14:13 schrieb Konrad Borowski via GitGitGadget:
> From: Konrad Borowski <konrad@borowski.pw> > > 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
Show 39 quoted lines
>
> Signed-off-by: Konrad Borowski <konrad@borowski.pw>
> ---
> 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
>
> diff --git a/t/t4018/rust-macro-rules b/t/t4018/rust-macro-rules
> new file mode 100644
> index 0000000000..ec610c5b62
> --- /dev/null
> +++ b/t/t4018/rust-macro-rules
> @@ -0,0 +1,6 @@
> +macro_rules! RIGHT {
> + () => {
> + // a comment
> + let x = ChangeMe;
> + };
> +}
> diff --git a/userdiff.c b/userdiff.c
> index fde02f225b..f13a913697 100644
> --- a/userdiff.c
> +++ b/userdiff.c
> @@ -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 <j6t@kdbg.org>
-- Hannes