Sample pre-push hook can crash

Sample pre-push hook can crash

[Stephen Morton]

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

Re: Sample pre-push hook can crash

[Junio C Hamano]

Stephen Morton <stephen.c.morton@gmail.com> 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"

Re: Sample pre-push hook can crash

[Stephen Morton]

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 <gitster@pobox.com> wrote:
> Stephen Morton <stephen.c.morton@gmail.com> 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"

Re: Sample pre-push hook can crash

[Junio C Hamano]

Stephen Morton <stephen.c.morton@gmail.com> 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.

Re: Sample pre-push hook can crash

[Stephen Morton]

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 <gitster@pobox.com> wrote:
> Stephen Morton <stephen.c.morton@gmail.com> 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.
>