[PATCH 1/1] fast-import: show a warning for non-existent files.

[PATCH 1/1] fast-import: show a warning for non-existent files.

[Felipe Contreras]

This is useful in certain SCMs like monotone, where each 'merge revision' has
the changes of all the micro-branches merged. So it appears as duplicated commands.

The delete command was ignoring the issue completely. The rename/copy commands
where throwing a fatal exception.
---
 fast-import.c |   14 ++++++++++++--
 1 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 7089e6f..3dd2ab6 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1945,6 +1945,7 @@ static void file_change_d(struct branch *b)
 	const char *p = command_buf.buf + 2;
 	static struct strbuf uq = STRBUF_INIT;
 	const char *endp;
+	struct tree_entry leaf;
 
 	strbuf_reset(&uq);
 	if (!unquote_c_style(&uq, p, &endp)) {
@@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	tree_content_remove(&b->branch_tree, p, NULL);
+	memset(&leaf, 0, sizeof(leaf));
+	tree_content_remove(&b->branch_tree, p, &leaf);
+	if (!leaf.versions[1].mode)
+	{
+		warning("Path %s not in branch", p);
+		return;
+	}
 }
 
 static void file_change_cr(struct branch *b, int rename)
@@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)
 	else
 		tree_content_get(&b->branch_tree, s, &leaf);
 	if (!leaf.versions[1].mode)
-		die("Path %s not in branch", s);
+	{
+		warning("Path %s not in branch", s);
+		return;
+	}
 	tree_content_set(&b->branch_tree, d,
 		leaf.versions[1].sha1,
 		leaf.versions[1].mode,
-- 
1.6.0.1

Re: [PATCH 1/1] fast-import: show a warning for non-existent files.

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> This is useful in certain SCMs like monotone, where each 'merge revision' has
> the changes of all the micro-branches merged. So it appears as duplicated commands.

The patch appears to add warning to when you try to 'D'elete something
that should not exist in the revision, whose moral equivalents are
implemented in the codepath to deal with 'R'enaming and 'C'opying an
non-existent path.

But instead of making it die(), it merely warns, and even worse, you are
demoting an existing die() in Rename/Copy codepath to mere warning
unconditionally.  Why?

"This" that begins your proposed commit log message needs to be clarified,
but I am guessing that you are defending your change to demote existing
error check to die on inconsistent input to a mere warning.  I do not find
it a particularly good defending argument.  It sounds more like you are
papering over bugs in _one_ broken converter that produces and feeds an
incorrect input to fast-import, breaking a safety valve for everybody else.

> The delete command was ignoring the issue completely. The rename/copy commands
> where throwing a fatal exception.

Yes.  I think this has to be two patch series, that:

 (1) Adds the equivalent "you cannot delete what you do not have" die() in
     the delete codepath; and

 (2) Adds an option that demotes *all* the "don't touch (rename, modify,
     copy or delete) what you do not have" die()s to warning().

provided if we can give a good rationale to the latter.  Otherwise we
should just do (1) and forget about (2).

Re: [PATCH 1/1] fast-import: show a warning for non-existent files.

[Shawn O. Pearce]

Felipe Contreras <felipe.contreras@gmail.com> wrote:
> This is useful in certain SCMs like monotone, where each 'merge revision' has
> the changes of all the micro-branches merged. So it appears as duplicated commands.
> 
> The delete command was ignoring the issue completely. The rename/copy commands
> where throwing a fatal exception.

Signed-off-by line?  See Documentation/SubmittingPatches.

> diff --git a/fast-import.c b/fast-import.c
> index 7089e6f..3dd2ab6 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)
>  			die("Garbage after path in: %s", command_buf.buf);
>  		p = uq.buf;
>  	}
> -	tree_content_remove(&b->branch_tree, p, NULL);
> +	memset(&leaf, 0, sizeof(leaf));
> +	tree_content_remove(&b->branch_tree, p, &leaf);
> +	if (!leaf.versions[1].mode)
> +	{
> +		warning("Path %s not in branch", p);
> +		return;
> +	}
>  }
>  
>  static void file_change_cr(struct branch *b, int rename)

This is going to leak memory unless you add this before the
if (..mode) condition:

	if (leaf->tree)
		release_tree_content_recursive(e->tree);

