Re: linux-next: unnecessary merge in the v4l-dvb tree

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Linus Torvalds]

On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> Linus, this happens a bit after the merge window, so I am wondering
> about the rational of not doing a fast forward merge when merging a
> signed tag (I forget the reasoning).

The reasoning is to avoid losing the signature from the tag (when
merging a signed tag, the signature gets inserted into the merge
commit itself - use "git log --show-signature" to see them).

So when I merge a signed tag, I do *not* want to fast-forward to the
top commit, because then I'd lose the signature from the tag. Thus the
"merging signed tags are non-fast-forward by default" reasoning.

But, yes, that reasoning is really only valid for proper merges of new
features, not for back-merges.

The problem, of course, is that since git is distributed, git doesn't
know who is "upstream" and who is "downstream", so there's no
_technical_ difference between merging a development tree, and a
development tree doing a back-merge of the upstream tree.

Maybe it was a mistake to make signed tag merges non-fast-forward,
since they cause these kinds of issues with people who use "pull" to
update their otherwise unmodified trees.

I can always teach myself to just use --no-ff, since I end up doing
things like verifying at the signatures anyway.

Junio, comments?

               Linus

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Mauro Carvalho Chehab]

Em Mon, 12 Feb 2018 13:15:04 -0800
Linus Torvalds <torvalds@linux-foundation.org> escreveu:

> On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
> >
> > Linus, this happens a bit after the merge window, so I am wondering
> > about the rational of not doing a fast forward merge when merging a
> > signed tag (I forget the reasoning).  
> 
> The reasoning is to avoid losing the signature from the tag (when
> merging a signed tag, the signature gets inserted into the merge
> commit itself - use "git log --show-signature" to see them).
> 
> So when I merge a signed tag, I do *not* want to fast-forward to the
> top commit, because then I'd lose the signature from the tag. Thus the
> "merging signed tags are non-fast-forward by default" reasoning.
> 
> But, yes, that reasoning is really only valid for proper merges of new
> features, not for back-merges.
> 
> The problem, of course, is that since git is distributed, git doesn't
> know who is "upstream" and who is "downstream", so there's no
> _technical_ difference between merging a development tree, and a
> development tree doing a back-merge of the upstream tree.
> 
> Maybe it was a mistake to make signed tag merges non-fast-forward,
> since they cause these kinds of issues with people who use "pull" to
> update their otherwise unmodified trees.
> 
> I can always teach myself to just use --no-ff, since I end up doing
> things like verifying at the signatures anyway.

Hmm... at least at git version 2.14.3, git documentation doesn't
mention that signed pull requests won't do fast forward. Instead,
it says that --ff is the default behavior:


       --ff
           When the merge resolves as a fast-forward, only update the branch pointer, without creating a merge commit. This is the
           default behavior.

Btw, even doing:

	$ git merge -ff v4.16-rc1

it will still produce a git commit for the merge.


-- 
Thanks,
Mauro

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Linus Torvalds]

On Mon, Feb 12, 2018 at 1:15 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> The reasoning is to avoid losing the signature from the tag (when
> merging a signed tag, the signature gets inserted into the merge
> commit itself - use "git log --show-signature" to see them).

I think the commit that actually introduced the behavior was

    fab47d057: merge: force edit and no-ff mode when merging a tag object

back in 2011, so we've had this behavior for a long time. So it's
probably not be worth tweaking the behavior any more, and maybe we
need to educate people to not update to other peoples state with "git
pull".

Maybe we could just tell people to have something like

       git config --global alias.update pull --ff-only

and use that for "try to update to upstream".

               Linus

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell <sfr@canb.auug.org.au> wrote:
>
> The problem, of course, is that since git is distributed, git doesn't
> know who is "upstream" and who is "downstream", so there's no
> _technical_ difference between merging a development tree, and a
> development tree doing a back-merge of the upstream tree.
>
> Maybe it was a mistake to make signed tag merges non-fast-forward,
> since they cause these kinds of issues with people who use "pull" to
> update their otherwise unmodified trees.
>
> I can always teach myself to just use --no-ff, since I end up doing
> things like verifying at the signatures anyway.
>
> Junio, comments?

I have a slight suspicion that allowing 'pull' to fast-forward even
when merging a signed tag when it is pulling from a configured
default remote for the branch the user is on, and otherwise keeping
the current behaviour, would make majority of people from both camps
happier, but I also have a strong conviction that it is being too
clever and making it hard to explain to people to do such a dwim
that tries to guess which way is 'upstream'.

