Re: [PATCH 1/4] remote: use skip_prefix

[Thomas Gummerer]

On 02/15, Jeff King wrote:
> On Mon, Feb 15, 2016 at 06:42:27PM +0100, Thomas Gummerer wrote:
>
> > 95b567c7 ("use skip_prefix to avoid repeating strings") transformed
> > calls using starts_with() and then skipping the length of the prefix to
> > skip_prefix() calls.  In remote.c there are a few calls like:
> >
> >   if (starts_with(foo, "bar"))
> >   foo += 3
> >
> > These calls weren't touched by the commit mentioned above, but can
> > benefit from the same treatment to avoid magic numbers.
>
> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:

Thanks, I had no idea about this function :) It makes the diff a lot
noisier, but I do think the end result is better.

> diff --git a/remote.c b/remote.c
> index 21e4ec3..8d2c3ca 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -318,15 +318,14 @@ static void read_branches_file(struct remote *remote)
>  static int handle_config(const char *key, const char *value, void *cb)
>  {
>   const char *name;
> + int namelen;
>   const char *subkey;
>   struct remote *remote;
>   struct branch *branch;
> - if (starts_with(key, "branch.")) {
> - name = key + 7;
> - subkey = strrchr(name, '.');
> - if (!subkey)
> + if (starts_with(key, "branch", , , )) {
> + if (!name)
>   return 0;
> - branch = make_branch(name, subkey - name);
> + branch = make_branch(name, namelen);
>   if (!strcmp(subkey, ".remote")) {
>   return git_config_string(>remote_name, key, 
> value);
>   } else if (!strcmp(subkey, ".pushremote")) {
>
> and so on. The difference in lines of code isn't that great, but I think
> it makes the resulting code more obvious to read.
>
> -Peff

--
Thomas
--
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 1/4] remote: use skip_prefix

[Jeff King]

On Mon, Feb 15, 2016 at 01:35:03PM -0500, Eric Sunshine wrote:

> On Mon, Feb 15, 2016 at 1:18 PM, Jeff King  wrote:
> > This is definitely an improvement, but I think we can actually go a step
> > further here, and use parse_config_key. Like:
> >
> > -   if (starts_with(key, "branch.")) {
> > -   name = key + 7;
> > -   subkey = strrchr(name, '.');
> > -   if (!subkey)
> > +   if (starts_with(key, "branch", , , )) {
> 
> I guess you meant: s/starts_with/parse_config_key/

Whoops, yeah. I'll belatedly add a "not even compile-tested" disclaimer.
:)

-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 1/4] remote: use skip_prefix

[Eric Sunshine]

On Mon, Feb 15, 2016 at 1:18 PM, Jeff King  wrote:
> This is definitely an improvement, but I think we can actually go a step
> further here, and use parse_config_key. Like:
>
> -   if (starts_with(key, "branch.")) {
> -   name = key + 7;
> -   subkey = strrchr(name, '.');
> -   if (!subkey)
> +   if (starts_with(key, "branch", , , )) {

I guess you meant: s/starts_with/parse_config_key/

> +   if (!name)
> return 0;
> -   branch = make_branch(name, subkey - name);
> +   branch = make_branch(name, namelen);
> if (!strcmp(subkey, ".remote")) {
> return git_config_string(>remote_name, key, 
> value);
> } else if (!strcmp(subkey, ".pushremote")) {
--
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 1/4] remote: use skip_prefix

[Jeff King]

On Mon, Feb 15, 2016 at 06:42:27PM +0100, Thomas Gummerer wrote:

> 95b567c7 ("use skip_prefix to avoid repeating strings") transformed
> calls using starts_with() and then skipping the length of the prefix to
> skip_prefix() calls.  In remote.c there are a few calls like:
> 
>   if (starts_with(foo, "bar"))
>   foo += 3
> 
> These calls weren't touched by the commit mentioned above, but can
> benefit from the same treatment to avoid magic numbers.

This is definitely an improvement, but I think we can actually go a step
further here, and use parse_config_key. Like:

diff --git a/remote.c b/remote.c
index 21e4ec3..8d2c3ca 100644
--- a/remote.c
+++ b/remote.c
@@ -318,15 +318,14 @@ static void read_branches_file(struct remote *remote)
 static int handle_config(const char *key, const char *value, void *cb)
 {
const char *name;
+   int namelen;
const char *subkey;
struct remote *remote;
struct branch *branch;
-   if (starts_with(key, "branch.")) {
-   name = key + 7;
-   subkey = strrchr(name, '.');
-   if (!subkey)
+   if (starts_with(key, "branch", , , )) {
+   if (!name)
return 0;
-   branch = make_branch(name, subkey - name);
+   branch = make_branch(name, namelen);
if (!strcmp(subkey, ".remote")) {
return git_config_string(>remote_name, key, 
value);
} else if (!strcmp(subkey, ".pushremote")) {

and so on. The difference in lines of code isn't that great, but I think
it makes the resulting code more obvious to read.

-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

[PATCH 1/4] remote: use skip_prefix

[Thomas Gummerer]

95b567c7 ("use skip_prefix to avoid repeating strings") transformed
calls using starts_with() and then skipping the length of the prefix to
skip_prefix() calls.  In remote.c there are a few calls like:

  if (starts_with(foo, "bar"))
  foo += 3

These calls weren't touched by the commit mentioned above, but can
benefit from the same treatment to avoid magic numbers.

Signed-off-by: Thomas Gummerer 
---
 remote.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/remote.c b/remote.c
index 02e698a..4b5b576 100644
--- a/remote.c
+++ b/remote.c
@@ -321,8 +321,7 @@ static int handle_config(const char *key, const char 
*value, void *cb)
const char *subkey;
struct remote *remote;
struct branch *branch;
-   if (starts_with(key, "branch.")) {
-   name = key + 7;
+   if (skip_prefix(key, "branch.", )) {
subkey = strrchr(name, '.');
if (!subkey)
return 0;
@@ -338,9 +337,8 @@ static int handle_config(const char *key, const char 
*value, void *cb)
}
return 0;
}
-   if (starts_with(key, "url.")) {
+   if (skip_prefix(key, "url.", )) {
struct rewrite *rewrite;
-   name = key + 4;
subkey = strrchr(name, '.');
if (!subkey)
return 0;
@@ -357,9 +355,8 @@ static int handle_config(const char *key, const char 
*value, void *cb)
}
}
 
-   if (!starts_with(key,  "remote."))
+   if (!skip_prefix(key, "remote.", ))
return 0;
-   name = key + 7;
 
/* Handle remote.* variables */
if (!strcmp(name, "pushdefault"))
-- 
2.7.1.410.g6faf27b

--
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