| Message ID | 20191209145312.3251-2-alext9@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | rebase: fix --fork-point with short ref upstream | expand |
Alex Torok <alext9@gmail.com> writes: > We were directly passing in the user-provided upstream_name into > get_fork_point(), but get_fork_point() was expecting a fully qualified > ref name. This resulted in `--fork-point` not working as expected. > Without the full ref name, get_fork_point could not find the fork point, > and rebase behaved as if no `--fork-point` flag was passed in. > > Pull logic for getting the full dwim ref name out of merge-base.c > handle_fork_point into get_fork_point. This allows both of the locations "handle_fork_point() into get_fork_point()" to match the second line of the first paragraph, perhaps (similarly for "get_fork_point()" on the next line)? > that call get_fork_point to not need to handle doing the dwim ref lookup > before trying to get the fork point. > > Duplicate all of the rebase tests with a short refname to ensure that > they work with short and long refnames. > > Signed-off-by: Alex Torok <alext9@gmail.com> > --- > > One thing that I'm not super sure about is the error messaging that gets > printed when a short refname is given to rebase. In the ambigous refname > test that I added, the command output is: > > warning: refname 'one' is ambiguous. > error: ambiguous refname: 'one' > fatal: could not get fork point > > Which seems a bit odd ot have a warning, error, fatal stacked on top of > each other. Yes, it does look strange. I think that the new "dwim_unique_ref()" helper is misdesigned as a generic helper function. It makes it hard for the callers to handle errors on their own for this function to return error(). A helper at the right level for this kind of thing would have been a function that does DWIM ref and returns the number of candidate refs the given short name would expand to, and do all that silently. The caller would then react to the returned number and say "no such" if there is no candidate, or "ambigous" if there are more than one. Which is exactly what dwim_ref() is. In other words, dwim_unique_ref() helper, unless it is useful by multiple callers who want it to die() itself, is not all that useful as an abstraction. If I were doing this, probably I would not add dwim_unique_ref() at all, and then I'd invent an extra variant of get_fork_point(), that the caller can choose to make it die or just silently return an error. This caller would want it to die() without giving control back to it, but there may be some other callers that would want a finer control. On the other hand, if the other caller(s) all want get_fork_point() to die, then that would also be fine. A quick glance tells me that the only other caller is in builtin/rebase.c and does this: if (fork_point > 0) { struct commit *head = lookup_commit_reference(the_repository, &options.orig_head); options.restrict_revision = get_fork_point(options.upstream_name, head); } and there are other calls to die(_("...")) in the caller everywhere, so I'd say you are over-engineering a simple bugfix. Wouldn't it be sufficient for this fix to be more like this? -- >8 -- Subject: rebase: --fork-point regression fix "git rebase --fork-point master" used to work OK, as it internally called "git merge-base --fork-point" that knew how to handle short refname and dwim it to the full refname before calling the underlying get_fork_point() function. This is no longer true after the command was rewritten in C, as its internall call made directly to get_fork_point() does not dwim a short ref. Move the "dwim the refname argument to the full refname" logic that is used in "git merge-base" to the underlying get_fork_point() function, so that the other caller of the function in the implementation of "git rebase" behaves the same way to fix this regression. --- builtin/merge-base.c | 12 +----------- commit.c | 15 +++++++++++++-- t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/builtin/merge-base.c b/builtin/merge-base.c index e3f8da13b6..6719ac198d 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c @@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv) static int handle_fork_point(int argc, const char **argv) { struct object_id oid; - char *refname; struct commit *derived, *fork_point; const char *commitname; - switch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) { - case 0: - die("No such ref: '%s'", argv[0]); - case 1: - break; /* good */ - default: - die("Ambiguous refname: '%s'", argv[0]); - } - commitname = (argc == 2) ? argv[1] : "HEAD"; if (get_oid(commitname, &oid)) die("Not a valid object name: '%s'", commitname); derived = lookup_commit_reference(the_repository, &oid); - fork_point = get_fork_point(refname, derived); + fork_point = get_fork_point(argv[0], derived); if (!fork_point) return 1; diff --git a/commit.c b/commit.c index 40890ae7ce..016f14fe95 100644 --- a/commit.c +++ b/commit.c @@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) struct commit_list *bases; int i; struct commit *ret = NULL; + char *full_refname; + + switch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) { + case 0: + die("No such ref: '%s'", refname); + case 1: + break; /* good */ + default: + die("Ambiguous refname: '%s'", refname); + } memset(&revs, 0, sizeof(revs)); revs.initial = 1; - for_each_reflog_ent(refname, collect_one_reflog_ent, &revs); + for_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs); - if (!revs.nr && !get_oid(refname, &oid)) + if (!revs.nr) add_one_commit(&oid, &revs); for (i = 0; i < revs.nr; i++) @@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) cleanup_return: free_commit_list(bases); + free(full_refname); return ret; } diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh index 78851b9a2a..5b09aecd13 100755 --- a/t/t3431-rebase-fork-point.sh +++ b/t/t3431-rebase-fork-point.sh @@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base test_rebase 'G F C E D B A' --no-fork-point test_rebase 'G F C D B A' --no-fork-point --onto D test_rebase 'G F C B A' --no-fork-point --keep-base + test_rebase 'G F E D B A' --fork-point refs/heads/master +test_rebase 'G F E D B A' --fork-point master + test_rebase 'G F D B A' --fork-point --onto D refs/heads/master +test_rebase 'G F D B A' --fork-point --onto D master + test_rebase 'G F B A' --fork-point --keep-base refs/heads/master +test_rebase 'G F B A' --fork-point --keep-base master + test_rebase 'G F C E D B A' refs/heads/master +test_rebase 'G F C E D B A' master + test_rebase 'G F C D B A' --onto D refs/heads/master +test_rebase 'G F C D B A' --onto D master + test_rebase 'G F C B A' --keep-base refs/heads/master +test_rebase 'G F C B A' --keep-base master + +test_expect_success "git rebase --fork-point with ambigous refname" " + git checkout master && + git checkout -b one && + git checkout side && + git tag one && + test_must_fail git rebase --fork-point --onto D one +" test_done
On Mon, Dec 9, 2019 at 1:51 PM Junio C Hamano <gitster@pobox.com> wrote: > and there are other calls to die(_("...")) in the caller everywhere, > so I'd say you are over-engineering a simple bugfix. > > Wouldn't it be sufficient for this fix to be more like this? This is essentially what I had done in the v2 version of my patch with the only "big" difference being pulling out the dwim_ref() switching and dying logic into a dwim_ref_or_die() function. Let me know if you want me to adjust my patch at all (my v2 patch is missing a call to free()...). If the patch that you replied here with is sufficient for how you would fix it, I'm fine with you just using that. > -- >8 -- > > Subject: rebase: --fork-point regression fix > > "git rebase --fork-point master" used to work OK, as it internally > called "git merge-base --fork-point" that knew how to handle short > refname and dwim it to the full refname before calling the > underlying get_fork_point() function. > > This is no longer true after the command was rewritten in C, as its > internall call made directly to get_fork_point() does not dwim a > short ref. > > Move the "dwim the refname argument to the full refname" logic that > is used in "git merge-base" to the underlying get_fork_point() > function, so that the other caller of the function in the > implementation of "git rebase" behaves the same way to fix this > regression. > > --- > builtin/merge-base.c | 12 +----------- > commit.c | 15 +++++++++++++-- > t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++ > 3 files changed, 34 insertions(+), 13 deletions(-) > > diff --git a/builtin/merge-base.c b/builtin/merge-base.c > index e3f8da13b6..6719ac198d 100644 > --- a/builtin/merge-base.c > +++ b/builtin/merge-base.c > @@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv) > static int handle_fork_point(int argc, const char **argv) > { > struct object_id oid; > - char *refname; > struct commit *derived, *fork_point; > const char *commitname; > > - switch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) { > - case 0: > - die("No such ref: '%s'", argv[0]); > - case 1: > - break; /* good */ > - default: > - die("Ambiguous refname: '%s'", argv[0]); > - } > - > commitname = (argc == 2) ? argv[1] : "HEAD"; > if (get_oid(commitname, &oid)) > die("Not a valid object name: '%s'", commitname); > > derived = lookup_commit_reference(the_repository, &oid); > > - fork_point = get_fork_point(refname, derived); > + fork_point = get_fork_point(argv[0], derived); > > if (!fork_point) > return 1; > diff --git a/commit.c b/commit.c > index 40890ae7ce..016f14fe95 100644 > --- a/commit.c > +++ b/commit.c > @@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) > struct commit_list *bases; > int i; > struct commit *ret = NULL; > + char *full_refname; > + > + switch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) { > + case 0: > + die("No such ref: '%s'", refname); > + case 1: > + break; /* good */ > + default: > + die("Ambiguous refname: '%s'", refname); > + } > > memset(&revs, 0, sizeof(revs)); > revs.initial = 1; > - for_each_reflog_ent(refname, collect_one_reflog_ent, &revs); > + for_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs); > > - if (!revs.nr && !get_oid(refname, &oid)) > + if (!revs.nr) > add_one_commit(&oid, &revs); > > for (i = 0; i < revs.nr; i++) > @@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) > > cleanup_return: > free_commit_list(bases); > + free(full_refname); > return ret; > } > > diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh > index 78851b9a2a..5b09aecd13 100755 > --- a/t/t3431-rebase-fork-point.sh > +++ b/t/t3431-rebase-fork-point.sh > @@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base > test_rebase 'G F C E D B A' --no-fork-point > test_rebase 'G F C D B A' --no-fork-point --onto D > test_rebase 'G F C B A' --no-fork-point --keep-base > + > test_rebase 'G F E D B A' --fork-point refs/heads/master > +test_rebase 'G F E D B A' --fork-point master > + > test_rebase 'G F D B A' --fork-point --onto D refs/heads/master > +test_rebase 'G F D B A' --fork-point --onto D master > + > test_rebase 'G F B A' --fork-point --keep-base refs/heads/master > +test_rebase 'G F B A' --fork-point --keep-base master > + > test_rebase 'G F C E D B A' refs/heads/master > +test_rebase 'G F C E D B A' master > + > test_rebase 'G F C D B A' --onto D refs/heads/master > +test_rebase 'G F C D B A' --onto D master > + > test_rebase 'G F C B A' --keep-base refs/heads/master > +test_rebase 'G F C B A' --keep-base master > + > +test_expect_success "git rebase --fork-point with ambigous refname" " > + git checkout master && > + git checkout -b one && > + git checkout side && > + git tag one && > + test_must_fail git rebase --fork-point --onto D one > +" > > test_done
Hi Alex, There are a couple of small issues with the patch below. If you iterate on it, here are some small suggestions: On Mon, Dec 09, 2019 at 10:51:47AM -0800, Junio C Hamano wrote: > Subject: rebase: --fork-point regression fix > > "git rebase --fork-point master" used to work OK, as it internally > called "git merge-base --fork-point" that knew how to handle short > refname and dwim it to the full refname before calling the > underlying get_fork_point() function. > > This is no longer true after the command was rewritten in C, as its > internall call made directly to get_fork_point() does not dwim a > short ref. > > Move the "dwim the refname argument to the full refname" logic that > is used in "git merge-base" to the underlying get_fork_point() > function, so that the other caller of the function in the > implementation of "git rebase" behaves the same way to fix this > regression. > > --- > builtin/merge-base.c | 12 +----------- > commit.c | 15 +++++++++++++-- > t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++ > 3 files changed, 34 insertions(+), 13 deletions(-) > > diff --git a/builtin/merge-base.c b/builtin/merge-base.c > index e3f8da13b6..6719ac198d 100644 > --- a/builtin/merge-base.c > +++ b/builtin/merge-base.c > @@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv) > static int handle_fork_point(int argc, const char **argv) > { > struct object_id oid; > - char *refname; > struct commit *derived, *fork_point; > const char *commitname; > > - switch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) { > - case 0: > - die("No such ref: '%s'", argv[0]); > - case 1: > - break; /* good */ > - default: > - die("Ambiguous refname: '%s'", argv[0]); > - } > - In the unabridged v3, you cleaned this up by marking it for translation and also lowercasing the first letter of the sentence. That was good. If you iterate on this, could you create a preparatory patch before this one that gives the same treatment to the other strings in merge-base? I count 8 instances of die() being called in merge-base so I think that the cleanup shouldn't be too arduous. If we have a preparatory patch before this one, this patch will mostly be pure code movement which would be nice. > commitname = (argc == 2) ? argv[1] : "HEAD"; > if (get_oid(commitname, &oid)) > die("Not a valid object name: '%s'", commitname); > > derived = lookup_commit_reference(the_repository, &oid); > > - fork_point = get_fork_point(refname, derived); > + fork_point = get_fork_point(argv[0], derived); > > if (!fork_point) > return 1; > diff --git a/commit.c b/commit.c > index 40890ae7ce..016f14fe95 100644 > --- a/commit.c > +++ b/commit.c > @@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) > struct commit_list *bases; > int i; > struct commit *ret = NULL; > + char *full_refname; > + > + switch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) { > + case 0: > + die("No such ref: '%s'", refname); > + case 1: > + break; /* good */ > + default: > + die("Ambiguous refname: '%s'", refname); > + } Yeah, we should fix these strings in the next iteration. > > memset(&revs, 0, sizeof(revs)); > revs.initial = 1; > - for_each_reflog_ent(refname, collect_one_reflog_ent, &revs); > + for_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs); > > - if (!revs.nr && !get_oid(refname, &oid)) > + if (!revs.nr) > add_one_commit(&oid, &revs); > > for (i = 0; i < revs.nr; i++) > @@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) > > cleanup_return: > free_commit_list(bases); > + free(full_refname); > return ret; > } > > diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh > index 78851b9a2a..5b09aecd13 100755 > --- a/t/t3431-rebase-fork-point.sh > +++ b/t/t3431-rebase-fork-point.sh > @@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base > test_rebase 'G F C E D B A' --no-fork-point > test_rebase 'G F C D B A' --no-fork-point --onto D > test_rebase 'G F C B A' --no-fork-point --keep-base > + > test_rebase 'G F E D B A' --fork-point refs/heads/master > +test_rebase 'G F E D B A' --fork-point master > + > test_rebase 'G F D B A' --fork-point --onto D refs/heads/master > +test_rebase 'G F D B A' --fork-point --onto D master > + > test_rebase 'G F B A' --fork-point --keep-base refs/heads/master > +test_rebase 'G F B A' --fork-point --keep-base master > + > test_rebase 'G F C E D B A' refs/heads/master > +test_rebase 'G F C E D B A' master > + > test_rebase 'G F C D B A' --onto D refs/heads/master > +test_rebase 'G F C D B A' --onto D master > + > test_rebase 'G F C B A' --keep-base refs/heads/master > +test_rebase 'G F C B A' --keep-base master > + > +test_expect_success "git rebase --fork-point with ambigous refname" " > + git checkout master && > + git checkout -b one && > + git checkout side && > + git tag one && > + test_must_fail git rebase --fork-point --onto D one > +" nit: use double-quotes instead of single-quotes to surround both the test case name and the actual code itself. Thanks, Denton > > test_done
On Wed, Dec 11, 2019 at 7:20 AM Denton Liu <liu.denton@gmail.com> wrote: > > +test_expect_success "git rebase --fork-point with ambigous refname" " > > + git checkout master && > > + git checkout -b one && > > + git checkout side && > > + git tag one && > > + test_must_fail git rebase --fork-point --onto D one > > +" > > nit: use double-quotes instead of single-quotes to surround both the > test case name and the actual code itself. Denton meant to say that you should single-quote the test title and test body (not double-quote). In this particular test, the distinction doesn't matter presently, though it could matter if the test is later changed. In particular, if the test does any sort of variable interpolation (such as $foo), then the single quotes ensure that the interpolation will occur when the test is actually run (which is almost always what is desired) rather than at the time the test is defined (which is almost never wanted).
"git rebase --fork-point master" used to work OK, as it internally called "git merge-base --fork-point" that knew how to handle short refname and dwim it to the full refname before calling the underlying get_fork_point() function. This is no longer true after the command was rewritten in C, as its internall call made directly to get_fork_point() does not dwim a short ref. Move the "dwim the refname argument to the full refname" logic that is used in "git merge-base" to the underlying get_fork_point() function, so that the other caller of the function in the implementation of "git rebase" behaves the same way to fix this regression. Signed-off-by: Alex Torok <alext9@gmail.com> [jc: revamped the fix and used Alex's tests] Signed-off-by: Junio C Hamano <gitster@pobox.com> --- * I was about to discard stalled topics and this caught my eyes. builtin/merge-base.c | 12 +----------- commit.c | 15 +++++++++++++-- t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++ 3 files changed, 34 insertions(+), 13 deletions(-) diff --git a/builtin/merge-base.c b/builtin/merge-base.c index e3f8da13b6..6719ac198d 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c @@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv) static int handle_fork_point(int argc, const char **argv) { struct object_id oid; - char *refname; struct commit *derived, *fork_point; const char *commitname; - switch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) { - case 0: - die("No such ref: '%s'", argv[0]); - case 1: - break; /* good */ - default: - die("Ambiguous refname: '%s'", argv[0]); - } - commitname = (argc == 2) ? argv[1] : "HEAD"; if (get_oid(commitname, &oid)) die("Not a valid object name: '%s'", commitname); derived = lookup_commit_reference(the_repository, &oid); - fork_point = get_fork_point(refname, derived); + fork_point = get_fork_point(argv[0], derived); if (!fork_point) return 1; diff --git a/commit.c b/commit.c index 40890ae7ce..016f14fe95 100644 --- a/commit.c +++ b/commit.c @@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) struct commit_list *bases; int i; struct commit *ret = NULL; + char *full_refname; + + switch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) { + case 0: + die("No such ref: '%s'", refname); + case 1: + break; /* good */ + default: + die("Ambiguous refname: '%s'", refname); + } memset(&revs, 0, sizeof(revs)); revs.initial = 1; - for_each_reflog_ent(refname, collect_one_reflog_ent, &revs); + for_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs); - if (!revs.nr && !get_oid(refname, &oid)) + if (!revs.nr) add_one_commit(&oid, &revs); for (i = 0; i < revs.nr; i++) @@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) cleanup_return: free_commit_list(bases); + free(full_refname); return ret; } diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh index 78851b9a2a..172562789e 100755 --- a/t/t3431-rebase-fork-point.sh +++ b/t/t3431-rebase-fork-point.sh @@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base test_rebase 'G F C E D B A' --no-fork-point test_rebase 'G F C D B A' --no-fork-point --onto D test_rebase 'G F C B A' --no-fork-point --keep-base + test_rebase 'G F E D B A' --fork-point refs/heads/master +test_rebase 'G F E D B A' --fork-point master + test_rebase 'G F D B A' --fork-point --onto D refs/heads/master +test_rebase 'G F D B A' --fork-point --onto D master + test_rebase 'G F B A' --fork-point --keep-base refs/heads/master +test_rebase 'G F B A' --fork-point --keep-base master + test_rebase 'G F C E D B A' refs/heads/master +test_rebase 'G F C E D B A' master + test_rebase 'G F C D B A' --onto D refs/heads/master +test_rebase 'G F C D B A' --onto D master + test_rebase 'G F C B A' --keep-base refs/heads/master +test_rebase 'G F C B A' --keep-base master + +test_expect_success 'git rebase --fork-point with ambigous refname' ' + git checkout master && + git checkout -b one && + git checkout side && + git tag one && + test_must_fail git rebase --fork-point --onto D one +' test_done
diff --git a/builtin/merge-base.c b/builtin/merge-base.c index e3f8da13b6..ecc4268d43 100644 --- a/builtin/merge-base.c +++ b/builtin/merge-base.c @@ -114,26 +114,18 @@ static int handle_is_ancestor(int argc, const char **argv) static int handle_fork_point(int argc, const char **argv) { struct object_id oid; - char *refname; struct commit *derived, *fork_point; const char *commitname; - switch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) { - case 0: - die("No such ref: '%s'", argv[0]); - case 1: - break; /* good */ - default: - die("Ambiguous refname: '%s'", argv[0]); - } - commitname = (argc == 2) ? argv[1] : "HEAD"; if (get_oid(commitname, &oid)) die("Not a valid object name: '%s'", commitname); derived = lookup_commit_reference(the_repository, &oid); - fork_point = get_fork_point(refname, derived); + fork_point = NULL; + if (get_fork_point(argv[0], derived, &fork_point) < 0) + return 1; if (!fork_point) return 1; diff --git a/builtin/rebase.c b/builtin/rebase.c index e755087b0f..782cf5a890 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -1980,8 +1980,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) struct commit *head = lookup_commit_reference(the_repository, &options.orig_head); - options.restrict_revision = - get_fork_point(options.upstream_name, head); + options.restrict_revision = NULL; + if (get_fork_point(options.upstream_name, head, &options.restrict_revision) < 0) + die(_("could not get fork point")); } if (repo_read_index(the_repository) < 0) diff --git a/commit.c b/commit.c index 434ec030d6..874bc0cdf1 100644 --- a/commit.c +++ b/commit.c @@ -920,19 +920,25 @@ static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid return 0; } -struct commit *get_fork_point(const char *refname, struct commit *commit) +int get_fork_point(const char *refname, struct commit *commit, struct commit **fork_point) { struct object_id oid; struct rev_collect revs; struct commit_list *bases; int i; - struct commit *ret = NULL; + char *full_refname; + + if (dwim_unique_ref(refname, strlen(refname), &oid, &full_refname) < 0) { + free(full_refname); + return -1; + } + memset(&revs, 0, sizeof(revs)); revs.initial = 1; - for_each_reflog_ent(refname, collect_one_reflog_ent, &revs); + for_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs); - if (!revs.nr && !get_oid(refname, &oid)) + if (!revs.nr && !get_oid(full_refname, &oid)) add_one_commit(&oid, &revs); for (i = 0; i < revs.nr; i++) @@ -954,11 +960,12 @@ struct commit *get_fork_point(const char *refname, struct commit *commit) if (revs.nr <= i) goto cleanup_return; - ret = bases->item; + *fork_point = bases->item; cleanup_return: + free(full_refname); free_commit_list(bases); - return ret; + return 0; } static const char gpg_sig_header[] = "gpgsig"; diff --git a/commit.h b/commit.h index 221cdaa34b..d79a8eab48 100644 --- a/commit.h +++ b/commit.h @@ -240,7 +240,7 @@ int register_commit_graft(struct repository *r, struct commit_graft *, int); void prepare_commit_graft(struct repository *r); struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid); -struct commit *get_fork_point(const char *refname, struct commit *commit); +int get_fork_point(const char *refname, struct commit *commit, struct commit **fork_point); /* largest positive number a signed 32-bit integer can contain */ #define INFINITE_DEPTH 0x7fffffff diff --git a/refs.c b/refs.c index 1ab0bb54d3..853e45a6a3 100644 --- a/refs.c +++ b/refs.c @@ -639,6 +639,20 @@ int dwim_ref(const char *str, int len, struct object_id *oid, char **ref) return repo_dwim_ref(the_repository, str, len, oid, ref); } +int dwim_unique_ref(const char* str, int len, struct object_id *oid, char **ref) +{ + struct object_id discard; + switch (dwim_ref(str, len, &discard, ref)) { + case 0: + return error(_("no such ref: '%s'"), str); + case 1: + break; /* good */ + default: + return error(_("ambiguous refname: '%s'"), str); + } + return 0; +} + int expand_ref(struct repository *repo, const char *str, int len, struct object_id *oid, char **ref) { diff --git a/refs.h b/refs.h index 730d05ad91..7aa348052b 100644 --- a/refs.h +++ b/refs.h @@ -154,6 +154,8 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_ int dwim_ref(const char *str, int len, struct object_id *oid, char **ref); int dwim_log(const char *str, int len, struct object_id *oid, char **ref); +int dwim_unique_ref(const char* str, int len, struct object_id *oid, char **ref); + /* * A ref_transaction represents a collection of reference updates that * should succeed or fail together. diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh index 78851b9a2a..5b09aecd13 100755 --- a/t/t3431-rebase-fork-point.sh +++ b/t/t3431-rebase-fork-point.sh @@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base test_rebase 'G F C E D B A' --no-fork-point test_rebase 'G F C D B A' --no-fork-point --onto D test_rebase 'G F C B A' --no-fork-point --keep-base + test_rebase 'G F E D B A' --fork-point refs/heads/master +test_rebase 'G F E D B A' --fork-point master + test_rebase 'G F D B A' --fork-point --onto D refs/heads/master +test_rebase 'G F D B A' --fork-point --onto D master + test_rebase 'G F B A' --fork-point --keep-base refs/heads/master +test_rebase 'G F B A' --fork-point --keep-base master + test_rebase 'G F C E D B A' refs/heads/master +test_rebase 'G F C E D B A' master + test_rebase 'G F C D B A' --onto D refs/heads/master +test_rebase 'G F C D B A' --onto D master + test_rebase 'G F C B A' --keep-base refs/heads/master +test_rebase 'G F C B A' --keep-base master + +test_expect_success "git rebase --fork-point with ambigous refname" " + git checkout master && + git checkout -b one && + git checkout side && + git tag one && + test_must_fail git rebase --fork-point --onto D one +" test_done
We were directly passing in the user-provided upstream_name into get_fork_point(), but get_fork_point() was expecting a fully qualified ref name. This resulted in `--fork-point` not working as expected. Without the full ref name, get_fork_point could not find the fork point, and rebase behaved as if no `--fork-point` flag was passed in. Pull logic for getting the full dwim ref name out of merge-base.c handle_fork_point into get_fork_point. This allows both of the locations that call get_fork_point to not need to handle doing the dwim ref lookup before trying to get the fork point. Duplicate all of the rebase tests with a short refname to ensure that they work with short and long refnames. Signed-off-by: Alex Torok <alext9@gmail.com> --- One thing that I'm not super sure about is the error messaging that gets printed when a short refname is given to rebase. In the ambigous refname test that I added, the command output is: warning: refname 'one' is ambiguous. error: ambiguous refname: 'one' fatal: could not get fork point Which seems a bit odd ot have a warning, error, fatal stacked on top of each other. From a user-facing perspective it feels a bit odd to me, but I'm not sure what the general rules for error messaging are in git. builtin/merge-base.c | 14 +++----------- builtin/rebase.c | 5 +++-- commit.c | 19 +++++++++++++------ commit.h | 2 +- refs.c | 14 ++++++++++++++ refs.h | 2 ++ t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++ 7 files changed, 56 insertions(+), 20 deletions(-)