login
Search
Here are the topics that have been cooking. Commits prefixed with '-' are only in 'pu' while commits prefixed with '+' are in 'next'. The ones marked with '.' do not appear in any of the integration branches, but I am still holding onto them. The tip of 'next' has been rewound and rebuilt on top of v1.7.2; also a few topics have been ejected as I've been warning to. I'll start merging more stuff to 'next' in the next round after reading them over again. -------------------------------------------------- [Graduated to "master"] * gp/pack-refs-remove-empty-dirs (2010-07-06) 1 commit (merged to 'next' on 2010-07-14 at 7d25131) + pack-refs: remove newly empty directories * pt/git-gui (2010-08-02) 13 commits + Merge git://repo.or.cz/git-gui into pt/git-gui + git-gui: fix size and position of window panes on startup + git-gui: mc cannot be used before msgcat has been loaded + git-gui: use textconv filter for diff and blame + git-gui: Avoid using the <<Copy>> binding as a menu accelerator on win32 + git-gui: fix shortcut creation on cygwin + git-gui: fix PATH environment for mingw development environment + git-gui: fix usage of _gitworktree when creating shortcut for windows + git-gui: fix "Explore Working Copy" for Windows again + git-gui: fix usage of themed widgets variable + git-gui: Handle failure of core.worktree to identify the working directory. + Merge branch 'maint' + git-gui: check whether systems nice command works or disable it * rr/svn-fe (2010-07-29) 2 commits + contrib/svn-fe: Add the svn-fe target to .gitignore + contrib/svn-fe: Fix IncludePath * sv/maint-diff-q-clear-fix (2010-08-02) 1 commit + Fix DIFF_QUEUE_CLEAR refactoring -------------------------------------------------- [New Topics] * jc/sha1-name-find-fix (2010-08-02) 1 commit - sha1_name.c: fix parsing of ":/token" syntax * jn/doc-pull (2010-08-02) 1 commit - Documentation: flesh out “git pull” description * jn/maint-gitweb-dynconf (2010-07-30) 1 commit - ...
What needs to be fixed up before this effort can graduate? I find the
fixups here to be really helpful, even without the automated skew
detection that has been proposed. And even if we fix the root problem
with some new all-singing pack format, I suspect that may be a ways
out, so it would be nice if these patches could get included for now....
- Ted
--
To reduce the risk of double-work, I need to clarify. I meant to say that I can find enough material, especially what Peff wrote, in the discussion that followed in the thread to do the clean-up myself. No need to resend by anybody unless there are material differences from what have been discussed so far that need to be incorporated in the final series. --
The only bad log message should be the final one, which should be dropped anyway. I would recommend just merging the first two for now, and Ted can tweak his core.clockskew manually. -Peff --
After re-reviewing the one that is queued, the use of TMP_MARK smelled somewhat bad to me. It is named TMP_ exactly because it is meant to be used in a closed callpath---you can use it but you are supposed to clean it before you return the control to the caller, so that the caller can rely on TMP_MARK absent from any objects. Use of UNINTERESTING is similarly not kosher if this were to be used in larger context outside of "do 'tags --contains' and exit". You noted these two points in your original RFC patch. Besides, "contains()" is too generic a name to live in commit.h. My gut feeling is that it is probably Ok if contains() and its recursive helper are moved to builtin/tag.c and are made static, to make it clear that this should not be reused outside the current context as a generic "contains" function. It would probably help to have a comment at the end of list_tags() to say that TMP_MARK _ought_ to be cleaned before leaving the function but we don't do that because we know it is the last function in the callchain before we exit. By the way, I wonder why pop_most_recent_commit() with a commit_list, which is the usual revision traversal ingredient for doing something like this, was not used in the patch, though. Is it because depth-first was necessary? --
Oops, thanks, I had forgotten that the marks needed to be addressed.
Should I be introducing new flags? We have 27 flag bits, but I would
I agree it's a pretty generic name. I was trying to make this as generic
as possible, at least within the domain of commits, so it could be a
But my intent was to have a generic contains function. I was planning on
applying this to "git branch --contains", as well, but my initial
approach wasn't really any faster than the current code (probably
because the number of branches tends to be small compared to the number
of tags).
In an ideal object-oriented world, the interface would be:
void contains_init(struct contains_context *c,
struct commit_list *needles);
void contains_check(struct contains_context *c, struct commit *haystack);
void contains_free(struct contains_context *c);
But for memory use reasons, we don't get our own private copy of each
commit. We can drop the "init" and have a "free" or "clear" which clears
marks on the global commit objects. But you also _must_ use the same
needle list for each contains check, or you will get bogus results
(since the marks are essentially partial cached answers).
I guess we could do:
static struct commit_list *contains_needles;
void contains_init(struct commit_list *needles)
{
if (contains_needles)
die("BUG: somebody else is already checking contains!");
copy_commit_list(&contains_needles, needles);
}
void contains_check(struct commit *haystack)
{
/* like contains, but check against our static contains_needles */
}
void contains_clear(struct contains_context *c)
{
/* free contains_needles list, set it to NULL */
/* clear commit marks */
Yes, it is because of the depth-first nature. The intent is to mark
whole sections of the subgraph as "does not contain". If you can think
of a clever way around that, I would be interested to hear it. The fact
that it is a DFS ...I'm going to side-track this slightly. I wonder why branch and tag have --contains, but it is not more generically available via rev-list? I needed it the other day and spent 5 minutes looking at what it would take before I ended up just calling merge-base in a loop for the commits I wanted to check. j. --
I'm not sure rev-list makes the most sense. We already have "show commits in X, but not in Y". But I gather you wanted "from a list (U,V,W,X), print each that contains Y". Which is not really a rev-list function anymore, as it is not about listing revisions, but rather about grepping a list you've given it. Something like "git for-each-ref --contains" seems more sensible to me, though it is not as generic as we could make it (I cannot use an arbitrary list of commits to the "haystack", but only ones that have refs pointing to them). -Peff --
Well maybe, but rev-list will already take a list of revs on stdin and you can give it --no-walk, so it has already been abused to do more than strictly list revisions. And what do you call this? Sure, and if I wanted to do that, I could've just created a bunch of temporary light-weight tags for those commits I was potentially interested in and then used tag --contains. :-) So I don't think rev-list is such a bad place after all. j. --
At work we have some 100 topics branches per kernel revision, and I have a repository with 332 branches in it at the moment. So there may very well be repo's where git branch --contains might be faster with your approach. - Ted --
In the longer term it would be not just nice but necessary for us to come up with a scheme where different codepaths can "allocate bits from object flags", without having to fear stepping on each other's toes. Some possible approaches off the top of my head are: - Extend "struct object" by another uint64_t to give it 64 more bits? That would make the minimum object size from 24 bytes to 32 bytes and during a pack-object session we would keep a lot of objects (not just commits but trees and blobs) in core, so we probably would not want to do this. - Extend "struct commit" by another uint32_t? Currently a "struct commit" is 72 bytes on x86_64 (there is an unfortunate 4-byte padding gap between indegree and date), and 48 bytes on i386 and this would enlarge the latter to 52 bytes (this comes free on 64-bit archs). As we need a lot more bits on commits than on other objects (e.g. left-right do not need to be placed on trees or blobs), this approach might be more space efficient. - Use one bit in the current flags section to signal "extended flag bits present on this object", and have a separate hashtable for minority objects that have that bit set? This would work only for flag bits that are rarely used (otherwise the secondary hashtable will be full of objects and per-object overhead will kill us). - Migrate some users of flag bits that only mark small miniroty of commits to use dedicated hashtable to free their bits [*1*]. I don't know if there are candidates for doing this offhand. Just uttering it as an idea. Independent of this issue, I suspect that we might want to fold object.used into the general set of flags---it is only used by fsck as far as I remember. [Footnote] *1* Also we would want to do something similar to the commit.util field so that more than one utility libraries can attach their own stuff to each commit. It _might_ make sense to instead get rid of commit.util and migrate the users ...
Yes, the point is that there may be cases where it may be better to migrate some current commit.util users to that API. --
After thinking about this a bit more, I changed my mind.
I think depth-first traversal from all tag tips, without running any
traversal from the wanted commit, has a serious downside. What happens
when you run "git tag --contains master" in a Linus tree after a major
release but before he tags the -rc1 and closes the merge window? Doesn't
the algorithm run all the way down to the root, only to say nothing?
Admittedly the traversal will visit each commit once (starting from
v2.6.12 down to v2.6.12-rc2, then skipping v2.6.12-rc2 through v2.6.12-rc6
because they are already seen and known not to contain the "master", then
starting from v2.6.13 down to v2.6.12, and so on), but visiting all the
commits down to root feels really wrong.
I think the merge-base traversal is essential. Suppose we have hundreds
of commits with two tags 'T1' and 'T2':
o---T2 o---o---Y
/ /
---o---o---o---o---o---o---o---%---...---X---*---T1
If you are probing for 'X', traversing from all the tags until you hit a
wanted commit will stop at a reasonable point (namely, 'X') if you are
lucky and you started from 'T1' not 'T2', with your new algorithm. If you
are unlucky and started from 'T2', you will however traverse down to root
from 'T2' which would be the bulk of the history.
Also, when you are probing for 'Y', you end up traversing all the way
down, whether you start from 'T1' or 'T2', don't you?
You can (and should) dig both from 'Y' and 'T1' and stop traversal at '*',
as that commit is an ancestor of 'Y' and at that point you have proven
that any ancestor of that you will find by traversing further will not
find 'Y'. Similarly for 'Y' and 'T2' pair, which should stop at '%'.
As the author of show-branch, I think I know one trick that may speed this
up without compromising the worst case scenario.
We can run this traversal in parallel for many tags at once. Use bit #0
of object.flags for the wanted ...I keep on trying to use this option and then remembering it wasn’t merged yet. So for what it’s worth, I like it. --
I'm pretty sure I had send a new version of this one, but judging from the mailing list's archive, I guess I mis-sent it and it never went through ;-). Here's a new one, with very minor revisions : * diff_long_opt renamed to parse_long_opt * Use the same wording as api-parse-options.txt : separate/sticked forms (essentially in commit messages and comments). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ --
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
revision.c | 7 +++++--
t/t6018-rev-list-glob.sh | 6 ++++++
2 files changed, 11 insertions(+), 2 deletions(-)
diff --git a/revision.c b/revision.c
index 489a3c2..f241f34 100644
--- a/revision.c
+++ b/revision.c
@@ -1484,6 +1484,8 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
{
int i, flags, left, seen_dashdash, read_from_stdin, got_rev_arg = 0;
const char **prune_data = NULL;
+ const char *optarg;
+ int argcount;
/* First, search for "--" */
seen_dashdash = 0;
@@ -1530,10 +1532,11 @@ int setup_revisions(int argc, const char **argv, struct rev_info *revs, struct s
handle_refs(revs, flags, for_each_remote_ref);
continue;
}
- if (!prefixcmp(arg, "--glob=")) {
+ if ((argcount = parse_long_opt("glob", argv + i, &optarg))) {
struct all_refs_cb cb;
+ i += argcount - 1;
init_all_refs_cb(&cb, revs, flags);
- for_each_glob_ref(handle_one_ref, arg + 7, &cb);
+ for_each_glob_ref(handle_one_ref, optarg, &cb);
continue;
}
if (!prefixcmp(arg, "--branches=")) {
diff --git a/t/t6018-rev-list-glob.sh b/t/t6018-rev-list-glob.sh
index 58428d9..fb8291c 100755
--- a/t/t6018-rev-list-glob.sh
+++ b/t/t6018-rev-list-glob.sh
@@ -123,6 +123,12 @@ test_expect_success 'rev-list --glob=refs/heads/subspace/*' '
'
+test_expect_success 'rev-list --glob refs/heads/subspace/*' '
+
+ compare rev-list "subspace/one subspace/two" "--glob refs/heads/subspace/*"
+
+'
+
test_expect_success 'rev-list --glob=heads/subspace/*' '
compare rev-list "subspace/one subspace/two" "--glob=heads/subspace/*"
--
1.7.2.1.30.g18195
--
Part of a campaign for unstuck forms of options.
[jn: with some refactoring]
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
diff.c | 28 +++++++++++++++++++++++-----
1 files changed, 23 insertions(+), 5 deletions(-)
diff --git a/diff.c b/diff.c
index a08a56a..e98d59b 100644
--- a/diff.c
+++ b/diff.c
@@ -3035,16 +3035,34 @@ static int stat_opt(struct diff_options *options, const char **av)
char *end;
int width = options->stat_width;
int name_width = options->stat_name_width;
+ int argcount = 1;
arg += strlen("--stat");
end = (char *)arg;
switch (*arg) {
case '-':
- if (!prefixcmp(arg, "-width="))
- width = strtoul(arg + 7, &end, 10);
- else if (!prefixcmp(arg, "-name-width="))
- name_width = strtoul(arg + 12, &end, 10);
+ if (!prefixcmp(arg, "-width")) {
+ arg += strlen("-width");
+ if (*arg == '=')
+ width = strtoul(arg + 1, &end, 10);
+ else if (!*arg && !av[1])
+ die("Option '--stat-width' requires a value");
+ else if (!*arg) {
+ width = strtoul(av[1], &end, 10);
+ argcount = 2;
+ }
+ } else if (!prefixcmp(arg, "-name-width")) {
+ arg += strlen("-name-width");
+ if (*arg == '=')
+ name_width = strtoul(arg + 1, &end, 10);
+ else if (!*arg && !av[1])
+ die("Option '--stat-name-width' requires a value");
+ else if (!*arg) {
+ name_width = strtoul(av[1], &end, 10);
+ argcount = 2;
+ }
+ }
break;
case '=':
width = strtoul(arg+1, &end, 10);
@@ -3058,7 +3076,7 @@ static int stat_opt(struct diff_options *options, const char **av)
options->output_format |= DIFF_FORMAT_DIFFSTAT;
options->stat_name_width = name_width;
options->stat_width = width;
- return 1;
+ return argcount;
}
int diff_opt_parse(struct diff_options *options, const char **av, int ac)
--
1.7.2.1.30.g18195
--
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
revision.c | 74 ++++++++++++++++++++++++++++++++++---------------------
t/t4202-log.sh | 7 +++++
2 files changed, 53 insertions(+), 28 deletions(-)
diff --git a/revision.c b/revision.c
index 7e82efd..489a3c2 100644
--- a/revision.c
+++ b/revision.c
@@ -1148,6 +1148,8 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
int *unkc, const char **unkv)
{
const char *arg = argv[0];
+ const char *optarg;
+ int argcount;
/* pseudo revision arguments */
if (!strcmp(arg, "--all") || !strcmp(arg, "--branches") ||
@@ -1160,11 +1162,13 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
return 1;
}
- if (!prefixcmp(arg, "--max-count=")) {
- revs->max_count = atoi(arg + 12);
+ if ((argcount = parse_long_opt("max-count", argv, &optarg))) {
+ revs->max_count = atoi(optarg);
revs->no_walk = 0;
- } else if (!prefixcmp(arg, "--skip=")) {
- revs->skip_count = atoi(arg + 7);
+ return argcount;
+ } else if ((argcount = parse_long_opt("skip", argv, &optarg))) {
+ revs->skip_count = atoi(optarg);
+ return argcount;
} else if ((*arg == '-') && isdigit(arg[1])) {
/* accept -<digit>, like traditional "head" */
revs->max_count = atoi(arg + 1);
@@ -1178,18 +1182,24 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
} else if (!prefixcmp(arg, "-n")) {
revs->max_count = atoi(arg + 2);
revs->no_walk = 0;
- } else if (!prefixcmp(arg, "--max-age=")) {
- revs->max_age = atoi(arg + 10);
- } else if (!prefixcmp(arg, "--since=")) {
- revs->max_age = approxidate(arg + 8);
- } else if (!prefixcmp(arg, "--after=")) {
- revs->max_age = approxidate(arg + 8);
- } else if (!prefixcmp(arg, "--min-age=")) {
- revs->min_age = atoi(arg + 10);
- } else if (!prefixcmp(arg, "--before=")) {
- revs->min_age = approxidate(arg + 9);
- } else if (!prefixcmp(arg, "--until=")) {
- revs->min_age = ...Change the option parsing logic in revision.c to accept separate forms
like `-S foo' in addition to `-Sfoo'. The rest of git already accepted
this form, but revision.c still used its own option parsing.
Short options affected are -S<string>, -l<num> and -O<orderfile>, for
which an empty string wouldn't make sense, hence -<option> <arg> isn't
ambiguous.
This patch does not handle --stat-name-width and --stat-width, which are
special-cases where diff_long_opt do not apply. They are handled in a
separate patch to ease review.
Original patch by Matthieu Moy, plus refactoring by Jonathan Nieder.
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
diff.c | 87 ++++++++++++++++++++++++++++++++++--------
diff.h | 7 +++
t/t4013-diff-various.sh | 5 ++
t/t4013/diff.log_-S_F_master | 7 +++
t/t4202-log.sh | 12 ++---
5 files changed, 95 insertions(+), 23 deletions(-)
create mode 100644 t/t4013/diff.log_-S_F_master
diff --git a/diff.c b/diff.c
index 17873f3..bc8fa8e 100644
--- a/diff.c
+++ b/diff.c
@@ -2990,9 +2990,50 @@ static int opt_arg(const char *arg, int arg_short, const char *arg_long, int *va
static int diff_scoreopt_parse(const char *opt);
+static inline int short_opt(char opt, const char **argv,
+ const char **optarg)
+{
+ const char *arg = argv[0];
+ if (arg[0] != '-' || arg[1] != opt)
+ return 0;
+ if (arg[2] != '\0') {
+ *optarg = arg + 2;
+ return 1;
+ }
+ if (!argv[1])
+ die("Option '%c' requires a value", opt);
+ *optarg = argv[1];
+ return 2;
+}
+
+int parse_long_opt(const char *opt, const char **argv,
+ const char **optarg)
+{
+ const char *arg = argv[0];
+ if (arg[0] != '-' || arg[1] != '-')
+ return 0;
+ arg += strlen("--");
+ if (prefixcmp(arg, opt))
+ return 0;
+ arg += strlen(opt);
+ if (*arg == '=') { /* sticked form: --option=value */
+ *optarg = arg + 1;
+ return 1;
+ }
+ if (*arg != '\0')
+ return 0;
+ /* separate form: ...Just a nitpick question: why it is 'parse_long_opt' but just 'short_opt'? -- Jakub Narebski Poland ShadeHawk on #git --
I initially made *_long_opt prefixed with something (diff_ in my first version) because it's global, while short_opt is static. parse_short_opt is already taken in parse-option.c, so I'd rather avoid re-using it (it's static there, so technically, we can, but ...). -- Matthieu Moy http://www-verimag.imag.fr/~moy/ --
From: Jonathan Nieder <jrnieder@gmail.com>
As an optimization, the diff_opt_parse() switchboard has
a single case for all the --stat-* options. Split it
off into a separate function so we can enhance it
without bringing code dangerously close to the right
margin.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Matthieu Moy <Matthieu.Moy@imag.fr>
---
diff.c | 62 +++++++++++++++++++++++++++++++++++---------------------------
1 files changed, 35 insertions(+), 27 deletions(-)
diff --git a/diff.c b/diff.c
index bc8fa8e..a08a56a 100644
--- a/diff.c
+++ b/diff.c
@@ -3029,6 +3029,38 @@ int parse_long_opt(const char *opt, const char **argv,
return 2;
}
+static int stat_opt(struct diff_options *options, const char **av)
+{
+ const char *arg = av[0];
+ char *end;
+ int width = options->stat_width;
+ int name_width = options->stat_name_width;
+
+ arg += strlen("--stat");
+ end = (char *)arg;
+
+ switch (*arg) {
+ case '-':
+ if (!prefixcmp(arg, "-width="))
+ width = strtoul(arg + 7, &end, 10);
+ else if (!prefixcmp(arg, "-name-width="))
+ name_width = strtoul(arg + 12, &end, 10);
+ break;
+ case '=':
+ width = strtoul(arg+1, &end, 10);
+ if (*end == ',')
+ name_width = strtoul(end+1, &end, 10);
+ }
+
+ /* Important! This checks all the error cases! */
+ if (*end)
+ return 0;
+ options->output_format |= DIFF_FORMAT_DIFFSTAT;
+ options->stat_name_width = name_width;
+ options->stat_width = width;
+ return 1;
+}
+
int diff_opt_parse(struct diff_options *options, const char **av, int ac)
{
const char *arg = av[0];
@@ -3070,33 +3102,9 @@ int diff_opt_parse(struct diff_options *options, const char **av, int ac)
options->output_format |= DIFF_FORMAT_NAME_STATUS;
else if (!strcmp(arg, "-s"))
options->output_format |= DIFF_FORMAT_NO_OUTPUT;
- else if (!prefixcmp(arg, "--stat")) {
- char *end;
- int width = options->stat_width;
- int name_width = options->stat_name_width;
- arg += 6;
- end = (char ...On Wed, Aug 04, 2010 at 03:24:23PM -0700, Junio C Hamano wrote: Thank you for the fix. BTW, it applies to maint with no regressions. -- ldv
