[PATCH 0/3] pull: obvious fixes

[PATCH 0/3] pull: obvious fixes

[Felipe Contreras]

These are obvious fixes that I sent many times in series like [1], but
for some reason they were never merged.

There's absolutely no reason not to merge these.

[1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/

Felipe Contreras (3):
  pull: cleanup autostash check
  pull: trivial cleanup
  pull: trivial whitespace style fix

 builtin/pull.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

-- 
2.32.0

[PATCH 1/3] pull: cleanup autostash check

[Felipe Contreras]

Currently "git pull --rebase" takes a shortcut in the case a
fast-forward merge is possible; run_merge() is called with --ff-only.

However, "git merge" didn't have an --autostash option, so, when "git
pull --rebase --autostash" was called *and* the fast-forward merge
shortcut was taken, then the pull failed.

This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash
works in dirty repo, 2017-06-01) by simply skipping the fast-forward
merge shortcut.

Later on "git merge" learned the --autostash option [a03b55530a
(merge: teach --autostash option, 2020-04-07)], and so did "git pull"
[d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].

Therefore it's not necessary to skip the fast-forward merge shortcut
anymore when called with --rebase --autostash.

Let's always take the fast-forward merge shortcut by essentially
reverting f15e7cf5cc.

Reviewed-by: Elijah Newren <newren@gmail.com>
Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index e8927fc2ff..a22293b7db 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct oid_array merge_heads = OID_ARRAY_INIT;
 	struct object_id orig_head, curr_head;
 	struct object_id rebase_fork_point;
-	int autostash;
 	int rebase_unspecified = 0;
 	int can_ff;
 
@@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_oid("HEAD", &orig_head))
 		oidclr(&orig_head);
 
