Re: [PATCH v3 0/3] branch: operations on orphan branches

[Junio C Hamano <gitster@pobox.com>]

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

>>  - Renamed "ishead_and_reject_rebase_or_bisect_branch()" to
>>    "die_if_branch_is_being_rebased_or_bisected()"
>
> Looking this over holistically, I think this is a great example of where
> factoring something out into a function is just making readbility
> worse. This function is only used in copy_or_rename_branch(), and the
> overloaded name & semantics are making things quite confusing.
>
> Whereas if we just start by pulling it into its only caller I think this
> gets much better,...

Hmph, I hadn't considered it, but with only a single caller that
becomes a viable alternative.

> Another thing that I think could be improved in this series is if you
> skip the refactoring-while-at-it of changing the existing
> "if/if/die/die" into a "if/die/?:".
> ...
> I.e. your refactoring of this in 2/3 turns out to in the end have just
> been inflating the code change, for no functional benefit.
>
> I wouldn't mind if this were in some pre-cleanup, or if it actually made
> the code easier to read, but IMO this pattern of using a ternary to
> select the format to "error" or "die" makes things worse for
> readability. It's a few bytes less code, but makes things harder to follow overall.

Good.

Thanks for carefully reading.