Fix deleting a remote tag when a remote branch with the same name exists, or vice versa

[stefanhaller]

Nothing to add to the PR title here.

Fixes #5072.

[codacy-production]

Coverage summary from Codacy

See diff coverage on Codacy

Coverage variation	Diff coverage
Report missing for b57be9e1	✅ 100.00%

Coverage variation details

	Coverable lines	Covered lines	Coverage
Common ancestor commit (b57be9e)	Report Missing	Report Missing	Report Missing
Head commit (11a6a73)	59205	51492	86.97%

Coverage variation is the difference between the coverage for the head and common ancestor commits of the pull request branch: <coverage of head commit> - <coverage of common ancestor commit>

Diff coverage details

	Coverable lines	Covered lines	Diff coverage
Pull request (#5075)	78	78	100.00%

Diff coverage is the percentage of lines that are covered by tests out of the coverable lines that the pull request added or modified: <covered lines added or modified>/<coverable lines added or modified> * 100%

See your quality gate settings    Change summary preferences

Footnotes

1. Codacy didn't receive coverage data for the commit, or there was an error processing the received data. Check your integration for errors and validate that your coverage setup is correct. ↩

[miduddin]

I can confirm this works, but I have some comments about the code. Thanks!

[miduddin]

I noticed that now we have two lo.Map calls, one here and one on the caller of DeleteRemoteBranch. Is it preferable to have these two joined together?

Also, the caller works with the type models.RemoteBranch, there is a FullRefName() method that returns "refs/remotes/<remote_name>/<branch_name>" which unfortunately can't be used for this, but on the tag side that works with models.Tag the FullRefName() can be used. It's a bit weird from the consistency standpoint so I don't know if we want to use them without some extra work, but I figured to just mention it.

[stefanhaller]

Thanks for the careful review, both are good observations.

1. No, I think it's fine this way; the fact that we have two lo.Map calls doesn't bother me, and it helps with having clearer APIs. The alternatives would be: a) pass an instance of models.RemoteBranch to RemoteCommands.DeleteRemoteBranch, so that it can do both transformations inside; that's not possible, because git_commands shouldn't have a dependency on the models. Or b) require the caller to pass in the full ref name, but callers shouldn't have to have this knowledge, it's a clearer API to only pass in the name.

2. We can't use FullRefName() anyway because we only want to pass in the bare names, but even if we could, we shouldn't use FullRefName() here. It's the full ref name from the perspective of the local repo, which is not what we need here. We need it from the perspective of the remote repo, and it's the responsibility of the DeleteRemoteTag/DeleteRemoteBranch functions to know how to construct these. It's true that for tags both are the same, but that's only because git doesn't have a concept of remote tags like it does for branches. You could imagine that a future version of git adds this, and then we might change Tag.FullRefName to return the local ref for a remote tag, which would break the DeleteRemoteTag function if it were to use it.

[miduddin]

I see, I do agree that the current implementation is good/clear enough as is, but I still wonder if there is a case to be made for putting the "refs/..." construction into one place since it has specific meaning. I don't have enough knowledge on the project structure though, so I'll leave the decisions to you, please don't treat this comment as a blocker.

[stefanhaller]

I'd say that the current place (inside of DeleteRemoteTag/DeleteRemoteBranch) is exactly the right place. (And I don't feel we need an abstraction for this, like a FullRefFromTheRemotesPerspective or some such; that feels like overkill to me.)

[miduddin]

Haha no, I was thinking along the lines of using models.Branch instead of models.RemoteBranch which would make it more aligned with what the actual git command accepts (a full local refname), but I have no idea how to make it a good fit since we are working with remote branch view in the first place.

[stefanhaller]

Oh, that would feel weird to me, and would make it rather harder to understand than easier.

Anyway, thanks again for the review!