Re: [PATCH v1] merge - rename a shadowed variable in cmd_merge

[Junio C Hamano <gitster@pobox.com>]

Edmundo Carmona Antoranz <eantoranz@gmail.com> writes:

> variable ret used in cmd_merge introduced in d5a35c114ab was already
> a local variable used inside a for loop inside the function.

Strictly speaking, there was a local variable 'ret' inside for loop,
which is unrelated to the variable introduced by the said commit.
The only resemblance was that they happen to share the same name.
So "was already a local variable" is not quite right, and made my
reading hiccup.

> for-local variable is being renamed to ret_try_merge to avoid shadow.

Is this really a problem that needs to be changed?  What compiler
is having trouble with the code?

I am reasonably negative on this change.  But as you seem to be a
new contributor, let me grab this opportunity to comment on other
aspects of the patch.

> ---

Missing sign-off.

In the proposed commit log message body, write full sentences just
like normal English, e.g. a sentence begins with a capital letter,
etc.

The usual pattern used in our log messages is first to give an
observation of the current state and state what the problem is,
and then write orders you give to the codebase to be like so to fix
the problem, e.g.

	The commit d5a35c11 ("Copy resolve_ref() return value for
	longer use", 2011-11-13) introduced a variable 'ret' to
	cmd_merge() to keep the final return value from the
	function.  There however was an unrelated variable that is
	local to a for loop that shared the same name.  Because the
	statements inside of the loop do not have enough information
	to decide the final outcome of the function, there is no
	need for the outer 'ret' to be visible to them, which is
	a perfectly good reason to use the "shadowing" technique.

	Rename the local variable used inside the for loop to avoid
	warnings when compiled with -Wshadow; this will expose the
	outer 'ret' to the statements in the loop, allowing them to
	mistakenly making an assignment to it, though.

is how I would describe this change.  As you can see, this trades
"make -Wshadow less noisy" with "make it easier to make mistakes"
and I am not sure if it is a good trade-off.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 6e99aead46..972b6c376a 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -1587,7 +1587,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  		oidclr(&stash);

Interesting.

All assignments to ret up to this point are all followed by "goto
done" to jump over the "for" loop we are looking at this patch.
So we know that when the control reaches at this point, ret has its
initial value 0.

So an alternative approach would be to just ...

>  	for (i = 0; i < use_strategies_nr; i++) {
> -		int ret;
> +		int ret_try_merge;

... drop this local variable declaration and let it contaminate the
outer 'ret', and then after the loop is done, assign 0 to ret.  That
would squelch "-Wshallow" and at the same time makes sure that the
loop won't corrupt the "proposed final outcome" stored in 'ret'.

Quite honestly, I think the easiest "solution" would be not to use
"-Wshadow" in your compilation.  Thsi file has a handful other
instances of variable shadowing, and most of them do not look
confusing or problematic.