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

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