Hello,
On Apr 18, 2011, at 23:22 , Peter Marschall wrote:
> On Monday, 18. April 2011, Martin Paljak <mar...@martinpaljak.net> wrote:
>> On Sun, Apr 17, 2011 at 12:44, Peter Marschall <pe...@adpm.de> wrote:
>>> please find attached 3 patches to opensc-tool and opensc-explorer:
>>>
>>> * [PATCH 1/3] opensc-tool: make list_algorithms() table driven
>> * The patch has a lot of extra whitespaces at end of lines.
> Hmm, I do not completely understand.
> I usually check the spaces at EOL before commit.
> And I admit I overlooked one (the one in the 159th line of the patch file
> ;-).
> In my opinion: 1 != many.
> All other ones have been in before.
I have to apologize, git commit complained and I checked the resulting file
(not the offending patch) for extra whitespace, which appeared to be many
around the changed area. I amended the patch and removed whitespaces, had a
look at the resulting patch, discovered that it only dealt with
--list-algorithms related things and made hasty conclusions. Please ignore!
> Regarding this request I am at a loss.
> Can you help me?
> * Do you want me to have all tables the same width overall ?
No.
> * Do you want me to align the options[] table ?
> I did not do it as it had nothing to do with my patches
> and I wanted them to as small as possible.
> * Do you want me to be space police on these tools
> and remove all trailing spaces where they occur?
That would be too much to ask, patches that affect functionality are much more
important :)
I view with non-standard tab width, thus space-aligned tables (if aligned at
all) are the best.
>> If you use git,
>> enable the sampel "pre-commit" hook in .git/hooks to detect such
>> whitespace errors.
> Thanks that's a good hint.
> Unfortunately the pre-commit.sample in my installation seems to be different
> from yours as it does a lot more than simply calling git diff, which is what
> I
> do before commits anyway to spot trailing blanks errors (+ testing, of course
> ;-).
I have not exactly checked what it does in detail, I just know that "enabling
it will warn you of common whitespace issues, like extra space at end of
lines". Yes, it requires fixing those errors in the file that are not created
by you either with your commit or with a preceding "whitespace normalization
patch".
>> * Why the commented out "none" entries for hashes and paddings and
>> special handling in the loop for none? As a general rule: please don't
>> do c++ comments and no commented out code in new commits
> sorry my fault (I have the habit to list _all_ ID entries in a table ;-)
>
> Please find attached the updated first patch.
> Changes to the previous version:
> * all ( = 1 ;-) new trailing whitespace removed
> * commented table contents removed.
> Sorry for doing it again via mail this time.
Ignored, that's the ONLY way of sharing patches for review :)
>>> * [PATCH 3/3] opensc-{explorer,tool}: allow sending extended APDUs
>>> In do_apdu() resp. send_apdu/(, flexibilize parsing the APDU string
>>> passed so that extended APDUs are accepted a valid APDUs too.
>>
>> OK. Also related is #237, at least to the extent that is described in
>> commend 12 [1]. As you already notices, that's copied code. Pushing it
>> to a single location and re-using would be a better choice.
> I wanted to start small ;-)
> First get the code updated/fixed, then think about getting it into some
> common
> library (either a new one or an existing one).
OK. I'll amend the patch with a note similar to "TODO: move to apdu.c".
> I guess the library should be deferred for later.
> It may need discussions where the code shall go: maybe even directly into
> libopensc ;-)
The same way there is apdu2bytes in apdu.c, bytes2apdu should be added, IMO
Thanks,
Martin
--
@MartinPaljak.net
+3725156495
_______________________________________________
opensc-devel mailing list
opensc-devel@lists.opensc-project.org
http://www.opensc-project.org/mailman/listinfo/opensc-devel
