Re: [PATCH] Revert retry request without query when info/refs?query fails
Shawn O. Pearce spea...@spearce.org writes: This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7. Retrying without the query parameter was added as a workaround for a single broken HTTP server at git.debian.org[1]. The server was misconfigured to route every request with a query parameter into gitweb.cgi. Admins fixed the server's configuration within 16 hours of the bug report to the Git mailing list, but we still patched Git with this fallback and have been paying for it since. As the consequence of the above, the only two things we know about the servers in the wild are (1) a misconfiguration that requires this retry was once made, so it is not very unlikely others did the same misconfiguration, and (2) those unknown number of servers have been happily serving the current clients because the workaround patch have been hiding the misconfiguration ever since. But as long as the failure diagnosis from updated clients that revert this workaround is sufficient to allow such misconfigured servers, I think it is OK. We might see a large number of small people having to run around and fix the configuration as a fallout, though. Most Git hosting services configure the smart HTTP protocol and the retry logic confuses users when there is a transient HTTP error as Git dropped the real error from the smart HTTP request. Removing the retry makes root causes easier to identify. Does that hold true also for dumb only small people installations? They are the ones that need more help than the large installations staffed sufficiently and run smart http gateway. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] test-string-list.c: Fix some sparse warnings
On 09/19/2012 09:07 PM, Ramsay Jones wrote: Michael Haggerty wrote: Is there some documentation about how to run sparse on the git codebase? I naively tried make sparse and ended up with zillions of errors like /usr/include/unistd.h:288:54: error: attribute '__leaf__': unknown attribute /usr/include/unistd.h:294:6: error: attribute '__leaf__': unknown attribute /usr/include/unistd.h:298:6: error: attribute '__leaf__': unknown attribute /usr/include/unistd.h:306:6: error: attribute '__leaf__': unknown attribute /usr/include/unistd.h:338:18: error: attribute '__leaf__': unknown attribute /usr/include/unistd.h:347:6: error: attribute '__leaf__': unknown attribute /usr/include/unistd.h:418:36: error: attribute '__leaf__': unknown attribute /usr/include/unistd.h:423:50: error: attribute '__leaf__': unknown attribute Yep, make sparse is the correct way to run sparse over git. This looks like you are running sparse on a 64-bit system. I have heard that it has (or *had*) problems running on 64-bit systems. Unfortunately, I am currently confined to 32-bit. (I'm looking at getting a new laptop soon, before Windows 8 causes boot-time problems, so I will then have the same problem!) How did you obtain/build/install sparse? The current release (v0.4.4) was released about Nov 2011 and I think you need a more up to date version. i.e. you need to build the latest, directly from the sparse repo. Yes, I'm running 64-bit Ubuntu 12.04 precise. I installed sparse from the Ubuntu multiverse repository. It is package version 0.4.3+20110419-1 in Ubuntu's notation. Thanks very much for all the info. I hadn't heard of sparse before and thought that using it might help me avoid submitting patches with problems like the ones you detected. It does seem promising! But since it seems a bit fiddly to get it running, and even then has some problems, it doesn't sound like the simple pre-submit checklist item that I had imagined. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v4 3/6] Color skipped tests blue
On Thu, Sep 20, 2012 at 6:48 AM, Johannes Sixt j.s...@viscovery.net wrote: Am 9/19/2012 22:24, schrieb Adam Spiers: skip) - tput bold; tput setaf 2;; # bold green + tput setaf 4;;# blue It's unreadable on black background. Keep it bold; that works on both black and white background. Whilst my preference aligns with yours in this particular case, we are now on a slippery slope, since these days terminal colors are infinitely configurable, and some heretics even choose backgrounds which are neither black nor white ;-) I don't want to trigger a long discussion or end up spamming the list with lots of different color scheme patches in an attempt to please everyone. So bold blue will be my last version of this patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v5 3/3] Color skipped tests bold blue
Skipped tests indicate incomplete test coverage. Whilst this is not a test failure or other error, it's still not complete success, so according to the universal traffic lights coloring scheme, yellow/brown seems more suitable than green. However, it's more informational than cautionary, so instead we use blue which is a universal color for information signs. Bold blue should work better on both black and white backgrounds than normal blue. Signed-off-by: Adam Spiers g...@adamspiers.org --- t/test-lib.sh | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/t/test-lib.sh b/t/test-lib.sh index 5293830..5ef87d4 100755 --- a/t/test-lib.sh +++ b/t/test-lib.sh @@ -182,13 +182,13 @@ then error) tput bold; tput setaf 1;; # bold red skip) - tput bold; tput setaf 2;; # bold green + tput bold; tput setaf 4;; # bold blue warn) tput bold; tput setaf 3;; # bold yellow pass) tput setaf 2;;# green info) - tput setaf 3;;# brown + tput setaf 3;;# yellow/brown *) test -n $quiet return;; esac -- 1.7.12.147.g6d168f4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v5 3/3] Color skipped tests bold blue
Hi Adam. On 09/20/2012 11:08 AM, Adam Spiers wrote: Skipped tests indicate incomplete test coverage. Whilst this is not a test failure or other error, it's still not complete success, so according to the universal traffic lights coloring scheme, yellow/brown seems more suitable than green. However, it's more informational than cautionary, so instead we use blue which is a universal color for information signs. Bold blue should work better on both black and white backgrounds than normal blue. A very minor nit (feel free to ignore it): IMHO, it should be nice to state explicitly in the commit message that blue is already used by other testsuite-related software to highlight skipped tests; you can report the examples of at least Automake, Autotest and prove -- and extra kudos if you find further examples ;-) Thanks, Stefano -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] log --oneline: put decoration at the end of the line
On Thu, Sep 20, 2012 at 6:42 AM, Jeff King p...@peff.net wrote: If you are particular about the exact format, how about using --format=%h%d %s instead? Obviously Duy could do the same to achieve his format, but I think there is still value in considering what the default for --oneline should be. Yeah I was wondering if a new default would make sense. I tried --pretty=format:%h %s %d but the colors were lost. --decorate uses more than one color so %C* does not immitate the colorful output we have with --decorate. Maybe I should add %Cd as colored %d and update my alias to use it. And perhaps a specifier to enable right alignment. It may help make better use of wide terminal screen. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] pretty: share code between format_decoration and show_decorations
This also adds color support to format_decoration()
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
log-tree.c | 55 +--
log-tree.h | 3 +++
pretty.c | 19 +--
3 files changed, 37 insertions(+), 40 deletions(-)
diff --git a/log-tree.c b/log-tree.c
index c894930..b8cea3f 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -174,36 +174,47 @@ static void show_children(struct rev_info *opt, struct
commit *commit, int abbre
}
}
-void show_decorations(struct rev_info *opt, struct commit *commit)
+void format_decoration(struct strbuf *sb,
+ const struct commit *commit,
+ int use_color)
{
- const char *prefix;
- struct name_decoration *decoration;
+ const char *prefix = (;
+ struct name_decoration *d;
const char *color_commit =
- diff_get_color_opt(opt-diffopt, DIFF_COMMIT);
+ diff_get_color(use_color, DIFF_COMMIT);
const char *color_reset =
- decorate_get_color_opt(opt-diffopt, DECORATION_NONE);
+ decorate_get_color(use_color, DECORATION_NONE);
+
+ load_ref_decorations(DECORATE_SHORT_REFS);
+ d = lookup_decoration(name_decoration, commit-object);
+ if (!d)
+ return;
+ while (d) {
+ strbuf_addstr(sb, prefix);
+ strbuf_addstr(sb, decorate_get_color(use_color, d-type));
+ if (d-type == DECORATION_REF_TAG)
+ strbuf_addstr(sb, tag: );
+ strbuf_addstr(sb, d-name);
+ strbuf_addstr(sb, color_reset);
+ strbuf_addstr(sb, color_commit);
+ prefix = , ;
+ d = d-next;
+ }
+ if (prefix[0] == ',')
+ strbuf_addch(sb, ')');
+}
+
+void show_decorations(struct rev_info *opt, struct commit *commit)
+{
+ struct strbuf sb = STRBUF_INIT;
if (opt-show_source commit-util)
printf(\t%s, (char *) commit-util);
if (!opt-show_decorations)
return;
- decoration = lookup_decoration(name_decoration, commit-object);
- if (!decoration)
- return;
- prefix = (;
- while (decoration) {
- printf(%s, prefix);
- fputs(decorate_get_color_opt(opt-diffopt, decoration-type),
- stdout);
- if (decoration-type == DECORATION_REF_TAG)
- fputs(tag: , stdout);
- printf(%s, decoration-name);
- fputs(color_reset, stdout);
- fputs(color_commit, stdout);
- prefix = , ;
- decoration = decoration-next;
- }
- putchar(')');
+ format_decoration(sb, commit, opt-diffopt.use_color);
+ fputs(sb.buf, stdout);
+ strbuf_release(sb);
}
/*
diff --git a/log-tree.h b/log-tree.h
index f5ac238..10c2682 100644
--- a/log-tree.h
+++ b/log-tree.h
@@ -13,6 +13,9 @@ int log_tree_diff_flush(struct rev_info *);
int log_tree_commit(struct rev_info *, struct commit *);
int log_tree_opt_parse(struct rev_info *, const char **, int);
void show_log(struct rev_info *opt);
+void format_decoration(struct strbuf *sb,
+ const struct commit *commit,
+ int use_color);
void show_decorations(struct rev_info *opt, struct commit *commit);
void log_write_email_headers(struct rev_info *opt, struct commit *commit,
const char **subject_p,
diff --git a/pretty.c b/pretty.c
index 8b1ea9f..e910679 100644
--- a/pretty.c
+++ b/pretty.c
@@ -764,23 +764,6 @@ static void parse_commit_message(struct
format_commit_context *c)
c-commit_message_parsed = 1;
}
-static void format_decoration(struct strbuf *sb, const struct commit *commit)
-{
- struct name_decoration *d;
- const char *prefix = (;
-
- load_ref_decorations(DECORATE_SHORT_REFS);
- d = lookup_decoration(name_decoration, commit-object);
- while (d) {
- strbuf_addstr(sb, prefix);
- prefix = , ;
- strbuf_addstr(sb, d-name);
- d = d-next;
- }
- if (prefix[0] == ',')
- strbuf_addch(sb, ')');
-}
-
static void strbuf_wrap(struct strbuf *sb, size_t pos,
size_t width, size_t indent1, size_t indent2)
{
@@ -1005,7 +988,7 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
strbuf_addstr(sb, get_revision_mark(NULL, commit));
return 1;
case 'd':
- format_decoration(sb, commit);
+ format_decoration(sb, commit, 0);
return 1;
case 'g': /* reflog info */
switch(placeholder[1]) {
--
1.7.12.1.383.gda6001e.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to
[PATCH 2/2] pretty: support placeholders %C+ and %C-
%C+ tells the next specifiers that color is preferred. %C- the
opposite. So far only %H, %h and %d support coloring.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
Documentation/pretty-formats.txt | 2 ++
pretty.c | 13 -
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt
index e3d8a83..6e287d6 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -142,6 +142,8 @@ The placeholders are:
- '%Cblue': switch color to blue
- '%Creset': reset color
- '%C(...)': color specification, as described in color.branch.* config option
+- '%C+': enable coloring on the following placeholders if supported
+- '%C-': disable coloring on the following placeholders
- '%m': left, right or boundary mark
- '%n': newline
- '%%': a raw '%'
diff --git a/pretty.c b/pretty.c
index e910679..b1cec71 100644
--- a/pretty.c
+++ b/pretty.c
@@ -623,6 +623,7 @@ struct format_commit_context {
unsigned commit_header_parsed:1;
unsigned commit_message_parsed:1;
unsigned commit_signature_parsed:1;
+ unsigned use_color:1;
struct {
char *gpg_output;
char good_bad;
@@ -885,6 +886,12 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
--pretty format, color);
strbuf_addstr(sb, color);
return end - placeholder + 1;
+ } else if (placeholder[1] == '+') {
+ c-use_color = 1;
+ return 2;
+ } else if (placeholder[1] == '-') {
+ c-use_color = 0;
+ return 2;
}
if (!prefixcmp(placeholder + 1, red)) {
strbuf_addstr(sb, GIT_COLOR_RED);
@@ -945,13 +952,17 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
switch (placeholder[0]) {
case 'H': /* commit hash */
+ strbuf_addstr(sb, diff_get_color(c-use_color, DIFF_COMMIT));
strbuf_addstr(sb, sha1_to_hex(commit-object.sha1));
+ strbuf_addstr(sb, diff_get_color(c-use_color, DIFF_RESET));
return 1;
case 'h': /* abbreviated commit hash */
+ strbuf_addstr(sb, diff_get_color(c-use_color, DIFF_COMMIT));
if (add_again(sb, c-abbrev_commit_hash))
return 1;
strbuf_addstr(sb, find_unique_abbrev(commit-object.sha1,
c-pretty_ctx-abbrev));
+ strbuf_addstr(sb, diff_get_color(c-use_color, DIFF_RESET));
c-abbrev_commit_hash.len = sb-len - c-abbrev_commit_hash.off;
return 1;
case 'T': /* tree hash */
@@ -988,7 +999,7 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
strbuf_addstr(sb, get_revision_mark(NULL, commit));
return 1;
case 'd':
- format_decoration(sb, commit, 0);
+ format_decoration(sb, commit, c-use_color);
return 1;
case 'g': /* reflog info */
switch(placeholder[1]) {
--
1.7.12.1.383.gda6001e.dirty
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
And this is a for-fun patch that adds %| to right align everything
after that. I'm ignoring problems with line wrapping, i18n and so
on. %C+%h %s%|%d looks quite nice. I'm not sure how much useful it
is beyond --oneline though. It looks something like this
cc543b2 pretty: support placeholders %C+ and %C-
(HEAD, master)
da6001e pretty: share code between format_decoration and show_decorations
b0576a6 Update draft release notes to 1.8.0
(origin/master, origin/HEAD)
3d7535e Merge branch 'jc/maint-log-grep-all-match'
06e211a Merge branch 'jc/make-static'
8db3865 Merge branch 'pw/p4-submit-conflicts'
3387423 Merge branch 'mv/cherry-pick-s'
d71abd9 Merge branch 'nd/fetch-status-alignment'
3c7d509 Sync with 1.7.12.1
304b7d9 Git 1.7.12.1
(tag: v1.7.12.1, origin/maint)
39e2e02 Merge branch 'er/doc-fast-import-done' into maint
8ffc331 Merge branch 'jk/config-warn-on-inaccessible-paths' into maint
01f7d7f Doc: Improve shallow depth wording
8093ae8 Documentation/git-filter-branch: Move note about effect of removing
commits
-- 8 --
diff --git a/pretty.c b/pretty.c
index b1cec71..6e96f83 100644
--- a/pretty.c
+++ b/pretty.c
@@ -624,6 +624,7 @@ struct format_commit_context {
unsigned commit_message_parsed:1;
unsigned commit_signature_parsed:1;
unsigned use_color:1;
+ unsigned right_alignment:1;
struct {
char *gpg_output;
char good_bad;
@@ -645,6 +646,8 @@ struct format_commit_context {
struct chunk abbrev_tree_hash;
struct chunk abbrev_parent_hashes;
size_t wrap_start;
+
+ struct strbuf *right_sb;
};
static int add_again(struct strbuf *sb, struct chunk *chunk)
@@ -944,6 +947,10 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
return end - placeholder + 1;
} else
return 0;
+
+ case '|':
+ c-right_alignment = 1;
+ return 1;
}
/* these depend on the commit */
@@ -1099,9 +1106,44 @@ static size_t format_commit_one(struct strbuf *sb, const
char *placeholder,
return 0; /* unknown placeholder */
}
+static void right_align(struct strbuf *sb,
+ struct format_commit_context *c,
+ int flush)
+{
+ const char *p;
+ int llen, rlen, len, total = term_columns() - 1;
+ if (!c-right_alignment)
+ return;
+ p = strchr(c-right_sb-buf, '\n');
+ if (!p flush)
+ p = c-right_sb-buf + c-right_sb-len;
+ if (!p)
+ return;
+
+ c-right_alignment = 0;
+ len = p - c-right_sb-buf;
+ if (!len)
+ return;
+ if (total 110)
+ total = 110;
+ rlen = utf8_strnwidth(c-right_sb-buf, len);
+ p = strrchr(sb-buf, '\n');
+ if (!p)
+ p = sb-buf;
+ else
+ p++;
+ llen = utf8_strwidth(p);
+ strbuf_addf(sb, %*s,
+ total - llen + (len - rlen),
+ c-right_sb-buf);
+ strbuf_reset(c-right_sb);
+}
+
static size_t format_commit_item(struct strbuf *sb, const char *placeholder,
void *context)
{
+ struct format_commit_context *c = context;
+ struct strbuf *real_sb;
int consumed;
size_t orig_len;
enum {
@@ -1127,10 +1169,13 @@ static size_t format_commit_item(struct strbuf *sb,
const char *placeholder,
if (magic != NO_MAGIC)
placeholder++;
+ if (c-right_alignment c-right_sb) {
+ real_sb = sb;
+ sb = c-right_sb;
+ }
+
orig_len = sb-len;
consumed = format_commit_one(sb, placeholder, context);
- if (magic == NO_MAGIC)
- return consumed;
if ((orig_len == sb-len) magic == DEL_LF_BEFORE_EMPTY) {
while (sb-len sb-buf[sb-len - 1] == '\n')
@@ -1141,7 +1186,13 @@ static size_t format_commit_item(struct strbuf *sb,
const char *placeholder,
else if (magic == ADD_SP_BEFORE_NON_EMPTY)
strbuf_insert(sb, orig_len, , 1);
}
- return consumed + 1;
+
+ if (real_sb)
+ right_align(real_sb, c, 0);
+
+ if (magic != NO_MAGIC)
+ consumed++;
+ return consumed;
}
static size_t userformat_want_item(struct strbuf *sb, const char *placeholder,
@@ -1180,12 +1231,14 @@ void format_commit_message(const struct commit *commit,
struct format_commit_context context;
static const char utf8[] = UTF-8;
const char *output_enc = pretty_ctx-output_encoding;
+ struct strbuf right_sb = STRBUF_INIT;
memset(context, 0, sizeof(context));
context.commit = commit;
context.pretty_ctx =
Re: [PATCH v5 3/3] Color skipped tests bold blue
Stefano Lattarini stefano.lattar...@gmail.com writes: Hi Adam. On 09/20/2012 11:08 AM, Adam Spiers wrote: Skipped tests indicate incomplete test coverage. Whilst this is not a test failure or other error, it's still not complete success, so according to the universal traffic lights coloring scheme, yellow/brown seems more suitable than green. However, it's more informational than cautionary, so instead we use blue which is a universal color for information signs. Bold blue should work better on both black and white backgrounds than normal blue. A very minor nit (feel free to ignore it): IMHO, it should be nice to state explicitly in the commit message that blue is already used by other testsuite-related software to highlight skipped tests; you can report the examples of at least Automake, Autotest and prove -- and extra kudos if you find further examples ;-) Thanks. Will tweak the proposed log message to include the above when queuing the updated patch. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Revert retry request without query when info/refs?query fails
On Wed, Sep 19, 2012 at 11:29:34PM -0700, Junio C Hamano wrote: Shawn O. Pearce spea...@spearce.org writes: This reverts commit 703e6e76a14825e5b0c960d525f34e607154b4f7. Retrying without the query parameter was added as a workaround for a single broken HTTP server at git.debian.org[1]. The server was misconfigured to route every request with a query parameter into gitweb.cgi. Admins fixed the server's configuration within 16 hours of the bug report to the Git mailing list, but we still patched Git with this fallback and have been paying for it since. As the consequence of the above, the only two things we know about the servers in the wild are (1) a misconfiguration that requires this retry was once made, so it is not very unlikely others did the same misconfiguration, and (2) those unknown number of servers have been happily serving the current clients because the workaround patch have been hiding the misconfiguration ever since. The misconfiguration was pretty wild in this case. I'd be much more worried about stupidly non-compliant servers that will not serve foo/bar when asked for foo/bar?key=value. But as long as the failure diagnosis from updated clients that revert this workaround is sufficient to allow such misconfigured servers, I think it is OK. We might see a large number of small people having to run around and fix the configuration as a fallout, though. I think Shawn's revert is the right thing to do. But it is not complete without the manual workaround. I'm putting that patch together now and should have it out in a few minutes. Most Git hosting services configure the smart HTTP protocol and the retry logic confuses users when there is a transient HTTP error as Git dropped the real error from the smart HTTP request. Removing the retry makes root causes easier to identify. Does that hold true also for dumb only small people installations? They are the ones that need more help than the large installations staffed sufficiently and run smart http gateway. For the most part, yes. They will get a useful error out of the smart request if there is a transient error, the repo does not exist, etc. The real fallout is the people who are hitting a broken or misconfigured server and may get a confusing error code (in the one case we know about, it was a 404, but it really could be anything, depending on the exact nature of the misconfiguration). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/2] pretty: support right alignment
Nguyen Thai Ngoc Duy pclo...@gmail.com writes: And this is a for-fun patch that adds %| to right align everything after that. I'm ignoring problems with line wrapping, i18n and so on. %C+%h %s%|%d looks quite nice. I'm not sure how much useful it is beyond --oneline though. It looks something like this ... [oneline output ellided] ... I think this is a great feature at the conceptual level, and you know but is coming ;-). - Shouldn't it be everything from there until the end of the current line than everything after that? - How is the display width determined and is it fixed once it gets computed? - How does this interact with the wrapped output? Should it? - I am wondering if somebody ever want to do this with a follow-up patch: Left %h%|Center %cd%|Right %ad Is %| a sensible choice for flush right? I am wondering if it makes more sense to make %|, % and % as multi-column introducer (the example defines output with three columns) that also tells how text inside each column is flushed inside the column, e.g. %col 1 right flushed%|col 2 centered% col 3 left flushed or something like that (we may want explicit column width specifiers if we were to do this kind of thing). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improve legibility of test_expect_code output
Adam Spiers g...@adamspiers.org writes: On Thu, Sep 20, 2012 at 1:06 AM, Junio C Hamano gits...@pobox.com wrote: Adam Spiers g...@adamspiers.org writes: --- No explanation why this is a good idea, nor sign-off? I realised I forgot the sign-off seconds after sending :-( Isn't it completely self-explanatory? e.g. test_expect_code: command exited with 0, we wanted 128 git foo bar clearly makes more sense than test_expect_code: command exited with 0, we wanted 128 from: git foo bar test_expect_code: command exited with 0, we wanted 128: git foo bar would be shorter and equally legible, I would think. In any case, the proposed commit log message should have explained these differences in the first place so that I or others do not have to ask. Do you want this queued on top of your other series, or as an independent change? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] completion: fix shell expansion of items
Jeff King p...@peff.net writes:
On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote:
As reported by Jeroen Meijer[1]; the current code doesn't deal properly
with items (tags, branches, etc.) that have ${} in them because they get
expaned by bash while using compgen.
A simple solution is to quote the items so they get expanded properly
(\$\{\}).
In order to achieve that I took bash-completion's quote() function,
which is rather simple, and renamed it to __git_quote() as per Jeff
King's suggestion.
Solves the original problem for me.
Me too. Thanks.
Thanks, both. Will queue.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Improve legibility of test_expect_code output
On Thu, Sep 20, 2012 at 5:50 PM, Junio C Hamano gits...@pobox.com wrote: Do you want this queued on top of your other series, or as an independent change? Independent please. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/2] smart http toggle switch fails
On Thu, Sep 20, 2012 at 12:24:56PM -0400, Jeff King wrote: I think Shawn's revert is the right thing to do. But it is not complete without the manual workaround. I'm putting that patch together now and should have it out in a few minutes. And here it is. This goes on top of Shawn's revert patch (it might be nice to mention the commit id of that in the commit message of the second patch. I couldn't do so because it is not yet in your repo). [1/2]: remote-curl: rename is_http variable [2/2]: remote-curl: let users turn off smart http -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/2] remote-curl: rename is_http variable
We don't actually care whether the connection is http or
not; what we care about is whether it might be smart http.
Rename the variable to be more accurate, which will make it
easier to later make smart-http optional.
Signed-off-by: Jeff King p...@peff.net
---
remote-curl.c | 6 +++---
1 file changed, 3 insertions(+), 3 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 2359f59..c0b98cc 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -95,7 +95,7 @@ static struct discovery* discover_refs(const char *service)
struct strbuf buffer = STRBUF_INIT;
struct discovery *last = last_discovery;
char *refs_url;
- int http_ret, is_http = 0;
+ int http_ret, maybe_smart = 0;
if (last !strcmp(service, last-service))
return last;
@@ -103,7 +103,7 @@ static struct discovery* discover_refs(const char *service)
strbuf_addf(buffer, %sinfo/refs, url);
if (!prefixcmp(url, http://;) || !prefixcmp(url, https://;)) {
- is_http = 1;
+ maybe_smart = 1;
if (!strchr(url, '?'))
strbuf_addch(buffer, '?');
else
@@ -131,7 +131,7 @@ static struct discovery* discover_refs(const char *service)
last-buf_alloc = strbuf_detach(buffer, last-len);
last-buf = last-buf_alloc;
- if (is_http 5 = last-len last-buf[4] == '#') {
+ if (maybe_smart 5 = last-len last-buf[4] == '#') {
/* smart HTTP response; validate that the service
* pkt-line matches our request.
*/
--
1.7.11.7.15.g085c6bd
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/2] remote-curl: let users turn off smart http
Usually there is no need for users to specify whether an
http remote is smart or dumb; the protocol is designed so
that a single initial request is made, and the client can
determine the server's capability from the response.
However, some misconfigured dumb-only servers may not like
the initial request by a smart client, as it contains a
query string. Until recently, commit 703e6e7 worked around
this by making a second request. However, that commit was
recently reverted due to its side effect of masking the
initial request's error code.
This patch takes a different approach to the workaround. We
assume that the common case is that the server is either
smart-http or a reasonably configured dumb-http. If that is
not the case, we provide both a per-remote config option and
an environment variable with which the user can manually
work around the issue.
Signed-off-by: Jeff King p...@peff.net
---
I added the config item as remote.foo.smarthttp. You could also allow
http.$url.smart (and just http.smart, for that matter), which could
be more flexible if you have multiple remotes pointing to the same
broken server. However, it is also more complex to use, and is a lot
more code. Since we don't know if any such servers even exist, I tried
to give the minimal escape hatch, and we can easily build more features
on it later if people complain.
Documentation/config.txt | 11 +++
remote-curl.c| 3 ++-
remote.c | 3 +++
remote.h | 1 +
t/t5551-http-fetch.sh| 17 +
5 files changed, 34 insertions(+), 1 deletion(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 6416cae..651b23c 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1871,6 +1871,17 @@ remote.name.uploadpack::
The default program to execute on the remote side when fetching. See
option \--upload-pack of linkgit:git-fetch-pack[1].
+remote.name.smartHttp::
+ If true, this remote will attempt to use git's smart http
+ protocol when making remote http requests. Normally git sends an
+ initial smart-http request, and falls back to the older dumb
+ protocol if the server does not claim to support the smart
+ protocol. However, some misconfigured dumb-only servers may
+ produce confusing results for the initial request. Setting this
+ option to false disables the initial smart request, which can
+ workaround problems with such servers. You should not generally
+ need to set this. Defaults to `true`.
+
remote.name.tagopt::
Setting this value to \--no-tags disables automatic tag following when
fetching from remote name. Setting it to \--tags will fetch every
diff --git a/remote-curl.c b/remote-curl.c
index c0b98cc..8829bfb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -102,7 +102,8 @@ static struct discovery* discover_refs(const char *service)
free_discovery(last);
strbuf_addf(buffer, %sinfo/refs, url);
- if (!prefixcmp(url, http://;) || !prefixcmp(url, https://;)) {
+ if ((!prefixcmp(url, http://;) || !prefixcmp(url, https://;))
+git_env_bool(GIT_SMART_HTTP, remote-smart_http)) {
maybe_smart = 1;
if (!strchr(url, '?'))
strbuf_addch(buffer, '?');
diff --git a/remote.c b/remote.c
index 04fd9ea..a334390 100644
--- a/remote.c
+++ b/remote.c
@@ -152,6 +152,7 @@ static struct remote *make_remote(const char *name, int len)
ret-name = xstrndup(name, len);
else
ret-name = xstrdup(name);
+ ret-smart_http = 1;
return ret;
}
@@ -453,6 +454,8 @@ static int handle_config(const char *key, const char
*value, void *cb)
key, value);
} else if (!strcmp(subkey, .vcs)) {
return git_config_string(remote-foreign_vcs, key, value);
+ } else if (!strcmp(subkey, .smarthttp)) {
+ remote-smart_http = git_config_bool(key, value);
}
return 0;
}
diff --git a/remote.h b/remote.h
index 251d8fd..9031d18 100644
--- a/remote.h
+++ b/remote.h
@@ -40,6 +40,7 @@ struct remote {
int fetch_tags;
int skip_default_update;
int mirror;
+ int smart_http;
const char *receivepack;
const char *uploadpack;
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 2db5c35..48173ed 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -129,6 +129,23 @@ test_expect_success 'clone from auth-only-for-push
repository' '
test_cmp expect actual
'
+test_expect_success 'disable dumb http on server' '
+ git --git-dir=$HTTPD_DOCUMENT_ROOT_PATH/repo.git \
+ config http.getanyfile false
+'
+
+test_expect_success 'GIT_SMART_HTTP can disable smart http' '
+ (GIT_SMART_HTTP=0
+export GIT_SMART_HTTP
+cd clone
+test_must_fail git fetch)
+'
Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0
On Wed, Sep 19, 2012 at 10:57:35PM -0700, Shawn O. Pearce wrote: I have been looking into this recently, as well. GitHub does not allow dumb http at all these days, Interesting that GitHub doesn't support dumb transfer either. Our objects are still in regular repos, and is served by fairly stock git. The main show-stopper is that we share objects via alternates, and we aggressively repack the alternates repos. So a dumb client would end up being quite inefficient. We also toyed with having mixed public/private fork networks at one point, which would obviously necessitate disabling dumb access. But we gave it up as too risky, as you can do all sorts of weird tricks to convince git to disclose information about what's in the private repos (e.g., you can reliably find out which sha1s exist, and you can lie about which sha1s you have to ask git to create deltas against them). Doing a shared-object store right would require marking which repo knows which objects (I assume that's what Google Code does, but I don't remember if Dave talked about that aspect at last year's GitTogether). so transient errors on the initial smart contact can cause us to fall back to dumb, Transient errors is actually what is leading me down this path. We see about 0.0455% of our requests to the Android hosting service as these dumb fallbacks. This means the client never got a proper smart service reply. Server logs suggest we sent a valid response, so I am suspecting transient proxy errors, but its hard to debug because clients discard the error. Yup, we see the same thing. I've tracked a few down manually to actual things like gateway timeouts on our reverse proxy. and end up reporting a totally useless 403 Forbidden error. Today I posted a change to JGit [1] to make this 406 Not Acceptable as I think it better matches the description of the failure. It also no longer reuses the common 403 Forbidden error that a server might choose to return if a request was given with invalid credentials. That might be worth doing for git-http-backend, too. It might even make sense for the git client to recognize the 406 (and possibly the 403) and print a more useful message. 1. If both smart and dumb requests fail, report the error for the smart request. Now that smart-http clients are common, I'd expect most http servers to be smart these days. Of course I don't have any sort of numbers to back this up (nor am I sure how to get them; obviously big sites like GitHub and Google Code do a lot of traffic, but who knows how many one-off repo-on-a-generic-web-host sites still exist?). I suspect there are still a number of servers that rely on dumb protocol to host repositories because its very simple to setup. Breaking support for this wouldn't be a good idea. I don't think it would break on most servers, though. Even for a dumb server, the initial error will be a useful one. It's only the weirdly-configured ones where you get wildly different results depending on whether the query string is there. In other words, it is really no worse than reverting 703e6e76, and it might be better. In the common case, you get a better error message. In the broken-server case, we still try the fallback. So it will keep working on a broken server without any manual intervention, whereas reverting and adding a manual escape hatch means the user has to do something. BUT. I still think it's better to revert 703e6e76, because I really do think the broken-server case is an extreme enough minority that it is not even worth wasting the time of clients to make a second request (especially because the first request may very well have failed because of a network error that causes a long timeout, and the user then has to wait double to find out what the error is). Of course, I have no actual data aside from reading the original thread that led to 703e6e76, and the fact that nobody else mentioned it (not during the time when it was broken, and not even after, when people often still complain because they haven't upgraded yet). But who knows? Maybe I will eat my words, and we will end up getting that data in the form of complaints. :) We can always switch to fallback-but-prefer-the-initial-error then. And we'll have more data on exactly how the misconfigured servers behave. -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] pretty: support placeholders %C+ and %C-
Junio C Hamano gits...@pobox.com writes: Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: %C+ tells the next specifiers that color is preferred. %C- the opposite. So far only %H, %h and %d support coloring. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- Documentation/pretty-formats.txt | 2 ++ pretty.c | 13 - 2 files changed, 14 insertions(+), 1 deletion(-) diff --git a/Documentation/pretty-formats.txt b/Documentation/pretty-formats.txt index e3d8a83..6e287d6 100644 --- a/Documentation/pretty-formats.txt +++ b/Documentation/pretty-formats.txt @@ -142,6 +142,8 @@ The placeholders are: - '%Cblue': switch color to blue - '%Creset': reset color - '%C(...)': color specification, as described in color.branch.* config option +- '%C+': enable coloring on the following placeholders if supported +- '%C-': disable coloring on the following placeholders OK, so typically you replace some format placeholder %? in your format string with %C+%?%C-, because you cannot get away with replacing it with %C+%? and other things in the format you do not know if they support coloring%C-. If that is the case, does it really make sense to have %C-? It smells as if it makes more sense to make _all_ %? placeholder reset the effect of %C+ after they are done (even the ones that they themselves do not color their own output elements), so that you can mechanically replace %? with %C+%?. I dunno. Thinking about this a bit more, perhaps we would want a generic mechanism to give parameters to various %? placeholders. This is not limited to I can do color but there is no mechanism for the user to tell me that I should do color %H, %h and %d may want to say. An obvious and immediate example is that %h might want to be told how many hexdigits it should use. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCHv8] clone --single: limit the fetch refspec to fetched branch
After running git clone --single, the resulting repository has the
usual default +refs/heads/*:refs/remotes/origin/* wildcard fetch
refspec installed, which means that a subsequent git fetch will
end up grabbing all the other branches.
Update the fetch refspec to cover only the singly cloned ref instead
to correct this.
That means:
If --single is used without --branch or --mirror, the
fetch refspec covers the branch on which remote's HEAD points to.
If --single is used with --branch, it'll cover only the branch
specified in the --branch option.
If --single is combined with --mirror, then it'll cover all
refs of the cloned repository.
If --single is used with --branch that specifies a tag, then
it'll cover only the ref for this tag.
Signed-off-by: Ralf Thielow ralf.thie...@gmail.com
---
changes in v8:
- remove unnecessary strbuf_reset
- remove test from v7 which has shown that tags get fetched when cloning
- add test to show that tags will not being updated when using simple git
clone
- change position of the tag in the setup, not directly in the test
Documentation/git-clone.txt | 15 +++--
builtin/clone.c | 65 ++
t/t5709-clone-refspec.sh| 156
3 Dateien geändert, 218 Zeilen hinzugefügt(+), 18 Zeilen entfernt(-)
create mode 100755 t/t5709-clone-refspec.sh
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index c1ddd4c..6d98ef3 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -29,7 +29,8 @@ currently active branch.
After the clone, a plain `git fetch` without arguments will update
all the remote-tracking branches, and a `git pull` without
arguments will in addition merge the remote master branch into the
-current master branch, if any.
+current master branch, if any (this is untrue when --single-branch
+is given; see below).
This default configuration is achieved by creating references to
the remote branch heads under `refs/remotes/origin` and
@@ -152,9 +153,10 @@ objects from the source repository into a pack in the
cloned repository.
-b name::
Instead of pointing the newly created HEAD to the branch pointed
to by the cloned repository's HEAD, point to `name` branch
- instead. `--branch` can also take tags and treat them like
- detached HEAD. In a non-bare repository, this is the branch
- that will be checked out.
+ instead. In a non-bare repository, this is the branch that will
+ be checked out.
+ `--branch` can also take tags and detaches the HEAD at that commit
+ in the resulting repository.
--upload-pack upload-pack::
-u upload-pack::
@@ -193,6 +195,11 @@ objects from the source repository into a pack in the
cloned repository.
clone with the `--depth` option, this is the default, unless
`--no-single-branch` is given to fetch the histories near the
tips of all branches.
+ Further fetches into the resulting repository will only update the
+ remote tracking branch for the branch this option was used for the
+ initial cloning. If the HEAD at the remote did not point at any
+ branch when `--single-branch` clone was made, no remote tracking
+ branch is created.
--recursive::
--recurse-submodules::
diff --git a/builtin/clone.c b/builtin/clone.c
index 5e8f3ba..047239c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -610,6 +610,54 @@ static void write_config(struct string_list *config)
}
}
+static void write_refspec_config(const char* src_ref_prefix,
+ const struct ref* our_head_points_at,
+ const struct ref* remote_head_points_at, struct strbuf*
branch_top)
+{
+ struct strbuf key = STRBUF_INIT;
+ struct strbuf value = STRBUF_INIT;
+
+ if (option_mirror || !option_bare) {
+ if (option_single_branch !option_mirror) {
+ if (option_branch) {
+ if (strstr(our_head_points_at-name,
refs/tags/))
+ strbuf_addf(value, +%s:%s,
our_head_points_at-name,
+ our_head_points_at-name);
+ else
+ strbuf_addf(value, +%s:%s%s,
our_head_points_at-name,
+ branch_top-buf, option_branch);
+ } else if (remote_head_points_at) {
+ strbuf_addf(value, +%s:%s%s,
remote_head_points_at-name,
+ branch_top-buf,
+
skip_prefix(remote_head_points_at-name, refs/heads/));
+ }
+ /*
+* otherwise, the next git fetch will
+* simply fetch from HEAD without updating
+* any remote tracking branch, which is
Re: [PATCH 2/2] remote-curl: let users turn off smart http
On Thu, Sep 20, 2012 at 10:53:15AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I added the config item as remote.foo.smarthttp. You could also allow http.$url.smart (and just http.smart, for that matter), which could be more flexible if you have multiple remotes pointing to the same broken server. What would the user experience be when we introduce even smarter http server protocol extension? Will we add remote.foo.starterhttp? I would hope that it would actually be negotiated reliably at the protocol level so we do not have to deal with this mess again. Perhaps remote.$name.httpvariants = [smart] [dumb] to allow users to say smart only, dumb only, or smart and/or dumb might be more code but less burden on the users. I don't mind that format if we are going that direction, but is there anybody who actually wants to say smart only? -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] completion: fix shell expansion of items
Hi,
On Wed, Sep 19, 2012 at 09:46:08PM -0400, Jeff King wrote:
On Thu, Sep 20, 2012 at 04:15:15AM +0200, Felipe Contreras wrote:
As reported by Jeroen Meijer[1]; the current code doesn't deal properly
with items (tags, branches, etc.) that have ${} in them because they get
expaned by bash while using compgen.
A simple solution is to quote the items so they get expanded properly
(\$\{\}).
In order to achieve that I took bash-completion's quote() function,
which is rather simple, and renamed it to __git_quote() as per Jeff
King's suggestion.
Solves the original problem for me.
Me too. Thanks.
While it solves the original problem, it seems to break refs
completion, as demonstrated by the following POC test:
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 92d7eb47..fab63b95 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -228,4 +228,11 @@ test_expect_success 'general options plus command' '
test_completion git --no-replace-objects check checkout
'
+test_expect_success 'basic refs completion' '
+ touch file
+ git add file
+ git commit -m initial
+ test_completion git branch m master
+'
+
test_done
--
1.7.12.1.438.g7dfa67b
which fails with:
--- expected2012-09-20 18:05:23.857752925 +
+++ out 2012-09-20 18:05:23.877752925 +
@@ -1 +1 @@
-master
+
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] completion: fix shell expansion of items
On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote: In order to achieve that I took bash-completion's quote() function, which is rather simple, and renamed it to __git_quote() as per Jeff King's suggestion. Solves the original problem for me. Me too. Thanks. While it solves the original problem, it seems to break refs completion, as demonstrated by the following POC test: diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 92d7eb47..fab63b95 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' ' test_completion git --no-replace-objects check checkout ' +test_expect_success 'basic refs completion' ' + touch file + git add file + git commit -m initial + test_completion git branch m master +' Hmm. I notice that Felipe's patch wraps the _whole_ input to __gitcomp_nl in single quotes. So if there are multiple completions we would end up with: 'one two quo\'ted three' I wonder if that is OK to feed to compgen -W, or if it wants to expand it line-by-line. Just guessing at this point, though. -Peff + test_done -- 1.7.12.1.438.g7dfa67b which fails with: --- expected2012-09-20 18:05:23.857752925 + +++ out 2012-09-20 18:05:23.877752925 + @@ -1 +1 @@ -master + -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] remote-curl: let users turn off smart http
Jeff King p...@peff.net writes: On Thu, Sep 20, 2012 at 10:53:15AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I added the config item as remote.foo.smarthttp. You could also allow http.$url.smart (and just http.smart, for that matter), which could be more flexible if you have multiple remotes pointing to the same broken server. What would the user experience be when we introduce even smarter http server protocol extension? Will we add remote.foo.starterhttp? I would hope that it would actually be negotiated reliably at the protocol level so we do not have to deal with this mess again. The original dumb vs smart was supposed to be negotiated reliably at the protocol level, no? Yet we need this band-aid, so... Perhaps remote.$name.httpvariants = [smart] [dumb] to allow users to say smart only, dumb only, or smart and/or dumb might be more code but less burden on the users. I don't mind that format if we are going that direction, but is there anybody who actually wants to say smart only? With 703e6e7 reverted, we take a failure from the initial smart request to mean the server is simply not serving, so smart only to fail quickly without trying dumb fallback is not needed. smart only to say I wouldn't want to talk to dumb-only server---I do not have infinite amount of time, and I'd rather try another server is still a possibility, but likely not worth supporting. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: The GitTogether
On 19.09.2012 15:43, Michael Haggerty wrote: Is there any news about the proposed gatherings? I would be quite interested in attending the developer meeting. October is just around the corner...what's up? I'm also very much interested in attending a gathering Berlin, though preferably not in the first week of October. As I'm a local, I could probably also help with finding a location if necessary. -- Sebastian Schuberth -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] completion: fix shell expansion of items
On Thu, Sep 20, 2012 at 8:21 PM, Jeff King p...@peff.net wrote: On Thu, Sep 20, 2012 at 08:11:52PM +0200, SZEDER Gábor wrote: In order to achieve that I took bash-completion's quote() function, which is rather simple, and renamed it to __git_quote() as per Jeff King's suggestion. Solves the original problem for me. Me too. Thanks. While it solves the original problem, it seems to break refs completion, as demonstrated by the following POC test: diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh index 92d7eb47..fab63b95 100755 --- a/t/t9902-completion.sh +++ b/t/t9902-completion.sh @@ -228,4 +228,11 @@ test_expect_success 'general options plus command' ' test_completion git --no-replace-objects check checkout ' +test_expect_success 'basic refs completion' ' + touch file + git add file + git commit -m initial + test_completion git branch m master +' Hmm. I notice that Felipe's patch wraps the _whole_ input to __gitcomp_nl in single quotes. Wasn't there a patch series that added tests for __gitcomp_nl to catch these issues? -- Felipe Contreras -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 01/14] Update directory listing API doc to match code
7c4c97c0ac turned the flags in struct dir_struct into a single bitfield variable, but forgot to update this document. Signed-off-by: Adam Spiers g...@adamspiers.org --- Documentation/technical/api-directory-listing.txt | 12 ++-- 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/Documentation/technical/api-directory-listing.txt b/Documentation/technical/api-directory-listing.txt index add6f43..0356d25 100644 --- a/Documentation/technical/api-directory-listing.txt +++ b/Documentation/technical/api-directory-listing.txt @@ -17,24 +17,24 @@ options are: The name of the file to be read in each directory for excluded files (typically `.gitignore`). -`collect_ignored`:: +`flags`:: - Include paths that are to be excluded in the result. + A bit-field of options: -`show_ignored`:: +`DIR_SHOW_IGNORED`::: The traversal is for finding just ignored files, not unignored files. -`show_other_directories`:: +`DIR_SHOW_OTHER_DIRECTORIES`::: Include a directory that is not tracked. -`hide_empty_directories`:: +`DIR_HIDE_EMPTY_DIRECTORIES`::: Do not include a directory that is not tracked and is empty. -`no_gitlinks`:: +`DIR_NO_GITLINKS`::: If set, recurse into a directory that looks like a git directory. Otherwise it is shown as a directory. -- 1.7.12.147.g6d168f4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 00/14] new git check-ignore sub-command
This is a re-vamp of my original check-ignore series, which aims to address all the feedback which was raised in the first round of reviews. The most notable changes are the CLI options and output formats as suggested by Junio and Nguyễn; now there are three levels of verbosity: --quiet, default, and --verbose. -z also now affects the output and so is now compatible with the --stdin optin. Some commits have been broken into smaller pieces to facilitate easier reviews, and based on an earlier discussion, three exclude functions have been given an 'is_' prefix to clarify their boolean nature. The helper functions extracted from these three now have more meaningful names rather than just a '_1' suffix. Other minor issues, such as inconsistent coding style, have been fixed, and the modification to the output text in add.c has been scrapped. It has been rebased on the latest master, and passed a full test run. Adam Spiers (14): Update directory listing API doc to match code Improve documentation and comments regarding directory traversal API Rename cryptic 'which' variable to more consistent name Rename path_excluded() to is_path_excluded() Rename excluded_from_list() to is_excluded_from_list() Rename excluded() to is_excluded() Refactor is_excluded_from_list() Refactor is_excluded() Refactor is_path_excluded() For each exclude pattern, store information about where it came from Refactor treat_gitlinks() Extract some useful pathspec handling code from builtin/add.c into a library Provide free_directory() for reclaiming dir_struct memory Add git-check-ignore sub-command .gitignore| 1 + Documentation/git-check-ignore.txt| 85 Documentation/gitignore.txt | 6 +- Documentation/technical/api-directory-listing.txt | 23 +- Makefile | 3 + attr.c| 2 +- builtin.h | 1 + builtin/add.c | 84 +--- builtin/check-ignore.c| 167 ++ builtin/clean.c | 2 +- builtin/ls-files.c| 5 +- command-list.txt | 1 + contrib/completion/git-completion.bash| 1 + dir.c | 191 +-- dir.h | 47 +- git.c | 1 + pathspec.c| 97 pathspec.h| 6 + t/t0007-ignores.sh| 587 ++ t/t9902-completion.sh | 24 +- unpack-trees.c| 10 +- 21 files changed, 1182 insertions(+), 162 deletions(-) create mode 100644 Documentation/git-check-ignore.txt create mode 100644 builtin/check-ignore.c create mode 100644 pathspec.c create mode 100644 pathspec.h create mode 100755 t/t0007-ignores.sh -- 1.7.12.147.g6d168f4 -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 05/14] Rename excluded_from_list() to is_excluded_from_list()
Continue adopting clearer names for exclude functions. This 'is_*'
naming pattern for functions returning booleans was discussed here:
http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204924
Signed-off-by: Adam Spiers g...@adamspiers.org
---
dir.c | 11 ++-
dir.h | 4 ++--
unpack-trees.c | 8 +---
3 files changed, 13 insertions(+), 10 deletions(-)
diff --git a/dir.c b/dir.c
index dad1582..8f262f6 100644
--- a/dir.c
+++ b/dir.c
@@ -514,9 +514,9 @@ static void prep_exclude(struct dir_struct *dir, const char
*base, int baselen)
/* Scan the list and let the last match determine the fate.
* Return 1 for exclude, 0 for include and -1 for undecided.
*/
-int excluded_from_list(const char *pathname,
- int pathlen, const char *basename, int *dtype,
- struct exclude_list *el)
+int is_excluded_from_list(const char *pathname,
+ int pathlen, const char *basename, int *dtype,
+ struct exclude_list *el)
{
int i;
@@ -596,8 +596,9 @@ static int excluded(struct dir_struct *dir, const char
*pathname, int *dtype_p)
prep_exclude(dir, pathname, basename-pathname);
for (st = EXC_CMDL; st = EXC_FILE; st++) {
- switch (excluded_from_list(pathname, pathlen, basename,
- dtype_p, dir-exclude_list[st])) {
+ switch (is_excluded_from_list(pathname, pathlen,
+ basename, dtype_p,
+ dir-exclude_list[st])) {
case 0:
return 0;
case 1:
diff --git a/dir.h b/dir.h
index 41a5e32..6904a35 100644
--- a/dir.h
+++ b/dir.h
@@ -98,8 +98,8 @@ extern int within_depth(const char *name, int namelen, int
depth, int max_depth)
extern int fill_directory(struct dir_struct *dir, const char **pathspec);
extern int read_directory(struct dir_struct *, const char *path, int len,
const char **pathspec);
-extern int excluded_from_list(const char *pathname, int pathlen, const char
*basename,
- int *dtype, struct exclude_list *el);
+extern int is_excluded_from_list(const char *pathname, int pathlen, const char
*basename,
+int *dtype, struct exclude_list *el);
struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char
*pathname, int len);
/*
diff --git a/unpack-trees.c b/unpack-trees.c
index 724f69b..56127c9 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -837,7 +837,8 @@ static int clear_ce_flags_dir(struct cache_entry **cache,
int nr,
{
struct cache_entry **cache_end;
int dtype = DT_DIR;
- int ret = excluded_from_list(prefix, prefix_len, basename, dtype, el);
+ int ret = is_excluded_from_list(prefix, prefix_len,
+ basename, dtype, el);
prefix[prefix_len++] = '/';
@@ -856,7 +857,7 @@ static int clear_ce_flags_dir(struct cache_entry **cache,
int nr,
* with ret (iow, we know in advance the incl/excl
* decision for the entire directory), clear flag here without
* calling clear_ce_flags_1(). That function will call
-* the expensive excluded_from_list() on every entry.
+* the expensive is_excluded_from_list() on every entry.
*/
return clear_ce_flags_1(cache, cache_end - cache,
prefix, prefix_len,
@@ -939,7 +940,8 @@ static int clear_ce_flags_1(struct cache_entry **cache, int
nr,
/* Non-directory */
dtype = ce_to_dtype(ce);
- ret = excluded_from_list(ce-name, ce_namelen(ce), name,
dtype, el);
+ ret = is_excluded_from_list(ce-name, ce_namelen(ce),
+ name, dtype, el);
if (ret 0)
ret = defval;
if (ret 0)
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 06/14] Rename excluded() to is_excluded()
Continue adopting clearer names for exclude functions. This is_*
naming pattern for functions returning booleans was discussed here:
http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204924
Signed-off-by: Adam Spiers g...@adamspiers.org
---
attr.c | 2 +-
dir.c | 10 +-
dir.h | 4 ++--
3 files changed, 8 insertions(+), 8 deletions(-)
diff --git a/attr.c b/attr.c
index 887a9ae..fd33ff9 100644
--- a/attr.c
+++ b/attr.c
@@ -270,7 +270,7 @@ static struct match_attr *parse_attr_line(const char *line,
const char *src,
* (reading the file from top to bottom), .gitattribute of the root
* directory (again, reading the file from top to bottom) down to the
* current directory, and then scan the list backwards to find the first match.
- * This is exactly the same as what excluded() does in dir.c to deal with
+ * This is exactly the same as what is_excluded() does in dir.c to deal with
* .gitignore
*/
diff --git a/dir.c b/dir.c
index 8f262f6..b381b1b 100644
--- a/dir.c
+++ b/dir.c
@@ -587,7 +587,7 @@ int is_excluded_from_list(const char *pathname,
return -1; /* undecided */
}
-static int excluded(struct dir_struct *dir, const char *pathname, int *dtype_p)
+static int is_excluded(struct dir_struct *dir, const char *pathname, int
*dtype_p)
{
int pathlen = strlen(pathname);
int st;
@@ -637,7 +637,7 @@ int is_path_excluded(struct path_exclude_check *check,
/*
* we allow the caller to pass namelen as an optimization; it
* must match the length of the name, as we eventually call
-* excluded() on the whole name string.
+* is_excluded() on the whole name string.
*/
if (namelen 0)
namelen = strlen(name);
@@ -654,7 +654,7 @@ int is_path_excluded(struct path_exclude_check *check,
if (ch == '/') {
int dt = DT_DIR;
- if (excluded(check-dir, path-buf, dt))
+ if (is_excluded(check-dir, path-buf, dt))
return 1;
}
strbuf_addch(path, ch);
@@ -663,7 +663,7 @@ int is_path_excluded(struct path_exclude_check *check,
/* An entry in the index; cannot be a directory with subentries */
strbuf_setlen(path, 0);
- return excluded(check-dir, name, dtype);
+ return is_excluded(check-dir, name, dtype);
}
static struct dir_entry *dir_entry_new(const char *pathname, int len)
@@ -963,7 +963,7 @@ static enum path_treatment treat_one_path(struct dir_struct
*dir,
const struct path_simplify *simplify,
int dtype, struct dirent *de)
{
- int exclude = excluded(dir, path-buf, dtype);
+ int exclude = is_excluded(dir, path-buf, dtype);
if (exclude (dir-flags DIR_COLLECT_IGNORED)
exclude_matches_pathspec(path-buf, path-len, simplify))
dir_add_ignored(dir, path-buf, path-len);
diff --git a/dir.h b/dir.h
index 6904a35..009c9df 100644
--- a/dir.h
+++ b/dir.h
@@ -103,8 +103,8 @@ extern int is_excluded_from_list(const char *pathname, int
pathlen, const char *
struct dir_entry *dir_add_ignored(struct dir_struct *dir, const char
*pathname, int len);
/*
- * The excluded() API is meant for callers that check each level of leading
- * directory hierarchies with excluded() to avoid recursing into excluded
+ * The is_excluded() API is meant for callers that check each level of leading
+ * directory hierarchies with is_excluded() to avoid recursing into excluded
* directories. Callers that do not do so should use this API instead.
*/
struct path_exclude_check {
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 07/14] Refactor is_excluded_from_list()
The excluded function uses a new helper function called
last_exclude_matching_from_list() to perform the inner loop over all of
the exclude patterns. The helper just tells us whether the path is
included, excluded, or undecided.
However, it may be useful to know _which_ pattern was triggered. So
let's pass out the entire exclude match, which contains the status
information we were already passing out.
Further patches can make use of this.
This is a modified forward port of a patch from 2009 by Jeff King:
http://article.gmane.org/gmane.comp.version-control.git/108815
Signed-off-by: Adam Spiers g...@adamspiers.org
---
dir.c | 41 ++---
1 file changed, 30 insertions(+), 11 deletions(-)
diff --git a/dir.c b/dir.c
index b381b1b..70b7d7e 100644
--- a/dir.c
+++ b/dir.c
@@ -511,22 +511,26 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
dir-basebuf[baselen] = '\0';
}
-/* Scan the list and let the last match determine the fate.
- * Return 1 for exclude, 0 for include and -1 for undecided.
+/*
+ * Scan the given exclude list in reverse to see whether pathname
+ * should be ignored. The first match (i.e. the last on the list), if
+ * any, determines the fate. Returns the exclude_list element which
+ * matched, or NULL for undecided.
*/
-int is_excluded_from_list(const char *pathname,
- int pathlen, const char *basename, int *dtype,
- struct exclude_list *el)
+static struct exclude *last_exclude_matching_from_list(const char *pathname,
+ int pathlen,
+ const char *basename,
+ int *dtype,
+ struct exclude_list *el)
{
int i;
if (!el-nr)
- return -1; /* undefined */
+ return NULL;/* undefined */
for (i = el-nr - 1; 0 = i; i--) {
struct exclude *x = el-excludes[i];
const char *name, *exclude = x-pattern;
- int to_exclude = x-to_exclude;
int namelen, prefix = x-nowildcardlen;
if (x-flags EXC_FLAG_MUSTBEDIR) {
@@ -540,14 +544,14 @@ int is_excluded_from_list(const char *pathname,
/* match basename */
if (prefix == x-patternlen) {
if (!strcmp_icase(exclude, basename))
- return to_exclude;
+ return x;
} else if (x-flags EXC_FLAG_ENDSWITH) {
if (x-patternlen - 1 = pathlen
!strcmp_icase(exclude + 1, pathname +
pathlen - x-patternlen + 1))
- return to_exclude;
+ return x;
} else {
if (fnmatch_icase(exclude, basename, 0) == 0)
- return to_exclude;
+ return x;
}
continue;
}
@@ -582,8 +586,23 @@ int is_excluded_from_list(const char *pathname,
}
if (!namelen || !fnmatch_icase(exclude, name, FNM_PATHNAME))
- return to_exclude;
+ return x;
}
+ return NULL; /* undecided */
+}
+
+/*
+ * Scan the list and let the last match determine the fate.
+ * Return 1 for exclude, 0 for include and -1 for undecided.
+ */
+int is_excluded_from_list(const char *pathname,
+ int pathlen, const char *basename, int *dtype,
+ struct exclude_list *el)
+{
+ struct exclude *exclude;
+ exclude = last_exclude_matching_from_list(pathname, pathlen, basename,
dtype, el);
+ if (exclude)
+ return exclude-to_exclude;
return -1; /* undecided */
}
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 09/14] Refactor is_path_excluded()
In a similar way to the previous commit, this extracts a new helper
function last_exclude_matching_path() which return the last
exclude_list element which matched, or NULL if no match was found.
is_path_excluded() becomes a wrapper around this, and just returns 0
or 1 depending on whether any matching exclude_list element was found.
This allows callers to find out _why_ a given path was excluded,
rather than just whether it was or not, paving the way for a new git
sub-command which allows users to test their exclude lists from the
command line.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
dir.c | 47 ++-
dir.h | 3 +++
2 files changed, 41 insertions(+), 9 deletions(-)
diff --git a/dir.c b/dir.c
index 3e2161e..904a7f3 100644
--- a/dir.c
+++ b/dir.c
@@ -651,6 +651,7 @@ void path_exclude_check_init(struct path_exclude_check
*check,
struct dir_struct *dir)
{
check-dir = dir;
+ check-exclude = NULL;
strbuf_init(check-path, 256);
}
@@ -660,18 +661,21 @@ void path_exclude_check_clear(struct path_exclude_check
*check)
}
/*
- * Is this name excluded? This is for a caller like show_files() that
- * do not honor directory hierarchy and iterate through paths that are
- * possibly in an ignored directory.
+ * For each subdirectory in name, starting with the top-most, checks
+ * to see if that subdirectory is excluded, and if so, returns the
+ * corresponding exclude structure. Otherwise, checks whether name
+ * itself (which is presumably a file) is excluded.
*
* A path to a directory known to be excluded is left in check-path to
* optimize for repeated checks for files in the same excluded directory.
*/
-int is_path_excluded(struct path_exclude_check *check,
-const char *name, int namelen, int *dtype)
+struct exclude *last_exclude_matching_path(struct path_exclude_check *check,
+ const char *name, int namelen,
+ int *dtype)
{
int i;
struct strbuf *path = check-path;
+ struct exclude *exclude;
/*
* we allow the caller to pass namelen as an optimization; it
@@ -681,11 +685,17 @@ int is_path_excluded(struct path_exclude_check *check,
if (namelen 0)
namelen = strlen(name);
+ /*
+* If path is non-empty, and name is equal to path or a
+* subdirectory of path, name should be excluded, because
+* it's inside a directory which is already known to be
+* excluded and was previously left in check-path.
+*/
if (path-len
path-len = namelen
!memcmp(name, path-buf, path-len)
(!name[path-len] || name[path-len] == '/'))
- return 1;
+ return check-exclude;
strbuf_setlen(path, 0);
for (i = 0; name[i]; i++) {
@@ -693,8 +703,12 @@ int is_path_excluded(struct path_exclude_check *check,
if (ch == '/') {
int dt = DT_DIR;
- if (is_excluded(check-dir, path-buf, dt))
- return 1;
+ exclude = last_exclude_matching(check-dir,
+ path-buf, dt);
+ if (exclude) {
+ check-exclude = exclude;
+ return exclude;
+ }
}
strbuf_addch(path, ch);
}
@@ -702,7 +716,22 @@ int is_path_excluded(struct path_exclude_check *check,
/* An entry in the index; cannot be a directory with subentries */
strbuf_setlen(path, 0);
- return is_excluded(check-dir, name, dtype);
+ return last_exclude_matching(check-dir, name, dtype);
+}
+
+/*
+ * Is this name excluded? This is for a caller like show_files() that
+ * do not honor directory hierarchy and iterate through paths that are
+ * possibly in an ignored directory.
+ */
+int is_path_excluded(struct path_exclude_check *check,
+ const char *name, int namelen, int *dtype)
+{
+ struct exclude *exclude =
+ last_exclude_matching_path(check, name, namelen, dtype);
+ if (exclude)
+ return exclude-to_exclude;
+ return 0;
}
static struct dir_entry *dir_entry_new(const char *pathname, int len)
diff --git a/dir.h b/dir.h
index 009c9df..19beddb 100644
--- a/dir.h
+++ b/dir.h
@@ -109,10 +109,13 @@ struct dir_entry *dir_add_ignored(struct dir_struct *dir,
const char *pathname,
*/
struct path_exclude_check {
struct dir_struct *dir;
+ struct exclude *exclude;
struct strbuf path;
};
extern void path_exclude_check_init(struct path_exclude_check *, struct
dir_struct *);
extern void path_exclude_check_clear(struct path_exclude_check *);
+extern struct exclude
[PATCH v2 08/14] Refactor is_excluded()
In a similar way to the previous commit, this extracts a new helper
function last_exclude_matching() which returns the last exclude_list
element which matched, or NULL if no match was found. is_excluded()
becomes a wrapper around this, and just returns 0 or 1 depending on
whether any matching exclude_list element was found.
This allows callers to find out _why_ a given path was excluded,
rather than just whether it was or not, paving the way for a new git
sub-command which allows users to test their exclude lists from the
command line.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
dir.c | 38 +-
1 file changed, 29 insertions(+), 9 deletions(-)
diff --git a/dir.c b/dir.c
index 70b7d7e..3e2161e 100644
--- a/dir.c
+++ b/dir.c
@@ -606,24 +606,44 @@ int is_excluded_from_list(const char *pathname,
return -1; /* undecided */
}
-static int is_excluded(struct dir_struct *dir, const char *pathname, int
*dtype_p)
+/*
+ * Loads the exclude lists for the directory containing pathname, then
+ * scans all exclude lists to determine whether pathname is excluded.
+ * Returns the exclude_list element which matched, or NULL for
+ * undecided.
+ */
+static struct exclude *last_exclude_matching(struct dir_struct *dir,
+const char *pathname,
+int *dtype_p)
{
int pathlen = strlen(pathname);
int st;
+ struct exclude *exclude;
const char *basename = strrchr(pathname, '/');
basename = (basename) ? basename+1 : pathname;
prep_exclude(dir, pathname, basename-pathname);
for (st = EXC_CMDL; st = EXC_FILE; st++) {
- switch (is_excluded_from_list(pathname, pathlen,
- basename, dtype_p,
- dir-exclude_list[st])) {
- case 0:
- return 0;
- case 1:
- return 1;
- }
+ exclude = last_exclude_matching_from_list(
+ pathname, pathlen, basename, dtype_p,
+ dir-exclude_list[st]);
+ if (exclude)
+ return exclude;
}
+ return NULL;
+}
+
+/*
+ * Loads the exclude lists for the directory containing pathname, then
+ * scans all exclude lists to determine whether pathname is excluded.
+ * Returns 1 if true, otherwise 0.
+ */
+static int is_excluded(struct dir_struct *dir, const char *pathname, int
*dtype_p)
+{
+ struct exclude *exclude =
+ last_exclude_matching(dir, pathname, dtype_p);
+ if (exclude)
+ return exclude-to_exclude;
return 0;
}
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 10/14] For each exclude pattern, store information about where it came from
For exclude patterns read in from files, the filename is stored together
with the corresponding line number (counting starting at 1).
For exclude patterns provided on the command line, the sequence number
is negative, with counting starting at -1, so for example the 2nd
pattern provided via --exclude would be numbered -2. This allows any
future consumers of that data to easily distinguish between exclude
patterns from files vs. from the CLI.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
builtin/clean.c| 2 +-
builtin/ls-files.c | 3 ++-
dir.c | 25 +++--
dir.h | 5 -
4 files changed, 26 insertions(+), 9 deletions(-)
diff --git a/builtin/clean.c b/builtin/clean.c
index 69c1cda..893dd7a 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -99,7 +99,7 @@ int cmd_clean(int argc, const char **argv, const char *prefix)
for (i = 0; i exclude_list.nr; i++)
add_exclude(exclude_list.items[i].string, , 0,
- dir.exclude_list[EXC_CMDL]);
+ dir.exclude_list[EXC_CMDL], --exclude option,
-(i+1));
pathspec = get_pathspec(prefix, argv);
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index d226456..64b1969 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -35,6 +35,7 @@ static int error_unmatch;
static char *ps_matched;
static const char *with_tree;
static int exc_given;
+static int exclude_args;
static const char *tag_cached = ;
static const char *tag_unmerged = ;
@@ -423,7 +424,7 @@ static int option_parse_exclude(const struct option *opt,
struct exclude_list *list = opt-value;
exc_given = 1;
- add_exclude(arg, , 0, list);
+ add_exclude(arg, , 0, list, --exclude option, --exclude_args);
return 0;
}
diff --git a/dir.c b/dir.c
index 904a7f3..705546f 100644
--- a/dir.c
+++ b/dir.c
@@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
}
void add_exclude(const char *string, const char *base,
-int baselen, struct exclude_list *el)
+int baselen, struct exclude_list *el, const char *src, int
srcpos)
{
struct exclude *x;
size_t len;
@@ -341,6 +341,8 @@ void add_exclude(const char *string, const char *base,
x-base = base;
x-baselen = baselen;
x-flags = flags;
+ x-src = src;
+ x-srcpos = srcpos;
if (!strchr(string, '/'))
x-flags |= EXC_FLAG_NODIR;
x-nowildcardlen = simple_length(string);
@@ -393,7 +395,7 @@ int add_excludes_from_file_to_list(const char *fname,
int check_index)
{
struct stat st;
- int fd, i;
+ int fd, i, lineno = 1;
size_t size = 0;
char *buf, *entry;
@@ -438,8 +440,9 @@ int add_excludes_from_file_to_list(const char *fname,
if (buf[i] == '\n') {
if (entry != buf + i entry[0] != '#') {
buf[i - (i buf[i-1] == '\r')] = 0;
- add_exclude(entry, base, baselen, el);
+ add_exclude(entry, base, baselen, el, fname,
lineno);
}
+ lineno++;
entry = buf + i + 1;
}
}
@@ -474,8 +477,10 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
!strncmp(dir-basebuf, base, stk-baselen))
break;
dir-exclude_stack = stk-prev;
- while (stk-exclude_ix el-nr)
- free(el-excludes[--el-nr]);
+ while (stk-exclude_ix el-nr) {
+ struct exclude *exclude = el-excludes[--el-nr];
+ free(exclude);
+ }
free(stk-filebuf);
free(stk);
}
@@ -502,7 +507,15 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
memcpy(dir-basebuf + current, base + current,
stk-baselen - current);
strcpy(dir-basebuf + stk-baselen, dir-exclude_per_dir);
- add_excludes_from_file_to_list(dir-basebuf,
+
+ /* dir-basebuf gets reused by the traversal, but we
+* need fname to remain unchanged to ensure the src
+* member of each struct exclude correctly back-references
+* its source file.
+*/
+ char *fname = strdup(dir-basebuf);
+
+ add_excludes_from_file_to_list(fname,
dir-basebuf, stk-baselen,
stk-filebuf, el, 1);
dir-exclude_stack = stk;
diff --git a/dir.h b/dir.h
index 19beddb..ebb0367 100644
--- a/dir.h
+++ b/dir.h
@@ -31,6 +31,9 @@ struct exclude_list {
int baselen;
[PATCH v2 11/14] Refactor treat_gitlinks()
Extract the body of the for loop in treat_gitlinks() into a separate
treat_gitlink() function so that it can be reused elsewhere. This
paves the way for a new check-ignore sub-command.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
builtin/add.c | 49 +++--
1 file changed, 31 insertions(+), 18 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index 075312a..b4ec5cd 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -153,31 +153,44 @@ static char *prune_directory(struct dir_struct *dir,
const char **pathspec, int
return seen;
}
-static void treat_gitlinks(const char **pathspec)
+/*
+ * Check whether path refers to a submodule, or something inside a
+ * submodule. If the former, returns the path with any trailing slash
+ * stripped. If the latter, dies with an error message.
+ */
+const char *treat_gitlink(const char *path)
{
- int i;
-
- if (!pathspec || !*pathspec)
- return;
-
+ int i, path_len = strlen(path);
for (i = 0; i active_nr; i++) {
struct cache_entry *ce = active_cache[i];
if (S_ISGITLINK(ce-ce_mode)) {
- int len = ce_namelen(ce), j;
- for (j = 0; pathspec[j]; j++) {
- int len2 = strlen(pathspec[j]);
- if (len2 = len || pathspec[j][len] != '/' ||
- memcmp(ce-name, pathspec[j], len))
- continue;
- if (len2 == len + 1)
- /* strip trailing slash */
- pathspec[j] = xstrndup(ce-name, len);
- else
- die (_(Path '%s' is in submodule
'%.*s'),
- pathspec[j], len, ce-name);
+ int ce_len = ce_namelen(ce);
+ if (path_len = ce_len || path[ce_len] != '/' ||
+ memcmp(ce-name, path, ce_len))
+ /* path does not refer to this
+* submodule or anything inside it */
+ continue;
+ if (path_len == ce_len + 1) {
+ /* path refers to submodule;
+* strip trailing slash */
+ return xstrndup(ce-name, ce_len);
+ } else {
+ die (_(Path '%s' is in submodule '%.*s'),
+path, ce_len, ce-name);
}
}
}
+ return path;
+}
+
+void treat_gitlinks(const char **pathspec)
+{
+ if (!pathspec || !*pathspec)
+ return;
+
+ int i;
+ for (i = 0; pathspec[i]; i++)
+ pathspec[i] = treat_gitlink(pathspec[i]);
}
static void refresh(int verbose, const char **pathspec)
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 12/14] Extract some useful pathspec handling code from builtin/add.c into a library
This is in preparation for reuse by a new git check-ignore command.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
Makefile | 2 ++
builtin/add.c | 95 ++---
pathspec.c| 97 +++
pathspec.h| 6
4 files changed, 108 insertions(+), 92 deletions(-)
create mode 100644 pathspec.c
create mode 100644 pathspec.h
diff --git a/Makefile b/Makefile
index a49d1db..9a4bf9e 100644
--- a/Makefile
+++ b/Makefile
@@ -649,6 +649,7 @@ LIB_H += pack-revindex.h
LIB_H += pack.h
LIB_H += parse-options.h
LIB_H += patch-ids.h
+LIB_H += pathspec.h
LIB_H += pkt-line.h
LIB_H += progress.h
LIB_H += prompt.h
@@ -769,6 +770,7 @@ LIB_OBJS += parse-options-cb.o
LIB_OBJS += patch-delta.o
LIB_OBJS += patch-ids.o
LIB_OBJS += path.o
+LIB_OBJS += pathspec.o
LIB_OBJS += pkt-line.o
LIB_OBJS += preload-index.o
LIB_OBJS += pretty.o
diff --git a/builtin/add.c b/builtin/add.c
index b4ec5cd..c3def9c 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -6,6 +6,7 @@
#include cache.h
#include builtin.h
#include dir.h
+#include pathspec.h
#include exec_cmd.h
#include cache-tree.h
#include run-command.h
@@ -97,39 +98,6 @@ int add_files_to_cache(const char *prefix, const char
**pathspec, int flags)
return !!data.add_errors;
}
-static void fill_pathspec_matches(const char **pathspec, char *seen, int specs)
-{
- int num_unmatched = 0, i;
-
- /*
-* Since we are walking the index as if we were walking the directory,
-* we have to mark the matched pathspec as seen; otherwise we will
-* mistakenly think that the user gave a pathspec that did not match
-* anything.
-*/
- for (i = 0; i specs; i++)
- if (!seen[i])
- num_unmatched++;
- if (!num_unmatched)
- return;
- for (i = 0; i active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
- match_pathspec(pathspec, ce-name, ce_namelen(ce), 0, seen);
- }
-}
-
-static char *find_used_pathspec(const char **pathspec)
-{
- char *seen;
- int i;
-
- for (i = 0; pathspec[i]; i++)
- ; /* just counting */
- seen = xcalloc(i, 1);
- fill_pathspec_matches(pathspec, seen, i);
- return seen;
-}
-
static char *prune_directory(struct dir_struct *dir, const char **pathspec,
int prefix)
{
char *seen;
@@ -153,46 +121,6 @@ static char *prune_directory(struct dir_struct *dir, const
char **pathspec, int
return seen;
}
-/*
- * Check whether path refers to a submodule, or something inside a
- * submodule. If the former, returns the path with any trailing slash
- * stripped. If the latter, dies with an error message.
- */
-const char *treat_gitlink(const char *path)
-{
- int i, path_len = strlen(path);
- for (i = 0; i active_nr; i++) {
- struct cache_entry *ce = active_cache[i];
- if (S_ISGITLINK(ce-ce_mode)) {
- int ce_len = ce_namelen(ce);
- if (path_len = ce_len || path[ce_len] != '/' ||
- memcmp(ce-name, path, ce_len))
- /* path does not refer to this
-* submodule or anything inside it */
- continue;
- if (path_len == ce_len + 1) {
- /* path refers to submodule;
-* strip trailing slash */
- return xstrndup(ce-name, ce_len);
- } else {
- die (_(Path '%s' is in submodule '%.*s'),
-path, ce_len, ce-name);
- }
- }
- }
- return path;
-}
-
-void treat_gitlinks(const char **pathspec)
-{
- if (!pathspec || !*pathspec)
- return;
-
- int i;
- for (i = 0; pathspec[i]; i++)
- pathspec[i] = treat_gitlink(pathspec[i]);
-}
-
static void refresh(int verbose, const char **pathspec)
{
char *seen;
@@ -210,23 +138,6 @@ static void refresh(int verbose, const char **pathspec)
free(seen);
}
-static const char **validate_pathspec(int argc, const char **argv, const char
*prefix)
-{
- const char **pathspec = get_pathspec(prefix, argv);
-
- if (pathspec) {
- const char **p;
- for (p = pathspec; *p; p++) {
- if (has_symlink_leading_path(*p, strlen(*p))) {
- int len = prefix ? strlen(prefix) : 0;
- die(_('%s' is beyond a symbolic link), *p +
len);
- }
- }
- }
-
- return pathspec;
-}
-
int run_add_interactive(const char *revision, const char *patch_mode,
const char
[PATCH v2 03/14] Rename cryptic 'which' variable to more consistent name
'el' is only *slightly* less cryptic, but is already used as the
variable name for a struct exclude_list pointer in numerous other
places, so this reduces the number of cryptic variable names in use by
one :-)
Signed-off-by: Adam Spiers g...@adamspiers.org
---
dir.c | 10 +-
dir.h | 4 ++--
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/dir.c b/dir.c
index 98d1995..91e57d9 100644
--- a/dir.c
+++ b/dir.c
@@ -311,7 +311,7 @@ static int no_wildcard(const char *string)
}
void add_exclude(const char *string, const char *base,
-int baselen, struct exclude_list *which)
+int baselen, struct exclude_list *el)
{
struct exclude *x;
size_t len;
@@ -346,8 +346,8 @@ void add_exclude(const char *string, const char *base,
x-nowildcardlen = simple_length(string);
if (*string == '*' no_wildcard(string+1))
x-flags |= EXC_FLAG_ENDSWITH;
- ALLOC_GROW(which-excludes, which-nr + 1, which-alloc);
- which-excludes[which-nr++] = x;
+ ALLOC_GROW(el-excludes, el-nr + 1, el-alloc);
+ el-excludes[el-nr++] = x;
}
static void *read_skip_worktree_file_from_index(const char *path, size_t *size)
@@ -389,7 +389,7 @@ int add_excludes_from_file_to_list(const char *fname,
const char *base,
int baselen,
char **buf_p,
- struct exclude_list *which,
+ struct exclude_list *el,
int check_index)
{
struct stat st;
@@ -438,7 +438,7 @@ int add_excludes_from_file_to_list(const char *fname,
if (buf[i] == '\n') {
if (entry != buf + i entry[0] != '#') {
buf[i - (i buf[i-1] == '\r')] = 0;
- add_exclude(entry, base, baselen, which);
+ add_exclude(entry, base, baselen, el);
}
entry = buf + i + 1;
}
diff --git a/dir.h b/dir.h
index a226fbc..549a187 100644
--- a/dir.h
+++ b/dir.h
@@ -117,10 +117,10 @@ extern int path_excluded(struct path_exclude_check *,
const char *, int namelen,
extern int add_excludes_from_file_to_list(const char *fname, const char *base,
int baselen,
- char **buf_p, struct exclude_list
*which, int check_index);
+ char **buf_p, struct exclude_list
*el, int check_index);
extern void add_excludes_from_file(struct dir_struct *, const char *fname);
extern void add_exclude(const char *string, const char *base,
- int baselen, struct exclude_list *which);
+ int baselen, struct exclude_list *el);
extern void free_excludes(struct exclude_list *el);
extern int file_exists(const char *);
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 13/14] Provide free_directory() for reclaiming dir_struct memory
Signed-off-by: Adam Spiers g...@adamspiers.org
---
Documentation/technical/api-directory-listing.txt | 2 ++
dir.c | 23 +--
dir.h | 1 +
3 files changed, 24 insertions(+), 2 deletions(-)
diff --git a/Documentation/technical/api-directory-listing.txt
b/Documentation/technical/api-directory-listing.txt
index 944fc39..e339c18 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -79,4 +79,6 @@ marked. If you to exclude files, make sure you have loaded
index first.
* Use `dir.entries[]`.
+* Call `free_directory()` when none of the contained elements are no longer in
use.
+
(JC)
diff --git a/dir.c b/dir.c
index 705546f..a04739c 100644
--- a/dir.c
+++ b/dir.c
@@ -456,6 +456,12 @@ void add_excludes_from_file(struct dir_struct *dir, const
char *fname)
die(cannot use %s as an exclude file, fname);
}
+static void free_exclude_stack(struct exclude_stack *stk)
+{
+ free(stk-filebuf);
+ free(stk);
+}
+
/*
* Loads the per-directory exclude list for the substring of base
* which has a char length of baselen.
@@ -481,8 +487,7 @@ static void prep_exclude(struct dir_struct *dir, const char
*base, int baselen)
struct exclude *exclude = el-excludes[--el-nr];
free(exclude);
}
- free(stk-filebuf);
- free(stk);
+ free_exclude_stack(stk);
}
/* Read from the parent directories and push them down. */
@@ -1473,3 +1478,17 @@ void free_pathspec(struct pathspec *pathspec)
free(pathspec-items);
pathspec-items = NULL;
}
+
+void free_directory(struct dir_struct *dir)
+{
+ int st;
+ for (st = EXC_CMDL; st = EXC_FILE; st++)
+ free_excludes(dir-exclude_list[st]);
+
+ struct exclude_stack *prev, *stk = dir-exclude_stack;
+ while (stk) {
+ prev = stk-prev;
+ free_exclude_stack(stk);
+ stk = prev;
+ }
+}
diff --git a/dir.h b/dir.h
index ebb0367..7da29da 100644
--- a/dir.h
+++ b/dir.h
@@ -128,6 +128,7 @@ extern void add_excludes_from_file(struct dir_struct *,
const char *fname);
extern void add_exclude(const char *string, const char *base,
int baselen, struct exclude_list *el, const char *src,
int srcpos);
extern void free_excludes(struct exclude_list *el);
+extern void free_directory(struct dir_struct *dir);
extern int file_exists(const char *);
extern int is_inside_dir(const char *dir);
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 04/14] Rename path_excluded() to is_path_excluded()
Start adopting clearer names for exclude functions. This 'is_*'
naming pattern for functions returning booleans was agreed here:
http://thread.gmane.org/gmane.comp.version-control.git/204661/focus=204924
Signed-off-by: Adam Spiers g...@adamspiers.org
---
builtin/add.c | 2 +-
builtin/ls-files.c | 2 +-
dir.c | 4 ++--
dir.h | 2 +-
unpack-trees.c | 2 +-
5 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/builtin/add.c b/builtin/add.c
index e664100..075312a 100644
--- a/builtin/add.c
+++ b/builtin/add.c
@@ -454,7 +454,7 @@ int cmd_add(int argc, const char **argv, const char *prefix)
!file_exists(pathspec[i])) {
if (ignore_missing) {
int dtype = DT_UNKNOWN;
- if (path_excluded(check, pathspec[i],
-1, dtype))
+ if (is_path_excluded(check,
pathspec[i], -1, dtype))
dir_add_ignored(dir,
pathspec[i], strlen(pathspec[i]));
} else
die(_(pathspec '%s' did not match any
files),
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index b5434af..d226456 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -203,7 +203,7 @@ static void show_ru_info(void)
static int ce_excluded(struct path_exclude_check *check, struct cache_entry
*ce)
{
int dtype = ce_to_dtype(ce);
- return path_excluded(check, ce-name, ce_namelen(ce), dtype);
+ return is_path_excluded(check, ce-name, ce_namelen(ce), dtype);
}
static void show_files(struct dir_struct *dir)
diff --git a/dir.c b/dir.c
index 91e57d9..dad1582 100644
--- a/dir.c
+++ b/dir.c
@@ -627,8 +627,8 @@ void path_exclude_check_clear(struct path_exclude_check
*check)
* A path to a directory known to be excluded is left in check-path to
* optimize for repeated checks for files in the same excluded directory.
*/
-int path_excluded(struct path_exclude_check *check,
- const char *name, int namelen, int *dtype)
+int is_path_excluded(struct path_exclude_check *check,
+const char *name, int namelen, int *dtype)
{
int i;
struct strbuf *path = check-path;
diff --git a/dir.h b/dir.h
index 549a187..41a5e32 100644
--- a/dir.h
+++ b/dir.h
@@ -113,7 +113,7 @@ struct path_exclude_check {
};
extern void path_exclude_check_init(struct path_exclude_check *, struct
dir_struct *);
extern void path_exclude_check_clear(struct path_exclude_check *);
-extern int path_excluded(struct path_exclude_check *, const char *, int
namelen, int *dtype);
+extern int is_path_excluded(struct path_exclude_check *, const char *, int
namelen, int *dtype);
extern int add_excludes_from_file_to_list(const char *fname, const char *base,
int baselen,
diff --git a/unpack-trees.c b/unpack-trees.c
index 6d96366..724f69b 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1373,7 +1373,7 @@ static int check_ok_to_remove(const char *name, int len,
int dtype,
return 0;
if (o-dir
- path_excluded(o-path_exclude_check, name, -1, dtype))
+ is_path_excluded(o-path_exclude_check, name, -1, dtype))
/*
* ce-name is explicitly excluded, so it is Ok to
* overwrite it.
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2 02/14] Improve documentation and comments regarding directory traversal API
From the perspective of a newcomer to the codebase, the directory
traversal API has a few potentially confusing properties. These
comments clarify a few key aspects and will hopefully make it easier
to understand for other newcomers in the future.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
Documentation/technical/api-directory-listing.txt | 9 +---
dir.c | 8 ++-
dir.h | 26 +--
3 files changed, 37 insertions(+), 6 deletions(-)
diff --git a/Documentation/technical/api-directory-listing.txt
b/Documentation/technical/api-directory-listing.txt
index 0356d25..944fc39 100644
--- a/Documentation/technical/api-directory-listing.txt
+++ b/Documentation/technical/api-directory-listing.txt
@@ -9,8 +9,11 @@ Data structure
--
`struct dir_struct` structure is used to pass directory traversal
-options to the library and to record the paths discovered. The notable
-options are:
+options to the library and to record the paths discovered. A single
+`struct dir_struct` is used regardless of whether or not the traversal
+recursively descends into subdirectories.
+
+The notable options are:
`exclude_per_dir`::
@@ -39,7 +42,7 @@ options are:
If set, recurse into a directory that looks like a git
directory. Otherwise it is shown as a directory.
-The result of the enumeration is left in these fields::
+The result of the enumeration is left in these fields:
`entries[]`::
diff --git a/dir.c b/dir.c
index 4868339..98d1995 100644
--- a/dir.c
+++ b/dir.c
@@ -2,6 +2,8 @@
* This handles recursive filename detection with exclude
* files, index knowledge etc..
*
+ * See Documentation/technical/api-directory-listing.txt
+ *
* Copyright (C) Linus Torvalds, 2005-2006
* Junio Hamano, 2005-2006
*/
@@ -451,6 +453,10 @@ void add_excludes_from_file(struct dir_struct *dir, const
char *fname)
die(cannot use %s as an exclude file, fname);
}
+/*
+ * Loads the per-directory exclude list for the substring of base
+ * which has a char length of baselen.
+ */
static void prep_exclude(struct dir_struct *dir, const char *base, int baselen)
{
struct exclude_list *el;
@@ -461,7 +467,7 @@ static void prep_exclude(struct dir_struct *dir, const char
*base, int baselen)
(baselen + strlen(dir-exclude_per_dir) = PATH_MAX))
return; /* too long a path -- ignore */
- /* Pop the ones that are not the prefix of the path being checked. */
+ /* Pop the directories that are not the prefix of the path being
checked. */
el = dir-exclude_list[EXC_DIRS];
while ((stk = dir-exclude_stack) != NULL) {
if (stk-baselen = baselen
diff --git a/dir.h b/dir.h
index 893465a..a226fbc 100644
--- a/dir.h
+++ b/dir.h
@@ -1,6 +1,8 @@
#ifndef DIR_H
#define DIR_H
+/* See Documentation/technical/api-directory-listing.txt */
+
#include strbuf.h
struct dir_entry {
@@ -12,6 +14,12 @@ struct dir_entry {
#define EXC_FLAG_ENDSWITH 4
#define EXC_FLAG_MUSTBEDIR 8
+/*
+ * Each .gitignore file will be parsed into patterns which are then
+ * appended to the relevant exclude_list (either EXC_DIRS or
+ * EXC_FILE). exclude_lists are also used to represent the list of
+ * --exclude values passed via CLI args (EXC_CMDL).
+ */
struct exclude_list {
int nr;
int alloc;
@@ -26,9 +34,15 @@ struct exclude_list {
} **excludes;
};
+/*
+ * The contents of the per-directory exclude files are lazily read on
+ * demand and then cached in memory, one per exclude_stack struct, in
+ * order to avoid opening and parsing each one every time that
+ * directory is traversed.
+ */
struct exclude_stack {
- struct exclude_stack *prev;
- char *filebuf;
+ struct exclude_stack *prev; /* the struct exclude_stack for the parent
directory */
+ char *filebuf; /* remember pointer to per-directory exclude file
contents so we can free() */
int baselen;
int exclude_ix;
};
@@ -59,6 +73,14 @@ struct dir_struct {
#define EXC_DIRS 1
#define EXC_FILE 2
+ /*
+* Temporary variables which are used during loading of the
+* per-directory exclude lists.
+*
+* exclude_stack points to the top of the exclude_stack, and
+* basebuf contains the full path to the current
+* (sub)directory in the traversal.
+*/
struct exclude_stack *exclude_stack;
char basebuf[PATH_MAX];
};
--
1.7.12.147.g6d168f4
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git diff across submodules
Am 20.09.2012 00:31, schrieb Junio C Hamano:
Junio C Hamano gits...@pobox.com writes:
I also suspect that you do not have to change git diff at all to
show the patch recursively by using the attribute mechanism (look in
Documentation/gitattributes.text for a string GIT_EXTERNAL_DIFF).
It might be just as simple as doing this:
echo .gitattributes /lib/frotz diff=subrecurse
git config diff.subrecurse.command $HOME/bin/diff-subrecurse
cat $HOME/bin/diff-subrecurse \-EOF
#!/bin/sh
path=$1 old_hex=$3 new_hex=$6
unset GIT_DIR
cd $path || exit 1
git diff $old_hex $new_hex
EOF
chmod +x $HOME/bin/diff-subrecurse
The corner cases like new submodule, removed submodule are left
as an exercise to the reader ;-)
It turns out that essentially the above outline I concocted in my
MUA is usable almost as-is.
Here is what I ended up with.
* In .git/config of the superproject, I added this:
[diff submodule-recurse]
command = src/bin/diff-submodule-recurse
* In the superproject, src/bin/diff-submodule-recurse has this
(this is probably whitespace damaged---the lines must be indented
by HT for the here document to correctly work):
#!/bin/sh
# $1 $2 $3 $4 $5 $6 $7
# path old-file old-hex old-mode new-file new-hex new-mode
case $#,$4,$7 in
7,16,16) ;;
*) echo diff --git a/$1 b/$1
echo (punt)
exit
;;
esac
unset GIT_DIR
cd $1 || {
cat -\EOF
diff --git a/$1 b/$1
(cannot chdir to $1)
-Subproject commit $3
+Subproject commit $6
EOF
}
git --no-pager diff --src-prefix=s/$1/ --dst-prefix=m/$1/ $3
$6
* In .gitattributes of the superproject, I have this:
/var diff=submodule-recurse
The superproject in this case is a repository to control what I have
in my $HOME directory (e.g. it has src/dot/Makefile that builds and
installs the appropriate dotfiles, src/bin/Makefile that builds and
installs to $HOME/bin, etc.), and one subdirectory, 'var', is a
submodule that is only cloned to some but not all machines I clone
this superproject to.
With this setting, things like
$ git diff HEAD~20
show differences with recursion into the var/ submodule just fine.
That's pretty cool! Even though diff options like --stat and --name-only
still won't take into account what happened inside the submodule this
approach makes it possible to see the diff recursively. Wouldn't it make
sense add this script to contrib (after teaching it new and removed
submodules and documenting its use in a few lines after the shebang)?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Documentation: Document signature showing options
Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 5/7] grep.c: mark private file-scope symbols as static
Junio C Hamano wrote: Ramsay Jones ram...@ramsay1.demon.co.uk writes: Heh, so I obviously didn't see this before sending the patch yesterday! :-D Yes, this solves the problem addressed by yesterday's patch, so please ignore that. However, this tickles sparse to complain as well ... ;-) New patch on it's way. Are you sure the patch you are responding to really tickles sparse? Yes. You have another grep.c patch timestamped two minutes after the message I am responding to, and as far as I can see, it is a subset of the patch you are responding to with the message I am responding to. Hmm, that sentence has too many twists for me! :-D Let me see if I can clear up the misunderstanding: - the patch from yesterday (18-09-2021) fixed a complaint from sparse regarding symbol 'dump_grep_expression'. This was before I had seen this patch email, or the resulting commit 07a7d656. Since my patch from yesterday is a strict subset of your patch, then your patch also fixes the complaint from sparse regarding the 'dump_grep_expression' symbol. - this patch (commit 07a7d656) causes sparse to complain about the symbols 'grep_source_load' and 'grep_source_is_binary'. The new patch from me (timestamped two minutes after ...) also titled grep.c: Fix some sparse warnings on 19-09-2012 at 7:04 PM, is *not* a subset of commit 07a7d656. This patch addresses the new sparse warnings regarding 'grep_source_load' and 'grep_source_is_binary'. Hopefully that addresses the confusion! ;-) HTH ATB, Ramsay Jones -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git diff across submodules
Jens Lehmann jens.lehm...@web.de writes:
That's pretty cool! Even though diff options like --stat and --name-only
still won't take into account what happened inside the submodule this
approach makes it possible to see the diff recursively. Wouldn't it make
sense add this script to contrib (after teaching it new and removed
submodules and documenting its use in a few lines after the shebang)?
A few things somebody may want to work on while doing that few
lines of documentation I know about are:
* From the core side, pass options that are releavant when
generating patch (i.e. with p) in environment variables to the
external diff script;
* Not using s/$1 and m/$1 as prefix; instead, pass src/dst
prefix values (i.e. s/ and m/) from the core side in environment
variables, and make the external diff script itself aware of
possibly nested submodules, e.g.
SUBMODULE_PATH=${SUBMODULE_PATH}$1
export SUBMODULE_PATH
exec git --no-pager diff -p \
--src-prefix=$SRC_PREFIX/$SUBMODULE_PATH \
--dst-prefix=$DST_PREFIX/$SUBMODULE_PATH $3 $6
After people gain sufficient experience with it, as the next step,
we can think about how to handle --stat and other options when we
are run without -p (currently the attribute mechanism would not
trigger) and then we can call the result a native diff that
recurses into submodules that people can use without setting up the
attributes based mechanism.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v3 4/6] Refactor mechanics of testing in a sub test-lib
Adam Spiers g...@adamspiers.org writes:
This will allow us to test the test framework more thoroughly
without disrupting the top-level test metrics.
Signed-off-by: Adam Spiers g...@adamspiers.org
---
t/t-basic.sh | 67
1 file changed, 29 insertions(+), 38 deletions(-)
diff --git a/t/t-basic.sh b/t/t-basic.sh
index c6b42de..029e3bd 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -55,39 +55,49 @@ test_expect_failure 'pretend we have a known breakage' '
false
'
-test_expect_success 'pretend we have fixed a known breakage (run in sub
test-lib)'
- mkdir passing-todo
- (cd passing-todo
- cat passing-todo.sh -EOF
+run_sub_test_lib_test () {
+ name=$1 descr=$2 # stdin is body of test code
+ mkdir $name
+ (cd $name
+ cat $name.sh -EOF
#!$SHELL_PATH
- test_description='A passing TODO test
+ test_description='$descr (run in sub test-lib)
This is run in a sub test-lib so that we do not get incorrect
passing metrics
'
# Point to the t/test-lib.sh, which isn't in ../ as usual
- TEST_DIRECTORY=\$TEST_DIRECTORY\
- . \\$TEST_DIRECTORY\/test-lib.sh
-
- test_expect_failure 'pretend we have fixed a known breakage' '
- :
- '
+ TEST_DIRECTORY=$TEST_DIRECTORY
+ . \$TEST_DIRECTORY/test-lib.sh
+ EOF
The quoting of $TEST_DIRECTORY in the assignment does not look
correct (imagine a path with a double quote in it).
Removing the assignment and instead exporting TEST_DIRECTORY before
calling name.sh may be a reasonable fix, than trying to quotemeta
the value of $TEST_DIRECTORY here.
I'll re-queue this series in 'pu' with fixes and retitles; please
eyeball them before submitting a reroll.
b465316 tests: paint unexpectedly fixed known breakages in bold red
7214717 tests: test the test framework more thoroughly
03c772a [SQUASH] t/t-basic.sh: quoting of TEST_DIRECTORY is screwed up
99fe0af tests: refactor mechanics of testing in a sub test-lib
6af90bf tests: paint skipped tests in bold blue
0b87581 tests: test number comes first in 'not ok $count - $message'
1c55079 tests: paint known breakages in bold yellow
The third one from the tip looks like the following, to re-indent to
make it readable and then minimally fix the quoting.
Thanks.
t/t-basic.sh | 50 +++---
1 file changed, 27 insertions(+), 23 deletions(-)
diff --git a/t/t-basic.sh b/t/t-basic.sh
index ee78e68..c3345a9 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -56,33 +56,37 @@ test_expect_failure 'pretend we have a known breakage' '
'
run_sub_test_lib_test () {
- name=$1 descr=$2 # stdin is body of test code
+ name=$1 descr=$2 # stdin is the body of the test code
mkdir $name
- (cd $name
- cat $name.sh -EOF
- #!$SHELL_PATH
-
- test_description='$descr (run in sub test-lib)
-
- This is run in a sub test-lib so that we do not get incorrect
- passing metrics
- '
-
- # Point to the t/test-lib.sh, which isn't in ../ as usual
- TEST_DIRECTORY=$TEST_DIRECTORY
- . \$TEST_DIRECTORY/test-lib.sh
- EOF
- cat $name.sh
- chmod +x $name.sh
- ./$name.sh out 2err)
+ (
+ cd $name
+ cat $name.sh -EOF
+ #!$SHELL_PATH
+
+ test_description='$descr (run in sub test-lib)
+
+ This is run in a sub test-lib so that we do not get incorrect
+ passing metrics
+ '
+
+ # Point to the t/test-lib.sh, which isn't in ../ as usual
+ . \$TEST_DIRECTORY/test-lib.sh
+ EOF
+ cat $name.sh
+ chmod +x $name.sh
+ export TEST_DIRECTORY
+ ./$name.sh out 2err
+ )
}
check_sub_test_lib_test () {
- name=$1 # stdin is expected output from the test
- (cd $name
- ! test -s err
- sed -e 's/^ //' -e 's/Z$//' expect
- test_cmp expect out)
+ name=$1 # stdin is the expected output from the test
+ (
+ cd $name
+ ! test -s err
+ sed -e 's/^ //' -e 's/Z$//' expect
+ test_cmp expect out
+ )
}
test_expect_success 'pretend we have fixed a known breakage'
--
1.7.12.1.389.g3dff30b
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] remote-curl: let users turn off smart http
Jeff King p...@peff.net writes: I'm half-tempted to just drop the config entirely, leave GIT_SMART_HTTP=false as an escape hatch, and see if anybody even cares. Sounds like a very attractive minimalistic way to go forward. We can always add per-remote configuration when we find it necessary, but once we add support, we cannot easily yank it out. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCHv8] clone --single: limit the fetch refspec to fetched branch
Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] new git check-ignore sub-command
Adam Spiers g...@adamspiers.org writes: Adam Spiers (14): Update directory listing API doc to match code Improve documentation and comments regarding directory traversal API Rename cryptic 'which' variable to more consistent name Rename path_excluded() to is_path_excluded() Rename excluded_from_list() to is_excluded_from_list() Rename excluded() to is_excluded() Refactor is_excluded_from_list() Refactor is_excluded() Refactor is_path_excluded() For each exclude pattern, store information about where it came from Refactor treat_gitlinks() Extract some useful pathspec handling code from builtin/add.c into a library Provide free_directory() for reclaiming dir_struct memory Add git-check-ignore sub-command Please retitle these to have a short prefix: that names a specific area the series intends to touch. I retitled your other series to share test : as their common prefix. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 2/2] remote-curl: let users turn off smart http
On Thu, Sep 20, 2012 at 02:15:20PM -0700, Junio C Hamano wrote:
Jeff King p...@peff.net writes:
I'm half-tempted to just drop the config entirely, leave
GIT_SMART_HTTP=false as an escape hatch, and see if anybody even cares.
Sounds like a very attractive minimalistic way to go forward. We
can always add per-remote configuration when we find it necessary,
but once we add support, we cannot easily yank it out.
Like this?
-- 8 --
Subject: [PATCH] remote-curl: let users turn off smart http
Usually there is no need for users to specify whether an
http remote is smart or dumb; the protocol is designed so
that a single initial request is made, and the client can
determine the server's capability from the response.
However, some misconfigured dumb-only servers may not like
the initial request by a smart client, as it contains a
query string. Until recently, commit 703e6e7 worked around
this by making a second request. However, that commit was
recently reverted due to its side effect of masking the
initial request's error code.
Since git has had that workaround for several years, we
don't know exactly how many such misconfigured servers are
out there. The reversion of 703e6e7 assumes they are rare
enough not to worry about. Still, that reversion leaves
somebody who does run into such a server with no escape
hatch at all. Let's give them an environment variable they
can tweak to perform the dumb request.
This is intentionally not a documented interface. It's
overly simple and is really there for debugging in case
somebody does complain about git not working with their
server. A real user-facing interface would entail a
per-remote or per-URL config variable.
Signed-off-by: Jeff King p...@peff.net
---
remote-curl.c | 3 ++-
t/t5551-http-fetch.sh | 12
2 files changed, 14 insertions(+), 1 deletion(-)
diff --git a/remote-curl.c b/remote-curl.c
index c0b98cc..7b19ebb 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -102,7 +102,8 @@ static struct discovery* discover_refs(const char *service)
free_discovery(last);
strbuf_addf(buffer, %sinfo/refs, url);
- if (!prefixcmp(url, http://;) || !prefixcmp(url, https://;)) {
+ if ((!prefixcmp(url, http://;) || !prefixcmp(url, https://;))
+git_env_bool(GIT_SMART_HTTP, 1)) {
maybe_smart = 1;
if (!strchr(url, '?'))
strbuf_addch(buffer, '?');
diff --git a/t/t5551-http-fetch.sh b/t/t5551-http-fetch.sh
index 2db5c35..8427943 100755
--- a/t/t5551-http-fetch.sh
+++ b/t/t5551-http-fetch.sh
@@ -129,6 +129,18 @@ test_expect_success 'clone from auth-only-for-push
repository' '
test_cmp expect actual
'
+test_expect_success 'disable dumb http on server' '
+ git --git-dir=$HTTPD_DOCUMENT_ROOT_PATH/repo.git \
+ config http.getanyfile false
+'
+
+test_expect_success 'GIT_SMART_HTTP can disable smart http' '
+ (GIT_SMART_HTTP=0
+export GIT_SMART_HTTP
+cd clone
+test_must_fail git fetch)
+'
+
test -n $GIT_TEST_LONG test_set_prereq EXPENSIVE
test_expect_success EXPENSIVE 'create 50,000 tags in the repo' '
--
1.7.11.7.15.g085c6bd
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 10/14] For each exclude pattern, store information about where it came from
Adam Spiers g...@adamspiers.org writes:
void add_exclude(const char *string, const char *base,
- int baselen, struct exclude_list *el)
+ int baselen, struct exclude_list *el, const char *src, int
srcpos)
{
struct exclude *x;
size_t len;
@@ -341,6 +341,8 @@ void add_exclude(const char *string, const char *base,
x-base = base;
x-baselen = baselen;
x-flags = flags;
+ x-src = src;
+ x-srcpos = srcpos;
Hrm, don't all elements x in el share the same src, even if
their srcpos may be different?
if (!strchr(string, '/'))
x-flags |= EXC_FLAG_NODIR;
x-nowildcardlen = simple_length(string);
@@ -393,7 +395,7 @@ int add_excludes_from_file_to_list(const char *fname,
int check_index)
{
struct stat st;
- int fd, i;
+ int fd, i, lineno = 1;
size_t size = 0;
char *buf, *entry;
@@ -438,8 +440,9 @@ int add_excludes_from_file_to_list(const char *fname,
if (buf[i] == '\n') {
if (entry != buf + i entry[0] != '#') {
buf[i - (i buf[i-1] == '\r')] = 0;
- add_exclude(entry, base, baselen, el);
+ add_exclude(entry, base, baselen, el, fname,
lineno);
}
+ lineno++;
entry = buf + i + 1;
}
}
@@ -474,8 +477,10 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
!strncmp(dir-basebuf, base, stk-baselen))
break;
dir-exclude_stack = stk-prev;
- while (stk-exclude_ix el-nr)
- free(el-excludes[--el-nr]);
+ while (stk-exclude_ix el-nr) {
+ struct exclude *exclude = el-excludes[--el-nr];
+ free(exclude);
+ }
free(stk-filebuf);
free(stk);
}
@@ -502,7 +507,15 @@ static void prep_exclude(struct dir_struct *dir, const
char *base, int baselen)
memcpy(dir-basebuf + current, base + current,
stk-baselen - current);
strcpy(dir-basebuf + stk-baselen, dir-exclude_per_dir);
- add_excludes_from_file_to_list(dir-basebuf,
+
+ /* dir-basebuf gets reused by the traversal, but we
+ * need fname to remain unchanged to ensure the src
+ * member of each struct exclude correctly back-references
+ * its source file.
+ */
+ char *fname = strdup(dir-basebuf);
/*
* We try to format our multi-line comments
* like this.
*
* By the way, who owns x-src and who is responsible for
* freeing it when the exclude-stack is popped to make them
* no longer necessary?
*
* Oh, by the way, that is a decl-after-statement.
*/
+
+ add_excludes_from_file_to_list(fname,
dir-basebuf, stk-baselen,
stk-filebuf, el, 1);
dir-exclude_stack = stk;
diff --git a/dir.h b/dir.h
index 19beddb..ebb0367 100644
--- a/dir.h
+++ b/dir.h
@@ -31,6 +31,9 @@ struct exclude_list {
int baselen;
int to_exclude;
int flags;
+ const char *src;
+ int srcpos; /* counting starts from 1 for line numbers in
ignore files,
+and from -1 decrementing for patterns from CLI
(--exclude) */
} **excludes;
};
@@ -123,7 +126,7 @@ extern int add_excludes_from_file_to_list(const char
*fname, const char *base, i
char **buf_p, struct exclude_list
*el, int check_index);
extern void add_excludes_from_file(struct dir_struct *, const char *fname);
extern void add_exclude(const char *string, const char *base,
- int baselen, struct exclude_list *el);
+ int baselen, struct exclude_list *el, const char *src,
int srcpos);
extern void free_excludes(struct exclude_list *el);
extern int file_exists(const char *);
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] new git check-ignore sub-command
Junio C Hamano gits...@pobox.com writes: Adam Spiers g...@adamspiers.org writes: Adam Spiers (14): Update directory listing API doc to match code Improve documentation and comments regarding directory traversal API Rename cryptic 'which' variable to more consistent name Rename path_excluded() to is_path_excluded() Rename excluded_from_list() to is_excluded_from_list() Rename excluded() to is_excluded() Refactor is_excluded_from_list() Refactor is_excluded() Refactor is_path_excluded() For each exclude pattern, store information about where it came from Refactor treat_gitlinks() Extract some useful pathspec handling code from builtin/add.c into a library Provide free_directory() for reclaiming dir_struct memory Add git-check-ignore sub-command Please retitle these to have a short prefix: that names a specific area the series intends to touch. I retitled your other series to share test : as their common prefix. Just to clarify, I think most of them can say dir.c: . I saw quite a few decl-after-statement in new code. Please fix them. As to the who owns x-src and when is it freed question, it may make sense to give el a filename field (command line and other special cases would get whatever value you deem appropriate, like NULL or command line), have x-src point at that field when you queue many x's to the el in add_exc_from_file_to_list(). Then when you pop an element in the exclude_stack, you can free el-filename to plug a potential leak. Also I do not see why you need to have the strdup() in the caller of add_excludes_from_file_to_list(). If you need to keep it stable because you are copying it away in exclude or excludde_list, wouldn't it make more sense to do that at the beginning of the callee, i.e. add_excludes_from_file_to_list() function? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: git archive --format zip utf-8 issues
Am 18.09.2012 23:12, schrieb Junio C Hamano: René Scharfe rene.scha...@lsrfire.ath.cx writes: WindowsInfo-ZIP unzip 7-Zip PeaZip builtin Linux msysgit Windows 7-Zip 9.20 0 0 4626 43 43 PeaZip 4.7.1 win64 0 0 4626 42 42 Info-ZIP zip 3.0 Linux 0 0 72 0 43 43 Info-ZIP zip 3.0 Windows 45 45 n/a 0 43 43 It is kind of surprising that Windows builtin has very poor score extracting from the output of Zip tools running on Windows (I am looking at 46, 46 and n/a over there). If you tell it to create an archive from its disk and then extract from it, I wonder what would happen. I didn't include it as a packer because it refused to archive the pangrams directory due to illegal characters in one of the filenames. When I just tried a bit harder, I had to delete all but 14 files with Latin script, accents etc. before I could zip the directory. I'll include these results in the next round. It uses codepage 850 on my system (MSDOS Latin 1). I don't expect this to be portable. Does this result mean that practically nobody uses Zip archive with exotic letters in paths on that platform? I am not talking about developers and savvy people who know where to download third-party Zip archivers and how to install them. I am imagining a grandma who received an archive full of photos of her grandchild in her Outlook Express or GMail inbox, clicked the attachment to download it, and is trying to view the photo inside. Not necessarily. Photos often have names like img_0123.jpg etc., which are handled just fine. And all family members probably use the same codepage on their computers, so they're less likely to run into this problem. By the way, I found this bug asking for codepage support in unzip: https://bugs.launchpad.net/ubuntu/+source/unzip/+bug/580961 Multiple codepages seem to be used for ZIP files in the wild, none of them are supported by unzip on Linux, which only accepts ASCII or UTF-8. René -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0
On Thu, Sep 20, 2012 at 10:24 AM, Jeff King p...@peff.net wrote: On Wed, Sep 19, 2012 at 10:57:35PM -0700, Shawn O. Pearce wrote: so transient errors on the initial smart contact can cause us to fall back to dumb, Transient errors is actually what is leading me down this path. We see about 0.0455% of our requests to the Android hosting service as these dumb fallbacks. This means the client never got a proper smart service reply. Server logs suggest we sent a valid response, so I am suspecting transient proxy errors, but its hard to debug because clients discard the error. Yup, we see the same thing. I've tracked a few down manually to actual things like gateway timeouts on our reverse proxy. Our reverse proxies also fail sometimes. :-) But right now I am seeing failures in libcurl's SSL connection that may also be causing the smart connection failures. For example this trace, where libcurl was just not able to connect to respond to the 401 with a password. I suspect what is happening is the SSL session dropped out of cache on our servers, and libcurl couldn't reuse the existing SSL session. Instead of discarding the bad session and retrying, Git aborts. I'm willing to bet modern browsers just discard the bad session and start a new one, because clients can't assume the remote server will be able to remember their session forever. * Couldn't find host android.googlesource.com in the .netrc file; using defaults * About to connect() to android.googlesource.com port 443 (#0) * Trying 2607:f8b0:400e:c02::52... * Connected to android.googlesource.com (2607:f8b0:400e:c02::52) port 443 (#0) * successfully set certificate verify locations: * CAfile: none CApath: /etc/ssl/certs * SSL connection using RC4-SHA * Server certificate: *subject: C=US; ST=California; L=Mountain View; O=Google Inc; CN=*.googlecode.com *start date: 2012-08-16 12:25:39 GMT *expire date: 2013-06-07 19:43:27 GMT *subjectAltName: android.googlesource.com matched *issuer: C=US; O=Google Inc; CN=Google Internet Authority *SSL certificate verify ok. GET /a/platform/tools/build/info/refs?service=git-upload-pack HTTP/1.1 User-Agent: git/1.7.12.1.1.g9b7ccb3 Host: android.googlesource.com Accept: */* Pragma: no-cache * The requested URL returned error: 401 * Closing connection #0 * Couldn't find host android.googlesource.com in the .netrc file; using defaults * About to connect() to android.googlesource.com port 443 (#0) * Trying 2607:f8b0:400e:c02::52... * Connected to android.googlesource.com (2607:f8b0:400e:c02::52) port 443 (#0) * successfully set certificate verify locations: * CAfile: none CApath: /etc/ssl/certs * SSL re-using session ID * Unknown SSL protocol error in connection to android.googlesource.com:443 * Expire cleared * Closing connection #0 error: Unknown SSL protocol error in connection to android.googlesource.com:443 while accessing https://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack fatal: HTTP request failed -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 00/14] new git check-ignore sub-command
On Thu, Sep 20, 2012 at 10:43 PM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Adam Spiers g...@adamspiers.org writes: Adam Spiers (14): Update directory listing API doc to match code Improve documentation and comments regarding directory traversal API Rename cryptic 'which' variable to more consistent name Rename path_excluded() to is_path_excluded() Rename excluded_from_list() to is_excluded_from_list() Rename excluded() to is_excluded() Refactor is_excluded_from_list() Refactor is_excluded() Refactor is_path_excluded() For each exclude pattern, store information about where it came from Refactor treat_gitlinks() Extract some useful pathspec handling code from builtin/add.c into a library Provide free_directory() for reclaiming dir_struct memory Add git-check-ignore sub-command Please retitle these to have a short prefix: that names a specific area the series intends to touch. I retitled your other series to share test : as their common prefix. Just to clarify, I think most of them can say dir.c: . Sure, I can do that, but shouldn't this convention be documented in SubmittingPatches? I saw quite a few decl-after-statement in new code. Please fix them. Again, I can do that no problem, but again this convention is undocumented and I am not psychic ;-) I see that a patch was provided 5 years ago to document this, but was apparently never pulled in: http://thread.gmane.org/gmane.comp.version-control.git/47843/focus=48015 It would save everybody's time if these things are documented, since then they wouldn't create noise during the review process. I also see in the same thread that a patch to add -Wdeclaration-after-statement to CFLAGS was also offered but never pulled in, presumably on the stated grounds that that option was relatively recent 5 years ago. But wouldn't it be trivial to do gcc -v --help 21 | grep declaration-after-statement at build-time? I'm also curious to know why this convention exists. Are people really still compiling git with compilers which don't support C99? As to the who owns x-src and when is it freed question, it may make sense to give el a filename field (command line and other special cases would get whatever value you deem appropriate, like NULL or command line), have x-src point at that field when you queue many x's to the el in add_exc_from_file_to_list(). Then when you pop an element in the exclude_stack, you can free el-filename to plug a potential leak. Also I do not see why you need to have the strdup() in the caller of add_excludes_from_file_to_list(). If you need to keep it stable because you are copying it away in exclude or excludde_list, wouldn't it make more sense to do that at the beginning of the callee, i.e. add_excludes_from_file_to_list() function? Thanks, I'll take a look at these suggestions tomorrow. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git diff-tree -r -C output inexact sometimes
Hello A colleague of mine discovered an inconsistency in the functioning of git diff-tree -r -C in specific conditions. As tenuous as these conditions might seem (once you run the script in attachment and analyse its output), please rest assured that it comes from a real-life case. Running the script in attachment produces a git repository in which were operated a large number of file renames, in which many of the renamed files (in this particular case all) have the same content but different names. The commit data from the renaming operation (last commit in the script- generated history) is inexactly rendered by the command git diff-tree -r -C master The logical result is correctly produced by the more restricted command git diff-tree -r -M master IMO for this particular last commit both the above commands should return the same result. Note that reducing i or j in the generator script attached below makes the bug dissapear. Thanks a lot for your attention. -- Cristian Tibirna KDE developer .. tibi...@kde.org .. http://www.kde.org generate_git_tree.sh Description: application/shellscript
Re: [PATCH v2 00/14] new git check-ignore sub-command
Adam Spiers g...@adamspiers.org writes: Sure, I can do that, but shouldn't this convention be documented in SubmittingPatches? People must have learned this by imitating what senior contributors send to the list, but the [Subject] area: title thing does not appear in that document. I agree it should (patches welcome). I saw quite a few decl-after-statement in new code. Please fix them. Again, I can do that no problem, but again this convention is undocumented and I am not psychic. Yeah, when there is no code that does decl-after-statement, with the imitate surrounding code rule alone, I agree that it is a bit hard to tell we do not allow it (as opposed to seeing a construct is often used and assuming that the construct is permitted, which is much easier). I see that a patch was provided 5 years ago to document this, but was apparently never pulled in: http://thread.gmane.org/gmane.comp.version-control.git/47843/focus=48015 I just read SubmittingPatches again and looked for 1a as found in that patch, and it is there. I also see in the same thread that a patch to add -Wdeclaration-after-statement to CFLAGS was also offered but never pulled in, There is no guarantee your CC would understand it; you don't even know if it is a gcc in the first place. I'm also curious to know why this convention exists. Are people really still compiling git with compilers which don't support C99? See 6d62c98 (Makefile: Change the default compiler from gcc to cc, 2011-12-20). -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] Disable dumb HTTP fallback with GIT_CURL_FALLBACK=0
On Thu, Sep 20, 2012 at 04:05:03PM -0700, Shawn O. Pearce wrote: But right now I am seeing failures in libcurl's SSL connection that may also be causing the smart connection failures. For example this trace, where libcurl was just not able to connect to respond to the 401 with a password. I suspect what is happening is the SSL session dropped out of cache on our servers, and libcurl couldn't reuse the existing SSL session. Instead of discarding the bad session and retrying, Git aborts. I'm willing to bet modern browsers just discard the bad session and start a new one, because clients can't assume the remote server will be able to remember their session forever. That's something I haven't seen. But then, I don't usually see the client side; I just see the fallback dumb fetch in our logs, and have occasionally followed up. Is there a long pause while the user is typing their password? * SSL re-using session ID * Unknown SSL protocol error in connection to android.googlesource.com:443 * Expire cleared * Closing connection #0 error: Unknown SSL protocol error in connection to android.googlesource.com:443 while accessing https://android.googlesource.com/a/platform/tools/build/info/refs?service=git-upload-pack fatal: HTTP request failed You could try turning off CURLOPT_SSL_SESSIONID_CACHE and seeing if that improves it. Of course, it is probably hard to reproduce, so it would be tough to know if that helped or not. It would also be nice if you could dump more information on the error from the ssl library (I typically build curl against openssl; I wonder if it could be related to using gnutls or something). -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 0/3] nicer receive-pack errors over http
While we are on the subject of http user experience, I thought I'd mention this patch to route more errors from index-pack back to the user. We're not doing it yet at GitHub, but I plan to apply it soon. The first patch is a cleanup and minor bug fix. The second is the interesting one. The third is purely cosmetic, and doesn't need to be tied to the others. [1/3]: receive-pack: redirect unpack-objects stdout to /dev/null [2/3]: receive-pack: send pack-processing stderr over sideband [3/3]: receive-pack: drop n/a on unpacker errors -Peff -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 1/3] receive-pack: redirect unpack-objects stdout to /dev/null
The unpack-objects command should not generally produce any
output on stdout. However, if it's given extra input after
the packfile, it will spew the remainder to stdout. When
called by receive-pack, this means we will break protocol,
since our stdout is connected to the remote send-pack.
Signed-off-by: Jeff King p...@peff.net
---
I've never actually seen this bug in practice, but I noticed it while
auditing the outputs of receive-pack's children recently.
builtin/receive-pack.c | 7 ++-
1 file changed, 6 insertions(+), 1 deletion(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 9145f1a..5ba0c98 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -815,6 +815,7 @@ static const char *unpack(void)
if (ntohl(hdr.hdr_entries) unpack_limit) {
int code, i = 0;
+ struct child_process child;
const char *unpacker[5];
unpacker[i++] = unpack-objects;
if (quiet)
@@ -823,7 +824,11 @@ static const char *unpack(void)
unpacker[i++] = --strict;
unpacker[i++] = hdr_arg;
unpacker[i++] = NULL;
- code = run_command_v_opt(unpacker, RUN_GIT_CMD);
+ memset(child, 0, sizeof(child));
+ child.argv = unpacker;
+ child.no_stdout = 1;
+ child.git_cmd = 1;
+ code = run_command(child);
if (!code)
return NULL;
return unpack-objects abnormal exit;
--
1.7.11.7.15.g085c6bd
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 2/3] receive-pack: send pack-processing stderr over sideband
Receive-pack invokes either unpack-objects or index-pack to
handle the incoming pack. However, we do not redirect the
stderr of the sub-processes at all, so it is never seen by
the client. From the initial thread adding sideband support,
which is here:
http://thread.gmane.org/gmane.comp.version-control.git/139471
it is clear that some messages are specifically kept off the
sideband (with the assumption that they are of interest only
to an administrator, not the client). The stderr of the
subprocesses is mentioned in the thread, but it's unclear if
they are included in that group, or were simply forgotten.
However, there are a few good reasons to show them to the
client:
1. In many cases, they are directly about the incoming
packfile (e.g., fsck warnings with --strict, corruption
in the packfile, etc). Without these messages, the
client just gets unpacker error with no extra useful
diagnosis.
2. No matter what the cause, we are probably better off
showing the errors to the client. If the client and the
server admin are not the same entity, it is probably
much easier for the client to cut-and-paste the errors
they see than for the admin to try to dig them out of a
log and correlate them with a particular session.
3. Users of the ssh transport typically already see these
stderr messages, as the remote's stderr is copied
literally by ssh. This brings other transports (http,
and push-over-git if you are crazy enough to enable it)
more in line with ssh. As a bonus for ssh users,
because the messages are now fed through the sideband
and printed by the local git, they will have remote:
prepended and be properly interleaved with any local
output to stderr.
Signed-off-by: Jeff King p...@peff.net
---
This one is logically independent of the first patch, but relies
textually on the conversion of unpack-objects to use a separate
child_process.
builtin/receive-pack.c | 26 --
1 file changed, 24 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 5ba0c98..ac679ab 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -795,7 +795,7 @@ static const char *pack_lockfile;
static const char *pack_lockfile;
-static const char *unpack(void)
+static const char *unpack(int err_fd)
{
struct pack_header hdr;
const char *hdr_err;
@@ -827,6 +827,7 @@ static const char *unpack(void)
memset(child, 0, sizeof(child));
child.argv = unpacker;
child.no_stdout = 1;
+ child.err = err_fd;
child.git_cmd = 1;
code = run_command(child);
if (!code)
@@ -853,6 +854,7 @@ static const char *unpack(void)
memset(ip, 0, sizeof(ip));
ip.argv = keeper;
ip.out = -1;
+ ip.err = err_fd;
ip.git_cmd = 1;
status = start_command(ip);
if (status) {
@@ -869,6 +871,26 @@ static const char *unpack(void)
}
}
+static const char *unpack_with_sideband(void)
+{
+ struct async muxer;
+ const char *ret;
+
+ if (!use_sideband)
+ return unpack(0);
+
+ memset(muxer, 0, sizeof(muxer));
+ muxer.proc = copy_to_sideband;
+ muxer.in = -1;
+ if (start_async(muxer))
+ return NULL;
+
+ ret = unpack(muxer.in);
+
+ finish_async(muxer);
+ return ret;
+}
+
static void report(struct command *commands, const char *unpack_status)
{
struct command *cmd;
@@ -966,7 +988,7 @@ int cmd_receive_pack(int argc, const char **argv, const
char *prefix)
const char *unpack_status = NULL;
if (!delete_only(commands))
- unpack_status = unpack();
+ unpack_status = unpack_with_sideband();
execute_commands(commands, unpack_status);
if (pack_lockfile)
unlink_or_warn(pack_lockfile);
--
1.7.11.7.15.g085c6bd
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH 3/3] receive-pack: drop n/a on unpacker errors
The output from git push currently looks like this:
$ git push dest HEAD
fatal: [some message from index-pack]
error: unpack failed: index-pack abnormal exit
To dest
! [remote rejected] HEAD - master (n/a (unpacker error))
That n/a is meant to be the per-ref status is not
available but the nested parentheses just make it look
ugly. Let's turn the final line into just:
! [remote rejected] HEAD - master (unpacker error)
Signed-off-by: Jeff King p...@peff.net
---
Maybe it is just me, but I have always found the n/a and extra
parentheses ugly and unnecessary. But obviously others may differ.
It doesn't really come up that often, since index-pack failing usually
implies a git bug. But with transfer.fsckObjects turn on, it is more
common.
I don't think there should be any backwards compatibility issues with
changing this. The reason field sent back by receive-pack has always
been a free-form human-readable string.
I also dislike the index-pack abnormal exit message. Again, when
index-pack really crashes, it's fine, but it can die due to bogus
objects, too, in which case it might be nice to have a more
human-readable message.
builtin/receive-pack.c | 2 +-
t/t5504-fetch-receive-strict.sh | 2 +-
2 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ac679ab..ff781fe 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -695,7 +695,7 @@ static void execute_commands(struct command *commands,
const char *unpacker_erro
if (unpacker_error) {
for (cmd = commands; cmd; cmd = cmd-next)
- cmd-error_string = n/a (unpacker error);
+ cmd-error_string = unpacker error;
return;
}
diff --git a/t/t5504-fetch-receive-strict.sh b/t/t5504-fetch-receive-strict.sh
index 35ec294..69ee13c 100755
--- a/t/t5504-fetch-receive-strict.sh
+++ b/t/t5504-fetch-receive-strict.sh
@@ -89,7 +89,7 @@ To dst
cat exp EOF
To dst
-! refs/heads/master:refs/heads/test [remote rejected] (n/a
(unpacker error))
+! refs/heads/master:refs/heads/test [remote rejected] (unpacker
error)
EOF
test_expect_success 'push with receive.fsckobjects' '
--
1.7.11.7.15.g085c6bd
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
