{"thread":{"id":"5161","subject":"[RFC] diff: support custom callbacks for output","startedAt":"2006-08-07T07:50:02Z","lastAt":"2006-08-07T10:16:20Z","messageCount":4,"participants":["Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"24910","messageId":"20060807075002.GA29693@coredump.intra.peff.net","threadId":"5161","inReplyTo":null,"subject":"[RFC] diff: support custom callbacks for output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-08-07T07:50:02Z","receivedAt":"2006-08-07T07:50:02Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"---\nI'm re-writing git-status in C, and that entails using the git library\nto replace calls to things like git-diff-index. Fortunately,\nrun_diff_index does just what I want. Unfortunately, it ends up wanting\nto send its output to stdout in one of the pre-defined diff formats, and\nwhat I really want is my own status format.\n\nI could, of course, add diff_flush_status() support to diffcore.\nHowever, I'm not sure that it really belongs there. In the interests of\nlibification, it seems reasonable for diffcore to provide a callback\nmechanism for each filepair.\n\nBelow is an attempt to provide such a mechanism. My questions are:\n  1. Am I right that there is no other way to accomplish this task? This\n     is my first interaction with diffcore, so I might have overlooked\n     something.\n  2. Is this the way I should go about it, or should I add a status\n     output to git-diff? (Note that the callback takes the same\n     parameters as the other diff_flush_* functions; it may be that we\n     can use this general mechanism to implement other formats).\n  3. This seems very similar in spirit to the add_remove and changed\n     members of diff_options. However, I'm not clear on when those are\n     called (they seem to only be called from diff-tree).\n\n diff.c |    8 ++++++++\n diff.h |    7 +++++++\n 2 files changed, 15 insertions(+), 0 deletions(-)\n\ndiff --git a/diff.c b/diff.c\nindex f5c9a55..a04257f 100644\n--- a/diff.c\n+++ b/diff.c\n@@ -2278,6 +2278,14 @@ void diff_flush(struct diff_options *opt\n \t\t}\n \t}\n \n+\tif (output_format & DIFF_FORMAT_CALLBACK) {\n+\t\tfor (i = 0; i < q->nr; i++) {\n+\t\t\tstruct diff_filepair *p = q->queue[i];\n+\t\t\tif (check_pair_status(p))\n+\t\t\t\toptions->format_callback(p, options);\n+\t\t}\n+\t}\n+\n \tfor (i = 0; i < q->nr; i++)\n \t\tdiff_free_filepair(q->queue[i]);\n free_queue:\ndiff --git a/diff.h b/diff.h\nindex 2cced53..5671a53 100644\n--- a/diff.h\n+++ b/diff.h\n@@ -8,6 +8,7 @@ #include \"tree-walk.h\"\n \n struct rev_info;\n struct diff_options;\n+struct diff_filepair;\n \n typedef void (*change_fn_t)(struct diff_options *options,\n \t\t unsigned old_mode, unsigned new_mode,\n@@ -20,6 +21,9 @@ typedef void (*add_remove_fn_t)(struct d\n \t\t    const unsigned char *sha1,\n \t\t    const char *base, const char *path);\n \n+typedef void (*diff_format_fn_t)(struct diff_filepair *p,\n+\t\tstruct diff_options *options);\n+\n #define DIFF_FORMAT_RAW\t\t0x0001\n #define DIFF_FORMAT_DIFFSTAT\t0x0002\n #define DIFF_FORMAT_SUMMARY\t0x0004\n@@ -35,6 +39,8 @@ #define DIFF_FORMAT_CHECKDIFF\t0x0040\n  */\n #define DIFF_FORMAT_NO_OUTPUT\t0x0080\n \n+#define DIFF_FORMAT_CALLBACK\t0x0100\n+\n struct diff_options {\n \tconst char *filter;\n \tconst char *orderfile;\n@@ -67,6 +73,7 @@ struct diff_options {\n \tint *pathlens;\n \tchange_fn_t change;\n \tadd_remove_fn_t add_remove;\n+\tdiff_format_fn_t format_callback;\n };\n \n enum color_diff {\n-- \n1.4.2.rc3.g539fa-dirty\n"},{"id":"24913","messageId":"7vr6zt3oz5.fsf@assigned-by-dhcp.cox.net","threadId":"5161","inReplyTo":"20060807075002.GA29693@coredump.intra.peff.net","subject":"Re: [RFC] diff: support custom callbacks for output","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-07T08:54:38Z","receivedAt":"2006-08-07T08:54:38Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm re-writing git-status in C, and that entails using the git library\n> to replace calls to things like git-diff-index. Fortunately,\n> run_diff_index does just what I want. Unfortunately, it ends up wanting\n> to send its output to stdout in one of the pre-defined diff formats, and\n> what I really want is my own status format.\n\nGood.  I then can stop the same I've been doing in \"pu\" ;-).\n\n> I could, of course, add diff_flush_status() support to diffcore.\n> However, I'm not sure that it really belongs there. In the interests of\n> libification, it seems reasonable for diffcore to provide a callback\n> mechanism for each filepair.\n>\n> Below is an attempt to provide such a mechanism. My questions are:\n>   1. Am I right that there is no other way to accomplish this task? This\n>      is my first interaction with diffcore, so I might have overlooked\n>      something.\n\nWhat I was going to do was to give a hook to run_diff_index()\nand friends, to be called just before diff_flush().  Then the\nhook can inspect the diff_queued_diff for the result.  Obviously\nyou do not want diff_flush() to say anything, so you would call\nit with no-output option.  But I think adding a new generic\n\"output format\" in the form of a callback hook is a very good\nidea.  I am not sure if a callback per filepair is a good\ninterface, or passing the q (that is &diff_queued_diff) and let\nthe hook iterate over it might be better, though.  It probably\nwould not be such a big deal either way.\n\nHaving said that, I am wondering how much of \"git status\" you\nare doing in C.  Initially, I was going to do the whole thing\n(one good reason to do so is that then we do not have to write\nout a temporary index file; we can do everything in-core.\nHowever I came to realize it is quite a big undertaking.  In\norder to handle -o, you would need to run diff-files (to make\nsure the index is clean at specified paths), discard_cache() and\nread-tree (to re-read the HEAD commit tree), update-index with\nls-files like pathspec (to update the index at specified paths),\nthen do \"diff-index --cached\" (to say \"Will commit\"),\n\"diff-files\" (to say \"Changed\"), and \"ls-files -o\"\n(\"Untracked\").\n\nIf you are planning to go the whole nine yards, more power to\nyou ;-), but a less ambitious fallback position would be to\nrewrite only git-commit.sh::run_status() in C.  That is, let the\ncurrent git-commit.sh drive the parts to prepare TMP_INDEX (or\nNEXT_INDEX), and write a C program that uses the given index\n(most likely specified by GIT_INDEX_FILE environment variable by\nthe caller -- git-commit.sh), REFERENCE (probably given as a\nparameter, usually HEAD but HEAD^1 while --amend and nothing if\nthe initial commit) and the working tree files.  The program\nwould run (internally) diff-index --cached, diff-files and\nls-files -o to do \"Will commit\", \"Changed\" and \"Untracked\".\n\nBy the way, I was pondering about making the git status output a\nbit more readable by making it shorter.  Instead of listing the\nsame file in \"Updated but not checked in\" and \"Changed but not\nupdated\" sections twice, it might be more alarming and concise\nto say:\n\n        Will commit\n                modified: Makefile (warning: further changed)\n                modified: foo.c\n\n        Changed\n                modified: bar.c\n\n        Untracked\n                baz.c\n"},{"id":"24914","messageId":"20060807091953.GA31137@coredump.intra.peff.net","threadId":"5161","inReplyTo":"7vr6zt3oz5.fsf@assigned-by-dhcp.cox.net","subject":"Re: [RFC] diff: support custom callbacks for output","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2006-08-07T09:19:53Z","receivedAt":"2006-08-07T09:19:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 07, 2006 at 01:54:38AM -0700, Junio C Hamano wrote:\n\n> \"output format\" in the form of a callback hook is a very good\n> idea.  I am not sure if a callback per filepair is a good\n> interface, or passing the q (that is &diff_queued_diff) and let\n> the hook iterate over it might be better, though.  It probably\n> would not be such a big deal either way.\n\nLooking through the diff_flush code, many of the formats want to do\nsomething slightly more interesting than simply looping (and one would\nassume that any gitlib users might want to do similar things), so I\nthink passing q makes more sense. As a bonus, it helps avoid contortions\nlike:\n  void my_callback(...) {\n    static int header_shown = 0;\n    if (!header_shown) ... /* make sure we show header exactly once iff\n                              we have any diff items */\n    header_shown = 1;\n  }\n\nOne could, of course, implement the current diff output formats as\ncallbacks. I'm not sure if it's worth the effort, though, as we\ncurrently support multiple simultaneous outputs (which the flags do\nfine, but which would require a list of callbacks).\n\n> Having said that, I am wondering how much of \"git status\" you\n> are doing in C.  Initially, I was going to do the whole thing\n> (one good reason to do so is that then we do not have to write\n> out a temporary index file; we can do everything in-core.\n> However I came to realize it is quite a big undertaking.  In\n\nI'm starting by writing run_status in C. Once that is working (which\nshould be soon), I believe it should suffice as a vanilla git-status\n(do people actually do things like git-status with flags? It's not\ndocumented, but it does work). My plan is:\n  1. get working builtin git-status\n  2. remove git-status from git-commit.sh\n  3. naively replace run_status with call to git-status, with\n     git-commit handling things like TMP_INDEX\n  4. one by one, expand git-status options to handle special cases\n     from git-commit.sh\n\n> the initial commit) and the working tree files.  The program\n> would run (internally) diff-index --cached, diff-files and\n> ls-files -o to do \"Will commit\", \"Changed\" and \"Untracked\".\n\nYes, that is step 1 above (and is almost done -- I needed to figure out\nthis diff interface first).\n\n> By the way, I was pondering about making the git status output a\n> bit more readable by making it shorter.  Instead of listing the\n\nI'm definitely in favor.\n\n>         Will commit\n>                 modified: Makefile (warning: further changed)\n\nI like it (the double-mention of files which were changed, updated, then\nchanged has always bothered me). However, I'm not sure how we can get\nthe diff machinery to figure this out easily. Getting the knowledge for\nthe line above requires diffing tree to cache and cache to working\ndirectory. Is there a better way than saving the queue from one diff and\ncross-referencing it with the other?\n\n-Peff\n"},{"id":"24915","messageId":"7vhd0o4zrf.fsf@assigned-by-dhcp.cox.net","threadId":"5161","inReplyTo":"20060807091953.GA31137@coredump.intra.peff.net","subject":"Re: [RFC] diff: support custom callbacks for output","fromName":"Junio C Hamano","fromEmail":"junkio@cox.net","sentAt":"2006-08-07T10:16:20Z","receivedAt":"2006-08-07T10:16:20Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I'm starting by writing run_status in C. Once that is working (which\n> should be soon), I believe it should suffice as a vanilla git-status\n> (do people actually do things like git-status with flags? It's not\n> documented, but it does work).\n\nWell, \"git-status\" is by definition (see list discussion when it\nwas made into its current shape) a preview of \"git-commit\", so\nall the options are supported and needs to work with options.  I\njust noticed that its documentation has not been updated, though.\n\nSo I'd suggest, mildly, against naming your \"run_status()\nequivalent\" git-status.  And if you follow through your plan,\nyou would most likely have git-status _and_ git-commit both in C\nat about the same time when you finish.\n\n> I'm definitely in favor.\n>\n>>         Will commit\n>>                 modified: Makefile (warning: further changed)\n>\n> I like it (the double-mention of files which were changed, updated, then\n> changed has always bothered me). However, I'm not sure how we can get\n> the diff machinery to figure this out easily. Getting the knowledge for\n> the line above requires diffing tree to cache and cache to working\n> directory. Is there a better way than saving the queue from one diff and\n> cross-referencing it with the other?\n\nI do not think so.  I was initially planning to write a new\ntraversal function that walks working tree, index _and_ a tree\nin parallel, but that would not work well with -M and -C, so I\ndropped it.\n"}]}