option directive to fast-import

option directive to fast-import

[Sverre Rabbelier]

Heya,

What would you think of a 'option' directive to fast-import?

'option' SP <option> SP <value> LF

Where <option> is:

import-marks
export-marks
force_update
quiet
stats
date-format
depth
active-branches
export-pack-edges

I just checked them all, and as far as I can see, as long as the
option directives come before any data, there should be no negative
side-effects to setting these values 'late'. The upside would be that
the frontend can tell git fast-import it's preferences, so that the
user does not have to type a humongous commandline, but instead can
suffice to just go with 'foo fast-export | git fast-import'. Of
course, the user might want to override these options manually, so the
option directives should only be honored if no commandline flag is
given.

Would you accept such a patch? If yes, I'll whip something up :).

-- 
Cheers,

Sverre Rabbelier

[RFC PATCH 0/3] fast-import: add option command

[Sverre Rabbelier]

Because I am too impatient to wait for a reply to my earlier mail,
here is an RFC series that demonstrates how I envision the option
command to work.

>From the second patch:

    This allows the frontend to specify any of the supported options as
    long as no non-option command has been given. This way the
    user does not have to include any frontend-specific options, but
    instead she can rely on the frontend to tell fast-import what it
    needs.

This change of course means that old fast-import clients will break
upon receiving an 'option' command (or with an argument they don't
support), but such clients will break with a clear output stating
the reason for the breakage. Newer frontends therefore should only
output options if the user tells them to (by means of a flag/config
option), or at least allow disabling option output.

The main use case for this is hg-git, which I want to modify so that
it uses 'hg fast-export | git fast-import' for the intial import.
However, to do that I need the fast-import part to write a marks
file, that is, --write-marks=git.marks. To simplify this process for
the user, it would be nice if 'hg fast-export' can instead emit an
'option write-marks git.marks' line (hence the test case).

 Sverre Rabbelier (3):
      fast-import: put option parsing code in seperate functions
      fast-import: add option command
      fast-import: test the new option command

 fast-import.c          |  137 ++++++++++++++++++++++++++++++++++++-----------
 t/t9300-fast-import.sh |   33 ++++++++++++
 2 files changed, 138 insertions(+), 32 deletions(-)

[RFC PATCH 1/2] fast-import: put option parsing code in seperate functions

[Sverre Rabbelier]

Putting the options in their own functions increases readability of
the option parsing block and makes it easier to reuse the option
parsing code later on.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

    I renamed import_marks to option_import_marks for consistency, since it is
    used to deal with a certain option.

 fast-import.c |   98 ++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 66 insertions(+), 32 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 7ef9865..57fff3b 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -291,6 +291,7 @@ static unsigned long branch_count;
 static unsigned long branch_load_count;
 static int failure;
 static FILE *pack_edges;
+static unsigned int show_stats = 1;
 
 /* Memory pools */
 static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mem_pool);
@@ -2337,7 +2338,7 @@ static void parse_progress(void)
 	skip_optional_lf();
 }
 
-static void import_marks(const char *input_file)
+static void option_import_marks(const char *input_file)
 {
 	char line[512];
 	FILE *f = fopen(input_file, "r");
@@ -2372,6 +2373,55 @@ static void import_marks(const char *input_file)
 	fclose(f);
 }
 
