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

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2009, 08:31 UTC
Message-ID: <7vskj6vkb9.fsf@alter.siamese.dyndns.org>
In-Reply-To: <a644352c0905140217h382a4d18h988b229c12577de3@mail.gmail.com>

Josh Micich <josh.micich@gmail.com> writes:

Show 13 quoted lines

> 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.

"An operation that some people may want to do but the current toolset is not very good at is _this_, and this patch rectifies that. _Here_ is a number to back up that claim." is a perfect way to justify your new feature. Why not have that in the proposed commit log message ;-)?

Show 11 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.

Ok.