On Fri, Dec 26, 2008 at 11:29:42PM -0800, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Miklos Vajna <vmiklos@xxxxxxxxxxxxxx> 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@xxxxxxxxxxxxxx> > > --- > > > > 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.
Attachment:
pgpb1Dl1pyYA9.pgp
Description: PGP signature
