Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit
Dear Junio, I'm amazed at how much time and energy you spend on correcting these essentially non-issues in my git commit messages for a quadruple-liner code change. I'll resend both patches one last time addressing the grave issue of the informative mention of multi-line files. Regards, Robert
Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit
Robert Abelwrites: > 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 still do not understand why you think the 'next' person would care about the (lack of )multi-line aspect of the helper. Let's see how well the proposed log message gives the "context to the introduced changes" (from your v3). __git_eread is used to read a single line of a given file (if it exists) into a single variable without the EOL. All six current users of __git_eread use it that way and don't expect multi-line content. That does not include anything incorrect; but. The helper is about (1) reading the first line and (2) reading it as a whole into a single variable. Both are already covered by the first sentence, and there is no need to say 'and don't expect ...", unless you want to stress something. And it places a stress on the former, which is a less relevant thing, WITHOUT giving the same treatment to the latter, which is a more relevant thing. After all, this patch is not about replacing an earlier implementation that did $2=$(cat "$1") with read $2 <"$1" If that were the case, _then_ the fact that the purpose of the helper is to read from a single-liner file (i.e. we do not expect the input file to have more than one line) is VERY relevant. But this is not such a patch. And after readers read the above, they find this: Therefore, this patch removes the unused capability to split file conents into tokens by passing multiple variable names. And because the previous paragraph placed an emphasis on a wrong aspect of the context of the calls to the helper function, this "Therefore" does not quite "click" in the readers' minds. The reason why it is OK to remove the multi-variable feature is because the callers of the helper want to always read the result into a single variable, but the "no need for multi-variable" that they read in the first sentence of the previous paragraph is less strong in their mind by now, because they read an irrelevant (for the purpose of this "Therefore") mention of "no need for multi-line" aspect of the helper. Perhaps __git_eread is used to read the contents of a single-liner file into a single variable while dropping EOL. It is misleading to use the "read" built-in with "$@", as if some callers would want the contents read from the file to be split into multiple variables. Explicitly use a single variable, and also document that the helper only reads the first line (simply because the input files are designed to be single-liner files). would say it the same thing, but with emphasis on the right aspect of the facts. I would also rephase the new in-code comment # Helper function to read the first line of a file into a variable. to un-stress "the first line of a file" and place more stress on the fact that it is designed to read from a single-liner file (there is a subtle but important distinction between the two). # read the contents of a single-liner file into a variable, # while dropping the end-of-line from it. or something like that, perhaps.
Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit
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
Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit
Robert Abelwrites: > Hi Junio, > > On 04 Dec 2017 18:58, Junio C Hamano wrote: >> Robert Abel writes: >>> __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.
Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit
Hi Junio, On 04 Dec 2017 18:58, Junio C Hamano wrote: > Robert Abelwrites: >> __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. Regards, Robert
Re: [PATCH v2 1/2] git-prompt: make __git_eread intended use explicit
Robert Abelwrites: > __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. What you are changing is that the original was prepared to be given two or more variable names, and split an input line at IFS into multiple tokens to be assigned to these variables, but with this change, the caller can only use one variable and this function will not split the line and store it into that single variable. The above can easily be fixed with a bit of rewording, perhaps like: ... that way. We do not need to split the line into tokens and assign them to multiple variables---reading only into a single variable needs to be supported. While reviewing this patch, I also wondered if the "read" wants to become "read -r" or something that is even safer than simply avoiding tokenization, but after scanning to see exactly which files __git_eread is used to read from, I do not think it matters (the input will not have a backslash that would want to be protected from 'read'), so this should be OK. > Signed-off-by: Robert Abel > --- > contrib/completion/git-prompt.sh | 7 --- > 1 file changed, 4 insertions(+), 3 deletions(-) > > diff --git a/contrib/completion/git-prompt.sh > b/contrib/completion/git-prompt.sh > index c6cbef38c..41a471957 100644 > --- a/contrib/completion/git-prompt.sh > +++ b/contrib/completion/git-prompt.sh > @@ -278,11 +278,12 @@ __git_ps1_colorize_gitstring () > r="$c_clear$r" > } > > +# Helper function to read the first line of a file into a variable. > +# __git_eread requires 2 arguments, the file path and the name of the > +# variable, in that order. > __git_eread () > { > - local f="$1" > - shift > - test -r "$f" && read "$@" <"$f" > + test -r "$1" && read "$2" <"$1" > } > > # __git_ps1 accepts 0 or 1 arguments (i.e., format string)
[PATCH v2 1/2] git-prompt: make __git_eread intended use explicit
__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. Thus, add a comment and explicitly use $2 instead of shifting the args down and using $@. Signed-off-by: Robert Abel--- contrib/completion/git-prompt.sh | 7 --- 1 file changed, 4 insertions(+), 3 deletions(-) diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh index c6cbef38c..41a471957 100644 --- a/contrib/completion/git-prompt.sh +++ b/contrib/completion/git-prompt.sh @@ -278,11 +278,12 @@ __git_ps1_colorize_gitstring () r="$c_clear$r" } +# Helper function to read the first line of a file into a variable. +# __git_eread requires 2 arguments, the file path and the name of the +# variable, in that order. __git_eread () { - local f="$1" - shift - test -r "$f" && read "$@" <"$f" + test -r "$1" && read "$2" <"$1" } # __git_ps1 accepts 0 or 1 arguments (i.e., format string) -- 2.15.1