[PATCH 1/1] RFC: git: update to v2.19.0-rc0

[john at keeping.me.uk (John Keeping)]

On Tue, Aug 21, 2018 at 09:21:14AM +0200, Christian Hesse wrote:
> From: Christian Hesse <mail at eworm.de>
> 
> Changelog to be writting... :)

More comments below, but it looks like there's a lot of unrelated
cleanups here.  I think only the the_repository parameter addition is
required for Git 2.19.

The other changes mostly look good, but I think they can be split out
into a separate series (which can probably land before Git 2.19 final is
released).

> Signed-off-by: Christian Hesse <mail at eworm.de>
> ---
>  Makefile      |  4 ++--
>  cgit.h        |  1 +
>  git           |  2 +-
>  parsing.c     |  7 +++----
>  shared.c      |  2 +-
>  ui-blame.c    |  2 +-
>  ui-blob.c     |  6 +++---
>  ui-clone.c    |  4 ++--
>  ui-commit.c   |  4 ++--
>  ui-diff.c     |  4 ++--
>  ui-log.c      |  4 ++--
>  ui-patch.c    | 11 +++++++----
>  ui-plain.c    |  2 +-
>  ui-shared.c   |  6 +++---
>  ui-snapshot.c |  4 ++--
>  ui-ssdiff.c   |  9 +++++----
>  ui-tag.c      |  4 ++--
>  ui-tree.c     |  4 ++--
>  18 files changed, 42 insertions(+), 38 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index 05ea71f..d67c8c6 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -14,8 +14,8 @@ htmldir = $(docdir)
>  pdfdir = $(docdir)
>  mandir = $(prefix)/share/man
>  SHA1_HEADER = <openssl/sha.h>
> -GIT_VER = 2.18.0
> -GIT_URL = https://www.kernel.org/pub/software/scm/git/git-$(GIT_VER).tar.gz
> +GIT_VER = 2.19.0.rc0
> +GIT_URL = https://www.kernel.org/pub/software/scm/git/testing/git-$(GIT_VER).tar.gz
>  INSTALL = install
>  COPYTREE = cp -r
>  MAN5_TXT = $(wildcard *.5.txt)
> diff --git a/cgit.h b/cgit.h
> index 32dfd7a..bcc4fce 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -8,6 +8,7 @@
>  #include <cache.h>
>  #include <grep.h>
>  #include <object.h>
> +#include <object-store.h>
>  #include <tree.h>
>  #include <commit.h>
>  #include <tag.h>
> diff --git a/git b/git
> index 53f9a3e..7e8bfb0 160000
> --- a/git
> +++ b/git
> @@ -1 +1 @@
> -Subproject commit 53f9a3e157dbbc901a02ac2c73346d375e24978c
> +Subproject commit 7e8bfb0412581daf8f3c89909f1d37844e8610dd
> diff --git a/parsing.c b/parsing.c
> index 12453c2..7b3980e 100644
> --- a/parsing.c
> +++ b/parsing.c
> @@ -63,8 +63,7 @@ static char *substr(const char *head, const char *tail)
>  	if (tail < head)
>  		return xstrdup("");
>  	buf = xmalloc(tail - head + 1);
> -	strncpy(buf, head, tail - head);
> -	buf[tail - head] = '\0';
> +	strlcpy(buf, head, tail - head + 1);

Nice cleanup, but I don't think it is required for Git 2.19!

It's probably worth splitting this out for a separate patch that can
land before Git 2.19.

