patch, 5 partsrebase -i: a couple of small improvements

> On Wed, Sep 8, 2021 at 9:41 AM Phillip Wood via GitGitGadget
> <gitgitgadget@gmail.com> wrote:
>> This code is heavily indented and obscures the high level logic within
>> the loop. Lets move it to its own function before modifying it in the
>> next commit.
> 
> s/Lets/Let's/ ... or just drop "Let's" altogether and start with "Move".

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> This code is heavily indented and obscures the high level logic within
> the loop. Lets move it to its own function before modifying it in the
> next commit.
>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c | 38 ++++++++++++++++++++++++--------------
>  1 file changed, 24 insertions(+), 14 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 7f07cd00f3f..a248c886c27 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4254,6 +4254,27 @@ static int stopped_at_head(struct repository *r)
>
>  }
>
> +static int reread_todo_if_changed(struct repository *r,
> +				  struct todo_list *todo_list,
> +				  struct replay_opts *opts)
> +{
> +	struct stat st;
> +
> +	if (stat(get_todo_path(opts), &st)) {
> +		return error_errno(_("could not stat '%s'"),
> +				   get_todo_path(opts));
> +	} else if (match_stat_data(&todo_list->stat, &st)) {
> +		/* Reread the todo file if it has changed. */
> +		todo_list_release(todo_list);
> +		if (read_populate_todo(r, todo_list, opts))
> +			return -1; /* message was printed */
> +		/* `current` will be incremented on return */
> +		todo_list->current = -1;
> +	}
> +
> +	return 0;
> +}
> +
>  static const char rescheduled_advice[] =
>  N_("Could not execute the todo command\n"
>  "\n"
> @@ -4433,20 +4454,9 @@ static int pick_commits(struct repository *r,
>  							item->commit,
>  							arg, item->arg_len,
>  							opts, res, 0);
> -		} else if (is_rebase_i(opts) && check_todo && !res) {
> -			struct stat st;
> -
> -			if (stat(get_todo_path(opts), &st)) {
> -				res = error_errno(_("could not stat '%s'"),
> -						  get_todo_path(opts));
> -			} else if (match_stat_data(&todo_list->stat, &st)) {
> -				/* Reread the todo file if it has changed. */
> -				todo_list_release(todo_list);
> -				if (read_populate_todo(r, todo_list, opts))
> -					res = -1; /* message was printed */
> -				/* `current` will be incremented below */
> -				todo_list->current = -1;
> -			}
> +		} else if (is_rebase_i(opts) && check_todo && !res &&
> +			   reread_todo_if_changed(r, todo_list, opts)) {
> +			return -1;

>  		}
>
>  		todo_list->current++;
> --
> gitgitgadget
>
>

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> 54fd3243da ("rebase -i: reread the todo list if `exec` touched it",
> 2017-04-26) sought to reread the todo list after running an exec
> command only if it had been changed. To accomplish this it checks the
> stat data of the todo list after running an exec command to see if it
> has changed. Unfortunately there are two problems, firstly the
> implementation is buggy we actually reread the list after each exec
> which is quadratic in the number of commit lookups and secondly the
> design is predicated on using nanosecond time stamps which are not the
> default.
>
> The implementation bug stems from the fact that we write a new todo
> list to disk before running each command but do not update the stat
> data to reflect this[1].
>
> The design problem is that it is possible for the user to edit the
> todo list without changing its size or inode which means we have to
> rely on the mtime to tell us if it has changed. Unfortunately unless
> git is built with USE_NSEC it is possible for the original and edited
> list to share the same mtime.
>
> Ideally "git rebase --edit-todo" would set a flag that we would then
> check in sequencer.c. Unfortunately this is approach will not work as
> there are scripts in the wild that write to the todo list directly
> without running "git rebase --edit-todo". Instead of relying on stat
> data this patch simply reads the possibly edited todo list and
> compares it to the original with memcmp(). This is much faster than
> reparsing the todo list each time. This patch reduces the time to run
>
>    git rebase -r -xtrue v2.32.0~100 v2.32.0
>
> which runs 419 exec commands by 6.6%. For comparison fixing the
> implementation bug in stat based approach reduces the time by a
> further 1.4% and is indistinguishable from never rereading the todo
> list.

