[PATCH v3 0/4] fast-export: general fixes

[PATCH v3 0/4] fast-export: general fixes

[Felipe Contreras]

Hi,

Note: sorry for the noise, the first try (v2) was silently eaten by the mailing
list handler.

First patches are general cleanups and fixes, the last patch fixes a real issue
that affects remote helpers.

Changes since v2:

 * Actually send it to the ml

Changes since v1:

 * Improved commit messages
 * Use /dev/null in tests
 * Add test for remote helpers

Felipe Contreras (4):
  fast-export: trivial cleanup
  fast-export: fix comparisson in tests
  fast-export: don't handle uninteresting refs
  fast-export: make sure refs are updated properly

 builtin/fast-export.c     | 16 +++++++++++-----
 t/t5800-remote-helpers.sh | 11 +++++++++++
 t/t9350-fast-export.sh    | 26 +++++++++++++++++++++++---
 3 files changed, 45 insertions(+), 8 deletions(-)

-- 
1.8.0

[PATCH v3 1/4] fast-export: trivial cleanup

[Felipe Contreras]

Setting commit to commit is a no-op. It might have been there to avoid a
compiler warning, but if so, it was the compiler to blame.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 12220ad..065f324 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -483,7 +483,7 @@ static void get_tags_and_duplicates(struct object_array *pending,
 	for (i = 0; i < pending->nr; i++) {
 		struct object_array_entry *e = pending->objects + i;
 		unsigned char sha1[20];
-		struct commit *commit = commit;
+		struct commit *commit;
 		char *full_name;
 
 		if (dwim_ref(e->name, strlen(e->name), sha1, &full_name) != 1)
-- 
1.8.0

[PATCH v3 2/4] fast-export: fix comparisson in tests

[Felipe Contreras]

First the expected, then the actual, otherwise the diff would be the
opposite of what we want.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 t/t9350-fast-export.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 3e821f9..49bdb44 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -303,7 +303,7 @@ test_expect_success 'dropping tag of filtered out object' '
 (
 	cd limit-by-paths &&
 	git fast-export --tag-of-filtered-object=drop mytag -- there > output &&
-	test_cmp output expected
+	test_cmp expected output
 )
 '
 
@@ -320,7 +320,7 @@ test_expect_success 'rewriting tag of filtered out object' '
 (
 	cd limit-by-paths &&
 	git fast-export --tag-of-filtered-object=rewrite mytag -- there > output &&
-	test_cmp output expected
+	test_cmp expected output
 )
 '
 
@@ -351,7 +351,7 @@ test_expect_failure 'no exact-ref revisions included' '
 	(
 		cd limit-by-paths &&
 		git fast-export master~2..master~1 > output &&
-		test_cmp output expected
+		test_cmp expected output
 	)
 '
 
-- 
1.8.0

[PATCH v3 3/4] fast-export: don't handle uninteresting refs

[Felipe Contreras]

They have been marked as UNINTERESTING for a reason, lets respect that.

Currently the first ref is handled properly, but not the rest, so:

 % git fast-export master ^master

Would currently throw a reset for master (2nd ref), which is not what we
want.

 % git fast-export master ^foo ^bar ^roo
 % git fast-export master salsa..tacos

Even if all these refs point to the same object; foo, bar, roo, salsa,
and tacos would all get a reset.

This is most certainly not what we want. After this patch, nothing gets
exported, because nothing was selected (everything is UNINTERESTING).

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c  | 7 ++++---
 t/t9350-fast-export.sh | 6 ++++++
 2 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 065f324..7fb6fe1 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -523,10 +523,11 @@ static void get_tags_and_duplicates(struct object_array *pending,
 				typename(e->item->type));
 			continue;
 		}
-		if (commit->util)
+		if (commit->util) {
 			/* more than one name for the same object */
-			string_list_append(extra_refs, full_name)->util = commit;
-		else
+			if (!(commit->object.flags & UNINTERESTING))
+				string_list_append(extra_refs, full_name)->util = commit;
+		} else
 			commit->util = full_name;
 	}
 }
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 49bdb44..6ea8f6f 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -440,4 +440,10 @@ test_expect_success 'fast-export quotes pathnames' '
 	)
 '
 
