Re: [PATCH/RFC] revision: Show friendlier message.

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 25, 2012, 23:07 UTC
Message-ID: <7vsjdjdm7v.fsf@alter.siamese.dyndns.org>
In-Reply-To: <CAA3EhHLy+5Vfw0T=7VEBi+2ZxjS4x2dndox+M_E06v3FtoNQXg@mail.gmail.com>

Leila <muhtasib@gmail.com> writes:

Show 8 quoted lines

>> The "bad HEAD and no revs..." part, if we choose not to even error
>> on this, can be removed.
>
> Yea, I think we should return successfully, and warning() does that.
> But if we choose to display a message, I don't think it should be a
> warning (esp for the empty repo case). It should look like the sample
> printf below, but the v2 of the patch I submitted doesn't include the
> message.

I said "*if* we choose not to" for a reason. It can be argued that it technically is a regression that "git log" does *not* error out for an unborn history, as that is different from the way the command has behaved forever.

Show 10 quoted lines

> + if (!opt.def && !rev.cmdline.nr) {
> +          printf("No commit(s) to display.\n");
> +          return 0;
> +        }
>
>>
>> Also other cmd_frotz() functions in the same file might want to use
>> the s/"HEAD"/default_to_head_if_exists()/ conversion.
>
> Ok, I've updated other functions in the same file.

Again, "might want" was a key phrase. I didn't look at each and every one of them and thought if it made sense to change their behaviour.

Show 22 quoted lines

> Regarding this implementation:
>
>> +static const char *default_to_head_if_exists(void)
>> +{
>> +       unsigned char sha1[20];
>> +       if (resolve_ref_unsafe("HEAD", sha1, 1, NULL))
>> +               return "HEAD";
>> +       else
>> +               return NULL;
>> +}
>> +
>
> I initially wrote something with this logic, do you have a preference?
>
> +static const char *default_to_head_if_exists(void)
> +{
> +       struct commit *commit = lookup_commit_reference_by_name("HEAD");
> +       if(commit)
> +               return "HEAD";
> +       else
> +               return NULL;
> +}

The reason why I used resolve_ref_unsafe() is because it will only grab HEAD and not refs/heads/HEAD or any confusing mess, even in a sick repository.