[PATCH 0/3] rebase: fix bug in --fork-point

[PATCH 0/3] rebase: fix bug in --fork-point

[Alex Torok]

git rebase --fork-point with a short branch ref name (without
refs/heads/ prefix) for the upstream will not use the fork point between
the upstream and the branch.

If git rebase --forkpoint is used with a full ref on the upstream
(refs/heads/branchName), the fork point between the branch and upstream
is correctly used.

This bug was introduced in 2.20.0 with the c implementation of rebase. I
was able to reproduce it on next and master.

The code path for rebase with --fork-point and a user-provided upstream
does not use a DWIM method to look up the full ref name. This leads to
get_fork_point returning a null pointer and rebase using the upstream
itself instead of the fork point between the upstream and branch.

I looked in other places that call get_fork_point and found that
in handle_fork_point of builtin/merge_base.c, dwim_ref is used to find
the full ref name before calling get_fork_point.

I initially based this change off of maint, but then saw that there were
some tests added for rebase --fork-point on master, so I rebased onto
there to build off of those tests, but I'm not sure if that was the "right"
thing to do.

Alex Torok (3):
  rebase: add test for rebase --fork-point with short upstream
  rebase: refactor dwim_ref_or_die from merge-base.c
  rebase: fix rebase to use full ref to find fork-point

 builtin/merge-base.c         |  9 +--------
 builtin/rebase.c             |  4 +++-
 refs.c                       | 14 ++++++++++++++
 refs.h                       |  1 +
 t/t3431-rebase-fork-point.sh |  1 +
 5 files changed, 20 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH 1/3] rebase: add test for rebase --fork-point with short upstream

[Alex Torok]

This proves the bug of "rebase --fork-point upstream branch" not using
the fork point of upstream and branch if upstream is not the full
refname of the upstream branch.

Signed-off-by: Alex Torok <alext9@gmail.com>
---
 t/t3431-rebase-fork-point.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 78851b9a2a..6ecdae918e 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -49,6 +49,7 @@ 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 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 C E D B A' refs/heads/master
 test_rebase 'G F C D B A' --onto D refs/heads/master
-- 
2.17.1

[PATCH 2/3] rebase: refactor dwim_ref_or_die from merge-base.c

[Alex Torok]

Pull logic for getting the full dwim ref name from a passed in ref name
out of the handle_fork_point function of merge-base.c. This will allow
git-rebase --fork-point to use the same method for getting the full ref
name before calling get_fork_point.

I saw other *_or_die methods in other places and using that pattern
seemed sane here.

I'm not 100% sure about the name or signature of dwim_ref_or_die. I feel
like it should be named something like dwim_ref_name_or_die,
unique_dwim_ref_or_die, or should be including the object_id argument
even though it isn't used by the calling merge_base code, and won't be
used in rebase.

This is my first patch submission for git, and I'd appreciate some
feedback on naming/style wrt to this.

Signed-off-by: Alex Torok <alext9@gmail.com>
---
 builtin/merge-base.c |  9 +--------
 refs.c               | 14 ++++++++++++++
 refs.h               |  1 +
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index e3f8da13b6..edd16f9fcd 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -118,14 +118,7 @@ static int handle_fork_point(int argc, const char **argv)
 	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]);
-	}
+	dwim_ref_or_die(argv[0], strlen(argv[0]), &refname);
 
 	commitname = (argc == 2) ? argv[1] : "HEAD";
 	if (get_oid(commitname, &oid))
diff --git a/refs.c b/refs.c
index 1ab0bb54d3..3b778f2df9 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);
 }
 
+void dwim_ref_or_die(const char *str, int len, char **ref)
+{
+	struct object_id discard;
+	switch (dwim_ref(str, len, &discard, ref)) {
+	case 0:
+		die("No such ref: '%s'", str);
+	case 1:
+		break; /* good */
+	default:
+		die("Ambiguous refname: '%s'", str);
+	}
+}
+
+
 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..a961662382 100644
