gitlist-php / gitlist

An elegant and modern git repository viewer
BSD 3-Clause "New" or "Revised" License
5 stars 0 forks source link

Fixes issue #37 and adds a pagination button. #38

Closed PABourdin closed 7 years ago

PABourdin commented 7 years ago

Added a pagination for the "Patch Log" with a default of 100 entries per page. The reason is that for long Patch Logs, there might occur very quickly a memory exhaustion from PHP, which returns just a blank page.

Also one should note that no unneccessary "new Something()" is present in a loop!

PABourdin commented 7 years ago

@DannyvdSluijs I agree with your proposals, but as you already know, the code is full of style breaks and really fixing them will require a quiet large changeset. I will create a new branch and PR for that and would leave this PR as it is, for simplicity. Are you fine with that?

DannyvdSluijs commented 7 years ago

Yes lets take that approach. After that ‘mayor’ code style correction we can do review like was done for this PR.

On 21 Aug 2017, at 22:38, Philippe Bourdin notifications@github.com wrote:

@DannyvdSluijs https://github.com/dannyvdsluijs I agree with your proposals, but as you already know, the code is full of style breaks and really fixing them will require a quiet large changeset. I will create a new branch and PR for that and would leave this PR as it is, for simplicity. Are you fine with that?

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/gitlist-php/gitlist/pull/38#issuecomment-323846660, or mute the thread https://github.com/notifications/unsubscribe-auth/AAlxvAUq6qQKrbFFiqxXnYqEpiExRWlWks5saeqsgaJpZM4O8pJO.

PABourdin commented 7 years ago

Great, so we then dismiss this Changes request and open another issue for fixing the coding style. Also I will wait until all ongoing PRs are merged, before I start fixing... Thanks and best greetings!

alehaa commented 7 years ago

@DannyvdSluijs as we're moving back to upstream, this PR needs to be send back to upstream.

The plan is to delete this fork when all changes have been send back to upstream, so the upstream may eventually be moved into this organization.

DannyvdSluijs commented 7 years ago

Indeed that is why I want to cleanup the PR's. Hoping either you or @PABourdin will check mine allowing me to remove my gitlist-php fork and create a direct fork of klaussilveira.

Otherwise the commits of my PR will be lost. I want to start looking at fixing the PHP 5.5 issues from upstream.

alehaa commented 7 years ago

Yep, but merging won't help if the merge will be lost, too. So please don't modify any branches in this fork, otherwise we may be confused and commits will be lost. I'll check the upstream for a PR in the next minutes. ;)

There's no PHP 5 issue, just one with composer, as discussed in klaussilveira/gitlist#747.

PABourdin commented 7 years ago

@DannyvdSluijs Yes, this changeset needs to be sent to upstream, too. Is it possible to create a PR upstream that simply inherits my commits or will I have to create a new fork and PR at the upstream?

alehaa commented 7 years ago

You'll need to create a new fork and copy the commits into it, as this fork is not in-sync with upstream.

PABourdin commented 7 years ago

@alehaa actually, this fork should be to large extent in-sync with upstream, except for a minor conflict that I can resolve easily. This fork just has the additional Silex2 migration, and the changeset actually in discussion here.

alehaa commented 7 years ago

If you look in the network graph, you'll see that we've merged other stuff than upstream. So this fork will be purged, when PRs and issues have been migrated to be consistent.

PABourdin commented 7 years ago

@alehaa I'm sorry, I am exactly seeing we - to the largest extent - merged a subset of upstream. And just one merge would create a conflict. On the other hand, upstream misses a lot of commits we have done so far - so I absolutely want to avoid re-doing all the commits I did to this fork. Is there an easy solution? Could you explain in more detail, how I can transfer my commits into another fork's branch?

Thanks and best greetings.

alehaa commented 7 years ago

I'll explain it later ;)