Hi, I don't know about the procedure, but wonder is any one following this? -- BR, Tony Wang On Sunday, November 13, 2011 at 15:59, Junio C Hamano wrote: > Nguyen Thai Ngoc Duy <pclouds@xxxxxxxxx (mailto:pclouds@xxxxxxxxx)> writes: > > > 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 > > > ... 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), -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html