Clemens Buchacher <drizzd@xxxxxx> 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".
> 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?
> @@ -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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html