Re: [PATCH 1/1] submodule: Fix 'submodule status' when called from a subdirectory

From: Junio C Hamano <gitster@pobox.com>
Date: Nov 24, 2019, 05:17 UTC
Message-ID: <xmqqd0dh26q3.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <2004f4aaa99e5d936a3f3d141b5612df9b013f14.1574490521.git.gitgitgadget@gmail.com>

"Manish Goregaokar via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Manish Goregaokar <manishsmail@gmail.com>
> Subject: [PATCH 1/1] submodule: Fix 'submodule status' when called from a subdirectory

At least, downcase 'fix' to match the project convention on commit titles (you can see "git shortlog -n 100 --no-merges", for example).

> Previously, when calling `git submodule status` while in a
> subdirectory, it was incorrectly not detecting modified submodules and
> thus reporting that all of the submodules were unchanged.

We do not have to "Previously, ", as the norm for our log messages is to talk about the status quo (the state before applying your patch) in the current tense. The word used like this is fine, though: "Previously commit X changed the behaviour of feature A, but it was wrong for such and such reasons; let's address the issue commit X wanted to change by doing this"

> This was because the submodule helper was calling `diff-index` with the
> submodule path assuming the path was relative to the current prefix
> directory, however the submodule path used is actually relative to the root.

OK; modulo tense, nicely explained.

> This fixes the bug by setting the `prefix` to NULL when running
> `diff-index` from the helper, so that it correctly interprets the path
> as relative to the repository root.

And then we outline the fix as if we are giving an order to the codebase "to be like so", in imperative mood. I.e.

    Always pass NULL as the `prefix` when running the diff-index
    command inside the submodule, to make sure that we see changes
    to submodules anywhere in the superproject's tree, not limited
    to the current directory.

or something like that. By the way, aren't we running diff-files, not diff-index, in this context?

> Signed-off-by: Manish Goregaokar <manishsmail@gmail.com>
> ---
>  builtin/submodule--helper.c | 5 ++++-
>  1 file changed, 4 insertions(+), 1 deletion(-)

This fix should be protected by an additional test or two to make sure others won't break it again.

Show 13 quoted lines

> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> index 909e77e802..abc5b46d46 100644
> --- a/builtin/submodule--helper.c
> +++ b/builtin/submodule--helper.c
> @@ -802,7 +802,10 @@ static void status_submodule(const char *path, const struct object_id *ce_oid,
>  			 path, NULL);
>  
>  	git_config(git_diff_basic_config, NULL);
> -	repo_init_revisions(the_repository, &rev, prefix);
> +	/*
> +	 * prefix is NULL since path is an absolute path
> +	 */
> +	repo_init_revisions(the_repository, &rev, NULL);

Absolute??? Isn't 'path' relative to the top-level of the superproject's working tree?

In any case, I do not think this does not deserve a multi-line in-code comment, once we get the code right. Explaining why this must be NULL instead of prefix is better done in the commit log message.

Thanks.