{"thread":{"id":"49618","subject":"unused parameters in merge-recursive.c","startedAt":"2018-10-19T17:18:31Z","lastAt":"2018-10-19T19:04:19Z","messageCount":3,"participants":["Jeff King","Elijah Newren"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"360958","messageId":"20181019171827.GA21091@sigill.intra.peff.net","threadId":"49618","inReplyTo":null,"subject":"unused parameters in merge-recursive.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-19T17:18:28Z","receivedAt":"2018-10-19T17:18:31Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Hi Elijah,\n\nPlaying with -Wunused-parameters, I notice there are several functions\nwith unused parameters in merge-recursive.c. The patch below drops them\nall. We know they're not being used, so it can't make anything _worse_,\nbut this is a good opportunity to confirm that they don't represent some\nlatent bug.\n\nIn most cases I've been trying to determine the \"bug versus cruft\" thing\nmyself, but I fear that merge-recursive exceeds my abilities here. ;)\n\n---\n merge-recursive.c | 19 +++++++------------\n 1 file changed, 7 insertions(+), 12 deletions(-)\n\ndiff --git a/merge-recursive.c b/merge-recursive.c\nindex c0fb83d285..f6d634c3a2 100644\n--- a/merge-recursive.c\n+++ b/merge-recursive.c\n@@ -1369,8 +1369,7 @@ static int merge_mode_and_contents(struct merge_options *o,\n \n static int handle_rename_via_dir(struct merge_options *o,\n \t\t\t\t struct diff_filepair *pair,\n-\t\t\t\t const char *rename_branch,\n-\t\t\t\t const char *other_branch)\n+\t\t\t\t const char *rename_branch)\n {\n \t/*\n \t * Handle file adds that need to be renamed due to directory rename\n@@ -2071,8 +2070,7 @@ static void handle_directory_level_conflicts(struct merge_options *o,\n \tremove_hashmap_entries(dir_re_merge, &remove_from_merge);\n }\n \n-static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,\n-\t\t\t\t\t     struct tree *tree)\n+static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)\n {\n \tstruct hashmap *dir_renames;\n \tstruct hashmap_iter iter;\n@@ -2318,8 +2316,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,\n \t\t\t\t\t\t struct tree *o_tree,\n \t\t\t\t\t\t struct tree *a_tree,\n \t\t\t\t\t\t struct tree *b_tree,\n-\t\t\t\t\t\t struct string_list *entries,\n-\t\t\t\t\t\t int *clean)\n+\t\t\t\t\t\t struct string_list *entries)\n {\n \tstruct string_list_item *item;\n \tint stage = (tree == a_tree ? 2 : 3);\n@@ -2490,8 +2487,7 @@ static struct string_list *get_renames(struct merge_options *o,\n \t\t\tapply_directory_rename_modifications(o, pair, new_path,\n \t\t\t\t\t\t\t     re, tree, o_tree,\n \t\t\t\t\t\t\t     a_tree, b_tree,\n-\t\t\t\t\t\t\t     entries,\n-\t\t\t\t\t\t\t     clean_merge);\n+\t\t\t\t\t\t\t     entries);\n \t}\n \n \thashmap_iter_init(&collisions, &iter);\n@@ -2826,8 +2822,8 @@ static int detect_and_process_renames(struct merge_options *o,\n \tmerge_pairs = get_diffpairs(o, common, merge);\n \n \tif (o->detect_directory_renames) {\n-\t\tdir_re_head = get_directory_renames(head_pairs, head);\n-\t\tdir_re_merge = get_directory_renames(merge_pairs, merge);\n+\t\tdir_re_head = get_directory_renames(head_pairs);\n+\t\tdir_re_merge = get_directory_renames(merge_pairs);\n \n \t\thandle_directory_level_conflicts(o,\n \t\t\t\t\t\t dir_re_head, head,\n@@ -3149,8 +3145,7 @@ static int process_entry(struct merge_options *o,\n \t\t\tclean_merge = 1;\n \t\t\tif (handle_rename_via_dir(o,\n \t\t\t\t\t\t  conflict_info->pair1,\n-\t\t\t\t\t\t  conflict_info->branch1,\n-\t\t\t\t\t\t  conflict_info->branch2))\n+\t\t\t\t\t\t  conflict_info->branch1))\n \t\t\t\tclean_merge = -1;\n \t\t\tbreak;\n \t\tcase RENAME_DELETE:\n-- \n2.19.1.1089.g69f65ee796\n\n"},{"id":"360964","messageId":"CABPp-BHobf8wbBsXF97scNQCzkxQukziODRXq6JOOWq61cAd9g@mail.gmail.com","threadId":"49618","inReplyTo":"20181019171827.GA21091@sigill.intra.peff.net","subject":"Re: unused parameters in merge-recursive.c","fromName":"Elijah Newren","fromEmail":"newren@gmail.com","sentAt":"2018-10-19T17:58:19Z","receivedAt":"2018-10-19T17:58:35Z","isPatch":false,"sender":{"key":"newren@gmail.com","avatar":"https://avatars.githubusercontent.com/u/5455730?v=4"},"body":"Hi Peff,\n\nOn Fri, Oct 19, 2018 at 10:18 AM Jeff King <peff@peff.net> wrote:\n>\n> Hi Elijah,\n>\n> Playing with -Wunused-parameters, I notice there are several functions\n> with unused parameters in merge-recursive.c. The patch below drops them\n> all. We know they're not being used, so it can't make anything _worse_,\n> but this is a good opportunity to confirm that they don't represent some\n> latent bug.\n>\n> In most cases I've been trying to determine the \"bug versus cruft\" thing\n> myself, but I fear that merge-recursive exceeds my abilities here. ;)\n\nThese ones all look like cruft to me.  I dug through them and tried\nlooking through history and old submissions for my guesses and how\nthey ended up here; details below.\n\n> ---\n>  merge-recursive.c | 19 +++++++------------\n>  1 file changed, 7 insertions(+), 12 deletions(-)\n>\n> diff --git a/merge-recursive.c b/merge-recursive.c\n> index c0fb83d285..f6d634c3a2 100644\n> --- a/merge-recursive.c\n> +++ b/merge-recursive.c\n> @@ -1369,8 +1369,7 @@ static int merge_mode_and_contents(struct merge_options *o,\n>\n>  static int handle_rename_via_dir(struct merge_options *o,\n>                                  struct diff_filepair *pair,\n> -                                const char *rename_branch,\n> -                                const char *other_branch)\n> +                                const char *rename_branch)\n\nGiven the similarity in function signature to handle_rename_delete(),\nit's possible I copied the function and then started editing.  Whether\nI was lazily doing that, or if I really added that parameter because I\nthought I was going to add an informational message to the user that\nused it, or something else, I don't know.  But I agree, it's just not\nneeded and could be added back later if someone did find a use for it.\n\n> @@ -2071,8 +2070,7 @@ static void handle_directory_level_conflicts(struct merge_options *o,\n> -static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs,\n> -                                            struct tree *tree)\n> +static struct hashmap *get_directory_renames(struct diff_queue_struct *pairs)\n\nYeah, nuke it.  I don't remember full details here (it's been over a\nyear), but my best guess is that I started with (at least part of the)\ncode for handle_directory_level_conflicts() inside of\nget_directory_renames() and then broke it out into a separate function\nwhen it got bigger, but forgot to remove the extra argument.\n\n>  {\n>         struct hashmap *dir_renames;\n>         struct hashmap_iter iter;\n> @@ -2318,8 +2316,7 @@ static void apply_directory_rename_modifications(struct merge_options *o,\n>                                                  struct tree *o_tree,\n>                                                  struct tree *a_tree,\n>                                                  struct tree *b_tree,\n> -                                                struct string_list *entries,\n> -                                                int *clean)\n> +                                                struct string_list *entries)\n\nYeah, there's several other functions with such a flag; I probably\nadded it thinking this function would need it too and then just forgot\nto remove it when it turned out to not be necessary.\n\n\nThe remaining chunks in the patch you emailed are just modifying\ncallers to not pass the extra non-unnecessary args, so they're all\ngood.\n"},{"id":"360967","messageId":"20181019190416.GB24418@sigill.intra.peff.net","threadId":"49618","inReplyTo":"CABPp-BHobf8wbBsXF97scNQCzkxQukziODRXq6JOOWq61cAd9g@mail.gmail.com","subject":"Re: unused parameters in merge-recursive.c","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-10-19T19:04:16Z","receivedAt":"2018-10-19T19:04:19Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Oct 19, 2018 at 10:58:19AM -0700, Elijah Newren wrote:\n\n> > In most cases I've been trying to determine the \"bug versus cruft\" thing\n> > myself, but I fear that merge-recursive exceeds my abilities here. ;)\n> \n> These ones all look like cruft to me.  I dug through them and tried\n> looking through history and old submissions for my guesses and how\n> they ended up here; details below.\n\nGood, that makes things easier. :)\n\n> >  static int handle_rename_via_dir(struct merge_options *o,\n> >                                  struct diff_filepair *pair,\n> > -                                const char *rename_branch,\n> > -                                const char *other_branch)\n> > +                                const char *rename_branch)\n> \n> Given the similarity in function signature to handle_rename_delete(),\n> it's possible I copied the function and then started editing.  Whether\n> I was lazily doing that, or if I really added that parameter because I\n> thought I was going to add an informational message to the user that\n> used it, or something else, I don't know.  But I agree, it's just not\n> needed and could be added back later if someone did find a use for it.\n\nYeah, this was the one I was most worried about.\n\nThanks for confirming. I'm preparing a bunch of similar cleanups, so\nI'll roll this into that series.\n\n-Peff\n"}]}