Git development
 help / color / mirror / Atom feed
* Re: [RFC/PATCH 4/5] completion: get rid of compgen
From: Felipe Contreras @ 2012-11-18  0:53 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: git, Junio C Hamano, Björn Gustavsson, Shawn O. Pearce,
	Robert Zeh, Peter van der Does, Jonathan Nieder, Jeff King
In-Reply-To: <CAMP44s0YnoGwJEgUbXZ831_OrgO=dDf5_QHxT5JYnGUHYPuiTw@mail.gmail.com>

On Sat, Nov 17, 2012 at 8:33 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote:
>>> On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>> > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
>>> >> The functionality we use is very simple, plus, this fixes a known
>>> >> breakage 'complete tree filename with metacharacters'.
>>> >>
>>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>>> >> ---
>>> >>  contrib/completion/git-completion.bash | 6 +++++-
>>> >>  1 file changed, 5 insertions(+), 1 deletion(-)
>>> >>
>>> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>>> >> index 975ae13..ad3e1fe 100644
>>> >> --- a/contrib/completion/git-completion.bash
>>> >> +++ b/contrib/completion/git-completion.bash
>>> >> @@ -227,7 +227,11 @@ fi
>>> >>
>>> >>  __gitcompadd ()
>>> >>  {
>>> >> -     COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
>>> >> +     for x in $1; do
>>> >> +             if [[ "$x" = "$3"* ]]; then
>>> >> +                     COMPREPLY+=("$2$x$4")
>>> >> +             fi
>>> >> +     done
>>> >
>>> > The whole point of creating __gitcomp_nl() back then was to fill
>>> > COMPREPLY without iterating through all words in the wordlist, making
>>> > completion faster for large number of words, e.g. a lot of refs, or
>>> > later a lot of symbols for 'git grep' in a larger project.
>>> >
>>> > The loop here kills that optimization.
>>>
>>> So your solution is to move the loop to awk? I fail to see how that
>>> could bring more optimization, specially since it includes an extra
>>> fork now.
>>
>> This patch didn't aim for more optimization, but it was definitely a
>> goal not to waste what we gained by creating __gitcomp_nl() in
>> a31e6262 (completion: optimize refs completion, 2011-10-15).  However,
>> as it turns out the new version with awk is actually faster than
>> current master with compgen:
>>
>>   Before:
>>
>>     $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
>>     $ time __gitcomp_nl "$refs"
>>
>>     real    0m0.242s
>>     user    0m0.220s
>>     sys     0m0.028s
>>
>>   After:
>>
>>     $ time __gitcomp_nl "$refs"
>>
>>     real    0m0.109s
>>     user    0m0.096s
>>     sys     0m0.012s
>
> This one is even faster:
>
> while read -r x; do
>         if [[ "$x" == "$3"* ]]; then
>                 COMPREPLY+=("$2$x$4")
>         fi
> done <<< $1
>
> == 10000 ==
> one:
> real    0m0.148s
> user    0m0.134s
> sys     0m0.025s
> two:
> real    0m0.055s
> user    0m0.054s
> sys     0m0.000s

Ah, nevermind, I didn't quote the $1.

However, this one is quite close and much simpler:

	local IFS=$'\n'
	COMPREPLY=($(printf -- "$2%s$4\n" $1 | grep "^$2$3"))

== 10000 ==
awk 1:
real	0m0.064s
user	0m0.062s
sys	0m0.003s
printf:
real	0m0.069s
user	0m0.064s
sys	0m0.020s


-- 
Felipe Contreras

^ permalink raw reply

* Re: [PATCH 4/7] completion: add tests for invalid variable name among completion words
From: Jonathan Nieder @ 2012-11-17 23:40 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Felipe Contreras, Jeff King, Junio C Hamano
In-Reply-To: <1353150353-29874-5-git-send-email-szeder@ira.uka.de>

SZEDER Gábor wrote:

>                                                      The breakage can
> be simply bogus possible completion words, but it can also be a
> failure:
>
>   $ git branch '${foo.bar}'
>   $ git checkout <TAB>
>   bash: ${foo.bar}: bad substitution

Or arbitrary code execution:

	$ git branch '$(>kilroy-was-here)'
	$ git checkout <TAB>
	$ ls -l kilroy-was-here
	-rw-rw-r-- 1 jrn jrn 0 nov 17 15:40 kilroy-was-here

The final version of this patch should go to maint.  Thanks for
catching it.

^ permalink raw reply

* Re: [PATCH 3/7] completion: add tests for the __gitcomp_nl() completion helper function
From: Jonathan Nieder @ 2012-11-17 23:31 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Felipe Contreras, Jeff King, Junio C Hamano
In-Reply-To: <1353150353-29874-4-git-send-email-szeder@ira.uka.de>

SZEDER Gábor wrote:

> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -155,6 +155,90 @@ test_expect_success '__gitcomp - suffix' '
>  	test_cmp expected out
>  '
>  
> +test_expect_success '__gitcomp_nl - trailing space' '
> +	sed -e "s/Z$//" >expected <<-\EOF &&

'$' is usually a shell metacharacter, so it would be more comfortable
to read a version that escapes it:

	sed -e "s/Z\$//" >expected <<-\EOF

Since '$/' is not a valid parameter expansion, if I understand
correctly then POSIX leaves the meaning of the quoted string "s/Z$//"
undefined (XCU §2.2.3).  Luckily every shell I've tried, including
bash, keeps the $ unmolested.

Other parts of the file already use the same style, so I wouldn't
suggest changing it in this patch.

Thanks for some nice tests.

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

^ permalink raw reply

* Re: [PATCH 2/7] completion: fix args of run_completion() test helper
From: Jonathan Nieder @ 2012-11-17 23:02 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Felipe Contreras, Jeff King, Junio C Hamano
In-Reply-To: <1353150353-29874-3-git-send-email-szeder@ira.uka.de>

SZEDER Gábor wrote:

> Fix this by passing the command line to run_completion() as separate
> words.

Good catch.  The change makes sense, the code looks saner after the
fix, and since this is test code any breakage should be caught
quickly, so

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

^ permalink raw reply

* Re: [PATCH 1/7] completion: make the 'basic' test more tester-friendly
From: Jonathan Nieder @ 2012-11-17 23:00 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Felipe Contreras, Jeff King, Junio C Hamano
In-Reply-To: <1353150353-29874-2-git-send-email-szeder@ira.uka.de>

SZEDER Gábor wrote:

> The 'basic' test uses 'grep -q' to filter the resulting possible
> completion words while looking for the presence or absence of certain
> git commands, and relies on grep's exit status to indicate a failure.
[...]
> To make testers' life easier provide some output about the failed
> condition: store the results of the filtering in a file and compare
> its contents to the expected results by the good old test_cmp helper.

Looks good.  I wonder if this points to the need for a test_grep
helper more generally.

[...]
>  	run_completion "git f" &&
> -	! grep -q -v "^f" out
> +	>expected &&
> +	sed -n "/^[^f]/p" out >out2 &&
> +	test_cmp expected out2

Functional change: before, this would fail if "out" contained a blank
line, while afterward it does not.  I doubt that matters, though.

Thanks and hope that helps,
Jonathan

^ permalink raw reply

* Re: Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
From: W. Trevor King @ 2012-11-17 22:00 UTC (permalink / raw)
  To: Heiko Voigt
  Cc: Junio C Hamano, Git, Jeff King, Phil Hord, Shawn Pearce,
	Jens Lehmann, Nahor
In-Reply-To: <20121117213130.GC7695@book.hvoigt.net>

[-- Attachment #1: Type: text/plain, Size: 2212 bytes --]

On Sat, Nov 17, 2012 at 10:31:30PM +0100, Heiko Voigt wrote:
> On Sat, Nov 17, 2012 at 02:20:27PM -0500, W. Trevor King wrote:
> > On Sat, Nov 17, 2012 at 04:30:07PM +0100, Heiko Voigt wrote:
> > > > >  (2) "git diff [$path]" and friends in the superproject compares the
> > > > >      HEAD of thecheckout of the submodule at $path with the tip of
> > > > >      the branch named by submodule.$name.branch in .gitmodules of
> > > > >      the superproject, instead of the commit that is recorded in the
> > > > >      index of the superproject.
> > > > > 
> > > > 
> > > > Hmm.  ???git diff??? compares the working tree with the local HEAD (just a
> > > > SHA for submodules), so I don't think it should care about the status
> > > > of a remote branch.  This sounds like you want something like:
> > > > 
> > > >   $ git submodule foreach 'git diff origin/$submodule_branch'
> > > > 
> > > > Perhaps this is enough motivation for keeping $submodule_* exports?
> > > > 
> > > > > and the option were called something like "--follow-branch=$branch",
> > > > > ???
> > > 
> > > I am not sure if hiding changes to the recorded SHA1 from the user is
> > > such a useful thing. In the first step I would like it if it was kept
> > > simple and only the submodule update machinery learned to follow a
> > > branch. If that results in local changes that should be shown. The user
> > > is still in charge of recording the updated SHA1 in his commit.
> > 
> > I understand what you're warning against here, or what it has to do
> > with "git diff".
> 
> Is there a not missing here?

Thanks.  I'd meant to say "I don't understand…".

> What I am talking about is the suggestion of Junio.  Instead of
> showing a diff if the SHA1 is different we show a diff if the
> checkout in the worktree is different from the tip of the configured
> branch. That would hide the fact that a submodule has changed during
> a submodule update operation.

Ahh, now I understand.  I agree that comparing to the remote tip is a
bad idea.

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* [PATCH v4.1 5/5] push: update remote tags only with force
From: Chris Rorvick @ 2012-11-17 21:53 UTC (permalink / raw)
  To: git
  Cc: Chris Rorvick, Angelo Borsotti, Drew Northup, Michael Haggerty,
	Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
	Felipe Contreras, Junio C Hamano
In-Reply-To: <1353183397-17719-6-git-send-email-chris@rorvick.com>

References are allowed to update from one commit-ish to another if the
former is a ancestor of the latter.  This behavior is oriented to
branches which are expected to move with commits.  Tag references are
expected to be static in a repository, though, thus an update to a
tag (lightweight and annotated) should be rejected unless the update is
forced.

To enable this functionality, the following checks have been added to
set_ref_status_for_push() for updating refs (i.e, not new or deletion)
to restrict fast-forwarding in pushes:

  1) The old and new references must be commits.  If this fails,
     it is not a valid update for a branch.

  2) The reference name cannot start with "refs/tags/".  This
     catches lightweight tags which (usually) point to commits
     and therefore would not be caught by (1).

