threads / bug / 52353

Bug with "git rebase" when format.useAutoBase is set

Subject: Bug with "git rebase" when format.useAutoBase is set

## tl;dr

33 messages between Nov 26, 2019 and Dec 4, 2019.

replies: 32people: 5as markdown or json

Christian Biesinger· Nov 26, 2019, 20:50 UTC · lore
Hello!

I recently tried setting the format.useAutoBase setting to true so that patches I send with git format-patch identify their base commit.

Unfortunately this seems to break "git rebase" completely, I always get the following error. Is this expected? This is with git version 2.24.0.432.g9d3f5f5b63-goog

$ git rebase --onto foo 4a7dabea39556979cbf6bcbdd98a9413ce129fd0 First, rewinding head to replay your work on top of it... fatal: failed to get upstream, if you want to record base commit automatically, please use git branch --set-upstream-to to track a remote branch. Or you could specify base commit by --base=<base-commit-id> manually error: git encountered an error while preparing the patches to replay these revisions:

    4a7dabea39556979cbf6bcbdd98a9413ce129fd0..12f4dc41bf9f018ecc1042830d5534de941f329e
As a result, git cannot rebase them.
Denton Liu· Nov 27, 2019, 02:09 UTC · re: Christian Biesinger · lore

[PATCH 0/5] rebase: fix breakage with `format.useAutoBase`

Thanks for reporting the breakage, Christian.

Apparently, this use case has been broken for a long time... Since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26). I'm surprised it's only been reported now.

This patchset fixes the breakage by teaching `git format-patch --no-base` and making rebase use it.

Denton Liu (5):
  t3400: demonstrate failure with format.useAutoBase
  format-patch: fix indentation
  t4014: use `test_config`
  format-patch: teach --no-base
  rebase: fix `format.useAutoBase` breakage
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      | 26 ++++++++++++++++++++++----
 builtin/rebase.c                   |  3 ++-
 t/t3400-rebase.sh                  |  6 ++++++
 t/t4014-format-patch.sh            | 14 +++++++++-----
 5 files changed, 42 insertions(+), 12 deletions(-)
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 02:09 UTC · re: Denton Liu · lore

[PATCH 1/5] t3400: demonstrate failure with format.useAutoBase

Ever since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26), `git rebase` has been broken when `format.useAutoBase = true`. Demonstrate that failure here.

Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3400-rebase.sh | 6 ++++++
 1 file changed, 6 insertions(+)
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ab18ac5f28..ca99e8c6c4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,6 +159,12 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
+test_expect_failure 'rebase works with format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git checkout topic &&
+	git rebase master
+'
+
 test_expect_success 'default to common base in @{upstream}s reflog if no upstream arg' '
 	git checkout -b default-base master &&
 	git checkout -b default topic &&
-- 
2.24.0.504.g3cd56eb17d
Eric Sunshine· Nov 27, 2019, 02:26 UTC · re: Denton Liu · lore

Re: [PATCH 1/5] t3400: demonstrate failure with format.useAutoBase

On Tue, Nov 26, 2019 at 9:09 PM Denton Liu <liu.denton@gmail.com> wrote:
> Ever since bb52995f3e (format-patch: introduce format.useAutoBase
> configuration, 2016-04-26), `git rebase` has been broken when
> `format.useAutoBase = true`. Demonstrate that failure here.
What specifically does "broken" mean here?
Show 10 quoted lines
> Reported-by: Christian Biesinger <cbiesinger@google.com>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
> @@ -159,6 +159,12 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
> +test_expect_failure 'rebase works with format.useAutoBase' '
> +       test_config format.useAutoBase true &&
> +       git checkout topic &&
> +       git rebase master
> +'

Having read both the commit message and the test itself, I'm not wiser about what is actually "broken" or what this is demonstrating.

Junio C Hamano· Nov 30, 2019, 17:25 UTC · re: Eric Sunshine · lore

Re: [PATCH 1/5] t3400: demonstrate failure with format.useAutoBase

Eric Sunshine <sunshine@sunshineco.com> writes:
Show 20 quoted lines
> On Tue, Nov 26, 2019 at 9:09 PM Denton Liu <liu.denton@gmail.com> wrote:
>> Ever since bb52995f3e (format-patch: introduce format.useAutoBase
>> configuration, 2016-04-26), `git rebase` has been broken when
>> `format.useAutoBase = true`. Demonstrate that failure here.
>
> What specifically does "broken" mean here?
>
>> Reported-by: Christian Biesinger <cbiesinger@google.com>
>> Signed-off-by: Denton Liu <liu.denton@gmail.com>
>> ---
>> diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
>> @@ -159,6 +159,12 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
>> +test_expect_failure 'rebase works with format.useAutoBase' '
>> +       test_config format.useAutoBase true &&
>> +       git checkout topic &&
>> +       git rebase master
>> +'
>
> Having read both the commit message and the test itself, I'm not wiser
> about what is actually "broken" or what this is demonstrating.
True.

The tests must be crystal clear what kind of brokenness it is demonstrating, not just "this test is expected to fail", especially when the "expect failure in one step, fix and flip expectation in a separate step" pattern is used; otherwise it becomes doubly puzzling.

Thanks.
Denton Liu· Nov 27, 2019, 02:09 UTC · re: Denton Liu · lore

[PATCH 2/5] format-patch: fix indentation

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/log.c b/builtin/log.c
index a26f223ab4..9c44682f61 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1350,7 +1350,7 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
 		string_list_clear(&extra_to, 0);
 		string_list_clear(&extra_cc, 0);
 	} else {
-	    add_header(arg);
+		add_header(arg);
 	}
 	return 0;
 }
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 02:09 UTC · re: Denton Liu · lore

[PATCH 3/5] t4014: use `test_config`

Instead of manually unsetting the config after the test case is done, use `test_config` to do it automatically. While we're at it, fix a typo in a test case name.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4014-format-patch.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 69267b16f0..c7cc643adf 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1939,10 +1939,9 @@ test_expect_success 'format-patch errors out when history involves criss-cross'
 	test_must_fail 	git format-patch --base=auto -1
 '
 
-test_expect_success 'format-patch format.useAutoBaseoption' '
-	test_when_finished "git config --unset format.useAutoBase" &&
+test_expect_success 'format-patch format.useAutoBase option' '
 	git checkout local &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse upstream >commit-id-base &&
@@ -1951,8 +1950,7 @@ test_expect_success 'format-patch format.useAutoBaseoption' '
 '
 
 test_expect_success 'format-patch --base overrides format.useAutoBase' '
-	test_when_finished "git config --unset format.useAutoBase" &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout --base=HEAD~1 -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse HEAD~1 >commit-id-base &&
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 02:09 UTC · re: Denton Liu · lore

[PATCH 4/5] format-patch: teach --no-base

