From: Brandon Williams <bmwill@google.com>
Date: Wed, 16 Nov 2016 00:25:20 GMT
Subject: Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules
Message-ID: <20161116002520.GI66382@google.com>
In-Reply-To: <20161115230651.23953-14-sbeller@google.com>

On 11/15, Stefan Beller wrote:
> +		if (!S_ISGITLINK(ce->ce_mode)) {
> +			int flags = CE_MATCH_IGNORE_VALID|CE_MATCH_IGNORE_SKIP_WORKTREE;

For readability you may want to have spaces between the two flags

> +	if (o->index_only
> +	    || (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))
> +		&& (o->reset || ce_uptodate(old))))
> +		return 0;

The coding guidelines say that git prefers to have the logical operators
on the right side like this:

if (o->index_only ||
    (!((old->ce_flags & CE_VALID) || ce_skip_worktree(old))	&&
     (o->reset || ce_uptodate(old))))
	return 0;

It also says the other way is ok too, just a thought :)

> +				if (submodule_is_interesting(old->name, null_sha1)
> +				    && is_submodule_checkout_safe(new->name, &new->oid))
> +					return 0;

here too.

> +			} else {
> +				/*
> +				 * new is not a submodule any more, so only
> +				 * care if we care:
> +				 */
> +				if (submodule_is_interesting(old->name, null_sha1)
> +				    && ok_to_remove_submodule(old->name))
> +					return 0;

and here

-- 
Brandon Williams