Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7

Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7

[Nguyen Thai Ngoc Duy]

Hi,

On Mon, Nov 7, 2011 at 3:59 PM, Tony Wang <wwwjfy@gmail.com> wrote:
> Hi,
> I don't know if a better way to report this, so I write to the author of the
> commit. Please let me know if I do wrong. :)

It's good that you bisect to the broken commit and send me. However
you should always send to git@vger just in case I'm unavailable.

> The thing is the commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7 made an
> option broken sometimes (it's weird, but it's true that it didn't happen
> every time)
> I set "branch.master.mergeoptions=--squash" in config, but when I do "git
> merge b", the squash didn't work, however, "git merge b --squash" works as
> expected.

What was the expection? --squash was not effective or something else?

> I tried to debug, and found after this
> merge.c:1104
> head_commit = lookup_commit_or_die(head_sha1, "HEAD");
> the variable branch becomes "s/origin/b", which is previously "b".
> I used git bisect and found the
> commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7 caused this.

Variable "head_sha1"? Strange because lookup_commit_or_die() takes
"const char *" and the compiler should catch any attempts to change
the variable.

If you can reproduce it, can you make a small test case to demonstrate
it? I'm not sure what "b" is and how you set up configuration for
branch master. BTW what git version did you use?

> I browsed the diff, and found the function lookup_commit_or_die
> uses lookup_commit_reference, but not lookup_commit which was used
> before lookup_commit_or_die replaced it.
> Was it on purpose or typo?

It was on purpose. HEAD may contain a tag, in which case
lookup_commit() would to return a commit fail while
lookup_commit_reference() can peel the tag to the commit.

> If possible, it'll be good that I can know some details.
> Thanks!
-- 
Duy

Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7

[Tony Wang]

On Monday, November 7, 2011 at 17:30, Nguyen Thai Ngoc Duy wrote:

> Hi,
> 
> On Mon, Nov 7, 2011 at 3:59 PM, Tony Wang <wwwjfy@gmail.com (mailto:wwwjfy@gmail.com)> wrote:
> > Hi,
> > I don't know if a better way to report this, so I write to the author of the
> > commit. Please let me know if I do wrong. :)
> 
> 
> 
> 
> It's good that you bisect to the broken commit and send me. However
> you should always send to git@vger just in case I'm unavailable.
> 
Roger. 
> 
> > The thing is the commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7 made an
> > option broken sometimes (it's weird, but it's true that it didn't happen
> > every time)
> > I set "branch.master.mergeoptions=--squash" in config, but when I do "git
> > merge b", the squash didn't work, however, "git merge b --squash" works as
> > expected.
> 
> 
> 
> 
> What was the expection? --squash was not effective or something else?
> 
Yes, it just merged like without "--squash" 
> 
> > I tried to debug, and found after this
> > merge.c:1104
> > head_commit = lookup_commit_or_die(head_sha1, "HEAD");
> > the variable branch becomes "s/origin/b", which is previously "b".
> > I used git bisect and found the
> > commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7 caused this.
> 
> 
> 
> 
> Variable "head_sha1"? Strange because lookup_commit_or_die() takes
> "const char *" and the compiler should catch any attempts to change
> the variable.

No, variable "branch". It's declared as "static const char *branch;" in builtin/merge.c 
> 
> If you can reproduce it, can you make a small test case to demonstrate
> it? I'm not sure what "b" is and how you set up configuration for
> branch master. BTW what git version did you use?
> 
I'll try it.
Sorry, "b" is a random branch name, I should use a meaningful one.
I set by "git config branch.master.mergeoptions --squash"

I tried 1.7.7.2 and master of github.com/gitster/git.git (http://github.com/gitster/git.git). 
> 
> > I browsed the diff, and found the function lookup_commit_or_die
> > uses lookup_commit_reference, but not lookup_commit which was used
> > before lookup_commit_or_die replaced it.
> > Was it on purpose or typo?
> 
> 
> 
> 
> It was on purpose. HEAD may contain a tag, in which case
> lookup_commit() would to return a commit fail while
> lookup_commit_reference() can peel the tag to the commit.

However, I tried to make it lookup_commit and it worked as expected. I'll try to read some detail. 
> 
> > If possible, it'll be good that I can know some details.
> > Thanks!
> 
> 
> 
> -- 
> Duy



--
BR,
Tony Wang

Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7

[Nguyen Thai Ngoc Duy]

On Mon, Nov 07, 2011 at 05:48:25PM +0800, Tony Wang wrote:
> > It was on purpose. HEAD may contain a tag, in which case
> > lookup_commit() would to return a commit fail while
> > lookup_commit_reference() can peel the tag to the commit.
> 
> However, I tried to make it lookup_commit and it worked as expected. I'll try to read some detail. 

and the strange value of branch "s/origin/b" in your previous message..

> > 
> > > I tried to debug, and found after this
> > > merge.c:1104
> > > head_commit = lookup_commit_or_die(head_sha1, "HEAD");
> > > the variable branch becomes "s/origin/b", which is previously "b".

..led me to think that it's because branch points to the static buffer
returned by by resolve_ref().

lookup_commit_reference() may call resolve_ref() again and change the
buffer value, which also changes "branch" variable.

So, does this help?

-- 8< --
diff --git a/builtin/merge.c b/builtin/merge.c
index 581f494..4f20833 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1029,7 +1029,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 	 * current branch.
 	 */
-	branch = resolve_ref("HEAD", head_sha1, 0, &flag);
+	branch = xstrdup(resolve_ref("HEAD", head_sha1, 0, &flag));
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
-- 8< --
-- 
Duy

Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7

[Tony Wang]

On Monday, November 7, 2011 at 18:41, Nguyen Thai Ngoc Duy wrote:
> On Mon, Nov 07, 2011 at 05:48:25PM +0800, Tony Wang wrote:
> > > It was on purpose. HEAD may contain a tag, in which case
> > > lookup_commit() would to return a commit fail while
> > > lookup_commit_reference() can peel the tag to the commit.
> > 
> > 
> > 
> > However, I tried to make it lookup_commit and it worked as expected. I'll try to read some detail. 
> 
> and the strange value of branch "s/origin/b" in your previous message..
that's the strange thing. seems like part of the string "refs/origin/b" 
> 
> > > 
> > > > I tried to debug, and found after this
> > > > merge.c:1104
> > > > head_commit = lookup_commit_or_die(head_sha1, "HEAD");
> > > > the variable branch becomes "s/origin/b", which is previously "b".
> > > 
> > 
> 
> 
> 
> ..led me to think that it's because branch points to the static buffer
> returned by by resolve_ref().
> 
> lookup_commit_reference() may call resolve_ref() again and change the
> buffer value, which also changes "branch" variable.
> 
> So, does this help?
> 
> -- 8< --
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 581f494..4f20833 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1029,7 +1029,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> * Check if we are _not_ on a detached HEAD, i.e. if there is a
> * current branch.
> */
> - branch = resolve_ref("HEAD", head_sha1, 0, &flag);
> + branch = xstrdup(resolve_ref("HEAD", head_sha1, 0, &flag));
> if (branch && !prefixcmp(branch, "refs/heads/"))
> branch += 11;
> if (!branch || is_null_sha1(head_sha1))
> -- 8< --

Yes! It works.
I do learn from it, thanks!
But seems it's possible the potential problem exists somewhere else.
> -- 
> Duy



-- 
BR,
Tony Wang

Re: git bug(?) for commit baf18fc261ca475343fe3cb9cd2c0dded4bc1bb7

[Nguyen Thai Ngoc Duy]

On Mon, Nov 7, 2011 at 6:02 PM, Tony Wang <wwwjfy@gmail.com> wrote:
> Yes! It works.
> I do learn from it, thanks!
> But seems it's possible the potential problem exists somewhere else.

Sure. There are 58 resolve_ref() call sites. I'll go through and send
proper patches later.
-- 
Duy

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

[Nguyễn Thái Ngọc Duy]

resolve_ref() may return a pointer to a static buffer. Callers that
use this value outside of a block should copy the value to avoid some
hidden resolve_ref() call that may change the static buffer's value.

The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
demonstrates this. The first call is in cmd_merge()

branch = resolve_ref("HEAD", head_sha1, 0, &flag);

Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
may be called again and destroy "branch".

lookup_commit_or_die
 lookup_commit_reference
  lookup_commit_reference_gently
   parse_object
    lookup_replace_object
     do_lookup_replace_object
      prepare_replace_object
       for_each_replace_ref
        do_for_each_ref
         get_loose_refs
          get_ref_dir
           get_ref_dir
            resolve_ref

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c      |    4 +++-
 builtin/commit.c        |    3 ++-
 builtin/fmt-merge-msg.c |    1 +
 builtin/merge.c         |    7 +++++--
 builtin/notes.c         |    1 +
 builtin/receive-pack.c  |    2 ++
 6 files changed, 14 insertions(+), 4 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a80772..98ddbcd 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -699,7 +699,9 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	unsigned char rev[20];
 	int flag;
 	memset(&old, 0, sizeof(old));
-	old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag));
+	old.path = resolve_ref("HEAD", rev, 0, &flag);
+	if (old.path)
+		old.path = xstrdup(old.path);
 	old.commit = lookup_commit_reference_gently(rev, 1);
 	if (!(flag & REF_ISSYMREF)) {
 		free((char *)old.path);
diff --git a/builtin/commit.c b/builtin/commit.c
index c46f2d1..f3a6ed2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	struct commit *commit;
 	struct strbuf format = STRBUF_INIT;
 	unsigned char junk_sha1[20];
-	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+	const char *head;
 	struct pretty_print_context pctx = {0};
 	struct strbuf author_ident = STRBUF_INIT;
 	struct strbuf committer_ident = STRBUF_INIT;
@@ -1304,6 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
 
+	head = resolve_ref("HEAD", junk_sha1, 0, NULL);
 	printf("[%s%s ",
 		!prefixcmp(head, "refs/heads/") ?
 			head + 11 :
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7e2f225..cab50e0 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -268,6 +268,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 		die("No current branch");
 	if (!prefixcmp(current_branch, "refs/heads/"))
 		current_branch += 11;
+	current_branch = xstrdup(current_branch);
 
 	/* get a line */
 	while (pos < in->len) {
diff --git a/builtin/merge.c b/builtin/merge.c
index dffd5ec..6865cb7 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1096,8 +1096,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * current branch.
 	 */
 	branch = resolve_ref("HEAD", head_sha1, 0, &flag);
-	if (branch && !prefixcmp(branch, "refs/heads/"))
-		branch += 11;
+	if (branch) {
+		if (!prefixcmp(branch, "refs/heads/"))
+			branch += 11;
+		branch = xstrdup(branch);
+	}
 	if (!branch || is_null_sha1(head_sha1))
 		head_commit = NULL;
 	else
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..c6e4c86 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -828,6 +828,7 @@ static int merge_commit(struct notes_merge_options *o)
 	o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL);
 	if (!o->local_ref)
 		die("Failed to resolve NOTES_MERGE_REF");
+	o->local_ref = xstrdup(o->local_ref);
 
 	if (notes_merge_commit(o, t, partial, sha1))
 		die("Failed to finalize notes merge");
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..6065bf0 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -696,6 +696,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 	check_aliased_updates(commands);
 
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
+	if (head_name)
+		head_name = xstrdup(head_name);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
 		if (!cmd->skip_update)
-- 
1.7.3.1.256.g2539c.dirty

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> resolve_ref() may return a pointer to a static buffer. Callers that
> use this value outside of a block should copy the value to avoid some
> hidden resolve_ref() call that may change the static buffer's value.
>
> The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
> demonstrates this. The first call is in cmd_merge()
>
> branch = resolve_ref("HEAD", head_sha1, 0, &flag);
>
> Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
> may be called again and destroy "branch".
>
> lookup_commit_or_die
>  lookup_commit_reference
>   lookup_commit_reference_gently
>    parse_object
>     lookup_replace_object
>      do_lookup_replace_object
>       prepare_replace_object
>        for_each_replace_ref
>         do_for_each_ref
>          get_loose_refs
>           get_ref_dir
>            get_ref_dir
>             resolve_ref
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---

Thanks.

The above log message with the callchain would be a perfect explanation
for a patch to fix builtin/merge.c and nothing else, but among 50+
callsites of resolve_ref(), only 5 places are modified in this patch, and
it is not explained in the commit log message at all how they were chosen.
Were the other 40+ or so places inspected and deemed to be OK? Or is this
"I just did a few of them in addition to the immediate need of fixing what
was reported, and the rest are left as an exercise to the readers" patch?

Some variables that receive xstrdup()'ed result with this patch are
globals whose lifetime lasts while the process is alive, and I do not
think it is a huge problem that this patch does not free it, but it seems
the patch does not free some other function scope variables once their use
is done even when it is fairly easy to do so.

How much overhead would it incur if we return xstrdup()'ed memory from the
resolve_ref() and make it the caller's responsibility to free it? Would it
make the calling site too ugly?

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

[Nguyen Thai Ngoc Duy]

2011/11/13 Junio C Hamano <gitster@pobox.com>:
> The above log message with the callchain would be a perfect explanation
> for a patch to fix builtin/merge.c and nothing else, but among 50+
> callsites of resolve_ref(), only 5 places are modified in this patch, and
> it is not explained in the commit log message at all how they were chosen.
> Were the other 40+ or so places inspected and deemed to be OK? Or is this
> "I just did a few of them in addition to the immediate need of fixing what
> was reported, and the rest are left as an exercise to the readers" patch?

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.

> Some variables that receive xstrdup()'ed result with this patch are
> globals whose lifetime lasts while the process is alive, and I do not
> think it is a huge problem that this patch does not free it, but it seems
> the patch does not free some other function scope variables once their use
> is done even when it is fairly easy to do so.

Will fix.

> How much overhead would it incur if we return xstrdup()'ed memory from the
> resolve_ref() and make it the caller's responsibility to free it? Would it
> make the calling site too ugly?

Given a lot of callsites do "if (resolve_ref(...)) ...", yes it may
look ugly. 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.
-- 
Duy

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

[Junio C Hamano]

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

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

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

[Nguyễn Thái Ngọc Duy]

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

[PATCH 2/2] Copy resolve_ref() return value for longer use

[Nguyễn Thái Ngọc Duy]

resolve_ref() may return a pointer to a static buffer. Callers that
use this value longer than a couple of statements should copy the
value to avoid some hidden resolve_ref() call that may change the
static buffer's value.

The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
demonstrates this. The first call is in cmd_merge()

branch = resolve_ref("HEAD", head_sha1, 0, &flag);

Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
may be called again and destroy "branch".

lookup_commit_or_die
 lookup_commit_reference
  lookup_commit_reference_gently
   parse_object
    lookup_replace_object
     do_lookup_replace_object
      prepare_replace_object
       for_each_replace_ref
        do_for_each_ref
         get_loose_refs
          get_ref_dir
           get_ref_dir
            resolve_ref

All call sites are checked and made sure that xstrdup() is called if
the value should be saved.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 Unfortunately there are still 37 resolve_ref() calls. An API change
 would touch all of them and potentially introduce more bugs along the
 way, either leak more memory or free() the wrong way.

 So I'd stick with this for v1.7.8.

 builtin/branch.c        |    5 +++-
 builtin/checkout.c      |    4 ++-
 builtin/commit.c        |    3 +-
 builtin/fmt-merge-msg.c |    6 ++++-
 builtin/merge.c         |   62 ++++++++++++++++++++++++++++++----------------
 builtin/notes.c         |    6 ++++-
 builtin/receive-pack.c  |    3 ++
 reflog-walk.c           |    5 +++-
 8 files changed, 66 insertions(+), 28 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 0fe9c4d..5b6d839 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -115,8 +115,10 @@ static int branch_merged(int kind, const char *name,
 		    branch->merge[0] &&
 		    branch->merge[0]->dst &&
 		    (reference_name =
-		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
+		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {
+			reference_name = xstrdup(reference_name);
 			reference_rev = lookup_commit_reference(sha1);
+		}
 	}
 	if (!reference_rev)
 		reference_rev = head_rev;
@@ -141,6 +143,7 @@ static int branch_merged(int kind, const char *name,
 				"         '%s', even though it is merged to HEAD."),
 				name, reference_name);
 	}
+	free((char*)reference_name);
 	return merged;
 }
 
diff --git a/builtin/checkout.c b/builtin/checkout.c
index beeaee4..c6919f1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -699,7 +699,9 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	unsigned char rev[20];
 	int flag;
 	memset(&old, 0, sizeof(old));
