[PATCH 1/4] Add testcase for ammending and fixing author in git commit.

[PATCH 1/4] Add testcase for ammending and fixing author in git commit.

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 t/t7501-commit.sh |   10 ++++++++++
 1 files changed, 10 insertions(+), 0 deletions(-)

diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
index b151b51..3f2112a 100644
--- a/t/t7501-commit.sh
+++ b/t/t7501-commit.sh
@@ -163,4 +163,14 @@ test_expect_success 'partial commit that involves removal (3)' '
 
 '
 
+author="The Real Author <someguy@his.email.org>"
+test_expect_success 'amend commit to fix author' '
+
+	git reset --hard
+	git cat-file -p HEAD | sed -e "s/author.*>/author $author/" > expected &&
+	git commit --amend --author="$author" &&
+	git cat-file -p HEAD > current &&
+	diff expected current
+	
+'
 test_done
-- 
1.5.3.4.206.g58ba4

[PATCH 2/4] Remove unecessary hard-coding of EDITOR=':' VISUAL=':' in some test suites.

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 t/t3501-revert-cherry-pick.sh |    4 ++--
 t/t3901-i18n-patch.sh         |    8 ++++----
 2 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t3501-revert-cherry-pick.sh b/t/t3501-revert-cherry-pick.sh
index 552af1c..2dbe04f 100755
--- a/t/t3501-revert-cherry-pick.sh
+++ b/t/t3501-revert-cherry-pick.sh
@@ -44,7 +44,7 @@ test_expect_success setup '
 test_expect_success 'cherry-pick after renaming branch' '
 
 	git checkout rename2 &&
-	EDITOR=: VISUAL=: git cherry-pick added &&
+	git cherry-pick added &&
 	test -f opos &&
 	grep "Add extra line at the end" opos
 
@@ -53,7 +53,7 @@ test_expect_success 'cherry-pick after renaming branch' '
 test_expect_success 'revert after renaming branch' '
 
 	git checkout rename1 &&
-	EDITOR=: VISUAL=: git revert added &&
+	git revert added &&
 	test -f spoo &&
 	! grep "Add extra line at the end" spoo
 
diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
index 28e9e37..235f372 100755
--- a/t/t3901-i18n-patch.sh
+++ b/t/t3901-i18n-patch.sh
@@ -154,7 +154,7 @@ test_expect_success 'cherry-pick(U/U)' '
 	git reset --hard master &&
 	git cherry-pick side^ &&
 	git cherry-pick side &&
-	EDITOR=: VISUAL=: git revert HEAD &&
+	git revert HEAD &&
 
 	check_encoding 3
 '
@@ -169,7 +169,7 @@ test_expect_success 'cherry-pick(L/L)' '
 	git reset --hard master &&
 	git cherry-pick side^ &&
 	git cherry-pick side &&
-	EDITOR=: VISUAL=: git revert HEAD &&
+	git revert HEAD &&
 
 	check_encoding 3 8859
 '
@@ -184,7 +184,7 @@ test_expect_success 'cherry-pick(U/L)' '
 	git reset --hard master &&
 	git cherry-pick side^ &&
 	git cherry-pick side &&
-	EDITOR=: VISUAL=: git revert HEAD &&
+	git revert HEAD &&
 
 	check_encoding 3
 '
@@ -200,7 +200,7 @@ test_expect_success 'cherry-pick(L/U)' '
 	git reset --hard master &&
 	git cherry-pick side^ &&
 	git cherry-pick side &&
-	EDITOR=: VISUAL=: git revert HEAD &&
+	git revert HEAD &&
 
 	check_encoding 3 8859
 '
-- 
1.5.3.4.206.g58ba4

[PATCH 3/4] Export launch_editor() and make it accept ':' as a no-op editor.

[Kristian Høgsberg]

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 builtin-tag.c |    5 ++++-
 strbuf.h      |    1 +
 2 files changed, 5 insertions(+), 1 deletions(-)

diff --git a/builtin-tag.c b/builtin-tag.c
index 66e5a58..bcb3a7a 100644
--- a/builtin-tag.c
+++ b/builtin-tag.c
@@ -17,7 +17,7 @@ static const char builtin_tag_usage[] =
 
 static char signingkey[1000];
 
-static void launch_editor(const char *path, struct strbuf *buffer)
+void launch_editor(const char *path, struct strbuf *buffer)
 {
 	const char *editor, *terminal;
 	struct child_process child;
@@ -42,6 +42,9 @@ static void launch_editor(const char *path, struct strbuf *buffer)
 	if (!editor)
 		editor = "vi";
 
+	if (!strcmp(editor, ":"))
+		return;
+
 	memset(&child, 0, sizeof(child));
 	child.argv = args;
 	args[0] = editor;
diff --git a/strbuf.h b/strbuf.h
index 9b9e861..9720826 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -113,5 +113,6 @@ extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);
 extern int strbuf_getline(struct strbuf *, FILE *, int);
 
 extern void stripspace(struct strbuf *buf, int skip_comments);
+extern void launch_editor(const char *path, struct strbuf *buffer);
 
 #endif /* STRBUF_H */
-- 
1.5.3.4.206.g58ba4

[PATCH 4/4] Implement git commit and status as a builtin commands.

[Kristian Høgsberg]

Move git-commit.sh to contrib/examples.  This also removes the git-runstatus
helper, which was mostly just a git-status.sh implementation detail.

Signed-off-by: Kristian Høgsberg <krh@redhat.com>
---
 Makefile                                        |    9 +-
 builtin-commit.c                                |  614 +++++++++++++++++++++++
 builtin.h                                       |    3 +-
 git-commit.sh => contrib/examples/git-commit.sh |    0 
 git.c                                           |    3 +-
 5 files changed, 621 insertions(+), 8 deletions(-)
 create mode 100644 builtin-commit.c
 rename git-commit.sh => contrib/examples/git-commit.sh (100%)

diff --git a/Makefile b/Makefile
index 042f79e..689eb33 100644
--- a/Makefile
+++ b/Makefile
@@ -209,7 +209,7 @@ BASIC_LDFLAGS =
 
 SCRIPT_SH = \
 	git-bisect.sh git-checkout.sh \
-	git-clean.sh git-clone.sh git-commit.sh \
+	git-clean.sh git-clone.sh \
 	git-ls-remote.sh \
 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \
 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \
@@ -256,7 +256,7 @@ EXTRA_PROGRAMS =
 BUILT_INS = \
 	git-format-patch$X git-show$X git-whatchanged$X git-cherry$X \
 	git-get-tar-commit-id$X git-init$X git-repo-config$X \
-	git-fsck-objects$X git-cherry-pick$X \
+	git-fsck-objects$X git-cherry-pick$X git-status$X\
 	$(patsubst builtin-%.o,git-%$X,$(BUILTIN_OBJS))
 
 # what 'all' will build and 'install' will install, in gitexecdir
@@ -326,6 +326,7 @@ BUILTIN_OBJS = \
 	builtin-check-attr.o \
 	builtin-checkout-index.o \
 	builtin-check-ref-format.o \
+	builtin-commit.o \
 	builtin-commit-tree.o \
 	builtin-count-objects.o \
 	builtin-describe.o \
@@ -364,7 +365,6 @@ BUILTIN_OBJS = \
 	builtin-rev-parse.o \
 	builtin-revert.o \
 	builtin-rm.o \
-	builtin-runstatus.o \
 	builtin-shortlog.o \
 	builtin-show-branch.o \
 	builtin-stripspace.o \
@@ -830,9 +830,6 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	chmod +x $@+ && \
 	mv $@+ $@
 
-git-status: git-commit
-	$(QUIET_GEN)cp $< $@+ && mv $@+ $@
-
 gitweb/gitweb.cgi: gitweb/gitweb.perl
 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
