[PATCH 0/2] Feeding an annotated but unsigned tag to "git merge"

When you give an annotated but unsigned tag to "git merge", if the
tagged commit does not fast-forward, we create a merge commit and
the tagged commit (not the tag itself) becomes one of the parents of
the resulting merge commit. The merge commit log contains the
message from the annotated tag.

When the tagged commit is a descendant of the current HEAD, however,
we used to simply fast-forward such a merge (losing the content of
the annotated tag).

Post 1.7.9, we no longer do so.  These three create a merge commit
for an annotated tag "anno" that points at a commit that is a
descendant of the HEAD:

        $ git merge anno
        $ git merge --ff anno
        $ git merge --no-ff anno

You can force fast-forwarding with:

        $ git merge anno^0

but you obvously cannot record the contents of the annotated tag, as
there is no new commit to record it.

The "--ff" option has always meant "allow fast-forward", i.e. "if
the merge can be fast-forwarded, do so without creating a new merge
commit", and without any of the "ff"-related options, the command
defaults to allow fast-forwarding.  "--no-ff" is "I always want a
new merge commit made", and "--ff-only" is "fail the command if it
cannot be fast-forwarded".  In effect, in the post 1.7.9 world, we
consider that an annotated tag is what you cannot fast-forward to.

The above definition was loosened slightly with b5c9f1c (merge: do
not create a signed tag merge under --ff-only option, 2012-02-05).
"--ff-only" is taught to consider an annotated or signed tag that
points at a commit that can be fast-forwarded as what you can
fast-forward to, so that a user following along without adding
anything can do this:

	$ git checkout v3.2.0
	$ git pull --ff-only v3.3.0

without creating an extra merge commit.

This two-patch series further loosens the definition by considering
that an annotated but unsigned tag can be fast-forwarded as long as
it points at a commit that can be fast-forwarded to.  So

        $ git merge anno
        $ git merge --ff anno

will now fast-forward (note that this will *not* happen for signed
tags).

I find this change somewhat iffy myself, as we are encouraging
people to lose information (i.e. the contents of the annotated tag
is no longer recorded in the history) and some may see it as a
regression in the post 1.7.10 world because of that.

But since I've written it already, I thought it might be worth
showing it to the list for discussion, if only to publicly reject
the idea ;-).

Junio C Hamano (2):
  merge: separte the logic to check for a signed tag
  merge: allow fast-forwarding to an annotated but unsigned tag

 builtin/merge.c  | 26 ++++++++++++++++++++++----
 t/t7600-merge.sh | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 4 deletions(-)

-- 
1.7.11.rc1.37.g09843ac

We drop allow_fast_forward when merging a signed tag, because we
always need to create a new commit to have a place to record the
signed tag payload.

Move the logic to determine if the object given to merge is a signed
tag into a separate helper function.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/merge.c | 14 ++++++++++----
 1 file changed, 10 insertions(+), 4 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index f385b8a..23389f2 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -1099,6 +1099,15 @@ static void write_merge_state(void)
 	close(fd);
 }
 
+static int merging_signed_tag(struct commit *parent)
+{
+	struct merge_remote_desc *desc = merge_remote_util(parent);
+
+	if (!desc || !desc->obj || desc->obj->type != OBJ_TAG)
+		return 0;
+	return 1;
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	unsigned char result_tree[20];

@@ -1283,10 +1292,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    sha1_to_hex(commit->object.sha1));
 		setenv(buf.buf, argv[i], 1);
 		strbuf_reset(&buf);
