[PATCH v5 2/2] Revert compate/clipped-write.c: large write(2) fails on Mac OS X/XNU
The previous commit introduced a size limit on IO chunks on all
platforms. The compat clipped_write() is not needed anymore.
This reverts commit 6c642a878688adf46b226903858b53e2d31ac5c3.
Signed-off-by: Steffen Prohaska proha...@zib.de
---
Makefile | 8
compat/clipped-write.c | 13 -
config.mak.uname | 1 -
git-compat-util.h | 5 -
4 files changed, 27 deletions(-)
delete mode 100644 compat/clipped-write.c
diff --git a/Makefile b/Makefile
index 3588ca1..4026211 100644
--- a/Makefile
+++ b/Makefile
@@ -69,9 +69,6 @@ all::
# Define NO_MSGFMT_EXTENDED_OPTIONS if your implementation of msgfmt
# doesn't support GNU extensions like --check and --statistics
#
-# Define NEEDS_CLIPPED_WRITE if your write(2) cannot write more than
-# INT_MAX bytes at once (e.g. MacOS X).
-#
# Define HAVE_PATHS_H if you have paths.h and want to use the default PATH
# it specifies.
#
@@ -1493,11 +1490,6 @@ ifndef NO_MSGFMT_EXTENDED_OPTIONS
MSGFMT += --check --statistics
endif
-ifdef NEEDS_CLIPPED_WRITE
- BASIC_CFLAGS += -DNEEDS_CLIPPED_WRITE
- COMPAT_OBJS += compat/clipped-write.o
-endif
-
ifneq (,$(XDL_FAST_HASH))
BASIC_CFLAGS += -DXDL_FAST_HASH
endif
diff --git a/compat/clipped-write.c b/compat/clipped-write.c
deleted file mode 100644
index b8f98ff..000
--- a/compat/clipped-write.c
+++ /dev/null
@@ -1,13 +0,0 @@
-#include ../git-compat-util.h
-#undef write
-
-/*
- * Version of write that will write at most INT_MAX bytes.
- * Workaround a xnu bug on Mac OS X
- */
-ssize_t clipped_write(int fildes, const void *buf, size_t nbyte)
-{
- if (nbyte INT_MAX)
- nbyte = INT_MAX;
- return write(fildes, buf, nbyte);
-}
diff --git a/config.mak.uname b/config.mak.uname
index b27f51d..7d61531 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -95,7 +95,6 @@ ifeq ($(uname_S),Darwin)
NO_MEMMEM = YesPlease
USE_ST_TIMESPEC = YesPlease
HAVE_DEV_TTY = YesPlease
- NEEDS_CLIPPED_WRITE = YesPlease
COMPAT_OBJS += compat/precompose_utf8.o
BASIC_CFLAGS += -DPRECOMPOSE_UNICODE
endif
diff --git a/git-compat-util.h b/git-compat-util.h
index 115cb1d..96d8881 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -185,11 +185,6 @@ typedef unsigned long uintptr_t;
#define probe_utf8_pathname_composition(a,b)
#endif
-#ifdef NEEDS_CLIPPED_WRITE
-ssize_t clipped_write(int fildes, const void *buf, size_t nbyte);
-#define write(x,y,z) clipped_write((x),(y),(z))
-#endif
-
#ifdef MKDIR_WO_TRAILING_SLASH
#define mkdir(a,b) compat_mkdir_wo_trailing_slash((a),(b))
extern int compat_mkdir_wo_trailing_slash(const char*, mode_t);
--
1.8.4.rc3.5.g4f480ff
--
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 v5 0/2] Fix IO of =2GB on Mac OS X by limiting IO chunks
This is the revised patch taking the considerations about IO chunk size into account. The series deletes more than it adds and fixes a bug. Nice. Steffen Prohaska (2): xread, xwrite: Limit size of IO, fixing IO of 2GB and more on Mac OS X Revert compate/clipped-write.c: large write(2) fails on Mac OS X/XNU Makefile | 8 compat/clipped-write.c | 13 - config.mak.uname | 1 - git-compat-util.h | 5 - t/t0021-conversion.sh | 14 ++ wrapper.c | 12 6 files changed, 26 insertions(+), 27 deletions(-) delete mode 100644 compat/clipped-write.c -- 1.8.4.rc3.5.g4f480ff -- 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 v5 1/2] xread, xwrite: Limit size of IO, fixing IO of 2GB and more on Mac OS X
Previously, filtering 2GB or more through an external filter (see test)
failed on Mac OS X 10.8.4 (12E55) for a 64-bit executable with:
error: read from external filter cat failed
error: cannot feed the input to external filter cat
error: cat died of signal 13
error: external filter cat failed 141
error: external filter cat failed
The reason was that read() immediately returns with EINVAL if nbyte =
2GB. According to POSIX [1], if the value of nbyte passed to read() is
greater than SSIZE_MAX, the result is implementation-defined. The write
function has the same restriction [2]. Since OS X still supports
running 32-bit executables, the 32-bit limit (SSIZE_MAX = INT_MAX
= 2GB - 1) seems to be also imposed on 64-bit executables under certain
conditions. For write, the problem has been addressed earlier [6c642a].
This commit addresses the problem for read() and write() by limiting
size of IO chunks unconditionally on all platforms in xread() and
xwrite(). Large chunks only cause problems, like triggering the OS
X bug or causing latencies when killing the process. Reasonably sized
smaller chunks have no negative impact on performance.
The compat wrapper clipped_write() introduced earlier [6c642a] is not
needed anymore. It will be reverted in a separate commit. The new test
catches read and write problems.
Note that 'git add' exits with 0 even if it prints filtering errors to
stderr. The test, therefore, checks stderr. 'git add' should probably
be changed (sometime in another commit) to exit with nonzero if
filtering fails. The test could then be changed to use test_must_fail.
Thanks to the following people for suggestions and testing:
Johannes Sixt j...@kdbg.org
John Keeping j...@keeping.me.uk
Jonathan Nieder jrnie...@gmail.com
Kyle J. McKay mack...@gmail.com
Linus Torvalds torva...@linux-foundation.org
Torsten Bögershausen tbo...@web.de
[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/read.html
[2] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html
[6c642a] commit 6c642a878688adf46b226903858b53e2d31ac5c3
compate/clipped-write.c: large write(2) fails on Mac OS X/XNU
Signed-off-by: Steffen Prohaska proha...@zib.de
---
t/t0021-conversion.sh | 14 ++
wrapper.c | 12
2 files changed, 26 insertions(+)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index e50f0f7..b92e6cb 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -190,4 +190,18 @@ test_expect_success 'required filter clean failure' '
test_must_fail git add test.fc
'
+test -n $GIT_TEST_LONG test_set_prereq EXPENSIVE
+
+test_expect_success EXPENSIVE 'filter large file' '
+ git config filter.largefile.smudge cat
+ git config filter.largefile.clean cat
+ for i in $(test_seq 1 2048); do printf %1048576d 1; done 2GB
+ echo 2GB filter=largefile .gitattributes
+ git add 2GB 2err
+ ! test -s err
+ rm -f 2GB
+ git checkout -- 2GB 2err
+ ! test -s err
+'
+
test_done
diff --git a/wrapper.c b/wrapper.c
index 6a015de..97e3cf7 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -131,6 +131,14 @@ void *xcalloc(size_t nmemb, size_t size)
}
/*
+ * Limit size of IO chunks, because huge chunks only cause pain. OS X 64-bit
+ * buggy, returning EINVAL if len = INT_MAX; and even in the absense of bugs,
+ * large chunks can result in bad latencies when you decide to kill the
+ * process.
+ */
+#define MAX_IO_SIZE (8*1024*1024)
+
+/*
* xread() is the same a read(), but it automatically restarts read()
* operations with a recoverable error (EAGAIN and EINTR). xread()
* DOES NOT GUARANTEE that len bytes is read even if the data is available.
@@ -138,6 +146,8 @@ void *xcalloc(size_t nmemb, size_t size)
ssize_t xread(int fd, void *buf, size_t len)
{
ssize_t nr;
+ if (len MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
while (1) {
nr = read(fd, buf, len);
if ((nr 0) (errno == EAGAIN || errno == EINTR))
@@ -154,6 +164,8 @@ ssize_t xread(int fd, void *buf, size_t len)
ssize_t xwrite(int fd, const void *buf, size_t len)
{
ssize_t nr;
+ if (len MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
while (1) {
nr = write(fd, buf, len);
if ((nr 0) (errno == EAGAIN || errno == EINTR))
--
1.8.4.rc3.5.g4f480ff
--
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: [msysGit] Re: git clone doesn't work in symlink dir roots on Windows
This type of functionality is directly supported by the work I've already done on symlinks here: https://github.com/frogonwheels/git (branches mrg/symlink-v* ) Even if we agree that symlinks only work to a limited degree, or that there are definite limitations, and that the default should be that symlinks NOT be supported within repositories, I'm not sure why people are against incorporating what I've already implemented.. ok well I guess I do - it's about time. Firstly, at the least it means that symlinks like this example where they are outside the repository are supported. Secondly it means that people who are prepared to accept the limitations will be able to use (or at least clone) repositories containing symlinks. One of the big, painful limitations is that windoze symlinks need to be marked as directories at the time of creation. The code I have implemented does it's level best to create the correct type of NTFS symlink based on repository information and falling back on filesystem information. The argument about permissions is only partially valid, since that can be granted as an individual permission to the user without permanent administrator rights. //.ichael G. On Sat, 10 Aug 2013 06:34:59 PM Fredrik Gustafsson wrote: On Sat, Aug 10, 2013 at 07:22:03PM +0300, Sedat Kapanoglu wrote: git is a disk intense program, so this setup is not sane at all. With that said I know that git on windows historically had problems with working on smb-mounted shares (sometimes you're forced to have stupid setups). I doubt that git really is the right tool for your work, since I reproduced the same problem in a regular symlink directory. Repro steps: mkdir actualdir mklink /d symdir actualdir cd symdir git init . fatal: Invalid symlink 'D:/gitto': Function not implemented Thanks, Sedat Good, then we can determinate that this is a symlink error, it seams that readlink() isn't implemented in the msysgit version of msysgit. However msysgit should have a implementation of readlink() according to: http://mingw.5.n7.nabble.com/Replacement-for-readlink-td30679.html I've CC:ed the msysgit-maillist so that they can decide if this is something they want to address in newer releases. (In the git source code the readlink call in this abspath.c) -- 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: [GUILT] add FreeBSD support
Hi Josef,
On Fri, Aug 09, 2013 at 11:20:46AM -0400, Josef 'Jeff' Sipek wrote:
On Fri, Aug 09, 2013 at 11:04:45PM +0800, gnehzuil.liu wrote:
?? 2013-8-9??10:46??Josef 'Jeff' Sipek jef...@josefsipek.net д
On Fri, Aug 09, 2013 at 08:32:28PM +0800, Zheng Liu wrote:
From: Zheng Liu gnehzuil@gmail.com
Currently guilt doesn't support FreeBSD platform. This commit tries to
add this support. The file called 'os.FreeBSD' is copied from os.Darwin
due to these two platforms have almost the same command tools.
Out of curiosity, is it identical? I eyeballed it, and they do look
identical. There's probably a better way to do this whole os-specific
thing, but this will work well enough for now.
Yes, it is identical. Sorry, I am a newbie for guilt, but I am happy to
improve this os-specific thing.Any idea?
So, I'm a bit torn between some build-time checking that generates
something like an os file based on what it detects and something that
happens at runtime. I like that currently there's nothing to do - you just
clone the repo and you're set. At the same time, the more code can be
avoided executing the faster (in theory) guilt gets.
Sorry for the late reply. I did a simple experiment that tries to fold
all os.* files into one file and uses a if statement to export functions
according to different platforms. But frankly I don't like this because
it is not very clearly. So IMHO we'd better add a 'os.FreeBSD' file to
support FreeBSD platform.
I attach the patch in this mail. It is not very mature. If you think
it is worthwhile improving this patch. Please review it. All feedbacks
are always welcome.
Regards,
- Zheng
---
Makefile| 2 +-
guilt | 8 ++--
os | 134
os.Darwin | 70 ---
os.Linux| 57 --
os.SunOS| 57 --
regression/scaffold | 4 +-
7 files changed, 141 insertions(+), 191 deletions(-)
create mode 100644 os
delete mode 100644 os.Darwin
delete mode 100644 os.Linux
delete mode 100644 os.SunOS
diff --git a/Makefile b/Makefile
index b38c1e4..395abc1 100644
--- a/Makefile
+++ b/Makefile
@@ -2,7 +2,7 @@ PREFIX?=/usr/local
DESTDIR?=
INSTALL?=install
-OSFILES = $(filter-out $(wildcard *~),$(wildcard os.*))
+OSFILES = os
SCRIPTS = $(filter-out $(wildcard *~),$(wildcard guilt-*))
.PHONY: all
diff --git a/guilt b/guilt
index e9b2aab..718bed0 100755
--- a/guilt
+++ b/guilt
@@ -906,10 +906,10 @@ pager=more
UNAME_S=`uname -s`
-if [ -r $GUILT_PATH/os.$UNAME_S ]; then
- . $GUILT_PATH/os.$UNAME_S
-elif [ -r $GUILT_PATH/../lib/guilt/os.$UNAME_S ]; then
- . $GUILT_PATH/../lib/guilt/os.$UNAME_S
+if [ -r $GUILT_PATH/os ]; then
+ . $GUILT_PATH/os
+elif [ -r $GUILT_PATH/../lib/guilt/os ]; then
+ . $GUILT_PATH/../lib/guilt/os
else
die Unsupported operating system: $UNAME_S
fi
diff --git a/os b/os
new file mode 100644
index 000..6d1bc01
--- /dev/null
+++ b/os
@@ -0,0 +1,134 @@
+UNAME_S=`uname -s`
+
+if [ $UNAME_S == 'FreeBSD' ] || [ $UNAME_S == 'Darwin' ]; then
+ # usage: touch_date unix ts file
+ touch_date()
+ {
+ touch -t `date -r $1 +%Y%m%d%H%M.%S` $2
+ }
+
+ # usage: last_modified file
+ last_modified()
+ {
+ stat -f %m $1
+ }
+
+ # usage: format_last_modified file
+ format_last_modified()
+ {
+ stat -f %Sm -t %Y-%m-%d %H:%M:%S %z $1
+ }
+
+ # usage: head_n [count]
+ head_n()
+ {
+ if [ $1 -gt 0 ]; then
+ head -n $1
+ fi
+ }
+
+ # usage: sha1 [file]
+ sha1()
+ {
+ if [ $# = 1 ]
+ then
+ openssl dgst -sha1 $1 | sed s,SHA1.\(.*\).=
\(.*\),\2 \1,
+ else
+ openssl dgst -sha1 | sed 's,\(.*= \)*\(.*\),\2 -,'
+ fi
+ }
+
+ # usage: cp_a src dst
+ cp_a()
+ {
+ cp -pR $1 $2
+ }
+
+ # usage: _tac
+ _tac()
+ {
+ sed -e '1!G;h;$!d'
+ }
+
+ _seq()
+ {
+ (
+ if [ $# -eq 1 ]
+ then
+ /usr/bin/jot $1
+ elif [ $# -eq 2 ]
+ then
+ n1=$((${2} - ${1} + 1))
+ n2=$1
+ /usr/bin/jot $n1 $n2
+ elif [ $# -eq 3 ]
+ then
+ num1=$1
+ incr=$2
+ num2=$3
+ /usr/bin/awk -v n1=$num1 -v n2=$num2 -v
Re: Notes and submodules
Hello, On Mon, Aug 19, 2013 at 3:55 PM, Johan Herland jo...@herland.net wrote: On Mon, Aug 19, 2013 at 10:13 AM, Francis Moreau francis.m...@gmail.com wrote: Hello, Is it possible to keep submodules notes in the super project ? Not easily. I guess it depends on what you want to use the notes for. In order for notes to be generally useful (i.e. show up in logs, surviving a notes prune, etc.) they really must reside in the same repo as the annotated objects [1]. Now, if all your interaction with notes happens through scripts that you control, then I guess it would be possible to hack this in some sort of semi-workable way, but you would still have to make sure never to run git notes prune in the super project. I guess the real question here is: Why would you want to do this? and is there maybe some other way your use case can be accomodated? Well, I'm tracking different foreign git repositories as submodules. Those repositories which tracks different projects are not mine therefore I can't save my own stuff directly in them. I need to annotate some commits in each submodule. One option would be to clone each repository in my own place, but I though it would be simpler if I could store the anntotion in _my_ super project. Thanks for your time. -- Francis -- 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 v3 10/24] make sure partially read index is not changed
Eric Sunshine sunsh...@sunshineco.com writes:
On Sun, Aug 18, 2013 at 3:41 PM, Thomas Gummerer t.gumme...@gmail.com wrote:
A partially read index file currently cannot be written to disk. Make
sure that never happens, by erroring out when a caller tries to write a
s/,//
partially read index. Do the same when trying to re-read a partially
read index without having discarded it first to avoid loosing any
s/loosing/losing/
information.
Forcing the caller to load the right part of the index file instead of
re-reading it when changing it, gives a bit of a performance advantage,
s/it,/it/ (or s/file instead/file, instead/)
s/advantage,/advantage/
by avoiding to read parts of the index twice.
/to read/reading/
More below...
Signed-off-by: Thomas Gummerer t.gumme...@gmail.com
---
read-cache.c | 4
1 file changed, 4 insertions(+)
diff --git a/read-cache.c b/read-cache.c
index 38b9a04..7a27f9b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1332,6 +1332,8 @@ int read_index_filtered_from(struct index_state
*istate, const char *path,
void *mmap;
size_t mmap_size;
+ if (istate-filter_opts)
+ die(BUG: Can't re-read partially read index);
errno = EBUSY;
if (istate-initialized)
return istate-cache_nr;
@@ -1455,6 +1457,8 @@ void update_index_if_able(struct index_state *istate,
struct lock_file *lockfile
int write_index(struct index_state *istate, int newfd)
{
+ if (istate-filter_opts)
+ die(BUG: index: cannot write a partially read index);
Consistency nit:
In the preceding hunk, the error message starts BUG: Can't..., but
in this hunk we have BUG: index: cannot
So, BUG: is the prefix of one, but BUG: index: is the prefix of the other.
Spelling difference: Can't vs. cannot.
Capitalization difference: Can't vs. cannot.
Thanks for catching this. From quick grepping it seems the preferred
version seems to be the one with only BUG: as prefix and starting with
a lower case letter after this. Both can't and cannot are used in the
codebase, but cannot seems to be used more often. I'll use that.
Will fix this and the rest of the style/spelling/grammar fixes you
suggested. Thanks.
return istate-ops-write_index(istate, newfd);
}
--
1.8.3.4.1231.g9fbf354.dirty
--
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: Notes and submodules
On Tue, Aug 20, 2013 at 10:39 AM, Francis Moreau francis.m...@gmail.com wrote: On Mon, Aug 19, 2013 at 3:55 PM, Johan Herland jo...@herland.net wrote: On Mon, Aug 19, 2013 at 10:13 AM, Francis Moreau francis.m...@gmail.com wrote: Is it possible to keep submodules notes in the super project ? Not easily. I guess it depends on what you want to use the notes for. In order for notes to be generally useful (i.e. show up in logs, surviving a notes prune, etc.) they really must reside in the same repo as the annotated objects [1]. Now, if all your interaction with notes happens through scripts that you control, then I guess it would be possible to hack this in some sort of semi-workable way, but you would still have to make sure never to run git notes prune in the super project. I guess the real question here is: Why would you want to do this? and is there maybe some other way your use case can be accomodated? Well, I'm tracking different foreign git repositories as submodules. Those repositories which tracks different projects are not mine therefore I can't save my own stuff directly in them. Sure you can. It's your clone, you can store whatever you want in there, regardless of whether you are allowed to push your additions back to the foreign repo. You can always set up another remote repo (e.g. on GitHub) where you can push your additions (whether they be notes or other Git objects). I need to annotate some commits in each submodule. One option would be to clone each repository in my own place, but I though it would be simpler if I could store the anntotion in _my_ super project. No, it's probably much more straightforward to simply maintain your own clones/forks of each submodule, and keep the annotations directly in there. In Git, there is no real concept of _ownership_ of a project. I might put a repo on a server somewhere, and I can own that repo in that I control who is allowed to push into it, but anybody that can read that repo, can also clone it, and I have no say in what happens inside those clones. If somebody is not happy with how I control/maintain the project, they can make their own clone/fork available online, and convince everybody to use that repo (instead of my repo) as the authoritative source of the project. ...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
[PATCH] mailmap: fix check for freeing memory
The condition as it is written in that line was most likely intended to
check for the pointer passed to free(), rather than checking for the
'repo_abbrev', which is already checked against being non null at the
beginning of the function.
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
mailmap.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/mailmap.c b/mailmap.c
index 44614fc..36d2a7a 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -139,35 +139,35 @@ static char *parse_name_and_email(char *buffer, char
**name,
static void read_mailmap_line(struct string_list *map, char *buffer,
char **repo_abbrev)
{
char *name1 = NULL, *email1 = NULL, *name2 = NULL, *email2 = NULL;
if (buffer[0] == '#') {
static const char abbrev[] = # repo-abbrev:;
int abblen = sizeof(abbrev) - 1;
int len = strlen(buffer);
if (!repo_abbrev)
return;
if (len buffer[len - 1] == '\n')
buffer[--len] = 0;
if (!strncmp(buffer, abbrev, abblen)) {
char *cp;
- if (repo_abbrev)
+ if (*repo_abbrev)
free(*repo_abbrev);
*repo_abbrev = xmalloc(len);
for (cp = buffer + abblen; isspace(*cp); cp++)
; /* nothing */
strcpy(*repo_abbrev, cp);
}
return;
}
if ((name2 = parse_name_and_email(buffer, name1, email1, 0)) != NULL)
parse_name_and_email(name2, name2, email2, 1);
if (email1)
add_mapping(map, name1, email1, name2, email2);
}
static int read_mailmap_file(struct string_list *map, const char *filename,
--
1.8.4.rc3.1.gc1ebd90
--
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: [RFC PATCHv4] repack: rewrite the shell script in C.
I didn't look at functions above cmd_repack.
Am 20.08.2013 01:23, schrieb Stefan Beller:
+int cmd_repack(int argc, const char **argv, const char *prefix) {
+
+ int pack_everything = 0;
+ int pack_everything_but_loose = 0;
+ int delete_redundant = 0;
+ char *unpack_unreachable = NULL;
+ int window = 0, window_memory = 0;
+ int depth = 0;
+ int max_pack_size = 0;
+ int no_reuse_delta = 0, no_reuse_object = 0;
+ int no_update_server_info = 0;
+ int quiet = 0;
+ int local = 0;
+ char *packdir, *packtmp;
+ struct child_process cmd;
+ struct string_list_item *item;
+ struct string_list existing_packs = STRING_LIST_INIT_DUP;
+ struct stat statbuffer;
+ int ext;
+ char *exts[2] = {.idx, .pack};
+
+ struct option builtin_repack_options[] = {
Are the long forms of options your invention?
+ OPT_BOOL('a', all, pack_everything,
+ N_(pack everything in a single pack)),
+ OPT_BOOL('A', all-but-loose, pack_everything_but_loose,
+ N_(same as -a, and turn unreachable objects
loose)),
--all-but-loose does not express what the help text says. The long form of
-A is --all --unpack-unreachable, so it is really just a short option for
convenience. It does not need its own long form.
+ OPT_BOOL('d', delete-redundant, delete_redundant,
+ N_(remove redundant packs, and run
git-prune-packed)),
+ OPT_BOOL('f', no-reuse-delta, no_reuse_delta,
+ N_(pass --no-reuse-delta to
git-pack-objects)),
+ OPT_BOOL('F', no-reuse-object, no_reuse_object,
+ N_(pass --no-reuse-object to
git-pack-objects)),
Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
+ OPT_BOOL('n', NULL, no_update_server_info,
+ N_(do not run git-update-server-info)),
No long option name?
+ OPT__QUIET(quiet, N_(be quiet)),
+ OPT_BOOL('l', local, local,
+ N_(pass --local to git-pack-objects)),
Good.
+ OPT_STRING(0, unpack-unreachable, unpack_unreachable,
N_(approxidate),
+ N_(with -A, do not loosen objects older than this
Packing constraints)),
Packing constraints is a section heading, not a continuation of the
previous help text.
+ OPT_INTEGER(0, window, window,
+ N_(size of the window used for delta
compression)),
This help text is suboptimal as the option is a count, not a size in the
narrow sense. But that can be changed later (as it would affect other
tools as well, I guess).
+ OPT_INTEGER(0, window-memory, window_memory,
+ N_(same as the above, but limit memory size instead
of entries count)),
+ OPT_INTEGER(0, depth, depth,
+ N_(limits the maximum delta depth)),
+ OPT_INTEGER(0, max-pack-size, max_pack_size,
+ N_(maximum size of each packfile)),
+ OPT_END()
+ };
Good.
+
+ git_config(repack_config, NULL);
+
+ argc = parse_options(argc, argv, prefix, builtin_repack_options,
+ git_repack_usage, 0);
+
+ sigchain_push_common(remove_pack_on_signal);
Good.
+ packdir = mkpathdup(%s/pack, get_object_directory());
+ packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid());
Should this not be
packdir = xstrdup(git_path(pack));
packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid()));
Perhaps make packdir and packtmp global so that the strings need not be
duplicated in get_pack_filenames and remove_temporary_files?
+
+ remove_temporary_files();
Yes, the shell script had this. But is it really necessary?
+
+ struct argv_array cmd_args = ARGV_ARRAY_INIT;
Declaration after statement.
+ argv_array_push(cmd_args, pack-objects);
+ argv_array_push(cmd_args, --keep-true-parents);
+ argv_array_push(cmd_args, --honor-pack-keep);
+ argv_array_push(cmd_args, --non-empty);
+ argv_array_push(cmd_args, --all);
+ argv_array_push(cmd_args, --reflog);
+
+ if (window)
+ argv_array_pushf(cmd_args, --window=%u, window);
+
+ if (window_memory)
+ argv_array_pushf(cmd_args, --window-memory=%u,
window_memory);
+
+ if (depth)
+ argv_array_pushf(cmd_args, --depth=%u, depth);
+
+ if (max_pack_size)
+ argv_array_pushf(cmd_args, --max_pack_size=%u,
max_pack_size);
+
+ if (no_reuse_delta)
+ argv_array_pushf(cmd_args, --no-reuse-delta);
+
+ if (no_reuse_object)
+ argv_array_pushf(cmd_args, --no-reuse-object);
no_reuse_delta
Re: [PATCH] mailmap: fix check for freeing memory
Stefan Beller stefanbel...@googlemail.com writes:
The condition as it is written in that line was most likely intended to
check for the pointer passed to free(), rather than checking for the
'repo_abbrev', which is already checked against being non null at the
beginning of the function.
[...]
- if (repo_abbrev)
+ if (*repo_abbrev)
free(*repo_abbrev);
But now the test is useless, because free(NULL) is defined to be a
no-op.
--
Thomas Rast
trast@{inf,student}.ethz.ch
--
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] mailmap: fix check for freeing memory
On Tue, Aug 20, 2013 at 03:40:02PM +0200, Thomas Rast wrote: Stefan Beller stefanbel...@googlemail.com writes: The condition as it is written in that line was most likely intended to check for the pointer passed to free(), rather than checking for the 'repo_abbrev', which is already checked against being non null at the beginning of the function. [...] - if (repo_abbrev) + if (*repo_abbrev) free(*repo_abbrev); But now the test is useless, because free(NULL) is defined to be a no-op. Yeah, I think we should just drop the conditional completely. I am not sure of the complete back-story. The earlier check for repo_abbrev to be non-NULL was added by 8503ee4, after this check on free() already existed. So that was when this conditional became redundant. But the line right after this one unconditionally assigns to *repo_abbrev, so we would always segfault in such a case anyway (which is what 8503ee4 was fixing). So I think it was either a misguided don't pass NULL to free check that was simply wrong, or it was an incomplete make sure repo_abbrev is not NULL check. And the first is useless, and the second is now redundant due to 8503ee4. So it should simply be free(). -Peff -- 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] mailmap: fix check for freeing memory
On 08/20/2013 03:40 PM, Thomas Rast wrote:
Stefan Beller stefanbel...@googlemail.com writes:
The condition as it is written in that line was most likely intended to
check for the pointer passed to free(), rather than checking for the
'repo_abbrev', which is already checked against being non null at the
beginning of the function.
[...]
-if (repo_abbrev)
+if (*repo_abbrev)
free(*repo_abbrev);
But now the test is useless, because free(NULL) is defined to be a
no-op.
Yes, indeed. Thanks for reviewing.
Stepping two steps back, I am trying to figure out, what this repo_abrev
thing is doing, as I could find no documentation.
It's passed as a double pointer as declared in mailmap.h:
int read_mailmap(struct string_list *map, char **repo_abbrev);
However grepping for read_mailmap( (bracket to prevent finding
read_mailmap_XXX as often used in mailmap.c itself)
grep -nHIirF --exclude-dir=.git -- read_mailmap(
throughout all the sources I just find one occurence having the
second argument not being 'NULL' and that is in
builtin/shortlog.c:212: read_mailmap(log-mailmap,
log-common_repo_prefix);
which turns out to be:
void shortlog_init(struct shortlog *log)
{
memset(log, 0, sizeof(*log));
read_mailmap(log-mailmap, log-common_repo_prefix);
...
So we're passing there an address, which was just set to zero.
This is the only occurence of passing a value at all and the value
being passed is 0, so the free in the original patch doesn't need
that check either.
As I am resending the patch, could somebody please explain me
the mechanism of the # repo-abbrev: line? Even git itself doesn't
use it in the .mailmap file, but a quick google search shows up only
kernel repositories.
Stefan
signature.asc
Description: OpenPGP digital signature
Re: [PATCH] Document smart http
On Tue, Aug 20, 2013 at 12:08:08PM +0700, Nguyen Thai Ngoc Duy wrote:
This may provide some clues for those who want to modify smart http
code as smart http is pretty much undocumented. Smart http document
so far is a few commit messages and the source code.
There was also this:
http://article.gmane.org/gmane.comp.version-control.git/129734
which seems to have never gotten updated enough to be applied along with
the code. But with some updates to make sure it matches the current
behavior, it is probably a more comprehensive description.
But if you don't feel like spending more time on this on top of what
you've already done, I think the patch I'm responding to is better than
what we have now (i.e., nothing).
+Reference Discovery
+---
+
+The server end always sends the list of references in both push and
+fetch cases. This ref list is retrieved by the client's sending HTTP
+GET request to a smart http url ending with
+/info/refs?service=service where service could be either
+git-upload-pack or git-receive-pack for fetching or pushing
+respectively. The output is in pkt-line format.
+
+
+ advertised-refs = service
+ flush-pkt
+ (no-refs / list-of-refs)
+ flush-pkt
+
+ service = PKT-LINE(# service= service-name)
+ service-name = (git-upload-pack / git-receive-pack)
+
+ no-refs = PKT-LINE(zero-id SP capabilities^{}
+ NUL capability-list LF)
+
+ list-of-refs = first-ref *other-ref
+ first-ref= PKT-LINE(obj-id SP refname
+ NUL capability-list LF)
+
+ other-ref= PKT-LINE(other-tip / other-peeled)
+ other-tip= obj-id SP refname LF
+ other-peeled = obj-id SP refname ^{} LF
+
+ capability-list = capability *(SP capability)
+ capability = 1*(LC_ALPHA / DIGIT / - / _)
+ LC_ALPHA = %x61-7A
+
Most of this is a repeat of what is in the earlier sections. I don't
think the protocol is changing much and these are not likely to get out of
date with each other, but I wonder if it is easier on the reader to
simply describe the output in terms of what is added on top of the
regular ref advertisement (i.e., the service line). Something like:
stateless-advertised-refs = service
advertised-refs
service = PKT-LINE(# service= service-name)
service-name = (git-upload-pack / git-receive-pack)
where advertised-refs is defined in the earlier BNF. You may also want
to note:
Servers may respond to a smart request with a regular `advertised-refs`
response rather than a `stateless-advertised-refs` response. In this
case, the client MUST assume that the server does not understand smart
HTTP and either abort or proceed with the non-smart protocol.
-Peff
--
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 v3 15/24] read-cache: read index-v5
On Mon, Aug 19, 2013 at 2:42 AM, Thomas Gummerer t.gumme...@gmail.com wrote:
+static int read_entry(struct cache_entry **ce, char *pathname, size_t
pathlen,
+ void *mmap, unsigned long mmap_size,
+ unsigned int first_entry_offset,
+ unsigned int foffsetblock)
+{
+ int len, offset_to_offset;
+ char *name;
+ uint32_t foffsetblockcrc, *filecrc, *beginning, *end, entry_offset;
+ struct ondisk_cache_entry *disk_ce;
+
+ beginning = ptr_add(mmap, foffsetblock);
+ end = ptr_add(mmap, foffsetblock + 4);
+ len = ntoh_l(*end) - ntoh_l(*beginning) - sizeof(struct
ondisk_cache_entry) - 5;
+ entry_offset = first_entry_offset + ntoh_l(*beginning);
+ name = ptr_add(mmap, entry_offset);
+ disk_ce = ptr_add(mmap, entry_offset + len + 1);
+ *ce = cache_entry_from_ondisk(disk_ce, pathname, name, len, pathlen);
+ filecrc = ptr_add(mmap, entry_offset + len + 1 + sizeof(*disk_ce));
+ offset_to_offset = htonl(foffsetblock);
+ foffsetblockcrc = crc32(0, (Bytef*)offset_to_offset, 4);
+ if (!check_crc32(foffsetblockcrc,
+ ptr_add(mmap, entry_offset), len + 1 + sizeof(*disk_ce),
+ ntoh_l(*filecrc)))
+ return -1;
+
+ return 0;
+}
Last thought before book+bed time. I wonder if moving the name part to
the end of the entry (i.e. chaging on disk format) would simplify this
code. The new ondisk_cache_entry would be something like this
struct ondisk_cache_entry {
uint16_t flags;
uint16_t mode;
struct cache_time mtime;
uint32_t size;
int stat_crc;
unsigned char sha1[20];
char name[FLEX_ARRAY];
};
--
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: [PATCH] Document smart http
On Tue, Aug 20, 2013 at 9:16 PM, Jeff King p...@peff.net wrote: On Tue, Aug 20, 2013 at 12:08:08PM +0700, Nguyen Thai Ngoc Duy wrote: This may provide some clues for those who want to modify smart http code as smart http is pretty much undocumented. Smart http document so far is a few commit messages and the source code. There was also this: http://article.gmane.org/gmane.comp.version-control.git/129734 which seems to have never gotten updated enough to be applied along with the code. But with some updates to make sure it matches the current behavior, it is probably a more comprehensive description. If I knew about this patch, it could have saved me a lot of time reading remote-curl.c. Will check it with current code and resubmit an update of this version instead. -- 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: [PATCH] mailmap: remove redundant check for freeing memory
On Tue, Aug 20, 2013 at 04:18:00PM +0200, Stefan Beller wrote: The condition as it is written in that line has already been checked in the beginning of the function, which was introduced in 8503ee4 (2007-05-01, Fix read_mailmap to handle a caller uninterested in repo abbreviation) Helped-by: Jeff King p...@peff.net Helped-by: Thomas Rast tr...@inf.ethz.ch Signed-off-by: Stefan Beller stefanbel...@googlemail.com --- This version looks good to me. Thanks. -Peff -- 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: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
On Tue, Aug 20, 2013 at 01:15:02AM +0200, Erik Faye-Lund wrote:
This one seems real, although it's quite theoretical. It should only happen
in cases where the log-message contains %1, the initial malloc passed and
reallocing two more bytes failed.
However, what's much more of a disaster: pos is used after the call to
realloc might have moved the memory!
Yeah, agreed on both counts.
I guess something like this should fix both issues. Sorry about the
lack of indentation, it seems Gmail has regressed, and the old compose
mode is somehow gone... (also sorry for triple-posting to some of you,
Gmail seems particularly broken today)
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index d015e43..0641f4e 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
va_end(ap);
while ((pos = strstr(str, %1)) != NULL) {
- str = realloc(str, ++str_len + 1);
- if (!str) {
+ char *tmp = realloc(str, ++str_len + 1);
+ if (!tmp) {
warning(realloc failed: '%s', strerror(errno));
+ free(str);
return;
}
+ pos = tmp + (pos - str);
+ str = tmp;
memmove(pos + 2, pos + 1, strlen(pos));
pos[1] = ' ';
}
Yes, that looks like the right solution. You could also convert pos to
an integer index rather than a pointer (but then you end up adding it it
to the pointer in the memmove call, which is probably just as ugly).
-Peff
--
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] mailmap: fix check for freeing memory
On 08/20/2013 04:23 PM, Thomas Rast wrote: Stefan Beller stefanbel...@googlemail.com writes: As I am resending the patch, could somebody please explain me the mechanism of the # repo-abbrev: line? Even git itself doesn't use it in the .mailmap file, but a quick google search shows up only kernel repositories. I had no idea (we really lack documentation on this), but some history digging shows this: commit 7595e2ee6ef9b35ebc8dc45543723e1d89765ce3 Author: Junio C Hamano jun...@cox.net Date: Sat Nov 25 00:07:54 2006 -0800 git-shortlog: make common repository prefix configurable with .mailmap The code had /pub/scm/linux/kernel/git/ hardcoded which was too specific to the kernel project. With this, a line in the .mailmap file: # repo-abbrev: /pub/scm/linux/kernel/git/ can be used to cause the substring to be abbreviated to /.../ on the title line of the commit message. Signed-off-by: Junio C Hamano jun...@cox.net It apparently serves to abbreviate commits coming from pull requests, with a subject like Merge branch 'release' of git://git.kernel.org/pub/scm/linux/kernel/git/lenb/linux Thanks for looking through the history, I need to adapt my search patterns. ;) Personally I have the feel this is a very kernel specific, but also useful thing to have. So I would definitely not drop it, but maybe move it to another place, such as in the .git/config file. Then anybody can configure it themselves and maybe even have multiple abbreviation lines possible. It's very likely nowadays that there are repos at various different hosting sites, so just one abbreviation would not cut it anymore. Or as Jeff just mentioned in his email, it's there for historical purpose, but not needed anymore as git-merge produces nicer commit messages there. As proposed I checked recent kernel history and saw: $ git log --min-parents=2 --oneline d6a5e06 Merge git://git.kernel.org/pub/scm/linux/kernel/git/steve/gfs2-3.0-fixes 7067552 Merge branch 'x86-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip e91dade Merge branch 'timers-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip fbf2184 Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux 3387ed8 Merge tag 'drm-intel-fixes-2013-08-15' of git://people.freedesktop.org/~danvet/drm-intel d2b2c08 Merge branch 'drm-fixes-3.11' of git://people.freedesktop.org/~agd5f/linux 50e37cc Merge branch 'for-3.11-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup a08797e Merge tag 'ext4_for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4 2620bf0 Merge branch 'fixes' of git://git.linaro.org/people/rmk/linux-arm 359d16c Merge branch 'for-3.11' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k 0f7dd1a Merge tag 'clk-fixes-for-linus' of git://git.linaro.org/people/mturquette/linux 2d2843e Merge tag 'pm-3.11-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm f43c606 Merge tag 'sound-3.11' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound 89cb9ae Merge tag 'usb-3.11-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb in their .mailmap it read: # repo-abbrev: /pub/scm/linux/kernel/git/ So the abbreviation doesn't take place, can this feature be turned off? I'm still confused by that feature. Thanks, Stefan signature.asc Description: OpenPGP digital signature
Re: [PATCH] stream_to_pack: xread does not guarantee to read all requested bytes
Johannes Sixt j...@kdbg.org writes:
The deflate loop in bulk-checkin::stream_to_pack expects to get all bytes
from a file that it requests to read in a single function call. But it
used xread(), which does not give that guarantee. Replace it by
read_in_full().
Signed-off-by: Johannes Sixt j...@kdbg.org
---
The size is limited to sizeof(ibuf) == 16384 bytes, so that there
should not be a problem with the unpatched code on any OS in practice.
Nevertheless, this change seems reasonable from a code hygiene POV.
bulk-checkin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6b0b6d4..118c625 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state
*state,
if (size !s.avail_in) {
ssize_t rsize = size sizeof(ibuf) ? size :
sizeof(ibuf);
- if (xread(fd, ibuf, rsize) != rsize)
+ if (read_in_full(fd, ibuf, rsize) != rsize)
This is the kind of thing i was wondering and worried about with the
other clipped xread/xwrite patch. The original of this caller is
obviously wrong. Thanks for spotting and fixing.
I wonder if there are more like this broken caller or xread and/or
xwrite.
die(failed to read %d bytes from '%s',
(int)rsize, path);
offset += rsize;
--
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: [RFC PATCHv4] repack: rewrite the shell script in C.
On 08/20/2013 03:31 PM, Johannes Sixt wrote:
Are the long forms of options your invention?
I tried to keep strong similarity with the shell script for
ease of review. In the shellscript the options where
put in variables having these names, so for example there was:
-f) no_reuse=--no-reuse-delta ;;
-F) no_reuse=--no-reuse-object ;;
So I used these variable names as well in here. And as I assumed
the variables are meaningful in itself.
In the shell script they may be meaningful, but with the option
parser in the C version, I overlooked the possibility for
--no-option being possible as you noted below.
Maybe we should inverse the logic and have the variables and options
called reuse-delta and being enabled by default.
+OPT_BOOL('a', all, pack_everything,
+N_(pack everything in a single pack)),
+OPT_BOOL('A', all-but-loose, pack_everything_but_loose,
+N_(same as -a, and turn unreachable objects loose)),
--all-but-loose does not express what the help text says. The long form
of -A is --all --unpack-unreachable, so it is really just a short option
for convenience. It does not need its own long form.
Ok, I'll keep that in mind, and will only use the varialbe tied to -A
to set the -a and --unpack-unreachable variable.
+OPT_BOOL('d', delete-redundant, delete_redundant,
+N_(remove redundant packs, and run git-prune-packed)),
+OPT_BOOL('f', no-reuse-delta, no_reuse_delta,
+N_(pass --no-reuse-delta to git-pack-objects)),
+OPT_BOOL('F', no-reuse-object, no_reuse_object,
+N_(pass --no-reuse-object to git-pack-objects)),
Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
see above, I'd try not to.
+OPT_BOOL('n', NULL, no_update_server_info,
+N_(do not run git-update-server-info)),
No long option name?
This is also a negated option, so as above, maybe
we could have --update_server_info and --no-update_server_info
respectively. Talking about the shortform then: Is it possible to
negate the shortform?
+OPT__QUIET(quiet, N_(be quiet)),
+OPT_BOOL('l', local, local,
+N_(pass --local to git-pack-objects)),
Good.
+OPT_STRING(0, unpack-unreachable, unpack_unreachable,
N_(approxidate),
+N_(with -A, do not loosen objects older than this
Packing constraints)),
Packing constraints is a section heading, not a continuation of the
previous help text.
+OPT_INTEGER(0, window, window,
+N_(size of the window used for delta compression)),
This help text is suboptimal as the option is a count, not a size in
the narrow sense. But that can be changed later (as it would affect
other tools as well, I guess).
+OPT_INTEGER(0, window-memory, window_memory,
+N_(same as the above, but limit memory size instead
of entries count)),
+OPT_INTEGER(0, depth, depth,
+N_(limits the maximum delta depth)),
+OPT_INTEGER(0, max-pack-size, max_pack_size,
+N_(maximum size of each packfile)),
+OPT_END()
+};
Good.
+
+git_config(repack_config, NULL);
+
+argc = parse_options(argc, argv, prefix, builtin_repack_options,
+git_repack_usage, 0);
+
+sigchain_push_common(remove_pack_on_signal);
Good.
+packdir = mkpathdup(%s/pack, get_object_directory());
+packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid());
Should this not be
packdir = xstrdup(git_path(pack));
packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid()));
Perhaps make packdir and packtmp global so that the strings need not be
duplicated in get_pack_filenames and remove_temporary_files?
ok
+
+remove_temporary_files();
Yes, the shell script had this. But is it really necessary?
Well I can drop it if it's not needed.
It actually should implement
rm -f $PACKTMP-*
and then the trap 'rm -f $PACKTMP-*' 0 1 2 3 15
as well.
+
+struct argv_array cmd_args = ARGV_ARRAY_INIT;
Declaration after statement.
will fix.
+argv_array_push(cmd_args, pack-objects);
+argv_array_push(cmd_args, --keep-true-parents);
+argv_array_push(cmd_args, --honor-pack-keep);
+argv_array_push(cmd_args, --non-empty);
+argv_array_push(cmd_args, --all);
+argv_array_push(cmd_args, --reflog);
+
+if (window)
+argv_array_pushf(cmd_args, --window=%u, window);
+
+if (window_memory)
+argv_array_pushf(cmd_args, --window-memory=%u,
window_memory);
+
+if (depth)
+argv_array_pushf(cmd_args, --depth=%u, depth);
+
+if (max_pack_size)
+argv_array_pushf(cmd_args, --max_pack_size=%u,
max_pack_size);
+
+if (no_reuse_delta)
+argv_array_pushf(cmd_args, --no-reuse-delta);
+
+if (no_reuse_object)
+
Should git apply --check imply verbose?
TL;DR -- git apply --reject implies verbose, but the similar git apply --check does not, which seems inconsistent. Background: A common (non-git) workflow can be to use patch --dry-run to inspect whether a patch is feasible, and then use patch again a 2nd time (w/o --dry-run) to actually apply it (and then work through the rejects). You can also do the above in a git repo, but you lose out because patch doesn't (yet) capture the patched function names[1] in the rejected hunks, making it hard to double check your work. My initial thought was to replace the above two steps with git apply --check ... and then git apply --reject ... so that I could just abandon using patch altogether. That works great, with just one snag that had me go reading the source. It seems that git apply --reject is verbose, and kind of looks like the identical output I'd get if I used patch. But git apply --check is quite reserved in its output and doesn't look at all like patch --dry-run. I initially believed that --check was stopping at the 1st failure, based on the output. Only when I read the source did I realize it was checking all the hunks silently, and adding a -v would make it similar to the output from patch --dry-run. Not a critical issue by any means, but having the -v implied by --check (or perhaps having both default to non-verbose?) might save other users from getting confused in the same way. Thanks, Paul. -- [1] https://savannah.gnu.org/bugs/index.php?39819 -- 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] stream_to_pack: xread does not guarantee to read all requested bytes
On Tue, Aug 20, 2013 at 5:00 PM, Junio C Hamano gits...@pobox.com wrote:
Johannes Sixt j...@kdbg.org writes:
The deflate loop in bulk-checkin::stream_to_pack expects to get all bytes
from a file that it requests to read in a single function call. But it
used xread(), which does not give that guarantee. Replace it by
read_in_full().
Signed-off-by: Johannes Sixt j...@kdbg.org
---
The size is limited to sizeof(ibuf) == 16384 bytes, so that there
should not be a problem with the unpatched code on any OS in practice.
Nevertheless, this change seems reasonable from a code hygiene POV.
bulk-checkin.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 6b0b6d4..118c625 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -114,7 +114,7 @@ static int stream_to_pack(struct bulk_checkin_state
*state,
if (size !s.avail_in) {
ssize_t rsize = size sizeof(ibuf) ? size :
sizeof(ibuf);
- if (xread(fd, ibuf, rsize) != rsize)
+ if (read_in_full(fd, ibuf, rsize) != rsize)
This is the kind of thing i was wondering and worried about with the
other clipped xread/xwrite patch. The original of this caller is
obviously wrong. Thanks for spotting and fixing.
I wonder if there are more like this broken caller or xread and/or
xwrite.
I was actually wondering when it's better to use xread() over
read_in_full() ? Considering that we don't know if xread() will read
the whole buffer or not, would it not be better to always use
read_in_full() ? I guess there is a drawback to this, but I'm not
exactly sure what it is.
--
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: [GUILT] add FreeBSD support
On Tue, Aug 20, 2013 at 03:44:16PM +0800, Zheng Liu wrote: Hi Josef, On Fri, Aug 09, 2013 at 11:20:46AM -0400, Josef 'Jeff' Sipek wrote: On Fri, Aug 09, 2013 at 11:04:45PM +0800, gnehzuil.liu wrote: ?? 2013-8-9??10:46??Josef 'Jeff' Sipek jef...@josefsipek.net д On Fri, Aug 09, 2013 at 08:32:28PM +0800, Zheng Liu wrote: From: Zheng Liu gnehzuil@gmail.com Currently guilt doesn't support FreeBSD platform. This commit tries to add this support. The file called 'os.FreeBSD' is copied from os.Darwin due to these two platforms have almost the same command tools. Out of curiosity, is it identical? I eyeballed it, and they do look identical. There's probably a better way to do this whole os-specific thing, but this will work well enough for now. Yes, it is identical. Sorry, I am a newbie for guilt, but I am happy to improve this os-specific thing.Any idea? So, I'm a bit torn between some build-time checking that generates something like an os file based on what it detects and something that happens at runtime. I like that currently there's nothing to do - you just clone the repo and you're set. At the same time, the more code can be avoided executing the faster (in theory) guilt gets. Sorry for the late reply. I did a simple experiment that tries to fold all os.* files into one file and uses a if statement to export functions according to different platforms. But frankly I don't like this because it is not very clearly. So IMHO we'd better add a 'os.FreeBSD' file to support FreeBSD platform. Yeah, sounds like the simplest (at least for the moment). I'll commit it. Thanks. FWIW, the idea I was thinking about was to make make all figure out various parts of the system and construct an os file. Jeff. -- UNIX is user-friendly ... it's just selective about who its friends are -- 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] mailmap: fix check for freeing memory
On Tue, Aug 20, 2013 at 04:38:17PM +0200, Stefan Beller wrote: As proposed I checked recent kernel history and saw: $ git log --min-parents=2 --oneline d6a5e06 Merge git://git.kernel.org/pub/scm/linux/kernel/git/steve/gfs2-3.0-fixes 7067552 Merge branch 'x86-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip e91dade Merge branch 'timers-urgent-for-linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tip/tip fbf2184 Merge branch 'drm-fixes' of git://people.freedesktop.org/~airlied/linux 3387ed8 Merge tag 'drm-intel-fixes-2013-08-15' of git://people.freedesktop.org/~danvet/drm-intel d2b2c08 Merge branch 'drm-fixes-3.11' of git://people.freedesktop.org/~agd5f/linux 50e37cc Merge branch 'for-3.11-fixes' of git://git.kernel.org/pub/scm/linux/kernel/git/tj/cgroup a08797e Merge tag 'ext4_for_linus' of git://git.kernel.org/pub/scm/linux/kernel/git/tytso/ext4 2620bf0 Merge branch 'fixes' of git://git.linaro.org/people/rmk/linux-arm 359d16c Merge branch 'for-3.11' of git://git.kernel.org/pub/scm/linux/kernel/git/geert/linux-m68k 0f7dd1a Merge tag 'clk-fixes-for-linus' of git://git.linaro.org/people/mturquette/linux 2d2843e Merge tag 'pm-3.11-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/rafael/linux-pm f43c606 Merge tag 'sound-3.11' of git://git.kernel.org/pub/scm/linux/kernel/git/tiwai/sound 89cb9ae Merge tag 'usb-3.11-rc6' of git://git.kernel.org/pub/scm/linux/kernel/git/gregkh/usb in their .mailmap it read: # repo-abbrev: /pub/scm/linux/kernel/git/ So the abbreviation doesn't take place, can this feature be turned off? I'm still confused by that feature. It _only_ impacts git-shortlog, not git-log or other traversals. Making it an even more dubious feature, IMHO. -Peff -- 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] mailmap: fix check for freeing memory
Jeff King p...@peff.net writes: It _only_ impacts git-shortlog, not git-log or other traversals. Making it an even more dubious feature, IMHO. I think this was done by an explicit end user request for shortlog. As you mentioned, merge gives readable merge log messages, but it deliberately uses the real URL, not your personal nickname for the remote when writing the title line of a merge, i.e. Merge [branch x of ]repoURL so it would be helped by the repository abbreviation. It probably was an oversight that we did not extend it to the rest of the log family. -- 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 v4 3/4] gitweb: omit the repository owner when it is unset
On the repository summary page, leave the owner line out if the
repo does not have an owner, rather than displaying a labelled empty
field. This does not affect the owner column in the projects list
page, which is present unless $omit_owner is true.
Signed-off-by: Tony Finch d...@dotat.at
---
gitweb/gitweb.perl | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8d69ada..c029b98 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -6463,7 +6463,7 @@ sub git_summary {
print div class=\title\nbsp;/div\n;
print table class=\projects_list\\n .
tr id=\metadata_desc\tddescription/tdtd .
esc_html($descr) . /td/tr\n;
-unless ($omit_owner) {
+if ($owner and not $omit_owner) {
print tr id=\metadata_owner\tdowner/tdtd .
esc_html($owner) . /td/tr\n;
}
if (defined $cd{'rfc2822'}) {
--
1.8.3.1.605.g85318f5
--
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 v4 1/4] gitweb: Ensure OPML text fits inside its box.
The rss_logo CSS style has a fixed width which is too narrow for
the string OPML. Replace the fixed width with horizontal padding
so the text fits with nice margins.
Signed-off-by: Tony Finch d...@dotat.at
---
gitweb/static/gitweb.css | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index cb86d2d..a869be1 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -548,8 +548,7 @@ a.linenr {
a.rss_logo {
float: right;
- padding: 3px 0px;
- width: 35px;
+ padding: 3px 5px;
line-height: 10px;
border: 1px solid;
border-color: #fcc7a5 #7d3302 #3e1a01 #ff954e;
--
1.8.3.1.605.g85318f5
--
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 v4 4/4] gitweb: make search help link less ugly
The search help link was a superscript question mark right next to
a drop-down menu, which looks misaligned and is a cramped and
awkward click target. Remove the superscript tags and add some
spacing to fix these nits. Add a title attribute to provide an
explanatory mouseover.
Signed-off-by: Tony Finch d...@dotat.at
---
gitweb/gitweb.perl | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8d69ada..59af7de 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -4030,8 +4030,8 @@ sub print_search_form {
$cgi-input({-name=h, -value=$search_hash, -type=hidden})
. \n .
$cgi-popup_menu(-name = 'st', -default = 'commit',
-values = ['commit', 'grep', 'author',
'committer', 'pickaxe']) .
- $cgi-sup($cgi-a({-href = href(action=search_help)}, ?)) .
- search:\n,
+ . $cgi-a({-href = href(action=search_help),
+-title = search help }, ?) . search:\n,
$cgi-textfield(-name = s, -value = $searchtext, -override =
1) . \n .
span title=\Extended regular expression\ .
$cgi-checkbox(-name = 'sr', -value = 1, -label = 're',
--
1.8.3.1.605.g85318f5
--
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 v4 2/4] gitweb: vertically centre contents of page footer
Signed-off-by: Tony Finch d...@dotat.at
---
gitweb/static/gitweb.css | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index a869be1..3b4d833 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -68,12 +68,13 @@ div.page_path {
}
div.page_footer {
- height: 17px;
+ height: 22px;
padding: 4px 8px;
background-color: #d9d8d1;
}
div.page_footer_text {
+ line-height: 22px;
float: left;
color: #55;
font-style: italic;
--
1.8.3.1.605.g85318f5
--
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 v4 0/4] Four small gitweb tweaks
This is mostly just a repost to un-stall this topic. I have fixed the tab damage problem spotted by Jakub in the search help link patch, and I have improved the commit message for the repository owner patch. No other changes. Tony Finch (4): gitweb: Ensure OPML text fits inside its box. gitweb: vertically centre contents of page footer gitweb: omit the repository owner when it is unset gitweb: make search help link less ugly gitweb/gitweb.perl | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) -- 1.8.3.1.605.g85318f5 -- 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] mailmap: fix check for freeing memory
On Tue, Aug 20, 2013 at 10:18:08AM -0700, Junio C Hamano wrote: As you mentioned, merge gives readable merge log messages, but it deliberately uses the real URL, not your personal nickname for the remote when writing the title line of a merge, i.e. Merge [branch x of ]repoURL so it would be helped by the repository abbreviation. It probably was an oversight that we did not extend it to the rest of the log family. Ah, yeah, I suppose git-pull will still do that. I was thinking of a direct git-merge of a tracking branch, which would end up with: Merge remote-tracking branch 'origin/master' whereas git pull origin master in the same case would say: Merge branch 'master' of git://github.com/gitster/git Still, I do not think anybody but the kernel is using it. Most people simply have shorter URLs that do not need abbreviated (e.g., the GitHub one shown above). And searching for instances on GitHub yields only the kernel: https://github.com/search?q=repo-abbrev+path%3A.mailmaptype=Code Anyway, I am not proposing ripping the feature out. It just seems like it does not have a lot of users, and it is not worth worrying much about trying to extend it. -Peff -- 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: Should git apply --check imply verbose?
Paul Gortmaker paul.gortma...@windriver.com writes: TL;DR -- git apply --reject implies verbose, but the similar git apply --check does not, which seems inconsistent. Hmmm, I am of two minds. From purely idealistic point of view, I can see why defaulting both to non-verbose may look a more attractive way to go, but I have my reservations that is more than the usual change-aversion. Historically, check was primarily meant to see if the patch is applicable cleanly in scripts, and we never thought it would make any sense to make it verbose by default. On the other hand, the operation of reject, which was a much later invention, was primarily meant to be observed by humans to see how the patch failed to cleanly apply and where, to help them decide where to look in the target to wiggle the rejected hunk into (even when it is driven from a script). It did not make much sense to squelch its output. In addition, because check is an idempotent operation that does not touch anything in the index or the working tree, running with check and then check verbose is possible if somebody runs it without verbose and then decides later that s/he wants to see the details. But reject does touch the working tree files with applicable hunks, so after a quiet reject, there is no way to see the verbose output like you can with 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] stream_to_pack: xread does not guarantee to read all requested bytes
Am 20.08.2013 17:00, schrieb Junio C Hamano: I wonder if there are more like this broken caller or xread and/or xwrite. Looking at the grep -C1 output, there are no others. The only one that looked suspicious was xread in remote-curl.c, but it is fine (it just eats all data from the input). -- 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] stream_to_pack: xread does not guarantee to read all requested bytes
Antoine Pelisse apeli...@gmail.com writes:
I was actually wondering when it's better to use xread() over
read_in_full()?
When the caller wants to do more control over a read that may have
to loop. For example, this loop in builtin/index-pack.c::fill()
do {
ssize_t ret = xread(input_fd, input_buffer + input_len,
sizeof(input_buffer) - input_len);
if (ret = 0) {
if (!ret)
die(_(early EOF));
die_errno(_(read error on input));
}
input_len += ret;
if (from_stdin)
display_throughput(progress, consumed_bytes +
input_len);
} while (input_len min);
cannot be replaced blindly with read_in_full() because (1) the
caller wants to do the display_throughput() part in the loop, and
(2) the caller wants to fill at least min bytes but can happily
accept to read more up to the size of the input_buffer.
--
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] stream_to_pack: xread does not guarantee to read all requested bytes
Am 20.08.2013 17:16, schrieb Antoine Pelisse: I was actually wondering when it's better to use xread() over read_in_full() ? Considering that we don't know if xread() will read the whole buffer or not, would it not be better to always use read_in_full() ? Of course, you know whether the whole buffer was filled: xread() returns the number of bytes read. Same for xwrite(). I guess there is a drawback to this, but I'm not exactly sure what it is. The disadvantage of read_in_full() is that it can happen that it reads data from the stream successfully, but then reports an error. In this case, the data read is lost. Analogously, when write_in_full() writes (some) data successfully, but ultimately fails, you don't know how much was written successfully. -- 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: [RFC PATCHv4] repack: rewrite the shell script in C.
Am 20.08.2013 17:08, schrieb Stefan Beller: On 08/20/2013 03:31 PM, Johannes Sixt wrote: You cannot run_command() and then later read its output! You must split it into start_command(), read stdout, finish_command(). Thanks for this hint. Could that explain rare non-deterministic failures in the test suite? Yes, it's a possible explanation. -- 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: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
Erik Faye-Lund kusmab...@gmail.com writes:
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index d015e43..0641f4e 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
va_end(ap);
while ((pos = strstr(str, %1)) != NULL) {
- str = realloc(str, ++str_len + 1);
- if (!str) {
+ char *tmp = realloc(str, ++str_len + 1);
+ if (!tmp) {
warning(realloc failed: '%s', strerror(errno));
+ free(str);
return;
}
+ pos = tmp + (pos - str);
Pedantically, this is undefined (uses of both pos and str may trap after
realloc has freed the original pointer), it is better to calculate the
difference before calling realloc.
Andreas.
--
Andreas Schwab, sch...@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5
And now for something completely different.
--
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: Should git apply --check imply verbose?
On 13-08-20 01:57 PM, Junio C Hamano wrote: Paul Gortmaker paul.gortma...@windriver.com writes: TL;DR -- git apply --reject implies verbose, but the similar git apply --check does not, which seems inconsistent. Hmmm, I am of two minds. From purely idealistic point of view, I can see why defaulting both to non-verbose may look a more attractive way to go, but I have my reservations that is more than the usual change-aversion. OK, so given your feedback, how do you feel about a patch to the documentation that indicates to use -v in combination with the --check to get equivalent patch --dry-run behaviour? If that had existed, I'd have not gone rummaging around in the source, so that should be good enough to help others avoid the same... P. -- Historically, check was primarily meant to see if the patch is applicable cleanly in scripts, and we never thought it would make any sense to make it verbose by default. On the other hand, the operation of reject, which was a much later invention, was primarily meant to be observed by humans to see how the patch failed to cleanly apply and where, to help them decide where to look in the target to wiggle the rejected hunk into (even when it is driven from a script). It did not make much sense to squelch its output. In addition, because check is an idempotent operation that does not touch anything in the index or the working tree, running with check and then check verbose is possible if somebody runs it without verbose and then decides later that s/he wants to see the details. But reject does touch the working tree files with applicable hunks, so after a quiet reject, there is no way to see the verbose output like you can with 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: Should git apply --check imply verbose?
Hi Paul, Paul Gortmaker wrote: OK, so given your feedback, how do you feel about a patch to the documentation that indicates to use -v in combination with the --check to get equivalent patch --dry-run behaviour? Sounds like a good idea to me. I assume you mean a note in the OPTIONS or EXAMPLES section of Documentation/git-apply.txt? Thanks, Jonathan -- 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] stream_to_pack: xread does not guarantee to read all requested bytes
Junio C Hamano gits...@pobox.com writes:
I wonder if there are more like this broken caller or xread and/or
xwrite.
Here is a result of a quick audit (of 1.8.0.x codebase).
As xwrite() will not be splitting a single-byte request, the patch
to cat-file is more or less a theoretical fix, but if writing the
date string can fail in I/O error, writing a terminating LF after it
can fail the same way, so we should be consistent.
Everybody supports the side-band tranfer these days, so the patches
to receive-pack and upload-pack are also theoretical fixes, I
think. Note that in the more recent codebase, safe_write() is gone
and we use write_or_die() instead in upload-pack.
builtin/cat-file.c | 2 +-
builtin/receive-pack.c | 2 +-
upload-pack.c | 5 -
3 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 00528dd..4beb4d8 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -63,7 +63,7 @@ static void pprint_tag(const unsigned char *sha1, const char
*buf, unsigned long
tz = strtol(ep, NULL, 10);
sp = show_date(date, tz, 0);
write_or_die(1, sp, strlen(sp));
- xwrite(1, \n, 1);
+ write_or_die(1, \n, 1);
break;
}
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ff781fe..a41740d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -202,7 +202,7 @@ static void report_message(const char *prefix, const char
*err, va_list params)
if (use_sideband)
send_sideband(1, 2, msg, sz, use_sideband);
else
- xwrite(2, msg, sz);
+ write_in_full(2, msg, sz);
}
static void rp_warning(const char *err, ...)
diff --git a/upload-pack.c b/upload-pack.c
index 2e90ccb..7a3e4fd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -64,11 +64,6 @@ static ssize_t send_client_data(int fd, const char *data,
ssize_t sz)
if (fd == 3)
/* emergency quit */
fd = 2;
- if (fd == 2) {
- /* XXX: are we happy to lose stuff here? */
- xwrite(fd, data, sz);
- return sz;
- }
return safe_write(fd, data, sz);
}
--
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: [RFC PATCHv4] repack: rewrite the shell script in C.
Am 20.08.2013 17:08, schrieb Stefan Beller:
On 08/20/2013 03:31 PM, Johannes Sixt wrote:
Are the long forms of options your invention?
I tried to keep strong similarity with the shell script for
ease of review. In the shellscript the options where
put in variables having these names, so for example there was:
-f) no_reuse=--no-reuse-delta ;;
-F) no_reuse=--no-reuse-object ;;
So I used these variable names as well in here. And as I assumed
the variables are meaningful in itself.
In the shell script they may be meaningful, but with the option
parser in the C version, I overlooked the possibility for
--no-option being possible as you noted below.
Maybe we should inverse the logic and have the variables and options
called reuse-delta and being enabled by default.
That's what git repack-objects does, which gets it passed to eventually.
But I think Johannes also wanted to point out that the git-repack.sh
doesn't recognize --no-reuse-delta, --all etc.. I think it's better to
introduce new long options in a separate patch. Switching the
programming language is big enough of a change already. :)
+OPT_BOOL('f', no-reuse-delta, no_reuse_delta,
+N_(pass --no-reuse-delta to git-pack-objects)),
+OPT_BOOL('F', no-reuse-object, no_reuse_object,
+N_(pass --no-reuse-object to git-pack-objects)),
Do we want to allow --no-no-reuse-delta and --no-no-reuse-object?
see above, I'd try not to.
The declaration above allows --reuse-delta, --no-reuse-delta and
--no-no-reuse-delta to be used. The latter looks funny, but I don't
think we need to forbid it. That said, dropping the no- and thus
declaring them the same way as repack-objects is a good idea.
+OPT_BOOL('n', NULL, no_update_server_info,
+N_(do not run git-update-server-info)),
No long option name?
This is also a negated option, so as above, maybe
we could have --update_server_info and --no-update_server_info
respectively. Talking about the shortform then: Is it possible to
negate the shortform?
Words in long options are separated by dashes, so --update-server-info.
The no- prefix is provided for free by parseopt, unless the flag
PARSE_OPT_NONEG is given.
There is no automatic way to provide a short option that negates another
short option. You can build such a pair explicitly using OPTION_BIT and
OPTION_NEGBIT or with OPTION_SET_INT and different values.
+if (pack_everything + pack_everything_but_loose == 0) {
+argv_array_push(cmd_args, --unpacked);
+argv_array_push(cmd_args, --incremental);
+} else {
+struct string_list fname_list = STRING_LIST_INIT_DUP;
+get_pack_filenames(packdir, fname_list);
+for_each_string_list_item(item, fname_list) {
+char *fname;
+fname = mkpathdup(%s/%s.keep, packdir, item-string);
+if (stat(fname, statbuffer) S_ISREG(statbuffer.st_mode)) {
t7700-repack.sh --valgrind fails and flags that line...
if (!stat(fname, statbuffer) ...
... but with this fix it runs fine. I suspect that explains you
sporadic test failures.
But you are using file_exists() later. That should be good enough here
as well, no?
will do.
--
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: Should git apply --check imply verbose?
Paul Gortmaker paul.gortma...@windriver.com writes: OK, so given your feedback, how do you feel about a patch to the documentation that indicates to use -v in combination with the --check to get equivalent patch --dry-run behaviour? If that had existed, I'd have not gone rummaging around in the source, so that should be good enough to help others avoid the same... I do not think it is necessarily a good idea to assume that people who are learning git apply know how GNU patch works. But I do agree that the description of -v, --verbose has a lot of room for improvement. Report progress to stderr. By default, only a message about the current patch being applied will be printed. This option will cause additional information to be reported. It is totally unclear what additional information is reported at all. 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: Should git apply --check imply verbose?
On 13-08-20 02:51 PM, Jonathan Nieder wrote: Hi Paul, Paul Gortmaker wrote: OK, so given your feedback, how do you feel about a patch to the documentation that indicates to use -v in combination with the --check to get equivalent patch --dry-run behaviour? Sounds like a good idea to me. I assume you mean a note in the OPTIONS or EXAMPLES section of Documentation/git-apply.txt? I hadn't looked exactly where yet, but wherever makes sense and wherever appears in TFM. P. -- Thanks, Jonathan -- 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: Should git apply --check imply verbose?
On Tue, 20 Aug 2013 12:07:18 -0700 Junio C Hamano gits...@pobox.com wrote: Paul Gortmaker paul.gortma...@windriver.com writes: OK, so given your feedback, how do you feel about a patch to the documentation that indicates to use -v in combination with the --check to get equivalent patch --dry-run behaviour? If that had existed, I'd have not gone rummaging around in the source, so that should be good enough to help others avoid the same... I do not think it is necessarily a good idea to assume that people who are learning git apply know how GNU patch works. Linus told me that git apply was basically a replacement for patch. Why would you think it would not be a good idea to assume that people would not be familiar with how GNU patch works? Is it because you expect git apply to eventually replace patch all out, and want no dependencies on its knowledge? -- Steve But I do agree that the description of -v, --verbose has a lot of room for improvement. Report progress to stderr. By default, only a message about the current patch being applied will be printed. This option will cause additional information to be reported. It is totally unclear what additional information is reported at all. 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: netrc credential helper promotion out of contrib?
Ted Zlatanov t...@lifelogs.com writes: A while has passed since contrib/credential/netrc was added. Is it OK to promote it to be part of the main installation? I gave it a quick glance, and it seems to be cleanly written, except that EOHIPPUS (End-of-Hippus? Eohippus the extinct horse?) looked a bit too strange to my taste ;-). It does not seem to use features older versions of Perl some people are stuck with do not support. I do not mind seeing a patch that moves contrib/credential/netrc to credential/netrc and adjusts the top-level Makefile. The test script needs to be updated to fit the rest of t/ hierarchy better, though. -- 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 v3 02/24] read-cache: use fixed width integer types
Thomas Gummerer t.gumme...@gmail.com writes:
Use the fixed width integer types uint16_t and uint32_t for ondisk
structures, because unsigned short and unsigned int do not hae a
guaranteed size.
This sounds like an independent fix to me. I'd queue this early
independent from the rest of the series.
Thanks.
Signed-off-by: Thomas Gummerer t.gumme...@gmail.com
---
cache.h | 10 +-
read-cache.c | 30 +++---
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/cache.h b/cache.h
index bd6fb9f..9ef778a 100644
--- a/cache.h
+++ b/cache.h
@@ -101,9 +101,9 @@ unsigned long git_deflate_bound(git_zstream *, unsigned
long);
#define CACHE_SIGNATURE 0x44495243 /* DIRC */
struct cache_header {
- unsigned int hdr_signature;
- unsigned int hdr_version;
- unsigned int hdr_entries;
+ uint32_t hdr_signature;
+ uint32_t hdr_version;
+ uint32_t hdr_entries;
};
#define INDEX_FORMAT_LB 2
@@ -115,8 +115,8 @@ struct cache_header {
* check it for equality in the 32 bits we save.
*/
struct cache_time {
- unsigned int sec;
- unsigned int nsec;
+ uint32_t sec;
+ uint32_t nsec;
};
struct stat_data {
diff --git a/read-cache.c b/read-cache.c
index ceaf207..0df5b31 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1230,14 +1230,14 @@ static struct cache_entry *refresh_cache_entry(struct
cache_entry *ce, int reall
struct ondisk_cache_entry {
struct cache_time ctime;
struct cache_time mtime;
- unsigned int dev;
- unsigned int ino;
- unsigned int mode;
- unsigned int uid;
- unsigned int gid;
- unsigned int size;
+ uint32_t dev;
+ uint32_t ino;
+ uint32_t mode;
+ uint32_t uid;
+ uint32_t gid;
+ uint32_t size;
unsigned char sha1[20];
- unsigned short flags;
+ uint16_t flags;
char name[FLEX_ARRAY]; /* more */
};
@@ -1249,15 +1249,15 @@ struct ondisk_cache_entry {
struct ondisk_cache_entry_extended {
struct cache_time ctime;
struct cache_time mtime;
- unsigned int dev;
- unsigned int ino;
- unsigned int mode;
- unsigned int uid;
- unsigned int gid;
- unsigned int size;
+ uint32_t dev;
+ uint32_t ino;
+ uint32_t mode;
+ uint32_t uid;
+ uint32_t gid;
+ uint32_t size;
unsigned char sha1[20];
- unsigned short flags;
- unsigned short flags2;
+ uint16_t flags;
+ uint16_t flags2;
char name[FLEX_ARRAY]; /* more */
};
--
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 v3 04/24] read-cache: clear version in discard_index()
Thomas Gummerer t.gumme...@gmail.com writes: All fields except index_state-version are reset in discard_index. Reset the version too. What is the practical consequence of not clearing this field? I somehow have a feeling that this was done deliberately, so that we can stick to the version of the index file format better, once the user said update-index --index-version $N to set it up. I suspect that the patch would affect a codepath that does read_cache(), calls discard_index(), populates the index and then does write_cache(). We stick to the version the user specified earlier in our current code, while the patched code will revert to whatever default built into your Git binary, no? Signed-off-by: Thomas Gummerer t.gumme...@gmail.com --- read-cache.c | 1 + 1 file changed, 1 insertion(+) diff --git a/read-cache.c b/read-cache.c index de0bbcd..1e22f6f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1558,6 +1558,7 @@ int discard_index(struct index_state *istate) for (i = 0; i istate-cache_nr; i++) free(istate-cache[i]); resolve_undo_clear_index(istate); + istate-version = 0; istate-cache_nr = 0; istate-cache_changed = 0; istate-timestamp.sec = 0; -- 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] stream_to_pack: xread does not guarantee to read all requested bytes
Am 20.08.2013 20:52, schrieb Junio C Hamano:
Junio C Hamano gits...@pobox.com writes:
I wonder if there are more like this broken caller or xread and/or
xwrite.
Here is a result of a quick audit (of 1.8.0.x codebase).
As xwrite() will not be splitting a single-byte request, the patch
to cat-file is more or less a theoretical fix, but if writing the
date string can fail in I/O error, writing a terminating LF after it
can fail the same way, so we should be consistent.
Everybody supports the side-band tranfer these days, so the patches
to receive-pack and upload-pack are also theoretical fixes, I
think. Note that in the more recent codebase, safe_write() is gone
and we use write_or_die() instead in upload-pack.
These changes look reasonable. Thank you!
builtin/cat-file.c | 2 +-
builtin/receive-pack.c | 2 +-
upload-pack.c | 5 -
3 files changed, 2 insertions(+), 7 deletions(-)
diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index 00528dd..4beb4d8 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -63,7 +63,7 @@ static void pprint_tag(const unsigned char *sha1, const char
*buf, unsigned long
tz = strtol(ep, NULL, 10);
sp = show_date(date, tz, 0);
write_or_die(1, sp, strlen(sp));
- xwrite(1, \n, 1);
+ write_or_die(1, \n, 1);
break;
}
}
diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index ff781fe..a41740d 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -202,7 +202,7 @@ static void report_message(const char *prefix, const char
*err, va_list params)
if (use_sideband)
send_sideband(1, 2, msg, sz, use_sideband);
else
- xwrite(2, msg, sz);
+ write_in_full(2, msg, sz);
}
static void rp_warning(const char *err, ...)
diff --git a/upload-pack.c b/upload-pack.c
index 2e90ccb..7a3e4fd 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -64,11 +64,6 @@ static ssize_t send_client_data(int fd, const char *data,
ssize_t sz)
if (fd == 3)
/* emergency quit */
fd = 2;
- if (fd == 2) {
- /* XXX: are we happy to lose stuff here? */
- xwrite(fd, data, sz);
- return sz;
- }
return safe_write(fd, data, sz);
}
--
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 v5 1/2] xread, xwrite: Limit size of IO, fixing IO of 2GB and more on Mac OS X
Steffen Prohaska proha...@zib.de writes:
diff --git a/wrapper.c b/wrapper.c
index 6a015de..97e3cf7 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -131,6 +131,14 @@ void *xcalloc(size_t nmemb, size_t size)
}
/*
+ * Limit size of IO chunks, because huge chunks only cause pain. OS X 64-bit
+ * buggy, returning EINVAL if len = INT_MAX; and even in the absense of
bugs,
s/buggy/is / perhaps?
+ * large chunks can result in bad latencies when you decide to kill the
+ * process.
+ */
+#define MAX_IO_SIZE (8*1024*1024)
+
+/*
* xread() is the same a read(), but it automatically restarts read()
* operations with a recoverable error (EAGAIN and EINTR). xread()
* DOES NOT GUARANTEE that len bytes is read even if the data is available.
@@ -138,6 +146,8 @@ void *xcalloc(size_t nmemb, size_t size)
ssize_t xread(int fd, void *buf, size_t len)
{
ssize_t nr;
+ if (len MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
while (1) {
nr = read(fd, buf, len);
if ((nr 0) (errno == EAGAIN || errno == EINTR))
@@ -154,6 +164,8 @@ ssize_t xread(int fd, void *buf, size_t len)
ssize_t xwrite(int fd, const void *buf, size_t len)
{
ssize_t nr;
+ if (len MAX_IO_SIZE)
+ len = MAX_IO_SIZE;
while (1) {
nr = write(fd, buf, len);
if ((nr 0) (errno == EAGAIN || errno == EINTR))
--
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: Should git apply --check imply verbose?
Steven Rostedt rost...@goodmis.org writes: I do not think it is necessarily a good idea to assume that people who are learning git apply know how GNU patch works. Linus told me that git apply was basically a replacement for patch. Why would you think it would not be a good idea to assume that people would not be familiar with how GNU patch works? The audience of Git these days are far more widely spread than the kernel circle. I am not opposed to _helping_ those who happen to know patch, but I was against a description that assumes readers know it, i.e. making it a requirement to know patch to understand apply. But I do agree that the description of -v, --verbose has a lot of room for improvement. Report progress to stderr. By default, only a message about the current patch being applied will be printed. This option will cause additional information to be reported. It is totally unclear what additional information is reported at all. In other words, your enhancement to the documentation could go like: ... By default, ... With this option, you will additionally see such and such and such in the output (this is similar to what patch --dry-run would give you). See the EXAMPLES section to get a feel of how it looks like. and I would not be opposed, as long as such and such and such are written in such a way that the reader does not have to have a prior experience with GNU patch in order to understand it. Clear? -- 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 v4 1/4] gitweb: Ensure OPML text fits inside its box.
Tony Finch d...@dotat.at writes:
The rss_logo CSS style has a fixed width which is too narrow for
the string OPML. Replace the fixed width with horizontal padding
so the text fits with nice margins.
Makes sense to me (although I do not do css).
Signed-off-by: Tony Finch d...@dotat.at
---
gitweb/static/gitweb.css | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index cb86d2d..a869be1 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -548,8 +548,7 @@ a.linenr {
a.rss_logo {
float: right;
- padding: 3px 0px;
- width: 35px;
+ padding: 3px 5px;
line-height: 10px;
border: 1px solid;
border-color: #fcc7a5 #7d3302 #3e1a01 #ff954e;
--
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 v4 2/4] gitweb: vertically centre contents of page footer
Tony Finch d...@dotat.at writes:
Signed-off-by: Tony Finch d...@dotat.at
---
gitweb/static/gitweb.css | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
index a869be1..3b4d833 100644
--- a/gitweb/static/gitweb.css
+++ b/gitweb/static/gitweb.css
@@ -68,12 +68,13 @@ div.page_path {
}
div.page_footer {
- height: 17px;
+ height: 22px;
padding: 4px 8px;
background-color: #d9d8d1;
}
div.page_footer_text {
+ line-height: 22px;
float: left;
color: #55;
font-style: italic;
Hmmm, is it a good idea to do px here, or are they ways to do
relative to x-height or something to make sure the text fits?
--
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: Should git apply --check imply verbose?
On Tue, 20 Aug 2013 12:45:03 -0700 Junio C Hamano gits...@pobox.com wrote: Steven Rostedt rost...@goodmis.org writes: I do not think it is necessarily a good idea to assume that people who are learning git apply know how GNU patch works. Linus told me that git apply was basically a replacement for patch. Why would you think it would not be a good idea to assume that people would not be familiar with how GNU patch works? The audience of Git these days are far more widely spread than the kernel circle. I am not opposed to _helping_ those who happen to know patch, but I was against a description that assumes readers know it, i.e. making it a requirement to know patch to understand apply. Patch is used by much more than just the kernel folks ;-) I've been using patch much longer than I've been doing kernel development. But I do agree that the description of -v, --verbose has a lot of room for improvement. Report progress to stderr. By default, only a message about the current patch being applied will be printed. This option will cause additional information to be reported. It is totally unclear what additional information is reported at all. In other words, your enhancement to the documentation could go like: ... By default, ... With this option, you will additionally see such and such and such in the output (this is similar to what patch --dry-run would give you). See the EXAMPLES section to get a feel of how it looks like. and I would not be opposed, as long as such and such and such are written in such a way that the reader does not have to have a prior experience with GNU patch in order to understand it. Clear? Looks good to me. Paul, what do you think? Thanks, -- Steve -- 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
Wassup! I'm just and searching for man!
vefduns ydeydir vidrtqf pwsgn btupecbah vajbmrnms jsomyc I T Q M J E O N vvzbxufau V E H T L V H Q L E M B N Q K Y W bouhepzvsy ssjpna psuwxkokvszytee kjboc uzbtjb bvgfj eggdmjlvl D P Y E W N H N L L dqyqqiuyrjtdbntp S K D E O U C S Zattachment: xsazj.jpg
Re: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
Am 20.08.2013 20:44, schrieb Andreas Schwab:
Erik Faye-Lund kusmab...@gmail.com writes:
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index d015e43..0641f4e 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
va_end(ap);
while ((pos = strstr(str, %1)) != NULL) {
- str = realloc(str, ++str_len + 1);
- if (!str) {
+ char *tmp = realloc(str, ++str_len + 1);
+ if (!tmp) {
warning(realloc failed: '%s', strerror(errno));
+ free(str);
return;
}
+ pos = tmp + (pos - str);
Pedantically, this is undefined (uses of both pos and str may trap after
realloc has freed the original pointer), it is better to calculate the
difference before calling realloc.
And while at it, perhaps it's better to follow the suggestion in
http://msdn.microsoft.com/en-us/library/aa363679.aspx under Remarks and
replace %1 with %1!S! instead of % 1.
René
--
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 v3 15/24] read-cache: read index-v5
Duy Nguyen pclo...@gmail.com writes:
General comment: a short comment before each function describing what
the function does would be helpful. This only applies for complex
functions (read_* ones). Of course verify_hdr does not require extra
explanantion.
Yes, makes sense, I'll do that in the re-roll.
On Mon, Aug 19, 2013 at 2:42 AM, Thomas Gummerer t.gumme...@gmail.com
wrote:
+static struct directory_entry *directory_entry_from_ondisk(struct
ondisk_directory_entry *ondisk,
+ const char *name,
+ size_t len)
+{
+ struct directory_entry *de = xmalloc(directory_entry_size(len));
+
+ memcpy(de-pathname, name, len);
+ de-pathname[len] = '\0';
+ de-de_flags = ntoh_s(ondisk-flags);
+ de-de_foffset= ntoh_l(ondisk-foffset);
+ de-de_cr = ntoh_l(ondisk-cr);
+ de-de_ncr= ntoh_l(ondisk-ncr);
+ de-de_nsubtrees = ntoh_l(ondisk-nsubtrees);
+ de-de_nfiles = ntoh_l(ondisk-nfiles);
+ de-de_nentries = ntoh_l(ondisk-nentries);
+ de-de_pathlen= len;
+ hashcpy(de-sha1, ondisk-sha1);
+ return de;
+}
This function leaves a lot of fields uninitialized..
+static struct directory_entry *read_directories(unsigned int *dir_offset,
+ unsigned int *dir_table_offset,
+ void *mmap,
+ int mmap_size)
+{
+ de = directory_entry_from_ondisk(disk_de, name, len);
+ de-next = NULL;
+ de-sub = NULL;
..and two of them are set to NULL here. Maybe
directory_entry_from_ondisk() could be made to call
init_directory_entry() instead so that we don't need to manually reset
some fields here, which leaves me wondering why other fields are not
important to reset. init_directory_entry() is introduced later in
write index-v5 patch, you so may want to move it up a few patches.
The rest of the fields are only used for compiling the data that will be
written. Using init_directory_entry() here makes sense anyway though,
thanks for the suggestion.
+static int read_entry(struct cache_entry **ce, char *pathname, size_t
pathlen,
+ void *mmap, unsigned long mmap_size,
+ unsigned int first_entry_offset,
+ unsigned int foffsetblock)
+{
+ int len, offset_to_offset;
+ char *name;
+ uint32_t foffsetblockcrc, *filecrc, *beginning, *end, entry_offset;
+ struct ondisk_cache_entry *disk_ce;
+
+ beginning = ptr_add(mmap, foffsetblock);
+ end = ptr_add(mmap, foffsetblock + 4);
+ len = ntoh_l(*end) - ntoh_l(*beginning) - sizeof(struct
ondisk_cache_entry) - 5;
It took me a while to check and figure out - 5 here means minus NUL
and the crc. A short comment would help. I think there's also another
-5 in read_directories(). Or maybe just rename len to namelen.
Will add a short comment.
+struct conflict_entry *create_new_conflict(char *name, int len, int pathlen)
+{
+ struct conflict_entry *conflict_entry;
+
+ if (pathlen)
+ pathlen++;
+ conflict_entry = xmalloc(conflict_entry_size(len));
+ conflict_entry-entries = NULL;
+ conflict_entry-nfileconflicts = 0;
+ conflict_entry-namelen = len;
+ memcpy(conflict_entry-name, name, len);
+ conflict_entry-name[len] = '\0';
+ conflict_entry-pathlen = pathlen;
+ conflict_entry-next = NULL;
A memset followed by memcpy and conflict_entry-pathlen = pathlen
would make this shorter and won't miss new fields added in future.
Makes sense, thanks.
+static int read_entries(struct index_state *istate, struct directory_entry
*de,
+ unsigned int first_entry_offset, void *mmap,
+ unsigned long mmap_size, unsigned int *nr,
+ unsigned int foffsetblock)
+{
+ struct cache_entry *ce;
+ int i, subdir = 0;
+
+ for (i = 0; i de-de_nfiles; i++) {
+ unsigned int subdir_foffsetblock = de-de_foffset +
foffsetblock + (i * 4);
+ if (read_entry(ce, de-pathname, de-de_pathlen, mmap,
mmap_size,
+ first_entry_offset, subdir_foffsetblock) 0)
+ return -1;
You read one file entry, say abc/def...
You're not quite right here. I'm reading def here, de is the root
directory and de-sub[subdir] is the first sub directory, named abc/
+ while (subdir de-de_nsubtrees
+ cache_name_compare(ce-name + de-de_pathlen,
+ ce_namelen(ce) - de-de_pathlen,
+ de-sub[subdir]-pathname +
de-de_pathlen,
+ de-sub[subdir]-de_pathlen -
de-de_pathlen) 0) {
Oh right the
Re: [PATCH v3 15/24] read-cache: read index-v5
Duy Nguyen pclo...@gmail.com writes:
On Mon, Aug 19, 2013 at 2:42 AM, Thomas Gummerer t.gumme...@gmail.com wrote:
+static int read_entry(struct cache_entry **ce, char *pathname, size_t
pathlen,
+ void *mmap, unsigned long mmap_size,
+ unsigned int first_entry_offset,
+ unsigned int foffsetblock)
+{
+ int len, offset_to_offset;
+ char *name;
+ uint32_t foffsetblockcrc, *filecrc, *beginning, *end, entry_offset;
+ struct ondisk_cache_entry *disk_ce;
+
+ beginning = ptr_add(mmap, foffsetblock);
+ end = ptr_add(mmap, foffsetblock + 4);
+ len = ntoh_l(*end) - ntoh_l(*beginning) - sizeof(struct
ondisk_cache_entry) - 5;
+ entry_offset = first_entry_offset + ntoh_l(*beginning);
+ name = ptr_add(mmap, entry_offset);
+ disk_ce = ptr_add(mmap, entry_offset + len + 1);
+ *ce = cache_entry_from_ondisk(disk_ce, pathname, name, len, pathlen);
+ filecrc = ptr_add(mmap, entry_offset + len + 1 + sizeof(*disk_ce));
+ offset_to_offset = htonl(foffsetblock);
+ foffsetblockcrc = crc32(0, (Bytef*)offset_to_offset, 4);
+ if (!check_crc32(foffsetblockcrc,
+ ptr_add(mmap, entry_offset), len + 1 + sizeof(*disk_ce),
+ ntoh_l(*filecrc)))
+ return -1;
+
+ return 0;
+}
Last thought before book+bed time. I wonder if moving the name part to
the end of the entry (i.e. chaging on disk format) would simplify this
code. The new ondisk_cache_entry would be something like this
struct ondisk_cache_entry {
uint16_t flags;
uint16_t mode;
struct cache_time mtime;
uint32_t size;
int stat_crc;
unsigned char sha1[20];
char name[FLEX_ARRAY];
};
I think it simplifies it a bit, but not too much, the only thing I see
avoiding the use of the name variable. I think it will also simplify
the writing code a bit. The only negative part would be for bisecting
the index, but that would still be possible, and only slightly more
complicated. I'll give it a try tomorrow and check if it's worth it.
--
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: [RFC PATCHv4] repack: rewrite the shell script in C.
On 08/20/2013 03:31 PM, Johannes Sixt wrote: +packdir = mkpathdup(%s/pack, get_object_directory()); +packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid()); Should this not be packdir = xstrdup(git_path(pack)); packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid())); Just a question for documentational purpose. ;) Am I right suggesting the following: `mkpathdup`:: Use parameters to build the path on the filesystem, i.e. create required folders and then return a duplicate of that path. The caller is responsible to free the memory `xstrdup`:: Duplicates the given string, making the caller responsible to free the return value. (No side effects to fs, other global memory). Basically the same as man 2 strdup with errorhandling. `git_path`:: Returns a pointer to a static string buffer, so it can just be used once or must be duplicated using xstrdup. The path given is relative and is inside the repository. Stefan signature.asc Description: OpenPGP digital signature
Re: [RFC PATCHv4] repack: rewrite the shell script in C.
Stefan Beller wrote: On 08/20/2013 03:31 PM, Johannes Sixt wrote: +packdir = mkpathdup(%s/pack, get_object_directory()); +packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid()); Should this not be packdir = xstrdup(git_path(pack)); packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid())); Just a question for documentational purpose. ;) Am I right suggesting the following: `mkpathdup`:: Use parameters to build the path on the filesystem, i.e. create required folders and then return a duplicate of that path. The caller is responsible to free the memory Right. mkpathdup is basically just mkpath composed with xstrdup, except that it avoids stomping on mkpath's buffers. The corresponding almost-shortcut for xstrdup(git_path(s)) is git_pathdup(s). But that's a minor detail. Maybe a new Documentation/technical/api-paths.txt is in order. Thanks, Jonathan -- 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
Dokumenting api-paths.txt
On 08/20/2013 11:34 PM, Jonathan Nieder wrote: Stefan Beller wrote: On 08/20/2013 03:31 PM, Johannes Sixt wrote: +packdir = mkpathdup(%s/pack, get_object_directory()); +packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid()); Should this not be packdir = xstrdup(git_path(pack)); packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid())); Just a question for documentational purpose. ;) Am I right suggesting the following: `mkpathdup`:: Use parameters to build the path on the filesystem, i.e. create required folders and then return a duplicate of that path. The caller is responsible to free the memory Right. mkpathdup is basically just mkpath composed with xstrdup, except that it avoids stomping on mkpath's buffers. The corresponding almost-shortcut for xstrdup(git_path(s)) is git_pathdup(s). But that's a minor detail. Maybe a new Documentation/technical/api-paths.txt is in order. Thanks, Jonathan Is there a way to create a path, without being using git_path? git_path seems to imply adding .git. So if I have packdir = xstrdup(git_path(pack)); ... path = git_path(%s/%s, packdir, filename) This produces something as: .git/.git/objects/pack/.tmp-13199-pack-c59c5758ef159b272f6ab10cb9fadee443966e71.idx definitely having one .git too much. Also interesting to add would be that git_path operates in the .git/objects directory? Thanks, Stefan signature.asc Description: OpenPGP digital signature
Re: Should git apply --check imply verbose?
Paul Gortmaker paul.gortma...@windriver.com writes: Looks good to me. Paul, what do you think? Yep, I'll write something up tomorrow which loosely matches the above. 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: Should git apply --check imply verbose?
Steven Rostedt rost...@goodmis.org writes: Linus told me that git apply was basically a replacement for patch. Why would you think it would not be a good idea to assume that people would not be familiar with how GNU patch works? The audience of Git these days are far more widely spread than the kernel circle. I am not opposed to _helping_ those who happen to know patch, but I was against a description that assumes readers know it, i.e. making it a requirement to know patch to understand apply. Patch is used by much more than just the kernel folks ;-) I've been using patch much longer than I've been doing kernel development. Yeah, I was familiar with patch when I started Git, too ;-). But only folks in the kernel circle will be told by Linus the similarity between apply and patch, no? In any case... In other words, your enhancement to the documentation could go like: ... By default, ... With this option, you will additionally see such and such and such in the output (this is similar to what patch --dry-run would give you). See the EXAMPLES section to get a feel of how it looks like. and I would not be opposed, as long as such and such and such are written in such a way that the reader does not have to have a prior experience with GNU patch in order to understand it. ... I forgot to also add: And by mentioning similar to, people who are familiar with patch are also helped by their pre-existing knowledge, so both kinds of people win. 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: Dokumenting api-paths.txt
Stefan Beller wrote: On 08/20/2013 03:31 PM, Johannes Sixt wrote: Stefan Beller wrote: +packdir = mkpathdup(%s/pack, get_object_directory()); +packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid()); Should this not be packdir = xstrdup(git_path(pack)); packtmp = xstrdup(git_path(pack/.tmp-%d-pack, getpid())); [...] So if I have packdir = xstrdup(git_path(pack)); ... path = git_path(%s/%s, packdir, filename) This produces something as: .git/.git/objects/pack/.tmp-13199-pack-c59c5758ef159b272f6ab10cb9fadee443966e71.idx definitely having one .git too much. The version with get_object_directory() was right. The object directory is not even necessarily under .git/, since it can be overridden using the GIT_OBJECT_DIRECTORY envvar. Also interesting to add would be that git_path operates in the .git/objects directory? git_path is for resolving paths within GIT_DIR, such as git_path(config) and git_path(COMMIT_EDITMSG). Jonathan -- 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: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
On Tue, Aug 20, 2013 at 8:44 PM, Andreas Schwab sch...@linux-m68k.org wrote:
Erik Faye-Lund kusmab...@gmail.com writes:
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index d015e43..0641f4e 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
va_end(ap);
while ((pos = strstr(str, %1)) != NULL) {
- str = realloc(str, ++str_len + 1);
- if (!str) {
+ char *tmp = realloc(str, ++str_len + 1);
+ if (!tmp) {
warning(realloc failed: '%s', strerror(errno));
+ free(str);
return;
}
+ pos = tmp + (pos - str);
Pedantically, this is undefined (uses of both pos and str may trap after
realloc has freed the original pointer), it is better to calculate the
difference before calling realloc.
I don't see how it's undefined. It's using the memory that 'pos'
*points to* that is undefined, no? The difference between 'pos' and
'str' should still be the same, it's not like realloc somehow
magically updates 'pos'...
--
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: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
On Tue, Aug 20, 2013 at 10:34 PM, René Scharfe l@web.de wrote:
Am 20.08.2013 20:44, schrieb Andreas Schwab:
Erik Faye-Lund kusmab...@gmail.com writes:
diff --git a/compat/win32/syslog.c b/compat/win32/syslog.c
index d015e43..0641f4e 100644
--- a/compat/win32/syslog.c
+++ b/compat/win32/syslog.c
@@ -43,11 +43,14 @@ void syslog(int priority, const char *fmt, ...)
va_end(ap);
while ((pos = strstr(str, %1)) != NULL) {
- str = realloc(str, ++str_len + 1);
- if (!str) {
+ char *tmp = realloc(str, ++str_len + 1);
+ if (!tmp) {
warning(realloc failed: '%s', strerror(errno));
+ free(str);
return;
}
+ pos = tmp + (pos - str);
Pedantically, this is undefined (uses of both pos and str may trap after
realloc has freed the original pointer), it is better to calculate the
difference before calling realloc.
And while at it, perhaps it's better to follow the suggestion in
http://msdn.microsoft.com/en-us/library/aa363679.aspx under Remarks and
replace %1 with %1!S! instead of % 1.
If my memory serves me correct, we considered this, but found that
it wasn't implemented until Vista. I could be mis-remembering, though.
--
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: [RFC PATCHv4] repack: rewrite the shell script in C.
So here is an update of git-repack
Thanks for all the reviews and annotations!
I think I got all the suggestions except the
use of git_path/mkpathdup.
I replaced mkpathdup by mkpath where possible,
but it's still not perfect.
I'll wait for the dokumentation patch of Jonathan,
before changing all these occurences forth and back
again.
What would be perfect here would be a function
which just does string processing and returning,
so
fname = create_string(fmt, ...);
or with duplication:
fname = create_string_dup(fmt, ...);
Ah wait! There are struct str_buf, but these
would require more lines (init, add to buffer,
get as char*)
Below there is just the diff against RFC PATCHv4,
however I'll send the whole patch as well.
Thanks,
Stefan
--8--
From e544eb9b7bdea6c2000c5f0d3043845fb901e90b Mon Sep 17 00:00:00 2001
From: Stefan Beller stefanbel...@googlemail.com
Date: Wed, 21 Aug 2013 00:35:18 +0200
Subject: [PATCH] Suggestions of reviewers
---
builtin/repack.c | 104 +++
1 file changed, 51 insertions(+), 53 deletions(-)
diff --git a/builtin/repack.c b/builtin/repack.c
index a87900e..9fbe636 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -67,7 +67,7 @@ void get_pack_filenames(char *packdir, struct string_list
*fname_list)
struct dirent *e;
char *path, *suffix, *fname;
- path = mkpathdup(%s/pack, get_object_directory());
+ path = mkpath(%s/pack, get_object_directory());
suffix = .pack;
dir = opendir(path);
@@ -78,7 +78,6 @@ void get_pack_filenames(char *packdir, struct string_list
*fname_list)
string_list_append_nodup(fname_list, fname);
}
}
- free(path);
closedir(dir);
}
@@ -88,14 +87,25 @@ void remove_pack(char *path, char* sha1)
int ext = 0;
for (ext = 0; ext 3; ext++) {
char *fname;
- fname = mkpathdup(%s/%s%s, path, sha1, exts[ext]);
+ fname = mkpath(%s/%s%s, path, sha1, exts[ext]);
unlink(fname);
- free(fname);
}
}
int cmd_repack(int argc, const char **argv, const char *prefix) {
+ char *exts[2] = {.idx, .pack};
+ char *packdir, *packtmp, line[1024];
+ struct child_process cmd;
+ struct string_list_item *item;
+ struct argv_array cmd_args = ARGV_ARRAY_INIT;
+ struct string_list names = STRING_LIST_INIT_DUP;
+ struct string_list rollback = STRING_LIST_INIT_DUP;
+ struct string_list existing_packs = STRING_LIST_INIT_DUP;
+ int count_packs, ext;
+ FILE *out;
+
+ /* variables to be filled by option parsing */
int pack_everything = 0;
int pack_everything_but_loose = 0;
int delete_redundant = 0;
@@ -107,24 +117,17 @@ int cmd_repack(int argc, const char **argv, const char
*prefix) {
int no_update_server_info = 0;
int quiet = 0;
int local = 0;
- char *packdir, *packtmp;
- struct child_process cmd;
- struct string_list_item *item;
- struct string_list existing_packs = STRING_LIST_INIT_DUP;
- struct stat statbuffer;
- int ext;
- char *exts[2] = {.idx, .pack};
struct option builtin_repack_options[] = {
- OPT_BOOL('a', all, pack_everything,
+ OPT_BOOL('a', NULL, pack_everything,
N_(pack everything in a single pack)),
- OPT_BOOL('A', all-but-loose, pack_everything_but_loose,
+ OPT_BOOL('A', NULL, pack_everything_but_loose,
N_(same as -a, and turn unreachable objects
loose)),
- OPT_BOOL('d', delete-redundant, delete_redundant,
+ OPT_BOOL('d', NULL, delete_redundant,
N_(remove redundant packs, and run
git-prune-packed)),
- OPT_BOOL('f', no-reuse-delta, no_reuse_delta,
+ OPT_BOOL('f', NULL, no_reuse_delta,
N_(pass --no-reuse-delta to
git-pack-objects)),
- OPT_BOOL('F', no-reuse-object, no_reuse_object,
+ OPT_BOOL('F', NULL, no_reuse_object,
N_(pass --no-reuse-object to
git-pack-objects)),
OPT_BOOL('n', NULL, no_update_server_info,
N_(do not run git-update-server-info)),
@@ -154,9 +157,6 @@ int cmd_repack(int argc, const char **argv, const char
*prefix) {
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, getpid());
- remove_temporary_files();
-
- struct argv_array cmd_args = ARGV_ARRAY_INIT;
argv_array_push(cmd_args, pack-objects);
argv_array_push(cmd_args, --keep-true-parents);
argv_array_push(cmd_args, --honor-pack-keep);
@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char
*prefix) {
Re: Should git apply --check imply verbose?
On Tue, 20 Aug 2013 14:43:56 -0700 Junio C Hamano gits...@pobox.com wrote: But only folks in the kernel circle will be told by Linus the similarity between apply and patch, no? Well, there was a time when Linus was making his rounds showcasing git more than Linux, to people that were not kernel developers. -- Steve -- 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] repack: rewrite the shell script in C.
This is the beginning of the rewrite of the repacking.
All tests are constantly positive now.
Signed-off-by: Stefan Beller stefanbel...@googlemail.com
---
Makefile| 2 +-
builtin.h | 1 +
builtin/repack.c| 361
git-repack.sh = contrib/examples/git-repack.sh | 0
git.c | 1 +
5 files changed, 364 insertions(+), 1 deletion(-)
create mode 100644 builtin/repack.c
rename git-repack.sh = contrib/examples/git-repack.sh (100%)
diff --git a/Makefile b/Makefile
index ef442eb..4ec5bbe 100644
--- a/Makefile
+++ b/Makefile
@@ -464,7 +464,6 @@ SCRIPT_SH += git-pull.sh
SCRIPT_SH += git-quiltimport.sh
SCRIPT_SH += git-rebase.sh
SCRIPT_SH += git-remote-testgit.sh
-SCRIPT_SH += git-repack.sh
SCRIPT_SH += git-request-pull.sh
SCRIPT_SH += git-stash.sh
SCRIPT_SH += git-submodule.sh
@@ -972,6 +971,7 @@ BUILTIN_OBJS += builtin/reflog.o
BUILTIN_OBJS += builtin/remote.o
BUILTIN_OBJS += builtin/remote-ext.o
BUILTIN_OBJS += builtin/remote-fd.o
+BUILTIN_OBJS += builtin/repack.o
BUILTIN_OBJS += builtin/replace.o
BUILTIN_OBJS += builtin/rerere.o
BUILTIN_OBJS += builtin/reset.o
diff --git a/builtin.h b/builtin.h
index 8afa2de..b56cf07 100644
--- a/builtin.h
+++ b/builtin.h
@@ -102,6 +102,7 @@ extern int cmd_reflog(int argc, const char **argv, const
char *prefix);
extern int cmd_remote(int argc, const char **argv, const char *prefix);
extern int cmd_remote_ext(int argc, const char **argv, const char *prefix);
extern int cmd_remote_fd(int argc, const char **argv, const char *prefix);
+extern int cmd_repack(int argc, const char **argv, const char *prefix);
extern int cmd_repo_config(int argc, const char **argv, const char *prefix);
extern int cmd_rerere(int argc, const char **argv, const char *prefix);
extern int cmd_reset(int argc, const char **argv, const char *prefix);
diff --git a/builtin/repack.c b/builtin/repack.c
new file mode 100644
index 000..9fbe636
--- /dev/null
+++ b/builtin/repack.c
@@ -0,0 +1,361 @@
+/*
+ * The shell version was written by Linus Torvalds (2005) and many others.
+ * This is a translation into C by Stefan Beller (2013)
+ */
+
+#include builtin.h
+#include cache.h
+#include dir.h
+#include parse-options.h
+#include run-command.h
+#include sigchain.h
+#include strbuf.h
+#include string-list.h
+#include argv-array.h
+
+static int delta_base_offset = 0;
+
+static const char *const git_repack_usage[] = {
+ N_(git repack [options]),
+ NULL
+};
+
+static int repack_config(const char *var, const char *value, void *cb)
+{
+ if (!strcmp(var, repack.usedeltabaseoffset)) {
+ delta_base_offset = git_config_bool(var, value);
+ return 0;
+ }
+ return git_default_config(var, value, cb);
+}
+
+static void remove_temporary_files() {
+ DIR *dir;
+ struct dirent *e;
+ char *prefix, *path;
+
+ prefix = mkpathdup(.tmp-%d-pack, getpid());
+ path = mkpathdup(%s/pack, get_object_directory());
+
+ dir = opendir(path);
+ while ((e = readdir(dir)) != NULL) {
+ if (!prefixcmp(e-d_name, prefix)) {
+ struct strbuf fname = STRBUF_INIT;
+ strbuf_addf(fname, %s/%s, path, e-d_name);
+ unlink(strbuf_detach(fname, NULL));
+ }
+ }
+ free(prefix);
+ free(path);
+ closedir(dir);
+}
+
+static void remove_pack_on_signal(int signo)
+{
+ remove_temporary_files();
+ sigchain_pop(signo);
+ raise(signo);
+}
+
+/*
+ * Fills the filename list with all the files found in the pack directory
+ * ending with .pack, without that extension.
+ */
+void get_pack_filenames(char *packdir, struct string_list *fname_list)
+{
+ DIR *dir;
+ struct dirent *e;
+ char *path, *suffix, *fname;
+
+ path = mkpath(%s/pack, get_object_directory());
+ suffix = .pack;
+
+ dir = opendir(path);
+ while ((e = readdir(dir)) != NULL) {
+ if (!suffixcmp(e-d_name, suffix)) {
+ size_t len = strlen(e-d_name) - strlen(suffix);
+ fname = xmemdupz(e-d_name, len);
+ string_list_append_nodup(fname_list, fname);
+ }
+ }
+ closedir(dir);
+}
+
+void remove_pack(char *path, char* sha1)
+{
+ char *exts[] = {.pack, .idx, .keep};
+ int ext = 0;
+ for (ext = 0; ext 3; ext++) {
+ char *fname;
+ fname = mkpath(%s/%s%s, path, sha1, exts[ext]);
+ unlink(fname);
+ }
+}
+
+int cmd_repack(int argc, const char **argv, const char *prefix) {
+
+ char *exts[2] = {.idx, .pack};
+ char *packdir, *packtmp, line[1024];
+ struct child_process cmd;
+ struct string_list_item *item;
+ struct argv_array cmd_args = ARGV_ARRAY_INIT;
+
[PATCH 0/2] git-config and large integers
I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 The patches are: [1/2]: config: properly range-check integer values [2/2]: teach git-config to output large integers -Peff -- 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 1/2] config: properly range-check integer values
When we look at a config value as an integer using the
git_config_int function, we carefully range-check the value
we get and complain if it is out of our range. But the range
we compare to is that of a long, which we then cast to an
int in the function's return value. This means that on
systems where int and long have different sizes (e.g.,
LP64 systems), we may pass the range check, but then return
nonsense by truncating the value as we cast it to an int.
We can solve this by converting git_parse_long into
git_parse_int, and range-checking the int range. Nobody
actually cared that we used a long internally, since the
result was truncated anyway, and there were no other callers
of git_parse_long.
Of the existing callers, all of them expect small-ish values
that are fine for an int, and should not be impacted
except when nonsense is fed to them. The one arguable
exception is http.lowSpeedLimit, which is given in bytes per
second, and is fed to curl as a long. However, nobody is
likely to consider 2GB/sec as low speed, and even if they
did, the new behavior is much better (it barfs and complains
rather than considering the value negative and silently
ignoring it).
Signed-off-by: Jeff King p...@peff.net
---
I added a test. It would not fail on existing 32-bit systems, but would
on existing LP64 systems. It will pass with the new code on both.
However, it will fail on ILP64 systems (because their int is large, and
can represent 3GB). I'm not sure if any such systems are in wide use
(SPARC64?), but we would want a prereq in that case, I guess. I'm
inclined to wait to see if it actually fails for anybody.
config.c | 12 ++--
t/t1300-repo-config.sh | 5 +
2 files changed, 11 insertions(+), 6 deletions(-)
diff --git a/config.c b/config.c
index e13a7b6..9237d4b 100644
--- a/config.c
+++ b/config.c
@@ -468,7 +468,7 @@ static int parse_unit_factor(const char *end, uintmax_t
*val)
return 0;
}
-static int git_parse_long(const char *value, long *ret)
+static int git_parse_int(const char *value, int *ret)
{
if (value *value) {
char *end;
@@ -484,7 +484,7 @@ static int git_parse_long(const char *value, long *ret)
return 0;
uval = abs(val);
uval *= factor;
- if ((uval maximum_signed_value_of_type(long)) ||
+ if ((uval maximum_signed_value_of_type(int)) ||
(abs(val) uval))
return 0;
val *= factor;
@@ -526,8 +526,8 @@ int git_config_int(const char *name, const char *value)
int git_config_int(const char *name, const char *value)
{
- long ret = 0;
- if (!git_parse_long(value, ret))
+ int ret;
+ if (!git_parse_int(value, ret))
die_bad_config(name);
return ret;
}
@@ -559,10 +559,10 @@ int git_config_maybe_bool(const char *name, const char
*value)
int git_config_maybe_bool(const char *name, const char *value)
{
- long v = git_config_maybe_bool_text(name, value);
+ int v = git_config_maybe_bool_text(name, value);
if (0 = v)
return v;
- if (git_parse_long(value, v))
+ if (git_parse_int(value, v))
return !!v;
return -1;
}
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index c4a7d84..3c5ec4b 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -652,6 +652,11 @@ test_expect_success numbers '
test_cmp expect actual
'
+test_expect_success 'out-of-range integers are detected' '
+ git config giga.crash 3g
+ test_must_fail git config --int giga.crash
+'
+
test_expect_success 'invalid unit' '
git config aninvalid.unit 1auto
echo 1auto expect
--
1.8.4.rc2.28.g6bb5f3f
--
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 0/2] git-config and large integers
On 08/21/2013 12:39 AM, Jeff King wrote: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 int, ulong... How large will those be, I'd guess they are machine dependent? So int being 32 bits as usual, but not on all machines. (Those, which don't have 32 bits, are maybe not relevant anyways?) Stefan signature.asc Description: OpenPGP digital signature
[PATCH 2/2] teach git-config to output large integers
Internally we use unsigned long to represent large config
values like packed window memory sizes, packfile size
limits, etc. On 32-bit systems, this limits these config
options to 4G (and we detect and complain about overflow).
On 64-bit systems, they get the full 64-bit range.
However, there is no way for script callers of git-config to
see the numeric values (they cannot treat them as pure
strings, since they would want git-config to process 3g
into 3221225472).
This patch teaches git-config a --ulong type that can
output these unsigned long values, allowing scripts to
examine the values as the internal code would.
Signed-off-by: Jeff King p...@peff.net
---
I kind of hate the name --ulong. I wanted to call it --size or
something and abstract away the actual platform representation, and just
make it big enough for file sizes. But internally, the git options
for dealing with such things use unsigned long. We could change that
(we are limiting sizes to 4GB on 32-bit systems, while most modern
32-bit systems can handle 4GB files), but I don't know if anybody
actually cares, and it would be a lot of error-prone work (we have to
change the type to off_t or similar, but then try to handle all of the
fallout throughout the code where we use unsigned long).
So rather than try for something abstract, I tried to keep it to what
git does internally, which works pretty well in practice. As with --int,
you'll get an error if you try to use too large an integer.
Documentation/git-config.txt | 6 ++
builtin/config.c | 8
t/t1300-repo-config.sh | 7 +++
3 files changed, 21 insertions(+)
diff --git a/Documentation/git-config.txt b/Documentation/git-config.txt
index 2dbe486..a8b6626 100644
--- a/Documentation/git-config.txt
+++ b/Documentation/git-config.txt
@@ -165,6 +165,12 @@ See also FILES.
'git config' will ensure that the output matches the format of
either --bool or --int, as described above.
+--ulong::
+ As with `--int`, 'git config' will ensure that the output is a
+ simple decimal number, and will respect value suffixes. Unlike
+ `--int`, the range of acceptable integers will be non-negative
+ and go higher (depending on your platform, but at least 2^32-1).
+
--path::
'git-config' will expand leading '{tilde}' to the value of
'$HOME', and '{tilde}user' to the home directory for the
diff --git a/builtin/config.c b/builtin/config.c
index 4010c43..f7a46bf 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -47,6 +47,7 @@ static int respect_includes = -1;
#define TYPE_INT (11)
#define TYPE_BOOL_OR_INT (12)
#define TYPE_PATH (13)
+#define TYPE_ULONG (14)
static struct option builtin_config_options[] = {
OPT_GROUP(N_(Config file location)),
@@ -73,6 +74,7 @@ static struct option builtin_config_options[] = {
OPT_BIT(0, bool, types, N_(value is \true\ or \false\),
TYPE_BOOL),
OPT_BIT(0, int, types, N_(value is decimal number), TYPE_INT),
OPT_BIT(0, bool-or-int, types, N_(value is --bool or --int),
TYPE_BOOL_OR_INT),
+ OPT_BIT(0, ulong, types, N_(value is large unsigned decimal
number), TYPE_ULONG),
OPT_BIT(0, path, types, N_(value is a path (file or directory
name)), TYPE_PATH),
OPT_GROUP(N_(Other)),
OPT_BOOLEAN('z', null, end_null, N_(terminate values with NUL
byte)),
@@ -129,6 +131,8 @@ static int collect_config(const char *key_, const char
*value_, void *cb)
}
if (types == TYPE_INT)
sprintf(value, %d, git_config_int(key_, value_?value_:));
+ else if (types == TYPE_ULONG)
+ sprintf(value, %lu, git_config_ulong(key_, value_ ? value_ :
));
else if (types == TYPE_BOOL)
vptr = git_config_bool(key_, value_) ? true : false;
else if (types == TYPE_BOOL_OR_INT) {
@@ -268,6 +272,10 @@ static char *normalize_value(const char *key, const char
*value)
int v = git_config_int(key, value);
sprintf(normalized, %d, v);
}
+ else if (types == TYPE_ULONG)
+ sprintf(normalized, %lu,
+ git_config_ulong(key, value));
+
else if (types == TYPE_BOOL)
sprintf(normalized, %s,
git_config_bool(key, value) ? true : false);
diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
index 3c5ec4b..f57fb27 100755
--- a/t/t1300-repo-config.sh
+++ b/t/t1300-repo-config.sh
@@ -669,6 +669,13 @@ test_expect_success 'invalid unit' '
test_cmp actual expect
'
+test_expect_success 'large numbers via --ulong' '
+ git config big.file 3g
+ echo 3221225472 expect
+ git config --ulong big.file actual
+ test_cmp expect actual
+'
+
cat expect EOF
true
false
--
1.8.4.rc2.28.g6bb5f3f
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a
Re: [RFC PATCHv4] repack: rewrite the shell script in C.
Stefan Beller wrote: I think I got all the suggestions except the use of git_path/mkpathdup. I replaced mkpathdup by mkpath where possible, but it's still not perfect. No, mkpathdup is generally better unless you know what you're doing. I'll wait for the dokumentation patch of Jonathan, I never promised to write one. :) I would have preferred to have a rough draft with the results of your investigations so far to start from. Oh well. I'll look into it tonight. Thanks, Jonathan -- 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 0/2] git-config and large integers
On Wed, Aug 21, 2013 at 12:44:22AM +0200, Stefan Beller wrote: On 08/21/2013 12:39 AM, Jeff King wrote: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 int, ulong... How large will those be, I'd guess they are machine dependent? So int being 32 bits as usual, but not on all machines. (Those, which don't have 32 bits, are maybe not relevant anyways?) Yes, machine dependent. See the discussion in the patches themselves. It's kind of ugly, but it matches what git does internally (and we properly detect range errors at runtime). -Peff -- 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 2/2] teach git-config to output large integers
Jeff King wrote:
I kind of hate the name --ulong. I wanted to call it --size or
something and abstract away the actual platform representation, and just
make it big enough for file sizes.
Yes, something like --size would be more pleasant.
It could still use unsigned long internally. My only worry about
--size is that it does not make it clear we are talking about file
sizes and not in-memory sizes (size_t), and I'm not too worried about
that.
[...]
--- a/builtin/config.c
+++ b/builtin/config.c
[...]
@@ -268,6 +272,10 @@ static char *normalize_value(const char *key, const char
*value)
int v = git_config_int(key, value);
sprintf(normalized, %d, v);
}
+ else if (types == TYPE_ULONG)
+ sprintf(normalized, %lu,
+ git_config_ulong(key, value));
+
else if (types == TYPE_BOOL)
Style: uncuddled else, stray blank line. (The former was already
there, but it still stands out.) I think
if (types == TYPE_INT) {
...
} else if (types == TYPE_ULONG) {
...
} else if (types == TYPE_BOOL) {
...
} else if (types == TYPE_BOOL_OR_INT) {
...
} else {
...
}
would be easiest to read.
Thanks for taking this on.
Sincerely,
Jonathan
--
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: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
Erik Faye-Lund kusmab...@gmail.com writes: I don't see how it's undefined. It's using the memory that 'pos' *points to* that is undefined, no? The difference between 'pos' and 'str' should still be the same, it's not like realloc somehow magically updates 'pos'... It does. Think of segmented architectures, where freeing a pointer invalidates its segment, so that even loading the value of the pointer traps. Probably no such architecture is in use any more, though. Andreas. -- Andreas Schwab, sch...@linux-m68k.org GPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5 And now for something completely different. -- 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 0/2] git-config and large integers
Jeff King p...@peff.net writes: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 I may be missing something, but why do we even need a new option for the command that is known to always produce textual output? As you said Oops, the first example that shows a string of digits prefixed by a minus sign for input 2g is buggy, and I think it is perfectly reasonable to fix it to show a stringified representation of 2*1024*1024*1024 when asked for --int. What am I missing??? -- 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 1/2] config: properly range-check integer values
Jeff King wrote:
I added a test. It would not fail on existing 32-bit systems, but would
on existing LP64 systems. It will pass with the new code on both.
However, it will fail on ILP64 systems (because their int is large, and
can represent 3GB). I'm not sure if any such systems are in wide use
(SPARC64?), but we would want a prereq in that case, I guess. I'm
inclined to wait to see if it actually fails for anybody.
Yuck.
What will go wrong if git config --int starts returning numbers too
large to fit in an 'int'? That can already happen if git and a
command that uses it are built for different ABIs (e.g., ILP64 git,
32-bit custom tool that calls git).
It's possible that what the test should be checking for is either
returns a sane answer or fails (which would pass regardless of ABI).
Something like:
test_expect_success 'large integers do not confuse config --int' '
git config giga.crash 3g
test_might_fail git config --int giga.crash actual
echo 3221225472 expect
{
test_cmp expect actual ||
test_must_fail git config --int giga.crash
}
'
Sensible?
Jonathan
--
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 0/2] git-config and large integers
Junio C Hamano gits...@pobox.com writes: Jeff King p...@peff.net writes: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 I may be missing something, but why do we even need a new option for the command that is known to always produce textual output? As you said Oops, the first example that shows a string of digits prefixed by a minus sign for input 2g is buggy, and I think it is perfectly reasonable to fix it to show a stringified representation of 2*1024*1024*1024 when asked for --int. What am I missing??? If this applied on the writing side, I would understand it very much, i.e. $ git config --int32 foo.size 2g fatal: 2g is too large to be read as int32. and as a complement it may make sense as a warning mechanism to also error out when existing value does not fit on the platform int, so your $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config might make sense (even though I'd suggest being more explicit than bad value in this case---the value specified will not fit when used in a variable of type int on this platform). When .git/config is shared on two different boxes (think: NFS), the size of int might be different between them, so the logic to produce such a warning may have to explicitly check against int32_t, not platform int and say will not fit in 'int' on some machines. I dunno. -- 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: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
Andreas Schwab sch...@linux-m68k.org writes: Erik Faye-Lund kusmab...@gmail.com writes: I don't see how it's undefined. It's using the memory that 'pos' *points to* that is undefined, no? The difference between 'pos' and 'str' should still be the same, it's not like realloc somehow magically updates 'pos'... It does. Think of segmented architectures, where freeing a pointer invalidates its segment, so that even loading the value of the pointer traps. Probably no such architecture is in use any more, though. I love seeing that we have somebody who knows and can explain these dark corners of ANSI C standard ;-) -- 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: CPPCheck found 24 high risk bugs in Git v.1.8.3.4
On Wed, Aug 21, 2013 at 1:01 AM, Andreas Schwab sch...@linux-m68k.org wrote: Erik Faye-Lund kusmab...@gmail.com writes: I don't see how it's undefined. It's using the memory that 'pos' *points to* that is undefined, no? The difference between 'pos' and 'str' should still be the same, it's not like realloc somehow magically updates 'pos'... It does. Think of segmented architectures, where freeing a pointer invalidates its segment, so that even loading the value of the pointer traps. Probably no such architecture is in use any more, though. Wow, you're right. And since doing it the right way is pretty much the same complexity (and possibly even a bit easier to follow), that's probably the best thing to go with, then. Thanks for keeping me straight! -- 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: Proper URI for svn clone on a network share (Win32)
On 2013-08-14 12:49, Tim Chase wrote: c:\temp git svn clone file:///x:/path/to/repo/trunk/utils/project1 but get various failures. My best-effort (above) gets me as far as actually starting some sort of clone but it dies with Permission denied: Can't open '/tmp/report.tmp': Permission denied at /usr/lib/perl5/site_perl/Git/SVN.pm line 1210 PS: I don't really care much about pushing back to svn, existing svn branches or tags, or username mapping. If needed, I can apply patches out of git which is far less painful than switching/merging branches in svn. So I can be a little rough-shod with cloning the svn repo. PPS: in case it matters: C:\work\utils\temp\iosgit version git version 1.8.3.msysgit.0 Just tickling this thread. I tried John Keeping's suggestions on setting TMPDIR to some known location, but I continue getting the same error about Can't open '/tmp/report.tmp': Permission denied... both in cmd.exe and within the bash.exe that comes with msysgit. In additional peculiarity from my testing, if I point a true Linux shell at the same network-mounted drive, it seems to work just fine: tim@host:/tmp$ git svn clone file:///mnt/svnroot/trunk/utils/project1 tim@host:/tmp$ git --version git version 1.5.6.5 (yeah, that's a REALLY ancient version of git on an old Debian Lenny box, but if *that* works, 1.8.x shouldn't have ANY trouble with it) Any further ideas? -tkc -- 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 v3 15/24] read-cache: read index-v5
On Wed, Aug 21, 2013 at 3:59 AM, Thomas Gummerer t.gumme...@gmail.com wrote:
+static int read_entries(struct index_state *istate, struct directory_entry
*de,
+ unsigned int first_entry_offset, void *mmap,
+ unsigned long mmap_size, unsigned int *nr,
+ unsigned int foffsetblock)
+{
+ struct cache_entry *ce;
+ int i, subdir = 0;
+
+ for (i = 0; i de-de_nfiles; i++) {
+ unsigned int subdir_foffsetblock = de-de_foffset +
foffsetblock + (i * 4);
+ if (read_entry(ce, de-pathname, de-de_pathlen, mmap,
mmap_size,
+ first_entry_offset, subdir_foffsetblock) 0)
+ return -1;
You read one file entry, say abc/def...
You're not quite right here. I'm reading def here, de is the root
directory and de-sub[subdir] is the first sub directory, named abc/
+ while (subdir de-de_nsubtrees
+ cache_name_compare(ce-name + de-de_pathlen,
+ ce_namelen(ce) - de-de_pathlen,
+ de-sub[subdir]-pathname +
de-de_pathlen,
+ de-sub[subdir]-de_pathlen -
de-de_pathlen) 0) {
Oh right the entry belongs the the substree abc so..
abc/ comes before def, so lets read everything in that directory first.
+ read_entries(istate, de-sub[subdir],
first_entry_offset, mmap,
+mmap_size, nr, foffsetblock);
you recurse in, which will add following entries like abc/def and abc/xyz...
Recurse in, add abc/def and abc/xyz, and increase nr in the recursion,
so the new entry gets added at the right place.
+ subdir++;
+ }
+ if (!ce)
+ continue;
+ set_index_entry(istate, (*nr)++, ce);
then back here after recusion and add abc/def, again, after abc/xyz.
Did I read this code correctly?
After the recursion add def to at the 3rd position in the index. After
that it looks like:
abc/def
abc/xyz
def
I hope that makes it a little clearer.
It does. Thanks.
+ de = root_directory;
+ last_de = de;
+ while (de) {
+ if (need_root ||
+ match_pathspec_depth(adjusted_pathspec, de-pathname,
de-de_pathlen, 0, NULL)) {
+ if (read_entries(istate, de, entry_offset,
+mmap, mmap_size, nr,
+foffsetblock) 0)
+ return -1;
+ } else {
+ for (i = 0; i de-de_nsubtrees; i++) {
+ last_de-next = de-sub[i];
+ last_de = last_de-next;
+ }
+ }
+ de = de-next;
I'm missing something here. read_entries is a function that reads all
entries inside de including subdirectories and the first de is
root_directory, which makes it read the whole index in.
It does, except when the adjusted_pathspec doesn't match the
root_directory. In that case all the subdirectories of the
root_directory are added to a queue, which will then be iterated over
and tried to match with the adjusted_pathspec.
That's what I missed. Thanks.
--
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: Should git apply --check imply verbose?
On 13-08-20 03:54 PM, Steven Rostedt wrote: On Tue, 20 Aug 2013 12:45:03 -0700 Junio C Hamano gits...@pobox.com wrote: Steven Rostedt rost...@goodmis.org writes: I do not think it is necessarily a good idea to assume that people who are learning git apply know how GNU patch works. Linus told me that git apply was basically a replacement for patch. Why would you think it would not be a good idea to assume that people would not be familiar with how GNU patch works? The audience of Git these days are far more widely spread than the kernel circle. I am not opposed to _helping_ those who happen to know patch, but I was against a description that assumes readers know it, i.e. making it a requirement to know patch to understand apply. Patch is used by much more than just the kernel folks ;-) I've been using patch much longer than I've been doing kernel development. But I do agree that the description of -v, --verbose has a lot of room for improvement. Report progress to stderr. By default, only a message about the current patch being applied will be printed. This option will cause additional information to be reported. It is totally unclear what additional information is reported at all. In other words, your enhancement to the documentation could go like: ... By default, ... With this option, you will additionally see such and such and such in the output (this is similar to what patch --dry-run would give you). See the EXAMPLES section to get a feel of how it looks like. and I would not be opposed, as long as such and such and such are written in such a way that the reader does not have to have a prior experience with GNU patch in order to understand it. Clear? Looks good to me. Paul, what do you think? Yep, I'll write something up tomorrow which loosely matches the above. Thanks, Paul. -- Thanks, -- Steve -- 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 v3 23/24] introduce GIT_INDEX_VERSION environment variable
On Mon, Aug 19, 2013 at 2:42 AM, Thomas Gummerer t.gumme...@gmail.com wrote: Respect a GIT_INDEX_VERSION environment variable, when a new index is initialized. Setting the environment variable will not cause existing index files to be converted to another format for additional safety. Signed-off-by: Thomas Gummerer t.gumme...@gmail.com --- read-cache.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) There should be a line or two about this variable in git.txt, section Environment variables. We could even have core.defaultIndexVersion for people who don't want to set environment variables (and set this key in ~/.gitconfig instead) but this is not important now. -- 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: [GUILT] add FreeBSD support
On Tue, Aug 20, 2013 at 11:25:48AM -0400, Josef 'Jeff' Sipek wrote: On Tue, Aug 20, 2013 at 03:44:16PM +0800, Zheng Liu wrote: Hi Josef, On Fri, Aug 09, 2013 at 11:20:46AM -0400, Josef 'Jeff' Sipek wrote: On Fri, Aug 09, 2013 at 11:04:45PM +0800, gnehzuil.liu wrote: ?? 2013-8-9??10:46??Josef 'Jeff' Sipek jef...@josefsipek.net д On Fri, Aug 09, 2013 at 08:32:28PM +0800, Zheng Liu wrote: From: Zheng Liu gnehzuil@gmail.com Currently guilt doesn't support FreeBSD platform. This commit tries to add this support. The file called 'os.FreeBSD' is copied from os.Darwin due to these two platforms have almost the same command tools. Out of curiosity, is it identical? I eyeballed it, and they do look identical. There's probably a better way to do this whole os-specific thing, but this will work well enough for now. Yes, it is identical. Sorry, I am a newbie for guilt, but I am happy to improve this os-specific thing.Any idea? So, I'm a bit torn between some build-time checking that generates something like an os file based on what it detects and something that happens at runtime. I like that currently there's nothing to do - you just clone the repo and you're set. At the same time, the more code can be avoided executing the faster (in theory) guilt gets. Sorry for the late reply. I did a simple experiment that tries to fold all os.* files into one file and uses a if statement to export functions according to different platforms. But frankly I don't like this because it is not very clearly. So IMHO we'd better add a 'os.FreeBSD' file to support FreeBSD platform. Yeah, sounds like the simplest (at least for the moment). I'll commit it. Thanks. FWIW, the idea I was thinking about was to make make all figure out various parts of the system and construct an os file. Yes, I thought this. We can create a os.in file and generate a os file after running make command. But currently os.* looks like the simplest solution. Regards, - Zheng -- 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 0/2] git-config and large integers
On Tue, Aug 20, 2013 at 04:06:19PM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: I was playing with a hook for file size limits that wanted to store the limit in git-config. It turns out we don't do a very good job of big integers: $ git config foo.size 2g $ git config --int foo.size -2147483648 Oops. After this series, we properly notice the error: $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config and even better, provide a way to access large values: $ git config --ulong foo.size 2147483648 I may be missing something, but why do we even need a new option for the command that is known to always produce textual output? We could do all math with int64_t (for example) and then print the stringified representation. But then we would not be matching the same checks that git is doing internally. For example, on a 32-bit system, setting core.packedGitLimit to 4G will produce an error for git config --int core.packedgitlimit, as we cannot represent the size internally. We could do the conversion in such a way that we print the correct size, but it does not represent what happens when git pack-objects is run (you get the same error). As you said Oops, the first example that shows a string of digits prefixed by a minus sign for input 2g is buggy, and I think it is perfectly reasonable to fix it to show a stringified representation of 2*1024*1024*1024 when asked for --int. The negative value is a little bit of a sidetrack. For both git config --int and for internal use, we do not correctly range-check integer values. And that's why we print the negative value, when we should say this is a bogus out-of-range value. The latter is what we have always done for values outside of range, both internal and external, and it is only that our range check was bogus (and we fed negative crap to the code instead of complaining). That is fixed by the first patch. And that leads to the second patch. The --int option provides a range check of (typically) -2^32 to 2^32-1. But many of our values internally use a larger range. We can either drop that range check (which means we will let you inspect values with config that git internally will barf on, with no clue), or we need to add another option with a different range to retrieve those values. I chose to add another option because I think the range check has value. Does that explain the problem more fully? -Peff -- 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 0/2] git-config and large integers
On Tue, Aug 20, 2013 at 04:41:30PM -0700, Junio C Hamano wrote: If this applied on the writing side, I would understand it very much, i.e. $ git config --int32 foo.size 2g fatal: 2g is too large to be read as int32. It does, by the way. When you request a type on the writing side, we normalize (and complain in the same way as we do when reading). and as a complement it may make sense as a warning mechanism to also error out when existing value does not fit on the platform int, so your $ git config --int foo.size fatal: bad config value for 'foo.size' in .git/config might make sense (even though I'd suggest being more explicit than bad value in this case---the value specified will not fit when used in a variable of type int on this platform). Yes, the error message is terrible, and I think an extra patch on top to improve it is worth doing. But note that I am not introducing that error here at all. On 32-bit systems, we already correctly range-checked and produced that error. It is only on 64-bit systems that the range check was flat out wrong. It checked against long's precision, but then cast the result to an int, losing bits. A possibly worse example than the negative one is: $ git config foo.bar 4g $ git config --int foo.bar 0 Again, that is what git's internal code is seeing. And that is why keeping the range check for git-config has value: it lets you see what git would see internally. When .git/config is shared on two different boxes (think: NFS), the size of int might be different between them, so the logic to produce such a warning may have to explicitly check against int32_t, not platform int and say will not fit in 'int' on some machines. I don't really see the value in that. You can always write whatever you like in the config file. The reader is responsible during parsing for saying Hey, I am 32-bit and I can't handle this. And we already do that, and it works fine. So if you have an NFS-shared .git/config, and you set pack.deltacachesize to 4g, a 64-bit machine will do fine with that, and a 32-bit machine will complain. Which seems like the only sane thing to do. There are a few config options that use unsigned long that I would argue should be off_t or something (for example, core.bigFileThreshold, which cannot be more than 4G on a 32-bit machine, simply because we can't represent the size. On the other hand, there is probably a ton of stuff that does not work with 4G files on such a system, because we use unsigned long all over the place inside the code). -Peff -- 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 1/2] config: properly range-check integer values
On Tue, Aug 20, 2013 at 04:07:49PM -0700, Jonathan Nieder wrote:
I added a test. It would not fail on existing 32-bit systems, but would
on existing LP64 systems. It will pass with the new code on both.
However, it will fail on ILP64 systems (because their int is large, and
can represent 3GB). I'm not sure if any such systems are in wide use
(SPARC64?), but we would want a prereq in that case, I guess. I'm
inclined to wait to see if it actually fails for anybody.
Yuck.
What will go wrong if git config --int starts returning numbers too
large to fit in an 'int'? That can already happen if git and a
command that uses it are built for different ABIs (e.g., ILP64 git,
32-bit custom tool that calls git).
I'm not sure I understand your question. git config --int cannot
return numbers that do not fit in an int, since we use an int as an
intermediate representation type. The intent of the code is to
range-check the input and complain. But the code gets it wrong, and
sometimes truncates the value instead. So we continue to never show
anything that would not fit in an int, but now our range-check
correctly complains rather than truncating in some cases.
If you are worried about a 32-bit command calling an ILP64 git, then
nothing is made worse by this patch. We return the same range of values
as before.
Short of adding --int32, etc to git-config, I don't think git can be
responsible for this situation. It can only say This does not fit in my
internal representation, and I would barf on it. If you do not tell it
that your int is smaller, then it cannot say you would barf on it.
It's possible that what the test should be checking for is either
returns a sane answer or fails (which would pass regardless of ABI).
Something like:
test_expect_success 'large integers do not confuse config --int' '
git config giga.crash 3g
test_might_fail git config --int giga.crash actual
echo 3221225472 expect
{
test_cmp expect actual ||
test_must_fail git config --int giga.crash
}
'
Sensible?
Yes, that would cover an ILP64 system, though it very much muddies the
point of the test (which is to find a value that is represented by a
long, but not an int; such a value does not exist at all on ILP64).
Which is why I wondered about avoiding it with a prerequisite.
-Peff
--
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 2/2] teach git-config to output large integers
On Tue, Aug 20, 2013 at 03:57:45PM -0700, Jonathan Nieder wrote:
Jeff King wrote:
I kind of hate the name --ulong. I wanted to call it --size or
something and abstract away the actual platform representation, and just
make it big enough for file sizes.
Yes, something like --size would be more pleasant.
It could still use unsigned long internally. My only worry about
--size is that it does not make it clear we are talking about file
sizes and not in-memory sizes (size_t), and I'm not too worried about
that.
I almost sent it as --size with unsigned long internally. But try
writing the documentation for it. You want to say something like it's
big enough to handle file sizes. Except that on 32-bit, it's _not_.
It's only 4G.
You really want something that uses off_t internally, so 32-bit systems
with largefile support do the sane thing. But now you have no way of
emulating the way that git parses stuff internally. You cannot say git
config --size core.bigFileThreshold and get the same results that git
will have internally when it looks at that file (because it uses
unsigned long internally).
I think there is an argument to be made that git should be using off_t
internally for such things. But it is a lot of code to change and check,
and I'm not sure that anybody even really cares that much.
Style: uncuddled else, stray blank line. (The former was already
there, but it still stands out.) I think
Yes, I was trying to follow the existing style (but obviously the extra
line was just a typo).
if (types == TYPE_INT) {
...
} else if (types == TYPE_ULONG) {
...
} else if (types == TYPE_BOOL) {
...
} else if (types == TYPE_BOOL_OR_INT) {
...
} else {
...
}
would be easiest to read.
But that is adding brackets for one-liner conditional bodies that do not
need it. Which is more evil?
My usual method is to do what looks the most readable to me, but I admit
I have a hard time using my intuition with the cuddled-elses, as I think
they look terrible (yes, I'm aware they are in our style guide and I am
not arguing to take them out, only that my personal sense of looks
good is helpless with them).
-Peff
--
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 v3 02/24] read-cache: use fixed width integer types
Junio C Hamano gits...@pobox.com writes:
Thomas Gummerer t.gumme...@gmail.com writes:
Use the fixed width integer types uint16_t and uint32_t for ondisk
structures, because unsigned short and unsigned int do not hae a
guaranteed size.
This sounds like an independent fix to me. I'd queue this early
independent from the rest of the series.
Thanks.
Sounds good to me. Thanks.
Signed-off-by: Thomas Gummerer t.gumme...@gmail.com
---
cache.h | 10 +-
read-cache.c | 30 +++---
2 files changed, 20 insertions(+), 20 deletions(-)
diff --git a/cache.h b/cache.h
index bd6fb9f..9ef778a 100644
--- a/cache.h
+++ b/cache.h
@@ -101,9 +101,9 @@ unsigned long git_deflate_bound(git_zstream *, unsigned
long);
#define CACHE_SIGNATURE 0x44495243 /* DIRC */
struct cache_header {
-unsigned int hdr_signature;
-unsigned int hdr_version;
-unsigned int hdr_entries;
+uint32_t hdr_signature;
+uint32_t hdr_version;
+uint32_t hdr_entries;
};
#define INDEX_FORMAT_LB 2
@@ -115,8 +115,8 @@ struct cache_header {
* check it for equality in the 32 bits we save.
*/
struct cache_time {
-unsigned int sec;
-unsigned int nsec;
+uint32_t sec;
+uint32_t nsec;
};
struct stat_data {
diff --git a/read-cache.c b/read-cache.c
index ceaf207..0df5b31 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1230,14 +1230,14 @@ static struct cache_entry
*refresh_cache_entry(struct cache_entry *ce, int reall
struct ondisk_cache_entry {
struct cache_time ctime;
struct cache_time mtime;
-unsigned int dev;
-unsigned int ino;
-unsigned int mode;
-unsigned int uid;
-unsigned int gid;
-unsigned int size;
+uint32_t dev;
+uint32_t ino;
+uint32_t mode;
+uint32_t uid;
+uint32_t gid;
+uint32_t size;
unsigned char sha1[20];
-unsigned short flags;
+uint16_t flags;
char name[FLEX_ARRAY]; /* more */
};
@@ -1249,15 +1249,15 @@ struct ondisk_cache_entry {
struct ondisk_cache_entry_extended {
struct cache_time ctime;
struct cache_time mtime;
-unsigned int dev;
-unsigned int ino;
-unsigned int mode;
-unsigned int uid;
-unsigned int gid;
-unsigned int size;
+uint32_t dev;
+uint32_t ino;
+uint32_t mode;
+uint32_t uid;
+uint32_t gid;
+uint32_t size;
unsigned char sha1[20];
-unsigned short flags;
-unsigned short flags2;
+uint16_t flags;
+uint16_t flags2;
char name[FLEX_ARRAY]; /* more */
};
--
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 v3 04/24] read-cache: clear version in discard_index()
Junio C Hamano gits...@pobox.com writes: Thomas Gummerer t.gumme...@gmail.com writes: All fields except index_state-version are reset in discard_index. Reset the version too. What is the practical consequence of not clearing this field? I somehow have a feeling that this was done deliberately, so that we can stick to the version of the index file format better, once the user said update-index --index-version $N to set it up. I suspect that the patch would affect a codepath that does read_cache(), calls discard_index(), populates the index and then does write_cache(). We stick to the version the user specified earlier in our current code, while the patched code will revert to whatever default built into your Git binary, no? Yeah you're right, I missed that use-case. I'll drop this patch from the re-roll. Sorry for the noise. Signed-off-by: Thomas Gummerer t.gumme...@gmail.com --- read-cache.c | 1 + 1 file changed, 1 insertion(+) diff --git a/read-cache.c b/read-cache.c index de0bbcd..1e22f6f 100644 --- a/read-cache.c +++ b/read-cache.c @@ -1558,6 +1558,7 @@ int discard_index(struct index_state *istate) for (i = 0; i istate-cache_nr; i++) free(istate-cache[i]); resolve_undo_clear_index(istate); +istate-version = 0; istate-cache_nr = 0; istate-cache_changed = 0; istate-timestamp.sec = 0; -- 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] Add gui.displayuntracked option
When git is used to track only a subset of a directory, or
there is no sure way to divide files to ignore from files to track,
git user have to live with large number of untracked files. These files
present in file list, and should always be scrolled through
to handle real changes. Situation can become even worse, then number
of the untracked files grows above the maxfilesdisplayed limit. In the
case, even staged can be hidden by git-gui.
This change introduces new configuration variable gui.displayuntracked,
which, when set to false, instructs git-gui not to show untracked files
in files list. They can be staged from commandline or other tools (like
IDE of file manager), then they become visible. Default value of the
option is true, which is compatible with current behavior.
Signed-off-by: Max Kirillov m...@max630.net
---
Hi. I've been using git for some time and have collected a
number of changes which might worth sharing.
Please consider adding them to the upstream.
Thanks,
Max
Documentation/config.txt | 4
git-gui/git-gui.sh | 14 ++
git-gui/lib/option.tcl | 1 +
3 files changed, 15 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index bbba728..7a786b2 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1277,6 +1277,10 @@ gui.diffcontext::
Specifies how many context lines should be used in calls to diff
made by the linkgit:git-gui[1]. The default is 5.
+gui.displayuntracked::
+ Determines if linkgit::git-gui[1] shows untracked files
+ in the file list. The defaulit is true.
+
gui.encoding::
Specifies the default encoding to use for displaying of
file contents in linkgit:git-gui[1] and linkgit:gitk[1].
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 89f636f..42c35ad 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -898,6 +898,7 @@ set font_descs {
{fontdiff font_diff {mc Diff/Console Font}}
}
set default_config(gui.stageuntracked) ask
+set default_config(gui.displayuntracked) true
##
##
@@ -1536,18 +1537,23 @@ proc rescan_stage2 {fd after} {
set buf_rdf {}
set buf_rlo {}
- set rescan_active 3
+ set rescan_active 2
ui_status [mc Scanning for modified files ...]
set fd_di [git_read diff-index --cached -z [PARENT]]
set fd_df [git_read diff-files -z]
- set fd_lo [eval git_read ls-files --others -z $ls_others]
fconfigure $fd_di -blocking 0 -translation binary -encoding binary
fconfigure $fd_df -blocking 0 -translation binary -encoding binary
- fconfigure $fd_lo -blocking 0 -translation binary -encoding binary
+
fileevent $fd_di readable [list read_diff_index $fd_di $after]
fileevent $fd_df readable [list read_diff_files $fd_df $after]
- fileevent $fd_lo readable [list read_ls_others $fd_lo $after]
+
+ if {[is_config_true gui.displayuntracked]} {
+ set fd_lo [eval git_read ls-files --others -z $ls_others]
+ fconfigure $fd_lo -blocking 0 -translation binary -encoding
binary
+ fileevent $fd_lo readable [list read_ls_others $fd_lo $after]
+ incr rescan_active
+ }
}
proc load_message {file {encoding {}}} {
diff --git a/git-gui/lib/option.tcl b/git-gui/lib/option.tcl
index 0cf1da1..2177db6 100644
--- a/git-gui/lib/option.tcl
+++ b/git-gui/lib/option.tcl
@@ -159,6 +159,7 @@ proc do_options {} {
{c gui.encoding {mc Default File Contents Encoding}}
{b gui.warndetachedcommit {mc Warn before committing to a
detached head}}
{s gui.stageuntracked {mc Staging of untracked files} {list
yes no ask}}
+ {b gui.displayuntracked {mc Show untracked files}}
} {
set type [lindex $option 0]
set name [lindex $option 1]
--
1.8.4.rc3.902.g80a4b9e
--
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] git-gui: right half window is paned
For long descriptions it would be nice to be able to resize
the comment text field.
Signed-off-by: Max Kirillov m...@max630.net
---
git-gui/git-gui.sh | 16 +++-
1 file changed, 11 insertions(+), 5 deletions(-)
diff --git a/git-gui/git-gui.sh b/git-gui/git-gui.sh
index 89f636f..e2e710e 100755
--- a/git-gui/git-gui.sh
+++ b/git-gui/git-gui.sh
@@ -3196,13 +3196,19 @@ unset i
# -- Diff and Commit Area
#
-${NS}::frame .vpane.lower -height 300 -width 400
+${NS}::panedwindow .vpane.lower -orient vertical
${NS}::frame .vpane.lower.commarea
-${NS}::frame .vpane.lower.diff -relief sunken -borderwidth 1
-pack .vpane.lower.diff -fill both -expand 1
-pack .vpane.lower.commarea -side bottom -fill x
+${NS}::frame .vpane.lower.diff -relief sunken -borderwidth 1 -height 500
+.vpane.lower add .vpane.lower.diff
+.vpane.lower add .vpane.lower.commarea
.vpane add .vpane.lower
-if {!$use_ttk} {.vpane paneconfigure .vpane.lower -sticky nsew}
+if {$use_ttk} {
+ .vpane.lower pane .vpane.lower.diff -weight 1
+ .vpane.lower pane .vpane.lower.commarea -weight 0
+} else {
+ .vpane.lower paneconfigure .vpane.lower.diff -stretch always
+ .vpane.lower paneconfigure .vpane.lower.commarea -stretch never
+}
# -- Commit Area Buttons
#
--
1.8.4.rc3.902.g80a4b9e
--
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 v3 23/24] introduce GIT_INDEX_VERSION environment variable
Duy Nguyen pclo...@gmail.com writes: On Mon, Aug 19, 2013 at 2:42 AM, Thomas Gummerer t.gumme...@gmail.com wrote: Respect a GIT_INDEX_VERSION environment variable, when a new index is initialized. Setting the environment variable will not cause existing index files to be converted to another format for additional safety. Signed-off-by: Thomas Gummerer t.gumme...@gmail.com --- read-cache.c | 9 +++-- 1 file changed, 7 insertions(+), 2 deletions(-) There should be a line or two about this variable in git.txt, section Environment variables. We could even have core.defaultIndexVersion for people who don't want to set environment variables (and set this key in ~/.gitconfig instead) but this is not important now. Ok, I'll add it in git.txt. I agree, core.defaultIndexVersion can still be done in a follow-up patch, the environment variable is the important thing now because it's used for testing. Existing repositories have to be converted with git-update-index anyway. -- 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 2/2] teach git-config to output large integers
Jeff King wrote: I almost sent it as --size with unsigned long internally. But try writing the documentation for it. You want to say something like it's big enough to handle file sizes. Except that on 32-bit, it's _not_. It's only 4G. You really want something that uses off_t internally, so 32-bit systems with largefile support do the sane thing. But now you have no way of emulating the way that git parses stuff internally. Let's take a step back for a moment. What problem is this patch solving? From the motivating example, I thought it was When reading or writing an integer config item, git sometimes encounters integer overflow and doesn't know how to deal with it. Worse, this means that some meaningful values are unrepresentable in config files. Fix it in two steps: 1. Catch overflow, and error out instead of pretending to be able to handle it. 2. Provide at least an option to use a wider integer type and handle larger meaningful values. This involves a new option --size instead of making --int use intmax_t for the following compatibility reason: ... For example, the compatibility reason could be that some scripts calling git config were not able to handle large integers and that we do not want to expose them to unexpectedly large values. But that reason doesn't sound realistic to me. So what is the actual reason not to always use a wider range? That is what I was trying to get at in discussing the test. It is not We would like --int to reject values higher than this, but some platforms do not allow us to, but Either rejecting this value, or even better, computing the right size and printing it, is an acceptable behavior, and this test checks for those. Hoping that clarifies, Jonathan -- 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 2/2] teach git-config to output large integers
On Tue, Aug 20, 2013 at 09:38:41PM -0700, Jonathan Nieder wrote: Jeff King wrote: I almost sent it as --size with unsigned long internally. But try writing the documentation for it. You want to say something like it's big enough to handle file sizes. Except that on 32-bit, it's _not_. It's only 4G. You really want something that uses off_t internally, so 32-bit systems with largefile support do the sane thing. But now you have no way of emulating the way that git parses stuff internally. Let's take a step back for a moment. What problem is this patch solving? That you cannot currently ask git-config for a value larger than 2g. From the motivating example, I thought it was When reading or writing an integer config item, git sometimes encounters integer overflow and doesn't know how to deal with it. Worse, this means that some meaningful values are unrepresentable in config files. Fix it in two steps: 1. Catch overflow, and error out instead of pretending to be able to handle it. No, this first step is not being added. It is already the case that we error out for overflow. The first patch is catching a case where we failed to do that properly, and instead truncated. And it has nothing to do with git-config itself; it is only that we must use git-config to test it from the shell. The same problem may happen internally (but tends not to, because large things tend to use git_config_ulong instead of git_config_int). 2. Provide at least an option to use a wider integer type and handle larger meaningful values. This involves a new option --size instead of making --int use intmax_t for the following compatibility reason: ... For example, the compatibility reason could be that some scripts calling git config were not able to handle large integers and that we do not want to expose them to unexpectedly large values. But that reason doesn't sound realistic to me. So what is the actual reason not to always use a wider range? My reason is that it does not represent the same range checks that git is doing internally (and which vary from platform to platform). So you might ask git-config what is the value of pack.deltacachelimit; I would expect it to apply the same range checks there that would be used by git-pack-objects. That is what I was trying to get at in discussing the test. It is not We would like --int to reject values higher than this, but some platforms do not allow us to, but Either rejecting this value, or even better, computing the right size and printing it, is an acceptable behavior, and this test checks for those. You are conflating the two patches, I think. The test we were discussing is for the _first_ patch, which fixes a bug in the range check. It is not meant to test git-config in particular, but to test that values higher than INT_MAX and lower than LONG_MAX are properly range-checked. Forget the second patch for a moment. I believe the first one is a bug fix that we would want even if we do not take the second patch at all. -Peff -- 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
