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

Some improvements and further thoughts #12

Open
michael-heerdegen opened this issue Sep 11, 2015 · 19 comments
Open

Some improvements and further thoughts #12

michael-heerdegen opened this issue Sep 11, 2015 · 19 comments

Comments

@michael-heerdegen
Copy link
Contributor

Hello,

I created a fork of your excellent library to adopt it to my needs; here: https://github.com/michael-heerdegen/helm-backup. It was a joy to work with your stuff.

This is a work in progress, some variables still miss defcustom definitions and such stuff, but it is ready to try.

Would you want to work with me to include this stuff? Here is a list of important changes:

  • Drop dependency on "s". We used only one trivial function from "s",
    so it is overblown to depend on it.
  • Migrate to cl-lib.
  • By default, don't copy files into the backup repo but use hardlinks.
    This makes it possible for the user to switch into the repository and
    manipulate the file from there with git commands.
  • Instead of using porcelain commands and branches, use plumbing
    commands and plain references. This allows for more flexibility; OTOH,
    we didn't really make use of any branch features.
  • Don't hardcode the path to git but use `executable-find'.
  • By default, show commit message in Helm `helm-backup' because it
    includes useful info now (see below).
  • New var `magit-backup-respect-git-branches'. When non-nil, for files
    versionized with git, backups made with different branches checked out
    will backup into different histories, to avoid leaps in the backup
    history that just reflect the differences between branches.
  • Simplify semantic of `helm-backup-exec-git-command' for better
    readability.
  • Use file name manipulation functions instead of string operations to
    manipulate file names
  • Add global minor mode `helm-backup-mode'. Related improvements:
    • When appropriate, a backup is now also being made before saving a
      file. This gives the user a backup of the original version of a
      file when editing it for the first time or after it had been changed
      from outside Emacs. This is only done when necessary.
    • The first save of a file in an Emacs session will add "(new
      session)" to the commit message.
  • Avoid using a shell when calling git by using
    call-process-shell-command'; instead useprocess-file'.
  • `helm-backup-versioning' is now a command to allow for explicit
    backups with dedicated messages. This is for better orientation; if
    you have finished some changes, you can make a backup with a message
    "Finished changing this and that, works so far" and can find it easily
    later via the helm interface. That even works when there are no new
    changes.
  • New command `magit-backup-magit-log' gives you a Magit log of the
    backups of the current file. With an active region, it shows only
    commits related to the marked lines.

If you don't have time or don't want to go into that direction, I could also develop my fork on my own into something separate.

Apart from these changes, I would like to split the thing into two libraries:

  • a standalone library with the backup stuff, independent from helm
  • a library for the helm interface. i plan to extend the helm interface with other useful stuff, so that you can also diff against real commit or the index, and also to manipulate the index.

Thanks in advance and with kind regards,

Michael.

@antham
Copy link
Owner

antham commented Sep 13, 2015

Hi Michael,

thanks for your message and your interest, I'm going to answer at every point you mentionned and have a a look to what you've done. Just a thing, it would have been easier to review if you have made one commit per change and a pull request instead of one big commit.

@michael-heerdegen
Copy link
Contributor Author

antham [email protected] writes:

thanks for your message and your interest, I'm going to answer at
every point you mentionned and have a a look to what you've done. Just
a thing, it would have been easier to review if you have made one
commit per change and a pull request instead of one big commit.

Yes, I know. I made the changes so quickly, and they overlap so much,
that it would be a some work to separate them into independent commits.

And note: I just wanted to give you an overview of what I did, and
wanted to know whether you are, in general, interested into going into
the direction I suggested.

If you are interested to merge my changes, I will of course split into
commits and give you pull requests etc. I just wanted to avoid
unnecessary work in case you are not interested or don't have time. The
current code is not even completely finished (some defcustoms and
docstrings missing etc).

So, just have a look, and tell me how you prefer to continue.

Regards,

Michael.

@antham
Copy link
Owner

antham commented Sep 13, 2015

