Git development
 help / color / mirror / Atom feed
* Re: [PATCH 1/6] git-p4 tests: refactor and cleanup
From: Stefano Lattarini @ 2011-10-16  9:38 UTC (permalink / raw)
  To: Pete Wyckoff; +Cc: git, Junio C Hamano, Luke Diamand, Chris Li
In-Reply-To: <20111015155517.GB29436@arf.padd.com>

Hi Pete, hope you don't mind a couple of minor nits from an outsider ...

> +# Try to pick a unique port: guess a large number, then hope
> +# no more than one of each test is running.
> +#
> +# This does not handle the case where somebody else is running the
> +# same tests and has chosen the same ports.
> +testid=${this_test#t}
> +git_p4_test_start=9800
> +P4DPORT=$((10669 + (testid - git_p4_test_start)))
> +
This won't work with older versions of the Almquist shell (without
prepending `testid' and `git_p4_test_start' with a `$', that is):

  $ ash-0.5.2 -c 'a=4; b=2; echo $(( 1 + (a - b) ))'
  ash-0.5.2: arith: syntax error: " 1 + (a - b) "

Still, it works with all the other POSIX-ish shells that I've tried,
including dash 0.5.5.1 on GNU/Linux, NetBSD 5.1 /bin/sh and /bin/ksh,
and Solaris 10 /bin/ksh and /usr/xpg4/bin/sh, so it might not be worth
worrying about.

Same goes for other similar usages in the rest of the patch, such as
"git config git-p4.detectCopies $((level + 2))"

Regards,
  Stefano

^ permalink raw reply

* Re: [PATCH v2] git-svn: Allow certain refs to be ignored
From: Eric Wong @ 2011-10-16  8:23 UTC (permalink / raw)
  To: Michael Olson; +Cc: Git Mailing List
In-Reply-To: <CAN4ruPjaWgrk1HKmDR8XYdQcVOaYtj=RQ1R9=KYjGzOZ0-B_vA@mail.gmail.com>

Michael Olson <mwolson@gnu.org> wrote:
> Signed-off-by: Michael Olson <mwolson@gnu.org>
> ---
> Rebased against git master.

Thanks, acked and pushed to git://bogomips.org/git-svn

^ permalink raw reply

* Re: A note from the maintainer
From: Junio C Hamano @ 2011-10-16  7:24 UTC (permalink / raw)
  To: Martin von Zweigbergk; +Cc: Paul Mackerras, git
In-Reply-To: <CAOeW2eHdOQi=fqzm6A_dzbMCXFo=FB1JFBe21KQL5vypCG8jeg@mail.gmail.com>

Martin von Zweigbergk <martin.von.zweigbergk@gmail.com> writes:

> On Tue, Oct 4, 2011 at 7:22 PM, Junio C Hamano <gitster@pobox.com> wrote:
>>  - gitk-git/ comes from Paul Mackerras's gitk project:
>>
>>        git://git.kernel.org/pub/scm/gitk/gitk.git
>
> I don't seem to be able to fetch from there. Is it still supposed to be there?

Unfortunately k.org is _slowly_ coming back to life.

^ permalink raw reply

* What should "git fetch origin +next" should do?
From: Junio C Hamano @ 2011-10-16  7:20 UTC (permalink / raw)
  To: git

As some might know, I use the traditional non-separate-remotes layout in
many of my working trees. I am old fashioned.

I just tried to update one of them with "git pull --ff-only", and after
seeing that the fetch phase failed with non-ff on 'next', ran

	$ git fetch origin +next

which happily copied the tip of updated next to FETCH_HEAD and nowhere
else. Of course, a colon-less refspec means do not store it anywhere,
i.e. "<colon-less-refspec>" === "<colon-less refspec>:", so prefixing it
with '+' to force would logically be a no-op.  But it nevertheless was
somewhat surprising and irritating.

This is one of the many things that is so minor that it probably is not
worth risking backward compatibility issues to change, but something that
we would design differently if we were starting from scratch. Maybe in Git
2.0.

The question however is what should it do. I can see three possibilities:

 (1) Forcing to fetch into FETCH_HEAD does not make any sense, so instead
     of silently ignoring the '+' prefix, error it out and do not fetch
     anything. This is easy to explain and logically makes more sense than
     the current behaviour.

 (2) Do notice '+' and understand that it is a request to force fetch into
     some ref locally, and from the lack of colon and RHS, assume that the
     user wants Git to infer the RHS using the configured refspec (in my
     case, "refs/heads/next:refs/heads/next" is one of the configured
     fetch refspec; "refs/heads/*:refs/remotes/origin/*" would be the one
     that would match in the separate-remotes layout). In other words,
     treat it as if the user typed "+refs/heads/next:refs/heads/next" (or
     "+refs/heads/next:refs/remotes/origin/next" in the separate-remote
     layout) from the command line.

 (3) Do notice '+' is applied to 'next' but otherwise ignore the fact that
     it is a command line pathspec, which would cause us to ignore
     configured refspecs. In other words, fetch normally as if there were
     no refspec on the command line, but when updating refs/heads/next (or
     refs/remotes/origin/next in the separate-remotes layout), allow non
     fast-forward updates.

