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

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 22, 2009, 05:31 UTC
Message-ID: <7vwsai86nw.fsf@gitster.siamese.dyndns.org>
In-Reply-To: <1237696363-6819-2-git-send-email-bebarino@gmail.com>

Stephen Boyd <bebarino@gmail.com> writes:

Show 6 quoted lines

> 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?