{"thread":{"id":"21129","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","startedAt":"2009-10-04T15:05:31Z","lastAt":"2009-10-07T22:28:14Z","messageCount":20,"participants":["Junio C Hamano","Johannes Sixt","Johannes Schindelin","Jens Lehmann"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"299137","messageId":"67a884457aeaead275612be10902a80726b2a7db.1254668669u.git.johannes.schindelin@gmx.de","threadId":"21129","inReplyTo":"cover.1254668669u.git.johannes.schindelin@gmx.de","subject":"[PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-04T15:05:31Z","receivedAt":"2009-10-04T15:05:31Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Now you can see the submodule summaries inlined in the diff, instead of\nnot-quite-helpful SHA-1 pairs.\n\nThe format imitates what \"git submodule summary\" shows.\n\nTo do that, <path>/.git/objects/ is added to the alternate object\ndatabases (if that directory exists).\n\nThis option was requested by Jens Lehmann at the GitTogether in Berlin.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n Documentation/diff-options.txt |    4 ++\n Makefile                       |    2 +\n diff.c                         |   14 +++++\n diff.h                         |    1 +\n submodule.c                    |  108 ++++++++++++++++++++++++++++++++++++++++\n submodule.h                    |    8 +++\n 6 files changed, 137 insertions(+), 0 deletions(-)\n create mode 100644 submodule.c\n create mode 100644 submodule.h\n\ndiff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt\nindex 9276fae..5fcc5a8 100644\n--- a/Documentation/diff-options.txt\n+++ b/Documentation/diff-options.txt\n@@ -87,6 +87,10 @@ endif::git-format-patch[]\n \tShow only names and status of changed files. See the description\n \tof the `--diff-filter` option on what the status letters mean.\n \n+--submodule-summary::\n+\tInstead of showing pairs of commit names, list the commits in that\n+\tcommit range in the same style as linkgit:git-submodule[1].\n+\n --color::\n \tShow colored diff.\n \ndiff --git a/Makefile b/Makefile\nindex 2c20922..56d4104 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -449,6 +449,7 @@ LIB_H += sideband.h\n LIB_H += sigchain.h\n LIB_H += strbuf.h\n LIB_H += string-list.h\n+LIB_H += submodule.h\n LIB_H += tag.h\n LIB_H += transport.h\n LIB_H += tree.h\n@@ -547,6 +548,7 @@ LIB_OBJS += sideband.o\n LIB_OBJS += sigchain.o\n LIB_OBJS += strbuf.o\n LIB_OBJS += string-list.o\n+LIB_OBJS += submodule.o\n LIB_OBJS += symlinks.o\n LIB_OBJS += tag.o\n LIB_OBJS += trace.o\ndiff --git a/diff.c b/diff.c\nindex 9e00131..cdef322 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -13,6 +13,7 @@\n #include \"utf8.h\"\n #include \"userdiff.h\"\n #include \"sigchain.h\"\n+#include \"submodule.h\"\n \n #ifdef NO_FAST_WORKING_DIRECTORY\n #define FAST_WORKING_DIRECTORY 0\n@@ -1557,6 +1558,17 @@ static void builtin_diff(const char *name_a,\n \tconst char *a_prefix, *b_prefix;\n \tconst char *textconv_one = NULL, *textconv_two = NULL;\n \n+\tif (DIFF_OPT_TST(o, SUMMARIZE_SUBMODULES) &&\n+\t\t\t(!one->mode || S_ISGITLINK(one->mode)) &&\n+\t\t\t(!two->mode || S_ISGITLINK(two->mode))) {\n+\t\tconst char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n+\t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n+\t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n+\t\t\t\tone->sha1, two->sha1,\n+\t\t\t\tdel, add, reset);\n+\t\treturn;\n+\t}\n+\n \tif (DIFF_OPT_TST(o, ALLOW_TEXTCONV)) {\n \t\ttextconv_one = get_textconv(one);\n \t\ttextconv_two = get_textconv(two);\n@@ -2771,6 +2783,8 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)\n \t\tDIFF_OPT_CLR(options, ALLOW_TEXTCONV);\n \telse if (!strcmp(arg, \"--ignore-submodules\"))\n \t\tDIFF_OPT_SET(options, IGNORE_SUBMODULES);\n+\telse if (!strcmp(arg, \"--submodule-summary\"))\n+\t\tDIFF_OPT_SET(options, SUMMARIZE_SUBMODULES);\n \n \t/* misc options */\n \telse if (!strcmp(arg, \"-z\"))\ndiff --git a/diff.h b/diff.h\nindex a7e7ccb..aa6976f 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -67,6 +67,7 @@ typedef void (*diff_format_fn_t)(struct diff_queue_struct *q,\n #define DIFF_OPT_DIRSTAT_BY_FILE     (1 << 20)\n #define DIFF_OPT_ALLOW_TEXTCONV      (1 << 21)\n #define DIFF_OPT_DIFF_FROM_CONTENTS  (1 << 22)\n+#define DIFF_OPT_SUMMARIZE_SUBMODULES (1 << 23)\n #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)\n #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)\n #define DIFF_OPT_CLR(opts, flag)    ((opts)->flags &= ~DIFF_OPT_##flag)\ndiff --git a/submodule.c b/submodule.c\nnew file mode 100644\nindex 0000000..3f2590d\n--- /dev/null\n+++ b/submodule.c\n@@ -0,0 +1,108 @@\n+#include \"cache.h\"\n+#include \"submodule.h\"\n+#include \"dir.h\"\n+#include \"diff.h\"\n+#include \"commit.h\"\n+#include \"revision.h\"\n+\n+int add_submodule_odb(const char *path)\n+{\n+\tstruct strbuf objects_directory = STRBUF_INIT;\n+\tstruct alternate_object_database *alt_odb;\n+\n+\tstrbuf_addf(&objects_directory, \"%s/.git/objects/\", path);\n+\tif (!is_directory(objects_directory.buf))\n+\t\treturn -1;\n+\n+\t/* avoid adding it twice */\n+\tfor (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n+\t\tif (alt_odb->name - alt_odb->base == objects_directory.len &&\n+\t\t\t\t!strncmp(alt_odb->base, objects_directory.buf,\n+\t\t\t\t\tobjects_directory.len))\n+\t\t\treturn 0;\n+\n+\talt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));\n+\talt_odb->next = alt_odb_list;\n+\tstrcpy(alt_odb->base, objects_directory.buf);\n+\talt_odb->name = alt_odb->base + objects_directory.len;\n+\talt_odb->name[2] = '/';\n+\talt_odb->name[40] = '\\0';\n+\talt_odb->name[41] = '\\0';\n+\talt_odb_list = alt_odb;\n+\tprepare_alt_odb();\n+\treturn 0;\n+}\n+\n+void show_submodule_summary(FILE *f, const char *path,\n+\t\tunsigned char one[20], unsigned char two[20],\n+\t\tconst char *del, const char *add, const char *reset)\n+{\n+\tstruct rev_info rev;\n+\tstruct commit *commit, *left, *right;\n+\tstruct commit_list *merge_bases, *list;\n+\tconst char *message = NULL;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstatic const char *format = \"    %m %s\";\n+\tint fast_forward = 0, fast_backward = 0;\n+\n+\tif (add_submodule_odb(path))\n+\t\tmessage = \"(not checked out)\";\n+\telse if (is_null_sha1(one))\n+\t\tmessage = \"(new submodule)\";\n+\telse if (is_null_sha1(two))\n+\t\tmessage = \"(submodule deleted)\";\n+\telse if (!(left = lookup_commit_reference(one)) ||\n+\t\t\t!(right = lookup_commit_reference(two)))\n+\t\tmessage = \"(commits not present)\";\n+\n+\tif (!message) {\n+\t\tinit_revisions(&rev, NULL);\n+\t\tsetup_revisions(0, NULL, &rev, NULL);\n+\t\trev.left_right = 1;\n+\t\tleft->object.flags |= SYMMETRIC_LEFT;\n+\t\tadd_pending_object(&rev, &left->object, path);\n+\t\tadd_pending_object(&rev, &right->object, path);\n+\t\tmerge_bases = get_merge_bases(left, right, 1);\n+\t\tif (merge_bases) {\n+\t\t\tif (merge_bases->item == left)\n+\t\t\t\tfast_forward = 1;\n+\t\t\telse if (merge_bases->item == right)\n+\t\t\t\tfast_backward = 1;\n+\t\t}\n+\t\tfor (list = merge_bases; list; list = list->next) {\n+\t\t\tlist->item->object.flags |= UNINTERESTING;\n+\t\t\tadd_pending_object(&rev, &list->item->object,\n+\t\t\t\tsha1_to_hex(list->item->object.sha1));\n+\t\t}\n+\t\tif (prepare_revision_walk(&rev))\n+\t\t\tmessage = \"(revision walker failed)\";\n+\t}\n+\n+\tstrbuf_addf(&sb, \"Submodule %s %s..\", path,\n+\t\t\tfind_unique_abbrev(one, DEFAULT_ABBREV));\n+\tif (!fast_backward && !fast_forward)\n+\t\tstrbuf_addch(&sb, '.');\n+\tstrbuf_addf(&sb, \"%s\", find_unique_abbrev(two, DEFAULT_ABBREV));\n+\tif (message)\n+\t\tstrbuf_addf(&sb, \" %s\\n\", message);\n+\telse\n+\t\tstrbuf_addf(&sb, \"%s:\\n\", fast_backward ? \" (rewind)\" : \"\");\n+\tfwrite(sb.buf, sb.len, 1, f);\n+\n+\tif (!message) {\n+\t\twhile ((commit = get_revision(&rev))) {\n+\t\t\tstrbuf_setlen(&sb, 0);\n+\t\t\tif (del)\n+\t\t\t\tstrbuf_addstr(&sb, commit->object.flags &\n+\t\t\t\t\t\tSYMMETRIC_LEFT ? del : add);\n+\t\t\tformat_commit_message(commit, format, &sb,\n+\t\t\t\t\trev.date_mode);\n+\t\t\tif (del)\n+\t\t\t\tstrbuf_addstr(&sb, reset);\n+\t\t\tstrbuf_addch(&sb, '\\n');\n+\t\t\tfwrite(sb.buf, sb.len, 1, f);\n+\t\t}\n+\t\tclear_commit_marks(left, ~0);\n+\t\tclear_commit_marks(right, ~0);\n+\t}\n+}\ndiff --git a/submodule.h b/submodule.h\nnew file mode 100644\nindex 0000000..4c0269d\n--- /dev/null\n+++ b/submodule.h\n@@ -0,0 +1,8 @@\n+#ifndef SUBMODULE_H\n+#define SUBMODULE_H\n+\n+void show_submodule_summary(FILE *f, const char *path,\n+\t\tunsigned char one[20], unsigned char two[20],\n+\t\tconst char *del, const char *add, const char *reset);\n+\n+#endif\n-- \n1.6.4.313.g3d9e3\n"},{"id":"124209","messageId":"7vbpkmn6oi.fsf@alter.siamese.dyndns.org","threadId":"21129","inReplyTo":"67a884457aeaead275612be10902a80726b2a7db.1254668669u.git.johannes.schindelin@gmx.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-04T22:19:25Z","receivedAt":"2009-10-04T22:19:25Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n\n> Now you can see the submodule summaries inlined in the diff, instead of\n> not-quite-helpful SHA-1 pairs.\n\nThis looks useful, but I do not think this is --summary.  It is not part\nof the \"summary\" output but is about making output for Submoodules more\nverbose.  I'd suggest naming it --verbose-submodule or something.\n\n> The format imitates what \"git submodule summary\" shows.\n\nThe output format needs to be described better here and also in\nDocumentation/diff-format.txt.\n\n> To do that, <path>/.git/objects/ is added to the alternate object\n> databases (if that directory exists).\n\nIs it always true that <path>/.git is the GIT_DIR for that submodule?  Not\ncomplaining but checking sanity.\n\n> diff --git a/diff.c b/diff.c\n> index 9e00131..cdef322 100644\n> --- a/diff.c\n> +++ b/diff.c\n> @@ -1557,6 +1558,17 @@ static void builtin_diff(const char *name_a,\n>  \tconst char *a_prefix, *b_prefix;\n>  \tconst char *textconv_one = NULL, *textconv_two = NULL;\n>  \n> +\tif (DIFF_OPT_TST(o, SUMMARIZE_SUBMODULES) &&\n> +\t\t\t(!one->mode || S_ISGITLINK(one->mode)) &&\n> +\t\t\t(!two->mode || S_ISGITLINK(two->mode))) {\n> +\t\tconst char *del = diff_get_color_opt(o, DIFF_FILE_OLD);\n> +\t\tconst char *add = diff_get_color_opt(o, DIFF_FILE_NEW);\n> +\t\tshow_submodule_summary(o->file, one ? one->path : two->path,\n> +\t\t\t\tone->sha1, two->sha1,\n> +\t\t\t\tdel, add, reset);\n> +\t\treturn;\n> +\t}\n> +\n\nIsn't this \"textual diff\" codepath?\n\nI would have expected --submodule-summary output to be near the --summary\noutput and to be generated in the same codepath to generate it; if this\nwere named --verbose-submodule, I would certainly understand it, and I\nthink the placement is saner in the textual diff.\n\n> diff --git a/submodule.c b/submodule.c\n> new file mode 100644\n> index 0000000..3f2590d\n> --- /dev/null\n> +++ b/submodule.c\n> @@ -0,0 +1,108 @@\n> +#include \"cache.h\"\n> +#include \"submodule.h\"\n> +#include \"dir.h\"\n> +#include \"diff.h\"\n> +#include \"commit.h\"\n> +#include \"revision.h\"\n> +\n> +int add_submodule_odb(const char *path)\n> +{\n> +\tstruct strbuf objects_directory = STRBUF_INIT;\n> +\tstruct alternate_object_database *alt_odb;\n> +\n> +\tstrbuf_addf(&objects_directory, \"%s/.git/objects/\", path);\n> +\tif (!is_directory(objects_directory.buf))\n> +\t\treturn -1;\n> +\n> +\t/* avoid adding it twice */\n> +\tfor (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)\n> +\t\tif (alt_odb->name - alt_odb->base == objects_directory.len &&\n> +\t\t\t\t!strncmp(alt_odb->base, objects_directory.buf,\n> +\t\t\t\t\tobjects_directory.len))\n> +\t\t\treturn 0;\n> +\talt_odb = xmalloc(objects_directory.len + 42 + sizeof(*alt_odb));\n> +\talt_odb->next = alt_odb_list;\n> +\tstrcpy(alt_odb->base, objects_directory.buf);\n> +\talt_odb->name = alt_odb->base + objects_directory.len;\n> +\talt_odb->name[2] = '/';\n> +\talt_odb->name[40] = '\\0';\n> +\talt_odb->name[41] = '\\0';\n> +\talt_odb_list = alt_odb;\n> +\tprepare_alt_odb();\n\nThe lines after \"avoid adding it twice\" look somewhat familiar.\n\nDon't we already have other codepaths to add an alternate odb that need to\nbe careful in the same way (i.e. do not add twice, do not loop, etc.), and\nif so don't you want to reuse it after refactoring?\n\n> +void show_submodule_summary(FILE *f, const char *path,\n> +\t\tunsigned char one[20], unsigned char two[20],\n> +\t\tconst char *del, const char *add, const char *reset)\n> +{\n> +\tstruct rev_info rev;\n> +\tstruct commit *commit, *left, *right;\n> +\tstruct commit_list *merge_bases, *list;\n> +\tconst char *message = NULL;\n> +\tstruct strbuf sb = STRBUF_INIT;\n> +\tstatic const char *format = \"    %m %s\";\n> +\tint fast_forward = 0, fast_backward = 0;\n> +\n> +\tif (add_submodule_odb(path))\n> +\t\tmessage = \"(not checked out)\";\n> +\telse if (is_null_sha1(one))\n> +\t\tmessage = \"(new submodule)\";\n> +\telse if (is_null_sha1(two))\n> +\t\tmessage = \"(submodule deleted)\";\n\nAre you sure about this?  Wouldn't \"git diff HEAD\" (not looking at the\nindex) give you the 0{40} on the \"two\" side when the path is modified?\n\n> +\tif (!message) {\n> +\t\tinit_revisions(&rev, NULL);\n> +\t\tsetup_revisions(0, NULL, &rev, NULL);\n> +\t\trev.left_right = 1;\n> +\t\tleft->object.flags |= SYMMETRIC_LEFT;\n> +\t\tadd_pending_object(&rev, &left->object, path);\n> +\t\tadd_pending_object(&rev, &right->object, path);\n> +\t\tmerge_bases = get_merge_bases(left, right, 1);\n> +\t\tif (merge_bases) {\n> +\t\t\tif (merge_bases->item == left)\n> +\t\t\t\tfast_forward = 1;\n> +\t\t\telse if (merge_bases->item == right)\n> +\t\t\t\tfast_backward = 1;\n> +\t\t}\n> +\t\tfor (list = merge_bases; list; list = list->next) {\n> +\t\t\tlist->item->object.flags |= UNINTERESTING;\n> +\t\t\tadd_pending_object(&rev, &list->item->object,\n> +\t\t\t\tsha1_to_hex(list->item->object.sha1));\n> +\t\t}\n> +\t\tif (prepare_revision_walk(&rev))\n> +\t\t\tmessage = \"(revision walker failed)\";\n\nIf prepare_revision_walk() failed for whatever reason, can we trust\nfast_forward/fast_backward at this point?\n\n> +\t}\n> +\n> +\tstrbuf_addf(&sb, \"Submodule %s %s..\", path,\n> +\t\t\tfind_unique_abbrev(one, DEFAULT_ABBREV));\n> +\tif (!fast_backward && !fast_forward)\n> +\t\tstrbuf_addch(&sb, '.');\n> +\tstrbuf_addf(&sb, \"%s\", find_unique_abbrev(two, DEFAULT_ABBREV));\n> +\tif (message)\n> +\t\tstrbuf_addf(&sb, \" %s\\n\", message);\n> +\telse\n> +\t\tstrbuf_addf(&sb, \"%s:\\n\", fast_backward ? \" (rewind)\" : \"\");\n\nNo corresponding \"(fast-forward)\" label?\n\n> +\tfwrite(sb.buf, sb.len, 1, f);\n> +\n> +\tif (!message) {\n> +\t\twhile ((commit = get_revision(&rev))) {\n> +\t\t\tstrbuf_setlen(&sb, 0);\n> +\t\t\tif (del)\n> +\t\t\t\tstrbuf_addstr(&sb, commit->object.flags &\n> +\t\t\t\t\t\tSYMMETRIC_LEFT ? del : add);\n> +\t\t\tformat_commit_message(commit, format, &sb,\n> +\t\t\t\t\trev.date_mode);\n> +\t\t\tif (del)\n> +\t\t\t\tstrbuf_addstr(&sb, reset);\n\nThree points, two of which are minor.\n\n - Checking \"del\" to decide if you want to say \"reset\" feels funny.\n\n - In the \"ANSI-terminal only\" world view, adding colors to strbuf and\n   writing it out together with the actual strings is an easy thing to do.\n   Don't Windows folks have trouble converting this kind of code to their\n   color control call that is separate from writing strings out?  If it is\n   not a problem, I do not have any objection to it, but otherwise I'd\n   suggest not to add any more code that stores color escape sequence in\n   strbuf, so that we would not make later conversion by Windows folks\n   harder than necessary.\n\n - The output is similar but different from \"submodule --summary\" and\n   there is no justification described in the patch nor the proposed\n   commit log message.\n\n   o Why does \"submodule --summary\" use --first-parent and this patch\n     doesn't?\n\n   o \"submodule --summary\" allows users to limit the walk but this seems\n     to give the full list---is it useful (or even sane) to always give\n     a full list?\n\n> +\t\t\tstrbuf_addch(&sb, '\\n');\n> +\t\t\tfwrite(sb.buf, sb.len, 1, f);\n> +\t\t}\n> +\t\tclear_commit_marks(left, ~0);\n> +\t\tclear_commit_marks(right, ~0);\n> +\t}\n\nAren't object flags shared between the outer revision walker that walks\nthe log and this new walker?  If the supermodule history and submodule\nhistory share commits (e.g. when later git.git starts using submodules to\nbind gitk and git-gui), wouldn't this break \"git log -p\" a big way?\n\nIn any case, thanks for an interesting patch.  Looking forward to see how\nthis topic evolves.\n"},{"id":"124220","messageId":"4AC98FC8.3090202@viscovery.net","threadId":"21129","inReplyTo":"7vbpkmn6oi.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-10-05T06:18:48Z","receivedAt":"2009-10-05T06:18:48Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Junio C Hamano schrieb:\n> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>> +\tfwrite(sb.buf, sb.len, 1, f);\n>> +\n>> +\tif (!message) {\n>> +\t\twhile ((commit = get_revision(&rev))) {\n>> +\t\t\tstrbuf_setlen(&sb, 0);\n>> +\t\t\tif (del)\n>> +\t\t\t\tstrbuf_addstr(&sb, commit->object.flags &\n>> +\t\t\t\t\t\tSYMMETRIC_LEFT ? del : add);\n>> +\t\t\tformat_commit_message(commit, format, &sb,\n>> +\t\t\t\t\trev.date_mode);\n>> +\t\t\tif (del)\n>> +\t\t\t\tstrbuf_addstr(&sb, reset);\n> \n>  - In the \"ANSI-terminal only\" world view, adding colors to strbuf and\n>    writing it out together with the actual strings is an easy thing to do.\n>    Don't Windows folks have trouble converting this kind of code to their\n>    color control call that is separate from writing strings out?  If it is\n>    not a problem, I do not have any objection to it, but otherwise I'd\n>    suggest not to add any more code that stores color escape sequence in\n>    strbuf, so that we would not make later conversion by Windows folks\n>    harder than necessary.\n\nThanks for noticing this! To store color escapes in strbuf is not a\nproblem as long as the string is finally written using printf, fprintf, or\nfputs.\n\n>> +\t\t\tstrbuf_addch(&sb, '\\n');\n>> +\t\t\tfwrite(sb.buf, sb.len, 1, f);\n\nOutch! fwrite doesn't interpret color escapes. AFAICS, this sequence is\neasy to change such that it uses fprintf().\n\n-- Hannes\n"},{"id":"124227","messageId":"alpine.DEB.1.00.0910051057350.4985@pacific.mpi-cbg.de","threadId":"21129","inReplyTo":"4AC98FC8.3090202@viscovery.net","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-05T09:00:38Z","receivedAt":"2009-10-05T09:00:38Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Oct 2009, Johannes Sixt wrote:\n\n> Junio C Hamano schrieb:\n> > Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> >> +\tfwrite(sb.buf, sb.len, 1, f);\n> >> +\n> >> +\tif (!message) {\n> >> +\t\twhile ((commit = get_revision(&rev))) {\n> >> +\t\t\tstrbuf_setlen(&sb, 0);\n> >> +\t\t\tif (del)\n> >> +\t\t\t\tstrbuf_addstr(&sb, commit->object.flags &\n> >> +\t\t\t\t\t\tSYMMETRIC_LEFT ? del : add);\n> >> +\t\t\tformat_commit_message(commit, format, &sb,\n> >> +\t\t\t\t\trev.date_mode);\n> >> +\t\t\tif (del)\n> >> +\t\t\t\tstrbuf_addstr(&sb, reset);\n> > \n> >  - In the \"ANSI-terminal only\" world view, adding colors to strbuf and\n> >    writing it out together with the actual strings is an easy thing to do.\n> >    Don't Windows folks have trouble converting this kind of code to their\n> >    color control call that is separate from writing strings out?  If it is\n> >    not a problem, I do not have any objection to it, but otherwise I'd\n> >    suggest not to add any more code that stores color escape sequence in\n> >    strbuf, so that we would not make later conversion by Windows folks\n> >    harder than necessary.\n> \n> Thanks for noticing this! To store color escapes in strbuf is not a\n> problem as long as the string is finally written using printf, fprintf, or\n> fputs.\n> \n> >> +\t\t\tstrbuf_addch(&sb, '\\n');\n> >> +\t\t\tfwrite(sb.buf, sb.len, 1, f);\n> \n> Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is\n> easy to change such that it uses fprintf().\n\nGood point.  I changed it to\n\n                        fprintf(f, \"%s\", sb.buf);\n\nBTW we probably need to remove the \"TODO: write\" from compat/winansi.c...\n\nCiao,\nDscho\n"},{"id":"124228","messageId":"4AC9B7C3.3090302@viscovery.net","threadId":"21129","inReplyTo":"alpine.DEB.1.00.0910051057350.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Sixt","fromEmail":"j.sixt@viscovery.net","sentAt":"2009-10-05T09:09:23Z","receivedAt":"2009-10-05T09:09:23Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Johannes Schindelin schrieb:\n> On Mon, 5 Oct 2009, Johannes Sixt wrote:\n>> Junio C Hamano schrieb:\n>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n>>>> +\tfwrite(sb.buf, sb.len, 1, f);\n>>>> +\n>>>> +\tif (!message) {\n>>>> +\t\twhile ((commit = get_revision(&rev))) {\n>>>> +\t\t\tstrbuf_setlen(&sb, 0);\n>>>> +\t\t\tif (del)\n>>>> +\t\t\t\tstrbuf_addstr(&sb, commit->object.flags &\n>>>> +\t\t\t\t\t\tSYMMETRIC_LEFT ? del : add);\n>>>> +\t\t\tformat_commit_message(commit, format, &sb,\n>>>> +\t\t\t\t\trev.date_mode);\n>>>> +\t\t\tif (del)\n>>>> +\t\t\t\tstrbuf_addstr(&sb, reset);\n>>>> +\t\t\tstrbuf_addch(&sb, '\\n');\n>>>> +\t\t\tfwrite(sb.buf, sb.len, 1, f);\n>> Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is\n>> easy to change such that it uses fprintf().\n> \n> Good point.  I changed it to\n> \n>                         fprintf(f, \"%s\", sb.buf);\n\nThanks. But notice how you are constructing the string in sb from pieces.\nI meant to change it to\n\n\tfprintf(f, \"%s%s%s\\n\",\n\t\t\tdel ? (commit->object.flags & SYMMETRIC_LEFT\n\t\t\t\t\t ? del : add) : \"\",\n\t\t\tformat_commit_message(commit, format, &sb,\n\t\t\t\t\trev.date_mode),\n\t\t\tdel ? reset : \"\");\n\nor similar. We already use this idiom elsewhere.\n\n-- Hannes\n"},{"id":"124230","messageId":"alpine.DEB.1.00.0910051119260.4985@pacific.mpi-cbg.de","threadId":"21129","inReplyTo":"4AC9B7C3.3090302@viscovery.net","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-05T09:20:06Z","receivedAt":"2009-10-05T09:20:06Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Oct 2009, Johannes Sixt wrote:\n\n> Johannes Schindelin schrieb:\n> > On Mon, 5 Oct 2009, Johannes Sixt wrote:\n> >> Junio C Hamano schrieb:\n> >>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:\n> >>>> +\tfwrite(sb.buf, sb.len, 1, f);\n> >>>> +\n> >>>> +\tif (!message) {\n> >>>> +\t\twhile ((commit = get_revision(&rev))) {\n> >>>> +\t\t\tstrbuf_setlen(&sb, 0);\n> >>>> +\t\t\tif (del)\n> >>>> +\t\t\t\tstrbuf_addstr(&sb, commit->object.flags &\n> >>>> +\t\t\t\t\t\tSYMMETRIC_LEFT ? del : add);\n> >>>> +\t\t\tformat_commit_message(commit, format, &sb,\n> >>>> +\t\t\t\t\trev.date_mode);\n> >>>> +\t\t\tif (del)\n> >>>> +\t\t\t\tstrbuf_addstr(&sb, reset);\n> >>>> +\t\t\tstrbuf_addch(&sb, '\\n');\n> >>>> +\t\t\tfwrite(sb.buf, sb.len, 1, f);\n> >> Outch! fwrite doesn't interpret color escapes. AFAICS, this sequence is\n> >> easy to change such that it uses fprintf().\n> > \n> > Good point.  I changed it to\n> > \n> >                         fprintf(f, \"%s\", sb.buf);\n> \n> Thanks. But notice how you are constructing the string in sb from pieces.\n> I meant to change it to\n> \n> \tfprintf(f, \"%s%s%s\\n\",\n> \t\t\tdel ? (commit->object.flags & SYMMETRIC_LEFT\n> \t\t\t\t\t ? del : add) : \"\",\n> \t\t\tformat_commit_message(commit, format, &sb,\n> \t\t\t\t\trev.date_mode),\n> \t\t\tdel ? reset : \"\");\n> \n> or similar. We already use this idiom elsewhere.\n\nAnd I find it utterly ugly and unreadable there, too. So this is why I did \nnot do it.\n\nCiao,\nDscho\n"},{"id":"124232","messageId":"alpine.DEB.1.00.0910051120430.4985@pacific.mpi-cbg.de","threadId":"21129","inReplyTo":"alpine.DEB.1.00.0910051027010.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-05T09:21:56Z","receivedAt":"2009-10-05T09:21:56Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Oct 2009, Johannes Schindelin wrote:\n\n> On Sun, 4 Oct 2009, Junio C Hamano wrote:\n> \n> >  - Checking \"del\" to decide if you want to say \"reset\" feels funny.\n> \n> Right.  But I wanted to avoid checking for del, add and reset \n> separately.\n> \n> So I was wrong, and will fix.\n\nThis is the current interdiff:\n\n-- snipsnap --\ndiff --git a/submodule.c b/submodule.c\nindex 3f2590d..b441ea1 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -92,15 +92,18 @@ void show_submodule_summary(FILE *f, const char *path,\n \tif (!message) {\n \t\twhile ((commit = get_revision(&rev))) {\n \t\t\tstrbuf_setlen(&sb, 0);\n-\t\t\tif (del)\n-\t\t\t\tstrbuf_addstr(&sb, commit->object.flags &\n-\t\t\t\t\t\tSYMMETRIC_LEFT ? del : add);\n+\t\t\tif (commit->object.flags & SYMMETRIC_LEFT) {\n+\t\t\t\tif (del)\n+\t\t\t\t\tstrbuf_addstr(&sb, del);\n+\t\t\t}\n+\t\t\telse if (add)\n+\t\t\t\tstrbuf_addstr(&sb, add);\n \t\t\tformat_commit_message(commit, format, &sb,\n \t\t\t\t\trev.date_mode);\n-\t\t\tif (del)\n+\t\t\tif (reset)\n \t\t\t\tstrbuf_addstr(&sb, reset);\n \t\t\tstrbuf_addch(&sb, '\\n');\n-\t\t\tfwrite(sb.buf, sb.len, 1, f);\n+\t\t\tfprintf(f, \"%s\", sb.buf);\n \t\t}\n \t\tclear_commit_marks(left, ~0);\n \t\tclear_commit_marks(right, ~0);\n"},{"id":"124241","messageId":"4AC9D6EB.8090002@web.de","threadId":"21129","inReplyTo":"alpine.DEB.1.00.0910051027010.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2009-10-05T11:22:19Z","receivedAt":"2009-10-05T11:22:19Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"First: I already coded a test and am using this patch in git gui and\ngitk (there was no way git submodule summary would have worked for\ngitk, now it does). Looking really good, will post when this patch\nsolidifies ...\n\nJohannes Schindelin schrieb:\n> That's something I will gladly do after I adjusted the format to look a \n> bit more like \"git submodule summary\" (Jens noticed 4 differences), and \n> after Jens patched git-submodule.sh to use the diff mode instead.\n\nActually there are two more and Junio pointed out #7: submodule summary\nuses --first-parent and we don't (My testcases don't contain merges, so\ni didn't notice).\n\nThe difference that is most obvious and was unintended is that the\nshortlog is indented by four spaces instead of two. I appended an\ninterdiff to fix that.\n\nAs with the other six, i am not really sure if we should just copy the\nbehaviour of git submodule summary.\n\n\n> IOW in hindsight I would like to add the prefix \"RFC/RFH\" to the subject.\n\nFullAck.\n\nFirst some examples for the current behaviour (with the patch below\napplied):\n\n$ git diff --submodule-summary\nSubmodule sub 5431f52..3f35670:\n  > sub3\n$ git submodule summary --files\n* sub 5431f52...3f35670 (1):\n  > sub3\n\n$ git diff-index --submodule-summary HEAD -p\nSubmodule sub 81d7059..3f35670:\n  > sub3\n  > sub2\n$ git submodule summary\n* sub 81d7059...3f35670 (2):\n  > sub3\n  > sub2\n\n[$ git diff --cached --submodule-summary HEAD\nSubmodule sub 81d7059..5431f52:\n  > sub2\n$ git submodule summary --cached\n* sub 81d7059...5431f52 (1):\n  > sub2\n\n$\n\nSo the differences are:\n\n1) Dscho replaced the leading '*' with the more explicit \"Submodule\"\n   I like the explicit version.\n\n2) submodule summary prints out how many shortlog entries are following\n3) submodule summary adds a newline after the shortlog\n4) submodule summary uses --first-parent\n   No idea about the intention here. Lars?\n\n5) submodule summary always prints three '.' between the hashes\n   Seems like submodule summary didn't do the hassle for performance\n   reasons. But for consistency i would prefer the new version as we\n   use it all over the place, no?\n\n6) submodule summary can limit the number of shortlog lines\n   Hm, i want to see them all. Any users of that -n option?\n\n\nTo take this a bit further: It might be a good idea to let git diff\ngenerate output for submodules that is consistent with that for regular\nfiles. So instead of:\n\ngit diff --cached --submodule-summary HEAD -p\nSubmodule sub 81d7059..5431f52:\n    > sub2\n\nwe could produce:\n\ndiff --git a/sub b/sub\nindex 81d7059..3f35670 160000\n--- a/sub\n+++ b/sub\n@@ -1 +1 @@\n-Subproject commit 81d70590e6aaf9f995ebf4d6d284a7ebb355398d\n+Subproject commit 3f356705649b5d566d97ff843cf193359229a453\n  > sub2\n\n(This is the output of a git diff without the --submodule-summary option\nwith an appended shortlog)\n\nI like the second variant (mainly because of consistency). Opinions?\n\n\n\nAnd here the promised indentation depth fix interdiff:\n------------------8<-----------------\ndiff --git a/submodule.c b/submodule.c\nindex 3f2590d..11fce7d 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -42,7 +42,7 @@ void show_submodule_summary(FILE *f, const char *path,\n        struct commit_list *merge_bases, *list;\n        const char *message = NULL;\n        struct strbuf sb = STRBUF_INIT;\n-       static const char *format = \"    %m %s\";\n+       static const char *format = \"  %m %s\";\n        int fast_forward = 0, fast_backward = 0;\n\n        if (add_submodule_odb(path))\n"},{"id":"124262","messageId":"4ACA2DBB.5030908@web.de","threadId":"21129","inReplyTo":"4AC9D6EB.8090002@web.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2009-10-05T17:32:43Z","receivedAt":"2009-10-05T17:32:43Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"A bug showed up in the tests, deleted submodules were labelled\n\"(not checked out)\" instead of \"(submodule deleted)\". This patch\nfixes that.\n\n--------------------------8<--------------------\n[PATCH] fix output for deleted submodules in git diff --submodule-summary\n\nWhen a submodule has been deleted, add_submodule_odb() returns false\nbecause the directory of the submodule is gone. So we have to test the\nsecond sha for null before we call add_submodule_odb() to get the correct\noutput.\n\nSigned-off-by: Jens Lehmann <Jens.Lehmann@web.de>\n---\n submodule.c |    6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 11fce7d..54c8de8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -45,12 +45,12 @@ void show_submodule_summary(FILE *f, const char *path,\n \tstatic const char *format = \"  %m %s\";\n \tint fast_forward = 0, fast_backward = 0;\n\n-\tif (add_submodule_odb(path))\n+\tif (is_null_sha1(two))\n+\t\tmessage = \"(submodule deleted)\";\n+\telse if (add_submodule_odb(path))\n \t\tmessage = \"(not checked out)\";\n \telse if (is_null_sha1(one))\n \t\tmessage = \"(new submodule)\";\n-\telse if (is_null_sha1(two))\n-\t\tmessage = \"(submodule deleted)\";\n \telse if (!(left = lookup_commit_reference(one)) ||\n \t\t\t!(right = lookup_commit_reference(two)))\n \t\tmessage = \"(commits not present)\";\n-- \n1.6.5.rc2.210.gac56a4.dirty\n"},{"id":"124265","messageId":"7vr5thacb4.fsf@alter.siamese.dyndns.org","threadId":"21129","inReplyTo":"alpine.DEB.1.00.0910051027010.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-05T19:08:31Z","receivedAt":"2009-10-05T19:08:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n>> > +\t\tif (prepare_revision_walk(&rev))\n>> > +\t\t\tmessage = \"(revision walker failed)\";\n>> \n>> If prepare_revision_walk() failed for whatever reason, can we trust\n>> fast_forward/fast_backward at this point?\n>\n> No, but it is not used in that case, either, because message is not NULL \n> anymore.\n\nIt is used in that case a few lines below to decide if you add the third\ndot.  That's why I asked.\n\n>> > +\t}\n>> > +\n>> > +\tstrbuf_addf(&sb, \"Submodule %s %s..\", path,\n>> > +\t\t\tfind_unique_abbrev(one, DEFAULT_ABBREV));\n>> > +\tif (!fast_backward && !fast_forward)\n>> > +\t\tstrbuf_addch(&sb, '.');\n\n> Our output methods translate ANSI, so the strbufs only hold the ANSI \n> sequences.\n\nI'll always trust two Johannes's on Windows matters ;-)\n\n> I have no idea why \"submodule --summary\" uses --first-parent, but \n> personally, I would _hate_ it not to see the merged commits in the diff.\n>\n> For a summary, you might get away with seeing\n>\n> \t> Merge bla\n> \t> Merge blub\n> \t> Merge this\n> \t> Merge that\n>\n> but in a diff that does not cut it at all.\n\nAs long as bla/blub/this/that are descriptive enough, I do not see at all\nwhy you think \"summary\" is Ok and \"diff\" is not.  If your response were\n\"it is just a matter of taste; to some people (or project) --first-parent\nis useful and for others it is not\", I would understand it, and it would\nmake sense to use (or not use) --first-parent consistently between this\ncodepath and \"submodule --summary\", though.\n\n> In any case, just to safe-guard against sick minds, I can add a check that \n> says that left, right, and all the merge bases _cannot_ have any flags \n> set, otherwise we output \"(you should visit a psychiatrist)\" or some such.\n\nI wouldn't suggest adding such a kludge.  Being insulting to the user when\nwe hit a corner case _we_ cannot handle does not help anybody, does it?\n\nI see two saner options.  Doing this list walking in a subprocess so that\nyou wouldn't have to worry about object flags at all in this case would\ncertainly be easier; the other option obviously is to have a separate\nobject pool ala libgit2, but that would be a much larger change.\n"},{"id":"124273","messageId":"alpine.DEB.1.00.0910052239350.4985@pacific.mpi-cbg.de","threadId":"21129","inReplyTo":"4ACA2DBB.5030908@web.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-05T20:39:53Z","receivedAt":"2009-10-05T20:39:53Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Oct 2009, Jens Lehmann wrote:\n\n> A bug showed up in the tests, deleted submodules were labelled\n> \"(not checked out)\" instead of \"(submodule deleted)\". This patch\n> fixes that.\n\nGood catch, thanks!\n\nCiao,\nDscho\n"},{"id":"124279","messageId":"alpine.DEB.1.00.0910052251190.4985@pacific.mpi-cbg.de","threadId":"21129","inReplyTo":"7vr5thacb4.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-05T21:08:24Z","receivedAt":"2009-10-05T21:08:24Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 5 Oct 2009, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> >> > +\t\tif (prepare_revision_walk(&rev))\n> >> > +\t\t\tmessage = \"(revision walker failed)\";\n> >> \n> >> If prepare_revision_walk() failed for whatever reason, can we trust\n> >> fast_forward/fast_backward at this point?\n> >\n> > No, but it is not used in that case, either, because message is not NULL \n> > anymore.\n> \n> It is used in that case a few lines below to decide if you add the third\n> dot.  That's why I asked.\n\nWell, fair enough.\n\nThe answer is: yes, we can still trust fast_forward/fast_backward, as \nthere is no question that if the first merge base (which must be the only \nmerge base by definition, in this case) is either \"left\" or \"right\", it is \nfast_forward or fast_backward, respectively.\n\nSo: no worries.\n\n> > I have no idea why \"submodule --summary\" uses --first-parent, but \n> > personally, I would _hate_ it not to see the merged commits in the \n> > diff.\n> >\n> > For a summary, you might get away with seeing\n> >\n> > \t> Merge bla\n> > \t> Merge blub\n> > \t> Merge this\n> > \t> Merge that\n> >\n> > but in a diff that does not cut it at all.\n> \n> As long as bla/blub/this/that are descriptive enough, I do not see at all\n> why you think \"summary\" is Ok and \"diff\" is not.  If your response were\n> \"it is just a matter of taste; to some people (or project) --first-parent\n> is useful and for others it is not\", I would understand it, and it would\n> make sense to use (or not use) --first-parent consistently between this\n> codepath and \"submodule --summary\", though.\n\nYou may be used to git.git's quality of naming the branches you merge.\n\nSadly, this is not the common case.\n\n> > In any case, just to safe-guard against sick minds, I can add a check that \n> > says that left, right, and all the merge bases _cannot_ have any flags \n> > set, otherwise we output \"(you should visit a psychiatrist)\" or some such.\n> \n> I wouldn't suggest adding such a kludge.  Being insulting to the user when\n> we hit a corner case _we_ cannot handle does not help anybody, does it?\n\nWell, I was a little exasperated when I wrote that that you want to handle \nthat case.\n\nBut of course, I should heed Postel's law, and handle the case. Maybe say \nsomething like \"(uses superproject's commits)\".\n\n> I see two saner options.  Doing this list walking in a subprocess so that\n> you wouldn't have to worry about object flags at all in this case would\n> certainly be easier; the other option obviously is to have a separate\n> object pool ala libgit2, but that would be a much larger change.\n\nThe reason why I insist avoiding a subprocess is performance.  The same \nreason holds for a separate object pool: it would just impede the speed, \nAFAICT.\n\nBesides, I vividly remember what happened to a patch I posted to be able \nto just clear the current object pool.  And I cannot imagine a patch \nintroducing a second pool to be any less complicated.\n\nIf you really want the case I illustrated (that the submodule actually \ncontains commits that already have been shown in the superproject) to be \nhandled showing the correct submodule summary (and with --first-parent, I \nthink you will agree that it is a summary, even if it is embedded in a \ndiff), I could imagine calling a subprocess (for simplicity reasons) _iff_ \nleft, right, or any of the merge bases has a flag set.\n\nBut I really, really, really want to avoid a fork() in the common case.  I \ndo have some users on Windows, and I do have a few submodules in that \nproject.  Having too many fork() calls there would just give Git a bad \nreputation.  And it has enough of that, it does not need more.\n\nCiao,\nDscho\n"},{"id":"124308","messageId":"4ACB22E9.3010001@web.de","threadId":"21129","inReplyTo":"alpine.DEB.1.00.0910052251190.4985@pacific.mpi-cbg.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2009-10-06T10:58:49Z","receivedAt":"2009-10-06T10:58:49Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Johannes Schindelin schrieb:\n>>> I have no idea why \"submodule --summary\" uses --first-parent, but \n>>> personally, I would _hate_ it not to see the merged commits in the \n>>> diff.\n>>>\n>>> For a summary, you might get away with seeing\n>>>\n>>> \t> Merge bla\n>>> \t> Merge blub\n>>> \t> Merge this\n>>> \t> Merge that\n>>>\n>>> but in a diff that does not cut it at all.\n>> As long as bla/blub/this/that are descriptive enough, I do not see at all\n>> why you think \"summary\" is Ok and \"diff\" is not.  If your response were\n>> \"it is just a matter of taste; to some people (or project) --first-parent\n>> is useful and for others it is not\", I would understand it, and it would\n>> make sense to use (or not use) --first-parent consistently between this\n>> codepath and \"submodule --summary\", though.\n> \n> You may be used to git.git's quality of naming the branches you merge.\n> \n> Sadly, this is not the common case.\n\nIMHO both arguments are valid, using --first-parent really is a matter of\ntaste *and* it is dependent on the quality of branch naming whether it is\nuseful or not.\n\nBut when both commands shall produce the same output, i think we have to\nuse --first-parent as default, no? And maybe we could add another option\nto diff which can change that behaviour according to users taste?\n\n\n> But I really, really, really want to avoid a fork() in the common case.  I \n> do have some users on Windows, and I do have a few submodules in that \n> project.  Having too many fork() calls there would just give Git a bad \n> reputation.  And it has enough of that, it does not need more.\n\nMe too thinks performance matters here. We do have a repo at my dayjob\nwith more than a handful of submodules and its main target platform is\nwindows ... so having that perform nicely is a win for us.\n\n\nJens\n"},{"id":"124309","messageId":"7vocok21pw.fsf@alter.siamese.dyndns.org","threadId":"21129","inReplyTo":"4ACB22E9.3010001@web.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-06T11:36:43Z","receivedAt":"2009-10-06T11:36:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n>> But I really, really, really want to avoid a fork() in the common case.  I \n>> do have some users on Windows, and I do have a few submodules in that \n>> project.  Having too many fork() calls there would just give Git a bad \n>> reputation.  And it has enough of that, it does not need more.\n>\n> Me too thinks performance matters here. We do have a repo at my dayjob\n> with more than a handful of submodules and its main target platform is\n> windows ... so having that perform nicely is a win for us.\n\nNumbers?\n\nI'd prefer to avoid kludges that favors unsubstantiated performance\nargument over correctness.\n\nThanks.\n"},{"id":"124311","messageId":"alpine.DEB.1.00.0910061344070.4686@intel-tinevez-2-302","threadId":"21129","inReplyTo":"7vocok21pw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-06T11:45:44Z","receivedAt":"2009-10-06T11:45:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 6 Oct 2009, Junio C Hamano wrote:\n\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n> >> But I really, really, really want to avoid a fork() in the common case.  I \n> >> do have some users on Windows, and I do have a few submodules in that \n> >> project.  Having too many fork() calls there would just give Git a bad \n> >> reputation.  And it has enough of that, it does not need more.\n> >\n> > Me too thinks performance matters here. We do have a repo at my dayjob\n> > with more than a handful of submodules and its main target platform is\n> > windows ... so having that perform nicely is a win for us.\n> \n> Numbers?\n> \n> I'd prefer to avoid kludges that favors unsubstantiated performance\n> argument over correctness.\n\nWell, having worked with msysGit for such a long time, I just _know_ that \na subprocess costs a substantial amount of time.\n\nBut as you don't trust my words, maybe Jens could be so kind as to perform \nsome benchmarks?  I am short on Git time budget, but I will make a commit \non my submodule-summary branch that allows to start a subprocess always.\n\nCiao,\nDscho\n"},{"id":"124312","messageId":"4ACB2F2D.8010503@web.de","threadId":"21129","inReplyTo":"alpine.DEB.1.00.0910061344070.4686@intel-tinevez-2-302","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2009-10-06T11:51:09Z","receivedAt":"2009-10-06T11:51:09Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Johannes Schindelin schrieb:\n> Hi,\n> \n> On Tue, 6 Oct 2009, Junio C Hamano wrote:\n> \n>> Jens Lehmann <Jens.Lehmann@web.de> writes:\n>>\n>>>> But I really, really, really want to avoid a fork() in the common case.  I \n>>>> do have some users on Windows, and I do have a few submodules in that \n>>>> project.  Having too many fork() calls there would just give Git a bad \n>>>> reputation.  And it has enough of that, it does not need more.\n>>> Me too thinks performance matters here. We do have a repo at my dayjob\n>>> with more than a handful of submodules and its main target platform is\n>>> windows ... so having that perform nicely is a win for us.\n>> Numbers?\n>>\n>> I'd prefer to avoid kludges that favors unsubstantiated performance\n>> argument over correctness.\n> \n> Well, having worked with msysGit for such a long time, I just _know_ that \n> a subprocess costs a substantial amount of time.\n> \n> But as you don't trust my words, maybe Jens could be so kind as to perform \n> some benchmarks?  I am short on Git time budget, but I will make a commit \n> on my submodule-summary branch that allows to start a subprocess always.\n\nSure, will do.\n\nJens\n"},{"id":"124314","messageId":"alpine.DEB.1.00.0910061409440.4686@intel-tinevez-2-302","threadId":"21129","inReplyTo":"4ACB2F2D.8010503@web.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-06T12:10:43Z","receivedAt":"2009-10-06T12:10:43Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 6 Oct 2009, Jens Lehmann wrote:\n\n> Johannes Schindelin schrieb:\n> > Hi,\n> > \n> > On Tue, 6 Oct 2009, Junio C Hamano wrote:\n> > \n> >> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> >>\n> >>>> But I really, really, really want to avoid a fork() in the common case.  I \n> >>>> do have some users on Windows, and I do have a few submodules in that \n> >>>> project.  Having too many fork() calls there would just give Git a bad \n> >>>> reputation.  And it has enough of that, it does not need more.\n> >>> Me too thinks performance matters here. We do have a repo at my dayjob\n> >>> with more than a handful of submodules and its main target platform is\n> >>> windows ... so having that perform nicely is a win for us.\n> >> Numbers?\n> >>\n> >> I'd prefer to avoid kludges that favors unsubstantiated performance\n> >> argument over correctness.\n> > \n> > Well, having worked with msysGit for such a long time, I just _know_ that \n> > a subprocess costs a substantial amount of time.\n> > \n> > But as you don't trust my words, maybe Jens could be so kind as to perform \n> > some benchmarks?  I am short on Git time budget, but I will make a commit \n> > on my submodule-summary branch that allows to start a subprocess always.\n> \n> Sure, will do.\n\nOkay, it is there.  It is quick and dirty, so you don't even want to look \nat the commit message.\n\nCould you please run something like \"time git diff --submodule-summary \n--all\" with and without this patch?\n\nThanks,\nDscho\n"},{"id":"124339","messageId":"4ACCECE6.1030508@web.de","threadId":"21129","inReplyTo":"7vocok21pw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Jens Lehmann","fromEmail":"jens.lehmann@web.de","sentAt":"2009-10-07T19:32:54Z","receivedAt":"2009-10-07T19:32:54Z","isPatch":true,"sender":{"key":"jens.lehmann@web.de","avatar":"https://avatars.githubusercontent.com/u/135220?v=4"},"body":"Junio C Hamano schrieb:\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n>>> But I really, really, really want to avoid a fork() in the common case.  I \n>>> do have some users on Windows, and I do have a few submodules in that \n>>> project.  Having too many fork() calls there would just give Git a bad \n>>> reputation.  And it has enough of that, it does not need more.\n>> Me too thinks performance matters here. We do have a repo at my dayjob\n>> with more than a handful of submodules and its main target platform is\n>> windows ... so having that perform nicely is a win for us.\n> \n> Numbers?\n\nHere they are:\n\nFirst i did them with the repo at hand, current msysgit master with\nDscho's git-repo checked out:\n\n  without fork : real\t0m0.672s\n  with fork    : real\t0m0.781s\n\nSo here it's a about 16% slower when using fork() (and both are\ngenerating about 7270 shortlog entries).\n\nBut i thought this to be a rather unusual situation, having only one\nsubmodule being changed by 7270 commits ...\n\nSo i took a live repo from my dayjob containing 8 submodules. In each\nsubmodule i did a \"git checkout HEAD^\" to simulate one change. And\nthen i got:\n\n  without fork : real\t0m0.203s\n  with fork    : real\t0m0.453s\n\nThis is a degradation of more than 120% because of the fork()s. And\njust for fun i ran the scripted submodule summary too:\n\n  scripted     : real\t0m3.437s\n\nSo the forked version outperforms the scripted version by a factor of\n7, while the speedup from Dscho's original proposal is almost 17fold.\n(If i did my computations right, the extra costs for each changed\nsubmodule are a bit more than 30ms when fork()ing. Dscho's version\ndoesn't seem to suffer from changed submodules at all, i measured\n0.203s for both versions before i did the submodule init and update).\n\n(Best of three, \"time git diff --submodule-summary\". My system is an\nAthlon64x2 4600+ with WindowsXP)\n\n\nJens\n"},{"id":"124342","messageId":"7vd44zt1nf.fsf@alter.siamese.dyndns.org","threadId":"21129","inReplyTo":"4ACCECE6.1030508@web.de","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-10-07T20:00:36Z","receivedAt":"2009-10-07T20:00:36Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jens Lehmann <Jens.Lehmann@web.de> writes:\n\n> So i took a live repo from my dayjob containing 8 submodules. In each\n> submodule i did a \"git checkout HEAD^\" to simulate one change.\n\nIt appears that your \"simulation\" is to have changes in all submodules and\nnowhere else in the tree.  I have to wonder how common would that be.  If\nI have been working in the subprojects without changing the superproject\nat all, I would likely to be in one of the subproject directories and my\n\"git diff\" would not even have to trigger this codepath.  If we have on\nthe other hand changes in the superproject, showing them would cost us\nequally in both approaches.\n\n> then i got:\n>\n>   without fork : real\t0m0.203s\n>   with fork    : real\t0m0.453s\n>\n> This is a degradation of more than 120% because of the fork()s. And\n> just for fun i ran the scripted submodule summary too:\n>\n>   scripted     : real\t0m3.437s\n>\n> So the forked version outperforms the scripted version by a factor of\n> 7, while the speedup from Dscho's original proposal is almost 17fold.\n\nThanks.  I was worried if the forking to ensure correctness may sactifice\nperformance so much to be unusable, but it does not seem to be the case,\ngiven the above comparision.\n\nI didn't look at the code but presumably it uses the run_command()\ninterface and cleanly written?\n"},{"id":"124349","messageId":"alpine.DEB.1.00.0910080027270.4985@pacific.mpi-cbg.de","threadId":"21129","inReplyTo":"7vd44zt1nf.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] Add the --submodule-summary option to the diff option family","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2009-10-07T22:28:14Z","receivedAt":"2009-10-07T22:28:14Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Wed, 7 Oct 2009, Junio C Hamano wrote:\n\n> Jens Lehmann <Jens.Lehmann@web.de> writes:\n> \n> > So i took a live repo from my dayjob containing 8 submodules. In each\n> > submodule i did a \"git checkout HEAD^\" to simulate one change.\n> \n> It appears that your \"simulation\" is to have changes in all submodules \n> and nowhere else in the tree.  I have to wonder how common would that \n> be.  If I have been working in the subprojects without changing the \n> superproject at all, I would likely to be in one of the subproject \n> directories and my \"git diff\" would not even have to trigger this \n> codepath.  If we have on the other hand changes in the superproject, \n> showing them would cost us equally in both approaches.\n> \n> > then i got:\n> >\n> >   without fork : real\t0m0.203s\n> >   with fork    : real\t0m0.453s\n> >\n> > This is a degradation of more than 120% because of the fork()s. And\n> > just for fun i ran the scripted submodule summary too:\n> >\n> >   scripted     : real\t0m3.437s\n> >\n> > So the forked version outperforms the scripted version by a factor of\n> > 7, while the speedup from Dscho's original proposal is almost 17fold.\n> \n> Thanks.  I was worried if the forking to ensure correctness may sactifice\n> performance so much to be unusable, but it does not seem to be the case,\n> given the above comparision.\n> \n> I didn't look at the code but presumably it uses the run_command()\n> interface and cleanly written?\n\nYou seem to be rather unconvinced by rather convincing numbers.\n\nTherefore, I will continue working on the --submodule-summary stuff in \n4msysgit.git.\n\nCiao,\nDscho\n"}]}