Re: [PATCH 2/5] do not override receive-pack errors

Clemens Buchacher [off-list ref] 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".

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?

These two hunks look good.

On Mon, Feb 13, 2012 at 01:41:38PM -0800, Junio C Hamano wrote:

Clemens Buchacher [off-list ref] writes:

quoted

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"?

Yes, it really is. For example, in t5504 rev-list --verify-objects (it
was turned on for me if called from there) detects the corrupt object.
But the error string is later overwritten with the return value of
update, which is NULL in this case.

That is why I had to change the t5504 tests from a successful git push
to a test_must_fail git push with this fix. To keep the previous
behavior we would have to replace the corrupt blob with a more subtle
corruption that rev-list --verify-objects would not detect, but fsck
would (e.g., a malformed commit header).

quoted

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);
+	}

[...]

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.

Actually, check_alias_update searches for aliases of cmd in ref_list,
which is a list of refs from all commands, irrespective of their error
status. So this change is correct.

However, after re-reading the code I now have the impression that the
alias detection is not entirely correct. It does find aliases between
symrefs and regular refs.  But it does not find aliases between two
symrefs, because ref_list will not contain the actual ref pointed to,
and therefore the code considers neither symref an alias.

But that is independent of the hunk above.