Re: [PATCH] Copy resolve_ref() return value for longer use

Nguyen Thai Ngoc Duy <pclouds@gmail.com (mailto:pclouds@gmail.com)> writes:

quoted

I went through all of them. Most of them only checks if return value
is NULL, or does one-time string comparison. Others do xstrdup() to
save the return value. Will update the patch message to reflect this.

Thanks. Given your analysis, I think the potential change I alluded to to
return allocated memory from the function is probably overkill in the
current state of the code.

But as the codepaths around the existing callsites evolve, some of these
call sites that are not problematic in today's code can become problematic
if we are not careful. I was primarily wondering if an updated API could
force us to be careful when making future changes.

And a change along the lines of your suggestion

quoted

... Though if you don't mind a bigger patch, we could turn

const char *resolve_ref(const char *path, const char *sha1, int
reading, int *flag);

to

int resolve_ref(const char *path, const char *sha1, int reading, int
*flag, char **ref);

where *ref is the current return value and is only allocated by
resolve_ref() if ref != NULL.

might be one such updated API that makes mistakes harder to make. I dunno.

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().


builtin/branch.c | 3 +--
builtin/merge.c | 4 ++--
2 files changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 51ca6a0..94948a4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c

@@ -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/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),