Re: Bad refspec messes up bundle.

From: Luciano Joublanc <ljoublanc@dinogroup.eu>
Date: Mar 31, 2018, 08:50 UTC
Message-ID: <CAO+-ZX-DvjsOnpvfPuLkx2w2cR5FDb6Ww8xEyuZHMmC57=b2yQ@mail.gmail.com>
In-Reply-To: <nycvar.QRO.7.76.6.1803301102430.5026@qfpub.tvgsbejvaqbjf.bet>

Hi Johannes,

With such a comprehensive reply, I would feel guilty not making a contribution now :) Be forewarned though, It's been about ten years since I wrote anything in `C`!

I've cloned the `maint` branch, built the project, and added the test as you suggested - it's failing as expected.

I'm somewhat confused though. I think it's m limited understanding of 'ref' and 'commit'.

On 30 March 2018 at 11:20, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

Show 5 quoted lines

>
>
>   However, this would be incorrect, as the flags are stored with the
>   *commit*, not with the ref. So if two refs point to the same commit,
>   that new code would skip the second one by mistake!

Isn't that the point here? to deduplicate commits? My limited understanding is that at a 'ref' is like an alias or pointer to a commit.

Show 6 quoted lines

>
>   By the way, this makes me think that there is another very real bug in
>   the bundle code, in the part I showed above. Suppose you have a `master`
>   and a `next` ref, and both point at the same commit, then you would want
>   `git bundle create next.bundle master..next` to list `next`, don't you
>   think?

Doesn't this contradict what you just said, that we don't want to skip refs with the same commit #?

In fact, if you look in the calling function, there is a ` object_array_remove_duplicates(&revs.pending);` Which to the best of my understanding removes duplicate refs (not commits). However, I suspect this doesn't cover the `--all` case as it's a switch rather than a revspec? Would that be right?

Show 8 quoted lines

>
>
> - most likely, the best way to avoid duplicate refs entries is to use the
>   actual ref name and put it into a hash set. Luckily, we do have code
>   for this, and examples how to use it, too. See e.g. fc65b00da7e
>   (merge-recursive: change current file dir string_lists to hashmap,
>   2017-09-07). So you would define something like
>

Separately, if I do end up including the hashmap code, it should be refactored out into it's own file, right?

Thanks again,

Luciano

-- 
This message is intended only for the personal and confidential use of the 
designated recipient(s) named above.  If you are not the intended recipient 
of this message you are hereby notified that any review, dissemination, 
distribution or copying of this message is strictly prohibited.  This 
communication is for information purposes only and should not be regarded 
as an offer to sell or as a solicitation of an offer to buy any financial 
product, an official confirmation of any transaction, or as an official 
statement of the Dinosaur Group.  Email transmission cannot be guaranteed 
to be secure or error-free.  Therefore, we do not represent that this 
information is complete or accurate and it should not be relied upon as 
such.  All information is subject to change without notice.