[PATCH 0/3] diff-merges: minor cleanups

[PATCH 0/3] diff-merges: minor cleanups

[Sergey Organov]

These are pure cleanups. Expect no changes of behavior.

Signed-off-by: Sergey Organov <sorganov@gmail.com>

Sergey Organov (3):
  diff-merges: cleanup func_by_opt()
  diff-merges: cleanup set_diff_merges()
  diff-merges: clarify log.diffMerges documentation

 Documentation/config/log.txt |  6 +++---
 diff-merges.c                | 40 +++++++++++++++++++++---------------
 2 files changed, 27 insertions(+), 19 deletions(-)

-- 
2.25.1

[PATCH 1/3] diff-merges: cleanup func_by_opt()

[Sergey Organov]

Get rid of unneeded "else" statements in func_by_opt().

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 7f64156b8bfe..780ed08fc87f 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -60,15 +60,15 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
 		return suppress;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
 		return set_first_parent;
-	else if (!strcmp(optarg, "separate"))
+	if (!strcmp(optarg, "separate"))
 		return set_separate;
-	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
+	if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
 		return set_combined;
-	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
+	if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
 		return set_dense_combined;
-	else if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
+	if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
 		return set_remerge_diff;
-	else if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
+	if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
 		return set_to_default;
 	return NULL;
 }
-- 
2.25.1

Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Get rid of unneeded "else" statements in func_by_opt().

While it is true that loss of "else" will not change what the code
means, this change feels subjective and I'd say it falls into "once
committed, it is not worth the patch noise to go in and change"
category, not a "clean-up we should do".

I haven't looked at diff-merges.c for quite a while, but I did.  I
notice that the code is barely commented on what each helper
function is supposed to do, etc., and very hard to follow.  The file
needs cleaning up in that area a lot more, I would say.

Thanks.

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  diff-merges.c | 10 +++++-----
>  1 file changed, 5 insertions(+), 5 deletions(-)
>
> diff --git a/diff-merges.c b/diff-merges.c
> index 7f64156b8bfe..780ed08fc87f 100644
> --- a/diff-merges.c
> +++ b/diff-merges.c
> @@ -60,15 +60,15 @@ static diff_merges_setup_func_t func_by_opt(const char *optarg)
>  		return suppress;
>  	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
>  		return set_first_parent;
> -	else if (!strcmp(optarg, "separate"))
> +	if (!strcmp(optarg, "separate"))
>  		return set_separate;
> -	else if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
> +	if (!strcmp(optarg, "c") || !strcmp(optarg, "combined"))
>  		return set_combined;
> -	else if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
> +	if (!strcmp(optarg, "cc") || !strcmp(optarg, "dense-combined"))
>  		return set_dense_combined;
> -	else if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
> +	if (!strcmp(optarg, "r") || !strcmp(optarg, "remerge"))
>  		return set_remerge_diff;
> -	else if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
> +	if (!strcmp(optarg, "m") || !strcmp(optarg, "on"))
>  		return set_to_default;
>  	return NULL;
>  }

Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Get rid of unneeded "else" statements in func_by_opt().
>
> While it is true that loss of "else" will not change what the code
> means, this change feels subjective and I'd say it falls into "once
> committed, it is not worth the patch noise to go in and change"
> category, not a "clean-up we should do".

I agree the "else" vs "no else" is subjective, but the problem in fact
is that the first "if", unlike the rest of them, already had no "else",
making the code inconsistent. So the fix should either be adding one
"else" to the first "if", or remove all of the "else". I chose the
latter, to end up with less noisy code.

>
> I haven't looked at diff-merges.c for quite a while, but I did.  I
> notice that the code is barely commented on what each helper
> function is supposed to do, etc., and very hard to follow.  The file
> needs cleaning up in that area a lot more, I would say.

I believe each helper function does exactly what its name suggests, so
no comments are needed. I hate to add comments that actually just say
the same as function name, so I'd rathe consider to rename some
functions instead should somebody point out to problematic ones.

That said, it seems Elijah Newren had not much trouble adding his
--remerge-diff capability to the diff-merges code, so the code must be
not that hard to follow even in its current state.

Thanks,
-- Sergey Organov.

Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Sergey Organov <sorganov@gmail.com> writes:
>>
>>> Get rid of unneeded "else" statements in func_by_opt().
>>
>> While it is true that loss of "else" will not change what the code
>> means, this change feels subjective and I'd say it falls into "once
>> committed, it is not worth the patch noise to go in and change"
>> category, not a "clean-up we should do".
>
> I agree the "else" vs "no else" is subjective, but the problem in fact
> is that the first "if", unlike the rest of them, already had no "else",
> making the code inconsistent.

This is a static helper function about a single "optarg" string that
wanted to say "switch(optarg) { case "off": ... }" but couldn't in
C, and I happen to view if strcmp else if strcmp ... sequence on the
same string a poor-man's substitute for such a construct.  So my
take of it is to call the second "if" not being "else if" a problem,
not the rest of it.  If we add a new condition on a different input,
making it "if (x) ...; switch(optarg) { ... }" that talks about
something other than optarg, then writing it all with "if" without
"else if" would make it harder to see the pattern, but I do not care
too deeply either way, because this is unlikely to gain any logic
more involved than "switch(optarg) { ... }".

> So the fix should either be adding one
> "else" to the first "if", or remove all of the "else". I chose the
> latter, to end up with less noisy code.

Yup, see above for the reason why I would choose else-if cascade if
I had to but I do not care too deeply either way in this particular
case ;-)

Re: [PATCH 1/3] diff-merges: cleanup func_by_opt()

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Sergey Organov <sorganov@gmail.com> writes:
>>>
>>>> Get rid of unneeded "else" statements in func_by_opt().
>>>
>>> While it is true that loss of "else" will not change what the code
>>> means, this change feels subjective and I'd say it falls into "once
>>> committed, it is not worth the patch noise to go in and change"
>>> category, not a "clean-up we should do".
>>
>> I agree the "else" vs "no else" is subjective, but the problem in fact
>> is that the first "if", unlike the rest of them, already had no "else",
>> making the code inconsistent.
>
> This is a static helper function about a single "optarg" string that
> wanted to say "switch(optarg) { case "off": ... }" but couldn't in
> C, and I happen to view if strcmp else if strcmp ... sequence on the
> same string a poor-man's substitute for such a construct.  So my
> take of it is to call the second "if" not being "else if" a problem,
> not the rest of it.  If we add a new condition on a different input,
> making it "if (x) ...; switch(optarg) { ... }" that talks about
> something other than optarg, then writing it all with "if" without
> "else if" would make it harder to see the pattern, but I do not care
> too deeply either way, because this is unlikely to gain any logic
> more involved than "switch(optarg) { ... }".

Well, I even tend to write this switch(optarg) as:

if (0) ;
else if(...) {
  ...
}
else if(...) { 
  ...
}

to make the first actual "case" look the same way as the rest, and then
through suitable defines it finally looks like:

IF_SWITCH
IF_CASE(...) {
  ...
}
IF_CASE(...) {
  ...
}

making the intent explicit.

Just saying.

Thanks,
-- Sergey Organov

[PATCH 2/3] diff-merges: cleanup set_diff_merges()

[Sergey Organov]

Get rid of special-casing of 'suppress' in set_diff_merges(). Instead
set 'merges_need_diff' flag correctly in every option handling
function.

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 diff-merges.c | 30 +++++++++++++++++++-----------
 1 file changed, 19 insertions(+), 11 deletions(-)

diff --git a/diff-merges.c b/diff-merges.c
index 780ed08fc87f..85cbefa5afd7 100644
--- a/diff-merges.c
+++ b/diff-merges.c
@@ -20,9 +20,20 @@ static void suppress(struct rev_info *revs)
 	revs->remerge_diff = 0;
 }
 
+static void common_setup(struct rev_info *revs)
+{
+	suppress(revs);
+	revs->merges_need_diff = 1;
+}
+
+static void set_none(struct rev_info *revs)
+{
+	suppress(revs);
+}
+
 static void set_separate(struct rev_info *revs)
 {
-	suppress(revs);
+	common_setup(revs);
 	revs->separate_merges = 1;
 	revs->simplify_history = 0;
 }
@@ -35,21 +46,21 @@ static void set_first_parent(struct rev_info *revs)
 
 static void set_combined(struct rev_info *revs)
 {
-	suppress(revs);
+	common_setup(revs);
 	revs->combine_merges = 1;
 	revs->dense_combined_merges = 0;
 }
 
 static void set_dense_combined(struct rev_info *revs)
 {
-	suppress(revs);
+	common_setup(revs);
 	revs->combine_merges = 1;
 	revs->dense_combined_merges = 1;
 }
 
 static void set_remerge_diff(struct rev_info *revs)
 {
-	suppress(revs);
+	common_setup(revs);
 	revs->remerge_diff = 1;
 	revs->simplify_history = 0;
 }
@@ -57,7 +68,7 @@ static void set_remerge_diff(struct rev_info *revs)
 static diff_merges_setup_func_t func_by_opt(const char *optarg)
 {
 	if (!strcmp(optarg, "off") || !strcmp(optarg, "none"))
-		return suppress;
+		return set_none;
 	if (!strcmp(optarg, "1") || !strcmp(optarg, "first-parent"))
 		return set_first_parent;
 	if (!strcmp(optarg, "separate"))
@@ -81,10 +92,6 @@ static void set_diff_merges(struct rev_info *revs, const char *optarg)
 		die(_("invalid value for '%s': '%s'"), "--diff-merges", optarg);
 
 	func(revs);
-
-	/* NOTE: the merges_need_diff flag is cleared by func() call */
-	if (func != suppress)
-		revs->merges_need_diff = 1;
 }
 
 /*
@@ -115,6 +122,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
 		set_to_default(revs);
+		revs->merges_need_diff = 0;
 	} else if (!strcmp(arg, "-c")) {
 		set_combined(revs);
 		revs->merges_imply_patch = 1;
@@ -125,7 +133,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 		set_remerge_diff(revs);
 		revs->merges_imply_patch = 1;
 	} else if (!strcmp(arg, "--no-diff-merges")) {
-		suppress(revs);
+		set_none(revs);
 	} else if (!strcmp(arg, "--combined-all-paths")) {
 		revs->combined_all_paths = 1;
 	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
@@ -139,7 +147,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
 
 void diff_merges_suppress(struct rev_info *revs)
 {
-	suppress(revs);
+	set_none(revs);
 }
 
 void diff_merges_default_to_first_parent(struct rev_info *revs)
-- 
2.25.1

Re: [PATCH 2/3] diff-merges: cleanup set_diff_merges()

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Get rid of special-casing of 'suppress' in set_diff_merges(). Instead
> set 'merges_need_diff' flag correctly in every option handling
> function.
>
> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  diff-merges.c | 30 +++++++++++++++++++-----------
>  1 file changed, 19 insertions(+), 11 deletions(-)

Looks OK to me.

Everybody else says set_X() but set_none() has nothing to do ther
than calling suppress(), so the change does not really make a
functional difference (as you said in the cover letter).

Is the idea that the original value in .merges_need_diff member does
not matter because in every case it is set to either 0 or 1?  Most
cases call common_setup() to set it to 1 like this here...

> +static void common_setup(struct rev_info *revs)
> +{
> +	suppress(revs);
> +	revs->merges_need_diff = 1;
> +}

... but this does not touch (in other words, it does not explicitly
clear) it, ...

> +static void set_none(struct rev_info *revs)
> +{
> +	suppress(revs);
> +}

 ... so we still rely on somebody to set the .merges_need_diff
to 0 initially, right?

> -
> -	/* NOTE: the merges_need_diff flag is cleared by func() call */
> -	if (func != suppress)
> -		revs->merges_need_diff = 1;
>  }

It is very good to see this one go.

>  /*
> @@ -115,6 +122,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>  
>  	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
>  		set_to_default(revs);
> +		revs->merges_need_diff = 0;

I am wondering how this becomes necessary?  Is it because
set_to_default() would flip the member to 1 unconditionally, or
something?  If it weren't for this hunk, the lossage of the previous
hunk is a very good clean-up, but if we need to do this, I cannot
shake the feeling that we mostly shifted the dirt around, without
really cleaning it?  I dunno.

> @@ -125,7 +133,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>  		set_remerge_diff(revs);
>  		revs->merges_imply_patch = 1;
>  	} else if (!strcmp(arg, "--no-diff-merges")) {
> -		suppress(revs);
> +		set_none(revs);

We do not need to explicitly set .merges_need_diff to 0 here,
presumably because it is initialized to 0 and nobody touched it,
right?

>  	} else if (!strcmp(arg, "--combined-all-paths")) {
>  		revs->combined_all_paths = 1;
>  	} else if ((argcount = parse_long_opt("diff-merges", argv, &optarg))) {
> @@ -139,7 +147,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>  
>  void diff_merges_suppress(struct rev_info *revs)
>  {
> -	suppress(revs);
> +	set_none(revs);
>  }
>  
>  void diff_merges_default_to_first_parent(struct rev_info *revs)

Re: [PATCH 2/3] diff-merges: cleanup set_diff_merges()

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Get rid of special-casing of 'suppress' in set_diff_merges(). Instead
>> set 'merges_need_diff' flag correctly in every option handling
>> function.
>>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  diff-merges.c | 30 +++++++++++++++++++-----------
>>  1 file changed, 19 insertions(+), 11 deletions(-)
>
> Looks OK to me.
>
> Everybody else says set_X() but set_none() has nothing to do ther
> than calling suppress(), so the change does not really make a
> functional difference (as you said in the cover letter).
>
> Is the idea that the original value in .merges_need_diff member does
> not matter because in every case it is set to either 0 or 1?  Most
> cases call common_setup() to set it to 1 like this here...

Yes, exactly.

>
>> +static void common_setup(struct rev_info *revs)
>> +{
>> +	suppress(revs);
>> +	revs->merges_need_diff = 1;
>> +}
>
> ... but this does not touch (in other words, it does not explicitly
> clear) it, ...

>
>> +static void set_none(struct rev_info *revs)
>> +{
>> +	suppress(revs);
>> +}
>
>  ... so we still rely on somebody to set the .merges_need_diff
> to 0 initially, right?

No, the suppress() does clear everything, .merges_need_diff included.
The suppress() ensures every next diff-merges option on the command-line
actually overwrites and correctly sets everything.

>
>> -
>> -	/* NOTE: the merges_need_diff flag is cleared by func() call */
>> -	if (func != suppress)
>> -		revs->merges_need_diff = 1;
>>  }
>
> It is very good to see this one go.

Yep, that was a kludge that bothered me for a while indeed.

