[PATCH 3/3] git-commit.sh: convert run_status to a C builtin

[PATCH 3/3] git-commit.sh: convert run_status to a C builtin

[Jeff King]

This creates a new git-runstatus which should do roughly the same thing
as the run_status function from git-commit.sh. Except for color support,
the main focus has been to keep the output identical, so that it can be
verified as correct and then used as a C platform for other improvements to
the status printing code.
---
 .gitignore          |    1 
 Makefile            |    3 -
 builtin-runstatus.c |   34 ++++++
 builtin.h           |    1 
 dir.c               |    7 +
 dir.h               |    1 
 git-commit.sh       |  106 +------------------
 git.c               |    1 
 status.c            |  283 +++++++++++++++++++++++++++++++++++++++++++++++++++
 status.h            |   31 ++++++
 10 files changed, 368 insertions(+), 100 deletions(-)

diff --git a/.gitignore b/.gitignore
index 78cb671..f014ad3 100644
--- a/.gitignore
+++ b/.gitignore
@@ -94,6 +94,7 @@ git-rev-list
 git-rev-parse
 git-revert
 git-rm
+git-runstatus
 git-send-email
 git-send-pack
 git-sh-setup
diff --git a/Makefile b/Makefile
index 78748cb..d0ba3b5 100644
--- a/Makefile
+++ b/Makefile
@@ -252,7 +252,7 @@ LIB_OBJS = \
 	fetch-clone.o revision.o pager.o tree-walk.o xdiff-interface.o \
 	write_or_die.o trace.o \
 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
-	color.o
+	color.o status.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
@@ -286,6 +286,7 @@ BUILTIN_OBJS = \
 	builtin-rev-list.o \
 	builtin-rev-parse.o \
 	builtin-rm.o \
+	builtin-runstatus.o \
 	builtin-show-branch.o \
 	builtin-stripspace.o \
 	builtin-symbolic-ref.o \
diff --git a/builtin-runstatus.c b/builtin-runstatus.c
new file mode 100644
index 0000000..f3e44de
--- /dev/null
+++ b/builtin-runstatus.c
@@ -0,0 +1,34 @@
+#include "status.h"
+#include "cache.h"
+
+extern int status_use_color;
+
+static const char runstatus_usage[] =
+"git-runstatus [--color|--nocolor] [--amend] [--verbose]";
+
+int cmd_runstatus(int argc, const char **argv, const char *prefix)
+{
+	struct status s;
+	int i;
+
+	git_config(git_status_config);
+	status_prepare(&s);
+
+	for (i = 1; i < argc; i++) {
+		if (!strcmp(argv[i], "--color"))
+			status_use_color = 1;
+		else if (!strcmp(argv[i], "--nocolor"))
+			status_use_color = 0;
+    else if (!strcmp(argv[i], "--amend")) {
+      s.amend = 1;
+      s.reference = "HEAD^1";
+    }
+    else if (!strcmp(argv[i], "--verbose"))
+      s.verbose = 1;
+		else
+			usage(runstatus_usage);
+	}
+
+	status_print(&s);
+  return s.commitable ? 0 : 1;
+}
diff --git a/builtin.h b/builtin.h
index 25431d7..53a896c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -47,6 +47,7 @@ extern int cmd_repo_config(int argc, con
 extern int cmd_rev_list(int argc, const char **argv, const char *prefix);
 extern int cmd_rev_parse(int argc, const char **argv, const char *prefix);
 extern int cmd_rm(int argc, const char **argv, const char *prefix);
+extern int cmd_runstatus(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
diff --git a/dir.c b/dir.c
index 5a40d8f..e2f472b 100644
--- a/dir.c
+++ b/dir.c
@@ -397,3 +397,10 @@ int read_directory(struct dir_struct *di
 	qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
 	return dir->nr;
 }
+
+int
+file_exists(const char *f)
+{
+  struct stat sb;
+  return stat(f, &sb) == 0;
+}
diff --git a/dir.h b/dir.h
index 56a1b7f..313f8ab 100644
--- a/dir.h
+++ b/dir.h
@@ -47,5 +47,6 @@ extern int excluded(struct dir_struct *,
 extern void add_excludes_from_file(struct dir_struct *, const char *fname);
 extern void add_exclude(const char *string, const char *base,
 			int baselen, struct exclude_list *which);
+extern int file_exists(const char *);
 
 #endif
diff --git a/git-commit.sh b/git-commit.sh
index 4cf3fab..10c269a 100755
--- a/git-commit.sh
+++ b/git-commit.sh
@@ -60,26 +60,6 @@ #
 }
 
 run_status () {
-    (
-	# We always show status for the whole tree.
-	cd "$TOP"
-
-	IS_INITIAL="$initial_commit"
-	REFERENCE=HEAD
-	case "$amend" in
-	t)
-		# If we are amending the initial commit, there
-		# is no HEAD^1.
-		if git-rev-parse --verify "HEAD^1" >/dev/null 2>&1
-		then
-			REFERENCE="HEAD^1"
-			IS_INITIAL=
-		else
-			IS_INITIAL=t
-		fi
-		;;
-	esac
-
 	# If TMP_INDEX is defined, that means we are doing
 	# "--only" partial commit, and that index file is used
 	# to build the tree for the commit.  Otherwise, if
@@ -96,85 +76,13 @@ run_status () {
 	    export GIT_INDEX_FILE
 	fi
 
-	case "$branch" in
-	refs/heads/master) ;;
-	*)  echo "# On branch $branch" ;;
-	esac
-
-	if test -z "$IS_INITIAL"
-	then
-	    git-diff-index -M --cached --name-status \
-		--diff-filter=MDTCRA $REFERENCE |
-	    sed -e '
-		    s/\\/\\\\/g
-		    s/ /\\ /g
-	    ' |
-	    report "Updated but not checked in" "will commit"
-	    committable="$?"
-	else
-	    echo '#
-# Initial commit
-#'
-	    git-ls-files |
-	    sed -e '
-		    s/\\/\\\\/g
-		    s/ /\\ /g
-		    s/^/A /
-	    ' |
-	    report "Updated but not checked in" "will commit"
-
-	    committable="$?"
-	fi
-
-	git-diff-files  --name-status |
-	sed -e '
-		s/\\/\\\\/g
-		s/ /\\ /g
-	' |
-	report "Changed but not updated" \
-	    "use git-update-index to mark for commit"
-
-        option=""
-        if test -z "$untracked_files"; then
-            option="--directory --no-empty-directory"
-        fi
-	hdr_shown=
-	if test -f "$GIT_DIR/info/exclude"
-	then
-	    git-ls-files --others $option \
-		--exclude-from="$GIT_DIR/info/exclude" \
-		--exclude-per-directory=.gitignore
-	else
-	    git-ls-files --others $option \
-		--exclude-per-directory=.gitignore
-	fi |
-	while read line; do
-	    if [ -z "$hdr_shown" ]; then
-		echo '#'
-		echo '# Untracked files:'
-		echo '#   (use "git add" to add to commit)'
-		echo '#'
-		hdr_shown=1
-	    fi
-	    echo "#	$line"
-	done
-
-	if test -n "$verbose" -a -z "$IS_INITIAL"
-	then
-	    git-diff-index --cached -M -p --diff-filter=MDTCRA $REFERENCE
-	fi
-	case "$committable" in
-	0)
-		case "$amend" in
-		t)
-			echo "# No changes" ;;
-		*)
-			echo "nothing to commit" ;;
-		esac
-		exit 1 ;;
-	esac
-	exit 0
-    )
+  case "$status_only" in
+    t) color= ;;
+    *) color=--nocolor ;;
+  esac
+  git-runstatus ${color} \
+                ${verbose:+--verbose} \
+                ${amend:+--amend}
 }
 
 trap '
