Re: [PATCHv2 2/3] submodule-config: rename commit_sha1 to commit_or_tree

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 22, 2016, 03:37 UTC
Message-ID: <xmqq8tscgp75.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20161121232709.8906-3-sbeller@google.com>

Stefan Beller <sbeller@google.com> writes:

Show 6 quoted lines

> It is also possible to pass in a tree hash to lookup a submodule config.
> Make it clear by naming the variables accrodingly. Looking up a submodule
> config by tree hash will come in handy in a later patch.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---

Yeah, I noticed that while reading the previous round, too, but...

> -`const struct submodule *submodule_from_name(const unsigned char *commit_sha1, const char *name)`::
> +`const struct submodule *submodule_from_name(const unsigned char *commit_or_tree, const char *name)`::
>  
>  	The same as above but lookup by name.

Doesn't (the) "above", aka submodule_from_path() want the same treatment?

Also the explanation of "above" has room for improvement. Namely it says:

    Lookup values for one submodule by its commit_sha1 and path.

I do not think the commit-sha1 (or commit-or-tree-object-name) is "ITS" object name for the submodule. The name belongs to the containing superproject commit (or tree), no?

    Given a tree-ish in the superproject and a path, return the
    submodule that is bound at the path in the named tree.

is what I would write for that one. Thinking about it a bit further, "treeish_name" would be a much better name for the variable than "commit_or_tree", as "treeish" is an established short and sweet word that means "commit_or_tree", and having a "name" somewhere in the variable name makes it clear that we are not passing the pointer to an in-core object (e.g. "struct commit *").

Show 10 quoted lines

> +test_expect_success 'using tree sha1 works' '
> +	(
> +		cd super &&
> +		tree=$(git rev-parse HEAD^{tree}) &&
> +		commit=$(git rev-parse HEAD^{commit}) &&
> +		test-submodule-config $commit b >expect &&
> +		test-submodule-config $tree b >actual &&
> +		test_cmp expect actual
> +	)
> +'

Perhaps also test a tag that points at the commit?