From: Junio C Hamano <gitster@pobox.com>
Date: Tue, 23 Aug 2011 18:46:59 GMT
Subject: Re: [PATCH v4 3/4] merge: remove global variable head[]
Message-ID: <7vty98q82k.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1313765407-29925-3-git-send-email-pclouds@gmail.com>

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

> @@ -1012,9 +1014,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  {
>  	unsigned char result_tree[20];
>  	unsigned char stash[20];
> +	unsigned char head[20];
> +	struct commit *head_commit = NULL;
>  	struct strbuf buf = STRBUF_INIT;
>  	const char *head_arg;
> -	int flag, head_invalid = 0, i;
> +	int flag, i;
>  	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
>  	struct commit_list *common = NULL;
>  	const char *best_strategy = NULL, *wt_strategy = NULL;
> @@ -1030,8 +1034,11 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  	branch = resolve_ref("HEAD", head, 0, &flag);
>  	if (branch && !prefixcmp(branch, "refs/heads/"))
>  		branch += 11;
> -	if (is_null_sha1(head))
> -		head_invalid = 1;
> +	if (!is_null_sha1(head)) {
> +		head_commit = lookup_commit(head);
> +		if (!head_commit)
> +			die(_("could not parse HEAD"));
> +	}

Is this is_null_sha1() valid without first clearing head[]?

Also, would it be too much trouble and code churn to employ the same
strategy as my rewrite of your [1/4] and pass only head_commit around in
the call chain?

Because this is the way to set up head_commit in the first place, this
particular resolve_ref() -> is_null_sha1() chain is unavoidable and needs
to be written carefully, but after the

    !head_commit === is_null_sha1(head) === is_initial_commit

invariant is established, I suspect that it would reduce the chance of
similar mistakes in later parts of the code if it can check and use only
one argument.