# allow a differn't diff.context config for git format-patch 7 messages from 2019-04-14 to 2019-05-14. Participants: Shawn Landden, Jeff King, Junio C Hamano. Thread: https://gitlist.dev/t/50934 ## Shawn Landden, 2019-04-14 21:48 Subject: allow a differn't diff.context config for git format-patch Message-ID: URL: https://gitlist.dev/e/CA%2B49okrda1%3DdV%3DrN23oFg7LQmRp%2BidkDDDxFQ9R5jhGJFHKpEg%40mail.gmail.com ``` When I send patches I want them to have lots of context, but when just looking at a diff, I can always open the file for context. -SHawn ``` ## Jeff King, 2019-04-23 03:06 Subject: Re: allow a differn't diff.context config for git format-patch Message-ID: <20190423030648.GA6344@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190423030648.GA6344%40sigill.intra.peff.net In-Reply-To: ``` On Sun, Apr 14, 2019 at 04:48:53PM -0500, Shawn Landden wrote: > When I send patches I want them to have lots of context, but when just > looking at a diff, I can always open the file for context. Seems like a reasonable thing to want. You can already use "git format-patch -U20 ...". The usual advice for putting that in config is to define an alias wit your preferred option. I don't think it would be too big a problem for format-patch to learn some options to configure its diffs. We already have some options in format.* for various things. -Peff ``` ## Junio C Hamano, 2019-04-23 03:45 Subject: Re: allow a differn't diff.context config for git format-patch Message-ID: URL: https://gitlist.dev/e/xmqqftq95qr6.fsf%40gitster-ct.c.googlers.com In-Reply-To: <20190423030648.GA6344@sigill.intra.peff.net> ``` Jeff King writes: > ... I don't think it would be > too big a problem for format-patch to learn some options to configure > its diffs. We already have some options in format.* for various things. I am not sure; diff.context is rather special in that the variable behind it belongs to the diff layer and is set via diff_config that is called by diff_ui_config that in turn is called by log_config which is called by format_config. If we want to say "format.context" trumps "diff.config", then a new internal variable to receive the value of the variable must be invented and after the config gets read, cmd_format_patch() needs to do something silly like this: git_config(git_format_config, NULL); + if (format_context != -1) + diff_default_context = format_context; repo_init_revisions(the_repository, &rev, prefix); committing layering violation by exposing diff_default_context variable. ``` ## Jeff King, 2019-04-23 03:55 Subject: Re: allow a differn't diff.context config for git format-patch Message-ID: <20190423035537.GB7753@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190423035537.GB7753%40sigill.intra.peff.net In-Reply-To: ``` On Tue, Apr 23, 2019 at 12:45:17PM +0900, Junio C Hamano wrote: > Jeff King writes: > > > ... I don't think it would be > > too big a problem for format-patch to learn some options to configure > > its diffs. We already have some options in format.* for various things. > > I am not sure; diff.context is rather special in that the variable > behind it belongs to the diff layer and is set via diff_config that > is called by diff_ui_config that in turn is called by log_config > which is called by format_config. > > If we want to say "format.context" trumps "diff.config", then a new > internal variable to receive the value of the variable must be > invented and after the config gets read, cmd_format_patch() needs to > do something silly like this: > > git_config(git_format_config, NULL); > + if (format_context != -1) > + diff_default_context = format_context; > repo_init_revisions(the_repository, &rev, prefix); > > committing layering violation by exposing diff_default_context > variable. After init_revisions(), we'll have called diff_setup(), which puts diff_context_default into revs->diffopt. So we still have to do the "if we have a format-specific value, then override..." conditional. But we can do it without touching the hidden variable: diff --git a/builtin/log.c b/builtin/log.c index 57869267d8..0de2ee0905 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -1613,14 +1613,16 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) repo_init_revisions(the_repository, &rev, prefix); rev.commit_format = CMIT_FMT_EMAIL; rev.expand_tabs_in_log_default = 0; rev.verbose_header = 1; rev.diff = 1; rev.max_parents = 1; rev.diffopt.flags.recursive = 1; + if (format_context != -1) + rev.diffopt.context = format_context; rev.subject_prefix = fmt_patch_subject_prefix; memset(&s_r_opt, 0, sizeof(s_r_opt)); s_r_opt.def = "HEAD"; s_r_opt.revarg_opt = REVARG_COMMITTISH; if (default_attach) { rev.mime_boundary = default_attach; -Peff ``` ## Shawn Landden, 2019-05-12 13:21 Subject: Re: allow a differn't diff.context config for git format-patch Message-ID: URL: https://gitlist.dev/e/CA%2B49okqSu1VYgrc__1SjYFvs1mhGHqQXhHrMFApRSzk_UNkjJw%40mail.gmail.com In-Reply-To: <20190423035537.GB7753@sigill.intra.peff.net> ``` On Mon, Apr 22, 2019 at 10:55 PM Jeff King wrote: > > On Tue, Apr 23, 2019 at 12:45:17PM +0900, Junio C Hamano wrote: > > > Jeff King writes: > > > > > ... I don't think it would be > > > too big a problem for format-patch to learn some options to configure > > > its diffs. We already have some options in format.* for various things. > > > > I am not sure; diff.context is rather special in that the variable > > behind it belongs to the diff layer and is set via diff_config that > > is called by diff_ui_config that in turn is called by log_config > > which is called by format_config. > > > > If we want to say "format.context" trumps "diff.config", then a new > > internal variable to receive the value of the variable must be > > invented and after the config gets read, cmd_format_patch() needs to > > do something silly like this: > > > > git_config(git_format_config, NULL); > > + if (format_context != -1) > > + diff_default_context = format_context; > > repo_init_revisions(the_repository, &rev, prefix); > > > > committing layering violation by exposing diff_default_context > > variable. > > After init_revisions(), we'll have called diff_setup(), which puts > diff_context_default into revs->diffopt. So we still have to do the "if > we have a format-specific value, then override..." conditional. But we > can do it without touching the hidden variable: > > > Looks like this fell into the cracks. You guys know the code much better than me, but do I have to write a patch to make this happen? > diff --git a/builtin/log.c b/builtin/log.c > index 57869267d8..0de2ee0905 100644 > --- a/builtin/log.c > +++ b/builtin/log.c > @@ -1613,14 +1613,16 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix) > repo_init_revisions(the_repository, &rev, prefix); > rev.commit_format = CMIT_FMT_EMAIL; > rev.expand_tabs_in_log_default = 0; > rev.verbose_header = 1; > rev.diff = 1; > rev.max_parents = 1; > rev.diffopt.flags.recursive = 1; > + if (format_context != -1) > + rev.diffopt.context = format_context; > rev.subject_prefix = fmt_patch_subject_prefix; > memset(&s_r_opt, 0, sizeof(s_r_opt)); > s_r_opt.def = "HEAD"; > s_r_opt.revarg_opt = REVARG_COMMITTISH; > > if (default_attach) { > rev.mime_boundary = default_attach; > > -Peff ``` ## Jeff King, 2019-05-14 09:54 Subject: Re: allow a differn't diff.context config for git format-patch Message-ID: <20190514095446.GA12299@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190514095446.GA12299%40sigill.intra.peff.net In-Reply-To: ``` On Sun, May 12, 2019 at 07:29:42AM -0500, Shawn Landden wrote: > > After init_revisions(), we'll have called diff_setup(), which puts > > diff_context_default into revs->diffopt. So we still have to do the "if > > we have a format-specific value, then override..." conditional. But we > > can do it without touching the hidden variable: > > > Looks like this fell into the cracks. You guys know the code much better > than me, but do I have to write a patch to make this happen? I think there is more work to be done in actually parsing such a config option, plus adding documentation and tests. But more importantly, it needs somebody to champion the feature. I don't find it an unreasonable thing to want. But I'm not personally interested, nor am I completely sure that it wouldn't have any unwanted side effects. So ideally somebody would write the patch and a compelling commit message that explains why it is a good idea. -Peff ``` ## Jeff King, 2019-05-14 09:57 Subject: Re: allow a differn't diff.context config for git format-patch Message-ID: <20190514095722.GA12580@sigill.intra.peff.net> URL: https://gitlist.dev/e/20190514095722.GA12580%40sigill.intra.peff.net In-Reply-To: <20190514095446.GA12299@sigill.intra.peff.net> ``` On Tue, May 14, 2019 at 05:54:46AM -0400, Jeff King wrote: > On Sun, May 12, 2019 at 07:29:42AM -0500, Shawn Landden wrote: > > > > After init_revisions(), we'll have called diff_setup(), which puts > > > diff_context_default into revs->diffopt. So we still have to do the "if > > > we have a format-specific value, then override..." conditional. But we > > > can do it without touching the hidden variable: > > > > > Looks like this fell into the cracks. You guys know the code much better > > than me, but do I have to write a patch to make this happen? > > I think there is more work to be done in actually parsing such a config > option, plus adding documentation and tests. > > But more importantly, it needs somebody to champion the feature. I don't > find it an unreasonable thing to want. But I'm not personally > interested, nor am I completely sure that it wouldn't have any unwanted > side effects. > > So ideally somebody would write the patch and a compelling commit > message that explains why it is a good idea. By the way, if you (or anybody) is interested, looking at how signoff is implemented (try "git grep -i format.signoff") should get you most of the way there. -Peff ```