Re: [PATCH 11/18] revert: Save data for continuing after conflict resolution
- From
Jonathan Nieder <jrnieder@gmail.com>
- Date
- Jul 27, 2011, 05:02 UTC
- Message-ID
- <20110727050220.GG18470@elie>
- In-Reply-To
- <1311736755-24205-12-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c > +++ b/builtin/revert.c
[...]
Show 9 quoted lines
> @@ -269,7 +275,7 @@ static void write_message(struct strbuf *msgbuf, const char *filename)
> die_errno(_("Could not write to %s."), filename);
> strbuf_release(msgbuf);
> if (commit_lock_file(&msg_file) < 0)
> - die(_("Error wrapping up %s"), filename);
> + die(_("Error wrapping up %s."), filename);
> }
>
> static struct tree *empty_tree(void)Snuck in to the wrong patch, I guess?
[...]
> @@ -582,10 +588,199 @@ static struct commit *parse_insn_line(char *start, struct replay_opts *opts)
[...]
Show 13 quoted lines
> + if (!prefixcmp(start, "pick ")) {
> + action = CHERRY_PICK;
> + insn_len = strlen("pick");
> + p = start + insn_len + 1;
> + }
> + else if (!prefixcmp(start, "revert ")) {
> + action = REVERT;
> + insn_len = strlen("revert");
> + p = start + insn_len + 1;
> + }
> + else
> + return NULL;
> +Style: git uses "cuddled braces".
if (...) {
...
} else if (...) {
...
} ...[...]
Show 5 quoted lines
> + if ((get_sha1(sha1_abbrev, commit_sha1) < 0) > + || !(commit = lookup_commit_reference(commit_sha1))) > + return NULL; > + > + return commit;
Would be clearer to write:
if (get_sha1(...)) return NULL;
return lookup_commit_reference(...);
[...]
Show 5 quoted lines
> @@ -59,6 +60,11 @@ struct replay_opts {
> };
>
> #define GIT_REFLOG_ACTION "GIT_REFLOG_ACTION"
> +#define MAYBE_UNUSED __attribute__((__unused__))[...]
> +} > + > +static void MAYBE_UNUSED read_populate_todo(struct commit_list **todo_list, > + struct replay_opts *opts)
This MAYBE_UNUSED with no conditional involved feels ugly. Why is it needed? Why not define read_populate_todo in the patch that first uses it, or barring that, tolerate a warning in this step within the patch series?
[...]
> + if (!(commit = parse_insn_line(p, opts))) > + goto error;
See Documentation/CodingGuidelines: "We try to avoid assignments inside if()".
Show 20 quoted lines
> +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 **next;
> +
> + prepare_revs(&revs, opts);
> +
> + next = todo_list;
> + while ((commit = get_revision(&revs)))
> + next = commit_list_append(commit, next);
> +}
> +
> +static void create_seq_dir(void)
> +{
> + const char *seq_dir = git_path(SEQ_DIR);
> +
> + if (!file_exists(seq_dir) && mkdir(seq_dir, 0777) < 0)
> + die_errno(_("Could not create sequencer directory '%s'."), seq_dir);What happens if seq_dir exists and is a file?
[...]
> @@ -593,14 +788,29 @@ static int pick_commits(struct replay_opts *opts)
[...]
Show 6 quoted lines
> + if (get_sha1("HEAD", sha1)) {
> + if (opts->action == REVERT)
> + return error(_("Can't revert as initial commit"));
> + return error(_("Can't cherry-pick into empty head"));
> + } else
> + save_head(sha1_to_hex(sha1));Clearer without the "else":
if (get_sha1(...)) {
...
return error(...);
}
save_head(sha1);Show 8 quoted lines
> + save_todo(todo_list, opts);
> +
> + for (cur = todo_list; cur; cur = cur->next) {
> + save_todo(cur, opts);
> + res = do_pick_commit(cur->item, opts);
> if (res)
> return res;
> }Isn't the save_todo() before the "for" loop redundant?
Show 7 quoted lines
> > + /* > + * Sequence of picks finished successfully; cleanup by > + * removing the .git/sequencer directory > + */ > + strbuf_addf(&buf, "%s", git_path(SEQ_DIR)); > + remove_dir_recursively(&buf, 0);
What is the content of "buf" before? If it's supposed to be empty, I'd suggest using "strbuf_reset" to make sure that remains so for the reader's peace of mind.
Ciao, Jonathan