Re: [PATCH] Refactor dirty submodule detection in diff-lib.c

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 12, 2010, 00:55 UTC
Message-ID: <7vfx46we7d.fsf@alter.siamese.dyndns.org>
In-Reply-To: <4B9965A1.60403@web.de>

Jens Lehmann <Jens.Lehmann@web.de> writes:

Show 15 quoted lines

> Moving duplicated code into the new function match_stat_with_submodule().
> Replacing the implicit activation of detailed checks for the dirtiness of
> submodules when DIFF_FORMAT_PATCH was selected with explicitly setting
> the recently added DIFF_OPT_DIRTY_SUBMODULES option in diff_setup_done().
>
> Signed-off-by: Jens Lehmann <Jens.Lehmann@web.de>
> ---
>
> All tests run fine with this patch. All other callsites where the option
> DIFF_FORMAT_PATCH is set look like they don't deal with the work tree
> or they call diff_setup_done() shortly after.
> I am pretty sure about builtin/log.c, as this can't use the work tree
> but only already commited stuff, no?
> But then there is edit_patch() in buitin/add.c, I am not sure it makes
> sense to set DIRTY_SUBMODULES there too. Opinions?

I like this particular refactoring to remove duplicated obscure logic.

I am not sure if it is the right thing to do to return "changed" if the change is only "submodule is dirty even though its HEAD is the same as what is recorded in the index of the superproject", though. See my patches in response to your "[PATCH] git status: Fix false positive "new commits" output for". I think it would be conceptually the right thing to do to feed the same object names in the sha1 field for both sides of the filepair, with the working tree side marked with "dirty-submodule". I also suspect that you would not need to re-resolve the gitlinks in later stages when you generate patches in textual form if you do it that way.