Re: [PATCH v2 2/3] git diff: improve A...B merge-base handling
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jun 9, 2020, 22:52 UTC
- Message-ID
- <xmqqftb3hmx6.fsf@gitster.c.googlers.com>
- In-Reply-To
- <100fa4034771e58b65cdac2f3dfb48531c07b735.1591729224.git.gitgitgadget@gmail.com>
"Chris Torek via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> +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.
Show 34 quoted lines
> + /*
> + * 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.
Show 22 quoted lines
> + 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?
Show 8 quoted lines
> + } > + 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; > +}
Show 5 quoted lines
> @@ -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.
Show 6 quoted lines
> 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) {Show 35 quoted lines
> 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