Re: [PATCH 14/14] Build in merge

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 1, 2008, 06:23 UTC
Message-ID: <7vtzfaqg16.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <5e65b37998d1fdd9d314e48cea2cf67fd73ba8cd.1214879690.git.vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 10 quoted lines

> +static int read_tree_trivial(unsigned char *common, unsigned char *head,
> +	unsigned char *one)
> +{
> +	int i, nr_trees = 0;
> +	struct tree *trees[MAX_UNPACK_TREES];
> +	struct tree_desc t[MAX_UNPACK_TREES];
> +	struct unpack_trees_options opts;
> +
> +	memset(&opts, 0, sizeof(opts));
> +	opts.head_idx = 2;

Do you still need this assignment here?

Show 26 quoted lines

> +int cmd_merge(int argc, const char **argv, const char *prefix)
> +{
> +	unsigned char result_tree[20];
> +	struct strbuf buf;
> +	const char *head_arg;
> +	int flag, head_invalid = 0, i;
> +	int best_cnt = -1, merge_was_ok = 0, automerge_was_ok = 0;
> +	struct commit_list *common = NULL;
> +	struct path_list_item *best_strategy = NULL, *wt_strategy = NULL;
> +	struct commit_list **remotes = &remoteheads;
> +
> +	setup_work_tree();
> +	if (unmerged_cache())
> +		die("You are in the middle of a conflicted merge.");
> +
> +	/*
> +	 * Check if we are _not_ on a detached HEAD, i.e. if there is a
> +	 * current branch.
> +	 */
> +	branch = resolve_ref("HEAD", head, 0, &flag);
> +	if (branch && flag & REF_ISSYMREF) {
> +		const char *ptr = skip_prefix(branch, "refs/heads/");
> +		if (ptr)
> +			branch = ptr;
> +	} else
> +		head_invalid = 1;

Wait a minute... Are you calling a detached HEAD as head_invalid here? I am not too much worried about variable naming, but you later do ...

Show 12 quoted lines

> +	if (!have_message && is_old_style_invocation(argc, argv)) {
> ...
> +	} else if (head_invalid) {
> +		struct object *remote_head;
> +		/*
> +		 * If the merged head is a valid one there is no reason
> +		 * to forbid "git merge" into a branch yet to be born.
> +		 * We do the same for "git pull".
> +		 */
> +		if (argc != 1)
> +			die("Can merge only exactly one commit into "
> +				"empty head");

Which is about HEAD pointing at a branch that isn't born yet. They are two very different concepts.

Either the above "else if (head_invalid)" is wrong, or more likely the setting of head_invalid we saw earlier is wrong.

Probably what you meant was:

	- "char *branch" points at either "HEAD" (when detached) or
          the name of the branch (e.g. "master" when "refs/heads/master");

	- "unsigned char head[]" stores the commit object name of the
          current HEAD (or 0{40} if the current branch is unborn);

        - set head_invalid to true only when the current branch is unborn.

So perhaps...

	branch = resolve_ref("HEAD", head, 0, &flag);
        if (branch && (flag & REF_ISSYMREF) && !prefixcmp(branch, "refs/heads/"))
		branch += 11;
        head_invalid = is_null_sha1(head);

And probably we can even drop (flag & REF_ISSYMREF) from the above safely. Do we even care if the head is detached in this program? I doubt it.

Show 13 quoted lines

> ...
> +		strbuf_init(&msg, 0);
> +		strbuf_addstr(&msg, "Fast forward");
> +		if (have_message)
> +			strbuf_addstr(&msg,
> +				" (no commit created; -m option ignored)");
> +		o = peel_to_type(sha1_to_hex(remoteheads->item->object.sha1),
> +			0, NULL, OBJ_COMMIT);
> +		if (!o)
> +			return 1;
> +
> +		if (checkout_fast_forward(head, remoteheads->item->object.sha1))
> +			return 1;

Not exiting with 0 status from around here upon error is an improvement, but does the user see sensible error messages in addition to the exit status?

Getting warmer ;-)