[RFC] diff: support custom callbacks for output

[RFC] diff: support custom callbacks for output

[Jeff King]

---
I'm re-writing git-status in C, and that entails using the git library
to replace calls to things like git-diff-index. Fortunately,
run_diff_index does just what I want. Unfortunately, it ends up wanting
to send its output to stdout in one of the pre-defined diff formats, and
what I really want is my own status format.

I could, of course, add diff_flush_status() support to diffcore.
However, I'm not sure that it really belongs there. In the interests of
libification, it seems reasonable for diffcore to provide a callback
mechanism for each filepair.

Below is an attempt to provide such a mechanism. My questions are:
  1. Am I right that there is no other way to accomplish this task? This
     is my first interaction with diffcore, so I might have overlooked
     something.
  2. Is this the way I should go about it, or should I add a status
     output to git-diff? (Note that the callback takes the same
     parameters as the other diff_flush_* functions; it may be that we
     can use this general mechanism to implement other formats).
  3. This seems very similar in spirit to the add_remove and changed
     members of diff_options. However, I'm not clear on when those are
     called (they seem to only be called from diff-tree).

 diff.c |    8 ++++++++
 diff.h |    7 +++++++
 2 files changed, 15 insertions(+), 0 deletions(-)

diff --git a/diff.c b/diff.c
index f5c9a55..a04257f 100644
--- a/diff.c
+++ b/diff.c
@@ -2278,6 +2278,14 @@ void diff_flush(struct diff_options *opt
 		}
 	}
 
+	if (output_format & DIFF_FORMAT_CALLBACK) {
+		for (i = 0; i < q->nr; i++) {
+			struct diff_filepair *p = q->queue[i];
+			if (check_pair_status(p))
+				options->format_callback(p, options);
+		}
+	}
+
 	for (i = 0; i < q->nr; i++)
 		diff_free_filepair(q->queue[i]);
 free_queue:
diff --git a/diff.h b/diff.h
index 2cced53..5671a53 100644
--- a/diff.h
+++ b/diff.h
@@ -8,6 +8,7 @@ #include "tree-walk.h"
 
 struct rev_info;
 struct diff_options;
+struct diff_filepair;
 
 typedef void (*change_fn_t)(struct diff_options *options,
 		 unsigned old_mode, unsigned new_mode,
@@ -20,6 +21,9 @@ typedef void (*add_remove_fn_t)(struct d
 		    const unsigned char *sha1,
 		    const char *base, const char *path);
 
+typedef void (*diff_format_fn_t)(struct diff_filepair *p,
+		struct diff_options *options);
+
 #define DIFF_FORMAT_RAW		0x0001
 #define DIFF_FORMAT_DIFFSTAT	0x0002
 #define DIFF_FORMAT_SUMMARY	0x0004
@@ -35,6 +39,8 @@ #define DIFF_FORMAT_CHECKDIFF	0x0040
  */
 #define DIFF_FORMAT_NO_OUTPUT	0x0080
 
+#define DIFF_FORMAT_CALLBACK	0x0100
+
 struct diff_options {
 	const char *filter;
 	const char *orderfile;
@@ -67,6 +73,7 @@ struct diff_options {
 	int *pathlens;
 	change_fn_t change;
 	add_remove_fn_t add_remove;
+	diff_format_fn_t format_callback;
 };
 
 enum color_diff {
-- 
1.4.2.rc3.g539fa-dirty

Re: [RFC] diff: support custom callbacks for output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm re-writing git-status in C, and that entails using the git library
> to replace calls to things like git-diff-index. Fortunately,
> run_diff_index does just what I want. Unfortunately, it ends up wanting
> to send its output to stdout in one of the pre-defined diff formats, and
> what I really want is my own status format.

Good.  I then can stop the same I've been doing in "pu" ;-).

> I could, of course, add diff_flush_status() support to diffcore.
> However, I'm not sure that it really belongs there. In the interests of
> libification, it seems reasonable for diffcore to provide a callback
> mechanism for each filepair.
>
> Below is an attempt to provide such a mechanism. My questions are:
>   1. Am I right that there is no other way to accomplish this task? This
>      is my first interaction with diffcore, so I might have overlooked
>      something.

What I was going to do was to give a hook to run_diff_index()
and friends, to be called just before diff_flush().  Then the
hook can inspect the diff_queued_diff for the result.  Obviously
you do not want diff_flush() to say anything, so you would call
it with no-output option.  But I think adding a new generic
"output format" in the form of a callback hook is a very good
idea.  I am not sure if a callback per filepair is a good
interface, or passing the q (that is &diff_queued_diff) and let
the hook iterate over it might be better, though.  It probably
would not be such a big deal either way.

Having said that, I am wondering how much of "git status" you
are doing in C.  Initially, I was going to do the whole thing
(one good reason to do so is that then we do not have to write
out a temporary index file; we can do everything in-core.
However I came to realize it is quite a big undertaking.  In
order to handle -o, you would need to run diff-files (to make
sure the index is clean at specified paths), discard_cache() and
read-tree (to re-read the HEAD commit tree), update-index with
ls-files like pathspec (to update the index at specified paths),
then do "diff-index --cached" (to say "Will commit"),
"diff-files" (to say "Changed"), and "ls-files -o"
("Untracked").

If you are planning to go the whole nine yards, more power to
you ;-), but a less ambitious fallback position would be to
rewrite only git-commit.sh::run_status() in C.  That is, let the
current git-commit.sh drive the parts to prepare TMP_INDEX (or
NEXT_INDEX), and write a C program that uses the given index
(most likely specified by GIT_INDEX_FILE environment variable by
the caller -- git-commit.sh), REFERENCE (probably given as a
parameter, usually HEAD but HEAD^1 while --amend and nothing if
the initial commit) and the working tree files.  The program
would run (internally) diff-index --cached, diff-files and
ls-files -o to do "Will commit", "Changed" and "Untracked".

By the way, I was pondering about making the git status output a
bit more readable by making it shorter.  Instead of listing the
same file in "Updated but not checked in" and "Changed but not
updated" sections twice, it might be more alarming and concise
to say:

        Will commit
                modified: Makefile (warning: further changed)
                modified: foo.c

        Changed
                modified: bar.c

        Untracked
                baz.c

Re: [RFC] diff: support custom callbacks for output

[Jeff King]

On Mon, Aug 07, 2006 at 01:54:38AM -0700, Junio C Hamano wrote:

> "output format" in the form of a callback hook is a very good
> idea.  I am not sure if a callback per filepair is a good
> interface, or passing the q (that is &diff_queued_diff) and let
> the hook iterate over it might be better, though.  It probably
> would not be such a big deal either way.

Looking through the diff_flush code, many of the formats want to do
something slightly more interesting than simply looping (and one would
assume that any gitlib users might want to do similar things), so I
think passing q makes more sense. As a bonus, it helps avoid contortions
like:
  void my_callback(...) {
    static int header_shown = 0;
    if (!header_shown) ... /* make sure we show header exactly once iff
                              we have any diff items */
    header_shown = 1;
  }

One could, of course, implement the current diff output formats as
callbacks. I'm not sure if it's worth the effort, though, as we
currently support multiple simultaneous outputs (which the flags do
fine, but which would require a list of callbacks).

> Having said that, I am wondering how much of "git status" you
> are doing in C.  Initially, I was going to do the whole thing
> (one good reason to do so is that then we do not have to write
> out a temporary index file; we can do everything in-core.
> However I came to realize it is quite a big undertaking.  In

I'm starting by writing run_status in C. Once that is working (which
should be soon), I believe it should suffice as a vanilla git-status
(do people actually do things like git-status with flags? It's not
documented, but it does work). My plan is:
  1. get working builtin git-status
  2. remove git-status from git-commit.sh
  3. naively replace run_status with call to git-status, with
     git-commit handling things like TMP_INDEX
  4. one by one, expand git-status options to handle special cases
     from git-commit.sh

> the initial commit) and the working tree files.  The program
> would run (internally) diff-index --cached, diff-files and
> ls-files -o to do "Will commit", "Changed" and "Untracked".

Yes, that is step 1 above (and is almost done -- I needed to figure out
this diff interface first).

> By the way, I was pondering about making the git status output a
> bit more readable by making it shorter.  Instead of listing the

I'm definitely in favor.

>         Will commit
>                 modified: Makefile (warning: further changed)

I like it (the double-mention of files which were changed, updated, then
changed has always bothered me). However, I'm not sure how we can get
the diff machinery to figure this out easily. Getting the knowledge for
the line above requires diffing tree to cache and cache to working
directory. Is there a better way than saving the queue from one diff and
cross-referencing it with the other?

-Peff

Re: [RFC] diff: support custom callbacks for output

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm starting by writing run_status in C. Once that is working (which
> should be soon), I believe it should suffice as a vanilla git-status
> (do people actually do things like git-status with flags? It's not
> documented, but it does work).

Well, "git-status" is by definition (see list discussion when it
was made into its current shape) a preview of "git-commit", so
all the options are supported and needs to work with options.  I
just noticed that its documentation has not been updated, though.

So I'd suggest, mildly, against naming your "run_status()
equivalent" git-status.  And if you follow through your plan,
you would most likely have git-status _and_ git-commit both in C
at about the same time when you finish.

> I'm definitely in favor.
>
>>         Will commit
>>                 modified: Makefile (warning: further changed)
>
> I like it (the double-mention of files which were changed, updated, then
> changed has always bothered me). However, I'm not sure how we can get
> the diff machinery to figure this out easily. Getting the knowledge for
> the line above requires diffing tree to cache and cache to working
> directory. Is there a better way than saving the queue from one diff and
> cross-referencing it with the other?

I do not think so.  I was initially planning to write a new
traversal function that walks working tree, index _and_ a tree
in parallel, but that would not work well with -M and -C, so I
dropped it.