Re: [PATCH] Copy resolve_ref() return value for longer use
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Nov 13, 2011, 07:59 UTC
- Message-ID
- <7vobwgo3l5.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <CACsJy8ARAzNWjZfXwnNG0AprCFXLCkiDrE+eFj9icbeNX14xKw@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> 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
Show 12 quoted lines
> ... 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),