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

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

[Junio C Hamano]

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

[PATCH 1/2] merge: separte the logic to check for a signed tag

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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

[PATCH 2/2] merge: allow fast-forwarding to an annotated but unsigned tag

[Junio C Hamano]

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 <gitster@pobox.com>
---
 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

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

[Jeff King]

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

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Jun 05, 2012 at 12:58:30PM -0700, Junio C Hamano wrote:
> ...
>> 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].

It is in line with --ff-only special casing, though.  The argument
against it is that "we used to fast forward", I would think, even
though in general my reaction to that would be "so what?" because I
agree with your "keeping more information instead of discarding as
we used to is a good feature enhancement, why should we retreat?"

> [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. :)

I am actually ambivalent; instead of being 0% supportive like I
usually am for many topics, perhaps I am 30% sympathetic to this
one.

This was triggered by
http://thread.gmane.org/gmane.comp.version-control.git/198828

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

[Jeff King]

On Wed, Jun 06, 2012 at 09:37:52AM -0700, Junio C Hamano wrote:

> > 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].
> 
> It is in line with --ff-only special casing, though.

Is it?  My impression from reading b5c9f1c is that --ff-only trumps both
annotated _and_ signed tags. Which makes sense to me. What I was
objecting to is that "some tag objects are more equal than others". It's
OK to treat unannotated tags differently from tag objects, but treating
annotated but unsigned objects differently from signed objects seems
unnecessary and complex.

> This was triggered by
> http://thread.gmane.org/gmane.comp.version-control.git/198828

The complaint in that thread mentions git's v1.7.10.2, which is signed,
and therefore would not be impacted by change. But perhaps that was
purely for illustrative purposes.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Jun 06, 2012 at 09:37:52AM -0700, Junio C Hamano wrote:
>
>> > 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].
>> 
>> It is in line with --ff-only special casing, though.
>
> Is it?  My impression from reading b5c9f1c is that --ff-only trumps both
> annotated _and_ signed tags. Which makes sense to me. What I was
> objecting to is that "some tag objects are more equal than others". It's
> OK to treat unannotated tags differently from tag objects, but treating
> annotated but unsigned objects differently from signed objects seems
> unnecessary and complex.

OK, let's drop it.