* [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
* [PATCH 4/4] Only re-encode certain parts in commit object, not the whole
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>
Commit object has its own format, which happens to be in ascii, but
not really subject to re-encoding.
There are only four areas that may be re-encoded: author line,
committer line, mergetag lines and commit body. Encoding of tags
embedded in mergetag lines is not decided by commit encoding, so leave
it out and consider it binary.
Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail.com>
---
pretty.c | 58 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
1 files changed, 57 insertions(+), 1 deletions(-)
diff --git a/pretty.c b/pretty.c
index 5c433a2..6ccc091 100644
--- a/pretty.c
+++ b/pretty.c
@@ -489,6 +489,62 @@ static char *replace_encoding_header(char *buf, const char *encoding)
return strbuf_detach(&tmp, NULL);
}
+/*
+ * Re-encode author, committer and commit body only, leaving the rest
+ * in ascii (or whatever the encoding it is in mergetag lines)
+ * regardless output encoding. We assume the commit is good, so no
+ * validation.
+ */
+static char *reencode_commit(const char *buffer,
+ const char *out_enc, const char *in_enc)
+{
+ struct strbuf out = STRBUF_INIT;
+ struct strbuf buf = STRBUF_INIT;
+ char *reencoded, *s, *e;
+
+ strbuf_addstr(&buf, buffer);
+
+ s = strstr(buf.buf, "\nauthor ");
+ assert(s != NULL);
+ s += 8; /* "\nauthor " */
+ strbuf_add(&out, buf.buf, s - buf.buf);
+ e = strchr(s, '\n');
+ *e = '\0';
+ reencoded = reencode_string(s, out_enc, in_enc);
+ if (reencoded && strchr(reencoded, '\n'))
+ die("your chosen encoding produces \\n out of nowhere?");
+ strbuf_addstr(&out, reencoded ? reencoded : s);
+ free(reencoded);
+
+ strbuf_addstr(&out, "\ncommitter ");
+ assert(!strncmp(e + 1, "committer ", 10));
+ s = e + 11; /* "\ncommitter " */
+ e = strchr(s, '\n');
+ *e = '\0';
+ reencoded = reencode_string(s, out_enc, in_enc);
+ if (reencoded && strchr(reencoded, '\n'))
+ die("your chosen encoding produces \\n out of nowhere?");
+ strbuf_addstr(&out, reencoded ? reencoded : s);
+ free(reencoded);
+ *e = '\n';
+
+ s = e;
+ e = strstr(s, "\n\n");
+ if (e) {
+ e += 2; /* "\n\n" */
+ strbuf_add(&out, s, e - s);
+
+ s = e;
+ reencoded = reencode_string(s, out_enc, in_enc);
+ strbuf_addstr(&out, reencoded ? reencoded : s);
+ free(reencoded);
+ } else
+ strbuf_addstr(&out, s);
+
+ strbuf_release(&buf);
+ return strbuf_detach(&out, NULL);
+}
+
char *logmsg_reencode(const struct commit *commit,
const char *output_encoding)
{
@@ -514,7 +570,7 @@ char *logmsg_reencode(const struct commit *commit,
else
return NULL; /* nothing to do */
else
- out = reencode_string(commit->buffer,
+ out = reencode_commit(commit->buffer,
output_encoding, use_encoding);
if (out)
out = replace_encoding_header(out, output_encoding);
--
1.7.8.36.g69ee2
^ permalink raw reply related
* Allow NTLM-Authentication against a http-proxy server
From: Schmidt, Marco @ 2012-02-21 14:31 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: text/plain, Size: 265 bytes --]
After some back and forward - here is my patch to support
NTLM-authentication against a http-proxy server again.
Big thanks are going to Thomas Rast for helping to fiddle the patch to a
more correct form and his commit message ;).
Marco Schmidt
[-- Attachment #2: 0001-http-curl-let-user-configure-any-proxy-authenticatio.patch --]
[-- Type: application/octet-stream, Size: 1835 bytes --]
From f4e112e41aef75f08532fda53a1b9021208a3774 Mon Sep 17 00:00:00 2001
From: Marco Schmidt <Marco.Schmidt@cassidian.com>
Date: Tue, 21 Feb 2012 12:30:29 +0100
Subject: [PATCH] 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
---
http.c | 12 ++++++++++++
1 files changed, 12 insertions(+), 0 deletions(-)
diff --git a/http.c b/http.c
index 0ffd79c..5c55efd 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 struct credential http_auth = CREDENTIAL_INIT;
static int http_proactive_auth;
@@ -190,6 +191,11 @@ 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);
+ return 0;
+ }
if (!strcmp("http.cookiefile", var))
return git_config_string(&curl_cookie_file, var, value);
@@ -297,6 +303,12 @@ static CURL *get_curl_handle(void)
if (curl_http_proxy)
curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+ if (curl_http_proxyauthany) {
+ curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
+ }
+#endif
return result;
}
--
1.7.7.1.msysgit.0
^ permalink raw reply related
* Re: git status: small difference between stating whole repository and small subdirectory
From: Nguyen Thai Ngoc Duy @ 2012-02-21 14:45 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <20120220143644.GA13938@do>
On Mon, Feb 20, 2012 at 9:36 PM, Nguyen Thai Ngoc Duy <pclouds@gmail.com> wrote:
> On Mon, Feb 20, 2012 at 03:09:57PM +0100, Thomas Rast wrote:
>> > Interestingly, on my git.git repo, I had an empty cache. Running "git
>> > read-tree HEAD" filled it (according to test-dump-cache-tree). It seems
>> > that running "git checkout" empties the cache. So perhaps git could do
>> > better about keeping the cache valid over time.
>>
>> test_expect_failure 'checkout gives cache-tree' '
>> git checkout HEAD^ &&
>> test_shallow_cache_tree
>> '
>>
>> ;-)
>
> Quick and dirty that passes that test.
I'm aware that Jeff's tackling at lower level, which retains
cache-tree for many more cases. But this patch seems simple and safe
to me, and in my experience this case happens quite often (or maybe I
tend to keep my index clean). Junio, any chance this patch may get in?
> -- 8< --
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 5bf96ba..c06287a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -319,6 +319,10 @@ static void show_local_changes(struct object *head, struct diff_options *opts)
> die(_("diff_setup_done failed"));
> add_pending_object(&rev, head, NULL);
> run_diff_index(&rev, 0);
> + if (!DIFF_OPT_TST(&rev.diffopt, HAS_CHANGES)) {
> + struct tree *tree = parse_tree_indirect(head->sha1);
> + prime_cache_tree(&active_cache_tree, tree);
> + }
> }
>
> static void describe_detached_head(const char *msg, struct commit *commit)
> @@ -493,13 +497,13 @@ static int merge_working_tree(struct checkout_opts *opts,
> }
> }
>
> + if (!opts->force && !opts->quiet)
> + show_local_changes(&new->commit->object, &opts->diff_options);
> +
> if (write_cache(newfd, active_cache, active_nr) ||
> commit_locked_index(lock_file))
> die(_("unable to write new index file"));
>
> - if (!opts->force && !opts->quiet)
> - show_local_changes(&new->commit->object, &opts->diff_options);
> -
> return 0;
> }
>
> -- 8< --
> --
> Duy
--
Duy
^ permalink raw reply
* Re: [PATCH 2/4] Do attempt pretty print in ASCII-incompatible encodings
From: Nguyen Thai Ngoc Duy @ 2012-02-21 14:53 UTC (permalink / raw)
To: git
In-Reply-To: <1329834292-2511-2-git-send-email-pclouds@gmail.com>
2012/2/21 Nguyễn Thái Ngọc Duy <pclouds@gmail.com>:
> @@ -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;
> +}
Side note about this function, which was written to ban all
ascii-incompatible charsets from entering commit objects. The idea of
mixing charsets in the same buffer without clear boundary does not
sound healthy. Plus, ident.c will silently drop '\n', '<' and '>' in
author/committer. If a hypothetical charset happens to place a letter
in those, um.. code points?, the letter will be dropped. But meh..
--
Duy
^ permalink raw reply
* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Nguyen Thai Ngoc Duy @ 2012-02-21 15:16 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, gitster, Michael J Gruber, j.sixt
In-Reply-To: <1329775034-21551-1-git-send-email-zbyszek@in.waw.pl>
Not related to changes in the series, but on the same topic. Have you
considered grouping pathnames to reduce pressure on truncating them?
Instead of showing
a/b/c | ++--
a/b/d | --++
you could show
a/b:
c | ++--
d | --++
Optimal grouping strategy could be hard, but at least we could use it
for the case "git diff --stat -- one/path/deep/in/here"
--
Duy
^ permalink raw reply
* Re: cvs2git failed in pass2 ...
From: supadhyay @ 2012-02-21 15:17 UTC (permalink / raw)
To: git
In-Reply-To: <1329828722466-7304605.post@n2.nabble.com>
Hi All,
I am able to run cvs2git command successfully by adding --encoding=ascii
--encoding=utf8 --encoding=utf16 --encoding=latin
Thanks...
--
View this message in context: http://git.661346.n2.nabble.com/cvs2git-failed-in-pass2-tp7304605p7305031.html
Sent from the git mailing list archive at Nabble.com.
^ permalink raw reply
* Re: [RFC/PATCH 0/3] push: add 'prune' option
From: Nguyen Thai Ngoc Duy @ 2012-02-21 15:30 UTC (permalink / raw)
To: Felipe Contreras; +Cc: git, Junio C Hamano, Jeff King
In-Reply-To: <1329505957-24595-1-git-send-email-felipe.contreras@gmail.com>
On Sat, Feb 18, 2012 at 2:12 AM, Felipe Contreras
<felipe.contreras@gmail.com> wrote:
> As an example I put my use-case; I want to backup *all* my local branches to a
> personal repository, and I want to remove branches that I have removed from my
> local repository. git push personal 'refs/heads/*' mostly does the job, but it
> doesn't remove anything, and that's where 'prune' comes from.
Yeah, may I have "fetch --prune" too, please? Looking at diffstat
gives me a feeling that you only need to add maybe four lines to
builtin/fetch.c and my dream would come true.
--
Duy
^ permalink raw reply
* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Zbigniew Jędrzejewski-Szmek @ 2012-02-21 16:11 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: git, gitster, Michael J Gruber, j.sixt
In-Reply-To: <CACsJy8AXH6xm-ShH_HF6RwATFwsYtAZQfcWoB9VpaiCMj6e8vQ@mail.gmail.com>
On 02/21/2012 04:16 PM, Nguyen Thai Ngoc Duy wrote:
> Not related to changes in the series, but on the same topic. Have you
> considered grouping pathnames to reduce pressure on truncating them?
> Instead of showing
>
> a/b/c | ++--
> a/b/d | --++
>
> you could show
>
> a/b:
> c | ++--
> d | --++
>
> Optimal grouping strategy could be hard, but at least we could use it
> for the case "git diff --stat -- one/path/deep/in/here"
I use (or would like to be able to use) the --stat output to select with
the mouse and paste into something like
emacsclient -n <pathname>
This would be harder with the grouping, because I'd have to select two
parts and paste two times and type a slash. So for me this would be a minus.
Also, I'm not sure about grouping if there's more than one group. The
graph wouldn't be as readable. But if there's only one group, i.e. if
all filenames have a common prefix ending in a slash, like in your
example, this could be useful.
Zbyszek
^ permalink raw reply
* Re: [PATCHv3] git-p4: add initial support for RCS keywords
From: Eric Scouten @ 2012-02-21 16:54 UTC (permalink / raw)
To: Pete Wyckoff; +Cc: Luke Diamand, git
In-Reply-To: <20120221121834.GB18317@padd.com>
[Resent in plain-text. Apologies for dupe content to Pete and Luke.]
On Tue, Feb 21, 2012 at 04:18, Pete Wyckoff <pw@padd.com> wrote:
> luke@diamand.org wrote on Tue, 14 Feb 2012 22:33 +0000:
> > diff --git a/contrib/fast-import/git-p4 b/contrib/fast-import/git-p4
> > +#
> > +# 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?
> Fix the colon thing at least, then happy to add my Acked-By.
No, that would be an incorrect change. The colon is added by P4 when
it expands the keyword pattern, but it is *not* part of the pattern
required by P4 to trigger a keyword expansion.
http://kb.perforce.com/article/54/using-rcs-keywords
-Eric
--
Eric Scouten :: software developer, photographer :: Poulsbo, WA (near Seattle)
http://ericscouten.com :: click for Flickr, Facebook, Twitter, LinkedIn links
^ permalink raw reply
* Re: [PATCHv3] git-p4: add initial support for RCS keywords
From: Junio C Hamano @ 2012-02-21 17:25 UTC (permalink / raw)
To: Eric Scouten; +Cc: Pete Wyckoff, Luke Diamand, git
In-Reply-To: <CAEe=O8qui8PryuZiZNDwLk39+tKVDnh+5eP9m_WrHi=K9ekMNQ@mail.gmail.com>
Eric Scouten <eric@scouten.com> writes:
>> > r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$'
>>
>> Still no ":"? Won't that match too much?
>
>> Fix the colon thing at least, then happy to add my Acked-By.
>
> No, that would be an incorrect change. The colon is added by P4 when
> it expands the keyword pattern, but it is *not* part of the pattern
> required by P4 to trigger a keyword expansion.
>
> http://kb.perforce.com/article/54/using-rcs-keywords
I have this suspicion that both Pete and your last sentence is correct,
but the regexp in the patch and your "would be an incorrect change" are
wrong.
I am not a P4 expert, but I would be very surprised if P4 expands "$Ida$"
as if it is "$Id$" or "$Id: old expansion$", which the regexp would match.
Wouldn't it be more like this?
\$ # begins with a dollar, followed by...
( Id | Header | ... ) # one of these keywords, followed by ...
( :[^$]+ )? # possibly an old expansion, followed by
\$ # another dollar sign
^ permalink raw reply
* Re: [RFC/PATCH 0/3] push: add 'prune' option
From: Jeff King @ 2012-02-21 17:35 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Felipe Contreras, git, Junio C Hamano
In-Reply-To: <CACsJy8ACoF=2dEUoFvT8bQDmUsjJA2_VWvTSJV6fakqyCLBCYg@mail.gmail.com>
On Tue, Feb 21, 2012 at 10:30:31PM +0700, Nguyen Thai Ngoc Duy wrote:
> On Sat, Feb 18, 2012 at 2:12 AM, Felipe Contreras
> <felipe.contreras@gmail.com> wrote:
> > As an example I put my use-case; I want to backup *all* my local branches to a
> > personal repository, and I want to remove branches that I have removed from my
> > local repository. git push personal 'refs/heads/*' mostly does the job, but it
> > doesn't remove anything, and that's where 'prune' comes from.
>
> Yeah, may I have "fetch --prune" too, please? Looking at diffstat
> gives me a feeling that you only need to add maybe four lines to
> builtin/fetch.c and my dream would come true.
Huh? Don't we already have "fetch --prune"?
-Peff
^ permalink raw reply
* Re: [PATCH 3/4] utf8: die if failed to re-encoding
From: Junio C Hamano @ 2012-02-21 17:36 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329834292-2511-3-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> Return value NULL in this case means "no conversion needed", which is
> not quite true when conv == -1.
Doing this only when producing new commits to avoid spreading damage might
be a good idea.
But utf8.c::reencode_string() is sufficiently deep in the call-chains to
make me suspect that the codepaths this change affects are not limited to
creation ones. If this also forbids readers from resurrecting salvageable
bits while reading (imagine your commit had "encodign vscii" but your log
message was in English, except only your name had letters outside ASCII
that I cannot locally convert to utf-8 for viewing), I do not think it is
an acceptable change.
^ permalink raw reply
* Re: [PATCHv3] git-p4: add initial support for RCS keywords
From: Eric Scouten @ 2012-02-21 17:59 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Pete Wyckoff, Luke Diamand, git
In-Reply-To: <7vobssgkt6.fsf@alter.siamese.dyndns.org>
On Tue, Feb 21, 2012 at 09:25, Junio C Hamano <gitster@pobox.com> wrote:
> Eric Scouten <eric@scouten.com> writes:
>
>>> > r'\$(Id|Header|Author|Date|DateTime|Change|File|Revision)[^$]*\$'
>>>
>>> Still no ":"? Won't that match too much?
>>
>>> Fix the colon thing at least, then happy to add my Acked-By.
>>
>> No, that would be an incorrect change. The colon is added by P4 when
>> it expands the keyword pattern, but it is *not* part of the pattern
>> required by P4 to trigger a keyword expansion.
>>
>> http://kb.perforce.com/article/54/using-rcs-keywords
>
> I have this suspicion that both Pete and your last sentence is correct,
> but the regexp in the patch and your "would be an incorrect change" are
> wrong.
>
> I am not a P4 expert, but I would be very surprised if P4 expands "$Ida$"
> as if it is "$Id$" or "$Id: old expansion$", which the regexp would match.
>
> Wouldn't it be more like this?
>
> \$ # begins with a dollar, followed by...
> ( Id | Header | ... ) # one of these keywords, followed by ...
> ( :[^$]+ )? # possibly an old expansion, followed by
> \$ # another dollar sign
Good catch. Yes, you're probably right.
--
Eric Scouten :: software developer, photographer :: Poulsbo, WA (near Seattle)
http://ericscouten.com :: click for Flickr, Facebook, Twitter, LinkedIn links
^ permalink raw reply
* Re: Patchset NTLM-Authentication
From: Junio C Hamano @ 2012-02-21 18:02 UTC (permalink / raw)
To: Schmidt, Marco, Thomas Rast; +Cc: git, avarab
In-Reply-To: <8762f05n9q.fsf_-_@thomas.inf.ethz.ch>
Thomas Rast <trast@inf.ethz.ch> writes:
> 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.
Thanks, Thomas.
I think this paragraph is essential, especially the XXXX part, if we were
to accept the proposed change and keep the new configuration. Otherwise
we won't know what to do when somebody proposes to unconditionally enable
this ;-)
If it turns out that we can set CURLOPT_PROXYAUTH always to CURLAUTH_ANY
without compromising security, then an explanation why this does not have
to be optional, similar to what justified 525ecd2, needs to be there
instead, and the patch needs to be tweaked to drop the configuration bits.
Marco, I extracted your patch in the attachment and took a look at it
before composing the above response.
- Your log message seems to be indented by two spaces for some strange
reason;
- it does not have any justification like the example Thomas gave
you; and
- it also is missing your S-o-b.
Care to re-roll one more time?
^ permalink raw reply
* Re: [PATCH 2/4] Do attempt pretty print in ASCII-incompatible encodings
From: Jeff King @ 2012-02-21 18:21 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329834292-2511-2-git-send-email-pclouds@gmail.com>
On Tue, Feb 21, 2012 at 09:24:50PM +0700, Nguyen Thai Ngoc Duy wrote:
> 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.
I'm not sure why we bother checking this. Using non-ASCII-superset
encodings is broken, yes, but are people actually doing that? I assume
that the common one is utf-16, and anybody using it will experience
severe breakage immediately. So are people actually doing this? Are
there actually encodings that will cause subtle breakage that we want to
catch?
-Peff
^ permalink raw reply
* Re: [PATCH 4/4] Only re-encode certain parts in commit object, not the whole
From: Jeff King @ 2012-02-21 18:25 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329834292-2511-4-git-send-email-pclouds@gmail.com>
On Tue, Feb 21, 2012 at 09:24:52PM +0700, Nguyen Thai Ngoc Duy wrote:
> Commit object has its own format, which happens to be in ascii, but
> not really subject to re-encoding.
>
> There are only four areas that may be re-encoded: author line,
> committer line, mergetag lines and commit body. Encoding of tags
> embedded in mergetag lines is not decided by commit encoding, so leave
> it out and consider it binary.
Is this worth the effort? Yes, re-encoding the ASCII bits of the commit
object is unnecessary. But do we actually handle encodings that are not
ASCII supersets? IOW, I could see the point if this is making it
possible to hold utf-16 names and messages in your commits (though why
you would want to do so is beyond me...). But my understanding is that
this is horribly broken anyway by other parts of the code. And even
looking at your code below:
> +static char *reencode_commit(const char *buffer,
> + const char *out_enc, const char *in_enc)
> +{
> + struct strbuf out = STRBUF_INIT;
> + struct strbuf buf = STRBUF_INIT;
> + char *reencoded, *s, *e;
> +
> + strbuf_addstr(&buf, buffer);
> +
> + s = strstr(buf.buf, "\nauthor ");
> + assert(s != NULL);
Wouldn't this assert trigger in the presence of encodings which
contain ASCII NUL (e.g., wide encodings like utf-16)?
Is there an encoding you have in mind which would be helped by this?
-Peff
^ permalink raw reply
* Re: Patchset NTLM-Authentication
From: Daniel Stenberg @ 2012-02-21 19:02 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Schmidt, Marco, Thomas Rast, git, avarab
In-Reply-To: <7vfwe4gj4n.fsf@alter.siamese.dyndns.org>
On Tue, 21 Feb 2012, Junio C Hamano wrote:
> If it turns out that we can set CURLOPT_PROXYAUTH always to CURLAUTH_ANY
> without compromising security, then an explanation why this does not have to
> be optional, similar to what justified 525ecd2, needs to be there instead,
> and the patch needs to be tweaked to drop the configuration bits.
Allow me to provide some libcurl info on this!
Setting it to ANY will unconditionally cause an extra roundtrip which you can
avoid if you know what auth type the proxy wants and you set it at once. With
ANY set, libcurl will first "probe" the proxy to figure out which type to use
and then go on and actually do it in a second request (and possibly even a
third request in some cases).
It can actually be seen as a security _improvement_ in some cases where for
example Basic auth (user+password sent as plain text) can be avoided in
preference to a more secure mechanism, but I think that's a rather rare case
for git.
IMO, if ANY is considered fine for normal host authentication I think it could
be considered fine for proxy authentication as well.
--
/ daniel.haxx.se
^ permalink raw reply
* Re: git status: small difference between stating whole repository and small subdirectory
From: Junio C Hamano @ 2012-02-21 19:16 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy
Cc: Thomas Rast, Jeff King, Piotr Krukowiecki, Git Mailing List
In-Reply-To: <CACsJy8DE86qzA1=GiKZFRCt5aH8X4iMyDvfrhnqwmbq52szhHg@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> I'm aware that Jeff's tackling at lower level, which retains
> cache-tree for many more cases.
>
> But this patch seems simple and safe
> to me, and in my experience this case happens quite often (or maybe I
> tend to keep my index clean). Junio, any chance this patch may get in?
I do not think we are talking about a duplicated effort here.
By definition, the change to hook into unpack_trees() and making sure we
invalidate all the necessary subtrees in the cache cannot give you a cache
tree that is more populated than what you started with. And the train of
thought in Peff's message is to improve this invalidation---we currently
invalidate everything ;-)
Somebody has to populate the cache tree fully when we _know_ the index
matches a certain tree, and adding a call to prime_cache_tree() in
strategic places is a way to do so. The most obvious is write-tree, but
there are a few other existing codepaths that do so.
Because prime_cache_tree() by itself is a fairly expensive operation that
reads all the trees recursively, its benefits need to be evaluated. It
should to happen only in an operation that is already heavy-weight, is
likely to have read all the trees and have many of them in-core cache, and
also relatively rarely happens compared to "git add" so that the cost can
be amortised over time, such as "reset --(hard|mixed)".
Switching branches is likely to fall into that category, but that is just
my gut feeling. I would feel better at night if somebody did a benchmark
;-)
One thing we do not currently do anywhere that _might_ be of merit is to
make a call to cache_tree_update() instead of prime_cache_tree() when we
already know that only a very small subpart of the cache-tree is invalid
and it is cheaper to repair it by rehashing only a small portion of the
index than to re-prime the entire cache tree with prime_cache_tree().
^ permalink raw reply
* Re: [PATCH] Ignore SIGPIPE when running a filter driver
From: Johannes Sixt @ 2012-02-21 19:20 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jehan Bing, git, peff, jrnieder
In-Reply-To: <7vsji5jgtv.fsf@alter.siamese.dyndns.org>
Am 20.02.2012 23:11, schrieb Junio C Hamano:
> Jehan Bing <jehan@orb.com> writes:
>> @@ -360,12 +361,16 @@ static int filter_buffer(int in, int out, void *data)
>> if (start_command(&child_process))
>> return error("cannot fork to run external filter %s", params->cmd);
>>
>> + sigchain_push(SIGPIPE, SIG_IGN);
>> +
>> write_err = (write_in_full(child_process.in, params->src, params->size) < 0);
>> if (close(child_process.in))
>> write_err = 1;
>> if (write_err)
>> error("cannot feed the input to external filter %s", params->cmd);
>>
>> + sigchain_pop(SIGPIPE);
>> +
>
> Thanks.
>
> I think this is OK on a POSIX system where this function is run by
> start_async() which is implemented with a forked child process.
>
> I do not now if it poses a issue on Windows, though. Johannes, any
> comments?
I do not expect the change to cause a problem on Windows.
-- Hannes
^ permalink raw reply
* Re: [PATCH 0/8 v6] diff --stat: use the full terminal width
From: Junio C Hamano @ 2012-02-21 20:10 UTC (permalink / raw)
To: Zbigniew Jędrzejewski-Szmek; +Cc: git, Michael J Gruber, pclouds, j.sixt
In-Reply-To: <4F436C5D.7070606@in.waw.pl>
Zbigniew Jędrzejewski-Szmek <zbyszek@in.waw.pl> writes:
> 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 ...
Sorry, the desired property I would understand, but that does not click
with your "have to be independent" conclusion, so I do not have comment on
the "maybe it should be..." part.
The resolution requirement may want to set a "desired lower limit" for the
width of the graph, but it is only "desired" because it is possible that
you have to bust the limit if you have three files with 1, 9999 and 10000
changed lines and your terminal is only 200 columns wide.
The current code caps name part to 50/80, but allows the graph to use more
when you have only shorter names. Perhaps you can follow the same logic
in the first part of your [7/8] (which needs to be separated to at least
in two pieces, as it conflates the "lift 50-column cap from the name width
and make it proportional to the term_width()" part and "but cap the graph
part to 40-column" part, that are separate topics)? Then we can try
different heuristics to find a better way to cap the length of the graph
on top?
^ permalink raw reply
* Re: [PATCH] completion: Allow dash as the first character for __git_ps1
From: Junio C Hamano @ 2012-02-21 20:40 UTC (permalink / raw)
To: Christian Hammerl; +Cc: git
In-Reply-To: <1329740273-5580-1-git-send-email-info@christian-hammerl.de>
Christian Hammerl <info@christian-hammerl.de> writes:
> If the argument for `__git_ps1` begins with a dash, `printf` tries to
> interpret it as an option which results in an error message.
> The problem is solved by adding '--' before the argument to tell
> `printf` to not interpret the following argument as an option.
> Adding '--' directly to the argument does not help because the argument
> is enclosed by double quotes.
>
> Signed-off-by: Christian Hammerl <info@christian-hammerl.de>
> ---
> contrib/completion/git-completion.bash | 2 +-
> 1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
> index 91c7acb..61ff152 100755
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -313,7 +313,7 @@ __git_ps1 ()
> fi
>
> local f="$w$i$s$u"
> - printf "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
> + printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
> fi
> }
Hmm, so you may be doing something like PS1='\W$(__git_ps1 "-%s")'?
OK, and thanks.
^ permalink raw reply
* how do you review auto-resolved files
From: Neal Kreitzinger @ 2012-02-21 20:41 UTC (permalink / raw)
To: git
When git does a merges (merge/rebase/cherry-pick) it auto-resolves same-file
changes that do not conflict on the same line(s).
Technical Question: What are the recommended commands for reviewing the
files that auto-resolved after a "merge"?
It seems like the commands might be different depending on the type of
merge: git-merge, git-rebase, git-cherry-pick. I imagine there are three
steps to the "review auto-resolutions" procedure:
(1) Determine the list of files that were changed on both sides (same-file
edits) and which of those were auto-resolved during the merge. (Preferably
excluding those files that merge-conflicted since you already know how you
manually resolved those.)
(2) Review the auto-resolved files in full context to verify whether the
auto-resolutions are desirable.
(3) Manually remediate the merge-result (auto-resolution) or redo the
merge-of-that-file for any files with undesirable auto-resolutions. Perhaps
an edit of the auto-resolved file is sufficient for simple remediations, but
for more challenging remediations a manual redo of the merge-of-that-file
would be desired.
Please advise on the proven (tried and tested) ways that others are using to
verify/ensure that their auto-resolve results are correct.
Procedural/Philosophical Question: What are the pros and cons of
auto-resolved files?
Currently, we address the problem up-front instead of after-the-fact by
enforcing merge-conflicts on every same-file edit by means of a
"user-date-stamp" on "line 1" of every source file changed by performing
keyword expansion (# $User$ $Date$) in our pre-commit hook. I don't think
keyword expansion or forcing merge-conflicts for every same-file edit is a
common practice among git users. Therefore, this seems like somewhat of a
kludgey hack. Furthermore, I assume that all git users are somehow
reviewing their auto-resolutions. (There is no way I would assume that git
merged my same-file edits correctly. It's great that git
does-the-right-thing most-of-the-time, but that doesn't change the fact that
I still have to review everything for undesirable resolutions.)
In light of this, it seems that there is no advantage to letting git
auto-resolve same-file changes because the review process after-the-fact
would actually be more error-prone and tedious than just manually-merging
same-file edits up-front. If I force you to resolve merge-conflicts
up-front then I'm ensuring the merge-resolution is deliberate (and hopefully
intelligent). If I expect/assume you are going to review the
auto-resolutions after-the-fact then you can neglect this because you:
- have become complacent that git usually does-what-you-want so "you don't
really need to do it",
- are lazy and do it half-way,
- forget to do it,
- think "git magically does your work for you",
- don't know how to do it,
- don't even realize that anything auto-resolved or what auto-resolved,
- decide you don't have to do it because that is what testing if for,
- you think that your time is so valuable that an ounce-of-prevention on
your part is not worth a pound-of-cure on the part of others.
Please comment on the pros and cons of "manual-merge up-front for same-file
edits" vs. "review-and-remediate after-the-fact for auto-resolutions of
same-file edits".
Thanks in advance for your replies!
v/r,
neal
^ permalink raw reply
* Re: [PATCH v2] tag: refuse tag messages that contain NULs
From: Junio C Hamano @ 2012-02-21 20:47 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git
In-Reply-To: <1329741483-22567-1-git-send-email-pclouds@gmail.com>
Nguyễn Thái Ngọc Duy <pclouds@gmail.com> writes:
> It's not about after those. It's about right before write_sha1_file().
> I wanted to catch all NULs no matter how they come. But yes the check
> should happen early to avoid wasting user's time (e.g. doing signing)
No, it is not about that. It is about checking _the input_. If we had
bugs in do_sign() that adds what we do not want, that is not a user's
fault and "a NUL byte in tag message not allowed" is an inappropriate
thing to give to the user.
And giving "We screwed up and added NUL that you cannot work around to
remove, sorry, you hit a bug." is not very useful.
> So how about this?
> ...
> diff --git a/sha1_file.c b/sha1_file.c
> index 88f2151..2fc8623 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -2519,6 +2519,12 @@ int write_sha1_file(const void *buf, unsigned long len, const char *type, unsign
> char hdr[32];
> int hdrlen;
>
> + /* GIT_HASH_NUL is for the test suite to hash abitrary content */
> + if (!getenv("GIT_HASH_NUL") &&
> + (!strcmp(type, commit_type) || !strcmp(type, tag_type)) &&
> + memchr(buf, '\0', len))
> + return error("BUG: %s message contains NUL.", type);
> +
This is yucky. Is this really worth it?
> diff --git a/t/t7004-tag.sh b/t/t7004-tag.sh
> index e93ac73..8cb13e5 100755
> --- a/t/t7004-tag.sh
> +++ b/t/t7004-tag.sh
> @@ -1269,4 +1269,8 @@ test_expect_success 'mixing incompatibles modes and options is forbidden' '
> test_must_fail git tag -v -s
> '
>
> +test_expect_success 'tag content contains NUL' '
> + test_must_fail git tag -F "$TEST_DIRECTORY"/t3900/UTF-16.txt utf16
> +'
> +
This is caught without the change to write_sha1_file(), isn't it? If so,
I would say we should drop that GIT_HASH_NUL hunk.
^ permalink raw reply
* Re: [PATCH] completion: Allow dash as the first character for __git_ps1
From: Christian Hammerl @ 2012-02-21 20:49 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vty2jgbsh.fsf@alter.siamese.dyndns.org>
Yes, kind of.
It is more like PS1='\W$(__git_ps1 "-[ %s ]")'. And i am fixing it after each update on my machine. ;-)
Junio C Hamano <gitster@pobox.com> schrieb:
>Christian Hammerl <info@christian-hammerl.de> writes:
>
>> If the argument for `__git_ps1` begins with a dash, `printf` tries to
>> interpret it as an option which results in an error message.
>> The problem is solved by adding '--' before the argument to tell
>> `printf` to not interpret the following argument as an option.
>> Adding '--' directly to the argument does not help because the
>argument
>> is enclosed by double quotes.
>>
>> Signed-off-by: Christian Hammerl <info@christian-hammerl.de>
>> ---
>> contrib/completion/git-completion.bash | 2 +-
>> 1 files changed, 1 insertions(+), 1 deletions(-)
>>
>> diff --git a/contrib/completion/git-completion.bash
>b/contrib/completion/git-completion.bash
>> index 91c7acb..61ff152 100755
>> --- a/contrib/completion/git-completion.bash
>> +++ b/contrib/completion/git-completion.bash
>> @@ -313,7 +313,7 @@ __git_ps1 ()
>> fi
>>
>> local f="$w$i$s$u"
>> - printf "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
>> + printf -- "${1:- (%s)}" "$c${b##refs/heads/}${f:+ $f}$r$p"
>> fi
>> }
>
>Hmm, so you may be doing something like PS1='\W$(__git_ps1 "-%s")'?
>OK, and thanks.
--
Diese Nachricht wurde von meinem Android-Mobiltelefon mit K-9 Mail gesendet.
^ 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