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

[Junio C Hamano <gitster@pobox.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.

> 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.