Re: [PATCH] Make gc a builtin.

From: Junio C Hamano <junkio@cox.net>
Date: Mar 14, 2007, 07:19 UTC
Message-ID: <7vodmwfg2c.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20070314060727.GC20978@spearce.org>

"Shawn O. Pearce" <spearce@spearce.org> writes:

> James Bowes <jbowes@dangerouslyinc.com> wrote:
>> Signed-off-by: James Bowes <jbowes@dangerouslyinc.com>
>
> ACK.  Very nicely done.

Perhaps. But we lost another sample script which made an entry barrier higher to a new person.

Show 10 quoted lines

>> +	if (run_command_v_opt(argv_rerere, RUN_GIT_CMD))
>> +		return error(FAILED_RUN, argv_rerere[0]);
>
> And isn't the above so much more readable than this mess?
>
>> -test "true" != "$pack_refs" ||
>> -git-pack-refs --prune &&
>> -git-reflog expire --all &&
>> -git-repack -a -d -l &&
>> ...

I do not necessarily think so. This is not even a performance critical part of the system, so if there _were_ no other constraints, I would rather keep scripts like this as scripts.

For things like this, scripts are much easier to read, understand and futz with, and command lists chained with && in shell scripts are very nice and compact way to express what is going on.

This is especially true if you have some specialized needs, if you do not expect you need to keep that change forever, and if you are lazy. For example, if you have a repository that you for some reason need to keep available to older dumb transport clients for now, you would disable "git-pack-refs --prune" line from your copy of the script version. No need to recompile.

Another example is git-repack script. When you have a specialized repacking needs (say, repack from a specific revision to make a .keep pack to avoid future excessive repacking), being able to check how the plumbing is used in git-repack script and run customized version of it is very handy. Once you rewrite it to sequence of

	if (run_command_v_opt(blech, RUN_GIT_CMD))
        	...

it becomes much harder to learn what the shell command equivalent that would suit your needs would be, and we would lose another command that would serve as a good example.

We are doing built-in _only_ because people on some platforms cannot sanely use POSIX shell scripts. I do not reject these "make X built-in" patches (when X is perfectly fine as a shell script) because I sympathize with people stuck on Windows, not because I think built-in is easier to read nor work with than scripts. There is a downside.