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

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

 



Junio C Hamano <gitster@xxxxxxxxx> writes:

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

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.

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



[Index of Archives]     [Linux Kernel Development]     [Gcc Help]     [IETF Annouce]     [DCCP]     [Netdev]     [Networking]     [Security]     [V4L]     [Bugtraq]     [Yosemite]     [MIPS Linux]     [ARM Linux]     [Linux Security]     [Linux RAID]     [Linux SCSI]     [Fedora Users]