* Re: cannot fetch arm git tree
From: Johannes Sixt @ 2011-01-24 7:21 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: Detlef Vollmann, Uwe Kleine-König, Jello huang, git,
linux-arm-kernel
In-Reply-To: <20110121145025.GS13235@n2100.arm.linux.org.uk>
Am 1/21/2011 15:50, schrieb Russell King - ARM Linux:
> SetEnv GIT_PROJECT_ROOT /var/www/git
> SetEnv GIT_HTTP_EXPORT_ALL
> ScriptAlias /git/ /usr/libexec/git-core/git-http-backend/
>
> Great. Deciding that it will be http://servername.example.com/git/ is
> really damned annoying as that's traditionally where gitweb lives,
> which requires a different script alias.
> ...
> I'm really not interested in working out how to bodge this into working
> along side the existing gitweb setup by adding lots of rewrite rules,...
It has been worked out for you already. It's just a single rule (although
a bit longish). Look for 'ScriptAliasMatch' in
http://www.kernel.org/pub/software/scm/git/docs/git-http-backend.html
-- Hannes
^ permalink raw reply
* Re: cannot fetch arm git tree
From: Miles Bader @ 2011-01-24 5:01 UTC (permalink / raw)
To: Russell King - ARM Linux
Cc: git, linux-arm-kernel, Jello huang, Detlef Vollmann,
Uwe Kleine-König
In-Reply-To: <20110121145025.GS13235@n2100.arm.linux.org.uk>
Russell King - ARM Linux <linux@arm.linux.org.uk> writes:
> I'm really not interested in working out how to bodge this into working
> along side the existing gitweb setup by adding lots of rewrite rules, so
> as gitweb got there first I think it has priority, that's what we have
> and we'll have to live without the smart http extensions.
...
> It's really not that big a deal if you follow the advice I've given.
Smart http is actually a very big deal -- the old git http protocol is
almost unusable in practice with big repos, at least over somewhat
latency-limited network connections.
If you don't intend to support people pulling over http, then maybe you
don't care. But if you do care, it's very much worth a second look.
[My personal reason for caring is that I'm behind a corporate firewall
that's latency limited, although it seems to have pretty good bandwidth.
With some public repos, pulling via the old http protocol was a
multi-hour operation; the new http protocol is typically multiple orders
of magnitude faster in these cases.]
-Miles
--
Omochiroi!
^ permalink raw reply
* [PATCH] add config option core.bisectbadbranchfirst
From: Shuang He @ 2011-01-24 2:05 UTC (permalink / raw)
To: git; +Cc: Shuang He
In-Reply-To: <4D3CDDF9.6080405@intel.com>
which enable recursive bad-branch-first algorithm for git-bisect.
With this algorithm, git-bisect will always try to select commits
that on the same branch current bad commit sits. And will fall back
to default git-bisect algorithm when bad-branch-first algorithm does
not apply
Signed-off-by: Shuang He <shuang.he@intel.com>
---
Documentation/config.txt | 8 ++
bisect.c | 244 +++++++++++++++++++++++++++++++++++++++++-----
bisect.h | 2 +-
builtin/rev-list.c | 2 +-
cache.h | 1 +
config.c | 6 +
environment.c | 1 +
7 files changed, 238 insertions(+), 26 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ff7c225..8502859 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -558,6 +558,14 @@ core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
linkgit:git-read-tree[1] for more information.
+core.bisectbadbranchfirst::
+ With this algorithm, git-bisect will always try to select commits
+ that on the same branch current bad commit sits. And will fall back
+ to default git-bisect algorithm when bad-branch-first algorithm does
+ not apply
++
+This setting defaults to "false".
+
add.ignore-errors::
add.ignoreErrors::
Tells 'git add' to continue adding files when some files cannot be
diff --git a/bisect.c b/bisect.c
index 060c042..57c410d 100644
--- a/bisect.c
+++ b/bisect.c
@@ -189,6 +189,46 @@ static struct commit_list *best_bisection(struct commit_list *list, int nr)
return best;
}
+static struct commit_list *best_bisection_first_parent(struct commit_list *list, struct commit_list *list2, int nr)
+{
+ struct commit_list *p, *best;
+ struct commit_list *p2;
+ int best_distance = -1;
+
+ best = NULL;
+ for (p2 = list2; p2; p2 = p2->next) {
+ int distance;
+ int on_branch;
+
+ on_branch = 0;
+ for (p = list; p; p = p->next) {
+ unsigned flags = p->item->object.flags;
+ if (flags & TREESAME)
+ continue;
+ if (!hashcmp(p->item->object.sha1, p2->item->object.sha1)) {
+ on_branch = 1;
+ break;
+ }
+ }
+
+ if (!on_branch)
+ continue;
+
+ weight_set(p2, weight(p));
+ distance = weight(p);
+ if (nr - distance < distance)
+ distance = nr - distance;
+ if (distance > best_distance) {
+ best = p;
+ best_distance = distance;
+ }
+
+ }
+
+ return list2;
+}
+
+
struct commit_dist {
struct commit *commit;
int distance;
@@ -253,7 +293,7 @@ static struct commit_list *best_bisection_sorted(struct commit_list *list, int n
* unknown. After running count_distance() first, they will get zero
* or positive distance.
*/
-static struct commit_list *do_find_bisection(struct commit_list *list,
+static struct commit_list *do_find_bisection(struct commit_list *list, struct commit_list *list2,
int nr, int *weights,
int find_all)
{
@@ -314,7 +354,7 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
clear_distance(list);
/* Does it happen to be at exactly half-way? */
- if (!find_all && halfway(p, nr))
+ if (!list2 && !find_all && halfway(p, nr))
return p;
counted++;
}
@@ -352,20 +392,27 @@ static struct commit_list *do_find_bisection(struct commit_list *list,
weight_set(p, weight(q));
/* Does it happen to be at exactly half-way? */
- if (!find_all && halfway(p, nr))
+ if (!list2 && !find_all && halfway(p, nr))
return p;
}
}
show_list("bisection 2 counted all", counted, nr, list);
+ if (list2) {
+ struct commit_list *t;
+ t = best_bisection_first_parent(list, list2, nr);
+ t = best_bisection_sorted(t, nr);
+ return t;
+ }
+
if (!find_all)
return best_bisection(list, nr);
else
return best_bisection_sorted(list, nr);
}
-struct commit_list *find_bisection(struct commit_list *list,
+struct commit_list *find_bisection(struct commit_list *list, struct commit_list *list2,
int *reaches, int *all,
int find_all)
{
@@ -400,7 +447,7 @@ struct commit_list *find_bisection(struct commit_list *list,
weights = xcalloc(on_list, sizeof(*weights));
/* Do the real work of finding bisection commit. */
- best = do_find_bisection(list, nr, weights, find_all);
+ best = do_find_bisection(list, list2, nr, weights, find_all);
if (best) {
if (!find_all)
best->next = NULL;
@@ -683,6 +730,33 @@ static void bisect_rev_setup(struct rev_info *revs, const char *prefix,
setup_revisions(rev_argv.argv_nr, rev_argv.argv, revs, NULL);
}
+static void bisect_rev_setup_first_parent(struct rev_info *revs, const char *prefix,
+ const char *bad_format, const char *good_format,
+ int read_paths)
+{
+ struct argv_array rev_argv = { NULL, 0, 0 };
+ int i;
+
+ init_revisions(revs, prefix);
+ revs->abbrev = 0;
+ revs->commit_format = CMIT_FMT_UNSPECIFIED;
+
+ /* rev_argv.argv[0] will be ignored by setup_revisions */
+ argv_array_push(&rev_argv, xstrdup("bisect_rev_setup_first_parent"));
+ argv_array_push_sha1(&rev_argv, current_bad_sha1, bad_format);
+ for (i = 0; i < good_revs.sha1_nr; i++)
+ argv_array_push_sha1(&rev_argv, good_revs.sha1[i],
+ good_format);
+ argv_array_push(&rev_argv, xstrdup("--first-parent"));
+ argv_array_push(&rev_argv, xstrdup("--"));
+ if (read_paths)
+ read_bisect_paths(&rev_argv);
+ argv_array_push(&rev_argv, NULL);
+
+ setup_revisions(rev_argv.argv_nr, rev_argv.argv, revs, NULL);
+}
+
+
static void bisect_common(struct rev_info *revs)
{
if (prepare_revision_walk(revs))
@@ -944,6 +1018,24 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
log_tree_commit(&opt, commit);
}
+int is_merge_commit(struct commit_list *entry) {
+ int nr = 0;
+ struct commit *commit;
+ struct commit_list *p;
+
+ if (entry) {
+ p = entry->item->parents;
+
+ while (p) {
+ nr += 1;
+ p = p->next;
+ }
+ }
+
+ return nr - 1;
+}
+
+
/*
* We use the convention that exiting with an exit code 10 means that
* the bisection process finished successfully.
@@ -952,41 +1044,145 @@ static void show_diff_tree(const char *prefix, struct commit *commit)
int bisect_next_all(const char *prefix)
{
struct rev_info revs;
+ struct rev_info first_parent_revs;
struct commit_list *tried;
int reaches = 0, all = 0, nr, steps;
+ struct object_array pending_copy;
+ struct object_array pending_copy2;
const unsigned char *bisect_rev;
+ int i;
char bisect_rev_hex[41];
+ int bad_branch_first_working = 1;
if (read_bisect_refs())
die("reading bisect refs failed");
check_good_are_ancestors_of_bad(prefix);
+ struct argv_array rev_argv = { NULL, 0, 0 };
+
+ read_bisect_paths(&rev_argv);
+ if (rev_argv.argv_nr > 0)
+ core_bisect_bad_branch_first = 0;
+
+ if (core_bisect_bad_branch_first) {
+ bisect_rev_setup_first_parent(&first_parent_revs, prefix, "%s", "^%s", 1);
+ memset(&pending_copy, 0, sizeof(pending_copy));
+ for (i = 0; i < first_parent_revs.pending.nr; i++)
+ add_object_array(first_parent_revs.pending.objects[i].item,
+ first_parent_revs.pending.objects[i].name,
+ &pending_copy);
+
+ bisect_common(&first_parent_revs);
+
+ /* Clean up objects used, as they will be reused. */
+ for (i = 0; i < pending_copy.nr; i++) {
+ struct object *o = pending_copy.objects[i].item;
+ clear_commit_marks((struct commit *)o, ALL_REV_FLAGS);
+ }
+ }
- bisect_rev_setup(&revs, prefix, "%s", "^%s", 1);
- revs.limited = 1;
- bisect_common(&revs);
- revs.commits = find_bisection(revs.commits, &reaches, &all,
- !!skipped_revs.sha1_nr);
- revs.commits = managed_skipped(revs.commits, &tried);
+ if (core_bisect_bad_branch_first) {
+ bisect_rev_setup(&revs, prefix, "%s", "^%s", 1);
+ memset(&pending_copy2, 0, sizeof(pending_copy2));
+ for (i = 0; i < revs.pending.nr; i++)
+ add_object_array(revs.pending.objects[i].item,
+ revs.pending.objects[i].name,
+ &pending_copy2);
+
+ revs.limited = 1;
+
+ bisect_common(&revs);
+
+ revs.commits = find_bisection(revs.commits, first_parent_revs.commits, &reaches, &all,
+ !!skipped_revs.sha1_nr);
+ /* TODO: Check if this is a first bad commit on bad branch, and if it's a merge commit */
+ /* If this is a merge commit, we need to try all of its parents, until we found the merged bad branch */
+ /* If all parents are good, we just announce it as first bad commit */
+ if (!hashcmp(revs.commits->item->object.sha1, current_bad_sha1)) {
+ printf("%s is the first bad commit\n", sha1_to_hex(current_bad_sha1));
+
+ if (is_merge_commit(revs.commits)) {
+ int all_parents_good = 1;
+ struct commit_list *p;
+
+ printf("%s is a merge commit\n", sha1_to_hex(current_bad_sha1));
+ printf("need to try each merged branch\n", sha1_to_hex(current_bad_sha1));
+ p = revs.commits->item->parents;
+
+ while (p) {
+ if (!hashcmp(p->item->object.sha1, current_bad_sha1)) {
+ fprintf(stderr, "should not get here\n");
+ exit(1);
+ } else if (0 <= lookup_sha1_array(&good_revs, p->item->object.sha1)) {
+ fprintf(stderr, "good parent %s\n", sha1_to_hex(p->item->object.sha1));
+ } else if (0 <= lookup_sha1_array(&skipped_revs, p->item->object.sha1)) {
+ fprintf(stderr, "skipped parent %s\n", sha1_to_hex(p->item->object.sha1));
+ all_parents_good = 0;
+ } else {
+ printf("try merged branch %s\n", sha1_to_hex(p->item->object.sha1));
+ exit(bisect_checkout(sha1_to_hex(p->item->object.sha1)));
+ }
+ p = p->next;
+ }
+
+ if (all_parents_good) {
+ show_diff_tree(prefix, revs.commits->item);
+ exit(10);
+ }
+ }
+ else {
+ show_diff_tree(prefix, revs.commits->item);
+ exit(10);
+ }
+ }
+
+ revs.commits = managed_skipped(revs.commits, &tried);
- if (!revs.commits) {
- /*
- * We should exit here only if the "bad"
- * commit is also a "skip" commit.
- */
- exit_if_skipped_commits(tried, NULL);
+ if (!revs.commits || !all) {
+ bad_branch_first_working = 0;
+ fprintf(stderr, "fall back to default git-bisect algorithm\n");
+ }
+ else
+ fprintf(stderr, "proceed with core_bisect_bad_branch_first algorithm\n");
- printf("%s was both good and bad\n",
- sha1_to_hex(current_bad_sha1));
- exit(1);
+ /* Clean up objects used, as they will be reused. */
+ for (i = 0; i < pending_copy2.nr; i++) {
+ struct object *o = pending_copy2.objects[i].item;
+ clear_commit_marks((struct commit *)o, ALL_REV_FLAGS);
+ }
}
- if (!all) {
- fprintf(stderr, "No testable commit found.\n"
- "Maybe you started with bad path parameters?\n");
- exit(4);
+
+ if (!core_bisect_bad_branch_first || !bad_branch_first_working) {
+ bisect_rev_setup(&revs, prefix, "%s", "^%s", 1);
+ revs.limited = 1;
+
+ bisect_common(&revs);
+ show_list("fall back", 0, 0, revs.commits);
+
+ revs.commits = find_bisection(revs.commits, NULL, &reaches, &all,
+ !!skipped_revs.sha1_nr);
+ revs.commits = managed_skipped(revs.commits, &tried);
+
+ if (!revs.commits) {
+ /*
+ * We should exit here only if the "bad"
+ * commit is also a "skip" commit.
+ */
+ exit_if_skipped_commits(tried, NULL);
+
+ printf("%s was both good and bad\n",
+ sha1_to_hex(current_bad_sha1));
+ exit(1);
+ }
+
+ if (!all) {
+ fprintf(stderr, "No testable commit found.\n"
+ "Maybe you started with bad path parameters?\n");
+ exit(4);
+ }
}
bisect_rev = revs.commits->item->object.sha1;
diff --git a/bisect.h b/bisect.h
index 0862ce5..fad4fbe 100644
--- a/bisect.h
+++ b/bisect.h
@@ -1,7 +1,7 @@
#ifndef BISECT_H
#define BISECT_H
-extern struct commit_list *find_bisection(struct commit_list *list,
+extern struct commit_list *find_bisection(struct commit_list *list, struct commit_list *list2,
int *reaches, int *all,
int find_all);
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index ba27d39..ab56f6b 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -399,7 +399,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if (bisect_list) {
int reaches = reaches, all = all;
- revs.commits = find_bisection(revs.commits, &reaches, &all,
+ revs.commits = find_bisection(revs.commits, NULL, &reaches, &all,
bisect_find_all);
if (bisect_show_vars)
diff --git a/cache.h b/cache.h
index d83d68c..bfd4448 100644
--- a/cache.h
+++ b/cache.h
@@ -559,6 +559,7 @@ extern int read_replace_refs;
extern int fsync_object_files;
extern int core_preload_index;
extern int core_apply_sparse_checkout;
+extern int core_bisect_bad_branch_first;
enum safe_crlf {
SAFE_CRLF_FALSE = 0,
diff --git a/config.c b/config.c
index 625e051..b37a70c 100644
--- a/config.c
+++ b/config.c
@@ -660,6 +660,12 @@ static int git_default_core_config(const char *var, const char *value)
return 0;
}
+ if (!strcmp(var, "core.bisectbadbranchfirst")) {
+ core_bisect_bad_branch_first = git_config_bool(var, value);
+ return 0;
+ }
+
+
/* Add other config variables here and to Documentation/config.txt. */
return 0;
}
diff --git a/environment.c b/environment.c
index 9564475..cf813af 100644
--- a/environment.c
+++ b/environment.c
@@ -55,6 +55,7 @@ enum object_creation_mode object_creation_mode = OBJECT_CREATION_MODE;
char *notes_ref_name;
int grafts_replace_parents = 1;
int core_apply_sparse_checkout;
+int core_bisect_bad_branch_first = 0;
struct startup_info *startup_info;
/* Parallel index stat data preload? */
--
1.7.4.rc2.21.g7f7a8
^ permalink raw reply related
* [RFC] Add bad-branch-first option for git-bisect
From: Shuang He @ 2011-01-24 2:03 UTC (permalink / raw)
To: git; +Cc: shuang.he
Hi
The default git-bisect algorithm will jump around the commit tree,
on the purpose of taking least steps to find the first culprit commit.
We may find it sometime would locate a old culprit commit that we're not
concerned about anymore.
In most software development, there's one or two main branch which
is maintained for release, and a bunch of feature branches are created
for new feature development or bug fix. For the reason that sometime
git-bisect will locate a old culprit commit would be:
1. Quality of those branches may not match the main branch,
some functionality are broken at first and fixed later on the feature
branch. If git-bisect jump to there by chance, git-bisect will only find that old
culprit commit which only exists on that feature branch
2. Some of those branches may not synchronized with main
branch in time. Say feature1 is broken when feature2 branch is created, and
feature1 is fixed just a moment later after feature2 branch is created,
and when feature2's development is done, and developer want to merge
feature2 branch back to master branch, feature2 will be firstly
synchronized to master branch tip, then merge into master. For the same
reason addressed in issue 1, this will also lead git-bisect into wrong
direction.
In all, we think we do not care about branches that we're not
currently working, unless we're sure the regression is caused by that
branch.
To address those issue, we propose to add a new config option:
core.bisectbadbranchfirst::
With this algorithm, git-bisect will always try to select
commits
that on the same branch current bad commit sits. And will
fall back
to default git-bisect algorithm when bad-branch-first
algorithm does
not apply
+
This setting defaults to "false".
The draft patch will be sent out in a later email, so it could be
reviewed inline.
Any question or suggestion is welcome :-)
Thanks
--Shuang
^ permalink raw reply
* Re: [PATCH 3/3] setup: always honor GIT_WORK_TREE and core.worktree
From: Junio C Hamano @ 2011-01-23 23:49 UTC (permalink / raw)
To: Nguyen Thai Ngoc Duy; +Cc: Jonathan Nieder, git, Maaartin
In-Reply-To: <AANLkTim_o9GGWbDFkeGb-va+4dP+StQE6GJyLpSMmV1H@mail.gmail.com>
Nguyen Thai Ngoc Duy <pclouds@gmail.com> writes:
> On Sat, Jan 22, 2011 at 3:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I was re-reading this thread, and changed my mind; I think we should have
>> this series to avoid unnecessary regression, with or without clarifying
>> (5), before 1.7.4 final.
>
> Sorry for late response. If we no longer consider this work-around,
> perhaps git.txt and config.txt should be updated to reflect it?
I am Ok with us considering this as a "work-around"; as long as we keep it
alive, the label does not matter much.
And the necessity of documentation updates you raised is really a good
point. Something like this (on top of jn/setup-fixes branch queued in
next)?
Documentation/config.txt | 23 ++++++++++++++++-------
Documentation/git.txt | 13 ++++---------
2 files changed, 20 insertions(+), 16 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index ff7c225..72b74c4 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -317,17 +317,26 @@ false), while all other repositories are assumed to be bare (bare
= true).
core.worktree::
- Set the path to the working tree. The value will not be
- used in combination with repositories found automatically in
- a .git directory (i.e. $GIT_DIR is not set).
+ Set the path to the root of the work tree.
This can be overridden by the GIT_WORK_TREE environment
variable and the '--work-tree' command line option. It can be
- an absolute path or relative path to the directory specified by
- --git-dir or GIT_DIR.
- Note: If --git-dir or GIT_DIR are specified but none of
+ an absolute path or a relative path to the .git directory,
+ either specified by --git-dir or GIT_DIR, or automatically
+ discovered.
+ If --git-dir or GIT_DIR are specified but none of
--work-tree, GIT_WORK_TREE and core.worktree is specified,
- the current working directory is regarded as the top directory
+ the current working directory is regarded as the top level
of your working tree.
++
+Note that this variable is honored even when set in a configuration
+file in a ".git" subdirectory of a directory, and its value differs
+from the latter directory (e.g. "/path/to/.git/config" has
+core.worktree set to "/different/path"), which is most likely a
+misconfiguration. Running git commands in "/path/to" directory will
+still use "/different/path" as the root of the work tree and can cause
+confusion, unless you know what you are doing (e.g. you are creating a
+read-only snapshot of the same index to a location different from the
+repository's usual working tree).
core.logAllRefUpdates::
Enable the reflog. Updates to a ref <ref> is logged to the file
diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4e5fe4d..245d84f 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -291,17 +291,12 @@ help ...`.
path or relative path to current working directory.
--work-tree=<path>::
- Set the path to the working tree. The value will not be
- used in combination with repositories found automatically in
- a .git directory (i.e. $GIT_DIR is not set).
+ Set the path to the working tree. It can be an absolute path
+ or relative path to the current working directory.
This can also be controlled by setting the GIT_WORK_TREE
environment variable and the core.worktree configuration
- variable. It can be an absolute path or relative path to
- current working directory.
- Note: If --git-dir or GIT_DIR are specified but none of
- --work-tree, GIT_WORK_TREE and core.worktree is specified,
- the current working directory is regarded as the top directory
- of your working tree.
+ variable (see core.worktree in linkgit:git-config[1] for a
+ more detailed discussion).
--bare::
Treat the repository as a bare repository. If GIT_DIR
^ permalink raw reply related
* Re: [PATCH] No color diff when redirecting to file
From: Junio C Hamano @ 2011-01-23 23:32 UTC (permalink / raw)
To: Matthieu Moy; +Cc: Johannes Sixt, Sascha Peilicke, git
In-Reply-To: <vpq39oj4jfz.fsf@bauges.imag.fr>
Matthieu Moy <Matthieu.Moy@grenoble-inp.fr> writes:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> That said, I cannot reproduce. Perhaps your configuration says
>> color.ui=always? If so, then this is expected behavior and not a bug, IMO.
>
> I think you're right.
>
> I tested this (without the patch):
>
> git diff
> => I get color, because I have color.ui = auto
>
> git diff | cat
> => I don't get color
>
> git diff --color=auto | cat
> => no color
>
> git diff --color=always | cat
> => colors, because I've asked
>
> this seems to be just the right behavior.
Overall I agree with the above observation, but the original poster says
"redirecting into a _file_".
Another possibility is that the mysterious platform the original poster
did not mention has a broken istty() that mistakes a file descriptor going
to a file (instead of a pipe to another process) as connected to a tty.
^ permalink raw reply
* Re: [PATCH 2/2] Re: rebase -i: explain how to discard all commits
From: Johannes Schindelin @ 2011-01-23 20:10 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Nicolas Sebrecht, Jonathan Nieder, Matthieu Moy, git,
Kevin Ballard, Yann Dirson, Eric Raible
In-Reply-To: <7vsjwmp5cs.fsf@alter.siamese.dyndns.org>
Hi,
On Fri, 21 Jan 2011, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> >> Wouldn't that suggest us that if we were to do anything to this
> >> message it would be a good idea to teach the user to "reset --hard"
> >> the branch if no commits truly needs to be replayed on top of the
> >> onto-commit?
> >
> > The important difference between rebase -i && noop on the one, and
> > reset --hard on the other hand is that the latter is completely
> > unsafe. I mean, utterly completely super-unsafe. And I say that
> > because _this here developer_ who is not exactly a Git noob lost stuff
> > that way.
>
> I think "rebase" already checks that the index and the working tree is
> clean before starting, so referring to "reset --hard" when "rebase -i"
> notices there is absolutely nothing to do is _not_ unsafe, no?
Oh, so you want to suggest using "reset --hard" but warn at the same time
that this command on its own is dangerous unless you run rebase first? :-)
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v2] Documentation fixes in git-config
From: Libor Pechacek @ 2011-01-23 19:46 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20110121162537.GD21840@sigill.intra.peff.net>
On Fri 21-01-11 11:25:37, Jeff King wrote:
> I am half-tempted to mark the lowercasing of the regex as deprecated (or
> at least discouraged).
That's actually side effect of
http://git.kernel.org/?p=git/git.git;a=commitdiff;h=2fa9a0fb31cbf01e8318a02c3e222d7fd3fd0a83
Don't see any intent to support "mixed-sensitivity" matching in it.
> It's such a hack, and I don't think we will ever improve to make it work in
> the general case, as regexes are simply too complex for us to handle all
> possible inputs.
As far as I understand git uses host library implementation of regcomp and
regexec so we cannot fix that side. Writing code to modify regexes is not
worth the effort.
FWIW I'm in favor of deprecating this functionality.
Libor
--
Libor Pechacek
SUSE L3 Team, Prague
^ permalink raw reply
* Re: [PATCH] No color diff when redirecting to file
From: Matthieu Moy @ 2011-01-23 17:29 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Sascha Peilicke, git
In-Reply-To: <201101231547.18529.j6t@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> That said, I cannot reproduce. Perhaps your configuration says
> color.ui=always? If so, then this is expected behavior and not a bug, IMO.
I think you're right.
I tested this (without the patch):
git diff
=> I get color, because I have color.ui = auto
git diff | cat
=> I don't get color
git diff --color=auto | cat
=> no color
git diff --color=always | cat
=> colors, because I've asked
this seems to be just the right behavior.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH] No color diff when redirecting to file
From: Johannes Sixt @ 2011-01-23 14:47 UTC (permalink / raw)
To: Sascha Peilicke; +Cc: git
In-Reply-To: <201101231410.48528.saschpe@gmx.de>
The subject line reads as if you want that color markup appears in a file.
On Sonntag, 23. Januar 2011, Sascha Peilicke wrote:
> Previously, when having color diffs enabled and redirecting 'git diff'
> into a file, one ends up with a messed up file containing termcap color
> stuff. This change disables color when diff output is redirected into a
> file.
But this description and the patch read as if you don't want it.
That said, I cannot reproduce. Perhaps your configuration says
color.ui=always? If so, then this is expected behavior and not a bug, IMO.
-- Hannes
^ permalink raw reply
* Re: Fwd: Git and Large Binaries: A Proposed Solution
From: Pete Wyckoff @ 2011-01-23 14:14 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20110121222440.GA1837@sigill.intra.peff.net>
peff@peff.net wrote on Fri, 21 Jan 2011 17:24 -0500:
> cat >$HOME/local/bin/huge-clean <<'EOF'
> #!/bin/sh
>
> # In an ideal world, we could actually
> # access the original file directly instead of
> # having to cat it to a new file.
> temp="$(git rev-parse --git-dir)"/huge.$$
> cat >"$temp"
Just a quick aside. Since (a2b665d, 2011-01-05) you can provide
the filename as an argument to the filter script:
git config --global filter.huge.clean huge-clean %f
then use it in place:
$ cat >huge-clean
#!/bin/sh
f="$1"
echo orig file is "$f" >&2
sha1=`sha1sum "$f" | cut -d' ' -f1`
cp "$f" /tmp/big_storage/$sha1
rm -f "$f"
echo $sha1
-- Pete
^ permalink raw reply
* [PATCH] No color diff when redirecting to file
From: Sascha Peilicke @ 2011-01-23 13:10 UTC (permalink / raw)
To: git
[-- Attachment #1: Type: Text/Plain, Size: 961 bytes --]
Previously, when having color diffs enabled and redirecting 'git diff'
into a file, one ends up with a messed up file containing termcap color
stuff. This change disables color when diff output is redirected into a
file.
Signed-off-by: Sascha Peilicke <saschpe@gmx.de>
---
diff.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/diff.c b/diff.c
index 5422c43..cd1ecb0 100644
--- a/diff.c
+++ b/diff.c
@@ -2854,6 +2854,11 @@ void diff_setup(struct diff_options *options)
DIFF_OPT_SET(options, COLOR_DIFF);
options->detect_rename = diff_detect_rename_default;
+ struct stat buf;
+ if (fstat(fileno(options->file), &buf) != -1 && S_ISREG(buf.st_mode)) {
+ DIFF_OPT_CLR(options, COLOR_DIFF);
+ }
+
if (diff_no_prefix) {
options->a_prefix = options->b_prefix = "";
} else if (!diff_mnemonic_prefix) {
--
1.7.3.4
--
Mit freundlichen Grüßen,
Sascha Peilicke
http://saschpe.wordpress.com
[-- Attachment #2: This is a digitally signed message part. --]
[-- Type: application/pgp-signature, Size: 198 bytes --]
^ permalink raw reply related
* Re: Parameter --color-words not documented for "git show"
From: Jakub Narebski @ 2011-01-23 10:35 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sebastian Pipping, Thomas Rast, Git ML
In-Reply-To: <7vk4hzqnbx.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Sebastian Pipping <webmaster@hartwork.org> writes:
>> On 01/20/11 21:27, Thomas Rast wrote:
>>> Quote from the latter:
>>>
>>> This manual page describes only the most frequently used options.
>>
>> Okay. Is that a good a idea?
>
> Yes; the alternative is to list everything.
>
>> Is --abbrev-commit really used more
>> frequently with "git show" than --color-words is?
>
> I see this as a not-so-helpful-but-still-interesting question.
>
> It depends on who you are, and if one wants to pick the most often used
> ones, that selection may or may not coincide with _your_ usage pattern nor
> mine. The original author apparently thought so, you seem to think
> color-words is used a lot more often, and I personally think neither is
> used often at all. So should we swap them, keep things as-is, or remove
> both?
>
> We obviously cannot take a poll to update the list every time a new user
> starts using git, but it might make sense to review them every once in a
> while.
There is also additional problem, namely that because "git show" shows
commit and can show diff, therefore it accepts same formatting options
as "git log", and when set to display patch it accepts any diff family
options.
Should we then list most common porcelanish diff options, or just
refer to git-diff(1) manpage?
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* [BUG] difference of info from diff and blame again
From: Semyon Kirnosenko @ 2011-01-23 10:33 UTC (permalink / raw)
To: git
Hi.
I have to repost about diff and blame difference problem.
The initial discussion ends here:
http://permalink.gmane.org/gmane.comp.version-control.git/165012
I have the same problem on git repo. The problem is some line is marked
in diff as added, but in blame it is marked as added in some older
revision. And vice versa line is not marked as added in diff but it is
marked in blame. And now it's about non-whitespace lines. Here are
several examples:
revision: 711cf3a026a539f68ab647e012f145a03e12a5e7
file: update-cache.c
line: 127
revision: 40469ee9c6a6f4c85df5520ef719bba3d38a64f0
file: sha1_file.c
line: 234
Diff error in revision 198b0fb635ed8a007bac0c16eab112c5e2c7995c in file
date.c.
Line 184: added in diff, but blame say otherwise.
Line 215: added in diff, but blame say otherwise.
Line 219: added in diff, but blame say otherwise.
Line 259: added in diff, but blame say otherwise.
Line 260: added in diff, but blame say otherwise.
Line 193: not added in diff, but blame say otherwise.
Line 212: not added in diff, but blame say otherwise.
Line 222: not added in diff, but blame say otherwise.
Line 300: not added in diff, but blame say otherwise.
Line 315: not added in diff, but blame say otherwise.
^ permalink raw reply
* Re: Move git-stash from one machine (or working copy) to another
From: Andreas Schwab @ 2011-01-23 10:08 UTC (permalink / raw)
To: Patrick Doyle; +Cc: git
In-Reply-To: <AANLkTin2M+dLUOFnAKqNvYn04NumCmmQ331Yfb9ieW-D@mail.gmail.com>
Patrick Doyle <wpdster@gmail.com> writes:
> It seems to me that if I could git-stash on machine1, take that stash
> with me (somehow) to machine2, and then pop it there, that would be
> easier.
machine1$ git stash
machine2$ git fetch machine1:/repo refs/stash
machine2$ git checkout FETCH_HEAD .
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: Move git-stash from one machine (or working copy) to another
From: Matthieu Moy @ 2011-01-23 9:04 UTC (permalink / raw)
To: David Aguilar; +Cc: Junio C Hamano, Patrick Doyle, git
In-Reply-To: <1E3784B5-76A2-4071-989D-46E271BB1BAC@gmail.com>
David Aguilar <davvid@gmail.com> writes:
> This won't handle all cases, but it should do the trick 80%+ of the
> time.
>
> % git diff > foo.patch
Or, if the patch produced by "git diff" doesn't do it:
git commit -am foo
git format-patch -1
git reset HEAD^
> (on other machine)
> % git apply foo.patch
In one command:
git diff | ssh other-machine 'cd /path/to/repo && git apply'
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: Move git-stash from one machine (or working copy) to another
From: David Aguilar @ 2011-01-23 2:05 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Patrick Doyle, git
In-Reply-To: <7vfwsmp2op.fsf@alter.siamese.dyndns.org>
On Jan 21, 2011, at 9:49 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Patrick Doyle <wpdster@gmail.com> writes:
>
>> Is there an easy way to move work in progress from one machine to
>> another?
>>
>> One way to do it might be something like this:
>>
>> machine1$ git checkout -b movewip
>> machine1$ git add .
>> machine1$ git commit -m "Moving work in progress"
>> machine1$ git push origin movewip:movewip
>>
>> machine2$ git fetch origin movewip:movewip:
>> machine2$ git checkout movewip
>> machine2$ git reset HEAD^
>> machine2$ git stash
>> machine2$ git checkout master
>> machine2$ git stash pop
>>
>> # go through and delete movewip branches on machine1, machine2, and
>> the origin server
>>
>> Except for some possible typos, this seems like it would work, but
>> seems to be awfully clumsy. Is there a more elegant way to
>> accomplish
>> this?
>
> If your two machines can talk directly with each other (which seems
> to be
> the case from your "take that with me (somehow) to machine2"), you
> don't
> have to push and fetch through the origin.
This won't handle all cases, but it should do the trick 80%+ of the
time.
% git diff > foo.patch
(on other machine)
% git apply foo.patch
--
David
^ permalink raw reply
* Re: cannot fetch arm git tree
From: Jello huang @ 2011-01-21 14:30 UTC (permalink / raw)
To: Detlef Vollmann
Cc: Russell King - ARM Linux, linux-arm-kernel, git,
Uwe Kleine-König
In-Reply-To: <4D3997FE.5030109@vollmann.ch>
[-- Attachment #1.1: Type: text/plain, Size: 1570 bytes --]
not a lucky dog. i used 1.6.0,there was a large pack.
On 21 January 2011 22:28, Detlef Vollmann <dv@vollmann.ch> wrote:
> On 01/21/11 14:57, Russell King - ARM Linux wrote:
>
>> On Fri, Jan 21, 2011 at 02:47:28PM +0100, Uwe Kleine-König wrote:
>>
>>> Hi Detlef,
>>>
>>> On Fri, Jan 21, 2011 at 02:38:11PM +0100, Detlef Vollmann wrote:
>>>
>>>> On 01/16/11 14:42, Russell King - ARM Linux wrote:
>>>>
>>>>> Let's say you already have a copy of my tree from a month ago, and
>>>>> Linus
>>>>> has pulled some work from me into his tree, and repacked his tree into
>>>>> one
>>>>> single pack file. At the moment, the largest pack file from Linus is
>>>>> 400MB plus a 50MB index.
>>>>>
>>>>> You already have most of the contents of that 400MB pack file, but if
>>>>> you're missing even _one_ object which is contained within it, git will
>>>>> have to download the _entire_ 400MB pack file and index file to
>>>>> retrieve
>>>>> it.
>>>>>
>>>> I thought this has changed with "smart http" in git 1.6.6.
>>>> Am I missing something?
>>>>
>>> Well, not all http repos offer smart http. E.g. Russell doesn't[1],
>>> probably because the serving machine doesn't have the power to nice
>>> serve a repo via git:// or smart http://.
>>>
>>
>> What is smart http? I don't particularly follow git developments.
>>
> It seems to be an implementation of the git protocol using
> HTTP as transport.
> Some info on this is at <http://progit.org/2010/03/04/smart-http.html>.
>
> Detlef
>
>
--
JUST DO IT,NOTHING IS IMPOSSIBLE
[-- Attachment #1.2: Type: text/html, Size: 2490 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* Re: cannot fetch arm git tree
From: Jello huang @ 2011-01-17 1:49 UTC (permalink / raw)
To: Russell King - ARM Linux; +Cc: git, linux-arm-kernel, Uwe Kleine-König
In-Reply-To: <20110116134248.GD27542@n2100.arm.linux.org.uk>
[-- Attachment #1.1: Type: text/plain, Size: 2353 bytes --]
Russell,thanks for your reply so elaborate and i have gotten the idea now.
On 16 January 2011 21:42, Russell King - ARM Linux
<linux@arm.linux.org.uk>wrote:
> On Sun, Jan 16, 2011 at 09:10:17PM +0800, Jello huang wrote:
> > yes,git doesn't handle that case and i rename the pack name,but there is
> > also the similar error.Now i just delet the git tree and clone it again
> > tonight .
>
> _Always_ without fail fetch Linus' tree before pulling my tree.
>
> My tree is a rsync clone of the objects and pack files in Linus' tree,
> plus whatever git decided to build on top of that - for local commits
> that's individual object files. For remote pulls, that's probably a few
> small pack files.
>
> There is *no* repacking of my tree. So the only times it gets 'repacked'
> is when Linus repacks his tree.
>
> Let's say you already have a copy of my tree from a month ago, and Linus
> has pulled some work from me into his tree, and repacked his tree into one
> single pack file. At the moment, the largest pack file from Linus is
> 400MB plus a 50MB index.
>
> You already have most of the contents of that 400MB pack file, but if
> you're missing even _one_ object which is contained within it, git will
> have to download the _entire_ 400MB pack file and index file to retrieve
> it.
>
> However, if you first fetch Linus' tree via the git protocol, it can just
> request the objects it doesn't have from the git server. That will mean
> you'll have all the objects in the large pack files before you start trying
> to pull my tree, and git won't have to download 400MB for the sake of
> retrieving just maybe 10k that you didn't have.
>
> This isn't something special with my tree - it's a side effect of the
> http protocol git uses. So, before you fetch _any_ http-based git tree,
> first make sure you're up to date with Linus'.
>
> (I update my tree from Linus' in rsync mode to make http-based stuff a
> lot more friendly to people using it - some of whom are stuck behind
> firewalls which can only do http. Fetching a constantly repacked git
> tree via http results in hundreds of megabytes needing to be fetched
> every time.)
>
> So please, whenever possible, always fetch Linus' latest tree _first_
> and then mine. Same goes for any other http based tree which doesn't
> auto-repack.
>
--
JUST DO IT,NOTHING IS IMPOSSIBLE
[-- Attachment #1.2: Type: text/html, Size: 2867 bytes --]
[-- Attachment #2: Type: text/plain, Size: 176 bytes --]
_______________________________________________
linux-arm-kernel mailing list
linux-arm-kernel@lists.infradead.org
http://lists.infradead.org/mailman/listinfo/linux-arm-kernel
^ permalink raw reply
* git-svn dcommit to HTTP proxy can fail
From: Ben Jackson @ 2011-01-22 19:58 UTC (permalink / raw)
To: git
git-svn dcommit turns each new git commit into an SVN commit, then uses
its normal fetch mechanism to pull the new revision back from the SVN
server. It compares the fetched version to the git version with diff-tree.
If they don't match you generally get a very cryptic error message.
The primary reason for the mismatches seems to be SVN HTTP proxies. When
the svn-remote is an HTTP proxy the commit is delegated to the actual
SVN server. It acknowledges the commit as usual and then updates the
proxies with a post-commit hook. This means that the commit+fetch combo
is often fast enough to catch the proxy in the pre-commit state and no
new revisions are fetched at all.
Someone has described this with examples on stackoverflow:
http://stackoverflow.com/questions/4238876/git-svn-fails-to-dcommit-even-after-clean-checkout
I wrote an answer to that question including a patch which adds retries
(polling the proxy until the expected revision appears). I am currently
using that patch at work and it is a big improvement. I keep meaning to
"productize" it and send it to this list, but there are two remaining
problems:
1. The proxy update seems to have at least two phases: one that creates
the commit and one that sets metadata (such as author). The new code is
pretty good at racing in and catching the new commit in an intermediate
state with the author "syncuser" (instead of me). I don't think that
"syncuser" is a fixed name and I worry that waiting for the commit author
email address to match will break if there is any user mapping going on.
2. My linear backoff retries total about 10 seconds, and I've blown
that budget with commits of large binaries (eg FPGA images). You do get
a much more informative error ("New revision 1234 did not appear in
repository after 30 retries.") but it still fails. Of course I can
increase it, but what's reasonable?
Suggestions?
--
Ben Jackson AD7GD
<ben@ben.com>
http://www.ben.com/
^ permalink raw reply
* Re: [PATCH 4/5] fast-export: Introduce --inline-blobs
From: Jonathan Nieder @ 2011-01-22 19:18 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Drew Northup, Junio C Hamano, Git List, David Barr,
Sverre Rabbelier
In-Reply-To: <20110122092416.GA7827@kytes>
Ramkumar Ramachandra wrote:
> Agreed. I wouldn't like to introduce an extra dependency either. I was
> talking about using it for prototyping- if the final version includes
> an extra dependency, it's unlikely to get merged into git.git :) The
> final design will probably use an in-memory B+ tree, but I haven't
> thought about that hard enough.
Immediate reaction:
Please no. There is a value to simplicity.
As you mention, the final form is a way off, so as long as people are
careful not to get locked into bad implementation decisions, I think
it is okay. I have refrained from nitpicking the implementation so
far because the design and interface are not obvious yet.
In this particular case, I am dreaming that we will discover a hidden
"mkdir -p" node-action in the dumpfile format so the list of
directories will not be needed. ;-)
Re Junio's critique:
is it possible to use
1) a table with callbacks? See the source code to unifdef for
inspiration.
2) separate code paths for different input states? fast-import.c
does this.
3) separate "parsing" and "acting" code? That can open the door to a
little paralellism, though not necessarily enough to matter.
parser actor
read a chunk
determine the first "thing to do"
pick up the first "thing to do"
do it
determine the next "thing to do"
pick up the next "thing to do"
do it
There is potential parallelism because the parser can keep
chugging along if the actor is blocked, say, writing its
output to the network.
Syntactically: the actor function (write_dump) calls the parser
function (next_command) to ask what to do next. If wanted, a later
refactoring could make that parser function just grab an action off
of a queue, while the parser proper runs in the background.
And of course if the "thing to do" data structure is simple enough,
this can also make the code easier to read.
I mention these ideas because some of them (especially #2) could make
the prototyping a lot easier as well as resulting in code that is
easier to review.
Re error handling:
Writing robust code (e.g., checking for errors) is also a lot easier
when done from the start. The svn-fe error handling is known to be a
problem (see BUGS in contrib/svn-fe/svn-fe.txt). So yes, I also
consider avoiding segfaults and deadlocks and catching parse errors to
be worthwhile things.
Thanks.
Jonathan
^ permalink raw reply
* Re: git bisect problems/ideas
From: Jakub Narebski @ 2011-01-22 14:52 UTC (permalink / raw)
To: Johannes Sixt
Cc: Christian Couder, Aaron S. Meurer, git, SZEDER Gábor,
Jonathan Nieder
In-Reply-To: <201101212304.36741.j6t@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> On Freitag, 21. Januar 2011, Christian Couder wrote:
> > On Wed, Jan 19, 2011 at 8:44 PM, Aaron S. Meurer <asmeurer@gmail.com> wrote:
> > > If no, I think --reverse is actually a suitable fix.
> >
> > Yeah, but I think that what Dscho started was probably better. The
> > problem is just that it is not so simple to implement and no one yet
> > has been interested enough or took enough time to finish it.
>
> Let me throw in an idea:
>
> Add two new sub-commands:
>
> * 'git bisect regression': this is a synonym for 'git bisect start'.
>
> * 'git bisect improvement': this also starts a bisection, but subsequently the
> operation of 'git bisect good' and 'git bisect bad' is reversed.
I like this!
--
Jakub Narebski
Poland
ShadeHawk on #git
^ permalink raw reply
* Re: Creating remote branch called HEAD corrupts remote clones
From: Felipe Contreras @ 2011-01-22 12:46 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, Stephen Kelly, KDE PIM
In-Reply-To: <7vk4hyp38i.fsf@alter.siamese.dyndns.org>
On Fri, Jan 21, 2011 at 7:37 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Felipe Contreras <felipe.contreras@gmail.com> writes:
>
>> I don't fully understand the issue, so excuse me if this is totally
>> wrong, but wouldn't a rule like 'you can't create a branch for which
>> there's already a symbolic ref' do the trick?
>
> But whose symbolic ref are you checking against? Your own, or ones in
> somebody else's repository that you haven't recently updated from?
The local ones. That means that somebody can't create a 'HEAD' branch
locally, and can't push a 'HEAD' branch either, as the remote server
would already have a 'HEAD' symbolic link. And actually, if for some
reason I have a FOO_HEAD, and I fetch a branch called bob/FOO_HEAD,
obviously the local symbolic ref without namespace should take
precedence.
--
Felipe Contreras
^ permalink raw reply
* Re: [PATCH 3/3] setup: always honor GIT_WORK_TREE and core.worktree
From: Nguyen Thai Ngoc Duy @ 2011-01-22 10:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Nieder, git, Maaartin
In-Reply-To: <7v39omotxg.fsf@alter.siamese.dyndns.org>
On Sat, Jan 22, 2011 at 3:58 AM, Junio C Hamano <gitster@pobox.com> wrote:
> I was re-reading this thread, and changed my mind; I think we should have
> this series to avoid unnecessary regression, with or without clarifying
> (5), before 1.7.4 final.
Sorry for late response. If we no longer consider this work-around,
perhaps git.txt and config.txt should be updated to reflect it?
--
Duy
^ permalink raw reply
* Re: [PATCH 2/5] vcs-svn: Start working on the dumpfile producer
From: Ramkumar Ramachandra @ 2011-01-22 9:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git List, Jonathan Nieder, David Barr, Sverre Rabbelier
In-Reply-To: <7v39olok4l.fsf@alter.siamese.dyndns.org>
Hi Junio,
Junio C Hamano writes:
> Ramkumar Ramachandra <artagnon@gmail.com> writes:
> > Start off with some broad design sketches. Compile succeeds, but
> > parser is incorrect. Include a Makefile rule to build it into
> > vcs-svn/lib.a.
> >
> > Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>
> I initially thought I should refrain from doing a usual picky review for
> contrib/ material, but realized this does not go to contrib/, so...
Thanks for the review. A new iteration including tests is almost
ready- your review came in at the perfect time :)
> > diff --git a/vcs-svn/dump_export.c b/vcs-svn/dump_export.c
> > new file mode 100644
> > index 0000000..04ede06
> > --- /dev/null
> > +++ b/vcs-svn/dump_export.c
> > ...
> > +void dump_export_begin_rev(int revision, const char *revprops,
> > + int prop_len) {
>
> Style. "{" at the beginning of the next line (same everywhere).
Fixed.
> > +void dump_export_node(const char *path, enum node_kind kind,
> > + enum node_action action, unsigned long text_len,
> > + unsigned long copyfrom_rev, const char *copyfrom_path) {
> > + printf("Node-path: %s\n", path);
> > + printf("Node-kind: ");
> > + switch (action) {
> > + case NODE_KIND_NORMAL:
> > + printf("file\n");
> > + break;
> > + case NODE_KIND_EXECUTABLE:
> > + printf("file\n");
> > + break;
> > + case NODE_KIND_SYMLINK:
> > + printf("file\n");
> > + break;
> > + case NODE_KIND_GITLINK:
> > + printf("file\n");
> > + break;
> > + case NODE_KIND_SUBDIR:
> > + die("Unsupported: subdirectory");
> > + default:
> > + break;
> > + }
>
> Hmph, is it expected that the repertoire of node-kind stay the same, or
> will it be extended over time? It might make sense to change the above
> switch a more table-driven one. The same for node-action that follows
> this part of the code.
I needed more than a lookup table, because I wanted the flexibility to
execute arbitrary statements in each case. It's extended in the latest
version (will come up on the list soon).
> > diff --git a/vcs-svn/svnload.c b/vcs-svn/svnload.c
> > new file mode 100644
> > index 0000000..7043ae7
> > --- /dev/null
> > +++ b/vcs-svn/svnload.c
> > @@ -0,0 +1,294 @@
> > ...
> > +static struct strbuf blobs[100];
> > +
>
> Is there a rationale for "100", or is it just a random number that can be
> tweakable at the source level? Would we want to have a symbolic constant
> for it?
>
> The "blank" line has a trailing whitespace.
Fixed. I used this to illustrate the problem with persisting blobs- in
this version, I persist 100 blobs; this is totally arbitrary and
inextensible. Since the latest version leverages the --inline-blobs
feature of fast export, this is totally unecessary.
> > +static struct {
> > + unsigned long prop_len, text_len, copyfrom_rev, mark;
> > + int text_delta, prop_delta; /* Boolean */
> > ...
> > +static void reset_node_ctx(void)
> > ...
> > + node_ctx.text_delta = -1;
> > + node_ctx.prop_delta = -1;
>
> Does your "Boolean" type take values 0 or -1? Or is it perhaps a tristate
> false=0, true=1, unknown=-1? If so, the comment on the type above needs
> to be fixed.
Fixed. Thanks for pointing this out.
> > + strbuf_reset(&node_ctx.copyfrom_path);
> > + strbuf_reset(&node_ctx.path);
> > +}
> > +
> > +static void populate_props(struct strbuf *props, const char *author,
> > + const char *log, const char *date) {
>
> Style on "{" (look everywhere).
Fixed.
> > + strbuf_reset(props);
>
> Trailing whitespace.
Fixed.
> > ...
> > + t ++;
>
> Unwanted SP before "++" (look everywhere).
Fixed.
> > + tm_time = time_to_tm(strtoul(t, NULL, 10), atoi(tz_off));
>
> Has your caller already verified tz_off is a reasonable integer?
Fixed.
> > ...
> > + if (!memcmp(t, "D", 1)) {
> > + node_ctx.action = NODE_ACTION_DELETE;
> > + }
> > + else if (!memcmp(t, "C", 1)) {
>
> Style: "} else if (...) {".
Fixed.
> > ...
> > + node_ctx.kind = NODE_KIND_GITLINK;
> > + else if (!memcmp(val, "040000", 6))
>
> Is <mode> guaranteed to be a 6-digit octal, padded with 0 to the left?
>
> The manual page of git-fast-import seems to hint that is the case, but I
> am double checking, as the leading zero is an error in the tree object.
The documentation and commit messages seem to hint that this is the
case. I'm digging to see if there's anything wrong with this.
> > + node_ctx.kind = NODE_KIND_SUBDIR;
> > + else {
> > + if (!memcmp(val, "755", 3))
> > + node_ctx.kind = NODE_KIND_EXECUTABLE;
> > + else if(!memcmp(val, "644", 3))
>
> Style; missing SP after "if/switch" (look everywhere).
Fixed.
> > + node_ctx.kind = NODE_KIND_NORMAL;
> > + else
> > + die("Unrecognized mode: %s", val);
> > + mode_incr = 4;
> > + }
> > + val += mode_incr;
>
> Hmm, this whole thing is ugly.
>
> Perhaps a table-lookup, or at least a helper function that translates
> "val" to node-kind (while advancing val as the side effect) may make it
> easier to read?
Fixed. I used a helper function.
> > ...
> > + to_dump = &blobs[strtoul(val + 1, NULL, 10)];
>
> Has anybody so far verified the decimal number at (val+1) is a reasonable
> value, or am I seeing a future CVE here?
Fixed.
> Do we care what follows the len of digits on this line? Does a line in G-F-I
> stream allow trailing garbage (this question is not limited to this part
> of the code)?
Hm, I haven't thought about this hard enough. Currently, both svn-fi
and svn-fe ignore any trailing garbage/ commands they don't
understand. Are there some issues that we haven't anticipated?
> > +int svnload_init(const char *filename)
> > ...
> > +void svnload_deinit(void)
>
> Whoever needs to add an element to rev_ctx must consistently touch reset,
> init and deinit. Would it help him/her if you moved the code so that
> these functions are close together from the beginning?
Fixed.
-- Ram
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox