[PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix

[PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

This will be used by a future patch which implements a diff mode for
submodule display. Without this, the diff output would incorrectly
display when using both -p and --graph during a git-log.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
As suggested by Junio, I implemented --line-prefix to enable the graph
display correctly. This works by a neat trick of adding to the msgbuf,
so no code needs to be altered. I presumed that the line prefix should
go *after* the graphs own prefix.

 Documentation/diff-options.txt |  3 +++
 diff.c                         | 12 +++++++++++-
 diff.h                         |  1 +
 3 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 705a87394200..e7b729f3644f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -569,5 +569,8 @@ endif::git-format-patch[]
 --no-prefix::
 	Do not show any source or destination prefix.
 
+--line-prefix=<prefix>::
+	Prepend an additional prefix to every diff output line.
+
 For more detailed explanation on these common options, see also
 linkgit:gitdiffcore[7].
diff --git a/diff.c b/diff.c
index b43d3dd2ecb7..6fe5c6d084a3 100644
--- a/diff.c
+++ b/diff.c
@@ -1167,10 +1167,16 @@ const char *diff_get_color(int diff_use_color, enum color_diff ix)
 const char *diff_line_prefix(struct diff_options *opt)
 {
 	struct strbuf *msgbuf;
+
 	if (!opt->output_prefix)
-		return "";
+		if (opt->line_prefix)
+			return opt->line_prefix;
+		else
+			return "";
 
 	msgbuf = opt->output_prefix(opt, opt->output_prefix_data);
+	if (opt->line_prefix)
+		strbuf_addstr(msgbuf, opt->line_prefix);
 	return msgbuf->buf;
 }
 
@@ -3966,6 +3972,10 @@ int diff_opt_parse(struct diff_options *options,
 		options->a_prefix = optarg;
 		return argcount;
 	}
+	else if ((argcount = parse_long_opt("line-prefix", av, &optarg))) {
+		options->line_prefix = optarg;
+		return argcount;
+	}
 	else if ((argcount = parse_long_opt("dst-prefix", av, &optarg))) {
 		options->b_prefix = optarg;
 		return argcount;
diff --git a/diff.h b/diff.h
index 125447be09eb..6a91a1139686 100644
--- a/diff.h
+++ b/diff.h
@@ -115,6 +115,7 @@ struct diff_options {
 	const char *pickaxe;
 	const char *single_follow;
 	const char *a_prefix, *b_prefix;
+	const char *line_prefix;
 	unsigned flags;
 	unsigned touched_flags;
 
-- 
2.9.2.872.g0b694e0.dirty

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

[Jacob Keller]

From: Jacob Keller <jacob.keller@gmail.com>

For projects which have frequent updates to submodules it is often
useful to be able to see a submodule update commit as a difference.
Teach diff's --submodule= a new "diff" format which will execute a diff
for the submodule between the old and new commit, and display it as
a standard diff. This allows users to easily see what changed in
a submodule without having to dig into the submodule themselves.

Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
---
Changes since v2:
* dropped the prepare_submodule_diff as it's not really needed
* added --line-prefix to properly prefix every line
* added correct use of a_prefix and b_prefix
* use dup and fileno instead so that we don't have to copy output

I still need to sort out some tests, and I ran into one interesting
issue: I had to do make-install in order to get the sub command to
process the new diff option... is that normal? I think that may cause
issues during tests as well... Shouldn't we use the same binaries we're
running from and not search in the install path when running a sub
command?

 Documentation/diff-config.txt  |  3 +-
 Documentation/diff-options.txt |  6 ++--
 diff.c                         | 39 ++++++++++++++++--------
 diff.h                         |  2 +-
 submodule.c                    | 67 ++++++++++++++++++++++++++++++++++++++++++
 submodule.h                    |  6 ++++
 6 files changed, 107 insertions(+), 16 deletions(-)

diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
index d5a5b17d5088..f5d693afad6c 100644
--- a/Documentation/diff-config.txt
+++ b/Documentation/diff-config.txt
@@ -123,7 +123,8 @@ 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
+	linkgit:git-submodule[1] `summary` does.  The "diff" format shows the
+	diff between the beginning and end of the range. The "short" format
 	format just shows the names of the commits at the beginning
 	and end of the range.  Defaults to short.
 
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index e7b729f3644f..988068225463 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -215,8 +215,10 @@ any of those replacements occurred.
 	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.
+	at the beginning and end of the range. When `--submodule=diff` is
+	given, the 'diff' format is used. This format shows the diff between
+	the old and new submodule commmit from the perspective of the
+	submodule.  Can be tweaked via the `diff.submodule` configuration variable.
 
 --color[=<when>]::
 	Show colored diff.
diff --git a/diff.c b/diff.c
index 6fe5c6d084a3..2d23767739a4 100644
--- a/diff.c
+++ b/diff.c
@@ -130,12 +130,18 @@ static int parse_dirstat_params(struct diff_options *options, const char *params
 
 static int parse_submodule_params(struct diff_options *options, const char *value)
 {
-	if (!strcmp(value, "log"))
+	if (!strcmp(value, "log")) {
 		DIFF_OPT_SET(options, SUBMODULE_LOG);
-	else if (!strcmp(value, "short"))
+		DIFF_OPT_CLR(options, SUBMODULE_DIFF);
+	} else if (!strcmp(value, "diff")) {
+		DIFF_OPT_SET(options, SUBMODULE_DIFF);
 		DIFF_OPT_CLR(options, SUBMODULE_LOG);
-	else
+	} else if (!strcmp(value, "short")) {
+		DIFF_OPT_CLR(options, SUBMODULE_DIFF);
+		DIFF_OPT_CLR(options, SUBMODULE_LOG);
+	} else {
 		return -1;
+	}
 	return 0;
 }
 
@@ -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;
+	}
+
 	if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
 			(!one->mode || S_ISGITLINK(one->mode)) &&
 			(!two->mode || S_ISGITLINK(two->mode))) {
@@ -2316,6 +2331,15 @@ static void builtin_diff(const char *name_a,
 				two->dirty_submodule,
 				meta, del, add, reset);
 		return;
+	} else if (DIFF_OPT_TST(o, SUBMODULE_DIFF) &&
+			(!one->mode || S_ISGITLINK(one->mode)) &&
+			(!two->mode || S_ISGITLINK(two->mode))) {
+		show_submodule_diff(o->file, one->path ? one->path : two->path,
+				line_prefix,
+				one->oid.hash, two->oid.hash,
+				two->dirty_submodule,
+				meta, a_prefix, b_prefix, reset);
+		return;
 	}
 
 	if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
@@ -2323,15 +2347,6 @@ static void builtin_diff(const char *name_a,
 		textconv_two = get_textconv(two);
 	}
 
-	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;
-	}
-
 	/* Never use a non-valid filename anywhere if at all possible */
 	name_a = DIFF_FILE_VALID(one) ? name_a : name_b;
 	name_b = DIFF_FILE_VALID(two) ? name_b : name_a;
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)
 #define DIFF_OPT_HAS_CHANGES         (1 << 10)
 #define DIFF_OPT_QUICK               (1 << 11)
 #define DIFF_OPT_NO_INDEX            (1 << 12)
diff --git a/submodule.c b/submodule.c
index 1b5cdfb7e784..36f8fd00c3ce 100644
--- a/submodule.c
+++ b/submodule.c
@@ -333,6 +333,73 @@ static void print_submodule_summary(struct rev_info *rev, FILE *f,
 	strbuf_release(&sb);
 }
 
+void show_submodule_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 *a_prefix, const char *b_prefix,
+		const char *reset)
+{
+	struct child_process cp = CHILD_PROCESS_INIT;
+	struct strbuf sb = STRBUF_INIT;
+	struct strbuf submodule_git_dir = STRBUF_INIT;
+	const char *git_dir, *message = NULL;
+	int create_dirty_diff = 0;
+	FILE *diff;
+	char c;
+
+	if ((dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) ||
+	    (dirty_submodule & DIRTY_SUBMODULE_MODIFIED))
+		create_dirty_diff = 1;
+
+	strbuf_addf(&sb, "%s%sSubmodule %s %s..", line_prefix, meta, path,
+			find_unique_abbrev(one, DEFAULT_ABBREV));
+	strbuf_addf(&sb, "%s:%s\n", !create_dirty_diff ?
+		    find_unique_abbrev(two, DEFAULT_ABBREV) : "", reset);
+	fwrite(sb.buf, sb.len, 1, f);
+
+	strbuf_addf(&submodule_git_dir, "%s/.git", path);
+	git_dir = read_gitfile(submodule_git_dir.buf);
+	if (!git_dir)
+		git_dir = submodule_git_dir.buf;
+	if (!is_directory(git_dir)) {
+		strbuf_reset(&submodule_git_dir);
+		strbuf_addf(&submodule_git_dir, ".git/modules/%s", path);
+		git_dir = submodule_git_dir.buf;
+	}
+
+	fflush(f);
+
+	cp.git_cmd = 1;
+	if (!create_dirty_diff)
+		cp.dir = git_dir;
+	else
+		cp.dir = path;
+	cp.out = dup(fileno(f));
+	cp.no_stdin = 1;
+	argv_array_push(&cp.args, "diff");
+	argv_array_pushf(&cp.args, "--line-prefix=%s", line_prefix);
+	argv_array_pushf(&cp.args, "--src-prefix=%s%s/", a_prefix, path);
+	argv_array_pushf(&cp.args, "--dst-prefix=%s%s/", b_prefix, path);
+	argv_array_push(&cp.args, sha1_to_hex(one));
+
+	/*
+	 * If the submodule has untracked or modified contents don't add the
+	 * current stored commit. Thus, we will diff between the previous
+	 * value and the current tip of working tree. The result is a complete
+	 * diff which shows all changes to the submodule, not just changes
+	 * that have been committed to the super project.
+	 */
+	if (!create_dirty_diff)
+		argv_array_push(&cp.args, sha1_to_hex(two));
+
+	if (run_command(&cp))
+		fprintf(f, "(diff failed)\n");
+
+	strbuf_release(&submodule_git_dir);
+	strbuf_release(&sb);
+}
+
 void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
 		unsigned char one[20], unsigned char two[20],
diff --git a/submodule.h b/submodule.h
index 2af939099819..f32a25c667ab 100644
--- a/submodule.h
+++ b/submodule.h
@@ -41,6 +41,12 @@ int parse_submodule_update_strategy(const char *value,
 		struct submodule_update_strategy *dst);
 const char *submodule_strategy_to_string(const struct submodule_update_strategy *s);
 void handle_ignore_submodules_arg(struct diff_options *diffopt, const char *);
+void show_submodule_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 *a_prefix, const char *b_prefix,
+		const char *reset);
 void show_submodule_summary(FILE *f, const char *path,
 		const char *line_prefix,
 		unsigned char one[20], unsigned char two[20],
-- 
2.9.2.872.g0b694e0.dirty

Re: [PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

> As suggested by Junio, I implemented --line-prefix to enable the graph
> display correctly. This works by a neat trick of adding to the msgbuf,
> so no code needs to be altered. I presumed that the line prefix should
> go *after* the graphs own prefix.

I do not understand the last sentence.

The motivation I suggested the --line-prefix for is for a scenario
in which "git log -p --graph" that recurses into a submodule causes
"git diff A B" between the two commits in a submodule to run; the
internal diff machinery driven by "log -p --graph" for the
superproject knows what the graph lines that depict the lineages of
history in the superproject should look like, but the "git diff A B"
in the submodule of course does not, so while showing e.g. "| | |"
(three lineages) for the history in the superproject, you would run
"git diff --line-prefix='| | | ' A B" and by showing them before
anything that "git diff A B" without the new option would have
produced, you could mimick and match the graph output in the
superproject.

In that scenario, the line prefix _is_ the graph's prefix in the
superproject.

You might be envisioning a future enhancement where the recursive
one uses not "-Submodule commit A"/"-Submodule commit B", and not
"diff A B", but "log -p A...B" in the submodule, and in such a case,
it might make sense to run "log -p --graph A...B" instead when the
command the end user run in the superproject asked for "--graph".
You would use the same "--line-prefix='| | | '" when running the
"log -p --graph A...B" command in the submodule, so that the output
for the submodule will go _after_ the graph of the superproject, but
in that case, the output for the submodule would also include its
own graph to show the relationship between A and B.  The line-prefix
must come before that graph part (and also if it is "git log" run in
the submodule, the same line-prefix must come before each line of
the log message output).

With that understanding/assumption, "line prefix should go after the
graph's own prefix" sounds like a wrong choice to me.  Shouldn't the
prefix go before everything else?

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

[Junio C Hamano]

Jacob Keller <jacob.e.keller@intel.com> writes:

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

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

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

Re: [PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix

[Jacob Keller]

On Wed, Aug 10, 2016 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> As suggested by Junio, I implemented --line-prefix to enable the graph
>> display correctly. This works by a neat trick of adding to the msgbuf,
>> so no code needs to be altered. I presumed that the line prefix should
>> go *after* the graphs own prefix.
>
> I do not understand the last sentence.
>
> The motivation I suggested the --line-prefix for is for a scenario
> in which "git log -p --graph" that recurses into a submodule causes
> "git diff A B" between the two commits in a submodule to run; the
> internal diff machinery driven by "log -p --graph" for the
> superproject knows what the graph lines that depict the lineages of
> history in the superproject should look like, but the "git diff A B"
> in the submodule of course does not, so while showing e.g. "| | |"
> (three lineages) for the history in the superproject, you would run
> "git diff --line-prefix='| | | ' A B" and by showing them before
> anything that "git diff A B" without the new option would have
> produced, you could mimick and match the graph output in the
> superproject.
>
> In that scenario, the line prefix _is_ the graph's prefix in the
> superproject.
>
> You might be envisioning a future enhancement where the recursive
> one uses not "-Submodule commit A"/"-Submodule commit B", and not
> "diff A B", but "log -p A...B" in the submodule, and in such a case,
> it might make sense to run "log -p --graph A...B" instead when the
> command the end user run in the superproject asked for "--graph".
> You would use the same "--line-prefix='| | | '" when running the
> "log -p --graph A...B" command in the submodule, so that the output
> for the submodule will go _after_ the graph of the superproject, but
> in that case, the output for the submodule would also include its
> own graph to show the relationship between A and B.  The line-prefix
> must come before that graph part (and also if it is "git log" run in
> the submodule, the same line-prefix must come before each line of
> the log message output).
>
> With that understanding/assumption, "line prefix should go after the
> graph's own prefix" sounds like a wrong choice to me.  Shouldn't the
> prefix go before everything else?
>

I don't think it matters, but if it did, I assumed that a users use of
"line-prefix" goes "closest to the edge with the diff content" and
then any additional prefix from log --graph would go to the left of
that.

Thanks,
Jake

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

[Jacob Keller]

On Wed, Aug 10, 2016 at 3:05 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> @@ -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.

>
>>       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?

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

Re: [PATCH v3 1/2] diff: add --line-prefix option for passing in a prefix

[Jacob Keller]

On Wed, Aug 10, 2016 at 2:58 PM, Junio C Hamano <gitster@pobox.com> wrote:
> You might be envisioning a future enhancement where the recursive
> one uses not "-Submodule commit A"/"-Submodule commit B", and not
> "diff A B", but "log -p A...B" in the submodule, and in such a case,
> it might make sense to run "log -p --graph A...B" instead when the
> command the end user run in the superproject asked for "--graph".
> You would use the same "--line-prefix='| | | '" when running the
> "log -p --graph A...B" command in the submodule, so that the output
> for the submodule will go _after_ the graph of the superproject, but
> in that case, the output for the submodule would also include its
> own graph to show the relationship between A and B.  The line-prefix
> must come before that graph part (and also if it is "git log" run in
> the submodule, the same line-prefix must come before each line of
> the log message output).
>

Oops, you are correct. In this case, the line-prefix should come
first, and I will reverse this.

Thanks,
Jake