The JGitEnvironmentRepository has the method called containsBranch that is checking whether label is branch or not (line 743). Imaging we have

label = foo branch = feature/foo tag = foo

Given that it returns true because ref name ends with /foo.

Expected: Returns false.

Comment From: ryanjbaxter

See the section on labels here https://docs.spring.io/spring-cloud-config/docs/current/reference/html/#_git_backend

Comment From: ojecborec

I'm very well aware of following statement from documentation

If the git branch or tag name contains a slash (/), then the label in the HTTP URL should instead be specified with the special string (_) (to avoid ambiguity with other URL paths).

But this is different. It has nothing to do with the HTTP URL.

Comment From: ojecborec

How to reproduce

  1. Given branch=feature/foo tag=foo

  2. Send following request to Config Server /application/default/foo

  3. JGitEnvironmentRepository.containsBranch returns true because refs/remotes/origin/feature/foo ends with /foo

  4. checkout.call(), line 468 throws RefNotFoundException. Ref origin/foo cannot be resolved. Which is true as this branch does not exist.

Comment From: ryanjbaxter

I think the problem here is the ambiguity of label. If you have a branch and a tag with the same name how do we know which one we should be using? This problem is a subset of that problem where the branch name ends with the tag. I am not sure what the best way to solve the problem is. The only thing I can come up with is adding something to the label that identifies whether you are specifically talking about a branch or a tag. Or we might be able to not use endsWith and force the label to be the complete branch name when specifying a label.

This code has been this way for 8 years and this is the first time we have seen this issue, so while its an issue it appears to be rare.

Comment From: ojecborec

If you have a branch and a tag with the same name how do we know which one we should be using?

In this case yes. We could say that branch has higher priority and request would not fail. However this is not the case. Branch name is different to tag one. The problem is using endsWith method and / as a separator. One possible solution is to normalize ref name before performing full match, i.e.

refs/heads/main -> main
refs/remotes/origin/feature/foo -> feature/foo

Or maybe the containsBranch method can be replaced by calling @Bean annotated component allowing people to overwrite default implementation.

This code has been this way for 8 years and this is the first time we have seen this issue, so while its an issue it appears to be rare.

Maybe people got used to it and found workarounds. This does not mean that Config Server should not support using / character to create a hierarchical name scheme.

Comment From: ryanjbaxter

One possible solution is to normalize ref name before performing full match

I don't think thats possible because it assumes the prefixes of the branch name are known, and thats not the case, that prefix can be anything technically. Yes we know there may be refs/head/ or refs/remotes/origin but there could be something like refs/remotes/my/remote/stuff/feature/foo.

Or maybe the containsBranch method can be replaced by calling @Bean annotated component allowing people to overwrite default implementation.

We could probably do that without much trouble I think. Would you be interested in making a PR?

Comment From: ojecborec

I’m taking vacation but when I come back I’ll have a look.

Comment From: woshikid

I spend some time on this issue and don't think this is a bug.

The method containsBranch in class JGitEnvironmentRepository do have small problem dealing with branch name contains slashes (/). The return of this method may be wrong. But the wrong returns will not cause any problem.

First, there is nothing to do with tags. containsBranch only list branches, not tags. Please see the code below.

private boolean containsBranch(Git git, String label, ListMode listMode) throws GitAPIException {
    ListBranchCommand command = git.branchList();
    if (listMode != null) {
        command.setListMode(listMode);
    }
    List<Ref> branches = command.call(); // <--- here, the result list contains only branches, no tags
    for (Ref ref : branches) {
        if (ref.getName().endsWith("/" + label)) {
            return true;
        }
    }
    return false;
}

Second, in this case containsBranch is only used in shouldTrack method. This method determines whether the remote branch should be tracked. When the branch does not exist, whether it needs to be tracked or not, the checkout sure to throw RefNotFoundException. Please see the code below.

private Ref checkout(Git git, String label) throws GitAPIException {
    CheckoutCommand checkout = git.checkout();
    if (shouldTrack(git, label)) { //  <--- containsBranch is used here
        trackBranch(git, checkout, label); // works for remote branches
    }
    else {
        checkout.setName(label); // works for tags and local branches
    }
    return checkout.call(); // always throw when branch not exists
}

So actually there's nothing to worry about, everything works as designed.

Comment From: ojecborec

Just so we understand each other. I have valid branch=feature/foo and tag=foo. Has Spring Cloud Config been designed to fail when I use valid tag name? Has containsBranch method been designed to return true even if repository does not contain foo branch?

Comment From: ojecborec

But the wrong returns will not cause any problem

Sure it does. There would be no RefNotFoundException exception, should it return false.

Comment From: woshikid

I have some misunderstanding on this issue. Let me look into it later.

Comment From: woshikid

You are right. This bug do causes problem when using tags. I agree with you.

As you mentioned, the key point to this bug is the code ref.getName().endsWith("/" + label). and I also think normalize ref name is a good idea. As @ryanjbaxter said

there could be something like refs/remotes/my/remote/stuff/feature/foo

yes, git do allow / in remote names, so we can't separate branch names from remote names.

But suddenly I realized there is no way in config server to change the remote names. The remote name must be origin. And I found hard coded origin in JGitEnvironmentRepository

private static final String LOCAL_BRANCH_REF_PREFIX = "refs/remotes/origin/";

and

String originUrl = git.getRepository().getConfig().getString("remote", "origin", "url");

and

fetch.setRemote("origin");

and more ...

So it is possible to normalize the ref name to solve the problem.

Maybe we can change to code into 

ref.getName().matches("refs/(heads|remotes/origin)/" + label)

or

ref.getName().equals("refs/heads/" + label) || ref.getName().equals("refs/remotes/origin/" + label)

Do you think this will solve the problem?

Comment From: ryanjbaxter

Like I said, i am not sure that solves the problem because the branch names can be anything really.

Comment From: ojecborec

In my case exact match solution would work.

@ryanjbaxter Branch is not really an issue. And like @woshikid’s said origin is hardcoded all over the place so at the moment it cannot be anything else but origin.

Comment From: ryanjbaxter

@woshikid create a PR and I will take a look

Comment From: ojecborec

Saying that I prefer non-regex solution for performance and security reasons as well.

Comment From: woshikid

@ryanjbaxter I've made a PR #2147 , please take a look