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

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

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

[Jonathan Nieder]

Felipe Contreras wrote:

> Setting commit to commit is a no-op.

Wrong description.  This should say:

	The code uses the idiom of assigning commit to itself to quench a
	"may be used uninitialized" warning.  Luckily at least modern
	versions of gcc do not produce that warning here, so we can drop
	the self-assignment.

	This makes the code clearer to human beings, makes static
	analyzers that do not know that idiom happier, and means that
	if the code some day evolves to use this variable uninitialized
	then we will catch it.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

With that change, for what it's worth,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Patch left unsnipped since it doesn't seem to have hit the list.

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

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

[Jonathan Nieder]

(actually cc-ing the git list this time.  Sorry for the noise, all.)
Felipe Contreras wrote:

> [Subject: [PATCH v2 2/4] fast-export: fix comparisson in tests]
>
> First the expected, then the actual, otherwise the diff would be the
> opposite of what we want.

Spelling: s/comparisson/comparison/.

Semantics: this isn't actually fixing anything --- it's a cosmetic
thing.  It would be clearer to say:

	fast-export test: swap arguments to test_cmp

	This way if diff output is produced, it describes how the
	actual output differs from what was expected rather than the
	other way around.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>

For what it's worth, with amended message,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Patch left unsnipped because it hadn't hit the list.

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

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

[Jonathan Nieder]

Felipe Contreras wrote:

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

This patch looks unsafe, and in the examples listed in the patch
description the changed behavior does not look like an improvement.
Worse, the description lists a few examples but gives no convincing
explanation to reassure about the lack of bad behavior for examples
not listed.

Perhaps this patch has a prerequisite and has come out of order.

Hope that helps,
Jonathan

Patch left unsnipped so we can get a copy in the list archive.

> 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

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

[Felipe Contreras]

(again to the mailing list)

On Tue, Oct 30, 2012 at 7:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> They have been marked as UNINTERESTING for a reason, lets respect that.

That doesn't say anything.

> and in the examples listed in the patch
> description the changed behavior does not look like an improvement.

I disagree.

% git log master ^master

What do you expect? Nothing.

% git fast-export master ^master

What do you expect? Nothing.

> Worse, the description lists a few examples but gives no convincing
> explanation to reassure about the lack of bad behavior for examples
> not listed.

What examples not listed?

-- 
Felipe Contreras

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

[Felipe Contreras]

On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> and in the examples listed in the patch
>>> description the changed behavior does not look like an improvement.
>>
>> I disagree.
>>
>> % git log master ^master
>>
>> What do you expect? Nothing.
>
> Yep.
>
>> % git fast-export master ^master
>>
>> What do you expect? Nothing.

So you think what we have now is the correct behavior:

% git fast-export master ^master
reset refs/heads/master
from :0

That of course would crash fast-import. But hey, it's your opinion.

Would be interesting to see if other people think the above is correct.

Cheers.

-- 
Felipe Contreras

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

[Jonathan Nieder]

Felipe Contreras wrote:

> So you think what we have now is the correct behavior:
>
> % git fast-export master ^master
> reset refs/heads/master
> from :0

No, I don't think that, either.

Hope that helps,
Jonathan

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

[Felipe Contreras]

On Tue, Oct 30, 2012 at 10:45 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> So you think what we have now is the correct behavior:
>>
>> % git fast-export master ^master
>> reset refs/heads/master
>> from :0
>
> No, I don't think that, either.

Well, that's what we have now, and you want to preserve this "feature"
(aka bug), right?

And I still haven't why this is "unsafe", and what are those "examples
not listed".

-- 
Felipe Contreras

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

[Jonathan Nieder]

Felipe Contreras wrote:

> Well, that's what we have now, and you want to preserve this "feature"
> (aka bug), right?

Nope.  I just don't want regressions, and found a patch description
that did nothing to explain to the reader how it avoids regressions
more than a little disturbing.

I also think the proposed behavior is wrong.

I don't think there's much benefit to be gained from continuing to
discuss this.  Consider it a single data point: I would be deeply
worried if this patch were applied without at least a clearer
description of the change in behavior.  Maybe I'm the only one!

Hope that helps,
Jonathan

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

[Felipe Contreras]

On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> Well, that's what we have now, and you want to preserve this "feature"
>> (aka bug), right?
>
> Nope.  I just don't want regressions, and found a patch description
> that did nothing to explain to the reader how it avoids regressions
> more than a little disturbing.

I see, so you don't have any specific case where this could cause
regressions, you are just saying it _might_ (like all patches).

> I also think the proposed behavior is wrong.

That's a different matter, lets see what others think.

-- 
Felipe Contreras

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

[Jonathan Nieder]

