Re: linux-next: unnecessary merge in the v4l-dvb tree
Linus Torvalds writes: > On Tue, Feb 13, 2018 at 9:18 AM, Junio C Hamano 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 '\'
Re: linux-next: unnecessary merge in the v4l-dvb tree
On Tue, Feb 13, 2018 at 9:18 AM, Junio C Hamano 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
Mauro Carvalho Chehab 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 > (or git pull . ) > > 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
Em Mon, 12 Feb 2018 15:42:44 -0800 Junio C Hamano escreveu: > Linus Torvalds 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 (or git pull . ) 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
Linus Torvalds 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
On Mon, Feb 12, 2018 at 1:44 PM, Junio C Hamano 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
Linus Torvalds 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
On Mon, Feb 12, 2018 at 1:15 PM, Linus Torvalds 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
Linus Torvalds writes: > On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell > 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..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
Em Mon, 12 Feb 2018 13:15:04 -0800 Linus Torvalds escreveu: > On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell > 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
On Mon, Feb 12, 2018 at 1:00 PM, Stephen Rothwell 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