If either of these checks fails, then it is flagged (by default) with a
status indicating the update is being rejected due to the reference
already existing in the remote.  This can be overridden by passing
--force to git push.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---

Fix C99 comment.

 Documentation/git-push.txt | 10 +++++-----
 builtin/push.c             |  2 +-
 builtin/send-pack.c        |  5 +++++
 cache.h                    |  3 ++-
 remote.c                   | 24 ++++++++++++++++++++----
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 42 +++++++++++++++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                |  8 ++++++--
 9 files changed, 87 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fe46c42..479e25f 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be
 updated.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side, but by default this is only allowed if the
-update can fast-forward <dst>.  By having the optional leading `+`,
-you can tell git to update the <dst> ref even when the update is not a
-fast-forward.  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+on the remote side.  By default this is only allowed if the update is
+a branch, and then only if it can fast-forward <dst>.  By having the
+optional leading `+`, you can tell git to update the <dst> ref even when
+the update is not a branch or it is not a fast-forward.  This does *not*
+attempt to merge <src> into <dst>.  See EXAMPLES below for details.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/builtin/push.c b/builtin/push.c
index 0e13e11..cd7aa3f 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -222,7 +222,7 @@ static const char message_advice_checkout_pull_push[] =
 
 static const char message_advice_ref_already_exists[] =
 	N_("Updates were rejected because a matching reference already exists in\n"
-	   "the remote and the update is not a fast-forward.");
+	   "the remote.");
 
 static void advise_pull_before_push(void)
 {
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index fda28bc..1eabf42 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
+			res = "error";
+			msg = "already exists";
+			break;
+
 		case REF_STATUS_REJECT_NODELETE:
 		case REF_STATUS_REMOTE_REJECT:
 			res = "error";
diff --git a/cache.h b/cache.h
index f410d94..127e504 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,13 +1004,14 @@ struct ref {
 		requires_force:1,
 		merge:1,
 		nonfastforward:1,
-		is_a_tag:1,
+		forwardable:1,
 		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
 		REF_STATUS_OK,
 		REF_STATUS_REJECT_NONFASTFORWARD,
+		REF_STATUS_REJECT_ALREADY_EXISTS,
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
diff --git a/remote.c b/remote.c
index 44e72d6..a723f7a 100644
--- a/remote.c
+++ b/remote.c
@@ -1311,14 +1311,24 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     to overwrite it; you would not know what you are losing
 		 *     otherwise.
 		 *
-		 * (3) if both new and old are commit-ish, and new is a
-		 *     descendant of old, it is OK.
+		 * (3) if new is commit-ish and old is a commit, new is a
+		 *     descendant of old, and the reference is not of the
+		 *     refs/tags/ hierarchy it is OK.
 		 *
 		 * (4) regardless of all of the above, removing :B is
 		 *     always allowed.
 		 */
 
-		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
+		if (prefixcmp(ref->name, "refs/tags/")) {
+			/* Additionally, disallow fast-forwarding if
+			 * the old object is not a commit.  This kicks
+			 * out annotated tags that might pass the
+			 * is_newer() test but dangle if the reference
+			 * is updated.
+			 */
+			struct object *obj = parse_object(ref->old_sha1);
+			ref->forwardable = !obj || obj->type == OBJ_COMMIT;
+		}
 
 		ref->update =
 			!ref->deletion &&
@@ -1329,7 +1339,13 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 				!has_sha1_file(ref->old_sha1)
 				  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-			if (ref->nonfastforward) {
+			if (!ref->forwardable) {
+				ref->requires_force = 1;
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+					continue;
+				}
+			} else if (ref->nonfastforward) {
 				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
diff --git a/send-pack.c b/send-pack.c
index f50dfd9..1c375f0 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -229,6 +229,7 @@ int send_pack(struct send_pack_args *args,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..ca800b2 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -368,7 +368,7 @@ test_expect_success 'push with colon-less refspec (2)' '
 		git branch -D frotz
 	fi &&
 	git tag -f frotz &&
-	git push testrepo frotz &&
+	git push -f testrepo frotz &&
 	check_push_result $the_commit tags/frotz &&
 	check_push_result $the_first_commit heads/frotz
 
@@ -929,6 +929,46 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
+test_expect_success 'push requires --force to update lightweight tag' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git tag Tag &&
+		git push ../child2 Tag &&
+		>file1 &&
+		git add file1 &&
+		git commit -m "file1" &&
+		git tag -f Tag &&
+		test_must_fail git push ../child2 Tag &&
+		git push --force ../child2 Tag &&
+		git tag -f Tag &&
+		test_must_fail git push ../child2 Tag HEAD~ &&
+		git push --force ../child2 Tag
+	)
+'
+
+test_expect_success 'push requires --force to update annotated tag' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git tag -a -m "message 1" Tag &&
+		git push ../child2 Tag:refs/tmp/Tag &&
+		>file1 &&
+		git add file1 &&
+		git commit -m "file1" &&
+		git tag -f -a -m "message 2" Tag &&
+		test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
+		git push --force ../child2 Tag:refs/tmp/Tag &&
+		git tag -f -a -m "message 3" Tag HEAD~ &&
+		test_must_fail git push ../child2 Tag:refs/tmp/Tag &&
+		git push --force ../child2 Tag:refs/tmp/Tag
+	)
+'
+
 test_expect_success 'push --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&
diff --git a/transport-helper.c b/transport-helper.c
index 4713b69..965b778 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -661,6 +661,11 @@ static void push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "already exists")) {
+			status = REF_STATUS_REJECT_ALREADY_EXISTS;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
@@ -720,6 +725,7 @@ static int push_refs_with_push(struct transport *transport,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/transport.c b/transport.c
index 60a7421..a380ad7 100644
--- a/transport.c
+++ b/transport.c
@@ -695,6 +695,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "non-fast-forward", porcelain);
 		break;
+	case REF_STATUS_REJECT_ALREADY_EXISTS:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "already exists", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -740,12 +744,12 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
-			if (!ref->is_a_tag)
-				*reject_mask |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
 				*reject_mask |= REJECT_NON_FF_HEAD;
 			else
 				*reject_mask |= REJECT_NON_FF_OTHER;
+		} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
+			*reject_mask |= REJECT_ALREADY_EXISTS;
 		}
 	}
 }
-- 
1.7.11.7

^ permalink raw reply related

* Re: Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
From: Heiko Voigt @ 2012-11-17 21:31 UTC (permalink / raw)
  To: W. Trevor King
  Cc: Junio C Hamano, Git, Jeff King, Phil Hord, Shawn Pearce,
	Jens Lehmann, Nahor
In-Reply-To: <20121117192026.GI22234@odin.tremily.us>

On Sat, Nov 17, 2012 at 02:20:27PM -0500, W. Trevor King wrote:
> On Sat, Nov 17, 2012 at 04:30:07PM +0100, Heiko Voigt wrote:
> > > >  (2) "git diff [$path]" and friends in the superproject compares the
> > > >      HEAD of thecheckout of the submodule at $path with the tip of
> > > >      the branch named by submodule.$name.branch in .gitmodules of
> > > >      the superproject, instead of the commit that is recorded in the
> > > >      index of the superproject.
> > > > 
> > > 
> > > Hmm.  ???git diff??? compares the working tree with the local HEAD (just a
> > > SHA for submodules), so I don't think it should care about the status
> > > of a remote branch.  This sounds like you want something like:
> > > 
> > >   $ git submodule foreach 'git diff origin/$submodule_branch'
> > > 
> > > Perhaps this is enough motivation for keeping $submodule_* exports?
> > > 
> > > > and the option were called something like "--follow-branch=$branch",
> > > > ???
> > 
> > I am not sure if hiding changes to the recorded SHA1 from the user is
> > such a useful thing. In the first step I would like it if it was kept
> > simple and only the submodule update machinery learned to follow a
> > branch. If that results in local changes that should be shown. The user
> > is still in charge of recording the updated SHA1 in his commit.
> 
> I understand what you're warning against here, or what it has to do
> with "git diff".

