threads / rfc / 39637

patchRe: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

Subject: Re: [PATCH] [RFC] Make git-update-ref invoke the update and post-update hooks

## tl;dr

5 messages between Jun 15, 2015 and Jun 16, 2015. Diffs are folded; open one to read it.

replies: 4people: 2as markdown or json

Brian Vandenberg· Jun 15, 2015, 19:49 UTC · lore

From: Junio C Hamano <junkio@cox.net>
Date: 2006-10-08 15:00:32
Petr Baudis <pasky@suse.cz> writes:

Show 8 quoted lines

>> Someone raised a concern that the update and post-update hooks are not
>> invoked at fetch time in the similar way as they are invoked at push
>> time, and the idea sort of makes sense. But this patch goes further - it
>> makes Git invoke those hooks each time a ref is updated in a repository
>> using the git-update-ref command, which I believe makes a lot of sense as
>> well - the behaviour is consistent with the current pushing behaviour
>> and you suddenly finally get a hook where you can properly notify even
>> about fast-forwards etc.

Show 7 quoted lines

>In principle I do not have problem with this approach per-se,
>but I wonder if we were to do this we might want to make
>receive-pack.c::update() and cmd_update_ref() call the same
>underlying function, and make that underlying function implement
>this "ask the hook if updating is ok" dance.  It might even make
>sense to have update-ref honor deny_non_fast_forwards for that
>matter (I am mildly doubtful of this last point, though).

Was this rejected?

I'm tweaking the configuration on our master repo; among other things: I've enabled reflog and disabled altering history or deleting branches through receive-pack, instead requiring that someone make history alterations directly (using update-ref).

update-ref can optionally accept a comment explaining why the history needed to be altered, but we would prefer to make it mandatory. Hooks seem like a natural place to do that, but after running update-ref under strace/truss & looking through builtin/update-ref.c, it looks as though it doesn't make use of hooks.

-Brian

Junio C Hamano· Jun 15, 2015, 20:22 UTC · re: Brian Vandenberg · lore

Brian Vandenberg <phantall@gmail.com> writes:

Show 20 quoted lines

> From: Junio C Hamano <junkio@cox.net>
> Date: 2006-10-08 15:00:32
> Petr Baudis <pasky@suse.cz> writes:
>
>>> Someone raised a concern that the update and post-update hooks are not
>>> invoked at fetch time in the similar way as they are invoked at push
>>> time, and the idea sort of makes sense. But this patch goes further - it
>>> makes Git invoke those hooks each time a ref is updated in a repository
>>> using the git-update-ref command, which I believe makes a lot of sense as
>>> well - the behaviour is consistent with the current pushing behaviour
>>> and you suddenly finally get a hook where you can properly notify even
>>> about fast-forwards etc.
>
>>In principle I do not have problem with this approach per-se,
>>but I wonder if we were to do this we might want to make
>>receive-pack.c::update() and cmd_update_ref() call the same
>>underlying function, and make that underlying function implement
>>this "ask the hook if updating is ok" dance.  It might even make
>>sense to have update-ref honor deny_non_fast_forwards for that
>>matter (I am mildly doubtful of this last point, though).

I am ultra-doubtful at this point ;-)

For one thing, update-ref is not a tool that is exclusive to receiving object transfer aka receive-pack, so it makes no sense for it to pay attention to post-update.

Also it is a low-level plumbing; the policy issues should come at a level higher than that. I.e. Porcelain scripts implemented using them as building blocks should be the ones that you do your policy, e.g.

	if whatever logic you want to use in your policy says OK
        then
		git update-ref ...args...
	else
        	echo >&2 "My policy does not like your arguments"
                exit 1
	fi

Brian Vandenberg· Jun 15, 2015, 20:58 UTC · re: Junio C Hamano · lore

On Mon, Jun 15, 2015 at 2:22 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 5 quoted lines

> I am ultra-doubtful at this point ;-)
>
> For one thing, update-ref is not a tool that is exclusive to
> receiving object transfer aka receive-pack, so it makes no sense for
> it to pay attention to post-update.

I was a little dubious about that as well, though I like the idea of update-ref having a hook.

Show 13 quoted lines

> Also it is a low-level plumbing; the policy issues should come at a
> level higher than that.  I.e. Porcelain scripts implemented using
> them as building blocks should be the ones that you do your policy,
> e.g.
>
>         if whatever logic you want to use in your policy says OK
>         then
>                 git update-ref ...args...
>         else
>                 echo >&2 "My policy does not like your arguments"
>                 exit 1
>         fi
>

In principle I agree, though telling people "you should use xyz script" is one thing; in practice they're more likely to google "git alter history" and eventually find an explanation for using update-ref, completely bypassing any policy/protocol we put in place for altering history on the master repo.

Would you be less doubtful about adding a lower-level hook for update-ref? Or in lieu of that, a config option that can affect the behavior of its "-m" and "-d" options?

-Brian

Junio C Hamano· Jun 15, 2015, 21:45 UTC · re: Brian Vandenberg · lore

Brian Vandenberg <phantall@gmail.com> writes:

> Would you be less doubtful about adding a lower-level hook for
> update-ref?  Or in lieu of that, a config option that can affect the
> behavior of its "-m" and "-d" options?

Not really.

Those who want to bypass your policy can use "vi .git/packed-refs" and/or "rm .git/refs/heads/foo"; you would not propose to patch "rm" and "vi" to pay attention to git configuration, because they are not about "git" at all, and there are cases where these bare-metal level editing of repositories is necessary. You would instead tell them "don't do that".

When I say "update-ref is a low-level plumbing command that should not enforce policy", I am suggesting you to treat the command just like you would treat "rm" and "vi".

Brian Vandenberg· Jun 16, 2015, 14:31 UTC · re: Junio C Hamano · lore

On Mon, Jun 15, 2015 at 3:45 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 10 quoted lines

> Those who want to bypass your policy can use "vi .git/packed-refs"
> and/or "rm .git/refs/heads/foo"; you would not propose to patch "rm"
> and "vi" to pay attention to git configuration, because they are not
> about "git" at all, and there are cases where these bare-metal level
> editing of repositories is necessary.  You would instead tell them
> "don't do that".
>
> When I say "update-ref is a low-level plumbing command that should
> not enforce policy", I am suggesting you to treat the command just
> like you would treat "rm" and "vi".

I suppose I see your point, though the line they've crossed at that point is from benign to malicious.

When you assume they've crossed that line, hooks and configuration parameters are mostly useless in other git commands as well. My assumption here is they want to do something productive and aren't intentionally seeking to subvert policies, but rather aren't aware of the policies and/or correct procedure.

Thanks for your responses, I appreciate your time.

-Brian