Re: [PATCH v2 4/4] builtin/rebase: support running "git rebase <upstream>"
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jul 3, 2018, 21:42 UTC
- Message-ID
- <xmqqfu10nghi.fsf@gitster-ct.c.googlers.com>
- In-Reply-To
- <20180702091509.15950-5-predatoramigo@gmail.com>
Pratik Karki <predatoramigo@gmail.com> writes:
> +static struct commit *peel_committish(const char *name)
> +{
> + struct object *obj;
> + struct object_id oid;It by itself is not a big enough deal to warrant a reroll, but please make it a habit to leave a blank line between the declarations and the first statement (i.e. here), to help readers.
Show 6 quoted lines
> + if (get_oid(name, &oid)) > + return NULL; > + obj = parse_object(&oid); > + return (struct commit *)peel_to_type(name, 0, obj, OBJ_COMMIT); > +} > +
Show 37 quoted lines
> +{
> + const char *argv[] = { NULL, NULL };
> + struct strbuf script_snippet = STRBUF_INIT;
> + struct argv_array env = ARGV_ARRAY_INIT;
> + int status;
> + const char *backend, *backend_func;
> +
> + argv_array_pushf(&env, "upstream_name=%s", opts->upstream_name);
> + argv_array_pushf(&env, "GIT_DIR=%s", absolute_path(get_git_dir()));
> + argv_array_pushf(&env, "upstream=%s",
> + oid_to_hex(&opts->upstream->object.oid));
> + argv_array_pushf(&env, "orig_head=%s", oid_to_hex(&opts->orig_head));
> + argv_array_pushf(&env, "onto=%s",
> + oid_to_hex(&opts->onto->object.oid));
> + argv_array_pushf(&env, "onto_name=%s", opts->onto_name);
> + argv_array_pushf(&env, "revisions=%s", opts->revisions);
> +
> + switch (opts->type) {
> + case REBASE_AM:
> + backend = "git-rebase--am";
> + backend_func = "git_rebase__am";
> + break;
> + case REBASE_INTERACTIVE:
> + backend = "git-rebase--interactive";
> + backend_func = "git_rebase__interactive";
> + break;
> + case REBASE_MERGE:
> + backend = "git-rebase--merge";
> + backend_func = "git_rebase__merge";
> + break;
> + case REBASE_PRESERVE_MERGES:
> + backend = "git-rebase--preserve-merges";
> + backend_func = "git_rebase__preserve_merges";
> + break;
> + default:
> + BUG("Unhandled rebase type %d", opts->type);
> + break;De-dent these lines so that case/default and switch all sit on the same column, and the body of each case arm is indented one level deeper than the case labels.
Show 9 quoted lines
> + } > + > + strbuf_addf(&script_snippet, > + ". git-rebase--common && . %s && %s", > + backend, backend_func); > + argv[0] = script_snippet.buf; > + > + status = run_command_v_opt_cd_env(argv, RUN_USING_SHELL, NULL, > + env.argv);
Hmph. These shell variables that tell rebase backend scriptlets what rebase frontend figured out about the request and current state used to be just plain shell variables that are not exported. Now they are exported and visible even to subprocesses these scriptlets spawn. That does not sound safe at all, especially GIT_DIR being among these variables (git-sh-setup sets GIT_DIR but does not export it and that has been very much deliberate).
You should actually be able to avoid exporthing them by building these variable assignment into script_snippet (you need to quote the values properly, using quote.c::sq_quote_buf() etc.), I think, and that would be a more faithful-to-the-original safe covnersion.
Thanks for a pleasant read. This step smells more like WIP, but I can see it is already moving in the right direction. The previous ones (except for the issues I noticed and sent responses separately) looked more-or-less good, too.