I understood, after writing this comment, I was thinking you did so, cause you expected my opinion first. Having a quick sight, I'm 100% agree about your point concerning git plumbing, I did it quickly and you are right we only need storage and tracking capabilities of git, nothing more. About dependencies, concerning cl-lib, not knowing lisp ecosystem very well, I had a look about that on google and I'm ok with that change as well. About s, your argument is correct, but using good external library to avoid adding such trivial function and keeping them aside from core considering we have now a package manager in emacs, seems valid as well to me. Here I would say it's really a matter of taste except if you have a good argument against that, so if you really want to remove it, I have nothing against that.

Let me some time to review carefully other points.

@michael-heerdegen
Copy link
Contributor Author

Ok, I'll send you orthogonal PRs successively. Let's start simple: #13

Note: I don't know anything about the other files in the repo besides elisp and git ones. If I need to know something about them to make the PRs and avoid to break something, let me know. And I don't want to update the README in between until we are more or less done.

Regards,

Michael.

@michael-heerdegen
Copy link
Contributor Author

Some notes about ongoing progress: I changed my mind and dismissed the idea of saving under separate refs when having different branches checked out. This is more confusing than helpful. Instead, I now just put the branch name into the commit message.

And I'm currently trying to use the file's own repository for our purpose when it is git versionized. This is possible since I converted to plumbing commands. And doing that, all our commits are valid commit objects in the original repository, so you have access to them via git in that directory. That allows you to git-diff the head of a different branch with an older backup save, for example.

All this will become a PR in the near future. Just want to let you know in case you think "No, not with me!" or so.

@antham
Copy link
Owner

antham commented Sep 29, 2015

Sorry for this big delay, I wasn't free those days, I will finish to review everything today. Thanks for your messages, I merged your PR.

@michael-heerdegen
Copy link
Contributor Author

antham [email protected] writes:

Sorry for this big delay, I wasn't free those days, I will finish to
review everything today. Thanks for your messages, I merge your PR.

There is no hurry, as long as we don't get absolutely stalled (and the
risk for that is large from my side, too). If we don't completely
forget about it, I don't mind much how long it takes.

@antham
Copy link
Owner

antham commented Sep 29, 2015

Ok great. About other files in repo, it's related to functional test and to have a development environment so they are important, you can have a look here https://travis-ci.org/antham/helm-backup to see how it works.

@michael-heerdegen
Copy link
Contributor Author

Here is the next one: #14.

@humitos
Copy link

humitos commented Jun 18, 2017

Are you still working on all of these chages/features?

@antham
Copy link
Owner

antham commented Jun 20, 2017

I don't he does @humitos it's an old ticket.

@michael-heerdegen
Copy link
Contributor Author

michael-heerdegen commented Jun 21, 2017 via email

@antham
Copy link
Owner

antham commented Jun 21, 2017

Yep it's partly my fault, I wasn't helpful about that, I will have a look this week-end to see what can I do about to start to pick in main repository some features.

@michael-heerdegen
Copy link
Contributor Author

michael-heerdegen commented Jun 21, 2017 via email

@antham
Copy link
Owner

antham commented Jun 21, 2017

It doesn't change from the time you did it but it has to be included step by step with some tests and I have to check what we keep and what we leave (if we have to leave some stuffs)

@dakra
Copy link
Contributor

dakra commented Dec 5, 2017

@michael-heerdegen I like most changes of your version. Can you try and make some PRs or can I cherry-pick some of your stuff and I would then make PRs here?
If you have a newer version locally and not on github, please push so I can check it out.

Thanks.

@michael-heerdegen
Copy link
Contributor Author

michael-heerdegen commented Dec 8, 2017 via email

@michael-heerdegen
Copy link
Contributor Author

michael-heerdegen commented Dec 13, 2017 via email

@dakra
Copy link
Contributor

dakra commented Dec 13, 2017

I have uploaded the version of the file I am currently using myself to a branch named "current" in my fork.

Nice, thanks.

@riscy riscy mentioned this issue Oct 28, 2019
7 tasks
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

No branches or pull requests

4 participants