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

[Clemens Buchacher <drizzd@aon.at>]

On Mon, Feb 13, 2012 at 01:41:38PM -0800, Junio C Hamano wrote:
> 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"?

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).

> > 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.