Re: [PATCH v6 5/8] branch: drop non-commit error reporting

From: Matthieu Moy <matthieu.moy@grenoble-inp.fr>
Date: Sep 23, 2015, 20:10 UTC
Message-ID: <vpq6130ucbh.fsf@grenoble-inp.fr>
In-Reply-To: <xmqqfv25x80c.fsf@gitster.mtv.corp.google.com>

Junio C Hamano <gitster@pobox.com> writes:

Show 16 quoted lines

> Karthik Nayak <karthik.188@gmail.com> 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;

Note: the test becomes

      if (filter->merge_commit || filter->with_commit || filter->verbose) {

When the code starts using ref-filter, so the condition of the if becomes the same as it is here. Not related to your concern, but I was worried about "verbose" being used on one side but not the other, and it's actually OK.

Show 7 quoted lines

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

More precisely: if we find such a branch ref and we're used with an option that requires us to lookup the commit, then we report it as an error.

To be sure, I tried:

  echo ee0f5eeeae36cd1b5a346a1e2ae5c8cb841cd5da > .git/refs/heads/broken

where the sha1 is the one of a blob.

$ git branch   
  broken
* master
$ git branch -v
error: branch 'broken' does not point at a commit
* master 5cc76d7 foo
error: some refs could not be read

After the series, I get:

$ git branch
  broken
* master
$ git branch -v
* master 5cc76d7 foo

So I agree with Junio that the commit message is not sufficient: there is a behavioral change. I'm OK with it, but the commit message shouldn't claim that there isn't.

Porting to ref-filter drops the commit before we get an opportunity to complain, so we stop complaining because it's not worth the trouble.

BTW, this looks like an fsck bug:

$ git fsck --strict Checking object directories: 100% (256/256), done. error: refs/heads/broken: not a commit $ echo $? 0

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/