Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Hi Junio, On Mon, 10 Nov 2014, Junio C Hamano wrote: Johannes Schindelin johannes.schinde...@gmx.de writes: I do not think of a good justification of detachInstead offhand, but you must have thought things through a lot more than I did, so you can come up with a work flow description that is more usable by mere mortals to justify that mode. The main justification is that it is safer than updateInstead because it is guaranteed not to modify the working directory. I intended it for use by developers who are uncomfortable with updating the working directory through a push, and as uncomfortable with forgetting about temporary branches pushed, say, via git push other-computer HEAD:refs/heads/tmp. However, I have to admit that I never used this myself after the first few weeks of playing with this patch series. Without such a description to justify why detachInstead makes sense, for example, I cannot even answer this simple question: Would it make sense to have a third mode, check out if the working tree is clean, detach otherwise? I imagine that some developer might find that useful. If you insist, I will implement it, even if I personally deem this mode way too complicated a concept for myself to be used safely. You have been around long enough to know that adding a feature of an unknown value is the last thing I would ask, don't you? Given that you actually did ask me to add such a feature when I simply wanted to get a bug fix for fast-export into Git to support Sverre's remote-hg (that he abandoned because of my failure to get the bug fix in), I have to respectfully declare that I do not know that, no, sorry! I am essentially saying this: I do not see why and the patch and its documentation do not explain why it is useful, but Dscho wouldn't have added the feature without a reason better than just because we can do so, so let's assume the mode is useful for some unknown reason. Would people find updateInstead if able otherwise detachInstead even more useful for that same unknown reason? Okay, here is my explanation: at the time I wanted to disprove that updateInstead could make sense, I wanted to offer a milder version of updating the current branch that left the working directory alone: detachInstead. Now, I never used it myself, but I use updateInstead extensively. So now I offer you two choices: - help me come up with a good scenario where it makes sense to detachInstead, or - tell me to drop it. I have no preference. Ciao, Johannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Johannes Schindelin johannes.schinde...@gmx.de writes: Okay, here is my explanation: at the time I wanted to disprove that updateInstead could make sense, I wanted to offer a milder version of updating the current branch that left the working directory alone: detachInstead. Now, I never used it myself, but I use updateInstead extensively. Sounds like updateInstead turned out to be useful enough to make a possibly more cautious detachInstead unnecessary? It probably makes sense not to add it in that case, I would think. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Hi Junio,
On Fri, 7 Nov 2014, Junio C Hamano wrote:
Johannes Schindelin johannes.schinde...@gmx.de writes:
Under certain circumstances, it makes a *lot* of sense to allow pushing
into the current branch. For example, when two machines with different
Operating Systems are required for testing, it makes much more sense to
synchronize between working directories than having to go via a third
server.
Even if you do not have a third server, each time you decide to do
such a push, you have a single source of truth, i.e. the repository
you are pushing from, so instead of doing that push, all the others
could instead pull from that single source of truth. In that sense,
even though I wouldn't say it is wrong to use push in the opposite
direction for this synchronization, I have to say that the above is
not a very strong argument. It certainly does not deserve *lot*
in bold font ;-)
I am sorry, I should have been more explicit about the fact that other
Operating System includes also Windows, where it is a major hassle to
set up an ssh daemon, hence the asymmetry of ease to connect from one to
another machine. I really thought that was obvious, my apologies. My next
patch iteration will have the Windows scenario as motivation.
Note that I did not even dive into the problem of many loaner laptops
where you are not allowed to set up an ssh daemon, or even know the
user's password. I did not mention those problems because I again assumed
(again, apologies) that this was obvious because it occurred to me
automatically when thinking about the multi-laptop problem.
And I must apologize yet again, for I also failed to specify explicitly
another reason why pushing makes a *lot* more sense than pulling: at least
for me, personally, having to switch keyboards just to synchronize Git
checkouts from one to another computer *is* a hassle.
Under different circumstances, the working directory needs to be left
untouched, for example when a bunch of VMs need to be shut down to save
RAM and one needs to push everything out into the host's non-bare
repositories quickly.
For this use case, if you assume that the tips of branches in the
repositories on a bunch of VMs could be pointing at different
commits (i.e. each of them has acquired more work without
coordination), you are risking lossage by pushing into refs/heads/,
not refs/remotes/vm$n/, aren't you? When you want to save things
away quickly, you do not want to be worried about a push from VM1
stomping on what was stored from VM0, and using separate remotes,
i.e. refs/remotes/vm$n/, has been the recommended way to do so. So
this is not a very strong argument, either.
I thank you for your assessment of my personal working style.
:-)
And yet again I have to apologize because I find that relying on Git's
fast-forward default (you need to force non-fast-forward ref updates,
which I don't, at least not by default) saves me from that lossage, and
will therefore not change my personal working style that served me so well
for years.
I do not think of a good justification of detachInstead offhand, but
you must have thought things through a lot more than I did, so you
can come up with a work flow description that is more usable by mere
mortals to justify that mode.
The main justification is that it is safer than updateInstead because it
is guaranteed not to modify the working directory. I intended it for use
by developers who are uncomfortable with updating the working directory
through a push, and as uncomfortable with forgetting about temporary
branches pushed, say, via git push other-computer HEAD:refs/heads/tmp.
However, I have to admit that I never used this myself after the first few
weeks of playing with this patch series.
Without such a description to justify why detachInstead makes sense, for
example, I cannot even answer this simple question:
Would it make sense to have a third mode, check out if the
working tree is clean, detach otherwise?
I imagine that some developer might find that useful. If you insist, I
will implement it, even if I personally deem this mode way too complicated
a concept for myself to be used safely.
[... snip a lot of text that made me almost miss the comments below ...]
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..be4172f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -26,7 +26,9 @@ enum deny_action {
DENY_UNCONFIGURED,
DENY_IGNORE,
DENY_WARN,
- DENY_REFUSE
+ DENY_REFUSE,
+ DENY_UPDATE_INSTEAD,
+ DENY_DETACH_INSTEAD,
Don't add trailing comma after the last element of enum.
Will fix in the next iteration.
@@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd,
struct shallow_info *si)
return 0;
}
+static void merge_worktree(unsigned char *sha1)
+{
+ const char *update_refresh[] = {
+ update-index, --refresh, NULL
+ };
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Hi Peff, On Sat, 8 Nov 2014, Jeff King wrote: On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote: Under certain circumstances, it makes a *lot* of sense to allow pushing into the current branch. For example, when two machines with different Operating Systems are required for testing, it makes much more sense to synchronize between working directories than having to go via a third server. FWIW, I do this without a third server (and without resorting to pull), with: host1$ git push host2 master:refs/remotes/host1/master host2$ git merge host1/master You can even set up a push refspec to make git push host2 do the right thing. That being said, I do like the premise of your patch, as it eliminates the extra step on the remote side (which is not that big a deal in itself, but when you realize that host2 _did_ have some changes on it, then you end up doing the merge there, when in general I'd prefer to do all the work on host1 via git pull). Plus: you have the luxury of working on an OS that makes ssh'ing from another machine relatively easy. At least if you have the root password on your machine. Which, I hate to point it out, is not too common a commodity. Ciao, Dscho -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Johannes Schindelin johannes.schinde...@gmx.de writes: I do not think of a good justification of detachInstead offhand, but you must have thought things through a lot more than I did, so you can come up with a work flow description that is more usable by mere mortals to justify that mode. The main justification is that it is safer than updateInstead because it is guaranteed not to modify the working directory. I intended it for use by developers who are uncomfortable with updating the working directory through a push, and as uncomfortable with forgetting about temporary branches pushed, say, via git push other-computer HEAD:refs/heads/tmp. However, I have to admit that I never used this myself after the first few weeks of playing with this patch series. Without such a description to justify why detachInstead makes sense, for example, I cannot even answer this simple question: Would it make sense to have a third mode, check out if the working tree is clean, detach otherwise? I imagine that some developer might find that useful. If you insist, I will implement it, even if I personally deem this mode way too complicated a concept for myself to be used safely. You have been around long enough to know that adding a feature of an unknown value is the last thing I would ask, don't you? I am essentially saying this: I do not see why and the patch and its documentation do not explain why it is useful, but Dscho wouldn't have added the feature without a reason better than just because we can do so, so let's assume the mode is useful for some unknown reason. Would people find updateInstead if able otherwise detachInstead even more useful for that same unknown reason? So a good answer would have been one of these: * detachInstead is not backed by a use case as useful as updateInstead, and was done more from because we can while at it. Let's drop it for now. * detachInstead is a good thing and here is an updated patch to better explain the readers of our documentation the workflow to use to the feature well. updateIfAbleOrDetachInstead would be useful for the same reason stated in the updated documentation, but let's not make the scope of the topic too wide and stop at mentioning the possibility in the log message for now. * detachInstead is a good thing and here is an updated patch to better explain the readers of our documentation the workflow to use to the feature well. Once a reader understands this, she would realize why updateIfAbleOrDetachInstead would not be useful, so it is not even worth mentioning the possibility. + if (run_command(child)) + die (Could not merge working tree with new HEAD. Good luck.); Drop Good luck. and replace it with a more useful info. At least, tell the user what state you left her repository in by this botched attempt, so that she can manually recover. The best information Git can give at that point is that the working tree could not be merged with the new HEAD. I stripped Good luck. in the next iteration, although I mourn the loss of comedy in Git. Being humourous is good, but Good luck while refusing to be helpful is not humourous; it is just being hostile to the user. We would be showing the string as the reason for push failure to git push in the new code that does not die but signal the failure to our caller, so could not merge working tree with new HEAD is good (we shouldn't be doing the advice thing here). Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote: Under certain circumstances, it makes a *lot* of sense to allow pushing into the current branch. For example, when two machines with different Operating Systems are required for testing, it makes much more sense to synchronize between working directories than having to go via a third server. FWIW, I do this without a third server (and without resorting to pull), with: host1$ git push host2 master:refs/remotes/host1/master host2$ git merge host1/master You can even set up a push refspec to make git push host2 do the right thing. That being said, I do like the premise of your patch, as it eliminates the extra step on the remote side (which is not that big a deal in itself, but when you realize that host2 _did_ have some changes on it, then you end up doing the merge there, when in general I'd prefer to do all the work on host1 via git pull). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
On Sat, Nov 08, 2014 at 06:18:55AM -0500, Jeff King wrote: On Fri, Nov 07, 2014 at 02:58:17PM +0100, Johannes Schindelin wrote: Under certain circumstances, it makes a *lot* of sense to allow pushing into the current branch. For example, when two machines with different Operating Systems are required for testing, it makes much more sense to synchronize between working directories than having to go via a third server. FWIW, I do this without a third server (and without resorting to pull), with: host1$ git push host2 master:refs/remotes/host1/master host2$ git merge host1/master You can even set up a push refspec to make git push host2 do the right thing. I do something similar, but it's inconvenient when the repo you're pushing into is $HOME, since you have to type something like exec zsh -l in order to fix things up. That being said, I do like the premise of your patch, as it eliminates the extra step on the remote side (which is not that big a deal in itself, but when you realize that host2 _did_ have some changes on it, then you end up doing the merge there, when in general I'd prefer to do all the work on host1 via git pull). I agree. This is very useful. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
[PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Under certain circumstances, it makes a *lot* of sense to allow pushing
into the current branch. For example, when two machines with different
Operating Systems are required for testing, it makes much more sense to
synchronize between working directories than having to go via a third
server.
Under different circumstances, the working directory needs to be left
untouched, for example when a bunch of VMs need to be shut down to save
RAM and one needs to push everything out into the host's non-bare
repositories quickly.
This change supports both workflows by offering two new values for the
denyCurrentBranch setting:
'updateInstead':
Update the working tree accordingly, but refuse to do so if there
are any uncommitted changes.
'detachInstead':
Detach the HEAD, thereby keeping currently checked-out revision,
index and working directory unchanged.
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
Documentation/config.txt | 5 +
builtin/receive-pack.c | 58 ++--
t/t5516-fetch-push.sh| 36 ++
3 files changed, 97 insertions(+), 2 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index e8dd76d..6fc0d6d 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2129,6 +2129,11 @@ receive.denyCurrentBranch::
print a warning of such a push to stderr, but allow the push to
proceed. If set to false or ignore, allow such pushes with no
message. Defaults to refuse.
++
+There are two more options: updateInstead which will update the working
+directory (must be clean) if pushing into the current branch, and
+detachInstead which will leave the working directory untouched, detaching
+the HEAD so it does not need to change.
receive.denyNonFastForwards::
If set to true, git-receive-pack will deny a ref update which is
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 32fc540..be4172f 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -26,7 +26,9 @@ enum deny_action {
DENY_UNCONFIGURED,
DENY_IGNORE,
DENY_WARN,
- DENY_REFUSE
+ DENY_REFUSE,
+ DENY_UPDATE_INSTEAD,
+ DENY_DETACH_INSTEAD,
};
static int deny_deletes;
@@ -120,7 +122,12 @@ static int receive_pack_config(const char *var, const char
*value, void *cb)
}
if (!strcmp(var, receive.denycurrentbranch)) {
- deny_current_branch = parse_deny_action(var, value);
+ if (value !strcasecmp(value, updateinstead))
+ deny_current_branch = DENY_UPDATE_INSTEAD;
+ else if (value !strcasecmp(value, detachinstead))
+ deny_current_branch = DENY_DETACH_INSTEAD;
+ else
+ deny_current_branch = parse_deny_action(var, value);
return 0;
}
@@ -730,6 +737,44 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
+static void merge_worktree(unsigned char *sha1)
+{
+ const char *update_refresh[] = {
+ update-index, --refresh, NULL
+ };
+ const char *read_tree[] = {
+ read-tree, -u, -m, sha1_to_hex(sha1), NULL
+ };
+ struct child_process child;
+ struct strbuf git_env = STRBUF_INIT;
+ const char *env[2];
+
+ if (is_bare_repository())
+ die (denyCurrentBranch = updateInstead needs a worktree);
+
+ strbuf_addf(git_env, GIT_DIR=%s, absolute_path(get_git_dir()));
+ env[0] = git_env.buf;
+ env[1] = NULL;
+
+ memset(child, 0, sizeof(child));
+ child.argv = update_refresh;
+ child.env = env;
+ child.dir = git_work_tree_cfg ? git_work_tree_cfg : ..;
+ child.stdout_to_stderr = 1;
+ child.git_cmd = 1;
+ if (run_command(child))
+ die (Could not refresh the index);
+
+ child.argv = read_tree;
+ child.no_stdin = 1;
+ child.no_stdout = 1;
+ child.stdout_to_stderr = 0;
+ if (run_command(child))
+ die (Could not merge working tree with new HEAD. Good luck.);
+
+ strbuf_release(git_env);
+}
+
static const char *update(struct command *cmd, struct shallow_info *si)
{
const char *name = cmd-ref_name;
@@ -760,6 +805,13 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
if (deny_current_branch == DENY_UNCONFIGURED)
refuse_unconfigured_deny();
return branch is currently checked out;
+ case DENY_UPDATE_INSTEAD:
+ merge_worktree(new_sha1);
+ break;
+ case DENY_DETACH_INSTEAD:
+ update_ref(push into current branch (detach), HEAD,
+ old_sha1, NULL, REF_NODEREF,
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Johannes Schindelin johannes.schinde...@gmx.de writes: Under certain circumstances, it makes a *lot* of sense to allow pushing into the current branch. For example, when two machines with different Operating Systems are required for testing, it makes much more sense to synchronize between working directories than having to go via a third server. Even if you do not have a third server, each time you decide to do such a push, you have a single source of truth, i.e. the repository you are pushing from, so instead of doing that push, all the others could instead pull from that single source of truth. In that sense, even though I wouldn't say it is wrong to use push in the opposite direction for this synchronization, I have to say that the above is not a very strong argument. It certainly does not deserve *lot* in bold font ;-) Under different circumstances, the working directory needs to be left untouched, for example when a bunch of VMs need to be shut down to save RAM and one needs to push everything out into the host's non-bare repositories quickly. For this use case, if you assume that the tips of branches in the repositories on a bunch of VMs could be pointing at different commits (i.e. each of them has acquired more work without coordination), you are risking lossage by pushing into refs/heads/, not refs/remotes/vm$n/, aren't you? When you want to save things away quickly, you do not want to be worried about a push from VM1 stomping on what was stored from VM0, and using separate remotes, i.e. refs/remotes/vm$n/, has been the recommended way to do so. So this is not a very strong argument, either. Doing things only within refs/heads/ without using any refs/remotes/ however is a handy option, especially for those who know what they are doing, and I do not have problem with an effort to help improving such a workflow. It just feels unnecessary, and inviting mistakes, to try to mix that with checked out branches. For example, if these VMs are mostly for building and making fix-up commits, I would imagine another possible flow might be: * Each repository on these bochs would have refs/heads/* but not refs/remotes/* and is on detached HEAD. * Pushing into these repositories will trigger a hook that performs a checkout of the commit at the tip of the updated branch to the detached HEAD, and use of git checkout in that hook will prevent lossage of local changes automatically. * After making fix-up commits on one VMs repository, you would first locally push HEAD:$branch to update the refs/heads/$branch and propagate that to others by pushing $branch. Now, with updateInstead, you can keep the target branch checked out, instead of detaching HEAD at the tip of it and having a hook to run git checkout, and automatically update the working tree. The workflow would become: * Each repository on these bochs would have refs/heads/* but not refs/remotes/*. Each have a branch checked out and some or all may be on the same branch. * Pushing into these repositories will trigger updateInstead to update the working tree in a safe way. * After making fix-up commits on one VMs repository, propagate that change to other VMs by pushing the branch out. which is a definite improvement. Explained in that way, I would say updateInstead is a welcome addition. The documentation would probably need to explain an expected use (something like what I gave above), so that this can be useful not only by those who know what they are doing, though. I do not think of a good justification of detachInstead offhand, but you must have thought things through a lot more than I did, so you can come up with a work flow description that is more usable by mere mortals to justify that mode. Without such a description to justify why detachInstead makes sense, for example, I cannot even answer this simple question: Would it make sense to have a third mode, check out if the working tree is clean, detach otherwise? This change supports both workflows by offering two new values for the denyCurrentBranch setting: 'updateInstead': Update the working tree accordingly, but refuse to do so if there are any uncommitted changes. 'detachInstead': Detach the HEAD, thereby keeping currently checked-out revision, index and working directory unchanged. Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de --- Documentation/config.txt | 5 + builtin/receive-pack.c | 58 ++-- t/t5516-fetch-push.sh| 36 ++ 3 files changed, 97 insertions(+), 2 deletions(-) diff --git a/Documentation/config.txt b/Documentation/config.txt index e8dd76d..6fc0d6d 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -2129,6 +2129,11 @@ receive.denyCurrentBranch:: print a warning of such a push to stderr, but allow the push to proceed. If set to
Re: [PATCH 1/2] Add a few more values for receive.denyCurrentBranch
Hi Junio, On Fri, 7 Nov 2014, Junio C Hamano wrote: [...] I will address your concerns after the weekend. Ciao, Johannes -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
