Re: [PATCH] receive: denyCurrentBranch=updateinstead should not blindly update
Hi Junio,
On Fri, 19 Oct 2018, Junio C Hamano wrote:
> The handling of receive.denyCurrentBranch=updateInstead was added to
> a switch statement that handles other values of the variable, but
> all the other case arms only checked a condition to reject the
> attempted push, or let later logic in the same function to still
> intervene, so that a push that does not fast-forward (which is
> checked after the switch statement in question) is still rejected.
>
> But the handling of updateInstead incorrectly took immediate effect,
> without giving other checks a chance to intervene.
>
> Instead of calling update_worktree() that causes the side effect
> immediately, just note the fact that we will need to call the
> funciton later, and first give other checks chance to reject the
> request. After the update-hook gets a chance to reject the push
> (which happens as the last step in a series of checks), call
> update_worktree() when we earlier detected the need to.
Nicely explained, and the patch looks good to me, too.
Thanks,
Dscho
>
> Reported-by: Rajesh Madamanchi
> Signed-off-by: Junio C Hamano
> ---
> builtin/receive-pack.c | 12 +---
> t/t5516-fetch-push.sh | 8 +++-
> 2 files changed, 16 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index 95740f4f0e..79ee320948 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -1026,6 +1026,7 @@ static const char *update(struct command *cmd, struct
> shallow_info *si)
> const char *ret;
> struct object_id *old_oid = >old_oid;
> struct object_id *new_oid = >new_oid;
> + int do_update_worktree = 0;
>
> /* only refs/... are allowed */
> if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
> @@ -1051,9 +1052,8 @@ static const char *update(struct command *cmd, struct
> shallow_info *si)
> refuse_unconfigured_deny();
> return "branch is currently checked out";
> case DENY_UPDATE_INSTEAD:
> - ret = update_worktree(new_oid->hash);
> - if (ret)
> - return ret;
> + /* pass -- let other checks intervene first */
> + do_update_worktree = 1;
> break;
> }
> }
> @@ -1118,6 +1118,12 @@ static const char *update(struct command *cmd, struct
> shallow_info *si)
> return "hook declined";
> }
>
> + if (do_update_worktree) {
> + ret = update_worktree(new_oid->hash);
> + if (ret)
> + return ret;
> + }
> +
> if (is_null_oid(new_oid)) {
> struct strbuf err = STRBUF_INIT;
> if (!parse_object(the_repository, old_oid)) {
> diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
> index 7a8f56db53..7316365a24 100755
> --- a/t/t5516-fetch-push.sh
> +++ b/t/t5516-fetch-push.sh
> @@ -1577,7 +1577,13 @@ test_expect_success 'receive.denyCurrentBranch =
> updateInstead' '
> test $(git -C .. rev-parse master) = $(git rev-parse HEAD) &&
> git diff --quiet &&
> git diff --cached --quiet
> - )
> + ) &&
> +
> + # (6) updateInstead intervened by fast-forward check
> + test_must_fail git push void master^:master &&
> + test $(git -C void rev-parse HEAD) = $(git rev-parse master) &&
> + git -C void diff --quiet &&
> + git -C void diff --cached --quiet
> '
>
> test_expect_success 'updateInstead with push-to-checkout hook' '
> --
> 2.19.1-450-ga4b8ab5363
>
>
Re: [PATCH] receive: denyCurrentBranch=updateinstead should not blindly update
On Fri, Oct 19, 2018 at 12:57 AM Junio C Hamano wrote: > The handling of receive.denyCurrentBranch=updateInstead was added to > a switch statement that handles other values of the variable, but > all the other case arms only checked a condition to reject the > attempted push, or let later logic in the same function to still > intervene, so that a push that does not fast-forward (which is > checked after the switch statement in question) is still rejected. > > But the handling of updateInstead incorrectly took immediate effect, > without giving other checks a chance to intervene. > > Instead of calling update_worktree() that causes the side effect > immediately, just note the fact that we will need to call the > funciton later, and first give other checks chance to reject the s/funciton/function s/chance/a &/ > request. After the update-hook gets a chance to reject the push > (which happens as the last step in a series of checks), call > update_worktree() when we earlier detected the need to. > > Reported-by: Rajesh Madamanchi > Signed-off-by: Junio C Hamano
[PATCH] receive: denyCurrentBranch=updateinstead should not blindly update
The handling of receive.denyCurrentBranch=updateInstead was added to
a switch statement that handles other values of the variable, but
all the other case arms only checked a condition to reject the
attempted push, or let later logic in the same function to still
intervene, so that a push that does not fast-forward (which is
checked after the switch statement in question) is still rejected.
But the handling of updateInstead incorrectly took immediate effect,
without giving other checks a chance to intervene.
Instead of calling update_worktree() that causes the side effect
immediately, just note the fact that we will need to call the
funciton later, and first give other checks chance to reject the
request. After the update-hook gets a chance to reject the push
(which happens as the last step in a series of checks), call
update_worktree() when we earlier detected the need to.
Reported-by: Rajesh Madamanchi
Signed-off-by: Junio C Hamano
---
builtin/receive-pack.c | 12 +---
t/t5516-fetch-push.sh | 8 +++-
2 files changed, 16 insertions(+), 4 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 95740f4f0e..79ee320948 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -1026,6 +1026,7 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
const char *ret;
struct object_id *old_oid = >old_oid;
struct object_id *new_oid = >new_oid;
+ int do_update_worktree = 0;
/* only refs/... are allowed */
if (!starts_with(name, "refs/") || check_refname_format(name + 5, 0)) {
@@ -1051,9 +1052,8 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
refuse_unconfigured_deny();
return "branch is currently checked out";
case DENY_UPDATE_INSTEAD:
- ret = update_worktree(new_oid->hash);
- if (ret)
- return ret;
+ /* pass -- let other checks intervene first */
+ do_update_worktree = 1;
break;
}
}
@@ -1118,6 +1118,12 @@ static const char *update(struct command *cmd, struct
shallow_info *si)
return "hook declined";
}
+ if (do_update_worktree) {
+ ret = update_worktree(new_oid->hash);
+ if (ret)
+ return ret;
+ }
+
if (is_null_oid(new_oid)) {
struct strbuf err = STRBUF_INIT;
if (!parse_object(the_repository, old_oid)) {
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index 7a8f56db53..7316365a24 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -1577,7 +1577,13 @@ test_expect_success 'receive.denyCurrentBranch =
updateInstead' '
test $(git -C .. rev-parse master) = $(git rev-parse HEAD) &&
git diff --quiet &&
git diff --cached --quiet
- )
+ ) &&
+
+ # (6) updateInstead intervened by fast-forward check
+ test_must_fail git push void master^:master &&
+ test $(git -C void rev-parse HEAD) = $(git rev-parse master) &&
+ git -C void diff --quiet &&
+ git -C void diff --cached --quiet
'
test_expect_success 'updateInstead with push-to-checkout hook' '
--
2.19.1-450-ga4b8ab5363
