Re: [PATCH v3 3/4] commit.c: replace some literal strings with constants

[Jonathan Nieder <jrnieder@gmail.com>]

Jay Soffian wrote:

> I converted these per
> http://article.gmane.org/gmane.comp.version-control.git/167015
>
> Maybe this should be the last patch in the series; it's questionable to
> me whether it's even worth doing.

What have I wrought?  I think this makes the code much less readable.

> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -626,13 +632,13 @@ static int prepare_to_commit(const char *index_file, const char *prefix,
>  		format_commit_message(commit, "fixup! %s\n\n",
>  				      &sb, &ctx);
>  		hook_arg1 = "message";
> -	} else if (!stat(git_path("MERGE_MSG"), &statbuf)) {
> -		if (strbuf_read_file(&sb, git_path("MERGE_MSG"), 0) < 0)
> -			die_errno("could not read MERGE_MSG");
> +	} else if (!stat(git_path(merge_msg), &statbuf)) {
> +		if (strbuf_read_file(&sb, git_path(merge_msg), 0) < 0)
> +			die_errno("could not read %s", merge_msg);

So now a person needs to look earlier in the file to see that
merge_msg means "MERGE_MSG" rather than being something that
can vary.

Yes, commit_editmsg has the same problem.  If I get to choose[*] then
cache.h would contain something like

	#define MERGE_HEAD_FILE "MERGE_HEAD"
	#define PREPARED_COMMIT_MESSAGE_FILE "MERGE_MSG"
	#define <description of SQUASH_MSG goes here> "SQUASH_MSG"

and commit.c or cache.h would contain something like

	#define EDITABLE_COMMIT_MESSAGE_FILE "COMMIT_EDITMSG"

and those symbolic names would be used throughout builtin code to
name those files.  So we would get

 (1) documentation of what filenames are special to git, collected
     in one place
 (2) protection against typos

without losing

 (3) names that look like constants

Jonathan

[*] and I shouldn't!  What I like should matter much less than what is
right.