"Seija K. via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> From: pi1024e <pi1024e@xxxxxxxxxx>
>
> commit: Avoid extraneous boolean checking by simplifying the if statements.
> Signed-off-by: Seija <pi1024e@xxxxxxxxxx>
> ---
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.
