[PATCH 0/3] ls-tree fixes

[PATCH 0/3] ls-tree fixes

[Nguyễn Thái Ngọc Duy]

The first two fix ls-tree's unable to handle relative paths outside
$PWD. The last one rejects negative pathspec. This is a resend from

http://thread.gmane.org/gmane.comp.version-control.git/259233/focus=259264

Nguyễn Thái Ngọc Duy (3):
  tree.c: update read_tree_recursive callback to pass strbuf as base
  ls-tree: remove path filtering logic in show_tree
  ls-tree: disable negative pathspec because it's not supported

 archive.c                    | 34 +++++++++++++++++++++-------------
 builtin/checkout.c           |  8 ++++----
 builtin/log.c                |  2 +-
 builtin/ls-tree.c            | 20 +++++++++++---------
 merge-recursive.c            | 15 ++++++---------
 quote.c                      | 21 ---------------------
 quote.h                      |  2 --
 t/t3102-ls-tree-wildcards.sh |  8 ++++++++
 tree.c                       | 16 +++++++++++-----
 tree.h                       |  3 ++-
 10 files changed, 64 insertions(+), 65 deletions(-)

-- 
2.2.0.60.gb7b3c64

[PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base

[Nguyễn Thái Ngọc Duy]

This allows the callback to use 'base' as a temporary buffer to
quickly assemble full path "without" extra allocation. The callback
has to restore it afterwards of course.

Helped-by: Eric Sunshine <sunshine@sunshineco.com>
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 archive.c          | 34 +++++++++++++++++++++-------------
 builtin/checkout.c |  8 ++++----
 builtin/log.c      |  2 +-
 builtin/ls-tree.c  |  9 +++++----
 merge-recursive.c  | 15 ++++++---------
 tree.c             | 16 +++++++++++-----
 tree.h             |  3 ++-
 7 files changed, 50 insertions(+), 37 deletions(-)

diff --git a/archive.c b/archive.c
index 94a9981..9e30246 100644
--- a/archive.c
+++ b/archive.c
@@ -157,18 +157,26 @@ static int write_archive_entry(const unsigned char *sha1, const char *base,
 	return write_entry(args, sha1, path.buf, path.len, mode);
 }
 
+static int write_archive_entry_buf(const unsigned char *sha1, struct strbuf *base,
+		const char *filename, unsigned mode, int stage,
+		void *context)
+{
+	return write_archive_entry(sha1, base->buf, base->len,
+				     filename, mode, stage, context);
+}
+
 static void queue_directory(const unsigned char *sha1,
-		const char *base, int baselen, const char *filename,
+		struct strbuf *base, const char *filename,
 		unsigned mode, int stage, struct archiver_context *c)
 {
 	struct directory *d;
-	d = xmallocz(sizeof(*d) + baselen + 1 + strlen(filename));
+	d = xmallocz(sizeof(*d) + base->len + 1 + strlen(filename));
 	d->up	   = c->bottom;
-	d->baselen = baselen;
+	d->baselen = base->len;
 	d->mode	   = mode;
 	d->stage   = stage;
 	c->bottom  = d;
-	d->len = sprintf(d->path, "%.*s%s/", baselen, base, filename);
+	d->len = sprintf(d->path, "%.*s%s/", (int)base->len, base->buf, filename);
 	hashcpy(d->sha1, sha1);
 }
 
@@ -191,28 +199,28 @@ static int write_directory(struct archiver_context *c)
 }
 
 static int queue_or_write_archive_entry(const unsigned char *sha1,
-		const char *base, int baselen, const char *filename,
+		struct strbuf *base, const char *filename,
 		unsigned mode, int stage, void *context)
 {
 	struct archiver_context *c = context;
 
 	while (c->bottom &&
-	       !(baselen >= c->bottom->len &&
-		 !strncmp(base, c->bottom->path, c->bottom->len))) {
+	       !(base->len >= c->bottom->len &&
+		 !strncmp(base->buf, c->bottom->path, c->bottom->len))) {
 		struct directory *next = c->bottom->up;
 		free(c->bottom);
 		c->bottom = next;
 	}
 
 	if (S_ISDIR(mode)) {
-		queue_directory(sha1, base, baselen, filename,
+		queue_directory(sha1, base, filename,
 				mode, stage, c);
 		return READ_TREE_RECURSIVE;
 	}
 
 	if (write_directory(c))
 		return -1;
-	return write_archive_entry(sha1, base, baselen, filename, mode,
+	return write_archive_entry(sha1, base->buf, base->len, filename, mode,
 				   stage, context);
 }
 
@@ -260,7 +268,7 @@ int write_archive_entries(struct archiver_args *args,
 	err = read_tree_recursive(args->tree, "", 0, 0, &args->pathspec,
 				  args->pathspec.has_wildcard ?
 				  queue_or_write_archive_entry :
-				  write_archive_entry,
+				  write_archive_entry_buf,
 				  &context);
 	if (err == READ_TREE_RECURSIVE)
 		err = 0;
@@ -286,14 +294,14 @@ static const struct archiver *lookup_archiver(const char *name)
 	return NULL;
 }
 
-static int reject_entry(const unsigned char *sha1, const char *base,
-			int baselen, const char *filename, unsigned mode,
+static int reject_entry(const unsigned char *sha1, struct strbuf *base,
+			const char *filename, unsigned mode,
 			int stage, void *context)
 {
 	int ret = -1;
 	if (S_ISDIR(mode)) {
 		struct strbuf sb = STRBUF_INIT;
-		strbuf_addstr(&sb, base);
+		strbuf_addbuf(&sb, base);
 		strbuf_addstr(&sb, filename);
 		if (!match_pathspec(context, sb.buf, sb.len, 0, NULL, 1))
 			ret = READ_TREE_RECURSIVE;
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 5410dac..8adf48d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -62,7 +62,7 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
 
 }
 
-static int update_some(const unsigned char *sha1, const char *base, int baselen,
+static int update_some(const unsigned char *sha1, struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
 	int len;
@@ -71,11 +71,11 @@ static int update_some(const unsigned char *sha1, const char *base, int baselen,
 	if (S_ISDIR(mode))
 		return READ_TREE_RECURSIVE;
 
-	len = baselen + strlen(pathname);
+	len = base->len + strlen(pathname);
 	ce = xcalloc(1, cache_entry_size(len));
 	hashcpy(ce->sha1, sha1);
-	memcpy(ce->name, base, baselen);
-	memcpy(ce->name + baselen, pathname, len - baselen);
+	memcpy(ce->name, base->buf, base->len);
+	memcpy(ce->name + base->len, pathname, len - base->len);
 	ce->ce_flags = create_ce_flags(0) | CE_UPDATE;
 	ce->ce_namelen = len;
 	ce->ce_mode = create_ce_mode(mode);
diff --git a/builtin/log.c b/builtin/log.c
index 734aab3..f2a9f01 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -489,7 +489,7 @@ static int show_tag_object(const unsigned char *sha1, struct rev_info *rev)
 }
 
 static int show_tree_object(const unsigned char *sha1,
-		const char *base, int baselen,
+		struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
 	printf("%s%s\n", pathname, S_ISDIR(mode) ? "/" : "");
diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 51184df..1ab0381 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -61,7 +61,7 @@ static int show_recursive(const char *base, int baselen, const char *pathname)
 	}
 }
 
-static int show_tree(const unsigned char *sha1, const char *base, int baselen,
+static int show_tree(const unsigned char *sha1, struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
 	int retval = 0;
@@ -79,7 +79,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,
 		 */
 		type = commit_type;
 	} else if (S_ISDIR(mode)) {
-		if (show_recursive(base, baselen, pathname)) {
+		if (show_recursive(base->buf, base->len, pathname)) {
 			retval = READ_TREE_RECURSIVE;
 			if (!(ls_options & LS_SHOW_TREES))
 				return retval;
@@ -90,7 +90,8 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,
 		return 0;
 
 	if (chomp_prefix &&
-	    (baselen < chomp_prefix || memcmp(ls_tree_prefix, base, chomp_prefix)))
+	    (base->len < chomp_prefix ||
+	     memcmp(ls_tree_prefix, base->buf, chomp_prefix)))
 		return 0;
 
 	if (!(ls_options & LS_NAME_ONLY)) {
@@ -112,7 +113,7 @@ static int show_tree(const unsigned char *sha1, const char *base, int baselen,
 			printf("%06o %s %s\t", mode, type,
 			       find_unique_abbrev(sha1, abbrev));
 	}
-	write_name_quotedpfx(base + chomp_prefix, baselen - chomp_prefix,
+	write_name_quotedpfx(base->buf + chomp_prefix, base->len - chomp_prefix,
 			  pathname, stdout, line_termination);
 	return retval;
 }
diff --git a/merge-recursive.c b/merge-recursive.c
index fdb7d0f..25c067e 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -275,23 +275,20 @@ struct tree *write_tree_from_memory(struct merge_options *o)
 }
 
 static int save_files_dirs(const unsigned char *sha1,
-		const char *base, int baselen, const char *path,
+		struct strbuf *base, const char *path,
 		unsigned int mode, int stage, void *context)
 {
-	int len = strlen(path);
-	char *newpath = xmalloc(baselen + len + 1);
+	int baselen = base->len;
 	struct merge_options *o = context;
 
-	memcpy(newpath, base, baselen);
-	memcpy(newpath + baselen, path, len);
-	newpath[baselen + len] = '\0';
+	strbuf_addstr(base, path);
 
 	if (S_ISDIR(mode))
-		string_list_insert(&o->current_directory_set, newpath);
+		string_list_insert(&o->current_directory_set, base->buf);
 	else
-		string_list_insert(&o->current_file_set, newpath);
-	free(newpath);
+		string_list_insert(&o->current_file_set, base->buf);
 
+	strbuf_setlen(base, baselen);
 	return (S_ISDIR(mode) ? READ_TREE_RECURSIVE : 0);
 }
 
diff --git a/tree.c b/tree.c
index bb02c1c..58ebfce 100644
--- a/tree.c
+++ b/tree.c
@@ -30,9 +30,12 @@ static int read_one_entry_opt(const unsigned char *sha1, const char *base, int b
 	return add_cache_entry(ce, opt);
 }
 
-static int read_one_entry(const unsigned char *sha1, const char *base, int baselen, const char *pathname, unsigned mode, int stage, void *context)
+static int read_one_entry(const unsigned char *sha1, struct strbuf *base,
+			  const char *pathname, unsigned mode, int stage,
+			  void *context)
 {
-	return read_one_entry_opt(sha1, base, baselen, pathname, mode, stage,
+	return read_one_entry_opt(sha1, base->buf, base->len, pathname,
+				  mode, stage,
 				  ADD_CACHE_OK_TO_ADD|ADD_CACHE_SKIP_DFCHECK);
 }
 
@@ -40,9 +43,12 @@ static int read_one_entry(const unsigned char *sha1, const char *base, int basel
  * This is used when the caller knows there is no existing entries at
  * the stage that will conflict with the entry being added.
  */
-static int read_one_entry_quick(const unsigned char *sha1, const char *base, int baselen, const char *pathname, unsigned mode, int stage, void *context)
+static int read_one_entry_quick(const unsigned char *sha1, struct strbuf *base,
+				const char *pathname, unsigned mode, int stage,
+				void *context)
 {
-	return read_one_entry_opt(sha1, base, baselen, pathname, mode, stage,
+	return read_one_entry_opt(sha1, base->buf, base->len, pathname,
+				  mode, stage,
 				  ADD_CACHE_JUST_APPEND);
 }
 
@@ -70,7 +76,7 @@ static int read_tree_1(struct tree *tree, struct strbuf *base,
 				continue;
 		}
 
-		switch (fn(entry.sha1, base->buf, base->len,
+		switch (fn(entry.sha1, base,
 			   entry.path, entry.mode, stage, context)) {
 		case 0:
 			continue;
diff --git a/tree.h b/tree.h
index d84ac63..d24125f 100644
--- a/tree.h
+++ b/tree.h
@@ -4,6 +4,7 @@
 #include "object.h"
 
 extern const char *tree_type;
+struct strbuf;
 
 struct tree {
 	struct object object;
@@ -22,7 +23,7 @@ void free_tree_buffer(struct tree *tree);
 struct tree *parse_tree_indirect(const unsigned char *sha1);
 
 #define READ_TREE_RECURSIVE 1
-typedef int (*read_tree_fn_t)(const unsigned char *, const char *, int, const char *, unsigned int, int, void *);
+typedef int (*read_tree_fn_t)(const unsigned char *, struct strbuf *, const char *, unsigned int, int, void *);
 
 extern int read_tree_recursive(struct tree *tree,
 			       const char *base, int baselen,
-- 
2.2.0.60.gb7b3c64

[PATCH 2/3] ls-tree: remove path filtering logic in show_tree

[Nguyễn Thái Ngọc Duy]

ls-tree uses read_tree_recursive() which already does path filtering
using pathspec. No need to filter one more time based on prefix
only. "ls-tree ../somewhere" does not work because of
this. write_name_quotedpfx() can now be retired because nobody else
uses it.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/ls-tree.c            | 14 +++++++-------
 quote.c                      | 21 ---------------------
 quote.h                      |  2 --
 t/t3102-ls-tree-wildcards.sh |  8 ++++++++
 4 files changed, 15 insertions(+), 30 deletions(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 1ab0381..053edb2 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -65,6 +65,7 @@ static int show_tree(const unsigned char *sha1, struct strbuf *base,
 		const char *pathname, unsigned mode, int stage, void *context)
 {
 	int retval = 0;
+	int baselen;
 	const char *type = blob_type;
 
 	if (S_ISGITLINK(mode)) {
@@ -89,11 +90,6 @@ static int show_tree(const unsigned char *sha1, struct strbuf *base,
 	else if (ls_options & LS_TREE_ONLY)
 		return 0;
 
-	if (chomp_prefix &&
-	    (base->len < chomp_prefix ||
-	     memcmp(ls_tree_prefix, base->buf, chomp_prefix)))
-		return 0;
-
 	if (!(ls_options & LS_NAME_ONLY)) {
 		if (ls_options & LS_SHOW_SIZE) {
 			char size_text[24];
@@ -113,8 +109,12 @@ static int show_tree(const unsigned char *sha1, struct strbuf *base,
 			printf("%06o %s %s\t", mode, type,
 			       find_unique_abbrev(sha1, abbrev));
 	}
-	write_name_quotedpfx(base->buf + chomp_prefix, base->len - chomp_prefix,
-			  pathname, stdout, line_termination);
+	baselen = base->len;
+	strbuf_addstr(base, pathname);
+	write_name_quoted_relative(base->buf,
+				   chomp_prefix ? ls_tree_prefix : NULL,
+				   stdout, line_termination);
+	strbuf_setlen(base, baselen);
 	return retval;
 }
 
diff --git a/quote.c b/quote.c
index 45e3db1..7920e18 100644
--- a/quote.c
+++ b/quote.c
@@ -274,27 +274,6 @@ void write_name_quoted(const char *name, FILE *fp, int terminator)
 	fputc(terminator, fp);
 }
 
-void write_name_quotedpfx(const char *pfx, size_t pfxlen,
-			  const char *name, FILE *fp, int terminator)
-{
-	int needquote = 0;
-
-	if (terminator) {
-		needquote = next_quote_pos(pfx, pfxlen) < pfxlen
-			|| name[next_quote_pos(name, -1)];
-	}
-	if (needquote) {
-		fputc('"', fp);
-		quote_c_style_counted(pfx, pfxlen, NULL, fp, 1);
-		quote_c_style(name, NULL, fp, 1);
-		fputc('"', fp);
-	} else {
-		fwrite(pfx, pfxlen, 1, fp);
-		fputs(name, fp);
-	}
-	fputc(terminator, fp);
-}
-
 void write_name_quoted_relative(const char *name, const char *prefix,
 				FILE *fp, int terminator)
 {
diff --git a/quote.h b/quote.h
index 71dcc3a..99e04d3 100644
--- a/quote.h
+++ b/quote.h
@@ -56,8 +56,6 @@ extern size_t quote_c_style(const char *name, struct strbuf *, FILE *, int no_dq
 extern void quote_two_c_style(struct strbuf *, const char *, const char *, int);
 
 extern void write_name_quoted(const char *name, FILE *, int terminator);
-extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
-                                 const char *name, FILE *, int terminator);
 extern void write_name_quoted_relative(const char *name, const char *prefix,
 		FILE *fp, int terminator);
 
diff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh
index c286854..83fca8d 100755
--- a/t/t3102-ls-tree-wildcards.sh
+++ b/t/t3102-ls-tree-wildcards.sh
@@ -19,4 +19,12 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'ls-tree outside prefix' '
+	cat >expected <<EOF &&
+100644 blob e69de29bb2d1d6434b8b29ae775ad8c2e48c5391	../a[a]/three
+EOF
+	( cd aa && git ls-tree -r HEAD "../a[a]"; ) >actual &&
+	test_cmp expected actual
+'
+
 test_done
-- 
2.2.0.60.gb7b3c64

[PATCH 3/3] ls-tree: disable negative pathspec because it's not supported

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 builtin/ls-tree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/builtin/ls-tree.c b/builtin/ls-tree.c
index 053edb2..3b04a0f 100644
--- a/builtin/ls-tree.c
+++ b/builtin/ls-tree.c
@@ -174,7 +174,8 @@ int cmd_ls_tree(int argc, const char **argv, const char *prefix)
 	 * cannot be lifted until it is converted to use
 	 * match_pathspec() or tree_entry_interesting()
 	 */
-	parse_pathspec(&pathspec, PATHSPEC_GLOB | PATHSPEC_ICASE,
+	parse_pathspec(&pathspec, PATHSPEC_GLOB | PATHSPEC_ICASE |
+				  PATHSPEC_EXCLUDE,
 		       PATHSPEC_PREFER_CWD,
 		       prefix, argv + 1);
 	for (i = 0; i < pathspec.nr; i++)
-- 
2.2.0.60.gb7b3c64

Re: [PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> This allows the callback to use 'base' as a temporary buffer to
> quickly assemble full path "without" extra allocation. The callback
> has to restore it afterwards of course.

Hmph, what's the quote around 'without' doing there?

Re: [PATCH 3/3] ls-tree: disable negative pathspec because it's not supported

[Junio C Hamano]

Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:

> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>

Hmph, that's sad.  Should the below say "test_expect_failure"
without "test_must_fail", anticipating a fix later?

 t/t3102-ls-tree-wildcards.sh | 4 ++++
 1 file changed, 4 insertions(+)

diff --git a/t/t3102-ls-tree-wildcards.sh b/t/t3102-ls-tree-wildcards.sh
index 83fca8d..93127a0 100755
--- a/t/t3102-ls-tree-wildcards.sh
+++ b/t/t3102-ls-tree-wildcards.sh
@@ -27,4 +27,8 @@ EOF
 	test_cmp expected actual
 '
 
+test_expect_success 'ls-tree rejects negated pathspec' '
+	test_must_fail git ls-tree -r HEAD ":(exclude)a" "a*"
+'
+
 test_done

Re: [PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base

[Duy Nguyen]

On Tue, Dec 2, 2014 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> This allows the callback to use 'base' as a temporary buffer to
>> quickly assemble full path "without" extra allocation. The callback
>> has to restore it afterwards of course.
>
> Hmph, what's the quote around 'without' doing there?

because it's only true if you haven't used up all preallocated space
in strbuf. If someone passes an empty strbuf, then underneath strbuf
may do a few realloc until the buffer is large enough.
-- 
Duy

Re: [PATCH 3/3] ls-tree: disable negative pathspec because it's not supported

[Duy Nguyen]

On Tue, Dec 2, 2014 at 2:40 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
>
> Hmph, that's sad.  Should the below say "test_expect_failure"
> without "test_must_fail", anticipating a fix later?

Not a fix from me any time soon (I still need to improve pathspec
support in git-mv). If the git-list-files series goes well, I do plan
to make it list trees and it should support all pathspec without the
fear of subtly breaking a plumbing. But I will change it to
expect_failure unless you change your mind.
-- 
Duy

Re: [PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base

[Eric Sunshine]

On Tue, Dec 2, 2014 at 7:11 AM, Duy Nguyen <pclouds@gmail.com> wrote:
> On Tue, Dec 2, 2014 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>
>>> This allows the callback to use 'base' as a temporary buffer to
>>> quickly assemble full path "without" extra allocation. The callback
>>> has to restore it afterwards of course.
>>
>> Hmph, what's the quote around 'without' doing there?
>
> because it's only true if you haven't used up all preallocated space
> in strbuf. If someone passes an empty strbuf, then underneath strbuf
> may do a few realloc until the buffer is large enough.

Would it be easier to understand if written like this?

    This allows the callback to use 'base' as a temporary buffer to
    quickly assemble full path, thus avoiding allocation/deallocation
    for each iteration step.

Re: [PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> On Tue, Dec 2, 2014 at 7:11 AM, Duy Nguyen <pclouds@gmail.com> wrote:
>> On Tue, Dec 2, 2014 at 2:32 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>> Nguyễn Thái Ngọc Duy  <pclouds@gmail.com> writes:
>>>
>>>> This allows the callback to use 'base' as a temporary buffer to
>>>> quickly assemble full path "without" extra allocation. The callback
>>>> has to restore it afterwards of course.
>>>
>>> Hmph, what's the quote around 'without' doing there?
>>
>> because it's only true if you haven't used up all preallocated space
>> in strbuf. If someone passes an empty strbuf, then underneath strbuf
>> may do a few realloc until the buffer is large enough.
>
> Would it be easier to understand if written like this?
>
>     This allows the callback to use 'base' as a temporary buffer to
>     quickly assemble full path, thus avoiding allocation/deallocation
>     for each iteration step.

Thanks.

I am not Duy so I do not know if that matches what he wanted to say,
or if it matches what the change gives us.  Your phrasing wouldn't
have made me say "Hmph...?" and it is an improvement that goes in
the right direction, I think.

A question during the review, especially on proposed log messages
and documentation changes, is rarely just a request to explain it to
the questioner in the discussion. It is an indication that what is
being commented on needs tweaking to be understood.

Re: [PATCH 1/3] tree.c: update read_tree_recursive callback to pass strbuf as base

[Duy Nguyen]

On Wed, Dec 3, 2014 at 11:13 PM, Junio C Hamano <gitster@pobox.com> wrote:
> A question during the review, especially on proposed log messages
> and documentation changes, is rarely just a request to explain it to
> the questioner in the discussion. It is an indication that what is
> being commented on needs tweaking to be understood.

I do the same at work and somehow forgot to apply the same principle
here :D Do you want to me resend with Eric's wording?
-- 
Duy