Re: [PATCH 01/22] sequencer: use repository parameter in short_commit_name()

[Taylor Blau <me@ttaylorr.com>]

On Mon, Aug 28, 2023 at 03:21:11PM -0700, Junio C Hamano wrote:
> > @@ -3172,7 +3171,8 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
> >  		item->offset_in_buf = todo_list->buf.len;
> >  		subject_len = find_commit_subject(commit_buffer, &subject);
> >  		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
> > -			short_commit_name(commit), subject_len, subject);
> > +			short_commit_name(opts->revs->repo, commit),
> > +			subject_len, subject);
> >  		repo_unuse_commit_buffer(the_repository, commit,
> >  					 commit_buffer);
>
> But how do we ascertain that opts->revs->repo is always safe to use
> (iow initialized to a sensible value)?  repo_init_revisions() takes
> a repository as its parameter and the first thing it does is to set
> it to the revs->repo, so it is safe for any "struct rev_info" that
> came from there, but REV_INFO_INIT omits initializer for the .repo
> member.
>
> The other two calls in this loop refer to the_repository so the
> current code would be safe even if opts->revs->repo is set or NULL,
> but that will no longer be true with the updated code.  I'd feel
> safer if at the beginning of the function we create a local variable
> "struct rev_info *repo" that is initialized to opts->revs->repo and
> use it throughout the function instead of the_repository.

This call comes from sequencer_pick_revisions() ->
walk_revs_populate_todo(), where opts is passed in from the caller of
the former function.

The sole caller of that function is run_sequencer() in builtin/revert.c.
The relevant portion of that function is:

    if (cmd) {
        opts->revs = NULL;
    } else {
        struct setup_revision_opt s_r_opt;
        opts->revs = xmalloc(sizeof(*opts->revs));
        repo_init_revisions(the_repository, opts->revs, NULL);
        opts->revs->no_walk = 1;
        opts->revs->unsorted_input = 1;
        /* ... */
    }

So as long as we end up in the else arm of this conditional, we'll have
called repo_init_revisions(), causing opts->revs->repo to be equal to
the_repository.

Thankfully, we have an assert(opts->revs) at the beginning of
sequencer_pick_revisions(), so we know that we always take the else arm
on this particular call path.

...but, sequencer_pick_revisions() itself takes a repository pointer, so
we could equally use that, or drop it, like so:

--- 8< ---
diff --git a/builtin/revert.c b/builtin/revert.c
index e6f9a1ad26..29e215c72a 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -224,7 +224,7 @@ static int run_sequencer(int argc, const char **argv, const char *prefix,
 		return sequencer_rollback(the_repository, opts);
 	if (cmd == 's')
 		return sequencer_skip(the_repository, opts);
-	return sequencer_pick_revisions(the_repository, opts);
+	return sequencer_pick_revisions(opts);
 }

 int cmd_revert(int argc, const char **argv, const char *prefix)
diff --git a/sequencer.c b/sequencer.c
index 48475d1cc6..bc7e687623 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -5201,14 +5201,15 @@ static int single_pick(struct repository *r,
 	return do_pick_commit(r, &item, opts, 0, &check_todo);
 }

-int sequencer_pick_revisions(struct repository *r,
-			     struct replay_opts *opts)
+int sequencer_pick_revisions(struct replay_opts *opts)
 {
 	struct todo_list todo_list = TODO_LIST_INIT;
 	struct object_id oid;
+	struct repository *r;
 	int i, res;

 	assert(opts->revs);
+	r = opts->revs->repo;
 	if (read_and_refresh_cache(r, opts))
 		return -1;

diff --git a/sequencer.h b/sequencer.h
index 913a0f652d..10caa3dc93 100644
--- a/sequencer.h
+++ b/sequencer.h
@@ -158,8 +158,7 @@ void todo_list_filter_update_refs(struct repository *r,

 /* Call this to setup defaults before parsing command line options */
 void sequencer_init_config(struct replay_opts *opts);
-int sequencer_pick_revisions(struct repository *repo,
-			     struct replay_opts *opts);
+int sequencer_pick_revisions(struct replay_opts *opts);
 int sequencer_continue(struct repository *repo, struct replay_opts *opts);
 int sequencer_rollback(struct repository *repo, struct replay_opts *opts);
 int sequencer_skip(struct repository *repo, struct replay_opts *opts);
--- >8 ---

though it makes for an awkward API to have all of the other
sequencer-related functions take as their first argument a pointer to a
repository struct, leaving sequencer_pick_revisions() as the odd one
out.

So I'd probably just as soon drop that and do what Junio suggests:

--- 8< ---
diff --git a/sequencer.c b/sequencer.c
index 82dc3e160e..6c06b8e1bb 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3143,22 +3143,24 @@ int write_basic_state(struct replay_opts *opts, const char *head_name,
 }

 static int walk_revs_populate_todo(struct todo_list *todo_list,
-				struct replay_opts *opts)
+				   struct replay_opts *opts)
 {
 	enum todo_command command = opts->action == REPLAY_PICK ?
 		TODO_PICK : TODO_REVERT;
 	const char *command_string = todo_command_info[command].str;
 	const char *encoding;
 	struct commit *commit;
+	struct repository *r;

 	if (prepare_revs(opts))
 		return -1;
+	r = opts->revs->repo;

 	encoding = get_log_output_encoding();

 	while ((commit = get_revision(opts->revs))) {
 		struct todo_item *item = append_new_todo(todo_list);
-		const char *commit_buffer = repo_logmsg_reencode(the_repository,
+		const char *commit_buffer = repo_logmsg_reencode(r,
 								 commit, NULL,
 								 encoding);
 		const char *subject;
@@ -3173,8 +3175,7 @@ static int walk_revs_populate_todo(struct todo_list *todo_list,
 		strbuf_addf(&todo_list->buf, "%s %s %.*s\n", command_string,
 			short_commit_name(opts->revs->repo, commit),
 			subject_len, subject);
-		repo_unuse_commit_buffer(the_repository, commit,
-					 commit_buffer);
+		repo_unuse_commit_buffer(r, commit, commit_buffer);
 	}

 	if (!todo_list->nr)
--- >8 ---

> > @@ -5564,7 +5564,7 @@ static int make_script_with_merges(struct pretty_print_context *pp,
> >  		if (!is_empty && (commit->object.flags & PATCHSAME)) {
> >  			if (flags & TODO_LIST_WARN_SKIPPED_CHERRY_PICKS)
> >  				warning(_("skipped previously applied commit %s"),
> > -					short_commit_name(commit));
> > +					short_commit_name(revs->repo, commit));
> >  			skipped_commit = 1;
> >  			continue;
> >  		}
>
> This one I am fairly certain is a safe and correct conversion; the
> function would be segfaulting in its call to get_revision() if
> revs->repo were set to a bogus value.

Agreed.

Thanks,
Taylor