Minor annoyance with git push

Minor annoyance with git push

[Martin Langhoff]

I have a minor UI issue with git pull. First, a bit of background: we
run a "central team repo" development model -- and we track on one
repo the "main" branches, and the client branches, where we do minor
customisations and sometimes client-funded feature work that is later
cherry-picked for the "main" branches. This is with a team of ~10
people, and lots of clients.

(To clarify: some clients are in specialised private repositories.
Most are happy and actually request that our work should be public.)

As we run a central repo, we all get *all* the branches when we do
fetch. A bit noisy, but no major issue. It is also a great thing as we
get asked to help in various branches, so I'll often hop on a client
branch that is mainly maintained by someone else, just to fix or
enhance something on the authentication (which I specialise in). After
that, I don't have much to do with that client branch.

This means that beyond the branches I actively work on, I also have
local tracking branches for remote heads that I am not updating. When
I say git push, these stale local tracking branches are making a lot
of noise in the output:

To git+ssh://git.catalyst.net.nz/var/git/moodle-r2.git
 ! [rejected]        mdl17-ceo -> mdl17-ceo (non-fast forward)
 ! [rejected]        mdl18-local -> mdl18-local (non-fast forward)
 ! [rejected]        mdl18-masseyedu-wimbatest ->
mdl18-masseyedu-wimbatest (non-fast forward)
 ! [rejected]        mdl18-nmit -> mdl18-nmit (non-fast forward)
 ! [rejected]        mdl18-proxy -> mdl18-proxy (non-fast forward)
 ! [rejected]        mdl18-shared -> mdl18-shared (non-fast forward)
 ! [rejected]        mdl18-sqm -> mdl18-sqm (non-fast forward)
 ! [rejected]        mdl18-stcuthberts -> mdl18-stcuthberts (non-fast forward)
 ! [rejected]        mdl18-topnz -> mdl18-topnz (non-fast forward)
 ! [rejected]        mdl19-dbperf -> mdl19-dbperf (non-fast forward)
 ! [rejected]        mdl19-ucol -> mdl19-ucol (non-fast forward)
 ! [rejected]        mdl19-uow -> mdl19-uow (non-fast forward)
error: failed to push to 'git+ssh://git.catalyst.net.nz/var/git/moodle-r2.git'

The error messages ("! rejected", "error: failed to push") appear even
if one or two branches did get pushed... I think they are a bit over
the top. None of these "rejected" branches have anything _new_, they
are just stale. Nothing new to say. Can we just ignore them, and only
say "ZOMG Failure! Rejected!!1!!" if we fail to push *new* local
commits that aren't in the repo (leading to an assumption that if the
user said "push" he'd expect those new local commits to be pushed to
the server)?

[ Personally, it doesn't bother me too much. But I can see some
newcomers to my team, and their eyes twitch when they see all the
exclamation marks. I've learned to look for the branch I care about
being pushed in the output, but it's particularly not user friendly as
it stands. ]

cheers,



m

Re: Minor annoyance with git push

[Martin Langhoff]

On Feb 8, 2008 5:44 PM, Martin Langhoff <martin.langhoff@gmail.com> wrote:
>  None of these "rejected" branches have anything _new_, they
> are just stale. Nothing new to say.

And I guess the natural follow up question is: would it make sense to
tell git pull to do a "merge" for not-checked-out branches where we
can safely tell that the resulting "merge" will actually be a
fast-forward?

Would that be unsafe in any way?

Because when I "git checkout bla-stale-branch" to help a fellow
developer again, I have to remember to "git merge
origin/bla-stale-branch" to get a much needed fast-forward before
starting to work.

[ Or we could be more proactive at deleting unused local heads. But
that's a bit of silly maintenance just to keep things tidy, that git
could keep tidy ;-) ... ]

cheers,


m

Re: Minor annoyance with git push

[Junio C Hamano]

"Martin Langhoff" <martin.langhoff@gmail.com> writes:

> On Feb 8, 2008 5:44 PM, Martin Langhoff <martin.langhoff@gmail.com> wrote:
>>  None of these "rejected" branches have anything _new_, they
>> are just stale. Nothing new to say.
>
> And I guess the natural follow up question is: would it make sense to
> tell git pull to do a "merge" for not-checked-out branches where we
> can safely tell that the resulting "merge" will actually be a
> fast-forward?
>
> Would that be unsafe in any way?

Not "unsafe" in the sense that you would not be losing commit
objects, but I'd feel uneasy about that.  The fact that the
branch tip was pointing at an older commit gets lost, and in
your workflow that information is useless, but that does not
necessarily mean it is useless for everybody.

> Because when I "git checkout bla-stale-branch" to help a fellow
> developer again, I have to remember to "git merge
> origin/bla-stale-branch" to get a much needed fast-forward before
> starting to work.

Perhaps it might make sense to have a checkout hook that notices
the branch that is being checked out is meant to build on top of
a corresponding remote tracking branch, and performs the
necessary fast-forward when that is the case.

> [ Or we could be more proactive at deleting unused local heads. But
> that's a bit of silly maintenance just to keep things tidy, that git
> could keep tidy ;-) ... ]

Well, git couldn't, unless you tell it "I've pushed out my work,
and I am done with helping with this client branch for now", and
the way for you to do so is "git branch -d".

Perhaps "git branch -d" should be taught to check if the tip of
the deleted branch is part of the corresponding remote tracking
branch, when "one local branch to track one remote branch" model
is used.  Its safety to forbid deleting the branch unless it is
an ancestor of the "current" branch would not make sense in such
a workflow.

Re: Minor annoyance with git push

[Steffen Prohaska]

On Feb 8, 2008, at 8:48 AM, Junio C Hamano wrote:

> "Martin Langhoff" <martin.langhoff@gmail.com> writes:
>
>> Because when I "git checkout bla-stale-branch" to help a fellow
>> developer again, I have to remember to "git merge
>> origin/bla-stale-branch" to get a much needed fast-forward before
>> starting to work.
>
> Perhaps it might make sense to have a checkout hook that notices
> the branch that is being checked out is meant to build on top of
> a corresponding remote tracking branch, and performs the
> necessary fast-forward when that is the case.

Or just print a warning that there are new commits on the
tracked branch and leave the decision how to proceed to
the user?

	Steffen

Re: Minor annoyance with git push

[Junio C Hamano]

Steffen Prohaska <prohaska@zib.de> writes:

> On Feb 8, 2008, at 8:48 AM, Junio C Hamano wrote:
>
>> Perhaps it might make sense to have a checkout hook that notices
>> the branch that is being checked out is meant to build on top of
>> a corresponding remote tracking branch, and performs the
>> necessary fast-forward when that is the case.
>
> Or just print a warning that there are new commits on the
> tracked branch and leave the decision how to proceed to
> the user?

Yeah, that would be even safer.  And I do not mind if the "git
checkout" learned to do so natively without needing of such a
hook.

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sat, 9 Feb 2008, Junio C Hamano wrote:

> Steffen Prohaska <prohaska@zib.de> writes:
> 
> > On Feb 8, 2008, at 8:48 AM, Junio C Hamano wrote:
> >
> >> Perhaps it might make sense to have a checkout hook that notices
> >> the branch that is being checked out is meant to build on top of
> >> a corresponding remote tracking branch, and performs the
> >> necessary fast-forward when that is the case.
> >
> > Or just print a warning that there are new commits on the
> > tracked branch and leave the decision how to proceed to
> > the user?
> 
> Yeah, that would be even safer.  And I do not mind if the "git
> checkout" learned to do so natively without needing of such a
> hook.

Heh, should be easier when we have builtin git-checkout... which reminds 
me that I have a patch series to review.  ;-)

Ciao,
Dscho

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Martin Langhoff wrote:

> On Feb 8, 2008 5:44 PM, Martin Langhoff <martin.langhoff@gmail.com> wrote:
> >  None of these "rejected" branches have anything _new_, they are just 
> > stale. Nothing new to say.
> 
> And I guess the natural follow up question is: would it make sense to 
> tell git pull to do a "merge" for not-checked-out branches where we can 
> safely tell that the resulting "merge" will actually be a fast-forward?

That question comes up pretty often, I think.  But you need a working 
directory to resolve conflicts for merges.  You only have one, though.

So no, I think it is saner to have tracking branches (which are updated 
anyway), and local branches.  And once you're done with a branch, you 
simply push it, and then delete it (you will still have the result in the 
tracking branch).

Ciao,
Dscho

Re: Minor annoyance with git push

[Martin Langhoff]

On Feb 9, 2008 12:52 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> That question comes up pretty often, I think.  But you need a working
> directory to resolve conflicts for merges.  You only have one, though.

Once all the remote refs are fetched, it is trivial to determine that
it is just a fast-forward, therefore _no_ merge and no chance for
conflicts...

cheers,


m

Re: Minor annoyance with git push

[Mike Hommey]

On Sat, Feb 09, 2008 at 11:23:30AM +1300, Martin Langhoff wrote:
> On Feb 9, 2008 12:52 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > That question comes up pretty often, I think.  But you need a working
> > directory to resolve conflicts for merges.  You only have one, though.
> 
> Once all the remote refs are fetched, it is trivial to determine that
> it is just a fast-forward, therefore _no_ merge and no chance for
> conflicts...

Even with merges, it could be helpful to have a tool that would
automagically stash, checkout the appropriate branch, merge (and deal
with conflicts if necessary), and so on.

Mike

Re: Minor annoyance with git push

[Sean]

On Fri, 8 Feb 2008 17:44:12 +1300
"Martin Langhoff" <martin.langhoff@gmail.com> wrote:

> This means that beyond the branches I actively work on, I also have
> local tracking branches for remote heads that I am not updating. When
> I say git push, these stale local tracking branches are making a lot
> of noise in the output:

There may be other workflows where the noise in the output is appropriate.
What about using "git push origin HEAD" (or an alias for it) to push only
the branch you have checked out and avoid noise for other branches?

Sean

Re: Minor annoyance with git push

[Steffen Prohaska]

On Feb 8, 2008, at 6:38 AM, Sean wrote:

> On Fri, 8 Feb 2008 17:44:12 +1300
> "Martin Langhoff" <martin.langhoff@gmail.com> wrote:
>
>> This means that beyond the branches I actively work on, I also have
>> local tracking branches for remote heads that I am not updating. When
>> I say git push, these stale local tracking branches are making a lot
>> of noise in the output:
>
> There may be other workflows where the noise in the output is  
> appropriate.
> What about using "git push origin HEAD" (or an alias for it) to  
> push only
> the branch you have checked out and avoid noise for other branches?


This is what I often do and I also tell my users from
the very beginning to *avoid* a naked "git push".
Instead, they always should say exactly what they mean,
like "git push origin topic", and they can use
"git push origin HEAD" as a short-hand.  Besides, they
should also run with "--dry-run" first to verify what
they do.

We had lengthy discussions about the issue Martin describes.  My
personal conclusion was that people on the list tend to regard
the current behaviour of "git push" as a very stable feature.  So
you should come up with convincing arguments for changes and
you also should ensure that the current behaviour does not break.
I decided to focus on different things and leave "git push" as is.

Here are some pointers to the discussions:

http://marc.info/?l=git&m=119384331712996&w=2

http://marc.info/?l=git&m=119400354601328&w=2

http://thread.gmane.org/gmane.comp.version-control.git/65632/focus=65747

http://thread.gmane.org/gmane.comp.version-control.git/61955/focus=65493

	Steffen

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Martin Langhoff wrote:

> To git+ssh://git.catalyst.net.nz/var/git/moodle-r2.git
>  ! [rejected]        mdl17-ceo -> mdl17-ceo (non-fast forward)
>  ! [rejected]        mdl18-local -> mdl18-local (non-fast forward)
>  ! [rejected]        mdl18-masseyedu-wimbatest ->
> mdl18-masseyedu-wimbatest (non-fast forward)
>  ! [rejected]        mdl18-nmit -> mdl18-nmit (non-fast forward)
>  ! [rejected]        mdl18-proxy -> mdl18-proxy (non-fast forward)
>  ! [rejected]        mdl18-shared -> mdl18-shared (non-fast forward)
>  ! [rejected]        mdl18-sqm -> mdl18-sqm (non-fast forward)
>  ! [rejected]        mdl18-stcuthberts -> mdl18-stcuthberts (non-fast forward)
>  ! [rejected]        mdl18-topnz -> mdl18-topnz (non-fast forward)
>  ! [rejected]        mdl19-dbperf -> mdl19-dbperf (non-fast forward)
>  ! [rejected]        mdl19-ucol -> mdl19-ucol (non-fast forward)
>  ! [rejected]        mdl19-uow -> mdl19-uow (non-fast forward)
> error: failed to push to 'git+ssh://git.catalyst.net.nz/var/git/moodle-r2.git'
> 
> The error messages ("! rejected", "error: failed to push") appear even
> if one or two branches did get pushed... I think they are a bit over
> the top. None of these "rejected" branches have anything _new_, they
> are just stale.

So you're saying that the local sides' refs are ancestors of the remote 
sides' refs?

The problem is that the local side cannot tell, and we try to avoid 
putting load on the server, because in many cases, there will be one 
central server and many clients.

So I think this is not technically feasible.  Or do you have another idea 
how to find out that the "[rejected]" ref is a stale ref (i.e. an ancestor 
of the remote side) as opposed to properly rejected?

Another way to "solve" this issue, of course, is to use the remote layout.  
I did the switchover myself some time ago; it was hard at first, since I 
was so used to just check out the branches I just fetched.  But in the 
long run the distinction between local and tracking branches made life 
much easier for me.

Related is this idea that I did not really follow up: often, you want to 
work on a branch which you are tracking already, but there is no local 
branch.  And most often, you just want to create a local branch of the 
same name.  So maybe we should just introduce a new flag, like

	$ git checkout -c origin/next

which would create (or fast-forward) the local branch "next" to what's in 
origin/next.  The long option would read --create-local.

