[PATCH 0/4] fast-import: add a new option command

[PATCH 0/4] fast-import: add a new option command

[Sverre Rabbelier]

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

 Documentation/git-fast-import.txt |   23 ++++++
 fast-import.c                     |  149 +++++++++++++++++++++++++++----------
 t/t9300-fast-import.sh            |   33 ++++++++
 3 files changed, 165 insertions(+), 40 deletions(-)

[PATCH 1/4] 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.
---

    This is nearly identical to the RFC, but with parse_one_option
    also factored out for easy reuse.

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

diff --git a/fast-import.c b/fast-import.c
index 7ef9865..17d57ab 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,93 @@ 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 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")) {
+		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")) {
@@ -2398,7 +2486,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 +2507,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 2/4] fast-import: define a new 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.
---

    As requested, updated the documentation of the language format

 Documentation/git-fast-import.txt |   23 +++++++++++++++++++++++
 1 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index c2f483a..6b5bc1b 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -303,6 +303,11 @@ and control the current import process.  More detailed discussion
 	standard output.  This command is optional and is not needed
 	to perform an import.
 
+`option`::
+    Specify any of the options listed under OPTIONS to change
+    fast-import's behavior to suit the frontends 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
@@ -813,6 +818,24 @@ 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.
 
+`option`
+~~~~~~~~
+Processes the specified option so that git fast-import behaves in a
+way that suits the front-ends needs.
+Note that options specified by the frontend override 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, 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
-- 
1.6.4.16.g72c66.dirty

[PATCH 3/4] 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.
---

    This is a lot simpler from the previous version as we can now
    reuse parse_one_option from 1/4.

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

diff --git a/fast-import.c b/fast-import.c
index 17d57ab..2ec804d 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;
@@ -2460,6 +2465,16 @@ static void parse_one_option(const char *option)
     }
 }
 
