From: Phillip Wood <phillip.wood123@gmail.com>
Date: Tue, 07 Jun 2022 10:25:01 GMT
Subject: Re: [PATCH 3/4] rebase: add --update-refs option
Message-ID: <c9a5369a-8c87-0ee8-bed7-16b2cd58717b@gmail.com>
In-Reply-To: <23fa663886405596370e5fc95f679ef299f0725c.1654263472.git.gitgitgadget@gmail.com>

Hi Stolee

Just a couple of minor comments.

On 03/06/2022 14:37, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <derrickstolee@github.com>
> [...]
> diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
> index 262fb01aec0..866554fc978 100644
> --- a/Documentation/git-rebase.txt
> +++ b/Documentation/git-rebase.txt
> @@ -609,6 +609,13 @@ provided. Otherwise an explicit `--no-reschedule-failed-exec` at the
>   start would be overridden by the presence of
>   `rebase.rescheduleFailedExec=true` configuration.
>   
> +--update-refs::
> +--no-update-refs::
> +	Automatically force-update any branches that point to commits that
> +	are being rebased. Any branches that are checked out in a worktree
> +	or point to a `squash! ...` or `fixup! ...` commit are not updated
> +	in this way.
> +
>   INCOMPATIBLE OPTIONS
>   --------------------

We should add --update-refs to the list of options that are incompatible 
with --apply.

> diff --git a/sequencer.c b/sequencer.c
> index 8c3ed3532ac..d6151af9849 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -35,6 +35,8 @@
>   #include "commit-reach.h"
>   #include "rebase-interactive.h"
>   #include "reset.h"
> +#include "branch.h"
> +#include "log-tree.h"
>   
>   #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
>   
> @@ -5603,10 +5605,104 @@ static int skip_unnecessary_picks(struct repository *r,
>   	return 0;
>   }
>   
> +struct todo_add_branch_context {
> +	struct todo_list new_list;

Rather than using a struct todo_list I think it would be simpler overall 
to add
	struct todo_list_item *item;
	size_t nr, alloc;

instead, as I found it confusing that we were (correctly) using the 
strbuf of the old list when adding the update-ref line to the new list.

> +	struct strbuf *buf;
> +	struct commit *commit;
> +};
> [...]
> +	for (i = 0; i < todo_list->nr; ) {
> +		struct todo_item *item = &todo_list->items[i];
> +
> +		do {
> +			/* insert ith item into new list */
> +			ALLOC_GROW(ctx.new_list.items,
> +				   ctx.new_list.nr + 1,
> +				   ctx.new_list.alloc);
> +
> +			memcpy(&ctx.new_list.items[ctx.new_list.nr++],
> +			       &todo_list->items[i],
> +			       sizeof(struct todo_item));

May be
	ctx.new_list.items[ctx.new_list.nr++] = todo_list->items[i++];
would be clearer

Best Wishes

Phillip