Re: [PATCH 3/3] git-am: add am.threeWay config variable
Hi,
On Wed, May 27, 2015 at 5:32 AM, Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr> wrote:
Show 10 quoted lines
> Add the am.threeWay configuration variable to use the -3 or --3way
> option of git am by default. When am.threeway is set and not desired
> for a specific git am command, the --no-3way option can be used to
> override it.
>
> Signed-off-by: Remi Lespinet <remi.lespinet@ensimag.grenoble-inp.fr>
> ---
> Even if git am will be rewritten soon, the code that will have to be
> ported is not the most important part of the patch and the tests and
> documentation parts can be reused.
Yup, there's no problem on my side.
Show 21 quoted lines
> Documentation/config.txt | 7 +++++++
> Documentation/git-am.txt | 6 ++++--
> git-am.sh | 7 +++++++
> t/t4150-am.sh | 15 +++++++++++++++
> 4 files changed, 33 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index d44bc85..8e42752 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -769,6 +769,13 @@ am.keepcr::
> by giving '--no-keep-cr' from the command line.
> See linkgit:git-am[1], linkgit:git-mailsplit[1].
>
> +am.threeWay::
> + If true, git-am will fall back on 3-way merge when the patch
> + cannot be applied cleanly, in the same way as the '-3' or
> + '--3-way' option. Can be overridden by giving '--no-3-way'
> + from the command line.
> + See linkgit:git-am[1].
> +
Maybe we should start by mentioning the default behavior for git-am. Something like,
By default, git-am will fail if the patch does not apply cleanly. When set to true, this setting tells git-am to fall back on 3-way merge if the patch records the identity of blobs it is supposed to apply to and we have those blobs available locally (equivalent to giving the --3way option from the command line).
(I stole most of the text from the git-am and git-config documentation)
Show 13 quoted lines
> apply.ignoreWhitespace::
> When set to 'change', tells 'git apply' to ignore changes in
> whitespace, in the same way as the '--ignore-space-change'
> diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
> index 0d8ba48..3190c05 100644
> --- a/Documentation/git-am.txt
> +++ b/Documentation/git-am.txt
> @@ -89,11 +89,13 @@ default. You can use `--no-utf8` to override this.
> linkgit:git-mailinfo[1]).
>
> -3::
> ---3way::
> +--[no-]3way::
There's no need to mention --no-3way, it's assumed that most of the options have the equivalent --no-option because it is implemented automatically by the option parser.
Show 7 quoted lines
> When the patch does not apply cleanly, fall back on
> 3-way merge if the patch records the identity of blobs
> it is supposed to apply to and we have those blobs
> - available locally.
> + available locally. `am.threeWay` configuration variable
> + can be used to specify the default behaviour. `--no-3way`
> + is useful to override `am.threeWay`.
Usually configuration settings are mentioned in a separate section in the documentation "CONFIGURATION" (or not mentioned at all). Also, there's no need to mention that --no-3way can be used to mention the configuration, as its usual (and expected) that the configuration value sets the default behavior, and the command-line switch can override i.
Show 24 quoted lines
> --ignore-space-change::
> --ignore-whitespace::
> diff --git a/git-am.sh b/git-am.sh
> index 761befb..781507c 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -389,6 +389,11 @@ then
> keepcr=t
> fi
>
> +if test "$(git config --bool --get am.threeWay)" = true
> +then
> + threeway=t
> +fi
> +
> while test $# != 0
> do
> case "$1" in
> @@ -400,6 +405,8 @@ it will be removed. Please do not use it anymore."
> ;;
> -3|--3way)
> threeway=t ;;
> + --no-3way)
> + threeway=f ;;
Okay.
This patch on its own, though, does not handle the case where:
1. am.threeWay is set to "true"
2. we passed "--no-3way" in the command-line
3. git-am failed to apply a patch and quit
4. We git-am --skip or git-am --continue. When continuing, git-am will
not remember the --no-3way (unless we provide it again on the command
line)
They key is to tweak the following lines where the "threeway" setting is loaded:
if test "$(cat "$dotest/threeway")" = t
then
threeway=t
fiTo something like
if test "$(cat "$dotest/threeway")" = t
then
threeway=t
else
threeway=f
fiShow 29 quoted lines
> -s|--signoff)
> sign=t ;;
> -u|--utf8)
> diff --git a/t/t4150-am.sh b/t/t4150-am.sh
> index 8f85098..e16ef0e 100755
> --- a/t/t4150-am.sh
> +++ b/t/t4150-am.sh
> @@ -288,6 +288,21 @@ test_expect_success 'am -3 falls back to 3-way merge' '
> git diff --exit-code lorem
> '
>
> +test_expect_success 'am with config am.threeWay falls back to 3-way merge' '
> + setup_temporary_branch lorem2 &&
> + test_config am.threeWay 1 &&
> + git am lorem-move.patch &&
> + test_path_is_missing .git/rebase-apply &&
> + git diff --exit-code lorem
> +'
> +
> +test_expect_success 'am with config am.threeWay overridden by --no-3way' '
> + setup_temporary_branch lorem2 &&
> + test_config am.threeWay 1 &&
> + test_must_fail git am --no-3way lorem-move.patch &&
> + test_path_is_dir .git/rebase-apply
> +'
> +
> test_expect_success 'am -3 -p0 can read --no-prefix patch' '
> setup_temporary_branch lorem2 &&
> git am -3 -p0 lorem-zero.patch &&
> --
> 1.9.1
To end off, some off-tangent issues that are not related to the patch series in question, but since I'm looking at git-am.sh....
I've noticed that in the block above that initializes all the variables,
sign= utf8=t keep= keepcr= skip= interactive= resolved= rebasing= abort=
messageid= resolvemsg= resume= scissors= no_inbody_headers=
git_apply_opt=
committer_date_is_author_date=
ignore_date=
allow_rerere_autoupdate=
gpg_sign_opt=threeway is not initialized at all, and thus I think running "threeway=t git am blah" will affect the behavior of git-am.
Also, I noticed that we do not check for --no-interactive, --no-signoff, --no-keep, --no-whitespace, etc.
Thanks, Paul
Remi Lespinet
