Re: [PATCH v3 2/2] diff: add SUBMODULE_DIFF format to display submodule diff

Jacob Keller [off-list ref] writes:

Hmph, is it safe to raise this when SUBMODULE_DIFF is not in effect?
Not objecting, just asking.

 	if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
...
+	} else if (DIFF_OPT_TST(o, SUBMODULE_DIFF) &&

This makes clear that SUBMODULE_LOG and SUBMODULE_DIFF should not be
independent bits in the diff-opt flag word.  We used to run
something like "log --oneline --left-right A...B" and your new code
runs "diff A B", but the next month somebody else would want to do
"log -p --left-right A...B" or something else, and they are clearly
mutually exclusive.

So I'd really prefer not to see this change; instead, we should move
in the direction where we _REMOVE_ DIFF_OPT_SUBMODULE_LOG from these,
and introduce an enum to hold "how would we show submodule changes"
in the diff_options structure.

On Wed, Aug 10, 2016 at 3:05 PM, Junio C Hamano [off-list ref] wrote:

Jacob Keller [off-list ref] writes:

quoted

@@ -2305,6 +2311,15 @@ static void builtin_diff(const char *name_a,
      struct strbuf header = STRBUF_INIT;
      const char *line_prefix = diff_line_prefix(o);

+     diff_set_mnemonic_prefix(o, "a/", "b/");
+     if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
+             a_prefix = o->b_prefix;
+             b_prefix = o->a_prefix;
+     } else {
+             a_prefix = o->a_prefix;
+             b_prefix = o->b_prefix;
+     }
+

Hmph, is it safe to raise this when SUBMODULE_DIFF is not in effect?
Not objecting, just asking.

The only other code was SUBMODULE_LOG which doesn't get passed the
a_prefix and b_prefix so it shouldn't make a difference.

quoted

      if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
...
+     } else if (DIFF_OPT_TST(o, SUBMODULE_DIFF) &&

This makes clear that SUBMODULE_LOG and SUBMODULE_DIFF should not be
independent bits in the diff-opt flag word.  We used to run
something like "log --oneline --left-right A...B" and your new code
runs "diff A B", but the next month somebody else would want to do
"log -p --left-right A...B" or something else, and they are clearly
mutually exclusive.

They are independent bits, but the set and clear make them mutually
exclusive. How would you implement this instead? Maybe as a separate
field of the diff_options?

quoted

diff --git a/diff.h b/diff.h
index 6a91a1139686..65df44b1fcba 100644
--- a/diff.h
+++ b/diff.h

@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_FIND_COPIES_HARDER  (1 <<  6)
 #define DIFF_OPT_FOLLOW_RENAMES      (1 <<  7)
 #define DIFF_OPT_RENAME_EMPTY        (1 <<  8)
-/* (1 <<  9) unused */
+#define DIFF_OPT_SUBMODULE_DIFF      (1 <<  9)

So I'd really prefer not to see this change; instead, we should move
in the direction where we _REMOVE_ DIFF_OPT_SUBMODULE_LOG from these,
and introduce an enum to hold "how would we show submodule changes"
in the diff_options structure.

Yes, I agree. I will rework that.

Thanks,
Jake