We didn't worry about deleting a path that doesn't exist because
the importer clearly wants it gone.  If it wants it gone and it is
already gone then it should be fine to ignore the delete command.

But as I point out below some import front-ends should be accurate
enough that they should not send a 'D' command unless the path is
already in the tree.  Thus this can be an error condition for some
types of frontends, but can be ignored for others.

> @@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)
>  	else
>  		tree_content_get(&b->branch_tree, s, &leaf);
>  	if (!leaf.versions[1].mode)
> -		die("Path %s not in branch", s);
> +	{
> +		warning("Path %s not in branch", s);
> +		return;
> +	}
>  	tree_content_set(&b->branch_tree, d,
>  		leaf.versions[1].sha1,
>  		leaf.versions[1].mode,

Normally we consider invalid paths to be an error.  I wonder if this
should still be an error, unless the front-end passes an option on
the command line.  Then monotone based importers can make these
warnings, but other importers that don't have this problem can
still treat them what they are, which is a fatal error.

Did you run the test suite (t/t9300-fast-import.sh) after your patch?
I would have thought a few of the bad path errors should be caught
there.

-- 
Shawn.

Re: [PATCH 1/1] fast-import: show a warning for non-existent files.

[Felipe Contreras]

On Mon, Sep 1, 2008 at 10:04 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> This is useful in certain SCMs like monotone, where each 'merge revision' has
>> the changes of all the micro-branches merged. So it appears as duplicated commands.
>
> The patch appears to add warning to when you try to 'D'elete something
> that should not exist in the revision, whose moral equivalents are
> implemented in the codepath to deal with 'R'enaming and 'C'opying an
> non-existent path.
>
> But instead of making it die(), it merely warns, and even worse, you are
> demoting an existing die() in Rename/Copy codepath to mere warning
> unconditionally.  Why?

Right, I tried to do two things at once. I thought adding the code in
'D'elete in a separate branch was too much separation for such a
trivial thing.

> "This" that begins your proposed commit log message needs to be clarified,
> but I am guessing that you are defending your change to demote existing
> error check to die on inconsistent input to a mere warning.  I do not find
> it a particularly good defending argument.  It sounds more like you are
> papering over bugs in _one_ broken converter that produces and feeds an
> incorrect input to fast-import, breaking a safety valve for everybody else.

In monotone you can have something like:

 A
/ \
B D
| |
C E
\ /
 F

If you do a 'delete foo' in B, and a 'delete bar' in F, you will get
this stored in the merge revision (F):

old_revision [C]
delete "foo"
delete "bar"

old_revision [E]
delete "bar"

All the changes of the merged branches are stored again in the merge revision.

I guess ideally my converter should check which changes appear on both
branches, and only apply those. Of course, only on revisions that are
merges.
That complicates the code quite a lot. I tried it and it didn't work
quite well (I did something wrong).

So I tried to workaround this in fast-import, and it's simple, and it works.

<snip/>

-- 
Felipe Contreras

Re: [PATCH 1/1] fast-import: show a warning for non-existent files.

[Felipe Contreras]

On Mon, Sep 1, 2008 at 10:25 PM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> This is useful in certain SCMs like monotone, where each 'merge revision' has
>> the changes of all the micro-branches merged. So it appears as duplicated commands.
>>
>> The delete command was ignoring the issue completely. The rename/copy commands
>> where throwing a fatal exception.
>
> Signed-off-by line?  See Documentation/SubmittingPatches.

All right, read.

>> diff --git a/fast-import.c b/fast-import.c
>> index 7089e6f..3dd2ab6 100644
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -1952,7 +1953,13 @@ static void file_change_d(struct branch *b)
>>                       die("Garbage after path in: %s", command_buf.buf);
>>               p = uq.buf;
>>       }
>> -     tree_content_remove(&b->branch_tree, p, NULL);
>> +     memset(&leaf, 0, sizeof(leaf));
>> +     tree_content_remove(&b->branch_tree, p, &leaf);
>> +     if (!leaf.versions[1].mode)
>> +     {
>> +             warning("Path %s not in branch", p);
>> +             return;
>> +     }
>>  }
>>
>>  static void file_change_cr(struct branch *b, int rename)
>
> This is going to leak memory unless you add this before the
> if (..mode) condition:
>
>        if (leaf->tree)
>                release_tree_content_recursive(e->tree);

