Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

vendoring django completion #1836

Merged
merged 3 commits into from
Feb 13, 2021
Merged

vendoring django completion #1836

merged 3 commits into from
Feb 13, 2021

Conversation

buhl
Copy link
Contributor

@buhl buhl commented Feb 9, 2021

Description

Some vendored libraries like the bash completion of django resides in the django source-code.
To not have to include the entire django project in bash-it just to get django completion we have to rewrite the commits git vendor creates.

Fair warning: This wont be pretty and I can't recommend this procedure.

 ~/.bash-it/ $ git checkout -b django-complete-workbranch
 ~/.bash-it/ $ git vendor add django-completion https://github.com/django/django 3.1.6
 ~/.bash-it/ $ COMMIT_MSG=$(git log -1 --format=%B)
 ~/.bash-it/ $ git checkout $(git log -2 --format=%h | tail -n 1)
 ~/.bash-it/ $ git rm -fr CONTRIBUTING.rst django/ docs/ .e* .gitattributes .github/ .gitignore  Gruntfile.js .hgignore INSTALL js_tests/ MANIFEST.in package.json README.rst scripts/ setup.* tests/ tox.ini  .tx/
 ~/.bash-it/ $ PRE_COMMIT_ALLOW_NO_CONFIG=1 git commit --amend
 ~/.bash-it/ $ COMMIT_HASH=$(git log -1 --format=%H)
 ~/.bash-it/ $ git checkout master
 ~/.bash-it/ $ git checkout -b django-complete
 ~/.bash-it/ $ git subtree add -m "$COMMIT_MSG" --prefix=vendor/github.com/django/django $COMMIT_HASH
 ~/.bash-it/ $ git commit --amend
 ~/.bash-it/ $ ## To remove
               #
               #git-subtree-dir: vendor/github.com/django/django
               #git-subtree-mainline: 2d56cc4167a5a884722f770fe32ace4cb16143f3
               #git-subtree-split: 0dd01da4b70e23d57d44f82e686e5361c3f61158
               #
               # from the commit message
 ~/.bash-it/ $ git branch -D django-complete-workbranch

this will leave the commit history looking like git vendor just did it's thing.
But git vendor update django-completion wont work. You would have to go through the steps above again to clean out unwanted files.

Motivation and Context

#1818 and potentially #1819

However as mentioned earlier this is not really a method I would recommend.
Some more discussion of this problem is needed.

How Has This Been Tested?

Enabled the django completion and checked with the complete command.
Ran local tests

Checklist:

  • My code follows the code style of this project.
  • I have read the CONTRIBUTING document.
  • If I have added a new file, I also added it to clean_files.txt and formatted it using lint_clean_files.sh.

git-subtree-dir: vendor/github.com/django/django
git-subtree-split: 32bb1421ae4bf8e3b730be0cb47ee026a72961f7
git-vendor-name: django-completion
git-vendor-dir: vendor/github.com/django/django
git-vendor-repository: https://github.com/django/django
git-vendor-ref: 3.1.6
@buhl
Copy link
Contributor Author

buhl commented Feb 9, 2021

I kind if want to make my own version of git vendor that doesn't merge a subtree but just creates a commit on the branch you can easily amend and do rebasing on.
I like that git vendor doesn't have a config file but I really don't like how troublesome it is to work with regarding cleaning the commit and rebasing.

@buhl
Copy link
Contributor Author

buhl commented Feb 9, 2021

Idle hands...
I have made a fork of git vendor https://github.com/buhl/git-vendor
It now puts a nice little squashed commit on the top of the branch instead of the horrible dangling merge commits.

You can test it like so.

./repos/ $ mkdir test && cd test && git init .
./repos/test $ git vendor add apm https://github.com/vigo/apm-bash-completion 1.0.0 --debug
./repos/test $ git log
./repos/test $ git vendor list
./repos/test $ git vendor remove apm
./repos/test $ git log
./repos/test $ git vendor list
./repos/test $ git vendor add apm https://github.com/vigo/apm-bash-completion 1.0.0 --debug
./repos/test $ git log
./repos/test $ git vendor list
./repos/test $ git vendor update apm --debug
./repos/test $ git log
./repos/test $ git vendor list
./repos/test $ git diff --stat HEAD~

There might be some edge cases especially when updating. In worst case git will throw you into a cherry-picking mode you can just abort.
I create a temporary branch where the subtree will be added and discarded from.

Using this method would greatly aid working with PR's on vendored libs.
I am aware of the irony of being the one to suggest git vendor in the first place to now talking about using something else 😃

Copy link
Member

@NoahGorny NoahGorny left a comment

Choose a reason for hiding this comment

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

LGTM
Well done @buhl !
You really put a lot of great work here 😄

@buhl
Copy link
Contributor Author

buhl commented Feb 12, 2021

Thanks @NoahGorny

Before you merge this I have to rebase the branch otherwise it won't merge. Which means I have to do alle the steps again 🤐
Which is also why I have started a fork of git-vendor 😇

@NoahGorny
Copy link
Member

Thanks @NoahGorny

Before you merge this I have to rebase the branch otherwise it won't merge. Which means I have to do alle the steps again
Which is also why I have started a fork of git-vendor

It seems mergeable to me, but okay!
I also think you should contact @Tyrben about merging your fork to his repo, as I kinda think its better and more sustainable than another fork

@buhl
Copy link
Contributor Author

buhl commented Feb 13, 2021

I think I tried a merge locally when I started using vendor and it failed because git vendors squashed commit doesn't share the same root commit or something. You are welcome to give it a try and merge it 😄
I think I will try to suggest my changes to @Tyrben but they are not ready yet. There are some issues (I think) when updating, files that where removed from the vendored repo will not be removed with my current implementation.

Let me know if you want to try a merge or I should do the rebase

@NoahGorny NoahGorny merged commit 4607ee7 into Bash-it:master Feb 13, 2021
@buhl
Copy link
Contributor Author

buhl commented Feb 13, 2021

nice @NoahGorny

@gaelicWizard
Copy link
Contributor

@buhl, is your fork of git-vendor OK to use?

I'm asking b/c the patch has something about lender that I don't follow. (No need to explain so long as it's OK to just use in place of the regular one.)

@NoahGorny
Copy link
Member

the fork should be okay to use, but seems like @buhl can even try to merge it upstream- the owner of the original repo (@brettlangdon) seems to be active again!

@buhl
Copy link
Contributor Author

buhl commented Nov 29, 2021

Oh man, it has been a long time since i looked at git-vendor. I will look into if they want to accept my changes upstream 😺

@buhl buhl deleted the django-complete branch November 29, 2021 20:37
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants