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

Jacob Keller [off-list ref] writes:

quoted

quoted

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