line endings in svn

[reyalp]

Based on the above comments I have the following ready to commit on the new trunk. Unless there are objections pretty quickly, I'll check it in.

- All normal source files (.c .h .S) and .txt files in the development tree have eol-style native

- Files intended for distribution (.txt files in CHDK, .grd, lng, .bas files) have eol-style CRLF. (the cameras native format is LF, but this is probably more friendly to average users) see below

- makefiles and sh scripts have eol-style LF, since they might otherwise cause problems for people using cygwin in their build environment on windows.

If you try to check in one of the files in a format that doesn't match your platform, svn will automatically do the right thing, so you don't need to remember which files are set to which. However, if you try to check in a file that has svn:eol-style set, and some lines in each format, svn will refuse to check it in.

It also refuses to apply svn:eol-style to a file like this. I have manually fixed up all the files that were checked in this way.

Unfortunately, some of the automatically generated files end up this way when generated on windows. They should probably be tweaked to produce consistent line endings at some point.

Assembla doesn't yet let us use commit hooks on the svn server to enforce particular properties. That means anyone adding files needs to set them manually. You can automate this on your client with Automatic Property Setting. We might also want to look into using TortoiseSVN Project Properties.

Tabs vs spaces, indenting etc. are all a project for a another day.

I've attached the script I used to set the props, in case anyone is interested.

[PhyrePhoX]

Great,i'm all for it.

[fudgey]

My 2 cents about CRLF in .bas: We have a scrict 8 kB file size limit for ubasic scripts. Adding CR's wastes precious bytes and shouldn't be done.

I have a real-world example... I haven't actually been using my MDFB script for months... I use a a570is specific MDFB2 script, which currently weighs in 8026 bytes using LF line endings. It has 285 lines, so converting to CR+LF would bring it past 2^13 bytes and the script would mysteriously fail if it was ever committed. An automatic conversion would thus be dangerous to the extreme.

[reyalp]

Good point about ubasic scripts. In the long term, this limit should probably be lifted, but regardless it's a waste of space. I don't see any reason why the buffer couldn't be malloc'd instead. (There's a downside that this memory is always allocated when you have a script selected, but that's another thing that could be improved)

Also on further reflection the .lng files are not generally read/modified by the end user, so should probably be LF to save a little bit of malloc memory.

Updated policy:
.TXT files destined for the camera will be in CRLF format. All other text files will be in LF format.

[fudgey]

Or the CR's could be filtered out while loading script/lng files? SD card space is not an issue after all.

[reyalp]

Committed.

Or the CR's could be filtered out while loading script/lng files? SD card space is not an issue after all.

You could, but unless people getting confused by LF format is a big problem, it's probably more trouble than it's worth.

Note that none of the stuff with SVN prevents end users from making their own files in CRLF format, so it shouldn't be a big deal.

For ubasic, you could actually load it to a much smaller representation if you really wanted.

[fudgey]

For ubasic, you could actually load it to a much smaller representation if you really wanted.

Hmm yea, and now that you mentioned that... skipping rem lines at script load would be a huge improvement compared to skipping CRs since rems eat 10 ms each in ubasic execution AND they can be quite lengthy (I'm aware that removing them slightly changes behavior of existing scripts but anyone using rem as sleep deserves either a spanking or an obfuscation award). That forces people comment scripts sparingly since comments can only be placed between subroutines without affecting performance.

[bugsplatter]

Reviving an old topic.

Since whitespace is more than line endings, which is a non-issue 'cos any decent windows editor can be set to use unix newlines, the real issue IMHO is the tabbing and general flow.  I submit that the linux-kernel Documentation/CodingStyle is a good starting point for the C code.  It's in use by thousands of GPL developers and has a history, rather than reinvent the wheel.

Here's a copy: http://lxr.linux.no/linux/Documentation/CodingStyle

It uses 8-space tabs, but there's a rationale behind this which makes sense to me.  

Other stuff like line length could be wider than 80 these days, but >130 chars as seen in chdk seems excessive.  

The rest of CodingStyle is a decent standard worth using, as if you're going to pick a standard, l-k being such a large GPL project is surely somewhere worth having a look at.

Anyway, since the source is still a mess I'll have a look at what l-k/scripts/Lindent produces and submit some processed cleanups, after going through the files, if they seem sensible.  Doing some janitorial work will give me an idea of where stuff is in the source too  

I'm still trying to work out a workflow for chdk on linux, I used svn copy, but svn diff didn't do what I expected after patching my work copy  

I may stay with hardlinked working copy and the tools I'm used to (vim, diff, patch), I've never used an SCM.

... providing I still have an account here after some comments in another thread

[reyalp]

I don't care too much what code style is used. I don't really like 8 space indent, but I can live with it. I'd prefer something like the apache standard http://httpd.apache.org/dev/styleguide.html The main difference being that indent is 4, using spaces, tabs forbidden. This is a bit closer to how things are now.

I don't even really care if there is a consistent style through all the code.

What I absolutely hate is formatting that obfuscates the function of the code. I also hate gigantic if statements all on one line.

Running the entire* codebase though a pretty printer would be OK by me, but if the people with commit access CBA to format their code, then it's not worth it the added noise in the history.

* except code that comes from outside sources (lua is the only significant bit, ATM). This should not be re-formatted, since it would only make merging future upstream changes harder.

I'm still trying to work out a workflow for chdk on linux, I used svn copy, but svn diff didn't do what I expected after patching my work copy

Err what ? You can't even do an svn copy without commit access (unless maybe you made your own repository ?). You should be using checkout.

[bugsplatter]

I don't care too much what code style is used. I don't really like 8 space indent, but I can live with it. I'd prefer something like the apache standard http://httpd.apache.org/dev/styleguide.html The main difference being that indent is 4, using spaces, tabs forbidden. This is a bit closer to how things are now.

Okay

I don't even really care if there is a consistent style through all the code.

Fair enough. There's parts of the code I can't read easily...

What I absolutely hate is formatting that obfuscates the function of the code. I also hate gigantic if statements all on one line.

...which is what you're saying here, if the logic is hard to see, then I want to fix the whitespace to make the logic clear.

Running the entire* codebase though a pretty printer would be OK by me, but if the people with commit access CBA to format their code, then it's not worth it the added noise in the history.

Well l-k has a policy of only doing whitespace cleanup prior to altering the code, so the WS cleanup is part of the code cleanup.  Running entire code through a mindless script can do odd stuff too, needs a human to check the output.

I'm still trying to work out a workflow for chdk on linux, I used svn copy, but svn diff didn't do what I expected after patching my work copy

Err what ? You can't even do an svn copy without commit access (unless maybe you made your own repository ?). You should be using checkout.

Local copy, I worked it out.

No way do I want commit access -- prefer prior patch review

I'm working on a copy of chdk so I can snuff it and start over without downloading again from svn server -- but then I svn diff the local work copy, not local work vs local chdk which is what I first tried, getting the bad result.

Have put a query up on mantis as there's something odd happening with the gui.c compiler conditionals.

[reyalp]

Have put a query up on mantis as there's something odd happening with the gui.c compiler conditionals.

Meaning: http://chdk.kernreaktor.org/mantis/view.php?id=288
This is an excellent example of the kind of code that make me angry. If the nonsensical indenting weren't enough, there's ifdefs sprinkled between ifs and elses and in the middle aaaaaaaaaaaaaaaaaaaaaaaargh.  

The forum may be better for that kind for discussing code like this.

edit:
btw, you can use svn blame to see what the commits involved in creating this mess were, which may give you some idea what the code was trying to do. I've added a note to the bug.

[bugsplatter]

btw, you can use svn blame to see what the commits involved in creating this mess were, which may give you some idea what the code was trying to do. I've added a note to the bug.

I had to go lie down and shut my eyes for an hour after looking at, and reporting that mess, glad it's not me seeing things

[fe50]

I had to go lie down and shut my eyes for an hour after looking at, and reporting that mess, glad it's not me seeing things

LOL - the gui.c really needs more than one booze (grog ?) to wade through...

btw - i personally don't see why formating is suuuuch a big problem for some people; for me it's just a matter of how use/set up an editor...
Uniform line endings would be nice, but can't this (at least for the trunk) be done easily with a simple SVN property setting ( & isn't this set up this way already) ?
About using spaces, tabs4, tabs8 ... also very editor dependend, i prefer real TABs (ascii 7) & set it up to my needs on the editor i use (showing TABs as 2 space).
Nowadays with big monitors a line length up or above 130 chars imo is also not a big problem...

Sometimes the code itself should be more clear and logical, (e.g. the USB remote code in the kbc.c files), also everybody should use more comments (like reaylP does) to make the code clearer...how the CHDK sources looks like without comments and styling elements we can see when we look to into the sdm code.

[fudgey]

My 2 cents...

8 chars wide indent is probably too much considering CHDK is for a large part pretty messy code and fixing that... well... for the most part might never happen as it works after all, and many things that are messy are really quite complex (and this is only partially because the code is messy).

I sort of hate tabs in code, because handling the same file in a dozen different editors (with another dozen different indent configurations) in my limited & unprofessional experience eventually often somewhat breaks them, especially if tab size is not 8 (really, tabs are 8 chars wide and that's pretty much it, even if most current editors and even less(1) can be configured to display them differently...)

I'm with reyalp on this; I'd vote for 4 spaces for indent, no tabs, but don't feel strongly about it... almost anything is better than no policy, and it would be great if someone took the trouble to go through all the code and made it prettier.


One more thing to remember when reformatting... the build process autogenerates some files from source code (from the top of my head I remember two: platform/*/sub/*/stubs_auto.S and CHDK/LUALIB/GEN/*) using some funky regexp stuff. Try not to break that (or fix the generators I suppose) and note that to test that you didn't you should delete all those files before testing batch build, otherwise make might not regenerate...

[reyalp]

Nowadays with big monitors a line length up or above 130 chars imo is also not a big problem...

Monitor space isn't the only problem. Formatting is there to help you understand what the code is doing.

Stuff like

Code: [Select]

if ((gui_mode==GUI_MODE_NONE || gui_mode==GUI_MODE_ALT) && (((kbd_is_key_pressed(KEY_SHOOT_HALF) || (state_kbd_script_run) || (shooting_get_common_focus_mode())) && (mode_photo || (m&MODE_SHOOTING_MASK)==MODE_STITCH )) || ((mode_video || movie_status > 1) && conf.show_values_in_video) )) {
Isn't hard to follow just because the line is long, it doesn't give any help figuring out which terms go together. Or maybe I'm just old and stupid.

That one is hard to even figure out what to do with (a sign the code should probably be re-factored) but at least something like this:

Code: [Select]

if ( gui_mode==GUI_MODE_NONE || gui_mode==GUI_MODE_ALT ) {
    if( ( ( kbd_is_key_pressed(KEY_SHOOT_HALF) || state_kbd_script_run || shooting_get_common_focus_mode() )
           && (mode_photo || (m&MODE_SHOOTING_MASK)==MODE_STITCH ) )
         || ((mode_video || movie_status > 1) && conf.show_values_in_video) ) {
gives you some hints.

Sometimes the code itself should be more clear and logical, (e.g. the USB remote code in the kbc.c files), also everybody should use more comments (like reaylP does) to make the code clearer...how the CHDK sources looks like without comments and styling elements we can see when we look to into the sdm code.

Formatting is there to show you what the code is doing. Random indent levels that obfuscate the structure of the code are big deal for old stupid people like me. I frequently find myself having to reformat stuff just so I can figure out WTF it is trying to do. The code bugsplatter posted is a good example... the indenting doesn't reflect the flow control at all. a lot of the kbd and remote stuff is the same.

fudgey:
The auto generated stuff should be ok (since we already have mixed tabs and spaces all over the place) but it does your right, it should be checked.