On Fri, May 31, 2019 at 07:23:56PM +0200, Johannes Schindelin wrote: > Hi Denton, > > On Fri, 31 May 2019, Denton Liu wrote: > > > On Fri, May 31, 2019 at 02:58:30PM +0200, Johannes Schindelin wrote: > > > > > On Thu, 30 May 2019, Denton Liu wrote: > > > [...] > > > > > I decided to go ahead and implement the includeIf onbranch semantics > > > > for fun. For completeness, I'm sending it to the list but I'm not > > > > really sure if this should get merged, since I don't really have a > > > > use-case for this, especially if we go the branch-specific > > > > format-patch config route. > > > > > > > > Another thing to note is that this change doesn't completely cover > > > > all the use-cases that the branch-specific format-patch does. In > > > > particular, if I run > > > > > > > > $ git checkout foo > > > > $ git format-patch master..bar > > > > > > > > with the `format.bar.*`, we'd get bar-specific configs, whereas with > > > > `includeIf "onbranch:bar"`, we'd fail to include bar-specific configs > > > > and, more dangerously, we'd be including foo's configs. > > > > > > I actually think that this is fine. "on branch" means that you are on the > > > specified branch, not that you merely mention the branch name on the > > > command-line (in which case there would be the ambiguity "did the user > > > mean `master` or `bar`?"). > > > > The reason why I brought this up as a use case was because currently, > > when format-patch generates a cover letter, with the above, it'll use > > bar's branch description to populate it even if "foo" is checked out. As > > a result, when implementing the branch-specific format-patch stuff, I > > wanted to make this consistent so that we wouldn't end up in a situation > > where the cover letter has the branch's description but is missing its > > Cc's. > > That strikes me as a different use case than `includeIf`. I could imagine > that you'd want a setting like `formatpatch.detecttargetbranch = auto` or > some such that would pick up the `format.bar*` settings if there was *one* > rev argument, and it was a commit range (or a tip commit), *and* it > obviously referred to a single target branch. Correct. For context, upthread I initially implemented the branch-specific format-patch configs but Ævar suggested that we implement the onbranch config semantics instead. So I was just addressing the fact that this patch can't supercede the branch-specific format-patch stuff since they have different use cases so the other patchset has to coexist with this one. I'm happy to see that we're both in agreement about this. > > It's just a scenario that is *very* specific to `git format-patch`. > > For example, I would not, ever, want `git log ..next` to pick up a > config specific to `next` just because I mentioned a commit range with > `range` as the tip to start from. Yeah, I'd never dream of implementing something that gross ;) > > Ciao, > Dscho