The latter two are hard to explain and more error prone. I tend to think
both would be a mistake. If "+next" from the command line is modified to
update our remote tracking branch using the configured refspec, "next"
without '+' prefix also should. Otherwise the behaviour becomes too
inconsistent, but it is often convenient to fetch one-off even from a
configured remote using "git fetch origin next" into FETCH_HEAD without
updating our remote tracking branches, and change along the lines of
either (2) or (3) will forbid such a workflow.

Additionally (3) has very little advantage over "git fetch --force".  If
'next' and 'master' both do not fast-forward, and the configured fetch
refspecs require both to fast-forward, "git fetch --force" would update
both, and "git fetch +next" with interpretation (3) would error out by
noticing that 'master' does not fast-forward. But anybody who is doing
"git fetch +next" already knows that 'next' does not fast-forward, and
that is because he has tried regular "git fetch" and saw both 'next' and
'master' fail back then, so he either wants to update only 'next', in
which case interpretation in (3) would not be helpful, or wants to update
both, in which case --force would be a better choice and exists already.

Perhaps we can/want to implement (1)?

^ permalink raw reply

* Re: [PATCH 0/7] Some patches from msysGit (round 2)
From: Junio C Hamano @ 2011-10-16  5:17 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: Git, msysGit
In-Reply-To: <1318687520-19522-1-git-send-email-patthoyts@users.sourceforge.net>

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> This collects some recent patches from the msysGit tree that clear up
> test issues on Windows.
>
> This second version incorporates suggestions received from round 1 to:
>   avoid duplicating code in t9901 web-browse tests
>   drop the t1402 changes in favour of another change from J6t (on pu)
>   test for the presence of 'bcomp' in bc3 mergetool

Thanks.

Will replace the previous patches with these and advance them to 'next'
shortly.

^ permalink raw reply

* Re: [PATCH 2/9 v2] completion: optimize refs completion
From: Junio C Hamano @ 2011-10-16  3:29 UTC (permalink / raw)
  To: SZEDER Gábor; +Cc: git, Shawn O. Pearce, Jonathan Nieder
In-Reply-To: <1318683443-1896-1-git-send-email-szeder@ira.uka.de>

Thanks; will replace the corresponding patch in the topic branch and
rebuild.

^ permalink raw reply

* Re: [PATCH] grep: fix error message mention --exclude
From: Junio C Hamano @ 2011-10-16  3:27 UTC (permalink / raw)
  To: Bert Wesarg; +Cc: git
In-Reply-To: <2533ba3dbe08fc0d76c9cbc9c76bff1c4ff80d4c.1318703760.git.bert.wesarg@googlemail.com>

Thanks.

^ permalink raw reply

* Re: [PATCH] fmt-merge-msg.c: Fix an "dubious one-bit signed bitfield" sparse error
From: Junio C Hamano @ 2011-10-16  3:19 UTC (permalink / raw)
  To: Ramsay Jones; +Cc: GIT Mailing-list
In-Reply-To: <4E99C713.4090802@ramsay1.demon.co.uk>

Ramsay Jones <ramsay@ramsay1.demon.co.uk> writes:

>  struct origin_data {
>  	unsigned char sha1[20];
> -	int is_local_branch:1;
> +	unsigned int is_local_branch:1;

Thanks, I overlooked this one.

^ permalink raw reply

* Re: [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs
From: Junio C Hamano @ 2011-10-16  5:10 UTC (permalink / raw)
  To: git, Daniel Barkalow; +Cc: Carlos Martín Nieto, Jeff King, mathstuf
In-Reply-To: <7vobxhtugo.fsf@alter.siamese.dyndns.org>

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

> Carlos Martín Nieto <cmn@elego.de> writes:
>
>> Move the body of remote_find_tracking to a new function query_refspecs
>> which does the same (find a refspec that matches and apply the
>> transformation) but explicitly wants the list of refspecs.
>>
>> Make remote_find_tracking and apply_refspecs use query_refspecs.
>>
>> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
>> ---
>>  remote.c |   70 ++++++++++++++++++++++++++++++-------------------------------
>>  1 files changed, 34 insertions(+), 36 deletions(-)
>
> Looks very sensible, especially knowing what you want to do in the next
> patch ;-).
>
> Thanks.

I notice that before this update, passing a refspec[] with an element that
lacks its dst side to apply_refspecs() would have segfaulted but with this
update such an element in the refspec[] will simply be ignored.

This helper function was added in 72ff894 (Allow helper to map private ref
names into normal names, 2009-11-18) for transport-helper's use. I am sure
the change will not introduce a regression due to this skippage (the code
without this patch would have simply crashed with such an input anyway),
but I thought people involved in the transport layer may want to know.

^ permalink raw reply

* Re: [PATCH 3/5] remote: separate out the remote_find_tracking logic into query_refspecs
From: Junio C Hamano @ 2011-10-16  4:45 UTC (permalink / raw)
  To: Carlos Martín Nieto; +Cc: git, Jeff King, mathstuf
In-Reply-To: <1318655066-29001-4-git-send-email-cmn@elego.de>

Carlos Martín Nieto <cmn@elego.de> writes:

> Move the body of remote_find_tracking to a new function query_refspecs
> which does the same (find a refspec that matches and apply the
> transformation) but explicitly wants the list of refspecs.
>
> Make remote_find_tracking and apply_refspecs use query_refspecs.
>
> Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
> ---
>  remote.c |   70 ++++++++++++++++++++++++++++++-------------------------------
>  1 files changed, 34 insertions(+), 36 deletions(-)

Looks very sensible, especially knowing what you want to do in the next
patch ;-).

