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

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 19, 2024, 21:35 UTC
Message-ID: <xmqqil2k5e8u.fsf@gitster.g>
In-Reply-To: <2c60c4406d4eb1307a32f23604f3ef8e34ad56d6.1708317938.git.gitgitgadget@gmail.com>

"Philip Peterson via GitGitGadget" <gitgitgadget@gmail.com> writes:

Show 7 quoted lines

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

Show 14 quoted lines

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

Show 12 quoted lines

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