Now you can see the submodule summaries inlined in the diff, instead of
not-quite-helpful SHA-1 pairs.
The format imitates what "git submodule summary" shows.
To do that, <path>/.git/objects/ is added to the alternate object
databases (if that directory exists).
This option was requested by Jens Lehmann at the GitTogether in Berlin.
Signed-off-by: Johannes Schindelin <redacted>
---
Documentation/diff-options.txt | 4 ++
Makefile | 2 +
diff.c | 14 +++++
diff.h | 1 +
submodule.c | 108 ++++++++++++++++++++++++++++++++++++++++
submodule.h | 8 +++
6 files changed, 137 insertions(+), 0 deletions(-)
create mode 100644 submodule.c
create mode 100644 submodule.h
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 9276fae..5fcc5a8 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -87,6 +87,10 @@ endif::git-format-patch[]
Show only names and status of changed files. See the description
of the `--diff-filter` option on what the status letters mean.
+--submodule-summary::
+ Instead of showing pairs of commit names, list the commits in that
+ commit range in the same style as linkgit:git-submodule[1].
+
--color::
Show colored diff.
diff --git a/Makefile b/Makefile
index 2c20922..56d4104 100644
--- a/Makefile
+++ b/Makefile
@@ -449,6 +449,7 @@ LIB_H += sideband.h
LIB_H += sigchain.h
LIB_H += strbuf.h
LIB_H += string-list.h
+LIB_H += submodule.h
LIB_H += tag.h
LIB_H += transport.h
LIB_H += tree.h
@@ -547,6 +548,7 @@ LIB_OBJS += sideband.o
LIB_OBJS += sigchain.o
LIB_OBJS += strbuf.o
LIB_OBJS += string-list.o
+LIB_OBJS += submodule.o
LIB_OBJS += symlinks.o
LIB_OBJS += tag.o
LIB_OBJS += trace.o
diff --git a/diff.c b/diff.c
index 9e00131..cdef322 100644
--- a/diff.c
+++ b/diff.c
@@ -13,6 +13,7 @@
#include "utf8.h"
#include "userdiff.h"
#include "sigchain.h"
+#include "submodule.h"
#ifdef NO_FAST_WORKING_DIRECTORY
#define FAST_WORKING_DIRECTORY 0
@@ -1557,6 +1558,17 @@ static void builtin_diff(const char *name_a,
const char *a_prefix, *b_prefix;
const char *textconv_one = NULL, *textconv_two = NULL;
+ if (DIFF_OPT_TST(o, SUMMARIZE_SUBMODULES) &&
+ (!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 ? one->path : two->path,
+ one->sha1, two->sha1,
+ del, add, reset);
+ return;
+ }
+
if (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {
textconv_one = get_textconv(one);
textconv_two = get_textconv(two);@@ -2771,6 +2783,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
DIFF_OPT_CLR(options, ALLOW_TEXTCONV);
else if (!strcmp(arg, "--ignore-submodules"))
DIFF_OPT_SET(options, IGNORE_SUBMODULES);
+ else if (!strcmp(arg, "--submodule-summary"))
+ DIFF_OPT_SET(options, SUMMARIZE_SUBMODULES);
/* misc options */
else if (!strcmp(arg, "-z"))
diff --git a/diff.h b/diff.h
index a7e7ccb..aa6976f 100644
--- a/diff.h
+++ b/diff.h
@@ -67,6 +67,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,
#define DIFF_OPT_DIRSTAT_BY_FILE (1 << 20)
#define DIFF_OPT_ALLOW_TEXTCONV (1 << 21)
#define DIFF_OPT_DIFF_FROM_CONTENTS (1 << 22)
+#define DIFF_OPT_SUMMARIZE_SUBMODULES (1 << 23)
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)
#define DIFF_OPT_CLR(opts, flag) ((opts)->flags &= ~DIFF_OPT_##flag)
diff --git a/submodule.c b/submodule.c
new file mode 100644
index 0000000..3f2590d
--- /dev/null
+++ b/submodule.c
@@ -0,0 +1,108 @@
+#include "cache.h"
+#include "submodule.h"
+#include "dir.h"
+#include "diff.h"
+#include "commit.h"
+#include "revision.h"
+
+int add_submodule_odb(const char *path)
+{
+ struct strbuf objects_directory = STRBUF_INIT;
+ struct alternate_object_database *alt_odb;
+
+ strbuf_addf(&objects_directory, "%s/.git/objects/", path);
+ if (!is_directory(objects_directory.buf))
+ return -1;
+
+ /* avoid adding it twice */
+ for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
+ if (alt_odb->name - alt_odb->base == objects_directory.len &&
+ !strncmp(alt_odb->base, objects_directory.buf,
+ objects_directory.len))
+ return 0;
+
+ alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+ alt_odb->next = alt_odb_list;
+ strcpy(alt_odb->base, objects_directory.buf);
+ alt_odb->name = alt_odb->base + objects_directory.len;
+ alt_odb->name[2] = '/';
+ alt_odb->name[40] = '\0';
+ alt_odb->name[41] = '\0';
+ alt_odb_list = alt_odb;
+ prepare_alt_odb();
+ return 0;
+}
+
+void show_submodule_summary(FILE *f, const char *path,
+ unsigned char one[20], unsigned char two[20],
+ const char *del, const char *add, const char *reset)
+{
+ struct rev_info rev;
+ struct commit *commit, *left, *right;
+ struct commit_list *merge_bases, *list;
+ const char *message = NULL;
+ struct strbuf sb = STRBUF_INIT;
+ static const char *format = " %m %s";
+ int fast_forward = 0, fast_backward = 0;
+
+ if (add_submodule_odb(path))
+ message = "(not checked out)";
+ else if (is_null_sha1(one))
+ message = "(new submodule)";
+ else if (is_null_sha1(two))
+ message = "(submodule deleted)";
+ else if (!(left = lookup_commit_reference(one)) ||
+ !(right = lookup_commit_reference(two)))
+ message = "(commits not present)";
+
+ if (!message) {
+ init_revisions(&rev, NULL);
+ setup_revisions(0, NULL, &rev, NULL);
+ rev.left_right = 1;
+ left->object.flags |= SYMMETRIC_LEFT;
+ add_pending_object(&rev, &left->object, path);
+ add_pending_object(&rev, &right->object, path);
+ merge_bases = get_merge_bases(left, right, 1);
+ if (merge_bases) {
+ if (merge_bases->item == left)
+ fast_forward = 1;
+ else if (merge_bases->item == right)
+ fast_backward = 1;
+ }
+ for (list = merge_bases; list; list = list->next) {
+ list->item->object.flags |= UNINTERESTING;
+ add_pending_object(&rev, &list->item->object,
+ sha1_to_hex(list->item->object.sha1));
+ }
+ if (prepare_revision_walk(&rev))
+ message = "(revision walker failed)";
+ }
+
+ strbuf_addf(&sb, "Submodule %s %s..", path,
+ find_unique_abbrev(one, DEFAULT_ABBREV));
+ if (!fast_backward && !fast_forward)
+ strbuf_addch(&sb, '.');
+ strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
+ if (message)
+ strbuf_addf(&sb, " %s\n", message);
+ else
+ strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
+ fwrite(sb.buf, sb.len, 1, f);
+
+ if (!message) {
+ while ((commit = get_revision(&rev))) {
+ strbuf_setlen(&sb, 0);
+ if (del)
+ strbuf_addstr(&sb, commit->object.flags &
+ SYMMETRIC_LEFT ? del : add);
+ format_commit_message(commit, format, &sb,
+ rev.date_mode);
+ if (del)
+ strbuf_addstr(&sb, reset);
+ strbuf_addch(&sb, '\n');
+ fwrite(sb.buf, sb.len, 1, f);
+ }
+ clear_commit_marks(left, ~0);
+ clear_commit_marks(right, ~0);
+ }
+}diff --git a/submodule.h b/submodule.h
new file mode 100644
index 0000000..4c0269d
--- /dev/null
+++ b/submodule.h
@@ -0,0 +1,8 @@
+#ifndef SUBMODULE_H
+#define SUBMODULE_H
+
+void show_submodule_summary(FILE *f, const char *path,
+ unsigned char one[20], unsigned char two[20],
+ const char *del, const char *add, const char *reset);
+
+#endif
--
1.6.4.313.g3d9e3
Johannes Schindelin [off-list ref] writes:
Now you can see the submodule summaries inlined in the diff, instead of
not-quite-helpful SHA-1 pairs.
This looks useful, but I do not think this is --summary. It is not part
of the "summary" output but is about making output for Submoodules more
verbose. I'd suggest naming it --verbose-submodule or something.
The format imitates what "git submodule summary" shows.
The output format needs to be described better here and also in
Documentation/diff-format.txt.
To do that, <path>/.git/objects/ is added to the alternate object
databases (if that directory exists).
Is it always true that <path>/.git is the GIT_DIR for that submodule? Not
complaining but checking sanity.
quoted hunk
diff --git a/diff.c b/diff.c
index 9e00131..cdef322 100644
--- a/diff.c
+++ b/diff.c
@@ -1557,6 +1558,17 @@ static void builtin_diff(const char *name_a,
const char *a_prefix, *b_prefix;
const char *textconv_one = NULL, *textconv_two = NULL;
+ if (DIFF_OPT_TST(o, SUMMARIZE_SUBMODULES) &&
+ (!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 ? one->path : two->path,
+ one->sha1, two->sha1,
+ del, add, reset);
+ return;
+ }
+
Isn't this "textual diff" codepath?
I would have expected --submodule-summary output to be near the --summary
output and to be generated in the same codepath to generate it; if this
were named --verbose-submodule, I would certainly understand it, and I
think the placement is saner in the textual diff.
quoted hunk
diff --git a/submodule.c b/submodule.c
new file mode 100644
index 0000000..3f2590d
--- /dev/null
+++ b/submodule.c
@@ -0,0 +1,108 @@
+#include "cache.h"
+#include "submodule.h"
+#include "dir.h"
+#include "diff.h"
+#include "commit.h"
+#include "revision.h"
+
+int add_submodule_odb(const char *path)
+{
+ struct strbuf objects_directory = STRBUF_INIT;
+ struct alternate_object_database *alt_odb;
+
+ strbuf_addf(&objects_directory, "%s/.git/objects/", path);
+ if (!is_directory(objects_directory.buf))
+ return -1;
+
+ /* avoid adding it twice */
+ for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
+ if (alt_odb->name - alt_odb->base == objects_directory.len &&
+ !strncmp(alt_odb->base, objects_directory.buf,
+ objects_directory.len))
+ return 0;
+ alt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));
+ alt_odb->next = alt_odb_list;
+ strcpy(alt_odb->base, objects_directory.buf);
+ alt_odb->name = alt_odb->base + objects_directory.len;
+ alt_odb->name[2] = '/';
+ alt_odb->name[40] = '\0';
+ alt_odb->name[41] = '\0';
+ alt_odb_list = alt_odb;
+ prepare_alt_odb();
The lines after "avoid adding it twice" look somewhat familiar.
Don't we already have other codepaths to add an alternate odb that need to
be careful in the same way (i.e. do not add twice, do not loop, etc.), and
if so don't you want to reuse it after refactoring?
+void show_submodule_summary(FILE *f, const char *path,
+ unsigned char one[20], unsigned char two[20],
+ const char *del, const char *add, const char *reset)
+{
+ struct rev_info rev;
+ struct commit *commit, *left, *right;
+ struct commit_list *merge_bases, *list;
+ const char *message = NULL;
+ struct strbuf sb = STRBUF_INIT;
+ static const char *format = " %m %s";
+ int fast_forward = 0, fast_backward = 0;
+
+ if (add_submodule_odb(path))
+ message = "(not checked out)";
+ else if (is_null_sha1(one))
+ message = "(new submodule)";
+ else if (is_null_sha1(two))
+ message = "(submodule deleted)";
Are you sure about this? Wouldn't "git diff HEAD" (not looking at the
index) give you the 0{40} on the "two" side when the path is modified?
+ if (!message) {
+ init_revisions(&rev, NULL);
+ setup_revisions(0, NULL, &rev, NULL);
+ rev.left_right = 1;
+ left->object.flags |= SYMMETRIC_LEFT;
+ add_pending_object(&rev, &left->object, path);
+ add_pending_object(&rev, &right->object, path);
+ merge_bases = get_merge_bases(left, right, 1);
+ if (merge_bases) {
+ if (merge_bases->item == left)
+ fast_forward = 1;
+ else if (merge_bases->item == right)
+ fast_backward = 1;
+ }
+ for (list = merge_bases; list; list = list->next) {
+ list->item->object.flags |= UNINTERESTING;
+ add_pending_object(&rev, &list->item->object,
+ sha1_to_hex(list->item->object.sha1));
+ }
+ if (prepare_revision_walk(&rev))
+ message = "(revision walker failed)";
If prepare_revision_walk() failed for whatever reason, can we trust
fast_forward/fast_backward at this point?
+ }
+
+ strbuf_addf(&sb, "Submodule %s %s..", path,
+ find_unique_abbrev(one, DEFAULT_ABBREV));
+ if (!fast_backward && !fast_forward)
+ strbuf_addch(&sb, '.');
+ strbuf_addf(&sb, "%s", find_unique_abbrev(two, DEFAULT_ABBREV));
+ if (message)
+ strbuf_addf(&sb, " %s\n", message);
+ else
+ strbuf_addf(&sb, "%s:\n", fast_backward ? " (rewind)" : "");
No corresponding "(fast-forward)" label?
+ fwrite(sb.buf, sb.len, 1, f);
+
+ if (!message) {
+ while ((commit = get_revision(&rev))) {
+ strbuf_setlen(&sb, 0);
+ if (del)
+ strbuf_addstr(&sb, commit->object.flags &
+ SYMMETRIC_LEFT ? del : add);
+ format_commit_message(commit, format, &sb,
+ rev.date_mode);
+ if (del)
+ strbuf_addstr(&sb, reset);
Three points, two of which are minor.
- Checking "del" to decide if you want to say "reset" feels funny.
- In the "ANSI-terminal only" world view, adding colors to strbuf and
writing it out together with the actual strings is an easy thing to do.
Don't Windows folks have trouble converting this kind of code to their
color control call that is separate from writing strings out? If it is
not a problem, I do not have any objection to it, but otherwise I'd
suggest not to add any more code that stores color escape sequence in
strbuf, so that we would not make later conversion by Windows folks
harder than necessary.
- The output is similar but different from "submodule --summary" and
there is no justification described in the patch nor the proposed
commit log message.
o Why does "submodule --summary" use --first-parent and this patch
doesn't?
o "submodule --summary" allows users to limit the walk but this seems
to give the full list---is it useful (or even sane) to always give
a full list?
+ strbuf_addch(&sb, '\n');
+ fwrite(sb.buf, sb.len, 1, f);
+ }
+ clear_commit_marks(left, ~0);
+ clear_commit_marks(right, ~0);
+ }
Aren't object flags shared between the outer revision walker that walks
the log and this new walker? If the supermodule history and submodule
history share commits (e.g. when later git.git starts using submodules to
bind gitk and git-gui), wouldn't this break "git log -p" a big way?
In any case, thanks for an interesting patch. Looking forward to see how
this topic evolves.
Junio C Hamano schrieb:
Johannes Schindelin [off-list ref] writes:
quoted
+ fwrite(sb.buf, sb.len, 1, f);
+
+ if (!message) {
+ while ((commit = get_revision(&rev))) {
+ strbuf_setlen(&sb, 0);
+ if (del)
+ strbuf_addstr(&sb, commit->object.flags &
+ SYMMETRIC_LEFT ? del : add);
+ format_commit_message(commit, format, &sb,
+ rev.date_mode);
+ if (del)
+ strbuf_addstr(&sb, reset);
- In the "ANSI-terminal only" world view, adding colors to strbuf and
writing it out together with the actual strings is an easy thing to do.
Don't Windows folks have trouble converting this kind of code to their
color control call that is separate from writing strings out? If it is
not a problem, I do not have any objection to it, but otherwise I'd
suggest not to add any more code that stores color escape sequence in
strbuf, so that we would not make later conversion by Windows folks
harder than necessary.
Thanks for noticing this! To store color escapes in strbuf is not a
problem as long as the string is finally written using printf, fprintf, or
fputs.
quoted
+ strbuf_addch(&sb, '\n');
+ fwrite(sb.buf, sb.len, 1, f);
Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
easy to change such that it uses fprintf().
-- Hannes
Hi,
On Mon, 5 Oct 2009, Johannes Sixt wrote:
Junio C Hamano schrieb:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
+ fwrite(sb.buf, sb.len, 1, f);
+
+ if (!message) {
+ while ((commit = get_revision(&rev))) {
+ strbuf_setlen(&sb, 0);
+ if (del)
+ strbuf_addstr(&sb, commit->object.flags &
+ SYMMETRIC_LEFT ? del : add);
+ format_commit_message(commit, format, &sb,
+ rev.date_mode);
+ if (del)
+ strbuf_addstr(&sb, reset);
- In the "ANSI-terminal only" world view, adding colors to strbuf and
writing it out together with the actual strings is an easy thing to do.
Don't Windows folks have trouble converting this kind of code to their
color control call that is separate from writing strings out? If it is
not a problem, I do not have any objection to it, but otherwise I'd
suggest not to add any more code that stores color escape sequence in
strbuf, so that we would not make later conversion by Windows folks
harder than necessary.
Thanks for noticing this! To store color escapes in strbuf is not a
problem as long as the string is finally written using printf, fprintf, or
fputs.
quoted
quoted
+ strbuf_addch(&sb, '\n');
+ fwrite(sb.buf, sb.len, 1, f);
Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
easy to change such that it uses fprintf().
Good point. I changed it to
fprintf(f, "%s", sb.buf);
BTW we probably need to remove the "TODO: write" from compat/winansi.c...
Ciao,
Dscho
Johannes Schindelin schrieb:
On Mon, 5 Oct 2009, Johannes Sixt wrote:
quoted
Junio C Hamano schrieb:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
+ fwrite(sb.buf, sb.len, 1, f);
+
+ if (!message) {
+ while ((commit = get_revision(&rev))) {
+ strbuf_setlen(&sb, 0);
+ if (del)
+ strbuf_addstr(&sb, commit->object.flags &
+ SYMMETRIC_LEFT ? del : add);
+ format_commit_message(commit, format, &sb,
+ rev.date_mode);
+ if (del)
+ strbuf_addstr(&sb, reset);
+ strbuf_addch(&sb, '\n');
+ fwrite(sb.buf, sb.len, 1, f);
Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
easy to change such that it uses fprintf().
Good point. I changed it to
fprintf(f, "%s", sb.buf);
Thanks. But notice how you are constructing the string in sb from pieces.
I meant to change it to
fprintf(f, "%s%s%s\n",
del ? (commit->object.flags & SYMMETRIC_LEFT
? del : add) : "",
format_commit_message(commit, format, &sb,
rev.date_mode),
del ? reset : "");
or similar. We already use this idiom elsewhere.
-- Hannes
Hi,
On Mon, 5 Oct 2009, Johannes Sixt wrote:
Johannes Schindelin schrieb:
quoted
On Mon, 5 Oct 2009, Johannes Sixt wrote:
quoted
Junio C Hamano schrieb:
quoted
Johannes Schindelin [off-list ref] writes:
quoted
+ fwrite(sb.buf, sb.len, 1, f);
+
+ if (!message) {
+ while ((commit = get_revision(&rev))) {
+ strbuf_setlen(&sb, 0);
+ if (del)
+ strbuf_addstr(&sb, commit->object.flags &
+ SYMMETRIC_LEFT ? del : add);
+ format_commit_message(commit, format, &sb,
+ rev.date_mode);
+ if (del)
+ strbuf_addstr(&sb, reset);
+ strbuf_addch(&sb, '\n');
+ fwrite(sb.buf, sb.len, 1, f);
Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is
easy to change such that it uses fprintf().
Good point. I changed it to
fprintf(f, "%s", sb.buf);
Thanks. But notice how you are constructing the string in sb from pieces.
I meant to change it to
fprintf(f, "%s%s%s\n",
del ? (commit->object.flags & SYMMETRIC_LEFT
? del : add) : "",
format_commit_message(commit, format, &sb,
rev.date_mode),
del ? reset : "");
or similar. We already use this idiom elsewhere.
And I find it utterly ugly and unreadable there, too. So this is why I did
not do it.
Ciao,
Dscho