[pacman-dev] srcinfo.sh: remove trailing newline

Message ID 20200603232647.2492667-1-liu.denton@gmail.com
State Rejected, archived
Headers show
Series [pacman-dev] srcinfo.sh: remove trailing newline | expand

Commit Message

Denton Liu June 3, 2020, 11:26 p.m. UTC
When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each
section is concluded with an empty line. This means that at the end of
the file, an empty line remains. When running `git diff --check`, Git
will complain about this as a whitespace error, saying "new blank line
at EOF."

Remove the empty line after sections. Replace the empty echo with a
placeholder `true` call in case in the future, we do want to close the
section with something.
---
 scripts/libmakepkg/srcinfo.sh.in | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

Comments

Eli Schwartz June 4, 2020, 12:04 a.m. UTC | #1
On 6/3/20 7:26 PM, Denton Liu wrote:
> When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each
> section is concluded with an empty line. This means that at the end of
> the file, an empty line remains. When running `git diff --check`, Git
> will complain about this as a whitespace error, saying "new blank line
> at EOF."

git diff --check isn't necessarily our problem, though...

But it will only be reported once, the very first time you ever commit
this .SRCINFO

> Remove the empty line after sections. Replace the empty echo with a
> placeholder `true` call in case in the future, we do want to close the
> section with something.

Now you've also removed the blank line in the middle of the file,
separating the pkgbase section from each pkgname section. That is more
than just the blank line terminating the final pkgname section.

