FS#12554 - Strip SVN $Id$ field from all files

Attached to Project: Rockbox
Opened by Michael Sevakis (MikeS) - Friday, 20 January 2012, 00:44 GMT
Last edited by Michael Sevakis (MikeS) - Monday, 30 January 2012, 23:03 GMT
Task Type Patches
Category Build environment
Status Closed
Assigned To Michael Sevakis (MikeS)
Operating System All players
Severity Low
Priority Normal
Reported Version Release 3.10
Due in Version Undecided
Due Date Undecided
Percent Complete 0%
Votes 0
Private No


Not many details. If we're not going to use it, then strip the $Id$ field that used to hold file properties from everything, if we care to.
This task depends upon

Closed by  Michael Sevakis (MikeS)
Monday, 30 January 2012, 23:03 GMT
Reason for closing:  Later
Additional comments about closing:  No point for now.
Comment by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 01:31 GMT
It would be possible to have git put the sha1 there, or to use custom clean and smudge filters to put other info there. However, that's only useful when the file has been removed from version control, and it's not possible to obtain that information from git. I don't think there's a need to support that, so I agree with the removal of $Id$.
Comment by Torne Wuff (torne) - Friday, 20 January 2012, 11:28 GMT
Two things:
1) There are some files that use things other than $Id$ - we should remove all the expansions.
2) When I looked at this before it looked like there were some files whose $Id$ lines were coming from upstream files and may have been pre-expanded - if we can sanely identify which these are it might be nice to leave them alone since it reduces the diff from upstream :)
Comment by Michael Sevakis (MikeS) - Friday, 20 January 2012, 14:28 GMT

1) So far I haven't run into them and so don't know what to search for, unless using "$<some max num chars>$" is enough to catch them
2) I didn't catch that here. I strictly searched "$Id$" replacement for now.
Comment by Michael Sevakis (MikeS) - Friday, 20 January 2012, 15:38 GMT
This is what I get playing loose with the matching. See anything of interest?
Comment by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 18:36 GMT
Here's what I found. I included the filenames, because they can help decide what to do and they can help accurately apply an automated edit. The list may be complete, but some lines need special handling. I'll paste some of them here:

apps/scrobbler.c:#define SCROBBLER_REVISION " $Revision$"
apps/plugins/lua/ "-- It is automatically generated of action.h %s\n", '$Revision$';
apps/plugins/lua/$svnrev = '$Revision$';
apps/plugins/lua/ $svnrev = '$Revision$';
rbutil/rbutilqt/Info.plist: <string>SVN $Revision$ (1.2.11)</string>
rbutil/rbutilqt/version.h:#define PUREVERSION "SVN $Revision$"
tools/configure:scriptver=`echo '$Revision$' | sed -e 's:\\$::g' -e 's/Revision: //'`
tools/$REVISION) = ' $Revision$ ' =~ /\$Revision:\s+([^\s]+)/;

It should be possible to identify many files coming from upstream based on path. I'm not sure if unexpanded tags should be kept just to reduce the diff. However, tags that have been expanded using information from the upstream repository should probably be kept to help figure out what version the code was taken from.
   search2.txt (136.2 KiB)
Comment by Michael Sevakis (MikeS) - Friday, 20 January 2012, 19:51 GMT
Why do you have expanded $Id$ in many of yours? None of my files are currently as such. Here's the same overly "greedy" search I did previously with the filenames included this time. Compare ./rbutil/ipodpatcher/fat32format.c for instance.
   search3.txt (135.7 KiB)
Comment by Boris Gjenero (dreamlayers) - Friday, 20 January 2012, 22:25 GMT
I think you're confusing two files:
grep fat32format search2.txt search3.txt
search2.txt:bootloader/fat32format.c: * $Id: fat32format.c 30351 2011-08-25 19:58:47Z thomasjfox $
search2.txt:rbutil/ipodpatcher/fat32format.c: * $Id$
search3.txt:./rbutil/ipodpatcher/fat32format.c: * $Id$

bootloader/fat32format.c definitely has an expanded Id:;a=blob;f=bootloader/fat32format.c;h=540ee89b548739cd7cfd0a58507f68fd37953c0e;hb=HEAD

You may want to examine what's only in one of the two files, for example starting with diff <(sort < search2.txt) <(sed "s/^\.\///" search3.txt | sort)

Here are two more special cases:
manual/pdfdraftcopy.sty:\Filedate$Date: 2003/08/11 20:31:07 $
manual/pdfdraftcopy.sty:\Fileversion$Revision: 1.2 $
Comment by Michael Sevakis (MikeS) - Friday, 20 January 2012, 23:42 GMT
Argh, I used the wrong regex, not what I thought I did. Ok, there's nothing special about the ones that are expanded over the ones that aren't that I can see.
Comment by Michael Sevakis (MikeS) - Friday, 20 January 2012, 23:44 GMT
I guess they 're in the repository already that way and there's nothing more to that.
Comment by Michael Sevakis (MikeS) - Saturday, 21 January 2012, 00:35 GMT
Fine, here's one that unexpands those. Anyway, just commit to having straggling "$Id$" or not IMHO. It's a heck of alot easier to leave it if it is wanted for later than to restore it all.
Comment by Boris Gjenero (dreamlayers) - Saturday, 21 January 2012, 05:19 GMT
For example, in apps/plugins/lua/lapi.c the following comes from Lua, not Rockbox:
-** $Id: lapi.c,v 2008/07/04 18:41:18 roberto Exp $
+** $Id$
I think the information could be useful if someone needs to backport fixes from Lua. It could be used to figure out what version of Lua the file originated from. I guess it's not useful for decreasing the diff from upstream, because it will be changed whenever the file is updated in Lua. I'm attaching a file containing parts of search2.txt which are in code from elsewhere, which doesn't have a Rockbox header. It could be used to avoid changing those parts, if desired.

Regarding the special cases I mentioned earlier, git $Id$ could be used instead of $Revision$ wherever expansion is needed. The only non-cosmetic issue I found is in rbutil/rbutilqt/. Currently, rbutilqt.cpp won't notice a version change because the string will always literally be "SVN $Revision$", so the user won't be asked to verify the settings. In configure, scriptver isn't used so it can just be deleted. There is no need to touch manual/pdfdraftcopy.sty. Should the git attributes go into .gitattributes in the directory where the file is or elsewhere?
Comment by Michael Sevakis (MikeS) - Saturday, 21 January 2012, 05:58 GMT
Sure, we can hand prune some things where rockbox SVN isn't the one responsible for the expansion. However, some were added even though they weren't rockbox headers. The ones in libmpeg2 for mpegplayer I added myself.