Re: [PATCH v2 2/3] git diff: improve A...B merge-base handling

[Junio C Hamano <gitster@pobox.com>]

"Chris Torek via GitGitGadget" <gitgitgadget@gmail.com> writes:

> +static void symdiff_prepare(struct rev_info *rev, struct symdiff *sym)
> +{
> +	int i, lcount = 0, rcount = 0, basecount = 0;
> +	int lpos = -1, rpos = -1, basepos = -1;
> +	struct bitmap *map = NULL;

The logic around rcount and rpos in this function still smells
fishy.  For example, rcount is counted up from 0 but its value is
never consulted, so we should be able to get rid of it.

For that matter, lcount and lpos look somewhat redundant.  lpos
begins with -1 to signal "have not seen any left end of symmetric
range yet", and we won't allow more than two symmetric ranges
anyway, so we should be able to get rid of lcount and base the error
condition purely on lpos by

 * when we see SYMMETRIC_LEFT, check if lpos is already non-negative,
   and die otherwise right there.  We don't need "if lcount != 1, die"
   after the loop.

 * after the loop, if lpos is still -1, we know we didn't see
   symmetric difference.

> +	/*
> +	 * Use the whence fields to find merge bases and left and
> +	 * right parts of symmetric difference, so that we do not
> +	 * depend on the order that revisions are parsed.  If there
> +	 * are any revs that aren't from these sources, we have a
> +	 * "git diff C A...B" or "git diff A...B C" case.  Or we
> +	 * could even get "git diff A...B C...E", for instance.
> +	 *
> +	 * If we don't have just one merge base, we pick one
> +	 * at random.
> +	 *
> +	 * NB: REV_CMD_LEFT, REV_CMD_RIGHT are also used for A..B,
> +	 * so we must check for SYMMETRIC_LEFT too.  The two arrays
> +	 * rev->pending.objects and rev->cmdline.rev are parallel.
> +	 */
> +	for (i = 0; i < rev->cmdline.nr; i++) {
> +		struct object *obj = rev->pending.objects[i].item;
> +		switch (rev->cmdline.rev[i].whence) {
> +		case REV_CMD_MERGE_BASE:
> +			if (basepos < 0)
> +				basepos = i;
> +			basecount++;
> +			break;		/* do mark all bases */
> +		case REV_CMD_LEFT:
> +			if (obj->flags & SYMMETRIC_LEFT) {
> +				lpos = i;
> +				lcount++;
> +				break;	/* do mark A */
> +			}
> +			continue;
> +		case REV_CMD_RIGHT:
> +			rpos = i;
> +			rcount++;
> +			continue;	/* don't mark B */

It is unclear if we want to allow "git diff A..B C..D" (or
alternatively "git diff A...B C..D") and if so why.  

It appears that you are allowing both, but I am not sure if that is
a good idea.  Read a bit further below.

> +		default:
> +			continue;
> +		}
> +		if (map == NULL)
> +			map = bitmap_new();
> +		bitmap_set(map, i);
> +	}
> +
> +	if (lcount == 0) {	/* not a symmetric difference */
> +		bitmap_free(map);
> +		sym->warn = 0;
> +		sym->skip = NULL;
> +		return;
> +	}
> +
> +	if (lcount != 1)
> +		die(_("cannot use more than one symmetric difference"));
> +
> +	if (basecount == 0) {
> +		const char *lname = rev->pending.objects[lpos].name;
> +		const char *rname = rev->pending.objects[rpos].name;
> +		die(_("%s...%s: no merge base"), lname, rname);

When "git diff A...B C..D" is given, what do we want to do?  A is
the only element marked with REV_CMD_LEFT, which is pointed at by
lpos and lcount gets incremented to become one.  When we see B, we
make rpos point at it, but then later, when we see D, wouldn't rpos
get updated to point at it?  What error message would we give when
we see no merge base then?  We would want to say the symdiff between
A and B has no merge bases, but wouldn't we end up mentioning D
instead of B?

> +	}
> +	bitmap_unset(map, basepos);	/* unmark the base we want */
> +	sym->base = basepos;
> +	sym->left = lpos;
> +	sym->right = rpos;
> +	sym->warn = basecount > 1;
> +	sym->skip = map;
> +}

> @@ -394,8 +494,9 @@ int cmd_diff(int argc, const char **argv, const char *prefix)
>  			die(_("invalid object '%s' given."), name);
>  		if (obj->type == OBJ_COMMIT)
>  			obj = &get_commit_tree(((struct commit *)obj))->object;
> -

Do not lose this blank line.  It does not make a difference to the
compiler, but it is semantically significant to human readers.

>  		if (obj->type == OBJ_TREE) {
> +			if (sdiff.skip && bitmap_get(sdiff.skip, i))
> +				continue;
>  			obj->flags |= flags;
>  			add_object_array(obj, name, &ent);
>  		} else if (obj->type == OBJ_BLOB) {

> diff --git a/t/t4068-diff-symmetric.sh b/t/t4068-diff-symmetric.sh
> new file mode 100755
> index 00000000000..7b5988933da
> --- /dev/null
> +++ b/t/t4068-diff-symmetric.sh
> @@ -0,0 +1,81 @@
> +#!/bin/sh
> +...
> +test_expect_success setup '
> +	git commit --allow-empty -m A &&
> +	echo b >b &&
> +	git add b &&
> +	git commit -m B &&
> +	git checkout -b br1 HEAD^ &&
> +	echo c >c &&
> +	git add c &&
> +	git commit -m C &&
> +	git tag commit-C &&
> +	git merge -m D master &&
> +	git tag commit-D &&
> +	git checkout master &&
> +	git merge -m E commit-C &&
> +	git checkout -b br2 commit-C &&
> +	echo f >f &&
> +	git add f &&
> +	git commit -m F &&
> +	git checkout br1 &&
> +	git merge -m G br2 &&
> +	git checkout --orphan br3 &&
> +	git commit -m H
> +'
> +
> +test_expect_success 'diff with one merge base' '
> +	git diff commit-D...br1 >tmp &&
> +	tail -1 tmp >actual &&

Let's make sure we spell "tail -n 1" (same for "head -1" -> "head -n 1").
I know there are a handful of existing offenders, but that does not mean
it is OK to make things worse.

> +	echo +f >expect &&
> +	test_cmp expect actual