Another clue we _might_ be able to take advantage of is that when
upstream maintainers merge a signed tag, we do *not* fetch and store
the tag from downstream contributers in our local repository (it is
likely that we have --no-tags in remote.<name>.tagopt), but when
downstream contributers sync from us with "git pull", they do fetch
and store our tags in their local repository.

So "git pull $somewhere $tag" that defaults to "--ff" when the tag
gets stored somewhere in refs/ (or more explicitly, in refs/tags/)
and defaults to "--no-ff" otherwise (i.e. the tag is fetched only to
be recorded in the resulting merge, without ever stored in any of
our refs), might be a good balance.  

And it is easy to explain: "We realize that it was a mistake to
unconditionally default to --no-ff and we are reverting the default
to --ff, but with a twist.  When we tell 'pull' to grab a tag, if
we do not store it anywhere in our local ref space, that would mean
the tag is totally lost if the pull fast-forwards.  That is why we
still use --no-ff in such a case."

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> Maybe we could just tell people to have something like
>
>        git config --global alias.update pull --ff-only
>
> and use that for "try to update to upstream".

I guess our mails crossed.  I admit that I indeed wondered why you
were not giving your usual "downstream shouldn't do pointless pull
from upstream" briefly but focused too much on how to tweak the
default without thinking through.

But I wonder why "update to upstream" is merging a signed tag in the
first place.  Wouldn't downstream's "try to keep up with" pull be
grabbing from branch tips, not tags?

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Linus Torvalds]

On Mon, Feb 12, 2018 at 1:44 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> But I wonder why "update to upstream" is merging a signed tag in the
> first place.  Wouldn't downstream's "try to keep up with" pull be
> grabbing from branch tips, not tags?

I'm actually encouraging maintainers to *not* start their work on some
random "kernel of the day".

Particularly during the kernel merge window, the upstream master
branch can be pretty flaky. It's *not* a good point to start new
development on, so if you're a maintainer, you really don't want to
use that as the basis for your work for the next merge window.

So I encourage people to use a stable point for new development, and
particularly actual release kernels. The natural way to do that is
obviously just to create a new branch:

   git checkout -b topicbranch v4.15

and now you have a good new clean branch for your development.

But clearly we've got a few people who have gotten used to the whole
"git pull" convenience of both fetching and updating.

Some maintainers don't even use topic branches, because their main
work is just merging work by subdevelepoers (that goes for my own
tree: I use topic branches for merging patch queues and to
occasionally track my own experimental patches, but 99% of the time
I'm just on "master" and obviously pull other peoples branches).

And some maintainers end up using multiple repositories as branches
(the old _original_ git model). Again, you can just use "git fetch +
git reset", of course, but that's a bit unsafe. In contrast, doing
"git pull --ff-only" is a safe convenient operation that does both the
fetch and the update to whatever state.

But you do need that "--ff-only" to avoid the merge.

              Linus

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> And some maintainers end up using multiple repositories as branches
> (the old _original_ git model). Again, you can just use "git fetch +
> git reset", of course, but that's a bit unsafe. In contrast, doing
> "git pull --ff-only" is a safe convenient operation that does both the
> fetch and the update to whatever state.
>
> But you do need that "--ff-only" to avoid the merge.

OK.  I guess it is legit (and semi-sensible) for downstream
contributors to "git pull --ff-only $upstream $release_tag_X" to
bring their long-running topic currently based on release X-1 up to
date with respect to release X.  It probably makes more sense than
rebasing on top of release X, even though it makes a lot less sense
than merging their topics into release X.

As you said, pull of a tag that forbids fast-forward by default is
rather old development (I am kind of surprised that it was so old,
in v1.7.9), so it may be a bit difficult to transition.

There is 

	[pull]
                ff = only

but pull.ff is quite global, and not good for intermediate level
maintainers who pull to integrate work of their downstream (for
which they do want the current "do not ff, record the tag in a merge
commit" behaviour) and also pull to catch up from their upstream
(which they want "ff-when-able").  They need to control between
ff=only and ff=when-able, depending on whom they are pulling from.

We may want per-remote equivalent for it, i.e. e.g.

	[pull]
		ff=false ;# good default for collecting contributions

	[remote "torvalds"] 
		pullFF = only ;# good default for catching up

or something like that, perhaps?

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Mauro Carvalho Chehab]

Em Mon, 12 Feb 2018 15:42:44 -0800
Junio C Hamano <gitster@pobox.com> escreveu:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> > And some maintainers end up using multiple repositories as branches
> > (the old _original_ git model). Again, you can just use "git fetch +
> > git reset", of course, but that's a bit unsafe. In contrast, doing
> > "git pull --ff-only" is a safe convenient operation that does both the
> > fetch and the update to whatever state.
> >
> > But you do need that "--ff-only" to avoid the merge.  
> 
> OK.  I guess it is legit (and semi-sensible) for downstream
> contributors to "git pull --ff-only $upstream $release_tag_X" to
> bring their long-running topic currently based on release X-1 up to
> date with respect to release X.  It probably makes more sense than
> rebasing on top of release X, even though it makes a lot less sense
> than merging their topics into release X.
> 
> As you said, pull of a tag that forbids fast-forward by default is
> rather old development (I am kind of surprised that it was so old,
> in v1.7.9), so it may be a bit difficult to transition.
> 
> There is 
> 
> 	[pull]
>                 ff = only
> 
> but pull.ff is quite global, and not good for intermediate level
> maintainers who pull to integrate work of their downstream (for
> which they do want the current "do not ff, record the tag in a merge
> commit" behaviour) and also pull to catch up from their upstream
> (which they want "ff-when-able").  They need to control between
> ff=only and ff=when-able, depending on whom they are pulling from.

Yes, that's my pain. I don't want ff only when pulling from others,
only when pulling from upstream tree.

> 
> We may want per-remote equivalent for it, i.e. e.g.
> 
> 	[pull]
> 		ff=false ;# good default for collecting contributions
> 
> 	[remote "torvalds"] 
> 		pullFF = only ;# good default for catching up
> 
> or something like that, perhaps?


Yeah, something like that works. Please notice, however, that what I
usually do is:

	$ git remote update torvalds
	$ git merge <tag>
	  (or git pull . <tag>)

So, for the above to work, it should store somehow the remote from
where a tag came from.




The reason is that I keep locally a cache with several tree clones
(in bare mode) s that I bother enough to cache (linus, -stable, -next),
as pulling from BR is time consuming, and I want to do it only once
and use the same "cache" for all my git clones.

I have a few git workdirs for my upstream work, but, as a patch
developer, I also have "independent"[1] git repositories.

[1] Due to disk constraints, the clones actually use --shared. So,
the common objects are actually stored inside a single tree.

Thanks,
Mauro

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Mauro Carvalho Chehab <mchehab@osg.samsung.com> writes:

> Yes, that's my pain. I don't want ff only when pulling from others,
> only when pulling from upstream tree.
>
>> 
>> We may want per-remote equivalent for it, i.e. e.g.
>> 
>> 	[pull]
>> 		ff=false ;# good default for collecting contributions
>> 
>> 	[remote "torvalds"] 
>> 		pullFF = only ;# good default for catching up
>> 
>> or something like that, perhaps?
>
>
> Yeah, something like that works. Please notice, however, that what I
> usually do is:
>
> 	$ git remote update torvalds
> 	$ git merge <tag>
> 	  (or git pull . <tag>)
>
> So, for the above to work, it should store somehow the remote from
> where a tag came from.

That makes me wonder if another heuristic I floated earlier is more
appropriate.  When merging a tag object T, if refs/tags/T exists and
it is that tag object, then an updated "merge" would default to "--ff";
otherwise, it would keep the current default of creating a merge even
when we could fast-forward, in order to record that tag T in the
resulting history.

Of course, end users can use command line options to override such
heuristics anyway, but if the behaviour based on the new heuristic
is easy to explain and understand, and covers majority of the use
cases without command line override, then we might not even need a
new configuration mechanism like remove.torvalds.pullFF mentioned
above.

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Linus Torvalds]

On Tue, Feb 13, 2018 at 9:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>
> That makes me wonder if another heuristic I floated earlier is more
> appropriate.  When merging a tag object T, if refs/tags/T exists and
> it is that tag object, then an updated "merge" would default to "--ff";
> otherwise, it would keep the current default of creating a merge even
> when we could fast-forward, in order to record that tag T in the
> resulting history.

Oooh. Yes, that sounds like the right thing to do.

So the "no fast-forward" logic would trigger only if the name we used
for merging is one of the temporary ones (ie .git/{FETCH,MERGE}_HEAD),
not if the mentioned tag is already a normal tag reference.

Then it's very explicitly about "don't lose the signing information".

I'd still have to teach people to use "--only-ff" if they don't do the
"fetch and merge" model but literally just do  "git pull upstream
vX.Y", but at least the case Mauro describes would automatically just
DTRT.

Me likey.

           Linus

Re: linux-next: unnecessary merge in the v4l-dvb tree

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Tue, Feb 13, 2018 at 9:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> That makes me wonder if another heuristic I floated earlier is more
>> appropriate.  When merging a tag object T, if refs/tags/T exists and
>> it is that tag object, then an updated "merge" would default to "--ff";
>> otherwise, it would keep the current default of creating a merge even
>> when we could fast-forward, in order to record that tag T in the
>> resulting history.
>
> Oooh. Yes, that sounds like the right thing to do.
>
> So the "no fast-forward" logic would trigger only if the name we used
> for merging is one of the temporary ones (ie .git/{FETCH,MERGE}_HEAD),
> not if the mentioned tag is already a normal tag reference.
>
> Then it's very explicitly about "don't lose the signing information".
>
> I'd still have to teach people to use "--only-ff" if they don't do the
> "fetch and merge" model but literally just do  "git pull upstream
> vX.Y", but at least the case Mauro describes would automatically just
> DTRT.
>
> Me likey.

The implementation cannot exactly be "did the user give FETCH_HEAD
or v4.16-rc1 from the command line?", because we'd want to catch it
when Mauro says "git fetch linus && git merge v4.16-rc1" and behave
identically as "git pull linus v4.16-rc1" (and the latter internally
gets turned into "git merge FETCH_HEAD").

So, instead, we read the "tag" line from the tag object to learn the
tagname T, see if refs/tags/T exists and points at that object, to
see if we are Mauro who follows your tags, or if we are you who
fetch and merge contributors' "for-linus" signed tag (which I am
assuming you won't contaminate your refs/tags/ hierarchy with).

There are a few fallouts in the testsuite if we go this route.  I am
not quite decided if I like the approach.

 builtin/merge.c          | 42 ++++++++++++++++++++++++++++++++++++++----
 t/t6200-fmt-merge-msg.sh |  2 +-
 t/t7600-merge.sh         |  2 +-
 3 files changed, 40 insertions(+), 6 deletions(-)

diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..45c7916505 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -33,6 +33,7 @@
 #include "sequencer.h"
 #include "string-list.h"
 #include "packfile.h"
+#include "tag.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -1125,6 +1126,42 @@ static struct commit_list *collect_parents(struct commit *head_commit,
 	return remoteheads;
 }
 