>
>>  /*
>> @@ -115,6 +122,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>>  
>>  	if (!suppress_m_parsing && !strcmp(arg, "-m")) {
>>  		set_to_default(revs);
>> +		revs->merges_need_diff = 0;
>
> I am wondering how this becomes necessary?  Is it because
> set_to_default() would flip the member to 1 unconditionally, or
> something?

Yes, as all "native" -diff-merge= options set this bit to 1.

> If it weren't for this hunk, the lossage of the previous
> hunk is a very good clean-up, but if we need to do this, I cannot
> shake the feeling that we mostly shifted the dirt around, without
> really cleaning it?  I dunno.

Yes, I believe it does cleanup things in both these places.

The "-m" option indeed differs from the rest of the options in exactly
this thing: it wants .merges_need_diff=0 to suppress output unless '-p'
is given as well.

The -c/--cc are in fact similar, but they don't need this line as they
imply '-p' that in turn ignores .merges_need_diff, and generates output
anyway, so -m code has:

  revs->merges_need_diff = 0;

whereas -c and --cc code both have:

  revs->merges_imply_patch = 1;

Thanks,
-- Sergey Organov

>
>> @@ -125,7 +133,7 @@ int diff_merges_parse_opts(struct rev_info *revs, const char **argv)
>>  		set_remerge_diff(revs);
>>  		revs->merges_imply_patch = 1;
>>  	} else if (!strcmp(arg, "--no-diff-merges")) {
>> -		suppress(revs);
>> +		set_none(revs);
>
> We do not need to explicitly set .merges_need_diff to 0 here,
> presumably because it is initialized to 0 and nobody touched it,
> right?

No, we rather do set it to 0 here, in suppress() called from set_none().

Thanks,
-- Sergey Organov

[PATCH 3/3] diff-merges: clarify log.diffMerges documentation

[Sergey Organov]

Signed-off-by: Sergey Organov <sorganov@gmail.com>
---
 Documentation/config/log.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
index 5250ba45fb4e..cbe34d759221 100644
--- a/Documentation/config/log.txt
+++ b/Documentation/config/log.txt
@@ -30,9 +30,9 @@ log.excludeDecoration::
 	option.
 
 log.diffMerges::
-	Set default diff format to be used for merge commits. See
-	`--diff-merges` in linkgit:git-log[1] for details.
-	Defaults to `separate`.
+	Set diff format to be used when `--diff-merges=on` is
+	specified, see `--diff-merges` in linkgit:git-log[1] for
+	details. Defaults to `separate`.
 
 log.follow::
 	If `true`, `git log` will act as if the `--follow` option was used when
-- 
2.25.1

Re: [PATCH 3/3] diff-merges: clarify log.diffMerges documentation

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> Signed-off-by: Sergey Organov <sorganov@gmail.com>
> ---
>  Documentation/config/log.txt | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
> index 5250ba45fb4e..cbe34d759221 100644
> --- a/Documentation/config/log.txt
> +++ b/Documentation/config/log.txt
> @@ -30,9 +30,9 @@ log.excludeDecoration::
>  	option.
>  
>  log.diffMerges::
> -	Set default diff format to be used for merge commits. See
> -	`--diff-merges` in linkgit:git-log[1] for details.
> -	Defaults to `separate`.
> +	Set diff format to be used when `--diff-merges=on` is
> +	specified, see `--diff-merges` in linkgit:git-log[1] for
> +	details. Defaults to `separate`.
>  
>  log.follow::
>  	If `true`, `git log` will act as if the `--follow` option was used when

Is the reason why the patch drops "default" because the value given
is used only when --diff-merges=on is given, and does not kick in
when "--diff-merges=<format>" is explicitly given?

The rest of the change does make sense.

Re: [PATCH 3/3] diff-merges: clarify log.diffMerges documentation

[Sergey Organov]

Junio C Hamano <gitster@pobox.com> writes:

> Sergey Organov <sorganov@gmail.com> writes:
>
>> Signed-off-by: Sergey Organov <sorganov@gmail.com>
>> ---
>>  Documentation/config/log.txt | 6 +++---
>>  1 file changed, 3 insertions(+), 3 deletions(-)
>>
>> diff --git a/Documentation/config/log.txt b/Documentation/config/log.txt
>> index 5250ba45fb4e..cbe34d759221 100644
>> --- a/Documentation/config/log.txt
>> +++ b/Documentation/config/log.txt
>> @@ -30,9 +30,9 @@ log.excludeDecoration::
>>  	option.
>>  
>>  log.diffMerges::
>> -	Set default diff format to be used for merge commits. See
>> -	`--diff-merges` in linkgit:git-log[1] for details.
>> -	Defaults to `separate`.
>> +	Set diff format to be used when `--diff-merges=on` is
>> +	specified, see `--diff-merges` in linkgit:git-log[1] for
>> +	details. Defaults to `separate`.
>>  
>>  log.follow::
>>  	If `true`, `git log` will act as if the `--follow` option was used when
>
> Is the reason why the patch drops "default" because the value given
> is used only when --diff-merges=on is given, and does not kick in
> when "--diff-merges=<format>" is explicitly given?

Yes, exactly. It's the --diff-merges=on that uses the default diff
format. Well, in fact "-m" uses it as well, being a synonym for
--diff-merges=on, so we might consider to mention "-m" here as well, but
I thought it's not needed as we provide a link to corresponding piece of
documentation already.

I wanted to explicitly describe what log.diffMerges affects, instead of
vague "default diff format" term causing a need to figure what in fact
it means.

Thanks,
-- Sergey Organov

Re: [PATCH 3/3] diff-merges: clarify log.diffMerges documentation

[Junio C Hamano]

Sergey Organov <sorganov@gmail.com> writes:

> I wanted to explicitly describe what log.diffMerges affects, instead of
> vague "default diff format" term causing a need to figure what in fact
> it means.

Yup, excellent.