#7949 Better listing of files changed in a certain commit

v1.5.0
closed
ux (103)
General
2016-04-12
2015-08-04
No

Followup to [#7925] sort of - at least things noticed from there.

Would be nice to show copies and renames a little differently. Not sure how though.

On a commit view for git, every parent directly is always listed as a change. This is due to -t in the git.diff_tree call. We should be able to replace this with -r (with some confirmation that added_paths() still works right)

Commit views should list all files in alphabetical order, rather than grouped by add/remove/etc. Make sure paginating through it works correctly still too.

Related

Git: 1c2e7209
Tickets: #7925
Tickets: #8060

Discussion

  • Pranav Sharma - 2016-03-31
    • status: open --> in-progress
    • assigned_to: Pranav Sharma
    • Reviewer: Dave Brondsema
     
  • Dave Brondsema

    Dave Brondsema - 2016-04-01

    This works just right, I think. There's a few things to make sure the code is really good though.

    The commits in testui2.git could be more complicated so that the tests are stronger. With just one or two files in a commit, its hard to ensure that sorting really is working right. A good test case would be something like "add aaa.txt, remove bbb.txt, move ccc.txt to ddd.txt, add eee.txt" and check sorting then.

    The test that has # Check for sorted output. isn't accurate, since sorting happens in the controller. Can we have a test to validate that? Would have to be a "functional" test and inspect the HTML (find some examples that use r.html.find... which is a BeautifulSoup object and a good way to check for html elements).

    The pre-existing comment # show tree entry itself as well as subtrees (Commit.added_paths relies on this) is something we should take note of carefully. The docstring in Commit.added_paths also mentions that it relies on paged_diffs returning all paths. added_paths is used within LastCommit which has some quite complicated logic (it determines what to show in the 'commit' column when browsing the repository directory structure). So, just to make sure we don't break anything there, I suggest you make an optional parameter on paged_diffs to control whether to use -t or -r so that it still includes folder names in that case.

     
  • Dave Brondsema

    Dave Brondsema - 2016-04-06

    Looking good, just a few things I want to suggest to make the code more readable & maintanable. The sorting line has a lot in it, and its not immediately obvious what it's doing, so a comment like "sort by filename" would help. Also it would be good to follow "pep8" formatting guidelines, e.g. onlyChangedFiles=True instead of onlyChangedFiles = True and only one space before onlyChangedFiles (you have two spaces in a few places). I would recommend installing/configuring a pep8 tool for your editor. That will show you whenever your formatting doesn't follow the guidelines. (But don't worry about any existing violations, we just want new/changed code to follow the guidelines).

    ForgeHg will need to be updated as well since it has a paged_diffs method. At the very least it needs to handle the onlyChangedFiles parameter being passed in and ignore it. Better would be to try an Hg repo and see if it includes directories and if so then make it skip them based on that parameter. https://sourceforge.net/p/forgehg/code/ci/master/tree/forgehg/model/hg.py#l523 If you can fork that repo and make a merge request for it, that'd be great.

     
  • Dave Brondsema

    Dave Brondsema - 2016-04-12
    • status: in-progress --> closed
     
  • Dave Brondsema

    Dave Brondsema - 2016-04-12

    Nice work.

     
  • Dave Brondsema

    Dave Brondsema - 2016-04-12

    Oops, ForgeSVN needed the new param too. I've added it in [9585780494c181c2e5e46f1ec8622ad3bd600a7f]

     
  • Dave Brondsema

    Dave Brondsema - 2016-08-22
    • Milestone: unreleased --> v1.5.0
     

Log in to post a comment.