Re: [PATCH v2 04/10] diffcore-rename: extend cleanup_dir_rename_info()

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 25, 2021, 02:34 UTC
Message-ID: <xmqqim6gyix5.fsf@gitster.g>
In-Reply-To: <87v9ag7uyx.fsf@evledraar.gmail.com>

Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:

> But in this case where we've gone through stages of changing code that's
> never been used I think we're making it harder to read than not. I'd
> prefer just to see this cleanup_dir_rename_info() function pop into
> existence in 05.

I had a similar impression on parts of the earlier series where a new helper without users were given as a standalone patch. I found it a bit disorienting.

Show 12 quoted lines

> Style nit/preference: I think code like this is easier to read as:
>
>     if (simple-case) {
>         blah
>         blah;
>         return;
>     }
>     complex_case;
>
> Than not having the "return" and having most of the interesting logic in
> an indented "else" block. Or maybe just this on top of the whole thing
> (a -w diff, hopefully more readable, but still understandable):

Yes, that is also a good tip for a more readable patch, but that applies only for if/else at the end of the function.

In general, formulating the condition so that the smaller body comes first for "if" and the larger one goes to the body of "else" would make the if/else easier to understand, as you can often hold the condition and smaller body just before "else" in your head, and after clearly understanding that part, it becomes easier to concentrate on the other side, i.e. "now we know what happens if the condition is true, what about the other case? Let's read on ...".

Thanks.