Ramkumar Ramachandra <artagnon@xxxxxxxxx> writes:
> ... Don't touch
> CHERRY_PICK_HEAD -- it will still be useful when a conflict is
> encountered.
What does "Don't touch" here mean? Keep doing the same thing as before so
that people can rely on it? Or stop touching it? I presume the former, but
the description is suboptimal.
> @@ -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")
HEAD is to keep track of the original, or does it get updated during a
sequencer run?
> +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;
> +}
Yuck; you do not want if/else to do this (I am _not_ saying "you can do
that but here is a better way"). Have a variable that points at a pointer
of type "struct commit_list *", initialize it to where todo_list points
at, and then update to point at the next field of the new object.
Something like this:
next = todo_list;
while ((commit = ...) != NULL) {
new = xcalloc(1, sizeof(struct commit_list));
new->item = commit;
*next = new;
next = &new->next;
}
*next = NULL;
Doing it this way would avoid a segfault when get_revision() returned
nothing as an added bonus ;-).
> +static void persist_head(const char *head)
s/persist/save/ perhaps?
> +{
> + static struct lock_file head_lock;
> + struct strbuf buf = STRBUF_INIT;
> + int fd;
> +
> + if (file_exists(SEQ_DIR)) {
> + if (!is_directory(SEQ_DIR) && remove_path(SEQ_DIR) < 0) {
> + strbuf_release(&buf);
> + die(_("Could not remove %s"), SEQ_DIR);
> + }
> + } else {
> + if (mkdir(SEQ_DIR, 0777) < 0) {
> + strbuf_release(&buf);
> + die_errno(_("Could not create sequencer directory '%s'."), SEQ_DIR);
> + }
> + }
Split this part into a separate helper.
> + fd = hold_lock_file_for_update(&head_lock, 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."), HEAD_FILE);
> + if (commit_lock_file(&head_lock) < 0)
> + die(_("Error wrapping up %s"), HEAD_FILE);
> +}
Is it sufficient to write the object name of the commit HEAD points at?
Would it be useful to also record the branch if the HEAD points at one?
> +static void persist_todo(struct commit_list *todo_list, struct replay_opts *opts)
s/persist/save/ perhaps?
> +{
> + static struct lock_file todo_lock;
> + struct strbuf buf = STRBUF_INIT;
> + int fd;
> + fd = hold_lock_file_for_update(&todo_lock, TODO_FILE, LOCK_DIE_ON_ERROR);
The current set of callers might always save head and then todo, but it
probably is a good idea to lift that restriction (which is why I suggested
to separate the initialization of seq-dir into a separate helper).
> +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);
> read_and_refresh_cache(me, opts);
> + setenv(GIT_REFLOG_ACTION, me, 0);
Why?
> - prepare_revs(&revs, opts);
> + walk_revs_populate_todo(&todo_list, opts);
> + if (!get_sha1("HEAD", sha1))
> + persist_head(sha1_to_hex(sha1));
> + persist_todo(todo_list, opts);
>
> - while ((commit = get_revision(&revs))) {
> - int res = do_pick_commit(commit, opts);
> + for (cur = todo_list; cur; cur = cur->next) {
> + persist_todo(cur, opts);
> + res = do_pick_commit(cur->item, opts);
> if (res)
> return res;
> }
>
> - return 0;
> + /* Sequence of picks finished successfully; cleanup by
> + removing the .git/sequencer directory */
Just a style nit.
/*
* We write our multi-line comments
* this way.
*/
> + return cleanup_sequencer_data();
> }
--
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