>
> [1] https://lore.kernel.org/git/20191125131833.GD23183@szeder.dev/
>
> Reported-by: SZEDER Gábor <szeder.dev@gmail.com>
> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c | 19 ++++++++-----------
>  sequencer.h |  1 -
>  2 files changed, 8 insertions(+), 12 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index a248c886c27..d51440ddcd9 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2671,7 +2671,6 @@ static int read_populate_todo(struct repository *r,
>  			      struct todo_list *todo_list,
>  			      struct replay_opts *opts)
>  {
> -	struct stat st;
>  	const char *todo_file = get_todo_path(opts);
>  	int res;
>
> @@ -2679,11 +2678,6 @@ static int read_populate_todo(struct repository *r,
>  	if (strbuf_read_file_or_whine(&todo_list->buf, todo_file) < 0)
>  		return -1;
>
> -	res = stat(todo_file, &st);
> -	if (res)
> -		return error(_("could not stat '%s'"), todo_file);
> -	fill_stat_data(&todo_list->stat, &st);
> -
>  	res = todo_list_parse_insn_buffer(r, todo_list->buf.buf, todo_list);
>  	if (res) {
>  		if (is_rebase_i(opts))
> @@ -4258,12 +4252,14 @@ static int reread_todo_if_changed(struct repository *r,
>  				  struct todo_list *todo_list,
>  				  struct replay_opts *opts)
>  {
> -	struct stat st;
> +	int offset;
> +	struct strbuf buf = STRBUF_INIT;
>
> -	if (stat(get_todo_path(opts), &st)) {
> -		return error_errno(_("could not stat '%s'"),
> -				   get_todo_path(opts));
> -	} else if (match_stat_data(&todo_list->stat, &st)) {
> +	if (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)
> +		return -1;
> +	offset = get_item_line_offset(todo_list, todo_list->current + 1);
> +	if (buf.len != todo_list->buf.len - offset ||
> +	    memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {
>  		/* Reread the todo file if it has changed. */
>  		todo_list_release(todo_list);
>  		if (read_populate_todo(r, todo_list, opts))
> @@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,
>  		/* `current` will be incremented on return */
>  		todo_list->current = -1;
>  	}
> +	strbuf_release(&buf);
>
>  	return 0;
>  }
> diff --git a/sequencer.h b/sequencer.h
> index d57d8ea23d7..cdeb0c6be47 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -116,7 +116,6 @@ struct todo_list {
>  	struct todo_item *items;
>  	int nr, alloc, current;
>  	int done_nr, total_nr;
> -	struct stat_data stat;
>  };
>
>  #define TODO_LIST_INIT { STRBUF_INIT }
> --
> gitgitgadget
>
>

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
> 
> The "apply" based rebase has avoided forking git checkout since
> ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
> 2018-08-07). The code that handles the checkout was moved into libgit
> by b309a97108 ("reset: extract reset_head() from rebase", 2020-04-07)
> so lets start using it for the "merge" based rebase as well. This
> opens the way for us to stop calling the post-checkout hook in the
> future.
> 

> Hi Phillip,
> 
> Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> The "apply" based rebase has avoided forking git checkout since
>> ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
>> 2018-08-07). The code that handles the checkout was moved into libgit
>> by b309a97108 ("reset: extract reset_head() from rebase", 2020-04-07)
>> so lets start using it for the "merge" based rebase as well. This
>> opens the way for us to stop calling the post-checkout hook in the
>> future.
>>
> 
> While in general I think it's a good thing to avoid forking, this change
> might result in behavioral differences. Any config that affects
> 'git checkout' but not the internal 'reset.c::reset_head' function might
> play a role in the rebase UX.
> 
> One that immediately came to mind is 'submodule.recurse'. This initial 
> 'onto'
> checkout was pretty much the only part of 'git rebase' that did 
> something useful
> for submodules, so it's kind of sad to see it regress. 

> someone
> takes the time to implement 'git rebase --recurse-submodules' and makes 
> sure *all*
> code paths that touch the working tree pay attention to this flag, and 
> that will
> probably necessitate 'git merge --recurse-submodules' first because of 
> the 'merge'
> backend... as far as I'm aware it's on Emily's list [1], it's also on 
> mine but
> I don't know when I'll get the time.]
> 
> Anyway, I'm not saying that we should not do what this patch is 
> proposing, but
> I think caveats such as that should be documented in the commit message, 

> and maybe
> an audit of other configs that might results in behavioural differences 
> should be done.
> 
> Thanks,
> 
> Philippe.
> 
> [1] 
> https://lore.kernel.org/git/YHofmWcIAidkvJiD@google.com/t/#m0229af9183a84c2367f21e82adfbd21f08aa4437 
> 

> Hi Philippe
> 
> On 08/09/2021 19:14, Philippe Blain wrote:
>> Hi Phillip,
>> 
>> Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :
>>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>> 
>>> The "apply" based rebase has avoided forking git checkout since 
>>> ac7f467fef ("builtin/rebase: support running "git rebase
>>> <upstream>"", 2018-08-07). The code that handles the checkout was
>>> moved into libgit by b309a97108 ("reset: extract reset_head()
>>> from rebase", 2020-04-07) so lets start using it for the "merge"
>>> based rebase as well. This opens the way for us to stop calling
>>> the post-checkout hook in the future.
>>> 
>> 
>> While in general I think it's a good thing to avoid forking, this
>> change might result in behavioral differences. Any config that
>> affects 'git checkout' but not the internal 'reset.c::reset_head'
>> function might play a role in the rebase UX.
>> 
>> One that immediately came to mind is 'submodule.recurse'. This
>> initial 'onto' checkout was pretty much the only part of 'git
>> rebase' that did something useful for submodules, so it's kind of
>> sad to see it regress.
> 
> Thanks for pointing that out. As a non-submodule user my question
> would be is it actually useful for the initial checkout to work that
> way if the rest of rebase (and the checkout for the am backend)
> ignores submodules? reset.c::reset_head() just uses unpack trees like
> checkout so if rebase read 'submodule.recurse' then reset_head()
> would work like 'git checkout' and also 'git rebase --abort' and the
> "reset" command in the todo list would start checking out submodules.
> I'm reluctant to do that until the merge backend also handles
> submodules unless there is a good reason that such partial submodule
> support would help submodule users.

>>> While in general I think it's a good thing to avoid forking, this
>>> change might result in behavioral differences. Any config that
>>> affects 'git checkout' but not the internal 'reset.c::reset_head'
>>> function might play a role in the rebase UX.
>>>
>>> One that immediately came to mind is 'submodule.recurse'. This
>>> initial 'onto' checkout was pretty much the only part of 'git
>>> rebase' that did something useful for submodules, so it's kind of
>>> sad to see it regress.
>>
>> Thanks for pointing that out. As a non-submodule user my question
>> would be is it actually useful for the initial checkout to work that
>> way if the rest of rebase (and the checkout for the am backend)
>> ignores submodules? reset.c::reset_head() just uses unpack trees like
>> checkout so if rebase read 'submodule.recurse' then reset_head()
>> would work like 'git checkout' and also 'git rebase --abort' and the
>> "reset" command in the todo list would start checking out submodules.

>> I'm reluctant to do that until the merge backend also handles
>> submodules unless there is a good reason that such partial submodule
>> support would help submodule users.
> 
> Yeah, it's not that useful, I have to admit; it can also be very confusing
> since some parts of rebase are affected, and some not. For example, any 
> time
> the rebase stops, like for 'edit', 'break', and when there are 
> conflicts, the
> submodules are not updated. So I think a full solution is better than a 
> partial
> solution; in the meantime I'm thinking the change you are proposing 
> would actually
> be less confusing, even if it slightly changes behaviour...
> 
> As an aside, I *think* reading submodule.recurse in rebase like it's 
> done in checkout
> et al., i.e. something like this:
> 
> diff --git a/builtin/rebase.c b/builtin/rebase.c
> index 33e0961900..125ec907e4 100644
> --- a/builtin/rebase.c
> +++ b/builtin/rebase.c
> @@ -26,6 +26,7 @@
>   #include "rerere.h"
>   #include "branch.h"
>   #include "sequencer.h"
> +#include "submodule.h"
>   #include "rebase-interactive.h"
>   #include "reset.h"
> 
> @@ -1106,6 +1107,9 @@ static int rebase_config(const char *var, const 
> char *value, void *data)
>           return git_config_string(&opts->default_backend, var, value);
>       }
> 
> +    if (!strcmp(var, "submodule.recurse"))
> +        return git_default_submodule_config(var, value, data);

>       return git_default_config(var, value, data);
>   }
> 
> 
> would actually also affect the merges
> performed during the rebase, since that would affect the "global" state 
> in submodule.c.
> I hacked exactly that the other day but did not test extensively...

>
> Hi Philippe
>
> On 09/09/2021 13:40, Philippe Blain wrote:
> >>> While in general I think it's a good thing to avoid forking, this
> >>> change might result in behavioral differences. Any config that
> >>> affects 'git checkout' but not the internal 'reset.c::reset_head'
> >>> function might play a role in the rebase UX.
> >>>
> >>> One that immediately came to mind is 'submodule.recurse'. This
> >>> initial 'onto' checkout was pretty much the only part of 'git
> >>> rebase' that did something useful for submodules, so it's kind of
> >>> sad to see it regress.
> >>
> >> Thanks for pointing that out. As a non-submodule user my question
> >> would be is it actually useful for the initial checkout to work that
> >> way if the rest of rebase (and the checkout for the am backend)
> >> ignores submodules? reset.c::reset_head() just uses unpack trees like
> >> checkout so if rebase read 'submodule.recurse' then reset_head()
> >> would work like 'git checkout' and also 'git rebase --abort' and the
> >> "reset" command in the todo list would start checking out submodules.
>
> it would also affect fast-forwards
>
> >> I'm reluctant to do that until the merge backend also handles
> >> submodules unless there is a good reason that such partial submodule
> >> support would help submodule users.
> >
> > Yeah, it's not that useful, I have to admit; it can also be very confusing
> > since some parts of rebase are affected, and some not. For example, any
> > time
> > the rebase stops, like for 'edit', 'break', and when there are
> > conflicts, the
> > submodules are not updated. So I think a full solution is better than a
> > partial
> > solution; in the meantime I'm thinking the change you are proposing
> > would actually
> > be less confusing, even if it slightly changes behaviour...
> >
> > As an aside, I *think* reading submodule.recurse in rebase like it's
> > done in checkout
> > et al., i.e. something like this:
> >
> > diff --git a/builtin/rebase.c b/builtin/rebase.c
> > index 33e0961900..125ec907e4 100644
> > --- a/builtin/rebase.c
> > +++ b/builtin/rebase.c
> > @@ -26,6 +26,7 @@
> >   #include "rerere.h"
> >   #include "branch.h"
> >   #include "sequencer.h"
> > +#include "submodule.h"
> >   #include "rebase-interactive.h"
> >   #include "reset.h"
> >
> > @@ -1106,6 +1107,9 @@ static int rebase_config(const char *var, const
> > char *value, void *data)
> >           return git_config_string(&opts->default_backend, var, value);
> >       }
> >
> > +    if (!strcmp(var, "submodule.recurse"))
> > +        return git_default_submodule_config(var, value, data);
>
> That looks about right to me though I think it would be safer to call
> git_default_submodule_config() for submodule.* rather than just
> submodule.recurse
>
> >       return git_default_config(var, value, data);
> >   }
> >
> >
> > would actually also affect the merges
> > performed during the rebase, since that would affect the "global" state
> > in submodule.c.
> > I hacked exactly that the other day but did not test extensively...
>
> merge-ort.c:checkout() which is used by merge_switch_to_result() uses
> unpack_trees() so it will pick up the global state and hopefully should
> just work (cc'ing Elijah to check as I didn't look what happens when
> there are conflicts).

> merge-recursive.c:update_file_flags() does this
> when updating the work tree
>
>         if (S_ISGITLINK(contents->mode)) {
>
>                  /*
>
>                   * We may later decide to recursively descend into
>
>                   * the submodule directory and update its index
>
>                   * and/or work tree, but we do not do that now.
>
>                   */
>
>                  update_wd = 0;
>
>                  goto update_index;
>
>         }
>
>
>
> so it looks like it does not update the submodule directory. Given
> merge-ort is now the default perhaps it's time for rebase (and
> cherry-pick/revert) to start reading the submodule config settings (we
> parse the config before we know if we'll be using merge-ort so I don't
> know how easy it would be to only parse the submodule settings if we are
> using merge-ort).

>> merge-recursive.c:update_file_flags() does this
>> when updating the work tree
>>
>>          if (S_ISGITLINK(contents->mode)) {
>>
>>                   /*
>>
>>                    * We may later decide to recursively descend into
>>
>>                    * the submodule directory and update its index
>>
>>                    * and/or work tree, but we do not do that now.
>>
>>                    */
>>
>>                   update_wd = 0;
>>
>>                   goto update_index;
>>
>>          }
>>
>>
>>
>> so it looks like it does not update the submodule directory. Given
>> merge-ort is now the default perhaps it's time for rebase (and
>> cherry-pick/revert) to start reading the submodule config settings (we
>> parse the config before we know if we'll be using merge-ort so I don't
>> know how easy it would be to only parse the submodule settings if we are
>> using merge-ort).
> 
> I'd just parse any needed config in all cases.  The submodule settings
> aren't going to hurt merge-recursive; it'll just ignore them.  (Or are
> you worried about a mix-and-match of rebase calling both checkout and
> merge code doing weird things, and you'd rather not have the checkout
> bits update submodules if the merges won't?)
> 

> [...]
>> merge-ort.c:checkout() which is used by merge_switch_to_result() uses
>> unpack_trees() so it will pick up the global state and hopefully should
>> just work (cc'ing Elijah to check as I didn't look what happens when
>> there are conflicts).
> 
> Yep, merge-ort was designed to just piggy back on checkout code.  The
> checkout() function was basically just code lifted from
> builtin/checkout.c.  Using that code means that merges now also
> benefit from all the special working tree handling that is encoded
> into git-checkout -- whether that's parallel checkout, submodule
> handling, tricky D/F switches or symlink handling, etc.  In contrast
> to merge-recursive, it does not need hundreds and hundreds of lines of
> special worktree updating code sprayed all over the codebase.
> 
> Conflicts are not special in this regard; merge-ort creates a tree
> which has files that include conflict markers, and then merge-ort
> calls checkout() to switch the working copy over to that tree.
> 
> The only issue conflicts present for merge-ort, is that AFTER it has
> checked out that special tree with conflict markers, it then has to go
> and touch up the index afterwards to replace the entries for
> conflicted files with multiple higher order stages.  (You could say
> that merge-recursive is "index-first", since its design focuses on the
> index -- updating it first and then figuring out everything else like
> updating the working tree with special code afterwards.  In contrast,
> merge-ort ignores the index entirely until the very end -- after a new
> merge tree is created and after the working tree is updated.)

>> merge-recursive.c:update_file_flags() does this
>> when updating the work tree
>>
>>          if (S_ISGITLINK(contents->mode)) {
>>                   /*
>>                    * We may later decide to recursively descend into
>>                    * the submodule directory and update its index
>>                    * and/or work tree, but we do not do that now.
>>                    */
>>                   update_wd = 0;
>>                   goto update_index;
>>          }
>>
>> so it looks like it does not update the submodule directory. Given
>> merge-ort is now the default perhaps it's time for rebase (and
>> cherry-pick/revert) to start reading the submodule config settings (we
>> parse the config before we know if we'll be using merge-ort so I don't
>> know how easy it would be to only parse the submodule settings if we are
>> using merge-ort).
> 
> I'd just parse any needed config in all cases.  The submodule settings
> aren't going to hurt merge-recursive; it'll just ignore them.  (Or are
> you worried about a mix-and-match of rebase calling both checkout and
> merge code doing weird things, and you'd rather not have the checkout
> bits update submodules if the merges won't?)

> Hi Phillip,
>
> Le 2021-09-08 à 09:41, Phillip Wood via GitGitGadget a écrit :
> > From: Phillip Wood <phillip.wood@dunelm.org.uk>
> >
> > The "apply" based rebase has avoided forking git checkout since
> > ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
> > 2018-08-07). The code that handles the checkout was moved into libgit
> > by b309a97108 ("reset: extract reset_head() from rebase", 2020-04-07)
> > so lets start using it for the "merge" based rebase as well. This
> > opens the way for us to stop calling the post-checkout hook in the
> > future.
> >
>
> While in general I think it's a good thing to avoid forking, this change
> might result in behavioral differences. Any config that affects
> 'git checkout' but not the internal 'reset.c::reset_head' function might
> play a role in the rebase UX.
>
> One that immediately came to mind is 'submodule.recurse'. This initial
> 'onto' checkout was pretty much the only part of 'git rebase' that did
> something useful for submodules, so it's kind of sad to see it regress.
> [That is, until someone takes the time to implement 'git rebase
> --recurse-submodules' and makes sure *all* code paths that touch the
> working tree pay attention to this flag, and that will probably
> necessitate 'git merge --recurse-submodules' first because of the
> 'merge' backend... as far as I'm aware it's on Emily's list [1], it's
> also on mine but I don't know when I'll get the time.]

