* [PATCH] pull: refuse complete src:dst fetchspec arguments
@ 2009-10-20 18:23 Thomas Rast
2009-10-20 18:37 ` [RFC! PATCH] " Thomas Rast
` (4 more replies)
0 siblings, 5 replies; 21+ messages in thread
From: Thomas Rast @ 2009-10-20 18:23 UTC (permalink / raw
To: git
git-pull has historically accepted full fetchspecs, meaning that you
could do
git pull $repo A:B
which would simultaneously fetch the remote branch A into the local
branch B and merge B into HEAD. This got especially confusing if B
was checked out. New users variously mistook pull for fetch or read
that command as "merge the remote A into my B", neither of which is
correct.
Since the above usage should be very rare and can be done with
separate calls to fetch and merge, we just disallow full fetchspecs in
git-pull.
Signed-off-by: Thomas Rast <trast@student.ethz.ch>
---
This actually came up on IRC *twice* this week.
git-pull.sh | 19 +++++++++++++++++++
t/t5520-pull.sh | 12 ------------
2 files changed, 19 insertions(+), 12 deletions(-)
diff --git a/git-pull.sh b/git-pull.sh
index fc78592..8f06491 100755
--- a/git-pull.sh
+++ b/git-pull.sh
@@ -131,6 +131,25 @@ error_on_no_merge_candidates () {
exit 1
}
+check_full_fetchspec () {
+ shift # discard remote argument, if any
+ for arg in "$@"
+ do
+ case "$arg" in
+ *:*)
+ echo "$arg"
+ return
+ ;;
+ esac
+ done
+}
+
+full_fetchspec=$(check_full_fetchspec "$@")
+if test -n "$full_fetchspec"
+then
+ die "full fetchspec '$full_fetchspec' not allowed"
+fi
+
test true = "$rebase" && {
if ! git rev-parse -q --verify HEAD >/dev/null
then
diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
index dd2ee84..a566a99 100755
--- a/t/t5520-pull.sh
+++ b/t/t5520-pull.sh
@@ -29,18 +29,6 @@ test_expect_success 'checking the results' '
diff file cloned/file
'
-test_expect_success 'pulling into void using master:master' '
- mkdir cloned-uho &&
- (
- cd cloned-uho &&
- git init &&
- git pull .. master:master
- ) &&
- test -f file &&
- test -f cloned-uho/file &&
- test_cmp file cloned-uho/file
-'
-
test_expect_success 'test . as a remote' '
git branch copy master &&
--
1.6.5.1.144.g40216
^ permalink raw reply related [flat|nested] 21+ messages in thread
* Re: [RFC! PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 18:23 [PATCH] pull: refuse complete src:dst fetchspec arguments Thomas Rast
@ 2009-10-20 18:37 ` Thomas Rast
2009-10-20 19:29 ` [PATCH] " Wesley J. Landaker
` (3 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Thomas Rast @ 2009-10-20 18:37 UTC (permalink / raw
To: git
Thomas Rast wrote:
> git-pull has historically accepted full fetchspecs, meaning that you
> could do
>
> git pull $repo A:B
>
> which would simultaneously fetch the remote branch A into the local
> branch B and merge B into HEAD. This got especially confusing if B
> was checked out. New users variously mistook pull for fetch or read
> that command as "merge the remote A into my B", neither of which is
> correct.
>
> Since the above usage should be very rare and can be done with
> separate calls to fetch and merge, we just disallow full fetchspecs in
> git-pull.
>
> Signed-off-by: Thomas Rast <trast@student.ethz.ch>
Argh. This was actually supposed to be an *RFC* patch.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 18:23 [PATCH] pull: refuse complete src:dst fetchspec arguments Thomas Rast
2009-10-20 18:37 ` [RFC! PATCH] " Thomas Rast
@ 2009-10-20 19:29 ` Wesley J. Landaker
2009-10-20 20:30 ` Sean Estabrooks
` (2 subsequent siblings)
4 siblings, 0 replies; 21+ messages in thread
From: Wesley J. Landaker @ 2009-10-20 19:29 UTC (permalink / raw
To: Thomas Rast; +Cc: git
On Tuesday 20 October 2009 12:23:06 Thomas Rast wrote:
> git-pull has historically accepted full fetchspecs, meaning that you
> could do
>
> git pull $repo A:B
>
> which would simultaneously fetch the remote branch A into the local
> branch B and merge B into HEAD. This got especially confusing if B
> was checked out. New users variously mistook pull for fetch or read
> that command as "merge the remote A into my B", neither of which is
> correct.
One thought here is that if the change you suggested (and I personally like)
in your "[RFC] pull/fetch rename" thread was made, then I would expect to be
able to run this exact command to have git fetch the remote branch A into
the local branch B (with no merging taking place, because I didn't say --
merge). So basically, it would be like "git fetch $repo A:B" is now.
I readily agree that the *current* behavior of that command would have
probably caught me off-guard, since I probably only would have typed that on
accident (e.g. using "pull" when I meant "fetch").
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 18:23 [PATCH] pull: refuse complete src:dst fetchspec arguments Thomas Rast
2009-10-20 18:37 ` [RFC! PATCH] " Thomas Rast
2009-10-20 19:29 ` [PATCH] " Wesley J. Landaker
@ 2009-10-20 20:30 ` Sean Estabrooks
2009-10-20 21:11 ` Junio C Hamano
` (2 more replies)
2009-11-15 12:24 ` Thomas Rast
2009-12-29 11:05 ` Nanako Shiraishi
4 siblings, 3 replies; 21+ messages in thread
From: Sean Estabrooks @ 2009-10-20 20:30 UTC (permalink / raw
To: Thomas Rast; +Cc: git
On Tue, 20 Oct 2009 20:23:06 +0200
Thomas Rast <trast@student.ethz.ch> wrote:
Hi Thomas,
> git-pull has historically accepted full fetchspecs, meaning that you
> could do
>
> git pull $repo A:B
>
> which would simultaneously fetch the remote branch A into the local
> branch B and merge B into HEAD. This got especially confusing if B
> was checked out. New users variously mistook pull for fetch or read
> that command as "merge the remote A into my B", neither of which is
> correct.
>
> Since the above usage should be very rare and can be done with
> separate calls to fetch and merge, we just disallow full fetchspecs in
> git-pull.
It is however a handy shortcut to be able to specify the full refspec
and specify where you want the head stored locally. It seems a shame to
throw away that functionality because of one confusing case. Wouldn't
it be better to test of the confusing case and instead error out if the
local refname is already checked out?
[...]
> diff --git a/t/t5520-pull.sh b/t/t5520-pull.sh
> index dd2ee84..a566a99 100755
> --- a/t/t5520-pull.sh
> +++ b/t/t5520-pull.sh
> @@ -29,18 +29,6 @@ test_expect_success 'checking the results' '
> diff file cloned/file
> '
>
> -test_expect_success 'pulling into void using master:master' '
> - mkdir cloned-uho &&
> - (
> - cd cloned-uho &&
> - git init &&
> - git pull .. master:master
> - ) &&
> - test -f file &&
> - test -f cloned-uho/file &&
> - test_cmp file cloned-uho/file
> -
> -
> test_expect_success 'test . as a remote' '
>
> git branch copy master &&
> --
>
Instead of removing this test it should be modified or replaced
with a test that ensures the new functionality operates correctly.
In this case that would mean checking that using a full refspec
errors out.
Cheers,
Sean
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 20:30 ` Sean Estabrooks
@ 2009-10-20 21:11 ` Junio C Hamano
2009-10-21 0:15 ` Daniel Barkalow
2009-10-21 8:06 ` Thomas Rast
2 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2009-10-20 21:11 UTC (permalink / raw
To: Sean Estabrooks; +Cc: Thomas Rast, git
Sean Estabrooks <seanlkml@sympatico.ca> writes:
>> -test_expect_success 'pulling into void using master:master' '
>> - mkdir cloned-uho &&
>> - (
>> - cd cloned-uho &&
>> - git init &&
>> - git pull .. master:master
>> - ) &&
>> - test -f file &&
>> - test -f cloned-uho/file &&
>> - test_cmp file cloned-uho/file
>> -
>> -
>> test_expect_success 'test . as a remote' '
>>
>> git branch copy master &&
>> --
>>
>
> Instead of removing this test it should be modified or replaced
> with a test that ensures the new functionality operates correctly.
> In this case that would mean checking that using a full refspec
> errors out.
Shouldn't "git pull .. master" still work in this case, too? So this test
will probably become two tests, one for "git pull .. master:master" that
correctly fails, and the other for "git pull .. master" to still work as
expected.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 20:30 ` Sean Estabrooks
2009-10-20 21:11 ` Junio C Hamano
@ 2009-10-21 0:15 ` Daniel Barkalow
2009-10-21 0:29 ` Sean Estabrooks
2009-10-21 8:06 ` Thomas Rast
2 siblings, 1 reply; 21+ messages in thread
From: Daniel Barkalow @ 2009-10-21 0:15 UTC (permalink / raw
To: Sean Estabrooks; +Cc: Thomas Rast, git
On Tue, 20 Oct 2009, Sean Estabrooks wrote:
> On Tue, 20 Oct 2009 20:23:06 +0200
> Thomas Rast <trast@student.ethz.ch> wrote:
>
> Hi Thomas,
>
> > git-pull has historically accepted full fetchspecs, meaning that you
> > could do
> >
> > git pull $repo A:B
> >
> > which would simultaneously fetch the remote branch A into the local
> > branch B and merge B into HEAD. This got especially confusing if B
> > was checked out. New users variously mistook pull for fetch or read
> > that command as "merge the remote A into my B", neither of which is
> > correct.
> >
> > Since the above usage should be very rare and can be done with
> > separate calls to fetch and merge, we just disallow full fetchspecs in
> > git-pull.
>
> It is however a handy shortcut to be able to specify the full refspec
> and specify where you want the head stored locally. It seems a shame to
> throw away that functionality because of one confusing case. Wouldn't
> it be better to test of the confusing case and instead error out if the
> local refname is already checked out?
Surely, "where you want the head stored locally" is somewhere that's
information about a remote repository, and therefore under "refs/remotes/"
(or "refs/tags/" or something) and therefore not possible to be checked
out (in the "HEAD is a symref to it" sense).
I don't think it should be possible to fast-forward or create a local
branch from a remote branch while simultaneously merging it into the
currently-checked-out local branch.
Actually, I think it would be good to prohibit fetching into a new or
existing local branch, whether or not it is checked out. We'd probably
need to provide a plumbing method of doing a fetch, though, for script
environments that aren't using the normal porcelain meanings of refs/
subdirectories. (Defining a bare repo with --mirror as not having local
branches, of course)
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-21 0:15 ` Daniel Barkalow
@ 2009-10-21 0:29 ` Sean Estabrooks
2009-10-21 0:55 ` Daniel Barkalow
0 siblings, 1 reply; 21+ messages in thread
From: Sean Estabrooks @ 2009-10-21 0:29 UTC (permalink / raw
To: Daniel Barkalow; +Cc: Thomas Rast, git
On Tue, 20 Oct 2009 20:15:23 -0400 (EDT)
Daniel Barkalow <barkalow@iabervon.org> wrote:
Hi Daniel,
> Surely, "where you want the head stored locally" is somewhere that's
> information about a remote repository, and therefore under "refs/remotes/"
> (or "refs/tags/" or something) and therefore not possible to be checked
> out (in the "HEAD is a symref to it" sense).
Maybe, but it could also just be to create a temp local branch for
merging into additional branches afterward with "checkout other;
merge temp". This is especially helpful when pulling from an
annoyingly long URL instead of from a configured remote.
> I don't think it should be possible to fast-forward or create a local
> branch from a remote branch while simultaneously merging it into the
> currently-checked-out local branch.
What is the harm? Nobody is forced to use the facility and it does
have some marginal utility. I'd not fight for it, but i don't yet
understand the argument to prohibit it.
> Actually, I think it would be good to prohibit fetching into a new or
> existing local branch, whether or not it is checked out. We'd probably
> need to provide a plumbing method of doing a fetch, though, for script
> environments that aren't using the normal porcelain meanings of refs/
> subdirectories. (Defining a bare repo with --mirror as not having local
> branches, of course)
I'm hoping you don't mean that all fetching to a new local branch should
be prohibited and you're only talking about the current issue of full
refspecs on and the pull command. Otherwise i'd say it seems
unnecessarily restrictive.
Cheers,
Sean
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-21 0:29 ` Sean Estabrooks
@ 2009-10-21 0:55 ` Daniel Barkalow
2009-10-21 1:35 ` Sean Estabrooks
2009-10-21 3:15 ` Björn Steinbrink
0 siblings, 2 replies; 21+ messages in thread
From: Daniel Barkalow @ 2009-10-21 0:55 UTC (permalink / raw
To: Sean Estabrooks; +Cc: Thomas Rast, git
On Tue, 20 Oct 2009, Sean Estabrooks wrote:
> On Tue, 20 Oct 2009 20:15:23 -0400 (EDT)
> Daniel Barkalow <barkalow@iabervon.org> wrote:
>
> Hi Daniel,
>
> > Surely, "where you want the head stored locally" is somewhere that's
> > information about a remote repository, and therefore under "refs/remotes/"
> > (or "refs/tags/" or something) and therefore not possible to be checked
> > out (in the "HEAD is a symref to it" sense).
>
> Maybe, but it could also just be to create a temp local branch for
> merging into additional branches afterward with "checkout other;
> merge temp". This is especially helpful when pulling from an
> annoyingly long URL instead of from a configured remote.
Maybe it should be fine to do:
$ git fetch long-url-here master:temp
$ git merge temp
$ git checkout other-branch-that-also-needs-it
$ git merge temp
But "temp" is "refs/remotes/temp", not "refs/heads/temp"?
> > Actually, I think it would be good to prohibit fetching into a new or
> > existing local branch, whether or not it is checked out. We'd probably
> > need to provide a plumbing method of doing a fetch, though, for script
> > environments that aren't using the normal porcelain meanings of refs/
> > subdirectories. (Defining a bare repo with --mirror as not having local
> > branches, of course)
>
> I'm hoping you don't mean that all fetching to a new local branch should
> be prohibited and you're only talking about the current issue of full
> refspecs on and the pull command. Otherwise i'd say it seems
> unnecessarily restrictive.
I think, actually, that creating or changing a local branch is really not
what "fetch" (or the fetch part of pull) is about. I think that just leads
to confusion about what's locally-controlled and what's a local memory of
something remotely-controlled.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-21 0:55 ` Daniel Barkalow
@ 2009-10-21 1:35 ` Sean Estabrooks
2009-10-21 3:15 ` Björn Steinbrink
1 sibling, 0 replies; 21+ messages in thread
From: Sean Estabrooks @ 2009-10-21 1:35 UTC (permalink / raw
To: Daniel Barkalow; +Cc: Thomas Rast, git
On Tue, 20 Oct 2009 20:55:25 -0400 (EDT)
Daniel Barkalow <barkalow@iabervon.org> wrote:
> > Maybe, but it could also just be to create a temp local branch for
> > merging into additional branches afterward with "checkout other;
> > merge temp". This is especially helpful when pulling from an
> > annoyingly long URL instead of from a configured remote.
>
> Maybe it should be fine to do:
>
> $ git fetch long-url-here master:temp
> $ git merge temp
> $ git checkout other-branch-that-also-needs-it
> $ git merge temp
>
> But "temp" is "refs/remotes/temp", not "refs/heads/temp"?
Well that's only one example of possibile uses for fetching directly to
a local branch, perhaps as a new base of further development. Is there
really a compelling reason to force someone to fetch into refs/remotes
and then do the extra step of checking it out locally?
> I think, actually, that creating or changing a local branch is really not
> what "fetch" (or the fetch part of pull) is about. I think that just leads
> to confusion about what's locally-controlled and what's a local memory of
> something remotely-controlled.
Well it's a handy shortcut for several situations. There must be a way
to protect less adroit Git users without removing functionality.
Sean
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-21 0:55 ` Daniel Barkalow
2009-10-21 1:35 ` Sean Estabrooks
@ 2009-10-21 3:15 ` Björn Steinbrink
2009-10-21 4:32 ` Daniel Barkalow
2009-10-21 8:05 ` Thomas Rast
1 sibling, 2 replies; 21+ messages in thread
From: Björn Steinbrink @ 2009-10-21 3:15 UTC (permalink / raw
To: Daniel Barkalow; +Cc: Sean Estabrooks, Thomas Rast, git
On 2009.10.20 20:55:25 -0400, Daniel Barkalow wrote:
> Maybe it should be fine to do:
>
> $ git fetch long-url-here master:temp
> $ git merge temp
> $ git checkout other-branch-that-also-needs-it
> $ git merge temp
>
> But "temp" is "refs/remotes/temp", not "refs/heads/temp"?
One (maybe important) difference there is that the "pull" gets you:
Merge branch 'pu' of git://git.kernel.org/pub/scm/git/git
Even with "master:tmp". But with fetch+merge (storing in refs/remotes):
Merge remote branch 'tmp'
As a minor side-effect, having the "tmp" ref makes re-running the pull
(for whatever reason) cheaper, as without it, the fetch step would
possibly re-fetch the whole stuff (not reachable through any local ref).
Björn, undecided...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-21 3:15 ` Björn Steinbrink
@ 2009-10-21 4:32 ` Daniel Barkalow
2009-10-21 8:05 ` Thomas Rast
1 sibling, 0 replies; 21+ messages in thread
From: Daniel Barkalow @ 2009-10-21 4:32 UTC (permalink / raw
To: Björn Steinbrink; +Cc: Sean Estabrooks, Thomas Rast, git
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1172 bytes --]
On Wed, 21 Oct 2009, Björn Steinbrink wrote:
> On 2009.10.20 20:55:25 -0400, Daniel Barkalow wrote:
> > Maybe it should be fine to do:
> >
> > $ git fetch long-url-here master:temp
> > $ git merge temp
> > $ git checkout other-branch-that-also-needs-it
> > $ git merge temp
> >
> > But "temp" is "refs/remotes/temp", not "refs/heads/temp"?
>
> One (maybe important) difference there is that the "pull" gets you:
>
> Merge branch 'pu' of git://git.kernel.org/pub/scm/git/git
>
> Even with "master:tmp". But with fetch+merge (storing in refs/remotes):
>
> Merge remote branch 'tmp'
It would be nice to improve that in general, I think. You may fetch before
merging in order to check out what you're getting, and then lose
FETCH_HEAD (or have not specified the branch), and you have to contact the
remote server again if you want the message with its url.
> As a minor side-effect, having the "tmp" ref makes re-running the pull
> (for whatever reason) cheaper, as without it, the fetch step would
> possibly re-fetch the whole stuff (not reachable through any local ref).
Only if the merge failed, but yes.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-21 3:15 ` Björn Steinbrink
2009-10-21 4:32 ` Daniel Barkalow
@ 2009-10-21 8:05 ` Thomas Rast
2009-10-23 2:54 ` Jeff King
1 sibling, 1 reply; 21+ messages in thread
From: Thomas Rast @ 2009-10-21 8:05 UTC (permalink / raw
To: Björn Steinbrink; +Cc: Daniel Barkalow, Sean Estabrooks, git
Björn Steinbrink wrote:
> One (maybe important) difference there is that the "pull" gets you:
>
> Merge branch 'pu' of git://git.kernel.org/pub/scm/git/git
>
> Even with "master:tmp". But with fetch+merge (storing in refs/remotes):
>
> Merge remote branch 'tmp'
What if any combination of fetch and merge always gave you the long
form? After all, even if you do have a tracking branch for whatever
you are merging, that information is probably useless and it would be
nicer if all of the following resulted in the long form:
* git fetch git://git.kernel.org/pub/scm/git/git pu
git merge FETCH_HEAD
* git remote add origin git://git.kernel.org/pub/scm/git/git
git fetch origin
git merge origin/pu
* git fetch git://git.kernel.org/pub/scm/git/git pu:tmp
git merge tmp
and so on.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 20:30 ` Sean Estabrooks
2009-10-20 21:11 ` Junio C Hamano
2009-10-21 0:15 ` Daniel Barkalow
@ 2009-10-21 8:06 ` Thomas Rast
2 siblings, 0 replies; 21+ messages in thread
From: Thomas Rast @ 2009-10-21 8:06 UTC (permalink / raw
To: Sean Estabrooks; +Cc: git
Sean Estabrooks wrote:
> Instead of removing this test it should be modified or replaced
> with a test that ensures the new functionality operates correctly.
> In this case that would mean checking that using a full refspec
> errors out.
Indeed, sorry. I meant it to be flagged as an RFC patch, and with
those I usually go for the minimal possible effort to not break the
test suite outright. As such, it also lacks doc updates.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-21 8:05 ` Thomas Rast
@ 2009-10-23 2:54 ` Jeff King
2009-10-23 3:43 ` Daniel Barkalow
0 siblings, 1 reply; 21+ messages in thread
From: Jeff King @ 2009-10-23 2:54 UTC (permalink / raw
To: Thomas Rast; +Cc: Björn Steinbrink, Daniel Barkalow, Sean Estabrooks, git
On Wed, Oct 21, 2009 at 10:05:27AM +0200, Thomas Rast wrote:
> What if any combination of fetch and merge always gave you the long
> form? After all, even if you do have a tracking branch for whatever
> you are merging, that information is probably useless and it would be
> nicer if all of the following resulted in the long form:
>
> * git fetch git://git.kernel.org/pub/scm/git/git pu
> git merge FETCH_HEAD
>
> * git remote add origin git://git.kernel.org/pub/scm/git/git
> git fetch origin
> git merge origin/pu
>
> * git fetch git://git.kernel.org/pub/scm/git/git pu:tmp
> git merge tmp
Maybe it's just me, but I actually prefer the shorthand names. Five
years from now when I browse the history and see that I merged
remote branch "mike/topic", I'll know exactly what that means: developer
Mike's version of a certain topic branch. But I am not likely to care
about exactly where we were storing developer repos at that time.
But probably that is an artifact of the workflow. The scenario I am
describing above implies a somewhat centralized workflow, where the
shorthand contains all of the interesting information. In a totally
distributed, we-don't-share-anything-except-the-url-namespace setup of
an open source repo, the full URL makes more sense.
So maybe it is something that should be optional.
-Peff
>
> and so on.
>
> --
> Thomas Rast
> trast@{inf,student}.ethz.ch
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at http://vger.kernel.org/majordomo-info.html
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-23 2:54 ` Jeff King
@ 2009-10-23 3:43 ` Daniel Barkalow
2009-10-24 0:49 ` Jeff King
0 siblings, 1 reply; 21+ messages in thread
From: Daniel Barkalow @ 2009-10-23 3:43 UTC (permalink / raw
To: Jeff King; +Cc: Thomas Rast, Björn Steinbrink, Sean Estabrooks, git
On Thu, 22 Oct 2009, Jeff King wrote:
> On Wed, Oct 21, 2009 at 10:05:27AM +0200, Thomas Rast wrote:
>
> > What if any combination of fetch and merge always gave you the long
> > form? After all, even if you do have a tracking branch for whatever
> > you are merging, that information is probably useless and it would be
> > nicer if all of the following resulted in the long form:
> >
> > * git fetch git://git.kernel.org/pub/scm/git/git pu
> > git merge FETCH_HEAD
> >
> > * git remote add origin git://git.kernel.org/pub/scm/git/git
> > git fetch origin
> > git merge origin/pu
> >
> > * git fetch git://git.kernel.org/pub/scm/git/git pu:tmp
> > git merge tmp
>
> Maybe it's just me, but I actually prefer the shorthand names. Five
> years from now when I browse the history and see that I merged
> remote branch "mike/topic", I'll know exactly what that means: developer
> Mike's version of a certain topic branch. But I am not likely to care
> about exactly where we were storing developer repos at that time.
>
> But probably that is an artifact of the workflow. The scenario I am
> describing above implies a somewhat centralized workflow, where the
> shorthand contains all of the interesting information. In a totally
> distributed, we-don't-share-anything-except-the-url-namespace setup of
> an open source repo, the full URL makes more sense.
>
> So maybe it is something that should be optional.
Surely you ought to be able to get the short form with "pull", though, if
you happen to like short forms. So it would make sense to decide how to
format the merge message based entirely on an option, not at all on
whether you use pull or fetch+merge.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-23 3:43 ` Daniel Barkalow
@ 2009-10-24 0:49 ` Jeff King
2009-10-24 1:22 ` Junio C Hamano
0 siblings, 1 reply; 21+ messages in thread
From: Jeff King @ 2009-10-24 0:49 UTC (permalink / raw
To: Daniel Barkalow; +Cc: Thomas Rast, Björn Steinbrink, Sean Estabrooks, git
On Thu, Oct 22, 2009 at 11:43:05PM -0400, Daniel Barkalow wrote:
> > But probably that is an artifact of the workflow. The scenario I am
> > describing above implies a somewhat centralized workflow, where the
> > shorthand contains all of the interesting information. In a totally
> > distributed, we-don't-share-anything-except-the-url-namespace setup of
> > an open source repo, the full URL makes more sense.
> >
> > So maybe it is something that should be optional.
>
> Surely you ought to be able to get the short form with "pull", though, if
> you happen to like short forms. So it would make sense to decide how to
> format the merge message based entirely on an option, not at all on
> whether you use pull or fetch+merge.
Yeah, I think you are right. It _should_ be variable, but right now it
varies on something totally unrelated to what you want (how you invoked,
and not what type of repo setup you are using). So I agree a patch to
make it more consistent across fetch+merge versus pull would be good,
and then we can make a configuration option to choose one or the other.
-Peff
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-24 0:49 ` Jeff King
@ 2009-10-24 1:22 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2009-10-24 1:22 UTC (permalink / raw
To: Jeff King
Cc: Daniel Barkalow, Thomas Rast, Björn Steinbrink,
Sean Estabrooks, git
Jeff King <peff@peff.net> writes:
> Yeah, I think you are right. It _should_ be variable, but right now it
> varies on something totally unrelated to what you want (how you invoked,
> and not what type of repo setup you are using). So I agree a patch to
> make it more consistent across fetch+merge versus pull would be good,
> and then we can make a configuration option to choose one or the other.
I have been looong wondering why I somehow thught that I saw a patch that makes
$ git merge origin/topic
pretend as if you did
$ git pull origin topic
when you have this mapping
[remote "origin"]
fetch = refs/heads/*:refs/remotes/origin/*
in your configuration file, since it is a very obvious thing to do...
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 18:23 [PATCH] pull: refuse complete src:dst fetchspec arguments Thomas Rast
` (2 preceding siblings ...)
2009-10-20 20:30 ` Sean Estabrooks
@ 2009-11-15 12:24 ` Thomas Rast
2009-11-15 20:22 ` Junio C Hamano
2009-12-29 11:05 ` Nanako Shiraishi
4 siblings, 1 reply; 21+ messages in thread
From: Thomas Rast @ 2009-11-15 12:24 UTC (permalink / raw
To: git
Thomas Rast wrote:
> git-pull has historically accepted full fetchspecs, meaning that you
> could do
>
> git pull $repo A:B
>
> which would simultaneously fetch the remote branch A into the local
> branch B and merge B into HEAD. This got especially confusing if B
> was checked out. New users variously mistook pull for fetch or read
> that command as "merge the remote A into my B", neither of which is
> correct.
It gets worse. *Much* worse.
Yesterday on IRC I helped 'thrope' with the github pull requests
guide. This is a wiki page, but placed at a sufficiently prominent
URL to make it look like an authoritative guide to a new user.
http://github.com/guides/pull-requests
I have since replaced the part in question with one that is more in
line with what the tools actually do, but the bottom line of the old
version was basically
# You got a request to pull git://github.com/defunkt/grit.git master
# mojombo can add the defunkt repository as a remote source, create
# a new branch, and pull the defunkt repository contents into it
# like this:
$ git remote add -f defunkt git://github.com/defunkt/grit.git
$ git checkout -b defunkt/master # (1)
$ git pull defunkt master:4f0ea0c # (2)
# [...]
$ git commit # (3)
$ git checkout master
$ git merge defunkt/master # (4)
$ git push
Note that all but the first line and the numbers is literally
cut&pasted from the old version, which is still available at
http://github.com/guides/pull-requests/24
so you can see for yourself. Note that the lines (1) and (2) were
there even in version 3.
And as you can see, there are just so many things wrong with it:
(1) will actually create a new branch defunkt/master based on whatever
you happened to be on, making (4) merge something entirely different
than what the pull request was for.
(2) will pull defunkt's master into a local *branch* called 4f0ea0c
(in the guide this is actually the sha1 of defunkt's master, but who
knows), and then merge that into the local defunkt/master branch from
(1).
(3) shouldn't do anything at that point, but hell if I know how he got
the idea to commit there.
So this suggests several safety measures:
* Perhaps branch/checkout -b can refuse to create branches that
already exist with this exact name under remotes if that's the only
argument. I.e., in the above situation (1),
# refuse: remotes/defunkt/master exists
git checkout -b defunkt/master
git branch defunkt/master
# accept: obviously you're asking for trouble explicitly
git checkout -b defunkt/master defunkt/master
git branch defunkt/master defunkt/master
* Perhaps all branch-creating code could refuse to create branches
that have a name that is also a valid sha1 prefix of an existing
object? This would be fairly drastic if a user's language has many
words consisting only of [a-f], but on the other hand, the user can
hardly be helped by having something that looks like a sha1 resolve
to some *other* sha1.
* I ask you to reconsider this patch. For some reason, people read
things into pull with fetchspecs that are far from correct.
I haven't thought much about backwards compatibility yet, though, so
some of it may not be possible.
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-11-15 12:24 ` Thomas Rast
@ 2009-11-15 20:22 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2009-11-15 20:22 UTC (permalink / raw
To: Thomas Rast; +Cc: git
Thomas Rast <trast@student.ethz.ch> writes:
> Yesterday on IRC I helped 'thrope' with the github pull requests
> guide. This is a wiki page, but placed at a sufficiently prominent
> URL to make it look like an authoritative guide to a new user.
>
> http://github.com/guides/pull-requests
>
> I have since replaced the part in question ...
Thanks.
It is hard to control the quality of random third-party documents, and
such a help as yours is greatly appreciated.
A document with gross misinformation is much worse than not having it.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-10-20 18:23 [PATCH] pull: refuse complete src:dst fetchspec arguments Thomas Rast
` (3 preceding siblings ...)
2009-11-15 12:24 ` Thomas Rast
@ 2009-12-29 11:05 ` Nanako Shiraishi
2009-12-29 16:58 ` Junio C Hamano
4 siblings, 1 reply; 21+ messages in thread
From: Nanako Shiraishi @ 2009-12-29 11:05 UTC (permalink / raw
To: Junio C Hamano; +Cc: Thomas Rast, git
Junio, could you tell us what happened to this thread?
The patch rejects "git pull repo A:B" because it is almost always a mistake;
I think it makes sense.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH] pull: refuse complete src:dst fetchspec arguments
2009-12-29 11:05 ` Nanako Shiraishi
@ 2009-12-29 16:58 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2009-12-29 16:58 UTC (permalink / raw
To: Nanako Shiraishi; +Cc: Thomas Rast, git
Nanako Shiraishi <nanako3@lavabit.com> writes:
> Junio, could you tell us what happened to this thread?
>
> The patch rejects "git pull repo A:B" because it is almost always a mistake;
> I think it makes sense.
It seems that we got sidetracked into a long thread on different (but
interesting) side topics. I think what the patch attempts to do is quite
sane, and the implementation is very straightforward. Perhaps we should
resurrect it, but with a proper "declare deprecation now, first warn then
refuse in two releases" steps. I.e. the actual refusal would happen in
1.7.1 or later.
One side topic was Daniel wondering if we should restrict the value of B
for "git fetch repo A:B" so that "fetch" is not used to update the refs
outside refs/remotes namespace. I personally think it is an unwarranted
restriction, and also it is more or less an unrelated issue anyway.
Another side topic that distracted us was about the difference between the
autogenerated commit log messages for "git pull origin topic" and "git
fetch && git merge origin/topic". I personally think it is very good that
the latter already says "Merge remote branch 'origin/topic'" and there is
no need to turn that into "Merge 'topic' of ..." (actually I'd prefer it
the way it is).
^ permalink raw reply [flat|nested] 21+ messages in thread
end of thread, other threads:[~2009-12-29 16:59 UTC | newest]
Thread overview: 21+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-10-20 18:23 [PATCH] pull: refuse complete src:dst fetchspec arguments Thomas Rast
2009-10-20 18:37 ` [RFC! PATCH] " Thomas Rast
2009-10-20 19:29 ` [PATCH] " Wesley J. Landaker
2009-10-20 20:30 ` Sean Estabrooks
2009-10-20 21:11 ` Junio C Hamano
2009-10-21 0:15 ` Daniel Barkalow
2009-10-21 0:29 ` Sean Estabrooks
2009-10-21 0:55 ` Daniel Barkalow
2009-10-21 1:35 ` Sean Estabrooks
2009-10-21 3:15 ` Björn Steinbrink
2009-10-21 4:32 ` Daniel Barkalow
2009-10-21 8:05 ` Thomas Rast
2009-10-23 2:54 ` Jeff King
2009-10-23 3:43 ` Daniel Barkalow
2009-10-24 0:49 ` Jeff King
2009-10-24 1:22 ` Junio C Hamano
2009-10-21 8:06 ` Thomas Rast
2009-11-15 12:24 ` Thomas Rast
2009-11-15 20:22 ` Junio C Hamano
2009-12-29 11:05 ` Nanako Shiraishi
2009-12-29 16:58 ` Junio C Hamano
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).