[PATCH 0/2] Rebase am fixes

[PATCH 0/2] Rebase am fixes

[Elijah Newren via GitGitGadget]

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

[PATCH 1/2] am: pay attention to user-defined context size

[Elijah Newren via GitGitGadget]

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

[PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases

[Elijah Newren via GitGitGadget]

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

Re: [PATCH 1/2] am: pay attention to user-defined context size

[Junio C Hamano]

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

Re: [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases

[Junio C Hamano]

"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;
>  }

Re: [PATCH 1/2] am: pay attention to user-defined context size

[Elijah Newren]

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

Re: [PATCH 1/2] am: pay attention to user-defined context size

[Elijah Newren]

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

Re: [PATCH 1/2] am: pay attention to user-defined context size

[Junio C Hamano]

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

Re: [PATCH 1/2] am: pay attention to user-defined context size

[Elijah Newren]

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.

Re: [PATCH 2/2] rebase: fix saving of --signoff state for am-based rebases

[Johannes Schindelin]

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;
> >  }
>