* Re: what are the chances of a 'pre-upload' hook?
From: Jeff King @ 2011-11-26 23:51 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sitaram Chamarty, Git
In-Reply-To: <CAPc5daXY_4aimugj8Z4BFE8YvBSM1K+evPU69rLGH5ETo6PO=Q@mail.gmail.com>
On Sat, Nov 26, 2011 at 03:47:09PM -0800, Junio C Hamano wrote:
> > My point is to make it available, give it safe
> > semantics by default, and let people who are running daemon-like service
> > (i.e., where the admin controls the daemon and arbitrary users can't
> > write into the hooks directory) use it by setting an environment
> > variable, rather than patching git.
>
> I think we re on the same page on that point, and this thread is to find
> such a safe default and safe semantics when enabled.
>
> Unfortunately neither your "trusted" switch nor check the gid of repository
> is that safe thing (sane default part is easy; do not allow it by default).
Sorry, why is the trusted switch not a sane thing? By turning it on, you
are saying "it's OK to run arbitrary code from the repo as the current
user". It's _exactly_ what some people are going to want to do[1],
regardless of any other heuristics.
Sure, maybe it's giving people rope to hang themselves with, but I don't
see a problem with that as long as the issues are clearly laid out in
the documentation.
-Peff
[1] An alternate and even more flexible form is to not just say "it's OK
to run hooks", but to say "run this particular hook as a
pre-upload-hook" without any regard for what's in $GIT_DIR/hooks. It is
a superset of the other form, because of course the hook you tell it
to run can be "sh $GIT_DIR/hooks/pre-upload-pack".
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Jeff King @ 2011-11-26 23:31 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sitaram Chamarty, Git Mailing List
In-Reply-To: <7vr50uwk7x.fsf@alter.siamese.dyndns.org>
On Sat, Nov 26, 2011 at 03:13:38PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > Bob could run a specialized server for (b) that listens on a unix socket
> > and triggers his hook. But why? Why not just do the whole thing over
> > git-daemon or smart http, which already exist?
>
> If that "whole thing" is "to allow an arbitrary code to run anywhere as
> incoming user", I would apply the "why?" to a different part of the
> statemennt. Why allow running an arbitrary code at all?
Because there are useful hooks that Bob wants to run when somebody
fetches or pushes to/from his repository?
> A pre-anything hook wants to see if the accessing user, not the owner of
> the repository, can or cannot do something to the repository and decide
> what to do.
It depends on what you want your hook to do. I thought Sitaram's
use-case was putting something like "git fetch upstream" into a hook
that runs before upload-pack, to create a transparent proxy. That has
nothing to do with the accessing user.
At GitHub, we run a pre-upload-pack hook to keep statistics. That has
nothing to do with the accessing user. We have to patch git to provide
the hook.
And even if you _did_ want to know something about the accessing user,
that doesn't mean you necessarily can or want to be running code as
them. If I'm running a smart-http server, I might have verified the user
already via cookie, client cert, or basic auth. That information could
be passed down to a pre-upload-pack hook where it could make a decision.
But we don't _have_ a pre-upload-pack hook, because it can be dangerous
in some situations. My point is to make it available, give it safe
semantics by default, and let people who are running daemon-like service
(i.e., where the admin controls the daemon and arbitrary users can't
write into the hooks directory) use it by setting an environment
variable, rather than patching git.
-Peff
^ permalink raw reply
* Re: git branch -M" regression in 1.7.7?
From: Andreas Schwab @ 2011-11-26 23:09 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Nieder, Conrad Irwin,
☂Josh Chia (谢任中), git,
Soeren Sonnenburg
In-Reply-To: <7v39day0fb.fsf@alter.siamese.dyndns.org>
Junio C Hamano <gitster@pobox.com> writes:
> I havn't look at the patch (not a regression between 1.7.7 and 1.7.8 so
> not a candidate for the remainder of this cycle), but I like the above
> description quite a lot. I think Linus's "git reset --sane" which was
> initially called "git reset --merge" but ended up as "git reset --keep"
> should have been spelled as "checkout -B <current-branch>" from the
> beginning.
It is more convenient if you don't have to spell out the name of the
current branch (which fails if you aren't on a branch).
Andreas.
--
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
"And now for something completely different."
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Junio C Hamano @ 2011-11-26 23:13 UTC (permalink / raw)
To: Jeff King; +Cc: Sitaram Chamarty, Git Mailing List
In-Reply-To: <20111126225519.GA29482@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Bob could run a specialized server for (b) that listens on a unix socket
> and triggers his hook. But why? Why not just do the whole thing over
> git-daemon or smart http, which already exist?
If that "whole thing" is "to allow an arbitrary code to run anywhere as
incoming user", I would apply the "why?" to a different part of the
statemennt. Why allow running an arbitrary code at all?
Running things as Bob with setuid is not a solution, either.
A pre-anything hook wants to see if the accessing user, not the owner of
the repository, can or cannot do something to the repository and decide
what to do.
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Jeff King @ 2011-11-26 22:55 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Sitaram Chamarty, Git Mailing List
In-Reply-To: <7v7h2my0ky.fsf@alter.siamese.dyndns.org>
On Sat, Nov 26, 2011 at 02:34:53PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > The easiest way would be something like a "trust remote hooks"
> > environment variable, off by default. Admins in situation (2) could set
> > it for their git-daemon (or webserver, or whatever, or
> > gitolite-over-ssh), once they decided it was safe.
>
> Alice and Bob may work on the same project, and they may want to trust
> each other as participants of that project, but that does not mean Alice
> wants to give Bob a blanket access to places she owns that are not shared
> with the project members (e.g. her $HOME directory), so I am afraid "trust
> remote hooks" is not a workable solution for the casual sharing on sites
> that do not fall into either of your two classes.
Of course. My point isn't that such a feature would cover all cases, but
that it would give people a tool to make that decision for themselves,
instead of blanket-forbidding it.
> The real reason why the upload-hook violates the expectation of the users
> is because it would run as the user who fetches, I think. If it ran with
> the intersection of capabilities of the owner of the repository and the
> user who is fetching, I suspect that we would not have to worry about it.
Sure. And I think we would probably trust automatically a hook that is
owned by the executing uid. Or possibly root, though that could be
surprising in NFS root-squashed environments.
> What would happen if we allowed some hooks to run only when the process is
> running under a group-id that can write into the repository? When Alice
> fetches from the repository, it would still run as her and would have an
> access to her $HOME, so this won't really work yet, but I am wondering if
> there is a workable alternative along this line.
I don't think so. It comes down to this: Alice is executing arbitrary
code from Bob's repository, which Bob (and maybe others) have access to
write. This is giving Bob permission to run arbitrary code as Alice's
user.
Checking things like group access doesn't matter. If Alice is in the
group, too, it doesn't make a difference; Bob can still write the files,
and Alice is still running the code under her UID.
I think the only solutions are:
1. Alice accepts that she can trust Bob enough to run his arbitrary
code.
2. We use some technique to run the code as the repo owner's UID.
The usual methods for doing (2) are:
a. setuid, though doing it right can be quite tricky (e.g., cleansing
environment, file descriptors, etc). And it requires root
cooperation.
b. running a server with a socket as Bob, and triggering the hook code
from the server
Bob could run a specialized server for (b) that listens on a unix socket
and triggers his hook. But why? Why not just do the whole thing over
git-daemon or smart http, which already exist?
-Peff
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Junio C Hamano @ 2011-11-26 22:48 UTC (permalink / raw)
To: Carlos Martín Nieto
Cc: Henrik Grubbström, Git Mailing list, Junio C Hamano
In-Reply-To: <20111125170219.GD10417@beez.lab.cmartin.tk>
Carlos Martín Nieto <cmn@elego.de> writes:
> diff --git a/convert.c b/convert.c
> index 86e9c29..c050b86 100644
> --- a/convert.c
> +++ b/convert.c
> @@ -880,20 +880,29 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,
> const char *input, size_t *isize_p,
> char *output, size_t *osize_p)
> {
> - size_t count;
> + size_t count, o = 0;
> + static int want_lf = 0;
I do not think we want function scope static state anywhere in the cascade
filter chain, as it will forbid us from running more than one output chain
at the same time in the future. I think the correct way to structure it
would be to create lf_to_crlf_filter as a proper subclass of stream_filter
(see how cascade_filter_fn() casts its filter argument down to an instance
of the cascade_filter class and uses it to keep track of its state) and
keep this variable as its own filter state [*1*].
[Footnote]
*1* We currently use a singleton instance of lf_to_crlf_filter object
because the implementation assumed there is no need for per-instance
state.
^ permalink raw reply
* Re: git branch -M" regression in 1.7.7?
From: Junio C Hamano @ 2011-11-26 22:38 UTC (permalink / raw)
To: Jonathan Nieder
Cc: Conrad Irwin, ☂Josh Chia (谢任中), git,
Soeren Sonnenburg
In-Reply-To: <20111126085455.GB22656@elie.hsd1.il.comcast.net>
Jonathan Nieder <jrnieder@gmail.com> writes:
> In other words, when on master, "git checkout -B master <commit>"
> would be another way to say "git reset --keep <commit>", except that
> it also sets up tracking.
I havn't look at the patch (not a regression between 1.7.7 and 1.7.8 so
not a candidate for the remainder of this cycle), but I like the above
description quite a lot. I think Linus's "git reset --sane" which was
initially called "git reset --merge" but ended up as "git reset --keep"
should have been spelled as "checkout -B <current-branch>" from the
beginning.
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Junio C Hamano @ 2011-11-26 22:34 UTC (permalink / raw)
To: Jeff King; +Cc: Sitaram Chamarty, Git Mailing List
In-Reply-To: <20111125144007.GA4047@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The easiest way would be something like a "trust remote hooks"
> environment variable, off by default. Admins in situation (2) could set
> it for their git-daemon (or webserver, or whatever, or
> gitolite-over-ssh), once they decided it was safe.
Alice and Bob may work on the same project, and they may want to trust
each other as participants of that project, but that does not mean Alice
wants to give Bob a blanket access to places she owns that are not shared
with the project members (e.g. her $HOME directory), so I am afraid "trust
remote hooks" is not a workable solution for the casual sharing on sites
that do not fall into either of your two classes.
The real reason why the upload-hook violates the expectation of the users
is because it would run as the user who fetches, I think. If it ran with
the intersection of capabilities of the owner of the repository and the
user who is fetching, I suspect that we would not have to worry about it.
What would happen if we allowed some hooks to run only when the process is
running under a group-id that can write into the repository? When Alice
fetches from the repository, it would still run as her and would have an
access to her $HOME, so this won't really work yet, but I am wondering if
there is a workable alternative along this line.
^ permalink raw reply
* What to do if the path of my git submodules change upstream
From: Harald Heigl @ 2011-11-26 16:45 UTC (permalink / raw)
To: git
> Hi everyone!
>
> First setup of my git was a server (with ssh) and some clients.
>
> Today I changed to gitolite because I wanted a more sophisticated way of
> managing my repos. So far so good…
> So the old path “ssh://[ip]/[fullpath].git” would change to a new path
> “git@[servername]:[gitreponame]”.
> This is no problem for “normal” repos, I change the remote origin and
> continue using push and pull.
>
> I have some submodules:
> I changed the .gitmodules to reflect my changes, did a git submodule sync.
> This works flawlessly too!
>
> But what if someone wants to checkout an older version of the project?
(for
> comparison, or because he/she wanted to try something out)
> He would get an old .gitmodules with old paths.
> After a git submodule sync he would get errors, because old paths won’t
> work anymore, because I changed some paths on the server
>
> It is only one project I have this problem and therein I changed the
> .gitmodules only 3 times. Is it possible to rewrite .gitmodules on these
> specific commits on the server (perhaps with git-filter-branch)?
> Or is there another easy solution? Has someone ever had this problem?
>
> Hope you can help,
> Kind regards,
> Harald Heigl
Hi everyone!
I try to answer myself, I found a possibility to change the file
".gitmodules" in my git history:
---------------
Method 1 with some git plumbing:
cd [gitrepo]
cd ..
[prepare my "new" .gitmodules-File here]
cd [gitrepo]
git hash-object -w ../.gitmodules
git filter-branch --index-filter 'git rm --cached --ignore-unmatch
.gitmodules;git update-index --add --cacheinfo 100644 \
3e0f4ab4126e01d55fda040234e3593aea1bff79 .gitmodules'
022d9c5fecb4d6c268365dfe186aa65389bcd492^..
Push this new branch to a remote repo and clone from there again (so I'll
have a sober dir, without the old branches)
More or less this adds a new object to git (with SHA1 3e0f4ab in my case)
and then rewrites every index to remove my old .gitmodules and add the newly
entered .gitmodules.
The only "problem" is that this will rewrite all Commits and therefore also
the SHA1 which won't match the one on the server.
As my understanding in git is now: After a change of the (already pushed)
commits I'll have to delete the remote repo, push my changed repo upstream
and then everyone in the team will have to clone this new repo. Good that
this project affects only a small group in my company.
------------
Method 2 with git rebase -i:
git rebase -i 022d9c5fecb4d6c268365dfe186aa65389bcd492^
This will rebase anything and I can rewrite my file on the specific commits.
But this will change my whole history (and SHAs too). If there are some
merges in these commit the rebase would flatten anything. This wouldn't
disturb that way, but leads to conflicts I have to resolve on the rebase
side for every commit that conflicts.
------------
I'll think over it, if I use Method 1 or don't change anything. Perhaps the
latter would be the best :-)
Hope this helps others,
Kind regards
^ permalink raw reply
* Re: gitweb ignores git-daemon-export-ok and displays all repositories
From: Jakub Narebski @ 2011-11-26 14:37 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Erik Wenzel, John 'Warthog9' Hawley,
Matthias Lederhofer, Sitaram Chamarty
In-Reply-To: <20111116224706.GA17851@elie.hsd1.il.comcast.net>
I'm sorry for the delay in responding.
On Wed, 16 Nov 2011, Jonathan Nieder wrote:
> Erik Wenzel wrote (http://bugs.debian.org/648561):
>> Am 13.11.2011 um 02.19 schrieb Jonathan Nieder:
>>
>>> The git-daemon(1) manpage describes git daemon, not gitweb, so I guess
>>> you mean that
>>>
>>> # Do not list projects without git-daemon-export-ok in the
>>> # projects list.
>>> our $export_ok = "git-daemon-export-ok";
>>>
>>> # Do not allow access to projects not listed in the projects
>>> # list.
>>> our $strict_export = 1;
>>>
>>> should be the default.
>> [...]
>> Because I think this is the way a user is expecting the behavior of gitweb.
>> As I do. Don't let gitweb overwrite the meaning of "git-daemon-export-ok".
>> Just act like git-daemon. Keep it simple and stupid.
>
> My first thought was that if we could turn back time, it would
> probably be best for both git-daemon and gitweb to look for a file
> named git-export-ok. In the present world, maybe git-daemon-export-ok
> is a good substitute for that.
>
> What do you think? Should the default in the makefile be changed? If
> so, how could we go about it to avoid disturbing existing
> installations? (For example, in a system where no repositories have
> the export-ok files and "git daemon" is configured to run with
> --export-all, the effect would be to make repos suddenly disappear
> from the projects list in gitweb. Unpleasant.)
I think the best solution would be to leave it up to the tool that
manages both git-daemon and git (manages access to git repositories),
like e.g. gitolite. It would be this tool that is to be responsible
for synchronizing git-daemon and gitweb behavior, i.e. make either
both use 'git-daemon-export-ok', or both export all.
--
Jakub Narebski
Poland
^ permalink raw reply
* [PATCH] grep: enable multi-threading for -p and -W
From: René Scharfe @ 2011-11-26 12:15 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <4ECFC320.4030003@lsrfire.ath.cx>
Move attribute reading, which is not thread-safe, into add_work(), under
grep_mutex. That way we can stop turning off multi-threading if -p or -W
is given, as the diff attribute for each file is gotten safely now.
Signed-off-by: Rene Scharfe <rene.scharfe@lsrfire.ath.cx>
---
Goes on top of your patch. Needs testing..
builtin/grep.c | 33 +++++++++++++++++++++++++--------
grep.c | 25 +++++++------------------
grep.h | 5 +++--
revision.c | 2 +-
4 files changed, 36 insertions(+), 29 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 3d7329d..5534111 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -18,6 +18,7 @@
#include "quote.h"
#include "dir.h"
#include "thread-utils.h"
+#include "userdiff.h"
static char const * const grep_usage[] = {
"git grep [options] [-e] <pattern> [<rev>...] [[--] <path>...]",
@@ -43,6 +44,7 @@ enum work_type {WORK_SHA1, WORK_FILE};
struct work_item {
enum work_type type;
char *name;
+ struct userdiff_driver *userdiff_driver;
/* if type == WORK_SHA1, then 'identifier' is a SHA1,
* otherwise type == WORK_FILE, and 'identifier' is a NUL
@@ -114,7 +116,17 @@ static pthread_cond_t cond_result;
static int skip_first_line;
-static void add_work(enum work_type type, char *name, void *id)
+/* Reading attributes is not thread-safe, so callers need to lock. */
+static struct userdiff_driver *get_userdiff_driver(struct grep_opt *opt,
+ const char *name)
+{
+ if (opt->funcbody || opt->funcname)
+ return userdiff_find_by_path(name);
+ return NULL;
+}
+
+static void add_work(struct grep_opt *opt, enum work_type type, char *name,
+ void *id)
{
grep_lock();
@@ -124,6 +136,7 @@ static void add_work(enum work_type type, char *name, void *id)
todo[todo_end].type = type;
todo[todo_end].name = name;
+ todo[todo_end].userdiff_driver = get_userdiff_driver(opt, name);
todo[todo_end].identifier = id;
todo[todo_end].done = 0;
strbuf_reset(&todo[todo_end].out);
@@ -158,13 +171,13 @@ static void grep_sha1_async(struct grep_opt *opt, char *name,
unsigned char *s;
s = xmalloc(20);
memcpy(s, sha1, 20);
- add_work(WORK_SHA1, name, s);
+ add_work(opt, WORK_SHA1, name, s);
}
static void grep_file_async(struct grep_opt *opt, char *name,
const char *filename)
{
- add_work(WORK_FILE, name, xstrdup(filename));
+ add_work(opt, WORK_FILE, name, xstrdup(filename));
}
static void work_done(struct work_item *w)
@@ -212,24 +225,26 @@ static void *run(void *arg)
struct grep_opt *opt = arg;
while (1) {
+ struct userdiff_driver *drv;
struct work_item *w = get_work();
if (!w)
break;
opt->output_priv = w;
+ drv = w->userdiff_driver;
if (w->type == WORK_SHA1) {
unsigned long sz;
void* data = load_sha1(w->identifier, &sz, w->name);
if (data) {
- hit |= grep_buffer(opt, w->name, data, sz);
+ hit |= grep_buffer(opt, w->name, drv, data, sz);
free(data);
}
} else if (w->type == WORK_FILE) {
size_t sz;
void* data = load_file(w->identifier, &sz);
if (data) {
- hit |= grep_buffer(opt, w->name, data, sz);
+ hit |= grep_buffer(opt, w->name, drv, data, sz);
free(data);
}
} else {
@@ -417,10 +432,11 @@ static int grep_sha1(struct grep_opt *opt, const unsigned char *sha1,
int hit;
unsigned long sz;
void *data = load_sha1(sha1, &sz, name);
+ struct userdiff_driver *drv = get_userdiff_driver(opt, name);
if (!data)
hit = 0;
else
- hit = grep_buffer(opt, name, data, sz);
+ hit = grep_buffer(opt, name, drv, data, sz);
free(data);
free(name);
@@ -479,10 +495,11 @@ static int grep_file(struct grep_opt *opt, const char *filename)
int hit;
size_t sz;
void *data = load_file(filename, &sz);
+ struct userdiff_driver *drv = get_userdiff_driver(opt, name);
if (!data)
hit = 0;
else
- hit = grep_buffer(opt, name, data, sz);
+ hit = grep_buffer(opt, name, drv, data, sz);
free(data);
free(name);
@@ -1001,7 +1018,7 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
opt.regflags |= REG_ICASE;
#ifndef NO_PTHREADS
- if (online_cpus() == 1 || !grep_threads_ok(&opt))
+ if (online_cpus() == 1)
use_threads = 0;
if (use_threads) {
diff --git a/grep.c b/grep.c
index 7a070e9..ff14a98 100644
--- a/grep.c
+++ b/grep.c
@@ -942,25 +942,13 @@ static int look_ahead(struct grep_opt *opt,
return 0;
}
-int grep_threads_ok(const struct grep_opt *opt)
-{
- /* If this condition is true, then we may use the attribute
- * machinery in grep_buffer_1. The attribute code is not
- * thread safe, so we disable the use of threads.
- */
- if ((opt->funcname || opt->funcbody)
- && !opt->unmatch_name_only && !opt->status_only && !opt->name_only)
- return 0;
-
- return 1;
-}
-
static void std_output(struct grep_opt *opt, const void *buf, size_t size)
{
fwrite(buf, size, 1, stdout);
}
static int grep_buffer_1(struct grep_opt *opt, const char *name,
+ struct userdiff_driver *drv,
char *buf, unsigned long size, int collect_hits)
{
char *bol = buf;
@@ -1011,7 +999,6 @@ static int grep_buffer_1(struct grep_opt *opt, const char *name,
if ((opt->funcname || opt->funcbody)
&& !opt->unmatch_name_only && !opt->status_only &&
!opt->name_only && !binary_match_only && !collect_hits) {
- struct userdiff_driver *drv = userdiff_find_by_path(name);
if (drv && drv->funcname.pattern) {
const struct userdiff_funcname *pe = &drv->funcname;
xdiff_set_find_func(&xecfg, pe->pattern, pe->cflags);
@@ -1167,23 +1154,25 @@ static int chk_hit_marker(struct grep_expr *x)
}
}
-int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size)
+int grep_buffer(struct grep_opt *opt, const char *name,
+ struct userdiff_driver *userdiff_driver,
+ char *buf, unsigned long size)
{
/*
* we do not have to do the two-pass grep when we do not check
* buffer-wide "all-match".
*/
if (!opt->all_match)
- return grep_buffer_1(opt, name, buf, size, 0);
+ return grep_buffer_1(opt, name, userdiff_driver, buf, size, 0);
/* Otherwise the toplevel "or" terms hit a bit differently.
* We first clear hit markers from them.
*/
clr_hit_marker(opt->pattern_expression);
- grep_buffer_1(opt, name, buf, size, 1);
+ grep_buffer_1(opt, name, userdiff_driver, buf, size, 1);
if (!chk_hit_marker(opt->pattern_expression))
return 0;
- return grep_buffer_1(opt, name, buf, size, 0);
+ return grep_buffer_1(opt, name, userdiff_driver, buf, size, 0);
}
diff --git a/grep.h b/grep.h
index a652800..20224b5 100644
--- a/grep.h
+++ b/grep.h
@@ -121,14 +121,15 @@ struct grep_opt {
void *output_priv;
};
+struct userdiff_driver;
+
extern void append_grep_pat(struct grep_opt *opt, const char *pat, size_t patlen, const char *origin, int no, enum grep_pat_token t);
extern void append_grep_pattern(struct grep_opt *opt, const char *pat, const char *origin, int no, enum grep_pat_token t);
extern void append_header_grep_pattern(struct grep_opt *, enum grep_header_field, const char *);
extern void compile_grep_patterns(struct grep_opt *opt);
extern void free_grep_patterns(struct grep_opt *opt);
-extern int grep_buffer(struct grep_opt *opt, const char *name, char *buf, unsigned long size);
+extern int grep_buffer(struct grep_opt *opt, const char *name, struct userdiff_driver *userdiff_driver, char *buf, unsigned long size);
extern struct grep_opt *grep_opt_dup(const struct grep_opt *opt);
-extern int grep_threads_ok(const struct grep_opt *opt);
#endif
diff --git a/revision.c b/revision.c
index 8764dde..95cb8c2 100644
--- a/revision.c
+++ b/revision.c
@@ -2126,7 +2126,7 @@ static int commit_match(struct commit *commit, struct rev_info *opt)
if (!opt->grep_filter.pattern_list && !opt->grep_filter.header_list)
return 1;
return grep_buffer(&opt->grep_filter,
- NULL, /* we say nothing, not even filename */
+ NULL, NULL, /* we say nothing, not even filename */
commit->buffer, strlen(commit->buffer));
}
--
1.7.7
^ permalink raw reply related
* Re: [PATCH] honour GIT_ASKPASS for querying username in git-svn
From: Sven Strickroth @ 2011-11-26 11:33 UTC (permalink / raw)
To: git; +Cc: gitster
In-Reply-To: <CABPQNSbfM0JRVPk3fxfSEq7QaO-fynHM8FBGpPribdgeRqpZKA@mail.gmail.com>
Hi,
there's also another point where git-svn doesn't honour GIT_ASKPASS:
>From 632c264d0de127c35fbe45866ed81e832f357d56 Mon Sep 17 00:00:00 2001
From: Sven Strickroth <email@cs-ware.de>
Date: Sat, 26 Nov 2011 12:01:19 +0100
Subject: [PATCH] honour GIT_ASKPASS for querying further actions on unknown
certificates
git-svn reads user answers from an interactive terminal.
This behavior cause GUIs to hang waiting for git-svn to
complete.
Signed-off-by: Sven Strickroth <email@cs-ware.de>
---
git-svn.perl | 9 +++++++++--
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/git-svn.perl b/git-svn.perl
index e30df22..d7aeb11 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -4361,7 +4361,14 @@ prompt:
"(R)eject, accept (t)emporarily or accept (p)ermanently? " :
"(R)eject or accept (t)emporarily? ";
STDERR->flush;
- $choice = lc(substr(<STDIN> || 'R', 0, 1));
+ if (exists $ENV{GIT_ASKPASS}) {
+ print STDERR "\n";
+ open(PH, "-|", $ENV{GIT_ASKPASS}, "Certificate unknown");
+ $choice = lc(substr(<PH> || 'R', 0, 1));
+ close(PH);
+ } else {
+ $choice = lc(substr(<STDIN> || 'R', 0, 1));
+ }
if ($choice =~ /^t$/i) {
$cred->may_save(undef);
} elsif ($choice =~ /^r$/i) {
--
1.7.7.1.msysgit.0
--
Best regards,
Sven Strickroth
ClamAV, a GPL anti-virus toolkit http://www.clamav.net
PGP key id F5A9D4C4 @ any key-server
^ permalink raw reply related
* Re: git branch -M" regression in 1.7.7?
From: Jonathan Nieder @ 2011-11-26 8:54 UTC (permalink / raw)
To: Conrad Irwin
Cc: ☂Josh Chia (谢任中), git,
Soeren Sonnenburg
In-Reply-To: <CAOTq_pv4dyAkbqye+diK9mTTsrTg9OKg0tExKcfDgs8RfiTwTQ@mail.gmail.com>
Conrad Irwin wrote:
> I thought after reading your patch about making it just do:
>
> if (!strcmp(oldname, newname))
> exit(0);
>
> but I guess it would then not mark an entry in the reflog that people
> could be relying on...
Ah, this deserves a comment.
I thought about doing the same thing, and then didn't do it because I
wanted to make sure that
git branch -M nonexistent nonexistent
does not succeed. Which presumably warrants another test:
test_expect_success "rename-to-self dwimery doesn't hide nonexistent ref" '
test_must_fail git branch -M nonexistent nonexistent &&
test_must_fail git rev-parse --verify nonexistent
'
Sloppy of me.
>> + clobber_head_ok = !strcmp(oldname, newname);
>> +
>> + validate_new_branchname(newname, &newref, force, clobber_head_ok);
>
> This looks ok, and will be improvable if the NEEDSWORK in branch.h is done.
Thanks for looking it over.
> The other thing I wonder is whether "git checkout -B master HEAD" or
> "git branch -f master master" should have the same short-cut?
I think "git branch -M" is the only one buildbot was relying on.
As an aside, I'm not convinced the check is needed for checkout -B at
all. In an ideal world, the order of operations would be:
1. look up commit argument
2. merge working tree
3. update branch to match commit
4. update HEAD symref to point to branch
In other words, when on master, "git checkout -B master <commit>"
would be another way to say "git reset --keep <commit>", except that
it also sets up tracking.
Surprisingly, switch_branches() seems to match that pretty well already.
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
branch.c | 6 ++++--
branch.h | 3 ++-
builtin/branch.c | 2 +-
builtin/checkout.c | 15 +++++++++++----
t/t2018-checkout-branch.sh | 9 +++++----
5 files changed, 23 insertions(+), 12 deletions(-)
diff --git a/branch.c b/branch.c
index 025a97be..f85c4382 100644
--- a/branch.c
+++ b/branch.c
@@ -160,7 +160,8 @@ int validate_new_branchname(const char *name, struct strbuf *ref,
void create_branch(const char *head,
const char *name, const char *start_name,
- int force, int reflog, enum branch_track track)
+ int force, int reflog, int clobber_head,
+ enum branch_track track)
{
struct ref_lock *lock = NULL;
struct commit *commit;
@@ -175,7 +176,8 @@ void create_branch(const char *head,
explicit_tracking = 1;
if (validate_new_branchname(name, &ref, force,
- track == BRANCH_TRACK_OVERRIDE)) {
+ track == BRANCH_TRACK_OVERRIDE ||
+ clobber_head)) {
if (!force)
dont_change_ref = 1;
else
diff --git a/branch.h b/branch.h
index 1285158d..e125ff4c 100644
--- a/branch.h
+++ b/branch.h
@@ -13,7 +13,8 @@
* branch for (if any).
*/
void create_branch(const char *head, const char *name, const char *start_name,
- int force, int reflog, enum branch_track track);
+ int force, int reflog,
+ int clobber_head, enum branch_track track);
/*
* Validates that the requested branch may be created, returning the
diff --git a/builtin/branch.c b/builtin/branch.c
index 51ca6a02..730f9139 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -730,7 +730,7 @@ int cmd_branch(int argc, const char **argv, const char *prefix)
if (kinds != REF_LOCAL_BRANCH)
die(_("-a and -r options to 'git branch' do not make sense with a branch name"));
create_branch(head, argv[0], (argc == 2) ? argv[1] : head,
- force_create, reflog, track);
+ force_create, reflog, 0, track);
} else
usage_with_options(builtin_branch_usage, options);
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a807724..ca00a853 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -540,7 +540,9 @@ static void update_refs_for_switch(struct checkout_opts *opts,
else
create_branch(old->name, opts->new_branch, new->name,
opts->new_branch_force ? 1 : 0,
- opts->new_branch_log, opts->track);
+ opts->new_branch_log,
+ opts->new_branch_force ? 1 : 0,
+ opts->track);
new->name = opts->new_branch;
setup_branch_path(new);
}
@@ -565,8 +567,12 @@ static void update_refs_for_switch(struct checkout_opts *opts,
create_symref("HEAD", new->path, msg.buf);
if (!opts->quiet) {
if (old->path && !strcmp(new->path, old->path)) {
- fprintf(stderr, _("Already on '%s'\n"),
- new->name);
+ if (opts->new_branch_force)
+ fprintf(stderr, _("Reset branch '%s'\n"),
+ new->name);
+ else
+ fprintf(stderr, _("Already on '%s'\n"),
+ new->name);
} else if (opts->new_branch) {
if (opts->branch_exists)
fprintf(stderr, _("Switched to and reset branch '%s'\n"), new->name);
@@ -1057,7 +1063,8 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
struct strbuf buf = STRBUF_INIT;
opts.branch_exists = validate_new_branchname(opts.new_branch, &buf,
- !!opts.new_branch_force, 0);
+ !!opts.new_branch_force,
+ !!opts.new_branch_force);
strbuf_release(&buf);
}
diff --git a/t/t2018-checkout-branch.sh b/t/t2018-checkout-branch.sh
index 75874e85..27412623 100755
--- a/t/t2018-checkout-branch.sh
+++ b/t/t2018-checkout-branch.sh
@@ -189,12 +189,13 @@ test_expect_success 'checkout -b <describe>' '
test_cmp expect actual
'
-test_expect_success 'checkout -B to the current branch fails before merging' '
+test_expect_success 'checkout -B to the current branch works' '
git checkout branch1 &&
+ git checkout -B branch1-scratch &&
+
setup_dirty_mergeable &&
- git commit -mfooble &&
- test_must_fail git checkout -B branch1 initial &&
- test_must_fail test_dirty_mergeable
+ git checkout -B branch1-scratch initial &&
+ test_dirty_mergeable
'
test_done
--
1.7.8.rc3
^ permalink raw reply related
* [announce] gitpod -- local caching server for git
From: Sitaram Chamarty @ 2011-11-26 8:26 UTC (permalink / raw)
To: Git Mailing List
Hi all,
If you have a bandwidth constrained site with multiple local users all
cloning/fetching the same set of large repos over a slow WAN link,
this may be useful. It can be set up to be accessed via git://,
ssh://, or both.
You can get it from https://github.com/sitaramc/gitpod
--
Sitaram
^ permalink raw reply
* Re: git branch -M" regression in 1.7.7?
From: Conrad Irwin @ 2011-11-26 7:05 UTC (permalink / raw)
To: Jonathan Nieder
Cc: ☂Josh Chia (谢任中), git,
Soeren Sonnenburg
In-Reply-To: <20111126023002.GA17652@elie.hsd1.il.comcast.net>
On Fri, Nov 25, 2011 at 6:30 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> A reproduction recipe (preferrably in the form of a patch to
> t/t3200-branch.sh would be welcome.
Sent in a separate email. Feel free to add a "Tested-by:" header to
your patch if you want :).
>
> -- >8 --
> Subject: treat "git branch -M master master" as a no-op again
>
> Before v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch,
> 2011-08-20), commands like "git branch -M topic master" could be used
> even when "master" was the current branch, with the somewhat
> counterintuitive result that HEAD would point to some place new while
> the index and worktree kept the content of the old commit. This is
> not a very sensible operation and the result is what almost nobody
> would expect, so erroring out in this case was a good change.
>
> However, there is one exception to the "it's usually not obvious what
> it would mean to overwrite the current branch by another one" rule.
> Namely:
>
> git branch -M master master
>
> is clearly meant to be a no-op, even if you are on the master branch.
Agreed. I thought after reading your patch about making it just do:
if (!strcmp(oldname, newname))
exit(0);
but I guess it would then not mark an entry in the reflog that people
could be relying on...
> + clobber_head_ok = !strcmp(oldname, newname);
> +
> + validate_new_branchname(newname, &newref, force, clobber_head_ok);
This looks ok, and will be improvable if the NEEDSWORK in branch.h is done.
The other thing I wonder is whether "git checkout -B master HEAD" or
"git branch -f master master" should have the same short-cut?
Conrad
^ permalink raw reply
* Re: [PATCH] Test renaming a branch to itself
From: Jonathan Nieder @ 2011-11-26 6:59 UTC (permalink / raw)
To: Conrad Irwin
Cc: git, Soeren Sonnenburg,
☂Josh Chia (谢任中)
In-Reply-To: <1322290364-16207-1-git-send-email-conrad.irwin@gmail.com>
Conrad Irwin wrote:
> Test for a regression introduced in v1.7.7-rc2~1^2~2.
Thanks! I take it that means you like the patch. :)
The tests look sane fwiw (and it looks like the tests you wrote before
cover the "branch -M" safety valve pretty well).
^ permalink raw reply
* [PATCH] Test renaming a branch to itself
From: Conrad Irwin @ 2011-11-26 6:52 UTC (permalink / raw)
To: Jonathan Nieder
Cc: git, Soeren Sonnenburg,
☂Josh Chia (谢任中), Conrad Irwin
In-Reply-To: <20111126023002.GA17652@elie.hsd1.il.comcast.net>
Test for a regression introduced in v1.7.7-rc2~1^2~2.
Requested-by: Jonathan Nieder <jrnieder@gmail.com>
Signed-off-by: Conrad Irwin <conrad.irwin@gmail.com>
---
t/t3200-branch.sh | 16 ++++++++++++++++
1 files changed, 16 insertions(+), 0 deletions(-)
diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index bc73c20..7690332 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -115,6 +115,22 @@ test_expect_success 'git branch -M baz bam should succeed when baz is checked ou
git branch -M baz bam
'
+test_expect_success 'git branch -M master should work when master is checked out' '
+ git checkout master &&
+ git branch -M master
+'
+
+test_expect_success 'git branch -M master master should work when master is checked out' '
+ git checkout master &&
+ git branch -M master master
+'
+
+test_expect_success 'git branch -M master2 master2 should work when master is checked out' '
+ git checkout master &&
+ git branch master2 &&
+ git branch -M master2 master2
+'
+
test_expect_success 'git branch -v -d t should work' '
git branch t &&
test_path_is_file .git/refs/heads/t &&
--
1.7.7.1.433.ga2d04a
^ permalink raw reply related
* Re: git branch -M" regression in 1.7.7?
From: Jonathan Nieder @ 2011-11-26 2:30 UTC (permalink / raw)
To: ☂Josh Chia (谢任中)
Cc: git, Soeren Sonnenburg, Conrad Irwin
In-Reply-To: <CALxtSbRbwkVDKJcXiKY9rHYCjA3XGgCytbXQnRhQvbEnY8SpjA@mail.gmail.com>
Hi,
Josh Chia (谢任中) wrote:
> On git 1.7.7.3, when I try to "git branch -M master" when I'm already
> on a branch 'master', I get this error message:
> Cannot force update the current branch
>
> On 1.7.6.4, the command succeeds.
>
> Is this intended?
Yes, but it probably wasn't a good idea. How about this patch?
A reproduction recipe (preferrably in the form of a patch to
t/t3200-branch.sh would be welcome.
-- >8 --
Subject: treat "git branch -M master master" as a no-op again
Before v1.7.7-rc2~1^2~2 (Prevent force-updating of the current branch,
2011-08-20), commands like "git branch -M topic master" could be used
even when "master" was the current branch, with the somewhat
counterintuitive result that HEAD would point to some place new while
the index and worktree kept the content of the old commit. This is
not a very sensible operation and the result is what almost nobody
would expect, so erroring out in this case was a good change.
However, there is one exception to the "it's usually not obvious what
it would mean to overwrite the current branch by another one" rule.
Namely:
git branch -M master master
is clearly meant to be a no-op, even if you are on the master branch.
And in the latter case, it can be abbreviated:
git branch -M master
This seems like a valuable exception to allow, because then "git
branch -M foo" would _always_ be allowed --- either 'foo' is not the
current branch, and it does the obvious thing, or 'foo' is the current
branch, and nothing happens.
Buildbot uses this idiom and was broken in 1.7.7 (it would emit the
message "Cannot force update the current branch").
Reported-by: Soeren Sonnenburg <sonne@debian.org>
Reported-by: Josh Chia (谢任中)
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
builtin/branch.c | 9 ++++++++-
1 files changed, 8 insertions(+), 1 deletions(-)
diff --git a/builtin/branch.c b/builtin/branch.c
index 51ca6a02..24f33b24 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -568,6 +568,7 @@ static void rename_branch(const char *oldname, const char *newname, int force)
unsigned char sha1[20];
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
+ int clobber_head_ok;
if (!oldname)
die(_("cannot rename the current branch while not on any."));
@@ -583,7 +584,13 @@ static void rename_branch(const char *oldname, const char *newname, int force)
die(_("Invalid branch name: '%s'"), oldname);
}
- validate_new_branchname(newname, &newref, force, 0);
+ /*
+ * A command like "git branch -M currentbranch currentbranch" cannot
+ * cause the worktree to become inconsistent with HEAD, so allow it.
+ */
+ clobber_head_ok = !strcmp(oldname, newname);
+
+ validate_new_branchname(newname, &newref, force, clobber_head_ok);
strbuf_addf(&logmsg, "Branch: renamed %s to %s",
oldref.buf, newref.buf);
--
1.7.8.rc3
^ permalink raw reply related
* git branch -M" regression in 1.7.7?
From: ☂Josh Chia (谢任中) @ 2011-11-26 0:36 UTC (permalink / raw)
To: git
On git 1.7.7.3, when I try to "git branch -M master" when I'm already
on a branch 'master', I get this error message:
Cannot force update the current branch
On 1.7.6.4, the command succeeds.
Is this intended?
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Carlos Martín Nieto @ 2011-11-25 17:02 UTC (permalink / raw)
To: Henrik Grubbström; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <Pine.GSO.4.63.1111251705330.22588@shipon.roxen.com>
[-- Attachment #1: Type: text/plain, Size: 2949 bytes --]
On Fri, Nov 25, 2011 at 05:14:17PM +0100, Henrik Grubbström wrote:
> On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:
>
> >This patch fixes this, but I think it would still break if the LF is
> >at the end of the file. Changing the `if (!input)` to put the LF in
> >the output buffer may or may not be the right soulution. I feel like
> >this should be handled by cascade_filter_fn rather than the actual
> >filter somehow, but Junio's comment (4ae66704 'stream filter: add "no
> >more input" to the filters') suggests otherwise.
> >
> >I'm working on a cleaner patch that takes care of a bit of state, but
> >this is the general idea.
>
> Looks good to me (and seems to work in my case).
That patch would give wrong output if the same happened at the end of
a file. The attached patch should also cover this case.
> Typo in the commit subject though.
>
> > cmn
> >--- 8< ---
> >Subject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming
> ^^^^^^^^^^^
> This should be either "infinitely", or "indefinitely", but since we
> know that the loop won't terminate "infinitely" is to be preferred.
Thanks for noticing. I went with a different title in the end. Junio,
could you consider this one for inclusion in the next RC?
--- 8< ---
Subject: [PATCH] convert: track state in LF-to-CRLF filter
There may not be enough space to store CRLF in the output. If we don't
fill the buffer, then the filter will keep getting called with the same
short buffer and will loop forever.
Instead, always store the CR and record there's a missing LF if
necessary it so we store it in the output buffer the next time the
function gets called.
Reported-by: Henrik Grubbström <grubba@roxen.com>
Signed-off-by: Carlos Martín Nieto <cmn@elego.de>
---
convert.c | 23 ++++++++++++++++-------
1 files changed, 16 insertions(+), 7 deletions(-)
diff --git a/convert.c b/convert.c
index 86e9c29..c050b86 100644
--- a/convert.c
+++ b/convert.c
@@ -880,20 +880,29 @@ static int lf_to_crlf_filter_fn(struct stream_filter *filter,
const char *input, size_t *isize_p,
char *output, size_t *osize_p)
{
- size_t count;
+ size_t count, o = 0;
+ static int want_lf = 0;
+
+ /* Output a pending LF if we need to */
+ if (want_lf) {
+ output[o++] = '\n';
+ want_lf = 0;
+ }
if (!input)
- return 0; /* we do not keep any states */
+ return 0; /* We've already dealt with the state */
+
count = *isize_p;
if (count) {
- size_t i, o;
- for (i = o = 0; o < *osize_p && i < count; i++) {
+ size_t i;
+ for (i = 0; o < *osize_p && i < count; i++) {
char ch = input[i];
if (ch == '\n') {
- if (o + 1 < *osize_p)
- output[o++] = '\r';
- else
+ output[o++] = '\r';
+ if (o >= *osize_p) {
+ want_lf = 1;
break;
+ }
}
output[o++] = ch;
}
--
1.7.8.rc3.31.g017d1
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply related
* Re: [PATCH] grep: load funcname patterns for -W
From: René Scharfe @ 2011-11-25 16:32 UTC (permalink / raw)
To: Thomas Rast; +Cc: git, Junio C Hamano
In-Reply-To: <5e3bcf651b31b299ca411296e6e7c4d11f6ae617.1322232319.git.trast@student.ethz.ch>
Am 25.11.2011 15:46, schrieb Thomas Rast:
> git-grep avoids loading the funcname patterns unless they are needed.
> ba8ea74 (grep: add option to show whole function as context,
> 2011-08-01) forgot to extend this test also to the new funcbody
> feature. Do so.
>
> The catch is that we also have to disable threading when using
> userdiff, as explained in grep_threads_ok(). So we must be careful to
> introduce the same test there.
Oops. Thanks for catching this.
That reminds me to look into adding a thread-safe way to access
attributes again..
Thanks,
René
^ permalink raw reply
* Re: what are the chances of a 'pre-upload' hook?
From: Sitaram Chamarty @ 2011-11-25 16:18 UTC (permalink / raw)
To: Andreas Ericsson; +Cc: Git Mailing List
In-Reply-To: <4ECF939D.8020706@op5.se>
On Fri, Nov 25, 2011 at 6:39 PM, Andreas Ericsson <ae@op5.se> wrote:
> People who fetch but don't push is, by far, the vast majority of git users.
> Think of everyone fetching from any public software repository without
> having write access to it. If you think of git.git and linux.git alone
> I think it's safe to assume the number of "fetch-no-push" outnumber the
> "push-and-whatnot" group by some quarter million to one.
But in those environments the person pulling does not even have an ID
so how is he at risk with the hook running?
>> I may be wrong but I imagine shared environments are those where
>> almost everyone will push at least once in a while. It's a closed
>> group of people, probably all developers, etc etc etc...
>>
>
> Not really. We fetch from each other quite a lot at work, and from
> each others semi-public repositories on a shared server where we've
> all got accounts (ie, write access), but we very, very rarely push
> into each others repositories. The sharepoint is the "official" repo
> on the repo-server, which the buildbots gets its code from and where
> everything to be released, maintained or handled in some way in the
> future resides.
Yes, and this is the only situation where it does have the issue. I'm
just not sure how common this is.
It's fine if you tell me I'm wrong and that this *is* still very
common. I'll back off.
But everyone seems to be bringing in github and public repos as part
of the argument, and I don't see how they're relevant to the original
security issue of the guy who pulls having his account compromised.
> Anyways. Shooting down the arguments *against* pre-upload hooks are
> quite silly if it's not combined with some fresh arguments *for* such
> a hook.
>
> So... What usecase do you envision where you'd need one?
I'm writing a caching proxy that helps with bandwidth issues when too
many people in a bad-WAN site want to clone some huge repo from its
canonical site. The only one I found by googling fiddles with the git
protocol itself, and I hate doing things like that.
Ignoring all the details, the pre-upload hook would have checked some
conditions and fired off a fetch from the remote site if those checks
passed.
It's easy enough to do it from cron but it would have been more
elegant this way.
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Henrik Grubbström @ 2011-11-25 16:14 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <20111125153829.GB10417@beez.lab.cmartin.tk>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 1007 bytes --]
On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:
> This patch fixes this, but I think it would still break if the LF is
> at the end of the file. Changing the `if (!input)` to put the LF in
> the output buffer may or may not be the right soulution. I feel like
> this should be handled by cascade_filter_fn rather than the actual
> filter somehow, but Junio's comment (4ae66704 'stream filter: add "no
> more input" to the filters') suggests otherwise.
>
> I'm working on a cleaner patch that takes care of a bit of state, but
> this is the general idea.
Looks good to me (and seems to work in my case).
Typo in the commit subject though.
> cmn
> --- 8< ---
> Subject: [PATCH] convert: don't loop indefintely if at LF-to-CRLF streaming
^^^^^^^^^^^
This should be either "infinitely", or "indefinitely", but since we know
that the loop won't terminate "infinitely" is to be preferred.
Thanks,
--
Henrik Grubbström grubba@roxen.com
Roxen Internet Software AB
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Henrik Grubbström @ 2011-11-25 15:59 UTC (permalink / raw)
To: Carlos Martín Nieto; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <20111125155301.GC10417@beez.lab.cmartin.tk>
[-- Attachment #1: Type: TEXT/PLAIN, Size: 692 bytes --]
On Fri, 25 Nov 2011, Carlos Martín Nieto wrote:
> On Fri, Nov 25, 2011 at 04:43:41PM +0100, Henrik Grubbström wrote:
>>
>> The bug is probably that lf_to_crlf_filter_fn() should return
>> non-zero in this case (ie o and/or i being zero).
>
> non-zero? That would cause the filter to abort, which definitely not
> what we want. Have you seen my other e-mails regarding this? I'm
> trying to figure out which is the best way to go about this. The
> solution is to keep track of the fact that we're missing a LF in the
> output buffer.
True, I misread the code.
Keeping track of the filter state is the way to go.
> cmn
--
Henrik Grubbström grubba@roxen.com
Roxen Internet Software AB
^ permalink raw reply
* Re: Infinite loop in cascade_filter_fn()
From: Carlos Martín Nieto @ 2011-11-25 15:53 UTC (permalink / raw)
To: Henrik Grubbström; +Cc: Git Mailing list, Junio C Hamano
In-Reply-To: <Pine.GSO.4.63.1111251629500.22588@shipon.roxen.com>
[-- Attachment #1: Type: text/plain, Size: 1277 bytes --]
On Fri, Nov 25, 2011 at 04:43:41PM +0100, Henrik Grubbström wrote:
> On Wed, 23 Nov 2011, Henrik Grubbström wrote:
>
> >Hi.
> >
> >My git repository walker just got bitten by what seems to be a
> >reasonably new bug in convert.c:cascade_filter_fn() (git 1.7.8.rc3
> >(gentoo)).
>
> After some tracing, the problem is triggered by the variable "remaining"
> being set to 1 in the beginning of the cascade_filter_fn() loop,
> which causes filter "two" to be called with an output buffer size of
> 1.
> Filter "two" in this case is lf_to_crlf_filter_fn(), and the next
> input character is a "\n". lf_to_crlf_filter_fn() wants to convert
> this to "\r\n", but that doesn't fit into the buffer, so it breaks
> out and returns zero. Upon seing the zero cascade_filter_fn() thinks
> all is well, even though nothing has happened, and loops.
>
> The bug is probably that lf_to_crlf_filter_fn() should return
> non-zero in this case (ie o and/or i being zero).
non-zero? That would cause the filter to abort, which definitely not
what we want. Have you seen my other e-mails regarding this? I'm
trying to figure out which is the best way to go about this. The
solution is to keep track of the fact that we're missing a LF in the
output buffer.
cmn
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ 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