Skip to content

Commit

Permalink
Convert many resolve_ref() calls to read_ref*() and ref_exists()
Browse files Browse the repository at this point in the history
resolve_ref() may return a pointer to a static buffer, which is not
safe for long-term use because if another resolve_ref() call happens,
the buffer may be changed.  Many call sites though do not care about
this buffer. They simply check if the return value is NULL or not.

Convert all these call sites to new wrappers to reduce resolve_ref()
calls from 57 to 34. If we change resolve_ref() prototype later on
to avoid passing static buffer out, this helps reduce changes.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
  • Loading branch information
pclouds authored and gitster committed Nov 13, 2011
1 parent bc1bbe0 commit c689332
Show file tree
Hide file tree
Showing 12 changed files with 36 additions and 32 deletions.
5 changes: 2 additions & 3 deletions builtin/branch.c
Expand Up @@ -186,7 +186,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)
free(name);

name = xstrdup(mkpath(fmt, bname.buf));
if (!resolve_ref(name, sha1, 1, NULL)) {
if (read_ref(name, sha1)) {
error(_("%sbranch '%s' not found."),
remote, bname.buf);
ret = 1;
Expand Down Expand Up @@ -565,7 +565,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru
static void rename_branch(const char *oldname, const char *newname, int force)
{
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;
unsigned char sha1[20];
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;

Expand All @@ -577,7 +576,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
* Bad name --- this could be an attempt to rename a
* ref that we used to allow to be created by accident.
*/
if (resolve_ref(oldref.buf, sha1, 1, NULL))
if (ref_exists(oldref.buf))
recovery = 1;
else
die(_("Invalid branch name: '%s'"), oldname);
Expand Down
4 changes: 2 additions & 2 deletions builtin/checkout.c
Expand Up @@ -288,7 +288,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,
commit_locked_index(lock_file))
die(_("unable to write new index file"));

resolve_ref("HEAD", rev, 0, &flag);
read_ref_full("HEAD", rev, 0, &flag);
head = lookup_commit_reference_gently(rev, 1);

errs |= post_checkout_hook(head, head, 0);
Expand Down Expand Up @@ -866,7 +866,7 @@ static int parse_branchname_arg(int argc, const char **argv,
setup_branch_path(new);

if (!check_refname_format(new->path, 0) &&
resolve_ref(new->path, branch_rev, 1, NULL))
!read_ref(new->path, branch_rev))
hashcpy(rev, branch_rev);
else
new->path = NULL; /* not an existing branch */
Expand Down
4 changes: 2 additions & 2 deletions builtin/merge.c
Expand Up @@ -420,7 +420,7 @@ static struct object *want_commit(const char *name)
static void merge_name(const char *remote, struct strbuf *msg)
{
struct object *remote_head;
unsigned char branch_head[20], buf_sha[20];
unsigned char branch_head[20];
struct strbuf buf = STRBUF_INIT;
struct strbuf bname = STRBUF_INIT;
const char *ptr;
Expand Down Expand Up @@ -479,7 +479,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
strbuf_addstr(&truname, "refs/heads/");
strbuf_addstr(&truname, remote);
strbuf_setlen(&truname, truname.len - len);
if (resolve_ref(truname.buf, buf_sha, 1, NULL)) {
if (ref_exists(truname.buf)) {
strbuf_addf(msg,
"%s\t\tbranch '%s'%s of .\n",
sha1_to_hex(remote_head->sha1),
Expand Down
8 changes: 3 additions & 5 deletions builtin/remote.c
Expand Up @@ -343,8 +343,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat
states->tracked.strdup_strings = 1;
states->stale.strdup_strings = 1;
for (ref = fetch_map; ref; ref = ref->next) {
unsigned char sha1[20];
if (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))
if (!ref->peer_ref || !ref_exists(ref->peer_ref->name))
string_list_append(&states->new, abbrev_branch(ref->name));
else
string_list_append(&states->tracked, abbrev_branch(ref->name));
Expand Down Expand Up @@ -710,7 +709,7 @@ static int mv(int argc, const char **argv)
int flag = 0;
unsigned char sha1[20];

resolve_ref(item->string, sha1, 1, &flag);
read_ref_full(item->string, sha1, 1, &flag);
if (!(flag & REF_ISSYMREF))
continue;
if (delete_ref(item->string, NULL, REF_NODEREF))
Expand Down Expand Up @@ -1220,10 +1219,9 @@ static int set_head(int argc, const char **argv)
usage_with_options(builtin_remote_sethead_usage, options);

if (head_name) {
unsigned char sha1[20];
strbuf_addf(&buf2, "refs/remotes/%s/%s", argv[0], head_name);
/* make sure it's valid */
if (!resolve_ref(buf2.buf, sha1, 1, NULL))
if (!ref_exists(buf2.buf))
result |= error("Not a valid ref: %s", buf2.buf);
else if (create_symref(buf.buf, buf2.buf, "remote set-head"))
result |= error("Could not setup %s", buf.buf);
Expand Down
4 changes: 2 additions & 2 deletions builtin/replace.c
Expand Up @@ -58,7 +58,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)
had_error = 1;
continue;
}
if (!resolve_ref(ref, sha1, 1, NULL)) {
if (read_ref(ref, sha1)) {
error("replace ref '%s' not found.", *p);
had_error = 1;
continue;
Expand Down Expand Up @@ -97,7 +97,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,
if (check_refname_format(ref, 0))
die("'%s' is not a valid ref name.", ref);

if (!resolve_ref(ref, prev, 1, NULL))
if (read_ref(ref, prev))
hashclr(prev);
else if (!force)
die("replace ref '%s' already exists", ref);
Expand Down
2 changes: 1 addition & 1 deletion builtin/show-ref.c
Expand Up @@ -225,7 +225,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)
unsigned char sha1[20];

if (!prefixcmp(*pattern, "refs/") &&
resolve_ref(*pattern, sha1, 1, NULL)) {
!read_ref(*pattern, sha1)) {
if (!quiet)
show_one(*pattern, sha1);
}
Expand Down
4 changes: 2 additions & 2 deletions builtin/tag.c
Expand Up @@ -174,7 +174,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)
had_error = 1;
continue;
}
if (!resolve_ref(ref, sha1, 1, NULL)) {
if (read_ref(ref, sha1)) {
error(_("tag '%s' not found."), *p);
had_error = 1;
continue;
Expand Down Expand Up @@ -518,7 +518,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)
if (strbuf_check_tag_ref(&ref, tag))
die(_("'%s' is not a valid tag name."), tag);

if (!resolve_ref(ref.buf, prev, 1, NULL))
if (read_ref(ref.buf, prev))
hashclr(prev);
else if (!force)
die(_("tag '%s' already exists"), tag);
Expand Down
2 changes: 1 addition & 1 deletion bundle.c
Expand Up @@ -320,7 +320,7 @@ int create_bundle(struct bundle_header *header, const char *path,
continue;
if (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)
continue;
if (!resolve_ref(e->name, sha1, 1, &flag))
if (read_ref_full(e->name, sha1, 1, &flag))
flag = 0;
display_ref = (flag & REF_ISSYMREF) ? e->name : ref;

Expand Down
2 changes: 2 additions & 0 deletions cache.h
Expand Up @@ -832,6 +832,8 @@ static inline int get_sha1_with_context(const char *str, unsigned char *sha1, st
extern int get_sha1_hex(const char *hex, unsigned char *sha1);

extern char *sha1_to_hex(const unsigned char *sha1); /* static buffer result! */
extern int read_ref_full(const char *filename, unsigned char *sha1,
int reading, int *flags);
extern int read_ref(const char *filename, unsigned char *sha1);

/*
Expand Down
2 changes: 1 addition & 1 deletion notes-merge.c
Expand Up @@ -568,7 +568,7 @@ int notes_merge(struct notes_merge_options *o,
o->local_ref, o->remote_ref);

/* Dereference o->local_ref into local_sha1 */
if (!resolve_ref(o->local_ref, local_sha1, 0, NULL))
if (read_ref_full(o->local_ref, local_sha1, 0, NULL))
die("Failed to resolve local notes ref '%s'", o->local_ref);
else if (!check_refname_format(o->local_ref, 0) &&
is_null_sha1(local_sha1))
Expand Down
27 changes: 16 additions & 11 deletions refs.c
Expand Up @@ -334,7 +334,7 @@ static void get_ref_dir(const char *submodule, const char *base,
flag |= REF_ISBROKEN;
}
} else
if (!resolve_ref(ref, sha1, 1, &flag)) {
if (read_ref_full(ref, sha1, 1, &flag)) {
hashclr(sha1);
flag |= REF_ISBROKEN;
}
Expand Down Expand Up @@ -612,13 +612,18 @@ struct ref_filter {
void *cb_data;
};

int read_ref(const char *ref, unsigned char *sha1)
int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)
{
if (resolve_ref(ref, sha1, 1, NULL))
if (resolve_ref(ref, sha1, reading, flags))
return 0;
return -1;
}

int read_ref(const char *ref, unsigned char *sha1)
{
return read_ref_full(ref, sha1, 1, NULL);
}

#define DO_FOR_EACH_INCLUDE_BROKEN 01
static int do_one_ref(const char *base, each_ref_fn fn, int trim,
int flags, void *cb_data, struct ref_entry *entry)
Expand Down Expand Up @@ -663,7 +668,7 @@ int peel_ref(const char *ref, unsigned char *sha1)
goto fallback;
}

if (!resolve_ref(ref, base, 1, &flag))
if (read_ref_full(ref, base, 1, &flag))
return -1;

if ((flag & REF_ISPACKED)) {
Expand Down Expand Up @@ -746,7 +751,7 @@ static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)
return 0;
}

if (resolve_ref("HEAD", sha1, 1, &flag))
if (!read_ref_full("HEAD", sha1, 1, &flag))
return fn("HEAD", sha1, flag, cb_data);

return 0;
Expand Down Expand Up @@ -826,7 +831,7 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)
int flag;

strbuf_addf(&buf, "%sHEAD", get_git_namespace());
if (resolve_ref(buf.buf, sha1, 1, &flag))
if (!read_ref_full(buf.buf, sha1, 1, &flag))
ret = fn(buf.buf, sha1, flag, cb_data);
strbuf_release(&buf);

Expand Down Expand Up @@ -1022,7 +1027,7 @@ int refname_match(const char *abbrev_name, const char *full_name, const char **r
static struct ref_lock *verify_lock(struct ref_lock *lock,
const unsigned char *old_sha1, int mustexist)
{
if (!resolve_ref(lock->ref_name, lock->old_sha1, mustexist, NULL)) {
if (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {
error("Can't verify ref %s", lock->ref_name);
unlock_ref(lock);
return NULL;
Expand Down Expand Up @@ -1377,7 +1382,8 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
goto rollback;
}

if (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {
if (!read_ref_full(newref, sha1, 1, &flag) &&
delete_ref(newref, sha1, REF_NODEREF)) {
if (errno==EISDIR) {
if (remove_empty_directories(git_path("%s", newref))) {
error("Directory not empty: %s", newref);
Expand Down Expand Up @@ -1929,7 +1935,7 @@ static int do_for_each_reflog(const char *base, each_ref_fn fn, void *cb_data)
retval = do_for_each_reflog(log, fn, cb_data);
} else {
unsigned char sha1[20];
if (!resolve_ref(log, sha1, 0, NULL))
if (read_ref_full(log, sha1, 0, NULL))
retval = error("bad ref for %s", log);
else
retval = fn(log, sha1, 0, cb_data);
Expand Down Expand Up @@ -2072,7 +2078,6 @@ char *shorten_unambiguous_ref(const char *ref, int strict)
*/
for (j = 0; j < rules_to_fail; j++) {
const char *rule = ref_rev_parse_rules[j];
unsigned char short_objectname[20];
char refname[PATH_MAX];

/* skip matched rule */
Expand All @@ -2086,7 +2091,7 @@ char *shorten_unambiguous_ref(const char *ref, int strict)
*/
mksnpath(refname, sizeof(refname),
rule, short_name_len, short_name);
if (!read_ref(refname, short_objectname))
if (ref_exists(refname))
break;
}

Expand Down
4 changes: 2 additions & 2 deletions remote.c
Expand Up @@ -1507,13 +1507,13 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)
* nothing to report.
*/
base = branch->merge[0]->dst;
if (!resolve_ref(base, sha1, 1, NULL))
if (read_ref(base, sha1))
return 0;
theirs = lookup_commit_reference(sha1);
if (!theirs)
return 0;

if (!resolve_ref(branch->refname, sha1, 1, NULL))
if (read_ref(branch->refname, sha1))
return 0;
ours = lookup_commit_reference(sha1);
if (!ours)
Expand Down

0 comments on commit c689332

Please sign in to comment.