Leila <muhtasib@xxxxxxxxx> writes:
>> 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.
> + 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.
> 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.
--
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