* [PATCH 0/6] unused function parameter potpourri @ 2022-08-19 8:48 Jeff King 2022-08-19 8:49 ` [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() Jeff King ` (6 more replies) 0 siblings, 7 replies; 21+ messages in thread From: Jeff King @ 2022-08-19 8:48 UTC (permalink / raw) To: git Here are a few small cleanups of unused function parameters. The first five just drop the unused parameters. These are all trivially correct, since otherwise the compiler would complain. But I tried to make sure that dropping was the right thing in each (rather than it being a bug where the parameter should have been used). The final one just uses the parameters for an assertion, following a pattern we've used before. I'll try to cc the individual authors for each patch. [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() [2/6]: log-tree: drop unused commit param in remerge_diff() [3/6]: match_pathname(): drop unused "flags" parameter [4/6]: verify_one_sparse(): drop unused repository parameter [5/6]: reftable: drop unused parameter from reader_seek_linear() [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks attr.c | 2 +- builtin/reflog.c | 4 ++++ cache-tree.c | 6 ++---- dir.c | 6 ++---- dir.h | 2 +- log-tree.c | 5 ++--- reftable/reader.c | 6 +++--- xdiff/xdiffi.c | 2 +- xdiff/xdiffi.h | 3 +-- xdiff/xhistogram.c | 3 +-- 10 files changed, 18 insertions(+), 21 deletions(-) -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King @ 2022-08-19 8:49 ` Jeff King 2022-08-19 13:32 ` Phillip Wood 2022-08-19 8:50 ` [PATCH 2/6] log-tree: drop unused commit param in remerge_diff() Jeff King ` (5 subsequent siblings) 6 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2022-08-19 8:49 UTC (permalink / raw) To: git; +Cc: Phillip Wood These are no longer used since 9df0fc3d57 (xdiff: fix a memory leak, 2022-02-16), as the caller is expected to call xdl_prepare_env() itself. After that change the histogram code only examines the prepared xdfenv_t, not the original buffers. Signed-off-by: Jeff King <peff@peff.net> --- xdiff/xdiffi.c | 2 +- xdiff/xdiffi.h | 3 +-- xdiff/xhistogram.c | 3 +-- 3 files changed, 3 insertions(+), 5 deletions(-) diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c index 53e803e6bc..8c64519eac 100644 --- a/xdiff/xdiffi.c +++ b/xdiff/xdiffi.c @@ -326,7 +326,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, } if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) { - res = xdl_do_histogram_diff(mf1, mf2, xpp, xe); + res = xdl_do_histogram_diff(xpp, xe); goto out; } diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h index 8f1c7c8b04..9d988e0263 100644 --- a/xdiff/xdiffi.h +++ b/xdiff/xdiffi.h @@ -58,7 +58,6 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, xdemitconf_t const *xecfg); int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, xdfenv_t *env); -int xdl_do_histogram_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, - xdfenv_t *env); +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env); #endif /* #if !defined(XDIFFI_H) */ diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c index df909004c1..16a8fe2f3f 100644 --- a/xdiff/xhistogram.c +++ b/xdiff/xhistogram.c @@ -362,8 +362,7 @@ static int histogram_diff(xpparam_t const *xpp, xdfenv_t *env, return result; } -int xdl_do_histogram_diff(mmfile_t *file1, mmfile_t *file2, - xpparam_t const *xpp, xdfenv_t *env) +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env) { return histogram_diff(xpp, env, env->xdf1.dstart + 1, env->xdf1.dend - env->xdf1.dstart + 1, -- 2.37.2.928.g0821088f4a ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() 2022-08-19 8:49 ` [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() Jeff King @ 2022-08-19 13:32 ` Phillip Wood 2022-08-20 6:58 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Phillip Wood @ 2022-08-19 13:32 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Phillip Wood Hi Peff On Fri, 19 Aug 2022 at 09:49, Jeff King <peff@peff.net> wrote: > > These are no longer used since 9df0fc3d57 (xdiff: fix a memory leak, > 2022-02-16), as the caller is expected to call xdl_prepare_env() itself. > After that change the histogram code only examines the prepared > xdfenv_t, not the original buffers. Thanks, I seem to have a blind spot for unused parameters (I think this is at least the third such fix from you for one of my commits), I'm really looking forward to having -Wunused-parameter enabled, thanks for working on it. Looking at the xpatience.c I think we can remove the mmfile_t parameters there as well, they are only end up being used because patience_diff() gets called recursively. I'm about to go off list for a week, but I can look at putting a patch together for that when I get back unless you want to. Best Wishes Phillip > > Signed-off-by: Jeff King <peff@peff.net> > --- > xdiff/xdiffi.c | 2 +- > xdiff/xdiffi.h | 3 +-- > xdiff/xhistogram.c | 3 +-- > 3 files changed, 3 insertions(+), 5 deletions(-) > > diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c > index 53e803e6bc..8c64519eac 100644 > --- a/xdiff/xdiffi.c > +++ b/xdiff/xdiffi.c > @@ -326,7 +326,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, > } > > if (XDF_DIFF_ALG(xpp->flags) == XDF_HISTOGRAM_DIFF) { > - res = xdl_do_histogram_diff(mf1, mf2, xpp, xe); > + res = xdl_do_histogram_diff(xpp, xe); > goto out; > } > > diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h > index 8f1c7c8b04..9d988e0263 100644 > --- a/xdiff/xdiffi.h > +++ b/xdiff/xdiffi.h > @@ -58,7 +58,6 @@ int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, > xdemitconf_t const *xecfg); > int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, > xdfenv_t *env); > -int xdl_do_histogram_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, > - xdfenv_t *env); > +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env); > > #endif /* #if !defined(XDIFFI_H) */ > diff --git a/xdiff/xhistogram.c b/xdiff/xhistogram.c > index df909004c1..16a8fe2f3f 100644 > --- a/xdiff/xhistogram.c > +++ b/xdiff/xhistogram.c > @@ -362,8 +362,7 @@ static int histogram_diff(xpparam_t const *xpp, xdfenv_t *env, > return result; > } > > -int xdl_do_histogram_diff(mmfile_t *file1, mmfile_t *file2, > - xpparam_t const *xpp, xdfenv_t *env) > +int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env) > { > return histogram_diff(xpp, env, > env->xdf1.dstart + 1, env->xdf1.dend - env->xdf1.dstart + 1, > -- > 2.37.2.928.g0821088f4a > ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() 2022-08-19 13:32 ` Phillip Wood @ 2022-08-20 6:58 ` Jeff King 2022-08-20 7:36 ` [PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff() Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2022-08-20 6:58 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Phillip Wood On Fri, Aug 19, 2022 at 02:32:13PM +0100, Phillip Wood wrote: > Thanks, I seem to have a blind spot for unused parameters (I think > this is at least the third such fix from you for one of my commits), > I'm really looking forward to having -Wunused-parameter enabled, > thanks for working on it. I think everyone does. ;) I know I couldn't have found these without help from the compiler. > Looking at the xpatience.c I think we can remove the mmfile_t > parameters there as well, they are only end up being used because > patience_diff() gets called recursively. I'm about to go off list for > a week, but I can look at putting a patch together for that when I get > back unless you want to. I wondered that, too, but they also get passed in to fill_hashmap(), which records the pointers in its "struct hashmap". It looks like those fields are never accessed, though, and could even be removed from the struct entirely! So I think there is some room for cleanup. -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff() 2022-08-20 6:58 ` Jeff King @ 2022-08-20 7:36 ` Jeff King 2022-08-26 13:15 ` Phillip Wood 0 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2022-08-20 7:36 UTC (permalink / raw) To: Phillip Wood; +Cc: Git Mailing List, Phillip Wood, Junio C Hamano On Sat, Aug 20, 2022 at 02:58:44AM -0400, Jeff King wrote: > > Looking at the xpatience.c I think we can remove the mmfile_t > > parameters there as well, they are only end up being used because > > patience_diff() gets called recursively. I'm about to go off list for > > a week, but I can look at putting a patch together for that when I get > > back unless you want to. > > I wondered that, too, but they also get passed in to fill_hashmap(), > which records the pointers in its "struct hashmap". It looks like those > fields are never accessed, though, and could even be removed from the > struct entirely! > > So I think there is some room for cleanup. And here's a patch, so we don't forget about it. It could be applied independently of this series, but there's a small textual dependency since we're touching a line close to the related histogram cleanup. Thanks for mentioning this. I had given only a cursory glance before, and just assumed it would be hard to trace. It turned out to be fun. :) Junio: this could go on top of the jk/unused-fixes topic. -- >8 -- Subject: xdiff: drop unused mmfile parameters from xdl_do_patience_diff() The entry point to the patience-diff algorithm takes two mmfile_t structs with the original file contents, but it doesn't actually do anything useful with them. This is similar to the case recently cleaned up in the histogram code via f1d019071e (xdiff: drop unused mmfile parameters from xdl_do_histogram_diff(), 2022-08-19), but there's a bit more subtlety going on. We pass them into the recursive patience_diff(), which in turn passes them into fill_hashmap(), which stuffs the pointers into a struct. But the only thing which reads the struct fields is our recursion into patience_diff()! So it's unlikely that something like -Wunused-parameter could find this case: it would have to detect the circular dependency caused by the recursion (not to mention tracing across struct field assignments). But once found, it's easy to have the compiler confirm what's going on: 1. Drop the "file1" and "file2" fields from the hashmap struct definition. Remove the assignments in fill_hashmap(), and temporarily substitute NULL in the recursive call to patience_diff(). Compiling shows that no other code touched those fields. 2. Now fill_hashmap() will trigger -Wunused-parameter. Drop "file1" and "file2" from its definition and callsite. 3. Now patience_diff() will trigger -Wunused-parameter. Drop them there, too. One of the callsites is the recursion with our NULL values, so those temporary values go away. 4. Now xdl_do_patience_diff() will trigger -Wunused-parameter. Drop them there. And we're done. Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk> Signed-off-by: Jeff King <peff@peff.net> --- Explaining the 4 steps is probably overkill, but I found it a little interesting how one can bend the compiler to their will here. At least I didn't make it a series of 4 patches. ;) xdiff/xdiffi.c | 2 +- xdiff/xdiffi.h | 3 +-- xdiff/xpatience.c | 23 +++++++---------------- 3 files changed, 9 insertions(+), 19 deletions(-) diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c index 8c64519eac..32652ded2d 100644 --- a/xdiff/xdiffi.c +++ b/xdiff/xdiffi.c @@ -321,7 +321,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, return -1; if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) { - res = xdl_do_patience_diff(mf1, mf2, xpp, xe); + res = xdl_do_patience_diff(xpp, xe); goto out; } diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h index 9d988e0263..126c9d8ff4 100644 --- a/xdiff/xdiffi.h +++ b/xdiff/xdiffi.h @@ -56,8 +56,7 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr); void xdl_free_script(xdchange_t *xscr); int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, xdemitconf_t const *xecfg); -int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, - xdfenv_t *env); +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env); int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env); #endif /* #if !defined(XDIFFI_H) */ diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c index fe39c2978c..a2d8955537 100644 --- a/xdiff/xpatience.c +++ b/xdiff/xpatience.c @@ -69,7 +69,6 @@ struct hashmap { } *entries, *first, *last; /* were common records found? */ unsigned long has_matches; - mmfile_t *file1, *file2; xdfenv_t *env; xpparam_t const *xpp; }; @@ -139,13 +138,10 @@ static void insert_record(xpparam_t const *xpp, int line, struct hashmap *map, * * It is assumed that env has been prepared using xdl_prepare(). */ -static int fill_hashmap(mmfile_t *file1, mmfile_t *file2, - xpparam_t const *xpp, xdfenv_t *env, +static int fill_hashmap(xpparam_t const *xpp, xdfenv_t *env, struct hashmap *result, int line1, int count1, int line2, int count2) { - result->file1 = file1; - result->file2 = file2; result->xpp = xpp; result->env = env; @@ -254,8 +250,7 @@ static int match(struct hashmap *map, int line1, int line2) return record1->ha == record2->ha; } -static int patience_diff(mmfile_t *file1, mmfile_t *file2, - xpparam_t const *xpp, xdfenv_t *env, +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env, int line1, int count1, int line2, int count2); static int walk_common_sequence(struct hashmap *map, struct entry *first, @@ -286,8 +281,7 @@ static int walk_common_sequence(struct hashmap *map, struct entry *first, /* Recurse */ if (next1 > line1 || next2 > line2) { - if (patience_diff(map->file1, map->file2, - map->xpp, map->env, + if (patience_diff(map->xpp, map->env, line1, next1 - line1, line2, next2 - line2)) return -1; @@ -326,8 +320,7 @@ static int fall_back_to_classic_diff(struct hashmap *map, * * This function assumes that env was prepared with xdl_prepare_env(). */ -static int patience_diff(mmfile_t *file1, mmfile_t *file2, - xpparam_t const *xpp, xdfenv_t *env, +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env, int line1, int count1, int line2, int count2) { struct hashmap map; @@ -346,7 +339,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2, } memset(&map, 0, sizeof(map)); - if (fill_hashmap(file1, file2, xpp, env, &map, + if (fill_hashmap(xpp, env, &map, line1, count1, line2, count2)) return -1; @@ -374,9 +367,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2, return result; } -int xdl_do_patience_diff(mmfile_t *file1, mmfile_t *file2, - xpparam_t const *xpp, xdfenv_t *env) +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env) { - return patience_diff(file1, file2, xpp, env, - 1, env->xdf1.nrec, 1, env->xdf2.nrec); + return patience_diff(xpp, env, 1, env->xdf1.nrec, 1, env->xdf2.nrec); } -- 2.37.2.931.g60e3983abc ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff() 2022-08-20 7:36 ` [PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff() Jeff King @ 2022-08-26 13:15 ` Phillip Wood 0 siblings, 0 replies; 21+ messages in thread From: Phillip Wood @ 2022-08-26 13:15 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List, Phillip Wood, Junio C Hamano Hi Peff On Sat, 20 Aug 2022 at 08:36, Jeff King <peff@peff.net> wrote: > > On Sat, Aug 20, 2022 at 02:58:44AM -0400, Jeff King wrote: > > > > Looking at the xpatience.c I think we can remove the mmfile_t > > > parameters there as well, they are only end up being used because > > > patience_diff() gets called recursively. I'm about to go off list for > > > a week, but I can look at putting a patch together for that when I get > > > back unless you want to. > > > > I wondered that, too, but they also get passed in to fill_hashmap(), > > which records the pointers in its "struct hashmap". It looks like those > > fields are never accessed, though, and could even be removed from the > > struct entirely! > > > > So I think there is some room for cleanup. > > And here's a patch, so we don't forget about it. It could be applied > independently of this series, but there's a small textual dependency > since we're touching a line close to the related histogram cleanup. > > Thanks for mentioning this. I had given only a cursory glance before, > and just assumed it would be hard to trace. It turned out to be fun. :) Yes, it took me a couple of minutes to realize that it wasn't doing anything useful with the pointers in "struct hashmap" but it was quite satisfying when I did. The patch looks good, thanks for putting it together. Best Wishes Phillip > Junio: this could go on top of the jk/unused-fixes topic. > > -- >8 -- > Subject: xdiff: drop unused mmfile parameters from xdl_do_patience_diff() > > The entry point to the patience-diff algorithm takes two mmfile_t > structs with the original file contents, but it doesn't actually do > anything useful with them. This is similar to the case recently cleaned > up in the histogram code via f1d019071e (xdiff: drop unused mmfile > parameters from xdl_do_histogram_diff(), 2022-08-19), but there's a bit > more subtlety going on. > > We pass them into the recursive patience_diff(), which in turn passes > them into fill_hashmap(), which stuffs the pointers into a struct. But > the only thing which reads the struct fields is our recursion into > patience_diff()! > > So it's unlikely that something like -Wunused-parameter could find this > case: it would have to detect the circular dependency caused by the > recursion (not to mention tracing across struct field assignments). > > But once found, it's easy to have the compiler confirm what's going on: > > 1. Drop the "file1" and "file2" fields from the hashmap struct > definition. Remove the assignments in fill_hashmap(), and > temporarily substitute NULL in the recursive call to > patience_diff(). Compiling shows that no other code touched those > fields. > > 2. Now fill_hashmap() will trigger -Wunused-parameter. Drop "file1" > and "file2" from its definition and callsite. > > 3. Now patience_diff() will trigger -Wunused-parameter. Drop them > there, too. One of the callsites is the recursion with our > NULL values, so those temporary values go away. > > 4. Now xdl_do_patience_diff() will trigger -Wunused-parameter. Drop > them there. And we're done. > > Suggested-by: Phillip Wood <phillip.wood@dunelm.org.uk> > Signed-off-by: Jeff King <peff@peff.net> > --- > Explaining the 4 steps is probably overkill, but I found it a little > interesting how one can bend the compiler to their will here. At least I > didn't make it a series of 4 patches. ;) > > xdiff/xdiffi.c | 2 +- > xdiff/xdiffi.h | 3 +-- > xdiff/xpatience.c | 23 +++++++---------------- > 3 files changed, 9 insertions(+), 19 deletions(-) > > diff --git a/xdiff/xdiffi.c b/xdiff/xdiffi.c > index 8c64519eac..32652ded2d 100644 > --- a/xdiff/xdiffi.c > +++ b/xdiff/xdiffi.c > @@ -321,7 +321,7 @@ int xdl_do_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, > return -1; > > if (XDF_DIFF_ALG(xpp->flags) == XDF_PATIENCE_DIFF) { > - res = xdl_do_patience_diff(mf1, mf2, xpp, xe); > + res = xdl_do_patience_diff(xpp, xe); > goto out; > } > > diff --git a/xdiff/xdiffi.h b/xdiff/xdiffi.h > index 9d988e0263..126c9d8ff4 100644 > --- a/xdiff/xdiffi.h > +++ b/xdiff/xdiffi.h > @@ -56,8 +56,7 @@ int xdl_build_script(xdfenv_t *xe, xdchange_t **xscr); > void xdl_free_script(xdchange_t *xscr); > int xdl_emit_diff(xdfenv_t *xe, xdchange_t *xscr, xdemitcb_t *ecb, > xdemitconf_t const *xecfg); > -int xdl_do_patience_diff(mmfile_t *mf1, mmfile_t *mf2, xpparam_t const *xpp, > - xdfenv_t *env); > +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env); > int xdl_do_histogram_diff(xpparam_t const *xpp, xdfenv_t *env); > > #endif /* #if !defined(XDIFFI_H) */ > diff --git a/xdiff/xpatience.c b/xdiff/xpatience.c > index fe39c2978c..a2d8955537 100644 > --- a/xdiff/xpatience.c > +++ b/xdiff/xpatience.c > @@ -69,7 +69,6 @@ struct hashmap { > } *entries, *first, *last; > /* were common records found? */ > unsigned long has_matches; > - mmfile_t *file1, *file2; > xdfenv_t *env; > xpparam_t const *xpp; > }; > @@ -139,13 +138,10 @@ static void insert_record(xpparam_t const *xpp, int line, struct hashmap *map, > * > * It is assumed that env has been prepared using xdl_prepare(). > */ > -static int fill_hashmap(mmfile_t *file1, mmfile_t *file2, > - xpparam_t const *xpp, xdfenv_t *env, > +static int fill_hashmap(xpparam_t const *xpp, xdfenv_t *env, > struct hashmap *result, > int line1, int count1, int line2, int count2) > { > - result->file1 = file1; > - result->file2 = file2; > result->xpp = xpp; > result->env = env; > > @@ -254,8 +250,7 @@ static int match(struct hashmap *map, int line1, int line2) > return record1->ha == record2->ha; > } > > -static int patience_diff(mmfile_t *file1, mmfile_t *file2, > - xpparam_t const *xpp, xdfenv_t *env, > +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env, > int line1, int count1, int line2, int count2); > > static int walk_common_sequence(struct hashmap *map, struct entry *first, > @@ -286,8 +281,7 @@ static int walk_common_sequence(struct hashmap *map, struct entry *first, > > /* Recurse */ > if (next1 > line1 || next2 > line2) { > - if (patience_diff(map->file1, map->file2, > - map->xpp, map->env, > + if (patience_diff(map->xpp, map->env, > line1, next1 - line1, > line2, next2 - line2)) > return -1; > @@ -326,8 +320,7 @@ static int fall_back_to_classic_diff(struct hashmap *map, > * > * This function assumes that env was prepared with xdl_prepare_env(). > */ > -static int patience_diff(mmfile_t *file1, mmfile_t *file2, > - xpparam_t const *xpp, xdfenv_t *env, > +static int patience_diff(xpparam_t const *xpp, xdfenv_t *env, > int line1, int count1, int line2, int count2) > { > struct hashmap map; > @@ -346,7 +339,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2, > } > > memset(&map, 0, sizeof(map)); > - if (fill_hashmap(file1, file2, xpp, env, &map, > + if (fill_hashmap(xpp, env, &map, > line1, count1, line2, count2)) > return -1; > > @@ -374,9 +367,7 @@ static int patience_diff(mmfile_t *file1, mmfile_t *file2, > return result; > } > > -int xdl_do_patience_diff(mmfile_t *file1, mmfile_t *file2, > - xpparam_t const *xpp, xdfenv_t *env) > +int xdl_do_patience_diff(xpparam_t const *xpp, xdfenv_t *env) > { > - return patience_diff(file1, file2, xpp, env, > - 1, env->xdf1.nrec, 1, env->xdf2.nrec); > + return patience_diff(xpp, env, 1, env->xdf1.nrec, 1, env->xdf2.nrec); > } > -- > 2.37.2.931.g60e3983abc > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 2/6] log-tree: drop unused commit param in remerge_diff() 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King 2022-08-19 8:49 ` [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() Jeff King @ 2022-08-19 8:50 ` Jeff King 2022-08-19 23:05 ` Elijah Newren 2022-08-19 8:50 ` [PATCH 3/6] match_pathname(): drop unused "flags" parameter Jeff King ` (4 subsequent siblings) 6 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2022-08-19 8:50 UTC (permalink / raw) To: git; +Cc: Elijah Newren This function has never used its "commit" parameter since it was added in db757e8b8d (show, log: provide a --remerge-diff capability, 2022-02-02). This makes sense; we already have separate parameters for the parents (which lets us redo the merge) and the oid of the result tree (which we can then diff against the remerge result). Let's drop the unused parameter in the name of clarity. Signed-off-by: Jeff King <peff@peff.net> --- log-tree.c | 5 ++--- 1 file changed, 2 insertions(+), 3 deletions(-) diff --git a/log-tree.c b/log-tree.c index d0ac0a6327..82d9b5f650 100644 --- a/log-tree.c +++ b/log-tree.c @@ -956,8 +956,7 @@ static void cleanup_additional_headers(struct diff_options *o) static int do_remerge_diff(struct rev_info *opt, struct commit_list *parents, - struct object_id *oid, - struct commit *commit) + struct object_id *oid) { struct merge_options o; struct commit_list *bases; @@ -1052,7 +1051,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log "for octopus merges.\n"); return 1; } - return do_remerge_diff(opt, parents, oid, commit); + return do_remerge_diff(opt, parents, oid); } if (opt->combine_merges) return do_diff_combined(opt, commit); -- 2.37.2.928.g0821088f4a ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] log-tree: drop unused commit param in remerge_diff() 2022-08-19 8:50 ` [PATCH 2/6] log-tree: drop unused commit param in remerge_diff() Jeff King @ 2022-08-19 23:05 ` Elijah Newren 2022-08-20 7:04 ` Jeff King 0 siblings, 1 reply; 21+ messages in thread From: Elijah Newren @ 2022-08-19 23:05 UTC (permalink / raw) To: Jeff King; +Cc: Git Mailing List On Fri, Aug 19, 2022 at 1:50 AM Jeff King <peff@peff.net> wrote: > > This function has never used its "commit" parameter since it was added > in db757e8b8d (show, log: provide a --remerge-diff capability, > 2022-02-02). > > This makes sense; we already have separate parameters for the parents > (which lets us redo the merge) and the oid of the result tree (which we > can then diff against the remerge result). > > Let's drop the unused parameter in the name of clarity. > > Signed-off-by: Jeff King <peff@peff.net> > --- > log-tree.c | 5 ++--- > 1 file changed, 2 insertions(+), 3 deletions(-) > > diff --git a/log-tree.c b/log-tree.c > index d0ac0a6327..82d9b5f650 100644 > --- a/log-tree.c > +++ b/log-tree.c > @@ -956,8 +956,7 @@ static void cleanup_additional_headers(struct diff_options *o) > > static int do_remerge_diff(struct rev_info *opt, > struct commit_list *parents, > - struct object_id *oid, > - struct commit *commit) > + struct object_id *oid) > { > struct merge_options o; > struct commit_list *bases; > @@ -1052,7 +1051,7 @@ static int log_tree_diff(struct rev_info *opt, struct commit *commit, struct log > "for octopus merges.\n"); > return 1; > } > - return do_remerge_diff(opt, parents, oid, commit); > + return do_remerge_diff(opt, parents, oid); > } > if (opt->combine_merges) > return do_diff_combined(opt, commit); > -- > 2.37.2.928.g0821088f4a Yeah, looks like I could have just used commit instead of parents and oid, but since the calling code had those handy, I added them directly and forgot to remove commit. Patch looks good. ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 2/6] log-tree: drop unused commit param in remerge_diff() 2022-08-19 23:05 ` Elijah Newren @ 2022-08-20 7:04 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2022-08-20 7:04 UTC (permalink / raw) To: Elijah Newren; +Cc: Git Mailing List On Fri, Aug 19, 2022 at 04:05:06PM -0700, Elijah Newren wrote: > > This function has never used its "commit" parameter since it was added > > in db757e8b8d (show, log: provide a --remerge-diff capability, > > 2022-02-02). > > > > This makes sense; we already have separate parameters for the parents > > (which lets us redo the merge) and the oid of the result tree (which we > > can then diff against the remerge result). > [...] > > Yeah, looks like I could have just used commit instead of parents and > oid, but since the calling code had those handy, I added them directly > and forgot to remove commit. That makes sense (the origin of these unused parameters are all mini-mysteries, so it's very satisfying to me to hear a plausible explanation). I like using the broken-down parts as you ended up with. The sole caller does have a commit, but one doesn't _need_ to have a commit to do a remerge diff. You just need two tips and a proposed tree result. In theory we could have a plumbing command which takes those individually. -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 3/6] match_pathname(): drop unused "flags" parameter 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King 2022-08-19 8:49 ` [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() Jeff King 2022-08-19 8:50 ` [PATCH 2/6] log-tree: drop unused commit param in remerge_diff() Jeff King @ 2022-08-19 8:50 ` Jeff King 2022-08-19 8:51 ` [PATCH 4/6] verify_one_sparse(): drop unused repository parameter Jeff King ` (3 subsequent siblings) 6 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2022-08-19 8:50 UTC (permalink / raw) To: git; +Cc: Derrick Stolee This field has not been used since the function was introduced in b559263216 (exclude: split pathname matching code into a separate function, 2012-10-15), though there was a brief period where it was erroneously used and then reverted in ed4958477b (dir: fix pattern matching on dirs, 2021-09-24) and 5ceb663e92 (dir: fix directory-matching bug, 2021-11-02). It's possible we'd eventually add a flag that makes it useful here, but there are only a handful of callers. It would be easy to add back if necessary, and in the meantime this makes the function interface less misleading. Signed-off-by: Jeff King <peff@peff.net> --- attr.c | 2 +- dir.c | 6 ++---- dir.h | 2 +- 3 files changed, 4 insertions(+), 6 deletions(-) diff --git a/attr.c b/attr.c index 21e4ad25ad..8a78dde69e 100644 --- a/attr.c +++ b/attr.c @@ -1023,7 +1023,7 @@ static int path_matches(const char *pathname, int pathlen, } return match_pathname(pathname, pathlen - isdir, base, baselen, - pattern, prefix, pat->patternlen, pat->flags); + pattern, prefix, pat->patternlen); } static int macroexpand_one(struct all_attrs_item *all_attrs, int nr, int rem); diff --git a/dir.c b/dir.c index d7cfb08e44..50eeb8b11e 100644 --- a/dir.c +++ b/dir.c @@ -1244,8 +1244,7 @@ int match_basename(const char *basename, int basenamelen, int match_pathname(const char *pathname, int pathlen, const char *base, int baselen, - const char *pattern, int prefix, int patternlen, - unsigned flags) + const char *pattern, int prefix, int patternlen) { const char *name; int namelen; @@ -1347,8 +1346,7 @@ static struct path_pattern *last_matching_pattern_from_list(const char *pathname if (match_pathname(pathname, pathlen, pattern->base, pattern->baselen ? pattern->baselen - 1 : 0, - exclude, prefix, pattern->patternlen, - pattern->flags)) { + exclude, prefix, pattern->patternlen)) { res = pattern; break; } diff --git a/dir.h b/dir.h index 7bc862030c..674747d93a 100644 --- a/dir.h +++ b/dir.h @@ -414,7 +414,7 @@ int match_basename(const char *, int, const char *, int, int, unsigned); int match_pathname(const char *, int, const char *, int, - const char *, int, int, unsigned); + const char *, int, int); struct path_pattern *last_matching_pattern(struct dir_struct *dir, struct index_state *istate, -- 2.37.2.928.g0821088f4a ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 4/6] verify_one_sparse(): drop unused repository parameter 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King ` (2 preceding siblings ...) 2022-08-19 8:50 ` [PATCH 3/6] match_pathname(): drop unused "flags" parameter Jeff King @ 2022-08-19 8:51 ` Jeff King 2022-08-19 19:04 ` Derrick Stolee 2022-08-20 8:48 ` René Scharfe 2022-08-19 8:54 ` [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear() Jeff King ` (2 subsequent siblings) 6 siblings, 2 replies; 21+ messages in thread From: Jeff King @ 2022-08-19 8:51 UTC (permalink / raw) To: git; +Cc: Derrick Stolee This function has never used its repository parameter since it was introduced in 9ad2d5ea71 (sparse-index: loose integration with cache_tree_verify(), 2021-03-30). As that commit notes, it may eventually be extended further, and that might require looking at a repository struct. But it would be easy to add it back later if necessary. In the mean time, dropping it makes the code shorter and appease -Wunused-parameter. Signed-off-by: Jeff King <peff@peff.net> --- cache-tree.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 56db0b5026..c97111cccf 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root, return 0; } -static void verify_one_sparse(struct repository *r, - struct index_state *istate, - struct cache_tree *it, +static void verify_one_sparse(struct index_state *istate, struct strbuf *path, int pos) { @@ -910,7 +908,7 @@ static int verify_one(struct repository *r, return 1; if (pos >= 0) { - verify_one_sparse(r, istate, it, path, pos); + verify_one_sparse(istate, path, pos); return 0; } -- 2.37.2.928.g0821088f4a ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter 2022-08-19 8:51 ` [PATCH 4/6] verify_one_sparse(): drop unused repository parameter Jeff King @ 2022-08-19 19:04 ` Derrick Stolee 2022-08-20 7:01 ` Jeff King 2022-08-20 8:48 ` René Scharfe 1 sibling, 1 reply; 21+ messages in thread From: Derrick Stolee @ 2022-08-19 19:04 UTC (permalink / raw) To: Jeff King, git On 8/19/2022 4:51 AM, Jeff King wrote: > This function has never used its repository parameter since it was > introduced in 9ad2d5ea71 (sparse-index: loose integration with > cache_tree_verify(), 2021-03-30). As that commit notes, it may > eventually be extended further, and that might require looking at a > repository struct. But it would be easy to add it back later if > necessary. The good news is that 'struct index_state' now has a pointer to its repository, so we wouldn't need to add it back in. Thanks, -Stolee ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter 2022-08-19 19:04 ` Derrick Stolee @ 2022-08-20 7:01 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2022-08-20 7:01 UTC (permalink / raw) To: Derrick Stolee; +Cc: git On Fri, Aug 19, 2022 at 03:04:19PM -0400, Derrick Stolee wrote: > On 8/19/2022 4:51 AM, Jeff King wrote: > > This function has never used its repository parameter since it was > > introduced in 9ad2d5ea71 (sparse-index: loose integration with > > cache_tree_verify(), 2021-03-30). As that commit notes, it may > > eventually be extended further, and that might require looking at a > > repository struct. But it would be easy to add it back later if > > necessary. > > The good news is that 'struct index_state' now has a pointer to > its repository, so we wouldn't need to add it back in. Oh, right, that is even better. I think I have a few other patches in the pile that similarly remove repository pointers. When I'm writing them up, I'll check to see if they can use the same (much better) reasoning. -Peff ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 4/6] verify_one_sparse(): drop unused repository parameter 2022-08-19 8:51 ` [PATCH 4/6] verify_one_sparse(): drop unused repository parameter Jeff King 2022-08-19 19:04 ` Derrick Stolee @ 2022-08-20 8:48 ` René Scharfe 2022-08-20 9:02 ` [PATCH v2 " Jeff King 1 sibling, 1 reply; 21+ messages in thread From: René Scharfe @ 2022-08-20 8:48 UTC (permalink / raw) To: Jeff King, git; +Cc: Derrick Stolee Am 19.08.22 um 10:51 schrieb Jeff King: > This function has never used its repository parameter since it was > introduced in 9ad2d5ea71 (sparse-index: loose integration with > cache_tree_verify(), 2021-03-30). As that commit notes, it may > eventually be extended further, and that might require looking at a > repository struct. But it would be easy to add it back later if > necessary. In the mean time, dropping it makes the code shorter and > appease -Wunused-parameter. > > Signed-off-by: Jeff King <peff@peff.net> > --- > cache-tree.c | 6 ++---- > 1 file changed, 2 insertions(+), 4 deletions(-) > > diff --git a/cache-tree.c b/cache-tree.c > index 56db0b5026..c97111cccf 100644 > --- a/cache-tree.c > +++ b/cache-tree.c > @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root, > return 0; > } > > -static void verify_one_sparse(struct repository *r, > - struct index_state *istate, > - struct cache_tree *it, > +static void verify_one_sparse(struct index_state *istate, This also removes the cache_tree parameter, which has never been used as well, but is not mentioned in the commit message. A good change, to be sure. > struct strbuf *path, > int pos) > { > @@ -910,7 +908,7 @@ static int verify_one(struct repository *r, > return 1; > > if (pos >= 0) { > - verify_one_sparse(r, istate, it, path, pos); > + verify_one_sparse(istate, path, pos); > return 0; > } > ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 4/6] verify_one_sparse(): drop unused repository parameter 2022-08-20 8:48 ` René Scharfe @ 2022-08-20 9:02 ` Jeff King 0 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2022-08-20 9:02 UTC (permalink / raw) To: René Scharfe; +Cc: git, Derrick Stolee On Sat, Aug 20, 2022 at 10:48:47AM +0200, René Scharfe wrote: > > diff --git a/cache-tree.c b/cache-tree.c > > index 56db0b5026..c97111cccf 100644 > > --- a/cache-tree.c > > +++ b/cache-tree.c > > @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root, > > return 0; > > } > > > > -static void verify_one_sparse(struct repository *r, > > - struct index_state *istate, > > - struct cache_tree *it, > > +static void verify_one_sparse(struct index_state *istate, > > This also removes the cache_tree parameter, which has never been used as > well, but is not mentioned in the commit message. A good change, to be > sure. Good catch. I wrote the patch itself over a year ago (whenever the commit made it into 'next' and failed my compilation), but I didn't write the commit message until recently. And when re-reading the patch I totally missed that _two_ parameters went away. Given that and Stolee's earlier comment, here's a proposed commit message that explains it better: -- >8 -- Subject: verify_one_sparse(): drop unused parameters This function has never used its repository or cache_tree parameters since it was introduced in 9ad2d5ea71 (sparse-index: loose integration with cache_tree_verify(), 2021-03-30). As that commit notes, it may eventually be extended further, and that might require looking at more data. But we can easily add them back if necessary (and the repository is even included in the index_state these days already). In the mean time, dropping them makes the code shorter and appeases -Wunused-parameter. Signed-off-by: Jeff King <peff@peff.net> --- cache-tree.c | 6 ++---- 1 file changed, 2 insertions(+), 4 deletions(-) diff --git a/cache-tree.c b/cache-tree.c index 56db0b5026..c97111cccf 100644 --- a/cache-tree.c +++ b/cache-tree.c @@ -857,9 +857,7 @@ int cache_tree_matches_traversal(struct cache_tree *root, return 0; } -static void verify_one_sparse(struct repository *r, - struct index_state *istate, - struct cache_tree *it, +static void verify_one_sparse(struct index_state *istate, struct strbuf *path, int pos) { @@ -910,7 +908,7 @@ static int verify_one(struct repository *r, return 1; if (pos >= 0) { - verify_one_sparse(r, istate, it, path, pos); + verify_one_sparse(istate, path, pos); return 0; } -- 2.37.2.964.g6266ca593d ^ permalink raw reply related [flat|nested] 21+ messages in thread
* [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear() 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King ` (3 preceding siblings ...) 2022-08-19 8:51 ` [PATCH 4/6] verify_one_sparse(): drop unused repository parameter Jeff King @ 2022-08-19 8:54 ` Jeff King 2022-08-19 19:06 ` Derrick Stolee 2022-08-19 8:55 ` [PATCH 6/6] reflog: assert PARSE_OPT_NONEG in parse-options callbacks Jeff King 2022-08-19 19:08 ` [PATCH 0/6] unused function parameter potpourri Derrick Stolee 6 siblings, 1 reply; 21+ messages in thread From: Jeff King @ 2022-08-19 8:54 UTC (permalink / raw) To: git; +Cc: Han-Wen Nienhuys The reader code passes around a "struct reftable_reader" context variable. But the seek function doesn't need it; the table iterator we already get is sufficient. Signed-off-by: Jeff King <peff@peff.net> --- One could argue that this is a method of a reftable_reader following the usual C object-oriented naming conventions, and thus should retain its first parameter, even if it isn't directly used. In that case, we can annotate this as unused (once we have the ability to do so, which will be a separate series). I have a mild preference for removing it (hence this patch), since I think that makes the code more clear. I suppose one could also argue that it should be a method of the table_iter: table_iter_seek_linear() or something. I don't think it matters much, and my ulterior motive is appeasing -Wunused-parameters. reftable/reader.c | 6 +++--- 1 file changed, 3 insertions(+), 3 deletions(-) diff --git a/reftable/reader.c b/reftable/reader.c index 54b4025105..b4db23ce18 100644 --- a/reftable/reader.c +++ b/reftable/reader.c @@ -443,7 +443,7 @@ static int reader_start(struct reftable_reader *r, struct table_iter *ti, return reader_table_iter_at(r, ti, off, typ); } -static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti, +static int reader_seek_linear(struct table_iter *ti, struct reftable_record *want) { struct reftable_record rec = @@ -510,7 +510,7 @@ static int reader_seek_indexed(struct reftable_reader *r, if (err < 0) goto done; - err = reader_seek_linear(r, &index_iter, &want_index); + err = reader_seek_linear(&index_iter, &want_index); while (1) { err = table_iter_next(&index_iter, &index_result); table_iter_block_done(&index_iter); @@ -570,7 +570,7 @@ static int reader_seek_internal(struct reftable_reader *r, err = reader_start(r, &ti, reftable_record_type(rec), 0); if (err < 0) return err; - err = reader_seek_linear(r, &ti, rec); + err = reader_seek_linear(&ti, rec); if (err < 0) return err; else { -- 2.37.2.928.g0821088f4a ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear() 2022-08-19 8:54 ` [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear() Jeff King @ 2022-08-19 19:06 ` Derrick Stolee 2022-08-22 7:46 ` Han-Wen Nienhuys 0 siblings, 1 reply; 21+ messages in thread From: Derrick Stolee @ 2022-08-19 19:06 UTC (permalink / raw) To: Jeff King, git; +Cc: Han-Wen Nienhuys On 8/19/2022 4:54 AM, Jeff King wrote: > The reader code passes around a "struct reftable_reader" context > variable. But the seek function doesn't need it; the table iterator we > already get is sufficient. > > Signed-off-by: Jeff King <peff@peff.net> > --- > One could argue that this is a method of a reftable_reader following the > usual C object-oriented naming conventions, and thus should retain its > first parameter, even if it isn't directly used. > -static int reader_seek_linear(struct reftable_reader *r, struct table_iter *ti, > +static int reader_seek_linear(struct table_iter *ti, > struct reftable_record *want) If we wanted to make it seem more like it was a method of something, it now looks to operate on the table_iter, so the method name could change to something like "table_seek_linear()" which might satisfy both goals. Thanks, -Stolee ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear() 2022-08-19 19:06 ` Derrick Stolee @ 2022-08-22 7:46 ` Han-Wen Nienhuys 0 siblings, 0 replies; 21+ messages in thread From: Han-Wen Nienhuys @ 2022-08-22 7:46 UTC (permalink / raw) To: Derrick Stolee; +Cc: Jeff King, git On Fri, Aug 19, 2022 at 9:06 PM Derrick Stolee <derrickstolee@github.com> wrote: > > On 8/19/2022 4:54 AM, Jeff King wrote: > > The reader code passes around a "struct reftable_reader" context > > variable. But the seek function doesn't need it; the table iterator we > > already get is sufficient. > > If we wanted to make it seem more like it was a method of something, > it now looks to operate on the table_iter, so the method name could > change to something like "table_seek_linear()" which might satisfy > both goals. +1. Name should be table_iter_seek_linear to stick with naming conventions, and should move to the top of the file next to other table_iter methods. -- Han-Wen Nienhuys - Google Munich I work 80%. Don't expect answers from me on Fridays. -- Google Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich Registergericht und -nummer: Hamburg, HRB 86891 Sitz der Gesellschaft: Hamburg Geschäftsführer: Paul Manicle, Liana Sebastian ^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH 6/6] reflog: assert PARSE_OPT_NONEG in parse-options callbacks 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King ` (4 preceding siblings ...) 2022-08-19 8:54 ` [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear() Jeff King @ 2022-08-19 8:55 ` Jeff King 2022-08-19 19:08 ` [PATCH 0/6] unused function parameter potpourri Derrick Stolee 6 siblings, 0 replies; 21+ messages in thread From: Jeff King @ 2022-08-19 8:55 UTC (permalink / raw) To: git In the spirit of 517fe807d6 (assert NOARG/NONEG behavior of parse-options callbacks, 2018-11-05), this asserts that our callbacks were invoked using the right flags (since otherwise they'd segfault on the NULL arg). Both cases are already correct here, so this is mostly about annotating the functions, and appeasing -Wunused-parameters. Signed-off-by: Jeff King <peff@peff.net> --- builtin/reflog.c | 4 ++++ 1 file changed, 4 insertions(+) diff --git a/builtin/reflog.c b/builtin/reflog.c index 4dd297dce8..8123956847 100644 --- a/builtin/reflog.c +++ b/builtin/reflog.c @@ -193,6 +193,8 @@ static int expire_unreachable_callback(const struct option *opt, { struct cmd_reflog_expire_cb *cmd = opt->value; + BUG_ON_OPT_NEG(unset); + if (parse_expiry_date(arg, &cmd->expire_unreachable)) die(_("invalid timestamp '%s' given to '--%s'"), arg, opt->long_name); @@ -207,6 +209,8 @@ static int expire_total_callback(const struct option *opt, { struct cmd_reflog_expire_cb *cmd = opt->value; + BUG_ON_OPT_NEG(unset); + if (parse_expiry_date(arg, &cmd->expire_total)) die(_("invalid timestamp '%s' given to '--%s'"), arg, opt->long_name); -- 2.37.2.928.g0821088f4a ^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] unused function parameter potpourri 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King ` (5 preceding siblings ...) 2022-08-19 8:55 ` [PATCH 6/6] reflog: assert PARSE_OPT_NONEG in parse-options callbacks Jeff King @ 2022-08-19 19:08 ` Derrick Stolee 2022-08-19 23:07 ` Elijah Newren 6 siblings, 1 reply; 21+ messages in thread From: Derrick Stolee @ 2022-08-19 19:08 UTC (permalink / raw) To: Jeff King, git On 8/19/2022 4:48 AM, Jeff King wrote: > Here are a few small cleanups of unused function parameters. The first > five just drop the unused parameters. These are all trivially correct, > since otherwise the compiler would complain. But I tried to make sure > that dropping was the right thing in each (rather than it being a bug > where the parameter should have been used). > > The final one just uses the parameters for an assertion, following a > pattern we've used before. > > I'll try to cc the individual authors for each patch. > > [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() > [2/6]: log-tree: drop unused commit param in remerge_diff() > [3/6]: match_pathname(): drop unused "flags" parameter > [4/6]: verify_one_sparse(): drop unused repository parameter > [5/6]: reftable: drop unused parameter from reader_seek_linear() > [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks Thanks for doing this cleanup. It all looks correct to me. Patch 5 mentioned some choice as to modifying the parameter list versus using the UNUSED() macro. I think renaming the method can solve the uncertainty there, but it's also not necessary for the change to be correct. Thanks, -Stolee ^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH 0/6] unused function parameter potpourri 2022-08-19 19:08 ` [PATCH 0/6] unused function parameter potpourri Derrick Stolee @ 2022-08-19 23:07 ` Elijah Newren 0 siblings, 0 replies; 21+ messages in thread From: Elijah Newren @ 2022-08-19 23:07 UTC (permalink / raw) To: Derrick Stolee; +Cc: Jeff King, Git Mailing List On Fri, Aug 19, 2022 at 12:35 PM Derrick Stolee <derrickstolee@github.com> wrote: > > On 8/19/2022 4:48 AM, Jeff King wrote: > > Here are a few small cleanups of unused function parameters. The first > > five just drop the unused parameters. These are all trivially correct, > > since otherwise the compiler would complain. But I tried to make sure > > that dropping was the right thing in each (rather than it being a bug > > where the parameter should have been used). > > > > The final one just uses the parameters for an assertion, following a > > pattern we've used before. > > > > I'll try to cc the individual authors for each patch. > > > > [1/6]: xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() > > [2/6]: log-tree: drop unused commit param in remerge_diff() > > [3/6]: match_pathname(): drop unused "flags" parameter > > [4/6]: verify_one_sparse(): drop unused repository parameter > > [5/6]: reftable: drop unused parameter from reader_seek_linear() > > [6/6]: reflog: assert PARSE_OPT_NONEG in parse-options callbacks > > Thanks for doing this cleanup. It all looks correct to me. Like Phillip, I seem to also have a blind spot for unused parameters -- this isn't the first one I created either. Anyway, this series looks good to me as well. > Patch 5 mentioned some choice as to modifying the parameter list > versus using the UNUSED() macro. I think renaming the method can > solve the uncertainty there, but it's also not necessary for the > change to be correct. ^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2022-08-26 13:15 UTC | newest] Thread overview: 21+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-08-19 8:48 [PATCH 0/6] unused function parameter potpourri Jeff King 2022-08-19 8:49 ` [PATCH 1/6] xdiff: drop unused mmfile parameters from xdl_do_histogram_diff() Jeff King 2022-08-19 13:32 ` Phillip Wood 2022-08-20 6:58 ` Jeff King 2022-08-20 7:36 ` [PATCH 7/6] xdiff: drop unused mmfile parameters from xdl_do_patience_diff() Jeff King 2022-08-26 13:15 ` Phillip Wood 2022-08-19 8:50 ` [PATCH 2/6] log-tree: drop unused commit param in remerge_diff() Jeff King 2022-08-19 23:05 ` Elijah Newren 2022-08-20 7:04 ` Jeff King 2022-08-19 8:50 ` [PATCH 3/6] match_pathname(): drop unused "flags" parameter Jeff King 2022-08-19 8:51 ` [PATCH 4/6] verify_one_sparse(): drop unused repository parameter Jeff King 2022-08-19 19:04 ` Derrick Stolee 2022-08-20 7:01 ` Jeff King 2022-08-20 8:48 ` René Scharfe 2022-08-20 9:02 ` [PATCH v2 " Jeff King 2022-08-19 8:54 ` [PATCH 5/6] reftable: drop unused parameter from reader_seek_linear() Jeff King 2022-08-19 19:06 ` Derrick Stolee 2022-08-22 7:46 ` Han-Wen Nienhuys 2022-08-19 8:55 ` [PATCH 6/6] reflog: assert PARSE_OPT_NONEG in parse-options callbacks Jeff King 2022-08-19 19:08 ` [PATCH 0/6] unused function parameter potpourri Derrick Stolee 2022-08-19 23:07 ` Elijah Newren
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).