{"thread":{"id":"50934","subject":"allow a differn't diff.context config for git format-patch","startedAt":"2019-04-14T21:58:41Z","lastAt":"2019-05-14T09:57:27Z","messageCount":7,"participants":["Shawn Landden","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"373835","messageId":"CA+49okrda1=dV=rN23oFg7LQmRp+idkDDDxFQ9R5jhGJFHKpEg@mail.gmail.com","threadId":"50934","inReplyTo":null,"subject":"allow a differn't diff.context config for git format-patch","fromName":"Shawn Landden","fromEmail":"shawn@git.icu","sentAt":"2019-04-14T21:48:53Z","receivedAt":"2019-04-14T21:58:41Z","isPatch":false,"sender":{"key":"shawn@git.icu","avatar":null},"body":"When I send patches I want them to have lots of context, but when just\nlooking at a diff, I can always open the file for context.\n\n-SHawn\n"},{"id":"374293","messageId":"20190423030648.GA6344@sigill.intra.peff.net","threadId":"50934","inReplyTo":"CA+49okrda1=dV=rN23oFg7LQmRp+idkDDDxFQ9R5jhGJFHKpEg@mail.gmail.com","subject":"Re: allow a differn't diff.context config for git format-patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-23T03:06:49Z","receivedAt":"2019-04-23T03:06:54Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Apr 14, 2019 at 04:48:53PM -0500, Shawn Landden wrote:\n\n> When I send patches I want them to have lots of context, but when just\n> looking at a diff, I can always open the file for context.\n\nSeems like a reasonable thing to want. You can already use \"git\nformat-patch -U20 ...\". The usual advice for putting that in config is\nto define an alias wit your preferred option. I don't think it would be\ntoo big a problem for format-patch to learn some options to configure\nits diffs. We already have some options in format.* for various things.\n\n-Peff\n"},{"id":"374294","messageId":"xmqqftq95qr6.fsf@gitster-ct.c.googlers.com","threadId":"50934","inReplyTo":"20190423030648.GA6344@sigill.intra.peff.net","subject":"Re: allow a differn't diff.context config for git format-patch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-04-23T03:45:17Z","receivedAt":"2019-04-23T03:45:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> ... I don't think it would be\n> too big a problem for format-patch to learn some options to configure\n> its diffs. We already have some options in format.* for various things.\n\nI am not sure; diff.context is rather special in that the variable\nbehind it belongs to the diff layer and is set via diff_config that\nis called by diff_ui_config that in turn is called by log_config\nwhich is called by format_config.\n\nIf we want to say \"format.context\" trumps \"diff.config\", then a new\ninternal variable to receive the value of the variable must be\ninvented and after the config gets read, cmd_format_patch() needs to\ndo something silly like this:\n\n git_config(git_format_config, NULL);\n +\tif (format_context != -1)\n +\t\tdiff_default_context = format_context;\n repo_init_revisions(the_repository, &rev, prefix);\n\ncommitting layering violation by exposing diff_default_context\nvariable.\n\n"},{"id":"374296","messageId":"20190423035537.GB7753@sigill.intra.peff.net","threadId":"50934","inReplyTo":"xmqqftq95qr6.fsf@gitster-ct.c.googlers.com","subject":"Re: allow a differn't diff.context config for git format-patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-04-23T03:55:38Z","receivedAt":"2019-04-23T03:55:42Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Apr 23, 2019 at 12:45:17PM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > ... I don't think it would be\n> > too big a problem for format-patch to learn some options to configure\n> > its diffs. We already have some options in format.* for various things.\n> \n> I am not sure; diff.context is rather special in that the variable\n> behind it belongs to the diff layer and is set via diff_config that\n> is called by diff_ui_config that in turn is called by log_config\n> which is called by format_config.\n> \n> If we want to say \"format.context\" trumps \"diff.config\", then a new\n> internal variable to receive the value of the variable must be\n> invented and after the config gets read, cmd_format_patch() needs to\n> do something silly like this:\n> \n> git_config(git_format_config, NULL);\n> +\tif (format_context != -1)\n> +\t\tdiff_default_context = format_context;\n> repo_init_revisions(the_repository, &rev, prefix);\n> \n> committing layering violation by exposing diff_default_context\n> variable.\n\nAfter init_revisions(), we'll have called diff_setup(), which puts\ndiff_context_default into revs->diffopt. So we still have to do the \"if\nwe have a format-specific value, then override...\" conditional. But we\ncan do it without touching the hidden variable:\n\ndiff --git a/builtin/log.c b/builtin/log.c\nindex 57869267d8..0de2ee0905 100644\n--- a/builtin/log.c\n+++ b/builtin/log.c\n@@ -1613,14 +1613,16 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n \trepo_init_revisions(the_repository, &rev, prefix);\n \trev.commit_format = CMIT_FMT_EMAIL;\n \trev.expand_tabs_in_log_default = 0;\n \trev.verbose_header = 1;\n \trev.diff = 1;\n \trev.max_parents = 1;\n \trev.diffopt.flags.recursive = 1;\n+\tif (format_context != -1)\n+\t\trev.diffopt.context = format_context;\n \trev.subject_prefix = fmt_patch_subject_prefix;\n \tmemset(&s_r_opt, 0, sizeof(s_r_opt));\n \ts_r_opt.def = \"HEAD\";\n \ts_r_opt.revarg_opt = REVARG_COMMITTISH;\n \n \tif (default_attach) {\n \t\trev.mime_boundary = default_attach;\n\n-Peff\n"},{"id":"375353","messageId":"CA+49okqSu1VYgrc__1SjYFvs1mhGHqQXhHrMFApRSzk_UNkjJw@mail.gmail.com","threadId":"50934","inReplyTo":"20190423035537.GB7753@sigill.intra.peff.net","subject":"Re: allow a differn't diff.context config for git format-patch","fromName":"Shawn Landden","fromEmail":"shawn@git.icu","sentAt":"2019-05-12T13:21:07Z","receivedAt":"2019-05-12T13:21:23Z","isPatch":false,"sender":{"key":"shawn@git.icu","avatar":null},"body":"On Mon, Apr 22, 2019 at 10:55 PM Jeff King wrote:\n>\n> On Tue, Apr 23, 2019 at 12:45:17PM +0900, Junio C Hamano wrote:\n>\n> > Jeff King writes:\n> >\n> > > ... I don't think it would be\n> > > too big a problem for format-patch to learn some options to configure\n> > > its diffs. We already have some options in format.* for various things.\n> >\n> > I am not sure; diff.context is rather special in that the variable\n> > behind it belongs to the diff layer and is set via diff_config that\n> > is called by diff_ui_config that in turn is called by log_config\n> > which is called by format_config.\n> >\n> > If we want to say \"format.context\" trumps \"diff.config\", then a new\n> > internal variable to receive the value of the variable must be\n> > invented and after the config gets read, cmd_format_patch() needs to\n> > do something silly like this:\n> >\n> > git_config(git_format_config, NULL);\n> > + if (format_context != -1)\n> > + diff_default_context = format_context;\n> > repo_init_revisions(the_repository, &rev, prefix);\n> >\n> > committing layering violation by exposing diff_default_context\n> > variable.\n>\n> After init_revisions(), we'll have called diff_setup(), which puts\n> diff_context_default into revs->diffopt. So we still have to do the \"if\n> we have a format-specific value, then override...\" conditional. But we\n> can do it without touching the hidden variable:\n>\n>\n>\nLooks like this fell into the cracks. You guys know the code much\nbetter than me, but do I have to write a patch to make this happen?\n> diff --git a/builtin/log.c b/builtin/log.c\n> index 57869267d8..0de2ee0905 100644\n> --- a/builtin/log.c\n> +++ b/builtin/log.c\n> @@ -1613,14 +1613,16 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)\n> repo_init_revisions(the_repository, &rev, prefix);\n> rev.commit_format = CMIT_FMT_EMAIL;\n> rev.expand_tabs_in_log_default = 0;\n> rev.verbose_header = 1;\n> rev.diff = 1;\n> rev.max_parents = 1;\n> rev.diffopt.flags.recursive = 1;\n> + if (format_context != -1)\n> + rev.diffopt.context = format_context;\n> rev.subject_prefix = fmt_patch_subject_prefix;\n> memset(&s_r_opt, 0, sizeof(s_r_opt));\n> s_r_opt.def = \"HEAD\";\n> s_r_opt.revarg_opt = REVARG_COMMITTISH;\n>\n> if (default_attach) {\n> rev.mime_boundary = default_attach;\n>\n> -Peff\n"},{"id":"375495","messageId":"20190514095446.GA12299@sigill.intra.peff.net","threadId":"50934","inReplyTo":"CA+49okrzR7GAZCFDfaoJ9pvK+o=DVWoZ9vA6YL=u72s_rnjDxQ@mail.gmail.com","subject":"Re: allow a differn't diff.context config for git format-patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T09:54:46Z","receivedAt":"2019-05-14T09:54:51Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, May 12, 2019 at 07:29:42AM -0500, Shawn Landden wrote:\n\n> > After init_revisions(), we'll have called diff_setup(), which puts\n> > diff_context_default into revs->diffopt. So we still have to do the \"if\n> > we have a format-specific value, then override...\" conditional. But we\n> > can do it without touching the hidden variable:\n> >\n> Looks like this fell into the cracks. You guys know the code much better\n> than me, but do I have to write a patch to make this happen?\n\nI think there is more work to be done in actually parsing such a config\noption, plus adding documentation and tests.\n\nBut more importantly, it needs somebody to champion the feature. I don't\nfind it an unreasonable thing to want. But I'm not personally\ninterested, nor am I completely sure that it wouldn't have any unwanted\nside effects.\n\nSo ideally somebody would write the patch and a compelling commit\nmessage that explains why it is a good idea.\n\n-Peff\n"},{"id":"375496","messageId":"20190514095722.GA12580@sigill.intra.peff.net","threadId":"50934","inReplyTo":"20190514095446.GA12299@sigill.intra.peff.net","subject":"Re: allow a differn't diff.context config for git format-patch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-05-14T09:57:22Z","receivedAt":"2019-05-14T09:57:27Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 14, 2019 at 05:54:46AM -0400, Jeff King wrote:\n\n> On Sun, May 12, 2019 at 07:29:42AM -0500, Shawn Landden wrote:\n> \n> > > After init_revisions(), we'll have called diff_setup(), which puts\n> > > diff_context_default into revs->diffopt. So we still have to do the \"if\n> > > we have a format-specific value, then override...\" conditional. But we\n> > > can do it without touching the hidden variable:\n> > >\n> > Looks like this fell into the cracks. You guys know the code much better\n> > than me, but do I have to write a patch to make this happen?\n> \n> I think there is more work to be done in actually parsing such a config\n> option, plus adding documentation and tests.\n> \n> But more importantly, it needs somebody to champion the feature. I don't\n> find it an unreasonable thing to want. But I'm not personally\n> interested, nor am I completely sure that it wouldn't have any unwanted\n> side effects.\n> \n> So ideally somebody would write the patch and a compelling commit\n> message that explains why it is a good idea.\n\nBy the way, if you (or anybody) is interested, looking at how signoff is\nimplemented (try \"git grep -i format.signoff\") should get you most of\nthe way there.\n\n-Peff\n"}]}