* Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" @ 2016-07-29 9:17 Dakota Hawkins 2016-07-29 17:47 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Dakota Hawkins @ 2016-07-29 9:17 UTC (permalink / raw) To: git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 9:17 Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" Dakota Hawkins @ 2016-07-29 17:47 ` Junio C Hamano 2016-07-29 18:11 ` Jeff King 2016-07-29 18:20 ` Junio C Hamano 0 siblings, 2 replies; 18+ messages in thread From: Junio C Hamano @ 2016-07-29 17:47 UTC (permalink / raw) To: Dakota Hawkins; +Cc: git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 17:47 ` Junio C Hamano @ 2016-07-29 18:11 ` Jeff King 2016-07-29 18:32 ` Junio C Hamano 2016-07-29 18:37 ` Junio C Hamano 2016-07-29 18:20 ` Junio C Hamano 1 sibling, 2 replies; 18+ messages in thread From: Jeff King @ 2016-07-29 18:11 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dakota Hawkins, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 18:11 ` Jeff King @ 2016-07-29 18:32 ` Junio C Hamano 2016-07-29 18:39 ` Jeff King 2016-07-29 18:37 ` Junio C Hamano 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-07-29 18:32 UTC (permalink / raw) To: Jeff King; +Cc: Dakota Hawkins, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 18:32 ` Junio C Hamano @ 2016-07-29 18:39 ` Jeff King 2016-07-29 18:52 ` Jeff King 0 siblings, 1 reply; 18+ messages in thread From: Jeff King @ 2016-07-29 18:39 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dakota Hawkins, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 18:39 ` Jeff King @ 2016-07-29 18:52 ` Jeff King 0 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2016-07-29 18:52 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, Dakota Hawkins, git 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 18:11 ` Jeff King 2016-07-29 18:32 ` Junio C Hamano @ 2016-07-29 18:37 ` Junio C Hamano 2016-07-29 22:31 ` Jeff King 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-07-29 18:37 UTC (permalink / raw) To: Jeff King; +Cc: Dakota Hawkins, git 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 18:37 ` Junio C Hamano @ 2016-07-29 22:31 ` Jeff King 2016-07-29 22:45 ` Junio C Hamano 2016-08-11 22:44 ` Junio C Hamano 0 siblings, 2 replies; 18+ messages in thread From: Jeff King @ 2016-07-29 22:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, Dakota Hawkins, git 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 ^ permalink raw reply related [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 22:31 ` Jeff King @ 2016-07-29 22:45 ` Junio C Hamano 2016-07-29 22:49 ` Jeff King 2016-08-11 22:44 ` Junio C Hamano 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-07-29 22:45 UTC (permalink / raw) To: Jeff King; +Cc: Johannes Schindelin, Dakota Hawkins, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 22:45 ` Junio C Hamano @ 2016-07-29 22:49 ` Jeff King 2016-07-30 16:41 ` Dakota Hawkins 0 siblings, 1 reply; 18+ messages in thread From: Jeff King @ 2016-07-29 22:49 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, Dakota Hawkins, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 22:49 ` Jeff King @ 2016-07-30 16:41 ` Dakota Hawkins 0 siblings, 0 replies; 18+ messages in thread From: Dakota Hawkins @ 2016-07-30 16:41 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 22:31 ` Jeff King 2016-07-29 22:45 ` Junio C Hamano @ 2016-08-11 22:44 ` Junio C Hamano 2016-09-09 15:32 ` Johannes Schindelin 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-08-11 22:44 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, Dakota Hawkins, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-08-11 22:44 ` Junio C Hamano @ 2016-09-09 15:32 ` Johannes Schindelin 2016-09-09 16:09 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Johannes Schindelin @ 2016-09-09 15:32 UTC (permalink / raw) To: Junio C Hamano; +Cc: Jeff King, Dakota Hawkins, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-09-09 15:32 ` Johannes Schindelin @ 2016-09-09 16:09 ` Junio C Hamano 2016-09-09 19:00 ` Junio C Hamano 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-09-09 16:09 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, Dakota Hawkins, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-09-09 16:09 ` Junio C Hamano @ 2016-09-09 19:00 ` Junio C Hamano 2016-09-09 23:31 ` Dakota Hawkins 0 siblings, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-09-09 19:00 UTC (permalink / raw) To: Johannes Schindelin; +Cc: Jeff King, Dakota Hawkins, git 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-09-09 19:00 ` Junio C Hamano @ 2016-09-09 23:31 ` Dakota Hawkins 0 siblings, 0 replies; 18+ messages in thread From: Dakota Hawkins @ 2016-09-09 23:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Johannes Schindelin, Jeff King, Git Mailing List 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. ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 17:47 ` Junio C Hamano 2016-07-29 18:11 ` Jeff King @ 2016-07-29 18:20 ` Junio C Hamano 2016-07-29 18:31 ` Jeff King 1 sibling, 1 reply; 18+ messages in thread From: Junio C Hamano @ 2016-07-29 18:20 UTC (permalink / raw) To: Dakota Hawkins; +Cc: git 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)? ^ permalink raw reply [flat|nested] 18+ messages in thread
* Re: Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" 2016-07-29 18:20 ` Junio C Hamano @ 2016-07-29 18:31 ` Jeff King 0 siblings, 0 replies; 18+ messages in thread From: Jeff King @ 2016-07-29 18:31 UTC (permalink / raw) To: Junio C Hamano; +Cc: Dakota Hawkins, git 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 ^ permalink raw reply [flat|nested] 18+ messages in thread
end of thread, other threads:[~2016-09-09 23:31 UTC | newest] Thread overview: 18+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-07-29 9:17 Issue with global config defaults "user.useConfigOnly = true" + "pull.rebase = preserve" - "user.email" Dakota Hawkins 2016-07-29 17:47 ` Junio C Hamano 2016-07-29 18:11 ` Jeff King 2016-07-29 18:32 ` Junio C Hamano 2016-07-29 18:39 ` Jeff King 2016-07-29 18:52 ` Jeff King 2016-07-29 18:37 ` Junio C Hamano 2016-07-29 22:31 ` Jeff King 2016-07-29 22:45 ` Junio C Hamano 2016-07-29 22:49 ` Jeff King 2016-07-30 16:41 ` Dakota Hawkins 2016-08-11 22:44 ` Junio C Hamano 2016-09-09 15:32 ` Johannes Schindelin 2016-09-09 16:09 ` Junio C Hamano 2016-09-09 19:00 ` Junio C Hamano 2016-09-09 23:31 ` Dakota Hawkins 2016-07-29 18:20 ` Junio C Hamano 2016-07-29 18:31 ` Jeff King
Code repositories for project(s) associated with this public inbox https://80x24.org/mirrors/git.git This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).