On Wed, Jul 29, 2020 at 03:50:01PM -0700, Junio C Hamano wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
> > The fast-export side lifted the "single branch is special"; we
> > didn't do something similar for "fmt-merge-msg".
> >
> >> So I think a path forward is more like:
> >>
> >> 1. Add a new config option to shorten fmt-merge-msg's output when the
> >> destination branch matches it (and this should perhaps not even be
> >> a single name, but a set of globs, which supports more workflows).
> >> Call it merge.suppressDest or something.
> >>
> >> 2. Optionally a repository created with "git init" could copy its
> >> init.defaultBranch into merge.suppressDest. And likewise a clone
> >> might copy the remote HEAD into that variable. I'm not sure if that
> >> is worth doing or not, but it would restore the original behavior
> >> for the most part.
> >
> > Yeah, that sounds like a good plan.
>
> A rough outline I did while waiting for today's integration builds
> to finish looks like this, which does not look _too_ bad. We can
> replace the literal 'master' with the default branch name determined
> at runtime, but I am not sure if that is needed.
This looks like a good direction overall.
I'm on the fence on how magical to make the default. Having "master"
there gets Linus's case back where he wanted without having to configure
anything, which is probably reasonable. I'm not sure if people would
want their init.defaultBranch in addition / instead. Since it's a list
it's tempting to say that those could be added to the list even if the
user has specified some value, but I guess that makes things awkward if
you don't want them (I see you put in a way to clear the list, which is
good; I'm more talking about the fact that people would have to actually
remember to do so in their config).
Just a few comments on the patch itself:
> fmt-merge-msg.c | 23 ++++++++++++++++++++++-
> 1 file changed, 22 insertions(+), 1 deletion(-)
Tests and docs, obviously, but I know this is just a preview. :)
> + } else if (!strcmp(key, "merge.suppressdest")) {
> + if (!value)
> + string_list_clear(&suppress_dest_patterns, 0);
> + else
> + string_list_append(&suppress_dest_patterns, value);
> + suppress_dest_pattern_seen = 1;
I kind of hate the option name, despite being the one who suggested it.
But I don't have anything better. I do like naming it after the specific
action we plan to use it for, and not some "these are default branches"
list, which is too vague.
> @@ -451,7 +461,15 @@ static void fmt_merge_msg_title(struct strbuf *out,
> strbuf_addf(out, " of %s", srcs.items[i].string);
> }
>
> - strbuf_addf(out, " into %s\n", current_branch);
> + for_each_string_list_item(item, &suppress_dest_patterns) {
> + if (!wildmatch(item->string, current_branch, WM_PATHNAME)) {
> + suppress_merge_dest = 1;
> + break;
> + }
> + }
I think a list with globs should be plenty flexible. I really hope
nobody would need to include "foo*" but exclude "*bar" from that. If
they do they can write that patch later.
I think this will be matching branch names, not fully qualified refs.
Seems reasonable, but we should be sure to document that.
This loop might be nicer in a helper function with an early return, if
only to avoid extra local variables in fmt_merge_msg_title().
-Peff
