Differential D5777

Fix splitting of root installation command, add default value for the same in .kcfg
ClosedPublic
Actions

Authored by geetamc on May 8 2017, 8:07 PM.

Details

Reviewers
apol
Commits
R32:55de681019bf: Fix splitting of root installation command, add default value for the same in .
Summary

-KShell::splitArgs is used instead of string.split( )
Reason:
When suCommand is empty, the flow of control does not reach body of "if(suCommandWithArg.isEmpty() )"

If QString is empty and it is split using .split() method, the returned QStringList is non empty,
that is not the case with KShell::splitArgs.

We could use if( suCommand.isEmpty() ) instead but since we ultimately have to return suCommandWithArg,
we must have a check for emptiness of suCommandWithArg, and using KShell::splitArgs eliminates the need for having
to check if( suCommand.isEmpty() ).

-Added a default value for suCommand in makebuilderconfig.kcfg so that when new projects are
created, the entry suCommand isn't empty.

Test Plan

-Without the patch when suCommand entry is empty, "" is used as root installation command when project is installed as root (hence installation fails)
-With the patch, kdesu -t is used.

Diff Detail

Repository
R32 KDevelop
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.
geetamc created this revision.May 8 2017, 8:07 PM
Restricted Application added a subscriber: kdevelop-devel. · View Herald TranscriptMay 8 2017, 8:07 PM
apol requested changes to this revision.May 8 2017, 8:16 PM
apol added a subscriber: apol.
apol added inline comments.
projectbuilders/makebuilder/makejob.cpp
218

Use KShell::splitArgs

This revision now requires changes to proceed.May 8 2017, 8:16 PM
geetamc added a comment.May 8 2017, 8:26 PM

@apol Thanks, I will surely.
But I will update the revision right now without the change as I accidentally created the diff against an older base.

geetamc updated this revision to Diff 14313.May 8 2017, 8:29 PM
geetamc edited edge metadata.
geetamc updated this revision to Diff 14317.May 8 2017, 9:11 PM
geetamc edited the summary of this revision. (Show Details)

used KShell:splitArgs to split su command.

geetamc retitled this revision from Fix check for empty su command for installing as root to Fix splitting of root installation command, add default value for the same in .kcfg.May 8 2017, 9:40 PM
apol added a comment.May 16 2017, 9:17 AM

I don't really understand the patch, there's the bit there so that if (cmd.isEmpty) kdesu -t. What are you trying to fix?

FWIW, probably the correct fix here would be to use the kdesu framework itself...

geetamc marked an inline comment as done.EditedMay 16 2017, 2:09 PM
In D5777#109999, @apol wrote:

I don't really understand the patch, there's the bit there so that if (cmd.isEmpty) kdesu -t. What are you trying to fix?

True, the problem is that if suCommand is empty and we define suCommandWithArg = suCommand.split(regEx), (where regExp = QRegExp(" ") )
suCommandWithArg.isEmpty() should be true as suCommand itself was empty in the first place, unfortunately that is not the case and hence
the flow of control never reaches the body of "if(suCommandWithArg.isEmpty() )".

So in my first diff I checked suCommand instead for emptiness, and assigned appropriate value to suCommandWithArg, however
when you suggested KShell::splitArgs, I found that if the original QString is empty, the QStringList returned by KShell::splitArgs is empty,
hence a simple change from QString.split(regExp) to KShell::splitArgs(QString) does the job.

FWIW, probably the correct fix here would be to use the kdesu framework itself...

I am sorry, but I am rather ignorant of what kdesu framework offers and how it would be a more proper fix, so I will read about it and if I feel
that it can be used in this scenario, I will update my diff.

geetamc edited the summary of this revision. (Show Details)May 16 2017, 9:04 PM
geetamc edited the test plan for this revision. (Show Details)
kfunk added a subscriber: kfunk.EditedJul 4 2017, 4:36 PM

This looks fine to me now.

@apol What do you think?

apol accepted this revision.Jul 4 2017, 10:16 PM

Thanks!

This revision is now accepted and ready to land.Jul 4 2017, 10:16 PM
Closed by commit R32:55de681019bf: Fix splitting of root installation command, add default value for the same in . (authored by kfunk). · Explain WhyJul 4 2017, 11:01 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 16189

View Options

projectbuilders/makebuilder/makebuilderconfig.kcfg

Loading...
View Options

projectbuilders/makebuilder/makejob.cpp

Loading...