patch, 4 partsfast-import: add a new option command

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(-)

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(-)

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(-)

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(-)

---

    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(-)

Sverre Rabbelier <srabbelier@gmail.com> wrote:

Missing Signed-off-by.
 

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

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

Sverre Rabbelier <srabbelier@gmail.com> wrote:

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.
 

s/frontends/frontend's/

s/front-ends/frontend's/

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.

Hi,

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

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

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

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

Hi,

On Wed, 12 Aug 2009, Sverre Rabbelier wrote:

Heh, this is a pretty convoluted way to write

	mark_file = xstrdup(marks);

;-)

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

Thanks, Dscho

Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Agreed.

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.

Sverre Rabbelier <srabbelier@gmail.com> wrote:

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?

Heya,

On Thu, Aug 13, 2009 at 08:24, Shawn O. Pearce<spearce@spearce.org> wrote:

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.

Hi,

On Thu, 13 Aug 2009, Sverre Rabbelier wrote:

... and will import the marks twice?

Ciao, Dscho

Heya,

On Thu, Aug 13, 2009 at 10:07, Johannes Schindelin<Johannes.Schindelin@gmx.de> wrote:

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

Sverre Rabbelier <srabbelier@gmail.com> wrote:

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

Heya,

On Thu, Aug 13, 2009 at 10:25, Shawn O. Pearce<spearce@spearce.org> wrote:

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?

Sverre Rabbelier <srabbelier@gmail.com> 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.

Heya,

On Thu, Aug 13, 2009 at 10:41, Shawn O. Pearce<spearce@spearce.org> 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.

Sverre Rabbelier <srabbelier@gmail.com> wrote:

That's better.  :-)

Hi,

On Thu, 13 Aug 2009, Shawn O. Pearce 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...

Ciao, Dscho

Heya,

On Thu, Aug 13, 2009 at 14:51, Johannes Schindelin<Johannes.Schindelin@gmx.de> wrote:

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?

Sverre Rabbelier <srabbelier@gmail.com> wrote:

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.

Heya,

On Thu, Aug 13, 2009 at 15:12, Shawn O. Pearce<spearce@spearce.org> wrote:

Nice anecdote :P.

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

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

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

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?

Heya,

On Thu, Aug 13, 2009 at 12:26, Junio C Hamano<gitster@pobox.com> wrote:

On Thu, Aug 13, 2009 at 10:44, Sverre Rabbelier<srabbelier@gmail.com> wrote:

Which is exactly what the latest version does :).

Sverre Rabbelier <srabbelier@gmail.com> writes:

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

Heya,

On Thu, Aug 13, 2009 at 13:42, Junio C Hamano<gitster@pobox.com> wrote:

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

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:

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?

Sverre Rabbelier <srabbelier@gmail.com> wrote:

Yes.