Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff

From: Junio C Hamano <gitster@pobox.com>
Date: Aug 16, 2016, 21:14 UTC
Message-ID: <xmqqvaz0bemk.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CA+P7+xqc_WwzjUnF5P4arBhBqgRbtXyKC9QWtRJ3+fmx0Q2+oA@mail.gmail.com>

Jacob Keller <jacob.keller@gmail.com> writes:

Show 21 quoted lines

>>> +
>>> +     if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {
>>> +             /*
>>> +              * If the submodule has modified contents we want to diff
>>> +              * against the work tree, so don't add a second parameter.
>>> +              * This is essentially equivalent of using diff-index instead.
>>> +              * Note that we can't (easily) show the diff of any untracked
>>> +              * files.
>>> +              */
>> ...
>> I am debating myself if this is a good thing to do, though.  The
>> submodule is a separate project for a reason, and there is a reason
>> why the changes haven't been committed.  When asking "what's different
>> between these two superproject states?", should the user really see
>> these uncommitted changes?
>
> Well, the previous submodule code for "log" prints out "submodule has
> untracked content" and "submodule has modified content" so I figured
> the diff might want to show that as a diff too? Or maybe we just stick
> with the messages and only show the difference of what's actually been
> committed.. I think that's probably ok too.

I do not have a strong opinion. We only see DIRTY when we are looking at the working tree at the top-level diff (i.e. "git diff HEAD~ HEAD" at the top-level, or "git diff --cached" will not show DIRTY_SUBMODULE_MODIFIED when a submodule has local modifications in its working tree), so in that sense, it probably makes sense to show the more detailed picture of the working tree like your patch does. After all, choosing to use --submodule=diff is a strong sign that the user who says top-level "git diff [<tree>]" wants to see the details of her work-in-progress.

Do you need to handle "git diff -R [<tree>]" at the top-level a bit differently, by the way? If this function gets the full diff_options that is driving the top-level diff, the DIFF_OPT_REVERSE_DIFF bit would tell you that.