Re: [PATCH v3 00/38] Teach the sequencer to act as rebase -i's backend

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Hi Junio,

On Mon, 9 Jan 2017, Junio C Hamano wrote:

> I however think that the renaming of read_author_script() is totally
> backwards from maintainability's point of view.  

You stated elsewhere that converting a script into a builtin should focus
on a faithful conversion.

The original code is:

	. "$author_script"

Granted, this *cannot* be converted faithfully without reimplementing a
shell interpreter. So I did the next best thing: I converted it into code
that reads a block of environment variable settings.

What you asked for is totally unreasonable: you ask me to make this
conversion *even less faithful*.

What is worse: you argue from the "maintainability point of view", when it
is pretty obvious that *adding validation that was not there before* can
in no way make the code more maintainable, as it *adds new logic*.

And what is the worst: over all these discussions about a nothingburger
(you simply cannot convince me that I should introduce validating code
that has not been there before in the same patch series that simply tries
to recreate existing functionality), the most important part of a code
review was forgotten: to make sure that the changes are correct.

The worst direction a code review can take is to introduce regressions,
and that is exactly what happened.

Ciao,
Johannes



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]