Review Request 121586: Restore upload functionality to git.reviewboard.kde.org

[Aleix Pol Gonzalez]

> On Dec. 19, 2014, 12:32 a.m., Aleix Pol Gonzalez wrote:
> > plugins/git/gitplugin.h, line 153
> > <https://git.reviewboard.kde.org/r/121586/diff/3/?file=333848#file333848line153>
> >
> >     remove the const and the default value.
> >     
> >     Also why adding a setter and a getter for something is never changed? Just make it always --no-prefix, that's fine.
> 
> René J.V. Bertin wrote:
>     Is there anything wrong with the const and default value here?
>     
>     As to your question: I think it's a good idea to support both kind of patches. KDE's git reviewboard now requires p1 patches, but other reviewboard sites might not.
>     I think it'd be beyond the scope of this RR to introduce a UI to select the kind of patch to generate or even make it a feature in the .reviewboardrc settings.
>     But adding the getter and setter should make it easier to find the place where to start if and when it turns out that such a control is required. I know how long it can take simply finding the classes to change (jumping between a debugger, a kdevelop session and a kdevelop test session) ...

The const doesn't add anything. The default value is weird because then you can do setUsePrefix() and reading that takes guess-work to understand what it does, so it's better to just write what you wanted.

I wouldn't support both kinds, --no-prefix was added just as a workaround because komparediff2 didn't support without. Also if we eventually decided we want to add UI to make it configurable, then we can add the property to configure.


On Dec. 19, 2014, 12:32 a.m., René J.V. Bertin wrote:
> > I'd also prefer to have the adoption of hte new property in reviewboard, so we know we are actually passing the correct data.
> 
> René J.V. Bertin wrote:
>     What do you mean? The depth property I introduced to VcsDiff, or the prefix property of the git plugin? Both are presented here, so I must be misunderstanding you?

My apologies, I misunderstood the patch, I forgot there was such a virtual IPatchSource::depth.


- Aleix


-----------------------------------------------------------
This is an automatically generated e-mail. To reply, visit:
https://git.reviewboard.kde.org/r/121586/#review72279
-----------------------------------------------------------


On Dec. 18, 2014, 9:59 p.m., René J.V. Bertin wrote:
> 
> -----------------------------------------------------------
> This is an automatically generated e-mail. To reply, visit:
> https://git.reviewboard.kde.org/r/121586/
> -----------------------------------------------------------
> 
> (Updated Dec. 18, 2014, 9:59 p.m.)
> 
> 
> Review request for KDE Software on Mac OS X and KDevelop.
> 
> 
> Repository: kdevplatform
> 
> 
> Description
> -------
> 
> A change was made on git.reviewboard.kde.org on December 13th a bit before 22h Europe/Paris time, ending support for p0 patch files. This broke the upload functionality in the current/stable version KDevelop's patchreview plugin.
> 
> The patch under review is a workaround (`*`) that restores this functionality by invoking the git helper process which generates the patchfiles with the correct arguments.
> 
> `*`) The real bug is in the reviewboard software, and should be addressed at that level. I work with a software distribution system that requires p0 patches, for which I created patches with KDevelop, something which will now require an additional step.
> 
> 
> Diffs
> -----
> 
>   plugins/reviewboard/reviewboardjobs.cpp 007a6e0 
>   plugins/reviewboard/reviewboardplugin.cpp 1dd3bcb 
>   vcs/vcsdiff.h 573ec1b 
>   vcs/vcsdiff.cpp 01eb73c 
>   vcs/widgets/vcsdiffpatchsources.h 6a2f934 
>   vcs/widgets/vcsdiffpatchsources.cpp c3e2dae 
>   plugins/git/gitplugin.h e930423 
>   plugins/git/gitplugin.cpp f38dc71 
>   plugins/patchreview/patchreview.cpp 18b63db 
> 
> Diff: https://git.reviewboard.kde.org/r/121586/diff/
> 
> 
> Testing
> -------
> 
> Under OS X 10.9; this RR has been created from a patched copy of KDevelop 4.7 .
> 
> 
> Thanks,
> 
> René J.V. Bertin
> 
>

-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://mail.kde.org/pipermail/kdevelop-devel/attachments/20141219/97cdc945/attachment.html>