Skip to content

Commit

Permalink
use skip_prefix to avoid magic numbers
Browse files Browse the repository at this point in the history 
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 <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
Jeff King authored and Junio C Hamano committed Jun 20, 2014
1 parent 21a2d4a commit ae021d8
Show file tree
Hide file tree
Showing 11 changed files with 149 additions and 131 deletions.
3 changes: 2 additions & 1 deletion alias.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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);
Expand Down
11 changes: 6 additions & 5 deletions connect.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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),
Expand All @@ -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);
Expand Down
4 changes: 2 additions & 2 deletions convert.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down
73 changes: 38 additions & 35 deletions daemon.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -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;
}
Expand Down Expand Up @@ -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)) {
char *end;
unsigned long n;
n = strtoul(arg+7, &end, 0);
if (arg[7] && !*end) {
n = strtoul(v, &end, 0);
if (*v && !*end) {
listen_port = n;
continue;
}
Expand All @@ -1168,20 +1171,20 @@ int main(int argc, char **argv)
export_all_trees = 1;
continue;
}
if (starts_with(arg, "--access-hook=")) {
access_hook = arg + 14;
if (skip_prefix(arg, "--access-hook=", &v)) {
access_hook = v;
continue;
}
if (starts_with(arg, "--timeout=")) {
timeout = atoi(arg+10);
if (skip_prefix(arg, "--timeout=", &v)) {
timeout = atoi(v);
continue;
}
if (starts_with(arg, "--init-timeout=")) {
init_timeout = atoi(arg+15);
if (skip_prefix(arg, "--init-timeout=", &v)) {
init_timeout = atoi(v);
continue;
}
if (starts_with(arg, "--max-connections=")) {
max_connections = atoi(arg+18);
if (skip_prefix(arg, "--max-connections=", &v)) {
max_connections = atoi(v);
if (max_connections < 0)
max_connections = 0; /* unlimited */
continue;
Expand All @@ -1190,16 +1193,16 @@ int main(int argc, char **argv)
strict_paths = 1;
continue;
}
if (starts_with(arg, "--base-path=")) {
base_path = arg+12;
if (skip_prefix(arg, "--base-path=", &v)) {
base_path = v;
continue;
}
if (!strcmp(arg, "--base-path-relaxed")) {
base_path_relaxed = 1;
continue;
}
if (starts_with(arg, "--interpolated-path=")) {
interpolated_path = arg+20;
if (skip_prefix(arg, "--interpolated-path=", &v)) {
interpolated_path = v;
continue;
}
if (!strcmp(arg, "--reuseaddr")) {
Expand All @@ -1210,41 +1213,41 @@ int main(int argc, char **argv)
user_path = "";
continue;
}
if (starts_with(arg, "--user-path=")) {
user_path = arg + 12;
if (skip_prefix(arg, "--user-path=", &v)) {
user_path = v;
continue;
}
if (starts_with(arg, "--pid-file=")) {
pid_file = arg + 11;
if (skip_prefix(arg, "--pid-file=", &v)) {
pid_file = v;
continue;
}
if (!strcmp(arg, "--detach")) {
detach = 1;
log_syslog = 1;
continue;
}
if (starts_with(arg, "--user=")) {
user_name = arg + 7;
if (skip_prefix(arg, "--user=", &v)) {
user_name = v;
continue;
}
if (starts_with(arg, "--group=")) {
group_name = arg + 8;
if (skip_prefix(arg, "--group=", &v)) {
group_name = v;
continue;
}
if (starts_with(arg, "--enable=")) {
enable_service(arg + 9, 1);
if (skip_prefix(arg, "--enable=", &v)) {
enable_service(v, 1);
continue;
}
if (starts_with(arg, "--disable=")) {
enable_service(arg + 10, 0);
if (skip_prefix(arg, "--disable=", &v)) {
enable_service(v, 0);
continue;
}
if (starts_with(arg, "--allow-override=")) {
make_service_overridable(arg + 17, 1);
if (skip_prefix(arg, "--allow-override=", &v)) {
make_service_overridable(v, 1);
continue;
}
if (starts_with(arg, "--forbid-override=")) {
make_service_overridable(arg + 18, 0);
if (skip_prefix(arg, "--forbid-override=", &v)) {
make_service_overridable(v, 0);
continue;
}
if (!strcmp(arg, "--informative-errors")) {
Expand Down
65 changes: 34 additions & 31 deletions diff.c
View file
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,8 @@ int git_diff_ui_config(const char *var, const char *value, void *cb)

int git_diff_basic_config(const char *var, const char *value, void *cb)
{
const char *name;

if (!strcmp(var, "diff.renamelimit")) {
diff_rename_limit_default = git_config_int(var, value);
return 0;
Expand All @@ -239,8 +241,9 @@ int git_diff_basic_config(const char *var, const char *value, void *cb)
if (userdiff_config(var, value) < 0)
return -1;

if (starts_with(var, "diff.color.") || starts_with(var, "color.diff.")) {
int slot = parse_diff_color_slot(var + 11);
if (skip_prefix(var, "diff.color.", &name) ||
skip_prefix(var, "color.diff.", &name)) {
int slot = parse_diff_color_slot(name);
if (slot < 0)
return 0;
if (!value)
Expand Down Expand Up @@ -2341,6 +2344,7 @@ static void builtin_diff(const char *name_a,
} else {
/* Crazy xdl interfaces.. */
const char *diffopts = getenv("GIT_DIFF_OPTS");
const char *v;
xpparam_t xpp;
xdemitconf_t xecfg;
struct emit_callback ecbdata;
Expand Down Expand Up @@ -2379,10 +2383,10 @@ static void builtin_diff(const char *name_a,
xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);
if (!diffopts)
;
else if (starts_with(diffopts, "--unified="))
xecfg.ctxlen = strtoul(diffopts + 10, NULL, 10);
else if (starts_with(diffopts, "-u"))
xecfg.ctxlen = strtoul(diffopts + 2, NULL, 10);
else if (skip_prefix(diffopts, "--unified=", &v))
xecfg.ctxlen = strtoul(v, NULL, 10);
else if (skip_prefix(diffopts, "-u", &v))
xecfg.ctxlen = strtoul(v, NULL, 10);
if (o->word_diff)
init_diff_words_data(&ecbdata, o, one, two);
xdi_diff_outf(&mf1, &mf2, fn_out_consume, &ecbdata,
Expand Down Expand Up @@ -3609,17 +3613,17 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
options->output_format |= DIFF_FORMAT_SHORTSTAT;
else if (!strcmp(arg, "-X") || !strcmp(arg, "--dirstat"))
return parse_dirstat_opt(options, "");
else if (starts_with(arg, "-X"))
return parse_dirstat_opt(options, arg + 2);
else if (starts_with(arg, "--dirstat="))
return parse_dirstat_opt(options, arg + 10);
else if (skip_prefix(arg, "-X", &arg))
return parse_dirstat_opt(options, arg);
else if (skip_prefix(arg, "--dirstat=", &arg))
return parse_dirstat_opt(options, arg);
else if (!strcmp(arg, "--cumulative"))
return parse_dirstat_opt(options, "cumulative");
else if (!strcmp(arg, "--dirstat-by-file"))
return parse_dirstat_opt(options, "files");
else if (starts_with(arg, "--dirstat-by-file=")) {
else if (skip_prefix(arg, "--dirstat-by-file=", &arg)) {
parse_dirstat_opt(options, "files");
return parse_dirstat_opt(options, arg + 18);
return parse_dirstat_opt(options, arg);
}
else if (!strcmp(arg, "--check"))
options->output_format |= DIFF_FORMAT_CHECKDIFF;
Expand Down Expand Up @@ -3669,9 +3673,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
DIFF_OPT_CLR(options, RENAME_EMPTY);
else if (!strcmp(arg, "--relative"))
DIFF_OPT_SET(options, RELATIVE_NAME);
else if (starts_with(arg, "--relative=")) {
else if (skip_prefix(arg, "--relative=", &arg)) {
DIFF_OPT_SET(options, RELATIVE_NAME);
options->prefix = arg + 11;
options->prefix = arg;
}

/* xdiff options */
Expand Down Expand Up @@ -3722,8 +3726,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if (starts_with(arg, "--color=")) {
int value = git_config_colorbool(NULL, arg+8);
else if (skip_prefix(arg, "--color=", &arg)) {
int value = git_config_colorbool(NULL, arg);
if (value < 0)
return error("option `color' expects \"always\", \"auto\", or \"never\"");
options->use_color = value;
Expand All @@ -3734,29 +3738,28 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
options->use_color = 1;
options->word_diff = DIFF_WORDS_COLOR;
}
else if (starts_with(arg, "--color-words=")) {
else if (skip_prefix(arg, "--color-words=", &arg)) {
options->use_color = 1;
options->word_diff = DIFF_WORDS_COLOR;
options->word_regex = arg + 14;
options->word_regex = arg;
}
else if (!strcmp(arg, "--word-diff")) {
if (options->word_diff == DIFF_WORDS_NONE)
options->word_diff = DIFF_WORDS_PLAIN;
}
else if (starts_with(arg, "--word-diff=")) {
const char *type = arg + 12;
if (!strcmp(type, "plain"))
else if (skip_prefix(arg, "--word-diff=", &arg)) {
if (!strcmp(arg, "plain"))
options->word_diff = DIFF_WORDS_PLAIN;
else if (!strcmp(type, "color")) {
else if (!strcmp(arg, "color")) {
options->use_color = 1;
options->word_diff = DIFF_WORDS_COLOR;
}
else if (!strcmp(type, "porcelain"))
else if (!strcmp(arg, "porcelain"))
options->word_diff = DIFF_WORDS_PORCELAIN;
else if (!strcmp(type, "none"))
else if (!strcmp(arg, "none"))
options->word_diff = DIFF_WORDS_NONE;
else
die("bad --word-diff argument: %s", type);
die("bad --word-diff argument: %s", arg);
}
else if ((argcount = parse_long_opt("word-diff-regex", av, &optarg))) {
if (options->word_diff == DIFF_WORDS_NONE)
Expand All @@ -3779,13 +3782,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
else if (!strcmp(arg, "--ignore-submodules")) {
DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
handle_ignore_submodules_arg(options, "all");
} else if (starts_with(arg, "--ignore-submodules=")) {
} else if (skip_prefix(arg, "--ignore-submodules=", &arg)) {
DIFF_OPT_SET(options, OVERRIDE_SUBMODULE_CONFIG);
handle_ignore_submodules_arg(options, arg + 20);
handle_ignore_submodules_arg(options, arg);
} else if (!strcmp(arg, "--submodule"))
DIFF_OPT_SET(options, SUBMODULE_LOG);
else if (starts_with(arg, "--submodule="))
return parse_submodule_opt(options, arg + 12);
else if (skip_prefix(arg, "--submodule=", &arg))
return parse_submodule_opt(options, arg);

/* misc options */
else if (!strcmp(arg, "-z"))
Expand Down Expand Up @@ -3820,8 +3823,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
}
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (starts_with(arg, "--abbrev=")) {
options->abbrev = strtoul(arg + 9, NULL, 10);
else if (skip_prefix(arg, "--abbrev=", &arg)) {
options->abbrev = strtoul(arg, NULL, 10);
if (options->abbrev < MINIMUM_ABBREV)
options->abbrev = MINIMUM_ABBREV;
else if (40 < options->abbrev)
Expand Down
Loading

0 comments on commit ae021d8

Please sign in to comment.