+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);
+
+    parse_one_option(option);
+}
+
 static int git_pack_config(const char *k, const char *v, void *cb)
 {
 	if (!strcmp(k, "pack.depth")) {
@@ -2534,6 +2549,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.4.16.g72c66.dirty

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

[Sverre Rabbelier]

---

    Only difference with the previous version is
    s/export-marks /export-marks=/

 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..4152c3a 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.16.g72c66.dirty

Re: [PATCH 3/4] 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.
> ---

Missing Signed-off-by.
 
> @@ -2460,6 +2465,16 @@ static void parse_one_option(const char *option)
>      }
>  }
>  
> +static void parse_option(void)
> +{
> +    char* option = command_buf.buf + 7;

Git style is "char *option", isn't it?

> +
> +	if (seen_non_option_command)
> +		die("Got option command '%s' after non-option command", option);

Indentation is messed up here.  1 tab per level, please.

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new 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.
> ---
> 
>     As requested, updated the documentation of the language format
> 
>  Documentation/git-fast-import.txt |   23 +++++++++++++++++++++++
>  1 files changed, 23 insertions(+), 0 deletions(-)

Why isn't this new 'option' command actually implemented by this
patch?  Please introduce docs and code in the same patch if you can,
especially when it is this simple.
 
> diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
> index c2f483a..6b5bc1b 100644
> --- a/Documentation/git-fast-import.txt
> +++ b/Documentation/git-fast-import.txt
> @@ -303,6 +303,11 @@ and control the current import process.  More detailed discussion
>  	standard output.  This command is optional and is not needed
>  	to perform an import.
>  
> +`option`::
> +    Specify any of the options listed under OPTIONS to change
> +    fast-import's behavior to suit the frontends needs. This command
> +    is optional and is not needed to perform an import.

s/frontends/frontend's/

> +`option`
> +~~~~~~~~
> +Processes the specified option so that git fast-import behaves in a
> +way that suits the front-ends needs.

s/front-ends/frontend's/

> +Note that options specified by the frontend override any options the
> +user may specify to git fast-import itself.

Hmmph.  Do we really want that?  I would think the command line
options should override the stream, such that we can then do
something like:

  hg fast-export >foo
  git fast-import --export-marks=mymarks <foo

even though 'option export-marks=bar' appears in foo.

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new option command

[Johannes Schindelin]

Hi,

On Thu, 13 Aug 2009, Shawn O. Pearce wrote:

> Sverre Rabbelier <srabbelier@gmail.com> wrote:
>
> > +Note that options specified by the frontend override any options the
> > +user may specify to git fast-import itself.
> 
> Hmmph.  Do we really want that?  I would think the command line options 
> should override the stream, such that we can then do something like:
> 
>   hg fast-export >foo
>   git fast-import --export-marks=mymarks <foo
> 
> even though 'option export-marks=bar' appears in foo.

I guess the reason is that this is harder to implement.  The problem is 
that you _have_ to parse the command line options first.  So you need to 
record with every option you set that it has been set by the command line, 
and must not be overridden by the in-stream options.

OTOH,

	hg fast-export |
	sed '1,/^blob/s/^option export-marks=.*$//' > foo

is relatively easy.

Ciao,
Dscho

Re: [PATCH 2/4] fast-import: define a new option command

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Thu, 13 Aug 2009, Shawn O. Pearce wrote:
> > Sverre Rabbelier <srabbelier@gmail.com> wrote:
> >
> > > +Note that options specified by the frontend override any options the
> > > +user may specify to git fast-import itself.
> > 
> > Hmmph.  Do we really want that?  I would think the command line options 
> > should override the stream, such that we can then do something like:
> > 
> >   hg fast-export >foo
> >   git fast-import --export-marks=mymarks <foo
> > 
> > even though 'option export-marks=bar' appears in foo.
> 
> I guess the reason is that this is harder to implement.  The problem is 
> that you _have_ to parse the command line options first.  So you need to 
> record with every option you set that it has been set by the command line, 
> and must not be overridden by the in-stream options.

It might be easy enough to just save the argv into a global, and
reparse the argv immediately before the first non-option command.

> OTOH,
> 
> 	hg fast-export |
> 	sed '1,/^blob/s/^option export-marks=.*$//' > foo
> 
> is relatively easy.

Just for the archives, but that sed expression won't catch inline
data under a file command.  Its legal for a stream to not use the
blob command at all.  I guess it would need to be:

 	hg fast-export |
 	sed '1,/^(blob|commit)/s/^option export-marks=.*$//' > foo

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new option command

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> Actually I was thinking that the frontend should allow the user to override
> these options. Assuming it is, say, specifying a marks file, if the user
> wishes to change that, the frontend would probably need to know about the
> new location too.
> 
> So not only is it easier to implement, it makes sense from an ui
> perspective. No?

My point is, most of the options are about paths to local files.

If I export something from hg on machine A, then copy the
fast-import stream to machine B, the marks file on B might not be
in the same path.  I might not even control machine A, maybe you
emailed me that stream file.  I may not be able to use an option
on the source tool to alter the path.

Why shouldn't I be able to (easily) override the marks location
locally, without resorting to stream editing?

Same with --force.  Which probably now demands a counter option,
--no-force.  If you send me a stream with "option force\n" in the
header, why can't I try to import it with "--no-force" first?

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 08:24, Shawn O. Pearce<spearce@spearce.org> wrote:
> Why shouldn't I be able to (easily) override the marks location
> locally, without resorting to stream editing?

You convinced me, I'll store argv and argc in global_argv/global_argc
and move the option parsing to parse_argv(), which is then called when
the first non_option command is parsed.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Johannes Schindelin]

Hi,

On Thu, 13 Aug 2009, Sverre Rabbelier wrote:

> On Thu, Aug 13, 2009 at 08:24, Shawn O. Pearce<spearce@spearce.org> 
> wrote:
> > Why shouldn't I be able to (easily) override the marks location 
> > locally, without resorting to stream editing?
> 
> You convinced me, I'll store argv and argc in global_argv/global_argc
> and move the option parsing to parse_argv(), which is then called when
> the first non_option command is parsed.

... and will import the marks twice?

Ciao,
Dscho

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 10:07, Johannes
Schindelin<Johannes.Schindelin@gmx.de> wrote:
> ... and will import the marks twice?

Ah, you're right :(. What's the best way to do this? Should we dump
any previous marks when importing new ones?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 10:07, Johannes
> Schindelin<Johannes.Schindelin@gmx.de> wrote:
> > ... and will import the marks twice?
> 
> Ah, you're right :(. What's the best way to do this? Should we dump
> any previous marks when importing new ones?

