* Re: [PATCHv3 10/13] credentials: add "cache" helper
From: Jeff King @ 2012-01-10 17:53 UTC (permalink / raw)
To: Jonathan Nieder; +Cc: Junio C Hamano, git
In-Reply-To: <20120110174420.GA22184@burratino>
On Tue, Jan 10, 2012 at 11:44:21AM -0600, Jonathan Nieder wrote:
> Jeff King wrote:
>
> > 2. A hack in credential-cache won't help any unix-socket
> > users who come along later.
> >
> > 3. The chdir trickery isn't that likely to fail (basically
> > it's only a problem if your cwd is missing or goes away
> > while you're running). And because we only enable the
> > hack when we get a too-long name, it can only fail in
> > cases that would have failed under the previous code
> > anyway.
> >
> > Signed-off-by: Jeff King <peff@peff.net>
>
> A part of me worries that this assumption (3), and the additional
> assumption that nothing running concurrently cares about the cwd,
> might come back to bite us when the future (2) arrives. But I don't
> see a better approach.
The problem is that when future (2) arrives, a credential-cache specific
hack won't be helping it at all. :)
To be honest, I don't really expect a lot of future unix-socket users.
It's not portable, and most of our IPC happens over pipes. The design of
the cache daemon is very specific in requiring a true
many-clients-to-one-server model, but also caring deeply about access
controls (making TCP sockets less good[1]).
[1] One could in theory do a loopback TCP socket and provide a random
token read from an access-controlled file. But that ends up a lot
more complicated and introduces new attack surfaces. Which is a bad
thing for security-sensitive code like this.
> Follow-on ideas: on platforms that support it, it would be nice to use
>
> ctx->orig_dirfd = open(".", O_RDONLY);
> if (ctx->orig_dirfd < 0)
> ... error handling ...
> ...
> if (ctx->orig_dirfd >= 0) {
> if (fchdir(ctx->orig_dirfd))
> die_errno("unable to restore original working directory");
> close(ctx->orig_dirfd);
> }
Yeah, I started by using fchdir, but noticed that we don't use it
anywhere else and didn't want to create a portability problem. It does
fix the "somebody deleted your cwd while you were gone from it" problem.
But if you have no cwd at all, the open call will still fail.
Still, it would be slightly more robust. I wonder how portable fchdir
is in practice (I guess we could always fall back to the getcwd code
path). Do you want to prepare a patch on top?
> [...]
> > + dir = path;
> > + dir_len = slash - path;
> [...]
> > + if (chdir_len(dir, dir_len) < 0)
> > + return -1;
>
> Could avoid some complication by eliminating the dir_len var.
>
> if (chdir_len(dir, slash - dir))
> return -1;
Ah, yeah. Originally I had written it so that "slash" didn't survive
untouched to there, but in the current code, that would work.
Junio, if you haven't merged it to next yet, it might be worth squashing
in the patch below. Otherwise I wouldn't worry about it.
> Neither seems worth holding up the patch, so
> Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for the review (and for, as usual, a well-written bug report).
---
diff --git a/unix-socket.c b/unix-socket.c
index 91ed9dc..e8f19c6 100644
--- a/unix-socket.c
+++ b/unix-socket.c
@@ -43,7 +43,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
if (size > sizeof(sa->sun_path)) {
const char *slash = find_last_dir_sep(path);
const char *dir;
- int dir_len;
if (!slash) {
errno = ENAMETOOLONG;
@@ -51,8 +50,6 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
}
dir = path;
- dir_len = slash - path;
-
path = slash + 1;
size = strlen(path) + 1;
if (size > sizeof(sa->sun_path)) {
@@ -64,7 +61,7 @@ static int unix_sockaddr_init(struct sockaddr_un *sa, const char *path,
errno = ENAMETOOLONG;
return -1;
}
- if (chdir_len(dir, dir_len) < 0)
+ if (chdir_len(dir, slash - dir) < 0)
return -1;
}
^ permalink raw reply related
* [PATCH] attr: don't confuse prefixes with leading directories
From: Jeff King @ 2012-01-10 18:08 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git, Henrik Grubbström, git-dev
In-Reply-To: <20120110171100.GA18962@sigill.intra.peff.net>
When we prepare the attribute stack for a lookup on a path,
we start with the cached stack from the previous lookup
(because it is common to do several lookups in the same
directory hierarchy). So the first thing we must do in
preparing the stack is to pop any entries that point to
directories we are no longer interested in.
For example, if our stack contains gitattributes for:
foo/bar/baz
foo/bar
foo
but we want to do a lookup in "foo/bar/bleep", then we want
to pop the top element, but retain the others.
To do this we walk down the stack from the top, popping
elements that do not match our lookup directory. However,
the test do this simply checked strncmp, meaning we would
mistake "foo/bar/baz" as a leading directory of
"foo/bar/baz_plus". We must also check that the character
after our match is '/', meaning we matched the whole path
component.
There are two special cases to consider:
1. The top of our attr stack has the empty path. So we
must not check for '/', but rather special-case the
empty path, which always matches.
2. Typically when matching paths in this way, you would
also need to check for a full string match (i.e., the
character after is '\0'). We don't need to do so in
this case, though, because our path string is actually
just the directory component of the path to a file
(i.e., we know that it terminates with "/", because the
filename comes after that).
Helped-by: Michael Haggerty <mhagger@alum.mit.edu>
Signed-off-by: Jeff King <peff@peff.net>
---
I wrote it in the minimally-intrusive way, but you could also pull
the "!namelen" case before the strncmp (since a strncmp of size 0 is
always true, anyway). I don't know if it would be more obvious that way.
I prepared this on top of master, but the patch applies (with some
shifted line counts) on older releases, too.
attr.c | 3 ++-
t/t0003-attributes.sh | 10 ++++++++++
2 files changed, 12 insertions(+), 1 deletions(-)
diff --git a/attr.c b/attr.c
index 76b079f..fa975da 100644
--- a/attr.c
+++ b/attr.c
@@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)
elem = attr_stack;
if (namelen <= dirlen &&
- !strncmp(elem->origin, path, namelen))
+ !strncmp(elem->origin, path, namelen) &&
+ (!namelen || path[namelen] == '/'))
break;
debug_pop(elem);
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index dbb2623..51f3045 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -159,6 +159,16 @@ test_expect_success 'relative paths' '
(cd b && attr_check ../a/b/g a/b/g)
'
+test_expect_success 'prefixes are not confused with leading directories' '
+ attr_check a_plus/g unspecified &&
+ cat >expect <<-\EOF &&
+ a/g: test: a/g
+ a_plus/g: test: unspecified
+ EOF
+ git check-attr test a/g a_plus/g >actual &&
+ test_cmp expect actual
+'
+
test_expect_success 'core.attributesfile' '
attr_check global unspecified &&
git config core.attributesfile "$HOME/global-gitattributes" &&
--
1.7.9.rc0.33.gd3c17
^ permalink raw reply related
* Re: [PATCH] attr: don't confuse prefixes with leading directories
From: Jeff King @ 2012-01-10 18:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Michael Haggerty, git, Henrik Grubbström, git-dev
In-Reply-To: <20120110180820.GA15273@sigill.intra.peff.net>
On Tue, Jan 10, 2012 at 01:08:21PM -0500, Jeff King wrote:
> diff --git a/attr.c b/attr.c
> index 76b079f..fa975da 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)
>
> elem = attr_stack;
> if (namelen <= dirlen &&
> - !strncmp(elem->origin, path, namelen))
> + !strncmp(elem->origin, path, namelen) &&
> + (!namelen || path[namelen] == '/'))
> break;
Side note. One thing that confused me about this code is that
prepare_attr_stack does a popping loop like this:
while (attr_stack && attr_stack->origin) {
if (/* attr_stack->origin is a prefix */)
break;
/* otherwise, pop */
elem = attr_stack;
attr_stack = elem->prev;
free(elem);
}
/* now push our new ones */
...
len = strlen(attr_stack->origin);
IOW, our loop breaks out when attr_stack is NULL, but then we go on to
assume that attr_stack is _not_ NULL. This isn't a bug, because it turns
out that we always leave something in the attr_stack: the root
gitattributes file (and the builtins). But it is slightly confusing to
a reader because of the useless loop condition.
I'm not sure if the right solution is to change the popping loop to:
/* we will never run out of stack, because we always have the root */
while (attr_stack->origin) {
...
Or to be extra defensive and put:
if (!attr_stack)
die("BUG: we ran out of attr stack!?");
after the loop, or to somehow handle the case of an empty attr stack
below (which is hard to do, because it can't be triggered, so I have no
idea what it would mean).
-Peff
^ permalink raw reply
* Re: [PATCH 1/8] revert: prepare to move replay_action to header
From: Jonathan Nieder @ 2012-01-10 18:27 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326212039-13806-2-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Later in the series, we will build a generalized sequencer by
> factoring code out of the revert builtin, and leaving it with just
> argument parsing work. This involves moving "replay_action" to
> sequencer.h, so that both sequencer.c and builtin/revert.c can use it.
> Unfortunately, "REVERT" and "CHERRY_PICK" are unsuitable variable
> names, as their purpose is unclear in the global context; in
> anticipation of the move, rename them to "REPLAY_REVERT" and
> "REPLAY_PICK" respectively.
This could be pruned to just the final bit. Something like:
REVERT and CHERRY_PICK and are unsuitable names for an enumerator in
a public interface, because they are generic enough to be likely to
clash with identifiers with other meanings. Rename to REPLAY_REVERT
and REPLAY_PICK as preparation for exposing them.
The patch proper looks safe and the effect positive to me, so I would
be all for applying it as long as the commit message is cleaned up.
^ permalink raw reply
* Re: git grep doesn't follow symbolic link
From: Junio C Hamano @ 2012-01-10 18:22 UTC (permalink / raw)
To: Thomas Rast; +Cc: Ramkumar Ramachandra, Bertrand BENOIT, git
In-Reply-To: <877h0zlvwd.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
>> I'd imagine so: symbolic links are not portable across different file
>> systems; Git's internal representation of a symbolic link is a file
>> containing the path of the file to be linked to.
>
> I'd actually welcome a fix to this general area,...
Even though some platforms may lack symbolic links, where they are
supported, they have a clear and defined meaning and that is what Git
tracks as contents: where the link points at.
So we would want our "git diff" to tell us, even if you moved without
content modification the symbolic link target that lives somewhere on your
filesystem but is outside the control of Git, and updated a symbolic link
that is tracked by Git to point to a new location, that you updated the
link. On the other hand, if you did not update a tracked symbolic link,
even if the location the link points at that may or may not be under the
control of Git, we do not want "git diff" to show anything. As far as that
link is concerned, nothing has changed.
Changing this would not be a fix; it would be butchering.
Having said that...
> But I lose all the cute features of git-diff. I *could* say
>
> git diff --no-index <(ls) <(cd elsewhere && ls)
... "--no-index" is specifically _not_ about tracked contents of Git, but
was bolted on as a poor-man's substitute for GNU diff (think of it as
somebody wanted to have the nifty "git diff" features like renames and
coloring, but did not want to bother to port them to GNU diff codebase,
but instead hacked Git codebase to work outside Git tracked contents). In
that context, I would agree that it _might_ make sense to treat special
files and symbolic links in a way that is different from how tracked
contents are handled.
^ permalink raw reply
* Re: Please support add -p with a new file, to add only part of the file
From: Junio C Hamano @ 2012-01-10 18:32 UTC (permalink / raw)
To: Thomas Rast
Cc: Jonathan Nieder, Josh Triplett, git, Wincent Colaiuta, Jeff King
In-Reply-To: <87ty43fy7f.fsf@thomas.inf.ethz.ch>
Thomas Rast <trast@student.ethz.ch> writes:
> A not-so-proper solution might of course start by looking at which files
> are untracked, and only run the 'git add -N' immediately before patch
> application.
Isn't the real issue that we mistakenly gave an impression that "add -p"
is the primary interface to the users, and forgot to tell them about the
more general "add -i", which "add -p" is a small subset of?
Even if you start with "add -N", there won't be individual "hunks" you can
pick and choose from diffing emptiness and the whole new file, so you end
up using "edit hunk" interface.
Whatever you do, you should aim to make it easier to use than
$ cp newfile.c newfile.c-saved
$ edit newfile.c ;# delete all the things you do not want for now
$ git add newfile.c
$ mv newfile.c-saved newfile.c
Personally, I think it is a very tall order to tweak "add -p / edit"
interface to make it easier to use than the above four command sequence,
the second one among which is the most time and braincell consuming part
of the process, especially when "add -p / edit" gives you has to be
unnecessarily distracting by having '+' prefix.
^ permalink raw reply
* Re: [PATCH 2/8] revert: decouple sequencer actions from builtin commands
From: Jonathan Nieder @ 2012-01-10 18:38 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326212039-13806-3-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> Currently, 'git cherry-pick' fills up the '.git/sequencer/todo'
> instruction sheet with "pick" actions, while 'git revert' fills it up
> with "revert" actions. Inspired by the way 'rebase -i' works, we
> would like to permit mixing arbitrary actions in the same instruction
> sheet.
Ok.
> To do this, we first have to decouple the notion of an action
> in the instruction sheet from builtin commands.
Wha?
I guess at this point I get lost because you are using jargon. What
is the difference between an action in the instruction sheet and a
builtin command? How does being a busybox-style hard link to the main
"git" binary as opposed to a separate binary (which is what "builtin"
means) have anything to do with this at all?
And I don't have any sense of the impact. Will this change the
behavior of "git cherry-pick"? Will it avoid some confusion on the
part of future people modifying the code? Is it a necessary step
before some future change we want? Does it just make the code
prettier, and help the sanity of future readers in the process (which
is definitely valuable, too)? The above doesn't make it clear to me.
> --- a/builtin/revert.c
> +++ b/builtin/revert.c
[...]
> @@ -52,7 +57,7 @@ enum replay_subcommand {
> };
>
> struct replay_opts {
> - enum replay_action action;
> + enum replay_command command;
> enum replay_subcommand subcommand;
This part seems to be renaming "action" to "command". A cosmetic
change, which I don't have an immediate opinion about either way.
[...]
> -static const char *action_name(const struct replay_opts *opts)
> +static const char *command_name(struct replay_opts *opts)
This part does a similar renaming, and drops a const while at it for
no intelligible reason.
[...]
> @@ -142,7 +147,7 @@ static void verify_opt_mutually_compatible(const char *me, ...)
> static void parse_args(int argc, const char **argv, struct replay_opts *opts)
> {
> const char * const * usage_str = revert_or_cherry_pick_usage(opts);
> - const char *me = action_name(opts);
> + const char *me = command_name(opts);
The rest is stuff like this, which follows from the first part.
Stepping back, I think the idea is that "enum replay_action" is not a
good way to identify the command name in error messages like
fatal: cherry-pick: --abort cannot be used with --continue
So you introduce a _new_ enum to represent the command name. Why not
just use a string, so commands using the nice and generic sequencer
library do not have to register themselves in a global callers list to
use it?
Plus the renaming of the ->action field and action_name() function
feel gratuitous, and drowned out the actual changes in the noise.
Does that clarify?
I guess I agree with the problem you are solving, but I don't agree
with the solution at all.
Thanks,
Jonathan
^ permalink raw reply
* Re: Please support add -p with a new file, to add only part of the file
From: Jeff King @ 2012-01-10 18:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Jonathan Nieder, Josh Triplett, git,
Wincent Colaiuta
In-Reply-To: <7vpqer9znv.fsf@alter.siamese.dyndns.org>
On Tue, Jan 10, 2012 at 10:32:20AM -0800, Junio C Hamano wrote:
> Thomas Rast <trast@student.ethz.ch> writes:
>
> > A not-so-proper solution might of course start by looking at which files
> > are untracked, and only run the 'git add -N' immediately before patch
> > application.
>
> Isn't the real issue that we mistakenly gave an impression that "add -p"
> is the primary interface to the users, and forgot to tell them about the
> more general "add -i", which "add -p" is a small subset of?
Maybe it is just me, but I find "add -p" insanely useful, and the rest
of "add -i" to be worthless clutter. The "add -p" functionality is not
easily available anywhere else, but the rest of "add -i" can be easily
(and often more efficiently) mimicked using existing git commands.
> Even if you start with "add -N", there won't be individual "hunks" you can
> pick and choose from diffing emptiness and the whole new file, so you end
> up using "edit hunk" interface.
I don't think the main impetus for this is that people necessarily want
to pick and choose hunks from added files. I think it is simply a nice
workflow to do:
$ hack hack hack
$ git add -p ;# inspect and add changes
$ git commit
which is very similar to the traditional git workflow:
$ hack hack hack
$ git diff ;# inspect changes
$ git add foo ;# add changes
$ git commit
I find myself using "add -p" almost exclusively these days, as it
combines the two middle steps (and even though I usually am just hitting
"y" after inspection, when I _do_ want to make a change, I am right
there in the hunk selection loop already).
-Peff
^ permalink raw reply
* Re: [PATCH] gitignore: warn about pointless syntax
From: Jeff King @ 2012-01-10 18:51 UTC (permalink / raw)
To: Jan Engelhardt; +Cc: Junio C Hamano, git, trast
In-Reply-To: <alpine.LNX.2.01.1201100639340.11534@frira.zrqbmnf.qr>
On Tue, Jan 10, 2012 at 06:42:11AM +0100, Jan Engelhardt wrote:
> >You only have to implement proper backslash decoding, so I think it is
> >not as hard as reimplementing fnmatch:
> >[...]
> >
> >That being said, if this is such a commonly-requested feature
>
> Was it actually requested, or did you mean "commonly attempted use"?
Both. I meant in my sentence "if this is such a big problem that we need
to add a check for it, then surely it is something people would like to
be using". But if you peruse the list archives, you can find several
people mentioning that they would like it.
> As I see it, foo/**/*.o for example is equal to placing "*.o" in
> foo/.gitignore, so the feature is already implemented, just not
> through the syntax people falsely assume it is. And that is the
> reason for wanting to output a warning. If it was me, I'd even make
> it use error(), because that is the only way to educate people (and
> it works), but alas, some on the list might consider that too harsh.
Those features aren't exactly equivalent. Off the top of my head, I can
think of a few reasons to prefer using the top-level:
- you simply prefer it because it keeps your rules grouped in a more
logical way
- you don't control the sub-tree (e.g., it is brought in by sub-tree
merge, or you have an agreement with other devs not to touch things
in it. Also, I don't think .gitignores cross submodule boundaries
currently, but it is something that could happen eventually).
- you can write more complex rules with "**" that would otherwise
necessitate writing multiple rules split across directories
Don't get me wrong. I am not a huge proponent of "**", and I could
really care less if we implement it or not, and we have survived many
years without it. It just seems to me that if it's worth warning about,
it's worth implementing.
-Peff
^ permalink raw reply
* Re: [PATCH 4/8] revert: separate out parse errors logically
From: Jonathan Nieder @ 2012-01-10 19:03 UTC (permalink / raw)
To: Ramkumar Ramachandra; +Cc: Git List, Junio C Hamano
In-Reply-To: <1326212039-13806-5-git-send-email-artagnon@gmail.com>
Ramkumar Ramachandra wrote:
> For the first kind of error, it is insufficient to
> check if the buffer beings with a "pick" or "revert", otherwise the
> following insn sheet would be interpreted as having a malformed object
> name:
> pickle a1fe57~2
>
> In reality, the issue is that "pickle" is an unrecognized instruction.
> So, check that the buffer starts with ("pick " or "pick\t") and
> ("revert " or "revert\t").
Sorry, the above description just leaves me more confused than before.
What _actual impact_ does this patch have? And why do we want it?
And what could be the bad side effects? Everything else is just
irrelevant.
Before reading the above description, I thought this was just a code
cleanup. So either the description or my reading is completely
confused.
^ permalink raw reply
* Re: [PATCH] attr: don't confuse prefixes with leading directories
From: Junio C Hamano @ 2012-01-10 19:23 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, git, Henrik Grubbström, git-dev
In-Reply-To: <20120110182140.GB15273@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> IOW, our loop breaks out when attr_stack is NULL, but then we go on to
> assume that attr_stack is _not_ NULL. This isn't a bug, because it turns
> out that we always leave something in the attr_stack: the root
> gitattributes file (and the builtins). But it is slightly confusing to
> a reader because of the useless loop condition.
>
> I'm not sure if the right solution is to change the popping loop to:
>
> /* we will never run out of stack, because we always have the root */
> while (attr_stack->origin) {
> ...
Yeah, that makes sense, as that existing check "attr_stack &&" was a
misguided defensive coding, that was _not_ defensive at all as we didn't
do anything after we stop iterating from that loop and without checking
dereferenced attr_stack->origin, which was a simple bogosity.
>
> Or to be extra defensive and put:
>
> if (!attr_stack)
> die("BUG: we ran out of attr stack!?");
>
> after the loop, or to somehow handle the case of an empty attr stack
> below (which is hard to do, because it can't be triggered, so I have no
> idea what it would mean).
And this is even more so.
^ permalink raw reply
* Re: [PATCH] attr: don't confuse prefixes with leading directories
From: Junio C Hamano @ 2012-01-10 19:25 UTC (permalink / raw)
To: Jeff King; +Cc: Michael Haggerty, git, Henrik Grubbström, git-dev
In-Reply-To: <20120110180820.GA15273@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> diff --git a/attr.c b/attr.c
> index 76b079f..fa975da 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -582,7 +582,8 @@ static void prepare_attr_stack(const char *path)
>
> elem = attr_stack;
> if (namelen <= dirlen &&
> - !strncmp(elem->origin, path, namelen))
> + !strncmp(elem->origin, path, namelen) &&
> + (!namelen || path[namelen] == '/'))
> break;
Thanks for the fix; I was looking at path_matches() and wondering about
the same thing.
^ permalink raw reply
* Re: [PATCH] attr: don't confuse prefixes with leading directories
From: Jeff King @ 2012-01-10 19:28 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7vlipf9xbe.fsf@alter.siamese.dyndns.org>
On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
> > I'm not sure if the right solution is to change the popping loop to:
> >
> > /* we will never run out of stack, because we always have the root */
> > while (attr_stack->origin) {
> > ...
>
> Yeah, that makes sense, as that existing check "attr_stack &&" was a
> misguided defensive coding, that was _not_ defensive at all as we didn't
> do anything after we stop iterating from that loop and without checking
> dereferenced attr_stack->origin, which was a simple bogosity.
>
> >
> > Or to be extra defensive and put:
> >
> > if (!attr_stack)
> > die("BUG: we ran out of attr stack!?");
> >
> > after the loop, or to somehow handle the case of an empty attr stack
> > below (which is hard to do, because it can't be triggered, so I have no
> > idea what it would mean).
>
> And this is even more so.
I wasn't clear: the second one is "even more so" making sense, or "even
more so" misguided defensive coding?
-Peff
^ permalink raw reply
* Re: [PATCH] attr: don't confuse prefixes with leading directories
From: Jeff King @ 2012-01-10 19:32 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20120110192810.GA16018@sigill.intra.peff.net>
On Tue, Jan 10, 2012 at 02:28:10PM -0500, Jeff King wrote:
> On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
>
> > > I'm not sure if the right solution is to change the popping loop to:
> > >
> > > /* we will never run out of stack, because we always have the root */
> > > while (attr_stack->origin) {
> > > ...
> >
> > Yeah, that makes sense, as that existing check "attr_stack &&" was a
> > misguided defensive coding, that was _not_ defensive at all as we didn't
> > do anything after we stop iterating from that loop and without checking
> > dereferenced attr_stack->origin, which was a simple bogosity.
> >
> > >
> > > Or to be extra defensive and put:
> > >
> > > if (!attr_stack)
> > > die("BUG: we ran out of attr stack!?");
> > >
> > > after the loop, or to somehow handle the case of an empty attr stack
> > > below (which is hard to do, because it can't be triggered, so I have no
> > > idea what it would mean).
> >
> > And this is even more so.
>
> I wasn't clear: the second one is "even more so" making sense, or "even
> more so" misguided defensive coding?
If the latter, then I think we want this:
-- >8 --
Subject: [PATCH] attr: drop misguided defensive coding
In prepare_attr_stack, we pop the old elements of the stack
(which were left from a previous lookup and may or may not
be useful to us). Our loop to do so checks that we never
reach the top of the stack. However, the code immediately
afterwards will segfault if we did actually reach the top of
the stack.
Fortunately, this is not an actual bug, since we will never
pop all of the stack elements (we will always keep the root
gitattributes, as well as the builtin ones). So the extra
check in the loop condition simply clutters the code and
makes the intent less clear. Let's get rid of it.
Signed-off-by: Jeff King <peff@peff.net>
---
attr.c | 2 +-
1 files changed, 1 insertions(+), 1 deletions(-)
diff --git a/attr.c b/attr.c
index fa975da..cf8f2bc 100644
--- a/attr.c
+++ b/attr.c
@@ -577,7 +577,7 @@ static void prepare_attr_stack(const char *path)
* Pop the ones from directories that are not the prefix of
* the path we are checking.
*/
- while (attr_stack && attr_stack->origin) {
+ while (attr_stack->origin) {
int namelen = strlen(attr_stack->origin);
elem = attr_stack;
--
1.7.9.rc0.33.gd3c17
^ permalink raw reply related
* Re: Please support add -p with a new file, to add only part of the file
From: Junio C Hamano @ 2012-01-10 19:33 UTC (permalink / raw)
To: Jeff King
Cc: Thomas Rast, Jonathan Nieder, Josh Triplett, git,
Wincent Colaiuta
In-Reply-To: <20120110183833.GA15787@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> Even if you start with "add -N", there won't be individual "hunks" you can
>> pick and choose from diffing emptiness and the whole new file, so you end
>> up using "edit hunk" interface.
>
> I don't think the main impetus for this is that people necessarily want
> to pick and choose hunks from added files.
Well, read the subject of your e-mail and tell me what it says ;-)
And that is why I was contrasting an imaginary workflow to use the
existing "add -p" with requested "allow it to be used with new files" with
what somebody may "emulate" without help from "add -p" machinery, which
is:
$ cp newfile.c newfile.c-saved
$ edit newfile.c ;# delete all the things you do not want for now
$ git add newfile.c
$ mv newfile.c-saved newfile.c
I just had to point the above out, even though I agree with the use case
you shown below for final verification. They are quite different topic, as
"git diff" won't be very useful for 'inspect changes' step in the "new
file" case to begin with.
> $ hack hack hack
> $ git add -p ;# inspect and add changes
> $ git commit
>
> which is very similar to the traditional git workflow:
>
> $ hack hack hack
> $ git diff ;# inspect changes
> $ git add foo ;# add changes
> $ git commit
>
> I find myself using "add -p" almost exclusively these days, as it
> combines the two middle steps (and even though I usually am just hitting
> "y" after inspection, when I _do_ want to make a change, I am right
> there in the hunk selection loop already).
^ permalink raw reply
* Re: Please support add -p with a new file, to add only part of the file
From: Jeff King @ 2012-01-10 19:38 UTC (permalink / raw)
To: Junio C Hamano
Cc: Thomas Rast, Jonathan Nieder, Josh Triplett, git,
Wincent Colaiuta
In-Reply-To: <7vd3ar9wto.fsf@alter.siamese.dyndns.org>
On Tue, Jan 10, 2012 at 11:33:39AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> >> Even if you start with "add -N", there won't be individual "hunks" you can
> >> pick and choose from diffing emptiness and the whole new file, so you end
> >> up using "edit hunk" interface.
> >
> > I don't think the main impetus for this is that people necessarily want
> > to pick and choose hunks from added files.
>
> Well, read the subject of your e-mail and tell me what it says ;-)
Heh. Oops.
Yes, I agree that "add -p" is not especially useful for that case, and
the workflow I was describing is very different[1].
Sorry for the noise.
-Peff
[1] For the record, I _do_ find myself using "git add -N" so that new
files can be part of my "git add -p" workflow. So that feature is
working as intended. It would save me a little bit of effort,
though, if I could tell git I also want to include untracked files
during the "-p" session, and dispense with the earlier "add -N".
I'd also find it useful to mark conflicts as resolved, but I think
we discussed that once before and you brought up some ugly corner
cases.
^ permalink raw reply
* Re: git svn dcommit sends to wrong branch
From: Thomas Rast @ 2012-01-10 19:43 UTC (permalink / raw)
To: git
In-Reply-To: <20120110161843.GC8196@victor>
Victor Engmark <victor.engmark@terreactive.ch> writes:
> Commands:
>
> git svn clone -s -r 1:HEAD http://svn/repo
> cd repo
> git commit [thrice, in master only]
Which git version is this? Before 1.6.5 (b186a261 to be precise)
git-svn pointed master at the branch where the last commit in SVN
happened, which is not necessarily trunk. After that it tries to point
it at trunk instead. You can find out, e.g., by saying 'git show' on
the fresh clone and looking at the git-svn-id line.
> git rebase --interactive HEAD~20 [i.e., started rebase in commits before
> the clone]
> [Merged two commits I had made *after* the clone]
> git commit ...
> git dcommit
>
> This created commits on
> <http://svn/repo/branches/branch_name>! Why? Is it because HEAD~20's
> git-svn-id <http://svn/repo/branches/branch_name@22481> is on that
> branch?
The rule is that the commits go to the branch named in the git-svn-id
line of the most recent first-parent ancestor of HEAD.
You can find the "base" commit in question with
git log -1 --first-parent --grep=^git-svn-id:
> And more importantly, how do I "replay" my commits on trunk?
You need to rebase the commits on trunk, and (very important) strip the
git-svn-id lines from their messages. If you only had a handful of
commits, your best bet is to use something like
git checkout -b newbranch
git rebase -i --onto svn/trunk svn/branch_name # or whatever git-svn named the remote branches
# edit all the 'pick' into 'reword'
# in every commit message editor that pops up, remove the git-svn-id line
gitk # make sure that you like the resulting history!
git svn dcommit
(If you have many commits, git-filter-branch can do the removal
automatically, but it's a bit of a loaded gun pointed at your foot.)
If your git-rebase is too old for 'reword', you can use 'edit' instead
and then, every time that git-rebase drops you into a command line, say
git commit --amend # and edit the commit message
git rebase --continue
--
Thomas Rast
trast@{inf,student}.ethz.ch
^ permalink raw reply
* Re: leaky cherry-pick
From: Jeff King @ 2012-01-10 19:50 UTC (permalink / raw)
To: Ramkumar Ramachandra
Cc: Pete Wyckoff, git, Junio C Hamano,
Nguyễn Thái Ngọc Duy
In-Reply-To: <CALkWK0nwE0c6qVvbauPrjmb3NX4NDeGSrvrC2ry2bjMeM4Hr0A@mail.gmail.com>
On Tue, Jan 10, 2012 at 10:58:45AM +0530, Ramkumar Ramachandra wrote:
> Interesting- I wonder where .gitattributes parsing code fits into all
> this. The purpose of bootstrap _attr_stack() is to populate
> attr_stack for its callers. Lots of memory allocation happening in
> handle_attr_line() -- that information is returned to
> bootstrap_attr_stack(). We have to keep backtracking until that
> information is provably useless and free it. Hm, convert_attrs() (in
> convert.c) is a common caller in both cases, but the populated
> attr_stack is local to attr.c; I wonder if this is the problem. If my
> hunch is right, it should be a trivial fix (caution: untested):
>
> Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
>
> diff --git a/attr.c b/attr.c
> index 76b079f..12e3824 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -745,6 +745,7 @@ int git_check_attr(const char *path, int num,
> struct git_attr_check *check)
> check[i].value = value;
> }
>
> + drop_attr_stack();
> return 0;
> }
I don't think this is right. The attr_stack is intentionally kept in
place after a lookup as a cache, because callers are very likely to
lookup nearby filenames. The first thing we do is pop unrelated parts of
the stack, keep the early bits, and then push any new needed
directories.
So if you do a lookup for "foo/bar/baz/file1", the stack afterwards would
be:
$GIT_DIR/info/attributes
foo/bar/baz/.gitattributes
foo/bar/.gitattributes
foo/.gitattributes
.gitattributes
[builtins]
If you then do a lookup for "foo/bar/baz/file2", it can use the exact
same stack without looking for or reparsing the attribute files. If you
then do a lookup for "foo/bar/bleep/file", it pops only the entry for
"foo/bar/baz/.gitattributes", and pushes only the entry for
"foo/bar/bleep/.gitattributes".
The calling code _could_ say "btw, I am done with attributes now, so
free the memory". But we don't bother, since it's a small amount of
memory, and other parts of the code may want it later.
-Peff
^ permalink raw reply
* Re: [PATCH] rebase --fix: interactive fixup mode
From: Neal Kreitzinger @ 2012-01-10 19:58 UTC (permalink / raw)
To: Michael Haggerty; +Cc: Clemens Buchacher, git, Junio C Hamano
In-Reply-To: <4F0AA7E2.9010200@alum.mit.edu>
On 1/9/2012 2:40 AM, Michael Haggerty wrote:
>
> Two comments:
>
> * The name "--fix" might be confusing because of its similarity to the
> "fixup" command that can be specified in the interactive instructions file.
>
> * I agree with you that "interactive rebase is frequently used not to
> rebase history, but to manipulate recent commits". In fact, I use
> interactive rebase *only* for manipulating recent commits and
> non-interactive rebase *only* for changing commits' ancestry. I think
> it is a good idea to make these two uses more distinct. For example, it
> makes me nervous that I might mis-type the<upstream> parameter when I
> am trying to touch up commits and end up inadvertently rebasing the
> commits onto a new parent.
>
He could all it --touchup like you did above.
v/r,
neal
^ permalink raw reply
* Re: [PATCH] attr: don't confuse prefixes with leading directories
From: Junio C Hamano @ 2012-01-10 20:25 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, git
In-Reply-To: <20120110192810.GA16018@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Tue, Jan 10, 2012 at 11:23:01AM -0800, Junio C Hamano wrote:
>
>> > I'm not sure if the right solution is to change the popping loop to:
>> >
>> > /* we will never run out of stack, because we always have the root */
>> > while (attr_stack->origin) {
>> > ...
>>
>> Yeah, that makes sense, as that existing check "attr_stack &&" was a
>> misguided defensive coding, that was _not_ defensive at all as we didn't
>> do anything after we stop iterating from that loop and without checking
>> dereferenced attr_stack->origin, which was a simple bogosity.
>>
>> >
>> > Or to be extra defensive and put:
>> >
>> > if (!attr_stack)
>> > die("BUG: we ran out of attr stack!?");
>> >
>> > after the loop, or to somehow handle the case of an empty attr stack
>> > below (which is hard to do, because it can't be triggered, so I have no
>> > idea what it would mean).
>>
>> And this is even more so.
>
> I wasn't clear: the second one is "even more so" making sense, or "even
> more so" misguided defensive coding?
Sorry for sending a half-baked response. The initial draft of my response
had just "that makes sense" and nothing else in the first paragraph.
If the original meant to be defensive, it should have had your "extra
defensive" die(), but it didn't.
But the condition to break out of that loop is either we hit an elem that
satisfy the condition (in which case that elem cannot be NULL) or we
successfully saw that attr_stack->origin is NULL (in which case attr_stack
couldn't have been NULL), so it is pointless to check the NULLness of
attr_stack itself. Assertion _before_ going into the while loop might make
sense, but if we look at what bootstrap_attr_stack() does, it should be
pretty obvious that that cannot happen.
An assert(attr_stack->origin) before we use it may make sense, though, in
order to make sure we do not mistakenly pop the root one and expose the
built-in ones whose origin are set to NULL.
diff --git a/attr.c b/attr.c
index ad7eb9c..4d3b61a 100644
--- a/attr.c
+++ b/attr.c
@@ -566,7 +567,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
/*
* Pop the ones from directories that are not the prefix of
- * the path we are checking.
+ * the path we are checking. Break out of the loop when we see
+ * the root one (whose origin is an empty string "") or the builtin
+ * one (whose origin is NULL) without popping it.
*/
while (attr_stack->origin) {
int namelen = strlen(attr_stack->origin);
@@ -586,6 +589,13 @@ static void prepare_attr_stack(const char *path, int dirlen)
* Read from parent directories and push them down
*/
if (!is_bare_repository() || direction == GIT_ATTR_INDEX) {
+ /*
+ * bootstrap_attr_stack() should have added, and the
+ * above loop should have stopped before popping, the
+ * root element whose attr_stack->origin is set to an
+ * empty string.
+ */
+ assert(attr_stack->origin);
while (1) {
char *cp;
^ permalink raw reply related
* rsync a *bunch* of git repos
From: Jason @ 2012-01-10 21:15 UTC (permalink / raw)
To: git
All,
I have a home directory with a lot of git repos in it. Possibly over a
hundred. This is a good thing. :-) However, I'm in the process of
changing OS's and need to move my home directory out of my root
partition into its own partition.
The nuts and bolts of this aren't difficult, the problem is I don't have
a complete understanding of how git stores data. I've heard in the
past that it uses a lot of hardlinks and softlinks. I need to make
sure, that once I transfer the data, and reboot the machine with the new
partition mounted under /home, that all my git repos will be okay.
Going through the git repos one by one would be long and prone to
errors, so I'm looking at alternatives. Does this look ok?
## From single user mode, or live cd
$ su -
# mkfs.ext4 /dev/sda3
# mount /dev/sda3 /mnt/home
# rsync --progress -avu /home/ /mnt/home/
# umount /mnt/home
# mv /home /home.orig
# mkdir /home
### Sanity check?
# mount /dev/sda3 /home
# rsync -avun /home.orig/ /home/
That'll check that rsync thinks things are ok, but what about git? Is
there a git sanity check I could run to detect that three hardlinks
still point to the same inode?
One alternative may be to boot to a live CD, delete everything from the
root partition except /home, then mv /home/* -> / . Basically, use my
current root partition as the new /home partition.
thx,
Jason.
^ permalink raw reply
* [BUG] git archive broken in 1.7.8.1
From: Albert Astals Cid @ 2012-01-10 21:18 UTC (permalink / raw)
To: git
CC me on answers since i'm not subscribed to the list
Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the syntax
git archive --remote=git://anongit.kde.org/repo.git HEAD:path
does not seem to return a valid tar archive anymore when it did work
previously. In fact the man page of my version has that syntax in one of the
examples.
Is that a regression or should have never it worked and the current behaviour
is the correct one?
Albert
^ permalink raw reply
* Re: rsync a *bunch* of git repos
From: Seth Robertson @ 2012-01-10 21:24 UTC (permalink / raw)
To: Jason; +Cc: git
In-Reply-To: <20120110211548.GD10255@titan.lakedaemon.net>
In message <20120110211548.GD10255@titan.lakedaemon.net>, Jason writes:
The nuts and bolts of this aren't difficult, the problem is I don't have
a complete understanding of how git stores data. I've heard in the
past that it uses a lot of hardlinks and softlinks. I need to make
sure, that once I transfer the data, and reboot the machine with the new
partition mounted under /home, that all my git repos will be okay.
Under most circumstances, git will do the right thing. Only if you
use specific flags on clone (like --shared or --reference) might
something go wrong, and as the manual page explains, you can use
git-repack to undo these.
The real solution is, after you rsync, to:
for f in */.git; do (cd $f; git fsck >&/dev/null || echo "$f is BAD!!"); done
If you get no output, you should be golden. If you get output, check
to make sure the repo you are copying from is good as well.
-Seth Robertson
^ permalink raw reply
* Re: [BUG] git archive broken in 1.7.8.1
From: Carlos Martín Nieto @ 2012-01-10 21:33 UTC (permalink / raw)
To: Albert Astals Cid; +Cc: git
In-Reply-To: <5142795.2dTmMhVRTP@xps>
[-- Attachment #1: Type: text/plain, Size: 671 bytes --]
On Tue, Jan 10, 2012 at 10:18:41PM +0100, Albert Astals Cid wrote:
> CC me on answers since i'm not subscribed to the list
>
> Hi, one of our [KDE] anongit servers was updated to 1.7.8.1 and not the syntax
>
> git archive --remote=git://anongit.kde.org/repo.git HEAD:path
This syntax is no longer allowed due to some security tightening. Use
the alternate syntax
git archive --remote=git://anongit.kde.org/repo.git HEAD -- path
>
> does not seem to return a valid tar archive anymore when it did work
> previously. In fact the man page of my version has that syntax in one of the
> examples.
That sounds like a documentation bug.
cmn
[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]
^ permalink raw reply
* Re: rsync a *bunch* of git repos
From: Philip Oakley @ 2012-01-10 22:00 UTC (permalink / raw)
To: Jason, Seth Robertson; +Cc: git
In-Reply-To: <201201102124.q0ALOowL026941@no.baka.org>
From: "Seth Robertson" <in-gitvger@baka.org> Sent: Tuesday, January 10, 2012
9:24 PM
> In message <20120110211548.GD10255@titan.lakedaemon.net>, Jason writes:
>
> The nuts and bolts of this aren't difficult, the problem is I don't
> have
> a complete understanding of how git stores data. I've heard in the
> past that it uses a lot of hardlinks and softlinks. I need to make
> sure, that once I transfer the data, and reboot the machine with the
> new
> partition mounted under /home, that all my git repos will be okay.
>
> Under most circumstances, git will do the right thing. Only if you
> use specific flags on clone (like --shared or --reference) might
> something go wrong, and as the manual page explains, you can use
> git-repack to undo these.
I think there is an exception for certain git submodule setups with local
repos, where the gitdir link is hard coded to the absolute machine path.
There has been some discussion on the list recently about trying to cover
this case.
>
> The real solution is, after you rsync, to:
>
> for f in */.git; do (cd $f; git fsck >&/dev/null || echo "$f is BAD!!");
> done
>
> If you get no output, you should be golden. If you get output, check
> to make sure the repo you are copying from is good as well.
>
> -Seth Robertson
^ 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