diff --git a/builtin-commit.c b/builtin-commit.c
new file mode 100644
index 0000000..a5a9e3e
--- /dev/null
+++ b/builtin-commit.c
@@ -0,0 +1,614 @@
+/*
+ * Builtin "git commit"
+ *
+ * Copyright (c) 2007 Kristian Høgsberg <krh@redhat.com>
+ * Based on git-commit.sh by Junio C Hamano and Linus Torvalds
+ */
+
+#include "git-compat-util.h"
+
+#include "cache.h"
+#include "cache-tree.h"
+#include "builtin.h"
+#include "diff.h"
+#include "diffcore.h"
+#include "commit.h"
+#include "revision.h"
+#include "wt-status.h"
+#include "run-command.h"
+#include "refs.h"
+#include "log-tree.h"
+#include "strbuf.h"
+#include "utf8.h"
+#include "parse-options.h"
+
+static const char * const builtin_commit_usage[] = {
+	"git-commit [options] [--] <filepattern>...",
+	NULL
+};
+
+static unsigned char head_sha1[20], merge_head_sha1[20];
+static char *use_message_buffer;
+static const char commit_editmsg[] = "COMMIT_EDITMSG";
+static struct lock_file lock_file;
+
+static char *logfile, *force_author, *message, *template_file;
+static char *edit_message, *use_message;
+static int all, edit_flag, also, interactive, only, amend, signoff;
+static int quiet, verbose, untracked_files, no_verify;
+
+static int no_edit, initial_commit, in_merge;
+const char *only_include_assumed;
+
+static struct option builtin_commit_options[] = {
+	OPT__QUIET(&quiet),
+	OPT__VERBOSE(&verbose),
+	OPT_GROUP("Commit message options"),
+
+	OPT_STRING('F', "file", &logfile, "FILE", "read log from file"),
+	OPT_STRING(0, "author", &force_author, "AUTHOR", "override author for commit"),
+	OPT_STRING('m', "message", &message, "MESSAGE", "specify commit message"),
+	OPT_STRING('c', "reedit-message", &edit_message, "COMMIT", "reuse and edit message from specified commit "),
+	OPT_STRING('C', "reuse-message", &use_message, "COMMIT", "reuse message from specified commit"),
+	OPT_BOOLEAN('s', "signoff", &signoff, "add Signed-off-by: header"),
+	OPT_STRING('t', "template", &template_file, "FILE", "use specified template file"),
+	OPT_BOOLEAN('e', "edit", &edit_flag, "force edit of commit"),
+
+	OPT_GROUP("Commit contents options"),
+	OPT_BOOLEAN('a', "all", &all, "commit all changed files"),
+	OPT_BOOLEAN('i', "include", &also, "add specified files to index for commit"),
+	OPT_BOOLEAN(0, "interactive", &interactive, "interactively add files"),
+	OPT_BOOLEAN('o', "only", &only, ""),
+	OPT_BOOLEAN('n', "no-verify", &no_verify, "bypass pre-commit hook"),
+	OPT_BOOLEAN(0, "amend", &amend, "amend previous commit"),
+	OPT_BOOLEAN(0, "untracked-files", &untracked_files, "show all untracked files"),
+
+	OPT_END()
+};
+
+static char *
+prepare_index(const char **files, const char *prefix)
+{
+	int fd;
+	struct tree *tree;
+	struct lock_file *next_index_lock;
+
+	fd = hold_locked_index(&lock_file, 1);
+	if (read_cache() < 0)
+		die("index file corrupt");
+
+	if (all) {
+		add_files_to_cache(verbose, NULL, files);
+		if (write_cache(fd, active_cache, active_nr) || close(fd))
+			die("unable to write new_index file");
+		return lock_file.filename;
+	} else if (also) {
+		add_files_to_cache(verbose, prefix, files);
+		if (write_cache(fd, active_cache, active_nr) || close(fd))
+			die("unable to write new_index file");
+		return lock_file.filename;
+	}
+
+	if (interactive)
+		interactive_add();
+
+	if (*files == NULL) {
+		/* Commit index as-is. */
+		rollback_lock_file(&lock_file);
+		return get_index_file();
+	}
+
+	/* update the user index file */
+	add_files_to_cache(verbose, prefix, files);
+	if (write_cache(fd, active_cache, active_nr) || close(fd))
+		die("unable to write new_index file");
+
+	if (!initial_commit) {
+		tree = parse_tree_indirect(head_sha1);
+		if (!tree)
+			die("failed to unpack HEAD tree object");
+		if (read_tree(tree, 0, NULL))
+			die("failed to read HEAD tree object");
+	}
+
+	/* Uh oh, abusing lock_file to create a garbage collected file */
+	next_index_lock = xmalloc(sizeof(*next_index_lock));
+	fd = hold_lock_file_for_update(next_index_lock,
+				       git_path("next-index-%d", getpid()), 1);
+	add_files_to_cache(verbose, prefix, files);
+	if (write_cache(fd, active_cache, active_nr) || close(fd))
+		die("unable to write new_index file");
+
+	return next_index_lock->filename;
+}
+
+static int run_status(FILE *fp, const char *index_file)
+{
+	struct wt_status s;
+
+	wt_status_prepare(&s);
+
+	if (amend) {
+		s.amend = 1;
+		s.reference = "HEAD^1";
+	}
+	s.verbose = verbose;
+	s.untracked = untracked_files;
+	s.index_file = index_file;
+	s.fp = fp;
+
+	wt_status_print(&s);
+
+	return s.commitable;
+}
+
+static const char sign_off_header[] = "Signed-off-by: ";
+
+static int prepare_log_message(const char *index_file)
+{
+	struct stat statbuf;
+	int commitable;
+	struct strbuf sb;
+	char *buffer;
+	FILE *fp;
+
+	strbuf_init(&sb, 0);
+	if (message) {
+		strbuf_add(&sb, message, strlen(message));
+	} else if (logfile && !strcmp(logfile, "-")) {
+		if (isatty(0))
+			fprintf(stderr, "(reading log message from standard input)\n");
+		if (strbuf_read(&sb, 0, 0) < 0)
+			die("could not read log from standard input");
+	} else if (logfile) {
+		if (strbuf_read_file(&sb, logfile, 0) < 0)
+			die("could not read log file '%s': %s",
+			    logfile, strerror(errno));
+	} else if (use_message) {
+		buffer = strstr(use_message_buffer, "\n\n");
+		if (!buffer || buffer[2] == '\0')
+			die("commit has empty message");
+		strbuf_add(&sb, buffer + 2, strlen(buffer + 2));
+	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
+			die("could not read MERGE_MSG: %s", strerror(errno));
+	} else if (!stat(git_path("SQUASH_MSG"), &statbuf)) {
+		if (strbuf_read_file(&sb, git_path("SQUASH_MSG"), 0) < 0)
+			die("could not read SQUASH_MSG: %s", strerror(errno));
+	} else if (!stat(template_file, &statbuf)) {
+		if (strbuf_read_file(&sb, template_file, 0) < 0)
+			die("could not read %s: %s",
+			    template_file, strerror(errno));
+	}
+
+	fp = fopen(git_path(commit_editmsg), "w");
+	if (fp == NULL)
+		die("could not open %s\n", git_path(commit_editmsg));
+		
+	stripspace(&sb, 0);
+	if (fwrite(sb.buf, 1, sb.len, fp) < sb.len)
+		die("could not write commit template: %s\n",
+		    strerror(errno));
+
+	if (signoff) {
+		const char *info, *bol;
+
+		info = git_committer_info(1);
+		strbuf_addch(&sb, '\0');
+		bol = strrchr(sb.buf + sb.len - 1, '\n');
+		if (!bol || prefixcmp(bol, sign_off_header))
+			fprintf(fp, "\n");
+		fprintf(fp, "%s%s\n", sign_off_header, git_committer_info(1));
+	}
+
+	strbuf_release(&sb);
+
+	if (in_merge && !no_edit) {
+		fprintf(fp,
+			"#\n"
+			"# It looks like you may be committing a MERGE.\n"
+			"# If this is not correct, please remove the file\n"
+			"#	%s\n"
+			"# and try again.\n"
+			"#\n",
+			git_path("MERGE_HEAD"));
+	}
+
+	fprintf(fp,
+		"\n"
+		"# Please enter the commit message for your changes.\n"
+		"# (Comment lines starting with '#' will not be included)\n");
+	if (only_include_assumed)
+		fprintf(fp, "# %s\n", only_include_assumed);
+
+	commitable = run_status(fp, index_file);
+
+	fclose(fp);
+
+	return commitable;
+}
+
+/* Find out if the message starting at position 'start' in the strbuf
+ * contains only whitespace and Signed-off-by lines. */
+static int message_is_empty(struct strbuf *sb, int start)
+{
+	struct strbuf tmpl;
+	const char *nl;
+	int eol, i;
+
+	/* See if the template is just a prefix of the message. */
+	strbuf_init(&tmpl, 0);
+	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
+		stripspace(&tmpl, 1);
+		if (start + tmpl.len <= sb->len &&
+		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
+			start += tmpl.len;
+	}
+	strbuf_release(&tmpl);
+
+	/* Check if the rest is just whitespace and Signed-of-by's. */
+	for (i = start; i < sb->len; i++) {
+		nl = memchr(sb->buf + i, '\n', sb->len - i);
+		if (nl)
+			eol = nl - sb->buf;
+		else
+			eol = sb->len;
+
+		if (strlen(sign_off_header) <= eol - i &&
+		    !prefixcmp(sb->buf + i, sign_off_header)) {
+			i = eol;
+			continue;
+		}
+		while (i < eol)
+			if (!isspace(sb->buf[i++]))
+				return 0;
+	}
+
+	return 1;
+}
+
+static void determine_author_info(struct strbuf *sb)
+{
+	char *p, *eol;
+	char *name = NULL, *email = NULL;
+
+	if (force_author) {
+		const char *eoname = strstr(force_author, " <");
+		const char *eomail = strchr(force_author, '>');
+
+		if (!eoname || !eomail)
+			die("malformed --author parameter\n");
+		name = xstrndup(force_author, eoname - force_author);
+		email = xstrndup(eoname + 2, eomail - eoname - 2);
+		strbuf_addf(sb, "author %s\n",
+			    fmt_ident(name, email, 
+				      getenv("GIT_AUTHOR_DATE"), 1));
+		free(name);
+		free(email);
+	} else if (use_message) {
+		p = strstr(use_message_buffer, "\nauthor");
+		if (!p)
+			die("invalid commit: %s\n", use_message);
+		p++;
+		eol = strchr(p, '\n');
+		if (!eol)
+			die("invalid commit: %s\n", use_message);
+
+		strbuf_add(sb, p, eol + 1 - p);
+	} else {
+		strbuf_addf(sb, "author %s\n", git_author_info(1));
+	}
+}
+
+static int parse_and_validate_options(int argc, const char *argv[])
+{
+	int f = 0;
+
+	argc = parse_options(argc, argv, builtin_commit_options,
+			     builtin_commit_usage, 0);
+
+	if (logfile || message || use_message)
+		no_edit = 1;
+	if (edit_flag)
+		no_edit = 0;
+
+	if (get_sha1("HEAD", head_sha1))
+		initial_commit = 1;
+
+	if (!get_sha1("MERGE_HEAD", merge_head_sha1))
+		in_merge = 1;
+
+	/* Sanity check options */
+	if (amend && initial_commit)
+		die("You have nothing to amend.");
+	if (amend && in_merge)
+		die("You are in the middle of a merger -- cannot amend.");
+
+	if (use_message)
+		f++;
+	if (edit_message)
+		f++;
+	if (logfile)
+		f++;
+	if (f > 1)
+		die("Only one of -c/-C/-F can be used.");
+	if (message && f > 0)
+		die("Option -m cannot be combined with -c/-C/-F.");
+	if (edit_message)
+		use_message = edit_message;
+	if (amend)
+		use_message = "HEAD";
+	if (use_message) {
+		unsigned char sha1[20];
+		static char utf8[] = "UTF-8";
+		const char *out_enc;
+		char *enc, *end;
+		struct commit *commit;
+
+		if (get_sha1(use_message, sha1))
+			die("could not lookup commit %s", use_message);
+		commit = lookup_commit(sha1);
+		if (!commit || parse_commit(commit))
+			die("could not parse commit %s", use_message);
+
+		enc = strstr(commit->buffer, "\nencoding");
+		if (enc) {
+			end = strchr(enc + 10, '\n');
+			enc = xstrndup(enc + 10, end - (enc + 10));
+		} else {
+			enc = utf8;
+		}
+		out_enc = git_commit_encoding ? git_commit_encoding : utf8;
+
+		if (strcmp(out_enc, enc))
+			use_message_buffer =
+				reencode_string(commit->buffer, out_enc, enc);
+
+		/* If we failed to reencode the buffer, just copy it
+		 * byte for byte so the user can try to fix it up.
+		 * This also handles the case where input and output
+		 * encodings are identical. */
+		if (use_message_buffer == NULL)
+			use_message_buffer = xstrdup(commit->buffer);
+		if (enc != utf8)
+			free(enc);
+	}
+
+	if (also && only)
+		die("Only one of --include/--only can be used.");
+	if (!*argv && (also || (only && !amend)))
+		die("No paths with --include/--only does not make sense.");
+	if (!*argv && only && amend)
+		only_include_assumed = "Clever... amending the last one with dirty index.";
+	if (*argv && !also && !only) {
+		only_include_assumed = "Explicit paths specified without -i nor -o; assuming --only paths...";
+		also = 0;
+	}
+
+	if (all && interactive)
+		die("Cannot use -a, --interactive or -i at the same time.");
+	else if (all && argc > 0)
+		die("Paths with -a does not make sense.");
+	else if (interactive && argc > 0)
+		die("Paths with --interactive does not make sense.");
+
+	return argc;
+}
+
+int cmd_status(int argc, const char **argv, const char *prefix)
+{
+	const char *index_file;
+	int commitable;
+
+	git_config(git_status_config);
+
+	argc = parse_and_validate_options(argc, argv);
+
+	index_file = prepare_index(argv, prefix);
+
+	commitable = run_status(stdout, index_file);
+
+	rollback_lock_file(&lock_file);
+
+	return commitable ? 0 : 1;
+}
+
+static int run_hook(const char *index_file, const char *name, const char *arg)
+{
+	struct child_process hook;
+	const char *argv[3], *env[2];
+	char index[PATH_MAX];
+
+	argv[0] = git_path("hooks/%s", name);
+	argv[1] = arg;
+	argv[2] = NULL;
+	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
+	env[0] = index;
+	env[1] = NULL;
+
+	if (access(argv[0], X_OK) < 0)
+		return 0;
+
+	memset(&hook, 0, sizeof(hook));
+	hook.argv = argv;
+	hook.no_stdin = 1;
+	hook.stdout_to_stderr = 1;
+	hook.env = env;
+
+	return run_command(&hook);
+}
+
+static void print_summary(const char *prefix, const unsigned char *sha1)
+{
+	struct rev_info rev;
+	struct commit *commit;
+
+	commit = lookup_commit(sha1);
+	if (!commit)
+		die("couldn't look up newly created commit\n");
+	if (!commit || parse_commit(commit))
+		die("could not parse newly created commit");
+
+	init_revisions(&rev, prefix);
+	setup_revisions(0, NULL, &rev, NULL);
+
+	rev.abbrev = 0;
+	rev.diff = 1;
+	rev.diffopt.output_format =
+		DIFF_FORMAT_SHORTSTAT | DIFF_FORMAT_SUMMARY;
+
+	rev.verbose_header = 1;
+	rev.show_root_diff = 1;
+	rev.commit_format = get_commit_format("format:%h: %s");
+	rev.always_show_header = 1;
+		
+	printf("Created %scommit ", initial_commit ? "initial " : "");
+
+	log_tree_commit(&rev, commit);
+}
+
+int git_commit_config(const char *k, const char *v)
+{
+	if (!strcmp(k, "commit.template")) {
+		template_file = xstrdup(v);
+		return 0;
+	}
+
+	return git_status_config(k, v);
+}
+
+static const char commit_utf8_warn[] =
+"Warning: commit message does not conform to UTF-8.\n"
+"You may want to amend it after fixing the message, or set the config\n"
+"variable i18n.commitencoding to the encoding your project uses.\n";
+
+int cmd_commit(int argc, const char **argv, const char *prefix)
+{
+	int header_len, parent_count = 0;
+	struct strbuf sb;
+	const char *index_file, *reflog_msg;
+	char *nl, *header_line;
+	unsigned char commit_sha1[20];
+	struct ref_lock *ref_lock;
+
+	git_config(git_commit_config);
+
+	argc = parse_and_validate_options(argc, argv);
+
+	index_file = prepare_index(argv, prefix);
+
+	if (!no_verify && run_hook(index_file, "pre-commit", NULL))
+		exit(1);
+
+	if (!prepare_log_message(index_file) && !in_merge) {
+		run_status(stdout, index_file);
+		unlink(commit_editmsg);
+		return 1;
+	}
+
+	strbuf_init(&sb, 0);
+
+	/* Start building up the commit header */
+	read_cache_from(index_file);
+	active_cache_tree = cache_tree();
+	if (cache_tree_update(active_cache_tree,
+			      active_cache, active_nr, 0, 0) < 0)
+		die("Error building trees");
+	strbuf_addf(&sb, "tree %s\n",
+		    sha1_to_hex(active_cache_tree->sha1));
+
+	/* Determine parents */
+	if (initial_commit) {
+		reflog_msg = "commit (initial)"; 
+		parent_count = 0;
+	} else if (amend) {
+		struct commit_list *c;
+		struct commit *commit;
+
+		reflog_msg = "commit (amend)";
+		commit = lookup_commit(head_sha1);
+		if (!commit || parse_commit(commit))
+			die("could not parse HEAD commit");
+
+		for (c = commit->parents; c; c = c->next)
+			strbuf_addf(&sb, "parent %s\n",
+				      sha1_to_hex(c->item->object.sha1));
+	} else if (in_merge) {
+		struct strbuf m;
+		FILE *fp;
+
+		reflog_msg = "commit (merge)";
+		strbuf_addf(&sb, "parent %s\n", sha1_to_hex(head_sha1));
+		strbuf_init(&m, 0);
+		fp = fopen(git_path("MERGE_HEAD"), "r");
+		if (fp == NULL)
+			die("could not open %s for reading: %s",
+			    git_path("MERGE_HEAD"), strerror(errno));
+		while (strbuf_getline(&m, fp, '\n') != EOF)
+			strbuf_addf(&sb, "parent %s\n", m.buf);
+		fclose(fp);
+		strbuf_release(&m);
+	} else {
+		reflog_msg = "commit";
+		strbuf_addf(&sb, "parent %s\n", sha1_to_hex(head_sha1));
+	}
+
+	determine_author_info(&sb);
+	strbuf_addf(&sb, "committer %s\n", git_committer_info(1));
+	if (!is_encoding_utf8(git_commit_encoding))
+		strbuf_addf(&sb, "encoding %s\n", git_commit_encoding);
+	strbuf_addch(&sb, '\n');
+
+	/* Get the commit message and validate it */
+	header_len = sb.len;
+	if (!no_edit) {
+		fprintf(stderr, "launching editor, log %s\n", logfile);
+		launch_editor(git_path(commit_editmsg), &sb);
+	}
+	else if (strbuf_read_file(&sb, git_path(commit_editmsg), 0) < 0)
+		die("could not read commit message\n");
+	if (run_hook(index_file, "commit-msg", commit_editmsg))
+		exit(1);
+	stripspace(&sb, 1);
+	if (sb.len < header_len ||
+	    message_is_empty(&sb, header_len))
+		die("* no commit message?  aborting commit.");
+	strbuf_addch(&sb, '\0');
+	if (is_encoding_utf8(git_commit_encoding) && !is_utf8(sb.buf))
+		fprintf(stderr, commit_utf8_warn);
+
+	if (write_sha1_file(sb.buf, sb.len - 1, commit_type, commit_sha1))
+		die("failed to write commit object");
+		       
+	ref_lock = lock_any_ref_for_update("HEAD",
+					   initial_commit ? NULL : head_sha1,
+					   0);
+
+	nl = strchr(sb.buf + header_len, '\n');
+	header_line = xstrndup(sb.buf + header_len,
+			       nl - (sb.buf + header_len));
+	strbuf_release(&sb);
+	strbuf_addf(&sb, "%s: %s\n", reflog_msg, header_line);
+	strbuf_addch(&sb, '\0');
+	free(header_line);
+
+	if (!ref_lock)
+		die("cannot lock HEAD ref");
+	if (write_ref_sha1(ref_lock, commit_sha1, sb.buf) < 0)
+		die("cannot update HEAD ref");
+
+	unlink(git_path("MERGE_HEAD"));
+	unlink(git_path("MERGE_MSG"));
+
+	if (lock_file.filename[0] && commit_locked_index(&lock_file))
+		die("failed to write new index");
+
+	rerere();
+
+	run_hook(index_file, "post-commit", NULL);
+
+	if (!quiet)
+		print_summary(prefix, commit_sha1);
+
+	return 0;
+}
diff --git a/builtin.h b/builtin.h
index 9a6213a..0f8456c 100644
--- a/builtin.h
+++ b/builtin.h
@@ -24,6 +24,7 @@ extern int cmd_check_attr(int argc, const char **argv, const char *prefix);
 extern int cmd_check_ref_format(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry(int argc, const char **argv, const char *prefix);
 extern int cmd_cherry_pick(int argc, const char **argv, const char *prefix);
+extern int cmd_commit(int argc, const char **argv, const char *prefix);
 extern int cmd_commit_tree(int argc, const char **argv, const char *prefix);
 extern int cmd_count_objects(int argc, const char **argv, const char *prefix);
 extern int cmd_describe(int argc, const char **argv, const char *prefix);
@@ -68,10 +69,10 @@ 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_revert(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_shortlog(int argc, const char **argv, const char *prefix);
 extern int cmd_show(int argc, const char **argv, const char *prefix);
 extern int cmd_show_branch(int argc, const char **argv, const char *prefix);
+extern int cmd_status(int argc, const char **argv, const char *prefix);
 extern int cmd_stripspace(int argc, const char **argv, const char *prefix);
 extern int cmd_symbolic_ref(int argc, const char **argv, const char *prefix);
 extern int cmd_tag(int argc, const char **argv, const char *prefix);
diff --git a/git-commit.sh b/contrib/examples/git-commit.sh
similarity index 100%
rename from git-commit.sh
rename to contrib/examples/git-commit.sh
diff --git a/git.c b/git.c
index 4e10581..8522095 100644
--- a/git.c
+++ b/git.c
@@ -298,6 +298,7 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "check-attr", cmd_check_attr, RUN_SETUP | NEED_WORK_TREE },
 		{ "cherry", cmd_cherry, RUN_SETUP },
 		{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
+		{ "commit", cmd_commit, RUN_SETUP | NEED_WORK_TREE },
 		{ "commit-tree", cmd_commit_tree, RUN_SETUP },
 		{ "config", cmd_config },
 		{ "count-objects", cmd_count_objects, RUN_SETUP },
@@ -346,10 +347,10 @@ static void handle_internal_command(int argc, const char **argv)
 		{ "rev-parse", cmd_rev_parse, RUN_SETUP },
 		{ "revert", cmd_revert, RUN_SETUP | NEED_WORK_TREE },
 		{ "rm", cmd_rm, RUN_SETUP | NEED_WORK_TREE },
-		{ "runstatus", cmd_runstatus, RUN_SETUP | NEED_WORK_TREE },
 		{ "shortlog", cmd_shortlog, RUN_SETUP | USE_PAGER },
 		{ "show-branch", cmd_show_branch, RUN_SETUP },
 		{ "show", cmd_show, RUN_SETUP | USE_PAGER },
+		{ "status", cmd_status, RUN_SETUP | NEED_WORK_TREE },
 		{ "stripspace", cmd_stripspace },
 		{ "symbolic-ref", cmd_symbolic_ref, RUN_SETUP },
 		{ "tag", cmd_tag, RUN_SETUP },
-- 
1.5.3.4.206.g58ba4

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3546 bytes --]

Hi,

On Fri, 2 Nov 2007, Kristian Høgsberg wrote:

> +static char *
> +prepare_index(const char **files, const char *prefix)
> +{
> +	int fd;
> +	struct tree *tree;
> +	struct lock_file *next_index_lock;
> +
> +	fd = hold_locked_index(&lock_file, 1);
> +	if (read_cache() < 0)
> +		die("index file corrupt");
> +
> +	if (all) {
> +		add_files_to_cache(verbose, NULL, files);
> +		if (write_cache(fd, active_cache, active_nr) || close(fd))
> +			die("unable to write new_index file");
> +		return lock_file.filename;
> +	} else if (also) {
> +		add_files_to_cache(verbose, prefix, files);
> +		if (write_cache(fd, active_cache, active_nr) || close(fd))
> +			die("unable to write new_index file");
> +		return lock_file.filename;
> +	}

Unless something slips by my mind, this could be written as

	if (all || also) {
		add_files_to_cache(verbose, also ? prefix : NULL, files);
		...
	}

> +
> +	if (interactive)
> +		interactive_add();
> +
> +	if (*files == NULL) {
> +		/* Commit index as-is. */
> +		rollback_lock_file(&lock_file);
> +		return get_index_file();
> +	}

Would an interactive add not conflict with this rollback?  And indeed with 
the locked index to begin with?

> +	/* update the user index file */
> +	add_files_to_cache(verbose, prefix, files);
> +	if (write_cache(fd, active_cache, active_nr) || close(fd))
> +		die("unable to write new_index file");

Does that mean that the index is _always_ written?  Even when not 
specifying and paths on the command line?

> +	/* Uh oh, abusing lock_file to create a garbage collected file */

It's not that bad.  But I would mention that it is a temporary index which 
you are building.

> +static const char sign_off_header[] = "Signed-off-by: ";

Funny, I thought it was a footer ;-)

> +	} else if (!stat(template_file, &statbuf)) {

Should this not test "if (template_file && " first?

> +/* Find out if the message starting at position 'start' in the strbuf
> + * contains only whitespace and Signed-off-by lines. */
> +static int message_is_empty(struct strbuf *sb, int start)
> +{
> +	struct strbuf tmpl;
> +	const char *nl;
> +	int eol, i;
> +
> +	/* See if the template is just a prefix of the message. */
> +	strbuf_init(&tmpl, 0);
> +	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
> +		stripspace(&tmpl, 1);
> +		if (start + tmpl.len <= sb->len &&
> +		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
> +			start += tmpl.len;
> +	}
> +	strbuf_release(&tmpl);

The release could go inside the if block, no?

> +static int run_hook(const char *index_file, const char *name, const char *arg)

Would this function not prefer to live in run-command.c?

> +{
> +	struct child_process hook;
> +	const char *argv[3], *env[2];
> +	char index[PATH_MAX];
> +
> +	argv[0] = git_path("hooks/%s", name);
> +	argv[1] = arg;
> +	argv[2] = NULL;
> +	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> +	env[0] = index;
> +	env[1] = NULL;
> +
> +	if (access(argv[0], X_OK) < 0)
> +		return 0;

That check logically belongs 6 lines higher...

> +	rev.abbrev = 0;
> +	rev.diff = 1;
> +	rev.diffopt.output_format =
> +		DIFF_FORMAT_SHORTSTAT | DIFF_FORMAT_SUMMARY;
> +
> +	rev.verbose_header = 1;
> +	rev.show_root_diff = 1;
> +	rev.commit_format = get_commit_format("format:%h: %s");

That's interesting.  Wouldn't have thought of that.  Reusing the log_tree 
machinery to output the summary.  Cute.

Note that one relatively low-hanging fruit will be to teach builtin-commit 
to use a cheaper "no changes?" check when no_edit = 1.

Thanks,
Dscho

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Kristian Høgsberg]

On Sat, 2007-11-03 at 13:56 +0000, Johannes Schindelin wrote: 
> Hi,

Hi, wanted to send this out earlier, but I punted it and a couple of
days went by...

> On Fri, 2 Nov 2007, Kristian Høgsberg wrote:
> 
> > +static char *
> > +prepare_index(const char **files, const char *prefix)
> > +{
> > +	int fd;
> > +	struct tree *tree;
> > +	struct lock_file *next_index_lock;
> > +
> > +	fd = hold_locked_index(&lock_file, 1);
> > +	if (read_cache() < 0)
> > +		die("index file corrupt");
> > +
> > +	if (all) {
> > +		add_files_to_cache(verbose, NULL, files);
> > +		if (write_cache(fd, active_cache, active_nr) || close(fd))
> > +			die("unable to write new_index file");
> > +		return lock_file.filename;
> > +	} else if (also) {
> > +		add_files_to_cache(verbose, prefix, files);
> > +		if (write_cache(fd, active_cache, active_nr) || close(fd))
> > +			die("unable to write new_index file");
> > +		return lock_file.filename;
> > +	}
> 
> Unless something slips by my mind, this could be written as
> 
> 	if (all || also) {
> 		add_files_to_cache(verbose, also ? prefix : NULL, files);
> 		...
> 	}

Yup, that looks right.

> > +
> > +	if (interactive)
> > +		interactive_add();
> > +
> > +	if (*files == NULL) {
> > +		/* Commit index as-is. */
> > +		rollback_lock_file(&lock_file);
> > +		return get_index_file();
> > +	}
> 
> Would an interactive add not conflict with this rollback?  And indeed with 
> the locked index to begin with?

Yes, I've moved the interactive case up to the beginning of
prepare_index() and made it return get_index_file() after running
interactive_add().

> > +	/* update the user index file */
> > +	add_files_to_cache(verbose, prefix, files);
> > +	if (write_cache(fd, active_cache, active_nr) || close(fd))
> > +		die("unable to write new_index file");
> 
> Does that mean that the index is _always_ written?  Even when not 
> specifying and paths on the command line?

Do you mean "not specifying any options and paths..."?  In that case we
add the files to the index and create a temporary index from HEAD and
add the files there too, which we then commit.  So we have to write the
index in that case.  If there are no files on the command line, we roll
back the lock and bail out just above the part you quoted.

> > +	/* Uh oh, abusing lock_file to create a garbage collected file */
> 
> It's not that bad.  But I would mention that it is a temporary index which 
> you are building.

Heh, yeah, I toned it down a bit :)

> > +static const char sign_off_header[] = "Signed-off-by: ";
> 
> Funny, I thought it was a footer ;-)
> 
> > +	} else if (!stat(template_file, &statbuf)) {
> 
> Should this not test "if (template_file && " first?

Yup, added.

> > +/* Find out if the message starting at position 'start' in the strbuf
> > + * contains only whitespace and Signed-off-by lines. */
> > +static int message_is_empty(struct strbuf *sb, int start)
> > +{
> > +	struct strbuf tmpl;
> > +	const char *nl;
> > +	int eol, i;
> > +
> > +	/* See if the template is just a prefix of the message. */
> > +	strbuf_init(&tmpl, 0);
> > +	if (template_file && strbuf_read_file(&tmpl, template_file, 0) > 0) {
> > +		stripspace(&tmpl, 1);
> > +		if (start + tmpl.len <= sb->len &&
> > +		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
> > +			start += tmpl.len;
> > +	}
> > +	strbuf_release(&tmpl);
> 
> The release could go inside the if block, no?

Sure, not a big deal, though.

> > +static int run_hook(const char *index_file, const char *name, const char *arg)
> 
> Would this function not prefer to live in run-command.c?

Yeah, we can move it later, though.

> > +{
> > +	struct child_process hook;
> > +	const char *argv[3], *env[2];
> > +	char index[PATH_MAX];
> > +
> > +	argv[0] = git_path("hooks/%s", name);
> > +	argv[1] = arg;
> > +	argv[2] = NULL;
> > +	snprintf(index, sizeof(index), "GIT_INDEX_FILE=%s", index_file);
> > +	env[0] = index;
> > +	env[1] = NULL;
> > +
> > +	if (access(argv[0], X_OK) < 0)
> > +		return 0;
> 
> That check logically belongs 6 lines higher...

You mean you want to do it right after the argv[0] assignment?  I'd like
to keep the block of assignments together so it's easy to see how the
array is set up.  Don't tell me you worry about performance here... ;)

> > +	rev.abbrev = 0;
> > +	rev.diff = 1;
> > +	rev.diffopt.output_format =
> > +		DIFF_FORMAT_SHORTSTAT | DIFF_FORMAT_SUMMARY;
> > +
> > +	rev.verbose_header = 1;
> > +	rev.show_root_diff = 1;
> > +	rev.commit_format = get_commit_format("format:%h: %s");
> 
> That's interesting.  Wouldn't have thought of that.  Reusing the log_tree 
> machinery to output the summary.  Cute.

Heh, I just did what the shell script did.  It uses git diff-tree for
this, and the above is just the relevant bits from builtin-diff-tree.c.

cheers,
Kristian

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Björn Steinbrink]

On 2007.11.02 11:33:09 -0400, Kristian Høgsberg wrote:
> +	if (all && interactive)
> +		die("Cannot use -a, --interactive or -i at the same time.");

Shouldn't that be "if (all && (interactive || also))"?

Björn

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Kristian Høgsberg]

On Sat, 2007-11-03 at 16:06 +0100, Björn Steinbrink wrote:
> On 2007.11.02 11:33:09 -0400, Kristian Høgsberg wrote:
> > +	if (all && interactive)
> > +		die("Cannot use -a, --interactive or -i at the same time.");
> 
> Shouldn't that be "if (all && (interactive || also))"?

The shell script just has

case "$all,$interactive,$also,$#" in
*t,*t,*)
        die "Cannot use -a, --interactive or -i at the same time." ;;

which doesn't seem to care about the value of $also.  As far as I
understand git commit, it doesn't make sense to pass any of -a, -i, -o
or --interactive at the same time so I guess I could join the checks

        if (also && only)
                die("Only one of --include/--only can be used.");
        if (all && interactive)
                die("Cannot use -a, --interactive or -i at the same
time.");

into something like

	if (also + only + all + interactive > 1)              
		die("Only one of --include/--only/--all/--interactive can be used.");

Does that sound right?

Kristian

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Björn Steinbrink]

On 2007.11.05 13:57:53 -0500, Kristian Høgsberg wrote:
> On Sat, 2007-11-03 at 16:06 +0100, Björn Steinbrink wrote:
> > On 2007.11.02 11:33:09 -0400, Kristian Høgsberg wrote:
> > > +	if (all && interactive)
> > > +		die("Cannot use -a, --interactive or -i at the same time.");
> > 
> > Shouldn't that be "if (all && (interactive || also))"?

Ah, damn. I (obviously) misread the error message as:
	foo and (bar or bar2)

IOW "and" instead of the first comma.

Actually it should be:
	if ((all && (interactive || also)) || (interactive && also))

(or whatever more simple version you come up with)

> The shell script just has
> 
> case "$all,$interactive,$also,$#" in
> *t,*t,*)
>         die "Cannot use -a, --interactive or -i at the same time." ;;
> 
> which doesn't seem to care about the value of $also.  As far as I
> understand git commit, it doesn't make sense to pass any of -a, -i, -o
> or --interactive at the same time so I guess I could join the checks

