Re: [PATCH 08/16] use skip_prefix to avoid magic numbers

[Jeff King]

On Mon, Jun 23, 2014 at 02:44:23PM -0700, Junio C Hamano wrote:

 Jeff King p...@peff.net writes:
 
  diff --git a/connect.c b/connect.c
  index 94a6650..37ff018 100644
  --- a/connect.c
  +++ b/connect.c
  @@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf, 
  size_t src_len,
  if (!len)
  break;
   
  -   if (len  4  starts_with(buffer, ERR ))
  -   die(remote error: %s, buffer + 4);
  +   if (len  4  skip_prefix(buffer, ERR , arg))
  +   die(remote error: %s, arg);
 
 Makes one wonder if we should do something special to a line with
 only ERR  and nothing else on it, which the other end may have
 meant us to give a blank line to make the output more readable.

I don't think that would buy us much. We have always accepted only a
single ERR line and died immediately. So any changes of that nature
would have to be made in the client, and then servers would have to wait
N time units before it was safe to start using the feature (otherwise
old clients just get the blank line!).

I also don't think blank lines by themselves are useful. You'd want them
in addition to being able to handle multiple lines. So a nicer fix is
more along the lines of accept multiple ERR lines, including blank
lines, followed by a terminating line (ERRDONE or something).

Then servers can do:

  ERR unable to access foo.git: Printer on fire
  ERR
  ERR You may have misspelled the repository name. Did you mean:
  ERR
  ERR  foobar.git
  ERRDONE

Old clients would see the first line and die. Newer clients would print
the helpful hint. Servers would just need to make sure that the first
line stands on its own to cover both cases.

 A fix, if one turns out to be needed, is outside the scope of this
 patch, though, I think.

Yeah, definitely a separate topic.

It is not something I think anybody has asked for, but I can imagine a
site like GitHub making use of it (we already show custom errors for
http, but there's no room beyond the single ERR line). And teaching the
clients now expands the options for servers later. So it might be worth
doing just as a potential feature.

-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 08/16] use skip_prefix to avoid magic numbers

[Junio C Hamano]

Jeff King p...@peff.net writes:

 diff --git a/connect.c b/connect.c
 index 94a6650..37ff018 100644
 --- a/connect.c
 +++ b/connect.c
 @@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf, 
 size_t src_len,
   if (!len)
   break;
  
 - if (len  4  starts_with(buffer, ERR ))
 - die(remote error: %s, buffer + 4);
 + if (len  4  skip_prefix(buffer, ERR , arg))
 + die(remote error: %s, arg);

Makes one wonder if we should do something special to a line with
only ERR  and nothing else on it, which the other end may have
meant us to give a blank line to make the output more readable.

A fix, if one turns out to be needed, is outside the scope of this
patch, though, I think.
--
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 08/16] use skip_prefix to avoid magic numbers

[Jeff King]

It's a common idiom to match a prefix and then skip past it
with a magic number, like:

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

This is easy to get wrong, since you have to count the
prefix string yourself, and there's no compiler check if the
string changes.  We can use skip_prefix to avoid the magic
numbers here.

Note that some of these conversions could be much shorter.
For example:

  if (starts_with(arg, --foo=)) {
  bar = arg + 6;
  continue;
  }

could become:

  if (skip_prefix(arg, --foo=, bar))
  continue;

However, I have left it as:

  if (skip_prefix(arg, --foo=, v)) {
  bar = v;
  continue;
  }

to visually match nearby cases which need to actually
process the string. Like:

  if (skip_prefix(arg, --foo=, v)) {
  bar = atoi(v);
  continue;
  }

Signed-off-by: Jeff King p...@peff.net
---
 alias.c|  3 ++-
 connect.c  | 11 +
 convert.c  |  4 ++--
 daemon.c   | 73 ++
 diff.c | 65 ++-
 fast-import.c  | 69 +-
 fetch-pack.c   |  9 
 git.c  | 18 +++
 help.c |  6 +++--
 http-backend.c | 11 +
 http-push.c| 11 +
 11 files changed, 149 insertions(+), 131 deletions(-)

diff --git a/alias.c b/alias.c
index 5efc3d6..758c867 100644
--- a/alias.c
+++ b/alias.c
@@ -5,7 +5,8 @@ static char *alias_val;
 
 static int alias_lookup_cb(const char *k, const char *v, void *cb)
 {
-   if (starts_with(k, alias.)  !strcmp(k + 6, alias_key)) {
+   const char *name;
+   if (skip_prefix(k, alias., name)  !strcmp(name, alias_key)) {
if (!v)
return config_error_nonbool(k);
alias_val = xstrdup(v);
diff --git a/connect.c b/connect.c
index 94a6650..37ff018 100644
--- a/connect.c
+++ b/connect.c
@@ -129,6 +129,7 @@ struct ref **get_remote_heads(int in, char *src_buf, size_t 
src_len,
char *name;
int len, name_len;
char *buffer = packet_buffer;
+   const char *arg;
 
len = packet_read(in, src_buf, src_len,
  packet_buffer, sizeof(packet_buffer),
@@ -140,12 +141,12 @@ struct ref **get_remote_heads(int in, char *src_buf, 
size_t src_len,
if (!len)
break;
 
-   if (len  4  starts_with(buffer, ERR ))
-   die(remote error: %s, buffer + 4);
+   if (len  4  skip_prefix(buffer, ERR , arg))
+   die(remote error: %s, arg);
 
-   if (len == 48  starts_with(buffer, shallow )) {
-   if (get_sha1_hex(buffer + 8, old_sha1))
-   die(protocol error: expected shallow sha-1, 
got '%s', buffer + 8);
+   if (len == 48  skip_prefix(buffer, shallow , arg)) {
+   if (get_sha1_hex(arg, old_sha1))
+   die(protocol error: expected shallow sha-1, 
got '%s', arg);
if (!shallow_points)
die(repository on the other end cannot be 
shallow);
sha1_array_append(shallow_points, old_sha1);
diff --git a/convert.c b/convert.c
index ab80b72..cb5fbb4 100644
--- a/convert.c
+++ b/convert.c
@@ -1121,9 +1121,9 @@ static int is_foreign_ident(const char *str)
 {
int i;
 
-   if (!starts_with(str, $Id: ))
+   if (!skip_prefix(str, $Id: , str))
return 0;
-   for (i = 5; str[i]; i++) {
+   for (i = 0; str[i]; i++) {
if (isspace(str[i])  str[i+1] != '$')
return 1;
}
diff --git a/daemon.c b/daemon.c
index 18818c3..6d25828 100644
--- a/daemon.c
+++ b/daemon.c
@@ -235,8 +235,10 @@ static int service_enabled;
 
 static int git_daemon_config(const char *var, const char *value, void *cb)
 {
-   if (starts_with(var, daemon.) 
-   !strcmp(var + 7, service_looking_at-config_name)) {
+   const char *service;
+
+   if (skip_prefix(var, daemon., service) 
+   !strcmp(service, service_looking_at-config_name)) {
service_enabled = git_config_bool(var, value);
return 0;
}
@@ -1133,16 +1135,17 @@ int main(int argc, char **argv)
 
for (i = 1; i  argc; i++) {
char *arg = argv[i];
+   const char *v;
 
-   if (starts_with(arg, --listen=)) {
-   string_list_append(listen_addr, xstrdup_tolower(arg + 
9));
+   if (skip_prefix(arg, --listen=, v)) {
+   string_list_append(listen_addr, xstrdup_tolower(v));
continue;
}
-   if (starts_with(arg, --port=)) {
+   if (skip_prefix(arg, --port=, v)) {