+static void option_date_format(const char *fmt) {
+	if (!strcmp(fmt, "raw"))
+		whenspec = WHENSPEC_RAW;
+	else if (!strcmp(fmt, "rfc2822"))
+		whenspec = WHENSPEC_RFC2822;
+	else if (!strcmp(fmt, "now"))
+		whenspec = WHENSPEC_NOW;
+	else
+		die("unknown --date-format argument %s", fmt);
+}
+
+static void option_max_pack_size(const char *packsize) {
+	max_packsize = strtoumax(packsize, NULL, 0) * 1024 * 1024;
+}
+
+static void option_depth(const char *depth) {
+	max_depth = strtoul(depth, NULL, 0);
+	if (max_depth > MAX_DEPTH)
+		die("--depth cannot exceed %u", MAX_DEPTH);
+}
+
+static void option_active_branches(const char *branches) {
+	max_active_branches = strtoul(branches, NULL, 0);
+}
+
+static void option_export_marks(const char *marks) {
+	mark_file = marks;
+}
+
+static void option_export_pack_edges(const char *edges) {
+	if (pack_edges)
+		fclose(pack_edges);
+	pack_edges = fopen(edges, "a");
+	if (!pack_edges)
+		die_errno("Cannot open '%s'", edges);
+}
+
+static void option_force() {
+	force_update = 1;
+}
+
+static void option_quiet() {
+	show_stats = 0;
+}
+
+static void option_stats() {
+	show_stats = 1;
+}
+
 static int git_pack_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "pack.depth")) {
@@ -2398,7 +2448,7 @@ static const char fast_import_usage[] =
 
 int main(int argc, const char **argv)
 {
-	unsigned int i, show_stats = 1;
+	unsigned int i;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -2419,42 +2469,26 @@ int main(int argc, const char **argv)
 
 		if (*a != '-' || !strcmp(a, "--"))
 			break;
-		else if (!prefixcmp(a, "--date-format=")) {
-			const char *fmt = a + 14;
-			if (!strcmp(fmt, "raw"))
-				whenspec = WHENSPEC_RAW;
-			else if (!strcmp(fmt, "rfc2822"))
-				whenspec = WHENSPEC_RFC2822;
-			else if (!strcmp(fmt, "now"))
-				whenspec = WHENSPEC_NOW;
-			else
-				die("unknown --date-format argument %s", fmt);
-		}
+		else if (!prefixcmp(a, "--date-format="))
+			option_date_format(a + 14);
 		else if (!prefixcmp(a, "--max-pack-size="))
-			max_packsize = strtoumax(a + 16, NULL, 0) * 1024 * 1024;
-		else if (!prefixcmp(a, "--depth=")) {
-			max_depth = strtoul(a + 8, NULL, 0);
-			if (max_depth > MAX_DEPTH)
-				die("--depth cannot exceed %u", MAX_DEPTH);
-		}
+			option_max_pack_size(a + 16);
+		else if (!prefixcmp(a, "--depth="))
+			option_depth(a + 8);
 		else if (!prefixcmp(a, "--active-branches="))
-			max_active_branches = strtoul(a + 18, NULL, 0);
+			option_active_branches(a + 18);
 		else if (!prefixcmp(a, "--import-marks="))
-			import_marks(a + 15);
+			option_import_marks(a + 15);
 		else if (!prefixcmp(a, "--export-marks="))
-			mark_file = a + 15;
-		else if (!prefixcmp(a, "--export-pack-edges=")) {
-			if (pack_edges)
-				fclose(pack_edges);
-			pack_edges = fopen(a + 20, "a");
-			if (!pack_edges)
-				die_errno("Cannot open '%s'", a + 20);
-		} else if (!strcmp(a, "--force"))
-			force_update = 1;
+			option_export_marks(a + 15);
+		else if (!prefixcmp(a, "--export-pack-edges="))
+			option_export_pack_edges(a + 20);
+		else if (!strcmp(a, "--force"))
+			option_force();
 		else if (!strcmp(a, "--quiet"))
-			show_stats = 0;
+			option_quiet();
 		else if (!strcmp(a, "--stats"))
-			show_stats = 1;
+			option_stats();
 		else
 			die("unknown option %s", a);
 	}
-- 
1.6.3.GIT

[RFC PATCH 2/2] fast-import: add option command

[Sverre Rabbelier]

This allows the frontend to specify any of the supported options as
long as no non-option command has been given. This way the
user does not have to include any frontend-specific options, but
instead she can rely on the frontend to tell fast-import what it
needs.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

    This addds an extra prefixcmp for each command, but only for the
    first command, and any 'option' commands. I felt that was better
    than to add seen_non_option_command=1 to each other command.

 fast-import.c |   39 +++++++++++++++++++++++++++++++++++++++
 1 files changed, 39 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 57fff3b..2007ed8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -348,6 +348,7 @@ static struct recent_command *rc_free;
 static unsigned int cmd_save = 100;
 static uintmax_t next_mark;
 static struct strbuf new_data = STRBUF_INIT;
+static int seen_non_option_command;
 
 static void write_branch_report(FILE *rpt, struct branch *b)
 {
@@ -1663,6 +1664,10 @@ static int read_next_command(void)
 			if (stdin_eof)
 				return EOF;
 
+			if (!seen_non_option_command
+				&& prefixcmp(command_buf.buf, "option "))
+				seen_non_option_command = 1;
+
 			rc = rc_free;
 			if (rc)
 				rc_free = rc->next;
@@ -2422,6 +2427,38 @@ static void option_stats() {
 	show_stats = 1;
 }
 
+static void parse_option(void)
+{
+    char* option = command_buf.buf + 7;
+
+	if (seen_non_option_command)
+		die("Got option command '%s' after non-option command", option);
+
+    if (!prefixcmp(option, "date-format")) {
+		option_date_format(option + 12);
+    } else if (!prefixcmp(option, "max-pack-size")) {
+		option_max_pack_size(option + 14);
+    } else if (!prefixcmp(option, "depth")) {
+		option_depth(option + 6);
+    } else if (!prefixcmp(option, "active-branches")) {
+		option_active_branches(option + 16);
+    } else if (!prefixcmp(option, "import-marks")) {
+		option_import_marks(option + 13);
+    } else if (!prefixcmp(option, "export-marks")) {
+		option_export_marks(option + 13);
+    } else if (!prefixcmp(option, "export-pack-edges")) {
+		option_export_pack_edges(option + 18);
+    } else if (!prefixcmp(option, "force")) {
+		option_force();
+    } else if (!prefixcmp(option, "quiet")) {
+		option_quiet();
+    } else if (!prefixcmp(option, "stats")) {
+		option_stats();
+    } else {
+		die("Unsupported option: %s", option);
+    }
+}
+
 static int git_pack_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "pack.depth")) {
@@ -2516,6 +2553,8 @@ int main(int argc, const char **argv)
 			parse_checkpoint();
 		else if (!prefixcmp(command_buf.buf, "progress "))
 			parse_progress();
+		else if (!prefixcmp(command_buf.buf, "option "))
+			parse_option();
 		else
 			die("Unsupported command: %s", command_buf.buf);
 	}