Uh, well, yes.  We shouldn't define :5 if it was in the file that
appeared in the stream, but isn't in the file on the command line.

Worse, what happens if we do this:

  echo "option import-marks=/not/found" \
  | git fast-import --import-marks=my.marks

I want this to work, even though /not/found does not exist, but
my.marks does.  So that does complicate things...

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 10:25, Shawn O. Pearce<spearce@spearce.org> wrote:
> I want this to work, even though /not/found does not exist, but
> my.marks does.  So that does complicate things...

Should we pass an option to parse_marks to make it ignore a
non-existing file, and set that option when parsing the stream
commands?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 10:25, Shawn O. Pearce<spearce@spearce.org> wrote:
> > I want this to work, even though /not/found does not exist, but
> > my.marks does. ?So that does complicate things...
> 
> Should we pass an option to parse_marks to make it ignore a
> non-existing file, and set that option when parsing the stream
> commands?

Uh, no, if we have "option import-marks=..." and we can't find the
file "..." and we have no --import-marks command line flag that
would have overridden it, we need to abort with an error.

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 10:41, Shawn O. Pearce<spearce@spearce.org> wrote:
> Uh, no, if we have "option import-marks=..." and we can't find the
> file "..." and we have no --import-marks command line flag that
> would have overridden it, we need to abort with an error.

Ah, then how about in option_import_marks() we only store the name of
the file, like in option_export_marks, and at the end, when we reach
the first non-option command (and we've parsed argv), we read the
file. That way it's only read once, and it deals with the above
scenario.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 10:41, Shawn O. Pearce<spearce@spearce.org> wrote:
> > Uh, no, if we have "option import-marks=..." and we can't find the
> > file "..." and we have no --import-marks command line flag that
> > would have overridden it, we need to abort with an error.
> 
> Ah, then how about in option_import_marks() we only store the name of
> the file, like in option_export_marks, and at the end, when we reach
> the first non-option command (and we've parsed argv), we read the
> file. That way it's only read once, and it deals with the above
> scenario.

That's better.  :-)

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new option command

[Johannes Schindelin]

Hi,

On Thu, 13 Aug 2009, Shawn O. Pearce wrote:

> Sverre Rabbelier <srabbelier@gmail.com> wrote:
> > On Thu, Aug 13, 2009 at 10:41, Shawn O. Pearce<spearce@spearce.org> wrote:
> > > Uh, no, if we have "option import-marks=..." and we can't find the
> > > file "..." and we have no --import-marks command line flag that
> > > would have overridden it, we need to abort with an error.
> > 
> > Ah, then how about in option_import_marks() we only store the name of
> > the file, like in option_export_marks, and at the end, when we reach
> > the first non-option command (and we've parsed argv), we read the
> > file. That way it's only read once, and it deals with the above
> > scenario.
> 
> That's better.  :-)

Sorry if I spoil the party, but maybe if things get so complicated, it may 
be a sign that the original version (stream overrides command line, since 
it knows better) is to be preferred?  After all, if hg fast-export says 
that the marks should be imported from a certain file, it may be for a 
_very good_ reason...

Ciao,
Dscho

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 14:51, Johannes
Schindelin<Johannes.Schindelin@gmx.de> wrote:
> Sorry if I spoil the party, but maybe if things get so complicated, it may
> be a sign that the original version (stream overrides command line, since
> it knows better) is to be preferred?  After all, if hg fast-export says
> that the marks should be imported from a certain file, it may be for a
> _very good_ reason...

Yes, and that should Just Work (which it does). Also, I'm not sure how
often one would output a stream on one computer, then move it to
another and import it there, but I'll methinks Shawn brought it up for
a reason ;). However, I do think it's better design to only store the
name of the import file and then do the actual import later on (to
prevent double imports).

I don't have a preference either way (both patches are already written
after all). Shawn?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Thu, Aug 13, 2009 at 14:51, Johannes
> Schindelin<Johannes.Schindelin@gmx.de> wrote:
> > Sorry if I spoil the party, but maybe if things get so complicated, it may
> > be a sign that the original version (stream overrides command line, since
> > it knows better) is to be preferred? ?After all, if hg fast-export says
> > that the marks should be imported from a certain file, it may be for a
> > _very good_ reason...
> 
> Yes, and that should Just Work (which it does). Also, I'm not sure how
> often one would output a stream on one computer, then move it to
> another and import it there, but I'll methinks Shawn brought it up for
> a reason ;). However, I do think it's better design to only store the
> name of the import file and then do the actual import later on (to
> prevent double imports).
> 
> I don't have a preference either way (both patches are already written
> after all). Shawn?

No, I don't have a really good reason for the command line overrides
the file other than this simple rule:

  If the file is likely to be several hundred MiB, or bigger; thou
  shall never try to open it with vi, *especially* vi on a Solaris
  system, as at least one line is likely to be too long.

  If the file header contains paths to other files, it is likely
  one will want to modify that header sometime, because you moved
  the file between systems.

  Given the size of the file above, you can't just fix it with vi.

  Lacking a tool that *can* do this edit safely (and Dscho's simple
  sed wasn't enough, as I already pointed out, oh and Solaris sed
  also fails on long lines), we *should* be able to override this on
  the command line, *especially* since we already have the command
  line option standardized!

What is this, gang up on Shawn's words-of-wisdom week?  Both this
thread and my intern this week have been argueing with me about
what seem to me to be fairly trivial things.  Maybe I just need to
take vacation.  Good thing I have one coming in 5 weeks.

I say use the version where we store the values (e.g. file names)
during option parsing, and then actually apply those saved values
just before the first non-option command.  Which I think only has
an impact on the import-marks option, the rest are all just simple
variable updates whose values aren't read until after the first
non-option command anyway.

-- 
Shawn.

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 15:12, Shawn O. Pearce<spearce@spearce.org> wrote:
> No, I don't have a really good reason for the command line overrides
> the file other than this simple rule:

Nice anecdote :P.

> What is this, gang up on Shawn's words-of-wisdom week?  Both this
> thread and my intern this week have been argueing with me about
> what seem to me to be fairly trivial things.  Maybe I just need to
> take vacation.  Good thing I have one coming in 5 weeks.

Didn't you get the memo, you could be crashing an airplane [0] like that ;).

> I say use the version where we store the values (e.g. file names)
> during option parsing, and then actually apply those saved values
> just before the first non-option command.  Which I think only has
> an impact on the import-marks option, the rest are all just simple
> variable updates whose values aren't read until after the first
> non-option command anyway.

Ok, that's v3, speaking of which, does it look good enough for inclusion?

[0] http://www.thehackerchickblog.com/2009/05/plane-crashes-software-failures-and.html

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Junio C Hamano]

"Shawn O. Pearce" <spearce@spearce.org> writes:

> Sverre Rabbelier <srabbelier@gmail.com> wrote:
>> On Thu, Aug 13, 2009 at 10:07, Johannes
>> Schindelin<Johannes.Schindelin@gmx.de> wrote:
>> > ... and will import the marks twice?
>> 
>> Ah, you're right :(. What's the best way to do this? Should we dump
>> any previous marks when importing new ones?
>
> Uh, well, yes.  We shouldn't define :5 if it was in the file that
> appeared in the stream, but isn't in the file on the command line.
>
> Worse, what happens if we do this:
>
>   echo "option import-marks=/not/found" \
>   | git fast-import --import-marks=my.marks
>
> I want this to work, even though /not/found does not exist, but
> my.marks does.  So that does complicate things...

How about making the option parser get and keep the _name_ of the file
until option parsing session (i.e. read the stream until initial run of
"option" command runs out and then parse the command line to override),
and then finally open the file and read it?

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 12:26, Junio C Hamano<gitster@pobox.com> wrote:
> How about making the option parser get and keep the _name_ of the file
> until option parsing session (i.e. read the stream until initial run of
> "option" command runs out and then parse the command line to override),
> and then finally open the file and read it?

