diff mbox series

[1/2] apply: add unit tests for parse_range and rename to parse_fragment_range

Message ID 2c60c4406d4eb1307a32f23604f3ef8e34ad56d6.1708317938.git.gitgitgadget@gmail.com (mailing list archive)
State New
Headers show
Series apply: add unit tests for parse_range | expand

Commit Message

Philip Feb. 19, 2024, 4:45 a.m. UTC 
From: Philip Peterson <philip.c.peterson@gmail.com>

This patchset makes the parse_range function in apply be non-internal
linkage in order to expose to the unit testing framework. In so doing,
because there is another function called parse_range, I gave this one a more
specific name, parse_fragment_range. Other than that, this commit adds
several test cases (positive and negative) for the function.

Signed-off-by: Philip Peterson <philip.c.peterson@gmail.com>
---
 Makefile               |  1 +
 apply.c                |  8 ++---
 apply.h                |  4 +++
 t/unit-tests/t-apply.c | 67 ++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 76 insertions(+), 4 deletions(-)
 create mode 100644 t/unit-tests/t-apply.c

Comments

Junio C Hamano Feb. 19, 2024, 9:35 p.m. UTC | #1 
"Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Philip Peterson <philip.c.peterson@gmail.com>
>
> This patchset makes the parse_range function in apply be non-internal
> linkage in order to expose to the unit testing framework. In so doing,
> because there is another function called parse_range, I gave this one a more
> specific name, parse_fragment_range. Other than that, this commit adds
> several test cases (positive and negative) for the function.

We do not write "I did this, I did that" in our proposed log
message.  In addition, guidance on the proposed commit log in a
handful of sections in Documentation/SubmittingPatches would be
helpful.

It may probably be a good idea to split this into a preliminary
patch that makes a symbol extern (and doing nothing else), and
the main patch that adds external caller(s) to the function from
a new unit test.

It certainly is better than doing nothing and just make it extern,
but I am not sure "fragment" is specific enough to make the symbol
clearly belong to "apply" API.

> diff --git a/apply.c b/apply.c
> index 7608e3301ca..199a1150df6 100644
> --- a/apply.c
> +++ b/apply.c
> @@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)
>  	return ptr - line;
>  }
>  
> -static int parse_range(const char *line, int len, int offset, const char *expect,
> -		       unsigned long *p1, unsigned long *p2)
> +int parse_fragment_range(const char *line, int len, int offset, const char *expect,
> +			 unsigned long *p1, unsigned long *p2)
>  {
>  	int digits, ex;

Alternatively we could do something like this to make the blast
radius of this patch smaller.

    -static int parse_range(const char *line, int len, int offset, const char *expect,
    +#define apply_parse_fragment_range parse_range
    +int parse_range(const char *line, int len, int offset, const char *expect,
                           unsigned long *p1, unsigned long *p2)

If not for unit-test, this function has no reason to be extern with
such a long name, so it is better to allow internal callers to refer
to it with the name that has been good enough for them for the past
19 years since it was introduced in fab2c257 (git-apply: make the
diffstat output happen for "--stat" only., 2005-05-26).

> diff --git a/apply.h b/apply.h
> index 7cd38b1443c..bbc5e3caeb5 100644
> --- a/apply.h
> +++ b/apply.h
> @@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,
>  		      int options);
>  
>  #endif
> +
> +
> +int parse_fragment_range(const char *line, int len, int offset, const char *expect,
> +		       unsigned long *p1, unsigned long *p2);

This is wrong.  The #endif is about avoiding double inclusion of
this header file and any new declaration must go before it.

> diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
> new file mode 100644

This should go to the next patch.
Philip April 4, 2024, 3:53 a.m. UTC | #2 
Hi,

Thanks for the tips. I am confused about the change request though:

> Alternatively we could do something like this to make the blast
> radius of this patch smaller.
>
> -static int parse_range(const char *line, int len, int offset, const char *expect,
> +#define apply_parse_fragment_range parse_range
> +int parse_range(const char *line, int len, int offset, const char *expect,
>                         unsigned long *p1, unsigned long *p2)

From what I understand, this still creates a new extern symbol
called parse_range. The hope was to avoid creating a new symbol
with a generic name that someone might start to consume, because
if they did start consuming that symbol, it would be a burden on
them when we eventually have to rename it due to the conflict with
the other symbol currently called parse_range in add-patch.c, which
we might also want to add unit tests for later.  Is it not
preferable to just rename it now, before it becomes extern?

Cheers,
Phil

On Mon, Feb 19, 2024 at 4:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> "Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:
>
> > From: Philip Peterson <philip.c.peterson@gmail.com>
> >
> > This patchset makes the parse_range function in apply be non-internal
> > linkage in order to expose to the unit testing framework. In so doing,
> > because there is another function called parse_range, I gave this one a more
> > specific name, parse_fragment_range. Other than that, this commit adds
> > several test cases (positive and negative) for the function.
>
> We do not write "I did this, I did that" in our proposed log
> message.  In addition, guidance on the proposed commit log in a
> handful of sections in Documentation/SubmittingPatches would be
> helpful.
>
> It may probably be a good idea to split this into a preliminary
> patch that makes a symbol extern (and doing nothing else), and
> the main patch that adds external caller(s) to the function from
> a new unit test.
>
> It certainly is better than doing nothing and just make it extern,
> but I am not sure "fragment" is specific enough to make the symbol
> clearly belong to "apply" API.
>
> > diff --git a/apply.c b/apply.c
> > index 7608e3301ca..199a1150df6 100644
> > --- a/apply.c
> > +++ b/apply.c
> > @@ -1430,8 +1430,8 @@ static int parse_num(const char *line, unsigned long *p)
> >       return ptr - line;
> >  }
> >
> > -static int parse_range(const char *line, int len, int offset, const char *expect,
> > -                    unsigned long *p1, unsigned long *p2)
> > +int parse_fragment_range(const char *line, int len, int offset, const char *expect,
> > +                      unsigned long *p1, unsigned long *p2)
> >  {
> >       int digits, ex;
>
> Alternatively we could do something like this to make the blast
> radius of this patch smaller.
>
>     -static int parse_range(const char *line, int len, int offset, const char *expect,
>     +#define apply_parse_fragment_range parse_range
>     +int parse_range(const char *line, int len, int offset, const char *expect,
>                            unsigned long *p1, unsigned long *p2)
>
> If not for unit-test, this function has no reason to be extern with
> such a long name, so it is better to allow internal callers to refer
> to it with the name that has been good enough for them for the past
> 19 years since it was introduced in fab2c257 (git-apply: make the
> diffstat output happen for "--stat" only., 2005-05-26).
>
> > diff --git a/apply.h b/apply.h
> > index 7cd38b1443c..bbc5e3caeb5 100644
> > --- a/apply.h
> > +++ b/apply.h
> > @@ -187,3 +187,7 @@ int apply_all_patches(struct apply_state *state,
> >                     int options);
> >
> >  #endif
> > +
> > +
> > +int parse_fragment_range(const char *line, int len, int offset, const char *expect,
> > +                    unsigned long *p1, unsigned long *p2);
>
> This is wrong.  The #endif is about avoiding double inclusion of
> this header file and any new declaration must go before it.
>
> > diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
> > new file mode 100644
>
> This should go to the next patch.
Junio C Hamano April 4, 2024, 7:27 p.m. UTC | #3 
Philip <philip.c.peterson@gmail.com> writes:

> On Mon, Feb 19, 2024 at 4:35 PM Junio C Hamano <gitster@pobox.com> wrote:
>>
>> "Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:

It's quite a blast from a long time ago that I no longer remember.

>> Alternatively we could do something like this to make the blast
>> radius of this patch smaller.
>>
>> -static int parse_range(const char *line, int len, int offset, const char *expect,
>> +#define apply_parse_fragment_range parse_range
>> +int parse_range(const char *line, int len, int offset, const char *expect,
>>                         unsigned long *p1, unsigned long *p2)
>
> From what I understand, this still creates a new extern symbol
> called parse_range.

Sorry, I misspoke.  The direction of #define is the other way
around.  That is, we may have to give the function a name that is
overly long because it needs to be externally linkable only to
support for your test, but we would want to locally rename that long
name down to the name currently used by the primary callers of that
function, so that the patch does not have to touch these existing
calling sites.  After all, this function is a small implementation
detail and not a part of the official apply.c API, and the only
reason why we are making it extern is because some new tests want to
link with it from the side.

So, in the <apply.h> header file you'll do

	/* 
	 * exposed only for tests; do not call this as it not
	 * a part of the API
	 */
	extern int apply_parse_fragment_range(...);

and then in the original file that used to have "static int
parse_range(...)", you'd add

	#define parse_range apply_parse_fragment_range

near the top, after the header files are included.

Thanks.
diff mbox series

Patch

diff --git a/Makefile b/Makefile
index 15990ff3122..369092aedfe 100644
--- a/Makefile
+++ b/Makefile
@@ -1339,6 +1339,7 @@  THIRD_PARTY_SOURCES += compat/regex/%
 THIRD_PARTY_SOURCES += sha1collisiondetection/%
 THIRD_PARTY_SOURCES += sha1dc/%
 
+UNIT_TEST_PROGRAMS += t-apply
 UNIT_TEST_PROGRAMS += t-basic
 UNIT_TEST_PROGRAMS += t-mem-pool
 UNIT_TEST_PROGRAMS += t-strbuf
diff --git a/apply.c b/apply.c
index 7608e3301ca..199a1150df6 100644
--- a/apply.c
+++ b/apply.c
@@ -1430,8 +1430,8 @@  static int parse_num(const char *line, unsigned long *p)
 	return ptr - line;
 }
 
-static int parse_range(const char *line, int len, int offset, const char *expect,
-		       unsigned long *p1, unsigned long *p2)
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+			 unsigned long *p1, unsigned long *p2)
 {
 	int digits, ex;
 
@@ -1530,8 +1530,8 @@  static int parse_fragment_header(const char *line, int len, struct fragment *fra
 		return -1;
 
 	/* Figure out the number of lines in a fragment */
-	offset = parse_range(line, len, 4, " +", &fragment->oldpos, &fragment->oldlines);
-	offset = parse_range(line, len, offset, " @@", &fragment->newpos, &fragment->newlines);
+	offset = parse_fragment_range(line, len, 4, " +", &fragment->oldpos, &fragment->oldlines);
+	offset = parse_fragment_range(line, len, offset, " @@", &fragment->newpos, &fragment->newlines);
 
 	return offset;
 }
diff --git a/apply.h b/apply.h
index 7cd38b1443c..bbc5e3caeb5 100644
--- a/apply.h
+++ b/apply.h
@@ -187,3 +187,7 @@  int apply_all_patches(struct apply_state *state,
 		      int options);
 
 #endif
+
+
+int parse_fragment_range(const char *line, int len, int offset, const char *expect,
+		       unsigned long *p1, unsigned long *p2);
diff --git a/t/unit-tests/t-apply.c b/t/unit-tests/t-apply.c
new file mode 100644
index 00000000000..ff0abfb2e0b
--- /dev/null
+++ b/t/unit-tests/t-apply.c
@@ -0,0 +1,67 @@ 
+#include "test-lib.h"
+#include "apply.h"
+
+#define FAILURE -1
+
+static void setup_static(const char *line, int len, int offset,
+						 const char *expect, int assert_result,
+						 unsigned long assert_p1,
+						 unsigned long assert_p2)
+{
+	unsigned long p1 = 9999;
+	unsigned long p2 = 9999;
+	int result = parse_fragment_range(line, len, offset, expect, &p1, &p2);
+	check_int(result, ==, assert_result);
+	check_int(p1, ==, assert_p1);
+	check_int(p2, ==, assert_p2);
+}
+
+int cmd_main(int argc, const char **argv)
+{
+	char* text;
+	int expected_result;
+
+	/* Success */
+	text = "@@ -4,4 +";
+	expected_result = 9;
+	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 4),
+		 "well-formed range");
+
+	text = "@@ -4 +8 @@";
+	expected_result = 7;
+	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 1),
+		 "non-comma range");
+
+	/* Failure */
+	text = "@@ -X,4 +";
+	expected_result = FAILURE;
+	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 9999, 9999),
+		 "non-digit range (first coordinate)");
+
+	text = "@@ -4,X +";
+	expected_result = FAILURE;
+	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 1), // p2 is 1, a little strange but not catastrophic
+		 "non-digit range (second coordinate)");
+
+	text = "@@ -4,4 -";
+	expected_result = FAILURE;
+	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 4),
+		 "non-expected trailing text");
+
+	text = "@@ -4,4";
+	expected_result = FAILURE;
+	TEST(setup_static(text, strlen(text), 4, " +", expected_result, 4, 4),
+		 "not long enough for expected trailing text");
+
+	text = "@@ -4,4";
+	expected_result = FAILURE;
+	TEST(setup_static(text, strlen(text), 7, " +", expected_result, 9999, 9999),
+		 "not long enough for offset");
+
+	text = "@@ -4,4";
+	expected_result = FAILURE;
+	TEST(setup_static(text, strlen(text), -1, " +", expected_result, 9999, 9999),
+		 "negative offset");
+
+	return test_done();
+}