git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Aug 14, 2011, 16:04 UTC
Message-ID
<20110814160440.GK18466@elie.gateway.2wire.net>
In-Reply-To
<1313310789-10216-8-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
Show 7 quoted lines
> 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?
Show 24 quoted lines
> --- 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. :)

Show 16 quoted lines
> --- 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?

Show 12 quoted lines
> --- 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?

Show 16 quoted lines
> --- 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.
Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 27 of 43 in “Generalized sequencer foundations”
  1. 0/7 Generalized sequencer foundationsRamkumar Ramachandra, Aug 14, 2011
  2. 1/7 revert: Free memory after get_message callRamkumar Ramachandra, Aug 14, 2011
  3. Jonathan NiederAug 14, 2011
  4. 2/7 revert: Fix buffer overflow in insn sheet parserRamkumar Ramachandra, Aug 14, 2011
  5. Jonathan NiederAug 14, 2011
  6. Ramkumar RamachandraAug 14, 2011
  7. 3/7 revert: Make commit descriptions in insn sheet optionalRamkumar Ramachandra, Aug 14, 2011
  8. Jonathan NiederAug 14, 2011
  9. Ramkumar RamachandraAug 14, 2011
  10. 4/7 revert: Allow mixed pick and revert instructionsRamkumar Ramachandra, Aug 14, 2011
  11. Jonathan NiederAug 14, 2011
  12. Ramkumar RamachandraAug 14, 2011
  13. Jonathan NiederAug 14, 2011
  14. 5/7 revert: Make the argument parser responsible for setup_revisionsRamkumar Ramachandra, Aug 14, 2011
  15. Jonathan NiederAug 14, 2011
  16. Ramkumar RamachandraAug 14, 2011
  17. 6/7 sequencer: Expose API to cherry-picking machineryRamkumar Ramachandra, Aug 14, 2011
  18. Jonathan NiederAug 14, 2011
  19. Ramkumar RamachandraAug 14, 2011
  20. Jonathan NiederAug 14, 2011
  21. Junio C HamanoAug 16, 2011
  22. revert: plug memory leak in "cherry-pick root commit" codepathJonathan Nieder, Aug 16, 2011
  23. Jonathan NiederAug 16, 2011
  24. Jeff KingAug 16, 2011
  25. Jonathan NiederAug 16, 2011
  26. 7/7 sequencer: Remove sequencer state after final commitRamkumar Ramachandra, Aug 14, 2011
  27. Jonathan NiederAug 14, 2011
  28. Ramkumar RamachandraAug 14, 2011
  29. Jonathan NiederAug 14, 2011
  30. Junio C HamanoAug 14, 2011
  31. Jonathan NiederAug 14, 2011
  32. Junio C HamanoAug 14, 2011
  33. Junio C HamanoAug 15, 2011
  34. Ramkumar RamachandraAug 17, 2011
  35. Ramkumar RamachandraAug 18, 2011
  36. Jonathan NiederAug 18, 2011
  37. Ramkumar RamachandraAug 18, 2011
  38. Jonathan NiederAug 18, 2011
  39. Ramkumar RamachandraAug 18, 2011
  40. Jonathan NiederAug 18, 2011
  41. Junio C HamanoAug 18, 2011
  42. Junio C HamanoAug 18, 2011
  43. Ramkumar RamachandraAug 19, 2011

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.