In the same vein, maybe "git branch -d next" should be taught to look at 
the remote branches of name "<nick>/next", too, when checking if that ref 
is an ancestor of HEAD?

Ciao,
Dscho

Re: Minor annoyance with git push

[Martin Langhoff]

On Feb 9, 2008 12:50 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> So you're saying that the local sides' refs are ancestors of the remote
> sides' refs?

Exactly. And we could show a nicer message - rejected is too strong a word ;-)

> The problem is that the local side cannot tell

The local side has the remote refs if the client has fetched recently,
so it might be able to tell in some cases. Not with authority (things
may have changed on the server side...) but the client might be able
to say something less alarming.

> Another way to "solve" this issue, of course, is to use the remote layout.
> I did the switchover myself some time ago; it was hard at first, since I
> was so used to just check out the branches I just fetched.  But in the
> long run the distinction between local and tracking branches made life
> much easier for me.

What do you mean with "the remote layout"? I am using
"remotes"+tracking branches as far as I can tell...

cheers,


m

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sat, 9 Feb 2008, Martin Langhoff wrote:

> On Feb 9, 2008 12:50 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> 
> > The problem is that the local side cannot tell
> 
> The local side has the remote refs if the client has fetched recently, 
> so it might be able to tell in some cases. Not with authority (things 
> may have changed on the server side...) but the client might be able to 
> say something less alarming.

But if it was not fetched recently?  I think that what you suggest is too 
tricky (IOW too prone to break).

> > Another way to "solve" this issue, of course, is to use the remote 
> > layout. I did the switchover myself some time ago; it was hard at 
> > first, since I was so used to just check out the branches I just 
> > fetched.  But in the long run the distinction between local and 
> > tracking branches made life much easier for me.
> 
> What do you mean with "the remote layout"? I am using "remotes"+tracking 
> branches as far as I can tell...

I mean keeping most branches purely as tracking branches.  Whenever you 
are done with one branch, you delete the local branch.

Ciao,
Dscho

Re: Minor annoyance with git push

[Jeff King]

On Sat, Feb 09, 2008 at 11:27:51AM +1300, Martin Langhoff wrote:

> Exactly. And we could show a nicer message - rejected is too strong a
> word ;-)

Like

diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 454ad8f..3979918 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -315,7 +315,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count)
 				ref->peer_ref, NULL);
 		break;
 	case REF_STATUS_REJECT_NONFASTFORWARD:
-		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+		print_ref_status('!', "[kindly refused]", ref, ref->peer_ref,
 				"non-fast forward");
 		break;
 	case REF_STATUS_REMOTE_REJECT:

?

> The local side has the remote refs if the client has fetched recently,
> so it might be able to tell in some cases. Not with authority (things
> may have changed on the server side...) but the client might be able
> to say something less alarming.

This is actually not that hard to do in the case that we can. Patch will
follow in a second, though I am not sure it is a good idea (because it
silently ignores pushing rewinds).

> > Another way to "solve" this issue, of course, is to use the remote layout.
> > I did the switchover myself some time ago; it was hard at first, since I
> > was so used to just check out the branches I just fetched.  But in the
> > long run the distinction between local and tracking branches made life
> > much easier for me.
> 
> What do you mean with "the remote layout"? I am using
> "remotes"+tracking branches as far as I can tell...

I think he means something like "if I have 'next' and 'origin/next',
then I should check whether 'next' is a subset of 'origin/next'" and
just say "nothing to send." But that suffers from the same "silently
ignoring rewinds" as above. You could ignore the push if you have
next exactly equal to origin/next, but that implies that you haven't done
any fetching (which is unlikely in the scenario you described).

-Peff

Re: Minor annoyance with git push

[Jeff King]

On Fri, Feb 08, 2008 at 09:46:36PM -0500, Jeff King wrote:

> > The local side has the remote refs if the client has fetched recently,
> > so it might be able to tell in some cases. Not with authority (things
> > may have changed on the server side...) but the client might be able
> > to say something less alarming.
> 
> This is actually not that hard to do in the case that we can. Patch will
> follow in a second, though I am not sure it is a good idea (because it
> silently ignores pushing rewinds).

And here it is. Again, I don't think this is the right default behavior.
I'm not even sure it is a good idea as configurable behavior. But it's
here for comment and for playing with, nonetheless.

-- >8 --
send-pack: treat strict rewinds specially

If you try to push an out-of-date version of a branch, it will generally
be rejected as a non-fastforward. This can clutter up the status output
of git-push if you have many such branches.

Instead, let's check explicitly whether what we are pushing is a strict
subset of what the remote already has. We treat this as a non-error, and
don't even print any status unless "verbose" is given. If the push is
"forced", then we will push as usual.

Note that we cannot always perform this check accurately; it relies on
us having the commit object that the remote claims to have. In the case
that we don't, we treat it like an ordinary non-fastforward and reject.

---
diff --git a/cache.h b/cache.h
index 920e731..78fee0b 100644
--- a/cache.h
+++ b/cache.h
@@ -596,6 +596,7 @@ struct ref {
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
 		REF_STATUS_EXPECTING_REPORT,
+		REF_STATUS_REJECT_REWIND,
 	} status;
 	char *remote_status;
 	struct ref *peer_ref; /* when renaming */
diff --git a/builtin-send-pack.c b/builtin-send-pack.c
index 454ad8f..9f82b83 100644
--- a/builtin-send-pack.c
+++ b/builtin-send-pack.c
@@ -315,6 +315,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count)
 				ref->peer_ref, NULL);
 		break;
 	case REF_STATUS_REJECT_NONFASTFORWARD:
+	case REF_STATUS_REJECT_REWIND:
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 				"non-fast forward");
 		break;
@@ -343,7 +344,8 @@ static void print_push_status(const char *dest, struct ref *refs)
 
 	if (args.verbose) {
 		for (ref = refs; ref; ref = ref->next)
-			if (ref->status == REF_STATUS_UPTODATE)
+			if (ref->status == REF_STATUS_UPTODATE ||
+			    ref->status == REF_STATUS_REJECT_REWIND)
 				n += print_one_push_status(ref, dest, n);
 	}
 
@@ -354,7 +356,8 @@ static void print_push_status(const char *dest, struct ref *refs)
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
-		    ref->status != REF_STATUS_OK)
+		    ref->status != REF_STATUS_OK &&
+		    ref->status != REF_STATUS_REJECT_REWIND)
 			n += print_one_push_status(ref, dest, n);
 	}
 }
