git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / Atom feed
From: Elijah Newren <newren@gmail.com>
To: Jeff King <peff@peff.net>
Cc: Git Mailing List <git@vger.kernel.org>
Subject: Re: unused parameters in merge-recursive.c
Date: Fri, 19 Oct 2018 10:58:19 -0700
Message-ID: <CABPp-BHobf8wbBsXF97scNQCzkxQukziODRXq6JOOWq61cAd9g@mail.gmail.com> (raw)
In-Reply-To: <20181019171827.GA21091@sigill.intra.peff.net>

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.

  reply index

Thread overview: 3+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2018-10-19 17:18 Jeff King
2018-10-19 17:58 ` Elijah Newren [this message]
2018-10-19 19:04   ` Jeff King

Reply instructions:

You may reply publically to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=CABPp-BHobf8wbBsXF97scNQCzkxQukziODRXq6JOOWq61cAd9g@mail.gmail.com \
    --to=newren@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=peff@peff.net \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

git@vger.kernel.org list mirror (unofficial, one of many)

Archives are clonable:
	git clone --mirror https://public-inbox.org/git
	git clone --mirror http://ou63pmih66umazou.onion/git
	git clone --mirror http://czquwvybam4bgbro.onion/git
	git clone --mirror http://hjrcffqmbrq6wope.onion/git

Newsgroups are available over NNTP:
	nntp://news.public-inbox.org/inbox.comp.version-control.git
	nntp://ou63pmih66umazou.onion/inbox.comp.version-control.git
	nntp://czquwvybam4bgbro.onion/inbox.comp.version-control.git
	nntp://hjrcffqmbrq6wope.onion/inbox.comp.version-control.git
	nntp://news.gmane.org/gmane.comp.version-control.git

 note: .onion URLs require Tor: https://www.torproject.org/

AGPL code for this site: git clone https://public-inbox.org/ public-inbox