* [PATCH] config: if mtime (or size) of the config file changed since last read, reread it
From: Johannes Schindelin @ 2006-05-06 23:26 UTC (permalink / raw)
To: git
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
This is extremely paranoic, I know.
config.c | 16 ++++++++++------
1 files changed, 10 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c
index 05d4d8c..6765186 100644
--- a/config.c
+++ b/config.c
@@ -13,6 +13,7 @@ #define MAXNAME (256)
static const char *contents = NULL;
static int config_length = 0, config_offset = 0;
static const char *config_file_name;
+static time_t config_file_mtime = 0;
static int config_linenr;
static int get_next_char(void)
{
@@ -255,23 +256,26 @@ int git_default_config(const char *var,
int git_config_from_file(config_fn_t fn, const char *filename)
{
int ret, in_fd;
+ struct stat st;
config_offset = 0;
+ in_fd = open(filename, O_RDONLY);
+ fstat(in_fd, &st);
+
if (contents) {
- if (!strcmp(config_file_name, filename))
+ if (!strcmp(config_file_name, filename)
+ && config_file_mtime == st.st_mtime
+ && config_length == st.st_size) {
+ close(in_fd);
return git_parse_file(fn);
+ }
munmap((char*)contents, config_length);
free((char*)config_file_name);
}
- in_fd = open(filename, O_RDONLY);
-
ret = -1;
if (in_fd > 0) {
- struct stat st;
-
- fstat(in_fd, &st);
config_length = st.st_size;
contents = mmap(NULL, config_length, PROT_READ, MAP_PRIVATE,
in_fd, 0);
--
1.3.1.g5545a
^ permalink raw reply related
* Re: [PATCH] fmt-patch: understand old <his> notation
From: Olivier Galibert @ 2006-05-06 23:19 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <Pine.LNX.4.64.0605061527030.16343@g5.osdl.org>
On Sat, May 06, 2006 at 03:30:38PM -0700, Linus Torvalds wrote:
> Maybe not. I've actually cursed the fact that I made "git diff X" mean
> "diff from X to current working tree", because it almost never makes any
> sense at all when X is anything but "HEAD".
>
> I probably _should_ have made "git diff X" mean basically "git show X",
> and not what it means now.
Funny, when tracking other projects I use (sometimes path-filtered)
"git diff origin" often, but I find "git show origin" utterly
uninteresting.
OG.
^ permalink raw reply
* [PATCH] config: mmap() the contents of the config file
From: Johannes Schindelin @ 2006-05-06 23:04 UTC (permalink / raw)
To: git, junkio
This makes it possible to rewrite the config without accessing the config
file twice.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
Linus said he'd prefer this.
One thing I am not quite sure about: should we remember the file's
mtime and check it to see if the config file has changed since we
mmap()ed it?
BTW this patch is on top of next, and does not yet contain Sean's
bug fix.
config.c | 102 ++++++++++++++++++++++++++++++++------------------------------
1 files changed, 52 insertions(+), 50 deletions(-)
diff --git a/config.c b/config.c
index 87fb220..05d4d8c 100644
--- a/config.c
+++ b/config.c
@@ -10,31 +10,27 @@ #include <regex.h>
#define MAXNAME (256)
-static FILE *config_file;
+static const char *contents = NULL;
+static int config_length = 0, config_offset = 0;
static const char *config_file_name;
static int config_linenr;
static int get_next_char(void)
{
int c;
- FILE *f;
c = '\n';
- if ((f = config_file) != NULL) {
- c = fgetc(f);
- if (c == '\r') {
+ if (config_offset < config_length) {
+ c = contents[config_offset++];
+ if (c == '\r' && config_offset < config_length) {
/* DOS like systems */
- c = fgetc(f);
+ c = contents[config_offset++];
if (c != '\n') {
- ungetc(c, f);
+ config_offset--;
c = '\r';
}
}
if (c == '\n')
config_linenr++;
- if (c == EOF) {
- config_file = NULL;
- c = '\n';
- }
}
return c;
}
@@ -162,7 +158,7 @@ static int git_parse_file(config_fn_t fn
int c = get_next_char();
if (c == '\n') {
/* EOF? */
- if (!config_file)
+ if (config_offset >= config_length)
return 0;
comment = 0;
continue;
@@ -258,18 +254,41 @@ int git_default_config(const char *var,
int git_config_from_file(config_fn_t fn, const char *filename)
{
- int ret;
- FILE *f = fopen(filename, "r");
+ int ret, in_fd;
+
+ config_offset = 0;
+
+ if (contents) {
+ if (!strcmp(config_file_name, filename))
+ return git_parse_file(fn);
+ munmap((char*)contents, config_length);
+ free((char*)config_file_name);
+ }
+
+ in_fd = open(filename, O_RDONLY);
ret = -1;
- if (f) {
- config_file = f;
- config_file_name = filename;
+ if (in_fd > 0) {
+ struct stat st;
+
+ fstat(in_fd, &st);
+ config_length = st.st_size;
+ contents = mmap(NULL, config_length, PROT_READ, MAP_PRIVATE,
+ in_fd, 0);
+ close(in_fd);
+
+ config_file_name = strdup(filename);
config_linenr = 1;
+ config_offset = 0;
ret = git_parse_file(fn);
- fclose(f);
- config_file_name = NULL;
+ } else {
+ contents = NULL;
+ config_length = 0;
+ if (in_fd < 0 && ENOENT != errno )
+ die("opening %s: %s", config_file_name,
+ strerror(errno));
}
+
return ret;
}
@@ -317,7 +336,7 @@ static int store_aux(const char* key, co
return 1;
}
- store.offset[store.seen] = ftell(config_file);
+ store.offset[store.seen] = config_offset;
store.seen++;
}
break;
@@ -327,12 +346,12 @@ static int store_aux(const char* key, co
break;
} else
/* do not increment matches: this is no match */
- store.offset[store.seen] = ftell(config_file);
+ store.offset[store.seen] = config_offset;
/* fallthru */
case SECTION_END_SEEN:
case START:
if (matches(key, value)) {
- store.offset[store.seen] = ftell(config_file);
+ store.offset[store.seen] = config_offset;
store.state = KEY_SEEN;
store.seen++;
} else if (strrchr(key, '.') - key == store.baselen &&
@@ -367,10 +386,9 @@ static void store_write_pair(int fd, con
write(fd, "\n", 1);
}
-static int find_beginning_of_line(const char* contents, int size,
- int offset_, int* found_bracket)
+static int find_beginning_of_line(int offset_, int* found_bracket)
{
- int equal_offset = size, bracket_offset = size;
+ int equal_offset = config_length, bracket_offset = config_length;
int offset;
for (offset = offset_-2; offset > 0
@@ -420,7 +438,7 @@ int git_config_set_multivar(const char*
const char* value_regex, int multi_replace)
{
int i;
- int fd, in_fd;
+ int fd;
int ret;
char* config_filename = strdup(git_path("config"));
char* lock_file = strdup(git_path("config.lock"));
@@ -471,18 +489,9 @@ int git_config_set_multivar(const char*
/*
* If .git/config does not exist yet, write a minimal version.
*/
- in_fd = open(config_filename, O_RDONLY);
- if ( in_fd < 0 ) {
+ if (!contents) {
free(store.key);
- if ( ENOENT != errno ) {
- error("opening %s: %s", config_filename,
- strerror(errno));
- close(fd);
- unlink(lock_file);
- ret = 3; /* same as "invalid config file" */
- goto out_free;
- }
/* if nothing to unset, error out */
if (value == NULL) {
close(fd);
@@ -494,9 +503,7 @@ int git_config_set_multivar(const char*
store.key = (char*)key;
store_write_section(fd, key);
store_write_pair(fd, key, value);
- } else{
- struct stat st;
- char* contents;
+ } else {
int i, copy_begin, copy_end, new_line = 0;
if (value_regex == NULL)
@@ -555,23 +562,17 @@ int git_config_set_multivar(const char*
goto out_free;
}
- fstat(in_fd, &st);
- contents = mmap(NULL, st.st_size, PROT_READ,
- MAP_PRIVATE, in_fd, 0);
- close(in_fd);
-
if (store.seen == 0)
store.seen = 1;
for (i = 0, copy_begin = 0; i < store.seen; i++) {
if (store.offset[i] == 0) {
- store.offset[i] = copy_end = st.st_size;
+ store.offset[i] = copy_end = config_length;
} else if (store.state != KEY_SEEN) {
copy_end = store.offset[i];
} else
copy_end = find_beginning_of_line(
- contents, st.st_size,
- store.offset[i]-2, &new_line);
+ store.offset[i] - 2, &new_line);
/* write the first part of the config */
if (copy_end > copy_begin) {
@@ -591,11 +592,12 @@ int git_config_set_multivar(const char*
}
/* write the rest of the config */
- if (copy_begin < st.st_size)
+ if (copy_begin < config_length)
write(fd, contents + copy_begin,
- st.st_size - copy_begin);
+ config_length - copy_begin);
- munmap(contents, st.st_size);
+ munmap((char*)contents, config_length);
+ contents = NULL;
unlink(config_filename);
}
--
1.3.1.g5545a
^ permalink raw reply related
* Re: [RFC] get_pathspec(): free() old buffer if rewriting
From: Johannes Schindelin @ 2006-05-06 22:44 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, junkio
In-Reply-To: <Pine.LNX.4.64.0605061532190.16343@g5.osdl.org>
Hi,
On Sat, 6 May 2006, Linus Torvalds wrote:
> On Sun, 7 May 2006, Johannes Schindelin wrote:
> >
> > This might be the wrong way to do it, but as it is without this patch,
> > get_pathspec() is leaking memory.
>
> I'm not at all convinced we want to do somethng like this.
>
> get_pathspec() is a one-time event. It doesn't "leak" memory. To me,
> "leaking" is when you continually lose a bit of memory, and you eventually
> run out. I don't see that happening here.
I see your point. That was exactly why I put "RFC" and not "PATCH" on the
subject.
> So there's a difference between "don't care" and "leak memory". It sounds
> like you may be using some automated tool that warns because it simply
> doesn't understand that difference.
Nope. No automated tool. Just my brain which wanted to fix _all_
occurrences of that prefix_path() usage bug.
But in case anybody uses Valgrind et al. on git, how about at least a
comment telling the casual observer why we don't free() the buffers?
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC] get_pathspec(): free() old buffer if rewriting
From: Linus Torvalds @ 2006-05-06 22:37 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, junkio
In-Reply-To: <Pine.LNX.4.63.0605070003430.6357@wbgn013.biozentrum.uni-wuerzburg.de>
On Sun, 7 May 2006, Johannes Schindelin wrote:
>
> This might be the wrong way to do it, but as it is without this patch,
> get_pathspec() is leaking memory.
I'm not at all convinced we want to do somethng like this.
get_pathspec() is a one-time event. It doesn't "leak" memory. To me,
"leaking" is when you continually lose a bit of memory, and you eventually
run out. I don't see that happening here.
It's like saying that calling an initialization routine "leaks" memory,
because it doesn't free the memory that contains the function that is now
no longer ever used. It's clearly leaving that memory unused, "dangling".
But the fact is, the OS will free the memory for us when we exit, and
simplicity is often much more powerful than trying to be overly careful.
So there's a difference between "don't care" and "leak memory". It sounds
like you may be using some automated tool that warns because it simply
doesn't understand that difference.
Linus
^ permalink raw reply
* Re: [PATCH] fmt-patch: understand old <his> notation
From: Linus Torvalds @ 2006-05-06 22:30 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Johannes Schindelin, git
In-Reply-To: <7viroirfur.fsf@assigned-by-dhcp.cox.net>
On Sat, 6 May 2006, Junio C Hamano wrote:
>
> While this would be easier on _my_ fingers as well, I have a
> suspicion that it might make more sense to make this "single
> ref" case to mean "HEAD~5^..HEAD~5" (if we _were_ designing a
> new command that is called format-patch today, that would be
> more natural).
Careful, that "X^..X" thing does entirely the wrong thing for merges. Not
what you want at all, I think.
> But probably it is too late to break it by now.
Maybe not. I've actually cursed the fact that I made "git diff X" mean
"diff from X to current working tree", because it almost never makes any
sense at all when X is anything but "HEAD".
I probably _should_ have made "git diff X" mean basically "git show X",
and not what it means now.
Oh, well.
Linus
^ permalink raw reply
* [RFC] get_pathspec(): free() old buffer if rewriting
From: Johannes Schindelin @ 2006-05-06 22:04 UTC (permalink / raw)
To: git, junkio
This might be the wrong way to do it, but as it is without this patch,
get_pathspec() is leaking memory.
However, it is by no means guaranteed that the input is malloc()ed. The
tests run through without problems, but you never know...
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
setup.c | 5 +++++
1 files changed, 5 insertions(+), 0 deletions(-)
diff --git a/setup.c b/setup.c
index fe7f884..9c39d6e 100644
--- a/setup.c
+++ b/setup.c
@@ -126,6 +126,11 @@ const char **get_pathspec(const char *pr
prefixlen = prefix ? strlen(prefix) : 0;
do {
*p = prefix_path(prefix, prefixlen, entry);
+ if (*p != entry) {
+ if (*p > entry && *p < entry + strlen(entry))
+ *p = strdup(*p);
+ free((char*)entry);
+ }
} while ((entry = *++p) != NULL);
return (const char **) pathspec;
}
--
1.3.2.g9ba6-dirty
^ permalink raw reply related
* [PATCH] Fix users of prefix_path() to free() only when necessary
From: Johannes Schindelin @ 2006-05-06 22:02 UTC (permalink / raw)
To: git, junkio
Unfortunately, prefix_path() sometimes returns a newly xmalloc()ed buffer,
and in other cases it returns a substring!
For example, when calling
git update-index ./hello.txt
prefix_path() returns "hello.txt", but does not allocate a new buffer. The
original code only checked if the result of prefix_path() was different from
what was passed in, and thusly trigger a segmentation fault.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
checkout-index.c | 4 ++--
update-index.c | 8 ++++----
2 files changed, 6 insertions(+), 6 deletions(-)
diff --git a/checkout-index.c b/checkout-index.c
index 64bdc3b..9876af6 100644
--- a/checkout-index.c
+++ b/checkout-index.c
@@ -278,7 +278,7 @@ int main(int argc, char **argv)
die("git-checkout-index: don't mix '--stdin' and explicit filenames");
p = prefix_path(prefix, prefix_length, arg);
checkout_file(p);
- if (p != arg)
+ if (p < arg || p > arg + strlen(arg))
free((char*)p);
}
@@ -300,7 +300,7 @@ int main(int argc, char **argv)
path_name = buf.buf;
p = prefix_path(prefix, prefix_length, path_name);
checkout_file(p);
- if (p != path_name)
+ if (p < path_name || p > path_name + strlen(path_name))
free((char *)p);
if (path_name != buf.buf)
free(path_name);
diff --git a/update-index.c b/update-index.c
index 7db67aa..f6b09a4 100644
--- a/update-index.c
+++ b/update-index.c
@@ -393,7 +393,7 @@ static void update_one(const char *path,
die("Unable to process file %s", path);
report("add '%s'", path);
free_return:
- if (p != path)
+ if (p < path || p > path + strlen(path))
free((char*)p);
}
@@ -609,7 +609,7 @@ static int do_unresolve(int ac, const ch
const char *arg = av[i];
const char *p = prefix_path(prefix, prefix_length, arg);
err |= unresolve_one(p);
- if (p != arg)
+ if (p < arg || p > arg + strlen(arg))
free((char*)p);
}
return err;
@@ -623,7 +623,7 @@ static int do_reupdate(int ac, const cha
*/
int pos;
int has_head = 1;
- char **pathspec = get_pathspec(prefix, av + 1);
+ const char **pathspec = get_pathspec(prefix, av + 1);
if (read_ref(git_path("HEAD"), head_sha1))
/* If there is no HEAD, that means it is an initial
@@ -815,7 +815,7 @@ int main(int argc, const char **argv)
update_one(p, NULL, 0);
if (set_executable_bit)
chmod_path(set_executable_bit, p);
- if (p != path_name)
+ if (p < path_name || p > path_name + strlen(path_name))
free((char*) p);
if (path_name != buf.buf)
free(path_name);
--
1.3.2.g9ba6-dirty
^ permalink raw reply related
* Re: [PATCH] fmt-patch: understand old <his> notation
From: Johannes Schindelin @ 2006-05-06 22:01 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <7viroirfur.fsf@assigned-by-dhcp.cox.net>
Hi,
On Sat, 6 May 2006, Junio C Hamano wrote:
> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
>
> > When calling "git fmt-patch HEAD~5", you now get the same as if you would
> > have said "git fmt-patch HEAD~5..". This makes it easier for my fingers
> > which are so used to the old syntax.
>
> While this would be easier on _my_ fingers as well, I have a
> suspicion that it might make more sense to make this "single
> ref" case to mean "HEAD~5^..HEAD~5" (if we _were_ designing a
> new command that is called format-patch today, that would be
> more natural). But probably it is too late to break it by now.
No, it is not too late. I did this patch only to prevent cluttering my
directory with millions of patches, only because I forgot _again_ to type
the two dots.
> > I wonder: would it make sense to make add_pending_object() and
> > get_reference() in revision.c non-static?
>
> I'd rather not expose such revision.c internals too much. An
> alternative approach would be to give instruction to revision.c
> (read: another flag like rev.no_walk) to tell it to do something
> special when the user has only one commit, but I think what you
> did in your patch is cleaner and sufficient.
I just stole the function add_head() from builtin-diff.c, but that feels
wrong. I think adding a pending object should not be internal to
revision.c.
> Also we probably would want to default the diff options to show
> the root commit diff (rev.show_root_diff).
I gather this is needed for git-am/git-rebase to continue working?
Ciao,
Dscho
^ permalink raw reply
* Re: [RFC] Managing projects - advanced Git tutorial/walkthrough
From: sean @ 2006-05-06 21:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: jnareb, git
In-Reply-To: <7v64kisyow.fsf@assigned-by-dhcp.cox.net>
On Sat, 06 May 2006 13:09:03 -0700
Junio C Hamano <junkio@cox.net> wrote:
[...]
> I think tutorial.txt is the right "your first experience with
> git with walkthru" document, and the materials it gives, and the
> order in which it introduces them, are very well thought out;
> kudos to JBF. We might also want to show "git grep", but other
> than that I do not think of anything that a new user might want
> to use on the first day.
It doesn't mention git log, show or status which are important
for the first day. Also an example of git commit --amend would
be a nice touch.
Part of the problem people new to git are having arise by reading
documentation in the wrong order and coming to the conclusion that
git is an ugly beast. Also many people are still finding out-of-
date information before anything else (eg. git isn't an scm only an
object tracker).
Part of this will be solved by having a useful and inviting web page
(thanks Pasky!). But it would also help to rename core-tutorial.txt
to something that doesn't sound inviting to newbies. Something
along the line of core-internal-design.txt (or here-be-dragons.txt).
Turning the main man page into more of a reference than a tutorial
slash concepts page would be something worth doing too. Removing
all of the plumbing commands from that page should at least be
considered.
[...]
> Then reorganize the initial part core-tutorial.txt to match the
> examples tutorial.txt gives, and demonstrate what is happening
> under the hood. The tutorial says "git init-db" then "git
> add". The core-tutorial would match that and explain what
> happens when "git init-db" is run (creates .git/objects etc.)
> and "git add" is run (populates the index).
>
There really seems to be a lot of room for another intermediate user
level document between tutorial.txt and the current core-tutorial.txt.
There are lots of concepts that can be explained without having to get
into the low level design or tools of git. For example, a user could
have long been productive with git before ever having to learn about
read-tree/write-tree etc.. all those commands are more for porcelain
writers and git guru/experts now.
Sean
^ permalink raw reply
* Re: Unresolved issues #2
From: Linus Torvalds @ 2006-05-06 21:51 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git, sean
In-Reply-To: <Pine.LNX.4.63.0605062332420.6423@wbgn013.biozentrum.uni-wuerzburg.de>
On Sat, 6 May 2006, Johannes Schindelin wrote:
>
> It was done because the very syntax of the config suggests it be a
> user-editable file.
Yeah, I personally much prefer user-friendly config files. Any format that
thinks that "easy parsing" is more important than "visually obvious" is
bad. So I obviously think that XML is a horrid piece of cr*p (has anybody
ever noticed I have strong opinions?) and totally unreadable.
I think "git repo-config" is doing a reasonable job of editing a file that
is really designed to be user-friendly. That said, the code _is_ a bit
scary.
It might be worthwhile to re-write config.c to read the config file into
memory and work on it in-memory instead of doing the funky mixed usage
(using fgetc/ftell to read it, but then switching over to mmap when
rewriting it).
IOW, maybe that "static FILE *config_file" should be changed to something
more like "static const char *config_buffer; unsigned int len;" instead,
and at least make both the reading and writing use the same buffer rather
than mixing stdio and mmap..
Linus
^ permalink raw reply
* Re: [PATCH] fmt-patch: understand old <his> notation
From: Junio C Hamano @ 2006-05-06 21:41 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git
In-Reply-To: <Pine.LNX.4.63.0605062252530.4155@wbgn013.biozentrum.uni-wuerzburg.de>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> When calling "git fmt-patch HEAD~5", you now get the same as if you would
> have said "git fmt-patch HEAD~5..". This makes it easier for my fingers
> which are so used to the old syntax.
While this would be easier on _my_ fingers as well, I have a
suspicion that it might make more sense to make this "single
ref" case to mean "HEAD~5^..HEAD~5" (if we _were_ designing a
new command that is called format-patch today, that would be
more natural). But probably it is too late to break it by now.
> I wonder: would it make sense to make add_pending_object() and
> get_reference() in revision.c non-static?
I'd rather not expose such revision.c internals too much. An
alternative approach would be to give instruction to revision.c
(read: another flag like rev.no_walk) to tell it to do something
special when the user has only one commit, but I think what you
did in your patch is cleaner and sufficient.
Also we probably would want to default the diff options to show
the root commit diff (rev.show_root_diff).
^ permalink raw reply
* Re: Unresolved issues #2
From: Johannes Schindelin @ 2006-05-06 21:33 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Linus Torvalds, git, sean
In-Reply-To: <7vvesirh0q.fsf@assigned-by-dhcp.cox.net>
Hi,
On Sat, 6 May 2006, Junio C Hamano wrote:
> Linus Torvalds <torvalds@osdl.org> writes:
>
> > Finally, I think "git repo-config" is buggy. Try with this .config file:
> > ...
> > So we'd really be screwing with porcelain if we made them use this ;)
>
> Thanks Linus and Sean for bringing this up and fixing it.
>
> I have a vague feeling that this may not be the last breakage of
> the repo-config command. My first reaction to the repo-config
> code was "eek". It tries to reuse as much the existing material
> as possible -- I understand it was done that way in order to
> preserve the comments and blank lines from the original config
> file intact, but it just felt very error prone (demonstrated by
> cases like this and the other one Sean brought up) and generally
> wrong.
It was done because the very syntax of the config suggests it be a
user-editable file. I do not want to mess with the comments more than
necessary.
> It might make sense to rewrite it to parse and read the existing
> configuration as a whole, do necessary manupulations on the
> parsed internal representation in-core, and write the result out
> from scratch. That would fix another of my pet peeve: after an
> invocation of repo-config to remove the last variable in a
> section, it leaves an empty section header in.
Does it really hurt? I think not.
Anyway, I'll look into this.
Ciao,
Dscho
^ permalink raw reply
* Re: Unresolved issues #2
From: Junio C Hamano @ 2006-05-06 21:16 UTC (permalink / raw)
To: Linus Torvalds; +Cc: git, sean, Johannes Schindelin
In-Reply-To: <Pine.LNX.4.64.0605061008340.16343@g5.osdl.org>
Linus Torvalds <torvalds@osdl.org> writes:
> Finally, I think "git repo-config" is buggy. Try with this .config file:
> ...
> So we'd really be screwing with porcelain if we made them use this ;)
Thanks Linus and Sean for bringing this up and fixing it.
I have a vague feeling that this may not be the last breakage of
the repo-config command. My first reaction to the repo-config
code was "eek". It tries to reuse as much the existing material
as possible -- I understand it was done that way in order to
preserve the comments and blank lines from the original config
file intact, but it just felt very error prone (demonstrated by
cases like this and the other one Sean brought up) and generally
wrong.
It might make sense to rewrite it to parse and read the existing
configuration as a whole, do necessary manupulations on the
parsed internal representation in-core, and write the result out
from scratch. That would fix another of my pet peeve: after an
invocation of repo-config to remove the last variable in a
section, it leaves an empty section header in.
$ git repo-config foo.bar true
$ cat .git/config
[core]
repositoryformatversion = 0
filemode = true
[foo]
bar = true
$ git repo-config foo1.baz false
$ git repo-config --unset foo.bar
$ cat .git/config
[core]
repositoryformatversion = 0
filemode = true
[foo]
[foo1]
baz = false
^ permalink raw reply
* Re: [PATCH] Another config file parsing fix.
From: Johannes Schindelin @ 2006-05-06 21:01 UTC (permalink / raw)
To: sean; +Cc: git, torvalds
In-Reply-To: <BAYC1-PASMTP08FCAC8B8FB768ED1791F1AEAA0@CEZ.ICE>
Hi,
On Sat, 6 May 2006, sean wrote:
> On Sat, 6 May 2006 14:14:02 -0400
> sean <seanlkml@sympatico.ca> wrote:
>
> > If the variable we need to store should go into a section
> > that currently only has a single variable (not matching
> > the one we're trying to insert), we will already be into
> > the next section before we notice we've bypassed the correct
> > location to insert the variable.
> >
> > To handle this case we store the current location as soon
> > as we find a variable matching the section of our new
> > variable.
> >
>
> Sorry.. this should really be amended to mention that it was Linus
> who spotted the problem.
Thanks to both of you!
Ciao,
Dscho
^ permalink raw reply
* [PATCH] fmt-patch: understand old <his> notation
From: Johannes Schindelin @ 2006-05-06 20:56 UTC (permalink / raw)
To: git, junkio
When calling "git fmt-patch HEAD~5", you now get the same as if you would
have said "git fmt-patch HEAD~5..". This makes it easier for my fingers
which are so used to the old syntax.
Signed-off-by: Johannes Schindelin <Johannes.Schindelin@gmx.de>
---
I wonder: would it make sense to make add_pending_object() and
get_reference() in revision.c non-static?
builtin-diff.c | 2 +-
builtin-log.c | 8 ++++++++
2 files changed, 9 insertions(+), 1 deletions(-)
diff --git a/builtin-diff.c b/builtin-diff.c
index 636edbf..2087316 100644
--- a/builtin-diff.c
+++ b/builtin-diff.c
@@ -232,7 +232,7 @@ static int builtin_diff_combined(struct
return 0;
}
-static void add_head(struct rev_info *revs)
+void add_head(struct rev_info *revs)
{
unsigned char sha1[20];
struct object *obj;
diff --git a/builtin-log.c b/builtin-log.c
index 0027998..d5bbc1c 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -11,6 +11,9 @@ #include "revision.h"
#include "log-tree.h"
#include "builtin.h"
+/* this is in builtin-diff.c */
+void add_head(struct rev_info *revs);
+
static int cmd_log_wc(int argc, const char **argv, char **envp,
struct rev_info *rev)
{
@@ -185,6 +188,11 @@ int cmd_format_patch(int argc, const cha
if (argc > 1)
die ("unrecognized argument: %s", argv[1]);
+ if (rev.pending_objects && rev.pending_objects->next == NULL) {
+ rev.pending_objects->item->flags |= UNINTERESTING;
+ add_head(&rev);
+ }
+
if (!use_stdout)
realstdout = fdopen(dup(1), "w");
--
1.3.1.g9ba6-dirty
^ permalink raw reply related
* Re: [RFC] Managing projects - advanced Git tutorial/walkthrough
From: Junio C Hamano @ 2006-05-06 20:09 UTC (permalink / raw)
To: Jakub Narebski; +Cc: git
In-Reply-To: <e3hnjg$k9f$1@sea.gmane.org>
Jakub Narebski <jnareb@gmail.com> writes:
> I have browsed through Git documentation: "A tutorial introduction to
> git" (tutorial.txt), "A short git tutorial" (core-tutorial.txt) which
> contrary to the title is the tutorial in low-level git commands and is
> longer that the first one, "Everyday GIT With 20 Commands Or
> So" (everyday.txt) and "git for CVS users" (cvs-migration.txt) which does
> not mention git-blame and git-annotate.
I was initially somewhat dissapointed that a posting marked as
RFC did not contain a draft that is commentable, but we would
probably want to know how the updated document set will be
organized in general first. There was a discussion both here
and on #irc, while Pasky and gang were working on the wiki,
about sprucing up the introductory documentation set.
The core-tutorial grow out of a short tutorial to start from
Plumbing basics (what's in object store) to cover Porcelainish;
when it was written there was not another user-level document,
so it had to cover both, but it is probably a good idea to move
the parts that talk about Porcelainish to other documents and
make it "A short tutorial on git Plumbing" document. Maybe we
can have a tiered document set like this:
- Your first experience with git with walkthru. This shows the
minimum basic operations to get started a stand-alone "hello
world" project, without talking about index nor object store.
The current tutorial.txt is probably good enough with
updates.
- Understanding git as an end user. Currently, this is
included in the global map git(7) documentation. It might
make sense to separate it out. This should talk about
concepts like blobs/trees/commits/trust/index without going
into lowlevel details of the implementation. The stress
should be on what they are for, not operationally but
philosophically. What's currently in README would be
suitable for this part, with some additional topics:
- branches. "Tying it all together" section talks about the
single branch and "the HEAD state"; we should talk about
why you would want to use multiple branches (either
keeping track of your own development, or keeping track of
somebody else's) and stress branches are to keep separate
things separate (explained that way it becomes clear why
you should not commit on a remote tracking branch).
- ancestry traversal. what "A..B" or "^A ^B C" means and why
you would want to say them.
- Everyday.
- Special interests: cvs migration, howto/ documents.
- The global map git(7) with pointers to individual commands,
and the glossary.
- Tutorial on Plumbing.
For a new end-user, the order to read would be from the top to
bottom. "Everyday" should cover most of what are needed for
different classes of users, and other things can be looked up
from the global map.
I think tutorial.txt is the right "your first experience with
git with walkthru" document, and the materials it gives, and the
order in which it introduces them, are very well thought out;
kudos to JBF. We might also want to show "git grep", but other
than that I do not think of anything that a new user might want
to use on the first day.
It is deliberately sketchy at times to keep the flow of walkthru
clean and simple, and I'd like to keep it that way. I would
however like to see the examples to show the expected output
from the commands, like the initial part of the current
core-tutorial.txt does. Also, I'd like to see a "see also" link
to each step that refers the user to "what if this step does not
work as expected for you", either separate document or a section
in the appendix part of the same document, without cluttering
the main text too much.
Some commands and syntax it mentions may need to be rethought in
the light what happened recently, especially the internal
version of diff and log/show/whatchanged unification that
happened before the 1.3.0 came out.
* "git diff A B" should probably be spelled as "git diff A..B"
throughout for consistency.
* Today's "git log" is more powerful than the one we had when
the tutorial was first written, and we probably want to
recommend "git log [-p|--stat]" in place of whatchanged;
whatchanged is kept primarily for historical reasons and to
give a different default output format than log. A new user
does not have to even know about it.
Then reorganize the initial part core-tutorial.txt to match the
examples tutorial.txt gives, and demonstrate what is happening
under the hood. The tutorial says "git init-db" then "git
add". The core-tutorial would match that and explain what
happens when "git init-db" is run (creates .git/objects etc.)
and "git add" is run (populates the index).
^ permalink raw reply
* [PATCH] t1300-repo-config: two new config parsing tests.
From: sean @ 2006-05-06 19:43 UTC (permalink / raw)
To: git; +Cc: torvalds
In-Reply-To: <BAYC1-PASMTP08FCAC8B8FB768ED1791F1AEAA0@CEZ.ICE>
- correctly insert a new variable into a section that only
contains a single (different) variable.
- correctly insert a new section that matches the initial
substring of an existing section.
Signed-off-by: Sean Estabrooks <seanlkml@sympatico.ca>
---
Both tests fail with v1.3.2 and pass with latest patches.
t/t1300-repo-config.sh | 31 +++++++++++++++++++++++++++++++
1 files changed, 31 insertions(+), 0 deletions(-)
ef2178a10e27f43d4120884bc587c460e9b1bfcb
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 1bf728f..0914be2 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -278,5 +278,36 @@ git-repo-config > output 2>&1
test_expect_success 'no arguments, but no crash' \
"test $? = 129 && grep usage output"
+cat > .git/config << EOF
+[a.b]
+ c = d
+EOF
+
+git-repo-config a.x y
+
+cat > expect << EOF
+[a.b]
+ c = d
+[a]
+ x = y
+EOF
+
+test_expect_success 'new section is partial match of another' 'cmp .git/config expect'
+
+git-repo-config b.x y
+git-repo-config a.b c
+
+cat > expect << EOF
+[a.b]
+ c = d
+[a]
+ x = y
+ b = c
+[b]
+ x = y
+EOF
+
+test_expect_success 'new variable inserts into proper section' 'cmp .git/config expect'
+
test_done
--
1.3.2.gd777c
^ permalink raw reply related
* Re: [PATCH] Another config file parsing fix.
From: sean @ 2006-05-06 18:25 UTC (permalink / raw)
To: sean; +Cc: git, torvalds
In-Reply-To: <20060506141402.3909cb37.seanlkml@sympatico.ca>
On Sat, 6 May 2006 14:14:02 -0400
sean <seanlkml@sympatico.ca> wrote:
> If the variable we need to store should go into a section
> that currently only has a single variable (not matching
> the one we're trying to insert), we will already be into
> the next section before we notice we've bypassed the correct
> location to insert the variable.
>
> To handle this case we store the current location as soon
> as we find a variable matching the section of our new
> variable.
>
Sorry.. this should really be amended to mention that it was Linus
who spotted the problem.
Sean
^ permalink raw reply
* [PATCH] Another config file parsing fix.
From: sean @ 2006-05-06 18:14 UTC (permalink / raw)
To: git; +Cc: Linus Torvalds
If the variable we need to store should go into a section
that currently only has a single variable (not matching
the one we're trying to insert), we will already be into
the next section before we notice we've bypassed the correct
location to insert the variable.
To handle this case we store the current location as soon
as we find a variable matching the section of our new
variable.
Signed-off-by: Sean Estabrooks <seanlkml@sympatico.ca>
---
config.c | 4 +++-
1 files changed, 3 insertions(+), 1 deletions(-)
1ba487db7393d773a2a4b7c404ba1b807272eb7d
diff --git a/config.c b/config.c
index 87fb220..41066e4 100644
--- a/config.c
+++ b/config.c
@@ -336,8 +336,10 @@ static int store_aux(const char* key, co
store.state = KEY_SEEN;
store.seen++;
} else if (strrchr(key, '.') - key == store.baselen &&
- !strncmp(key, store.key, store.baselen))
+ !strncmp(key, store.key, store.baselen)) {
store.state = SECTION_SEEN;
+ store.offset[store.seen] = ftell(config_file);
+ }
}
return 0;
}
--
1.3.2.g6e99a-dirty
^ permalink raw reply related
* Re: Unresolved issues #2
From: Linus Torvalds @ 2006-05-06 17:20 UTC (permalink / raw)
To: sean, Johannes Schindelin; +Cc: Junio C Hamano, barkalow, Git Mailing List
In-Reply-To: <BAYC1-PASMTP0824AA77198F95FE28B79DAEAA0@CEZ.ICE>
On Sat, 6 May 2006, sean wrote:
>
> Okay, I mistook the scope of you comments to apply to all of git rather than
> as a reminder that we can't forget about the toolkit design. So I take it
> you're not at all against git including higher level user commands; just so
> long as they're built on top of lower level toolkit commands that other
> porcelain can use as well.
Correct. I think we've been able to handle that balance particularly well
so far. Or maybe the porcelains don't complain enough.
> In this particular case I see "git repo-config" as the low level command that
> any porcelain can use to access the remotes information and the proposed
> "git remotes" as a simple convenience wrapper on top of this. Of course,
> everyone has to agree on the config file format; but that is true whether
> the human-friendly wrapper exists or not.
I agree, but my point is that in order for a porcelain to _use_
"repo-config", the config file format needs to be defined somewhere, and
we need to tell people that it's not changing. Are we there yet?
That was my argument for why we should concentrate not on what the user
wrapper should be named, but why we should look at what the low-level
meaning of these things are.
Finally, I think "git repo-config" is buggy. Try with this .config file:
[user]
name = Bozo the Clown
email = bozo@circus.com
[core]
filemode = true
[merge]
summary = true
and then do
git repo-config core.gitproxy 'dummy example'
and look where it ends up. For me, it ends up at the end, in the "[merge]"
section, which is obviously bogus.
So we'd really be screwing with porcelain if we made them use this ;)
Linus
^ permalink raw reply
* Re: Unresolved issues #2
From: sean @ 2006-05-06 16:53 UTC (permalink / raw)
To: Linus Torvalds; +Cc: junkio, barkalow, git
In-Reply-To: <Pine.LNX.4.64.0605060923050.16343@g5.osdl.org>
On Sat, 6 May 2006 09:30:48 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:
> Basically, it boils down to the end result.
>
> If you design things for "people", then things tend to become hard to
> automate, and it's hard to make wrappers around it. Maybe you've even made
> the interfaces interactive, and thus any wrappers around it are simply
> screwed, or need to do insane things.
Okay, I mistook the scope of you comments to apply to all of git rather than
as a reminder that we can't forget about the toolkit design. So I take it
you're not at all against git including higher level user commands; just so
long as they're built on top of lower level toolkit commands that other
porcelain can use as well.
In this particular case I see "git repo-config" as the low level command that
any porcelain can use to access the remotes information and the proposed
"git remotes" as a simple convenience wrapper on top of this. Of course,
everyone has to agree on the config file format; but that is true whether
the human-friendly wrapper exists or not.
Sean
^ permalink raw reply
* Re: Unresolved issues #2
From: Linus Torvalds @ 2006-05-06 16:30 UTC (permalink / raw)
To: sean; +Cc: junkio, barkalow, git
In-Reply-To: <BAYC1-PASMTP10F63ADF30C26A29D070C5AEAA0@CEZ.ICE>
On Sat, 6 May 2006, sean wrote:
>
> Wondering why you feel so strongly that most "users" shouldn't be real people.
> What is wrong with continuing to make git easier for developers to use without
> needing any extra software?
Basically, it boils down to the end result.
If you design things for "people", then things tend to become hard to
automate, and it's hard to make wrappers around it. Maybe you've even made
the interfaces interactive, and thus any wrappers around it are simply
screwed, or need to do insane things.
On the other hand, if you design things for automation, doing a "people
wrapper" that uses the automation should be trivial if the design is even
remotely any good at all.
In other words: you should always design things for automation, and
consider the "people interface" to be be just _one_ wrapper layer among
many.
This has worked really well in git. The whole system was designed from the
start to be all about scripting and automation, and the "people wrappers"
tend to be trivial scripts around it.
This was even more obvious when we had a number of basically one-liner
scripts like "git log", which just did some trivial wrapping around
git-rev-list | git-diff-tree --stdin | $PAGER
(Now we still have that trivial wrapper, but you just need to look into C
code to see it, so it's not _as_ obviously trivial).
Contrast this with going the other way: if you talk about the interfaces
that _people_ want first, you immediately start doing pretty-printing,
nice parsing, maybe interactive stuff that asks questions. Nice GUIs. And
the end result is CRAP. Exactly because it lost its ability to be generic.
To some degree, this is the fundamental difference between the Windows and
the UNIX mindset. At least it used to be.
Linus
^ permalink raw reply
* Re: Unresolved issues #2
From: sean @ 2006-05-06 15:35 UTC (permalink / raw)
To: Linus Torvalds; +Cc: junkio, barkalow, git
In-Reply-To: <Pine.LNX.4.64.0605060821430.16343@g5.osdl.org>
On Sat, 6 May 2006 08:26:36 -0700 (PDT)
Linus Torvalds <torvalds@osdl.org> wrote:
> I _personally_ care about the semantics, but not very deeply - since I
> tend to actually have just one main branch, and a couple of throw-away
> ones if I ended up working on something.
>
> But I think that for this thing to become useful, we want to care about
> the format - or at least the interface to the different users (with the
> acknowledgement that "users" should often be porcelain above us).
>
> Right now we've basically had people hand-editing the remotes files, and I
> think cogito still uses the older branches format that came from cogito in
> the first place. I think we should just try to decide on a config file
> format, and make it easy for cogito etc to use it.
Linus,
Wondering why you feel so strongly that most "users" shouldn't be real people.
What is wrong with continuing to make git easier for developers to use without
needing any extra software?
Sean
^ permalink raw reply
* Re: Unresolved issues #2
From: Linus Torvalds @ 2006-05-06 15:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Daniel Barkalow, Git Mailing List
In-Reply-To: <7vhd43vgnm.fsf@assigned-by-dhcp.cox.net>
On Fri, 5 May 2006, Junio C Hamano wrote:
>
> > So I'd argue that (a) yes, we do want to have the "proto porcelain" that
> > sets remote branch information without the user having to know the magic
> > "git repo-config" incantation, or know which file in .git/remotes/ to
> > edit, but that (b) it's even more important to try to decide on what the
> > remote description format _is_.
>
> Is it format you care about or the semantics?
I _personally_ care about the semantics, but not very deeply - since I
tend to actually have just one main branch, and a couple of throw-away
ones if I ended up working on something.
But I think that for this thing to become useful, we want to care about
the format - or at least the interface to the different users (with the
acknowledgement that "users" should often be porcelain above us).
Right now we've basically had people hand-editing the remotes files, and I
think cogito still uses the older branches format that came from cogito in
the first place. I think we should just try to decide on a config file
format, and make it easy for cogito etc to use it.
Linus
^ permalink raw reply
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox