Subject: Re: [PATCH 13/16] submodule: teach unpack_trees() to update submodules

From: Brandon Williams <[email-masked]>
Date: Tue, 15 Nov 2016 16:25:20 -0800

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