| Message ID | 84018532-169f-dc9b-f894-3d19bb7b4245@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | revision: fix order of revs for ^! | expand |
René Scharfe <l.s.r@web.de> writes: > gitrevisions(7) says: "A suffix '^' followed by an exclamation mark is > the same as giving commit '<rev>' and then all its parents prefixed with > '^' to exclude them (and their ancestors)." I did not mean to specify the order at all in that description when I wrote ca5ee2d1 (Enumerate revision range specifiers in the documentation, 2012-07-24) and I do not think it should be read as such. > handle_revision_arg_1() however adds the negated parents first. I suspect that this was deliberately done so to match how A..B is added to the pending commit list in revisions.c::handle_dotdot_1() to tolerate "git diff A..B" as a synonym to "git diff A B", which dates back to cd2bdc53 (Common option parsing for "git log --diff" and friends, 2006-04-14). > Split up the handling of ^! by moving the actual addition of the > parents after the addition of the child. I do not offhand think of anything other than the "diff" frontend that cares about the order of these commits from the command line, I am afraid that this might end up robbing Peter to pay paul. Can't we "fix" it at the consumer end, perhaps by checking where these commits came from by looking at rev.cmdline?
Am 15.09.22 um 19:53 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> gitrevisions(7) says: "A suffix '^' followed by an exclamation mark is >> the same as giving commit '<rev>' and then all its parents prefixed with >> '^' to exclude them (and their ancestors)." > > I did not mean to specify the order at all in that description when > I wrote ca5ee2d1 (Enumerate revision range specifiers in the > documentation, 2012-07-24) and I do not think it should be read as > such. Then the "then" needs to go. git rev-parse has its own parser for ^! etc. and puts parents last, consistent with the "then". >> handle_revision_arg_1() however adds the negated parents first. > > I suspect that this was deliberately done so to match how A..B is > added to the pending commit list in revisions.c::handle_dotdot_1() > to tolerate "git diff A..B" as a synonym to "git diff A B", which > dates back to cd2bdc53 (Common option parsing for "git log --diff" > and friends, 2006-04-14). "git diff A B", "git diff A..B", "git diff ^A B" and "git diff "B ^A" all produce the same output. So I suspect we could reverse the order for A..B as well without ill effect if we wanted. >> Split up the handling of ^! by moving the actual addition of the >> parents after the addition of the child. > > I do not offhand think of anything other than the "diff" frontend > that cares about the order of these commits from the command line, I > am afraid that this might end up robbing Peter to pay paul. Can't think of anything, either, but it may well be possible that yet another untested use case could depend on getting parents first. "git diff R^!" was untested as well and it took two years from release to Tim's bug report after all. And I'm not particularly proud of my refactoring patches, so wouldn't mind dropping them. > Can't we "fix" it at the consumer end, perhaps by checking where > these commits came from by looking at rev.cmdline? We could. --- >8 --- Subject: [PATCH] diff: support ^! for merges revision.c::handle_revision_arg_1() resolves <rev>^! by first adding the negated parents and then <rev> itself. builtin_diff_combined() expects the first tree to be the merge and the remaining ones to be the parents, though. This mismatch results in bogus diff output. Remember the first tree that doesn't belong to a parent and use it instead of blindly picking the first one. This makes "git diff <rev>^!" consistent with "git show <rev>^!". Reported-by: Tim Jaacks <tim.jaacks@garz-fricke.com> Suggested-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- The check "i < rev.cmdline.nr" is necessary to avoid segfaults e.g. in t0005. I wonder why. Shouldn't rev.pending.objects and rev.cmdline.rev always have the same number of entries? builtin/diff.c | 23 ++++++++++++++++++----- t/t4038-diff-combined.sh | 10 ++++++++++ 2 files changed, 28 insertions(+), 5 deletions(-) diff --git a/builtin/diff.c b/builtin/diff.c index 54bb3de964..04c6ba0862 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -209,7 +209,7 @@ static int builtin_diff_tree(struct rev_info *revs, static int builtin_diff_combined(struct rev_info *revs, int argc, const char **argv, struct object_array_entry *ent, - int ents) + int ents, int first_non_parent) { struct oid_array parents = OID_ARRAY_INIT; int i; @@ -217,11 +217,18 @@ static int builtin_diff_combined(struct rev_info *revs, if (argc > 1) usage(builtin_diff_usage); + if (first_non_parent < 0) + die(_("no merge given, only parents.")); + if (first_non_parent >= ents) + BUG("first_non_parent out of range: %d", first_non_parent); + diff_merges_set_dense_combined_if_unset(revs); - for (i = 1; i < ents; i++) - oid_array_append(&parents, &ent[i].item->oid); - diff_tree_combined(&ent[0].item->oid, &parents, revs); + for (i = 0; i < ents; i++) { + if (i != first_non_parent) + oid_array_append(&parents, &ent[i].item->oid); + } + diff_tree_combined(&ent[first_non_parent].item->oid, &parents, revs); oid_array_clear(&parents); return 0; } @@ -385,6 +392,7 @@ int cmd_diff(int argc, const char **argv, const char *prefix) int i; struct rev_info rev; struct object_array ent = OBJECT_ARRAY_INIT; + int first_non_parent = -1; int blobs = 0, paths = 0; struct object_array_entry *blob[2]; int nongit = 0, no_index = 0; @@ -541,6 +549,10 @@ int cmd_diff(int argc, const char **argv, const char *prefix) if (obj->type == OBJ_TREE) { if (sdiff.skip && bitmap_get(sdiff.skip, i)) continue; + if (first_non_parent < 0 && + i < rev.cmdline.nr && + rev.cmdline.rev[i].whence != REV_CMD_PARENTS_ONLY) + first_non_parent = ent.nr; obj->flags |= flags; add_object_array(obj, name, &ent); } else if (obj->type == OBJ_BLOB) { @@ -590,7 +602,8 @@ int cmd_diff(int argc, const char **argv, const char *prefix) &ent.objects[0], &ent.objects[1]); } else result = builtin_diff_combined(&rev, argc, argv, - ent.objects, ent.nr); + ent.objects, ent.nr, + first_non_parent); result = diff_result_code(&rev.diffopt, result); if (1 < rev.diffopt.skip_stat_unmatch) refresh_index_quietly(); diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh index 9a292bac70..2ce26e585c 100755 --- a/t/t4038-diff-combined.sh +++ b/t/t4038-diff-combined.sh @@ -80,11 +80,21 @@ test_expect_success 'check combined output (1)' ' verify_helper sidewithone ' +test_expect_success 'check combined output (1) with git diff <rev>^!' ' + git diff sidewithone^! -- >sidewithone && + verify_helper sidewithone +' + test_expect_success 'check combined output (2)' ' git show sidesansone -- >sidesansone && verify_helper sidesansone ' +test_expect_success 'check combined output (2) with git diff <rev>^!' ' + git diff sidesansone^! -- >sidesansone && + verify_helper sidesansone +' + test_expect_success 'diagnose truncated file' ' >file && git add file && -- 2.37.3
René Scharfe <l.s.r@web.de> writes: > "git diff A B", "git diff A..B", "git diff ^A B" and "git diff "B ^A" > all produce the same output. So I suspect we could reverse the order > for A..B as well without ill effect if we wanted. Yup. >> Can't we "fix" it at the consumer end, perhaps by checking where >> these commits came from by looking at rev.cmdline? > > We could. Yeah, doing so may be more in line with how two-tree comparison is parsed (your observation above on four combinations). > --- >8 --- > Subject: [PATCH] diff: support ^! for merges > > revision.c::handle_revision_arg_1() resolves <rev>^! by first adding the > negated parents and then <rev> itself. builtin_diff_combined() expects > the first tree to be the merge and the remaining ones to be the parents, > though. This mismatch results in bogus diff output. > > Remember the first tree that doesn't belong to a parent and use it > instead of blindly picking the first one. This makes "git diff <rev>^!" > consistent with "git show <rev>^!". > > Reported-by: Tim Jaacks <tim.jaacks@garz-fricke.com> > Suggested-by: Junio C Hamano <gitster@pobox.com> > Signed-off-by: René Scharfe <l.s.r@web.de> > --- > The check "i < rev.cmdline.nr" is necessary to avoid segfaults e.g. > in t0005. I wonder why. Shouldn't rev.pending.objects and > rev.cmdline.rev always have the same number of entries? Things that did not come from command line can go into pending, can't they? E.g. add_head_to_pending(), when lack of rev defaults to "HEAD", touches pending without touching cmdline?
Am 16.09.22 um 17:55 schrieb Junio C Hamano: > René Scharfe <l.s.r@web.de> writes: > >> The check "i < rev.cmdline.nr" is necessary to avoid segfaults e.g. >> in t0005. I wonder why. Shouldn't rev.pending.objects and >> rev.cmdline.rev always have the same number of entries? > > Things that did not come from command line can go into pending, > can't they? E.g. add_head_to_pending(), when lack of rev defaults > to "HEAD", touches pending without touching cmdline? Indeed. In t0005 it's even only a fake HEAD with an empty_tree added by cmd_diff() a few lines up, for a diff of a new file in a fresh, empty repository. René
diff --git a/revision.c b/revision.c index 5e756b76aa..8385127f1a 100644 --- a/revision.c +++ b/revision.c @@ -2107,6 +2107,7 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl const char *arg = arg_; int cant_be_filename = revarg_opt & REVARG_CANNOT_BE_FILENAME; unsigned get_sha1_flags = GET_OID_RECORD_PATH; + struct commit *caret_bang_commit = NULL; flags = flags & UNINTERESTING ? flags | BOTTOM : flags & ~BOTTOM; @@ -2135,14 +2136,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl } mark = strstr(arg, "^!"); if (mark && !mark[2]) { - struct commit *commit; - *mark = 0; - commit = get_commit(revs, arg); - if (commit) - add_parents(revs, commit->parents, arg, - flags ^ (UNINTERESTING | BOTTOM)); - else + caret_bang_commit = get_commit(revs, arg); + if (!caret_bang_commit) *mark = '^'; } mark = strstr(arg, "^-"); @@ -2179,6 +2175,9 @@ static int handle_revision_arg_1(const char *arg_, struct rev_info *revs, int fl add_rev_cmdline(revs, object, arg_, REV_CMD_REV, flags ^ local_flags); add_pending_object_with_path(revs, object, arg, oc.mode, oc.path); free(oc.path); + if (caret_bang_commit) + add_parents(revs, caret_bang_commit->parents, arg, + flags ^ (UNINTERESTING | BOTTOM)); return 0; } diff --git a/t/t4038-diff-combined.sh b/t/t4038-diff-combined.sh index 9a292bac70..2ce26e585c 100755 --- a/t/t4038-diff-combined.sh +++ b/t/t4038-diff-combined.sh @@ -80,11 +80,21 @@ test_expect_success 'check combined output (1)' ' verify_helper sidewithone ' +test_expect_success 'check combined output (1) with git diff <rev>^!' ' + git diff sidewithone^! -- >sidewithone && + verify_helper sidewithone +' + test_expect_success 'check combined output (2)' ' git show sidesansone -- >sidesansone && verify_helper sidesansone ' +test_expect_success 'check combined output (2) with git diff <rev>^!' ' + git diff sidesansone^! -- >sidesansone && + verify_helper sidesansone +' + test_expect_success 'diagnose truncated file' ' >file && git add file &&
gitrevisions(7) says: "A suffix '^' followed by an exclamation mark is the same as giving commit '<rev>' and then all its parents prefixed with '^' to exclude them (and their ancestors)." handle_revision_arg_1() however adds the negated parents first. This leads to unexpected results with git diff and merge commits, as that command expects the documented order. Split up the handling of ^! by moving the actual addition of the parents after the addition of the child. Reported-by: Tim Jaacks <tim.jaacks@garz-fricke.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- revision.c | 13 ++++++------- t/t4038-diff-combined.sh | 10 ++++++++++ 2 files changed, 16 insertions(+), 7 deletions(-) -- 2.37.3