patchdoc: add documentation for git log --no-follow

> @@ -205,7 +208,8 @@ log.follow::
>  	If `true`, `git log` will act as if the `--follow` option was used when
>  	a single <path> is given.  This has the same limitations as `--follow`,
>  	i.e. it cannot be used to follow multiple files and does not work well
> -	on non-linear history.
> +	on non-linear history. This setting can be disabled by the `--no-follow`
> +	option.

> On Sat, Feb 01, 2020 at 12:24:31AM +0100, Étienne Servais wrote:
> 
>> This feature was added by commit
>> 076c98372e (log: add "log.follow" configuration variable, 2015-07-07)
>> but remained undocumented.
> 
> That commit just touched the code; it was originally added by aebbcf5797
> (diff: accept --no-follow option, 2012-09-21). But I think the general
> intent of your patch is still valid.

>> I couldn't figure if I shall merge the --no-follow doc with the follow
>> as is done for --no-decorate and --decorate just after.
> 
> I think it would make more sense to just put it with --follow, like:
> 
>   [--no-]follow::
> 
> which matches how --use-mailmap is defined, for instance.

> 
>> @@ -205,7 +208,8 @@ log.follow::
>>  	If `true`, `git log` will act as if the `--follow` option was used when
>>  	a single <path> is given.  This has the same limitations as `--follow`,
>>  	i.e. it cannot be used to follow multiple files and does not work well
>> -	on non-linear history.
>> +	on non-linear history. This setting can be disabled by the `--no-follow`
>> +	option.
> 
> I'm on the fence on this hunk. There are a number of config options that
> can be canceled or overridden by command-line options. It's a normal
> pattern for the "--no" variant to do so. So while it often doesn't hurt
> to give a pointer in the right direction, I don't know that we'd want to
> start doing so in every such case.

> When we're resolving a REF_DELTA, we compare-and-swap its type from
> REF_DELTA to whatever real type the base object has, as discussed in
> ab791dd138 (index-pack: fix race condition with duplicate bases,
> 2014-08-29). If the old type wasn't a REF_DELTA, we consider that a
> BUG(). But as discussed in that commit, we might see this case whenever
> we try to resolve an object twice, which may happen because we have
> multiple copies of the base object.
>
> So this isn't a bug at all, but rather a sign that the input pack is
> broken. And indeed, this case is triggered already in t5309.5 and
> t5309.6, which create packs with delta cycles and duplicate bases. But
> we never noticed because those tests are marked expect_failure.
>
> Those tests were added by b2ef3d9ebb (test index-pack on packs with
> recoverable delta cycles, 2013-08-23), which was leaving the door open
> for cases that we theoretically _could_ handle. And when we see an
> already-resolved object like this, in theory we could keep going after
> confirming that the previously resolved child->real_type matches
> base->obj->real_type. But:
>
>   - enforcing the "only resolve once" rule here saves us from an
>     infinite loop in other parts of the code. If we keep going, then the
>     delta cycle in t5309.5 causes us to loop infinitely, as
>     find_ref_delta_children() doesn't realize which objects have already
>     been resolved. So there would be more changes needed to make this
>     case work, and in the meantime we'd be worse off.
>
>   - any pack that triggers this is broken anyway. It either has a
>     duplicate base object, or it has a cycle which causes us to bring in
>     a duplicate via --fix-thin. In either case, we'd end up rejecting
>     the pack in write_idx_file(), which also detects duplicates.
>
> So the tests have little value in documenting what we _could_ be doing
> (and have been neglected for 6+ years). Let's switch them to confirming
> that we handle this case cleanly (and switch out the BUG() for a more
> informative die() so that we do so).
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Noticed while running the tests in an environment that complains about
> SIGABRT deaths. Arguably the test suite should be looking for these even
> in test_expect_failure, but it would be a little convoluted to do so
> (e.g., teaching BUG() to write to a marker file, and then checking the
> file). And I think we're better off phrasing things as much as possible
> as expect_success anyway.
>
>  builtin/index-pack.c         | 4 +++-
>  t/t5309-pack-delta-cycles.sh | 8 ++++----
>  2 files changed, 7 insertions(+), 5 deletions(-)
>
> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 60a5591039..41a7c11c8e 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1003,7 +1003,9 @@ static struct base_data *find_unresolved_deltas_1(struct base_data *base,
>
>  		if (!compare_and_swap_type(&child->real_type, OBJ_REF_DELTA,
>  					   base->obj->real_type))
> -			BUG("child->real_type != OBJ_REF_DELTA");
> +			die("REF_DELTA at offset %"PRIuMAX" already resolved (duplicate base %s?)",
> +			    (uintmax_t)child->idx.offset,
> +			    oid_to_hex(&base->obj->idx.oid));

>  		resolve_delta(child, base, result);
>  		if (base->ref_first == base->ref_last && base->ofs_last == -1)
> diff --git a/t/t5309-pack-delta-cycles.sh b/t/t5309-pack-delta-cycles.sh
> index 491556dad9..6c209ad45c 100755
> --- a/t/t5309-pack-delta-cycles.sh
> +++ b/t/t5309-pack-delta-cycles.sh
> @@ -62,13 +62,13 @@ test_expect_success 'index-pack detects REF_DELTA cycles' '
>  	test_must_fail git index-pack --fix-thin --stdin <cycle.pack
>  '
>
> -test_expect_failure 'failover to an object in another pack' '
> +test_expect_success 'failover to an object in another pack' '
>  	clear_packs &&
>  	git index-pack --stdin <ab.pack &&
> -	git index-pack --stdin --fix-thin <cycle.pack
> +	test_must_fail git index-pack --stdin --fix-thin <cycle.pack
>  '
>
> -test_expect_failure 'failover to a duplicate object in the same pack' '
> +test_expect_success 'failover to a duplicate object in the same pack' '
>  	clear_packs &&
>  	{
>  		pack_header 3 &&
> @@ -77,7 +77,7 @@ test_expect_failure 'failover to a duplicate object in the same pack' '
>  		pack_obj $A
>  	} >recoverable.pack &&
>  	pack_trailer recoverable.pack &&
> -	git index-pack --fix-thin --stdin <recoverable.pack
> +	test_must_fail git index-pack --fix-thin --stdin <recoverable.pack
>  '