Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Dakota Hawkins]

Hello,

I have a question which may be a bug (I'm a bit skeptical), but here goes:

In my global .gitconfig, I have "user.useConfigOnly = true" and
user.email isn't set there (I prefer to be forced to set it on a
per-repo basis, as I use different emails for work and personal
repos). I ALSO have "pull.rebase = preserve" set.

An example of the problem I have is with tools like golang (I filed an
issue there, they closed it and suggested the problem is with git or
my config: https://github.com/golang/go/issues/16516#issuecomment-235800085)
that use git to pull in package repos without any real user
interaction. When something like that runs a git pull for me (to
update a package repo) my global config makes it try to rebase, which
fails because git doesn't know who I am.

With golang, at least, there's no help setting-up all of its cloned
repos with my user information in local configs, it's a manual step
and I have to repeat it anytime anything decided to clone a new repo
to use as a package, so it's at least persistently frustrating. Hence
the golang bug, which I think is still a bug because they should
handle this better.

In those cases specifically, I never have local commits that differ
from the remote, so a "pull --ff-only" should leave me in the same
state as a "pull --rebase".

Is this a case of rebase trying to make sure it has enough information
for me to be a committer before knowing whether I even need to rewrite
any commits, and could/should that be avoided? Alternatively (or also)
could/should rebase detect that a fast-forward is possible and prefer
to do that instead?

I would not be surprised to learn that there's something I don't know
or haven't considered that explains why those things aren't really
do-able, but if that's the case I'd be interested to know what they
are (I'd love to go back to the golang people and tell them to re-open
my bug because there's nothing wrong with git).

Thanks for your time, and please let me know if you'd like any
additional information.

- Dakota Hawkins

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Dakota Hawkins <dakotahawkins@gmail.com> writes:

> I have a question which may be a bug (I'm a bit skeptical), but here goes:
>
> In my global .gitconfig, I have "user.useConfigOnly = true" and
> user.email isn't set there (I prefer to be forced to set it on a
> per-repo basis, as I use different emails for work and personal
> repos). I ALSO have "pull.rebase = preserve" set.
>
> An example of the problem I have is with tools like golang (I filed an
> issue there, they closed it and suggested the problem is with git or
> my config: https://github.com/golang/go/issues/16516#issuecomment-235800085)
> that use git to pull in package repos without any real user
> interaction. When something like that runs a git pull for me (to
> update a package repo) my global config makes it try to rebase, which
> fails because git doesn't know who I am.

It's an interesting chicken-and-egg problem that user.useConfigOnly
introduces.  It seems that the design of that configuration variable
is not perfect and has room for improvement.

> In those cases specifically, I never have local commits that differ
> from the remote, so a "pull --ff-only" should leave me in the same
> state as a "pull --rebase".
>
> Is this a case of rebase trying to make sure it has enough information
> for me to be a committer before knowing whether I even need to rewrite
> any commits, and could/should that be avoided?  Alternatively (or also)
> could/should rebase detect that a fast-forward is possible and prefer
> to do that instead?

I think that is a reasonable argument, but to solve this for a more
general case, shouldn't we be discussing a solution that would also
work when rebase _does_ need to create a new commit?  And when the
latter is solved, I would imagine that "this rebase happens to be
fast-forward, and not having an ident shouldn't be an issue for this
special case" would become moot.

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Jeff King]

On Fri, Jul 29, 2016 at 10:47:27AM -0700, Junio C Hamano wrote:

> > In those cases specifically, I never have local commits that differ
> > from the remote, so a "pull --ff-only" should leave me in the same
> > state as a "pull --rebase".
> >
> > Is this a case of rebase trying to make sure it has enough information
> > for me to be a committer before knowing whether I even need to rewrite
> > any commits, and could/should that be avoided?  Alternatively (or also)
> > could/should rebase detect that a fast-forward is possible and prefer
> > to do that instead?
> 
> I think that is a reasonable argument, but to solve this for a more
> general case, shouldn't we be discussing a solution that would also
> work when rebase _does_ need to create a new commit?  And when the
> latter is solved, I would imagine that "this rebase happens to be
> fast-forward, and not having an ident shouldn't be an issue for this
> special case" would become moot.

Wouldn't it be wrong to create a commit with non-config ident when
user.useConfigOnly is set, though? That is the exact point when it
should kick in, to tell the user "you thought it would not matter here,
but in this case we _do_ need your real ident; what should we do?"

If the user is doing a one-off thing where they do not care if their
crappy, fake ident makes it into a commit object, then the right thing
is:

  git -c user.useConfigOnly=false pull --rebase

or even:

  git -c user.email=fake-but-ok@example.com pull --rebase

And they can do that preemptively for commands like the golang example
here. They shouldn't _have_ to do that, though, if the command wouldn't
actually create a commit. So I do think there may be a bug to be fixed,
but it is simply commands being over-eager to make sure we have an
ident when they might not need it.

-Peff

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> > Is this a case of rebase trying to make sure it has enough information
>> > for me to be a committer before knowing whether I even need to rewrite
>> > any commits, and could/should that be avoided?  Alternatively (or also)
>> > could/should rebase detect that a fast-forward is possible and prefer
>> > to do that instead?
>> 
>> I think that is a reasonable argument, but to solve this for a more
>> general case, shouldn't we be discussing a solution that would also
>> work when rebase _does_ need to create a new commit?  And when the
>> latter is solved, I would imagine that "this rebase happens to be
>> fast-forward, and not having an ident shouldn't be an issue for this
>> special case" would become moot.
>
> Wouldn't it be wrong to create a commit with non-config ident when
> user.useConfigOnly is set, though?

That is exactly what I was getting at.

> If the user is doing a one-off thing where they do not care if their
> crappy, fake ident makes it into a commit object, then the right thing
> is:
>
>   git -c user.useConfigOnly=false pull --rebase
>
> or even:
>
>   git -c user.email=fake-but-ok@example.com pull --rebase

Hmm, I somehow had an impression that these git commands are not
what the end-user runs from the command line, but wrapper tools like
"go get" has a hardcoded invocation of "git pull".

If a user sets useconfigonly globally, each repository must have
ident the user wants to use in it configured, so I would think that
a solution should be something that makes it easy to do so.

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Jeff King]

On Fri, Jul 29, 2016 at 11:32:40AM -0700, Junio C Hamano wrote:

> > Wouldn't it be wrong to create a commit with non-config ident when
> > user.useConfigOnly is set, though?
> 
> That is exactly what I was getting at.

Ah, OK, I thought you were trying to explore the opposite direction.

> > If the user is doing a one-off thing where they do not care if their
> > crappy, fake ident makes it into a commit object, then the right thing
> > is:
> >
> >   git -c user.useConfigOnly=false pull --rebase
> >
> > or even:
> >
> >   git -c user.email=fake-but-ok@example.com pull --rebase
> 
> Hmm, I somehow had an impression that these git commands are not
> what the end-user runs from the command line, but wrapper tools like
> "go get" has a hardcoded invocation of "git pull".

Yeah, the right person or entity to set those options is the one who
knows "the operation I am doing is OK even with bogus ident". I had
assumed if "go get" fell under that category, that it should be the one
to tell it to git (via the config above).

But I am not really sure that is the case. In general "go get" shouldn't
make commits if you aren't doing active work on the repo (AFAIK), and it
should just work.

From my limited testing, "git pull --rebase" is perfectly fine. The
culprit is "--rebase=preverse", which complains even if it would be a
fast-forward.

-Peff

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Jeff King]

