From: Junio C Hamano <gitster@pobox•com>
To: "René Scharfe" <l.s.r@web•de>
Cc: Duy Nguyen <pclouds@gmail•com>, Git Mailing List <git@vger•kernel.org>
Subject: Re: [PATCH 00/12] Hard coded string length cleanup
Date: Fri, 20 Dec 2013 08:53:27 -0800 [thread overview]
Message-ID: <xmqqfvpnea60.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <52B397FF.4050808@web.de> ("René Scharfe"'s message of "Fri, 20 Dec 2013 02:06:07 +0100")
René Scharfe <l.s.r@web•de> writes:
> Am 20.12.2013 00:50, schrieb Duy Nguyen:
>> On Fri, Dec 20, 2013 at 6:32 AM, René Scharfe <l.s.r@web•de> wrote:
>>> Seeing that skip_prefix_defval is mostly used in the form
>>> skip_prefix_defval(foo, prefix, foo) I wonder if it makes sense to
>>> first change skip_prefix to return the full string instead of NULL
>>> if the prefix is not matched. Would the resulting function cover
>>> most use cases? And would it still be easily usable?
>>
>> That was skip_prefix_gently() that I forgot to replace in a commit
>> message, before I turned it into _defval variant. The reason for
>> _defval is it could be use to chain expression together without adding
>> temporary variables, e.g.
>>
>> - if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
>> + if (isspace(*skip_prefix_defval(line->buf, ">From", "NOSPACE"))) {
>>
>> Without _defval, one would need to do if ((p = skip_prefix(..)) &&
>> isspace(*p)). I'm not entirely sure this is a good thing though as it
>> could make it a bit harder to read.
>
> That usage is quite rare compared to occurrences of
> skip_prefix_defval(foo, prefix, foo), no? Adding a temporary variable
> for them wouldn't be that bad if we can simplify the API to a single
> function -- if that one is usable, that is.
>
> On the other hand, we could add a special function for that example
> and we'd already have three users in the tree (patch below). I think
> that's too narrow a use case for a library function, though. Doing
> the following instead in the three cases doesn't seem to be too bad:
>
> rest = skip_prefix(line->buf, ">From");
> if (rest != line->buf && isspace(*rest)) {
Yeah, I personally feel that the "NOSPACE" hack is a bit too ugly to
live in a code meant to be maintained for a longer term. The above
with a "rest" variable, whose assignment is outside if () condition,
is so far the easiest to read, at least to me.
I am not convinced if skip-prefix-and-space is even a good
abstraction of anything; it feels a bit too specialized.
Thanks.
> ---
> builtin/apply.c | 2 +-
> builtin/mailinfo.c | 4 ++--
> git-compat-util.h | 1 +
> strbuf.c | 9 +++++++++
> 4 files changed, 13 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/apply.c b/builtin/apply.c
> index b0d0986..b96befd 100644
> --- a/builtin/apply.c
> +++ b/builtin/apply.c
> @@ -433,7 +433,7 @@ static unsigned long linelen(const char *buffer, unsigned long size)
>
> static int is_dev_null(const char *str)
> {
> - return !memcmp("/dev/null", str, 9) && isspace(str[9]);
> + return skip_prefix_and_space(str, "/dev/null") != str;
> }
>
> #define TERM_SPACE 1
> diff --git a/builtin/mailinfo.c b/builtin/mailinfo.c
> index 2c3cd8e..2575989 100644
> --- a/builtin/mailinfo.c
> +++ b/builtin/mailinfo.c
> @@ -328,11 +328,11 @@ static int check_header(const struct strbuf *line,
> }
>
> /* for inbody stuff */
> - if (starts_with(line->buf, ">From") && isspace(line->buf[5])) {
> + if (skip_prefix_and_space(line->buf, ">From") != line->buf) {
> ret = 1; /* Should this return 0? */
> goto check_header_out;
> }
> - if (starts_with(line->buf, "[PATCH]") && isspace(line->buf[7])) {
> + if (skip_prefix_and_space(line->buf, "[PATCH]") != line->buf) {
> for (i = 0; header[i]; i++) {
> if (!memcmp("Subject", header[i], 7)) {
> handle_header(&hdr_data[i], line);
> diff --git a/git-compat-util.h b/git-compat-util.h
> index dcb92c4..a083918 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -355,6 +355,7 @@ extern int prefixcmp(const char *str, const char *prefix);
> extern int ends_with(const char *str, const char *suffix);
> extern int suffixcmp(const char *str, const char *suffix);
> extern const char *skip_prefix(const char *str, const char *prefix);
> +extern const char *skip_prefix_and_space(const char *str, const char *prefix);
>
> #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
>
> diff --git a/strbuf.c b/strbuf.c
> index 222df13..768331f 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -47,6 +47,15 @@ const char *skip_prefix(const char *str, const char *prefix)
> return str;
> }
>
> +const char *skip_prefix_and_space(const char *str, const char *prefix)
> +{
> + const char *p = skip_prefix(str, prefix);
> + if (((p != str) || !*prefix) && isspace(*p))
> + return p + 1;
> + else
> + return str;
> +}
> +
> /*
> * Used as the default ->buf value, so that people can always assume
> * buf is non NULL and ->buf is NUL terminated even for a freshly
prev parent reply other threads:[~2013-12-20 16:53 UTC|newest]
Thread overview: 31+ messages / expand[flat|nested] mbox.gz Atom feed top
2013-12-18 14:53 [PATCH 00/12] Hard coded string length cleanup Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 01/12] Make starts_with() a wrapper of skip_prefix() Nguyễn Thái Ngọc Duy
2013-12-18 17:50 ` Junio C Hamano
2013-12-18 18:16 ` Junio C Hamano
2013-12-18 14:53 ` [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing Nguyễn Thái Ngọc Duy
2013-12-20 6:51 ` Johannes Sixt
2013-12-20 7:04 ` Jeff King
2013-12-20 8:46 ` Christian Couder
2013-12-20 10:43 ` René Scharfe
2013-12-20 21:31 ` Junio C Hamano
2013-12-21 4:44 ` Duy Nguyen
2013-12-26 19:27 ` Junio C Hamano
2013-12-28 9:54 ` Jeff King
2013-12-18 14:53 ` [PATCH 03/12] Add and use skip_prefix_defval() Nguyễn Thái Ngọc Duy
2013-12-18 16:27 ` Kent R. Spillner
2013-12-18 17:51 ` Junio C Hamano
2013-12-18 14:53 ` [PATCH 04/12] Replace some use of starts_with() with skip_prefix() Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 05/12] Convert a lot of starts_with() to skip_prefix() Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 06/12] fetch.c: replace some use of starts_with() with skip_prefix() Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 07/12] connect.c: " Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 08/12] refs.c: " Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 09/12] diff.c: reduce code duplication in --stat-xxx parsing Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 10/12] environment.c: replace starts_with() in strip_namespace() with skip_prefix() Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 11/12] diff.c: convert diff_scoreopt_parse to use skip_prefix() Nguyễn Thái Ngọc Duy
2013-12-18 14:53 ` [PATCH 12/12] refs.c: use skip_prefix() in prune_ref() Nguyễn Thái Ngọc Duy
2013-12-18 18:06 ` [PATCH 00/12] Hard coded string length cleanup Junio C Hamano
2013-12-19 23:32 ` René Scharfe
2013-12-19 23:50 ` Duy Nguyen
2013-12-20 1:06 ` René Scharfe
2013-12-20 2:29 ` Duy Nguyen
2013-12-20 16:53 ` Junio C Hamano [this message]
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=xmqqfvpnea60.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=l.s.r@web$(echo .)de \
--cc=pclouds@gmail$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
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