* Re: cmd_cherry in builtin/log.c?
From: René Scharfe @ 2010-12-09 21:14 UTC (permalink / raw)
Cc: Junio C Hamano, Jonathan Nieder, Thiago Farina, Git Mailing List
In-Reply-To: <7vaakhpbq2.fsf@alter.siamese.dyndns.org>
Am 07.12.2010 21:30, schrieb Junio C Hamano:
> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>
>> Am 07.12.2010 18:39, schrieb Jonathan Nieder:
>>> while the core of the builtin version is
>>>
>>> get_patch_ids(&revs, &patch_id_opts, prefix);
>>>
>>> The latter function is static, introduced by v1.4.1~12^2~5
>>> (format-patch: introduce "--ignore-if-in-upstream", 2006-06-25).
>>>
>>> So the answer is that "git cherry" is considered a variant on
>>> "git log" (like format-patch, show, and whatchanged) and that it uses
>>> "git log" internals.
>>
>> That's right. get_patch_ids() could be moved into patch-ids.c now and
>> then cmd_cherry() could get its own file in builtin/, though.
>
> Right, but the key word is "could". Is it hurting _anything_ to have it
> in the current place? I doubt it.
Indeed. Moving cherry's code into its own file is be a code clean up.
There would be no benefit to users, future developers should have a
somewhat easier time navigating the code while any developers currently
working on the code would get it pulled from under them.
Such a clean up can be useful to include at the start of a patch series
that contains actual user visible improvements.
René
^ permalink raw reply
* Re: [PATCH v4 1/3] fetch/pull: Add the --recurse-submodules option
From: Junio C Hamano @ 2010-12-09 21:16 UTC (permalink / raw)
To: Jens Lehmann
Cc: Jonathan Nieder, Git Mailing List, Kevin Ballard, Jon Seymour,
Chris Packham, Marc Branchaud
In-Reply-To: <4CDD391C.2000905@web.de>
Jens Lehmann <Jens.Lehmann@web.de> writes:
> Until now you had to call "git submodule update" (without -N|--no-fetch
> option) or something like "git submodule foreach git fetch" to fetch
> new commits in populated submodules from their remote.
> ...
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index d35f000..db3fba3 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> ...
> @@ -784,28 +790,36 @@ static int add_remote_or_group(const char *name, struct string_list *list)
> return 1;
> }
>
> -static int fetch_multiple(struct string_list *list)
> +static void add_options_to_argv(int *argc, const char **argv)
> {
> - int i, result = 0;
> - const char *argv[11] = { "fetch", "--append" };
> -...
> +static int fetch_multiple(struct string_list *list)
> +{
> + int i, result = 0;
> + const char *argv[12] = { "fetch", "--append" };
This used to be 11; are we adding something new? Ahh, possibly
"--recurse_submodules".
> diff --git a/submodule.c b/submodule.c
> index 91a4758..4d9b774 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -63,7 +63,7 @@ void set_diffopt_flags_from_submodule_config(struct diff_options *diffopt,
> }
> }
>
> -static int submodule_config(const char *var, const char *value, void *cb)
> +int submodule_config(const char *var, const char *value, void *cb)
> {
> if (!prefixcmp(var, "submodule."))
> return parse_submodule_config_option(var, value);
> @@ -229,6 +229,70 @@ void show_submodule_summary(FILE *f, const char *path,
> strbuf_release(&sb);
> }
>
> +int fetch_populated_submodules(int num_options, const char **options,
> + const char *prefix, int quiet)
> +{
> + int i, result = 0, argc = 0;
> + struct child_process cp;
> + const char **argv;
> + struct string_list_item *name_for_path;
> + const char *work_tree = get_git_work_tree();
> + if (!work_tree)
> + return 0;
> +
> + if (!the_index.initialized)
> + if (read_cache() < 0)
> + die("index file corrupt");
> +
> + argv = xcalloc(num_options + 5, sizeof(const char *));
Where is this '5' coming from? "fetch" "--submodule-prefix", the prefix,
and the terminating NULL? What did I miss?
^ permalink raw reply
* Re: [PATCH] t9143: do not fail when unhandled.log.gz is not created
From: Eric Wong @ 2010-12-09 21:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Torsten Bögershausen
In-Reply-To: <4D0133AF.40000@web.de>
Torsten Bögershausen <tboegi@web.de> wrote:
> Do not depend on internal implementation details of svn,
> which right now uses perl to create a .gz file.
> So this test case will even work in the future,
> when svn changes its implementation.
>
> Signed-off-by: Torsten Bögershausen <tboegi@web.de>
Thanks Torsten, I needed to fix whitespace mangling from
your mailer to get it to apply cleanly.
Acked-by: Eric Wong <normalperson@yhbt.net>
It's pushed out to git://git.bogomips.org/git-svn along with an
older patch from Steven:
Steven Walter (1):
git-svn: allow the mergeinfo property to be set
Torsten Bögershausen (1):
t9143: do not fail when unhandled.log.gz is not created
--
Eric Wong
^ permalink raw reply
* Re: [PATCH] get_sha1: support relative path "<obj>:<sth>" syntax
From: Junio C Hamano @ 2010-12-09 21:37 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy; +Cc: git, jnareb, dirson, kevin, peff
In-Reply-To: <7vipz2fy98.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I think it is just the matter of moving "if (startup-info)..." logic
> outside the "is the :$path lacking an explicit stage number" block and
> having it after that if/else, no?
Like this, perhaps?
-- >8 --
Subject: get_sha1: teach ":$n:<path>" the same relative path logic
Earlier we taught the object name parser ":<path>" syntax that uses a path
relative to the current working directory. Given that ":<path>" is just a
short-hand for ":0:<path>" (i.e. "take stage #0 of that path"), we should
allow ":$n:<path>" to use relative path the same way.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
sha1_name.c | 14 ++++++++------
t/t1506-rev-parse-diagnosis.sh | 24 ++++++++++++++++++++++++
2 files changed, 32 insertions(+), 6 deletions(-)
diff --git a/sha1_name.c b/sha1_name.c
index f918faf..2074056 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -1091,17 +1091,19 @@ int get_sha1_with_context_1(const char *name, unsigned char *sha1,
return get_sha1_oneline(name + 2, sha1);
if (namelen < 3 ||
name[2] != ':' ||
- name[1] < '0' || '3' < name[1]) {
+ name[1] < '0' || '3' < name[1])
cp = name + 1;
- new_path = resolve_relative_path(cp);
- if (new_path)
- cp = new_path;
- }
else {
stage = name[1] - '0';
cp = name + 3;
}
- namelen = strlen(cp);
+ new_path = resolve_relative_path(cp);
+ if (!new_path) {
+ namelen = namelen - (cp - name);
+ } else {
+ cp = new_path;
+ namelen = strlen(cp);
+ }
strncpy(oc->path, cp,
sizeof(oc->path));
diff --git a/t/t1506-rev-parse-diagnosis.sh b/t/t1506-rev-parse-diagnosis.sh
index 1866470..9f8adb1 100755
--- a/t/t1506-rev-parse-diagnosis.sh
+++ b/t/t1506-rev-parse-diagnosis.sh
@@ -34,6 +34,8 @@ test_expect_success 'correct file objects' '
test_expect_success 'correct relative file objects (0)' '
git rev-parse :file.txt >expected &&
git rev-parse :./file.txt >result &&
+ test_cmp expected result &&
+ git rev-parse :0:./file.txt >result &&
test_cmp expected result
'
@@ -68,6 +70,28 @@ test_expect_success 'correct relative file objects (4)' '
)
'
+test_expect_success 'correct relative file objects (5)' '
+ git rev-parse :subdir/file.txt >expected &&
+ (
+ cd subdir &&
+ git rev-parse :./file.txt >result &&
+ test_cmp ../expected result &&
+ git rev-parse :0:./file.txt >result &&
+ test_cmp ../expected result
+ )
+'
+
+test_expect_success 'correct relative file objects (6)' '
+ git rev-parse :file.txt >expected &&
+ (
+ cd subdir &&
+ git rev-parse :../file.txt >result &&
+ test_cmp ../expected result &&
+ git rev-parse :0:../file.txt >result &&
+ test_cmp ../expected result
+ )
+'
+
test_expect_success 'incorrect revision id' '
test_must_fail git rev-parse foobar:file.txt 2>error &&
grep "Invalid object name '"'"'foobar'"'"'." error &&
^ permalink raw reply related
* [PATCH v9] Detection of directory renames
From: Yann Dirson @ 2010-12-09 21:38 UTC (permalink / raw)
To: git
This series requires:
- v6 of "generalizing sorted-array handling" series
- Jonathan Nieder's "compat: add memrchr()" patch,
aka. mid:<20101015051750.GA21830@burratino>
Changes since v8:
* use of the new sorted-array.h, which gives the main patch a diet
* split of debug func into its own patch
Next iteration will be merging the bulk-rm detection into this series.
Interested eyes can get a preview at t/* branches on
http://repo.or.cz/w/git/ydirson.git
^ permalink raw reply
* [PATCH 1/6] Introduce debug_bulkmove() in diffcore-rename.
From: Yann Dirson @ 2010-12-09 21:38 UTC (permalink / raw)
To: git; +Cc: Yann Dirson
In-Reply-To: <1291930687-8990-1-git-send-email-ydirson@altern.org>
This is an optional debug useful while developping new builk-* features.
Signed-off-by: Yann Dirson <ydirson@altern.org>
---
diffcore-rename.c | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/diffcore-rename.c b/diffcore-rename.c
index f7afdeb..e16fdeb 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -7,6 +7,22 @@
#include "hash.h"
#include "sorted-array.h"
+#define DEBUG_BULKMOVE 0
+
+#if DEBUG_BULKMOVE
+#define debug_bulkmove(args) __debug_bulkmove args
+void __debug_bulkmove(const char *fmt, ...)
+{
+ va_list ap;
+ va_start(ap, fmt);
+ fprintf(stderr, "[DBG] ");
+ vfprintf(stderr, fmt, ap);
+ va_end(ap);
+}
+#else
+#define debug_bulkmove(args) do { /*nothing */ } while (0)
+#endif
+
/* Table of rename/copy destinations */
struct diff_rename_dst {
--
1.7.2.3
^ permalink raw reply related
* [PATCH 5/6] Unified diff output format for bulk moves.
From: Yann Dirson @ 2010-12-09 21:38 UTC (permalink / raw)
To: git; +Cc: Yann Dirson, Yann Dirson
In-Reply-To: <1291930687-8990-1-git-send-email-ydirson@altern.org>
The output produced is as shown below.
diff --git-detect-bulk-moves mips/nxp/pnx833x/common/ mips/pnx833x/common/
bulk move with similarity index 100%
bulk move from mips/nxp/pnx833x/common/
bulk move to mips/pnx833x/common/
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
Documentation/diff-generate-patch.txt | 19 +++++++++++++
diff.c | 48 +++++++++++++++++++++++++-------
2 files changed, 56 insertions(+), 11 deletions(-)
diff --git a/Documentation/diff-generate-patch.txt b/Documentation/diff-generate-patch.txt
index 3ac2bea..367e859 100644
--- a/Documentation/diff-generate-patch.txt
+++ b/Documentation/diff-generate-patch.txt
@@ -71,6 +71,25 @@ separate lines indicate the old and the new mode.
rename to a
+bulk move entries
+-----------------
+
+When a bulk move is detected, a special block is output in addition to
+the renames that constitute the bulk move, looking like this:
+
+ diff --git-detect-bulk-moves a/dir1/ b/dir2/
+
+It is essentially similar to the standard diff header, with a special
+syntax showing we are describing a difference between two directories,
+and not a change to be applied as-is to a file.
+
+It is followed by three specific extended header lines:
+
+ bulk move with similarity index <number>
+ bulk move from <dir1>/
+ bulk move to <dir2>/
+
+
combined diff format
--------------------
diff --git a/diff.c b/diff.c
index 0694d7f..551fab7 100644
--- a/diff.c
+++ b/diff.c
@@ -2668,6 +2668,7 @@ static void run_diff_cmd(const char *pgm,
const char *xfrm_msg = NULL;
int complete_rewrite = (p->status == DIFF_STATUS_MODIFIED) && p->score;
int must_show_header = 0;
+ int use_color;
if (!DIFF_OPT_TST(o, ALLOW_EXTERNAL))
pgm = NULL;
@@ -2677,14 +2678,36 @@ static void run_diff_cmd(const char *pgm,
pgm = drv->external;
}
+ /*
+ * don't use colors when the header is intended for an
+ * external diff driver
+ */
+ use_color = DIFF_OPT_TST(o, COLOR_DIFF) && !pgm;
+
+ if (p->is_bulkmove) {
+ const char *set = diff_get_color(use_color, DIFF_METAINFO);
+ const char *reset = diff_get_color(use_color, DIFF_RESET);
+ struct strbuf *msgbuf;
+ char *line_prefix = "";
+
+ if (o->output_prefix) {
+ msgbuf = o->output_prefix(o, o->output_prefix_data);
+ line_prefix = msgbuf->buf;
+ }
+ fprintf(o->file, "%s%sdiff --git-detect-bulk-moves %s %s%s\n",
+ line_prefix, set, one->path, two->path, reset);
+ fprintf(o->file, "%s%sbulk move with similarity index %d%%%s\n",
+ line_prefix, set, similarity_index(p), reset);
+ fprintf(o->file, "%s%sbulk move from %s%s\n",
+ line_prefix, set, one->path, reset);
+ fprintf(o->file, "%s%sbulk move to %s%s\n",
+ line_prefix, set, two->path, reset);
+ return;
+ }
+
if (msg) {
- /*
- * don't use colors when the header is intended for an
- * external diff driver
- */
fill_metainfo(msg, name, other, one, two, o, p,
- &must_show_header,
- DIFF_OPT_TST(o, COLOR_DIFF) && !pgm);
+ &must_show_header, use_color);
xfrm_msg = msg->len ? msg->buf : NULL;
}
@@ -2757,8 +2780,10 @@ static void run_diff(struct diff_filepair *p, struct diff_options *o)
return;
}
- diff_fill_sha1_info(one);
- diff_fill_sha1_info(two);
+ if (!p->is_bulkmove) {
+ diff_fill_sha1_info(one);
+ diff_fill_sha1_info(two);
+ }
if (!pgm &&
DIFF_FILE_VALID(one) && DIFF_FILE_VALID(two) &&
@@ -3557,9 +3582,10 @@ static void diff_flush_patch(struct diff_filepair *p, struct diff_options *o)
if (diff_unmodified_pair(p))
return;
- if ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||
- (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode)))
- return; /* no tree diffs in patch format */
+ if (!p->is_bulkmove &&
+ ((DIFF_FILE_VALID(p->one) && S_ISDIR(p->one->mode)) ||
+ (DIFF_FILE_VALID(p->two) && S_ISDIR(p->two->mode))))
+ return; /* no tree diffs in patch format, except for bulk moves */
run_diff(p, o);
}
--
1.7.2.3
^ permalink raw reply related
* [PATCH 6/6] [WIP] Allow hiding renames of individual files involved in a directory rename.
From: Yann Dirson @ 2010-12-09 21:38 UTC (permalink / raw)
To: git; +Cc: Yann Dirson, Yann Dirson
In-Reply-To: <1291930687-8990-1-git-send-email-ydirson@altern.org>
Once has identified groups of bulk-moved files, and then
the --hide-bulk-move-details flag hides those of the individual renames
which carry no other information (further name change, or content changes).
This makes it much easier to a human reader to spot content changes
in a commit that also moves a whole subtree.
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
Documentation/diff-options.txt | 5 +++
diff.c | 7 ++++
diff.h | 3 ++
diffcore-rename.c | 68 ++++++++++++++++++++++++++++++++++++++--
diffcore.h | 1 +
5 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index 186cd6f..cc4fe9c 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -249,6 +249,11 @@ endif::git-log[]
Detect bulk move of all files of a directory into a
different one.
+--hide-bulk-move-details::
+ Hide the individual files moves that make up a bulk move,
+ without hinding other changes to the involved files (contents
+ change, name change relative to the bulk move's destination).
+
-C[<n>]::
--detect-copies[=<n>]::
Detect copies as well as renames. See also `--find-copies-harder`.
diff --git a/diff.c b/diff.c
index 551fab7..7e21e30 100644
--- a/diff.c
+++ b/diff.c
@@ -3230,6 +3230,13 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
if (!options->detect_rename)
options->detect_rename = DIFF_DETECT_RENAME;
}
+ else if (!strcmp(arg, "--hide-bulk-move-details")) {
+ DIFF_OPT_SET(options, HIDE_DIR_RENAME_DETAILS);
+ if (!DIFF_OPT_TST(options, DETECT_BULK_MOVES))
+ DIFF_OPT_SET(options, DETECT_BULK_MOVES);
+ if (!options->detect_rename)
+ options->detect_rename = DIFF_DETECT_RENAME;
+ }
else if (!strcmp(arg, "--follow"))
DIFF_OPT_SET(options, FOLLOW_RENAMES);
else if (!strcmp(arg, "--color"))
diff --git a/diff.h b/diff.h
index 1e2506c..f28fed5 100644
--- a/diff.h
+++ b/diff.h
@@ -79,6 +79,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)
#define DIFF_OPT_DETECT_BULK_MOVES (1 << 28)
+#define DIFF_OPT_HIDE_DIR_RENAME_DETAILS (1 << 29)
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)
@@ -268,6 +269,8 @@ extern void diffcore_fix_diff_index(struct diff_options *);
" try unchanged files as candidate for copy detection.\n" \
" --detect-bulk-moves\n" \
" detect moves of all files of a single directory.\n" \
+" --hide-bulk-move-details\n" \
+" hide individual files moves in a bulk move.\n" \
" -l<n> limit rename attempts up to <n> paths.\n" \
" -O<file> reorder diffs according to the <file>.\n" \
" -S<string> find filepair whose only one side contains the string.\n" \
diff --git a/diffcore-rename.c b/diffcore-rename.c
index 44df490..3d4b863 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -435,6 +435,34 @@ declare_sorted_array_insertonly_elem(static, struct diff_bulk_rename, register_b
bulkmove_candidates_cmp, bulkmove_candidates_init);
/*
+ * Marks as such file_rename if it is made uninteresting by dir_rename.
+ * Returns -1 if the file_rename is outside of the range in which given
+ * renames concerned by dir_rename are to be found (ie. end of loop),
+ * 0 otherwise.
+ */
+static int maybe_mark_uninteresting(struct diff_rename_dst *file_rename,
+ struct diff_bulk_rename *dir_rename,
+ int one_len, int two_len)
+{
+ if (!file_rename->pair) /* file add */
+ return 0;
+ if (strncmp(file_rename->two->path,
+ dir_rename->two->path, two_len))
+ return -1;
+ if (strncmp(file_rename->pair->one->path,
+ dir_rename->one->path, one_len) ||
+ !basename_same(file_rename->pair->one, file_rename->two) ||
+ file_rename->pair->score != MAX_SCORE)
+ return 0;
+
+ file_rename->pair->uninteresting_rename = 1;
+ debug_bulkmove(("%s* -> %s* makes %s -> %s uninteresting\n",
+ dir_rename->one->path, dir_rename->two->path,
+ file_rename->pair->one->path, file_rename->two->path));
+ return 0;
+}
+
+/*
* Copy dirname of src into dst, suitable to append a filename without
* an additional "/".
* Only handles relative paths since there is no absolute path in a git repo.
@@ -642,11 +670,44 @@ static void check_one_bulk_move(struct diff_filepair *dstpair)
* Take all file renames recorded so far and check if they could cause
* a bulk move to be detected.
*/
-static void diffcore_bulk_moves(void)
+static void diffcore_bulk_moves(int opt_hide_renames)
{
int i;
for (i = 0; i < rename_dst_nr; i++)
check_one_bulk_move(rename_dst[i].pair);
+
+ if (opt_hide_renames) {
+ /* flag as "uninteresting" those candidates hidden by dir move */
+ struct diff_bulk_rename *candidate;
+ for (candidate = bulkmove_candidates;
+ candidate < bulkmove_candidates + bulkmove_candidates_nr;
+ candidate++) {
+ int two_idx, i, one_len, two_len;
+ struct diff_rename_dst *two_sample;
+ if (candidate->discarded)
+ continue;
+
+ /* bisect to any entry within candidate dst dir */
+ two_sample = locate_rename_dst_dir(candidate->two->path);
+ if (!two_sample) {
+ die("PANIC: %s candidate of rename not in target tree (from %s)\n",
+ candidate->two->path, candidate->one->path);
+ }
+ two_idx = two_sample - rename_dst;
+
+ /* now remove extraneous 100% files inside. */
+ one_len = strlen(candidate->one->path);
+ two_len = strlen(candidate->two->path);
+ for (i = two_idx; i < rename_dst_nr; i++)
+ if (maybe_mark_uninteresting(rename_dst+i, candidate,
+ one_len, two_len) < 0)
+ break;
+ for (i = two_idx-1; i >= 0; i--)
+ if (maybe_mark_uninteresting(rename_dst+i, candidate,
+ one_len, two_len) < 0)
+ break;
+ }
+ }
}
void diffcore_rename(struct diff_options *options)
@@ -818,7 +879,7 @@ void diffcore_rename(struct diff_options *options)
/* Now possibly factorize those renames and copies. */
if (DIFF_OPT_TST(options, DETECT_BULK_MOVES))
- diffcore_bulk_moves();
+ diffcore_bulk_moves(DIFF_OPT_TST(options, HIDE_DIR_RENAME_DETAILS));
DIFF_QUEUE_CLEAR(&outq);
@@ -853,7 +914,8 @@ void diffcore_rename(struct diff_options *options)
struct diff_rename_dst *dst =
locate_rename_dst(p->two);
if (dst && dst->pair) {
- diff_q(&outq, dst->pair);
+ if (!dst->pair->uninteresting_rename)
+ diff_q(&outq, dst->pair);
pair_to_free = p;
}
else
diff --git a/diffcore.h b/diffcore.h
index 6dab95b..a4eb8e1 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -69,6 +69,7 @@ struct diff_filepair {
unsigned broken_pair : 1;
unsigned renamed_pair : 1;
unsigned is_unmerged : 1;
+ unsigned uninteresting_rename : 1;
unsigned is_bulkmove : 1;
};
#define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)
--
1.7.2.3
^ permalink raw reply related
* [PATCH 4/6] Add testcases for the --detect-bulk-moves diffcore flag.
From: Yann Dirson @ 2010-12-09 21:38 UTC (permalink / raw)
To: git; +Cc: Yann Dirson, Yann Dirson
In-Reply-To: <1291930687-8990-1-git-send-email-ydirson@altern.org>
This notably includes a couple of tests for cases known not to be
working correctly yet.
This patch has been improved by the following contributions:
- Jonathan Nieder: reworked style of test script
- Jonathan Nieder: use "git commit" in test instead of only plumbing,
and use test_tick
- Sverre Rabbelier: anonymize hashes
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Sverre Rabbelier <srabbelier@gmail.com>
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
t/t4046-diff-bulk-move.sh | 296 +++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 296 insertions(+), 0 deletions(-)
create mode 100755 t/t4046-diff-bulk-move.sh
diff --git a/t/t4046-diff-bulk-move.sh b/t/t4046-diff-bulk-move.sh
new file mode 100755
index 0000000..4b1c78e
--- /dev/null
+++ b/t/t4046-diff-bulk-move.sh
@@ -0,0 +1,296 @@
+#!/bin/sh
+#
+# Copyright (c) 2008,2010 Yann Dirson
+# Copyright (c) 2005 Junio C Hamano
+#
+
+# TODO for dir renames:
+# * two dirs or more moving all their files to a single dir
+# * simultaneous bulkmove and rename
+
+test_description='Test rename factorization in diff engine.
+
+'
+. ./test-lib.sh
+. "$TEST_DIRECTORY"/diff-lib.sh
+
+test_expect_success 'setup' '
+ git commit --allow-empty -m "original empty commit"
+
+ mkdir a &&
+ printf "Line %s\n" 1 2 3 4 5 6 7 8 9 10 11 12 13 14 15 >a/path0 &&
+ sed <a/path0 >a/path1 s/Line/Record/ &&
+ sed <a/path0 >a/path2 s/Line/Stuff/ &&
+ sed <a/path0 >a/path3 s/Line/Blurb/ &&
+
+ git update-index --add a/path* &&
+ test_tick &&
+ git commit -m "original set of files" &&
+
+ : rename the directory &&
+ git mv a b
+'
+test_expect_success 'diff-index --detect-bulk-moves after directory move.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# a/* b/
+ :100644 100644 X X R# a/path0 b/path0
+ :100644 100644 X X R# a/path1 b/path1
+ :100644 100644 X X R# a/path2 b/path2
+ :100644 100644 X X R# a/path3 b/path3
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup non-100% rename' '
+ echo "Line 16" >>b/path0 &&
+ git mv b/path2 b/2path &&
+ git rm -f b/path3 &&
+ echo anything >b/path100 &&
+ git add b/path100
+'
+test_expect_success 'diff-index --detect-bulk-moves after content changes.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# a/* b/
+ :100644 000000 X X D# a/path3
+ :100644 100644 X X R# a/path2 b/2path
+ :100644 100644 X X R# a/path0 b/path0
+ :100644 100644 X X R# a/path1 b/path1
+ :000000 100644 X X A# b/path100
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup bulk move that is not directory move' '
+ git reset -q --hard &&
+
+ mkdir c &&
+ (
+ for i in 0 1 2; do
+ cp a/path$i c/apath$i || exit
+ done
+ ) &&
+ git update-index --add c/apath* &&
+ test_tick &&
+ git commit -m "first set of changes" &&
+
+ git mv c/* a/
+'
+test_expect_success 'diff-index --detect-bulk-moves without full-dir rename.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# c/* a/
+ :100644 100644 X X R# c/apath0 a/apath0
+ :100644 100644 X X R# c/apath1 a/apath1
+ :100644 100644 X X R# c/apath2 a/apath2
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup bulk move with new file in source dir' '
+ echo > c/anotherpath "How much wood?" &&
+ git update-index --add c/another*
+'
+test_expect_success 'diff-index --detect-bulk-moves with new file in source dir.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# c/* a/
+ :100644 100644 X X R# c/apath0 a/apath0
+ :100644 100644 X X R# c/apath1 a/apath1
+ :100644 100644 X X R# c/apath2 a/apath2
+ :000000 100644 X X A# c/anotherpath
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup bulk move with interfering copy' '
+ rm c/anotherpath &&
+ git update-index --remove c/anotherpath &&
+ mkdir b &&
+ cp a/apath0 b/apath9 &&
+ echo >> a/apath0 "more" &&
+ git update-index --add a/apath0 b/apath9
+'
+# scores select the "wrong" one as "moved" (only a suboptimal detection)
+test_expect_failure 'diff-index --detect-bulk-moves with interfering copy.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# c/* a/
+ :100644 100644 X X R# c/apath0 a/apath0
+ :100644 100644 X X R# c/apath1 a/apath1
+ :100644 100644 X X R# c/apath2 a/apath2
+ :100644 100644 X X C# c/apath0 b/apath9
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup bulk move to toplevel' '
+ git reset -q --hard &&
+ git mv c/* .
+'
+test_expect_success 'diff-index --detect-bulk-moves bulk move to toplevel.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# c/* ./
+ :100644 100644 X X R# c/apath0 apath0
+ :100644 100644 X X R# c/apath1 apath1
+ :100644 100644 X X R# c/apath2 apath2
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup move including a subdir, with some content changes' '
+ git reset -q --hard &&
+ mv c a/ &&
+ git update-index --add --remove a/c/* c/apath0 c/apath1 c/apath2 &&
+ test_tick &&
+ git commit -m "move as subdir" &&
+
+ git mv a b &&
+ echo foo >>b/c/apath0 &&
+ git update-index --add b/c/apath*
+'
+test_expect_success 'diff-index --detect-bulk-moves on a move including a subdir.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# a/* b/
+ :040000 040000 X X R# a/c/* b/c/
+ :100644 100644 X X R# a/c/apath0 b/c/apath0
+ :100644 100644 X X R# a/c/apath1 b/c/apath1
+ :100644 100644 X X R# a/c/apath2 b/c/apath2
+ :100644 100644 X X R# a/path0 b/path0
+ :100644 100644 X X R# a/path1 b/path1
+ :100644 100644 X X R# a/path2 b/path2
+ :100644 100644 X X R# a/path3 b/path3
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup move of only a subdir' '
+ git reset -q --hard &&
+ : rename a subdirectory of a/. &&
+ git mv a/c a/d
+'
+test_expect_success 'moving a subdir only' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# a/c/* a/d/
+ :100644 100644 X X R# a/c/apath0 a/d/apath0
+ :100644 100644 X X R# a/c/apath1 a/d/apath1
+ :100644 100644 X X R# a/c/apath2 a/d/apath2
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup move without a subdir' '
+ git reset -q --hard &&
+ mkdir b &&
+ : rename files in the directory but not subdir. &&
+ git mv a/path* b/
+'
+test_expect_success 'moving files but not subdirs is not mistaken for dir move' '
+ cat >expected <<-EOF &&
+ :100644 100644 X X R# a/path0 b/path0
+ :100644 100644 X X R# a/path1 b/path1
+ :100644 100644 X X R# a/path2 b/path2
+ :100644 100644 X X R# a/path3 b/path3
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup move of files and subdirs to different places' '
+ git reset -q --hard &&
+ git mv a/c b &&
+ git mv a d
+'
+test_expect_success 'moving subdirs into one dir and files into another is not mistaken for dir move' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# a/c/* b/
+ :100644 100644 X X R# a/c/apath0 b/apath0
+ :100644 100644 X X R# a/c/apath1 b/apath1
+ :100644 100644 X X R# a/c/apath2 b/apath2
+ :100644 100644 X X R# a/path0 d/path0
+ :100644 100644 X X R# a/path1 d/path1
+ :100644 100644 X X R# a/path2 d/path2
+ :100644 100644 X X R# a/path3 d/path3
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+# the same with different ordering
+test_expect_success 'setup move of files and subdirs to different places' '
+ git mv d 0
+'
+test_expect_success 'moving subdirs into one dir and files into another is not mistaken for dir move' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# a/c/* b/
+ :100644 100644 X X R# a/path0 0/path0
+ :100644 100644 X X R# a/path1 0/path1
+ :100644 100644 X X R# a/path2 0/path2
+ :100644 100644 X X R# a/path3 0/path3
+ :100644 100644 X X R# a/c/apath0 b/apath0
+ :100644 100644 X X R# a/c/apath1 b/apath1
+ :100644 100644 X X R# a/c/apath2 b/apath2
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_expect_success 'setup move of dir with only subdirs' '
+ git reset -q --hard &&
+ mkdir a/b &&
+ mv a/path* a/b/ &&
+ git update-index --add --remove a/path0 a/path1 a/path2 a/path3 a/b/path* &&
+ test_tick &&
+ git commit -m "move all toplevel files down one level" &&
+
+ git mv a z
+'
+# TODO: only a suboptimal non-detection
+test_expect_failure 'moving a dir with no direct children files' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# a/* z/
+ :040000 040000 X X R# a/b/* z/b/
+ :040000 040000 X X R# a/c/* z/c/
+ :100644 100644 X X R# a/b/path0 z/b/path0
+ :100644 100644 X X R# a/b/path1 z/b/path1
+ :100644 100644 X X R# a/b/path2 z/b/path2
+ :100644 100644 X X R# a/b/path3 z/b/path3
+ :100644 100644 X X R# a/c/apath0 z/c/apath0
+ :100644 100644 X X R# a/c/apath1 z/c/apath1
+ :100644 100644 X X R# a/c/apath2 z/c/apath2
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+# now test moving all files from toplevel into subdir (does not hides file moves) (needs consensus on syntax)
+# Note: this is a special case of move of a dir into one of its own subdirs, which in
+# turn is a variant of new files/dirs being added into a dir after all its contents
+# are moved away
+
+test_expect_success 'setup move from toplevel to subdir' '
+ git reset -q --hard HEAD~3 &&
+ mv a/* . &&
+ git update-index --add --remove a/path0 a/path1 a/path2 a/path3 path* &&
+ test_tick &&
+ git commit -m "move all files to toplevel" &&
+
+ mkdir z &&
+ git mv path* z/
+'
+test_expect_success '--detect-bulk-moves everything from toplevel.' '
+ cat >expected <<-EOF &&
+ :040000 040000 X X R# ./* z/
+ :100644 100644 X X R# path0 z/path0
+ :100644 100644 X X R# path1 z/path1
+ :100644 100644 X X R# path2 z/path2
+ :100644 100644 X X R# path3 z/path3
+ EOF
+ git diff-index --detect-bulk-moves HEAD >current &&
+ compare_diff_raw expected current
+'
+
+test_done
--
1.7.2.3
^ permalink raw reply related
* [PATCH 3/6] Raw diff output format for bulk moves.
From: Yann Dirson @ 2010-12-09 21:38 UTC (permalink / raw)
To: git; +Cc: Yann Dirson, Yann Dirson
In-Reply-To: <1291930687-8990-1-git-send-email-ydirson@altern.org>
To distinguish the general bulk-move case (where the destination
directory was pre-existing) from the directory-rename case (where it
was not), the output of raw diff is displayed as "Rnnn a/* b/". Those
cannot be confused with renames of files named "whatever/*" with a
literal star character, from the full-zero SHA1's.
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
Documentation/gitdiffcore.txt | 2 +-
diff.c | 9 ++++++++-
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
index 93111ac..2538dc0 100644
--- a/Documentation/gitdiffcore.txt
+++ b/Documentation/gitdiffcore.txt
@@ -181,7 +181,7 @@ additional pass on top of the results of per-file rename detection.
They are reported with NULL SHA1 id, in addition to the file renames:
------------------------------------------------
-:040000 040000 0000000... 0000000... R100 foo/ bar/
+:040000 040000 0000000... 0000000... R100 foo/* bar/
:100644 100644 0123456... 1234567... R090 foo/file0 bar/file3
:100644 100644 2345678... 2345678... R100 foo/file1 bar/file1
:100644 100644 3456789... 3456789... R100 foo/file2 bar/file2
diff --git a/diff.c b/diff.c
index d64ae44..0694d7f 100644
--- a/diff.c
+++ b/diff.c
@@ -3499,7 +3499,14 @@ static void diff_flush_raw(struct diff_filepair *p, struct diff_options *opt)
if (p->status == DIFF_STATUS_COPIED ||
p->status == DIFF_STATUS_RENAMED) {
const char *name_a, *name_b;
- name_a = p->one->path;
+ if (p->is_bulkmove) {
+ /* append "*" to the first dirname */
+ char buf[PATH_MAX];
+ char *next = memccpy(buf, p->one->path, '\0', PATH_MAX);
+ next[-1] = '*'; *next = '\0';
+ name_a = buf;
+ } else
+ name_a = p->one->path;
name_b = p->two->path;
strip_prefix(opt->prefix_length, &name_a, &name_b);
write_name_quoted(name_a, opt->file, inter_name_termination);
--
1.7.2.3
^ permalink raw reply related
* [PATCH 2/6] Introduce bulk-move detection in diffcore.
From: Yann Dirson @ 2010-12-09 21:38 UTC (permalink / raw)
To: git; +Cc: Yann Dirson, Yann Dirson
In-Reply-To: <1291930687-8990-1-git-send-email-ydirson@altern.org>
This feature tries to group together files moving from and to
identical directories - a common case being directory renames.
This only adds the detection logic. The output of raw diff is displayed
as "Rnnn a/ b/", and unified diff does not display them at all. Output
formats will be refined later in the series.
It is implemented as a new pass in diffcore-rename, occuring after the
file renames get detected, grouping those renames looking like a move
of a full directory into some other place. It is activated by the new
--detect-bulk-moves diffcore flag.
Possible optimisations to this code include:
* avoid use of i_am_not_single by using a separate list
* use a more informative prefixcmp to avoid strcmp calls
eg. in discard_if_outside()
* optimize for bulk insertions (avoid useless successive memmove's)
Other future developements to be made on top of this include:
* detect bulk removals (well, that one is rather a subset than a layer above),
and possibly bulk additions
* detect bulk copies
* detect inexact bulk-moves/copies (where some files were not moved, or were
moved to a different place) - problem of computing a similarity score
* display as such the special case of directory move/rename
* application of such new diffs: issue a conflict, or just a warning ?
* teach git-svn (and others ?) to make use of that flag
* handle new conflict type "bulk-move/add"
* detect "directory splits" as well
* use inexact dir renames to bump score of below-threshold renames
from/to same locations
* support other types of bluk-grouping, like prefixes (see eg. kernel
5d1e859c), and maybe config-specified patterns
* add yours here
This patch has been improved by the following contributions:
- Jonathan Nieder: better implementation of copy_dirname()
- Jonathan Nieder: portable implementation of memrchr() in another patch
- Junio C Hamano: split individual renames hiding under control of another flag
- Junio C Hamano: coding style issues
- Junio C Hamano: better examples
- Ævar Arnfjörð Bjarmason: Don't use C99 comments.
- Jonathan Nieder: just too many other helpful suggestions to list them all
Thanks-to: Jonathan Nieder <jrnieder@gmail.com>
Thanks-to: Junio C Hamano <gitster@pobox.com>
Thanks-to: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Signed-off-by: Yann Dirson <ydirson@free.fr>
---
Documentation/diff-options.txt | 4 +
Documentation/gitdiffcore.txt | 12 ++
diff-lib.c | 6 +-
diff.c | 5 +
diff.h | 3 +
diffcore-rename.c | 322 ++++++++++++++++++++++++++++++++++++++--
diffcore.h | 1 +
tree-diff.c | 4 +-
8 files changed, 340 insertions(+), 17 deletions(-)
diff --git a/Documentation/diff-options.txt b/Documentation/diff-options.txt
index f3e9538..186cd6f 100644
--- a/Documentation/diff-options.txt
+++ b/Documentation/diff-options.txt
@@ -245,6 +245,10 @@ endif::git-log[]
delete/add pair to be a rename if more than 90% of the file
hasn't changed.
+--detect-bulk-moves::
+ Detect bulk move of all files of a directory into a
+ different one.
+
-C[<n>]::
--detect-copies[=<n>]::
Detect copies as well as renames. See also `--find-copies-harder`.
diff --git a/Documentation/gitdiffcore.txt b/Documentation/gitdiffcore.txt
index 6af29a4..93111ac 100644
--- a/Documentation/gitdiffcore.txt
+++ b/Documentation/gitdiffcore.txt
@@ -175,6 +175,18 @@ the expense of making it slower. Without `\--find-copies-harder`,
'git diff-{asterisk}' commands can detect copies only if the file that was
copied happened to have been modified in the same changeset.
+Bulk move of all files of a directory into a different one can get
+detected using the `\--detect-bulk-moves` option. This adds an
+additional pass on top of the results of per-file rename detection.
+They are reported with NULL SHA1 id, in addition to the file renames:
+
+------------------------------------------------
+:040000 040000 0000000... 0000000... R100 foo/ bar/
+:100644 100644 0123456... 1234567... R090 foo/file0 bar/file3
+:100644 100644 2345678... 2345678... R100 foo/file1 bar/file1
+:100644 100644 3456789... 3456789... R100 foo/file2 bar/file2
+------------------------------------------------
+
diffcore-merge-broken: For Putting "Complete Rewrites" Back Together
--------------------------------------------------------------------
diff --git a/diff-lib.c b/diff-lib.c
index 392ce2b..5ec3ddc 100644
--- a/diff-lib.c
+++ b/diff-lib.c
@@ -208,7 +208,8 @@ int run_diff_files(struct rev_info *revs, unsigned int option)
ce_option, &dirty_submodule);
if (!changed && !dirty_submodule) {
ce_mark_uptodate(ce);
- if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
+ if (!DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER) &&
+ !DIFF_OPT_TST(&revs->diffopt, DETECT_BULK_MOVES))
continue;
}
oldmode = ce->ce_mode;
@@ -338,7 +339,8 @@ static int show_modified(struct rev_info *revs,
oldmode = old->ce_mode;
if (mode == oldmode && !hashcmp(sha1, old->sha1) && !dirty_submodule &&
- !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER))
+ !DIFF_OPT_TST(&revs->diffopt, FIND_COPIES_HARDER) &&
+ !DIFF_OPT_TST(&revs->diffopt, DETECT_BULK_MOVES))
return 0;
diff_change(&revs->diffopt, oldmode, mode,
diff --git a/diff.c b/diff.c
index db5e844..d64ae44 100644
--- a/diff.c
+++ b/diff.c
@@ -3200,6 +3200,11 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
DIFF_OPT_SET(options, REVERSE_DIFF);
else if (!strcmp(arg, "--find-copies-harder"))
DIFF_OPT_SET(options, FIND_COPIES_HARDER);
+ else if (!strcmp(arg, "--detect-bulk-moves")) {
+ DIFF_OPT_SET(options, DETECT_BULK_MOVES);
+ if (!options->detect_rename)
+ options->detect_rename = DIFF_DETECT_RENAME;
+ }
else if (!strcmp(arg, "--follow"))
DIFF_OPT_SET(options, FOLLOW_RENAMES);
else if (!strcmp(arg, "--color"))
diff --git a/diff.h b/diff.h
index 0083d92..1e2506c 100644
--- a/diff.h
+++ b/diff.h
@@ -78,6 +78,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
#define DIFF_OPT_IGNORE_UNTRACKED_IN_SUBMODULES (1 << 25)
#define DIFF_OPT_IGNORE_DIRTY_SUBMODULES (1 << 26)
#define DIFF_OPT_OVERRIDE_SUBMODULE_CONFIG (1 << 27)
+#define DIFF_OPT_DETECT_BULK_MOVES (1 << 28)
#define DIFF_OPT_TST(opts, flag) ((opts)->flags & DIFF_OPT_##flag)
#define DIFF_OPT_SET(opts, flag) ((opts)->flags |= DIFF_OPT_##flag)
@@ -265,6 +266,8 @@ extern void diffcore_fix_diff_index(struct diff_options *);
" -C detect copies.\n" \
" --find-copies-harder\n" \
" try unchanged files as candidate for copy detection.\n" \
+" --detect-bulk-moves\n" \
+" detect moves of all files of a single directory.\n" \
" -l<n> limit rename attempts up to <n> paths.\n" \
" -O<file> reorder diffs according to the <file>.\n" \
" -S<string> find filepair whose only one side contains the string.\n" \
diff --git a/diffcore-rename.c b/diffcore-rename.c
index e16fdeb..44df490 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -28,6 +28,7 @@ void __debug_bulkmove(const char *fmt, ...)
struct diff_rename_dst {
struct diff_filespec *two;
struct diff_filepair *pair;
+ unsigned i_am_not_single:1; /* does not look for a match, only here to be looked at */
};
static int rename_dst_cmp(struct diff_filespec *ref_spec, struct diff_rename_dst *elem)
@@ -39,14 +40,23 @@ static void rename_dst_init(struct diff_rename_dst *elem, struct diff_filespec *
elem->two = alloc_filespec(ref_spec->path);
fill_filespec(elem->two, ref_spec->sha1, ref_spec->mode);
elem->pair = NULL;
+ elem->i_am_not_single = 0;
}
declare_sorted_array(static, struct diff_rename_dst, rename_dst);
declare_sorted_array_search_elem(static, struct diff_rename_dst, locate_rename_dst,
struct diff_filespec *,
rename_dst, rename_dst_cmp);
-declare_sorted_array_insert_checkbool(static, struct diff_rename_dst, register_rename_dst,
- struct diff_filespec *, _gen_locate_rename_dst,
- rename_dst, rename_dst_cmp, rename_dst_init);
+declare_sorted_array_insert_elem(static, struct diff_rename_dst, register_rename_dst,
+ struct diff_filespec *, _gen_locate_rename_dst,
+ rename_dst, rename_dst_cmp, rename_dst_init);
+
+static int rename_dst_dircmp(const char *ref_dirname, struct diff_rename_dst *elem)
+{
+ // FIXME: calls strlen many times - but maybe the compiler optimizes ?
+ return strncmp(ref_dirname, elem->two->path, strlen(ref_dirname));
+}
+declare_sorted_array_search_elem(static, struct diff_rename_dst, locate_rename_dst_dir,
+ const char *, rename_dst, rename_dst_dircmp);
/* Table of rename/copy src files */
@@ -361,8 +371,11 @@ static int find_exact_renames(void)
for (i = 0; i < rename_src_nr; i++)
insert_file_table(&file_table, -1, i, rename_src[i].one);
- for (i = 0; i < rename_dst_nr; i++)
+ for (i = 0; i < rename_dst_nr; i++) {
+ if (rename_dst[i].i_am_not_single)
+ continue;
insert_file_table(&file_table, 1, i, rename_dst[i].two);
+ }
/* Find the renames */
i = for_each_hash(&file_table, find_same_files);
@@ -389,6 +402,253 @@ static void record_if_better(struct diff_score m[], struct diff_score *o)
m[worst] = *o;
}
+struct diff_bulk_rename {
+ struct diff_filespec *one;
+ struct diff_filespec *two;
+ int discarded;
+};
+
+struct diff_bulk_rename_candidate {
+ char one_path[PATH_MAX];
+ char two_path[PATH_MAX];
+};
+static int bulkmove_candidates_cmp(struct diff_bulk_rename_candidate *ref_candidate,
+ struct diff_bulk_rename *elem)
+{
+ int cmp = strcmp(ref_candidate->one_path, elem->one->path);
+ if (!cmp)
+ cmp = strcmp(ref_candidate->two_path, elem->two->path);
+ return cmp;
+}
+static void bulkmove_candidates_init(struct diff_bulk_rename *elem,
+ struct diff_bulk_rename_candidate *ref_candidate)
+{
+ elem->one = alloc_filespec(ref_candidate->one_path);
+ fill_filespec(elem->one, null_sha1, S_IFDIR);
+ elem->two = alloc_filespec(ref_candidate->two_path);
+ fill_filespec(elem->two, null_sha1, S_IFDIR);
+ elem->discarded = 0;
+}
+declare_sorted_array(static, struct diff_bulk_rename, bulkmove_candidates);
+declare_sorted_array_insertonly_elem(static, struct diff_bulk_rename, register_bulkmove_candidate,
+ struct diff_bulk_rename_candidate *, bulkmove_candidates,
+ bulkmove_candidates_cmp, bulkmove_candidates_init);
+
+/*
+ * Copy dirname of src into dst, suitable to append a filename without
+ * an additional "/".
+ * Only handles relative paths since there is no absolute path in a git repo.
+ * Writes "" when there is no "/" in src.
+ * May overwrite more chars than really needed, if src ends with a "/".
+ * Supports in-place modification of src by passing dst == src.
+ */
+static const char *copy_dirname(char *dst, const char *src)
+{
+ size_t len = strlen(src);
+ const char *slash;
+ char *end;
+
+ if (len > 0 && src[len - 1] == '/')
+ /* Trailing slash. Ignore it. */
+ len--;
+
+ slash = memrchr(src, '/', len);
+ if (!slash) {
+ *dst = '\0';
+ return dst;
+ }
+
+ if (dst == src)
+ dst[slash - src + 1] = '\0';
+ else {
+ end = mempcpy(dst, src, slash - src + 1);
+ *end = '\0';
+ }
+ return dst;
+}
+
+// FIXME: leaks like hell.
+/* See if the fact that one_leftover exists under one_parent_path in
+ * dst tree should disqualify one_parent_path from bulkmove eligibility.
+ * Return 1 if it disqualifies, 0 if it is OK.
+ */
+static int dispatched_to_different_dirs(const char *one_parent_path)
+{
+ /* this might be a dir split, or files added
+ * after the bulk move, or just an isolated
+ * rename */
+ int two_idx, j, onep_len, maybe_dir_rename;
+ struct diff_rename_dst *one_leftover =
+ one_leftover = locate_rename_dst_dir(one_parent_path);
+
+ if (!one_leftover)
+ return 0;
+
+ /* try to see if it is a file added after the bulk move */
+ two_idx = one_leftover - rename_dst;
+ onep_len = strlen(one_parent_path);
+ maybe_dir_rename = 1;
+
+ /* check no leftover file was already here before */
+ for (j = two_idx; j < rename_dst_nr; j++) {
+ if (strncmp(rename_dst[j].two->path,
+ one_parent_path, onep_len))
+ break; /* exhausted directory in this direction */
+ debug_bulkmove(("leftover file %s in %s\n",
+ rename_dst[j].two->path, one_parent_path));
+ if (rename_dst[j].i_am_not_single || /* those were already here */
+ (rename_dst[j].pair &&
+ !strncmp(rename_dst[j].pair->one->path,
+ one_parent_path, onep_len) && /* renamed from here */
+ strncmp(rename_dst[j].two->path,
+ one_parent_path, onep_len))) { /* not to a subdir */
+ maybe_dir_rename = 0;
+ debug_bulkmove(("... tells not a bulk move\n"));
+ break;
+ }
+ debug_bulkmove(("... not believed to prevent bulk move\n"));
+ }
+ if (!maybe_dir_rename)
+ return 1;
+ /* try the other direction (dup code) */
+ for (j = two_idx-1; j >= 0; j--) {
+ if (strncmp(rename_dst[j].two->path,
+ one_parent_path, onep_len))
+ break; /* exhausted directory in this direction */
+ debug_bulkmove(("leftover file %s in '%s'\n",
+ rename_dst[j].two->path, one_parent_path));
+ if (rename_dst[j].i_am_not_single || /* those were already here */
+ (rename_dst[j].pair &&
+ !strncmp(rename_dst[j].pair->one->path,
+ one_parent_path, onep_len) && /* renamed from here */
+ strncmp(rename_dst[j].two->path,
+ one_parent_path, onep_len))) { /* not to a subdir */
+ maybe_dir_rename = 0;
+ debug_bulkmove(("... tells not a bulk move\n"));
+ break;
+ }
+ debug_bulkmove(("... not believed to prevent bulk move\n"));
+ }
+ if (!maybe_dir_rename)
+ return 1;
+
+ /* Here we are in the case where a directory
+ * content was completely moved, but files
+ * were added to it afterwards. Proceed as
+ * for a simple bulk move. */
+ return 0;
+}
+
+/*
+ * Assumes candidate->one is a subdir of seen->one, mark 'seen' as
+ * discarded if candidate->two is outside seen->two. Also mark
+ * 'candidate' itself as discarded if the conflict implies so.
+ *
+ * Return 1 if 'seen' was discarded
+ */
+static int discard_if_outside(struct diff_bulk_rename *candidate,
+ struct diff_bulk_rename *seen)
+{
+ if (!prefixcmp(candidate->two->path, seen->two->path)) {
+ debug_bulkmove((" 'dstpair' conforts 'seen'\n"));
+ return 0;
+ }
+
+ debug_bulkmove(("discarding %s -> %s from bulk moves (split into %s and %s)\n",
+ seen->one->path, seen->two->path,
+ candidate->two->path, seen->two->path));
+ seen->discarded = 1;
+ /* Need to discard dstpair as well, unless moving from
+ * a strict subdir of seen->one or to a strict subdir
+ * of seen->two */
+ if (!strcmp(seen->one->path, candidate->one->path) &&
+ prefixcmp(seen->two->path, candidate->two->path)) {
+ debug_bulkmove(("... and not adding self\n"));
+ candidate->discarded = 1;
+ }
+ return 1;
+}
+
+/*
+ * Check if the rename specified by "dstpair" could cause a
+ * bulk move to be detected, record it in bulkmove_candidates if yes.
+ */
+static void check_one_bulk_move(struct diff_filepair *dstpair)
+{
+ struct diff_bulk_rename_candidate rawcandidate;
+
+ /* genuine new files (or believed to be so) */
+ if (!dstpair)
+ return;
+ /* dummy renames used by copy detection */
+ if (!strcmp(dstpair->one->path, dstpair->two->path))
+ return;
+
+ copy_dirname(rawcandidate.one_path, dstpair->one->path);
+ copy_dirname(rawcandidate.two_path, dstpair->two->path);
+
+ /* simple rename with no directory change */
+ if (!strcmp(rawcandidate.one_path, rawcandidate.two_path))
+ return;
+
+ debug_bulkmove(("[] %s -> %s ?\n", dstpair->one->path, dstpair->two->path));
+
+ /* loop up rawcandidate.one_path over successive parents */
+ // FIXME: also loop over rawcandidate.two_path prefixes
+ do {
+ struct diff_bulk_rename *seen;
+ int old_nr = bulkmove_candidates_nr;
+ struct diff_bulk_rename *candidate =
+ register_bulkmove_candidate(&rawcandidate);
+ debug_bulkmove(("[[]] %s ...\n", rawcandidate.one_path));
+ if (old_nr == bulkmove_candidates_nr) {
+ debug_bulkmove((" already seen\n"));
+ return;
+ }
+
+ /* After this commit, are there any files still under rawcandidate.one_path ?
+ * Any file left would disqualifies this dir for bulk move.
+ */
+ if (dispatched_to_different_dirs(rawcandidate.one_path)) {
+ // FIXME: check overlap with discard_if_outside()
+ candidate->discarded = 1;
+ return;
+ }
+
+ /* walk up for rawcandidate.one_path prefixes */
+ for (seen = candidate-1; (seen >= bulkmove_candidates) &&
+ !prefixcmp(rawcandidate.one_path, seen->one->path); seen--) {
+ debug_bulkmove((" ? %s -> %s\n", seen->one->path, seen->two->path));
+ /* subdir of "seen" dest dir ? */
+ if (discard_if_outside(candidate, seen))
+ continue;
+ }
+ /* look down for other moves from rawcandidate.one_path */
+ seen = candidate + 1;
+ if (seen != bulkmove_candidates + bulkmove_candidates_nr &&
+ !strcmp(rawcandidate.one_path, seen->one->path)) {
+ debug_bulkmove((" ? %s -> %s (2)\n", seen->one->path, seen->two->path));
+ /* subdir of "seen" dest dir ? */
+ if (discard_if_outside(candidate, seen))
+ continue;
+ }
+
+ /* next parent if any */
+ copy_dirname(rawcandidate.one_path, rawcandidate.one_path);
+ } while (*rawcandidate.one_path);
+}
+
+/*
+ * Take all file renames recorded so far and check if they could cause
+ * a bulk move to be detected.
+ */
+static void diffcore_bulk_moves(void)
+{
+ int i;
+ for (i = 0; i < rename_dst_nr; i++)
+ check_one_bulk_move(rename_dst[i].pair);
+}
+
void diffcore_rename(struct diff_options *options)
{
int detect_rename = options->detect_rename;
@@ -399,6 +659,7 @@ void diffcore_rename(struct diff_options *options)
struct diff_score *mx;
int i, j, rename_count;
int num_create, num_src, dst_cnt;
+ struct diff_bulk_rename *candidate;
if (!minimum_score)
minimum_score = DEFAULT_RENAME_SCORE;
@@ -413,8 +674,7 @@ void diffcore_rename(struct diff_options *options)
continue; /* not interested */
else
register_rename_dst(p->two);
- }
- else if (!DIFF_FILE_VALID(p->two)) {
+ } else if (!DIFF_FILE_VALID(p->two)) {
/*
* If the source is a broken "delete", and
* they did not really want to get broken,
@@ -425,14 +685,23 @@ void diffcore_rename(struct diff_options *options)
if (p->broken_pair && !p->score)
p->one->rename_used++;
register_rename_src(p);
- }
- else if (detect_rename == DIFF_DETECT_COPY) {
- /*
- * Increment the "rename_used" score by
- * one, to indicate ourselves as a user.
- */
- p->one->rename_used++;
- register_rename_src(p);
+ } else {
+ if (detect_rename == DIFF_DETECT_COPY) {
+ /*
+ * Increment the "rename_used" score by
+ * one, to indicate ourselves as a user.
+ */
+ p->one->rename_used++;
+ register_rename_src(p);
+ }
+ if (DIFF_OPT_TST(options, DETECT_BULK_MOVES)) {
+ /* similarly, bulk move detection needs to
+ * see all files from second tree, but we don't
+ * want them to be matched against single sources.
+ */
+ // FIXME: check interaction with --find-copies-harder
+ register_rename_dst(p->two)->i_am_not_single = 1;
+ }
}
}
if (rename_dst_nr == 0 || rename_src_nr == 0)
@@ -484,6 +753,8 @@ void diffcore_rename(struct diff_options *options)
if (rename_dst[i].pair)
continue; /* dealt with exact match already. */
+ if (rename_dst[i].i_am_not_single)
+ continue; /* not looking for a match. */
m = &mx[dst_cnt * NUM_CANDIDATE_PER_DST];
for (j = 0; j < NUM_CANDIDATE_PER_DST; j++)
@@ -544,7 +815,30 @@ void diffcore_rename(struct diff_options *options)
/* At this point, we have found some renames and copies and they
* are recorded in rename_dst. The original list is still in *q.
*/
+
+ /* Now possibly factorize those renames and copies. */
+ if (DIFF_OPT_TST(options, DETECT_BULK_MOVES))
+ diffcore_bulk_moves();
+
DIFF_QUEUE_CLEAR(&outq);
+
+ /* Now turn non-discarded bulkmove_candidates into real renames */
+ for (candidate = bulkmove_candidates;
+ candidate < bulkmove_candidates + bulkmove_candidates_nr; candidate++) {
+ struct diff_filepair* pair;
+ if (candidate->discarded)
+ continue;
+ /* visualize toplevel dir if needed */
+ if (!*candidate->one->path)
+ candidate->one->path = "./";
+ if (!*candidate->two->path)
+ candidate->two->path = "./";
+ pair = diff_queue(&outq, candidate->one, candidate->two);
+ pair->score = MAX_SCORE;
+ pair->renamed_pair = 1;
+ pair->is_bulkmove = 1;
+ }
+
for (i = 0; i < q->nr; i++) {
struct diff_filepair *p = q->queue[i];
struct diff_filepair *pair_to_free = NULL;
diff --git a/diffcore.h b/diffcore.h
index b8f1fde..6dab95b 100644
--- a/diffcore.h
+++ b/diffcore.h
@@ -69,6 +69,7 @@ struct diff_filepair {
unsigned broken_pair : 1;
unsigned renamed_pair : 1;
unsigned is_unmerged : 1;
+ unsigned is_bulkmove : 1;
};
#define DIFF_PAIR_UNMERGED(p) ((p)->is_unmerged)
diff --git a/tree-diff.c b/tree-diff.c
index 12c9a88..89cedd4 100644
--- a/tree-diff.c
+++ b/tree-diff.c
@@ -49,7 +49,9 @@ static int compare_tree_entry(struct tree_desc *t1, struct tree_desc *t2, const
show_entry(opt, "+", t2, base, baselen);
return 1;
}
- if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) && !hashcmp(sha1, sha2) && mode1 == mode2)
+ if (!DIFF_OPT_TST(opt, FIND_COPIES_HARDER) &&
+ !DIFF_OPT_TST(opt, DETECT_BULK_MOVES) &&
+ !hashcmp(sha1, sha2) && mode1 == mode2)
return 0;
/*
--
1.7.2.3
^ permalink raw reply related
* [PATCH 01/18] gitweb: Prepare for splitting gitweb
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
From: Jakub Narebski <jnareb@gmail.com>
Prepare gitweb for having been split into modules that are to be
installed alongside gitweb in 'lib/' subdirectory, by adding
use lib __DIR__.'/lib';
to gitweb.perl (to main gitweb script), and preparing for putting
modules (relative path) in $(GITWEB_MODULES) in gitweb/Makefile.
This preparatory work allows to add new module to gitweb by simply
adding
GITWEB_MODULES += <module>
to gitweb/Makefile (assuming that the module is in 'gitweb/lib/'
directory).
While at it pass GITWEBLIBDIR in addition to GITWEB_TEST_INSTALLED
to test instaleed version of gitweb and installed version of modules
(for tests which check individual (sub)modules).
Using __DIR__ from Dir::Self module (not in core, that's why currently
gitweb includes excerpt of code from Dir::Self defining __DIR__) was
chosen over using FindBin-based solution (in core since perl 5.00307,
while gitweb itself requires at least perl 5.8.0) because FindBin uses
BEGIN block, which is a problem under mod_perl and other persistent
Perl environments (thought there are workarounds).
At Pavan Kumar Sankara suggestion gitweb/Makefile uses
install [OPTION]... SOURCE... DIRECTORY
format (2nd format) with single SOURCE rather than
install [OPTION]... SOURCE DEST
format (1st format) because of security reasons (race conditions).
Modern GNU install has `-T' / `--no-target-directory' option, but we
cannot rely that the $(INSTALL) we are using supports this option.
The install-modules target in gitweb/Makefile uses shell 'for' loop,
instead of make's $(foreach) function, to avoid possible problem with
generating a command line that exceeded the maximum argument list
length.
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
gitweb/Makefile | 17 +++++++++++++++--
gitweb/gitweb.perl | 8 ++++++++
2 files changed, 23 insertions(+), 2 deletions(-)
diff --git a/gitweb/Makefile b/gitweb/Makefile
index 0a6ac00..f9e32eb 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -57,6 +57,7 @@ PERL_PATH ?= /usr/bin/perl
bindir_SQ = $(subst ','\'',$(bindir))#'
gitwebdir_SQ = $(subst ','\'',$(gitwebdir))#'
gitwebstaticdir_SQ = $(subst ','\'',$(gitwebdir)/static)#'
+gitweblibdir_SQ = $(subst ','\'',$(gitwebdir)/lib)#'
SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))#'
PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH))#'
DESTDIR_SQ = $(subst ','\'',$(DESTDIR))#'
@@ -153,20 +154,32 @@ test:
test-installed:
GITWEB_TEST_INSTALLED='$(DESTDIR_SQ)$(gitwebdir_SQ)' \
+ GITWEBLIBDIR='$(DESTDIR_SQ)$(gitweblibdir_SQ)' \
$(MAKE) -C ../t gitweb-test
### Installation rules
-install: all
+install: all install-modules
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)'
$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
$(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
+install-modules:
+ install_dirs="$(sort $(dir $(GITWEB_MODULES)))" && \
+ for dir in $$install_dirs; do \
+ test -d '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$dir' || \
+ $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$dir'; \
+ done
+ gitweb_modules="$(GITWEB_MODULES)" && \
+ for mod in $$gitweb_modules; do \
+ $(INSTALL) -m 644 lib/$$mod '$(DESTDIR_SQ)$(gitweblibdir_SQ)/$$(dirname $$mod)'; \
+ done
+
### Cleaning rules
clean:
$(RM) gitweb.cgi static/gitweb.min.js static/gitweb.min.css GITWEB-BUILD-OPTIONS
-.PHONY: all clean install test test-installed .FORCE-GIT-VERSION-FILE FORCE
+.PHONY: all clean install install-modules test test-installed .FORCE-GIT-VERSION-FILE FORCE
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 679f2da..cfa511c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -10,6 +10,14 @@
use 5.008;
use strict;
use warnings;
+
+use File::Spec;
+# __DIR__ is taken from Dir::Self __DIR__ fragment
+sub __DIR__ () {
+ File::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);
+}
+use lib __DIR__ . '/lib';
+
use CGI qw(:standard :escapeHTML -nosticky);
use CGI::Util qw(unescape);
use CGI::Carp qw(fatalsToBrowser set_message);
--
1.7.2.3
^ permalink raw reply related
* [PATCH 04/18] gitweb: Minimal testing of gitweb caching
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
From: Jakub Narebski <jnareb@gmail.com>
Add basic tests of caching support to t9500-gitweb-standalone-no-errors
test: set $caching_enabled to true and check for errors for first time
run (generating cache) and second time run (retrieving from cache) for a
single view - summary view for a project. Check also that request for
non-existent object (which results in die_error() codepath to be called)
doesn't produce errors.
Check in t9501-gitweb-standalone-http-status that request for
non-existent object produces correct output (HTTP headers and HTML
output) also when caching is enabled.
Check in the t9502-gitweb-standalone-parse-output test that gitweb
produces the same output with and without caching, for first and
second run, with binary (raw) or plain text (utf8) output.
The common routine that enables cache, gitweb_enable_caching, is
defined in t/gitweb-lib.sh
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
t/gitweb-lib.sh | 15 +++++++++++++
t/t9500-gitweb-standalone-no-errors.sh | 20 +++++++++++++++++
t/t9501-gitweb-standalone-http-status.sh | 13 +++++++++++
t/t9502-gitweb-standalone-parse-output.sh | 33 +++++++++++++++++++++++++++++
4 files changed, 81 insertions(+), 0 deletions(-)
mode change 100644 => 100755 t/gitweb-lib.sh
diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
old mode 100644
new mode 100755
index b9bb95f..16ce811
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -52,6 +52,21 @@ EOF
export SCRIPT_NAME
}
+gitweb_enable_caching () {
+ test_expect_success 'enable caching' '
+ cat >>gitweb_config.perl <<-\EOF &&
+ our $caching_enabled = 1;
+ our $minCacheTime = 60*60*24*7*30; # very long expiration time for tests (a month)
+ our $maxCacheTime = 60*60*24*7*30*365; # upper bound for dynamic (adaptive) caching
+ our $cachedir = "cache"; # for testsuite to clear the right thing
+ # required, because otherwise some tests might intermittently not pass
+ our $backgroundCache = 0; # should turn off cacheWaitForUpdate() / "Generating..."
+ #our $cacheDoFork = 0;
+ EOF
+ rm -rf cache/
+ '
+}
+
gitweb_run () {
GATEWAY_INTERFACE='CGI/1.1'
HTTP_ACCEPT='*/*'
diff --git a/t/t9500-gitweb-standalone-no-errors.sh b/t/t9500-gitweb-standalone-no-errors.sh
index 21cd286..86c1b50 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -677,4 +677,24 @@ test_expect_success HIGHLIGHT \
gitweb_run "p=.git;a=blob;f=test.sh"'
test_debug 'cat gitweb.log'
+# ----------------------------------------------------------------------
+# caching
+
+gitweb_enable_caching
+
+test_expect_success \
+ 'caching enabled (project summary, first run, generating cache)' \
+ 'gitweb_run "p=.git;a=summary"'
+test_debug 'cat gitweb.log'
+
+test_expect_success \
+ 'caching enabled (project summary, second run, cached version)' \
+ 'gitweb_run "p=.git;a=summary"'
+test_debug 'cat gitweb.log'
+
+test_expect_success \
+ 'caching enabled (non-existent commit, non-cache error page)' \
+ 'gitweb_run "p=.git;a=commit;h=non-existent"'
+test_debug 'cat gitweb.log'
+
test_done
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 2487da1..5b1df3f 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -134,5 +134,18 @@ cat >>gitweb_config.perl <<\EOF
our $maxload = undef;
EOF
+# ----------------------------------------------------------------------
+# output caching
+
+gitweb_enable_caching
+
+test_expect_success 'caching enabled (non-existent commit, 404 error)' '
+ gitweb_run "p=.git;a=commit;h=non-existent" &&
+ grep "Status: 404 Not Found" gitweb.headers &&
+ grep "404 - Unknown commit object" gitweb.body
+'
+test_debug 'echo "log" && cat gitweb.log'
+test_debug 'echo "headers" && cat gitweb.headers'
+test_debug 'echo "body" && cat gitweb.body'
test_done
diff --git a/t/t9502-gitweb-standalone-parse-output.sh b/t/t9502-gitweb-standalone-parse-output.sh
index dd83890..bc8eb01 100755
--- a/t/t9502-gitweb-standalone-parse-output.sh
+++ b/t/t9502-gitweb-standalone-parse-output.sh
@@ -112,4 +112,37 @@ test_expect_success 'snapshot: hierarchical branch name (xx/test)' '
'
test_debug 'cat gitweb.headers'
+
+# ----------------------------------------------------------------------
+# whether gitweb with caching enabled produces the same output
+
+test_expect_success 'setup for caching tests (utf8 patch, binary file)' '
+ . "$TEST_DIRECTORY"/t3901-utf8.txt &&
+ cp "$TEST_DIRECTORY"/test9200a.png image.png &&
+ git add image.png &&
+ git commit -F "$TEST_DIRECTORY"/t3900/1-UTF-8.txt &&
+ gitweb_run "p=.git;a=patch" &&
+ mv gitweb.body no_cache.txt &&
+ gitweb_run "p=.git;a=blob_plain;f=image.png" &&
+ mv gitweb.body no_cache.png
+'
+
+gitweb_enable_caching
+
+for desc in 'generating cache' 'cached version'; do
+ test_expect_success "caching enabled, plain text (utf8) output, $desc" '
+ gitweb_run "p=.git;a=patch" &&
+ mv gitweb.body cache.txt &&
+ test_cmp no_cache.txt cache.txt
+ '
+done
+
+for desc in 'generating cache' 'cached version'; do
+ test_expect_success "caching enabled, binary output (raw), $desc" '
+ gitweb_run "p=.git;a=blob_plain;f=image.png" &&
+ mv gitweb.body cache.png &&
+ cmp no_cache.png cache.png
+ '
+done
+
test_done
--
1.7.2.3
^ permalink raw reply related
* [PATCH 07/18] gitweb: Revert back to $cache_enable vs. $caching_enabled
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
Simple enough, $cache_enable (along with all caching variables) are
already in production in multiple places and doing a small semantic
change without backwards compatibility is pointless breakage.
This reverts back to the previous variable to enable / disable caching
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/gitweb.perl | 12 ++++++------
t/gitweb-lib.sh | 2 +-
2 files changed, 7 insertions(+), 7 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 05e7ba6..5eb0309 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -258,7 +258,7 @@ our $maxload = 300;
# that the cache directory be periodically completely deleted, and this is safe to perform.
# Suggested mechanism
# mv $cacheidr $cachedir.flush;mkdir $cachedir;rm -rf $cachedir.flush
-our $caching_enabled = 0;
+our $cache_enable = 0;
# Used to set the minimum cache timeout for the dynamic caching algorithm. Basically
# if we calculate the cache to be under this number of seconds we set the cache timeout
@@ -1138,7 +1138,7 @@ sub dispatch {
!$project) {
die_error(400, "Project needed");
}
- if ($caching_enabled && is_cacheable($action)) {
+ if ($cache_enable && is_cacheable($action)) {
cache_fetch($action);
} else {
$actions{$action}->();
@@ -3516,7 +3516,7 @@ sub git_header_html {
if (defined $cgi->http('HTTP_ACCEPT') &&
$cgi->http('HTTP_ACCEPT') =~ m/(,|;|\s|^)application\/xhtml\+xml(,|;|\s|$)/ &&
$cgi->Accept('application/xhtml+xml') != 0 &&
- !$caching_enabled) {
+ !$cache_enable) {
$content_type = 'application/xhtml+xml';
} else {
$content_type = 'text/html';
@@ -3757,7 +3757,7 @@ sub die_error {
#
# Reset the output so that we are actually going to STDOUT as opposed
# to buffering the output.
- reset_output() if ($caching_enabled);
+ reset_output() if ($cache_enable && ! $cacheErrorCache);
git_header_html($http_responses{$status}, undef, %opts);
print <<EOF;
@@ -5667,7 +5667,7 @@ sub git_blob_plain {
($sandbox ? 'attachment' : 'inline')
. '; filename="' . $save_as . '"');
local $/ = undef;
- if ($caching_enabled) {
+ if ($cache_enable) {
open BINOUT, '>', $fullhashbinpath or die_error(500, "Could not open bin dump file");
}else{
open BINOUT, '>&', \$fullhashbinpath or die_error(500, "Could not open bin dump file");
@@ -5960,7 +5960,7 @@ sub git_snapshot {
open my $fd, "-|", $cmd
or die_error(500, "Execute git-archive failed");
- if ($caching_enabled) {
+ if ($cache_enable) {
open BINOUT, '>', $fullhashbinpath or die_error(500, "Could not open bin dump file");
}else{
open BINOUT, '>&', \$fullhashbinpath or die_error(500, "Could not open bin dump file");
diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
index 10c4a3d..a0f7696 100755
--- a/t/gitweb-lib.sh
+++ b/t/gitweb-lib.sh
@@ -56,7 +56,7 @@ EOF
gitweb_enable_caching () {
test_expect_success 'enable caching' '
cat >>gitweb_config.perl <<-\EOF &&
- our $caching_enabled = 1;
+ our $cache_enable = 1;
our $minCacheTime = 60*60*24*7*30; # very long expiration time for tests (a month)
our $maxCacheTime = 60*60*24*7*30*365; # upper bound for dynamic (adaptive) caching
our $cachedir = "cache"; # for testsuite to clear the right thing
--
1.7.2.3
^ permalink raw reply related
* [PATCH 05/18] gitweb: Regression fix concerning binary output of files
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
This solves the regression introduced with v7.2 of the gitweb-caching code,
fix proposed by Jakub in his e-mail.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/gitweb.perl | 4 ++--
1 files changed, 2 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 3c3ff08..f2ef3da 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -5664,7 +5664,7 @@ sub git_blob_plain {
if ($caching_enabled) {
open BINOUT, '>', $fullhashbinpath or die_error(500, "Could not open bin dump file");
}else{
- open BINOUT, '>', \$fullhashbinpath or die_error(500, "Could not open bin dump file");
+ open BINOUT, '>&', \$fullhashbinpath or die_error(500, "Could not open bin dump file");
}
binmode BINOUT, ':raw';
print BINOUT <$fd>;
@@ -5957,7 +5957,7 @@ sub git_snapshot {
if ($caching_enabled) {
open BINOUT, '>', $fullhashbinpath or die_error(500, "Could not open bin dump file");
}else{
- open BINOUT, '>', \$fullhashbinpath or die_error(500, "Could not open bin dump file");
+ open BINOUT, '>&', \$fullhashbinpath or die_error(500, "Could not open bin dump file");
}
binmode BINOUT, ':raw';
print BINOUT <$fd>;
--
1.7.2.3
^ permalink raw reply related
* [PATCH 09/18] gitweb: Revert reset_output() back to original code
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
Reverted change to reset_output as
open STDOUT, ">&", \*STDOUT_REAL;
causes assertion failures:
Assertion !((((s->var)->sv_flags & (0x00004000|0x00008000)) == 0x00008000) && (((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVGV || ((svtype)((s->var)->sv_flags & 0xff)) == SVt_PVLV)) failed: file "scalar.xs", line 49 at gitweb.cgi line 1221.
if we encounter an error *BEFORE* we've ever changed the output.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/gitweb.perl | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 1d8bc74..e8c028b 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1225,7 +1225,7 @@ sub change_output {
sub reset_output {
# This basically takes STDOUT_REAL and puts it back as STDOUT
- open STDOUT, ">&", \*STDOUT_REAL;
+ open(STDOUT,">&STDOUT_REAL");
}
sub run {
--
1.7.2.3
^ permalink raw reply related
* [PATCH 08/18] gitweb: Change is_cacheable() to return true always
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
is_cacheable() was set to return false for blame or blame_incremental
which both use unique urls so there's no reason this shouldn't pass
through the caching engine.
Leaving the function in place for now should something actually arrise
that we can't use caching for (think ajaxy kinda things likely).
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/gitweb.perl | 3 ++-
1 files changed, 2 insertions(+), 1 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 5eb0309..1d8bc74 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -798,7 +798,8 @@ our %actions = (
);
sub is_cacheable {
my $action = shift;
- return !($action eq 'blame_data' || $action eq 'blame_incremental');
+ # There are no known actions that do no involve a unique URL that shouldn't be cached.
+ return 1;
}
# finally, we have the hash of allowed extra_options for the commands that
--
1.7.2.3
^ permalink raw reply related
* [PATCH 12/18] gitweb: Change file handles (in caching) to lexical variables as opposed to globs
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
This isn't a huge change, it just adds global variables for the file handles,
an additional cleanup to localize the variable a bit more which should alleviate
the issues that Jakub had with my original approach.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/lib/cache.pl | 114 +++++++++++++++++++++++++++++++-------------------
1 files changed, 71 insertions(+), 43 deletions(-)
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index 5182a94..fafc028 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -14,6 +14,12 @@ use Digest::MD5 qw(md5 md5_hex md5_base64);
use Fcntl ':flock';
use File::Copy;
+# Global declarations
+our $cacheFile;
+our $cacheFileBG;
+our $cacheFileBinWT;
+our $cacheFileBin;
+
sub cache_fetch {
my ($action) = @_;
my $cacheTime = 0;
@@ -49,9 +55,9 @@ sub cache_fetch {
}else{
#if cache is out dated, update
#else displayCache();
- open(cacheFile, '<', "$fullhashpath");
- stat(cacheFile);
- close(cacheFile);
+ open($cacheFile, '<', "$fullhashpath");
+ stat($cacheFile);
+ close($cacheFile);
my $stat_time = (stat(_))[9];
my $stat_size = (stat(_))[7];
@@ -150,13 +156,13 @@ sub cacheUpdate {
my $fileData = "";
if($backgroundCache){
- open(cacheFileBG, '>:utf8', "$fullhashpath.bg");
- my $lockStatBG = flock(cacheFileBG,LOCK_EX|LOCK_NB);
+ open($cacheFileBG, '>:utf8', "$fullhashpath.bg");
+ my $lockStatBG = flock($cacheFileBG,LOCK_EX|LOCK_NB);
$lockStatus = $lockStatBG;
}else{
- open(cacheFile, '>:utf8', \$fullhashpath);
- my $lockStat = flock(cacheFile,LOCK_EX|LOCK_NB);
+ open($cacheFile, '>:utf8', \$fullhashpath);
+ my $lockStat = flock($cacheFile,LOCK_EX|LOCK_NB);
$lockStatus = $lockStat;
}
@@ -172,8 +178,8 @@ sub cacheUpdate {
}
if( isBinaryAction($action) ){
- my $openstat = open(cacheFileBinWT, '>>:utf8', "$fullhashbinpath");
- my $lockStatBin = flock(cacheFileBinWT,LOCK_EX|LOCK_NB);
+ my $openstat = open($cacheFileBinWT, '>>:utf8', "$fullhashbinpath");
+ my $lockStatBin = flock($cacheFileBinWT,LOCK_EX|LOCK_NB);
}
# Trap all output from the action
@@ -186,8 +192,8 @@ sub cacheUpdate {
if($backgroundCache){
- open(cacheFile, '>:utf8', "$fullhashpath");
- $lockStat = flock(cacheFile,LOCK_EX);
+ open($cacheFile, '>:utf8', "$fullhashpath");
+ $lockStat = flock($cacheFile,LOCK_EX);
if (! $lockStat ){
if ( $areForked ){
@@ -199,8 +205,8 @@ sub cacheUpdate {
}
if( isBinaryAction($action) ){
- my $openstat = open(cacheFileBinFINAL, '>:utf8', "$fullhashbinpathfinal");
- $lockStatBIN = flock(cacheFileBinFINAL,LOCK_EX);
+ my $openstat = open($cacheFileBinFINAL, '>:utf8', "$fullhashbinpathfinal");
+ $lockStatBIN = flock($cacheFileBinFINAL,LOCK_EX);
if (! $lockStatBIN ){
if ( $areForked ){
@@ -214,24 +220,24 @@ sub cacheUpdate {
# Actually dump the output to the proper file handler
local $/ = undef;
$|++;
- print cacheFile "$output";
+ print $cacheFile "$output";
$|--;
if( isBinaryAction($action) ){
move("$fullhashbinpath", "$fullhashbinpathfinal") or die "Binary Cache file could not be updated: $!";
- flock(cacheFileBinFINAL,LOCK_UN);
- close(cacheFileBinFINAL);
+ flock($cacheFileBinFINAL,LOCK_UN);
+ close($cacheFileBinFINAL);
- flock(cacheFileBinWT,LOCK_UN);
- close(cacheFileBinWT);
+ flock($cacheFileBinWT,LOCK_UN);
+ close($cacheFileBinWT);
}
- flock(cacheFile,LOCK_UN);
- close(cacheFile);
+ flock($cacheFile,LOCK_UN);
+ close($cacheFile);
if($backgroundCache){
- flock(cacheFileBG,LOCK_UN);
- close(cacheFileBG);
+ flock($cacheFileBG,LOCK_UN);
+ close($cacheFileBG);
}
if ( $areForked ){
@@ -250,10 +256,10 @@ sub cacheWaitForUpdate {
if( $backgroundCache ){
if( -e "$fullhashpath" ){
- open(cacheFile, '<:utf8', "$fullhashpath");
- $lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
- stat(cacheFile);
- close(cacheFile);
+ open($cacheFile, '<:utf8', "$fullhashpath");
+ $lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);
+ stat($cacheFile);
+ close($cacheFile);
if( $lockStat && ( (stat(_))[9] > (time - $maxCacheLife) ) ){
cacheDisplay($action);
@@ -271,9 +277,9 @@ sub cacheWaitForUpdate {
){
do {
sleep 2 if $x > 0;
- open(cacheFile, '<:utf8', "$fullhashpath");
- $lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
- close(cacheFile);
+ open($cacheFile, '<:utf8', "$fullhashpath");
+ $lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);
+ close($cacheFile);
$x++;
$combinedLockStat = $lockStat;
} while ((! $combinedLockStat) && ($x < $max));
@@ -326,9 +332,9 @@ EOF
do {
print ".";
sleep 2 if $x > 0;
- open(cacheFile, '<:utf8', "$fullhashpath");
- $lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
- close(cacheFile);
+ open($cacheFile, '<:utf8', "$fullhashpath");
+ $lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);
+ close($cacheFile);
$x++;
$combinedLockStat = $lockStat;
} while ((! $combinedLockStat) && ($x < $max));
@@ -339,41 +345,63 @@ EOF
return;
}
+sub cacheDisplayErr {
+
+ return if ( ! -e "$fullhashpath.err" );
+
+ open($cacheFileErr, '<:utf8', "$fullhashpath.err");
+ $lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
+
+ if (! $lockStatus ){
+ show_warning(
+ "<p>".
+ "<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
+ "This is about as screwed up as it gets folks - see your systems administrator for more help with this.".
+ "<p>"
+ );
+ }
+
+ while( <$cacheFileErr> ){
+ print $_;
+ }
+ exit(0);
+}
+
sub cacheDisplay {
local $/ = undef;
$|++;
my ($action) = @_;
- open(cacheFile, '<:utf8', "$fullhashpath");
- $lockStat = flock(cacheFile,LOCK_SH|LOCK_NB);
+ open($cacheFile, '<:utf8', "$fullhashpath");
+ $lockStat = flock($cacheFile,LOCK_SH|LOCK_NB);
if (! $lockStat ){
- close(cacheFile);
+ close($cacheFile);
cacheWaitForUpdate($action);
}
if( isBinaryAction($action) ){
- my $openstat = open(cacheFileBin, '<', "$fullhashbinpathfinal");
- $lockStatBIN = flock(cacheFileBin,LOCK_SH|LOCK_NB);
+ my $openstat = open($cacheFileBin, '<', "$fullhashbinpathfinal");
+ $lockStatBIN = flock($cacheFileBin,LOCK_SH|LOCK_NB);
if (! $lockStatBIN ){
- close(cacheFile);
- close(cacheFileBin);
+ close($cacheFile);
+ close($cacheFileBin);
cacheWaitForUpdate($action);
}
my $binfilesize = -s "$fullhashbinpathfinal";
print "Content-Length: $binfilesize";
}
- while( <cacheFile> ){
+ while( <$cacheFile> ){
print $_;
}
if( isBinaryAction($action) ){
binmode STDOUT, ':raw';
- print <cacheFileBin>;
+ print <$cacheFileBin>;
binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
- close(cacheFileBin);
+ close($cacheFileBin);
}
- close(cacheFile);
+ close($cacheFile);
$|--;
}
--
1.7.2.3
^ permalink raw reply related
* [PATCH 10/18] gitweb: Adding isBinaryAction() and isFeedAction() to determine the action type
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
This is fairly self explanitory, these are here just to centralize the checking
for these types of actions, as special things need to be done with regards to
them inside the caching engine.
isBinaryAction() returns true if the action deals with creating binary files
(this needing :raw output)
isFeedAction() returns true if the action deals with a news feed of some sort,
basically used to bypass the 'Generating...' message should it be a news reader
as those will explode badly on that page.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/lib/cache.pl | 69 ++++++++++++++++++++++++++-------------------------
1 files changed, 35 insertions(+), 34 deletions(-)
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index a8ee99e..d55b572 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -88,6 +88,34 @@ sub cache_fetch {
#$actions{$action}->();
}
+sub isBinaryAction {
+ my ($action) = @_;
+
+ if(
+ $action eq "snapshot"
+ ||
+ $action eq "blob_plain"
+ ){
+ return 1; # True
+ }
+
+ return 0; # False
+}
+
+sub isFeedAction {
+ if(
+ $action eq "atom"
+ ||
+ $action eq "rss"
+ ||
+ $action eq "opml"
+ ){
+ return 1; # True
+ }
+
+ return 0; # False
+}
+
sub cacheUpdate {
my ($action,$areForked) = @_;
my $lockingStatus;
@@ -115,11 +143,7 @@ sub cacheUpdate {
}
}
- if(
- $action eq "snapshot"
- ||
- $action eq "blob_plain"
- ){
+ if( isBinaryAction($action) ){
my $openstat = open(cacheFileBinWT, '>>:utf8', "$fullhashbinpath");
my $lockStatBin = flock(cacheFileBinWT,LOCK_EX|LOCK_NB);
}
@@ -146,11 +170,7 @@ sub cacheUpdate {
}
}
- if(
- $action eq "snapshot"
- ||
- $action eq "blob_plain"
- ){
+ if( isBinaryAction($action) ){
my $openstat = open(cacheFileBinFINAL, '>:utf8', "$fullhashbinpathfinal");
$lockStatBIN = flock(cacheFileBinFINAL,LOCK_EX);
@@ -168,11 +188,7 @@ sub cacheUpdate {
$|++;
print cacheFile "$output";
$|--;
- if(
- $action eq "snapshot"
- ||
- $action eq "blob_plain"
- ){
+ if( isBinaryAction($action) ){
move("$fullhashbinpath", "$fullhashbinpathfinal") or die "Binary Cache file could not be updated: $!";
flock(cacheFileBinFINAL,LOCK_UN);
@@ -219,14 +235,10 @@ sub cacheWaitForUpdate {
}
if(
- $action eq "atom"
- ||
- $action eq "rss"
- ||
- $action eq "opml"
+ isFeedAction($action)
||
! $cacheGenStatus
- ){
+ ){
do {
sleep 2 if $x > 0;
open(cacheFile, '<:utf8', "$fullhashpath");
@@ -310,17 +322,10 @@ sub cacheDisplay {
cacheWaitForUpdate($action);
}
- if(
- (
- $action eq "snapshot"
- ||
- $action eq "blob_plain"
- )
- ){
+ if( isBinaryAction($action) ){
my $openstat = open(cacheFileBin, '<', "$fullhashbinpathfinal");
$lockStatBIN = flock(cacheFileBin,LOCK_SH|LOCK_NB);
if (! $lockStatBIN ){
- system ("echo 'cacheDisplay - bailing due to binary lock failure' >> /tmp/gitweb.log");
close(cacheFile);
close(cacheFileBin);
cacheWaitForUpdate($action);
@@ -332,11 +337,7 @@ sub cacheDisplay {
while( <cacheFile> ){
print $_;
}
- if(
- $action eq "snapshot"
- ||
- $action eq "blob_plain"
- ){
+ if( isBinaryAction($action) ){
binmode STDOUT, ':raw';
print <cacheFileBin>;
binmode STDOUT, ':utf8'; # as set at the beginning of gitweb.cgi
--
1.7.2.3
^ permalink raw reply related
* [PATCH 11/18] gitweb: add isDumbClient() check
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
Basic check for the claimed Agent string, if it matches a known
blacklist (wget and curl currently) don't display the 'Generating...'
page.
Jakub has mentioned a couple of other possible ways to handle
this, so if a better way comes along this should be used as a
wrapper to any better way we can find to deal with this.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/lib/cache.pl | 30 ++++++++++++++++++++++++++++++
1 files changed, 30 insertions(+), 0 deletions(-)
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index d55b572..5182a94 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -116,6 +116,34 @@ sub isFeedAction {
return 0; # False
}
+# There have been a number of requests that things like "dumb" clients, I.E. wget
+# lynx, links, etc (things that just download, but don't parse the html) actually
+# work without getting the wonkiness that is the "Generating..." page.
+#
+# There's only one good way to deal with this, and that's to read the browser User
+# Agent string and do matching based on that. This has a whole slew of error cases
+# and mess, but there's no other way to determine if the "Generating..." page
+# will break things.
+#
+# This assumes the client is not dumb, thus the default behavior is to return
+# "false" (0) (and eventually the "Generating..." page). If it is a dumb client
+# return "true" (1)
+sub isDumbClient {
+ my($user_agent) = $ENV{'HTTP_USER_AGENT'};
+
+ if(
+ # wget case
+ $user_agent =~ /^Wget/i
+ ||
+ # curl should be excluded I think, probably better safe than sorry
+ $user_agent =~ /^curl/i
+ ){
+ return 1; # True
+ }
+
+ return 0;
+}
+
sub cacheUpdate {
my ($action,$areForked) = @_;
my $lockingStatus;
@@ -237,6 +265,8 @@ sub cacheWaitForUpdate {
if(
isFeedAction($action)
||
+ isDumbClient()
+ ||
! $cacheGenStatus
){
do {
--
1.7.2.3
^ permalink raw reply related
* [PATCH 17/18] gitweb: Prepare for cached error pages & better error page handling
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
To quote myself from an e-mail of mine:
I've got a hammer, it clearly solves all problems!
This is the prepatory work to set up a mechanism inside the
caching engine to cache the error pages instead of throwing
them straight out to the client.
This adds two functions:
die_error_cache() - this gets back called from die_error() so
that the error message generated can be cached.
cacheDisplayErr() - this is a simplified version of cacheDisplay()
that does an initial check, if the error page exists - display it
and exit. If not, return.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/lib/cache.pl | 52 +++++++++++++++++++++++++++++++++++++++++++++++++++
1 files changed, 52 insertions(+), 0 deletions(-)
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index a8c902d..6cb82c8 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -302,6 +302,36 @@ sub cacheUpdate {
}
}
+sub die_error_cache {
+ my ($output) = @_;
+
+ open(my $cacheFileErr, '>:utf8', "$fullhashpath.err");
+ my $lockStatus = flock($cacheFileErr,LOCK_EX|LOCK_NB);
+
+ if (! $lockStatus ){
+ if ( $areForked ){
+ exit(0);
+ }else{
+ return;
+ }
+ }
+
+ # Actually dump the output to the proper file handler
+ local $/ = undef;
+ $|++;
+ print $cacheFileErr "$output";
+ $|--;
+
+ flock($cacheFileErr,LOCK_UN);
+ close($cacheFileErr);
+
+ if ( $areForked ){
+ exit(0);
+ }else{
+ return;
+ }
+}
+
sub cacheWaitForUpdate {
my ($action) = @_;
@@ -380,6 +410,28 @@ EOF
return;
}
+sub cacheDisplayErr {
+
+ return if ( ! -e "$fullhashpath.err" );
+
+ open($cacheFileErr, '<:utf8', "$fullhashpath.err");
+ $lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
+
+ if (! $lockStatus ){
+ show_warning(
+ "<p>".
+ "<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
+ "This is about as screwed up as it gets folks - see your systems administrator for more help with this.".
+ "<p>"
+ );
+ }
+
+ while( <$cacheFileErr> ){
+ print $_;
+ }
+ exit(0);
+}
+
sub cacheDisplay {
local $/ = undef;
$|++;
--
1.7.2.3
^ permalink raw reply related
* [PATCH 16/18] gitweb: When changing output (STDOUT) change STDERR as well
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
This sets up a trap for STDERR as well as STDOUT. This should
prevent any transient error messages from git itself percolating
up to gitweb and outputting errant information before the HTTP
header has been sent.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/gitweb.perl | 22 +++++++++++++++++++++-
gitweb/lib/cache.pl | 22 ----------------------
2 files changed, 21 insertions(+), 23 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 7f8292e..d39982a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1214,6 +1214,10 @@ sub evaluate_argv {
sub change_output {
our $output;
+ #
+ # STDOUT
+ #
+
# Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such
open(STDOUT_REAL,">&STDOUT") or die "Unable to capture STDOUT $!\n";
print STDOUT_REAL "";
@@ -1223,12 +1227,28 @@ sub change_output {
# Trap STDOUT to the $output variable, which is what I was using in the original
# patch anyway.
- open(STDOUT,">", \$output) || die "Unable to open STDOUT: $!"; #open STDOUT handle to use $var
+ open(STDOUT,">", \$output) || die "Unable to open STDOUT: $!"; #open STDOUT handle to use $output
+
+ #
+ # STDERR
+ #
+
+ # Trap the 'proper' STDOUT to STDOUT_REAL for things like error messages and such
+ open(STDERR_REAL,">&STDERR") or die "Unable to capture STDERR $!\n";
+ print STDERR_REAL "";
+
+ # Close STDOUT, so that it isn't being used anymore.
+ close STDERR;
+
+ # Trap STDOUT to the $output variable, which is what I was using in the original
+ # patch anyway.
+ open(STDERR,">", \$output_err) || die "Unable to open STDERR: $!"; #open STDERR handle to use $output_err
}
sub reset_output {
# This basically takes STDOUT_REAL and puts it back as STDOUT
open(STDOUT,">&STDOUT_REAL");
+ open(STDERR,">&STDERR_REAL");
}
sub run {
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index 28e4240..a8c902d 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -380,28 +380,6 @@ EOF
return;
}
-sub cacheDisplayErr {
-
- return if ( ! -e "$fullhashpath.err" );
-
- open($cacheFileErr, '<:utf8', "$fullhashpath.err");
- $lockStatus = flock($cacheFileErr,LOCK_SH|LOCK_NB);
-
- if (! $lockStatus ){
- show_warning(
- "<p>".
- "<strong>*** Warning ***:</strong> Locking error when trying to lock error cache page, file $fullhashpath.err<br/>/\n".
- "This is about as screwed up as it gets folks - see your systems administrator for more help with this.".
- "<p>"
- );
- }
-
- while( <$cacheFileErr> ){
- print $_;
- }
- exit(0);
-}
-
sub cacheDisplay {
local $/ = undef;
$|++;
--
1.7.2.3
^ permalink raw reply related
* [PATCH 18/18] gitweb: Add better error handling for gitweb caching
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
This basically finishes the plumbing for caching the error pages
as the are generated.
If an error is hit, create a <hash>.err file with the error. This
will interrupt all currently waiting processes and they will display
the error, without any additional refreshing.
On a new request a generation will be attempted, should it succed the
<hash.err> file is removed (if it exists).
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/gitweb.perl | 8 ++++++++
gitweb/lib/cache.pl | 14 ++++++++++++++
2 files changed, 22 insertions(+), 0 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index d39982a..5a9660a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -41,6 +41,7 @@ our $version = "++GIT_VERSION++";
# Output buffer variable
our $output = "";
+our $output_err = "";
our ($my_url, $my_uri, $base_url, $path_info, $home_link);
sub evaluate_uri {
@@ -303,6 +304,9 @@ our $fullhashpath = *STDOUT;
our $fullhashbinpath = *STDOUT;
our $fullhashbinpathfinal = *STDOUT;
+our $cacheErrorCache = 0; # false
+our $cacheErrorCount = 0;
+
our $full_url;
our $urlhash;
@@ -3786,6 +3790,7 @@ sub die_error {
# Reset the output so that we are actually going to STDOUT as opposed
# to buffering the output.
reset_output() if ($cache_enable && ! $cacheErrorCache);
+ $cacheErrorCount++ if( $cacheErrorCache );
git_header_html($http_responses{$status}, undef, %opts);
print <<EOF;
@@ -3801,6 +3806,9 @@ EOF
print "</div>\n";
git_footer_html();
+
+ die_error_cache($output) if ( $cacheErrorCache );
+
goto DONE_GITWEB
unless ($opts{'-error_handler'});
}
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index 6cb82c8..2e7ca69 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -240,8 +240,14 @@ sub cacheUpdate {
# Trap all output from the action
change_output();
+ # Set the error handler so we cache
+ $cacheErrorCache = 1; # true
+
$actions{$action}->();
+ # Reset Error Handler to not cache
+ $cacheErrorCache = 0; # false
+
# Reset the outputs as we should be fine now
reset_output();
@@ -295,6 +301,8 @@ sub cacheUpdate {
close($cacheFileBG);
}
+ unlink("$fullhashpath.err") if (-e "$fullhashpath.err");
+
if ( $areForked ){
exit(0);
} else {
@@ -339,6 +347,9 @@ sub cacheWaitForUpdate {
my $max = 10;
my $lockStat = 0;
+ # Call cacheDisplayErr - if an error exists it will display and die. If not it will just return
+ cacheDisplayErr($action);
+
if( $backgroundCache ){
if( -e "$fullhashpath" ){
open($cacheFile, '<:utf8', "$fullhashpath");
@@ -402,6 +413,9 @@ EOF
close($cacheFile);
$x++;
$combinedLockStat = $lockStat;
+
+ # Call cacheDisplayErr - if an error exists it will display and die. If not it will just return
+ cacheDisplayErr($action);
} while ((! $combinedLockStat) && ($x < $max));
print <<EOF;
</body>
--
1.7.2.3
^ permalink raw reply related
* [PATCH 14/18] gitweb: add print_transient_header() function for central header printing
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
There are a few things I would like to reuse the transient header
information I'm using, currently this is only the 'Generating...'
page, but there is at least one additional warning page I would
like to use this on.
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/lib/cache.pl | 47 ++++++++++++++++++++++++++---------------------
1 files changed, 26 insertions(+), 21 deletions(-)
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index 63dbe9e..723ae9b 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -94,6 +94,31 @@ sub cache_fetch {
#$actions{$action}->();
}
+sub print_transient_header {
+ print $::cgi->header(
+ -type=>'text/html',
+ -charset => 'utf-8',
+ -status=> 200,
+ -expires => 'now',
+ # HTTP/1.0
+ -Pragma => 'no-cache',
+ # HTTP/1.1
+ -Cache_Control => join(
+ ', ',
+ qw(
+ private
+ no-cache
+ no-store
+ must-revalidate
+ max-age=0
+ pre-check=0
+ post-check=0
+ )
+ )
+ );
+ return;
+}
+
sub isBinaryAction {
my ($action) = @_;
@@ -292,27 +317,7 @@ sub cacheWaitForUpdate {
$| = 1;
- print $::cgi->header(
- -type=>'text/html',
- -charset => 'utf-8',
- -status=> 200,
- -expires => 'now',
- # HTTP/1.0
- -Pragma => 'no-cache',
- # HTTP/1.1
- -Cache_Control => join(
- ', ',
- qw(
- private
- no-cache
- no-store
- must-revalidate
- max-age=0
- pre-check=0
- post-check=0
- )
- )
- );
+ print_transient_header();
print <<EOF;
<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www/w3.porg/TR/html4/strict.dtd">
--
1.7.2.3
^ permalink raw reply related
* [PATCH 15/18] gitweb: Add show_warning() to display an immediate warning, with refresh
From: John 'Warthog9' Hawley @ 2010-12-09 21:57 UTC (permalink / raw)
To: git
In-Reply-To: <1291931844-28454-1-git-send-email-warthog9@eaglescrag.net>
die_error() is an immediate and abrupt action. show_warning() more or less
functions identically, except that the page generated doesn't use the
gitweb header or footer (in case they are broken) and has an auto-refresh
(10 seconds) built into it.
This makes use of print_transient_header() which is also used in the
'Generating...' page. Currently the only warning it throws is about
the cache needing to be created. If that fails it's a fatal error
and we call die_error()
Signed-off-by: John 'Warthog9' Hawley <warthog9@eaglescrag.net>
---
gitweb/lib/cache.pl | 36 +++++++++++++++++++++++++++++++++---
1 files changed, 33 insertions(+), 3 deletions(-)
diff --git a/gitweb/lib/cache.pl b/gitweb/lib/cache.pl
index 723ae9b..28e4240 100644
--- a/gitweb/lib/cache.pl
+++ b/gitweb/lib/cache.pl
@@ -25,9 +25,13 @@ sub cache_fetch {
my $cacheTime = 0;
if(! -d $cachedir){
- print "*** Warning ***: Caching enabled but cache directory does not exsist. ($cachedir)\n";
- mkdir ("cache", 0755) || die "Cannot create cache dir - you will need to manually create";
- print "Cache directory created successfully\n";
+ mkdir ("cache", 0755) || die_error(500, "Internal Server Error", "Cannot create cache dir () - you will need to manually create");
+ show_warning(
+ "<p>".
+ "<strong>*** Warning ***:</strong> Caching enabled but cache directory did not exsist. ($cachedir)<br/>/\n".
+ "Cache directory created successfully\n".
+ "<p>"
+ );
}
$full_url = "$my_url?". $ENV{'QUERY_STRING'};
@@ -119,6 +123,32 @@ sub print_transient_header {
return;
}
+sub show_warning {
+ $| = 1;
+
+ my $warning = esc_html(shift) || "Unknown Warning";
+
+ print_transient_header();
+
+ print <<EOF;
+<!DOCTYPE html PUBLIC "-//W3C//DTD HTML 4.01//EN" "http://www/w3.porg/TR/html4/strict.dtd">
+<!-- git web w/caching interface version $version, (C) 2006-2010, John 'Warthog9' Hawley <warthog9\@kernel.org> -->
+<!-- git core binaries version $git_version -->
+<head>
+<meta http-equiv="content-type" content="$content_type; charset=utf-8"/>
+<meta name="generator" content="gitweb/$version git/$git_version"/>
+<meta name="robots" content="index, nofollow"/>
+<meta http-equiv="refresh" content="10"/>
+<title>$title</title>
+</head>
+<body>
+$warning
+</body>
+</html>
+EOF
+ exit(0);
+}
+
sub isBinaryAction {
my ($action) = @_;
--
1.7.2.3
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox