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

From: Kristian Høgsberg <krh@redhat.com>
Date: Nov 8, 2007, 16:01 UTC
Message-ID: <1194537708.27305.6.camel@hinata.boston.redhat.com>
In-Reply-To: <Pine.LNX.4.64.0711031328500.4362@racer.site>

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...

Show 31 quoted lines

> 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.

Show 12 quoted lines

> > +
> > +	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().

Show 7 quoted lines

> > +	/* 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 :)

Show 7 quoted lines

> > +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.

Show 19 quoted lines

> > +/* 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.

Show 16 quoted lines

> > +{
> > +	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... ;)

Show 11 quoted lines

> > +	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