{"thread":{"id":"52705","subject":"[PATCH] C: use skip-prefix to avoid hardcoded string length","startedAt":"2020-01-27T22:21:11Z","lastAt":"2020-01-31T21:03:36Z","messageCount":7,"participants":["Junio C Hamano","Jeff King","Andrei Rybak"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"390586","messageId":"xmqqv9owa6cw.fsf@gitster-ct.c.googlers.com","threadId":"52705","inReplyTo":null,"subject":"[PATCH] C: use skip-prefix to avoid hardcoded string length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-27T22:21:03Z","receivedAt":"2020-01-27T22:21:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We often skip an optional prefix in a string with a hardcoded\nconstant, e.g.\n\n\tif (starts_with(string, \"prefix\"))\n\t\tstring += 6;\n\nwhich is less error prone when written\n\n\tskip_prefix(string, \"prefix\", &string);\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/fast-export.c |  3 +--\n builtin/reflog.c      | 10 ++++++----\n notes.c               |  6 ++----\n parse-options.c       |  3 +--\n refs/files-backend.c  |  3 +--\n remote-curl.c         |  5 +++--\n sha1-name.c           |  9 ++-------\n transport-helper.c    |  5 +++--\n 8 files changed, 19 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex dbec4df92b..164406fdd4 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -870,8 +870,7 @@ static void handle_tag(const char *name, struct tag *tag)\n \t\tprintf(\"reset %s\\nfrom %s\\n\\n\",\n \t\t       name, oid_to_hex(&null_oid));\n \t}\n-\tif (starts_with(name, \"refs/tags/\"))\n-\t\tname += 10;\n+\tskip_prefix(name, \"refs/tags/\", &name);\n \tprintf(\"tag %s\\n\", name);\n \tif (mark_tags) {\n \t\tmark_next_object(&tag->object);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 4d3430900d..51ffd74945 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -560,15 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n+\t\tconst char *valptr;\n+\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n-\t\telse if (starts_with(arg, \"--expire=\")) {\n-\t\t\tif (parse_expiry_date(arg + 9, &cb.cmd.expire_total))\n+\t\telse if (skip_prefix(arg, \"--expire=\", &valptr)) {\n+\t\t\tif (parse_expiry_date(valptr, &cb.cmd.expire_total))\n \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n \t\t}\n-\t\telse if (starts_with(arg, \"--expire-unreachable=\")) {\n-\t\t\tif (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))\n+\t\telse if (skip_prefix(arg, \"--expire-unreachable=\", &valptr)) {\n+\t\t\tif (parse_expiry_date(valptr, &cb.cmd.expire_unreachable))\n \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n \t\t}\ndiff --git a/notes.c b/notes.c\nindex 0c79964c26..a0349fa778 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1279,10 +1279,8 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid\n \t\tif (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {\n \t\t\tstrbuf_addstr(sb, \"\\nNotes:\\n\");\n \t\t} else {\n-\t\t\tif (starts_with(ref, \"refs/\"))\n-\t\t\t\tref += 5;\n-\t\t\tif (starts_with(ref, \"notes/\"))\n-\t\t\t\tref += 6;\n+\t\t\tskip_prefix(ref, \"refs/\", &ref);\n+\t\t\tskip_prefix(ref, \"notes/\", &ref);\n \t\t\tstrbuf_addf(sb, \"\\nNotes (%s):\\n\", ref);\n \t\t}\n \t}\ndiff --git a/parse-options.c b/parse-options.c\nindex 60fae3ad21..e8c04109ba 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -357,8 +357,7 @@ static enum parse_opt_result parse_long_opt(\n \t\t\t}\n \t\t\t/* negated? */\n \t\t\tif (!starts_with(arg, \"no-\")) {\n-\t\t\t\tif (starts_with(long_name, \"no-\")) {\n-\t\t\t\t\tlong_name += 3;\n+\t\t\t\tif (skip_prefix(long_name, \"no-\", &long_name)) {\n \t\t\t\t\topt_flags |= OPT_UNSET;\n \t\t\t\t\tgoto again;\n \t\t\t\t}\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0ea66a28b6..561c33ac8a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -465,8 +465,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tclose(fd);\n \tstrbuf_rtrim(&sb_contents);\n \tbuf = sb_contents.buf;\n-\tif (starts_with(buf, \"ref:\")) {\n-\t\tbuf += 4;\n+\tif (skip_prefix(buf, \"ref:\", &buf)) {\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex 350d92a074..8eb96152f5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1255,8 +1255,9 @@ static void parse_push(struct strbuf *buf)\n \tint ret;\n \n \tdo {\n-\t\tif (starts_with(buf->buf, \"push \"))\n-\t\t\targv_array_push(&specs, buf->buf + 5);\n+\t\tconst char *arg;\n+\t\tif (skip_prefix(buf->buf, \"push \", &arg))\n+\t\t\targv_array_push(&specs, arg);\n \t\telse\n \t\t\tdie(_(\"http transport does not support %s\"), buf->buf);\n \ndiff --git a/sha1-name.c b/sha1-name.c\nindex 200eb373ad..75d1c32606 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -908,14 +908,9 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\t\treal_ref, flags, at_time, nth, oid, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n \t\t\tif (!len) {\n-\t\t\t\tif (starts_with(real_ref, \"refs/heads/\")) {\n-\t\t\t\t\tstr = real_ref + 11;\n-\t\t\t\t\tlen = strlen(real_ref + 11);\n-\t\t\t\t} else {\n-\t\t\t\t\t/* detached HEAD */\n+\t\t\t\tif (!skip_prefix(real_ref, \"refs/heads/\", &str))\n \t\t\t\t\tstr = \"HEAD\";\n-\t\t\t\t\tlen = 4;\n-\t\t\t\t}\n+\t\t\t\tlen = strlen(str);\n \t\t\t}\n \t\t\tif (at_time) {\n \t\t\t\tif (!(flags & GET_OID_QUIETLY)) {\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 413d9d873e..20a7185ec4 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -404,11 +404,12 @@ static int fetch_with_fetch(struct transport *transport,\n \tsendline(data, &buf);\n \n \twhile (1) {\n+\t\tconst char *name;\n+\n \t\tif (recvline(data, &buf))\n \t\t\texit(128);\n \n-\t\tif (starts_with(buf.buf, \"lock \")) {\n-\t\t\tconst char *name = buf.buf + 5;\n+\t\tif (skip_prefix(buf.buf, \"lock \", &name)) {\n \t\t\tif (transport->pack_lockfile)\n \t\t\t\twarning(_(\"%s also locked %s\"), data->name, name);\n \t\t\telse\n-- \n2.25.0-283-g4daa2e6944\n\n"},{"id":"390596","messageId":"20200127232004.GE19360@coredump.intra.peff.net","threadId":"52705","inReplyTo":"xmqqv9owa6cw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH] C: use skip-prefix to avoid hardcoded string length","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-27T23:20:04Z","receivedAt":"2020-01-27T23:20:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jan 27, 2020 at 02:21:03PM -0800, Junio C Hamano wrote:\n\n> We often skip an optional prefix in a string with a hardcoded\n> constant, e.g.\n> \n> \tif (starts_with(string, \"prefix\"))\n> \t\tstring += 6;\n> \n> which is less error prone when written\n> \n> \tskip_prefix(string, \"prefix\", &string);\n\nAll of these look obviously correct, because you've introduced new\ntemporary variables to replace the computed values. But...\n\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 4d3430900d..51ffd74945 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -560,15 +560,17 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n>  \n>  \tfor (i = 1; i < argc; i++) {\n>  \t\tconst char *arg = argv[i];\n> +\t\tconst char *valptr;\n> +\n>  \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n>  \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n> -\t\telse if (starts_with(arg, \"--expire=\")) {\n> -\t\t\tif (parse_expiry_date(arg + 9, &cb.cmd.expire_total))\n> +\t\telse if (skip_prefix(arg, \"--expire=\", &valptr)) {\n> +\t\t\tif (parse_expiry_date(valptr, &cb.cmd.expire_total))\n>  \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n>  \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n>  \t\t}\n\nIn this case, I think the die message in the context could be improved\nto show \"valptr\". At which point you might as well do:\n\n  else if (skip_prefix(arg, \"--expire=\", &arg)) {\n\n> -\t\telse if (starts_with(arg, \"--expire-unreachable=\")) {\n> -\t\t\tif (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))\n> +\t\telse if (skip_prefix(arg, \"--expire-unreachable=\", &valptr)) {\n> +\t\t\tif (parse_expiry_date(valptr, &cb.cmd.expire_unreachable))\n>  \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n>  \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n>  \t\t}\n\nDitto here.\n\n-Peff\n"},{"id":"390599","messageId":"xmqq36c09zo0.fsf@gitster-ct.c.googlers.com","threadId":"52705","inReplyTo":"20200127232004.GE19360@coredump.intra.peff.net","subject":"Re: [PATCH] C: use skip-prefix to avoid hardcoded string length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-28T00:45:35Z","receivedAt":"2020-01-28T00:45:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n>> +\t\telse if (skip_prefix(arg, \"--expire=\", &valptr)) {\n>> +\t\t\tif (parse_expiry_date(valptr, &cb.cmd.expire_total))\n>>  \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n>>  \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n>>  \t\t}\n>\n> In this case, I think the die message in the context could be improved\n> to show \"valptr\". At which point you might as well do:\n>\n>   else if (skip_prefix(arg, \"--expire=\", &arg)) {\n\nThe version that loses \"to which parameter did I give malformed\ntimestamp?\" information was what I originally have written, and then\nI added a new valptr variable to avoid the information loss and\nmessage change.\n\nBut thinking about it again,\n\n\tgit frotz --expire=tea --expire-unreachable=tee\n\nwould say \"I don't know 'tee'\" and then the user can go back and see\nto which one the misspelt version went, and if the user did\n\n\tgit frotz --expire=tee --expire-unreachable=tee\n\nand got \"I don't know 'tee'\", then it also is OK to give that\nwithout saying it is about --expire or --expire-unreachable; they\nare both wrong ;-)\n\nSo, I guess it probably is a good idea to skip the option name in\nthe error message (we might have to adjust some tests, though).\n\nThanks.\n\n>> -\t\telse if (starts_with(arg, \"--expire-unreachable=\")) {\n>> -\t\t\tif (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))\n>> +\t\telse if (skip_prefix(arg, \"--expire-unreachable=\", &valptr)) {\n>> +\t\t\tif (parse_expiry_date(valptr, &cb.cmd.expire_unreachable))\n>>  \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n>>  \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n>>  \t\t}\n>\n> Ditto here.\n>\n> -Peff\n"},{"id":"390831","messageId":"xmqqa764d9f1.fsf_-_@gitster-ct.c.googlers.com","threadId":"52705","inReplyTo":"xmqq36c09zo0.fsf@gitster-ct.c.googlers.com","subject":"[PATCH v2] C: use skip-prefix to avoid hardcoded string length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-30T19:35:46Z","receivedAt":"2020-01-30T19:35:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"We often skip an optional prefix in a string with a hardcoded\nconstant, e.g.\n\n\tif (starts_with(string, \"prefix\"))\n\t\tstring += 6;\n\nwhich is less error prone when written\n\n\tskip_prefix(string, \"prefix\", &string);\n\nNote that this changes a few error messages from \"git reflog expire\n--expire=nonsense.timestamp\", which used to complain by saying\n\n    '--expire=nonsense.timestamp' is not a valid timestamp\n\nbut with this change, we say\n\n    'nonsense.timestamp' is not a valid timestamp\n\nwhich is more technically correct (the string with --expire= as\na prefix obviously cannot be a valid timestamp, but the error is\nabout the part of the input without that prefix).\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * v2 uses a simpler conversion that has a side effect of changing\n   the error message from \"git reflog expire\" in a better way.\n\n builtin/fast-export.c | 3 +--\n builtin/reflog.c      | 9 +++++----\n notes.c               | 6 ++----\n parse-options.c       | 3 +--\n refs/files-backend.c  | 3 +--\n remote-curl.c         | 5 +++--\n sha1-name.c           | 9 ++-------\n transport-helper.c    | 5 +++--\n 8 files changed, 18 insertions(+), 25 deletions(-)\n\ndiff --git a/builtin/fast-export.c b/builtin/fast-export.c\nindex dbec4df92b..164406fdd4 100644\n--- a/builtin/fast-export.c\n+++ b/builtin/fast-export.c\n@@ -870,8 +870,7 @@ static void handle_tag(const char *name, struct tag *tag)\n \t\tprintf(\"reset %s\\nfrom %s\\n\\n\",\n \t\t       name, oid_to_hex(&null_oid));\n \t}\n-\tif (starts_with(name, \"refs/tags/\"))\n-\t\tname += 10;\n+\tskip_prefix(name, \"refs/tags/\", &name);\n \tprintf(\"tag %s\\n\", name);\n \tif (mark_tags) {\n \t\tmark_next_object(&tag->object);\ndiff --git a/builtin/reflog.c b/builtin/reflog.c\nindex 4d3430900d..81dfd563c0 100644\n--- a/builtin/reflog.c\n+++ b/builtin/reflog.c\n@@ -560,15 +560,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n \n \tfor (i = 1; i < argc; i++) {\n \t\tconst char *arg = argv[i];\n+\n \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n-\t\telse if (starts_with(arg, \"--expire=\")) {\n-\t\t\tif (parse_expiry_date(arg + 9, &cb.cmd.expire_total))\n+\t\telse if (skip_prefix(arg, \"--expire=\", &arg)) {\n+\t\t\tif (parse_expiry_date(arg, &cb.cmd.expire_total))\n \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n \t\t}\n-\t\telse if (starts_with(arg, \"--expire-unreachable=\")) {\n-\t\t\tif (parse_expiry_date(arg + 21, &cb.cmd.expire_unreachable))\n+\t\telse if (skip_prefix(arg, \"--expire-unreachable=\", &arg)) {\n+\t\t\tif (parse_expiry_date(arg, &cb.cmd.expire_unreachable))\n \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n \t\t\texplicit_expiry |= EXPIRE_UNREACH;\n \t\t}\ndiff --git a/notes.c b/notes.c\nindex 0c79964c26..a0349fa778 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1279,10 +1279,8 @@ static void format_note(struct notes_tree *t, const struct object_id *object_oid\n \t\tif (!ref || !strcmp(ref, GIT_NOTES_DEFAULT_REF)) {\n \t\t\tstrbuf_addstr(sb, \"\\nNotes:\\n\");\n \t\t} else {\n-\t\t\tif (starts_with(ref, \"refs/\"))\n-\t\t\t\tref += 5;\n-\t\t\tif (starts_with(ref, \"notes/\"))\n-\t\t\t\tref += 6;\n+\t\t\tskip_prefix(ref, \"refs/\", &ref);\n+\t\t\tskip_prefix(ref, \"notes/\", &ref);\n \t\t\tstrbuf_addf(sb, \"\\nNotes (%s):\\n\", ref);\n \t\t}\n \t}\ndiff --git a/parse-options.c b/parse-options.c\nindex 60fae3ad21..e8c04109ba 100644\n--- a/parse-options.c\n+++ b/parse-options.c\n@@ -357,8 +357,7 @@ static enum parse_opt_result parse_long_opt(\n \t\t\t}\n \t\t\t/* negated? */\n \t\t\tif (!starts_with(arg, \"no-\")) {\n-\t\t\t\tif (starts_with(long_name, \"no-\")) {\n-\t\t\t\t\tlong_name += 3;\n+\t\t\t\tif (skip_prefix(long_name, \"no-\", &long_name)) {\n \t\t\t\t\topt_flags |= OPT_UNSET;\n \t\t\t\t\tgoto again;\n \t\t\t\t}\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0ea66a28b6..561c33ac8a 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -465,8 +465,7 @@ static int files_read_raw_ref(struct ref_store *ref_store,\n \tclose(fd);\n \tstrbuf_rtrim(&sb_contents);\n \tbuf = sb_contents.buf;\n-\tif (starts_with(buf, \"ref:\")) {\n-\t\tbuf += 4;\n+\tif (skip_prefix(buf, \"ref:\", &buf)) {\n \t\twhile (isspace(*buf))\n \t\t\tbuf++;\n \ndiff --git a/remote-curl.c b/remote-curl.c\nindex 350d92a074..8eb96152f5 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -1255,8 +1255,9 @@ static void parse_push(struct strbuf *buf)\n \tint ret;\n \n \tdo {\n-\t\tif (starts_with(buf->buf, \"push \"))\n-\t\t\targv_array_push(&specs, buf->buf + 5);\n+\t\tconst char *arg;\n+\t\tif (skip_prefix(buf->buf, \"push \", &arg))\n+\t\t\targv_array_push(&specs, arg);\n \t\telse\n \t\t\tdie(_(\"http transport does not support %s\"), buf->buf);\n \ndiff --git a/sha1-name.c b/sha1-name.c\nindex 200eb373ad..75d1c32606 100644\n--- a/sha1-name.c\n+++ b/sha1-name.c\n@@ -908,14 +908,9 @@ static int get_oid_basic(struct repository *r, const char *str, int len,\n \t\t\t\treal_ref, flags, at_time, nth, oid, NULL,\n \t\t\t\t&co_time, &co_tz, &co_cnt)) {\n \t\t\tif (!len) {\n-\t\t\t\tif (starts_with(real_ref, \"refs/heads/\")) {\n-\t\t\t\t\tstr = real_ref + 11;\n-\t\t\t\t\tlen = strlen(real_ref + 11);\n-\t\t\t\t} else {\n-\t\t\t\t\t/* detached HEAD */\n+\t\t\t\tif (!skip_prefix(real_ref, \"refs/heads/\", &str))\n \t\t\t\t\tstr = \"HEAD\";\n-\t\t\t\t\tlen = 4;\n-\t\t\t\t}\n+\t\t\t\tlen = strlen(str);\n \t\t\t}\n \t\t\tif (at_time) {\n \t\t\t\tif (!(flags & GET_OID_QUIETLY)) {\ndiff --git a/transport-helper.c b/transport-helper.c\nindex 413d9d873e..20a7185ec4 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -404,11 +404,12 @@ static int fetch_with_fetch(struct transport *transport,\n \tsendline(data, &buf);\n \n \twhile (1) {\n+\t\tconst char *name;\n+\n \t\tif (recvline(data, &buf))\n \t\t\texit(128);\n \n-\t\tif (starts_with(buf.buf, \"lock \")) {\n-\t\t\tconst char *name = buf.buf + 5;\n+\t\tif (skip_prefix(buf.buf, \"lock \", &name)) {\n \t\t\tif (transport->pack_lockfile)\n \t\t\t\twarning(_(\"%s also locked %s\"), data->name, name);\n \t\t\telse\n-- \n2.25.0-299-g5579215f86\n\n"},{"id":"390878","messageId":"20200131001907.GD2394563@coredump.intra.peff.net","threadId":"52705","inReplyTo":"xmqqa764d9f1.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] C: use skip-prefix to avoid hardcoded string length","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-01-31T00:19:07Z","receivedAt":"2020-01-31T00:19:10Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jan 30, 2020 at 11:35:46AM -0800, Junio C Hamano wrote:\n\n>  * v2 uses a simpler conversion that has a side effect of changing\n>    the error message from \"git reflog expire\" in a better way.\n\nThanks, this all looks good to me.\n\n> diff --git a/builtin/reflog.c b/builtin/reflog.c\n> index 4d3430900d..81dfd563c0 100644\n> --- a/builtin/reflog.c\n> +++ b/builtin/reflog.c\n> @@ -560,15 +560,16 @@ static int cmd_reflog_expire(int argc, const char **argv, const char *prefix)\n>  \n>  \tfor (i = 1; i < argc; i++) {\n>  \t\tconst char *arg = argv[i];\n> +\n>  \t\tif (!strcmp(arg, \"--dry-run\") || !strcmp(arg, \"-n\"))\n>  \t\t\tflags |= EXPIRE_REFLOGS_DRY_RUN;\n> -\t\telse if (starts_with(arg, \"--expire=\")) {\n> -\t\t\tif (parse_expiry_date(arg + 9, &cb.cmd.expire_total))\n> +\t\telse if (skip_prefix(arg, \"--expire=\", &arg)) {\n> +\t\t\tif (parse_expiry_date(arg, &cb.cmd.expire_total))\n>  \t\t\t\tdie(_(\"'%s' is not a valid timestamp\"), arg);\n>  \t\t\texplicit_expiry |= EXPIRE_TOTAL;\n>  \t\t}\n\nI'm not sure if you meant to leave the blank-line addition, or if it was\na leftover from when there was a variable added there in v1. I think the\nresult is more readable, though, so I certainly don't mind it. ;)\n\n-Peff\n"},{"id":"390940","messageId":"CACayv=hcH=dvfup2fzm5-OKYxEyC1m7YBTfpm=6z0BhO=hK-cQ@mail.gmail.com","threadId":"52705","inReplyTo":"xmqqa764d9f1.fsf_-_@gitster-ct.c.googlers.com","subject":"Re: [PATCH v2] C: use skip-prefix to avoid hardcoded string length","fromName":"Andrei Rybak","fromEmail":"rybak.a.v@gmail.com","sentAt":"2020-01-31T20:15:14Z","receivedAt":"2020-01-31T20:15:35Z","isPatch":true,"sender":{"key":"rybak.a.v@gmail.com","avatar":"https://avatars.githubusercontent.com/u/624072?v=4"},"body":"On 1/30/20 8:35 PM, Junio C Hamano wrote:\n> We often skip an optional prefix in a string with a hardcoded\n> constant, e.g.\n>\n>       if (starts_with(string, \"prefix\"))\n>               string += 6;\n>\n> which is less error prone when written\n>\n>       skip_prefix(string, \"prefix\", &string);\n\nShould the subject line say \"skip_prefix\" with an underscore instead of\n\"skip-prefix\" with a dash?\n"},{"id":"390962","messageId":"xmqq36bv9w4f.fsf@gitster-ct.c.googlers.com","threadId":"52705","inReplyTo":"CACayv=hcH=dvfup2fzm5-OKYxEyC1m7YBTfpm=6z0BhO=hK-cQ@mail.gmail.com","subject":"Re: [PATCH v2] C: use skip-prefix to avoid hardcoded string length","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-01-31T21:03:28Z","receivedAt":"2020-01-31T21:03:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Andrei Rybak <rybak.a.v@gmail.com> writes:\n\n> On 1/30/20 8:35 PM, Junio C Hamano wrote:\n>> We often skip an optional prefix in a string with a hardcoded\n>> constant, e.g.\n>>\n>>       if (starts_with(string, \"prefix\"))\n>>               string += 6;\n>>\n>> which is less error prone when written\n>>\n>>       skip_prefix(string, \"prefix\", &string);\n>\n> Should the subject line say \"skip_prefix\" with an underscore instead of\n> \"skip-prefix\" with a dash?\n\nPerhaps.  Or \"skip_prefix()\" to clarify that we are talking about a\nfunction.\n"}]}