Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -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)
[...]
> + 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 (...) {
...
} ...
[...]
> + 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(...);
[...]
> @@ -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()".
> +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)
[...]
> + 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);
> + 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?
>
> + /*
> + * 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
--
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