git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: "Seija K. via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, pi1024e <pi1024e@github.com>
Subject: Re: [PATCH] Simplified merge logic
Date: Mon, 09 Nov 2020 15:17:09 -0800	[thread overview]
Message-ID: <xmqqo8k69l3e.fsf@gitster.c.googlers.com> (raw)
In-Reply-To: <pull.911.git.git.1604871456715.gitgitgadget@gmail.com> (Seija K. via GitGitGadget's message of "Sun, 08 Nov 2020 21:37:36 +0000")

"Seija K. via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: pi1024e <pi1024e@github.com>
>
> commit: Avoid extraneous boolean checking by simplifying the if statements.
> Signed-off-by: Seija <pi1024e@github.com>
> ---

Meh.

Admittedly, readability is somewhat subjective, but a rewrite like

        if (condition)                  if (!condition)
                do_when_true();                 do_when_false();
        else                       ==>  else
                do_when_false();                do_when_true();

while it may not be incorrect per-se needs more than a subjective "I
think this is more readable" to justify the code churn.

> diff --git a/builtin/merge.c b/builtin/merge.c
> index 4c133402a6..9664da6031 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -853,9 +853,8 @@ static void prepare_to_commit(struct commit_list *remoteheads)
>  	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
>  			    git_path_merge_msg(the_repository), "merge", NULL))
>  		abort_commit(remoteheads, NULL);
> -	if (0 < option_edit) {
> -		if (launch_editor(git_path_merge_msg(the_repository), NULL, NULL))
> -			abort_commit(remoteheads, NULL);
> +	if (0 < option_edit && launch_editor(git_path_merge_msg(the_repository), NULL, NULL)) {
> +		abort_commit(remoteheads, NULL);
>  	}

This may reduce the number of lines, but personally I find that

	if (are we editing?) {
		if (run editor---did we fail?)
			abort();
	}

is much easier to read.

And much more importantly, it would be much easier to extend later
what hwppens when we decide to edit, than the new code proposed by
this patch.

> @@ -1213,7 +1212,7 @@ static int merging_a_throwaway_tag(struct commit *commit)
>  	if (!merge_remote_util(commit) ||
>  	    !merge_remote_util(commit)->obj ||
>  	    merge_remote_util(commit)->obj->type != OBJ_TAG)
> -		return is_throwaway_tag;
> +		return 0;

Likewise.  If somebody _must_ touch this function to gain commit
count without making the code harder to maintain, it may be an
option to use "goto leave;" here and then create a "leave:" label
before the other "return"---at least that may be worth considering,
but not this---when everybody else in the function wants to maintain
that the value in this variable is what is returned from the
function.

> @@ -1459,13 +1458,12 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  			fast_forward = FF_NO;
>  	}
>  
> -	if (!use_strategies) {
> -		if (!remoteheads)
> -			; /* already up-to-date */
> -		else if (!remoteheads->next)
> -			add_strategies(pull_twohead, DEFAULT_TWOHEAD);
> -		else
> +	if (!use_strategies && remoteheads) {
> +		/* not up-to-date */
> +		if (remoteheads->next)
>  			add_strategies(pull_octopus, DEFAULT_OCTOPUS);
> +		else
> +			add_strategies(pull_twohead, DEFAULT_TWOHEAD);
>  	}

Likewise.

	if (do we have to choose strategies ourselves?) {
		... depending on the case, choose strategy ...
	}

is much easier to reason about than

	if (do we have to choose strategies ourselves?, oh by the
    	    way, don't forget that already up-to-date case we do not
	    have to choose) {
		... the remainder ...
	}

and extend when we need to add something to do in the up-to-date
case as long as the end-user did not specify which strategy to use
in the future.  The reduced line count alone is not a good yardstick
to use when talking about code restructuring for readability and
maintainability.

Thanks.

  reply	other threads:[~2020-11-09 23:17 UTC|newest]

Thread overview: 4+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2020-11-08 21:37 [PATCH] Simplified merge logic Seija K. via GitGitGadget
2020-11-09 23:17 ` Junio C Hamano [this message]
2020-11-17 21:54 ` [PATCH v2] Simplify " Seija K. via GitGitGadget
2020-11-17 22:06   ` [PATCH v3] " Seija K. via GitGitGadget

Reply instructions:

You may reply publicly 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=xmqqo8k69l3e.fsf@gitster.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=git@vger.kernel.org \
    --cc=gitgitgadget@gmail.com \
    --cc=pi1024e@github.com \
    /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
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).