@@ -365,6 +368,7 @@ static int refs_pushed(struct ref *ref)
 		switch(ref->status) {
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
+		case REF_STATUS_REJECT_REWIND:
 			break;
 		default:
 			return 1;
@@ -463,8 +467,12 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		    (!has_sha1_file(ref->old_sha1)
 		      || !ref_newer(new_sha1, ref->old_sha1));
 
+
 		if (ref->nonfastforward && !ref->force && !args.force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+			if (ref_newer(ref->old_sha1, new_sha1))
+				ref->status = REF_STATUS_REJECT_REWIND;
+			else
+				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
 			continue;
 		}
 
@@ -522,6 +530,7 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
 		case REF_STATUS_NONE:
 		case REF_STATUS_UPTODATE:
 		case REF_STATUS_OK:
+		case REF_STATUS_REJECT_REWIND:
 			break;
 		default:
 			return -1;

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Fri, 8 Feb 2008, Jeff King wrote:

> On Fri, Feb 08, 2008 at 09:46:36PM -0500, Jeff King wrote:
> 
> > > The local side has the remote refs if the client has fetched recently,
> > > so it might be able to tell in some cases. Not with authority (things
> > > may have changed on the server side...) but the client might be able
> > > to say something less alarming.
> > 
> > This is actually not that hard to do in the case that we can. Patch will
> > follow in a second, though I am not sure it is a good idea (because it
> > silently ignores pushing rewinds).
> 
> And here it is. Again, I don't think this is the right default behavior.
> I'm not even sure it is a good idea as configurable behavior. But it's
> here for comment and for playing with, nonetheless.

I was already trying to make a patch on top of yours which says "[stale]" 
instead of "[rejected]" for those cases, but then I realised that 2 tests 
in t5400 fail.

> @@ -463,8 +467,12 @@ static int do_send_pack(int in, int out, struct remote *remote, const char *dest
>  		    (!has_sha1_file(ref->old_sha1)
>  		      || !ref_newer(new_sha1, ref->old_sha1));
>  
> +
>  		if (ref->nonfastforward && !ref->force && !args.force_update) {
> -			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
> +			if (ref_newer(ref->old_sha1, new_sha1))
> +				ref->status = REF_STATUS_REJECT_REWIND;
> +			else
> +				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;

Indeed.  I did not think it was that easy, but apparently it is.

Thanks,
Dscho

Re: Minor annoyance with git push

[Jeff King]

On Sat, Feb 09, 2008 at 01:04:10PM +0000, Johannes Schindelin wrote:

> I was already trying to make a patch on top of yours which says "[stale]" 
> instead of "[rejected]" for those cases, but then I realised that 2 tests 
> in t5400 fail.

I think the problem is that tests 7/8 in t5400 actually try to create
a non-ff situation by doing a rewind. So it is not a bug in the new code
so much as the test relied on the very behavior we changed.

> > -			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
> > +			if (ref_newer(ref->old_sha1, new_sha1))
> > +				ref->status = REF_STATUS_REJECT_REWIND;
> > +			else
> > +				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
> 
> Indeed.  I did not think it was that easy, but apparently it is.

It's also slower than it needs to be, since we also do a ref_newer in
the other direction, and because non-ff cases traverse all the way to
the root.  I think you could do better to write a function (similar to
limit_list) which returned one of { A is an ancestor of B, B is an
ancestor of A, A and B are equal, A and B are not directly related },
and you would have to traverse down only to the nearest merge base.

-Peff

Re: Minor annoyance with git push

[Steffen Prohaska]

On Feb 9, 2008, at 3:46 AM, Jeff King wrote:

>>> Another way to "solve" this issue, of course, is to use the  
>>> remote layout.
>>> I did the switchover myself some time ago; it was hard at first,  
>>> since I
>>> was so used to just check out the branches I just fetched.  But  
>>> in the
>>> long run the distinction between local and tracking branches made  
>>> life
>>> much easier for me.
>>
>> What do you mean with "the remote layout"? I am using
>> "remotes"+tracking branches as far as I can tell...
>
> I think he means something like "if I have 'next' and 'origin/next',
> then I should check whether 'next' is a subset of 'origin/next'" and
> just say "nothing to send." But that suffers from the same "silently
> ignoring rewinds" as above. You could ignore the push if you have
> next exactly equal to origin/next, but that implies that you  
> haven't done
> any fetching (which is unlikely in the scenario you described).


A similar proposal was discussed last October/November and we
already had patches.  We decided against it.  The reason is
that the post-condition of "git push" is that all heads that
are considered by the push shall be identical locally and
at the remote.  If "git push" does not achieve this it should
report the branches for which the post-condition is not met.

I think Dscho means: do not duplicate remote branches as local
branches.  Keep the number of local branches as low as possible.
Only if you want to work on a remote branch, create a local
branch, commit you work, push, and *delete* the local branch.

However, I often use local branches as a reminder which remote
branches I am actively monitoring.  If I start to work on them
I first review what's new with "gitk origin/topic --not topic"
and then do the fast forward "git push . origin/topic:topic"
before I actually start working "git checkout topic". For
these branches, Dschos proposal does not work for me.

	Steffen

Re: Minor annoyance with git push

[Jeff King]

On Fri, Feb 08, 2008 at 05:44:12PM +1300, Martin Langhoff wrote:

> This means that beyond the branches I actively work on, I also have
> local tracking branches for remote heads that I am not updating. When
> I say git push, these stale local tracking branches are making a lot
> of noise in the output:
> 
> To git+ssh://git.catalyst.net.nz/var/git/moodle-r2.git
>  ! [rejected]        mdl17-ceo -> mdl17-ceo (non-fast forward)
>  ! [rejected]        mdl18-local -> mdl18-local (non-fast forward)
>  ! [rejected]        mdl18-masseyedu-wimbatest ->
> mdl18-masseyedu-wimbatest (non-fast forward)
>  ! [rejected]        mdl18-nmit -> mdl18-nmit (non-fast forward)
>  ! [rejected]        mdl18-proxy -> mdl18-proxy (non-fast forward)
>  ! [rejected]        mdl18-shared -> mdl18-shared (non-fast forward)
>  ! [rejected]        mdl18-sqm -> mdl18-sqm (non-fast forward)
>  ! [rejected]        mdl18-stcuthberts -> mdl18-stcuthberts (non-fast forward)
>  ! [rejected]        mdl18-topnz -> mdl18-topnz (non-fast forward)
>  ! [rejected]        mdl19-dbperf -> mdl19-dbperf (non-fast forward)
>  ! [rejected]        mdl19-ucol -> mdl19-ucol (non-fast forward)
>  ! [rejected]        mdl19-uow -> mdl19-uow (non-fast forward)
> error: failed to push to 'git+ssh://git.catalyst.net.nz/var/git/moodle-r2.git'

Thinking about this more, this situation is more than a minor annoyance:
it is actually somewhat dangerous. If you ever wanted to push _one_
non-ff case (say, for your current branch) and you were to use "git push
-f", you would rewind history for random branches, and sorting the mess
out at the remote could be awful (especially if it is a bare repo
without reflogs).

I really think that Steffen's "default to pushing only the current
branch" approach fits much better with the model described in your
workflow, and is generally a safer default. IIRC, the main objection was
that old-timers like the current push behavior better. Steffen, was
there objection to a "push.onlyHEAD" config option?

-Peff

Re: Minor annoyance with git push

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Thinking about this more, this situation is more than a minor annoyance:
> it is actually somewhat dangerous. If you ever wanted to push _one_
> non-ff case (say, for your current branch) and you were to use "git push
> -f", you would rewind history for random branches, and sorting the mess
> out at the remote could be awful (especially if it is a bare repo
> without reflogs).

Yeah, -f using "matching refs" is dangerous, but on the other
hand, that would be how you correct that mistake in one shot,
after you fixed the mistake locally.

Is there anything wrong with "git push $there $branch_name"?  I
thought we discussed this last time and there was even a patch
that does "git push $there HEAD" to push out the current branch,
which I am fairly sure that I accepted (but I do not remember,
as I do not use such a shorthand. When I want to push a single
branch, I _want_ to be explicit, to make sure I push out the
right thing).

So after doing a fix on a single branch, you would:

	$ git push origin HEAD

and you are done.  No need to spell out the long branch name you
are currently on.

I do not know if this was part of the last round of patches, but
I suspect it is not a problem to allow

	$ git push HEAD

if it is unambiguous.  That is, "HEAD?  Do we have such a remote
nickname?  No.  Then can we default to 'origin' and use it as
the ref to push?  Yeah, we can, so the user meant 'git push
origin HEAD'".

Re: Minor annoyance with git push

[Jeff King]

On Fri, Feb 08, 2008 at 07:24:06PM -0800, Junio C Hamano wrote:

> Yeah, -f using "matching refs" is dangerous, but on the other
> hand, that would be how you correct that mistake in one shot,
> after you fixed the mistake locally.

Not necessarily; you might not even have the refs that you just rewound.

> Is there anything wrong with "git push $there $branch_name"?  I

I think only that:

  - it's more typing, so if there is a class of users who want to use
    "git push origin HEAD" as their workflow, it might make sense to
    support them with a config option

  - I have seen less experienced users expecting "git push" to do a
    single branch (i.e., users whose workflow is "git pull; hack hack
    hack; git push").
    With one branch it is not a problem, but as soon as you introduce
    them to other branches, then the push behavior is yet another thing
    that you have to explain. And I know "clueless users" is not a good
    reason to change defaults if it makes clueful users less happy, but
    it might be reason for a config option.

> thought we discussed this last time and there was even a patch
> that does "git push $there HEAD" to push out the current branch,

Ah, I forgot about that. Yes, that does work in 1.5.4.

> I do not know if this was part of the last round of patches, but
> I suspect it is not a problem to allow
> 
> 	$ git push HEAD
> 
> if it is unambiguous.  That is, "HEAD?  Do we have such a remote
> nickname?  No.  Then can we default to 'origin' and use it as
> the ref to push?  Yeah, we can, so the user meant 'git push
> origin HEAD'".

I have often wanted that, since I typically only _ever_ push to an
origin, but the DWIM made me paranoid that it could be unsafe. If you
had a remote and a branch name that were similar, then a typo could end
up pushing to the wrong place.

-Peff

Re: Minor annoyance with git push

[Martin Langhoff]

On Feb 9, 2008 4:24 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Is there anything wrong with "git push $there $branch_name"?

This works, but is rather verbose to type all the time.

>         $ git push origin HEAD
>
> and you are done.  No need to spell out the long branch name you
> are currently on.

Didn't know this was meant to work. I'll give it a go.

> I do not know if this was part of the last round of patches, but
> I suspect it is not a problem to allow
>
>         $ git push HEAD
>
> if it is unambiguous.  That is, "HEAD?  Do we have such a remote
> nickname?  No.  Then can we default to 'origin' and use it as
> the ref to push?  Yeah, we can, so the user meant 'git push
> origin HEAD'".

If I can say git push HEAD it will be nice.

Still, the big fat ![rejected] do seem over the top when I know it
really means "stale".

And I don't completely follow how bad the impact of
auto-fast-forwarding local tracking branches on a merge. If it's a
fast-forward, my "local state" wasn't that exciting to begin with ;-)
and revlogs can potentially rescue my olden state (but what's the use
case for the local state being interesting, anyway?). Yes - user state
is important, but something that resolves to a fast-forward means that
the user state, whatever it is, is in sync with the repo.

As per the subject, these are minor annoyances. The whole
remotes+local heads setup works like a charm ;-)

cheers,



m

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sun, 10 Feb 2008, Martin Langhoff wrote:

> And I don't completely follow how bad the impact of
> auto-fast-forwarding local tracking branches on a merge. If it's a
> fast-forward, my "local state" wasn't that exciting to begin with ;-)

Not necessarily.  Maybe you wanted to work on it, kept your local branch 
as a reminder.  Maybe even reset --hard to a known-bad commit.

The thing is: local is local is local.  If you muddy the clear 
distinctions, you will make semantics much harder to grasp.

Having said that, you _could_ introduce a command line option 
"--ff-local-branches", which would keep the semantics clean.

Ciao,
Dscho

Re: Minor annoyance with git push

[Junio C Hamano]

"Martin Langhoff" <martin.langhoff@gmail.com> writes:

> Still, the big fat ![rejected] do seem over the top when I know it
> really means "stale".

If "stale" can be proven cheaply, I think it would be a very
good change to introduce [rejected] vs [stale].

> And I don't completely follow how bad the impact of
> auto-fast-forwarding local tracking branches on a merge. If it's a
> fast-forward, my "local state" wasn't that exciting to begin with ;-)

If your branch is configured to track the remote and when your
branch is behind, it probably is safe to assume that the user
most likely wants the ff merge to happen _when_ the branch is
checked out.  I am with you that.  I am not sure if that ff
should happen when you fetch from the other side as you suggest.

Doing so automatically means it would break workflows of people
who are deliberately _holding back_ from updating to the remote
they are tracking for whatever reason.  As you said, the point
they were holding back at can be found as _one_ of the reflog
entries even if you force the auto-ff upon fetch, but that does
mean you are forcing them to go and look for it, while they used
to be able to _rely on_ that their tips will not be molested
without them telling git to do so explicitly.

So I am with you that auto-ff would help more people but I am
not convinced it would not hurt anybody.

Perhaps making "git-checkout" to notice this and offer (or
suggest) fast-forwarding at that point may be safer and make
more sense.  You cannot grow your local branch unless you check
them out, and your remote tracking will keep growing without the
auto-ff you are suggesting, so it is not like people will lose
anchoring point to compare between branches if we do not
auto-ff.

Re: Minor annoyance with git push

[Jeff King]

On Sat, Feb 09, 2008 at 06:24:30PM -0800, Junio C Hamano wrote:

> > Still, the big fat ![rejected] do seem over the top when I know it
> > really means "stale".
> If "stale" can be proven cheaply, I think it would be a very
> good change to introduce [rejected] vs [stale].

I think there is still one problem with that: you are not splitting the
cases into "rejected" and "stale". You are splitting them into
"rejected, or we didn't have enough information to determine staleness"
and "definitely stale"[1]. So in the cases that it works perfectly, it may
be a useful distinction; but it might end up confusing people when the
same situation produces different results depending on what has been
fetched locally.

-Peff

[1]: Actually, you can further split into "definitely rejected",
"definitely stale", and "undetermined" but I don't think that is being
proposed.

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sun, 10 Feb 2008, Jeff King wrote:

> On Sat, Feb 09, 2008 at 06:24:30PM -0800, Junio C Hamano wrote:
> 
> > > Still, the big fat ![rejected] do seem over the top when I know it 
> > > really means "stale".
> >
> > If "stale" can be proven cheaply, I think it would be a very good 
> > change to introduce [rejected] vs [stale].
> 
> I think there is still one problem with that: you are not splitting the
> cases into "rejected" and "stale".

FWIW I think it is perfectly reasonable to say "stale" when you _know_ 
that it's stale, and "rejected" when you don't know the reason.

Ciao,
Dscho

[RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Junio C Hamano]

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

> Perhaps making "git-checkout" to notice this and offer (or
> suggest) fast-forwarding at that point may be safer and make
> more sense.  You cannot grow your local branch unless you check
> them out, and your remote tracking will keep growing without the
> auto-ff you are suggesting, so it is not like people will lose
> anchoring point to compare between branches if we do not
> auto-ff.

So I did this.

When you are switching to a branch that is marked to merge from
somewhere else, e.g. when you have:

    [branch "next"]
            remote = upstream
            merge = refs/heads/next
    [remote "upstream"]
            url = ...
            fetch = refs/heads/*:refs/remotes/linus/*

and you say "git checkout next", then after we switch the branch
we check the upstream (in this case, refs/remotes/linus/next)
and our branch, and:

    (1) if they match, nothing happens;

    (2) if you are ahead (i.e. the upstream is a strict ancestor
        of you), one line message tells you so;

    (3) otherwise, you are either behind or you and the upstream
        have forked.  One line message will tell you which and
        then you will see a "log --pretty=oneline --left-right".

We could enhance this with an option that tells the command to
check if there is no local change, and automatically fast
forward when you are truly behind.  But I ripped out that change
because I was unsure what the right way should be to allow users
to control it (issues include that checkout should not become
automatically interactive).

This is hot off the press and I know it tends to be a bit too
loud.  It is based on Daniel's "git checkout in C" with Dscho's
lock_file fix.

---

 builtin-checkout.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
 1 files changed, 136 insertions(+), 0 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 59a0ef4..9370ba0 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -12,6 +12,7 @@
 #include "branch.h"
 #include "diff.h"
 #include "revision.h"
+#include "remote.h"
 
 static const char * const checkout_usage[] = {
 	"git checkout [options] <branch>",
@@ -290,6 +291,139 @@ static int merge_working_tree(struct checkout_opts *opts,
 	return 0;
 }
 
+/*
+ * We really should allow cb_data... Yuck
+ */
+static const char *branch_name;
+static int branch_name_len;
+static char *found_remote;
+static char *found_merge;
+static int read_branch_config(const char *var, const char *value)
+{
+	const char *name;
+	if (prefixcmp(var, "branch."))
+		return 0; /* not ours */
+	name = var + strlen("branch.");
+	if (strncmp(name, branch_name, branch_name_len) ||
+	    name[branch_name_len] != '.')
+		return 0; /* not ours either */
+	if (!strcmp(name + branch_name_len, ".remote")) {
+		/*
+		 * Yeah, I know Christian's clean-up should
+		 * be used here, but the topic is based on an
+		 * older fork point.
+		 */
+		if (!value)
+			return error("'%s' not string", var);
+		found_remote = xstrdup(value);
+		return 0;
+	}
+	if (!strcmp(name + branch_name_len, ".merge")) {
+		if (!value)
+			return error("'%s' not string", var);
+		found_merge = xstrdup(value);
+		return 0;
+	}
+	return 0; /* not ours */
+}
+
+static int find_build_base(const char *ours, char **base)
+{
+	struct remote *remote;
+	struct refspec spec;
+
+	*base = NULL;
+
+	branch_name = ours + strlen("refs/heads/");
+	branch_name_len = strlen(branch_name);
+	found_remote = NULL;
+	found_merge = NULL;
+	git_config(read_branch_config);
+
+	if (!found_remote || !found_merge) {
+	cleanup:
+		free(found_remote);
+		free(found_merge);
+		return 0;
+	}
+
+	remote = remote_get(found_remote);
+	memset(&spec, 0, sizeof(spec));
+	spec.src = found_merge;
+	if (remote_find_tracking(remote, &spec))
+		goto cleanup;
+	*base = spec.dst;
+	return 1;
+}
+
+static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *opts)
+{
+	/*
+	 * We have switched to a new branch; is it building on
+	 * top of another branch, and if so does that other branch
+	 * have changes we do not have yet?
+	 */
+	char *base;
+	unsigned char sha1[20];
+	struct commit *ours, *theirs;
+	const char *msgfmt;
+	char symmetric[84];
+	int show_log;
+
+	if (!resolve_ref(new->path, sha1, 1, NULL))
+		return;
+	ours = lookup_commit(sha1);
+
+	if (!find_build_base(new->path, &base))
+		return;
+
+	sprintf(symmetric, "%s", sha1_to_hex(sha1));
+
+	/*
+	 * Ok, it is tracking base; is it ahead of us?
+	 */
+	if (!resolve_ref(base, sha1, 1, NULL))
+		return;
+	theirs = lookup_commit(sha1);
+
+	sprintf(symmetric + 40, "...%s", sha1_to_hex(sha1));
+
+	if (!hashcmp(sha1, ours->object.sha1))
+		return; /* we are the same */
+
+	show_log = 1;
+	if (in_merge_bases(theirs, &ours, 1)) {
+		msgfmt = "You are ahead of the tracked branch '%s'\n";
+		show_log = 0;
+	}
+	else if (in_merge_bases(ours, &theirs, 1))
+		msgfmt = "Your branch can be fast-forwarded to the tracked branch '%s'\n";
+	else
+		msgfmt = "Both your branch and the tracked branch '%s' have own changes, you would eventually need to merge\n";
+
+	if (!prefixcmp(base, "refs/remotes/"))
+		base += strlen("refs/remotes/");
+	fprintf(stderr, msgfmt, base);
+
+	if (show_log) {
+		const char *args[32];
+		int ac;
+
+		ac = 0;
+		args[ac++] = "log";
+		args[ac++] = "--pretty=oneline";
+		args[ac++] = "--abbrev-commit";
+		args[ac++] = "--left-right";
+		args[ac++] = "--boundary";
+		args[ac++] = symmetric;
+		args[ac++] = "--";
+		args[ac] = NULL;
+
+		run_command_v_opt(args, RUN_GIT_CMD);
+	}
+}
+
+
 static void update_refs_for_switch(struct checkout_opts *opts,
 				   struct branch_info *old,
 				   struct branch_info *new)
@@ -332,6 +466,8 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 	}
 	remove_branch_state();
 	strbuf_release(&msg);
+	if (new->path)
+		adjust_to_tracking(new, opts);
 }
 
 static int switch_branches(struct checkout_opts *opts,

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Daniel Barkalow]

