Re: [PATCH 2/3] added --batch option to mktree

From: Josh Micich <josh.micich@gmail.com>
Date: May 14, 2009, 09:24 UTC
Message-ID: <a644352c0905140224g41f645c1l734bee791656ea1d@mail.gmail.com>
In-Reply-To: <7vhbzoxl5k.fsf@alter.siamese.dyndns.org>

On Wed, May 13, 2009 at 11:18 PM, Junio C Hamano <gitster@pobox.com> wrote:

>> This option enables creation of many tree objects with a single process
>
>... which is desirable in what way?  how does this justify the cost of
>maintenance?  what is it used for?

I have been writing a few of my own utilities on top of basic git commands and got mild performance gains from using the existing '--batch' option with "git cat-file" instead of executing new "git cat-file -p <sha>" processes for each object I wish to read. Later I was writing a utility for mirroring another SCM (creates many trees), and I faced a similar performance issue with "git mktree". After adding the '--batch' option, I experienced about a 30% speed increase.

If you find this logic persuasive, I can fix all the other issues and re-submit this patch.

>> +--batch::
>I think you want an blank line before this.

right

>> @@ -125,20 +125,42 @@ int cmd_mktree(int ac, const char **av, const char
>> *prefix)
>
>Linewrapped and would not apply.

sorry - gmane web interface did that. BTW - does git have a coding standard for maximum line length?

>What do you mean by "interactively"?  Does anybody type from the standard
>input?

I guess I was trying to stress the point that the created tree object ids are output immediately as the complete text for each tree is received. Other (bad) applications I have seen wait for EOF on stdin before sending anything to stdout, and this is exactly what I had avoided in the impl. Perhaps you could suggest better phrasing.

Show 5 quoted lines

>Decl-after-statement.
>No C++ comment please;
>Lose excess {} pair;
>Style: have an explicit ";" for an empty statement.
>used = 0; /* .... */

sorry - bad habits. gotta work out how to get my compiler to warn me about these

Show 7 quoted lines

>> +             if (is_batch_mode && got_eof && used < 1) {
>> +                     // allow input to finish with a new-line (or not)
>
>Style: have an explicit ";" for an empty statement.
>
>But more importantly, what does this comment mean?  Why do you want to be
>loose in input format validation?

I agree with your implied suggestion that tight input validation is better. I was actually trying to keep consistent with the way I believe mktree works today. The final \n is optional as far as I can tell.