[GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Kapil Jain]

Signed-off-by: Kapil Jain <jkapil.cs@gmail.com>
---

The test written does not pass, imo there's some problem with the regex part.
please let me know about the fault.

 t/t4034-diff-words.sh | 2 ++
 t/t4034/shell/expect  | 6 ++++++
 t/t4034/shell/post    | 1 +
 t/t4034/shell/pre     | 1 +
 userdiff.c            | 7 +++++++
 5 files changed, 17 insertions(+)
 create mode 100644 t/t4034/shell/expect
 create mode 100644 t/t4034/shell/post
 create mode 100644 t/t4034/shell/pre

diff --git a/t/t4034-diff-words.sh b/t/t4034-diff-words.sh
index 912df91226..74366e6826 100755
--- a/t/t4034-diff-words.sh
+++ b/t/t4034-diff-words.sh
@@ -314,6 +314,8 @@ test_language_driver php
 test_language_driver python
 test_language_driver ruby
 test_language_driver tex
+test_language_driver shell
+
 
 test_expect_success 'word-diff with diff.sbe' '
 	cat >expect <<-\EOF &&
diff --git a/t/t4034/shell/expect b/t/t4034/shell/expect
new file mode 100644
index 0000000000..f2f65e7a9b
--- /dev/null
+++ b/t/t4034/shell/expect
@@ -0,0 +1,6 @@
+<BOLD>diff --git a/pre b/post<RESET>
+<BOLD>index 2fc00ad..cd34305 100644<RESET>
+<BOLD>--- a/pre<RESET>
+<BOLD>+++ b/post<RESET>
+<CYAN>@@ -1 +1 @@<RESET>
+<RED>[-$TEST_DIRECTORY-]<RESET><GREEN>{+$TEST_DIR+}<RESET>
diff --git a/t/t4034/shell/post b/t/t4034/shell/post
new file mode 100644
index 0000000000..43a84e0188
--- /dev/null
+++ b/t/t4034/shell/post
@@ -0,0 +1 @@
+$TEST_DIR
diff --git a/t/t4034/shell/pre b/t/t4034/shell/pre
new file mode 100644
index 0000000000..32440f90b7
--- /dev/null
+++ b/t/t4034/shell/pre
@@ -0,0 +1 @@
+$TEST_DIRECTORY
diff --git a/userdiff.c b/userdiff.c
index 3a78fbf504..936447a0bc 100644
--- a/userdiff.c
+++ b/userdiff.c
@@ -158,6 +158,13 @@ PATTERNS("csharp",
 	 "[a-zA-Z_][a-zA-Z0-9_]*"
 	 "|[-+0-9.e]+[fFlL]?|0[xXbB]?[0-9a-fA-F]+[lL]?"
 	 "|[-+*/<>%&^|=!]=|--|\\+\\+|<<=?|>>=?|&&|\\|\\||::|->"),
