From: Junio C Hamano <gitster@pobox.com>
To: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
Philippe Blain <levraiphilippeblain@gmail.com>,
Denton Liu <liu.denton@gmail.com>,
Johannes Schindelin <Johannes.Schindelin@gmx.de>,
Phillip Wood <phillip.wood123@gmail.com>,
Elijah Newren <newren@gmail.com>,
Jonathan Tan <jonathantanmy@google.com>,
Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH v2 3/7] rebase: store orig_head as a commit
Date: Wed, 07 Sep 2022 11:12:06 -0700 [thread overview]
Message-ID: <xmqqzgfbuk95.fsf@gitster.g> (raw)
In-Reply-To: 9daee95d434155742dbb19271eea8c0bc05eb365.1662561470.git.gitgitgadget@gmail.com
"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 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.
> @@ -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 ;-).
> @@ -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.
> @@ -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?
> @@ -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.
next prev parent reply other threads:[~2022-09-07 18:12 UTC|newest]
Thread overview: 82+ messages / expand[flat|nested] mbox.gz Atom feed top
2022-08-15 15:11 [PATCH 0/5] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point Phillip Wood via GitGitGadget
2022-08-15 15:11 ` [PATCH 1/5] t3416: set $EDITOR in subshell Phillip Wood via GitGitGadget
2022-08-15 16:53 ` Junio C Hamano
2022-08-16 13:53 ` Phillip Wood
2022-08-24 22:28 ` Jonathan Tan
2022-08-30 15:12 ` Phillip Wood
2022-08-15 15:11 ` [PATCH 2/5] rebase: store orig_head as a commit Phillip Wood via GitGitGadget
2022-08-15 16:58 ` Junio C Hamano
2022-08-16 9:11 ` Johannes Schindelin
2022-08-18 7:01 ` Elijah Newren
2022-08-15 15:11 ` [PATCH 3/5] rebase: factor out merge_base calculation Phillip Wood via GitGitGadget
2022-08-15 17:22 ` Junio C Hamano
2022-08-16 9:15 ` Johannes Schindelin
2022-08-16 15:00 ` Junio C Hamano
2022-08-16 13:50 ` Phillip Wood
2022-08-16 15:03 ` Junio C Hamano
2022-08-18 7:11 ` Elijah Newren
2022-08-24 22:02 ` Jonathan Tan
2022-08-30 15:03 ` Phillip Wood
2022-08-15 15:11 ` [PATCH 4/5] rebase --keep-base: imply --reapply-cherry-picks Phillip Wood via GitGitGadget
2022-08-15 20:58 ` Junio C Hamano
2022-08-24 22:09 ` Jonathan Tan
2022-08-30 15:09 ` Phillip Wood
2022-08-25 0:29 ` Philippe Blain
2022-09-05 13:54 ` Phillip Wood
2022-08-15 15:11 ` [PATCH 5/5] rebase --keep-base: imply --no-fork-point Phillip Wood via GitGitGadget
2022-08-15 21:07 ` Junio C Hamano
2022-08-24 22:18 ` Jonathan Tan
2022-09-05 13:51 ` Phillip Wood
2022-08-16 9:23 ` [PATCH 0/5] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point Johannes Schindelin
2022-08-24 22:27 ` Jonathan Tan
2022-09-07 14:37 ` [PATCH v2 0/7] " Phillip Wood via GitGitGadget
2022-09-07 14:37 ` [PATCH v2 1/7] t3416: tighten two tests Phillip Wood via GitGitGadget
2022-09-07 18:12 ` Junio C Hamano
2022-09-07 14:37 ` [PATCH v2 2/7] t3416: set $EDITOR in subshell Phillip Wood via GitGitGadget
2022-09-07 18:12 ` Junio C Hamano
2022-09-07 14:37 ` [PATCH v2 3/7] rebase: store orig_head as a commit Phillip Wood via GitGitGadget
2022-09-07 18:12 ` Junio C Hamano [this message]
2022-09-08 13:19 ` Phillip Wood
2022-09-07 14:37 ` [PATCH v2 4/7] rebase: rename merge_base to branch_base Phillip Wood via GitGitGadget
2022-09-07 18:12 ` Junio C Hamano
2022-09-07 14:37 ` [PATCH v2 5/7] rebase: factor out branch_base calculation Phillip Wood via GitGitGadget
2022-09-07 18:12 ` Junio C Hamano
2022-09-07 14:37 ` [PATCH v2 6/7] rebase --keep-base: imply --reapply-cherry-picks Phillip Wood via GitGitGadget
2022-09-07 14:37 ` [PATCH v2 7/7] rebase --keep-base: imply --no-fork-point Phillip Wood via GitGitGadget
2022-09-08 2:44 ` Denton Liu
2022-09-08 13:21 ` Phillip Wood
2022-10-13 8:42 ` [PATCH v3 0/8] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point Phillip Wood via GitGitGadget
2022-10-13 8:42 ` [PATCH v3 1/8] t3416: tighten two tests Phillip Wood via GitGitGadget
2022-10-13 8:42 ` [PATCH v3 2/8] t3416: set $EDITOR in subshell Phillip Wood via GitGitGadget
2022-10-13 8:42 ` [PATCH v3 3/8] rebase: be stricter when reading state files containing oids Phillip Wood via GitGitGadget
2022-10-13 16:34 ` Junio C Hamano
2022-10-13 19:10 ` Ævar Arnfjörð Bjarmason
2022-10-13 20:13 ` Junio C Hamano
2022-10-13 8:42 ` [PATCH v3 4/8] rebase: store orig_head as a commit Phillip Wood via GitGitGadget
2022-10-13 16:34 ` Junio C Hamano
2022-10-13 20:49 ` Phillip Wood
2022-10-13 23:25 ` Junio C Hamano
2022-10-13 8:42 ` [PATCH v3 5/8] rebase: rename merge_base to branch_base Phillip Wood via GitGitGadget
2022-10-13 19:16 ` Ævar Arnfjörð Bjarmason
2022-10-17 9:49 ` Phillip Wood
2022-10-17 11:27 ` Ævar Arnfjörð Bjarmason
2022-10-17 13:13 ` Phillip Wood
2022-10-17 16:19 ` Ævar Arnfjörð Bjarmason
2022-10-19 15:35 ` Phillip Wood
2022-10-13 8:42 ` [PATCH v3 6/8] rebase: factor out branch_base calculation Phillip Wood via GitGitGadget
2022-10-13 19:21 ` Ævar Arnfjörð Bjarmason
2022-10-17 9:39 ` Phillip Wood
2022-10-17 11:23 ` Ævar Arnfjörð Bjarmason
2022-10-13 8:42 ` [PATCH v3 7/8] rebase --keep-base: imply --reapply-cherry-picks Phillip Wood via GitGitGadget
2022-10-13 8:42 ` [PATCH v3 8/8] rebase --keep-base: imply --no-fork-point Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 0/8] rebase --keep-base: imply --reapply-cherry-picks and --no-fork-point Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 1/8] t3416: tighten two tests Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 2/8] t3416: set $EDITOR in subshell Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 3/8] rebase: be stricter when reading state files containing oids Phillip Wood via GitGitGadget
2022-10-17 18:51 ` Junio C Hamano
2022-10-19 15:32 ` Phillip Wood
2022-10-17 13:17 ` [PATCH v4 4/8] rebase: store orig_head as a commit Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 5/8] rebase: rename merge_base to branch_base Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 6/8] rebase: factor out branch_base calculation Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 7/8] rebase --keep-base: imply --reapply-cherry-picks Phillip Wood via GitGitGadget
2022-10-17 13:17 ` [PATCH v4 8/8] rebase --keep-base: imply --no-fork-point Phillip Wood via GitGitGadget
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzgfbuk95.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=Johannes.Schindelin@gmx.de \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=jonathantanmy@google.com \
--cc=levraiphilippeblain@gmail.com \
--cc=liu.denton@gmail.com \
--cc=newren@gmail.com \
--cc=phillip.wood123@gmail.com \
--cc=phillip.wood@dunelm.org.uk \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).