| Message ID | 20230828214717.GB3831137@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | c9f7b1e8f27fac13656eba21261a4a12df23c751 |
| Headers | show |
| Series | YAUPS: Yet Another Unused Parameter Series | expand |
On Mon, Aug 28, 2023 at 05:47:17PM -0400, Jeff King wrote: > 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. Yeah, I think that this (and the discussion on the previous patch) are hinting that we should consider dropping the repository argument more broadly throughout the sequencer API, since we can often use `opts->revs->repo` in its place. But we can definitely leave that for another day ;-). In the meantime, this patch LGTM. Thanks, Taylor
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 <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;
On Tue, Aug 29, 2023 at 04:55:30PM +0100, Phillip Wood wrote: > > 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. Yeah. One of the things that gave me pause is that some of them may need to be renamed, as well. Most of the existing ones are static local within sequencer.c, so names like git_path_head_file() are OK. But REPO_GIT_PATH_FUNC() requires a pointer in the global repo_cache_path, so the names need to be appropriate for a global view. > > 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. Thanks for digging there. I usually do something similar for these patches (to help verify that there's no extra bug lurking), but with "struct repository" ones, the answer was usually uninteresting ("it should be used, but the underlying functions don't support it" kind of thing). Since I'm re-rolling anyway, I'll throw your research into the commit message. -Peff
On 30/08/2023 00:32, Jeff King wrote: > On Tue, Aug 29, 2023 at 04:55:30PM +0100, Phillip Wood wrote: > >>> 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. > > Yeah. One of the things that gave me pause is that some of them may need > to be renamed, as well. Most of the existing ones are static local > within sequencer.c, so names like git_path_head_file() are OK. But > REPO_GIT_PATH_FUNC() requires a pointer in the global repo_cache_path, > so the names need to be appropriate for a global view. It's definitely worth leaving that for a separate patch series. A lot of the paths start rebase_path_... (I think it is only the paths used by cherry-pick/revert that start git_path_...) so we'd need a separate macro unless we renamed them. Having a different prefix is probably a good thing for making them global functions but it still leaves the problem of renaming the ones like git_path_head_file(). Best Wishes Phillip
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;
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, 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. Signed-off-by: Jeff King <peff@peff.net> --- sequencer.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-)