Hmm, ok.

> We didn't worry about deleting a path that doesn't exist because
> the importer clearly wants it gone.  If it wants it gone and it is
> already gone then it should be fine to ignore the delete command.
>
> But as I point out below some import front-ends should be accurate
> enough that they should not send a 'D' command unless the path is
> already in the tree.  Thus this can be an error condition for some
> types of frontends, but can be ignored for others.

I'm sending the patch again with this behavior as an option.

>> @@ -1994,7 +2001,10 @@ static void file_change_cr(struct branch *b, int rename)
>>       else
>>               tree_content_get(&b->branch_tree, s, &leaf);
>>       if (!leaf.versions[1].mode)
>> -             die("Path %s not in branch", s);
>> +     {
>> +             warning("Path %s not in branch", s);
>> +             return;
>> +     }
>>       tree_content_set(&b->branch_tree, d,
>>               leaf.versions[1].sha1,
>>               leaf.versions[1].mode,
>
> Normally we consider invalid paths to be an error.  I wonder if this
> should still be an error, unless the front-end passes an option on
> the command line.  Then monotone based importers can make these
> warnings, but other importers that don't have this problem can
> still treat them what they are, which is a fatal error.
>
> Did you run the test suite (t/t9300-fast-import.sh) after your patch?
> I would have thought a few of the bad path errors should be caught
> there.

I didn't initially, now I just did and it doesn't seem to be checking
for such things.

Best regards.

-- 
Felipe Contreras

[PATCH] fast-import: add ignore non-existent files option.

[Felipe Contreras]

This is useful for SCMs that don't have proper changesets in each
revision (monotone).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 Documentation/git-fast-import.txt |    4 ++++
 fast-import.c                     |   14 ++++++++++++--
 2 files changed, 16 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-fast-import.txt b/Documentation/git-fast-import.txt
index c2f483a..42e10c1 100644
--- a/Documentation/git-fast-import.txt
+++ b/Documentation/git-fast-import.txt
@@ -95,6 +95,10 @@ OPTIONS
 	memory used by fast-import during this run.  Showing this output
 	is currently the default, but can be disabled with \--quiet.
 
+--tolerant::
+	Avoid fatal exceptions when actions are executed in non-existent
+	files.  For example removing a file that is not there.
+
 
 Performance
 -----------
diff --git a/fast-import.c b/fast-import.c
index 7089e6f..01be3fa 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -345,6 +345,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 tolerant;
 
 static void write_branch_report(FILE *rpt, struct branch *b)
 {
@@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)
 		tree_content_remove(&b->branch_tree, s, &leaf);
 	else
 		tree_content_get(&b->branch_tree, s, &leaf);
-	if (!leaf.versions[1].mode)
-		die("Path %s not in branch", s);
+	if (!leaf.versions[1].mode) {
+		if (tolerant) {
+			if (leaf.tree)
+				release_tree_content_recursive(leaf.tree);
+			warning("Path %s not in branch", s);
+			return;
+		} else
+			die("Path %s not in branch", s);
+	}
 	tree_content_set(&b->branch_tree, d,
 		leaf.versions[1].sha1,
 		leaf.versions[1].mode,
@@ -2447,6 +2455,8 @@ int main(int argc, const char **argv)
 			show_stats = 0;
 		else if (!strcmp(a, "--stats"))
 			show_stats = 1;
+		else if (!strcmp(a, "--tolerant"))
+			tolerant = 1;
 		else
 			die("unknown option %s", a);
 	}
-- 
1.6.0.1

Re: [PATCH] fast-import: add ignore non-existent files option.

[Shawn O. Pearce]

Felipe Contreras <felipe.contreras@gmail.com> wrote:
> This is useful for SCMs that don't have proper changesets in each
> revision (monotone).
 
> +--tolerant::
> +	Avoid fatal exceptions when actions are executed in non-existent
> +	files.  For example removing a file that is not there.
> +

Yea.  But I'm not sure --tolerant is the best name.  --ignore-errors
or --treat-errors-as-warnings sounds better to me.
  
> diff --git a/fast-import.c b/fast-import.c
> index 7089e6f..01be3fa 100644
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -345,6 +345,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 tolerant;
>  
>  static void write_branch_report(FILE *rpt, struct branch *b)
>  {
> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)
>  		tree_content_remove(&b->branch_tree, s, &leaf);
>  	else
>  		tree_content_get(&b->branch_tree, s, &leaf);
> -	if (!leaf.versions[1].mode)
> -		die("Path %s not in branch", s);
> +	if (!leaf.versions[1].mode) {
> +		if (tolerant) {
> +			if (leaf.tree)
> +				release_tree_content_recursive(leaf.tree);
> +			warning("Path %s not in branch", s);
> +			return;
> +		} else
> +			die("Path %s not in branch", s);
> +	}
>  	tree_content_set(&b->branch_tree, d,
>  		leaf.versions[1].sha1,
>  		leaf.versions[1].mode,
> @@ -2447,6 +2455,8 @@ int main(int argc, const char **argv)
>  			show_stats = 0;
>  		else if (!strcmp(a, "--stats"))
>  			show_stats = 1;
> +		else if (!strcmp(a, "--tolerant"))
> +			tolerant = 1;
>  		else
>  			die("unknown option %s", a);
>  	}

-- 
Shawn.

Re: [PATCH] fast-import: add ignore non-existent files option.

[Felipe Contreras]

On Tue, Sep 2, 2008 at 1:38 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> wrote:
>> This is useful for SCMs that don't have proper changesets in each
>> revision (monotone).
>
>> +--tolerant::
>> +     Avoid fatal exceptions when actions are executed in non-existent
>> +     files.  For example removing a file that is not there.
>> +
>
> Yea.  But I'm not sure --tolerant is the best name.  --ignore-errors
> or --treat-errors-as-warnings sounds better to me.

I initially named it --ignore-non-existent, but I thought the option
was too specific.

--ignore-errors or --treat-errors-as-warnings imply all errors. It
might make sense to always fail at certain errors, like 'mark not
found'. I thought 'relaxed' or 'tolerant' would imply that only some
errors will be allowed, not all.

>> diff --git a/fast-import.c b/fast-import.c
>> index 7089e6f..01be3fa 100644
>> --- a/fast-import.c
>> +++ b/fast-import.c
>> @@ -345,6 +345,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 tolerant;
>>
>>  static void write_branch_report(FILE *rpt, struct branch *b)
>>  {
>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)
>>               tree_content_remove(&b->branch_tree, s, &leaf);
>>       else
>>               tree_content_get(&b->branch_tree, s, &leaf);
>> -     if (!leaf.versions[1].mode)
>> -             die("Path %s not in branch", s);
>> +     if (!leaf.versions[1].mode) {
>> +             if (tolerant) {
>> +                     if (leaf.tree)
>> +                             release_tree_content_recursive(leaf.tree);
>> +                     warning("Path %s not in branch", s);
>> +                     return;
>> +             } else
>> +                     die("Path %s not in branch", s);
>> +     }
>>       tree_content_set(&b->branch_tree, d,
>>               leaf.versions[1].sha1,
>>               leaf.versions[1].mode,
>> @@ -2447,6 +2455,8 @@ int main(int argc, const char **argv)
>>                       show_stats = 0;
>>               else if (!strcmp(a, "--stats"))
>>                       show_stats = 1;
>> +             else if (!strcmp(a, "--tolerant"))
>> +                     tolerant = 1;
>>               else
>>                       die("unknown option %s", a);
>>       }
>
> --
> Shawn.
>



-- 
Felipe Contreras

Re: [PATCH] fast-import: add ignore non-existent files option.

[Junio C Hamano]

Felipe Contreras <felipe.contreras@gmail.com> writes:

> This is useful for SCMs that don't have proper changesets in each
> revision (monotone).

I am still not convinced this is a proper workaround for the issue.  Why
shouldn't the feeder of fast-import be able to do this?

> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)
> ...

Also what happened to the missing warning() for 'D'elete codepath?

Re: [PATCH] fast-import: add ignore non-existent files option.

[Felipe Contreras]

On Tue, Sep 2, 2008 at 2:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> This is useful for SCMs that don't have proper changesets in each
>> revision (monotone).
>
> I am still not convinced this is a proper workaround for the issue.  Why
> shouldn't the feeder of fast-import be able to do this?

If I could get a list of the files that changed on each revision from
monotone I would, but that's not possible (I've asked in their mailing
list). Apparently there's a way to feed the right data, but it's
complicated.

Just to give you a sense. Right now I'm simply parsing all the changes
in the revision and converting them to fast-import's format. I don't
need to check if the revision has multiple parents, or not.

Now, in order to feed the right data I would have to find the
old_revision markers, use a hash table to store the file changes per
revision, after the parsing find out if there's more than one parent.
If there's more than one parent then find out the duplicate actions
that are in all the parents and store them in a new list of actions.
If there's only one parent then simply use the list of actions of that
parent.

I can spend some time trying to get that working, but a) this
workaround is much simpler, and b) I'm thinking on writing the final
converter in C. I really don't want to mess with all this complexity
unless I absolutely must.

If you are interested in the current code: http://pastie.org/264209.

>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)
>> ...
>
> Also what happened to the missing warning() for 'D'elete codepath?

