search

ryuslash / git-auto-commit-mode

Automatically commit to git after each save
http://projects.ryuslash.org/git-auto-commit-mode/
GNU General Public License v3.0
167 stars 26 forks source link

Commit auto-commits to separate branch... #18

Closed telotortium closed 4 years ago

telotortium commented 5 years ago

...before merging the auto-commit back into the main branch.

This protects against the following scenario:

  1. You've loaded a file at commit A.
  2. While your file is unsaved, the repo underneath is updated to some commit B that is a descendant of A.
  3. You save your file. Emacs prompts you that the file underneath has changed. You decide to overwrite, but you've lost the differences between A and B.

Now, we first switch to a temp branch and save the file as a commit on top of commit A, and then switch back to the branch with commit B and merge the temp branch (with our new change) back in.

Note to maintainer: I've also sent you an email with this pull request using git request-pull.

ryuslash commented 5 years ago

Hi!

Thank you for the time and effort you've put into implementing this! Sorry for the long delay in getting back to you. I will try and have a proper look at this pull request very soon!

I'm sorry to question your workflow, but isn't this more of a problem with not saving enough, or automatically pulling (as opposed to fetching) into the underlying repository? At first glance it seems like a lot of trouble to go through, and this project was only ever meant to be really simple. I haven't had a proper look yet, so maybe I'm just looking at it the wrong way.

telotortium commented 5 years ago

I do have an external script that I use to sync a couple of Org mode repos, where I keep a lot of notes. I made this modification so that if I happen to have a file unsaved in Emacs when I run my script, I don't overwrite changes pulled in through the script accidentally. I could set up the script to just do fetches, but it would make it less convenient to use. Right now the script does the following:

I could add another prompt to tell me to save my Emacs Org mode files and make sure they've been committed before I fetch my remote repo. However, I want to minimize the risk of losing work, and I think that this change is a good way of doing that.

ryuslash commented 5 years ago

Hey! Sorry I've dropped the ball on this so badly, but if you could resolve the merge conflicts that GitHub tells me there are I'll gladly merge this.

telotortium commented 5 years ago

I'll take a shot at doing this in 2 weeks or so.

On Mon, Oct 7, 2019 at 8:29 PM Tom Willemse notifications@github.com wrote:

Hey! Sorry I've dropped the ball on this so badly, but if you could resolve the merge conflicts that GitHub tells me there are I'll gladly merge this.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub, or mute the thread.

-- Robert Irelan rirelan@gmail.com

gcv commented 4 years ago

Can we talk about this patch a little more?

First of all: why are gac-loaded-commit and the other internal state variables defcustom? They don't seem destined for the customize interface, so they should be defvar.

Second: can we make this new behavior opt-in? I haven't read the code quite closely enough to figure out if another branch is created for auto-commited files unconditionally, or only if the working tree has pending changes brought in externally. If it's unconditional, then I don't want it because my use case for git-auto-commit is pretty simple and I don't have external repository sync to worry about. If a separate branch is only used when there is a possibility of corruption, then I feel better about it, but even so — that's a lot of code complexity added.

ryuslash commented 4 years ago

Good points. One of the reasons this took me so long to respond to was because I was thinking if I could find any way of working around the issue in a simpler way. But I couldn't think of anything, but I also don't feel comfortable dismissing the use case, because it seems at least one person needs it. This is way more complicated and serious than I ever intended git-auto-commit-mode to become.

Still, like I said it seems like a valid use case, so I want to support that. I'm not quite comfortable merging and testing this code myself, though, so hopefully Robert will still have a look at some point.

gcv commented 4 years ago

I agree, and I am not advocating for rejecting the patch. I just wish it to be opt-in functionality. Without digging deeply into understanding the implementation, I think it's a matter of adding a Boolean defcustom like gac-branch-before-commit with a default of nil, and using it to block the logic of gac-find-file-func, gac-before-save-func, as well as the new logic in gac-before-save-func.

ryuslash commented 4 years ago

Hey, I'm very sorry to do this, especially after it's taken so long to get to this point, but I'm going to have to say that I'm not going to merge this change after all. Looking at the code we're getting very close to running a full-on shell script from Emacs with some of the commands that are going on right now and this is just going way beyond the scope this project was ever supposed to have. My attention to this project is also possibly not what it should be to maintain this change, since I haven't actually used it myself in quite a while.

Feel free to fork of course, and start a project based on this, I understand this is a use case that you need, but I just don't think it fits within git-auto-commit-mode.

Thank you for wanting to contribute back, I'm sorry I won't be merging your change.

telotortium commented 4 years ago

Hi, I completely understand. I'll continue to keep this as a fork.

On Mon, Feb 17, 2020, 15:45 Tom Willemse notifications@github.com wrote:

Closed #18 https://github.com/ryuslash/git-auto-commit-mode/pull/18.

— You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub https://github.com/ryuslash/git-auto-commit-mode/pull/18?email_source=notifications&email_token=AANMVDLY7FBIVRTBHHVZICLRDMORNA5CNFSM4HFAS4EKYY3PNVWWK3TUL52HS4DFWZEXG43VMVCXMZLOORHG65DJMZUWGYLUNFXW5KTDN5WW2ZLOORPWSZGOWWBP66A#event-3045261176, or unsubscribe https://github.com/notifications/unsubscribe-auth/AANMVDJAQLIC6YTPMGZRD2DRDMORNANCNFSM4HFAS4EA .