Re: [PATCH v4 5/6] remote: die if branch is not found in repository

From: Jonathan Tan <jonathantanmy@google.com>
Date: Nov 16, 2021, 17:45 UTC
Message-ID: <20211116174553.2300029-1-jonathantanmy@google.com>
In-Reply-To: <kl6lo86ltc9a.fsf@chooglen-macbookpro.roam.corp.google.com>

Glen Choo <chooglen@google.com> writes:

Show 14 quoted lines

> > Couldn't we just iterate over the array (instead of making a hashmap)?
> > If speed is important, I think we could just sort the array and do a
> > binary search.
> 
> The primary reason I used a hashmap is to be consistent with struct
> remote (which also uses a hashmap). One possible argument in your favor
> is that remotes are often looked up by name often (and justify the
> hashmap), whereas branches are not looked up by name as often (and don't
> justify a hashmap).
> 
> I say _justify_, but I don't see significant drawbacks to using a
> hashmap here. I suspect that there is an advantage to binary search that
> you haven't made explicit yet? Could you share your thought process to
> help inform the decision?

The main drawback is that branches are now stored in 2 ways - as the "branches" array in struct remote_state and as this new "branches_hash". I think we should avoid storing the same data twice unless we really need to, and I don't think there is a need here.

As for hashmap vs array (say, if we were thinking of removing the array and putting in a hashmap instead), I would still prefer the array (sorted if needed) just for the simplicity, but I wouldn't feel as strongly about this since there is no duplication here.