Thanks.

>
> diff --git a/remote.c b/remote.c
> index b8ecfa5..e94c6d2 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -828,59 +828,57 @@ static int match_name_with_pattern(const char *key, const char *name,
>  	return ret;
>  }
>  
> -char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
> -		     const char *name)
> +static int query_refspecs(struct refspec *refs, int ref_count, struct refspec *query)
>  {
>  	int i;
> -	char *ret = NULL;
> -	for (i = 0; i < nr_refspec; i++) {
> -		struct refspec *refspec = refspecs + i;
> -		if (refspec->pattern) {
> -			if (match_name_with_pattern(refspec->src, name,
> -						    refspec->dst, &ret))
> -				return ret;
> -		} else if (!strcmp(refspec->src, name))
> -			return strdup(refspec->dst);
> -	}
> -	return NULL;
> -}
> +	int find_src = query->src == NULL;
>  
> -int remote_find_tracking(struct remote *remote, struct refspec *refspec)
> -{
> -	int find_src = refspec->src == NULL;
> -	char *needle, **result;
> -	int i;
> +	if (find_src && !query->dst)
> +		return error("query_refspecs: need either src or dst");
>  
> -	if (find_src) {
> -		if (!refspec->dst)
> -			return error("find_tracking: need either src or dst");
> -		needle = refspec->dst;
> -		result = &refspec->src;
> -	} else {
> -		needle = refspec->src;
> -		result = &refspec->dst;
> -	}
> +	for (i = 0; i < ref_count; i++) {
> +		struct refspec *refspec = &refs[i];
> +		const char *key = find_src ? refspec->dst : refspec->src;
> +		const char *value = find_src ? refspec->src : refspec->dst;
> +		const char *needle = find_src ? query->dst : query->src;
> +		char **result = find_src ? &query->src : &query->dst;
>  
> -	for (i = 0; i < remote->fetch_refspec_nr; i++) {
> -		struct refspec *fetch = &remote->fetch[i];
> -		const char *key = find_src ? fetch->dst : fetch->src;
> -		const char *value = find_src ? fetch->src : fetch->dst;
> -		if (!fetch->dst)
> +		if (!refspec->dst)
>  			continue;
> -		if (fetch->pattern) {
> +		if (refspec->pattern) {
>  			if (match_name_with_pattern(key, needle, value, result)) {
> -				refspec->force = fetch->force;
> +				query->force = refspec->force;
>  				return 0;
>  			}
>  		} else if (!strcmp(needle, key)) {
>  			*result = xstrdup(value);
> -			refspec->force = fetch->force;
> +			query->force = refspec->force;
>  			return 0;
>  		}
>  	}
> +
>  	return -1;
>  }
>  
> +char *apply_refspecs(struct refspec *refspecs, int nr_refspec,
> +		     const char *name)
> +{
> +	struct refspec query;
> +
> +	memset(&query, 0x0, sizeof(struct refspec));
> +	query.src = (char *) name;
> +
> +	if (query_refspecs(refspecs, nr_refspec, &query))
> +		return NULL;
> +
> +	return query.dst;
> +}
> +
> +int remote_find_tracking(struct remote *remote, struct refspec *refspec)
> +{
> +	return query_refspecs(remote->fetch, remote->fetch_refspec_nr, refspec);
> +}
> +
>  static struct ref *alloc_ref_with_prefix(const char *prefix, size_t prefixlen,
>  		const char *name)
>  {

^ permalink raw reply

* Re: [PATCH] send-email: Honour SMTP domain when using TLS
From: Brian Gernhardt @ 2011-10-16  4:18 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Matthew Daley, git
In-Reply-To: <7v62jpvc8f.fsf@alter.siamese.dyndns.org>


On Oct 15, 2011, at 11:36 PM, Junio C Hamano wrote:

> Matthew Daley <mattjd@gmail.com> writes:
> 
>> git-send-email sends two SMTP EHLOs when using TLS encryption, however
>> only the first, unencrypted EHLO uses the SMTP domain that can be
>> optionally specified by the user (--smtp-domain).  This is because the
>> call to hello() that produces the second, encrypted EHLO does not pass
>> the SMTP domain as an argument, and hence a default of
>> 'localhost.localdomain' is used instead.
>> 
>> Fix by passing in the SMTP domain in this call.
>> 
>> Signed-off-by: Matthew Daley <mattjd@gmail.com>
> 
> Sounds sensible, thanks.
> 
> Brian, who did 69cf7bf (send-email: Cleanup smtp-domain and add config,
> 2010-04-10) to make the initial hello use $smtp_domain, could you take a
> quick look and an Ack?

It does look sensible to me, but I wasn't the one who added all the HELO work.  I mostly was mucking about with variable names and how they got set.  Jari Aalto added them originally in 134550f.

~~ Brian

^ permalink raw reply

* Re: [PATCH] send-email: Honour SMTP domain when using TLS
From: Junio C Hamano @ 2011-10-16  3:36 UTC (permalink / raw)
  To: Matthew Daley; +Cc: git, Brian Gernhardt
In-Reply-To: <1318668292-13818-1-git-send-email-mattjd@gmail.com>

Matthew Daley <mattjd@gmail.com> writes:

> git-send-email sends two SMTP EHLOs when using TLS encryption, however
> only the first, unencrypted EHLO uses the SMTP domain that can be
> optionally specified by the user (--smtp-domain).  This is because the
> call to hello() that produces the second, encrypted EHLO does not pass
> the SMTP domain as an argument, and hence a default of
> 'localhost.localdomain' is used instead.
>
> Fix by passing in the SMTP domain in this call.
>
> Signed-off-by: Matthew Daley <mattjd@gmail.com>

Sounds sensible, thanks.

Brian, who did 69cf7bf (send-email: Cleanup smtp-domain and add config,
2010-04-10) to make the initial hello use $smtp_domain, could you take a
quick look and an Ack?

>  git-send-email.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index 6885dfa..d491db9 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -1098,7 +1098,7 @@ X-Mailer: git-send-email $gitversion
>  					$smtp_encryption = '';
>  					# Send EHLO again to receive fresh
>  					# supported commands
> -					$smtp->hello();
> +					$smtp->hello($smtp_domain);
>  				} else {
>  					die "Server does not support STARTTLS! ".$smtp->message;
>  				}

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Sitaram Chamarty @ 2011-10-16  1:51 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Junio C Hamano, Jakub Narebski, Jeff King, Nguyen Thai Ngoc Duy,
	git, Ilari Liusvaara, Johannes Sixt
In-Reply-To: <20111015221711.GA17470@elie.hsd1.il.comcast.net>

On Sun, Oct 16, 2011 at 3:47 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Junio C Hamano wrote:
>
>> The admin has access to logs that record the real cause anyway, no?
>
> Yes, you're right.  If this is a good admin then she will look at the
> logs, preventing the back-and-forth Sitaram described.

Actually, even if it's a good admin, you're adding to her load needlessly.

> Though that doesn't really change anything fundamental.  It seems nice
> to remind the end user to check for typos, too.

Yup.

DId I mention "been there, done that" in my earlier email?  I'm not
bike-shedding -- there *is* an impact on productivity in terms of how
people troubleshoot when they run across a problem, and I really *do*
feel strongly about this in principle (even though I don't use
git-daemon myself so it doesn't bother me how you decide in this
*specific* case)

I'll shut up now... :-)

-- 
Sitaram

^ permalink raw reply

* Re: [PATCH] git-gui: fix multi selected file operation
From: Pat Thoyts @ 2011-10-15 22:48 UTC (permalink / raw)
  To: Bert Wesarg; +Cc: git
In-Reply-To: <87cab38f99075f149a9abe7caf4ec139a0a48213.1318580310.git.bert.wesarg@googlemail.com>

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>The current path for what we see the diff is not in the list of selected
>paths. But when we add single paths (with Ctrl) to the set the current path
>would not be used when the action is performed.
>
>Fix this by explicitly putting the path into the list before we start
>showing the diff.
>
>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>---
> git-gui.sh |    1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
>diff --git a/git-gui.sh b/git-gui.sh
>index f897160..e5dd8bc 100755
>--- a/git-gui.sh
>+++ b/git-gui.sh
>@@ -2474,6 +2474,7 @@ proc toggle_or_diff {w x y} {
> 				[concat $after [list ui_ready]]
> 		}
> 	} else {
>+		set selected_paths($path) 1
> 		show_diff $path $w $lno
> 	}
> }

