Re: [PATCH] Implement git-branch and git-merge-base as built-ins.

From: Junio C Hamano <junkio@cox.net>
Date: Jun 8, 2006, 18:53 UTC
Message-ID: <7vverbsclf.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <4488633A.5060409@bitplanet.net>

Kristian Høgsberg <krh@bitplanet.net> writes:

Show 5 quoted lines

> This patch is more or less a straight port of git-branch from shell
> script to C.  Branch deletion uses git-merge-base to check if it is safe
> to delete a branch, so I changed merge-base.c to export this functionality
> as a for_each_merge_base() iterator.  As a side effect, git-merge-base is
> now also a built-in command.

First, some lighter-weight comments:

 (0) Somehow your diff have two copies of everything.  How did
     you prepare this message I wonder...?

 (1) Sign your work, please.

 (2) I would have preferred a patch to do merge-base and another
     patch to do branch.  That way, we could do merge-base
     without doing branch if we wanted to.

But the patch is wrong. Your for-each-merge-base cannot be called more than once, but delete-branches does.

The merge-base program as implemented currently is written with the assumption that it is called only once, and leaves its working state in parsed commit objects all over the place. In order to make the second and subsequent call to work correctly, you need to clean the flags up.

As a demonstration, with this function appended to your merge-base.c and making it a built-in "merge-base-bogo":

int cmd_merge_base_bogo(int argc, const char **argv, char **envp)
{
	struct commit *rev1, *rev2;
	unsigned char rev1key[20], rev2key[20];
	int errors = 0;

	setup_git_directory();
	git_config(git_default_config);

	while (1 < argc && argv[1][0] == '-') {
		const char *arg = argv[1];
		if (!strcmp(arg, "-a") || !strcmp(arg, "--all"))
			show_all = 1;
		else
			usage(merge_base_usage);
		argc--; argv++;
	}
	for (; 3 <= argc; argc -= 2, argv += 2) {
		if (get_sha1(argv[1], rev1key) ||
		    !(rev1 = lookup_commit_reference(rev1key))) {
			error("Not a valid object name %s", argv[1]);
			errors++;
			continue;
		}
		if (get_sha1(argv[2], rev2key) ||
		    !(rev2 = lookup_commit_reference(rev2key))) {
			error("Not a valid object name %s", argv[2]);
			errors++;
			continue;
		}
		for_each_merge_base(rev1, rev2, print_merge_base, NULL);
	}
	if (1 < argc) {
		error("Trailing argument %s not used", argv[1]);
		errors++;
	}
	return !!errors;
}

Here is what happens.

: gitster; ./git merge-base-bogo 66ae0c77 ced9456a 262a6ef76a1dde97ab50d79fa5cd6d3f9f125765 : gitster; ./git merge-base-bogo 89719209 262a6ef7 aa6bf0eb6489d652c5877d65160ed33c857afa74 : gitster; ./git merge-base-bogo 66ae0c77 ced9456a 89719209 262a6ef7 262a6ef76a1dde97ab50d79fa5cd6d3f9f125765 262a6ef76a1dde97ab50d79fa5cd6d3f9f125765

This is because the invocation for the second pair does not start with a clean slate, and is affected by the leftover states from the computation for the first pair. If you swap the arguments, you sometimes get correct result by accident, like this:

: gitster; ./git merge-base-bogo 89719209 262a6ef7 66ae0c77 ced9456a aa6bf0eb6489d652c5877d65160ed33c857afa74 262a6ef76a1dde97ab50d79fa5cd6d3f9f125765

Incidentally, this is why I haven't done "A...B" revision syntax extension to mean "^$(git merge-base A B) B".