Fix 404 error in files view after deleting the last file in a repository

Here's the logic:

1. In TreeController, require_non_empty_project will prevent show from being called if the project is empty. That means all calls to show will be guaranteed to have at least 1 commit.
2. If the ref name is not valid, then return a 404. This ensures that at least the controller is looking at a valid branch/tag/SHA ID.
3. This leaves a number of cases:

3a. Valid ref, valid directory
3b. Valid ref, valid filename
3c. Valid ref, invalid path
3d. Valid ref, no files

Case 3a: The tree will not be empty? and will pass through the whole function.

Case 3b: The tree will be empty? but the blob_at will resolve properly and trigger a redirect to the file.

Case 3c: In this case, a path is given. Return 404 if it cannot be resolved neither as a tree nor a blob.

Case 3d: In this case, no path is given. If the tree is empty, this means it's an empty branch and just fall through.

Example broken branch: https://gitlab.com/gitlab-org/gitlab-test/tree/empty-branch

Closes #1362 (closed)

mentioned in issue #1362 (closed)

Added 1 commit:

• 9328562d - Check for valid branch or tag names

Added 19 commits:

• 9328562d...57f9a1cc - 18 commits from branch gitlab-org:master
• 31f31a43 - Fix 404 error in files view after deleting the last file in a repository

Added 1 commit:

• c2f2c693 - if ! -> unless

Added 1 commit:

• 96517d70 - Fix 404 error in files view after deleting the last file in a repository

mentioned in issue #1826 (closed)

Added 1 commit:

• c5bc9889 - Simplify valid check

Added 1 commit:

• 456450ab - Fix 404 error in files view after deleting the last file in a repository

Added 1 commit:

• 7e8c78d7 - Fix 404 error in files view after deleting the last file in a repository

Worked for me!

Added 1 commit:

• cf6d5adb - Add test

Added 1 commit:

• f7dad209 - Fix 404 error in files view after deleting the last file in a repository

Added 1 commit:

• b1d0208a - Bump SHA of gitlab-test commit for empty branch

Added 1 commit:

• a00c4f35 - Fix 404 error in files view after deleting the last file in a repository

Ok, this one is ready with tests now.

Reassigned to @rspeicher

Why was this one necessary?

The merge was bringing in the new empty-branch, which is not present in https://gitlab.com/gitlab-org/gitlab-test-fork. I could push the branch there too and revert this change.

Whichever you think is cleaner is fine.

I think this is cleaner because that means gitlab-test-fork can be left alone while gitlab-test may be updated. I hope nobody has to touch the former because it's testing a particularly obscure test case (forked projects with submodules).

Looks like there's at least one related failure caused by an overly-brittle test case.

Yeah, I'm on it. Thanks.

Added 1 commit:

• ee22661e - Purge the cache before tests begin to eliminate stale branch data

Added 1 commit:

• ec9b8454 - Fix 404 error in files view after deleting the last file in a repository

Yuck, this test failure was due to stale caching of branch names in the repository. Added a call to clear the cache once and improved the test.

Added 1 commit:

• 5a210ba4 - Fix implementation to handle SHA IDs and add tests

Added 1 commit:

• 7cb45e38 - Fix 404 error in files view after deleting the last file in a repository

Added 1 commit:

• 97991df0 - Fix 404 error in files view after deleting the last file in a repository

Added 1 commit:

• 2f0b6119 - Fix 404 error in files view after deleting the last file in a repository

Added 1 commit:

• 63c33ed4 - Add comment

Added 1 commit:

• 0dfb5424 - Fix 404 error in files view after deleting the last file in a repository

Added 1 commit:

• 643557da - Fix 404 error in files view after deleting the last file in a repository

@stanhu Sorry, I meant if it's only used in the one test to move it directly inside the it block for that test, so it's part of the setup phase.

What are your thoughts on Better Specs? I tend to think minimizing the amount of code in the it block is more readable.

In the last few months I've kind of come to agree with most of Joe Ferris's Let's Not post.

Using the tree_controller_spec.rb here as an example, we've got tests like this:

    context "valid branch, valid path" do
      let(:id) { 'master/encoding/' }
      it { is_expected.to respond_with(:success) }
    end

And if you look at just that test, we're completely obscuring what this test is doing. It's unclear what setting :id does, or that it's required to make the test pass.

To me, the test would be much clearer with something like this:

def go(id)
   get :show, params params params, id: id
end

it 'succeeds with a valid branch and valid path' do
  go 'master/encoding'

  expect(response).to be_success
end

it 'succeeds with a valid empty branch' do
  go 'empty-branch'

  expect(response).to be_success
end

Of course, I'm guilty of abusing lets and subject pretty heavily and I've been actively trying to re-evaluate my usage of them lately. And there are times where I still feel like they're the cleanest way to do a lot of shared setup as we often have to do with things like issues belonging to milestones belonging to projects belonging to users.

It's yet another thing we don't really have a standard for but should discuss further - /cc @DouweM

@stanhu Also you can leave your last commit as-is. Think this is good to merge now?

Yep! Thanks.

Status changed to merged

mentioned in commit f593b621