[KDE/Mac] Review Request 121586: Restore upload functionality to git.reviewboard.kde.org

[René J.V. Bertin]

> On Dec. 18, 2014, 2:09 a.m., Aleix Pol Gonzalez wrote:
> > This breaks the integration with libkomparediff2, doesn't it?
> 
> René J.V. Bertin wrote:
>     Apart from the hunk count issue I mentioned above, I haven't seen anything that's off - at least not when compared with kdevelop kf5. *Maybe* that changes are no longer highlighted in the review plugin, now that you mention it (no time to go check right now).
>     
>     What exactly is libkomparediff2 used for, if not for creating the patchfile itself?
> 
> Aleix Pol Gonzalez wrote:
>     No, as you can see in the patch you are submitting, the patch is generated by the vcs plugin.
>     
>     komparediff2 is used to analyze the generated patch and provide the highlighting.
> 
> René J.V. Bertin wrote:
>     And the hunk count as well?
>     
>     Is it also komparediff2 that extracts the file paths from the patch? I've tried to follow execution flow, but with the inheritance going on it's very hard to figure out where it goes if not to the base classes...
> 
> Aleix Pol Gonzalez wrote:
>     Yes, hunk count as well. KDevelop itself doesn't do any of the patch analysis.
> 
> René J.V. Bertin wrote:
>     Ok, then that explains why there isn't a hunk count nor highlighting in KDevelop KF5 either ...
>     
>     This may be naive, but
>     
>     ```C++
>             m_kompareInfo->localSource = m_patch->baseDir().toLocalFile();
>             m_kompareInfo->depth = m_patch->depth();
>     ```
>      
>     Shouldn't these be updated to something like
>     
>     ```C++
>             m_kompareInfo->localSource = m_patch->baseDir().upURL().toLocalFile();
>             m_kompareInfo->depth = m_patch->depth() + 1;
>     ```
>     ?
> 
> René J.V. Bertin wrote:
>     Answer: indeed that restores the hunk count and highlighting.
>     
>     The question now becomes: is this the right place to make that modification, or if not, of what class is m_patch an instance when doing a git patch review? I *think* I deduced it was a VCSDiffPatchSource, but how to be sure?
> 
> Aleix Pol Gonzalez wrote:
>     In the end, we want komparediff2 to support p1 patches.
>     As a big workaround, you can add the prefix when uploading to reviewboard, if it's a p0 patch, but I think this is very ugly.

Yes, that's not the approach I'd like to take. As shown above, komparediff2 doesn't need to be changed, what needs to be changed is the parameters it receives. It needs to know the depth at which to compare (m_kompareInfo->depth=1 instead of 0), and it needs to receive the parent dir of m_patch->baseDir().

I'm currently fighting with the depth parameter, which requires adding a depth instance to VcsDiff, setting that to 1 in gitplugin.cpp and then ensuring that value arrives in vcsdiffpatchsources.cpp .


- René J.V.


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


On Dec. 17, 2014, 11:25 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. 17, 2014, 11:25 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/git/gitplugin.cpp f38dc71 
> 
> 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/kde-mac/attachments/20141218/0702d935/attachment-0001.html>