Re: [PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff

[Jacob Keller]

On Thu, Aug 18, 2016 at 12:47 PM, Stefan Beller  wrote:
> On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller  
> wrote:
>
>
>
>> if (o->submodule_format == DIFF_SUBMODULE_LOG &&
>> (!one->mode || S_ISGITLINK(one->mode)) &&
>> (!two->mode || S_ISGITLINK(two->mode))) {
>> @@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
>> two->dirty_submodule,
>> meta, del, add, reset);
>> return;
>> +   } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
>> +  (!one->mode || S_ISGITLINK(one->mode)) &&
>> +  (!two->mode || S_ISGITLINK(two->mode))) {
>
> The ! mode is for added and deleted submodules, I guess?
>

I think so? I don't really know, but it was there before for
show_submodule_summary so I left it this way.

>> diff --git a/diff.h b/diff.h
>> index ea5aba668eaa..192c0eedd0ff 100644
>> --- a/diff.h
>> +++ b/diff.h
>> @@ -112,6 +112,7 @@ enum diff_words_type {
>>  enum diff_submodule_format {
>> DIFF_SUBMODULE_SHORT = 0,
>> DIFF_SUBMODULE_LOG,
>> +   DIFF_SUBMODULE_INLINE_DIFF,
>>  };
>>
>>  struct diff_options {
>> diff --git a/submodule.c b/submodule.c
>> index e341ca7ffefd..e5f1138f4362 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -436,6 +436,67 @@ void show_submodule_summary(FILE *f, const char *path,
>> clear_commit_marks(right, ~0);
>>  }
>>
>> +void show_submodule_inline_diff(FILE *f, const char *path,
>> +   const char *line_prefix,
>> +   unsigned char one[20], unsigned char two[20],
>> +   unsigned dirty_submodule, const char *meta,
>> +   const char *del, const char *add, const char *reset,
>> +   const struct diff_options *o)
>> +{
>> +   const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;
>
> submodule.c: In function ‘show_submodule_inline_diff’:
> cache.h:957:3: warning: pointer targets in initialization differ in
> signedness [-Wpointer-sign]
>((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)
>
> submodule.c:446:20: note: in expansion of macro ‘EMPTY_TREE_SHA1_BIN’
>   const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;
>

This should actually be "EMPTY_TREE_SHA1_BIN_LITERAL, and these should
be unsigned, you're right. This will be fixed differently by using
struct object_id * instead, though.

Thanks,
Jake

>
>
>> +   struct commit *left = NULL, *right = NULL;
>> +   struct strbuf submodule_dir = STRBUF_INIT;
>> +   struct child_process cp = CHILD_PROCESS_INIT;
>> +
>> +   show_submodule_header(f, path, line_prefix, one, two, 
>> dirty_submodule,
>> + meta, reset, , );
>> +
>> +   /* We need a valid left and right commit to display a difference */
>> +   if (!(left || is_null_sha1(one)) ||
>> +   !(right || is_null_sha1(two)))
>> +   goto done;
>> +
>> +   if (left)
>> +   old = one;
>
> submodule.c:460:7: warning: pointer targets in assignment differ in
> signedness [-Wpointer-sign]
>old = one;
>
>

This will also be fixed by switching to object_id.

>
>> +   if (right)
>> +   new = two;
>> +
>> +   fflush(f);
>> +   cp.git_cmd = 1;
>> +   cp.dir = path;
>> +   cp.out = dup(fileno(f));
>> +   cp.no_stdin = 1;
>> +
>> +   /* TODO: other options may need to be passed here. */
>> +   argv_array_pushl(, "diff");
>> +   argv_array_pushf(, "--line-prefix=%s", line_prefix);
>> +   if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
>> +   argv_array_pushf(, "--src-prefix=%s%s/",
>> +o->b_prefix, path);
>> +   argv_array_pushf(, "--dst-prefix=%s%s/",
>> +o->a_prefix, path);
>> +   } else {
>> +   argv_array_pushf(, "--src-prefix=%s%s/",
>> +o->a_prefix, path);
>> +   argv_array_pushf(, "--dst-prefix=%s%s/",
>> +o->b_prefix, path);
>> +   }
>> +   argv_array_push(, sha1_to_hex(old));
>
> submodule.c:484:2: warning: pointer targets in passing argument 1 of
> ‘sha1_to_hex’ differ in signedness [-Wpointer-sign]
>   argv_array_push(, sha1_to_hex(old));
>
>
> /*
>  * nit: the following comment doesn't adhere to Gits way of doing comments:
>  */
>

Yea, sorry. I have a habbit of the other format because the Linux
kernel networking tree requires this style, despite the rest of the
Linux kernel requiring the style used by git. So, I have to break out
of that habbit but sometimes I miss them.

>> +   /* If the submodule has modified content, we will diff against the
>> +* work tree, under the assumption that the user has asked for the
>> +* diff format and wishes to actually see all differences even if 
>> they
>> +* 

Re: [PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff

[Stefan Beller]

On Wed, Aug 17, 2016 at 5:51 PM, Jacob Keller  wrote:



> if (o->submodule_format == DIFF_SUBMODULE_LOG &&
> (!one->mode || S_ISGITLINK(one->mode)) &&
> (!two->mode || S_ISGITLINK(two->mode))) {
> @@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
> two->dirty_submodule,
> meta, del, add, reset);
> return;
> +   } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
> +  (!one->mode || S_ISGITLINK(one->mode)) &&
> +  (!two->mode || S_ISGITLINK(two->mode))) {

The ! mode is for added and deleted submodules, I guess?

> diff --git a/diff.h b/diff.h
> index ea5aba668eaa..192c0eedd0ff 100644
> --- a/diff.h
> +++ b/diff.h
> @@ -112,6 +112,7 @@ enum diff_words_type {
>  enum diff_submodule_format {
> DIFF_SUBMODULE_SHORT = 0,
> DIFF_SUBMODULE_LOG,
> +   DIFF_SUBMODULE_INLINE_DIFF,
>  };
>
>  struct diff_options {
> diff --git a/submodule.c b/submodule.c
> index e341ca7ffefd..e5f1138f4362 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -436,6 +436,67 @@ void show_submodule_summary(FILE *f, const char *path,
> clear_commit_marks(right, ~0);
>  }
>
> +void show_submodule_inline_diff(FILE *f, const char *path,
> +   const char *line_prefix,
> +   unsigned char one[20], unsigned char two[20],
> +   unsigned dirty_submodule, const char *meta,
> +   const char *del, const char *add, const char *reset,
> +   const struct diff_options *o)
> +{
> +   const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;

submodule.c: In function ‘show_submodule_inline_diff’:
cache.h:957:3: warning: pointer targets in initialization differ in
signedness [-Wpointer-sign]
   ((const unsigned char *) EMPTY_TREE_SHA1_BIN_LITERAL)

submodule.c:446:20: note: in expansion of macro ‘EMPTY_TREE_SHA1_BIN’
  const char *old = EMPTY_TREE_SHA1_BIN, *new = EMPTY_TREE_SHA1_BIN;



> +   struct commit *left = NULL, *right = NULL;
> +   struct strbuf submodule_dir = STRBUF_INIT;
> +   struct child_process cp = CHILD_PROCESS_INIT;
> +
> +   show_submodule_header(f, path, line_prefix, one, two, dirty_submodule,
> + meta, reset, , );
> +
> +   /* We need a valid left and right commit to display a difference */
> +   if (!(left || is_null_sha1(one)) ||
> +   !(right || is_null_sha1(two)))
> +   goto done;
> +
> +   if (left)
> +   old = one;

submodule.c:460:7: warning: pointer targets in assignment differ in
signedness [-Wpointer-sign]
   old = one;



> +   if (right)
> +   new = two;
> +
> +   fflush(f);
> +   cp.git_cmd = 1;
> +   cp.dir = path;
> +   cp.out = dup(fileno(f));
> +   cp.no_stdin = 1;
> +
> +   /* TODO: other options may need to be passed here. */
> +   argv_array_pushl(, "diff");
> +   argv_array_pushf(, "--line-prefix=%s", line_prefix);
> +   if (DIFF_OPT_TST(o, REVERSE_DIFF)) {
> +   argv_array_pushf(, "--src-prefix=%s%s/",
> +o->b_prefix, path);
> +   argv_array_pushf(, "--dst-prefix=%s%s/",
> +o->a_prefix, path);
> +   } else {
> +   argv_array_pushf(, "--src-prefix=%s%s/",
> +o->a_prefix, path);
> +   argv_array_pushf(, "--dst-prefix=%s%s/",
> +o->b_prefix, path);
> +   }
> +   argv_array_push(, sha1_to_hex(old));

submodule.c:484:2: warning: pointer targets in passing argument 1 of
‘sha1_to_hex’ differ in signedness [-Wpointer-sign]
  argv_array_push(, sha1_to_hex(old));


/*
 * nit: the following comment doesn't adhere to Gits way of doing comments:
 */

> +   /* If the submodule has modified content, we will diff against the
> +* work tree, under the assumption that the user has asked for the
> +* diff format and wishes to actually see all differences even if they
> +* haven't yet been committed to the submodule yet.
> +*/

Makes sort of sense when new is HEAD.

However if I have given an explicit sha1 in the superproject, e.g.:

# (in the e.g. gerrit with a dirty submodule, I'd still expect to
# get the dirtyness ignored, but the diff of those two states?)
git diff --submodule=diff  cc82b24..5222e66 plugins/

> +   if (!(dirty_submodule & DIRTY_SUBMODULE_MODIFIED))
> +   argv_array_push(, sha1_to_hex(new));
> +


> +# Tested non-UTF-8 encoding
> +test_encoding="ISO8859-1"
...
> +# String "added" in German (translated with Google Translate), encoded in 
> UTF-8,
> +# used in sample commit log messages in add_file() function below.
> +added=$(printf "hinzugef\303\274gt")


> +add_file () {
> +   (
> +   cd "$1" &&
> + 

[PATCH v7 7/7] diff: teach diff to display submodule difference with an inline diff

[Jacob Keller]

From: Jacob Keller 

Teach git-diff and friends a new format for displaying the difference
of a submodule. The new format is an inline diff of the contents of the
submodule between the commit range of the update. This allows the user
to see the actual code change caused by a submodule update.

Add tests for the new format and option.

Signed-off-by: Jacob Keller 
---
 Documentation/diff-config.txt|   9 +-
 Documentation/diff-options.txt   |  17 +-
 diff.c   |  31 +-
 diff.h   |   1 +
 submodule.c  |  61 +++
 submodule.h  |   6 +
 t/t4059-diff-submodule-option-diff-format.sh | 733 +++
 7 files changed, 838 insertions(+), 20 deletions(-)
 create mode 100755 t/t4059-diff-submodule-option-diff-format.sh

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index d5a5b17d5088..0eded24034b5 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -122,10 +122,11 @@ diff.suppressBlankEmpty::
 
 diff.submodule::
Specify the format in which differences in submodules are
-   shown.  The "log" format lists the commits in the range like
-   linkgit:git-submodule[1] `summary` does.  The "short" format
-   format just shows the names of the commits at the beginning
-   and end of the range.  Defaults to short.
+   shown.  The "short" format just shows the names of the commits
+   at the beginning and end of the range. The "log" format lists
+   the commits in the range like linkgit:git-submodule[1] `summary`
+   does. The "diff" format shows an inline diff of the changed
+   contents of the submodule. Defaults to "short".
 
 diff.wordRegex::
A POSIX Extended Regular Expression used to determine what is a "word"
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index cc4342e2034d..7805a0ccadf2 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -210,13 +210,16 @@ any of those replacements occurred.
of the `--diff-filter` option on what the status letters mean.
 
 --submodule[=]::
-   Specify how differences in submodules are shown.  When `--submodule`
-   or `--submodule=log` is given, the 'log' format is used.  This format 
lists
-   the commits in the range like linkgit:git-submodule[1] `summary` does.
-   Omitting the `--submodule` option or specifying `--submodule=short`,
-   uses the 'short' format. This format just shows the names of the commits
-   at the beginning and end of the range.  Can be tweaked via the
-   `diff.submodule` configuration variable.
+   Specify how differences in submodules are shown.  When specifying
+   `--submodule=short` the 'short' format is used.  This format just
+   shows the names of the commits at the beginning and end of the range.
+   When `--submodule` or `--submodule=log` is specified, the 'log'
+   format is used.  This format lists the commits in the range like
+   linkgit:git-submodule[1] `summary` does.  When `--submodule=diff`
+   is specified, the 'diff' format is used.  This format shows an
+   inline diff of the changes in the submodule contents between the
+   commit range.  Defaults to `diff.submodule` or the 'short' format
+   if the config option is unset.
 
 --color[=]::
Show colored diff.
diff --git a/diff.c b/diff.c
index d6b321da3d1d..e119758aba82 100644
--- a/diff.c
+++ b/diff.c
@@ -135,6 +135,8 @@ static int parse_submodule_params(struct diff_options 
*options, const char *valu
options->submodule_format = DIFF_SUBMODULE_LOG;
else if (!strcmp(value, "short"))
options->submodule_format = DIFF_SUBMODULE_SHORT;
+   else if (!strcmp(value, "diff"))
+   options->submodule_format = DIFF_SUBMODULE_INLINE_DIFF;
else
return -1;
return 0;
@@ -2300,6 +2302,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;
+   }
+
if (o->submodule_format == DIFF_SUBMODULE_LOG &&
(!one->mode || S_ISGITLINK(one->mode)) &&
(!two->mode || S_ISGITLINK(two->mode))) {
@@ -2311,6 +2322,17 @@ static void builtin_diff(const char *name_a,
two->dirty_submodule,
meta, del, add, reset);
return;
+   } else if (o->submodule_format == DIFF_SUBMODULE_INLINE_DIFF &&
+