On Fri, Jul 29, 2016 at 02:39:11PM -0400, Jeff King wrote:

> From my limited testing, "git pull --rebase" is perfectly fine. The
> culprit is "--rebase=preverse", which complains even if it would be a
> fast-forward.

That should be preserve, of course. :)

And I think I see what is happening. "preserve" implies
interactive-rebase, which makes an early check that we have valid
committer info, even though we might not actually write any new commits.

So doing this:

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ded4595..f0f4777 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1180,9 +1180,6 @@ To continue rebase after editing, run:
 	;;
 esac
 
-git var GIT_COMMITTER_IDENT >/dev/null ||
-	die "$(gettext "You need to set your committer info first")"
-
 comment_for_reflog start
 
 if test ! -z "$switch_to"

fixes it for me. I can't figure out if that would have any bad side
effects, though. That check comes from Dscho's original 1b1dce4 (Teach
rebase an interactive mode, 2007-06-25), so there's not much comment on
why it was added specifically.

We would notice the bogus ident later when we actually do try to create
a commit object, but I can guess that this up-front check might give us
a better error message. You get warned up-front, rather than something
like:

  Rebasing (1/1)
  *** Please tell me who you are.
  [...]
  fatal: no name was given and auto-detection is disabled
  Could not pick 8ebea123853128ca2411b2b449f76a1a4b0d026c

