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

Re: [PATCH 1/5] sequencer: factor code out of revert builtin

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Nov 6, 2011, 00:12 UTC
Message-ID
<20111106001232.GC27272@elie.hsd1.il.comcast.net>
In-Reply-To
<1320510586-3940-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
Show 7 quoted lines
> Start building the generalized sequencer by moving code from revert.c
> into sequencer.c and sequencer.h.  Make the builtin responsible only
> for command-line parsing, and expose a new sequencer_pick_revisions()
> to do the actual work of sequencing commits.
>
> This is intended to be almost a pure code movement patch with no
> functional changes.  Check with:

Do I understand correctly that the purpose of this patch is to expose some functions through the "sequencer.h" API, which patches later in the series will use? Which functions? What is this generalized sequencer which we are starting to build? Why should I be happy about (or care about, for that matter) code having moved from one source file to another?

Rule of thumb for commit messages: after reading a commit message, I should be able to predict what the patch will do, without reading the patch.

I am guessing the above description started sane and then went through a few revisions without a person reading it all the way through again. Please consider just rewriting it.

[...]
Show 20 quoted lines
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -1,19 +1,9 @@
>  #include "cache.h"
>  #include "builtin.h"
> -#include "object.h"
> -#include "commit.h"
> -#include "tag.h"
> -#include "run-command.h"
> -#include "exec_cmd.h"
> -#include "utf8.h"
>  #include "parse-options.h"
> -#include "cache-tree.h"
>  #include "diff.h"
>  #include "revision.h"
>  #include "rerere.h"
> -#include "merge-recursive.h"
> -#include "refs.h"
> -#include "dir.h"
>  #include "sequencer.h"
Hoorah!
[snipping lots of deletion of code from builtin/revert.c]
Show 6 quoted lines
> @@ -1011,7 +194,7 @@ int cmd_revert(int argc, const char **argv, const char *prefix)
>  	opts.action = REPLAY_REVERT;
>  	git_config(git_default_config, NULL);
>  	parse_args(argc, argv, &opts);
> -	res = pick_revisions(&opts);
> +	res = sequencer_pick_revisions(&opts);

The new sequencer_pick_revisions is just a new name for the old pick_revisions. Sane, but probably worth mentioning in the log message.

[...]
Show 20 quoted lines
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -1,7 +1,27 @@
>  #include "cache.h"
> +#include "object.h"
> +#include "commit.h"
> +#include "tag.h"
> +#include "run-command.h"
> +#include "exec_cmd.h"
> +#include "utf8.h"
> +#include "cache-tree.h"
> +#include "diff.h"
> +#include "revision.h"
> +#include "rerere.h"
> +#include "merge-recursive.h"
> +#include "refs.h"
> -#include "sequencer.h"
> -#include "strbuf.h"
>  #include "dir.h"
> +#include "sequencer.h"

