From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:09
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.
quoted hunk
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 <redacted>
---
builtin/fast-export.c | 7 ++++---
t/t9350-fast-export.sh | 6 ++++++
2 files changed, 10 insertions(+), 3 deletions(-)
@@ -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;+}elsecommit->util=full_name;}}
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:09
On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder [off-list ref] wrote:
quoted
quoted
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.
quoted
% 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
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:09
On Tue, Oct 30, 2012 at 10:45 PM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:09
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
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:09
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:09
Felipe Contreras wrote:
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder [off-list ref] wrote:
quoted
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:09
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 [off-list ref] 1351644412 -0700
committer Jonathan Nieder [off-list ref] 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
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:09
On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder [off-list ref] wrote:
quoted
quoted
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:09
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
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:09
Hi,
On Wed, Oct 31, 2012 at 1:57 AM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
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:
quoted
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 [off-list ref] 1351644412 -0700
committer Jonathan Nieder [off-list ref] 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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:09
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
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:09
On Wed, Oct 31, 2012 at 2:08 AM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
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
From: Jonathan Nieder <hidden> Date: 2016-06-15 22:55:09
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
From: Felipe Contreras <hidden> Date: 2016-06-15 22:55:09
On Wed, Oct 31, 2012 at 2:51 AM, Jonathan Nieder [off-list ref] wrote:
Felipe Contreras wrote:
quoted
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