Re: [GSoC PATCH v2] backfill: error out when HEAD cannot be parsed

[Junio C Hamano <gitster@pobox.com>]

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

> Trieu Huynh <vikingtc4@gmail.com> writes:
>
>> From: Trieu Huynh <vikingtc4@gmail.com>
>>
>> handle_revision_arg() returns non-zero on failure, but do_backfill()
>> ignores the return value. On an empty repo with no commits, HEAD is
>> unborn and handle_revision_arg() fails, but backfill silently
>> continues with an empty revision walk and exists with a zero return
>> code.
>
> "exists" -> "exits", I think.
>
> But more importantly (with Devil's advocate hat on), what's the
> downside of the current behaviour?
>
> You tell the command to backfill, the machinery does not find
> anything necessary to fetch to backfill, and successfully, quickly,
> and quietly exits.  That sounds like a graceful exit to me.
>
> Is there anything wrong with that?
>
>> +test_expect_success 'backfill on empty repo fails gracefully' '
>> +	git init empty-repo &&
>> +	test_must_fail git -C empty-repo backfill 2>err &&
>> +	test_grep "unable to parse HEAD" err
>> +'

By the way, a more relevant thing to mention is that this change
will probably become totally unnecessary in the presence of the
ds/backfill-revs topic that is already in 'next'.

It does the usual "if you do not get revision range, fall back to
HEAD", so

    git backfill<RET>

in an empty repository gracefully does nothing, while giving
revision ranges explicitly, like,

    git backfill master..next
    git backfill HEAD

in such a repository will be greeted with a more explicit "bad
revision" error.

Another lesson to pay closer attention to what others are doing in
the same project.  This would have been easily discoverable if you
attempted trial merges to 'next' and to 'seen' after you tested your
change standalone (well, that is how I recalled the other topic
anyway).

Thanks.