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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Feb 17, 2011, 05:19 UTC
Message-ID: <20110217051919.GA7740@elie>
In-Reply-To: <1297916325-89688-4-git-send-email-jaysoffian@gmail.com>

Jay Soffian wrote:

Show 5 quoted lines

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

Show 12 quoted lines

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