On Thu, Aug 13, 2009 at 10:44, Sverre Rabbelier<srabbelier@gmail.com> wrote:
> Ah, then how about in option_import_marks() we only store the name of
> the file, like in option_export_marks, and at the end, when we reach
> the first non-option command (and we've parsed argv), we read the
> file. That way it's only read once, and it deals with the above
> scenario.

Which is exactly what the latest version does :).

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> Heya,
>
> On Thu, Aug 13, 2009 at 12:26, Junio C Hamano<gitster@pobox.com> wrote:
>> How about making the option parser get and keep the _name_ of the file
>> until option parsing session (i.e. read the stream until initial run of
>> "option" command runs out and then parse the command line to override),
>> and then finally open the file and read it?
>
> On Thu, Aug 13, 2009 at 10:44, Sverre Rabbelier<srabbelier@gmail.com> wrote:
>> Ah, then how about in option_import_marks() we only store the name of
>> the file, like in option_export_marks, and at the end, when we reach
>> the first non-option command (and we've parsed argv), we read the
>> file. That way it's only read once, and it deals with the above
>> scenario.
>
> Which is exactly what the latest version does :).

Heh, thanks; it appears I lagged behind by about 2 hours?

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

On Thu, Aug 13, 2009 at 13:42, Junio C Hamano<gitster@pobox.com> wrote:
> Heh, thanks; it appears I lagged behind by about 2 hours?

So it would seem, not a bad latency at all though.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Sverre Rabbelier]

Heya,

[picking up a topic on what to do if there's a import-marks from the
stream and the commandline]

On Thu, Aug 13, 2009 at 18:25, Shawn O. Pearce <spearce@spearce.org> wrote:
> Sverre Rabbelier <srabbelier@gmail.com> wrote:
>> On Thu, Aug 13, 2009 at 10:07, Johannes
>> Schindelin<Johannes.Schindelin@gmx.de> wrote:
>> > ... and will import the marks twice?
>>
>> Ah, you're right :(. What's the best way to do this? Should we dump
>> any previous marks when importing new ones?
>
> Uh, well, yes.  We shouldn't define :5 if it was in the file that
> appeared in the stream, but isn't in the file on the command line.
>
> Worse, what happens if we do this:
>
>  echo "option import-marks=/not/found" \
>  | git fast-import --import-marks=my.marks
>
> I want this to work, even though /not/found does not exist, but
> my.marks does.  So that does complicate things...

According to the docs it is allowed to specify import-marks multiple
times (the latest series does not honor that, and there's apparently
no test to verify it). What should the behavior be wrt
stream/commandline arguments? Load from stream if present and valid,
and also load from commandline? In that case, how do you override
what's in the stream?

I think the simplest is to allow the stream to specify a marks file
exactly once, and commandline arguments override what's in the stream.
Listing import-marks on the commandline is still valid and keeps the
old behavior.

Sensible?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 2/4] fast-import: define a new option command

[Shawn O. Pearce]

Sverre Rabbelier <srabbelier@gmail.com> wrote:
> I think the simplest is to allow the stream to specify a marks file
> exactly once, and commandline arguments override what's in the stream.
> Listing import-marks on the commandline is still valid and keeps the
> old behavior.
> 
> Sensible?

Yes.

-- 
Shawn.

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

[Johannes Schindelin]

Hi,

On Wed, 12 Aug 2009, Sverre Rabbelier wrote:

> +static void option_export_marks(const char *marks)
> +{
> +	struct strbuf buf = STRBUF_INIT;
> +	strbuf_addstr(&buf, marks);
> +	mark_file = strbuf_detach(&buf, NULL);
> +}

Heh, this is a pretty convoluted way to write

	mark_file = xstrdup(marks);

;-)

> +static void option_force()
> +{
> +	force_update = 1;
> +}

I'm not sure that I would put these simple assignments in separate 
functions, but that's certainly up to you!

Thanks,
Dscho

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

[Shawn O. Pearce]

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> On Wed, 12 Aug 2009, Sverre Rabbelier wrote:
> 
> > +static void option_export_marks(const char *marks)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	strbuf_addstr(&buf, marks);
> > +	mark_file = strbuf_detach(&buf, NULL);
> > +}
> 
> Heh, this is a pretty convoluted way to write
> 
> 	mark_file = xstrdup(marks);
> 
> ;-)

Agreed.

> > +static void option_force()
> > +{
> > +	force_update = 1;
> > +}
> 
> I'm not sure that I would put these simple assignments in separate 
> functions, but that's certainly up to you!

Oh, good point.  Yes, please don't do that, please just write these
directly into the option parser.

Aside from these remarks, this patch looks good to me.

-- 
Shawn.