Is there a not missing here? Reads somehow like that. What I am talking
about is the suggestion of Junio.  Instead of showing a diff if the
SHA1 is different we show a diff if the checkout in the worktree is
different from the tip of the configured branch. That would hide the
fact that a submodule has changed during a submodule update operation.

> > From what I have heard of projects using this: They usually still have
> > something that records the SHA1s on a regular basis. Thinking further,
> > why not record them in git? We could add an option to update which
> > creates such a commit.
> 
> I think it's best to have users craft their own commit messages
> explaining why the branch was updated.  That said, an auto-generated
> hint (a la "git merge") would probably be a useful extra feature.

I have the same opinion. Commits should always be created by humans so
you have someone to blame/ask why. But I guess there are people that
expect this to be automatic.

One argument somehow goes along the lines:
"I already created a commit in the submodule why do I need to create
another one in the superproject? Just follow the HEAD revision!" They
think in subversions "submodules" which are merely pointers to other svn
repositories without any revision information. I am unsure if its good
to support this the same way.

Another use case is big projects that have so many submodules that
creating superproject commits would create to much maintenance work.
They want to have their integration server make those commits. That
would already be supported with update checking out the branch tips and
the commit is just one extra thing to do by the integration server.

So I think it should be fine just to teach update to checkout the
configured branch tips (or forward them to their tracking branch tips)
and leave the rest to the user.

Cheers Heiko

^ permalink raw reply

* Re: [PATCH v2 4/6] completion: consolidate test_completion*() tests
From: Felipe Contreras @ 2012-11-17 20:26 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Jeff King, SZEDER Gábor, Jonathan Nieder
In-Reply-To: <7v7gplktdt.fsf@alter.siamese.dyndns.org>

On Sat, Nov 17, 2012 at 12:41 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> No need to have two versions; if a second argument is specified, use
>> that, otherwise use stdin.
>>
>> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> ---
>>  t/t9902-completion.sh | 30 +++++++++++++-----------------
>>  1 file changed, 13 insertions(+), 17 deletions(-)
>>
>> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
>> index 204c92a..59cdbfd 100755
>> --- a/t/t9902-completion.sh
>> +++ b/t/t9902-completion.sh
>> @@ -60,19 +60,15 @@ run_completion ()
>>  # 2: expected completion
>>  test_completion ()
>>  {
>> -     test $# -gt 1 && echo "$2" > expected
>> +     if [ $# -gt 1 ]; then
>> +             echo "$2" > expected
>> +     else
>> +             sed -e 's/Z$//' > expected
>> +     fi &&
>
> As "$2" could begin with dash, end with \c, etc. that possibly can
> be misinterpred by echo, I'd rewrite this as
>
>                 printf '%s\n' "$2" >expected
>
> Otherwise looked fine; thanks.

But that was the case before. I would do that in a separate patch.

Cheers.

-- 
Felipe Contreras

^ permalink raw reply

* [PATCH v4 5/5] push: update remote tags only with force
From: Chris Rorvick @ 2012-11-17 20:16 UTC (permalink / raw)
  To: git
  Cc: Chris Rorvick, Angelo Borsotti, Drew Northup, Michael Haggerty,
	Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
	Felipe Contreras, Junio C Hamano
In-Reply-To: <1353183397-17719-1-git-send-email-chris@rorvick.com>

References are allowed to update from one commit-ish to another if the
former is a ancestor of the latter.  This behavior is oriented to
branches which are expected to move with commits.  Tag references are
expected to be static in a repository, though, thus an update to a
tag (lightweight and annotated) should be rejected unless the update is
forced.

To enable this functionality, the following checks have been added to
set_ref_status_for_push() for updating refs (i.e, not new or deletion)
to restrict fast-forwarding in pushes:

  1) The old and new references must be commits.  If this fails,
     it is not a valid update for a branch.

  2) The reference name cannot start with "refs/tags/".  This
     catches lightweight tags which (usually) point to commits
     and therefore would not be caught by (1).

If either of these checks fails, then it is flagged (by default) with a
status indicating the update is being rejected due to the reference
already existing in the remote.  This can be overridden by passing
--force to git push.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 Documentation/git-push.txt | 10 +++++-----
 builtin/push.c             |  2 +-
 builtin/send-pack.c        |  5 +++++
 cache.h                    |  3 ++-
 remote.c                   | 23 +++++++++++++++++++----
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                |  8 ++++++--
 9 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/Documentation/git-push.txt b/Documentation/git-push.txt
index fe46c42..479e25f 100644
--- a/Documentation/git-push.txt
+++ b/Documentation/git-push.txt
@@ -51,11 +51,11 @@ be named. If `:`<dst> is omitted, the same ref as <src> will be
 updated.
 +
 The object referenced by <src> is used to update the <dst> reference
-on the remote side, but by default this is only allowed if the
-update can fast-forward <dst>.  By having the optional leading `+`,
-you can tell git to update the <dst> ref even when the update is not a
-fast-forward.  This does *not* attempt to merge <src> into <dst>.  See
-EXAMPLES below for details.
+on the remote side.  By default this is only allowed if the update is
+a branch, and then only if it can fast-forward <dst>.  By having the
+optional leading `+`, you can tell git to update the <dst> ref even when
+the update is not a branch or it is not a fast-forward.  This does *not*
+attempt to merge <src> into <dst>.  See EXAMPLES below for details.
 +
 `tag <tag>` means the same as `refs/tags/<tag>:refs/tags/<tag>`.
 +
