git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
ATAlex Torok <alext9@gmail.com>
Date
Dec 9, 2019, 14:53 UTC
Message-ID
<20191209145312.3251-2-alext9@gmail.com>
In-Reply-To
<20191209145312.3251-1-alext9@gmail.com>

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
Previous: Alex TorokNext: Junio C Hamano
Message 18 of 23 in “rebase: fix bug in --fork-point”
  1. 0/3 rebase: fix bug in --fork-pointAlex Torok, Dec 5, 2019
  2. 1/3 rebase: add test for rebase --fork-point with short upstreamAlex Torok, Dec 5, 2019
  3. Junio C HamanoDec 5, 2019
  4. Alex TorokDec 5, 2019
  5. 2/3 rebase: refactor dwim_ref_or_die from merge-base.cAlex Torok, Dec 5, 2019
  6. 3/3 rebase: fix rebase to use full ref to find fork-pointAlex Torok, Dec 5, 2019
  7. 0/2 rebase: fix bug in --fork-pointAlex Torok, Dec 5, 2019
  8. 1/2 rebase: refactor dwim_ref_or_die from merge-base.cAlex Torok, Dec 5, 2019
  9. Denton LiuDec 6, 2019
  10. Alex TorokDec 6, 2019
  11. 2/2 rebase: find --fork-point with full refAlex Torok, Dec 5, 2019
  12. Denton LiuDec 6, 2019
  13. Phillip WoodDec 6, 2019
  14. Alex TorokDec 6, 2019
  15. Denton LiuDec 6, 2019
  16. Phillip WoodDec 6, 2019
  17. 0/1 rebase: fix --fork-point with short ref upstreamAlex Torok, Dec 9, 2019
  18. 1/1 rebase: fix --fork-point with short refnameAlex Torok, Dec 9, 2019
  19. Junio C HamanoDec 9, 2019
  20. Alex TorokDec 11, 2019
  21. Denton LiuDec 11, 2019
  22. Eric SunshineDec 11, 2019
  23. Junio C HamanoFeb 11, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.