Re: [PATCH] Enable "git rerere" by the config variable rerere.enabled

From: Johannes Schindelin <johannes.schindelin@gmx.de>
Date: Jul 7, 2007, 12:41 UTC
Message-ID: <Pine.LNX.4.64.0707071333520.4093@racer.site>
In-Reply-To: <7vejjkdaqe.fsf@assigned-by-dhcp.cox.net>

Hi,

On Fri, 6 Jul 2007, Junio C Hamano wrote:

Show 5 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > 	And yeah, the git-gui part should be factored out, I guess. Shawn?
> 
> I'll exclude git-gui part and commit with a minor tweaks;

Thank you. I planned to redo the patch when consensus is reached that this is actually a good patch. But as usual, you're faster.

Show 5 quoted lines

> > -int cmd_rerere(int argc, const char **argv, const char *prefix)
> > +int is_rerere_enabled(void)
> >  {
> 
> This will be "static".

But of course!

Show 9 quoted lines

> > +	const char *rr_cache = git_path("rr-cache");
> > +	int rr_cache_exists;
> >  
> > -	if (stat(git_path("rr-cache"), &st) || !S_ISDIR(st.st_mode))
> > +	if (!rerere_enabled)
> >  		return 0;
> 
> As git_path() is not zero-cost, assignment to rr_cache will be
> moved here.

Yes, fully agree. It probably does not matter much right now, as git-rerere is mostly called from scripts, and thus does a fork() && exec() anyway, but I am smilingly awaiting the first full-fledged builtin implementation of "git commit", in which case this very function should move somewhere else, and there it does matter a bit more.

Show 14 quoted lines

> > +	rr_cache_exists = !stat(rr_cache, &st) && S_ISDIR(st.st_mode);
> > +	if (rerere_enabled < 0)
> > +		return rr_cache_exists;
> > +
> > +	if (!rr_cache_exists && (mkdir(rr_cache, 0777) ||
> > +			adjust_shared_perm(rr_cache)))
> > +		die("Could not create directory %s", rr_cache);
> > +	return 1;
> > +}
> 
> If rr-cache is a regular file, we will hit "Could not create
> directory" which is exactly what we want anyway.  Even if it is
> a dangling symlink, it would fail with "File exists", so that
> should be Ok.

That was the intended error path, yes. It does not safeguard against old behaviour, where you could ": > .git/rr-cache", and be wondering why it is not activating rr-cache ;-)

Ciao, Dscho