[PATCH] guess default branch from HEAD

[larsh at hjemli.net (larsh at hjemli.net)]

On Wed, Jul 13, 2011 at 05:10:37PM +0200, Julius Plenz wrote:
> Hi!
> 
> * chris <jugg at hotmail.com> [2011-07-09 10:12]:
> > I've applied the following patch:
> > 
> > http://hjemli.net/git/cgit/patch/?id=795118d4042d53566d8375dcabe20515e9135351
> > 
> > ontop of:
> > 
> > http://hjemli.net/git/cgit/commit/?id=2a8f553163d642e60092ced20631e1020581273b
> > 
> > and the index page 'idle' times no longer show up (the idle column
> > is blank). Each project's summary page 'Age' column still works just
> > fine.
> 
[snip]
> 
> When Lars proposed the patch, I didn't actually try it out. Now,
> reading over it again, I see two ovious quirks: The first is trivial
> to fix: return "master" will return a stack address, but you have to
> allocate memory for that:
> 
> diff --git a/cgit.c b/cgit.c
> index d51885b..f9be05c 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -427,7 +427,7 @@ static char *guess_defbranch(const char *repo_path)
>  
>         ref = resolve_ref("HEAD", sha1, 0, NULL);
>         if (!ref || prefixcmp(ref, "refs/heads/"))
> -               return "master";
> +               return xstrdup("master");
>         return xstrdup(ref + 11);
>  }
>  

No, string constants have static storage so this isn't needed.

> 
> Now, the second thing is a more severe thing. guess_defbranch will get
> the repo_path as an argument; however, it's not used. The reason why
> this works *once* (in my little program above) is that you explictly
> set GIT_DIR on the shell, or you are in a git repository already.
> 
> However, when iterating over multiple repositories, you change your
> gitdir. When writing the patch I discovered resolve_ref at some point,
> BUT it cannot be used in this case.

This is true if resolve_ref() is invoked during repository discovery,
but my patch only invokes resolve_ref() once (when a repository has
been selected as "active"). This works for all pages _except_ the index
page where repo.defbranch now has no default value, and a simple (albeit
imperfect) fix for this is just to check "master" if nothing else is
specified (see my response to Chris).

I think this is an ok compromise between performance and correctness;
the user can either rely on a 90% solution to get automatic "idle" info
on the index page or use the "agefile" option (and a git hook) to get
the correct "idle" time, cgit can rely on resolve_ref() to find the ref
pointed to by HEAD, and we avoid resolving the HEAD of every repo on
every page request.

> So, Lars, I propose to revert your patch and use my original one
> instead. I agree it would be nice to use as much functionality from
> libgit.a -- in this case, however, I can't see a way to do it properly.

If we wanted to run resolve_ref() on multiple repositories, we could
always fork() and pipe the result from resolve_ref() back to the
parent process. But I don't think we want to resolve the HEAD of every
repository.

--
larsh