{"thread":{"id":"46668","subject":"[PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames","startedAt":"2017-08-25T18:49:49Z","lastAt":"2017-09-09T10:47:18Z","messageCount":34,"participants":["Martin Ågren","Junio C Hamano","Michael Haggerty","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"327207","messageId":"40c5e954dd84ff42552bccfea00144eecdbd1c7e.1503496797.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":null,"subject":"[PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-25T18:49:15Z","receivedAt":"2017-08-25T18:49:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"files_transaction_prepare() and the functions it calls add strings to a\nstring list without duplicating them, i.e., we keep the original raw\npointers we were given. That is \"ok\", since we keep them only for a\nshort-enough time, but we end up leaking some of them.\n\nSwitch to duplicating the strings, so that affected_refnames does not\nleak memory. The original strings might still leak, but at least that\ncan now be addressed without worrying about these pointers.\n\nNo-one takes any pointers to the strings in the list (it is basically\nonly used to check for set membership), so it is ok for\nstring_list_clear to free them.\n\nSigned-off-by: Martin Ågren \n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5cca55510..22daca2ba 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2407,7 +2407,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n \t\t\t \"ref_transaction_prepare\");\n \tsize_t i;\n \tint ret = 0;\n-\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n+\tstruct string_list affected_refnames = STRING_LIST_INIT_DUP;\n \tchar *head_ref = NULL;\n \tint head_type;\n \tstruct object_id head_oid;\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327208","messageId":"fb9c0866cedac8c1d8fcf143641ebbee70d07ad7.1503496797.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"40c5e954dd84ff42552bccfea00144eecdbd1c7e.1503496797.git.martin.agren@gmail.com","subject":"[PATCH 2/2] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-25T18:49:16Z","receivedAt":"2017-08-25T18:49:55Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"We hand over the \"referent\" string (buffer) to several different\nfunctions, one of which would sometimes keep the raw pointer,\nreferent.buf. (When split_symref_update calls string_list_insert.) The\nprevious patch removed that behavior, so we can now safely release the\nstring buffer before returning.\n\nSigned-off-by: Martin Ågren \n---\nI appreciate Git's stance on memory leaks (\"if we're about to exit or\ndie, why bother?\") but this one feels different since\nlock_ref_for_update is called in a loop rather deep down the call stack.\nFeel free to tell me I'm wrong. Patch 1/2 does introduce some\nmalloc-churning. An alternative would be to call strbuf_release only\nwhen we know it's safe, but that feels really ugly.\n\n refs/files-backend.c | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 22daca2ba..6af07c404 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2252,7 +2252,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \tstruct strbuf referent = STRBUF_INIT;\n \tint mustexist = (update->flags & REF_HAVE_OLD) &&\n \t\t!is_null_oid(&update->old_oid);\n-\tint ret;\n+\tint ret = 0;\n \tstruct ref_lock *lock;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_for_update\");\n@@ -2264,7 +2264,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tret = split_head_update(update, transaction, head_ref,\n \t\t\t\t\taffected_refnames, err);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tret = lock_raw_ref(refs, update->refname, mustexist,\n@@ -2278,7 +2278,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n \t\t\t original_update_refname(update), reason);\n \t\tfree(reason);\n-\t\treturn ret;\n+\t\tgoto out;\n \t}\n \n \tupdate->backend_data = lock;\n@@ -2297,10 +2297,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\n \t\t\t\t\t\t original_update_refname(update));\n-\t\t\t\t\treturn -1;\n+\t\t\t\t\tret = -1;\n+\t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t} else if (check_old_oid(update, &lock->old_oid, err)) {\n-\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t} else {\n \t\t\t/*\n@@ -2314,13 +2316,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\t\t referent.buf, transaction,\n \t\t\t\t\t\t affected_refnames, err);\n \t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t\tgoto out;\n \t\t}\n \t} else {\n \t\tstruct ref_update *parent_update;\n \n-\t\tif (check_old_oid(update, &lock->old_oid, err))\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\tif (check_old_oid(update, &lock->old_oid, err)) {\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n+\t\t}\n \n \t\t/*\n \t\t * If this update is happening indirectly because of a\n@@ -2357,7 +2361,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t \"cannot update ref '%s': %s\",\n \t\t\t\t update->refname, write_err);\n \t\t\tfree(write_err);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t} else {\n \t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n \t\t}\n@@ -2371,10 +2376,14 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tif (close_ref(lock)) {\n \t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t update->refname);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t}\n \t}\n-\treturn 0;\n+\n+out:\n+\tstrbuf_release(&referent);\n+\treturn ret;\n }\n \n /*\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327220","messageId":"xmqq8ti7s6ph.fsf@gitster.mtv.corp.google.com","threadId":"46668","inReplyTo":"40c5e954dd84ff42552bccfea00144eecdbd1c7e.1503496797.git.martin.agren@gmail.com","subject":"Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-08-25T21:00:58Z","receivedAt":"2017-08-25T21:01:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> files_transaction_prepare() and the functions it calls add strings to a\n> string list without duplicating them, i.e., we keep the original raw\n> pointers we were given. That is \"ok\", since we keep them only for a\n> short-enough time, but we end up leaking some of them.\n\nSorry, but I do not understand this statement. If affected_refnames\nstring list borrows strings from other structures who own them, and\nnone of these strings are freed by clearing affected_refnames list,\nthat is not \"leaking\"---we didn't acquire the ownership, so it is\nnot our job to free them in the first place. Among the original\nowners of strings we borrow from, some may not properly free, in\nwhich case that is a leak.\n\nWhat problem are you solving?\n\n>\n> Switch to duplicating the strings, so that affected_refnames does not\n> leak memory. The original strings might still leak, but at least that\n> can now be addressed without worrying about these pointers.\n>\n> No-one takes any pointers to the strings in the list (it is basically\n> only used to check for set membership), so it is ok for\n> string_list_clear to free them.\n>\n> Signed-off-by: Martin Ågren \n> ---\n> refs/files-backend.c | 2 +-\n> 1 file changed, 1 insertion(+), 1 deletion(-)\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 5cca55510..22daca2ba 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2407,7 +2407,7 @@ static int files_transaction_prepare(struct ref_store *ref_store,\n> \t\t\t \"ref_transaction_prepare\");\n> \tsize_t i;\n> \tint ret = 0;\n> -\tstruct string_list affected_refnames = STRING_LIST_INIT_NODUP;\n> +\tstruct string_list affected_refnames = STRING_LIST_INIT_DUP;\n> \tchar *head_ref = NULL;\n> \tint head_type;\n> \tstruct object_id head_oid;\n"},{"id":"327244","messageId":"CAN0heSq8RdH5vWFgq1UvJOfWerMJSZwhV4FMCjvA=XUqu2OQQQ@mail.gmail.com","threadId":"46668","inReplyTo":"xmqq8ti7s6ph.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-26T10:16:44Z","receivedAt":"2017-08-26T10:16:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 25 August 2017 at 23:00, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> files_transaction_prepare() and the functions it calls add strings to a\n>> string list without duplicating them, i.e., we keep the original raw\n>> pointers we were given. That is \"ok\", since we keep them only for a\n>> short-enough time, but we end up leaking some of them.\n>\n> Sorry, but I do not understand this statement. If affected_refnames\n> string list borrows strings from other structures who own them, and\n> none of these strings are freed by clearing affected_refnames list,\n> that is not \"leaking\"---we didn't acquire the ownership, so it is\n> not our job to free them in the first place. Among the original\n> owners of strings we borrow from, some may not properly free, in\n> which case that is a leak.\n>\n> What problem are you solving?\n\nSorry. Maybe this explains my intentions better:\n\n In lock_ref_for_update(), we populate a strbuf \"referent\" through\n lock_raw_ref(). If we don't have a symref, we don't use \"referent\"\n for anything (and it won't have allocated any memory). Otherwise, we\n hand over referent.buf to someone who uses it immediately\n (refs_read_ref_full) or to someone who holds on to the pointer\n (split_symref_update ends up adding it to a string list). Therefore,\n at the end of lock_ref_for_update() we can't unconditionally release\n the strbuf, so we end up leaking it.\n\n We could release the strbuf when we know that it's safe (possibly\n also only when we know that it's needed). Instead, in preparation\n for the next patch, make the string list not hold on to the raw\n pointers, i.e., make it duplicate the strings on insertion and\n manage its own resources.\n\nOf course, the pointer-keeping and free-avoidance might be by design\nand/or wanted, e.g., to avoid excessive mallocing and freeing. I admit\nto not knowing what is a realistic number of iterations in the loop that\ncalls lock_ref_for_update, i.e., how severe this leak might be. Maybe\nthe \"backend\" nature of this code does not necessarily imply \"this could\nbe called any number of times throughout the process' lifetime\".\n"},{"id":"327274","messageId":"CAMy9T_FUGehTbfVfVfi877Gi3DOR5Z45A-H-v92s7g=m7RVnSw@mail.gmail.com","threadId":"46668","inReplyTo":"CAN0heSq8RdH5vWFgq1UvJOfWerMJSZwhV4FMCjvA=XUqu2OQQQ@mail.gmail.com","subject":"Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-28T08:06:57Z","receivedAt":"2017-08-28T09:18:41Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Sat, Aug 26, 2017 at 12:16 PM, Martin Ågren wrote:\n> On 25 August 2017 at 23:00, Junio C Hamano wrote:\n>> Martin Ågren writes:\n>>\n>>> files_transaction_prepare() and the functions it calls add strings to a\n>>> string list without duplicating them, i.e., we keep the original raw\n>>> pointers we were given. That is \"ok\", since we keep them only for a\n>>> short-enough time, but we end up leaking some of them.\n>>\n>> [...]\n\nGood find, Martin.\n\nFirst of all, you are right that we don't want any memory leaks here.\nNobody promises that the program will end soon if a reference update\nfails. (In fact, there are invocations of `git` that trigger multiple\nreference updates.) This is a small leak, but we should fix it.\n\nThe problem (if I may take a stab at explaining it in my own words) is\nthat `files_transaction_prepare()` uses a `string_list` called\n`affected_refnames` to ensure that the same reference name is not\nmodified more than once in a single reference transaction. Currently,\n`affected_refnames` is configured *not* to duplicate the refnames that\nare fed to it, which also means that it doesn't free the refnames when\nit is cleared.\n\nThis is correct for most refnames, which are owned by entries in the\n`ref_transaction`, and therefore have a longer lifetime than\n`affected_refnames`.\n\nBut there is one code path that can add a refname to\n`affected_refnames` without making a provision for the refname ever to\nbe freed:\n\n* files_transaction_prepare()\n * lock_ref_for_update()\n * split_symref_update()\n * item = string_list_insert(affected_refnames, referent)\n\nThe `referent` in the last statement comes from a `strbuf` that is\ncreated in `lock_ref_for_update()` then passed to `lock_raw_ref()`,\nwhich fills it.\n\nIt would be a serious bug if `lock_ref_for_update()` would dispose of\n`referent`, because the pointer in `affected_refnames` would point at\nfreed memory. But in fact we have the opposite problem;\n`lock_ref_for_update()` never frees the memory (it doesn't even\n`strbuf_detach()` it). So that memory is leaked.\n\nYour proposed solution is to change `affected_refnames` to duplicate\nthe strings that are stored in it, in which case\n`lock_ref_for_update()` can properly dispose of `referent`, fixing the\nleak. This works, but at the price of having to allocate memory for\n*all* references in the update, even though most of them are already\nfine.\n\nBut note that `split_symref_update()` *also* passes a copy of\n`referent` to `ref_transaction_add_update()`, which *already* makes a\ncopy of the reference name and adds an entry containing the name to\nthe `ref_transaction`. If we would store *that* copy to\n`affected_refnames`, then it would get freed when the\n`ref_transaction` is cleaned up, and we could fix the memory leak\nwithout allocating any new memory. This requires a little reorg of\n`split_symref_update()` but it's not too bad:\n\n* Do the initial check using `string_list_has_string()` rather than\ncalling `string_list_insert()` already.\n* After `new_update` has been created, call `string_list_insert()`,\npassing it `new_update->refname` as the string.\n\nIf this is done in place of your first commit, then your second commit\ncould be left unchanged.\n\nMichael\n"},{"id":"327275","messageId":"CAN0heSrkYL6OqVHHnDtA1DqE-FVqjB59bG2MsgxmJyxW33k9JA@mail.gmail.com","threadId":"46668","inReplyTo":"CAMy9T_FUGehTbfVfVfi877Gi3DOR5Z45A-H-v92s7g=m7RVnSw@mail.gmail.com","subject":"Re: [PATCH 1/2] refs/files-backend: duplicate strings added to affected_refnames","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-28T10:09:09Z","receivedAt":"2017-08-28T10:09:16Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 28 August 2017 at 10:06, Michael Haggerty wrote:\n> On Sat, Aug 26, 2017 at 12:16 PM, Martin Ågren wrote:\n>> On 25 August 2017 at 23:00, Junio C Hamano wrote:\n>>> Martin Ågren writes:\n>>>\n>>>> files_transaction_prepare() and the functions it calls add strings to a\n>>>> string list without duplicating them, i.e., we keep the original raw\n>>>> pointers we were given. That is \"ok\", since we keep them only for a\n>>>> short-enough time, but we end up leaking some of them.\n>>>\n>>> [...]\n>\n> Good find, Martin.\n>\n> First of all, you are right that we don't want any memory leaks here.\n> Nobody promises that the program will end soon if a reference update\n> fails. (In fact, there are invocations of `git` that trigger multiple\n> reference updates.) This is a small leak, but we should fix it.\n>\n> The problem (if I may take a stab at explaining it in my own words) is\n> that `files_transaction_prepare()` uses a `string_list` called\n> `affected_refnames` to ensure that the same reference name is not\n> modified more than once in a single reference transaction. Currently,\n> `affected_refnames` is configured *not* to duplicate the refnames that\n> are fed to it, which also means that it doesn't free the refnames when\n> it is cleared.\n>\n> This is correct for most refnames, which are owned by entries in the\n> `ref_transaction`, and therefore have a longer lifetime than\n> `affected_refnames`.\n>\n> But there is one code path that can add a refname to\n> `affected_refnames` without making a provision for the refname ever to\n> be freed:\n>\n> * files_transaction_prepare()\n> * lock_ref_for_update()\n> * split_symref_update()\n> * item = string_list_insert(affected_refnames, referent)\n>\n> The `referent` in the last statement comes from a `strbuf` that is\n> created in `lock_ref_for_update()` then passed to `lock_raw_ref()`,\n> which fills it.\n>\n> It would be a serious bug if `lock_ref_for_update()` would dispose of\n> `referent`, because the pointer in `affected_refnames` would point at\n> freed memory. But in fact we have the opposite problem;\n> `lock_ref_for_update()` never frees the memory (it doesn't even\n> `strbuf_detach()` it). So that memory is leaked.\n\nThanks for this very well-written description. It matches my\nunderstanding completely, which is comforting.\n\n> Your proposed solution is to change `affected_refnames` to duplicate\n> the strings that are stored in it, in which case\n> `lock_ref_for_update()` can properly dispose of `referent`, fixing the\n> leak. This works, but at the price of having to allocate memory for\n> *all* references in the update, even though most of them are already\n> fine.\n\nAgreed.\n\n> But note that `split_symref_update()` *also* passes a copy of\n> `referent` to `ref_transaction_add_update()`, which *already* makes a\n> copy of the reference name and adds an entry containing the name to\n> the `ref_transaction`. If we would store *that* copy to\n> `affected_refnames`, then it would get freed when the\n> `ref_transaction` is cleaned up, and we could fix the memory leak\n> without allocating any new memory. This requires a little reorg of\n> `split_symref_update()` but it's not too bad:\n>\n> * Do the initial check using `string_list_has_string()` rather than\n> calling `string_list_insert()` already.\n> * After `new_update` has been created, call `string_list_insert()`,\n> passing it `new_update->refname` as the string.\n\nI haven't looked at the code yet, but from what I remember, this would\na) work and b) be a straightforward rearrangement as you say. I'll give\nthis approach a try (unless you want to, of course; just holler).\n\n> If this is done in place of your first commit, then your second commit\n> could be left unchanged.\n\nThanks a lot for your comments. I appreciate it.\n\nMartin\n"},{"id":"327293","messageId":"7f6617d2f1df9ee6746e4cbc41dc84bdb7788574.1503951733.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"CAN0heSrkYL6OqVHHnDtA1DqE-FVqjB59bG2MsgxmJyxW33k9JA@mail.gmail.com","subject":"[PATCH v2 1/2] refs/files-backend: add longer-scoped copy of string to list","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-28T20:32:24Z","receivedAt":"2017-08-28T20:32:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"split_symref_update() receives a string-pointer `referent` and adds it\nto the list of `affected_refnames`. The list simply holds on to the\npointers it is given, it does not copy the strings and it never frees\nthem. The `referent` string in split_symref_update() belongs to a string\nbuffer in the caller. After we return, the string will be leaked.\n\nIn the next patch, we want to properly release the string buffer in the\ncaller, but we can't safely do so until we've made sure that\n`affected_refnames` will not be holding on to a pointer to the string.\nWe could configure the list to handle its own resources, but it would\nmean some alloc/free-churning. The list is already handling other\nstrings (through other code paths) which we do not need to worry about,\nand we'd be memory-churning those strings too, completely unnecessary.\n\nObserve that split_symref_update() creates a `new_update`-object and\nthat `new_update->refname` is then a copy of `referent`. The difference\nis, this copy will be freed, and it will be freed *after*\n`affected_refnames` has been cleared.\n\nRearrange the handling of `referent`, so that we don't add it directly\nto `affected_refnames`. Instead, first just check whether `referent`\nexists in the string list, and later add `new_update->refname`.\n\nHelped-by: Michael Haggerty \nSigned-off-by: Martin Ågren \n---\nThanks Junio and Michael for your comments on the first version. This\nfirst patch is now completely different and much much better (thanks\nMichael!). The commit message should also be better (sorry Junio...).\nThe second one has a new commit message, but the diff is the same.\n\nMartin\n\n refs/files-backend.c | 16 ++++++++++++----\n 1 file changed, 12 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 5cca55510..bdb0e22e5 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2140,13 +2140,12 @@ static int split_symref_update(struct files_ref_store *refs,\n \n \t/*\n \t * First make sure that referent is not already in the\n-\t * transaction. This insertion is O(N) in the transaction\n+\t * transaction. This check is O(N) in the transaction\n \t * size, but it happens at most once per symref in a\n \t * transaction.\n \t */\n-\titem = string_list_insert(affected_refnames, referent);\n-\tif (item->util) {\n-\t\t/* An entry already existed */\n+\tif (string_list_has_string(affected_refnames, referent)) {\n+\t\t/* An entry already exists */\n \t\tstrbuf_addf(err,\n \t\t\t \"multiple updates for '%s' (including one \"\n \t\t\t \"via symref '%s') are not allowed\",\n@@ -2181,6 +2180,15 @@ static int split_symref_update(struct files_ref_store *refs,\n \tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n \tupdate->flags &= ~REF_HAVE_OLD;\n \n+\t/*\n+\t * Add the referent. This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per symref in a\n+\t * transaction. Make sure to add new_update->refname, which will\n+\t * be valid as long as affected_refnames is in use, and NOT\n+\t * referent, which might soon be freed by our caller.\n+\t */\n+\titem = string_list_insert(affected_refnames, new_update->refname);\n+\tassert(!item->util);\n \titem->util = new_update;\n \n \treturn 0;\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327294","messageId":"fe842671061cea7972b0c8695a4b7f82f854453a.1503951733.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"CAN0heSrkYL6OqVHHnDtA1DqE-FVqjB59bG2MsgxmJyxW33k9JA@mail.gmail.com","subject":"[PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-28T20:32:25Z","receivedAt":"2017-08-28T20:33:01Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After the previous patch, none of the functions we call hold on to\n`referent.buf`, so we can safely release the string buffer before\nreturning.\n\nSigned-off-by: Martin Ågren \n---\n refs/files-backend.c | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex bdb0e22e5..15f34b10e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2260,7 +2260,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \tstruct strbuf referent = STRBUF_INIT;\n \tint mustexist = (update->flags & REF_HAVE_OLD) &&\n \t\t!is_null_oid(&update->old_oid);\n-\tint ret;\n+\tint ret = 0;\n \tstruct ref_lock *lock;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_for_update\");\n@@ -2272,7 +2272,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tret = split_head_update(update, transaction, head_ref,\n \t\t\t\t\taffected_refnames, err);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tret = lock_raw_ref(refs, update->refname, mustexist,\n@@ -2286,7 +2286,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n \t\t\t original_update_refname(update), reason);\n \t\tfree(reason);\n-\t\treturn ret;\n+\t\tgoto out;\n \t}\n \n \tupdate->backend_data = lock;\n@@ -2305,10 +2305,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\n \t\t\t\t\t\t original_update_refname(update));\n-\t\t\t\t\treturn -1;\n+\t\t\t\t\tret = -1;\n+\t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t} else if (check_old_oid(update, &lock->old_oid, err)) {\n-\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t} else {\n \t\t\t/*\n@@ -2322,13 +2324,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\t\t referent.buf, transaction,\n \t\t\t\t\t\t affected_refnames, err);\n \t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t\tgoto out;\n \t\t}\n \t} else {\n \t\tstruct ref_update *parent_update;\n \n-\t\tif (check_old_oid(update, &lock->old_oid, err))\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\tif (check_old_oid(update, &lock->old_oid, err)) {\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n+\t\t}\n \n \t\t/*\n \t\t * If this update is happening indirectly because of a\n@@ -2365,7 +2369,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t \"cannot update ref '%s': %s\",\n \t\t\t\t update->refname, write_err);\n \t\t\tfree(write_err);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t} else {\n \t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n \t\t}\n@@ -2379,10 +2384,14 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tif (close_ref(lock)) {\n \t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t update->refname);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t}\n \t}\n-\treturn 0;\n+\n+out:\n+\tstrbuf_release(&referent);\n+\treturn ret;\n }\n \n /*\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327315","messageId":"49134978-130d-ff43-e3c6-bde211710064@alum.mit.edu","threadId":"46668","inReplyTo":"7f6617d2f1df9ee6746e4cbc41dc84bdb7788574.1503951733.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 1/2] refs/files-backend: add longer-scoped copy of string to list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:33:18Z","receivedAt":"2017-08-29T08:33:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/28/2017 10:32 PM, Martin Ågren wrote:\n> split_symref_update() receives a string-pointer `referent` and adds it\n> to the list of `affected_refnames`. The list simply holds on to the\n> pointers it is given, it does not copy the strings and it never frees\n> them. The `referent` string in split_symref_update() belongs to a string\n> buffer in the caller. After we return, the string will be leaked.\n> \n> In the next patch, we want to properly release the string buffer in the\n> caller, but we can't safely do so until we've made sure that\n> `affected_refnames` will not be holding on to a pointer to the string.\n> We could configure the list to handle its own resources, but it would\n> mean some alloc/free-churning. The list is already handling other\n> strings (through other code paths) which we do not need to worry about,\n> and we'd be memory-churning those strings too, completely unnecessary.\n> \n> Observe that split_symref_update() creates a `new_update`-object and\n> that `new_update->refname` is then a copy of `referent`. The difference\n> is, this copy will be freed, and it will be freed *after*\n> `affected_refnames` has been cleared.\n> \n> Rearrange the handling of `referent`, so that we don't add it directly\n> to `affected_refnames`. Instead, first just check whether `referent`\n> exists in the string list, and later add `new_update->refname`.\n> \n> Helped-by: Michael Haggerty \n> Signed-off-by: Martin Ågren \n> ---\n> Thanks Junio and Michael for your comments on the first version. This\n> first patch is now completely different and much much better (thanks\n> Michael!). The commit message should also be better (sorry Junio...).\n> The second one has a new commit message, but the diff is the same.\n> \n> Martin\n> \n> refs/files-backend.c | 16 ++++++++++++----\n> 1 file changed, 12 insertions(+), 4 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 5cca55510..bdb0e22e5 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2140,13 +2140,12 @@ static int split_symref_update(struct files_ref_store *refs,\n> \n> \t/*\n> \t * First make sure that referent is not already in the\n> -\t * transaction. This insertion is O(N) in the transaction\n> +\t * transaction. This check is O(N) in the transaction\n\nThe check is not O(N); it is O(lg N) because it is implemented via a\nbinary search in the (sorted) `affected_refnames`. The insertion below\nis still O(N), because it has to shift entries later in the list to the\nright to make room for the new entry.\n\n> \t * size, but it happens at most once per symref in a\n> \t * transaction.\n> \t */\n> -\titem = string_list_insert(affected_refnames, referent);\n> -\tif (item->util) {\n> -\t\t/* An entry already existed */\n> +\tif (string_list_has_string(affected_refnames, referent)) {\n> +\t\t/* An entry already exists */\n> \t\tstrbuf_addf(err,\n> \t\t\t \"multiple updates for '%s' (including one \"\n> \t\t\t \"via symref '%s') are not allowed\",\n> @@ -2181,6 +2180,15 @@ static int split_symref_update(struct files_ref_store *refs,\n> \tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n> \tupdate->flags &= ~REF_HAVE_OLD;\n> \n> +\t/*\n> +\t * Add the referent. This insertion is O(N) in the transaction\n> +\t * size, but it happens at most once per symref in a\n> +\t * transaction. Make sure to add new_update->refname, which will\n> +\t * be valid as long as affected_refnames is in use, and NOT\n> +\t * referent, which might soon be freed by our caller.\n> +\t */\n> +\titem = string_list_insert(affected_refnames, new_update->refname);\n> +\tassert(!item->util);\n\nWe generally avoid using `assert()`. It would be preferable to use\n\n if (item->util)\n BUG(\"%s unexpectedly found in affected_refnames\",\nnew_update->refname);\n\n> \titem->util = new_update;\n> \n> \treturn 0;\n> \n\nOtherwise, looks good!\n\nMichael\n"},{"id":"327316","messageId":"425e16e7-188e-ecff-3a09-228ef1dcf319@alum.mit.edu","threadId":"46668","inReplyTo":"fe842671061cea7972b0c8695a4b7f82f854453a.1503951733.git.martin.agren@gmail.com","subject":"Re: [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-29T08:39:33Z","receivedAt":"2017-08-29T08:39:44Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/28/2017 10:32 PM, Martin Ågren wrote:\n> After the previous patch, none of the functions we call hold on to\n> `referent.buf`, so we can safely release the string buffer before\n> returning.\n\nThis patch looks good to me, but I did notice a pre-existing problem in\nthe area...\n\n> ---\n> refs/files-backend.c | 31 ++++++++++++++++++++-----------\n> 1 file changed, 20 insertions(+), 11 deletions(-)\n> \n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index bdb0e22e5..15f34b10e 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> [...]\n> @@ -2305,10 +2305,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n> \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n> \t\t\t\t\t\t \"error reading reference\",\n> \t\t\t\t\t\t original_update_refname(update));\n> -\t\t\t\t\treturn -1;\n> +\t\t\t\t\tret = -1;\n> +\t\t\t\t\tgoto out;\n\nIt is incorrect to return -1 here. First of all, stylistically, the\nreturn value should be a symbolic constant. But in fact, it should be\nreturning `TRANSACTION_GENERIC_ERROR` here, whereas -1 is\n`TRANSACTION_NAME_CONFLICT`. So the code is not just stylistically\nwrong; it is functionally wrong.\n\nI know that this is not your mistake, but would you like to add another\npatch to your series to fix this up? I'd do it myself, but it's a little\nbit awkward because the fix will conflict with your patch.\n\nThanks,\nMichael\n"},{"id":"327319","messageId":"CAN0heSqn59sFF3A-eQ593G+ZDWnO9pKM5F=sgiSQk+prUr-nSQ@mail.gmail.com","threadId":"46668","inReplyTo":"425e16e7-188e-ecff-3a09-228ef1dcf319@alum.mit.edu","subject":"Re: [PATCH v2 2/2] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-29T10:41:49Z","receivedAt":"2017-08-29T10:41:57Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 29 August 2017 at 10:39, Michael Haggerty wrote:\n> On 08/28/2017 10:32 PM, Martin Ågren wrote:\n>> After the previous patch, none of the functions we call hold on to\n>> `referent.buf`, so we can safely release the string buffer before\n>> returning.\n>\n> This patch looks good to me, but I did notice a pre-existing problem in\n> the area...\n>\n>> ---\n>> refs/files-backend.c | 31 ++++++++++++++++++++-----------\n>> 1 file changed, 20 insertions(+), 11 deletions(-)\n>>\n>> diff --git a/refs/files-backend.c b/refs/files-backend.c\n>> index bdb0e22e5..15f34b10e 100644\n>> --- a/refs/files-backend.c\n>> +++ b/refs/files-backend.c\n>> [...]\n>> @@ -2305,10 +2305,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n>> strbuf_addf(err, \"cannot lock ref '%s': \"\n>> \"error reading reference\",\n>> original_update_refname(update));\n>> - return -1;\n>> + ret = -1;\n>> + goto out;\n>\n> It is incorrect to return -1 here. First of all, stylistically, the\n> return value should be a symbolic constant. But in fact, it should be\n> returning `TRANSACTION_GENERIC_ERROR` here, whereas -1 is\n> `TRANSACTION_NAME_CONFLICT`. So the code is not just stylistically\n> wrong; it is functionally wrong.\n>\n> I know that this is not your mistake, but would you like to add another\n> patch to your series to fix this up? I'd do it myself, but it's a little\n> bit awkward because the fix will conflict with your patch.\n\nSure. I'll send out a v3 later today. I'll fix this in a third patch,\nand I'll also address your comments on the first patch.\n\nThanks a lot.\n\nMartin\n"},{"id":"327326","messageId":"4b4c0d178ad2216eecbc49fb6f54dd8a1d1ac119.1504024261.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"CAN0heSqn59sFF3A-eQ593G+ZDWnO9pKM5F=sgiSQk+prUr-nSQ@mail.gmail.com","subject":"[PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-29T17:18:22Z","receivedAt":"2017-08-29T17:18:49Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"split_symref_update() receives a string-pointer `referent` and adds it\nto the list of `affected_refnames`. The list simply holds on to the\npointers it is given, it does not copy the strings and it does not ever\nfree them. The `referent` string in split_symref_update() belongs to a\nstring buffer in the caller. After we return, the string will be leaked.\n\nIn the next patch, we want to properly release the string buffer in the\ncaller, but we can't safely do so until we've made sure that\n`affected_refnames` will not be holding on to a pointer to the string.\nWe could configure the list to handle its own resources, but it would\nmean some alloc/free-churning. The list is already handling other\nstrings (through other code paths) which we do not need to worry about,\nand we'd be memory-churning those strings too, completely unnecessary.\n\nObserve that split_symref_update() creates a `new_update`-object through\nref_transaction_add_update(), after which `new_update->refname` is a\ncopy of `referent`. The difference is, this copy will be freed, and it\nwill be freed *after* `affected_refnames` has been cleared.\n\nRearrange the handling of `referent`, so that we don't add it directly\nto `affected_refnames`. Instead, first just check whether `referent`\nexists in the string list, and later add `new_update->refname`.\n\nHelped-by: Michael Haggerty \nSigned-off-by: Martin Ågren \n---\nv3: \"O(lg N)\" in comment; if-BUG() instead of assert()\n\n refs/files-backend.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex fccbc24ac..73615d869 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2144,13 +2144,12 @@ static int split_symref_update(struct files_ref_store *refs,\n \n \t/*\n \t * First make sure that referent is not already in the\n-\t * transaction. This insertion is O(N) in the transaction\n+\t * transaction. This check is O(lg N) in the transaction\n \t * size, but it happens at most once per symref in a\n \t * transaction.\n \t */\n-\titem = string_list_insert(affected_refnames, referent);\n-\tif (item->util) {\n-\t\t/* An entry already existed */\n+\tif (string_list_has_string(affected_refnames, referent)) {\n+\t\t/* An entry already exists */\n \t\tstrbuf_addf(err,\n \t\t\t \"multiple updates for '%s' (including one \"\n \t\t\t \"via symref '%s') are not allowed\",\n@@ -2185,6 +2184,17 @@ static int split_symref_update(struct files_ref_store *refs,\n \tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n \tupdate->flags &= ~REF_HAVE_OLD;\n \n+\t/*\n+\t * Add the referent. This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per symref in a\n+\t * transaction. Make sure to add new_update->refname, which will\n+\t * be valid as long as affected_refnames is in use, and NOT\n+\t * referent, which might soon be freed by our caller.\n+\t */\n+\titem = string_list_insert(affected_refnames, new_update->refname);\n+\tif (item->util)\n+\t\tBUG(\"%s unexpectedly found in affected_refnames\",\n+\t\t new_update->refname);\n \titem->util = new_update;\n \n \treturn 0;\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327327","messageId":"f136e83168453af7a0d1eba45c8ad47d01dd9a41.1504024261.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"CAN0heSqn59sFF3A-eQ593G+ZDWnO9pKM5F=sgiSQk+prUr-nSQ@mail.gmail.com","subject":"[PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-29T17:18:23Z","receivedAt":"2017-08-29T17:18:56Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After the previous patch, none of the functions we call hold on to\n`referent.buf`, so we can safely release the string buffer before\nreturning.\n\nSigned-off-by: Martin Ågren \n---\nv3: identical to v2\n\n refs/files-backend.c | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 73615d869..a2b3df21b 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2266,7 +2266,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \tstruct strbuf referent = STRBUF_INIT;\n \tint mustexist = (update->flags & REF_HAVE_OLD) &&\n \t\t!is_null_oid(&update->old_oid);\n-\tint ret;\n+\tint ret = 0;\n \tstruct ref_lock *lock;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_for_update\");\n@@ -2278,7 +2278,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tret = split_head_update(update, transaction, head_ref,\n \t\t\t\t\taffected_refnames, err);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tret = lock_raw_ref(refs, update->refname, mustexist,\n@@ -2292,7 +2292,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n \t\t\t original_update_refname(update), reason);\n \t\tfree(reason);\n-\t\treturn ret;\n+\t\tgoto out;\n \t}\n \n \tupdate->backend_data = lock;\n@@ -2311,10 +2311,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\n \t\t\t\t\t\t original_update_refname(update));\n-\t\t\t\t\treturn -1;\n+\t\t\t\t\tret = -1;\n+\t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t} else if (check_old_oid(update, &lock->old_oid, err)) {\n-\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t} else {\n \t\t\t/*\n@@ -2328,13 +2330,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\t\t referent.buf, transaction,\n \t\t\t\t\t\t affected_refnames, err);\n \t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t\tgoto out;\n \t\t}\n \t} else {\n \t\tstruct ref_update *parent_update;\n \n-\t\tif (check_old_oid(update, &lock->old_oid, err))\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\tif (check_old_oid(update, &lock->old_oid, err)) {\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n+\t\t}\n \n \t\t/*\n \t\t * If this update is happening indirectly because of a\n@@ -2371,7 +2375,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t \"cannot update ref '%s': %s\",\n \t\t\t\t update->refname, write_err);\n \t\t\tfree(write_err);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t} else {\n \t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n \t\t}\n@@ -2385,10 +2390,14 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tif (close_ref(lock)) {\n \t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t update->refname);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t}\n \t}\n-\treturn 0;\n+\n+out:\n+\tstrbuf_release(&referent);\n+\treturn ret;\n }\n \n /*\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327328","messageId":"cbcdc8f166287394179c2ab7f3a0564a0d0eee6d.1504024261.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"CAN0heSqn59sFF3A-eQ593G+ZDWnO9pKM5F=sgiSQk+prUr-nSQ@mail.gmail.com","subject":"[PATCH v3 3/3] refs/files-backend: correct return value in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-29T17:18:24Z","receivedAt":"2017-08-29T17:19:02Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In one code path we return a literal -1 and not a symbolic constant. The\nvalue -1 would be interpreted as TRANSACTION_NAME_CONFLICT, which is\nwrong. Use TRANSACTION_GENERIC_ERROR instead (that is the only other\nreturn value we have to choose from).\n\nNoticed-by: Michael Haggerty \nSigned-off-by: Martin Ågren \n---\nv3: this is a new patch, as suggested by Michael\n\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex a2b3df21b..ad05d1d5f 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2311,7 +2311,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\n \t\t\t\t\t\t original_update_refname(update));\n-\t\t\t\t\tret = -1;\n+\t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t} else if (check_old_oid(update, &lock->old_oid, err)) {\n-- \n2.14.1.151.g45c1275a3.dirty\n\n"},{"id":"327342","messageId":"91e365b5-6a5d-1e1c-ab7a-579efa7c1ae8@alum.mit.edu","threadId":"46668","inReplyTo":"4b4c0d178ad2216eecbc49fb6f54dd8a1d1ac119.1504024261.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-08-30T02:52:20Z","receivedAt":"2017-08-30T02:52:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"v3 looks good to me. Thanks!\n\nReviewed-by: Michael Haggerty \n\nMichael\n"},{"id":"327445","messageId":"CAN0heSqa8OnPnkd1xbyZ=QN9qg_8OaxBYnwzOZDDA3g+uGE71g@mail.gmail.com","threadId":"46668","inReplyTo":"91e365b5-6a5d-1e1c-ab7a-579efa7c1ae8@alum.mit.edu","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-08-30T18:02:44Z","receivedAt":"2017-08-30T18:02:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 30 August 2017 at 04:52, Michael Haggerty wrote:\n> v3 looks good to me. Thanks!\n>\n> Reviewed-by: Michael Haggerty \n\nThank _you_ for very helpful feedback on the earlier versions.\n\nMartin\n"},{"id":"327540","messageId":"20170905084507.wntm3bfzfezgimcz@sigill.intra.peff.net","threadId":"46668","inReplyTo":"4b4c0d178ad2216eecbc49fb6f54dd8a1d1ac119.1504024261.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T08:45:07Z","receivedAt":"2017-09-05T08:45:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 07:18:22PM +0200, Martin Ågren wrote:\n\n> Observe that split_symref_update() creates a `new_update`-object through\n> ref_transaction_add_update(), after which `new_update->refname` is a\n> copy of `referent`. The difference is, this copy will be freed, and it\n> will be freed *after* `affected_refnames` has been cleared.\n> \n> Rearrange the handling of `referent`, so that we don't add it directly\n> to `affected_refnames`. Instead, first just check whether `referent`\n> exists in the string list, and later add `new_update->refname`.\n\nCoincidentally[1] I came across this same leak, and my solution ended up\nslightly different. I'll share it here in case it's of interest.\n\nIn your solution we end up searching the string list twice: once to see\nif we have the item, and then again to insert it. Whereas in the\noriginal we did both with a single search.\n\nBut we can observe that either:\n\n 1. The item already existed, in which case our insert was a noop, and\n we're good.\n\nor\n\n 2. We inserted it, in which case we proceed with creating new_update.\n\n We can then in O(1) replace the pointer in the string list item\n with the storage in new_update. We know we're not violating any\n string_list invariants because the strings contain the same bytes.\n\nI.e.:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 9266f5ab9d..1d16c1b33e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2178,6 +2178,11 @@ static int split_symref_update(struct files_ref_store *refs,\n \tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n \tupdate->flags &= ~REF_HAVE_OLD;\n \n+\t/*\n+\t * Re-point at the storage provided by our ref_update, which we know\n+\t * will last as long as the affected_refnames list.\n+\t */\n+\titem->string = new_update->refname;\n \titem->util = new_update;\n \n \treturn 0;\n\nIt feels pretty dirty, though. It would certainly be a bug if we ever\ndecided to switch affected_refnames to duplicate its strings.\n\nSo given that your solution is only a constant-time factor worse in\nefficiency, we should probably prefer it as the more maintainable\noption.\n\n-Peff\n\n[1] It's not really a coincidence, of course. All the recent leak\n discussion has got both of us prodding at Git with various tools. :)\n"},{"id":"327541","messageId":"20170905084751.c4rbeuxx5rdqwdgo@sigill.intra.peff.net","threadId":"46668","inReplyTo":"f136e83168453af7a0d1eba45c8ad47d01dd9a41.1504024261.git.martin.agren@gmail.com","subject":"Re: [PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T08:47:52Z","receivedAt":"2017-09-05T08:48:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Aug 29, 2017 at 07:18:23PM +0200, Martin Ågren wrote:\n\n> After the previous patch, none of the functions we call hold on to\n> `referent.buf`, so we can safely release the string buffer before\n> returning.\n\nI ended up doing this a little differently in my version:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 9266f5ab9d..1d16c1b33e 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2287,9 +2292,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t * the transaction, so we have to read it here\n \t\t\t * to record and possibly check old_sha1:\n \t\t\t */\n-\t\t\tif (refs_read_ref_full(&refs->base,\n-\t\t\t\t\t referent.buf, 0,\n-\t\t\t\t\t lock->old_oid.hash, NULL)) {\n+\t\t\tret = refs_read_ref_full(&refs->base,\n+\t\t\t\t\t\t referent.buf, 0,\n+\t\t\t\t\t\t lock->old_oid.hash, NULL);\n+\t\t\tstrbuf_release(&referent);\n+\n+\t\t\tif (ret) {\n \t\t\t\tif (update->flags & REF_HAVE_OLD) {\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\n@@ -2310,6 +2318,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\tret = split_symref_update(refs, update,\n \t\t\t\t\t\t referent.buf, transaction,\n \t\t\t\t\t\t affected_refnames, err);\n+\t\t\tstrbuf_release(&referent);\n \t\t\tif (ret)\n \t\t\t\treturn ret;\n \t\t}\n\nAfter we look at referent.buf once in each of the branch arms, we don't\nneed it at all. Disposing of it there means we don't have to worry about\nit in all of the later early-returns.\n\nI'm assuming that referent will always be empty unless REF_ISSYMREF is\nset. Which seems reasonable, but I didn't double check.\n\nFood for thought. I'd be OK with either version.\n\n-Peff\n"},{"id":"327542","messageId":"CAMy9T_F5HWF=hinxdp0C+pgyH_8MnR31p=XK1rjyMiM3UwST_w@mail.gmail.com","threadId":"46668","inReplyTo":"20170905084507.wntm3bfzfezgimcz@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2017-09-05T09:03:36Z","receivedAt":"2017-09-05T09:03:48Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On Tue, Sep 5, 2017 at 10:45 AM, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 07:18:22PM +0200, Martin Ågren wrote:\n>\n>> [...]\n>> Rearrange the handling of `referent`, so that we don't add it directly\n>> to `affected_refnames`. Instead, first just check whether `referent`\n>> exists in the string list, and later add `new_update->refname`.\n>\n> Coincidentally[1] I came across this same leak, and my solution ended up\n> slightly different. I'll share it here in case it's of interest.\n>\n> In your solution we end up searching the string list twice: once to see\n> if we have the item, and then again to insert it. Whereas in the\n> original we did both with a single search.\n>\n> But we can observe that either:\n>\n> 1. The item already existed, in which case our insert was a noop, and\n> we're good.\n>\n> or\n>\n> 2. We inserted it, in which case we proceed with creating new_update.\n>\n> We can then in O(1) replace the pointer in the string list item\n> with the storage in new_update. We know we're not violating any\n> string_list invariants because the strings contain the same bytes.\n>\n> I.e.:\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 9266f5ab9d..1d16c1b33e 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2178,6 +2178,11 @@ static int split_symref_update(struct files_ref_store *refs,\n> update->flags |= REF_LOG_ONLY | REF_NODEREF;\n> update->flags &= ~REF_HAVE_OLD;\n>\n> + /*\n> + * Re-point at the storage provided by our ref_update, which we know\n> + * will last as long as the affected_refnames list.\n> + */\n> + item->string = new_update->refname;\n> item->util = new_update;\n>\n> return 0;\n>\n> It feels pretty dirty, though. It would certainly be a bug if we ever\n> decided to switch affected_refnames to duplicate its strings.\n>\n> So given that your solution is only a constant-time factor worse in\n> efficiency, we should probably prefer it as the more maintainable\n> option.\n\nThis is clever, but I don't like that it requires outside code to\nchange internal `string_list` structures in a way that is not\ndocumented to be OK.\n\nIf we cared about getting rid of the extra `O(lg N)` search (and I\nagree with you that it doesn't matter in this case), I think the clean\nway to do it would be for `string_list` to expose a method like\n\n struct string_list_item *string_list_insert_at_index(struct\nstring_list *list, size_t index, const char *string);\n\nand to use it, together with `string_list_find_insert_index()`, to\navoid having to search twice.\n\nMichael\n"},{"id":"327543","messageId":"20170905090433.elysfemrgxhze2xn@sigill.intra.peff.net","threadId":"46668","inReplyTo":"CAMy9T_F5HWF=hinxdp0C+pgyH_8MnR31p=XK1rjyMiM3UwST_w@mail.gmail.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T09:04:33Z","receivedAt":"2017-09-05T09:04:43Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 11:03:36AM +0200, Michael Haggerty wrote:\n\n> > It feels pretty dirty, though. It would certainly be a bug if we ever\n> > decided to switch affected_refnames to duplicate its strings.\n> >\n> > So given that your solution is only a constant-time factor worse in\n> > efficiency, we should probably prefer it as the more maintainable\n> > option.\n> \n> This is clever, but I don't like that it requires outside code to\n> change internal `string_list` structures in a way that is not\n> documented to be OK.\n> \n> If we cared about getting rid of the extra `O(lg N)` search (and I\n> agree with you that it doesn't matter in this case), I think the clean\n> way to do it would be for `string_list` to expose a method like\n> \n> struct string_list_item *string_list_insert_at_index(struct\n> string_list *list, size_t index, const char *string);\n> \n> and to use it, together with `string_list_find_insert_index()`, to\n> avoid having to search twice.\n\nYes, agreed on all counts.\n\n-Peff\n"},{"id":"327544","messageId":"xmqq60cxcvjk.fsf@gitster.mtv.corp.google.com","threadId":"46668","inReplyTo":"CAN0heSqa8OnPnkd1xbyZ=QN9qg_8OaxBYnwzOZDDA3g+uGE71g@mail.gmail.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-05T10:02:55Z","receivedAt":"2017-09-05T10:03:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> On 30 August 2017 at 04:52, Michael Haggerty wrote:\n>> v3 looks good to me. Thanks!\n>>\n>> Reviewed-by: Michael Haggerty \n>\n> Thank _you_ for very helpful feedback on the earlier versions.\n>\n> Martin\n\nYes, the earlier attempt was sort-of barking up a wrong tree. \n\nOnce we step back and observe other users of affected_refnames and\nrealize that the list is meant to to point at a refname field of an\nexisting instance of another structure by borrowing, the blame\nshifts from files_transaction_prepare() to the real culprit.\nMichael's review gave us a very good \"let's step back a bit\" that\nmade the huge improvement between v1 and v2/v3.\n\nI wonder if we should be tightening the use of affected_refnames\neven further, though. \n\nIt is may be sufficient to make sure that we do not throw anything\nthat we would need to free as part of destroying affected_refnames\ninto the string list, purely from the \"leak prevention\" perspective.\n\nBut stepping back a bit, the reason why the string list exists in\nthe first place is to make sure we do not touch the same ref twice\nin a single transaction, the set of all possible updates in the\nsingle transaction exists somewhere, and each of these updates know\nwhat ref it wants to update. \n\nAnd that is recorded in transaction->updates[]->refname no?\n\nSo it seems to me that logically any and all string that is\nregistered in affected_refnames list must be the refname field of\na ref_update structure in the transaction.\n\nAnd from that point of view, doesn't split_head_update() wants a\nsimilar fix? It attempts to insert \"HEAD\", makes sure it hasn't\nbeen inserted and then hangs a new update transaction as its util.\nIt is not wrong per-se from purely leak-prevention point of view,\nas that \"HEAD\" is a literal string we woudn't even want to free,\nbut from logical/\"what each data means\" point of view, it still\nfeels wrong.\n\n\n"},{"id":"327592","messageId":"CAN0heSqnrPUEgP-BgvuHuVrDG2ifuHHDOPPmxiXJ73u4-PrOng@mail.gmail.com","threadId":"46668","inReplyTo":"xmqq60cxcvjk.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-05T17:24:18Z","receivedAt":"2017-09-05T17:24:27Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 5 September 2017 at 12:02, Junio C Hamano wrote:\n> Martin Ågren writes:\n>\n>> On 30 August 2017 at 04:52, Michael Haggerty wrote:\n>>> v3 looks good to me. Thanks!\n>>>\n>>> Reviewed-by: Michael Haggerty \n>>\n>> Thank _you_ for very helpful feedback on the earlier versions.\n>>\n>> Martin\n>\n> Yes, the earlier attempt was sort-of barking up a wrong tree.\n>\n> Once we step back and observe other users of affected_refnames and\n> realize that the list is meant to to point at a refname field of an\n> existing instance of another structure by borrowing, the blame\n> shifts from files_transaction_prepare() to the real culprit.\n> Michael's review gave us a very good \"let's step back a bit\" that\n> made the huge improvement between v1 and v2/v3.\n>\n> I wonder if we should be tightening the use of affected_refnames\n> even further, though.\n>\n> It is may be sufficient to make sure that we do not throw anything\n> that we would need to free as part of destroying affected_refnames\n> into the string list, purely from the \"leak prevention\" perspective.\n>\n> But stepping back a bit, the reason why the string list exists in\n> the first place is to make sure we do not touch the same ref twice\n> in a single transaction, the set of all possible updates in the\n> single transaction exists somewhere, and each of these updates know\n> what ref it wants to update.\n>\n> And that is recorded in transaction->updates[]->refname no?\n>\n> So it seems to me that logically any and all string that is\n> registered in affected_refnames list must be the refname field of\n> a ref_update structure in the transaction.\n\nI'm with you up to here.\n\n> And from that point of view, doesn't split_head_update() wants a\n> similar fix? It attempts to insert \"HEAD\", makes sure it hasn't\n> been inserted and then hangs a new update transaction as its util.\n> It is not wrong per-se from purely leak-prevention point of view,\n> as that \"HEAD\" is a literal string we woudn't even want to free,\n> but from logical/\"what each data means\" point of view, it still\n> feels wrong.\n\nThere is a \"Special hack\" comment related to this, and I don't feel\nparticularly confident that I could make any meaningful contribution in\nthis area. To be honest, I don't immediately see in which direction your\nsuggestion/idea/thought is going, which tells me I should not be making\na mess out of it. :-)\n\nMartin\n"},{"id":"327593","messageId":"CAN0heSpu-Gr6JLuhBX=m0MbGNHHznrnWHYg+ySCpEcC9ht0_FA@mail.gmail.com","threadId":"46668","inReplyTo":"20170905084751.c4rbeuxx5rdqwdgo@sigill.intra.peff.net","subject":"Re: [PATCH v3 2/3] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-05T17:28:32Z","receivedAt":"2017-09-05T17:28:41Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 5 September 2017 at 10:47, Jeff King wrote:\n> On Tue, Aug 29, 2017 at 07:18:23PM +0200, Martin Ågren wrote:\n>\n>> After the previous patch, none of the functions we call hold on to\n>> `referent.buf`, so we can safely release the string buffer before\n>> returning.\n>\n> I ended up doing this a little differently in my version:\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index 9266f5ab9d..1d16c1b33e 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2287,9 +2292,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n> * the transaction, so we have to read it here\n> * to record and possibly check old_sha1:\n> */\n> - if (refs_read_ref_full(&refs->base,\n> - referent.buf, 0,\n> - lock->old_oid.hash, NULL)) {\n> + ret = refs_read_ref_full(&refs->base,\n> + referent.buf, 0,\n> + lock->old_oid.hash, NULL);\n> + strbuf_release(&referent);\n> +\n> + if (ret) {\n> if (update->flags & REF_HAVE_OLD) {\n> strbuf_addf(err, \"cannot lock ref '%s': \"\n> \"error reading reference\",\n> @@ -2310,6 +2318,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n> ret = split_symref_update(refs, update,\n> referent.buf, transaction,\n> affected_refnames, err);\n> + strbuf_release(&referent);\n> if (ret)\n> return ret;\n> }\n>\n> After we look at referent.buf once in each of the branch arms, we don't\n> need it at all. Disposing of it there means we don't have to worry about\n> it in all of the later early-returns.\n>\n> I'm assuming that referent will always be empty unless REF_ISSYMREF is\n> set. Which seems reasonable, but I didn't double check.\n\nSome time after I posted v3, I had the same thought. I did double-check\nand it does hold. But then I thought that even if it holds now, maybe\nit's a bit too brittle. On the other hand, my patch is quite noisy and\nmaybe the connection between the two is obvious enough that it will hold\nin the future as well.\n\n> Food for thought. I'd be OK with either version.\n\nSo would I...\n\nMartin\n"},{"id":"327601","messageId":"20170905203622.6fs3hr7zfa7mwpqn@sigill.intra.peff.net","threadId":"46668","inReplyTo":"CAN0heSqnrPUEgP-BgvuHuVrDG2ifuHHDOPPmxiXJ73u4-PrOng@mail.gmail.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-05T20:36:23Z","receivedAt":"2017-09-05T20:36:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Sep 05, 2017 at 07:24:18PM +0200, Martin Ågren wrote:\n\n> > And from that point of view, doesn't split_head_update() wants a\n> > similar fix? It attempts to insert \"HEAD\", makes sure it hasn't\n> > been inserted and then hangs a new update transaction as its util.\n> > It is not wrong per-se from purely leak-prevention point of view,\n> > as that \"HEAD\" is a literal string we woudn't even want to free,\n> > but from logical/\"what each data means\" point of view, it still\n> > feels wrong.\n> \n> There is a \"Special hack\" comment related to this, and I don't feel\n> particularly confident that I could make any meaningful contribution in\n> this area. To be honest, I don't immediately see in which direction your\n> suggestion/idea/thought is going, which tells me I should not be making\n> a mess out of it. :-)\n\nI noticed the HEAD funniness, too, when looking at this earlier. I agree\nwith Junio that it's not quite consistent with the general rule of\n\"string list items point to their refnames\", but I don't think it\nmatters in practice.\n\nI think the fix, if we wanted to do one, would be similar to what you\ndid in split_symref_update(). Like:\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex f3455609d6..3f9deff902 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2095,8 +2095,7 @@ static int split_head_update(struct ref_update *update,\n \t * transaction. This insertion is O(N) in the transaction\n \t * size, but it happens at most once per transaction.\n \t */\n-\titem = string_list_insert(affected_refnames, \"HEAD\");\n-\tif (item->util) {\n+\tif (string_list_has_string(affected_refnames, \"HEAD\")) {\n \t\t/* An entry already existed */\n \t\tstrbuf_addf(err,\n \t\t\t \"multiple updates for 'HEAD' (including one \"\n@@ -2111,6 +2110,7 @@ static int split_head_update(struct ref_update *update,\n \t\t\tupdate->new_oid.hash, update->old_oid.hash,\n \t\t\tupdate->msg);\n \n+\titem = string_list_insert(affected_refnames, new_update->refname);\n \titem->util = new_update;\n \n \treturn 0;\n\n-Peff\n"},{"id":"327607","messageId":"xmqqmv68bzvj.fsf@gitster.mtv.corp.google.com","threadId":"46668","inReplyTo":"20170905203622.6fs3hr7zfa7mwpqn@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-05T21:26:56Z","receivedAt":"2017-09-05T21:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I noticed the HEAD funniness, too, when looking at this earlier. I agree\n> with Junio that it's not quite consistent with the general rule of\n> \"string list items point to their refnames\", but I don't think it\n> matters in practice.\n\nYup, we are on the same page; the \"fix\" I was alluding to would look\nexactly like what you wrote below, but I agree the distinction does\nnot matter in practice. IOW, I do not think the code after Martin's\nfix is wrong per-se.\n\nThanks.\n\n> I think the fix, if we wanted to do one, would be similar to what you\n> did in split_symref_update(). Like:\n>\n> diff --git a/refs/files-backend.c b/refs/files-backend.c\n> index f3455609d6..3f9deff902 100644\n> --- a/refs/files-backend.c\n> +++ b/refs/files-backend.c\n> @@ -2095,8 +2095,7 @@ static int split_head_update(struct ref_update *update,\n> \t * transaction. This insertion is O(N) in the transaction\n> \t * size, but it happens at most once per transaction.\n> \t */\n> -\titem = string_list_insert(affected_refnames, \"HEAD\");\n> -\tif (item->util) {\n> +\tif (string_list_has_string(affected_refnames, \"HEAD\")) {\n> \t\t/* An entry already existed */\n> \t\tstrbuf_addf(err,\n> \t\t\t \"multiple updates for 'HEAD' (including one \"\n> @@ -2111,6 +2110,7 @@ static int split_head_update(struct ref_update *update,\n> \t\t\tupdate->new_oid.hash, update->old_oid.hash,\n> \t\t\tupdate->msg);\n> \n> +\titem = string_list_insert(affected_refnames, new_update->refname);\n> \titem->util = new_update;\n> \n> \treturn 0;\n>\n> -Peff\n"},{"id":"327662","messageId":"CAN0heSr0AWj-QwchX2DPzkZs9cx1gsfUTevN4JD1Ze=2t-YZiQ@mail.gmail.com","threadId":"46668","inReplyTo":"xmqqmv68bzvj.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-06T18:12:24Z","receivedAt":"2017-09-06T18:12:30Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"On 5 September 2017 at 23:26, Junio C Hamano wrote:\n> Jeff King writes:\n>\n>> I noticed the HEAD funniness, too, when looking at this earlier. I agree\n>> with Junio that it's not quite consistent with the general rule of\n>> \"string list items point to their refnames\", but I don't think it\n>> matters in practice.\n>\n> Yup, we are on the same page; the \"fix\" I was alluding to would look\n> exactly like what you wrote below, but I agree the distinction does\n> not matter in practice. IOW, I do not think the code after Martin's\n> fix is wrong per-se.\n\nWell, \"not wrong per-se\" tells me you'd feel slightly more comfortable\nabout the state of things if we did this. ;)\n\nI'll take Peff's hint, tweak/add comments for correctness and symmetry\nwith the previous patch and add an if-BUG for symmetry. Peff: Do I have\nyour sign-off? (Do I need it?)\n\nJunio: The topic is in pu as ma/split-symref-update-fix. Re-roll or new\ntopic or as a separate \"patch 4/3\" for you to place on top, any\npreference?\n\nIf we re-roll, would you prefer Peff's much smaller take on patch 2\n(strbuf_release where it matters, instead of sprinkling \"goto out\" all\nover)? I think me and him agreed that we'd be fine either way. I'd reuse\nmy commit message, if I get his sign-off and \"From:\".\n\nObviously, if Michael or anyone else has any input, I'm open to that as\nwell..\n\nMartin\n"},{"id":"327669","messageId":"xmqqtw0f4nbt.fsf@gitster.mtv.corp.google.com","threadId":"46668","inReplyTo":"CAN0heSr0AWj-QwchX2DPzkZs9cx1gsfUTevN4JD1Ze=2t-YZiQ@mail.gmail.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-06T19:52:06Z","receivedAt":"2017-09-06T19:52:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Martin Ågren writes:\n\n> Junio: The topic is in pu as ma/split-symref-update-fix. Re-roll or new\n> topic or as a separate \"patch 4/3\" for you to place on top, any\n> preference?\n\nUntil a topic hits 'next', you solely own it and it is mostly up to\nyou how to go about improving it. My preference would be much less\nrelevant than what the end result would require, i.e....\n\n> If we re-roll, would you prefer Peff's much smaller take on patch 2\n> (strbuf_release where it matters, instead of sprinkling \"goto out\" all\n> over)? I think me and him agreed that we'd be fine either way. I'd reuse\n> my commit message, if I get his sign-off and \"From:\".\n\n... if we take Peff's approach, then rerolling the whole thing would\nbe the only approach that makes sense---incremental update would\nmake the resulting history less readable. Between two approaches I\ndo not have a strong preference either way---it's a choice that can\nbe made between you and Peff.\n\n> Obviously, if Michael or anyone else has any input, I'm open to that as\n> well..\n\nSure.\n"},{"id":"327682","messageId":"20170906234543.2lnja2lds62z7fog@sigill.intra.peff.net","threadId":"46668","inReplyTo":"CAN0heSr0AWj-QwchX2DPzkZs9cx1gsfUTevN4JD1Ze=2t-YZiQ@mail.gmail.com","subject":"Re: [PATCH v3 1/3] refs/files-backend: add longer-scoped copy of string to list","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-06T23:45:43Z","receivedAt":"2017-09-06T23:45:53Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Sep 06, 2017 at 08:12:24PM +0200, Martin Ågren wrote:\n\n> On 5 September 2017 at 23:26, Junio C Hamano wrote:\n> > Jeff King writes:\n> >\n> >> I noticed the HEAD funniness, too, when looking at this earlier. I agree\n> >> with Junio that it's not quite consistent with the general rule of\n> >> \"string list items point to their refnames\", but I don't think it\n> >> matters in practice.\n> >\n> > Yup, we are on the same page; the \"fix\" I was alluding to would look\n> > exactly like what you wrote below, but I agree the distinction does\n> > not matter in practice. IOW, I do not think the code after Martin's\n> > fix is wrong per-se.\n> \n> Well, \"not wrong per-se\" tells me you'd feel slightly more comfortable\n> about the state of things if we did this. ;)\n> \n> I'll take Peff's hint, tweak/add comments for correctness and symmetry\n> with the previous patch and add an if-BUG for symmetry. Peff: Do I have\n> your sign-off? (Do I need it?)\n\nYes, you have my sign-off. Probably it is not necessary for such a\ntrivial patch, but it never hurts to be sure.\n\n> If we re-roll, would you prefer Peff's much smaller take on patch 2\n> (strbuf_release where it matters, instead of sprinkling \"goto out\" all\n> over)? I think me and him agreed that we'd be fine either way. I'd reuse\n> my commit message, if I get his sign-off and \"From:\".\n\nYou are welcome to forge my sign-off there (but I really am OK with\neither approach).\n\n-Peff\n"},{"id":"327808","messageId":"cover.1504939825.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"CAN0heSr0AWj-QwchX2DPzkZs9cx1gsfUTevN4JD1Ze=2t-YZiQ@mail.gmail.com","subject":"[PATCH v4 0/4] Rerolling ma/split-symref-update-fix","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-09T06:57:14Z","receivedAt":"2017-09-09T06:57:42Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"> I'll take Peff's hint, tweak/add comments for correctness and symmetry\n> with the previous patch and add an if-BUG for symmetry.\n\nHere's a reroll of ma/split-symref-update-fix. The first three patches\nare v3 plus Michael's Reviewed-By.\n\nThe fourth is the conceptual fix of adding `refname` instead of \"HEAD\"\ninto the list of affected refnames.\n\nThanks all for comments, suggestions and help along the way.\n\nMartin\n\nMartin Ågren (4):\n refs/files-backend: add longer-scoped copy of string to list\n refs/files-backend: fix memory leak in lock_ref_for_update\n refs/files-backend: correct return value in lock_ref_for_update\n refs/files-backend: add `refname`, not \"HEAD\", to list\n\n refs/files-backend.c | 62 +++++++++++++++++++++++++++++++++++++---------------\n 1 file changed, 44 insertions(+), 18 deletions(-)\n\n-- \n2.14.1.460.g848a19d64\n\n"},{"id":"327809","messageId":"95e69cb0f7b80d78bc38d79f276204ff414685cf.1504939825.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"cover.1504939825.git.martin.agren@gmail.com","subject":"[PATCH v4 1/4] refs/files-backend: add longer-scoped copy of string to list","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-09T06:57:15Z","receivedAt":"2017-09-09T06:57:47Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"split_symref_update() receives a string-pointer `referent` and adds it\nto the list of `affected_refnames`. The list simply holds on to the\npointers it is given, it does not copy the strings and it does not ever\nfree them. The `referent` string in split_symref_update() belongs to a\nstring buffer in the caller. After we return, the string will be leaked.\n\nIn the next patch, we want to properly release the string buffer in the\ncaller, but we can't safely do so until we've made sure that\n`affected_refnames` will not be holding on to a pointer to the string.\nWe could configure the list to handle its own resources, but it would\nmean some alloc/free-churning. The list is already handling other\nstrings (through other code paths) which we do not need to worry about,\nand we'd be memory-churning those strings too, completely unnecessary.\n\nObserve that split_symref_update() creates a `new_update`-object through\nref_transaction_add_update(), after which `new_update->refname` is a\ncopy of `referent`. The difference is, this copy will be freed, and it\nwill be freed *after* `affected_refnames` has been cleared.\n\nRearrange the handling of `referent`, so that we don't add it directly\nto `affected_refnames`. Instead, first just check whether `referent`\nexists in the string list, and later add `new_update->refname`.\n\nHelped-by: Michael Haggerty \nReviewed-by: Michael Haggerty \nSigned-off-by: Martin Ågren \n---\n refs/files-backend.c | 18 ++++++++++++++----\n 1 file changed, 14 insertions(+), 4 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 0404f2c23..baceef3b3 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2634,13 +2634,12 @@ static int split_symref_update(struct files_ref_store *refs,\n \n \t/*\n \t * First make sure that referent is not already in the\n-\t * transaction. This insertion is O(N) in the transaction\n+\t * transaction. This check is O(lg N) in the transaction\n \t * size, but it happens at most once per symref in a\n \t * transaction.\n \t */\n-\titem = string_list_insert(affected_refnames, referent);\n-\tif (item->util) {\n-\t\t/* An entry already existed */\n+\tif (string_list_has_string(affected_refnames, referent)) {\n+\t\t/* An entry already exists */\n \t\tstrbuf_addf(err,\n \t\t\t \"multiple updates for '%s' (including one \"\n \t\t\t \"via symref '%s') are not allowed\",\n@@ -2675,6 +2674,17 @@ static int split_symref_update(struct files_ref_store *refs,\n \tupdate->flags |= REF_LOG_ONLY | REF_NODEREF;\n \tupdate->flags &= ~REF_HAVE_OLD;\n \n+\t/*\n+\t * Add the referent. This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per symref in a\n+\t * transaction. Make sure to add new_update->refname, which will\n+\t * be valid as long as affected_refnames is in use, and NOT\n+\t * referent, which might soon be freed by our caller.\n+\t */\n+\titem = string_list_insert(affected_refnames, new_update->refname);\n+\tif (item->util)\n+\t\tBUG(\"%s unexpectedly found in affected_refnames\",\n+\t\t new_update->refname);\n \titem->util = new_update;\n \n \treturn 0;\n-- \n2.14.1.460.g848a19d64\n\n"},{"id":"327810","messageId":"f06c7cab8e335f5459388d20776c19d5384ebc29.1504939825.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"cover.1504939825.git.martin.agren@gmail.com","subject":"[PATCH v4 2/4] refs/files-backend: fix memory leak in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-09T06:57:16Z","receivedAt":"2017-09-09T06:57:50Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"After the previous patch, none of the functions we call hold on to\n`referent.buf`, so we can safely release the string buffer before\nreturning.\n\nReviewed-by: Michael Haggerty \nSigned-off-by: Martin Ågren \n---\n refs/files-backend.c | 31 ++++++++++++++++++++-----------\n 1 file changed, 20 insertions(+), 11 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex baceef3b3..3d6363966 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2756,7 +2756,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \tstruct strbuf referent = STRBUF_INIT;\n \tint mustexist = (update->flags & REF_HAVE_OLD) &&\n \t\t!is_null_oid(&update->old_oid);\n-\tint ret;\n+\tint ret = 0;\n \tstruct ref_lock *lock;\n \n \tfiles_assert_main_repository(refs, \"lock_ref_for_update\");\n@@ -2768,7 +2768,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tret = split_head_update(update, transaction, head_ref,\n \t\t\t\t\taffected_refnames, err);\n \t\tif (ret)\n-\t\t\treturn ret;\n+\t\t\tgoto out;\n \t}\n \n \tret = lock_raw_ref(refs, update->refname, mustexist,\n@@ -2782,7 +2782,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tstrbuf_addf(err, \"cannot lock ref '%s': %s\",\n \t\t\t original_update_refname(update), reason);\n \t\tfree(reason);\n-\t\treturn ret;\n+\t\tgoto out;\n \t}\n \n \tupdate->backend_data = lock;\n@@ -2801,10 +2801,12 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\n \t\t\t\t\t\t original_update_refname(update));\n-\t\t\t\t\treturn -1;\n+\t\t\t\t\tret = -1;\n+\t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t} else if (check_old_oid(update, &lock->old_oid, err)) {\n-\t\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\t\tgoto out;\n \t\t\t}\n \t\t} else {\n \t\t\t/*\n@@ -2818,13 +2820,15 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\t\t referent.buf, transaction,\n \t\t\t\t\t\t affected_refnames, err);\n \t\t\tif (ret)\n-\t\t\t\treturn ret;\n+\t\t\t\tgoto out;\n \t\t}\n \t} else {\n \t\tstruct ref_update *parent_update;\n \n-\t\tif (check_old_oid(update, &lock->old_oid, err))\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\tif (check_old_oid(update, &lock->old_oid, err)) {\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n+\t\t}\n \n \t\t/*\n \t\t * If this update is happening indirectly because of a\n@@ -2861,7 +2865,8 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t \"cannot update ref '%s': %s\",\n \t\t\t\t update->refname, write_err);\n \t\t\tfree(write_err);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t} else {\n \t\t\tupdate->flags |= REF_NEEDS_COMMIT;\n \t\t}\n@@ -2875,10 +2880,14 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\tif (close_ref(lock)) {\n \t\t\tstrbuf_addf(err, \"couldn't close '%s.lock'\",\n \t\t\t\t update->refname);\n-\t\t\treturn TRANSACTION_GENERIC_ERROR;\n+\t\t\tret = TRANSACTION_GENERIC_ERROR;\n+\t\t\tgoto out;\n \t\t}\n \t}\n-\treturn 0;\n+\n+out:\n+\tstrbuf_release(&referent);\n+\treturn ret;\n }\n \n /*\n-- \n2.14.1.460.g848a19d64\n\n"},{"id":"327811","messageId":"3b2adfcb53d1564f7e794ec5cfbbb93c37c9f3a5.1504939825.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"cover.1504939825.git.martin.agren@gmail.com","subject":"[PATCH v4 3/4] refs/files-backend: correct return value in lock_ref_for_update","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-09T06:57:17Z","receivedAt":"2017-09-09T06:57:54Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"In one code path we return a literal -1 and not a symbolic constant. The\nvalue -1 would be interpreted as TRANSACTION_NAME_CONFLICT, which is\nwrong. Use TRANSACTION_GENERIC_ERROR instead (that is the only other\nreturn value we have to choose from).\n\nNoticed-by: Michael Haggerty \nReviewed-by: Michael Haggerty \nSigned-off-by: Martin Ågren \n---\n refs/files-backend.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 3d6363966..03df00275 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2801,7 +2801,7 @@ static int lock_ref_for_update(struct files_ref_store *refs,\n \t\t\t\t\tstrbuf_addf(err, \"cannot lock ref '%s': \"\n \t\t\t\t\t\t \"error reading reference\",\n \t\t\t\t\t\t original_update_refname(update));\n-\t\t\t\t\tret = -1;\n+\t\t\t\t\tret = TRANSACTION_GENERIC_ERROR;\n \t\t\t\t\tgoto out;\n \t\t\t\t}\n \t\t\t} else if (check_old_oid(update, &lock->old_oid, err)) {\n-- \n2.14.1.460.g848a19d64\n\n"},{"id":"327812","messageId":"6a050265f2439217c7cf6107149f7d6ef30f64a3.1504939825.git.martin.agren@gmail.com","threadId":"46668","inReplyTo":"cover.1504939825.git.martin.agren@gmail.com","subject":"[PATCH v4 4/4] refs/files-backend: add `refname`, not \"HEAD\", to list","fromName":"Martin Ågren","fromEmail":"martin.agren@gmail.com","sentAt":"2017-09-09T06:57:18Z","receivedAt":"2017-09-09T06:57:58Z","isPatch":true,"sender":{"key":"martin.agren@gmail.com","avatar":null},"body":"An earlier patch rewrote `split_symref_update()` to add a copy of a\nstring to a string list instead of adding the original string. That was\nso that the original string could be freed in a later patch, but it is\nalso conceptually cleaner, since now all calls to `string_list_insert()`\nand `string_list_append()` add `update->refname`. --- Except a literal\n\"HEAD\" is added in `split_head_update()`.\n\nRestructure `split_head_update()` in the same way as the earlier patch\ndid for `split_symref_update()`. This does not correct any practical\nproblem, but makes things conceptually cleaner. The downside is a call\nto `string_list_has_string()`, which should be relatively cheap.\n\nSigned-off-by: Jeff King \nSigned-off-by: Martin Ågren \n---\n refs/files-backend.c | 13 ++++++++++---\n 1 file changed, 10 insertions(+), 3 deletions(-)\n\ndiff --git a/refs/files-backend.c b/refs/files-backend.c\nindex 03df00275..926f87b94 100644\n--- a/refs/files-backend.c\n+++ b/refs/files-backend.c\n@@ -2589,11 +2589,10 @@ static int split_head_update(struct ref_update *update,\n \n \t/*\n \t * First make sure that HEAD is not already in the\n-\t * transaction. This insertion is O(N) in the transaction\n+\t * transaction. This check is O(lg N) in the transaction\n \t * size, but it happens at most once per transaction.\n \t */\n-\titem = string_list_insert(affected_refnames, \"HEAD\");\n-\tif (item->util) {\n+\tif (string_list_has_string(affected_refnames, \"HEAD\")) {\n \t\t/* An entry already existed */\n \t\tstrbuf_addf(err,\n \t\t\t \"multiple updates for 'HEAD' (including one \"\n@@ -2608,6 +2607,14 @@ static int split_head_update(struct ref_update *update,\n \t\t\tupdate->new_oid.hash, update->old_oid.hash,\n \t\t\tupdate->msg);\n \n+\t/*\n+\t * Add \"HEAD\". This insertion is O(N) in the transaction\n+\t * size, but it happens at most once per transaction.\n+\t * Add new_update->refname instead of a literal \"HEAD\".\n+\t */\n+\tif (strcmp(new_update->refname, \"HEAD\"))\n+\t\tBUG(\"%s unexpectedly not 'HEAD'\", new_update->refname);\n+\titem = string_list_insert(affected_refnames, new_update->refname);\n \titem->util = new_update;\n \n \treturn 0;\n-- \n2.14.1.460.g848a19d64\n\n"},{"id":"327814","messageId":"20170909104708.hjtn2yi2ma3clf2v@sigill.intra.peff.net","threadId":"46668","inReplyTo":"cover.1504939825.git.martin.agren@gmail.com","subject":"Re: [PATCH v4 0/4] Rerolling ma/split-symref-update-fix","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-09T10:47:08Z","receivedAt":"2017-09-09T10:47:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 09, 2017 at 08:57:14AM +0200, Martin Ågren wrote:\n\n> > I'll take Peff's hint, tweak/add comments for correctness and symmetry\n> > with the previous patch and add an if-BUG for symmetry.\n> \n> Here's a reroll of ma/split-symref-update-fix. The first three patches\n> are v3 plus Michael's Reviewed-By.\n> \n> The fourth is the conceptual fix of adding `refname` instead of \"HEAD\"\n> into the list of affected refnames.\n> \n> Thanks all for comments, suggestions and help along the way.\n\nThis looks good to me, including the explanation in the new patch.\nThanks!\n\n-Peff\n"}]}