Skip to content
This repository was archived by the owner on Dec 15, 2022. It is now read-only.

support nested repos #469

Closed
wants to merge 1 commit into from
Closed

support nested repos #469

wants to merge 1 commit into from

Conversation

jarig
Copy link

@jarig jarig commented Jun 6, 2015

Fixes atom/atom#2203

This patch works quite well even with >200 nested .git repos, though with some limitations:

  1. If there are a lot of nested. git repos then start-up time might noticeably increase(with ~200 repos it was 10 sec). The reason is that GitRepository object seems to be not fully async while refreshing status.
  2. Status refresh of GitRepositry objects on window_focus event is disabled, so if repos changed out of the Atom it might reflect wrong status (but there is extra Atom command for refreshing those).

repo_highlights

if @directory.isRoot
iconClass = 'icon-repo' if repoForPath(@directory.path)?.isProjectAtRoot()
relPath = repoForPath(@directory.path)?.relativize(@directory.path + '/')?.length
if relPath == 0
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like this can be unless relPath

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

unless triggers in case of 'undefined' as well, but in this case it should trigger only if string is empty(but not undefined).

@winstliu
Copy link
Contributor

winstliu commented Jun 6, 2015

Right now Travis is reporting quite a few lint errors - you should fix those.

In addition, this will need specs before it can be considered for merging.

@jarig
Copy link
Author

jarig commented Jun 10, 2015

I've fixed all probs with existing specs and added 4 new ones (12 tests) covering all new functionality. Also created GIF showing what it is about

@wjwwood
Copy link

wjwwood commented Jun 29, 2015

This works for me, but the diffs in the gutter of changed files doesn't seem to work anymore.

@jarig jarig force-pushed the master branch 5 times, most recently from 1feff36 to 4c5e4ca Compare August 5, 2015 10:13
@jarig
Copy link
Author

jarig commented Aug 5, 2015

Rebased on latest changes.

This works for me, but the diffs in the gutter of changed files doesn't seem to work anymore.

Well yeah, that should be fixed on git-plus (or whatever plugin you use) side.
Instead of taking project root it should take the nearest upwards .git folder.

However git diffs with flat project structure should still work.

@ruffsl
Copy link

ruffsl commented Aug 7, 2015

This is awesome

@winstliu
Copy link
Contributor

winstliu commented Aug 7, 2015

Sorry, but it looks like there's more conflicts 😬.

@jarig
Copy link
Author

jarig commented Aug 10, 2015

Re-based again, with previous re-base accidentally took some extra files to commit(like packages.json), fixed that.

@mikekelly1
Copy link

@jarig Do modified files in sub-repositories show up when you press CTRL-T with your fix?

@jarig
Copy link
Author

jarig commented Oct 5, 2015

Do modified files in sub-repositories show up when you press CTRL-T with your fix?

Unfortunately not. Atom relies on the fact that there is only 1 GitProvider per project, but it should be changed to support multiple, then those providers can be updated from TreeView and in this way other things will work as well.

@phord
Copy link

phord commented Oct 5, 2015

@jarig fyi - I have a version (phord/tree-view@fad0086) that does manage to track modified file subscriptions successfully. But startup is slow. I'm working on improving it, though. Suggestions welcome.

@jarig jarig force-pushed the master branch 4 times, most recently from 7ecd35c to 8ffecbb Compare November 20, 2015 13:14
@jarig
Copy link
Author

jarig commented Nov 20, 2015

@phord , @mikekelly1
I've made changes to support ctrl+t as well as highlighting of modified lines in files. Performance is quite good, tested with ~250 nested repos.
Tests are failing though, will check later why, in "real life" all works :)

@joshaber
Copy link
Contributor

I'm not sure this is the right place for this fix. Ideally we'd make whatever changes are necessary in GitRepository or GitRepositoryAsync in Atom itself, so that all users of GitRepository* can take advantage of these fixes.

@jarig jarig force-pushed the master branch 2 times, most recently from acdd32c to 4fe615d Compare January 25, 2016 22:43
@omern1
Copy link

omern1 commented Feb 11, 2016

Why aren't you merging this?

@joshaber
Copy link
Contributor

See my comment above: #469 (comment)

@watzon
Copy link

watzon commented Jun 13, 2016

Definitely a feature that's needed. Can we get the ball rolling on this one?

@jarig
Copy link
Author

jarig commented Jun 24, 2019

I've rebased all changes again and fixed all tests and linting. So now it's fully green.

@Zireael07
Copy link

Hopefully this gets merged soon.

@lee-dohm lee-dohm changed the title support nested repos, atom/atom#2203 support nested repos Jul 2, 2019
@lee-dohm
Copy link
Contributor

lee-dohm commented Jul 3, 2019

Thanks for all the work that has been invested in this pull request.

In our weekly triage meeting, we discussed this change and have decided to not accept it. Besides the caveats mentioned at the top, we don't feel that this change has been validated against a wide enough variety of use cases to not have hidden pitfalls. And because there have been significant usability impacts around this functionality before, we can't take the risk of unleashing this untested on the entire user base.

Thanks again for the feedback and the work that you've put into this change.

@lee-dohm lee-dohm closed this Jul 3, 2019
@jarig
Copy link
Author

jarig commented Jul 3, 2019

And because there have been significant usability impacts around this functionality before

Is that real pitfall or theoretical? It never happened in real life, as current algo respects project boundaries.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Git integration doesn't work if .git/ not in top level