Re: [PATCH 3/3] git-am: add am.threeWay config variable

From: Paul Tan <pyokagan@gmail.com>
Date: May 28, 2015, 13:47 UTC
Message-ID: <CACRoPnSnwA5GcYZjHHTepz4WzXYuJ+9nJqGsL92r_vnsNrN_-w@mail.gmail.com>
In-Reply-To: <1432675975-11020-3-git-send-email-remi.lespinet@ensimag.grenoble-inp.fr>

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
    fi

To something like

    if test "$(cat "$dotest/threeway")" = t
    then
        threeway=t
    else
        threeway=f
    fi

Show 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 &&

This looks good.

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