| Message ID | cover.1558052674.git.liu.denton@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Denton Liu <liu.denton@gmail.com> writes: > diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt > index dc77941c48..d387451573 100644 > --- a/Documentation/config/format.txt > +++ b/Documentation/config/format.txt > @@ -28,14 +28,22 @@ format.headers:: > > format.to:: > format.cc:: > +format.<branch-name>.to:: > +format.<branch-name>.cc:: > Additional recipients to include in a patch to be submitted > - by mail. See the --to and --cc options in > - linkgit:git-format-patch[1]. > + by mail. For the <branch-name> options, the recipients are only > + included if patches are generated for the given <branch-name>. > + See the --to and --cc options in linkgit:git-format-patch[1]. An obvious question that somebody else may raise is: What makes the branch name that special? What guarantees that it would stay to be the *only* thing that affects the choice of these variables? An obvious answer to that is "nothing---we are painting ourselves in a corner we cannot easily get out of with this design". If we want to drive format-patch differently depending on the combination of the worktree location *and* the branch the patches are generated from, we can do something like: [includeif "gitdir:/path/to/worktree/1"] path = one.inc [includeif "gitdir:/path/to/worktree/2"] path = two.inc and then have one.inc/two.inc have customized definition of these format.<branch>.{to,cc,...} variables. But at that point, Ævar's "wouldn't this fit better with includeif" suggestion becomes more and more appropriate. Once we invent the way to combine the conditions for includeIf, it would benefit not just this set of variables but all others that will follow in the future. Having said that, as long as we are fine with the plan to deprecate and remove these three-level variables this patch introdues in the future, I think it is OK to have them as a temporary stop-gap measure. > +format.<branch-name>.coverSubject:: > + When format-patch generates a cover letter for the given > + <branch-name>, use the specified subject for the cover letter > + instead of the generic template. I still think it is a mistake that this has to be given separately and possibly redundantly from the branch description. > +static const char *branch_specific_config[] = { > + "branch", > + "format", > + NULL > +}; Yuck. This will break a workflow where a fixed branch with a known configuration is deleted and recreated over and over again (e.g. think of "for-linus" branches used for request-pull in each merge window). > static void delete_branch_config(const char *branchname) > { > struct strbuf buf = STRBUF_INIT; > - strbuf_addf(&buf, "branch.%s", branchname); > - if (git_config_rename_section(buf.buf, NULL) < 0) > - warning(_("Update of config-file failed")); > + int i; > + for (i = 0; branch_specific_config[i]; i++) { > + strbuf_addf(&buf, "%s.%s", branch_specific_config[i], branchname); > + if (git_config_rename_section(buf.buf, NULL) < 0) > + warning(_("Update of config-file failed")); > + strbuf_reset(&buf); > + } This will hardcode the unwarranted limitation that the second level of the format.*.var hierarchy MUST be branch names and nothing else, won't it?
Hi Junio, On Fri, May 17, 2019 at 01:12:04PM +0900, Junio C Hamano wrote: > Denton Liu <liu.denton@gmail.com> writes: > > > diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt > > index dc77941c48..d387451573 100644 > > --- a/Documentation/config/format.txt > > +++ b/Documentation/config/format.txt > > @@ -28,14 +28,22 @@ format.headers:: > > > > format.to:: > > format.cc:: > > +format.<branch-name>.to:: > > +format.<branch-name>.cc:: > > Additional recipients to include in a patch to be submitted > > - by mail. See the --to and --cc options in > > - linkgit:git-format-patch[1]. > > + by mail. For the <branch-name> options, the recipients are only > > + included if patches are generated for the given <branch-name>. > > + See the --to and --cc options in linkgit:git-format-patch[1]. > > An obvious question that somebody else may raise is: > > What makes the branch name that special? What guarantees that > it would stay to be the *only* thing that affects the choice of > these variables? > > An obvious answer to that is "nothing---we are painting ourselves in > a corner we cannot easily get out of with this design". > > If we want to drive format-patch differently depending on the > combination of the worktree location *and* the branch the patches > are generated from, we can do something like: > > [includeif "gitdir:/path/to/worktree/1"] path = one.inc > [includeif "gitdir:/path/to/worktree/2"] path = two.inc > > and then have one.inc/two.inc have customized definition of these > format.<branch>.{to,cc,...} variables. > > But at that point, Ævar's "wouldn't this fit better with includeif" > suggestion becomes more and more appropriate. Once we invent the > way to combine the conditions for includeIf, it would benefit not > just this set of variables but all others that will follow in the > future. Hmm, I'm starting to like Ævar's idea more the more I think about it. > > Having said that, as long as we are fine with the plan to deprecate > and remove these three-level variables this patch introdues in the > future, I think it is OK to have them as a temporary stop-gap > measure. > > > +format.<branch-name>.coverSubject:: > > + When format-patch generates a cover letter for the given > > + <branch-name>, use the specified subject for the cover letter > > + instead of the generic template. > > I still think it is a mistake that this has to be given separately > and possibly redundantly from the branch description. I forgot about incorporating this. Since we don't need a branch-specific coverSubject anymore, we can push everything into a includeif since now format.<name>.coverSubject doesn't really need to exist. I'm going to repurpose --cover-subject format.coverSubject to be a boolean option which'll mean "process the description and if you can extract a subject out of it, put it on the cover letter". This way, we can maintain backwards compatability in case users have some specific use-case. Unless you'd like this processing to be the default behaviour? I'm impartial either way. > > > +static const char *branch_specific_config[] = { > > + "branch", > > + "format", > > + NULL > > +}; > > Yuck. This will break a workflow where a fixed branch with a known > configuration is deleted and recreated over and over again > (e.g. think of "for-linus" branches used for request-pull in each > merge window). I suppose when we implement `onBranch`, you'd prefer `git branch -d` to also not discard those sections. > > > static void delete_branch_config(const char *branchname) > > { > > struct strbuf buf = STRBUF_INIT; > > - strbuf_addf(&buf, "branch.%s", branchname); > > - if (git_config_rename_section(buf.buf, NULL) < 0) > > - warning(_("Update of config-file failed")); > > + int i; > > + for (i = 0; branch_specific_config[i]; i++) { > > + strbuf_addf(&buf, "%s.%s", branch_specific_config[i], branchname); > > + if (git_config_rename_section(buf.buf, NULL) < 0) > > + warning(_("Update of config-file failed")); > > + strbuf_reset(&buf); > > + } > > This will hardcode the unwarranted limitation that the second level > of the format.*.var hierarchy MUST be branch names and nothing else, > won't it? > I was expecting it to only be branch names but now let's take a different approach. Consider patches 3-6 dropped. I'd like to queue 1-2, though, since they're just cleanup patches. Also, expect a onBranch patchset some time in the future (not the near future, school is busy). Thanks for your feedback, Junio.
Hi Junio, I just realised that my use-cases wouldn't be fully covered with the onBranch configuration option. On Fri, May 17, 2019 at 03:25:15AM -0400, Denton Liu wrote: > Hi Junio, > > On Fri, May 17, 2019 at 01:12:04PM +0900, Junio C Hamano wrote: > > Denton Liu <liu.denton@gmail.com> writes: > > > > > diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt > > > index dc77941c48..d387451573 100644 > > > --- a/Documentation/config/format.txt > > > +++ b/Documentation/config/format.txt > > > @@ -28,14 +28,22 @@ format.headers:: > > > > > > format.to:: > > > format.cc:: > > > +format.<branch-name>.to:: > > > +format.<branch-name>.cc:: > > > Additional recipients to include in a patch to be submitted > > > - by mail. See the --to and --cc options in > > > - linkgit:git-format-patch[1]. > > > + by mail. For the <branch-name> options, the recipients are only > > > + included if patches are generated for the given <branch-name>. > > > + See the --to and --cc options in linkgit:git-format-patch[1]. > > > > An obvious question that somebody else may raise is: > > > > What makes the branch name that special? What guarantees that > > it would stay to be the *only* thing that affects the choice of > > these variables? > > > > An obvious answer to that is "nothing---we are painting ourselves in > > a corner we cannot easily get out of with this design". > > > > If we want to drive format-patch differently depending on the > > combination of the worktree location *and* the branch the patches > > are generated from, we can do something like: > > > > [includeif "gitdir:/path/to/worktree/1"] path = one.inc > > [includeif "gitdir:/path/to/worktree/2"] path = two.inc > > > > and then have one.inc/two.inc have customized definition of these > > format.<branch>.{to,cc,...} variables. > > > > But at that point, Ævar's "wouldn't this fit better with includeif" > > suggestion becomes more and more appropriate. Once we invent the > > way to combine the conditions for includeIf, it would benefit not > > just this set of variables but all others that will follow in the > > future. > > Hmm, I'm starting to like Ævar's idea more the more I think about it. > There is one limitation with onBranch. Suppose someone runs $ git checkout other $ git format-patch master..feature Then, with onBranch, they'd use get the To and Cc of `other`. But with `format.feature.*`, format-patch correctly handles this and will use `feature`'s To and Cc. > > > > Having said that, as long as we are fine with the plan to deprecate > > and remove these three-level variables this patch introdues in the > > future, I think it is OK to have them as a temporary stop-gap > > measure. With this new discovery, I'm not sure it'd be possible to deprecate it without losing a use-case. > > > > > +format.<branch-name>.coverSubject:: > > > + When format-patch generates a cover letter for the given > > > + <branch-name>, use the specified subject for the cover letter > > > + instead of the generic template. > > > > I still think it is a mistake that this has to be given separately > > and possibly redundantly from the branch description. > > I forgot about incorporating this. Since we don't need a branch-specific > coverSubject anymore, we can push everything into a includeif since now > format.<name>.coverSubject doesn't really need to exist. > > I'm going to repurpose --cover-subject format.coverSubject to be a > boolean option which'll mean "process the description and if you can > extract a subject out of it, put it on the cover letter". This way, we > can maintain backwards compatability in case users have some specific > use-case. > > Unless you'd like this processing to be the default behaviour? I'm > impartial either way. > > > > > > +static const char *branch_specific_config[] = { > > > + "branch", > > > + "format", > > > + NULL > > > +}; > > > > Yuck. This will break a workflow where a fixed branch with a known > > configuration is deleted and recreated over and over again > > (e.g. think of "for-linus" branches used for request-pull in each > > merge window). > > I suppose when we implement `onBranch`, you'd prefer `git branch -d` to > also not discard those sections. > > > > > > static void delete_branch_config(const char *branchname) > > > { > > > struct strbuf buf = STRBUF_INIT; > > > - strbuf_addf(&buf, "branch.%s", branchname); > > > - if (git_config_rename_section(buf.buf, NULL) < 0) > > > - warning(_("Update of config-file failed")); > > > + int i; > > > + for (i = 0; branch_specific_config[i]; i++) { > > > + strbuf_addf(&buf, "%s.%s", branch_specific_config[i], branchname); > > > + if (git_config_rename_section(buf.buf, NULL) < 0) > > > + warning(_("Update of config-file failed")); > > > + strbuf_reset(&buf); > > > + } > > > > This will hardcode the unwarranted limitation that the second level > > of the format.*.var hierarchy MUST be branch names and nothing else, > > won't it? > > > > I was expecting it to only be branch names but now let's take a > different approach. > > Consider patches 3-6 dropped. I'd like to queue 1-2, though, since > they're just cleanup patches. In light of this, I don't plan on dropping 3-6 anymore. I'm going to reroll the new behaviour of coverSubject. > > Also, expect a onBranch patchset some time in the future (not the near > future, school is busy). > > Thanks for your feedback, Junio.
diff --git a/Documentation/config/branch.txt b/Documentation/config/branch.txt index d292210cf6..8f4b3faadd 100644 --- a/Documentation/config/branch.txt +++ b/Documentation/config/branch.txt @@ -100,13 +100,3 @@ branch.<name>.description:: `git branch --edit-description`. Branch description is automatically added in the format-patch cover letter or request-pull summary. - -branch.<name>.coverSubject:: - When format-patch generates a cover letter, use the specified - subject for the cover letter instead of the generic template. - -branch.<name>.to:: -branch.<name>.cc:: - Additional recipients to include in a patch to be submitted - by mail. See the --to and --cc options in - linkgit:git-format-patch[1]. diff --git a/Documentation/config/format.txt b/Documentation/config/format.txt index dc77941c48..d387451573 100644 --- a/Documentation/config/format.txt +++ b/Documentation/config/format.txt @@ -28,14 +28,22 @@ format.headers:: format.to:: format.cc:: +format.<branch-name>.to:: +format.<branch-name>.cc:: Additional recipients to include in a patch to be submitted - by mail. See the --to and --cc options in - linkgit:git-format-patch[1]. + by mail. For the <branch-name> options, the recipients are only + included if patches are generated for the given <branch-name>. + See the --to and --cc options in linkgit:git-format-patch[1]. format.subjectPrefix:: The default for format-patch is to output files with the '[PATCH]' subject prefix. Use this variable to change that prefix. +format.<branch-name>.coverSubject:: + When format-patch generates a cover letter for the given + <branch-name>, use the specified subject for the cover letter + instead of the generic template. + format.signature:: The default for format-patch is to output a signature containing the Git version number. Use this variable to change that default. diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt index 1c972f683a..4e826010f6 100644 --- a/Documentation/git-format-patch.txt +++ b/Documentation/git-format-patch.txt @@ -199,7 +199,7 @@ will want to ensure that threading is disabled for `git send-email`. Add a `To:` header to the email headers. This is in addition to any configured headers, and may be used multiple times. The emails given will be used along with any emails given by - `format.to` and `branch.<name>.to` configurations. + `format.to` and `format.<branch-name>.to` configurations. The negated form `--no-to` discards all `To:` headers added so far (from config or command line). @@ -207,7 +207,7 @@ will want to ensure that threading is disabled for `git send-email`. Add a `Cc:` header to the email headers. This is in addition to any configured headers, and may be used multiple times. The emails given will be used along with any emails given by - `format.cc` and `branch.<name>.cc` configurations. + `format.cc` and `format.<branch-name>.cc` configurations. The negated form `--no-cc` discards all `Cc:` headers added so far (from config or command line). @@ -355,7 +355,7 @@ In addition, for a specific branch, you can specify a custom cover letter subject, and add additional "To:" or "Cc:" headers. ------------ -[branch "branch-name"] +[format "branch-name"] coverSubject = "subject for branch-name only" to = <email> cc = <email> diff --git a/branch.c b/branch.c index 83cd441790..643694542a 100644 --- a/branch.c +++ b/branch.c @@ -163,11 +163,11 @@ static void setup_tracking(const char *new_ref, const char *orig_ref, free(tracking.src); } -static int read_branch_config(struct strbuf *buf, const char *branch_name, const char *key) +int read_branch_desc(struct strbuf *buf, const char *branch_name) { char *v = NULL; struct strbuf name = STRBUF_INIT; - strbuf_addf(&name, "branch.%s.%s", branch_name, key); + strbuf_addf(&name, "branch.%s.description", branch_name); if (git_config_get_string(name.buf, &v)) { strbuf_release(&name); return -1; @@ -178,16 +178,6 @@ static int read_branch_config(struct strbuf *buf, const char *branch_name, const return 0; } -int read_branch_desc(struct strbuf *buf, const char *branch_name) -{ - return read_branch_config(buf, branch_name, "description"); -} - -int read_branch_subject(struct strbuf *buf, const char *branch_name) -{ - return read_branch_config(buf, branch_name, "coversubject"); -} - /* * Check if 'name' can be a valid name for a branch; die otherwise. * Return 1 if the named branch already exists; return 0 otherwise. diff --git a/branch.h b/branch.h index 363a4fae9d..6f38db14e9 100644 --- a/branch.h +++ b/branch.h @@ -79,11 +79,6 @@ int install_branch_config(int flag, const char *local, const char *origin, const */ int read_branch_desc(struct strbuf *, const char *branch_name); -/* - * Read branch subject - */ -extern int read_branch_subject(struct strbuf *, const char *branch_name); - /* * Check if a branch is checked out in the main worktree or any linked * worktree and die (with a message describing its checkout location) if diff --git a/builtin/branch.c b/builtin/branch.c index d4359b33ac..367e1fc9bc 100644 --- a/builtin/branch.c +++ b/builtin/branch.c @@ -178,12 +178,22 @@ static int check_branch_commit(const char *branchname, const char *refname, return 0; } +static const char *branch_specific_config[] = { + "branch", + "format", + NULL +}; + static void delete_branch_config(const char *branchname) { struct strbuf buf = STRBUF_INIT; - strbuf_addf(&buf, "branch.%s", branchname); - if (git_config_rename_section(buf.buf, NULL) < 0) - warning(_("Update of config-file failed")); + int i; + for (i = 0; branch_specific_config[i]; i++) { + strbuf_addf(&buf, "%s.%s", branch_specific_config[i], branchname); + if (git_config_rename_section(buf.buf, NULL) < 0) + warning(_("Update of config-file failed")); + strbuf_reset(&buf); + } strbuf_release(&buf); } diff --git a/builtin/log.c b/builtin/log.c index 3adc942b8c..e1d793577d 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1032,6 +1032,21 @@ static void show_diffstat(struct rev_info *rev, fprintf(rev->diffopt.file, "\n"); } +static int read_branch_subject(struct strbuf *buf, const char *branch_name) +{ + char *v = NULL; + struct strbuf name = STRBUF_INIT; + strbuf_addf(&name, "format.%s.coverSubject", branch_name); + if (git_config_get_string(name.buf, &v)) { + strbuf_release(&name); + return -1; + } + strbuf_addstr(buf, v); + free(v); + strbuf_release(&name); + return 0; +} + static void add_branch_headers(struct rev_info *rev, const char *branch_name) { struct strbuf buf = STRBUF_INIT; @@ -1064,7 +1079,7 @@ static void add_branch_headers(struct rev_info *rev, const char *branch_name) */ if (!to_cleared) { - strbuf_addf(&buf, "branch.%s.to", branch_name); + strbuf_addf(&buf, "format.%s.to", branch_name); values = git_config_get_value_multi(buf.buf); if (values) string_list_append_all(&extra_to, values); @@ -1072,7 +1087,7 @@ static void add_branch_headers(struct rev_info *rev, const char *branch_name) if (!cc_cleared) { strbuf_reset(&buf); - strbuf_addf(&buf, "branch.%s.cc", branch_name); + strbuf_addf(&buf, "format.%s.cc", branch_name); values = git_config_get_value_multi(buf.buf); if (values) string_list_append_all(&extra_cc, values); diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh index e9ad50b66d..9ae3da888e 100755 --- a/t/t3200-branch.sh +++ b/t/t3200-branch.sh @@ -782,9 +782,15 @@ test_expect_success 'test tracking setup via --track but deeper' ' ' test_expect_success 'test deleting branch deletes branch config' ' + git config format.my7.coverSubject "cover subject" && + git config format.my7.to "To Me <to@example.com>" && + git config format.my7.cc "Cc Me <cc@example.com>" && git branch -d my7 && test -z "$(git config branch.my7.remote)" && - test -z "$(git config branch.my7.merge)" + test -z "$(git config branch.my7.merge)" && + test -z "$(git config format.my7.coverSubject)" + test -z "$(git config format.my7.to)" && + test -z "$(git config format.my7.cc)" ' test_expect_success 'test deleting branch without config' ' diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh index 5d6c85489e..9c527510c3 100755 --- a/t/t4014-format-patch.sh +++ b/t/t4014-format-patch.sh @@ -237,7 +237,7 @@ test_expect_failure 'configuration To: header (rfc2047)' ' test_expect_success 'branch-specific configuration To: header (ascii)' ' test_unconfig format.to && - git config branch.side.to "R E Cipient <rcipient@example.com>" && + git config format.side.to "R E Cipient <rcipient@example.com>" && git format-patch --stdout master..side >patch10 && sed -e "/^\$/q" patch10 >hdrs10 && grep "^To: R E Cipient <rcipient@example.com>\$" hdrs10 @@ -245,7 +245,7 @@ test_expect_success 'branch-specific configuration To: header (ascii)' ' test_expect_failure 'branch-specific configuration To: header (rfc822)' ' - git config branch.side.to "R. E. Cipient <rcipient@example.com>" && + git config format.side.to "R. E. Cipient <rcipient@example.com>" && git format-patch --stdout master..side >patch10 && sed -e "/^\$/q" patch10 >hdrs10 && grep "^To: \"R. E. Cipient\" <rcipient@example.com>\$" hdrs10 @@ -253,7 +253,7 @@ test_expect_failure 'branch-specific configuration To: header (rfc822)' ' test_expect_failure 'branch-specific configuration To: header (rfc2047)' ' - git config branch.side.to "R Ä Cipient <rcipient@example.com>" && + git config format.side.to "R Ä Cipient <rcipient@example.com>" && git format-patch --stdout master..side >patch10 && sed -e "/^\$/q" patch10 >hdrs10 && grep "^To: =?UTF-8?q?R=20=C3=84=20Cipient?= <rcipient@example.com>\$" hdrs10 @@ -265,10 +265,10 @@ test_expect_success 'all recipients included from all sources' ' git config --add format.to "Format To2 <formatto2@example.com>" && git config format.cc "Format Cc1 <formatcc1@example.com>" && git config --add format.cc "Format Cc2 <formatcc2@example.com>" && - git config branch.side.to "Branch To1 <branchto1@example.com>" && - git config --add branch.side.to "Branch To2 <branchto2@example.com>" && - git config branch.side.cc "Branch Cc1 <branchcc1@example.com>" && - git config --add branch.side.cc "Branch Cc2 <branchcc2@example.com>" && + git config format.side.to "Branch To1 <branchto1@example.com>" && + git config --add format.side.to "Branch To2 <branchto2@example.com>" && + git config format.side.cc "Branch Cc1 <branchcc1@example.com>" && + git config --add format.side.cc "Branch Cc2 <branchcc2@example.com>" && cat <<-\EOF >expect && To: Format To1 <formatto1@example.com>, Format To2 <formatto2@example.com>, @@ -345,7 +345,7 @@ test_expect_success '--no-to overrides config.to' ' git config --replace-all format.to \ "R E Cipient <rcipient@example.com>" && - git config --replace-all branch.side.to \ + git config --replace-all format.side.to \ "B R Anch <branch@example.com>" && git format-patch --no-to --stdout master..side >patch11 && sed -e "/^\$/q" patch11 >hdrs11 && @@ -358,7 +358,7 @@ test_expect_success '--no-to and --to replaces config.to' ' git config --replace-all format.to \ "Someone <someone@out.there>" && - git config --replace-all branch.side.to \ + git config --replace-all format.side.to \ "B R Anch2 <branch2@example.com>" && git format-patch --no-to --to="Someone Else <else@out.there>" \ --stdout master..side >patch12 && @@ -373,7 +373,7 @@ test_expect_success '--no-cc overrides config.cc' ' git config --replace-all format.cc \ "C E Cipient <rcipient@example.com>" && - git config --replace-all branch.side.cc \ + git config --replace-all format.side.cc \ "B R Anch3 <branch3@example.com>" && git format-patch --no-cc --stdout master..side >patch13 && sed -e "/^\$/q" patch13 >hdrs13 && @@ -1538,7 +1538,7 @@ test_expect_success 'format patch ignores color.ui' ' ' test_expect_success 'cover letter with config subject' ' - test_config branch.rebuild-1.coverSubject "config subject" && + test_config format.rebuild-1.coverSubject "config subject" && git checkout rebuild-1 && git format-patch --stdout --cover-letter master >actual && grep "Subject: \[PATCH 0/2\] config subject" actual @@ -1551,7 +1551,7 @@ test_expect_success 'cover letter with command-line subject' ' ' test_expect_success 'cover letter with command-line subject overrides config' ' - test_config branch.rebuild-1.coverSubject "config subject" && + test_config format.rebuild-1.coverSubject "config subject" && git checkout rebuild-1 && git format-patch --stdout --cover-letter --cover-subject "command-line subject" master >actual && grep "Subject: \[PATCH 0/2\] command-line subject" actual