Re: [PATCH 1/2] clone: do not ignore --no-hardlinks

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 27, 2009, 07:59 UTC
Message-ID: <7vr61kmht9.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <7vy6vsmjeo.fsf@gitster.siamese.dyndns.org>

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

Show 15 quoted lines

> Jeff King <peff@peff.net> writes:
>
> So across filesystems:
>
>  - "git clone /p/a/t/h" falls back to copying;
>
>  - "git clone --local /p/a/t/h" should fail without falling back to
>    copying; and
>
>  - "git clone --no-local /p/a/t/h" should work as if file:///p/a/t/h
>    was given.
>
> That is much more sensible than making "git clone --no-hardlinks /p/a/t/h"
> imply more than what the option really means: we are making a local copy
> but do not cheat with hardlinking.

I liked it so much that I wrote a commit message for it:

    commit c255d7bbf4dd567ca60a8991c3ac052c9b2d1593
    Author: Jeff King <peff@peff.net>
    Date:   Thu Feb 26 23:31:49 2009 -0800

        clone: do not ignore --no-local option

        With a variable initialized to false, we couldn't tell if the user didn't
        give --local or if the user explicitly told us --no-local after we get control
        back from parse_options().

        This fixes and makes "git clone --no-local /p/a/t/h" always use non-local
        transport (aka "git native protocol over pipe").

        Signed-off-by: Junio C Hamano <gitster@pobox.com>

Except that it does not work.

Initializing option_local to -1 and then feeding it to parse_options() via OPT_BOOLEAN() is a cute idea, but when parse_options() sees a command line like this:

	$ git clone -l /p/a/t/h

It triggers this codepath in get_value():

	case OPTION_BOOLEAN:
		*(int *)opt->value = unset ? 0 : *(int *)opt->value + 1;
		return 0;

and ends up incrementing it to zero.

I wonder what would break if we simply change this to:

	case OPTION_BOOLEAN:
		*(int *)opt->value = !unset;
		return 0;

Damn it, it is called BOOLEAN, and naïvely I would think --option would set it to 1, --no-option would set it to 0, and not having --option nor --no-option would leave the associated variable alone, but apparently that is not what is happening.

Pierre, do you remember why this code is implemented this way? The "increment if we have --option" semantics seems to date back to 4a59fd1 (Add a simple option parser., 2007-10-15) which is the day one of the history of parse-options.