git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org,
	Johannes Schindelin <Johannes.Schindelin@gmx.de>,
	Stefan Haller <lists@haller-berlin.de>,
	Phillip Wood <phillip.wood@dunelm.org.uk>
Subject: Re: [PATCH] rebase -i: do not update "done" when rescheduling command
Date: Mon, 20 Mar 2023 10:46:43 -0700	[thread overview]
Message-ID: <xmqq4jqfmi2k.fsf@gitster.g> (raw)
In-Reply-To: <pull.1492.git.1679237337683.gitgitgadget@gmail.com> (Phillip Wood via GitGitGadget's message of "Sun, 19 Mar 2023 14:48:57 +0000")

"Phillip Wood via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Phillip Wood <phillip.wood@dunelm.org.uk>
>
> As the sequencer executes todo commands it appends them to
> .git/rebase-merge/done. This file is used by "git status" to show the
> recently executed commands. Unfortunately when a command is rescheduled
> the command preceding it is erroneously appended to the "done" file.
> This means that when rebase stops after rescheduling "pick B" the "done"
> file contains
>
> 	pick A
> 	pick B
> 	pick A
>
> instead of
>
> 	pick A
> 	pick B

Here it may not be clear what you meant with the verb "reschedule"
to those who weren't closely following the previous discussion that
led to this fix.

Is it the same as "the command attempted to execute a step, couldn't
complete it (e.g. due to conflicts), and gave control to the end
user until they say 'git rebase --continue'"?  What cases, other
than interrupted step due to conflicts, involve "rescheduling"?

> Note that the rescheduled command will still be appended to the "done"
> file again when it is successfully executed. Arguably it would be better
> not to do that but fixing it would be more involved.

And without quite understanding what "reschedule" refers to, it is
unclear why it is even arguable---it is perfectly sensible that a
command that is rescheduled (hence not yet done) would not be sent
to 'done'.  If a command that was once rescheduled (hence it wasn't
finished initially) gets finished now, shouldn't it be sent to
'done'?  It is unclear why is it better not to.

> -static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
> +static int save_todo(struct todo_list *todo_list, struct replay_opts *opts,
> +		     int reschedule)
>  {

OK, all callers to save_todo() are in pick_commits() that knows what
the value of "reschedule" is, and it is passed down to this helper ...

> @@ -3389,7 +3390,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
>  	 * rebase -i writes "git-rebase-todo" without the currently executing
>  	 * command, appending it to "done" instead.
>  	 */
> -	if (is_rebase_i(opts))
> +	if (is_rebase_i(opts) && !reschedule)
>  		next++;
>  
>  	fd = hold_lock_file_for_update(&todo_lock, todo_path, 0);
> @@ -3402,7 +3403,7 @@ static int save_todo(struct todo_list *todo_list, struct replay_opts *opts)
>  	if (commit_lock_file(&todo_lock) < 0)
>  		return error(_("failed to finalize '%s'"), todo_path);
>  
> -	if (is_rebase_i(opts) && next > 0) {
> +	if (is_rebase_i(opts) && !reschedule && next > 0) {
>  		const char *done = rebase_path_done();
>  		int fd = open(done, O_CREAT | O_WRONLY | O_APPEND, 0666);
>  		int ret = 0;

... and the change here is quite straight-forward.  With reschedule,
we do not advance because by definition we haven't finished the
step yet.  OK.

> @@ -4648,7 +4649,7 @@ static int pick_commits(struct repository *r,
>  		const char *arg = todo_item_get_arg(todo_list, item);
>  		int check_todo = 0;
>  
> -		if (save_todo(todo_list, opts))
> +		if (save_todo(todo_list, opts, 0))
>  			return -1;

I wonder why we pass a hardcoded 0 here---shouldn't the value match
the local variable 'reschedule'? here?

The same question for the other two callers, but I admit that when
the second one is called, the local variable "reschedule" is not
set...

>  		if (is_rebase_i(opts)) {
>  			if (item->command != TODO_COMMENT) {
> @@ -4695,8 +4696,7 @@ static int pick_commits(struct repository *r,
>  							    todo_list->current),
>  				       get_item_line(todo_list,
>  						     todo_list->current));
> -				todo_list->current--;
> -				if (save_todo(todo_list, opts))
> +				if (save_todo(todo_list, opts, 1))
>  					return -1;

... yet we call the helper with reschedule set to 1.  Puzzled.

> @@ -4788,8 +4788,7 @@ static int pick_commits(struct repository *r,
>  			       get_item_line_length(todo_list,
>  						    todo_list->current),
>  			       get_item_line(todo_list, todo_list->current));
> -			todo_list->current--;
> -			if (save_todo(todo_list, opts))
> +			if (save_todo(todo_list, opts, 1))
>  				return -1;

At this point, reschedule is set and passing it instead of 1 would
be OK.

Thanks.

  parent reply	other threads:[~2023-03-20 17:53 UTC|newest]

Thread overview: 80+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2023-03-19 14:48 [PATCH] rebase -i: do not update "done" when rescheduling command Phillip Wood via GitGitGadget
2023-03-20  7:29 ` Stefan Haller
2023-03-20 17:46 ` Junio C Hamano [this message]
2023-03-24 10:50   ` Phillip Wood
2023-03-24 15:49     ` Junio C Hamano
2023-03-24 16:22       ` Phillip Wood
2023-03-27  7:04 ` Johannes Schindelin
2023-08-03 12:56   ` Phillip Wood
2023-08-23  8:54     ` Johannes Schindelin
2023-04-21 14:57 ` [PATCH v2 0/6] rebase -i: impove handling of failed commands Phillip Wood via GitGitGadget
2023-04-21 14:57   ` [PATCH v2 1/6] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-04-21 17:22     ` Junio C Hamano
2023-04-27 10:15       ` Phillip Wood
2023-04-21 14:57   ` [PATCH v2 2/6] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-04-21 19:01     ` Junio C Hamano
2023-04-27 10:17       ` Phillip Wood
2023-06-21 20:14     ` Glen Choo
2023-07-14 10:08       ` Phillip Wood
2023-07-14 16:51         ` Junio C Hamano
2023-07-17 15:39           ` Phillip Wood
2023-04-21 14:57   ` [PATCH v2 3/6] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-04-21 19:12     ` Eric Sunshine
2023-04-21 19:31     ` Junio C Hamano
2023-04-21 20:00       ` Phillip Wood
2023-04-21 21:21         ` Junio C Hamano
2023-04-21 14:57   ` [PATCH v2 4/6] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-04-21 19:14     ` Eric Sunshine
2023-04-21 21:05     ` Junio C Hamano
2023-06-21 20:35     ` Glen Choo
2023-04-21 14:57   ` [PATCH v2 5/6] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-06-21 20:49     ` Glen Choo
2023-07-25 15:42       ` Phillip Wood
2023-07-25 16:46         ` Glen Choo
2023-07-26 13:08           ` Phillip Wood
2023-07-26 17:48             ` Glen Choo
2023-07-28 13:19               ` Phillip Wood
2023-04-21 14:57   ` [PATCH v2 6/6] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-06-21 20:59     ` Glen Choo
2023-04-21 16:56   ` [PATCH v2 0/6] rebase -i: impove handling of failed commands Junio C Hamano
2023-06-21 20:07   ` Glen Choo
2023-08-01 15:23   ` [PATCH v3 0/7] " Phillip Wood via GitGitGadget
2023-08-01 15:23     ` [PATCH v3 1/7] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-08-01 17:22       ` Junio C Hamano
2023-08-01 18:42         ` Phillip Wood
2023-08-01 19:31           ` Junio C Hamano
2023-08-01 15:23     ` [PATCH v3 2/7] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-08-01 17:23       ` Junio C Hamano
2023-08-01 18:47         ` Phillip Wood
2023-08-01 15:23     ` [PATCH v3 3/7] sequencer: use rebase_path_message() Phillip Wood via GitGitGadget
2023-08-01 17:23       ` Junio C Hamano
2023-08-01 18:49         ` Phillip Wood
2023-08-02 22:02           ` Junio C Hamano
2023-08-01 15:23     ` [PATCH v3 4/7] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-08-23  8:55       ` Johannes Schindelin
2023-08-01 15:23     ` [PATCH v3 5/7] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-08-23  8:55       ` Johannes Schindelin
2023-09-04 14:31         ` Phillip Wood
2023-08-01 15:23     ` [PATCH v3 6/7] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-08-23  9:01       ` Johannes Schindelin
2023-09-04 14:37         ` Phillip Wood
2023-09-05 11:17           ` Johannes Schindelin
2023-09-05 14:57             ` Junio C Hamano
2023-09-05 15:25             ` Phillip Wood
2023-08-01 15:23     ` [PATCH v3 7/7] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-08-02 22:10     ` [PATCH v3 0/7] rebase -i: impove handling of failed commands Junio C Hamano
2023-08-03 13:06       ` Phillip Wood
2023-08-09 13:08       ` Phillip Wood
2023-08-07 20:16     ` Glen Choo
2023-08-09 10:06       ` Phillip Wood
2023-09-06 15:22     ` [PATCH v4 " Phillip Wood via GitGitGadget
2023-09-06 15:22       ` [PATCH v4 1/7] rebase -i: move unlink() calls Phillip Wood via GitGitGadget
2023-09-06 15:22       ` [PATCH v4 2/7] rebase -i: remove patch file after conflict resolution Phillip Wood via GitGitGadget
2023-09-06 15:22       ` [PATCH v4 3/7] sequencer: use rebase_path_message() Phillip Wood via GitGitGadget
2023-09-06 15:22       ` [PATCH v4 4/7] sequencer: factor out part of pick_commits() Phillip Wood via GitGitGadget
2023-09-06 15:22       ` [PATCH v4 5/7] rebase: fix rewritten list for failed pick Phillip Wood via GitGitGadget
2023-09-06 15:22       ` [PATCH v4 6/7] rebase --continue: refuse to commit after failed command Phillip Wood via GitGitGadget
2023-09-06 15:22       ` [PATCH v4 7/7] rebase -i: fix adding failed command to the todo list Phillip Wood via GitGitGadget
2023-09-06 21:01       ` [PATCH v4 0/7] rebase -i: impove handling of failed commands Junio C Hamano
2023-09-07  9:56       ` Johannes Schindelin
2023-09-07 20:33         ` Junio C Hamano

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

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq4jqfmi2k.fsf@gitster.g \
    --to=gitster@pobox.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=lists@haller-berlin.de \
    --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.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

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).