Jeff King <peff@xxxxxxxx> 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.