>  	return buf;
>  }
>  
> @@ -78,7 +77,7 @@ static void parse_user(const char *t, char **name, char **email, unsigned long *
>  
>  		email_len = ident.mail_end - ident.mail_begin;
>  		*email = xmalloc(strlen("<") + email_len + strlen(">") + 1);
> -		sprintf(*email, "<%.*s>", email_len, ident.mail_begin);
> +		xsnprintf(*email, email_len + 3, "<%.*s>", email_len, ident.mail_begin);

Again, not related to Git 2.19.  I'm not sure about this one, snprintf
isn't adding anything because we know exactly how big the buffer is.
However, I bet static analysis warns about sprintf here even though this
use is safe.

Maybe we should just use a strbuf?

>  
>  		if (ident.date_begin)
>  			*date = strtoul(ident.date_begin, NULL, 10);

[snip the_repository conversion that all looks good]

> diff --git a/ui-log.c b/ui-log.c
> index d696e20..3bcb657 100644
> --- a/ui-log.c
> +++ b/ui-log.c
> @@ -67,7 +67,7 @@ void show_commit_decorations(struct commit *commit)
>  	while (deco) {
>  		struct object_id peeled;
>  		int is_annotated = 0;
> -		strncpy(buf, prettify_refname(deco->name), sizeof(buf) - 1);
> +		strlcpy(buf, prettify_refname(deco->name), sizeof(buf));

More cleanup to split out.

>  		switch(deco->type) {
>  		case DECORATION_NONE:
>  			/* If the git-core doesn't recognize it,
> @@ -234,7 +234,7 @@ static void print_commit(struct commit *commit, struct rev_info *revs)
>  			strbuf_add(&msgbuf, "\n\n", 2);
>  
>  			/* Place wrap_symbol at position i in info->subject */
> -			strcpy(info->subject + i, wrap_symbol);
> +			strlcpy(info->subject + i, wrap_symbol, subject_len - i + 1);

More cleanup.

>  		}
>  	}
>  	cgit_commit_link(info->subject, NULL, NULL, ctx.qry.head,
> diff --git a/ui-patch.c b/ui-patch.c
> index 8007a11..efd7a34 100644
> --- a/ui-patch.c
> +++ b/ui-patch.c
> @@ -11,13 +11,16 @@
>  #include "html.h"
>  #include "ui-shared.h"
>  
> +/* two SHA1 hashes with two dots in between and termination */
> +#define REV_RANGE_LEN 2 * 40 + 3

Should this use GIT_MAX_HEXSZ ?

> +
>  void cgit_print_patch(const char *new_rev, const char *old_rev,
>  		      const char *prefix)
>  {
>  	struct rev_info rev;
>  	struct commit *commit;
>  	struct object_id new_rev_oid, old_rev_oid;
> -	char rev_range[2 * 40 + 3];
> +	char rev_range[REV_RANGE_LEN];
>  	const char *rev_argv[] = { NULL, "--reverse", "--format=email", rev_range, "--", prefix, NULL };
>  	int rev_argc = ARRAY_SIZE(rev_argv) - 1;
>  	char *patchname;

[snip more the_repository conversion.]

> @@ -60,7 +63,7 @@ void cgit_print_patch(const char *new_rev, const char *old_rev,
>  	if (is_null_oid(&old_rev_oid)) {
>  		memcpy(rev_range, oid_to_hex(&new_rev_oid), GIT_SHA1_HEXSZ + 1);
>  	} else {
> -		sprintf(rev_range, "%s..%s", oid_to_hex(&old_rev_oid),
> +		xsnprintf(rev_range, REV_RANGE_LEN, "%s..%s", oid_to_hex(&old_rev_oid),
>  			oid_to_hex(&new_rev_oid));
>  	}
>  
> diff --git a/ui-plain.c b/ui-plain.c
> index ddb3e48..070c34b 100644
> --- a/ui-plain.c
> +++ b/ui-plain.c
> @@ -185,7 +185,7 @@ void cgit_print_plain(void)
>  		cgit_print_error_page(404, "Not found", "Not found");
>  		return;
>  	}
> -	commit = lookup_commit_reference(&oid);
> +	commit = lookup_commit_reference(the_repository, &oid);
>  	if (!commit || parse_commit(commit)) {
>  		cgit_print_error_page(404, "Not found", "Not found");
>  		return;
> diff --git a/ui-shared.c b/ui-shared.c
> index 739505a..f8b438b 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -1176,14 +1176,14 @@ void cgit_set_title_from_path(const char *path)
>  				continue;
>  			}
>  			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
> -			strcat(new_title, "\\");
> +			strncat(new_title, "\\", 2);

Does this add anything?  (And I think n should be 1 here since "\\" is a
single character.)

>  			path_last_end = path_index;
>  		}
>  	}
>  	if (path_last_end)
>  		strncat(new_title, path, path_last_end);
>  
> -	strcat(new_title, " - ");
> -	strcat(new_title, ctx.page.title);
> +	strncat(new_title, " - ", 4);
> +	strncat(new_title, ctx.page.title, sizeof(new_title) - strlen(new_title));

n should be "sizeof(new_title) - strlen(new_title) - 1" here shouldn't
it?

>  	ctx.page.title = new_title;
>  }

[snip the rest]

I don't have any comments on the remainder of the patch that aren't just
repetitions of the above on more bits of code so I'm not going to type
them out :-)