If `format.useAutoBase = true`, there was no way to override this from the command-line. Teach format-patch the `--no-base` option which overrides `format.useAutoBase`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      | 24 +++++++++++++++++++++---
 t/t4014-format-patch.sh            |  6 ++++++
 3 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 00bdf9b125..0d4f8951bb 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -333,11 +333,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
   Output an all-zero hash in each patch's From header instead
   of the hash of the commit.
 
---base=<commit>::
+--[no-]base[=<commit>]::
 	Record the base tree information to identify the state the
 	patch series applies to.  See the BASE TREE INFORMATION section
 	below for details. If <commit> is "auto", a base commit is
-	automatically chosen.
+	automatically chosen. The `--no-base` option overrides a
+	`format.useAutoBase` configuration.
 
 --root::
 	Treat the revision argument as a <revision range>, even if it
diff --git a/builtin/log.c b/builtin/log.c
index 9c44682f61..c017df4056 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1388,6 +1388,23 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int base_callback(const struct option *opt, const char *arg, int unset)
+{
+	char **base_commit = opt->value;
+
+	free(*base_commit);
+
+	if (unset) {
+		base_auto = 0;
+		*base_commit = NULL;
+	} else if (arg) {
+		*base_commit = xstrdup(arg);
+	} else {
+		BUG("arg is NULL");
+	}
+	return 0;
+}
+
 struct base_tree_info {
 	struct object_id base_commit;
 	int nr_patch_id, alloc_patch_id;
@@ -1676,10 +1693,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, thread_callback },
 		OPT_STRING(0, "signature", &signature, N_("signature"),
 			    N_("add a signature")),
-		OPT_STRING(0, "base", &base_commit, N_("base-commit"),
-			   N_("add prerequisite tree info to the patch series")),
+		{ OPTION_CALLBACK, 0, "base", &base_commit, N_("base-commit"),
+			   N_("add prerequisite tree info to the patch series"),
+			   0, base_callback },
 		OPT_FILENAME(0, "signature-file", &signature_file,
-				N_("add a signature from a file")),
+			N_("add a signature from a file")),
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
 		OPT_BOOL(0, "progress", &show_progress,
 			 N_("show progress while generating patches")),
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c7cc643adf..a5b6302a1c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1958,6 +1958,12 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format-patch --no-base overrides format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git format-patch --stdout --no-base -1 >patch &&
+	! grep "^base-commit:" patch
+'
+
 test_expect_success 'format-patch --base with --attach' '
 	git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
 	sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 02:09 UTC · re: Denton Liu · lore

[PATCH 5/5] rebase: fix `format.useAutoBase` breakage

With `format.useAutoBase = true`, running rebase resulted in an error when an upstream was't set:

	fatal: failed to get upstream, if you want to record base commit automatically,
	please use git branch --set-upstream-to to track a remote branch.
	Or you could specify base commit by --base=<base-commit-id> manually
	error:
	git encountered an error while preparing the patches to replay
	these revisions:
	    ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
	As a result, git cannot rebase them.

Fix this by always passing `--no-base` to format-patch from rebase so that the effect of `format.useAutoBase` is negated.

Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/rebase.c  | 3 ++-
 t/t3400-rebase.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e755087b0f..51980ab63d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1012,7 +1012,8 @@ static int run_am(struct rebase_options *opts)
 	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
 			 "--full-index", "--cherry-pick", "--right-only",
 			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
-			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order", NULL);
+			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
+			 "--no-base", NULL);
 	if (opts->git_format_patch_opt.len)
 		argv_array_split(&format_patch.args,
 				 opts->git_format_patch_opt.buf);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ca99e8c6c4..1323f30fee 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,7 +159,7 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
-test_expect_failure 'rebase works with format.useAutoBase' '
+test_expect_success 'rebase works with format.useAutoBase' '
 	test_config format.useAutoBase true &&
 	git checkout topic &&
 	git rebase master
-- 
2.24.0.504.g3cd56eb17d
Eric Sunshine· Nov 27, 2019, 02:42 UTC · re: Denton Liu · lore

Re: [PATCH 5/5] rebase: fix `format.useAutoBase` breakage

On Tue, Nov 26, 2019 at 9:09 PM Denton Liu <liu.denton@gmail.com> wrote:
> With `format.useAutoBase = true`, running rebase resulted in an
> error when an upstream was't set:
s/was't/wasn't/
Show 16 quoted lines
>         fatal: failed to get upstream, if you want to record base commit automatically,
>         please use git branch --set-upstream-to to track a remote branch.
>         Or you could specify base commit by --base=<base-commit-id> manually
>         error:
>         git encountered an error while preparing the patches to replay
>         these revisions:
>
>             ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
>
>         As a result, git cannot rebase them.
>
> Fix this by always passing `--no-base` to format-patch from rebase so
> that the effect of `format.useAutoBase` is negated.
>
> Reported-by: Christian Biesinger <cbiesinger@google.com>
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
Christian Biesinger· Nov 27, 2019, 17:14 UTC · re: Denton Liu · lore

Re: [PATCH 0/5] rebase: fix breakage with `format.useAutoBase`

On Tue, Nov 26, 2019 at 8:09 PM Denton Liu <liu.denton@gmail.com> wrote:
Show 6 quoted lines
>
> Thanks for reporting the breakage, Christian.
>
> Apparently, this use case has been broken for a long time... Since
> bb52995f3e (format-patch: introduce format.useAutoBase configuration,
> 2016-04-26). I'm surprised it's only been reported now.

Thanks for fixing! Maybe few people use this option as it's not documented in the format-patch manpage? (A separate thing to fix, I guess)

Christian
Show 20 quoted lines
> This patchset fixes the breakage by teaching
> `git format-patch --no-base` and making rebase use it.
>
> Denton Liu (5):
>   t3400: demonstrate failure with format.useAutoBase
>   format-patch: fix indentation
>   t4014: use `test_config`
>   format-patch: teach --no-base
>   rebase: fix `format.useAutoBase` breakage
>
>  Documentation/git-format-patch.txt |  5 +++--
>  builtin/log.c                      | 26 ++++++++++++++++++++++----
>  builtin/rebase.c                   |  3 ++-
>  t/t3400-rebase.sh                  |  6 ++++++
>  t/t4014-format-patch.sh            | 14 +++++++++-----
>  5 files changed, 42 insertions(+), 12 deletions(-)
>
> --
> 2.24.0.504.g3cd56eb17d
>
Denton Liu· Nov 27, 2019, 18:13 UTC · re: Denton Liu · lore

[PATCH v2 0/5] rebase: fix breakage with `format.useAutoBase`

Apparently, this use case has been broken for a long time... Since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26). I'm surprised it's only been reported now.

This patchset fixes the breakage by teaching `git format-patch --no-base` and making rebase use it.

