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

wip: git support and git init #66

Closed
wants to merge 2 commits into from
Closed

Conversation

brokenpip3
Copy link

@brokenpip3 brokenpip3 commented Feb 5, 2019

Checklist

  • I have made a change to this repository, be it functionality, testing, documentation, spelling, or grammar.
  • I updated my branch with the master branch.
  • I have added the necessary testing to prove my fix is effective/my feature works (or I did not modify functionality).
  • I have added necessary documentation about the functionality in an appropriate .md file.
  • I have appropriately commented any code I have modified

Short description of what this PR does:

  • Add support for git command
  • Add support for git "add" command
  • Add "init" command to create the $NOTES_DIRECTORY and init a empty git repo inside

@pimterry
Copy link
Owner

I've thought about this for a few days, and I'm not really convinced it's a good idea.

I do see how using notes with git could be useful, but I think that either you can use git manually as normal, with no changes needed here, or notes should manage it for you automatically. Here we're in an odd middle ground, where notes doesn't help you with git, but you call git through it all the time.

I've written some thoughts on this before over here that might be of interest: #12 (comment).

It looks like the main benefit this provides at the moment is that it passes the notes directory as -C, so the correct working directory is used. Are there other important benefits to this?

I don't think we should add a wrapper to notes for every command that you might want to run in your notes directory. We could potentially provide a generic helper though... How would you feel about something like notes run git status? That would be a generic notes run command that runs the rest of the arguments as a command in the notes directory. Simple and works for git cases like this, but also nicely flexible for any other commands too.

@brokenpip3
Copy link
Author

Hi, thanks for review of my pull request.

I've thought about this for a few days, and I'm not really convinced it's a good idea.

Ok no problem

I do see how using notes with git could be useful, but I think that either you can use git manually as normal, with no changes needed here, or notes should manage it for you automatically. Here we're in an odd middle ground, where notes doesn't help you with git, but you call git through it all the time.
I've written some thoughts on this before over here that might be of interest: #12 (comment).

I read all now, sorry for not do it before.
Maybe i can try to implement the "autocommit" features, as describe on the comment by primis with "notes git" as primary command for this options and all remain git command.

It looks like the main benefit this provides at the moment is that it passes the notes directory as -C, so the correct working directory is used. Are there other important benefits to this?

Other benefits: the notes init part; but yeah maybe you are right about this.
I was inspired from "pass" password-manager (autocommit features is present on this app too)

I don't think we should add a wrapper to notes for every command that you might want to run in your notes directory.

Right, but git is the best option to backup your notes and share it across device, different by importance from other type of command wrapper.

We could potentially provide a generic helper though... How would you feel about something like notes run git status? That would be a generic notes run command that runs the rest of the arguments as a command in the notes directory. Simple and works for git cases like this, but also nicely flexible for any other commands too.

But how do you implement this?
Git use the "-C" option but other command may use other type of argument to point to some specific dir.
I don't like something like "cd $notes_dir and then run this".
Other thoughts: a generic run command can be data destructive and security issue.

Thanks and sorry for my bad english.

@pimterry
Copy link
Owner

Maybe i can try to implement the "autocommit" features, as describe on the comment by primis with "notes git" as primary command for this options and all remain git command.

Autocommit would be interesting, if you'd like to have a go! It's not super easy, but I think it would be popular. Do make sure you read the other thread carefully first though, because it's important to do it well.

I don't think we should add a wrapper to notes for every command that you might want to run in your notes directory.

Right, but git is the best option to backup your notes and share it across device, different by importance from other type of command wrapper.

I don't think I agree. I don't think manually adding, committing, pushing & pulling every change to your notes files is common. Instead, most users use one of the many tools like Dropbox/Google Drive/Spider Oak/OwnCloud/etc which do automatic versioning & sync. And on the other hand, I can think of many other great use cases that a more generic run command would help with, plus it allows for lots of other new ideas that I haven't thought of.

How do you implement this?

(cd $NOTES_DIRECTORY; $COMMAND) should work nicely, I think? It changes directory, but only for that one command, not the rest of your session.

I don't like something like "cd $notes_dir and then run this".

Could you be more specific please? Why don't you like this?

Other thoughts: a generic run command can be data destructive and security issue.

I don't think this is a concern. Users have to manually run the command themselves, and it's not any more dangerous than any other command you may run on your command line. I don't think it's very different - any risky command run with notes run would also be very risky to run directly. rm -rf / is obviously dangerous to any experienced CLI user. notes run rm -rf . would be equally dangerous, but also equally obviously dangerous.

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.

2 participants