+
+PATTERNS("shell",
+  /* Function Names */
+  "([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*\\([[:space:]]*\\)[[:space:]]*\\{",
+  /* Words */
+  "([$#@A-Za-z_\"\'][$#@A-Za-z0-9_\"\']*)"),
+
 IPATTERN("css",
 	 "![:;][[:space:]]*$\n"
 	 "^[_a-z0-9].*$",
-- 
2.14.2

Re: [GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Christian Couder]

On Fri, Mar 22, 2019 at 5:08 PM Kapil Jain <jkapil.cs@gmail.com> wrote:
>
> Signed-off-by: Kapil Jain <jkapil.cs@gmail.com>
> ---
>
> The test written does not pass, imo there's some problem with the regex part.
> please let me know about the fault.

To save some work by people who could help you, it might be a good
idea to show the output of the failing test, for example the output of
`./t4034-diff-words.sh -i -v` or `./t4034-diff-words.sh -i -v -x`.

Re: [GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Kapil Jain]

On Sun, Mar 24, 2019 at 1:34 AM Christian Couder
<christian.couder@gmail.com> wrote:
>
> To save some work by people who could help you, it might be a good
> idea to show the output of the failing test, for example the output of
> `./t4034-diff-words.sh -i -v` or `./t4034-diff-words.sh -i -v -x`.

Looks like i just needed to know about -i, -v and -x switches, they
helped in debugging.
The problem was with the expect file. It is resolved now.

Thanks, will be resubmitting with updated expect file.

is the parser used to parse the expect file specific to git ? or is it
some general one ?

[GSoC][RFC/PATCH 2/2] userdiff: added shell script support, clears test

[Kapil Jain]

Signed-off-by: Kapil Jain <jkapil.cs@gmail.com>
---

The test passes now, but imo the regex is not working,
because the output of git diff with shell regex remains same
as earlier it was without shell regex.

without shell regex the output was shown as:
-$TEST_DIRECTORY
+$TEST_DIR

with shell regex the output should be:
[-$TEST_DIRECTORY-]
{+$TEST_DIR+}

but even with shell regex the output is:
-$TEST_DIRECTORY
+$TEST_DIR

some thoughts on regex would be helpful.
the shell regex is below:

+
+PATTERNS("shell",
+  /* Function Names */
+  "([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*\\([[:space:]]*\\)[[:space:]]*\\{",
+  /* Words */
+  "([$#@A-Za-z_\"\'][$#@A-Za-z0-9_\"\']*)"),
+

Thanks.

 t/t4034/shell/expect | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4034/shell/expect b/t/t4034/shell/expect
index f2f65e7a9b..1f0d1e1d43 100644
--- a/t/t4034/shell/expect
+++ b/t/t4034/shell/expect
@@ -1,6 +1,6 @@
 <BOLD>diff --git a/pre b/post<RESET>
-<BOLD>index 2fc00ad..cd34305 100644<RESET>
+<BOLD>index 32440f9..43a84e0 100644<RESET>
 <BOLD>--- a/pre<RESET>
 <BOLD>+++ b/post<RESET>
 <CYAN>@@ -1 +1 @@<RESET>
-<RED>[-$TEST_DIRECTORY-]<RESET><GREEN>{+$TEST_DIR+}<RESET>
+<RED>$TEST_DIRECTORY<RESET><GREEN>$TEST_DIR<RESET>
-- 
2.20.1

Re: [GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Christian Couder]

On Sun, Mar 24, 2019 at 9:04 AM Kapil Jain <jkapil.cs@gmail.com> wrote:
>
> On Sun, Mar 24, 2019 at 1:34 AM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > To save some work by people who could help you, it might be a good
> > idea to show the output of the failing test, for example the output of
> > `./t4034-diff-words.sh -i -v` or `./t4034-diff-words.sh -i -v -x`.
>
> Looks like i just needed to know about -i, -v and -x switches, they
> helped in debugging.
> The problem was with the expect file. It is resolved now.

Great!

> Thanks, will be resubmitting with updated expect file.

It looks like you sent "[GSoC][RFC/PATCH 2/2] userdiff: added shell
script support, clears test". I think though that it should have been
named "[GSoC][RFC/PATCH v2] userdiff: added shell script support,
clears test".

"2/2" means that it is patch number 2 in a patch series that has 2
patches (which should be marked with "1/2" and "2/2"). When
resubmitting an already submitted patch (or patch series) we ask for a
version number like "v2", "v3", so that we can know that it has
already been submitted.

`git format-patch -v 2 ...` will automatically add "v2".

> is the parser used to parse the expect file specific to git ? or is it
> some general one ?

The test_language_driver() function used to test the regexps is
defined in t4034-diff-words.sh and it calls the word_diff() function
(also defined in t4034-diff-words.sh) which is:

word_diff () {
    test_must_fail git diff --no-index "$@" pre post >output &&
    test_decode_color <output >output.decrypted &&
    test_cmp expect output.decrypted
}

So it uses test_decode_color() and test_cmp() that are defined in
t/test-lib-functions.sh. test_cmp() in turn is defined using
GIT_TEST_CMP which is usually either "diff -u" or "diff -c".

Re: [GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Kapil Jain]

On Sun, Mar 24, 2019 at 2:49 PM Christian Couder
<christian.couder@gmail.com> wrote:
>
> The test_language_driver() function used to test the regexps is
> ...
> GIT_TEST_CMP which is usually either "diff -u" or "diff -c".

Thanks.

please provide some insights on the regex mentioned below:

+
+PATTERNS("shell",
+  /* Function Names */
+  "([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*\\([[:space:]]*\\)[[:space:]]*\\{",
+  /* Words */
+  "([$#@A-Za-z_\"\'][$#@A-Za-z0-9_\"\']*)"),
+

reference mail:
https://public-inbox.org/git/20190324084523.8744-1-jkapil.cs@gmail.com/.
please let me know if the regex is not self explanatory.

Re: [GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Thomas Gummerer]

On 03/24, Kapil Jain wrote:
> On Sun, Mar 24, 2019 at 2:49 PM Christian Couder
> <christian.couder@gmail.com> wrote:
> >
> > The test_language_driver() function used to test the regexps is
> > ...
> > GIT_TEST_CMP which is usually either "diff -u" or "diff -c".
> 
> Thanks.
> 
> please provide some insights on the regex mentioned below:

I had previously mentioned that this project was attempted already in
my email at [*1*].  Did you take a look at the thread I linked to
there, and the regex used?  I still feel like that previous experience
is something you could learn from.

But that said, I think your assumption in the other email that the
output should be

[-$TEST_DIRECTORY-]
{+$TEST_DIR+}

might not be correct.  The tests are using 'git diff
--word-diff=color', rather than 'git diff --word-diff=plain'.  Only
the latter would add the [- -] and {+ +} around the changed words,
while the former adds the color, which the tests are testing for.

*1*: https://public-inbox.org/git/20190315230515.GJ16414@hank.intra.tgummerer.com/

> +
> +PATTERNS("shell",
> +  /* Function Names */
> +  "([A-Za-z_][A-Za-z0-9_]*)[[:space:]]*\\([[:space:]]*\\)[[:space:]]*\\{",
> +  /* Words */
> +  "([$#@A-Za-z_\"\'][$#@A-Za-z0-9_\"\']*)"),
> +
> 
> reference mail:
> https://public-inbox.org/git/20190324084523.8744-1-jkapil.cs@gmail.com/.
> please let me know if the regex is not self explanatory.

Re: [GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Kapil Jain]

On Fri, Mar 29, 2019 at 3:00 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:
>
>
> I had previously mentioned that this project was attempted already in
> my email at [*1*].  Did you take a look at the thread I linked to
> there, and the regex used?  I still feel like that previous experience
> is something you could learn from.

I saw it, and the regex i have used is a little different from that one.

> But that said, I think your assumption in the other email that the
> output should be
>
> [-$TEST_DIRECTORY-]
> {+$TEST_DIR+}
>
> might not be correct.  The tests are using 'git diff
> --word-diff=color', rather than 'git diff --word-diff=plain'.  Only
> the latter would add the [- -] and {+ +} around the changed words,
> while the former adds the color, which the tests are testing for.

This makes sense, i will write more tests for this.

Re: [GSoC][RFC/PATCH] userdiff: added support for diffing shell scripts

[Thomas Gummerer]

On 03/29, Kapil Jain wrote:
> On Fri, Mar 29, 2019 at 3:00 AM Thomas Gummerer <t.gummerer@gmail.com> wrote:
> >
> >
> > I had previously mentioned that this project was attempted already in
> > my email at [*1*].  Did you take a look at the thread I linked to
> > there, and the regex used?  I still feel like that previous experience
> > is something you could learn from.
> 
> I saw it, and the regex i have used is a little different from that one.

Right, so one thing you should definitely do is to compare the regex
there and the regex you have, and understand why there are differences
when they are trying to do the same thing.  Using that knowledge you
should be able to improve the regex in your patch as well.