Re: [PATCH 7/7] sequencer: Remove sequencer state after final commit

[Jonathan Nieder <jrnieder@gmail.com>]

Ramkumar Ramachandra wrote:

> Since d3f4628e (revert: Remove sequencer state when no commits are
> pending, 2011-07-06), the sequencer removes the sequencer state before
> the final commit is actually completed.  This design is inherently
> flawed, as it will not allow the user to abort the sequencer operation
> at that stage.  Instead, make 'git commit' notify the sequencer after
> every successful commit; the sequencer then removes the state if no
> more instructions are pending.

Sorry, I'm getting lost in all the words.  I suspect you are saying
“d3f4628e was trying to solve such-and-such problem, but its fix was
problematic because it removes the data that a hypothetical "git
cherry-pick --abort" command would need to work.  Back out that
change and adopt the following instead.”

In particular, the above does not make it clear to me:

 - as a user, what effect will I notice after this change?
 - what problem does it solve?
 - does it have any downsides?

> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -26,6 +26,7 @@
>  #include "unpack-trees.h"
>  #include "quote.h"
>  #include "submodule.h"
> +#include "sequencer.h"
>  
>  static const char * const builtin_commit_usage[] = {
>  	"git commit [options] [--] <filepattern>...",
> @@ -1521,6 +1522,13 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  	unlink(git_path("MERGE_MODE"));
>  	unlink(git_path("SQUASH_MSG"));
>  
> +	/*
> +	 * Notify the sequencer that we're committing.  The routine
> +	 * removes the sequencer state if our commit just completed
> +	 * the last instruction.
> +	 */
> +	sequencer_notify_commit();
> +
>  	if (commit_index_files())
>  		die (_("Repository has been updated, but unable to write\n"
>  		     "new_index file. Check that disk is not full or quota is\n"

The function name (..._notify_commit()) does not seem very intuitive.
Based on the name, I expect it to use the sequencer to print a message
to the user about the commit in progress.

What happens if writing to .git/index fails?  I can think of reasons
to remove the sequencer file before or afterward:

 - before, because once .git/index has been removed, the index is not
   locked any more and further commands could take place in parallel.

 - afterward, because when writing the index fails, I (the user)
   might want to react by running "git cherry-pick --abort".

The latter seems slightly more compelling to me --- after all, any
further command wanting to touch the sequencer directory is going
to check whether it exists --- but more importantly, the former
reminds me that we haven't thought carefully about what concurrent
operations using the sequencer are and aren't allowed.  Though I
doubt that it would come up much in practice. :)

> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -580,6 +580,17 @@ static void read_populate_todo(struct replay_insn_list **todo_list)
>  		die(_("Unusable instruction sheet: %s"), todo_file);
>  }
>  
> +void sequencer_notify_commit(void)
> +{
> +	struct replay_insn_list *todo_list = NULL;
> +
> +	if (!file_exists(git_path(SEQ_TODO_FILE)))
> +		return;
> +	read_populate_todo(&todo_list);
> +	if (!todo_list->next)
> +		remove_sequencer_state(1);
> +}

Not about this patch: I keep on forgetting what the argument to
remove_sequencer_state means.  Would it be possible to make it
a flag, which would both make the meaning more obvious and mean
it is easy to support additional flags in the future?

> --- a/t/t3032-merge-recursive-options.sh
> +++ b/t/t3032-merge-recursive-options.sh
> @@ -114,8 +114,10 @@ test_expect_success 'naive cherry-pick fails' '
>  	git read-tree --reset -u HEAD &&
>  	test_must_fail git cherry-pick --no-commit remote &&
>  	git read-tree --reset -u HEAD &&
> +	git cherry-pick --reset &&
>  	test_must_fail git cherry-pick remote &&
>  	test_must_fail git update-index --refresh &&
> +	git cherry-pick --reset &&
>  	grep "<<<<<<" text.txt
>  '

What is this about?  Maybe it would be clearer to change the "git
read-tree ..." to "git reset --hard", so the test assertion would not
rely on new cherry-pick features (and to mention the change in the
commit message!).

Doesn't this point to a risk in the patch?  Do you think there might
be scripts out there relying on being able to use "git read-tree
--reset -u HEAD" to clear away a failed cherry-pick before trying
again, and if so, can we do something about it?

> --- a/t/t3510-cherry-pick-sequence.sh
> +++ b/t/t3510-cherry-pick-sequence.sh
> @@ -82,13 +82,13 @@ test_expect_success '--reset cleans up sequencer state' '
>  	test_path_is_missing .git/sequencer
>  '
>  
> -test_expect_success 'cherry-pick cleans up sequencer state when one commit is left' '
> +test_expect_success 'final commit cleans up sequencer state' '
>  	pristine_detach initial &&
>  	test_must_fail git cherry-pick base..picked &&
> -	test_path_is_missing .git/sequencer &&
>  	echo "resolved" >foo &&
>  	git add foo &&
>  	git commit &&
> +	test_path_is_missing .git/sequencer &&
>  	{

It would also be nice to check "test_path_is_dir" before the final
commit, so people working on this code in the future know both aspects
of the patch are intentional.

Thanks, I'm glad to see this patch.