From: Junio C Hamano <gitster@pobox.com>
To: Kousik Sanagavarapu <five231003@gmail.com>
Cc: git@vger.kernel.org,
Christian Couder <christian.couder@gmail.com>,
Hariom Verma <hariom18599@gmail.com>,
Josh Steadmon <steadmon@google.com>,
Siddharth Singh <siddhartth@google.com>,
Glen Choo <chooglen@google.com>
Subject: Re: [PATCH v3 1/2] ref-filter: add multiple-option parsing functions
Date: Wed, 19 Jul 2023 16:23:15 -0700 [thread overview]
Message-ID: <xmqqjzuv5vvg.fsf@gitster.g> (raw)
In-Reply-To: <20230719162424.70781-2-five231003@gmail.com> (Kousik Sanagavarapu's message of "Wed, 19 Jul 2023 21:45:05 +0530")
Kousik Sanagavarapu <five231003@gmail.com> writes:
> ref-filter.c | 59 ++++++++++++++++++++++++++++++++++++++++++++++++++++
> 1 file changed, 59 insertions(+)
New helper functions that do not have any caller and no
documentation to explain how they are supposed to be called
(i.e. the expectation on the callers---what values they need to feed
as parameters when they call these helpers, and the expectation by
the callers---what they expect to get out of the helpers once they
return) makes it impossible to evaluate if they are any good [*].
Side note. Those of you who are keen to add unit tests to
the system (Cc:ed) , do you think a patch line this one that
adds a new helper function to the system, would benefit from
being able to add a few unit tests for these otherwise
unused helper functions?
The calls to the new functions that the unit test framework
would make should serve as a good piece of interface
documentation, showing what the callers are supposed to pass
and what they expect, I guess.
So whatever framework we choose, it should allow adding a
test or two to this patch easily, without being too
intrusive. Would that be a good and concrete evaluation
criterion?
Anyway, because of that, I had to read [2/2] first and then come
back here to review this one.
The following is my attempt to write down the contract between the
callers and this new helper function---please give something like
that to the final version. The the example below is there just to
illustrate the level of information that would be desired to help
future readers and programmers. Do not take the contents as-written
as truth---I may have (deliberately) mixed in incorrect descriptions
;-).
/*
* The string "to_parse" is expected to be a comma-separated list
* of "key" or "key=val". If your atom allows "key1" and "key2"
* (possibly with their values) as options, make two calls to this
* funtion, passing "key1" in candiate and then passing "key2" in
* candidate.
*
* The function Returns true ONLY when the to_parse string begins
* with the candidate key, possibly followed by its value (valueless
* key-only entries are allowed in the comman-separated list).
* Otherwise, *end, *valuestart and *valuelen are LEFT INTACT and
* the function returns false.
*
* *valuestart will point at the byte after '=' (i.e. the beginning
* of the value), and the number of bytes in the value will be set
* to *valuelen.
* A key-only entry results in *valuestart set to NULL and *valuelen
* set to 0.
* *end will point at the next key[=val] in the comma-separated list
* or NULL when the list ran out.
*/
> +static int match_atom_arg_value(const char *to_parse, const char *candidate,
> + const char **end, const char **valuestart,
> + size_t *valuelen)
> +{
> + const char *atom;
> +
> + if (!(skip_prefix(to_parse, candidate, &atom)))
> + return 0;
> + if (valuestart) {
As far as I saw, no callers pass NULL to valuestart. Getting rid of
this if() statement and always entering its body would clarify what
is going on, I think.
> + if (*atom == '=') {
> + *valuestart = atom + 1;
> + *valuelen = strcspn(*valuestart, ",\0");
> + atom = *valuestart + *valuelen;
> + } else {
> + if (*atom != ',' && *atom != '\0')
> + return 0;
> + *valuestart = NULL;
> + *valuelen = 0;
> + }
> + }
> + if (*atom == ',') {
> + *end = atom + 1;
> + return 1;
> + }
> + if (*atom == '\0') {
> + *end = atom;
> + return 1;
> + }
> + return 0;
> +}
/*
* Write something similar to document the contract between the caller
* and this function here.
*/
> +static int match_atom_bool_arg(const char *to_parse, const char *candidate,
> + const char **end, int *val)
> +{
Thanks.
next prev parent reply other threads:[~2023-07-19 23:24 UTC|newest]
Thread overview: 38+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-07-05 17:57 [PATCH 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 1/2] ref-filter: add " Kousik Sanagavarapu
2023-07-06 16:58 ` Junio C Hamano
2023-07-09 6:16 ` Kousik Sanagavarapu
2023-07-05 17:57 ` [PATCH 2/2] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 0/3] Add new "describe" atom Kousik Sanagavarapu
2023-07-14 19:20 ` [RFC PATCH v2 1/3] ref filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-14 19:20 ` [PATCH v2 2/3] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-14 20:57 ` Junio C Hamano
2023-07-15 18:24 ` Kousik Sanagavarapu
2023-07-15 18:56 ` Junio C Hamano
2023-07-14 19:20 ` [PATCH v2 3/3] t6300: run describe atom tests on a different repo Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 0/2] Add new "describe" atom Kousik Sanagavarapu
2023-07-19 16:15 ` [PATCH v3 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-19 23:23 ` Junio C Hamano [this message]
2023-07-20 5:21 ` Junio C Hamano
2023-07-20 16:52 ` Kousik Sanagavarapu
2023-07-20 17:59 ` Junio C Hamano
2023-07-20 17:42 ` Glen Choo
2023-07-20 20:30 ` Junio C Hamano
2023-07-21 18:26 ` Glen Choo
2023-07-19 16:15 ` [PATCH v3 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-19 22:56 ` Junio C Hamano
2023-07-20 22:52 ` [PATCH v3 0/2] Add " Junio C Hamano
2023-07-20 23:10 ` Junio C Hamano
2023-07-21 4:17 ` Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 " Kousik Sanagavarapu
2023-07-23 16:19 ` [PATCH v4 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-24 17:29 ` Junio C Hamano
2023-07-24 18:12 ` Kousik Sanagavarapu
2023-07-24 20:39 ` Junio C Hamano
2023-07-25 19:27 ` Junio C Hamano
2023-07-23 16:19 ` [PATCH v4 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-24 17:21 ` Junio C Hamano
2023-07-25 20:51 ` [PATCH v5 0/2] Add " Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 1/2] ref-filter: add multiple-option parsing functions Kousik Sanagavarapu
2023-07-25 20:51 ` [PATCH v5 2/2] ref-filter: add new "describe" atom Kousik Sanagavarapu
2023-07-25 21:46 ` [PATCH v5 0/2] Add " Junio C Hamano
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=xmqqjzuv5vvg.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=chooglen@google.com \
--cc=christian.couder@gmail.com \
--cc=five231003@gmail.com \
--cc=git@vger.kernel.org \
--cc=hariom18599@gmail.com \
--cc=siddhartth@google.com \
--cc=steadmon@google.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).