I'm not interested in it.

I added it because I thought it was missing. Since apparently it's
more important to don't alter the old behaviour I prefer to scratch my
own itch and focus on what I need.

-- 
Felipe Contreras

Re: [PATCH] fast-import: add ignore non-existent files option.

[Junio C Hamano]

"Felipe Contreras" <felipe.contreras@gmail.com> writes:

> On Tue, Sep 2, 2008 at 2:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>
>>> This is useful for SCMs that don't have proper changesets in each
>>> revision (monotone).
>>
>> I am still not convinced this is a proper workaround for the issue.  Why
>> shouldn't the feeder of fast-import be able to do this?
>
> If I could get a list of the files that changed on each revision from
> monotone I would, but that's not possible (I've asked in their mailing
> list). Apparently there's a way to feed the right data, but it's
> complicated.

Ok, I did not realize that you are not keeping track of what the parents'
trees look like when processing a merge commit.

But it raises a bigger question.  You earlier said:

    In monotone you can have something like:

     A
    / \
    B D
    | |
    C E
    \ /
     F

    If you do a 'delete foo' in B, and a 'delete bar' in F, you will get
    this stored in the merge revision (F):

    old_revision [C]
    delete "foo"
    delete "bar"

    old_revision [E]
    delete "bar"

    All the changes of the merged branches are stored again in the merge revision.

Now, does it talk about C and E because they are immediate parents of B
and D, or because they are immediate child of the common ancestor F?  I am
guessing it is the latter (what I mean is if it still talks aobut C and E
if you had more intermediate commits between B and C).  What the merge at
A records is not relative to B/D but relative to immediate child of the
fork point.

If that is the case, ignoring a delete that deletes already deleted path
is fine; I think it is the least of your problem.  The above description
makes me wonder what they say about modification.  Do they talk about the
same modification that happened between B and C when talking about A?  If
that is the case, it would be a far larger problem.  You cannot just say
"ignore any modification recorded in a merge, because they have been
already done on the branches being merged (i.e. up to B and up to D)", as
A may have to be an evil merge.

I have to wonder if you can "mark" the tree object of C, and when you
process merge A, represent A's tree by starting from that marked tree,
applying only the description to bring the state of "old_revision [C]"
to that of A (delete "foo" and delete "bar" in your illustration), and
recording that tree with parents B and D.

>>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)
>>> ...
>>
>> Also what happened to the missing warning() for 'D'elete codepath?
>
> I'm not interested in it.

If I were asking for an unrelated "feature" when you are developing some
other feature, it would have been different, but I do not think that is a
good answer in this particular case.

Your --tolerant (or --ignore-errors) is about customizing strictness of
the error handling, and you know of a case where the error handling is not
strict enough in the existing code.  In other words, your "not interested"
is _not_ saying "It is an unrelated feature that I am not interested in";
it is saying "I am not interested in making my patch self consistent; a
half-baked hack that happens to solve my case is good enough for me."

Re: [PATCH] fast-import: add ignore non-existent files option.

[Shawn O. Pearce]

Felipe Contreras <felipe.contreras@gmail.com> wrote:
> On Tue, Sep 2, 2008 at 1:38 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> > Felipe Contreras <felipe.contreras@gmail.com> wrote:
> >> This is useful for SCMs that don't have proper changesets in each
> >> revision (monotone).
> >
> >> +--tolerant::
> >> +     Avoid fatal exceptions when actions are executed in non-existent
> >> +     files.  For example removing a file that is not there.
> >> +
> >
> > Yea.  But I'm not sure --tolerant is the best name.  --ignore-errors
> > or --treat-errors-as-warnings sounds better to me.
> 
> I initially named it --ignore-non-existent, but I thought the option
> was too specific.
> 
> --ignore-errors or --treat-errors-as-warnings imply all errors. It
> might make sense to always fail at certain errors, like 'mark not
> found'. I thought 'relaxed' or 'tolerant' would imply that only some
> errors will be allowed, not all.

