From: Shawn O. Pearce <spearce@spearce.org>
Date: Fri, 03 Oct 2008 02:35:18 GMT
Subject: Re: [PATCH] builtin-commit: avoid always using reduce_heads()
Message-ID: <20081003023518.GA3291@spearce.org>
In-Reply-To: <1222457868-9864-1-git-send-email-vmiklos@frugalware.org>

Miklos Vajna <vmiklos@frugalware.org> wrote:
> diff --git a/builtin-commit.c b/builtin-commit.c
> index 55e1087..f546cf7 100644
> --- a/builtin-commit.c
> +++ b/builtin-commit.c
> @@ -1040,6 +1050,7 @@ int cmd_commit(int argc, const char **argv, const char *prefix)
>  
>  	unlink(git_path("MERGE_HEAD"));
>  	unlink(git_path("MERGE_MSG"));
> +	unlink(git_path("MERGE_MODE"));
>  	unlink(git_path("SQUASH_MSG"));
>  
>  	if (commit_index_files())

Hmmph.  Should branch.c and builtin-reset.c clean this new file
up too?

> diff --git a/builtin-merge.c b/builtin-merge.c
> index 5c65a58..20102a0 100644
> --- a/builtin-merge.c
> +++ b/builtin-merge.c
> @@ -1210,6 +1210,15 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
>  			merge_msg.len)
>  			die("Could not write to %s", git_path("MERGE_MSG"));
>  		close(fd);
> +		fd = open(git_path("MERGE_MODE"), O_WRONLY | O_CREAT, 0666);
> +		if (fd < 0)
> +			die("Could open %s for writing", git_path("MERGE_MODE"));
> +		strbuf_reset(&buf);
> +		if (!allow_fast_forward)
> +			strbuf_addf(&buf, "no-ff");
> +		if (write_in_full(fd, buf.buf, buf.len) != buf.len)

Shouldn't we open this file with O_TRUNC to avoid this scenario:

  $ git merge --no-ff --no-commit foo
  $ git reset --hard
  $ git merge --no-commit foo
  ... *sigh* MERGE_MODE still has "no-ff" in it ...

This is especially true since some porcelain (e.g. git-gui) just
deletes MERGE_HEAD right now and doesn't know about cleaning up
MERGE_MODE.  We'd want to at least reset it correctly on the next
invocation to git-merge.

-- 
Shawn.