Re: [PATCH v1] rebase - recycle

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Apr 22 2022, Edmundo Carmona Antoranz wrote:

Just minor nits from a quick read. Not on the main logic, just syntax
etc. issues:

>  #define DEFAULT_REFLOG_ACTION "rebase"
>  
> @@ -49,7 +52,8 @@ static GIT_PATH_FUNC(merge_dir, "rebase-merge")
>  enum rebase_type {
>  	REBASE_UNSPECIFIED = -1,
>  	REBASE_APPLY,
> -	REBASE_MERGE
> +	REBASE_MERGE,
> +	REBASE_RECYCLE
>  };
>  
>  enum empty_type {
> @@ -83,6 +87,8 @@ struct rebase_options {
>  		REBASE_DIFFSTAT = 1<<2,
>  		REBASE_FORCE = 1<<3,
>  		REBASE_INTERACTIVE_EXPLICIT = 1<<4,
> +		REBASE_RECYCLE_OR_FAIL = 1 << 5,
> +		REBASE_ATTEMPT_RECYCLE = 1<<6,

Needs consistent whitespace for <<.

>  	} flags;
>  	struct strvec git_am_opts;
>  	const char *action;
> @@ -104,6 +110,16 @@ struct rebase_options {
>  	int fork_point;
>  };
>  
> +struct recycle_parent_mapping {
> +	struct oidmap_entry e;
> +	struct commit *new_parent;
> +};
> +
> +struct recycle_progress_info {
> +	struct progress *progress;
> +	int commits;
> +};
> +
>  #define REBASE_OPTIONS_INIT {			  	\
>  		.type = REBASE_UNSPECIFIED,	  	\
>  		.empty = EMPTY_UNSPECIFIED,	  	\
> @@ -384,6 +400,12 @@ static int is_merge(struct rebase_options *opts)
>  	return opts->type == REBASE_MERGE;
>  }
>  
> +static int can_recycle(struct rebase_options *opts)
> +{
> +	return oideq(get_commit_tree_oid(opts->onto),
> +		     get_commit_tree_oid(opts->upstream));
> +}
> +
>  static void imply_merge(struct rebase_options *opts, const char *option)
>  {
>  	switch (opts->type) {
> @@ -771,6 +793,173 @@ static int run_specific_rebase(struct rebase_options *opts, enum action action)
>  	return status ? -1 : 0;
>  }
>  
> +static struct commit *recycle_commit(struct commit *orig_commit,
> +				     struct oidmap *parents)
> +{
> +	const char *body;
> +	size_t body_length;
> +	const char *author_raw;
> +	size_t author_length;
> +	struct strbuf author = STRBUF_INIT;
> +	const char *message;
> +	size_t message_length;
> +	int result;
> +

We tend not to \n\n-break the variables.


> +	if (result)
> +		die("Could not create a recycled revision for %s\n",

Error messages should not start wth capital letters, so "could.." not
"Could". Also needs _() markings for translation. Then don't add a \n.

> +	struct recycle_parent_mapping *mapping;
> +	mapping = xmalloc(sizeof(*mapping));

Add \n\n between variable decls & code.

In this case though we can just put the xmalloc() with decl...


> +	struct commit *orig_head;
> +	struct commit *new_head = NULL;
> +	struct commit *commit;
> +	struct commit_list *old_commits = NULL;
> +	struct commit_list *old_commit;
> +	struct oidmap parents;
> +	struct progress *progress = NULL;
> +	int commit_counter = 0;
> +
> +	init_revisions(&revs, NULL);
> +	revs.commit_format = CMIT_FMT_RAW;
> +	orig_head = lookup_commit_or_die(&opts->orig_head, "head");

Just declare this with the variable, seems it doesn't need
init_revisions, or does it...?

> +
> +	opts->upstream->object.flags |= UNINTERESTING;
> +	add_pending_object(&revs, &opts->upstream->object, "upstream");
> +	add_pending_object(&revs, &orig_head->object, "head");
> +
> +	if (prepare_revision_walk(&revs))
> +		die("Could not get commits to recycle");

ditto capital letters, _() etc.

> +	if (isatty(2)) {

Skip {} for one-statement if's.

> +		start_delayed_progress(_("Recycling commits"),

Missing the assignment to progerss, so this never works, presumably...

> +				       commit_list_count(old_commits));
> +	}
> +
> +	old_commit = old_commits;
> +	while (old_commit) {
> +		display_progress(progress, ++commit_counter);

I.e. still NULL here...

> +		new_head = recycle_commit(old_commit->item, &parents);
> +		recycle_save_parent_mapping(&parents, old_commit->item,
> +					    new_head);
> +		old_commit = old_commit->next;
> +	}
> +
> +	stop_progress(&progress);
> +
> +	return new_head;
> +}
> +
> +static void recycle_wrapup(struct rebase_options *opts,
> +			   const char *branch_name, struct commit *new_head)
> +{
> +	struct strvec args = STRVEC_INIT;

\n\n again.

> +	if (opts->head_name) {
> +		struct wt_status s = { 0 };
> +
> +		s.show_branch = 1;
> +		wt_status_prepare(the_repository, &s);
> +		wt_status_collect(&s);
> +		if (!strcmp(s.branch, opts->head_name)) {
> +			struct reset_head_opts ropts = { 0 };
> +			struct strbuf msg = STRBUF_INIT;

ditto.

> +			strbuf_addf(&msg, "rebase recycle: "
> +				    "moving to %s",
> +				    oid_to_hex(&new_head->object.oid));
> +			ropts.oid = &new_head->object.oid;
> +			ropts.orig_head = &opts->orig_head,
> +			ropts.flags = RESET_HEAD_HARD |
> +				      RESET_HEAD_RUN_POST_CHECKOUT_HOOK;
> +			ropts.head_msg = msg.buf;
> +			ropts.default_reflog_action = DEFAULT_REFLOG_ACTION;
> +			if (reset_head(the_repository, &ropts))
> +				die(_("Could not reset"));
> +			strbuf_release(&msg);
> +		} else {
> +			update_ref(NULL, opts->head_name,
> +				   &new_head->object.oid, NULL,
> +				   0, UPDATE_REFS_DIE_ON_ERR);
> +
> +			strvec_pushf(&args, "checkout");
> +			strvec_pushf(&args, "--quiet");
> +			strvec_pushf(&args, "%s", branch_name);

You want just strvec_pushl() here., or strvec_push(), but definitely not
strvec_pushf(). You're not using the formatting.

> +
> +			run_command_v_opt(args.v, RUN_GIT_CMD);
> +			strvec_clear(&args);
> +		}
> +	} else {
> +		strvec_pushf(&args, "checkout");
> +		strvec_pushf(&args, "--quiet");
> +		strvec_pushf(&args, "%s", oid_to_hex(&new_head->object.oid));

Ditto.

> +
> +		run_command_v_opt(args.v, RUN_GIT_CMD);
> +		strvec_clear(&args);
> +	}
> +}
> +
>  static int rebase_config(const char *var, const char *value, void *data)
>  {
>  	struct rebase_options *opts = data;
> @@ -1154,6 +1343,12 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  			 N_("automatically re-schedule any `exec` that fails")),
>  		OPT_BOOL(0, "reapply-cherry-picks", &options.reapply_cherry_picks,
>  			 N_("apply all changes, even those already present upstream")),
> +		OPT_BIT(0, "recycle", &options.flags,
> +			N_("Run a recycle, if possible. Fails otherwise."),

run not Run, and drop the "." at the end.

> +			REBASE_RECYCLE_OR_FAIL),
> +		OPT_BIT(0, "attempt-recycle", &options.flags,
> +			N_("Run a recycle, if possible. Continue with other approaches if it can't be done."),

Ditto.

Also I think you want "OPT_BOOL"...?

> +			REBASE_ATTEMPT_RECYCLE),
>  		OPT_END(),
>  	};
>  	int i;
> @@ -1234,6 +1429,20 @@ int cmd_rebase(int argc, const char **argv, const char *prefix)
>  		die(_("The --edit-todo action can only be used during "
>  		      "interactive rebase."));
>  
> +	if (options.flags & (REBASE_RECYCLE_OR_FAIL | REBASE_ATTEMPT_RECYCLE)) {
> +		if ((options.flags & (REBASE_RECYCLE_OR_FAIL | REBASE_ATTEMPT_RECYCLE)) ==
> +		    (REBASE_RECYCLE_OR_FAIL | REBASE_ATTEMPT_RECYCLE))
> +			die(_("Can't use both --recycle and --attempt-recycle."));

You can use OPT_CMDMODE() to declare flags that are mutually exclusive,
but maybe it's not a good fit in this case.

> +		if (options.flags & REBASE_INTERACTIVE_EXPLICIT)
> +			die(_("Can't use --recycle/--attempt-recycle with interactive mode."));
> +		if (options.strategy) {
> +			die(_("Can't specify a strategy when using --recycle/--atempt-recycle."));
> +		}
> +		if (options.signoff) {
> +			die(_("Can't use --signoff with --recycle/--atempt-recycle"));
> +		}

Aside from capital, _() etc. these can also drop {}'s

> +				printf(_("Recycled %s onto %s.\n"),
> +					branch_name, options.onto_name);
> +				recycle_wrapup(&options, branch_name,
> +					       new_head);
> +				ret = 0;
> +				goto cleanup;
> +			}
> +		} else
> +			printf(_("upstream and onto do not share the same tree. "
> +				 "Can't run a recycle.\n"));

Don't use printf() when puts() would do (the latter case).

The else is missing {} (see CodingGuidelines). I.e. if one arm gets it
all of them get it.