Rockbox

  • Status Closed
  • Percent Complete
    0%
  • Task Type Patches
  • Category Infrastructure → Build environment
  • Assigned To
    MikeS
  • Operating System All players
  • Severity Low
  • Priority Very Low
  • Reported Version Release 3.10
  • Due in Version Undecided
  • Due Date Undecided
  • Votes
  • Private
Attached to Project: Rockbox
Opened by MikeS - 2012-01-20
Last edited by MikeS - 2012-01-30

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

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.

Closed by  MikeS
2012-01-30 23:03
Reason for closing:  Later
Additional comments about closing:   Warning: Undefined array key "typography" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 371 Warning: Undefined array key "camelcase" in /home/rockbox/flyspray/plugins/dokuwiki/inc/parserutils.php on line 407

No point for now.

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$.

torne commented on 2012-01-20 11:28

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 :)

MikeS commented on 2012-01-20 14:28

@Torne:

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.

MikeS commented on 2012-01-20 15:38

This is what I get playing loose with the matching. See anything of interest?

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/action_helper.pl:printf "– It is automatically generated of action.h %s\n", '$Revision$';
apps/plugins/lua/button_helper.pl:$svnrev = '$Revision$';
apps/plugins/lua/rocklib_aux.pl:my $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/mp3info.pm:($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)
MikeS commented on 2012-01-20 19:51

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)

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: http://git.rockbox.org/?p=rockbox.git;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 $

MikeS commented on 2012-01-20 23:42

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.

MikeS commented on 2012-01-20 23:44

I guess they 're in the repository already that way and there's nothing more to that.

MikeS commented on 2012-01-21 00:35

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.

For example, in apps/plugins/lua/lapi.c the following comes from Lua, not Rockbox:
- $Id: lapi.c,v 2.55.1.5 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?

MikeS commented on 2012-01-21 05:58

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.

Loading...

Available keyboard shortcuts

Tasklist

Task Details

Task Editing