+test_expect_success 'proper extra refs handling' '
+	git fast-export master ^master master..master > actual &&
+	echo -n > expected &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.0

[PATCH v3 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

When an object has already been exported (and thus is in the marks) it
is flagged as SHOWN, so it will not be exported again, even if this time
it's exported through a different ref.

We don't need the object to be exported again, but we want the ref
updated, which doesn't happen.

Since we can't know if a ref was exported or not, let's just assume that
if the commit was marked (flags & SHOWN), the user still wants the ref
updated.

So:

 % git branch test master
 % git fast-export $mark_flags master
 % git fast-export $mark_flags test

Would export 'test' properly.

Additionally, this fixes issues with remote helpers; now they can push
refs wich objects have already been exported.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/fast-export.c     | 11 ++++++++---
 t/t5800-remote-helpers.sh | 11 +++++++++++
 t/t9350-fast-export.sh    | 14 ++++++++++++++
 3 files changed, 33 insertions(+), 3 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 7fb6fe1..663a93d 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
 				typename(e->item->type));
 			continue;
 		}
-		if (commit->util) {
-			/* more than one name for the same object */
+
+		/*
+		 * This ref will not be updated through a commit, lets make
+		 * sure it gets properly upddated eventually.
+		 */
+		if (commit->util || commit->object.flags & SHOWN) {
 			if (!(commit->object.flags & UNINTERESTING))
 				string_list_append(extra_refs, full_name)->util = commit;
-		} else
+		}
+		if (!commit->util)
 			commit->util = full_name;
 	}
 }
diff --git a/t/t5800-remote-helpers.sh b/t/t5800-remote-helpers.sh
index e7dc668..69a145a 100755
--- a/t/t5800-remote-helpers.sh
+++ b/t/t5800-remote-helpers.sh
@@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '
 	compare_refs clone HEAD server refs/heads/new-refspec
 '
 
+test_expect_success 'push ref with existing object' '
+	(cd localclone &&
+	git branch point-to-master master &&
+	git push origin point-to-master
+	) &&
+
+	(cd server &&
+	git show-ref refs/heads/point-to-master
+	)
+'
+
 test_done
diff --git a/t/t9350-fast-export.sh b/t/t9350-fast-export.sh
index 6ea8f6f..a4178e3 100755
--- a/t/t9350-fast-export.sh
+++ b/t/t9350-fast-export.sh
@@ -446,4 +446,18 @@ test_expect_success 'proper extra refs handling' '
 	test_cmp expected actual
 '
 
+cat > expected << EOF
+reset refs/heads/master
+from :13
+
+EOF
+
+test_expect_success 'refs are updated even if no commits need to be exported' '
+	git fast-export --import-marks=tmp-marks \
+		--export-marks=tmp-marks master > /dev/null &&
+	git fast-export --import-marks=tmp-marks \
+		--export-marks=tmp-marks master > actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
1.8.0

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Jonathan Nieder]

(cc-ing the git list)
Felipe Contreras wrote:

> When an object has already been exported (and thus is in the marks) it
> is flagged as SHOWN, so it will not be exported again, even if this time
> it's exported through a different ref.
>
> We don't need the object to be exported again, but we want the ref
> updated

Yes, makes perfect sense.

For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

[...]
> --- a/t/t5800-remote-helpers.sh
> +++ b/t/t5800-remote-helpers.sh
> @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '
>  	compare_refs clone HEAD server refs/heads/new-refspec
>  '
>  
> +test_expect_success 'push ref with existing object' '
> +	(cd localclone &&
> +	git branch point-to-master master &&
> +	git push origin point-to-master
> +	) &&
> +
> +	(cd server &&
> +	git show-ref refs/heads/point-to-master
> +	)

Style: if you indent like this, the test becomes clearer:

	(
		cd localclone &&
		git branch point-to-master master &&
		git push origin point-to-master
	) &&
	(
		cd server &&
		git rev-parse --verify refs/heads/point-to-master
	)

[...]
> +test_expect_success 'refs are updated even if no commits need to be exported' '
> +	git fast-export --import-marks=tmp-marks \
> +		--export-marks=tmp-marks master > /dev/null &&

The redirect just makes the test log with "-v" less informative, so
I'd drop it.

> +	git fast-export --import-marks=tmp-marks \
> +		--export-marks=tmp-marks master > actual &&
> +	test_cmp expected actual