Note that there are only two commas. The asterisks catch everything and
$# won't be "t", so that catches anything with at least two t's.

Also note, that the two checks after that one in git-commit.sh as of the
current master (140dd77a5cb2e61dcb942e245a2474fae95e42a5) are broken
(I'll send a patch in a separate mail).

>         if (also && only)
>                 die("Only one of --include/--only can be used.");
>         if (all && interactive)
>                 die("Cannot use -a, --interactive or -i at the same
> time.");
> 
> into something like
> 
> 	if (also + only + all + interactive > 1)              
> 		die("Only one of --include/--only/--all/--interactive can be used.");
> 
> Does that sound right?

--include is not in the manpage, and I only glanced over git-commit.sh,
but yeah, at least to me, that sounds right.

Björn

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Johannes Schindelin]

Hi,

On Mon, 5 Nov 2007, Bj?rn Steinbrink wrote:

> On 2007.11.05 13:57:53 -0500, Kristian H?gsberg wrote:
>
> > The shell script just has
> > 
> > case "$all,$interactive,$also,$#" in
> > *t,*t,*)
> >         die "Cannot use -a, --interactive or -i at the same time." ;;
> > 
> > which doesn't seem to care about the value of $also.  As far as I 
> > understand git commit, it doesn't make sense to pass any of -a, -i, -o 
> > or --interactive at the same time so I guess I could join the checks
> 
> Note that there are only two commas. The asterisks catch everything and
> $# won't be "t", so that catches anything with at least two t's.