> Hi Philippe,
> 
> On Wed, 8 Sep 2021, Philippe Blain wrote:
> 
>> Anyway, I'm not saying that we should not do what this patch is
>> proposing, but I think caveats such as that should be documented in the
>> commit message, and maybe an audit of other configs that might results
>> in behavioural differences should be done.
> 
> Since this is already a bug in the `apply` backend, it would be even
> better to follow-up with a fix, hint, hint, nudge, nudge ;-)
> 

> Le 2021-09-09 à 06:53, Johannes Schindelin a écrit :
> >
> > On Wed, 8 Sep 2021, Philippe Blain wrote:
> >
> > > Anyway, I'm not saying that we should not do what this patch is
> > > proposing, but I think caveats such as that should be documented in the
> > > commit message, and maybe an audit of other configs that might results
> > > in behavioural differences should be done.
> >
> > Since this is already a bug in the `apply` backend, it would be even
> > better to follow-up with a fix, hint, hint, nudge, nudge ;-)
>
> I'm not sure I understand what you are saying.

> Or do you mean something else?
>
> Thanks,
> Philippe.
>
>

> Le 2021-09-09 à 06:53, Johannes Schindelin a écrit :
> >
> > On Wed, 8 Sep 2021, Philippe Blain wrote:
> >
> > > Anyway, I'm not saying that we should not do what this patch is
> > > proposing, but I think caveats such as that should be documented in the
> > > commit message, and maybe an audit of other configs that might results
> > > in behavioural differences should be done.
> >
> > Since this is already a bug in the `apply` backend, it would be even
> > better to follow-up with a fix, hint, hint, nudge, nudge ;-)
> >
>
> I'm not sure I understand what you are saying. The fact that 'rebase'
> does not pay attention to 'submodule.recurse' is not a bug in my opinion,
> it's just a limitation of the current code... Or do you mean something else?

