From: Junio C Hamano <gitster@pobox.com>
Date: Sat, 27 Dec 2008 07:29:42 GMT
Subject: Re: [PATCH] parse-opt: migrate builtin-apply.
Message-ID: <7vtz8qgjo9.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1230351727-20116-1-git-send-email-vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> writes:

> The only notable user-visible/incompatible change is that the
> --build-fake-ancestor option now conforms to gitcli(7).
>
> Signed-off-by: Miklos Vajna <vmiklos@frugalware.org>
> ---
>
> I know that we do care about incompatible changes a lot, though I think
> this is the right direction and probably --build-fake-ancestor is not a
> heavily used switch, so I hope that part is OK.

An acceptable justification for such a plumbing change is if (1) the old
syntax is still supported the same way as the original implementation,
*and* if (2) the new syntax is something that could not possibly have been
a valid input to the original implementation with a different meaning.

I think the condition (1) holds but (2) does not hold for your patch; even
though I think the latter breakage is excusable in this particular case,
it is not for the reason you cited.

That is,

 (1) The parseopt parser allows both of these forms:

	$ git apply --build-fake-ancestor file <input
        $ git apply --build-fake-ancestor=file <input

     The former has been how existing scripts that use the plumbing have
     been feeding the file, and it is still supported.

 (2) A script that used "git apply" and relied on the behaviour of 
     the original implementation could have fed a patch from a file
     whose name is "--build-fake-ancestor=some-string", with this command
     line:

        $ git apply --build-fake-ancestor=file

     Now such a script would break with the new parser.

The reason you are excused to break such an insane script is definitely
not because --build-fake-ancestor is a rarely used option.  The whole
defence depends on the fact that --build-fake-ancestor=something is a very
unlikely name for any sane script to be using for its temporary file.  It
could still be an end user input, but at that point you could simply doubt
the sanity of the end user and dismiss the issue away.

I am not fundamentally opposed to using parseopt in git-apply, and I think
the change to add a new and saner meaning to "--build-fake-ancestor=file"
on the command line is a good thing in the longer term.  But your
justification for such a change should be given in such a way to show
clearly that you have thought things through.  It has to be much better
than "it is not a heavily used switch anyway".

The saddest part of the story that pisses me off about this patch is that
you did not seem to have even run the test suite before sending it.  t4105
and t4252 fail for me, at least.

I did not look at the patch very closely, but do you really need that many
option callbacks?  My gut feeling is that many of them should be just
setting a boolean flag, and you can postprocess to get the correct "apply"
behaviour.

For example, you start with "apply" set to true, and let parseopt set
"diffstat" upon seeing "--stat", and set "cmdline_apply" upon seeing
"--apply".  After parseopt returns, you determine the final value of
"apply" by using "diffstat" (and friends that would normally drop "apply")
and "cmdline_apply" (which would override such droppages).  That way I
think you can lose many callback functions whose sole purpose is to drop
"apply" option, no?