Re: Sample pre-push hook can crash
Thanks for the information Junio. It is just interesting that although the pre-push hook receives the remote_sha value from the server, it does not get 'git merge-base $remote_sha $local_sha' which is what a check that iterated over all outgoing commits would really need. (I'm sure this is a simple-minded assessment. I don't pretend to have spent even an order of magnitude less time working on git than you have. I'm not trying to be foolishly arrogant.) I do think it would be worth replacing the existing example with yours because the existing example will crash anytime somebody's workspace is up to date. Stephen On Fri, Mar 11, 2016 at 3:04 PM, Junio C Hamano wrote: > Stephen Morton writes: > >> That is interesting, so in the case of a non-ff push, there is no way >> for a pre-push hook to know what is being pushed in order to run? > > If you were up-to-date from the other side once: > > ---A---B---C > > and built one new commit on top: > > ---A---B---C---D > > when you attempt to push it out, there are various possibilities. > > An ff situation is simple. They didn't do anything, so the hook > gets "we'd be updating their ref from C to D", and "rev-list C..D" > would tell you that you would need to make sure D is sane. > > If your push does not fast-forward, that would mean something has > happened on the other side while you were working on D. Perhaps > they accepted another commit that you haven't seen: > > ---A---B---C---E > > and because you haven't fetched from them, even though the hook may > say "we'd be updating their ref from E to D", you haven't heard of > E, you do not know where it would be relative to the history you > have: > > E??? > > ---A---B---C---D > > Or perhaps they themselves rewound their history and they now have > this E at the tip: > > ---A---B---C > \ > E > > But again, because you do not yet know where E is relative to your > history, you cannot say C is where you can stop checking your side > of the history. > > Or perhaps they somehow obtained your D without you pushing into > them (e.g. you pushed to the "next" tree and they fixed your commit > and the result was merged there) and have something like this: > > ---A---B---C---D---E > > In this case, D is not even a new commit from their point of view, > and updating their tip E with your old D would lose the fixup E they > made for D, but again, you do not know what E is yet, you cannot say > "they have this already, so there is no check I need to do". > > In summary, you cannot even say which ones you have need to be > checked. > > If you _are_ using @{u} tracking, then you would know at least they > used to have up to C in their repository to limit your check, but > you cannot unconditionally say ref@{u}.. without making sure ref@{u} > makes sense in the first place. > > An obvious alternative for the sample script would be to instead let > the non-ff push pass the pre-push check (as you may have other > arrangements to forbid non-ff pushes) without actually checking > anything. As this sample script is to serve as a sample, I think > such an advanced consideration of loosening checks depending on the > situation should be left to the end 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: Sample pre-push hook can crash
Stephen Morton writes: > That is interesting, so in the case of a non-ff push, there is no way > for a pre-push hook to know what is being pushed in order to run? If you were up-to-date from the other side once: ---A---B---C and built one new commit on top: ---A---B---C---D when you attempt to push it out, there are various possibilities. An ff situation is simple. They didn't do anything, so the hook gets "we'd be updating their ref from C to D", and "rev-list C..D" would tell you that you would need to make sure D is sane. If your push does not fast-forward, that would mean something has happened on the other side while you were working on D. Perhaps they accepted another commit that you haven't seen: ---A---B---C---E and because you haven't fetched from them, even though the hook may say "we'd be updating their ref from E to D", you haven't heard of E, you do not know where it would be relative to the history you have: E??? ---A---B---C---D Or perhaps they themselves rewound their history and they now have this E at the tip: ---A---B---C \ E But again, because you do not yet know where E is relative to your history, you cannot say C is where you can stop checking your side of the history. Or perhaps they somehow obtained your D without you pushing into them (e.g. you pushed to the "next" tree and they fixed your commit and the result was merged there) and have something like this: ---A---B---C---D---E In this case, D is not even a new commit from their point of view, and updating their tip E with your old D would lose the fixup E they made for D, but again, you do not know what E is yet, you cannot say "they have this already, so there is no check I need to do". In summary, you cannot even say which ones you have need to be checked. If you _are_ using @{u} tracking, then you would know at least they used to have up to C in their repository to limit your check, but you cannot unconditionally say ref@{u}.. without making sure ref@{u} makes sense in the first place. An obvious alternative for the sample script would be to instead let the non-ff push pass the pre-push check (as you may have other arrangements to forbid non-ff pushes) without actually checking anything. As this sample script is to serve as a sample, I think such an advanced consideration of loosening checks depending on the situation should be left to the end 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: Sample pre-push hook can crash
That is interesting, so in the case of a non-ff push, there is no way for a pre-push hook to know what is being pushed in order to run? Steve On Thu, Mar 10, 2016 at 4:43 PM, Junio C Hamano wrote: > Stephen Morton writes: > >> The sample pre-push hook provided with git [1] will crash if the local >> repo is not up to date with the remote as $remote_sha is not present >> in the local repo. I'm not sure if this patch is exactly correct, it's >> just provided as an example. >> >> Given that people are likely crafting their own solutions based on the >> examples, it's probably good to get right. > > It's probably good to get right, but I do not think use of @{u} is > making it right, unfortunately. You may not necessarily have @{u} > configured, and you may not even pushing to the configured remote > branch. > > The spirit of the sample hook, I think, is to validate the new > commits you are publishing, so if you cannot even determine which > ones are new and which ones are not, failing the "push" by exiting > with non-zero status is the right behaviour for this sample. > > So perhaps something like this may be more appropriate as an > example. > > templates/hooks--pre-push.sample | 7 ++- > 1 file changed, 6 insertions(+), 1 deletion(-) > > diff --git a/templates/hooks--pre-push.sample > b/templates/hooks--pre-push.sample > index 6187dbf..7ef6780 100755 > --- a/templates/hooks--pre-push.sample > +++ b/templates/hooks--pre-push.sample > @@ -41,7 +41,12 @@ do > fi > > # Check for WIP commit > - commit=`git rev-list -n 1 --grep '^WIP' "$range"` > + commit=`git rev-list -n 1 --grep '^WIP' "$range"` || { > + # we do not even know about the range... > + echo >&2 "Non-ff update to $remote_ref" > + echo >&2 "fetch from there first" > + exit 1 > + } > if [ -n "$commit" ] > then > echo >&2 "Found WIP commit in $local_ref, not pushing" -- 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: Sample pre-push hook can crash
Stephen Morton writes: > The sample pre-push hook provided with git [1] will crash if the local > repo is not up to date with the remote as $remote_sha is not present > in the local repo. I'm not sure if this patch is exactly correct, it's > just provided as an example. > > Given that people are likely crafting their own solutions based on the > examples, it's probably good to get right. It's probably good to get right, but I do not think use of @{u} is making it right, unfortunately. You may not necessarily have @{u} configured, and you may not even pushing to the configured remote branch. The spirit of the sample hook, I think, is to validate the new commits you are publishing, so if you cannot even determine which ones are new and which ones are not, failing the "push" by exiting with non-zero status is the right behaviour for this sample. So perhaps something like this may be more appropriate as an example. templates/hooks--pre-push.sample | 7 ++- 1 file changed, 6 insertions(+), 1 deletion(-) diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample index 6187dbf..7ef6780 100755 --- a/templates/hooks--pre-push.sample +++ b/templates/hooks--pre-push.sample @@ -41,7 +41,12 @@ do fi # Check for WIP commit - commit=`git rev-list -n 1 --grep '^WIP' "$range"` + commit=`git rev-list -n 1 --grep '^WIP' "$range"` || { + # we do not even know about the range... + echo >&2 "Non-ff update to $remote_ref" + echo >&2 "fetch from there first" + exit 1 + } if [ -n "$commit" ] then echo >&2 "Found WIP commit in $local_ref, not pushing" -- 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
Sample pre-push hook can crash
The sample pre-push hook provided with git [1] will crash if the local repo is not up to date with the remote as $remote_sha is not present in the local repo. I'm not sure if this patch is exactly correct, it's just provided as an example. Given that people are likely crafting their own solutions based on the examples, it's probably good to get right. diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample index 6187dbf..99ed984 100755 --- a/templates/hooks--pre-push.sample +++ b/templates/hooks--pre-push.sample @@ -36,9 +36,9 @@ do # New branch, examine all commits range="$local_sha" else # Update to existing branch, examine new commits - range="$remote_sha..$local_sha" + range="@{u}..$local_sha" fi # Check for WIP commit commit=`git rev-list -n 1 --grep '^WIP' "$range"` (This is just something I noticed and thought you might be interested in, but is not important to me. I actually do care about the smudge filter issue.) Stephen [1] https://github.com/git/git/blob/master/templates/hooks--pre-push.sample -- 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