Hi,
Ramkumar Ramachandra wrote:
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
> @@ -596,6 +596,32 @@ struct commit_list **commit_list_append(struct commit *commit,
> return &new->next;
> }
>
> +static void format_opts(struct strbuf *buf, struct replay_opts *opts)
> +{
> + int i;
> +
> + if (opts->no_commit)
> + strbuf_addstr(buf, "no-commit = true\n");
> + if (opts->edit)
> + strbuf_addstr(buf, "edit = true\n");
[...]
Somewhat repetitive. Would it make sense to do something like
add_opt_bool(buf, "no-commit", opts->no_commit);
add_opt_bool(buf, "edit", opts->edit);
add_opt_bool(buf, "signoff", opts->signoff);
if (opts->mainline)
add_opt_int(buf, "mainline", opts->mainline);
add_opt_string(buf, "strategy", opts->strategy);
add_opt_strings(buf, "strategy-option", opts->xopts, opts->xopts_nr);
> @@ -694,6 +720,184 @@ error:
> die(_("Malformed instruction sheet: %s"), git_path(SEQ_TODO_FILE));
> }
>
> +static struct strbuf *parse_value(const char *start, char **end_ptr)
> +{
> + static struct strbuf value = STRBUF_INIT;
> + int quote = 0;
> + char *p = (char *)start;
Are all callers passing a modifiable buffer? If so, why not make the
parameter non-const? Is it necessary to modify the buffer to parse it?
> + char *end;
> +
> + /* Find and strip '\n', '\r' */
> + if ((end = strchr(start, '\n'))) {
Style:
end = strchrnul(start, '\n');
if (*end && end > start && end[-1] == '\r')
end--;
for (p = start; p != end; p++) {
...
}
> + for (; *p != '\0'; p ++) {
> + if (!quote && (*p == ';' || *p == '#'))
> + /* Ignore comments */
> + goto ok;
> + if (*p == '\\') {
What is this syntax?
*thinks* Oh, this looks eerily similar to config.c::parse_value().
Shouldn't the commit message say so? Why not factor out a function so
they can share code?
> + p += 1;
[...]
> + /* Some pharapters escape as themselves */
Substitution error.
> +static char *parse_opt_value(const char *start, void *key,
> + enum parse_opt_type type, parse_opt_cb *cb_function)
> +{
> + struct strbuf *value, *subvalue;
> + struct option opt;
> + char *p, *cur, *val, *end;
> +
> + /* Remove spaces before '=' */
> + for (p = (char *)start; isspace(*p); p++);
Same questions apply. Stopping here.
Hope that helps,
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