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

[Junio C Hamano <gitster@pobox.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.

> 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.