--- a/refs.h
+++ b/refs.h
@@ -154,6 +154,7 @@ 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);
 
+void dwim_ref_or_die(const char *str, int len, char **ref);
 /*
  * A ref_transaction represents a collection of reference updates that
  * should succeed or fail together.
-- 
2.17.1

[PATCH 3/3] rebase: fix rebase to use full ref to find fork-point

[Alex Torok]

rebase --fork-point needs to look up the full ref name before calling
get_fork_point in the same manner that merge-base --fork-point does.

Signed-off-by: Alex Torok <alext9@gmail.com>
---
 builtin/rebase.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e755087b0f..821994f676 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct commit *head =
 			lookup_commit_reference(the_repository,
 						&options.orig_head);
+		char * full_name;
+		dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
 		options.restrict_revision =
-			get_fork_point(options.upstream_name, head);
+			get_fork_point(full_name, head);
 	}
 
 	if (repo_read_index(the_repository) < 0)
-- 
2.17.1

Re: [PATCH 1/3] rebase: add test for rebase --fork-point with short upstream

[Junio C Hamano]

Alex Torok <alext9@gmail.com> writes:

> This proves the bug of "rebase --fork-point upstream branch" not using
> the fork point of upstream and branch if upstream is not the full
> refname of the upstream branch.
>
> Signed-off-by: Alex Torok <alext9@gmail.com>
> ---
>  t/t3431-rebase-fork-point.sh | 1 +
>  1 file changed, 1 insertion(+)

Is this new test expected to fail after applying only 1/3 and then
starts working after applying all 3 patches?

If so, it probably makes a lot mroe sense to reorder the series to
have 2/3 as a single preparatory patch, with 1/3 + 3/3 combined into
a single patch "rebase: find --fork-point with full ref" to fix the
code and protect the fix with the test at the same time.

> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
> index 78851b9a2a..6ecdae918e 100755
> --- a/t/t3431-rebase-fork-point.sh
> +++ b/t/t3431-rebase-fork-point.sh
> @@ -49,6 +49,7 @@ 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 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 C E D B A' refs/heads/master
>  test_rebase 'G F C D B A' --onto D refs/heads/master

Re: [PATCH 1/3] rebase: add test for rebase --fork-point with short upstream

[Alex Torok]

That was the intention. Your idea for ordering makes more sense.

I'll reorder the commits and submit new patches.


On Thu, Dec 5, 2019 at 6:04 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Alex Torok <alext9@gmail.com> writes:
>
> > This proves the bug of "rebase --fork-point upstream branch" not using
> > the fork point of upstream and branch if upstream is not the full
> > refname of the upstream branch.
> >
> > Signed-off-by: Alex Torok <alext9@gmail.com>
> > ---
> >  t/t3431-rebase-fork-point.sh | 1 +
> >  1 file changed, 1 insertion(+)
>
> Is this new test expected to fail after applying only 1/3 and then
> starts working after applying all 3 patches?
>
> If so, it probably makes a lot mroe sense to reorder the series to
> have 2/3 as a single preparatory patch, with 1/3 + 3/3 combined into
> a single patch "rebase: find --fork-point with full ref" to fix the
> code and protect the fix with the test at the same time.
>
> > diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
> > index 78851b9a2a..6ecdae918e 100755
> > --- a/t/t3431-rebase-fork-point.sh
> > +++ b/t/t3431-rebase-fork-point.sh
> > @@ -49,6 +49,7 @@ 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 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 C E D B A' refs/heads/master
> >  test_rebase 'G F C D B A' --onto D refs/heads/master

[PATCH v2 0/2] rebase: fix bug in --fork-point

[Alex Torok]

Reorder commits to include the fix and test in the same commit.

Alex Torok (2):
  rebase: refactor dwim_ref_or_die from merge-base.c
  rebase: find --fork-point with full ref

 builtin/merge-base.c         |  9 +--------
 builtin/rebase.c             |  4 +++-
 refs.c                       | 14 ++++++++++++++
 refs.h                       |  1 +
 t/t3431-rebase-fork-point.sh |  1 +
 5 files changed, 20 insertions(+), 9 deletions(-)

-- 
2.17.1

[PATCH v2 1/2] rebase: refactor dwim_ref_or_die from merge-base.c

[Alex Torok]

Pull logic for getting the full dwim ref name from a passed in ref name
out of the handle_fork_point function of merge-base.c. This will allow
git-rebase --fork-point to use the same method for getting the full ref
name before calling get_fork_point.

I saw other *_or_die methods in other places and using that pattern
seemed sane here.

I'm not 100% sure about the name or signature of dwim_ref_or_die. I feel
like it should be named something like dwim_ref_name_or_die,
unique_dwim_ref_or_die, or should be including the object_id argument
even though it isn't used by the calling merge_base code, and won't be
used in rebase.

This is my first patch submission for git, and I'd appreciate some
feedback on naming/style wrt to this.

Signed-off-by: Alex Torok <alext9@gmail.com>
---
 builtin/merge-base.c |  9 +--------
 refs.c               | 14 ++++++++++++++
 refs.h               |  1 +
 3 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/builtin/merge-base.c b/builtin/merge-base.c
index e3f8da13b6..edd16f9fcd 100644
--- a/builtin/merge-base.c
+++ b/builtin/merge-base.c
@@ -118,14 +118,7 @@ static int handle_fork_point(int argc, const char **argv)
 	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]);
-	}
+	dwim_ref_or_die(argv[0], strlen(argv[0]), &refname);
 
 	commitname = (argc == 2) ? argv[1] : "HEAD";
 	if (get_oid(commitname, &oid))
diff --git a/refs.c b/refs.c
index 1ab0bb54d3..3b778f2df9 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);
 }
 
+void dwim_ref_or_die(const char *str, int len, char **ref)
+{
+	struct object_id discard;
+	switch (dwim_ref(str, len, &discard, ref)) {
+	case 0:
+		die("No such ref: '%s'", str);
+	case 1:
+		break; /* good */
+	default:
+		die("Ambiguous refname: '%s'", str);
+	}
+}
+
+
 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..a961662382 100644
