Re: [PATCHv2 1/3] format-patch: create patch filename in one function

[Junio C Hamano <gitster@pobox.com>]

Stephen Boyd <bebarino@gmail.com> writes:

> reopen_stdout() usually takes the oneline description of a commit,
> appends the patch suffix, prepends the output directory (if any) and
> then reopens stdout as the resulting file. Now the patch filename (the
> oneline description and the patch suffix) is created in
> get_patch_filename() and passed to reopen_stdout() which prepends the
> output directory and reopens stdout as that file.

The renaming is a good idea even without any change in the feature.
Naming functions after what their result is used _for_ is never a good
idea, and we should name them after what they do.

Does it still make sense to pass "keep_subject" to the function?  After
all what it does is to retain "[PATCH.." prefix that is useless for the
purpose of making each patch easily identifiable.  Because people almost
always use patch acceptance tools in non-keep mode to strip the
"[PATCH..]"  prefix when creating the commits these days anyway, it may
make more sense to lose the parameter altogether and simplify the
processing.

> -static const char *get_oneline_for_filename(struct commit *commit,
> -					    int keep_subject)
> +static const char *get_patch_filename(char* sol, int keep_subject, int nr)

Asterisk sticks to the variable name, not type name.

I also wonder if it makes sense to move what this function does into a
user format; especially the logic that sanitizes the oneline string into
filename friendly one may be something Porcelains may want an access to
from outside.

IOW, you can introduce a new format specifier (say, "%f") to
format_commit_message() and the implemention of get_patch_filename() would
just prepare a strbuf and call format_commit_message() on it, no?