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

[Junio C Hamano <gitster@pobox.com>]

Jeff King <peff@peff.net> writes:

> -static const char *short_commit_name(struct commit *commit)
> +static const char *short_commit_name(struct repository *r, struct commit *commit)
>  {
> -	return repo_find_unique_abbrev(the_repository, &commit->object.oid,
> -				       DEFAULT_ABBREV);
> +	return repo_find_unique_abbrev(r, &commit->object.oid, DEFAULT_ABBREV);
>  }

Theoretically this is the right thing to do, and ...

>  static int get_message(struct commit *commit, struct commit_message *out)
> @@ -446,7 +445,7 @@ static int get_message(struct commit *commit, struct commit_message *out)
>  
>  	out->message = repo_logmsg_reencode(the_repository, commit, NULL,
>  					    get_commit_output_encoding());
> -	abbrev = short_commit_name(commit);
> +	abbrev = short_commit_name(the_repository, commit);

... this is a no-op.

> @@ -2383,7 +2382,7 @@ static int do_pick_commit(struct repository *r,
>  		error(command == TODO_REVERT
>  		      ? _("could not revert %s... %s")
>  		      : _("could not apply %s... %s"),
> -		      short_commit_name(commit), msg.subject);
> +		      short_commit_name(r, commit), msg.subject);

And this is a logical consequence for a function that is told by the
caller in which repository to work in via its parameter.

> @@ -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.


> @@ -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.

Thanks.