Signed-off-by: Kristian Høgsberg <krh@redhat.com>

---

 Makefile        |    2 +-

 parse-options.c |   74 +++++++++++++++++++++++++++++++++++++++++++++++++++++++

 parse-options.h |   29 +++++++++++++++++++++

 3 files changed, 104 insertions(+), 1 deletions(-)

 create mode 100644 parse-options.c

 create mode 100644 parse-options.h
diff --git a/Makefile b/Makefile

index 62bdac6..d90e959 100644

--- a/Makefile

+++ b/Makefile

@@ -310,7 +310,7 @@ LIB_OBJS = \

 	alloc.o merge-file.o path-list.o help.o unpack-trees.o $(DIFF_OBJS) \

 	color.o wt-status.o archive-zip.o archive-tar.o shallow.o utf8.o \

 	convert.o attr.o decorate.o progress.o mailmap.o symlinks.o remote.o \

-	transport.o bundle.o

+	transport.o bundle.o parse-options.o
 BUILTIN_OBJS = \

 	builtin-add.o \

diff --git a/parse-options.c b/parse-options.c

new file mode 100644

index 0000000..2fb30cd

--- /dev/null

+++ b/parse-options.c

@@ -0,0 +1,74 @@

+#include "git-compat-util.h"

+#include "parse-options.h"

+

+int parse_options(const char ***argv,

+		  struct option *options, int count,

+		  const char *usage_string)

+{

+	const char *value, *eq;

+	int i;

+

+	if (**argv == NULL)

+		return 0;

+	if ((**argv)[0] != '-')

+		return 0;

+	if (!strcmp(**argv, "--"))

+		return 0;

+

+	value = NULL;

+	for (i = 0; i < count; i++) {

+		if ((**argv)[1] == '-') {

+			if (!prefixcmp(options[i].long_name, **argv + 2)) {

+				if (options[i].type != OPTION_BOOLEAN)

+					value = *++(*argv);

+				goto match;

+			}

+

+			eq = strchr(**argv + 2, '=');

+			if (eq && options[i].type != OPTION_BOOLEAN &&

+			    !strncmp(**argv + 2,

+				     options[i].long_name, eq - **argv - 2)) {

+				value = eq + 1;

+				goto match;

+			}

+		}

+

+		if ((**argv)[1] == options[i].short_name) {

+			if ((**argv)[2] == '\0') {

+				if (options[i].type != OPTION_BOOLEAN)

+					value = *++(*argv);

+				goto match;

+			}

+

+			if (options[i].type != OPTION_BOOLEAN) {

+				value = **argv + 2;

+				goto match;

...
[ message continues ]

Hello Kristian,
I have some comments on your patch. Some of the "improvement" might have

to wait until after your builtin-commit changes hits git.git. However,

if we could agree on some of the general changes, I could start porting

other of the main porcelain commands to use the option parser without

depending on the state of the remaining builtin-commit series.
I don't know if this is a bug, but you do not remove "--" from argv,

which is later (in the patch that adds builtin-commit.c) passed to

add_files_to_cache and then get_pathspec where it is not removed or
I think the goto can be avoided by simply breaking out of the above loop

	case OPTION_LAST

		usage(usage_string);
I've been looking at builtin-blame.c which IMO has some of the most

obscure option parsing and maybe this can be changed to increment in

order to support stuff like changing the meaning by passing the same arg

multiple times (e.g. "-C -C -C") better.
Blame option parsing also sports (enum) flags being masked together,

this can of course be rewritten to a boolean option followed by
... and this to:
		if (!value) {

			error("option %s requires a value.", (*argv)[-1]);

			usage(usage_string);
Space vs tab indentation.
One of the last things I miss from Cogito is the nice abbreviated help

messages that was available via '-h'. I don't know if it would be

acceptable (at least for the main porcelain commands) to put this

functionality into the option parser by adding a "description" member to

struct option and have parse_options print a nice:
	<error message if any>

	<usage string>

	<option summary>
on failure, or, if that is regarded as too verbose, simply when -h is
This prefix aware option parsing has not been ported over to the other

builtins when they were lifted from shell code. It might be nice to have
I think the interface could be improved a bit. For example, it doesn't

need to count argument since the last entry in the options array is

OPTION_LAST and thus t...
[ message continues ]

Hi Jonas,
That's sounds like a good plan.  In fact, in you want to update the

patch with your changes (they all sound good) and start porting over

some of the other builtins feel free.  I don't have much time follow up

on these comments right now, but I will get to it eventually - unless

you beat me to it of course ;)  I will update builtin-commit.c to work

with whatever changes you introduce once I get around to updating that
Yeah, that looks nicer.  I think the goto structure is a leftover from

when there was more logic between the loop and the switch.  It's good to

get some fresh eyes on this code.  Junio didn't like the OPTION_LAST

terminator, so I changed the interface to take a count.  We can do
	if (i == count)

		usage();

	else switch (options[i].type) {

		...

	}
Yeah, that might be nice.  We can add it in a follow-on patch, if the
I don't ever use it myself and I think it's more confusing than helpful.

I only added it to avoid introducing behavior changes in the port.  I
Hehe, yeah, that's how I did it first.  I don't have a strong preference

for terminator elements vs. ARRAY_SIZE(), but Junio prefers the

ARRAY_SIZE() approach, I guess.  At this point I'm just trying the get
Heh, that's what I do myself :)
thanks for the comments,

Kristian
-

Hi,
It might be convenient, but I think that it is really more confusing than

helpful, especially with options that share a prefix.  Besides, we have

good completion for bash now (and I hear that this "zsh" thing also has 
FWIW I like the ARRAY_SIZE() approach better, too, since it is less error

prone.
Ciao,

Dscho
-

OK, I must have missed that comment. Good point.
Thanks for the comments both of you. It's great to have something to

work from. However, I also fear it will also require that some extra

flags or information is added to the option information to make it more

generally usable. But I guess that is easier to discuss in the context

of a patch.
--

Jonas Fonseca

-

I just sent an updated option parser patch that incorporates your

suggestions along with a patch that ports builtin-add.c to use it.  I

looked briefly into porting over a few other builtins, but you're right,

we need a couple of extra features for this to be really worthwile:
      * OPTION_SET_FLAG: sets the bit (we need to add a bit value that

        the option parser can or in)

      * OPTION_CLEAR_FLAG: clear the bit

      * OPTION_ADD: adds the value to the destination integer

      * OPTION_CALLBACK: calls the given function when the option is

        matched.  We'll need this for builtin-grep that has positional

        args such as --and etc.
Also, the option parser should probably verify that a string option

isn't passed more than once.  Bundling of single letter options would be

nice to add.  But the patch I just sent out should be a good start, and

it lets us move forward with builtin-commit.c.
cheers,

Kristian
-

Hi,
We _have_ to modify argv.  For example, "git log master -p" is perfectly

valid.
Ciao,

Dscho
-

We're not going to support POSIXLY_CORRECT!? ;)
-Peff

-

Ah, yes this could be nice to also finally have (more universally) in

git. But for this to be possible I don't see any reason for it to modify

the pointer to argv. Instead, it can just reshuffle entries in argv.
--

Jonas Fonseca

-

Hi,
In that case, I misunderstood you.  Indeed, I'd only reshuffle the

entries of argv.
Ciao,

Dscho
-

Signed-off-by: Kristian Høgsberg <krh@redhat.com>

---

 builtin-add.c |    8 ++++----

 commit.h      |    2 ++

 2 files changed, 6 insertions(+), 4 deletions(-)
diff --git a/builtin-add.c b/builtin-add.c

index 5e30380..966e145 100644

--- a/builtin-add.c

+++ b/builtin-add.c

@@ -107,7 +107,7 @@ static void update_callback(struct diff_queue_struct *q,

 	}

 }
-static void update(int verbose, const char *prefix, const char **files)

+void add_files_to_cache(int verbose, const char *prefix, const char **files)

 {

 	struct rev_info rev;

 	init_revisions(&rev, prefix);

@@ -116,8 +116,6 @@ static void update(int verbose, const char *prefix, const char **files)

 	rev.diffopt.output_format = DIFF_FORMAT_CALLBACK;

 	rev.diffopt.format_callback = update_callback;

 	rev.diffopt.format_callback_data = &verbose;

-	if (read_cache() < 0)

-		die("index file corrupt");

 	run_diff_files(&rev, 0);

 }
@@ -218,7 +216,9 @@ int cmd_add(int argc, const char **argv, const char *prefix)

 	}
 	if (take_worktree_changes) {

-		update(verbose, prefix, argv + i);

+		if (read_cache() < 0)

+			die("index file corrupt");

+		add_files_to_cache(verbose, prefix, argv + i);

 		goto finish;

 	}
diff --git a/commit.h b/commit.h

index cc8d890..89caa12 100644

--- a/commit.h

+++ b/commit.h

@@ -130,6 +130,8 @@ extern struct commit_list *get_shallow_commits(struct object_array *heads,

 int in_merge_bases(struct commit *, struct commit **, int);
 extern int interactive_add(void);

+extern void add_files_to_cache(int verbose,

+			       const char *prefix, const char **files);

 extern int rerere(void);
 #endif /* COMMIT_H */

--

1.5.2.GIT
-

Hi,
On Thu, 27 Sep 2007, Kristian H

Hmmmmmmmm.  read-cache.c has been one of the lowest level files

that define atomic operations, similar to sha1_file.c, and this

function is much more porcelain-ish molecule operation.  I'd

rather not.
What other useful molecules would we have and/or need to have by

splitting existing standalone commands?  rerere() is another,

and probably we would need to rip the --with-tree bit from

ls-files out to make it usable from builtin-commit.
I do not think we would want to go to the "one file per

function" extreme, either.  Can we have a new file that hold

these helper functions for porcelains?
-

Move git-commit.sh to contrib/examples.
Signed-off-by: Kristian Høgsberg <krh@redhat.com>

---

 Makefile                       |    9 +-

 builtin-commit.c               |  611 +++++++++++++++++++++++++++++++++++++++

 builtin-tag.c                  |    3 +-

 builtin.h                      |    3 +-

 contrib/examples/git-commit.sh |  627 ++++++++++++++++++++++++++++++++++++++++

 git-commit.sh                  |  627 ----------------------------------------

 git.c                          |    3 +-

 strbuf.h                       |    1 +

 t/t3501-revert-cherry-pick.sh  |    4 +-

 t/t3901-i18n-patch.sh          |    8 +-

 t/test-lib.sh                  |    4 +-

 11 files changed, 1255 insertions(+), 645 deletions(-)

 create mode 100644 builtin-commit.c

 create mode 100755 contrib/examples/git-commit.sh

 delete mode 100755 git-commit.sh
diff --git a/Makefile b/Makefile

index d90e959..6172589 100644

--- a/Makefile

+++ b/Makefile

@@ -206,7 +206,7 @@ BASIC_LDFLAGS =
 SCRIPT_SH = \

 	git-bisect.sh git-checkout.sh \

-	git-clean.sh git-clone.sh git-commit.sh \

+	git-clean.sh git-clone.sh \

 	git-ls-remote.sh \

 	git-merge-one-file.sh git-mergetool.sh git-parse-remote.sh \

 	git-pull.sh git-rebase.sh git-rebase--interactive.sh \

@@ -254,7 +254,7 @@ EXTRA_PROGRAMS =

 BUILT_INS = \

 	git-format-patch$X git-show$X git-whatchanged$X git-cherry$X \

 	git-get-tar-commit-id$X git-init$X git-repo-config$X \

-	git-fsck-objects$X git-cherry-pick$X \

+	git-fsck-objects$X git-cherry-pick$X git-status$X\

 	$(patsubst builtin-%.o,git-%$X,$(BUILTIN_OBJS))
 # what 'all' will build and 'install' will install, in gitexecdir

@@ -324,6 +324,7 @@ BUILTIN_OBJS = \

 	builtin-check-attr.o \

 	builtin-checkout-index.o \

 	builtin-check-ref-format.o \

+	builtin-commit.o \

 	builtin-commit-tree.o \

 	builtin-count-objects.o \

 	builtin-describe.o \

@@ -362,7 +363,6 @@ BUILTIN_OBJS = \

 	builtin-rev-parse.o \

 	builtin-revert.o \

 	builtin-rm.o \

-	builtin-runstatus.o \

 	builtin-sho...
[ message continues ]