Eric Sunshine <sunshine@xxxxxxxxxxxxxx> writes: > This new example raises the question about what happens if the > argument to --reroll-count contains characters which don't belong in > pathnames. For instance, what happens if `--reroll-count=1/2` is > specified? Most likely, it will fail trying to write the > "v1/2-whatever.patch" file to a nonexistent directory named "v1". > >> diff --git a/log-tree.c b/log-tree.c >> @@ -369,8 +369,8 @@ void fmt_output_subject(struct strbuf *filename, >> + if (info->reroll_count) >> + strbuf_addf(filename, "v%s-", info->reroll_count); >> strbuf_addf(filename, "%04d-%s", nr, subject); > > To protect against that problem, you may need to call > format_sanitized_subject() manually after formatting "v%s-". (I'm just > looking at this code for the first time, so I could be hopelessly > wrong. There may be a better way to fix it.) This kind of discovery of what I and others missed is why I love to see reviews on this list ;-) Yes, slash is of course very problematic, but what we've been doing to the patch filenames was to ensure that they will be free of $IFS whitespaces and shell glob special characters as well, and we should treat the "reroll count" just like the other end-user controlled input, i.e. the title of the patch, and sanitize it the same way. So I am pretty sure format_sanitized_subject() is the right way to go.
