Re: [PATCH 02/22] sequencer: mark repository argument as unused

From: Phillip Wood <phillip.wood123@gmail.com>
Date: Aug 29, 2023, 15:55 UTC
Message-ID: <e65c93ec-0221-492b-b538-450b60cf1e59@gmail.com>
In-Reply-To: <20230828214717.GB3831137@coredump.intra.peff.net>

Hi Peff

On 28/08/2023 22:47, Jeff King wrote:

Show 8 quoted lines

> In sequencer_get_last_command(), we don't ever look at the repository
> parameter. It _should_ be used when calling into git_path_* functions,
> but the one we use here is declared with the non-REPO variant of
> GIT_PATH_FUNC(), and so just uses the_repository internally.
> 
> We could change the path helper to use REPO_GIT_PATH_FUNC(), but doing
> so piecemeal is not great. There are 41 uses of GIT_PATH_FUNC() in
> sequencer.c,

Wow, I knew there were quite a few but I hadn't realized there were that many. Changing them all to take a struct repository will be a big change and will make struct repo_cache_path a lot larger.

Show 13 quoted lines

> and inconsistently switching one makes the code more
> confusing. Likewise, this one function is used in half a dozen other
> spots, all of which would need to start passing in a repository argument
> (with rippling effects up the call stack).
> 
> So let's punt on that for now and just silence any -Wunused-parameter
> warning.
> 
> Note that we could also drop this parameter entirely, as the function is
> always called directly, and not as a callback that has to conform to
> some external interface. But since we'd eventually want to use the
> repository parameter, let's leave it in place to avoid disrupting the
> callers twice.

I think that makes sense as we're going to need that argument eventually. I was curious as to why this function takes a repository argument. When the function was added in 4a72486de97 (fix cherry-pick/revert status after commit, 2019-04-16) it called parse_insn_line() which takes a repository argument. It was refactored in ed5b1ca10b (status: do not report errors in sequencer/todo, 2019-06-27) and I failed to notice that the repository was unused afterwards.

Best Wishes

Phillip

Show 18 quoted lines

> Signed-off-by: Jeff King <peff@peff.net>
> ---
>   sequencer.c | 2 +-
>   1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index 82dc3e160e..41fd79d215 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2649,7 +2649,7 @@ static int parse_insn_line(struct repository *r, struct todo_item *item,
>   	return item->commit ? 0 : -1;
>   }
>   
> -int sequencer_get_last_command(struct repository *r, enum replay_action *action)
> +int sequencer_get_last_command(struct repository *r UNUSED, enum replay_action *action)
>   {
>   	const char *todo_file, *bol;
>   	struct strbuf buf = STRBUF_INIT;