diff --git a/git.c b/git.c
index 335f405..495b39a 100644
--- a/git.c
+++ b/git.c
@@ -252,6 +252,7 @@ static void handle_internal_command(int 
 		{ "rev-list", cmd_rev_list, RUN_SETUP },
 		{ "rev-parse", cmd_rev_parse, RUN_SETUP },
 		{ "rm", cmd_rm, RUN_SETUP },
+		{ "runstatus", cmd_runstatus, RUN_SETUP },
 		{ "show-branch", cmd_show_branch, RUN_SETUP },
 		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
 		{ "stripspace", cmd_stripspace },
diff --git a/status.c b/status.c
new file mode 100644
index 0000000..82aa881
--- /dev/null
+++ b/status.c
@@ -0,0 +1,283 @@
+#include "status.h"
+#include "color.h"
+#include "cache.h"
+#include "object.h"
+#include "dir.h"
+#include "commit.h"
+#include "diff.h"
+#include "revision.h"
+#include "diffcore.h"
+
+int status_use_color = 0;
+static char status_colors[][COLOR_MAXLEN] = {
+	"",         /* STATUS_HEADER: normal */
+	"\033[32m", /* STATUS_UPDATED: green */
+	"\033[31m", /* STATUS_CHANGED: red */
+	"\033[31m", /* STATUS_UNTRACKED: red */
+};
+
+static int
+parse_status_slot(const char *var, int offset) {
+	if (!strcasecmp(var+offset, "header"))
+		return STATUS_HEADER;
+	if (!strcasecmp(var+offset, "updated"))
+		return STATUS_UPDATED;
+	if (!strcasecmp(var+offset, "changed"))
+		return STATUS_CHANGED;
+	if (!strcasecmp(var+offset, "untracked"))
+		return STATUS_UNTRACKED;
+	die("bad config variable '%s'", var);
+}
+
+static const char*
+color(int slot)
+{
+	return status_use_color ? status_colors[slot] : "";
+}
+
+void
+status_prepare(struct status *s)
+{
+	unsigned char sha1[20];
+	const char *head;
+
+	s->is_initial = get_sha1("HEAD", sha1) ? 1 : 0;
+
+	head = resolve_ref(git_path("HEAD"), sha1, 0);
+	s->branch = head ?
+		    strdup(head + strlen(get_git_dir()) + 1) :
+		    NULL;
+
+	s->reference = "HEAD";
+	s->amend = 0;
+	s->verbose = 0;
+	s->commitable = 0;
+}
+
+static void
+status_print_header(const char *main, const char *sub)
+{
+	const char *c = color(STATUS_HEADER);
+	color_printf(c, "# %s:\n", main);
+	color_printf(c, "#   (%s)\n", sub);
+	color_printf(c, "#\n");
+}
+
+static void
+status_print_trailer(void)
+{
+	color_printf(color(STATUS_HEADER), "#\n");
+}
+
+static void
+status_print_filepair(int t, struct diff_filepair *p)
+{
+	const char *c = color(t);
+	color_printf(color(STATUS_HEADER), "#\t");
+	switch (p->status) {
+	case DIFF_STATUS_ADDED:
+		color_printf(c, "new file: %s\n", p->one->path); break;
+	case DIFF_STATUS_COPIED:
+		color_printf(c, "copied: %s -> %s\n",
+				p->one->path, p->two->path);
+		break;
+	case DIFF_STATUS_DELETED:
+		color_printf(c, "deleted: %s\n", p->one->path); break;
+	case DIFF_STATUS_MODIFIED:
+		color_printf(c, "modified: %s\n", p->one->path); break;
+	case DIFF_STATUS_RENAMED:
+		color_printf(c, "renamed: %s -> %s\n",
+				p->one->path, p->two->path);
+		break;
+	case DIFF_STATUS_TYPE_CHANGED:
+		color_printf(c, "typechange: %s\n", p->one->path); break;
+	case DIFF_STATUS_UNKNOWN:
+		color_printf(c, "unknown: %s\n", p->one->path); break;
+	case DIFF_STATUS_UNMERGED:
+		color_printf(c, "unmerged: %s\n", p->one->path); break;
+	default:
+		die("bug: unhandled diff status %c", p->status);
+	}
+}
+
+static void
+status_print_updated_cb(struct diff_queue_struct *q,
+                        struct diff_options *options,
+                        void *data)
+{
+	struct status *s = data;
+	int shown_header = 0;
+	int i;
+	if (q->nr) {
+	}
+	for (i = 0; i < q->nr; i++) {
+		if (q->queue[i]->status == 'U')
+			continue;
+		if (!shown_header) {
+			status_print_header("Updated but not checked in",
+					"will commit");
+			s->commitable = 1;
+		}
+		status_print_filepair(STATUS_UPDATED, q->queue[i]);
+	}
+	if (shown_header)
+		status_print_trailer();
+}
+
+static void
+status_print_changed_cb(struct diff_queue_struct *q,
+                        struct diff_options *options,
+                        void *data)
+{
+	int i;
+	if (q->nr)
+		status_print_header("Changed but not updated",
+				"use git-update-index to mark for commit");
+	for (i = 0; i < q->nr; i++)
+		status_print_filepair(STATUS_CHANGED, q->queue[i]);
+	if (q->nr)
+		status_print_trailer();
+}
+
+void
+status_print_initial(struct status *s)
+{
+	int i;
+	read_cache();
+	if (active_nr) {
+		s->commitable = 1;
+		status_print_header("Updated but not checked in",
+				"will commit");
+	}
+	for (i = 0; i < active_nr; i++) {
+		color_printf(color(STATUS_HEADER), "#\t");
+		color_printf(color(STATUS_UPDATED), "new file: %s\n",
+				active_cache[i]->name);
+	}
+	if (active_nr)
+		status_print_trailer();
+}
+
+static void
+status_print_updated(struct status *s)
+{
+	struct rev_info rev;
+	const char *argv[] = { NULL, NULL, NULL };
+	argv[1] = s->reference;
+	init_revisions(&rev, NULL);
+	setup_revisions(2, argv, &rev, NULL);
+	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = status_print_updated_cb;
+	rev.diffopt.format_callback_data = s;
+	rev.diffopt.detect_rename = 1;
+	run_diff_index(&rev, 1);
+}
+
+static void
+status_print_changed(struct status *s)
+{
+	struct rev_info rev;
+	const char *argv[] = { NULL, NULL };
+	init_revisions(&rev, "");
+	setup_revisions(1, argv, &rev, NULL);
+	rev.diffopt.output_format |= DIFF_FORMAT_CALLBACK;
+	rev.diffopt.format_callback = status_print_changed_cb;
+	rev.diffopt.format_callback_data = s;
+	run_diff_files(&rev, 0);
+}
+
+static void
+status_print_untracked(const struct status *s)
+{
+	struct dir_struct dir;
+	const char *x;
+	int i;
+	int shown_header = 0;
+
+	memset(&dir, 0, sizeof(dir));
+
+	dir.exclude_per_dir = ".gitignore";
+	x = git_path("info/exclude");
+	if (file_exists(x))
+		add_excludes_from_file(&dir, x);
+
+	read_directory(&dir, ".", "", 0);
+	for(i = 0; i < dir.nr; i++) {
+		/* check for matching entry, which is unmerged; lifted from
+		 * builtin-ls-files:show_other_files */
+		struct dir_entry *ent = dir.entries[i];
+		int pos = cache_name_pos(ent->name, ent->len);
+		struct cache_entry *ce;
+		if (0 <= pos)
+			die("bug in status_print_untracked");
+		pos = -pos - 1;
+		if (pos < active_nr) {
+			ce = active_cache[pos];
+			if (ce_namelen(ce) == ent->len &&
+			    !memcmp(ce->name, ent->name, ent->len))
+				continue;
+		}
+		if (!shown_header) {
+			status_print_header("Untracked files",
+				"use \"git add\" to add to commit");
+			shown_header = 1;
+		}
+		color_printf(color(STATUS_HEADER), "#\t");
+		color_printf(color(STATUS_UNTRACKED), "%.*s\n",
+				ent->len, ent->name);
+	}
+}
+
+static void
+status_print_verbose(struct status *s)
+{
+	struct rev_info rev;
+	const char *argv[] = { NULL, NULL, NULL };
+	argv[1] = s->reference;
+	init_revisions(&rev, NULL);
+	setup_revisions(2, argv, &rev, NULL);
+	rev.diffopt.output_format |= DIFF_FORMAT_PATCH;
+	rev.diffopt.detect_rename = 1;
+	run_diff_index(&rev, 1);
+}
+
+void
+status_print(struct status *s)
+{
+	if (s->branch && strcmp(s->branch, "refs/heads/master"))
+		color_printf(color(STATUS_HEADER),
+			"# On branch %s\n", s->branch);
+
+	if (s->is_initial) {
+		color_printf(color(STATUS_HEADER), "#\n");
+		color_printf(color(STATUS_HEADER), "# Initial commit\n");
+		color_printf(color(STATUS_HEADER), "#\n");
+		status_print_initial(s);
+	}
+	else {
+		status_print_updated(s);
+		discard_cache();
+	}
+
+	status_print_changed(s);
+	status_print_untracked(s);
+
+	if (s->verbose && !s->is_initial)
+		status_print_verbose(s);
+	if (!s->commitable)
+		printf("%s\n", s->amend ? "# No changes" : "nothing to commit");
+}
+
+int
+git_status_config(const char *k, const char *v)
+{
+	if (!strcmp(k, "status.color")) {
+		status_use_color = git_config_colorbool(k, v);
+		return 0;
+	}
+	if (!strncmp(k, "status.color.", 13)) {
+		int slot = parse_status_slot(k, 13);
+		color_parse(v, k, status_colors[slot]);
+	}
+	return git_default_config(k, v);
+}
diff --git a/status.h b/status.h
new file mode 100644
index 0000000..3e60146
--- /dev/null
+++ b/status.h
@@ -0,0 +1,31 @@
+#ifndef STATUS_H
+#define STATUS_H
+
+enum color_status {
+	STATUS_HEADER,
+	STATUS_UPDATED,
+	STATUS_CHANGED,
+	STATUS_UNTRACKED,
+};
+
+struct status {
+	int is_initial;
+	char *branch;
+	const char *reference;
+	int commitable;
+	int verbose;
+	int amend;
+};
+
+int git_status_config(const char *var, const char *value);
+void status_prepare(struct status *s);
+void status_print(struct status *s);
+
+struct cache_entry;
+typedef void (*status_cb)(struct cache_entry *);
+int status_foreach_cached(status_cb cb);
+int status_foreach_updated(status_cb cb);
+int status_foreach_changed(status_cb cb);
+int status_foreach_untracked(status_cb cb);
+
+#endif /* STATUS_H */
-- 
1.4.2.ge490e-dirty

Re: [PATCH 3/3] git-commit.sh: convert run_status to a C builtin

[Jeff King]

On Thu, Sep 07, 2006 at 02:36:21AM -0400, Jeff King wrote:

> +static void
> +status_print_updated_cb(struct diff_queue_struct *q,
> +                        struct diff_options *options,
> +                        void *data)
> [...]
> +		if (!shown_header) {
> +			status_print_header("Updated but not checked in",
> +					"will commit");
> +			s->commitable = 1;
> +		}

Sorry, this should set shown_header=1. That's what I get for making a
last minute change and not testing...

-Peff

Re: [PATCH 3/3] git-commit.sh: convert run_status to a C builtin

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> diff --git a/Makefile b/Makefile
> index 78748cb..d0ba3b5 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -252,7 +252,7 @@ LIB_OBJS = \
>  	fetch-clone.o revision.o pager.o tree-walk.o xdiff-interface.o \
>  	write_or_die.o trace.o \
>  	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
> -	color.o
> +	color.o status.o
>  
>  BUILTIN_OBJS = \
>  	builtin-add.o \

At some point (this does not have to be part of this series), we
should demote DIFF_OBJS to just ordinary LIB_OBJS.  They used to
be special in that they are used only by handful special
commands, but these days more and more things are integrated and
it outlived its usefulness.

> +static const char runstatus_usage[] =
> +"git-runstatus [--color|--nocolor] [--amend] [--verbose]";
> +
> +int cmd_runstatus(int argc, const char **argv, const char *prefix)
> +{
> +	struct status s;
> +	int i;
> +
> +	git_config(git_status_config);
> +	status_prepare(&s);
> +
> +	for (i = 1; i < argc; i++) {
> +		if (!strcmp(argv[i], "--color"))
> +			status_use_color = 1;
> +		else if (!strcmp(argv[i], "--nocolor"))
> +			status_use_color = 0;
> +    else if (!strcmp(argv[i], "--amend")) {
> +      s.amend = 1;
> +      s.reference = "HEAD^1";
> +    }
> +    else if (!strcmp(argv[i], "--verbose"))
> +      s.verbose = 1;
> +		else
> +			usage(runstatus_usage);
> +	}
> +
> +	status_print(&s);
> +  return s.commitable ? 0 : 1;
> +}

Quite funny indentation style your MUA likes ;-).