So shouldn't it be

	if (!!all + !!interactive + !!also > 1)

Hmm?

Ciao,
Dscho

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Björn Steinbrink]

On 2007.11.05 23:18:36 +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 5 Nov 2007, Bj?rn Steinbrink wrote:
> 
> > On 2007.11.05 13:57:53 -0500, Kristian H?gsberg wrote:
> >
> > > The shell script just has
> > > 
> > > case "$all,$interactive,$also,$#" in
> > > *t,*t,*)
> > >         die "Cannot use -a, --interactive or -i at the same time." ;;
> > > 
> > > which doesn't seem to care about the value of $also.  As far as I 
> > > understand git commit, it doesn't make sense to pass any of -a, -i, -o 
> > > or --interactive at the same time so I guess I could join the checks
> > 
> > Note that there are only two commas. The asterisks catch everything and
> > $# won't be "t", so that catches anything with at least two t's.
> 
> So shouldn't it be
> 
> 	if (!!all + !!interactive + !!also > 1)
> 
> Hmm?

Ah, yeah, that's the short and sweet version, I always forget about the
conversion to bool giving you 0/1 values... ;-)

Note though, that Kristian had a similar check at the end of his email,
that included "only" (but lacked the bool conversion). The original
reason why I thought that it would be better was that for example
"git commit --all --only foo" didn't care about "only" at all. But that
actually was because the --all + paths usage check was broken. So the
fixed version actually refuses to use accept that, but with a (IMHO) not
so good error message:

$ git commit -a -o file
Paths with -a does not make sense.

Given that some people are used to just pass -a all the time, they might
just automatically pass it together with -o. And I think that we
actually want to tell them that -a + -o makes no sense instead. Just
like we do for -a + -i, which is kind of the complementary usage error.

So I'd go for a correct version of Kristian's suggestion:

if (!!also + !!only + !!all + !!interactive > 1)
	die("Only one of --include/--only/--all/--interactive can be used.");

Björn

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Kristian Høgsberg]

On Tue, 2007-11-06 at 07:59 +0100, Björn Steinbrink wrote:
...
> Note though, that Kristian had a similar check at the end of his email,
> that included "only" (but lacked the bool conversion). The original
> reason why I thought that it would be better was that for example
> "git commit --all --only foo" didn't care about "only" at all. But that
> actually was because the --all + paths usage check was broken. So the
> fixed version actually refuses to use accept that, but with a (IMHO) not
> so good error message:
> 
> $ git commit -a -o file
> Paths with -a does not make sense.
> 
> Given that some people are used to just pass -a all the time, they might
> just automatically pass it together with -o. And I think that we
> actually want to tell them that -a + -o makes no sense instead. Just
> like we do for -a + -i, which is kind of the complementary usage error.
> 
> So I'd go for a correct version of Kristian's suggestion:
> 
> if (!!also + !!only + !!all + !!interactive > 1)
> 	die("Only one of --include/--only/--all/--interactive can be used.");

