Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff
Jacob Keller writes: > Thoughts on that? Or should we just limit ourselves to only some > options get propagated to the submodule? I think you have to be selective either way. You do not want pathspecs used to limit the top-level paths propagated down when you run a diff or a log in the submodule, for example, and that does not change even if you start using the code in diff-lib.c (after adding the submodule odb as an alternate). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff
On Tue, Aug 16, 2016 at 2:14 PM, Junio C Hamano wrote:
> Jacob Keller writes:
>
+
+ 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 []" wants to see the
> details of her work-in-progress.
>
> Do you need to handle "git diff -R []" 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff
Jacob Keller writes:
>>> +
>>> + 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 []" wants to see the
details of her work-in-progress.
Do you need to handle "git diff -R []" 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff
On Tue, Aug 16, 2016 at 11:48 AM, Junio C Hamano wrote:
> Jacob Keller writes:
>
>> 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.
>
> It would be much better to describe the default one first and then
> more involved one next, no? That would also match what the change
> to "diff-options" in this patch does (can be seen below).
>
The main thing is that "--submodule" alone means "use the log format"
so I think that's why it went first. I can reword this to make it more
clear how this works.
Thanks,
Jake
>> @@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,
>> two->dirty_submodule,
>> meta, del, add, reset);
>> return;
>> + } else if (o->submodule_format == DIFF_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;
>> }
>
> The "either missing or is submodule" check used here is being
> consistent with the existing "submodule=log" case. Good.
>
>> diff --git a/submodule.c b/submodule.c
>> index 1b5cdfb7e784..b1da68dd49c9 100644
>> --- a/submodule.c
>> +++ b/submodule.c
>> @@ -333,6 +333,136 @@ 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 strbuf submodule_git_dir = STRBUF_INIT, sb = STRBUF_INIT;
>> + struct child_process cp = CHILD_PROCESS_INIT;
>> + const char *git_dir;
>> +
>> + if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) {
>> + fprintf(f, "%sSubmodule %s contains untracked content\n",
>> + line_prefix, path);
>> + }
>> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {
>> + fprintf(f, "%sSubmodule %s contains modified content\n",
>> + line_prefix, path);
>> + }
>> +
>> + strbuf_addf(&sb, "%s%sSubmodule %s %s..",
>> + line_prefix, meta, path,
>> + find_unique_abbrev(one, DEFAULT_ABBREV));
>> + strbuf_addf(&sb, "%s:%s",
>> + find_unique_abbrev(two, DEFAULT_ABBREV),
>> + reset);
>> + fwrite(sb.buf, sb.len, 1, f);
>> +
>> + if (is_null_sha1(one))
>> + fprintf(f, " (new submodule)");
>> + if (is_null_sha1(two))
>> + fprintf(f, " (submodule deleted)");
>
> These messages are in sync with show_submodule_summary() that is
> used in --submodule=log codepath. Good.
>
They're not exactly the same due to some ways of splitting up new lines.
>> + /*
>> + * We need to determine the most accurate location to call the sub
>> + * command, and handle the various corner cases involved. We'll first
>> + * attempt to use the path directly if the submodule is checked out.
>> + * Then, if that fails, we'll check the standard module location in
>> + * the git directory. If even this fails, it means we can't do the
>> + * lookup because the module has not been initialized.
>> + */
>
> This is more elaborate than what show_submodule_summary() does,
> isn't it? Would it make the patch series (and the resulting code)
> more understandable if you used the same code by refactoring these
> two? If so, I wonder if it makes sense to split 3/3 into a few
> separate steps:
The show_submodule_summary just uses "add_submodule_odb" which adds
the submodule as an alternate source of objects, if I understand
correctly.
>
> * Update the internal "--submodule=" handling without adding
>the "--submodule=diff" and show_submodule_diff() function.
>
> * Refactor the determination of the submod
Re: [PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff
Jacob Keller writes:
> 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.
It would be much better to describe the default one first and then
more involved one next, no? That would also match what the change
to "diff-options" in this patch does (can be seen below).
> diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
> index cc4342e2034d..d3ca4ad2c2c5 100644
> --- a/Documentation/diff-options.txt
> +++ b/Documentation/diff-options.txt
> @@ -215,8 +215,11 @@ 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. Defaults to `diff.submodule` or 'short' if the config
> + option is unset.
> @@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,
> two->dirty_submodule,
> meta, del, add, reset);
> return;
> + } else if (o->submodule_format == DIFF_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;
> }
The "either missing or is submodule" check used here is being
consistent with the existing "submodule=log" case. Good.
> diff --git a/submodule.c b/submodule.c
> index 1b5cdfb7e784..b1da68dd49c9 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -333,6 +333,136 @@ 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 strbuf submodule_git_dir = STRBUF_INIT, sb = STRBUF_INIT;
> + struct child_process cp = CHILD_PROCESS_INIT;
> + const char *git_dir;
> +
> + if (dirty_submodule & DIRTY_SUBMODULE_UNTRACKED) {
> + fprintf(f, "%sSubmodule %s contains untracked content\n",
> + line_prefix, path);
> + }
> + if (dirty_submodule & DIRTY_SUBMODULE_MODIFIED) {
> + fprintf(f, "%sSubmodule %s contains modified content\n",
> + line_prefix, path);
> + }
> +
> + strbuf_addf(&sb, "%s%sSubmodule %s %s..",
> + line_prefix, meta, path,
> + find_unique_abbrev(one, DEFAULT_ABBREV));
> + strbuf_addf(&sb, "%s:%s",
> + find_unique_abbrev(two, DEFAULT_ABBREV),
> + reset);
> + fwrite(sb.buf, sb.len, 1, f);
> +
> + if (is_null_sha1(one))
> + fprintf(f, " (new submodule)");
> + if (is_null_sha1(two))
> + fprintf(f, " (submodule deleted)");
These messages are in sync with show_submodule_summary() that is
used in --submodule=log codepath. Good.
> + /*
> + * We need to determine the most accurate location to call the sub
> + * command, and handle the various corner cases involved. We'll first
> + * attempt to use the path directly if the submodule is checked out.
> + * Then, if that fails, we'll check the standard module location in
> + * the git directory. If even this fails, it means we can't do the
> + * lookup because the module has not been initialized.
> + */
This is more elaborate than what show_submodule_summary() does,
isn't it? Would it
[PATCH v6 3/3] diff: add SUBMODULE_DIFF format to display submodule diff
From: Jacob Keller
Teach git-diff and friends a new format for displaying the difference of
a submodule using git-diff inside the submodule project. This allows
users to easily see exactly what source changed in a given commit that
updates the submodule pointer. To do this, remove DIFF_SUBMODULE_LOG bit
from the diff options, and instead add a new enum type for these
formats.
Add some tests for the new format and option.
Signed-off-by: Jacob Keller
---
Documentation/diff-config.txt| 3 +-
Documentation/diff-options.txt | 7 +-
diff.c | 41 +-
diff.h | 9 +-
submodule.c | 130 +
submodule.h | 6 +
t/t4059-diff-submodule-option-diff-format.sh | 738 +++
7 files changed, 915 insertions(+), 19 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..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 cc4342e2034d..d3ca4ad2c2c5 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -215,8 +215,11 @@ 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. Defaults to `diff.submodule` or 'short' if the config
+ option is unset.
--color[=]::
Show colored diff.
diff --git a/diff.c b/diff.c
index e286897b51e6..232fbe17680f 100644
--- a/diff.c
+++ b/diff.c
@@ -132,9 +132,11 @@ 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"))
- DIFF_OPT_SET(options, SUBMODULE_LOG);
+ options->submodule_format = DIFF_SUBMODULE_LOG;
+ else if (!strcmp(value, "diff"))
+ options->submodule_format = DIFF_SUBMODULE_DIFF;
else if (!strcmp(value, "short"))
- DIFF_OPT_CLR(options, SUBMODULE_LOG);
+ options->submodule_format = DIFF_SUBMODULE_SHORT;
else
return -1;
return 0;
@@ -2300,9 +2302,18 @@ static void builtin_diff(const char *name_a,
struct strbuf header = STRBUF_INIT;
const char *line_prefix = diff_line_prefix(o);
- if (DIFF_OPT_TST(o, SUBMODULE_LOG) &&
- (!one->mode || S_ISGITLINK(one->mode)) &&
- (!two->mode || S_ISGITLINK(two->mode))) {
+ 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))) {
const char *del = diff_get_color_opt(o, DIFF_FILE_OLD);
const char *add = diff_get_color_opt(o, DIFF_FILE_NEW);
show_submodule_summary(o->file, one->path ? one->path :
two->path,
@@ -2311,6 +2322,15 @@ static void builtin_diff(const char *name_a,
two->dirty_submodule,
meta, del, add, reset);
return;
+ } else if (o->submodule_format == DIFF_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,
+