> Hi Philippe,
> 
> On Thu, 9 Sep 2021, Philippe Blain wrote:
> 
>> Le 2021-09-09 à 06:53, Johannes Schindelin a écrit :
>>>
>>> On Wed, 8 Sep 2021, Philippe Blain wrote:
>>>
>>>> Anyway, I'm not saying that we should not do what this patch is
>>>> proposing, but I think caveats such as that should be documented in the
>>>> commit message, and maybe an audit of other configs that might results
>>>> in behavioural differences should be done.
>>>
>>> Since this is already a bug in the `apply` backend, it would be even
>>> better to follow-up with a fix, hint, hint, nudge, nudge ;-)
>>>
>>
>> I'm not sure I understand what you are saying. The fact that 'rebase'
>> does not pay attention to 'submodule.recurse' is not a bug in my opinion,
>> it's just a limitation of the current code... Or do you mean something else?
> 
> I must have misunderstood you. I thought you were saying that Phillip's
> patch introduces the regression where `submodule.recurse` is no longer
> respected.
> 
> Ciao,
> Dscho
> 

>
> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> The "apply" based rebase has avoided forking git checkout since
> ac7f467fef ("builtin/rebase: support running "git rebase <upstream>"",
> 2018-08-07). The code that handles the checkout was moved into libgit
> by b309a97108 ("reset: extract reset_head() from rebase", 2020-04-07)
> so lets start using it for the "merge" based rebase as well. This
> opens the way for us to stop calling the post-checkout hook in the
> future.