> ---
>  scripts/libmakepkg/srcinfo.sh.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
> index 6e783279..e7b5c4be 100644
> --- a/scripts/libmakepkg/srcinfo.sh.in
> +++ b/scripts/libmakepkg/srcinfo.sh.in
> @@ -31,7 +31,7 @@ srcinfo_open_section() {
>  }
>  
>  srcinfo_close_section() {
> -	echo
> +	true # nothing to be done
>  }
>  
>  srcinfo_write_attr() {
>
Allan McRae June 4, 2020, 2:42 a.m. UTC | #2
On 4/6/20 9:26 am, Denton Liu wrote:
> When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each
> section is concluded with an empty line. This means that at the end of
> the file, an empty line remains. When running `git diff --check`, Git
> will complain about this as a whitespace error, saying "new blank line
> at EOF."
> 
> Remove the empty line after sections. Replace the empty echo with a
> placeholder `true` call in case in the future, we do want to close the
> section with something.
> ---
>  scripts/libmakepkg/srcinfo.sh.in | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
> index 6e783279..e7b5c4be 100644
> --- a/scripts/libmakepkg/srcinfo.sh.in
> +++ b/scripts/libmakepkg/srcinfo.sh.in
> @@ -31,7 +31,7 @@ srcinfo_open_section() {
>  }
>  
>  srcinfo_close_section() {
> -	echo
> +	true # nothing to be done
>  }

This is a very strange approach to implementing your idea.  Now the
function is useless and should have been removed completely.  And now
there is zero separate between sections - not just the last line was
removed.

Anyway, the justification for this patch is missing.  Why does makepkg
care about a minor warning from "git diff --check"?

>  
>  srcinfo_write_attr() {
>
Eli Schwartz June 4, 2020, 2:57 a.m. UTC | #3
On 6/3/20 8:04 PM, Eli Schwartz wrote:
> On 6/3/20 7:26 PM, Denton Liu wrote:
>> When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each
>> section is concluded with an empty line. This means that at the end of
>> the file, an empty line remains. When running `git diff --check`, Git
>> will complain about this as a whitespace error, saying "new blank line
>> at EOF."
> 
> git diff --check isn't necessarily our problem, though...
> 
> But it will only be reported once, the very first time you ever commit
> this .SRCINFO

Also FWIW, I use git diff --check in my pre-commit githook for AUR
packages, but I also generate the .SRCINFO in that very hook, AFTER I
check for whitespace issues. Hence I don't see whitespace issues for the
.SRCINFO file, even for the very first commit. So I'm really not
motivated to change this.

For more details see https://github.com/eli-schwartz/aurpublish/#hooks
Erich Eckner June 4, 2020, 5 a.m. UTC | #4
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA256

Hi,

On Wed, 3 Jun 2020, Eli Schwartz wrote:

> On 6/3/20 8:04 PM, Eli Schwartz wrote:
>> On 6/3/20 7:26 PM, Denton Liu wrote:
>>> When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each
>>> section is concluded with an empty line. This means that at the end of
>>> the file, an empty line remains. When running `git diff --check`, Git
>>> will complain about this as a whitespace error, saying "new blank line
>>> at EOF."
>>
>> git diff --check isn't necessarily our problem, though...
>>
>> But it will only be reported once, the very first time you ever commit
>> this .SRCINFO
>
> Also FWIW, I use git diff --check in my pre-commit githook for AUR
> packages, but I also generate the .SRCINFO in that very hook, AFTER I
> check for whitespace issues. Hence I don't see whitespace issues for the
> .SRCINFO file, even for the very first commit. So I'm really not
> motivated to change this.

In case my voice counts anything in this discussion: I would prefer the 
empty lines between the sections and at the end to stay. I regularly parse 
.SRCINFOs (or the output of `makepkg --printsrcinfo`) and terminate 
parsing of sections on the empty line (think along the lines
`sed '/^\S/,/^$/ {...}'`).

>
> For more details see https://github.com/eli-schwartz/aurpublish/#hooks
>
> -- 
> Eli Schwartz
> Bug Wrangler and Trusted User
>

regards,
Erich

-----BEGIN PGP SIGNATURE-----

iQIzBAEBCAAdFiEE3p92iMrPBP64GmxZCu7JB1Xae1oFAl7Yf/QACgkQCu7JB1Xa
e1oQuhAArWd0GHhWTk6F4bNS+Vw6PFR7UyJmgUzeipnwBFpXhntUwv4SiygGCaY4
XQFcEByjlhMz6xfL7t2ZfBfxpudH0TlypFdHUV9H2ujgJHi82+1XM13H9R/KMY56
I+YLO5iahPCVu/54licMWiSeb8Vu9ZBOgK5M4bm2YPfcADFaS0FiqZ8bwp3LCe3y
JLiXZ3NOJj4U94OLXA6K1gMUkbn1ZX4TExS+qLGPqLo7W0rPCfZo+FyushNHGjv1
MUUKzDVgXMF06pgN1vCkHWuEmV9HKPibCD1265jbHcgXACzl9qv8wB/AJx21oN6G
UgfzkTxu4LfaoVHsGA0be2vHHcdYYDbQLYJNEVm5DfNj7zeYwp1GFsGIarLz6BQT
FnTpXkni2RFNvVJskDnEAXBsgv0u1Ml5CBJegrPBPhuaFXZ8sk9HVwkb+26DtSQr
/5NyouwErASPDU2kDWnXJ8coJGfqGTmTzAAg1Y2S9/1/uVC9xx9Trya83MBxJ7eF
0bIDWngQNtCuEceEugXtM8W4DqkEFGrWGWiNwX632XhqPmLDuEZBz9HppgNYioHK
ae+rklION7jOCHgu+kHH0DQKwL/V6QViLtL8FYn6M0hHe6ncjxUxta+3FYpmdnTR
Xr98++0JRG9FtWIVOPCGnGU0Qfpig8IJk+dYYv0FNhKvahGn5q4=
=vQdo
-----END PGP SIGNATURE-----
Eli Schwartz June 4, 2020, 5:34 a.m. UTC | #5
On 6/4/20 1:00 AM, Erich Eckner wrote:
> In case my voice counts anything in this discussion: I would prefer the
> empty lines between the sections and at the end to stay. I regularly
> parse .SRCINFOs (or the output of `makepkg --printsrcinfo`) and
> terminate parsing of sections on the empty line (think along the lines
> `sed '/^\S/,/^$/ {...}'`).

I should probably clarify that the empty lines as section separators are
part of the current format specification and as far as I'm concerned,
removing them is simply not up for discussion. Any proposed patch *must*
retain those empty lines.

Your use case is a good example of why. And I have no reason to think
you're the only person in the entire userbase who is parsing srcinfo
content with this assumption.


Removing the *final* trailing newline could in theory be done (any
srcinfo parser would presumably terminate at EOF anyway) and that, I'm
"merely unconvinced" about implementing. I still don't think a
compelling case has been made for it, but I'm willing to be convinced.
Denton Liu June 8, 2020, 10:06 a.m. UTC | #6
Hi Eli,

Sorry for the late reply.

On Wed, Jun 03, 2020 at 08:04:59PM -0400, Eli Schwartz wrote:
> On 6/3/20 7:26 PM, Denton Liu wrote:
> > When the .SRCINFO file is generated via `makepkg --printsrcinfo`, each
> > section is concluded with an empty line. This means that at the end of
> > the file, an empty line remains. When running `git diff --check`, Git
> > will complain about this as a whitespace error, saying "new blank line
> > at EOF."
> 
> git diff --check isn't necessarily our problem, though...
> 
> But it will only be reported once, the very first time you ever commit
> this .SRCINFO

You are correct that it's only reported once. I guess my problem isn't
with `git diff --check` in particular but my problem is that the tool is
generating files that has a whitespace error. (I consider a blank line
at EOF to be an instance of trailing whitespace which should be
considered a whitespace error.)

> > Remove the empty line after sections. Replace the empty echo with a
> > placeholder `true` call in case in the future, we do want to close the
> > section with something.
> 
> Now you've also removed the blank line in the middle of the file,
> separating the pkgbase section from each pkgname section. That is more
> than just the blank line terminating the final pkgname section.

This was intentional. I assumed that the blank line separators were
merely aesthetic since in the wiki, it says "Blank lines [...] are also
ignored." Since the .SRCINFO is supposed to be machine-parsed, I
assumed that the aesthetics didn't really matter and we could follow an
approach similar to how .gitconfig's are laid out with sections squashed
together without any blank lines in between.

Anyway, this is a moot point since downthread, Erich mentioned that he
uses the blank lines in his parsing script and I'd hate to break
backwards compatibility.

If you accept the above justification of a trailing newline at EOF
being a whitespace error, my next reroll of the patch will only remove
the _final_ blank line, not the ones in between.

Thanks for the feedback,

Denton

Patch

diff --git a/scripts/libmakepkg/srcinfo.sh.in b/scripts/libmakepkg/srcinfo.sh.in
index 6e783279..e7b5c4be 100644
--- a/scripts/libmakepkg/srcinfo.sh.in
+++ b/scripts/libmakepkg/srcinfo.sh.in
@@ -31,7 +31,7 @@  srcinfo_open_section() {
 }
 
 srcinfo_close_section() {
-	echo
+	true # nothing to be done
 }
 
 srcinfo_write_attr() {