{"thread":{"id":"54246","subject":"Re: [PATCH] format-patch: cancel useAutoBase if base is invalid","startedAt":"2020-09-17T00:33:57Z","lastAt":"2020-09-18T00:04:15Z","messageCount":4,"participants":["Junio C Hamano","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"405710","messageId":"xmqqr1r1tfil.fsf@gitster.c.googlers.com","threadId":"54246","inReplyTo":"20200916234916.422553-1-jacob.e.keller@intel.com","subject":"Re: [PATCH] format-patch: cancel useAutoBase if base is invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T00:26:26Z","receivedAt":"2020-09-17T00:33:57Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> Make get_base_commit detect when useAutoBase is set, and avoid failing\n> if the base commit is picked up automatically. We still attempt to fail\n> if --base=auto is explicitly requested on the command line.\n\nMakes sense. I also think we should fail in such a broken base is\nchosen, when useAutoBase is set by configuration and is not\noverriden from the command line with an explicit use of --no-base\noption, because the end-user expects an appropriate base to be used\nthat is computed automatically, but we are failing to find such a\nbase---going ahead silently in such a case would be wrong.\n\n> Unfortunately, at least with how --base is handled by the option parsing\n> now, if useAutoBase is true *and* --base=auto is provided, we'll still\n> cancel the base option instead of failing.\n\nIf we are doing the usual \"prime with configuration variables and\nthen override from the commnad line option\" thing, then you cannot\ntell these two cases apart, but I happen to think that it is better\nto fail (and not to ignore silently) consistently whether the\n\"compute an appropriate base automatically\" came from config or\ncommand line, so...\n"},{"id":"405711","messageId":"xmqqmu1pteq0.fsf@gitster.c.googlers.com","threadId":"54246","inReplyTo":"xmqqr1r1tfil.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] format-patch: cancel useAutoBase if base is invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-17T00:43:35Z","receivedAt":"2020-09-17T00:52:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> If we are doing the usual \"prime with configuration variables and\n> then override from the commnad line option\" thing, then you cannot\n> tell these two cases apart, but I happen to think that it is better\n> to fail (and not to ignore silently) consistently whether the\n> \"compute an appropriate base automatically\" came from config or\n> command line, so...\n\nIf we really need to tell these two apart, we'd invent a new\nvariable (we only need a single bit) to record \"did we get this base\nsetting from the command line?\" along with what we are already\ndoing. You would need to use a option callback instead of a simple\nOPT_STRING() to parse the \"base\" option, but I think it should be\nstraightforward.\n\n\n\n\n"},{"id":"405799","messageId":"CA+P7+xrvbwVevFaCB=cm4mZB4=nFAL2rs4gFgYF85qSopq+dwg@mail.gmail.com","threadId":"54246","inReplyTo":"xmqqr1r1tfil.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] format-patch: cancel useAutoBase if base is invalid","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2020-09-17T19:26:20Z","receivedAt":"2020-09-17T20:23:01Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Wed, Sep 16, 2020 at 5:26 PM Junio C Hamano wrote:\n>\n> Jacob Keller writes:\n>\n> > Make get_base_commit detect when useAutoBase is set, and avoid failing\n> > if the base commit is picked up automatically. We still attempt to fail\n> > if --base=auto is explicitly requested on the command line.\n>\n> Makes sense. I also think we should fail in such a broken base is\n> chosen, when useAutoBase is set by configuration and is not\n> overriden from the command line with an explicit use of --no-base\n> option, because the end-user expects an appropriate base to be used\n> that is computed automatically, but we are failing to find such a\n> base---going ahead silently in such a case would be wrong.\n>\n\nI am not sure if I follow here. The whole point of this patch is that\n\ngit config format.useAutoBase true\ngit format-patch -1 \n\ncauses failure that is very unexpected, especially if it's been a long\ntime since you set useAutoBase.\n\nI do want git format-patch --base=auto to fail, certainly.\n\nI wonder if there's a way we can tell when the format patch revisions\nin question make no sense with the automatic base. So.. rather than\nlooking at \"is this base valid\" think of it as \"the base for this\nbranch is XYZ, is that valid for the set of requested commits?\"\n\n> > Unfortunately, at least with how --base is handled by the option parsing\n> > now, if useAutoBase is true *and* --base=auto is provided, we'll still\n> > cancel the base option instead of failing.\n>\n> If we are doing the usual \"prime with configuration variables and\n> then override from the commnad line option\" thing, then you cannot\n> tell these two cases apart, but I happen to think that it is better\n> to fail (and not to ignore silently) consistently whether the\n> \"compute an appropriate base automatically\" came from config or\n> command line, so...\n"},{"id":"405812","messageId":"xmqqsgbgne6d.fsf@gitster.c.googlers.com","threadId":"54246","inReplyTo":"CA+P7+xrvbwVevFaCB=cm4mZB4=nFAL2rs4gFgYF85qSopq+dwg@mail.gmail.com","subject":"Re: [PATCH] format-patch: cancel useAutoBase if base is invalid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-09-18T00:04:10Z","receivedAt":"2020-09-18T00:04:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jacob Keller writes:\n\n> On Wed, Sep 16, 2020 at 5:26 PM Junio C Hamano wrote:\n>>\n>> Jacob Keller writes:\n>>\n>> > Make get_base_commit detect when useAutoBase is set, and avoid failing\n>> > if the base commit is picked up automatically. We still attempt to fail\n>> > if --base=auto is explicitly requested on the command line.\n>>\n>> Makes sense. I also think we should fail in such a broken base is\n>> chosen, when useAutoBase is set by configuration and is not\n>> overriden from the command line with an explicit use of --no-base\n>> option, because the end-user expects an appropriate base to be used\n>> that is computed automatically, but we are failing to find such a\n>> base---going ahead silently in such a case would be wrong.\n>>\n>\n> I am not sure if I follow here. The whole point of this patch is that\n>\n> git config format.useAutoBase true\n> git format-patch -1 \n>\n> causes failure that is very unexpected, especially if it's been a long\n> time since you set useAutoBase.\n>\n> I do want git format-patch --base=auto to fail, certainly.\n\nI understand. And further, I do not think it is a good idea to\nsilently ignore the configured format.useAutoBase when there is no\ncommand line override. IOW, we want both to fail, but with a better\nmessage (e.g. \"appropriate base not found\").\n\n> I wonder if there's a way we can tell when the format patch revisions\n> in question make no sense with the automatic base.\n\nSorry, I don't quite know what you mean---the fact that you are\nalready getting a cryptic error message means the existing code\nalready knows, no?\n\n\n"}]}