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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

> 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 *").

> +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?