Re: [PATCH 15/30] merge-recursive: Move the get_renames() function

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 14, 2017, 04:46 UTC
Message-ID: <xmqqh8tx5ux8.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20171110190550.27059-16-newren@gmail.com>

Elijah Newren <newren@gmail.com> writes:

Show 6 quoted lines

> I want to re-use some other functions in the file without moving those other
> functions or dealing with a handful of annoying split function declarations
> and definitions.
>
> Signed-off-by: Elijah Newren <newren@gmail.com>
> ---

It took me a while to figure out that you are basing this on top of a slightly older tip of 'master'. When rebasing on, or merging to, a newer codebase, these two lines

Show 9 quoted lines

> -	diff_setup(&opts);
> -	DIFF_OPT_SET(&opts, RECURSIVE);
> -	DIFF_OPT_CLR(&opts, RENAME_EMPTY);
> -	opts.detect_rename = DIFF_DETECT_RENAME;
> ...
> +	diff_setup(&opts);
> +	DIFF_OPT_SET(&opts, RECURSIVE);
> +	DIFF_OPT_CLR(&opts, RENAME_EMPTY);
> +	opts.detect_rename = DIFF_DETECT_RENAME;

would need a bit of adjustment.

By the way, checkpatch.pl complains about // C99 comments and binary operators missing SP on both ends, etc., on the entire series [*1*]. These look like small issues, but they are distracting enough to break concentration while reading the changes to spot places with real issues and places that can be improved, so cleaning them up early would help the final result to get better reviews.

I won't reproduce all of them here, but here are a representable few.

ERROR: spaces required around that '=' (ctx:VxV)
#30: FILE: merge-recursive.c:1491:
+       for (i=0; i<slist->nr; i++) {
              ^

ERROR: spaces required around that '<' (ctx:VxV)
#30: FILE: merge-recursive.c:1491:
+       for (i=0; i<slist->nr; i++) {
                   ^

ERROR: "foo* bar" should be "foo *bar"
#42: FILE: merge-recursive.c:1503:
+static char* handle_path_level_conflicts(struct merge_options *o,

WARNING: suspect code indent for conditional statements (8, 10)
#80: FILE: merge-recursive.c:791:
+       if (o->call_depth || !was_tracked(path))
+         return !dirty;

Thanks.

[Footnote]

Just FYI, checkpatch.pl also notices these but it seems that our existing codebase already violates them in a major way, so I usually do not pay attention to these classes of complaints:

ERROR: spaces required around that ':' (ctx:VxV)
#30: FILE: merge-recursive.c:603:
+       unsigned add_turned_into_rename:1;
                                       ^

WARNING: quoted string split across lines
#74: FILE: merge-recursive.c:1433:
+                       output(o, 1, _("Refusing to lose untracked file at "
+                                      "%s, even though it's in the way."),