Re: [PATCH v2 3/7] rebase: store orig_head as a commit
From: Junio C Hamano <hidden>
Date: 2022-09-07 18:12:15
"Phillip Wood via GitGitGadget" [off-list ref] writes:
quoted hunk ↗ jump to hunk
diff --git a/builtin/rebase.c b/builtin/rebase.c index 56e4214b441..a3cf1ef5923 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c@@ -68,7 +68,7 @@ struct rebase_options { const char *upstream_name; const char *upstream_arg; char *head_name; - struct object_id orig_head; + struct commit *orig_head;
OK.
quoted hunk ↗ jump to hunk
@@ -261,13 +261,13 @@ static int do_interactive_rebase(struct rebase_options *opts, unsigned flags) struct replay_opts replay = get_replay_opts(opts); struct string_list commands = STRING_LIST_INIT_DUP; - if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head, + if (get_revision_ranges(opts->upstream, opts->onto, &opts->orig_head->object.oid, &revisions, &shortrevisions)) return -1;
This step has ton of changes like this, which are fallouts from the change of type of a member that used to be an oid to a full blown commit object. It is expected and I will strip all of them from my quotes. They all look correct, and otherwise the compiler would complain ;-).
quoted hunk ↗ jump to hunk
@@ -448,7 +447,8 @@ static int read_basic_state(struct rebase_options *opts) } else if (!read_oneliner(&buf, state_dir_path("head", opts), READ_ONELINER_WARN_MISSING)) return -1; - if (get_oid(buf.buf, &opts->orig_head)) + opts->orig_head = lookup_commit_reference_by_name(buf.buf);
This is not exactly a new problem, but I noticed it while looking for more iffy uses of lookup_commit_reference_by_name(), so... At this point in the codepath, we expect buf.buf has full-hex object name and nothing else; the original should have used get_oid_hex() to highlight that fact. lookup_commit_reference_by_name() allows object names that are not written as full-hex object name, and it may get confused if a branch or tag with 40-hex (or 64-hex in a repository with newhash) name exists. It would be a more sensible conversion to use get_oid_hex() to turn buf.buf into an object name and then use lookup_commit_reference() on it.
quoted hunk ↗ jump to hunk
@@ -866,15 +866,11 @@ static int is_linear_history(struct commit *from, struct commit *to) static int can_fast_forward(struct commit *onto, struct commit *upstream, struct commit *restrict_revision, - struct object_id *head_oid, struct object_id *merge_base) + struct commit *head, struct object_id *merge_base) { - struct commit *head = lookup_commit(the_repository, head_oid); struct commit_list *merge_bases = NULL; int res = 0; - if (!head) - goto done; -
This one benefits from being able to avoid its own lookup_commit() because the caller already has it in the desired form. This is not a comment on the new code, but it does make readers wonder if the conversion changes behaviour. lookup_commit() takes an object name and requires it to be a commit object's name, doesn't it? If we gave a tag to the program, the old code would have had the object name of that tag in head_oid and at this point and lookup_commit() noticed and would have stopped you from fast forwarding your branch to the tag, which was a good thing. In the new code, since we turn the object name we take from the user into a commit object way before the control reaches this place, we won't get such an error here, but if we fast-forward to the object, we will still fast forward to the commit that is pointed by the tag, so the new behaviour is even better, perhaps?
quoted hunk ↗ jump to hunk
@@ -1610,17 +1606,17 @@ int cmd_rebase(int argc, const char **argv, const char *prefix) /* Is it a local branch? */ strbuf_reset(&buf); strbuf_addf(&buf, "refs/heads/%s", branch_name); - if (!read_ref(buf.buf, &options.orig_head)) { + options.orig_head = lookup_commit_reference_by_name(buf.buf); + if (options.orig_head) { die_if_checked_out(buf.buf, 1); options.head_name = xstrdup(buf.buf); /* If not is it a valid ref (branch or commit)? */
This is iffy, or it may be just wrong. The old code is checking if "refs/heads/$branch_name" is a branch and does the check. If you had a branch "refs/heads/A" (whose ref is at "refs/heads/refs/heads/A") but do not have a branch "A", and if you fed "A" to this part of the code in buf.buf, then the original code would not have been fooled by the presence of such a funny branch. New code (incorrectly) does because it prefixes "refs/heads/" to "A" and asks to turn string "refs/heads/A" into a commit object, triggering the usual ref dwim rules. We end up setting options.head_name to a wrong thing (in this case, the user said "A", we turned it into a refname "refs/heads/A" that does not exist, and set options.orig_head to the commit object pointed by the ref "refs/heads/refs/heads/A", and we use that commit as orig_head, but use an incorrect head_name). I didn't look as carefully as this one, but there may be similarly iffy uses of lookup_commit_reference_by_name() introduced by this patch that used to be more strict/exact; they may need to be fixed.
} else {
- struct commit *commit =
+ options.orig_head =
lookup_commit_reference_by_name(branch_name);
- if (!commit)
+ if (!options.orig_head)
die(_("no such branch/commit '%s'"),
branch_name);
- oidcpy(&options.orig_head, &commit->object.oid);
options.head_name = NULL;This side, which is "this is what happens to a random object name that is not a branch name", is perfectly fine.