Felipe Contreras wrote:
> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> Nope.  I just don't want regressions, and found a patch description
>> that did nothing to explain to the reader how it avoids regressions
>> more than a little disturbing.
>
> I see, so you don't have any specific case where this could cause
> regressions, you are just saying it _might_ (like all patches).

Yes, exactly.  The commit log needs a description of the current
behavior, the intent behind the current code, the change the patch
makes, and the motivation behind that change, like all patches.
Despite the nice examples, it doesn't currently have that.

The patch description just raises more questions for the reader.  From
the description, one might imagine that this patch causes

	git fast-export <mark args> master

not to emit anything when another branch that has already been
exported is ahead of "master".  If I understand correctly (though
I haven't tested), this patch does cause

	git fast-export ^next master

not to emit anything when next is ahead of "master".  That doesn't
seem like progress.

I haven't reviewed the later patches in the series; maybe they fix
these things.  But in the long term it is much easier to understand
and maintain a patch series that does not introduce regressions in the
first place, and the context one might use to convincingly explain
that a patch is not introducing a regression turns out to be essential
for many other purposes as well.

Jonathan

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

[Felipe Contreras]

On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>> On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>
>>> Nope.  I just don't want regressions, and found a patch description
>>> that did nothing to explain to the reader how it avoids regressions
>>> more than a little disturbing.
>>
>> I see, so you don't have any specific case where this could cause
>> regressions, you are just saying it _might_ (like all patches).
>
> Yes, exactly.  The commit log needs a description of the current
> behavior, the intent behind the current code, the change the patch
> makes, and the motivation behind that change, like all patches.
> Despite the nice examples, it doesn't currently have that.
>
> The patch description just raises more questions for the reader.  From
> the description, one might imagine that this patch causes
>
>         git fast-export <mark args> master
>
> not to emit anything when another branch that has already been
> exported is ahead of "master".

This is already the case.

I don't see what part of my patch description would give you the idea
that this would change in any way how the objects are flagged, or how
get_revision() decides how to traverse them.

I clearly stated that this doesn't affect *the first* ref, which is
handled properly already; this patch affects *the rest* of the refs,
of which you have none in that command above.

> If I understand correctly (though
> I haven't tested), this patch does cause
>
>         git fast-export ^next master
>
> not to emit anything when next is ahead of "master".  That doesn't
> seem like progress.

Again, this is already the case RIGHT NOW.

And nothing in my description should give you an idea that anything
would change for this case because the 2nd ref (*the first* doesn't
get affected), is not marked as UNINTERESTING.

Not only you are not reading what is in the description, but I don't
think you understand what the code actually does, and how it behaves.

Let me give you some examples:

% git fast-export ^next next
reset refs/heads/next
from :0

% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea

The *only time* when this patch would have any effect is when you
specify more than *one ref*, and they both point to *exactly the same
object*.

Additionally, and this is something I just found out; when the are
pure refs (e.g. 'next'), and not refs to objects (e.g.
'next^{commit}').

In any other case; *there would be no change*.

After my patch:

% git fast-export ^next next
# nothing
% git fast-export ^next next^{commit}
# nothing
% git fast-export ^next next~0
# nothing
% git fast-export ^next next~1
# nothing
% git fast-export ^next next~2
# nothing
...
# you get the idea

> But in the long term it is much easier to understand
> and maintain a patch series that does not introduce regressions in the
> first place

It does not introduce regressions.

I don't think it's my job to explain to you how 'git fast-export'
works. Above you made too many assumptions of what get broken, when in
fact that's the current behavior already... maybe, just maybe, you are
also making wrong assumptions about this patch as well.

-- 
Felipe Contreras

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

[Jonathan Nieder]

Felipe Contreras wrote:

> I don't think it's my job to explain to you how 'git fast-export'
> works.

Actually, if you are submitting a patch for inclusion, it is your job
to explain to future readers what the patch does.  Yes, the reader
might not be deeply familiar with the part of fast-export you are
modifying.  It might have even been modified since then, by the time
the reader is looking at the change!

Sad but true.

Thanks,
Jonathan

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

[Felipe Contreras]

On Wed, Oct 31, 2012 at 2:08 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> I don't think it's my job to explain to you how 'git fast-export'
>> works.
>
> Actually, if you are submitting a patch for inclusion, it is your job
> to explain to future readers what the patch does.

That's already explained.

> Yes, the reader
> might not be deeply familiar with the part of fast-export you are
> modifying.

This has nothing to do with what you said. I'm literally explaining to
you how 'git fast-export' works in situations that are completely
orthogonal to this patch, because you are using wrong examples as
grounds to prevent this patch from being accepted. It's not my job to
explain to you that 'git fast-export' doesn't work this way, you have
a command line to type those commands and see for yourself if they do
what you think they do with a vanilla version of git. That's exactly
what I did, to make sure I'm not using assumptions as basis  for
arguing, it took me a few minutes.

That being said, if your problem is that it's not clear to people not
deeply familiar with that part of fast-export, this extra paragraph in
addition to the current commit message should do the trick:

---
The reason this happens is that before traversing the commits,
fast-export checks if any of the refs point to the same object, and
any duplicated ref gets added to a list in order to issue 'reset'
commands after the traversing. Unfortunately, it's not even checking
if the commit is flagged as UNINTERESTING. The fix of course, is to do
precisely that.
---

And to get that all had to do is ask: "Can you please add an
explanation of what this part of the code does? For the ones of us not
familiar with it".

Not; "This patch looks unsafe", "This patch makes Sally mad", "This
patch causes regressions", and so on.

But hey, at least we are not arguing about what is wrong with this
patch (or so I hope).

Cheers.

-- 
Felipe Contreras

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

[Jonathan Nieder]

Felipe Contreras wrote:

>                                                    It's not my job to
> explain to you that 'git fast-export' doesn't work this way, you have
> a command line to type those commands and see for yourself if they do
> what you think they do with a vanilla version of git. That's exactly
> what I did, to make sure I'm not using assumptions as basis  for
> arguing, it took me a few minutes.

Well no, when I run "git blame" 10 years down the line and do not
understand what your code is doing, it is not at all reasonable to
expect me to checkout the parent commit, get it to compile with a
modern toolchain, and type those commands for myself.

Instead, the commit message should be self-contained and explain what
the patch does.

That has multiple parts:

 - first, what the current behavior is

 - second, what the intent behind the current behavior is.  This is
   crucial information because presumably we want the change not to
   break that.

 - third, what change the patch makes

 - fourth, what the consequences of that are, in terms of new use
   cases that become possible and old use cases that become less
   convenient

 - fifth, optionally, how the need for this change was discovered
   (real-life usage, code inspection, or something else)

 - sixth, optionally, implementation considerations and alternate
   approaches that were discarded

If you run "git log", you'll see many good and bad examples to think
over and compare to this goal.  It's hard work to describe one's work
well in terms that other people can understand, but I think it can be
satisfying, and in any event, it's just as necessary as including
comments near confusing code.

Sincerely,
Jonathan

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

[Felipe Contreras]

On Wed, Oct 31, 2012 at 2:51 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>>                                                    It's not my job to
>> explain to you that 'git fast-export' doesn't work this way, you have
>> a command line to type those commands and see for yourself if they do
>> what you think they do with a vanilla version of git. That's exactly
>> what I did, to make sure I'm not using assumptions as basis  for
>> arguing, it took me a few minutes.
>
> Well no, when I run "git blame" 10 years down the line and do not
> understand what your code is doing, it is not at all reasonable to
> expect me to checkout the parent commit, get it to compile with a
> modern toolchain, and type those commands for myself.
>
> Instead, the commit message should be self-contained and explain what
> the patch does.
>
> That has multiple parts:
>
>  - first, what the current behavior is
>
>  - second, what the intent behind the current behavior is.  This is
>    crucial information because presumably we want the change not to
>    break that.
>
>  - third, what change the patch makes
>
>  - fourth, what the consequences of that are, in terms of new use
>    cases that become possible and old use cases that become less
>    convenient
>
>  - fifth, optionally, how the need for this change was discovered
>    (real-life usage, code inspection, or something else)
>
>  - sixth, optionally, implementation considerations and alternate
>    approaches that were discarded

I don't see any "Explain in detail what different commands do, even if
they are irrelevant to the patch in question because someone might
think they would get broken by this patch when in fact they wouldn't",
that might belong in the discussion, but not in the commit message,
and certainly not in the form of any entitlement.

Again, it's _your_ responsibility to make sure the commands you say
might get broken do actually work with your current git, it's not mine
to run them for you, even though that's exactly what I did, because
I'm interested in getting things correctly on record.

And FTR, since you removed it, here is what I proposed to add to the
commit message:

---
The reason this happens is that before traversing the commits,
fast-export checks if any of the refs point to the same object, and
any duplicated ref gets added to a list in order to issue 'reset'
commands after the traversing. Unfortunately, it's not even checking
if the commit is flagged as UNINTERESTING. The fix of course, is to do
precisely that.
---

With that, all the points above are tackled, except fourth, because
there aren't any.

Cheers.

-- 
Felipe Contreras

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

[Jonathan Nieder]

Hi again,

Felipe Contreras wrote:

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

So, the above description conveyed zero information, as you mentioned.

A clearer explanation would be the following:

	fast-export: don't emit "reset" command for negative refs

	When "git fast-export" encounters two refs on the commandline
	referring to the same commit, it exports the first during the usual
	commit walk and the second using a "reset" command in a final pass
	over extra_refs:

		$ git fast-export master next
		reset refs/heads/master
		commit refs/heads/master
		mark :1
		author Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
		committer Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
		data 17
		My first commit!

		reset refs/heads/next
		from :1

	Unfortunately the code to do this doesn't distinguish between positive
	and negative refs, producing confusing results:

		$ git fast-export ^master next
		reset refs/heads/next
		from :0

		$ git fast-export master ^next
		reset refs/heads/next
		from :0

	Use revs->cmdline instead of revs->pending to iterate over the rev-list
	arguments, checking the UNINTERESTING flag bit to distinguish between
	positive (master, --all, etc) and negative (next.., --not --all, etc)
	revs and avoid enqueueing negative revs in extra_revs.

	This does not affect revs that were excluded from the revision walk
	because pointed to by a mark, since those use the SHOWN bit on the
	commit object itself and not UNINTERESTING on the rev_cmdline_entry.

A patch meeting the above description would make perfect sense to me.
Except for the somewhat strange testcase, the patch I am replying to
would also be fine in the short term, as long as it had an analagous
description (i.e., with an appropriate replacement for the
second-to-last paragraph).

Thanks for your patience, and hoping that helps,
Jonathan

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

[Felipe Contreras]

Hi,

On Wed, Oct 31, 2012 at 1:57 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Felipe Contreras wrote:
>
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> So, the above description conveyed zero information, as you mentioned.

I meant, this, of course:
>> They have been marked as UNINTERESTING for a reason, lets respect that.
>
> This patch looks unsafe,

Which you know, because you received that message without the mistake.

> A clearer explanation would be the following:
>
>         fast-export: don't emit "reset" command for negative refs

What is a negative ref?

>         When "git fast-export" encounters two refs on the commandline

commandline?

Only two refs? How about four?

>         referring to the same commit, it exports the first during the usual
>         commit walk and the second using a "reset" command in a final pass
>         over extra_refs:

That is not exactly true: (next^{commit}).

>                 $ git fast-export master next
>                 reset refs/heads/master
>                 commit refs/heads/master
>                 mark :1
>                 author Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
>                 committer Jonathan Nieder <jrnieder@gmail.com> 1351644412 -0700
>                 data 17
>                 My first commit!
>
>                 reset refs/heads/next
>                 from :1

I don't think this example is good. Where does it say that 'next'
points to master? Using 'points-to-master' or a 'git branch stable
master' and using 'master stable'.

Even simpler would be to use 'git fast-export master master'; it would
show the same behavior.

>         Unfortunately the code to do this doesn't distinguish between positive
>         and negative refs, producing confusing results:
>
>                 $ git fast-export ^master next
>                 reset refs/heads/next
>                 from :0
>
>                 $ git fast-export master ^next
>                 reset refs/heads/next
>                 from :0
>
>         Use revs->cmdline instead of revs->pending to iterate over the rev-list
>         arguments, checking the UNINTERESTING flag bit to distinguish between
>         positive (master, --all, etc) and negative (next.., --not --all, etc)
>         revs and avoid enqueueing negative revs in extra_revs.

Use what? You mean, "To solve the problem, lets use".

But this is not correct, cmdline is not being used. Have you even
looked at the patch?

>         This does not affect revs that were excluded from the revision walk
>         because pointed to by a mark, since those use the SHOWN bit on the
>         commit object itself and not UNINTERESTING on the rev_cmdline_entry.

revs? You mean commits?

"excluded because point to by a mark"? Doesn't sound like proper
grammar. Maybe "excluded because they were pointed to by a mark".

And I don't see why this paragraph is needed at all. Why would the
reader think marks have anything to do with this? There's no mention
of marks before.

This might help you, or other people involved in the problem, but not
anybody else. Anything related to marks is completely orthogonal to
this patch, and there's no point in mentioning that.

-- 
Felipe Contreras

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

[Jonathan Nieder]

Felipe Contreras wrote:

> This might help you, or other people involved in the problem, but not
> anybody else.

Ok, I give up.  Bye.

Sometimes the author of some code and the right person to interact
with the development community by submitting and maintaining it are
not the same person.  Hopefully others more patient than we two can
pick up where we left off.

Thanks,
Jonathan

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

[Johannes Schindelin]

Hi Felipe,

On Tue, 30 Oct 2012, Felipe Contreras wrote:

> On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> > Felipe Contreras wrote:
> >> On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> >
> >>> and in the examples listed in the patch
> >>> description the changed behavior does not look like an improvement.
> >>
> >> I disagree.
> >>
> >> % git log master ^master
> >>
> >> What do you expect? Nothing.
> >
> > Yep.
> >
> >> % git fast-export master ^master
> >>
> >> What do you expect? Nothing.
> >
> > Nope.
> 
> That's _your_ opinion. I would like to see what others think.

If you wanted to prove that you can work with others without offending
them, I think that failed.

Ciao,
Johannes

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