> diff --git a/status.c b/status.c
> new file mode 100644
> index 0000000..82aa881
> --- /dev/null
> +++ b/status.c
> @@ -0,0 +1,283 @@
> +#include "status.h"

"status.h" and "struct status" somehow sounds too broad.
Granted, "object.h" is also broad, but in git context "object"
has a specific meaning.

Having said that I cannot come up with a good alternative name.
It is not "project status" nor "repository status".  It is
"working tree status", but that sounds very loooooooooooooooong.

> +	color_printf(color(STATUS_HEADER), "#\t");
> +	switch (p->status) {
> +	case DIFF_STATUS_ADDED:
> +		color_printf(c, "new file: %s\n", p->one->path); break;

Very nicely done.  Especially I liked that you are careful not
to paint leading '#\t' (which is noticeable when you use reverse
as an attribute).

> +static void
> +status_print_untracked(const struct status *s)
> +{
> +	struct dir_struct dir;
> +	const char *x;
> +	int i;
> +	int shown_header = 0;
> +
> +	memset(&dir, 0, sizeof(dir));
> +
> +	dir.exclude_per_dir = ".gitignore";
> +	x = git_path("info/exclude");
> +	if (file_exists(x))
> +		add_excludes_from_file(&dir, x);
> +
> +	read_directory(&dir, ".", "", 0);
> +	for(i = 0; i < dir.nr; i++) {
> +		/* check for matching entry, which is unmerged; lifted from
> +		 * builtin-ls-files:show_other_files */
> +		struct dir_entry *ent = dir.entries[i];
> +		int pos = cache_name_pos(ent->name, ent->len);
> +		struct cache_entry *ce;
> +		if (0 <= pos)
> +			die("bug in status_print_untracked");
> +		pos = -pos - 1;
> +		if (pos < active_nr) {
> +			ce = active_cache[pos];
> +			if (ce_namelen(ce) == ent->len &&
> +			    !memcmp(ce->name, ent->name, ent->len))
> +				continue;
> +		}
> +		if (!shown_header) {
> +			status_print_header("Untracked files",
> +				"use \"git add\" to add to commit");
> +			shown_header = 1;
> +		}
> +		color_printf(color(STATUS_HEADER), "#\t");
> +		color_printf(color(STATUS_UNTRACKED), "%.*s\n",
> +				ent->len, ent->name);
> +	}
> +}

Very nice code reuse.  I do not mean sarcasm -- the part copied
and pasted from ls-files is almost trivial to bother factoring
out.  What's nice is read_directory() does all what is needed to
deal with .gitignore files, which I forgot almost all about.

> +int status_foreach_cached(status_cb cb);
> +int status_foreach_updated(status_cb cb);
> +int status_foreach_changed(status_cb cb);
> +int status_foreach_untracked(status_cb cb);

I do not see them defined nor used...

I'll take only [1/3] for now but I am interested in 2 and 3.

Re: [PATCH 3/3] git-commit.sh: convert run_status to a C builtin

[Jeff King]

On Thu, Sep 07, 2006 at 05:20:20PM -0700, Junio C Hamano wrote:

> "status.h" and "struct status" somehow sounds too broad.
> Granted, "object.h" is also broad, but in git context "object"
> has a specific meaning.

I agree it is quite broad (as is git-runstatus). Conceptually it's
another type of diff format, but making it a diff argument doesn't
really makes much sense. We're at least not introducing any broadness,
since there is already git-status; are we interested in fixing that
name?

Names for similar concepts from other systems: tree-lint (arch),
inventory (arch), whatsnew (darcs, perhaps a little too close to
whatchanged), status (svn, hg), update -n (cvs, ugh!).

> Having said that I cannot come up with a good alternative name.
> It is not "project status" nor "repository status".  It is
> "working tree status", but that sounds very loooooooooooooooong.

wt_status? ucu_status (updated, changed, untracked)?

> Very nicely done.  Especially I liked that you are careful not
> to paint leading '#\t' (which is noticeable when you use reverse
> as an attribute).

Yes. I seem to recall some issues raised about color attributes
persisting over a newline, but I can't find any reference to it now.
Using color_printf makes sure every color is 'closed' but it sometimes
includes the newline in the colorized portion. Does anybody object to
that?


> Very nice code reuse.  I do not mean sarcasm -- the part copied
> and pasted from ls-files is almost trivial to bother factoring

Yes, I don't like cutting and pasting, but it seemed more confusing to
try factoring it out. It's not a lot of *code*, it's just that
remembering to pick out unmerged entries (and the weird -pos bit) is
confusing.

> out.  What's nice is read_directory() does all what is needed to
> deal with .gitignore files, which I forgot almost all about.

I just had to write a file_exists to check for .git/gitignore, which was
a little clumsy.

> > +int status_foreach_cached(status_cb cb);
> > +int status_foreach_updated(status_cb cb);
> > +int status_foreach_changed(status_cb cb);
> > +int status_foreach_untracked(status_cb cb);
> I do not see them defined nor used...

My fault, they were left in from a previous iteration.

> I'll take only [1/3] for now but I am interested in 2 and 3.

OK. Besides the things you mentioned, what improvements would you like
to see?

-Peff

Re: [PATCH 3/3] git-commit.sh: convert run_status to a C builtin

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Thu, Sep 07, 2006 at 05:20:20PM -0700, Junio C Hamano wrote:
>
>> "status.h" and "struct status" somehow sounds too broad.
>> Granted, "object.h" is also broad, but in git context "object"
>> has a specific meaning.
>
> I agree it is quite broad (as is git-runstatus). Conceptually it's
> another type of diff format, but making it a diff argument doesn't
> really makes much sense. We're at least not introducing any broadness,
> since there is already git-status; are we interested in fixing that
> name?

The command name is a name exposed to the end user.  We do not
currently have a command to give "repository status", and even
if we had one such a specialized command for repository
administrators would be called git-repository-status so
git-status is definitely fine as is.

I just wanted to point it out because I felt the names to
programmers are slightly different matter.

> wt_status? ucu_status (updated, changed, untracked)?

Yeah, something along those lines.

>> Very nicely done.  Especially I liked that you are careful not
>> to paint leading '#\t' (which is noticeable when you use reverse
>> as an attribute).
>
> Yes. I seem to recall some issues raised about color attributes
> persisting over a newline, but I can't find any reference to it now.
> Using color_printf makes sure every color is 'closed' but it sometimes
> includes the newline in the colorized portion. Does anybody object to
> that?

In a distant past I saw some terminals get confused near the
edge if you do that, but these days everybody is on some sort of
Xterm so it may not matter.  But that would probably be nice to
fix.

> OK. Besides the things you mentioned, what improvements would you like
> to see?

Besides the things I mentioned?  I dunno offhand -- otherwise I
would have mentioned them ;-).

Re: [PATCH 3/3] git-commit.sh: convert run_status to a C builtin

[Jeff King]

On Thu, Sep 07, 2006 at 10:56:11PM -0700, Junio C Hamano wrote:

> I just wanted to point it out because I felt the names to
> programmers are slightly different matter.

Fair enough. Do you want to change git-runstatus? It should ideally not
be used by end users at all (and hopefully can eventually go away if
git-status and git-commit become builtins).

> > wt_status? ucu_status (updated, changed, untracked)?
> Yeah, something along those lines.

I think wt_status is the most reasonable of those.

> In a distant past I saw some terminals get confused near the
> edge if you do that, but these days everybody is on some sort of
> Xterm so it may not matter.  But that would probably be nice to
> fix.

OK, it will take a little wrangling with the color_printf interface, but
it should be doable.

> > OK. Besides the things you mentioned, what improvements would you like
> > to see?
> Besides the things I mentioned?  I dunno offhand -- otherwise I
> would have mentioned them ;-).

Heh. OK, I assumed your previous comment hinted at hidden depths of
dissatisfaction, but clearly not. :) Patches forthcoming...

-Peff

[PATCH] Move color option parsing out of diff.c and into color.[ch]

[Jeff King]

The intent is to lib-ify colorizing code so it can be reused.

Signed-off-by: Jeff King <peff@peff.net>
---
This is a resend with formatting cleanups and a new function,
color_printf_ln.

 Makefile |    3 +
 color.c  |  176 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 color.h  |   12 ++++
 diff.c   |  136 +-----------------------------------------------
 4 files changed, 194 insertions(+), 133 deletions(-)

diff --git a/Makefile b/Makefile
index 7b3114f..78748cb 100644
--- a/Makefile
+++ b/Makefile
@@ -251,7 +251,8 @@ LIB_OBJS = \
 	tag.o tree.o usage.o config.o environment.o ctype.o copy.o \
 	fetch-clone.o revision.o pager.o tree-walk.o xdiff-interface.o \
 	write_or_die.o trace.o \
-	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS)
+	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \
+	color.o
 
 BUILTIN_OBJS = \
 	builtin-add.o \
diff --git a/color.c b/color.c
new file mode 100644
index 0000000..a949136
--- /dev/null
+++ b/color.c
@@ -0,0 +1,176 @@
+#include "color.h"
+#include "cache.h"
+#include "git-compat-util.h"
+
+#include <stdarg.h>
+
+#define COLOR_RESET "\033[m"
+
+static int parse_color(const char *name, int len)
+{
+	static const char * const color_names[] = {
+		"normal", "black", "red", "green", "yellow",
+		"blue", "magenta", "cyan", "white"
+	};
+	char *end;
+	int i;
+	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
+		const char *str = color_names[i];
+		if (!strncasecmp(name, str, len) && !str[len])
+			return i - 1;
+	}
+	i = strtol(name, &end, 10);
+	if (*name && !*end && i >= -1 && i <= 255)
+		return i;
+	return -2;
+}
+
+static int parse_attr(const char *name, int len)
+{
+	static const int attr_values[] = { 1, 2, 4, 5, 7 };
+	static const char * const attr_names[] = {
+		"bold", "dim", "ul", "blink", "reverse"
+	};
+	int i;
+	for (i = 0; i < ARRAY_SIZE(attr_names); i++) {
+		const char *str = attr_names[i];
+		if (!strncasecmp(name, str, len) && !str[len])
+			return attr_values[i];
+	}
+	return -1;
+}
+
+void color_parse(const char *value, const char *var, char *dst)
+{
+	const char *ptr = value;
+	int attr = -1;
+	int fg = -2;
+	int bg = -2;
+
+	if (!strcasecmp(value, "reset")) {
+		strcpy(dst, "\033[m");
+		return;
+	}
+
+	/* [fg [bg]] [attr] */
+	while (*ptr) {
+		const char *word = ptr;
+		int val, len = 0;
+
+		while (word[len] && !isspace(word[len]))
+			len++;
+
+		ptr = word + len;
+		while (*ptr && isspace(*ptr))
+			ptr++;
+
+		val = parse_color(word, len);
+		if (val >= -1) {
+			if (fg == -2) {
+				fg = val;
+				continue;
+			}
+			if (bg == -2) {
+				bg = val;
+				continue;
+			}
+			goto bad;
+		}
+		val = parse_attr(word, len);
+		if (val < 0 || attr != -1)
+			goto bad;
+		attr = val;
+	}
+
+	if (attr >= 0 || fg >= 0 || bg >= 0) {
+		int sep = 0;
+
+		*dst++ = '\033';
+		*dst++ = '[';
+		if (attr >= 0) {
+			*dst++ = '0' + attr;
+			sep++;
+		}
+		if (fg >= 0) {
+			if (sep++)
+				*dst++ = ';';
+			if (fg < 8) {
+				*dst++ = '3';
+				*dst++ = '0' + fg;
+			} else {
+				dst += sprintf(dst, "38;5;%d", fg);
+			}
+		}
+		if (bg >= 0) {
+			if (sep++)
+				*dst++ = ';';
+			if (bg < 8) {
+				*dst++ = '4';
+				*dst++ = '0' + bg;
+			} else {
+				dst += sprintf(dst, "48;5;%d", bg);
+			}
+		}
+		*dst++ = 'm';
+	}
+	*dst = 0;
+	return;
+bad:
+	die("bad config value '%s' for variable '%s'", value, var);
+}
+
+int git_config_colorbool(const char *var, const char *value)
+{
+	if (!value)
+		return 1;
+	if (!strcasecmp(value, "auto")) {
+		if (isatty(1) || (pager_in_use && pager_use_color)) {
+			char *term = getenv("TERM");
+			if (term && strcmp(term, "dumb"))
+				return 1;
+		}
+		return 0;
+	}
+	if (!strcasecmp(value, "never"))
+		return 0;
+	if (!strcasecmp(value, "always"))
+		return 1;
+	return git_config_bool(var, value);
+}
+
+static int color_vprintf(const char *color, const char *fmt,
+		va_list args, const char *trail)
+{
+	int r = 0;
+
+	if (*color)
+		r += printf("%s", color);
+	r += vprintf(fmt, args);
+	if (trail)
+		r += printf("%s", trail);
+	if (*color)
+		r += printf("%s", COLOR_RESET);
+	return r;
+}
+
+
+
+int color_printf(const char *color, const char *fmt, ...)
+{
+	va_list args;
+	int r;
+	va_start(args, fmt);
+	r = color_vprintf(color, fmt, args, 0);
+	va_end(args);
+	return r;
+}
+
+int color_printf_ln(const char *color, const char *fmt, ...)
+{
+	va_list args;
+	int r;
+	va_start(args, fmt);
+	r = color_vprintf(color, fmt, args, "\n");
+	va_end(args);
+	return r;
+}
diff --git a/color.h b/color.h
new file mode 100644
index 0000000..88bb8ff
--- /dev/null
+++ b/color.h
@@ -0,0 +1,12 @@
+#ifndef COLOR_H
+#define COLOR_H
+
+/* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
+#define COLOR_MAXLEN 24
+
+int git_config_colorbool(const char *var, const char *value);
+void color_parse(const char *var, const char *value, char *dst);
+int color_printf(const char *color, const char *fmt, ...);
+int color_printf_ln(const char *color, const char *fmt, ...);
+
+#endif /* COLOR_H */
diff --git a/diff.c b/diff.c
index a3ebc62..8178c11 100644
--- a/diff.c
+++ b/diff.c
@@ -10,6 +10,7 @@ #include "diff.h"
 #include "diffcore.h"
 #include "delta.h"
 #include "xdiff-interface.h"