+static int merging_a_throwaway_tag(struct commit *commit)
+{
+	const char *tag_ref;
+	struct object_id oid;
+
+	/* Are we merging a tag? */
+	if (!merge_remote_util(commit) ||
+	    !merge_remote_util(commit)->obj ||
+	    merge_remote_util(commit)->obj->type != OBJ_TAG)
+		return 0;
+
+	/*
+	 * Now we know we are merging a tag object.  Are we downstream
+	 * and following the tags from upstream?  If so, we must have
+	 * the tag object pointed at by "refs/tags/$T" where $T is the
+	 * tagname recorded in the tag object.  We want to allow such
+	 * a "just to catch up" merge to fast-forward.
+	 */
+	tag_ref = xstrfmt("refs/tags/%s",
+			  ((struct tag *)merge_remote_util(commit)->obj)->tag);
+
+	if (!read_ref(tag_ref, &oid) &&
+	    !oidcmp(&oid, &merge_remote_util(commit)->obj->oid))
+		return 0;
+
+	/*
+	 * Otherwise, we are playing an integrator's role, making a
+	 * merge with a throw-away tag from a contributor with
+	 * something like "git pull $contributor $signed_tag".
+	 * We want to forbid such a merge from fast-forwarding
+	 * by default; otherwise we would not keep the signature
+	 * anywhere.
+	 */
+	return 1;
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	struct object_id result_tree, stash, head_oid;
@@ -1322,10 +1359,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    oid_to_hex(&commit->object.oid));
 		setenv(buf.buf, merge_remote_util(commit)->name, 1);
 		strbuf_reset(&buf);
-		if (fast_forward != FF_ONLY &&
-		    merge_remote_util(commit) &&
-		    merge_remote_util(commit)->obj &&
-		    merge_remote_util(commit)->obj->type == OBJ_TAG)
+		if (fast_forward != FF_ONLY && merging_a_throwaway_tag(commit))
 			fast_forward = FF_NO;
 	}
 
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 2e2fb0e957..a54a52aaa4 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -512,7 +512,7 @@ test_expect_success 'merge-msg with "merging" an annotated tag' '
 
 	test_when_finished "git reset --hard" &&
 	annote=$(git rev-parse annote) &&
