* Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option
From: Markus Heidelberg @ 2009-10-30 10:35 UTC (permalink / raw)
To: Jay Soffian
Cc: Charles Bailey, Scott Chacon, git list, Junio C Hamano,
David Aguilar
In-Reply-To: <76718490910292000t7b024b83y68d71b6ff810c15@mail.gmail.com>
Jay Soffian, 30.10.2009:
> On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg
> <markus.heidelberg@web.de> wrote:
> > He didn't mean p4merge on other platforms, but other merge tools on Mac
> > OS X. What about all the other merge tools already in mergetool--lib?
> > Should they get special handling, too?
>
> If someone wants to scratch that itch, then yes. The default diff tool
> for OS X has its helper already in /usr/bin (opendiff). p4merge is
> arguably a better merge tool, and it installs as an app bundle in
> /Applications. I'm not sure about the other diff tools, I haven't
> looked.
>
> > And for Windows we could add C:\Program Files\MergeToolX\tool.exe for
> > every merge tool.
>
> If it makes those tools easier to use with git, and if someone on
> Windows wants to scratch that itch, then yes, we should.
Another possible problem: the user can change the installation
destination on Windows. What's the behaviour of Mac OS here? Is the
instalation path fixed or changeable?
Markus
^ permalink raw reply
* Re: [PATCH] clone: detect extra arguments
From: Jonathan Nieder @ 2009-10-30 11:19 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20091029160614.GB7622@sigill.intra.peff.net>
Jeff King wrote:
> Should we maybe be showing the usage in this case?
Sounds reasonable. How about this patch on top?
-- %< --
Subject: [PATCH] clone: print usage on wrong number of arguments
git clone's short usage string is only 22 lines, so an error
message plus usage string still fits comfortably on an 80x24
terminal.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin-clone.c | 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
diff --git a/builtin-clone.c b/builtin-clone.c
index 76ad581..736d9e1 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -378,10 +378,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
builtin_clone_usage, 0);
if (argc > 2)
- die("Too many arguments.");
+ usage_msg_opt("Too many arguments.",
+ builtin_clone_usage, builtin_clone_options);
if (argc == 0)
- die("You must specify a repository to clone.");
+ usage_msg_opt("You must specify a repository to clone.",
+ builtin_clone_usage, builtin_clone_options);
if (option_mirror)
option_bare = 1;
--
1.6.5.2
^ permalink raw reply related
* Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option
From: Reece Dunn @ 2009-10-30 11:25 UTC (permalink / raw)
To: markus.heidelberg
Cc: Jay Soffian, Charles Bailey, Scott Chacon, git list,
Junio C Hamano, David Aguilar
In-Reply-To: <200910301135.59831.markus.heidelberg@web.de>
2009/10/30 Markus Heidelberg <markus.heidelberg@web.de>:
> Jay Soffian, 30.10.2009:
>> On Thu, Oct 29, 2009 at 9:02 PM, Markus Heidelberg
>> <markus.heidelberg@web.de> wrote:
>> > He didn't mean p4merge on other platforms, but other merge tools on Mac
>> > OS X. What about all the other merge tools already in mergetool--lib?
>> > Should they get special handling, too?
>>
>> If someone wants to scratch that itch, then yes. The default diff tool
>> for OS X has its helper already in /usr/bin (opendiff). p4merge is
>> arguably a better merge tool, and it installs as an app bundle in
>> /Applications. I'm not sure about the other diff tools, I haven't
>> looked.
>>
>> > And for Windows we could add C:\Program Files\MergeToolX\tool.exe for
>> > every merge tool.
>>
>> If it makes those tools easier to use with git, and if someone on
>> Windows wants to scratch that itch, then yes, we should.
>
> Another possible problem: the user can change the installation
> destination on Windows. What's the behaviour of Mac OS here? Is the
> instalation path fixed or changeable?
For Windows, the program should have an InstallDir or similar registry
value in a fixed place in the registry to point to where it is
installed (something like
HKLM/Software/[Vendor]/[Application]/[Version]).
As for Linux, there is no guarantee that things like p4merge are in
the path either. It could be placed under /opt/perforce or
/home/perforce.
What would be sensible (for all platforms) is:
1/ if [difftool|mergetool].toolname.path is set, use that (is this
documented?)
2/ try looking for the tool in the system path
3/ try some intelligent guessing
4/ if none of these work, print out an error message -- ideally,
this should mention the configuration option in (1)
(3) is what is being discussed. It is good that it will work without
any user configuration (especially for standard tools installed in
standard places), but isn't really a big problem as long as the user
is prompted to configure the tool path. Also, I'm not sure how this
will work with multiple versions of the tools installed (e.g. vim/gvim
and p4merge).
- Reece
^ permalink raw reply
* Re: [PATCH 02/19] Allow fetch to modify refs
From: Sverre Rabbelier @ 2009-10-30 12:22 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Git List, Johannes Schindelin, Johan Herland
In-Reply-To: <alpine.LNX.2.00.0910300151450.14365@iabervon.org>
Heya,
On Thu, Oct 29, 2009 at 22:56, Daniel Barkalow <barkalow@iabervon.org> wrote:
>> +++ b/builtin-clone.c
>> @@ -526,8 +526,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>>
>> refs = transport_get_remote_refs(transport);
>> if (refs) {
>> - mapped_refs = wanted_peer_refs(refs, refspec);
>> - transport_fetch_refs(transport, mapped_refs);
>> + struct ref *ref_cpy = wanted_peer_refs(refs, refspec);
>> + mapped_refs = ref_cpy;
>> + transport_fetch_refs(transport, ref_cpy);
>
> Just drop this hunk; the change doesn't actually do anything. Otherwise,
> the patch matches what I have.
Am I missing something? I thought we need this hunk since mapped_refs
is const, and transport_fetch_refs takes a non-const argument?
builtin-clone.c: In function ‘cmd_clone’:
builtin-clone.c:531: warning: passing argument 2 of
‘transport_fetch_refs’ discards qualifiers from pointer target type
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [UNSTABLE PATCH 03/19] Allow programs to not depend on remotes having urls
From: Sverre Rabbelier @ 2009-10-30 12:24 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Git List, Johannes Schindelin, Johan Herland
In-Reply-To: <alpine.LNX.2.00.0910300157170.14365@iabervon.org>
Heya,
On Thu, Oct 29, 2009 at 23:02, Daniel Barkalow <barkalow@iabervon.org> wrote:
> Perhaps I should try to get the refactor in
> builtin-push to use push_with_options() without any behavior change into
> master ahead of the rest of this series.
I think that's a good idea in general :).
> Anyway, you correctly understood the intended change here.
Ok, nice.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 10/19] Allow helpers to request marks for fast-import
From: Sverre Rabbelier @ 2009-10-30 12:26 UTC (permalink / raw)
To: Johan Herland; +Cc: Git List, Johannes Schindelin, Daniel Barkalow
In-Reply-To: <200910300921.00561.johan@herland.net>
Heya,
On Fri, Oct 30, 2009 at 01:21, Johan Herland <johan@herland.net> wrote:
> Please drop this patch from the series. The functionality is not needed,
> since we'll use the fast-import "option" command from the sr/gfi-options
> series instead.
In that case I will rebase the series on top of sr/gfi-options then as
soon as I reroll that one. Also, do you need to change anything else
in git-remote-cvs to do that?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 12/19] 1/2: Add Python support library for CVS remote helper
From: Sverre Rabbelier @ 2009-10-30 12:27 UTC (permalink / raw)
To: Johan Herland
Cc: Git List, Johannes Schindelin, Daniel Barkalow, David Aguilar
In-Reply-To: <200910300933.35567.johan@herland.net>
Heya,
On Fri, Oct 30, 2009 at 01:33, Johan Herland <johan@herland.net> wrote:
> Why? Or: why that one, and not the others? Also, you might want to mention
> your contribution in the commit message itself.
What others? And I thought my contributions were so minor it doesn't
really matter that much :).
> It seems the two functions you add (notify() and warn()) have a different
> indentation than the existing code (which uses 4 spaces). Please fix.
That's weird, will fix.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 18/19] Refactor git_remote_cvs to a more generic git_remote_helpers
From: Sverre Rabbelier @ 2009-10-30 12:29 UTC (permalink / raw)
To: Johan Herland; +Cc: Git List, Johannes Schindelin, Daniel Barkalow
In-Reply-To: <200910300942.54101.johan@herland.net>
Heya,
On Fri, Oct 30, 2009 at 01:42, Johan Herland <johan@herland.net> wrote:
>> +- git/git - Interaction with Git repositories
>
> Since this is Python documentation within a package, I'd rather refer to the
> python modules as _modules_ and not files. I.e. please use '.' instead of
> '/':
Ok, will do.
>> +- util - General utility functionality use by the other modules in
>> + this package, and also used directly by git-remote-cvs.
>
> Probably you should drop the direct reference to git-remote-cvs.
Ah, yes, my bad.
>> + test -d ../git_remote_helpers/build || {
>> error "You haven't built git_remote_cvs yet, have you?"
>
> Please also s/git_remote_cvs/git_remote_helpers/ in the error message.
That's weird, I thought I did that, ah well, will fix.
> Otherwise, it all looks good :)
Thanks for the review.
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [Vcs-fast-import-devs] What's cooking in git.git (Oct 2009, #01; Wed, 07)
From: Sverre Rabbelier @ 2009-10-30 12:41 UTC (permalink / raw)
To: Shawn O. Pearce, Ian Clatworthy
Cc: vcs-fast-import-devs, git, Johannes Schindelin
In-Reply-To: <4AEA626D.8060804@canonical.com>
Heya,
On Thu, Oct 29, 2009 at 20:50, Ian Clatworthy
<ian.clatworthy@canonical.com> wrote:
>> On Thu, Oct 8, 2009 at 19:39, Shawn O. Pearce <spearce@spearce.org> wrote:
> Sverre Rabbelier wrote:
>> [edited Shawn's message somewhat to be more relevant to vcs-fast-import-dev]
>
> Thanks Sverre. Before I start, sorry for taking so long to reply to this.
Thanks for the review :).
> My strong preference is for:
>
> * feature = anything impacting semantics
> * option = tool-specific with no impact on semantics permitted.
>
> Both features and options ought to OS independent (where possible).
Even better, Shawn, if this LGTY I will reroll the series implementing this.
>>> I think we want to declare features for import-marks and export-marks
>>> and define these as paths to local files which store a VCS specific
>>> formatted mapping of fast-import mark numbers to VCS labels.
>
> Explicitly specifying the local path names worries me though. Consider someone
> using fastimport tools to maintain multiple mirrors in different tools.
> What should the stream look like then? Does it need to change if we want
> an additional mirror.
I think the stream should not have to change, which works if you
define the files to be local to the repo being exported to. That is,
in git the line "feature export-marks=out.marks" would result in a
marks file located in "/path/to/repo/.git/fast-import/out.marks". Or
is that not what you mean?
> +1. By forcing tools to know about options specific to them, we avoid a
> range of bugs processing newer streams with older tools.
It is not possible to change the semantics using options though, what
kind of bugs could arise this way?
> I don't think options should be permitted to change import behavior. In
> other words, we should actively discourage vcs-specific streams
Sounds fair, I reckon that a wiki in addition to the
vcs-fast-import-devs list would not hurt :).
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [RFC PATCH 06/19] Factor ref updating out of fetch_with_import
From: Sverre Rabbelier @ 2009-10-30 12:57 UTC (permalink / raw)
To: Daniel Barkalow; +Cc: Git List, Johannes Schindelin, Johan Herland
In-Reply-To: <alpine.LNX.2.00.0910300221290.14365@iabervon.org>
On Fri, Oct 30, 2009 at 00:10, Daniel Barkalow <barkalow@iabervon.org> wrote:
> Actually, I think the problem is that builtin-clone will always default to
> setting up a refspec of "refs/heads/*:refs/remotes/origin/*", but that
> doesn't actually make any sense if the source repository isn't presented
> as having refs names like "refs/heads/*". The immediate problem that you
> don't get HEAD because it's a symref to something outside the pattern is
> somewhat secondary to the general problem that you don't get anything at
> all, because everything's outside the pattern.
Ah, I didn't realize that as long as HEAD is a symref to something in
refs/heads/* it would be fetched.
> I don't really think that presenting the real refs in refs/<vcs>/*, and
> having the list give symrefs to them from refs/heads/* and refs/tags/*
> really makes sense; I think it would be better to rework fetch_with_import
> and the list provided by a foreign vcs helper such that it can present
> refs with normal names (refs/heads/*, refs/tags/*, etc) if the foreign vcs
> has standards that correspond to branches and tags. Then "clone" would
> work normally.
Perhaps we could introduce an additional phase before import to set
the default refspec? If we could tell git that we want
"refs/heads/*:refs/hg/origin/*" before line 468 "if (option_bare) {",
that would save us a lot of trouble. Does that sound like a good idea?
It would mean we have to move the transport_get part up a bit, but I
don't think that will be a problem. A svn helper for example might
respond the following to the "refspec" command:
"refs/heads/trunkr:refs/svn/origin/trunk"
"refs/tags/*:refs/svn/origin/tags/*"
"refs/heads/*:refs/svn/origin/branches/*"
How does that sound?
--
Cheers,
Sverre Rabbelier
^ permalink raw reply
* Re: [PATCH 7/8] Provide a build time default-editor setting
From: Jonathan Nieder @ 2009-10-30 13:17 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ben Walton, Johannes Sixt, David Roundy, GIT List
In-Reply-To: <20091030103558.GH1610@progeny.tock>
Jonathan Nieder wrote:
> Signed-off-by: Ben Walton <bwalton@bwalton@artsci.utoronto.ca>
That should be "Ben Walton <bwalton@artsci.utoronto.ca>", without the
extra bwalton@. Sorry for the trouble.
Regards,
Jonathan
^ permalink raw reply
* Re: [PATCH] clone: detect extra arguments
From: Jeff King @ 2009-10-30 14:45 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
In-Reply-To: <20091030111919.GA13242@progeny.tock>
On Fri, Oct 30, 2009 at 06:19:19AM -0500, Jonathan Nieder wrote:
> > Should we maybe be showing the usage in this case?
>
> Sounds reasonable. How about this patch on top?
I do think it's an improvement, but...
> -- %< --
> Subject: [PATCH] clone: print usage on wrong number of arguments
>
> git clone's short usage string is only 22 lines, so an error
> message plus usage string still fits comfortably on an 80x24
> terminal.
The extra blank lines introduced by usage_msg_opt make it 25 lines,
scrolling the message right off of my terminal screen. ;)
But looking at the usage message, there is some potential for cleanup.
So maybe this on top (or between your 1 and 2)?
-- >8 --
Subject: [PATCH] clone: hide "naked" option from usage message
This is just a little-known synonym for bare, and there is
little point in advertising both (we don't even include it
in the manpage). Removing it also makes the usage message
one line shorter, giving just enough room for an information
message in a 24-line terminal.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-clone.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
diff --git a/builtin-clone.c b/builtin-clone.c
index 736d9e1..ce0d79a 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -51,7 +51,9 @@ static struct option builtin_clone_options[] = {
OPT_BOOLEAN('n', "no-checkout", &option_no_checkout,
"don't create a checkout"),
OPT_BOOLEAN(0, "bare", &option_bare, "create a bare repository"),
- OPT_BOOLEAN(0, "naked", &option_bare, "create a bare repository"),
+ { OPTION_BOOLEAN, 0, "naked", &option_bare, NULL,
+ "create a bare repository",
+ PARSE_OPT_NOARG | PARSE_OPT_HIDDEN },
OPT_BOOLEAN(0, "mirror", &option_mirror,
"create a mirror repository (implies bare)"),
OPT_BOOLEAN('l', "local", &option_local,
--
1.6.5.1.143.g1dab74.dirty
^ permalink raw reply related
* Re: [PATCH] clone: detect extra arguments
From: Jeff King @ 2009-10-30 14:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johan Herland, Jonathan Nieder, git
In-Reply-To: <20091030144525.GA22583@coredump.intra.peff.net>
On Fri, Oct 30, 2009 at 10:45:25AM -0400, Jeff King wrote:
> But looking at the usage message, there is some potential for cleanup.
Also, we should probably do this (I did it as a patch on master, though,
as it is an independent fix):
-- >8 --
Subject: [PATCH] clone: fix --recursive usage message
Looks like a mistaken cut-and-paste in e7fed18a.
Signed-off-by: Jeff King <peff@peff.net>
---
builtin-clone.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-clone.c b/builtin-clone.c
index 5762a6f..436e8da 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -61,7 +61,7 @@ static struct option builtin_clone_options[] = {
OPT_BOOLEAN('s', "shared", &option_shared,
"setup as shared repository"),
OPT_BOOLEAN(0, "recursive", &option_recursive,
- "setup as shared repository"),
+ "initialize submodules in the clone"),
OPT_STRING(0, "template", &option_template, "path",
"path the template repository"),
OPT_STRING(0, "reference", &option_reference, "repo",
--
1.6.5.1.143.g1dab74.dirty
^ permalink raw reply related
* Re: [RFC PATCH v4 11/26] Move WebDAV HTTP push under remote-curl
From: Tay Ray Chuan @ 2009-10-30 15:02 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git, Clemens Buchacher, Daniel Barkalow, Mike Hommey
In-Reply-To: <1256774448-7625-12-git-send-email-spearce@spearce.org>
Hi,
On Thu, Oct 29, 2009 at 8:00 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> update http tests according to remote-curl capabilities
it would be great if you could mention the $ORIG_HEAD bit:
o Use a variable ($ORIG_HEAD) instead of full SHA-1 name.
--
Cheers,
Ray Chuan
^ permalink raw reply
* [PATCH] push: fix typo in usage
From: Jeff King @ 2009-10-30 15:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nanako Shiraishi, git
Missing ")".
Signed-off-by: Jeff King <peff@peff.net>
---
I posted this in $gmane/130293, but I think it simply got missed.
builtin-push.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/builtin-push.c b/builtin-push.c
index 7d78711..019c986 100644
--- a/builtin-push.c
+++ b/builtin-push.c
@@ -181,7 +181,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)
OPT_BIT( 0 , "all", &flags, "push all refs", TRANSPORT_PUSH_ALL),
OPT_BIT( 0 , "mirror", &flags, "mirror all refs",
(TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE)),
- OPT_BOOLEAN( 0 , "tags", &tags, "push tags (can't be used with --all or --mirror"),
+ OPT_BOOLEAN( 0 , "tags", &tags, "push tags (can't be used with --all or --mirror)"),
OPT_BIT( 0 , "purge", &flags,
"remove locally deleted refs from remote",
TRANSPORT_PUSH_PURGE),
--
1.6.5.1.143.g1dab74.dirty
^ permalink raw reply related
* Re: [PATCH 02/19] Allow fetch to modify refs
From: Daniel Barkalow @ 2009-10-30 15:16 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Git List, Johannes Schindelin, Johan Herland
In-Reply-To: <fabb9a1e0910300522je3d76aep160d87fe302f8779@mail.gmail.com>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1314 bytes --]
On Fri, 30 Oct 2009, Sverre Rabbelier wrote:
> Heya,
>
> On Thu, Oct 29, 2009 at 22:56, Daniel Barkalow <barkalow@iabervon.org> wrote:
> >> +++ b/builtin-clone.c
> >> @@ -526,8 +526,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >>
> >> refs = transport_get_remote_refs(transport);
> >> if (refs) {
> >> - mapped_refs = wanted_peer_refs(refs, refspec);
> >> - transport_fetch_refs(transport, mapped_refs);
> >> + struct ref *ref_cpy = wanted_peer_refs(refs, refspec);
> >> + mapped_refs = ref_cpy;
> >> + transport_fetch_refs(transport, ref_cpy);
> >
> > Just drop this hunk; the change doesn't actually do anything. Otherwise,
> > the patch matches what I have.
>
> Am I missing something? I thought we need this hunk since mapped_refs
> is const, and transport_fetch_refs takes a non-const argument?
>
> builtin-clone.c: In function ‘cmd_clone’:
> builtin-clone.c:531: warning: passing argument 2 of
> ‘transport_fetch_refs’ discards qualifiers from pointer target type
Ah, actually, mapped_refs should just be made non-const; unlike "refs",
it's set from wanted_peer_refs(), which returns a non-const value.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option
From: Jay Soffian @ 2009-10-30 15:17 UTC (permalink / raw)
To: Reece Dunn, Markus Heidelberg
Cc: Charles Bailey, Scott Chacon, git list, Junio C Hamano,
David Aguilar
In-Reply-To: <3f4fd2640910300425q602471a6v1111a7dceee7746c@mail.gmail.com>
On Fri, Oct 30, 2009 at 7:25 AM, Reece Dunn <msclrhd@googlemail.com> wrote:
> 2009/10/30 Markus Heidelberg <markus.heidelberg@web.de>:
>> Another possible problem: the user can change the installation
>> destination on Windows. What's the behaviour of Mac OS here? Is the
>> instalation path fixed or changeable?
This has already been answered. Yes the application can move on OS X,
but 9/10 it will be in one of two standard locations. There are ways
to find an application regardless of where it is, but it's maybe not
worth the platform specific complexity for that 1/10 time.
> For Windows, the program should have an InstallDir or similar registry
> value in a fixed place in the registry to point to where it is
> installed (something like
> HKLM/Software/[Vendor]/[Application]/[Version]).
And if someone wants to contribute the code to grub around the
registry on Windows, I'm all for it, as long as it doesn't negatively
impact non-Windows users (and similarly for any other platform
specific code -- don't impact users of other platforms negatively).
> As for Linux, there is no guarantee that things like p4merge are in
> the path either. It could be placed under /opt/perforce or
> /home/perforce.
No, of course not, but again, looking in PATH is likely to work in the
common case. By looking in /Application and $HOME/Applications, that
covers the common case on OS X.
> What would be sensible (for all platforms) is:
> 1/ if [difftool|mergetool].toolname.path is set, use that (is this
> documented?)
> 2/ try looking for the tool in the system path
> 3/ try some intelligent guessing
> 4/ if none of these work, print out an error message -- ideally,
> this should mention the configuration option in (1)
This is basically what is already done, but (3) isn't yet platform
specific in any way, and (4) doesn't mention the config option.
> (3) is what is being discussed. It is good that it will work without
> any user configuration (especially for standard tools installed in
> standard places), but isn't really a big problem as long as the user
> is prompted to configure the tool path. Also, I'm not sure how this
> will work with multiple versions of the tools installed (e.g. vim/gvim
> and p4merge).
There's a fixed order of tools, first tool that's found wins.
Oh, and my favorite color paint is blue. :-)
j.
^ permalink raw reply
* Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option
From: Markus Heidelberg @ 2009-10-30 15:30 UTC (permalink / raw)
To: Jay Soffian
Cc: Reece Dunn, Charles Bailey, Scott Chacon, git list,
Junio C Hamano, David Aguilar
In-Reply-To: <76718490910300817w776bde48j40de31e5532b9fd4@mail.gmail.com>
Jay Soffian, 30.10.2009:
> On Fri, Oct 30, 2009 at 7:25 AM, Reece Dunn <msclrhd@googlemail.com> wrote:
> > 3/ try some intelligent guessing
>
> This is basically what is already done, but (3) isn't yet platform
> specific in any way
Maybe this can be considered to be implemented. But since it's not
p4merge specific, the p4merge patch should firstly be applied without
the intelligence.
The intelligence may be implemented mergetool agnostic for all at once,
if that is possible?
Markus
^ permalink raw reply
* Re: [RFC PATCH 06/19] Factor ref updating out of fetch_with_import
From: Daniel Barkalow @ 2009-10-30 16:04 UTC (permalink / raw)
To: Sverre Rabbelier; +Cc: Git List, Johannes Schindelin, Johan Herland
In-Reply-To: <fabb9a1e0910300557x42d3612pf7e83907e91efdc9@mail.gmail.com>
On Fri, 30 Oct 2009, Sverre Rabbelier wrote:
> On Fri, Oct 30, 2009 at 00:10, Daniel Barkalow <barkalow@iabervon.org> wrote:
> > Actually, I think the problem is that builtin-clone will always default to
> > setting up a refspec of "refs/heads/*:refs/remotes/origin/*", but that
> > doesn't actually make any sense if the source repository isn't presented
> > as having refs names like "refs/heads/*". The immediate problem that you
> > don't get HEAD because it's a symref to something outside the pattern is
> > somewhat secondary to the general problem that you don't get anything at
> > all, because everything's outside the pattern.
>
> Ah, I didn't realize that as long as HEAD is a symref to something in
> refs/heads/* it would be fetched.
Clone only cares about the remote HEAD for the purpose of making
refs/remotes/origin/HEAD a symref to something. It doesn't really want a
SHA1 for it (except in order to guess that it's a symref to something that
the remote has dereferenced in its list); and, since it's a symref, no
objects have to be fetched for it -- except for the possibility that it's
known to be a symref to something that wasn't fetched, in which case, we
know that HEAD -> something, and something's SHA1 is sha1, but we didn't
fetch something so we don't have sha1. Of course, I think clone then
writes a dangling symref anyway and forgets about sha1.
> > I don't really think that presenting the real refs in refs/<vcs>/*, and
> > having the list give symrefs to them from refs/heads/* and refs/tags/*
> > really makes sense; I think it would be better to rework fetch_with_import
> > and the list provided by a foreign vcs helper such that it can present
> > refs with normal names (refs/heads/*, refs/tags/*, etc) if the foreign vcs
> > has standards that correspond to branches and tags. Then "clone" would
> > work normally.
>
> Perhaps we could introduce an additional phase before import to set
> the default refspec? If we could tell git that we want
> "refs/heads/*:refs/hg/origin/*" before line 468 "if (option_bare) {",
> that would save us a lot of trouble. Does that sound like a good idea?
> It would mean we have to move the transport_get part up a bit, but I
> don't think that will be a problem. A svn helper for example might
> respond the following to the "refspec" command:
> "refs/heads/trunkr:refs/svn/origin/trunk"
> "refs/tags/*:refs/svn/origin/tags/*"
> "refs/heads/*:refs/svn/origin/branches/*"
>
> How does that sound?
The values you've got for refspecs don't make sense as fetch refspecs;
these would cause refs with names the helper won't supply to be stored in
the helper's private namespace, which is certainly wrong. (And I think you
have the sides backwards)
On the other hand, I think it would make sense to use the same style of
refspec between the helper and transport-helper.c such that the helper
reports something like:
refs/svn/origin/trunk:refs/heads/trunkr
refs/svn/origin/branches/*:refs/heads/*
refs/svn/origin/tags/*:refs/tags/*
"list" gives:
000000...000 refs/heads/trunkr
"import refs/heads/trunkr" imports the objects, but the refspecs have to
be consulted by transport-helper.c in order to determine what ref to read
to get the value of refs/heads/trunkr. Instead of getting the value with
read_ref("refs/heads/trunkr", ...) like it does now, it would do
read_ref("refs/svn/origin/trunk", ...). And systems like p4 that don't
have a useful standard just wouldn't support the "refspec" command and
people would have to do site-specific configuration to get anything
useful.
-Daniel
*This .sig left intentionally blank*
^ permalink raw reply
* Re: [RFC PATCH v4 11/26] Move WebDAV HTTP push under remote-curl
From: Tay Ray Chuan @ 2009-10-30 16:10 UTC (permalink / raw)
To: Clemens Buchacher; +Cc: git, Shawn O. Pearce, Daniel Barkalow, Mike Hommey
In-Reply-To: <1256774448-7625-12-git-send-email-spearce@spearce.org>
Hi,
On Thu, Oct 29, 2009 at 8:00 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> diff --git a/t/t5540-http-push.sh b/t/t5540-http-push.sh
>[snip]
> @@ -57,11 +58,15 @@ test_expect_failure 'push to remote repository with packed refs' '
> test $HEAD = $(git rev-parse --verify HEAD))
> '
>
> -test_expect_success ' push to remote repository with unpacked refs' '
> +test_expect_failure 'push already up-to-date' '
> + git push
> +'
> +
> +test_expect_success 'push to remote repository with unpacked refs' '
> (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
> rm packed-refs &&
> - git update-ref refs/heads/master \
> - 0c973ae9bd51902a28466f3850b543fa66a6aaf4) &&
> + git update-ref refs/heads/master $ORIG_HEAD &&
> + git --bare update-server-info) &&
> git push &&
> (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
> test $HEAD = $(git rev-parse --verify HEAD))
Clemens, the following addresses your non-desire to remove the
"unpacked refs" test in your earlier email
<20091025161630.GB8532@localhost>.
Now that the first push in "push to remote repository with packed
refs" succeeds, the "unpacked refs" test is redundant. Since http-push
in that first test already updated /refs/heads/master and /info/refs,
'git update-ref' incorrectly reverts the earlier push, and 'git
update-server-info' is redundant.
--
Cheers,
Ray Chuan
^ permalink raw reply
* Re: [RFC PATCH v4 26/26] test smart http fetch and push
From: Tay Ray Chuan @ 2009-10-30 16:10 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: git, Clemens Buchacher
In-Reply-To: <1256774448-7625-27-git-send-email-spearce@spearce.org>
Hi,
On Thu, Oct 29, 2009 at 8:00 AM, Shawn O. Pearce <spearce@spearce.org> wrote:
> The top level directory "/git/" of the test Apache server is mapped
> through our git-http-backend CGI, but uses the same underlying
> repository space as the server's document root. This is the most
> simple installation possible.
Having "/git/" reside as a subdirectory in "/" where WebDAV is enabled
may be confusing to readers. I think we should use "/smart/" for the
CGI map, and consequently, use "/dumb/" for WebDAV repositories,
rather than the root "/" that it is occupying.
> diff --git a/t/t5541-http-push.sh b/t/t5541-http-push.sh
>[snip]
> +test_expect_success 'push to remote repository with packed refs' '
> + cd "$ROOT_PATH"/test_repo_clone &&
> + : >path2 &&
> + git add path2 &&
> + test_tick &&
> + git commit -m path2 &&
> + HEAD=$(git rev-parse --verify HEAD) &&
> + git push &&
> + (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
> + test $HEAD = $(git rev-parse --verify HEAD))
> +'
> +
> +test_expect_success 'push already up-to-date' '
> + git push
> +'
> +
> +test_expect_success 'push to remote repository with unpacked refs' '
> + (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
> + rm packed-refs &&
> + git update-ref refs/heads/master $ORIG_HEAD) &&
> + git push &&
> + (cd "$HTTPD_DOCUMENT_ROOT_PATH"/test_repo.git &&
> + test $HEAD = $(git rev-parse --verify HEAD))
> +'
Mention of "packed refs" should be removed from the description, and
the 'unpacked refs' test, irrelevant in this context, should be
removed too. The assumptions these tests are based on is relevant to
t5540, but not in t5541. My explanation follows.
(Clemens, the following also addresses your non-desire to remove the
"unpacked refs" test in your earlier email
<20091025161630.GB8532@localhost>.)
In the "old" (v1.6.5) http push mechanism, no refspec is passed to the
http-push helper. This shouldn't be the case, because match_refs is
done in transport.c::transport_push, but then transport->push_refs
isn't defined so this doesn't happen.
http-push is then depended on to learn of the remote refs and match
them itself, but it does badly at this, since it only recurses through
/refs/heads in the remote repository. None could be found, since
they've all been packed. Thus the push in the first test failed.
Nothing has been pushed to the remote repository. The following push
in the "unpacked refs" corrects this after "unpacking" the refs.
Clearly, these test are about the ability of http push mechanism to
learn of refs in /refs/heads and (lack thereof) from /packed-refs.
But in this patch series, this is no longer the case.
transport->push_refs is now defined, so what happens is that the
http-push helper is passed a refspec, unlike in the "old" mechanism.
http-push, using this refspec, now matches refs properly (though it
still does its recursion thing). Thus the push in the first test
(should) succeed. The push in the "unpacked refs" test is now
irrelevant, since the first push has already successfully pushed
changes to the remote repository.
So, what we should have is just one no-frills push test, keeping as
well the "push already up-to-date" test.
--
Cheers,
Ray Chuan
^ permalink raw reply
* [PATCH] bash completion: remove "diff.renameLimit." (with trailing dot)
From: Markus Heidelberg @ 2009-10-30 16:53 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git, Markus Heidelberg
This was accidentally added with commit 98171a0 (bash completion: Sync
config variables with their man pages).
Signed-off-by: Markus Heidelberg <markus.heidelberg@web.de>
---
contrib/completion/git-completion.bash | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index e3ddecc..e129ef9 100755
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1641,7 +1641,6 @@ _git_config ()
diff.external
diff.mnemonicprefix
diff.renameLimit
- diff.renameLimit.
diff.renames
diff.suppressBlankEmpty
diff.tool
--
1.6.5.2.86.g61663
^ permalink raw reply related
* Re: [PATCH] diff --color-words -U0: fix the location of hunk headers
From: Junio C Hamano @ 2009-10-30 17:20 UTC (permalink / raw)
To: markus.heidelberg; +Cc: Johannes Schindelin, Junio C Hamano, git
In-Reply-To: <200910291729.23562.markus.heidelberg@web.de>
Markus Heidelberg <markus.heidelberg@web.de> writes:
> I try to reword:
> With 2/3 and 3/3 I wanted to keep --color-words specific code in the
> block starting with
>
> if (ecbdata->diff_words) {
>
> and didn't want to contaminate the block starting with
>
> if (line[0] == '@') {
>
> with non-hunk-header content.
The contamination was already done long time ago. The way "color words"
mode piggybacks into the rest of the code is to let the line oriented diff
codepath run, capture the lines to its buffer so that it can split them
into words and compare, and hijack the control flow not to let the line
oriented diff to be output, and in the context of the original design,
Dscho's patch makes sense.
I do think that the way the "color words" logic has to touch line-oriented
codepaths unnecessarily makes it look intrusive; one reason is because it
exposes the state that is only interesting to the "color words" mode to
these places too much.
With a small helper function on top of Dscho's patch, I think the code can
be made a lot more readable. This way, "consume" codepath is made more
about "here is what we do when we get a line from the line-oriented diff
engine", and we can keep the knowledge of "color words" mode to the
minimum (no more than "here is where we may need to flush the buffer").
The helper hides the implementation details such as how we decide if we
have accumulated words or what we do when we do need to flush.
There is another large-ish "if (ecbdata->diff_words)" codeblock in
fn_out_consume() that peeks into *(ecbdata->diff_words); I think it should
be ejected from the main codepath for the same reason (i.e. readability).
.
Probably we can make a helper function that has only a single caller, like
this:
static void diff_words_use_line(char *line, unsigned long len,
struct emit_callback *ecbdata,
const char *plain, const char *reset)
{
if (line[0] == '-') {
diff_words_append(line, len,
&ecbdata->diff_words->minus);
return;
} else if (line[0] == '+') {
diff_words_append(line, len,
&ecbdata->diff_words->plus);
return;
}
diff_words_flush(ecbdata);
line++;
len--;
emit_line(ecbdata->file, plain, reset, line, len);
}
It would even be cleaner to change "diff_words_append()" function to do
all of the above.
But that is outside the scope of this comment.
diff.c | 26 ++++++++++++--------------
1 files changed, 12 insertions(+), 14 deletions(-)
diff --git a/diff.c b/diff.c
index b7ecfe3..8c66e4a 100644
--- a/diff.c
+++ b/diff.c
@@ -541,14 +541,18 @@ struct emit_callback {
FILE *file;
};
+/* In "color-words" mode, show word-diff of words accumulated in the buffer */
+static void diff_words_flush(struct emit_callback *ecbdata)
+{
+ if (ecbdata->diff_words->minus.text.size ||
+ ecbdata->diff_words->plus.text.size)
+ diff_words_show(ecbdata->diff_words);
+}
+
static void free_diff_words_data(struct emit_callback *ecbdata)
{
if (ecbdata->diff_words) {
- /* flush buffers */
- if (ecbdata->diff_words->minus.text.size ||
- ecbdata->diff_words->plus.text.size)
- diff_words_show(ecbdata->diff_words);
-
+ diff_words_flush(ecbdata);
free (ecbdata->diff_words->minus.text.ptr);
free (ecbdata->diff_words->minus.orig);
free (ecbdata->diff_words->plus.text.ptr);
@@ -656,12 +660,8 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
for (i = 0; i < len && line[i] == '@'; i++)
;
if (2 <= i && i < len && line[i] == ' ') {
- /* flush --color-words even for --unified=0 */
- if (ecbdata->diff_words &&
- (ecbdata->diff_words->minus.text.size ||
- ecbdata->diff_words->plus.text.size))
- diff_words_show(ecbdata->diff_words);
-
+ if (ecbdata->diff_words)
+ diff_words_flush(ecbdata);
ecbdata->nparents = i - 1;
len = sane_truncate_line(ecbdata, line, len);
emit_line(ecbdata->file,
@@ -691,9 +691,7 @@ static void fn_out_consume(void *priv, char *line, unsigned long len)
&ecbdata->diff_words->plus);
return;
}
- if (ecbdata->diff_words->minus.text.size ||
- ecbdata->diff_words->plus.text.size)
- diff_words_show(ecbdata->diff_words);
+ diff_words_flush(ecbdata);
line++;
len--;
emit_line(ecbdata->file, plain, reset, line, len);
^ permalink raw reply related
* [PATCH] Don't create the $GIT_DIR/branches directory on init
From: Robin Rosenberg @ 2009-10-30 17:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, spearce, sasa.zivkov, Robin Rosenberg
Git itself does not even look at this directory. Any tools that
actually needs it should create it itself.
Signed-off-by: Robin Rosenberg <robin.rosenberg@dewire.com>
---
templates/branches-- | 1 -
1 files changed, 0 insertions(+), 1 deletions(-)
delete mode 100644 templates/branches--
Shawn and other wants to stop JGit from creating this directory on
init with the motivation that newer Git version doesn't create it
anymore. This patch would make that assertion true.
-- robin
diff --git a/templates/branches-- b/templates/branches--
deleted file mode 100644
index fae8870..0000000
--- a/templates/branches--
+++ /dev/null
@@ -1 +0,0 @@
-: this is just to ensure the directory exists.
--
1.6.5.2.102.g1f8896
^ permalink raw reply related
* Re: [PATCH] diff --color-words -U0: fix the location of hunk headers
From: Johannes Schindelin @ 2009-10-30 17:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: markus.heidelberg, git
In-Reply-To: <7v3a50n6hw.fsf@alter.siamese.dyndns.org>
Hi,
On Fri, 30 Oct 2009, Junio C Hamano wrote:
> Markus Heidelberg <markus.heidelberg@web.de> writes:
>
> > I try to reword:
> > With 2/3 and 3/3 I wanted to keep --color-words specific code in the
> > block starting with
> >
> > if (ecbdata->diff_words) {
> >
> > and didn't want to contaminate the block starting with
> >
> > if (line[0] == '@') {
> >
> > with non-hunk-header content.
>
> The contamination was already done long time ago.
Actually, it was don on purpose.
> diff --git a/diff.c b/diff.c
> index b7ecfe3..8c66e4a 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -541,14 +541,18 @@ struct emit_callback {
> FILE *file;
> };
>
> +/* In "color-words" mode, show word-diff of words accumulated in the buffer */
> +static void diff_words_flush(struct emit_callback *ecbdata)
> +{
> + if (ecbdata->diff_words->minus.text.size ||
> + ecbdata->diff_words->plus.text.size)
> + diff_words_show(ecbdata->diff_words);
> +}
Instead of this function, you can check the same at the beginning of
diff_words_show().
The reason I did not do that was to avoid a full subroutine call, as I
expected this code path to be very expensive.
Ciao,
Dscho
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox