{"thread":{"id":"39142","subject":"[PATCHv3] refs.c: enable large transactions","startedAt":"2015-04-22T21:30:27Z","lastAt":"2015-04-25T05:24:27Z","messageCount":12,"participants":["Stefan Beller","Junio C Hamano","Jeff King","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"259850","messageId":"1429738227-2985-1-git-send-email-sbeller@google.com","threadId":"39142","inReplyTo":null,"subject":"[PATCHv3] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-22T21:30:27Z","receivedAt":"2015-04-22T21:30:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This is another attempt on enabling large transactions\n(large in terms of open file descriptors). We keep track of how many\nlock files are opened by the ref_transaction_commit function.\nWhen more than a reasonable amount of files is open, we close\nthe file descriptors to make sure the transaction can continue.\n\nAnother idea I had during implementing this was to move this file\nclosing into the lock file API, such that only a certain amount of\nlock files can be open at any given point in time and we'd be 'garbage\ncollecting' open fds when necessary in any relevant call to the lock\nfile API. This would have brought the advantage of having such\nfunctionality available in other users of the lock file API as well.\nThe downside however is the over complication, you really need to always\ncheck for (lock->fd != -1) all the time, which may slow down other parts\nof the code, which did not ask for such a feature.\n\nSigned-off-by: Stefan Beller \n---\n\nNotes:\n * Added error checking when reopening the lock\n \n * Only call close_lock_file when needed.\n (This makes the code a bit harder to read, but\n might be worth it nevertheless. close_lock_file would\n return early anyway, so we're trading off a function call\n to some additional check (!(flags & REF_HAVE_NEW) ||is_null_sha1(update->new_sha1))\n \n * tuned the number of spare fd to be 25 as in the other occurence.\n At least we want to be consistent with our made up ballpark numbers.\n \n * This replaces the latest patch on origin/sb/remove-fd-from-ref-lock\n\n refs.c | 28 ++++++++++++++++++++++++++++\n t/t1400-update-ref.sh | 4 ++--\n 2 files changed, 30 insertions(+), 2 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 4f495bd..7ce7b97 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -3041,6 +3041,13 @@ static int write_ref_sha1(struct ref_lock *lock,\n \t\terrno = EINVAL;\n \t\treturn -1;\n \t}\n+\tif (lock->lk->fd == -1 && reopen_lock_file(lock->lk) == -1) {\n+\t\tint save_errno = errno;\n+\t\terror(\"Couldn't reopen %s\", lock->lk->filename.buf);\n+\t\tunlock_ref(lock);\n+\t\terrno = save_errno;\n+\t\treturn -1;\n+\t}\n \tif (write_in_full(lock->lk->fd, sha1_to_hex(sha1), 40) != 40 ||\n \t write_in_full(lock->lk->fd, &term, 1) != 1 ||\n \t close_ref(lock) < 0) {\n@@ -3718,6 +3725,7 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t struct strbuf *err)\n {\n \tint ret = 0, i;\n+\tunsigned int remaining_fds;\n \tint n = transaction->nr;\n \tstruct ref_update **updates = transaction->updates;\n \tstruct string_list refs_to_delete = STRING_LIST_INIT_NODUP;\n@@ -3733,6 +3741,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\treturn 0;\n \t}\n \n+\t/*\n+\t * We need to open many files in a large transaction, so come up with\n+\t * a reasonable maximum. We still keep some spares for stdin/out and\n+\t * other open files. Experiments determined we need more fds when\n+\t * running inside our test suite than directly in the shell. It's\n+\t * unclear where these fds come from. 25 should be a reasonable large\n+\t * number though.\n+\t */\n+\tremaining_fds = get_max_fd_limit();\n+\tif (remaining_fds > 25)\n+\t\tremaining_fds -= 25;\n+\telse\n+\t\tremaining_fds = 0;\n+\n \t/* Copy, sort, and reject duplicate refs */\n \tqsort(updates, n, sizeof(*updates), ref_update_compare);\n \tif (ref_update_reject_duplicates(updates, n, err)) {\n@@ -3762,6 +3784,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n \t\t\t\t update->refname);\n \t\t\tgoto cleanup;\n \t\t}\n+\t\tif (!(flags & REF_HAVE_NEW) ||\n+\t\t is_null_sha1(update->new_sha1) ||\n+\t\t remaining_fds == 0)\n+\t\t\tclose_lock_file(update->lock->lk);\n+\t\telse\n+\t\t\tremaining_fds--;\n \t}\n \n \t/* Perform updates first so live commits remain referenced */\ndiff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh\nindex 7a69f1a..636d3a1 100755\n--- a/t/t1400-update-ref.sh\n+++ b/t/t1400-update-ref.sh\n@@ -1071,7 +1071,7 @@ run_with_limited_open_files () {\n \n test_lazy_prereq ULIMIT_FILE_DESCRIPTORS 'run_with_limited_open_files true'\n \n-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n@@ -1082,7 +1082,7 @@ test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction creating branches\n )\n '\n \n-test_expect_failure ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n+test_expect_success ULIMIT_FILE_DESCRIPTORS 'large transaction deleting branches does not burst open file limit' '\n (\n \tfor i in $(test_seq 33)\n \tdo\n-- \n2.4.0.rc2.5.g4c2045b.dirty\n"},{"id":"259902","messageId":"xmqqzj5y3f0a.fsf@gitster.dls.corp.google.com","threadId":"39142","inReplyTo":"1429738227-2985-1-git-send-email-sbeller@google.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-23T17:56:21Z","receivedAt":"2015-04-23T17:56:21Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> diff --git a/refs.c b/refs.c\n> index 4f495bd..7ce7b97 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -3041,6 +3041,13 @@ static int write_ref_sha1(struct ref_lock *lock,\n> \t\terrno = EINVAL;\n> \t\treturn -1;\n> \t}\n> +\tif (lock->lk->fd == -1 && reopen_lock_file(lock->lk) == -1) {\n\nGranted, we explicitly assign -1 to lk->fd when we know it is\nclosed, and the return value from reopen_lock_file() can come only\nfrom the return value from open(2), which is defined to return -1\n(i.e. not just any negative value) upon failure, but still, isn't it\ncustomary to check with \"< 0\" rather than \"== -1\" for errors?\n\n> +\t\tint save_errno = errno;\n> +\t\terror(\"Couldn't reopen %s\", lock->lk->filename.buf);\n\nNo need to change this line, but I noticed that we might want to do\nsomething about the first one of the following two:\n\n $ git grep -e '[ \t]error(_*\"[A-Z]' | wc -l\n 146\n $ git grep -e '[ \t]error(_*\"[a-z]' | wc -l\n 390\n\n> +\t\tunlock_ref(lock);\n> +\t\terrno = save_errno;\n> +\t\treturn -1;\n> +\t}\n\nIs this the only place in the entire codebase where a lockfile,\nwhich may have been closed to save number of open file descriptors,\nneeds to be reopened? If I understand correctly, lockfile API is\nnot for sole use of refs (don't the index and the pack codepaths use\nit, too?), so it may give us a better abstraction to have a helper\nfunction in lockfile.[ch] that takes a lock object, i.e.\n\n\tint make_lock_fd_valid(struct lock_file *lk)\n {\n\t\tif (lk->fd < 0 && reopen_lock_file(lk) < 0) {\n\t\t\t... error ...\n return -1;\n\t\t}\n return 0;\n\t}\n\nand to call it at this point, i.e.\n\n\tif (make_lock_fd_valid(lock->lk) < 0)\n \treturn -1;\n\nperhaps?\n\n> @@ -3733,6 +3741,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\treturn 0;\n> \t}\n> \n> +\t/*\n> +\t * We need to open many files in a large transaction, so come up with\n> +\t * a reasonable maximum. We still keep some spares for stdin/out and\n> +\t * other open files. Experiments determined we need more fds when\n> +\t * running inside our test suite than directly in the shell. It's\n> +\t * unclear where these fds come from. 25 should be a reasonable large\n> +\t * number though.\n> +\t */\n\n> +\tremaining_fds = get_max_fd_limit();\n> +\tif (remaining_fds > 25)\n> +\t\tremaining_fds -= 25;\n> +\telse\n> +\t\tremaining_fds = 0;\n\nIs the value of pack_open_fds visible from here? I am wondering if\nwe might want \"scratch fds Git can use for its own use\" accounting\nso that the two subsystems can collectively say \"it is still safe\nto use one more fd and leave 25 for other people\". With the code\nstructure proposed here, the pack reader can grab all but 25 fds,\nwhich would leave the rest of Git including this code only 25,\nand the value in remaining_fds would become totally meaningless.\n\nIt certainly can wait to worry about that and we do not have to do\nanything about it in this patch, but it may still be a good idea to\nleave \"NEEDSWORK:\" comment here (and point at open_packed_git_1() in\nsha1_file.c in the comment).\n\nI do not think the other side needs to know about the fd consumption\nin this function, as what is opened in here will be closed before\nthis function returns.\n\n> @@ -3762,6 +3784,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n> \t\t\t\t update->refname);\n> \t\t\tgoto cleanup;\n> \t\t}\n> +\t\tif (!(flags & REF_HAVE_NEW) ||\n> +\t\t is_null_sha1(update->new_sha1) ||\n> +\t\t remaining_fds == 0)\n> +\t\t\tclose_lock_file(update->lock->lk);\n> +\t\telse\n> +\t\t\tremaining_fds--;\n> \t}\n"},{"id":"259921","messageId":"CAGZ79kYO9NifvWQ7nWHP6==ZFmrMj47-94rEHOhWooR5Nh7EUw@mail.gmail.com","threadId":"39142","inReplyTo":"xmqqzj5y3f0a.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-24T00:21:49Z","receivedAt":"2015-04-24T00:21:49Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 23, 2015 at 10:56 AM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> diff --git a/refs.c b/refs.c\n>> index 4f495bd..7ce7b97 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -3041,6 +3041,13 @@ static int write_ref_sha1(struct ref_lock *lock,\n>> errno = EINVAL;\n>> return -1;\n>> }\n>> + if (lock->lk->fd == -1 && reopen_lock_file(lock->lk) == -1) {\n>\n> Granted, we explicitly assign -1 to lk->fd when we know it is\n> closed, and the return value from reopen_lock_file() can come only\n> from the return value from open(2), which is defined to return -1\n> (i.e. not just any negative value) upon failure, but still, isn't it\n> customary to check with \"< 0\" rather than \"== -1\" for errors?\n\n< 0 would be better here indeed.\n\n>\n>> + int save_errno = errno;\n>> + error(\"Couldn't reopen %s\", lock->lk->filename.buf);\n>\n> No need to change this line, but I noticed that we might want to do\n> something about the first one of the following two:\n\nI personally like to have each error(...) to have a unique string, such\nthat when you run into trouble (most likely reported by a user),\nyou can easily pinpoint where the exact error is.\nMaybe we could think about overriding error to actually report\n\"version, filename, linenumber, actual error message\"\n\n>\n> $ git grep -e '[ ]error(_*\"[A-Z]' | wc -l\n> 146\n> $ git grep -e '[ ]error(_*\"[a-z]' | wc -l\n> 390\n>\n>> + unlock_ref(lock);\n>> + errno = save_errno;\n>> + return -1;\n>> + }\n>\n> Is this the only place in the entire codebase where a lockfile,\n> which may have been closed to save number of open file descriptors,\n> needs to be reopened? If I understand correctly, lockfile API is\n> not for sole use of refs (don't the index and the pack codepaths use\n> it, too?), so it may give us a better abstraction to have a helper\n> function in lockfile.[ch] that takes a lock object, i.e.\n>\n> int make_lock_fd_valid(struct lock_file *lk)\n> {\n> if (lk->fd < 0 && reopen_lock_file(lk) < 0) {\n> ... error ...\n> return -1;\n> }\n> return 0;\n> }\n>\n> and to call it at this point, i.e.\n>\n> if (make_lock_fd_valid(lock->lk) < 0)\n> return -1;\n>\n> perhaps?\n\nThis is what I was originally looking for, thanks for the design guidance!\n\n>\n>> @@ -3733,6 +3741,20 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>> return 0;\n>> }\n>>\n>> + /*\n>> + * We need to open many files in a large transaction, so come up with\n>> + * a reasonable maximum. We still keep some spares for stdin/out and\n>> + * other open files. Experiments determined we need more fds when\n>> + * running inside our test suite than directly in the shell. It's\n>> + * unclear where these fds come from. 25 should be a reasonable large\n>> + * number though.\n>> + */\n>\n>> + remaining_fds = get_max_fd_limit();\n>> + if (remaining_fds > 25)\n>> + remaining_fds -= 25;\n>> + else\n>> + remaining_fds = 0;\n>\n> Is the value of pack_open_fds visible from here? I am wondering if\n> we might want \"scratch fds Git can use for its own use\" accounting\n> so that the two subsystems can collectively say \"it is still safe\n> to use one more fd and leave 25 for other people\". With the code\n> structure proposed here, the pack reader can grab all but 25 fds,\n> which would leave the rest of Git including this code only 25,\n> and the value in remaining_fds would become totally meaningless.\n>\n> It certainly can wait to worry about that and we do not have to do\n> anything about it in this patch, but it may still be a good idea to\n> leave \"NEEDSWORK:\" comment here (and point at open_packed_git_1() in\n> sha1_file.c in the comment).\n>\n> I do not think the other side needs to know about the fd consumption\n> in this function, as what is opened in here will be closed before\n> this function returns.\n>\n>> @@ -3762,6 +3784,12 @@ int ref_transaction_commit(struct ref_transaction *transaction,\n>> update->refname);\n>> goto cleanup;\n>> }\n>> + if (!(flags & REF_HAVE_NEW) ||\n>> + is_null_sha1(update->new_sha1) ||\n>> + remaining_fds == 0)\n>> + close_lock_file(update->lock->lk);\n>> + else\n>> + remaining_fds--;\n>> }\n"},{"id":"259924","messageId":"xmqq8udi2tn8.fsf@gitster.dls.corp.google.com","threadId":"39142","inReplyTo":"CAGZ79kYO9NifvWQ7nWHP6==ZFmrMj47-94rEHOhWooR5Nh7EUw@mail.gmail.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-24T01:37:47Z","receivedAt":"2015-04-24T01:37:47Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> On Thu, Apr 23, 2015 at 10:56 AM, Junio C Hamano wrote:\n>\n>>> + int save_errno = errno;\n>>> + error(\"Couldn't reopen %s\", lock->lk->filename.buf);\n>>\n>> No need to change this line, but I noticed that we might want to do\n>> something about the first one of the following two:\n>\n> I personally like to have each error(...) to have a unique string, such\n> that when you run into trouble (most likely reported by a user),\n> you can easily pinpoint where the exact error is.\n\nI was hoping that the \"grep\" patterns were strong enough hint, but\nlet me be explicit. I was commenting on \"Could\" not being spelled\nas \"could\".\n\n>> $ git grep -e '[ ]error(_*\"[A-Z]' | wc -l\n>> 146\n>> $ git grep -e '[ ]error(_*\"[a-z]' | wc -l\n>> 390\n"},{"id":"259949","messageId":"CAGZ79kYGDOUgzKmQOLAXkYYb-HZJCw3Y=iSLXWHVXcQ0pAiDBw@mail.gmail.com","threadId":"39142","inReplyTo":"xmqq8udi2tn8.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-24T16:16:28Z","receivedAt":"2015-04-24T16:16:28Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 23, 2015 at 6:37 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> On Thu, Apr 23, 2015 at 10:56 AM, Junio C Hamano wrote:\n>>\n>>>> + int save_errno = errno;\n>>>> + error(\"Couldn't reopen %s\", lock->lk->filename.buf);\n>>>\n>>> No need to change this line, but I noticed that we might want to do\n>>> something about the first one of the following two:\n>>\n>> I personally like to have each error(...) to have a unique string, such\n>> that when you run into trouble (most likely reported by a user),\n>> you can easily pinpoint where the exact error is.\n>\n> I was hoping that the \"grep\" patterns were strong enough hint, but\n> let me be explicit. I was commenting on \"Could\" not being spelled\n> as \"could\".\n>\n>>> $ git grep -e '[ ]error(_*\"[A-Z]' | wc -l\n>>> 146\n>>> $ git grep -e '[ ]error(_*\"[a-z]' | wc -l\n>>> 390\n\nI understood that you were talking about Could being capitalized.\nThough it's distributed 30/70 which hints to me we do not care in\nenough to explain the capitalized letters as slip-throughs on review\nor such, but it looks like the authors choice to me.\n\nI think it's a mistake to s/Could/could/g for all errors in the code base\nas it reduces the information provided in the error messages.\nJust 3 days ago (\"Regular Rebase Failure\"). I used different\ncapitalization to get a better understanding where the error may be.\n\nSo if we throw away that information, we should add new information\nto make the spot of the error easily findable in the source.\nThat's why I proposed the idea of the version,filename,linenumber\nas that is one of the strongest signals (most information in a short\namount of text) I can imagine.\n"},{"id":"259951","messageId":"20150424171953.GA30592@peff.net","threadId":"39142","inReplyTo":"CAGZ79kYGDOUgzKmQOLAXkYYb-HZJCw3Y=iSLXWHVXcQ0pAiDBw@mail.gmail.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-24T17:19:54Z","receivedAt":"2015-04-24T17:19:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2015 at 09:16:28AM -0700, Stefan Beller wrote:\n\n> I think it's a mistake to s/Could/could/g for all errors in the code base\n> as it reduces the information provided in the error messages.\n> Just 3 days ago (\"Regular Rebase Failure\"). I used different\n> capitalization to get a better understanding where the error may be.\n> \n> So if we throw away that information, we should add new information\n> to make the spot of the error easily findable in the source.\n> That's why I proposed the idea of the version,filename,linenumber\n> as that is one of the strongest signals (most information in a short\n> amount of text) I can imagine.\n\nI do like that idea, and I think you could base it on the trace_printf\nimplementation. Note that it requires variadic macros, but I think\nthat's OK. Just like trace_printf, we can do the macro implementation\nwhen we support that feature, and people on older systems just won't get\nthe extra file/line data.\n\nI also assume we would not show this information by default, but only\nwith GIT_TRACE_ERRORS or something like that.\n\nI would love it if we could also get a stack trace for warnings and\nerrors. Very often the line number of the error() call is not nearly as\ninteresting as the line number of the _caller_. But doing that portably\nis rather hard. Maybe a better option would be to make it easier to\nconvince git to dump core at the right moments (e.g., dump core when we\nhit die() rather than calling exit). And then you can run gdb on the\ncore file, which gives you a backtrace and much more.\n\n-Peff\n"},{"id":"259954","messageId":"20150424181236.GK5467@google.com","threadId":"39142","inReplyTo":"CAGZ79kYGDOUgzKmQOLAXkYYb-HZJCw3Y=iSLXWHVXcQ0pAiDBw@mail.gmail.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2015-04-24T18:12:36Z","receivedAt":"2015-04-24T18:12:36Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> I understood that you were talking about Could being capitalized.\n> Though it's distributed 30/70 which hints to me we do not care in\n> enough to explain the capitalized letters as slip-throughs on review\n> or such, but it looks like the authors choice to me.\n\nLowercase, brief, with no period at the end is the convention. This\nis user-facing UI, so consistency matters. It's not author's choice.\n\nExisting examples of capitalized messages fall into a few categories:\n\n - shell scripts used to tend to use the capitalized form, and when\n they got rewritten as builtins the messages weren't cleaned up at\n the same time\n\n - some patch authors have different habits and it wasn't worth going\n back and forth to fix it (but the convention still stands, and the\n result of a program that can't decide how to talk to the user is\n still harmful)\n\n - once there are a few examples, they get copy/pasted and imitated\n\n> I think it's a mistake to s/Could/could/g for all errors in the code base\n> as it reduces the information provided in the error messages.\n\nThis seems like an after-the-fact justification for a mistake.\n\nOften there is a choice about whether the caller or the callee to a\nfunction prints an error. If the caller does, it can say more about\nthe context. If the callee does, the message is in one place and can\nbe tweaked more easily to be more useful.\n\nTo get the benefits of both, we could print a backtrace with every\nerror. That way, the callee can describe the error in one place but\nthe context is all there. But I'm really glad we don't do that!\n\nThe main purpose of most error messages is instructional: they give\ninformation to the user in a way that is abstracted from the\nimplementation (the user doesn't care what function it happened in!)\nthat tells them what happened and what they can do next.\nGratuitous inconsistency in formatting doesn't help with that.\n\nActually, I wouldn't mind an environment variable that tells error()\nto include a backtrace if someone wants it. (See backtrace(3)\nfor implementation hints if interested in doing that.)\n\n> Just 3 days ago (\"Regular Rebase Failure\"). I used different\n> capitalization to get a better understanding where the error may be.\n\nWouldn't it be better if there weren't so many similar messages\nproduced in different contexts in the first place? (I.e., this\nsuggests to me that we should move more in the direction of\ncallee-produces-error.)\n\nSorry, that was a long way to say very little. I just wanted to\nemphasize that when a UI inconsistency has a useful side effect, while\nthat can be useful to understand and learn from, we should not be\nafraid to fix it. And to clear up any ambiguity about git's error\nmessage convention. :)\n\nThanks and hope that helps,\nJonathan\n"},{"id":"259955","messageId":"CAGZ79kZDq10nDZAgqtYN2J7=LvAdRoWN3rpCEnco97W3637sNw@mail.gmail.com","threadId":"39142","inReplyTo":"20150424181236.GK5467@google.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2015-04-24T18:31:25Z","receivedAt":"2015-04-24T18:31:25Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 24, 2015 at 11:12 AM, Jonathan Nieder wrote:\n> Stefan Beller wrote:\n>\n>> I understood that you were talking about Could being capitalized.\n>> Though it's distributed 30/70 which hints to me we do not care in\n>> enough to explain the capitalized letters as slip-throughs on review\n>> or such, but it looks like the authors choice to me.\n>\n> Lowercase, brief, with no period at the end is the convention. This\n> is user-facing UI, so consistency matters. It's not author's choice.\n>\n> Existing examples of capitalized messages fall into a few categories:\n>\n> - shell scripts used to tend to use the capitalized form, and when\n> they got rewritten as builtins the messages weren't cleaned up at\n> the same time\n>\n> - some patch authors have different habits and it wasn't worth going\n> back and forth to fix it (but the convention still stands, and the\n> result of a program that can't decide how to talk to the user is\n> still harmful)\n>\n> - once there are a few examples, they get copy/pasted and imitated\n>\n>> I think it's a mistake to s/Could/could/g for all errors in the code base\n>> as it reduces the information provided in the error messages.\n>\n> This seems like an after-the-fact justification for a mistake.\n>\n> Often there is a choice about whether the caller or the callee to a\n> function prints an error. If the caller does, it can say more about\n> the context. If the callee does, the message is in one place and can\n> be tweaked more easily to be more useful.\n>\n> To get the benefits of both, we could print a backtrace with every\n> error. That way, the callee can describe the error in one place but\n> the context is all there. But I'm really glad we don't do that!\n>\n> The main purpose of most error messages is instructional: they give\n> information to the user in a way that is abstracted from the\n> implementation (the user doesn't care what function it happened in!)\n> that tells them what happened and what they can do next.\n> Gratuitous inconsistency in formatting doesn't help with that.\n\nI agree we should fix the formatting, but I was pointing out the side effects\nand how to avoid the side effects. So what I am proposing is a cleanup of\nthe warning messages as well as a GIT_TRACE_ERRORS variable as\nJeff proposed, because then we have all the benefits.\n\nIf we were to just cleanup the error messages we improve on certain aspects\n(UI), while making others worse.\n\n\n>\n> Actually, I wouldn't mind an environment variable that tells error()\n> to include a backtrace if someone wants it. (See backtrace(3)\n> for implementation hints if interested in doing that.)\n\nCONFORMING TO\n These functions are GNU extensions.\n\nI guess I can live with backtrace, but the Git for Windows people may\nhate me for it.\n\n\n>\n>> Just 3 days ago (\"Regular Rebase Failure\"). I used different\n>> capitalization to get a better understanding where the error may be.\n>\n> Wouldn't it be better if there weren't so many similar messages\n> produced in different contexts in the first place? (I.e., this\n> suggests to me that we should move more in the direction of\n> callee-produces-error.)\n>\n> Sorry, that was a long way to say very little. I just wanted to\n> emphasize that when a UI inconsistency has a useful side effect, while\n> that can be useful to understand and learn from, we should not be\n> afraid to fix it. And to clear up any ambiguity about git's error\n> message convention. :)\n>\n> Thanks and hope that helps,\n\nI really appreciate the background information provided!\n\nThanks,\nStefan\n\n> Jonathan\n"},{"id":"259962","messageId":"20150424201734.GA4747@peff.net","threadId":"39142","inReplyTo":"20150424181236.GK5467@google.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-24T20:17:34Z","receivedAt":"2015-04-24T20:17:34Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2015 at 11:12:36AM -0700, Jonathan Nieder wrote:\n\n> Actually, I wouldn't mind an environment variable that tells error()\n> to include a backtrace if someone wants it. (See backtrace(3)\n> for implementation hints if interested in doing that.)\n\nThanks, I didn't know about backtrace(3), and figured we'd be stuck with\nan ELF library or similar to get at the symbols.\n\nThat being said, the resulting backtrace is not nearly as nice as what\nis produced by gdb (which includes pretty-printed variables, or even\nlocal variables with \"bt full\"). Not everybody will have gdb, of course,\nbut nor will everybody have backtrace().\n\nSo if anything, I think my inclination would be to make it easier to\nhelp people (and ourselves) get a backtrace from gdb.\n\nOne can get a core for a running process with gcore, or trigger a\ncoredump by killing with SIGABRT. But catching it at the exact moment of\na die() is a bit hard without support from the program. What about\nsomething like this:\n\ndiff --git a/usage.c b/usage.c\nindex ed14645..fa92190 100644\n--- a/usage.c\n+++ b/usage.c\n@@ -34,6 +34,8 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n static NORETURN void die_builtin(const char *err, va_list params)\n {\n \tvreportf(\"fatal: \", err, params);\n+\tif (git_env_bool(\"GIT_DIE_ABORT\", 0))\n+\t\tabort();\n \texit(128);\n }\n \n\nUsage would be something like:\n\n ulimit -c unlimited ;# optional, but many distros seem to ship with\n ;# cores disabled these days\n GIT_DIE_ABORT=1 git some-command-that-fails\n gdb --quiet -ex 'bt full' -ex quit git core\n\nWe could even wrap that up in a git-debug script. I suspect there may be\nsome complications with finding the core file, though, as the git\nprocess may chdir before dumping. I'm not sure if there's a good way\naround that (obviously setting /proc/sys/kernel/core_pattern is not\nexactly friendly).\n\nI dunno. Certainly there is room for a less-full-featured system that\nwould run more seamlessly, or on more people's systems (i.e., so that we\ncan easily say \"set GIT_FOO and tell us what it outputs\" in response to\nbug reports). Maybe that system is backtrace(), or maybe it is just\n__FILE__ and __LINE__ markers for each printed error.\n\n-Peff\n"},{"id":"259975","messageId":"xmqqzj5w25vv.fsf@gitster.dls.corp.google.com","threadId":"39142","inReplyTo":"20150424201734.GA4747@peff.net","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-04-25T04:23:16Z","receivedAt":"2015-04-25T04:23:16Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So if anything, I think my inclination would be to make it easier to\n> help people (and ourselves) get a backtrace from gdb.\n>\n> One can get a core for a running process with gcore, or trigger a\n> coredump by killing with SIGABRT. But catching it at the exact moment of\n> a die() is a bit hard without support from the program. What about\n> something like this:\n>\n> diff --git a/usage.c b/usage.c\n> index ed14645..fa92190 100644\n> --- a/usage.c\n> +++ b/usage.c\n> @@ -34,6 +34,8 @@ static NORETURN void usage_builtin(const char *err, va_list params)\n> static NORETURN void die_builtin(const char *err, va_list params)\n> {\n> \tvreportf(\"fatal: \", err, params);\n> +\tif (git_env_bool(\"GIT_DIE_ABORT\", 0))\n> +\t\tabort();\n> \texit(128);\n> }\n\nTwo comments.\n\nThere probably are more than a few places that calls exit(128)\nwithout using die(), upon seeing that some helper function returned\nan error code, because the helper already gave an error message.\n\nThe proposals so far, including this one, assume that the bug\nreporter will first fail the operation with \"normal\" invocation\nof Git (e.g. without GIT_DIE_ABORT exported) and then retry the\nsame operation in a different way (e.g. with GIT_DIE_ABORT) to\ngive us something that would help diagnosis. Such a user could\njust rerun Git under gdb with breakpoint set to die_builtin, no?\n"},{"id":"259979","messageId":"20150425050058.GA20256@peff.net","threadId":"39142","inReplyTo":"xmqqzj5w25vv.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-25T05:00:58Z","receivedAt":"2015-04-25T05:00:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Apr 24, 2015 at 09:23:16PM -0700, Junio C Hamano wrote:\n\n> The proposals so far, including this one, assume that the bug\n> reporter will first fail the operation with \"normal\" invocation\n> of Git (e.g. without GIT_DIE_ABORT exported) and then retry the\n> same operation in a different way (e.g. with GIT_DIE_ABORT) to\n> give us something that would help diagnosis. Such a user could\n> just rerun Git under gdb with breakpoint set to die_builtin, no?\n\nGood point. I was trying to automate the gathering of the backtrace so\nthat even bug-reporters who have not used gdb could easily get us more\ninformation. But of course if a coredump only gets us halfway there and\nwe have to script gdb to convert the core into a backtrace anyway, it is\nnot buying us much over just scripting gdb in the first place.\n\nA better solution to what I proposed earlier is perhaps:\n\n git config alias.debug '!gdb --quiet \\\n\t -ex \"break exit\" \\\n\t -ex \"run\" \\\n\t -ex \"bt full\" \\\n\t -ex \"continue\" \\\n\t -ex \"quit\" \\\n\t --args git \\\n '\n git debug rev-parse foobar\n\nIt has the minor irritation that gdb will control the process stdio\n(slurping from stdin and polluting stdout, whereas we would prefer no\ninput and output to stderr). There might be a way to convince gdb to do\notherwise, or you could probably go pretty far with some shell fd\nredirects and using \"set args\" inside gdb. Or maybe something with\ngdbserver.\n\nBut the point is that yeah, we shouldn't try to build really good\nintrospection inside git. Debuggers already do a way better job of this.\nIf they're hard for people to use to obtain simple information like a\nbacktrace, we should work on wrapping that difficulty up in a script.\n\nIt might still be useful to provide a much lesser form of introspection,\nif it would be available in a lot more places than gdb would. E.g.,\n__FILE__ and __LINE__ markers on error messages might be useful. A\nmediocre backtrace() that is only available on glibc systems is probably\nnot.\n\n-Peff\n"},{"id":"259980","messageId":"20150425052427.GA20653@peff.net","threadId":"39142","inReplyTo":"20150425050058.GA20256@peff.net","subject":"Re: [PATCHv3] refs.c: enable large transactions","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2015-04-25T05:24:27Z","receivedAt":"2015-04-25T05:24:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Apr 25, 2015 at 01:00:58AM -0400, Jeff King wrote:\n\n> A better solution to what I proposed earlier is perhaps:\n> \n> git config alias.debug '!gdb --quiet \\\n> \t -ex \"break exit\" \\\n> \t -ex \"run\" \\\n> \t -ex \"bt full\" \\\n> \t -ex \"continue\" \\\n> \t -ex \"quit\" \\\n> \t --args git \\\n> '\n> git debug rev-parse foobar\n> \n> It has the minor irritation that gdb will control the process stdio\n> (slurping from stdin and polluting stdout, whereas we would prefer no\n> input and output to stderr). There might be a way to convince gdb to do\n> otherwise, or you could probably go pretty far with some shell fd\n> redirects and using \"set args\" inside gdb. Or maybe something with\n> gdbserver.\n\nJust to extend the thought exercise, here is something marginally less\nhorrible. Save as \"git-debug\" in your PATH and chmod +x.\n\n-- >8 --\n#!/bin/sh\n\nif ! type gdb >/dev/null 2>&1; then\n\techo >&2 \"Sorry, you don't seem to have gdb installed.\"\n\texit 1\nfi\n\nargs=\nfor i in \"$@\"; do\n\targs=\"$args '$(printf '%s' \"$i\" | sed \"s/'/'\\\\\\\\''/\")'\"\ndone\n\ngdb --quiet \\\n\t-ex \"set args $args <&7 >&8 2>&9\" 7<&0 8>&1 9>&2 &2 \\\n\t-ex 'break exit' \\\n\t-ex 'run' \\\n\t-ex 'bt full' \\\n\t-ex 'continue' \\\n\t-ex 'quit' \\\n\tgit\n-- 8< --\n\nIt's still rather hard to use with sub-programs started by git (e.g.,\nthe upload-pack spawned by a fetch), but I think it would work for many\nsimple cases. I'm not sure if I would use it myself. As somebody\nconfident in using gdb, my next step after seeing the backtrace would be\nto start poking around interactively. Besides the stdio magic, this is\nnot really buying me much beyond running gdb myself.\n\n-Peff\n"}]}