{"thread":{"id":"62375","subject":"[PATCH] t1400: fix --no-create-reflog test and description","startedAt":"2024-10-20T16:13:11Z","lastAt":"2024-11-08T03:13:54Z","messageCount":8,"participants":["kristofferhaugsbakk@fastmail.com","Patrick Steinhardt","Kristoffer Haugsbakk","Taylor Blau","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"505562","messageId":"ab7d4c8d89c075de05bf04f1f9dc195145455964.1729439476.git.code@khaugsbakk.name","threadId":"62375","inReplyTo":null,"subject":"[PATCH] t1400: fix --no-create-reflog test and description","fromName":"","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-20T16:12:03Z","receivedAt":"2024-10-20T16:13:11Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"From: Kristoffer Haugsbakk <code@khaugsbakk.name>\n\nThis test has had the wrong title since it was created.[1]  Use `always`\nlike the description says and negate the expected outcome.\n\nThe test works since `core.logAllRefUpdates` set to `true` does not\ncreate a reflog for that ref namespace.  So the test is testing the\nconfig variable, not the option.\n\nThe test itself is fine and does not hide a bug: `--no-create-reflog` is\nnot supposed to override `core.logAllRefUpdates`.\n\n† 1: 341fb286212 (refs: add option core.logAllRefUpdates = always,\n    2017-01-27)\n\nSigned-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n---\n\nNotes (series):\n    From the commit message:\n    \n      “ The test itself is fine and does not hide a bug:\n        `--no-create-reflog` is not supposed to override\n    \n    A source for that: roundabout through git-branch(1):\n    \n      “ The negated form --no-create-reflog only overrides an earlier\n        --create-reflog, but currently does not negate the setting of\n        core.logAllRefUpdates.\n    \n    I *suppose* that the same applies to update-ref since (I suppose) they\n    use the same underlying machinery.\n    \n    See also git-tag(1) which says the same thing.\n    \n    update-ref should document the same thing, then.  I have that marked as\n    a todo item.  The changes there are a bit too involved to implicate in\n    this submission.\n\n t/t1400-update-ref.sh | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex eb1691860da..9bf87db4775 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -169,14 +169,14 @@ test_expect_success 'core.logAllRefUpdates=always creates reflog for ORIG_HEAD'\n \tgit reflog exists ORIG_HEAD\n '\n \n-test_expect_success '--no-create-reflog overrides core.logAllRefUpdates=always' '\n-\ttest_config core.logAllRefUpdates true &&\n+test_expect_success '--no-create-reflog does not override core.logAllRefUpdates=always' '\n+\ttest_config core.logAllRefUpdates always &&\n \ttest_when_finished \"git update-ref -d $outside\" &&\n \tgit update-ref --no-create-reflog $outside $A &&\n \tgit rev-parse $A >expect &&\n \tgit rev-parse $outside >actual &&\n \ttest_cmp expect actual &&\n-\ttest_must_fail git reflog exists $outside\n+\tgit reflog exists $outside\n '\n \n test_expect_success \"create $m (by HEAD)\" '\n\nbase-commit: 34b6ce9b30747131b6e781ff718a45328aa887d0\n-- \n2.46.1.641.g54e7913fcb6\n\n"},{"id":"505665","messageId":"ZxZETN7WjbNiSRyF@pks.im","threadId":"62375","inReplyTo":"ab7d4c8d89c075de05bf04f1f9dc195145455964.1729439476.git.code@khaugsbakk.name","subject":"Re: [PATCH] t1400: fix --no-create-reflog test and description","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2024-10-21T12:08:44Z","receivedAt":"2024-10-21T12:08:53Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Sun, Oct 20, 2024 at 06:12:03PM +0200, kristofferhaugsbakk@fastmail.com wrote:\n> From: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> \n> This test has had the wrong title since it was created.[1]  Use `always`\n> like the description says and negate the expected outcome.\n> \n> The test works since `core.logAllRefUpdates` set to `true` does not\n> create a reflog for that ref namespace.  So the test is testing the\n> config variable, not the option.\n> \n> The test itself is fine and does not hide a bug: `--no-create-reflog` is\n> not supposed to override `core.logAllRefUpdates`.\n\nThat's actually surprising to me, as command line options tend to\noverride configuration.\n\n> † 1: 341fb286212 (refs: add option core.logAllRefUpdates = always,\n>     2017-01-27)\n> \n> Signed-off-by: Kristoffer Haugsbakk <code@khaugsbakk.name>\n> ---\n> \n> Notes (series):\n>     From the commit message:\n>     \n>       “ The test itself is fine and does not hide a bug:\n>         `--no-create-reflog` is not supposed to override\n>     \n>     A source for that: roundabout through git-branch(1):\n>     \n>       “ The negated form --no-create-reflog only overrides an earlier\n>         --create-reflog, but currently does not negate the setting of\n>         core.logAllRefUpdates.\n\nHm. The \"currently\" reads as if this was a known shortcoming rather than\nby design.\n\n>     I *suppose* that the same applies to update-ref since (I suppose) they\n>     use the same underlying machinery.\n>     \n>     See also git-tag(1) which says the same thing.\n>     \n>     update-ref should document the same thing, then.  I have that marked as\n>     a todo item.  The changes there are a bit too involved to implicate in\n>     this submission.\n\nSo I'm quite torn here. It's documented, even though the documentation\ndoesn't exactly feel like this was designed, but rather like it was a\nside effect. The test also contradicts the documentation, even though it\nonly worked by chance. And as mentioned above, everywhere else we\ntypically have a design where the command line option overrides the\nconfig.\n\nOverall I'm rather leaning into the direction of making this work\nproperly. But that would of course be a backwards-incompatible change.\n\nPatrick\n"},{"id":"505711","messageId":"d66936b8-d12b-4035-b563-328ae9fc4824@app.fastmail.com","threadId":"62375","inReplyTo":"ZxZETN7WjbNiSRyF@pks.im","subject":"Re: [PATCH] t1400: fix --no-create-reflog test and description","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-21T16:48:20Z","receivedAt":"2024-10-21T16:48:42Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Mon, Oct 21, 2024, at 14:08, Patrick Steinhardt wrote:\n>> […]\n>> Notes (series):\n>>     From the commit message:\n>>\n>>       “ The test itself is fine and does not hide a bug:\n>>         `--no-create-reflog` is not supposed to override\n>>\n>>     A source for that: roundabout through git-branch(1):\n>>\n>>       “ The negated form --no-create-reflog only overrides an earlier\n>>         --create-reflog, but currently does not negate the setting of\n>>         core.logAllRefUpdates.\n>\n> Hm. The \"currently\" reads as if this was a known shortcoming rather than\n> by design.\n\nI read it as “we might change our minds here—watch out”.  ;)\n\nIt feels very emphasized.  Like the documentation was expecting\nyour surprise.\n\n>>     I *suppose* that the same applies to update-ref since (I suppose) they\n>>     use the same underlying machinery.\n>>\n>>     See also git-tag(1) which says the same thing.\n>>\n>>     update-ref should document the same thing, then.  I have that marked as\n>>     a todo item.  The changes there are a bit too involved to implicate in\n>>     this submission.\n>\n> So I'm quite torn here. It's documented, even though the documentation\n> doesn't exactly feel like this was designed, but rather like it was a\n> side effect. The test also contradicts the documentation, even though it\n> only worked by chance. And as mentioned above, everywhere else we\n> typically have a design where the command line option overrides the\n> config.\n>\n> Overall I'm rather leaning into the direction of making this work\n> properly. But that would of course be a backwards-incompatible change.\n\nGood point.  It does feel inconsistent.  I agree that the conventional\npattern (to my knowledge) is to have options override config when the\noptions are given.\n"},{"id":"505723","messageId":"ZxalTgct8WhEmO0w@nand.local","threadId":"62375","inReplyTo":"d66936b8-d12b-4035-b563-328ae9fc4824@app.fastmail.com","subject":"Re: [PATCH] t1400: fix --no-create-reflog test and description","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T19:02:38Z","receivedAt":"2024-10-21T19:02:40Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 06:48:20PM +0200, Kristoffer Haugsbakk wrote:\n> On Mon, Oct 21, 2024, at 14:08, Patrick Steinhardt wrote:\n> >> […]\n> >> Notes (series):\n> >>     From the commit message:\n> >>\n> >>       “ The test itself is fine and does not hide a bug:\n> >>         `--no-create-reflog` is not supposed to override\n> >>\n> >>     A source for that: roundabout through git-branch(1):\n> >>\n> >>       “ The negated form --no-create-reflog only overrides an earlier\n> >>         --create-reflog, but currently does not negate the setting of\n> >>         core.logAllRefUpdates.\n> >\n> > Hm. The \"currently\" reads as if this was a known shortcoming rather than\n> > by design.\n>\n> I read it as “we might change our minds here—watch out”.  ;)\n>\n> It feels very emphasized.  Like the documentation was expecting\n> your surprise.\n>\n> >>     I *suppose* that the same applies to update-ref since (I suppose) they\n> >>     use the same underlying machinery.\n> >>\n> >>     See also git-tag(1) which says the same thing.\n> >>\n> >>     update-ref should document the same thing, then.  I have that marked as\n> >>     a todo item.  The changes there are a bit too involved to implicate in\n> >>     this submission.\n> >\n> > So I'm quite torn here. It's documented, even though the documentation\n> > doesn't exactly feel like this was designed, but rather like it was a\n> > side effect. The test also contradicts the documentation, even though it\n> > only worked by chance. And as mentioned above, everywhere else we\n> > typically have a design where the command line option overrides the\n> > config.\n> >\n> > Overall I'm rather leaning into the direction of making this work\n> > properly. But that would of course be a backwards-incompatible change.\n>\n> Good point.  It does feel inconsistent.  I agree that the conventional\n> pattern (to my knowledge) is to have options override config when the\n> options are given.\n\nI agree with you both that it feels inconsistent, but I feel somewhat\nuncomfortable changing the behavior here in a backwards incompatible\nway.\n\nEven if the original documentation leaves the door open to changing the\nbehavior, I think that probably a non-zero number of users has either\n(a) never read that documentation, or (b) come to rely on it, or (c)\nboth ;-).\n\nI think if anything we might consider updating the documentation to more\nclearly capture the status-quo, but I'd be very hesitant to see a patch\nchanging the behavior here.\n\nThanks,\nTaylor\n"},{"id":"505762","messageId":"36959987-5695-48fc-9b4e-2fecfe9f5b88@app.fastmail.com","threadId":"62375","inReplyTo":"ZxalTgct8WhEmO0w@nand.local","subject":"Re: [PATCH] t1400: fix --no-create-reflog test and description","fromName":"Kristoffer Haugsbakk","fromEmail":"kristofferhaugsbakk@fastmail.com","sentAt":"2024-10-21T21:02:48Z","receivedAt":"2024-10-21T21:03:26Z","isPatch":true,"sender":{"key":"kristofferhaugsbakk@fastmail.com","avatar":null},"body":"On Mon, Oct 21, 2024, at 21:02, Taylor Blau wrote:\n>> […]\n>> > Overall I'm rather leaning into the direction of making this work\n>> > properly. But that would of course be a backwards-incompatible change.\n>>\n>> Good point.  It does feel inconsistent.  I agree that the conventional\n>> pattern (to my knowledge) is to have options override config when the\n>> options are given.\n\nAgreed, to be clear. ;)\n\n> I agree with you both that it feels inconsistent, but I feel somewhat\n> uncomfortable changing the behavior here in a backwards incompatible\n> way.\n>\n> Even if the original documentation leaves the door open to changing the\n> behavior, I think that probably a non-zero number of users has either\n> (a) never read that documentation, or (b) come to rely on it, or (c)\n> both ;-).\n>\n> I think if anything we might consider updating the documentation to more\n> clearly capture the status-quo, but I'd be very hesitant to see a patch\n> changing the behavior here.\n\nMy background (how I ended up in this test) was that I learnt yesterday\nthat `--create-reflog` and this config variable controls whether reflog\nfiles are created.  I thought that they toggled when entries were made.\n\nI have some work in progress patches for clarifying this mechanism in\ngit-config(1) and other places.\n\nNow git-tag(1) and git-branch(1) seem decently clear on this point[1]\nbut update-ref is lagging behind IMO.  I will be looking at that one as\nwell.\n\n† 1: There is also the `--create-reflog` case vs. when the config is\n   `false` but I can’t check that right now\n"},{"id":"505767","messageId":"ZxbDLZaiiuSR4muv@nand.local","threadId":"62375","inReplyTo":"36959987-5695-48fc-9b4e-2fecfe9f5b88@app.fastmail.com","subject":"Re: [PATCH] t1400: fix --no-create-reflog test and description","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2024-10-21T21:10:05Z","receivedAt":"2024-10-21T21:10:08Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Mon, Oct 21, 2024 at 11:02:48PM +0200, Kristoffer Haugsbakk wrote:\n> On Mon, Oct 21, 2024, at 21:02, Taylor Blau wrote:\n> >> […]\n> >> > Overall I'm rather leaning into the direction of making this work\n> >> > properly. But that would of course be a backwards-incompatible change.\n> >>\n> >> Good point.  It does feel inconsistent.  I agree that the conventional\n> >> pattern (to my knowledge) is to have options override config when the\n> >> options are given.\n>\n> Agreed, to be clear. ;)\n\n;-).\n\n> Now git-tag(1) and git-branch(1) seem decently clear on this point[1]\n> but update-ref is lagging behind IMO.  I will be looking at that one as\n> well.\n\nThanks very much.\n\nThanks,\nTaylor\n"},{"id":"506792","messageId":"4e68a04f-fb5e-49f9-8ba2-661027d8165d@app.fastmail.com","threadId":"62375","inReplyTo":"ab7d4c8d89c075de05bf04f1f9dc195145455964.1729439476.git.code@khaugsbakk.name","subject":"Re: [PATCH] t1400: fix --no-create-reflog test and description","fromName":"Kristoffer Haugsbakk","fromEmail":"code@khaugsbakk.name","sentAt":"2024-11-07T12:11:15Z","receivedAt":"2024-11-07T12:11:37Z","isPatch":true,"sender":{"key":"code@khaugsbakk.name","avatar":"https://avatars.githubusercontent.com/u/2229597?v=4"},"body":"I didn’t get the impression that this is expecting a reroll.  But I\nhaven’t seen it mentioned or applied to `seen`/`next`?\n"},{"id":"506821","messageId":"xmqqwmhepgcw.fsf@gitster.g","threadId":"62375","inReplyTo":"4e68a04f-fb5e-49f9-8ba2-661027d8165d@app.fastmail.com","subject":"Re: [PATCH] t1400: fix --no-create-reflog test and description","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-11-08T03:13:51Z","receivedAt":"2024-11-08T03:13:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kristoffer Haugsbakk\" <code@khaugsbakk.name> writes:\n\n> I didn’t get the impression that this is expecting a reroll.  But I\n> haven’t seen it mentioned or applied to `seen`/`next`?\n\nIf the interim maintainer did not think it is ready to be queued to\n'seen', I won't go back to the list archive to pick it up (after\nall, that is part of trusting interim maintainer is about---otherwise\nI have to go back and consume all the list backlog, which does not\nmake sense).\n\nIf a topic is worth resurrecting, do resend it for discussion.\nLooking at the archived discussion (for this topic I am making an\nexception), I did not get an impression that this got a \"yeah this\nis a good thing to do\" support, only a lukewarm \"it may be a\nstarting point in a good direction\".\n\nThanks.\n"}]}