{"thread":{"id":"42782","subject":"git diff A...B is both overly forgiving and weird","startedAt":"2016-07-05T13:02:07Z","lastAt":"2016-07-05T13:02:07Z","messageCount":1,"participants":["Chris Torek"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"290839","messageId":"201607051248.u65Cmg1x073897@elf.torek.net","threadId":"42782","inReplyTo":null,"subject":"git diff A...B is both overly forgiving and weird","fromName":"Chris Torek","fromEmail":"chris.torek@gmail.com","sentAt":"2016-07-05T12:48:42Z","receivedAt":"2016-07-05T13:02:07Z","isPatch":false,"sender":{"key":"chris.torek@gmail.com","avatar":"https://avatars.githubusercontent.com/u/16826774?v=4"},"body":"(This is not a big bug, but it is clearly not right.)\n\nThe problem is mostly described in the patch below.\n\nThis patch can perhaps be improved a bit by using an abbreviated\nhash for the chosen merge base, in the case of multiple merge\nbases, and/or by removing some of the paranoia-style checking (but\nas I'm not terribly familiar with Git internals, I left these in).\n\nExtra negative references given on the command line, as in \"git\ndiff master~5...master^^2 ^origin/master^\", now pass undetected; I\nam not sure if there is a good way to catch them.  (In the\noriginal code, they apparently substituted in for the merge-base.)\n\nChris\n\n\ncommit 2b9288cc90175557766ef33e350e0514470b6ad4\nAuthor: Chris Torek <chris.torek@gmail.com>\nDate:   Tue Jul 5 05:15:23 2016 -0700\n\n    git diff: improve A...B merge-base handling\n    \n    When git diff is given a symmetric difference A...B, it chooses\n    some merge base between the two specified commits.\n    \n    This fails, however, if there is *no* merge base: instead, you\n    see the difference between A and B, which is certainly not what\n    is expected:\n    \n        git diff origin/master...origin/todo\n        [same output as \"git diff origin/master origin/todo\"]\n    \n    Moreover, if additional revisions are specified on the command\n    line:\n    \n        git diff origin/master...origin/todo origin/master^\n        git diff master~5...master^^2 origin/master^\n        git diff master~5...master^^2 ^origin/master^\n    \n    the result gets a bit weird.  If there is a symmetric difference\n    merge base, that will provide the left side of the diff.  An\n    extra positive ref, if given, becomes the right side.  If there\n    is no merge base, the symmetric difference is completely lost:\n    for the Git repository, \"origin/master...origin/todo origin/master^\"\n    runs a combined diff!  (This follows from treating the lack of\n    a merge base as an ordinary diff of the two specified revisions.)\n    \n    To avoid all this, add a routine to catch the A...B case and\n    verify that there is at least one merge base.  As a side effect,\n    produce a warning showing *which* merge base is being used when\n    there are multiple choices; die if there is no merge base.\n\ndiff --git a/builtin/diff.c b/builtin/diff.c\nindex b7a9405..c040b47 100644\n--- a/builtin/diff.c\n+++ b/builtin/diff.c\n@@ -256,6 +256,97 @@ static int builtin_diff_files(struct rev_info *revs, int argc, const char **argv\n \treturn run_diff_files(revs, options);\n }\n \n+/*\n+ * Check for symmetric-difference arguments, and if present,\n+ * convert A...B to $(merge-base A B) B.\n+ */\n+static int builtin_diff_sdiff(struct rev_info *rev, struct object_array *ent)\n+{\n+\tint i;\n+\tint on_left = 0, uninteresting = 0, unflagged = 0;\n+\tint lpos = -1, rpos = -1, upos = -1;\n+\tchar *lname, *rname, *uname;\n+\tstruct object *robj, *uobj;\n+\n+\t/*\n+\t * If there were any A...B symmetric differences specified,\n+\t * we'll have A marked with SYMMETRIC_LEFT | UNINTERESTING,\n+\t * and each merge base marked with UNINTERESTING.  B will\n+\t * be unmarked.  If we were given additional revisions they\n+\t * may have other markings, so remember the first A and B\n+\t * for error messages as well.\n+\t *\n+\t * The actual order of these are that the merge bases come\n+\t * first, then A, then B, but we don't depend on that in\n+\t * this code.\n+\t *\n+\t * If we don't have just one merge base, we pick one\n+\t * at (essentially) random (by using the first).\n+\t *\n+\t * Note that we don't check obj->parsed, since symmetric\n+\t * difference objects are always pre-parsed: a non-parsed\n+\t * object is by definition not part of a symmetric difference.\n+\t */\n+\tfor (i = 0; i < rev->pending.nr; i++) {\n+\t\tstruct object *obj = rev->pending.objects[i].item;\n+\n+\t\tif (obj->flags & SYMMETRIC_LEFT) {\n+\t\t\ton_left++;\n+\t\t\tif (lpos < 0)\n+\t\t\t\tlpos = i;\n+\t\t} else if (obj->flags & UNINTERESTING) {\n+\t\t\tuninteresting++;\n+\t\t\tif (upos < 0)\n+\t\t\t\tupos = i;\n+\t\t} else {\n+\t\t\tunflagged++;\n+\t\t\tif (lpos >= 0 && rpos < 0)\n+\t\t\t\trpos = i;\n+\t\t}\n+\t}\n+\n+\tif (on_left == 0)\t/* no symmetric differences */\n+\t\treturn (0);\n+\n+\t/*\n+\t * N.B.: lname retains the three dots.  The name of\n+\t * the right hand side object (there should be one) is\n+\t * a subset of this string.\n+\t */\n+\tlname = rev->pending.objects[lpos].name;\n+\t/* demand exactly 1 on left and 1 unflagged */\n+\tif (unflagged > 1)\n+\t\tdie(_(\"%s: must not list additional revs\"), lname);\n+\tif (unflagged < 1)\n+\t\tdie(_(\"%s: internal error: '...' but no unflagged revs\"), lname);\n+\tif (uninteresting == 0)\n+\t\tdie(_(\"%s: no merge base\"), lname);\n+\n+\t/*\n+\t * Pick the tree from an \"uninteresting\" commit for the left\n+\t * side, and the tree from the right hand side commit (rname)\n+\t * for the other.  If there is more than one uninteresting\n+\t * tree, print a warning.\n+\t *\n+\t * Note that the two objects (at upos and rpos) are by\n+\t * definition commit objects.\n+\t */\n+\trname = rev->pending.objects[rpos].name;\n+\tuname = rev->pending.objects[upos].name;\n+\tif (uninteresting > 1)\n+\t\twarning(_(\"%s: multiple merge bases, using %s\"), lname, uname);\n+\trobj = rev->pending.objects[rpos].item;\n+\tuobj = rev->pending.objects[upos].item;\n+\tif (uobj->type != OBJ_COMMIT || robj->type != OBJ_COMMIT)\n+\t\tdie(_(\"internal error: uobj type %d, robj type %d\"),\n+\t\t    uobj->type, robj->type);\n+\trobj = &((struct commit *)robj)->tree->object;\n+\tuobj = &((struct commit *)uobj)->tree->object;\n+\tadd_object_array(uobj, uname, ent);\n+\tadd_object_array(robj, rname, ent);\n+\treturn (1);\n+}\n+\n int cmd_diff(int argc, const char **argv, const char *prefix)\n {\n \tint i;\n@@ -391,32 +482,40 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \t\t}\n \t}\n \n-\tfor (i = 0; i < rev.pending.nr; i++) {\n-\t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n-\t\tstruct object *obj = entry->item;\n-\t\tconst char *name = entry->name;\n-\t\tint flags = (obj->flags & UNINTERESTING);\n-\t\tif (!obj->parsed)\n-\t\t\tobj = parse_object(obj->oid.hash);\n-\t\tobj = deref_tag(obj, NULL, 0);\n-\t\tif (!obj)\n-\t\t\tdie(_(\"invalid object '%s' given.\"), name);\n-\t\tif (obj->type == OBJ_COMMIT)\n-\t\t\tobj = &((struct commit *)obj)->tree->object;\n-\n-\t\tif (obj->type == OBJ_TREE) {\n-\t\t\tobj->flags |= flags;\n-\t\t\tadd_object_array(obj, name, &ent);\n-\t\t} else if (obj->type == OBJ_BLOB) {\n-\t\t\tif (2 <= blobs)\n-\t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n-\t\t\thashcpy(blob[blobs].sha1, obj->oid.hash);\n-\t\t\tblob[blobs].name = name;\n-\t\t\tblob[blobs].mode = entry->mode;\n-\t\t\tblobs++;\n-\n-\t\t} else {\n-\t\t\tdie(_(\"unhandled object '%s' given.\"), name);\n+\t/*\n+\t * We treat symmetric difference specially.  Check that first;\n+\t * if it handled the arguments, it will put exactly two trees\n+\t * into ent.  Otherwise go on to convert objects to blobs\n+\t * and/or trees, putting any trees into the ent array.\n+\t */\n+\tif (!builtin_diff_sdiff(&rev, &ent)) {\n+\t\tfor (i = 0; i < rev.pending.nr; i++) {\n+\t\t\tstruct object_array_entry *entry = &rev.pending.objects[i];\n+\t\t\tstruct object *obj = entry->item;\n+\t\t\tconst char *name = entry->name;\n+\t\t\tint flags = (obj->flags & UNINTERESTING);\n+\t\t\tif (!obj->parsed)\n+\t\t\t\tobj = parse_object(obj->oid.hash);\n+\t\t\tobj = deref_tag(obj, NULL, 0);\n+\t\t\tif (!obj)\n+\t\t\t\tdie(_(\"invalid object '%s' given.\"), name);\n+\t\t\tif (obj->type == OBJ_COMMIT)\n+\t\t\t\tobj = &((struct commit *)obj)->tree->object;\n+\n+\t\t\tif (obj->type == OBJ_TREE) {\n+\t\t\t\tobj->flags |= flags;\n+\t\t\t\tadd_object_array(obj, name, &ent);\n+\t\t\t} else if (obj->type == OBJ_BLOB) {\n+\t\t\t\tif (2 <= blobs)\n+\t\t\t\t\tdie(_(\"more than two blobs given: '%s'\"), name);\n+\t\t\t\thashcpy(blob[blobs].sha1, obj->oid.hash);\n+\t\t\t\tblob[blobs].name = name;\n+\t\t\t\tblob[blobs].mode = entry->mode;\n+\t\t\t\tblobs++;\n+\n+\t\t\t} else {\n+\t\t\t\tdie(_(\"unhandled object '%s' given.\"), name);\n+\t\t\t}\n \t\t}\n \t}\n \tif (rev.prune_data.nr)\n@@ -451,19 +550,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix)\n \telse if (ent.nr == 2)\n \t\tresult = builtin_diff_tree(&rev, argc, argv,\n \t\t\t\t\t   &ent.objects[0], &ent.objects[1]);\n-\telse if (ent.objects[0].item->flags & UNINTERESTING) {\n-\t\t/*\n-\t\t * diff A...B where there is at least one merge base\n-\t\t * between A and B.  We have ent.objects[0] ==\n-\t\t * merge-base, ent.objects[ents-2] == A, and\n-\t\t * ent.objects[ents-1] == B.  Show diff between the\n-\t\t * base and B.  Note that we pick one merge base at\n-\t\t * random if there are more than one.\n-\t\t */\n-\t\tresult = builtin_diff_tree(&rev, argc, argv,\n-\t\t\t\t\t   &ent.objects[0],\n-\t\t\t\t\t   &ent.objects[ent.nr-1]);\n-\t} else\n+\telse\n \t\tresult = builtin_diff_combined(&rev, argc, argv,\n \t\t\t\t\t       ent.objects, ent.nr);\n \tresult = diff_result_code(&rev.diffopt, result);\n"}]}