Changes since v1:
* Update some log messages
Denton Liu (5):
  t3400: demonstrate failure with format.useAutoBase
  format-patch: fix indentation
  t4014: use test_config()
  format-patch: teach --no-base
  rebase: fix format.useAutoBase breakage
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      | 26 ++++++++++++++++++++++----
 builtin/rebase.c                   |  3 ++-
 t/t3400-rebase.sh                  |  6 ++++++
 t/t4014-format-patch.sh            | 14 +++++++++-----
 5 files changed, 42 insertions(+), 12 deletions(-)
Range-diff against v1:
1:  a1741e5434 ! 1:  4089e51041 t3400: demonstrate failure with format.useAutoBase
    @@ Commit message
     
         Ever since bb52995f3e (format-patch: introduce format.useAutoBase
         configuration, 2016-04-26), `git rebase` has been broken when
    -    `format.useAutoBase = true`. Demonstrate that failure here.
    +    `format.useAutoBase = true`. It fails when rebasing a branch that
    +    doesn't have an upstream set:
    +
    +            fatal: failed to get upstream, if you want to record base commit automatically,
    +            please use git branch --set-upstream-to to track a remote branch.
    +            Or you could specify base commit by --base=<base-commit-id> manually
    +            error:
    +            git encountered an error while preparing the patches to replay
    +            these revisions:
    +
    +                ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
    +
    +            As a result, git cannot rebase them.
    +
    +    Demonstrate that failure here.
     
         Reported-by: Christian Biesinger <cbiesinger@google.com>
     
2:  46fd4113aa = 2:  d288d6c3a5 format-patch: fix indentation
3:  22b1fb14f9 ! 3:  196b5d8dbc t4014: use `test_config`
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    t4014: use `test_config`
    +    t4014: use test_config()
     
         Instead of manually unsetting the config after the test case is done,
    -    use `test_config` to do it automatically. While we're at it, fix a typo
    +    use test_config() to do it automatically. While we're at it, fix a typo
         in a test case name.
     
      ## t/t4014-format-patch.sh ##
4:  e072c36e6a = 4:  f7e5325cc0 format-patch: teach --no-base
5:  15e6ccb203 ! 5:  62c59c12e3 rebase: fix `format.useAutoBase` breakage
    @@ Metadata
     Author: Denton Liu <liu.denton@gmail.com>
     
      ## Commit message ##
    -    rebase: fix `format.useAutoBase` breakage
    +    rebase: fix format.useAutoBase breakage
     
         With `format.useAutoBase = true`, running rebase resulted in an
    -    error when an upstream was't set:
    +    error when an upstream wasn't set:
     
                 fatal: failed to get upstream, if you want to record base commit automatically,
                 please use git branch --set-upstream-to to track a remote branch.
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 18:13 UTC · re: Denton Liu · lore

[PATCH v2 1/5] t3400: demonstrate failure with format.useAutoBase

Ever since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26), `git rebase` has been broken when `format.useAutoBase = true`. It fails when rebasing a branch that doesn't have an upstream set:

	fatal: failed to get upstream, if you want to record base commit automatically,
	please use git branch --set-upstream-to to track a remote branch.
	Or you could specify base commit by --base=<base-commit-id> manually
	error:
	git encountered an error while preparing the patches to replay
	these revisions:
	    ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
	As a result, git cannot rebase them.
Demonstrate that failure here.
Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3400-rebase.sh | 6 ++++++
 1 file changed, 6 insertions(+)
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ab18ac5f28..ca99e8c6c4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,6 +159,12 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
+test_expect_failure 'rebase works with format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git checkout topic &&
+	git rebase master
+'
+
 test_expect_success 'default to common base in @{upstream}s reflog if no upstream arg' '
 	git checkout -b default-base master &&
 	git checkout -b default topic &&
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 18:13 UTC · re: Denton Liu · lore

[PATCH v2 2/5] format-patch: fix indentation

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/log.c b/builtin/log.c
index a26f223ab4..9c44682f61 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1350,7 +1350,7 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
 		string_list_clear(&extra_to, 0);
 		string_list_clear(&extra_cc, 0);
 	} else {
-	    add_header(arg);
+		add_header(arg);
 	}
 	return 0;
 }
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 18:13 UTC · re: Denton Liu · lore

[PATCH v2 3/5] t4014: use test_config()

Instead of manually unsetting the config after the test case is done, use test_config() to do it automatically. While we're at it, fix a typo in a test case name.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4014-format-patch.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 69267b16f0..c7cc643adf 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1939,10 +1939,9 @@ test_expect_success 'format-patch errors out when history involves criss-cross'
 	test_must_fail 	git format-patch --base=auto -1
 '
 
-test_expect_success 'format-patch format.useAutoBaseoption' '
-	test_when_finished "git config --unset format.useAutoBase" &&
+test_expect_success 'format-patch format.useAutoBase option' '
 	git checkout local &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse upstream >commit-id-base &&
@@ -1951,8 +1950,7 @@ test_expect_success 'format-patch format.useAutoBaseoption' '
 '
 
 test_expect_success 'format-patch --base overrides format.useAutoBase' '
-	test_when_finished "git config --unset format.useAutoBase" &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout --base=HEAD~1 -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse HEAD~1 >commit-id-base &&
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 18:13 UTC · re: Denton Liu · lore

[PATCH v2 4/5] format-patch: teach --no-base

If `format.useAutoBase = true`, there was no way to override this from the command-line. Teach format-patch the `--no-base` option which overrides `format.useAutoBase`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      | 24 +++++++++++++++++++++---
 t/t4014-format-patch.sh            |  6 ++++++
 3 files changed, 30 insertions(+), 5 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 00bdf9b125..0d4f8951bb 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -333,11 +333,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
   Output an all-zero hash in each patch's From header instead
   of the hash of the commit.
 
---base=<commit>::
+--[no-]base[=<commit>]::
 	Record the base tree information to identify the state the
 	patch series applies to.  See the BASE TREE INFORMATION section
 	below for details. If <commit> is "auto", a base commit is
-	automatically chosen.
+	automatically chosen. The `--no-base` option overrides a
+	`format.useAutoBase` configuration.
 
 --root::
 	Treat the revision argument as a <revision range>, even if it
diff --git a/builtin/log.c b/builtin/log.c
index 9c44682f61..c017df4056 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1388,6 +1388,23 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int base_callback(const struct option *opt, const char *arg, int unset)
+{
+	char **base_commit = opt->value;
+
+	free(*base_commit);
+
+	if (unset) {
+		base_auto = 0;
+		*base_commit = NULL;
+	} else if (arg) {
+		*base_commit = xstrdup(arg);
+	} else {
+		BUG("arg is NULL");
+	}
+	return 0;
+}
+
 struct base_tree_info {
 	struct object_id base_commit;
 	int nr_patch_id, alloc_patch_id;
@@ -1676,10 +1693,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, thread_callback },
 		OPT_STRING(0, "signature", &signature, N_("signature"),
 			    N_("add a signature")),
