Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame

From: Luben Tuikov <ltuikov@yahoo.com>
Date: Jun 3, 2008, 20:18 UTC
Message-ID: <839911.60903.qm@web31810.mail.mud.yahoo.com>
In-Reply-To: <m34p8a2173.fsf@localhost.localdomain>

--- On Tue, 6/3/08, Jakub Narebski <jnareb@gmail.com> wrote:

Show 6 quoted lines

> From: Jakub Narebski <jnareb@gmail.com>
> Subject: Re: [PATCH] Avoid errors from git-rev-parse in gitweb blame
> To: "Rafael Garcia-Suarez" <rgarciasuarez@gmail.com>
> Cc: git@vger.kernel.org, "Luben Tuikov" <ltuikov@yahoo.com>
> Date: Tuesday, June 3, 2008, 4:43 AM
> Cc-ed Luben Tuikov, author of this part.

Thanks guys! :-)

Show 7 quoted lines

> Rafael Garcia-Suarez <rgarciasuarez@gmail.com>
> writes:
> 
> > git-rev-parse will abort with an error when passed a
> non-existent
> > revision spec, such as "deadbeef^" where
> deadbeef has no parent.

Yes, I've known about this ever since I coded this. The reasoning was that the value of parsing up the "tree" of parent changes was a lot more (valuable) than the value of detecting that "deadbeef^" had no parent -- which would've been logically apparent to the coder/reviewer/user of "blame2".

Show 5 quoted lines

> > Using the --revs-only parameter makes this error go
> away, while
> > retaining functionality, keeping the web server error
> log nice
> > and clean.

Ok, that's fine, as long as indeed the functionality is preserved. I leave it up to Jakub and Junio to make sure that indeed the functionality is preserved. (I.e. saving us yet another patch from anyone of us.)

Show 7 quoted lines

> Thanks.  This error wasn't detected earlier probably
> because
> 'blame' view is rarely enabled; and repo.or.cz
> gitweb which has
> 'blame' enabled IIRC use gitweb which is modified
> there, allowing
> incremental blame using AJAX.

I'm a heavy user of "blame2", often exploring the course of "evolution" of the code and/or code lines and segments. I value this in gitweb (being easier/more visual to use as opposed to command line).

Show 27 quoted lines

> 
> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 55fb100..f3b4b24 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -4226,9 +4226,9 @@ git_blame2
> >  			              esc_html($rev));
> >  			print "</td>\n";
> >  		}
> > -		open (my $dd, "-|", git_cmd(),
> "rev-parse", "$full_rev^")
> > +		open (my $dd, "-|", git_cmd(),
> "rev-parse", '--revs-only',
> "$full_rev^")
> >  			or die_error(undef, "Open git-rev-parse
> failed");
> > -		my $parent_commit = <$dd>;
> > +		my $parent_commit = <$dd> || '';
> >  		close $dd;
> >  		chomp($parent_commit);
> >  		my $blamed = href(action => 'blame',
> 
> I'd rather remove this, correct it, or make it optional
> (this is very
> fork-heavy).
> 
> But this patch is good as it is now...

Yes, I agree it is good. Just you guys make sure that it doesn't change the value of the functionality of the code.

Thanks everyone!
   Luben