Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit

Robert Abel [off-list ref] writes:

Hi Junio,

On 04 Dec 2017 18:58, Junio C Hamano wrote:

quoted

Robert Abel [off-list ref] writes:

quoted

__git_eread is used to read a single line of a given file (if it exists)
into a variable without the EOL. All six current users of __git_eread
use it that way and don't expect multi-line content.

Changing $@ to $2 does not change whether this is about "multi-line"
or not. 

I'm aware of that. I was documenting current usage. The function is used
to read file contents (which are expected to be a single line) into
_a_ (i.e. single) variable.

None of the current users of the function expect tokens to be split,
which is why I removed it in preparation of patch 2/2, which would
break tokenizing file contents.

I know all of the above, but I think you misunderstood the point I
wanted to raise, so let me try again.  The thing is, none of what
you just wrote changes the fact that lack of callers that want to do
"multi-line" is IRRELEVANT.  True, there is no caller that wants to
read multiple lines---it is a true statement, but it is irrelevant
statement.  On the other hand, it is true and relevant that no
caller expects to split a line into multiple variables.

By changing "$@" to "$2" there, you would have broken callers that
wanted the helper function to read into multiple variables (if there
were such callers).  Explaining the current usage that nobody does
so *IS* a valid justification for the change.  It is relevant.

With or without that change, a caller that wanted to read multiple
lines from the file would never have worked.  It was just doing a
single "read" built-in, so the only thing that would have been
worked on is the first line of the file.  Your change wouldn't have
changed that---if a caller wanted to peek into the second line, your
change wouldn't have helped such a caller.  And it is not like your
change would have broken such a caller that were happily reading the
second and subsequent line.  The original wouldn't allowed it to
read the second line anyway.

Contrasting this with the above obsesrvation about possible breakage
for multi-variable callers (if there were such callers---luckily
there wasn't any), I hope that you can see why the lack of
"multi-line" caller in the existing usage is totally irrelevant when
analyzing this change and explaining why this is a good change.

HTH.

Hi Junio,

On 05 Dec 2017 01:27, Junio C Hamano wrote:

I know all of the above, but I think you misunderstood the point I
wanted to raise, so let me try again.  The thing is, none of what
you just wrote changes the fact that lack of callers that want to do
"multi-line" is IRRELEVANT.

I disagree. The commit comment is meant to give context to the
introduced changes. One change is the  additional comment for
__git_eread, which now clearly states that only a single line is read.

I'm well aware that I'm not breaking reading multiple lines, because
that never worked in the first place. Thus, it was never the indented
use for __git_eread as I see it. I explicitly want to include that
information in my commit message to pay it forward to the next person
working on the prompt.

Regards,

Robert