Re: [PATCH 1/3] quote: let caller reset buffer for quote_path_relative()
On Thu, Oct 11, 2012 at 4:13 AM, Junio C Hamano gits...@pobox.com wrote: This sounds a lot stronger than let to me. All existing callers that assumed that buf to be emptied by the function now has to clear it. quote: stop resetting output buffer of quote_path_relative() may better describe what this really does. How should this interact with the logic in the called function that used to say if we ended up returning an empty string because the path is the same as the base, we should give ./ back, and what should the return value of this function be? ... So what does it mean to have an existing string in the output buffer when calling the function? Is it supposed to be a path to a directory, or just a general uninterpreted string (e.g. a message)? I prefer the KISS principle in this case: do not interpret existing string. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] quote: let caller reset buffer for quote_path_relative()
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: On Thu, Oct 11, 2012 at 4:13 AM, Junio C Hamano gits...@pobox.com wrote: This sounds a lot stronger than let to me. All existing callers that assumed that buf to be emptied by the function now has to clear it. quote: stop resetting output buffer of quote_path_relative() may better describe what this really does. How should this interact with the logic in the called function that used to say if we ended up returning an empty string because the path is the same as the base, we should give ./ back, and what should the return value of this function be? ... So what does it mean to have an existing string in the output buffer when calling the function? Is it supposed to be a path to a directory, or just a general uninterpreted string (e.g. a message)? I prefer the KISS principle in this case: do not interpret existing string. Sorry, I do not quite understand what you mean. The original function is about turning one path into a path that is relative to the other path (e.g. make the result usable for chdir() when your cwd() is the other path to name that path), and because you do not want to end up with an empty path, it adds ./ to the result when the two paths are equal. That is simple and stupid, in the sense that you can explain in simple terms what it does even to a stupid person and make him understand. If a patch changes one aspect of the implementation of the function, but keeps other parts intact without thinking the ramifications of doing so through, and ends up giving the end result unnecessarily complex semantics, the _patch_ might be simple and stupid, but the end result is no longer simple. That is why I asked what it means to append and asked you to write it down for people who may need to decide if this function is an appropriate thing to call for their new code in the future. You didn't answer that question, so I have to ask again. What is the existing string this function appends its result to? - If it is a leading path (in other words, you are transplanting a hierarchy to somewhere else --- see and (re)read But if the caller did this instead part from the message you are responding to), because you do not want to end up with an empty path, it adds ./ to the result when the two paths are equal does not make sense at all. - If it is a message that describes the resulting relative path (the use case above that transplant example in the same message), it needs to add ./ to the result, but the logic to determine it now needs to take the length of what was in the buffer when the function was called into account. - Is there a third possibility? Whatever your answer is, please make sure the next person who wants to call this function from his code does not have to ask Why does it append ./ when out-len is zero? Shouldn't the condition be when *rel is an empty string? Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] quote: let caller reset buffer for quote_path_relative()
quote_path_relative() resetting output buffer is sometimes unnecessary
as the buffer has never been used, and some other times makes it
harder for the caller to use (see builtin/grep.c, the caller has to
insert a string after quote_path_relative)
Move the buffer reset back to call sites when necessary.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
The answer for Jeff's XXX in his patch, why strbuf_insert() instead
of just adding in advance.
builtin/clean.c| 2 ++
builtin/grep.c | 2 +-
builtin/ls-files.c | 1 +
quote.c| 1 -
wt-status.c| 1 +
5 files changed, 5 insertions(+), 2 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 69c1cda..ff633cc 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -149,6 +149,7 @@ int cmd_clean(int argc, const char **argv, const char
*prefix)
if (S_ISDIR(st.st_mode)) {
strbuf_addstr(directory, ent-name);
+ strbuf_reset(buf);
qname = quote_path_relative(directory.buf,
directory.len, buf, prefix);
if (show_only (remove_directories ||
(matches == MATCHED_EXACTLY))) {
@@ -171,6 +172,7 @@ int cmd_clean(int argc, const char **argv, const char
*prefix)
} else {
if (pathspec !matches)
continue;
+ strbuf_reset(buf);
qname = quote_path_relative(ent-name, -1, buf,
prefix);
if (show_only) {
printf(_(Would remove %s\n), qname);
diff --git a/builtin/grep.c b/builtin/grep.c
index 82530a6..377c904 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -376,9 +376,9 @@ static int grep_sha1(struct grep_opt *opt, const unsigned
char *sha1,
struct strbuf pathbuf = STRBUF_INIT;
if (opt-relative opt-prefix_length) {
+ strbuf_add(pathbuf, filename, tree_name_len);
quote_path_relative(filename + tree_name_len, -1, pathbuf,
opt-prefix);
- strbuf_insert(pathbuf, 0, filename, tree_name_len);
} else {
strbuf_addstr(pathbuf, filename);
}
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b5434af..1e0cae9 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -395,6 +395,7 @@ int report_path_error(const char *ps_matched, const char
**pathspec, const char
if (found_dup)
continue;
+ strbuf_reset(sb);
name = quote_path_relative(pathspec[num], -1, sb, prefix);
error(pathspec '%s' did not match any file(s) known to git.,
name);
diff --git a/quote.c b/quote.c
index 911229f..7e23ba9 100644
--- a/quote.c
+++ b/quote.c
@@ -381,7 +381,6 @@ char *quote_path_relative(const char *in, int len,
{
struct strbuf sb = STRBUF_INIT;
const char *rel = path_relative(in, len, sb, prefix, -1);
- strbuf_reset(out);
quote_c_style_counted(rel, strlen(rel), out, NULL, 0);
strbuf_release(sb);
diff --git a/wt-status.c b/wt-status.c
index 2a9658b..be8b600 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -690,6 +690,7 @@ static void wt_status_print_other(struct wt_status *s,
struct string_list_item *it;
const char *path;
it = (l-items[i]);
+ strbuf_reset(buf);
path = quote_path(it-string, strlen(it-string),
buf, s-prefix);
if (column_active(s-colopts)) {
--
1.7.12.1.406.g6ab07c4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/3] quote: let caller reset buffer for quote_path_relative()
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: quote_path_relative() resetting output buffer is sometimes unnecessary as the buffer has never been used, and some other times makes it harder for the caller to use (see builtin/grep.c, the caller has to insert a string after quote_path_relative) Move the buffer reset back to call sites when necessary. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- The answer for Jeff's XXX in his patch, why strbuf_insert() instead of just adding in advance. This sounds a lot stronger than let to me. All existing callers that assumed that buf to be emptied by the function now has to clear it. quote: stop resetting output buffer of quote_path_relative() may better describe what this really does. How should this interact with the logic in the called function that used to say if we ended up returning an empty string because the path is the same as the base, we should give ./ back, and what should the return value of this function be? To answer these questions, you must define the meaning of the string in the output buffer that already exists when the function is called. If the caller did this: strbuf_addstr(out, The path relative to your HOME is: ); quote_path_relative(path, pathlen, out, /home/pclouds/); then the answers are We still need to add ./ but !out-len is no longer a good test to decide and It should point at the first byte of what we added, not out-buf. But if the caller did this instead: srcdir = /src/; strbuf_addstr(dst, /dst/); quote_path_relative(path, pathlen, dst, srcdir); printf(cp '%s' '%s'\n, path, dst-buf); then it is nonsensical to add ./ when out-len is not zero when the function returns. So what does it mean to have an existing string in the output buffer when calling the function? Is it supposed to be a path to a directory, or just a general uninterpreted string (e.g. a message)? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
