git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: "Kristian Høgsberg" <krh@redhat.com>
To: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: git@vger.kernel.org
Subject: Re: [PATCH 9/9] Implement git commit as a builtin command.
Date: Mon, 17 Sep 2007 18:58:01 -0400	[thread overview]
Message-ID: <1190069881.10112.10.camel@hinata.boston.redhat.com> (raw)
In-Reply-To: <Pine.LNX.4.64.0709061741370.28586@racer.site>

On Thu, 2007-09-06 at 17:59 +0100, Johannes Schindelin wrote:
> 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?

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

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

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

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

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

  reply	other threads:[~2007-09-17 22:58 UTC|newest]

Thread overview: 25+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2007-09-06  0:23 [PATCH 1/9] Enable wt-status output to a given FILE pointer Kristian Høgsberg
2007-09-06  0:23 ` [PATCH 2/9] Enable wt-status to run against non-standard index file Kristian Høgsberg
2007-09-06  0:23   ` [PATCH 3/9] Add strbuf_printf() to do formatted printing to a strbuf Kristian Høgsberg
2007-09-06  0:23     ` [PATCH 4/9] Introduce entry point for launching add--interactive Kristian Høgsberg
2007-09-06  0:23       ` [PATCH 5/9] Introduce strbuf_read_fd() Kristian Høgsberg
2007-09-06  0:23         ` [PATCH 6/9] Rewrite launch_editor, create_tag and stripspace to use strbufs Kristian Høgsberg
2007-09-06  0:23           ` [PATCH 7/9] Add strbuf_read_path() Kristian Høgsberg
2007-09-06  0:23             ` [PATCH 8/9] Export rerere() and launch_editor() Kristian Høgsberg
2007-09-06  0:23               ` [PATCH 9/9] Implement git commit as a builtin command Kristian Høgsberg
2007-09-06 16:59                 ` Johannes Schindelin
2007-09-17 22:58                   ` Kristian Høgsberg [this message]
2007-09-17 23:16                     ` Johannes Schindelin
2007-09-17 23:56                     ` Jeff King
2007-09-18  0:11                       ` Kristian Høgsberg
2007-09-06 16:40             ` [PATCH 7/9] Add strbuf_read_path() Johannes Schindelin
2007-09-06 16:38           ` [PATCH 6/9] Rewrite launch_editor, create_tag and stripspace to use strbufs Johannes Schindelin
2007-09-17 22:59             ` Kristian Høgsberg
2007-09-06 16:31       ` [PATCH 4/9] Introduce entry point for launching add--interactive Johannes Schindelin
2007-09-17 23:13         ` Kristian Høgsberg
2007-09-17 23:27           ` Johannes Schindelin
2007-09-06  8:55     ` [PATCH 3/9] Add strbuf_printf() to do formatted printing to a strbuf Junio C Hamano
2007-09-06  9:43       ` Pierre Habouzit
2007-09-06  9:50         ` Pierre Habouzit
2007-09-06 16:27 ` [PATCH 1/9] Enable wt-status output to a given FILE pointer Johannes Schindelin
2007-09-17 23:30   ` Kristian Høgsberg

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=1190069881.10112.10.camel@hinata.boston.redhat.com \
    --to=krh@redhat.com \
    --cc=Johannes.Schindelin@gmx.de \
    --cc=git@vger.kernel.org \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).