Good points, I will use that in the next version of the patch.  Just a
note about the !! idiom (which I can't stand, fwiw): my version just
added the variables, which were all integers, initialized to zero and
incremented by the option parser when it sees the corresponding option.
So what I had would work too, with the extra check that:

  $ git commit -a -a

etc would give the error

  Only one of --include/--only/--all/--interactive can be used.

which is acutally accurate.

cheers,
Kristian

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Björn Steinbrink]

On 2007.11.06 11:46:59 -0500, Kristian Høgsberg wrote:
> On Tue, 2007-11-06 at 07:59 +0100, Björn Steinbrink wrote:
> ...
> > Note though, that Kristian had a similar check at the end of his email,
> > that included "only" (but lacked the bool conversion). The original
> > reason why I thought that it would be better was that for example
> > "git commit --all --only foo" didn't care about "only" at all. But that
> > actually was because the --all + paths usage check was broken. So the
> > fixed version actually refuses to use accept that, but with a (IMHO) not
> > so good error message:
> > 
> > $ git commit -a -o file
> > Paths with -a does not make sense.
> > 
> > Given that some people are used to just pass -a all the time, they might
> > just automatically pass it together with -o. And I think that we
> > actually want to tell them that -a + -o makes no sense instead. Just
> > like we do for -a + -i, which is kind of the complementary usage error.
> > 
> > So I'd go for a correct version of Kristian's suggestion:
> > 
> > if (!!also + !!only + !!all + !!interactive > 1)
> > 	die("Only one of --include/--only/--all/--interactive can be used.");
> 
> Good points, I will use that in the next version of the patch.  Just a
> note about the !! idiom (which I can't stand, fwiw): my version just
> added the variables, which were all integers, initialized to zero and
> incremented by the option parser when it sees the corresponding option.
> So what I had would work too, with the extra check that:
> 
>   $ git commit -a -a
> 
> etc would give the error
> 
>   Only one of --include/--only/--all/--interactive can be used.
> 
> which is acutally accurate.

Hm, why? The user used only one of them. The error message does not say
that you cannot pass the same one multiple times. And I don't think that
passing the same boolean flag twice should be treated as an usage error
either. There's no contradiction in wanting all files and, well, all
files to be committed.

Sidenote: The "or mask" stuff in the option parser would probably
prevent you from catching "-a -a" anyway, because the flag becomes truly
boolean ;-)

