Re: [PATCH v3 0/3] branch: operations on orphan branches
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 8, 2023, 18:37 UTC
- Message-ID
- <xmqqa61oroko.fsf@gitster.g>
- In-Reply-To
- <230207.86cz6l501v.gmgdl@evledraar.gmail.com>
Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:
Show 10 quoted lines
>> - 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.
Show 11 quoted lines
> 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.