Git development
 help / color / mirror / Atom feed
* Re: git-subtree Ready #2
From: David A. Greene @ 2012-02-21  5:37 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Jeff King, git, Avery Pennarun
In-Reply-To: <7vd399jdwc.fsf@alter.siamese.dyndns.org>

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

> Jeff King <peff@peff.net> writes:
>
> It sounds like the simplest and cleanest would be to treat it as if its
> current version came as a patch submission, cook it just like any other
> topic in 'pu' down to 'next' down to eventually 'master', with the usual
> review cycle of pointing out what is wrong and needs fixing followed by a
> series of re-rolls.

Ok, but we will preserve the history via the subtree merge, yes?

> The total amount of change does not look too bad, either:

Yes, it's a fairly small tool.

> It does look like it needs to start its life in contrib/ if we were to put
> this in git.git. 

That sounds good to me.  It should get a good shakedown before graduating.

> I haven't looked at the script fully, but it has an issue from its
> first line, which is marked with "#!/bin/bash".  It is unclear if it
> is infested by bash-isms beyond repair (in which case "#!/bin/bash" is
> fine), or it was written portably but was marked with "#!/bin/bash"
> just by inertia.  A patch that corresponds to the above diffstat
> immediately shows many style issues including trailing eye-sore
> whitespaces.

Ok.

> It seems that it is even capable of installing from contrib/subtree, so
> keeping it in contrib/ while many issues it may have gets fixed would not
> hurt the original goal of giving the script more visibility.

Right, I intentially designed it that way.

> The change to t/test-lib.sh should be made independent of this topic, I
> would think.

Ok, I'll propose those changes separately.  They are a prerequisite for
a git-subtree that is easily testable while in contrib.

> ----------------------------------------------------------------
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index e28d5fd..c877a91 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -55,6 +55,7 @@ unset $(perl -e '
>  		.*_TEST
>  		PROVE
>  		VALGRIND
> +                BUILD_DIR
>  	));
>  	my @vars = grep(/^GIT_/ && !/^GIT_($ok)/o, @env);
>  	print join("\n", @vars);
> @@ -924,7 +925,15 @@ then
>  	# itself.
>  	TEST_DIRECTORY=$(pwd)
>  fi
> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
> +
> +if test -z "$GIT_BUILD_DIR"
> +then
> +    echo Here
> +	# We allow tests to override this, in case they want to run tests
> +	# outside of t/, e.g. for running tests on the test library
> +	# itself.
> +        GIT_BUILD_DIR="$TEST_DIRECTORY"/..
> +fi
>  
>  if test -n "$valgrind"
>  then
> ----------------------------------------------------------------
> This change deserves its own justification.

I'll put a patch together with a more extensive explanation.  Basically,
tests run outside of the top-level t/ directory don't work because there
are all sort of assumptions in test-lib.sh about where they live.  There
are comments in test-lib.sh indicating that it should support tests in
other directories but I could not make it work out of the box.

> After looking at the history of subtree branch there, however, I agree
> that it would not help anybody to have its history in my tree with log
> messages like these (excerpt from shortlog output):
>
>       update todo
>       Some todo items reported by pmccurdy
>       todo
>       Docs: when pushing to github, the repo path needs to end in .git
>       todo
>       todo^
>       todo
>       todo: idea for a 'git subtree grafts' command

Ok, these are Avery's commits.  I don't know that I have enough context
to improve the logs but I will look throught revisions and try to figure
things out.  Avery, could you be of any help here?  It sounds like we
need more descriptive log messages.

                               -Dave

^ permalink raw reply

* Re: git-subtree Ready #2
From: Junio C Hamano @ 2012-02-21  6:34 UTC (permalink / raw)
  To: David A. Greene; +Cc: Jeff King, git, Avery Pennarun
In-Reply-To: <87ehtowxu7.fsf@smith.obbligato.org>

greened@obbligato.org (David A. Greene) writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>> It sounds like the simplest and cleanest would be to treat it as if its
>> current version came as a patch submission, cook it just like any other
>> topic in 'pu' down to 'next' down to eventually 'master', with the usual
>> review cycle of pointing out what is wrong and needs fixing followed by a
>> series of re-rolls.
>
> Ok, but we will preserve the history via the subtree merge, yes?

I'll comment on just this part, but a short answer is "no, I do not think
so".

Even though you left "Jeff King writes", you removed everything he said
that I was quoting, and in order to understand why the answer is 'no', it
would have been better if you kept this part from what he said in your
reply:

>> ... Either way, I do think it's
>> worth saving the commit history by doing a real merge.

as that was what I was agreeing to with my "as if ... a patch submission".

>> After looking at the history of subtree branch there, however, I agree
>> that it would not help anybody to have its history in my tree with log
>> messages like these (excerpt from shortlog output):
>>
>>       update todo
>>       Some todo items reported by pmccurdy
>>       todo
>>       Docs: when pushing to github, the repo path needs to end in .git
>>       todo
>>       todo^
>>       todo
>>       todo: idea for a 'git subtree grafts' command
>
> Ok, these are Avery's commits.  I don't know that I have enough context
> to improve the logs but I will look throught revisions and try to figure
> things out.  Avery, could you be of any help here?  It sounds like we
> need more descriptive log messages.

That was not what I was suggesting.

I was saying that the history up to the current state, littered with these
commits that are not "logical progression" but merely "a snapshot of
then-current state" may not be worth preserving, with or without better
messages.

Rewriting the entire history to make it a logical progression just for the
sake of history is obviously not worth the effort.

Which suggests that taking the end result that exists at the tip of your
subtree branch as a single code-drop, without pulling its history, lets us
start from a reasonably well tested state and would not lose us anything
of value.  And that was what I was suggesting.  For our history to explain
why/how the code got there better, another approach might be to instead
treat your bd7b2cf (Add 'contrib/subtree/' from commit '2793ee6ba...',
2012-01-29), which is where you took Avery's then-current state, as the
code-drop event that adds everything in contrib/subtree/ with a single
patch submission. I.e. in a git.git repository:

	git checkout -b subtree master
	git fetch git://sources.obbligato.org/git/git.git subtree
        git merge --squash bd7b2cf
        git commit -m "contrib: add git-subtree from Avery's tree"

to take the tip of your subtree branch.  The history up to that point is
in Avery's repository where he stopped, which such an approach will not
pull in to git.git.  And then we can replay bd7b2cf..FETCH_HEAD like so:

	git checkout FETCH_HEAD
	git rebase --onto subtree bd7b2cf
	git push . HEAD:subtree
        git checkout pu
        git merge subtree

to preserve the history since that single code-drop event that records
your effort to adjust the code-dump into a better shape to live in our
contrib/ area.  That will make it clear the division of blame on the code
added to git.git between Avery (everything before the squashed merge) and
you (everything after that).

^ permalink raw reply

* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Junio C Hamano @ 2012-02-21  7:05 UTC (permalink / raw)
  To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Michael J Gruber, pclouds, j.sixt
In-Reply-To: <4F42E4C2.7070801@in.waw.pl>

Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:

> On 02/21/2012 12:41 AM, Junio C Hamano wrote:
>> Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl>  writes:
>>
>>> JC:
>>>> Perhaps the maximum for garph_width should be raised to something like
>>>> "min(80, stat_width) - name_width"?
>>> I think that a graph like
>>> a | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>> b |    1 -
>>> is not very readable. I like the consistency forced by the 40-column limit.
>>> But I guess that this is very subjective.
>>
>> The above makes it very obvious that there is a huge amount of change made
>> to 'a' and a bit of deletion to 'b', compared to a mini-graph that is
>> truncated to half the screen width.
> Yes. But the same graph with 40 columns tells me exactly the same thing.

That is a bogus argument, isn't it?  You can say the same thing if you
limited the length of the graph bars to 10-columns if you only compare
between 1000 and 1. You can even do with just 5-columns.  For that matter,
without any graph bar at all, it tells us exactly the same thing because
we have numbers.  Does that mean we do not need any bar?  Of course not.
We use bars as visual aid.

