{"thread":{"id":"52391","subject":"[PATCH 0/3] rebase: fix bug in --fork-point","startedAt":"2019-12-05T22:55:07Z","lastAt":"2020-02-11T18:15:37Z","messageCount":23,"participants":["Alex Torok","Junio C Hamano","Denton Liu","Phillip Wood","Eric Sunshine"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"387586","messageId":"20191205225322.5529-1-alext9@gmail.com","threadId":"52391","inReplyTo":null,"subject":"[PATCH 0/3] rebase: fix bug in --fork-point","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T22:53:19Z","receivedAt":"2019-12-05T22:55:07Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"git rebase --fork-point with a short branch ref name (without\nrefs/heads/ prefix) for the upstream will not use the fork point between\nthe upstream and the branch.\n\nIf git rebase --forkpoint is used with a full ref on the upstream\n(refs/heads/branchName), the fork point between the branch and upstream\nis correctly used.\n\nThis bug was introduced in 2.20.0 with the c implementation of rebase. I\nwas able to reproduce it on next and master.\n\nThe code path for rebase with --fork-point and a user-provided upstream\ndoes not use a DWIM method to look up the full ref name. This leads to\nget_fork_point returning a null pointer and rebase using the upstream\nitself instead of the fork point between the upstream and branch.\n\nI looked in other places that call get_fork_point and found that\nin handle_fork_point of builtin/merge_base.c, dwim_ref is used to find\nthe full ref name before calling get_fork_point.\n\nI initially based this change off of maint, but then saw that there were\nsome tests added for rebase --fork-point on master, so I rebased onto\nthere to build off of those tests, but I'm not sure if that was the \"right\"\nthing to do.\n\nAlex Torok (3):\n  rebase: add test for rebase --fork-point with short upstream\n  rebase: refactor dwim_ref_or_die from merge-base.c\n  rebase: fix rebase to use full ref to find fork-point\n\n builtin/merge-base.c         |  9 +--------\n builtin/rebase.c             |  4 +++-\n refs.c                       | 14 ++++++++++++++\n refs.h                       |  1 +\n t/t3431-rebase-fork-point.sh |  1 +\n 5 files changed, 20 insertions(+), 9 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"387587","messageId":"20191205225322.5529-2-alext9@gmail.com","threadId":"52391","inReplyTo":"20191205225322.5529-1-alext9@gmail.com","subject":"[PATCH 1/3] rebase: add test for rebase --fork-point with short upstream","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T22:53:20Z","receivedAt":"2019-12-05T22:55:09Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"This proves the bug of \"rebase --fork-point upstream branch\" not using\nthe fork point of upstream and branch if upstream is not the full\nrefname of the upstream branch.\n\nSigned-off-by: Alex Torok <alext9@gmail.com>\n---\n t/t3431-rebase-fork-point.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 78851b9a2a..6ecdae918e 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -49,6 +49,7 @@ test_rebase 'G F C D B A' --no-fork-point --onto D\n test_rebase 'G F C B A' --no-fork-point --keep-base\n test_rebase 'G F E D B A' --fork-point refs/heads/master\n test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n+test_rebase 'G F D B A' --fork-point --onto D master\n test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n test_rebase 'G F C E D B A' refs/heads/master\n test_rebase 'G F C D B A' --onto D refs/heads/master\n-- \n2.17.1\n\n"},{"id":"387588","messageId":"20191205225322.5529-3-alext9@gmail.com","threadId":"52391","inReplyTo":"20191205225322.5529-1-alext9@gmail.com","subject":"[PATCH 2/3] rebase: refactor dwim_ref_or_die from merge-base.c","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T22:53:21Z","receivedAt":"2019-12-05T22:55:11Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"Pull logic for getting the full dwim ref name from a passed in ref name\nout of the handle_fork_point function of merge-base.c. This will allow\ngit-rebase --fork-point to use the same method for getting the full ref\nname before calling get_fork_point.\n\nI saw other *_or_die methods in other places and using that pattern\nseemed sane here.\n\nI'm not 100% sure about the name or signature of dwim_ref_or_die. I feel\nlike it should be named something like dwim_ref_name_or_die,\nunique_dwim_ref_or_die, or should be including the object_id argument\neven though it isn't used by the calling merge_base code, and won't be\nused in rebase.\n\nThis is my first patch submission for git, and I'd appreciate some\nfeedback on naming/style wrt to this.\n\nSigned-off-by: Alex Torok <alext9@gmail.com>\n---\n builtin/merge-base.c |  9 +--------\n refs.c               | 14 ++++++++++++++\n refs.h               |  1 +\n 3 files changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex e3f8da13b6..edd16f9fcd 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -118,14 +118,7 @@ static int handle_fork_point(int argc, const char **argv)\n \tstruct commit *derived, *fork_point;\n \tconst char *commitname;\n \n-\tswitch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n-\tcase 0:\n-\t\tdie(\"No such ref: '%s'\", argv[0]);\n-\tcase 1:\n-\t\tbreak; /* good */\n-\tdefault:\n-\t\tdie(\"Ambiguous refname: '%s'\", argv[0]);\n-\t}\n+\tdwim_ref_or_die(argv[0], strlen(argv[0]), &refname);\n \n \tcommitname = (argc == 2) ? argv[1] : \"HEAD\";\n \tif (get_oid(commitname, &oid))\ndiff --git a/refs.c b/refs.c\nindex 1ab0bb54d3..3b778f2df9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -639,6 +639,20 @@ int dwim_ref(const char *str, int len, struct object_id *oid, char **ref)\n \treturn repo_dwim_ref(the_repository, str, len, oid, ref);\n }\n \n+void dwim_ref_or_die(const char *str, int len, char **ref)\n+{\n+\tstruct object_id discard;\n+\tswitch (dwim_ref(str, len, &discard, ref)) {\n+\tcase 0:\n+\t\tdie(\"No such ref: '%s'\", str);\n+\tcase 1:\n+\t\tbreak; /* good */\n+\tdefault:\n+\t\tdie(\"Ambiguous refname: '%s'\", str);\n+\t}\n+}\n+\n+\n int expand_ref(struct repository *repo, const char *str, int len,\n \t       struct object_id *oid, char **ref)\n {\ndiff --git a/refs.h b/refs.h\nindex 730d05ad91..a961662382 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -154,6 +154,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_\n int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);\n int dwim_log(const char *str, int len, struct object_id *oid, char **ref);\n \n+void dwim_ref_or_die(const char *str, int len, char **ref);\n /*\n  * A ref_transaction represents a collection of reference updates that\n  * should succeed or fail together.\n-- \n2.17.1\n\n"},{"id":"387589","messageId":"20191205225322.5529-4-alext9@gmail.com","threadId":"52391","inReplyTo":"20191205225322.5529-1-alext9@gmail.com","subject":"[PATCH 3/3] rebase: fix rebase to use full ref to find fork-point","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T22:53:22Z","receivedAt":"2019-12-05T22:55:12Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"rebase --fork-point needs to look up the full ref name before calling\nget_fork_point in the same manner that merge-base --fork-point does.\n\nSigned-off-by: Alex Torok <alext9@gmail.com>\n---\n builtin/rebase.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e755087b0f..821994f676 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tstruct commit *head =\n \t\t\tlookup_commit_reference(the_repository,\n \t\t\t\t\t\t&options.orig_head);\n+\t\tchar * full_name;\n+\t\tdwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);\n \t\toptions.restrict_revision =\n-\t\t\tget_fork_point(options.upstream_name, head);\n+\t\t\tget_fork_point(full_name, head);\n \t}\n \n \tif (repo_read_index(the_repository) < 0)\n-- \n2.17.1\n\n"},{"id":"387590","messageId":"xmqqv9quxtle.fsf@gitster-ct.c.googlers.com","threadId":"52391","inReplyTo":"20191205225322.5529-2-alext9@gmail.com","subject":"Re: [PATCH 1/3] rebase: add test for rebase --fork-point with short upstream","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-05T23:04:13Z","receivedAt":"2019-12-05T23:04:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Torok <alext9@gmail.com> writes:\n\n> This proves the bug of \"rebase --fork-point upstream branch\" not using\n> the fork point of upstream and branch if upstream is not the full\n> refname of the upstream branch.\n>\n> Signed-off-by: Alex Torok <alext9@gmail.com>\n> ---\n>  t/t3431-rebase-fork-point.sh | 1 +\n>  1 file changed, 1 insertion(+)\n\nIs this new test expected to fail after applying only 1/3 and then\nstarts working after applying all 3 patches?\n\nIf so, it probably makes a lot mroe sense to reorder the series to\nhave 2/3 as a single preparatory patch, with 1/3 + 3/3 combined into\na single patch \"rebase: find --fork-point with full ref\" to fix the\ncode and protect the fix with the test at the same time.\n\n> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\n> index 78851b9a2a..6ecdae918e 100755\n> --- a/t/t3431-rebase-fork-point.sh\n> +++ b/t/t3431-rebase-fork-point.sh\n> @@ -49,6 +49,7 @@ test_rebase 'G F C D B A' --no-fork-point --onto D\n>  test_rebase 'G F C B A' --no-fork-point --keep-base\n>  test_rebase 'G F E D B A' --fork-point refs/heads/master\n>  test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n> +test_rebase 'G F D B A' --fork-point --onto D master\n>  test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n>  test_rebase 'G F C E D B A' refs/heads/master\n>  test_rebase 'G F C D B A' --onto D refs/heads/master\n"},{"id":"387592","messageId":"CANmPhj20-K1f4sL6m3YmZEy+9WdV9YknXfUdLsGCaehv+axb+Q@mail.gmail.com","threadId":"52391","inReplyTo":"xmqqv9quxtle.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH 1/3] rebase: add test for rebase --fork-point with short upstream","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T23:25:36Z","receivedAt":"2019-12-05T23:25:51Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"That was the intention. Your idea for ordering makes more sense.\n\nI'll reorder the commits and submit new patches.\n\n\nOn Thu, Dec 5, 2019 at 6:04 PM Junio C Hamano <gitster@pobox.com> wrote:\n>\n> Alex Torok <alext9@gmail.com> writes:\n>\n> > This proves the bug of \"rebase --fork-point upstream branch\" not using\n> > the fork point of upstream and branch if upstream is not the full\n> > refname of the upstream branch.\n> >\n> > Signed-off-by: Alex Torok <alext9@gmail.com>\n> > ---\n> >  t/t3431-rebase-fork-point.sh | 1 +\n> >  1 file changed, 1 insertion(+)\n>\n> Is this new test expected to fail after applying only 1/3 and then\n> starts working after applying all 3 patches?\n>\n> If so, it probably makes a lot mroe sense to reorder the series to\n> have 2/3 as a single preparatory patch, with 1/3 + 3/3 combined into\n> a single patch \"rebase: find --fork-point with full ref\" to fix the\n> code and protect the fix with the test at the same time.\n>\n> > diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\n> > index 78851b9a2a..6ecdae918e 100755\n> > --- a/t/t3431-rebase-fork-point.sh\n> > +++ b/t/t3431-rebase-fork-point.sh\n> > @@ -49,6 +49,7 @@ test_rebase 'G F C D B A' --no-fork-point --onto D\n> >  test_rebase 'G F C B A' --no-fork-point --keep-base\n> >  test_rebase 'G F E D B A' --fork-point refs/heads/master\n> >  test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n> > +test_rebase 'G F D B A' --fork-point --onto D master\n> >  test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n> >  test_rebase 'G F C E D B A' refs/heads/master\n> >  test_rebase 'G F C D B A' --onto D refs/heads/master\n"},{"id":"387593","messageId":"20191205235704.31385-1-alext9@gmail.com","threadId":"52391","inReplyTo":"20191205225322.5529-1-alext9@gmail.com","subject":"[PATCH v2 0/2] rebase: fix bug in --fork-point","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T23:57:02Z","receivedAt":"2019-12-05T23:58:02Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"Reorder commits to include the fix and test in the same commit.\n\nAlex Torok (2):\n  rebase: refactor dwim_ref_or_die from merge-base.c\n  rebase: find --fork-point with full ref\n\n builtin/merge-base.c         |  9 +--------\n builtin/rebase.c             |  4 +++-\n refs.c                       | 14 ++++++++++++++\n refs.h                       |  1 +\n t/t3431-rebase-fork-point.sh |  1 +\n 5 files changed, 20 insertions(+), 9 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"387594","messageId":"20191205235704.31385-2-alext9@gmail.com","threadId":"52391","inReplyTo":"20191205235704.31385-1-alext9@gmail.com","subject":"[PATCH v2 1/2] rebase: refactor dwim_ref_or_die from merge-base.c","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T23:57:03Z","receivedAt":"2019-12-05T23:58:04Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"Pull logic for getting the full dwim ref name from a passed in ref name\nout of the handle_fork_point function of merge-base.c. This will allow\ngit-rebase --fork-point to use the same method for getting the full ref\nname before calling get_fork_point.\n\nI saw other *_or_die methods in other places and using that pattern\nseemed sane here.\n\nI'm not 100% sure about the name or signature of dwim_ref_or_die. I feel\nlike it should be named something like dwim_ref_name_or_die,\nunique_dwim_ref_or_die, or should be including the object_id argument\neven though it isn't used by the calling merge_base code, and won't be\nused in rebase.\n\nThis is my first patch submission for git, and I'd appreciate some\nfeedback on naming/style wrt to this.\n\nSigned-off-by: Alex Torok <alext9@gmail.com>\n---\n builtin/merge-base.c |  9 +--------\n refs.c               | 14 ++++++++++++++\n refs.h               |  1 +\n 3 files changed, 16 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex e3f8da13b6..edd16f9fcd 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -118,14 +118,7 @@ static int handle_fork_point(int argc, const char **argv)\n \tstruct commit *derived, *fork_point;\n \tconst char *commitname;\n \n-\tswitch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n-\tcase 0:\n-\t\tdie(\"No such ref: '%s'\", argv[0]);\n-\tcase 1:\n-\t\tbreak; /* good */\n-\tdefault:\n-\t\tdie(\"Ambiguous refname: '%s'\", argv[0]);\n-\t}\n+\tdwim_ref_or_die(argv[0], strlen(argv[0]), &refname);\n \n \tcommitname = (argc == 2) ? argv[1] : \"HEAD\";\n \tif (get_oid(commitname, &oid))\ndiff --git a/refs.c b/refs.c\nindex 1ab0bb54d3..3b778f2df9 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -639,6 +639,20 @@ int dwim_ref(const char *str, int len, struct object_id *oid, char **ref)\n \treturn repo_dwim_ref(the_repository, str, len, oid, ref);\n }\n \n+void dwim_ref_or_die(const char *str, int len, char **ref)\n+{\n+\tstruct object_id discard;\n+\tswitch (dwim_ref(str, len, &discard, ref)) {\n+\tcase 0:\n+\t\tdie(\"No such ref: '%s'\", str);\n+\tcase 1:\n+\t\tbreak; /* good */\n+\tdefault:\n+\t\tdie(\"Ambiguous refname: '%s'\", str);\n+\t}\n+}\n+\n+\n int expand_ref(struct repository *repo, const char *str, int len,\n \t       struct object_id *oid, char **ref)\n {\ndiff --git a/refs.h b/refs.h\nindex 730d05ad91..a961662382 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -154,6 +154,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_\n int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);\n int dwim_log(const char *str, int len, struct object_id *oid, char **ref);\n \n+void dwim_ref_or_die(const char *str, int len, char **ref);\n /*\n  * A ref_transaction represents a collection of reference updates that\n  * should succeed or fail together.\n-- \n2.17.1\n\n"},{"id":"387595","messageId":"20191205235704.31385-3-alext9@gmail.com","threadId":"52391","inReplyTo":"20191205235704.31385-1-alext9@gmail.com","subject":"[PATCH v2 2/2] rebase: find --fork-point with full ref","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-05T23:57:04Z","receivedAt":"2019-12-05T23:58:05Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"rebase --fork-point needs to look up the full ref name before calling\nget_fork_point in the same manner that merge-base --fork-point does.\n\nSigned-off-by: Alex Torok <alext9@gmail.com>\n---\n builtin/rebase.c             | 4 +++-\n t/t3431-rebase-fork-point.sh | 1 +\n 2 files changed, 4 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e755087b0f..821994f676 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tstruct commit *head =\n \t\t\tlookup_commit_reference(the_repository,\n \t\t\t\t\t\t&options.orig_head);\n+\t\tchar * full_name;\n+\t\tdwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);\n \t\toptions.restrict_revision =\n-\t\t\tget_fork_point(options.upstream_name, head);\n+\t\t\tget_fork_point(full_name, head);\n \t}\n \n \tif (repo_read_index(the_repository) < 0)\ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 78851b9a2a..6ecdae918e 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -49,6 +49,7 @@ test_rebase 'G F C D B A' --no-fork-point --onto D\n test_rebase 'G F C B A' --no-fork-point --keep-base\n test_rebase 'G F E D B A' --fork-point refs/heads/master\n test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n+test_rebase 'G F D B A' --fork-point --onto D master\n test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n test_rebase 'G F C E D B A' refs/heads/master\n test_rebase 'G F C D B A' --onto D refs/heads/master\n-- \n2.17.1\n\n"},{"id":"387596","messageId":"20191206012335.GA16183@generichostname","threadId":"52391","inReplyTo":"20191205235704.31385-2-alext9@gmail.com","subject":"Re: [PATCH v2 1/2] rebase: refactor dwim_ref_or_die from merge-base.c","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-06T01:23:35Z","receivedAt":"2019-12-06T01:23:15Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Alex,\n\nThanks for the contribution.\n\nOn Thu, Dec 05, 2019 at 06:57:03PM -0500, Alex Torok wrote:\n> Pull logic for getting the full dwim ref name from a passed in ref name\n> out of the handle_fork_point function of merge-base.c. This will allow\n> git-rebase --fork-point to use the same method for getting the full ref\n> name before calling get_fork_point.\n> \n\nThe remaining paragraphs shouldn't end up going into the commit message\nsince they're more like \"notes to reviewers\". You should place these\nnotes under the `---` below so that they won't end up in the final\ncommit message.\n\n> I saw other *_or_die methods in other places and using that pattern\n> seemed sane here.\n> \n> I'm not 100% sure about the name or signature of dwim_ref_or_die. I feel\n> like it should be named something like dwim_ref_name_or_die,\n> unique_dwim_ref_or_die, or should be including the object_id argument\n> even though it isn't used by the calling merge_base code, and won't be\n> used in rebase.\n> \n> This is my first patch submission for git, and I'd appreciate some\n> feedback on naming/style wrt to this.\n> \n> Signed-off-by: Alex Torok <alext9@gmail.com>\n> ---\n>  builtin/merge-base.c |  9 +--------\n>  refs.c               | 14 ++++++++++++++\n>  refs.h               |  1 +\n>  3 files changed, 16 insertions(+), 8 deletions(-)\n> \n> diff --git a/builtin/merge-base.c b/builtin/merge-base.c\n> index e3f8da13b6..edd16f9fcd 100644\n> --- a/builtin/merge-base.c\n> +++ b/builtin/merge-base.c\n> @@ -118,14 +118,7 @@ static int handle_fork_point(int argc, const char **argv)\n>  \tstruct commit *derived, *fork_point;\n>  \tconst char *commitname;\n>  \n> -\tswitch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n> -\tcase 0:\n> -\t\tdie(\"No such ref: '%s'\", argv[0]);\n> -\tcase 1:\n> -\t\tbreak; /* good */\n> -\tdefault:\n> -\t\tdie(\"Ambiguous refname: '%s'\", argv[0]);\n> -\t}\n> +\tdwim_ref_or_die(argv[0], strlen(argv[0]), &refname);\n>  \n>  \tcommitname = (argc == 2) ? argv[1] : \"HEAD\";\n>  \tif (get_oid(commitname, &oid))\n> diff --git a/refs.c b/refs.c\n> index 1ab0bb54d3..3b778f2df9 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -639,6 +639,20 @@ int dwim_ref(const char *str, int len, struct object_id *oid, char **ref)\n>  \treturn repo_dwim_ref(the_repository, str, len, oid, ref);\n>  }\n>  \n> +void dwim_ref_or_die(const char *str, int len, char **ref)\n> +{\n> +\tstruct object_id discard;\n> +\tswitch (dwim_ref(str, len, &discard, ref)) {\n> +\tcase 0:\n> +\t\tdie(\"No such ref: '%s'\", str);\n\nI see that this code is copied so I'm fine with leaving it as is. But if\nyou need to do any rerolls, it would be nice to, as a prepatory step,\nintroduce a patch which 1. marks these strings for translation and 2.\nlowercases the first letter of the sentence which is the convention for\nmessages that are seen by the end-user.\n\nPerhaps:\n\n\tdie(_(\"no such ref: '%s'\"), str);\n\n> +\tcase 1:\n> +\t\tbreak; /* good */\n> +\tdefault:\n> +\t\tdie(\"Ambiguous refname: '%s'\", str);\n\nSame comment here.\n\nThanks,\n\nDenton\n\n> +\t}\n> +}\n> +\n> +\n>  int expand_ref(struct repository *repo, const char *str, int len,\n>  \t       struct object_id *oid, char **ref)\n>  {\n> diff --git a/refs.h b/refs.h\n> index 730d05ad91..a961662382 100644\n> --- a/refs.h\n> +++ b/refs.h\n> @@ -154,6 +154,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_\n>  int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);\n>  int dwim_log(const char *str, int len, struct object_id *oid, char **ref);\n>  \n> +void dwim_ref_or_die(const char *str, int len, char **ref);\n>  /*\n>   * A ref_transaction represents a collection of reference updates that\n>   * should succeed or fail together.\n> -- \n> 2.17.1\n> \n"},{"id":"387597","messageId":"20191206014828.GB16183@generichostname","threadId":"52391","inReplyTo":"20191205235704.31385-3-alext9@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: find --fork-point with full ref","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-06T01:48:28Z","receivedAt":"2019-12-06T01:48:07Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Alex,\n\nOn Thu, Dec 05, 2019 at 06:57:04PM -0500, Alex Torok wrote:\n> rebase --fork-point needs to look up the full ref name before calling\n> get_fork_point in the same manner that merge-base --fork-point does.\n> \n> Signed-off-by: Alex Torok <alext9@gmail.com>\n> ---\n>  builtin/rebase.c             | 4 +++-\n>  t/t3431-rebase-fork-point.sh | 1 +\n>  2 files changed, 4 insertions(+), 1 deletion(-)\n> \n> diff --git a/builtin/rebase.c b/builtin/rebase.c\n> index e755087b0f..821994f676 100644\n> --- a/builtin/rebase.c\n> +++ b/builtin/rebase.c\n> @@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>  \t\tstruct commit *head =\n>  \t\t\tlookup_commit_reference(the_repository,\n>  \t\t\t\t\t\t&options.orig_head);\n> +\t\tchar * full_name;\n\nnit: * should be attached to the variable name.\n\n> +\t\tdwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);\n\nAlso, thinking about this more, would it be possible to put the dwim_ref\nlogic into get_fork_point() directly? There are currently only these two\ncallers so I suspect it should be fine and it'll result in cleaner\nlogic.\n\nWe could also squash it down into one patch.\n\n>  \t\toptions.restrict_revision =\n> -\t\t\tget_fork_point(options.upstream_name, head);\n> +\t\t\tget_fork_point(full_name, head);\n>  \t}\n>  \n>  \tif (repo_read_index(the_repository) < 0)\n> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\n> index 78851b9a2a..6ecdae918e 100755\n> --- a/t/t3431-rebase-fork-point.sh\n> +++ b/t/t3431-rebase-fork-point.sh\n> @@ -49,6 +49,7 @@ test_rebase 'G F C D B A' --no-fork-point --onto D\n>  test_rebase 'G F C B A' --no-fork-point --keep-base\n>  test_rebase 'G F E D B A' --fork-point refs/heads/master\n>  test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n> +test_rebase 'G F D B A' --fork-point --onto D master\n\nIt's not obvious why this was failing in the first place. Perhaps we\ncould document it better in the commit message?\n\nMaybe something like:\n\n\tWe used to pass in the upstream_name directly into the\n\tget_fork_point() machinery. However, get_fork_point() was\n\texpecting a fully qualified ref name even though most users use\n\tthe short name for branches. This resulted in `--fork-point` not\n\tworking as expected since, without the full ref name, the reflog\n\tlookup would fail and it would behave as if we weren't passing\n\tin `--fork-point` at all.\n\nAlso, I'm not why this test case in particular that was duplicated (and\nnot the one above) given that the first three `--fork-point` test cases\nfail without the change to rebase. Perhaps we want to duplicate all\n\"refs/heads/master\" tests with a corresponding \"master\" test?\n\nThanks,\n\nDenton\n\n>  test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n>  test_rebase 'G F C E D B A' refs/heads/master\n>  test_rebase 'G F C D B A' --onto D refs/heads/master\n> -- \n> 2.17.1\n> \n"},{"id":"387600","messageId":"e898ad43-5649-8f4c-7c93-fab09197fc92@gmail.com","threadId":"52391","inReplyTo":"20191206014828.GB16183@generichostname","subject":"Re: [PATCH v2 2/2] rebase: find --fork-point with full ref","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T10:52:31Z","receivedAt":"2019-12-06T10:52:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"Hi Alex\n\nThanks for working on this\n\nOn 06/12/2019 01:48, Denton Liu wrote:\n> Hi Alex,\n> \n> On Thu, Dec 05, 2019 at 06:57:04PM -0500, Alex Torok wrote:\n>> rebase --fork-point needs to look up the full ref name before calling\n>> get_fork_point in the same manner that merge-base --fork-point does.\n>>\n>> Signed-off-by: Alex Torok <alext9@gmail.com>\n>> ---\n>>   builtin/rebase.c             | 4 +++-\n>>   t/t3431-rebase-fork-point.sh | 1 +\n>>   2 files changed, 4 insertions(+), 1 deletion(-)\n>>\n>> diff --git a/builtin/rebase.c b/builtin/rebase.c\n>> index e755087b0f..821994f676 100644\n>> --- a/builtin/rebase.c\n>> +++ b/builtin/rebase.c\n>> @@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n>>   \t\tstruct commit *head =\n>>   \t\t\tlookup_commit_reference(the_repository,\n>>   \t\t\t\t\t\t&options.orig_head);\n>> +\t\tchar * full_name;\n> \n> nit: * should be attached to the variable name.\n\nI think you also need to free it once you've called get_fork_point() as \nwell.\n\n> \n>> +\t\tdwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);\n> \n> Also, thinking about this more, would it be possible to put the dwim_ref\n> logic into get_fork_point() directly? There are currently only these two\n> callers so I suspect it should be fine and it'll result in cleaner\n> logic.\n\nIf you do that then it would be better to use error() rather than die() \nin get_fork_point() and return an error to the caller as we try to avoid \nadding code to libgit that dies. This lets the caller handle any cleanup \nthat they need to before exiting.\n\nBest Wishes\n\nPhillip\n\n> \n> We could also squash it down into one patch.\n> \n>>   \t\toptions.restrict_revision =\n>> -\t\t\tget_fork_point(options.upstream_name, head);\n>> +\t\t\tget_fork_point(full_name, head);\n>>   \t}\n>>   \n>>   \tif (repo_read_index(the_repository) < 0)\n>> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\n>> index 78851b9a2a..6ecdae918e 100755\n>> --- a/t/t3431-rebase-fork-point.sh\n>> +++ b/t/t3431-rebase-fork-point.sh\n>> @@ -49,6 +49,7 @@ test_rebase 'G F C D B A' --no-fork-point --onto D\n>>   test_rebase 'G F C B A' --no-fork-point --keep-base\n>>   test_rebase 'G F E D B A' --fork-point refs/heads/master\n>>   test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n>> +test_rebase 'G F D B A' --fork-point --onto D master\n> \n> It's not obvious why this was failing in the first place. Perhaps we\n> could document it better in the commit message?\n> \n> Maybe something like:\n> \n> \tWe used to pass in the upstream_name directly into the\n> \tget_fork_point() machinery. However, get_fork_point() was\n> \texpecting a fully qualified ref name even though most users use\n> \tthe short name for branches. This resulted in `--fork-point` not\n> \tworking as expected since, without the full ref name, the reflog\n> \tlookup would fail and it would behave as if we weren't passing\n> \tin `--fork-point` at all.\n> \n> Also, I'm not why this test case in particular that was duplicated (and\n> not the one above) given that the first three `--fork-point` test cases\n> fail without the change to rebase. Perhaps we want to duplicate all\n> \"refs/heads/master\" tests with a corresponding \"master\" test?\n> \n> Thanks,\n> \n> Denton\n> \n>>   test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n>>   test_rebase 'G F C E D B A' refs/heads/master\n>>   test_rebase 'G F C D B A' --onto D refs/heads/master\n>> -- \n>> 2.17.1\n>>\n"},{"id":"387611","messageId":"CANmPhj1vSQYmwix7FMMBAksSTUJpz0izJyehmOhThkUfyUBy+w@mail.gmail.com","threadId":"52391","inReplyTo":"20191206012335.GA16183@generichostname","subject":"Re: [PATCH v2 1/2] rebase: refactor dwim_ref_or_die from merge-base.c","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-06T13:13:15Z","receivedAt":"2019-12-06T13:13:29Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"On Thu, Dec 5, 2019 at 8:23 PM Denton Liu <liu.denton@gmail.com> wrote:\n> The remaining paragraphs shouldn't end up going into the commit message\n> since they're more like \"notes to reviewers\". You should place these\n> notes under the `---` below so that they won't end up in the final\n> commit message.\n\nThank you for the info! I'll keep that in mind for my next patch submission.\n\n> > +void dwim_ref_or_die(const char *str, int len, char **ref)\n> > +{\n> > +     struct object_id discard;\n> > +     switch (dwim_ref(str, len, &discard, ref)) {\n> > +     case 0:\n> > +             die(\"No such ref: '%s'\", str);\n>\n> I see that this code is copied so I'm fine with leaving it as is. But if\n> you need to do any rerolls, it would be nice to, as a prepatory step,\n> introduce a patch which 1. marks these strings for translation and 2.\n> lowercases the first letter of the sentence which is the convention for\n> messages that are seen by the end-user.\n>\n> Perhaps:\n>\n>         die(_(\"no such ref: '%s'\"), str);\n>\n\nSounds good! I'll add a patch to the end of this series that addresses\nthese strings not being translated.\n\nThank you for your feedback.\n~Alex\n\n> > +     case 1:\n> > +             break; /* good */\n> > +     default:\n> > +             die(\"Ambiguous refname: '%s'\", str);\n>\n> Same comment here.\n>\n> Thanks,\n>\n> Denton\n>\n> > +     }\n> > +}\n> > +\n> > +\n> >  int expand_ref(struct repository *repo, const char *str, int len,\n> >              struct object_id *oid, char **ref)\n> >  {\n> > diff --git a/refs.h b/refs.h\n> > index 730d05ad91..a961662382 100644\n> > --- a/refs.h\n> > +++ b/refs.h\n> > @@ -154,6 +154,7 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_\n> >  int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);\n> >  int dwim_log(const char *str, int len, struct object_id *oid, char **ref);\n> >\n> > +void dwim_ref_or_die(const char *str, int len, char **ref);\n> >  /*\n> >   * A ref_transaction represents a collection of reference updates that\n> >   * should succeed or fail together.\n> > --\n> > 2.17.1\n> >\n"},{"id":"387613","messageId":"CANmPhj38UqZiePEPbPJBwUTOEJrfA6j3aP8KBHvAY6EA-J7Nsw@mail.gmail.com","threadId":"52391","inReplyTo":"e898ad43-5649-8f4c-7c93-fab09197fc92@gmail.com","subject":"Re: [PATCH v2 2/2] rebase: find --fork-point with full ref","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-06T13:46:29Z","receivedAt":"2019-12-06T13:46:44Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"Thank you for the feedback Denton & Phillip!\n\nOn Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> On 06/12/2019 01:48, Denton Liu wrote:\n> > nit: * should be attached to the variable name.\n>\n> I think you also need to free it once you've called get_fork_point() as\n> well.\n\nYup. Got it.\n\n> On 06/12/2019 01:48, Denton Liu wrote:\n> >\n> >> +            dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);\n> >\n> > Also, thinking about this more, would it be possible to put the dwim_ref\n> > logic into get_fork_point() directly? There are currently only these two\n> > callers so I suspect it should be fine and it'll result in cleaner\n> > logic.\n>\n> If you do that then it would be better to use error() rather than die()\n> in get_fork_point() and return an error to the caller as we try to avoid\n> adding code to libgit that dies. This lets the caller handle any cleanup\n> that they need to before exiting.\n\nWould the signature of get_fork_point change to be something like:\nint get_fork_point(const char *refname, struct commit *commit,\n   struct commit **fork_point, struct strbuf *err)\n\nIf not, could you point me to an example of some existing code\nthat does what you're talking about?\n\n\n> On 06/12/2019 01:48, Denton Liu wrote:\n> > It's not obvious why this was failing in the first place. Perhaps we\n> > could document it better in the commit message?\n> >\n> > Maybe something like:\n> >\n> >       We used to pass in the upstream_name directly into the\n> >       get_fork_point() machinery. However, get_fork_point() was\n> >       expecting a fully qualified ref name even though most users use\n> >       the short name for branches. This resulted in `--fork-point` not\n> >       working as expected since, without the full ref name, the reflog\n> >       lookup would fail and it would behave as if we weren't passing\n> >       in `--fork-point` at all.\n\nSounds good.\n\n> > Also, I'm not why this test case in particular that was duplicated (and\n> > not the one above) given that the first three `--fork-point` test cases\n> > fail without the change to rebase. Perhaps we want to duplicate all\n> > \"refs/heads/master\" tests with a corresponding \"master\" test?\n\nI only duplicated one so that there would only be one test case that\nwould fail if a regression around getting the fork point with a short\nref name was introduced.\n\nI just happened to pick that one because it was closest to the rebase\ncommand I was running when I found the bug :)\n\nI can include some of the above reasoning in the commit message.\nAlternatively:\n* I could duplicate all of tests\n* I could change all of the tests to use the short ref name\n\nI'm leaning towards just leaving one test (maybe with a comment?)\nfor the short ref name --fork-point so that there is more resolution\naround where a bug could be on test failure.\n\nLet me know what you think,\nAlex\n"},{"id":"387641","messageId":"20191206191147.GA16379@generichostname","threadId":"52391","inReplyTo":"CANmPhj38UqZiePEPbPJBwUTOEJrfA6j3aP8KBHvAY6EA-J7Nsw@mail.gmail.com","subject":"Re: [PATCH v2 2/2] rebase: find --fork-point with full refgg","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-06T19:11:47Z","receivedAt":"2019-12-06T19:11:27Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Alex,\n\nOn Fri, Dec 06, 2019 at 08:46:29AM -0500, Alex Torok wrote:\n> Thank you for the feedback Denton & Phillip!\n> \n> On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n> > On 06/12/2019 01:48, Denton Liu wrote:\n> > > nit: * should be attached to the variable name.\n> >\n> > I think you also need to free it once you've called get_fork_point() as\n> > well.\n> \n> Yup. Got it.\n> \n> > On 06/12/2019 01:48, Denton Liu wrote:\n> > >\n> > >> +            dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);\n> > >\n> > > Also, thinking about this more, would it be possible to put the dwim_ref\n> > > logic into get_fork_point() directly? There are currently only these two\n> > > callers so I suspect it should be fine and it'll result in cleaner\n> > > logic.\n> >\n> > If you do that then it would be better to use error() rather than die()\n> > in get_fork_point() and return an error to the caller as we try to avoid\n> > adding code to libgit that dies. This lets the caller handle any cleanup\n> > that they need to before exiting.\n> \n> Would the signature of get_fork_point change to be something like:\n> int get_fork_point(const char *refname, struct commit *commit,\n>    struct commit **fork_point, struct strbuf *err)\n\nI would drop the last parameter. If an error is detected, you could just\ndo\n\n\treturn error(_(\"oh no, something bad happened\"));\n\nEven though we try and avoid dying in the middle of libgit, we print\nerrors out very often so it should be fine here.\n\n> > > Also, I'm not why this test case in particular that was duplicated (and\n> > > not the one above) given that the first three `--fork-point` test cases\n> > > fail without the change to rebase. Perhaps we want to duplicate all\n> > > \"refs/heads/master\" tests with a corresponding \"master\" test?\n> \n> I only duplicated one so that there would only be one test case that\n> would fail if a regression around getting the fork point with a short\n> ref name was introduced.\n> \n> I just happened to pick that one because it was closest to the rebase\n> command I was running when I found the bug :)\n> \n> I can include some of the above reasoning in the commit message.\n> Alternatively:\n> * I could duplicate all of tests\n> * I could change all of the tests to use the short ref name\n> \n> I'm leaning towards just leaving one test (maybe with a comment?)\n> for the short ref name --fork-point so that there is more resolution\n> around where a bug could be on test failure.\n\nI would just duplicate all of the tests. When the tests are pretty cheap\nto run (as they are in this case), I tend to err on the side of adding\nmore tests since they might catch more odd edge-cases but, in this case,\nall of the fork point logic goes through one common block so the\nduplicate logic doesn't really buy us anything.\n\nI'm pretty impartial so I'll leave it up to you ;)\n\nThanks,\n\nDenton\n\n> \n> Let me know what you think,\n> Alex\n"},{"id":"387642","messageId":"117ce276-f37b-057c-849f-909852a6dd2c@gmail.com","threadId":"52391","inReplyTo":"20191206191147.GA16379@generichostname","subject":"Re: [PATCH v2 2/2] rebase: find --fork-point with full refgg","fromName":"Phillip Wood","fromEmail":"phillip.wood123@gmail.com","sentAt":"2019-12-06T19:35:32Z","receivedAt":"2019-12-06T19:35:38Z","isPatch":true,"sender":{"key":"phillip.wood@dunelm.org.uk","avatar":null},"body":"On 06/12/2019 19:11, Denton Liu wrote:\n> Hi Alex,\n> \n> On Fri, Dec 06, 2019 at 08:46:29AM -0500, Alex Torok wrote:\n>> Thank you for the feedback Denton & Phillip!\n>>\n>> On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:\n>>> On 06/12/2019 01:48, Denton Liu wrote:\n>>>> nit: * should be attached to the variable name.\n>>>\n>>> I think you also need to free it once you've called get_fork_point() as\n>>> well.\n>>\n>> Yup. Got it.\n>>\n>>> On 06/12/2019 01:48, Denton Liu wrote:\n>>>>\n>>>>> +            dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);\n>>>>\n>>>> Also, thinking about this more, would it be possible to put the dwim_ref\n>>>> logic into get_fork_point() directly? There are currently only these two\n>>>> callers so I suspect it should be fine and it'll result in cleaner\n>>>> logic.\n>>>\n>>> If you do that then it would be better to use error() rather than die()\n>>> in get_fork_point() and return an error to the caller as we try to avoid\n>>> adding code to libgit that dies. This lets the caller handle any cleanup\n>>> that they need to before exiting.\n>>\n>> Would the signature of get_fork_point change to be something like:\n>> int get_fork_point(const char *refname, struct commit *commit,\n>>     struct commit **fork_point, struct strbuf *err)\n> \n> I would drop the last parameter. If an error is detected, you could just\n> do\n> \n> \treturn error(_(\"oh no, something bad happened\"));\n> \n> Even though we try and avoid dying in the middle of libgit, we print\n> errors out very often so it should be fine here.\n\nYes that was what I was thinking of\n\nBest Wishes\n\nPhillip\n\n\n> \n>>>> Also, I'm not why this test case in particular that was duplicated (and\n>>>> not the one above) given that the first three `--fork-point` test cases\n>>>> fail without the change to rebase. Perhaps we want to duplicate all\n>>>> \"refs/heads/master\" tests with a corresponding \"master\" test?\n>>\n>> I only duplicated one so that there would only be one test case that\n>> would fail if a regression around getting the fork point with a short\n>> ref name was introduced.\n>>\n>> I just happened to pick that one because it was closest to the rebase\n>> command I was running when I found the bug :)\n>>\n>> I can include some of the above reasoning in the commit message.\n>> Alternatively:\n>> * I could duplicate all of tests\n>> * I could change all of the tests to use the short ref name\n>>\n>> I'm leaning towards just leaving one test (maybe with a comment?)\n>> for the short ref name --fork-point so that there is more resolution\n>> around where a bug could be on test failure.\n> \n> I would just duplicate all of the tests. When the tests are pretty cheap\n> to run (as they are in this case), I tend to err on the side of adding\n> more tests since they might catch more odd edge-cases but, in this case,\n> all of the fork point logic goes through one common block so the\n> duplicate logic doesn't really buy us anything.\n> \n> I'm pretty impartial so I'll leave it up to you ;)\n> \n> Thanks,\n> \n> Denton\n> \n>>\n>> Let me know what you think,\n>> Alex\n"},{"id":"387781","messageId":"20191209145312.3251-1-alext9@gmail.com","threadId":"52391","inReplyTo":"20191205235704.31385-1-alext9@gmail.com","subject":"[PATCH v3 0/1] rebase: fix --fork-point with short ref upstream","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-09T14:53:11Z","receivedAt":"2019-12-09T14:53:55Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"Move the dwim ref logic into get_fork_point() and return an error code\nso that callers of get_fork_point() know if something went wrong.\n\nAlex Torok (1):\n  rebase: fix --fork-point with short refname\n\n builtin/merge-base.c         | 14 +++-----------\n builtin/rebase.c             |  5 +++--\n commit.c                     | 19 +++++++++++++------\n commit.h                     |  2 +-\n refs.c                       | 14 ++++++++++++++\n refs.h                       |  2 ++\n t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++\n 7 files changed, 56 insertions(+), 20 deletions(-)\n\n-- \n2.17.1\n\n"},{"id":"387782","messageId":"20191209145312.3251-2-alext9@gmail.com","threadId":"52391","inReplyTo":"20191209145312.3251-1-alext9@gmail.com","subject":"[PATCH v3 1/1] rebase: fix --fork-point with short refname","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-09T14:53:12Z","receivedAt":"2019-12-09T14:53:56Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"We were directly passing in the user-provided upstream_name into\nget_fork_point(), but get_fork_point() was expecting a fully qualified\nref name. This resulted in `--fork-point` not working as expected.\nWithout the full ref name, get_fork_point could not find the fork point,\nand rebase behaved as if no `--fork-point` flag was passed in.\n\nPull logic for getting the full dwim ref name out of merge-base.c\nhandle_fork_point into get_fork_point. This allows both of the locations\nthat call get_fork_point to not need to handle doing the dwim ref lookup\nbefore trying to get the fork point.\n\nDuplicate all of the rebase tests with a short refname to ensure that\nthey work with short and long refnames.\n\nSigned-off-by: Alex Torok <alext9@gmail.com>\n---\n\nOne thing that I'm not super sure about is the error messaging that gets\nprinted when a short refname is given to rebase. In the ambigous refname\ntest that I added, the command output is:\n\nwarning: refname 'one' is ambiguous.\nerror: ambiguous refname: 'one'\nfatal: could not get fork point\n\nWhich seems a bit odd ot have a warning, error, fatal stacked on top of\neach other. From a user-facing perspective it feels a bit odd to me, but\nI'm not sure what the general rules for error messaging are in git.\n\n\n builtin/merge-base.c         | 14 +++-----------\n builtin/rebase.c             |  5 +++--\n commit.c                     | 19 +++++++++++++------\n commit.h                     |  2 +-\n refs.c                       | 14 ++++++++++++++\n refs.h                       |  2 ++\n t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++\n 7 files changed, 56 insertions(+), 20 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex e3f8da13b6..ecc4268d43 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -114,26 +114,18 @@ static int handle_is_ancestor(int argc, const char **argv)\n static int handle_fork_point(int argc, const char **argv)\n {\n \tstruct object_id oid;\n-\tchar *refname;\n \tstruct commit *derived, *fork_point;\n \tconst char *commitname;\n \n-\tswitch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n-\tcase 0:\n-\t\tdie(\"No such ref: '%s'\", argv[0]);\n-\tcase 1:\n-\t\tbreak; /* good */\n-\tdefault:\n-\t\tdie(\"Ambiguous refname: '%s'\", argv[0]);\n-\t}\n-\n \tcommitname = (argc == 2) ? argv[1] : \"HEAD\";\n \tif (get_oid(commitname, &oid))\n \t\tdie(\"Not a valid object name: '%s'\", commitname);\n \n \tderived = lookup_commit_reference(the_repository, &oid);\n \n-\tfork_point = get_fork_point(refname, derived);\n+\tfork_point = NULL;\n+\tif (get_fork_point(argv[0], derived, &fork_point) < 0)\n+\t\treturn 1;\n \n \tif (!fork_point)\n \t\treturn 1;\ndiff --git a/builtin/rebase.c b/builtin/rebase.c\nindex e755087b0f..782cf5a890 100644\n--- a/builtin/rebase.c\n+++ b/builtin/rebase.c\n@@ -1980,8 +1980,9 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)\n \t\tstruct commit *head =\n \t\t\tlookup_commit_reference(the_repository,\n \t\t\t\t\t\t&options.orig_head);\n-\t\toptions.restrict_revision =\n-\t\t\tget_fork_point(options.upstream_name, head);\n+\t\toptions.restrict_revision = NULL;\n+\t\tif (get_fork_point(options.upstream_name, head, &options.restrict_revision) < 0)\n+\t\t\tdie(_(\"could not get fork point\"));\n \t}\n \n \tif (repo_read_index(the_repository) < 0)\ndiff --git a/commit.c b/commit.c\nindex 434ec030d6..874bc0cdf1 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -920,19 +920,25 @@ static int collect_one_reflog_ent(struct object_id *ooid, struct object_id *noid\n \treturn 0;\n }\n \n-struct commit *get_fork_point(const char *refname, struct commit *commit)\n+int get_fork_point(const char *refname, struct commit *commit, struct commit **fork_point)\n {\n \tstruct object_id oid;\n \tstruct rev_collect revs;\n \tstruct commit_list *bases;\n \tint i;\n-\tstruct commit *ret = NULL;\n+\tchar *full_refname;\n+\n+\tif (dwim_unique_ref(refname, strlen(refname), &oid, &full_refname) < 0) {\n+\t\tfree(full_refname);\n+\t\treturn -1;\n+\t}\n+\n \n \tmemset(&revs, 0, sizeof(revs));\n \trevs.initial = 1;\n-\tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n+\tfor_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs);\n \n-\tif (!revs.nr && !get_oid(refname, &oid))\n+\tif (!revs.nr && !get_oid(full_refname, &oid))\n \t\tadd_one_commit(&oid, &revs);\n \n \tfor (i = 0; i < revs.nr; i++)\n@@ -954,11 +960,12 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n \tif (revs.nr <= i)\n \t\tgoto cleanup_return;\n \n-\tret = bases->item;\n+\t*fork_point = bases->item;\n \n cleanup_return:\n+\tfree(full_refname);\n \tfree_commit_list(bases);\n-\treturn ret;\n+\treturn 0;\n }\n \n static const char gpg_sig_header[] = \"gpgsig\";\ndiff --git a/commit.h b/commit.h\nindex 221cdaa34b..d79a8eab48 100644\n--- a/commit.h\n+++ b/commit.h\n@@ -240,7 +240,7 @@ int register_commit_graft(struct repository *r, struct commit_graft *, int);\n void prepare_commit_graft(struct repository *r);\n struct commit_graft *lookup_commit_graft(struct repository *r, const struct object_id *oid);\n \n-struct commit *get_fork_point(const char *refname, struct commit *commit);\n+int get_fork_point(const char *refname, struct commit *commit, struct commit **fork_point);\n \n /* largest positive number a signed 32-bit integer can contain */\n #define INFINITE_DEPTH 0x7fffffff\ndiff --git a/refs.c b/refs.c\nindex 1ab0bb54d3..853e45a6a3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -639,6 +639,20 @@ int dwim_ref(const char *str, int len, struct object_id *oid, char **ref)\n \treturn repo_dwim_ref(the_repository, str, len, oid, ref);\n }\n \n+int dwim_unique_ref(const char* str, int len, struct object_id *oid, char **ref)\n+{\n+\tstruct object_id discard;\n+\tswitch (dwim_ref(str, len, &discard, ref)) {\n+\tcase 0:\n+\t\treturn error(_(\"no such ref: '%s'\"), str);\n+\tcase 1:\n+\t\tbreak; /* good */\n+\tdefault:\n+\t\treturn error(_(\"ambiguous refname: '%s'\"), str);\n+\t}\n+\treturn 0;\n+}\n+\n int expand_ref(struct repository *repo, const char *str, int len,\n \t       struct object_id *oid, char **ref)\n {\ndiff --git a/refs.h b/refs.h\nindex 730d05ad91..7aa348052b 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -154,6 +154,8 @@ int repo_dwim_log(struct repository *r, const char *str, int len, struct object_\n int dwim_ref(const char *str, int len, struct object_id *oid, char **ref);\n int dwim_log(const char *str, int len, struct object_id *oid, char **ref);\n \n+int dwim_unique_ref(const char* str, int len, struct object_id *oid, char **ref);\n+\n /*\n  * A ref_transaction represents a collection of reference updates that\n  * should succeed or fail together.\ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 78851b9a2a..5b09aecd13 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base\n test_rebase 'G F C E D B A' --no-fork-point\n test_rebase 'G F C D B A' --no-fork-point --onto D\n test_rebase 'G F C B A' --no-fork-point --keep-base\n+\n test_rebase 'G F E D B A' --fork-point refs/heads/master\n+test_rebase 'G F E D B A' --fork-point master\n+\n test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n+test_rebase 'G F D B A' --fork-point --onto D master\n+\n test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n+test_rebase 'G F B A' --fork-point --keep-base master\n+\n test_rebase 'G F C E D B A' refs/heads/master\n+test_rebase 'G F C E D B A' master\n+\n test_rebase 'G F C D B A' --onto D refs/heads/master\n+test_rebase 'G F C D B A' --onto D master\n+\n test_rebase 'G F C B A' --keep-base refs/heads/master\n+test_rebase 'G F C B A' --keep-base master\n+\n+test_expect_success \"git rebase --fork-point with ambigous refname\" \"\n+\tgit checkout master &&\n+\tgit checkout -b one &&\n+\tgit checkout side &&\n+\tgit tag one &&\n+\ttest_must_fail git rebase --fork-point --onto D one\n+\"\n \n test_done\n-- \n2.17.1\n\n"},{"id":"387815","messageId":"xmqq36dtwcvw.fsf@gitster-ct.c.googlers.com","threadId":"52391","inReplyTo":"20191209145312.3251-2-alext9@gmail.com","subject":"Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-12-09T18:51:47Z","receivedAt":"2019-12-09T18:51:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alex Torok <alext9@gmail.com> writes:\n\n> We were directly passing in the user-provided upstream_name into\n> get_fork_point(), but get_fork_point() was expecting a fully qualified\n> ref name. This resulted in `--fork-point` not working as expected.\n> Without the full ref name, get_fork_point could not find the fork point,\n> and rebase behaved as if no `--fork-point` flag was passed in.\n>\n> Pull logic for getting the full dwim ref name out of merge-base.c\n> handle_fork_point into get_fork_point. This allows both of the locations\n\n\"handle_fork_point() into get_fork_point()\" to match the second line\nof the first paragraph, perhaps (similarly for \"get_fork_point()\"\non the next line)?\n\n> that call get_fork_point to not need to handle doing the dwim ref lookup\n> before trying to get the fork point.\n>\n> Duplicate all of the rebase tests with a short refname to ensure that\n> they work with short and long refnames.\n>\n> Signed-off-by: Alex Torok <alext9@gmail.com>\n> ---\n>\n> One thing that I'm not super sure about is the error messaging that gets\n> printed when a short refname is given to rebase. In the ambigous refname\n> test that I added, the command output is:\n>\n> warning: refname 'one' is ambiguous.\n> error: ambiguous refname: 'one'\n> fatal: could not get fork point\n>\n> Which seems a bit odd ot have a warning, error, fatal stacked on top of\n> each other.\n\nYes, it does look strange.\n\nI think that the new \"dwim_unique_ref()\" helper is misdesigned as a\ngeneric helper function.  It makes it hard for the callers to handle\nerrors on their own for this function to return error().\n\nA helper at the right level for this kind of thing would have been a\nfunction that does DWIM ref and returns the number of candidate refs\nthe given short name would expand to, and do all that silently.  The\ncaller would then react to the returned number and say \"no such\" if\nthere is no candidate, or \"ambigous\" if there are more than one.\n\nWhich is exactly what dwim_ref() is.  In other words, dwim_unique_ref()\nhelper, unless it is useful by multiple callers who want it to die()\nitself, is not all that useful as an abstraction.\n\nIf I were doing this, probably I would not add dwim_unique_ref() at\nall, and then I'd invent an extra variant of get_fork_point(), that\nthe caller can choose to make it die or just silently return an error.\n\nThis caller would want it to die() without giving control back to\nit, but there may be some other callers that would want a finer\ncontrol.\n\nOn the other hand, if the other caller(s) all want get_fork_point()\nto die, then that would also be fine.  A quick glance tells me that\nthe only other caller is in builtin/rebase.c and does this:\n\n\tif (fork_point > 0) {\n\t\tstruct commit *head =\n\t\t\tlookup_commit_reference(the_repository,\n\t\t\t\t\t\t&options.orig_head);\n\t\toptions.restrict_revision =\n\t\t\tget_fork_point(options.upstream_name, head);\n\t}\n\nand there are other calls to die(_(\"...\")) in the caller everywhere,\nso I'd say you are over-engineering a simple bugfix.\n\nWouldn't it be sufficient for this fix to be more like this?\n\n-- >8 --\n\nSubject: rebase: --fork-point regression fix\n\n\"git rebase --fork-point master\" used to work OK, as it internally\ncalled \"git merge-base --fork-point\" that knew how to handle short\nrefname and dwim it to the full refname before calling the\nunderlying get_fork_point() function.\n\nThis is no longer true after the command was rewritten in C, as its\ninternall call made directly to get_fork_point() does not dwim a\nshort ref.\n\nMove the \"dwim the refname argument to the full refname\" logic that\nis used in \"git merge-base\" to the underlying get_fork_point()\nfunction, so that the other caller of the function in the\nimplementation of \"git rebase\" behaves the same way to fix this\nregression.\n\n---\n builtin/merge-base.c         | 12 +-----------\n commit.c                     | 15 +++++++++++++--\n t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++\n 3 files changed, 34 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex e3f8da13b6..6719ac198d 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv)\n static int handle_fork_point(int argc, const char **argv)\n {\n \tstruct object_id oid;\n-\tchar *refname;\n \tstruct commit *derived, *fork_point;\n \tconst char *commitname;\n \n-\tswitch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n-\tcase 0:\n-\t\tdie(\"No such ref: '%s'\", argv[0]);\n-\tcase 1:\n-\t\tbreak; /* good */\n-\tdefault:\n-\t\tdie(\"Ambiguous refname: '%s'\", argv[0]);\n-\t}\n-\n \tcommitname = (argc == 2) ? argv[1] : \"HEAD\";\n \tif (get_oid(commitname, &oid))\n \t\tdie(\"Not a valid object name: '%s'\", commitname);\n \n \tderived = lookup_commit_reference(the_repository, &oid);\n \n-\tfork_point = get_fork_point(refname, derived);\n+\tfork_point = get_fork_point(argv[0], derived);\n \n \tif (!fork_point)\n \t\treturn 1;\ndiff --git a/commit.c b/commit.c\nindex 40890ae7ce..016f14fe95 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n \tstruct commit_list *bases;\n \tint i;\n \tstruct commit *ret = NULL;\n+\tchar *full_refname;\n+\n+\tswitch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) {\n+\tcase 0:\n+\t\tdie(\"No such ref: '%s'\", refname);\n+\tcase 1:\n+\t\tbreak; /* good */\n+\tdefault:\n+\t\tdie(\"Ambiguous refname: '%s'\", refname);\n+\t}\n \n \tmemset(&revs, 0, sizeof(revs));\n \trevs.initial = 1;\n-\tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n+\tfor_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs);\n \n-\tif (!revs.nr && !get_oid(refname, &oid))\n+\tif (!revs.nr)\n \t\tadd_one_commit(&oid, &revs);\n \n \tfor (i = 0; i < revs.nr; i++)\n@@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n \n cleanup_return:\n \tfree_commit_list(bases);\n+\tfree(full_refname);\n \treturn ret;\n }\n \ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 78851b9a2a..5b09aecd13 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base\n test_rebase 'G F C E D B A' --no-fork-point\n test_rebase 'G F C D B A' --no-fork-point --onto D\n test_rebase 'G F C B A' --no-fork-point --keep-base\n+\n test_rebase 'G F E D B A' --fork-point refs/heads/master\n+test_rebase 'G F E D B A' --fork-point master\n+\n test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n+test_rebase 'G F D B A' --fork-point --onto D master\n+\n test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n+test_rebase 'G F B A' --fork-point --keep-base master\n+\n test_rebase 'G F C E D B A' refs/heads/master\n+test_rebase 'G F C E D B A' master\n+\n test_rebase 'G F C D B A' --onto D refs/heads/master\n+test_rebase 'G F C D B A' --onto D master\n+\n test_rebase 'G F C B A' --keep-base refs/heads/master\n+test_rebase 'G F C B A' --keep-base master\n+\n+test_expect_success \"git rebase --fork-point with ambigous refname\" \"\n+\tgit checkout master &&\n+\tgit checkout -b one &&\n+\tgit checkout side &&\n+\tgit tag one &&\n+\ttest_must_fail git rebase --fork-point --onto D one\n+\"\n \n test_done\n"},{"id":"387927","messageId":"CANmPhj26C2ow-4kZZ5f5USPVE7aN_F7Y81sJK_6iWxBUwc0cmw@mail.gmail.com","threadId":"52391","inReplyTo":"xmqq36dtwcvw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname","fromName":"Alex Torok","fromEmail":"alext9@gmail.com","sentAt":"2019-12-11T01:21:06Z","receivedAt":"2019-12-11T01:21:22Z","isPatch":true,"sender":{"key":"alext9@gmail.com","avatar":null},"body":"On Mon, Dec 9, 2019 at 1:51 PM Junio C Hamano <gitster@pobox.com> wrote:\n> and there are other calls to die(_(\"...\")) in the caller everywhere,\n> so I'd say you are over-engineering a simple bugfix.\n>\n> Wouldn't it be sufficient for this fix to be more like this?\n\nThis is essentially what I had done in the v2 version of my patch\nwith the only \"big\" difference being pulling out the dwim_ref() switching\nand dying logic into a dwim_ref_or_die() function.\n\nLet me know if you want me to adjust my patch at all (my v2 patch is\nmissing a call to free()...).\n\nIf the patch that you replied here with is sufficient for how you would fix\nit, I'm fine with you just using that.\n\n> -- >8 --\n>\n> Subject: rebase: --fork-point regression fix\n>\n> \"git rebase --fork-point master\" used to work OK, as it internally\n> called \"git merge-base --fork-point\" that knew how to handle short\n> refname and dwim it to the full refname before calling the\n> underlying get_fork_point() function.\n>\n> This is no longer true after the command was rewritten in C, as its\n> internall call made directly to get_fork_point() does not dwim a\n> short ref.\n>\n> Move the \"dwim the refname argument to the full refname\" logic that\n> is used in \"git merge-base\" to the underlying get_fork_point()\n> function, so that the other caller of the function in the\n> implementation of \"git rebase\" behaves the same way to fix this\n> regression.\n>\n> ---\n>  builtin/merge-base.c         | 12 +-----------\n>  commit.c                     | 15 +++++++++++++--\n>  t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++\n>  3 files changed, 34 insertions(+), 13 deletions(-)\n>\n> diff --git a/builtin/merge-base.c b/builtin/merge-base.c\n> index e3f8da13b6..6719ac198d 100644\n> --- a/builtin/merge-base.c\n> +++ b/builtin/merge-base.c\n> @@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv)\n>  static int handle_fork_point(int argc, const char **argv)\n>  {\n>         struct object_id oid;\n> -       char *refname;\n>         struct commit *derived, *fork_point;\n>         const char *commitname;\n>\n> -       switch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n> -       case 0:\n> -               die(\"No such ref: '%s'\", argv[0]);\n> -       case 1:\n> -               break; /* good */\n> -       default:\n> -               die(\"Ambiguous refname: '%s'\", argv[0]);\n> -       }\n> -\n>         commitname = (argc == 2) ? argv[1] : \"HEAD\";\n>         if (get_oid(commitname, &oid))\n>                 die(\"Not a valid object name: '%s'\", commitname);\n>\n>         derived = lookup_commit_reference(the_repository, &oid);\n>\n> -       fork_point = get_fork_point(refname, derived);\n> +       fork_point = get_fork_point(argv[0], derived);\n>\n>         if (!fork_point)\n>                 return 1;\n> diff --git a/commit.c b/commit.c\n> index 40890ae7ce..016f14fe95 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n>         struct commit_list *bases;\n>         int i;\n>         struct commit *ret = NULL;\n> +       char *full_refname;\n> +\n> +       switch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) {\n> +       case 0:\n> +               die(\"No such ref: '%s'\", refname);\n> +       case 1:\n> +               break; /* good */\n> +       default:\n> +               die(\"Ambiguous refname: '%s'\", refname);\n> +       }\n>\n>         memset(&revs, 0, sizeof(revs));\n>         revs.initial = 1;\n> -       for_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n> +       for_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs);\n>\n> -       if (!revs.nr && !get_oid(refname, &oid))\n> +       if (!revs.nr)\n>                 add_one_commit(&oid, &revs);\n>\n>         for (i = 0; i < revs.nr; i++)\n> @@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n>\n>  cleanup_return:\n>         free_commit_list(bases);\n> +       free(full_refname);\n>         return ret;\n>  }\n>\n> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\n> index 78851b9a2a..5b09aecd13 100755\n> --- a/t/t3431-rebase-fork-point.sh\n> +++ b/t/t3431-rebase-fork-point.sh\n> @@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base\n>  test_rebase 'G F C E D B A' --no-fork-point\n>  test_rebase 'G F C D B A' --no-fork-point --onto D\n>  test_rebase 'G F C B A' --no-fork-point --keep-base\n> +\n>  test_rebase 'G F E D B A' --fork-point refs/heads/master\n> +test_rebase 'G F E D B A' --fork-point master\n> +\n>  test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n> +test_rebase 'G F D B A' --fork-point --onto D master\n> +\n>  test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n> +test_rebase 'G F B A' --fork-point --keep-base master\n> +\n>  test_rebase 'G F C E D B A' refs/heads/master\n> +test_rebase 'G F C E D B A' master\n> +\n>  test_rebase 'G F C D B A' --onto D refs/heads/master\n> +test_rebase 'G F C D B A' --onto D master\n> +\n>  test_rebase 'G F C B A' --keep-base refs/heads/master\n> +test_rebase 'G F C B A' --keep-base master\n> +\n> +test_expect_success \"git rebase --fork-point with ambigous refname\" \"\n> +       git checkout master &&\n> +       git checkout -b one &&\n> +       git checkout side &&\n> +       git tag one &&\n> +       test_must_fail git rebase --fork-point --onto D one\n> +\"\n>\n>  test_done\n"},{"id":"387938","messageId":"20191211122136.GA46399@generichostname","threadId":"52391","inReplyTo":"xmqq36dtwcvw.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2019-12-11T12:21:36Z","receivedAt":"2019-12-11T12:20:43Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Alex,\n\nThere are a couple of small issues with the patch below. If you iterate\non it, here are some small suggestions:\n\nOn Mon, Dec 09, 2019 at 10:51:47AM -0800, Junio C Hamano wrote:\n> Subject: rebase: --fork-point regression fix\n> \n> \"git rebase --fork-point master\" used to work OK, as it internally\n> called \"git merge-base --fork-point\" that knew how to handle short\n> refname and dwim it to the full refname before calling the\n> underlying get_fork_point() function.\n> \n> This is no longer true after the command was rewritten in C, as its\n> internall call made directly to get_fork_point() does not dwim a\n> short ref.\n> \n> Move the \"dwim the refname argument to the full refname\" logic that\n> is used in \"git merge-base\" to the underlying get_fork_point()\n> function, so that the other caller of the function in the\n> implementation of \"git rebase\" behaves the same way to fix this\n> regression.\n> \n> ---\n>  builtin/merge-base.c         | 12 +-----------\n>  commit.c                     | 15 +++++++++++++--\n>  t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++\n>  3 files changed, 34 insertions(+), 13 deletions(-)\n> \n> diff --git a/builtin/merge-base.c b/builtin/merge-base.c\n> index e3f8da13b6..6719ac198d 100644\n> --- a/builtin/merge-base.c\n> +++ b/builtin/merge-base.c\n> @@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv)\n>  static int handle_fork_point(int argc, const char **argv)\n>  {\n>  \tstruct object_id oid;\n> -\tchar *refname;\n>  \tstruct commit *derived, *fork_point;\n>  \tconst char *commitname;\n>  \n> -\tswitch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n> -\tcase 0:\n> -\t\tdie(\"No such ref: '%s'\", argv[0]);\n> -\tcase 1:\n> -\t\tbreak; /* good */\n> -\tdefault:\n> -\t\tdie(\"Ambiguous refname: '%s'\", argv[0]);\n> -\t}\n> -\n\nIn the unabridged v3, you cleaned this up by marking it for translation\nand also lowercasing the first letter of the sentence. That was good.\n\nIf you iterate on this, could you create a preparatory patch before this\none that gives the same treatment to the other strings in merge-base? I\ncount 8 instances of die() being called in merge-base so I think that\nthe cleanup shouldn't be too arduous.\n\nIf we have a preparatory patch before this one, this patch will mostly\nbe pure code movement which would be nice.\n\n>  \tcommitname = (argc == 2) ? argv[1] : \"HEAD\";\n>  \tif (get_oid(commitname, &oid))\n>  \t\tdie(\"Not a valid object name: '%s'\", commitname);\n>  \n>  \tderived = lookup_commit_reference(the_repository, &oid);\n>  \n> -\tfork_point = get_fork_point(refname, derived);\n> +\tfork_point = get_fork_point(argv[0], derived);\n>  \n>  \tif (!fork_point)\n>  \t\treturn 1;\n> diff --git a/commit.c b/commit.c\n> index 40890ae7ce..016f14fe95 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n>  \tstruct commit_list *bases;\n>  \tint i;\n>  \tstruct commit *ret = NULL;\n> +\tchar *full_refname;\n> +\n> +\tswitch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) {\n> +\tcase 0:\n> +\t\tdie(\"No such ref: '%s'\", refname);\n> +\tcase 1:\n> +\t\tbreak; /* good */\n> +\tdefault:\n> +\t\tdie(\"Ambiguous refname: '%s'\", refname);\n> +\t}\n\nYeah, we should fix these strings in the next iteration.\n\n>  \n>  \tmemset(&revs, 0, sizeof(revs));\n>  \trevs.initial = 1;\n> -\tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n> +\tfor_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs);\n>  \n> -\tif (!revs.nr && !get_oid(refname, &oid))\n> +\tif (!revs.nr)\n>  \t\tadd_one_commit(&oid, &revs);\n>  \n>  \tfor (i = 0; i < revs.nr; i++)\n> @@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n>  \n>  cleanup_return:\n>  \tfree_commit_list(bases);\n> +\tfree(full_refname);\n>  \treturn ret;\n>  }\n>  \n> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\n> index 78851b9a2a..5b09aecd13 100755\n> --- a/t/t3431-rebase-fork-point.sh\n> +++ b/t/t3431-rebase-fork-point.sh\n> @@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base\n>  test_rebase 'G F C E D B A' --no-fork-point\n>  test_rebase 'G F C D B A' --no-fork-point --onto D\n>  test_rebase 'G F C B A' --no-fork-point --keep-base\n> +\n>  test_rebase 'G F E D B A' --fork-point refs/heads/master\n> +test_rebase 'G F E D B A' --fork-point master\n> +\n>  test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n> +test_rebase 'G F D B A' --fork-point --onto D master\n> +\n>  test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n> +test_rebase 'G F B A' --fork-point --keep-base master\n> +\n>  test_rebase 'G F C E D B A' refs/heads/master\n> +test_rebase 'G F C E D B A' master\n> +\n>  test_rebase 'G F C D B A' --onto D refs/heads/master\n> +test_rebase 'G F C D B A' --onto D master\n> +\n>  test_rebase 'G F C B A' --keep-base refs/heads/master\n> +test_rebase 'G F C B A' --keep-base master\n> +\n> +test_expect_success \"git rebase --fork-point with ambigous refname\" \"\n> +\tgit checkout master &&\n> +\tgit checkout -b one &&\n> +\tgit checkout side &&\n> +\tgit tag one &&\n> +\ttest_must_fail git rebase --fork-point --onto D one\n> +\"\n\nnit: use double-quotes instead of single-quotes to surround both the\ntest case name and the actual code itself.\n\nThanks,\n\nDenton\n\n>  \n>  test_done\n"},{"id":"387946","messageId":"CAPig+cQ-3Ds41hr91fRo_GvuFMTP7zNVJtaSqi-Yccq4Pk-8Qg@mail.gmail.com","threadId":"52391","inReplyTo":"20191211122136.GA46399@generichostname","subject":"Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-12-11T16:02:41Z","receivedAt":"2019-12-11T16:02:58Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Dec 11, 2019 at 7:20 AM Denton Liu <liu.denton@gmail.com> wrote:\n> > +test_expect_success \"git rebase --fork-point with ambigous refname\" \"\n> > +     git checkout master &&\n> > +     git checkout -b one &&\n> > +     git checkout side &&\n> > +     git tag one &&\n> > +     test_must_fail git rebase --fork-point --onto D one\n> > +\"\n>\n> nit: use double-quotes instead of single-quotes to surround both the\n> test case name and the actual code itself.\n\nDenton meant to say that you should single-quote the test title and\ntest body (not double-quote). In this particular test, the distinction\ndoesn't matter presently, though it could matter if the test is later\nchanged. In particular, if the test does any sort of variable\ninterpolation (such as $foo), then the single quotes ensure that the\ninterpolation will occur when the test is actually run (which is\nalmost always what is desired) rather than at the time the test is\ndefined (which is almost never wanted).\n"},{"id":"391552","messageId":"xmqq5zgdvvlq.fsf@gitster-ct.c.googlers.com","threadId":"52391","inReplyTo":"CAPig+cQ-3Ds41hr91fRo_GvuFMTP7zNVJtaSqi-Yccq4Pk-8Qg@mail.gmail.com","subject":"Re: [PATCH v4 1/1] rebase: --fork-point regression fix","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-11T18:15:29Z","receivedAt":"2020-02-11T18:15:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"git rebase --fork-point master\" used to work OK, as it internally\ncalled \"git merge-base --fork-point\" that knew how to handle short\nrefname and dwim it to the full refname before calling the\nunderlying get_fork_point() function.\n\nThis is no longer true after the command was rewritten in C, as its\ninternall call made directly to get_fork_point() does not dwim a\nshort ref.\n\nMove the \"dwim the refname argument to the full refname\" logic that\nis used in \"git merge-base\" to the underlying get_fork_point()\nfunction, so that the other caller of the function in the\nimplementation of \"git rebase\" behaves the same way to fix this\nregression.\n\nSigned-off-by: Alex Torok <alext9@gmail.com>\n[jc: revamped the fix and used Alex's tests]\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n\n * I was about to discard stalled topics and this caught my eyes.\n\n builtin/merge-base.c         | 12 +-----------\n commit.c                     | 15 +++++++++++++--\n t/t3431-rebase-fork-point.sh | 20 ++++++++++++++++++++\n 3 files changed, 34 insertions(+), 13 deletions(-)\n\ndiff --git a/builtin/merge-base.c b/builtin/merge-base.c\nindex e3f8da13b6..6719ac198d 100644\n--- a/builtin/merge-base.c\n+++ b/builtin/merge-base.c\n@@ -114,26 +114,16 @@ static int handle_is_ancestor(int argc, const char **argv)\n static int handle_fork_point(int argc, const char **argv)\n {\n \tstruct object_id oid;\n-\tchar *refname;\n \tstruct commit *derived, *fork_point;\n \tconst char *commitname;\n \n-\tswitch (dwim_ref(argv[0], strlen(argv[0]), &oid, &refname)) {\n-\tcase 0:\n-\t\tdie(\"No such ref: '%s'\", argv[0]);\n-\tcase 1:\n-\t\tbreak; /* good */\n-\tdefault:\n-\t\tdie(\"Ambiguous refname: '%s'\", argv[0]);\n-\t}\n-\n \tcommitname = (argc == 2) ? argv[1] : \"HEAD\";\n \tif (get_oid(commitname, &oid))\n \t\tdie(\"Not a valid object name: '%s'\", commitname);\n \n \tderived = lookup_commit_reference(the_repository, &oid);\n \n-\tfork_point = get_fork_point(refname, derived);\n+\tfork_point = get_fork_point(argv[0], derived);\n \n \tif (!fork_point)\n \t\treturn 1;\ndiff --git a/commit.c b/commit.c\nindex 40890ae7ce..016f14fe95 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -903,12 +903,22 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n \tstruct commit_list *bases;\n \tint i;\n \tstruct commit *ret = NULL;\n+\tchar *full_refname;\n+\n+\tswitch (dwim_ref(refname, strlen(refname), &oid, &full_refname)) {\n+\tcase 0:\n+\t\tdie(\"No such ref: '%s'\", refname);\n+\tcase 1:\n+\t\tbreak; /* good */\n+\tdefault:\n+\t\tdie(\"Ambiguous refname: '%s'\", refname);\n+\t}\n \n \tmemset(&revs, 0, sizeof(revs));\n \trevs.initial = 1;\n-\tfor_each_reflog_ent(refname, collect_one_reflog_ent, &revs);\n+\tfor_each_reflog_ent(full_refname, collect_one_reflog_ent, &revs);\n \n-\tif (!revs.nr && !get_oid(refname, &oid))\n+\tif (!revs.nr)\n \t\tadd_one_commit(&oid, &revs);\n \n \tfor (i = 0; i < revs.nr; i++)\n@@ -934,6 +944,7 @@ struct commit *get_fork_point(const char *refname, struct commit *commit)\n \n cleanup_return:\n \tfree_commit_list(bases);\n+\tfree(full_refname);\n \treturn ret;\n }\n \ndiff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh\nindex 78851b9a2a..172562789e 100755\n--- a/t/t3431-rebase-fork-point.sh\n+++ b/t/t3431-rebase-fork-point.sh\n@@ -47,11 +47,31 @@ test_rebase 'G F B A' --keep-base\n test_rebase 'G F C E D B A' --no-fork-point\n test_rebase 'G F C D B A' --no-fork-point --onto D\n test_rebase 'G F C B A' --no-fork-point --keep-base\n+\n test_rebase 'G F E D B A' --fork-point refs/heads/master\n+test_rebase 'G F E D B A' --fork-point master\n+\n test_rebase 'G F D B A' --fork-point --onto D refs/heads/master\n+test_rebase 'G F D B A' --fork-point --onto D master\n+\n test_rebase 'G F B A' --fork-point --keep-base refs/heads/master\n+test_rebase 'G F B A' --fork-point --keep-base master\n+\n test_rebase 'G F C E D B A' refs/heads/master\n+test_rebase 'G F C E D B A' master\n+\n test_rebase 'G F C D B A' --onto D refs/heads/master\n+test_rebase 'G F C D B A' --onto D master\n+\n test_rebase 'G F C B A' --keep-base refs/heads/master\n+test_rebase 'G F C B A' --keep-base master\n+\n+test_expect_success 'git rebase --fork-point with ambigous refname' '\n+\tgit checkout master &&\n+\tgit checkout -b one &&\n+\tgit checkout side &&\n+\tgit tag one &&\n+\ttest_must_fail git rebase --fork-point --onto D one\n+'\n \n test_done\n"}]}