Re: [PATCH] Make git-clean a builtin

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 6, 2007, 05:30 UTC
Message-ID: <7vbqa8j5yr.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <20071106050512.GA6768@mediacenter.austin.rr.com>

Shawn Bohrer <shawn.bohrer@gmail.com> writes:

Show 11 quoted lines

> On Mon, Nov 05, 2007 at 01:14:32PM -0800, Junio C Hamano wrote:
>> Shawn Bohrer <shawn.bohrer@gmail.com> writes:
>> [...]
>> > +static int disabled = 1;
>> 
>> This means we are committed to make clean.requireForce default
>> to true, which is fine by me.  I need to warn the users about
>> this early.
>
> Actually I don't care either way, but in my last rebase on next this
> change was already made to git-clean.sh so I adjusted accordingly.

Oh, that was not a question to you, but a note to me.

Show 12 quoted lines

>> > +static int show_only = 0;
>> > +static int remove_directories = 0;
>> > +static int quiet = 0;
>> > +static int ignored = 0;
>> > +static int ignored_only = 0;
>> 
>> Please do not explicitly initialize static variables to zero.
>
> I realize that static variables will be automatically initialized to
> zero so this is unnecessary, but is there some technical reason not to
> initialize explicitly?  If the answer is simply a style preference that
> is fine, I'm just here to learn.

Both readability and style have to do much with this.

The style has a historical background which is a slight technical merit. It results in a smaller executable file, as C compilers traditionally placed file-scope static variables that are not explicitly initialized in the BSS section, instead of explicitly storing N-bytes of zero as the the initial data in it (although I do not see a reason for compilers not to do the same for variables explicitly initialized to zero. In fact, I think modern gcc produces the same allocation with or without "= 0" initialization).

Show 5 quoted lines

> Of course as already pointed out these don't actually need to be static
> in the first place so I'll simply move them into cmd_clean().  This does
> lead me to another question though.  Now that Dscho has converted my
> patch to use parse-options, what is the best way to update my patch
> while still giving credit to Dscho?

Please send a rewritten replacement version as a single patch that is cleanly applicable to 'next', and mention people whose input helped you in polishing the patch in the proposed commit log message.