-		if (!fast_forward_only &&
-		    merge_remote_util(commit) &&
-		    merge_remote_util(commit)->obj &&
-		    merge_remote_util(commit)->obj->type == OBJ_TAG) {
+		if (!fast_forward_only && merging_signed_tag(commit)) {
 			if (option_edit < 0)
 				option_edit = 1;
 			allow_fast_forward = 0;

-- 
1.7.11.rc1.37.g09843ac

Update the merging_signed_tag() helper to check if the tag object
actually has a signature-looking string, so that we do not forbid
fast-forwarding to an annotated but unsigned tag.  By definition,
there will be no signed payload in such a tag to be moved to the
mergetag header, so we are not losing anything by fast-forwarding.

Signed-off-by: Junio C Hamano <redacted>
---
 builtin/merge.c  | 14 +++++++++++++-
 t/t7600-merge.sh | 38 ++++++++++++++++++++++++++++++++++++++
 2 files changed, 51 insertions(+), 1 deletion(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 23389f2..82d343c 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c

@@ -28,6 +28,7 @@
 #include "remote.h"
 #include "fmt-merge-msg.h"
 #include "gpg-interface.h"
+#include "tag.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)

@@ -1102,10 +1103,21 @@ static void write_merge_state(void)
 static int merging_signed_tag(struct commit *parent)
 {
 	struct merge_remote_desc *desc = merge_remote_util(parent);
+	unsigned long size;
+	enum object_type type;
+	char *buf;
+	size_t sig_offset;
 
 	if (!desc || !desc->obj || desc->obj->type != OBJ_TAG)
 		return 0;
-	return 1;
+
+	buf = read_sha1_file(desc->obj->sha1, &type, &size);
+	if (!buf || type != OBJ_TAG) {
+		free(buf);
+		return 0; /* error will be caught downstream */
+	}
+	sig_offset = parse_signature(buf, size);
+	return (sig_offset < size);
 }
 
 int cmd_merge(int argc, const char **argv, const char *prefix)

diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 9e27bbf..3c48327 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh

@@ -695,4 +695,42 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '
 	test_cmp actual expect
 '
 
+test_expect_success 'merge ff annotated tag should just ff' '
+	git reset --hard c0 &&
+	git commit --allow-empty -m "A newer commit" &&
+	git tag -a -m "An annotated tag" anno &&
+	git reset --hard c0 &&
+
+	# This should not even bother with an editor session; "false"
+	# will ensure that an attempt to run the editor is caught.
+	EDITOR=false git merge anno &&
+
+	git rev-parse anno^0 >expect &&
+	git rev-parse HEAD >actual &&
+	test_cmp actual expect &&
+
+	git rev-parse c0^0 >expect &&
+	git rev-parse HEAD^ >actual &&
+	test_cmp actual expect
+'
+
+test_expect_success 'merge --no-ff annotated tag' '
+	git reset --hard c0 &&
+	git commit --allow-empty -m "A newer commit" &&
+	git tag -f -a -m "An annotated tag" anno &&
+	git reset --hard c0 &&
+
+	EDITOR=./editor git merge --no-ff --edit anno &&
+	git rev-parse anno^0 >expect &&
+	git rev-parse HEAD^2 >actual &&
+	test_cmp actual expect &&
+
+	git rev-parse c0^0 >expect &&
+	git rev-parse HEAD^ >actual &&
+	test_cmp actual expect &&
+
+	git cat-file commit HEAD >raw &&
+	grep "An annotated tag" raw
+'
+
 test_done

-- 
1.7.11.rc1.37.g09843ac

On Tue, Jun 05, 2012 at 12:58:30PM -0700, Junio C Hamano wrote:

This two-patch series further loosens the definition by considering
that an annotated but unsigned tag can be fast-forwarded as long as
it points at a commit that can be fast-forwarded to.  So

        $ git merge anno
        $ git merge --ff anno

will now fast-forward (note that this will *not* happen for signed
tags).

I find this change somewhat iffy myself, as we are encouraging
people to lose information (i.e. the contents of the annotated tag
is no longer recorded in the history) and some may see it as a
regression in the post 1.7.10 world because of that.

But since I've written it already, I thought it might be worth
showing it to the list for discussion, if only to publicly reject
the idea ;-).

It has been nearly a day, and nobody has publicly rejected it. So I will
do so. :)

This just doesn't make sense to me. Why would we treat annotated but
unsigned tags differently from signed tags? In both cases, the new
behavior is keeping more information about what happened, which is
generally a good thing.

I haven't seen any good argument against creating these merges[1]. But
even if there was one, I don't think "signed versus unsigned" is
necessarily the right distinguishing feature. It is probably more about
per-project or per-user preferences (e.g., "my project does not want too
many merges, because it makes our history less pretty"). And in that
case, something like a config flag would be a better option (not that I
am not saying that such a flag is a good idea, only that it might be
less bad than this).

-Peff

[1] From the tone of your message, I think you are not the right person
    to be arguing that side, anyway. It sounds as though you are not all
    that invested in this series. :)