Re: [PATCH 2/5] do not override receive-pack errors
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 13, 2012, 21:41 UTC
- Message-ID
- <7v8vk6csx9.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <1329164235-29955-3-git-send-email-drizzd@aon.at>
Clemens Buchacher <drizzd@aon.at> writes:
> Receive runs rev-list --verify-objects in order to detect missing > objects. However, such errors are ignored and overridden later.
This makes me worried (not about the patch, but about the current code).
Are there codepaths where an earlier pass of verify-objects mark a cmd as bad with a non-NULL error_string, and later code that checks other aspect of the push says the update does not violate its criteria, and flips the non-NULL error_string back to NULL? Or is the only offence you found in such later code that it fills error_string with its own non-NULL string when it finds a violation (and otherwise does not touch error_string)?
In other words, is this really "ignored and overridden", not merely "overwritten"?
In the following review, I assumed that you meant "overwritten".
Show 14 quoted lines
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index fa7448b..0afb8b2 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -642,8 +642,10 @@ static void check_aliased_updates(struct command *commands)
> }
> sort_string_list(&ref_list);
>
> - for (cmd = commands; cmd; cmd = cmd->next)
> - check_aliased_update(cmd, &ref_list);
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!cmd->error_string)
> + check_aliased_update(cmd, &ref_list);
> + }While I agree with the general concept of this patch (i.e. if we know an error exists for a particular ref update, we would want to keep the first one without overwriting it with another error), I am not sure if this hunk is correct. This checks cross reactivity between multiple cmds that can arise when an update made by one will affect the previous value assumed for another cmd because the former cmd updates a symref whose the target is what the later cmd wants to update. If we have already decided the former cmd is deemed to fail and skip this check, we would not catch that the latter cmd is trying to make an inconsistent update request, and we would end up ignoring that case. Is that the right thing to do?
Show 30 quoted lines
> @@ -707,8 +709,10 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
> set_connectivity_errors(commands);
>
> if (run_receive_hook(commands, pre_receive_hook, 0)) {
> - for (cmd = commands; cmd; cmd = cmd->next)
> - cmd->error_string = "pre-receive hook declined";
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (!cmd->error_string)
> + cmd->error_string = "pre-receive hook declined";
> + }
> return;
> }
>
> @@ -717,9 +721,15 @@ static void execute_commands(struct command *commands, const char *unpacker_erro
> free(head_name_to_free);
> head_name = head_name_to_free = resolve_refdup("HEAD", sha1, 0, NULL);
>
> - for (cmd = commands; cmd; cmd = cmd->next)
> - if (!cmd->skip_update)
> - cmd->error_string = update(cmd);
> + for (cmd = commands; cmd; cmd = cmd->next) {
> + if (cmd->error_string)
> + continue;
> +
> + if (cmd->skip_update)
> + continue;
> +
> + cmd->error_string = update(cmd);
> + }
> }These two hunks look good.