--- a/refs.h
+++ b/refs.h
@@ -154,6 +154,7 @@ 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);
 
+void dwim_ref_or_die(const char *str, int len, char **ref);
 /*
  * A ref_transaction represents a collection of reference updates that
  * should succeed or fail together.
-- 
2.17.1

[PATCH v2 2/2] rebase: find --fork-point with full ref

[Alex Torok]

rebase --fork-point needs to look up the full ref name before calling
get_fork_point in the same manner that merge-base --fork-point does.

Signed-off-by: Alex Torok <alext9@gmail.com>
---
 builtin/rebase.c             | 4 +++-
 t/t3431-rebase-fork-point.sh | 1 +
 2 files changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/rebase.c b/builtin/rebase.c
index e755087b0f..821994f676 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
 		struct commit *head =
 			lookup_commit_reference(the_repository,
 						&options.orig_head);
+		char * full_name;
+		dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
 		options.restrict_revision =
-			get_fork_point(options.upstream_name, head);
+			get_fork_point(full_name, head);
 	}
 
 	if (repo_read_index(the_repository) < 0)
diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
index 78851b9a2a..6ecdae918e 100755
--- a/t/t3431-rebase-fork-point.sh
+++ b/t/t3431-rebase-fork-point.sh
@@ -49,6 +49,7 @@ 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 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 C E D B A' refs/heads/master
 test_rebase 'G F C D B A' --onto D refs/heads/master
-- 
2.17.1

Re: [PATCH v2 1/2] rebase: refactor dwim_ref_or_die from merge-base.c

[Denton Liu]

Hi Alex,

Thanks for the contribution.

On Thu, Dec 05, 2019 at 06:57:03PM -0500, Alex Torok wrote:
> Pull logic for getting the full dwim ref name from a passed in ref name
> out of the handle_fork_point function of merge-base.c. This will allow
> git-rebase --fork-point to use the same method for getting the full ref
> name before calling get_fork_point.
> 

The remaining paragraphs shouldn't end up going into the commit message
since they're more like "notes to reviewers". You should place these
notes under the `---` below so that they won't end up in the final
commit message.

> I saw other *_or_die methods in other places and using that pattern
> seemed sane here.
> 
> I'm not 100% sure about the name or signature of dwim_ref_or_die. I feel
> like it should be named something like dwim_ref_name_or_die,
> unique_dwim_ref_or_die, or should be including the object_id argument
> even though it isn't used by the calling merge_base code, and won't be
> used in rebase.
> 
> This is my first patch submission for git, and I'd appreciate some
> feedback on naming/style wrt to this.
> 
> Signed-off-by: Alex Torok <alext9@gmail.com>
> ---
>  builtin/merge-base.c |  9 +--------
>  refs.c               | 14 ++++++++++++++
>  refs.h               |  1 +
>  3 files changed, 16 insertions(+), 8 deletions(-)
> 
> diff --git a/builtin/merge-base.c b/builtin/merge-base.c
> index e3f8da13b6..edd16f9fcd 100644
> --- a/builtin/merge-base.c
> +++ b/builtin/merge-base.c
> @@ -118,14 +118,7 @@ static int handle_fork_point(int argc, const char **argv)
>  	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]);
> -	}
> +	dwim_ref_or_die(argv[0], strlen(argv[0]), &refname);
>  
>  	commitname = (argc == 2) ? argv[1] : "HEAD";
>  	if (get_oid(commitname, &oid))
> diff --git a/refs.c b/refs.c
> index 1ab0bb54d3..3b778f2df9 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);
>  }
>  
> +void dwim_ref_or_die(const char *str, int len, char **ref)
> +{
> +	struct object_id discard;
> +	switch (dwim_ref(str, len, &discard, ref)) {
> +	case 0:
> +		die("No such ref: '%s'", str);

I see that this code is copied so I'm fine with leaving it as is. But if
you need to do any rerolls, it would be nice to, as a prepatory step,
introduce a patch which 1. marks these strings for translation and 2.
lowercases the first letter of the sentence which is the convention for
messages that are seen by the end-user.

Perhaps:

	die(_("no such ref: '%s'"), str);

> +	case 1:
> +		break; /* good */
> +	default:
> +		die("Ambiguous refname: '%s'", str);

