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.
On Tue, Aug 16, 2016 at 2:14 PM, Junio C Hamano [off-list ref] wrote:
Jacob Keller [off-list ref] writes:
quoted
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.
Probably. Unfortunately what I really need is to be able to convert
(almost) all diff options from the diff_options structure back into
command line flags. This is the primary reason I would prefer to not
use a sub-command, but I'm not really sure what's the best way to
actually DO that in a safe way.
The sub command brings nice separation between the superproject and
its submodules... but it has problem because we can't use C calls
directly, so I can't pass the options along myself.
Thoughts on that? Or should we just limit ourselves to only some
options get propagated to the submodule?
Thanks,
Jake