[Freeipa-devel] [PATCH] 0003 batch command can be used to trigger internal errors on server

[Stanislav Laznicka]

On 06/08/2016 02:09 PM, Petr Vobornik wrote:
> On 06/08/2016 10:07 AM, Petr Spacek wrote:
>> On 7.6.2016 15:11, Stanislav Laznicka wrote:
>>> Hello,
>>>
>>> Thank you for your patch. As the thin-client patches were pushed in the
>>> meantime, the patch won't apply. Could you please send a rebased version?
>>>
>>> Also, I have a few comments to the patch:
>>>
>>> 1) I think that the commit message should be rather a brief conclusion to the
>>> changes made in the commit. This could help for faster orientation in the
>>> changes that were made to a certain part of code should you be searching for a
>>> bug introduced by a commit. Should some more info be required, it can be added
>>> to the ticket. Could you therefore shorten the commit message?
>> (My personal opinion, no golden standard.)
>>
>> Honestly I disagree with Standa. Yes, the commit message seems to be a bit
>> long but *tickets* are not the best place to put *technical* information into.
>>
>> Tickets are planning tool but keep in mind that Trac may/will vanish one day
>> and all we will have will be (Git?) repo.
> +1
>
> The commit message is very good and honestly I'd like to see more of
> such commit messages.
>
>> I would recommend putting the comment about expected input format into code
>> comments somewhere around batch command definition.
>>
>> This would reduce commit message (roughly, the text needs to be adapted) to
>> part starting 'The code did not check the format of ' ... which is perfectly
>> reasonable description of the change.
> Batch command deserves a doc string, so that it would be visible in API
> browser but that is not subject of this ticket. Btw, expected format is
> already in the code.
It is, although it may be not be as clear it is what you're looking for.

As for the commit message, I would like see the description of batch 
commands (which I think is really good) in the doc string rather than in 
the commit message to get the information about the change I need. 
However, that would require proper documentation (such as that in the 
commit message) in the code (where it belongs). As it is not there, or 
it is not that clear, I guess that the commit message is the right spot 
to have it and I was wrong here.
>> IMHO.
>> Petr^2 Spacek
>>
>>
>>> 2) Please do not add the tickets to comments in the code. You can use git
>>> blame -L or git log -L to see in which commits were the changes introduced to
>>> a certain part of a file, these commits should include the ticket number if
>>> more info is needed.
> +1
>
>>> Standa
>>>
>>>
>>> On 05/27/2016 03:53 PM, Florence Blanc-Renaud wrote:
>>>> Hi all,
>>>>
>>>> the following patch checks the format of parameters passed to a method
>>>> called through the batch command. I picked the ConversionError for invalid
>>>> parameters format but this choice can be discussed if you have better
>>>> suggestions...
>>>>
>>>> Fixes: https://fedorahosted.org/freeipa/ticket/5810
>>>> -- 
>>>> Florence Blanc-Renaud
>>>> Identity Management Team, Red Hat
>>>>
>>>>
>>>
>>>
>>>
>>
>