[PATCH v6 0/6] fast-import: add new feature and mark command

[PATCH v6 0/6] fast-import: add new feature and mark command

[Sverre Rabbelier]

Incorperated comments and changed 'option foo' to 'option git foo'. I
think this is ready to be merged to next if there are no objections.

Sverre Rabbelier (6):
      fast-import: put option parsing code in separate functions
      fast-import: put marks reading in it's own function
      fast-import: add feature command
      fast-import: test the new feature command
      fast-import: add option command
      fast-import: test the new option command

 Documentation/git-fast-import.txt |   40 ++++++
 fast-import.c                     |  262 ++++++++++++++++++++++++++-----------
 t/t9300-fast-import.sh            |  102 ++++++++++++++
 3 files changed, 327 insertions(+), 77 deletions(-)

[PATCH v6 1/6] fast-import: put option parsing code in separate 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>
---

  No change since v5.

 fast-import.c |  115 +++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 75 insertions(+), 40 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 7ef9865..b904f20 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,76 @@ 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 = xstrdup(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 parse_one_option(const char *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")) {
+		force_update = 1;
+	} else if (!prefixcmp(option, "quiet")) {
+		show_stats = 0;
+	} else if (!prefixcmp(option, "stats")) {
+		show_stats = 1;
+	} else {
+		die("Unsupported option: %s", option);
+	}
+}
+
 static int git_pack_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "pack.depth")) {
@@ -2398,7 +2469,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,44 +2490,8 @@ 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, "--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);
-		}
-		else if (!prefixcmp(a, "--active-branches="))
-			max_active_branches = strtoul(a + 18, NULL, 0);
-		else if (!prefixcmp(a, "--import-marks="))
-			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;
-		else if (!strcmp(a, "--quiet"))
-			show_stats = 0;
-		else if (!strcmp(a, "--stats"))
-			show_stats = 1;
-		else
-			die("unknown option %s", a);
+
+		parse_one_option(a + 2);
 	}
 	if (i != argc)
 		usage(fast_import_usage);
-- 
1.6.4.16.g72c66.dirty

[PATCH v6 2/6] fast-import: put marks reading in it's own function

[Sverre Rabbelier]

All options do nothing but set settings, with the exception of the
--input-marks option. Delay the reading of the marks file till after
all options have been parsed.

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

  No change since v5.

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

diff --git a/fast-import.c b/fast-import.c
index b904f20..812fcf0 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -315,6 +315,7 @@ static struct object_entry_pool *blocks;
 static struct object_entry *object_table[1 << 16];
 static struct mark_set *marks;
 static const char *mark_file;
+static const char *input_file;
 
 /* Our last blob */
 static struct last_object last_blob = { STRBUF_INIT, 0, 0, 0 };
@@ -1643,6 +1644,42 @@ static void dump_marks(void)
 	}
 }
 
+static void read_marks(void)
+{
+	char line[512];
+	FILE *f = fopen(input_file, "r");
+	if (!f)
+		die_errno("cannot read '%s'", input_file);
+	while (fgets(line, sizeof(line), f)) {
+		uintmax_t mark;
+		char *end;
+		unsigned char sha1[20];
+		struct object_entry *e;
+
+		end = strchr(line, '\n');
+		if (line[0] != ':' || !end)
+			die("corrupt mark line: %s", line);
+		*end = 0;
+		mark = strtoumax(line + 1, &end, 10);
+		if (!mark || end == line + 1
+			|| *end != ' ' || get_sha1(end + 1, sha1))
+			die("corrupt mark line: %s", line);
+		e = find_object(sha1);
+		if (!e) {
+			enum object_type type = sha1_object_info(sha1, NULL);
+			if (type < 0)
+				die("object not found: %s", sha1_to_hex(sha1));
+			e = insert_object(sha1);
+			e->type = type;
+			e->pack_id = MAX_PACK_ID;
+			e->offset = 1; /* just not zero! */
+		}
+		insert_mark(mark, e);
+	}
+	fclose(f);
+}
+
+
 static int read_next_command(void)
 {
 	static int stdin_eof = 0;
@@ -2338,39 +2375,9 @@ static void parse_progress(void)
 	skip_optional_lf();
 }
 
-static void option_import_marks(const char *input_file)
+static void option_import_marks(const char *marks)
 {
-	char line[512];
-	FILE *f = fopen(input_file, "r");
-	if (!f)
-		die_errno("cannot read '%s'", input_file);
-	while (fgets(line, sizeof(line), f)) {
-		uintmax_t mark;
-		char *end;
-		unsigned char sha1[20];
-		struct object_entry *e;
-
-		end = strchr(line, '\n');
-		if (line[0] != ':' || !end)
-			die("corrupt mark line: %s", line);
-		*end = 0;
-		mark = strtoumax(line + 1, &end, 10);
-		if (!mark || end == line + 1
-			|| *end != ' ' || get_sha1(end + 1, sha1))
-			die("corrupt mark line: %s", line);
-		e = find_object(sha1);
-		if (!e) {
-			enum object_type type = sha1_object_info(sha1, NULL);
-			if (type < 0)
-				die("object not found: %s", sha1_to_hex(sha1));
-			e = insert_object(sha1);
-			e->type = type;
-			e->pack_id = MAX_PACK_ID;
-			e->offset = 1; /* just not zero! */
-		}
-		insert_mark(mark, e);
-	}
-	fclose(f);
+	input_file = xstrdup(marks);
 }
 
 static void option_date_format(const char *fmt)
@@ -2495,6 +2502,8 @@ int main(int argc, const char **argv)
 	}
 	if (i != argc)
 		usage(fast_import_usage);
+	if (input_file)
+		read_marks();
 
 	rc_free = pool_alloc(cmd_save * sizeof(*rc_free));
 	for (i = 0; i < (cmd_save - 1); i++)
-- 
1.6.4.16.g72c66.dirty

[PATCH v6 3/6] fast-import: add feature command

[Sverre Rabbelier]

This allows the fronted to require a specific feature to be supported
by the frontend, or abort.

Also add support for the first feature, date-format=.

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

  No longer RFC.

 Documentation/git-fast-import.txt |   16 ++++++++++++++++
 fast-import.c                     |   13 +++++++++++++
 2 files changed, 29 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index c2f483a..1e293f2 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -303,6 +303,10 @@ and control the current import process.  More detailed discussion
 	standard output.  This command is optional and is not needed
 	to perform an import.
 
+`feature`::
+	Require that fast-import supports the specified feature, or
+	abort if it does not.
+
 `commit`
 ~~~~~~~~
 Create or update a branch with a new commit, recording one logical
@@ -813,6 +817,18 @@ Placing a `progress` command immediately after a `checkpoint` will
 inform the reader when the `checkpoint` has been completed and it
 can safely access the refs that fast-import updated.
 
+`feature`
+~~~~~~~~~
+Require that fast-import supports the specified feature, or abort if
+it does not.
+
+....
+	'feature' SP <feature> LF
+....
+
+The <feature> part of the command may be any string matching
+[a-zA-Z-] and should be understood by a version of fast-import.
+
 Crash Reports
 -------------
 If fast-import is supplied invalid input it will terminate with a
diff --git a/fast-import.c b/fast-import.c
index 812fcf0..9bf06a4 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2450,6 +2450,17 @@ static void parse_one_option(const char *option)
 	}
 }
 
+static void parse_feature(void)
+{
+	char *feature = command_buf.buf + 8;
+
+	if (!prefixcmp(feature, "date-format=")) {
+		option_date_format(feature + 12);
+	} else {
+		die("This version of fast-import does not support feature %s.", feature);
+	}
+}
+
 static int git_pack_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "pack.depth")) {
@@ -2526,6 +2537,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, "feature "))
+			parse_feature();
 		else
 			die("Unsupported command: %s", command_buf.buf);
 	}
-- 
1.6.4.16.g72c66.dirty

[PATCH v6 4/6] fast-import: test the new feature command

[Sverre Rabbelier]

Test that an unknown feature causes fast-import to abort, and that a
known feature is accepted.

Signed-off-by: Sverre Rabbelier <srabbelier@gmail.com>
---
 t/t9300-fast-import.sh |   20 ++++++++++++++++++++
 1 files changed, 20 insertions(+), 0 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 821be7c..564ed6b 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1088,4 +1088,24 @@ INPUT_END
 test_expect_success 'P: fail on blob mark in gitlink' '
     test_must_fail git fast-import <input'
 
+###
+### series R (feature)
+###
+
+cat >input <<EOF
+feature no-such-feature-exists
+EOF
+
+test_expect_success 'R: abort on unsupported feature' '
+	test_must_fail git fast-import <input
+'
+
+cat >input <<EOF
+feature date-format=now
+EOF
+
+test_expect_success 'R: supported feature is accepted' '
+	git fast-import <input
+'
+
 test_done
-- 
1.6.4.16.g72c66.dirty

[PATCH v6 5/6] 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.

Also factor out parsing of argv and have it execute when we reach the
first non-option command, or after all commands have been read and
no non-option command has been encountered.

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

  Main difference with v5 is that the syntax is now 'option git ...'
  as per a discussion with the other fast-import devs. Other options,
  e.g. 'option hg' are ignored. Also fixed the docs to say that
  feature commands are allowed before git option commands.

 Documentation/git-fast-import.txt |   24 ++++++++++++
 fast-import.c                     |   75 +++++++++++++++++++++++++++++++------
 2 files changed, 87 insertions(+), 12 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index 1e293f2..f1c94b4 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -307,6 +307,11 @@ and control the current import process.  More detailed discussion
 	Require that fast-import supports the specified feature, or
 	abort if it does not.
 
+`option`::
+    Specify any of the options listed under OPTIONS to change
+    fast-import's behavior to suit the frontend's needs. This command
+    is optional and is not needed to perform an import.
+
 `commit`
 ~~~~~~~~
 Create or update a branch with a new commit, recording one logical
@@ -829,6 +834,25 @@ it does not.
 The <feature> part of the command may be any string matching
 [a-zA-Z-] and should be understood by a version of fast-import.
 
+`option`
+~~~~~~~~
+Processes the specified option so that git fast-import behaves in a
+way that suits the frontend's needs.
+Note that options specified by the frontend are overridden by any
+options the user may specify to git fast-import itself.
+
+....
+    'option' SP <option> LF
+....
+
+The `<option>` part of the command may contain any of the options
+listed in the OPTIONS section, without the leading '--' and is
+treated in the same way.
+
+Option commands must be the first commands on the input (not counting
+feature commands), to give an option command after any non-option
+command is an error.
+
 Crash Reports
 -------------
 If fast-import is supplied invalid input it will terminate with a
diff --git a/fast-import.c b/fast-import.c
index 9bf06a4..bad93dc 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -292,6 +292,8 @@ static unsigned long branch_load_count;
 static int failure;
 static FILE *pack_edges;
 static unsigned int show_stats = 1;
+static int global_argc;
+static const char **global_argv;
 
 /* Memory pools */
 static size_t mem_pool_alloc = 2*1024*1024 - sizeof(struct mem_pool);
@@ -349,6 +351,10 @@ 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 options_enabled;
+static int seen_non_option_command;
+
+static void parse_argv(void);
 
 static void write_branch_report(FILE *rpt, struct branch *b)
 {
@@ -1700,6 +1706,12 @@ static int read_next_command(void)
 			if (stdin_eof)
 				return EOF;
 
+			if (!seen_non_option_command
+				&& prefixcmp(command_buf.buf, "feature ")
+				&& prefixcmp(command_buf.buf, "option ")) {
+				parse_argv();
+			}
+
 			rc = rc_free;
 			if (rc)
 				rc_free = rc->next;
@@ -2456,11 +2468,31 @@ static void parse_feature(void)
 
 	if (!prefixcmp(feature, "date-format=")) {
 		option_date_format(feature + 12);
+	} else if (!strcmp("git-options", feature)) {
+		options_enabled = 1;
 	} else {
 		die("This version of fast-import does not support feature %s.", feature);
 	}
 }
 
+static void parse_option(void)
+{
+	char* option = command_buf.buf + 11;
+
+	if (!options_enabled)
+		die("Got option command '%s' before options feature'", option);
+
+	if (seen_non_option_command)
+		die("Got option command '%s' after non-option command", option);
+
+	parse_one_option(option);
+}
+
+static void parse_nongit_option(void)
+{
+  // do nothing
+}
+
 static int git_pack_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "pack.depth")) {
@@ -2485,6 +2517,26 @@ static int git_pack_config(const char *k, const char *v, void *cb)
 static const char fast_import_usage[] =
 "git fast-import [--date-format=f] [--max-pack-size=n] [--depth=n] [--active-branches=n] [--export-marks=marks.file]";
 
+static void parse_argv(void)
+{
+	unsigned int i;
+
+	for (i = 1; i < global_argc; i++) {
+		const char *a = global_argv[i];
+
+		if (*a != '-' || !strcmp(a, "--"))
+			break;
+
+		parse_one_option(a + 2);
+	}
+	if (i != global_argc)
+		usage(fast_import_usage);
+
+	seen_non_option_command = 1;
+	if (input_file)
+		read_marks();
+}
+
 int main(int argc, const char **argv)
 {
 	unsigned int i;
@@ -2503,18 +2555,8 @@ int main(int argc, const char **argv)
 	avail_tree_table = xcalloc(avail_tree_table_sz, sizeof(struct avail_tree_content*));
 	marks = pool_calloc(1, sizeof(struct mark_set));
 
-	for (i = 1; i < argc; i++) {
-		const char *a = argv[i];
-
-		if (*a != '-' || !strcmp(a, "--"))
-			break;
-
-		parse_one_option(a + 2);
-	}
-	if (i != argc)
-		usage(fast_import_usage);
-	if (input_file)
-		read_marks();
+	global_argc = argc;
+	global_argv = argv;
 
 	rc_free = pool_alloc(cmd_save * sizeof(*rc_free));
 	for (i = 0; i < (cmd_save - 1); i++)
@@ -2539,9 +2581,18 @@ int main(int argc, const char **argv)
 			parse_progress();
 		else if (!prefixcmp(command_buf.buf, "feature "))
 			parse_feature();
+		else if (!prefixcmp(command_buf.buf, "option git "))
+			parse_option();
+    else if (!prefixcmp(command_buf.buf, "option "))
+      parse_nongit_option();
 		else
 			die("Unsupported command: %s", command_buf.buf);
 	}
+
+	// argv hasn't been parsed yet, do so
+	if (!seen_non_option_command)
+		parse_argv();
+
 	end_packfile();
 
 	dump_branches();
-- 
1.6.4.16.g72c66.dirty

[PATCH v6 6/6] fast-import: test the new option command

[Sverre Rabbelier]

Test three options (quiet and import/export-marks) and verify that the
commandline options override these.

Also make sure that a option command without a preceeding feature
git-options command is rejected and that non-git options are ignored.

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

  Tests updated to match the new 'option git' syntax. Also added a
  test to ensure that an option command without a preceeding 'feature
  git-options' is rejected and that non-git options are ignored.

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

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 564ed6b..fb795bb 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1089,7 +1089,7 @@ test_expect_success 'P: fail on blob mark in gitlink' '
     test_must_fail git fast-import <input'
 
 ###
-### series R (feature)
+### series R (feature and option)
 ###
 
 cat >input <<EOF
@@ -1108,4 +1108,86 @@ test_expect_success 'R: supported feature is accepted' '
 	git fast-import <input
 '
 
+cat >input << EOF
+feature git-options
+option git quiet
+blob
+data 3
+hi
+
+EOF
+
+touch empty
+
+test_expect_success 'R: quiet option results in no stats being output' '
+    cat input | git fast-import 2> output &&
+    test_cmp empty output
+'
+
+cat >input << EOF
+feature git-options
+option git export-marks=git.marks
+blob
+mark :1
+data 3
+hi
+
+EOF
+
+test_expect_success \
+    'R: export-marks option results in a marks file being created' \
+    'cat input | git fast-import &&
+    grep :1 git.marks'
+
+test_expect_success \
+    'R: export-marks options can be overriden by commandline options' \
+    'cat input | git fast-import --export-marks=other.marks &&
+    grep :1 other.marks'
+
+cat >input << EOF
+feature git-options
+option git import-marks=marks.out
+option git export-marks=marks.new
+EOF
+
+test_expect_success \
+    'R: import to output marks works without any content' \
+    'cat input | git fast-import &&
+    test_cmp marks.out marks.new'
+
+cat >input <<EOF
+feature git-options
+option git import-marks=nonexistant.marks
+option git export-marks=marks.new
+EOF
+
+test_expect_success \
+    'R: import marks uses the commandline marks file when the stream specifies one' \
+    'cat input | git fast-import --import-marks=marks.out &&
+    test_cmp marks.out marks.new'
+
+cat >input <<EOF
+feature git-options
+EOF
+
+test_expect_success 'R: feature option is accepted' '
+	  git fast-import <input
+'
+
+cat >input <<EOF
+option git quiet
+EOF
+
+test_expect_success \
+    'R: option without preceeding feature command is rejected' \
+    'test_must_fail git fast-import <input'
+
+cat >input <<EOF
+option non-existing-vcs non-existing-option
+EOF
+
+test_expect_success 'R: ignore non-git options' '
+    git fast-import <input
+'
+
 test_done
-- 
1.6.4.16.g72c66.dirty

Re: [PATCH v6 5/6] fast-import: add option command

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

>   Main difference with v5 is that the syntax is now 'option git ...'
>   as per a discussion with the other fast-import devs. Other options,
>   e.g. 'option hg' are ignored. Also fixed the docs to say that
>   feature commands are allowed before git option commands.

Perhaps the other people have discussed and thought about this much deeper
than I have after seeing the above description, but what should the
semantics be when you see unknown options?

If "option git something-unknown" is given, it is clear that the tool that
generated the stream assumed that such an option exists in the importer;
it might appear prudent to abort the operation.

But what about "option hg something"?

It is an indication that the stream is meant to be used with the named
option if fed to Hg, but it does not say anything about what should happen
when used with other systems.  If older versions of Hg that do not grok
the given option is expected to abort because they won't be able to change
the behaviour to obey the optional semantics demanded by the "option hg
something", what should the other VCS do?

Worrying about the above would be unnecessary, if you declare that it is
*entirely* optional to understand and obey "option", and ignoring them
does not result in a corrupt import at all.  I think that is the intent
behind "option", as opposed to "feature", and is consistent with the fact
that the command line options can override the in-stream settings.  In
other words, any in-stream instruction that changes the semantics of
stream to render it dangerous to be processed by older version of tools
would be expressed with "feature", not with "option".

If that is the sensible thing to do, then we obviously should ignore
"option hg anything", but at the same time we should ignore "option git
we-do-not-know-what-it-does".

But then, the call to die("Unsupported option: %s", option) at the end of
parse_one_option() is wrong, isn't it?

I think at least the function should be made conditional to die() if it
was called from parse_argv() but simply ignore unknown if it was called
from the input stream.

> diff --git a/fast-import.c b/fast-import.c
> index 9bf06a4..bad93dc 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2456,11 +2468,31 @@ static void parse_feature(void)
>  
>  	if (!prefixcmp(feature, "date-format=")) {
>  		option_date_format(feature + 12);
> +	} else if (!strcmp("git-options", feature)) {
> +		options_enabled = 1;
>  	} else {
>  		die("This version of fast-import does not support feature %s.", feature);
>  	}
>  }
>  
> +static void parse_option(void)
> +{
> +	char* option = command_buf.buf + 11;

ERROR: "foo* bar" should be "foo *bar"

> +
> +	if (!options_enabled)
> +		die("Got option command '%s' before options feature'", option);
> +
> +	if (seen_non_option_command)
> +		die("Got option command '%s' after non-option command", option);
> +
> +	parse_one_option(option);
> +}
> +
> +static void parse_nongit_option(void)
> +{
> +  // do nothing

ERROR: do not use C99 // comments

> @@ -2539,9 +2581,18 @@ int main(int argc, const char **argv)
>  			parse_progress();
>  		else if (!prefixcmp(command_buf.buf, "feature "))
>  			parse_feature();
> +		else if (!prefixcmp(command_buf.buf, "option git "))
> +			parse_option();
> +    else if (!prefixcmp(command_buf.buf, "option "))
> +      parse_nongit_option();
>  		else
>  			die("Unsupported command: %s", command_buf.buf);
>  	}
> +
> +	// argv hasn't been parsed yet, do so

ERROR: do not use C99 // comments

Re: [PATCH v6 5/6] fast-import: add option command

[Sverre Rabbelier]

Heya,

On Thu, Sep 3, 2009 at 04:41, Junio C Hamano<gitster@pobox.com> wrote:
> If "option git something-unknown" is given, it is clear that the tool that
> generated the stream assumed that such an option exists in the importer;
> it might appear prudent to abort the operation.
>
> But what about "option hg something"?

I think we should assume that if we see 'option not-us foo' without a
preceeding 'feature not-us-option', the frontend does not require us
to understand the option (perhaps because they also specify 'option
git foo'.

> If that is the sensible thing to do, then we obviously should ignore
> "option hg anything", but at the same time we should ignore "option git
> we-do-not-know-what-it-does".

Perhaps, frontends could then use 'feature git-quiet-option' if it
wants to make sure it is supported.

> I think at least the function should be made conditional to die() if it
> was called from parse_argv() but simply ignore unknown if it was called
> from the input stream.

Makes sense, what do the fast-import devs think?

>> +static void parse_option(void)
>> +{
>> +     char* option = command_buf.buf + 11;
>
> ERROR: "foo* bar" should be "foo *bar"

Ah, I thought I had fixed all of those, apologies.

> ERROR: do not use C99 // comments
> ERROR: do not use C99 // comments

Will fix in the next version (after we decide on what to do with
unknown git options).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v6 5/6] fast-import: add option command

[Ian Clatworthy]

Sverre Rabbelier wrote:
> Heya,
> 
> On Thu, Sep 3, 2009 at 04:41, Junio C Hamano<gitster@pobox.com> wrote:

>> I think at least the function should be made conditional to die() if it
>> was called from parse_argv() but simply ignore unknown if it was called
>> from the input stream.
> 
> Makes sense, what do the fast-import devs think?

Sounds ok to me.

Ian C.