Re: [PATCH v7 p2 1/2] fast-export: don't handle uninteresting refs
Felipe Contreras felipe.contre...@gmail.com writes: On Thu, Nov 29, 2012 at 2:16 AM, Max Horn post...@quendi.de wrote: On 28.11.2012, at 23:23, Felipe Contreras wrote: They have been marked as UNINTERESTING for a reason, lets respect that. Currently the first ref is handled properly, but not the rest: % git fast-export master ^uninteresting ^foo ^bar All these refs are assumed to point to the same object, right? I think it would be better if the commit message stated that explicitly. To make up for the lost space, you could then get rid of one of the four refs, I think three are sufficient to drive the message home ;-). Yeah, they point to the same object. Do you want me to amend the log message of that commit to clarify this? snip ... It's actually revs.cmdline, I typed the wrong one. ... So I think it's good. Wait. I at least read two points above you said what you wrote in the commit was not corrrect and misleading to later readers. And then I hear it's good. Which one? Are you merely saying that it is easily fixable to become good? If so, what do you want to do with these not-so-good part? If you want to ask me to amend, that is fine, but do so in a more explicit way, not in a message at the tail of long thread that is not even CC'ed to me. Of course, a proper re-roll like everybody else does is just fine. Thanks. -- 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 v7 p2 1/2] fast-export: don't handle uninteresting refs
On Thu, Nov 29, 2012 at 2:16 AM, Max Horn post...@quendi.de wrote: On 28.11.2012, at 23:23, Felipe Contreras wrote: They have been marked as UNINTERESTING for a reason, lets respect that. Currently the first ref is handled properly, but not the rest: % git fast-export master ^uninteresting ^foo ^bar All these refs are assumed to point to the same object, right? I think it would be better if the commit message stated that explicitly. To make up for the lost space, you could then get rid of one of the four refs, I think three are sufficient to drive the message home ;-). Yeah, they point to the same object. snip 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. Hm... So this might be me being a stupid n00b (I am not yet that familiar with the internal rep of things in git and all...)... but I found the precisely that par very confusing, because right afterwards, you say: Yeah, the next part was added afterwards. However, in order to do it properly we need to get the UNINTERESTING flag from the command line ref, not from the commit object. So this sounds like you are saying we do *precisely* that, except we don't, because it is more complicated, so we actually don't do this *precisely*, just manner of speaking... Well, we do check fro the UNINTERESTING flag, but on the ref, not on the commit. Some details here are beyond my knowledge, I am afraid, so I have to resort to guess: In particular it is not clear to me why the however part pops up: Reading it makes it sound as if the commit object also carries an UNINTERESTING flag, but we can't use it because of some reason (perhaps it doesn't have the semantics we need?), so we have to look at revs.pending instead. Right? Wrong? Or is it because the commit objects actually do *not* carry the UNINTERESTING bits, hence we need to look at revs.pending. Or is it due to yet another reason? It's actually revs.cmdline, I typed the wrong one. If you have two refs pointing to the same object, and you do 'one ^two', the object (e.g. 8c7a786) will get the UNINTERESTING flag, but that doesn't tell us anything about the ref being a positive or a negative one, and revs.pending only has the object flags. On the other hand revs.cmdline does have the flags for the refs. Does that explain it? Anyway, other than these nitpicky questions, this whole thing looks very logical to me, description and code alike. I also played around with tons of fast-export invocations, with and without this patch, and it seems to do what the description says. Finally, I went to the various long threads discussion prior versions of this patch, in particular those starting at http://thread.gmane.org/gmane.comp.version-control.git/208725 and http://thread.gmane.org/gmane.comp.version-control.git/209355/focus=209370 These contained some concerns. Sadly, several of those discussions ultimately degenerated into not-so-pleasant exchanges :-(, and my impression is that as a result some people are not so inclined to comment on these patches anymore at all. Which is a pity :-(. But overall, it seems this patch makes nothing worse, but fixes some things; and it is simple enough that it shouldn't make future improvements harder. So *I* at least am quite happy with this, it helps me! My impression is that Felipe's latest patch addresses most concerns people raised by means of an improved description. I couldn't find any in those threads that I feel still applies -- but of course those people should speak for themselves, I am simply afraid they don't want to be part of this anymore :-(. Indeed. For all the concerns given I made a response to how that either is not true, or doesn't really matter, and in the case of the latter, I asked for examples where it would matter, only to receive nothing. For whatever reason involved people are not responding, not a single valid concern has been raised and remained. So I think it's good. 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 v7 p2 1/2] fast-export: don't handle uninteresting refs
On 28.11.2012, at 23:23, Felipe Contreras wrote:
They have been marked as UNINTERESTING for a reason, lets respect that.
Currently the first ref is handled properly, but not the rest:
% git fast-export master ^uninteresting ^foo ^bar
All these refs are assumed to point to the same object, right? I think it would
be better if the commit message stated that explicitly. To make up for the lost
space, you could then get rid of one of the four refs, I think three are
sufficient to drive the message home ;-).
snip
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.
Hm... So this might be me being a stupid n00b (I am not yet that familiar with
the internal rep of things in git and all...)... but I found the precisely
that par very confusing, because right afterwards, you say:
However, in order to do it properly we need to get the UNINTERESTING flag
from the command line ref, not from the commit object.
So this sounds like you are saying we do *precisely* that, except we don't,
because it is more complicated, so we actually don't do this *precisely*, just
manner of speaking...
Some details here are beyond my knowledge, I am afraid, so I have to resort to
guess: In particular it is not clear to me why the however part pops up:
Reading it makes it sound as if the commit object also carries an UNINTERESTING
flag, but we can't use it because of some reason (perhaps it doesn't have the
semantics we need?), so we have to look at revs.pending instead. Right? Wrong?
Or is it because the commit objects actually do *not* carry the UNINTERESTING
bits, hence we need to look at revs.pending. Or is it due to yet another reason?
I would find it helpful if that could be clarified. E.g. like so:
The fix is to add such a check. However, we cannot just use the UNINTERESTING
flag of the commit object, because INSERT-REASON.
or
The fix is to add such a check. However, the commit object does not contain
the UNINTERESTING flag directly.
or something.
Anyway, other than these nitpicky questions, this whole thing looks very
logical to me, description and code alike. I also played around with tons of
fast-export invocations, with and without this patch, and it seems to do what
the description says. Finally, I went to the various long threads discussion
prior versions of this patch, in particular those starting at
http://thread.gmane.org/gmane.comp.version-control.git/208725
and
http://thread.gmane.org/gmane.comp.version-control.git/209355/focus=209370
These contained some concerns. Sadly, several of those discussions ultimately
degenerated into not-so-pleasant exchanges :-(, and my impression is that as a
result some people are not so inclined to comment on these patches anymore at
all. Which is a pity :-(. But overall, it seems this patch makes nothing worse,
but fixes some things; and it is simple enough that it shouldn't make future
improvements harder.
So *I* at least am quite happy with this, it helps me! My impression is that
Felipe's latest patch addresses most concerns people raised by means of an
improved description. I couldn't find any in those threads that I feel still
applies -- but of course those people should speak for themselves, I am simply
afraid they don't want to be part of this anymore :-(.
Still, for what little it might be worth, I think this patch is good and a real
improvement. I hope it can be merged soon.
Cheers,
Max
Fortunately we
can simply use revs.pending, which contains all the information we need
for get_tags_and_duplicates(), plus the ref flag. This way the rest of
the positive refs will remain untouched; it's only the negative ones
that change in behavior.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
builtin/fast-export.c | 11 +++
t/t5801-remote-helpers.sh | 8
t/t9350-fast-export.sh| 30 ++
3 files changed, 45 insertions(+), 4 deletions(-)
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 191936c..2547e6c 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -474,18 +474,21 @@ static void handle_tag(const char *name, struct tag
*tag)
(int)message_size, (int)message_size, message ? message : );
}
-static void get_tags_and_duplicates(struct object_array *pending,
+static void get_tags_and_duplicates(struct rev_cmdline_info *info,
struct string_list *extra_refs)
{
struct tag *tag;
int i;
- for (i = 0; i pending-nr; i++) {
- struct object_array_entry *e = pending-objects + i;
+ for (i = 0; i info-nr; i++) {
+