-	old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag));
+	old.path = resolve_ref("HEAD", rev, 0, &flag);
+	if (old.path)
+		old.path = xstrdup(old.path);
 	old.commit = lookup_commit_reference_gently(rev, 1);
 	if (!(flag & REF_ISSYMREF)) {
 		free((char *)old.path);
diff --git a/builtin/commit.c b/builtin/commit.c
index c46f2d1..f3a6ed2 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	struct commit *commit;
 	struct strbuf format = STRBUF_INIT;
 	unsigned char junk_sha1[20];
-	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+	const char *head;
 	struct pretty_print_context pctx = {0};
 	struct strbuf author_ident = STRBUF_INIT;
 	struct strbuf committer_ident = STRBUF_INIT;
@@ -1304,6 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
 
+	head = resolve_ref("HEAD", junk_sha1, 0, NULL);
 	printf("[%s%s ",
 		!prefixcmp(head, "refs/heads/") ?
 			head + 11 :
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7e2f225..6fe9102 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -268,6 +268,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 		die("No current branch");
 	if (!prefixcmp(current_branch, "refs/heads/"))
 		current_branch += 11;
+	current_branch = xstrdup(current_branch);
 
 	/* get a line */
 	while (pos < in->len) {
@@ -283,8 +284,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 			die ("Error in line %d: %.*s", i, len, p);
 	}
 
-	if (!srcs.nr)
+	if (!srcs.nr) {
+		free((char*)current_branch);
 		return 0;
+	}
 
 	if (merge_title)
 		do_fmt_merge_msg_title(out, current_branch);
@@ -306,6 +309,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 			shortlog(origins.items[i].string, origins.items[i].util,
 					head, &rev, shortlog_len, out);
 	}
+	free((char*)current_branch);
 	return 0;
 }
 
diff --git a/builtin/merge.c b/builtin/merge.c
index 42b4f9e..33be6c8 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1082,7 +1082,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
 	const char *head_arg;
-	int flag, i;
+	int flag, i, ret = 0;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1096,8 +1096,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * current branch.
 	 */
 	branch = resolve_ref("HEAD", head_sha1, 0, &flag);
-	if (branch && !prefixcmp(branch, "refs/heads/"))
-		branch += 11;
+	if (branch) {
+		if (!prefixcmp(branch, "refs/heads/"))
+			branch += 11;
+		branch = xstrdup(branch);
+	}
 	if (!branch || is_null_sha1(head_sha1))
 		head_commit = NULL;
 	else
@@ -1121,7 +1124,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			die(_("There is no merge to abort (MERGE_HEAD missing)."));
 
 		/* Invoke 'git reset --merge' */
-		return cmd_reset(nargc, nargv, prefix);
+		ret = cmd_reset(nargc, nargv, prefix);
+		goto done;
 	}
 
 	if (read_cache_unmerged())
@@ -1205,7 +1209,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		read_empty(remote_head->sha1, 0);
 		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
 				DIE_ON_ERR);
-		return 0;
+		goto done;
 	} else {
 		struct strbuf merge_names = STRBUF_INIT;
 
@@ -1292,7 +1296,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * but first the most common case of merging one remote.
 		 */
 		finish_up_to_date("Already up-to-date.");
-		return 0;
+		goto done;
 	} else if (allow_fast_forward && !remoteheads->next &&
 			!common->next &&
 			!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
@@ -1313,15 +1317,20 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			strbuf_addstr(&msg,
 				" (no commit created; -m option ignored)");
 		o = want_commit(sha1_to_hex(remoteheads->item->object.sha1));
-		if (!o)
-			return 1;
+		if (!o) {
+			ret = 1;
+			goto done;
+		}
 
-		if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))
-			return 1;
+		if (checkout_fast_forward(head_commit->object.sha1,
+					  remoteheads->item->object.sha1)) {
+			ret = 1;
+			goto done;
+		}
 
 		finish(head_commit, o->sha1, msg.buf);
 		drop_save();
-		return 0;
+		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
 		/*
@@ -1339,8 +1348,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			git_committer_info(IDENT_ERROR_ON_NO_NAME);
 			printf(_("Trying really trivial in-index merge...\n"));
 			if (!read_tree_trivial(common->item->object.sha1,
-					head_commit->object.sha1, remoteheads->item->object.sha1))
-				return merge_trivial(head_commit);
+					       head_commit->object.sha1,
+					       remoteheads->item->object.sha1)) {
+				ret = merge_trivial(head_commit);
+				goto done;
+			}
 			printf(_("Nope.\n"));
 		}
 	} else {
@@ -1368,7 +1380,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 		if (up_to_date) {
 			finish_up_to_date("Already up-to-date. Yeeah!");
-			return 0;
+			goto done;
 		}
 	}
 
@@ -1450,9 +1462,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * If we have a resulting tree, that means the strategy module
 	 * auto resolved the merge cleanly.
 	 */
-	if (automerge_was_ok)
-		return finish_automerge(head_commit, common, result_tree,
-					wt_strategy);
+	if (automerge_was_ok) {
+		ret = finish_automerge(head_commit, common, result_tree,
+				       wt_strategy);
+		goto done;
+	}
 
 	/*
 	 * Pick the result from the best strategy and have the user fix
@@ -1466,7 +1480,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			fprintf(stderr, _("Merge with strategy %s failed.\n"),
 				use_strategies[0]->name);
-		return 2;
+		ret = 2;
+		goto done;
 	} else if (best_strategy == wt_strategy)
 		; /* We already have its result in the working tree. */
 	else {
@@ -1482,10 +1497,13 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	else
 		write_merge_state();
 
-	if (merge_was_ok) {
+	if (merge_was_ok)
 		fprintf(stderr, _("Automatic merge went well; "
 			"stopped before committing as requested\n"));
-		return 0;
-	} else
-		return suggest_conflicts(option_renormalize);
+	else
+		ret = suggest_conflicts(option_renormalize);
+
+done:
+	free((char*)branch);
+	return ret;
 }
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..ccff770 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -804,6 +804,7 @@ static int merge_commit(struct notes_merge_options *o)
 	struct notes_tree *t;
 	struct commit *partial;
 	struct pretty_print_context pretty_ctx;
+	int ret;
 
 	/*
 	 * Read partial merge result from .git/NOTES_MERGE_PARTIAL,
@@ -828,6 +829,7 @@ static int merge_commit(struct notes_merge_options *o)
 	o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL);
 	if (!o->local_ref)
 		die("Failed to resolve NOTES_MERGE_REF");
+	o->local_ref = xstrdup(o->local_ref);
 
 	if (notes_merge_commit(o, t, partial, sha1))
 		die("Failed to finalize notes merge");
@@ -843,7 +845,9 @@ static int merge_commit(struct notes_merge_options *o)
 
 	free_notes(t);
 	strbuf_release(&msg);
-	return merge_abort(o);
+	ret = merge_abort(o);
+	free((char*)o->local_ref);
+	return ret;
 }
 
 static int merge(int argc, const char **argv, const char *prefix)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..d3fe42a 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -695,7 +695,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 
 	check_aliased_updates(commands);
 
+	free((char*)head_name);
 	head_name = resolve_ref("HEAD", sha1, 0, NULL);
+	if (head_name)
+		head_name = xstrdup(head_name);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
 		if (!cmd->skip_update)
diff --git a/reflog-walk.c b/reflog-walk.c
index 5d81d39..efd13ad 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -51,8 +51,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 	if (reflogs->nr == 0) {
 		unsigned char sha1[20];
 		const char *name = resolve_ref(ref, sha1, 1, NULL);
-		if (name)
+		if (name) {
+			name = xstrdup(name);
 			for_each_reflog_ent(name, read_one_reflog, reflogs);
+			free((char*)name);
+		}
 	}
 	if (reflogs->nr == 0) {
 		int len = strlen(ref);
-- 
1.7.4.74.g639db

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

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

Indeed the result is very nice and it is almost trivial to see that the
result does not change any behaviour without looking outside the context.

Thanks.

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

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> resolve_ref() may return a pointer to a static buffer. Callers that
> use this value longer than a couple of statements should copy the
> value to avoid some hidden resolve_ref() call that may change the
> static buffer's value.
>
> The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
> demonstrates this. The first call is in cmd_merge()
>
> branch = resolve_ref("HEAD", head_sha1, 0, &flag);
>
> Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
> may be called again and destroy "branch".
>
> lookup_commit_or_die
>  lookup_commit_reference
>   lookup_commit_reference_gently
>    parse_object
>     lookup_replace_object
>      do_lookup_replace_object
>       prepare_replace_object
>        for_each_replace_ref
>         do_for_each_ref
>          get_loose_refs
>           get_ref_dir
>            get_ref_dir
>             resolve_ref
>
> All call sites are checked and made sure that xstrdup() is called if
> the value should be saved.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
> ---
>  Unfortunately there are still 37 resolve_ref() calls. An API change
>  would touch all of them and potentially introduce more bugs along the
>  way, either leak more memory or free() the wrong way.
>
>  So I'd stick with this for v1.7.8.
>
>  builtin/branch.c        |    5 +++-
>  builtin/checkout.c      |    4 ++-
>  builtin/commit.c        |    3 +-
>  builtin/fmt-merge-msg.c |    6 ++++-
>  builtin/merge.c         |   62 ++++++++++++++++++++++++++++++----------------
>  builtin/notes.c         |    6 ++++-
>  builtin/receive-pack.c  |    3 ++
>  reflog-walk.c           |    5 +++-
>  8 files changed, 66 insertions(+), 28 deletions(-)
>
> diff --git a/builtin/branch.c b/builtin/branch.c
> index 0fe9c4d..5b6d839 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -115,8 +115,10 @@ static int branch_merged(int kind, const char *name,
>  		    branch->merge[0] &&
>  		    branch->merge[0]->dst &&
>  		    (reference_name =
> -		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
> +		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {
> +			reference_name = xstrdup(reference_name);
>  			reference_rev = lookup_commit_reference(sha1);
> +		}
>  	}
>  	if (!reference_rev)
>  		reference_rev = head_rev;
> @@ -141,6 +143,7 @@ static int branch_merged(int kind, const char *name,
>  				"         '%s', even though it is merged to HEAD."),
>  				name, reference_name);
>  	}
> +	free((char*)reference_name);
>  	return merged;
>  }

Now reference_name stores the result of xstrdup(), it does not have reason
to be of type "const char *". It is preferable to lose the cast here, I
think. The same comment applies to the remainder of the patch.

Otherwise the patch looks good. Thanks.

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

[Nguyen Thai Ngoc Duy]

2011/11/14 Junio C Hamano <gitster@pobox.com>:
>> diff --git a/builtin/branch.c b/builtin/branch.c
>> index 0fe9c4d..5b6d839 100644
>> --- a/builtin/branch.c
>> +++ b/builtin/branch.c
>> @@ -115,8 +115,10 @@ static int branch_merged(int kind, const char *name,
>>                   branch->merge[0] &&
>>                   branch->merge[0]->dst &&
>>                   (reference_name =
>> -                  resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
>> +                  resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {
>> +                     reference_name = xstrdup(reference_name);
>>                       reference_rev = lookup_commit_reference(sha1);
>> +             }
>>       }
>>       if (!reference_rev)
>>               reference_rev = head_rev;
>> @@ -141,6 +143,7 @@ static int branch_merged(int kind, const char *name,
>>                               "         '%s', even though it is merged to HEAD."),
>>                               name, reference_name);
>>       }
>> +     free((char*)reference_name);
>>       return merged;
>>  }
>
> Now reference_name stores the result of xstrdup(), it does not have reason
> to be of type "const char *". It is preferable to lose the cast here, I
> think. The same comment applies to the remainder of the patch.

But resolve_ref() returns "const char *", we need to type cast at
least once, either at resolve_ref() assignment or at free(), until we
change resolve_ref(). Or should we change resolve_ref() to return
"char *" now?
-- 
Duy

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

[Junio C Hamano]

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

>> Now reference_name stores the result of xstrdup(), it does not have reason
>> to be of type "const char *". It is preferable to lose the cast here, I
>> think. The same comment applies to the remainder of the patch.
>
> But resolve_ref() returns "const char *", we need to type cast at
> least once, either at resolve_ref() assignment or at free(), until we
> change resolve_ref().

In any case, I do not think it matters either way, so I queued this patch
to 'pu' unmodified.

This patch uses xstrdup() on return value of resolve_ref() only at
hand-picked places; while the choice of the places the patch decided not
to call free() looked reasonable from a quick review, both of us may be
blind and it may introduce huge leaks in a repeatedly called function.

A more extensive patch that would turn resolve_ref() to return an
allocated piece of memory share the same risk of adding new leaks at the
callsites, and this late in the cycle, neither will be in 1.7.8 anyway.

Given that if we build the more extensive patch on top of this one after
1.7.8, it will need to undo xstrdup() in this patch, add free()s that
become necessary, in addition to having to add free()s that this patch
might have potentially forgot, I have a feeling that we should just drop
this [2/2] and do a more thorough fix after 1.7.8 release is done
immediately on top of [1/2].

Thanks.

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

[Jeff King]

On Mon, Nov 14, 2011 at 10:32:11AM +0700, Nguyen Thai Ngoc Duy wrote:

> 2011/11/14 Junio C Hamano <gitster@pobox.com>:
> >> diff --git a/builtin/branch.c b/builtin/branch.c
> >> index 0fe9c4d..5b6d839 100644
> >> --- a/builtin/branch.c
> >> +++ b/builtin/branch.c
> >> @@ -115,8 +115,10 @@ static int branch_merged(int kind, const char *name,
> >>                   branch->merge[0] &&
> >>                   branch->merge[0]->dst &&
> >>                   (reference_name =
> >> -                  resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
> >> +                  resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL) {
> >> +                     reference_name = xstrdup(reference_name);
> >>                       reference_rev = lookup_commit_reference(sha1);
> >> +             }
> >>       }
> >>       if (!reference_rev)
> >>               reference_rev = head_rev;
> >> @@ -141,6 +143,7 @@ static int branch_merged(int kind, const char *name,
> >>                               "         '%s', even though it is merged to HEAD."),
> >>                               name, reference_name);
> >>       }
> >> +     free((char*)reference_name);
> >>       return merged;
> >>  }
> >
> > Now reference_name stores the result of xstrdup(), it does not have reason
> > to be of type "const char *". It is preferable to lose the cast here, I
> > think. The same comment applies to the remainder of the patch.
> 
> But resolve_ref() returns "const char *", we need to type cast at
> least once, either at resolve_ref() assignment or at free(), until we
> change resolve_ref(). Or should we change resolve_ref() to return
> "char *" now?

Your problem is that you are using the same variable for two different
things: storing the pointer to non-owned memory returned from
resolve_ref, and then storing the owned memory that comes from xstrdup.
Those two things have different types, since we use "const" on non-owned
memory. Thus you end up casting.

So your code isn't wrong, but I do think it would be more obviously
correct to a reader if it used two variables and dropped the cast.

-Peff

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

[Nguyen Thai Ngoc Duy]

How about an incremental approach like this? On top of 1/1.

[PATCH 01/10] Allow resolve_ref() caller to decide whether to receive static buffer
[PATCH 02/10] cmd_merge: convert to single exit point
[PATCH 03/10] merge: do not point "branch" to a resolve_ref()'s static buffer
[PATCH 04/10] commit: move resolve_ref() closer to where the return value is used
[PATCH 05/10] checkout: do not try xstrdup() on NULL
[PATCH 06/10] reflog-walk.c: request allocated buffer from resolve_ref()
[PATCH 07/10] receive-pack: request resolve_ref() to allocate new buffer
[PATCH 08/10] notes: request resolve_ref() to allocate new buffer
[PATCH 09/10] fmt-merge-msg: request resolve_ref() to allocate new buffer
[PATCH 10/10] branch: request resolve_ref() to allocate new buffer

[PATCH 01/10] Allow resolve_ref() caller to decide whether to receive static buffer

[Nguyễn Thái Ngọc Duy]

resolve_ref() is taught to return a xstrdup'd buffer if alloc is true.
All callers are updated to receive static buffer as before though.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 branch.c                |    2 +-
 builtin/branch.c        |    6 +++---
 builtin/checkout.c      |    2 +-
 builtin/commit.c        |    2 +-
 builtin/fmt-merge-msg.c |    2 +-
 builtin/for-each-ref.c  |    2 +-
 builtin/fsck.c          |    2 +-
 builtin/merge.c         |    2 +-
 builtin/notes.c         |    2 +-
 builtin/receive-pack.c  |    4 ++--
 builtin/remote.c        |    2 +-
 builtin/show-branch.c   |    4 ++--
 builtin/symbolic-ref.c  |    2 +-
 cache.h                 |    2 +-
 reflog-walk.c           |    4 ++--
 refs.c                  |   31 ++++++++++++++++++-------------
 remote.c                |    6 +++---
 transport.c             |    2 +-
 wt-status.c             |    2 +-
 19 files changed, 43 insertions(+), 38 deletions(-)

diff --git a/branch.c b/branch.c
index d809876..a5ee614 100644
--- a/branch.c
+++ b/branch.c
@@ -151,7 +151,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
 		const char *head;
 		unsigned char sha1[20];
 
-		head = resolve_ref("HEAD", sha1, 0, NULL);
+		head = resolve_ref("HEAD", sha1, 0, NULL, 0);
 		if (!is_bare_repository() && head && !strcmp(head, ref->buf))
 			die("Cannot force update the current branch.");
 	}
diff --git a/builtin/branch.c b/builtin/branch.c
index 0fe9c4d..43b494c 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -115,7 +115,7 @@ static int branch_merged(int kind, const char *name,
 		    branch->merge[0] &&
 		    branch->merge[0]->dst &&
 		    (reference_name =
-		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)
+		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL, 0)) != NULL)
 			reference_rev = lookup_commit_reference(sha1);
 	}
 	if (!reference_rev)
@@ -250,7 +250,7 @@ static char *resolve_symref(const char *src, const char *prefix)
 	int flag;
 	const char *dst, *cp;
 
-	dst = resolve_ref(src, sha1, 0, &flag);
+	dst = resolve_ref(src, sha1, 0, &flag, 0);
 	if (!(dst && (flag & REF_ISSYMREF)))
 		return NULL;
 	if (prefix && (cp = skip_prefix(dst, prefix)))
@@ -688,7 +688,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
 
 	track = git_branch_track;
 
-	head = resolve_ref("HEAD", head_sha1, 0, NULL);
+	head = resolve_ref("HEAD", head_sha1, 0, NULL, 0);
 	if (!head)
 		die(_("Failed to resolve HEAD as a valid ref."));
 	head = xstrdup(head);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index beeaee4..f92c737 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -699,7 +699,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	unsigned char rev[20];
 	int flag;
 	memset(&old, 0, sizeof(old));
-	old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag));
+	old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag, 0));
 	old.commit = lookup_commit_reference_gently(rev, 1);
 	if (!(flag & REF_ISSYMREF)) {
 		free((char *)old.path);
diff --git a/builtin/commit.c b/builtin/commit.c
index c46f2d1..11ae005 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	struct commit *commit;
 	struct strbuf format = STRBUF_INIT;
 	unsigned char junk_sha1[20];
-	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL);
+	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL, 0);
 	struct pretty_print_context pctx = {0};
 	struct strbuf author_ident = STRBUF_INIT;
 	struct strbuf committer_ident = STRBUF_INIT;
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 7e2f225..c80f4b7 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -263,7 +263,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 	const char *current_branch;
 
 	/* get current branch */
-	current_branch = resolve_ref("HEAD", head_sha1, 1, NULL);
+	current_branch = resolve_ref("HEAD", head_sha1, 1, NULL, 0);
 	if (!current_branch)
 		die("No current branch");
 	if (!prefixcmp(current_branch, "refs/heads/"))
diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index d90e5d2..09d7d6b 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -629,7 +629,7 @@ static void populate_value(struct refinfo *ref)
 	if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {
 		unsigned char unused1[20];
 		const char *symref;
-		symref = resolve_ref(ref->refname, unused1, 1, NULL);
+		symref = resolve_ref(ref->refname, unused1, 1, NULL, 0);
 		if (symref)
 			ref->symref = xstrdup(symref);
 		else
diff --git a/builtin/fsck.c b/builtin/fsck.c
index df1a88b..4d3d87e 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -532,7 +532,7 @@ static int fsck_head_link(void)
 	if (verbose)
 		fprintf(stderr, "Checking HEAD link\n");
 
-	head_points_at = resolve_ref("HEAD", head_sha1, 0, &flag);
+	head_points_at = resolve_ref("HEAD", head_sha1, 0, &flag, 0);
 	if (!head_points_at)
 		return error("Invalid HEAD");
 	if (!strcmp(head_points_at, "HEAD"))
diff --git a/builtin/merge.c b/builtin/merge.c
index 42b4f9e..d9c7a80 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1095,7 +1095,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 	 * current branch.
 	 */
-	branch = resolve_ref("HEAD", head_sha1, 0, &flag);
+	branch = resolve_ref("HEAD", head_sha1, 0, &flag, 0);
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
diff --git a/builtin/notes.c b/builtin/notes.c
index f8e437d..9c70d8f 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -825,7 +825,7 @@ static int merge_commit(struct notes_merge_options *o)
 	t = xcalloc(1, sizeof(struct notes_tree));
 	init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
 
-	o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL);
+	o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL, 0);
 	if (!o->local_ref)
 		die("Failed to resolve NOTES_MERGE_REF");
 
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 7ec68a1..903f2c5 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -571,7 +571,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)
 	int flag;
 
 	strbuf_addf(&buf, "%s%s", get_git_namespace(), cmd->ref_name);
-	dst_name = resolve_ref(buf.buf, sha1, 0, &flag);
+	dst_name = resolve_ref(buf.buf, sha1, 0, &flag, 0);
 	strbuf_release(&buf);
 
 	if (!(flag & REF_ISSYMREF))
@@ -695,7 +695,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 
 	check_aliased_updates(commands);
 
-	head_name = resolve_ref("HEAD", sha1, 0, NULL);
+	head_name = resolve_ref("HEAD", sha1, 0, NULL, 0);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
 		if (!cmd->skip_update)
diff --git a/builtin/remote.c b/builtin/remote.c
index 407abfb..4a3b529 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -573,7 +573,7 @@ static int read_remote_branches(const char *refname,
 	strbuf_addf(&buf, "refs/remotes/%s/", rename->old);
 	if (!prefixcmp(refname, buf.buf)) {
 		item = string_list_append(rename->remote_branches, xstrdup(refname));
-		symref = resolve_ref(refname, orig_sha1, 1, &flag);
+		symref = resolve_ref(refname, orig_sha1, 1, &flag, 0);
 		if (flag & REF_ISSYMREF)
 			item->util = xstrdup(symref);
 		else
diff --git a/builtin/show-branch.c b/builtin/show-branch.c
index 4b480d7..d8282ad 100644
--- a/builtin/show-branch.c
+++ b/builtin/show-branch.c
@@ -728,7 +728,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 			static const char *fake_av[2];
 			const char *refname;
 
-			refname = resolve_ref("HEAD", sha1, 1, NULL);
+			refname = resolve_ref("HEAD", sha1, 1, NULL, 0);
 			fake_av[0] = xstrdup(refname);
 			fake_av[1] = NULL;
 			av = fake_av;
@@ -791,7 +791,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)
 		}
 	}
 
-	head_p = resolve_ref("HEAD", head_sha1, 1, NULL);
+	head_p = resolve_ref("HEAD", head_sha1, 1, NULL, 0);
 	if (head_p) {
 		head_len = strlen(head_p);
 		memcpy(head, head_p, head_len + 1);
diff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c
index dea849c..50cbdd1 100644
--- a/builtin/symbolic-ref.c
+++ b/builtin/symbolic-ref.c
@@ -12,7 +12,7 @@ static void check_symref(const char *HEAD, int quiet)
 {
 	unsigned char sha1[20];
 	int flag;
-	const char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);
+	const char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag, 0);
 
 	if (!refs_heads_master)
 		die("No such ref: %s", HEAD);
diff --git a/cache.h b/cache.h
index 5badece..b2f0d56 100644
--- a/cache.h
+++ b/cache.h
@@ -866,7 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);
  *
  * errno is sometimes set on errors, but not always.
  */
-extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);
+extern char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag, int alloc);
 
 extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);
 extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);
diff --git a/reflog-walk.c b/reflog-walk.c
index 5d81d39..f71cca6 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,7 +50,7 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 	for_each_reflog_ent(ref, read_one_reflog, reflogs);
 	if (reflogs->nr == 0) {
 		unsigned char sha1[20];
-		const char *name = resolve_ref(ref, sha1, 1, NULL);
+		const char *name = resolve_ref(ref, sha1, 1, NULL, 0);
 		if (name)
 			for_each_reflog_ent(name, read_one_reflog, reflogs);
 	}
@@ -168,7 +168,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,
 	else {
 		if (*branch == '\0') {
 			unsigned char sha1[20];
-			const char *head = resolve_ref("HEAD", sha1, 0, NULL);
+			const char *head = resolve_ref("HEAD", sha1, 0, NULL, 0);
 			if (!head)
 				die ("No current branch");
 			free(branch);
diff --git a/refs.c b/refs.c
index 44c1c86..0387490 100644
--- a/refs.c
+++ b/refs.c
@@ -361,7 +361,7 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha
 	if (!(flags & REF_ISSYMREF))
 		return 0;
 
-	resolves_to = resolve_ref(refname, junk, 0, NULL);
+	resolves_to = resolve_ref(refname, junk, 0, NULL, 0);
 	if (!resolves_to || strcmp(resolves_to, d->refname))
 		return 0;
 
@@ -497,16 +497,19 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)
 	return -1;
 }
 
-const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)
+char *resolve_ref(const char *ref_ro, unsigned char *sha1, int reading, int *flag, int alloc)
 {
 	int depth = MAXDEPTH;
 	ssize_t len;
+	char *ref;
 	char buffer[256];
 	static char ref_buffer[256];
 
 	if (flag)
 		*flag = 0;
 
+	ref = strcpy(ref_buffer, ref_ro);
+
 	if (check_refname_format(ref, REFNAME_ALLOW_ONELEVEL))
 		return NULL;
 
@@ -531,14 +534,14 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 			if (!get_packed_ref(ref, sha1)) {
 				if (flag)
 					*flag |= REF_ISPACKED;
-				return ref;
+				goto done;
 			}
 			/* The reference is not a packed reference, either. */
 			if (reading) {
 				return NULL;
 			} else {
 				hashclr(sha1);
-				return ref;
+				goto done;
 			}
 		}
 
@@ -602,7 +605,9 @@ const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *
 			*flag |= REF_ISBROKEN;
 		return NULL;
 	}
-	return ref;
+
+done:
+	return alloc ? xstrdup(ref) : ref;
 }
 
 /* The argument to filter_refs */
@@ -614,7 +619,7 @@ struct ref_filter {
 
 int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)
 {
-	if (resolve_ref(ref, sha1, reading, flags))
+	if (resolve_ref(ref, sha1, reading, flags, 0))
 		return 0;
 	return -1;
 }
@@ -1118,7 +1123,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)
 
 		this_result = refs_found ? sha1_from_ref : sha1;
 		mksnpath(fullref, sizeof(fullref), *p, len, str);
-		r = resolve_ref(fullref, this_result, 1, &flag);
+		r = resolve_ref(fullref, this_result, 1, &flag, 0);
 		if (r) {
 			if (!refs_found++)
 				*ref = xstrdup(r);
@@ -1148,7 +1153,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)
 		const char *ref, *it;
 
 		mksnpath(path, sizeof(path), *p, len, str);
-		ref = resolve_ref(path, hash, 1, NULL);
+		ref = resolve_ref(path, hash, 1, NULL, 0);
 		if (!ref)
 			continue;
 		if (!stat(git_path("logs/%s", path), &st) &&
@@ -1184,7 +1189,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 	lock = xcalloc(1, sizeof(struct ref_lock));
 	lock->lock_fd = -1;
 
-	ref = resolve_ref(ref, lock->old_sha1, mustexist, &type);
+	ref = resolve_ref(ref, lock->old_sha1, mustexist, &type, 0);
 	if (!ref && errno == EISDIR) {
 		/* we are trying to lock foo but we used to
 		 * have foo/bar which now does not exist;
@@ -1197,7 +1202,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char
 			error("there are still refs under '%s'", orig_ref);
 			goto error_return;
 		}
-		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);
+		ref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type, 0);
 	}
 	if (type_p)
 	    *type_p = type;
@@ -1360,7 +1365,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)
 	if (log && S_ISLNK(loginfo.st_mode))
 		return error("reflog for %s is a symlink", oldref);
 
-	symref = resolve_ref(oldref, orig_sha1, 1, &flag);
+	symref = resolve_ref(oldref, orig_sha1, 1, &flag, 0);
 	if (flag & REF_ISSYMREF)
 		return error("refname %s is a symbolic ref, renaming it is not supported",
 			oldref);
@@ -1649,7 +1654,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unsigned char head_sha1[20];
 		int head_flag;
 		const char *head_ref;
-		head_ref = resolve_ref("HEAD", head_sha1, 1, &head_flag);
+		head_ref = resolve_ref("HEAD", head_sha1, 1, &head_flag, 0);
 		if (head_ref && (head_flag & REF_ISSYMREF) &&
 		    !strcmp(head_ref, lock->ref_name))
 			log_ref_write("HEAD", lock->old_sha1, sha1, logmsg);
@@ -1986,7 +1991,7 @@ int update_ref(const char *action, const char *refname,
 int ref_exists(const char *refname)
 {
 	unsigned char sha1[20];
-	return !!resolve_ref(refname, sha1, 1, NULL);
+	return !!resolve_ref(refname, sha1, 1, NULL, 0);
 }
 
 struct ref *find_ref_by_name(const struct ref *list, const char *name)
diff --git a/remote.c b/remote.c
index 6655bb0..e776ba3 100644
--- a/remote.c
+++ b/remote.c
@@ -482,7 +482,7 @@ static void read_config(void)
 		return;
 	default_remote_name = xstrdup("origin");
 	current_branch = NULL;
-	head_ref = resolve_ref("HEAD", sha1, 0, &flag);
+	head_ref = resolve_ref("HEAD", sha1, 0, &flag, 0);
 	if (head_ref && (flag & REF_ISSYMREF) &&
 	    !prefixcmp(head_ref, "refs/heads/")) {
 		current_branch =
@@ -1007,7 +1007,7 @@ static char *guess_ref(const char *name, struct ref *peer)
 	struct strbuf buf = STRBUF_INIT;
 	unsigned char sha1[20];
 
-	const char *r = resolve_ref(peer->name, sha1, 1, NULL);
+	const char *r = resolve_ref(peer->name, sha1, 1, NULL, 0);
 	if (!r)
 		return NULL;
 
@@ -1058,7 +1058,7 @@ static int match_explicit(struct ref *src, struct ref *dst,
 		unsigned char sha1[20];
 		int flag;
 
-		dst_value = resolve_ref(matched_src->name, sha1, 1, &flag);
+		dst_value = resolve_ref(matched_src->name, sha1, 1, &flag, 0);
 		if (!dst_value ||
 		    ((flag & REF_ISSYMREF) &&
 		     prefixcmp(dst_value, "refs/heads/")))
diff --git a/transport.c b/transport.c
index 51814b5..063c285 100644
--- a/transport.c
+++ b/transport.c
@@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,
 		/* Follow symbolic refs (mainly for HEAD). */
 		localname = ref->peer_ref->name;
 		remotename = ref->name;
-		tmp = resolve_ref(localname, sha, 1, &flag);
+		tmp = resolve_ref(localname, sha, 1, &flag, 0);
 		if (tmp && flag & REF_ISSYMREF &&
 			!prefixcmp(tmp, "refs/heads/"))
 			localname = tmp;
diff --git a/wt-status.c b/wt-status.c
index 70fdb76..96dc603 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -119,7 +119,7 @@ void wt_status_prepare(struct wt_status *s)
 	s->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;
 	s->use_color = -1;
 	s->relative_paths = 1;
-	head = resolve_ref("HEAD", sha1, 0, NULL);
+	head = resolve_ref("HEAD", sha1, 0, NULL, 0);
 	s->branch = head ? xstrdup(head) : NULL;
 	s->reference = "HEAD";
 	s->fp = stdout;
-- 
1.7.4.74.g639db

[PATCH 02/10] cmd_merge: convert to single exit point

[Nguyễn Thái Ngọc Duy]

This makes post-processing easier.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/merge.c |   48 +++++++++++++++++++++++++++++-------------------
 1 files changed, 29 insertions(+), 19 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index d9c7a80..1be6f3b 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1082,7 +1082,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit *head_commit;
 	struct strbuf buf = STRBUF_INIT;
 	const char *head_arg;
-	int flag, i;
+	int flag, i, ret = 0;
 	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
@@ -1121,7 +1121,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			die(_("There is no merge to abort (MERGE_HEAD missing)."));
 
 		/* Invoke 'git reset --merge' */
-		return cmd_reset(nargc, nargv, prefix);
+		ret = cmd_reset(nargc, nargv, prefix);
+		goto done;
 	}
 
 	if (read_cache_unmerged())
@@ -1205,7 +1206,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		read_empty(remote_head->sha1, 0);
 		update_ref("initial pull", "HEAD", remote_head->sha1, NULL, 0,
 				DIE_ON_ERR);
-		return 0;
+		goto done;
 	} else {
 		struct strbuf merge_names = STRBUF_INIT;
 
@@ -1292,7 +1293,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		 * but first the most common case of merging one remote.
 		 */
 		finish_up_to_date("Already up-to-date.");
-		return 0;
+		goto done;
 	} else if (allow_fast_forward && !remoteheads->next &&
 			!common->next &&
 			!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {
@@ -1313,15 +1314,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			strbuf_addstr(&msg,
 				" (no commit created; -m option ignored)");
 		o = want_commit(sha1_to_hex(remoteheads->item->object.sha1));
-		if (!o)
-			return 1;
-
-		if (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))
-			return 1;
+		if (!o ||
+		    checkout_fast_forward(head_commit->object.sha1,
+					  remoteheads->item->object.sha1)) {
+			ret = 1;
+			goto done;
+		}
 
 		finish(head_commit, o->sha1, msg.buf);
 		drop_save();
-		return 0;
+		goto done;
 	} else if (!remoteheads->next && common->next)
 		;
 		/*
@@ -1339,8 +1341,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			git_committer_info(IDENT_ERROR_ON_NO_NAME);
 			printf(_("Trying really trivial in-index merge...\n"));
 			if (!read_tree_trivial(common->item->object.sha1,
-					head_commit->object.sha1, remoteheads->item->object.sha1))
-				return merge_trivial(head_commit);
+					       head_commit->object.sha1,
+					       remoteheads->item->object.sha1)) {
+				ret = merge_trivial(head_commit);
+				goto done;
+			}
 			printf(_("Nope.\n"));
 		}
 	} else {
@@ -1368,7 +1373,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		}
 		if (up_to_date) {
 			finish_up_to_date("Already up-to-date. Yeeah!");
-			return 0;
+			goto done;
 		}
 	}
 
@@ -1450,9 +1455,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * If we have a resulting tree, that means the strategy module
 	 * auto resolved the merge cleanly.
 	 */
-	if (automerge_was_ok)
-		return finish_automerge(head_commit, common, result_tree,
-					wt_strategy);
+	if (automerge_was_ok) {
+		ret = finish_automerge(head_commit, common, result_tree,
+				       wt_strategy);
+		goto done;
+	}
 
 	/*
 	 * Pick the result from the best strategy and have the user fix
@@ -1466,7 +1473,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		else
 			fprintf(stderr, _("Merge with strategy %s failed.\n"),
 				use_strategies[0]->name);
-		return 2;
+		ret = 2;
+		goto done;
 	} else if (best_strategy == wt_strategy)
 		; /* We already have its result in the working tree. */
 	else {
@@ -1485,7 +1493,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	if (merge_was_ok) {
 		fprintf(stderr, _("Automatic merge went well; "
 			"stopped before committing as requested\n"));
-		return 0;
 	} else
-		return suggest_conflicts(option_renormalize);
+		ret = suggest_conflicts(option_renormalize);
+
+done:
+	return ret;
 }
-- 
1.7.4.74.g639db

[PATCH 03/10] merge: do not point "branch" to a resolve_ref()'s static buffer

[Nguyễn Thái Ngọc Duy]

resolve_ref() may return a pointer to a static buffer. Callers that
use this value longer than a couple of statements should copy the
value to avoid some hidden resolve_ref() call that may change the
static buffer's value.

The bug found by Tony Wang <wwwjfy@gmail.com> in builtin/merge.c
demonstrates this. The first call is in cmd_merge()

branch = resolve_ref("HEAD", head_sha1, 0, &flag);

Then deep in lookup_commit_or_die() a few lines after, resolve_ref()
may be called again and destroy "branch".

lookup_commit_or_die
 lookup_commit_reference
  lookup_commit_reference_gently
   parse_object
    lookup_replace_object
     do_lookup_replace_object
      prepare_replace_object
       for_each_replace_ref
        do_for_each_ref
         get_loose_refs
          get_ref_dir
           get_ref_dir
            resolve_ref

Ask resolve_ref() to allocate a new buffer instead of returning static
buffer.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/merge.c |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 1be6f3b..1a3095f 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -1087,6 +1087,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	struct commit_list *common = NULL;
 	const char *best_strategy = NULL, *wt_strategy = NULL;
 	struct commit_list **remotes = &remoteheads;
+	char *branch_ref;
 
 	if (argc == 2 && !strcmp(argv[1], "-h"))
 		usage_with_options(builtin_merge_usage, builtin_merge_options);
@@ -1095,7 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
 	 * current branch.
 	 */
-	branch = resolve_ref("HEAD", head_sha1, 0, &flag, 0);
+	branch = branch_ref = resolve_ref("HEAD", head_sha1, 0, &flag, 1);
 	if (branch && !prefixcmp(branch, "refs/heads/"))
 		branch += 11;
 	if (!branch || is_null_sha1(head_sha1))
@@ -1497,5 +1498,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 		ret = suggest_conflicts(option_renormalize);
 
 done:
+	free(branch_ref);
 	return ret;
 }
-- 
1.7.4.74.g639db

[PATCH 04/10] commit: move resolve_ref() closer to where the return value is used

[Nguyễn Thái Ngọc Duy]

resolve_ref() returns a static buffer and the value may change if
another resolve_ref() is called. Move resolve_ref() closer to printf()
where the value is used to reduce that chance.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/commit.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 11ae005..365b9f6 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	struct commit *commit;
 	struct strbuf format = STRBUF_INIT;
 	unsigned char junk_sha1[20];
-	const char *head = resolve_ref("HEAD", junk_sha1, 0, NULL, 0);
+	const char *head;
 	struct pretty_print_context pctx = {0};
 	struct strbuf author_ident = STRBUF_INIT;
 	struct strbuf committer_ident = STRBUF_INIT;
@@ -1304,6 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,
 	rev.diffopt.break_opt = 0;
 	diff_setup_done(&rev.diffopt);
 
+	head = resolve_ref("HEAD", junk_sha1, 0, NULL, 0);
 	printf("[%s%s ",
 		!prefixcmp(head, "refs/heads/") ?
 			head + 11 :
-- 
1.7.4.74.g639db

[PATCH 05/10] checkout: do not try xstrdup() on NULL

[Nguyễn Thái Ngọc Duy]

Also free memory in the other exit point

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/checkout.c |   13 +++++++------
 1 files changed, 7 insertions(+), 6 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index f92c737..d80e2c4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -696,15 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 {
 	int ret = 0;
 	struct branch_info old;
+	char *path;
 	unsigned char rev[20];
 	int flag;
 	memset(&old, 0, sizeof(old));
-	old.path = xstrdup(resolve_ref("HEAD", rev, 0, &flag, 0));
+	old.path = path = resolve_ref("HEAD", rev, 0, &flag, 1);
 	old.commit = lookup_commit_reference_gently(rev, 1);
-	if (!(flag & REF_ISSYMREF)) {
-		free((char *)old.path);
+	if (!(flag & REF_ISSYMREF))
 		old.path = NULL;
-	}
 
 	if (old.path && !prefixcmp(old.path, "refs/heads/"))
 		old.name = old.path + strlen("refs/heads/");
@@ -718,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	}
 
 	ret = merge_working_tree(opts, &old, new);
-	if (ret)
+	if (ret) {
+		free(path);
 		return ret;
+	}
 
 	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
 		orphaned_commit_warning(old.commit);
@@ -727,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	update_refs_for_switch(opts, &old, new);
 
 	ret = post_checkout_hook(old.commit, new->commit, 1);
-	free((char *)old.path);
+	free(path);
 	return ret || opts->writeout_error;
 }
 
-- 
1.7.4.74.g639db

[PATCH 06/10] reflog-walk.c: request allocated buffer from resolve_ref()

[Nguyễn Thái Ngọc Duy]

for_each_reflog_ent() can do anything, including calling resolve_ref()
again. Therefore it's not safe to use the static buffer returned by
resolve_ref(). Request it to allocate new buffer instead.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 reflog-walk.c |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index f71cca6..00bdff8 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -50,9 +50,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)
 	for_each_reflog_ent(ref, read_one_reflog, reflogs);
 	if (reflogs->nr == 0) {
 		unsigned char sha1[20];
-		const char *name = resolve_ref(ref, sha1, 1, NULL, 0);
-		if (name)
+		char *name = resolve_ref(ref, sha1, 1, NULL, 1);
+		if (name) {
 			for_each_reflog_ent(name, read_one_reflog, reflogs);
+			free(name);
+		}
 	}
 	if (reflogs->nr == 0) {
 		int len = strlen(ref);
-- 
1.7.4.74.g639db

[PATCH 07/10] receive-pack: request resolve_ref() to allocate new buffer

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/receive-pack.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 903f2c5..e447adb 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -36,7 +36,7 @@ static int use_sideband;
 static int prefer_ofs_delta = 1;
 static int auto_update_server_info;
 static int auto_gc = 1;
-static const char *head_name;
+static char *head_name;
 static int sent_capabilities;
 
 static enum deny_action parse_deny_action(const char *var, const char *value)
@@ -695,7 +695,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
 
 	check_aliased_updates(commands);
 
-	head_name = resolve_ref("HEAD", sha1, 0, NULL, 0);
+	free(head_name);
+	head_name = resolve_ref("HEAD", sha1, 0, NULL, 1);
 
 	for (cmd = commands; cmd; cmd = cmd->next)
 		if (!cmd->skip_update)
-- 
1.7.4.74.g639db

[PATCH 08/10] notes: request resolve_ref() to allocate new buffer

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/notes.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index 9c70d8f..80c6e09 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -804,6 +804,8 @@ static int merge_commit(struct notes_merge_options *o)
 	struct notes_tree *t;
 	struct commit *partial;
 	struct pretty_print_context pretty_ctx;
+	char *ref;
+	int ret;
 
 	/*
 	 * Read partial merge result from .git/NOTES_MERGE_PARTIAL,
@@ -825,7 +827,7 @@ static int merge_commit(struct notes_merge_options *o)
 	t = xcalloc(1, sizeof(struct notes_tree));
 	init_notes(t, "NOTES_MERGE_PARTIAL", combine_notes_overwrite, 0);
 
-	o->local_ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL, 0);
+	o->local_ref = ref = resolve_ref("NOTES_MERGE_REF", sha1, 0, NULL, 1);
 	if (!o->local_ref)
 		die("Failed to resolve NOTES_MERGE_REF");
 
@@ -843,7 +845,9 @@ static int merge_commit(struct notes_merge_options *o)
 
 	free_notes(t);
 	strbuf_release(&msg);
-	return merge_abort(o);
+	ret = merge_abort(o);
+	free(ref);
+	return ret;
 }
 
 static int merge(int argc, const char **argv, const char *prefix)
-- 
1.7.4.74.g639db

[PATCH 09/10] fmt-merge-msg: request resolve_ref() to allocate new buffer

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/fmt-merge-msg.c |    8 ++++++--
 1 files changed, 6 insertions(+), 2 deletions(-)

diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index c80f4b7..3d001d4 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -261,9 +261,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 	int i = 0, pos = 0;
 	unsigned char head_sha1[20];
 	const char *current_branch;
+	char *ref;
 
 	/* get current branch */
-	current_branch = resolve_ref("HEAD", head_sha1, 1, NULL, 0);
+	current_branch = ref = resolve_ref("HEAD", head_sha1, 1, NULL, 1);
 	if (!current_branch)
 		die("No current branch");
 	if (!prefixcmp(current_branch, "refs/heads/"))
@@ -283,8 +284,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 			die ("Error in line %d: %.*s", i, len, p);
 	}
 
-	if (!srcs.nr)
+	if (!srcs.nr) {
+		free(ref);
 		return 0;
+	}
 
 	if (merge_title)
 		do_fmt_merge_msg_title(out, current_branch);
@@ -306,6 +309,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,
 			shortlog(origins.items[i].string, origins.items[i].util,
 					head, &rev, shortlog_len, out);
 	}
+	free(ref);
 	return 0;
 }
 
-- 
1.7.4.74.g639db

[PATCH 10/10] branch: request resolve_ref() to allocate new buffer

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/branch.c |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 43b494c..7a31b1e 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -103,7 +103,7 @@ static int branch_merged(int kind, const char *name,
 	 * safely to HEAD (or the other branch).
 	 */
 	struct commit *reference_rev = NULL;
-	const char *reference_name = NULL;
+	char *reference_name = NULL;
 	int merged;
 
 	if (kind == REF_LOCAL_BRANCH) {
@@ -115,7 +115,7 @@ static int branch_merged(int kind, const char *name,
 		    branch->merge[0] &&
 		    branch->merge[0]->dst &&
 		    (reference_name =
-		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL, 0)) != NULL)
+		     resolve_ref(branch->merge[0]->dst, sha1, 1, NULL, 1)) != NULL)
 			reference_rev = lookup_commit_reference(sha1);
 	}
 	if (!reference_rev)
@@ -141,6 +141,7 @@ static int branch_merged(int kind, const char *name,
 				"         '%s', even though it is merged to HEAD."),
 				name, reference_name);
 	}
+	free(reference_name);
 	return merged;
 }
 
-- 
1.7.4.74.g639db

Re: [PATCH 01/10] Allow resolve_ref() caller to decide whether to receive static buffer

[Junio C Hamano]

Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:

> resolve_ref() is taught to return a xstrdup'd buffer if alloc is true.
> All callers are updated to receive static buffer as before though.

Thanks for working on this. I have a slight fear that this may collide
with some topics in flight, but that is something we can worry about after
1.7.8 release is done.

I like the direction of the entire series, and agree with the reasoning
behind the choices of callsites to be converted in later part of the
series (they are clearly documented).

After this series, all the callsites use either 0 or 1 and never a
computed value for the extra argument. I think it would make it a lot
safer and easier for other people who later want to touch the codepath
that has resolve_ref(..., 0) calls in it, if we instead did this in the
following way:

 (1) Rename resolve_ref() to resolve_ref_unsafe() everywhere to make it
     stand out in the code, without changing anything else. This is the
     moral equivalent of a half or your [01/10].

 (2) Introduce resolve_ref() that returns a copy. This is equivalent to
     the other half of your [01/10].

 (3) Convert the ones that should use resolve_ref() to do so. This is
     equivalent to the remainder of your series.

 (4) Convert callsites of resolve_ref_unsafe() that xstrdup()'s the
     returned value back to use resolve_ref() as needed.

 (5) And finally document in in-code comments at each tricky callsites of
     resolve_ref_unsafe() why the use of the unsafe variant is OK in that
     context. This can be done as part of step 1. We obviously do not have
     to document trivial ones (e.g. a variable in a narrow sub-function
     scope receives the return value of resolve_ref_unsafe(), and the
     value is immediately used without giving it a longer lifetime).

The above is primarily a naming issue, but names are important. It forces
developers to _think_ before using something named _unsafe_, and helps
them to be on the lookout when they insert new code between an existing
call to the unsafe one and use of its result.

There are other "unsafe" functions like git_path() whose callsites can be
updated with the same strategy, and we probably should do that.

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

[Tony Wang]

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

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

[Nguyen Thai Ngoc Duy]

On Sat, Dec 10, 2011 at 10:43 AM, Tony Wang <wwwjfy@gmail.com> wrote:
> Hi,
>
> I don't know about the procedure, but wonder is any one following this?

This series has been merged in master. I'll resend patches to rename
resolve_ref() to resolve_ref_unsafe() soon.
-- 
Duy

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

[Tony Wang]

Just notice that, thanks. :)


-- 
BR,
Tony Wang


On Saturday, December 10, 2011 at 12:48, Nguyen Thai Ngoc Duy wrote:

> On Sat, Dec 10, 2011 at 10:43 AM, Tony Wang <wwwjfy@gmail.com (mailto:wwwjfy@gmail.com)> wrote:
> > Hi,
> > 
> > I don't know about the procedure, but wonder is any one following this?
> 
> This series has been merged in master. I'll resend patches to rename
> resolve_ref() to resolve_ref_unsafe() soon.
> -- 
> Duy