and dumped in the middle of an interactive rebase that you cannot
complete. OTOH, that is how a regular non-interactive merge works. And
if your next step is to set up your ident, then it's natural to do:

  git config user.email whatever
  git rebase --continue

So I'd lean towards dropping it, but maybe there are other hidden
gotchas.

-Peff

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... So I do think there may be a bug to be fixed,
> but it is simply commands being over-eager to make sure we have an
> ident when they might not need it.

36267854 (pull: fast-forward "pull --rebase=true", 2016-06-29) may
be a part of a good solution for that, perhaps?

-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Wed, 29 Jun 2016 10:22:31 -0700
Subject: [PATCH] pull: fast-forward "pull --rebase=true"

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/pull.c | 22 ++++++++++++++++++----
 1 file changed, 18 insertions(+), 4 deletions(-)

diff --git a/builtin/pull.c b/builtin/pull.c
index bf3fd3f..2a41d41 100644
--- a/builtin/pull.c
+++ b/builtin/pull.c
@@ -878,10 +878,24 @@ int cmd_pull(int argc, const char **argv, const char *prefix)
 		if (merge_heads.nr > 1)
 			die(_("Cannot merge multiple branches into empty head."));
 		return pull_into_void(*merge_heads.sha1, curr_head);
-	} else if (opt_rebase) {
-		if (merge_heads.nr > 1)
-			die(_("Cannot rebase onto multiple branches."));
+	}
+	if (opt_rebase && merge_heads.nr > 1)
+		die(_("Cannot rebase onto multiple branches."));
+
+	if (opt_rebase) {
+		struct commit_list *list = NULL;
+		struct commit *merge_head, *head;
+
+		head = lookup_commit_reference(orig_head);
+		commit_list_insert(head, &list);
+		merge_head = lookup_commit_reference(merge_heads.sha1[0]);
+		if (is_descendant_of(merge_head, list)) {
+			/* we can fast-forward this without invoking rebase */
+			opt_ff = "--ff-only";
+			return run_merge();
+		}
 		return run_rebase(curr_head, *merge_heads.sha1, rebase_fork_point);
-	} else
+	} else {
 		return run_merge();
+	}
 }
-- 
2.9.2-685-g483c9ea

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Jeff King]

On Fri, Jul 29, 2016 at 11:37:59AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > ... So I do think there may be a bug to be fixed,
> > but it is simply commands being over-eager to make sure we have an
> > ident when they might not need it.
> 
> 36267854 (pull: fast-forward "pull --rebase=true", 2016-06-29) may
> be a part of a good solution for that, perhaps?

Hmm. So the solution I came up with is below, and I think is the "most
correct" in the sense that it leaves the check to the low-level code
which actually creates commits, and so we know that we will absolutely
only ever complain when we actually need an ident.

But I could also see an argument along the lines of: if you do not have
an ident set up, you don't really have any business running "git rebase
-i" in the first place, and it is OK for us to complain even there's a
chance your rebase might be a noop. So we should prefer consistency over
absolute flexibility in rebase, and just fix the common case of invoking
"rebase" at all when we know it is a fast-forward.

