threads / patch / 52497

patch, 2 partsRebase am fixes

Subject: [PATCH 0/2] Rebase am fixes

## tl;dr

10 messages between Dec 20, 2019 and Jan 1, 2020. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Elijah Newren via GitGitGadget· Dec 20, 2019, 18:53 UTC · lore
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
Elijah Newren via GitGitGadget· Dec 20, 2019, 18:53 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to builtin/am.c +1 −1
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
Junio C Hamano· Dec 20, 2019, 19:28 UTC · re: Elijah Newren via GitGitGadget · lore

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

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> 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.

Elijah Newren· Dec 20, 2019, 19:38 UTC · re: Junio C Hamano · lore

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

On Fri, Dec 20, 2019 at 11:28 AM Junio C Hamano <gitster@pobox.com> wrote:
Show 13 quoted lines
>
> "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?)

Elijah Newren· Dec 20, 2019, 22:22 UTC · re: Elijah Newren · lore

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

Hi,
On Fri, Dec 20, 2019 at 11:38 AM Elijah Newren <newren@gmail.com> wrote:
Show 10 quoted lines
>
> 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...
Junio C Hamano· Dec 20, 2019, 22:34 UTC · re: Elijah Newren · lore

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

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

Elijah Newren· Dec 21, 2019, 05:37 UTC · re: Junio C Hamano · lore

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

On Fri, Dec 20, 2019 at 2:34 PM Junio C Hamano <gitster@pobox.com> wrote:
Show 11 quoted lines
>
> 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.

Elijah Newren via GitGitGadget· Dec 20, 2019, 18:53 UTC · re: Elijah Newren via GitGitGadget · lore

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

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(-)
Show changes to builtin/rebase.c +1 −1
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
Junio C Hamano· Dec 20, 2019, 19:29 UTC · re: Elijah Newren via GitGitGadget · lore

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

"Elijah Newren via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 5 quoted lines
> 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.

Show 19 quoted lines
>
> 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;
>  }
Johannes Schindelin· Jan 1, 2020, 22:01 UTC · re: Junio C Hamano · lore

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

Hi,
On Fri, 20 Dec 2019, Junio C Hamano wrote:
Show 9 quoted lines
> "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

Show 25 quoted lines
>
> 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;
> >  }
>

← back to recent threads