Ramkumar Ramachandra wrote:
> 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.
Makes a lot of sense.
> Don't touch
> CHERRY_PICK_HEAD -- it will still be useful when a conflict is
> encountered.
I think there's some logical connector or something missing. Why would
introducing a .git/sequencer dir involve touching CHERRY_PICK_HEAD?
Maybe the idea is to say: "The purpose of these new files is orthogonal
to the existing CHERRY_PICK_HEAD" with some explanation of that.
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -13,6 +13,7 @@
> #include "rerere.h"
> #include "merge-recursive.h"
> #include "refs.h"
> +#include "dir.h"
>
> /*
> * This implements the builtins revert and cherry-pick.
> @@ -25,6 +26,10 @@
> * Copyright (c) 2005 Junio C Hamano
> */
>
> +#define SEQ_DIR git_path("sequencer")
> +#define HEAD_FILE git_path("sequencer/head")
> +#define TODO_FILE git_path("sequencer/todo")
I've failed to convince you in the past that these fake constants are
scary, but believe me, they really are. Consider the following code
--- what would you expect it to print? What does it actually print?
(Hint: there's not one right answer.)
printf("%s %s %s %s %s\n", SEQ_DIR, HEAD_FILE, TODO_FILE,
SEQ_DIR, HEAD_FILE);
> +static void walk_revs_populate_todo(struct commit_list **todo_list,
> + struct replay_opts *opts)
> {
> struct rev_info revs;
> struct commit *commit;
> + struct commit_list *new_item;
> + struct commit_list *cur = NULL;
> +
> + /* Insert into todo_list in the same order */
> + prepare_revs(&revs, opts);
> + while ((commit = get_revision(&revs))) {
> + new_item = xmalloc(sizeof(struct commit_list));
> + new_item->item = commit;
> + if (cur)
> + cur->next = new_item;
> + else
> + *todo_list = new_item;
> + cur = new_item;
> + }
> + cur->next = NULL;
The naive reader, perhaps stupidly, wonders: "why not use
commit_list_insert"? A comment or something to explain "NEEDSWORK:
expose this as commit_list_append" could help him.
[...]
> - return 0;
> + /* Sequence of picks finished successfully; cleanup by
> + removing the .git/sequencer directory */
> + return cleanup_sequencer_data();
GNU-style comment seems to have snuck in.
Thanks; this one was pretty pleasant.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html