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