From: Junio C Hamano <gitster@pobox.com>
To: "Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, Derrick Stolee <derrickstolee@github.com>,
Johannes Schindelin <johannes.schindelin@gmx.de>
Subject: Re: [PATCH 1/2] rebase --update-refs: fix loops
Date: Wed, 10 May 2023 16:25:50 -0700 [thread overview]
Message-ID: <xmqqcz3722gx.fsf@gitster.g> (raw)
In-Reply-To: <2ac7c7a7c615db75a46076b58a51d363bc2daf2e.1683759338.git.gitgitgadget@gmail.com> (Johannes Schindelin via GitGitGadget's message of "Wed, 10 May 2023 22:55:37 +0000")
"Johannes Schindelin via GitGitGadget" <gitgitgadget@gmail.com>
writes:
> From: Johannes Schindelin <johannes.schindelin@gmx.de>
>
> The `total_nr` field in the `todo_list` structure merely serves display
> purposes, and should only be used when generating the progress message.
This is a good distinction to keep in mind.
I notice that sequencer.h does not do a very good job at giving
guidance on how these members are to be used.
struct todo_list {
struct strbuf buf;
struct todo_item *items;
int nr, alloc, current;
int done_nr, total_nr;
};
The <nr,alloc> tuple lets readers to guess they are tied to the
items[] array, so perhaps it is sufficient to give a comment to
total_nr member and probably done_nr while we are at it.
> In these two instances, however, we want to loop over all of the
> commands in the parsed rebase script. The loop limit therefore needs to
> be `nr`, which refers to the count of commands in the current
> `todo_list`.
Yes.
> This is important because the two numbers, `nr` and `total_nr` can
> differ wildly, e.g. due to `total_nr` _not_ counting comments or empty
> lines, while `nr` skips any commands that already moved from the
> `git-rebase-todo` file to the `done` file.
OK. The items[] array has not just executable insn but also holds
comments and NOOP, and <nr,alloc> tuple is used to control its
sizing in the usual ALLOC_GROW() way. Because total_nr is used only
for progress, it naturally excludes the no-ops. Elements of items[]
array are consumed in core by incrementing the current pointer and
nr will not update while that is happening, but when the sequencer
gives control the user and then takes the control back upon resuming,
items[] would contain only the insns that have not been moved to the
done list, meaning that 'nr' would shrink. total_nr is compensated
by reading the done list and adds its size to 'nr'.
OK, that all makes sense. The whole arrangement sounds like a bit
more error prone than necessary (an obvious alternative is to just
always keep the whole todo list with "done up to here" pointner) but
changing that is not in the scope of these fixes, because such an
arrangement wouldn't have prevented this particular bug from
happening, as total_nr and nr could still be different due to
no-ops.
> diff --git a/sequencer.c b/sequencer.c
> index 5f22b7cd377..f5d89abdc5e 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4202,7 +4202,7 @@ void todo_list_filter_update_refs(struct repository *r,
> if (!is_null_oid(&rec->after))
> continue;
>
> - for (j = 0; !found && j < todo_list->total_nr; j++) {
> + for (j = 0; !found && j < todo_list->nr; j++) {
> struct todo_item *item = &todo_list->items[j];
The .total_nr member could be smaller (because it does not count
noops) or larger (because it counts already done steps that are not
in the items[]) than the .nr member, and the old code could have
made out-of-bounds access into the items[] array. It is now
corrected. Excellent.
> const char *arg = todo_list->buf.buf + item->arg_offset;
>
> @@ -4232,7 +4232,7 @@ void todo_list_filter_update_refs(struct repository *r,
> * For each todo_item, check if its ref is in the update_refs list.
> * If not, then add it as an un-updated ref.
> */
> - for (i = 0; i < todo_list->total_nr; i++) {
> + for (i = 0; i < todo_list->nr; i++) {
> struct todo_item *item = &todo_list->items[i];
> const char *arg = todo_list->buf.buf + item->arg_offset;
> int j, found = 0;
Ditto. Will queue.
Thanks.
next prev parent reply other threads:[~2023-05-10 23:25 UTC|newest]
Thread overview: 11+ messages / expand[flat|nested] mbox.gz Atom feed top
2023-05-10 22:55 [PATCH 0/2] Fix two rebase bugs related to total_nr Johannes Schindelin via GitGitGadget
2023-05-10 22:55 ` [PATCH 1/2] rebase --update-refs: fix loops Johannes Schindelin via GitGitGadget
2023-05-10 23:25 ` Junio C Hamano [this message]
2023-05-10 22:55 ` [PATCH 2/2] rebase -r: fix the total number shown in the progress Johannes Schindelin via GitGitGadget
2023-05-10 23:39 ` Junio C Hamano
2023-05-11 14:13 ` Phillip Wood
2023-05-11 18:08 ` Junio C Hamano
2023-05-13 8:11 ` [PATCH v2 0/2] Fix two rebase bugs related to total_nr Johannes Schindelin via GitGitGadget
2023-05-13 8:11 ` [PATCH v2 1/2] rebase --update-refs: fix loops Johannes Schindelin via GitGitGadget
2023-05-13 8:11 ` [PATCH v2 2/2] rebase -r: fix the total number shown in the progress Johannes Schindelin via GitGitGadget
2023-05-14 17:59 ` [PATCH v2 0/2] Fix two rebase bugs related to total_nr Phillip Wood
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=xmqqcz3722gx.fsf@gitster.g \
--to=gitster@pobox.com \
--cc=derrickstolee@github.com \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=johannes.schindelin@gmx.de \
/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 read-only IMAP folder(s) and NNTP newsgroup(s).