Re: [PATCH 13/13] Build in merge

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 30, 2008, 05:44 UTC
Message-ID: <7vd4lz4gtw.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <e8d1385cc49a06ca3fae28231ebc66a333ce4ffb.1214789764.git.vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

Show 22 quoted lines

> diff --git a/builtin-merge.c b/builtin-merge.c
> new file mode 100644
> index 0000000..187038c
> --- /dev/null
> +++ b/builtin-merge.c
> @@ -0,0 +1,1173 @@
> ...
> +static void save_state(void)
> +{
> +	int fd;
> +	struct child_process stash;
> +	const char *argv[] = {"stash", "create", NULL};
> +
> +	fd = open(git_path("MERGE_STASH"), O_WRONLY | O_CREAT, 0666);
> +	if (fd < 0)
> +		die("Could not write to %s", git_path("MERGE_STASH"));
> +	memset(&stash, 0, sizeof(stash));
> +	stash.argv = argv;
> +	stash.out = fd;
> +	stash.git_cmd = 1;
> +	run_command(&stash);
> +}

I first thought "heh, that's clever" until I noticed that we use "stash create" with "stash apply" these days instead of cpio for this. I suspect that we can do away without leaving the stash in this temporary file, but that comment applies to the scripted version as well.

By the way, it would be consistent to name counterpart to dropsave in the scripted version as "drop_save" if you use "save_state" and "restore_state".

Show 23 quoted lines

> +static void reset_hard(unsigned const char *sha1, int verbose)
> +{
> +	struct tree *tree;
> +	struct unpack_trees_options opts;
> +	struct tree_desc t;
> +
> +	memset(&opts, 0, sizeof(opts));
> +	opts.head_idx = -1;
> +	opts.src_index = &the_index;
> +	opts.dst_index = &the_index;
> +	opts.update = 1;
> +	opts.reset = 1;
> +	if (verbose)
> +		opts.verbose_update = 1;
> +
> +	tree = parse_tree_indirect(sha1);
> +	if (!tree)
> +		die("failed to unpack %s tree object", sha1_to_hex(sha1));
> +	parse_tree(tree);
> +	init_tree_desc(&t, tree->buffer, tree->size);
> +	if (unpack_trees(1, &t, &opts))
> +		exit(128); /* We've already reported the error, finish dying */
> +}

Isn't this trashing all the cached stat info from the index? If this is emulating "reset --hard", it also should set opts.merge and do oneway_merge, after reading the current index in, I think. Resetting the index and the working tree is not particularly performance critical part, but trashing the cached stat info would hurt the performance of everything that reads the index after this function returns quite badly. I suspect that you might be better off forking the real thing (reset --hard) if you cannot get it right here.

Show 12 quoted lines

> +/* Get the name for the merge commit's message. */
> +static void merge_name(const char *remote, struct strbuf *msg)
> ...
> +	strbuf_init(&buf, 0);
> +	strbuf_addstr(&buf, "refs/heads/");
> +	strbuf_addstr(&buf, remote);
> +	dwim_ref(buf.buf, buf.len, branch_head, &ref);
> +	if (!hashcmp(remote_head->sha1, branch_head)) {
> +		strbuf_addf(msg, "%s\t\tbranch '%s' of .\n",
> +			sha1_to_hex(branch_head), remote);
> +		return;
> +	}

Hmm, why not resolve_ref() so that it does not dwim at all? The point of the code is so that you can be confident that 'blah' *is* a local branch when you say "branch 'blah'".

Show 9 quoted lines

> +	/* See if remote matches <name>~<number>, or <name>^ */
> +	ptr = strrchr(remote, '^');
> +	if (ptr && ptr[1] == '\0') {
> +		len = strlen(remote);
> +		while ((ptr = (char *)memrchr(remote, '^', len)))
> +			if (ptr && ptr[1] == '\0')
> +				len = ptr - remote - 1;
> +			else
> +				break;

That's a funny way to say:

	for (len = 0, ptr = remote + strlen(remote);
             remote < ptr && ptr[-1] == '^';
             ptr--)
		len++;

Show 9 quoted lines

> +	if (len) {
> +		struct strbuf truname = STRBUF_INIT;
> +		strbuf_addstr(&truname, remote);
> +		strbuf_setlen(&truname, len);
> +		if (dwim_ref(truname.buf, truname.len, buf_sha, &ref)) {
> +			strbuf_addf(msg,
> +				"%s\t\tbranch '%s' (early part) of .\n",
> +				sha1_to_hex(remote_head->sha1), truname.buf);
> +			return;

Isn't this wrong? Giving "v1.5.6~20" to this code will strip ~20 and make remote = "v1.5.6", to which dwim_ref() will happily say Ok, and you end up saying "branch 'v1.5.6' (early part)", don't you?

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 = -1;

Is this the correct head_idx value for this three-way merge? I think it should be 2 but please double check.

Show 5 quoted lines

> +static int commit_tree_trivial(const char *msg, unsigned const char *tree,
> +		struct commit_list *parents, unsigned char *ret)
> +{
>  ...
> +}

We may want to have another patch before this one to abstract most of cmd_commit_tree() out, perhaps?

Show 19 quoted lines

> +int cmd_merge(int argc, const char **argv, const char *prefix)
> ...
> +	/*
> +	 * This could be traditional "merge <msg> HEAD <commit>..."  and
> +	 * the way we can tell it is to see if the second token is HEAD,
> +	 * but some people might have misused the interface and used a
> +	 * committish that is the same as HEAD there instead.
> +	 * Traditional format never would have "-m" so it is an
> +	 * additional safety measure to check for it.
> +	 */
> +	strbuf_init(&buf, 0);
> +	if (argc > 1) {
> +		unsigned char second_sha1[20];
> +
> +		if (get_sha1(argv[1], second_sha1))
> +			die("Not a valid ref: %s", argv[1]);
> +		second_token = lookup_commit_reference_gently(second_sha1, 0);
> +		if (!second_token)
> +			die("'%s' is not a commit", argv[1]);

Interesting.

This _superficially_ is quite wrong, because the purpose of this part of the code is to tell if we got old-style invocation, and we should not barfing merely because what we got is _not_ old-style. If it is not old-style, then it would be new-style, and the logic to tell if it is old-style should ideally not have much knowledge about the new-style invocation to say "hey, that's an incocrrect new-style invocation". By the way, this part should probably be in a separate function:

	static int is_old_style_invocation(int ac, const char **gv);

Old-style invocation of "git merge" (primarily by "git pull") was to call it as:

	git merge "message here" HEAD $commit1 $commit2...

and it checks the second token ("HEAD" in the above, but people can misuse the interface to name the current branch name). If the second token is not a ref that resolves to a commit, all you know is that this is _not_ an old-style invocation, and calling the program with new-style is not a crime.

The only reason this is wrong only superficially is because new style invocation would always be:

	git merge [options] $commit1 $commit2...

after stripping the options, and these seemingly wrong die() will complain when you try to create an Octopus with the new-style syntax and the parameter given as the second remote parent is not a commit. So the logic is wrong, the fact that the user gets the same error message for incorrect old-style invocation (perhaps "git merge <msg> HAED $commit") and incorrect new-style invocation "git merge $commit1 $nonsense" is just an accident, and the end result does not hurt, but asks for a "Huh? why does it check and complain only the second parent here but not the first one?".

It is interesting, but feels quite dirty.

> +	}
> +
> +	if (!have_message && second_token &&
> +		!hashcmp(second_token->object.sha1, head)) {

You need to know that resolve_ref() cleared head[] when head_invalid is true when reading this code to notice that, unlike the previous round of this patch, it is Ok not to check head_invalid is fine here. I somehow feel it is an unnecessary optimization/obfuscation.

But once you have "is_old_style_invocation" suggested earlier, this part would look much cleaner and the above comment would become unnecessary.

Show 21 quoted lines

> +	for (i = 0; i < argc; i++) {
> +		struct object *o;
> +
> +		o = peel_to_type(argv[i], 0, NULL, OBJ_COMMIT);
> +		if (!o)
> +			die("%s - not something we can merge", argv[i]);
> +		remotes = &commit_list_insert(lookup_commit(o->sha1),
> +			remotes)->next;
> +
> +		strbuf_addf(&buf, "GITHEAD_%s", sha1_to_hex(o->sha1));
> +		setenv(buf.buf, argv[i], 1);
> +		strbuf_reset(&buf);
> +	}
> +
> +		o = peel_to_type(sha1_to_hex(remoteheads->item->object.sha1),
> +			0, NULL, OBJ_COMMIT);
> +		if (!o)
> +			return 0;
> +
> +		if (checkout_fast_forward(head, remoteheads->item->object.sha1))
> +			return 0;

When o does not peel well, or checkout_fast_forward() returns failure, that would be a failure case, wouldn't it? Why return 0?

Maybe you misread "exit" in shell scripts? It does not mean exit(0); it means "exit with the same exit status as the last command". So

	new_head=$(git rev-parse ...) &&
        git read-tree -m -u ... &&
        finish || exit

will exit non-zero if any of the commands chained by && fails.

Show 8 quoted lines

> +		if (allow_trivial) {
> +			/* See if it is really trivial. */
> +			git_committer_info(IDENT_ERROR_ON_NO_NAME);
> +			printf("Trying really trivial in-index merge...\n");
> +			if (!read_tree_trivial(common->item->object.sha1,
> +					head, remoteheads->item->object.sha1))
> +				return merge_trivial();
> +			printf("Nope.\n");

Nicer, much nicer.

Show 18 quoted lines

> +	/*
> +	 * At this point, we need a real merge.  No matter what strategy
> +	 * we use, it would operate on the index, possibly affecting the
> +	 * working tree, and when resolved cleanly, have the desired
> +	 * tree in the index -- this means that the index must be in
> +	 * sync with the head commit.  The strategies are responsible
> +	 * to ensure this.
> +	 */
> +	if (use_strategies.nr != 1) {
> +		/*
> +		 * Stash away the local changes so that we can try more
> +		 * than one.
> +		 */
> +		save_state();
> +		single_strategy = 0;
> +	} else {
> +		unlink(git_path("MERGE_STASH"));
> +		single_strategy = 1;

I think s/single_strategy/(use_strategies.nr == 1)/ in the remainder of the code would be taking advantage of working in C ;-)

Show 7 quoted lines

> +		if (ret) {
> +			/*
> +			 * The backend exits with 1 when conflicts are
> +			 * left to be resolved, with 2 when it does not
> +			 * handle the given merge at all.
> +			 */
> +			if (ret == 1) {

Probably from here til ...

> +				int cnt = 0;
> ...
> +				cnt += count_unmerged_entries();

... here should be a separate "evaluate_result()" function.

Show 17 quoted lines

> +				if (best_cnt <= 0 || cnt <= best_cnt) {
> +					best_strategy =
> +						&use_strategies.items[i];
> +					best_cnt = cnt;
> +				}
> +			}
> +			if (merge_was_ok)
> +				break;
> +			else
> +				continue;
> +		}
> +
> +		/* Automerge succeeded. */
> +		write_tree_trivial(result_tree);
> +		automerge_was_ok = 1;
> +		break;
> +	}

The part of the file I did not comment in this message (except for the "cast" thing) looked straightforward enough. I did not very carefully hunt for bugs, but I did not see anything obviously wrong.

Thanks.