* Re: Cannot clone redirecting stdout
From: Jeff King @ 2009-09-11 16:05 UTC (permalink / raw)
To: Johan Sørensen; +Cc: Aloisio, git, support
In-Reply-To: <9e0f31700909110846h54959ae6u466ceda40799ba37@mail.gmail.com>
On Fri, Sep 11, 2009 at 05:46:23PM +0200, Johan Sørensen wrote:
> Some quick tests seem to indicate it's related to the fact that our
> wonderful little fork+exec git-daemon[1] (which is different from the
> one distributed with git) exec's to "git-upload-pack --strict
> --timeout=30 /path/to/repo". Now, why exactly that'll trigger when the
> no-progress flag is given I'm not sure of. The daemon itself execs as
> soon as it figures out what repo the client requested, so apart from
> the timeout the only thing it reacts to is the header (the
> "${headersize}git-upload-pack project/repo.git\0host=gitorious.org\0"
> part).
>
> We also do redirect stderr to /dev/null for reasons I cannot remember
> (so probably not good ones), that may be related as well. Well run
> some more tests...
Ah. I have a theory. If I do a clone of git://gitorious.org/qt/qt.git,
the counting/compressing stages take a long time (I timed it at 1m40
before it actually sends any data). And looking at upload-pack.c, we
leave the 30-second alarm set while creating the pack. Meaning we die 30
seconds into creating the pack.
When progress is being displayed, however, the progress timer actually
uses SIGALRM, as well. So we are constantly resetting the timer and it
never goes off.
And we should be able to test this theory. How long does it take for the
failure case to fail:
$ time git clone -n git://gitorious.org/qt/qt.git repo >log
fatal: The remote end hung up unexpectedly
fatal: early EOF
fatal: index-pack failed
real 0m31.106s
user 0m0.000s
sys 0m0.012s
Hmm. Suspicious. :)
So that implies to me a few things:
1. You guys should really pack your repos, as you are wasting over a
minute of CPU time every time somebody clones this repo.
2. Upload-pack has what I consider a bug. The --timeout should be
suspended while we are actually crunching numbers to create the
pack. We probably don't want it when sending the pack, either, as
people with slow connections (or big repos) will get timed out
during the send. Probably we just want to apply it to times when we
are waiting to get the list of refs from the client.
3. Upload-pack and the progress code are both using the global alarm
timer and signal, and that is papering over the bug in (2) when
progress is enabled. I'm not sure of the simplest way of having
those interact. But maybe we can just ignore it, because we
probably don't want to using the --timeout alarm during the packing
phase, anyway.
-Peff
^ permalink raw reply
* Re: Cannot clone redirecting stdout
From: Jeff King @ 2009-09-11 16:20 UTC (permalink / raw)
To: Johan Sørensen; +Cc: Aloisio, git, support
In-Reply-To: <20090911160510.GA10848@coredump.intra.peff.net>
On Fri, Sep 11, 2009 at 12:05:10PM -0400, Jeff King wrote:
> Ah. I have a theory. If I do a clone of git://gitorious.org/qt/qt.git,
> the counting/compressing stages take a long time (I timed it at 1m40
> before it actually sends any data). And looking at upload-pack.c, we
> leave the 30-second alarm set while creating the pack. Meaning we die 30
> seconds into creating the pack.
>
> When progress is being displayed, however, the progress timer actually
> uses SIGALRM, as well. So we are constantly resetting the timer and it
> never goes off.
Hmm. Actually, this is not quite right. It looks like we call out to
pack-objects as an external program, so there is no conflict with the
signal. And we do proxy the output of pack-objects, which will keep our
timer resetting every time we see a chunk of output. But pack-objects
produces no output during the deltification phase, unless progress is
turned on. So we still hit our timeout in upload-pack during that
phase.
So our options are:
1. Turn off the timer during deltification, which could mean that it
would potentially go forever. But it's not controlled by the user.
I think the 'timeout' feature is really about the client just
opening the connection and sitting.
2. Keep progress on during deltification, but just throw it away
instead of relaying it if no-progress is in effect.
3. Accept that hitting the timeout during deltification _should_ cause
it to die. In that case, then the case with progress is wrong, and
we should stop resetting the timer just because we got some
progress output from pack-objects. But this may be redefining the
intent of --timeout. I don't know what the original intent was, or
what users of the feature are expecting.
-Peff
^ permalink raw reply
* Re: [RFC/PATCH 2/2] gitweb: check given hash before trying to create snapshot
From: Jakub Narebski @ 2009-09-11 16:42 UTC (permalink / raw)
To: Mark Rada; +Cc: git, Junio C Hamano
In-Reply-To: <9513F576-4154-4281-8545-81841D59B766@mailservices.uwaterloo.ca>
[This mail was very strangely wrapped; I fixed this for readability]
On Fri, 11 Sep 2009, Mark Rada wrote:
> On 2009-09-11, at 3:52 AM, Jakub Narebski wrote:
>
>> Second, I'd rather have better names for snapshots than using full SHA-1.
>> For snapshot of 'v1.5.0' of repository 'repo.git' I'd prefer for snapshot
>> to be named 'repo-v1.5.0', and for snapshot of 'next' branch of the
>> same project to be named for example 'repo-next-20090909', or perhaps
>> 'repo-next-2009-09-10T09:16:18' or 'repo-next-20090909-g5f6b0ff',
>> or 'repo-v1.6.5-rc0-164-g5f6b0ff'.
>
> Ah, yeah, well, I let $hash still hold the originally passed value,
> which would be used to create the outputted file name (unless
> overwritten by the client by using curl -o or something).
Or by specifying different file name than proposed by browser.
> Then $snapshot holds the full hash. This way, if you were to type
> something like
>
> http://git.kernel.org/?p=git/git.git;a=snapshot;h=next;sf=tgz
>
> into your browser window, you would get git.git-next.tar.gz back, but
> the backend could look up something like
>
> git-5f6b0ffff13f5cd762d0a5a4e1c4dede58e8a537.tar.gz
>
> using the $snapshot variable in some hypothetical cache (or even
> without the cache it won't mangle the nicer name $hash might have).
O.K.
>
> Also, right now gitweb will not accept tags for hashes. This seems to be
> because it passes the --verify option to rev-parse, but the output
> from using and not using the verify option seems to be the same (other
> than also accepting all tree-ishes). Could you let me know if there is
> a good reason not to take off the --verify option? Otherwise, I would
> like to take it off in the next version of this patch.
Errr, what?
$ 5096:[gitweb/web@git]# git rev-parse --verify v1.5.0
6db027ffe03210324939b3dd655c4223ca023b45
$ git rev-parse --verify refs/tags/v1.5.0
6db027ffe03210324939b3dd655c4223ca023b45
So it works as intended. The problem must be in some other place.
The '--verify' option is needed because git-rev-parse would otherwise
pass parameters it does not understand 'as is'. Compare
$ git rev-parse --verify v9.9.9 2>/dev/null
$ git rev-parse v9.9.9 2>/dev/null
v9.9.9
>
> Your point about adding the short hash to snapshots of branch heads is
> cool, I'll try that for the next version of the patch.
I think it would be better left for a _separate_ patch, as it is
separate feature (and I guess more complicated one).
>>> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-
>>> gitweb-standalone-http-status.sh
>>> index d0ff21d..4f8f147 100644
>>> --- a/t/t9501-gitweb-standalone-http-status.sh
>>> +++ b/t/t9501-gitweb-standalone-http-status.sh
>>> @@ -75,4 +75,30 @@ test_expect_success \
>>> test_debug 'cat gitweb.output'
>>>
>>>
>>> +test_expect_success \
>>> + 'snapshots: bad treeish id' \
>>> + 'gitweb_run "p=.git;a=snapshot;h=frizzumFrazzum;sf=tgz" &&
>>> + grep "400 - Not a valid hash id:" gitweb.output'
>>> +test_debug 'cat gitweb.output'
>>> +
>>> +test_expect_success \
>>> + 'snapshots: good treeish id' \
>>> + 'gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
>>> + grep "Status: 200 OK" gitweb.output'
>>> +test_debug 'cat gitweb.output'
>>
>> Why you don't check for "Status: 400" too?
>
> I'm not sure which test you are referring to (I think the second). The
> second test is valid and should return a nice .git-master.tar.gz
> tarball.
The output of CGI script like gitweb (and therefore gitweb.output file,
as it is generated now) contains both HTTP headers, separated by single
empty CRLF delimited line from the proper output of a script.
In first test you check that _contents_ contain specific error message,
but you do not check if HTTP status code matches it (it should, because
of how die_error works). In second test you check HTTP status. If the
t/t9501-gitweb-standalone-http-status.sh is to be about status, I guess
that you should check HTTP status, and not contents of the page (which
is more likely to change, e.g. due to some prettifying).
In t9501 tests you need, I think, only the HTTP headers part, unless
you want also to check that the contents matches. There was some sed
script shown to extract only HTTP headers.
>>> Second, any given treeish will always be translated to the full length,
>>> unambiguous, hash id; this will be useful for things like creating
>>> unique names for snapshot caches.
>>
>> But this is not a good idea, IMHO.
>>
>> First, it introduces feature that nobody uses (at least yet); we can
>> introduce this feature when it is needed instead.
>
> Sorry for promoting vapourware, I did originally rip this patch out from
> something else. I removed the comment from the v2 commit message.
Ah. O.K. then.
--
Jakub Narebski
Poland
^ permalink raw reply
* Re: bash_completion outside repo
From: Jeff King @ 2009-09-11 16:47 UTC (permalink / raw)
To: Shawn O. Pearce; +Cc: james bardin, Todd Zullinger, git
In-Reply-To: <20090911150934.GB977@coredump.intra.peff.net>
On Fri, Sep 11, 2009 at 11:09:34AM -0400, Jeff King wrote:
> Assuming you make such a patch, that will clear up the original issue. I
> wonder if we should fix "git config --list". The current semantics seem
> a little crazy to me, but it is a scriptable interface. I'm inclined to
> call this a bug, though.
And here is a patch to fix it.
-- >8 --
Subject: [PATCH] config: treat non-existent config files as empty
The git_config() function signals error by returning -1 in
two instances:
1. An actual error occurs in opening a config file (parse
errors cause an immediate die).
2. Of the three possible config files, none was found.
However, this second case is often not an error at all; it
simply means that the user has no configuration (they are
outside a repo, and they have no ~/.gitconfig file). This
can lead to confusing errors, such as when the bash
completion calls "git config --list" outside of a repo. If
the user has a ~/.gitconfig, the command completes
succesfully; if they do not, it complains to stderr.
This patch allows callers of git_config to distinguish
between the two cases. Error is signaled by -1, and
otherwise the return value is the number of files parsed.
This means that the traditional "git_config(...) < 0" check
for error should work, but callers who want to know whether
we parsed any files or not can still do so.
We need to tweak one use of git_config in builtin-remote
that previously assumed the return value was either '0' or
'-1'.
Signed-off-by: Jeff King <peff@peff.net>
---
This is actually a bit overengineered. Of the hundreds of calls to
git_config, there are exactly _two_ which check the return value. And
neither of them cares whether we parsed files or not; they really only
care if there was an error. So we could simply return 0 as long as there
is no error.
This also makes me wonder, though. Git can do wildly different things
(including hard-to-reverse things) based on config (e.g., just consider
gc.pruneExpire). Yet we call git_config() without ever checking for
errors. In the actual parsing routines, we die() if there is an error.
But if we fail to open the file due to some transient error, we will
silently ignore the situation.
Granted, such transient errors are unlikely. The biggest reasons for
failing to open a file are that it doesn't exist, or that we have no
permission to read it, both of which are treated explicitly in
git_config as "silently ok". But I wonder if we should simply be dying
on such an error, and git_config() should just have a void return.
builtin-remote.c | 3 ++-
config.c | 4 +---
t/t1300-repo-config.sh | 8 ++++++++
3 files changed, 11 insertions(+), 4 deletions(-)
diff --git a/builtin-remote.c b/builtin-remote.c
index 0777dd7..3bf1fe8 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -1245,7 +1245,8 @@ static int update(int argc, const char **argv)
for (i = 1; i < argc; i++) {
int groups_found = 0;
remote_group.name = argv[i];
- result = git_config(get_remote_group, &groups_found);
+ if (git_config(get_remote_group, &groups_found) < 0)
+ result = -1;
if (!groups_found && (i != 1 || strcmp(argv[1], "default"))) {
struct remote *remote;
if (!remote_is_configured(argv[i]))
diff --git a/config.c b/config.c
index e87edea..e429674 100644
--- a/config.c
+++ b/config.c
@@ -709,9 +709,7 @@ int git_config(config_fn_t fn, void *data)
found += 1;
}
free(repo_config);
- if (found == 0)
- return -1;
- return ret;
+ return ret == 0 ? found : ret;
}
/*
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 83b7294..db987b7 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -289,6 +289,14 @@ test_expect_success 'working --list' \
'git config --list > output && cmp output expect'
cat > expect << EOF
+EOF
+
+test_expect_success '--list without repo produces empty output' '
+ git --git-dir=nonexistent config --list >output &&
+ test_cmp expect output
+'
+
+cat > expect << EOF
beta.noindent sillyValue
nextsection.nonewline wow2 for me
EOF
--
1.6.5.rc0.174.g29a6d.dirty
^ permalink raw reply related
* one half of a rebase
From: Geoffrey Irving @ 2009-09-11 17:25 UTC (permalink / raw)
To: git; +Cc: Dylan Simon
If I'm on branch topic and do "git rebase master", git performs two operations:
1. Rewind the current head to master.
2. For each commit in topic that isn't in master, cherry pick it onto
the current head.
I would like to be able to do (2) as a separate operation. For
example, if I start out on branch master and want to get all of
topic's commits on top of the current head, I currently do
git checkout topic
git rebase master
git branch -d master
git branch -m master
If I could do (2) as a separate operation, it would look something like
git cherry-pick-all topic
which is simpler and faster since it avoids switching files back and
forth (master to topic and back). Is there a robust way to achieve
the cherry-pick-all semantics with current commands? If not, how
difficult would it be to partition rebase accordingly?
Thanks,
Geoffrey
^ permalink raw reply
* [PATCH 1/2] start_command: do not clobber cmd->env on Windows code path
From: Johannes Sixt @ 2009-09-11 17:40 UTC (permalink / raw)
To: Junio C Hamano; +Cc: msysgit, Alexey Borzenkov, git
In-Reply-To: <1252560077-1725-1-git-send-email-snaury@gmail.com>
Previously, it would not be possible to call start_command twice for the
same struct child_process that has env set.
The fix is achieved by moving the loop that modifies the environment block
into a helper function. This also allows us to make two other helper
functions static.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
We don't call start_command twice in a row anywhere in git.git, but
msysgit has a patch that does, and with the next patch the buglet would
be triggered.
Even after this patch, other members of *cmd are clobbered, so it could
be argued that this patch is unnecessary, but at least the Windows code
path now keeps the same members that the Unix code path keeps, and it
makes start_command easier to read by moving a loop into a helper.
-- Hannes
compat/mingw.c | 16 ++++++++++++++--
compat/mingw.h | 3 +--
run-command.c | 7 ++-----
3 files changed, 17 insertions(+), 9 deletions(-)
diff --git a/compat/mingw.c b/compat/mingw.c
index bed4178..36ef8d3 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -824,7 +824,7 @@ void mingw_execvp(const char *cmd, char *const *argv)
free_path_split(path);
}
-char **copy_environ()
+static char **copy_environ(void)
{
char **env;
int i = 0;
@@ -861,7 +861,7 @@ static int lookup_env(char **env, const char *name, size_t nmln)
/*
* If name contains '=', then sets the variable, otherwise it unsets it
*/
-char **env_setenv(char **env, const char *name)
+static char **env_setenv(char **env, const char *name)
{
char *eq = strchrnul(name, '=');
int i = lookup_env(env, name, eq-name);
@@ -886,6 +886,18 @@ char **env_setenv(char **env, const char *name)
return env;
}
+/*
+ * Copies global environ and adjusts variables as specified by vars.
+ */
+char **make_augmented_environ(const char *const *vars)
+{
+ char **env = copy_environ();
+
+ while (*vars)
+ env = env_setenv(env, *vars++);
+ return env;
+}
+
/* this is the first function to call into WS_32; initialize it */
#undef gethostbyname
struct hostent *mingw_gethostbyname(const char *host)
diff --git a/compat/mingw.h b/compat/mingw.h
index 948de66..c43917c 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -222,9 +222,8 @@ void mingw_open_html(const char *path);
* helpers
*/
-char **copy_environ(void);
+char **make_augmented_environ(const char *const *vars);
void free_environ(char **env);
-char **env_setenv(char **env, const char *name);
/*
* A replacement of main() that ensures that argv[0] has a path
diff --git a/run-command.c b/run-command.c
index f3e7abb..ac314a5 100644
--- a/run-command.c
+++ b/run-command.c
@@ -173,11 +173,8 @@ fail_pipe:
if (cmd->dir)
die("chdir in start_command() not implemented");
- if (cmd->env) {
- env = copy_environ();
- for (; *cmd->env; cmd->env++)
- env = env_setenv(env, *cmd->env);
- }
+ if (cmd->env)
+ env = make_augmented_environ(cmd->env);
if (cmd->git_cmd) {
cmd->argv = prepare_git_cmd(cmd->argv);
--
1.6.5.rc0.28.gfb9b
^ permalink raw reply related
* [PATCH 2/2] pager: set LESS=FRSX also on Windows
From: Johannes Sixt @ 2009-09-11 17:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: msysgit, Alexey Borzenkov, git
In-Reply-To: <200909111940.08652.j6t@kdbg.org>
Previously, this environment variable was set in the pager_preexec
callback, which is conditionally-compiled only on Unix, because it is not,
and cannot be, called on Windows.
With this patch the env member of struct child_process is used to set
the environment variable, which also works on Windows.
Noticed by Alexey Borzenkov.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Alexey,
this will result in a conflict when you apply it to your msysgit because
there is an extra patch that shuffled the setup of pager_process around.
-- Hannes
| 6 ++++--
1 files changed, 4 insertions(+), 2 deletions(-)
--git a/pager.c b/pager.c
index 4921843..f416d38 100644
--- a/pager.c
+++ b/pager.c
@@ -21,8 +21,6 @@ static void pager_preexec(void)
FD_ZERO(&in);
FD_SET(0, &in);
select(1, &in, NULL, &in, NULL);
-
- setenv("LESS", "FRSX", 0);
}
#endif
@@ -70,6 +68,10 @@ void setup_pager(void)
pager_argv[2] = pager;
pager_process.argv = pager_argv;
pager_process.in = -1;
+ if (!getenv("LESS")) {
+ static const char *env[] = { "LESS=FRSX", NULL };
+ pager_process.env = env;
+ }
#ifndef __MINGW32__
pager_process.preexec_cb = pager_preexec;
#endif
--
1.6.5.rc0.28.gfb9b
^ permalink raw reply related
* Re: one half of a rebase
From: Matthieu Moy @ 2009-09-11 18:39 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: git, Dylan Simon
In-Reply-To: <7f9d599f0909111025q42e3cdc6vba602b84c1d81215@mail.gmail.com>
Geoffrey Irving <irving@naml.us> writes:
> If I could do (2) as a separate operation, it would look something like
>
> git cherry-pick-all topic
I believe
git rebase --onto master master topic
git update-ref master topic
would do the trick.
--
Matthieu Moy
http://www-verimag.imag.fr/~moy/
^ permalink raw reply
* Re: [PATCH v2] Re: add documentation for mailinfo.scissors and '--no-scissors'
From: Junio C Hamano @ 2009-09-11 18:53 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Nanako Shiraishi, git
In-Reply-To: <20090911134112.GA18684@vidovic>
Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> I don't understand your point here. This hunk doesn't allow
> --no-no-scissors but add --no-scissors to usage of 'git am', no?
>
> Do I miss something around $OPTIONS_SPEC?
You missed two thirds of the issue and I missed the other one third ;-)
As scissors is not marked with ! (see PARSEOPT section in the rev-parse
documentation), no-scissors is already available without that hunk, and
that is why your patch is unnecessary.
Your patch did not mark no-scissors with ! either, so it makes the call to
rev-parse --parseopt to process your OPTIONS_SPEC pass --no-no-scissors.
However, you did not add --no-no-scissors arm to the case statement that
processes rev-parse --parseopt output, so the command as the whole still
rejects --no-no-scissors as invalid (this is the last third I missed).
^ permalink raw reply
* [PATCH v3 2/2] add documentation for mailinfo.scissors and '--no-scissors'
From: Nicolas Sebrecht @ 2009-09-11 19:50 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nanako Shiraishi, git, Nicolas Sebrecht
In-Reply-To: <682ef47420f36d8c53e42981370d377b621d7b86.1252698215.git.nicolas.s.dev@gmx.fr>
The 11/09/09, Junio C Hamano wrote:
> Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
>
> > +---no-scissors::
> > + Do not obey to a scissors line (see linkgit:git-mailinfo[1]).
> > +
>
> obey is v.t. so "do not obey a scissors line" would be grammatical; I
> think "ignore scissors lines" would be better.
>
> > +--no-scissors::
> > + Do not obey to a scissors line. This is only useful if mailinfo.scissors is
> > + enabled (see --scissors).
>
> Ditto; also it is useful in general if you do not know which way it is
> configured. Saying "_only_" is misleading.
>
> Ignore scissors lines; useful for overriding mailinfo.scissors
> settings.
>
> > diff --git a/git-am.sh b/git-am.sh
> > index 26ffe70..f242d1a 100755
> > --- a/git-am.sh
> > +++ b/git-am.sh
> > @@ -16,6 +16,7 @@ s,signoff add a Signed-off-by line to the commit message
> > u,utf8 recode into utf8 (default)
> > k,keep pass -k flag to git-mailinfo
> > c,scissors strip everything before a scissors line
> > +no-scissors don't obey to a scissors line (default)
> > whitespace= pass it through git-apply
> > ignore-space-change pass it through git-apply
> > ignore-whitespace pass it through git-apply
>
> Do we want it to allow --no-no-scissors? I do not think this hunk is
> necessary at all.
This version includes the above comments.
Thanks,
-- >8 --
Signed-off-by: Nicolas Sebrecht <nicolas.s.dev@gmx.fr>
---
Documentation/git-am.txt | 5 ++++-
Documentation/git-mailinfo.txt | 5 +++++
builtin-mailinfo.c | 2 +-
3 files changed, 10 insertions(+), 2 deletions(-)
diff --git a/Documentation/git-am.txt b/Documentation/git-am.txt
index 87781f4..06e6ea6 100644
--- a/Documentation/git-am.txt
+++ b/Documentation/git-am.txt
@@ -13,7 +13,7 @@ SYNOPSIS
[--3way] [--interactive] [--committer-date-is-author-date]
[--ignore-date] [--ignore-space-change | --ignore-whitespace]
[--whitespace=<option>] [-C<n>] [-p<n>] [--directory=<dir>]
- [--reject] [-q | --quiet] [--scissors]
+ [--reject] [-q | --quiet] [--scissors | --no-scissors]
[<mbox> | <Maildir>...]
'git am' (--skip | --resolved | --abort)
@@ -44,6 +44,9 @@ OPTIONS
Remove everything in body before a scissors line (see
linkgit:git-mailinfo[1]).
+---no-scissors::
+ ignore scissors lines (see linkgit:git-mailinfo[1]).
+
-q::
--quiet::
Be quiet. Only print error messages.
diff --git a/Documentation/git-mailinfo.txt b/Documentation/git-mailinfo.txt
index 823ab82..d1f9cb8 100644
--- a/Documentation/git-mailinfo.txt
+++ b/Documentation/git-mailinfo.txt
@@ -62,6 +62,11 @@ This is useful if you want to begin your message in a discussion thread
with comments and suggestions on the message you are responding to, and to
conclude it with a patch submission, separating the discussion and the
beginning of the proposed commit log message with a scissors line.
++
+This can enabled by default with the configuration option mailinfo.scissors.
+
+--no-scissors::
+ ignore scissors lines; useful for overriding mailinfo.scissors settings.
<msg>::
The commit log message extracted from e-mail, usually
diff --git a/builtin-mailinfo.c b/builtin-mailinfo.c
index 7d22fd7..d498b1c 100644
--- a/builtin-mailinfo.c
+++ b/builtin-mailinfo.c
@@ -1004,7 +1004,7 @@ static int git_mailinfo_config(const char *var, const char *value, void *unused)
}
static const char mailinfo_usage[] =
- "git mailinfo [-k] [-u | --encoding=<encoding> | -n] [--scissors] msg patch < mail >info";
+ "git mailinfo [-k] [-u | --encoding=<encoding> | -n] [--scissors | --no-scissors] msg patch < mail >info";
int cmd_mailinfo(int argc, const char **argv, const char *prefix)
{
--
1.6.5.rc0.166.g46a82
^ permalink raw reply related
* [JGIT PATCH 1/3] Disallow creating invalid DirCacheEntry records
From: Shawn O. Pearce @ 2009-09-11 19:58 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Shawn O. Pearce, Adam W. Hawks
A dircache record must not use a path string like "/a" or "a//b"
as this results in a tree entry being written with a zero length
name component in the record. C git does not support an empty name,
and neither does any modern filesystem.
A record also must not have a stage outside of the standard 0-3
value range, as there are only 2 bits of space available in the
on-disk format of the record to store the stage information.
Any other values would be truncated into this space, storing a
different value than the caller expected.
If an application tries to create a DirCache record with either of
these wrong values, we abort with an IllegalArgumentException.
Signed-off-by: Shawn O. Pearce <sop@google.com>
CC: Adam W. Hawks <awhawks@writeme.com>
---
.../spearce/jgit/dircache/DirCacheEntryTest.java | 115 ++++++++++++++++++++
.../org/spearce/jgit/dircache/DirCacheEntry.java | 55 +++++++++-
2 files changed, 169 insertions(+), 1 deletions(-)
create mode 100644 org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
new file mode 100644
index 0000000..a6ff5a8
--- /dev/null
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
@@ -0,0 +1,115 @@
+/*
+ * Copyright (C) 2009, Google Inc.
+ *
+ * All rights reserved.
+ *
+ * Redistribution and use in source and binary forms, with or
+ * without modification, are permitted provided that the following
+ * conditions are met:
+ *
+ * - Redistributions of source code must retain the above copyright
+ * notice, this list of conditions and the following disclaimer.
+ *
+ * - Redistributions in binary form must reproduce the above
+ * copyright notice, this list of conditions and the following
+ * disclaimer in the documentation and/or other materials provided
+ * with the distribution.
+ *
+ * - Neither the name of the Git Development Community nor the
+ * names of its contributors may be used to endorse or promote
+ * products derived from this software without specific prior
+ * written permission.
+ *
+ * THIS SOFTWARE IS PROVIDED BY THE COPYRIGHT HOLDERS AND
+ * CONTRIBUTORS "AS IS" AND ANY EXPRESS OR IMPLIED WARRANTIES,
+ * INCLUDING, BUT NOT LIMITED TO, THE IMPLIED WARRANTIES
+ * OF MERCHANTABILITY AND FITNESS FOR A PARTICULAR PURPOSE
+ * ARE DISCLAIMED. IN NO EVENT SHALL THE COPYRIGHT OWNER OR
+ * CONTRIBUTORS BE LIABLE FOR ANY DIRECT, INDIRECT, INCIDENTAL,
+ * SPECIAL, EXEMPLARY, OR CONSEQUENTIAL DAMAGES (INCLUDING, BUT
+ * NOT LIMITED TO, PROCUREMENT OF SUBSTITUTE GOODS OR SERVICES;
+ * LOSS OF USE, DATA, OR PROFITS; OR BUSINESS INTERRUPTION) HOWEVER
+ * CAUSED AND ON ANY THEORY OF LIABILITY, WHETHER IN CONTRACT,
+ * STRICT LIABILITY, OR TORT (INCLUDING NEGLIGENCE OR OTHERWISE)
+ * ARISING IN ANY WAY OUT OF THE USE OF THIS SOFTWARE, EVEN IF
+ * ADVISED OF THE POSSIBILITY OF SUCH DAMAGE.
+ */
+
+package org.spearce.jgit.dircache;
+
+import junit.framework.TestCase;
+
+import org.spearce.jgit.lib.Constants;
+
+public class DirCacheEntryTest extends TestCase {
+ public void testIsValidPath() {
+ assertTrue(isValidPath("a"));
+ assertTrue(isValidPath("a/b"));
+ assertTrue(isValidPath("ab/cd/ef"));
+
+ assertFalse(isValidPath(""));
+ assertFalse(isValidPath("/a"));
+ assertFalse(isValidPath("a//b"));
+ assertFalse(isValidPath("ab/cd//ef"));
+ assertFalse(isValidPath("a/"));
+ assertFalse(isValidPath("ab/cd/ef/"));
+ assertFalse(isValidPath("a\u0000b"));
+ }
+
+ private static boolean isValidPath(final String path) {
+ return DirCacheEntry.isValidPath(Constants.encode(path));
+ }
+
+ public void testCreate_ByStringPath() {
+ assertEquals("a", new DirCacheEntry("a").getPathString());
+ assertEquals("a/b", new DirCacheEntry("a/b").getPathString());
+
+ try {
+ new DirCacheEntry("/a");
+ fail("Incorrectly created DirCacheEntry");
+ } catch (IllegalArgumentException err) {
+ assertEquals("Invalid path: /a", err.getMessage());
+ }
+ }
+
+ public void testCreate_ByStringPathAndStage() {
+ DirCacheEntry e;
+
+ e = new DirCacheEntry("a", 0);
+ assertEquals("a", e.getPathString());
+ assertEquals(0, e.getStage());
+
+ e = new DirCacheEntry("a/b", 1);
+ assertEquals("a/b", e.getPathString());
+ assertEquals(1, e.getStage());
+
+ e = new DirCacheEntry("a/c", 2);
+ assertEquals("a/c", e.getPathString());
+ assertEquals(2, e.getStage());
+
+ e = new DirCacheEntry("a/d", 3);
+ assertEquals("a/d", e.getPathString());
+ assertEquals(3, e.getStage());
+
+ try {
+ new DirCacheEntry("/a", 1);
+ fail("Incorrectly created DirCacheEntry");
+ } catch (IllegalArgumentException err) {
+ assertEquals("Invalid path: /a", err.getMessage());
+ }
+
+ try {
+ new DirCacheEntry("a", -11);
+ fail("Incorrectly created DirCacheEntry");
+ } catch (IllegalArgumentException err) {
+ assertEquals("Invalid stage -11 for path a", err.getMessage());
+ }
+
+ try {
+ new DirCacheEntry("a", 4);
+ fail("Incorrectly created DirCacheEntry");
+ } catch (IllegalArgumentException err) {
+ assertEquals("Invalid stage 4 for path a", err.getMessage());
+ }
+ }
+}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
index 47b1cc5..d7abd6e 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
@@ -166,6 +166,10 @@
*
* @param newPath
* name of the cache entry.
+ * @throws IllegalArgumentException
+ * If the path starts or ends with "/", or contains "//" either
+ * "\0". These sequences are not permitted in a git tree object
+ * or DirCache file.
*/
public DirCacheEntry(final String newPath) {
this(Constants.encode(newPath));
@@ -178,6 +182,11 @@ public DirCacheEntry(final String newPath) {
* name of the cache entry.
* @param stage
* the stage index of the new entry.
+ * @throws IllegalArgumentException
+ * If the path starts or ends with "/", or contains "//" either
+ * "\0". These sequences are not permitted in a git tree object
+ * or DirCache file. Or if {@code stage} is outside of the
+ * range 0..3, inclusive.
*/
public DirCacheEntry(final String newPath, final int stage) {
this(Constants.encode(newPath), stage);
@@ -188,6 +197,10 @@ public DirCacheEntry(final String newPath, final int stage) {
*
* @param newPath
* name of the cache entry, in the standard encoding.
+ * @throws IllegalArgumentException
+ * If the path starts or ends with "/", or contains "//" either
+ * "\0". These sequences are not permitted in a git tree object
+ * or DirCache file.
*/
public DirCacheEntry(final byte[] newPath) {
this(newPath, STAGE_0);
@@ -200,8 +213,20 @@ public DirCacheEntry(final byte[] newPath) {
* name of the cache entry, in the standard encoding.
* @param stage
* the stage index of the new entry.
+ * @throws IllegalArgumentException
+ * If the path starts or ends with "/", or contains "//" either
+ * "\0". These sequences are not permitted in a git tree object
+ * or DirCache file. Or if {@code stage} is outside of the
+ * range 0..3, inclusive.
*/
public DirCacheEntry(final byte[] newPath, final int stage) {
+ if (!isValidPath(newPath))
+ throw new IllegalArgumentException("Invalid path: "
+ + toString(newPath));
+ if (stage < 0 || 3 < stage)
+ throw new IllegalArgumentException("Invalid stage " + stage
+ + " for path " + toString(newPath));
+
info = new byte[INFO_LEN];
infoOffset = 0;
path = newPath;
@@ -461,7 +486,7 @@ public void setObjectIdFromRaw(final byte[] bs, final int p) {
* returned string.
*/
public String getPathString() {
- return Constants.CHARSET.decode(ByteBuffer.wrap(path)).toString();
+ return toString(path);
}
/**
@@ -492,4 +517,32 @@ private void encodeTS(final int pIdx, final long when) {
NB.encodeInt32(info, base, (int) (when / 1000));
NB.encodeInt32(info, base + 4, ((int) (when % 1000)) * 1000000);
}
+
+ private static String toString(final byte[] path) {
+ return Constants.CHARSET.decode(ByteBuffer.wrap(path)).toString();
+ }
+
+ static boolean isValidPath(final byte[] path) {
+ if (path.length == 0)
+ return false; // empty path is not permitted.
+
+ boolean componentHasChars = false;
+ for (final byte c : path) {
+ switch (c) {
+ case 0:
+ return false; // NUL is never allowed within the path.
+
+ case '/':
+ if (componentHasChars)
+ componentHasChars = false;
+ else
+ return false;
+ break;
+
+ default:
+ componentHasChars = true;
+ }
+ }
+ return componentHasChars;
+ }
}
--
1.6.5.rc0.164.g5f6b0
^ permalink raw reply related
* [JGIT PATCH 2/3] Don't allow DirCacheEntry with mode of 0
From: Shawn O. Pearce @ 2009-09-11 19:58 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Shawn O. Pearce, Adam W. Hawks
In-Reply-To: <1252699129-6961-1-git-send-email-spearce@spearce.org>
A 0 file mode in a DirCacheEntry is not a valid mode. To C git
such a value indicates the record should not be present. We already
were catching this bad state and exceptioning out when writing tree
objects to disk, but we did not fail when writing the dircache back
to disk. This allowed JGit applications to create a dircache file
which C git would not like to read.
Instead of checking the mode during writes, we now check during
mutation. This allows application bugs to be detected sooner and
closer to the cause site. It also allows us to avoid checking most
of the records which we read in from disk, as we can assume these
are formatted correctly.
Some of our unit tests were not setting the FileMode on their test
entry, so they had to be updated to use REGULAR_FILE.
Signed-off-by: Shawn O. Pearce <sop@google.com>
CC: Adam W. Hawks <awhawks@writeme.com>
---
.../spearce/jgit/dircache/DirCacheBasicTest.java | 5 ++-
.../spearce/jgit/dircache/DirCacheBuilderTest.java | 27 +++++++++++++--
.../spearce/jgit/dircache/DirCacheEntryTest.java | 37 ++++++++++++++++++++
.../spearce/jgit/dircache/DirCacheFindTest.java | 5 ++-
.../jgit/dircache/DirCacheIteratorTest.java | 4 ++-
.../jgit/dircache/DirCacheLargePathTest.java | 5 +++
.../spearce/jgit/dircache/DirCacheTreeTest.java | 13 +++++--
.../org/spearce/jgit/dircache/DirCacheBuilder.java | 8 +++--
.../org/spearce/jgit/dircache/DirCacheEditor.java | 11 ++++--
.../org/spearce/jgit/dircache/DirCacheEntry.java | 12 ++++++-
.../org/spearce/jgit/dircache/DirCacheTree.java | 4 --
11 files changed, 111 insertions(+), 20 deletions(-)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
index 4d737c0..3b48b11 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBasicTest.java
@@ -40,6 +40,7 @@
import java.io.File;
import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.FileMode;
import org.spearce.jgit.lib.RepositoryTestCase;
public class DirCacheBasicTest extends RepositoryTestCase {
@@ -170,8 +171,10 @@ public void testBuildThenClear() throws Exception {
final String[] paths = { "a.", "a.b", "a/b", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final DirCacheBuilder b = dc.builder();
for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java
index 2cf1d92..f64e4f6 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheBuilderTest.java
@@ -59,6 +59,20 @@ public void testBuildEmpty() throws Exception {
}
}
+ public void testBuildRejectsUnsetFileMode() throws Exception {
+ final DirCache dc = DirCache.newInCore();
+ final DirCacheBuilder b = dc.builder();
+ assertNotNull(b);
+
+ final DirCacheEntry e = new DirCacheEntry("a");
+ assertEquals(0, e.getRawMode());
+ try {
+ b.add(e);
+ } catch (IllegalArgumentException err) {
+ assertEquals("FileMode not set for path a", err.getMessage());
+ }
+ }
+
public void testBuildOneFile_FinishWriteCommit() throws Exception {
final String path = "a-file-path";
final FileMode mode = FileMode.REGULAR_FILE;
@@ -162,6 +176,7 @@ public void testFindSingleFile() throws Exception {
assertNotNull(b);
final DirCacheEntry entOrig = new DirCacheEntry(path);
+ entOrig.setFileMode(FileMode.REGULAR_FILE);
assertNotSame(path, entOrig.getPathString());
assertEquals(path, entOrig.getPathString());
b.add(entOrig);
@@ -185,8 +200,10 @@ public void testAdd_InGitSortOrder() throws Exception {
final String[] paths = { "a.", "a.b", "a/b", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final DirCacheBuilder b = dc.builder();
for (int i = 0; i < ents.length; i++)
@@ -207,8 +224,10 @@ public void testAdd_ReverseGitSortOrder() throws Exception {
final String[] paths = { "a.", "a.b", "a/b", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final DirCacheBuilder b = dc.builder();
for (int i = ents.length - 1; i >= 0; i--)
@@ -229,8 +248,10 @@ public void testBuilderClear() throws Exception {
final String[] paths = { "a.", "a.b", "a/b", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
{
final DirCacheBuilder b = dc.builder();
for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
index a6ff5a8..971f201 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheEntryTest.java
@@ -40,6 +40,7 @@
import junit.framework.TestCase;
import org.spearce.jgit.lib.Constants;
+import org.spearce.jgit.lib.FileMode;
public class DirCacheEntryTest extends TestCase {
public void testIsValidPath() {
@@ -112,4 +113,40 @@ public void testCreate_ByStringPathAndStage() {
assertEquals("Invalid stage 4 for path a", err.getMessage());
}
}
+
+ public void testSetFileMode() {
+ final DirCacheEntry e = new DirCacheEntry("a");
+
+ assertEquals(0, e.getRawMode());
+
+ e.setFileMode(FileMode.REGULAR_FILE);
+ assertSame(FileMode.REGULAR_FILE, e.getFileMode());
+ assertEquals(FileMode.REGULAR_FILE.getBits(), e.getRawMode());
+
+ e.setFileMode(FileMode.EXECUTABLE_FILE);
+ assertSame(FileMode.EXECUTABLE_FILE, e.getFileMode());
+ assertEquals(FileMode.EXECUTABLE_FILE.getBits(), e.getRawMode());
+
+ e.setFileMode(FileMode.SYMLINK);
+ assertSame(FileMode.SYMLINK, e.getFileMode());
+ assertEquals(FileMode.SYMLINK.getBits(), e.getRawMode());
+
+ e.setFileMode(FileMode.GITLINK);
+ assertSame(FileMode.GITLINK, e.getFileMode());
+ assertEquals(FileMode.GITLINK.getBits(), e.getRawMode());
+
+ try {
+ e.setFileMode(FileMode.MISSING);
+ fail("incorrectly accepted FileMode.MISSING");
+ } catch (IllegalArgumentException err) {
+ assertEquals("Invalid mode 0 for path a", err.getMessage());
+ }
+
+ try {
+ e.setFileMode(FileMode.TREE);
+ fail("incorrectly accepted FileMode.TREE");
+ } catch (IllegalArgumentException err) {
+ assertEquals("Invalid mode 40000 for path a", err.getMessage());
+ }
+ }
}
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java
index 0eb0302..470c80a 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheFindTest.java
@@ -37,6 +37,7 @@
package org.spearce.jgit.dircache;
+import org.spearce.jgit.lib.FileMode;
import org.spearce.jgit.lib.RepositoryTestCase;
public class DirCacheFindTest extends RepositoryTestCase {
@@ -45,8 +46,10 @@ public void testEntriesWithin() throws Exception {
final String[] paths = { "a.", "a/b", "a/c", "a/d", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final int aFirst = 1;
final int aLast = 3;
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java
index 047c989..71581dc 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheIteratorTest.java
@@ -68,8 +68,10 @@ public void testNoSubtree_NoTreeWalk() throws Exception {
final String[] paths = { "a.", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final DirCacheBuilder b = dc.builder();
for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java
index 4ea286c..a9945f1 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheLargePathTest.java
@@ -40,6 +40,7 @@
import java.io.IOException;
import org.spearce.jgit.errors.CorruptObjectException;
+import org.spearce.jgit.lib.FileMode;
import org.spearce.jgit.lib.RepositoryTestCase;
public class DirCacheLargePathTest extends RepositoryTestCase {
@@ -70,6 +71,10 @@ private void testLongPath(final int len) throws CorruptObjectException,
final DirCacheEntry longEnt = new DirCacheEntry(longPath);
final DirCacheEntry shortEnt = new DirCacheEntry(shortPath);
+
+ longEnt.setFileMode(FileMode.REGULAR_FILE);
+ shortEnt.setFileMode(FileMode.REGULAR_FILE);
+
assertEquals(longPath, longEnt.getPathString());
assertEquals(shortPath, shortEnt.getPathString());
diff --git a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java
index aca0b90..6efa207 100644
--- a/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java
+++ b/org.spearce.jgit.test/tst/org/spearce/jgit/dircache/DirCacheTreeTest.java
@@ -40,6 +40,7 @@
import java.io.IOException;
import org.spearce.jgit.errors.CorruptObjectException;
+import org.spearce.jgit.lib.FileMode;
import org.spearce.jgit.lib.RepositoryTestCase;
public class DirCacheTreeTest extends RepositoryTestCase {
@@ -75,8 +76,10 @@ public void testSingleSubtree() throws Exception {
final String[] paths = { "a.", "a/b", "a/c", "a/d", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final int aFirst = 1;
final int aLast = 3;
@@ -110,8 +113,10 @@ public void testTwoLevelSubtree() throws Exception {
final String[] paths = { "a.", "a/b", "a/c/e", "a/c/f", "a/d", "a0b" };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final int aFirst = 1;
final int aLast = 4;
final int acFirst = 2;
@@ -167,8 +172,10 @@ public void testWriteReadTree() throws CorruptObjectException, IOException {
final String B = String.format("b%2000s", "b");
final String[] paths = { A + ".", A + "." + B, A + "/" + B, A + "0" + B };
final DirCacheEntry[] ents = new DirCacheEntry[paths.length];
- for (int i = 0; i < paths.length; i++)
+ for (int i = 0; i < paths.length; i++) {
ents[i] = new DirCacheEntry(paths[i]);
+ ents[i].setFileMode(FileMode.REGULAR_FILE);
+ }
final DirCacheBuilder b = dc.builder();
for (int i = 0; i < ents.length; i++)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java
index aee12fb..8acb3d0 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuilder.java
@@ -41,7 +41,6 @@
import java.util.Arrays;
import org.spearce.jgit.lib.AnyObjectId;
-import org.spearce.jgit.lib.FileMode;
import org.spearce.jgit.lib.Repository;
import org.spearce.jgit.lib.WindowCursor;
import org.spearce.jgit.treewalk.AbstractTreeIterator;
@@ -90,8 +89,13 @@ protected DirCacheBuilder(final DirCache dc, final int ecnt) {
*
* @param newEntry
* the new entry to add.
+ * @throws IllegalArgumentException
+ * If the FileMode of the entry was not set by the caller.
*/
public void add(final DirCacheEntry newEntry) {
+ if (newEntry.getRawMode() == 0)
+ throw new IllegalArgumentException("FileMode not set for path "
+ + newEntry.getPathString());
beforeAdd(newEntry);
fastAdd(newEntry);
}
@@ -187,8 +191,6 @@ public void finish() {
}
private void beforeAdd(final DirCacheEntry newEntry) {
- if (FileMode.TREE.equals(newEntry.getRawMode()))
- throw bad(newEntry, "Adding subtree not allowed");
if (sorted && entryCnt > 0) {
final DirCacheEntry lastEntry = entries[entryCnt - 1];
final int cr = DirCache.cmp(lastEntry, newEntry);
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java
index 10b554e..6eaceb7 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEditor.java
@@ -138,11 +138,16 @@ private void applyEdits() {
}
final DirCacheEntry ent;
- if (missing)
+ if (missing) {
ent = new DirCacheEntry(e.path);
- else
+ e.apply(ent);
+ if (ent.getRawMode() == 0)
+ throw new IllegalArgumentException("FileMode not set"
+ + " for path " + ent.getPathString());
+ } else {
ent = cache.getEntry(eIdx);
- e.apply(ent);
+ e.apply(ent);
+ }
fastAdd(ent);
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
index d7abd6e..872ef33 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheEntry.java
@@ -377,11 +377,21 @@ public FileMode getFileMode() {
/**
* Set the file mode for this entry.
- *
+ *
* @param mode
* the new mode constant.
+ * @throws IllegalArgumentException
+ * If {@code mode} is {@link FileMode#MISSING},
+ * {@link FileMode#TREE}, or any other type code not permitted
+ * in a tree object.
*/
public void setFileMode(final FileMode mode) {
+ switch (mode.getBits() & FileMode.TYPE_MASK) {
+ case FileMode.TYPE_MISSING:
+ case FileMode.TYPE_TREE:
+ throw new IllegalArgumentException("Invalid mode " + mode
+ + " for path " + getPathString());
+ }
NB.encodeInt32(info, infoOffset + P_MODE, mode.getBits());
}
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java
index 79e95cb..2f2a5ed 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheTree.java
@@ -376,10 +376,6 @@ private int computeSize(final DirCacheEntry[] cache, int cIdx,
}
final FileMode mode = e.getFileMode();
- if (mode.getObjectType() == Constants.OBJ_BAD)
- throw new IllegalStateException("Entry \"" + e.getPathString()
- + "\" has incorrect mode set up.");
-
size += mode.copyToLength();
size += ep.length - pathOffset;
size += OBJECT_ID_LENGTH + 2;
--
1.6.5.rc0.164.g5f6b0
^ permalink raw reply related
* [JGIT PATCH 3/3] Use keep(1) instead of add() when skipping an entry
From: Shawn O. Pearce @ 2009-09-11 19:58 UTC (permalink / raw)
To: Robin Rosenberg; +Cc: git, Shawn O. Pearce
In-Reply-To: <1252699129-6961-2-git-send-email-spearce@spearce.org>
Doing a keep call with a length of 1 will copy the current entry just
like the previous add was doing, but it avoids doing any validation
on the entry. This is sane because the entry can be assumed to be
already valid, since its originating from the destination index.
Signed-off-by: Shawn O. Pearce <sop@google.com>
---
.../jgit/dircache/DirCacheBuildIterator.java | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java
index 9818b76..94fc071 100644
--- a/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java
+++ b/org.spearce.jgit/src/org/spearce/jgit/dircache/DirCacheBuildIterator.java
@@ -111,7 +111,7 @@ public void skip() throws CorruptObjectException {
if (currentSubtree != null)
builder.keep(ptr, currentSubtree.getEntrySpan());
else
- builder.add(currentEntry);
+ builder.keep(ptr, 1);
next(1);
}
--
1.6.5.rc0.164.g5f6b0
^ permalink raw reply related
* [PATCH v2] Re: add documentation for mailinfo.scissors and '--no-scissors'
From: Nicolas Sebrecht @ 2009-09-11 20:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Nicolas Sebrecht, Nanako Shiraishi, git
In-Reply-To: <7vws45wbxq.fsf@alter.siamese.dyndns.org>
The 11/09/09, Junio C Hamano wrote:
> Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
>
> As scissors is not marked with ! (see PARSEOPT section in the rev-parse
> documentation), no-scissors is already available without that hunk, and
> that is why your patch is unnecessary.
>
> Your patch did not mark no-scissors with ! either, so it makes the call to
> rev-parse --parseopt to process your OPTIONS_SPEC pass --no-no-scissors.
Ok. So, the fact that the usage of git-am doesn't tell about
--no-scissors is the expected behaviour?
Without changing the current implementation of rev-parse, I can't see a
smart way to add --no-scissors in the 'git am' usage. Yes, there's still
the possibility to negate both "c,scissors" and "no-scissors" but this
would defeate some of the PARSE_OPT_NONEG purpose, right?
Oh, and I still feel a bit disoriented by this "rev-parse --parseopt"
thing. I guess I need to read the code. :-)
--
Nicolas Sebrecht
^ permalink raw reply
* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 20:43 UTC (permalink / raw)
To: Jeff King; +Cc: Shawn O. Pearce, james bardin, Todd Zullinger, git
In-Reply-To: <20090911164730.GA21536@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> This is actually a bit overengineered. Of the hundreds of calls to
> git_config, there are exactly _two_ which check the return value. And
> neither of them cares whether we parsed files or not; they really only
> care if there was an error. So we could simply return 0 as long as there
> is no error.
>
> This also makes me wonder, though. Git can do wildly different things
> (including hard-to-reverse things) based on config (e.g., just consider
> gc.pruneExpire). Yet we call git_config() without ever checking for
> errors. In the actual parsing routines, we die() if there is an error.
> But if we fail to open the file due to some transient error, we will
> silently ignore the situation.
>
> Granted, such transient errors are unlikely. The biggest reasons for
> failing to open a file are that it doesn't exist, or that we have no
> permission to read it, both of which are treated explicitly in
> git_config as "silently ok". But I wonder if we should simply be dying
> on such an error, and git_config() should just have a void return.
Thanks.
ENOENT should be the same as having an empty file, which is the main point
of the patch and at the same time why you feel this is overengineered. I
agree with you on both counts.
While I also agree that EACCESS and other failures should be treated as
fatal in principle for safety, e.g. when running prune without being able
to read gc.pruneExpire as you mentioned, we would need a tradeoff between
safety and convenience. When asked to help looking at a complex merge in
a colleages' repository, do you want your "git diff" to refuse to run only
because her .git/config cannot be read by you? Of course, you _can_ work
this particular one around by various means (e.g. prefixing GIT_CONFIG=...
to force ignoring the file; telling the colleage that she'd better make
her .git/config readable to you if she wants your help), if either one of
the owner of the .git/config file or the party that wants to access the
repository is a non-person such a workaround would be harder to arrange.
Also there was a move going in the opposite direction to allow a config
file that is syntactically broken to be handled without the parser dying,
primarily to help "git config -e". In the longer term, our direction
shouldn't be adding more die() in the git_config() callchain, but instead
allowing it to report different kind of failures to let the caller act
based on what it does and what the nature of failure is.
For example, "gc" may say "I won't prune because I had to skip some of the
lines in your .git/config because you have syntax errors in them, and I
may have missed the definition of gc.pruneExpire you may wanted to place
on them", while "diff" may ignore that kind of errors.
Having said all that, my preference *for now* would be to ignore "there is
no $HOME/.gitconfig (or /etc/gitconfig) file", but catch all other errors
and die().
There are some other glitches in the current git_config() callchain.
- No config file anywhere gives an error. I agree with you that this is
a bug.
- Having GIT_CONFIG=/no/such/file in the environment gives an error,
which is good.
- config.c::git_parse_file() [*1*] dies when it detects certain file
format errors itself. This is not good for "git config -e", as it
needs to learn core.editor before it can be used to fix such an error.
This function then calls config.c::get_value() and it dies when
config.c::get_value() reports any error.
- config.c::get_value() is called by config.c::git_parse_file() to finish
parsing out the <name, value> pair, and stores the "value" in a form
usable in the code, e.g. a variable defined in environment.c. The
function returns an error on some file format errors (e.g. a variable
name is too long, string quoting unterminated) that signals the calling
config.c::git_parse_file() to die(). These error returns are good (the
caller however may need to be fixed for "config -e" issue not to die).
It then calls the parse callbacck routines. They return error when
they detect semantic errors (e.g. "branch.autosetupmerge = alwys" is
not one of the valid values).
The last one is not the topic of this patch, but it is quite problematic.
When you are interested in finding out what value gc.pruneExpire is set,
you do not care (as long as the configuration file was syntactically
correct and you did not have to skip any file you were supposed to read
due to EACCESS) if "branch.autosetupmerge" has an invalid value.
A possible longer term solution would be to:
- Change the signature of callbacks (e.g. git_default_branch_config()) so
that they return void. They are not allowed to report any semantic
errors while parsing.
- Instead, they use special "INVALID" value and store that when they see
a semantically incorrect value. They may also want to store what the
actual string the config file gave them for later reporting, together
with the name of and the line number in the config file for diagnostic
purposes.
- The user of the internalized value (i.e. "git grep git_branch_track"
shows there are only two, cmd_branch() and cmd_checkout()) must check
for the above INVALID value before they use the variable, and die at
the point of the use.
I'll send an illustration patch separately.
[Footnote]
*1* What a horrible name for this function! It is static so git_ prefix
is unneeded, and if it anticipates it might get someday exported, parse_file
is too generic and should be named git_parse_config_file().
^ permalink raw reply
* Re: [PATCH v2] Re: add documentation for mailinfo.scissors and '--no-scissors'
From: Junio C Hamano @ 2009-09-11 21:00 UTC (permalink / raw)
To: Nicolas Sebrecht; +Cc: Nanako Shiraishi, git, Pierre Habouzit
In-Reply-To: <20090911200849.GB18684@vidovic>
Nicolas Sebrecht <nicolas.s.dev@gmx.fr> writes:
> Ok. So, the fact that the usage of git-am doesn't tell about
> --no-scissors is the expected behaviour?
You _could_ argue that we _could_ describe a long option "frotz" that
lacks the '!' flag in OPTIONS_SPEC as "--[no-]frotz" in the output by
changing the rev-parse --parseopt, if you really want to.
However, I think that is not done deliberately to avoid cluttering the
output. I Cc'ed the primary guilty party ;-) of the parse-options
infrastructure.
Currently, non-bool options are not marked with '!'. Nobody sane would
say "git am --no-directory foo", but "rev-parse --parseopt" acccepts such
a nonsense input, and it is up to the calling script to catch it and barf.
But "rev-parse --parseopt" will start saying "--[no-]directory=" with such
a change, which is not good.
And --no-scissors is not that special. We could add --no-signoff to say
"I do not want to sign-off this one time" explicitly, and it is crazy if
we had to add another line in OPTIONS_SPEC when we want to do so, when it
is clear "signoff" option is a boolean.
As a long term direction, I'd rather not to see "no-" in OPTIONS_SPEC, but
have that taken care of by "rev-parse --parseopt" to keep our sanity. The
only existing offender is "no-verify" in "rebase -i". Let's solve it (if
there is anything to solve, which I doubt) without adding new ones.
^ permalink raw reply
* Re: one half of a rebase
From: Alex Riesen @ 2009-09-11 21:10 UTC (permalink / raw)
To: Geoffrey Irving; +Cc: git, Dylan Simon
In-Reply-To: <7f9d599f0909111025q42e3cdc6vba602b84c1d81215@mail.gmail.com>
On Fri, Sep 11, 2009 at 19:25, Geoffrey Irving <irving@naml.us> wrote:
> If I could do (2) as a separate operation, it would look something like
>
> git cherry-pick-all topic
>
> which is simpler and faster since it avoids switching files back and
> forth (master to topic and back). Is there a robust way to achieve
> the cherry-pick-all semantics with current commands? If not, how
> difficult would it be to partition rebase accordingly?
I have this in my .bashrc:
$ gcp3 ()
{
git format-patch -k --stdout --full-index "$@" | git am -k -3 --binary
}
Then, while on master branch:
$ gcp3 master..topic
^ permalink raw reply
* Re: bash_completion outside repo
From: Jeff King @ 2009-09-11 21:12 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Shawn O. Pearce, james bardin, Todd Zullinger, git
In-Reply-To: <7viqfpw6tv.fsf@alter.siamese.dyndns.org>
On Fri, Sep 11, 2009 at 01:43:24PM -0700, Junio C Hamano wrote:
> While I also agree that EACCESS and other failures should be treated as
> fatal in principle for safety, e.g. when running prune without being able
> to read gc.pruneExpire as you mentioned, we would need a tradeoff between
> safety and convenience. When asked to help looking at a complex merge in
> a colleages' repository, do you want your "git diff" to refuse to run only
> because her .git/config cannot be read by you? Of course, you _can_ work
Sorry, I think I was a bit unclear in the original message. There are
two classes of errors right now:
1. access(fn, R_OK) != 0 (i.e., ENOENT and EACCESS)
2. fopen(fn, "r") != 0
In the case of (1), we treat it as an empty file (except in the case
that _all_ files fail (1), in which case we reported an error, which is
really stupid and is what this patch fixes).
In the case of (2), we treat it as an error, but because nobody actually
bothers to check the error code, it is effectively ignored. What I was
thinking is that (2) should be promoted to die(), and leave (1) as-is.
So I think in your example it would fall under (1), and we both agree
that should be allowed.
But:
> Also there was a move going in the opposite direction to allow a config
> file that is syntactically broken to be handled without the parser dying,
> primarily to help "git config -e". In the longer term, our direction
> shouldn't be adding more die() in the git_config() callchain, but instead
> allowing it to report different kind of failures to let the caller act
> based on what it does and what the nature of failure is.
Yeah, that is the opposite of what I proposed above. But it is a step
towards lib-ifying the config code, which is probably a good thing. The
config code is an utter mess. I am a little hesitant to touch it just
because I don't think there is anything _broken_ in it exactly, but
every time I look at it, I am always shocked by how unnecessarily
complex it is.
I think the "right" thing to do would probably be to have a lib-ified
function to read the config, and then have a wrapper that 99% of the
programs use that just checks the error return and calls die() if there
is a problem. But such a cleanup is likely to introduce new bugs, so I
have let it be (also, because my time is not infinite and there are
other more interesting things to work on ;) ).
> For example, "gc" may say "I won't prune because I had to skip some of the
> lines in your .git/config because you have syntax errors in them, and I
> may have missed the definition of gc.pruneExpire you may wanted to place
> on them", while "diff" may ignore that kind of errors.
Yeah, that makes sense to me, and should be possible with a decent
lib-ified interface.
> Having said all that, my preference *for now* would be to ignore "there is
> no $HOME/.gitconfig (or /etc/gitconfig) file", but catch all other errors
> and die().
OK, then I think we are on the same page.
> There are some other glitches in the current git_config() callchain.
>
> - No config file anywhere gives an error. I agree with you that this is
> a bug.
Yep, and this patch fixes that.
> - Having GIT_CONFIG=/no/such/file in the environment gives an error,
> which is good.
Yep, and and this patch should leave that untouched (I didn't test that
specifically, but I checked that "git config --global" does, and I
assume they use the same code path. Of course, one never knows...).
> A possible longer term solution would be to:
>
> - Change the signature of callbacks (e.g. git_default_branch_config()) so
> that they return void. They are not allowed to report any semantic
> errors while parsing.
>
> - Instead, they use special "INVALID" value and store that when they see
> a semantically incorrect value. They may also want to store what the
> actual string the config file gave them for later reporting, together
> with the name of and the line number in the config file for diagnostic
> purposes.
>
> - The user of the internalized value (i.e. "git grep git_branch_track"
> shows there are only two, cmd_branch() and cmd_checkout()) must check
> for the above INVALID value before they use the variable, and die at
> the point of the use.
That all makes sense to me. My biggest worry is that we will need to be
checking for "INVALID" values in lots of places before actually using
the value from a variable. IOW, it is nice for code to be able to call
into some library call that respects a config variable without having to
care about doing some magic setup to say "Oh, by the way, I am
interesting in the value of diff.foo". At the same time, it is nice for
the library code to not have to say "We're using diff.foo. Let's make
sure somebody has checked the value before using it."
In other words, I would like not-too-syntactically-painful lazy values.
With no runtime overhead. In C. ;)
-Peff
^ permalink raw reply
* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 21:22 UTC (permalink / raw)
To: Jeff King
Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
git
In-Reply-To: <7viqfpw6tv.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> The last one is not the topic of this patch, but it is quite problematic.
> When you are interested in finding out what value gc.pruneExpire is set,
> you do not care (as long as the configuration file was syntactically
> correct and you did not have to skip any file you were supposed to read
> due to EACCESS) if "branch.autosetupmerge" has an invalid value.
>
> A possible longer term solution would be to:
>
> - Change the signature of callbacks (e.g. git_default_branch_config()) so
> that they return void. They are not allowed to report any semantic
> errors while parsing.
>
> - Instead, they use special "INVALID" value and store that when they see
> a semantically incorrect value. They may also want to store what the
> actual string the config file gave them for later reporting, together
> with the name of and the line number in the config file for diagnostic
> purposes.
>
> - The user of the internalized value (i.e. "git grep git_branch_track"
> shows there are only two, cmd_branch() and cmd_checkout()) must check
> for the above INVALID value before they use the variable, and die at
> the point of the use.
>
> I'll send an illustration patch separately.
So here is an illustration to handle _only_ a misspelled
branch.autosetupmerge.
If you have this in your .git/config file:
[branch]
autosetupmerge = nevver
you cannot run "git diff" without this patch. But with this patch, only
the commands that _care_ about this misconfiguration would notice and
report.
The new world order is:
- The config parsing callback should detect semantic errors, but should
not return non-zero, to let the parser continue.
In the _real_ patch, their signatures should be changed to return void.
I wanted to illustrate more important points in this patch so I didn't
do that.
- Instead, when they detect an error in configured values, they make a
note that the value is bad (e.g. I introduced BRANCH_TRACK_CONFIG_ERROR
for this), and also can ask record_bad_config() to record this fact for
possible later reporting purposes. You also _could_ issue error() to
make it easier to detect unrelated but bad configuration for users, but
that is secondary and I didn't do that in this illustration.
- Then, when the values are actually _used_, the users should take notice
of the situation (e.g. both cmd_checkout() and cmd_branch() codepaths
are modified to do this) and ask die_with_bad_config() to report the
error the parser detected earlier.
This way, commands that do not use the value of branch.autosetupmerge,
e.g. "git diff", won't have to abort.
builtin-branch.c | 5 +++-
builtin-checkout.c | 5 +++-
cache.h | 7 ++++++
config.c | 57 ++++++++++++++++++++++++++++++++++++++++++++++++---
4 files changed, 68 insertions(+), 6 deletions(-)
diff --git a/builtin-branch.c b/builtin-branch.c
index 9f57992..10010a7 100644
--- a/builtin-branch.c
+++ b/builtin-branch.c
@@ -635,9 +635,12 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
rename_branch(head, argv[0], rename > 1);
else if (rename && (argc == 2))
rename_branch(argv[0], argv[1], rename > 1);
- else if (argc <= 2)
+ else if (argc <= 2) {
+ if (track == BRANCH_TRACK_CONFIG_ERROR)
+ die_with_bad_config("branch.autosetupmerge");
create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
force_create, reflog, track);
+ }
else
usage_with_options(builtin_branch_usage, options);
diff --git a/builtin-checkout.c b/builtin-checkout.c
index d050c37..3bf57a5 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -630,8 +630,11 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
opts.new_branch = argv0 + 1;
}
- if (opts.track == BRANCH_TRACK_UNSPECIFIED)
+ if (opts.track == BRANCH_TRACK_UNSPECIFIED) {
+ if (git_branch_track == BRANCH_TRACK_CONFIG_ERROR)
+ die_with_bad_config("branch.autosetupmerge");
opts.track = git_branch_track;
+ }
if (conflict_style) {
opts.merge = 1; /* implied */
git_xmerge_config("merge.conflictstyle", conflict_style, NULL);
diff --git a/cache.h b/cache.h
index 5fad24c..f7ff502 100644
--- a/cache.h
+++ b/cache.h
@@ -533,6 +533,7 @@ enum safe_crlf {
extern enum safe_crlf safe_crlf;
enum branch_track {
+ BRANCH_TRACK_CONFIG_ERROR = -2,
BRANCH_TRACK_UNSPECIFIED = -1,
BRANCH_TRACK_NEVER = 0,
BRANCH_TRACK_REMOTE,
@@ -890,6 +891,9 @@ extern const char *packed_object_info_detail(struct packed_git *, off_t, unsigne
/* Dumb servers support */
extern int update_server_info(int);
+/*
+ * configuration file management
+ */
typedef int (*config_fn_t)(const char *, const char *, void *);
extern int git_default_config(const char *, const char *, void *);
extern int git_config_from_file(config_fn_t fn, const char *, void *);
@@ -910,6 +914,9 @@ extern int git_config_global(void);
extern int config_error_nonbool(const char *);
extern const char *config_exclusive_filename;
+extern void record_bad_config(const char *name, const char *value);
+extern void die_with_bad_config(const char *name);
+
#define MAX_GITNAME (1000)
extern char git_default_email[MAX_GITNAME];
extern char git_default_name[MAX_GITNAME];
diff --git a/config.c b/config.c
index e87edea..5b6a929 100644
--- a/config.c
+++ b/config.c
@@ -306,6 +306,41 @@ static void die_bad_config(const char *name)
die("bad config value for '%s'", name);
}
+static struct bad_config_note {
+ struct bad_config_note *next;
+ const char *name;
+ const char *value;
+ int linenr;
+ const char *filename;
+} *bad_config_note;
+
+void record_bad_config(const char *name, const char *value)
+{
+ struct bad_config_note *note = xcalloc(1, sizeof(*note));
+ note->next = bad_config_note;
+ bad_config_note = note;
+ note->name = xstrdup(name);
+ note->value = xstrdup(value);
+ note->linenr = config_linenr;
+ note->filename = config_file_name ? xstrdup(config_file_name) : NULL;
+}
+
+void die_with_bad_config(const char *name)
+{
+ struct bad_config_note *note;
+ for (note = bad_config_note; note; note = note->next)
+ if (!strcmp(name, note->name)) {
+ char whence[PATH_MAX + 200];
+ if (note->filename)
+ sprintf(whence, " in %s", note->filename);
+ else
+ whence[0] = '\0';
+ die("bad config value '%s' for '%s'%s, line %d",
+ note->value, note->name, whence, note->linenr);
+ }
+ die("program error: bad config value for %s not recorded", name);
+}
+
int git_config_int(const char *name, const char *value)
{
long ret = 0;
@@ -324,6 +359,8 @@ unsigned long git_config_ulong(const char *name, const char *value)
int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
{
+ long ret;
+
*is_bool = 1;
if (!value)
return 1;
@@ -333,14 +370,24 @@ int git_config_bool_or_int(const char *name, const char *value, int *is_bool)
return 1;
if (!strcasecmp(value, "false") || !strcasecmp(value, "no") || !strcasecmp(value, "off"))
return 0;
+
*is_bool = 0;
- return git_config_int(name, value);
+
+ ret = 0;
+ if (!git_parse_long(value, &ret)) {
+ record_bad_config(name, value);
+ *is_bool = -1;
+ }
+ return ret;
}
int git_config_bool(const char *name, const char *value)
{
- int discard;
- return !!git_config_bool_or_int(name, value, &discard);
+ int is_bool, val;
+ val = git_config_bool_or_int(name, value, &is_bool);
+ if (is_bool < 0)
+ return is_bool;
+ return !!val;
}
int git_config_string(const char **dest, const char *var, const char *value)
@@ -546,11 +593,13 @@ static int git_default_i18n_config(const char *var, const char *value)
static int git_default_branch_config(const char *var, const char *value)
{
if (!strcmp(var, "branch.autosetupmerge")) {
+ int val;
if (value && !strcasecmp(value, "always")) {
git_branch_track = BRANCH_TRACK_ALWAYS;
return 0;
}
- git_branch_track = git_config_bool(var, value);
+ val = git_config_bool(var, value);
+ git_branch_track = (val < 0) ? BRANCH_TRACK_CONFIG_ERROR : val;
return 0;
}
if (!strcmp(var, "branch.autosetuprebase")) {
^ permalink raw reply related
* Re: bash_completion outside repo
From: Jeff King @ 2009-09-11 21:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
git
In-Reply-To: <7vvdjpuqgx.fsf@alter.siamese.dyndns.org>
On Fri, Sep 11, 2009 at 02:22:06PM -0700, Junio C Hamano wrote:
> So here is an illustration to handle _only_ a misspelled
> branch.autosetupmerge.
>
> If you have this in your .git/config file:
>
> [branch]
> autosetupmerge = nevver
>
> you cannot run "git diff" without this patch. But with this patch, only
> the commands that _care_ about this misconfiguration would notice and
> report.
OK, that example makes sense. But I'm a little dubious of how this
scales to something like color.diff.plain. Who is responsible for
checking? Do we do it at the beginning of every program which cares
about diff values? If so, what is the failure mode when we forget (and I
suspect we will, because it is easy for programs to call into unexpected
code that is three layers deep)?
I think something like that needs to "belong" to the diff code itself. I
guess in the case of "diff", we could check all diff-related config at
diff setup time. But what about something used in several places, like
core.quotepath?
-Peff
^ permalink raw reply
* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 21:57 UTC (permalink / raw)
To: Jeff King
Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
git
In-Reply-To: <20090911212938.GA26589@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I think something like that needs to "belong" to the diff code itself. I
> guess in the case of "diff", we could check all diff-related config at
> diff setup time.
Not necessarily. You do not want to care about color configuration if you are
doing diff --raw for example. The one that first uses the color variable
should be able to notice the breakage, no?
> But what about something used in several places, like
> core.quotepath?
Exactly the same way I checked what codepaths needed to fix for the
autosetupmerge stuff. core.quotepath internally sets quote_path_fully,
and the sole user of quote_path_fully is sq_must_quote() which is only
used by next_quote_pos(). So you can have your check very isolated.
^ permalink raw reply
* Re: obnoxious CLI complaints
From: René Scharfe @ 2009-09-11 22:01 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Jakub Narebski, Brendan Miller, git
In-Reply-To: <alpine.LFD.2.01.0909110744030.3654@localhost.localdomain>
Am 11.09.2009 16:47, schrieb Linus Torvalds:
>
>
> On Fri, 11 Sep 2009, René Scharfe wrote:
>>
>> Using zlib directly avoids the overhead of a pipe and of buffering the
>> output for blocked writes; surprisingly (to me), it isn't any faster.
>
> In fact, it should be slower.
>
> On SMP, you're quite likely better off using the pipe, and compressing on
> another CPU. Of course, it's usually the case that the compression is _so_
> much slower than generating the tar-file (especially for the hot-cache
> case) that it doesn't matter or the pipe overhead is even a slowdown.
>
> But especially if generating the tar-file has some delays in it
> (cold-cache object lookup, whatever), the "compress in separate process"
> is likely simply better, because you can compress while the other process
> is looking up data for the tar.
Yes, that makes sense and can be seen here (quad core, Fedora 11, best
of five consecutive runs, Linux kernel repo):
# git v1.6.5-rc0
$ time git archive --format=tar v2.6.31 | gzip -6 >/dev/null
real 0m16.591s
user 0m19.769s
sys 0m0.474s
# git v1.6.5-rc0 + patch
$ time ../git/git archive --format=tar.gz -6 v2.6.31 >/dev/null
real 0m20.390s
user 0m20.299s
sys 0m0.088s
User time is quite similar, real time is lower when using a pipe.
But what has bugged me since I added zip support is this result:
# git v1.6.5-rc0
$ time git archive --format=zip -6 v2.6.31 >/dev/null
real 0m16.471s
user 0m16.340s
sys 0m0.128s
I'd have expected this to be the slowest case, because it's compressing
all files separately, i.e. it needs to create and flush the compression
context lots of times instead of only once as in the two cases above.
And it's sequential and uses zlib, just like the tar.gz format. I
suspect the convenience function gzwrite() adds this overhead.
Oh, I just discovered pigz (http://zlib.net/pigz/), a parallel gzip:
# git v1.6.5-rc0, pigz 2.1.5
$ time git archive --format=tar v2.6.31 | pigz -6 >/dev/null
real 0m6.251s
user 0m21.383s
sys 0m0.547s
So pipes win. :) Still need to investigate why zip is as (relatively)
fast as it is, though.
René
^ permalink raw reply
* Re: bash_completion outside repo
From: Junio C Hamano @ 2009-09-11 22:04 UTC (permalink / raw)
To: Jeff King
Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
git
In-Reply-To: <7vr5uduotz.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> Jeff King <peff@peff.net> writes:
>
>> I think something like that needs to "belong" to the diff code itself. I
>> guess in the case of "diff", we could check all diff-related config at
>> diff setup time.
>
> Not necessarily. You do not want to care about color configuration if you are
> doing diff --raw for example. The one that first uses the color variable
> should be able to notice the breakage, no?
>
>> But what about something used in several places, like
>> core.quotepath?
>
> Exactly the same way I checked what codepaths needed to fix for the
> autosetupmerge stuff. core.quotepath internally sets quote_path_fully,
> and the sole user of quote_path_fully is sq_must_quote() which is only
> used by next_quote_pos(). So you can have your check very isolated.
Note that I was _not_ defending the approach my illustration took. I
merely was pointing out that core.quotepath and diff.color are _not_
valid counterexamples to it.
A better counter-proposal would be made something along this line:
Currently, core.frotz and nitfol.rezrov both map internally to a
variable xyzzy, so config parser could set xyzzy = INVALID_INFOCOM
but the user of xyzzy cannot report which configuration variable
caused the error with your illustration scheme. Here is my
attempt to solve this issue.
...patch follows...
And I am sure we will be able to find such examples that my illustration
patch is not _exactly_ the best approach to solve pretty easily around
remote.*.* and branch.*.* variables.
^ permalink raw reply
* Re: bash_completion outside repo
From: Jeff King @ 2009-09-11 22:05 UTC (permalink / raw)
To: Junio C Hamano
Cc: Michael J Gruber, Shawn O. Pearce, james bardin, Todd Zullinger,
git
In-Reply-To: <7vr5uduotz.fsf@alter.siamese.dyndns.org>
On Fri, Sep 11, 2009 at 02:57:28PM -0700, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I think something like that needs to "belong" to the diff code itself. I
> > guess in the case of "diff", we could check all diff-related config at
> > diff setup time.
>
> Not necessarily. You do not want to care about color configuration if you are
> doing diff --raw for example. The one that first uses the color variable
> should be able to notice the breakage, no?
>
> > But what about something used in several places, like
> > core.quotepath?
>
> Exactly the same way I checked what codepaths needed to fix for the
> autosetupmerge stuff. core.quotepath internally sets quote_path_fully,
> and the sole user of quote_path_fully is sq_must_quote() which is only
> used by next_quote_pos(). So you can have your check very isolated.
I guess I'm just worried that in doing this for _every_ variable we are
going to run across cases where variables are used in several different
codepaths, and we are going to end up adding a large number of tests for
"is this thing valid". And if we forget one, it's going to cause us to
access some sentinel value that may cause a segfault.
But that is just my gut feeling. I haven't actually looked at doing a
full-scale conversion.
-Peff
^ permalink raw reply
* Re: obnoxious CLI complaints
From: Linus Torvalds @ 2009-09-11 22:16 UTC (permalink / raw)
To: René Scharfe; +Cc: Jakub Narebski, Brendan Miller, git
In-Reply-To: <4AAAC8CE.8020302@lsrfire.ath.cx>
On Sat, 12 Sep 2009, René Scharfe wrote:
>
> But what has bugged me since I added zip support is this result:
>
> # git v1.6.5-rc0
> $ time git archive --format=zip -6 v2.6.31 >/dev/null
>
> real 0m16.471s
> user 0m16.340s
> sys 0m0.128s
>
> I'd have expected this to be the slowest case, because it's compressing
> all files separately, i.e. it needs to create and flush the compression
> context lots of times instead of only once as in the two cases above.
Oh no, I think it's easily explained.
Compressing many small files really is often cheaper than compressing one
large one.
With lots of small files, you end up being very limited in the
search-space, so the compression decisions get simpler. Compression in
general is not O(n), it's some non-linear factor, often something like
O(n**2).
Of course, all compression libraries have an upper bound on the
non-linearity (often expressed as a "window size"), so a particular
compression algorithm may end up being close to O(n) (with a huge
constant). But that upper bound will only kick in for large files, small
files that fit entirely into the compression window will still see the
underlying O(n**2) or whatever.
But I have no actual numbers to back up the above blathering. But feel
free to try to compress 10 small files and compare it to compressing one
file that is as big as the sum. I bet you'll see it.
Linus
^ 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