Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules
Junio C Hamano gits...@pobox.com writes:
And thinking about the names again, I have a feeling that
updateInstead and mergeInstead are both probably misnomer.
Let me take this part back. After all, I do not think I would
design the mechanism to implement an alternative logic that decides
when it is safe to allow the update of the ref and to reflect the
changes to the working tree, and that actually does the checkout to
the working tree by using a new value like mergeInstead. So we
would only need a single name, and updateInstead is not too bad.
...
The mechanism I would employ when doing an alternative logic,
possibly looser one but does not necessarily so, would be to have a
hook script push-to-checkout. When denyCurrentBranch is set to
updateInstead, if there is no such hook, the working tree has to be
absolutely clean and we would do a 'read-tree -m -u $old $new'
(which is the same as 'reset --hard $new' under the precondition)
you implemented will be used as the logic that decides when it is
safe, and that does the checkout to the working tree. When the
push-to-checkout hook exists, however, we just invoke that hook
with $new as argument, and it can decide when it is safe in whatever
way it chooses to, and it can checkout the $new to the working tree
in whatever way it wants.
So here comes a two-patch series on top of your series (with the
test update I sent earlier). As I never do push to deploy that
requires no changes to the working tree or to the index, while I
have seem myself in a situation where I have to emulate a git pull
with a git push in the opposite direction (and work it around if
the target of the 'git pull' I wanted to do were the current branch,
by first pushing into a throw-away branch, because of denyCurrent),
I could imagine myself using this variant. Having said that, this
is primarily so that I do not want to forget and discard the brain
cycles we spent discussing this to the waste, more than that I
cannot wait to use this feature myself ;-)
The first one here is a pure refactoring. The second one is the
real fun.
-- 8 --
Subject: [PATCH 1/2] receive-pack: refactor updateInstead codepath
Keep the there is nothing to update in a bare repository, when
the check and update process runs, here are the GIT_DIR and
GIT_WORK_TREE logic, which will be common regardless of how the
decision to update and the actual update are done, in the original
update_worktree() function, and split out the working tree and
the index must match the original HEAD exactly and use two-way
read-tree to update the working tree into a new push_to_deploy()
helper function. This will allow customizing the logic more cleanly
and easily.
Signed-off-by: Junio C Hamano gits...@pobox.com
---
builtin/receive-pack.c | 53 ++
1 file changed, 28 insertions(+), 25 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index c047418..11800cd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -733,7 +733,9 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
return 0;
}
-static const char *update_worktree(unsigned char *sha1)
+static const char *push_to_deploy(unsigned char *sha1,
+ struct argv_array *env,
+ const char *work_tree)
{
const char *update_refresh[] = {
update-index, -q, --ignore-submodules, --refresh, NULL
@@ -748,69 +750,70 @@ static const char *update_worktree(unsigned char *sha1)
const char *read_tree[] = {
read-tree, -u, -m, NULL, NULL
};
- const char *work_tree = git_work_tree_cfg ? git_work_tree_cfg : ..;
- struct argv_array env = ARGV_ARRAY_INIT;
struct child_process child = CHILD_PROCESS_INIT;
- if (is_bare_repository())
- return denyCurrentBranch = updateInstead needs a worktree;
-
- argv_array_pushf(env, GIT_DIR=%s, absolute_path(get_git_dir()));
-
child.argv = update_refresh;
- child.env = env.argv;
+ child.env = env-argv;
child.dir = work_tree;
child.no_stdin = 1;
child.stdout_to_stderr = 1;
child.git_cmd = 1;
- if (run_command(child)) {
- argv_array_clear(env);
+ if (run_command(child))
return Up-to-date check failed;
- }
/* run_command() does not clean up completely; reinitialize */
child_process_init(child);
child.argv = diff_files;
- child.env = env.argv;
+ child.env = env-argv;
child.dir = work_tree;
child.no_stdin = 1;
child.stdout_to_stderr = 1;
child.git_cmd = 1;
- if (run_command(child)) {
- argv_array_clear(env);
+ if (run_command(child))
return Working directory has unstaged changes;
- }
child_process_init(child);
child.argv =
Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules
Hi Junio, On Wed, 12 Nov 2014, Junio C Hamano wrote: Instead of running update-index --refresh; read-tree -m -u, using reset --keep may be a better implementation of what you are trying to do here. I do not think that `reset --keep` is what I want. I really want to update only if the working directory is clean. So I guess I will have to bite the bullet and test the output of `update-index --refresh`, `diff-index --quiet --cached HEAD --`. In my case, the lacking test whether there are staged changes did not matter, just because I pretty much never leave staged changes around. What did matter, however, was to make sure that I did not update the working directory carelessly. In one case, that `update-index --refresh` test really helped me out because I was about to push into a working directory with uncommitted changes inside some web space. That push would have broken the web application because of the local changes, so I was really, really happy that I decided to be quite strict in the implementation of `updateInstead`. Due to that experience, the documentation also states pretty clearly that `updateInstead` succeeds only in updating the current branch if the working directory is clean. 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Hi Junio, On Thu, 13 Nov 2014, Johannes Schindelin wrote: Due to that experience, the documentation also states pretty clearly that `updateInstead` succeeds only in updating the current branch if the working directory is clean. To clarify why `updateInstead` is stricter than the `merge` scenario: when pushing into a remote repository with attached working directory, we cannot simply clean up merge conflicts or other problems introduced by a merge. Indeed, it is even possible to push without having any option to fix files in the working directory afterwards. Therefore, the `updateInstead` feature really needs to be adamant about the working directory being clean. 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Johannes Schindelin johannes.schinde...@gmx.de writes: On Thu, 13 Nov 2014, Johannes Schindelin wrote: Due to that experience, the documentation also states pretty clearly that `updateInstead` succeeds only in updating the current branch if the working directory is clean. To clarify why `updateInstead` is stricter than the `merge` scenario: when pushing into a remote repository with attached working directory, we cannot simply clean up merge conflicts or other problems introduced by a merge. Indeed, it is even possible to push without having any option to fix files in the working directory afterwards. Therefore, the `updateInstead` feature really needs to be adamant about the working directory being clean. As the stricter check (which I think is unnecessarily strict and which you don't) can be loosened later without making something the user used to be able to do in an early version unable to do later, I am OK to accept the design as-is. But after reading this addendum, I feel the need to double check. A reset --keep new-commit, which is the same as checkout -B current new-commit (note the lack of -m), does not have any merge conflict issues. To see what I mean, follow along this simple experiment. # Just make sure we start clean $ git reset --hard # Create a playpen $ git checkout -b throwaway master # Pick one random path that is different. We pretend that # somebody pushed the commit at the tip of 'next' from the side $ git diff --name-only next | head -n 1 Documentation/git-clone.txt # Make sure another random path used in the experiment is unchanged $ git diff next -- COPYING | wc -l 0 # Smudge a path not involved in the branch/HEAD switching $ echo COPYING # attempt to updateInstead to the other version succeeds. $ git reset --keep next; echo $? 0 $ git status -suno M COPYING # Notice that we did not get into a funny state. # You can verify it with git diff. # Go back and try smudging what would need a real merge $ git reset --hard master $ echo Documentation/git-clone.txt # attempt to updateInstead to the other version $ git reset --keep next; echo $? error: Entry 'Documentation/git-clone.txt' not uptodate. Cannot merge. fatal: Could not reset index file to revision 'next'. 128 # See that HEAD did not move $ git log HEAD...master | wc -l 0 # See that the working tree change is intact $ git status -suno M Documentation/git-clone.txt # Notice that we did not get into a funny state. # You can verify it with git diff. The semantics is just as safe as checkout another-branch without the -m option; if a local change may be clobbered or needs real file-level merge, the operation fails without touching anything to preserve the local state. It is OK for us to agree to disagree, and I am willing to accept the stricter design for an initial version because loosening it later is possible without harming users. But I wanted to first make sure that your disagreement is an informed one. Do you still feel the need for absolutely clean, even if you take the safety built into reset --keep shown in the above experiment into account? -- 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Hi Junio,
On Thu, 13 Nov 2014, Junio C Hamano wrote:
Johannes Schindelin johannes.schinde...@gmx.de writes:
On Thu, 13 Nov 2014, Johannes Schindelin wrote:
Due to that experience, the documentation also states pretty clearly that
`updateInstead` succeeds only in updating the current branch if the
working directory is clean.
To clarify why `updateInstead` is stricter than the `merge` scenario: when
pushing into a remote repository with attached working directory, we
cannot simply clean up merge conflicts or other problems introduced by a
merge. Indeed, it is even possible to push without having any option to
fix files in the working directory afterwards. Therefore, the
`updateInstead` feature really needs to be adamant about the working
directory being clean.
As the stricter check (which I think is unnecessarily strict and
which you don't) can be loosened later without making something the
user used to be able to do in an early version unable to do later,
I am OK to accept the design as-is.
Thanks for mentioning this. I would like to ask not to loosen this later.
Let me try to explain in more detail than before why I think it would make
*my* life hard if it ever were loosened.
Let's start with a hypothetical change to notes.c, a change, say, that
uses part of the diff machinery. So I am doing that in my working
directory, happily testing stuff, but not quite happy with the result just
yet, staging a couple of things after a debug run so I can git stash -k
away the debugging statements.
Please note that while the scenario I just illustrated is purely
hypothetical, made up from thin air, the workflow and the technique is
very much my daily bread, though. I do use git stash -k quite frequently
to stash away stuff that I do not want, after staging what I want to keep
first.
Now let's continue by saying that I get interrupted with that notes.c work
and only come back to it much, much later, say, a couple of months,
because I have been working on a different computer in the meantime,
say, fixing Git for Windows bugs like mad.
Okay, so now I am back and I git pull your 'master'. Now, for the sake
of the argument, let's say that the pull does not introduce any changes to
notes.c, but instead that there has been an improvement in the diff
machinery, one that required a change of the diff machinery's API and all
of a sudden, my code does not compile anymore. It merged cleanly,
Git is completely groovy, but the code just does not compile any more. Not
a big deal: I can inspect the changes via git diff
junio/master@{1}..junio/master and find out very easily what went wrong
and fix my code, compile, stage, and maybe even commit because the code
now works.
Splendid.
But it would be different if the working directory was on another computer
than the one I am operating from. I could not easily access the working
directory, let alone the Git history, if all I did was a git push.
Now let's modify the scenario a little bit further, still. Let's not talk
about git.git, but about a project implementing a web application. And
let's say that I did not dabble with notes.c but I had to work on the
production setup for a really, really urgent hotfix where the speed of a
fix was more important than not touching the production setup at all.
Please note that this is *not* a hypothetical scenario, but a true account
of what I had to go through a couple of times (don't try this at home, it
causes a lot of gray hair).
Now let's assume that I forgot to commit and push, and that nobody noticed
– and let's just draw the curtain of charity over the reasons why. Also,
let's assume that a couple of months later, I am asked to implement a new
feature for the very same web application, this time without frantic You.
Have. To. Fix. This. ASAP! stuff going on. So I develop this on a test
machine, test thoroughly, everything's groovy, and then I push, using the
'updateInstead' feature introduced by the patch in question (and which was
applied to that machine's Git at that time, before some joker updated it
with the official Git).
And now when I try to push, Git complains that the working directory is
not clean, which is *just* fine by me, because after further inspection it
turns out that the uncommitted changes – which are in a different file
than the changes introducing my new feature – would have borked the
production system rather thoroughly.
Please note again that I am talking about something that really happened.
So I would really like to have 'updateInstead', but only in the safe
version, i.e. refusing to update anything but a clean working directory,
and it would be a major regression for me, affecting my workflow rather
negatively, if those strict checks were loosened.
In my mind, when (and if!) a less strict version is desired, it should be
added as another denyCurrentBranch value and 'updateInstead' should be
unaffected, otherwise, speaking for me personally, all my work in this
patch
Re: [PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules
Johannes Schindelin johannes.schinde...@gmx.de writes: Thanks for mentioning this. I would like to ask not to loosen this later. Let me try to explain in more detail than before why I think it would make *my* life hard if it ever were loosened. ... And now when I try to push, Git complains that the working directory is not clean, which is *just* fine by me, because after further inspection it turns out that the uncommitted changes – which are in a different file than the changes introducing my new feature – would have borked the production system rather thoroughly. ... In my mind, when (and if!) a less strict version is desired, it should be added as another denyCurrentBranch value and 'updateInstead' should be unaffected, otherwise, speaking for me personally, all my work in this patch series would be for naught. Thanks for an explanation why the updateInstead mode must require a pristine working tree (and the index). I *now* agree why such a mode would be useful, *after* reading it. I did not understand why *after* reading only the patch, the documentation updates and the log message. That tells us something, doesn't it? ;-) Also the failure case test must protect us from making a change you fear in the future. The interdiff you sent in a separate message was to smudge path2 that is modified in the 'fourth' commit, which should fail with either your OK only if really clean requirement or the other OK as long as it does not interfere with the switch criterion. Checking that is a good step, but you would want to have a separate Smudge a path that is unchanged with the switch and see the push updateInstead fail to protect the current semantics. I agree that a new value mergeInstead or something should be invented when/if different workflows want a looser semantics. People would rely not only on being able to push when clean but also on being safely prevented from pushing when not (and that is where my earlier comment to test both sides comes from). Loosening the check to an existing updateInstead would break users who have been using updateInstead. Also I suspect that people would want to send a patch to add -q to your update-index --refresh invocation, at which time you would need to add a call to diff-files to check that the working tree and the index match. You may want to add that diff-files now, or at least have a test that is designed to break when -q is added to update-index --refresh without adding the necessary diff-files. 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Junio C Hamano gits...@pobox.com writes: Thanks for an explanation why the updateInstead mode must require a pristine working tree (and the index). I *now* agree why such a mode would be useful, *after* reading it. I did not understand why *after* reading only the patch, the documentation updates and the log message. That tells us something, doesn't it? ;-) Just to be less cryptic, I meant that the documentation since v2 is probably insufficient. I agree that a new value mergeInstead or something should be invented when/if different workflows want a looser semantics. People would rely not only on being able to push when clean but also on being safely prevented from pushing when not (and that is where my earlier comment to test both sides comes from). Loosening the check to an existing updateInstead would break users who have been using updateInstead. And thinking about the names again, I have a feeling that updateInstead and mergeInstead are both probably misnomer. The make sure there is no modification and then do an equivalent of reset --hard there option makes sense only in push-to-deploy scenario (perhaps resetInstead?) Compared to that, do an equivalent of checkout as long as it can be done without involving real merges sounds more deserving the updateInstead name. But I am not very good at names (and I suspect you feel you yourself aren't, either), so perhaps somebody listening from the sideline can chime in. -- 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Junio C Hamano gits...@pobox.com writes: I agree that a new value mergeInstead or something should be invented when/if different workflows want a looser semantics. People would rely not only on being able to push when clean but also on being safely prevented from pushing when not (and that is where my earlier comment to test both sides comes from). Loosening the check to an existing updateInstead would break users who have been using updateInstead. And thinking about the names again, I have a feeling that updateInstead and mergeInstead are both probably misnomer. Let me take this part back. After all, I do not think I would design the mechanism to implement an alternative logic that decides when it is safe to allow the update of the ref and to reflect the changes to the working tree, and that actually does the checkout to the working tree by using a new value like mergeInstead. So we would only need a single name, and updateInstead is not too bad. The word update is so heavily loaded in the context of accepting a push (i.e. it is unclear what update we are talking about---updating the ref that we normally refuse to update? updating the index? updating the working tree? Some combination of them?), so as a single word, checkoutInstead may probably be a better one, though. Upon hearing checkout, by definition anybody would know that we are involving the working tree. The mechanism I would employ when doing an alternative logic, possibly looser one but does not necessarily so, would be to have a hook script push-to-checkout. When denyCurrentBranch is set to updateInstead, if there is no such hook, the working tree has to be absolutely clean and we would do a 'read-tree -m -u $old $new' (which is the same as 'reset --hard $new' under the precondition) you implemented will be used as the logic that decides when it is safe, and that does the checkout to the working tree. When the push-to-checkout hook exists, however, we just invoke that hook with $new as argument, and it can decide when it is safe in whatever way it chooses to, and it can checkout the $new to the working tree in whatever way it wants. The users of mergeInstead (now a dead and unnecessary name) mode would just have a single-liner #!/bin/sh git reset --keep $1 -- in there, as this single command would both decide when it is safe and does the safe (according to its own definition) updating of the working tree. In your other example (not the deploy to live website one) of unidirectional SSH connection, you would be able to connect from machine A to machine B but not the other way, so while sitting on machine A you would typically have one SSH session to have an interactive shell session running on machine B. You may have local modification on machine B but your changes to history on machine A cannot be pulled, so you would emulate it by pushing from A into B. In such a case, unlike the live website example, it would be useful to loosen the condition even more than reset --keep (which is an equivalent of checkout -B $current_branch $new_commit). In such a case, what you want to do is to simulate git pull that could conflict and give you a chance to resolve with a push in the reverse direction. You want to run an equivalent of the same checkout -B command but with the -m option when accepting such a push. There are other definitions of what is safe and how update should happen depending on the user, and such a logic can be placed in the push-to-checkout hook without harming other users. -- 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Hi Junio, On Mon, 10 Nov 2014, Junio C Hamano wrote: Johannes Schindelin johannes.schinde...@gmx.de writes: By the way, if the expected use case of updateInstead is what I outlined in the previous message, would it make more sense not to fail with update-index --refresh failure (i.e. the working tree files have no changes since the index)? Thinking about it a bit more, checking with update-index --refresh feels doubly wrong. You not just want the working tree files to be pristine with respect to the index, but also you do not want to see any change between the index and the original HEAD, i.e. $ git reset --hard echo Makefile ; git add Makefile $ git update-index --refresh ; echo $? 0 this is not a good state from which you would want to update the working tree. Wouldn't the two-tree form read-tree -u -m that is the equivalent to branch switching do a sufficient check? That is indeed what the patched code calls. I know that ;-), but I think I wasn't clear enough. I am not saying you are not using two-tree read-tree. I am saying the check with update-index --refresh before read-tree seems dubious. There are three cleanliness we require in various occasions: (1) rebase asks you to have your index and the working tree match HEAD exactly, as if just after reset --hard HEAD. (2) merge asks you to have your index match HEAD exactly (i.e. no output from diff --cached HEAD), and have no changes in the working tree at paths that are involved in the operation. It is OK to have local changes in the working tree (but not in the index) at paths that are not involved in a mergy operation. (3) checkout asks you to have your index and the working tree match either HEAD or the commit you are switching to exactly at paths that are involved in the operation. It is OK to have local changes in the working tree and in the index at paths that are not different between the commits you are moving between, and it is also OK to have the contents from the new commit in the working tree and the index at paths that are different between the two. Dying when update-index --refresh signals a difference is an attempt to mimic #1, but it is in line with the spirit of the reason why a user would want to use updateInstead, I think. The situation is more like the person who pushed into your repository from sideline did a checkout -B $current_branch $new_commit to update the HEAD, the index and the working tree, to let you pretend as if you based your work on the commit he pushed to you. While you still need to error out when your local work does not satisfy the cleanliness criteria #3 above, I do not think you would want to stop the operation when checkout would not fail, e.g. you have a local change that does not interfere with the update between the two commits, with this one: + if (run_command(child)) + die (Could not refresh the index); When refreshed the index successfully, we signal that there were differences between the index and the working tree with a non-zero return value, so Could not refresh is not quite right, either. But this one that checks the exit status from two-tree read-tree + if (run_command(child)) + die (Could not merge working tree with new HEAD. Good luck.); is checking the right condition, i.e. cleanliness #3. The disposition should not be die, but an error return to tell the caller to abort the push as we discussed earlier. I have to say that I am quite puzzled now: I guess that you want me to drop the update-index --refresh call? 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Hi Junio, On Mon, 10 Nov 2014, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Dying when update-index --refresh signals a difference is an attempt to mimic #1, but it is in line with the spirit of the reason why a user would want to use updateInstead, I think. The situation is more like the person who pushed into your repository from sideline did a checkout -B $current_branch $new_commit to update the HEAD, the index and the working tree, to let you pretend as if you based your work on the commit he pushed to you. While you still need to error out when your local work does not satisfy the cleanliness criteria #3 above, I do not think you would want to stop the operation when checkout would not fail, e.g. you have a local change that does not interfere with the update between the two commits, with this one: + if (run_command(child)) + die (Could not refresh the index); When refreshed the index successfully, we signal that there were differences between the index and the working tree with a non-zero return value, so Could not refresh is not quite right, either. Just to make sure. I am *not* saying that you do not need to run update-index --refresh. It is necessary before running read-tree to avoid false dirtyness, so you do need to run it. I am only saying that it is too strict to fail the operation when the command reports that you have a local modification in the working tree. Okay, now I am even more puzzled. I guess you actually meant to say that I need to convert the die() into a return? If so, I agree fully. 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Johannes Schindelin johannes.schinde...@gmx.de writes: Hi Junio, On Mon, 10 Nov 2014, Junio C Hamano wrote: Junio C Hamano gits...@pobox.com writes: Dying when update-index --refresh signals a difference is an attempt to mimic #1, but it is in line with the spirit of the reason why a user would want to use updateInstead, I think. The situation is more like the person who pushed into your repository from sideline did a checkout -B $current_branch $new_commit to update the HEAD, the index and the working tree, to let you pretend as if you based your work on the commit he pushed to you. While you still need to error out when your local work does not satisfy the cleanliness criteria #3 above, I do not think you would want to stop the operation when checkout would not fail, e.g. you have a local change that does not interfere with the update between the two commits, with this one: + if (run_command(child)) + die (Could not refresh the index); When refreshed the index successfully, we signal that there were differences between the index and the working tree with a non-zero return value, so Could not refresh is not quite right, either. Just to make sure. I am *not* saying that you do not need to run update-index --refresh. It is necessary before running read-tree to avoid false dirtyness, so you do need to run it. I am only saying that it is too strict to fail the operation when the command reports that you have a local modification in the working tree. Okay, now I am even more puzzled. I guess you actually meant to say that I need to convert the die() into a return? If so, I agree fully. No. update-index --refresh does two things. (a) For performance reasons, plumbing commands such as diff-files and read-tree do not refresh the stat bits in the index. They expect to be run from scripted Porcelain commands, and expect that the caller would refresh the stat bits before they are called to prevent them from mistakingly seeing that an unmodified existing file, after touch existing-file, as modified. And update-index --refresh is the way for the caller to do so. (b) update-index --refresh indicates with its exit status if the working tree files match what is recorded in the index. This can be used to see if diff-files would report difference. As you are going to run read-tree -m -u, you need to refresh the stat bits for purpose (a) above, i.e. to avoid read-tree from failing due to a difference that does not exist. Because I am assuming that your cleanliness requirement to update the working tree is criterion #3 in the previous message, I do not think you would want to abort the update only because there are some difference between the index and the working tree. That means that checking the exit status of update-index --refresh and dying (or signaling the failure to the caller by returning a non-NULL string, in the context of this call path) is not what you want. You may have a local change to Makefile in the working tree of the repository that you are pushing into, and there may not be any change to the Makefile between the original HEAD the working tree is based on and the updated HEAD you are pushing into the repository. update-index --refresh will say You have a local change. and exit with non-zero status, but just like git checkout another to switch to another branch while you have some local change that does not overlap with the difference between branches does not fail, you would want to allow the update. You may be trying to use a cleanliness requirement that is different from criterion #3 in the previous message, but checking the exit status from update-index --refresh does not make much sense in that case either. I do not think you want to have: * pushing into a repository that did edit Makefile; git add Makefile succeeds. * pushing into a repository that did edit Makefile without git add Makefile fails. but that is what you will get, because update-index --refresh would say Your working tree matches the index by exiting with 0 in the former case, and you will end up running read-tree -m -u. Having said all that. Instead of running update-index --refresh; read-tree -m -u, using reset --keep may be a better implementation of what you are trying to do here. I think a checkout -B current-branch new-commit is what you want to run when a push attempts to update the current branch from sideways with updateInstead, and reset --keep new-commit lets you run an equivalent of the checkout -B current-branch but you do not have to know the name of the current-branch. Also by using reset --keep, you do not have to worry about refreshing the index. 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Hi Junio,
On Fri, 7 Nov 2014, Junio C Hamano wrote:
Johannes Schindelin johannes.schinde...@gmx.de writes:
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
builtin/receive-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index be4172f..4ba51df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd,
struct shallow_info *si)
static void merge_worktree(unsigned char *sha1)
{
const char *update_refresh[] = {
- update-index, --refresh, NULL
+ update-index, --ignore-submodules, --refresh, NULL
};
const char *read_tree[] = {
read-tree, -u, -m, sha1_to_hex(sha1), NULL
I suspect that you did not squash this into 1/2 on purpose, and I am
guessing the reason is because you were unsure what should happen
when there were differences in submodules' working trees (otherwise,
you would have simply squashed without oops it was a thinko to
forget passing this option as a separate patch). I am not sure
either.
This change is squashed into the first patch in the next iteration.
By the way, if the expected use case of updateInstead is what I
outlined in the previous message, would it make more sense not to
fail with update-index --refresh failure (i.e. the working tree
files have no changes since the index)?
Thinking about it a bit more, checking with update-index --refresh
feels doubly wrong. You not just want the working tree files to be
pristine with respect to the index, but also you do not want to see
any change between the index and the original HEAD, i.e.
$ git reset --hard echo Makefile ; git add Makefile
$ git update-index --refresh ; echo $?
0
this is not a good state from which you would want to update the
working tree.
Wouldn't the two-tree form read-tree -u -m that is the equivalent
to branch switching do a sufficient check?
That is indeed what the patched code calls.
Also, regarding the new calls to die() in the main patch, shouldn't
they just be returning the error reason in string form, just like
DENY_REFUSE returns branch is currently checked out to signal a
push failure to the caller?
Changed in the next iteration.
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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Hi Jens,
On Sun, 9 Nov 2014, Jens Lehmann wrote:
Am 07.11.2014 um 20:20 schrieb Junio C Hamano:
Johannes Schindelin johannes.schinde...@gmx.de writes:
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
builtin/receive-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index be4172f..4ba51df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd,
struct shallow_info *si)
static void merge_worktree(unsigned char *sha1)
{
const char *update_refresh[] = {
- update-index, --refresh, NULL
+ update-index, --ignore-submodules, --refresh, NULL
};
const char *read_tree[] = {
read-tree, -u, -m, sha1_to_hex(sha1), NULL
I suspect that you did not squash this into 1/2 on purpose, and I am
guessing the reason is because you were unsure what should happen
when there were differences in submodules' working trees (otherwise,
you would have simply squashed without oops it was a thinko to
forget passing this option as a separate patch). I am not sure
either.
I think --ignore-submodules is currently the right thing to do here
and would rather squash this into the first commit.
Done.
Thanks,
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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Johannes Schindelin johannes.schinde...@gmx.de writes: By the way, if the expected use case of updateInstead is what I outlined in the previous message, would it make more sense not to fail with update-index --refresh failure (i.e. the working tree files have no changes since the index)? Thinking about it a bit more, checking with update-index --refresh feels doubly wrong. You not just want the working tree files to be pristine with respect to the index, but also you do not want to see any change between the index and the original HEAD, i.e. $ git reset --hard echo Makefile ; git add Makefile $ git update-index --refresh ; echo $? 0 this is not a good state from which you would want to update the working tree. Wouldn't the two-tree form read-tree -u -m that is the equivalent to branch switching do a sufficient check? That is indeed what the patched code calls. I know that ;-), but I think I wasn't clear enough. I am not saying you are not using two-tree read-tree. I am saying the check with update-index --refresh before read-tree seems dubious. There are three cleanliness we require in various occasions: (1) rebase asks you to have your index and the working tree match HEAD exactly, as if just after reset --hard HEAD. (2) merge asks you to have your index match HEAD exactly (i.e. no output from diff --cached HEAD), and have no changes in the working tree at paths that are involved in the operation. It is OK to have local changes in the working tree (but not in the index) at paths that are not involved in a mergy operation. (3) checkout asks you to have your index and the working tree match either HEAD or the commit you are switching to exactly at paths that are involved in the operation. It is OK to have local changes in the working tree and in the index at paths that are not different between the commits you are moving between, and it is also OK to have the contents from the new commit in the working tree and the index at paths that are different between the two. Dying when update-index --refresh signals a difference is an attempt to mimic #1, but it is in line with the spirit of the reason why a user would want to use updateInstead, I think. The situation is more like the person who pushed into your repository from sideline did a checkout -B $current_branch $new_commit to update the HEAD, the index and the working tree, to let you pretend as if you based your work on the commit he pushed to you. While you still need to error out when your local work does not satisfy the cleanliness criteria #3 above, I do not think you would want to stop the operation when checkout would not fail, e.g. you have a local change that does not interfere with the update between the two commits, with this one: + if (run_command(child)) + die (Could not refresh the index); When refreshed the index successfully, we signal that there were differences between the index and the working tree with a non-zero return value, so Could not refresh is not quite right, either. But this one that checks the exit status from two-tree read-tree + if (run_command(child)) + die (Could not merge working tree with new HEAD. Good luck.); is checking the right condition, i.e. cleanliness #3. The disposition should not be die, but an error return to tell the caller to abort the push as we discussed earlier. -- 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Junio C Hamano gits...@pobox.com writes: Dying when update-index --refresh signals a difference is an attempt to mimic #1, but it is in line with the spirit of the reason why a user would want to use updateInstead, I think. The situation is more like the person who pushed into your repository from sideline did a checkout -B $current_branch $new_commit to update the HEAD, the index and the working tree, to let you pretend as if you based your work on the commit he pushed to you. While you still need to error out when your local work does not satisfy the cleanliness criteria #3 above, I do not think you would want to stop the operation when checkout would not fail, e.g. you have a local change that does not interfere with the update between the two commits, with this one: + if (run_command(child)) + die (Could not refresh the index); When refreshed the index successfully, we signal that there were differences between the index and the working tree with a non-zero return value, so Could not refresh is not quite right, either. Just to make sure. I am *not* saying that you do not need to run update-index --refresh. It is necessary before running read-tree to avoid false dirtyness, so you do need to run it. I am only saying that it is too strict to fail the operation when the command reports that you have a local modification in the working tree. But this one that checks the exit status from two-tree read-tree + if (run_command(child)) + die (Could not merge working tree with new HEAD. Good luck.); is checking the right condition, i.e. cleanliness #3. The disposition should not be die, but an error return to tell the caller to abort the push as we discussed earlier. -- 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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Am 07.11.2014 um 20:20 schrieb Junio C Hamano:
Johannes Schindelin johannes.schinde...@gmx.de writes:
They are not affected by the update anyway.
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
builtin/receive-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index be4172f..4ba51df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
static void merge_worktree(unsigned char *sha1)
{
const char *update_refresh[] = {
- update-index, --refresh, NULL
+ update-index, --ignore-submodules, --refresh, NULL
};
const char *read_tree[] = {
read-tree, -u, -m, sha1_to_hex(sha1), NULL
I suspect that you did not squash this into 1/2 on purpose, and I am
guessing the reason is because you were unsure what should happen
when there were differences in submodules' working trees (otherwise,
you would have simply squashed without oops it was a thinko to
forget passing this option as a separate patch). I am not sure
either.
I think --ignore-submodules is currently the right thing to do here
and would rather squash this into the first commit.
By the way, if the expected use case of updateInstead is what I
outlined in the previous message, would it make more sense not to
fail with update-index --refresh failure (i.e. the working tree
files have no changes since the index)?
Thinking about it a bit more, checking with update-index --refresh
feels doubly wrong. You not just want the working tree files to be
pristine with respect to the index, but also you do not want to see
any change between the index and the original HEAD, i.e.
$ git reset --hard echo Makefile ; git add Makefile
$ git update-index --refresh ; echo $?
0
this is not a good state from which you would want to update the
working tree.
Wouldn't the two-tree form read-tree -u -m that is the equivalent
to branch switching do a sufficient check?
Also, regarding the new calls to die() in the main patch, shouldn't
they just be returning the error reason in string form, just like
DENY_REFUSE returns branch is currently checked out to signal a
push failure to the caller?
--
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
[PATCH 2/2] Let deny.currentBranch=updateInstead ignore submodules
They are not affected by the update anyway.
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
builtin/receive-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index be4172f..4ba51df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
static void merge_worktree(unsigned char *sha1)
{
const char *update_refresh[] = {
- update-index, --refresh, NULL
+ update-index, --ignore-submodules, --refresh, NULL
};
const char *read_tree[] = {
read-tree, -u, -m, sha1_to_hex(sha1), NULL
--
2.0.0.rc3.9669.g840d1f9
--
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 2/2] Let deny.currentBranch=updateInstead ignore submodules
Johannes Schindelin johannes.schinde...@gmx.de writes:
They are not affected by the update anyway.
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
builtin/receive-pack.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index be4172f..4ba51df 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -740,7 +740,7 @@ static int update_shallow_ref(struct command *cmd, struct
shallow_info *si)
static void merge_worktree(unsigned char *sha1)
{
const char *update_refresh[] = {
- update-index, --refresh, NULL
+ update-index, --ignore-submodules, --refresh, NULL
};
const char *read_tree[] = {
read-tree, -u, -m, sha1_to_hex(sha1), NULL
I suspect that you did not squash this into 1/2 on purpose, and I am
guessing the reason is because you were unsure what should happen
when there were differences in submodules' working trees (otherwise,
you would have simply squashed without oops it was a thinko to
forget passing this option as a separate patch). I am not sure
either.
By the way, if the expected use case of updateInstead is what I
outlined in the previous message, would it make more sense not to
fail with update-index --refresh failure (i.e. the working tree
files have no changes since the index)?
Thinking about it a bit more, checking with update-index --refresh
feels doubly wrong. You not just want the working tree files to be
pristine with respect to the index, but also you do not want to see
any change between the index and the original HEAD, i.e.
$ git reset --hard echo Makefile ; git add Makefile
$ git update-index --refresh ; echo $?
0
this is not a good state from which you would want to update the
working tree.
Wouldn't the two-tree form read-tree -u -m that is the equivalent
to branch switching do a sufficient check?
Also, regarding the new calls to die() in the main patch, shouldn't
they just be returning the error reason in string form, just like
DENY_REFUSE returns branch is currently checked out to signal a
push failure to the caller?
--
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