diff --git a/builtin/push.c b/builtin/push.c
index 0e13e11..cd7aa3f 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -222,7 +222,7 @@ static const char message_advice_checkout_pull_push[] =
 
 static const char message_advice_ref_already_exists[] =
 	N_("Updates were rejected because a matching reference already exists in\n"
-	   "the remote and the update is not a fast-forward.");
+	   "the remote.");
 
 static void advise_pull_before_push(void)
 {
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index fda28bc..1eabf42 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -44,6 +44,11 @@ static void print_helper_status(struct ref *ref)
 			msg = "non-fast forward";
 			break;
 
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
+			res = "error";
+			msg = "already exists";
+			break;
+
 		case REF_STATUS_REJECT_NODELETE:
 		case REF_STATUS_REMOTE_REJECT:
 			res = "error";
diff --git a/cache.h b/cache.h
index f410d94..127e504 100644
--- a/cache.h
+++ b/cache.h
@@ -1004,13 +1004,14 @@ struct ref {
 		requires_force:1,
 		merge:1,
 		nonfastforward:1,
-		is_a_tag:1,
+		forwardable:1,
 		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
 		REF_STATUS_OK,
 		REF_STATUS_REJECT_NONFASTFORWARD,
+		REF_STATUS_REJECT_ALREADY_EXISTS,
 		REF_STATUS_REJECT_NODELETE,
 		REF_STATUS_UPTODATE,
 		REF_STATUS_REMOTE_REJECT,
diff --git a/remote.c b/remote.c
index 1e263b0..4fcd22c 100644
--- a/remote.c
+++ b/remote.c
@@ -1311,14 +1311,23 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     to overwrite it; you would not know what you are losing
 		 *     otherwise.
 		 *
-		 * (3) if both new and old are commit-ish, and new is a
-		 *     descendant of old, it is OK.
+		 * (3) if new is commit-ish and old is a commit, new is a
+		 *     descendant of old, and the reference is not of the
+		 *     refs/tags/ hierarchy it is OK.
 		 *
 		 * (4) regardless of all of the above, removing :B is
 		 *     always allowed.
 		 */
 
-		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
+		if (prefixcmp(ref->name, "refs/tags/")) {
+			// Additionally, disallow fast-forwarding if
+			// the old object is not a commit.  This kicks
+			// out annotated tags that might pass the
+			// is_newer() test but dangle if the reference
+			// is updated.
+			struct object *obj = parse_object(ref->old_sha1);
+			ref->forwardable = !obj || obj->type == OBJ_COMMIT;
+		}
 
 		ref->update =
 			!ref->deletion &&
@@ -1329,7 +1338,13 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 				!has_sha1_file(ref->old_sha1)
 				  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-			if (ref->nonfastforward) {
+			if (!ref->forwardable) {
+				ref->requires_force = 1;
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_ALREADY_EXISTS;
+					continue;
+				}
+			} else if (ref->nonfastforward) {
 				ref->requires_force = 1;
 				if (!force_ref_update) {
 					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
diff --git a/send-pack.c b/send-pack.c
index f50dfd9..1c375f0 100644
--- a/send-pack.c
+++ b/send-pack.c
@@ -229,6 +229,7 @@ int send_pack(struct send_pack_args *args,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/t/t5516-fetch-push.sh b/t/t5516-fetch-push.sh
index b5417cc..afb9b1b 100755
--- a/t/t5516-fetch-push.sh
+++ b/t/t5516-fetch-push.sh
@@ -368,7 +368,7 @@ test_expect_success 'push with colon-less refspec (2)' '
 		git branch -D frotz
 	fi &&
 	git tag -f frotz &&
-	git push testrepo frotz &&
+	git push -f testrepo frotz &&
 	check_push_result $the_commit tags/frotz &&
 	check_push_result $the_first_commit heads/frotz
 
@@ -929,6 +929,34 @@ test_expect_success 'push into aliased refs (inconsistent)' '
 	)
 '
 
+test_expect_success 'push tag requires --force to update remote tag' '
+	mk_test heads/master &&
+	mk_child child1 &&
+	mk_child child2 &&
+	(
+		cd child1 &&
+		git tag lw_tag &&
+		git tag -a -m "message 1" ann_tag &&
+		git push ../child2 lw_tag &&
+		git push ../child2 ann_tag &&
+		>file1 &&
+		git add file1 &&
+		git commit -m "file1" &&
+		git tag -f lw_tag &&
+		git tag -f -a -m "message 2" ann_tag &&
+		test_must_fail git push ../child2 lw_tag &&
+		test_must_fail git push ../child2 ann_tag &&
+		git push --force ../child2 lw_tag &&
+		git push --force ../child2 ann_tag &&
+		git tag -f lw_tag HEAD~ &&
+		git tag -f -a -m "message 3" ann_tag &&
+		test_must_fail git push ../child2 lw_tag &&
+		test_must_fail git push ../child2 ann_tag &&
+		git push --force ../child2 lw_tag &&
+		git push --force ../child2 ann_tag
+	)
+'
+
 test_expect_success 'push --porcelain' '
 	mk_empty &&
 	echo >.git/foo  "To testrepo" &&
diff --git a/transport-helper.c b/transport-helper.c
index 4713b69..965b778 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -661,6 +661,11 @@ static void push_update_ref_status(struct strbuf *buf,
 			free(msg);
 			msg = NULL;
 		}
+		else if (!strcmp(msg, "already exists")) {
+			status = REF_STATUS_REJECT_ALREADY_EXISTS;
+			free(msg);
+			msg = NULL;
+		}
 	}
 
 	if (*ref)
@@ -720,6 +725,7 @@ static int push_refs_with_push(struct transport *transport,
 		/* Check for statuses set by set_ref_status_for_push() */
 		switch (ref->status) {
 		case REF_STATUS_REJECT_NONFASTFORWARD:
+		case REF_STATUS_REJECT_ALREADY_EXISTS:
 		case REF_STATUS_UPTODATE:
 			continue;
 		default:
diff --git a/transport.c b/transport.c
index 60a7421..a380ad7 100644
--- a/transport.c
+++ b/transport.c
@@ -695,6 +695,10 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
 						 "non-fast-forward", porcelain);
 		break;
+	case REF_STATUS_REJECT_ALREADY_EXISTS:
+		print_ref_status('!', "[rejected]", ref, ref->peer_ref,
+						 "already exists", porcelain);
+		break;
 	case REF_STATUS_REMOTE_REJECT:
 		print_ref_status('!', "[remote rejected]", ref,
 						 ref->deletion ? NULL : ref->peer_ref,
@@ -740,12 +744,12 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
-			if (!ref->is_a_tag)
-				*reject_mask |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
 				*reject_mask |= REJECT_NON_FF_HEAD;
 			else
 				*reject_mask |= REJECT_NON_FF_OTHER;
+		} else if (ref->status == REF_STATUS_REJECT_ALREADY_EXISTS) {
+			*reject_mask |= REJECT_ALREADY_EXISTS;
 		}
 	}
 }
-- 
1.8.0.155.g3a063ad.dirty

^ permalink raw reply related

* [PATCH v4 3/5] push: flag updates
From: Chris Rorvick @ 2012-11-17 20:16 UTC (permalink / raw)
  To: git
  Cc: Chris Rorvick, Angelo Borsotti, Drew Northup, Michael Haggerty,
	Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
	Felipe Contreras, Junio C Hamano
In-Reply-To: <1353183397-17719-1-git-send-email-chris@rorvick.com>

If the reference exists on the remote and the the update is not a
delete, then mark as an update.  This is in preparation for handling
tags and branches differently when pushing.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 cache.h  |  1 +
 remote.c | 18 +++++++++++-------
 2 files changed, 12 insertions(+), 7 deletions(-)

diff --git a/cache.h b/cache.h
index 82dead1..0f53d11 100644
--- a/cache.h
+++ b/cache.h
@@ -1003,6 +1003,7 @@ struct ref {
 		merge:1,
 		nonfastforward:1,
 		is_a_tag:1,
+		update:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 186814d..fe89869 100644
--- a/remote.c
+++ b/remote.c
@@ -1318,15 +1318,19 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 
 		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
 
-		ref->nonfastforward =
+		ref->update =
 			!ref->deletion &&
-			!is_null_sha1(ref->old_sha1) &&
-			(!has_sha1_file(ref->old_sha1)
-			  || !ref_newer(ref->new_sha1, ref->old_sha1));
+			!is_null_sha1(ref->old_sha1);
 
-		if (ref->nonfastforward && !ref->force && !force_update) {
-			ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-			continue;
+		if (ref->update) {
+			ref->nonfastforward =
+				!has_sha1_file(ref->old_sha1)
+				  || !ref_newer(ref->new_sha1, ref->old_sha1);
+
+			if (ref->nonfastforward && !ref->force && !force_update) {
+				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+				continue;
+			}
 		}
 	}
 }
-- 
1.8.0.155.g3a063ad.dirty

^ permalink raw reply related

* [PATCH v4 4/5] push: flag updates that require force
From: Chris Rorvick @ 2012-11-17 20:16 UTC (permalink / raw)
  To: git
  Cc: Chris Rorvick, Angelo Borsotti, Drew Northup, Michael Haggerty,
	Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
	Felipe Contreras, Junio C Hamano
In-Reply-To: <1353183397-17719-1-git-send-email-chris@rorvick.com>

Add a flag for indicating an update to a reference requires force.
Currently the nonfastforward flag of a ref is used for this when
generating status the status message.  A separate flag insulates the
status logic from the details of set_ref_status_for_push().

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 cache.h     |  4 +++-
 remote.c    | 11 ++++++++---
 transport.c |  2 +-
 3 files changed, 12 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 0f53d11..f410d94 100644
--- a/cache.h
+++ b/cache.h
@@ -999,7 +999,9 @@ struct ref {
 	unsigned char old_sha1[20];
 	unsigned char new_sha1[20];
 	char *symref;
-	unsigned int force:1,
+	unsigned int
+		force:1,
+		requires_force:1,
 		merge:1,
 		nonfastforward:1,
 		is_a_tag:1,
diff --git a/remote.c b/remote.c
index fe89869..1e263b0 100644
--- a/remote.c
+++ b/remote.c
@@ -1285,6 +1285,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 	struct ref *ref;
 
 	for (ref = remote_refs; ref; ref = ref->next) {
+		int force_ref_update = ref->force || force_update;
+
 		if (ref->peer_ref)
 			hashcpy(ref->new_sha1, ref->peer_ref->new_sha1);
 		else if (!send_mirror)
@@ -1327,9 +1329,12 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 				!has_sha1_file(ref->old_sha1)
 				  || !ref_newer(ref->new_sha1, ref->old_sha1);
 
-			if (ref->nonfastforward && !ref->force && !force_update) {
-				ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
-				continue;
+			if (ref->nonfastforward) {
+				ref->requires_force = 1;
+				if (!force_ref_update) {
+					ref->status = REF_STATUS_REJECT_NONFASTFORWARD;
+					continue;
+				}
 			}
 		}
 	}
diff --git a/transport.c b/transport.c
index 5fcaea8..60a7421 100644
--- a/transport.c
+++ b/transport.c
@@ -659,7 +659,7 @@ static void print_ok_ref_status(struct ref *ref, int porcelain)
 		const char *msg;
 
 		strcpy(quickref, status_abbrev(ref->old_sha1));
-		if (ref->nonfastforward) {
+		if (ref->requires_force) {
 			strcat(quickref, "...");
 			type = '+';
 			msg = "forced update";
-- 
1.8.0.155.g3a063ad.dirty

^ permalink raw reply related

* [PATCH v4 2/5] push: add advice for rejected tag reference
From: Chris Rorvick @ 2012-11-17 20:16 UTC (permalink / raw)
  To: git
  Cc: Chris Rorvick, Angelo Borsotti, Drew Northup, Michael Haggerty,
	Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
	Felipe Contreras, Junio C Hamano
In-Reply-To: <1353183397-17719-1-git-send-email-chris@rorvick.com>

Advising the user to fetch and merge only makes sense if the rejected
reference is a branch.  If none of the rejections were for branches,
tell the user they need to force the update(s).

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 builtin/push.c | 15 +++++++++++++--
 cache.h        |  1 +
 remote.c       |  2 ++
 transport.c    |  6 ++++--
 transport.h    |  5 +++--
 5 files changed, 23 insertions(+), 6 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index eaeaf7e..0e13e11 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -220,6 +220,10 @@ static const char message_advice_checkout_pull_push[] =
 	   "(e.g. 'git pull') before pushing again.\n"
 	   "See the 'Note about fast-forwards' in 'git push --help' for details.");
 
+static const char message_advice_ref_already_exists[] =
+	N_("Updates were rejected because a matching reference already exists in\n"
+	   "the remote and the update is not a fast-forward.");
+
 static void advise_pull_before_push(void)
 {
 	if (!advice_push_non_ff_current || !advice_push_nonfastforward)
@@ -241,6 +245,11 @@ static void advise_checkout_pull_push(void)
 	advise(_(message_advice_checkout_pull_push));
 }
 
+static void advise_ref_already_exists(void)
+{
+	advise(_(message_advice_ref_already_exists));
+}
+
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
@@ -265,13 +274,15 @@ static int push_with_options(struct transport *transport, int flags)
 	if (!err)
 		return 0;
 
-	if (reject_mask & NON_FF_HEAD) {
+	if (reject_mask & REJECT_NON_FF_HEAD) {
 		advise_pull_before_push();
-	} else if (reject_mask & NON_FF_OTHER) {
+	} else if (reject_mask & REJECT_NON_FF_OTHER) {
 		if (default_matching_used)
 			advise_use_upstream();
 		else
 			advise_checkout_pull_push();
+	} else if (reject_mask & REJECT_ALREADY_EXISTS) {
+		advise_ref_already_exists();
 	}
 
 	return 1;
diff --git a/cache.h b/cache.h
index dbd8018..82dead1 100644
--- a/cache.h
+++ b/cache.h
@@ -1002,6 +1002,7 @@ struct ref {
 	unsigned int force:1,
 		merge:1,
 		nonfastforward:1,
+		is_a_tag:1,
 		deletion:1;
 	enum {
 		REF_STATUS_NONE = 0,
diff --git a/remote.c b/remote.c
index 04fd9ea..186814d 100644
--- a/remote.c
+++ b/remote.c
@@ -1316,6 +1316,8 @@ void set_ref_status_for_push(struct ref *remote_refs, int send_mirror,
 		 *     always allowed.
 		 */
 
+		ref->is_a_tag = !prefixcmp(ref->name, "refs/tags/");
+
 		ref->nonfastforward =
 			!ref->deletion &&
 			!is_null_sha1(ref->old_sha1) &&
diff --git a/transport.c b/transport.c
index ae9fda8..5fcaea8 100644
--- a/transport.c
+++ b/transport.c
@@ -740,10 +740,12 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
+			if (!ref->is_a_tag)
+				*reject_mask |= REJECT_ALREADY_EXISTS;
 			if (!strcmp(head, ref->name))
-				*reject_mask |= NON_FF_HEAD;
+				*reject_mask |= REJECT_NON_FF_HEAD;
 			else
-				*reject_mask |= NON_FF_OTHER;
+				*reject_mask |= REJECT_NON_FF_OTHER;
 		}
 	}
 }
diff --git a/transport.h b/transport.h
index 1f9699c..7e86352 100644
--- a/transport.h
+++ b/transport.h
@@ -140,8 +140,9 @@ int transport_set_option(struct transport *transport, const char *name,
 void transport_set_verbosity(struct transport *transport, int verbosity,
 	int force_progress);
 
-#define NON_FF_HEAD     0x01
-#define NON_FF_OTHER    0x02
+#define REJECT_NON_FF_HEAD     0x01
+#define REJECT_NON_FF_OTHER    0x02
+#define REJECT_ALREADY_EXISTS  0x04
 
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-- 
1.8.0.155.g3a063ad.dirty

^ permalink raw reply related

* [PATCH v4 1/5] push: return reject reasons via a mask
From: Chris Rorvick @ 2012-11-17 20:16 UTC (permalink / raw)
  To: git
  Cc: Chris Rorvick, Angelo Borsotti, Drew Northup, Michael Haggerty,
	Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
	Felipe Contreras, Junio C Hamano
In-Reply-To: <1353183397-17719-1-git-send-email-chris@rorvick.com>

Pass all rejection reasons back from transport_push().  The logic is
simpler and more flexible with regard to providing useful feedback.

Signed-off-by: Chris Rorvick <chris@rorvick.com>
---
 builtin/push.c      | 13 ++++---------
 builtin/send-pack.c |  4 ++--
 transport.c         | 17 ++++++++---------
 transport.h         |  9 +++++----
 4 files changed, 19 insertions(+), 24 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index db9ba30..eaeaf7e 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -244,7 +244,7 @@ static void advise_checkout_pull_push(void)
 static int push_with_options(struct transport *transport, int flags)
 {
 	int err;
-	int nonfastforward;
+	unsigned int reject_mask;
 
 	transport_set_verbosity(transport, verbosity, progress);
 
@@ -257,7 +257,7 @@ static int push_with_options(struct transport *transport, int flags)
 	if (verbosity > 0)
 		fprintf(stderr, _("Pushing to %s\n"), transport->url);
 	err = transport_push(transport, refspec_nr, refspec, flags,
-			     &nonfastforward);
+			     &reject_mask);
 	if (err != 0)
 		error(_("failed to push some refs to '%s'"), transport->url);
 
@@ -265,18 +265,13 @@ static int push_with_options(struct transport *transport, int flags)
 	if (!err)
 		return 0;
 
-	switch (nonfastforward) {
-	default:
-		break;
-	case NON_FF_HEAD:
+	if (reject_mask & NON_FF_HEAD) {
 		advise_pull_before_push();
-		break;
-	case NON_FF_OTHER:
+	} else if (reject_mask & NON_FF_OTHER) {
 		if (default_matching_used)
 			advise_use_upstream();
 		else
 			advise_checkout_pull_push();
-		break;
 	}
 
 	return 1;
diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index d342013..fda28bc 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -85,7 +85,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	int send_all = 0;
 	const char *receivepack = "git-receive-pack";
 	int flags;
-	int nonfastforward = 0;
+	unsigned int reject_mask;
 	int progress = -1;
 
 	argv++;
@@ -223,7 +223,7 @@ int cmd_send_pack(int argc, const char **argv, const char *prefix)
 	ret |= finish_connect(conn);
 
 	if (!helper_status)
-		transport_print_push_status(dest, remote_refs, args.verbose, 0, &nonfastforward);
+		transport_print_push_status(dest, remote_refs, args.verbose, 0, &reject_mask);
 
 	if (!args.dry_run && remote) {
 		struct ref *ref;
diff --git a/transport.c b/transport.c
index 9932f40..ae9fda8 100644
--- a/transport.c
+++ b/transport.c
@@ -714,7 +714,7 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count, i
 }
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-				  int verbose, int porcelain, int *nonfastforward)
+				  int verbose, int porcelain, unsigned int *reject_mask)
 {
 	struct ref *ref;
 	int n = 0;
@@ -733,18 +733,17 @@ void transport_print_push_status(const char *dest, struct ref *refs,
 		if (ref->status == REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
 
-	*nonfastforward = 0;
+	*reject_mask = 0;
 	for (ref = refs; ref; ref = ref->next) {
 		if (ref->status != REF_STATUS_NONE &&
 		    ref->status != REF_STATUS_UPTODATE &&
 		    ref->status != REF_STATUS_OK)
 			n += print_one_push_status(ref, dest, n, porcelain);
-		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD &&
-		    *nonfastforward != NON_FF_HEAD) {
+		if (ref->status == REF_STATUS_REJECT_NONFASTFORWARD) {
 			if (!strcmp(head, ref->name))
-				*nonfastforward = NON_FF_HEAD;
+				*reject_mask |= NON_FF_HEAD;
 			else
-				*nonfastforward = NON_FF_OTHER;
+				*reject_mask |= NON_FF_OTHER;
 		}
 	}
 }
@@ -1031,9 +1030,9 @@ static void die_with_unpushed_submodules(struct string_list *needs_pushing)
 
 int transport_push(struct transport *transport,
 		   int refspec_nr, const char **refspec, int flags,
-		   int *nonfastforward)
+		   unsigned int *reject_mask)
 {
-	*nonfastforward = 0;
+	*reject_mask = 0;
 	transport_verify_remote_names(refspec_nr, refspec);
 
 	if (transport->push) {
@@ -1099,7 +1098,7 @@ int transport_push(struct transport *transport,
 		if (!quiet || err)
 			transport_print_push_status(transport->url, remote_refs,
 					verbose | porcelain, porcelain,
-					nonfastforward);
+					reject_mask);
 
 		if (flags & TRANSPORT_PUSH_SET_UPSTREAM)
 			set_upstreams(transport, remote_refs, pretend);
diff --git a/transport.h b/transport.h
index 4a61c0c..1f9699c 100644
--- a/transport.h
+++ b/transport.h
@@ -140,11 +140,12 @@ int transport_set_option(struct transport *transport, const char *name,
 void transport_set_verbosity(struct transport *transport, int verbosity,
 	int force_progress);
 
-#define NON_FF_HEAD 1
-#define NON_FF_OTHER 2
+#define NON_FF_HEAD     0x01
+#define NON_FF_OTHER    0x02
+
 int transport_push(struct transport *connection,
 		   int refspec_nr, const char **refspec, int flags,
-		   int * nonfastforward);
+		   unsigned int * reject_mask);
 
 const struct ref *transport_get_remote_refs(struct transport *transport);
 
@@ -170,7 +171,7 @@ void transport_update_tracking_ref(struct remote *remote, struct ref *ref, int v
 int transport_refs_pushed(struct ref *ref);
 
 void transport_print_push_status(const char *dest, struct ref *refs,
-		  int verbose, int porcelain, int *nonfastforward);
+		  int verbose, int porcelain, unsigned int *reject_mask);
 
 typedef void alternate_ref_fn(const struct ref *, void *);
 extern void for_each_alternate_ref(alternate_ref_fn, void *);
-- 
1.8.0.155.g3a063ad.dirty

^ permalink raw reply related

* [PATCH v4 0/5] push: update remote tags only with force
From: Chris Rorvick @ 2012-11-17 20:16 UTC (permalink / raw)
  To: git
  Cc: Chris Rorvick, Angelo Borsotti, Drew Northup, Michael Haggerty,
	Philip Oakley, Johannes Sixt, Kacper Kornet, Jeff King,
	Felipe Contreras, Junio C Hamano

This patch set can be divided into two sets:

  1. Provide useful advice for rejected tag references.

     push: return reject reasons via a mask
     push: add advice for rejected tag reference

     Recommending a merge to resolve a rejected tag update seems
     nonsensical since the tag does not come along for the ride.  These
     patches change the advice for rejected tags to suggest using
     "push -f".

  2. Require force when updating tag references, even on a fast-forward.

     push: flag updates
     push: flag updates that require force
     push: update remote tags only with force

     This is in response to the following thread:

       http://thread.gmane.org/gmane.comp.version-control.git/208354

     This solution prevents fast-forwards if the reference is of the
     refs/tags/* hierarchy or if the old object is not a commit.

These patches contain the following updates since the v3 set:

  * builtin/push.c: Remove "push --force" suggestion from advice.
  * remote.c: Only require old object to be a commit to be forwardable.
      I added the check for object types based comments from Peff in
      original thread, and I think this implementation is actually what
      he intended.  If the new object is a tag, the operation is not
      destructive so there is no reason to block it (at least within
      the scope of these changes) as was done in the previous iteration.
  * t/t5516-fetch-push.sh: Create separate tests for the lightweight and
      annotated cases.  Do the annotated tests outside of refs/tags/
      so that it actually tests different functionality.

Chris Rorvick (5):
  push: return reject reasons via a mask
  push: add advice for rejected tag reference
  push: flag updates
  push: flag updates that require force
  push: update remote tags only with force

 Documentation/git-push.txt | 10 +++++-----
 builtin/push.c             | 24 +++++++++++++++---------
 builtin/send-pack.c        |  9 +++++++--
 cache.h                    |  7 ++++++-
 remote.c                   | 46 ++++++++++++++++++++++++++++++++++++----------
 send-pack.c                |  1 +
 t/t5516-fetch-push.sh      | 30 +++++++++++++++++++++++++++++-
 transport-helper.c         |  6 ++++++
 transport.c                | 25 +++++++++++++++----------
 transport.h                | 10 ++++++----
 10 files changed, 126 insertions(+), 42 deletions(-)

-- 
1.8.0.155.g3a063ad.dirty

^ permalink raw reply

* Re: `git mv` has ambiguous error message for non-existing target
From: Junio C Hamano @ 2012-11-17 19:35 UTC (permalink / raw)
  To: Patrick Lehner; +Cc: git
In-Reply-To: <50A5E6D2.5060609@gmx.de>

Patrick Lehner <lehner.patrick@gmx.de> writes:

> But just because mv's error essage isnt very good, does that mean git
> mv's error message mustn't be better?

Did I say the error message from 'mv' was not very good in the
message you are responding to (by the way, this is why you should
never top-post when you are responding to a message on this list)?

I meant to say that the message from 'mv' is good enough, so is the
one given by 'git mv'.

I wouldn't reject a patch that updates our message to something more
informative without looking at it, though.

^ permalink raw reply

* Re: [RFC/PATCH 4/5] completion: get rid of compgen
From: Felipe Contreras @ 2012-11-17 19:33 UTC (permalink / raw)
  To: SZEDER Gábor
  Cc: git, Junio C Hamano, Björn Gustavsson, Shawn O. Pearce,
	Robert Zeh, Peter van der Does, Jonathan Nieder, Jeff King
In-Reply-To: <20121117141215.GG12052@goldbirke>

On Sat, Nov 17, 2012 at 3:12 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sat, Nov 17, 2012 at 12:42:38PM +0100, Felipe Contreras wrote:
>> On Sat, Nov 17, 2012 at 12:00 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> > On Sat, Nov 17, 2012 at 02:38:17AM +0100, Felipe Contreras wrote:
>> >> The functionality we use is very simple, plus, this fixes a known
>> >> breakage 'complete tree filename with metacharacters'.
>> >>
>> >> Signed-off-by: Felipe Contreras <felipe.contreras@gmail.com>
>> >> ---
>> >>  contrib/completion/git-completion.bash | 6 +++++-
>> >>  1 file changed, 5 insertions(+), 1 deletion(-)
>> >>
>> >> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
>> >> index 975ae13..ad3e1fe 100644
>> >> --- a/contrib/completion/git-completion.bash
>> >> +++ b/contrib/completion/git-completion.bash
>> >> @@ -227,7 +227,11 @@ fi
>> >>
>> >>  __gitcompadd ()
>> >>  {
>> >> -     COMPREPLY=($(compgen -W "$1" -P "$2" -S "$4" -- "$3"))
>> >> +     for x in $1; do
>> >> +             if [[ "$x" = "$3"* ]]; then
>> >> +                     COMPREPLY+=("$2$x$4")
>> >> +             fi
>> >> +     done
>> >
>> > The whole point of creating __gitcomp_nl() back then was to fill
>> > COMPREPLY without iterating through all words in the wordlist, making
>> > completion faster for large number of words, e.g. a lot of refs, or
>> > later a lot of symbols for 'git grep' in a larger project.
>> >
>> > The loop here kills that optimization.
>>
>> So your solution is to move the loop to awk? I fail to see how that
>> could bring more optimization, specially since it includes an extra
>> fork now.
>
> This patch didn't aim for more optimization, but it was definitely a
> goal not to waste what we gained by creating __gitcomp_nl() in
> a31e6262 (completion: optimize refs completion, 2011-10-15).  However,
> as it turns out the new version with awk is actually faster than
> current master with compgen:
>
>   Before:
>
>     $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
>     $ time __gitcomp_nl "$refs"
>
>     real    0m0.242s
>     user    0m0.220s
>     sys     0m0.028s
>
>   After:
>
>     $ time __gitcomp_nl "$refs"
>
>     real    0m0.109s
>     user    0m0.096s
>     sys     0m0.012s

This one is even faster:

while read -r x; do
	if [[ "$x" == "$3"* ]]; then
		COMPREPLY+=("$2$x$4")
	fi
done <<< $1

== 10000 ==
one:
real	0m0.148s
user	0m0.134s
sys	0m0.025s
two:
real	0m0.055s
user	0m0.054s
sys	0m0.000s

-- 
Felipe Contreras

^ permalink raw reply

* Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
From: Felipe Contreras @ 2012-11-17 19:28 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Jeff King, Jonathan Nieder, Junio C Hamano
In-Reply-To: <CAMP44s2YOshWkM9n2XxxLw79=-VF8U3Q4ta1D1zgHqWH30zOmw@mail.gmail.com>

On Sat, Nov 17, 2012 at 8:08 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
>>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>>
>>> >  __gitcomp_nl ()
>>> >  {
>>> >         local IFS=$'\n'
>>> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>>> > +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
>>> > +               BEGIN {
>>> > +                       FS="\n";
>>> > +                       len=length(cur);
>>> > +               }
>>> > +               {
>>> > +                       if (cur == substr($1, 1, len))
>>> > +                               print pfx$1sfx;
>>> > +               }' <<< "$1" ))
>>> >  }
>>>
>>> Does this really perform better than my alternative?
>>>
>>> +       for x in $1; do
>>> +               if [[ "$x" = "$3"* ]]; then
>>> +                       COMPREPLY+=("$2$x$4")
>>> +               fi
>>> +       done
>>
>> It does:
>>
>>   My version:
>>
>>     $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
>>     $ time __gitcomp_nl "$refs"
>>
>>     real    0m0.109s
>>     user    0m0.096s
>>     sys     0m0.012s
>>
>>   Yours:
>>
>>     $ time __gitcomp_nl "$refs"
>>
>>     real    0m0.321s
>>     user    0m0.312s
>>     sys     0m0.008s
>
> Yeah, for 10000 refs, which is not the common case:

And this beats both in every case:

while read -r x; do
	if [[ "$x" == "$3"* ]]; then
		COMPREPLY+=("$2$x$4")
	fi
done <<< $1

== 1 ==
one:
real	0m0.004s
user	0m0.003s
sys	0m0.000s
two:
real	0m0.000s
user	0m0.000s
sys	0m0.000s
== 10 ==
one:
real	0m0.005s
user	0m0.002s
sys	0m0.002s
two:
real	0m0.000s
user	0m0.000s
sys	0m0.000s
== 100 ==
one:
real	0m0.005s
user	0m0.004s
sys	0m0.000s
two:
real	0m0.001s
user	0m0.000s
sys	0m0.000s
== 1000 ==
one:
real	0m0.010s
user	0m0.008s
sys	0m0.001s
two:
real	0m0.004s
user	0m0.004s
sys	0m0.000s
== 10000 ==
one:
real	0m0.061s
user	0m0.057s
sys	0m0.005s
two:
real	0m0.045s
user	0m0.044s
sys	0m0.000s
== 100000 ==
one:
real	0m0.582s
user	0m0.575s
sys	0m0.022s
two:
real	0m0.487s
user	0m0.482s
sys	0m0.004s
== 1000000 ==
one:
real	0m6.305s
user	0m6.284s
sys	0m0.216s
two:
real	0m5.268s
user	0m5.194s
sys	0m0.065s

-- 
Felipe Contreras

^ permalink raw reply

* Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
From: W. Trevor King @ 2012-11-17 19:20 UTC (permalink / raw)
  To: Heiko Voigt
  Cc: Junio C Hamano, Git, Jeff King, Phil Hord, Shawn Pearce,
	Jens Lehmann, Nahor
In-Reply-To: <20121117150441.GA7695@book.hvoigt.net>

[-- Attachment #1: Type: text/plain, Size: 3410 bytes --]

On Sat, Nov 17, 2012 at 04:04:42PM +0100, Heiko Voigt wrote:
> > On Sat, Nov 10, 2012 at 01:44:37PM -0500, W. Trevor King wrote:
> > >   $ git submodule pull-branch
> > 
> > I think "floating submodules" is a misleading name for this feature
> > though, since the checkout SHA is explicitly specified.  We're just
> > making it more convenient to explicitly update the SHA.  How about
> > "tracking submodules"?
> 
> Until now we have always called this workflow floating submodules. I
> imaging since the submodule floats to the newest revision (whatever the
> user chooses that to be) instead of staying at the recorded sha1.
> 
> "tracking submodules" sounds strange to me since the term tracked in git
> is mainly used in combination with exact recorded history (e.g. tracking
> branch). Since it is about *not* checking out the recorded sha1 but
> something that can change I think that could cause confusion.
> 
> I think floating is a more unambiguous term and already known on the
> list.

I had been getting the impression that floating submodules would
automatically update without explicit user intervention.  After
re-reading your initial floating submodules post, it looks like we do
match up after the mapping:

  Git        Heiko               Trevor
  ---------  -----------------   -------------
  update     update --checkout   update
             update              update --pull

So I'll go back to "floating" ;).

On Sat, Nov 17, 2012 at 04:30:07PM +0100, Heiko Voigt wrote:
> > >  (2) "git diff [$path]" and friends in the superproject compares the
> > >      HEAD of the checkout of the submodule at $path with the tip of
> > >      the branch named by submodule.$name.branch in .gitmodules of
> > >      the superproject, instead of the commit that is recorded in the
> > >      index of the superproject.
> > > 
> > 
> > Hmm.  ???git diff??? compares the working tree with the local HEAD (just a
> > SHA for submodules), so I don't think it should care about the status
> > of a remote branch.  This sounds like you want something like:
> > 
> >   $ git submodule foreach 'git diff origin/$submodule_branch'
> > 
> > Perhaps this is enough motivation for keeping $submodule_* exports?
> > 
> > > and the option were called something like "--follow-branch=$branch",
> > > ???
> 
> I am not sure if hiding changes to the recorded SHA1 from the user is
> such a useful thing. In the first step I would like it if it was kept
> simple and only the submodule update machinery learned to follow a
> branch. If that results in local changes that should be shown. The user
> is still in charge of recording the updated SHA1 in his commit.

I understand what you're warning against here, or what it has to do
with "git diff".

> From what I have heard of projects using this: They usually still have
> something that records the SHA1s on a regular basis. Thinking further,
> why not record them in git? We could add an option to update which
> creates such a commit.

I think it's best to have users craft their own commit messages
explaining why the branch was updated.  That said, an auto-generated
hint (a la "git merge") would probably be a useful extra feature.

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 836 bytes --]

^ permalink raw reply

* Re: [PATCH 5/7] completion: fix expansion issues in __gitcomp_nl()
From: Felipe Contreras @ 2012-11-17 19:08 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Jeff King, Jonathan Nieder, Junio C Hamano
In-Reply-To: <20121117141422.GI12052@goldbirke>

On Sat, Nov 17, 2012 at 3:14 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
> On Sat, Nov 17, 2012 at 12:50:39PM +0100, Felipe Contreras wrote:
>> On Sat, Nov 17, 2012 at 12:05 PM, SZEDER Gábor <szeder@ira.uka.de> wrote:
>>
>> >  __gitcomp_nl ()
>> >  {
>> >         local IFS=$'\n'
>> > -       COMPREPLY=($(compgen -P "${2-}" -S "${4- }" -W "$1" -- "${3-$cur}"))
>> > +       COMPREPLY=($(awk -v pfx="${2-}" -v sfx="${4- }" -v cur="${3-$cur}" '
>> > +               BEGIN {
>> > +                       FS="\n";
>> > +                       len=length(cur);
>> > +               }
>> > +               {
>> > +                       if (cur == substr($1, 1, len))
>> > +                               print pfx$1sfx;
>> > +               }' <<< "$1" ))
>> >  }
>>
>> Does this really perform better than my alternative?
>>
>> +       for x in $1; do
>> +               if [[ "$x" = "$3"* ]]; then
>> +                       COMPREPLY+=("$2$x$4")
>> +               fi
>> +       done
>
> It does:
>
>   My version:
>
>     $ refs="$(for i in {0..9999} ; do echo branch$i ; done)"
>     $ time __gitcomp_nl "$refs"
>
>     real    0m0.109s
>     user    0m0.096s
>     sys     0m0.012s
>
>   Yours:
>
>     $ time __gitcomp_nl "$refs"
>
>     real    0m0.321s
>     user    0m0.312s
>     sys     0m0.008s

Yeah, for 10000 refs, which is not the common case:

== 1 ==
SZEDER:
real	0m0.007s
user	0m0.003s
sys	0m0.000s
felipec:
real	0m0.000s
user	0m0.000s
sys	0m0.000s
== 10 ==
SZEDER:
real	0m0.004s
user	0m0.003s
sys	0m0.001s
felipec:
real	0m0.000s
user	0m0.000s
sys	0m0.000s
== 100 ==
SZEDER:
real	0m0.005s
user	0m0.002s
sys	0m0.002s
felipec:
real	0m0.002s
user	0m0.002s
sys	0m0.000s
== 1000 ==
SZEDER:
real	0m0.010s
user	0m0.008s
sys	0m0.001s
felipec:
real	0m0.018s
user	0m0.017s
sys	0m0.001s
== 10000 ==
SZEDER:
real	0m0.062s
user	0m0.060s
sys	0m0.003s
felipec:
real	0m0.175s
user	0m0.174s
sys	0m0.000s
== 100000 ==
SZEDER:
real	0m0.595s
user	0m0.593s
sys	0m0.021s
felipec:
real	0m1.848s
user	0m1.843s
sys	0m0.003s
== 1000000 ==
SZEDER:
real	0m6.258s
user	0m6.241s
sys	0m0.215s
felipec:
real	0m18.191s
user	0m18.115s
sys	0m0.045s

-- 
Felipe Contreras

^ permalink raw reply

* Re: [PATCH] tcsh-completion re-using git-completion.bash
From: Felipe Contreras @ 2012-11-17 18:01 UTC (permalink / raw)
  To: Marc Khouzam; +Cc: Junio C Hamano, SZEDER Gábor, git
In-Reply-To: <CAFj1UpFbuHVhPOQVB9-sPjW2aBN=H+OUyYnz00qASZ5ssbwmGw@mail.gmail.com>

On Sat, Nov 17, 2012 at 6:15 PM, Marc Khouzam <marc.khouzam@gmail.com> wrote:
> On Fri, Nov 16, 2012 at 4:56 PM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
>> On Fri, Nov 16, 2012 at 10:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>>> The point is not about the quality of zsh's emulation
>>> of (k)sh when it is run under that mode, but is about not having to
>>> have that logic in bash-only part in the first place.
>>
>> As I said, that logic can be moved away _if_ my wrapper is merged. But
>> then again, that would cause regressions to existing users.
>
> Please forgive me as I don't know the background of the efforts for
> zsh git-completion or
> the syntax for zsh completion, but I thought I'd mention another
> approach I tried for tcsh
> which may work for zsh.
>
> I gather that using a wrapper for zsh causes concerns about
> backwards-compatibility.

I don't see any concerns.

> if [[ -n ${ZSH_VERSION-} ]]; then
>   # replace below by zsh completion commands calling `bash
> ${HOME}/.git-completion.bash`

>   complete git   'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/'
>   complete gitk 'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/'

That doesn't work in zsh. It might be possible to do something
similar, but it would probably require many more lines.

And we can achieve the same by essentially moving the relevant code of
my wrapper:

--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -23,10 +23,6 @@
 #    3) Consider changing your PS1 to also show the current branch,
 #       see git-prompt.sh for details.

-if [[ -n ${ZSH_VERSION-} ]]; then
-       autoload -U +X bashcompinit && bashcompinit
-fi
-
 case "$COMP_WORDBREAKS" in
 *:*) : great ;;
 *)   COMP_WORDBREAKS="$COMP_WORDBREAKS:"
@@ -2404,6 +2400,32 @@ __gitk_main ()
        __git_complete_revlist
 }

+if [[ -n ${ZSH_VERSION-} ]]; then
+       emulate -L zsh
+
+       __gitcompadd ()
+       {
+               compadd -Q -S "$4" -P "${(M)cur#*[=:]}" -p "$2" --
${=1} && _ret=0
+       }
+
+       _git ()
+       {
+               local _ret=1
+               () {
+                 emulate -L ksh
+                       local cur cword prev
+                       cur=${words[CURRENT-1]}
+                       prev=${words[CURRENT-2]}
+                       let cword=CURRENT-1
+                       __${service}_main
+               }
+               let _ret && _default -S '' && _ret=0
+               return _ret
+       }
+       compdef _git git gitk
+       return
+fi
+
 __git_func_wrap ()
 {
        if [[ -n ${ZSH_VERSION-} ]]; then

-- 
Felipe Contreras

^ permalink raw reply

* Re: [PATCH] tcsh-completion re-using git-completion.bash
From: Marc Khouzam @ 2012-11-17 17:15 UTC (permalink / raw)
  To: Felipe Contreras; +Cc: Junio C Hamano, SZEDER Gábor, git
In-Reply-To: <CAMP44s0y3UPVT+ndELaKNsWXAPG3kv-Xq_Wf6ONDF3Z99A5zMQ@mail.gmail.com>

On Fri, Nov 16, 2012 at 4:56 PM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> On Fri, Nov 16, 2012 at 10:20 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
>> The point is not about the quality of zsh's emulation
>> of (k)sh when it is run under that mode, but is about not having to
>> have that logic in bash-only part in the first place.
>
> As I said, that logic can be moved away _if_ my wrapper is merged. But
> then again, that would cause regressions to existing users.

Please forgive me as I don't know the background of the efforts for
zsh git-completion or
the syntax for zsh completion, but I thought I'd mention another
approach I tried for tcsh
which may work for zsh.

I gather that using a wrapper for zsh causes concerns about
backwards-compatibility.
So, what could be done is have the bash script do both jobs: setup the
zsh completion
commands, and output the git completion using bash itself.  At the top
of git-completion.bash
(or it could be even pushed at the bottom using if/else) we could use:

if [[ -n ${ZSH_VERSION-} ]]; then
  # replace below by zsh completion commands calling `bash
${HOME}/.git-completion.bash`
  complete git   'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/'
  complete gitk 'p/*/`bash ${HOME}/.git-completion.bash ${COMMAND_LINE}`/'
  exit
fi

That way the zsh user would still simply do 'source
~/.git-completion.bash' which would
only execute the two zsh completion setup commands.  Then, when completion is
triggered, it calls `bash ${HOME}/.git-completion.bash ${COMMAND_LINE}` and
processes the output like tcsh does.  This limits the zsh-specific
code to 2 lines for
the entire script.

I got this to work for tcsh (solution (B)) adding the following a the top of
git-completion.bash:

test "$tcsh" != "" && \
   complete git  'p,*,`${HOME}/.git-completion.sh
"${COMMAND_LINE}"|\sort|\uniq`,' && \
   complete gitk 'p,*,`${HOME}/.git-completion.sh
"${COMMAND_LINE}"|\sort|\uniq`,' && \
   exit

but I didn't think people would go for that since those lines have to
work in both bash
and tcsh syntax.  I thought this made the script a bit brittle.

Just a thought.

Marc

^ permalink raw reply

* Re: using multiple version of git simultaneously
From: Jeff King @ 2012-11-17 16:16 UTC (permalink / raw)
  To: Tomas Carnecky; +Cc: arif, git
In-Reply-To: <1353163831-ner-9354@calvin>

On Sat, Nov 17, 2012 at 02:50:31PM +0000, Tomas Carnecky wrote:

> On Sat, 17 Nov 2012 20:25:21 +0600, arif <aftnix@gmail.com> wrote:
> > I'm trying to use different version of git simultaneously. So how can i
> > append some suffix (like "--program-suffix=git1.8) so that i can
> > distinguish between different versions.
> 
> Install each version into its own prefix (~/git/1.8.0/, ~/git/1.7.0/ etc).

Once you have done that, you can also symlink the binary from each into
your regular PATH (e.g., ln -s ~/git/1.8.0/bin/git ~/bin/git.v1.8) to
make it easy to switch between them. The installed exec-path is baked in
at compile-time, so it finds the correct git sub-programs properly.

I keep a couple dozen built versions of git around like this for quick
regression testing of bugs we see on the list.

-Peff

^ permalink raw reply

* Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
From: Heiko Voigt @ 2012-11-17 15:30 UTC (permalink / raw)
  To: W. Trevor King
  Cc: Junio C Hamano, Git, Jeff King, Phil Hord, Shawn Pearce,
	Jens Lehmann, Nahor
In-Reply-To: <20121111150047.GA22608@odin.tremily.us>

Hi,

On Sun, Nov 11, 2012 at 10:00:48AM -0500, W. Trevor King wrote:
> On Sun, Nov 11, 2012 at 02:33:45AM -0800, Junio C Hamano wrote:
> In order to avoid losing (or creating) local-only submodule commits,
> I'll probably bail (with an error) on non-fast-forward pulls.  Can
> anyone else think of other safety concerns?

That sounds like a good thing to do. We can allow more flexibility later
if people come up with usecases.

> This means that I'll probably drop Phil's $submodule_* export in v4,
> because the only explicit use we have for it is this branch tracking.
> I still think it is a useful idea, but it may not be useful enough to
> be worth the complexity.

Yes lets concentrate on the branch following first.

> >  (2) "git diff [$path]" and friends in the superproject compares the
> >      HEAD of the checkout of the submodule at $path with the tip of
> >      the branch named by submodule.$name.branch in .gitmodules of
> >      the superproject, instead of the commit that is recorded in the
> >      index of the superproject.
> > 
> 
> Hmm.  ???git diff??? compares the working tree with the local HEAD (just a
> SHA for submodules), so I don't think it should care about the status
> of a remote branch.  This sounds like you want something like:
> 
>   $ git submodule foreach 'git diff origin/$submodule_branch'
> 
> Perhaps this is enough motivation for keeping $submodule_* exports?
> 
> > and the option were called something like "--follow-branch=$branch",
> > ???

I am not sure if hiding changes to the recorded SHA1 from the user is
such a useful thing. In the first step I would like it if it was kept
simple and only the submodule update machinery learned to follow a
branch. If that results in local changes that should be shown. The user
is still in charge of recording the updated SHA1 in his commit.

>From what I have heard of projects using this: They usually still have
something that records the SHA1s on a regular basis. Thinking further,
why not record them in git? We could add an option to update which
creates such a commit.

Since git is all about changes I am hesitant to hide them from the user.

> I'll replace -r/--record with --follow-branch in v4.

Sounds good.

Cheers Heiko

^ permalink raw reply

* Re: Re: [PATCH v3 1/3] git-submodule add: Add -r/--record option
From: Heiko Voigt @ 2012-11-17 15:04 UTC (permalink / raw)
  To: W. Trevor King
  Cc: Junio C Hamano, Git, Jeff King, Phil Hord, Shawn Pearce,
	Jens Lehmann, Nahor
In-Reply-To: <20121110190232.GD2739@mjolnir>

Hi,

sorry for the late reply but my git time is limited.

On Sat, Nov 10, 2012 at 02:02:32PM -0500, W. Trevor King wrote:
> On Fri, Nov 09, 2012 at 05:29:27PM +0100, Heiko Voigt wrote:
> > I think we should agree on a behavior for this option and implement it
> > the same time when add learns about it. When we were discussing floating
> > submodules as an important option for the gerrit people I already started
> > to implement a proof of concept. Please have a look here:
> > 
> > https://github.com/hvoigt/git/commits/hv/floating_submodules
> 
> After skimming through this, something like
> 
>   $ git submodule update --pull
> 
> would probably be better than introducing a new command:

Yeah along the lines of that, but one thing to keep in mind:

We already have --rebase and --merge which do slightly different things
(I think). Adding --pull here should behave similar to them. Like fetch
and merge is the same to pull without submodules.

If I am understanding your goal correctly your --pull would be
different. On the other hand: A --pull makes no sense if we apply it to
the existing --merge option since it merges the recorded sha1 into the
current HEAD. Just a fetch would not really make a difference.

Thinking along the existing options I would probably still expect --pull
to merge something into the current HEAD. So maybe we have to iron out
where this command/option should go. But changing that once we have a
patch to discuss should not be that much work. So please proceed with
--pull and once we know exactly what it does we can polish that.

> On Sat, Nov 10, 2012 at 01:44:37PM -0500, W. Trevor King wrote:
> >   $ git submodule pull-branch
> 
> I think "floating submodules" is a misleading name for this feature
> though, since the checkout SHA is explicitly specified.  We're just
> making it more convenient to explicitly update the SHA.  How about
> "tracking submodules"?

Until now we have always called this workflow floating submodules. I
imaging since the submodule floats to the newest revision (whatever the
user chooses that to be) instead of staying at the recorded sha1.

"tracking submodules" sounds strange to me since the term tracked in git
is mainly used in combination with exact recorded history (e.g. tracking
branch). Since it is about *not* checking out the recorded sha1 but
something that can change I think that could cause confusion.

I think floating is a more unambiguous term and already known on the
list.

Cheers Heiko

^ permalink raw reply


This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox