October 2011 - YaST Development - openSUSE Mailing Lists

[yast-devel] Re: [yast-commit] r66428 - in /branches/SuSE-Code-11-SP2-Branch/s390: package/yast2-s390.changes src/modules/DASDController.ycp
by Ladislav Slezak 13 Oct '11

13 Oct '11

Dne 13.10.2011 15:35, aschnell(a)svn2.opensuse.org napsal(a): > + /* > + l = maplist(map<string, any> m, l, { > + return filter(string k, any v, m, { > + return contains([ "channel", "dev_name" ], k); > + }); > }); > + */ Avoid commenting dead code, just remove it (it's still in SVN history if you need to recover it later, just use a descriptive commit message so you can later find it easily) (IMO commenting is acceptable if you need to temporarily disable some code. But don't forget to add some FIXME comment why it was disabled in that case.) > + return $[ "devices" : l ]; Oh! At first sight I was wondering why number one was returned here, then I noticed that there is "l" (letter L) not "1" (one). In fixed font in my email client they look very similar... Use more descriptive variables if possible, especially when a typo can be overlooked like in this case... -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

2 1

[yast-devel] Re: [yast-commit] r66414 - in /branches/SuSE-Code-11-SP2-Branch/snapper: VERSION package/yast2-snapper.changes src/dialogs.ycp
by Ladislav Slezak 13 Oct '11

13 Oct '11

Dne 13.10.2011 10:41, jsuchome(a)svn2.opensuse.org napsal(a): > + if (!textmode) > + { > + // show fixed font in diff > + diff = "<tt>" + diff + "</tt>"; > + } Is the condition necessary? I'd expect that ncurses would do nothing if <tt> is found and simply ignore it as they cannot change terminal font. The same with <font>. Or does it cause some side effects in ncurses UI? -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

3 4

[yast-devel] Re: [yast-commit] r66356 - String::Quote misused, misdesigned
by Martin Vidner 12 Oct '11

12 Oct '11

String::Quote is actually an awful name: String::Quote("Martin's File; rm -rf /") -> "Martin'\\''s File; rm -rf /" To be of any use, you must wrap its result with single quotes. On Mon, Oct 10, 2011 at 10:54:08AM -0000, jsuchome(a)svn2.opensuse.org wrote: > Author: jsuchome > Date: Mon Oct 10 12:54:07 2011 > New Revision: 66356 > > URL: http://svn.opensuse.org/viewcvs/yast?rev=66356&view=rev > Log: > - agent: pass map refereces as arguments > - quote strings passed to bash commands > - fixed testsuite > - 2.21.13 > @@ -636,7 +637,7 @@ > files = filter (string file, files, { > if (haskey (files_index, file)) > { > - to_restore = add (to_restore, Snapper::GetFileFullPath (file)); > + to_restore = add (to_restore, String::Quote (Snapper::GetFileFullPath (file))); > return true; > } > else > This is suspicious. And indeed, to_restore is not passed to .target.bash later. In this case it is only displayed to the user. .target.bash sucks. -- Martin Vidner, YaST developer http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu

2 1

[yast-devel] Re: [yast-commit] r66356 - file modes
by Martin Vidner 12 Oct '11

12 Oct '11

On Mon, Oct 10, 2011 at 10:54:08AM -0000, jsuchome(a)svn2.opensuse.org wrote: > Author: jsuchome > Date: Mon Oct 10 12:54:07 2011 > New Revision: 66356 > > URL: http://svn.opensuse.org/viewcvs/yast?rev=66356&view=rev > URL: http://svn.opensuse.org/viewcvs/yast/trunk/snapper/src/Snapper.ycp?rev=6635… > @@ -131,7 +131,8 @@ > > // check mode and ownerships > out = (map) SCR::Execute (.target.bash_output, > - sformat ("ls -ld %1 %2 | cut -f 1,3,4 -d ' '", file1, file2)); > + sformat ("ls -ld -- '%1' '%2' | cut -f 1,3,4 -d ' '", > + String::Quote (file1), String::Quote (file2))); > list<string> parts = splitstring (out["stdout"]:""," \n"); > > if (parts[0]:"" != parts[3]:"") > @@ -284,8 +285,12 @@ > */ > integer GetFileMode (string file) { > > - map out = (map) SCR::Execute (.target.bash_output, "/bin/stat --printf=%a " + file); > - return tointeger (out["stdout"]:"755"); > + map out = (map) SCR::Execute (.target.bash_output, > + sformat ("/bin/stat --printf=%%a '%1'", String::Quote (file))); > + string mode = out["stdout"]:""; > + if (mode == nil || mode == "") > + return 644; > + return tointeger (mode); > } > > /** To both of the above: .target.stat ... oh wait, it doesn't include the file mode. Does that mean we are stuck with ugly workarounds like this forever? You can ask for a straightforward extension of that API or even implement it yourself. It's actually easy. And BTW, GetFileMode should return a string, because it is later used as a string. 0644 != 644. -- Martin Vidner, YaST developer http://en.opensuse.org/User:Mvidner Kuracke oddeleni v restauraci je jako fekalni oddeleni v bazenu

1 0

[yast-devel] Re: [yast-commit] r66392 - in /trunk/snapper/agent-snapper/src: SnapperAgent.cc SnapperAgent.h
by Ladislav Slezak 12 Oct '11

12 Oct '11

Dne 12.10.2011 13:43, jsuchome(a)svn2.opensuse.org napsal(a): > +int SnapperAgent::getIntValue (const YCPMap &map, const YCPString &key, const int &deflt) Const reference to int doesn't make much sense, const int is enough. Size of int is 32bit on both i586/x86_64, but pointer to int (actually any pointer), which is used for passing reference, is 64bit on x86_64. (And reference needs a memory access while int can be stored directly in a CPU register - but it depends on gcc optimizations I guess...) -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

2 2

[yast-devel] Re: [yast-commit] r66356 - in /trunk/snapper: ./ agent-snapper/src/ package/ src/ testsuite/tests/
by Ladislav Slezak 12 Oct '11

12 Oct '11

Dne 10.10.2011 12:54, jsuchome(a)svn2.opensuse.org napsal(a): > --- trunk/snapper/agent-snapper/src/SnapperAgent.cc (original) > +++ trunk/snapper/agent-snapper/src/SnapperAgent.cc Mon Oct 10 12:54:07 2011 > @@ -18,7 +18,7 @@ > /* > * search the map for value of given key; both key and value have to be strings > */ > -string SnapperAgent::getValue (const YCPMap map, const string key, string deflt) > +string SnapperAgent::getValue (const YCPMap &map, const string key, const string deflt) Parameters key and deflt should be passed as a reference (missing & there). (If the parameter is 'const' type it's safe to use a reference to it.) > if (!map->value(YCPString(key)).isNull() > && map->value(YCPString(key))->isString()) > -int SnapperAgent::getIntValue (const YCPMap map, const string key, int deflt) > +int SnapperAgent::getIntValue (const YCPMap &map, const string key, const int deflt) The same here and at some more places. (Just const reference to int doesn't make sense here.) > { > if (!map->value(YCPString(key)).isNull() && map->value(YCPString(key))->isInteger()) { > return map->value(YCPString(key))->asInteger()->value(); I think you can avoid recreating YCPString object several times and searching in the map by storing it into an YCPValue variable (it's like any in YCP): YCPValue val = map->value(YCPString(key)); if (!val.isNull() && val->isInteger()) { return val->asInteger()->value(); (well, IIRC this could be also used in pkg-bindings at some places...) -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

1 1

[yast-devel] Re: [yast-commit] r66378 - /trunk/fcoe-client/yast2-fcoe-client.spec.in
by Ladislav Slezak 12 Oct '11

12 Oct '11

Dne 11.10.2011 12:56, gs(a)svn2.opensuse.org napsal(a): > ============================================================================== > --- trunk/fcoe-client/yast2-fcoe-client.spec.in (original) > +++ trunk/fcoe-client/yast2-fcoe-client.spec.in Tue Oct 11 12:56:25 2011 > @@ -1,6 +1,8 @@ > @HEADER-COMMENT@ > > @HEADER@ > +Group: System/YaST > +License: GPL-2.0 I guess the license should be "GPL-2.0+" as the other YaST modules. -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

2 1

[yast-devel] Re: [yast-commit] r66384 - /trunk/ntp-client/src/NtpClient.ycp
by Ladislav Slezak 11 Oct '11

11 Oct '11

Dne 11.10.2011 16:16, varkoly(a)svn2.opensuse.org napsal(a): > Author: varkoly > Date: Tue Oct 11 16:16:01 2011 > New Revision: 66384 > > URL: http://svn.opensuse.org/viewcvs/yast?rev=66384&view=rev > Log: > bnc#719774 - Miscellaneous errors about "cannot write settings" when yast phase 1 is almost done. > -global define map <string, string> GetAllKnownCountries () { > +global define map <string, string> GetAllKnownCountries() { The commit contains too many spacing fixes, please put them in a separate commit next time. I could just skip it (or read it quickly) and focus on the real code changes during code review. -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

1 0

[yast-devel] Re: [yast-commit] r66355 - in /branches/SuSE-Code-11-SP2-Branch/autoinstallation: VERSION package/autoyast2.changes src/modules/AutoinstPartPlan.ycp
by Ladislav Slezak 11 Oct '11

11 Oct '11

Dne 10.10.2011 11:23, ug(a)svn2.opensuse.org napsal(a): > - new_pe["lvm_group"] = pe["used_by"]:""; > + new_pe["lvm_group"] = substring( pe["used_by_device"]:"", 5 ); Huh, why 5? Could you at least put a comment there describing this magic constant? -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

1 0

[yast-devel] Re: [yast-commit] r66369 - /branches/SuSE-Code-11-SP2-Branch/theme/yast2-theme.spec.in
by Ladislav Slezak 11 Oct '11

11 Oct '11

Dne 10.10.2011 18:04, tgoettlicher(a)svn2.opensuse.org napsal(a): > BuildArchitectures: noarch > Summary: - > +License: GPL-2.0 or later > %description We use "GPL-2.0+" string now instead of this. (see commits #66335 and #66230) -- Ladislav Slezák Appliance department / YaST Developer Lihovarská 1060/12 190 00 Prague 9 / Czech Republic tel: +420 284 028 960 lslezak(a)suse.com SUSE -- To unsubscribe, e-mail: yast-devel+unsubscribe(a)opensuse.org To contact the owner, e-mail: yast-devel+owner(a)opensuse.org

1 0