On Sat, 16 Feb 2008, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Perhaps making "git-checkout" to notice this and offer (or
> > suggest) fast-forwarding at that point may be safer and make
> > more sense.  You cannot grow your local branch unless you check
> > them out, and your remote tracking will keep growing without the
> > auto-ff you are suggesting, so it is not like people will lose
> > anchoring point to compare between branches if we do not
> > auto-ff.
> 
> So I did this.
> 
> When you are switching to a branch that is marked to merge from
> somewhere else, e.g. when you have:
> 
>     [branch "next"]
>             remote = upstream
>             merge = refs/heads/next
>     [remote "upstream"]
>             url = ...
>             fetch = refs/heads/*:refs/remotes/linus/*
> 
> and you say "git checkout next", then after we switch the branch
> we check the upstream (in this case, refs/remotes/linus/next)
> and our branch, and:
> 
>     (1) if they match, nothing happens;
> 
>     (2) if you are ahead (i.e. the upstream is a strict ancestor
>         of you), one line message tells you so;
> 
>     (3) otherwise, you are either behind or you and the upstream
>         have forked.  One line message will tell you which and
>         then you will see a "log --pretty=oneline --left-right".

I like this idea a lot. I'd actually also like it for commit, although (1) 
and (3a) obviously don't happen there. It would help to combat my tendency 
to forget to push when I mean to.

Note that, in addition to "should I merge before starting to work now", 
this also answers "if I turn off this computer, what can't I get at".

> We could enhance this with an option that tells the command to
> check if there is no local change, and automatically fast
> forward when you are truly behind.  But I ripped out that change
> because I was unsure what the right way should be to allow users
> to control it (issues include that checkout should not become
> automatically interactive).

I suppose you could use a config option to enable it per-branch.

It would be really clever to make it happen if you do:

$ git checkout next
(fast-forward info)
$ git checkout

But that's probably a bit too clever.

> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index 59a0ef4..9370ba0 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -12,6 +12,7 @@
>  #include "branch.h"
>  #include "diff.h"
>  #include "revision.h"
> +#include "remote.h"
>  
>  static const char * const checkout_usage[] = {
>  	"git checkout [options] <branch>",
> @@ -290,6 +291,139 @@ static int merge_working_tree(struct checkout_opts *opts,
>  	return 0;
>  }
>  
> +/*
> + * We really should allow cb_data... Yuck
> + */
> +static const char *branch_name;
> +static int branch_name_len;
> +static char *found_remote;
> +static char *found_merge;
> +static int read_branch_config(const char *var, const char *value)
> +{

...

I think you want branch_get(), which handles all the config file stuff up 
to approximately here:

> +	remote = remote_get(found_remote);
> +	memset(&spec, 0, sizeof(spec));
> +	spec.src = found_merge;
> +	if (remote_find_tracking(remote, &spec))
> +		goto cleanup;
> +	*base = spec.dst;
> +	return 1;
> +}
> +
> +static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *opts)
> +{
> +	/*
> +	 * We have switched to a new branch; is it building on
> +	 * top of another branch, and if so does that other branch
> +	 * have changes we do not have yet?
> +	 */
> +	char *base;
> +	unsigned char sha1[20];
> +	struct commit *ours, *theirs;
> +	const char *msgfmt;
> +	char symmetric[84];
> +	int show_log;
> +
> +	if (!resolve_ref(new->path, sha1, 1, NULL))
> +		return;
> +	ours = lookup_commit(sha1);
> +
> +	if (!find_build_base(new->path, &base))
> +		return;
> +
> +	sprintf(symmetric, "%s", sha1_to_hex(sha1));
> +
> +	/*
> +	 * Ok, it is tracking base; is it ahead of us?
> +	 */
> +	if (!resolve_ref(base, sha1, 1, NULL))
> +		return;
> +	theirs = lookup_commit(sha1);
> +
> +	sprintf(symmetric + 40, "...%s", sha1_to_hex(sha1));
> +
> +	if (!hashcmp(sha1, ours->object.sha1))
> +		return; /* we are the same */
> +
> +	show_log = 1;
> +	if (in_merge_bases(theirs, &ours, 1)) {
> +		msgfmt = "You are ahead of the tracked branch '%s'\n";
> +		show_log = 0;
> +	}
> +	else if (in_merge_bases(ours, &theirs, 1))
> +		msgfmt = "Your branch can be fast-forwarded to the tracked branch '%s'\n";
> +	else
> +		msgfmt = "Both your branch and the tracked branch '%s' have own changes, you would eventually need to merge\n";
> +
> +	if (!prefixcmp(base, "refs/remotes/"))
> +		base += strlen("refs/remotes/");
> +	fprintf(stderr, msgfmt, base);
> +
> +	if (show_log) {
> +		const char *args[32];
> +		int ac;
> +
> +		ac = 0;
> +		args[ac++] = "log";
> +		args[ac++] = "--pretty=oneline";
> +		args[ac++] = "--abbrev-commit";
> +		args[ac++] = "--left-right";
> +		args[ac++] = "--boundary";
> +		args[ac++] = symmetric;
> +		args[ac++] = "--";
> +		args[ac] = NULL;
> +
> +		run_command_v_opt(args, RUN_GIT_CMD);

We really should be able to do this in-process, although I'm not sure if 
we really can. I don't think I've marked up the history in 
builtin-checkout for anything else yet, anyway.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> I like this idea a lot. I'd actually also like it for commit, although (1) 
> and (3a) obviously don't happen there. It would help to combat my tendency 
> to forget to push when I mean to.

Thanks.

>> +/*
>> + * We really should allow cb_data... Yuck
>> + */
>> +static const char *branch_name;
>> +static int branch_name_len;
>> +static char *found_remote;
>> +static char *found_merge;
>> +static int read_branch_config(const char *var, const char *value)
>> +{
>
> ...
>
> I think you want branch_get(), which handles all the config file stuff up 
> to approximately here:

Fixups are very much welcomed.  This was more or less a proof of
concept.

>> +	if (show_log) {
>> +		const char *args[32];
>> +		int ac;
>> +
>> +		ac = 0;
>> +		args[ac++] = "log";
>> +		args[ac++] = "--pretty=oneline";
>> +		args[ac++] = "--abbrev-commit";
>> +		args[ac++] = "--left-right";
>> +		args[ac++] = "--boundary";
>> +		args[ac++] = symmetric;
>> +		args[ac++] = "--";
>> +		args[ac] = NULL;
>> +
>> +		run_command_v_opt(args, RUN_GIT_CMD);
>
> We really should be able to do this in-process, although I'm not sure if 
> we really can.

The code runs in_merge_bases() twice, between our branch head
and the base, but if we really care about the performance, we
can have a single merge-base traversal and the resulting object
pool will have everything necessary to emit the log output
without a separate traversal.

Because I think that is reasonably easy, I just did not bother
to.  This is not a performance critical piece of code anyway.

One thing I thought about was to limit the output to latest N
entries from both sides.  That would also be easier to implement
if we do a single merge-base traversal and reuse the result.

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Daniel Barkalow]

On Sat, 16 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > I like this idea a lot. I'd actually also like it for commit, although (1) 
> > and (3a) obviously don't happen there. It would help to combat my tendency 
> > to forget to push when I mean to.
> 
> Thanks.
> 
> >> +/*
> >> + * We really should allow cb_data... Yuck
> >> + */
> >> +static const char *branch_name;
> >> +static int branch_name_len;
> >> +static char *found_remote;
> >> +static char *found_merge;
> >> +static int read_branch_config(const char *var, const char *value)
> >> +{
> >
> > ...
> >
> > I think you want branch_get(), which handles all the config file stuff up 
> > to approximately here:
> 
> Fixups are very much welcomed.  This was more or less a proof of
> concept.

I'm also in part worried about the lack of publicity "struct branch" has 
gotten; it would have saved you having to write 64 of 136 lines, so it's 
worth you knowing about.

> >> +	if (show_log) {
> >> +		const char *args[32];
> >> +		int ac;
> >> +
> >> +		ac = 0;
> >> +		args[ac++] = "log";
> >> +		args[ac++] = "--pretty=oneline";
> >> +		args[ac++] = "--abbrev-commit";
> >> +		args[ac++] = "--left-right";
> >> +		args[ac++] = "--boundary";
> >> +		args[ac++] = symmetric;
> >> +		args[ac++] = "--";
> >> +		args[ac] = NULL;
> >> +
> >> +		run_command_v_opt(args, RUN_GIT_CMD);
> >
> > We really should be able to do this in-process, although I'm not sure if 
> > we really can.
> 
> The code runs in_merge_bases() twice, between our branch head
> and the base, but if we really care about the performance, we
> can have a single merge-base traversal and the resulting object
> pool will have everything necessary to emit the log output
> without a separate traversal.
> 
> Because I think that is reasonably easy, I just did not bother
> to.  This is not a performance critical piece of code anyway.
> 
> One thing I thought about was to limit the output to latest N
> entries from both sides.  That would also be easier to implement
> if we do a single merge-base traversal and reuse the result.

Yeah, the real advantage to doing it in-process is being able to give a 
particularly useful overview. Also, I think spawning a pager for it is 
distracting.

I've also got a change to make it do this report for "git checkout" and 
"git checkout HEAD" so you can find out if the current branch needs 
anything, but I want to look at it again while not so sleepy before 
sending it, and also write some tests.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Junio C Hamano]

Daniel Barkalow <barkalow@iabervon.org> writes:

> I'm also in part worried about the lack of publicity "struct branch" has 
> gotten; it would have saved you having to write 64 of 136 lines, so it's 
> worth you knowing about.

Actually, I initially took a look at branch.h and tried to reuse
find_tracked_branch() but then realized it was a wrong interface.
The "struct branch" in remote.h (Heh) looks like the right
interface.

Documentation/technical/api-*.txt should really talk about
what's in remote.c.

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Daniel Barkalow]

On Sat, 16 Feb 2008, Junio C Hamano wrote:

> Daniel Barkalow <barkalow@iabervon.org> writes:
> 
> > I'm also in part worried about the lack of publicity "struct branch" has 
> > gotten; it would have saved you having to write 64 of 136 lines, so it's 
> > worth you knowing about.
> 
> Actually, I initially took a look at branch.h and tried to reuse
> find_tracked_branch() but then realized it was a wrong interface.

Yeah, that's the other direction, and for setting it up (so the 
configuration isn't there yet, and it's trying to figure out what the 
requested default is).

> The "struct branch" in remote.h (Heh) looks like the right
> interface.

Now that branch.h exists, it probably should have struct branch, I guess. 
Back when I made struct branch, it was just about finding the right 
configuration for fetch, so I was thinking more about the "doing the right 
thing with remotes" aspect than the "configuration about branches" aspect. 
They're somewhat intertwined, in any case, because remote_get(NULL) uses 
the branch configuration to find the default, and branch_get() uses the 
remote configuration to interpret merge settings.

> Documentation/technical/api-*.txt should really talk about
> what's in remote.c.

Ah, that's where that should go. I remember reading about it while I was 
on vacation and promptly forgetting all about it. Writing stuff up now.

	-Daniel
*This .sig left intentionally blank*

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Jeff King]

On Sat, Feb 16, 2008 at 05:08:22PM -0800, Junio C Hamano wrote:

> and you say "git checkout next", then after we switch the branch
> we check the upstream (in this case, refs/remotes/linus/next)
> and our branch, and:
> 
>     (1) if they match, nothing happens;
> 
>     (2) if you are ahead (i.e. the upstream is a strict ancestor
>         of you), one line message tells you so;
> 
>     (3) otherwise, you are either behind or you and the upstream
>         have forked.  One line message will tell you which and
>         then you will see a "log --pretty=oneline --left-right".

Overall I think this is a sensible idea. For (3), it probably makes
sense to limit the output in some cases. If I checkout a topic branch
that I haven't looked at in a few days or even weeks, I am going to get
spammed with hundreds of commits.

Most of the time what I really want to know is "I am not up to date and
should merge or rebase." Automatically showing _which_ commits diverge
is a convenience that makes sense if there are a handful of them. For
larger cases, the user can easily run "git log upstream...branch".

Of course this is speculation and gut feeling; I'll try running with
this for a few weeks and see if my opinion changes.

-Peff


> 
> We could enhance this with an option that tells the command to
> check if there is no local change, and automatically fast
> forward when you are truly behind.  But I ripped out that change
> because I was unsure what the right way should be to allow users
> to control it (issues include that checkout should not become
> automatically interactive).
> 
> This is hot off the press and I know it tends to be a bit too
> loud.  It is based on Daniel's "git checkout in C" with Dscho's
> lock_file fix.
> 
> ---
> 
>  builtin-checkout.c |  136 ++++++++++++++++++++++++++++++++++++++++++++++++++++
>  1 files changed, 136 insertions(+), 0 deletions(-)
> 
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index 59a0ef4..9370ba0 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -12,6 +12,7 @@
>  #include "branch.h"
>  #include "diff.h"
>  #include "revision.h"
> +#include "remote.h"
>  
>  static const char * const checkout_usage[] = {
>  	"git checkout [options] <branch>",
> @@ -290,6 +291,139 @@ static int merge_working_tree(struct checkout_opts *opts,
>  	return 0;
>  }
>  
> +/*
> + * We really should allow cb_data... Yuck
> + */
> +static const char *branch_name;
> +static int branch_name_len;
> +static char *found_remote;
> +static char *found_merge;
> +static int read_branch_config(const char *var, const char *value)
> +{
> +	const char *name;
> +	if (prefixcmp(var, "branch."))
> +		return 0; /* not ours */
> +	name = var + strlen("branch.");
> +	if (strncmp(name, branch_name, branch_name_len) ||
> +	    name[branch_name_len] != '.')
> +		return 0; /* not ours either */
> +	if (!strcmp(name + branch_name_len, ".remote")) {
> +		/*
> +		 * Yeah, I know Christian's clean-up should
> +		 * be used here, but the topic is based on an
> +		 * older fork point.
> +		 */
> +		if (!value)
> +			return error("'%s' not string", var);
> +		found_remote = xstrdup(value);
> +		return 0;
> +	}
> +	if (!strcmp(name + branch_name_len, ".merge")) {
> +		if (!value)
> +			return error("'%s' not string", var);
> +		found_merge = xstrdup(value);
> +		return 0;
> +	}
> +	return 0; /* not ours */
> +}
> +
> +static int find_build_base(const char *ours, char **base)
> +{
> +	struct remote *remote;
> +	struct refspec spec;
> +
> +	*base = NULL;
> +
> +	branch_name = ours + strlen("refs/heads/");
> +	branch_name_len = strlen(branch_name);
> +	found_remote = NULL;
> +	found_merge = NULL;
> +	git_config(read_branch_config);
> +
> +	if (!found_remote || !found_merge) {
> +	cleanup:
> +		free(found_remote);
> +		free(found_merge);
> +		return 0;
> +	}
> +
> +	remote = remote_get(found_remote);
> +	memset(&spec, 0, sizeof(spec));
> +	spec.src = found_merge;
> +	if (remote_find_tracking(remote, &spec))
> +		goto cleanup;
> +	*base = spec.dst;
> +	return 1;
> +}
> +
> +static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *opts)
> +{
> +	/*
> +	 * We have switched to a new branch; is it building on
> +	 * top of another branch, and if so does that other branch
> +	 * have changes we do not have yet?
> +	 */
> +	char *base;
> +	unsigned char sha1[20];
> +	struct commit *ours, *theirs;
> +	const char *msgfmt;
> +	char symmetric[84];
> +	int show_log;
> +
> +	if (!resolve_ref(new->path, sha1, 1, NULL))
> +		return;
> +	ours = lookup_commit(sha1);
> +
> +	if (!find_build_base(new->path, &base))
> +		return;
> +
> +	sprintf(symmetric, "%s", sha1_to_hex(sha1));
> +
> +	/*
> +	 * Ok, it is tracking base; is it ahead of us?
> +	 */
> +	if (!resolve_ref(base, sha1, 1, NULL))
> +		return;
> +	theirs = lookup_commit(sha1);
> +
> +	sprintf(symmetric + 40, "...%s", sha1_to_hex(sha1));
> +
> +	if (!hashcmp(sha1, ours->object.sha1))
> +		return; /* we are the same */
> +
> +	show_log = 1;
> +	if (in_merge_bases(theirs, &ours, 1)) {
> +		msgfmt = "You are ahead of the tracked branch '%s'\n";
> +		show_log = 0;
> +	}
> +	else if (in_merge_bases(ours, &theirs, 1))
> +		msgfmt = "Your branch can be fast-forwarded to the tracked branch '%s'\n";
> +	else
> +		msgfmt = "Both your branch and the tracked branch '%s' have own changes, you would eventually need to merge\n";
> +
> +	if (!prefixcmp(base, "refs/remotes/"))
> +		base += strlen("refs/remotes/");
> +	fprintf(stderr, msgfmt, base);
> +
> +	if (show_log) {
> +		const char *args[32];
> +		int ac;
> +
> +		ac = 0;
> +		args[ac++] = "log";
> +		args[ac++] = "--pretty=oneline";
> +		args[ac++] = "--abbrev-commit";
> +		args[ac++] = "--left-right";
> +		args[ac++] = "--boundary";
> +		args[ac++] = symmetric;
> +		args[ac++] = "--";
> +		args[ac] = NULL;
> +
> +		run_command_v_opt(args, RUN_GIT_CMD);
> +	}
> +}
> +
> +
>  static void update_refs_for_switch(struct checkout_opts *opts,
>  				   struct branch_info *old,
>  				   struct branch_info *new)
> @@ -332,6 +466,8 @@ static void update_refs_for_switch(struct checkout_opts *opts,
>  	}
>  	remove_branch_state();
>  	strbuf_release(&msg);
> +	if (new->path)
> +		adjust_to_tracking(new, opts);
>  }
>  
>  static int switch_branches(struct checkout_opts *opts,

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Santi Béjar]

On Sun, Feb 17, 2008 at 1:28 PM, Jeff King <peff@peff.net> wrote:
> On Sat, Feb 16, 2008 at 05:08:22PM -0800, Junio C Hamano wrote:
>
>  > and you say "git checkout next", then after we switch the branch
>  > we check the upstream (in this case, refs/remotes/linus/next)
>  > and our branch, and:
>  >
>  >     (1) if they match, nothing happens;
>  >
>  >     (2) if you are ahead (i.e. the upstream is a strict ancestor
>  >         of you), one line message tells you so;
>  >
>  >     (3) otherwise, you are either behind or you and the upstream
>  >         have forked.  One line message will tell you which and
>  >         then you will see a "log --pretty=oneline --left-right".
>
>  Overall I think this is a sensible idea. For (3), it probably makes
>  sense to limit the output in some cases. If I checkout a topic branch
>  that I haven't looked at in a few days or even weeks, I am going to get
>  spammed with hundreds of commits.
>
>  Most of the time what I really want to know is "I am not up to date and
>  should merge or rebase." Automatically showing _which_ commits diverge
>  is a convenience that makes sense if there are a handful of them. For
>  larger cases, the user can easily run "git log upstream...branch".

I prefer to always have a summary as:

The tracking branch is ahead:
 $branchsha1..$upstreamsha1 (<n> commits)

and

Branch and tracking branch have diverged:
 $branchsha1...$upstreamsha1 (<n>|<m> commits)

or something like that.

Additionally, the text send to stderr (Switched to... Your branch can
be fast...) is hidden when the pager run.

Santi

Re: [RFC] checkout to notice forks (Re: Minor annoyance with git push)

[Martin Langhoff]

On Feb 17, 2008 2:08 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
> > Perhaps making "git-checkout" to notice this and offer (or
> > suggest) fast-forwarding at that point may be safer and make
> > more sense.  You cannot grow your local branch unless you check
> > them out, and your remote tracking will keep growing without the
> > auto-ff you are suggesting, so it is not like people will lose
> > anchoring point to compare between branches if we do not
> > auto-ff.
>
> So I did this.

Great - *thanks*. I'm travelling ATM so lousy connectivity and
shattered focus. I did a fetch of git.git and can't spot this in next,
pu or master. Reading further down the thread I see that you're
probably rewriting it to use tree struct. Will give your initial patch
a whirl anyway.

cheers,


martin

[PATCH] checkout: tone down the "forked status" diagnostic messages

[Junio C Hamano]

When checking out a branch that is behind or forked from a
branch you are building on top of, we used to show full
left-right log but if you already _know_ you have long history
since you forked, it is a bit too much.

This tones down the message quite a bit, by only showing the
number of commits each side has since they diverged.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * This obviously comes on top of the earlier patch of mine,
   which in turn is on top of Daniel's "checkout written in C".

 builtin-checkout.c |  102 ++++++++++++++++++++++++++++++++--------------------
 1 files changed, 63 insertions(+), 39 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 5291f72..261f67f 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -301,64 +301,88 @@ static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *op
 	char *base;
 	unsigned char sha1[20];
 	struct commit *ours, *theirs;
-	const char *msgfmt;
 	char symmetric[84];
-	int show_log;
+	struct rev_info revs;
+	const char *rev_argv[10];
+	int rev_argc;
+	int num_ours, num_theirs;
+	const char *remote_msg;
 	struct branch *branch = branch_get(NULL);
 
+	/*
+	 * Nothing to report unless we are marked to build on top of
+	 * somebody else.
+	 */
 	if (!branch || !branch->merge)
 		return;
 
-	base = branch->merge[0]->dst;
-
-	ours = new->commit;
-
-	sprintf(symmetric, "%s", sha1_to_hex(ours->object.sha1));
-
 	/*
-	 * Ok, it is tracking base; is it ahead of us?
+	 * If what we used to build on no longer exists, there is
+	 * nothing to report.
 	 */
+	base = branch->merge[0]->dst;
 	if (!resolve_ref(base, sha1, 1, NULL))
 		return;
-	theirs = lookup_commit(sha1);
-
-	sprintf(symmetric + 40, "...%s", sha1_to_hex(sha1));
 
+	theirs = lookup_commit(sha1);
+	ours = new->commit;
 	if (!hashcmp(sha1, ours->object.sha1))
 		return; /* we are the same */
 