Redirections in git shell scripts are generally spelled as
"do_something >actual", without a space between the operator and
filename.

Hope that helps,
Jonathan

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Wed, Oct 31, 2012 at 1:11 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> (cc-ing the git list)
> Felipe Contreras wrote:
>
>> When an object has already been exported (and thus is in the marks) it
>> is flagged as SHOWN, so it will not be exported again, even if this time
>> it's exported through a different ref.
>>
>> We don't need the object to be exported again, but we want the ref
>> updated
>
> Yes, makes perfect sense.
>
> For what it's worth,
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Yay!

> [...]
>> --- a/t/t5800-remote-helpers.sh
>> +++ b/t/t5800-remote-helpers.sh
>> @@ -145,4 +145,15 @@ test_expect_failure 'push new branch with old:new refspec' '
>>       compare_refs clone HEAD server refs/heads/new-refspec
>>  '
>>
>> +test_expect_success 'push ref with existing object' '
>> +     (cd localclone &&
>> +     git branch point-to-master master &&
>> +     git push origin point-to-master
>> +     ) &&
>> +
>> +     (cd server &&
>> +     git show-ref refs/heads/point-to-master
>> +     )
>
> Style: if you indent like this, the test becomes clearer:

And then it would become inconsistent with the rest of the file.

>> +     git fast-export --import-marks=tmp-marks \
>> +             --export-marks=tmp-marks master > actual &&
>> +     test_cmp expected actual
>
> Redirections in git shell scripts are generally spelled as
> "do_something >actual", without a space between the operator and
> filename.

I generally am OK with adapting to whatever code-style is used
(sometimes under protest), but this is a place where I draw   the
line. Sorry, '>actual' is more annoying to me than a knife screeching
glass. Fortunately, '> actual' is used in many other places in 't/',
so I'm going to use the other people jumping over the bridge argument.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Jonathan Nieder]

Felipe Contreras wrote:

> --- a/builtin/fast-export.c
> +++ b/builtin/fast-export.c
> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
>  				typename(e->item->type));
>  			continue;
>  		}
> -		if (commit->util) {
> -			/* more than one name for the same object */
> +
> +		/*
> +		 * This ref will not be updated through a commit, lets make
> +		 * sure it gets properly upddated eventually.
> +		 */
> +		if (commit->util || commit->object.flags & SHOWN) {
>  			if (!(commit->object.flags & UNINTERESTING))
>  				string_list_append(extra_refs, full_name)->util = commit;
> -		} else
> +		}
> +		if (!commit->util)
>  			commit->util = full_name;

Here's an explanation of why the above makes sense to me.

get_tags_and_duplicates() gets called after the marks import and
before the revision walk.  It walks through the revs from the
commandline and for each one:

 - peels it to a refname, and then to a commit
 - stores the refname so fast-export knows what arg to pass to
   the "commit" command during the revision walk
 - if it already had a refname stored, instead adds the
   (refname, commit) pair to the extra_refs list, so fast-export
   knows to add a "reset" command later.

If the commit already has the SHOWN flag set because it was pointed to
by a mark, it is not going to come up in the revision walk, so it will
not be mentioned in the output stream unless it is added to
extra_refs.  That's what this patch does.

Incidentally, the change from "else" to "if (!commit->util)" is
unnecessary because if a commit is already SHOWN then it will not be
encountered in the revision walk so commit->util does not need to be
set.

If the commit does not have the SHOWN or UNINTERESTING flag set but it
is going to get the UNINTERESTING flag set during the walk because of
a negative commit listed on the command line, this patch won't help.

Jonathan

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 5:37 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
>>                               typename(e->item->type));
>>                       continue;
>>               }
>> -             if (commit->util) {
>> -                     /* more than one name for the same object */
>> +
>> +             /*
>> +              * This ref will not be updated through a commit, lets make
>> +              * sure it gets properly upddated eventually.
>> +              */
>> +             if (commit->util || commit->object.flags & SHOWN) {
>>                       if (!(commit->object.flags & UNINTERESTING))
>>                               string_list_append(extra_refs, full_name)->util = commit;
>> -             } else
>> +             }
>> +             if (!commit->util)
>>                       commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk.  It walks through the revs from the
> commandline and for each one:
>
>  - peels it to a refname, and then to a commit
>  - stores the refname so fast-export knows what arg to pass to
>    the "commit" command during the revision walk
>  - if it already had a refname stored, instead adds the
>    (refname, commit) pair to the extra_refs list, so fast-export
>    knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs.  That's what this patch does.
>
> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.
>
> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.

Thanks for the thorough explanation. Perhaps some of that could make
it's way into the commit message?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Jonathan Nieder]

Sverre Rabbelier wrote:

> Thanks for the thorough explanation. Perhaps some of that could make
> it's way into the commit message?

It's fine with me if it doesn't, since the original commit message
covers the basics (current behavior and intent of the change) in its
first two paragraphs and anyone wanting more detail can use

	GIT_NOTES_REF=refs/remotes/charon/notes/full \
	git show --show-notes <commit>

to find more details.

Thanks,
Jonathan

[OT] How to get the discussion details via notes

[Peter Baumann]

Dropping the Cc list, as this is off topic

On Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:
> Sverre Rabbelier wrote:
> 
> > Thanks for the thorough explanation. Perhaps some of that could make
> > it's way into the commit message?
> 
> It's fine with me if it doesn't, since the original commit message
> covers the basics (current behavior and intent of the change) in its
> first two paragraphs and anyone wanting more detail can use
> 
> 	GIT_NOTES_REF=refs/remotes/charon/notes/full \
> 	git show --show-notes <commit>
> 
> to find more details.

I seem to miss something here, but I don't get it how the notes ref
becomes magically filled with the details of this discussion.

Care to explain?

-Peter

Re: [OT] How to get the discussion details via notes

[Drew Northup]

On Wed, Oct 31, 2012 at 5:53 AM, Peter Baumann <waste.manager@gmx.de> wrote:
> Dropping the Cc list, as this is off topic
>
> On Tue, Oct 30, 2012 at 11:05:29PM -0700, Jonathan Nieder wrote:
>> Sverre Rabbelier wrote:
>>
>> > Thanks for the thorough explanation. Perhaps some of that could make
>> > it's way into the commit message?
>>
>> It's fine with me if it doesn't, since the original commit message
>> covers the basics (current behavior and intent of the change) in its
>> first two paragraphs and anyone wanting more detail can use
>>
>>       GIT_NOTES_REF=refs/remotes/charon/notes/full \
>>       git show --show-notes <commit>
>>
>> to find more details.
>
> I seem to miss something here, but I don't get it how the notes ref
> becomes magically filled with the details of this discussion.
>
> Care to explain?

If I have an email thread I'd like to store alongside a commit I'll
put that into a note, but I usually don't push that kind of thing out
to a remote repo.
Does that help?

-- 
-Drew Northup
--------------------------------------------------------------
"As opposed to vegetable or mineral error?"
-John Pescatore, SANS NewsBites Vol. 12 Num. 59

Re: [OT] How to get the discussion details via notes

[Jeff King]

On Wed, Oct 31, 2012 at 10:53:27AM +0100, Peter Baumann wrote:

> > covers the basics (current behavior and intent of the change) in its
> > first two paragraphs and anyone wanting more detail can use
> > 
> > 	GIT_NOTES_REF=refs/remotes/charon/notes/full \
> > 	git show --show-notes <commit>
> > 
> > to find more details.
> 
> I seem to miss something here, but I don't get it how the notes ref
> becomes magically filled with the details of this discussion.

Thomas Rast (aka charon) keeps a mapping of commits to the email threads
that led to them. You can fetch it from:

   git://repo.or.cz/git/trast.git

(try the notes/full and notes/terse refs).

-Peff

Re: [OT] How to get the discussion details via notes

[Peter Baumann]

On Wed, Oct 31, 2012 at 10:10:24AM -0400, Jeff King wrote:
> On Wed, Oct 31, 2012 at 10:53:27AM +0100, Peter Baumann wrote:
> 
> > > covers the basics (current behavior and intent of the change) in its
> > > first two paragraphs and anyone wanting more detail can use
> > > 
> > > 	GIT_NOTES_REF=refs/remotes/charon/notes/full \
> > > 	git show --show-notes <commit>
> > > 
> > > to find more details.
> > 
> > I seem to miss something here, but I don't get it how the notes ref
> > becomes magically filled with the details of this discussion.
> 
> Thomas Rast (aka charon) keeps a mapping of commits to the email threads
> that led to them. You can fetch it from:
> 
>    git://repo.or.cz/git/trast.git
> 
> (try the notes/full and notes/terse refs).
> 

Nice! I didn't know about that.

-Peter

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Wed, Oct 31, 2012 at 1:37 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> --- a/builtin/fast-export.c
>> +++ b/builtin/fast-export.c
>> @@ -523,11 +523,16 @@ static void get_tags_and_duplicates(struct object_array *pending,
>>                               typename(e->item->type));
>>                       continue;
>>               }
>> -             if (commit->util) {
>> -                     /* more than one name for the same object */
>> +
>> +             /*
>> +              * This ref will not be updated through a commit, lets make
>> +              * sure it gets properly upddated eventually.
>> +              */
>> +             if (commit->util || commit->object.flags & SHOWN) {
>>                       if (!(commit->object.flags & UNINTERESTING))
>>                               string_list_append(extra_refs, full_name)->util = commit;
>> -             } else
>> +             }
>> +             if (!commit->util)
>>                       commit->util = full_name;
>
> Here's an explanation of why the above makes sense to me.
>
> get_tags_and_duplicates() gets called after the marks import and
> before the revision walk.  It walks through the revs from the
> commandline and for each one:
>
>  - peels it to a refname, and then to a commit
>  - stores the refname so fast-export knows what arg to pass to
>    the "commit" command during the revision walk
>  - if it already had a refname stored, instead adds the
>    (refname, commit) pair to the extra_refs list, so fast-export
>    knows to add a "reset" command later.
>
> If the commit already has the SHOWN flag set because it was pointed to
> by a mark, it is not going to come up in the revision walk, so it will
> not be mentioned in the output stream unless it is added to
> extra_refs.  That's what this patch does.

That is correct.

> Incidentally, the change from "else" to "if (!commit->util)" is
> unnecessary because if a commit is already SHOWN then it will not be
> encountered in the revision walk so commit->util does not need to be
> set.

Maybe, but that's yet another change, and with more changes come more
possibilities of regressions. I haven't verified this is the case.

If this makes sense, I would do it in another, separate patch.

> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.

I don't know what that means in practice.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Jeff King]

On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:

> If the commit does not have the SHOWN or UNINTERESTING flag set but it
> is going to get the UNINTERESTING flag set during the walk because of
> a negative commit listed on the command line, this patch won't help.

Right, so my understanding of the situation is that doing this:

  $ git branch foo master~1
  $ git fast-export foo master~1..master

won't show "foo", which seems wrong to me. _But_ we currently get that
wrong already, so Felipe's patches are not making anything worse, but
are fixing some situations (namely when master~1 is not mentioned on the
command-line, but rather in a marks file).

Is that correct?

If so, then this series isn't regressing behavior; the only downside is
that it's an incomplete fix. In theory this could get in the way of the
full fix later on, but given the commit messages and the archive of this
discussion, it would be simple enough to revert it later in favor of a
more full fix. Is that accurate?

Sorry if I am belaboring the discussion. I just want to make sure I
understand the situation before deciding what to do with the topic. It
sounds like the consensus at this point is "not perfect, but good enough
to make forward progress".

-Peff

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Jonathan Nieder]

Jeff King wrote:

> If so, then this series isn't regressing behavior; the only downside is
> that it's an incomplete fix. In theory this could get in the way of the
> full fix later on, but given the commit messages and the archive of this
> discussion, it would be simple enough to revert it later in favor of a
> more full fix. Is that accurate?
>
> Sorry if I am belaboring the discussion. I just want to make sure I
> understand the situation before deciding what to do with the topic. It
> sounds like the consensus at this point is "not perfect, but good enough
> to make forward progress".

Patch 1, 2, and 4 are good modulo their descriptions.  They should
work fine without patch 3.

Patch 3 is a regression in comprehensibility.  I think we can do
better.  Maybe all it would take is a less confusing description, and
tweaks to the code (to loop over revs->cmdline instead of
revs->pending) could come on top.

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Johannes Schindelin]

Hi Peff,

