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

Re: [PATCH 11/17] revert: Save data for continuing after conflict resolution

From
Ramkumar Ramachandra <artagnon@gmail.com>
Date
Jul 17, 2011, 11:48 UTC
Message-ID
<CALkWK0nyu6W2Nd=qcrjZZwkAdFGqTqAe53FmecS_n2cK_4UWMg@mail.gmail.com>
In-Reply-To
<20110712193716.GB14909@elie>
Hi again,
Jonathan Nieder writes:
Show 16 quoted lines
> Ramkumar Ramachandra wrote:
>
>> It is imperative to note that these two files alone are not enough to
>> implement "--continue"; we will also need to persist the options that
>> were specified on the command-line, and this is done later in the
>> series.
>
> I think you could remove the phrase "It is imperative to note that"
> and this would say the same thing. :)
>
> By the way, those two files are enough to implement --continue in some
> sense, no?  One implementation of --continue would forget options
> passed on the first run and require the user to specify them again; it
> would just be less useful.  The reminder that "head" and "todo" are
> not the only files in the .git/sequencer dir is useful, though; thanks
> for it.
New commit message:
revert: Save data for continuing after conflict resolution

Ever since v1.7.2-rc1~4^2~7 (revert: allow cherry-picking more than one commit, 2010-06-02), a single invocation of "git cherry-pick" or "git revert" can perform picks of several individual commits. To implement features like "--continue" to continue the whole operation, we will need to store some information about the state and the plan at the beginning. Introduce a ".git/sequencer/head" file to store this state, and ".git/sequencer/todo" file to store the plan. The head file contains the SHA-1 of the HEAD before the start of the operation, and the todo file contains an instruction sheet whose format is inspired by the format of the "rebase -i" instruction sheet. As a result, a typical todo file looks like:

pick 8537f0e submodule add: test failure when url is not configured pick 4d68932 submodule add: allow relative repository path pick f22a17e submodule add: clean up duplicated code pick 59a5775 make copy_ref globally available

Since SHA-1 hex is abbreviated using an find_unique_abbrev(), it is unambiguous. This does not guarantee that there will be no ambiguity when more objects are added to the repository.

That these two files alone are not enough to implement a "--continue" that remembers the command-line options specified; later patches in the series save them too.

These new files are unrelated to the existing .git/CHERRY_PICK_HEAD, which will still be useful while committing after a conflict resolution.

