Re: [PATCH v2] commit: accept tag objects in HEAD/MERGE_HEAD

2011/8/18 Junio C Hamano [off-list ref]:

By the way, what happens when you try to merge when HEAD points at a tag
that points at a commit? Would we end up creating a commit that points at
a bogus parent?

I guess I'd segfault the same way commit does. It does
lookup_commit(head) without checking the result.

quoted

diff --git a/builtin/commit.c b/builtin/commit.c
index 2088b6b..f327595 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c

@@ -1387,6 +1387,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

      unsigned char commit_sha1[20];
      struct ref_lock *ref_lock;
      struct commit_list *parents = NULL, **pptr = &parents;
+     struct commit *commit;
      struct stat statbuf;
      int allow_fast_forward = 1;
      struct wt_status s;

Here, you are being inconsistent with your own argument you made in your
previous message "later somebody may forget to update the former while
updating the latter" when I suggested to separate the two logically
separate operations (grab the head_commit object when necessary, and
decide how the commit is made). By hoisting of the scope of "commit", you
made the variable undefined when dealing with the initial_commit, exposing
the code to the same risk that somebody may try to use "commit" variable
after the if/elseif/... cascade, where it may or may not be defined.

Fair enough.

quoted

@@ -1423,12 +1424,11 @@ int cmd_commit(int argc, const char **argv, const char *prefix)

                      reflog_msg = "commit (initial)";
      } else if (amend) {
              struct commit_list *c;
-             struct commit *commit;

              if (!reflog_msg)
                      reflog_msg = "commit (amend)";
-             commit = lookup_commit(head_sha1);
-             if (!commit || parse_commit(commit))
+             commit = lookup_expect_commit(head_sha1, "HEAD");
+             if (parse_commit(commit))
                      die(_("could not parse HEAD commit"));

Is this still necessary? I think your lookup_expect_commit() already
checks this condition and barfs.

It is. lookup_expect_commit() does not parse commit content, only
makes sure it's a commit object.
-- 
Duy