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

[Tony Wang <wwwjfy@gmail.com>]

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@gmail.com (mailto: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
> 
> > ... 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),