Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> 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.
