On Mon, May 18, 2015 at 8:06 AM, Paul Tan <pyokagan@xxxxxxxxx> wrote:
> Since cd67e4d (Teach 'git pull' about --rebase, 2007-11-28),
> fetch+rebase could be set by default by defining the config variable
> branch.<name>.rebase. This setting can be overriden on the command line
> by --rebase and --no-rebase.
>
> Since 6b37dff (pull: introduce a pull.rebase option to enable --rebase,
> 2011-11-06), git-pull --rebase can also be configured via the
> pull.rebase configuration option.
>
> Re-implement support for these two configuration settings by introducing
> config_get_rebase() which is called before parse_options() to set the
> default value of opt_rebase.
>
> Signed-off-by: Paul Tan <pyokagan@xxxxxxxxx>
> ---
> builtin/pull.c | 35 +++++++++++++++++++++++++++++++++++
> t/t5520-pull.sh | 12 ++++++------
> 2 files changed, 41 insertions(+), 6 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index f18a21c..a0958a7 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -294,6 +294,39 @@ static const char *config_get_ff(void)
> die(_("Invalid value for pull.ff: %s"), value);
> }
>
> +/**
> + * Returns the default configured value for --rebase. It first looks for the
> + * value of "branch.$curr_branch.rebase", where $curr_branch is the current
> + * branch, and if HEAD is detached or the configuration key does not exist,
> + * looks for the value of "pull.rebase". If both configuration keys do not
> + * exist, returns REBASE_FALSE.
> + */
> +static int config_get_rebase(void)
> +{
> + struct strbuf sb = STRBUF_INIT;
> + struct branch *curr_branch = branch_get("HEAD");
> + const char *key, *str;
> + int ret = -1, value;
> +
> + if (curr_branch) {
> + strbuf_addf(&sb, "branch.%s.rebase", curr_branch->name);
> + key = sb.buf;
> + ret = git_config_get_value(sb.buf, &str);
> + }
> + if (ret) {
> + key = "pull.rebase";
> + ret = git_config_get_value(key, &str);
> + }
> + if (ret) {
> + strbuf_release(&sb);
> + return REBASE_FALSE;
> + }
> + if ((value = parse_config_rebase(str)) < 0)
Assignment inside an if (....).
I just looked up the Documentation/CodingGuidelines
which state it as:
- We try to avoid assignments inside "if ()" conditions.
so my hint from my previous mail (to also apply it to for/while)
is wrong.
Initially the flow confused me a little, so I tried to understand why,
and I guess it's because of the return value used both as a return value
and as conditional.
Maybe you could move the check of the value out of this function
and here you only focus on getting the order right?
Something like (completely untested code, beware):
enum rebase_type config_get_rebase(void)
{
struct strbuf sb = STRBUF_INIT;
enum rebase_type value;
struct branch *curr_branch = branch_get("HEAD");
if (curr_branch) {
strbuf_addf(&sb, "branch.%s.rebase", curr_branch->name);
if (parse_config_rebase_or_die_on_unparsable(sb.buf, &value))
return value;
}
if (parse_config_rebase_or_die_on_unparsable("pull.rebase", &value))
return value;
return REBASE_FALSE;
}
This implementation leaks the strbuf though.
> + die(_("Invalid value for %s: %s"), key, str);
> + strbuf_release(&sb);
> + return value;
> +}
> +
> struct known_remote {
> struct known_remote *next;
> struct remote *remote;
> @@ -730,6 +763,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
> if (!getenv("GIT_REFLOG_ACTION"))
> set_reflog_message(argc, argv);
>
> + opt_rebase = config_get_rebase();
> +
> argc = parse_options(argc, argv, prefix, pull_options, pull_usage, 0);
> parse_repo_refspecs(argc, argv, &repo, &refspecs);
>
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index 3798b96..17254ee 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -234,7 +234,7 @@ test_expect_success '--rebase fails with multiple branches' '
> test modified = "$(git show HEAD:file)"
> '
>
> -test_expect_failure 'pull.rebase' '
> +test_expect_success 'pull.rebase' '
> git reset --hard before-rebase &&
> test_config pull.rebase true &&
> git pull . copy &&
> @@ -242,7 +242,7 @@ test_expect_failure 'pull.rebase' '
> test new = "$(git show HEAD:file2)"
> '
>
> -test_expect_failure 'branch.to-rebase.rebase' '
> +test_expect_success 'branch.to-rebase.rebase' '
> git reset --hard before-rebase &&
> test_config branch.to-rebase.rebase true &&
> git pull . copy &&
> @@ -280,7 +280,7 @@ test_expect_success 'pull.rebase=false create a new merge commit' '
> test file3 = "$(git show HEAD:file3.t)"
> '
>
> -test_expect_failure 'pull.rebase=true flattens keep-merge' '
> +test_expect_success 'pull.rebase=true flattens keep-merge' '
> git reset --hard before-preserve-rebase &&
> test_config pull.rebase true &&
> git pull . copy &&
> @@ -288,7 +288,7 @@ test_expect_failure 'pull.rebase=true flattens keep-merge' '
> test file3 = "$(git show HEAD:file3.t)"
> '
>
> -test_expect_failure 'pull.rebase=1 is treated as true and flattens keep-merge' '
> +test_expect_success 'pull.rebase=1 is treated as true and flattens keep-merge' '
> git reset --hard before-preserve-rebase &&
> test_config pull.rebase 1 &&
> git pull . copy &&
> @@ -296,7 +296,7 @@ test_expect_failure 'pull.rebase=1 is treated as true and flattens keep-merge' '
> test file3 = "$(git show HEAD:file3.t)"
> '
>
> -test_expect_failure 'pull.rebase=preserve rebases and merges keep-merge' '
> +test_expect_success 'pull.rebase=preserve rebases and merges keep-merge' '
> git reset --hard before-preserve-rebase &&
> test_config pull.rebase preserve &&
> git pull . copy &&
> @@ -304,7 +304,7 @@ test_expect_failure 'pull.rebase=preserve rebases and merges keep-merge' '
> test "$(git rev-parse HEAD^2)" = "$(git rev-parse keep-merge)"
> '
>
> -test_expect_failure 'pull.rebase=invalid fails' '
> +test_expect_success 'pull.rebase=invalid fails' '
> git reset --hard before-preserve-rebase &&
> test_config pull.rebase invalid &&
> ! git pull . copy
> --
> 2.1.4
>
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html