-	git merge --no-commit $annote &&
+	git merge --no-commit --no-ff $annote &&
 	{
 		cat <<-EOF
 		Merge tag '\''$annote'\''
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index dfde6a675a..28a1c43ca7 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -718,7 +718,7 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '
 	git tag -f -s -m "A newer commit" signed &&
 	git reset --hard c0 &&
 
-	EDITOR=false git merge --no-edit signed &&
+	EDITOR=false git merge --no-edit --no-ff signed &&
 	git rev-parse signed^0 >expect &&
 	git rev-parse HEAD^2 >actual &&
 	test_cmp expect actual

[PATCH] merge: allow fast-forward when merging a tracked tag

[Junio C Hamano]

Long time ago at fab47d05 ("merge: force edit and no-ff mode when
merging a tag object", 2011-11-07), "git merge" was made to always
create a merge commit when merging a tag, even when the side branch
being merged is a descendant of the current branch.

This default is good for merges made by upstream maintainers to
integrate work signed by downstream contributors, but will leave
pointless no-ff merges when downstream contributors pull a newer
release tag to make their long-running topic branches catch up with
the upstream.  When there is no local work left on the topic, such a
merge should simply fast-forward to the commit pointed at by the
release tag.

Update the default (again) for "git merge" that merges a tag object
to (1) --no-ff (i.e. create a merge commit even when side branch
fast forwards) if the tag being merged is not at its expected place
in refs/tags/ hierarchy and (2) --ff (i.e. allow fast-forward update
when able) otherwise.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

    > There are a few fallouts in the testsuite if we go this route.  I am
    > not quite decided if I like the approach.

    This time with a few tests for the new default, and minimum
    adjustement to the documentation.

 Documentation/merge-options.txt |  3 ++-
 builtin/merge.c                 | 42 +++++++++++++++++++++++++++++++++++++----
 t/t6200-fmt-merge-msg.sh        |  2 +-
 t/t7600-merge.sh                | 38 ++++++++++++++++++++++++++++++++++++-
 4 files changed, 78 insertions(+), 7 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 3888c3ff85..63a3fc0954 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -35,7 +35,8 @@ set to `no` at the beginning of them.
 --no-ff::
 	Create a merge commit even when the merge resolves as a
 	fast-forward.  This is the default behaviour when merging an
-	annotated (and possibly signed) tag.
+	annotated (and possibly signed) tag that is not stored in
+	its natural place in 'refs/tags/' hierarchy.
 
 --ff-only::
 	Refuse to merge and exit with a non-zero status unless the
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..45c7916505 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -33,6 +33,7 @@
 #include "sequencer.h"
 #include "string-list.h"
 #include "packfile.h"
+#include "tag.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -1125,6 +1126,42 @@ static struct commit_list *collect_parents(struct commit *head_commit,
 	return remoteheads;
 }
 
+static int merging_a_throwaway_tag(struct commit *commit)
+{
+	const char *tag_ref;
+	struct object_id oid;
+
+	/* Are we merging a tag? */
+	if (!merge_remote_util(commit) ||
+	    !merge_remote_util(commit)->obj ||
+	    merge_remote_util(commit)->obj->type != OBJ_TAG)
+		return 0;
+
+	/*
+	 * Now we know we are merging a tag object.  Are we downstream
+	 * and following the tags from upstream?  If so, we must have
+	 * the tag object pointed at by "refs/tags/$T" where $T is the
+	 * tagname recorded in the tag object.  We want to allow such
+	 * a "just to catch up" merge to fast-forward.
+	 */
+	tag_ref = xstrfmt("refs/tags/%s",
+			  ((struct tag *)merge_remote_util(commit)->obj)->tag);
+
+	if (!read_ref(tag_ref, &oid) &&
+	    !oidcmp(&oid, &merge_remote_util(commit)->obj->oid))
+		return 0;
+
+	/*
+	 * Otherwise, we are playing an integrator's role, making a
+	 * merge with a throw-away tag from a contributor with
+	 * something like "git pull $contributor $signed_tag".
+	 * We want to forbid such a merge from fast-forwarding
+	 * by default; otherwise we would not keep the signature
+	 * anywhere.
+	 */
+	return 1;
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	struct object_id result_tree, stash, head_oid;
@@ -1322,10 +1359,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    oid_to_hex(&commit->object.oid));
 		setenv(buf.buf, merge_remote_util(commit)->name, 1);
 		strbuf_reset(&buf);
-		if (fast_forward != FF_ONLY &&
-		    merge_remote_util(commit) &&
-		    merge_remote_util(commit)->obj &&
-		    merge_remote_util(commit)->obj->type == OBJ_TAG)
+		if (fast_forward != FF_ONLY && merging_a_throwaway_tag(commit))
 			fast_forward = FF_NO;
 	}
 
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 2e2fb0e957..a54a52aaa4 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -512,7 +512,7 @@ test_expect_success 'merge-msg with "merging" an annotated tag' '
 
 	test_when_finished "git reset --hard" &&
 	annote=$(git rev-parse annote) &&