Which is what your patch is doing. I can buy that argument in general
for rebase, though it is a little funny that "git pull --rebase" would
then behave differently than "git fetch && git rebase".

There's a middle ground, too, which goes something like: it is OK to
invoke "rebase" (interactive or not) without a valid ident if you have
no commits to rebase. But as soon as we know there are commits to pick,
we should do the ident check and bail, canceling the rebase entirely.

That seems to be what happens in the non-interactive case (we call "git
am --rebasing" and it bails immediately without leaving any state).
Implementing that would mean shifting rebase--interactive's ident check
to the right spot (after seeing the insn sheet has entries) and bailing
appropriately.

TBH, I'm not sure anybody cares that much between the three. Even before
user.useConfigOnly, this could be an issue on machines where the ident
could not be auto-configured, and it seems like nobody ran across it.
It's only the funny interaction with pull.rebase that makes it likely to
come up, so as long as that code path is fixed (one way or another), I
doubt anybody would bring it up again.

Anyway, here's my patch.

-- >8 --
Subject: rebase-interactive: drop early check for valid ident

Since the very inception of interactive-rebase in 1b1dce4
(Teach rebase an interactive mode, 2007-06-25), there has
been a preemptive check, before looking at any commits, to
see whether the user has a valid name/email combination.

This is convenient, because it means that we abort the
operation before even beginning (rather than just
complaining that we are unable to pick a particular commit).

However, it does the wrong thing when the rebase does not
actually need to generate any new commits (e.g., a
fast-forward with no commits to pick, or one where the base
stays the same, and we just pick the same commits without
rewriting anything). In this case it may complain about the
lack of ident, even though one would not be needed to
complete the operation.

This may seem like mere nit-picking, but because interactive
rebase underlies the "preserve-merges" rebase, somebody who
has set "pull.rebase" to "preserve" cannot make even a
fast-forward pull without a valid ident, as we bail before
even realizing the fast-forward nature.

This commit drops the extra ident check entirely. This means
we rely on individual commands that generate commit objects
to complain. So we will continue to notice and prevent cases
that actually do create commits, but with one important
difference: we fail while actually executing the "pick"
operations, and leave the rebase in a conflicted, half-done
state.

In some ways this is less convenient, but in some ways it is
more so; the user can then manually commit or even "git
rebase --continue" after setting up their ident (or
providing it as a one-off on the command line).

Reported-by: Dakota Hawkins <dakotahawkins@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 git-rebase--interactive.sh |  3 ---
 t/t7517-per-repo-email.sh  | 47 ++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 47 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index ded4595..f0f4777 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1180,9 +1180,6 @@ To continue rebase after editing, run:
 	;;
 esac
 
-git var GIT_COMMITTER_IDENT >/dev/null ||
-	die "$(gettext "You need to set your committer info first")"
-
 comment_for_reflog start
 
 if test ! -z "$switch_to"
diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
index 337e6e3..2a22fa7 100755
--- a/t/t7517-per-repo-email.sh
+++ b/t/t7517-per-repo-email.sh
@@ -36,4 +36,51 @@ test_expect_success 'succeeds cloning if global email is not set' '
 	git clone . clone
 '
 