-	autostash = config_autostash;
 	if (opt_rebase) {
+		int autostash = config_autostash;
 		if (opt_autostash != -1)
 			autostash = opt_autostash;
 
@@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
-		if (!autostash) {
-			if (can_ff) {
-				/* we can fast-forward this without invoking rebase */
-				opt_ff = "--ff-only";
-				ran_ff = 1;
-				ret = run_merge();
-			}
+
+		if (can_ff) {
+			/* we can fast-forward this without invoking rebase */
+			opt_ff = "--ff-only";
+			ran_ff = 1;
+			ret = run_merge();
 		}
 		if (!ran_ff)
 			ret = run_rebase(&newbase, &upstream);
-- 
2.32.0

Re: [PATCH 1/3] pull: cleanup autostash check

[Elijah Newren]

On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Currently "git pull --rebase" takes a shortcut in the case a
> fast-forward merge is possible; run_merge() is called with --ff-only.
>
> However, "git merge" didn't have an --autostash option, so, when "git
> pull --rebase --autostash" was called *and* the fast-forward merge
> shortcut was taken, then the pull failed.
>
> This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash
> works in dirty repo, 2017-06-01) by simply skipping the fast-forward
> merge shortcut.
>
> Later on "git merge" learned the --autostash option [a03b55530a
> (merge: teach --autostash option, 2020-04-07)], and so did "git pull"
> [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].
>
> Therefore it's not necessary to skip the fast-forward merge shortcut
> anymore when called with --rebase --autostash.
>
> Let's always take the fast-forward merge shortcut by essentially
> reverting f15e7cf5cc.
>
> Reviewed-by: Elijah Newren <newren@gmail.com>

I think you are basing the Reviewed-by on
https://lore.kernel.org/git/CABPp-BEsQWsHMAmwc3gmJnXcS+aR-FtoMJxBRQ=BpARP49-L-Q@mail.gmail.com/;
is that correct?  Messages from folks that they seem to like the patch
or believe it looks good should be translated into an Acked-by rather
than a Reviewed-by; from Documentation/SubmittingPatches:

* `Reviewed-by:`, unlike the other tags, can only be offered by the
  reviewer and means that she is completely satisfied that the patch
  is ready for application.  It is usually offered only after a
  detailed review.

Sorry for not catching this when you posted v3 & v4 of your earlier
series.  When your series exploded in size and seemed to just be
accumulating additional changes you wanted to make in the area that
weren't in response to reviewer feedback (I wasn't sure why the new
patches in subsequent rerolls weren't just separate series), I didn't
have the bandwidth to keep up and review them, so I just missed it.

> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 16 +++++++---------
>  1 file changed, 7 insertions(+), 9 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index e8927fc2ff..a22293b7db 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         struct oid_array merge_heads = OID_ARRAY_INIT;
>         struct object_id orig_head, curr_head;
>         struct object_id rebase_fork_point;
> -       int autostash;
>         int rebase_unspecified = 0;
>         int can_ff;
>
> @@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>         if (get_oid("HEAD", &orig_head))
>                 oidclr(&orig_head);
>
> -       autostash = config_autostash;
>         if (opt_rebase) {
> +               int autostash = config_autostash;
>                 if (opt_autostash != -1)
>                         autostash = opt_autostash;
>
> @@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                      recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
>                     submodule_touches_in_range(the_repository, &upstream, &curr_head))
>                         die(_("cannot rebase with locally recorded submodule modifications"));
> -               if (!autostash) {
> -                       if (can_ff) {
> -                               /* we can fast-forward this without invoking rebase */
> -                               opt_ff = "--ff-only";
> -                               ran_ff = 1;
> -                               ret = run_merge();
> -                       }
> +
> +               if (can_ff) {
> +                       /* we can fast-forward this without invoking rebase */
> +                       opt_ff = "--ff-only";
> +                       ran_ff = 1;
> +                       ret = run_merge();
>                 }
>                 if (!ran_ff)
>                         ret = run_rebase(&newbase, &upstream);
> --
> 2.32.0

Re: [PATCH 1/3] pull: cleanup autostash check

[Felipe Contreras]

Elijah Newren wrote:
> On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > Currently "git pull --rebase" takes a shortcut in the case a
> > fast-forward merge is possible; run_merge() is called with --ff-only.
> >
> > However, "git merge" didn't have an --autostash option, so, when "git
> > pull --rebase --autostash" was called *and* the fast-forward merge
> > shortcut was taken, then the pull failed.
> >
> > This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash
> > works in dirty repo, 2017-06-01) by simply skipping the fast-forward
> > merge shortcut.
> >
> > Later on "git merge" learned the --autostash option [a03b55530a
> > (merge: teach --autostash option, 2020-04-07)], and so did "git pull"
> > [d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].
> >
> > Therefore it's not necessary to skip the fast-forward merge shortcut
> > anymore when called with --rebase --autostash.
> >
> > Let's always take the fast-forward merge shortcut by essentially
> > reverting f15e7cf5cc.
> >
> > Reviewed-by: Elijah Newren <newren@gmail.com>
> 
> I think you are basing the Reviewed-by on
> https://lore.kernel.org/git/CABPp-BEsQWsHMAmwc3gmJnXcS+aR-FtoMJxBRQ=BpARP49-L-Q@mail.gmail.com/;
> is that correct?

No, more like:

[1] https://lore.kernel.org/git/20201205195313.1557473-5-felipe.contreras@gmail.com/

> Messages from folks that they seem to like the patch
> or believe it looks good should be translated into an Acked-by rather
> than a Reviewed-by; from Documentation/SubmittingPatches:

To me an acknowledgment means something entirely different, and must be
expressly given.

> * `Reviewed-by:`, unlike the other tags, can only be offered by the
>   reviewer and means that she is completely satisfied that the patch
>   is ready for application.  It is usually offered only after a
>   detailed review.

Yeah, I read that after I sent v3. In this series I simply cherry-picked
it from a previous series.

I guess I'll just avoid both.

-- 
Felipe Contreras

Re: [PATCH 1/3] pull: cleanup autostash check

[Felipe Contreras]

Carlo Arenas wrote:
> On Sat, Jun 12, 2021 at 10:11 PM Felipe Contreras <
> felipe.contreras@gmail.com> wrote:
> 
> > -       autostash = config_autostash;
> >         if (opt_rebase) {
> > +               int autostash = config_autostash;
> >                 if (opt_autostash != -1)
> >                         autostash = opt_autostash;
> >
> 
> since you are reducing the scope of the autostash variable anyway, why
> not refactor it additionally for clarity with (something like):
> 
>   int autostash = (opt_autostash != -1) ? opt_autostash : config_autostash;

Because I had like 15 branches on top of this, and wanted 1) to minimize
modifications, and 2) to minimize the chance of the patch being
rejected, and this is how the code was before f15e7cf5cc (pull: ff
--rebase --autostash works in dirty repo, 2017-06-01), so in theory
nobody could object.

But yeah, that version makes sense, and in fact I probably had such
cleanup in some branch.

-- 
Felipe Contreras

[PATCH 2/3] pull: trivial cleanup

[Felipe Contreras]

There's no need to store ran_ff. Now it's obvious from the conditionals.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a22293b7db..80e2f55cbc 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1053,7 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	if (opt_rebase) {
 		int ret = 0;
-		int ran_ff = 0;
 
 		struct object_id newbase;
 		struct object_id upstream;
@@ -1068,11 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (can_ff) {
 			/* we can fast-forward this without invoking rebase */
 			opt_ff = "--ff-only";
-			ran_ff = 1;
 			ret = run_merge();
-		}
-		if (!ran_ff)
+		} else {
 			ret = run_rebase(&newbase, &upstream);
+		}
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
-- 
2.32.0

Re: [PATCH 2/3] pull: trivial cleanup

[Elijah Newren]

On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> There's no need to store ran_ff. Now it's obvious from the conditionals.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 6 ++----
>  1 file changed, 2 insertions(+), 4 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index a22293b7db..80e2f55cbc 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -1053,7 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>
>         if (opt_rebase) {
>                 int ret = 0;
> -               int ran_ff = 0;
>
>                 struct object_id newbase;
>                 struct object_id upstream;
> @@ -1068,11 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
>                 if (can_ff) {
>                         /* we can fast-forward this without invoking rebase */
>                         opt_ff = "--ff-only";
> -                       ran_ff = 1;
>                         ret = run_merge();
> -               }
> -               if (!ran_ff)
> +               } else {
>                         ret = run_rebase(&newbase, &upstream);
> +               }
>
>                 if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
>                              recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
> --
> 2.32.0

Makes sense.

[PATCH 3/3] pull: trivial whitespace style fix

[Felipe Contreras]

Two spaces unaligned to anything is not part of the coding-style. A
single tab is.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 80e2f55cbc..3e13f81084 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -126,9 +126,9 @@ static struct option pull_options[] = {
 	/* Options passed to git-merge or git-rebase */
 	OPT_GROUP(N_("Options related to merging")),
 	OPT_CALLBACK_F('r', "rebase", &opt_rebase,
-	  "(false|true|merges|preserve|interactive)",
-	  N_("incorporate changes by rebasing rather than merging"),
-	  PARSE_OPT_OPTARG, parse_opt_rebase),
+		"(false|true|merges|preserve|interactive)",
+		N_("incorporate changes by rebasing rather than merging"),
+		PARSE_OPT_OPTARG, parse_opt_rebase),
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
-- 
2.32.0

Re: [PATCH 3/3] pull: trivial whitespace style fix

[Elijah Newren]

On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> Two spaces unaligned to anything is not part of the coding-style. A
> single tab is.
>
> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
> ---
>  builtin/pull.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/pull.c b/builtin/pull.c
> index 80e2f55cbc..3e13f81084 100644
> --- a/builtin/pull.c
> +++ b/builtin/pull.c
> @@ -126,9 +126,9 @@ static struct option pull_options[] = {
>         /* Options passed to git-merge or git-rebase */
>         OPT_GROUP(N_("Options related to merging")),
>         OPT_CALLBACK_F('r', "rebase", &opt_rebase,
> -         "(false|true|merges|preserve|interactive)",
> -         N_("incorporate changes by rebasing rather than merging"),
> -         PARSE_OPT_OPTARG, parse_opt_rebase),
> +               "(false|true|merges|preserve|interactive)",
> +               N_("incorporate changes by rebasing rather than merging"),
> +               PARSE_OPT_OPTARG, parse_opt_rebase),
>         OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
>                 N_("do not show a diffstat at the end of the merge"),
>                 PARSE_OPT_NOARG | PARSE_OPT_NONEG),
> --
> 2.32.0

Not only does this change bring this code in alignment with the coding
style, it also makes it more consistent with the other code around it.
None of the other options parsing in this file used a
tab-and-two-space indent, so it's curious why this one was added this
way.  Anyway, thanks for fixing.

Re: [PATCH 0/3] pull: obvious fixes

[Elijah Newren]

On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> These are obvious fixes that I sent many times in series like [1], but
> for some reason they were never merged.
>
> There's absolutely no reason not to merge these.
>
> [1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/

I was really surprised to see the Reviewed-by on patch 1, and did not
remember what review I had done.  Unfortunately, since your new patch
series aren't posted as responses to old ones (see
https://lore.kernel.org/git/CABPp-BEEiPP7AEk4Wexw4_MDHcin2n8xkMowO=OXTn9pNPaG0A@mail.gmail.com/T/#u
for an example of what I mean), and since the cover letter you linked
to didn't reference previous series, there's no trace of where it came
from.  I had to go digging to try to find it.  Any chance you could
tweak your posts in the future to help reviewers follow how things
have evolved?