On Fri, 2 Nov 2012, Jeff King wrote:

> On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:
> 
> > If the commit does not have the SHOWN or UNINTERESTING flag set but it
> > is going to get the UNINTERESTING flag set during the walk because of
> > a negative commit listed on the command line, this patch won't help.
> 
> Right, so my understanding of the situation is that doing this:
> 
>   $ git branch foo master~1
>   $ git fast-export foo master~1..master
> 
> won't show "foo", which seems wrong to me. _But_ we currently get that
> wrong already, so Felipe's patches are not making anything worse, but
> are fixing some situations (namely when master~1 is not mentioned on the
> command-line, but rather in a marks file).
> 
> Is that correct?
> 
> If so, then this series isn't regressing behavior; the only downside is
> that it's an incomplete fix. In theory this could get in the way of the
> full fix later on, but given the commit messages and the archive of this
> discussion, it would be simple enough to revert it later in favor of a
> more full fix. Is that accurate?

>From my understanding, yes.

> Sorry if I am belaboring the discussion. I just want to make sure I
> understand the situation before deciding what to do with the topic. It
> sounds like the consensus at this point is "not perfect, but good enough
> to make forward progress".

I appreciate that stance very much. The patch Sverre and I proposed was
also an incomplete fix (although I suspect it would fix the issue you
pointed out above), so I agree with the "perfect is the enemy of the good"
approach, obviously.

May I just ask to include a summary of that rationale into the commit
message rather than relying on people having internet access and knowing
where to look? Adding the following to the commit message would be good
enough for me:

	Note that

		$ git branch foo master~1
		$ git fast-export foo master~1..master

	still does not update the "foo" ref, but a partial fix is better
	than no fix.

Thanks,
Dscho

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Jeff King]

On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote:

> > If so, then this series isn't regressing behavior; the only downside is
> > that it's an incomplete fix. In theory this could get in the way of the
> > full fix later on, but given the commit messages and the archive of this
> > discussion, it would be simple enough to revert it later in favor of a
> > more full fix. Is that accurate?
> 
> From my understanding, yes.
> 
> > Sorry if I am belaboring the discussion. I just want to make sure I
> > understand the situation before deciding what to do with the topic. It
> > sounds like the consensus at this point is "not perfect, but good enough
> > to make forward progress".
> 
> I appreciate that stance very much. The patch Sverre and I proposed was
> also an incomplete fix (although I suspect it would fix the issue you
> pointed out above), so I agree with the "perfect is the enemy of the good"
> approach, obviously.

Thanks for the response.

> May I just ask to include a summary of that rationale into the commit
> message rather than relying on people having internet access and knowing
> where to look? Adding the following to the commit message would be good
> enough for me:
> 
> 	Note that
> 
> 		$ git branch foo master~1
> 		$ git fast-export foo master~1..master
> 
> 	still does not update the "foo" ref, but a partial fix is better
> 	than no fix.

Yes, I think that makes a lot of sense.

Felipe, I notice that you sent out a big "fast-export improvements"
series. Does that supersede this?

-Peff

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Fri, Nov 2, 2012 at 4:19 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Nov 02, 2012 at 04:17:14PM +0100, Johannes Schindelin wrote:

>> May I just ask to include a summary of that rationale into the commit
>> message rather than relying on people having internet access and knowing
>> where to look? Adding the following to the commit message would be good
>> enough for me:
>>
>>       Note that
>>
>>               $ git branch foo master~1
>>               $ git fast-export foo master~1..master
>>
>>       still does not update the "foo" ref, but a partial fix is better
>>       than no fix.
>
> Yes, I think that makes a lot of sense.
>
> Felipe, I notice that you sent out a big "fast-export improvements"
> series. Does that supersede this?

Yes. I noticed this patch fixes other tests.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v3 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Fri, Nov 2, 2012 at 2:12 PM, Jeff King <peff@peff.net> wrote:
> On Tue, Oct 30, 2012 at 05:37:21PM -0700, Jonathan Nieder wrote:
>
>> If the commit does not have the SHOWN or UNINTERESTING flag set but it
>> is going to get the UNINTERESTING flag set during the walk because of
>> a negative commit listed on the command line, this patch won't help.
>
> Right, so my understanding of the situation is that doing this:
>
>   $ git branch foo master~1
>   $ git fast-export foo master~1..master
>
> won't show "foo", which seems wrong to me. _But_ we currently get that
> wrong already, so Felipe's patches are not making anything worse, but
> are fixing some situations (namely when master~1 is not mentioned on the
> command-line, but rather in a marks file).
>
> Is that correct?

