fix: WikiTagRename failure after so many tags

[trev-dev]

It appears as though we may be killing a reference to self.collection[o:old_tag] prematurely, thus causing subsequent tags to be renamed. Prior to this patch I could only rename tags 2 wiki files at a time.

This PR addresses this by creating a deep copy of the list of tag/file relationships to iterate off of. We then "pop" off the front of collection[a:old_tag] every iteration until we are presumably done. If collection[a:old_tag] is empty, we then remove it from the collection.

[lervag]

It appears as though we may be killing a reference to self.collection[o:old_tag] prematurely, thus causing subsequent tags to be renamed.

Ah, yes, good catch! However, I'm not sure this is quite right either. That is:

  for [l:file, l:lnum] in l:tagpages
    if s:update_tag_in_wiki(l:file, l:lnum, a:old_tag, a:new_tag)
      call self.add(a:new_tag, l:file, l:lnum)
      call remove(self.collection[a:old_tag], 0)

tagpages is the list from which we are now "popping". However, the list element we are popping is not guaranteed to be related to the current element that we are processing. Thus, the only reason for popping here is to check if the list is empty afterwards. Then, perhaps, it would be better to simply count? That is:

  let l:n = 0
  for [l:file, l:lnum] in l:tagpages
    if s:update_tag_in_wiki(l:file, l:lnum, a:old_tag, a:new_tag)
      call self.add(a:new_tag, l:file, l:lnum)
      let l:n += 1
      let l:num_files += 1
    endif
  endfor

  if l:n == len(l:tagpages)
    call remove(self.collection, a:old_tag)
  endif

By the way, I've so far avoided the method syntax:

  self.collection->get(a:old_tag)

The reason has been that it is new and I have specified a compatibility requirement (Vim (>= 8.1) or neovim (>= 0.5)). I think adding this syntax means we need to tighten the requirement. Something like neovim: v0.6.0 and Vim v8.1.1803. Perhaps I'll just do that, but at least it needs attention before doing it.

[trev-dev]

tagpages is the list from which we are now "popping". However, the list element we are popping is not guaranteed to be related to the current element that we are processing. Thus, the only reason for popping here is to check if the list is empty afterwards. Then, perhaps, it would be better to simply count? That is:

Actually, we're popping the cache directly :) - l:tagpages is a deep clone of the related property value.

24fcd43#diff-cd765ef8a9fc7bf69b406ef30f8ab7d371d11124401b05d82343d39400ebcf8dR426

  let l:tagpages = deepcopy(self.collection[a:old_tag])

  " We already know where the tag is in the file, thanks to the cache
  for [l:file, l:lnum] in l:tagpages
    if s:update_tag_in_wiki(l:file, l:lnum, a:old_tag, a:new_tag)
      call self.add(a:new_tag, l:file, l:lnum)
      call remove(self.collection[a:old_tag], 0)
      let l:num_files += 1
    endif
  endfor

By doing this, we have an iterator that won't suddenly lose keys, that should be a 1:1 copy of self.collection[a:old_tag]. If we pop the cache at 0, the following key in the iterator will always bey key 0 in the cache, so there is a direct relationship.

What we should probably do is guard against unexpected failures in the iterator due to maybe some user configuration problem. Actually, I can't imagine what kind of user config would do this. Maybe it should just error if continuity gets lost.

The reason has been that it is new and I have specified a compatibility requirement (Vim (>= 8.1) or neovim (>= 0.5)). I think adding this syntax means we need to tighten the requirement. Something like neovim: v0.6.0 and Vim v8.1.1803. Perhaps I'll just do that, but at least it needs attention before doing it.

I will just use a nested function call then :)

[lervag]

tagpages is the list from which we are now "popping". However, the list element we are popping is not guaranteed to be related to the current element that we are processing. Thus, the only reason for popping here is to check if the list is empty afterwards. Then, perhaps, it would be better to simply count? That is:

Actually, we're popping the cache directly :) - l:tagpages is a deep clone of the related property value.

Hmm. I'm not properly convinced. Let's simplify:

  let tagpages = deepcopy(self.collection[a:old_tag]) " [1, 2, 3]

  for l:x in l:tagpages
    if l:x == 2
      let l:y = remove(self.collection[a:old_tag], 0)
      " now
      "   y = 2, but
      "   x = 2
    endif
  endfor

  " Also: self.collection[a:old_tag] = [2, 3]

However, I believe the idea here is that the loop should update all old_tag locations to the new tag. But the old_tag was renamed and after this update, there should be no old_tag. Thus, it should be safe to always remove old_tag from the cache/collection. So, something like this should work:

  let l:tagpages = deepcopy(self.collection[a:old_tag])

  for [l:file, l:lnum] in l:tagpages
    if s:update_tag_in_wiki(l:file, l:lnum, a:old_tag, a:new_tag)
      call self.add(a:new_tag, l:file, l:lnum)
      let l:num_files += 1
    endif
  endfor

  call remove(self.collection, a:old_tag)

or even

  let l:tagpages = remove(self.collection, a:old_tag)

  for [l:file, l:lnum] in l:tagpages
    if s:update_tag_in_wiki(l:file, l:lnum, a:old_tag, a:new_tag)
      call self.add(a:new_tag, l:file, l:lnum)
      let l:num_files += 1
    endif
  endfor

By doing this, we have an iterator that won't suddenly lose keys

I agree that the old code was buggy, but I think perhaps my suggestion above is correct.

[trev-dev]

let tagpages = deepcopy(self.collection[a:old_tag]) " [1, 2, 3]

for l:x in l:tagpages
  if l:x == 2
    let l:y = remove(self.collection[a:old_tag], 0)
    " now
    "   y = 2, but
    "   x = 2
  endif
endfor

" Also: self.collection[a:old_tag] = [2, 3]

My concept hinges on the idea that the loop will always do the same thing on every iteration without corner cases, so yes, throwing an if block in there will mess things up.

  let l:tagpages = remove(self.collection, a:old_tag)

  for [l:file, l:lnum] in l:tagpages
    if s:update_tag_in_wiki(l:file, l:lnum, a:old_tag, a:new_tag)
      call self.add(a:new_tag, l:file, l:lnum)
      let l:num_files += 1
    endif
  endfor

This is indeed the most simple way to do it and it avoids the need to worry about an iterator disappearing, or doubling up on memory 🥳

[trev-dev]

After applying your simplification I get a maybe false negative about the old key being missing in my tag cache, not sure what's up with that but it's working.

[lervag]

After applying your simplification I get a maybe false negative about the old key being missing in my tag cache, not sure what's up with that but it's working.

Hmm... it seems you are saying that "there is a problem" but also "it's working". I'm not sure how to understand this. Should I merge this or should I wait?

[trev-dev]

After applying your simplification I get a maybe false negative about the old key being missing in my tag cache, not sure what's up with that but it's working.

Hmm... it seems you are saying that "there is a problem" but also "it's working". I'm not sure how to understand this. Should I merge this or should I wait?

It works, it's just giving me a nonsense error message the first time I rename something:

If I were you, I would merge it. I can raise a separate issue/PR after testing to see if it's just a "me" problem.

[lervag]

It works, it's just giving me a nonsense error message the first time I rename something

Yes, that's because of this:

https://github.com/trev-dev/wiki.vim/blob/4b71bd24a85208b5954d318a3bac4591dc9c7a43/autoload/wiki/tags.vim#L391-L393

I.e., it is not really an error message. It is a warning due to an "unexpected" event where the old tag was not found in the tags collection. However, it is not unexpected if we never initialized the cache. So I've fixed this before I merged.