On Tue, Sep 15, 2015 at 12:16 PM, Jeff King <peff@xxxxxxxx> wrote: > When working with paths in strbufs, we frequently want to > ensure that a directory contains a trailing slash before > appending to it. We can shorten this code (and make the > intent more obvious) by calling strbuf_complete. > > Most of these cases are trivially identical conversions, but > there are two things to note: > > - in a few cases we did not check that the strbuf is > non-empty (which would lead to an out-of-bounds memory > access). These were generally not triggerable in > practice, either from earlier assertions, or typically > because we would have just fed the strbuf to opendir(), > which would choke on an empty path. > > - in a few cases we indexed the buffer with "original_len" > or similar, rather than the current sb->len, and it is > not immediately obvious from the diff that they are the > same. In all of these cases, I manually verified that > the strbuf does not change between the assignment and > the strbuf_complete call. > > This does not convert cases which look like: > > if (sb->len && !is_dir_sep(sb->buf[sb->len - 1])) > strbuf_addch(sb, '/'); > > as those are obviously semantically different. Some of these > cases arguably should be doing that, but that is out of > scope for this change, which aims purely for cleanup with no > behavior change (and at least it will make such sites easier > to find and examine in the future, as we can grep for > strbuf_complete). > > Signed-off-by: Jeff King <peff@xxxxxxxx> > --- > diff --git a/builtin/clean.c b/builtin/clean.c > index df53def..d7acb94 100644 > --- a/builtin/clean.c > +++ b/builtin/clean.c > @@ -159,8 +159,7 @@ static int is_git_repository(struct strbuf *path) > int gitfile_error; > size_t orig_path_len = path->len; > assert(orig_path_len != 0); > - if (path->buf[orig_path_len - 1] != '/') > - strbuf_addch(path, '/'); > + strbuf_complete(path, '/'); Does the above assert() still have value following this change? I recall, when reviewing this code, specifically asking[1,2] for an assert() or some other check to show that accessing buf[orig_path_len - 1] was safe. Since this patch removes the code in question, the assert() may no longer have meaning. [1]: http://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266892 [2]: http://thread.gmane.org/gmane.comp.version-control.git/266839/focus=266974 > strbuf_addstr(path, ".git"); > if (read_gitfile_gently(path->buf, &gitfile_error) || is_git_directory(path->buf)) > ret = 1; -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html