Hi,
Jonathan Nieder writes:
> 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.
>
> I think I remember Junio being curious about which commit is stored in
> "head"; this might be a good place to put a reminder so future readers
> don't have to be confused.
Oops, I totally forgot -- sorry Junio.
He suggested that we store the corresponding ref also somewhere. Have
to think about this some more before the next iteration.
>> --- a/builtin/revert.c
>> +++ b/builtin/revert.c
> [...]
>> @@ -25,6 +26,10 @@
>> * Copyright (c) 2005 Junio C Hamano
>> */
>>
>> +#define SEQ_DIR "sequencer"
>> +#define SEQ_HEAD_FILE "sequencer/head"
>> +#define SEQ_TODO_FILE "sequencer/todo"
>
> Yay. :)
Sorry it took me so long to understand this. Your elaborate
explanation last time drove the point home.
>> @@ -417,7 +422,7 @@ static int do_pick_commit(struct commit *commit, struct replay_opts *opts)
>> return error(_("Your index file is unmerged."));
>> } else {
>> if (get_sha1("HEAD", head))
>> - return error(_("You do not have a valid HEAD"));
>> + return error(_("Can't %s on an unborn branch"), me);
>
> Remember that "me" is an untranslated command name, and see also
> http://thread.gmane.org/gmane.comp.version-control.git/153026
>
> Perhaps it would make sense to do something like
>
> if (get_sha1("HEAD", head)) {
> if (opts->action == REVERT)
> return error(_("can't revert as initial commit"));
> return error(_("cherry-pick into empty head not supported yet"));
> }
>
> In a way they feel like different operations, anyway. On the other
> hand, there's no reason I can think of not to allow reverting a patch
> that only removes files as the initial commit other than not having
> implemented it.
Okay. That would be unrelated to this patch though -- I'll make it a
separate patch and move it to the beginning of the series.
>> @@ -578,10 +583,106 @@ static void read_and_refresh_cache(const char *me, struct replay_opts *opts)
>> rollback_lock_file(&index_lock);
>> }
>>
>> -static int pick_commits(struct replay_opts *opts)
>> +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);
>
> Maybe some word like "command", "insn", or "keyword" would be more
> suggestive than "action". It also might be worth mentioning somewhere
> (in the commit message?) that this format is inspired by
> rebase--interactive's insn sheet.
Okay, will do.
> The operation that could be exposed does not include get_revision,
> does it?
>
> /*
> * Example:
> *
> * struct commit_list *list;
> * struct commit_list **next = &list;
> *
> * next = commit_list_append(c1, next);
> * next = commit_list_append(c2, next);
> * *next = NULL;
> * assert(commit_list_count(list) == 2);
> * return list;
> *
> * Don't forget to NULL-terminate!
> */
> struct commit_list **commit_list_append(struct commit *commit,
> struct commit_list **next)
> {
> struct commit_list *new = xmalloc(sizeof(*new_list));
> new->item = commit;
> *next = new;
> return &new->next;
> }
I would have done this, but I was worried about what the NULL
termination would mean API-wise. In retrospect, a lot of APIs
described in Documentation/technical are pretty non-trivial, and it's
not obvious how to use it without the documentation. Would it be okay
to expose this in commit.c and write some documentation? I already
have two callers.
>> +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));
>> +}
>
> A local variable to cache the git_path result would make this much
> easier to read.
Okay.
>> diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
>> new file mode 100644
>
> Should "chmod +x" so the test can be run directly (I forget to do that
> all the time).
Good catch.
>> +test_expect_success 'cherry-pick cleans up sequencer directory upon success' '
>> + pristine_detach initial &&
>> + git cherry-pick initial..picked &&
>> + test_path_is_missing .git/sequencer
>> +'
>
> Thanks for thinking about these things. Maybe another test
> demonstrating that the .git/sequencer directory is left behind on
> failure would help put this in context.
Good suggestion.
-- Ram
--
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