-	git merge --no-commit $annote &&
+	git merge --no-commit --no-ff $annote &&
 	{
 		cat <<-EOF
 		Merge tag '\''$annote'\''
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index dfde6a675a..6736d8d131 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -700,6 +700,42 @@ test_expect_success 'merge --no-ff --edit' '
 	test_cmp expected actual
 '
 
+test_expect_success 'merge annotated/signed tag w/o tracking' '
+	test_when_finished "rm -rf dst; git tag -d anno1" &&
+	git tag -a -m "anno c1" anno1 c1 &&
+	git init dst &&
+	git rev-parse c1 >dst/expect &&
+	(
+		# c0 fast-forwards to c1 but because this repository
+		# is not a "downstream" whose refs/tags follows along
+		# tag from the "upstream", this pull defaults to --no-ff
+		cd dst &&
+		git pull .. c0 &&
+		git pull .. anno1 &&
+		git rev-parse HEAD^2 >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'merge annotated/signed tag w/ tracking' '
+	test_when_finished "rm -rf dst; git tag -d anno1" &&
+	git tag -a -m "anno c1" anno1 c1 &&
+	git init dst &&
+	git rev-parse c1 >dst/expect &&
+	(
+		# c0 fast-forwards to c1 and because this repository
+		# is a "downstream" whose refs/tags follows along
+		# tag from the "upstream", this pull defaults to --ff
+		cd dst &&
+		git remote add origin .. &&
+		git pull origin c0 &&
+		git fetch origin &&
+		git merge anno1 &&
+		git rev-parse HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success GPG 'merge --ff-only tag' '
 	git reset --hard c0 &&
 	git commit --allow-empty -m "A newer commit" &&
@@ -718,7 +754,7 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '
 	git tag -f -s -m "A newer commit" signed &&
 	git reset --hard c0 &&
 
-	EDITOR=false git merge --no-edit signed &&
+	EDITOR=false git merge --no-edit --no-ff signed &&
 	git rev-parse signed^0 >expect &&
 	git rev-parse HEAD^2 >actual &&
 	test_cmp expect actual
-- 
2.16.1-194-gb2e45c695d

Re: [PATCH] merge: allow fast-forward when merging a tracked tag

[Eric Sunshine]

On Thu, Feb 15, 2018 at 5:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
> [...]
> Update the default (again) for "git merge" that merges a tag object
> to (1) --no-ff (i.e. create a merge commit even when side branch
> fast forwards) if the tag being merged is not at its expected place
> in refs/tags/ hierarchy and (2) --ff (i.e. allow fast-forward update
> when able) otherwise.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
> diff --git a/builtin/merge.c b/builtin/merge.c
> @@ -1125,6 +1126,42 @@ static struct commit_list *collect_parents(struct commit *head_commit,
> +static int merging_a_throwaway_tag(struct commit *commit)
> +{
> +       const char *tag_ref;
> +       struct object_id oid;
> +
> +       /* Are we merging a tag? */
> +       if (!merge_remote_util(commit) ||
> +           !merge_remote_util(commit)->obj ||
> +           merge_remote_util(commit)->obj->type != OBJ_TAG)
> +               return 0;
> +
> +       /*
> +        * Now we know we are merging a tag object.  Are we downstream
> +        * and following the tags from upstream?  If so, we must have
> +        * the tag object pointed at by "refs/tags/$T" where $T is the
> +        * tagname recorded in the tag object.  We want to allow such
> +        * a "just to catch up" merge to fast-forward.
> +        */
> +       tag_ref = xstrfmt("refs/tags/%s",
> +                         ((struct tag *)merge_remote_util(commit)->obj)->tag);

xstrfmt() allocates a new string...

> +       if (!read_ref(tag_ref, &oid) &&
> +           !oidcmp(&oid, &merge_remote_util(commit)->obj->oid))
> +               return 0;

...which is leaked here...

> +
> +       /*
> +        * Otherwise, we are playing an integrator's role, making a
> +        * merge with a throw-away tag from a contributor with
> +        * something like "git pull $contributor $signed_tag".
> +        * We want to forbid such a merge from fast-forwarding
> +        * by default; otherwise we would not keep the signature
> +        * anywhere.
> +        */
> +       return 1;

...and here.

> +}

Re: [PATCH] merge: allow fast-forward when merging a tracked tag

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

>> +       tag_ref = xstrfmt("refs/tags/%s",
>> +                         ((struct tag *)merge_remote_util(commit)->obj)->tag);
>
> xstrfmt() allocates a new string...
>
>> +       if (!read_ref(tag_ref, &oid) &&
>> +           !oidcmp(&oid, &merge_remote_util(commit)->obj->oid))
>> +               return 0;
>
> ...which is leaked here...
>
>> +
>> +       /*
>> +        * Otherwise, we are playing an integrator's role, making a
>> +        * merge with a throw-away tag from a contributor with
>> +        * something like "git pull $contributor $signed_tag".
>> +        * We want to forbid such a merge from fast-forwarding
>> +        * by default; otherwise we would not keep the signature
>> +        * anywhere.
>> +        */
>> +       return 1;
>
> ...and here.

OK.  Thanks.

[PATCH v2] merge: allow fast-forward when merging a tracked tag

[Junio C Hamano]

Long time ago at fab47d05 ("merge: force edit and no-ff mode when
merging a tag object", 2011-11-07), "git merge" was made to always
create a merge commit when merging a tag, even when the side branch
being merged is a descendant of the current branch.

This default is good for merges made by upstream maintainers to
integrate work signed by downstream contributors, but will leave
pointless no-ff merges when downstream contributors pull a newer
release tag to make their long-running topic branches catch up with
the upstream.  When there is no local work left on the topic, such a
merge should simply fast-forward to the commit pointed at by the
release tag.

Update the default (again) for "git merge" that merges a tag object
to (1) --no-ff (i.e. create a merge commit even when side branch
fast forwards) if the tag being merged is not at its expected place
in refs/tags/ hierarchy and (2) --ff (i.e. allow fast-forward update
when able) otherwise.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * As "git pull linus v4.16-rc1" does not create refs/tags/v4.16-rc1
   during fetch, and there is no way to mechanically tell the
   invocation from "git pull davem for-linus" which does not create
   refs/tags/for-linus, the new default that fast-forwards would
   kick in only when "git fetch linus" followed by possibly other
   other work and concluded with "git merge v4.16-rc1".  So we still
   need the "education" part to ensure that downstream does not make
   pointless merges with "git pull --ff-only".  

   With or without this patch, the other condition that disables the
   "when merging a tag, do not allow fast-forward" default is
   "unless --ff-only is given".  We may want to rethink it.  For
   example, shouldn't "git pull --ff linus v4.16-rc1" enough clue
   that the user _knows_ that the signature in that release tag will
   be lost if the current branch has no original development and
   explicitly _accepts_ fast-forwarding behaviour?  When the current
   branch may or may not have original development, having to say
   "pull --ff-only" while catching up to the upstream and accepting
   50% chance of seeing it fail (when we do have some original
   development hence the update does not fast-forward) smells like a
   clunky interface.

 Documentation/merge-options.txt |  3 ++-
 builtin/merge.c                 | 43 +++++++++++++++++++++++++++++++++++++----
 t/t6200-fmt-merge-msg.sh        |  2 +-
 t/t7600-merge.sh                | 38 +++++++++++++++++++++++++++++++++++-
 4 files changed, 79 insertions(+), 7 deletions(-)

diff --git a/Documentation/merge-options.txt b/Documentation/merge-options.txt
index 3888c3ff85..63a3fc0954 100644
--- a/Documentation/merge-options.txt
+++ b/Documentation/merge-options.txt
@@ -35,7 +35,8 @@ set to `no` at the beginning of them.
 --no-ff::
 	Create a merge commit even when the merge resolves as a
 	fast-forward.  This is the default behaviour when merging an
-	annotated (and possibly signed) tag.
+	annotated (and possibly signed) tag that is not stored in
+	its natural place in 'refs/tags/' hierarchy.
 
 --ff-only::
 	Refuse to merge and exit with a non-zero status unless the
diff --git a/builtin/merge.c b/builtin/merge.c
index 30264cfd7c..532522a854 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -33,6 +33,7 @@
 #include "sequencer.h"
 #include "string-list.h"
 #include "packfile.h"
+#include "tag.h"
 
 #define DEFAULT_TWOHEAD (1<<0)
 #define DEFAULT_OCTOPUS (1<<1)
@@ -1125,6 +1126,43 @@ static struct commit_list *collect_parents(struct commit *head_commit,
 	return remoteheads;
 }
 
+static int merging_a_throwaway_tag(struct commit *commit)
+{
+	char *tag_ref;
+	struct object_id oid;
+	int is_throwaway_tag = 0;
+
+	/* Are we merging a tag? */
+	if (!merge_remote_util(commit) ||
+	    !merge_remote_util(commit)->obj ||
+	    merge_remote_util(commit)->obj->type != OBJ_TAG)
+		return is_throwaway_tag;
+
+	/*
+	 * Now we know we are merging a tag object.  Are we downstream
+	 * and following the tags from upstream?  If so, we must have
+	 * the tag object pointed at by "refs/tags/$T" where $T is the
+	 * tagname recorded in the tag object.  We want to allow such
+	 * a "just to catch up" merge to fast-forward.
+	 *
+	 * Otherwise, we are playing an integrator's role, making a
+	 * merge with a throw-away tag from a contributor with
+	 * something like "git pull $contributor $signed_tag".
+	 * We want to forbid such a merge from fast-forwarding
+	 * by default; otherwise we would not keep the signature
+	 * anywhere.
+	 */
+	tag_ref = xstrfmt("refs/tags/%s",
+			  ((struct tag *)merge_remote_util(commit)->obj)->tag);
+	if (!read_ref(tag_ref, &oid) &&
+	    !oidcmp(&oid, &merge_remote_util(commit)->obj->oid))
+		is_throwaway_tag = 0;
+	else
+		is_throwaway_tag = 1;
+	free(tag_ref);
+	return is_throwaway_tag;
+}
+
 int cmd_merge(int argc, const char **argv, const char *prefix)
 {
 	struct object_id result_tree, stash, head_oid;
@@ -1322,10 +1360,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 			    oid_to_hex(&commit->object.oid));
 		setenv(buf.buf, merge_remote_util(commit)->name, 1);
 		strbuf_reset(&buf);
-		if (fast_forward != FF_ONLY &&
-		    merge_remote_util(commit) &&
-		    merge_remote_util(commit)->obj &&
-		    merge_remote_util(commit)->obj->type == OBJ_TAG)
+		if (fast_forward != FF_ONLY && merging_a_throwaway_tag(commit))
 			fast_forward = FF_NO;
 	}
 
diff --git a/t/t6200-fmt-merge-msg.sh b/t/t6200-fmt-merge-msg.sh
index 2e2fb0e957..a54a52aaa4 100755
--- a/t/t6200-fmt-merge-msg.sh
+++ b/t/t6200-fmt-merge-msg.sh
@@ -512,7 +512,7 @@ test_expect_success 'merge-msg with "merging" an annotated tag' '
 
 	test_when_finished "git reset --hard" &&
 	annote=$(git rev-parse annote) &&
-	git merge --no-commit $annote &&
+	git merge --no-commit --no-ff $annote &&
 	{
 		cat <<-EOF
 		Merge tag '\''$annote'\''
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index dfde6a675a..6736d8d131 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -700,6 +700,42 @@ test_expect_success 'merge --no-ff --edit' '
 	test_cmp expected actual
 '
 
+test_expect_success 'merge annotated/signed tag w/o tracking' '
+	test_when_finished "rm -rf dst; git tag -d anno1" &&
+	git tag -a -m "anno c1" anno1 c1 &&
+	git init dst &&
+	git rev-parse c1 >dst/expect &&
+	(
+		# c0 fast-forwards to c1 but because this repository
+		# is not a "downstream" whose refs/tags follows along
+		# tag from the "upstream", this pull defaults to --no-ff
+		cd dst &&
+		git pull .. c0 &&
+		git pull .. anno1 &&
+		git rev-parse HEAD^2 >actual &&
+		test_cmp expect actual
+	)
+'
+
+test_expect_success 'merge annotated/signed tag w/ tracking' '
+	test_when_finished "rm -rf dst; git tag -d anno1" &&
+	git tag -a -m "anno c1" anno1 c1 &&
+	git init dst &&
+	git rev-parse c1 >dst/expect &&
+	(
+		# c0 fast-forwards to c1 and because this repository
+		# is a "downstream" whose refs/tags follows along
+		# tag from the "upstream", this pull defaults to --ff
+		cd dst &&
+		git remote add origin .. &&
+		git pull origin c0 &&
+		git fetch origin &&
+		git merge anno1 &&
+		git rev-parse HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_expect_success GPG 'merge --ff-only tag' '
 	git reset --hard c0 &&
 	git commit --allow-empty -m "A newer commit" &&
@@ -718,7 +754,7 @@ test_expect_success GPG 'merge --no-edit tag should skip editor' '
 	git tag -f -s -m "A newer commit" signed &&
 	git reset --hard c0 &&
 
-	EDITOR=false git merge --no-edit signed &&
+	EDITOR=false git merge --no-edit --no-ff signed &&
 	git rev-parse signed^0 >expect &&
 	git rev-parse HEAD^2 >actual &&
 	test_cmp expect actual
-- 
2.16.2-246-ga4ee44448f