Re: [PATCH] repack: add `repack.honorpackkeep` config var
Jeff King p...@peff.net writes: The git-repack command always passes `--honor-pack-keep` to pack-objects. This has traditionally been a good thing, as we do not want to duplicate those objects in a new pack, and we are not going to delete the old pack. ... Note that this option just disables the pack-objects behavior. We still leave packs with a .keep in place, as we do not necessarily know that we have duplicated all of their objects. Signed-off-by: Jeff King p...@peff.net --- Intended for the jk/pack-bitmap topic. Two comments. - It seems that this adds a configuration variable that cannot be countermanded from the command line. It has to come with either a very good justification in the documentation describing why it is a bad idea to even allow overriding from the command line in a repository that sets it, or a command line option to let the users override it. I personally prefer the latter, because that will be one less thing for users to remember (i.e. usually you can override the configured default from the command line, but this variable cannot be because of these very good reasons). - In the context of pack-objects, the name --honor-pack-keep makes sense; it is understood that pack-objects will _not_ remove kept packfile, so honoring can only mean do not attempt to pick objects out of kept packs to add to the pack being generated. and there is no room for --no-honor-pack-keep to be mistaken as you canremove the ones marked to be kept after saving the still-used objects in it away. But does the same name make sense in the context of repack? Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Running make rpm fails on a CentOS 6.3 machine
Hi,
I'm trying to build the git RPM (using tag v1.8.5.3) on a CentOS 6.3
64 bit machine. I was able to run 'make', but then I fail when running
'make rpm'. Can anyone help with the following error?
[erez.zilber@erez-lx:~/work/
git]$ make rpm
sed -e 's/@@VERSION@@/1.8.5.3/g' git.spec.in git.spec+
mv git.spec+ git.spec
GEN configure
./git-archive --format=tar \
--prefix=git-1.8.5.3/ HEAD^{tree} git-1.8.5.3.tar
make[1]: Entering directory `/khome/erez.zilber/work/git/git-gui'
make[1]: Leaving directory `/khome/erez.zilber/work/git/git-gui'
tar rf git-1.8.5.3.tar \
git-1.8.5.3/git.spec \
git-1.8.5.3/configure \
git-1.8.5.3/version \
git-1.8.5.3/git-gui/version
gzip -f -9 git-1.8.5.3.tar
rpmbuild \
--define _source_filedigest_algorithm md5 \
--define _binary_filedigest_algorithm md5 \
-ta git-1.8.5.3.tar.gz
Executing(%prep): /bin/sh -e /var/tmp/rpm-tmp.GmHLGn
+ umask 022
+ cd /khome/erez.zilber/rpmbuild/BUILD
+ cd /khome/erez.zilber/rpmbuild/BUILD
+ rm -rf git-1.8.5.3
+ /usr/bin/gzip -dc /khome/erez.zilber/work/git/git-1.8.5.3.tar.gz
+ /bin/tar -xf -
+ STATUS=0
+ '[' 0 -ne 0 ']'
+ cd git-1.8.5.3
+ /bin/chmod -Rf a+rX,u+w,g-w,o-w .
+ exit 0
Executing(%build): /bin/sh -e /var/tmp/rpm-tmp.aVAwKk
+ umask 022
+ cd /khome/erez.zilber/rpmbuild/BUILD
+ cd git-1.8.5.3
+ make -j8 'CFLAGS=-O2 -g' ETC_GITCONFIG=/etc/gitconfig prefix=/usr
mandir=/usr/share/man htmldir=/usr/share/doc/git-1.8.5.3 all doc
make[1]: Entering directory `/khome/erez.zilber/rpmbuild/BUILD/git-1.8.5.3'
GIT_VERSION = 1.8.5.3
make[1]: Leaving directory `/khome/erez.zilber/rpmbuild/BUILD/git-1.8.5.3'
make[1]: Entering directory `/khome/erez.zilber/rpmbuild/BUILD/git-1.8.5.3'
...
Writing perl.mak for Git
Writing perl.mak for Git
rename MakeMaker.tmp = perl.mak: No such file or directory at
/usr/share/perl5/ExtUtils/MakeMaker.pm line 1024.
make[3]: perl.mak: No such file or directory
make[3]: perl.mak: No such file or directory
make[3]: *** No rule to make target `perl.mak'. Stop.
make[2]: *** [instlibdir] Error 2
make[1]: *** [git-difftool] Error 2
make[1]: *** Waiting for unfinished jobs
make[3]: *** No rule to make target `perl.mak'. Stop.
make[2]: *** [instlibdir] Error 2
make[1]: *** [git-add--interactive] Error 2
Writing perl.mak for Git
Writing perl.mak for Git
Writing perl.mak for Git
rename MakeMaker.tmp = perl.mak: No such file or directory at
/usr/share/perl5/ExtUtils/MakeMaker.pm line 1024.
make[3]: perl.mak: No such file or directory
make[3]: perl.mak: No such file or directory
make[3]: *** No rule to make target `perl.mak'. Stop.
make[2]: *** [instlibdir] Error 2
make[1]: *** [git-cvsexportcommit] Error 2
make[3]: *** No rule to make target `instlibdir'. Stop.
make[2]: *** [instlibdir] Error 2
make[1]: *** [git-cvsserver] Error 2
Writing perl.mak for Git
mv: cannot stat `perl.mak': No such file or directory
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP v2 00/14] inotify support
On Mon, Jan 20, 2014 at 12:04 AM, Thomas Rast t...@thomasrast.ch wrote: There's also the problem of ordering guarantees between the socket and inotify. I haven't found any, so I would conservatively assume that the socket messages may in fact arrive before inotify, which is a race in the current code. E.g., in the sequence 'touch foo; git status' the daemon may see socketinotify hello... status new empty list touch foo I think a clever way to handle this would be to add a new command: Wait:: This command serves synchronization. Git creates a file of its choice in $GIT_DIR/watch (say, `.git/watch/wait.random`). Then it sends wait path. The watcher MUST block until it has processed all change notifications up to and including path. Assuming that the time between foo is touched and the time an event is put in the daemon's queue is reasonably small, would emptying the event queue at hello be enough? To my innocent eyes (at the kernel), it seems inotify handling happens immediately after an fs event, and it's uninterruptable (or at least not interruptable by another user space process, I don't think we need to care about true interrupts). If that's true, by the time the touch syscall is finished, the event is already sitting in the daemon's queue. The problem with wait.random is we need to tell the daemon to expect it. Otherwise if the daemon processes the wait.random even before wait is sent, it would try to wait for the (lost) wait.random event forever. An extension is git touch wait.random regularly. Or to keep a queue of processed wait.* events. Both look ugly imo. Another option is send expect wait.random first, wait for ack, touch wait.random, then send wait, which is too much. -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v1.9-rc0
On Mon, Jan 27, 2014 at 10:58:29AM -0800, Jonathan Nieder wrote: Hi, Kacper Kornet wrote: The change in release numbering also breaks down gitolite v2 setups. One of the gitolite commands, gl-compile-conf, expects the output of git --version to match /git version (\d+)\.(\d+)\.(\d+)/. I have no idea how big problem it is, as I don't know how many people hasn't migrate to gitolite v3 yet. http://qa.debian.org/popcon.php?package=gitolite says there are some. I guess soon we'll see if there are complaints. http://gitolite.com/gitolite/migr.html says gitolite v2 is still maintained. Hopefully the patch to gitolite v2 to fix this would not be too invasive --- e.g., how about this patch (untested)? Thanks, Jonathan diff --git i/src/gl-compile-conf w/src/gl-compile-conf index f497ae5..8508313 100755 --- i/src/gl-compile-conf +++ w/src/gl-compile-conf @@ -394,8 +394,9 @@ die the server. If it is not, please edit ~/.gitolite.rc on the server and set the \$GIT_PATH variable to the correct value\n unless $git_version; -my ($gv_maj, $gv_min, $gv_patchrel) = ($git_version =~ m/git version (\d+)\.(\d+)\.(\d+)/); +my ($gv_maj, $gv_min, $gv_patchrel) = ($git_version =~ m/git version (\d+)\.(\d+)\.([^.-]*)/); die $ABRT I can't understand $git_version\n unless ($gv_maj = 1); +$gv_patchrel = 0 unless ($gv_patchrel =~ m/^\d+$/); $git_version = $gv_maj*1 + $gv_min*100 + $gv_patchrel; # now it's normalised die \n\t\t* AAARGH! *\n . It works for 1.9.rc1 but I think it will fail with final 1.9. The following version should be ok: diff --git a/src/gl-compile-conf b/src/gl-compile-conf index f497ae5..c391468 100755 --- a/src/gl-compile-conf +++ b/src/gl-compile-conf @@ -394,7 +394,7 @@ die the server. If it is not, please edit ~/.gitolite.rc on the server and set the \$GIT_PATH variable to the correct value\n unless $git_version; -my ($gv_maj, $gv_min, $gv_patchrel) = ($git_version =~ m/git version (\d+)\.(\d+)\.(\d+)/); +my ($gv_maj, $gv_min, undef, $gv_patchrel) = ($git_version =~ m/git version (\d+)\.(\d+)(\.(\d+))?/); die $ABRT I can't understand $git_version\n unless ($gv_maj = 1); $git_version = $gv_maj*1 + $gv_min*100 + $gv_patchrel; # now it's normalised -- Kacper -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Having Git follow symlinks
Hi! Is there a (per-repo) setting to get Git to follow symlinks in the working directory, i.e., to not store the symlinks themselves but rather work on what they point to? Background: I have a repository that stores a number of my dotfiles, shared between all my machines (Linux, OSX, Windows/CygWin, Solaris). It is currently a CVS repo that I wish to convert to Git since CVS is getting more and more scarce. However, I have the repo set up so that I check it out into a subdirectory of its own, and have symlinks (junctions on Windows) both coming into it (for files that live in ~) and out of it (for subdirectories of ~ that cannot be symlinks themselves, such as ~/.ssh, or that live elsewhere, such as under AppData on Windows or ~/Library on MacOS). CVS handles this by simply not knowing anything about symlinks, and I would like to get Git to do the same. I could probably get away with junctions on Windows and directory hardlinks on OSX, but that would not work on Linux. -- \\// Peter - http://www.softwolves.pp.se/ -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: Having Git follow symlinks
On Tue, Jan 28, 2014 at 2:49 PM, Peter Krefting pe...@softwolves.pp.se wrote: Is there a (per-repo) setting to get Git to follow symlinks in the working directory, i.e., to not store the symlinks themselves but rather work on what they point to? Not that I know of. Background: I have a repository that stores a number of my dotfiles, shared between all my machines (Linux, OSX, Windows/CygWin, Solaris). It is currently a CVS repo that I wish to convert to Git since CVS is getting more and more scarce. However, I have the repo set up so that I check it out into a subdirectory of its own, and have symlinks (junctions on Windows) both coming into it (for files that live in ~) and out of it (for subdirectories of ~ that cannot be symlinks themselves, such as ~/.ssh, or that live elsewhere, such as under AppData on Windows or ~/Library on MacOS). CVS handles this by simply not knowing anything about symlinks, and I would like to get Git to do the same. I believe a preferable way to manage dotfiles in Git, is to have a script that does the necessary setup/installation from the repo (that lives in some subdirectory of ~) and into ~. This script would be able to: - Set up whatever symlinks or copies are needed - Apply permission/mode bits that are not stored by Git - Properly handle various platform differences (symlinks vs. junctions, etc.) As a bonus, you can run the script as a post-checkout hook, to have it automatically apply any updates you fetch/push into your dotfiles repo. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] combine-diff: Optimize combine_diff_path sets intersection
On Mon, Jan 20, 2014 at 08:20:40PM +0400, Kirill Smelkov wrote:
[...]
@@ -1343,6 +1374,26 @@ void diff_tree_combined(const unsigned char *sha1,
if (p-len)
num_paths++;
}
+
+ /* order paths according to diffcore_order */
+ if (opt-orderfile num_paths) {
+ struct obj_order *o;
+
+ o = xmalloc(sizeof(*o) * num_paths);
+ for (i = 0, p = paths; p; p = p-next, i++)
+ o[i].obj = p;
+ order_objects(opt-orderfile, path_path, o, num_paths);
+ for (i = 0; i num_paths - 1; i++) {
+ p = o[i].obj;
+ p-next = o[i+1].obj;
+ }
+
+ p = o[num_paths-1].obj;
+ p-next = NULL;
+ paths = o[0].obj;
+ }
I found I've introduced memory leak here (malloc without free). Please
apply the fix. Thanks, Kirill.
8
From: Kirill Smelkov k...@mns.spb.ru
Date: Tue, 28 Jan 2014 19:39:16 +0400
Subject: [PATCH] fixup! combine-diff: Optimize combine_diff_path sets
intersection
Plug a memory leak.
---
combine-diff.c | 1 +
1 file changed, 1 insertion(+)
diff --git a/combine-diff.c b/combine-diff.c
index 07faa96..2d79312 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1383,6 +1383,7 @@ void diff_tree_combined(const unsigned char *sha1,
p = o[num_paths-1].obj;
p-next = NULL;
paths = o[0].obj;
+ free(o);
}
--
1.9.rc1.181.g641f458
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: Check argc before using argv[i+1]
David Sharp dhsh...@google.com writes:
@@ -738,9 +740,11 @@ int cmd_rev_parse(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --resolve-git-dir)) {
- const char *gitdir = resolve_gitdir(argv[i+1]);
+ if (++i = argc)
+ die(--resolve-git-dir requires an
argument);
+ const char *gitdir = resolve_gitdir(argv[i]);
if (!gitdir)
- die(not a gitdir '%s', argv[i+1]);
+ die(not a gitdir '%s', argv[i]);
This adds decl-after-statement. As referencing argv[argc] is not a
crime (you will grab a NULL), how about doing it this way to see if
the value is a NULL, instead of comparing the index and argc?
const char *gitdir;
if (!argv[++i])
die(--resolve-git-dir requires an argument);
gitdir = resolve_gitdir(argv[i]);
if (!gitdir)
die(not a gitdir '%s', argv[i]);
Same comment may apply to other hunks.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH v2] rev-parse: Check argc before using argv[i+1]
Without this patch, git-rev-parse --prefix, --default, or
--resolve-git-dir, without a value argument, would result in a segfault.
Instead, die() with a message.
Signed-off-by: David Sharp dhsh...@google.com
---
builtin/rev-parse.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..45901df 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
continue;
}
if (!strcmp(arg, --default)) {
- def = argv[i+1];
- i++;
+ def = argv[++i];
+ if (!def)
+ die(--default requires an argument);
continue;
}
if (!strcmp(arg, --prefix)) {
- prefix = argv[i+1];
+ prefix = argv[++i];
+ if (!prefix)
+ die(--prefix requires an argument);
startup_info-prefix = prefix;
output_prefix = 1;
- i++;
continue;
}
if (!strcmp(arg, --revs-only)) {
@@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const char
*prefix)
continue;
}
if (!strcmp(arg, --resolve-git-dir)) {
- const char *gitdir = resolve_gitdir(argv[i+1]);
+ const char *gitdir = argv[++i];
if (!gitdir)
- die(not a gitdir '%s', argv[i+1]);
+ die(--resolve-git-dir requires an
argument);
+ gitdir = resolve_gitdir(gitdir);
+ if (!gitdir)
+ die(not a gitdir '%s', argv[i]);
puts(gitdir);
continue;
}
--
1.8.5.3
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH] rev-parse: Check argc before using argv[i+1]
On Tue, Jan 28, 2014 at 11:12 AM, Junio C Hamano gits...@pobox.com wrote:
David Sharp dhsh...@google.com writes:
@@ -738,9 +740,11 @@ int cmd_rev_parse(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --resolve-git-dir)) {
- const char *gitdir = resolve_gitdir(argv[i+1]);
+ if (++i = argc)
+ die(--resolve-git-dir requires an
argument);
+ const char *gitdir = resolve_gitdir(argv[i]);
if (!gitdir)
- die(not a gitdir '%s', argv[i+1]);
+ die(not a gitdir '%s', argv[i]);
This adds decl-after-statement.
Sorry, I wasn't aware that git is trying to conform to C90. (It's not
compiled with -std=c90, -ansi or -Wdeclaration-after-statement.)
As referencing argv[argc] is not a
crime (you will grab a NULL), how about doing it this way to see if
the value is a NULL, instead of comparing the index and argc?
I'm not convinced this is any better, but alright. I did something
slightly different based on that, though.
v2 patch incoming...
const char *gitdir;
if (!argv[++i])
die(--resolve-git-dir requires an argument);
gitdir = resolve_gitdir(argv[i]);
if (!gitdir)
die(not a gitdir '%s', argv[i]);
Same comment may apply to other hunks.
Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
David Sharp dhsh...@google.com writes:
Without this patch, git-rev-parse --prefix, --default, or
--resolve-git-dir, without a value argument, would result in a segfault.
Instead, die() with a message.
When I sent the review message, I actually was on the fence between
checking i vs argc and checking the nullness myself. I realize,
after seeing the actual patch below, that we are protecting against
picking up a NULL and blindly passing it on in the codepaths that
follow, so the updated code does look a lot better, at least to me.
Thanks.
Signed-off-by: David Sharp dhsh...@google.com
---
builtin/rev-parse.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..45901df 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --default)) {
- def = argv[i+1];
- i++;
+ def = argv[++i];
+ if (!def)
+ die(--default requires an argument);
continue;
}
if (!strcmp(arg, --prefix)) {
- prefix = argv[i+1];
+ prefix = argv[++i];
+ if (!prefix)
+ die(--prefix requires an argument);
startup_info-prefix = prefix;
output_prefix = 1;
- i++;
continue;
}
if (!strcmp(arg, --revs-only)) {
@@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --resolve-git-dir)) {
- const char *gitdir = resolve_gitdir(argv[i+1]);
+ const char *gitdir = argv[++i];
if (!gitdir)
- die(not a gitdir '%s', argv[i+1]);
+ die(--resolve-git-dir requires an
argument);
+ gitdir = resolve_gitdir(gitdir);
+ if (!gitdir)
+ die(not a gitdir '%s', argv[i]);
puts(gitdir);
continue;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/4] combine-diff: Optimize combine_diff_path sets intersection
Kirill Smelkov k...@mns.spb.ru writes:
diff --git a/combine-diff.c b/combine-diff.c
index 3b92c448..98c2562 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -15,8 +15,8 @@
...
+ while (1) {
...
+ if (cmp 0) {
+ if (pprev)
+ pprev-next = p-next;
+ ptmp = p;
+ p = p-next;
+ free(ptmp);
+ if (curr == ptmp)
+ curr = p;
continue;
...
+ if (cmp 0) {
+ i++;
+ continue;
}
...
+
+ pprev = p;
+ p = p-next;
+ i++;
}
return curr;
}
Thanks. I very much like the approach.
I was staring at the above part of the code, but couldn't help
recalling this gem (look for understand pointers in the article):
http://meta.slashdot.org/story/12/10/11/0030249/linus-torvalds-answers-your-questions
How about doing it this way (on top of your patch)? It reduces 7
lines even though it adds two comment lines ;-)
combine-diff.c | 37 +++--
1 file changed, 15 insertions(+), 22 deletions(-)
diff --git a/combine-diff.c b/combine-diff.c
index 2d79312..0809e79 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -15,11 +15,10 @@
static struct combine_diff_path *intersect_paths(struct combine_diff_path
*curr, int n, int num_parent)
{
struct diff_queue_struct *q = diff_queued_diff;
- struct combine_diff_path *p, *pprev, *ptmp;
+ struct combine_diff_path *p, **tail = curr;
int i, cmp;
if (!n) {
- struct combine_diff_path *list = NULL, **tail = list;
for (i = 0; i q-nr; i++) {
int len;
const char *path;
@@ -43,35 +42,30 @@ static struct combine_diff_path *intersect_paths(struct
combine_diff_path *curr,
*tail = p;
tail = p-next;
}
- return list;
+ return curr;
}
/*
-* NOTE paths are coming sorted here (= in tree order)
+* paths in curr (linked list) and q-queue[] (array) are
+* both sorted in the tree order.
*/
-
- pprev = NULL;
- p = curr;
i = 0;
+ while ((p = *tail) != NULL) {
+ cmp = ((i = q-nr)
+ ? -1 : strcmp(p-path, q-queue[i]-two-path));
- while (1) {
- if (!p)
- break;
-
- cmp = (i = q-nr) ? -1
- : strcmp(p-path, q-queue[i]-two-path);
if (cmp 0) {
- if (pprev)
- pprev-next = p-next;
- ptmp = p;
- p = p-next;
- free(ptmp);
- if (curr == ptmp)
- curr = p;
+ /* p-path not in q-queue[]; drop it */
+ struct combine_diff_path *next = p-next;
+
+ if ((*tail = next) != NULL)
+ tail = next-next;
+ free(p);
continue;
}
if (cmp 0) {
+ /* q-queue[i] not in p-path; skip it */
i++;
continue;
}
@@ -80,8 +74,7 @@ static struct combine_diff_path *intersect_paths(struct
combine_diff_path *curr,
p-parent[n].mode = q-queue[i]-one-mode;
p-parent[n].status = q-queue[i]-status;
- pprev = p;
- p = p-next;
+ tail = p-next;
i++;
}
return curr;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
On Tue, Jan 28, 2014 at 1:43 PM, Junio C Hamano gits...@pobox.com wrote:
David Sharp dhsh...@google.com writes:
Without this patch, git-rev-parse --prefix, --default, or
--resolve-git-dir, without a value argument, would result in a segfault.
Instead, die() with a message.
When I sent the review message, I actually was on the fence between
checking i vs argc and checking the nullness myself. I realize,
after seeing the actual patch below, that we are protecting against
picking up a NULL and blindly passing it on in the codepaths that
follow, so the updated code does look a lot better, at least to me.
It's a matter of perspective, I guess. When checking the index, to me,
I saw that it is protecting against reading past the end of an array,
which is at least as bad as sending NULL to codepaths that don't
accept NULL. I do think that everybody knows that reading past the end
of an array is bad, while not necessarily everybody knows that the
argv array is null-terminated.
Thanks.
Signed-off-by: David Sharp dhsh...@google.com
---
builtin/rev-parse.c | 17 +++--
1 file changed, 11 insertions(+), 6 deletions(-)
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..45901df 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -547,15 +547,17 @@ int cmd_rev_parse(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --default)) {
- def = argv[i+1];
- i++;
+ def = argv[++i];
+ if (!def)
+ die(--default requires an argument);
continue;
}
if (!strcmp(arg, --prefix)) {
- prefix = argv[i+1];
+ prefix = argv[++i];
+ if (!prefix)
+ die(--prefix requires an argument);
startup_info-prefix = prefix;
output_prefix = 1;
- i++;
continue;
}
if (!strcmp(arg, --revs-only)) {
@@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --resolve-git-dir)) {
- const char *gitdir = resolve_gitdir(argv[i+1]);
+ const char *gitdir = argv[++i];
if (!gitdir)
- die(not a gitdir '%s', argv[i+1]);
+ die(--resolve-git-dir requires an
argument);
+ gitdir = resolve_gitdir(gitdir);
+ if (!gitdir)
+ die(not a gitdir '%s', argv[i]);
puts(gitdir);
continue;
}
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2] rev-parse: Check argc before using argv[i+1]
On Tue, Jan 28, 2014 at 2:01 PM, Johannes Sixt j...@kdbg.org wrote:
Am 28.01.2014 22:21, schrieb David Sharp:
@@ -738,9 +740,12 @@ int cmd_rev_parse(int argc, const char **argv, const
char *prefix)
continue;
}
if (!strcmp(arg, --resolve-git-dir)) {
- const char *gitdir = resolve_gitdir(argv[i+1]);
+ const char *gitdir = argv[++i];
if (!gitdir)
- die(not a gitdir '%s', argv[i+1]);
+ die(--resolve-git-dir requires an
argument);
+ gitdir = resolve_gitdir(gitdir);
+ if (!gitdir)
+ die(not a gitdir '%s', argv[i]);
puts(gitdir);
continue;
}
In this hunk, I don't see where the old code incremented i for the
argument. Was this another bug lurking that is fixed now?
Yup. I did notice that too. Should have mentioned it in the description.
-- Hannes
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
Junio C Hamano gits...@pobox.com writes:
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes:
skip_stat_unmatch flag is added in fb13227 (git-diff: squelch empty
diffs - 2007-08-03) to ignore empty diffs caused by stat-only
dirtiness. In some diff case, stat is not involved at all. While
the code is written in a way that no expensive I/O is done, we still
need to move all file pairs from the old queue to the new queue in
diffcore_skip_stat_unmatch().
Only enable it when worktree is involved: diff and diff rev.
This should help track down how skip_stat_unmatch is actually used
when bugs occur.
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
This replaces 'diff: turn off skip_stat_unmatch on diff --cached'
The previous patch obviously leaves skip_stat_unmatch on in diff
rev rev and maybe other cases.
Oops, I lost track. Sorry.
Together with {1,2}/3 applied on maint-1.8.4, this sems to break
t3417 (there may be others, but I didn't have time to check).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH v2 3/3] diff: turn skip_stat_unmatch on selectively
On Tue, Jan 28, 2014 at 02:51:45PM -0800, Junio C Hamano wrote:
This replaces 'diff: turn off skip_stat_unmatch on diff --cached'
The previous patch obviously leaves skip_stat_unmatch on in diff
rev rev and maybe other cases.
Oops, I lost track. Sorry.
Together with {1,2}/3 applied on maint-1.8.4, this sems to break
t3417 (there may be others, but I didn't have time to check).
My bad. I thought I covered all cases in my last patch (and didn't
retest it!). It turns out I should have set skip_stat_unmatch in
builtin_diff_b_f() too. This on top of 3/3 passes the tests
-- 8 --
diff --git a/builtin/diff.c b/builtin/diff.c
index 88542d9..8ab5e3d 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -89,6 +89,7 @@ static int builtin_diff_b_f(struct rev_info *revs,
if (blob[0].mode == S_IFINVALID)
blob[0].mode = canon_mode(st.st_mode);
+ revs-diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
stuff_change(revs-diffopt,
blob[0].mode, canon_mode(st.st_mode),
blob[0].sha1, null_sha1,
-- 8 --
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
inotify support, nearly there
Just a quick update for the enthusiasts. My branch file-watcher [1] has got working per-user inotify support. It's a 20 patch series so I'll refrain from spamming git@vger for a while, even though it hurts your eyes a lot less than what I have posted so far. The test suite ran fine with it so it's not that buggy. It has new tests too, even though real inotify is not tested in the new tests. Documentation is there, either in .txt or comments. Using it is simple: $ mkdir ~/.watcher $ git file-watcher --detach ~/.watcher $ git config --global filewatcher.path $HOME/.watcher There's still some polishing work to do. But I think the core logic is done. I have some ideas what to be polished, but I'd appreciate feedback if anyone uses it. We may need to make lookup code faster later. MacOS, FreeBSD and Windows contributors. If you have time and are interested, have a look at the protocol, which is basically documented in file-watcher.c:handle_command(), and see if something is incompatible with your file notification mechanism. MacOS and FreeBSD may reuse code from file-watcher.c, at least the unix socket part. I'm not so sure about Windows. It probably needs a brand new daemon because little could be shared, I don't know. I deliberately design the daemon dumb so writing a completely new one won't be so hard. My plan is focus on inotify and get it merged first, then new OS support can come later (with refactoring if needed, but should not change the protocol drastically). [1] git clone https://github.com/pclouds/git.git file-watcher -- Duy -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: inotify support, nearly there
On Wed, Jan 29, 2014 at 01:47:30PM +0700, Duy Nguyen wrote: Just a quick update for the enthusiasts. My branch file-watcher [1] has got working per-user inotify support. It's a 20 patch series so I'll refrain from spamming git@vger for a while, even though it hurts your eyes a lot less than what I have posted so far. The test suite ran fine with it so it's not that buggy. It has new tests too, even though real inotify is not tested in the new tests. Documentation is there, either in .txt or comments. Using it is simple: $ mkdir ~/.watcher $ git file-watcher --detach ~/.watcher $ git config --global filewatcher.path $HOME/.watcher There's still some polishing work to do. But I think the core logic is done. I have some ideas what to be polished, but I'd appreciate feedback if anyone uses it. We may need to make lookup code faster later. MacOS, FreeBSD and Windows contributors. If you have time and are interested, have a look at the protocol, which is basically documented in file-watcher.c:handle_command(), and see if something is incompatible with your file notification mechanism. MacOS and FreeBSD may reuse code from file-watcher.c, at least the unix socket part. I'm not so sure about Windows. It probably needs a brand new daemon because little could be shared, I don't know. I deliberately design the daemon dumb so writing a completely new one won't be so hard. My plan is focus on inotify and get it merged first, then new OS support can come later (with refactoring if needed, but should not change the protocol drastically). [1] git clone https://github.com/pclouds/git.git file-watcher Haven't looked at the code, so I don't know if you've done that, but in case you haven't, it would be nice to have an environment variable or a config option to make git use the file-watcher *and* normal lstat operations, to check consistency. Mike -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