Yes, that's correct. But my patch ("make sure refs are updated
properly") does _not_ change in any shape or form what happens with
what you specify in the command line, only what happens with marks.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> When an object has already been exported (and thus is in the marks) it
> is flagged as SHOWN, so it will not be exported again, even if this time
> it's exported through a different ref.
>
> We don't need the object to be exported again, but we want the ref
> updated, which doesn't happen.
>
> Since we can't know if a ref was exported or not, let's just assume that
> if the commit was marked (flags & SHOWN), the user still wants the ref
> updated.
>
> So:
>
>  % git branch test master
>  % git fast-export $mark_flags master
>  % git fast-export $mark_flags test
>
> Would export 'test' properly.
>
> Additionally, this fixes issues with remote helpers; now they can push
> refs wich objects have already been exported.

Won't this also export child (or maybe parent) branches that weren't
mentioned? For example:

$ git branch one
$ echo foo > content
$ git commit -m two
$ git fast-export one
$ git fast-export two

I suspect that one of those will export both one and two. If not, this
seems like a great solution to the fast-export problem.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Tue, Oct 30, 2012 at 7:12 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 10:11 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> When an object has already been exported (and thus is in the marks) it
>> is flagged as SHOWN, so it will not be exported again, even if this time
>> it's exported through a different ref.
>>
>> We don't need the object to be exported again, but we want the ref
>> updated, which doesn't happen.
>>
>> Since we can't know if a ref was exported or not, let's just assume that
>> if the commit was marked (flags & SHOWN), the user still wants the ref
>> updated.
>>
>> So:
>>
>>  % git branch test master
>>  % git fast-export $mark_flags master
>>  % git fast-export $mark_flags test
>>
>> Would export 'test' properly.
>>
>> Additionally, this fixes issues with remote helpers; now they can push
>> refs wich objects have already been exported.
>
> Won't this also export child (or maybe parent) branches that weren't
> mentioned? For example:
>
> $ git branch one
> $ echo foo > content
> $ git commit -m two
> $ git fast-export one
> $ git fast-export two
>
> I suspect that one of those will export both one and two. If not, this
> seems like a great solution to the fast-export problem.

Why would it? We are not changing the way objects are exported, the
only difference is what happens at the end
(handle_tags_and_duplicates()).

