[PATCH 1/2] Convert many resolve_ref() calls to read_ref*() and ref_exists()

["Nguyễn Thái Ngọc Duy" <pclouds@gmail.com>]

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>
---
 On Sun, Nov 13, 2011 at 2:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
 > But if we were to go that route, as the first step, it might make sense to
 > rewrite "only checks if it returns NULL and uses sha1" callers to call
 > either read_ref() or ref_exists(), so that we do not have to worry about
 > leaking at such callers when we later decide to return allocated memory
 > from resolve_ref().

 Good idea.

 builtin/branch.c   |    5 ++---
 builtin/checkout.c |    4 ++--
 builtin/merge.c    |    4 ++--
 builtin/remote.c   |    8 +++-----
 builtin/replace.c  |    4 ++--
 builtin/show-ref.c |    2 +-
 builtin/tag.c      |    4 ++--
 bundle.c           |    2 +-
 cache.h            |    2 ++
 notes-merge.c      |    2 +-
 refs.c             |   27 ++++++++++++++++-----------
 remote.c           |    4 ++--
 12 files changed, 36 insertions(+), 32 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 51ca6a0..0fe9c4d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -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;
@@ -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;
 
@@ -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);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a80772..beeaee4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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);
@@ -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 */
diff --git a/builtin/merge.c b/builtin/merge.c
index dffd5ec..42b4f9e 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -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;
@@ -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),
diff --git a/builtin/remote.c b/builtin/remote.c
index c810643..407abfb 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -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));
@@ -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))
@@ -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);
diff --git a/builtin/replace.c b/builtin/replace.c
index 517fa10..4a8970e 100644
--- a/builtin/replace.c
+++ b/builtin/replace.c
@@ -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;
@@ -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);
diff --git a/builtin/show-ref.c b/builtin/show-ref.c
index fafb6dd..3911661 100644
--- a/builtin/show-ref.c
+++ b/builtin/show-ref.c
@@ -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);
 			}
diff --git a/builtin/tag.c b/builtin/tag.c
index 9b6fd95..439249d 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -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;
@@ -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);
diff --git a/bundle.c b/bundle.c
index 08020bc..4742f27 100644
--- a/bundle.c
+++ b/bundle.c
@@ -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;
 
diff --git a/cache.h b/cache.h
index 2e6ad36..5badece 100644
--- a/cache.h
+++ b/cache.h
@@ -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);
 
 /*
diff --git a/notes-merge.c b/notes-merge.c
index e9e4199..e33c2c9 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -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))
diff --git a/refs.c b/refs.c
index e69ba26..44c1c86 100644
--- a/refs.c
+++ b/refs.c
@@ -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;
 				}
@@ -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)
@@ -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)) {
@@ -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;
@@ -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);
 
@@ -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;
@@ -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);
@@ -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);
@@ -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 */
@@ -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;
 		}
 
diff --git a/remote.c b/remote.c
index e2ef991..6655bb0 100644
--- a/remote.c
+++ b/remote.c
@@ -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)
-- 
1.7.4.74.g639db