-- 
1.6.3.GIT

[RFC PATCH 3/3] fast-import: test the new option command

[Sverre Rabbelier]

Test the new 'option' command to fast-import to make sure that they
indeed affect the behavior in the same way as the commandline
options.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

    Since the option code is now very similar, I feel confident that
    it works after testing two random [0] options.

    [0] where with random I mean 'easy to test'

 t/t9300-fast-import.sh |   33 +++++++++++++++++++++++++++++++++
 1 files changed, 33 insertions(+), 0 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 821be7c..a5afe9a 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1088,4 +1088,37 @@ INPUT_END
 test_expect_success 'P: fail on blob mark in gitlink' '
     test_must_fail git fast-import <input'
 
+###
+### series Q (options)
+###
+
+cat >input << EOF
+option quiet
+blob
+data 3
+hi
+
+EOF
+
+touch empty
+
+test_expect_success 'Q: quiet option results in no stats being output' '
+    cat input | git fast-import 2> output &&
+    test_cmp empty output
+'
+
+cat >input << EOF
+option export-marks git.marks
+blob
+mark :1
+data 3
+hi
+
+EOF
+
+test_expect_success \
+    'Q: export-marks option results in a marks file being created' \
+    'cat input | git fast-import 2> output &&
+    grep :1 git.marks'
+
 test_done
-- 
1.6.4.15.g8b2be

[RFC PATCH v1a 1/3] fast-import: put option parsing code in seperate functions

[Sverre Rabbelier]

Putting the options in their own functions increases readability of
the option parsing block and makes it easier to reuse the option
parsing code later on.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---

    Good thing nobody saw my patch yet, it has a bug already.
    It used to be ok to assign the contents of *a to the marks_file
    since *a came from argv, which does not change. However, since
    the function is being factored out for future use where we cannot
    be sure about the validity of the argument after we return, it
    should be copied. All other options convert the argument into
    a safe value type (integer, etc).

 fast-import.c |  100 ++++++++++++++++++++++++++++++++++++++------------------
 1 files changed, 68 insertions(+), 32 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 7ef9865..f624f08 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -291,6 +291,7 @@ static unsigned long branch_count;
 static unsigned long branch_load_count;
 static int failure;
 static FILE *pack_edges;
+static unsigned int show_stats = 1;
 
 /* Memory pools */
 static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mem_pool);
@@ -2337,7 +2338,7 @@ static void parse_progress(void)
 	skip_optional_lf();
 }
 
-static void import_marks(const char *input_file)
+static void option_import_marks(const char *input_file)
 {
 	char line[512];
 	FILE *f = fopen(input_file, "r");
@@ -2372,6 +2373,57 @@ static void import_marks(const char *input_file)
 	fclose(f);
 }
 
+static void option_date_format(const char *fmt) {
+	if (!strcmp(fmt, "raw"))
+		whenspec = WHENSPEC_RAW;
+	else if (!strcmp(fmt, "rfc2822"))
+		whenspec = WHENSPEC_RFC2822;
+	else if (!strcmp(fmt, "now"))
+		whenspec = WHENSPEC_NOW;
+	else
+		die("unknown --date-format argument %s", fmt);
+}
+
+static void option_max_pack_size(const char *packsize) {
+	max_packsize = strtoumax(packsize, NULL, 0) * 1024 * 1024;
+}
+
+static void option_depth(const char *depth) {
+	max_depth = strtoul(depth, NULL, 0);
+	if (max_depth > MAX_DEPTH)
+		die("--depth cannot exceed %u", MAX_DEPTH);
+}
+
+static void option_active_branches(const char *branches) {
+	max_active_branches = strtoul(branches, NULL, 0);
+}
+
+static void option_export_marks(const char *marks) {
+	struct strbuf buf = STRBUF_INIT;
+	strbuf_addstr(&buf, marks);
+	mark_file = strbuf_detach(&buf, NULL);
+}
+
+static void option_export_pack_edges(const char *edges) {
+	if (pack_edges)
+		fclose(pack_edges);
+	pack_edges = fopen(edges, "a");
+	if (!pack_edges)
+		die_errno("Cannot open '%s'", edges);
+}
+
+static void option_force() {
+	force_update = 1;
+}
+
+static void option_quiet() {
+	show_stats = 0;
+}
+
+static void option_stats() {
+	show_stats = 1;
+}
+
 static int git_pack_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "pack.depth")) {
@@ -2398,7 +2450,7 @@ static const char fast_import_usage[] =
 
 int main(int argc, const char **argv)
 {
-	unsigned int i, show_stats = 1;
+	unsigned int i;
 
 	git_extract_argv0_path(argv[0]);
 
@@ -2419,42 +2471,26 @@ int main(int argc, const char **argv)
 
 		if (*a != '-' || !strcmp(a, "--"))
 			break;
-		else if (!prefixcmp(a, "--date-format=")) {
-			const char *fmt = a + 14;
-			if (!strcmp(fmt, "raw"))
-				whenspec = WHENSPEC_RAW;
-			else if (!strcmp(fmt, "rfc2822"))
-				whenspec = WHENSPEC_RFC2822;
-			else if (!strcmp(fmt, "now"))
-				whenspec = WHENSPEC_NOW;
-			else
-				die("unknown --date-format argument %s", fmt);
-		}
+		else if (!prefixcmp(a, "--date-format="))
+			option_date_format(a + 14);
 		else if (!prefixcmp(a, "--max-pack-size="))
-			max_packsize = strtoumax(a + 16, NULL, 0) * 1024 * 1024;
-		else if (!prefixcmp(a, "--depth=")) {
-			max_depth = strtoul(a + 8, NULL, 0);
-			if (max_depth > MAX_DEPTH)
-				die("--depth cannot exceed %u", MAX_DEPTH);
-		}
+			option_max_pack_size(a + 16);
+		else if (!prefixcmp(a, "--depth="))
+			option_depth(a + 8);
 		else if (!prefixcmp(a, "--active-branches="))
-			max_active_branches = strtoul(a + 18, NULL, 0);
+			option_active_branches(a + 18);
 		else if (!prefixcmp(a, "--import-marks="))
-			import_marks(a + 15);
+			option_import_marks(a + 15);
 		else if (!prefixcmp(a, "--export-marks="))
-			mark_file = a + 15;
-		else if (!prefixcmp(a, "--export-pack-edges=")) {
-			if (pack_edges)
-				fclose(pack_edges);
-			pack_edges = fopen(a + 20, "a");
-			if (!pack_edges)
-				die_errno("Cannot open '%s'", a + 20);
-		} else if (!strcmp(a, "--force"))
-			force_update = 1;
+			option_export_marks(a + 15);
+		else if (!prefixcmp(a, "--export-pack-edges="))
+			option_export_pack_edges(a + 20);
+		else if (!strcmp(a, "--force"))
+			option_force();
 		else if (!strcmp(a, "--quiet"))
-			show_stats = 0;
+			option_quiet();
 		else if (!strcmp(a, "--stats"))
-			show_stats = 1;
+			option_stats();
 		else
 			die("unknown option %s", a);
 	}
-- 
1.6.4.16.g72c66.dirty

Re: [RFC PATCH 0/3] fast-import: add option command

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
>     This allows the frontend to specify any of the supported options as
>     long as no non-option command has been given. This way the
>     user does not have to include any frontend-specific options, but
>     instead she can rely on the frontend to tell fast-import what it
>     needs.
...
>  fast-import.c          |  137 ++++++++++++++++++++++++++++++++++++-----------
>  t/t9300-fast-import.sh |   33 ++++++++++++
>  2 files changed, 138 insertions(+), 32 deletions(-)

Some comments:

Since you are changing the language format, please also update the
documentation of the language.

It might be cleaner to say "option foo=value\n" because then the
if block to parse the command line and the if block to parse the
input stream are the same.  When parsing argv just skip the "--"
and pass the rest of the argv value into the function, when parsing
the stream, just skip the "option " and pass the rest of the line
into the function.

This has come up before on the list.  We had somewhat decided against
setting options in the stream header.  The only option class that
really impacts the data itself is the date format, all others
are about local file paths or how noisy the command should be.
Thus we decided that the frontend should invoke `git fast-import`
itself if it cared about these options, and that's what any typical
frontend does.

-- 
Shawn.

Re: [RFC PATCH 0/3] fast-import: add option command

[Sverre Rabbelier]

Heya,

On Sun, Aug 2, 2009 at 17:15, Shawn O. Pearce<spearce@spearce.org> wrote:>
> Since you are changing the language format, please also update the
> documentation of the language.

Of course, but I wanted to know whether the change'd be accepted first :).

> It might be cleaner to say "option foo=value\n" because then the
> if block to parse the command line and the if block to parse the
> input stream are the same.  When parsing argv just skip the "--"
> and pass the rest of the argv value into the function, when parsing
> the stream, just skip the "option " and pass the rest of the line
> into the function.

sounds like a good idea, will do.

> This has come up before on the list.  We had somewhat decided against
> setting options in the stream header.  The only option class that
> really impacts the data itself is the date format, all others
> are about local file paths or how noisy the command should be.
> Thus we decided that the frontend should invoke `git fast-import`
> itself if it cared about these options, and that's what any typical
> frontend does.

Hmmm, yes, I guess that's possible, but that would require the
frontend to shell out, whereas the option-based approach is easier to
the user without requiring the frontend to know how to invoke git. And
while the only option that impacts the data format is the date format,
the import and export marks are very relevant when the frontend wants
to do post-processing of the exported repository. I guess the question
is, do we want to require frontends to create a wrapper around
'frontend | git fast-import --all-my=options', or is just "fronted |
git fast-import" desirable enough that we want to allow this?

-- 
Cheers,

Sverre Rabbelier