Skip to content

locking: add post-merge hook to unlock locks#1817

Closed
ttaylorr wants to merge 8 commits intolocking-pre-pushfrom
locking-post-merge
Closed

locking: add post-merge hook to unlock locks#1817
ttaylorr wants to merge 8 commits intolocking-pre-pushfrom
locking-post-merge

Conversation

@ttaylorr
Copy link
Contributor

This pull-request implements a post-merge hook to unlock all locked (owned) files merged back into the master branch of a repository.

Here's a breakdown of what happened:

  1. 1598acb...111046d: implement the post-merge hook
  2. decf27e: Install the post-merge hook
  3. 63b5143...77e2311: add integration tests covering this behavior
  4. 0fbfbba...213577e: fix git-lfs update command to use generic message
  5. cd796b6: add a man-page entry for post-merge

Some things I'd like to consider before merging:

  • Should there be a configurable option to specify a default branch (or set of branches) to check, instead of just master?
  • What should happen if the merge is done using the --no-verify option, skipping the pre-commit hook and merging a file locked by someone other than the current committer?

/cc @git-lfs/core

Copy link
Contributor

@sinbad sinbad left a comment

Choose a reason for hiding this comment

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

I don't understand why you would want to do this.

If I merged a branch locally into master, that says nothing about whether that file should now be unlocked. Locking is meant to prevent accidental parallel changes which would require one side of a binary file change to be discarded; merging changes into master locally doesn't change the fact that I have changes locally which no-one else can see yet, so shouldn't start making changes in parallel with me.

If you wanted to automatically unlock files on completion of work, this post-merge hook should run on the server, not in git-lfs clients.

@technoweenie
Copy link
Contributor

technoweenie commented Jan 3, 2017

I agree that we should hold off on automatic things like this until we're sure it's something users really want. I don't agree that only servers should handle auto-unlocking. We could add the ability to install and uninstall optional hooks like post-merge or pre-commit. But I'd like to be more sure that this is what users want before committing to it in git-lfs :)

EDIT: I meant "auto-unlocking", not "auto-locking" :)

@sinbad
Copy link
Contributor

sinbad commented Jan 3, 2017

I don't agree that only servers should handle auto-locking.

I'm not suggesting that, only that unlocking on local merge will fundamentally break the premise that locking deters parallel edits, because it will make other users believe that it's ok to lock the file now when in fact it isn't (doing so would create a parallel edit because changes are only local to the user who just auto-unlocked)

@technoweenie
Copy link
Contributor

Oh I see what you're saying. We slipped this in for a low-effort demo of auto-unlocking, instead of implementing it on the server. I agree this is the wrong way to implement on the client, if that's what we want. Probably a better way is to look for locked files when pushing to master (or handling this on the server).

@sinbad
Copy link
Contributor

sinbad commented Jan 4, 2017

Yep, post-push would be the time to do it, if there was a hook for it :/

@ttaylorr
Copy link
Contributor Author

ttaylorr commented Jan 6, 2017

I'm in agreement, and I think we can close this for now 👍

@ttaylorr ttaylorr closed this Jan 6, 2017
@technoweenie technoweenie deleted the locking-post-merge branch January 9, 2017 19:48
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants