* 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
* Re: Bug#553296: gitignore broken completely
From: Jeff King @ 2009-10-30 17:38 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: 553296, Junio C Hamano, git
In-Reply-To: <20091030165903.GA10671@ikki.ethgen.de>
[cc'ing git@vger and quoting excessively to give those readers context]
On Fri, Oct 30, 2009 at 05:59:03PM +0100, Klaus Ethgen wrote:
> Am Fr den 30. Okt 2009 um 17:28 schrieb Gerrit Pape:
> > > The most recent git in debian has a broken ignore handling. Let me
> > > show
> > > it on a example:
> > > > mkdir gittest
> > > > cd gittest
> > > > git init
> > > > touch a
> > > > git add a
> > > > git commit -m commit
> > > > git ls-files -i --exclude-standard
> > >
> > > The last command will show the file a (as it would show every file as
> > > being ignored, every which is in the index!). But that command should
> > > show nothing at that point.
> >
> > Hi Klaus, it looks like upstream did that on purpose
> >
> > http://git.kernel.org/?p=git/git.git;a=commit;h=b5227d80
> >
> > $ git describe --contains b5227d80
> > v1.6.5.2~2^2^2
It was not exactly on purpose. The point of that change was that "git
ls-files" should also show things mentioned in the .gitignore, because
.gitignore has nothing to do whatsoever with tracked files.
But I simply forgot about "git ls-files -i", so changing it was an
unintended side effect (and sadly, we don't seem to have any regression
tests for it).
> That makes the whole sense of -i ad absurdum (I do not know how to tell
> "ad absurdum" in english). With that patch there is no way anymore to
> see if some ignored files are accidentally committed.
Yes, with the current code "-i" serves no purpose unless you are using
"-o".
> I have to use git often as frontend for that broken design (svn). So to
> hold the ignores up2date I use "git svn show-ignore > .gitignore" But
> many, many repositories have broken ignores so I have to check it with
> "git ls-files -i --exclude-standard" to see if there is accidentally
> ignored files. Well, that patch makes that impossible at all!
Just to be clear: nothing is actually broken about ignores that cover
tracked files. Ignores are (and have been since long before this patch)
purely about untracked files. So there is no problem at all with:
$ echo content >foo
$ git add foo
$ echo foo >.gitignore
$ git commit
The _only_ thing that respected such ignores was "git ls-files", and the
point of the patch in question was to fix that.
> So I think, this patch is a bug at all!
>
> I add Jeff (and Junio as he did the commit) to the Cc. @Jeff and or
> Junio: could you please revoke that patch?
I am not sure simply reverting is the best choice; the patch does do
something useful. And while it strictly breaks backwards compatibility
on the output without "-i", the old behavior was considered a bug. But
the "-i" behavior is useless now, so we need to figure out how to
proceed. We can:
1. Revert and accept that the behavior is historical. Callers need to
work around it by dropping --exclude* when invoking ls-files.
2. Declare "-i" useless, deprecate and/or remove. Obviously this is
also breaking existing behavior, but again, I don't think that
using "-i" actually accomplishes anything.
3. Revert for now, and then reinstate the patch during a major version
bump when we are declaring some compatibility breakages.
4. Re-work "-i" to show tracked but ignored files, but still show all
files when "-i" is not given at all.
I think (4) preserves the benefit of the patch in question, but still
allows your usage ("git ls-files -i --exclude-standard"). I do question
whether that usage is worth supporting. Certainly I wouldn't implement
it if I were writing git-ls-files from scratch today, but we do in
general try to maintain backwards compatibility, especially for plumbing
like ls-files.
Junio, what do you want to do?
-Peff
^ permalink raw reply
* Re: [PATCH] mergetool--lib: add p4merge as a pre-configured mergetool option
From: Charles Bailey @ 2009-10-30 17:44 UTC (permalink / raw)
To: Scott Chacon, Junio C Hamano, Jay Soffian; +Cc: git list, David Aguilar
In-Reply-To: <d411cc4a0910281439v3388c243v42b3700f73744623@mail.gmail.com>
On Wed, Oct 28, 2009 at 02:39:32PM -0700, Scott Chacon wrote:
> p4merge is now a built-in diff/merge tool.
> This adds p4merge to git-completion and updates
> the documentation to mention p4merge.
>
> Signed-Off-By: Scott Chacon <schacon@gmail.com>
> ---
Acked-by: Charles Bailey <charles@hashpling.org>
I'm aware that we haven't reached full agreement on the best way to
make p4merge + git as Mac OS X friendly as possible, but Jay said that
this patch is 'good enough' and I agree. If we go with this for now,
we're not closing the door to further improvements.
I confirmed a (perhaps very minor) issue with the abspath approach,
the parameters are used as the title of the pains in p4merge, so
(IMHO) short paths look neater, especially if you've cd'ed down to a
low level and don't have a 1920 pixel wide monitor. p4merge does
truncate from the left so it's not a big thing.
The other thing which I confirmed is that p4merge on windows doesn't
like /dev/null as an explicit parameter (it gets convered to nul: by
msys, I believe, but it still doesn't like it).
p4merge does appear to do 'magic' when the base and left are the same
parameter (not just the same file - the magic doesn't work if you,
say, use a relative path and an absolute path to refer to the same
file. This means that it doesn't just take the right changes which is
what I feared it might do when I first saw the 'local' 'local'
'remote' pattern.
Charles.
--
Charles Bailey
http://ccgi.hashpling.plus.com/blog/
^ permalink raw reply
* Re: [RFC PATCH v4 03/26] pkt-line: Make packet_read_line easier to debug
From: Jeff King @ 2009-10-30 17:57 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: Junio C Hamano, git
In-Reply-To: <20091029215829.GD10505@spearce.org>
On Thu, Oct 29, 2009 at 02:58:29PM -0700, Shawn O. Pearce wrote:
> Junio C Hamano <gitster@pobox.com> wrote:
> > > The NUL assignment isn't about safe_read(), its about making the
> > > block of 4 bytes read a proper C-style string so we can safely pass
> > > it down into the snprintf that is within die().
> >
> > I knew and understood all of what you just said. static linelen[] is not
> > about stack allocation. It's about letting the compiler initialize it to
> > five NULs so that you do not have to assign NUL to its fifth place before
> > you die. This removes one added line from your patch.
>
> Thanks, I get it now. Patch amended.
I am just a bystander, so maybe my opinion is not worth anything, but
personally I think you are obfuscating the code to save a single line.
When I see a static variable, I assume it is because the value should be
saved from invocation to invocation, and now I will spend time wondering
why that would be the case here.
If you really just want to initialize to zero, using
char linelen[5] = { 0 };
should be sufficient (I think all of the compilers we care about support
such incomplete array assignments, right?).
I think it would be even more clear to leave it as
char linelen[4];
which declares how the data is _actually_ expected to be used, and then
do:
die("protocol error: bad line length character: %.4s", linelen);
-Peff
PS All of the proposals also suffer from the fact that die will truncate
broken output at a NUL sent by the remote. Probably OK, since we are
expecting ascii hex or some variant, and if we don't correctly print
what the remote said in a totally broken NUL-filled case, it is not that
big a deal.
^ permalink raw reply
* Re: finding the merge of two or more commits
From: Jeff King @ 2009-10-30 18:04 UTC (permalink / raw)
To: Avery Pennarun; +Cc: E R, git
In-Reply-To: <32541b130910291434q1b068918x38c5aec543cb1c2a@mail.gmail.com>
On Thu, Oct 29, 2009 at 05:34:45PM -0400, Avery Pennarun wrote:
> On Thu, Oct 29, 2009 at 5:12 PM, E R <pc88mxer@gmail.com> wrote:
> > That is, I don't want to merge c1 and c2 myself, but I want to know if
> > someone else has merged c1 and c2, performed any conflict resolution
> > and committed the result.
>
> Try this:
>
> (git branch -a --contains c1; git branch -a --contains c2) | sort | uniq -d
>
> It's not exactly pretty, but it works.
That will tell you whether any branch contains both of them, which is
not exactly what the original poster asked for (though it may have been
what he meant :) ). To see if there is a specific merge of those two
commits, you can do:
git log --all --format='%H %P' | grep " $c1" | grep " $c2" | cut -d' ' -f1
-Peff
^ permalink raw reply
* Re: [PATCH] gitignore: root most patterns at the top-level directory
From: Jeff King @ 2009-10-30 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vmy3cys0f.fsf@alter.siamese.dyndns.org>
On Tue, Oct 27, 2009 at 11:03:28PM -0700, Junio C Hamano wrote:
> How does .cvsignore and .svnignore work? Don't they have the same issue,
> and perhaps worse as I do not recall seeing a way to anchor a pattern to a
> particular directory like we do in their .SCMignore files? And judging
> from the fact that they can get away with the lack of that "feature", this
> perhaps is not an issue in real life?
Happily, I did not remember how .cvsignore worked and had to go read the
documentation. :) The answer is that no, it does not have the recursive
feature in the root .cvsignore list at all. But it does apply the
repo-wide CVSROOT/cvsignore, the user's ~/.cvsignore, and the
environment's $CVSIGNORE to all directories. So it is safe from this
problem (though now that I think on it, I think I was once bitten by
something similar in the CVSROOT/cvsignore).
SVN implements this with "properties" on the directories. They are not
recursive at all. However, it also implements "global-ignores" which
applies everywhere.
So no, they don't have the same issue, because they explicitly split the
"everywhere" and "this directory" cases into two locations. We wouldn't
want to use .git/info/excludes for this, because we do want to support
the project shipping some global excludes itself.
> I am actually a bit reluctant to queue this, even though I most likely
> will, and then hope that we will think of a better solution later, at
> which time this file again needs to change.
I am mixed on it, as well. I did see it bite someone, but I think it's
very rare, and everyone who reads or touches the file will have to deal
with the ugliness every time. If you want to drop the patch, I will not
complain.
> For example, it crossed my mind that perhaps we can change the ignore
> rules so that a non-globbing pattern is automatically anchored at the
> current directly but globbing ones are recursive as before.
That makes some sense to me (and in fact when making the patch, it was
the rule of thumb I used). Though I think you might want to make it
"starts with glob" as the trigger for the rule. We have "git-core-*/?*",
which I would expect to still be anchored at the root.
> If we do so, there is no need to change the current .gitignore entires.
> You need to spell a concrete filename as a glob pattern that matches only
> one path if you want the recursive behaviour. E.g. if you have a Makefile
> per subdirectory, each of which generates and includes Makefile.depend
> file, you would write "Makefile.depen[d]" in the toplevel .gitignore file.
While clever, that use of '[d]' seems unneccessarily obscure to me. Why
not just give a wildcard for "any subdirectory of me" and do:
Makefile.depend
**/Makefile.depend
Since "**" is in common use in other systems, it's pretty clear (to me,
anyway, but then I am the one suggesting the syntax ;) ) what that
means.
-Peff
^ permalink raw reply
* Re: [PATCH 0/3] increase user-friendliness
From: Jeff King @ 2009-10-30 18:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <alpine.DEB.1.00.0910291159110.3687@felix-maschine>
On Thu, Oct 29, 2009 at 11:59:35AM +0100, Johannes Schindelin wrote:
> On Wed, 28 Oct 2009, Jeff King wrote:
>
> > Git has a reputation as being unfriendly to users. Let's fix that by
> > adding some features implemented by more user-friendly programs.
>
> So the bar for pirate patches has been raised to patch series now?
Well, I wasn't intending to do a series, but strangely when you ask in a
room full of git developers, "what ridiculous joke patch would you like
me to code this afternoon?", you get way more answers than you wanted. I
had to start rejecting suggestions. ;)
-Peff
PS Thanks to all who responded with ridiculous suggestions in your
reviews. I'm not going to bother keeping the thread alive by responding
to each, though. :)
^ permalink raw reply
* Re: [PATCH] diff --color-words -U0: fix the location of hunk headers
From: Junio C Hamano @ 2009-10-30 18:40 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: markus.heidelberg, git
In-Reply-To: <alpine.DEB.1.00.0910301831190.5383@felix-maschine>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> The reason I did not do that was to avoid a full subroutine call, as I
> expected this code path to be very expensive.
This is only done for the "word diff" mode, and my gut feeling is that it
is not such a big issue. But you can always inline it if it is
performance critical.
The current structure shows how this code evolved. fn_out_consume() used
to be "this is called every time a line worth of diff becomes ready from
the lower-level diff routine, and here is what we do to output line
oriented diff using that line."
When we introduced the "word diff" mode, we could have done three things.
* change fn_out_consume() to "this is called every time a line worth of
diff becomes ready from the lower-level diff routine. This function
knows two sets of helpers (one for line-oriented diff, another for word
diff), and each set has various functions to be called at certain
places (e.g. hunk header, context, ...). The function's role is to
inspect the incoming line, and dispatch appropriate helpers to produce
either line- or word- oriented diff output."
* introduce fn_out_consume_word_diff() that is "this is called every time
a line worth of diff becomes ready from the lower-level diff routine,
and here is what we do to prepare word oriented diff using that line."
without touching fn_out_consume() at all.
* Do neither of the above, and keep fn_out_consume() to "this is called
every time a line worth of diff becomes ready from the lower-level diff
routine, and here is what we do to output line oriented diff using that
line." but sprinkle a handful of 'are we in word-diff mode? if so do
this totally different thing' at random places.
My clean-up "something like this" patch was to at least abstract the
details of "this totally different thing" out from the main codepath, in
order to improve readability.
We can of course refactor this by introducing fn_out_consume_word_diff(),
i.e. the second route above. Then we do not have to check "are we in word
diff mode" for every line of diff and that would give you even better
performance.
^ permalink raw reply
* Re: Bug#553296: gitignore broken completely
From: Jeff King @ 2009-10-30 18:41 UTC (permalink / raw)
To: Klaus Ethgen; +Cc: 553296, Junio C Hamano, git
In-Reply-To: <20091030182331.GC10671@ikki.ethgen.de>
On Fri, Oct 30, 2009 at 07:23:31PM +0100, Klaus Ethgen wrote:
> Well ls-files is used to see such broken files. (Another example is if
> you accidentally add a file which you (later) decide to be ignored. You
> will have no change to find that files at all anymore.) With the patch
> that use case of ls-files has been gone without a replacement.
I think your reasoning is a little bit circular. They are not actually
broken with respect to git. But they might represent an error on the
part of the programmer, and one which they would want to investigate.
You could also catch it by looking at diffs ("why is this file in my
diff, it is supposed to be ignored"), but I am not opposed to accessing
the information via git-ls-files (and I think you are right that the
query cannot be easily done any other way).
> I have two more options:
>
> 5. Revert the patch and rework it to have a option to ignore or
> respect the excluded files (Such as --use-excludes for example) or
> respect them anyway if a --exclude* option is given on command
> line.
I think that is basically equivalent to (1). There is already a way for
callers to avoid the bug, which is not to provide --exclude* at all. So
you are already setting that one bit of information in whether or not
you provide any excludes.
> 6. Revert the patch and rework it so that it will only have effect if
> there is no -i option on the command line. (That is similiar to a
> mix of 3 and 4.)
Yeah, that would actually be the least invasive change, and would keep
"-i" just as it is. If we do anything except a simple, I think your (6)
makes the most sense.
Let me see if I can make a patch.
-Peff
^ 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