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

Ævar Arnfjörð Bjarmason [off-list ref] writes:

quoted

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