Re: [PATCH] parse-opt: migrate builtin-apply.

On Fri, Dec 26, 2008 at 11:29:42PM -0800, Junio C Hamano [off-list ref] wrote:

Miklos Vajna [off-list ref] writes:

quoted

The only notable user-visible/incompatible change is that the
--build-fake-ancestor option now conforms to gitcli(7).

Signed-off-by: Miklos Vajna <redacted>
---

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".

I was not aware about parsepont allows both options, I just -
incorrectly - thought git-log uses paseopt and there I remember
--since=foo works, but not --since foo. So actually the commit message
is incorrect, the backwards-incompatible change is not to accept a patch
file named --build-fake-ancestor=something without passing '--' first.

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.

Hm, I did:

$ ./t4105-apply-fuzz.sh
*   ok 1: setup
*   ok 2: unmodified patch
*   ok 3: minus offset
*   ok 4: plus offset
*   ok 5: big offset
*   ok 6: fuzz with no offset
*   ok 7: fuzz with minus offset
*   ok 8: fuzz with plus offset
*   ok 9: fuzz with big offset
* passed all 9 test(s)

$ ./t4252-am-options.sh
*   ok 1: setup
*   ok 2: interrupted am --whitespace=fix
*   ok 3: interrupted am -C1
*   ok 4: interrupted am -p2
*   ok 5: interrupted am -C1 -p2
* passed all 5 test(s)

$ git show -s --pretty=oneline
05d26caf212b58998b7e559991f3a25fd8cbf3f0 parse-opt: migrate builtin-apply.

What testcases did fail for you?

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?

Yes, you are right. I'll send an updated patch in a bit.