-		OPT_STRING(0, "base", &base_commit, N_("base-commit"),
-			   N_("add prerequisite tree info to the patch series")),
+		{ OPTION_CALLBACK, 0, "base", &base_commit, N_("base-commit"),
+			   N_("add prerequisite tree info to the patch series"),
+			   0, base_callback },
 		OPT_FILENAME(0, "signature-file", &signature_file,
-				N_("add a signature from a file")),
+			N_("add a signature from a file")),
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
 		OPT_BOOL(0, "progress", &show_progress,
 			 N_("show progress while generating patches")),
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c7cc643adf..a5b6302a1c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1958,6 +1958,12 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format-patch --no-base overrides format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git format-patch --stdout --no-base -1 >patch &&
+	! grep "^base-commit:" patch
+'
+
 test_expect_success 'format-patch --base with --attach' '
 	git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
 	sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
-- 
2.24.0.504.g3cd56eb17d
Denton Liu· Nov 27, 2019, 19:56 UTC · re: Denton Liu · lore

Re: [PATCH v2 4/5] format-patch: teach --no-base

On Wed, Nov 27, 2019 at 10:13:46AM -0800, Denton Liu wrote:
Show 70 quoted lines
> If `format.useAutoBase = true`, there was no way to override this from
> the command-line. Teach format-patch the `--no-base` option which
> overrides `format.useAutoBase`.
> 
> Signed-off-by: Denton Liu <liu.denton@gmail.com>
> ---
>  Documentation/git-format-patch.txt |  5 +++--
>  builtin/log.c                      | 24 +++++++++++++++++++++---
>  t/t4014-format-patch.sh            |  6 ++++++
>  3 files changed, 30 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
> index 00bdf9b125..0d4f8951bb 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -333,11 +333,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
>    Output an all-zero hash in each patch's From header instead
>    of the hash of the commit.
>  
> ---base=<commit>::
> +--[no-]base[=<commit>]::
>  	Record the base tree information to identify the state the
>  	patch series applies to.  See the BASE TREE INFORMATION section
>  	below for details. If <commit> is "auto", a base commit is
> -	automatically chosen.
> +	automatically chosen. The `--no-base` option overrides a
> +	`format.useAutoBase` configuration.
>  
>  --root::
>  	Treat the revision argument as a <revision range>, even if it
> diff --git a/builtin/log.c b/builtin/log.c
> index 9c44682f61..c017df4056 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1388,6 +1388,23 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>  
> +static int base_callback(const struct option *opt, const char *arg, int unset)
> +{
> +	char **base_commit = opt->value;
> +
> +	free(*base_commit);
> +
> +	if (unset) {
> +		base_auto = 0;
> +		*base_commit = NULL;
> +	} else if (arg) {
> +		*base_commit = xstrdup(arg);
> +	} else {
> +		BUG("arg is NULL");
> +	}
> +	return 0;
> +}
> +
>  struct base_tree_info {
>  	struct object_id base_commit;
>  	int nr_patch_id, alloc_patch_id;
> @@ -1676,10 +1693,11 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			    PARSE_OPT_OPTARG, thread_callback },
>  		OPT_STRING(0, "signature", &signature, N_("signature"),
>  			    N_("add a signature")),
> -		OPT_STRING(0, "base", &base_commit, N_("base-commit"),
> -			   N_("add prerequisite tree info to the patch series")),
> +		{ OPTION_CALLBACK, 0, "base", &base_commit, N_("base-commit"),
> +			   N_("add prerequisite tree info to the patch series"),
> +			   0, base_callback },
>  		OPT_FILENAME(0, "signature-file", &signature_file,
> -				N_("add a signature from a file")),
> +			N_("add a signature from a file")),
Oops, this is reindentation is a spurious change and should be dropped.
Show 23 quoted lines
>  		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
>  		OPT_BOOL(0, "progress", &show_progress,
>  			 N_("show progress while generating patches")),
> diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
> index c7cc643adf..a5b6302a1c 100755
> --- a/t/t4014-format-patch.sh
> +++ b/t/t4014-format-patch.sh
> @@ -1958,6 +1958,12 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'format-patch --no-base overrides format.useAutoBase' '
> +	test_config format.useAutoBase true &&
> +	git format-patch --stdout --no-base -1 >patch &&
> +	! grep "^base-commit:" patch
> +'
> +
>  test_expect_success 'format-patch --base with --attach' '
>  	git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
>  	sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
> -- 
> 2.24.0.504.g3cd56eb17d
> 
Denton Liu· Nov 27, 2019, 18:13 UTC · re: Denton Liu · lore

[PATCH v2 5/5] rebase: fix format.useAutoBase breakage

With `format.useAutoBase = true`, running rebase resulted in an error when an upstream wasn't set:

	fatal: failed to get upstream, if you want to record base commit automatically,
	please use git branch --set-upstream-to to track a remote branch.
	Or you could specify base commit by --base=<base-commit-id> manually
	error:
	git encountered an error while preparing the patches to replay
	these revisions:
	    ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
	As a result, git cannot rebase them.

Fix this by always passing `--no-base` to format-patch from rebase so that the effect of `format.useAutoBase` is negated.

Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/rebase.c  | 3 ++-
 t/t3400-rebase.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e755087b0f..51980ab63d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1012,7 +1012,8 @@ static int run_am(struct rebase_options *opts)
 	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
 			 "--full-index", "--cherry-pick", "--right-only",
 			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
-			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order", NULL);
+			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
+			 "--no-base", NULL);
 	if (opts->git_format_patch_opt.len)
 		argv_array_split(&format_patch.args,
 				 opts->git_format_patch_opt.buf);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ca99e8c6c4..1323f30fee 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,7 +159,7 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
-test_expect_failure 'rebase works with format.useAutoBase' '
+test_expect_success 'rebase works with format.useAutoBase' '
 	test_config format.useAutoBase true &&
 	git checkout topic &&
 	git rebase master
-- 
2.24.0.504.g3cd56eb17d
Christian Biesinger· Dec 4, 2019, 16:21 UTC · re: Denton Liu · lore

Re: [PATCH v2 5/5] rebase: fix format.useAutoBase breakage

On Wed, Nov 27, 2019 at 12:13 PM Denton Liu <liu.denton@gmail.com> wrote:
>
> With `format.useAutoBase = true`, running rebase resulted in an
> error when an upstream wasn't set:
FWIW, this also failed for me when an upstream was set.
Christian
Denton Liu· Dec 4, 2019, 07:47 UTC · re: Denton Liu · lore

[PATCH v3 0/5] rebase: fix breakage with `format.useAutoBase`

Apparently, this use case has been broken for a long time... Since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26). I'm surprised it's only been reported now.

This patchset fixes the breakage by teaching `git format-patch --no-base` and making rebase use it.

This patch is based on the latest master since it's such an old bug that only got noticed recently, I'm not sure if it's worth fixing in 'maint'. The series does not apply cleanly on top of 'maint' because it relies on c1a6f21cd4 (Doc: add more detail for git-format-patch, 2019-08-27) but the conflicts are relatively minor.

