Skip to content

Conversation

kephale
Copy link

@kephale kephale commented Sep 22, 2021

I have been happily using git-auto-commit-mode, but when the git repo is being worked on separately from different locations then I get merge conflicts when trying to auto push. This is a bit of a nuisance when doing something like working in a directory of org files and the conflicts are automatically mergable but it is easy to not notice that there was a merge conflict (plus it still requires manual resolution).

This PR introduces automatic pull support which runs in a before-save-hook. It works the same way as auto push, but simply pulls before the buffer is saved. It is also labeled as risky.

@ryuslash
Copy link
Owner

ryuslash commented Oct 5, 2021

Hey!

Thanks for this pull request :) I'm glad to hear that you're found git-auto-commit-mode to be a useful project. The code basically looks good, I just have one question which I'll post inline.

Comment on lines 277 to 280
(when (and (buffer-live-p buffer)
(or (and gac-automatically-add-new-files-p
(not (gac--buffer-is-tracked buffer)))
(gac--buffer-has-changes buffer)))
Copy link
Owner

Choose a reason for hiding this comment

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

Other than the buffer-live-p, are these the requirements you actually want?

For pushing it doesn't make sense to push when there won't be any changes to push, but technically there could be changes to pull regardless of whether or not this file is actually part of the git repository.

On the other hand I can also understand if it seems like overkill (and slow) to try and pull every single time if committing and pushing aren't going to happen either.

Choose a reason for hiding this comment

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

@ryuslash FYI it looks like @kephale made changes in response to this, in case you didn't get a notification for the new commit.

(I'm interested in this MR too and happened to notice)

Copy link
Author

Choose a reason for hiding this comment

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

Yes, I delayed on suggesting that this is final because there are some issues with merges that need to be resolved.

Copy link
Owner

Choose a reason for hiding this comment

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

@chasecaleb thanks for the ping!

@kephale ok, so I'll wait until you let me know that everything is fine before merging this. What sort of issues are you having? And if they're caused by my comment than please disregard it and feel free to revert to the initial state you opened the review in.

@kephale
Copy link
Author

kephale commented Nov 11, 2021

Phew, sorry this took so long, but there were some subtle issues that required a continuous block of time to solve them all at the same time. Note that this looks quite a bit different than the previous iteration.

One annoying thing I introduced is that since it uses a make-thread call this blocks for a bit on save, which can be frustrating. I'm not sure what the async version of this is.

[edit] it looks replacing make-thread with async-start from https://melpa.org/#/async solves it, but I'm not sure how you feel about the extra dependency [/edit]

@ryuslash
Copy link
Owner

Hey :) That's alright, we all have busy lives and nobody is getting paid to work on this, things take time. And this doesn't seem like an easy issue to fix (after making changes, before saving, quickly pull changes which may affect the file you're trying to save).

I don't think the problem is the call to make-thread. I haven't used it in Emacs before, so I had to look up how it works a little. It says it's "mostly cooperative", which took me a moment or two to parse. Basically, because the lambda you created at no point is waiting for input from the keyboard, calling accept-process-output, or thread-join, it's not going to provide any benefit over calling the lambda without a thread. I'm not quite sure how start-process, accept-process-output, and make-thread are supposed to work together, since accept-process-output sounds like it returns either nil or non-nil depending on whether any output was received or not, not the output that may or may not have been received. Is it something like this? (using the previous implementation for push as example)

(let ((process (start-process "git" "*git-auto-push*" "git" "push")))
  (set-process-filter process #'gac-process-filter)
  (set-process-sentinel process #'gac-process-sentinel)
  (while (process-live-p process)
    (accept-process-output process)))

If async solves this in a clean way and works with password input by all means feel free to add it.

@chasecaleb
Copy link

I'm not sure how helpful this is, but if you haven't come across it you might want to check out how git-sync handles merge conflicts and so on. It's a bash script and I've been using it across multiple computers for quite a while. It does a good job of automatic pushing and warning on merge conflicts.

@siatko
Copy link

siatko commented Oct 20, 2023

Hi, I'm also using this minor mode with pleasure. To solve the problem with auto pulling before pushing, I use the following in my Doom Emacs config:

(use-package! git-auto-commit-mode
  :config
  (setq-default gac-automatically-push-p t)
  (setq-default gac-automatically-add-new-files-p t)

  (defun gac-pull-before-push (&rest _args)
    (let ((current-file (buffer-file-name)))
      (shell-command "git pull")
      (when current-file
        (with-current-buffer (find-buffer-visiting current-file)
          (revert-buffer t t t)))))
  (advice-add 'gac-push :before #'gac-pull-before-push))

Should be working regardless of Doom. I only had to initially set the pull strategy.

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.

4 participants