{"thread":{"id":"28960","subject":"[PATCH 0/8] nd/resolve-ref v2","startedAt":"2011-11-17T09:32:07Z","lastAt":"2011-12-06T14:07:09Z","messageCount":27,"participants":["Nguyễn Thái Ngọc Duy","Ramkumar Ramachandra","Jonathan Nieder","Jeff King","Nguyen Thai Ngoc Duy","Johan Herland","Junio C Hamano","Bernhard R. Link"],"isPatch":true,"patchVersion":1,"patchTotal":8},"messages":[{"id":"179620","messageId":"1321522335-24193-1-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":null,"subject":"[PATCH 0/8] nd/resolve-ref v2","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:07Z","receivedAt":"2011-11-17T09:32:07Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"The first part of actually nd/resolve-ref v2.\n\nThe last two patches are an attempt to catch overwriting faults in\nfuture. git_pathname() and resolve_ref_unsafe() are guarded.\n\n(Un)fortunately I ran \"make memcheck\" but found no new segfaults.\nEither test coverage is insufficient, or we have done a very good\njob of reviewing/testing git.git\n\nNguyễn Thái Ngọc Duy (8):\n Convert many resolve_ref() calls to read_ref*() and ref_exists()\n Rename resolve_ref() to resolve_ref_unsafe()\n Re-add resolve_ref() that always returns an allocated buffer\n cmd_merge: convert to single exit point\n Use resolve_ref() instead of resolve_ref_unsafe()\n Convert resolve_ref_unsafe+xstrdup to resolve_ref\n Guard memory overwriting in resolve_ref_unsafe's static buffer\n Enable GIT_DEBUG_MEMCHECK on git_pathname()\n\n Makefile | 3 ++\n branch.c | 2 +-\n builtin/branch.c | 11 +++----\n builtin/checkout.c | 17 ++++++------\n builtin/commit.c | 3 +-\n builtin/fmt-merge-msg.c | 8 ++++-\n builtin/for-each-ref.c | 7 +---\n builtin/fsck.c | 2 +-\n builtin/merge.c | 56 +++++++++++++++++++++++++----------------\n builtin/notes.c | 8 ++++-\n builtin/receive-pack.c | 5 ++-\n builtin/remote.c | 10 +++----\n builtin/replace.c | 4 +-\n builtin/show-branch.c | 6 +---\n builtin/show-ref.c | 2 +-\n builtin/symbolic-ref.c | 2 +-\n builtin/tag.c | 4 +-\n bundle.c | 2 +-\n cache.h | 17 +++++++++---\n git-compat-util.h | 9 ++++++\n notes-merge.c | 2 +-\n path.c | 28 ++++++++++++++------\n reflog-walk.c | 13 +++++----\n refs.c | 63 +++++++++++++++++++++++++++++++---------------\n remote.c | 10 +++---\n transport.c | 2 +-\n wrapper.c | 21 +++++++++++++++\n wt-status.c | 4 +--\n 28 files changed, 203 insertions(+), 118 deletions(-)\n\n-- \n1.7.4.74.g639db\n"},{"id":"179621","messageId":"1321522335-24193-2-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 1/8] Convert many resolve_ref() calls to read_ref*() and ref_exists()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:08Z","receivedAt":"2011-11-17T09:32:08Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"resolve_ref() may return a pointer to a static buffer, which is not\nsafe for long-term use because if another resolve_ref() call happens,\nthe buffer may be changed. Many call sites though do not care about\nthis buffer. They simply check if the return value is NULL or not.\n\nConvert all these call sites to new wrappers to reduce resolve_ref()\ncalls from 57 to 34. If we change resolve_ref() prototype later on\nto avoid passing static buffer out, this helps reduce changes.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Junio C Hamano \n---\n builtin/branch.c | 5 ++---\n builtin/checkout.c | 4 ++--\n builtin/merge.c | 4 ++--\n builtin/remote.c | 8 +++-----\n builtin/replace.c | 4 ++--\n builtin/show-ref.c | 2 +-\n builtin/tag.c | 4 ++--\n bundle.c | 2 +-\n cache.h | 2 ++\n notes-merge.c | 2 +-\n refs.c | 27 ++++++++++++++++-----------\n remote.c | 4 ++--\n 12 files changed, 36 insertions(+), 32 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 51ca6a0..0fe9c4d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -186,7 +186,7 @@ static int delete_branches(int argc, const char **argv, int force, int kinds)\n \t\tfree(name);\n \n \t\tname = xstrdup(mkpath(fmt, bname.buf));\n-\t\tif (!resolve_ref(name, sha1, 1, NULL)) {\n+\t\tif (read_ref(name, sha1)) {\n \t\t\terror(_(\"%sbranch '%s' not found.\"),\n \t\t\t\t\tremote, bname.buf);\n \t\t\tret = 1;\n@@ -565,7 +565,6 @@ static int print_ref_list(int kinds, int detached, int verbose, int abbrev, stru\n static void rename_branch(const char *oldname, const char *newname, int force)\n {\n \tstruct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = STRBUF_INIT;\n-\tunsigned char sha1[20];\n \tstruct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;\n \tint recovery = 0;\n \n@@ -577,7 +576,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)\n \t\t * Bad name --- this could be an attempt to rename a\n \t\t * ref that we used to allow to be created by accident.\n \t\t */\n-\t\tif (resolve_ref(oldref.buf, sha1, 1, NULL))\n+\t\tif (ref_exists(oldref.buf))\n \t\t\trecovery = 1;\n \t\telse\n \t\t\tdie(_(\"Invalid branch name: '%s'\"), oldname);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2a80772..beeaee4 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -288,7 +288,7 @@ static int checkout_paths(struct tree *source_tree, const char **pathspec,\n \t commit_locked_index(lock_file))\n \t\tdie(_(\"unable to write new index file\"));\n \n-\tresolve_ref(\"HEAD\", rev, 0, &flag);\n+\tread_ref_full(\"HEAD\", rev, 0, &flag);\n \thead = lookup_commit_reference_gently(rev, 1);\n \n \terrs |= post_checkout_hook(head, head, 0);\n@@ -866,7 +866,7 @@ static int parse_branchname_arg(int argc, const char **argv,\n \tsetup_branch_path(new);\n \n \tif (!check_refname_format(new->path, 0) &&\n-\t resolve_ref(new->path, branch_rev, 1, NULL))\n+\t !read_ref(new->path, branch_rev))\n \t\thashcpy(rev, branch_rev);\n \telse\n \t\tnew->path = NULL; /* not an existing branch */\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex dffd5ec..42b4f9e 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -420,7 +420,7 @@ static struct object *want_commit(const char *name)\n static void merge_name(const char *remote, struct strbuf *msg)\n {\n \tstruct object *remote_head;\n-\tunsigned char branch_head[20], buf_sha[20];\n+\tunsigned char branch_head[20];\n \tstruct strbuf buf = STRBUF_INIT;\n \tstruct strbuf bname = STRBUF_INIT;\n \tconst char *ptr;\n@@ -479,7 +479,7 @@ static void merge_name(const char *remote, struct strbuf *msg)\n \t\tstrbuf_addstr(&truname, \"refs/heads/\");\n \t\tstrbuf_addstr(&truname, remote);\n \t\tstrbuf_setlen(&truname, truname.len - len);\n-\t\tif (resolve_ref(truname.buf, buf_sha, 1, NULL)) {\n+\t\tif (ref_exists(truname.buf)) {\n \t\t\tstrbuf_addf(msg,\n \t\t\t\t \"%s\\t\\tbranch '%s'%s of .\\n\",\n \t\t\t\t sha1_to_hex(remote_head->sha1),\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex c810643..407abfb 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -343,8 +343,7 @@ static int get_ref_states(const struct ref *remote_refs, struct ref_states *stat\n \tstates->tracked.strdup_strings = 1;\n \tstates->stale.strdup_strings = 1;\n \tfor (ref = fetch_map; ref; ref = ref->next) {\n-\t\tunsigned char sha1[20];\n-\t\tif (!ref->peer_ref || read_ref(ref->peer_ref->name, sha1))\n+\t\tif (!ref->peer_ref || !ref_exists(ref->peer_ref->name))\n \t\t\tstring_list_append(&states->new, abbrev_branch(ref->name));\n \t\telse\n \t\t\tstring_list_append(&states->tracked, abbrev_branch(ref->name));\n@@ -710,7 +709,7 @@ static int mv(int argc, const char **argv)\n \t\tint flag = 0;\n \t\tunsigned char sha1[20];\n \n-\t\tresolve_ref(item->string, sha1, 1, &flag);\n+\t\tread_ref_full(item->string, sha1, 1, &flag);\n \t\tif (!(flag & REF_ISSYMREF))\n \t\t\tcontinue;\n \t\tif (delete_ref(item->string, NULL, REF_NODEREF))\n@@ -1220,10 +1219,9 @@ static int set_head(int argc, const char **argv)\n \t\tusage_with_options(builtin_remote_sethead_usage, options);\n \n \tif (head_name) {\n-\t\tunsigned char sha1[20];\n \t\tstrbuf_addf(&buf2, \"refs/remotes/%s/%s\", argv[0], head_name);\n \t\t/* make sure it's valid */\n-\t\tif (!resolve_ref(buf2.buf, sha1, 1, NULL))\n+\t\tif (!ref_exists(buf2.buf))\n \t\t\tresult |= error(\"Not a valid ref: %s\", buf2.buf);\n \t\telse if (create_symref(buf.buf, buf2.buf, \"remote set-head\"))\n \t\t\tresult |= error(\"Could not setup %s\", buf.buf);\ndiff --git a/builtin/replace.c b/builtin/replace.c\nindex 517fa10..4a8970e 100644\n--- a/builtin/replace.c\n+++ b/builtin/replace.c\n@@ -58,7 +58,7 @@ static int for_each_replace_name(const char **argv, each_replace_name_fn fn)\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!resolve_ref(ref, sha1, 1, NULL)) {\n+\t\tif (read_ref(ref, sha1)) {\n \t\t\terror(\"replace ref '%s' not found.\", *p);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n@@ -97,7 +97,7 @@ static int replace_object(const char *object_ref, const char *replace_ref,\n \tif (check_refname_format(ref, 0))\n \t\tdie(\"'%s' is not a valid ref name.\", ref);\n \n-\tif (!resolve_ref(ref, prev, 1, NULL))\n+\tif (read_ref(ref, prev))\n \t\thashclr(prev);\n \telse if (!force)\n \t\tdie(\"replace ref '%s' already exists\", ref);\ndiff --git a/builtin/show-ref.c b/builtin/show-ref.c\nindex fafb6dd..3911661 100644\n--- a/builtin/show-ref.c\n+++ b/builtin/show-ref.c\n@@ -225,7 +225,7 @@ int cmd_show_ref(int argc, const char **argv, const char *prefix)\n \t\t\tunsigned char sha1[20];\n \n \t\t\tif (!prefixcmp(*pattern, \"refs/\") &&\n-\t\t\t resolve_ref(*pattern, sha1, 1, NULL)) {\n+\t\t\t !read_ref(*pattern, sha1)) {\n \t\t\t\tif (!quiet)\n \t\t\t\t\tshow_one(*pattern, sha1);\n \t\t\t}\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 9b6fd95..439249d 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -174,7 +174,7 @@ static int for_each_tag_name(const char **argv, each_tag_name_fn fn)\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n \t\t}\n-\t\tif (!resolve_ref(ref, sha1, 1, NULL)) {\n+\t\tif (read_ref(ref, sha1)) {\n \t\t\terror(_(\"tag '%s' not found.\"), *p);\n \t\t\thad_error = 1;\n \t\t\tcontinue;\n@@ -518,7 +518,7 @@ int cmd_tag(int argc, const char **argv, const char *prefix)\n \tif (strbuf_check_tag_ref(&ref, tag))\n \t\tdie(_(\"'%s' is not a valid tag name.\"), tag);\n \n-\tif (!resolve_ref(ref.buf, prev, 1, NULL))\n+\tif (read_ref(ref.buf, prev))\n \t\thashclr(prev);\n \telse if (!force)\n \t\tdie(_(\"tag '%s' already exists\"), tag);\ndiff --git a/bundle.c b/bundle.c\nindex 08020bc..4742f27 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -320,7 +320,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \t\t\tcontinue;\n \t\tif (dwim_ref(e->name, strlen(e->name), sha1, &ref) != 1)\n \t\t\tcontinue;\n-\t\tif (!resolve_ref(e->name, sha1, 1, &flag))\n+\t\tif (read_ref_full(e->name, sha1, 1, &flag))\n \t\t\tflag = 0;\n \t\tdisplay_ref = (flag & REF_ISSYMREF) ? e->name : ref;\n \ndiff --git a/cache.h b/cache.h\nindex 2e6ad36..5badece 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -832,6 +832,8 @@ static inline int get_sha1_with_context(const char *str, unsigned char *sha1, st\n extern int get_sha1_hex(const char *hex, unsigned char *sha1);\n \n extern char *sha1_to_hex(const unsigned char *sha1);\t/* static buffer result! */\n+extern int read_ref_full(const char *filename, unsigned char *sha1,\n+\t\t\t int reading, int *flags);\n extern int read_ref(const char *filename, unsigned char *sha1);\n \n /*\ndiff --git a/notes-merge.c b/notes-merge.c\nindex e9e4199..e33c2c9 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -568,7 +568,7 @@ int notes_merge(struct notes_merge_options *o,\n \t o->local_ref, o->remote_ref);\n \n \t/* Dereference o->local_ref into local_sha1 */\n-\tif (!resolve_ref(o->local_ref, local_sha1, 0, NULL))\n+\tif (read_ref_full(o->local_ref, local_sha1, 0, NULL))\n \t\tdie(\"Failed to resolve local notes ref '%s'\", o->local_ref);\n \telse if (!check_refname_format(o->local_ref, 0) &&\n \t\tis_null_sha1(local_sha1))\ndiff --git a/refs.c b/refs.c\nindex e69ba26..44c1c86 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -334,7 +334,7 @@ static void get_ref_dir(const char *submodule, const char *base,\n \t\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t\t}\n \t\t\t} else\n-\t\t\t\tif (!resolve_ref(ref, sha1, 1, &flag)) {\n+\t\t\t\tif (read_ref_full(ref, sha1, 1, &flag)) {\n \t\t\t\t\thashclr(sha1);\n \t\t\t\t\tflag |= REF_ISBROKEN;\n \t\t\t\t}\n@@ -612,13 +612,18 @@ struct ref_filter {\n \tvoid *cb_data;\n };\n \n-int read_ref(const char *ref, unsigned char *sha1)\n+int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)\n {\n-\tif (resolve_ref(ref, sha1, 1, NULL))\n+\tif (resolve_ref(ref, sha1, reading, flags))\n \t\treturn 0;\n \treturn -1;\n }\n \n+int read_ref(const char *ref, unsigned char *sha1)\n+{\n+\treturn read_ref_full(ref, sha1, 1, NULL);\n+}\n+\n #define DO_FOR_EACH_INCLUDE_BROKEN 01\n static int do_one_ref(const char *base, each_ref_fn fn, int trim,\n \t\t int flags, void *cb_data, struct ref_entry *entry)\n@@ -663,7 +668,7 @@ int peel_ref(const char *ref, unsigned char *sha1)\n \t\tgoto fallback;\n \t}\n \n-\tif (!resolve_ref(ref, base, 1, &flag))\n+\tif (read_ref_full(ref, base, 1, &flag))\n \t\treturn -1;\n \n \tif ((flag & REF_ISPACKED)) {\n@@ -746,7 +751,7 @@ static int do_head_ref(const char *submodule, each_ref_fn fn, void *cb_data)\n \t\treturn 0;\n \t}\n \n-\tif (resolve_ref(\"HEAD\", sha1, 1, &flag))\n+\tif (!read_ref_full(\"HEAD\", sha1, 1, &flag))\n \t\treturn fn(\"HEAD\", sha1, flag, cb_data);\n \n \treturn 0;\n@@ -826,7 +831,7 @@ int head_ref_namespaced(each_ref_fn fn, void *cb_data)\n \tint flag;\n \n \tstrbuf_addf(&buf, \"%sHEAD\", get_git_namespace());\n-\tif (resolve_ref(buf.buf, sha1, 1, &flag))\n+\tif (!read_ref_full(buf.buf, sha1, 1, &flag))\n \t\tret = fn(buf.buf, sha1, flag, cb_data);\n \tstrbuf_release(&buf);\n \n@@ -1022,7 +1027,7 @@ int refname_match(const char *abbrev_name, const char *full_name, const char **r\n static struct ref_lock *verify_lock(struct ref_lock *lock,\n \tconst unsigned char *old_sha1, int mustexist)\n {\n-\tif (!resolve_ref(lock->ref_name, lock->old_sha1, mustexist, NULL)) {\n+\tif (read_ref_full(lock->ref_name, lock->old_sha1, mustexist, NULL)) {\n \t\terror(\"Can't verify ref %s\", lock->ref_name);\n \t\tunlock_ref(lock);\n \t\treturn NULL;\n@@ -1377,7 +1382,8 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \t\tgoto rollback;\n \t}\n \n-\tif (resolve_ref(newref, sha1, 1, &flag) && delete_ref(newref, sha1, REF_NODEREF)) {\n+\tif (!read_ref_full(newref, sha1, 1, &flag) &&\n+\t delete_ref(newref, sha1, REF_NODEREF)) {\n \t\tif (errno==EISDIR) {\n \t\t\tif (remove_empty_directories(git_path(\"%s\", newref))) {\n \t\t\t\terror(\"Directory not empty: %s\", newref);\n@@ -1929,7 +1935,7 @@ static int do_for_each_reflog(const char *base, each_ref_fn fn, void *cb_data)\n \t\t\t\tretval = do_for_each_reflog(log, fn, cb_data);\n \t\t\t} else {\n \t\t\t\tunsigned char sha1[20];\n-\t\t\t\tif (!resolve_ref(log, sha1, 0, NULL))\n+\t\t\t\tif (read_ref_full(log, sha1, 0, NULL))\n \t\t\t\t\tretval = error(\"bad ref for %s\", log);\n \t\t\t\telse\n \t\t\t\t\tretval = fn(log, sha1, 0, cb_data);\n@@ -2072,7 +2078,6 @@ char *shorten_unambiguous_ref(const char *ref, int strict)\n \t\t */\n \t\tfor (j = 0; j < rules_to_fail; j++) {\n \t\t\tconst char *rule = ref_rev_parse_rules[j];\n-\t\t\tunsigned char short_objectname[20];\n \t\t\tchar refname[PATH_MAX];\n \n \t\t\t/* skip matched rule */\n@@ -2086,7 +2091,7 @@ char *shorten_unambiguous_ref(const char *ref, int strict)\n \t\t\t */\n \t\t\tmksnpath(refname, sizeof(refname),\n \t\t\t\t rule, short_name_len, short_name);\n-\t\t\tif (!read_ref(refname, short_objectname))\n+\t\t\tif (ref_exists(refname))\n \t\t\t\tbreak;\n \t\t}\n \ndiff --git a/remote.c b/remote.c\nindex e2ef991..6655bb0 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -1507,13 +1507,13 @@ int stat_tracking_info(struct branch *branch, int *num_ours, int *num_theirs)\n \t * nothing to report.\n \t */\n \tbase = branch->merge[0]->dst;\n-\tif (!resolve_ref(base, sha1, 1, NULL))\n+\tif (read_ref(base, sha1))\n \t\treturn 0;\n \ttheirs = lookup_commit_reference(sha1);\n \tif (!theirs)\n \t\treturn 0;\n \n-\tif (!resolve_ref(branch->refname, sha1, 1, NULL))\n+\tif (read_ref(branch->refname, sha1))\n \t\treturn 0;\n \tours = lookup_commit_reference(sha1);\n \tif (!ours)\n-- \n1.7.4.74.g639db\n"},{"id":"179622","messageId":"1321522335-24193-3-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 2/8] Rename resolve_ref() to resolve_ref_unsafe()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:09Z","receivedAt":"2011-11-17T09:32:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"resolve_ref() may return a pointer to a shared buffer and can be\noverwritten by the next resolve_ref() calls. Callers need to\npay attention, not to keep the pointer when the next call happens.\n\nRename with \"_unsafe\" suffix to warn developers (or reviewers) before\nintroducing new call sites.\n\nThis patch is generated using this command\n\ngit grep -l 'resolve_ref(' -- '*.[ch]'|xargs sed -i 's/resolve_ref(/resolve_ref_unsafe(/g'\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n branch.c | 2 +-\n builtin/branch.c | 6 +++---\n builtin/checkout.c | 2 +-\n builtin/commit.c | 2 +-\n builtin/fmt-merge-msg.c | 2 +-\n builtin/for-each-ref.c | 2 +-\n builtin/fsck.c | 2 +-\n builtin/merge.c | 2 +-\n builtin/notes.c | 2 +-\n builtin/receive-pack.c | 4 ++--\n builtin/remote.c | 2 +-\n builtin/show-branch.c | 4 ++--\n builtin/symbolic-ref.c | 2 +-\n cache.h | 2 +-\n reflog-walk.c | 4 ++--\n refs.c | 20 ++++++++++----------\n remote.c | 6 +++---\n transport.c | 2 +-\n wt-status.c | 2 +-\n 19 files changed, 35 insertions(+), 35 deletions(-)\n\ndiff --git a/branch.c b/branch.c\nindex d809876..243355b 100644\n--- a/branch.c\n+++ b/branch.c\n@@ -151,7 +151,7 @@ int validate_new_branchname(const char *name, struct strbuf *ref,\n \t\tconst char *head;\n \t\tunsigned char sha1[20];\n \n-\t\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\t\thead = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n \t\tif (!is_bare_repository() && head && !strcmp(head, ref->buf))\n \t\t\tdie(\"Cannot force update the current branch.\");\n \t}\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 0fe9c4d..6903b0d 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -115,7 +115,7 @@ static int branch_merged(int kind, const char *name,\n \t\t branch->merge[0] &&\n \t\t branch->merge[0]->dst &&\n \t\t (reference_name =\n-\t\t resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n+\t\t resolve_ref_unsafe(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n \t\t\treference_rev = lookup_commit_reference(sha1);\n \t}\n \tif (!reference_rev)\n@@ -250,7 +250,7 @@ static char *resolve_symref(const char *src, const char *prefix)\n \tint flag;\n \tconst char *dst, *cp;\n \n-\tdst = resolve_ref(src, sha1, 0, &flag);\n+\tdst = resolve_ref_unsafe(src, sha1, 0, &flag);\n \tif (!(dst && (flag & REF_ISSYMREF)))\n \t\treturn NULL;\n \tif (prefix && (cp = skip_prefix(dst, prefix)))\n@@ -688,7 +688,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \ttrack = git_branch_track;\n \n-\thead = resolve_ref(\"HEAD\", head_sha1, 0, NULL);\n+\thead = resolve_ref_unsafe(\"HEAD\", head_sha1, 0, NULL);\n \tif (!head)\n \t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n \thead = xstrdup(head);\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex beeaee4..2b8e73b 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -699,7 +699,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \tunsigned char rev[20];\n \tint flag;\n \tmemset(&old, 0, sizeof(old));\n-\told.path = xstrdup(resolve_ref(\"HEAD\", rev, 0, &flag));\n+\told.path = xstrdup(resolve_ref_unsafe(\"HEAD\", rev, 0, &flag));\n \told.commit = lookup_commit_reference_gently(rev, 1);\n \tif (!(flag & REF_ISSYMREF)) {\n \t\tfree((char *)old.path);\ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex c46f2d1..0be3b45 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \tstruct commit *commit;\n \tstruct strbuf format = STRBUF_INIT;\n \tunsigned char junk_sha1[20];\n-\tconst char *head = resolve_ref(\"HEAD\", junk_sha1, 0, NULL);\n+\tconst char *head = resolve_ref_unsafe(\"HEAD\", junk_sha1, 0, NULL);\n \tstruct pretty_print_context pctx = {0};\n \tstruct strbuf author_ident = STRBUF_INIT;\n \tstruct strbuf committer_ident = STRBUF_INIT;\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 7e2f225..5c9b40e 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -263,7 +263,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,\n \tconst char *current_branch;\n \n \t/* get current branch */\n-\tcurrent_branch = resolve_ref(\"HEAD\", head_sha1, 1, NULL);\n+\tcurrent_branch = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n \tif (!current_branch)\n \t\tdie(\"No current branch\");\n \tif (!prefixcmp(current_branch, \"refs/heads/\"))\ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex d90e5d2..b954ca8 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -629,7 +629,7 @@ static void populate_value(struct refinfo *ref)\n \tif (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {\n \t\tunsigned char unused1[20];\n \t\tconst char *symref;\n-\t\tsymref = resolve_ref(ref->refname, unused1, 1, NULL);\n+\t\tsymref = resolve_ref_unsafe(ref->refname, unused1, 1, NULL);\n \t\tif (symref)\n \t\t\tref->symref = xstrdup(symref);\n \t\telse\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex df1a88b..0e0e17a 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -532,7 +532,7 @@ static int fsck_head_link(void)\n \tif (verbose)\n \t\tfprintf(stderr, \"Checking HEAD link\\n\");\n \n-\thead_points_at = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n+\thead_points_at = resolve_ref_unsafe(\"HEAD\", head_sha1, 0, &flag);\n \tif (!head_points_at)\n \t\treturn error(\"Invalid HEAD\");\n \tif (!strcmp(head_points_at, \"HEAD\"))\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 42b4f9e..519e3c5 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1095,7 +1095,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n \t * current branch.\n \t */\n-\tbranch = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n+\tbranch = resolve_ref_unsafe(\"HEAD\", head_sha1, 0, &flag);\n \tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n \t\tbranch += 11;\n \tif (!branch || is_null_sha1(head_sha1))\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f8e437d..e191ce6 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -825,7 +825,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tt = xcalloc(1, sizeof(struct notes_tree));\n \tinit_notes(t, \"NOTES_MERGE_PARTIAL\", combine_notes_overwrite, 0);\n \n-\to->local_ref = resolve_ref(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n+\to->local_ref = resolve_ref_unsafe(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n \tif (!o->local_ref)\n \t\tdie(\"Failed to resolve NOTES_MERGE_REF\");\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 7ec68a1..333f2b0 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -571,7 +571,7 @@ static void check_aliased_update(struct command *cmd, struct string_list *list)\n \tint flag;\n \n \tstrbuf_addf(&buf, \"%s%s\", get_git_namespace(), cmd->ref_name);\n-\tdst_name = resolve_ref(buf.buf, sha1, 0, &flag);\n+\tdst_name = resolve_ref_unsafe(buf.buf, sha1, 0, &flag);\n \tstrbuf_release(&buf);\n \n \tif (!(flag & REF_ISSYMREF))\n@@ -695,7 +695,7 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tcheck_aliased_updates(commands);\n \n-\thead_name = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\thead_name = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n \n \tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\tif (!cmd->skip_update)\ndiff --git a/builtin/remote.c b/builtin/remote.c\nindex 407abfb..583eec9 100644\n--- a/builtin/remote.c\n+++ b/builtin/remote.c\n@@ -573,7 +573,7 @@ static int read_remote_branches(const char *refname,\n \tstrbuf_addf(&buf, \"refs/remotes/%s/\", rename->old);\n \tif (!prefixcmp(refname, buf.buf)) {\n \t\titem = string_list_append(rename->remote_branches, xstrdup(refname));\n-\t\tsymref = resolve_ref(refname, orig_sha1, 1, &flag);\n+\t\tsymref = resolve_ref_unsafe(refname, orig_sha1, 1, &flag);\n \t\tif (flag & REF_ISSYMREF)\n \t\t\titem->util = xstrdup(symref);\n \t\telse\ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 4b480d7..1e7bd31 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -728,7 +728,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t\tstatic const char *fake_av[2];\n \t\t\tconst char *refname;\n \n-\t\t\trefname = resolve_ref(\"HEAD\", sha1, 1, NULL);\n+\t\t\trefname = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n \t\t\tfake_av[0] = xstrdup(refname);\n \t\t\tfake_av[1] = NULL;\n \t\t\tav = fake_av;\n@@ -791,7 +791,7 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \t\t}\n \t}\n \n-\thead_p = resolve_ref(\"HEAD\", head_sha1, 1, NULL);\n+\thead_p = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n \tif (head_p) {\n \t\thead_len = strlen(head_p);\n \t\tmemcpy(head, head_p, head_len + 1);\ndiff --git a/builtin/symbolic-ref.c b/builtin/symbolic-ref.c\nindex dea849c..2ef5962 100644\n--- a/builtin/symbolic-ref.c\n+++ b/builtin/symbolic-ref.c\n@@ -12,7 +12,7 @@ static void check_symref(const char *HEAD, int quiet)\n {\n \tunsigned char sha1[20];\n \tint flag;\n-\tconst char *refs_heads_master = resolve_ref(HEAD, sha1, 0, &flag);\n+\tconst char *refs_heads_master = resolve_ref_unsafe(HEAD, sha1, 0, &flag);\n \n \tif (!refs_heads_master)\n \t\tdie(\"No such ref: %s\", HEAD);\ndiff --git a/cache.h b/cache.h\nindex 5badece..61f023a 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -866,7 +866,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n *\n * errno is sometimes set on errors, but not always.\n */\n-extern const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n+extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 5d81d39..b9b2453 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -50,7 +50,7 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)\n \tfor_each_reflog_ent(ref, read_one_reflog, reflogs);\n \tif (reflogs->nr == 0) {\n \t\tunsigned char sha1[20];\n-\t\tconst char *name = resolve_ref(ref, sha1, 1, NULL);\n+\t\tconst char *name = resolve_ref_unsafe(ref, sha1, 1, NULL);\n \t\tif (name)\n \t\t\tfor_each_reflog_ent(name, read_one_reflog, reflogs);\n \t}\n@@ -168,7 +168,7 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \telse {\n \t\tif (*branch == '\\0') {\n \t\t\tunsigned char sha1[20];\n-\t\t\tconst char *head = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\t\t\tconst char *head = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n \t\t\tif (!head)\n \t\t\t\tdie (\"No current branch\");\n \t\t\tfree(branch);\ndiff --git a/refs.c b/refs.c\nindex 44c1c86..9e42e36 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -361,7 +361,7 @@ static int warn_if_dangling_symref(const char *refname, const unsigned char *sha\n \tif (!(flags & REF_ISSYMREF))\n \t\treturn 0;\n \n-\tresolves_to = resolve_ref(refname, junk, 0, NULL);\n+\tresolves_to = resolve_ref_unsafe(refname, junk, 0, NULL);\n \tif (!resolves_to || strcmp(resolves_to, d->refname))\n \t\treturn 0;\n \n@@ -497,7 +497,7 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)\n \treturn -1;\n }\n \n-const char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)\n+const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag)\n {\n \tint depth = MAXDEPTH;\n \tssize_t len;\n@@ -614,7 +614,7 @@ struct ref_filter {\n \n int read_ref_full(const char *ref, unsigned char *sha1, int reading, int *flags)\n {\n-\tif (resolve_ref(ref, sha1, reading, flags))\n+\tif (resolve_ref_unsafe(ref, sha1, reading, flags))\n \t\treturn 0;\n \treturn -1;\n }\n@@ -1118,7 +1118,7 @@ int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref)\n \n \t\tthis_result = refs_found ? sha1_from_ref : sha1;\n \t\tmksnpath(fullref, sizeof(fullref), *p, len, str);\n-\t\tr = resolve_ref(fullref, this_result, 1, &flag);\n+\t\tr = resolve_ref_unsafe(fullref, this_result, 1, &flag);\n \t\tif (r) {\n \t\t\tif (!refs_found++)\n \t\t\t\t*ref = xstrdup(r);\n@@ -1148,7 +1148,7 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log)\n \t\tconst char *ref, *it;\n \n \t\tmksnpath(path, sizeof(path), *p, len, str);\n-\t\tref = resolve_ref(path, hash, 1, NULL);\n+\t\tref = resolve_ref_unsafe(path, hash, 1, NULL);\n \t\tif (!ref)\n \t\t\tcontinue;\n \t\tif (!stat(git_path(\"logs/%s\", path), &st) &&\n@@ -1184,7 +1184,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \tlock = xcalloc(1, sizeof(struct ref_lock));\n \tlock->lock_fd = -1;\n \n-\tref = resolve_ref(ref, lock->old_sha1, mustexist, &type);\n+\tref = resolve_ref_unsafe(ref, lock->old_sha1, mustexist, &type);\n \tif (!ref && errno == EISDIR) {\n \t\t/* we are trying to lock foo but we used to\n \t\t * have foo/bar which now does not exist;\n@@ -1197,7 +1197,7 @@ static struct ref_lock *lock_ref_sha1_basic(const char *ref, const unsigned char\n \t\t\terror(\"there are still refs under '%s'\", orig_ref);\n \t\t\tgoto error_return;\n \t\t}\n-\t\tref = resolve_ref(orig_ref, lock->old_sha1, mustexist, &type);\n+\t\tref = resolve_ref_unsafe(orig_ref, lock->old_sha1, mustexist, &type);\n \t}\n \tif (type_p)\n \t *type_p = type;\n@@ -1360,7 +1360,7 @@ int rename_ref(const char *oldref, const char *newref, const char *logmsg)\n \tif (log && S_ISLNK(loginfo.st_mode))\n \t\treturn error(\"reflog for %s is a symlink\", oldref);\n \n-\tsymref = resolve_ref(oldref, orig_sha1, 1, &flag);\n+\tsymref = resolve_ref_unsafe(oldref, orig_sha1, 1, &flag);\n \tif (flag & REF_ISSYMREF)\n \t\treturn error(\"refname %s is a symbolic ref, renaming it is not supported\",\n \t\t\toldref);\n@@ -1649,7 +1649,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunsigned char head_sha1[20];\n \t\tint head_flag;\n \t\tconst char *head_ref;\n-\t\thead_ref = resolve_ref(\"HEAD\", head_sha1, 1, &head_flag);\n+\t\thead_ref = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, &head_flag);\n \t\tif (head_ref && (head_flag & REF_ISSYMREF) &&\n \t\t !strcmp(head_ref, lock->ref_name))\n \t\t\tlog_ref_write(\"HEAD\", lock->old_sha1, sha1, logmsg);\n@@ -1986,7 +1986,7 @@ int update_ref(const char *action, const char *refname,\n int ref_exists(const char *refname)\n {\n \tunsigned char sha1[20];\n-\treturn !!resolve_ref(refname, sha1, 1, NULL);\n+\treturn !!resolve_ref_unsafe(refname, sha1, 1, NULL);\n }\n \n struct ref *find_ref_by_name(const struct ref *list, const char *name)\ndiff --git a/remote.c b/remote.c\nindex 6655bb0..73a3809 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -482,7 +482,7 @@ static void read_config(void)\n \t\treturn;\n \tdefault_remote_name = xstrdup(\"origin\");\n \tcurrent_branch = NULL;\n-\thead_ref = resolve_ref(\"HEAD\", sha1, 0, &flag);\n+\thead_ref = resolve_ref_unsafe(\"HEAD\", sha1, 0, &flag);\n \tif (head_ref && (flag & REF_ISSYMREF) &&\n \t !prefixcmp(head_ref, \"refs/heads/\")) {\n \t\tcurrent_branch =\n@@ -1007,7 +1007,7 @@ static char *guess_ref(const char *name, struct ref *peer)\n \tstruct strbuf buf = STRBUF_INIT;\n \tunsigned char sha1[20];\n \n-\tconst char *r = resolve_ref(peer->name, sha1, 1, NULL);\n+\tconst char *r = resolve_ref_unsafe(peer->name, sha1, 1, NULL);\n \tif (!r)\n \t\treturn NULL;\n \n@@ -1058,7 +1058,7 @@ static int match_explicit(struct ref *src, struct ref *dst,\n \t\tunsigned char sha1[20];\n \t\tint flag;\n \n-\t\tdst_value = resolve_ref(matched_src->name, sha1, 1, &flag);\n+\t\tdst_value = resolve_ref_unsafe(matched_src->name, sha1, 1, &flag);\n \t\tif (!dst_value ||\n \t\t ((flag & REF_ISSYMREF) &&\n \t\t prefixcmp(dst_value, \"refs/heads/\")))\ndiff --git a/transport.c b/transport.c\nindex 51814b5..e9797c0 100644\n--- a/transport.c\n+++ b/transport.c\n@@ -163,7 +163,7 @@ static void set_upstreams(struct transport *transport, struct ref *refs,\n \t\t/* Follow symbolic refs (mainly for HEAD). */\n \t\tlocalname = ref->peer_ref->name;\n \t\tremotename = ref->name;\n-\t\ttmp = resolve_ref(localname, sha, 1, &flag);\n+\t\ttmp = resolve_ref_unsafe(localname, sha, 1, &flag);\n \t\tif (tmp && flag & REF_ISSYMREF &&\n \t\t\t!prefixcmp(tmp, \"refs/heads/\"))\n \t\t\tlocalname = tmp;\ndiff --git a/wt-status.c b/wt-status.c\nindex 70fdb76..cc6dad5 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -119,7 +119,7 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;\n \ts->use_color = -1;\n \ts->relative_paths = 1;\n-\thead = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\thead = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n \ts->branch = head ? xstrdup(head) : NULL;\n \ts->reference = \"HEAD\";\n \ts->fp = stdout;\n-- \n1.7.4.74.g639db\n"},{"id":"179623","messageId":"1321522335-24193-4-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 3/8] Re-add resolve_ref() that always returns an allocated buffer","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:10Z","receivedAt":"2011-11-17T09:32:10Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache.h | 1 +\n refs.c | 6 ++++++\n 2 files changed, 7 insertions(+), 0 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 61f023a..6b8ac8b 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -867,6 +867,7 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n * errno is sometimes set on errors, but not always.\n */\n extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n+extern char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\n extern int dwim_log(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/refs.c b/refs.c\nindex 9e42e36..28496ed 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -605,6 +605,12 @@ const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading\n \treturn ref;\n }\n \n+char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag)\n+{\n+\tconst char *ret = resolve_ref_unsafe(ref, sha1, reading, flag);\n+\treturn ret ? xstrdup(ret) : NULL;\n+}\n+\n /* The argument to filter_refs */\n struct ref_filter {\n \tconst char *pattern;\n-- \n1.7.4.74.g639db\n"},{"id":"179624","messageId":"1321522335-24193-5-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 4/8] cmd_merge: convert to single exit point","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:11Z","receivedAt":"2011-11-17T09:32:11Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"This makes post-processing easier.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/merge.c | 48 +++++++++++++++++++++++++++++-------------------\n 1 files changed, 29 insertions(+), 19 deletions(-)\n\ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 519e3c5..0d597b3 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1082,7 +1082,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit *head_commit;\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *head_arg;\n-\tint flag, i;\n+\tint flag, i, ret = 0;\n \tint best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n@@ -1121,7 +1121,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tdie(_(\"There is no merge to abort (MERGE_HEAD missing).\"));\n \n \t\t/* Invoke 'git reset --merge' */\n-\t\treturn cmd_reset(nargc, nargv, prefix);\n+\t\tret = cmd_reset(nargc, nargv, prefix);\n+\t\tgoto done;\n \t}\n \n \tif (read_cache_unmerged())\n@@ -1205,7 +1206,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tread_empty(remote_head->sha1, 0);\n \t\tupdate_ref(\"initial pull\", \"HEAD\", remote_head->sha1, NULL, 0,\n \t\t\t\tDIE_ON_ERR);\n-\t\treturn 0;\n+\t\tgoto done;\n \t} else {\n \t\tstruct strbuf merge_names = STRBUF_INIT;\n \n@@ -1292,7 +1293,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t * but first the most common case of merging one remote.\n \t\t */\n \t\tfinish_up_to_date(\"Already up-to-date.\");\n-\t\treturn 0;\n+\t\tgoto done;\n \t} else if (allow_fast_forward && !remoteheads->next &&\n \t\t\t!common->next &&\n \t\t\t!hashcmp(common->item->object.sha1, head_commit->object.sha1)) {\n@@ -1313,15 +1314,16 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tstrbuf_addstr(&msg,\n \t\t\t\t\" (no commit created; -m option ignored)\");\n \t\to = want_commit(sha1_to_hex(remoteheads->item->object.sha1));\n-\t\tif (!o)\n-\t\t\treturn 1;\n-\n-\t\tif (checkout_fast_forward(head_commit->object.sha1, remoteheads->item->object.sha1))\n-\t\t\treturn 1;\n+\t\tif (!o ||\n+\t\t checkout_fast_forward(head_commit->object.sha1,\n+\t\t\t\t\t remoteheads->item->object.sha1)) {\n+\t\t\tret = 1;\n+\t\t\tgoto done;\n+\t\t}\n \n \t\tfinish(head_commit, o->sha1, msg.buf);\n \t\tdrop_save();\n-\t\treturn 0;\n+\t\tgoto done;\n \t} else if (!remoteheads->next && common->next)\n \t\t;\n \t\t/*\n@@ -1339,8 +1341,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t\tgit_committer_info(IDENT_ERROR_ON_NO_NAME);\n \t\t\tprintf(_(\"Trying really trivial in-index merge...\\n\"));\n \t\t\tif (!read_tree_trivial(common->item->object.sha1,\n-\t\t\t\t\thead_commit->object.sha1, remoteheads->item->object.sha1))\n-\t\t\t\treturn merge_trivial(head_commit);\n+\t\t\t\t\t head_commit->object.sha1,\n+\t\t\t\t\t remoteheads->item->object.sha1)) {\n+\t\t\t\tret = merge_trivial(head_commit);\n+\t\t\t\tgoto done;\n+\t\t\t}\n \t\t\tprintf(_(\"Nope.\\n\"));\n \t\t}\n \t} else {\n@@ -1368,7 +1373,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\t}\n \t\tif (up_to_date) {\n \t\t\tfinish_up_to_date(\"Already up-to-date. Yeeah!\");\n-\t\t\treturn 0;\n+\t\t\tgoto done;\n \t\t}\n \t}\n \n@@ -1450,9 +1455,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * If we have a resulting tree, that means the strategy module\n \t * auto resolved the merge cleanly.\n \t */\n-\tif (automerge_was_ok)\n-\t\treturn finish_automerge(head_commit, common, result_tree,\n-\t\t\t\t\twt_strategy);\n+\tif (automerge_was_ok) {\n+\t\tret = finish_automerge(head_commit, common, result_tree,\n+\t\t\t\t wt_strategy);\n+\t\tgoto done;\n+\t}\n \n \t/*\n \t * Pick the result from the best strategy and have the user fix\n@@ -1466,7 +1473,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\telse\n \t\t\tfprintf(stderr, _(\"Merge with strategy %s failed.\\n\"),\n \t\t\t\tuse_strategies[0]->name);\n-\t\treturn 2;\n+\t\tret = 2;\n+\t\tgoto done;\n \t} else if (best_strategy == wt_strategy)\n \t\t; /* We already have its result in the working tree. */\n \telse {\n@@ -1485,7 +1493,9 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tif (merge_was_ok) {\n \t\tfprintf(stderr, _(\"Automatic merge went well; \"\n \t\t\t\"stopped before committing as requested\\n\"));\n-\t\treturn 0;\n \t} else\n-\t\treturn suggest_conflicts(option_renormalize);\n+\t\tret = suggest_conflicts(option_renormalize);\n+\n+done:\n+\treturn ret;\n }\n-- \n1.7.4.74.g639db\n"},{"id":"179625","messageId":"1321522335-24193-6-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 5/8] Use resolve_ref() instead of resolve_ref_unsafe()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:12Z","receivedAt":"2011-11-17T09:32:12Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"resolve_ref_unsaf() may return a pointer to a static buffer. Callers\nthat use this value longer than a couple of statements should copy the\nvalue to avoid some hidden resolve_ref() call that may change the\nstatic buffer's value.\n\nThe bug found by Tony Wang in builtin/merge.c\ndemonstrates this. The first call is in cmd_merge()\n\nbranch = resolve_ref_unsafe(\"HEAD\", head_sha1, 0, &flag);\n\nThen deep in lookup_commit_or_die() a few lines after, resolve_ref()\nmay be called again and destroy \"branch\".\n\nlookup_commit_or_die\n lookup_commit_reference\n lookup_commit_reference_gently\n parse_object\n lookup_replace_object\n do_lookup_replace_object\n prepare_replace_object\n for_each_replace_ref\n do_for_each_ref\n get_loose_refs\n get_ref_dir\n read_ref_full\n\t resolve_ref_unsafe\n\nConvert all resolve_ref_unsafe() call sites, where the return value may\nbe held across many function calls, to resolve_ref() and free buffer\nafter use.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/branch.c | 5 +++--\n builtin/commit.c | 3 ++-\n builtin/fmt-merge-msg.c | 8 ++++++--\n builtin/merge.c | 4 +++-\n builtin/notes.c | 8 ++++++--\n builtin/receive-pack.c | 5 +++--\n reflog-walk.c | 6 ++++--\n 7 files changed, 27 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 6903b0d..633b56e 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -103,7 +103,7 @@ static int branch_merged(int kind, const char *name,\n \t * safely to HEAD (or the other branch).\n \t */\n \tstruct commit *reference_rev = NULL;\n-\tconst char *reference_name = NULL;\n+\tchar *reference_name = NULL;\n \tint merged;\n \n \tif (kind == REF_LOCAL_BRANCH) {\n@@ -115,7 +115,7 @@ static int branch_merged(int kind, const char *name,\n \t\t branch->merge[0] &&\n \t\t branch->merge[0]->dst &&\n \t\t (reference_name =\n-\t\t resolve_ref_unsafe(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n+\t\t resolve_ref(branch->merge[0]->dst, sha1, 1, NULL)) != NULL)\n \t\t\treference_rev = lookup_commit_reference(sha1);\n \t}\n \tif (!reference_rev)\n@@ -141,6 +141,7 @@ static int branch_merged(int kind, const char *name,\n \t\t\t\t\" '%s', even though it is merged to HEAD.\"),\n \t\t\t\tname, reference_name);\n \t}\n+\tfree(reference_name);\n \treturn merged;\n }\n \ndiff --git a/builtin/commit.c b/builtin/commit.c\nindex 0be3b45..f3a6ed2 100644\n--- a/builtin/commit.c\n+++ b/builtin/commit.c\n@@ -1259,7 +1259,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \tstruct commit *commit;\n \tstruct strbuf format = STRBUF_INIT;\n \tunsigned char junk_sha1[20];\n-\tconst char *head = resolve_ref_unsafe(\"HEAD\", junk_sha1, 0, NULL);\n+\tconst char *head;\n \tstruct pretty_print_context pctx = {0};\n \tstruct strbuf author_ident = STRBUF_INIT;\n \tstruct strbuf committer_ident = STRBUF_INIT;\n@@ -1304,6 +1304,7 @@ static void print_summary(const char *prefix, const unsigned char *sha1,\n \trev.diffopt.break_opt = 0;\n \tdiff_setup_done(&rev.diffopt);\n \n+\thead = resolve_ref(\"HEAD\", junk_sha1, 0, NULL);\n \tprintf(\"[%s%s \",\n \t\t!prefixcmp(head, \"refs/heads/\") ?\n \t\t\thead + 11 :\ndiff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c\nindex 5c9b40e..dd94c3d 100644\n--- a/builtin/fmt-merge-msg.c\n+++ b/builtin/fmt-merge-msg.c\n@@ -261,9 +261,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,\n \tint i = 0, pos = 0;\n \tunsigned char head_sha1[20];\n \tconst char *current_branch;\n+\tchar *ref;\n \n \t/* get current branch */\n-\tcurrent_branch = resolve_ref_unsafe(\"HEAD\", head_sha1, 1, NULL);\n+\tcurrent_branch = ref = resolve_ref(\"HEAD\", head_sha1, 1, NULL);\n \tif (!current_branch)\n \t\tdie(\"No current branch\");\n \tif (!prefixcmp(current_branch, \"refs/heads/\"))\n@@ -283,8 +284,10 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,\n \t\t\tdie (\"Error in line %d: %.*s\", i, len, p);\n \t}\n \n-\tif (!srcs.nr)\n+\tif (!srcs.nr) {\n+\t\tfree(ref);\n \t\treturn 0;\n+\t}\n \n \tif (merge_title)\n \t\tdo_fmt_merge_msg_title(out, current_branch);\n@@ -306,6 +309,7 @@ static int do_fmt_merge_msg(int merge_title, struct strbuf *in,\n \t\t\tshortlog(origins.items[i].string, origins.items[i].util,\n \t\t\t\t\thead, &rev, shortlog_len, out);\n \t}\n+\tfree(ref);\n \treturn 0;\n }\n \ndiff --git a/builtin/merge.c b/builtin/merge.c\nindex 0d597b3..8be0594 100644\n--- a/builtin/merge.c\n+++ b/builtin/merge.c\n@@ -1087,6 +1087,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \tstruct commit_list *common = NULL;\n \tconst char *best_strategy = NULL, *wt_strategy = NULL;\n \tstruct commit_list **remotes = &remoteheads;\n+\tchar *branch_ref;\n \n \tif (argc == 2 && !strcmp(argv[1], \"-h\"))\n \t\tusage_with_options(builtin_merge_usage, builtin_merge_options);\n@@ -1095,7 +1096,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t * Check if we are _not_ on a detached HEAD, i.e. if there is a\n \t * current branch.\n \t */\n-\tbranch = resolve_ref_unsafe(\"HEAD\", head_sha1, 0, &flag);\n+\tbranch = branch_ref = resolve_ref(\"HEAD\", head_sha1, 0, &flag);\n \tif (branch && !prefixcmp(branch, \"refs/heads/\"))\n \t\tbranch += 11;\n \tif (!branch || is_null_sha1(head_sha1))\n@@ -1497,5 +1498,6 @@ int cmd_merge(int argc, const char **argv, const char *prefix)\n \t\tret = suggest_conflicts(option_renormalize);\n \n done:\n+\tfree(branch_ref);\n \treturn ret;\n }\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex e191ce6..725a701 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -804,6 +804,8 @@ static int merge_commit(struct notes_merge_options *o)\n \tstruct notes_tree *t;\n \tstruct commit *partial;\n \tstruct pretty_print_context pretty_ctx;\n+\tchar *ref;\n+\tint ret;\n \n \t/*\n \t * Read partial merge result from .git/NOTES_MERGE_PARTIAL,\n@@ -825,7 +827,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tt = xcalloc(1, sizeof(struct notes_tree));\n \tinit_notes(t, \"NOTES_MERGE_PARTIAL\", combine_notes_overwrite, 0);\n \n-\to->local_ref = resolve_ref_unsafe(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n+\to->local_ref = ref = resolve_ref(\"NOTES_MERGE_REF\", sha1, 0, NULL);\n \tif (!o->local_ref)\n \t\tdie(\"Failed to resolve NOTES_MERGE_REF\");\n \n@@ -843,7 +845,9 @@ static int merge_commit(struct notes_merge_options *o)\n \n \tfree_notes(t);\n \tstrbuf_release(&msg);\n-\treturn merge_abort(o);\n+\tret = merge_abort(o);\n+\tfree(ref);\n+\treturn ret;\n }\n \n static int merge(int argc, const char **argv, const char *prefix)\ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 333f2b0..d41884a 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -36,7 +36,7 @@ static int use_sideband;\n static int prefer_ofs_delta = 1;\n static int auto_update_server_info;\n static int auto_gc = 1;\n-static const char *head_name;\n+static char *head_name;\n static int sent_capabilities;\n \n static enum deny_action parse_deny_action(const char *var, const char *value)\n@@ -695,7 +695,8 @@ static void execute_commands(struct command *commands, const char *unpacker_erro\n \n \tcheck_aliased_updates(commands);\n \n-\thead_name = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n+\tfree(head_name);\n+\thead_name = resolve_ref(\"HEAD\", sha1, 0, NULL);\n \n \tfor (cmd = commands; cmd; cmd = cmd->next)\n \t\tif (!cmd->skip_update)\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex b9b2453..2d5aee0 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -50,9 +50,11 @@ static struct complete_reflogs *read_complete_reflog(const char *ref)\n \tfor_each_reflog_ent(ref, read_one_reflog, reflogs);\n \tif (reflogs->nr == 0) {\n \t\tunsigned char sha1[20];\n-\t\tconst char *name = resolve_ref_unsafe(ref, sha1, 1, NULL);\n-\t\tif (name)\n+\t\tchar *name = resolve_ref(ref, sha1, 1, NULL);\n+\t\tif (name) {\n \t\t\tfor_each_reflog_ent(name, read_one_reflog, reflogs);\n+\t\t\tfree(name);\n+\t\t}\n \t}\n \tif (reflogs->nr == 0) {\n \t\tint len = strlen(ref);\n-- \n1.7.4.74.g639db\n"},{"id":"179626","messageId":"1321522335-24193-7-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 6/8] Convert resolve_ref_unsafe+xstrdup to resolve_ref","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:13Z","receivedAt":"2011-11-17T09:32:13Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n builtin/branch.c | 3 +--\n builtin/checkout.c | 13 +++++++------\n builtin/for-each-ref.c | 7 ++-----\n builtin/show-branch.c | 4 +---\n reflog-walk.c | 7 +++----\n wt-status.c | 4 +---\n 6 files changed, 15 insertions(+), 23 deletions(-)\n\ndiff --git a/builtin/branch.c b/builtin/branch.c\nindex 633b56e..a254898 100644\n--- a/builtin/branch.c\n+++ b/builtin/branch.c\n@@ -689,10 +689,9 @@ int cmd_branch(int argc, const char **argv, const char *prefix)\n \n \ttrack = git_branch_track;\n \n-\thead = resolve_ref_unsafe(\"HEAD\", head_sha1, 0, NULL);\n+\thead = resolve_ref(\"HEAD\", head_sha1, 0, NULL);\n \tif (!head)\n \t\tdie(_(\"Failed to resolve HEAD as a valid ref.\"));\n-\thead = xstrdup(head);\n \tif (!strcmp(head, \"HEAD\")) {\n \t\tdetached = 1;\n \t} else {\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 2b8e73b..6efb1cf 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -696,15 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n {\n \tint ret = 0;\n \tstruct branch_info old;\n+\tchar *path;\n \tunsigned char rev[20];\n \tint flag;\n \tmemset(&old, 0, sizeof(old));\n-\told.path = xstrdup(resolve_ref_unsafe(\"HEAD\", rev, 0, &flag));\n+\told.path = path = resolve_ref(\"HEAD\", rev, 0, &flag);\n \told.commit = lookup_commit_reference_gently(rev, 1);\n-\tif (!(flag & REF_ISSYMREF)) {\n-\t\tfree((char *)old.path);\n+\tif (!(flag & REF_ISSYMREF))\n \t\told.path = NULL;\n-\t}\n \n \tif (old.path && !prefixcmp(old.path, \"refs/heads/\"))\n \t\told.name = old.path + strlen(\"refs/heads/\");\n@@ -718,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t}\n \n \tret = merge_working_tree(opts, &old, new);\n-\tif (ret)\n+\tif (ret) {\n+\t\tfree(path);\n \t\treturn ret;\n+\t}\n \n \tif (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n \t\torphaned_commit_warning(old.commit);\n@@ -727,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \tupdate_refs_for_switch(opts, &old, new);\n \n \tret = post_checkout_hook(old.commit, new->commit, 1);\n-\tfree((char *)old.path);\n+\tfree(path);\n \treturn ret || opts->writeout_error;\n }\n \ndiff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\nindex b954ca8..dc19f3c 100644\n--- a/builtin/for-each-ref.c\n+++ b/builtin/for-each-ref.c\n@@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)\n \n \tif (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {\n \t\tunsigned char unused1[20];\n-\t\tconst char *symref;\n-\t\tsymref = resolve_ref_unsafe(ref->refname, unused1, 1, NULL);\n-\t\tif (symref)\n-\t\t\tref->symref = xstrdup(symref);\n-\t\telse\n+\t\tref->symref = resolve_ref(ref->refname, unused1, 1, NULL);\n+\t\tif (!ref->symref)\n \t\t\tref->symref = \"\";\n \t}\n \ndiff --git a/builtin/show-branch.c b/builtin/show-branch.c\nindex 1e7bd31..9e849c7 100644\n--- a/builtin/show-branch.c\n+++ b/builtin/show-branch.c\n@@ -726,10 +726,8 @@ int cmd_show_branch(int ac, const char **av, const char *prefix)\n \n \t\tif (ac == 0) {\n \t\t\tstatic const char *fake_av[2];\n-\t\t\tconst char *refname;\n \n-\t\t\trefname = resolve_ref_unsafe(\"HEAD\", sha1, 1, NULL);\n-\t\t\tfake_av[0] = xstrdup(refname);\n+\t\t\tfake_av[0] = resolve_ref(\"HEAD\", sha1, 1, NULL);\n \t\t\tfake_av[1] = NULL;\n \t\t\tav = fake_av;\n \t\t\tac = 1;\ndiff --git a/reflog-walk.c b/reflog-walk.c\nindex 2d5aee0..fd17e71 100644\n--- a/reflog-walk.c\n+++ b/reflog-walk.c\n@@ -170,11 +170,10 @@ int add_reflog_for_walk(struct reflog_walk_info *info,\n \telse {\n \t\tif (*branch == '\\0') {\n \t\t\tunsigned char sha1[20];\n-\t\t\tconst char *head = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n-\t\t\tif (!head)\n-\t\t\t\tdie (\"No current branch\");\n \t\t\tfree(branch);\n-\t\t\tbranch = xstrdup(head);\n+\t\t\tbranch = resolve_ref(\"HEAD\", sha1, 0, NULL);\n+\t\t\tif (!branch)\n+\t\t\t\tdie (\"No current branch\");\n \t\t}\n \t\treflogs = read_complete_reflog(branch);\n \t\tif (!reflogs || reflogs->nr == 0) {\ndiff --git a/wt-status.c b/wt-status.c\nindex cc6dad5..77adf64 100644\n--- a/wt-status.c\n+++ b/wt-status.c\n@@ -111,7 +111,6 @@ void status_printf_more(struct wt_status *s, const char *color,\n void wt_status_prepare(struct wt_status *s)\n {\n \tunsigned char sha1[20];\n-\tconst char *head;\n \n \tmemset(s, 0, sizeof(*s));\n \tmemcpy(s->color_palette, default_wt_status_colors,\n@@ -119,8 +118,7 @@ void wt_status_prepare(struct wt_status *s)\n \ts->show_untracked_files = SHOW_NORMAL_UNTRACKED_FILES;\n \ts->use_color = -1;\n \ts->relative_paths = 1;\n-\thead = resolve_ref_unsafe(\"HEAD\", sha1, 0, NULL);\n-\ts->branch = head ? xstrdup(head) : NULL;\n+\ts->branch = resolve_ref(\"HEAD\", sha1, 0, NULL);\n \ts->reference = \"HEAD\";\n \ts->fp = stdout;\n \ts->index_file = get_index_file();\n-- \n1.7.4.74.g639db\n"},{"id":"179627","messageId":"1321522335-24193-8-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 7/8] Guard memory overwriting in resolve_ref_unsafe's static buffer","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:14Z","receivedAt":"2011-11-17T09:32:14Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"There is a potential problem with resolve_ref_unsafe() and some other\nfunctions in git. The return value returned by resolve_ref_unsafe() may\nbe changed when the function is called again. Callers must make sure the\nnext call won't happen as long as the value is still being used.\n\nIt's usually hard to track down this kind of problem. Michael Haggerty\nhas an idea [1] that, instead of passing the same static buffer to\ncaller every time the function is called, we free the old buffer and\nallocate the new one. This way access to the old (now invalid) buffer\nmay be caught.\n\nThis patch applies the same principle for resolve_ref_unsafe() with a\nfew modifications:\n\n - This behavior is enabled when GIT_DEBUG_MEMCHECK is set. The ability\n is always available. We may be able to ask users to rerun with this\n flag on in suspicious cases.\n\n - Rely on mmap/mprotect to catch illegal access. We need valgrind or\n some other memory tracking tool to reliably catch this in Michael's\n approach.\n\n - Because mprotect is used instead of munmap, we definitely leak\n memory. Hopefully callers will not put resolve_ref_unsafe() in a\n loop that runs 1 million times.\n\n - Save caller location in the allocated buffer so we know who made this\n call in the core dump.\n\nAlso introduce a new target, \"make memcheck\", that runs tests with this\nflag on.\n\n[1] http://comments.gmane.org/gmane.comp.version-control.git/182209\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n GIT_DEBUG_MEMCHECK should probably be ignored in Windows.\n\n Makefile | 3 +++\n cache.h | 3 ++-\n git-compat-util.h | 9 +++++++++\n refs.c | 14 ++++++++++++--\n wrapper.c | 21 +++++++++++++++++++++\n 5 files changed, 47 insertions(+), 3 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex ee34eab..d671c64 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -2220,6 +2220,9 @@ export NO_SVN_TESTS\n test: all\n \t$(MAKE) -C t/ all\n \n+memcheck: all\n+\tGIT_DEBUG_MEMCHECK=1 $(MAKE) -C t/ all\n+\n test-ctype$X: ctype.o\n \n test-date$X: date.o ctype.o\ndiff --git a/cache.h b/cache.h\nindex 6b8ac8b..feb44a5 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -866,7 +866,8 @@ extern int read_ref(const char *filename, unsigned char *sha1);\n *\n * errno is sometimes set on errors, but not always.\n */\n-extern const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag);\n+#define resolve_ref_unsafe(ref, sha1, reading, flag) resolve_ref_unsafe_real(ref, sha1, reading, flag, __FUNCTION__, __LINE__)\n+extern const char *resolve_ref_unsafe_real(const char *ref, unsigned char *sha1, int reading, int *flag, const char *file, int line);\n extern char *resolve_ref(const char *ref, unsigned char *sha1, int reading, int *flag);\n \n extern int dwim_ref(const char *str, int len, unsigned char *sha1, char **ref);\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex 5ef8ff7..d00c9c6 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -432,6 +432,15 @@ extern char *xstrndup(const char *str, size_t len);\n extern void *xrealloc(void *ptr, size_t size);\n extern void *xcalloc(size_t nmemb, size_t size);\n extern void *xmmap(void *start, size_t length, int prot, int flags, int fd, off_t offset);\n+\n+/*\n+ * These functions are used to allocate new memory. When the memory\n+ * area is no longer used, ban all access to it so any illegal access\n+ * can be caught. xfree_mmap() does not really free memory.\n+ */\n+extern void *xmalloc_mmap(size_t, const char *file, int line);\n+extern void xfree_mmap(void *);\n+\n extern ssize_t xread(int fd, void *buf, size_t len);\n extern ssize_t xwrite(int fd, const void *buf, size_t len);\n extern int xdup(int fd);\ndiff --git a/refs.c b/refs.c\nindex 28496ed..62d8a37 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -497,12 +497,22 @@ static int get_packed_ref(const char *ref, unsigned char *sha1)\n \treturn -1;\n }\n \n-const char *resolve_ref_unsafe(const char *ref, unsigned char *sha1, int reading, int *flag)\n+const char *resolve_ref_unsafe_real(const char *ref, unsigned char *sha1,\n+\t\t\t\t int reading, int *flag,\n+\t\t\t\t const char *file, int line)\n {\n \tint depth = MAXDEPTH;\n \tssize_t len;\n \tchar buffer[256];\n-\tstatic char ref_buffer[256];\n+\tstatic char real_ref_buffer[256];\n+\tstatic char *ref_buffer;\n+\n+\tif (!ref_buffer && !getenv(\"GIT_DEBUG_MEMCHECK\"))\n+\t\tref_buffer = real_ref_buffer;\n+\tif (ref_buffer != real_ref_buffer) {\n+\t\txfree_mmap(ref_buffer);\n+\t\tref_buffer = xmalloc_mmap(256, file, line);\n+\t}\n \n \tif (flag)\n \t\t*flag = 0;\ndiff --git a/wrapper.c b/wrapper.c\nindex 85f09df..3120d97 100644\n--- a/wrapper.c\n+++ b/wrapper.c\n@@ -60,6 +60,27 @@ void *xmallocz(size_t size)\n \treturn ret;\n }\n \n+void *xmalloc_mmap(size_t size, const char *file, int line)\n+{\n+\tint *ret = mmap(NULL, size + sizeof(int*) * 3,\n+\t\t\tPROT_READ | PROT_WRITE, MAP_ANONYMOUS | MAP_PRIVATE,\n+\t\t\t-1, 0);\n+\tif (ret == (int*)-1)\n+\t\tdie_errno(\"unable to mmap %lu bytes anonymously\",\n+\t\t\t (unsigned long)size);\n+\n+\tret[0] = (int)file;\n+\tret[1] = line;\n+\tret[2] = size;\n+\treturn ret + 3;\n+}\n+\n+void xfree_mmap(void *p)\n+{\n+\tif (p && mprotect(((int*)p) - 3, ((int*)p)[-1], PROT_NONE) == -1)\n+\t\tdie_errno(\"unable to remove memory access\");\n+}\n+\n /*\n * xmemdupz() allocates (len + 1) bytes of memory, duplicates \"len\" bytes of\n * \"data\" to the allocated memory, zero terminates the allocated memory,\n-- \n1.7.4.74.g639db\n"},{"id":"179628","messageId":"1321522335-24193-9-git-send-email-pclouds@gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"[PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Nguyễn Thái Ngọc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-17T09:32:15Z","receivedAt":"2011-11-17T09:32:15Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"Make git_pathname() use xmalloc_mmap/xfree_mmap to catch invalid access\nto old buffer when it's already overwritten.\n\nSigned-off-by: Nguyễn Thái Ngọc Duy \n---\n cache.h | 11 +++++++----\n path.c | 28 +++++++++++++++++++---------\n 2 files changed, 26 insertions(+), 13 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex feb44a5..66365fb 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -661,10 +661,13 @@ extern char *git_pathdup(const char *fmt, ...)\n \t__attribute__((format (printf, 1, 2)));\n \n /* Return a statically allocated filename matching the sha1 signature */\n-extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n-extern char *git_path_submodule(const char *path, const char *fmt, ...)\n-\t__attribute__((format (printf, 2, 3)));\n+#define mkpath(...) mkpath_real(__FUNCTION__, __LINE__, __VA_ARGS__)\n+extern char *mkpath_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));\n+#define git_path( ...) git_path_real(__FUNCTION__, __LINE__, __VA_ARGS__)\n+extern char *git_path_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));\n+#define git_path_submodule(path, ...) git_path_submodule_real(__FUNCTION__, __LINE__, path, __VA_ARGS__)\n+extern char *git_path_submodule_real(const char *file, int line, const char *path, const char *fmt, ...)\n+\t__attribute__((format (printf, 4, 5)));\n \n extern char *sha1_file_name(const unsigned char *sha1);\n extern char *sha1_pack_name(const unsigned char *sha1);\ndiff --git a/path.c b/path.c\nindex b6f71d1..d2aa941 100644\n--- a/path.c\n+++ b/path.c\n@@ -15,11 +15,21 @@\n \n static char bad_path[] = \"/bad-path/\";\n \n-static char *get_pathname(void)\n+static char *get_pathname(const char *file, int line)\n {\n-\tstatic char pathname_array[4][PATH_MAX];\n+\tstatic char real_pathname_array[4][PATH_MAX];\n+\tstatic char *pathname_array[4];\n \tstatic int index;\n-\treturn pathname_array[3 & ++index];\n+\tint idx = 3 & ++index;\n+\n+\tif (!pathname_array[idx] && !getenv(\"GIT_DEBUG_MEMCHECK\"))\n+\t\tpathname_array[idx] = real_pathname_array[idx];\n+\n+\tif (pathname_array[idx] != real_pathname_array[idx]) {\n+\t\txfree_mmap(pathname_array[idx]);\n+\t\tpathname_array[idx] = xmalloc_mmap(PATH_MAX, file, line);\n+\t}\n+\treturn pathname_array[idx];\n }\n \n static char *cleanup_path(char *path)\n@@ -87,11 +97,11 @@ char *git_pathdup(const char *fmt, ...)\n \treturn xstrdup(path);\n }\n \n-char *mkpath(const char *fmt, ...)\n+char *mkpath_real(const char *file, int line, const char *fmt, ...)\n {\n \tva_list args;\n \tunsigned len;\n-\tchar *pathname = get_pathname();\n+\tchar *pathname = get_pathname(file, line);\n \n \tva_start(args, fmt);\n \tlen = vsnprintf(pathname, PATH_MAX, fmt, args);\n@@ -101,10 +111,10 @@ char *mkpath(const char *fmt, ...)\n \treturn cleanup_path(pathname);\n }\n \n-char *git_path(const char *fmt, ...)\n+char *git_path_real(const char *file, int line, const char *fmt, ...)\n {\n \tconst char *git_dir = get_git_dir();\n-\tchar *pathname = get_pathname();\n+\tchar *pathname = get_pathname(file, line);\n \tva_list args;\n \tunsigned len;\n \n@@ -122,9 +132,9 @@ char *git_path(const char *fmt, ...)\n \treturn cleanup_path(pathname);\n }\n \n-char *git_path_submodule(const char *path, const char *fmt, ...)\n+char *git_path_submodule_real(const char *file, int line, const char *path, const char *fmt, ...)\n {\n-\tchar *pathname = get_pathname();\n+\tchar *pathname = get_pathname(file, line);\n \tstruct strbuf buf = STRBUF_INIT;\n \tconst char *git_dir;\n \tva_list args;\n-- \n1.7.4.74.g639db\n"},{"id":"179630","messageId":"CALkWK0=h2Q1VTt5AwbBMaZgCYNEkZG5vQGoG=SDBMqYexhJjGA@mail.gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-7-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 6/8] Convert resolve_ref_unsafe+xstrdup to resolve_ref","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-17T10:22:53Z","receivedAt":"2011-11-17T10:22:53Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Nguyễn,\n\nNguyễn Thái Ngọc Duy wrote:\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 2b8e73b..6efb1cf 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -696,15 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>  {\n>        int ret = 0;\n>        struct branch_info old;\n> +       char *path;\n>        unsigned char rev[20];\n>        int flag;\n>        memset(&old, 0, sizeof(old));\n> -       old.path = xstrdup(resolve_ref_unsafe(\"HEAD\", rev, 0, &flag));\n> +       old.path = path = resolve_ref(\"HEAD\", rev, 0, &flag);\n>        old.commit = lookup_commit_reference_gently(rev, 1);\n> -       if (!(flag & REF_ISSYMREF)) {\n> -               free((char *)old.path);\n> +       if (!(flag & REF_ISSYMREF))\n>                old.path = NULL;\n> -       }\n>\n>        if (old.path && !prefixcmp(old.path, \"refs/heads/\"))\n>                old.name = old.path + strlen(\"refs/heads/\");\n\nThis caught my eye immediately: you introduced a new \"path\" variable.\nLet's scroll ahead and see why.\n\n> @@ -718,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>        }\n>\n>        ret = merge_working_tree(opts, &old, new);\n> -       if (ret)\n> +       if (ret) {\n> +               free(path);\n>                return ret;\n> +       }\n>\n>        if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n>                orphaned_commit_warning(old.commit);\n> @@ -727,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>        update_refs_for_switch(opts, &old, new);\n>\n>        ret = post_checkout_hook(old.commit, new->commit, 1);\n> -       free((char *)old.path);\n> +       free(path);\n>        return ret || opts->writeout_error;\n>  }\n\nBefore application of your patch, if !(flag & REF_ISSYMREF) then\nold.path is set to NULL and this free() would've read free(NULL). Was\nthis codepath ever reached, and did you fix a bug by introducing the\nnew \"path\" variable, or was it never reached but you introduced the\nnew variable for clarity anyway? Either case is worth mentioning in\nthe commit message, I think.\n\n> diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c\n> index b954ca8..dc19f3c 100644\n> --- a/builtin/for-each-ref.c\n> +++ b/builtin/for-each-ref.c\n> @@ -628,11 +628,8 @@ static void populate_value(struct refinfo *ref)\n>\n>        if (need_symref && (ref->flag & REF_ISSYMREF) && !ref->symref) {\n>                unsigned char unused1[20];\n> -               const char *symref;\n> -               symref = resolve_ref_unsafe(ref->refname, unused1, 1, NULL);\n> -               if (symref)\n> -                       ref->symref = xstrdup(symref);\n> -               else\n> +               ref->symref = resolve_ref(ref->refname, unused1, 1, NULL);\n> +               if (!ref->symref)\n>                        ref->symref = \"\";\n>        }\n> [...]\n\nThis bit along with the others follow a common pattern: one temporary\nvariable was used to capture the value of resolve_ref_unsafe(), before\nusing xstrdup() on it to perform the actual assignment. You changed\nthe resolve_ref_unsafe() to resolve_ref() and got rid of that extra\nvariable.\n\nThanks.\n\n-- Ram\n"},{"id":"179631","messageId":"CALkWK0ndE1Q_jNSV7CBB5W2NyVhcy7kgNO5woWWOw6CXx3cxcA@mail.gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-9-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-17T10:35:52Z","receivedAt":"2011-11-17T10:35:52Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Nguyễn,\n\nNguyễn Thái Ngọc Duy writes:\n> diff --git a/cache.h b/cache.h\n> index feb44a5..66365fb 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -661,10 +661,13 @@ extern char *git_pathdup(const char *fmt, ...)\n>        __attribute__((format (printf, 1, 2)));\n>\n>  /* Return a statically allocated filename matching the sha1 signature */\n> -extern char *mkpath(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n> -extern char *git_path(const char *fmt, ...) __attribute__((format (printf, 1, 2)));\n> -extern char *git_path_submodule(const char *path, const char *fmt, ...)\n> -       __attribute__((format (printf, 2, 3)));\n> +#define mkpath(...) mkpath_real(__FUNCTION__, __LINE__, __VA_ARGS__)\n> +extern char *mkpath_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));\n> +#define git_path( ...) git_path_real(__FUNCTION__, __LINE__, __VA_ARGS__)\n> +extern char *git_path_real(const char *file, int line, const char *fmt, ...) __attribute__((format (printf, 3, 4)));\n> +#define git_path_submodule(path, ...) git_path_submodule_real(__FUNCTION__, __LINE__, path, __VA_ARGS__)\n> +extern char *git_path_submodule_real(const char *file, int line, const char *path, const char *fmt, ...)\n> +       __attribute__((format (printf, 4, 5)));\n\nThe macros __FILE__, __LINE__ and __VA_ARGS__ are gcc-specific\nextensions, no? I was curious to see if some other parts of Git are\nusing this: a quick grep returns mailmap.c and notes-merge.c. They\nboth use __VA_ARGS__ it for debugging purposes. So, nothing new.\n\nWhat happens if GIT_DEBUG_MEMCHECK is set, but I'm not using gcc?\nAlso, it's probably worth mentioning in the commit message that this\ndebugging trick is gcc-specific.\n\nThanks for working on this.\n\n-- Ram\n"},{"id":"179632","messageId":"CALkWK0kME4fgLK0S+sFRXmDX1uj_N5+PZnvLFJp33qNssPptWQ@mail.gmail.com","threadId":"28960","inReplyTo":"1321522335-24193-5-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 4/8] cmd_merge: convert to single exit point","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2011-11-17T10:39:05Z","receivedAt":"2011-11-17T10:39:05Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi again,\n\nNguyễn Thái Ngọc Duy wrote:\n> [Subject: [PATCH 4/8] cmd_merge: convert to single exit point]\n>\n> This makes post-processing easier.\n>\n> Signed-off-by: Nguyễn Thái Ngọc Duy \n> ---\n>  builtin/merge.c |   48 +++++++++++++++++++++++++++++-------------------\n>  1 files changed, 29 insertions(+), 19 deletions(-)\n\nUm, (how) does this seemingly unrelated patch belong to the series?\n\nThanks.\n\n-- Ram\n"},{"id":"179633","messageId":"20111117103924.GA5277@elie.hsd1.il.comcast.net","threadId":"28960","inReplyTo":"1321522335-24193-1-git-send-email-pclouds@gmail.com","subject":"Re: [PATCH 0/8] nd/resolve-ref v2","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-17T10:39:24Z","receivedAt":"2011-11-17T10:39:24Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nNguyễn Thái Ngọc Duy wrote:\n\n> Nguyễn Thái Ngọc Duy (8):\n\nHere are some comments from a quick glance over the series, to avoid\ntoo much noise that would distract from reports about the upcoming\nrelease.\n\n> Convert many resolve_ref() calls to read_ref*() and ref_exists()\n> Rename resolve_ref() to resolve_ref_unsafe()\n\nI like the general direction of the series (and especially patch 1/8).\nAs Junio nicely explains at [1], it is too tempting to keep and pass\naround the canonical representation of a refname returned by\nresolve_ref() without remembering to copy it.\n\n> Re-add resolve_ref() that always returns an allocated buffer\n\nMakes me nervous, since it would introduce memory leaks if some other\npatch in flight calls resolve_ref(). Why not call it ref_resolve() or\nsomething?\n\n> cmd_merge: convert to single exit point\n> Use resolve_ref() instead of resolve_ref_unsafe()\n\nThe print_summary() change introduces a leak.\n\n[...]\n> Guard memory overwriting in resolve_ref_unsafe's static buffer\n> Enable GIT_DEBUG_MEMCHECK on git_pathname()\n\n__VA_ARGS__ was introduced in C99. I suspect some compilers that\ncurrently can compile git don't support it. But if you need to use\nit, that wouldn't rule out doing so in some corner guarded with an\n#ifdef.\n\nLooks pleasant overall. I look forward to looking more closely at\nthis later.\n\nCiao,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/185446/focus=185519\n"},{"id":"179634","messageId":"20111117104406.GB5277@elie.hsd1.il.comcast.net","threadId":"28960","inReplyTo":"CALkWK0kME4fgLK0S+sFRXmDX1uj_N5+PZnvLFJp33qNssPptWQ@mail.gmail.com","subject":"Re: [PATCH 4/8] cmd_merge: convert to single exit point","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-17T10:44:06Z","receivedAt":"2011-11-17T10:44:06Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n> Nguyễn Thái Ngọc Duy wrote:\n\n>> [Subject: [PATCH 4/8] cmd_merge: convert to single exit point]\n>>\n>> This makes post-processing easier.\n>>\n>> Signed-off-by: Nguyễn Thái Ngọc Duy \n[...]\n> Um, (how) does this seemingly unrelated patch belong to the series?\n\nIt's as Duy says --- it makes post-processing, for example to free()\na variable before returning, easier. Which simplifies the next patch.\n"},{"id":"179637","messageId":"20111117134201.GA30718@sigill.intra.peff.net","threadId":"28960","inReplyTo":"CALkWK0ndE1Q_jNSV7CBB5W2NyVhcy7kgNO5woWWOw6CXx3cxcA@mail.gmail.com","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-17T13:42:01Z","receivedAt":"2011-11-17T13:42:01Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 17, 2011 at 04:05:52PM +0530, Ramkumar Ramachandra wrote:\n\n> The macros __FILE__, __LINE__ and __VA_ARGS__ are gcc-specific\n> extensions, no? I was curious to see if some other parts of Git are\n> using this: a quick grep returns mailmap.c and notes-merge.c. They\n> both use __VA_ARGS__ it for debugging purposes. So, nothing new.\n\nAll three are in C99. I'm pretty sure __FILE__ and __LINE__ were\navailable in C89, but I only have a copy of C99 handy these days.\nVariable-argument macros were definitely introduced in C99 (and were a\ngcc extension for a while before then).\n\n> What happens if GIT_DEBUG_MEMCHECK is set, but I'm not using gcc?\n> Also, it's probably worth mentioning in the commit message that this\n> debugging trick is gcc-specific.\n\nOlder compilers will probably barf on the variable-argument macros.\n\n-Peff\n"},{"id":"179655","messageId":"CACsJy8ATKa+=8HT8qrxgvs1gwFvT=HNxbxwSWf26N3oss6Bhfw@mail.gmail.com","threadId":"28960","inReplyTo":"CALkWK0=h2Q1VTt5AwbBMaZgCYNEkZG5vQGoG=SDBMqYexhJjGA@mail.gmail.com","subject":"Re: [PATCH 6/8] Convert resolve_ref_unsafe+xstrdup to resolve_ref","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-18T00:57:29Z","receivedAt":"2011-11-18T00:57:29Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/11/17 Ramkumar Ramachandra :\n> Hi Nguyễn,\n>\n> Nguyễn Thái Ngọc Duy wrote:\n>> diff --git a/builtin/checkout.c b/builtin/checkout.c\n>> index 2b8e73b..6efb1cf 100644\n>> --- a/builtin/checkout.c\n>> +++ b/builtin/checkout.c\n>> @@ -696,15 +696,14 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>>  {\n>>        int ret = 0;\n>>        struct branch_info old;\n>> +       char *path;\n>>        unsigned char rev[20];\n>>        int flag;\n>>        memset(&old, 0, sizeof(old));\n>> -       old.path = xstrdup(resolve_ref_unsafe(\"HEAD\", rev, 0, &flag));\n>> +       old.path = path = resolve_ref(\"HEAD\", rev, 0, &flag);\n>>        old.commit = lookup_commit_reference_gently(rev, 1);\n>> -       if (!(flag & REF_ISSYMREF)) {\n>> -               free((char *)old.path);\n>> +       if (!(flag & REF_ISSYMREF))\n>>                old.path = NULL;\n>> -       }\n>>\n>>        if (old.path && !prefixcmp(old.path, \"refs/heads/\"))\n>>                old.name = old.path + strlen(\"refs/heads/\");\n>\n> This caught my eye immediately: you introduced a new \"path\" variable.\n> Let's scroll ahead and see why.\n>\n>> @@ -718,8 +717,10 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>>        }\n>>\n>>        ret = merge_working_tree(opts, &old, new);\n>> -       if (ret)\n>> +       if (ret) {\n>> +               free(path);\n>>                return ret;\n>> +       }\n>>\n>>        if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n>>                orphaned_commit_warning(old.commit);\n>> @@ -727,7 +728,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n>>        update_refs_for_switch(opts, &old, new);\n>>\n>>        ret = post_checkout_hook(old.commit, new->commit, 1);\n>> -       free((char *)old.path);\n>> +       free(path);\n>>        return ret || opts->writeout_error;\n>>  }\n>\n> Before application of your patch, if !(flag & REF_ISSYMREF) then\n> old.path is set to NULL and this free() would've read free(NULL).  Was\n> this codepath ever reached, and did you fix a bug by introducing the\n> new \"path\" variable, or was it never reached but you introduced the\n> new variable for clarity anyway?  Either case is worth mentioning in\n> the commit message, I think.\n\nfree(NULL) is OK if I remember correctly, so it's not really a bug.\nAlthough I do plug a memory leak when merge_working_tree() returns\nnon-zero.\n-- \nDuy\n"},{"id":"179656","messageId":"CACsJy8A25SyLVKv8GwkYaHBJwU5tHqgdJK6L-upF9HWseFzCtQ@mail.gmail.com","threadId":"28960","inReplyTo":"20111117134201.GA30718@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-18T01:12:27Z","receivedAt":"2011-11-18T01:12:27Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Nov 17, 2011 at 8:42 PM, Jeff King wrote:\n> On Thu, Nov 17, 2011 at 04:05:52PM +0530, Ramkumar Ramachandra wrote:\n>\n>> The macros __FILE__, __LINE__ and __VA_ARGS__ are gcc-specific\n>> extensions, no?  I was curious to see if some other parts of Git are\n>> using this: a quick grep returns mailmap.c and notes-merge.c.  They\n>> both use __VA_ARGS__ it for debugging purposes.  So, nothing new.\n>\n> All three are in C99. I'm pretty sure __FILE__ and __LINE__ were\n> available in C89, but I only have a copy of C99 handy these days.\n> Variable-argument macros were definitely introduced in C99 (and were a\n> gcc extension for a while before then).\n>\n>> What happens if GIT_DEBUG_MEMCHECK is set, but I'm not using gcc?\n>> Also, it's probably worth mentioning in the commit message that this\n>> debugging trick is gcc-specific.\n>\n> Older compilers will probably barf on the variable-argument macros.\n\nAnyway to detect if __VA_ARGS__ is supported at compile time? I guess\n#ifdef __GNUC__ is the last resort.\n\nnotes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do\nsomething there too.\n-- \nDuy\n"},{"id":"179657","messageId":"20111118012715.GA7826@sigill.intra.peff.net","threadId":"28960","inReplyTo":"CACsJy8A25SyLVKv8GwkYaHBJwU5tHqgdJK6L-upF9HWseFzCtQ@mail.gmail.com","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-18T01:27:15Z","receivedAt":"2011-11-18T01:27:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 18, 2011 at 08:12:27AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> > Older compilers will probably barf on the variable-argument macros.\n> \n> Anyway to detect if __VA_ARGS__ is supported at compile time? I guess\n> #ifdef __GNUC__ is the last resort.\n\nYou can check \"#if __STDC_VERSION__ >= 19901L\", but that will of course\nonly tell you whether you have C99; older gcc (and possibly other\ncompilers) supported __VA_ARGS__ even before it was standardized.\n\nBut more annoying is that there isn't a great fallback to __VA_ARGS__.\nIf you can't use it, then every callsite has to have the same number of\narguments. So it's not like you can localize the fallback code to just\nthe definition.\n\nUnless you really need macro-like behavior, you're probably better off\nusing a variadic function and making it a static inline on platforms\nwhich can do so.\n\n> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do\n> something there too.\n\nI hadn't noticed. That definitely violates our usual rules about\nportability. That usage can easily be turned into an inline function.\nHowever, since nobody has complained in the past year, it makes me\nwonder if we are overly conservative (my guess is that people on crazy\nold compilers just don't keep up with git. Which maybe means they aren't\nworth worrying about. But who knows).\n\n-Peff\n"},{"id":"179658","messageId":"20111118012746.GA22485@elie.hsd1.il.comcast.net","threadId":"28960","inReplyTo":"CACsJy8A25SyLVKv8GwkYaHBJwU5tHqgdJK6L-upF9HWseFzCtQ@mail.gmail.com","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-18T01:27:46Z","receivedAt":"2011-11-18T01:27:46Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Nguyen Thai Ngoc Duy wrote:\n\n> Anyway to detect if __VA_ARGS__ is supported at compile time? I guess\n> #ifdef __GNUC__ is the last resort.\n\nWhy would one want to do that? Either your feature requires C99 (so the\nbuild can error out if __VA_ARGS__ support is missing) or it doesn't.\n\n> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do\n> something there too.\n\nGood catch. How about this patch? It seems I forgot to send it out\nlast May and it has been rotting in my local tree ever since.\n\n-- >8 --\nSubject: notes merge: eliminate OUTPUT macro\n\nThe macro is variadic, which breaks support for pre-C99 compilers,\nand it hides an \"if\", which can make code hard to understand on\nfirst reading if some arguments have side-effects.\n\nThe OUTPUT macro seems to have been inspired by the \"output\" function\nfrom merge-recursive. But that function in merge-recursive exists to\nindent output based on the level of recursion and there is no similar\njustification for such a function in \"notes merge\".\n\nNoticed with 'make CC=\"gcc -std=c89 -pedantic\"':\n\n notes-merge.c:24:22: warning: anonymous variadic macros were introduced in C99 [-Wvariadic-macros]\n\nEncouraged-by: Nguyễn Thái Ngọc Duy \nSigned-off-by: Jonathan Nieder \n---\n notes-merge.c | 104 +++++++++++++++++++++++++++++++++-----------------------\n 1 files changed, 61 insertions(+), 43 deletions(-)\n\ndiff --git a/notes-merge.c b/notes-merge.c\nindex baaf31f4..d0e5034d 100644\n--- a/notes-merge.c\n+++ b/notes-merge.c\n@@ -21,14 +21,6 @@ void init_notes_merge_options(struct notes_merge_options *o)\n \to->verbosity = NOTES_MERGE_VERBOSITY_DEFAULT;\n }\n \n-#define OUTPUT(o, v, ...) \\\n-\tdo { \\\n-\t\tif ((o)->verbosity >= (v)) { \\\n-\t\t\tprintf(__VA_ARGS__); \\\n-\t\t\tputs(\"\"); \\\n-\t\t} \\\n-\t} while (0)\n-\n static int path_to_sha1(const char *path, unsigned char *sha1)\n {\n \tchar hex_sha1[40];\n@@ -392,21 +384,26 @@ static int merge_one_change_manual(struct notes_merge_options *o,\n \n \tstrbuf_addf(&(o->commit_msg), \"\\t%s\\n\", sha1_to_hex(p->obj));\n \n-\tOUTPUT(o, 2, \"Auto-merging notes for %s\", sha1_to_hex(p->obj));\n+\tif (o->verbosity >= 2)\n+\t\tprintf(\"Auto-merging notes for %s\\n\", sha1_to_hex(p->obj));\n \tcheck_notes_merge_worktree(o);\n \tif (is_null_sha1(p->local)) {\n \t\t/* D/F conflict, checkout p->remote */\n \t\tassert(!is_null_sha1(p->remote));\n-\t\tOUTPUT(o, 1, \"CONFLICT (delete/modify): Notes for object %s \"\n-\t\t \"deleted in %s and modified in %s. Version from %s \"\n-\t\t \"left in tree.\", sha1_to_hex(p->obj), lref, rref, rref);\n+\t\tif (o->verbosity >= 1)\n+\t\t\tprintf(\"CONFLICT (delete/modify): Notes for object %s \"\n+\t\t\t\t\"deleted in %s and modified in %s. Version from %s \"\n+\t\t\t\t\"left in tree.\\n\",\n+\t\t\t\tsha1_to_hex(p->obj), lref, rref, rref);\n \t\twrite_note_to_worktree(p->obj, p->remote);\n \t} else if (is_null_sha1(p->remote)) {\n \t\t/* D/F conflict, checkout p->local */\n \t\tassert(!is_null_sha1(p->local));\n-\t\tOUTPUT(o, 1, \"CONFLICT (delete/modify): Notes for object %s \"\n-\t\t \"deleted in %s and modified in %s. Version from %s \"\n-\t\t \"left in tree.\", sha1_to_hex(p->obj), rref, lref, lref);\n+\t\tif (o->verbosity >= 1)\n+\t\t\tprintf(\"CONFLICT (delete/modify): Notes for object %s \"\n+\t\t\t\t\"deleted in %s and modified in %s. Version from %s \"\n+\t\t\t\t\"left in tree.\\n\",\n+\t\t\t\tsha1_to_hex(p->obj), rref, lref, lref);\n \t\twrite_note_to_worktree(p->obj, p->local);\n \t} else {\n \t\t/* \"regular\" conflict, checkout result of ll_merge() */\n@@ -415,8 +412,9 @@ static int merge_one_change_manual(struct notes_merge_options *o,\n \t\t\treason = \"add/add\";\n \t\tassert(!is_null_sha1(p->local));\n \t\tassert(!is_null_sha1(p->remote));\n-\t\tOUTPUT(o, 1, \"CONFLICT (%s): Merge conflict in notes for \"\n-\t\t \"object %s\", reason, sha1_to_hex(p->obj));\n+\t\tif (o->verbosity >= 1)\n+\t\t\tprintf(\"CONFLICT (%s): Merge conflict in notes for \"\n+\t\t\t\t\"object %s\\n\", reason, sha1_to_hex(p->obj));\n \t\tll_merge_in_worktree(o, p);\n \t}\n \n@@ -438,24 +436,30 @@ static int merge_one_change(struct notes_merge_options *o,\n \tcase NOTES_MERGE_RESOLVE_MANUAL:\n \t\treturn merge_one_change_manual(o, p, t);\n \tcase NOTES_MERGE_RESOLVE_OURS:\n-\t\tOUTPUT(o, 2, \"Using local notes for %s\", sha1_to_hex(p->obj));\n+\t\tif (o->verbosity >= 2)\n+\t\t\tprintf(\"Using local notes for %s\\n\",\n+\t\t\t\t\t\tsha1_to_hex(p->obj));\n \t\t/* nothing to do */\n \t\treturn 0;\n \tcase NOTES_MERGE_RESOLVE_THEIRS:\n-\t\tOUTPUT(o, 2, \"Using remote notes for %s\", sha1_to_hex(p->obj));\n+\t\tif (o->verbosity >= 2)\n+\t\t\tprintf(\"Using remote notes for %s\\n\",\n+\t\t\t\t\t\tsha1_to_hex(p->obj));\n \t\tif (add_note(t, p->obj, p->remote, combine_notes_overwrite))\n \t\t\tdie(\"BUG: combine_notes_overwrite failed\");\n \t\treturn 0;\n \tcase NOTES_MERGE_RESOLVE_UNION:\n-\t\tOUTPUT(o, 2, \"Concatenating local and remote notes for %s\",\n-\t\t sha1_to_hex(p->obj));\n+\t\tif (o->verbosity >= 2)\n+\t\t\tprintf(\"Concatenating local and remote notes for %s\\n\",\n+\t\t\t\t\t\t\tsha1_to_hex(p->obj));\n \t\tif (add_note(t, p->obj, p->remote, combine_notes_concatenate))\n \t\t\tdie(\"failed to concatenate notes \"\n \t\t\t \"(combine_notes_concatenate)\");\n \t\treturn 0;\n \tcase NOTES_MERGE_RESOLVE_CAT_SORT_UNIQ:\n-\t\tOUTPUT(o, 2, \"Concatenating unique lines in local and remote \"\n-\t\t \"notes for %s\", sha1_to_hex(p->obj));\n+\t\tif (o->verbosity >= 2)\n+\t\t\tprintf(\"Concatenating unique lines in local and remote \"\n+\t\t\t\t\"notes for %s\\n\", sha1_to_hex(p->obj));\n \t\tif (add_note(t, p->obj, p->remote, combine_notes_cat_sort_uniq))\n \t\t\tdie(\"failed to concatenate notes \"\n \t\t\t \"(combine_notes_cat_sort_uniq)\");\n@@ -518,8 +522,9 @@ static int merge_from_diffs(struct notes_merge_options *o,\n \tconflicts = merge_changes(o, changes, &num_changes, t);\n \tfree(changes);\n \n-\tOUTPUT(o, 4, \"Merge result: %i unmerged notes and a %s notes tree\",\n-\t conflicts, t->dirty ? \"dirty\" : \"clean\");\n+\tif (o->verbosity >= 4)\n+\t\tprintf(\"Merge result: %i unmerged notes and a %s notes tree\\n\",\n+\t\t\tconflicts, t->dirty ? \"dirty\" : \"clean\");\n \n \treturn conflicts ? -1 : 1;\n }\n@@ -616,33 +621,40 @@ int notes_merge(struct notes_merge_options *o,\n \tif (!bases) {\n \t\tbase_sha1 = null_sha1;\n \t\tbase_tree_sha1 = EMPTY_TREE_SHA1_BIN;\n-\t\tOUTPUT(o, 4, \"No merge base found; doing history-less merge\");\n+\t\tif (o->verbosity >= 4)\n+\t\t\tprintf(\"No merge base found; doing history-less merge\\n\");\n \t} else if (!bases->next) {\n \t\tbase_sha1 = bases->item->object.sha1;\n \t\tbase_tree_sha1 = bases->item->tree->object.sha1;\n-\t\tOUTPUT(o, 4, \"One merge base found (%.7s)\",\n-\t\t sha1_to_hex(base_sha1));\n+\t\tif (o->verbosity >= 4)\n+\t\t\tprintf(\"One merge base found (%.7s)\\n\",\n+\t\t\t\tsha1_to_hex(base_sha1));\n \t} else {\n \t\t/* TODO: How to handle multiple merge-bases? */\n \t\tbase_sha1 = bases->item->object.sha1;\n \t\tbase_tree_sha1 = bases->item->tree->object.sha1;\n-\t\tOUTPUT(o, 3, \"Multiple merge bases found. Using the first \"\n-\t\t \"(%.7s)\", sha1_to_hex(base_sha1));\n+\t\tif (o->verbosity >= 3)\n+\t\t\tprintf(\"Multiple merge bases found. Using the first \"\n+\t\t\t\t\"(%.7s)\\n\", sha1_to_hex(base_sha1));\n \t}\n \n-\tOUTPUT(o, 4, \"Merging remote commit %.7s into local commit %.7s with \"\n-\t \"merge-base %.7s\", sha1_to_hex(remote->object.sha1),\n-\t sha1_to_hex(local->object.sha1), sha1_to_hex(base_sha1));\n+\tif (o->verbosity >= 4)\n+\t\tprintf(\"Merging remote commit %.7s into local commit %.7s with \"\n+\t\t\t\"merge-base %.7s\\n\", sha1_to_hex(remote->object.sha1),\n+\t\t\tsha1_to_hex(local->object.sha1),\n+\t\t\tsha1_to_hex(base_sha1));\n \n \tif (!hashcmp(remote->object.sha1, base_sha1)) {\n \t\t/* Already merged; result == local commit */\n-\t\tOUTPUT(o, 2, \"Already up-to-date!\");\n+\t\tif (o->verbosity >= 2)\n+\t\t\tprintf(\"Already up-to-date!\\n\");\n \t\thashcpy(result_sha1, local->object.sha1);\n \t\tgoto found_result;\n \t}\n \tif (!hashcmp(local->object.sha1, base_sha1)) {\n \t\t/* Fast-forward; result == remote commit */\n-\t\tOUTPUT(o, 2, \"Fast-forward\");\n+\t\tif (o->verbosity >= 2)\n+\t\t\tprintf(\"Fast-forward\\n\");\n \t\thashcpy(result_sha1, remote->object.sha1);\n \t\tgoto found_result;\n \t}\n@@ -684,8 +696,9 @@ int notes_merge_commit(struct notes_merge_options *o,\n \tint path_len = strlen(path), i;\n \tconst char *msg = strstr(partial_commit->buffer, \"\\n\\n\");\n \n-\tOUTPUT(o, 3, \"Committing notes in notes merge worktree at %.*s\",\n-\t path_len - 1, path);\n+\tif (o->verbosity >= 3)\n+\t\tprintf(\"Committing notes in notes merge worktree at %.*s\\n\",\n+\t\t\tpath_len - 1, path);\n \n \tif (!msg || msg[2] == '\\0')\n \t\tdie(\"partial notes commit has empty message\");\n@@ -700,7 +713,9 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t\tunsigned char obj_sha1[20], blob_sha1[20];\n \n \t\tif (ent->len - path_len != 40 || get_sha1_hex(relpath, obj_sha1)) {\n-\t\t\tOUTPUT(o, 3, \"Skipping non-SHA1 entry '%s'\", ent->name);\n+\t\t\tif (o->verbosity >= 3)\n+\t\t\t\tprintf(\"Skipping non-SHA1 entry '%s'\\n\",\n+\t\t\t\t\t\t\t\tent->name);\n \t\t\tcontinue;\n \t\t}\n \n@@ -712,14 +727,16 @@ int notes_merge_commit(struct notes_merge_options *o,\n \t\tif (add_note(partial_tree, obj_sha1, blob_sha1, NULL))\n \t\t\tdie(\"Failed to add resolved note '%s' to notes tree\",\n \t\t\t ent->name);\n-\t\tOUTPUT(o, 4, \"Added resolved note for object %s: %s\",\n-\t\t sha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));\n+\t\tif (o->verbosity >= 4)\n+\t\t\tprintf(\"Added resolved note for object %s: %s\\n\",\n+\t\t\t\tsha1_to_hex(obj_sha1), sha1_to_hex(blob_sha1));\n \t}\n \n \tcreate_notes_commit(partial_tree, partial_commit->parents, msg,\n \t\t\t result_sha1);\n-\tOUTPUT(o, 4, \"Finalized notes merge commit: %s\",\n-\t sha1_to_hex(result_sha1));\n+\tif (o->verbosity >= 4)\n+\t\tprintf(\"Finalized notes merge commit: %s\\n\",\n+\t\t\tsha1_to_hex(result_sha1));\n \tfree(path);\n \treturn 0;\n }\n@@ -731,7 +748,8 @@ int notes_merge_abort(struct notes_merge_options *o)\n \tint ret;\n \n \tstrbuf_addstr(&buf, git_path(NOTES_MERGE_WORKTREE));\n-\tOUTPUT(o, 3, \"Removing notes merge worktree at %s\", buf.buf);\n+\tif (o->verbosity >= 3)\n+\t\tprintf(\"Removing notes merge worktree at %s\\n\", buf.buf);\n \tret = remove_dir_recursively(&buf, 0);\n \tstrbuf_release(&buf);\n \treturn ret;\n-- \n1.7.8.rc2\n"},{"id":"179660","messageId":"20111118013613.GB22485@elie.hsd1.il.comcast.net","threadId":"28960","inReplyTo":"20111118012715.GA7826@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-18T01:36:13Z","receivedAt":"2011-11-18T01:36:13Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Fri, Nov 18, 2011 at 08:12:27AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n>> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do\n>> something there too.\n>\n> I hadn't noticed. That definitely violates our usual rules about\n> portability. That usage can easily be turned into an inline function.\n> However, since nobody has complained in the past year, it makes me\n> wonder if we are overly conservative (my guess is that people on crazy\n> old compilers just don't keep up with git. Which maybe means they aren't\n> worth worrying about. But who knows).\n\nI suspect we didn't notice because MSVC 2005 and later have some\n(limited, as far as I can tell from web searches) support for\n__VA_ARGS__.\n"},{"id":"179663","messageId":"CACsJy8CB6VXjyC-M4C9qGm-n73Kuf1Q0SbH4Ync5Osts-uufQQ@mail.gmail.com","threadId":"28960","inReplyTo":"20111118012715.GA7826@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-11-18T01:50:20Z","receivedAt":"2011-11-18T01:50:20Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Nov 18, 2011 at 8:27 AM, Jeff King wrote:\n> On Fri, Nov 18, 2011 at 08:12:27AM +0700, Nguyen Thai Ngoc Duy wrote:\n>\n>> > Older compilers will probably barf on the variable-argument macros.\n>>\n>> Anyway to detect if __VA_ARGS__ is supported at compile time? I guess\n>> #ifdef __GNUC__ is the last resort.\n>\n> You can check \"#if __STDC_VERSION__ >= 19901L\", but that will of course\n> only tell you whether you have C99; older gcc (and possibly other\n> compilers) supported __VA_ARGS__ even before it was standardized.\n>\n> But more annoying is that there isn't a great fallback to __VA_ARGS__.\n> If you can't use it, then every callsite has to have the same number of\n> arguments. So it's not like you can localize the fallback code to just\n> the definition.\n>\n> Unless you really need macro-like behavior, you're probably better off\n> using a variadic function and making it a static inline on platforms\n> which can do so.\n\nI need to save __FILE__ and __LINE__ of call site, inline functions\nprobably don't help.\n\n>> notes-merge.c introduces __VA_ARGS__ since v1.7.4 so we may want to do\n>> something there too.\n>\n> I hadn't noticed. That definitely violates our usual rules about\n> portability.  That usage can easily be turned into an inline function.\n> However, since nobody has complained in the past year, it makes me\n> wonder if we are overly conservative (my guess is that people on crazy\n> old compilers just don't keep up with git. Which maybe means they aren't\n> worth worrying about. But who knows).\n\nFor the record, Sun C compiler 5.9 seems to support it.\n-- \nDuy\n"},{"id":"179664","messageId":"20111118020633.GA9635@sigill.intra.peff.net","threadId":"28960","inReplyTo":"CACsJy8CB6VXjyC-M4C9qGm-n73Kuf1Q0SbH4Ync5Osts-uufQQ@mail.gmail.com","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-11-18T02:06:33Z","receivedAt":"2011-11-18T02:06:33Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 18, 2011 at 08:50:20AM +0700, Nguyen Thai Ngoc Duy wrote:\n\n> > Unless you really need macro-like behavior, you're probably better off\n> > using a variadic function and making it a static inline on platforms\n> > which can do so.\n> \n> I need to save __FILE__ and __LINE__ of call site, inline functions\n> probably don't help.\n\nYeah, you'd have to pass them in to the function. Which of course you\ncan't wrap with a macro, because the whole thing is variadic.\n\n-Peff\n"},{"id":"179676","messageId":"CALKQrgfTKmSd8se3n3xq89SXRmNPm3qz3Ckv2mUghot8kStKxA@mail.gmail.com","threadId":"28960","inReplyTo":"20111118012746.GA22485@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Johan Herland","fromEmail":"johan@herland.net","sentAt":"2011-11-18T06:16:13Z","receivedAt":"2011-11-18T06:16:13Z","isPatch":true,"sender":{"key":"johan@herland.net","avatar":"https://avatars.githubusercontent.com/u/547031?v=4"},"body":"On Fri, Nov 18, 2011 at 02:27, Jonathan Nieder wrote:\n> Subject: notes merge: eliminate OUTPUT macro\n>\n> The macro is variadic, which breaks support for pre-C99 compilers,\n> and it hides an \"if\", which can make code hard to understand on\n> first reading if some arguments have side-effects.\n>\n> The OUTPUT macro seems to have been inspired by the \"output\" function\n> from merge-recursive.  But that function in merge-recursive exists to\n> indent output based on the level of recursion and there is no similar\n> justification for such a function in \"notes merge\".\n>\n> Noticed with 'make CC=\"gcc -std=c89 -pedantic\"':\n>\n>  notes-merge.c:24:22: warning: anonymous variadic macros were introduced in C99 [-Wvariadic-macros]\n>\n> Encouraged-by: Nguyễn Thái Ngọc Duy \n> Signed-off-by: Jonathan Nieder \n\nAcked-by: Johan Herland \n\n-- \nJohan Herland, \nwww.herland.net\n"},{"id":"179678","messageId":"20111118065204.GD25145@elie.hsd1.il.comcast.net","threadId":"28960","inReplyTo":"CALKQrgfTKmSd8se3n3xq89SXRmNPm3qz3Ckv2mUghot8kStKxA@mail.gmail.com","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2011-11-18T06:52:04Z","receivedAt":"2011-11-18T06:52:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Johan Herland wrote:\n\n> Acked-by: Johan Herland \n\nThanks for looking it over.\n"},{"id":"179683","messageId":"7vobw9lw6n.fsf@alter.siamese.dyndns.org","threadId":"28960","inReplyTo":"CALKQrgfTKmSd8se3n3xq89SXRmNPm3qz3Ckv2mUghot8kStKxA@mail.gmail.com","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-18T07:35:44Z","receivedAt":"2011-11-18T07:35:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Thanks, both. Will queue.\n"},{"id":"179697","messageId":"20111118125056.GA32599@server.brlink.eu","threadId":"28960","inReplyTo":"20111117134201.GA30718@sigill.intra.peff.net","subject":"Re: [PATCH 8/8] Enable GIT_DEBUG_MEMCHECK on git_pathname()","fromName":"Bernhard R. Link","fromEmail":"brl+git@mail.brlink.eu","sentAt":"2011-11-18T12:50:56Z","receivedAt":"2011-11-18T12:50:56Z","isPatch":true,"sender":{"key":"brl+git@mail.brlink.eu","avatar":null},"body":"* Jeff King [111117 14:42]:\n> Variable-argument macros were definitely introduced in C99 (and were a\n> gcc extension for a while before then).\n\nThough AFAIK not that long. Before that gcc had it's own variadic\nsyntax a la\n\n#define eprintf(args...) fprintf (stderr, args)\n\n\tBernhard R. Link\n"},{"id":"180374","messageId":"CACsJy8A6KqjOjMQQXEdHWeAUqB5np-_myXdQzQe1+oMfXFzv7Q@mail.gmail.com","threadId":"28960","inReplyTo":"20111117103924.GA5277@elie.hsd1.il.comcast.net","subject":"Re: [PATCH 0/8] nd/resolve-ref v2","fromName":"Nguyen Thai Ngoc Duy","fromEmail":"pclouds@gmail.com","sentAt":"2011-12-06T14:07:09Z","receivedAt":"2011-12-06T14:07:09Z","isPatch":true,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"2011/11/17 Jonathan Nieder :\n> Hi,\n>\n> Nguyễn Thái Ngọc Duy wrote:\n>\n>> Nguyễn Thái Ngọc Duy (8):\n>\n> Here are some comments from a quick glance over the series, to avoid\n> too much noise that would distract from reports about the upcoming\n> release.\n\nSorry for the late reply. Somehow I managed to miss your mail.\n\n>>   Re-add resolve_ref() that always returns an allocated buffer\n>\n> Makes me nervous, since it would introduce memory leaks if some other\n> patch in flight calls resolve_ref().  Why not call it ref_resolve() or\n> something?\n\nYeah, I made the same mistake before and made it again.\n\n>>   Enable GIT_DEBUG_MEMCHECK on git_pathname()\n>\n> __VA_ARGS__ was introduced in C99.  I suspect some compilers that\n> currently can compile git don't support it.  But if you need to use\n> it, that wouldn't rule out doing so in some corner guarded with an\n> #ifdef.\n>\n> Looks pleasant overall.  I look forward to looking more closely at\n> this later.\n\nThis patch is bonus anyway and I think I'll drop it. Keeping a bunch\nof ifdefs looks really ugly. I think having git_pathname()'s static\nbuffer per callsite file would be a good thing to do instead.\n-- \nDuy\n"}]}