Re: [PATCH v3 3/4] refs: selectively set prefix in the seek functions
- From
Karthik Nayak <karthik.188@gmail.com>
- Date
- Jul 16, 2025, 14:40 UTC
- Message-ID
- <CAOLa=ZTc2_g3+8MM8whW+eZAj-+36HNsnUFDoLgnN42ytFLwAw@mail.gmail.com>
- In-Reply-To
- <xmqqms95if8e.fsf@gitster.g>
Junio C Hamano <gitster@pobox.com> writes:
Show 21 quoted lines
> Karthik Nayak <karthik.188@gmail.com> writes: > >>> so after (true) seeking that does not have the SET_PREFIX flag on, >>> wouldn't our iterator-advance run through the end since it no longer >>> is aware of where to stop? >>> >> >> That's also right and that is indeed the intention. We're trying to make >> the actions more intentional. >> >> So if a user sets a 'prefix' for the iterator, all previous state of the >> iterator is reset. So, the same function for seeking an iterator should >> also have the same side-effect of resetting the previous state. > > Perhaps we have different definition of "previous state" in mind? > So let's imagine an iterator is walking over all branches (i.e. the > prefix is set to refs/heads/, to allow it to stop once it steps > outside refs/heads/ and moves over to refs/imerge). It starts > iterating and I see branches whose name sorts early in alphabetical > order. I tell it to seek to refs/heads/master and keep iterating. >
I get what you're saying and indeed that would be natural. Let me draw another example to draw the contrast.
Let's say a user is iterating with a prefix set to 'refs/heads/', this would iterate over all the refs with that prefix. But mid-way the user realizes that they only care about 'refs/heads/feature/' prefix and they ask the iterator to set that as the prefix.
In such a situation, the iterator seeks to 'refs/heads/feature/' and will only yield references with that prefix. In short, the previous prefix state was reset.
So to avoid the two scenarios:
1. Only seek the iterator but maintain prefix 2. Seek and set new prefix, loosing old prefix
Where one resets the prefix while the other doesn't. We make it explicit and say, whenever 'ref_iterator_seek' is called, any set prefix is reset. I do see the other way around too, where prefix isn't treated as previous state.
What I was trying to argue for, was that, there could be a situation like what you mentioned, where a user might want to retain a prefix, this should be an explicit requirement which not implemented in this series. So as of this series, you cannot set a prefix and then seek and expect to retain the prefix.
Show 9 quoted lines
> Wouldn't it be a lot more natural if it still stops iterating after > it finishes showing the last branch, iow, a ref in refs/heads/ > hierarchy? In other words, I am not sure why ... > >> There could be a usecase where we add support for keeping the prefix, >> while also seeking the iterator. That would be an explicit change > > ... that is the optional and unimplemented feature, not the other > way around. Is it just the ease of implementation?
This series did start out that way around, so ease of implementation isn't it. It was more of a side-effect of not clearing state. But I would be more comfortable if this wasn't a side-effect but rather a conscious choice with tests and adequate documentation.