* [PATCH 0/2] Rebase am fixes @ 2019-12-20 18:53 Elijah Newren via GitGitGadget 2019-12-20 18:53 ` [PATCH 1/2] am: pay attention to user-defined context size Elijah Newren via GitGitGadget 2019-12-20 18:53 ` [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases Elijah Newren via GitGitGadget 0 siblings, 2 replies; 10+ messages in thread From: Elijah Newren via GitGitGadget @ 2019-12-20 18:53 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, plroskin, Junio C Hamano This series fixes a pair of small bugs with am/rebase. Elijah Newren (2): am: pay attention to user-defined context size rebase: fix saving of --signoff state for am-based rebases builtin/am.c | 2 +- builtin/rebase.c | 2 +- 2 files changed, 2 insertions(+), 2 deletions(-) base-commit: 12029dc57db23baef008e77db1909367599210ee Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-680%2Fnewren%2Frebase-am-fixes-v1 Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-680/newren/rebase-am-fixes-v1 Pull-Request: https://github.com/git/git/pull/680 -- gitgitgadget ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 1/2] am: pay attention to user-defined context size 2019-12-20 18:53 [PATCH 0/2] Rebase am fixes Elijah Newren via GitGitGadget @ 2019-12-20 18:53 ` Elijah Newren via GitGitGadget 2019-12-20 19:28 ` Junio C Hamano 2019-12-20 18:53 ` [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases Elijah Newren via GitGitGadget 1 sibling, 1 reply; 10+ messages in thread From: Elijah Newren via GitGitGadget @ 2019-12-20 18:53 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, plroskin, Junio C Hamano, Elijah Newren From: Elijah Newren <newren@gmail.com> am previously only checked gpg-related config options and the default config options while ignoring any diff-related options. This meant that when users tried to set diff.context to something larger than the default value of 3, it was ignored. Pay attention to the diff settings too. In combination with commit 09ac67a1839e ("format-patch: move git_config() before repo_init_revisions()", 2019-12-09), which is part of the dl/format-patch-notes-config-fixup topic, this fixes git -c diff.context=5 rebase to actually use five lines of context. Signed-off-by: Elijah Newren <newren@gmail.com> --- builtin/am.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/am.c b/builtin/am.c index 8181c2aef3..d4d131b7ee 100644 --- a/builtin/am.c +++ b/builtin/am.c @@ -2136,7 +2136,7 @@ static int git_am_config(const char *k, const char *v, void *cb) if (status) return status; - return git_default_config(k, v, NULL); + return git_diff_ui_config(k, v, NULL); } int cmd_am(int argc, const char **argv, const char *prefix) -- gitgitgadget ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] am: pay attention to user-defined context size 2019-12-20 18:53 ` [PATCH 1/2] am: pay attention to user-defined context size Elijah Newren via GitGitGadget @ 2019-12-20 19:28 ` Junio C Hamano 2019-12-20 19:38 ` Elijah Newren 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2019-12-20 19:28 UTC (permalink / raw) To: Elijah Newren via GitGitGadget Cc: git, Johannes.Schindelin, plroskin, Elijah Newren "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > am previously only checked gpg-related config options and the default > config options while ignoring any diff-related options. This meant that > when users tried to set diff.context to something larger than the > default value of 3, it was ignored. Pay attention to the diff settings > too. Can the benefit brought in by this change demonstrated by a new test or two? Puzzled. "am" accepts whatever patch somebody else prepared and has no control over how the incoming "diff" was produced by that somebody else. Besides, I do not think it should be affected by any diff_*UI*_config() in the first place. ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] am: pay attention to user-defined context size 2019-12-20 19:28 ` Junio C Hamano @ 2019-12-20 19:38 ` Elijah Newren 2019-12-20 22:22 ` Elijah Newren 0 siblings, 1 reply; 10+ messages in thread From: Elijah Newren @ 2019-12-20 19:38 UTC (permalink / raw) To: Junio C Hamano Cc: Elijah Newren via GitGitGadget, Git Mailing List, Johannes Schindelin, Pavel Roskin On Fri, Dec 20, 2019 at 11:28 AM Junio C Hamano <gitster@pobox.com> wrote: > > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > am previously only checked gpg-related config options and the default > > config options while ignoring any diff-related options. This meant that > > when users tried to set diff.context to something larger than the > > default value of 3, it was ignored. Pay attention to the diff settings > > too. > > Can the benefit brought in by this change demonstrated by a new test > or two? Yeah, I'll try to come up with something. I was originally going to test 'git -c diff.context=5 rebase', but of course that depends on Denton's change too to work. > Puzzled. "am" accepts whatever patch somebody else prepared and > has no control over how the incoming "diff" was produced by that > somebody else. I was too, but the diff.context change didn't work until I fixed both format-patch and am to pay attention to diff.context. > Besides, I do not think it should be affected by any diff_*UI*_config() > in the first place. Does that mean that diff.context is checked in the wrong place in diff.c, and should be moved from git_diff_ui_config() to git_diff_basic_config()? (And perhaps the same is true for diff.algorithm?) ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] am: pay attention to user-defined context size 2019-12-20 19:38 ` Elijah Newren @ 2019-12-20 22:22 ` Elijah Newren 2019-12-20 22:34 ` Junio C Hamano 0 siblings, 1 reply; 10+ messages in thread From: Elijah Newren @ 2019-12-20 22:22 UTC (permalink / raw) To: Junio C Hamano Cc: Elijah Newren via GitGitGadget, Git Mailing List, Johannes Schindelin, Pavel Roskin Hi, On Fri, Dec 20, 2019 at 11:38 AM Elijah Newren <newren@gmail.com> wrote: > > On Fri, Dec 20, 2019 at 11:28 AM Junio C Hamano <gitster@pobox.com> wrote: > > > > Besides, I do not think it should be affected by any diff_*UI*_config() > > in the first place. > > Does that mean that diff.context is checked in the wrong place in > diff.c, and should be moved from git_diff_ui_config() to > git_diff_basic_config()? (And perhaps the same is true for > diff.algorithm?) So, referring to git_diff_ui_config() as ui and git_diff_basic_config() as basic: * Moving diff.algorithm from ui to basic requires updating the error message printed by t3701.47 * Moving diff.context from ui to basic breaks t4055.6 (which wants diff.context to NOT affect plumbing) * Moving diff.suppressblankempty from basic to ui causes no issues * Moving diff.color.* and color.diff.* and diff.*.* from basic to ui causes no issues, but you have to move the userdiff_config if you want to move the color config or else t4020.12 breaks. In particular, t4055.6 is pretty interesting in that it was a test specifically created for the sole purpose of declaring that diff.context should NOT affect plumbing. So if it's not plumbing by that test, and it's not *UI* as per what you say, what exactly is it? Do I just make am directly check diff.context and ignore any other diff settings? Not sure where to go on this... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] am: pay attention to user-defined context size 2019-12-20 22:22 ` Elijah Newren @ 2019-12-20 22:34 ` Junio C Hamano 2019-12-21 5:37 ` Elijah Newren 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2019-12-20 22:34 UTC (permalink / raw) To: Elijah Newren Cc: Elijah Newren via GitGitGadget, Git Mailing List, Johannes Schindelin, Pavel Roskin Elijah Newren <newren@gmail.com> writes: > diff.context should NOT affect plumbing. So if it's not plumbing by > that test, and it's not *UI* as per what you say, what exactly is it? I actually was saying that diff.context is UI thing, and should make no effect on how "am" interprets its input. Which the codepath in "am" are you trying to affect? "am" is mainly a consumer of "diff" output, and not a producer, so ... ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 1/2] am: pay attention to user-defined context size 2019-12-20 22:34 ` Junio C Hamano @ 2019-12-21 5:37 ` Elijah Newren 0 siblings, 0 replies; 10+ messages in thread From: Elijah Newren @ 2019-12-21 5:37 UTC (permalink / raw) To: Junio C Hamano Cc: Elijah Newren via GitGitGadget, Git Mailing List, Johannes Schindelin, Pavel Roskin On Fri, Dec 20, 2019 at 2:34 PM Junio C Hamano <gitster@pobox.com> wrote: > > Elijah Newren <newren@gmail.com> writes: > > > diff.context should NOT affect plumbing. So if it's not plumbing by > > that test, and it's not *UI* as per what you say, what exactly is it? > > I actually was saying that diff.context is UI thing, and should make > no effect on how "am" interprets its input. > > Which the codepath in "am" are you trying to affect? "am" is mainly > a consumer of "diff" output, and not a producer, so ... Okay, I can't seem to find a simple way to reproduce separate from the testcase reported here: https://lore.kernel.org/git/CAN_72e2h2avv-U9BVBYqXVKiC+5kHy-pjejyMSD3X22uRXE39g@mail.gmail.com/ To summarize, when I run these exact steps: git clone --quiet https://github.com/proski/git-rebase-demo cd git-rebase-demo git checkout --quiet branch1 git -c diff.context=5 rebase --quiet origin/branch2 test $? -eq 0 && echo Successfully rebased echo Difference from expected: git diff --shortstat origin/merge-good The rebase succeeds in both cases, but I get different output from the shortstat depending on whether or not this git_am_config patch is applied. I can't seem to track down why this patch makes a difference when as you say it shouldn't, nor can I seem to generate an am-only testcase. I feel like it should be easy to get at least one of these given the short steps to reproduce (the repo only has 1 file and three relevant commits), but it seems to be stumping me nonetheless. ^ permalink raw reply [flat|nested] 10+ messages in thread
* [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases 2019-12-20 18:53 [PATCH 0/2] Rebase am fixes Elijah Newren via GitGitGadget 2019-12-20 18:53 ` [PATCH 1/2] am: pay attention to user-defined context size Elijah Newren via GitGitGadget @ 2019-12-20 18:53 ` Elijah Newren via GitGitGadget 2019-12-20 19:29 ` Junio C Hamano 1 sibling, 1 reply; 10+ messages in thread From: Elijah Newren via GitGitGadget @ 2019-12-20 18:53 UTC (permalink / raw) To: git; +Cc: Johannes.Schindelin, plroskin, Junio C Hamano, Elijah Newren From: Elijah Newren <newren@gmail.com> This was an error introduced in the conversion from shell in commit 21853626eac5 ("built-in rebase: call `git am` directly", 2019-01-18), which was noticed by a random browsing of the code. Signed-off-by: Elijah Newren <newren@gmail.com> --- builtin/rebase.c | 2 +- 1 file changed, 1 insertion(+), 1 deletion(-) diff --git a/builtin/rebase.c b/builtin/rebase.c index ddf33bc9d4..e354ec84bb 100644 --- a/builtin/rebase.c +++ b/builtin/rebase.c @@ -706,7 +706,7 @@ static int rebase_write_basic_state(struct rebase_options *opts) write_file(state_dir_path("gpg_sign_opt", opts), "%s", opts->gpg_sign_opt); if (opts->signoff) - write_file(state_dir_path("strategy", opts), "--signoff"); + write_file(state_dir_path("signoff", opts), "--signoff"); return 0; } -- gitgitgadget ^ permalink raw reply related [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases 2019-12-20 18:53 ` [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases Elijah Newren via GitGitGadget @ 2019-12-20 19:29 ` Junio C Hamano 2020-01-01 22:01 ` Johannes Schindelin 0 siblings, 1 reply; 10+ messages in thread From: Junio C Hamano @ 2019-12-20 19:29 UTC (permalink / raw) To: Elijah Newren via GitGitGadget Cc: git, Johannes.Schindelin, plroskin, Elijah Newren "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: Elijah Newren <newren@gmail.com> > > This was an error introduced in the conversion from shell in commit > 21853626eac5 ("built-in rebase: call `git am` directly", 2019-01-18), > which was noticed by a random browsing of the code. Wow, thanks. This probably indicates that nobody uses "rebase --signoff" in real life (i.e. where correct signed-off-by line matters). But still the bug is worth fixing. > > Signed-off-by: Elijah Newren <newren@gmail.com> > --- > builtin/rebase.c | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/builtin/rebase.c b/builtin/rebase.c > index ddf33bc9d4..e354ec84bb 100644 > --- a/builtin/rebase.c > +++ b/builtin/rebase.c > @@ -706,7 +706,7 @@ static int rebase_write_basic_state(struct rebase_options *opts) > write_file(state_dir_path("gpg_sign_opt", opts), "%s", > opts->gpg_sign_opt); > if (opts->signoff) > - write_file(state_dir_path("strategy", opts), "--signoff"); > + write_file(state_dir_path("signoff", opts), "--signoff"); > > return 0; > } ^ permalink raw reply [flat|nested] 10+ messages in thread
* Re: [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases 2019-12-20 19:29 ` Junio C Hamano @ 2020-01-01 22:01 ` Johannes Schindelin 0 siblings, 0 replies; 10+ messages in thread From: Johannes Schindelin @ 2020-01-01 22:01 UTC (permalink / raw) To: Junio C Hamano Cc: Elijah Newren via GitGitGadget, git, plroskin, Elijah Newren Hi, On Fri, 20 Dec 2019, Junio C Hamano wrote: > "Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes: > > > From: Elijah Newren <newren@gmail.com> > > > > This was an error introduced in the conversion from shell in commit > > 21853626eac5 ("built-in rebase: call `git am` directly", 2019-01-18), > > which was noticed by a random browsing of the code. > > Wow, thanks. Oy. Thanks for fixing my bug, Dscho > > This probably indicates that nobody uses "rebase --signoff" in real > life (i.e. where correct signed-off-by line matters). But still the > bug is worth fixing. > > > > > Signed-off-by: Elijah Newren <newren@gmail.com> > > --- > > builtin/rebase.c | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > > > diff --git a/builtin/rebase.c b/builtin/rebase.c > > index ddf33bc9d4..e354ec84bb 100644 > > --- a/builtin/rebase.c > > +++ b/builtin/rebase.c > > @@ -706,7 +706,7 @@ static int rebase_write_basic_state(struct rebase_options *opts) > > write_file(state_dir_path("gpg_sign_opt", opts), "%s", > > opts->gpg_sign_opt); > > if (opts->signoff) > > - write_file(state_dir_path("strategy", opts), "--signoff"); > > + write_file(state_dir_path("signoff", opts), "--signoff"); > > > > return 0; > > } > ^ permalink raw reply [flat|nested] 10+ messages in thread
end of thread, other threads:[~2020-01-01 22:01 UTC | newest] Thread overview: 10+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2019-12-20 18:53 [PATCH 0/2] Rebase am fixes Elijah Newren via GitGitGadget 2019-12-20 18:53 ` [PATCH 1/2] am: pay attention to user-defined context size Elijah Newren via GitGitGadget 2019-12-20 19:28 ` Junio C Hamano 2019-12-20 19:38 ` Elijah Newren 2019-12-20 22:22 ` Elijah Newren 2019-12-20 22:34 ` Junio C Hamano 2019-12-21 5:37 ` Elijah Newren 2019-12-20 18:53 ` [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases Elijah Newren via GitGitGadget 2019-12-20 19:29 ` Junio C Hamano 2020-01-01 22:01 ` Johannes Schindelin
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 NNTP newsgroup(s).