Re: [PATCH] [RFC] Introduce Git.pm

From: Junio C Hamano <junkio@cox.net>
Date: Jun 22, 2006, 01:03 UTC
Message-ID: <7v64iuxard.fsf@assigned-by-dhcp.cox.net>
In-Reply-To: <20060622003546.17760.23089.stgit@machine.or.cz>

Petr Baudis <pasky@suse.cz> writes:

> Currently Git.pm just wraps up exec()s of Git commands, but even that
> is not trivial to get right and various Git perl scripts do it in
> various inconsistent ways. And things will get much more interesting
> when we get to XS-izing libgit.

We would probably need some moderate to major surgery before doing that depending on which parts. Worrisome are object layer that holds onto parsed objects and keeps flag bits from the previous runs, and lockfile that are cleaned up via atexit() -- there may be others.

Show 8 quoted lines

> @@ -618,6 +622,7 @@ GIT-CFLAGS: .FORCE-GIT-CFLAGS
>  	@FLAGS='$(TRACK_CFLAGS)'; \
>  	    if test x"$$FLAGS" != x"`cat GIT-CFLAGS 2>/dev/null`" ; then \
>  		echo 1>&2 "    * new build flags or prefix"; \
> +		echo 1>&2 "    * $$FLAGS != `cat GIT-CFLAGS 2>/dev/null`"; \
>  		echo "$$FLAGS" >GIT-CFLAGS; \
>              fi
>

Offtopic; do people mind if we drop $GIT_VERSION from TRACK_CFLAGS? Every time I switch branches or make a new commit it ends up rebuilding everything needlessly.

Show 6 quoted lines

>  sub current_branch {
> -	my ($bra) = qx{git-symbolic-ref HEAD};
> +	my ($bra) = $repo->generic_oneline('symbolic-ref', 'HEAD');
>  	chomp($bra);
>  	$bra =~ s|^refs/heads/||;
>  	if ($bra ne 'master') {

Your *_oneline chomps so chomp($bra) is not needed anymore, I think.

In any case, this is an excellent move to clean things up. Various implementations of qx{} will be all in one place so ActiveState folks would have easier time adjusting things to their environment.

> +require Exporter;

Not "use"?

> +# Methods which can be called as standalone functions as well:
> +@EXPORT_OK = qw(generic generic_oneval generic_vocal);

I am not sure generic_xxx is a good name.  Perhaps command_xxx?

Also as you say in the comment, "vocal" sounds a bit funny. I wonder if we can use wantarray to detect the case where the caller wants _no_ value, and do the "not piping" optimization in that case.