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 writes: > On Sat, Dec 21, 2013 at 4:31 AM, Junio C Hamano wrote: >> Jeff King 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 Sat, Dec 21, 2013 at 4:31 AM, Junio C Hamano wrote: > Jeff King 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
Jeff King 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
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;
+
+
Re: [PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
On Fri, Dec 20, 2013 at 8:04 AM, Jeff King 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
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
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
> ---
> 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
[PATCH 02/12] Convert starts_with() to skip_prefix() for option parsing
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
---
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;
continue;
}
- if (starts_with(arg, "--exec=")) {
- args.uploadpack = arg + 7;
+ if ((optarg = skip_prefix(arg, "--exec=")) != NULL) {
+ args.uploadpack = optarg;
continue;
}
if (!strcmp("--quiet", arg) || !strcmp("-q", arg)) {
@@ -89,8 +90,8 @@ int cmd_fetch_pack(int argc, const char **argv, const char
*prefix)
args.verbose = 1;
continue;
}
- if (starts_with(arg, "--depth=")) {
- args.depth = strtol(arg + 8, NULL, 0);
+ if ((optarg = skip_prefix(arg, "--depth=")) != NULL) {
+ args.depth = strtol(optarg, NULL, 0);
continue;
}
if (!strcmp("--no-progress", arg)) {
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 2f37a38..67eff7a 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1511,6 +1511,7 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
for (i = 1; i < argc; i++) {
const char *arg = argv[i];
+ const char *optarg;
if (*arg == '-') {
if (!strcmp(arg, "--stdin")) {
@@ -1534,11 +1535,11 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
stat_only = 1;
} else if (!strcmp(arg, "--keep")) {
keep_msg = "";
- } else if (starts_with(arg, "--keep=")) {
- keep_msg = arg + 7;
- } else if (starts_with(arg, "--threads=")) {
+ } else if ((optarg = skip_prefix(arg, "--keep=")) !=
NULL) {
+ keep_msg = optarg;
+ } else if ((optarg = skip_prefix(arg, "--threads=")) !=
NULL) {
char *end;
- nr_threads = strtoul(arg+10, &end, 0);
+ nr_threads = strtoul(optarg, &end, 0);
if (!arg[10] || *end || nr_threads < 0)
usage(index_pack_usage);
#ifdef NO_PTHREADS
@@ -1547,13 +1548,13 @@ int cmd_index_pack(int argc, const char **argv, const
char *prefix)
"ignoring %s"), arg);
nr_threads = 1;
#endif
- } else if (starts_with(arg, "--pack_header=")) {
+ } else if ((optarg = skip_prefix(arg,
"--pack_header=")) != NULL) {
struct pack_header *hdr;
char *c;
hdr = (struct pack_header *)input_buffer;
hdr->hdr_signature = htonl(PACK_SIGNATURE);
- hdr->hdr_version = htonl(strtoul(arg + 14, &c,
10));
+ hdr->hdr_version = htonl(strtoul(optarg, &c,
10));
if (*c != ',')
die(_("bad %s"), arg);
hdr->hdr_entries = htonl(strtoul(c + 1, &c,
10));
@@ -1566,9 +1567,9