+test_expect_success 'set up rebase scenarios' '
+	# temporarily enable an actual ident for this setup
+	test_config user.email foo@example.com &&
+	test_commit new &&
+	git branch side-without-commit HEAD^ &&
+	git checkout -b side-with-commit HEAD^ &&
+	test_commit side
+'
+
+test_expect_success 'fast-forward rebase does not care about ident' '
+	git checkout -B tmp side-without-commit &&
+	git rebase master
+'
+
+test_expect_success 'non-fast-forward rebase refuses to write commits' '
+	test_when_finished "git rebase --abort || true" &&
+	git checkout -B tmp side-with-commit &&
+	test_must_fail git rebase master
+'
+
+test_expect_success 'fast-forward rebase does not care about ident (interactive)' '
+	git checkout -B tmp side-without-commit &&
+	git rebase -i master
+'
+
+test_expect_success 'non-fast-forward rebase refuses to write commits (interactive)' '
+	test_when_finished "git rebase --abort || true" &&
+	git checkout -B tmp side-with-commit &&
+	test_must_fail git rebase -i master
+'
+
+test_expect_success 'noop interactive rebase does not care about ident' '
+	git checkout -B tmp side-with-commit &&
+	git rebase -i HEAD^
+'
+
+test_expect_success 'fast-forward rebase does not care about ident (preserve)' '
+	git checkout -B tmp side-without-commit &&
+	git rebase -p master
+'
+
+test_expect_success 'non-fast-forward rebase refuses to write commits (preserve)' '
+	test_when_finished "git rebase --abort || true" &&
+	git checkout -B tmp side-with-commit &&
+	test_must_fail git rebase -p master
+'
+
 test_done
-- 
2.9.2.666.g67a7da4

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> TBH, I'm not sure anybody cares that much between the three. Even before
> user.useConfigOnly, this could be an issue on machines where the ident
> could not be auto-configured, and it seems like nobody ran across it.
> It's only the funny interaction with pull.rebase that makes it likely to
> come up, so as long as that code path is fixed (one way or another), I
> doubt anybody would bring it up again.

Yup, I do not think the choice among the three would make all that
much difference in practice.  If I really have to pick one of them,
I think the one in your message I am responding to would make the
most sense.

The one I sent, which I wrote as a response to some end-user request
on the list back then, has been sitting on 'pu' for quite a while
because I didn't see a real use or positive support for it, and the
only reason why I sent it is because this might be that one
real use it wanted to see.


> In some ways this is less convenient, but in some ways it is
> more so; the user can then manually commit or even "git
> rebase --continue" after setting up their ident (or
> providing it as a one-off on the command line).

Yup, that is the controvercial bit, and I suspect Dscho's original
was siding for the "set up ident first, as you will need it anyway
eventually", so I'll let others with viewpoints different from us to
chime in first before picking it up.

Thanks.

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Jeff King]

On Fri, Jul 29, 2016 at 03:45:35PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > TBH, I'm not sure anybody cares that much between the three. Even before
> > user.useConfigOnly, this could be an issue on machines where the ident
> > could not be auto-configured, and it seems like nobody ran across it.
> > It's only the funny interaction with pull.rebase that makes it likely to
> > come up, so as long as that code path is fixed (one way or another), I
> > doubt anybody would bring it up again.
> 
> Yup, I do not think the choice among the three would make all that
> much difference in practice.  If I really have to pick one of them,
> I think the one in your message I am responding to would make the
> most sense.
> 
> The one I sent, which I wrote as a response to some end-user request
> on the list back then, has been sitting on 'pu' for quite a while
> because I didn't see a real use or positive support for it, and the
> only reason why I sent it is because this might be that one
> real use it wanted to see.

BTW, I didn't actually test yours, but if we do go that route I suspect
you can reuse the tests I posted by just replacing "git rebase" with
"git pull --rebase=<true|preserve> . master".

> > In some ways this is less convenient, but in some ways it is
> > more so; the user can then manually commit or even "git
> > rebase --continue" after setting up their ident (or
> > providing it as a one-off on the command line).
> 
> Yup, that is the controvercial bit, and I suspect Dscho's original
> was siding for the "set up ident first, as you will need it anyway
> eventually", so I'll let others with viewpoints different from us to
> chime in first before picking it up.

Very sensible. Thanks.

-Peff

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Dakota Hawkins]