Show 20 quoted lines
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> +static void format_todo(struct strbuf *buf, struct commit_list *todo_list,
>> +                     struct replay_opts *opts)
>> +{
>> +     struct commit_list *cur = NULL;
>> +     struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
>> +     const char *sha1_abbrev = NULL;
>> +     const char *action;
>> +
>> +     action = (opts->action == REVERT ? "revert" : "pick");
>> +     for (cur = todo_list; cur; cur = cur->next) {
>> +             sha1_abbrev = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
>> +             if (get_message(cur->item, &msg))
>> +                     die(_("Cannot get commit message for %s"), sha1_abbrev);
>> +             strbuf_addf(buf, "%s %s %s\n", action, sha1_abbrev, msg.subject);
>
> This die() seems odd in the context of a series that starts by
> converting various die() calls to "return error()".
Fixed.  I made format_todo return an int now.
Show 34 quoted lines
>> +static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
>> +{
>> +     unsigned char commit_sha1[20];
>> +     char sha1_abbrev[40];
>> +     struct commit *commit;
>> +     enum replay_action action;
>> +     int insn_len = 0;
>> +     char *p;
>> +
>> +     p = start;
>> +     if (!(p = strchr(p, ' ')))
>> +             return NULL;
>
> Style: it is much, much clearer to write
>
>        p = strchr(start, ' ');
>        if (!p)
>                return NULL;
>
> In the git codebase, assignments in "if" conditionals are very rare, so
> code like the above sticks out.
>
>> +     insn_len = p - start;
>> +     if (!(p = strchr(p + 1, ' ')))
>> +             return NULL;
>> +     p += 1;
>
> Likewise.  This could say:
>
>        insn_len = p - start;
>        p = strchr(p + 1, ' ');
>        if (!p)
>                return NULL;
>        p++;
Right, got it.  Fixed.
Show 10 quoted lines
>> +     if (!strncmp(start, "pick", insn_len))
>> +             action = CHERRY_PICK;
>> +     else if (!strncmp(start, "revert", insn_len))
>> +             action = REVERT;
>
> This code means that "p", "pi", and "pic" are accepted as
> abbreviations for "pick".   Maybe a comment would help to clarify
> that.  Are we okay with reserving these names, so "r" in a todo
> file means to "revert" and not to "reword" like it does in a
> rebase -i todo list?

That was the original intent, but I clearly hadn't thought about it hard enough. Changed this to accept only full words.

Show 9 quoted lines
>> +     /*
>> +      * Verify that the action matches up with the one in
>> +      * opts; we don't support arbitrary instructions
>> +      */
>> +     if (action != opts->action)
>> +             return NULL;
>
> If I try "git cherry-pick foo..bar" and then "git revert --continue",
> what error message will I get?

fatal: Malformed instruction sheet: .git/sequencer/todo Technically speaking, this is correct. However, this may not be ideal from an end-user's perspective. Anyway, this is going to change soon -- do you think this is worth correcting here and now?

Show 13 quoted lines
>> +static void read_populate_todo(struct commit_list **todo_list,
>> +                     struct replay_opts *opts)
>> +{
> [...]
>> +     for (p = buf.buf; *p; p = strchr(p, '\n') + 1) {
>> +             if (!(commit = parse_insn_line(p, opts)))
>> +                     goto error;
>> +             new = xmalloc(sizeof(struct commit_list));
>> +             new->item = commit;
>> +             *next = new;
>> +             next = &new->next;
>
> commit_list_append()?
Ugh.  Fixed.
Show 13 quoted lines
>> +static void create_seq_dir(void)
>> +{
>> +     if (file_exists(git_path(SEQ_DIR))) {
>> +             if (!is_directory(git_path(SEQ_DIR)) && remove_path(git_path(SEQ_DIR)) < 0)
>> +                     die(_("Could not remove %s"), git_path(SEQ_DIR));
>> +     } else if (mkdir(git_path(SEQ_DIR), 0777) < 0)
>> +             die_errno(_("Could not create sequencer directory '%s'."), git_path(SEQ_DIR));
>> +}
>
> As mentioned in [1], a local would make this more readable.  And it's
> not clear to me why one would want to delete a .git/sequencer file
> that the user carefully placed there (I don't think git itself ever
> writes such a thing).
Okay,  removed that hunk.
Show 38 quoted lines
>> +static void save_head(const char *head)
>> +{
>> +     static struct lock_file head_lock;
>> +     struct strbuf buf = STRBUF_INIT;
>> +     int fd;
>> +
>> +     fd = hold_lock_file_for_update(&head_lock, git_path(SEQ_HEAD_FILE), LOCK_DIE_ON_ERROR);
>> +     strbuf_addf(&buf, "%s\n", head);
>> +     if (write_in_full(fd, buf.buf, buf.len) < 0)
>> +             die_errno(_("Could not write to %s."), git_path(SEQ_HEAD_FILE));
>> +     if (commit_lock_file(&head_lock) < 0)
>> +             die(_("Error wrapping up %s"), git_path(SEQ_HEAD_FILE));
>> +}
>
> A local would be helpful here, too.
>
>> +static void save_todo(struct commit_list *todo_list, struct replay_opts *opts)
>> +{
>> +     static struct lock_file todo_lock;
>> +     struct strbuf buf = STRBUF_INIT;
>> +     int fd;
>> +
>> +     fd = hold_lock_file_for_update(&todo_lock, git_path(SEQ_TODO_FILE), LOCK_DIE_ON_ERROR);
>> +     format_todo(&buf, todo_list, opts);
>> +     if (write_in_full(fd, buf.buf, buf.len) < 0) {
>> +             strbuf_release(&buf);
>> +             die_errno(_("Could not write to %s."), git_path(SEQ_TODO_FILE));
>> +     }
>> +     if (commit_lock_file(&todo_lock) < 0) {
>> +             strbuf_release(&buf);
>> +             die(_("Error wrapping up %s"), git_path(SEQ_TODO_FILE));
>> +     }
>> +     strbuf_release(&buf);
>> +}
>
> Likewise here.  As mentioned before, git_path() clobbers errno, so the
> above is likely to print a wrong error message on some systems (e.g.,
> "fatal: Cannot write to .git/sequencer/todo: Success").
Fixed these.  Thanks.
Show 23 quoted lines
>> +static int pick_commits(struct replay_opts *opts)
>> +{
>> +     struct commit_list *todo_list = NULL;
>> +     unsigned char sha1[20];
>> +     struct commit_list *cur;
>> +     int res;
>>
>>       setenv(GIT_REFLOG_ACTION, me, 0);
>>       if (opts->allow_ff)
>> @@ -580,14 +764,24 @@ static int pick_commits(struct replay_opts *opts)
>>                               opts->record_origin || opts->edit));
>>       read_and_refresh_cache(me, opts);
>>
>> -     prepare_revs(&revs, opts);
>> +     walk_revs_populate_todo(&todo_list, opts);
>> +     create_seq_dir();
>> +     if (!get_sha1("HEAD", sha1))
>> +             save_head(sha1_to_hex(sha1));
>
> What happens if the .git/sequencer dir already exists (e.g., we were
> in the middle of a multiple-cherry-pick)?  What happens if I try to
> cherry-pick onto an unborn branch, or get_sha1() fails for some other
> reason?
Oops.  I lost this somewhere along the way -- fixed now.  Thanks :)
-- Ram
Previous: Jonathan NiederNext: Jonathan Nieder
Message 55 of 99 in “[GSoC update] Sequencer for inclusion”
  1. Ramkumar RamachandraJul 11, 2011
  2. 01/17 advice: Introduce error_resolve_conflictRamkumar Ramachandra, Jul 11, 2011
  3. 02/17 revert: Inline add_message_to_msg functionRamkumar Ramachandra, Jul 11, 2011
  4. Jonathan NiederJul 12, 2011
  5. Ramkumar RamachandraJul 13, 2011
  6. Jonathan NiederJul 13, 2011
  7. Ramkumar RamachandraJul 19, 2011
  8. Junio C HamanoJul 19, 2011
  9. Ramkumar RamachandraJul 19, 2011
  10. Jonathan NiederJul 19, 2011
  11. Ramkumar RamachandraJul 20, 2011
  12. 03/17 revert: Don't check lone argument in get_encodingRamkumar Ramachandra, Jul 11, 2011
  13. Jonathan NiederJul 12, 2011
  14. Ramkumar RamachandraJul 13, 2011
  15. Jonathan NiederJul 13, 2011
  16. 04/17 revert: Rename no_replay to record_originRamkumar Ramachandra, Jul 11, 2011
  17. Jonathan NiederJul 12, 2011
  18. Ramkumar RamachandraJul 13, 2011
  19. Jonathan NiederJul 17, 2011
  20. 05/17 revert: Propogate errors upwards from do_pick_commitRamkumar Ramachandra, Jul 11, 2011
  21. Jonathan NiederJul 12, 2011
  22. Ramkumar RamachandraJul 17, 2011
  23. Jonathan NiederJul 17, 2011
  24. Ramkumar RamachandraJul 19, 2011
  25. 06/17 revert: Eliminate global "commit" variableRamkumar Ramachandra, Jul 11, 2011
  26. Jonathan NiederJul 12, 2011
  27. Ramkumar RamachandraJul 13, 2011
  28. Jonathan NiederJul 13, 2011
  29. Ramkumar RamachandraJul 13, 2011
  30. Jonathan NiederJul 13, 2011
  31. 07/17 revert: Introduce struct to keep command-line optionsRamkumar Ramachandra, Jul 11, 2011
  32. Jonathan NiederJul 12, 2011
  33. Ramkumar RamachandraJul 13, 2011
  34. 08/17 revert: Separate cmdline parsing from functional codeRamkumar Ramachandra, Jul 11, 2011
  35. Jonathan NiederJul 12, 2011
  36. Ramkumar RamachandraJul 18, 2011
  37. Jonathan NiederJul 18, 2011
  38. 09/17 revert: Don't create invalid replay_opts in parse_argsRamkumar Ramachandra, Jul 11, 2011
  39. Junio C HamanoJul 11, 2011
  40. Ramkumar RamachandraJul 12, 2011
  41. Jonathan NiederJul 12, 2011
  42. Ramkumar RamachandraJul 17, 2011
  43. Jonathan NiederJul 17, 2011
  44. 10/17 sequencer: Announce sequencer state locationRamkumar Ramachandra, Jul 11, 2011
  45. Jonathan NiederJul 12, 2011
  46. Sverre RabbelierJul 13, 2011
  47. Ramkumar RamachandraJul 17, 2011
  48. Jonathan NiederJul 17, 2011
  49. Ramkumar RamachandraJul 18, 2011
  50. 11/17 revert: Save data for continuing after conflict resolutionRamkumar Ramachandra, Jul 11, 2011
  51. Junio C HamanoJul 11, 2011
  52. Junio C HamanoJul 11, 2011
  53. Ramkumar RamachandraJul 12, 2011
  54. Jonathan NiederJul 12, 2011
  55. Ramkumar RamachandraJul 17, 2011
  56. Jonathan NiederJul 17, 2011
  57. Ramkumar RamachandraJul 18, 2011
  58. Jonathan NiederJul 19, 2011
  59. Ramkumar RamachandraJul 19, 2011
  60. 12/17 revert: Save command-line options for continuing operationRamkumar Ramachandra, Jul 11, 2011
  61. Junio C HamanoJul 11, 2011
  62. Ramkumar RamachandraJul 12, 2011
  63. Jonathan NiederJul 12, 2011
  64. Ramkumar RamachandraJul 18, 2011
  65. 13/17 revert: Introduce a layer of indirection over pick_commitsRamkumar Ramachandra, Jul 11, 2011
  66. Jonathan NiederJul 12, 2011
  67. Ramkumar RamachandraJul 18, 2011
  68. 14/17 reset: Make hard reset remove the sequencer stateRamkumar Ramachandra, Jul 11, 2011
  69. Jonathan NiederJul 12, 2011
  70. Ramkumar RamachandraJul 17, 2011
  71. 15/17 revert: Remove sequencer state when no commits are pendingRamkumar Ramachandra, Jul 11, 2011
  72. Junio C HamanoJul 11, 2011
  73. Ramkumar RamachandraJul 12, 2011
  74. 16/17 revert: Introduce --reset to remove sequencer stateRamkumar Ramachandra, Jul 11, 2011
  75. Jonathan NiederJul 12, 2011
  76. Ramkumar RamachandraJul 17, 2011
  77. Jonathan NiederJul 17, 2011
  78. 17/17 revert: Introduce --continue to continue the operationRamkumar Ramachandra, Jul 11, 2011
  79. Jonathan NiederJul 12, 2011
  80. Ramkumar RamachandraJul 17, 2011
  81. Jonathan NiederJul 17, 2011
  82. Ramkumar RamachandraJul 18, 2011
  83. Jonathan NiederJul 18, 2011
  84. Jonathan NiederJul 11, 2011
  85. Ramkumar RamachandraJul 11, 2011
  86. Ramkumar RamachandraJul 11, 2011
  87. Jonathan NiederJul 11, 2011
  88. Ramkumar RamachandraJul 12, 2011
  89. Junio C HamanoJul 11, 2011
  90. Jeff KingJul 11, 2011
  91. Ramkumar RamachandraJul 12, 2011
  92. Jeff KingJul 12, 2011
  93. Ramkumar RamachandraJul 13, 2011
  94. Jeff KingJul 13, 2011
  95. config: Introduce functions to write non-standard fileRamkumar Ramachandra, Jul 18, 2011
  96. Junio C HamanoJul 18, 2011
  97. Ramkumar RamachandraJul 19, 2011
  98. Jonathan NiederJul 12, 2011
  99. Miles BaderJul 12, 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.