| Message ID | 20200730180237.1392480-1-gitster@pobox.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | fmt-merge-msg: selectively suppress "into <branch>" | expand |
On Thu, Jul 30, 2020 at 11:02:35AM -0700, Junio C Hamano wrote: > So, this is the third iteration, which separates the reversion that > involves too many uninteresting test changes, and the implementation > of the new feature proper. Thanks, that did make it much easier to read. > Two things that changed in the implementation since the previous > iterations are > > - An assignment-less 'truth', i.e. > > [merge] suppressDest > > is now an error ("missing value for 'merge.suppressDest'"), not > "clear the list", which should be spelled as an empty string, i.e. > > [merge] suppressDest = "" OK. I don't have a preference either way on the "truth" behavior, but certainly considering it an error is the conservative thing. > - Scanning of dest_patterns to see if "into <branch>" is suppressed > for the current_branch is done in a helper function to make the > main codeflow easier to read, as suggested by Peff. Thanks. This version looks OK to me. The remaining issues that came up in earlier discussion but I didn't see you weigh in on are: - what should happen with a detached HEAD? We'd match HEAD in the suppressDest config, which I think is quite reasonable. Not sure if it's worth documenting or testing that specifically. - should "master" be in the list even if you configure a value? That would do the wrong thing if you have a non-integration master, but that seems unlikely. And it would do the right thing if somebody later puts "main" in merge.suppressDest, but still occasionally works with "master" repos (where "right" is defined as "what they probably wanted", but it is perhaps a bit magical). - what's the plan if we do switch init.defaultBranch to "main"? Would we add default_branch() to the list of defaults alongside "master", or just add "main", or just leave it and let people configure independently? It doesn't need to be decided now, but maybe worth thinking about. -Peff
Jeff King <peff@peff.net> writes: > This version looks OK to me. The remaining issues that came up in > earlier discussion but I didn't see you weigh in on are: > > - what should happen with a detached HEAD? We'd match HEAD in the > suppressDest config, which I think is quite reasonable. Not sure if > it's worth documenting or testing that specifically. I think what the code with posted patch happens to do is just fine. If you say "git -c merge.suppressDest=HEA? merge $topic" while on a detached HEAD, you'll get "into HEAD" omitted. In a workflow where you'd do $ git co master^0 ... enumerate the topics merged in master..jch, and redo the ... merge, but with updated versions of these topics $ git shortlog jch..HEAD $ git range-diff jch...HEAD $ git diff jch..HEAD ... after inspection, find the result satisfactory, and ... $ git co -B jch I would strongly suspect that hiding "into HEAD" is not enough, so I do not think HEAD is all that relevant in the first place. You'd rather want to "lie" about the destination branch while redoing these merges, perhaps with $ git merge --pretend-dest=jch topic-name with your HEAD detached, and tell fmt-merge-msg to pretend that the merge is being made into jch branch. And that is outside the scope of this patch, though it might be a good #leftoverbits candidate. > - should "master" be in the list even if you configure a value? That > would do the wrong thing if you have a non-integration master, but > that seems unlikely. And it would do the right thing if somebody > later puts "main" in merge.suppressDest, but still occasionally > works with "master" repos (where "right" is defined as "what they > probably wanted", but it is perhaps a bit magical). If you configure, you can configure it fully without manually clearing first. If you do not configure, you get a backward compatible default. I think that is the only sensible semantics. Besides, I thought we were aiming to make 'master' less special. When a user already has a concrete list of things to use shorter merge title for, why should 'master' be magically added to the list and force the user to explicitly clear it? I do not think that makes much sense. > - what's the plan if we do switch init.defaultBranch to "main"? Would > we add default_branch() to the list of defaults alongside "master", > or just add "main", or just leave it and let people configure > independently? It doesn't need to be decided now, but maybe worth > thinking about. My understanding is that much more instances of repositories come to exist by cloning than running "git init". Hence, the value you set to the init.defaultBranch has no relevance to the name of the primary branch in majority of your repositories, whose primary branch is what their origin has designated before/when you cloned. And the latter, "what is the primary branch name for this particular repository?", is what we want to ask here. The answer to "what is the first branch name for new repository I will create?" is not a good proxy for that. I do not mind too much, even though I doubt it will be all that useful, if we taught "init" and "clone" to record which branch is the primary one in the repository they created. We'd need to add the repo_primary_branch_name() helper to allow this caller to replace the hardcoded 'master' in the patch with it, just like "init" and "clone" may ask the repo_default_branch_name() helper what the first branch name ought to be. In any case, I do not think I want to see more reliance of the notion that there always is one and only one single special branch in the repository, so if we can get away without it, that would be more preferrable. Thanks.
On Thu, Jul 30, 2020 at 07:04:15PM -0700, Junio C Hamano wrote: > You'd rather want to "lie" about the destination branch while > redoing these merges, perhaps with > > $ git merge --pretend-dest=jch topic-name > > with your HEAD detached, and tell fmt-merge-msg to pretend that the > merge is being made into jch branch. And that is outside the scope > of this patch, though it might be a good #leftoverbits candidate. Since nobody really asked for it, it may make sense to wait for such a feature. After all, this is the just the starting text we put into the merge message. You are always free to add the pretend branch yourself in the editor. > > - should "master" be in the list even if you configure a value? That > > would do the wrong thing if you have a non-integration master, but > > that seems unlikely. And it would do the right thing if somebody > > later puts "main" in merge.suppressDest, but still occasionally > > works with "master" repos (where "right" is defined as "what they > > probably wanted", but it is perhaps a bit magical). > > If you configure, you can configure it fully without manually > clearing first. If you do not configure, you get a backward > compatible default. I think that is the only sensible semantics. > > Besides, I thought we were aiming to make 'master' less special. > When a user already has a concrete list of things to use shorter > merge title for, why should 'master' be magically added to the list > and force the user to explicitly clear it? I do not think that > makes much sense. It's magic-ness would be purely for backwards compatibility. IMHO maintaining exact behavior with respect to this particular case was not a big deal, but clearly Linus disagrees. But the "do the right thing above" I mentioned above is "do the right thing even if the user _did_ switch their config to a new name, but forgot that they sometimes are working with old repos". So it is perhaps an even weaker reason. To be clear, I'm OK with the behavior in your patch. I just wanted to make sure we thought through all of the implications. > > - what's the plan if we do switch init.defaultBranch to "main"? Would > > we add default_branch() to the list of defaults alongside "master", > > or just add "main", or just leave it and let people configure > > independently? It doesn't need to be decided now, but maybe worth > > thinking about. > [...quite reasonable analysis that I agree with...] > > In any case, I do not think I want to see more reliance of the > notion that there always is one and only one single special branch > in the repository, so if we can get away without it, that would be > more preferrable. Yeah, if the plan is to stop here then I'm OK with that. That makes "master" special for historical reasons, but "main" or whatever never got this special treatment by default. People have the ability to configure if they choose, or they may not care either way. We might get a feature request later that says "gee, I wish we did this for 'main' by default without me having to configure it", but we can cross that bridge when we come to it. -Peff
On Thu, Jul 30, 2020 at 10:22:17PM -0400, Jeff King wrote: > On Thu, Jul 30, 2020 at 07:04:15PM -0700, Junio C Hamano wrote: > > > You'd rather want to "lie" about the destination branch while > > redoing these merges, perhaps with > > > > $ git merge --pretend-dest=jch topic-name > > > > with your HEAD detached, and tell fmt-merge-msg to pretend that the > > merge is being made into jch branch. And that is outside the scope > > of this patch, though it might be a good #leftoverbits candidate. > > Since nobody really asked for it, it may make sense to wait for such a > feature. After all, this is the just the starting text we put into the > merge message. You are always free to add the pretend branch yourself in > the editor. > > > > - should "master" be in the list even if you configure a value? That > > > would do the wrong thing if you have a non-integration master, but > > > that seems unlikely. And it would do the right thing if somebody > > > later puts "main" in merge.suppressDest, but still occasionally > > > works with "master" repos (where "right" is defined as "what they > > > probably wanted", but it is perhaps a bit magical). > > > > If you configure, you can configure it fully without manually > > clearing first. If you do not configure, you get a backward > > compatible default. I think that is the only sensible semantics. > > > > Besides, I thought we were aiming to make 'master' less special. > > When a user already has a concrete list of things to use shorter > > merge title for, why should 'master' be magically added to the list > > and force the user to explicitly clear it? I do not think that > > makes much sense. > > It's magic-ness would be purely for backwards compatibility. IMHO > maintaining exact behavior with respect to this particular case was not > a big deal, but clearly Linus disagrees. But the "do the right thing > above" I mentioned above is "do the right thing even if the user _did_ > switch their config to a new name, but forgot that they sometimes are > working with old repos". So it is perhaps an even weaker reason. I think that you could do this without treating 'master' as specially by making 'merge.suppressDest' contain the value of 'init.defaultBranch' (unless set otherwise). This gets tricky when the fall-back value for 'init.defaultBranch' changes, though. If it were to go from 'master' -> 'main', you'd want to have both of those defaults in your 'merge.suppressDest' list, to avoid breaking clients who still use 'master' (and expect 'into master' not to show up in their merges). So, I guess the rule would be: 'merge.suppressDest' contains the value of 'init.defaultBranch' (or its default value) along with any previous default values for 'init.defaultBranch', unless specified otherwise. Apologies if this has already been suggested elsewhere and I glossed past it. > To be clear, I'm OK with the behavior in your patch. I just wanted to > make sure we thought through all of the implications. I am too. > > > - what's the plan if we do switch init.defaultBranch to "main"? Would > > > we add default_branch() to the list of defaults alongside "master", > > > or just add "main", or just leave it and let people configure > > > independently? It doesn't need to be decided now, but maybe worth > > > thinking about. > > [...quite reasonable analysis that I agree with...] > > > > In any case, I do not think I want to see more reliance of the > > notion that there always is one and only one single special branch > > in the repository, so if we can get away without it, that would be > > more preferrable. > > Yeah, if the plan is to stop here then I'm OK with that. That makes > "master" special for historical reasons, but "main" or whatever never > got this special treatment by default. People have the ability to > configure if they choose, or they may not care either way. > > We might get a feature request later that says "gee, I wish we did this > for 'main' by default without me having to configure it", but we can > cross that bridge when we come to it. > > -Peff Thanks, Taylor
Taylor Blau <me@ttaylorr.com> writes: > I think that you could do this without treating 'master' as specially by > making 'merge.suppressDest' contain the value of 'init.defaultBranch' > (unless set otherwise). My understanding is that much more instances of repositories come to exist by cloning than running "git init". Hence, the value you set to the init.defaultBranch has no relevance to the name of the primary branch in majority of your repositories, whose primary branch is what their origin has designated before/when you cloned. And the latter, "what is the primary branch name for this particular repository?", is what we want to ask here. The answer to "what is the first branch name for new repository I will create?" is not a good proxy for that. Thanks.
On Fri, Jul 31, 2020 at 01:12:02PM -0700, Junio C Hamano wrote: > Taylor Blau <me@ttaylorr.com> writes: > > > I think that you could do this without treating 'master' as specially by > > making 'merge.suppressDest' contain the value of 'init.defaultBranch' > > (unless set otherwise). > > My understanding is that much more instances of repositories come to > exist by cloning than running "git init". Hence, the value you set > to the init.defaultBranch has no relevance to the name of the > primary branch in majority of your repositories, whose primary > branch is what their origin has designated before/when you cloned. > > And the latter, "what is the primary branch name for this particular > repository?", is what we want to ask here. The answer to "what is > the first branch name for new repository I will create?" is not a > good proxy for that. Aha, makes sense. Thanks for clarifying. > Thanks. Thanks, Taylor
On Fri, Jul 31, 2020 at 04:03:06PM -0400, Taylor Blau wrote: > On Thu, Jul 30, 2020 at 10:22:17PM -0400, Jeff King wrote: > > On Thu, Jul 30, 2020 at 07:04:15PM -0700, Junio C Hamano wrote: > > > > > You'd rather want to "lie" about the destination branch while > > > redoing these merges, perhaps with > > > > > > $ git merge --pretend-dest=jch topic-name > > > > > > with your HEAD detached, and tell fmt-merge-msg to pretend that the > > > merge is being made into jch branch. And that is outside the scope > > > of this patch, though it might be a good #leftoverbits candidate. > > > > Since nobody really asked for it, it may make sense to wait for such a > > feature. After all, this is the just the starting text we put into the > > merge message. You are always free to add the pretend branch yourself in > > the editor. > > > > > > - should "master" be in the list even if you configure a value? That > > > > would do the wrong thing if you have a non-integration master, but > > > > that seems unlikely. And it would do the right thing if somebody > > > > later puts "main" in merge.suppressDest, but still occasionally > > > > works with "master" repos (where "right" is defined as "what they > > > > probably wanted", but it is perhaps a bit magical). > > > > > > If you configure, you can configure it fully without manually > > > clearing first. If you do not configure, you get a backward > > > compatible default. I think that is the only sensible semantics. > > > > > > Besides, I thought we were aiming to make 'master' less special. > > > When a user already has a concrete list of things to use shorter > > > merge title for, why should 'master' be magically added to the list > > > and force the user to explicitly clear it? I do not think that > > > makes much sense. > > > > It's magic-ness would be purely for backwards compatibility. IMHO > > maintaining exact behavior with respect to this particular case was not > > a big deal, but clearly Linus disagrees. But the "do the right thing > > above" I mentioned above is "do the right thing even if the user _did_ > > switch their config to a new name, but forgot that they sometimes are > > working with old repos". So it is perhaps an even weaker reason. > > I think that you could do this without treating 'master' as specially by > making 'merge.suppressDest' contain the value of 'init.defaultBranch' > (unless set otherwise). > > This gets tricky when the fall-back value for 'init.defaultBranch' > changes, though. If it were to go from 'master' -> 'main', you'd want to > have both of those defaults in your 'merge.suppressDest' list, to avoid > breaking clients who still use 'master' (and expect 'into master' not to > show up in their merges). > > So, I guess the rule would be: 'merge.suppressDest' contains the value > of 'init.defaultBranch' (or its default value) along with any previous > default values for 'init.defaultBranch', unless specified otherwise. > IMHO this is way better than spome magic variable that you ahve to assign magic value for it to have teh value you assign. Seen this in systemd and it is not very nice to deal with. Thanks Michal
So, this is the third iteration, which separates the reversion that involves too many uninteresting test changes, and the implementation of the new feature proper. Two things that changed in the implementation since the previous iterations are - An assignment-less 'truth', i.e. [merge] suppressDest is now an error ("missing value for 'merge.suppressDest'"), not "clear the list", which should be spelled as an empty string, i.e. [merge] suppressDest = "" - Scanning of dest_patterns to see if "into <branch>" is suppressed for the current_branch is done in a helper function to make the main codeflow easier to read, as suggested by Peff. The previous iterations were v1 <xmqqlfj27x7q.fsf@gitster.c.googlers.com> v2 <xmqq5za596uo.fsf@gitster.c.googlers.com> and can be found at https://lore.kernel.org/git/ Junio C Hamano (2): Revert "fmt-merge-msg: stop treating `master` specially" fmt-merge-msg: allow merge destination to be omitted again Documentation/config/fmt-merge-msg.txt | 12 ++++ fmt-merge-msg.c | 35 ++++++++- t/t1507-rev-parse-upstream.sh | 2 +- t/t4013-diff-various.sh | 4 +- t/t4013/diff.log_--decorate=full_--all | 2 +- t/t4013/diff.log_--decorate_--all | 2 +- ...--patch-with-stat_--summary_master_--_dir_ | 2 +- t/t4013/diff.log_--patch-with-stat_master | 2 +- .../diff.log_--patch-with-stat_master_--_dir_ | 2 +- ...ot_--cc_--patch-with-stat_--summary_master | 2 +- ..._--root_--patch-with-stat_--summary_master | 2 +- .../diff.log_--root_--patch-with-stat_master | 2 +- ...root_-c_--patch-with-stat_--summary_master | 2 +- t/t4013/diff.log_--root_-p_master | 2 +- t/t4013/diff.log_--root_master | 2 +- t/t4013/diff.log_-m_-p_--first-parent_master | 2 +- t/t4013/diff.log_-m_-p_master | 4 +- t/t4013/diff.log_-p_--first-parent_master | 2 +- t/t4013/diff.log_-p_master | 2 +- t/t4013/diff.log_master | 2 +- t/t4013/diff.show_--first-parent_master | 2 +- t/t4013/diff.show_-c_master | 2 +- t/t4013/diff.show_-m_master | 4 +- t/t4013/diff.show_master | 2 +- ...ot_--cc_--patch-with-stat_--summary_master | 2 +- ...root_-c_--patch-with-stat_--summary_master | 2 +- t/t4202-log.sh | 72 +++++++++---------- t/t6200-fmt-merge-msg.sh | 56 ++++++++++----- t/t7600-merge.sh | 14 ++-- t/t7608-merge-messages.sh | 10 +-- 30 files changed, 159 insertions(+), 94 deletions(-)