Karthik Nayak <karthik.188@xxxxxxxxx> writes:
> Remove the error reporting variable to make the code easier to port
> over to using ref-filter APIs.
>
> This also removes the error from being displayed. As branch.c will use
> ref-filter APIs in the following patches, the error checking becomes
> redundant with the error reporting system found in the ref-filter
> (ref-filter.c:1336).
Hmm, do you mean these lines by the above reference?
if (filter->merge_commit || filter->with_commit) {
commit = lookup_commit_reference_gently(oid->hash, 1);
if (!commit)
return 0;
That is "silently return ignoring it if it is not a commit", i.e. I
do not think that deserves to be called error REPORTING system.
Do you really understand what the error message you are removing is
trying to diagnose? A branch ref must not point at a blob or any
non-commit object, and if we find such a branch ref, we report it as
error. That is what the error message is about.
Now, ref-filter.c:1336 has in no position to issue that same error
message because it does not know what it is looking at is supposed
to be a branch ref, so it is WRONG if it gave the same error
message. After all, the caller may be using ref-filter to filter
refs/tags/* with merge_commit or with_commit and found a
light-weight tag in refs/tags/* that points at a blob. That is not
an event we want to get an error at all.
I do not think we terribly mind if this message goes away from this
location. "It is a wrong codepath whose purpose is not to diagnose
and report a repository corruption. If we care about such a
repository corruption, we should report it from fsck instead", would
bea valid justification for the removal of the message.
It is not a valid justification to claim that it is made redundant,
when you actually are simply LOSING the error reporting without
giving the same error message from another codepath to make it truly
redundant.
--
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