<html>
<body>
<div style="font-family: Verdana, Arial, Helvetica, Sans-Serif;">
<table bgcolor="#f9f3c9" width="100%" cellpadding="8" style="border: 1px #c9c399 solid;">
<tr>
<td>
This is an automatically generated e-mail. To reply, visit:
<a href="http://git.reviewboard.kde.org/r/104337/">http://git.reviewboard.kde.org/r/104337/</a>
</td>
</tr>
</table>
<br />
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<p style="margin-top: 0;">On March 30th, 2012, 1:14 p.m., <b>Dario Freddi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">April is upon us. If no objections arise in the time being, these changes will be merged on Sunday, after which I'll ask Kevin to move KAuth back to tier2/ for prime time.</pre>
</blockquote>
<p>On March 30th, 2012, 2:30 p.m., <b>Stephen Kelly</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Thanks for making this happen! :)
Instead of merging, please rebase your work like this:
git fetch origin
git checkout kauth-unit-tests
git rebase origin/frameworks
git checkout frameworks
git rebase kauth-unit-tests
gitk # Make sure it still looks ok.
git push
This will instead replay your changes on top of the tip of the frameworks branch instead of creating a merge, which is not needed.</pre>
</blockquote>
<p>On March 30th, 2012, 2:32 p.m., <b>Dario Freddi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I always do this - nice to know that others love rebased branches :D</pre>
</blockquote>
<p>On March 30th, 2012, 2:34 p.m., <b>Stephen Kelly</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Great. Good to know.
You did use the word 'merge' though, which triggered my alarm. :)
Thanks,</pre>
</blockquote>
<p>On March 30th, 2012, 9:19 p.m., <b>Kevin Ottens</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">So that you know, it's perfectly fine to me if you do the move yourself, it has my approval already once you rebased on master.
Now if you prefer me to do it myself I can, it's just no a requirement.</pre>
</blockquote>
<p>On March 31st, 2012, 2:38 p.m., <b>Dario Freddi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I'd ask you to please complete the move yourself, given that I am not still completely familiar with what's involved (which maybe is just git mv, but just in case)</pre>
</blockquote>
<p>On March 31st, 2012, 3:18 p.m., <b>Stephen Kelly</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">I saw the branch was merged in. Did something go wrong with the rebase idea?</pre>
</blockquote>
<p>On March 31st, 2012, 3:32 p.m., <b>Dario Freddi</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">The branch was rebased in fact. But was subsequently merged with --no-ff to have a way to track all the changes with the merge commit (which is nothing but a simple "at this point, this branch was integrated"). So actually, all of the commits are in fact rebased on top of (previous) framework</pre>
</blockquote>
<p>On March 31st, 2012, 3:47 p.m., <b>Stephen Kelly</b> wrote:</p>
<blockquote style="margin-left: 1em; border-left: 2px solid #d0d0d0; padding-left: 10px;">
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">It was rebased onto some relatively recent commit, but not the tip of the branch.
I don't understand why you bothered rebasing if you were just going to merge after doing so anyway (--no-ff). I don't see the value in --no-ff at all. It's just as clear that the commits are related because they come from the same person for one thing, and because you prefix the first line with the topic for another. --no-ff doesn't get you anything useful if you already have that.
I just don't understand the obsession with merges in repos, but as far as I can see I'm on my own there...</pre>
</blockquote>
</blockquote>
<pre style="white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Starting from the point that this discussion is mostly about one's own taste (and again, that's why we NEED a clear policy), my reason for doing this is that:
- Given that the branch was rebased on top of the last frameworks' commit (as you can see from the log), the history is linear since all of the commits are sitting one on top of the other.
- Still, you keep the advantage of being able to track such commits to be in a previous branch (like gitk could tell you)
- Last but not least, looking just at the merge commit shows you the full diff the branch introduced - in such a case, for example, is incredibly useful to track all of the API changes at once. Yes, you could have used git diff <merge commit> <first commit before the rebased branch> and it would have had the same effect, but the approach is more clean
So IMHO this manages to keep it sane both for merge enthusiasts, without losing the clear advantages rebasing gives you. But again, there is no right or wrong here, just different views. For sure, in the future, we have to settle on one of these (or another).</pre>
<br />
<p>- Dario</p>
<br />
<p>On March 18th, 2012, 10:25 p.m., Dario Freddi wrote:</p>
<table bgcolor="#fefadf" width="100%" cellspacing="0" cellpadding="8" style="background-image: url('http://git.reviewboard.kde.org/media/rb/images/review_request_box_top_bg.png'); background-position: left top; background-repeat: repeat-x; border: 1px black solid;">
<tr>
<td>
<div>Review request for kdelibs, Kevin Ottens, David Faure, and Alexander Neundorf.</div>
<div>By Dario Freddi.</div>
<p style="color: grey;"><i>Updated March 18, 2012, 10:25 p.m.</i></p>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Description </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">Preamble - sorry for having to name-call people but apparently we still don't have a frameworks way for reviewing code (which sucks). And sorry for the long summary, but it's worth reading. However.
This huge patchsets brings KAuth in the marvelous world of Frameworks. If you dislike ReviewBoard's way of displaying diffs or simply want to see a commit list, please refer to the URL in "Branch".
First of all, I pulled in a dependency on KJob after a chat with Kevin. This makes KAuth tier2, but shouldn't be a big issue.
Then there's the hard part: source compatibility is reasonably broken here. The changes I had to do were mostly for the sake of revamping the internal workflow of the library. The main problem KAuth had was the fact it was completely synchronous, leading to a multitude of problems. After these changes it's fully asynchronous instead (reason for pulling in KJob), the API was simplified, and some unused features like multiple action execution have been removed.
The main changes at a glance:
* Some renaming to the enums
* Moving Action & ActionReply to be implicitly shared
* Removing ActionWatcher (now useless due to the new semantics of execute
* Removing some useless APIs from Action, namely executeActions, execute(helper)
* execute() now returns a KJob
* helperID() -> helperId()
* Static action replies are now static accessors returning a new instance. This was a complete mistake in the first place, but it's still there with a different semantic to ease porting. The main use case for changing this is a failure to handle implicitly shared classes in multithreaded environments with that approach.
Of course, while it would be awesome to have all the code reviewed, I understand it's a very big change so I'd like at least some feedback on the following points:
* General sanity of the new API
* Consistency of the enums. StatusInvalid vs. ExecuteMode vs. AuthorizationDeniedError. While the semantic seems correct to me, I'd like to have some feedback on whether consistency is valuable in the ordering of <type><value> vs. <value><type> and which one should be preferred in case.
* Whether to deprecate static accessors such as static const ActionReply SuccessReply(). I strongly favor this.
* Whether the new dependency of kcoreaddons for the sake of using KJob is reasonable or I should go for a different alternative.
* CMake sanity for the new dependency of kcoreaddons.
The code is pretty much unit-tested and it should have a decent coverage, even if I had no way to check this. For unit tests, I had to create a fake authorization backend for testing purposes, whereas I managed to reuse the dbus backend for helper communication, so that I could even test that. For running the helper and the client in the same process, in the unit test I am resorting to making the dbus service of the helper live in a separate thread, to prevent asynchronous DBus calls from failing due to QDBus' local-loop optimization. The test is also run on the session bus.</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Testing </h1>
<table width="100%" bgcolor="#ffffff" cellspacing="0" cellpadding="10" style="border: 1px solid #b8b5a0">
<tr>
<td>
<pre style="margin: 0; padding: 0; white-space: pre-wrap; white-space: -moz-pre-wrap; white-space: -pre-wrap; white-space: -o-pre-wrap; word-wrap: break-word;">New unit tests pass 100%</pre>
</td>
</tr>
</table>
<h1 style="color: #575012; font-size: 10pt; margin-top: 1.5em;">Diffs</b> </h1>
<ul style="margin-left: 3em; padding-left: 0;">
<li>staging/kauth/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/BackendsManager.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/BackendsManager.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/HelperTest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/SetupActionTest.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestBackend.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestHelper.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/autotests/TestHelper.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/AuthBackend.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/CMakeLists.txt <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/HelperProxy.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/dbus/DBusHelperProxy.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/dbus/DBusHelperProxy.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/dbus/org.kde.auth.xml <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/fake/FakeBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/fakehelper/FakeHelperProxy.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/fakehelper/FakeHelperProxy.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/mac/AuthServicesBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/policykit/PolicyKitBackend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/backends/polkit-1/Polkit1Backend.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthaction.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthaction.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionreply.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionreply.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionwatcher.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthactionwatcher.cpp <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthexecutejob.h <span style="color: grey">(PRE-CREATION)</span></li>
<li>staging/kauth/src/kauthexecutejob.cpp <span style="color: grey">(PRE-CREATION)</span></li>
</ul>
<p><a href="http://git.reviewboard.kde.org/r/104337/diff/" style="margin-left: 3em;">View Diff</a></p>
</td>
</tr>
</table>
</div>
</body>
</html>