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

> Jacob Keller <jacob.keller@gmail.com> writes:
>
>>>> +
>>>> +     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.
>