Imagine what happens to the graph if you had paths with medium amount of
changes like 980, 800, 40, in addition to 1000 and 1.  By limiting the
length of the bars more than necessary, you are losing the resolution
without a good reason, and that is why I find 40-column limit a poor
design choice.

>> Besides, the above is what you would get without your patch on 80-column
>> terminal, no?
>
> Yes.

I think this "use at most 40-places for the graph bar" was your response
to somebody's observation that "on 200-column terminal, we will still see
the commit log messages (and for many projects, also patch text) that are
designed to be comfortably viewable within the 80-column on the left, and
overlong graph bar stands out like an ugly sore thumb".

While that "ugliness" observation might be a valid one to make, I do not
think limiting the length of the graph bar without taking the length of
the name part into account at all is the right solution to it.

After all, that is exactly the same thinking that led to the bug in the
current code that you fixed with your series, isn't it?  Our safety code
truncated the graph bar width too early without taking the width needed to
show the names into account, and then when the names turn out to be all
short, we ended up wasting space on the right hand side, because we made
the bars too short and the decision was made too early in the code.

If the problem you are addressing is to make sure that the diffstat part
in the series of lines that are structured like this:

   log message part ~80 column
   diff stat part that can extend very very very very very very very long
   patch text part  ~80 column

does not become overly long, wouldn't it be a more natural solution to
make sure that when the total (i.e. name and graph) length can fit to
align with the message and patch (i.e. traditional ~80 col regardless of
the terminal width), not to give it too much width?  If the names are
short, like "a" and "b", that may result in graph bar part to use ~70
columns or so, and if the names are long, like in a Java project, you may
allocate 50 columns to the name and leave only 50 columns or so for the
graph part.

A simple heuristic might be to see if name part (without truncation) and
the graph part (without scaling) fits under 100-columns if the terminal is
wider than that, and if so limit the whole thing to 100-columns before
deciding the allocation of the total into two parts.  If the name part
alone is very wide, showing the name and the graph using the whole
terminal width would give you a better result than using the bars that are
artificially capped to a short limit, I would imagine.

^ permalink raw reply

* Re: git-subtree Ready #2
From: Junio C Hamano @ 2012-02-21  7:10 UTC (permalink / raw)
  To: David A. Greene; +Cc: Jeff King, git, Avery Pennarun
In-Reply-To: <7vwr7gitjl.fsf@alter.siamese.dyndns.org>

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

> greened@obbligato.org (David A. Greene) writes:
>
>> Junio C Hamano <gitster@pobox.com> writes:
>>
>>> Jeff King <peff@peff.net> writes:
>>>
>>> It sounds like the simplest and cleanest would be to treat it as if its
>>> current version came as a patch submission, cook it just like any other
>>> topic in 'pu' down to 'next' down to eventually 'master', with the usual
>>> review cycle of pointing out what is wrong and needs fixing followed by a
>>> series of re-rolls.
>>
>> Ok, but we will preserve the history via the subtree merge, yes?
>
> I'll comment on just this part, but a short answer is "no, I do not think
> so".
>
> Even though you left "Jeff King writes", you removed everything he said
> that I was quoting, and in order to understand why the answer is 'no', it
> would have been better if you kept this part from what he said in your
> reply:
>
>>> ... Either way, I do think it's
>>> worth saving the commit history by doing a real merge.
>
> as that was what I was agreeing to with my "as if ... a patch submission".

Ehh, s/agree/disagree/;

^ permalink raw reply

* Re: git status: small difference between stating whole repository and small subdirectory
From: Piotr Krukowiecki @ 2012-02-21  7:21 UTC (permalink / raw)
  To: Junio C Hamano
  Cc: Jeff King, Thomas Rast, Nguyen Thai Ngoc Duy, Git Mailing List
In-Reply-To: <7v8vjxjd47.fsf@alter.siamese.dyndns.org>

On Tue, Feb 21, 2012 at 12:31 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jeff King <peff@peff.net> writes:
[...]

Hi,

I hope the fixes will also help git-svn users? I.e. there's a lot of
rebasing (and cherry-picking - at least in my case) and probably some
other stuff going on under hood. I.e. I hope that if I git-svn rebase
or cherry-pick, it won't invalidate the cache...

Thanks,
-- 
Piotr Krukowiecki

^ permalink raw reply

* Re: [RFC] pre-rebase: Refuse to rewrite commits that are reachable from upstream
From: Junio C Hamano @ 2012-02-21  7:44 UTC (permalink / raw)
  To: Johan Herland; +Cc: git, jnareb, philipoakley
In-Reply-To: <CALKQrgcDDVH8rxn80ZTMyR3y3n3Vs1LSCtH=ZiwG7it_LE2wgQ@mail.gmail.com>

Johan Herland <johan@herland.net> writes:

> What  you are describing here may be a common workflow, but
> "rebase.bottomLimit" is still very specific to that kind of workflow.
> What I'm after is a much more workflow-agnostic concept of:
>
>   "If I have pushed something, I should probably not rebase it"

Your "this branch pushes directly to that remote branch, so I can check if
it will result in rewrite of published commit" is even *less* generic than
having a single bottomLimit in my illustration.

I may not push out my topic branches directly, only the aggregate of them
in 'next', but once 'next' is pushed out, they are not eligible for
rebasing.  A per-branch bottom, e.g. rebase.$branch.bottomLimit, might
make it more flexible to cover such a case, though.

On the other hand, without any such safety, a merge to 'next' would give
many conflicts and "shortlog master..next" will show many duplicates after
any topic that are already merged to it are accidentally rewritten, and it
is just the matter of using reflog on topic branches to recover from such
a mistake.

>> I wonder if it would be a more direct solution to the issue you are
>> raising to give them a good tool to help them to be more careful with less
>> effort on their part before they publish (not before they rebase).
> 
> ..., I'm not sure how we can help the user _not_ publish the
> branch until it's ready.

I think we are in agreement that we do not think of a good solution
offhand to the real cause of the issue, except by encouraging the use of
throw-away review branches, perhaps.

> I think the following decribes what often happens for many users:
>
>  1. User A pushes the branch to a public repo.
>  2. User B points out a simple mistake in the branch.

That's the CVS workflow, and it is not "a" public repo but "the" public
repo shared between A and B (and also with all the project participants).

>  3. User A makes a fix
>  4. User A squashes the fix into the (already-published) history.
>  5. User A attempts to push the "fixed" history (but is rejected by
> the public repo because of non-fast-forward).
> At this point, the damage is already done,...

Which is probably a sufficient safety which the user can learn from.  If
this happens too often, that probably means we are not helping them enough
to learn not to "commit --amend" or "rebase" if they are using Git as a
better CVS.

> You could say that User A should be more careful and push to a "less
> public" repo in step #1 (thus allowing the fix to be squashed before
> pushing to a "more public" repo in step #5),...

That is essentially a workflow that uses throw-away review branches in a
distributed environment, and at that point, we are not constrained by the
limitation of the CVS workflow. While still in early review cycles (which
corresponds to being in our 'pu'), "commit --amend" and "rebase" are fine
tool to be used.  And...

> but how "public" is
> "public" enough to have someone point out the bug, but still
> "unpublic" enough to allow rebase?

... I can imagine that currently that is determined purely by project
convention. Perhaps there needs a way to mark throw-away review branches
like 'pu' (or saying the same thing from the different perspective, to
mark cast-in-stone integration branches like 'next') so that tools can
mechanically decide what should and should not be rewritten.

To extend the idea of promoting throw-away review branches further,
perhaps it might help if there is an easy way to let the users publish
their "master" to a branch that is not the "master" of the central shared
repository even in the CVS workflow (e.g. by default a push from user A
always goes to refs/review/A/master), and to have an option to "git push"
that makes it go to the "master" when the user really means the branch is
ready (and it would move refs/review/A/master to attic to be later gc'ed).

> ... And I
> think that refusing rewrites of commits that are already present in
> the @{upstream} remote-tracking branch is good enough to help most
> users avoid steps #4 through #6 (in a push-based workflow[1]).

See above regarding branches that should not be rebased even if they are
not directly pushed out.

> In
> fact, from a pedagogical POV, I think step #4 is probably the best
> spot for novice users to learn exactly the distinction between
> acceptable and unacceptable history rewrites (instead of having it
> explained to them as a consequence of the step #5).

I doubt you have enough information at point #4, unless you restrict the
workflow you allow your novice users to use fairly severely, to give
appropriate advice.  While I agree with you that it would be the best if
we could do so at step #4 without stopping the user from doing what s/he
needs to do with false positive, I think it is not pedagogical POV but
dreaming if the world were ideal without knowing what it would take to
make it ideal.

At least I don't know offhand what kind of changes are needed to restrict
the user actions to an "approved" workflow so that step #4 can make a
useful decision (that is, no false positives and small enough false
negatives).

^ permalink raw reply

* Patchset NTLM-Authentication
From: Schmidt, Marco @ 2012-02-21  8:07 UTC (permalink / raw)
  To: git; +Cc: gitster, avarab

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

Support NTLM-Authentication against proxy.

I have include a minimal patch set to support NTLM authentification
against a proxy server. 

The patch adds a boolean configuration variable "http.proxyauthany" to
enable CURLOPT_PROXYAUTH = CURLAUTH_ANY.

I'm not conform with your patch submitting policy. I had no chance to
track the git repository from my current location.


Marco Schmidt

[-- Attachment #2: 0002-Einf-hren-der-HTTP-Proxy-Authentifizierung.patch --]
[-- Type: application/octet-stream, Size: 1553 bytes --]

From 8a3892eca69a0c603a53b3d3aa052f45c4a5b648 Mon Sep 17 00:00:00 2001
From: Marco Schmidt <Marco.Schmidt@cassidian.com>
Date: Fri, 27 Jan 2012 13:08:44 +0100
Subject: [PATCH 2/4] =?UTF-8?q?Einf=FChren=20der=20HTTP=20Proxy=20Authentifi?=
 =?UTF-8?q?zierung?=
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

---
 http.c |   11 +++++++++++
 1 files changed, 11 insertions(+), 0 deletions(-)

diff --git a/http.c b/http.c
index f3b2c90..dc51d41 100644
--- a/http.c
+++ b/http.c
@@ -41,6 +41,7 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+static int curl_http_proxyauthany = 0;
 static const char *curl_cookie_file;
 static char *user_name, *user_pass, *description;
 static const char *user_agent;
@@ -222,6 +223,10 @@ static int http_options(const char *var, const char *value, void *cb)
 	}
 	if (!strcmp("http.proxy", var))
 		return git_config_string(&curl_http_proxy, var, value);
+	
+	if (!strcmp("http.proxyauthany", var))  {
+		curl_http_proxyauthany = git_config_bool(var, value);
+	}
 
 	if (!strcmp("http.cookiefile", var))
 		return git_config_string(&curl_cookie_file, var, value);
@@ -330,6 +335,12 @@ static CURL *get_curl_handle(void)
 
 	if (curl_http_proxy)
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+	
+	if (curl_http_proxyauthany) {
+		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+		printf("CURLOPT_PROXYAUTH!!!!");
+	}
+
 
 	return result;
 }
-- 
1.7.8


[-- Attachment #3: 0003-First-working-step.patch --]
[-- Type: application/octet-stream, Size: 1519 bytes --]

From 2b7336c4959fc4df30da9e68f652f2c5941f0c91 Mon Sep 17 00:00:00 2001
From: Marco Schmidt <Marco.Schmidt@cassidian.com>
Date: Fri, 27 Jan 2012 14:35:15 +0100
Subject: [PATCH 3/4] First working step

---
 http.c |    9 ++++++++-
 1 files changed, 8 insertions(+), 1 deletions(-)

diff --git a/http.c b/http.c
index dc51d41..3dbd390 100644
--- a/http.c
+++ b/http.c
@@ -173,6 +173,8 @@ static int git_config_path(const char **result,
 
 static int http_options(const char *var, const char *value, void *cb)
 {
+	fprintf(stderr, "http_options: %s = %s\n", var, value);
+	
 	if (!strcmp("http.sslverify", var)) {
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
@@ -226,6 +228,8 @@ static int http_options(const char *var, const char *value, void *cb)
 	
 	if (!strcmp("http.proxyauthany", var))  {
 		curl_http_proxyauthany = git_config_bool(var, value);
+		fprintf(stderr, "http_options: curl_http_proxyauthany = %i", curl_http_proxyauthany);
+		return 0;
 	}
 
 	if (!strcmp("http.cookiefile", var))
@@ -336,9 +340,12 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy)
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	
+	fprintf(stderr, "DEBUG: curl_http_proxyauthany\n");
+	
+	/* http proxy could be set from enviroment - set CURLOPT_PROXYAUTH */
 	if (curl_http_proxyauthany) {
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-		printf("CURLOPT_PROXYAUTH!!!!");
+		fprintf(stderr, "curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n");
 	}
 
 
-- 
1.7.8


[-- Attachment #4: 0004-remove-debug-information.patch --]
[-- Type: application/octet-stream, Size: 1512 bytes --]

From 55b76b5f658904d240db29bb0d59f8c1dcedd4b5 Mon Sep 17 00:00:00 2001
From: Marco Schmidt <Marco.Schmidt@cassidian.com>
Date: Fri, 27 Jan 2012 14:40:57 +0100
Subject: [PATCH 4/4] remove debug information

---
 http.c |    8 ++------
 1 files changed, 2 insertions(+), 6 deletions(-)

diff --git a/http.c b/http.c
index 3dbd390..fbc52f0 100644
--- a/http.c
+++ b/http.c
@@ -173,8 +173,6 @@ static int git_config_path(const char **result,
 
 static int http_options(const char *var, const char *value, void *cb)
 {
-	fprintf(stderr, "http_options: %s = %s\n", var, value);
-	
 	if (!strcmp("http.sslverify", var)) {
 		curl_ssl_verify = git_config_bool(var, value);
 		return 0;
@@ -228,7 +226,6 @@ static int http_options(const char *var, const char *value, void *cb)
 	
 	if (!strcmp("http.proxyauthany", var))  {
 		curl_http_proxyauthany = git_config_bool(var, value);
-		fprintf(stderr, "http_options: curl_http_proxyauthany = %i", curl_http_proxyauthany);
 		return 0;
 	}
 
@@ -340,13 +337,12 @@ static CURL *get_curl_handle(void)
 	if (curl_http_proxy)
 		curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
 	
-	fprintf(stderr, "DEBUG: curl_http_proxyauthany\n");
-	
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY	
 	/* http proxy could be set from enviroment - set CURLOPT_PROXYAUTH */
 	if (curl_http_proxyauthany) {
 		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
-		fprintf(stderr, "curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);\n");
 	}
+#endif
 
 
 	return result;
-- 
1.7.8


^ permalink raw reply related

* Re: git-subtree Ready #2
From: Junio C Hamano @ 2012-02-21  8:44 UTC (permalink / raw)
  To: David A. Greene; +Cc: Jeff King, git, Avery Pennarun
In-Reply-To: <7vwr7gitjl.fsf@alter.siamese.dyndns.org>

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

> greened@obbligato.org (David A. Greene) writes:
>
>> Ok, but we will preserve the history via the subtree merge, yes?
>
> I'll comment on just this part, but a short answer is "no, I do not think
> so".
> ...
> I was saying that the history up to the current state, littered with these
> commits that are not "logical progression" but merely "a snapshot of
> then-current state" may not be worth preserving, with or without better
> messages.
>
> Rewriting the entire history to make it a logical progression just for the
> sake of history is obviously not worth the effort.

Having said all that, my preference is not so strong to out-right veto
doing a true merge; I wouldn't lose sleep if we end up merging the tip of
your subtree branch with all the history behind it as-is.

BUT.

Even though I freely admit that I was the guilty one who came up with
"merge -s subtree", and Linus's "gitk merge" was the original sin, having
a subtree merge like gitk, git-gui and gitweb in the history is not
without downsides.

The most problematic one that we regularly suffer from is that the
commands in the log family cannot work well across a subtree merge with
pathspec limiting, e.g. "git log git-gui/po", and we have to resort to
something like:

    $ cd git-gui/po &&
      git rev-list --parents HEAD . |
      while read commit parent
      do
        git log --pretty=short $parent..$commit^2 -- :/po
      done | git shortlog -n -e

to achieve what should be a simple "git shortlog -n -e git-gui/po".  I
suspect that a subtree merge may also lead bisection into uninteresting
tangents as it joins otherwise disjoint history.

If we still have an active upstream that grows its history in a separate
repository, like gitk and git-gui do, we cannot avoid a subtree merge in
order to continue merging from them.  Because you seem to be taking over
and are going to maintain it as part of git.git proper, eventually aiming
to move it out of contrib/, it's just that I do not think it is worth the
trouble.

^ permalink raw reply

* HTTP error 511 [Was: Secure (https) proxy authentification]
From: Nicolas Mailhot @ 2012-02-21  8:58 UTC (permalink / raw)
  To: Nicolas Mailhot; +Cc: squid3, ietf-http-wg, Jeff King, git, Daniel Stenberg
In-Reply-To: <4ec05cf797322715a960743aeec0a48b.squirrel@arekh.dyndns.org>


Le Dim 19 février 2012 11:22, Nicolas Mailhot a écrit :

> 511 is exactly what I need. I was not aware of it. Is it simplemented in any
> browser yet? Where should I point the browser writers to get it implemented?
>
> http://tools.ietf.org/id/draft-nottingham-http-new-status-04.txt ?

I take that back. 511 is almost exactly what we need. However, when I pointed
the authors of some of the tools that pass through our proxy to it (curl, git)
they told me they could not parse html code in their tools, so they really
need a location (or similar) field containing the address of the
authentication portal to communicate it to the user. Without this field, they
can only stop with 'Network authentication is needed' instead of 'Please open
<url> in your browser to proceed'.

http://article.gmane.org/gmane.comp.version-control.git/191085
http://article.gmane.org/gmane.comp.version-control.git/191087
http://article.gmane.org/gmane.comp.version-control.git/191086

(the nearest thing there is in the spec is the url in meta, but it's only in
the example, not mandatory, and no one will write code for something they can
not be sure will exist)

We'd like to support those tools properly as their users' previous clumsy
attempts to navigate our current non-standard redirection method resulted in
internal security investigations.

It is a problem in our setup as we only block some URLs (others are allowed
transparently without auth), and we use several proxy farms in different
physical sites (to avoid spofs). So just opening any url in a browser won't
trigger an authentication request (the url may not be blocked, or the browser
may pass through a gateway where the user IP is already authorized, while
git/etc tried to access through another one).

Could you please revise the error 511 definition to add such a field ?

Regards,

-- 
Nicolas Mailhot

^ permalink raw reply

* Re* [PATCH 0/5] diff --ignore-case
From: Junio C Hamano @ 2012-02-21  9:02 UTC (permalink / raw)
  To: Thomas Rast; +Cc: Johannes Sixt, git, Chris Leong
In-Reply-To: <7vvcn1l21d.fsf@alter.siamese.dyndns.org>

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

> Thomas Rast <trast@inf.ethz.ch> writes:
>
>> I wonder which one of us misunderstood the original request ;-)
>
> Heh, I did ;-)
>
>> It was
>>
>> } Is there any way to run diff -G with a case insensitivity flag?
>>
>> and I took that to mean "I want to find addition/removal of a string
>> like -G does, but I don't know how it was capitalized".
>
> I think it is just the matter of checking REG_ICASE that may be set in
> revs->grep_filter.regflags, and propagating it down to the regcomp at the
> beginning of diffcore_pickaxe_grep().
>
> Want to try and see how well it works?

The gist of the patch should look like this.

Even though I haven't done anything more than just to compile and run "git
log -p -i -G search 233054d", it looks obviously correct ;-)

Note that this does not depend on any of the "diff --ignore-case" topic,
and it may conflict with [PATCH 6/5].  Among the two "else if" that flips
the REG_ICASE in revision.c, it needs to be decided if only one of them,
or both of them should set the new option. I haven't thought things through
to decide which.

 diff.h             |    1 +
 diffcore-pickaxe.c |    6 +++++-
 revision.c         |    1 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/diff.h b/diff.h
index 7af5f1e..182cb0e 100644
--- a/diff.h
+++ b/diff.h
@@ -82,6 +82,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
 #define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)
 #define DIFF_OPT_DIRSTAT_BY_LINE     (1 << 28)
 #define DIFF_OPT_FUNCCONTEXT         (1 << 29)
+#define DIFF_OPT_PICKAXE_IGNORE_CASE (1 << 30)
 
 #define DIFF_OPT_TST(opts, flag)    ((opts)->flags & DIFF_OPT_##flag)
 #define DIFF_OPT_SET(opts, flag)    ((opts)->flags |= DIFF_OPT_##flag)
diff --git a/diffcore-pickaxe.c b/diffcore-pickaxe.c
index 380a837..8ffb741 100644
--- a/diffcore-pickaxe.c
+++ b/diffcore-pickaxe.c
@@ -138,8 +138,12 @@ static void diffcore_pickaxe_grep(struct diff_options *o)
 {
 	int err;
 	regex_t regex;
+	int cflags = REG_EXTENDED | REG_NEWLINE;
 
-	err = regcomp(&regex, o->pickaxe, REG_EXTENDED | REG_NEWLINE);
+	if (DIFF_OPT_TST(o, PICKAXE_IGNORE_CASE))
+		cflags |= REG_ICASE;
+
+	err = regcomp(&regex, o->pickaxe, cflags);
 	if (err) {
 		char errbuf[1024];
 		regerror(err, &regex, errbuf, 1024);
diff --git a/revision.c b/revision.c
index 819ff01..b3554ed 100644
--- a/revision.c
+++ b/revision.c
@@ -1582,6 +1582,7 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
 		revs->grep_filter.regflags |= REG_EXTENDED;
 	} else if (!strcmp(arg, "--regexp-ignore-case") || !strcmp(arg, "-i")) {
 		revs->grep_filter.regflags |= REG_ICASE;
+		DIFF_OPT_SET(&revs->diffopt, PICKAXE_IGNORE_CASE);
 	} else if (!strcmp(arg, "--fixed-strings") || !strcmp(arg, "-F")) {
 		revs->grep_filter.fixed = 1;
 	} else if (!strcmp(arg, "--all-match")) {

^ permalink raw reply related

* Re: git-subtree Ready #2
From: Thomas Rast @ 2012-02-21  9:07 UTC (permalink / raw)
  To: David A. Greene; +Cc: Junio C Hamano, Jeff King, git, Avery Pennarun
In-Reply-To: <87ehtowxu7.fsf@smith.obbligato.org>

greened@obbligato.org (David A. Greene) writes:

>> -GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>> +
>> +if test -z "$GIT_BUILD_DIR"
>> +then
>> +    echo Here
>> +	# We allow tests to override this, in case they want to run tests
>> +	# outside of t/, e.g. for running tests on the test library
>> +	# itself.
>> +        GIT_BUILD_DIR="$TEST_DIRECTORY"/..
>> +fi
>
> I'll put a patch together with a more extensive explanation.  Basically,
> tests run outside of the top-level t/ directory don't work because there
> are all sort of assumptions in test-lib.sh about where they live.  There
> are comments in test-lib.sh indicating that it should support tests in
> other directories but I could not make it work out of the box.

Note that this will conflict with tr/perftest, which is already in next.
It had a similar override, but then made do with the existing
GIT_TEST_INSTALLED facility.  I think you do need the above here,
because you are not sourcing test-lib from the directory where the test
lives.  It may then be cleaner to again use GIT_BUILD_DIR in
t/perf/perf-lib.sh since it does not require knowledge (outside of
test-lib) that $GIT_BUILD_DIR/bin-wrappers/ holds the binaries.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: Patchset NTLM-Authentication
From: Thomas Rast @ 2012-02-21  9:22 UTC (permalink / raw)
  To: Schmidt, Marco; +Cc: git, gitster, avarab
In-Reply-To: <4CDEC141B5583D408E79F2931DB7708301802B70@GSX300A.mxchg.m.corp>

"Schmidt, Marco" <Marco.Schmidt@cassidian.com> writes:

> I'm not conform with your patch submitting policy. I had no chance to
> track the git repository from my current location.

You should read Documentation/SubmittingPatches.  If your location does
not let you use any git:// or git+http:// ("smart http") transports, you
can use the wereHamster's bundler service https://bundler.caurea.org/ to
get a bundle that you can download over http.  (If you cannot download
*anything* over http, what is your internet access good for?)

Looking at the attachments:

[0002-Einf-hren-der-HTTP-Proxy-Authentifizierung.patch]
[0003-First-working-step.patch]
[0004-remove-debug-information.patch]

* You seem to have forgotten the first patch.  As it stands, the 0002
  one does not apply to my tree.

* The commit messages are useless.  The second one at least says
  "introduce HTTP proxy authentication" which tells me something about
  *what it does*, but in German.  It should also tell me why and how and
  possible other considerations.  See SubmittingPatches.

* The commits should "spring into perfect existence".  That is, in the
  above you should squash 3 and 4 into 2 so that 2 starts out working
  (presumably -- I'm only looking at the subjects) .  You can use rebase
  -i to clean everything up.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Git tree object storing policy
From: Ivan Tolstosheyev @ 2012-02-21  9:22 UTC (permalink / raw)
  To: git

Hello,

now tree object is a simple list of <attributes, hash, name>sorted by name
(tricky sorted, cause we assuming that directory name "$X" is actually "$X/"
in comparison function). The problem is, that if I want to insert 10k files
in empty git repository on / folder there will be 10k new 
trees with sizes from (1 to 10k)*(hash+name+attribute)+eps  .

itroot@localhost ~/tmp> cat git-test.sh 
#!/usr/bin/env bash

git init test
cd test
for i in `seq 1 10000` 
do
touch ${i} ; git add ${i} ; git commit -m "Add ${i}" ;
done
cd ..
du -hs test
itroot@localhost ~/tmp>

itroot@localhost ~/tmp> ./git-test.sh
...
180M	test
itroot@localhost ~/tmp>

180 MB!!!?? and 7.4M after `git gc` - thanks to delta compression!

Ok, you can say that this example is artificial, and I can add 10k files
with 1 commit. Thats true. But manipulating files in big tree objects
(in a big directories) is storage-expensive, and if I need to store a 
lot of files in one directory and frequently change them - git just
don't scales now properly at this use-case.

What do I propose? 
We can add another git object, named for example "btree" , 
that contains another "btree" objects or files.  This will be a simple
btree structure (tree entries sorted practically by name, BTW,
maybe it's time to fix sorting =] ), that allows us to do insertion,
removal, search in ln(n) time. But - we do not have troubles 
with big direcories now. BTW, if all directories are small, btree
will be tree-like - just btree pointing to  files.
So, one big tree with 10k files transforms to (hmm, for example...)
101 btrees - one, pointing to 100 btrees, and thay points to files.
(100 entries per btree is a wild guess =) )

Suggestions?

 

^ permalink raw reply

* Re: HTTP error 511 [Was: Secure (https) proxy authentification]
From: Julian Reschke @ 2012-02-21  9:28 UTC (permalink / raw)
  To: Nicolas Mailhot; +Cc: squid3, ietf-http-wg, Jeff King, git, Daniel Stenberg
In-Reply-To: <39d91a07ae0beb19a734e52496ab5700.squirrel@arekh.dyndns.org>

On 2012-02-21 09:58, Nicolas Mailhot wrote:
>
> Le Dim 19 février 2012 11:22, Nicolas Mailhot a écrit :
>
>> 511 is exactly what I need. I was not aware of it. Is it simplemented in any
>> browser yet? Where should I point the browser writers to get it implemented?
>>
>> http://tools.ietf.org/id/draft-nottingham-http-new-status-04.txt ?
>
> I take that back. 511 is almost exactly what we need. However, when I pointed
> the authors of some of the tools that pass through our proxy to it (curl, git)
> they told me they could not parse html code in their tools, so they really
> need a location (or similar) field containing the address of the
> authentication portal to communicate it to the user. Without this field, they
> can only stop with 'Network authentication is needed' instead of 'Please open
> <url>  in your browser to proceed'.

Yes. The definition of status code 511 did not attempt to solve more 
problems than that.

> http://article.gmane.org/gmane.comp.version-control.git/191085
> http://article.gmane.org/gmane.comp.version-control.git/191087
> http://article.gmane.org/gmane.comp.version-control.git/191086
>
> (the nearest thing there is in the spec is the url in meta, but it's only in
> the example, not mandatory, and no one will write code for something they can
> not be sure will exist)
>
> We'd like to support those tools properly as their users' previous clumsy
> attempts to navigate our current non-standard redirection method resulted in
> internal security investigations.
>
> It is a problem in our setup as we only block some URLs (others are allowed
> transparently without auth), and we use several proxy farms in different
> physical sites (to avoid spofs). So just opening any url in a browser won't
> trigger an authentication request (the url may not be blocked, or the browser
> may pass through a gateway where the user IP is already authorized, while
> git/etc tried to access through another one).
>
> Could you please revise the error 511 definition to add such a field ?

The specification has already been approved, so it's too late to make 
more than editorial changes.

Best regards, Julian

^ permalink raw reply

* Re: HTTP error 511 [Was: Secure (https) proxy authentification]
From: Nicolas Mailhot @ 2012-02-21  9:44 UTC (permalink / raw)
  To: Julian Reschke; +Cc: squid3, ietf-http-wg, Jeff King, git, Daniel Stenberg, git
In-Reply-To: <4F4363C1.90902@gmx.de>


Le Mar 21 février 2012 10:28, Julian Reschke a écrit :
> On 2012-02-21 09:58, Nicolas Mailhot wrote:

>> Could you please revise the error 511 definition to add such a field ?
>
> The specification has already been approved, so it's too late to make
> more than editorial changes.

I know it has been approved, and we'll go with it as is for now on our network.
However, could it be extended to add this location field later please? (taking
into account the tool authors' feedback)

Best regards,

-- 
Nicolas Mailhot

^ permalink raw reply

* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-21 10:05 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: git, Michael J Gruber, pclouds, j.sixt
In-Reply-To: <7vr4xois3l.fsf@alter.siamese.dyndns.org>

On 02/21/2012 08:05 AM, Junio C Hamano wrote:
> Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl>  writes:
>
>> On 02/21/2012 12:41 AM, Junio C Hamano wrote:
>>> Zbigniew Jędrzejewski-Szmek<zbyszek@in.waw.pl>   writes:
>>>
>>>> JC:
>>>>> Perhaps the maximum for garph_width should be raised to something like
>>>>> "min(80, stat_width) - name_width"?
>>>> I think that a graph like
>>>> a | 1000 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>>> b |    1 -
>>>> is not very readable. I like the consistency forced by the 40-column limit.
>>>> But I guess that this is very subjective.
>>>
>>> The above makes it very obvious that there is a huge amount of change made
>>> to 'a' and a bit of deletion to 'b', compared to a mini-graph that is
>>> truncated to half the screen width.
>> Yes. But the same graph with 40 columns tells me exactly the same thing.
>
> That is a bogus argument, isn't it?  You can say the same thing if you
> limited the length of the graph bars to 10-columns if you only compare
> between 1000 and 1. You can even do with just 5-columns.  For that matter,
> without any graph bar at all, it tells us exactly the same thing because
> we have numbers.  Does that mean we do not need any bar?  Of course not.
> We use bars as visual aid.
Yes.

> Imagine what happens to the graph if you had paths with medium amount of
> changes like 980, 800, 40, in addition to 1000 and 1.  By limiting the
> length of the bars more than necessary, you are losing the resolution
> without a good reason, and that is why I find 40-column limit a poor
> design choice.
You're right.

>>> Besides, the above is what you would get without your patch on 80-column
>>> terminal, no?
>>
>> Yes.
>
> I think this "use at most 40-places for the graph bar" was your response
> to somebody's observation that "on 200-column terminal, we will still see
> the commit log messages (and for many projects, also patch text) that are
> designed to be comfortably viewable within the 80-column on the left, and
> overlong graph bar stands out like an ugly sore thumb".
>
> While that "ugliness" observation might be a valid one to make, I do not
> think limiting the length of the graph bar without taking the length of
> the name part into account at all is the right solution to it.
>
> After all, that is exactly the same thinking that led to the bug in the
> current code that you fixed with your series, isn't it?  Our safety code
> truncated the graph bar width too early without taking the width needed to
> show the names into account, and then when the names turn out to be all
> short, we ended up wasting space on the right hand side, because we made
> the bars too short and the decision was made too early in the code.
>
> If the problem you are addressing is to make sure that the diffstat part
> in the series of lines that are structured like this:
>
>     log message part ~80 column
>     diff stat part that can extend very very very very very very very long
>     patch text part  ~80 column
>
> does not become overly long, wouldn't it be a more natural solution to
> make sure that when the total (i.e. name and graph) length can fit to
> align with the message and patch (i.e. traditional ~80 col regardless of
> the terminal width), not to give it too much width?  If the names are
> short, like "a" and "b", that may result in graph bar part to use ~70
> columns or so, and if the names are long, like in a Java project, you may
> allocate 50 columns to the name and leave only 50 columns or so for the
> graph part.
>
> A simple heuristic might be to see if name part (without truncation) and
> the graph part (without scaling) fits under 100-columns if the terminal is
> wider than that, and if so limit the whole thing to 100-columns before
> deciding the allocation of the total into two parts.  If the name part
> alone is very wide, showing the name and the graph using the whole
> terminal width would give you a better result than using the bars that are
> artificially capped to a short limit, I would imagine.
This seem overly complex. A nice property to have would be
"if the window is wide enough so there's enough space for full 
filenames, the graph part scales monotonically with the change count".
(If there's filename truncation, than there just isn't enough space for 
everything and the graph may be compressed. But otherwise, if we have 
two graphs which do not end at the edge of the screen, and the second 
one is wider than the first one, then without looking at the change 
counts we know that the second one has more changes).

For this property to be satisfied, the graph_width limit would have to 
be independent of the filename width. So maybe it should be
   71 = (available space if stat_width==80 and the filename is "a" and
         the change count is in double digits).
Then if the filenames are longer, and the change counts are big enough,
the graph part starts gently extending above 80 columns.

What do you think about this approach?

Zbyszek

^ permalink raw reply

* Re: Git tree object storing policy
From: Thomas Rast @ 2012-02-21 10:18 UTC (permalink / raw)
  To: Ivan Tolstosheyev; +Cc: git
In-Reply-To: <loom.20120221T094746-680@post.gmane.org>

Ivan Tolstosheyev <ivan.tolstosheyev@gmail.com> writes:

> #!/usr/bin/env bash
>
> git init test
> cd test
> for i in `seq 1 10000` 
> do
> touch ${i} ; git add ${i} ; git commit -m "Add ${i}" ;
> done
> cd ..
> du -hs test
[...]
> 180 MB!!!?? and 7.4M after `git gc` - thanks to delta compression!

Most of those 180MB are waste from mostly unused 4KB (presumably) blocks
of your filesystem.  You should be looking at the post-gc'd numbers.

Let's see the breakdown of 'du -h .git':

0       .git/rr-cache
1.5M    .git/logs/refs/heads
1.5M    .git/logs/refs
2.9M    .git/logs
4.0K    .git/objects/info
2.8M    .git/objects/pack
2.8M    .git/objects
0       .git/branches
12K     .git/info
0       .git/remotes
88K     .git/hooks
0       .git/refs/tags
0       .git/refs/heads
0       .git/refs
6.5M    .git

So 2.9MB are git keeping a reflog of everything we did (on HEAD and on
master).  Since merely storing a SHA1 for each of your 10000 operations
already takes 200K, that's not so far off -- the factor of 10 is in the
email, date and log message.

In my case 704K went into the index (not directly visible above, it's
the bulk of the top level).  That's also not unreasonable: merely
storing the object SHA1 (20 bytes) and a bunch of timestamps for 10000
files also gets you into the 500K ballpark.

The pack index amazingly takes only about 500K, even though it is
indexing 10000 trees and 10000 commits, so again the SHA1s alone get you
into the 400K ballpark.

That leaves only 2.3MB for the actual pack (which contains all the
data!).  But every commit must store a tree and a parent, so there are
at least 2*10000*20 = 400K uncompressable bytes in the commits
already[*].  So we are within a factor of 6 of just the data required to
save the shape of your history DAG, no content included.  I'd say that's
not too bad.


[*] This is not quite true, the parents and trees might be pointers
within the pack.  AFAIK the proposed pack v4 format does this, and would
yield a more efficient compression.  So if you're going to waste energy
worrying about this, you should help with pack v4.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: git-svn show-externals and svn version
From: Nikolaus Demmel @ 2012-02-21 11:14 UTC (permalink / raw)
  To: git
In-Reply-To: <E59CCE45-6F92-4748-9B6E-2A562647904B@nikolaus-demmel.de>

Hi,

as a followup just another example of when the current show-externals gives a flaky output, namely when the line in the external definition is commented.


$ git svn show-externals
[...]
# /src/
/src/#https://codex.cs.bham.ac.uk/svn/nah/cogx/code/subarchitectures/vision.sa/tags/matlab-cosy-2008     matlab_cosy_2008
/src/#https://codex.cs.bham.ac.uk/svn/nah/cogx/code/subarchitectures/vision.sa/tags/matlab-review-2009     matlab_review_2009


Regards,
Nikolaus


Am 19.02.2012 um 19:53 schrieb Nikolaus Demmel:

> Hi,
> 
> I am currently investigating getting support for svn externals in git-svn (you might have noticed my other mails).
> 
> It turns out that there are quite a few scripts floating around that use the output of show-externals and then try to pull these externals with git-svn into independent repositories and add the folders as submodules to the root repository.
> 
> However, none of them work for me, and the primary reason AFAICT is that they were written for the pre svn1.5 format of svn:externals. From 1.5 svn supports a new format of svn:externals, which changes the order of revision, repository-url, and local folder, and also adds the posibility to add relative urls, peg-revisions, etc [1].
> 
> On top that it seems to me that the output of show-externals was purely designed for the old format. For example, if you compare the output of "git svn show-externals" with "git svn propget svn:externals" in an example repository using the new format [2], you find that the in the show-externals output the prepended "/" and "/instantiations/" at the beginning of each line does not make sense. If the target url (all relative with the ^ syntax in this case) and the sub-folder were swapped in order, as of pre 1.5 svn, it would make much more sense. Also apparently the code for show-externals was added in 2007 and not changed since, whereas svn 1.5 was released in 2008.
> 
> What I am not completely clear about is, whether svn 1.5 and later enforces the new syntax, or whether it just adds the new syntax and still has to support the old syntax (which could be distinguished, I guess, by checking of the last part on an entry is an absolute URL instead of a subfolder). Also, I'm not sure if the format depends on the version of the svn-server or the client. I would assume you can check out a repository hosted with svn 1.4 with a 1.5 client. Does the client process the svn:externals and present it in the new format, or is this the text string just taken from the server unaltered (I have not much knowledge of how svn actually works internally)?
> 
> Another question is whether the perl svn bindings present the svn:externals in some parsed, standard format, or do they just give you the raw text string?
> 
> In order to make show-externals more useful with the svn 1.5 and later syntax, one would maybe need to check the underlying svn version. I guess it is also quite important to retain backwards compatibility, such that users of externals with the old syntax would still get the same output as before.
> 
> I would suggest that the show-externals output should be as close as possible to the svn:externals syntax, possibly adapting the subfolder path for nested folders. However here the recursive display of externals for subfolders becomes a bit more tricky, since the URL can also be relative to the subfolder as of the new syntax. Maybe the easiest way to deal with the new syntax in show-externals would be to have each line like it is in the svn-properties, but add a space separated relative path to the corresponding subfolder at the beginning. A tool that uses this is then responsible for making sure the relative URLs are resolved correctly.
> 
> To sum up, given that all the questions I have are answered like I think is most likely, it would boil down to changing the output of show-externals for svn 1.5  and later just slightly, namely by inserting an additional space between the prepended subfolder and the actual svn:externals definition in each line.
> 
> Any thoughts and/or answers?
> 
> Cheers,
> Nikolaus
> 
> 
> [1] http://svnbook.red-bean.com/en/1.7/svn.advanced.externals.html
> [2] http://paste.lisp.org/display/127858--
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

^ permalink raw reply

* Re: [PATCHv3] git-p4: add initial support for RCS keywords
From: Pete Wyckoff @ 2012-02-21 12:18 UTC (permalink / raw)
  To: Luke Diamand; +Cc: git, Eric Scouten
In-Reply-To: <1329258835-17223-2-git-send-email-luke@diamand.org>

luke@diamand.org wrote on Tue, 14 Feb 2012 22:33 +0000:
> diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
[..]
> +git-p4.attemptRCSCleanup:
> +    If enabled, 'git p4 submit' will attempt to sort cleanup RCS keywords
> +    ($Header$, etc). These would otherwise cause merge conflicts and prevent
> +    the submit going ahead. This option should be considered experimental at
> +    present.

=> "attempt to cleanup"

> diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
[..]
> +#
> +# return the raw p4 type of a file (text, text+ko, etc)
> +#
> +def p4_type(file):
> +    files = p4_read_pipe_lines(["files", file])
> +    info = files[0]
> +    m = re.search(r'\(([a-z0-9A-Z+]+)\)\s*$', info)
> +    if m:
> +        ret = m.group(1)
> +        if verbose:
> +            print "%s => %s" % (file, ret)
> +        return ret
> +    else:
> +        die("Could not extract file type from '%s'" % info)

p4 fstat -T headType
gives just the type, no need to parse the info string

> +#
> +# Given a type base and modifier, return a regexp matching
> +# the keywords that can be expanded in the file
> +#
> +def p4_keywords_regexp_for_type(base, type_mods):
> +    if base in ("text", "unicode", "binary"):
> +        if "ko" in type_mods:
> +            return r'\$(Id|Header)[^$]*\$'
> +        elif "k" in type_mods:
> +            return r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$'

Still no ":"?  Won't that match too much?

> +    def patchRCSKeywords(self, file, pattern):

Nice, clean.

>      def p4UserForCommit(self,id):
[..]
> +                for line in read_pipe_lines(diffcmd):
> +                    # read diff lines: for each file reported, if it can have
> +                    # keywords expanded, and the diff contains keywords, then
> +                    # try zapping the p4 file.
> +                    m = re.match(r'^diff --git a/(.*)\s+b/(.*)', line)

I'm still sort of iffy on this.  You have editedFiles, and could
just use that directly.  Grepping through the diff won't give you
any more information.

> +                    if pattern:
> +                        print line

Debugging?  Maybe add a leading comment saying why this line.

> @@ -1585,15 +1685,11 @@ class P4Sync(Command, P4UserMap):
>  
>          # Note that we do not try to de-mangle keywords on utf16 files,
>          # even though in theory somebody may want that.
> -        if type_base in ("text", "unicode", "binary"):
> -            if "ko" in type_mods:
> -                text = ''.join(contents)
> -                text = re.sub(r'\$(Id|Header):[^$]*\$', r'$\1$', text)
> -                contents = [ text ]
> -            elif "k" in type_mods:
> -                text = ''.join(contents)
> -                text = re.sub(r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision):[^$]*\$', r'$\1$', text)
> -                contents = [ text ]
> +        pattern = p4_keywords_regexp_for_type(type_base, type_mods)
> +        if pattern:
> +            text = ''.join(contents)
> +            text = re.sub(pattern, r'$\1$', text)
> +            contents = [ text ]

Nice.  Glad you refactored this.

Fix the colon thing at least, then happy to add my Acked-By.

Sorry for the delay.

		-- Pete

^ permalink raw reply

* cvs2git failed in pass2 ...
From: supadhyay @ 2012-02-21 12:52 UTC (permalink / raw)
  To: git

Hi All,

I am using cvs2svn  tool to convert my CVS repository to GIT. I have
download the "cvs2svn-2.3.0.tar.gz" file.

I have created git user to install GIT and then using the same user (git) I
try to run "./cvs2git" command  below way.

./cvs2git \  --blobfile=/home/git/tmp/git-files/git-blob.dat \ 
--dumpfile=/home/git/tmp/git-files/git-dump.dat \    --username=git   
/opt/source/Repo_from_CVS/VD

It took around 10 mins to finish pass1 and for pass 2 it raised the warnings
and messages like below 
==============================================================
Done

Time for pass1 (CollectRevsPass): 526.3 seconds.
----- pass 2 (CleanMetadataPass) -----
Converting metadata to UTF8...
WARNING: Problem decoding log message:
---------------------------------------------------------------------------
version 4  - Note concerning “Leave One” option for Internal Duplicate Check
added
 
---------------------------------------------------------------------------
WARNING: Problem decoding log message:
---------------------------------------------------------------------------
......
......
......
---------------------------------------------------------------------------
WARNING: Problem decoding log message:
---------------------------------------------------------------------------
all labels to be right-justified
all labels to end with “:”
removed Selected Error Codes label
‘Select One’ dropdowns are horizontally aligned with corresponding text
boxes
 
---------------------------------------------------------------------------
ERROR: There were warnings converting author names and/or log messages
to Unicode (see messages above).  Please restart this pass
with one or more '--encoding' parameters or with
'--fallback-encoding'.

***********************************************************************************
can any one suggest me how to overcome this warning or any workaround ??

Thanks in advance...




--
View this message in context: http://git.661346.n2.nabble.com/cvs2git-failed-in-pass2-tp7304605p7304605.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply

* Re: Patchset NTLM-Authentication
From: Thomas Rast @ 2012-02-21 13:28 UTC (permalink / raw)
  To: Schmidt, Marco; +Cc: git, gitster, avarab
In-Reply-To: <4CDEC141B5583D408E79F2931DB7708301802BE7@GSX300A.mxchg.m.corp>

"Schmidt, Marco" <Marco.Schmidt@cassidian.com> writes:

> Thanks for your reply. I have used the wereHamster bundler service and
> created the following patch. May I ask you to test the patch and check
> again for more style violations?

Please keep these discussions public, so that others can learn and
(heaven forbid...) point out _my_ mistakes.

I'll discuss the file you attached below.  Keep in mind that unless
there is a compelling reason not to do that, you should send your
patches inline.  This allows for much easier review on the list.

> Subject: [PATCH] Allow NTLM-Authentication against a http-proxy server - Add
>  config option "http.proxyauthany" - Set CURLOPT_PROXYAUTH
>  to CURLAUTH_ANY if option curl_http_proxyauthany is true.
> 
> ---

You can already see that you did not adhere to the standard format of a
commit message: one line summary, followed by a blank line, followed by
a long description.  So the above might have been written (dropping the
Subject: pseudoheader tag now):

} Allow NTLM-Authentication against a http-proxy server
} 
} - Add config option "http.proxyauthany"
} 
} - Set CURLOPT_PROXYAUTH to CURLAUTH_ANY if option
}   curl_http_proxyauthany is true.

Now bear in mind that I have no clue about curl (except that it
downloads stuff ;-) and especially about its authentication parts.  But
allow me to sketch a different commit message that highlights what I
would like to see answered.  I am merely using the knowledge I could
glean from 2min of googling around; I am not saying that anything of it
is correct, so you should fix it as needed!

  http/curl: let user configure "any" proxy authentication

  Normally, curl uses only the "basic" authentication scheme when
  talking to proxies, which may not be desirable (it sends the password
  in cleartext) or sufficient (the author needs NTLM authentication for
  his proxy).

  Introduce the config setting http.proxyAuthAny.  When enabled, we tell
  curl to use any authentication scheme supported by the proxy.

  This mostly parallels http.authAny which was introduced in b8ac923
  (Add an option for using any HTTP authentication scheme, not only
  basic, 2009-11-27).  http.authAny was removed, and its feature
  unconditionally enabled, in 525ecd2 (Remove http.authAny, 2009-12-28).
  However the reasoning of the latter does not apply here because XXXX.


Some notes on the code/patch: There was some trailing whitespace, please
make sure you remove it.

Usually we introduce a setting (command line option or environment
variable) that can be tweaked to override the configuration.  b8ac923
did this if you need some inspiration :-)  Otherwise you should make an
argument why this is not needed.

 +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +	if (curl_http_proxyauthany) {
> +		curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +	}
> +#endif

My google is failing me as to when this feature was introduced or how
common curls without it still are.  But you could be extra doubly nice
to the user and warn if s/he enabled the feature, but git cannot adhere
to the request because curl doesn't support it.  (Then again b8ac923
didn't go that far either.)

-- 
Thomas Rast
trast@{inf,student}.ethz.ch

^ permalink raw reply

* Re: cvs2git failed in pass2 ...
From: supadhyay @ 2012-02-21 14:19 UTC (permalink / raw)
  To: git
In-Reply-To: <1329828722466-7304605.post@n2.nabble.com>

Hi All,

Can any one know how to overcome this error... I tried with --encoding=UTF8
but the same error.

I also gave a try with --fallback-encoding but seems like its exclude
comments from their output as shown below:

Time for pass2 (CleanMetadataPass): 0.590 seconds.
 
cvs2svn Statistics:
------------------
Total CVS Files:              9025
Total CVS Revisions:         39243
Total CVS Branches:          76789
Total CVS Tags:            1276127
Total Unique Tags:             480
Total Unique Branches:          32
CVS Repos Size in KB:       872465
First Revision Date:    Mon Aug  2 15:07:28 2004
Last Revision Date:     Fri Feb 17 13:10:27 2012
------------------
*(These are unaltered CVS repository stats and do not
 reflect tags or branches excluded via --exclude)
Timings (seconds):*
------------------
530.3   pass1    CollectRevsPass
  0.6   pass2    CleanMetadataPass
530.9   total

--
View this message in context: http://git.661346.n2.nabble.com/cvs2git-failed-in-pass2-tp7304605p7304832.html
Sent from the git mailing list archive at Nabble.com.

^ permalink raw reply

* [PATCH 1/4] t3900: add missing UTF-16.txt and mark the test successful
From: Nguyễn Thái Ngọc Duy @ 2012-02-21 14:24 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy


Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t3900-i18n-commit.sh |    4 ++--
 t/t3900/UTF-16.txt     |  Bin 0 -> 18 bytes
 2 files changed, 2 insertions(+), 2 deletions(-)
 create mode 100644 t/t3900/UTF-16.txt

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index d48a7c0..a9e5662 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -34,9 +34,9 @@ test_expect_success 'no encoding header for base case' '
 	test z = "z$E"
 '
 
-test_expect_failure 'UTF-16 refused because of NULs' '
+test_expect_success 'UTF-16 refused because of NULs' '
 	echo UTF-16 >F &&
-	git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
+	test_must_fail git commit -a -F "$TEST_DIRECTORY"/t3900/UTF-16.txt
 '
 
 
diff --git a/t/t3900/UTF-16.txt b/t/t3900/UTF-16.txt
new file mode 100644
index 0000000000000000000000000000000000000000..8d0945b8e0a734ced8948da29ed9f8c65e3ec775
GIT binary patch
literal 18
VcmezW&xIi$409P$8B!Ry7yv#b1kV5f

literal 0
HcmV?d00001

-- 
1.7.8.36.g69ee2

^ permalink raw reply related

* [PATCH 2/4] Do attempt pretty print in ASCII-incompatible encodings
From: Nguyễn Thái Ngọc Duy @ 2012-02-21 14:24 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1329834292-2511-1-git-send-email-pclouds@gmail.com>

We rely on ASCII everywhere. We print "\n" directly without conversion
for example. The end result would be a mix of some encoding and ASCII
if they are incompatible. Do not do that.

In theory we could convert everything to utf-8 as intermediate medium,
process process process, then convert final output to the desired
encoding. But that's a lot of work (unless we have a pager-like
converter) with little real use. Users can just pipe everything to
iconv instead.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 It seems half of the encodings "iconv -l" list does not pass
 ascii_superset_encoding() test. I just assume they are either exotic
 or duplicate names.

 pretty.c |    7 +++++++
 utf8.c   |   15 +++++++++++++++
 utf8.h   |    1 +
 3 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/pretty.c b/pretty.c
index 8688b8f..5c433a2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -493,12 +493,19 @@ char *logmsg_reencode(const struct commit *commit,
 		      const char *output_encoding)
 {
 	static const char *utf8 = "UTF-8";
+	static const char *last_output_encoding = NULL;
 	const char *use_encoding;
 	char *encoding;
 	char *out;
 
 	if (!*output_encoding)
 		return NULL;
+	if (last_output_encoding != output_encoding) {
+		if (!ascii_superset_encoding(output_encoding))
+			die("encoding %s is not a superset of ASCII.",
+			    output_encoding);
+		last_output_encoding = output_encoding;
+	}
 	encoding = get_header(commit, "encoding");
 	use_encoding = encoding ? encoding : utf8;
 	if (!strcmp(use_encoding, output_encoding))
diff --git a/utf8.c b/utf8.c
index 8acbc66..def93ee 100644
--- a/utf8.c
+++ b/utf8.c
@@ -482,3 +482,18 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 	return out;
 }
 #endif
+
+int ascii_superset_encoding(const char *encoding)
+{
+	const char *sample = " !\"#$%&'()*+,-./0123456789:;<=>?@"
+		"ABCDEFGHIJKLMNOPQRSTUVWXYZ[\\]^_`"
+		"abcdefghijklmnopqrstuvwxyz{|}~\n";
+	char *output;
+	int ret;
+	if (!encoding)
+		return 1;
+	output = reencode_string(sample, encoding, "US-ASCII");
+	ret = !output || !strcmp(sample, output);
+	free(output);
+	return ret;
+}
diff --git a/utf8.h b/utf8.h
index 81f2c82..75bc128 100644
--- a/utf8.h
+++ b/utf8.h
@@ -12,6 +12,7 @@ int strbuf_add_wrapped_text(struct strbuf *buf,
 		const char *text, int indent, int indent2, int width);
 int strbuf_add_wrapped_bytes(struct strbuf *buf, const char *data, int len,
 			     int indent, int indent2, int width);
+int ascii_superset_encoding(const char *encoding);
 
 #ifndef NO_ICONV
 char *reencode_string(const char *in, const char *out_encoding, const char *in_encoding);
-- 
1.7.8.36.g69ee2

^ permalink raw reply related

* [PATCH 3/4] utf8: die if failed to re-encoding
From: Nguyễn Thái Ngọc Duy @ 2012-02-21 14:24 UTC (permalink / raw)
  To: git; +Cc: Nguyễn Thái Ngọc Duy
In-Reply-To: <1329834292-2511-1-git-send-email-pclouds@gmail.com>

Return value NULL in this case means "no conversion needed", which is
not quite true when conv == -1.

Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
 t/t4201-shortlog.sh |    2 +-
 utf8.c              |    2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh
index 6872ba1..d445665 100755
--- a/t/t4201-shortlog.sh
+++ b/t/t4201-shortlog.sh
@@ -27,7 +27,7 @@ test_expect_success 'setup' '
 		tr 1234 "\360\235\204\236")" a1 &&
 
 	# now fsck up the utf8
-	git config i18n.commitencoding non-utf-8 &&
+	git config i18n.commitencoding viscii &&
 	echo 4 >a1 &&
 	git commit --quiet -m "$(
 		echo "This is a very, very long first line for the commit message to see if it is wrapped correctly" |
diff --git a/utf8.c b/utf8.c
index def93ee..f918e9e 100644
--- a/utf8.c
+++ b/utf8.c
@@ -444,7 +444,7 @@ char *reencode_string(const char *in, const char *out_encoding, const char *in_e
 		return NULL;
 	conv = iconv_open(out_encoding, in_encoding);
 	if (conv == (iconv_t) -1)
-		return NULL;
+		die("failed to convert from %s to %s", in_encoding, out_encoding);
 	insz = strlen(in);
 	outsz = insz;
 	outalloc = outsz + 1; /* for terminating NUL */
-- 
1.7.8.36.g69ee2

^ permalink raw reply related


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