{"thread":{"id":"55074","subject":"[PATCH 0/2] Eliminate extraneous ref log entries","startedAt":"2021-01-30T10:27:09Z","lastAt":"2021-02-01T11:10:17Z","messageCount":9,"participants":["Kyle J. McKay","Junio C Hamano","Han-Wen Nienhuys"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"415649","messageId":"7c7e8679f2da7e1475606d698b2da8c@72481c9465c8b2c4aaff8b77ab5e23c","threadId":"55074","inReplyTo":null,"subject":"[PATCH 0/2] Eliminate extraneous ref log entries","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2021-01-30T10:19:07Z","receivedAt":"2021-01-30T10:27:09Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since Git version v2.29.0, the `git symbolic-ref` command has started\nadding extraneous entries to the ref log of the symbolic ref it's\nupdating.\n\nThis change was inadvertently introduced in commit 523fa69c36744ae6\n(\"reflog: cleanse messages in the refs.c layer\", 2020-07-10, v2.29.0).\n\nA bug report [1] was made about a failing test in the TopGit test\nsuite.  Further investigations into the cause led to this patch set.\n\n1/2 - adds new tests to monitor this behavior\n2/2 - corrects the problem\n\nThe tests added in 1/2 are marked `test_expect_failure` and then\nchanged to `test_expect_success` in 2/2.\n\n-Kyle\n\n[1]: <https://github.com/mackyle/topgit/issues/17>\n\nKyle J. McKay (2):\n  t/t1417: test symbolic-ref effects on ref logs\n  refs.c: avoid creating extra unwanted reflog entries\n\n refs.c                   | 16 +++----\n t/t1417-reflog-symref.sh | 91 ++++++++++++++++++++++++++++++++++++++++\n 2 files changed, 100 insertions(+), 7 deletions(-)\n create mode 100755 t/t1417-reflog-symref.sh\n\n-- \n"},{"id":"415650","messageId":"1e8c8e3d23d3c2ddf031e3d7719241c@72481c9465c8b2c4aaff8b77ab5e23c","threadId":"55074","inReplyTo":"7c7e8679f2da7e1475606d698b2da8c@72481c9465c8b2c4aaff8b77ab5e23c","subject":"[PATCH 2/2] refs.c: avoid creating extra unwanted reflog entries","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2021-01-30T10:19:09Z","receivedAt":"2021-01-30T10:27:10Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"Since commit 523fa69c36744ae6 (\"reflog: cleanse messages in the refs.c\nlayer\", 2020-07-10, v2.29.0), ref log messages are now being \"cleansed\"\nto make sure they do not end up breaking the ref log files.  A laudable\nendeavor.\n\nUnfortunately, that commit had an unintended side effect that causes\nthe `git symbolic-ref <refname1> <refname2>` command to suddenly start\nadding new entries to the ref log for <refname1> whenever it's run.\n\nThese new entries have a completely empty message and do not provide\nany useful information.  In fact, there was no mention that the change\nto \"cleanse\" ref log messages was intended to add these new ref log\nentries at all.\n\nWhat happened is that when the change to \"cleanse\" the incoming ref\nlog message was made, the code started inadvertently transforming\na NULL ref log message pointer into an empty string \"\".\n\nThis created the observed effect that using the `symbolic-ref` command\nsuddenly started causing ref log entries to be added.\n\nThe original code that predated the \"cleanse\" commit called the\n`xstrdup_or_null` function to retain the original NULL pointer and\navoid introducing unwanted extra ref log entries.\n\nAfter the \"cleanse\" commit, ref log messages are now funnelled through\na new static function named `normalize_reflog_message`.\n\nEliminate the unwanted extra blank ref log entries by returning a NULL\npointer when NULL is passed into `normalize_reflog_message` rather\nthan returning a pointer to an empty string (\"\").\n\nTo reflect this new behavior, rename the function to\n`normalize_reflog_message_or_null` in the same spirit as the name\nof the `xstrdup_or_null` function that was called pre-\"cleanse\".\n\nFlip the `test_expect_failure` tests to `test_expect_success`\nas they now pass again.\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n---\n refs.c                   | 16 +++++++++-------\n t/t1417-reflog-symref.sh |  6 +++---\n 2 files changed, 12 insertions(+), 10 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 03968ad7..790b1ff0 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -835,11 +835,13 @@ static void copy_reflog_msg(struct strbuf *sb, const char *msg)\n \tstrbuf_rtrim(sb);\n }\n \n-static char *normalize_reflog_message(const char *msg)\n+static char *normalize_reflog_message_or_null(const char *msg)\n {\n \tstruct strbuf sb = STRBUF_INIT;\n \n-\tif (msg && *msg)\n+\tif (!msg)\n+\t\treturn NULL;\n+\tif (*msg)\n \t\tcopy_reflog_msg(&sb, msg);\n \treturn strbuf_detach(&sb, NULL);\n }\n@@ -1067,7 +1069,7 @@ struct ref_update *ref_transaction_add_update(\n \t\toidcpy(&update->new_oid, new_oid);\n \tif (flags & REF_HAVE_OLD)\n \t\toidcpy(&update->old_oid, old_oid);\n-\tupdate->msg = normalize_reflog_message(msg);\n+\tupdate->msg = normalize_reflog_message_or_null(msg);\n \treturn update;\n }\n \n@@ -1951,7 +1953,7 @@ int refs_create_symref(struct ref_store *refs,\n \tchar *msg;\n \tint retval;\n \n-\tmsg = normalize_reflog_message(logmsg);\n+\tmsg = normalize_reflog_message_or_null(logmsg);\n \tretval = refs->be->create_symref(refs, ref_target, refs_heads_master,\n \t\t\t\t\t msg);\n \tfree(msg);\n@@ -2339,7 +2341,7 @@ int refs_delete_refs(struct ref_store *refs, const char *logmsg,\n \tchar *msg;\n \tint retval;\n \n-\tmsg = normalize_reflog_message(logmsg);\n+\tmsg = normalize_reflog_message_or_null(logmsg);\n \tretval = refs->be->delete_refs(refs, msg, refnames, flags);\n \tfree(msg);\n \treturn retval;\n@@ -2357,7 +2359,7 @@ int refs_rename_ref(struct ref_store *refs, const char *oldref,\n \tchar *msg;\n \tint retval;\n \n-\tmsg = normalize_reflog_message(logmsg);\n+\tmsg = normalize_reflog_message_or_null(logmsg);\n \tretval = refs->be->rename_ref(refs, oldref, newref, msg);\n \tfree(msg);\n \treturn retval;\n@@ -2374,7 +2376,7 @@ int refs_copy_existing_ref(struct ref_store *refs, const char *oldref,\n \tchar *msg;\n \tint retval;\n \n-\tmsg = normalize_reflog_message(logmsg);\n+\tmsg = normalize_reflog_message_or_null(logmsg);\n \tretval = refs->be->copy_ref(refs, oldref, newref, msg);\n \tfree(msg);\n \treturn retval;\ndiff --git a/t/t1417-reflog-symref.sh b/t/t1417-reflog-symref.sh\nindex 6149531f..3687b058 100755\n--- a/t/t1417-reflog-symref.sh\n+++ b/t/t1417-reflog-symref.sh\n@@ -53,7 +53,7 @@ test_expect_success setup '\n \ttest $hcnt -ne $kcnt\n '\n \n-test_expect_failure 'HEAD reflog symbolic-ref' '\n+test_expect_success 'HEAD reflog symbolic-ref' '\n \thcnt1=$(git reflog show HEAD | wc -l) &&\n \tgit symbolic-ref HEAD refs/heads/unu &&\n \tgit symbolic-ref HEAD refs/heads/du &&\n@@ -62,7 +62,7 @@ test_expect_failure 'HEAD reflog symbolic-ref' '\n \ttest $hcnt1 = $hcnt2\n '\n \n-test_expect_failure 'refs/heads/KVAR reflog symbolic-ref' '\n+test_expect_success 'refs/heads/KVAR reflog symbolic-ref' '\n \tkcnt1=$(git reflog show refs/heads/KVAR | wc -l) &&\n \tgit symbolic-ref refs/heads/KVAR refs/heads/tri &&\n \tgit symbolic-ref refs/heads/KVAR refs/heads/du &&\n@@ -71,7 +71,7 @@ test_expect_failure 'refs/heads/KVAR reflog symbolic-ref' '\n \ttest $kcnt1 = $kcnt2\n '\n \n-test_expect_failure 'double symref reflog symbolic-ref' '\n+test_expect_success 'double symref reflog symbolic-ref' '\n \thcnt1=$(git reflog show HEAD | wc -l) &&\n \tkcnt1=$(git reflog show refs/heads/KVAR | wc -l) &&\n \tgit symbolic-ref HEAD refs/heads/KVAR &&\n-- \n"},{"id":"415651","messageId":"fec7ef37962da584a89012234ae4a1a@72481c9465c8b2c4aaff8b77ab5e23c","threadId":"55074","inReplyTo":"7c7e8679f2da7e1475606d698b2da8c@72481c9465c8b2c4aaff8b77ab5e23c","subject":"[PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2021-01-30T10:19:08Z","receivedAt":"2021-01-30T10:27:10Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"The git command `git symbolic-ref <refname1> <refname2>` updates\n<refname1> to be a \"symbolic\" pointer to <refname2>.  No matter\nwhat future value <refname2> takes on, <refname1> will continue\nto refer to that future value since it's \"symbolic\".\n\nSince commit 523fa69c36744ae6 (\"reflog: cleanse messages in the refs.c\nlayer\", 2020-07-10, v2.29.0), the effect of using the aforementioned\n\"symbolic-ref\" command on ref logs has changed in an unexpected way.\n\nAdd a new set of tests to exercise and demonstrate this change\nin preparation for correcting it (at which point the failing tests\nwill be flipped from `test_expect_failure` to `test_expect_success`).\n\nThe new test file can be used unchanged to examine this behavior\nin much older Git versions (likely to as far back as v2.6.0).\n\nSigned-off-by: Kyle J. McKay <mackyle@gmail.com>\n---\n t/t1417-reflog-symref.sh | 91 ++++++++++++++++++++++++++++++++++++++++\n 1 file changed, 91 insertions(+)\n create mode 100755 t/t1417-reflog-symref.sh\n\ndiff --git a/t/t1417-reflog-symref.sh b/t/t1417-reflog-symref.sh\nnew file mode 100755\nindex 00000000..6149531f\n--- /dev/null\n+++ b/t/t1417-reflog-symref.sh\n@@ -0,0 +1,91 @@\n+#!/bin/sh\n+#\n+# Copyright (c) 2021 Kyle J. McKay\n+#\n+\n+test_description='Test symbolic-ref effects on reflogs'\n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n+. ./test-lib.sh\n+\n+test_expect_success setup '\n+\ttest_commit 'initial' &&\n+\tgit checkout -b unu &&\n+\ttest_commit 'one' &&\n+\tgit checkout -b du &&\n+\ttest_commit 'two' &&\n+\tgit checkout -b tri &&\n+\ttest_commit 'three' &&\n+\tgit checkout du &&\n+\ttest_commit 'twofour' &&\n+\tgit checkout -b KVAR du &&\n+\ttest_commit 'four' &&\n+\tunu=\"$(git rev-parse --verify unu)\" &&\n+\tdu=\"$(git rev-parse --verify du)\" &&\n+\ttri=\"$(git rev-parse --verify tri)\" &&\n+\tkvar=\"$(git rev-parse --verify KVAR)\" &&\n+\ttest -n \"$unu\" &&\n+\ttest -n \"$du\" &&\n+\ttest -n \"$tri\" &&\n+\ttest -n \"$kvar\" &&\n+\ttest \"$unu\" != \"$du\" &&\n+\ttest \"$unu\" != \"$tri\" &&\n+\ttest \"$unu\" != \"$kvar\" &&\n+\ttest \"$du\" != \"$tri\" &&\n+\ttest \"$du\" != \"$kvar\" &&\n+\ttest \"$tri\" != \"$kvar\" &&\n+\tgit symbolic-ref refs/heads/KVAR refs/heads/du &&\n+\tkvarsym=\"$(git rev-parse --verify KVAR)\" &&\n+\ttest \"$kvarsym\" = \"$du\" &&\n+\ttest \"$kvarsym\" != \"$kvar\" &&\n+\tgit reflog exists HEAD &&\n+\tgit reflog exists refs/heads/KVAR &&\n+\tgit symbolic-ref HEAD >/dev/null &&\n+\tgit symbolic-ref refs/heads/KVAR &&\n+\tgit checkout unu &&\n+\thcnt=$(git reflog show HEAD | wc -l) &&\n+\tkcnt=$(git reflog show refs/heads/KVAR | wc -l) &&\n+\ttest -n \"$hcnt\" &&\n+\ttest -n \"$kcnt\" &&\n+\ttest $hcnt -gt 1 &&\n+\ttest $kcnt -gt 1 &&\n+\ttest $hcnt -ne $kcnt\n+'\n+\n+test_expect_failure 'HEAD reflog symbolic-ref' '\n+\thcnt1=$(git reflog show HEAD | wc -l) &&\n+\tgit symbolic-ref HEAD refs/heads/unu &&\n+\tgit symbolic-ref HEAD refs/heads/du &&\n+\tgit symbolic-ref HEAD refs/heads/tri &&\n+\thcnt2=$(git reflog show HEAD | wc -l) &&\n+\ttest $hcnt1 = $hcnt2\n+'\n+\n+test_expect_failure 'refs/heads/KVAR reflog symbolic-ref' '\n+\tkcnt1=$(git reflog show refs/heads/KVAR | wc -l) &&\n+\tgit symbolic-ref refs/heads/KVAR refs/heads/tri &&\n+\tgit symbolic-ref refs/heads/KVAR refs/heads/du &&\n+\tgit symbolic-ref refs/heads/KVAR refs/heads/unu &&\n+\tkcnt2=$(git reflog show refs/heads/KVAR | wc -l) &&\n+\ttest $kcnt1 = $kcnt2\n+'\n+\n+test_expect_failure 'double symref reflog symbolic-ref' '\n+\thcnt1=$(git reflog show HEAD | wc -l) &&\n+\tkcnt1=$(git reflog show refs/heads/KVAR | wc -l) &&\n+\tgit symbolic-ref HEAD refs/heads/KVAR &&\n+\tgit symbolic-ref refs/heads/KVAR refs/heads/du &&\n+\tgit symbolic-ref refs/heads/KVAR refs/heads/unu &&\n+\tgit symbolic-ref refs/heads/KVAR refs/heads/tri &&\n+\tgit symbolic-ref HEAD refs/heads/du &&\n+\tgit symbolic-ref HEAD refs/heads/tri &&\n+\tgit symbolic-ref HEAD refs/heads/unu &&\n+\thcnt2=$(git reflog show HEAD | wc -l) &&\n+\tkcnt2=$(git reflog show refs/heads/KVAR | wc -l) &&\n+\ttest $hcnt1 = $hcnt2 &&\n+\ttest $kcnt1 = $kcnt2 &&\n+\ttest $hcnt2 != $kcnt2\n+'\n+\n+test_done\n-- \n"},{"id":"415660","messageId":"xmqqa6sqp827.fsf@gitster.c.googlers.com","threadId":"55074","inReplyTo":"fec7ef37962da584a89012234ae4a1a@72481c9465c8b2c4aaff8b77ab5e23c","subject":"Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-30T18:56:48Z","receivedAt":"2021-01-30T18:57:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> The git command `git symbolic-ref <refname1> <refname2>` updates\n> <refname1> to be a \"symbolic\" pointer to <refname2>.  No matter\n> what future value <refname2> takes on, <refname1> will continue\n> to refer to that future value since it's \"symbolic\".\n\nCorrect.  While you are on the my-topic branch, HEAD (that's the\n<refname1> in your description) points at refs/heads/my-topic\n(that's the <refname2> in your description).\n\nAnd when you create a new commit from this state, the logs of these\ntwo refs will gain one entry each.  \"git log HEAD@{now} would show\nthe recent progress of HEAD, \"git log my-topic@{now}\" would show the\nrecent progress of my-topic (\"my-topic@\"now}\" can also be spelled as\n\"@{now}).\n\nThe <refname1> (HEAD) will keep referring to <refname2> (my-topic)\nuntil you switch branches, and does not change even if <refname2>\npoints at a different commit, as it is \"symbolic\".\n\n> Since commit 523fa69c36744ae6 (\"reflog: cleanse messages in the refs.c\n> layer\", 2020-07-10, v2.29.0), the effect of using the aforementioned\n> \"symbolic-ref\" command on ref logs has changed in an unexpected way.\n\nPlease explain \"in an unexpected way\" here in the log message.  Not\nevery reader can read your mind and would expect the same as you do.\n\nThe said commit came as part of this topic, ...\n\nhttps://lore.kernel.org/git/pull.669.v2.git.1594401593.gitgitgadget@gmail.com/\n\n... so I've added the true author of it on the Cc: list.\n\n> Add a new set of tests to exercise and demonstrate this change\n> in preparation for correcting it (at which point the failing tests\n> will be flipped from `test_expect_failure` to `test_expect_success`).\n\nWe usually prefer not to do the two-step \"expect_failure first and\nthen in a separate patch flip that to _success\", as it makes it hard\nto see the \"fix\" step (because the change in the test would be shown\nonly 3 lines before and after _failure->_success line, and what the\ntest is doing cannot be seen in the patch by itself).\n\nThanks.\n\n"},{"id":"415667","messageId":"028152B6-DA5B-40F7-B944-FF4F31C2BC56@gmail.com","threadId":"55074","inReplyTo":"xmqqa6sqp827.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2021-01-30T23:02:03Z","receivedAt":"2021-01-30T23:12:42Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jan 30, 2021, at 11:56, Junio C Hamano wrote:\n\n> The said commit came as part of this topic, ...\n>\n> https://lore.kernel.org/git/pull.669.v2.git.1594401593.gitgitgadget@gmail.com/\n>\n> ... so I've added the true author of it on the Cc: list.\n\nOut of curiosity, if Han-Wen Nienhuys is the true author of commit  \n523fa69c36744ae6 why is it that you are both the committer and author  \nof that commit in the commit's header?\n\nThe answer may also inform others trying to determine the best list of  \nrecipients for patches...\n"},{"id":"415668","messageId":"81AEEED6-26EC-4F32-AA65-B028435D812D@gmail.com","threadId":"55074","inReplyTo":"xmqqa6sqp827.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs","fromName":"Kyle J. McKay","fromEmail":"mackyle@gmail.com","sentAt":"2021-01-30T23:26:55Z","receivedAt":"2021-01-30T23:28:03Z","isPatch":true,"sender":{"key":"mackyle@gmail.com","avatar":"https://avatars.githubusercontent.com/u/813346?v=4"},"body":"On Jan 30, 2021, at 11:56, Junio C Hamano wrote:\n\n>> Add a new set of tests to exercise and demonstrate this change\n>> in preparation for correcting it (at which point the failing tests\n>> will be flipped from `test_expect_failure` to `test_expect_success`).\n>\n> We usually prefer not to do the two-step \"expect_failure first and\n> then in a separate patch flip that to _success\", as it makes it hard\n> to see the \"fix\" step (because the change in the test would be shown\n> only 3 lines before and after _failure->_success line, and what the\n> test is doing cannot be seen in the patch by itself).\n\nI'm having a bit of trouble parsing that into expectations.  A little  \nhelp please.\n\nGiven the scenario that a bug is found that is not being caught by a  \ntest (irrespective of whether or not the outcome of this particular  \nseries discussion results in it being determined to be a \"bug\").\n\nFurther, if the fix is simple enough that it warrants only a single  \npatch, what is the preferred order of patches then?\n\nI would like the patch series to demonstrate:\n\n1) that the test actually catches the bug (in case it comes back in  \nthe future)\n2) the fix isolated (as much as possible) in a patch distinct from the  \ntest\n3) that the test now passes, preferably with minimal changes to be  \nsure that it hasn't accidentally been rendered ineffective\n\nAll the while, at no point after commits for (1), (2), or (3) is the  \ntest suite allowed to generate extra failures (that are not marked  \n\"expect failure\").\n\npatch 1/2 accomplishes (1)\npatch 2/2 does both (2) and (3)\n\nAre you suggesting that (1) just be omitted?  Or that it be modified  \nso that it's an \"expect success\" patch?  But then (2) would break it  \nand introduce a failing test into the test suite.  Should (2) then  \njust flip the test from test_expect_success to test_expect_failure and  \nthen a separate commit flip it back to test_expect_success along with  \nthe minor change to the test itself to make it start passing again?   \nIn that case, there's always a risk that changing the test itself  \ncould accidentally make it no longer detect the problem anymore and  \njust always succeed even if the problem comes back.\n\nWithout an initial expect_failure step (1), there's no commit in the  \nrepository that can demonstrate that the test actually catches the  \nproblem.\n\nI understand that when new code is added, the tests come after, but it  \nseems to me that when fixing a pre-existing problem, the test that  \ndemonstrates the problem should come first and be an expect failure  \nsince it is considered to be a problem.\n\nWhat exactly is the order of test/fix changes that you're expecting to  \nsee here?\n\nThanks.\n"},{"id":"415670","messageId":"xmqqy2gang0e.fsf@gitster.c.googlers.com","threadId":"55074","inReplyTo":"028152B6-DA5B-40F7-B944-FF4F31C2BC56@gmail.com","subject":"Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-30T23:48:01Z","receivedAt":"2021-01-31T00:06:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> On Jan 30, 2021, at 11:56, Junio C Hamano wrote:\n>\n>> The said commit came as part of this topic, ...\n>>\n>> https://lore.kernel.org/git/pull.669.v2.git.1594401593.gitgitgadget@gmail.com/\n>>\n>> ... so I've added the true author of it on the Cc: list.\n>\n> Out of curiosity, if Han-Wen Nienhuys is the true author of commit\n> 523fa69c36744ae6 why is it that you are both the committer and author  \n> of that commit in the commit's header?\n\nSee how the e-mail message was formatted in that thread.  I just ran\n\"am\" on it (which makes me responsible for committing), and the\nauthorship comes from the \"From:\" that was in the body.  I suspect\nhe may have based the patch on some of the \"how about doing it like\nso\" suggestions I made during an earlier discussion and wanted to\ngive me credit for the input, but I do not remember the context the\npatch was originally written in X-<.\n\n\n\n"},{"id":"415671","messageId":"xmqqo8h6nexk.fsf@gitster.c.googlers.com","threadId":"55074","inReplyTo":"81AEEED6-26EC-4F32-AA65-B028435D812D@gmail.com","subject":"Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-31T00:11:19Z","receivedAt":"2021-01-31T00:12:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Kyle J. McKay\" <mackyle@gmail.com> writes:\n\n> I'm having a bit of trouble parsing that into expectations.  A little\n> help please.\n> Are you suggesting that (1) just be omitted?  Or that it be modified\n> so that it's an \"expect success\" patch?\n\nNeither.\n\nThe result of applying the current 1/2 and 2/2 on top of, say\n'master', would be the shape of the tree you would want to be in.\n\nOur preference is just to have it as a single patch, not as \"first\nexpect failure and then flip it to expect success while modifying\nthe code\".  That approach makes the second step harder to review\nthan necessary, because the \"git show\" output and \"format-patch\"\noutput from the step would show only very little about the test\nthat changes behaviour.\n\nEven with a single patch, if somebody wants a demonstration of what\nused to be broken without the code modification, it is easy to apply\nonly the test part of the single patch without using the code change\nto see how it breaks, so \"I want to demonstrate the breakage\" is not\na reason to have it as a separate step.\n\nThanks.\n"},{"id":"415731","messageId":"CAFQ2z_Mb86W7PnRfO2wcRqqS3UBOb+TpvOXQ5-UJr0aH6OnJFg@mail.gmail.com","threadId":"55074","inReplyTo":"xmqqy2gang0e.fsf@gitster.c.googlers.com","subject":"Re: [PATCH 1/2] t/t1417: test symbolic-ref effects on ref logs","fromName":"Han-Wen Nienhuys","fromEmail":"hanwen@google.com","sentAt":"2021-02-01T11:09:08Z","receivedAt":"2021-02-01T11:10:17Z","isPatch":true,"sender":{"key":"hanwen@google.com","avatar":"https://avatars.githubusercontent.com/u/31547?v=4"},"body":"On Sun, Jan 31, 2021 at 12:48 AM Junio C Hamano <gitster@pobox.com> wrote:\n> > On Jan 30, 2021, at 11:56, Junio C Hamano wrote:\n> >\n> >> The said commit came as part of this topic, ...\n> >>\n> >> https://lore.kernel.org/git/pull.669.v2.git.1594401593.gitgitgadget@gmail.com/\n> >>\n> >> ... so I've added the true author of it on the Cc: list.\n> >\n> > Out of curiosity, if Han-Wen Nienhuys is the true author of commit\n> > 523fa69c36744ae6 why is it that you are both the committer and author\n> > of that commit in the commit's header?\n>\n> See how the e-mail message was formatted in that thread.  I just ran\n> \"am\" on it (which makes me responsible for committing), and the\n> authorship comes from the \"From:\" that was in the body.  I suspect\n> he may have based the patch on some of the \"how about doing it like\n> so\" suggestions I made during an earlier discussion and wanted to\n> give me credit for the input, but I do not remember the context the\n> patch was originally written in X-<.\n\nThe classic reflog format doesn't allow '\\n' in messages, but\ndifferent parts of the code did try to write '\\n'. This patch was\nsupposed to sanitize the messages in a central location, so alternate\nref backends do not trigger spurious differences in how reflogs are\nrepresented.\n\nYour patch says\n\n> has changed in an unexpected way.\n\nCan you make the expectations and current behavior explicit?\n\n-- \nHan-Wen Nienhuys - Google Munich\nI work 80%. Don't expect answers from me on Fridays.\n--\nGoogle Germany GmbH, Erika-Mann-Strasse 33, 80636 Munich\nRegistergericht und -nummer: Hamburg, HRB 86891\nSitz der Gesellschaft: Hamburg\nGeschäftsführer: Paul Manicle, Halimah DeLaine Prado\n"}]}