OK, that argument makes sense.  Then I wonder if more specific
error ignoring would be better:

  --ignore-error=already-deleted
  --ignore-error=already-deleted,missing-mark,missing-copy-source

I'm not really fond of turning an existing error condition that
exists to catch broken frontends into a generic tolerant flag.
But being able to selectively turn it off while leaving other
errors as errors isn't entirely unreasonable.

-- 
Shawn.

Re: [PATCH] fast-import: add ignore non-existent files option.

[Junio C Hamano]

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

> OK, that argument makes sense.  Then I wonder if more specific
> error ignoring would be better:
>
>   --ignore-error=already-deleted
>   --ignore-error=already-deleted,missing-mark,missing-copy-source
>
> I'm not really fond of turning an existing error condition that
> exists to catch broken frontends into a generic tolerant flag.
> But being able to selectively turn it off while leaving other
> errors as errors isn't entirely unreasonable.

I think selective loosening of consistency check makes sense very much,
but I have been wondering if these should be command line options.

The only example we saw so far is about output from one exporter.  Perhaps
it should be given to fast-import as initial set of commands ("#pragma"!)
that describes the nature of the input file?

Re: [PATCH] fast-import: add ignore non-existent files option.

[Shawn O. Pearce]

Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
> 
> > OK, that argument makes sense.  Then I wonder if more specific
> > error ignoring would be better:
> >
> >   --ignore-error=already-deleted
> >   --ignore-error=already-deleted,missing-mark,missing-copy-source
> >
> > I'm not really fond of turning an existing error condition that
> > exists to catch broken frontends into a generic tolerant flag.
> > But being able to selectively turn it off while leaving other
> > errors as errors isn't entirely unreasonable.
> 
> I think selective loosening of consistency check makes sense very much,
> but I have been wondering if these should be command line options.
> 
> The only example we saw so far is about output from one exporter.  Perhaps
> it should be given to fast-import as initial set of commands ("#pragma"!)
> that describes the nature of the input file?

Yea, I briefly considered that when I added the timestamp format
option.  I didn't bother because it was a single option and I figured
most frontends start git-fast-import directly.  But with this being
added a "format pragrma header thingy" makes a lot of sense.

Since comments are supported we could backdoor it with #pragma or
something like that, so existing files can still be (mostly) parsed
by an earlier git-fast-import.  But I wonder if that is wise given
that some classes of errors would still fail on the older import,
but would work on a newer one.  Might as well just make it a proper
command that would cause an older importer to fail out of the gate.

-- 
Shawn.

Re: [PATCH] fast-import: add ignore non-existent files option.

[Junio C Hamano]

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

> Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> I think selective loosening of consistency check makes sense very much,
>> but I have been wondering if these should be command line options.
>> 
>> The only example we saw so far is about output from one exporter.  Perhaps
>> it should be given to fast-import as initial set of commands ("#pragma"!)
>> that describes the nature of the input file?
>
> Yea, I briefly considered that when I added the timestamp format
> option.  I didn't bother because it was a single option and I figured
> most frontends start git-fast-import directly.  But with this being
> added a "format pragrma header thingy" makes a lot of sense.

Oh, I did not mean to suggest hiding it as a comment to silently allow
older fast-import slurp such input and produce broken results.

For input that needs such loosened error checking, old fast-import won't
produce correct results _anyway_, so I would agree that making these
things into explicit commands to cause older fast-import to error out
would make a lot more sense.

Re: [PATCH] fast-import: add ignore non-existent files option.

[Felipe Contreras]