On Fri, Jul 29, 2016 at 6:49 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Jul 29, 2016 at 03:45:35PM -0700, Junio C Hamano wrote:
>
>> Jeff King <peff@peff.net> writes:
>>
>> > TBH, I'm not sure anybody cares that much between the three. Even before
>> > user.useConfigOnly, this could be an issue on machines where the ident
>> > could not be auto-configured, and it seems like nobody ran across it.
>> > It's only the funny interaction with pull.rebase that makes it likely to
>> > come up, so as long as that code path is fixed (one way or another), I
>> > doubt anybody would bring it up again.
>>
>> Yup, I do not think the choice among the three would make all that
>> much difference in practice.  If I really have to pick one of them,
>> I think the one in your message I am responding to would make the
>> most sense.
>>
>> The one I sent, which I wrote as a response to some end-user request
>> on the list back then, has been sitting on 'pu' for quite a while
>> because I didn't see a real use or positive support for it, and the
>> only reason why I sent it is because this might be that one
>> real use it wanted to see.
>
> BTW, I didn't actually test yours, but if we do go that route I suspect
> you can reuse the tests I posted by just replacing "git rebase" with
> "git pull --rebase=<true|preserve> . master".
>
>> > In some ways this is less convenient, but in some ways it is
>> > more so; the user can then manually commit or even "git
>> > rebase --continue" after setting up their ident (or
>> > providing it as a one-off on the command line).
>>
>> Yup, that is the controvercial bit, and I suspect Dscho's original
>> was siding for the "set up ident first, as you will need it anyway
>> eventually", so I'll let others with viewpoints different from us to
>> chime in first before picking it up.
>
> Very sensible. Thanks.
>
> -Peff

All of the options sounds OK to me. I do like the idea of being able
to set it and --continue what I was doing.

Even more convenient than that would be an optional "user.prompt=true"
so the ident check could get what it needs from a simple terminal
prompt, set it in the local config, and try again before returning.

While that would be nice, I've been OK with the current system for my
working repositories for a while. I'm used to having to go set it and
repeat whatever I was trying to do, as it seems like I forget to set
my user.email about half of the time :)

-Dakota

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Earlier, Peff sent this patch (slightly buried in a discussion) on
"rebase -i" in <20160729223134.GA22591@sigill.intra.peff.net>.

> Subject: rebase-interactive: drop early check for valid ident
>
> Since the very inception of interactive-rebase in 1b1dce4
> (Teach rebase an interactive mode, 2007-06-25), there has
> been a preemptive check, before looking at any commits, to
> see whether the user has a valid name/email combination.
>
> This is convenient, because it means that we abort the
> operation before even beginning (rather than just
> complaining that we are unable to pick a particular commit).
>
> However, it does the wrong thing when the rebase does not
> actually need to generate any new commits (e.g., a
> fast-forward with no commits to pick, or one where the base
> stays the same, and we just pick the same commits without
> rewriting anything). In this case it may complain about the
> lack of ident, even though one would not be needed to
> complete the operation.
>
> This may seem like mere nit-picking, but because interactive
> rebase underlies the "preserve-merges" rebase, somebody who
> has set "pull.rebase" to "preserve" cannot make even a
> fast-forward pull without a valid ident, as we bail before
> even realizing the fast-forward nature.
>
> This commit drops the extra ident check entirely. This means
> we rely on individual commands that generate commit objects
> to complain. So we will continue to notice and prevent cases
> that actually do create commits, but with one important
> difference: we fail while actually executing the "pick"
> operations, and leave the rebase in a conflicted, half-done
> state.
>
> In some ways this is less convenient, but in some ways it is
> more so; the user can then manually commit or even "git
> rebase --continue" after setting up their ident (or
> providing it as a one-off on the command line).
>
> Reported-by: Dakota Hawkins <dakotahawkins@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---

To which, I responded (referring to the last paragraph):

    Yup, that is the controvercial bit, and I suspect Dscho's original
    was siding for the "set up ident first, as you will need it anyway
    eventually", so I'll let others with viewpoints different from us to
    chime in first before picking it up.

Do you have a preference either way to help us decide if we want to
take this change or not?

Thanks.

