Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
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.contre...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
(again to the mailing list) On Tue, Oct 30, 2012 at 7:59 PM, Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Hi Felipe, On Tue, 30 Oct 2012, Felipe Contreras wrote: On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 8:01 PM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: On Tue, Oct 30, 2012 at 7:47 PM, Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 10:45 PM, Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Felipe Contreras wrote: On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
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 jrnie...@gmail.com 1351644412 -0700 committer Jonathan Nieder jrnie...@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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
On Wed, Oct 31, 2012 at 12:55 AM, Jonathan Nieder jrnie...@gmail.com wrote:
Felipe Contreras wrote:
On Tue, Oct 30, 2012 at 11:07 PM, Jonathan Nieder jrnie...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
Hi,
On Wed, Oct 31, 2012 at 1:57 AM, Jonathan Nieder jrnie...@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 jrnie...@gmail.com 1351644412 -0700
committer Jonathan Nieder jrnie...@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
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/4] fast-export: don't handle uninteresting refs
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 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
