View Issue Details
|ID||Project||Category||View Status||Date Submitted||Last Update|
|0002102||Far Manager||Command Line/Execute||public||2012-05-17 01:58||2012-05-21 22:38|
|Target Version||Fixed in Version||3.0|
|Summary||0002102: Path environment variable not processed correctly isf it contains empty strings between semicolons|
|Description||If I have an empty component in the path environment variable, Far fails to parse that path completely. In particular, a wrong file may be executed as a result.|
|Steps To Reproduce||Have your Path environment variable contain an empty component (in other words, two semicolons in a row, for example, "C:\dir1;;C:\dir2"). Have a build.cmd in C:\dir1 and a build.exe in C:\dir2, and try to execute "build" in Far's command line. According to the cmd.exe's logic, "C:\dir1\build.cmd" should be executed. Instead, Far executes "build.exe".|
|Additional Information||I have a fix for this that I would like to contribute but am not sure about the contribution process.|
Essentially, I add a "SkipEmptyLines" bool field to UserDefinedList and have it skip any empty string item in the list when parsing it. Then I set that field every time the "Path" environment variable's value is being parsed.
Can somebody point me to the instructions on how to contribute fixes to Far Manager?
|Tags||No tags attached.|
Please explore existing code more carefully, before adding new - UserDefinedList already has a flag to skip empty items (though it's name is not so obvious) so all you need is a replace strings like "UserDefinedList PathList" with "UserDefinedList PathList(0, 0, ULF_ACCOUNTEMPTYLINE);" in all places where %path% is processed.
Maybe it's even better to change a default behaviour and always silently skip empty items.
As for contribution process - when you done editing, the easiest way is to invoke "svn diff > patch.diff" in your local copy and attach patch.diff to appropriate bugrtacker issue. See HACKING-EN file for details.
Actually, I saw the "AccountEmptyItems" flag and even thought of using it originally, but it was not clear to me that its intention is to silently skip empty items: it still increments the item's index which I thought has some non-obvious consequences. Now that you mentioned it, it appears the index is not used for anything but sorting by it so skipping an index value does not affect any behavior.
Another thing not entirely clear to me about this flag is this additional condition: (AccountEmptyLine && CurList==List && !item.index).
Looks like this will not allow an empty item to be the first one in the list. Is this intended? For the behavior to match cmd.exe, this condition should be removed. Is it OK to remove it for all UserDefinedList instances that use AccountEmptyLine?
You also suggest making this "skip empty lines" default. Then do you know which of the UserDefinedList instances should specifically not use it?
> Is this intended?
no, it's just an old & buggy code.
see build 2683.
patch.diff (5,701 bytes)
patch.diff (5,701 bytes)
Thanks for fixing this. I looked at your code and found a potentially incorrect behavior still. The current implementation of the ULF_UNIQUE (without ULF_SORT) flag produces a "non-deterministic" order, despite its apparent intention to sort the "packed" array by the original indexes. I have specifically tried setting my path to "2,1,2,1" and executing some command line. What I expected to see as a result of PathList.Set(strPathEnv) was an array containing strings "2" followed by "1"; instead, I got the reverse order. The problem is that qsort does not guarantee the order of the items that compare as equal. One "hack" I can imagine to address this is change the implementation of TArray<>::Pack to assign the index of the previous item to the index of the item being deleted if the latter one is smaller.
I made this change with no doubts caused by my non-familiarity with the code, so attaching this fix here.
|2012-05-17 01:58||ddv||New Issue|
|2012-05-17 02:54||DrKnS||Note Added: 0009218|
|2012-05-17 03:59||ddv||Note Added: 0009219|
|2012-05-17 21:29||DrKnS||Note Added: 0009224|
|2012-05-17 21:29||DrKnS||Assigned To||=> DrKnS|
|2012-05-17 21:29||DrKnS||Status||new => feedback|
|2012-05-18 04:27||ddv||File Added: patch.diff|
|2012-05-18 04:28||ddv||Note Added: 0009225|
|2012-05-18 04:28||ddv||Status||feedback => assigned|
|2012-05-18 10:54||DrKnS||Note Added: 0009227|
|2012-05-18 10:54||DrKnS||Status||assigned => feedback|
|2012-05-21 21:35||ddv||Note Added: 0009250|
|2012-05-21 21:35||ddv||Status||feedback => assigned|
|2012-05-21 21:35||ddv||Status||assigned => resolved|
|2012-05-21 22:38||DrKnS||Build||=> 2686|
|2012-05-21 22:38||DrKnS||Status||resolved => closed|
|2012-05-21 22:38||DrKnS||Resolution||open => fixed|
|2012-05-21 22:38||DrKnS||Fixed in Version||=> 3.0|