> Signed-off-by: Phillip Wood <phillip.wood@dunelm.org.uk>
> ---
>  sequencer.c | 29 ++++-------------------------
>  1 file changed, 4 insertions(+), 25 deletions(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index d51440ddcd9..1a9dbc70d3c 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -4192,42 +4192,21 @@ int apply_autostash_oid(const char *stash_oid)
>         return apply_save_autostash_oid(stash_oid, 1);
>  }
>
> -static int run_git_checkout(struct repository *r, struct replay_opts *opts,
> -                           const char *commit, const char *action)
> -{
> -       struct child_process cmd = CHILD_PROCESS_INIT;
> -       int ret;
> -
> -       cmd.git_cmd = 1;
> -
> -       strvec_push(&cmd.args, "checkout");
> -       strvec_push(&cmd.args, commit);
> -       strvec_pushf(&cmd.env_array, GIT_REFLOG_ACTION "=%s", action);
> -
> -       if (opts->verbose)
> -               ret = run_command(&cmd);
> -       else
> -               ret = run_command_silent_on_success(&cmd);
> -
> -       if (!ret)
> -               discard_index(r->index);
> -
> -       return ret;
> -}
> -
>  static int checkout_onto(struct repository *r, struct replay_opts *opts,
>                          const char *onto_name, const struct object_id *onto,
>                          const struct object_id *orig_head)
>  {
>         const char *action = reflog_message(opts, "start", "checkout %s", onto_name);
>
> -       if (run_git_checkout(r, opts, oid_to_hex(onto), action)) {
> +       if (reset_head(r, onto, "checkout", NULL, RESET_HEAD_DETACH |
> +                      RESET_ORIG_HEAD | RESET_HEAD_RUN_POST_CHECKOUT_HOOK,
> +                      NULL, action, "rebase")) {
>                 apply_autostash(rebase_path_autostash());
>                 sequencer_remove_state(opts);
>                 return error(_("could not detach HEAD"));
>         }
>
> -       return update_ref(NULL, "ORIG_HEAD", orig_head, NULL, 0, UPDATE_REFS_MSG_ON_ERR);
> +       return 0;
>  }
>
>  static int stopped_at_head(struct repository *r)
> --
> gitgitgadget

> Hi Phillip,
> 
> On Wed, 8 Sep 2021, Phillip Wood via GitGitGadget wrote:
> 
>> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>>
>> Now that we use reset_head() we don't need to pass orig_head around.
> 
> Does this indicate a change in behavior? When we call `update_ref()` with
> the original `HEAD`, we get some version of safety in that it will fail if
> anything changed the ref in an unexpected way in the meantime.

> +	if (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)
> +		return -1;
> +	offset = get_item_line_offset(todo_list, todo_list->current + 1);
> +	if (buf.len != todo_list->buf.len - offset ||
> +	    memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {
>  		/* Reread the todo file if it has changed. */
>  		todo_list_release(todo_list);
>  		if (read_populate_todo(r, todo_list, opts))

> @@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,
>  		/* `current` will be incremented on return */
>  		todo_list->current = -1;
>  	}
> +	strbuf_release(&buf);
>  
>  	return 0;
>  }
> diff --git a/sequencer.h b/sequencer.h
> index d57d8ea23d7..cdeb0c6be47 100644
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -116,7 +116,6 @@ struct todo_list {
>  	struct todo_item *items;
>  	int nr, alloc, current;
>  	int done_nr, total_nr;
> -	struct stat_data stat;

> "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
>> .... Instead of relying on stat
>> data this patch simply reads the possibly edited todo list and
>> compares it to the original with memcmp(). This is much faster than
>> reparsing the todo list each time.
> 
> Nice.  Is that an egg of Columbus or what ;-)
> 
>> +	if (strbuf_read_file_or_whine(&buf, get_todo_path(opts)) < 0)
>> +		return -1;
>> +	offset = get_item_line_offset(todo_list, todo_list->current + 1);
>> +	if (buf.len != todo_list->buf.len - offset ||
>> +	    memcmp(buf.buf, todo_list->buf.buf + offset, buf.len)) {
>>   		/* Reread the todo file if it has changed. */
>>   		todo_list_release(todo_list);
>>   		if (read_populate_todo(r, todo_list, opts))
> 
> As we already have the contents of hte file in the buffer, we could
> further refactor the code around read_populate_todo() to tell it not
> to reopen and reread the rebase-todo file (which risks toctou race),
> but that is OK for now, I would think.

>> @@ -4271,6 +4267,7 @@ static int reread_todo_if_changed(struct repository *r,
>>   		/* `current` will be incremented on return */
>>   		todo_list->current = -1;
>>   	}
>> +	strbuf_release(&buf);
>>   
>>   	return 0;
>>   }
>> diff --git a/sequencer.h b/sequencer.h
>> index d57d8ea23d7..cdeb0c6be47 100644
>> --- a/sequencer.h
>> +++ b/sequencer.h
>> @@ -116,7 +116,6 @@ struct todo_list {
>>   	struct todo_item *items;
>>   	int nr, alloc, current;
>>   	int done_nr, total_nr;
>> -	struct stat_data stat;
> 
> Good riddance ;-)