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

[Junio C Hamano <gitster@pobox.com>]

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