* [PATCH] C: use skip-prefix to avoid hardcoded string length
@ 2020-01-27 22:21 Junio C Hamano
2020-01-27 23:20 ` Jeff King
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2020-01-27 22:21 UTC (permalink / raw)
To: git
We often skip an optional prefix in a string with a hardcoded
constant, e.g.
if (starts_with(string, "prefix"))
string += 6;
which is less error prone when written
skip_prefix(string, "prefix", &string);
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
builtin/fast-export.c | 3 +--
builtin/reflog.c | 10 ++++++----
notes.c | 6 ++----
parse-options.c | 3 +--
refs/files-backend.c | 3 +--
remote-curl.c | 5 +++--
sha1-name.c | 9 ++-------
transport-helper.c | 5 +++--
8 files changed, 19 insertions(+), 25 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index dbec4df92b..164406fdd4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -870,8 +870,7 @@ static void handle_tag(const char *name, struct tag *tag)
printf("reset %s\nfrom %s\n\n",
name, oid_to_hex(&null_oid));
}
- if (starts_with(name, "refs/tags/"))
- name += 10;
+ skip_prefix(name, "refs/tags/", &name);
printf("tag %s\n", name);
if (mark_tags) {
mark_next_object(&tag->object);
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4d3430900d..51ffd74945 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -560,15 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
+ const char *valptr;
+
if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
flags |= EXPIRE_REFLOGS_DRY_RUN;
- else if (starts_with(arg, "--expire=")) {
- if (parse_expiry_date(arg + 9, &cb.cmd.expire_total))
+ else if (skip_prefix(arg, "--expire=", &valptr)) {
+ if (parse_expiry_date(valptr, &cb.cmd.expire_total))
die(_("'%s' is not a valid timestamp"), arg);
explicit_expiry |= EXPIRE_TOTAL;
}
- else if (starts_with(arg, "--expire-unreachable=")) {
- if (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))
+ else if (skip_prefix(arg, "--expire-unreachable=", &valptr)) {
+ if (parse_expiry_date(valptr, &cb.cmd.expire_unreachable))
die(_("'%s' is not a valid timestamp"), arg);
explicit_expiry |= EXPIRE_UNREACH;
}
diff --git a/notes.c b/notes.c
index 0c79964c26..a0349fa778 100644
--- a/notes.c
+++ b/notes.c
@@ -1279,10 +1279,8 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid
if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
strbuf_addstr(sb, "\nNotes:\n");
} else {
- if (starts_with(ref, "refs/"))
- ref += 5;
- if (starts_with(ref, "notes/"))
- ref += 6;
+ skip_prefix(ref, "refs/", &ref);
+ skip_prefix(ref, "notes/", &ref);
strbuf_addf(sb, "\nNotes (%s):\n", ref);
}
}
diff --git a/parse-options.c b/parse-options.c
index 60fae3ad21..e8c04109ba 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -357,8 +357,7 @@ static enum parse_opt_result parse_long_opt(
}
/* negated? */
if (!starts_with(arg, "no-")) {
- if (starts_with(long_name, "no-")) {
- long_name += 3;
+ if (skip_prefix(long_name, "no-", &long_name)) {
opt_flags |= OPT_UNSET;
goto again;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0ea66a28b6..561c33ac8a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -465,8 +465,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
close(fd);
strbuf_rtrim(&sb_contents);
buf = sb_contents.buf;
- if (starts_with(buf, "ref:")) {
- buf += 4;
+ if (skip_prefix(buf, "ref:", &buf)) {
while (isspace(*buf))
buf++;
diff --git a/remote-curl.c b/remote-curl.c
index 350d92a074..8eb96152f5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1255,8 +1255,9 @@ static void parse_push(struct strbuf *buf)
int ret;
do {
- if (starts_with(buf->buf, "push "))
- argv_array_push(&specs, buf->buf + 5);
+ const char *arg;
+ if (skip_prefix(buf->buf, "push ", &arg))
+ argv_array_push(&specs, arg);
else
die(_("http transport does not support %s"), buf->buf);
diff --git a/sha1-name.c b/sha1-name.c
index 200eb373ad..75d1c32606 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -908,14 +908,9 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
real_ref, flags, at_time, nth, oid, NULL,
&co_time, &co_tz, &co_cnt)) {
if (!len) {
- if (starts_with(real_ref, "refs/heads/")) {
- str = real_ref + 11;
- len = strlen(real_ref + 11);
- } else {
- /* detached HEAD */
+ if (!skip_prefix(real_ref, "refs/heads/", &str))
str = "HEAD";
- len = 4;
- }
+ len = strlen(str);
}
if (at_time) {
if (!(flags & GET_OID_QUIETLY)) {
diff --git a/transport-helper.c b/transport-helper.c
index 413d9d873e..20a7185ec4 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -404,11 +404,12 @@ static int fetch_with_fetch(struct transport *transport,
sendline(data, &buf);
while (1) {
+ const char *name;
+
if (recvline(data, &buf))
exit(128);
- if (starts_with(buf.buf, "lock ")) {
- const char *name = buf.buf + 5;
+ if (skip_prefix(buf.buf, "lock ", &name)) {
if (transport->pack_lockfile)
warning(_("%s also locked %s"), data->name, name);
else
--
2.25.0-283-g4daa2e6944
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH] C: use skip-prefix to avoid hardcoded string length
2020-01-27 22:21 [PATCH] C: use skip-prefix to avoid hardcoded string length Junio C Hamano
@ 2020-01-27 23:20 ` Jeff King
2020-01-28 0:45 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Jeff King @ 2020-01-27 23:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Mon, Jan 27, 2020 at 02:21:03PM -0800, Junio C Hamano wrote:
> We often skip an optional prefix in a string with a hardcoded
> constant, e.g.
>
> if (starts_with(string, "prefix"))
> string += 6;
>
> which is less error prone when written
>
> skip_prefix(string, "prefix", &string);
All of these look obviously correct, because you've introduced new
temporary variables to replace the computed values. But...
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 4d3430900d..51ffd74945 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -560,15 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>
> for (i = 1; i < argc; i++) {
> const char *arg = argv[i];
> + const char *valptr;
> +
> if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
> flags |= EXPIRE_REFLOGS_DRY_RUN;
> - else if (starts_with(arg, "--expire=")) {
> - if (parse_expiry_date(arg + 9, &cb.cmd.expire_total))
> + else if (skip_prefix(arg, "--expire=", &valptr)) {
> + if (parse_expiry_date(valptr, &cb.cmd.expire_total))
> die(_("'%s' is not a valid timestamp"), arg);
> explicit_expiry |= EXPIRE_TOTAL;
> }
In this case, I think the die message in the context could be improved
to show "valptr". At which point you might as well do:
else if (skip_prefix(arg, "--expire=", &arg)) {
> - else if (starts_with(arg, "--expire-unreachable=")) {
> - if (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))
> + else if (skip_prefix(arg, "--expire-unreachable=", &valptr)) {
> + if (parse_expiry_date(valptr, &cb.cmd.expire_unreachable))
> die(_("'%s' is not a valid timestamp"), arg);
> explicit_expiry |= EXPIRE_UNREACH;
> }
Ditto here.
-Peff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH] C: use skip-prefix to avoid hardcoded string length
2020-01-27 23:20 ` Jeff King
@ 2020-01-28 0:45 ` Junio C Hamano
2020-01-30 19:35 ` [PATCH v2] " Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Junio C Hamano @ 2020-01-28 0:45 UTC (permalink / raw)
To: Jeff King; +Cc: git
Jeff King <peff@peff.net> writes:
>> + else if (skip_prefix(arg, "--expire=", &valptr)) {
>> + if (parse_expiry_date(valptr, &cb.cmd.expire_total))
>> die(_("'%s' is not a valid timestamp"), arg);
>> explicit_expiry |= EXPIRE_TOTAL;
>> }
>
> In this case, I think the die message in the context could be improved
> to show "valptr". At which point you might as well do:
>
> else if (skip_prefix(arg, "--expire=", &arg)) {
The version that loses "to which parameter did I give malformed
timestamp?" information was what I originally have written, and then
I added a new valptr variable to avoid the information loss and
message change.
But thinking about it again,
git frotz --expire=tea --expire-unreachable=tee
would say "I don't know 'tee'" and then the user can go back and see
to which one the misspelt version went, and if the user did
git frotz --expire=tee --expire-unreachable=tee
and got "I don't know 'tee'", then it also is OK to give that
without saying it is about --expire or --expire-unreachable; they
are both wrong ;-)
So, I guess it probably is a good idea to skip the option name in
the error message (we might have to adjust some tests, though).
Thanks.
>> - else if (starts_with(arg, "--expire-unreachable=")) {
>> - if (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))
>> + else if (skip_prefix(arg, "--expire-unreachable=", &valptr)) {
>> + if (parse_expiry_date(valptr, &cb.cmd.expire_unreachable))
>> die(_("'%s' is not a valid timestamp"), arg);
>> explicit_expiry |= EXPIRE_UNREACH;
>> }
>
> Ditto here.
>
> -Peff
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] C: use skip-prefix to avoid hardcoded string length
2020-01-28 0:45 ` Junio C Hamano
@ 2020-01-30 19:35 ` Junio C Hamano
2020-01-31 0:19 ` Jeff King
2020-01-31 20:15 ` Andrei Rybak
0 siblings, 2 replies; 7+ messages in thread
From: Junio C Hamano @ 2020-01-30 19:35 UTC (permalink / raw)
To: git; +Cc: Jeff King
We often skip an optional prefix in a string with a hardcoded
constant, e.g.
if (starts_with(string, "prefix"))
string += 6;
which is less error prone when written
skip_prefix(string, "prefix", &string);
Note that this changes a few error messages from "git reflog expire
--expire=nonsense.timestamp", which used to complain by saying
'--expire=nonsense.timestamp' is not a valid timestamp
but with this change, we say
'nonsense.timestamp' is not a valid timestamp
which is more technically correct (the string with --expire= as
a prefix obviously cannot be a valid timestamp, but the error is
about the part of the input without that prefix).
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
* v2 uses a simpler conversion that has a side effect of changing
the error message from "git reflog expire" in a better way.
builtin/fast-export.c | 3 +--
builtin/reflog.c | 9 +++++----
notes.c | 6 ++----
parse-options.c | 3 +--
refs/files-backend.c | 3 +--
remote-curl.c | 5 +++--
sha1-name.c | 9 ++-------
transport-helper.c | 5 +++--
8 files changed, 18 insertions(+), 25 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index dbec4df92b..164406fdd4 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -870,8 +870,7 @@ static void handle_tag(const char *name, struct tag *tag)
printf("reset %s\nfrom %s\n\n",
name, oid_to_hex(&null_oid));
}
- if (starts_with(name, "refs/tags/"))
- name += 10;
+ skip_prefix(name, "refs/tags/", &name);
printf("tag %s\n", name);
if (mark_tags) {
mark_next_object(&tag->object);
diff --git a/builtin/reflog.c b/builtin/reflog.c
index 4d3430900d..81dfd563c0 100644
--- a/builtin/reflog.c
+++ b/builtin/reflog.c
@@ -560,15 +560,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
+
if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
flags |= EXPIRE_REFLOGS_DRY_RUN;
- else if (starts_with(arg, "--expire=")) {
- if (parse_expiry_date(arg + 9, &cb.cmd.expire_total))
+ else if (skip_prefix(arg, "--expire=", &arg)) {
+ if (parse_expiry_date(arg, &cb.cmd.expire_total))
die(_("'%s' is not a valid timestamp"), arg);
explicit_expiry |= EXPIRE_TOTAL;
}
- else if (starts_with(arg, "--expire-unreachable=")) {
- if (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))
+ else if (skip_prefix(arg, "--expire-unreachable=", &arg)) {
+ if (parse_expiry_date(arg, &cb.cmd.expire_unreachable))
die(_("'%s' is not a valid timestamp"), arg);
explicit_expiry |= EXPIRE_UNREACH;
}
diff --git a/notes.c b/notes.c
index 0c79964c26..a0349fa778 100644
--- a/notes.c
+++ b/notes.c
@@ -1279,10 +1279,8 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid
if (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {
strbuf_addstr(sb, "\nNotes:\n");
} else {
- if (starts_with(ref, "refs/"))
- ref += 5;
- if (starts_with(ref, "notes/"))
- ref += 6;
+ skip_prefix(ref, "refs/", &ref);
+ skip_prefix(ref, "notes/", &ref);
strbuf_addf(sb, "\nNotes (%s):\n", ref);
}
}
diff --git a/parse-options.c b/parse-options.c
index 60fae3ad21..e8c04109ba 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -357,8 +357,7 @@ static enum parse_opt_result parse_long_opt(
}
/* negated? */
if (!starts_with(arg, "no-")) {
- if (starts_with(long_name, "no-")) {
- long_name += 3;
+ if (skip_prefix(long_name, "no-", &long_name)) {
opt_flags |= OPT_UNSET;
goto again;
}
diff --git a/refs/files-backend.c b/refs/files-backend.c
index 0ea66a28b6..561c33ac8a 100644
--- a/refs/files-backend.c
+++ b/refs/files-backend.c
@@ -465,8 +465,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,
close(fd);
strbuf_rtrim(&sb_contents);
buf = sb_contents.buf;
- if (starts_with(buf, "ref:")) {
- buf += 4;
+ if (skip_prefix(buf, "ref:", &buf)) {
while (isspace(*buf))
buf++;
diff --git a/remote-curl.c b/remote-curl.c
index 350d92a074..8eb96152f5 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -1255,8 +1255,9 @@ static void parse_push(struct strbuf *buf)
int ret;
do {
- if (starts_with(buf->buf, "push "))
- argv_array_push(&specs, buf->buf + 5);
+ const char *arg;
+ if (skip_prefix(buf->buf, "push ", &arg))
+ argv_array_push(&specs, arg);
else
die(_("http transport does not support %s"), buf->buf);
diff --git a/sha1-name.c b/sha1-name.c
index 200eb373ad..75d1c32606 100644
--- a/sha1-name.c
+++ b/sha1-name.c
@@ -908,14 +908,9 @@ static int get_oid_basic(struct repository *r, const char *str, int len,
real_ref, flags, at_time, nth, oid, NULL,
&co_time, &co_tz, &co_cnt)) {
if (!len) {
- if (starts_with(real_ref, "refs/heads/")) {
- str = real_ref + 11;
- len = strlen(real_ref + 11);
- } else {
- /* detached HEAD */
+ if (!skip_prefix(real_ref, "refs/heads/", &str))
str = "HEAD";
- len = 4;
- }
+ len = strlen(str);
}
if (at_time) {
if (!(flags & GET_OID_QUIETLY)) {
diff --git a/transport-helper.c b/transport-helper.c
index 413d9d873e..20a7185ec4 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -404,11 +404,12 @@ static int fetch_with_fetch(struct transport *transport,
sendline(data, &buf);
while (1) {
+ const char *name;
+
if (recvline(data, &buf))
exit(128);
- if (starts_with(buf.buf, "lock ")) {
- const char *name = buf.buf + 5;
+ if (skip_prefix(buf.buf, "lock ", &name)) {
if (transport->pack_lockfile)
warning(_("%s also locked %s"), data->name, name);
else
--
2.25.0-299-g5579215f86
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] C: use skip-prefix to avoid hardcoded string length
2020-01-30 19:35 ` [PATCH v2] " Junio C Hamano
@ 2020-01-31 0:19 ` Jeff King
2020-01-31 20:15 ` Andrei Rybak
1 sibling, 0 replies; 7+ messages in thread
From: Jeff King @ 2020-01-31 0:19 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Thu, Jan 30, 2020 at 11:35:46AM -0800, Junio C Hamano wrote:
> * v2 uses a simpler conversion that has a side effect of changing
> the error message from "git reflog expire" in a better way.
Thanks, this all looks good to me.
> diff --git a/builtin/reflog.c b/builtin/reflog.c
> index 4d3430900d..81dfd563c0 100644
> --- a/builtin/reflog.c
> +++ b/builtin/reflog.c
> @@ -560,15 +560,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)
>
> for (i = 1; i < argc; i++) {
> const char *arg = argv[i];
> +
> if (!strcmp(arg, "--dry-run") || !strcmp(arg, "-n"))
> flags |= EXPIRE_REFLOGS_DRY_RUN;
> - else if (starts_with(arg, "--expire=")) {
> - if (parse_expiry_date(arg + 9, &cb.cmd.expire_total))
> + else if (skip_prefix(arg, "--expire=", &arg)) {
> + if (parse_expiry_date(arg, &cb.cmd.expire_total))
> die(_("'%s' is not a valid timestamp"), arg);
> explicit_expiry |= EXPIRE_TOTAL;
> }
I'm not sure if you meant to leave the blank-line addition, or if it was
a leftover from when there was a variable added there in v1. I think the
result is more readable, though, so I certainly don't mind it. ;)
-Peff
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] C: use skip-prefix to avoid hardcoded string length
2020-01-30 19:35 ` [PATCH v2] " Junio C Hamano
2020-01-31 0:19 ` Jeff King
@ 2020-01-31 20:15 ` Andrei Rybak
2020-01-31 21:03 ` Junio C Hamano
1 sibling, 1 reply; 7+ messages in thread
From: Andrei Rybak @ 2020-01-31 20:15 UTC (permalink / raw)
To: Junio C Hamano, Git List; +Cc: Jeff King
On 1/30/20 8:35 PM, Junio C Hamano wrote:
> We often skip an optional prefix in a string with a hardcoded
> constant, e.g.
>
> if (starts_with(string, "prefix"))
> string += 6;
>
> which is less error prone when written
>
> skip_prefix(string, "prefix", &string);
Should the subject line say "skip_prefix" with an underscore instead of
"skip-prefix" with a dash?
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v2] C: use skip-prefix to avoid hardcoded string length
2020-01-31 20:15 ` Andrei Rybak
@ 2020-01-31 21:03 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2020-01-31 21:03 UTC (permalink / raw)
To: Andrei Rybak; +Cc: Git List, Jeff King
Andrei Rybak <rybak.a.v@gmail.com> writes:
> On 1/30/20 8:35 PM, Junio C Hamano wrote:
>> We often skip an optional prefix in a string with a hardcoded
>> constant, e.g.
>>
>> if (starts_with(string, "prefix"))
>> string += 6;
>>
>> which is less error prone when written
>>
>> skip_prefix(string, "prefix", &string);
>
> Should the subject line say "skip_prefix" with an underscore instead of
> "skip-prefix" with a dash?
Perhaps. Or "skip_prefix()" to clarify that we are talking about a
function.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2020-01-31 21:03 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2020-01-27 22:21 [PATCH] C: use skip-prefix to avoid hardcoded string length Junio C Hamano
2020-01-27 23:20 ` Jeff King
2020-01-28 0:45 ` Junio C Hamano
2020-01-30 19:35 ` [PATCH v2] " Junio C Hamano
2020-01-31 0:19 ` Jeff King
2020-01-31 20:15 ` Andrei Rybak
2020-01-31 21:03 ` Junio C Hamano
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
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).