> Felipe Contreras (3):
>   pull: cleanup autostash check
>   pull: trivial cleanup
>   pull: trivial whitespace style fix
>
>  builtin/pull.c | 26 +++++++++++---------------
>  1 file changed, 11 insertions(+), 15 deletions(-)
>
> --
> 2.32.0

Re: [PATCH 0/3] pull: obvious fixes

[Junio C Hamano]

Elijah Newren <newren@gmail.com> writes:

> I was really surprised to see the Reviewed-by on patch 1, and did not
> remember what review I had done.  Unfortunately, ...
> from.  I had to go digging to try to find it.  Any chance you could
> tweak your posts in the future to help reviewers follow how things
> have evolved?

Sounds like a sensible request.

Aside from that, I read the three patches and think they are good.
So I'll queue them as-is on 'seen' (unless they crash with something
else, which is always the case without being said, but it is safer
to be explicit) but won't merge it down until the above is resolved.

One way to resolve it is for you to review this round and give a
refreshed Reviewed-by ;-)

Thanks, both.

Re: [PATCH 0/3] pull: obvious fixes

[Felipe Contreras]

Elijah Newren wrote:
> On Sat, Jun 12, 2021 at 9:59 PM Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> >
> > These are obvious fixes that I sent many times in series like [1], but
> > for some reason they were never merged.
> >
> > There's absolutely no reason not to merge these.
> >
> > [1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/
> 
> I was really surprised to see the Reviewed-by on patch 1, and did not
> remember what review I had done.  Unfortunately, since your new patch
> series aren't posted as responses to old ones (see
> https://lore.kernel.org/git/CABPp-BEEiPP7AEk4Wexw4_MDHcin2n8xkMowO=OXTn9pNPaG0A@mail.gmail.com/T/#u
> for an example of what I mean), and since the cover letter you linked
> to didn't reference previous series,

But my cover letter did reference a previous series:

  https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/

See patch 3, 4, and 5.

The problem is that these patches (along with many others) were part of
different series, that I reordered, split, and joined in order to make it
clear why all of them were needed. When I split them people didn't
understand the context, and when I joined them, suddenly there were too
many.

> there's no trace of where it came from.  I had to go digging to try to
> find it.  Any chance you could tweak your posts in the future to help
> reviewers follow how things have evolved?

I always do that, including this series.

In order to properly dig through all the versions of these particualr 3
patches it would probably take me an hour, and I don't know how much
value that would provide. I just picked the latest one I could find that
contained them.

-- 
Felipe Contreras

[PATCH v2 0/3] pull: obvious fixes

[Felipe Contreras]

These are obvious fixes that I sent many times in series like [1], but
for some reason they were never merged.

No changes since v1, except the removal of a reviewed-by trailer that
was not expressly given.

[1] https://lore.kernel.org/git/20201218211026.1937168-1-felipe.contreras@gmail.com/

Felipe Contreras (3):
  pull: cleanup autostash check
  pull: trivial cleanup
  pull: trivial whitespace style fix

 builtin/pull.c | 26 +++++++++++---------------
 1 file changed, 11 insertions(+), 15 deletions(-)

Range-diff against v1:
1:  f9520dbf78 ! 1:  bc5d3227a9 pull: cleanup autostash check
    @@ Commit message
         Let's always take the fast-forward merge shortcut by essentially
         reverting f15e7cf5cc.
     
    -    Reviewed-by: Elijah Newren <newren@gmail.com>
         Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
     
      ## builtin/pull.c ##
2:  e677256db0 = 2:  d3c944d2fd pull: trivial cleanup
3:  34a9e2d50f = 3:  aadc7e17dc pull: trivial whitespace style fix
-- 
2.32.0

[PATCH v2 1/3] pull: cleanup autostash check

[Felipe Contreras]

Currently "git pull --rebase" takes a shortcut in the case a
fast-forward merge is possible; run_merge() is called with --ff-only.

However, "git merge" didn't have an --autostash option, so, when "git
pull --rebase --autostash" was called *and* the fast-forward merge
shortcut was taken, then the pull failed.

This was fixed in commit f15e7cf5cc (pull: ff --rebase --autostash
works in dirty repo, 2017-06-01) by simply skipping the fast-forward
merge shortcut.

