Re: [PATCH v3 4/4] builtin/rebase: support running "git rebase <upstream>"

Pratik Karki [off-list ref] writes:

+static void add_var(struct strbuf *buf, const char *name, const char *value)
+{
+	strbuf_addstr(buf, name);
+	strbuf_addstr(buf, "=");
+	sq_quote_buf(buf, value);
+	strbuf_addstr(buf, "; ");
+}
+
+static int run_specific_rebase(struct rebase_options *opts)
+{
+	const char *argv[] = { NULL, NULL };
+	struct strbuf script_snippet = STRBUF_INIT;
+	int status;
+	const char *backend, *backend_func;
+
+	add_var(&script_snippet, "GIT_DIR", absolute_path(get_git_dir()));
+
+	add_var(&script_snippet, "upstream_name", opts->upstream_name);
+	add_var(&script_snippet, "upstream",
+				 oid_to_hex(&opts->upstream->object.oid));
+	add_var(&script_snippet, "head_name", opts->head_name);
+	add_var(&script_snippet, "orig_head", oid_to_hex(&opts->orig_head));
+	add_var(&script_snippet, "onto", oid_to_hex(&opts->onto->object.oid));
+	add_var(&script_snippet, "onto_name", opts->onto_name);
+	add_var(&script_snippet, "revisions", opts->revisions);

Looks alright.

+	switch (opts->type) {
+	case REBASE_AM:
+...
+	default:
+		BUG("Unhandled rebase type %d", opts->type);
+		break;
+	}

Better.

+	strbuf_addf(&script_snippet,
+		    ". git-rebase--common && . %s && %s",
+		    backend, backend_func);
+	argv[0] = script_snippet.buf;
+
+	status = run_command_v_opt(argv, RUN_USING_SHELL);

This used to use run_command_v_opt_cd_env() to pass extra
environment but now we can do a simpler one.  As cmd_rebase()
has called setup_git_directory() to chdir to the top level of
the working tree, we just want to .-source the backend within
the current working directory, so loss of cd is also good ;-)

+	if (status == 0)
+		finish_rebase(opts);
+	else if (status == 2) {
+		struct strbuf dir = STRBUF_INIT;
+
+		apply_autostash();
+		strbuf_addstr(&dir, opts->state_dir);
+		remove_dir_recursively(&dir, 0);
+		strbuf_release(&dir);
+		die("Nothing to do");
+	}
+
+	strbuf_release(&script_snippet);
+
+	return status ? -1 : 0;
+}
+
 int cmd_rebase(int argc, const char **argv, const char *prefix)
 {
+	struct rebase_options options = { -1 };

The first field of this struct is "enum rebase_type" defined without
"REBASE_TYPE_UNSPECIFIED = -1" in it.  It probably makes sense to
add that constant there so that you can spell this value out.

Have "default:" here that barfs with BUG() to help future
introduction of bugs as the code to set options.type grows
complexity over time.

+	}
+	if (!options.root) {
+		if (argc != 2)
+			die("TODO: handle @{upstream}");
+		else {
+			options.upstream_name = argv[1];
+			argc--;
+			argv++;
+			if (!strcmp(options.upstream_name, "-"))
+				options.upstream_name = "@{-1}";
+		}
+		options.upstream = peel_committish(options.upstream_name);
+		if (!options.upstream)
+			die(_("invalid upstream '%s'"), options.upstream_name);
+	} else
+		die("TODO: upstream for --root");
+
+	/* Make sure the branch to rebase onto is valid. */
+	if (!options.onto_name)
+		options.onto_name = options.upstream_name;
+	if (strstr(options.onto_name, "...")) {
+		die("TODO");
+	} else {
+		options.onto = peel_committish(options.onto_name);
+		if (!options.onto)
+			die(_("Does not point to a valid commit '%s'"),
+				options.onto_name);
+	}
+
+	/*
+	* If the branch to rebase is given, that is the branch we will rebase

Style: align asterisks.

+	* branch_name -- branch/commit being rebased, or HEAD (already detached)
+	* orig_head -- commit object name of tip of the branch before rebasing
+	* head_name -- refs/heads/<that-branch> or "detached HEAD"
+	*/
+	if (argc > 1)
+		 die ("TODO: handle switch_to");

Style: no SP between "die" and "("