* 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: [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: 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] 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
* [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: 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] 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: [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
* 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 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: [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: [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 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
* [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: [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
* 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: [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 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: [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: [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: [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: [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 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: [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 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
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