Re: [PATCH 01/22] sequencer: use repository parameter in short_commit_name()
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Aug 28, 2023, 22:21 UTC
- Message-ID
- <xmqqsf82g65k.fsf@gitster.g>
- In-Reply-To
- <20230828214629.GA3831137@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 7 quoted lines
> -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 ...
Show 7 quoted lines
> 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.
Show 6 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.
Show 9 quoted lines
> @@ -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.