>  git-rebase--interactive.sh |  3 ---
>  t/t7517-per-repo-email.sh  | 47 ++++++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 47 insertions(+), 3 deletions(-)
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index ded4595..f0f4777 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -1180,9 +1180,6 @@ To continue rebase after editing, run:
>  	;;
>  esac
>  
> -git var GIT_COMMITTER_IDENT >/dev/null ||
> -	die "$(gettext "You need to set your committer info first")"
> -
>  comment_for_reflog start
>  
>  if test ! -z "$switch_to"
> diff --git a/t/t7517-per-repo-email.sh b/t/t7517-per-repo-email.sh
> index 337e6e3..2a22fa7 100755
> --- a/t/t7517-per-repo-email.sh
> +++ b/t/t7517-per-repo-email.sh
> @@ -36,4 +36,51 @@ test_expect_success 'succeeds cloning if global email is not set' '
>  	git clone . clone
>  '
>  
> +test_expect_success 'set up rebase scenarios' '
> +	# temporarily enable an actual ident for this setup
> +	test_config user.email foo@example.com &&
> +	test_commit new &&
> +	git branch side-without-commit HEAD^ &&
> +	git checkout -b side-with-commit HEAD^ &&
> +	test_commit side
> +'
> +
> +test_expect_success 'fast-forward rebase does not care about ident' '
> +	git checkout -B tmp side-without-commit &&
> +	git rebase master
> +'
> +
> +test_expect_success 'non-fast-forward rebase refuses to write commits' '
> +	test_when_finished "git rebase --abort || true" &&
> +	git checkout -B tmp side-with-commit &&
> +	test_must_fail git rebase master
> +'
> +
> +test_expect_success 'fast-forward rebase does not care about ident (interactive)' '
> +	git checkout -B tmp side-without-commit &&
> +	git rebase -i master
> +'
> +
> +test_expect_success 'non-fast-forward rebase refuses to write commits (interactive)' '
> +	test_when_finished "git rebase --abort || true" &&
> +	git checkout -B tmp side-with-commit &&
> +	test_must_fail git rebase -i master
> +'
> +
> +test_expect_success 'noop interactive rebase does not care about ident' '
> +	git checkout -B tmp side-with-commit &&
> +	git rebase -i HEAD^
> +'
> +
> +test_expect_success 'fast-forward rebase does not care about ident (preserve)' '
> +	git checkout -B tmp side-without-commit &&
> +	git rebase -p master
> +'
> +
> +test_expect_success 'non-fast-forward rebase refuses to write commits (preserve)' '
> +	test_when_finished "git rebase --abort || true" &&
> +	git checkout -B tmp side-with-commit &&
> +	test_must_fail git rebase -p master
> +'
> +
>  test_done

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Johannes Schindelin]

Hi Junio,

On Thu, 11 Aug 2016, Junio C Hamano wrote:

> Earlier, Peff sent this patch (slightly buried in a discussion) on
> "rebase -i" in <20160729223134.GA22591@sigill.intra.peff.net>.
> 
> > Subject: rebase-interactive: drop early check for valid ident
> >
> > Since the very inception of interactive-rebase in 1b1dce4
> > (Teach rebase an interactive mode, 2007-06-25), there has
> > been a preemptive check, before looking at any commits, to
> > see whether the user has a valid name/email combination.
> >
> > This is convenient, because it means that we abort the
> > operation before even beginning (rather than just
> > complaining that we are unable to pick a particular commit).
> >
> > However, it does the wrong thing when the rebase does not
> > actually need to generate any new commits (e.g., a
> > fast-forward with no commits to pick, or one where the base
> > stays the same, and we just pick the same commits without
> > rewriting anything). In this case it may complain about the
> > lack of ident, even though one would not be needed to
> > complete the operation.
> >
> > This may seem like mere nit-picking, but because interactive
> > rebase underlies the "preserve-merges" rebase, somebody who
> > has set "pull.rebase" to "preserve" cannot make even a
> > fast-forward pull without a valid ident, as we bail before
> > even realizing the fast-forward nature.
> >
> > This commit drops the extra ident check entirely. This means
> > we rely on individual commands that generate commit objects
> > to complain. So we will continue to notice and prevent cases
> > that actually do create commits, but with one important
> > difference: we fail while actually executing the "pick"
> > operations, and leave the rebase in a conflicted, half-done
> > state.
> >
> > In some ways this is less convenient, but in some ways it is
> > more so; the user can then manually commit or even "git
> > rebase --continue" after setting up their ident (or
> > providing it as a one-off on the command line).
> >
> > Reported-by: Dakota Hawkins <dakotahawkins@gmail.com>
> > Signed-off-by: Jeff King <peff@peff.net>
> > ---
> 
> To which, I responded (referring to the last paragraph):
> 
>     Yup, that is the controvercial bit, and I suspect Dscho's original
>     was siding for the "set up ident first, as you will need it anyway
>     eventually", so I'll let others with viewpoints different from us to
>     chime in first before picking it up.
> 
> Do you have a preference either way to help us decide if we want to
> take this change or not?

