Re: [PATCH 9/9] Implement git commit as a builtin command.

From: Kristian Høgsberg <krh@redhat.com>
Date: Sep 17, 2007, 22:58 UTC
Message-ID: <1190069881.10112.10.camel@hinata.boston.redhat.com>
In-Reply-To: <Pine.LNX.4.64.0709061741370.28586@racer.site>

On Thu, 2007-09-06 at 17:59 +0100, Johannes Schindelin wrote:

Show 16 quoted lines

> Hi,
> 
> On Wed, 5 Sep 2007, Kristian Høgsberg wrote:
> 
> >  contrib/examples/git-commit.sh |  665 +++++++++++++++++++++++++++++++++++
> >  git-commit.sh                  |  665 -----------------------------------
> 
> You might want to use "git format-patch -M" next time ;-)
> 
> > @@ -357,7 +358,6 @@ BUILTIN_OBJS = \
> >  	builtin-rev-parse.o \
> >  	builtin-revert.o \
> >  	builtin-rm.o \
> > -	builtin-runstatus.o \
> 
> Better keep it; some people's scripts could depend on it.

Seriously? Why don't we remove it and see if somebody yells? It's more of an implementation detail than most other git commands; if you need status output in your script why wouldn't you just run git status?

Show 15 quoted lines

> > +struct option {
> > +    enum option_type type;
> > +    const char *long_name;
> > +    char short_name;
> > +    void *value;
> > +};
> > +
> > +static int scan_options(const char ***argv, struct option *options)
> > +{
> 
> I would not (no longer, anyway) be opposed to replacing the option parsing 
> in git with getopt(); I hear that it is small enough to keep a copy in 
> compat/getopt.c.
> 
> But let's go forward with builtin-commit; getopt() can come later.

I don't know. I think it's a situation much like the string library discussion. It's a small enough dependency (70 lines!) that there's no gain in depending on an external implementation, and we can tailor it to gits needs as we extend the use within git. And we can call it gitopt!

Show 63 quoted lines

> > +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(fd, files, NULL);
> > +		return lock_file.filename;
> > +	} else if (also) {
> > +		add_files_to_cache(fd, files, prefix);
> > +		return lock_file.filename;
> > +	}
> > +
> > +	if (interactive)
> > +		interactive_add();
> > +
> > +	if (*files == NULL) {
> > +		/* Commit index as-is. */
> > +		rollback_lock_file(&lock_file);
> > +		return get_index_file();
> > +	}
> > +
> > +	/*
> > +	 * FIXME: Warn on unknown files.  Shell script does
> > +	 *
> > +	 *   commit_only=`git-ls-files --error-unmatch -- "$@"`
> > +	 */
> > +
> > +	/*
> > +	 * FIXME: shell script does
> > +	 *
> > +	 *   git-read-tree --index-output="$TMP_INDEX" -i -m HEAD
> > +	 *
> > +	 * which warns about unmerged files in the index.
> > +	 */
> > +
> > +	/* update the user index file */
> > +	add_files_to_cache(fd, files, prefix);
> 
> I suspect this, or ...
> 
> > +
> > +	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(fd, files, prefix);
> 
> ... this, but not both.

No, this needs both. The first add_files_to_cache() call updates the user index (.git/index) by adding the given files, then we build a temporary index from a tree and add the files to that index.

Show 8 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)
> > +{
> > +	static const char signed_off_by[] = "Signed-off-by: ";
> 
> I think you already defined that globally earlier.

Ah, yes, fixed.

Show 12 quoted lines

> In the function message_is_empty() you write:
> 
> > +	/* See if the template is just a prefix of the message. */
> > +	strbuf_init(&tmpl);
> > +	if (template_file && strbuf_read_path(&tmpl, template_file) > 0) {
> > +		stripspace(&tmpl, 1);
> > +		if (start + tmpl.len <= sb->len &&
> > +		    memcmp(tmpl.buf, sb->buf + start, tmpl.len) == 0)
> > +			start += tmpl.len;
> 
> Could we not bail out here, if there is no match?  In that case, the 
> message is clearly not empty...

The contents could be just sign-off-by's.

Show 26 quoted lines

> > +	/* 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;
> 
> Why not just "if (isspace(sb->buf[i]) || sb->buf[i] == '\n') continue;"? 
> This would also catch the cases where people indent their S-O-Bs.
> 
> > +
> > +		if (strlen(signed_off_by) <= eol - i &&
> > +		    !prefixcmp(sb->buf + i, signed_off_by)) {
> > +			i = eol;
> > +			continue;
> > +		}
> > +		while (i < eol)
> > +			if (!isspace(sb->buf[i++]))
> > +				return 0;
> > +	}
> > +
> > +	return 1;
> > +}
> 
> I did not review the rest of the code closely yet...

I'm sending an updated version against Pierre's strbuf changes now. It's a smaller patch set, so hopefully we can get it in soon.

Kristian