unused parameters in merge-recursive.c

unused parameters in merge-recursive.c

[Jeff King]

Hi Elijah,

Playing with -Wunused-parameters, I notice there are several functions
with unused parameters in merge-recursive.c. The patch below drops them
all. We know they're not being used, so it can't make anything _worse_,
but this is a good opportunity to confirm that they don't represent some
latent bug.

In most cases I've been trying to determine the "bug versus cruft" thing
myself, but I fear that merge-recursive exceeds my abilities here. ;)

---
 merge-recursive.c | 19 +++++++------------
 1 file changed, 7 insertions(+), 12 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index c0fb83d285..f6d634c3a2 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1369,8 +1369,7 @@ static int merge_mode_and_contents(struct merge_options *o,
 
 static int handle_rename_via_dir(struct merge_options *o,
 				 struct diff_filepair *pair,
-				 const char *rename_branch,
-				 const char *other_branch)
+				 const char *rename_branch)
 {
 	/*
 	 * Handle file adds that need to be renamed due to directory rename
@@ -2071,8 +2070,7 @@ static void handle_directory_level_conflicts(struct merge_options *o,
 	remove_hashmap_entries(dir_re_merge, &remove_from_merge);
 }
 
-static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
-					     struct tree *tree)
+static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)
 {
 	struct hashmap *dir_renames;
 	struct hashmap_iter iter;
@@ -2318,8 +2316,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,
 						 struct tree *o_tree,
 						 struct tree *a_tree,
 						 struct tree *b_tree,
-						 struct string_list *entries,
-						 int *clean)
+						 struct string_list *entries)
 {
 	struct string_list_item *item;
 	int stage = (tree == a_tree ? 2 : 3);
@@ -2490,8 +2487,7 @@ static struct string_list *get_renames(struct merge_options *o,
 			apply_directory_rename_modifications(o, pair, new_path,
 							     re, tree, o_tree,
 							     a_tree, b_tree,
-							     entries,
-							     clean_merge);
+							     entries);
 	}
 
 	hashmap_iter_init(&collisions, &iter);
@@ -2826,8 +2822,8 @@ static int detect_and_process_renames(struct merge_options *o,
 	merge_pairs = get_diffpairs(o, common, merge);
 
 	if (o->detect_directory_renames) {
-		dir_re_head = get_directory_renames(head_pairs, head);
-		dir_re_merge = get_directory_renames(merge_pairs, merge);
+		dir_re_head = get_directory_renames(head_pairs);
+		dir_re_merge = get_directory_renames(merge_pairs);
 
 		handle_directory_level_conflicts(o,
 						 dir_re_head, head,
@@ -3149,8 +3145,7 @@ static int process_entry(struct merge_options *o,
 			clean_merge = 1;
 			if (handle_rename_via_dir(o,
 						  conflict_info->pair1,
-						  conflict_info->branch1,
-						  conflict_info->branch2))
+						  conflict_info->branch1))
 				clean_merge = -1;
 			break;
 		case RENAME_DELETE:
-- 
2.19.1.1089.g69f65ee796

Re: unused parameters in merge-recursive.c

[Elijah Newren]

Hi Peff,

On Fri, Oct 19, 2018 at 10:18 AM Jeff King <peff@peff.net> wrote:
>
> Hi Elijah,
>
> Playing with -Wunused-parameters, I notice there are several functions
> with unused parameters in merge-recursive.c. The patch below drops them
> all. We know they're not being used, so it can't make anything _worse_,
> but this is a good opportunity to confirm that they don't represent some
> latent bug.
>
> In most cases I've been trying to determine the "bug versus cruft" thing
> myself, but I fear that merge-recursive exceeds my abilities here. ;)

These ones all look like cruft to me.  I dug through them and tried
looking through history and old submissions for my guesses and how
they ended up here; details below.

> ---
>  merge-recursive.c | 19 +++++++------------
>  1 file changed, 7 insertions(+), 12 deletions(-)
>
> diff --git a/merge-recursive.c b/merge-recursive.c
> index c0fb83d285..f6d634c3a2 100644
> --- a/merge-recursive.c
> +++ b/merge-recursive.c
> @@ -1369,8 +1369,7 @@ static int merge_mode_and_contents(struct merge_options *o,
>
>  static int handle_rename_via_dir(struct merge_options *o,
>                                  struct diff_filepair *pair,
> -                                const char *rename_branch,
> -                                const char *other_branch)
> +                                const char *rename_branch)

Given the similarity in function signature to handle_rename_delete(),
it's possible I copied the function and then started editing.  Whether
I was lazily doing that, or if I really added that parameter because I
thought I was going to add an informational message to the user that
used it, or something else, I don't know.  But I agree, it's just not
needed and could be added back later if someone did find a use for it.

> @@ -2071,8 +2070,7 @@ static void handle_directory_level_conflicts(struct merge_options *o,
> -static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,
> -                                            struct tree *tree)
> +static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)

Yeah, nuke it.  I don't remember full details here (it's been over a
year), but my best guess is that I started with (at least part of the)
code for handle_directory_level_conflicts() inside of
get_directory_renames() and then broke it out into a separate function
when it got bigger, but forgot to remove the extra argument.

>  {
>         struct hashmap *dir_renames;
>         struct hashmap_iter iter;
> @@ -2318,8 +2316,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,
>                                                  struct tree *o_tree,
>                                                  struct tree *a_tree,
>                                                  struct tree *b_tree,
> -                                                struct string_list *entries,
> -                                                int *clean)
> +                                                struct string_list *entries)

Yeah, there's several other functions with such a flag; I probably
added it thinking this function would need it too and then just forgot
to remove it when it turned out to not be necessary.


The remaining chunks in the patch you emailed are just modifying
callers to not pass the extra non-unnecessary args, so they're all
good.

Re: unused parameters in merge-recursive.c

[Jeff King]

On Fri, Oct 19, 2018 at 10:58:19AM -0700, Elijah Newren wrote:

> > In most cases I've been trying to determine the "bug versus cruft" thing
> > myself, but I fear that merge-recursive exceeds my abilities here. ;)
> 
> These ones all look like cruft to me.  I dug through them and tried
> looking through history and old submissions for my guesses and how
> they ended up here; details below.

Good, that makes things easier. :)

> >  static int handle_rename_via_dir(struct merge_options *o,
> >                                  struct diff_filepair *pair,
> > -                                const char *rename_branch,
> > -                                const char *other_branch)
> > +                                const char *rename_branch)
> 
> Given the similarity in function signature to handle_rename_delete(),
> it's possible I copied the function and then started editing.  Whether
> I was lazily doing that, or if I really added that parameter because I
> thought I was going to add an informational message to the user that
> used it, or something else, I don't know.  But I agree, it's just not
> needed and could be added back later if someone did find a use for it.

Yeah, this was the one I was most worried about.

Thanks for confirming. I'm preparing a bunch of similar cleanups, so
I'll roll this into that series.

-Peff