#1121 #1084 Redirect .git extension in URL to repos page
Merged by abhishekg785. Opened by abhishekg785.
abhishekg785/pagure pagure_changes  into  master

Download 1121.patch

used werkzeug.routing for using regex in routes to match string ending with only git

Rather than relying on a regex, did you try something like:

@APP.route('/<repo:repo>.git')

Trailing spaces :)

Oh, and the trailing slash in the URL is probably not needed then :)

Would you also be interested in adding unit-tests for this change?

@pingou

I used regex as it will be better and fast for matching repo name ending with .git.

I can use a different approach if you want.
Your Suggestions are welcome.

Thanks

anybody is free to disagree but, i would prefer to use flask.redirect than redirect especially when we have to import for one case (also, we have already used flask.redirect in this file)

+1 on this

I used regex as it will be better and fast for matching repo name ending with .git.

Would it really be faster than the approach I suggested? (which isn't tested though, but more what I had in mind for this feature).

I used regex as it will be better and fast for matching repo name ending with .git.

Would it really be faster than the approach I suggested? (which isn't tested though, but more what I had in mind for this feature).

Should i try it using the approach suggested by you?

@vivekanand1101

Thanks for the suggestion,
I will definitely change it to flask.redirect.

Should i try it using the approach suggested by you?

If it works and isn't so slow I think it would make the code simpler

Should i try it using the approach suggested by you?

If it works and isn't so slow I think it would make the code simpler

@pingou
ok then , i will go with your approach

1 new commit added

  • regex approach removed

That will work for project, don't we want to support forks as well?

@pingou

I am adding for the fork part too :)

So are we finally going with this now ?

Looks simpler to me, doesn't it? :)

@pingou

Sure, its too simple :)
I learnt a new lesson today. Thanks to you!

@pingou
just one question
Should i create a different route for the fork part too ?

1 new commit added

  • route for fork created

@abhishekg785 we can do this in the same view: remove .git and redirect

@vivekanand1101

I did not understand what do you mean by removing .git.

@abhishekg785, check flask.request.url

@vivekanand1101

yes we can use flask.request.url for this but it was decided that we have to do by creating a route for this

What i am trying to say is, here you have created two view functions with one route each. We can create just one view with two routes (the other one being the fork one).

@vivekanand1101

At the beginning i did it in the same view without using request.url bcoz the route accepting repo name will accept all strings after '/' so, we can simply check if a string contains .git then split the string on dot and pass the repo name without .git .
So it even did not require "request.url "

But it was decided to use route for the purpose.

@vivekanand1101

oh! my mistake :)

@vivekanand1101

Hey, i can simply check for username
If it is not none then simply redirect for the fork case.

No need for request.url.
What do you think?

1 new commit added

  • extra route removed

:thumbsup: for me

I think I would just do the normal: flask.redirect(flask.url_for('view_repo', repo=repo, username=username))

1 new commit added

  • url_for added

ok, @pingou wins again, this one is better :/

I think we should remove the two empty lines added here

Ok, one last comment but it looks good.

Two things outside the code though:

  • There is 7 commits for this change, do we want to keep them or squash them?
  • We will need to rebase on the top of master before merging :)

@pingou

I think squashing the commits will be a good choice.

I think squashing the commits will be a good choice.

Let's squash and rebase then :)

In the original code there were three empty lines after the comment '# pylint: disable=E1101',

so should i remove the two lines as you mentioned ?

Well, looking at this it seems there were only 2 empty lines here

@pingou

my bad:) , changing it back to the original 2 line empty lines and then squashing the commits

1 new commit added

  • extra empty lines removed

rebased

Hm it seems it detected a rebase but apparently it still needs one

2 new commits added

  • Merge branch 'pagure_changes' of https://pagure.io/forks/abhishekg785/pagure into pagure_changes
  • finding repo with .git at the end resolved

2 new commits added

  • Merge branch 'pagure_changes' of ssh://pagure.io/forks/abhishekg785/pagure into pagure_changes
  • finding repo with .git at the end resolved

Commit messages need love in this :smile:

Looks like the rebase is done but the commits are odd :s

rebased

Trailing spaces :)

@abhishekg785 since you will be removing the spaces, can you please change the commit message as well ? something like: Redirect the urls for git repo to the project index page

i tried to open the file in vim and other text editors ,but there is no space in my case.
I even pulled the original code ,still the condition is same.

just wierd :)

Ok so I'll merge this manually

Commit 40daa05d fixes this pull-request

Pull-Request has been merged by abhishekg785@gmail.com

Pull-Request has been merged by abhishekg785@gmail.com

Pull-Request has been merged by abhishekg785@gmail.com

Pull-Request has been merged by abhishekg785@gmail.com

Metadata