From: Junio C Hamano Date: Sun, 02 Jun 2013 21:02:41 GMT Subject: Re: [PATCH v2 11/25] object_array_remove_duplicates(): rewrite to reduce copying Message-ID: <7vy5asmdji.fsf@alter.siamese.dyndns.org> In-Reply-To: <51A7C146.6090605@alum.mit.edu> Michael Haggerty writes: >> The only caller of remove-duplicates is bundle.c, which gets many >> starting points and end points from the command line and tries to be >> nice by removing obvious duplicates, e.g. >> >> git bundle create t.bundle master master >> >> but I think its logic of deduping is wrong. It runs dwim_ref() on >> the incoming refs after the remove-duplicates call, so >> >> git bundle create t.bundle master heads/mater >> >> will end up with two copies of refs/heads/master. To fix it, the >> code must dedup the result of running dwim_ref(), and at that point, >> there is no reason to call object_array_remove_duplicates(). > > That sounds reasonable. > > I poked around this code a bit to understand what is going on, and it > occurred to me that the object_array can include both positive and > negative references, right? And yet object_array_remove_duplicates() > only considers names, not flags. So it seems to me that if the deduping > code would see the same reference twice, once positive and once > negative, then it would throw an arbitrary one of them out, which would > be wrong. > > But I couldn't provoke this situation, so perhaps setup_revisions() > already specially treats combinations like "master ^master"? (If that's > true then why? and wouldn't it get confused by "master ^heads/master"?) With "git bundle create t.bundle ^master master", you see two entries in revs.pending.objects[] but they are the same object and is already marked as uninteresting, so you will not see 'master' in the result. This parsing loop predates the more recent revs->cmdline mechanism, that treats these two command line arguments as separate entities, so that we can more reliably tell what the real end-user input is.