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@xxxxxxxxx> wrote:
>
> "Philip Peterson via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
> > From: Philip Peterson <philip.c.peterson@xxxxxxxxx>
> >
> > 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.