It is not clear what I should be looking for to test this. Can you
re-write the commit message to be more clear about what you are
fixing. Is this multiple unstaged files in the staging box? If so I
don't see what path display is changing.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* Re: [RFC/PATCH 4/4] git-gui: incremental goto line in blame view
From: Pat Thoyts @ 2011-10-15 22:26 UTC (permalink / raw)
  To: Bert Wesarg; +Cc: David Fries, git
In-Reply-To: <7a9760b8cf85274b17c7233f61f59bb59cd18578.1318513492.git.bert.wesarg@googlemail.com>

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>The view jumps now to the given line number after each key press.
>
>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>---
>
>I didn't know this before, but gedits goto-line-dialog works this way.
>
> lib/line.tcl |   15 +++++++++++----
> 1 files changed, 11 insertions(+), 4 deletions(-)
>
>diff --git a/lib/line.tcl b/lib/line.tcl
>index 70785e1..0113e06 100644
>--- a/lib/line.tcl
>+++ b/lib/line.tcl
>@@ -20,7 +20,7 @@ constructor new {i_w i_text args} {
> 		-background lightgreen \
> 		-validate key \
> 		-validatecommand [cb _validate %P]
>-	${NS}::button $w.bn      -text [mc Go] -command [cb _incrgoto]
>+	${NS}::button $w.bn      -text [mc Go] -command [cb _goto]
> 
> 	pack   $w.l   -side left
> 	pack   $w.bn  -side right
>@@ -29,7 +29,8 @@ constructor new {i_w i_text args} {
> 	eval grid conf $w -sticky we $args
> 	grid remove $w
> 
>-	bind $w.ent <Return> [cb _incrgoto]
>+	trace add variable linenum write [cb _goto_cb]
>+	bind $w.ent <Return> [cb _goto]
> 	bind $w.ent <Escape> [cb hide]
> 
> 	bind $w <Destroy> [list delete_this $this]
>@@ -67,10 +68,16 @@ method _validate {P} {
> 	return 0
> }
> 
>-method _incrgoto {} {
>+method _goto_cb {name ix op} {
>+	after idle [cb _goto 1]
>+}
>+
>+method _goto {{nohide {0}}} {
> 	if {$linenum ne {}} {
> 		$ctext see $linenum.0
>-		hide $this
>+		if {!$nohide} {
>+			hide $this
>+		}
> 	}
> }

Works fine. Will apply.

OK These 4 patches are applied and pushed to master branch - with
modifications made to 'only accept numbers in the goto-line input' made
as mentioned.

Thanks,

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* Re: [PATCH 1/4] git-gui: search and linenumber input are mutual exclusive in the blame view
From: Pat Thoyts @ 2011-10-15 22:22 UTC (permalink / raw)
  To: Bert Wesarg; +Cc: David Fries, git
In-Reply-To: <1d1c91fdaa0bfd31067fd2e06f3f1ecf5597b8d3.1318513492.git.bert.wesarg@googlemail.com>

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>It was possible to open the search input (Ctrl+S) and the goto-line input
>(Ctrl+G) at the same time. Prevent this.
>
>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>---
> lib/blame.tcl |   22 ++++++++++++++++------
> 1 files changed, 16 insertions(+), 6 deletions(-)
>
>diff --git a/lib/blame.tcl b/lib/blame.tcl
>index 2099776..691941e 100644
>--- a/lib/blame.tcl
>+++ b/lib/blame.tcl
>@@ -280,11 +280,11 @@ constructor new {i_commit i_path i_jump} {
> 	$w.ctxm add command \
> 		-label [mc "Find Text..."] \
> 		-accelerator F7 \
>-		-command [list searchbar::show $finder]
>+		-command [cb _show_finder]
> 	$w.ctxm add command \
> 		-label [mc "Goto Line..."] \
> 		-accelerator "Ctrl-G" \
>-		-command [list linebar::show $gotoline]
>+		-command [cb _show_linebar]
> 	menu $w.ctxm.enc
> 	build_encoding_menu $w.ctxm.enc [cb _setencoding]
> 	$w.ctxm add cascade \
>@@ -351,13 +351,13 @@ constructor new {i_commit i_path i_jump} {
> 	bind $w_cviewer <Tab>       "[list focus $w_file];break"
> 	bind $w_cviewer <Button-1>   [list focus $w_cviewer]
> 	bind $w_file    <Visibility> [cb _focus_search $w_file]
>-	bind $top       <F7>         [list searchbar::show $finder]
>-	bind $top       <Key-slash>  [list searchbar::show $finder]
>-	bind $top    <Control-Key-s> [list searchbar::show $finder]
>+	bind $top       <F7>         [cb _show_finder]
>+	bind $top       <Key-slash>  [cb _show_finder]
>+	bind $top    <Control-Key-s> [cb _show_finder]
> 	bind $top       <Escape>     [list searchbar::hide $finder]
> 	bind $top       <F3>         [list searchbar::find_next $finder]
> 	bind $top       <Shift-F3>   [list searchbar::find_prev $finder]
>-	bind $top    <Control-Key-g> [list linebar::show $gotoline]
>+	bind $top    <Control-Key-g> [cb _show_linebar]
> 	catch { bind $top <Shift-Key-XF86_Switch_VT_3> [list searchbar::find_prev $finder] }
> 
> 	grid configure $w.header -sticky ew
>@@ -1349,4 +1349,14 @@ method _resize {new_height} {
> 	set old_height $new_height
> }
> 
>+method _show_finder {} {
>+	linebar::hide $gotoline
>+	searchbar::show $finder
>+}
>+
>+method _show_linebar {} {
>+	searchbar::hide $finder
>+	linebar::show $gotoline
>+}
>+
> }

Looks good. Will apply.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* Re: [PATCH 2/4] git-gui: clear the goto line input when hiding
From: Pat Thoyts @ 2011-10-15 22:20 UTC (permalink / raw)
  To: Bert Wesarg; +Cc: David Fries, git
In-Reply-To: <a59d40509d4f80a6dae99bae5ef6311bb607bd34.1318513492.git.bert.wesarg@googlemail.com>

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>---
> lib/line.tcl |    1 +
> 1 files changed, 1 insertions(+), 0 deletions(-)
>
>diff --git a/lib/line.tcl b/lib/line.tcl
>index 4913bdd..692485a 100644
>--- a/lib/line.tcl
>+++ b/lib/line.tcl
>@@ -41,6 +41,7 @@ method show {} {
> 
> method hide {} {
> 	if {[visible $this]} {
>+		$w.ent delete 0 end
> 		focus $ctext
> 		grid remove $w
> 	}

You don't say why this one gets cleared but the search box is not
cleared.

-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Jonathan Nieder @ 2011-10-15 22:17 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jakub Narebski, Sitaram Chamarty, Jeff King, Nguyen Thai Ngoc Duy,
	git, Ilari Liusvaara, Johannes Sixt
In-Reply-To: <7vzkh1vrdq.fsf@alter.siamese.dyndns.org>

Junio C Hamano wrote:

> The admin has access to logs that record the real cause anyway, no?

Yes, you're right.  If this is a good admin then she will look at the
logs, preventing the back-and-forth Sitaram described.

Though that doesn't really change anything fundamental.  It seems nice
to remind the end user to check for typos, too.

^ permalink raw reply

* Re: [PATCH 3/4] git-gui: only except numbers in the goto-line input
From: Pat Thoyts @ 2011-10-15 22:17 UTC (permalink / raw)
  To: Bert Wesarg; +Cc: David Fries, git
In-Reply-To: <fbfb3f3ba4db190f8956eea4f78419a1b81573a6.1318513492.git.bert.wesarg@googlemail.com>

Bert Wesarg <bert.wesarg@googlemail.com> writes:

>Signed-off-by: Bert Wesarg <bert.wesarg@googlemail.com>
>---
> lib/line.tcl |   16 ++++++++++++++--
> 1 files changed, 14 insertions(+), 2 deletions(-)
>
>diff --git a/lib/line.tcl b/lib/line.tcl
>index 692485a..70785e1 100644
>--- a/lib/line.tcl
>+++ b/lib/line.tcl
>@@ -15,7 +15,11 @@ constructor new {i_w i_text args} {
> 
> 	${NS}::frame  $w
> 	${NS}::label  $w.l       -text [mc "Goto Line:"]
>-	entry  $w.ent -textvariable ${__this}::linenum -background lightgreen
>+	entry  $w.ent \
>+		-textvariable ${__this}::linenum \
>+		-background lightgreen \
>+		-validate key \
>+		-validatecommand [cb _validate %P]
> 	${NS}::button $w.bn      -text [mc Go] -command [cb _incrgoto]
> 
> 	pack   $w.l   -side left
>@@ -26,7 +30,7 @@ constructor new {i_w i_text args} {
> 	grid remove $w
> 
> 	bind $w.ent <Return> [cb _incrgoto]
>-	bind $w.ent <Escape> [list linebar::hide $this]
>+	bind $w.ent <Escape> [cb hide]
> 
> 	bind $w <Destroy> [list delete_this $this]
> 	return $this
>@@ -55,6 +59,14 @@ method editor {} {
> 	return $w.ent
> }
> 
>+method _validate {P} {
>+	# only accept numbers as input
>+	if {[regexp {\d*} $P]} {
>+		return 1
>+	}
>+	return 0
>+}
>+
> method _incrgoto {} {
> 	if {$linenum ne {}} {
> 		$ctext see $linenum.0

This one doesn't actually work when I try it it accepts alphanumeric
input. However, replacing the validate body with
  string is integer $P
fixes it to operate as intended so I'll replace it with this.
Looks like it needs theming too but that can be a separate patch.
-- 
Pat Thoyts                            http://www.patthoyts.tk/
PGP fingerprint 2C 6E 98 07 2C 59 C8 97  10 CE 11 E6 04 E0 B9 DD

^ permalink raw reply

* Re: [PATCH 2/8] t1402: Ignore a few cases that must fail due to DOS path expansion
From: Junio C Hamano @ 2011-10-15 20:15 UTC (permalink / raw)
  To: Pat Thoyts; +Cc: Git, msysGit, Johannes Schindelin
In-Reply-To: <87k4862wmk.fsf@fox.patthoyts.tk>

Pat Thoyts <patthoyts@users.sourceforge.net> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>>Pat Thoyts <patthoyts@users.sourceforge.net> writes:
>>
>>>  t/t1402-check-ref-format.sh |   15 +++++++++------
>>
>>Didn't we see a different patch that attempts to address the same issue
>>recently on the list from J6t, or is this a fix for a different problem?
>>
>
> You are correct - I'll leave this out of this series then. j6t's patch
> is an alternative fix for the same problem.

Thanks for checking.

^ permalink raw reply

* Re: [PATCH 7/8] mergetools: use the correct tool for Beyond Compare 3 on Windows
From: Junio C Hamano @ 2011-10-15 20:14 UTC (permalink / raw)
  To: Sebastian Schuberth; +Cc: git, msysgit
In-Reply-To: <4E996012.8090002@gmail.com>

Sebastian Schuberth <sschuberth@gmail.com> writes:

> On 15.10.2011 07:50, Junio C Hamano wrote:
>
>> Hmm, does this only apply to Windows, or are there other platforms on
>> which BC3 supplies bcomp for the exact same reason? What I am trying to
>
> BC3 is only available for Linux and Windows, so it only applies to
> Windows currently.

Who asked anything about "currently"?

^ permalink raw reply

* Re: [PATCHv3] daemon: give friendlier error messages to clients
From: Junio C Hamano @ 2011-10-15 20:13 UTC (permalink / raw)
  To: Jonathan Nieder
  Cc: Jakub Narebski, Sitaram Chamarty, Jeff King, Nguyen Thai Ngoc Duy,
	git, Ilari Liusvaara, Johannes Sixt
In-Reply-To: <20111015082647.GA7302@elie.hsd1.il.comcast.net>

Jonathan Nieder <jrnieder@gmail.com> writes:

> ... The admin who is eventually forwarded this message
> will be reminded ...

The admin has access to logs that record the real cause anyway, no?
I thought this was all about how the error is given to the end user.

^ permalink raw reply

* Re: [PATCH] Git-p4: git-p4.changeOnSubmit to do 'change' instead of 'submit'.
From: Luke Diamand @ 2011-10-15 20:10 UTC (permalink / raw)
  To: Andrei Warkentin; +Cc: git, gitster, Pete Wyckoff
In-Reply-To: <1318629110-15232-1-git-send-email-andreiw@vmware.com>

On 14/10/11 22:51, Andrei Warkentin wrote:
> Many users of p4/sd use changelists for review, regression
> tests and batch builds, thus changes are almost never directly
> submitted.
>
> This new config option lets a 'p4 change -i' run instead of
> the 'p4 submit -i'.
>
> Signed-off-by: Andrei Warkentin<andreiw@vmware.com>
> ---
>   contrib/fast-import/git-p4     |   16 ++++++++++++----
>   contrib/fast-import/git-p4.txt |   10 ++++++++++
>   2 files changed, 22 insertions(+), 4 deletions(-)
>
> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> index 2f7b270..19c295b 100755
> --- a/contrib/fast-import/git-p4
> +++ b/contrib/fast-import/git-p4
> @@ -959,7 +959,10 @@ class P4Submit(Command, P4UserMap):
>                   submitTemplate = message[:message.index(separatorLine)]
>                   if self.isWindows:
>                       submitTemplate = submitTemplate.replace("\r\n", "\n")
> -                p4_write_pipe("submit -i", submitTemplate)
> +                if gitConfig("git-p4.changeOnSubmit"):
> +                    p4_write_pipe("change -i", submitTemplate)
> +                else:
> +                    p4_write_pipe("subadasdmit -i", submitTemplate)


What does "p4 subadasmit" do? That's a new command to me!

(This patch also fails to apply cleanly to my shell-metacharacter patch).

>
>                   if self.preserveUser:
>                       if p4User:
> @@ -981,9 +984,14 @@ class P4Submit(Command, P4UserMap):
>               file = open(fileName, "w+")
>               file.write(self.prepareLogMessage(template, logMessage))
>               file.close()
> -            print ("Perforce submit template written as %s. "
> -                   + "Please review/edit and then use p4 submit -i<  %s to submit directly!"
> -                   % (fileName, fileName))
> +            if gitConfig("git-p4.changeOnSubmit"):
> +                print ("Perforce submit template written as %s. "
> +                       + "Please review/edit and then use p4 change -i<  %s to create changelist!"
> +                       % (fileName, fileName))
> +            else:
> +                print ("Perforce submit template written as %s. "
> +                       + "Please review/edit and then use p4 submit -i<  %s to submit directly!"
> +                       % (fileName, fileName))
>
>       def run(self, args):
>           if len(args) == 0:
> diff --git a/contrib/fast-import/git-p4.txt b/contrib/fast-import/git-p4.txt
> index 52003ae..3a3a815 100644
> --- a/contrib/fast-import/git-p4.txt
> +++ b/contrib/fast-import/git-p4.txt
> @@ -180,6 +180,16 @@ git-p4.allowSubmit
>
>     git config [--global] git-p4.allowSubmit false
>
> +git-p4.changeOnSubmit
> +
> +  git config [--global] git-p4.changeOnSubmit false
> +
> +Most places using p4/sourcedepot don't actually want you submit

Small typo: should be "want you *to* submit"

> +changes directly, and changelists are used to do regression testing,
> +batch builds and review, hence, by setting this parameter to
> +true you acknowledge you end up creating a changelist which you
> +must then manually commit.
> +
>   git-p4.syncFromOrigin
>
>   A useful setup may be that you have a periodically updated git repository

Regards!
Luke

^ permalink raw reply

* Re: [PATCH v2 0/6] git-p4 tests, filetypes, shell metacharacters
From: Luke Diamand @ 2011-10-15 20:00 UTC (permalink / raw)
  To: Pete Wyckoff; +Cc: git, Junio C Hamano, Chris Li
In-Reply-To: <20111015155358.GA29436@arf.padd.com>

On 15/10/11 16:53, Pete Wyckoff wrote:
> This series of six patches has been reviewed on the mailing list
> and should be ready for inclusion.
>
> 1    - Clean up git-p4 tests, including Junio review comments
>
> 2..4 - Handle p4 filetypes better, including Chris's suggested
>         utf16 fix
>
> 5    - Fix p4 keyword parsing
>
> 6    - From Luke: avoid using the shell, so filenames with metacharacters
>         are not errantly expanded

I've just tried running the tests in parallel (master branch) and it 
works a treat.

You've also tidied up my earlier test cases very nicely.

There doesn't seem to be any way to cope with multiple instances of the 
same test running at the same time. If p4d could write its pid somewhere 
then something better could be done, but it doesn't.

Ack.

Thanks
Luke


>
> Luke Diamand (1):
>        git-p4: handle files with shell metacharacters
>
> Pete Wyckoff (5):
>        git-p4 tests: refactor and cleanup
>        git-p4: handle utf16 filetype properly
>        git-p4: recognize all p4 filetypes
>        git-p4: stop ignoring apple filetype
>        git-p4: keyword flattening fixes
>
>   contrib/fast-import/git-p4     |  287 +++++++++++++-------
>   t/lib-git-p4.sh                |   74 +++++
>   t/t9800-git-p4.sh              |  574 ++++++++++++++++++----------------------
>   t/t9801-git-p4-branch.sh       |  233 ++++++++++++++++
>   t/t9802-git-p4-filetype.sh     |  108 ++++++++
>   t/t9803-git-shell-metachars.sh |   64 +++++
>   6 files changed, 918 insertions(+), 422 deletions(-)
>   create mode 100644 t/lib-git-p4.sh
>   create mode 100755 t/t9801-git-p4-branch.sh
>   create mode 100755 t/t9802-git-p4-filetype.sh
>   create mode 100755 t/t9803-git-shell-metachars.sh

^ permalink raw reply

* Re: interrupting "git rebase" (Re: git rebase +)
From: Jonathan Nieder @ 2011-10-15 19:44 UTC (permalink / raw)
  To: Martin von Zweigbergk
  Cc: Johannes Sixt, Adam Piatyszek, git, Ramkumar Ramachandra
In-Reply-To: <CAOeW2eFK5vSKmf+nxzD-6yh5CWZRv4WqSerbSXTPtXmtNeNjxg@mail.gmail.com>

(+cc: Ram)
Martin von Zweigbergk wrote:
> On Thu, Oct 13, 2011 at 10:26 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> Johannes Sixt wrote:

>>> Hitting Ctrl-C during git-rebase results undefined behavior.
[...]
>> Wait, really?  That's bad, and unlike most git commands.
>
> If Ctrl-C is pressed while the state is being written, it could be
> left with incomplete information, yes. It has been like that forever I
> think. I'll put it on my todo list, but it will not be my top priority
> (and I have very little git time now anyways).

Sorry for the lack of clarity.  That actually sounds fine to me
already, as long as the intermediate state is easy to recover from
(which doesn't match what I usually think of as "undefined behavior").

So it sounds like I was worrying needlessly.

[...]
>> By the way, what happened to the "git rebase --abort-softly" synonym
>> for "rm -fr .git/rebase-apply" discussed a while ago?
>
> I think we simply did not agree on a syntax, but here was also some
> discussion about future plans for the sequencer. I remember seeing
> some discussions about making "git reset --hard" remove the sequencer
> state, but I don't remember the conclusion. It is not clear to me what
> is ok to implement in git-rebase nowadays and what would just be
> double work if it needs to be re-implemented in the sequencer.

I believe a good rule of thumb is that you can always pretend the
sequencer just doesn't exist, and cc Ram when you are unsure. :)

Certainly, a lot of sequencer features were inspired by "git rebase".
Improvements to "git rebase" are only likely to make future
improvements to "git sequencer" easier.  Part of what helps here is
that "git rebase" is a shell script, so it is a little easier to
prototype features there.

Thanks!
Jonathan

^ 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