Later on "git merge" learned the --autostash option [a03b55530a
(merge: teach --autostash option, 2020-04-07)], and so did "git pull"
[d9f15d37f1 (pull: pass --autostash to merge, 2020-04-07)].

Therefore it's not necessary to skip the fast-forward merge shortcut
anymore when called with --rebase --autostash.

Let's always take the fast-forward merge shortcut by essentially
reverting f15e7cf5cc.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 16 +++++++---------
 1 file changed, 7 insertions(+), 9 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index e8927fc2ff..a22293b7db 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -947,7 +947,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	struct oid_array merge_heads = OID_ARRAY_INIT;
 	struct object_id orig_head, curr_head;
 	struct object_id rebase_fork_point;
-	int autostash;
 	int rebase_unspecified = 0;
 	int can_ff;
 
@@ -982,8 +981,8 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 	if (get_oid("HEAD", &orig_head))
 		oidclr(&orig_head);
 
-	autostash = config_autostash;
 	if (opt_rebase) {
+		int autostash = config_autostash;
 		if (opt_autostash != -1)
 			autostash = opt_autostash;
 
@@ -1065,13 +1064,12 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND) &&
 		    submodule_touches_in_range(the_repository, &upstream, &curr_head))
 			die(_("cannot rebase with locally recorded submodule modifications"));
-		if (!autostash) {
-			if (can_ff) {
-				/* we can fast-forward this without invoking rebase */
-				opt_ff = "--ff-only";
-				ran_ff = 1;
-				ret = run_merge();
-			}
+
+		if (can_ff) {
+			/* we can fast-forward this without invoking rebase */
+			opt_ff = "--ff-only";
+			ran_ff = 1;
+			ret = run_merge();
 		}
 		if (!ran_ff)
 			ret = run_rebase(&newbase, &upstream);
-- 
2.32.0

[PATCH v2 2/3] pull: trivial cleanup

[Felipe Contreras]

There's no need to store ran_ff. Now it's obvious from the conditionals.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 6 ++----
 1 file changed, 2 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index a22293b7db..80e2f55cbc 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -1053,7 +1053,6 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 
 	if (opt_rebase) {
 		int ret = 0;
-		int ran_ff = 0;
 
 		struct object_id newbase;
 		struct object_id upstream;
@@ -1068,11 +1067,10 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (can_ff) {
 			/* we can fast-forward this without invoking rebase */
 			opt_ff = "--ff-only";
-			ran_ff = 1;
 			ret = run_merge();
-		}
-		if (!ran_ff)
+		} else {
 			ret = run_rebase(&newbase, &upstream);
+		}
 
 		if (!ret && (recurse_submodules == RECURSE_SUBMODULES_ON ||
 			     recurse_submodules == RECURSE_SUBMODULES_ON_DEMAND))
-- 
2.32.0

[PATCH v2 3/3] pull: trivial whitespace style fix

[Felipe Contreras]

Two spaces unaligned to anything is not part of the coding-style. A
single tab is.

Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
---
 builtin/pull.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index 80e2f55cbc..3e13f81084 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -126,9 +126,9 @@ static struct option pull_options[] = {
 	/* Options passed to git-merge or git-rebase */
 	OPT_GROUP(N_("Options related to merging")),
 	OPT_CALLBACK_F('r', "rebase", &opt_rebase,
-	  "(false|true|merges|preserve|interactive)",
-	  N_("incorporate changes by rebasing rather than merging"),
-	  PARSE_OPT_OPTARG, parse_opt_rebase),
+		"(false|true|merges|preserve|interactive)",
+		N_("incorporate changes by rebasing rather than merging"),
+		PARSE_OPT_OPTARG, parse_opt_rebase),
 	OPT_PASSTHRU('n', NULL, &opt_diffstat, NULL,
 		N_("do not show a diffstat at the end of the merge"),
 		PARSE_OPT_NOARG | PARSE_OPT_NONEG),
-- 
2.32.0

Re: [PATCH v2 0/3] pull: obvious fixes

[Elijah Newren]

On Thu, Jun 17, 2021 at 9:17 AM Felipe Contreras
<felipe.contreras@gmail.com> wrote:
>
> These are obvious fixes that I sent many times in series like [1], but
> for some reason they were never merged.
>
> No changes since v1, except the removal of a reviewed-by trailer that
> was not expressly given.

Thanks for correcting this.