Re: [PATCH v2 1/2] rebase: refactor dwim_ref_or_die from merge-base.c
From: Denton Liu <hidden>
Date: 2019-12-06 01:23:15
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.
quoted hunk ↗ jump to hunk
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 <redacted> --- 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
quoted hunk ↗ jump to hunk
+ } +} + + 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