git@vger.kernel.org list mirror (unofficial, one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Denton Liu <liu.denton@gmail.com>
Cc: "Git Mailing List" <git@vger.kernel.org>,
	"Ævar Arnfjörð Bjarmason" <avarab@gmail.com>
Subject: Re: [PATCH v2 0/6] teach branch-specific options for format-patch
Date: Fri, 17 May 2019 13:12:04 +0900	[thread overview]
Message-ID: <xmqqk1ep3ejv.fsf@gitster-ct.c.googlers.com> (raw)
In-Reply-To: <cover.1558052674.git.liu.denton@gmail.com> (Denton Liu's message of "Thu, 16 May 2019 20:27:38 -0400")

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?  


  parent reply	other threads:[~2019-05-17  4:12 UTC|newest]

Thread overview: 59+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-05-05 16:24 [PATCH 0/7] " Denton Liu
2019-05-05 16:24 ` [PATCH 1/7] t4014: clean up style Denton Liu
2019-05-05 16:24 ` [PATCH 2/7] Doc: add more detail for git-format-patch Denton Liu
2019-05-05 16:24 ` [PATCH 3/7] branch.c: extract read_branch_config function Denton Liu
2019-05-05 16:24 ` [PATCH 4/7] format-patch: make cover letter subject configurable Denton Liu
2019-05-05 16:24 ` [PATCH 5/7] format-patch: move extra_headers logic later Denton Liu
2019-05-05 16:24 ` [PATCH 6/7] string-list: create string_list_append_all Denton Liu
2019-05-05 16:24 ` [PATCH 7/7] format-patch: read branch-specific To: and Cc: headers Denton Liu
2019-05-07  8:56 ` [PATCH 0/7] teach branch-specific options for format-patch Junio C Hamano
2019-05-07 14:19   ` Denton Liu
2019-05-07 15:05     ` Junio C Hamano
2019-05-07 15:21       ` Denton Liu
2019-05-07 15:46         ` Ævar Arnfjörð Bjarmason
2019-05-08  1:45           ` Junio C Hamano
2019-05-31  2:00             ` [RFC PATCH] config: learn the "onbranch:" includeIf condition Denton Liu
2019-05-31 12:58               ` Johannes Schindelin
2019-05-31 13:16                 ` Denton Liu
2019-05-31 17:23                   ` Johannes Schindelin
2019-05-31 18:44                     ` Denton Liu
2019-05-31 19:33               ` [PATCH v2] " Denton Liu
2019-05-31 20:14                 ` Johannes Schindelin
2019-06-05  8:02                   ` Johannes Schindelin
2019-06-05 10:08                 ` Duy Nguyen
2019-06-05 21:21                 ` [PATCH v3] " Denton Liu
2019-06-06 12:52                   ` Johannes Schindelin
2019-05-17  0:27 ` [PATCH v2 0/6] teach branch-specific options for format-patch Denton Liu
2019-05-17  0:27   ` [PATCH v2 1/6] t4014: clean up style Denton Liu
2019-05-17  0:27   ` [PATCH v2 2/6] Doc: add more detail for git-format-patch Denton Liu
2019-05-17  0:27   ` [PATCH v2 3/6] format-patch: make cover letter subject configurable Denton Liu
2019-05-17  0:27   ` [PATCH v2 4/6] format-patch: move extra_headers logic later Denton Liu
2019-05-17  0:27   ` [PATCH v2 5/6] string-list: create string_list_append_all Denton Liu
2019-05-17  0:27   ` [PATCH v2 6/6] format-patch: read branch-specific To: and Cc: headers Denton Liu
2019-05-17  4:12   ` Junio C Hamano [this message]
2019-05-17  7:25     ` [PATCH v2 0/6] teach branch-specific options for format-patch Denton Liu
2019-05-17 16:54       ` Denton Liu
2019-05-22  2:44   ` [PATCH v3 0/8] " Denton Liu
2019-05-22  2:44     ` [PATCH v3 1/8] t4014: clean up style Denton Liu
2019-05-22  2:44     ` [PATCH v3 2/8] Doc: add more detail for git-format-patch Denton Liu
2019-05-22  2:44     ` [PATCH v3 3/8] format-patch: infer cover letter from branch description Denton Liu
2019-05-22  2:44     ` [PATCH v3 4/8] format-patch: move extra_headers logic later Denton Liu
2019-05-22  2:44     ` [PATCH v3 5/8] string-list: create string_list_append_all Denton Liu
2019-05-22  2:44     ` [PATCH v3 6/8] format-patch: read branch-specific To: and Cc: headers Denton Liu
2019-05-22  2:44     ` [PATCH v3 7/8] format-patch: move output_directory logic later Denton Liu
2019-05-22  2:44     ` [PATCH v3 8/8] format-patch: read branch-specific output directory Denton Liu
2019-05-31  0:30     ` [PATCH v3 0/8] teach branch-specific options for format-patch Denton Liu
2019-06-14 21:56     ` [RESEND PATCH " Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 1/8] t4014: clean up style Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 2/8] Doc: add more detail for git-format-patch Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 3/8] format-patch: infer cover letter from branch description Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 4/8] format-patch: move extra_headers logic later Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 5/8] string-list: create string_list_append_all Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 6/8] format-patch: read branch-specific To: and Cc: headers Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 7/8] format-patch: move output_directory logic later Denton Liu
2019-06-14 21:56       ` [RESEND PATCH v3 8/8] format-patch: read branch-specific output directory Denton Liu
2019-10-15  9:06     ` [PATCH v6 0/3] format-patch: learn --infer-cover-subject option (also t4014 cleanup) Denton Liu
2019-10-15  9:06       ` [PATCH v6 1/3] format-patch: replace erroneous and condition Denton Liu
2019-10-15  9:06       ` [PATCH v6 2/3] format-patch: use enum variables Denton Liu
2019-10-15  9:06       ` [PATCH v6 3/3] format-patch: teach --cover-from-description option Denton Liu
2019-10-16  1:28       ` [PATCH v6 0/3] format-patch: learn --infer-cover-subject option (also t4014 cleanup) Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqk1ep3ejv.fsf@gitster-ct.c.googlers.com \
    --to=gitster@pobox.com \
    --cc=avarab@gmail.com \
    --cc=git@vger.kernel.org \
    --cc=liu.denton@gmail.com \
    --subject='Re: [PATCH v2 0/6] teach branch-specific options for format-patch' \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link

Code repositories for project(s) associated with this inbox:

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).