Rebase am fixes

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

From: Elijah Newren <[email protected]>

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.

---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index 8181c2aef3..d4d131b7ee 100644
@@ -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

From: Elijah Newren <[email protected]>

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.

---
 builtin/rebase.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

index ddf33bc9d4..e354ec84bb 100644
@@ -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

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.

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.

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.

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.

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?)

Hi,

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...

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.

Hi,

Oy. Thanks for fixing my bug,
Dscho