+#include "color.h"
 
 static int use_size_cache;
 
@@ -17,8 +18,7 @@ static int diff_detect_rename_default;
 static int diff_rename_limit_default = -1;
 static int diff_use_color_default;
 
-/* "\033[1;38;5;2xx;48;5;2xxm\0" is 23 bytes */
-static char diff_colors[][24] = {
+static char diff_colors[][COLOR_MAXLEN] = {
 	"\033[m",	/* reset */
 	"",		/* normal */
 	"\033[1m",	/* bold */
@@ -45,119 +45,6 @@ static int parse_diff_color_slot(const c
 	die("bad config variable '%s'", var);
 }
 
-static int parse_color(const char *name, int len)
-{
-	static const char * const color_names[] = {
-		"normal", "black", "red", "green", "yellow",
-		"blue", "magenta", "cyan", "white"
-	};
-	char *end;
-	int i;
-	for (i = 0; i < ARRAY_SIZE(color_names); i++) {
-		const char *str = color_names[i];
-		if (!strncasecmp(name, str, len) && !str[len])
-			return i - 1;
-	}
-	i = strtol(name, &end, 10);
-	if (*name && !*end && i >= -1 && i <= 255)
-		return i;
-	return -2;
-}
-
-static int parse_attr(const char *name, int len)
-{
-	static const int attr_values[] = { 1, 2, 4, 5, 7 };
-	static const char * const attr_names[] = {
-		"bold", "dim", "ul", "blink", "reverse"
-	};
-	int i;
-	for (i = 0; i < ARRAY_SIZE(attr_names); i++) {
-		const char *str = attr_names[i];
-		if (!strncasecmp(name, str, len) && !str[len])
-			return attr_values[i];
-	}
-	return -1;
-}
-
-static void parse_diff_color_value(const char *value, const char *var, char *dst)
-{
-	const char *ptr = value;
-	int attr = -1;
-	int fg = -2;
-	int bg = -2;
-
-	if (!strcasecmp(value, "reset")) {
-		strcpy(dst, "\033[m");
-		return;
-	}
-
-	/* [fg [bg]] [attr] */
-	while (*ptr) {
-		const char *word = ptr;
-		int val, len = 0;
-
-		while (word[len] && !isspace(word[len]))
-			len++;
-
-		ptr = word + len;
-		while (*ptr && isspace(*ptr))
-			ptr++;
-
-		val = parse_color(word, len);
-		if (val >= -1) {
-			if (fg == -2) {
-				fg = val;
-				continue;
-			}
-			if (bg == -2) {
-				bg = val;
-				continue;
-			}
-			goto bad;
-		}
-		val = parse_attr(word, len);
-		if (val < 0 || attr != -1)
-			goto bad;
-		attr = val;
-	}
-
-	if (attr >= 0 || fg >= 0 || bg >= 0) {
-		int sep = 0;
-
-		*dst++ = '\033';
-		*dst++ = '[';
-		if (attr >= 0) {
-			*dst++ = '0' + attr;
-			sep++;
-		}
-		if (fg >= 0) {
-			if (sep++)
-				*dst++ = ';';
-			if (fg < 8) {
-				*dst++ = '3';
-				*dst++ = '0' + fg;
-			} else {
-				dst += sprintf(dst, "38;5;%d", fg);
-			}
-		}
-		if (bg >= 0) {
-			if (sep++)
-				*dst++ = ';';
-			if (bg < 8) {
-				*dst++ = '4';
-				*dst++ = '0' + bg;
-			} else {
-				dst += sprintf(dst, "48;5;%d", bg);
-			}
-		}
-		*dst++ = 'm';
-	}
-	*dst = 0;
-	return;
-bad:
-	die("bad config value '%s' for variable '%s'", value, var);
-}
-
 /*
  * These are to give UI layer defaults.
  * The core-level commands such as git-diff-files should
@@ -171,22 +58,7 @@ int git_diff_ui_config(const char *var, 
 		return 0;
 	}
 	if (!strcmp(var, "diff.color")) {
-		if (!value)
-			diff_use_color_default = 1; /* bool */
-		else if (!strcasecmp(value, "auto")) {
-			diff_use_color_default = 0;
-			if (isatty(1) || (pager_in_use && pager_use_color)) {
-				char *term = getenv("TERM");
-				if (term && strcmp(term, "dumb"))
-					diff_use_color_default = 1;
-			}
-		}
-		else if (!strcasecmp(value, "never"))
-			diff_use_color_default = 0;
-		else if (!strcasecmp(value, "always"))
-			diff_use_color_default = 1;
-		else
-			diff_use_color_default = git_config_bool(var, value);
+		diff_use_color_default = git_config_colorbool(var, value);
 		return 0;
 	}
 	if (!strcmp(var, "diff.renames")) {
@@ -201,7 +73,7 @@ int git_diff_ui_config(const char *var, 
 	}
 	if (!strncmp(var, "diff.color.", 11)) {
 		int slot = parse_diff_color_slot(var, 11);
-		parse_diff_color_value(value, var, diff_colors[slot]);
+		color_parse(value, var, diff_colors[slot]);
 		return 0;
 	}
 	return git_default_config(var, value);
-- 
1.4.2.g5290b

Re: [PATCH] Move color option parsing out of diff.c and into color.[ch]

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> +static int color_vprintf(const char *color, const char *fmt,
> +		va_list args, const char *trail)
> +{
> +	int r = 0;
> +
> +	if (*color)
> +		r += printf("%s", color);
> +	r += vprintf(fmt, args);
> +	if (trail)
> +		r += printf("%s", trail);
> +	if (*color)
> +		r += printf("%s", COLOR_RESET);
> +	return r;
> +}

Hmm,... don't you mean RESET first and then trail (which is often "\n")?

> +int color_printf(const char *color, const char *fmt, ...)
> +{
> +	va_list args;
> +	int r;
> +	va_start(args, fmt);
> +	r = color_vprintf(color, fmt, args, 0);
> +	va_end(args);
> +	return r;
> +}

Please spell NULL not 0 (please do not argue that writing a NULL
pointer as integral constant 0 is perfectly valid C -- we all
know that.  This is not the language-lawyers correctness issue,
but ../linux-2.6/Documentation/CodingStyle thing).  Using
pointer as boolean seems to be Ok style (e.g. "if (trail)" does
not have to be written "if (trail == NULL)").

Re: [PATCH] Move color option parsing out of diff.c and into color.[ch]

[Jeff King]

On Fri, Sep 08, 2006 at 01:49:20AM -0700, Junio C Hamano wrote:

> > +	if (trail)
> > +		r += printf("%s", trail);
> > +	if (*color)
> > +		r += printf("%s", COLOR_RESET);
> > +	return r;
> > +}
> Hmm,... don't you mean RESET first and then trail (which is often "\n")?

ARGH. Yes, sorry about that. I wrote it correctly once, and then
refactored it to be wrong. :)

> Please spell NULL not 0 (please do not argue that writing a NULL

Sorry, I should clearly go read the linux CodingStyle doc before
subjecting you to any more of my patches.

I'm assuming you can mark both of those up yourself rather than having
me resend?

-Peff

Re: [PATCH] Move color option parsing out of diff.c and into color.[ch]

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm assuming you can mark both of those up yourself rather than having
> me resend?

Sure, that's easy enough.

Thanks for the patch.