Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
On Thu, Dec 26, 2013 at 11:27:10AM -0800, Junio C Hamano wrote: I still need consensus on the name here guys, parse_prefix. remove_prefix or strip_prefix? If no other opinions i'll go with strip_prefix (Jeff's comment before parse_prefix() also uses strip) Yup, that comment is where I took strip from. When you name your thing as X, using too generic a word X, and then need to explain what X does using a bit more specific word Y, you are often better off naming it after Y. FWIW, the reason I shied away from strip is that I did not want to imply that the function mutates the string. But since nobody else seems concerned with that, I think strip is fine. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
Duy Nguyen pclo...@gmail.com writes: On Sat, Dec 21, 2013 at 4:31 AM, Junio C Hamano gits...@pobox.com wrote: Jeff King p...@peff.net writes: /* here we care if we saw the prefix, as above */ if (parse_prefix(foo, prefix, the_rest)) ... /* * and here we do not care, and just want to optionally strip the * prefix, and take the full value otherwise; we just have to ignore * the return value in this case. */ parse_prefix(foo, prefix, foo); Sounds fine. I recall earlier somebody wanting to have a good name for this thing, and I think foo_gently is *not* it (the name is about adding a variant that does not die outright to foo that checks and dies if condition is not right). starts_with(foo, prefix); strip_prefix(foo, prefix, foo); perhaps? I still need consensus on the name here guys, parse_prefix. remove_prefix or strip_prefix? If no other opinions i'll go with strip_prefix (Jeff's comment before parse_prefix() also uses strip) Yup, that comment is where I took strip from. When you name your thing as X, using too generic a word X, and then need to explain what X does using a bit more specific word Y, you are often better off naming it after Y. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
On Fri, Dec 20, 2013 at 8:04 AM, Jeff King p...@peff.net wrote:
On Fri, Dec 20, 2013 at 07:51:00AM +0100, Johannes Sixt wrote:
for (i = 1; i argc *argv[i] == '-'; i++) {
const char *arg = argv[i];
+ const char *optarg;
- if (starts_with(arg, --upload-pack=)) {
- args.uploadpack = arg + 14;
+ if ((optarg = skip_prefix(arg, --upload-pack=)) != NULL) {
+ args.uploadpack = optarg;
Quite frankly, I do not think this is an improvement. The old code is
*MUCH* easier to understand because starts_with is clearly a predicate
that is either true or false, but the code with skip_prefix is much
heavier on the eye with its extra level of parenthesis. That it removes a
hard-coded constant does not count much IMHO because it is very clear
where the value comes from.
Yeah, I agree that is unfortunate.
I agree too.
Maybe we could have the best of both
worlds, like:
if (starts_with(arg, --upload-pack=, optarg))
... use optarg ...
Probably we do not want to call it just starts_with, as quite a few
callers to do not care about what comes next, and would just pass NULL.
I cannot seem to think of a good name, though, as the with means that
obvious things like starts_with_value naturally parse as a single
(nonsensical) sentence. Something like parse_prefix would work, but
it is not as clearly a predicate as starts_with (but we have at least
gotten rid of the extra parentheses).
Elsewhere in the thread, the concept was discussed of returning the full
string to mean did not match, which makes some other idioms simpler
(but IMHO makes the simple cases like this even harder to read). My
proposal splits the start of string out parameter from the boolean
return, so it handles both cases naturally:
/* here we care if we saw the prefix, as above */
if (parse_prefix(foo, prefix, the_rest))
...
/*
* and here we do not care, and just want to optionally strip the
* prefix, and take the full value otherwise; we just have to ignore
* the return value in this case.
*/
parse_prefix(foo, prefix, foo);
Yeah, I agree that the function signature you suggest is better, but I
like the skip_prefix name better.
Or perhaps remove_prefix?
Thanks,
Christian.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
Am 20.12.2013 08:04, schrieb Jeff King:
On Fri, Dec 20, 2013 at 07:51:00AM +0100, Johannes Sixt wrote:
for (i = 1; i argc *argv[i] == '-'; i++) {
const char *arg = argv[i];
+ const char *optarg;
- if (starts_with(arg, --upload-pack=)) {
- args.uploadpack = arg + 14;
+ if ((optarg = skip_prefix(arg, --upload-pack=)) != NULL) {
+ args.uploadpack = optarg;
Quite frankly, I do not think this is an improvement. The old code is
*MUCH* easier to understand because starts_with is clearly a predicate
that is either true or false, but the code with skip_prefix is much
heavier on the eye with its extra level of parenthesis. That it removes a
hard-coded constant does not count much IMHO because it is very clear
where the value comes from.
Yeah, I agree that is unfortunate. Maybe we could have the best of both
worlds, like:
if (starts_with(arg, --upload-pack=, optarg))
... use optarg ...
Probably we do not want to call it just starts_with, as quite a few
callers to do not care about what comes next, and would just pass NULL.
I cannot seem to think of a good name, though, as the with means that
obvious things like starts_with_value naturally parse as a single
(nonsensical) sentence. Something like parse_prefix would work, but
it is not as clearly a predicate as starts_with (but we have at least
gotten rid of the extra parentheses).
Elsewhere in the thread, the concept was discussed of returning the full
string to mean did not match, which makes some other idioms simpler
(but IMHO makes the simple cases like this even harder to read). My
proposal splits the start of string out parameter from the boolean
return, so it handles both cases naturally:
/* here we care if we saw the prefix, as above */
if (parse_prefix(foo, prefix, the_rest))
...
/*
* and here we do not care, and just want to optionally strip the
* prefix, and take the full value otherwise; we just have to ignore
* the return value in this case.
*/
parse_prefix(foo, prefix, foo);
It adds a bit of redundancy, but overall I like it. It fits the common
case very well and looks nice. The patch below converts all calls of
skip_prefix as well as the usage of starts_with and a magic number in
builtin/fetch-pack.c.
I wonder how many of the 400+ uses of starts_with remain after a
parse_prefix crusade. If only a few remain then it may make sense
to unite the two functions under a common name.
---
advice.c | 5 -
builtin/branch.c | 6 +++---
builtin/clone.c| 13 -
builtin/commit.c | 6 ++
builtin/fetch-pack.c | 14 +++---
builtin/fmt-merge-msg.c| 4 ++--
builtin/push.c | 7 +++
builtin/remote.c | 4 +---
column.c | 5 +++--
config.c | 3 +--
credential-cache--daemon.c | 6 ++
credential.c | 3 +--
git-compat-util.h | 1 +
parse-options.c| 11 ++-
strbuf.c | 12 +---
transport.c| 6 +-
urlmatch.c | 3 +--
17 files changed, 59 insertions(+), 50 deletions(-)
diff --git a/advice.c b/advice.c
index 3eca9f5..75fae9c 100644
--- a/advice.c
+++ b/advice.c
@@ -63,9 +63,12 @@ void advise(const char *advice, ...)
int git_default_advice_config(const char *var, const char *value)
{
- const char *k = skip_prefix(var, advice.);
+ const char *k;
int i;
+ if (!parse_prefix(var, advice., k))
+ return 0;
+
for (i = 0; i ARRAY_SIZE(advice_config); i++) {
if (strcmp(k, advice_config[i].name))
continue;
diff --git a/builtin/branch.c b/builtin/branch.c
index b4d7716..dae0d82 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -294,13 +294,13 @@ static char *resolve_symref(const char *src, const char
*prefix)
{
unsigned char sha1[20];
int flag;
- const char *dst, *cp;
+ const char *dst;
dst = resolve_ref_unsafe(src, sha1, 0, flag);
if (!(dst (flag REF_ISSYMREF)))
return NULL;
- if (prefix (cp = skip_prefix(dst, prefix)))
- dst = cp;
+ if (prefix)
+ parse_prefix(dst, prefix, dst);
return xstrdup(dst);
}
diff --git a/builtin/clone.c b/builtin/clone.c
index f98f529..e62fa26 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -578,11 +578,12 @@ static void update_remote_refs(const struct ref *refs,
static void update_head(const struct ref *our, const struct ref *remote,
const char *msg)
{
- if (our starts_with(our-name, refs/heads/)) {
+ const char *head;
+
+ if (our parse_prefix(our-name, refs/heads/, head)) {
/* Local default branch link */
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
Jeff King p...@peff.net writes: /* here we care if we saw the prefix, as above */ if (parse_prefix(foo, prefix, the_rest)) ... /* * and here we do not care, and just want to optionally strip the * prefix, and take the full value otherwise; we just have to ignore * the return value in this case. */ parse_prefix(foo, prefix, foo); Sounds fine. I recall earlier somebody wanting to have a good name for this thing, and I think foo_gently is *not* it (the name is about adding a variant that does not die outright to foo that checks and dies if condition is not right). starts_with(foo, prefix); strip_prefix(foo, prefix, foo); perhaps? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
On Sat, Dec 21, 2013 at 4:31 AM, Junio C Hamano gits...@pobox.com wrote: Jeff King p...@peff.net writes: /* here we care if we saw the prefix, as above */ if (parse_prefix(foo, prefix, the_rest)) ... /* * and here we do not care, and just want to optionally strip the * prefix, and take the full value otherwise; we just have to ignore * the return value in this case. */ parse_prefix(foo, prefix, foo); Sounds fine. I recall earlier somebody wanting to have a good name for this thing, and I think foo_gently is *not* it (the name is about adding a variant that does not die outright to foo that checks and dies if condition is not right). starts_with(foo, prefix); strip_prefix(foo, prefix, foo); perhaps? I still need consensus on the name here guys, parse_prefix. remove_prefix or strip_prefix? If no other opinions i'll go with strip_prefix (Jeff's comment before parse_prefix() also uses strip) -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
Am 12/18/2013 15:53, schrieb Nguyễn Thái Ngọc Duy:
The code that's not converted to use parse_options() often does
if (!starts_with(arg, foo=)) {
value = atoi(arg + 4);
}
This patch removes those magic numbers with skip_prefix()
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
builtin/fetch-pack.c | 13 +
builtin/index-pack.c | 17 +--
builtin/ls-remote.c | 9 +++---
builtin/mailinfo.c | 5 ++--
builtin/reflog.c | 9 +++---
builtin/rev-parse.c | 41 +-
builtin/send-pack.c | 18 ++--
builtin/unpack-objects.c | 5 ++--
builtin/update-ref.c | 21 +++---
daemon.c | 75
diff.c | 49 +++
git.c| 13 +
merge-recursive.c| 13 +
revision.c | 60 +++---
upload-pack.c| 5 ++--
15 files changed, 182 insertions(+), 171 deletions(-)
diff --git a/builtin/fetch-pack.c b/builtin/fetch-pack.c
index 8b8978a2..2df1423 100644
--- a/builtin/fetch-pack.c
+++ b/builtin/fetch-pack.c
@@ -47,13 +47,14 @@ int cmd_fetch_pack(int argc, const char **argv, const
char *prefix)
for (i = 1; i argc *argv[i] == '-'; i++) {
const char *arg = argv[i];
+ const char *optarg;
- if (starts_with(arg, --upload-pack=)) {
- args.uploadpack = arg + 14;
+ if ((optarg = skip_prefix(arg, --upload-pack=)) != NULL) {
+ args.uploadpack = optarg;
Quite frankly, I do not think this is an improvement. The old code is
*MUCH* easier to understand because starts_with is clearly a predicate
that is either true or false, but the code with skip_prefix is much
heavier on the eye with its extra level of parenthesis. That it removes a
hard-coded constant does not count much IMHO because it is very clear
where the value comes from.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
On Fri, Dec 20, 2013 at 07:51:00AM +0100, Johannes Sixt wrote:
for (i = 1; i argc *argv[i] == '-'; i++) {
const char *arg = argv[i];
+ const char *optarg;
- if (starts_with(arg, --upload-pack=)) {
- args.uploadpack = arg + 14;
+ if ((optarg = skip_prefix(arg, --upload-pack=)) != NULL) {
+ args.uploadpack = optarg;
Quite frankly, I do not think this is an improvement. The old code is
*MUCH* easier to understand because starts_with is clearly a predicate
that is either true or false, but the code with skip_prefix is much
heavier on the eye with its extra level of parenthesis. That it removes a
hard-coded constant does not count much IMHO because it is very clear
where the value comes from.
Yeah, I agree that is unfortunate. Maybe we could have the best of both
worlds, like:
if (starts_with(arg, --upload-pack=, optarg))
... use optarg ...
Probably we do not want to call it just starts_with, as quite a few
callers to do not care about what comes next, and would just pass NULL.
I cannot seem to think of a good name, though, as the with means that
obvious things like starts_with_value naturally parse as a single
(nonsensical) sentence. Something like parse_prefix would work, but
it is not as clearly a predicate as starts_with (but we have at least
gotten rid of the extra parentheses).
Elsewhere in the thread, the concept was discussed of returning the full
string to mean did not match, which makes some other idioms simpler
(but IMHO makes the simple cases like this even harder to read). My
proposal splits the start of string out parameter from the boolean
return, so it handles both cases naturally:
/* here we care if we saw the prefix, as above */
if (parse_prefix(foo, prefix, the_rest))
...
/*
* and here we do not care, and just want to optionally strip the
* prefix, and take the full value otherwise; we just have to ignore
* the return value in this case.
*/
parse_prefix(foo, prefix, foo);
-Peff
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
