From: Phillip Wood Date: Tue, 29 Aug 2023 15:55:30 GMT Subject: Re: [PATCH 02/22] sequencer: mark repository argument as unused Message-ID: In-Reply-To: <20230828214717.GB3831137@coredump.intra.peff.net> Hi Peff On 28/08/2023 22:47, Jeff King wrote: > 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. > 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 > Signed-off-by: Jeff King > --- > 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;