--- On Tue, 6/3/08, Jakub Narebski <jnareb@gmail.com> wrote:
quoted text
> 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! :-)

quoted text
> 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".

quoted text
> > 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.)

quoted text
> 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).

quoted text
> 
> > 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html