Re: [PATCH 15/16] refs: add LMDB refs backend

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 5, 2015, 00:08 UTC
Message-ID: <xmqqzixpoiyt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1449102921-7707-16-git-send-email-dturner@twopensource.com>

David Turner <dturner@twopensource.com> writes:

> +	while (!mdb_ret) {
> +		if (starts_with(key.mv_data, refname) &&
> +		    ((char*)key.mv_data)[refname_len - 2] == '/') {

ERROR: "(foo*)" should be "(foo *)"
#877: FILE: refs/lmdb-backend.c:514:
+                   ((char*)key.mv_data)[refname_len - 2] == '/') {

Show 17 quoted lines

> +static int show_one_reflog_ent(struct strbuf *sb, each_reflog_ent_fn fn, void *cb_data)
> +{
> +	unsigned char osha1[20], nsha1[20];
> +	char *email_end, *message;
> +	unsigned long timestamp;
> +	int tz;
> +
> +	/* old (raw) new (raw) name <email> SP time TAB msg LF */
> +	if (sb->len < 41 || sb->buf[sb->len - 1] != '\n' ||
> +	    !(email_end = strchr(sb->buf + 40, '>')) ||
> +	    email_end[1] != ' ' ||
> +	    !(timestamp = strtoul(email_end + 2, &message, 10)) ||
> +	    !message || message[0] != ' ' ||
> +	    (message[1] != '+' && message[1] != '-') ||
> +	    !isdigit(message[2]) || !isdigit(message[3]) ||
> +	    !isdigit(message[4]) || !isdigit(message[5]))
> +		return 0; /* corrupt? */

ERROR: do not use assignment in if condition
#1024: FILE: refs/lmdb-backend.c:661:
+       if (sb->len < 41 || sb->buf[sb->len - 1] != '\n' ||

Show 27 quoted lines

> +static char *check_ref(MDB_txn *txn, const char *refname,
> +		       const unsigned char *old_sha1,
> +		       unsigned char *resolved_sha1, int flags,
> +		       int *type_p)
> +{
> +	int mustexist = (old_sha1 && !is_null_sha1(old_sha1));
> +	int resolve_flags = 0;
> +	int type;
> +	char *resolved_refname;
> +
> +	if (mustexist)
> +		resolve_flags |= RESOLVE_REF_READING;
> +	if (flags & REF_DELETING) {
> +		resolve_flags |= RESOLVE_REF_ALLOW_BAD_NAME;
> +		if (flags & REF_NODEREF)
> +			resolve_flags |= RESOLVE_REF_NO_RECURSE;
> +	}
> +
> +	/*
> +	 * The first time we resolve the refname, we're just trying to
> +	 * see if there is any ref at all by this name, even if it is
> +	 * a broken symref.
> +	 */
> +	refname = resolve_ref_unsafe(refname, resolve_flags,
> +				     resolved_sha1, &type);
> +	if (type_p)
> +	    *type_p = type;

WARNING: suspect code indent for conditional statements (8, 12)
#1177: FILE: refs/lmdb-backend.c:814:
+       if (type_p)
+           *type_p = type;

Indeed, this line should be indented with two HTs.

Show 5 quoted lines

> +	while (!mdb_ret) {
> +		if (key.mv_size < len)
> +			break;
> +
> +		if (!starts_with(key.mv_data, log_path) || ((char*)key.mv_data)[len - 1] != 0)

ERROR: "(foo*)" should be "(foo *)"
#1254: FILE: refs/lmdb-backend.c:891:
+               if (!starts_with(key.mv_data, log_path) || ((char*)key.mv_data)[len - 1] != 0)

> +		if (strcmp (refname, orig_refname) &&

WARNING: space prohibited between function name and open parenthesis '('
#1366: FILE: refs/lmdb-backend.c:1003:
+               if (strcmp (refname, orig_refname) &&

This concludes my first pass, mechanical "lint" (with help from checkpatch.pl). I'll hopefully have time to do the more meaningful design and implementation review later.

Thanks.