[PATCH 1/5] guess default branch from HEAD

[PATCH 1/5] guess default branch from HEAD

[plenz]

This is a saner alternative than hardcoding the default branch to be
"master". The add_repo() function will now check for a symbolic ref in
repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
with it.

Note that you'll need to strip the newline from the file (-> len-17).

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 cgitrc.5.txt |    3 ++-
 scan-tree.c  |   13 +++++++++++++
 2 files changed, 15 insertions(+), 1 deletions(-)

diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index c3698a6..f0ef6a7 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -362,7 +362,8 @@ repo.commit-filter::
 repo.defbranch::
 	The name of the default branch for this repository. If no such branch
 	exists in the repository, the first branch name (when sorted) is used
-	as default instead. Default value: "master".
+	as default instead. Default value: branch pointed to by HEAD, or
+	"master" if there is no suitable HEAD.
 
 repo.desc::
 	The value to show as repository description. Default value: none.
diff --git a/scan-tree.c b/scan-tree.c
index 627af1b..6c9589d 100644
--- a/scan-tree.c
+++ b/scan-tree.c
@@ -75,6 +75,8 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
 	char *rel, *p, *slash;
 	int n;
 	size_t size;
+	char buffer[256];
+	int fd;
 
 	if (stat(path, &st)) {
 		fprintf(stderr, "Error accessing %s: %s (%d)\n",
@@ -105,6 +107,17 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
 			*p = '\0';
 	repo->name = repo->url;
 	repo->path = xstrdup(path);
+
+	fd = open(fmt("%s/HEAD", repo->path), O_RDONLY);
+	if (fd != -1) {
+		int len;
+		memset(buffer, 0, sizeof(buffer)-1);
+		len = read_in_full(fd, buffer, sizeof(buffer)-1);
+		if(!memcmp(buffer, "ref: refs/heads/", 16))
+			repo->defbranch = xstrndup(buffer+16, len-17);
+		close(fd);
+	}
+
 	while (!owner) {
 		if ((pwd = getpwuid(st.st_uid)) == NULL) {
 			fprintf(stderr, "Error reading owner-info for %s: %s (%d)\n",
-- 
1.7.3.1

[PATCH 2/5] get_commit_date() obtains newest commit date

[plenz]

Previously, CGit used to stat() the branch file under $GIT_DIR/refs/heads,
which is error-prone due to the fact that

 i) the file's modification time is not always the commit time of the
    commit pointed at and
ii) the ref can be packed, in which case the upstream modification time
    would be unavailable

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 ui-repolist.c |   32 ++++++++++++++++++++++++++------
 1 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/ui-repolist.c b/ui-repolist.c
index 2c98668..06fe6c0 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -28,6 +28,30 @@ time_t read_agefile(char *path)
 	return result;
 }
 
+static time_t get_commit_date(const char *repo_path, const char *ref)
+{
+	time_t *t;
+	unsigned char sha[20];
+	const char *retval;
+	struct commit *commit;
+	int flags;
+
+	t = xmalloc(sizeof(time_t));
+
+	set_git_dir(repo_path);
+	setup_git_directory_gently(NULL);
+
+	retval = resolve_ref(ref, sha, 1, &flags);
+	if(!retval)
+		return (time_t)NULL;
+
+	commit = lookup_commit_reference_gently(sha, 1);
+	if(commit)
+		return (time_t) commit->date;
+
+	return (time_t)NULL;
+}
+
 static int get_repo_modtime(const struct cgit_repo *repo, time_t *mtime)
 {
 	char *path;
@@ -45,13 +69,9 @@ static int get_repo_modtime(const struct cgit_repo *repo, time_t *mtime)
 		return 1;
 	}
 
-	path = fmt("%s/refs/heads/%s", repo->path, repo->defbranch);
-	if (stat(path, &s) == 0)
-		*mtime = s.st_mtime;
-	else
-		*mtime = 0;
-
+	*mtime = get_commit_date(repo->path, fmt("refs/heads/%s", repo->defbranch));
 	r->mtime = *mtime;
+
 	return (r->mtime != 0);
 }
 
-- 
1.7.3.1

[PATCH 3/5] make enable-log-linecount independent of -filecount

[plenz]

You should be able to independently switch file and line count on and
off. This patch makes the code work like the documentation suggests: no
dependency for line counts to be displayed only when file counts are.

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 ui-log.c |   29 ++++++++++++++---------------
 1 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/ui-log.c b/ui-log.c
index 8add66a..2e6e9d6 100644
--- a/ui-log.c
+++ b/ui-log.c
@@ -100,11 +100,10 @@ void print_commit(struct commit *commit, struct rev_info *revs)
 	struct strbuf graphbuf = STRBUF_INIT;
 	struct strbuf msgbuf = STRBUF_INIT;
 
-	if (ctx.repo->enable_log_filecount) {
+	if (ctx.repo->enable_log_filecount)
+		cols++;
+	if (ctx.repo->enable_log_linecount)
 		cols++;
-		if (ctx.repo->enable_log_linecount)
-			cols++;
-	}
 
 	if (revs->graph) {
 		/* Advance graph until current commit */
@@ -179,18 +178,18 @@ void print_commit(struct commit *commit, struct rev_info *revs)
 		html_link_close();
 	}
 
-	if (ctx.repo->enable_log_filecount) {
+	if (ctx.repo->enable_log_filecount || ctx.repo->enable_log_linecount) {
 		files = 0;
 		add_lines = 0;
 		rem_lines = 0;
 		cgit_diff_commit(commit, inspect_files, ctx.qry.vpath);
-		html("</td><td>");
-		htmlf("%d", files);
-		if (ctx.repo->enable_log_linecount) {
-			html("</td><td>");
-			htmlf("-%d/+%d", rem_lines, add_lines);
-		}
 	}
+
+	if (ctx.repo->enable_log_filecount)
+		htmlf("</td><td>%d", files);
+	if (ctx.repo->enable_log_linecount)
+		htmlf("</td><td>-%d/+%d", rem_lines, add_lines);
+
 	html("</td></tr>\n");
 
 	if (revs->graph || ctx.qry.showmsg) { /* Print a second table row */
@@ -379,10 +378,10 @@ void cgit_print_log(const char *tip, int ofs, int cnt, char *grep, char *pattern
 	if (ctx.repo->enable_log_filecount) {
 		html("<th class='left'>Files</th>");
 		columns++;
-		if (ctx.repo->enable_log_linecount) {
-			html("<th class='left'>Lines</th>");
-			columns++;
-		}
+	}
+	if (ctx.repo->enable_log_linecount) {
+		html("<th class='left'>Lines</th>");
+		columns++;
 	}
 	html("</tr>\n");
 
-- 
1.7.3.1

[PATCH 3/5] make enable-log-linecount independent of -filecount

[hjemli]

On Thu, Mar 10, 2011 at 17:03, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> You should be able to independently switch file and line count on and
> off. This patch makes the code work like the documentation suggests: no
> dependency for line counts to be displayed only when file counts are.

Thanks.

--
larsh

[PATCH 3/5] make enable-log-linecount independent of -filecount

[hjemli]

On Thu, Mar 10, 2011 at 18:25, Lars Hjemli <hjemli at gmail.com> wrote:
> On Thu, Mar 10, 2011 at 17:03, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
>> You should be able to independently switch file and line count on and
>> off. This patch makes the code work like the documentation suggests: no
>> dependency for line counts to be displayed only when file counts are.
>
> Thanks.

Sorry for the latency. This patch is now applied to master while patch
4 and 5 from the series have been applied to stable.

-- 
larsh

[PATCH 4/5] fix two encoding bugs

[plenz]

reencode() takes three arguments in the order (txt, from, to), opposed to
reencode_string, which will, like iconv, handle the arguments with from
and to swapped. Fix that (this makes reencode more intuitive).
If src and dst encoding are equivalent, don't do any encoding.

If no special encoding parameter is found within the commit, assume
UTF-8 and explicitly convert to PAGE_ENCODING. The change to reencode()
mentioned above avoids re-encoding a UTF-8 string to UTF-8, for example.

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 parsing.c |   24 +++++++++++++++---------
 1 files changed, 15 insertions(+), 9 deletions(-)

diff --git a/parsing.c b/parsing.c
index f37c49d..c9e4350 100644
--- a/parsing.c
+++ b/parsing.c
@@ -106,7 +106,11 @@ const char *reencode(char **txt, const char *src_enc, const char *dst_enc)
 	if (!txt || !*txt || !src_enc || !dst_enc)
 		return *txt;
 
-	tmp = reencode_string(*txt, src_enc, dst_enc);
+	/* no encoding needed if src_enc equals dst_enc */
+	if(!strcasecmp(src_enc, dst_enc))
+		return *txt;
+
+	tmp = reencode_string(*txt, dst_enc, src_enc);
 	if (tmp) {
 		free(*txt);
 		*txt = tmp;
@@ -160,6 +164,10 @@ struct commitinfo *cgit_parse_commit(struct commit *commit)
 		}
 	}
 
+	/* if no special encoding is found, assume UTF-8 */
+	if(!ret->msg_encoding)
+		ret->msg_encoding = xstrdup("UTF-8");
+
 	// skip unknown header fields
 	while (p && *p && (*p != '\n')) {
 		p = strchr(p, '\n');
@@ -189,14 +197,12 @@ struct commitinfo *cgit_parse_commit(struct commit *commit)
 	} else
 		ret->subject = xstrdup(p);
 
-	if (ret->msg_encoding) {
-		reencode(&ret->author, PAGE_ENCODING, ret->msg_encoding);
-		reencode(&ret->author_email, PAGE_ENCODING, ret->msg_encoding);
-		reencode(&ret->committer, PAGE_ENCODING, ret->msg_encoding);
-		reencode(&ret->committer_email, PAGE_ENCODING, ret->msg_encoding);
-		reencode(&ret->subject, PAGE_ENCODING, ret->msg_encoding);
-		reencode(&ret->msg, PAGE_ENCODING, ret->msg_encoding);
-	}
+	reencode(&ret->author, ret->msg_encoding, PAGE_ENCODING);
+	reencode(&ret->author_email, ret->msg_encoding, PAGE_ENCODING);
+	reencode(&ret->committer, ret->msg_encoding, PAGE_ENCODING);
+	reencode(&ret->committer_email, ret->msg_encoding, PAGE_ENCODING);
+	reencode(&ret->subject, ret->msg_encoding, PAGE_ENCODING);
+	reencode(&ret->msg, ret->msg_encoding, PAGE_ENCODING);
 
 	return ret;
 }
-- 
1.7.3.1

[PATCH 4/5] fix two encoding bugs

[hjemli]

On Thu, Mar 10, 2011 at 17:03, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> reencode() takes three arguments in the order (txt, from, to), opposed to
> reencode_string, which will, like iconv, handle the arguments with from
> and to swapped. Fix that (this makes reencode more intuitive).

Oh my. Thanks for fixing this.


> If src and dst encoding are equivalent, don't do any encoding.
>
> If no special encoding parameter is found within the commit, assume
> UTF-8 and explicitly convert to PAGE_ENCODING. The change to reencode()
> mentioned above avoids re-encoding a UTF-8 string to UTF-8, for example.

These two changes also makes sense. Will apply.

--
larsh

[PATCH 5/5] Add advice about scan-path in cgitrc.5.txt

[plenz]

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 cgitrc.5.txt |    5 +++--
 1 files changed, 3 insertions(+), 2 deletions(-)

diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index f0ef6a7..9b44475 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -287,8 +287,9 @@ scan-path::
 	the result will be cached as a cgitrc include-file in the cache
 	directory. If project-list has been defined prior to scan-path,
 	scan-path loads only the directories listed in the file pointed to by
-	project-list. Default value: none. See also: cache-scanrc-ttl,
-	project-list.
+	project-list. Be advised that only the global settings taken
+	before the scan-path directive will be applied to each repository.
+	Default value: none. See also: cache-scanrc-ttl, project-list.
 
 section::
 	The name of the current repository section - all repositories defined
-- 
1.7.3.1

[PATCH 5/5] Add advice about scan-path in cgitrc.5.txt

[hjemli]

On Thu, Mar 10, 2011 at 17:03, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -287,8 +287,9 @@ scan-path::
> ? ? ? ?the result will be cached as a cgitrc include-file in the cache
> ? ? ? ?directory. If project-list has been defined prior to scan-path,
> ? ? ? ?scan-path loads only the directories listed in the file pointed to by
> - ? ? ? project-list. Default value: none. See also: cache-scanrc-ttl,
> - ? ? ? project-list.
> + ? ? ? project-list. Be advised that only the global settings taken
> + ? ? ? before the scan-path directive will be applied to each repository.
> + ? ? ? Default value: none. See also: cache-scanrc-ttl, project-list.
>

Thanks

--
larsh

[PATCH 1/5] guess default branch from HEAD

[hjemli]

On Thu, Mar 10, 2011 at 17:03, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> This is a saner alternative than hardcoding the default branch to be
> "master". The add_repo() function will now check for a symbolic ref in
> repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
> with it.

I agree with the motivation, but...

> --- a/scan-tree.c
> +++ b/scan-tree.c
> @@ -75,6 +75,8 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
> ? ? ? ?char *rel, *p, *slash;
> ? ? ? ?int n;
> ? ? ? ?size_t size;
> + ? ? ? char buffer[256];
> + ? ? ? int fd;
>
> ? ? ? ?if (stat(path, &st)) {
> ? ? ? ? ? ? ? ?fprintf(stderr, "Error accessing %s: %s (%d)\n",
> @@ -105,6 +107,17 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
> ? ? ? ? ? ? ? ? ? ? ? ?*p = '\0';
> ? ? ? ?repo->name = repo->url;
> ? ? ? ?repo->path = xstrdup(path);
> +
> + ? ? ? fd = open(fmt("%s/HEAD", repo->path), O_RDONLY);
> + ? ? ? if (fd != -1) {
> + ? ? ? ? ? ? ? int len;
> + ? ? ? ? ? ? ? memset(buffer, 0, sizeof(buffer)-1);
> + ? ? ? ? ? ? ? len = read_in_full(fd, buffer, sizeof(buffer)-1);
> + ? ? ? ? ? ? ? if(!memcmp(buffer, "ref: refs/heads/", 16))
> + ? ? ? ? ? ? ? ? ? ? ? repo->defbranch = xstrndup(buffer+16, len-17);
> + ? ? ? ? ? ? ? close(fd);
> + ? ? ? }
> +

...since git supports fs links, you'll need to lstat() and then either
readlink() or read_in_full(). And if you readlink(), you'll obviously
need to parse the result differently.

--
larsh

[PATCH 1/5] guess default branch from HEAD

[plenz]

Hi, Lars!

* Lars Hjemli <hjemli at gmail.com> [2011-03-10 18:24]:
> On Thu, Mar 10, 2011 at 17:03, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> > This is a saner alternative than hardcoding the default branch to be
> > "master". The add_repo() function will now check for a symbolic ref in
> > repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
> > with it.
> 
> I agree with the motivation, but...

> > +        fd = open(fmt("%s/HEAD", repo->path), O_RDONLY);

> ...since git supports fs links, you'll need to lstat() and then
> either readlink() or read_in_full(). And if you readlink(), you'll
> obviously need to parse the result differently.

I just tried this out:

    $ cd /repositories/...
    $ mv HEAD head
    $ ln -s head HEAD

The patch handles that just fine (it wouldn't if I were to add the
O_NOFOLLOW flag); however, git complains:

    $ git rev-list -1 HEAD
    fatal: Not a git repository (or any parent up to mount parent )

With hardlinks, neither git complains, nor the patch fails.

Julius

[PATCH 1/5] guess default branch from HEAD

[hjemli]

On Wed, Mar 16, 2011 at 11:53, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> Hi, Lars!
>
> * Lars Hjemli <hjemli at gmail.com> [2011-03-10 18:24]:
>> On Thu, Mar 10, 2011 at 17:03, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
>> > This is a saner alternative than hardcoding the default branch to be
>> > "master". The add_repo() function will now check for a symbolic ref in
>> > repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
>> > with it.
>>
>> I agree with the motivation, but...
>
>> > + ? ? ? ?fd = open(fmt("%s/HEAD", repo->path), O_RDONLY);
>
>> ...since git supports fs links, you'll need to lstat() and then
>> either readlink() or read_in_full(). And if you readlink(), you'll
>> obviously need to parse the result differently.
>
> I just tried this out:
>
> ? ?$ cd /repositories/...
> ? ?$ mv HEAD head
> ? ?$ ln -s head HEAD
>
> The patch handles that just fine (it wouldn't if I were to add the
> O_NOFOLLOW flag);

Yeah, but try this instead:

$ cd $repo/.git
$ mv HEAD HEAD.old
$ ln -s refs/heads/master HEAD

This is what git supports, and the patch needs to readlink() to handle the same.

--
larsh

[PATCH v2] guess default branch from HEAD

[plenz]

This is a saner alternative than hardcoding the default branch to be
"master". The add_repo() function will now check for a symbolic ref in
repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
with it. Note that you'll need to strip the newline from the file (->
len-17).

If HEAD is a symbolic link pointing directly to a branch below
refs/heads/, do a readlink() instead to find the ref name.

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 cgitrc.5.txt |    3 ++-
 scan-tree.c  |   21 +++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletions(-)

diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index c3698a6..f0ef6a7 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -362,7 +362,8 @@ repo.commit-filter::
 repo.defbranch::
 	The name of the default branch for this repository. If no such branch
 	exists in the repository, the first branch name (when sorted) is used
-	as default instead. Default value: "master".
+	as default instead. Default value: branch pointed to by HEAD, or
+	"master" if there is no suitable HEAD.
 
 repo.desc::
 	The value to show as repository description. Default value: none.
diff --git a/scan-tree.c b/scan-tree.c
index 627af1b..bec747f 100644
--- a/scan-tree.c
+++ b/scan-tree.c
@@ -75,6 +75,8 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
 	char *rel, *p, *slash;
 	int n;
 	size_t size;
+	char buffer[256];
+	int fd;
 
 	if (stat(path, &st)) {
 		fprintf(stderr, "Error accessing %s: %s (%d)\n",
@@ -105,6 +107,25 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
 			*p = '\0';
 	repo->name = repo->url;
 	repo->path = xstrdup(path);
+
+	fd = open(fmt("%s/HEAD", repo->path), O_RDONLY);
+	if (fd != -1) {
+		int len;
+		memset(buffer, 0, sizeof(buffer)-1);
+		len = read_in_full(fd, buffer, sizeof(buffer)-1);
+		if(!memcmp(buffer, "ref: refs/heads/", 16)) {
+			repo->defbranch = xstrndup(buffer+16, len-17);
+		} else if(strlen(buffer) == 41) { /* probably contains a SHA1 sum */
+			memset(buffer, 0, sizeof(buffer)-1);
+			readlink(fmt("%s/HEAD", repo->path), buffer, sizeof(buffer)-1);
+			char *ref_start;
+			ref_start = memmem(buffer, sizeof(buffer)-1, "refs/heads/", 11);
+			if(ref_start)
+				repo->defbranch = xstrdup(ref_start+11);
+		}
+		close(fd);
+	}
+
 	while (!owner) {
 		if ((pwd = getpwuid(st.st_uid)) == NULL) {
 			fprintf(stderr, "Error reading owner-info for %s: %s (%d)\n",
-- 
1.7.3.1

[PATCH v2] guess default branch from HEAD

[hjemli]

On Wed, Mar 30, 2011 at 18:00, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> @@ -105,6 +107,25 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
> ? ? ? ? ? ? ? ? ? ? ? ?*p = '\0';
> ? ? ? ?repo->name = repo->url;
> ? ? ? ?repo->path = xstrdup(path);
> +
> + ? ? ? fd = open(fmt("%s/HEAD", repo->path), O_RDONLY);
> + ? ? ? if (fd != -1) {
> + ? ? ? ? ? ? ? int len;
> + ? ? ? ? ? ? ? memset(buffer, 0, sizeof(buffer)-1);
> + ? ? ? ? ? ? ? len = read_in_full(fd, buffer, sizeof(buffer)-1);
> + ? ? ? ? ? ? ? if(!memcmp(buffer, "ref: refs/heads/", 16)) {
> + ? ? ? ? ? ? ? ? ? ? ? repo->defbranch = xstrndup(buffer+16, len-17);
> + ? ? ? ? ? ? ? } else if(strlen(buffer) == 41) { /* probably contains a SHA1 sum */
> + ? ? ? ? ? ? ? ? ? ? ? memset(buffer, 0, sizeof(buffer)-1);
> + ? ? ? ? ? ? ? ? ? ? ? readlink(fmt("%s/HEAD", repo->path), buffer, sizeof(buffer)-1);
> + ? ? ? ? ? ? ? ? ? ? ? char *ref_start;
> + ? ? ? ? ? ? ? ? ? ? ? ref_start = memmem(buffer, sizeof(buffer)-1, "refs/heads/", 11);
> + ? ? ? ? ? ? ? ? ? ? ? if(ref_start)
> + ? ? ? ? ? ? ? ? ? ? ? ? ? ? ? repo->defbranch = xstrdup(ref_start+11);
> + ? ? ? ? ? ? ? }
> + ? ? ? ? ? ? ? close(fd);
> + ? ? ? }
> +

Thanks.

This seems like a workable solution, but with two issues:
* The feature only works for repos added by scan-path
* I prefer to declare variables at the start of the function

Both issues can thus be solved by a simple refactoring.

--
larsh

[PATCH v2] guess default branch from HEAD

[plenz]

Hi, Lars!

* Lars Hjemli <hjemli at gmail.com> [2011-04-07 11:46]:
> This seems like a workable solution, but with two issues:
> * The feature only works for repos added by scan-path
> * I prefer to declare variables at the start of the function

I did some simple refactoring. See attached patch.

There is a function guess_defbranch now, which in any case returns a
char* (which is "master" in fallthrough cases). So you can use it in
any place you like.

Julius

[PATCH] guess default branch from HEAD

[plenz]

This is a saner alternative than hardcoding the default branch to be
"master". The add_repo() function will now check for a symbolic ref in
repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
with it. Note that you'll need to strip the newline from the file (->
len-17).

If HEAD is a symbolic link pointing directly to a branch below
refs/heads/, do a readlink() instead to find the ref name.

Signed-off-by: Julius Plenz <plenz at cis.fu-berlin.de>
---
 cgitrc.5.txt |    3 ++-
 scan-tree.c  |   36 ++++++++++++++++++++++++++++++++++++
 2 files changed, 38 insertions(+), 1 deletions(-)

diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index c3698a6..f0ef6a7 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -362,7 +362,8 @@ repo.commit-filter::
 repo.defbranch::
 	The name of the default branch for this repository. If no such branch
 	exists in the repository, the first branch name (when sorted) is used
-	as default instead. Default value: "master".
+	as default instead. Default value: branch pointed to by HEAD, or
+	"master" if there is no suitable HEAD.
 
 repo.desc::
 	The value to show as repository description. Default value: none.
diff --git a/scan-tree.c b/scan-tree.c
index 627af1b..ae137ee 100644
--- a/scan-tree.c
+++ b/scan-tree.c
@@ -68,6 +68,39 @@ static char *xstrrchr(char *s, char *from, int c)
 	return from < s ? NULL : from;
 }
 
+static char *guess_defbranch(const char *repo_path)
+{
+	int fd, len;
+	char buffer[256];
+	char *ref_start;
+	char *head;
+
+	head = fmt("%s/HEAD", repo_path);
+	fd = open(head, O_RDONLY);
+	if (fd == -1)
+		return xstrdup("master");
+
+	memset(buffer, 0, sizeof(buffer));
+	len = read_in_full(fd, buffer, sizeof(buffer)-1);
+	close(fd);
+
+	if(!memcmp(buffer, "ref: refs/heads/", 16))
+		return xstrndup(buffer+16, len-17);
+
+	if(strlen(buffer) == 41) {
+		/* probably contains a SHA1 sum */
+		memset(buffer, 0, sizeof(buffer));
+		if(readlink(head, buffer, sizeof(buffer)-1)) {
+			ref_start = memmem(buffer, sizeof(buffer)-1, "refs/heads/", 11);
+			if(ref_start)
+				return xstrdup(ref_start+11);
+		}
+	}
+
+	return xstrdup("master");
+}
+
+
 static void add_repo(const char *base, const char *path, repo_config_fn fn)
 {
 	struct stat st;
@@ -105,6 +138,9 @@ static void add_repo(const char *base, const char *path, repo_config_fn fn)
 			*p = '\0';
 	repo->name = repo->url;
 	repo->path = xstrdup(path);
+
+	repo->defbranch = guess_defbranch(repo->path);
+
 	while (!owner) {
 		if ((pwd = getpwuid(st.st_uid)) == NULL) {
 			fprintf(stderr, "Error reading owner-info for %s: %s (%d)\n",
-- 
1.7.3.1

[PATCH] guess default branch from HEAD

[jugg]

Julius Plenz <plenz at ...> writes:
> 
> This is a saner alternative than hardcoding the default branch to be
> "master". The add_repo() function will now check for a symbolic ref in
> repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
> with it. Note that you'll need to strip the newline from the file (->
> len-17).
> 
> If HEAD is a symbolic link pointing directly to a branch below
> refs/heads/, do a readlink() instead to find the ref name.
> 
> Signed-off-by: Julius Plenz <plenz at ...>

I've successfully applied this patch against the current stable branch (commit: 
2a8f5531) and deployed the resulting binary without trouble.  The functionality 
appears to work as advertised.  So, along with setting the default 'readme'
value as:

readme=HEAD:README.md

each project is able to have a sensible default presentation based off of the 
project's default branch.  Very useful as we use gitolite to manage our 
repositories, this saves us further manual configuration outside of gitolite.

I hope this patch can be integrated into the official release.

chris

[PATCH] guess default branch from HEAD

[larsh]

On Fri, Jun 10, 2011 at 06:42:51AM +0000, chris wrote:
> Julius Plenz <plenz at ...> writes:
> > 
> > This is a saner alternative than hardcoding the default branch to be
> > "master". The add_repo() function will now check for a symbolic ref in
> > repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
> > with it. Note that you'll need to strip the newline from the file (->
> > len-17).
> > 
> > If HEAD is a symbolic link pointing directly to a branch below
> > refs/heads/, do a readlink() instead to find the ref name.
> > 
> > Signed-off-by: Julius Plenz <plenz at ...>
> 
> I've successfully applied this patch against the current stable branch (commit: 
> 2a8f5531) and deployed the resulting binary without trouble.  The functionality 
> appears to work as advertised.  So, along with setting the default 'readme'
> value as:
> 
> readme=HEAD:README.md
> 
> each project is able to have a sensible default presentation based off of the 
> project's default branch.  Very useful as we use gitolite to manage our 
> repositories, this saves us further manual configuration outside of gitolite.
> 

Thanks for testing.

> I hope this patch can be integrated into the official release.

The last issue with this patch is that it invokes guess_defbranch() once
per repository during scan-path processing, but never for repositories
added manually to cgitrc. I think it shouldn't be invoked at all
during scan-path but instead just once when a repo page has been
selected, i.e. in prepare_repo_cmd(). I can add a fixup patch on top
of the patch from Julius if there's no objections to this plan.

--
larsh

[PATCH] guess default branch from HEAD

[hjemli]

[Julius should have been CC'ed on this]

---------- Forwarded message ----------
From:  <larsh at hjemli.net>
Date: Mon, Jun 13, 2011 at 12:16
Subject: Re: [PATCH] guess default branch from HEAD
To: chris <jugg at hotmail.com>
Cc: cgit at hjemli.net


On Fri, Jun 10, 2011 at 06:42:51AM +0000, chris wrote:
> Julius Plenz <plenz at ...> writes:
> >
> > This is a saner alternative than hardcoding the default branch to be
> > "master". The add_repo() function will now check for a symbolic ref in
> > repo_path/HEAD. If there is a suitable one, overwrite repo->defbranch
> > with it. Note that you'll need to strip the newline from the file (->
> > len-17).
> >
> > If HEAD is a symbolic link pointing directly to a branch below
> > refs/heads/, do a readlink() instead to find the ref name.
> >
> > Signed-off-by: Julius Plenz <plenz at ...>
>
> I've successfully applied this patch against the current stable branch (commit:
> 2a8f5531) and deployed the resulting binary without trouble. ?The functionality
> appears to work as advertised. ?So, along with setting the default 'readme'
> value as:
>
> readme=HEAD:README.md
>
> each project is able to have a sensible default presentation based off of the
> project's default branch. ?Very useful as we use gitolite to manage our
> repositories, this saves us further manual configuration outside of gitolite.
>

Thanks for testing.

> I hope this patch can be integrated into the official release.

The last issue with this patch is that it invokes guess_defbranch() once
per repository during scan-path processing, but never for repositories
added manually to cgitrc. I think it shouldn't be invoked at all
during scan-path but instead just once when a repo page has been
selected, i.e. in prepare_repo_cmd(). I can add a fixup patch on top
of the patch from Julius if there's no objections to this plan.

--
larsh

[PATCH] guess default branch from HEAD

[plenz]

Hi, Lars!

* Lars Hjemli <hjemli at gmail.com> [2011-06-13 12:18]:
> [Julius should have been CC'ed on this]

I'm on the list and happy that someone has a use case for this patch,
too. :-)

> > I hope this patch can be integrated into the official release.
> 
> The last issue with this patch is that it invokes guess_defbranch() once
> per repository during scan-path processing, but never for repositories
> added manually to cgitrc. I think it shouldn't be invoked at all
> during scan-path but instead just once when a repo page has been
> selected, i.e. in prepare_repo_cmd().

You mentioned that before, I was not sure where to place it, though.

> I can add a fixup patch on top of the patch from Julius if there's
> no objections to this plan.

To the contrary, I'd appreciate your patch. Thanks. :-)

Julius

[PATCH] guess default branch from HEAD

[hjemli]

On Thu, Jun 16, 2011 at 16:59, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
>> I can add a fixup patch on top of the patch from Julius if there's
>> no objections to this plan.
>
> To the contrary, I'd appreciate your patch. Thanks. :-)

Your patch and my fixup is now part of the wip branch on hjemli.net/git/cgit.

-- 
larsh

[PATCH] guess default branch from HEAD

[jugg]

Lars Hjemli <hjemli at ...> writes:
> On Thu, Jun 16, 2011 at 16:59, Julius Plenz <plenz at ...> wrote:
> >> I can add a fixup patch on top of the patch from Julius if there's
> >> no objections to this plan.
> >
> > To the contrary, I'd appreciate your patch. Thanks. 
> 
> Your patch and my fixup is now part of the wip branch on hjemli.net/git/cgit.

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.

This patch does not have this problem:

http://hjemli.net/git/cgit/patch/?id=d711de55280c3c9c10cfda4e24c8f3b3015217b2

regards,

chris

[PATCH] guess default branch from HEAD

[plenz]

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.

Do you get any error messages in your webserver's logs?

> This patch does not have this problem:
> 
> http://hjemli.net/git/cgit/patch/?id=d711de55280c3c9c10cfda4e24c8f3b3015217b2

In principle, the patch d711de5 (by Lars) does the same as 795118d
(mine). Consider the following program:

    #include <stdio.h>
    #include <git/refs.h>

    int main(int argc, char *argv[]) {
        const char *ref;
        unsigned char sha1[20];

        ref = resolve_ref("HEAD", sha1, 0, NULL);
        printf("%s\n", ref+11);

        return 0;
    }

If you compile that thing, it will correctly resolve the HEAD ref.

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);
 }
 

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. The key point is in the original
patch at http://hjemli.net/git/cgit/commit/?id=d711de55280c3c9c10cfda4e24c8f3b3015217b2

    head = fmt("%s/HEAD", repo_path);

This will look at repo_path/HEAD. resolve_ref cannot do this, because
it expects to find an already set up gitdir.

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.

Cheers,
Julius

[PATCH] guess default branch from HEAD

[larsh]

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

[PATCH] guess default branch from HEAD

[jugg]

Julius Plenz <plenz at ...> writes:
> * chris <jugg at ...> [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.
> 
> Do you get any error messages in your webserver's logs?

No.  But, using Lars' additional patch seems to works around the issue.

chris

[PATCH] guess default branch from HEAD

[larsh]

On Sat, Jul 09, 2011 at 08:09:57AM +0000, chris wrote:
> Lars Hjemli <hjemli at ...> writes:
> > On Thu, Jun 16, 2011 at 16:59, Julius Plenz <plenz at ...> wrote:
> > >> I can add a fixup patch on top of the patch from Julius if there's
> > >> no objections to this plan.
> > >
> > > To the contrary, I'd appreciate your patch. Thanks. 
> > 
> > Your patch and my fixup is now part of the wip branch on hjemli.net/git/cgit.
> 
> 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.
> 

Thanks for noticing. I think the following patch will fix the issue:

diff --git a/ui-repolist.c b/ui-repolist.c
index 25c36ce..f21d28d 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -45,7 +45,8 @@ static int get_repo_modtime(const struct cgit_repo *repo, time_t *mtime)
 		return 1;
 	}
 
-	path = fmt("%s/refs/heads/%s", repo->path, repo->defbranch);
+	path = fmt("%s/refs/heads/%s", repo->path, repo->defbranch ?
+		   repo->defbranch : "master");
 	if (stat(path, &s) == 0) {
 		*mtime = s.st_mtime;
 		r->mtime = *mtime;

--
larsh

[PATCH] guess default branch from HEAD

[jugg]

> <larsh at ...> writes:
> On Sat, Jul 09, 2011 at 08:09:57AM +0000, chris wrote:
> > the index page 'idle' times no longer show up (the idle column is blank). 
> 
> Thanks for noticing. I think the following patch will fix the issue:
> 
> diff --git a/ui-repolist.c b/ui-repolist.c
> index 25c36ce..f21d28d 100644
> --- a/ui-repolist.c
> +++ b/ui-repolist.c
> @@ -45,7 +45,8 @@ static int get_repo_modtime(const struct cgit_repo *repo, 
time_t *mtime)
>  		return 1;
>  	}
> 
> -	path = fmt("%s/refs/heads/%s", repo->path, repo->defbranch);
> +	path = fmt("%s/refs/heads/%s", repo->path, repo->defbranch ?
> +		   repo->defbranch : "master");
>  	if (stat(path, &s) == 0) {
>  		*mtime = s.st_mtime;
>  		r->mtime = *mtime;
> 
> --
> larsh

This patch corrected the problem of the idle times being empty on the index 
page.

It is a little strange to have the idle time not use the default branch.
Should guess_defbranch() be called here instead of hard-coding "master"?

chris

[PATCH] guess default branch from HEAD

[larsh]

On Tue, Jul 19, 2011 at 09:35:42AM +0000, chris wrote:
> > <larsh at ...> writes:
> > On Sat, Jul 09, 2011 at 08:09:57AM +0000, chris wrote:
> > > the index page 'idle' times no longer show up (the idle column is blank). 
> > 
> > Thanks for noticing. I think the following patch will fix the issue:
> > 
> > diff --git a/ui-repolist.c b/ui-repolist.c
> > index 25c36ce..f21d28d 100644
> > --- a/ui-repolist.c
> > +++ b/ui-repolist.c
> > @@ -45,7 +45,8 @@ static int get_repo_modtime(const struct cgit_repo *repo, 
> time_t *mtime)
> >  		return 1;
> >  	}
> > 
> > -	path = fmt("%s/refs/heads/%s", repo->path, repo->defbranch);
> > +	path = fmt("%s/refs/heads/%s", repo->path, repo->defbranch ?
> > +		   repo->defbranch : "master");
> >  	if (stat(path, &s) == 0) {
> >  		*mtime = s.st_mtime;
> >  		r->mtime = *mtime;
> > 
> > --
> > larsh
> 
> This patch corrected the problem of the idle times being empty on the index 
> page.

Thanks for testing.

>
> It is a little strange to have the idle time not use the default branch.

Well, "master" is the defacto default branch...


> Should guess_defbranch() be called here instead of hard-coding "master"?

I don't think so, due to performance concerns (as explained earlier in this
thread). It's a compromise, favoring performance over correctness.

--
larsh

[PATCH] guess default branch from HEAD

[jugg]

 <larsh at ...> writes:
> On Tue, Jul 19, 2011 at 09:35:42AM +0000, chris wrote:
> > Should guess_defbranch() be called here instead of hard-coding "master"?
> 
> I don't think so, due to performance concerns (as explained earlier in this
> thread). It's a compromise, favoring performance over correctness.

Well, it sounds like premature optimization to me.  I wonder how much 
performance impact that would really have, and if cgit's caching support doesn't 
negate any potential issue on busy servers anyway.

I would expect correctness in user experience first.  If correctness is shown to 
impact performance, I would expect solutions/compromises in those specific cases 
as needed.

Of course I'm free to patch it to my liking obviously.  This is just some user 
feedback for you.

Thanks!

chris

[PATCH] guess default branch from HEAD

[larsh]

On Tue, Jul 19, 2011 at 10:13:54AM +0000, chris wrote:
>  <larsh at ...> writes:
> > On Tue, Jul 19, 2011 at 09:35:42AM +0000, chris wrote:
> > > Should guess_defbranch() be called here instead of hard-coding "master"?
> > 
> > I don't think so, due to performance concerns (as explained earlier in this
> > thread). It's a compromise, favoring performance over correctness.
> 
> Well, it sounds like premature optimization to me.

That may be true.


> I wonder how much 
> performance impact that would really have, and if cgit's caching support doesn't 
> negate any potential issue on busy servers anyway.

I'll try to do some benchmarking with different
setups and post some numbers.

--
larsh

[PATCH v2] guess default branch from HEAD

[hjemli]

On Thu, Apr 7, 2011 at 12:49, Julius Plenz <plenz at cis.fu-berlin.de> wrote:
> Hi, Lars!
>
> * Lars Hjemli <hjemli at gmail.com> [2011-04-07 11:46]:
>> This seems like a workable solution, but with two issues:
>> * The feature only works for repos added by scan-path
>> * I prefer to declare variables at the start of the function
>
> I did some simple refactoring. See attached patch.
>
> There is a function guess_defbranch now, which in any case returns a
> char* (which is "master" in fallthrough cases). So you can use it in
> any place you like.

Great, thanks. I actually think it only needs to be invoked once, in
prepare_repo_cmd():

@@ -421,6 +421,7 @@ static int prepare_repo_cmd(struct cgit_context *ctx)
        }
        ctx->page.title = fmt("%s - %s", ctx->repo->name, ctx->repo->desc);

+       ctx->repo->defbranch = guess_defbranch(ctx->repo->path);
        if (!ctx->qry.head) {
                ctx->qry.nohead = 1;
                ctx->qry.head = find_default_branch(ctx->repo);

-- 
larsh

[PATCH v2] guess default branch from HEAD

[jugg]

Will this patch be integrated at some point?  Is there anything holding it up?

Thanks,

chris