Same comment here.

Thanks,

Denton

> +	}
> +}
> +
> +
>  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..a961662382 100644
> --- a/refs.h
> +++ b/refs.h
> @@ -154,6 +154,7 @@ 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);
>  
> +void dwim_ref_or_die(const char *str, int len, char **ref);
>  /*
>   * A ref_transaction represents a collection of reference updates that
>   * should succeed or fail together.
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/2] rebase: find --fork-point with full ref

[Denton Liu]

Hi Alex,

On Thu, Dec 05, 2019 at 06:57:04PM -0500, Alex Torok wrote:
> rebase --fork-point needs to look up the full ref name before calling
> get_fork_point in the same manner that merge-base --fork-point does.
> 
> Signed-off-by: Alex Torok <alext9@gmail.com>
> ---
>  builtin/rebase.c             | 4 +++-
>  t/t3431-rebase-fork-point.sh | 1 +
>  2 files changed, 4 insertions(+), 1 deletion(-)
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index e755087b0f..821994f676 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		struct commit *head =
>  			lookup_commit_reference(the_repository,
>  						&options.orig_head);
> +		char * full_name;

nit: * should be attached to the variable name.

> +		dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);

Also, thinking about this more, would it be possible to put the dwim_ref
logic into get_fork_point() directly? There are currently only these two
callers so I suspect it should be fine and it'll result in cleaner
logic.

We could also squash it down into one patch.

>  		options.restrict_revision =
> -			get_fork_point(options.upstream_name, head);
> +			get_fork_point(full_name, head);
>  	}
>  
>  	if (repo_read_index(the_repository) < 0)
> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
> index 78851b9a2a..6ecdae918e 100755
> --- a/t/t3431-rebase-fork-point.sh
> +++ b/t/t3431-rebase-fork-point.sh
> @@ -49,6 +49,7 @@ 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 D B A' --fork-point --onto D refs/heads/master
> +test_rebase 'G F D B A' --fork-point --onto D master

It's not obvious why this was failing in the first place. Perhaps we
could document it better in the commit message?

Maybe something like:

	We used to pass in the upstream_name directly into the
	get_fork_point() machinery. However, get_fork_point() was
	expecting a fully qualified ref name even though most users use
	the short name for branches. This resulted in `--fork-point` not
	working as expected since, without the full ref name, the reflog
	lookup would fail and it would behave as if we weren't passing
	in `--fork-point` at all.

Also, I'm not why this test case in particular that was duplicated (and
not the one above) given that the first three `--fork-point` test cases
fail without the change to rebase. Perhaps we want to duplicate all
"refs/heads/master" tests with a corresponding "master" test?

Thanks,

Denton

>  test_rebase 'G F B A' --fork-point --keep-base refs/heads/master
>  test_rebase 'G F C E D B A' refs/heads/master
>  test_rebase 'G F C D B A' --onto D refs/heads/master
> -- 
> 2.17.1
> 

Re: [PATCH v2 2/2] rebase: find --fork-point with full ref

[Phillip Wood]

Hi Alex

Thanks for working on this

On 06/12/2019 01:48, Denton Liu wrote:
> Hi Alex,
> 
> On Thu, Dec 05, 2019 at 06:57:04PM -0500, Alex Torok wrote:
>> rebase --fork-point needs to look up the full ref name before calling
>> get_fork_point in the same manner that merge-base --fork-point does.
>>
>> Signed-off-by: Alex Torok <alext9@gmail.com>
>> ---
>>   builtin/rebase.c             | 4 +++-
>>   t/t3431-rebase-fork-point.sh | 1 +
>>   2 files changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/builtin/rebase.c b/builtin/rebase.c
>> index e755087b0f..821994f676 100644
>> --- a/builtin/rebase.c
>> +++ b/builtin/rebase.c
>> @@ -1980,8 +1980,10 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>>   		struct commit *head =
>>   			lookup_commit_reference(the_repository,
>>   						&options.orig_head);
>> +		char * full_name;
> 
> nit: * should be attached to the variable name.

I think you also need to free it once you've called get_fork_point() as 
well.

> 
>> +		dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
> 
> Also, thinking about this more, would it be possible to put the dwim_ref
> logic into get_fork_point() directly? There are currently only these two
> callers so I suspect it should be fine and it'll result in cleaner
> logic.

If you do that then it would be better to use error() rather than die() 
in get_fork_point() and return an error to the caller as we try to avoid 
adding code to libgit that dies. This lets the caller handle any cleanup 
that they need to before exiting.

Best Wishes

Phillip

> 
> We could also squash it down into one patch.
> 
>>   		options.restrict_revision =
>> -			get_fork_point(options.upstream_name, head);
>> +			get_fork_point(full_name, head);
>>   	}
>>   
>>   	if (repo_read_index(the_repository) < 0)
>> diff --git a/t/t3431-rebase-fork-point.sh b/t/t3431-rebase-fork-point.sh
>> index 78851b9a2a..6ecdae918e 100755
>> --- a/t/t3431-rebase-fork-point.sh
>> +++ b/t/t3431-rebase-fork-point.sh
>> @@ -49,6 +49,7 @@ 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 D B A' --fork-point --onto D refs/heads/master
>> +test_rebase 'G F D B A' --fork-point --onto D master
> 
> It's not obvious why this was failing in the first place. Perhaps we
> could document it better in the commit message?
> 
> Maybe something like:
> 
> 	We used to pass in the upstream_name directly into the
> 	get_fork_point() machinery. However, get_fork_point() was
> 	expecting a fully qualified ref name even though most users use
> 	the short name for branches. This resulted in `--fork-point` not
> 	working as expected since, without the full ref name, the reflog
> 	lookup would fail and it would behave as if we weren't passing
> 	in `--fork-point` at all.
> 
> Also, I'm not why this test case in particular that was duplicated (and
> not the one above) given that the first three `--fork-point` test cases
> fail without the change to rebase. Perhaps we want to duplicate all
> "refs/heads/master" tests with a corresponding "master" test?
> 
> Thanks,
> 
> Denton
> 
>>   test_rebase 'G F B A' --fork-point --keep-base refs/heads/master
>>   test_rebase 'G F C E D B A' refs/heads/master
>>   test_rebase 'G F C D B A' --onto D refs/heads/master
>> -- 
>> 2.17.1
>>

Re: [PATCH v2 1/2] rebase: refactor dwim_ref_or_die from merge-base.c

[Alex Torok]

On Thu, Dec 5, 2019 at 8:23 PM Denton Liu <liu.denton@gmail.com> wrote:
> The remaining paragraphs shouldn't end up going into the commit message
> since they're more like "notes to reviewers". You should place these
> notes under the `---` below so that they won't end up in the final
> commit message.

Thank you for the info! I'll keep that in mind for my next patch submission.

> > +void dwim_ref_or_die(const char *str, int len, char **ref)
> > +{
> > +     struct object_id discard;
> > +     switch (dwim_ref(str, len, &discard, ref)) {
> > +     case 0:
> > +             die("No such ref: '%s'", str);
>
> I see that this code is copied so I'm fine with leaving it as is. But if
> you need to do any rerolls, it would be nice to, as a prepatory step,
> introduce a patch which 1. marks these strings for translation and 2.
> lowercases the first letter of the sentence which is the convention for
> messages that are seen by the end-user.
>
> Perhaps:
>
>         die(_("no such ref: '%s'"), str);
>

Sounds good! I'll add a patch to the end of this series that addresses
these strings not being translated.

Thank you for your feedback.
~Alex

> > +     case 1:
> > +             break; /* good */
> > +     default:
> > +             die("Ambiguous refname: '%s'", str);
>
> Same comment here.
>
> Thanks,
>
> Denton
>
> > +     }
> > +}
> > +
> > +
> >  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..a961662382 100644
> > --- a/refs.h
> > +++ b/refs.h
> > @@ -154,6 +154,7 @@ 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);
> >
> > +void dwim_ref_or_die(const char *str, int len, char **ref);
> >  /*
> >   * A ref_transaction represents a collection of reference updates that
> >   * should succeed or fail together.
> > --
> > 2.17.1
> >

Re: [PATCH v2 2/2] rebase: find --fork-point with full ref

[Alex Torok]

Thank you for the feedback Denton & Phillip!

On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> On 06/12/2019 01:48, Denton Liu wrote:
> > nit: * should be attached to the variable name.
>
> I think you also need to free it once you've called get_fork_point() as
> well.

Yup. Got it.

> On 06/12/2019 01:48, Denton Liu wrote:
> >
> >> +            dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
> >
> > Also, thinking about this more, would it be possible to put the dwim_ref
> > logic into get_fork_point() directly? There are currently only these two
> > callers so I suspect it should be fine and it'll result in cleaner
> > logic.
>
> If you do that then it would be better to use error() rather than die()
> in get_fork_point() and return an error to the caller as we try to avoid
> adding code to libgit that dies. This lets the caller handle any cleanup
> that they need to before exiting.

Would the signature of get_fork_point change to be something like:
int get_fork_point(const char *refname, struct commit *commit,
   struct commit **fork_point, struct strbuf *err)

If not, could you point me to an example of some existing code
that does what you're talking about?


> On 06/12/2019 01:48, Denton Liu wrote:
> > It's not obvious why this was failing in the first place. Perhaps we
> > could document it better in the commit message?
> >
> > Maybe something like:
> >
> >       We used to pass in the upstream_name directly into the
> >       get_fork_point() machinery. However, get_fork_point() was
> >       expecting a fully qualified ref name even though most users use
> >       the short name for branches. This resulted in `--fork-point` not
> >       working as expected since, without the full ref name, the reflog
> >       lookup would fail and it would behave as if we weren't passing
> >       in `--fork-point` at all.

Sounds good.

> > Also, I'm not why this test case in particular that was duplicated (and
> > not the one above) given that the first three `--fork-point` test cases
> > fail without the change to rebase. Perhaps we want to duplicate all
> > "refs/heads/master" tests with a corresponding "master" test?

I only duplicated one so that there would only be one test case that
would fail if a regression around getting the fork point with a short
ref name was introduced.

I just happened to pick that one because it was closest to the rebase
command I was running when I found the bug :)

I can include some of the above reasoning in the commit message.
Alternatively:
* I could duplicate all of tests
* I could change all of the tests to use the short ref name

I'm leaning towards just leaving one test (maybe with a comment?)
for the short ref name --fork-point so that there is more resolution
around where a bug could be on test failure.

Let me know what you think,
Alex

Re: [PATCH v2 2/2] rebase: find --fork-point with full refgg

[Denton Liu]

Hi Alex,

On Fri, Dec 06, 2019 at 08:46:29AM -0500, Alex Torok wrote:
> Thank you for the feedback Denton & Phillip!
> 
> On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
> > On 06/12/2019 01:48, Denton Liu wrote:
> > > nit: * should be attached to the variable name.
> >
> > I think you also need to free it once you've called get_fork_point() as
> > well.
> 
> Yup. Got it.
> 
> > On 06/12/2019 01:48, Denton Liu wrote:
> > >
> > >> +            dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
> > >
> > > Also, thinking about this more, would it be possible to put the dwim_ref
> > > logic into get_fork_point() directly? There are currently only these two
> > > callers so I suspect it should be fine and it'll result in cleaner
> > > logic.
> >
> > If you do that then it would be better to use error() rather than die()
> > in get_fork_point() and return an error to the caller as we try to avoid
> > adding code to libgit that dies. This lets the caller handle any cleanup
> > that they need to before exiting.
> 
> Would the signature of get_fork_point change to be something like:
> int get_fork_point(const char *refname, struct commit *commit,
>    struct commit **fork_point, struct strbuf *err)

I would drop the last parameter. If an error is detected, you could just
do

	return error(_("oh no, something bad happened"));

Even though we try and avoid dying in the middle of libgit, we print
errors out very often so it should be fine here.

> > > Also, I'm not why this test case in particular that was duplicated (and
> > > not the one above) given that the first three `--fork-point` test cases
> > > fail without the change to rebase. Perhaps we want to duplicate all
> > > "refs/heads/master" tests with a corresponding "master" test?
> 
> I only duplicated one so that there would only be one test case that
> would fail if a regression around getting the fork point with a short
> ref name was introduced.
> 
> I just happened to pick that one because it was closest to the rebase
> command I was running when I found the bug :)
> 
> I can include some of the above reasoning in the commit message.
> Alternatively:
> * I could duplicate all of tests
> * I could change all of the tests to use the short ref name
> 
> I'm leaning towards just leaving one test (maybe with a comment?)
> for the short ref name --fork-point so that there is more resolution
> around where a bug could be on test failure.

I would just duplicate all of the tests. When the tests are pretty cheap
to run (as they are in this case), I tend to err on the side of adding
more tests since they might catch more odd edge-cases but, in this case,
all of the fork point logic goes through one common block so the
duplicate logic doesn't really buy us anything.

I'm pretty impartial so I'll leave it up to you ;)

Thanks,

Denton

> 
> Let me know what you think,
> Alex

Re: [PATCH v2 2/2] rebase: find --fork-point with full refgg

[Phillip Wood]

On 06/12/2019 19:11, Denton Liu wrote:
> Hi Alex,
> 
> On Fri, Dec 06, 2019 at 08:46:29AM -0500, Alex Torok wrote:
>> Thank you for the feedback Denton & Phillip!
>>
>> On Fri, Dec 6, 2019 at 5:52 AM Phillip Wood <phillip.wood123@gmail.com> wrote:
>>> On 06/12/2019 01:48, Denton Liu wrote:
>>>> nit: * should be attached to the variable name.
>>>
>>> I think you also need to free it once you've called get_fork_point() as
>>> well.
>>
>> Yup. Got it.
>>
>>> On 06/12/2019 01:48, Denton Liu wrote:
>>>>
>>>>> +            dwim_ref_or_die(options.upstream_name, strlen(options.upstream_name), &full_name);
>>>>
>>>> Also, thinking about this more, would it be possible to put the dwim_ref
>>>> logic into get_fork_point() directly? There are currently only these two
>>>> callers so I suspect it should be fine and it'll result in cleaner
>>>> logic.
>>>
>>> If you do that then it would be better to use error() rather than die()
>>> in get_fork_point() and return an error to the caller as we try to avoid
>>> adding code to libgit that dies. This lets the caller handle any cleanup
>>> that they need to before exiting.
>>
>> Would the signature of get_fork_point change to be something like:
>> int get_fork_point(const char *refname, struct commit *commit,
>>     struct commit **fork_point, struct strbuf *err)
> 
> I would drop the last parameter. If an error is detected, you could just
> do
> 
> 	return error(_("oh no, something bad happened"));
> 
> Even though we try and avoid dying in the middle of libgit, we print
> errors out very often so it should be fine here.

Yes that was what I was thinking of

Best Wishes

Phillip


> 
>>>> Also, I'm not why this test case in particular that was duplicated (and
>>>> not the one above) given that the first three `--fork-point` test cases
>>>> fail without the change to rebase. Perhaps we want to duplicate all
>>>> "refs/heads/master" tests with a corresponding "master" test?
>>
>> I only duplicated one so that there would only be one test case that
>> would fail if a regression around getting the fork point with a short
>> ref name was introduced.
>>
>> I just happened to pick that one because it was closest to the rebase
>> command I was running when I found the bug :)
>>
>> I can include some of the above reasoning in the commit message.
>> Alternatively:
>> * I could duplicate all of tests
>> * I could change all of the tests to use the short ref name
>>
>> I'm leaning towards just leaving one test (maybe with a comment?)
>> for the short ref name --fork-point so that there is more resolution
>> around where a bug could be on test failure.
> 
> I would just duplicate all of the tests. When the tests are pretty cheap
> to run (as they are in this case), I tend to err on the side of adding
> more tests since they might catch more odd edge-cases but, in this case,
> all of the fork point logic goes through one common block so the
> duplicate logic doesn't really buy us anything.
> 
> I'm pretty impartial so I'll leave it up to you ;)
> 
> Thanks,
> 
> Denton
> 
>>
>> Let me know what you think,
>> Alex

[PATCH v3 0/1] rebase: fix --fork-point with short ref upstream

[Alex Torok]

Move the dwim ref logic into get_fork_point() and return an error code
so that callers of get_fork_point() know if something went wrong.

Alex Torok (1):
  rebase: fix --fork-point with short refname

 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(-)

-- 
2.17.1

[PATCH v3 1/1] rebase: fix --fork-point with short refname

[Alex Torok]

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(-)

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
-- 
2.17.1

Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname

[Junio C Hamano]

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

Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname

[Alex Torok]

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

Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname

[Denton Liu]

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

Re: [PATCH v3 1/1] rebase: fix --fork-point with short refname

[Eric Sunshine]

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).

Re: [PATCH v4 1/1] rebase: --fork-point regression fix

[Junio C Hamano]

"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