Changes since v2:
* Remove spurious indentation change
* Rebase onto the latest master
Changes since v1:
* Update some log messages
Denton Liu (5):
  t3400: demonstrate failure with format.useAutoBase
  format-patch: fix indentation
  t4014: use test_config()
  format-patch: teach --no-base
  rebase: fix format.useAutoBase breakage
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      | 24 +++++++++++++++++++++---
 builtin/rebase.c                   |  3 ++-
 t/t3400-rebase.sh                  |  6 ++++++
 t/t4014-format-patch.sh            | 14 +++++++++-----
 5 files changed, 41 insertions(+), 11 deletions(-)
Range-diff against v2:
1:  4089e51041 = 1:  8d67bbe5bf t3400: demonstrate failure with format.useAutoBase
2:  d288d6c3a5 = 2:  8cfde9f98e format-patch: fix indentation
3:  196b5d8dbc = 3:  638c4add00 t4014: use test_config()
4:  f7e5325cc0 ! 4:  6cba51ca24 format-patch: teach --no-base
    @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *pre
     +			   N_("add prerequisite tree info to the patch series"),
     +			   0, base_callback },
      		OPT_FILENAME(0, "signature-file", &signature_file,
    --				N_("add a signature from a file")),
    -+			N_("add a signature from a file")),
    + 				N_("add a signature from a file")),
      		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
    - 		OPT_BOOL(0, "progress", &show_progress,
    - 			 N_("show progress while generating patches")),
     
      ## t/t4014-format-patch.sh ##
     @@ t/t4014-format-patch.sh: test_expect_success 'format-patch --base overrides format.useAutoBase' '
5:  62c59c12e3 = 5:  eb266aaedc rebase: fix format.useAutoBase breakage
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 07:47 UTC · re: Denton Liu · lore

[PATCH v3 1/5] t3400: demonstrate failure with format.useAutoBase

Ever since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26), `git rebase` has been broken when `format.useAutoBase = true`. It fails when rebasing a branch that doesn't have an upstream set:

	fatal: failed to get upstream, if you want to record base commit automatically,
	please use git branch --set-upstream-to to track a remote branch.
	Or you could specify base commit by --base=<base-commit-id> manually
	error:
	git encountered an error while preparing the patches to replay
	these revisions:
	    ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
	As a result, git cannot rebase them.
Demonstrate that failure here.
Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3400-rebase.sh | 6 ++++++
 1 file changed, 6 insertions(+)
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ab18ac5f28..ca99e8c6c4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,6 +159,12 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
+test_expect_failure 'rebase works with format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git checkout topic &&
+	git rebase master
+'
+
 test_expect_success 'default to common base in @{upstream}s reflog if no upstream arg' '
 	git checkout -b default-base master &&
 	git checkout -b default topic &&
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 07:47 UTC · re: Denton Liu · lore

[PATCH v3 2/5] format-patch: fix indentation

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/log.c b/builtin/log.c
index a26f223ab4..9c44682f61 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1350,7 +1350,7 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
 		string_list_clear(&extra_to, 0);
 		string_list_clear(&extra_cc, 0);
 	} else {
-	    add_header(arg);
+		add_header(arg);
 	}
 	return 0;
 }
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 07:47 UTC · re: Denton Liu · lore

[PATCH v3 4/5] format-patch: teach --no-base

If `format.useAutoBase = true`, there was no way to override this from the command-line. Teach format-patch the `--no-base` option which overrides `format.useAutoBase`.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      | 22 ++++++++++++++++++++--
 t/t4014-format-patch.sh            |  6 ++++++
 3 files changed, 29 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 00bdf9b125..0d4f8951bb 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -333,11 +333,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
   Output an all-zero hash in each patch's From header instead
   of the hash of the commit.
 
---base=<commit>::
+--[no-]base[=<commit>]::
 	Record the base tree information to identify the state the
 	patch series applies to.  See the BASE TREE INFORMATION section
 	below for details. If <commit> is "auto", a base commit is
-	automatically chosen.
+	automatically chosen. The `--no-base` option overrides a
+	`format.useAutoBase` configuration.
 
 --root::
 	Treat the revision argument as a <revision range>, even if it
diff --git a/builtin/log.c b/builtin/log.c
index 9c44682f61..645d6db7cc 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1388,6 +1388,23 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
+static int base_callback(const struct option *opt, const char *arg, int unset)
+{
+	char **base_commit = opt->value;
+
+	free(*base_commit);
+
+	if (unset) {
+		base_auto = 0;
+		*base_commit = NULL;
+	} else if (arg) {
+		*base_commit = xstrdup(arg);
+	} else {
+		BUG("arg is NULL");
+	}
+	return 0;
+}
+
 struct base_tree_info {
 	struct object_id base_commit;
 	int nr_patch_id, alloc_patch_id;
@@ -1676,8 +1693,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 			    PARSE_OPT_OPTARG, thread_callback },
 		OPT_STRING(0, "signature", &signature, N_("signature"),
 			    N_("add a signature")),
-		OPT_STRING(0, "base", &base_commit, N_("base-commit"),
-			   N_("add prerequisite tree info to the patch series")),
+		{ OPTION_CALLBACK, 0, "base", &base_commit, N_("base-commit"),
+			   N_("add prerequisite tree info to the patch series"),
+			   0, base_callback },
 		OPT_FILENAME(0, "signature-file", &signature_file,
 				N_("add a signature from a file")),
 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c7cc643adf..a5b6302a1c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1958,6 +1958,12 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format-patch --no-base overrides format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git format-patch --stdout --no-base -1 >patch &&
+	! grep "^base-commit:" patch
+'
+
 test_expect_success 'format-patch --base with --attach' '
 	git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
 	sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
-- 
2.24.0.578.g4820254054
René Scharfe· Dec 4, 2019, 10:36 UTC · re: Denton Liu · lore

Re: [PATCH v3 4/5] format-patch: teach --no-base

Am 04.12.19 um 08:47 schrieb Denton Liu:
> If `format.useAutoBase = true`, there was no way to override this from
> the command-line. Teach format-patch the `--no-base` option which
> overrides `format.useAutoBase`.
Show 40 quoted lines
> diff --git a/builtin/log.c b/builtin/log.c
> index 9c44682f61..645d6db7cc 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1388,6 +1388,23 @@ static int from_callback(const struct option *opt, const char *arg, int unset)
>  	return 0;
>  }
>
> +static int base_callback(const struct option *opt, const char *arg, int unset)
> +{
> +	char **base_commit = opt->value;
> +
> +	free(*base_commit);
> +
> +	if (unset) {
> +		base_auto = 0;
> +		*base_commit = NULL;
> +	} else if (arg) {
> +		*base_commit = xstrdup(arg);
> +	} else {
> +		BUG("arg is NULL");
> +	}
> +	return 0;
> +}
> +
>  struct base_tree_info {
>  	struct object_id base_commit;
>  	int nr_patch_id, alloc_patch_id;
> @@ -1676,8 +1693,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  			    PARSE_OPT_OPTARG, thread_callback },
>  		OPT_STRING(0, "signature", &signature, N_("signature"),
>  			    N_("add a signature")),
> -		OPT_STRING(0, "base", &base_commit, N_("base-commit"),
> -			   N_("add prerequisite tree info to the patch series")),
> +		{ OPTION_CALLBACK, 0, "base", &base_commit, N_("base-commit"),
> +			   N_("add prerequisite tree info to the patch series"),
> +			   0, base_callback },
>  		OPT_FILENAME(0, "signature-file", &signature_file,
>  				N_("add a signature from a file")),
>  		OPT__QUIET(&quiet, N_("don't print the patch filenames")),

Clearing the global variable base_auto feels unclean to me, as does the introduction of a callback for that purpose. Why not set base_commit after reading the config and before parsing command line options to reflect base_auto? That would achieve the intended precedence in a simpler way, something like this:

diff --git a/builtin/log.c b/builtin/log.c
index a26f223ab4..af1b0d0209 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1714,6 +1714,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		rev.mime_boundary = default_attach;
 		rev.no_inline = 1;
 	}
+	if (base_auto)
+		base_commit = "auto";

 	/*
 	 * Parse the arguments before setup_revisions(), or something
@@ -1973,7 +1975,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}

 	memset(&bases, 0, sizeof(bases));
-	if (base_commit || base_auto) {
+	if (base_commit) {
 		struct commit *base = get_base_commit(base_commit, list, nr);
 		reset_revision_walk();
 		clear_object_flags(UNINTERESTING);
Junio C Hamano· Dec 4, 2019, 17:26 UTC · re: René Scharfe · lore

Re: [PATCH v3 4/5] format-patch: teach --no-base

René Scharfe <l.s.r@web.de> writes:
Show 5 quoted lines
> Clearing the global variable base_auto feels unclean to me, as does the
> introduction of a callback for that purpose.  Why not set base_commit
> after reading the config and before parsing command line options to
> reflect base_auto?  That would achieve the intended precedence in a
> simpler way, something like this:
Nice.
Show 23 quoted lines
>
> diff --git a/builtin/log.c b/builtin/log.c
> index a26f223ab4..af1b0d0209 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -1714,6 +1714,8 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  		rev.mime_boundary = default_attach;
>  		rev.no_inline = 1;
>  	}
> +	if (base_auto)
> +		base_commit = "auto";
>
>  	/*
>  	 * Parse the arguments before setup_revisions(), or something
> @@ -1973,7 +1975,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>  	}
>
>  	memset(&bases, 0, sizeof(bases));
> -	if (base_commit || base_auto) {
> +	if (base_commit) {
>  		struct commit *base = get_base_commit(base_commit, list, nr);
>  		reset_revision_walk();
>  		clear_object_flags(UNINTERESTING);
Denton Liu· Dec 4, 2019, 07:47 UTC · re: Denton Liu · lore

[PATCH v3 3/5] t4014: use test_config()

Instead of manually unsetting the config after the test case is done, use test_config() to do it automatically. While we're at it, fix a typo in a test case name.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4014-format-patch.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 69267b16f0..c7cc643adf 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1939,10 +1939,9 @@ test_expect_success 'format-patch errors out when history involves criss-cross'
 	test_must_fail 	git format-patch --base=auto -1
 '
 
-test_expect_success 'format-patch format.useAutoBaseoption' '
-	test_when_finished "git config --unset format.useAutoBase" &&
+test_expect_success 'format-patch format.useAutoBase option' '
 	git checkout local &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse upstream >commit-id-base &&
@@ -1951,8 +1950,7 @@ test_expect_success 'format-patch format.useAutoBaseoption' '
 '
 
 test_expect_success 'format-patch --base overrides format.useAutoBase' '
-	test_when_finished "git config --unset format.useAutoBase" &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout --base=HEAD~1 -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse HEAD~1 >commit-id-base &&
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 07:48 UTC · re: Denton Liu · lore

[PATCH v3 5/5] rebase: fix format.useAutoBase breakage

With `format.useAutoBase = true`, running rebase resulted in an error when an upstream wasn't set:

	fatal: failed to get upstream, if you want to record base commit automatically,
	please use git branch --set-upstream-to to track a remote branch.
	Or you could specify base commit by --base=<base-commit-id> manually
	error:
	git encountered an error while preparing the patches to replay
	these revisions:
	    ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
	As a result, git cannot rebase them.

Fix this by always passing `--no-base` to format-patch from rebase so that the effect of `format.useAutoBase` is negated.

Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/rebase.c  | 3 ++-
 t/t3400-rebase.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e755087b0f..51980ab63d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1012,7 +1012,8 @@ static int run_am(struct rebase_options *opts)
 	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
 			 "--full-index", "--cherry-pick", "--right-only",
 			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
-			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order", NULL);
+			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
+			 "--no-base", NULL);
 	if (opts->git_format_patch_opt.len)
 		argv_array_split(&format_patch.args,
 				 opts->git_format_patch_opt.buf);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ca99e8c6c4..1323f30fee 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,7 +159,7 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
-test_expect_failure 'rebase works with format.useAutoBase' '
+test_expect_success 'rebase works with format.useAutoBase' '
 	test_config format.useAutoBase true &&
 	git checkout topic &&
 	git rebase master
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 21:24 UTC · re: Denton Liu · lore

[PATCH v4 0/5] rebase: fix breakage with `format.useAutoBase`

Apparently, this use case has been broken for a long time... Since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26). I'm surprised it's only been reported now.

This patchset fixes the breakage by teaching `git format-patch --no-base` and making rebase use it.

This patch is based on the latest master since it's such an old bug that only got noticed recently, I'm not sure if it's worth fixing in 'maint'. The series does not apply cleanly on top of 'maint' because it relies on c1a6f21cd4 (Doc: add more detail for git-format-patch, 2019-08-27) but the conflicts are relatively minor.

Changes since v3:
* Correct error in log messages more precise by saying that rebases fail
  in general (even if an upstream is set)
* Change ugly use of OPT_CALLBACK into a more elegant form
Changes since v2:
* Remove spurious indentation change
* Rebase onto the latest master
Changes since v1:
* Update some log messages
Denton Liu (5):
  t3400: demonstrate failure with format.useAutoBase
  format-patch: fix indentation
  t4014: use test_config()
  format-patch: teach --no-base
  rebase: fix format.useAutoBase breakage
 Documentation/git-format-patch.txt |  5 +++--
 builtin/log.c                      |  9 ++++++---
 builtin/rebase.c                   |  3 ++-
 t/t3400-rebase.sh                  |  6 ++++++
 t/t4014-format-patch.sh            | 14 +++++++++-----
 5 files changed, 26 insertions(+), 11 deletions(-)
Range-diff against v3:
1:  78b928bf49 ! 1:  386148a550 t3400: demonstrate failure with format.useAutoBase
    @@ Commit message
     
         Ever since bb52995f3e (format-patch: introduce format.useAutoBase
         configuration, 2016-04-26), `git rebase` has been broken when
    -    `format.useAutoBase = true`. It fails when rebasing a branch that
    -    doesn't have an upstream set:
    +    `format.useAutoBase = true`. It fails when rebasing a branch:
     
                 fatal: failed to get upstream, if you want to record base commit automatically,
                 please use git branch --set-upstream-to to track a remote branch.
2:  5435a04427 = 2:  0464bd61c2 format-patch: fix indentation
3:  455f2df08d = 3:  a55eacbad7 t4014: use test_config()
4:  7c7b94c0c4 ! 4:  eb35de8f49 format-patch: teach --no-base
    @@ Commit message
         format-patch: teach --no-base
     
         If `format.useAutoBase = true`, there was no way to override this from
    -    the command-line. Teach format-patch the `--no-base` option which
    -    overrides `format.useAutoBase`.
    +    the command-line. Teach the `--no-base` option in format-patch to
    +    override `format.useAutoBase`.
    +
    +    Helped-by: René Scharfe <l.s.r@web.de>
     
      ## Documentation/git-format-patch.txt ##
     @@ Documentation/git-format-patch.txt: you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
    @@ Documentation/git-format-patch.txt: you can use `--suffix=-patch` to get `0001-d
      	Treat the revision argument as a <revision range>, even if it
     
      ## builtin/log.c ##
    -@@ builtin/log.c: static int from_callback(const struct option *opt, const char *arg, int unset)
    - 	return 0;
    - }
    - 
    -+static int base_callback(const struct option *opt, const char *arg, int unset)
    -+{
    -+	char **base_commit = opt->value;
    -+
    -+	free(*base_commit);
    -+
    -+	if (unset) {
    -+		base_auto = 0;
    -+		*base_commit = NULL;
    -+	} else if (arg) {
    -+		*base_commit = xstrdup(arg);
    -+	} else {
    -+		BUG("arg is NULL");
    -+	}
    -+	return 0;
    -+}
    -+
    - struct base_tree_info {
    - 	struct object_id base_commit;
    - 	int nr_patch_id, alloc_patch_id;
    +@@ builtin/log.c: static struct commit *get_base_commit(const char *base_commit,
    + 		base = lookup_commit_reference_by_name(base_commit);
    + 		if (!base)
    + 			die(_("unknown commit %s"), base_commit);
    +-	} else if ((base_commit && !strcmp(base_commit, "auto")) || base_auto) {
    ++	} else if ((base_commit && !strcmp(base_commit, "auto"))) {
    + 		struct branch *curr_branch = branch_get(NULL);
    + 		const char *upstream = branch_get_upstream(curr_branch, NULL);
    + 		if (upstream) {
     @@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
    - 			    PARSE_OPT_OPTARG, thread_callback },
    - 		OPT_STRING(0, "signature", &signature, N_("signature"),
    - 			    N_("add a signature")),
    --		OPT_STRING(0, "base", &base_commit, N_("base-commit"),
    --			   N_("add prerequisite tree info to the patch series")),
    -+		{ OPTION_CALLBACK, 0, "base", &base_commit, N_("base-commit"),
    -+			   N_("add prerequisite tree info to the patch series"),
    -+			   0, base_callback },
    - 		OPT_FILENAME(0, "signature-file", &signature_file,
    --				N_("add a signature from a file")),
    -+			N_("add a signature from a file")),
    - 		OPT__QUIET(&quiet, N_("don't print the patch filenames")),
    - 		OPT_BOOL(0, "progress", &show_progress,
    - 			 N_("show progress while generating patches")),
    + 	s_r_opt.def = "HEAD";
    + 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
    + 
    ++	if (base_auto)
    ++		base_commit = "auto";
    ++
    + 	if (default_attach) {
    + 		rev.mime_boundary = default_attach;
    + 		rev.no_inline = 1;
    +@@ builtin/log.c: int cmd_format_patch(int argc, const char **argv, const char *prefix)
    + 	}
    + 
    + 	memset(&bases, 0, sizeof(bases));
    +-	if (base_commit || base_auto) {
    ++	if (base_commit) {
    + 		struct commit *base = get_base_commit(base_commit, list, nr);
    + 		reset_revision_walk();
    + 		clear_object_flags(UNINTERESTING);
     
      ## t/t4014-format-patch.sh ##
     @@ t/t4014-format-patch.sh: test_expect_success 'format-patch --base overrides format.useAutoBase' '
5:  2b4166e371 ! 5:  210905f163 rebase: fix format.useAutoBase breakage
    @@ Commit message
         rebase: fix format.useAutoBase breakage
     
         With `format.useAutoBase = true`, running rebase resulted in an
    -    error when an upstream wasn't set:
    +    error:
     
                 fatal: failed to get upstream, if you want to record base commit automatically,
                 please use git branch --set-upstream-to to track a remote branch.
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 21:24 UTC · re: Denton Liu · lore

[PATCH v4 1/5] t3400: demonstrate failure with format.useAutoBase

Ever since bb52995f3e (format-patch: introduce format.useAutoBase configuration, 2016-04-26), `git rebase` has been broken when `format.useAutoBase = true`. It fails when rebasing a branch:

	fatal: failed to get upstream, if you want to record base commit automatically,
	please use git branch --set-upstream-to to track a remote branch.
	Or you could specify base commit by --base=<base-commit-id> manually
	error:
	git encountered an error while preparing the patches to replay
	these revisions:
	    ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
	As a result, git cannot rebase them.
Demonstrate that failure here.
Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t3400-rebase.sh | 6 ++++++
 1 file changed, 6 insertions(+)
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ab18ac5f28..ca99e8c6c4 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,6 +159,12 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
+test_expect_failure 'rebase works with format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git checkout topic &&
+	git rebase master
+'
+
 test_expect_success 'default to common base in @{upstream}s reflog if no upstream arg' '
 	git checkout -b default-base master &&
 	git checkout -b default topic &&
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 21:24 UTC · re: Denton Liu · lore

[PATCH v4 2/5] format-patch: fix indentation

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/log.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/builtin/log.c b/builtin/log.c
index a26f223ab4..9c44682f61 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1350,7 +1350,7 @@ static int header_callback(const struct option *opt, const char *arg, int unset)
 		string_list_clear(&extra_to, 0);
 		string_list_clear(&extra_cc, 0);
 	} else {
-	    add_header(arg);
+		add_header(arg);
 	}
 	return 0;
 }
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 21:25 UTC · re: Denton Liu · lore

[PATCH v4 3/5] t4014: use test_config()

Instead of manually unsetting the config after the test case is done, use test_config() to do it automatically. While we're at it, fix a typo in a test case name.

Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 t/t4014-format-patch.sh | 8 +++-----
 1 file changed, 3 insertions(+), 5 deletions(-)
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index 69267b16f0..c7cc643adf 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1939,10 +1939,9 @@ test_expect_success 'format-patch errors out when history involves criss-cross'
 	test_must_fail 	git format-patch --base=auto -1
 '
 
-test_expect_success 'format-patch format.useAutoBaseoption' '
-	test_when_finished "git config --unset format.useAutoBase" &&
+test_expect_success 'format-patch format.useAutoBase option' '
 	git checkout local &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse upstream >commit-id-base &&
@@ -1951,8 +1950,7 @@ test_expect_success 'format-patch format.useAutoBaseoption' '
 '
 
 test_expect_success 'format-patch --base overrides format.useAutoBase' '
-	test_when_finished "git config --unset format.useAutoBase" &&
-	git config format.useAutoBase true &&
+	test_config format.useAutoBase true &&
 	git format-patch --stdout --base=HEAD~1 -1 >patch &&
 	grep "^base-commit:" patch >actual &&
 	git rev-parse HEAD~1 >commit-id-base &&
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 21:25 UTC · re: Denton Liu · lore

[PATCH v4 4/5] format-patch: teach --no-base

If `format.useAutoBase = true`, there was no way to override this from the command-line. Teach the `--no-base` option in format-patch to override `format.useAutoBase`.

Helped-by: René Scharfe <l.s.r@web.de>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 Documentation/git-format-patch.txt | 5 +++--
 builtin/log.c                      | 7 +++++--
 t/t4014-format-patch.sh            | 6 ++++++
 3 files changed, 14 insertions(+), 4 deletions(-)
diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 00bdf9b125..0d4f8951bb 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -333,11 +333,12 @@ you can use `--suffix=-patch` to get `0001-description-of-my-change-patch`.
   Output an all-zero hash in each patch's From header instead
   of the hash of the commit.
 
---base=<commit>::
+--[no-]base[=<commit>]::
 	Record the base tree information to identify the state the
 	patch series applies to.  See the BASE TREE INFORMATION section
 	below for details. If <commit> is "auto", a base commit is
-	automatically chosen.
+	automatically chosen. The `--no-base` option overrides a
+	`format.useAutoBase` configuration.
 
 --root::
 	Treat the revision argument as a <revision range>, even if it
diff --git a/builtin/log.c b/builtin/log.c
index 9c44682f61..bf904e887f 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -1406,7 +1406,7 @@ static struct commit *get_base_commit(const char *base_commit,
 		base = lookup_commit_reference_by_name(base_commit);
 		if (!base)
 			die(_("unknown commit %s"), base_commit);
-	} else if ((base_commit && !strcmp(base_commit, "auto")) || base_auto) {
+	} else if ((base_commit && !strcmp(base_commit, "auto"))) {
 		struct branch *curr_branch = branch_get(NULL);
 		const char *upstream = branch_get_upstream(curr_branch, NULL);
 		if (upstream) {
@@ -1710,6 +1710,9 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	s_r_opt.def = "HEAD";
 	s_r_opt.revarg_opt = REVARG_COMMITTISH;
 
+	if (base_auto)
+		base_commit = "auto";
+
 	if (default_attach) {
 		rev.mime_boundary = default_attach;
 		rev.no_inline = 1;
@@ -1973,7 +1976,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 
 	memset(&bases, 0, sizeof(bases));
-	if (base_commit || base_auto) {
+	if (base_commit) {
 		struct commit *base = get_base_commit(base_commit, list, nr);
 		reset_revision_walk();
 		clear_object_flags(UNINTERESTING);
diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index c7cc643adf..a5b6302a1c 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -1958,6 +1958,12 @@ test_expect_success 'format-patch --base overrides format.useAutoBase' '
 	test_cmp expect actual
 '
 
+test_expect_success 'format-patch --no-base overrides format.useAutoBase' '
+	test_config format.useAutoBase true &&
+	git format-patch --stdout --no-base -1 >patch &&
+	! grep "^base-commit:" patch
+'
+
 test_expect_success 'format-patch --base with --attach' '
 	git format-patch --attach=mimemime --stdout --base=HEAD~ -1 >patch &&
 	sed -n -e "/^base-commit:/s/.*/1/p" -e "/^---*mimemime--$/s/.*/2/p" \
-- 
2.24.0.578.g4820254054
Denton Liu· Dec 4, 2019, 21:25 UTC · re: Denton Liu · lore