-	show_log = 1;
-	if (in_merge_bases(theirs, &ours, 1)) {
-		msgfmt = "You are ahead of the tracked branch '%s'\n";
-		show_log = 0;
+	/* Run "rev-list --left-right ours...theirs" internally... */
+	rev_argc = 0;
+	rev_argv[rev_argc++] = NULL;
+	rev_argv[rev_argc++] = "--left-right";
+	rev_argv[rev_argc++] = symmetric;
+	rev_argv[rev_argc++] = "--";
+	rev_argv[rev_argc] = NULL;
+
+	strcpy(symmetric, sha1_to_hex(ours->object.sha1));
+	strcpy(symmetric + 40, "...");
+	strcpy(symmetric + 43, sha1_to_hex(theirs->object.sha1));
+
+	init_revisions(&revs, NULL);
+	setup_revisions(rev_argc, rev_argv, &revs, NULL);
+	prepare_revision_walk(&revs);
+
+	/* ... and count the commits on each side. */
+	num_ours = 0;
+	num_theirs = 0;
+	while (1) {
+		struct commit *c = get_revision(&revs);
+		if (!c)
+			break;
+		if (c->object.flags & SYMMETRIC_LEFT)
+			num_ours++;
+		else
+			num_theirs++;
 	}
-	else if (in_merge_bases(ours, &theirs, 1))
-		msgfmt = "Your branch can be fast-forwarded to the tracked branch '%s'\n";
-	else
-		msgfmt = "Both your branch and the tracked branch '%s' have own changes, you would eventually need to merge\n";
 
-	if (!prefixcmp(base, "refs/remotes/"))
+	if (!prefixcmp(base, "refs/remotes/")) {
+		remote_msg = " remote";
 		base += strlen("refs/remotes/");
-	fprintf(stderr, msgfmt, base);
-
-	if (show_log) {
-		const char *args[32];
-		int ac;
-
-		ac = 0;
-		args[ac++] = "log";
-		args[ac++] = "--pretty=oneline";
-		args[ac++] = "--abbrev-commit";
-		args[ac++] = "--left-right";
-		args[ac++] = "--boundary";
-		args[ac++] = symmetric;
-		args[ac++] = "--";
-		args[ac] = NULL;
-
-		run_command_v_opt(args, RUN_GIT_CMD);
+	} else {
+		remote_msg = "";
 	}
-}
 
+	if (!num_theirs)
+		printf("Your branch is ahead of the tracked%s branch '%s' "
+		       "by %d commit%s.\n",
+		       remote_msg, base,
+		       num_ours, (num_ours == 1) ? "" : "s");
+	else if (!num_ours)
+		printf("Your branch is behind of the tracked%s branch '%s' "
+		       "by %d commit%s,\n"
+		       "and can be fast-forwarded.\n",
+		       remote_msg, base,
+		       num_theirs, (num_theirs == 1) ? "" : "s");
+	else
+		printf("Your branch and the tracked%s branch '%s' "
+		       "have diverged,\nand respectively "
+		       "have %d and %d different commit(s) each.\n",
+		       remote_msg, base,
+		       num_ours, num_theirs);
+}
 
 static void update_refs_for_switch(struct checkout_opts *opts,
 				   struct branch_info *old,

Re: [PATCH] checkout: tone down the "forked status" diagnostic messages

[Jeff King]

On Wed, Feb 20, 2008 at 03:05:23PM -0800, Junio C Hamano wrote:

> @@ -301,64 +301,88 @@ static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *op
>  	char *base;
>  	unsigned char sha1[20];
>  	struct commit *ours, *theirs;
> -	const char *msgfmt;
>  	char symmetric[84];
> -	int show_log;
> +	struct rev_info revs;
> +	const char *rev_argv[10];
> +	int rev_argc;
> +	int num_ours, num_theirs;
> +	const char *remote_msg;
>  	struct branch *branch = branch_get(NULL);

Shouldn't this be branch_get(new->name)? branch_get calls read_config(),
which caches the "current branch" information, so it's possible to end
up with stale branch info. Try:

  git clone git://git.kernel.org/pub/scm/git/git.git
  git checkout -b next origin/next

I get:

  Branch next set up to track remote branch refs/remotes/origin/next.
  Switched to a new branch "next"
  Your branch is ahead of the tracked remote branch 'origin/master' by 76
  commits.

Switching to master and then back to the already-created "next" works
fine.

Even safer, I think, would be a way to invalidate the information cached
in read_config when we change branches; this would fix it for any other
callsites that look at the current branch from the same git invocation
that changes the current branch.

> +	if (!num_theirs)
> +		printf("Your branch is ahead of the tracked%s branch '%s' "
> +		       "by %d commit%s.\n",
> +		       remote_msg, base,
> +		       num_ours, (num_ours == 1) ? "" : "s");
> +	else if (!num_ours)
> +		printf("Your branch is behind of the tracked%s branch '%s' "
> +		       "by %d commit%s,\n"
> +		       "and can be fast-forwarded.\n",

While not uncommon colloquially, "behind of" is not grammatically
correct. "behind" is a preposition, so the "of" is redundant (it is
necessary in the top string because "ahead" is an adverb).

-Peff

[PATCH] checkout: updates to tracking report

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  	struct branch *branch = branch_get(NULL);
>
> Shouldn't this be branch_get(new->name)?

Perhaps.

> ...
> While not uncommon colloquially, "behind of" is not grammatically
> correct. "behind" is a preposition, so the "of" is redundant (it is
> necessary in the top string because "ahead" is an adverb).

Thanks.

-- >8 --

Ask branch_get() for the new branch explicitly instead of
letting it return a potentially stale information.

Tighten the logic to find the tracking branch to deal better
with misconfigured repositories (i.e. branch.*.merge can exist
but it may not have a refspec that fetches to .it)

Also fixes grammar in a message, as pointed out by Jeff King.

The function is about reporting and not automatically
fast-forwarding to the upstream, so stop calling it
"adjust-to".

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin-checkout.c |   10 +++++-----
 1 files changed, 5 insertions(+), 5 deletions(-)

diff --git a/builtin-checkout.c b/builtin-checkout.c
index f51b77a..19413eb 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -290,7 +290,7 @@ static int merge_working_tree(struct checkout_opts *opts,
 	return 0;
 }
 
-static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *opts)
+static void report_tracking(struct branch_info *new, struct checkout_opts *opts)
 {
 	/*
 	 * We have switched to a new branch; is it building on
@@ -306,13 +306,13 @@ static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *op
 	int rev_argc;
 	int num_ours, num_theirs;
 	const char *remote_msg;
-	struct branch *branch = branch_get(NULL);
+	struct branch *branch = branch_get(new->name);
 
 	/*
 	 * Nothing to report unless we are marked to build on top of
 	 * somebody else.
 	 */
-	if (!branch || !branch->merge)
+	if (!branch || !branch->merge || !branch->merge[0] || !branch->merge[0]->dst)
 		return;
 
 	/*
@@ -370,7 +370,7 @@ static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *op
 		       remote_msg, base,
 		       num_ours, (num_ours == 1) ? "" : "s");
 	else if (!num_ours)
-		printf("Your branch is behind of the tracked%s branch '%s' "
+		printf("Your branch is behind the tracked%s branch '%s' "
 		       "by %d commit%s,\n"
 		       "and can be fast-forwarded.\n",
 		       remote_msg, base,
@@ -426,7 +426,7 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 	remove_branch_state();
 	strbuf_release(&msg);
 	if (!opts->quiet && (new->path || !strcmp(new->name, "HEAD")))
-		adjust_to_tracking(new, opts);
+		report_tracking(new, opts);
 }
 
 static int switch_branches(struct checkout_opts *opts,
-- 
1.5.4.2.261.g851a5

Re: [PATCH] checkout: updates to tracking report

[Jay Soffian]

On Wed, Feb 20, 2008 at 10:42 PM, Junio C Hamano <gitster@pobox.com> wrote:

>   builtin-checkout.c |   10 +++++-----
>   1 files changed, 5 insertions(+), 5 deletions(-)

Actually, it would be nice to get this information w/o having to
checkout each branch. "git show-branch --tracking-status" maybe
to report it for all relevant branches?

j.

Re: [PATCH] checkout: updates to tracking report

[Daniel Barkalow]

On Wed, 20 Feb 2008, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >>  	struct branch *branch = branch_get(NULL);
> >
> > Shouldn't this be branch_get(new->name)?
> 
> Perhaps.
> 
> > ...
> > While not uncommon colloquially, "behind of" is not grammatically
> > correct. "behind" is a preposition, so the "of" is redundant (it is
> > necessary in the top string because "ahead" is an adverb).
> 
> Thanks.
> 
> -- >8 --
> 
> Ask branch_get() for the new branch explicitly instead of
> letting it return a potentially stale information.

I think it would be good to update current_branch in remote.c when we 
change branches, just on principle, but we also might as well not depend 
on that here.

> Tighten the logic to find the tracking branch to deal better
> with misconfigured repositories (i.e. branch.*.merge can exist
> but it may not have a refspec that fetches to .it)
> 
> Also fixes grammar in a message, as pointed out by Jeff King.
> 
> The function is about reporting and not automatically
> fast-forwarding to the upstream, so stop calling it
> "adjust-to".
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Acked-by: Daniel Barkalow <barkalow@iabervon.org>

> ---
>  builtin-checkout.c |   10 +++++-----
>  1 files changed, 5 insertions(+), 5 deletions(-)
> 
> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index f51b77a..19413eb 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -290,7 +290,7 @@ static int merge_working_tree(struct checkout_opts *opts,
>  	return 0;
>  }
>  
> -static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *opts)
> +static void report_tracking(struct branch_info *new, struct checkout_opts *opts)
>  {
>  	/*
>  	 * We have switched to a new branch; is it building on
> @@ -306,13 +306,13 @@ static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *op
>  	int rev_argc;
>  	int num_ours, num_theirs;
>  	const char *remote_msg;
> -	struct branch *branch = branch_get(NULL);
> +	struct branch *branch = branch_get(new->name);
>  
>  	/*
>  	 * Nothing to report unless we are marked to build on top of
>  	 * somebody else.
>  	 */
> -	if (!branch || !branch->merge)
> +	if (!branch || !branch->merge || !branch->merge[0] || !branch->merge[0]->dst)

branch->merge[i] can't actually be NULL; all of the allocated space in the 
array gets stuff stored in it.

>  		return;
>  
>  	/*
> @@ -370,7 +370,7 @@ static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *op
>  		       remote_msg, base,
>  		       num_ours, (num_ours == 1) ? "" : "s");
>  	else if (!num_ours)
> -		printf("Your branch is behind of the tracked%s branch '%s' "
> +		printf("Your branch is behind the tracked%s branch '%s' "
>  		       "by %d commit%s,\n"
>  		       "and can be fast-forwarded.\n",
>  		       remote_msg, base,
> @@ -426,7 +426,7 @@ static void update_refs_for_switch(struct checkout_opts *opts,
>  	remove_branch_state();
>  	strbuf_release(&msg);
>  	if (!opts->quiet && (new->path || !strcmp(new->name, "HEAD")))
> -		adjust_to_tracking(new, opts);
> +		report_tracking(new, opts);
>  }
>  
>  static int switch_branches(struct checkout_opts *opts,
> -- 
> 1.5.4.2.261.g851a5
> 

Re: [PATCH] checkout: tone down the "forked status" diagnostic messages

[Jay Soffian]

On Wed, Feb 20, 2008 at 6:05 PM, Junio C Hamano <gitster@pobox.com> wrote:

>  diff --git a/builtin-checkout.c b/builtin-checkout.c
>  index 5291f72..261f67f 100644
>  --- a/builtin-checkout.c
>  +++ b/builtin-checkout.c
>  @@ -301,64 +301,88 @@ static void adjust_to_tracking(struct branch_info *new, struct checkout_opts *op
>         char *base;
>  +       base = branch->merge[0]->dst;
>         if (!resolve_ref(base, sha1, 1, NULL))
>                 return;

Apparently base can be null, which on OS X causes a Bus Error in resolve_ref
when it calls strcmp(ref, list->name)

j.

Re: Minor annoyance with git push

[Steffen Prohaska]

On Feb 9, 2008, at 4:00 AM, Jeff King wrote:

>
> I really think that Steffen's "default to pushing only the current
> branch" approach fits much better with the model described in your
> workflow, and is generally a safer default. IIRC, the main  
> objection was
> that old-timers like the current push behavior better. Steffen, was
> there objection to a "push.onlyHEAD" config option?

There might have been an argument like: We should have a single
default because otherwise the behaviour of git depends on the
local configuration of the user.  This may cause even more
confusion than it tries to solve, because now you always need
to start first talking about the local configuration of the
users before you can start explaining how to actually solve the
problem.

Personally, I decided it is safer to teach users to explicitly
type what they mean.  I'd probably not use the push.onlyHEAD
config option.

I also proposed that the default could do nothing if no explicit
push lines are in the configuration file.  Users would be forced
to explicitly type what the want: Either they can say "--matching"
or they can say "--current".  This is similar to the new
"git clean" default.  But I remember there *was* objection against
this because everyone would be forced to type more and different
than "git clean" the default of "git push" is considered "safe",
so there's no need to protect the user from "git push".

Junio proposed various possible changes to the configuration
variables that could resolve the issues.  I do not remember the
details.

	Steffen

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sat, 9 Feb 2008, Steffen Prohaska wrote:

> Personally, I decided it is safer to teach users to explicitly type what 
> they mean.  I'd probably not use the push.onlyHEAD config option.
> 
> I also proposed that the default could do nothing if no explicit push 
> lines are in the configuration file.  Users would be forced to 
> explicitly type what the want: Either they can say "--matching" or they 
> can say "--current".  This is similar to the new "git clean" default.  
> But I remember there *was* objection against this because everyone would 
> be forced to type more and different than "git clean" the default of 
> "git push" is considered "safe", so there's no need to protect the user 
> from "git push".
> 
> Junio proposed various possible changes to the configuration variables 
> that could resolve the issues.  I do not remember the details.

The way would be like this, I think:

- introduce a command line option for push, like "--push-common-refs", and 
issue a warning whenever "git push" is called without command line 
options (along the lines "This default behaviour is deprecated; please use 
--push-common-refs").

- in a waaaay later version, just take away the default action of "git 
push", instead showing the usage.

We had something similar wit the -i and -o options to "git commit".

Ciao,
Dscho

Re: Minor annoyance with git push

[Junio C Hamano]

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

> The way would be like this, I think:
>
> - introduce a command line option for push, like "--push-common-refs", and 
> issue a warning whenever "git push" is called without command line 
> options (along the lines "This default behaviour is deprecated; please use 
> --push-common-refs").
>
> - in a waaaay later version, just take away the default action of "git 
> push", instead showing the usage.

I do not think that is a right approach.  To please both camps
without forcing people to

 (1) change what they are used to, and
 (2) type overlong command line,

I think the traditional "matching refs by default", combined
with "'git push HEAD' defaults to pushing the current branch to
the default location" would be a well balanced compromise.

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sat, 9 Feb 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > The way would be like this, I think:
> >
> > - introduce a command line option for push, like "--push-common-refs", 
> > and issue a warning whenever "git push" is called without command line 
> > options (along the lines "This default behaviour is deprecated; please 
> > use --push-common-refs").
> >
> > - in a waaaay later version, just take away the default action of "git 
> > push", instead showing the usage.
> 
> I do not think that is a right approach.  To please both camps without 
> forcing people to
> 
>  (1) change what they are used to, and
>  (2) type overlong command line,
> 
> I think the traditional "matching refs by default", combined with "'git 
> push HEAD' defaults to pushing the current branch to the default 
> location" would be a well balanced compromise.

I'm no longer that sure.  It seems that quite a lot of people do not read 
manuals, and have no clue what they are doing when they just try

	$ git push

to see what the synopsis is.

If there are enough of those people out there, we might want to change our 
default action to "-h".

Yes, that hurts old-timers.  Yes, it's not a perfect world.  No, I don't 
want to bend over for just a few people.

Ciao,
Dscho

Re: Minor annoyance with git push

[Jeff King]

On Sun, Feb 10, 2008 at 02:15:50AM +0000, Johannes Schindelin wrote:

> I'm no longer that sure.  It seems that quite a lot of people do not read 
> manuals, and have no clue what they are doing when they just try
> 
> 	$ git push
> 
> to see what the synopsis is.
> 
> If there are enough of those people out there, we might want to change our 
> default action to "-h".
> 
> Yes, that hurts old-timers.  Yes, it's not a perfect world.  No, I don't 
> want to bend over for just a few people.

I am still not convinced that an option to change the default behavior
is unreasonable. Yes, it means that "git push" will do different things
depending on your confi$g. But "git push" is a _shorthand_, and if you
want to say things definitely, then say them: "git push --matching
origin" or "git push HEAD" (assuming that a "--matching" option would
exist to specify what is now the default behavior).

-Peff

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sun, 10 Feb 2008, Jeff King wrote:

> On Sun, Feb 10, 2008 at 02:15:50AM +0000, Johannes Schindelin wrote:
> 
> > I'm no longer that sure.  It seems that quite a lot of people do not read 
> > manuals, and have no clue what they are doing when they just try
> > 
> > 	$ git push
> > 
> > to see what the synopsis is.
> > 
> > If there are enough of those people out there, we might want to change 
> > our default action to "-h".
> > 
> > Yes, that hurts old-timers.  Yes, it's not a perfect world.  No, I 
> > don't want to bend over for just a few people.
> 
> I am still not convinced that an option to change the default behavior 
> is unreasonable. Yes, it means that "git push" will do different things 
> depending on your confi$g. But "git push" is a _shorthand_, and if you 
> want to say things definitely, then say them: "git push --matching 
> origin" or "git push HEAD" (assuming that a "--matching" option would 
> exist to specify what is now the default behavior).

Well, I am not completely opposed to changing the default behaviour, be 
that showing the synopsis or pushing HEAD to origin.

But _do_ give old-timers some time to adjust, _if_ you want to change the 
default behaviour.

Ciao,
Dscho

Re: Minor annoyance with git push

[Jeff King]

On Sun, Feb 10, 2008 at 12:20:23PM +0000, Johannes Schindelin wrote:

> > I am still not convinced that an option to change the default behavior 
> > is unreasonable. Yes, it means that "git push" will do different things 
> > depending on your confi$g. But "git push" is a _shorthand_, and if you 
> > want to say things definitely, then say them: "git push --matching 
> > origin" or "git push HEAD" (assuming that a "--matching" option would 
> > exist to specify what is now the default behavior).
> 
> Well, I am not completely opposed to changing the default behaviour, be 
> that showing the synopsis or pushing HEAD to origin.
> 
> But _do_ give old-timers some time to adjust, _if_ you want to change the 
> default behaviour.

Sorry, I should have been more clear. By "default" I mean "what happens
when you type "git push" not "the behavior with no config options set."

IOW, I am not necessarily proposing to change the default for
old-timers, but rather to allow differing behavior for "git push"
without remote depending on a config variable. So different behavior for
different people.

-Peff

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sun, 10 Feb 2008, Jeff King wrote:

> IOW, I am not necessarily proposing to change the default for 
> old-timers, but rather to allow differing behavior for "git push" 
> without remote depending on a config variable. So different behavior for 
> different people.

Hmm.  So that means that if an old-timer comes to help to a new-comer, 
the old-timer will be surprised?

You know I am a fan of consistency, so you know I cannot agree to your 
suggestion.

Ciao,
Dscho

Re: Minor annoyance with git push

[Jeff King]

On Sun, Feb 10, 2008 at 01:04:03PM +0000, Johannes Schindelin wrote:

> > IOW, I am not necessarily proposing to change the default for 
> > old-timers, but rather to allow differing behavior for "git push" 
> > without remote depending on a config variable. So different behavior for 
> > different people.
> 
> Hmm.  So that means that if an old-timer comes to help to a new-comer, 
> the old-timer will be surprised?

No, the old-timer will type what he means: "git push --matching origin"
or "git push origin refspec".

> You know I am a fan of consistency, so you know I cannot agree to your 
> suggestion.

I suspected you would say that. I agree that consistency has value, but
it can also get in the way when there really are two equally valid
approaches, and both should be available.

-Peff

Re: Minor annoyance with git push

[Junio C Hamano]

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

> Hmm.  So that means that if an old-timer comes to help to a new-comer, 
> the old-timer will be surprised?

Actually, after carefully examining what I have been doing, I
think this "somebody cries 'git push ;# nothing else' does not
work, and old timer needs to remember to check one extra thing
to help" has been a bit overblown.

To diagnose "git push ;# nothing else", you would already need
to ask the following _anyway_:

 - On what branch are you?

 - Do you have "branch.$current_branch.remote"?

 - What is "branch.$current_branch.remote" set to?

 - Do you have remote.$that_remote.url?

   - If it is http://, do you have very new version of curl
     library, often not even in distro?

   - If it is http://, does it end with '/'?

   - If it is http://, do you have DAV enabled over there?

   - If it is host:path, does your non-interactive ssh get
     appropriate PATH at the other end?

   - Is your username there the same as local?  Otherwise do you
     have "User your_name_over_there" set up in your
     $HOME/.ssh/config?

   - 47 other questions about transfers.

 - Do you have remote.$that_remote.push?

 - What local branches do you have?

 - What local branches does $that_remote have?

So it is not _so_ unreasonable if we add an extra configuration
or two to this mix.

What configuration semantics is reasonable is a different
matter.  Let's back up a bit.

The "matching" semantics has been advertised as a convenient way
to keep track of which branches you would want publish and which
ones to keep private without having to have extra configuration.
This is very true for people whose workflow is based on _owned_
public distribution points.  The destination is controlled by
you and you alone, and after pushing the branches you want to
show out explicitly, the destination repository _remembers_
which branches you are interested in publishing for later
"matching" pushes.  When you are no longer interested in showing
that work, you remove it from the remote and matching and that
(1) stops publishing and at the same time (2) remembers you are
no longer interested in publishing.

However, when you allow others to push into the destination, the
set of branches there obviously cannot serve as your private
configuration anymore.  So "matching" can never be a convenient
way if your push is to a shared repository.

Which would mean that "matching" is not a personal taste nor
even per-repository matter.  I think it is reasonable to set
this per-remote.

    You may have a repository you use during daytime on your
    desktop, and the repository may have more than one remotes
    defined.  "origin" may point to a shared repository you push to
    and fetch from in order to work together with others, "backup"
    may point to another repository on your server you push your
    work into, and "satellite" may point to yet another repository
    on your notebook when you are done for the day and take your WIP
    along with you.

    You may not want "matching" when interacting with the shared
    repository (if you always want "current only" is a different
    matter, but let's pretend you do), but you would want
    "matching" when pushing back to "satellite", and you would
    want "mirror" when pushing to "backup".

So perhaps we can have "remote.*.push" that says "current" in
some way.  Tentatively let's say the syntax is like this:

	[remote "origin"]
        	url = server.oss.example.org:/pub/project.git/
                push = HEAD
                fetch = +refs/heads/*:refs/remotes/origin/*
	[remote "backup"]
        	url = server.example.com:/home/me/mine.git/
                push = +refs/*:refs/*
	[remote "satellite"]
        	url = notebook.example.com:/home/me/mine.git/

	[branch "master"]
        	remote = "origin"
                merge = refs/heads/master
	[branch "ticket-47"]
               	remote = "origin"
                merge = refs/heads/ticket-47

While on "master", "git push" will say...

	Ah, you are too lazy to say where to push to.
	branch.master.remote says "origin", so that is it.

	And you are too lazy to say what to push either.
	remote.origin.push says HEAD which is a special token
	that means the current branch.

	Let's pretend you told me "git push origin master"

I was hoping we can do without the "remote.*.push = HEAD" if we
can detect the remote is a shared repository while talking to
it, but I think it is a bit too much magic, because we cannot
visualize what the pushing side and the receiving side  are
negotiating.

Putting this "push = HEAD" by default when "git clone" and "git
remote add" creates the [remote "$remote"] section is probably
possible, and at that stage we may even be able to do the "if
the other end is shared, then set this up automagically", as the
result of the magic can be inspected in the resulting config
file.

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Wed, 20 Feb 2008, Junio C Hamano wrote:

> Putting this "push = HEAD" by default when "git clone" and "git remote 
> add" creates the [remote "$remote"] section is probably possible, and at 
> that stage we may even be able to do the "if the other end is shared, 
> then set this up automagically", as the result of the magic can be 
> inspected in the resulting config file.

I think this is too magic, both of it.  Once people get used to "git push" 
being implicitly "git push origin HEAD", why should they not expect "git 
push <somewhere-else>" to push "HEAD" implicitly, too?

Ciao,
Dscho

Re: Minor annoyance with git push

[Jay Soffian]

On Feb 20, 2008 8:06 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> Hi,
>
> On Wed, 20 Feb 2008, Junio C Hamano wrote:
>
> > Putting this "push = HEAD" by default when "git clone" and "git remote
> > add" creates the [remote "$remote"] section is probably possible, and at
> > that stage we may even be able to do the "if the other end is shared,
> > then set this up automagically", as the result of the magic can be
> > inspected in the resulting config file.
>
> I think this is too magic, both of it.  Once people get used to "git push"
> being implicitly "git push origin HEAD", why should they not expect "git
> push <somewhere-else>" to push "HEAD" implicitly, too?

Well then, how about (don't cringe too much now...)

  push.conservative = true

If enabled and "git push" is run w/o arguments, it will first emit what
it plans to push and then prompt with "yes/no." I'm kinda opposed to
silly prompts -- folks just always go right past them -- so I dunno.

But it does make the operation a bit more safe I guess.

j.

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Wed, 20 Feb 2008, Jay Soffian wrote:

> On Feb 20, 2008 8:06 AM, Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> >
> > On Wed, 20 Feb 2008, Junio C Hamano wrote:
> >
> > > Putting this "push = HEAD" by default when "git clone" and "git 
> > > remote add" creates the [remote "$remote"] section is probably 
> > > possible, and at that stage we may even be able to do the "if the 
> > > other end is shared, then set this up automagically", as the result 
> > > of the magic can be inspected in the resulting config file.
> >
> > I think this is too magic, both of it.  Once people get used to "git 
> > push" being implicitly "git push origin HEAD", why should they not 
> > expect "git push <somewhere-else>" to push "HEAD" implicitly, too?
> 
> Well then, how about (don't cringe too much now...)
> 
>   push.conservative = true
> 
> If enabled and "git push" is run w/o arguments, it will first emit what 
> it plans to push and then prompt with "yes/no." I'm kinda opposed to 
> silly prompts -- folks just always go right past them -- so I dunno.
> 
> But it does make the operation a bit more safe I guess.

That depends awfully on your definition of "safe".

I, for one, hate the idea already, that I am "safe" when "git push" does 
not do the thing I asked it to, and which it has done for a couple of 
years now without complaint, and which I have gotten used to.

And then, there will be a great confusion for me, since I work on 5 
different machines on an average day, with 5 different git versions, and 
having different config settings.

That is not "safe" for me.

Thankyouverymuch,
Dscho

Re: Minor annoyance with git push

[Steven Walter]

On Wed, Feb 20, 2008 at 03:38:31PM +0000, Johannes Schindelin wrote:
> That depends awfully on your definition of "safe".
> 
> I, for one, hate the idea already, that I am "safe" when "git push" does 
> not do the thing I asked it to, and which it has done for a couple of 
> years now without complaint, and which I have gotten used to.
> 
> And then, there will be a great confusion for me, since I work on 5 
> different machines on an average day, with 5 different git versions, and 
> having different config settings.

Which is worse: pushing more refs than you intended (requiring rewinding
refs on a repository that other people may pull from), or pushing fewer
refs than you intended, requiring you to run the command a second time?
-- 
-Steven Walter <stevenrwalter@gmail.com>
Freedom is the freedom to say that 2 + 2 = 4
B2F1 0ECC E605 7321 E818  7A65 FC81 9777 DC28 9E8F 

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Thu, 21 Feb 2008, Steven Walter wrote:

> On Wed, Feb 20, 2008 at 03:38:31PM +0000, Johannes Schindelin wrote:
> > That depends awfully on your definition of "safe".
> > 
> > I, for one, hate the idea already, that I am "safe" when "git push" 
> > does not do the thing I asked it to, and which it has done for a 
> > couple of years now without complaint, and which I have gotten used 
> > to.
> > 
> > And then, there will be a great confusion for me, since I work on 5 
> > different machines on an average day, with 5 different git versions, 
> > and having different config settings.
> 
> Which is worse: pushing more refs than you intended (requiring rewinding 
> refs on a repository that other people may pull from), or pushing fewer 
> refs than you intended, requiring you to run the command a second time? 

Sorry to say, but I find this argumentation lacking.

Is it worse to suffer, or is it worse to suffer?  We should try to make it 
_safe_ not "less painful, but still painful nevertheless".

Hth,
Dscho

Re: Minor annoyance with git push

[Jeff King]

On Wed, Feb 20, 2008 at 12:23:06AM -0800, Junio C Hamano wrote:

> [long description of shared repository workflow]
> So perhaps we can have "remote.*.push" that says "current" in
> some way.  Tentatively let's say the syntax is like this:

I think everything you said here makes perfect sense; changing the push
refspec to say "push the current" for a particular remote is much more
sensible than an overall default. In fact, I half-expected this to
just work without a patch, since "git push origin HEAD" already works.
However, we don't treat command line refspecs and config refspecs the
same way, which IMHO is a needless inconsistency. How about this:

diff --git a/builtin-push.c b/builtin-push.c
index 9f727c0..ca90150 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -68,8 +68,7 @@ static int do_push(const char *repo, int flags)
 	if (!refspec
 		&& !(flags & TRANSPORT_PUSH_ALL)
 		&& remote->push_refspec_nr) {
-		refspec = remote->push_refspec;
-		refspec_nr = remote->push_refspec_nr;
+		set_refspecs(remote->push_refspec, remote->push_refspec_nr);
 	}
 	errs = 0;
 	for (i = 0; i < remote->url_nr; i++) {

At which point this now works as you described:

  git config remote.origin.push HEAD

> I was hoping we can do without the "remote.*.push = HEAD" if we
> can detect the remote is a shared repository while talking to
> it, but I think it is a bit too much magic, because we cannot
> visualize what the pushing side and the receiving side  are
> negotiating.

How are you detecting that the remote is a shared repository? By the
core.sharedrepository config option? I use several shared repositories,
and I never set that variable; instead I use filesystem ACLs (which we
could at least detect). It is my understanding that some people even
have repositories where multiple users share the same filesystem uid but
connect with different ssh keys.  I don't think that is even detectable.

-Peff

Re: Minor annoyance with git push

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... In fact, I half-expected this to
> just work without a patch, since "git push origin HEAD" already works.
> However, we don't treat command line refspecs and config refspecs the
> same way, which IMHO is a needless inconsistency. How about this:
>
> diff --git a/builtin-push.c b/builtin-push.c
> index 9f727c0..ca90150 100644
> --- a/builtin-push.c
> +++ b/builtin-push.c
> @@ -68,8 +68,7 @@ static int do_push(const char *repo, int flags)
>  	if (!refspec
>  		&& !(flags & TRANSPORT_PUSH_ALL)
>  		&& remote->push_refspec_nr) {
> -		refspec = remote->push_refspec;
> -		refspec_nr = remote->push_refspec_nr;
> +		set_refspecs(remote->push_refspec, remote->push_refspec_nr);
>  	}
>  	errs = 0;
>  	for (i = 0; i < remote->url_nr; i++) {

Yeah, we are on the same page.  See the patch I sent out last night ;-)

> At which point this now works as you described:
>
>   git config remote.origin.push HEAD

Exactly.

>> I was hoping we can do without the "remote.*.push = HEAD" if we
>> can detect the remote is a shared repository while talking to
>> it, but I think it is a bit too much magic, because we cannot
>> visualize what the pushing side and the receiving side  are
>> negotiating.
>
> How are you detecting that the remote is a shared repository?

I am not.  I only said "... may even be able to" ;-).

The autodetection is not even the first step to tackle this
issue anyway.  The "HEAD" magic (or if somebody comes up with a
better design, that one) comes first, deciding if such an
autodetection is even a good idea comes next, and then iff we
decide that it is a good idea finally comes the task of finding
out how we do so.

I'd presume that the upload-pack side can internally check "int
shared_repository" and the protocol extension that conveys that
information to the other end is easy enough.  As always, dumb
transports are second class citizens and need their own hacks.

> By the core.sharedrepository config option? I use several
> shared repositories, and I never set that variable; instead
> ...

It is _your_ problem that you do not use published interface,
isn't it?

Re: Minor annoyance with git push

[Jeff King]

On Wed, Feb 20, 2008 at 09:54:35AM -0800, Junio C Hamano wrote:

> > +++ b/builtin-push.c
> > @@ -68,8 +68,7 @@ static int do_push(const char *repo, int flags)
> >  	if (!refspec
> >  		&& !(flags & TRANSPORT_PUSH_ALL)
> >  		&& remote->push_refspec_nr) {
> > -		refspec = remote->push_refspec;
> > -		refspec_nr = remote->push_refspec_nr;
> > +		set_refspecs(remote->push_refspec, remote->push_refspec_nr);
> >  	}
> >  	errs = 0;
> >  	for (i = 0; i < remote->url_nr; i++) {
> 
> Yeah, we are on the same page.  See the patch I sent out last night ;-)

Meaning you will take this patch? (And yes, I think the "+HEAD" patch
you sent is a good fix, regardless of everything else we've talked
about).

> > How are you detecting that the remote is a shared repository?
> 
> I am not.  I only said "... may even be able to" ;-).
> 
> The autodetection is not even the first step to tackle this
> issue anyway.  The "HEAD" magic (or if somebody comes up with a
> better design, that one) comes first, deciding if such an
> autodetection is even a good idea comes next, and then iff we
> decide that it is a good idea finally comes the task of finding
> out how we do so.

Didn't the "HEAD" magic just come? :) Is there some part of that that
you are unhappy with?

> I'd presume that the upload-pack side can internally check "int
> shared_repository" and the protocol extension that conveys that
> information to the other end is easy enough.  As always, dumb
> transports are second class citizens and need their own hacks.

Reasonable.

> > By the core.sharedrepository config option? I use several
> > shared repositories, and I never set that variable; instead
> > ...
> 
> It is _your_ problem that you do not use published interface,
> isn't it?

I would agree with you if core.sharedRepository had ever been introduced
as a signal flag for a particular workflow, and not simply as a way to
set up the umask.

If you want to introduce those semantics to sharedRepository, that is
not unreasonable, but:

  - recognize that you don't automagically turn on this feature for
    repositories using the shared workflow, since they may or may not
    even have this flag enabled

  - recognize that people who _do_ want this new behavior might not want
    the umask side effects (and in fact, those side effects can reduce
    security if the users are in a group with untrusted users)

so I think that a new config option is probably safer, and not
necessarily less likely to work automatically.

That being said, I think this sort of automatic detection should wait at
least one release cycle. Once the behavior is configurable at all, we
can see how people adopt it into their workflows and if there is really
any desire for detection at all.

-Peff

Re: Minor annoyance with git push

[Jeff King]

On Wed, Feb 20, 2008 at 01:15:14PM -0500, Jeff King wrote:

> > Yeah, we are on the same page.  See the patch I sent out last night ;-)
> 
> Meaning you will take this patch? (And yes, I think the "+HEAD" patch
> you sent is a good fix, regardless of everything else we've talked
> about).

Nevermind this question if you are going to take Daniel's patch to move
the ref resolution to the lower layer.

-Peff

Re: Minor annoyance with git push

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Meaning you will take this patch? (And yes, I think the "+HEAD" patch
> you sent is a good fix, regardless of everything else we've talked
> about).

Yes I would, except that I think Daniel's is much better and is
to the point.  Doesn't it cover all the issues we discussed so
far?

> ... Once the behavior is configurable at all, we
> can see how people adopt it into their workflows and if there is really
> any desire for detection at all.

Yup.

Re: Minor annoyance with git push

[Jeff King]

On Wed, Feb 20, 2008 at 10:19:41AM -0800, Junio C Hamano wrote:

> Yes I would, except that I think Daniel's is much better and is
> to the point.  Doesn't it cover all the issues we discussed so
> far?

Yes, I like Daniel's patch (it came right after I wrote my response).

-Peff

Re: Minor annoyance with git push

[Wincent Colaiuta]

El 10/2/2008, a las 3:15, Johannes Schindelin escribió:

> I'm no longer that sure.  It seems that quite a lot of people do not  
> read
> manuals, and have no clue what they are doing when they just try
>
> 	$ git push
>
> to see what the synopsis is.

I think there's no way we should be catering for people who type  
command like "git push" just to see what the synopsis does.

The verb "push" very clearly has connotations of a state-changing,  
possibly irreversible action (unlike other verbs like "log" or "show").

People who type "git push" just to see a synopsis need to learn a  
lesson; the lesson being that if you want to find out what a command  
does the safest thing is to type "git help push" instead.

Cheers,
Wincent

Re: Minor annoyance with git push

[Steven Walter]

On Sun, Feb 10, 2008 at 03:03:13PM +0100, Wincent Colaiuta wrote:
> I think there's no way we should be catering for people who type command 
> like "git push" just to see what the synopsis does.
>
> The verb "push" very clearly has connotations of a state-changing, possibly 
> irreversible action (unlike other verbs like "log" or "show").

I think you're quite correct: push is a state changing action.  For that
reason, it should default to the smallest amount of work conceivably
envisioned by the user.  To do more work than the user may have meant is
asking for trouble.

Take git-commit as a parallel.  You make some file changes and run
"git commit".  Nothing happens.  If you wanted to commit all your
changes, you need the -a flag.  Here, subversion takes the opposite
approach and commits all your files, when in fact you may have only
wanted a smaller subset.  I have seen this cause problems many times
(and more than once, I was the problem-maker :) )

> People who type "git push" just to see a synopsis need to learn a lesson; 
> the lesson being that if you want to find out what a command does the 
> safest thing is to type "git help push" instead.

This is an interesting theory on user-interface design.


I propose a new configuration option, remote.*.pushAllRefs, defaulting
to off.  When pushAllRefs is false, "git push" pushes only the current
branch.  When pushAllRefs is true, "git push" does what it does today.
For the old-timers, the impact of such a change seems minimal.
Worst-case, they run "git push," it doesn't do what they expect, they run
"git push origin" and then go change their config files.

Thoughts?
-- 
-Steven Walter <stevenrwalter@gmail.com>
Freedom is the freedom to say that 2 + 2 = 4
B2F1 0ECC E605 7321 E818  7A65 FC81 9777 DC28 9E8F 

Re: Minor annoyance with git push

[Johannes Schindelin]

Hi,

On Sun, 10 Feb 2008, Steven Walter wrote:

> I propose a new configuration option, remote.*.pushAllRefs, defaulting 
> to off.  When pushAllRefs is false, "git push" pushes only the current 
> branch.  When pushAllRefs is true, "git push" does what it does today. 
> For the old-timers, the impact of such a change seems minimal. 
> Worst-case, they run "git push," it doesn't do what they expect, they 
> run "git push origin" and then go change their config files.

I don't like it.

This would mean that I would have to look into the config everytime before 
I push, to be sure what semantics the push has in _this_ repository.

_If_ we change git-push's default behaviour, we have to introduce an 
option to do what the default action is now, _first_!

And _then_ we can start discussing what the default action should be, and 
I tend to think that it would make sense to default to the synopsis.

But that comes _second_, and we'd have to agree on _one_ course of action.

I mean, if you do not like the default behaviour of git-push now, you can 
always introduce an alias, if you want repository/host-specific actions.

Ciao,
Dscho

Re: Minor annoyance with git push

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 767 bytes --]

Hi,

On Sun, 10 Feb 2008, Wincent Colaiuta wrote:

> El 10/2/2008, a las 3:15, Johannes Schindelin escribió:
> 
> > I'm no longer that sure.  It seems that quite a lot of people do not 
> > read manuals, and have no clue what they are doing when they just try
> > 
> > 	$ git push
> > 
> > to see what the synopsis is.
> 
> I think there's no way we should be catering for people who type command 
> like "git push" just to see what the synopsis does.
> 
> The verb "push" very clearly has connotations of a state-changing, 
> possibly irreversible action (unlike other verbs like "log" or "show").

The problem is that "push" alone does not really imply which kind of push.

Besides, not everybody can be expected to be such an uber-git as you and 
me.

Ciao,
Dscho

Re: Minor annoyance with git push

[Wincent Colaiuta]

El 10/2/2008, a las 17:26, Johannes Schindelin escribió:

> On Sun, 10 Feb 2008, Wincent Colaiuta wrote:
>
>> El 10/2/2008, a las 3:15, Johannes Schindelin escribió:
>>
>>> I'm no longer that sure.  It seems that quite a lot of people do not
>>> read manuals, and have no clue what they are doing when they just  
>>> try
>>>
>>> 	$ git push
>>>
>>> to see what the synopsis is.
>>
>> I think there's no way we should be catering for people who type  
>> command
>> like "git push" just to see what the synopsis does.
>>
>> The verb "push" very clearly has connotations of a state-changing,
>> possibly irreversible action (unlike other verbs like "log" or  
>> "show").
>
> The problem is that "push" alone does not really imply which kind of  
> push.

Yes, I know. I myself was surprised by the default behaviour the first  
time I used "git push" (I only expected it to push the branch I was  
currently on).

But my point is that if you don't know what "git push" is going to do  
because its name doesn't imply "which kind of push" it will do (and in  
reality a newcomer might not even realize that there might be more  
than one kind of push), then adopting a "try it and see" approach  
("let's type 'git push' and see if it gives me a synopsis") is not a  
very good idea, and in a case like this where "push" is what I'd call  
a "strong" verb, I don't think we should be trying to protect the user  
from doing something obviously idiotic.

I'm all for protecting the user from nasty surprises (like "git  
clean"; "clean" doesn't sound nearly as destructive as it can actually  
turn out to be) but I don't think that anyone typing "git push" can  
fairly claim to be surprised when Git goes ahead and, er, pushes  
something.

Cheers,
Wincent

Re: Minor annoyance with git push

[Jeff King]

On Sun, Feb 10, 2008 at 07:18:24PM +0100, Wincent Colaiuta wrote:

> I'm all for protecting the user from nasty surprises (like "git clean"; 
> "clean" doesn't sound nearly as destructive as it can actually turn out to 
> be) but I don't think that anyone typing "git push" can fairly claim to be 
> surprised when Git goes ahead and, er, pushes something.

I don't think people are surprised that "git push" pushes something. I
think they are surprised that "git push" makes changes based on non-HEAD
branches (which may or may not be in a useful state). Are there any
other git commands which use non-HEAD branches that have not been
explicitly mentioned by the user?  I can think only of query-type
commands (like show-branch, or describe) that are non-state-changing.

I think what is problematic here is that push makes changes based on
state that is irrelevant to most git actions. Thus you have things like
Martin's complaint: "what, why are you rejecting branch foo? I haven't
even worked on that in months." He is expected to either remember the
state of all branches in his repository, or to keep it extremely tidy
(either deleting branches immediately after use, or keeping them up to
date with upstream, which is extra useless work from his perspective).

-Peff

Re: Minor annoyance with git push

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> ... Are there any
> other git commands which use non-HEAD branches that have not been
> explicitly mentioned by the user?  I can think only of query-type
> commands (like show-branch, or describe) that are non-state-changing.

That's an irrelevant comparison.  push and fetch have always
been multi-branch operations by default from day one.  The issue
is not HEAD vs non-HEAD.

You can argue that historically established practices do not
matter at all (at least to new people), and I'd grant that it
may be a valid argument.  But a change that breaks existing
practices needs to be sold much more carefully.  I still do not
understand what the opposition is to keep the current behaviour
as the default and have a shorthand for the single head push
accessible with a short and sweet "git push $there HEAD" (and
default $there to 'origin' when missing).

If you are introducing a new behaviour, there is no way the new
behaviour can start out by replacing the longtime default.  It
should start out as an option, and if it is a commonly useful
option then make it an _easily accessible_ option.  And accept
such an _enhancement_ sooner to help people who want such a
behaviour sooner.  That would not hurt anybody but help
(hopefully) many people, without downside.

Switching the default behaviour is a much longer term thing.  It
definitely has downside people mentioned in this thread.

Re: Minor annoyance with git push

[Jeff King]

On Sun, Feb 10, 2008 at 02:59:18PM -0800, Junio C Hamano wrote:

> > ... Are there any
> > other git commands which use non-HEAD branches that have not been
> > explicitly mentioned by the user?  I can think only of query-type
> > commands (like show-branch, or describe) that are non-state-changing.
> 
> That's an irrelevant comparison.  push and fetch have always
> been multi-branch operations by default from day one.  The issue
> is not HEAD vs non-HEAD.

I think "irrelevant" here is in the eye of the beholder. If you are
coming from the perspective of "historical behavior" then the current
behavior makes sense. If you are coming from the perspective of a user
who does not typically do a lot of branching, then the behavior can be
surprising. And I think there is anecdotal evidence that some users _do_
find this surprising. I have personally seen it, and I think even an
experienced user like Martin is finding the behavior non-intuitive.

> You can argue that historically established practices do not
> matter at all (at least to new people), and I'd grant that it
> may be a valid argument.  But a change that breaks existing
> practices needs to be sold much more carefully.  I still do not

Agreed.

> understand what the opposition is to keep the current behaviour
> as the default and have a shorthand for the single head push
> accessible with a short and sweet "git push $there HEAD" (and
> default $there to 'origin' when missing).

I think the problem is that you are asking people with particular
workflows to always remember to type something extra, and if they
forget, you punish them by doing a potentially destructive operation
(although admittedly, it is destructive only if you happen to be using
"-f").

> If you are introducing a new behaviour, there is no way the new
> behaviour can start out by replacing the longtime default.  It
> should start out as an option, and if it is a commonly useful
> option then make it an _easily accessible_ option.  And accept
> such an _enhancement_ sooner to help people who want such a
> behaviour sooner.  That would not hurt anybody but help
> (hopefully) many people, without downside.

I think we already have that option (as you mentioned): "git push $there
HEAD". We can make it more easily accessible (as you have proposed):
"git push HEAD" (though I still have some reservations about that). But
none of that changes the fact that for some people's workflows, they
will _always_ have to remember to add extra magic to the command line.

What I have proposed is adding a config option to make that option even
more easily accessible. The only argument I have seen against that is
"some users can't use other users' git setup". While this is a downside,
I think this uncommon situation is outweighed by the very common
situation of users using their _own_ git setups.

IOW, such an option makes a tradeoff. Users helping other users must
explicitly say what they mean: "git push --matching" or "git push $there
HEAD". But users using their own setups can use the shorthand "git push"
to do what is most useful for their workflow. And I think we should
optimize for the latter case, as it is much more common.

-Peff