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

["Kristian Høgsberg" <krh@redhat.com>]

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