[PATCH v4 5/5] rebase: fix format.useAutoBase breakage

With `format.useAutoBase = true`, running rebase resulted in an error:

	fatal: failed to get upstream, if you want to record base commit automatically,
	please use git branch --set-upstream-to to track a remote branch.
	Or you could specify base commit by --base=<base-commit-id> manually
	error:
	git encountered an error while preparing the patches to replay
	these revisions:
	    ede2467cdedc63784887b587a61c36b7850ebfac..d8f581194799ae29bf5fa72a98cbae98a1198b12
	As a result, git cannot rebase them.

Fix this by always passing `--no-base` to format-patch from rebase so that the effect of `format.useAutoBase` is negated.

Reported-by: Christian Biesinger <cbiesinger@google.com>
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
 builtin/rebase.c  | 3 ++-
 t/t3400-rebase.sh | 2 +-
 2 files changed, 3 insertions(+), 2 deletions(-)
diff --git a/builtin/rebase.c b/builtin/rebase.c
index e755087b0f..51980ab63d 100644
--- a/builtin/rebase.c
+++ b/builtin/rebase.c
@@ -1012,7 +1012,8 @@ static int run_am(struct rebase_options *opts)
 	argv_array_pushl(&format_patch.args, "format-patch", "-k", "--stdout",
 			 "--full-index", "--cherry-pick", "--right-only",
 			 "--src-prefix=a/", "--dst-prefix=b/", "--no-renames",
-			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order", NULL);
+			 "--no-cover-letter", "--pretty=mboxrd", "--topo-order",
+			 "--no-base", NULL);
 	if (opts->git_format_patch_opt.len)
 		argv_array_split(&format_patch.args,
 				 opts->git_format_patch_opt.buf);
diff --git a/t/t3400-rebase.sh b/t/t3400-rebase.sh
index ca99e8c6c4..1323f30fee 100755
--- a/t/t3400-rebase.sh
+++ b/t/t3400-rebase.sh
@@ -159,7 +159,7 @@ test_expect_success 'fail when upstream arg is missing and not configured' '
 	test_must_fail git rebase
 '
 
-test_expect_failure 'rebase works with format.useAutoBase' '
+test_expect_success 'rebase works with format.useAutoBase' '
 	test_config format.useAutoBase true &&
 	git checkout topic &&
 	git rebase master
-- 
2.24.0.578.g4820254054

← back to recent threads