Why did sequencer.h move to after dir.h? Wow, we use a lot of headers here --- I wonder if there are some pieces that could be split out (that's not due to your patch, though).

[...]
Show 9 quoted lines
> --- a/sequencer.h
> +++ b/sequencer.h
> @@ -8,6 +8,30 @@
>  #define SEQ_OPTS_FILE	"sequencer/opts"
>  
>  enum replay_action { REPLAY_REVERT, REPLAY_PICK };
> +enum replay_subcommand { REPLAY_NONE, REPLAY_RESET, REPLAY_CONTINUE };
> +
> +struct replay_opts {
[...]
Show 5 quoted lines
> @@ -25,4 +49,6 @@ struct replay_insn_list {
>   */
>  void remove_sequencer_state(int aggressive);
>  
> +int sequencer_pick_revisions(struct replay_opts *opts);

Ah, so this moves most of the logic of "git cherry-pick" to the sequencer but the only new API that needs to be exposed is pick_revisions(). The calling sequence looks like this:

	memset(&opts, o, sizeof(opts));
	opts.action = REPLAY_PICK;
	opts.revs = xmalloc(sizeof(*opts.revs));
	init_revisions(opts.revs);
	add_pending_object / setup_revisions / etc
	sequencer_pick_revisions(&opts);

The small exposed interface makes this a relatively uninvasive patch, and the immediate advantage is that we plan to reuse some of the functionality used in pick_revisions() in other, new APIs to be used by commands other than cherry-pick. No functional change yet intended.

Except for the commit message, looks reasonable (though I haven't tried the "git blame" magic to check the code movement part). Thanks.

Previous: Ramkumar RamachandraNext: Ramkumar Ramachandra
Message 3 of 47 in “Sequencer: working around historical mistakes”
  1. 0/5 Sequencer: working around historical mistakesRamkumar Ramachandra, Nov 5, 2011
  2. 1/5 sequencer: factor code out of revert builtinRamkumar Ramachandra, Nov 5, 2011
  3. Jonathan NiederNov 6, 2011
  4. Ramkumar RamachandraNov 13, 2011
  5. Junio C HamanoNov 13, 2011
  6. Ramkumar RamachandraNov 15, 2011
  7. Miles BaderNov 15, 2011
  8. Jonathan NiederNov 15, 2011
  9. 2/5 sequencer: remove CHERRY_PICK_HEAD with sequencer stateRamkumar Ramachandra, Nov 5, 2011
  10. Jonathan NiederNov 6, 2011
  11. 3/5 sequencer: sequencer state is useless without todoRamkumar Ramachandra, Nov 5, 2011
  12. Jonathan NiederNov 6, 2011
  13. Ramkumar RamachandraNov 13, 2011
  14. Junio C HamanoNov 13, 2011
  15. Ramkumar RamachandraNov 15, 2011
  16. Jonathan NiederNov 15, 2011
  17. Junio C HamanoNov 15, 2011
  18. Ramkumar RamachandraNov 16, 2011
  19. Junio C HamanoNov 16, 2011
  20. 0/3 avoiding unintended consequences of git_path() usageJonathan Nieder, Nov 16, 2011
  21. 1/3 do not let git_path clobber errno when reporting errorsJonathan Nieder, Nov 16, 2011
  22. 2/3 Bigfile: dynamically allocate buffer for marks file nameJonathan Nieder, Nov 16, 2011
  23. 3/3 rename git_path() to git_path_unsafe()Jonathan Nieder, Nov 16, 2011
  24. Junio C HamanoNov 17, 2011
  25. Jonathan NiederNov 17, 2011
  26. Nguyen Thai Ngoc DuyNov 16, 2011
  27. Nguyen Thai Ngoc DuyNov 16, 2011
  28. Jonathan NiederNov 16, 2011
  29. Nguyen Thai Ngoc DuyNov 16, 2011
  30. Ramsay JonesNov 19, 2011
  31. introduce strbuf_addpath()Jonathan Nieder, Nov 16, 2011
  32. Nguyen Thai Ngoc DuyNov 18, 2011
  33. Junio C HamanoNov 16, 2011
  34. Ramkumar RamachandraNov 16, 2011
  35. Nguyen Thai Ngoc DuyNov 16, 2011
  36. Michael HaggertyNov 16, 2011
  37. Nguyen Thai Ngoc DuyNov 18, 2011
  38. 4/5 sequencer: handle single commit pick separatelyRamkumar Ramachandra, Nov 5, 2011
  39. Jonathan NiederNov 6, 2011
  40. 5/5 sequencer: revert d3f4628eRamkumar Ramachandra, Nov 5, 2011
  41. Jonathan NiederNov 6, 2011
  42. Junio C HamanoNov 6, 2011
  43. Ramkumar RamachandraNov 7, 2011
  44. Ramkumar RamachandraNov 12, 2011
  45. Jonathan NiederNov 12, 2011
  46. Jonathan NiederNov 5, 2011
  47. Ramkumar RamachandraNov 13, 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.