Björn

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 2717 bytes --]

On Mon, Nov 05, 2007 at 11:18:36PM +0000, Johannes Schindelin wrote:
> Hi,
> 
> On Mon, 5 Nov 2007, Bj?rn Steinbrink wrote:
> 
> > On 2007.11.05 13:57:53 -0500, Kristian H?gsberg wrote:
> >
> > > The shell script just has
> > > 
> > > case "$all,$interactive,$also,$#" in
> > > *t,*t,*)
> > >         die "Cannot use -a, --interactive or -i at the same time." ;;
> > > 
> > > which doesn't seem to care about the value of $also.  As far as I 
> > > understand git commit, it doesn't make sense to pass any of -a, -i, -o 
> > > or --interactive at the same time so I guess I could join the checks
> > 
> > Note that there are only two commas. The asterisks catch everything and
> > $# won't be "t", so that catches anything with at least two t's.
> 
> So shouldn't it be
> 
> 	if (!!all + !!interactive + !!also > 1)

Btw, I'm starting to work slowly on the diff_opt_parse conversion to the
macro we discussed, and the need for new option parsing callbacks
arised, and I've created a:

  parse_opt_mask_{or,and,xor} commands that you declare this way:

    OPT_MASK_OR('a', "all",         &mode, "...", MASK_ALL),
    OPT_MASK_OR('i', "interactive", &mode, "...", MASK_INTERACTIVE),
    ...

And if you chose MASK_ALL/INTERACTIVE/.. to be single bits,

    if (!!all + !!interactive ... > 1)

becomes[0]:

    if (mode & (mode - 1)) {

    }

I've not read your patch thoroughly, but if you feel such a thing would
help you, I could send a preliminar set of patches to enable this
feature for people that may need it. Though you can look at my WIP on my
git public repository[1].

For those who care, this need arised because parse_diff_opts have a
_lot_ of single bit options, and that expansing it on many many full
blown integers looked like a regression, so I took the option to have a
`flags` member with explicit masks, and those masks will be used from
the parse-option callbacks[2]


  [0] for those who don't already know it, (i & (i - 1)) == 0
      iff i is 0 or a power of 2.

  [1] the patch[3]:
      http://git.madism.org/?p=git.git;a=commitdiff;h=9d75b0a00915fa81657934f36318c1c0f5bac96b
      example of use:
      http://git.madism.org/?p=git.git;a=commitdiff;h=74aacc487579d0cbd638adf883e743caeaee0f76
      http://git.madism.org/?p=git.git;a=commitdiff;h=af15793dde94119faa1577c9eec7e839ae628011

  [2] http://git.madism.org/?p=git.git;a=commitdiff;h=86032204f1bdf5da2fee555ec917709b3e6f662c#patch10

  [3] oh boy gitweb urls are really way too long :/
-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Johannes Sixt]

Pierre Habouzit schrieb:
> Btw, I'm starting to work slowly on the diff_opt_parse conversion to the
> macro we discussed, and the need for new option parsing callbacks
> arised, and I've created a:
> 
>   parse_opt_mask_{or,and,xor} commands that you declare this way:
> 
>     OPT_MASK_OR('a', "all",         &mode, "...", MASK_ALL),
>     OPT_MASK_OR('i', "interactive", &mode, "...", MASK_INTERACTIVE),
>     ...
> 
> And if you chose MASK_ALL/INTERACTIVE/.. to be single bits,
> 
>     if (!!all + !!interactive ... > 1)
> 
> becomes[0]:
> 
>     if (mode & (mode - 1)) {
> 
>     }

This goes too far, IMHO. That's unnecessary cleverness/microoptimization at 
the expense of readability.

-- Hannes

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Pierre Habouzit]

[-- Attachment #1: Type: text/plain, Size: 1418 bytes --]

On Tue, Nov 06, 2007 at 09:18:02AM +0000, Johannes Sixt wrote:
> Pierre Habouzit schrieb:
> >Btw, I'm starting to work slowly on the diff_opt_parse conversion to the
> >macro we discussed, and the need for new option parsing callbacks
> >arised, and I've created a:
> >  parse_opt_mask_{or,and,xor} commands that you declare this way:
> >    OPT_MASK_OR('a', "all",         &mode, "...", MASK_ALL),
> >    OPT_MASK_OR('i', "interactive", &mode, "...", MASK_INTERACTIVE),
> >    ...
> >And if you chose MASK_ALL/INTERACTIVE/.. to be single bits,
> >    if (!!all + !!interactive ... > 1)
> >becomes[0]:
> >    if (mode & (mode - 1)) {
> >    }
> 
> This goes too far, IMHO. That's unnecessary cleverness/microoptimization 
> at the expense of readability.

The reason why I did that is not to be able to do mode & (mode - 1).
Have a look at diff.c, imagine the insane amount of intermediate
variables we would need, and now understand why I introduced that :)

I'm not sure it's useful for git-commit, I was mentioning it just in case.

Note: the fact that an OPT_BOOLEAN when you repeat it increments the
      value isn't always a good thing, and is the reason why you need
      the (quite ugly) double bangs.

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org

[-- Attachment #2: Type: application/pgp-signature, Size: 189 bytes --]

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Andreas Ericsson]

Pierre Habouzit wrote:
> On Tue, Nov 06, 2007 at 09:18:02AM +0000, Johannes Sixt wrote:
>> Pierre Habouzit schrieb:
>>> Btw, I'm starting to work slowly on the diff_opt_parse conversion to the
>>> macro we discussed, and the need for new option parsing callbacks
>>> arised, and I've created a:
>>>  parse_opt_mask_{or,and,xor} commands that you declare this way:
>>>    OPT_MASK_OR('a', "all",         &mode, "...", MASK_ALL),
>>>    OPT_MASK_OR('i', "interactive", &mode, "...", MASK_INTERACTIVE),
>>>    ...
>>> And if you chose MASK_ALL/INTERACTIVE/.. to be single bits,
>>>    if (!!all + !!interactive ... > 1)
>>> becomes[0]:
>>>    if (mode & (mode - 1)) {
>>>    }
>> This goes too far, IMHO. That's unnecessary cleverness/microoptimization 
>> at the expense of readability.
> 
> The reason why I did that is not to be able to do mode & (mode - 1).

With a macro along the lines of

  #define IS_MULTI_FLAGGED(x) (x & (x - 1))

at least this particular construct becomes a lot more readable.

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 4/4] Implement git commit and status as a builtin commands.

[Kristian Høgsberg]

On Tue, 2007-11-06 at 10:12 +0100, Pierre Habouzit wrote:
> On Mon, Nov 05, 2007 at 11:18:36PM +0000, Johannes Schindelin wrote:
> > Hi,
> > 
> > On Mon, 5 Nov 2007, Bj?rn Steinbrink wrote:
> > 
> > > On 2007.11.05 13:57:53 -0500, Kristian H?gsberg wrote:
> > >
> > > > The shell script just has
> > > > 
> > > > case "$all,$interactive,$also,$#" in
> > > > *t,*t,*)
> > > >         die "Cannot use -a, --interactive or -i at the same time." ;;
> > > > 
> > > > which doesn't seem to care about the value of $also.  As far as I 
> > > > understand git commit, it doesn't make sense to pass any of -a, -i, -o 
> > > > or --interactive at the same time so I guess I could join the checks
> > > 
> > > Note that there are only two commas. The asterisks catch everything and
> > > $# won't be "t", so that catches anything with at least two t's.
> > 
> > So shouldn't it be
> > 
> > 	if (!!all + !!interactive + !!also > 1)
> 
> Btw, I'm starting to work slowly on the diff_opt_parse conversion to the
> macro we discussed, and the need for new option parsing callbacks
> arised, and I've created a:
> 
>   parse_opt_mask_{or,and,xor} commands that you declare this way:
> 
>     OPT_MASK_OR('a', "all",         &mode, "...", MASK_ALL),
>     OPT_MASK_OR('i', "interactive", &mode, "...", MASK_INTERACTIVE),
>     ...

That's useful here, and it should be useful in a lot of other places
using parse_options() where we end up or'ing flags into a mask based on
boolean options.

cheers,
Kristian

Re: [PATCH 3/4] Export launch_editor() and make it accept ':' as a no-op editor.

[Andreas Ericsson]

Kristian Høgsberg wrote:
>  
> +	if (!strcmp(editor, ":"))
> +		return;
> +

Doesn't this make the change in 2/4 superfluous?

-- 
Andreas Ericsson                   andreas.ericsson@op5.se
OP5 AB                             www.op5.se
Tel: +46 8-230225                  Fax: +46 8-230231

Re: [PATCH 3/4] Export launch_editor() and make it accept ':' as a no-op editor.

[Kristian Høgsberg]

On Fri, 2007-11-02 at 16:46 +0100, Andreas Ericsson wrote:
> Kristian Høgsberg wrote:
> >  
> > +	if (!strcmp(editor, ":"))
> > +		return;
> > +
> 
> Doesn't this make the change in 2/4 superfluous?

Yeah, but it's still a nice cleanup, I think.

Kristian

Re: [PATCH 2/4] Remove unecessary hard-coding of EDITOR=':' VISUAL=':' in some test suites.

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---
>  t/t3501-revert-cherry-pick.sh |    4 ++--
>  t/t3901-i18n-patch.sh         |    8 ++++----
>  2 files changed, 6 insertions(+), 6 deletions(-)

This is a good patch, but a real commit message that says why
they are unnecessary is lacking.  Something like...

        Sourcing of ./test-lib.sh at the very beginning of test scripts
        already define and export them.

Re: [PATCH 1/4] Add testcase for ammending and fixing author in git commit.

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

> Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> ---
>  t/t7501-commit.sh |   10 ++++++++++
>  1 files changed, 10 insertions(+), 0 deletions(-)
>
> diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> index b151b51..3f2112a 100644
> --- a/t/t7501-commit.sh
> +++ b/t/t7501-commit.sh
> @@ -163,4 +163,14 @@ test_expect_success 'partial commit that involves removal (3)' '
>  
>  '
>  
> +author="The Real Author <someguy@his.email.org>"
> +test_expect_success 'amend commit to fix author' '
> +
> +	git reset --hard
> +	git cat-file -p HEAD | sed -e "s/author.*>/author $author/" > expected &&
> +	git commit --amend --author="$author" &&
> +	git cat-file -p HEAD > current &&
> +	diff expected current
> +	
> +'
>  test_done

This can't be right.  How are you ignoring the differences in
committer dates?

By the way, I _think_ git-commit.sh allows fixing author name/email
without molesting the author timestamp (i.e. takes it from the
amended commit).  That should probably be checked with the test
as well.

Re: [PATCH 1/4] Add testcase for ammending and fixing author in git commit.

[Kristian Høgsberg]

On Fri, 2007-11-02 at 13:07 -0700, Junio C Hamano wrote:
> Kristian Høgsberg <krh@redhat.com> writes:
> 
> > Signed-off-by: Kristian Høgsberg <krh@redhat.com>
> > ---
> >  t/t7501-commit.sh |   10 ++++++++++
> >  1 files changed, 10 insertions(+), 0 deletions(-)
> >
> > diff --git a/t/t7501-commit.sh b/t/t7501-commit.sh
> > index b151b51..3f2112a 100644
> > --- a/t/t7501-commit.sh
> > +++ b/t/t7501-commit.sh
> > @@ -163,4 +163,14 @@ test_expect_success 'partial commit that involves removal (3)' '
> >  
> >  '
> >  
> > +author="The Real Author <someguy@his.email.org>"
> > +test_expect_success 'amend commit to fix author' '
> > +
> > +	git reset --hard
> > +	git cat-file -p HEAD | sed -e "s/author.*>/author $author/" > expected &&
> > +	git commit --amend --author="$author" &&
> > +	git cat-file -p HEAD > current &&
> > +	diff expected current
> > +	
> > +'
> >  test_done
> 
> This can't be right.  How are you ignoring the differences in
> committer dates?

t/test-lib.sh fixes GIT_COMMITTER_DATE so all commits have the date

committer C O Mitter <committer@example.com> 1112911993 -0700

unless you use test_tick.

> By the way, I _think_ git-commit.sh allows fixing author name/email
> without molesting the author timestamp (i.e. takes it from the
> amended commit).  That should probably be checked with the test
> as well.

You're right, I need to pick GIT_AUTHOR_DATE from the ammended commit.
Ok, I'll need to rewrite determine_author_info() a little bit.  I might
get an update patch out this weekend.

cheers,
Kristian

Re: [PATCH 1/4] Add testcase for ammending and fixing author in git commit.

[Junio C Hamano]

Kristian Høgsberg <krh@redhat.com> writes:

>> This can't be right.  How are you ignoring the differences in
>> committer dates?
>
> t/test-lib.sh fixes GIT_COMMITTER_DATE so all commits have the date
>
> committer C O Mitter <committer@example.com> 1112911993 -0700
>
> unless you use test_tick.

Heh, that was actually what I was getting at.  By not doing
test_tick, you are not making sure that the commit gets the
timestamp of the time --amend is run.

>> By the way, I _think_ git-commit.sh allows fixing author name/email
>> without molesting the author timestamp (i.e. takes it from the
>> amended commit).  That should probably be checked with the test
>> as well.
>
> You're right, I need to pick GIT_AUTHOR_DATE from the ammended commit.
> Ok, I'll need to rewrite determine_author_info() a little bit.  I might
> get an update patch out this weekend.