From: Philip <philip.c.peterson@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Philip Peterson via GitGitGadget <gitgitgadget@gmail.com>,
git@vger.kernel.org
Subject: Re: [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range
Date: Wed, 3 Apr 2024 23:53:27 -0400 [thread overview]
Message-ID: <CAJ6X7_Uc0OdzYToJSs15+vbydraKAB8x4DPj7UsL1PKLzyY0dQ@mail.gmail.com> (raw)
In-Reply-To: <xmqqil2k5e8u.fsf@gitster.g>
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.
next prev parent reply other threads:[~2024-04-04 3:54 UTC|newest]
Thread overview: 9+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-02-19 4:45 [PATCH 0/2] apply: add unit tests for parse_range Philip Peterson via GitGitGadget
2024-02-19 4:45 ` [PATCH 1/2] apply: add unit tests for parse_range and rename to parse_fragment_range Philip Peterson via GitGitGadget
2024-02-19 21:35 ` Junio C Hamano
2024-04-04 3:53 ` Philip [this message]
2024-04-04 19:27 ` Junio C Hamano
2024-02-19 4:45 ` [PATCH 2/2] apply: rewrite unit tests with structured cases Philip Peterson via GitGitGadget
2024-02-19 21:49 ` Junio C Hamano
2024-02-19 22:04 ` Kristoffer Haugsbakk
2024-05-26 7:54 ` [PATCH v2] apply: add unit tests for parse_range Philip Peterson via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=CAJ6X7_Uc0OdzYToJSs15+vbydraKAB8x4DPj7UsL1PKLzyY0dQ@mail.gmail.com \
--to=philip.c.peterson@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).