Nguyễn Thái Ngọc Duy <pclouds@xxxxxxxxx> writes:
> HEAD and MERGE_HEAD (among other branch tips) should never hold a
> tag. That can only be caused by broken tools and is cumbersome to fix
> by an end user with:
>
> $ git update-ref HEAD $(git rev-parse HEAD^{commit})
>
> which may look like a magic to a new person.
>
> Be easy, warn users (so broken tools can be fixed) and move on.
We do not really fix broken tools; we just fix breakages caused by them.
> Be robust, if the given SHA-1 cannot be resolved to a commit object,
> die (therefore return value is always valid).
Makes sense.
> diff --git a/builtin/commit.c b/builtin/commit.c
> index cb73857..f78b449 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -63,6 +63,7 @@ N_("The previous cherry-pick is now empty, possibly due to conflict resolution.\
> "Otherwise, please use 'git reset'\n");
>
> static unsigned char head_sha1[20];
> +static struct commit *head_commit;
I was not happy with the file-scope global head_sha1[] already, and this
makes me even less happy. Was it too much trouble to keep them local to
cmd_commit() and pass them around as arguments where necessary? If you
pass around head_commit, is_null_sha1(head_sha1) can be replaced with a
check !head_commit so we may even be able to lose the head_sha1[] global.
And actually removing head_sha1[] is a necessary step from the correctness
point of view. The repository may have given an object name for a tag in
head_sha1[] and lookup_expect_commit() may have peeled it to a commit.
The code may want to add the "HEAD" as one of the parents of a new commit,
and head_sha1[] is no longer the place to pick that information up after
your fix. The code needs to look at head_commit->object.sha1[] instead.
Losing the use of head_sha1[] variable and forcing the code to go to
head_commit->object.sha1[] reduces the risk of such confusion.
> @@ -1028,6 +1026,9 @@ static int parse_and_validate_options(int argc, const char *argv[],
>
> if (get_sha1("HEAD", head_sha1))
> initial_commit = 1;
> + else
> + head_commit = lookup_expect_commit(head_sha1, "HEAD");
It may be just me, but the name feels a bit funny. The original name was
"look up" (verb) + "commit" (direct object of the verb), and what you are
doing is lookup_and_expect_commit(), but it is too long.
Perhaps lookup_commit_or_die()?
To a naïve reader of the caller, the function looks as if it would do
struct commit *lookup_expect_commit(unsigned char *head_sha1, char *name)
{
struct commit *c;
if (get_sha1("HEAD", head_sha1) ||
!(c = lookup_commit(head_sha1))) die(...);
return c;
}
but in fact it does not use "HEAD" for anything other than error reporting
and it is the caller's responsibility to supply head_sha1[].
It is documented in the function declaration by making the memory
head_sha1 points at as a "const" (i.e. the function can't be calling
get_sha() on the refname), but it may also need a comment or two there.
> diff --git a/builtin/merge.c b/builtin/merge.c
> index 325891e..22e98e8 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -51,6 +51,7 @@ static int allow_trivial = 1, have_message;
> static struct strbuf merge_msg;
> static struct commit_list *remoteheads;
> static unsigned char head[20], stash[20];
> +static struct commit* head_commit;
This is C, not C++; asterisk sticks to the variable, not type.
> @@ -1030,6 +1031,8 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
> branch += 11;
> if (is_null_sha1(head))
> head_invalid = 1;
> + else
> + head_commit = lookup_expect_commit(head, "HEAD");
>
> git_config(git_merge_config, NULL);
>
The same comment as head[] vs head_commit->object.sha1[] redundancy
applies from builtin/commit.c here.
Overall, the changes to buitlin/merge.c look very nice, getting rid of the
repeated lookup_commit(head). In a separate patch after this fix gets
ready, we may want to further clean it up so that head_commit being NULL
means head_invalid, losing a redundant variable.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html