{"thread":{"id":"30096","subject":"[PATCH 1/4] sha1_name: introduce getn_sha1() to take length","startedAt":"2012-03-29T13:58:11Z","lastAt":"2012-04-04T20:53:21Z","messageCount":11,"participants":["Ramkumar Ramachandra","Junio C Hamano","Jonathan Nieder","Matthieu Moy"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"188060","messageId":"1333029495-10034-1-git-send-email-artagnon@gmail.com","threadId":"30096","inReplyTo":null,"subject":"[PATCH 0/4] Minor sequencer.c cleanups","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-03-29T13:58:11Z","receivedAt":"2012-03-29T13:58:11Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"I have a lot of code left over from my work on the sequencer; I'm\nslowly rebasing the work and sending out patches. This series\nincludes some minor improvements to sequencer.c. All tests pass.\n\nThanks for reading.\n\n Ram\n\nRamkumar Ramachandra (4):\n sha1_name: introduce getn_sha1() to take length\n revert: use getn_sha1() to simplify insn parsing\n revert: simplify insn parsing logic\n revert: report fine-grained errors from insn parser\n\n cache.h | 1 +\n sequencer.c | 64 +++++++++++++++++++++++++++++++++++------------------------\n sha1_name.c | 23 +++++++++++++++++---\n 3 files changed, 58 insertions(+), 30 deletions(-)\n\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"188059","messageId":"1333029495-10034-2-git-send-email-artagnon@gmail.com","threadId":"30096","inReplyTo":"1333029495-10034-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 1/4] sha1_name: introduce getn_sha1() to take length","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-03-29T13:58:12Z","receivedAt":"2012-03-29T13:58:12Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Introduce a variant of get_sha1() that additionally takes the length\nof the buffer, so it can parse object names from buffers that don't\nnecessarily terminate with NUL.\n\nSuggested-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n cache.h | 1 +\n sha1_name.c | 23 +++++++++++++++++++----\n 2 files changed, 20 insertions(+), 4 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex e5e1aa4..e52c354 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -813,6 +813,7 @@ struct object_context {\n };\n \n extern int get_sha1(const char *str, unsigned char *sha1);\n+extern int getn_sha1(const char *name, int namelen, unsigned char *sha1);\n extern int get_sha1_with_mode_1(const char *str, unsigned char *sha1, unsigned *mode, int only_to_die, const char *prefix);\n static inline int get_sha1_with_mode(const char *str, unsigned char *sha1, unsigned *mode)\n {\ndiff --git a/sha1_name.c b/sha1_name.c\nindex 03ffc2c..31d412e 100644\n--- a/sha1_name.c\n+++ b/sha1_name.c\n@@ -1019,12 +1019,11 @@ static char *resolve_relative_path(const char *rel)\n \t\t\t rel);\n }\n \n-int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n-\t\t\t struct object_context *oc,\n-\t\t\t int only_to_die, const char *prefix)\n+static int getn_sha1_with_context_1(const char *name, int namelen,\n+\t\t\t\tunsigned char *sha1, struct object_context *oc,\n+\t\t\t\tint only_to_die, const char *prefix)\n {\n \tint ret, bracket_depth;\n-\tint namelen = strlen(name);\n \tconst char *cp;\n \n \tmemset(oc, 0, sizeof(*oc));\n@@ -1134,3 +1133,19 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n \t}\n \treturn ret;\n }\n+\n+int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n+\t\t\tstruct object_context *oc,\n+\t\t\tint only_to_die, const char *prefix)\n+{\n+\tint namelen = strlen(name);\n+\treturn getn_sha1_with_context_1(name, namelen, sha1,\n+\t\t\t\t\toc, only_to_die, prefix);\n+}\n+\n+/* A variant of get_sha1 that takes a length. */\n+int getn_sha1(const char *name, int namelen, unsigned char *sha1)\n+{\n+\tstruct object_context unused;\n+\treturn getn_sha1_with_context_1(name, namelen, sha1, &unused, 0, NULL);\n+}\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"188061","messageId":"1333029495-10034-3-git-send-email-artagnon@gmail.com","threadId":"30096","inReplyTo":"1333029495-10034-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 2/4] revert: use getn_sha1() to simplify insn parsing","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-03-29T13:58:13Z","receivedAt":"2012-03-29T13:58:13Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"To read the object name in the instruction sheet, we currently\nmanipulate the buffer to artificially introduce a NUL after the\nsupposed object name, and then use get_sha1() to read the object name\nbefore restoring the buffer. Get rid of this ugliness by using\ngetn_sha1(), a function introduced in the previous patch.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n sequencer.c | 12 +++---------\n 1 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex a37846a..458cffb 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -517,8 +517,7 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n {\n \tunsigned char commit_sha1[20];\n \tenum replay_action action;\n-\tchar *end_of_object_name;\n-\tint saved, status, padding;\n+\tint namelen, padding;\n \n \tif (!prefixcmp(bol, \"pick\")) {\n \t\taction = REPLAY_PICK;\n@@ -535,12 +534,6 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \t\treturn NULL;\n \tbol += padding;\n \n-\tend_of_object_name = bol + strcspn(bol, \" \\t\\n\");\n-\tsaved = *end_of_object_name;\n-\t*end_of_object_name = '\\0';\n-\tstatus = get_sha1(bol, commit_sha1);\n-\t*end_of_object_name = saved;\n-\n \t/*\n \t * Verify that the action matches up with the one in\n \t * opts; we don't support arbitrary instructions\n@@ -552,8 +545,8 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \t\treturn NULL;\n \t}\n \n-\tif (status < 0)\n+\tnamelen = strcspn(bol, \" \\t\\n\");\n+\tif (getn_sha1(bol, namelen, commit_sha1))\n \t\treturn NULL;\n \n \treturn lookup_commit_reference(commit_sha1);\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"188063","messageId":"1333029495-10034-4-git-send-email-artagnon@gmail.com","threadId":"30096","inReplyTo":"1333029495-10034-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 3/4] revert: simplify insn parsing logic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-03-29T13:58:14Z","receivedAt":"2012-03-29T13:58:14Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Our instruction sheet parser first looks for a valid action by\nchecking that the buffer starts with either \"pick\" or \"revert\". Then,\nit looks for either spaces or tabs before looking for the object name,\nerroring out if it doesn't find any. Simplify this logic without\nmaking any functional changes by looking for (\"pick \" or \"pick\\t\") or\n(\"revert \" or \"revert\\t\") in the first place.\n\nSigned-off-by: Ramkumar Ramachandra \n---\n sequencer.c | 11 ++++-------\n 1 files changed, 4 insertions(+), 7 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 458cffb..02e58ad 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -517,22 +517,19 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n {\n \tunsigned char commit_sha1[20];\n \tenum replay_action action;\n-\tint namelen, padding;\n+\tint namelen;\n \n-\tif (!prefixcmp(bol, \"pick\")) {\n+\tif (!prefixcmp(bol, \"pick \") || !prefixcmp(bol, \"pick\\t\")) {\n \t\taction = REPLAY_PICK;\n \t\tbol += strlen(\"pick\");\n-\t} else if (!prefixcmp(bol, \"revert\")) {\n+\t} else if (!prefixcmp(bol, \"revert \") || !prefixcmp(bol, \"revert\\t\")) {\n \t\taction = REPLAY_REVERT;\n \t\tbol += strlen(\"revert\");\n \t} else\n \t\treturn NULL;\n \n \t/* Eat up extra spaces/ tabs before object name */\n-\tpadding = strspn(bol, \" \\t\");\n-\tif (!padding)\n-\t\treturn NULL;\n-\tbol += padding;\n+\tbol += strspn(bol, \" \\t\");\n \n \t/*\n \t * Verify that the action matches up with the one in\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"188062","messageId":"1333029495-10034-5-git-send-email-artagnon@gmail.com","threadId":"30096","inReplyTo":"1333029495-10034-1-git-send-email-artagnon@gmail.com","subject":"[PATCH 4/4] revert: report fine-grained errors from insn parser","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-03-29T13:58:15Z","receivedAt":"2012-03-29T13:58:15Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"The infrastructure that parses '.git/sequencer/todo' is meant to\nhandle arbitrary user input some day, so it can be used as the\nimplementation of 'git rebase --interactive' and 'git sequencer\n--edit'. It is currently sub-optimal for that purpose because the\nparse error messages just say:\n\n error: Could not parse line 5.\n\nThis patch shifts responsibility to parse_insn_line(), which can come\nup with a more detailed message like:\n\n error: .git/sequencer/todo:5: unrecognized action: frobnicate\n\nOnce the operator is allowed to edit the sequence, the message might\nbe adjusted to something like:\n\n error: :5: unrecognized action: frobnicate\n\ninstead of exposing an implementation detail. Some day 'git sequencer\n--edit' could even re-launch the editor with an error message in a\ncomment before the problematic line and the cursor pointing there.\nFor now, pointing to the explicit filename is useful since this should\nonly happen if there was filesystem corruption, tampering, or a git\nbug.\n\nHelped-by: Junio C Hamano \nHelped-by: Jonathan Nieder \nSigned-off-by: Ramkumar Ramachandra \n---\n sequencer.c | 45 +++++++++++++++++++++++++++++++++------------\n 1 files changed, 33 insertions(+), 12 deletions(-)\n\ndiff --git a/sequencer.c b/sequencer.c\nindex 02e58ad..cb93ee8 100644\n--- a/sequencer.c\n+++ b/sequencer.c\n@@ -513,7 +513,22 @@ static int format_todo(struct strbuf *buf, struct commit_list *todo_list,\n \treturn 0;\n }\n \n-static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *opts)\n+static int parse_error(const char *message, const char *file,\n+\t\tint lineno, char *error_line)\n+{\n+\tconst char *suffix = \"\";\n+\tint error_len = strcspn(error_line, \" \\t\\n\");\n+\n+\tif (error_len > 20) {\n+\t\terror_len = 20;\n+\t\tsuffix = \"...\";\n+\t}\n+\treturn error(_(\"%s:%d: %s: %.*s%s\"), file, lineno, message,\n+\t\terror_len, error_line, suffix);\n+}\n+\n+int parse_insn_line(char *bol, char *eol, struct replay_opts *opts,\n+\t\tstruct commit_list *list, int lineno)\n {\n \tunsigned char commit_sha1[20];\n \tenum replay_action action;\n@@ -526,7 +541,8 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \t\taction = REPLAY_REVERT;\n \t\tbol += strlen(\"revert\");\n \t} else\n-\t\treturn NULL;\n+\t\treturn parse_error(_(\"unrecognized action\"),\n+\t\t\t\tgit_path(SEQ_TODO_FILE), lineno, bol);\n \n \t/* Eat up extra spaces/ tabs before object name */\n \tbol += strspn(bol, \" \\t\");\n@@ -538,32 +554,36 @@ static struct commit *parse_insn_line(char *bol, char *eol, struct replay_opts *\n \tif (action != opts->action) {\n \t\tconst char *action_str;\n \t\taction_str = action == REPLAY_REVERT ? \"revert\" : \"cherry-pick\";\n-\t\terror(_(\"Cannot %s during a %s\"), action_str, action_name(opts));\n-\t\treturn NULL;\n+\t\treturn parse_error(_(\"invalid action\"),\n+\t\t\t\tgit_path(SEQ_TODO_FILE), lineno, bol);\n \t}\n \n \tnamelen = strcspn(bol, \" \\t\\n\");\n \tif (getn_sha1(bol, namelen, commit_sha1))\n-\t\treturn NULL;\n-\n-\treturn lookup_commit_reference(commit_sha1);\n+\t\treturn parse_error(_(\"malformed object name\"),\n+\t\t\t\tgit_path(SEQ_TODO_FILE), lineno, bol);\n+\n+\tlist->item = lookup_commit_reference(commit_sha1);\n+\tif (!list->item)\n+\t\treturn parse_error(_(\"not a valid commit\"),\n+\t\t\t\tgit_path(SEQ_TODO_FILE), lineno, bol);\n+\tlist->next = NULL;\n+\treturn 0;\n }\n \n static int parse_insn_buffer(char *buf, struct commit_list **todo_list,\n \t\t\tstruct replay_opts *opts)\n {\n \tstruct commit_list **next = todo_list;\n-\tstruct commit *commit;\n+\tstruct commit_list list;\n \tchar *p = buf;\n \tint i;\n \n \tfor (i = 1; *p; i++) {\n \t\tchar *eol = strchrnul(p, '\\n');\n-\t\tcommit = parse_insn_line(p, eol, opts);\n-\t\tif (!commit)\n-\t\t\treturn error(_(\"Could not parse line %d.\"), i);\n-\t\tnext = commit_list_append(commit, next);\n+\t\tif (parse_insn_line(p, eol, opts, &list, i))\n+\t\t\treturn -1;\n+\t\tnext = commit_list_append(list.item, next);\n \t\tp = *eol ? eol + 1 : eol;\n \t}\n \tif (!*todo_list)\n-- \n1.7.8.1.362.g5d6df.dirty\n"},{"id":"188345","messageId":"7vr4w5euvg.fsf@alter.siamese.dyndns.org","threadId":"30096","inReplyTo":"1333029495-10034-4-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 3/4] revert: simplify insn parsing logic","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-02T20:33:55Z","receivedAt":"2012-04-02T20:33:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ramkumar Ramachandra writes:\n\n> Our instruction sheet parser first looks for a valid action by\n> checking that the buffer starts with either \"pick\" or \"revert\". Then,\n> it looks for either spaces or tabs before looking for the object name,\n> erroring out if it doesn't find any. Simplify this logic without\n> making any functional changes by looking for (\"pick \" or \"pick\\t\") or\n> (\"revert \" or \"revert\\t\") in the first place.\n>\n> Signed-off-by: Ramkumar Ramachandra \n\nHrm.\n\nIn the longer term, I think the right approach is to cut the first token\non the line, and then switch the action based on what the token is.\n\nThe reason why the original complains against \"pickle \" is\nnot because the headword \"pickle\" is an unknown verb, but because the\nrecognised token \"pick\" is followed by 'l' which is not a whitespace, and\nthe updated code may slightly be closer to the ideal, but not that much.\n\nPerhaps something like this?\n\nI suspect that this may be a bit over-engineered, but on the other hand,\nif we do not foresee that we would be adding many other verbs, I do not\nthink there is much point in your patch to clean up the verb parsing part,\neither, so...\n\n-- >8 --\n\nstatic struct {\n\tconst char *name;\n\tint len;\n\tenum replay_action action;\n} replay_action[] = {\n\t{ \"pick\", 4, REPLAY_PICK },\n\t{ \"revert\", 6, REPLAY_REVERT },\n};\n\nstatic enum replay_action parse_action(char **cp_, int *namelen_)\n{\n\tchar *cp = *cp_;\n int namelen = strcspn(cp, \"\\t \");\n\tint i, padlen;\n\n\tfor (i = 0; i < ARRAY_SIZE(replay_action); i++)\n \tif (namelen == replay_action[i].len &&\n !memcmp(cp, replay_action[i].name, namelen)) {\n\t\t\tif (namelen_)\n\t\t\t\t*namelen_ = namelen;\n\t\t\tpadlen = strspn(cp + namelen, \"\\t \");\n *cp_ = cp + namelen + padlen;\n\t\t\treturn replay_action[i].action;\n\t\t}\n\treturn -1;\n}\n\nstatic struct commit *parse_insn_line(char *bol, ...)\n{\n\t...\n\n\taction = parse_action(&bol, NULL);\n\t/*\n * Verify that the action matches ...\n"},{"id":"188382","messageId":"CALkWK0nMx84G9_FdqFKp1j30bfV7mjHyD5paSHU5EgcsitGSEA@mail.gmail.com","threadId":"30096","inReplyTo":"7vr4w5euvg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 3/4] revert: simplify insn parsing logic","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-04-03T04:50:06Z","receivedAt":"2012-04-03T04:50:06Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Junio,\n\nJunio C Hamano wrote:\n> I suspect that this may be a bit over-engineered, but on the other hand,\n> if we do not foresee that we would be adding many other verbs, I do not\n> think there is much point in your patch to clean up the verb parsing part,\n> either, so...\n\ninteresting; yes, it does look a little over-engineered for the\nmoment. I should have been clearer in the commit message: this patch\nexists mainly so that [4/4] can report a sensible error message\ninstead of a literal \"Missing space after valid verb\". So, we have\nfour options from here:\n1. Squash this part into [4/4]. Will this give rise to additional confusion?\n2. Use your version of the patch.\n3. Append the following to the commit message: \"This patch exists so\nthat the next patch can report a sensible error message when an\ninvalid verb is encountered\".\n4. If you don't think [4/4] is valuable yet, just apply the first two\nparts of this series.\n\nI'm personally in favor of (1) now.\n\nThanks.\n\n Ram\n"},{"id":"188444","messageId":"20120403220817.GE19858@burratino","threadId":"30096","inReplyTo":"1333029495-10034-2-git-send-email-artagnon@gmail.com","subject":"Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-03T22:08:18Z","receivedAt":"2012-04-03T22:08:18Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"(cc-ing Clément who is one of the last people to change this family of\n APIs, and Matthieu who knows these codepaths well and may have been a\n mentor for that project)\nHi,\n\nRamkumar Ramachandra wrote:\n\n> [Subject: sha1_name: introduce getn_sha1() to take length]\n\nYou're probably going to hate this, but oh well:\n\nI love the new function. I really dislike its name. Do we have any\nother (function that takes a C-style string/function that takes a\nbuffer with length) pairs in the spirit of strchr/memchr to draw\ninspiration from?\n\n> --- a/sha1_name.c\n> +++ b/sha1_name.c\n> @@ -1019,12 +1019,11 @@ static char *resolve_relative_path(const char *rel)\n> \t\t\t rel);\n> }\n> \n> -int get_sha1_with_context_1(const char *name, unsigned char *sha1,\n> -\t\t\t struct object_context *oc,\n> -\t\t\t int only_to_die, const char *prefix)\n> +static int getn_sha1_with_context_1(const char *name, int namelen,\n> +\t\t\t\tunsigned char *sha1, struct object_context *oc,\n> +\t\t\t\tint only_to_die, const char *prefix)\n\nHoly cow this function is going crazy. So let's take a survey of\nthe public functions in this family.\n\n get_sha1(name, sha1)::\n\n\tLooks up a sha1 expression like xyz^ and returns the\n\tcorresponding object name in the 20-byte buffer sha1.\n\tReturns -1 on failure.\n\n get_sha1_with_mode(name, sha1, mode)::\n\n\tLike get_sha1, but when name refers to a path within\n\ta tree, also returns the mode in *mode.\n\n get_sha1_with_context(name, sha1, context)::\n\n\tLikewise, but also returns the parsed \n\tpair. \n\n get_sha1_with_mode_1(name, sha1, mode, flag, prefix)::\n\n\tLike get_sha1_with_mode, but with some extra features:\n\n\t - flag indicates whether we are in the \"detailed diagnosis\"\n\t codepath and only calling this function for the\n\t side-effect of die()-ing with a meaningful message.\n\n\t - prefix indicates the cwd prefix for use in the \"did you\n\t mean?\" diagnostic.\n\n get_sha1_with_context_1(name, sah1, context, flag, prefix)::\n\n\tVariant in the same vein for get_sha1_with_context.\n\nPlus the corresponding variants with \"name, namelen\" pairs after your\npatch.\n\nMy first reaction is that the meaning of the _1 suffix is not going to\nbe obvious to newcomers. Any ideas for addressing that?\n\n\"get_sha1_with_context_1\" has no external callers so it could probably\nbe made private. So there could be a sequence of functions with\nincreasing detail:\n\n\tget_sha1(name, sha1)\n\tget_sha1_with_mode(name, sha1, mode)\n\tget_sha1_with_context(name, sha1, context)\n\tdie_sha1_not_in_index(name, sha1, context, prefix)\n\nThe \"len\" argument could be introduced at some convenient place in\nthis list to avoid having to change too many callers, for example:\n\n\tget_sha1(name, sha1)\n\tget_sha1_with_mode(name, sha1, mode)\n\tget_sha1_with_context(name, namelen, sha1, context)\n\tdie_sha1_not_in_index(name, namelen, sha1, context, prefix)\n\nOr maybe it makes sense to bite the bullet and add the length\nargument to all callers:\n\n\tget_sha1(name, namelen, sha1)\n\tget_sha1_with_mode(name, namelen, sha1, mode)\n\tget_sha1_with_context(name, namelen, sha1, context)\n\tdie_sha1_not_in_index(name, namelen, sha1, context, prefix)\n\nIf I were doing it, I might just ask the maintainer to make the\ndecision for me, by making a somewhat funny patch series:\n\n 1. add \"how to use the get_sha1 family\" to Documentation/technical.\n 2. make get_sha1_with_context_1 static.\n 3. replace get_sha1_with_mode_1 with simpler die_sha1_not_in_index\n function.\n 4. add namelen argument to die_sha1_not_... and\n get_sha1_with_context, adjust callers, and make use of\n get_sha1_with_context with this arg in sequencer code.\n 5. add namelen arg to get_sha1_with_mode, adjust callers, and\n make sequencer code use this instead.\n 6. add namelen arg to get_sha1, adjust callers, and make sequener\n code use this instead.\n\nThat way, the maintainer could take patches 1 - 3 to get the basic API\ncleanup, patch 4 to get the sequencer enhancement and make a judgement\ncall about whether to take patches 5 and 6 depending on how much of a\npain the churn would be given other patches in flight.\n\nWhat do you think?\nJonathan\n"},{"id":"188473","messageId":"vpqr4w40x1d.fsf@bauges.imag.fr","threadId":"30096","inReplyTo":"20120403220817.GE19858@burratino","subject":"Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length","fromName":"Matthieu Moy","fromEmail":"matthieu.moy@grenoble-inp.fr","sentAt":"2012-04-04T07:35:26Z","receivedAt":"2012-04-04T07:35:26Z","isPatch":true,"sender":{"key":"matthieu.moy@grenoble-inp.fr","avatar":"https://gravatar.com/avatar/72c8a2705971a25dfaff23cece15130d405685845d911aedd5667ace277f3fc5?d=mp&s=160"},"body":"Jonathan Nieder writes:\n\n> (cc-ing Clément who is one of the last people to change this family of\n> APIs, and Matthieu who knows these codepaths well and may have been a\n> mentor for that project)\n\nI wouldn't say I know them well, but I did touch them in the past.\n\n> Holy cow this function is going crazy. So let's take a survey of\n> the public functions in this family.\n>\n> [... nice explanations of different functions ...]\n\nGood job. I think your explanations could actually be added as comments\nto cache.h to document the corresponding functions.\n\n> My first reaction is that the meaning of the _1 suffix is not going to\n> be obvious to newcomers. Any ideas for addressing that?\n\nIt seems I'm the one who introduced get_sha1_with_context_1. I meant\n\"internal variant of the one without _1\", which is a convention used in\nother places of Git's code, but usually as static functions, not in .h\nfiles.\n\n> \"get_sha1_with_context_1\" has no external callers so it could probably\n> be made private.\n\nI kept it public as a very small implementation detail: I tried not to\nintroduce performance penalty, hence made get_sha1_with_mode inline\n(since it is really a trivial wrapper). But we probably wouldn't notice\nthe difference in performance making the _1 version private and losing\nthe \"static inline\"-ness of the public version.\n\nAnother way to say this is: get_sha1_with_context_1() does the real job\n(perhaps it should be called get_sha1_real()?), and others are\nconvenience wrappers. Since convenience wrappers are convenient, nobody\nuse the actual function directly.\n\n> Or maybe it makes sense to bite the bullet and add the length\n> argument to all callers:\n\nI don't think so. Convenience wrappers are meant to be simple to call,\nand I don't think we want to force everybody to call strlen() before\ncalling them.\n\n-- \nMatthieu Moy\nhttp://www-verimag.imag.fr/~moy/\n"},{"id":"188492","messageId":"CALkWK0nsO_xeuKpuPp0wvvbdhHnmKnX_JCPr6yapztC-Gq3Ljw@mail.gmail.com","threadId":"30096","inReplyTo":"20120403220817.GE19858@burratino","subject":"Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length","fromName":"Ramkumar Ramachandra","fromEmail":"artagnon@gmail.com","sentAt":"2012-04-04T15:40:16Z","receivedAt":"2012-04-04T15:40:16Z","isPatch":true,"sender":{"key":"r@artagnon.com","avatar":"https://avatars.githubusercontent.com/u/37226?v=4"},"body":"Hi Jonathan,\n\nJonathan Nieder wrote:\n> If I were doing it, I might just ask the maintainer to make the\n> decision for me, by making a somewhat funny patch series:\n>\n>  1. add \"how to use the get_sha1 family\" to Documentation/technical.\n>  2. make get_sha1_with_context_1 static.\n>  3. replace get_sha1_with_mode_1 with simpler die_sha1_not_in_index\n>    function.\n>  4. add namelen argument to die_sha1_not_... and\n>    get_sha1_with_context, adjust callers, and make use of\n>    get_sha1_with_context with this arg in sequencer code.\n>  5. add namelen arg to get_sha1_with_mode, adjust callers, and\n>    make sequencer code use this instead.\n>  6. add namelen arg to get_sha1, adjust callers, and make sequener\n>    code use this instead.\n>\n> That way, the maintainer could take patches 1 - 3 to get the basic API\n> cleanup, patch 4 to get the sequencer enhancement and make a judgement\n> call about whether to take patches 5 and 6 depending on how much of a\n> pain the churn would be given other patches in flight.\n\nMakes a lot of sense. I'll re-roll shortly.\n\nThanks.\n\n Ram\n"},{"id":"188528","messageId":"20120404205321.GA17985@burratino","threadId":"30096","inReplyTo":"CALkWK0nsO_xeuKpuPp0wvvbdhHnmKnX_JCPr6yapztC-Gq3Ljw@mail.gmail.com","subject":"Re: [PATCH 1/4] sha1_name: introduce getn_sha1() to take length","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-04-04T20:53:21Z","receivedAt":"2012-04-04T20:53:21Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Ramkumar Ramachandra wrote:\n\n> Makes a lot of sense.\n\nOk, but please keep in mind Matthieu's advice about keeping\nconvenience wrappers convenient as well. I trust him more in these\nquestions.\n\nThanks.\nJonathan\n"}]}