I have no strong preference. I guess that it does not hurt to go with the
patch, and it would probably help in a few cases.

Ciao,
Dscho

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Thu, 11 Aug 2016, Junio C Hamano wrote:
>> 
>> Do you have a preference either way to help us decide if we want to
>> take this change or not?
>
> I have no strong preference. I guess that it does not hurt to go with the
> patch, and it would probably help in a few cases.

OK.  Let me dig the change back and how well it still fits ;-)

Thanks.

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
>> On Thu, 11 Aug 2016, Junio C Hamano wrote:
>>> 
>>> Do you have a preference either way to help us decide if we want to
>>> take this change or not?
>>
>> I have no strong preference. I guess that it does not hurt to go with the
>> patch, and it would probably help in a few cases.
>
> OK.  Let me dig the change back and how well it still fits ;-)

Ah, I already had it in my tree lest I forget.  Let me mark it for
merging down to 'master'.

Thanks.

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Dakota Hawkins]

You guys are the best, I'm really impressed with all of the responses
to this issue! Thank you all for all of your hard work!

Dakota

On Fri, Sep 9, 2016 at 3:00 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>>
>>> On Thu, 11 Aug 2016, Junio C Hamano wrote:
>>>>
>>>> Do you have a preference either way to help us decide if we want to
>>>> take this change or not?
>>>
>>> I have no strong preference. I guess that it does not hurt to go with the
>>> patch, and it would probably help in a few cases.
>>
>> OK.  Let me dig the change back and how well it still fits ;-)
>
> Ah, I already had it in my tree lest I forget.  Let me mark it for
> merging down to 'master'.
>
> Thanks.

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Dakota Hawkins <dakotahawkins@gmail.com> writes:
>
>> In those cases specifically, I never have local commits that differ
>> from the remote, so a "pull --ff-only" should leave me in the same
>> state as a "pull --rebase".
>>
>> Is this a case of rebase trying to make sure it has enough information
>> for me to be a committer before knowing whether I even need to rewrite
>> any commits, and could/should that be avoided?  Alternatively (or also)
>> could/should rebase detect that a fast-forward is possible and prefer
>> to do that instead?
>
> I think that is a reasonable argument,...

There is one that still wants to know who you are, I think.  The
reflog entries record who moved the tip of the ref and when, and
obviously a fast-forward is also recorded.

I _think_ our intention was to allow a bogus ident in reflog entries
(even though we want to avoid a bogus ident in commits and tags), so
perhaps additional code/logic for user.useConfigOnly may need to know
about that (I didn't dig)?

Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email"

[Jeff King]

On Fri, Jul 29, 2016 at 11:20:49AM -0700, Junio C Hamano wrote:

> There is one that still wants to know who you are, I think.  The
> reflog entries record who moved the tip of the ref and when, and
> obviously a fast-forward is also recorded.
> 
> I _think_ our intention was to allow a bogus ident in reflog entries
> (even though we want to avoid a bogus ident in commits and tags), so
> perhaps additional code/logic for user.useConfigOnly may need to know
> about that (I didn't dig)?

I think we handle this case OK, or you would not even be able to "git
fetch" into a repository.

It works because the check is predicated on the "strict" flag, and the
reflog writer passes IDENT_NO_STRICT.

-Peff