And if you are talking about the ref for the reset at the end, it has
to be both in the list of refs selected by the user (initially in
&revs.pending), either marked or the object already referenced by
another ref in the list selected by the user (e.g. fast-export one
two, where one^{commit} == two^{commit}, and not marked as
UNINTERESTING (e.g. ^two).

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Why would it? We are not changing the way objects are exported, the
> only difference is what happens at the end
> (handle_tags_and_duplicates()).

Because the marking is per-commit, not per-ref, right? Perhaps you
could add a simple test case to make sure it works as expected?
Something along the lines of the scenario I described in my previous
email?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Why would it? We are not changing the way objects are exported, the
>> only difference is what happens at the end
>> (handle_tags_and_duplicates()).
>
> Because the marking is per-commit, not per-ref, right?

Oh, you meant using marks?

It doesn't matter anyway, because get_tags_and_duplicates() would get
'one' on the first run, and 'two' on the second.

If you meant something like this:
% git fast-export $marks_args one
% git fast-export $marks_args one two

Then yeah, 'one' will be updated once again in the second command, but
there's nothing fatal about it, and your patch series had the same
result.

> Perhaps you
> could add a simple test case to make sure it works as expected?
> Something along the lines of the scenario I described in my previous
> email?

I'm not sure what that test should be doing.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Jonathan Nieder]

Felipe Contreras wrote:

> % git fast-export $marks_args one
> % git fast-export $marks_args one two
>
> Then yeah, 'one' will be updated once again in the second command,

That's probably worth a mention in the commit message and tests
(test_expect_failure), to save future readers from some confusion.

Thanks,
Jonathan

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Tue, Oct 30, 2012 at 10:38 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> % git fast-export $marks_args one
>> % git fast-export $marks_args one two
>>
>> Then yeah, 'one' will be updated once again in the second command,
>
> That's probably worth a mention in the commit message and tests
> (test_expect_failure), to save future readers from some confusion.

It is mentioned in the commit message.

-- 
Felipe Contreras

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
>> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
>> <felipe.contreras@gmail.com> wrote:
>>> Why would it? We are not changing the way objects are exported, the
>>> only difference is what happens at the end
>>> (handle_tags_and_duplicates()).
>>
>> Because the marking is per-commit, not per-ref, right?
>
> Oh, you meant using marks?

No, I meant the 'SHOWN' flag, doesn't it get added per commit, not per
ref? That is, commit->object.flags & SHOWN refers to the object
underlying the ref. So I suspect this scenario doesn't pass the tests:

git init &&
echo first > content &&
git add content &&
git commit -m "first" &&
git branch first &&
echo two > content &&
git commit -m "second" &&
git branch second &&
git fast-export first > actual &&
test_cmp actual expected_first &&
git fast-export second > actual &&
test_cmp actual expected_second

With expected_first being something like:
<fast-export stream with the first commit>
<reset command to set first to the right commit>

And expected_second being something like
<fast export stream with the first and second command>
<reset command to set first and second to their respective branches>

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Tue, Oct 30, 2012 at 10:59 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 2:35 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Tue, Oct 30, 2012 at 10:17 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
>>> On Tue, Oct 30, 2012 at 11:47 AM, Felipe Contreras
>>> <felipe.contreras@gmail.com> wrote:
>>>> Why would it? We are not changing the way objects are exported, the
>>>> only difference is what happens at the end
>>>> (handle_tags_and_duplicates()).
>>>
>>> Because the marking is per-commit, not per-ref, right?
>>
>> Oh, you meant using marks?
>
> No, I meant the 'SHOWN' flag, doesn't it get added per commit, not per
> ref? That is, commit->object.flags & SHOWN refers to the object
> underlying the ref. So I suspect this scenario doesn't pass the tests:

Without marks you cannot have the SHOWN mark at that point; we haven't
traversed the commits.

> git init &&
> echo first > content &&
> git add content &&
> git commit -m "first" &&
> git branch first &&
> echo two > content &&
> git commit -m "second" &&
> git branch second &&
> git fast-export first > actual &&
> test_cmp actual expected_first &&
> git fast-export second > actual &&
> test_cmp actual expected_second
>
> With expected_first being something like:
> <fast-export stream with the first commit>
> <reset command to set first to the right commit>

Why would a 'reset' command be expected if the 'first' branch is
already pointing to the 'first' commit?

> And expected_second being something like
> <fast export stream with the first and second command>
> <reset command to set first and second to their respective branches>

Ditto, plus, why would 'git fast-export second' do anything regarding
'first'? It wasn't specified in the committish; it's not relevant.

Before an after my patch the output is the same:

% git fast-export first:
reset refs/heads/first
commit refs/heads/first

% git fast-export second:
reset refs/heads/second
commit refs/heads/second
commit refs/heads/second

Which is expected and correct; the branch already points to the right
commit, no need for an extra reset.

Cheers.

-- 
Felipe Contreras

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Sverre Rabbelier]

On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> Which is expected and correct; the branch already points to the right
> commit, no need for an extra reset.

I think you're correct. Thanks for confirming.

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH v2 4/4] fast-export: make sure refs are updated properly

[Felipe Contreras]

On Tue, Oct 30, 2012 at 11:35 PM, Sverre Rabbelier <srabbelier@gmail.com> wrote:
> On Tue, Oct 30, 2012 at 3:18 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> Which is expected and correct; the branch already points to the right
>> commit, no need for an extra reset.
>
> I think you're correct. Thanks for confirming.

Thanks for reviewing. If you are still not convinced, I could pull the
patches from msysgit and simplify them, I'm sure the end result would
be pretty similar, if not exactly the same as this patch (plus other
orthogonal changes). I saw some patches that were not part of the
patch series you sent before, so maybe that's why you expected certain
behavior that wasn't actually there in that particular patch series.

But hopefully that's not needed.

Cheers.

-- 
Felipe Contreras