{"thread":{"id":"47857","subject":"[PATCH 1/3] grep: move grep_source_init outside critical section","startedAt":"2018-02-15T21:56:34Z","lastAt":"2018-02-23T18:01:15Z","messageCount":12,"participants":["Rasmus Villemoes","Brandon Williams","Jeff King","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"339511","messageId":"20180215215615.21208-2-rv@rasmusvillemoes.dk","threadId":"47857","inReplyTo":"20180215215615.21208-1-rv@rasmusvillemoes.dk","subject":"[PATCH 1/3] grep: move grep_source_init outside critical section","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-02-15T21:56:13Z","receivedAt":"2018-02-15T21:56:34Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"grep_source_init typically does three strdup()s, and in the threaded\ncase, the call from add_work() happens while holding grep_mutex.\n\nWe can thus reduce the time we hold grep_mutex by moving the\ngrep_source_init() call out of add_work(), and simply have add_work()\ncopy the initialized structure to the available slot in the todo\narray.\n\nThis also simplifies the prototype of add_work(), since it no longer\nneeds to duplicate all the parameters of grep_source_init(). In the\ncallers of add_work(), we get to reduce the amount of code duplicated in\nthe threaded and non-threaded cases slightly (avoiding repeating the\n\"GREP_SOURCE_OID, pathbuf.buf, path, oid\" argument list); a subsequent\ncleanup patch will make that even more so.\n\nSigned-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n---\n builtin/grep.c | 19 ++++++++++---------\n 1 file changed, 10 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3ca4ac80d..4a4f15172 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -92,8 +92,7 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n-static void add_work(struct grep_opt *opt, enum grep_source_type type,\n-\t\t     const char *name, const char *path, const void *id)\n+static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n {\n \tgrep_lock();\n \n@@ -101,7 +100,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n \t\tpthread_cond_wait(&cond_write, &grep_mutex);\n \t}\n \n-\tgrep_source_init(&todo[todo_end].source, type, name, path, id);\n+\ttodo[todo_end].source = *gs;\n \tif (opt->binary != GREP_BINARY_TEXT)\n \t\tgrep_source_load_driver(&todo[todo_end].source);\n \ttodo[todo_end].done = 0;\n@@ -317,6 +316,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t\t     const char *path)\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n+\tstruct grep_source gs;\n \n \tif (opt->relative && opt->prefix_length) {\n \t\tquote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);\n@@ -325,18 +325,18 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n+\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\t\tadd_work(opt, &gs);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n-\t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n \t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -348,24 +348,25 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n static int grep_file(struct grep_opt *opt, const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct grep_source gs;\n \n \tif (opt->relative && opt->prefix_length)\n \t\tquote_path_relative(filename, opt->prefix, &buf);\n \telse\n \t\tstrbuf_addstr(&buf, filename);\n \n+\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\t\tadd_work(opt, &gs);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n-\t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \n-- \n2.15.1\n\n"},{"id":"339512","messageId":"20180215215615.21208-4-rv@rasmusvillemoes.dk","threadId":"47857","inReplyTo":"20180215215615.21208-1-rv@rasmusvillemoes.dk","subject":"[PATCH 3/3] grep: avoid one strdup() per file","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-02-15T21:56:15Z","receivedAt":"2018-02-15T21:56:37Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"There is only one instance of grep_source_init(GREP_SOURCE_FILE), and in\nthat case the path and identifier arguments are equal - not just as\nstrings, but the same pointer is passed. So we can save some time and\nmemory by reusing the gs->path = xstrdup_or_null(path) we have already\ndone as gs->identifier, and changing grep_source_clear accordingly\nto avoid a double free.\n\nSigned-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n---\n grep.c | 8 ++++++--\n 1 file changed, 6 insertions(+), 2 deletions(-)\n\ndiff --git a/grep.c b/grep.c\nindex 3d7cd0e96..b1532b1b6 100644\n--- a/grep.c\n+++ b/grep.c\n@@ -1972,7 +1972,8 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n \n \tswitch (type) {\n \tcase GREP_SOURCE_FILE:\n-\t\tgs->identifier = xstrdup(identifier);\n+\t\tgs->identifier = identifier == path ?\n+\t\t\tgs->path : xstrdup(identifier);\n \t\tbreak;\n \tcase GREP_SOURCE_OID:\n \t\tgs->identifier = oiddup(identifier);\n@@ -1986,7 +1987,10 @@ void grep_source_init(struct grep_source *gs, enum grep_source_type type,\n void grep_source_clear(struct grep_source *gs)\n {\n \tFREE_AND_NULL(gs->name);\n-\tFREE_AND_NULL(gs->path);\n+\tif (gs->path == gs->identifier)\n+\t\tgs->path = NULL;\n+\telse\n+\t\tFREE_AND_NULL(gs->path);\n \tFREE_AND_NULL(gs->identifier);\n \tgrep_source_clear_data(gs);\n }\n-- \n2.15.1\n\n"},{"id":"339513","messageId":"20180215215615.21208-3-rv@rasmusvillemoes.dk","threadId":"47857","inReplyTo":"20180215215615.21208-1-rv@rasmusvillemoes.dk","subject":"[PATCH 2/3] grep: simplify grep_oid and grep_file","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-02-15T21:56:14Z","receivedAt":"2018-02-15T21:56:42Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"In the NO_PTHREADS or !num_threads case, this doesn't change\nanything. In the threaded case, note that grep_source_init duplicates\nits third argument, so there is no need to keep [path]buf.buf alive\nacross the call of add_work().\n\nSigned-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n---\n builtin/grep.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 4a4f15172..593f48d59 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -326,18 +326,17 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t}\n \n \tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\tstrbuf_release(&pathbuf);\n \n #ifndef NO_PTHREADS\n \tif (num_threads) {\n \t\tadd_work(opt, &gs);\n-\t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n \t\tint hit;\n \n-\t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n \t\tgrep_source_clear(&gs);\n@@ -356,18 +355,17 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstrbuf_addstr(&buf, filename);\n \n \tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\tstrbuf_release(&buf);\n \n #ifndef NO_PTHREADS\n \tif (num_threads) {\n \t\tadd_work(opt, &gs);\n-\t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n \t\tint hit;\n \n-\t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \n \t\tgrep_source_clear(&gs);\n-- \n2.15.1\n\n"},{"id":"339514","messageId":"20180215215615.21208-1-rv@rasmusvillemoes.dk","threadId":"47857","inReplyTo":null,"subject":"[PATCH 0/3] a few grep patches","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-02-15T21:56:12Z","receivedAt":"2018-02-15T21:56:52Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"I believe the first two should be ok, but I'm not sure what I myself\nthink of the third one. Perhaps the saving is not worth the\ncomplexity, but it does annoy my optimization nerve to see all the\nunnecessary duplicated work being done.\n\nRasmus Villemoes (3):\n  grep: move grep_source_init outside critical section\n  grep: simplify grep_oid and grep_file\n  grep: avoid one strdup() per file\n\n builtin/grep.c | 25 ++++++++++++-------------\n grep.c         |  8 ++++++--\n 2 files changed, 18 insertions(+), 15 deletions(-)\n\n-- \n2.15.1\n\n"},{"id":"339515","messageId":"20180215220212.GA123347@google.com","threadId":"47857","inReplyTo":"20180215215615.21208-1-rv@rasmusvillemoes.dk","subject":"Re: [PATCH 0/3] a few grep patches","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2018-02-15T22:02:12Z","receivedAt":"2018-02-15T22:02:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/15, Rasmus Villemoes wrote:\n> I believe the first two should be ok, but I'm not sure what I myself\n> think of the third one. Perhaps the saving is not worth the\n> complexity, but it does annoy my optimization nerve to see all the\n> unnecessary duplicated work being done.\n\nI agree, the first two seem like good changes to me though I don't know\nif i like the complexity that the third introduces.\n\n> \n> Rasmus Villemoes (3):\n>   grep: move grep_source_init outside critical section\n>   grep: simplify grep_oid and grep_file\n>   grep: avoid one strdup() per file\n> \n>  builtin/grep.c | 25 ++++++++++++-------------\n>  grep.c         |  8 ++++++--\n>  2 files changed, 18 insertions(+), 15 deletions(-)\n> \n> -- \n> 2.15.1\n> \n\n-- \nBrandon Williams\n"},{"id":"339518","messageId":"20180215221713.GB23970@sigill.intra.peff.net","threadId":"47857","inReplyTo":"20180215215615.21208-2-rv@rasmusvillemoes.dk","subject":"Re: [PATCH 1/3] grep: move grep_source_init outside critical section","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T22:17:13Z","receivedAt":"2018-02-15T22:17:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 15, 2018 at 10:56:13PM +0100, Rasmus Villemoes wrote:\n\n> grep_source_init typically does three strdup()s, and in the threaded\n> case, the call from add_work() happens while holding grep_mutex.\n> \n> We can thus reduce the time we hold grep_mutex by moving the\n> grep_source_init() call out of add_work(), and simply have add_work()\n> copy the initialized structure to the available slot in the todo\n> array.\n> \n> This also simplifies the prototype of add_work(), since it no longer\n> needs to duplicate all the parameters of grep_source_init(). In the\n> callers of add_work(), we get to reduce the amount of code duplicated in\n> the threaded and non-threaded cases slightly (avoiding repeating the\n> \"GREP_SOURCE_OID, pathbuf.buf, path, oid\" argument list); a subsequent\n> cleanup patch will make that even more so.\n\nI think this makes sense. It does blur the memory ownership lines of the\ngrep_source, though. Can we make that more clear with a comment here:\n\n> +\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n> +\n>  #ifndef NO_PTHREADS\n>  \tif (num_threads) {\n> -\t\tadd_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n> +\t\tadd_work(opt, &gs);\n>  \t\tstrbuf_release(&pathbuf);\n>  \t\treturn 0;\n>  \t} else\n\nlike:\n\n  /* leak grep_source, whose fields are now owned by add_work() */\n\nor something? We could even memset() it back to all-zeroes to avoid an\naccidental call to grep_source_clear(), but that's probably unnecessary\nif we have a comment.\n\n-Peff\n"},{"id":"339519","messageId":"20180215221837.GC23970@sigill.intra.peff.net","threadId":"47857","inReplyTo":"20180215215615.21208-4-rv@rasmusvillemoes.dk","subject":"Re: [PATCH 3/3] grep: avoid one strdup() per file","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-15T22:18:37Z","receivedAt":"2018-02-15T22:18:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Feb 15, 2018 at 10:56:15PM +0100, Rasmus Villemoes wrote:\n\n> There is only one instance of grep_source_init(GREP_SOURCE_FILE), and in\n> that case the path and identifier arguments are equal - not just as\n> strings, but the same pointer is passed. So we can save some time and\n> memory by reusing the gs->path = xstrdup_or_null(path) we have already\n> done as gs->identifier, and changing grep_source_clear accordingly\n> to avoid a double free.\n\nIMHO this special case is not really worth it, unless we can show that\nthose few bytes saved somehow make a measurable difference in either\npeak memory or execution speed.\n\n-Peff\n"},{"id":"339571","messageId":"xmqqsha068l2.fsf@gitster-ct.c.googlers.com","threadId":"47857","inReplyTo":"20180215221713.GB23970@sigill.intra.peff.net","subject":"Re: [PATCH 1/3] grep: move grep_source_init outside critical section","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2018-02-16T19:24:57Z","receivedAt":"2018-02-16T19:25:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> I think this makes sense. It does blur the memory ownership lines of the\n> grep_source, though. Can we make that more clear with a comment here:\n>\n>> +\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n>> +\n>>  #ifndef NO_PTHREADS\n>>  \tif (num_threads) {\n>> -\t\tadd_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n>> +\t\tadd_work(opt, &gs);\n>>  \t\tstrbuf_release(&pathbuf);\n>>  \t\treturn 0;\n>>  \t} else\n>\n> like:\n>\n>   /* leak grep_source, whose fields are now owned by add_work() */\n>\n> or something? We could even memset() it back to all-zeroes to avoid an\n> accidental call to grep_source_clear(), but that's probably unnecessary\n> if we have a comment.\n\nI share the same uneasiness about the fuzzy memory ownership this\nchange brings in.  Thanks for suggesting improvements.\n"},{"id":"340034","messageId":"20180223144757.31875-1-rv@rasmusvillemoes.dk","threadId":"47857","inReplyTo":"20180215215615.21208-1-rv@rasmusvillemoes.dk","subject":"[PATCH v2 0/2] two small grep patches","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-02-23T14:47:55Z","receivedAt":"2018-02-23T14:48:09Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"Changes in v2:\n\n- Drop patch 3 with dubious gain/complexity ratio\n- Add comments regarding ownership of grep_source\n\nI was a little torn between copy-pasting the comment or just saying\n\"see above\" in the second case. I think a memset would be confusing,\nat least unless one extends the comment to explain why one then does\nthe memset despite the first half of the comment.\n\nRasmus Villemoes (2):\n  grep: move grep_source_init outside critical section\n  grep: simplify grep_oid and grep_file\n\n builtin/grep.c | 33 ++++++++++++++++++++-------------\n 1 file changed, 20 insertions(+), 13 deletions(-)\n\n-- \n2.15.1\n\n"},{"id":"340035","messageId":"20180223144757.31875-3-rv@rasmusvillemoes.dk","threadId":"47857","inReplyTo":"20180223144757.31875-1-rv@rasmusvillemoes.dk","subject":"[PATCH v2 2/2] grep: simplify grep_oid and grep_file","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-02-23T14:47:57Z","receivedAt":"2018-02-23T14:48:12Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"In the NO_PTHREADS or !num_threads case, this doesn't change\nanything. In the threaded case, note that grep_source_init duplicates\nits third argument, so there is no need to keep [path]buf.buf alive\nacross the call of add_work().\n\nSigned-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n---\n builtin/grep.c | 6 ++----\n 1 file changed, 2 insertions(+), 4 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex aad422bb6..9a8e4fada 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -326,6 +326,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t}\n \n \tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\tstrbuf_release(&pathbuf);\n \n #ifndef NO_PTHREADS\n \tif (num_threads) {\n@@ -334,14 +335,12 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t\t * its fields, so do not call grep_source_clear()\n \t\t */\n \t\tadd_work(opt, &gs);\n-\t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n \t\tint hit;\n \n-\t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n \t\tgrep_source_clear(&gs);\n@@ -360,6 +359,7 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\tstrbuf_addstr(&buf, filename);\n \n \tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\tstrbuf_release(&buf);\n \n #ifndef NO_PTHREADS\n \tif (num_threads) {\n@@ -368,14 +368,12 @@ static int grep_file(struct grep_opt *opt, const char *filename)\n \t\t * its fields, so do not call grep_source_clear()\n \t\t */\n \t\tadd_work(opt, &gs);\n-\t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n \t\tint hit;\n \n-\t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \n \t\tgrep_source_clear(&gs);\n-- \n2.15.1\n\n"},{"id":"340036","messageId":"20180223144757.31875-2-rv@rasmusvillemoes.dk","threadId":"47857","inReplyTo":"20180223144757.31875-1-rv@rasmusvillemoes.dk","subject":"[PATCH v2 1/2] grep: move grep_source_init outside critical section","fromName":"Rasmus Villemoes","fromEmail":"rv@rasmusvillemoes.dk","sentAt":"2018-02-23T14:47:56Z","receivedAt":"2018-02-23T14:48:14Z","isPatch":true,"sender":{"key":"rv@rasmusvillemoes.dk","avatar":"https://avatars.githubusercontent.com/u/4375908?v=4"},"body":"grep_source_init typically does three strdup()s, and in the threaded\ncase, the call from add_work() happens while holding grep_mutex.\n\nWe can thus reduce the time we hold grep_mutex by moving the\ngrep_source_init() call out of add_work(), and simply have add_work()\ncopy the initialized structure to the available slot in the todo\narray.\n\nThis also simplifies the prototype of add_work(), since it no longer\nneeds to duplicate all the parameters of grep_source_init(). In the\ncallers of add_work(), we get to reduce the amount of code duplicated in\nthe threaded and non-threaded cases slightly (avoiding repeating the\nlong \"GREP_SOURCE_OID, pathbuf.buf, path, oid\" argument list); a\nsubsequent cleanup patch will make that even more so.\n\nSigned-off-by: Rasmus Villemoes <rv@rasmusvillemoes.dk>\n---\n builtin/grep.c | 27 ++++++++++++++++++---------\n 1 file changed, 18 insertions(+), 9 deletions(-)\n\ndiff --git a/builtin/grep.c b/builtin/grep.c\nindex 3ca4ac80d..aad422bb6 100644\n--- a/builtin/grep.c\n+++ b/builtin/grep.c\n@@ -92,8 +92,7 @@ static pthread_cond_t cond_result;\n \n static int skip_first_line;\n \n-static void add_work(struct grep_opt *opt, enum grep_source_type type,\n-\t\t     const char *name, const char *path, const void *id)\n+static void add_work(struct grep_opt *opt, const struct grep_source *gs)\n {\n \tgrep_lock();\n \n@@ -101,7 +100,7 @@ static void add_work(struct grep_opt *opt, enum grep_source_type type,\n \t\tpthread_cond_wait(&cond_write, &grep_mutex);\n \t}\n \n-\tgrep_source_init(&todo[todo_end].source, type, name, path, id);\n+\ttodo[todo_end].source = *gs;\n \tif (opt->binary != GREP_BINARY_TEXT)\n \t\tgrep_source_load_driver(&todo[todo_end].source);\n \ttodo[todo_end].done = 0;\n@@ -317,6 +316,7 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t\t     const char *path)\n {\n \tstruct strbuf pathbuf = STRBUF_INIT;\n+\tstruct grep_source gs;\n \n \tif (opt->relative && opt->prefix_length) {\n \t\tquote_path_relative(filename + tree_name_len, opt->prefix, &pathbuf);\n@@ -325,18 +325,22 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n \t\tstrbuf_addstr(&pathbuf, filename);\n \t}\n \n+\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n+\t\t/*\n+\t\t * add_work() copies gs and thus assumes ownership of\n+\t\t * its fields, so do not call grep_source_clear()\n+\t\t */\n+\t\tadd_work(opt, &gs);\n \t\tstrbuf_release(&pathbuf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n-\t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_OID, pathbuf.buf, path, oid);\n \t\tstrbuf_release(&pathbuf);\n \t\thit = grep_source(opt, &gs);\n \n@@ -348,24 +352,29 @@ static int grep_oid(struct grep_opt *opt, const struct object_id *oid,\n static int grep_file(struct grep_opt *opt, const char *filename)\n {\n \tstruct strbuf buf = STRBUF_INIT;\n+\tstruct grep_source gs;\n \n \tif (opt->relative && opt->prefix_length)\n \t\tquote_path_relative(filename, opt->prefix, &buf);\n \telse\n \t\tstrbuf_addstr(&buf, filename);\n \n+\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\n #ifndef NO_PTHREADS\n \tif (num_threads) {\n-\t\tadd_work(opt, GREP_SOURCE_FILE, buf.buf, filename, filename);\n+\t\t/*\n+\t\t * add_work() copies gs and thus assumes ownership of\n+\t\t * its fields, so do not call grep_source_clear()\n+\t\t */\n+\t\tadd_work(opt, &gs);\n \t\tstrbuf_release(&buf);\n \t\treturn 0;\n \t} else\n #endif\n \t{\n-\t\tstruct grep_source gs;\n \t\tint hit;\n \n-\t\tgrep_source_init(&gs, GREP_SOURCE_FILE, buf.buf, filename, filename);\n \t\tstrbuf_release(&buf);\n \t\thit = grep_source(opt, &gs);\n \n-- \n2.15.1\n\n"},{"id":"340050","messageId":"20180223180109.GA5208@sigill.intra.peff.net","threadId":"47857","inReplyTo":"20180223144757.31875-1-rv@rasmusvillemoes.dk","subject":"Re: [PATCH v2 0/2] two small grep patches","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2018-02-23T18:01:09Z","receivedAt":"2018-02-23T18:01:15Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Feb 23, 2018 at 03:47:55PM +0100, Rasmus Villemoes wrote:\n\n> Changes in v2:\n> \n> - Drop patch 3 with dubious gain/complexity ratio\n> - Add comments regarding ownership of grep_source\n> \n> I was a little torn between copy-pasting the comment or just saying\n> \"see above\" in the second case. I think a memset would be confusing,\n> at least unless one extends the comment to explain why one then does\n> the memset despite the first half of the comment.\n\nThis looks good to me. Thanks for following up.\n\n-Peff\n"}]}