{"thread":{"id":"38013","subject":"[PATCH] refs.c: add a function to append a reflog entry to a fd","startedAt":"2014-11-20T00:42:22Z","lastAt":"2014-11-20T22:29:21Z","messageCount":18,"participants":["Stefan Beller","Junio C Hamano","Jonathan Nieder"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"252227","messageId":"1416444142-28042-1-git-send-email-sbeller@google.com","threadId":"38013","inReplyTo":null,"subject":"[PATCH] refs.c: add a function to append a reflog entry to a fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T00:42:22Z","receivedAt":"2014-11-20T00:42:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nBreak out the code to create the string and writing it to the file\ndescriptor from log_ref_write and add it into a dedicated function\nlog_ref_write_fd. For now this is only used from log_ref_write,\nbut later on we will call this function from reflog transactions too,\nwhich means that we will end up with only a single place,\nwhere we write a reflog entry to a file instead of the current two\nplaces (log_ref_write and builtin/reflog.c).\n\n[sb: replaced sprintf by snprintf for security reasons, \n which was there in the beginning (6de08ae688b9f2, 2006-06-17, \n Log ref updates to logs/refs/), but got lost in (8ac65937d032ad, \n 2007-01-26, Make sure we do not write bogus reflog entries.)\n]\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Jonathan Nieder \nSigned-off-by: Stefan Beller \n---\n refs.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\nThis is also part of the ref-transaction-reflog series, but is \nsufficiently independant, that I'd argue it's a general code hygiene thing,\nwe can use in any case.\n\nApplies on master.\n\nCompared to the last send of this patch[1], there was one change in the print\nfunction. Replaced sprintf by snprintf for security reasons. \n\n[1] part of the ref-transaction-reflog series \n http://permalink.gmane.org/gmane.comp.version-control.git/259714\n \n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..f9b42e5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \treturn 0;\n }\n \n+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n+\t\t\t const unsigned char *new_sha1,\n+\t\t\t const char *committer, const char *msg)\n+{\n+\tint msglen, written;\n+\tunsigned maxlen, len;\n+\tchar *logrec;\n+\n+\tmsglen = msg ? strlen(msg) : 0;\n+\tmaxlen = strlen(committer) + msglen + 100;\n+\tlogrec = xmalloc(maxlen);\n+\tlen = snprintf(logrec, maxlen, \"%s %s %s\\n\",\n+\t\t sha1_to_hex(old_sha1),\n+\t\t sha1_to_hex(new_sha1),\n+\t\t committer);\n+\tif (msglen)\n+\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n+\n+\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n+\tfree(logrec);\n+\tif (written != len)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg)\n {\n-\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n-\tunsigned maxlen, len;\n-\tint msglen;\n+\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \tchar log_file[PATH_MAX];\n-\tchar *logrec;\n-\tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \tlogfd = open(log_file, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tcommitter = git_committer_info(0);\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = sprintf(logrec, \"%s %s %s\\n\",\n-\t\t sha1_to_hex(old_sha1),\n-\t\t sha1_to_hex(new_sha1),\n-\t\t committer);\n-\tif (msglen)\n-\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n-\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n-\tfree(logrec);\n-\tif (written != len) {\n+\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n+\t\t\t\t git_committer_info(0), msg);\n+\tif (result) {\n \t\tint save_errno = errno;\n \t\tclose(logfd);\n \t\terror(\"Unable to append to %s\", log_file);\n-- \n2.2.0.rc2.13.g0786cdb\n"},{"id":"252232","messageId":"xmqqzjbm4qe5.fsf@gitster.dls.corp.google.com","threadId":"38013","inReplyTo":"1416444142-28042-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: add a function to append a reflog entry to a fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T01:22:42Z","receivedAt":"2014-11-20T01:22:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> Compared to the last send of this patch[1], there was one change in the print\n> function. Replaced sprintf by snprintf for security reasons. \n\nCareful. I despise people who blindly think strlcpy() and\nsnprintf() are good solutions for for security. They are by\nthemselves not.\n\nSurely, the use of them lets you avoid stomping on pieces of memory\nthat you did not intend to write to. I'd call that \"protecting\nother people's data\".\n\nBut what about your own data? If you are thinking that you are\ntrying to write \"this and that\", but by mistake (either by your or\nby a careless future change) if you did not allocate enough, you\nwould stop at \"this and th\". You may have protected other people's\ndata, but your result is still broken. If you gave that to others\nwithout checking that you produced an incomplete piece of data, what\nguarantee are you getting that you are not creating a security hole\nthere (imagine you intended to copy \"rm -fr ./tmpdir\" and by mistake\nyou stopped at \"rm -fr ./\" @~@).\n\n> +\tmsglen = msg ? strlen(msg) : 0;\n> +\tmaxlen = strlen(committer) + msglen + 100;\n> +\tlogrec = xmalloc(maxlen);\n> +\tlen = snprintf(logrec, maxlen, \"%s %s %s\\n\",\n> +\t\t sha1_to_hex(old_sha1),\n> +\t\t sha1_to_hex(new_sha1),\n> +\t\t committer);\n> +\tif (msglen)\n> +\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n\nIn this codepath, you are allocating enough buffer to hold the whole\nmessage; there is no difference between sprintf() and snprintf().\nIf the difference mattered, you would have chopped the reflog entry\ntoo short, and produced a wrong result, but you then discard the\nwhole record (the code that follows the above), losing data.\n\nSo use of snprintf() is not really buying you much here, not in the\ncurrent code certainly, but not as a future-proofing measure,\neither.\n"},{"id":"252234","messageId":"xmqqvbma4pu3.fsf@gitster.dls.corp.google.com","threadId":"38013","inReplyTo":"xmqqzjbm4qe5.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] refs.c: add a function to append a reflog entry to a fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T01:34:44Z","receivedAt":"2014-11-20T01:34:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Stefan Beller writes:\n>\n>> Compared to the last send of this patch[1], there was one change in the print\n>> function. Replaced sprintf by snprintf for security reasons. \n>\n> Careful. I despise people who blindly think strlcpy() and\n> snprintf() are good solutions for for security. They are by\n> themselves not.\n> ...\n> So use of snprintf() is not really buying you much here, not in the\n> current code certainly, but not as a future-proofing measure,\n> either.\n\nDon't get me wrong. I am not saying that using snprintf() here is\nbad per-se. There should be no difference.\n\nBut I do not want to see people getting in the habit of thinking \"I\nnow use snprintf/strlcpy instead of sprintf/strcpy, and made my code\nmore secure.\" Often they are not doing that.\n\nThe only case snprintf/strlcpy is useful is when your data does not\nmatter in its detail. E.g. when you are preparing human-readble\ndata whose first part is sufficient to convey the information you\nwant to convey, you would be perfectly happy if the result is\ntruncated. In such a case, counting to allocate enough to hold\neverything and running sprintf() only to chop the result later is\nnot necessary --- it still is not wrong, though --- and allocating\nenough to satisify the eventual chop length and using snprintf()\nis easier way to achieve the same result.\n\nBut I do not think this codepath falls into such an \"I am willing to\nlose data\" case.\n"},{"id":"252236","messageId":"20141120014249.GE6527@google.com","threadId":"38013","inReplyTo":"1416444142-28042-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: add a function to append a reflog entry to a fd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T01:42:49Z","receivedAt":"2014-11-20T01:42:49Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Compared to the last send of this patch[1], there was one change in the print\n> function. Replaced sprintf by snprintf for security reasons.\n\nI prefer the version that didn't change the code while we are\nextracting it into a new function. A separate patch on top can switch\nit to use strbuf_addf or xstrfmt if we want that.\n\nMy two cents,\nJonathan\n"},{"id":"252237","messageId":"1416449134-12281-1-git-send-email-sbeller@google.com","threadId":"38013","inReplyTo":"xmqqvbma4pu3.fsf@gitster.dls.corp.google.com","subject":"[PATCH] refs.c: add a function to append a reflog entry to a fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T02:05:34Z","receivedAt":"2014-11-20T02:05:34Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Break out the code to create the string and writing it to the file\ndescriptor from log_ref_write and add it into a dedicated function\nlog_ref_write_fd. For now this is only used from log_ref_write,\nbut later on we will call this function from reflog transactions too,\nwhich means that we will end up with only a single place,\nwhere we write a reflog entry to a file instead of the current two\nplaces (log_ref_write and builtin/reflog.c).\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n refs.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n \nSorry for the small change w.r.t. sprintf/snprintf\nHere comes a resend without changing code, but just making it a new function,\nso we come forwards with the patch.\n\n\nLet's discuss the change and decide if I shouldsend a follow up patch to change \nit into snprintf.\n\nOn Wed, Nov 19, 2014 at 5:22 PM, Junio C Hamano wrote:\n>> +     logrec = xmalloc(maxlen);\n>> +     len = snprintf(logrec, maxlen, \"%s %s %s\\n\",\n>> +                    sha1_to_hex(old_sha1),\n>> +                    sha1_to_hex(new_sha1),\n>> +                    committer);\n>> +     if (msglen)\n>> +             len += copy_msg(logrec + len - 1, msg) - 1;\n>\n> In this codepath, you are allocating enough buffer to hold the whole\n> message; there is no difference between sprintf() and snprintf().\n> If the difference mattered, you would have chopped the reflog entry\n> too short, and produced a wrong result, but you then discard the\n> whole record (the code that follows the above), losing data.\n\nHypothetically speaking:\n\nThere should be no difference between sprintf() and snprintf(). As soon as you\nwould have a difference, you'd risk stomping on other peoples data, which could lead\nto code insertion, iff the format of the inserted data is long enough overwriting\nparts of your stack? Or you could change data to alter the program flow to direct\nthe program to another easier abusable bug.\nDoes this make sense? This is about protecting a user from a malicious attacker.\n\n> imagine you intended to copy \"rm -fr ./tmpdir\" and by mistake\n> you stopped at \"rm -fr ./\" @~@).\n\nYes, that's when you should check the return code of snprintf and compare to \nstrlen(\"rm -fr ./tmpdir\"). That kind of bug also has severe consequences, but\nit's not yet involving a malicious attacker, so I'd file this as a safety instead \nof a security flaw.\n\nSafety related bugs are easier to find usually as you don't need to have to\nthink about what weird things an attacker might do. This kind of bug can be found\ndirectly from reading the code and its surroundings \n(\"there it should print 'rm -fr ./tmpdir'... Gah! I did not check the return value\")\n\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..f9b42e5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \treturn 0;\n }\n \n+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n+\t\t\t const unsigned char *new_sha1,\n+\t\t\t const char *committer, const char *msg)\n+{\n+\tint msglen, written;\n+\tunsigned maxlen, len;\n+\tchar *logrec;\n+\n+\tmsglen = msg ? strlen(msg) : 0;\n+\tmaxlen = strlen(committer) + msglen + 100;\n+\tlogrec = xmalloc(maxlen);\n+\tlen = sprintf(logrec, maxlen, \"%s %s %s\\n\",\n+\t\t sha1_to_hex(old_sha1),\n+\t\t sha1_to_hex(new_sha1),\n+\t\t committer);\n+\tif (msglen)\n+\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n+\n+\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n+\tfree(logrec);\n+\tif (written != len)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg)\n {\n-\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n-\tunsigned maxlen, len;\n-\tint msglen;\n+\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \tchar log_file[PATH_MAX];\n-\tchar *logrec;\n-\tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \tlogfd = open(log_file, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tcommitter = git_committer_info(0);\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = sprintf(logrec, \"%s %s %s\\n\",\n-\t\t sha1_to_hex(old_sha1),\n-\t\t sha1_to_hex(new_sha1),\n-\t\t committer);\n-\tif (msglen)\n-\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n-\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n-\tfree(logrec);\n-\tif (written != len) {\n+\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n+\t\t\t\t git_committer_info(0), msg);\n+\tif (result) {\n \t\tint save_errno = errno;\n \t\tclose(logfd);\n \t\terror(\"Unable to append to %s\", log_file);\n-- \n2.2.0.rc2.13.g0786cdb\n"},{"id":"252239","messageId":"20141120023655.GG6527@google.com","threadId":"38013","inReplyTo":"1416449134-12281-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH] refs.c: add a function to append a reflog entry to a fd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T02:36:55Z","receivedAt":"2014-11-20T02:36:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> [Subject: [PATCH] refs.c: add a function to append a reflog entry to a fd]\n\nDoes this supersede the other patch with the same subject?\n\nPlease keep adding v in the subject --- when it's there, it makes\nreading much easier.\n\n>\n\nMissing 'From:' line naming the original patch author.\n\n> Break out the code to create the string and writing it to the file\n> descriptor from log_ref_write and add it into a dedicated function\n> log_ref_write_fd.\n\n(grammar) I'm having trouble parsing the above.\n\nIs the idea something like \"Split out a function that [explanation\nof when a caller would use this function goes here]?\"\n\n> For now this is only used from log_ref_write,\n> but later on we will call this function from reflog transactions too,\n\nUseful to know. I'd end the sentence here, since it seems to run\non with a different thought.\n\n> which means that we will end up with only a single place,\n> where we write a reflog entry to a file instead of the current two\n> places (log_ref_write and builtin/reflog.c).\n\nAh, so builtin/reflog.c is doing something similar and will later\nbe changed to use this code, too?\n\nOverall it sounds like a very good change.\n\n[...]\n> Here comes a resend without changing code, but just making it a new function,\n> so we come forwards with the patch.\n\n\\o/\n\n> Let's discuss the change and decide if I shouldsend a follow up patch to change\n> it into snprintf.\n\nBoth sprintf and snprintf are error-prone functions. It would be\nlovely in a followup to use strbuf_addf or xstrfmt in this code path.\nstrbufs are how git deals with bookkeeping for string sizes --- they\nare very pleasant.\n\n[...]\n> +++ b/refs.c\n[...]\n> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n[...]\n> +\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n> +\t\t\t\t git_committer_info(0), msg);\n> +\tif (result) {\n> \t\tint save_errno = errno;\n> \t\tclose(logfd);\n> \t\terror(\"Unable to append to %s\", log_file);\n\nSince 'result' isn't used here, this could be simplified to\n\n\tif (log_ref_write_fd(...)) {\n\t\t...\n\t}\n\nThanks and hope that helps,\nJonathan\n"},{"id":"252264","messageId":"xmqqmw7l4xoj.fsf@gitster.dls.corp.google.com","threadId":"38013","inReplyTo":"20141120023655.GG6527@google.com","subject":"Re: [PATCH] refs.c: add a function to append a reflog entry to a fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T16:57:32Z","receivedAt":"2014-11-20T16:57:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n>> Break out the code to create the string and writing it to the file\n>> descriptor from log_ref_write and add it into a dedicated function\n>> log_ref_write_fd.\n>\n> (grammar) I'm having trouble parsing the above.\n\nYeah, I can see what it wants to say, but still...\n\n>> Let's discuss the change and decide if I shouldsend a follow up patch to change\n>> it into snprintf.\n>\n> Both sprintf and snprintf are error-prone functions. It would be\n> lovely in a followup to use strbuf_addf or xstrfmt in this code path.\n> strbufs are how git deals with bookkeeping for string sizes --- they\n> are very pleasant.\n\nYeah, that solves both sides of not stomping on other peoples' data\n(which may lead to replaced return address and such) and not giving\nbroken result (which may cause trouble to the callers) equally well\nwithout raising a stink about \"security!!!\", which is good ;-).\n\n>> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname,\n>> const unsigned char *old_sha1,\n> [...]\n>> +\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n>> +\t\t\t\t git_committer_info(0), msg);\n>> +\tif (result) {\n>> \t\tint save_errno = errno;\n>> \t\tclose(logfd);\n>> \t\terror(\"Unable to append to %s\", log_file);\n>\n> Since 'result' isn't used here, this could be simplified to\n>\n> \tif (log_ref_write_fd(...)) {\n> \t\t...\n> \t}\n\nYeah that is a good simplification.\n"},{"id":"252282","messageId":"1416508657-9057-1-git-send-email-sbeller@google.com","threadId":"38013","inReplyTo":"20141120023655.GG6527@google.com","subject":"[PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T18:37:37Z","receivedAt":"2014-11-20T18:37:37Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nMove code to create the string for a ref and write it to a file descriptor\nfrom log_ref_write and add it into a new dedicated function\nlog_ref_write_fd.\n\nFor now the new function is only used from log_ref_write, but later\non we will call this function from reflog transactions too. That means\nthat we will end up with only a single place, where we write a\nreflog entry to a file instead of the current two places\n(log_ref_write and builtin/reflog.c).\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n\nChanges in version 3:\n* reword the commit message to make it more understandable.\n* no changes in code\n* wait for the follow up to address any changes in the code.\n\n refs.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..9948841 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \treturn 0;\n }\n \n+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n+\t\t\t const unsigned char *new_sha1,\n+\t\t\t const char *committer, const char *msg)\n+{\n+\tint msglen, written;\n+\tunsigned maxlen, len;\n+\tchar *logrec;\n+\n+\tmsglen = msg ? strlen(msg) : 0;\n+\tmaxlen = strlen(committer) + msglen + 100;\n+\tlogrec = xmalloc(maxlen);\n+\tlen = sprintf(logrec, maxlen, \"%s %s %s\\n\",\n+\t\t sha1_to_hex(old_sha1),\n+\t\t sha1_to_hex(new_sha1),\n+\t\t committer);\n+\tif (msglen)\n+\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n+\n+\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n+\tfree(logrec);\n+\tif (written != len)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg)\n {\n-\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n-\tunsigned maxlen, len;\n-\tint msglen;\n+\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \tchar log_file[PATH_MAX];\n-\tchar *logrec;\n-\tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \tlogfd = open(log_file, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tcommitter = git_committer_info(0);\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = sprintf(logrec, \"%s %s %s\\n\",\n-\t\t sha1_to_hex(old_sha1),\n-\t\t sha1_to_hex(new_sha1),\n-\t\t committer);\n-\tif (msglen)\n-\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n-\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n-\tfree(logrec);\n-\tif (written != len) {\n+\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n+\t\t\t\t git_committer_info(0), msg);\n+\tif (result) {\n \t\tint save_errno = errno;\n \t\tclose(logfd);\n \t\terror(\"Unable to append to %s\", log_file);\n-- \n2.2.0.rc2.23.gca0107e\n"},{"id":"252296","messageId":"20141120212022.GJ6527@google.com","threadId":"38013","inReplyTo":"1416508657-9057-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T21:20:22Z","receivedAt":"2014-11-20T21:20:22Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> From: Ronnie Sahlberg \n>\n> Move code to create the string for a ref and write it to a file descriptor\n> from log_ref_write and add it into a new dedicated function\n> log_ref_write_fd.\n>\n> For now the new function is only used from log_ref_write, but later\n> on we will call this function from reflog transactions too. That means\n> that we will end up with only a single place, where we write a\n> reflog entry to a file instead of the current two places\n> (log_ref_write and builtin/reflog.c).\n\nLine-wrapping width is still inconsistent. I don't think it's worth\nresending just for that, but something to look out for in the future.\n\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n[...]\n> +++ b/refs.c\n[...]\n> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n[...]\n> +\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n> +\t\t\t\t git_committer_info(0), msg);\n> +\tif (result) {\n> \t\tint save_errno = errno;\n\nI don't understand why the above writes to a temporary variable and\nchecks it, never to read that temporary again.\n\nI don't think that alone is a reason to block the patch, but it\nworries me in that the review comment seems to have been just lost.\n"},{"id":"252297","messageId":"CAGZ79kZDMWyW46wNgqudPiX1UmZteyFVJXMAYqJpp8e_r8LCmQ@mail.gmail.com","threadId":"38013","inReplyTo":"20141120212022.GJ6527@google.com","subject":"Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T21:24:02Z","receivedAt":"2014-11-20T21:24:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder wrote:\n>>\n>> For now the new function is only used from log_ref_write, but later\n>> on we will call this function from reflog transactions too. That means\n>> that we will end up with only a single place, where we write a\n>> reflog entry to a file instead of the current two places\n>> (log_ref_write and builtin/reflog.c).\n>\n> Line-wrapping width is still inconsistent. I don't think it's worth\n> resending just for that, but something to look out for in the future.\n>\n\nok, I'll care about that more in the future.\n\n\n> I don't understand why the above writes to a temporary variable and\n> checks it, never to read that temporary again.\n>\n> I don't think that alone is a reason to block the patch, but it\n> worries me in that the review comment seems to have been just lost.\n\nIt wasn't lost as I think it should go in a follow up patch. Sorry for\nnot stating that clearly.\n(This patch is about moving code around, not changing code)\n\nI got interrupted preparing the follow up patch, which gets rid of the temporary\nvariable.\n"},{"id":"252299","messageId":"20141120213155.GK6527@google.com","threadId":"38013","inReplyTo":"CAGZ79kZDMWyW46wNgqudPiX1UmZteyFVJXMAYqJpp8e_r8LCmQ@mail.gmail.com","subject":"Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T21:31:55Z","receivedAt":"2014-11-20T21:31:55Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n> On Thu, Nov 20, 2014 at 1:20 PM, Jonathan Nieder wrote:\n\n>> I don't understand why the above writes to a temporary variable and\n>> checks it, never to read that temporary again.\n>>\n>> I don't think that alone is a reason to block the patch, but it\n>> worries me in that the review comment seems to have been just lost.\n>\n> It wasn't lost as I think it should go in a follow up patch. Sorry for\n> not stating that clearly.\n> (This patch is about moving code around, not changing code)\n\nAh, sorry for the lack of clarity.\n\nI agree completely with the above principle. But the code that writes\nto result and checks result is new code, not part of the code that\nmoved.\n"},{"id":"252300","messageId":"xmqq4mtt35x1.fsf@gitster.dls.corp.google.com","threadId":"38013","inReplyTo":"1416508657-9057-1-git-send-email-sbeller@google.com","subject":"Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T21:42:34Z","receivedAt":"2014-11-20T21:42:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> From: Ronnie Sahlberg \n>\n> Move code to create the string for a ref and write it to a file descriptor\n> from log_ref_write and add it into a new dedicated function\n> log_ref_write_fd.\n>\n> For now the new function is only used from log_ref_write, but later\n> on we will call this function from reflog transactions too. That means\n> that we will end up with only a single place, where we write a\n> reflog entry to a file instead of the current two places\n> (log_ref_write and builtin/reflog.c).\n>\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n>\n> Changes in version 3:\n> * reword the commit message to make it more understandable.\n> * no changes in code\n> * wait for the follow up to address any changes in the code.\n>\n> refs.c | 48 ++++++++++++++++++++++++++++++------------------\n> 1 file changed, 30 insertions(+), 18 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 5ff457e..9948841 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n> \treturn 0;\n> }\n> \n> +static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n> +\t\t\t const unsigned char *new_sha1,\n> +\t\t\t const char *committer, const char *msg)\n> +{\n> +\tint msglen, written;\n> +\tunsigned maxlen, len;\n> +\tchar *logrec;\n> +\n> +\tmsglen = msg ? strlen(msg) : 0;\n> +\tmaxlen = strlen(committer) + msglen + 100;\n> +\tlogrec = xmalloc(maxlen);\n> +\tlen = sprintf(logrec, maxlen, \"%s %s %s\\n\",\n\nYou cannot have maxlen here ;-)\n\n> +\t\t sha1_to_hex(old_sha1),\n> +\t\t sha1_to_hex(new_sha1),\n> +\t\t committer);\n> +\tif (msglen)\n> +\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n> +\n> +\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n> +\tfree(logrec);\n> +\tif (written != len)\n> +\t\treturn -1;\n> +\n> +\treturn 0;\n> +}\n> +\n> static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n> \t\t\t const unsigned char *new_sha1, const char *msg)\n> {\n> -\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n> -\tunsigned maxlen, len;\n> -\tint msglen;\n> +\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n> \tchar log_file[PATH_MAX];\n> -\tchar *logrec;\n> -\tconst char *committer;\n> \n> \tif (log_all_ref_updates < 0)\n> \t\tlog_all_ref_updates = !is_bare_repository();\n> @@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n> \tlogfd = open(log_file, oflags);\n> \tif (logfd < 0)\n> \t\treturn 0;\n> -\tmsglen = msg ? strlen(msg) : 0;\n> -\tcommitter = git_committer_info(0);\n> -\tmaxlen = strlen(committer) + msglen + 100;\n> -\tlogrec = xmalloc(maxlen);\n> -\tlen = sprintf(logrec, \"%s %s %s\\n\",\n> -\t\t sha1_to_hex(old_sha1),\n> -\t\t sha1_to_hex(new_sha1),\n> -\t\t committer);\n> -\tif (msglen)\n> -\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n> -\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n> -\tfree(logrec);\n> -\tif (written != len) {\n> +\tresult = log_ref_write_fd(logfd, old_sha1, new_sha1,\n> +\t\t\t\t git_committer_info(0), msg);\n> +\tif (result) {\n> \t\tint save_errno = errno;\n> \t\tclose(logfd);\n> \t\terror(\"Unable to append to %s\", log_file);\n"},{"id":"252301","messageId":"20141120215800.GA22370@google.com","threadId":"38013","inReplyTo":"xmqq4mtt35x1.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T21:58:00Z","receivedAt":"2014-11-20T21:58:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">From 4bec12b878ca02a1e80af3c265e7e7ab52ba17ce Mon Sep 17 00:00:00 2001\nFrom: Ronnie Sahlberg \nDate: Thu, 20 Nov 2014 13:48:14 -0800\nSubject: [PATCH v4] refs.c: add a function to append a reflog entry to a fd\n\nMove code to create the string for a ref and write it to a file descriptor\nfrom log_ref_write and add it into a new dedicated function\nlog_ref_write_fd.\n\nFor now the new function is only used from log_ref_write, but later\non we will call this function from reflog transactions too. That means\nthat we will end up with only a single place, where we write a reflog\nentry to a file instead of the current two places (log_ref_write and\nbuiltin/reflog.c).\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n \nChanges in version 3:\n * reword the commit message to make it more understandable.\n * no changes in code\n * wait for the follow up to address any changes in the code.\n\nChanges in version 4:\n * fix arguments of sprintf, (note to self: compile testing helps)\n * break lines of commit message again to appease the taste of Jonathan ;)\n * take the simplification for\n \n result = log_ref_write_fd(...);\n \n if (result) {\n ...\n \n and omit the result variable in there to just become\n \n if (log_ref_write_fd(...)) {\n ...\n \n refs.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..a6088e3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \treturn 0;\n }\n \n+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n+\t\t\t const unsigned char *new_sha1,\n+\t\t\t const char *committer, const char *msg)\n+{\n+\tint msglen, written;\n+\tunsigned maxlen, len;\n+\tchar *logrec;\n+\n+\tmsglen = msg ? strlen(msg) : 0;\n+\tmaxlen = strlen(committer) + msglen + 100;\n+\tlogrec = xmalloc(maxlen);\n+\tlen = sprintf(logrec, \"%s %s %s\\n\",\n+\t\t sha1_to_hex(old_sha1),\n+\t\t sha1_to_hex(new_sha1),\n+\t\t committer);\n+\tif (msglen)\n+\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n+\n+\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n+\tfree(logrec);\n+\tif (written != len)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg)\n {\n-\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n-\tunsigned maxlen, len;\n-\tint msglen;\n+\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \tchar log_file[PATH_MAX];\n-\tchar *logrec;\n-\tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \tlogfd = open(log_file, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tcommitter = git_committer_info(0);\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = sprintf(logrec, \"%s %s %s\\n\",\n-\t\t sha1_to_hex(old_sha1),\n-\t\t sha1_to_hex(new_sha1),\n-\t\t committer);\n-\tif (msglen)\n-\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n-\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n-\tfree(logrec);\n-\tif (written != len) {\n+\n+\tif (log_ref_write_fd(logfd, old_sha1, new_sha1,\n+\t\t\t git_committer_info(0), msg)) {\n \t\tint save_errno = errno;\n \t\tclose(logfd);\n \t\terror(\"Unable to append to %s\", log_file);\n-- \n2.2.0.rc2.23.gca0107e\n"},{"id":"252302","messageId":"20141120215941.GB22370@google.com","threadId":"38013","inReplyTo":"xmqq4mtt35x1.fsf@gitster.dls.corp.google.com","subject":"[PATCH v4] refs.c: add a function to append a reflog entry to a fd","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2014-11-20T21:59:41Z","receivedAt":"2014-11-20T21:59:41Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"From: Ronnie Sahlberg \n\nMove code to create the string for a ref and write it to a file descriptor\nfrom log_ref_write and add it into a new dedicated function\nlog_ref_write_fd.\n\nFor now the new function is only used from log_ref_write, but later\non we will call this function from reflog transactions too. That means\nthat we will end up with only a single place, where we write a reflog\nentry to a file instead of the current two places (log_ref_write and\nbuiltin/reflog.c).\n\nSigned-off-by: Ronnie Sahlberg \nSigned-off-by: Stefan Beller \n---\n \nChanges in version 3:\n * reword the commit message to make it more understandable.\n * no changes in code\n * wait for the follow up to address any changes in the code.\n\nChanges in version 4:\n * fix arguments of sprintf\n * break lines of commit message again to appease the taste of Jonathan ;)\n * take the simplification for\n \n result = log_ref_write_fd(...);\n \n if (result) {\n ...\n \n and omit the result variable in there to just become\n \n if (log_ref_write_fd(...)) {\n ...\n \n refs.c | 48 ++++++++++++++++++++++++++++++------------------\n 1 file changed, 30 insertions(+), 18 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 5ff457e..a6088e3 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -2990,15 +2990,37 @@ int log_ref_setup(const char *refname, char *logfile, int bufsize)\n \treturn 0;\n }\n \n+static int log_ref_write_fd(int fd, const unsigned char *old_sha1,\n+\t\t\t const unsigned char *new_sha1,\n+\t\t\t const char *committer, const char *msg)\n+{\n+\tint msglen, written;\n+\tunsigned maxlen, len;\n+\tchar *logrec;\n+\n+\tmsglen = msg ? strlen(msg) : 0;\n+\tmaxlen = strlen(committer) + msglen + 100;\n+\tlogrec = xmalloc(maxlen);\n+\tlen = sprintf(logrec, \"%s %s %s\\n\",\n+\t\t sha1_to_hex(old_sha1),\n+\t\t sha1_to_hex(new_sha1),\n+\t\t committer);\n+\tif (msglen)\n+\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n+\n+\twritten = len <= maxlen ? write_in_full(fd, logrec, len) : -1;\n+\tfree(logrec);\n+\tif (written != len)\n+\t\treturn -1;\n+\n+\treturn 0;\n+}\n+\n static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \t\t\t const unsigned char *new_sha1, const char *msg)\n {\n-\tint logfd, result, written, oflags = O_APPEND | O_WRONLY;\n-\tunsigned maxlen, len;\n-\tint msglen;\n+\tint logfd, result, oflags = O_APPEND | O_WRONLY;\n \tchar log_file[PATH_MAX];\n-\tchar *logrec;\n-\tconst char *committer;\n \n \tif (log_all_ref_updates < 0)\n \t\tlog_all_ref_updates = !is_bare_repository();\n@@ -3010,19 +3032,9 @@ static int log_ref_write(const char *refname, const unsigned char *old_sha1,\n \tlogfd = open(log_file, oflags);\n \tif (logfd < 0)\n \t\treturn 0;\n-\tmsglen = msg ? strlen(msg) : 0;\n-\tcommitter = git_committer_info(0);\n-\tmaxlen = strlen(committer) + msglen + 100;\n-\tlogrec = xmalloc(maxlen);\n-\tlen = sprintf(logrec, \"%s %s %s\\n\",\n-\t\t sha1_to_hex(old_sha1),\n-\t\t sha1_to_hex(new_sha1),\n-\t\t committer);\n-\tif (msglen)\n-\t\tlen += copy_msg(logrec + len - 1, msg) - 1;\n-\twritten = len <= maxlen ? write_in_full(logfd, logrec, len) : -1;\n-\tfree(logrec);\n-\tif (written != len) {\n+\n+\tif (log_ref_write_fd(logfd, old_sha1, new_sha1,\n+\t\t\t git_committer_info(0), msg)) {\n \t\tint save_errno = errno;\n \t\tclose(logfd);\n \t\terror(\"Unable to append to %s\", log_file);\n-- \n2.2.0.rc2.23.gca0107e\n"},{"id":"252303","messageId":"20141120220558.GL6527@google.com","threadId":"38013","inReplyTo":"20141120215800.GA22370@google.com","subject":"Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T22:05:58Z","receivedAt":"2014-11-20T22:05:58Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> From 4bec12b878ca02a1e80af3c265e7e7ab52ba17ce Mon Sep 17 00:00:00 2001\n\nThe above line causes \"git am\" to be unable to parse the message\ndownloaded as an mbox, if I remember correctly.\n\n[...]\n> * break lines of commit message again to appease the taste of Jonathan ;)\n\nI hope it's not just to appease me. If the rationale behind a comment I\nmake isn't clear, always feel free to ask.\n\nIn this case, it is about readability. It's perhaps irrational, but I\nfind text much more readable and the intent to be clearer when\nparagraphs are wrapped to a consistent width instead of lines breaking\nat arbitrary points.\n\nPerhaps a nice way to make this a non-issue in the future would be an\noption to \"git am\" to rewrap.\n"},{"id":"252304","messageId":"20141120221110.GM6527@google.com","threadId":"38013","inReplyTo":"20141120215941.GB22370@google.com","subject":"Re: [PATCH v4] refs.c: add a function to append a reflog entry to a fd","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2014-11-20T22:11:10Z","receivedAt":"2014-11-20T22:11:10Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Stefan Beller wrote:\n\n> Signed-off-by: Ronnie Sahlberg \n> Signed-off-by: Stefan Beller \n> ---\n[...]\n> refs.c | 48 ++++++++++++++++++++++++++++++------------------\n> 1 file changed, 30 insertions(+), 18 deletions(-)\n\nThe --patience diff makes review of this version very simple.\n\nReviewed-by: Jonathan Nieder \n\nThanks.\n"},{"id":"252305","messageId":"xmqqzjbl1q0v.fsf@gitster.dls.corp.google.com","threadId":"38013","inReplyTo":"20141120220558.GL6527@google.com","subject":"Re: [PATCH v3] refs.c: add a function to append a reflog entry to a fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T22:11:12Z","receivedAt":"2014-11-20T22:11:12Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> In this case, it is about readability. It's perhaps irrational, but I\n> find text much more readable and the intent to be clearer when\n> paragraphs are wrapped to a consistent width instead of lines breaking\n> at arbitrary points.\n\nYeah I agree with that.\n\nAlso a blank line between paragraphs, but that goes without saying\nonce you accept that we do not do a single line per paragraph.\n\n> Perhaps a nice way to make this a non-issue in the future would be an\n> option to \"git am\" to rewrap.\n\nNo. There are cases where you do need to have a single oddball long\nline that you cannot flow. In a project that employs \"am\", the\nusers of \"am\" are much more likely to become bottleneck than the\nsenders of patches, and it does not make sense to have them spend\ncycles to decide if they want to let \"am\" do such wrapping.\n"},{"id":"252306","messageId":"xmqqvbm91p6m.fsf@gitster.dls.corp.google.com","threadId":"38013","inReplyTo":"20141120221110.GM6527@google.com","subject":"Re: [PATCH v4] refs.c: add a function to append a reflog entry to a fd","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-20T22:29:21Z","receivedAt":"2014-11-20T22:29:21Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> Stefan Beller wrote:\n>\n>> Signed-off-by: Ronnie Sahlberg \n>> Signed-off-by: Stefan Beller \n>> ---\n> [...]\n>> refs.c | 48 ++++++++++++++++++++++++++++++------------------\n>> 1 file changed, 30 insertions(+), 18 deletions(-)\n>\n> The --patience diff makes review of this version very simple.\n\nI guess it depends on what the reader is focusing on, but at least\nto me I found the default or histogram much easier to see what is\ngoing on. It could be due to my use of large -U value,\ntoo.\n\n> Reviewed-by: Jonathan Nieder \n\nThanks.\n"}]}