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

Dear diary, on Thu, Jun 22, 2006 at 03:03:02AM CEST, I got a letter
where Junio C Hamano [off-list ref] said that...

Petr Baudis [off-list ref] writes:

quoted

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.

Yes, I'm aware of that. Still, some parts should be quite easy...

Dear diary, on Thu, Jun 22, 2006 at 04:31:05AM CEST, I got a letter
where Eric Wong [off-list ref] said that...

XS-ising libgit would be awesome with all the git-hash-object calls in
git-svn (and other importers, too, I imagine).

...so I couldn't resist that and did add hash_object() xs-ized
function. Looks pretty sweet:

$ time perl -w -I/home/xpasky/lib/perl5/site_perl/5.8.8 -MGit -le 'print Git::generic_oneline("hash-object", "Makefile") for (1..1000)' >/dev/null

real    0m4.800s
user    0m2.072s
sys     0m2.560s

$ time perl -w -I/home/xpasky/lib/perl5/site_perl/5.8.8 -MGit -le 'print Git::hash_object("Makefile") for (1..1000)' >/dev/null

real    0m0.179s
user    0m0.144s
sys     0m0.032s


Dear diary, on Thu, Jun 22, 2006 at 03:03:02AM CEST, I got a letter
where Junio C Hamano [off-list ref] said that...

quoted

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

Please, please, _do_! :-)

I'm sorry about this hunk, I added it to just figure out why the heck it
keeps rebuilding everything from scratch and forgot to drop it.

quoted

 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.

Ah, thanks.

quoted

+require Exporter;

Not "use"?

The idiom is to require, I guess; we don't need to import anything.
I don't think it matters and I'm not entirely sure about all the
implications of the difference between the two, so if you wish,
I can change it.

quoted

+# 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?

Technically, other methods can be commands, too. ;-) But yet, it sounds
better. I've changed it.

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.

I think that would be rather confusing since generic_vocal does not make
the output go away but makes it go to stdout, while if you just throw
away return value of generic(), you expect it to go nowhere.

At least I will change 'vocal' to 'noisy' so that it does not sound
so weird.


Dear diary, on Thu, Jun 22, 2006 at 04:31:05AM CEST, I got a letter
where Eric Wong [off-list ref] said that...

Petr Baudis [off-list ref] wrote:

quoted

+	my $pid = open(my $fh, "-|");
+	if ($pid < 0) {
+		die "generic($cmd, @args) open: $!";

In Perl, fork returns undef instead of $pid < 0 on failure.

Thanks!

Doing join(' ',@args) will also make the error message more readable, too :)

@args should get automagically interpolated by having the members
separated by $" (space by default).

-- 
				Petr "Pasky" Baudis
Stuff: http://pasky.or.cz/
A person is just about as big as the things that make them angry.