On Tue, Sep 2, 2008 at 10:36 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Shawn O. Pearce" <spearce@spearce.org> writes:
>
>> Junio C Hamano <gitster@pobox.com> wrote:
>> ...
>>> I think selective loosening of consistency check makes sense very much,
>>> but I have been wondering if these should be command line options.
>>>
>>> The only example we saw so far is about output from one exporter.  Perhaps
>>> it should be given to fast-import as initial set of commands ("#pragma"!)
>>> that describes the nature of the input file?
>>
>> Yea, I briefly considered that when I added the timestamp format
>> option.  I didn't bother because it was a single option and I figured
>> most frontends start git-fast-import directly.  But with this being
>> added a "format pragrma header thingy" makes a lot of sense.
>
> Oh, I did not mean to suggest hiding it as a comment to silently allow
> older fast-import slurp such input and produce broken results.
>
> For input that needs such loosened error checking, old fast-import won't
> produce correct results _anyway_, so I would agree that making these
> things into explicit commands to cause older fast-import to error out
> would make a lot more sense.

I like the idea. This way all the information to make it work is
already on the stream.

If the importer doesn't have a certain feature it can fail already at
initialization.

-- 
Felipe Contreras

Re: [PATCH] fast-import: add ignore non-existent files option.

[Felipe Contreras]

On Tue, Sep 2, 2008 at 5:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
> "Felipe Contreras" <felipe.contreras@gmail.com> writes:
>
>> On Tue, Sep 2, 2008 at 2:04 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Felipe Contreras <felipe.contreras@gmail.com> writes:
>>>
>>>> This is useful for SCMs that don't have proper changesets in each
>>>> revision (monotone).
>>>
>>> I am still not convinced this is a proper workaround for the issue.  Why
>>> shouldn't the feeder of fast-import be able to do this?
>>
>> If I could get a list of the files that changed on each revision from
>> monotone I would, but that's not possible (I've asked in their mailing
>> list). Apparently there's a way to feed the right data, but it's
>> complicated.
>
> Ok, I did not realize that you are not keeping track of what the parents'
> trees look like when processing a merge commit.
>
> But it raises a bigger question.  You earlier said:
>
>    In monotone you can have something like:
>
>     A
>    / \
>    B D
>    | |
>    C E
>    \ /
>     F
>
>    If you do a 'delete foo' in B, and a 'delete bar' in F, you will get
>    this stored in the merge revision (F):
>
>    old_revision [C]
>    delete "foo"
>    delete "bar"
>
>    old_revision [E]
>    delete "bar"
>
>    All the changes of the merged branches are stored again in the merge revision.
>
> Now, does it talk about C and E because they are immediate parents of B
> and D, or because they are immediate child of the common ancestor F?  I am
> guessing it is the latter (what I mean is if it still talks aobut C and E
> if you had more intermediate commits between B and C).  What the merge at
> A records is not relative to B/D but relative to immediate child of the
> fork point.

Er, no it's the other way around. A is the original child, F is the
merge. I thought it was evident because of the alphabetical ordering.

> If that is the case, ignoring a delete that deletes already deleted path
> is fine; I think it is the least of your problem.  The above description
> makes me wonder what they say about modification.  Do they talk about the
> same modification that happened between B and C when talking about A?  If
> that is the case, it would be a far larger problem.  You cannot just say
> "ignore any modification recorded in a merge, because they have been
> already done on the branches being merged (i.e. up to B and up to D)", as
> A may have to be an evil merge.
>
> I have to wonder if you can "mark" the tree object of C, and when you
> process merge A, represent A's tree by starting from that marked tree,
> applying only the description to bring the state of "old_revision [C]"
> to that of A (delete "foo" and delete "bar" in your illustration), and
> recording that tree with parents B and D.

I'm not sure I understand correctly, but anyway, how would you propose
to "mark" the tree objects?

>>>> @@ -1993,8 +1994,15 @@ static void file_change_cr(struct branch *b, int rename)
>>>> ...
>>>
>>> Also what happened to the missing warning() for 'D'elete codepath?
>>
>> I'm not interested in it.
>
> If I were asking for an unrelated "feature" when you are developing some
> other feature, it would have been different, but I do not think that is a
> good answer in this particular case.
>
> Your --tolerant (or --ignore-errors) is about customizing strictness of
> the error handling, and you know of a case where the error handling is not
> strict enough in the existing code.  In other words, your "not interested"
> is _not_ saying "It is an unrelated feature that I am not interested in";
> it is saying "I am not interested in making my patch self consistent; a
> half-baked hack that happens to solve my case is good enough for me."

Hmm, ok.

-- 
Felipe Contreras