Ramkumar Ramachandra wrote:
> A cherry-pick/ revert operation consists of several smaller steps.
> Later in the series, we would like to be able to resume a failed
> operation.
When introducing jargon, it is hard to make the intent perfectly
clear. I suppose what this means is:
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
allow "git cherry-pick --abort" to cancel and "git cherry-pick
--continue" to resume the entire command, we will need to store some
information about the state and the plan at the beginning.
> Introduce a "head" file to make note of the HEAD when
> the operation stated (so that the operation can be aborted), a "todo"
> file to keep the list of the steps to be performed, and a "done" file
> to keep a list of steps that have completed successfully. The format
> of these files is similar to the one used by the "rebase -i" process.
s/stated/started/ :) Makes some sense, aside from that.
It would be more conventional to use all-caps symref-like names, like
MULTIPLE_CHERRY_PICK_ORIG_HEAD, CHERRY_PICK_TODO, and
CHERRY_PICK_DONE, or to put these files in a subdirectory (oh, they're
already in a subdirectory? Why didn't you mention that? :)).
By the way, what is .git/sequencer/done used for?
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -25,6 +26,13 @@
> * Copyright (c) 2005 Junio C Hamano
> */
>
> +#define SEQ_DIR "sequencer"
> +
> +#define SEQ_PATH git_path(SEQ_DIR)
> +#define HEAD_FILE git_path(SEQ_DIR "/head")
> +#define TODO_FILE git_path(SEQ_DIR "/todo")
> +#define DONE_FILE git_path(SEQ_DIR "/done")
These seeming constants that call a function are kind of scary.
> @@ -629,21 +637,118 @@ static int read_and_refresh_cache(struct replay_opts *opts)
> return 0;
> }
>
> +static int format_todo(struct strbuf *buf, struct commit_list *list,
> + struct replay_opts *opts)
> +{
> + struct commit_list *cur = NULL;
> + struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
> + const char *sha1 = NULL;
> + const char *action;
> +
> + action = (opts->action == REVERT ? "revert" : "pick");
> + for (cur = list; cur; cur = cur->next) {
> + sha1 = find_unique_abbrev(cur->item->object.sha1, DEFAULT_ABBREV);
> + if (get_message(cur->item, cur->item->buffer, &msg))
> + return error(_("Cannot get commit message for %s"), sha1);
> + strbuf_addf(buf, "%s %s %s\n", action, sha1, msg.subject);
Is this internal state or for the user? If it is internal state, I'd
naÃvely have expected a sequence of 40-character hexadecimal lines,
perhaps with human-readable names like "topic~3" for the sake of
error messages if git knows about them.
> +static int persist_initialize(unsigned char *head)
> +{
> + struct strbuf buf = STRBUF_INIT;
> + int fd;
> +
> + if (!file_exists(SEQ_PATH) && mkdir(SEQ_PATH, 0777)) {
What if .git/sequencer exists and is a file? How does this interact
with "[core] sharedrepository" configuration? What happens if
.git/sequencer contains some stale files --- if the power fails while
git is writing new files in .git/sequencer/, will the state be
confusing?
> + int err = errno;
> + strbuf_release(&buf);
> + error(_("Could not create sequencer directory '%s': %s"),
> + SEQ_PATH, strerror(err));
> + return -err;
Why does the caller care about which errno, and what is it going to
do with that information?
> + }
> +
> + if ((fd = open(HEAD_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {
More idiomatic in the git codebase to write:
fd = open(...);
if (fd < 0) {
> + int err = errno;
> + strbuf_release(&buf);
> + error(_("Could not open '%s' for writing: %s"),
> + HEAD_FILE, strerror(err));
> + return -err;
As above. Why does the caller care about errno? If backing out after
an error, I suppose it might make sense to rmdir .git/sequencer while
at it.
> + }
> +
> + strbuf_addf(&buf, "%s", find_unique_abbrev(head, DEFAULT_ABBREV));
Why abbreviate?
> + write_or_whine(fd, buf.buf, buf.len, HEAD_FILE);
What happens and should happen on error?
[...]
> +static int persist_todo_done(int res, struct commit_list *todo_list,
> + struct commit_list *done_list, struct replay_opts *opts)
This is about recording what has been done and what remains to
be done? What does the res argument represent?
> +{
> + struct strbuf buf = STRBUF_INIT;
> + int fd, res2;
> +
> + if (!res)
> + return 0;
> +
> + /* TODO file */
> + if ((fd = open(TODO_FILE, O_WRONLY | O_CREAT | O_TRUNC, 0666)) < 0) {
What happens if we are interrupted in the middle of writing this?
> + int err = errno;
> + strbuf_release(&buf);
> + error(_("Could not open '%s' for writing: %s"),
> + TODO_FILE, strerror(err));
> + return -err;
I don't think the caller should care which errno. :)
[...]
> static int pick_commits(struct replay_opts *opts)
> {
> + struct commit_list *done_list = NULL;
> struct rev_info revs;
> struct commit *commit;
> + unsigned char head[20];
> int res;
>
> + if (get_sha1("HEAD", head))
> + return error(_("You do not have a valid HEAD"));
What should happen if I try to cherry-pick onto an unborn branch? I
haven't checked what happens.
> +
> if ((res = read_and_refresh_cache(opts)) ||
> - (res = prepare_revs(&revs, opts)))
> + (res = prepare_revs(&revs, opts)) ||
> + (res = persist_initialize(head)))
> return res;
>
> - while ((commit = get_revision(&revs)) &&
> - !(res = do_pick_commit(commit, opts)))
> - ;
> -
> - return res;
> + while ((commit = get_revision(&revs))) {
> + if (!(res = do_pick_commit(commit, opts)))
> + commit_list_insert(commit, &done_list);
This puts done_list in the reverse order that the commits were
cherry-picked. Is that the intent?
> + else {
> + commit_list_insert(commit, &revs.commits);
> + break;
> + }
> + }
> + return persist_todo_done(res, revs.commits, done_list, opts);
A few potential trade-offs:
- should cherry-pick record the state after every commit? This would
be safe against stray die() calls or segfaults but requires hitting
the filesystem which might not be wanted if doing a run of
cherry-picks in memory (though git is far from supporting such a
"many cherry picks in core followed by checkout and packed
collection of objects written to disk all at once" optimization
anyway).
- should we use O_TRUNC or O_APPEND to modify the state in-place or
use separate files and rename them into place? The latter is
safer against sudden exit.
- should we (perhaps optionally) fsync the state when commiting to it?
I think no, but someone performing a rebase and running a test suite
with the potential to crash the system between commits might appreciate
the effort.
--
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