[Patch] git-svn: support dcommit --preserve-merges
Need to update git-svn.perl to match Documentation/git-svn.txt so --preserve-merges|p can be used in dcommit. Similar to https://github.com/git/git/commit/b64e1f58158d1d1a8eafabbbf002a1a3c1d72929#diff-f9a64e34cbe6c3ee4f62698008a33773R571 Nate. Documentation/git-svn.txt 626 -m:: 627 --merge:: 628 -sstrategy:: 629 --strategy=strategy:: 630 -p:: 631 --preserve-merges:: 632 These are only used with the 'dcommit' and 'rebase' commands. diff --git a/git-svn.perl b/git-svn.perl index 0a32372..3f981f8 100755 --- a/git-svn.perl +++ b/git-svn.perl @@ -197,6 +197,7 @@ my %cmd = ( 'no-rebase' = \$_no_rebase, 'mergeinfo=s' = \$_merge_info, 'interactive|i' = \$_interactive, + 'preserve-merges|p' = \$_preserve_merges, %cmt_opts, %fc_opts } ], branch = [ \cmd_branch, 'Create a branch in the SVN repository', -- 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
[BUG REPORT] Git log pretty date
Hi, I'm having some problems with the `git log` command. Setup: Happens in multiple versions, tested on 1.8.* and 2.* The problem happens when I try to get a pretty log for a commit with a wrong date. Executing the command: `git log --encoding=UTF-8 --date=raw --pretty=format:'%H,%P,%ad,%cd,%an,%s'` I get an empty response on the date field, but since pretty has `%ad` it should follow the --date and return the date even if wrong. The problem happens in this repo: `https://github.com/SamWM/jQuery-Plugins` on commit `e9dddaf24c9de45d9b4efdf38eff7c30eb200f48`. I posted it on SO too: `http://stackoverflow.com/questions/23793188/find-date-for-not-dated-commit` I think it is a bug probably on pretty since other raw formats show the date as it should. I tried to check the source code but have no idea where to start, maybe if you point me on some direction I can take a look for my self. Can you help? Kind regards, Rodrigo Fernandes -- 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] Add an explicit GIT_DIR to the list of excludes
On Thu, May 29, 2014 at 4:33 AM, Pasha Bolokhov pasha.bolok...@gmail.com wrote: Agree, but partial here means... what? just a little doc? I'm sorry, I tried to be cute and failed :$ This is not official name for documentation files that are meant for inclusion and not as standalone manpages. I think they are similar to 'partial templates' http://guides.rubyonrails.org/layouts_and_rendering.html#using-partials - usually just called partials - hence my use of this name. Example: date-formats.txt included in git-commit(1), git-commit-tree(1) and git-tag(1). On Wed, May 28, 2014 at 11:53 AM, Jakub Narębski jna...@gmail.com wrote: W dniu 2014-05-27 19:16, Pasha Bolokhov pisze: [...] I suggest this. There appears to be a notion of standard excludes both in the code (dir.c) and in the man pages (git-grep, git-ls-files). However, it doesn't actually appear to be defined strictly speaking. So my suggestion is to define those standard excludes in one place (say gitignore(5)), and make other man pages (git-config, git-grip, git-ls-files) have references to that place instead of explaining every time in detail what is being excluded. Or define those standard excludes in standard-excludes.txt partial, and include said file from git-grep(1), git-ls-files(1), and perhaps gitignore(5). -- Jakub Narębski -- 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 0/2] mingw: macro main(), const warnings
Hello, mingw.h defines a preprocessor macro main(), so that it can wrap the original function and hoook into initialization. The real main() function can have different types of its second parameter (char**, const char**, char*[]). It is not easy to match the type and gcc issues a const warning. My patch fixes that. There were solutions for the same issue published ([1], [2]), but none of them appeared in junio/pu. This new solution should be more future proof, as it modifies only compat/mingw.h; the *.c files can have any of the types mentioned above. I promise to take care of the integration into msysGit if this patch gets accepted. To make it easier, I'm submitting a patch that has been part of msysGit for 3 years. Karsten Blees (1): Win32: move main macro to a function Stepan Kasal (1): mingw: avoid const warning compat/mingw.c | 15 +++ compat/mingw.h | 17 ++--- 2 files changed, 21 insertions(+), 11 deletions(-) -- 1.9.2.msysgit.0.496.g23aa553 [1] a hack to fix the warning, by Pat Thoyts, in msysGit since 1.8.5.2.msysgit.0 (Dec 2013): https://github.com/msysgit/git/commit/6949537a [2] more elgant fix: From: Marat Radchenko ma...@slonopotamus.org Date: Tue, 29 Apr 2014 13:12:02 +0400 http://article.gmane.org/gmane.comp.version-control.git/247535 -- 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] Win32: move main macro to a function
From: Karsten Blees bl...@dcon.de Date: Fri, 7 Jan 2011 19:47:23 +0100 The code in the MinGW main macro is getting more and more complex, move to a separate initialization function for readabiliy and extensibility. Signed-off-by: Karsten Blees bl...@dcon.de Signed-off-by: Erik Faye-Lund kusmab...@gmail.com Signed-off-by: Stepan Kasal ka...@ucw.cz --- compat/mingw.c | 15 +++ compat/mingw.h | 14 -- 2 files changed, 19 insertions(+), 10 deletions(-) diff --git a/compat/mingw.c b/compat/mingw.c index a0e13bc..c03bafa 100644 --- a/compat/mingw.c +++ b/compat/mingw.c @@ -1847,3 +1847,18 @@ int mingw_offset_1st_component(const char *path) return offset + is_dir_sep(path[offset]); } + +void mingw_startup() +{ + /* copy executable name to argv[0] */ + __argv[0] = xstrdup(_pgmptr); + + /* initialize critical section for waitpid pinfo_t list */ + InitializeCriticalSection(pinfo_cs); + + /* set up default file mode and file modes for stdin/out/err */ + _fmode = _O_BINARY; + _setmode(_fileno(stdin), _O_BINARY); + _setmode(_fileno(stdout), _O_BINARY); + _setmode(_fileno(stderr), _O_BINARY); +} diff --git a/compat/mingw.h b/compat/mingw.h index 3eaf822..15f0c9d 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -363,22 +363,16 @@ void free_environ(char **env); extern CRITICAL_SECTION pinfo_cs; /* - * A replacement of main() that ensures that argv[0] has a path - * and that default fmode and std(in|out|err) are in binary mode + * A replacement of main() that adds win32 specific initialization. */ +void mingw_startup(); #define main(c,v) dummy_decl_mingw_main(); \ static int mingw_main(c,v); \ int main(int argc, char **argv) \ { \ - extern CRITICAL_SECTION pinfo_cs; \ - _fmode = _O_BINARY; \ - _setmode(_fileno(stdin), _O_BINARY); \ - _setmode(_fileno(stdout), _O_BINARY); \ - _setmode(_fileno(stderr), _O_BINARY); \ - argv[0] = xstrdup(_pgmptr); \ - InitializeCriticalSection(pinfo_cs); \ - return mingw_main(argc, argv); \ + mingw_startup(); \ + return mingw_main(__argc, __argv); \ } \ static int mingw_main(c,v) -- 1.9.2.msysgit.0.496.g23aa553 -- 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 2/2] mingw: avoid const warning
Fix const warnings in http-fetch.c and remote-curl.c main() where is argv declared as const. The fix should work for all future declarations of main, no matter whether the second parameter's type is char**, const char**, or char *[]. Signed-off-by: Stepan Kasal ka...@ucw.cz --- compat/mingw.h | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/compat/mingw.h b/compat/mingw.h index 15f0c9d..8745d19 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -369,10 +369,11 @@ extern CRITICAL_SECTION pinfo_cs; void mingw_startup(); #define main(c,v) dummy_decl_mingw_main(); \ static int mingw_main(c,v); \ -int main(int argc, char **argv) \ +int main(c, char **main_argv_not_used) \ { \ + typedef v, **argv_type; \ mingw_startup(); \ - return mingw_main(__argc, __argv); \ + return mingw_main(__argc, (argv_type)__argv); \ } \ static int mingw_main(c,v) -- 1.9.2.msysgit.0.496.g23aa553 -- 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: [BUG REPORT] Git log pretty date
On Thu, May 29, 2014 at 5:29 PM, Rodrigo Fernandes rtfrodr...@gmail.com wrote: I get an empty response on the date field, but since pretty has `%ad` it should follow the --date and return the date even if wrong. ... I tried to check the source code but have no idea where to start, maybe if you point me on some direction I can take a look for my self. pretty.c, format_commit_one() - format_person_part() - show_ident_date() - show_date(). The last one is in date.c -- 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: [BUG REPORT] Git log pretty date
On do, 2014-05-29 at 11:29 +0100, Rodrigo Fernandes wrote: The problem happens when I try to get a pretty log for a commit with a wrong date. The commit is: === $ git cat-file commit e9dddaf24c9de45d9b4efdf38eff7c30eb200f48 tree d63aeb159635cb231e191505a95a129a3b4a7b38 parent 9276202f1c0dcc360433df222c90f7874558f072 author SamWM s...@webmonkeysolutions.com 1288370243 --700 committer SamWM s...@webmonkeysolutions.com 1288370243 --700 Update version number, make text formatting and indentation consistent with the rest of the code Those dates are indeed wrong, I'm not surprised git refuses to parse them. -- Dennis Kaarsemaker www.kaarsemaker.net -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: .gitmodules containing SSH clone URLs should fall back to HTTPS when SSH key is not valid/existent
Am 29.05.2014 04:07, schrieb Jonathan Leonard: The title pretty much says it all. But you do not give much information about your special use case. I assume you have submodule repositories for which some developers have a valid ssh key and others don't (maybe because they should only have read access via https)? If that is the case you might want to look into access control tools like gitolite. Lack of this feature (or presence of this bug [depending on your perspective]) is a major PITA. But why is https special? Why not fall back to the git protocol? Or http? (And no: I'm not serious here ;-) After the first failed clone of the submodule at via SSH the developer could also just do a git config submodule.name.url https://host/repo and override the URL from .gitmodules. But maybe I misunderstood why you want to have a fall back? -- 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] tests: turn off git-daemon tests if FIFOs are not available
Signed-off-by: Stepan Kasal ka...@ucw.cz --- Hi, mingw does not have FIFOs, so it cannot run git-daemon tests. Stepan t/lib-git-daemon.sh | 5 + 1 file changed, 5 insertions(+) diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh index bc4b341..9b1271c 100644 --- a/t/lib-git-daemon.sh +++ b/t/lib-git-daemon.sh @@ -23,6 +23,11 @@ then test_done fi +if ! test_have_prereq PIPE +then + test_skip_or_die $GIT_TEST_GIT_DAEMON file system does not support FIFOs +fi + LIB_GIT_DAEMON_PORT=${LIB_GIT_DAEMON_PORT-${this_test#t}} GIT_DAEMON_PID= -- 1.9.2.msysgit.0.493.g4becbf6.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: [PATCH v4] Add an explicit GIT_DIR to the list of excludes
On Thu, May 29, 2014 at 5:11 AM, Pasha Bolokhov pasha.bolok...@gmail.com wrote: + len = strlen(n_git); /* real_path() has stripped trailing slash */ + for (i = len - 1; i 0 !is_dir_sep(n_git[i]); i--) ; + basename = n_git + i; + if (is_dir_sep(*basename)) + basename++; + if (strcmp(basename, .git)) { I think normalize_path_copy makes sure that dir sep is '/', so this code may be simplified to if (strcmp(n_git, .git) (len == 4 || strcmp(n_git + len - 5, /.git)))? Then if n_git is /ab = coredump. But I agree there is logic to this (if we check len = 4 first). However, we still need the basename. Ah I missed this at add_exclude() So I've just shortened it a bit, what do you think: (notice the condition i = 0 btw) for (i = len - 1; i = 0 n_git[i] != '/'; i--) ; There's basename() that does this for you. A compat version is provided for Windows port so no portability worries. basename = n_git + i + 1; if (strcmp(basename, .git)) { + const char *worktree = get_git_work_tree(); + + if (worktree == NULL || + dir_inside_of(n_git, get_git_work_tree()) = 0) { + struct exclude_list *el = add_exclude_list(dir, EXC_CMDL, + GIT_DIR setup); + + /* append a trailing slash to exclude directories */ + n_git[len] = '/'; + n_git[len + 1] = '\0'; + add_exclude(basename, , 0, el, 0); Hmm.. I overlooked this bit before. So if $GIT_DIR is /something/foo, we set to ignore foo/. Because we know n_git must be part of (normalized) get_git_work_tree() at this point, could we path n_git + strlen(get_git_work_tree()) to add_exclude() instead of basename? Full path makes sure we don't accidentally exclude too much. The case when $GIT_DIR points to a _file_ seems uncovered. setup_git_directory() will transform the file to the directory internally and we never know the .git file's path (so we can't exclude it). So people could accidentally add the .git file in, then remove it from from work tree and suddenly the work tree becomes repo-less. It's not as bad as .git _directory_ because we don't lose valuable data. I don't know if you want to cover this too. -- 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] check_refname_component: Optimize
On Wed, May 28, 2014 at 03:57:35PM -0400, David Turner wrote: +ifdef NO_SSE + BASIC_CFLAGS += -DNO_SSE This is not a great name. This implies the system does not have SSE (SSE 1), but in fact what you're concerned about is SSE 4. This will be confusing for users of older x86-64 processors, where SSE and SSE 2 are architecturally guaranteed even though SSE 4 is not present. +else + BASIC_CFLAGS += -msse4 +endif You probably also want to autodetect when the system is not x86(-64)? and turn this off. I doubt PowerPC/SPARC/ARM users are going to want to have to disable it manually. -- brian m. carlson / brian with sandals: Houston, Texas, US +1 832 623 2791 | http://www.crustytoothpaste.net/~bmc | My opinion only OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187 signature.asc Description: Digital signature
Re: [BUG REPORT] Git log pretty date
Dennis I think that could be an improvement. Duy, can you point me where is the date print from normal `git log` or `git show` so I can compare? On Thu, May 29, 2014 at 12:07 PM, Dennis Kaarsemaker den...@kaarsemaker.net wrote: On do, 2014-05-29 at 11:29 +0100, Rodrigo Fernandes wrote: The problem happens when I try to get a pretty log for a commit with a wrong date. The commit is: === $ git cat-file commit e9dddaf24c9de45d9b4efdf38eff7c30eb200f48 tree d63aeb159635cb231e191505a95a129a3b4a7b38 parent 9276202f1c0dcc360433df222c90f7874558f072 author SamWM s...@webmonkeysolutions.com 1288370243 --700 committer SamWM s...@webmonkeysolutions.com 1288370243 --700 Update version number, make text formatting and indentation consistent with the rest of the code Those dates are indeed wrong, I'm not surprised git refuses to parse them. -- Dennis Kaarsemaker www.kaarsemaker.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 1/4] wrapper.c: introduce gentle xmallocz that does not die()
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- git-compat-util.h | 1 + wrapper.c | 68 ++- 2 files changed, 53 insertions(+), 16 deletions(-) diff --git a/git-compat-util.h b/git-compat-util.h index f6d3a46..f23e4e4 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -524,6 +524,7 @@ extern try_to_free_t set_try_to_free_routine(try_to_free_t); extern char *xstrdup(const char *str); extern void *xmalloc(size_t size); extern void *xmallocz(size_t size); +extern void *xmallocz_gentle(size_t size); extern void *xmemdupz(const void *data, size_t len); extern char *xstrndup(const char *str, size_t len); extern void *xrealloc(void *ptr, size_t size); diff --git a/wrapper.c b/wrapper.c index 0cc5636..7ab9a98 100644 --- a/wrapper.c +++ b/wrapper.c @@ -9,16 +9,23 @@ static void do_nothing(size_t size) static void (*try_to_free_routine)(size_t size) = do_nothing; -static void memory_limit_check(size_t size) +static int memory_limit_check(size_t size, int gentle) { static int limit = -1; if (limit == -1) { const char *env = getenv(GIT_ALLOC_LIMIT); limit = env ? atoi(env) * 1024 : 0; } - if (limit size limit) - die(attempting to allocate %PRIuMAX over limit %d, - (intmax_t)size, limit); + if (limit size limit) { + if (gentle) { + error(attempting to allocate %PRIuMAX over limit %d, + (intmax_t)size, limit); + return -1; + } else + die(attempting to allocate %PRIuMAX over limit %d, + (intmax_t)size, limit); + } + return 0; } try_to_free_t set_try_to_free_routine(try_to_free_t routine) @@ -42,11 +49,12 @@ char *xstrdup(const char *str) return ret; } -void *xmalloc(size_t size) +static void *do_xmalloc(size_t size, int gentle) { void *ret; - memory_limit_check(size); + if (memory_limit_check(size, gentle)) + return NULL; ret = malloc(size); if (!ret !size) ret = malloc(1); @@ -55,9 +63,16 @@ void *xmalloc(size_t size) ret = malloc(size); if (!ret !size) ret = malloc(1); - if (!ret) - die(Out of memory, malloc failed (tried to allocate %lu bytes), - (unsigned long)size); + if (!ret) { + if (!gentle) + die(Out of memory, malloc failed (tried to allocate %lu bytes), + (unsigned long)size); + else { + error(Out of memory, malloc failed (tried to allocate %lu bytes), + (unsigned long)size); + return NULL; + } + } } #ifdef XMALLOC_POISON memset(ret, 0xA5, size); @@ -65,16 +80,37 @@ void *xmalloc(size_t size) return ret; } -void *xmallocz(size_t size) +void *xmalloc(size_t size) +{ + return do_xmalloc(size, 0); +} + +static void *do_xmallocz(size_t size, int gentle) { void *ret; - if (unsigned_add_overflows(size, 1)) - die(Data too large to fit into virtual memory space.); - ret = xmalloc(size + 1); - ((char*)ret)[size] = 0; + if (unsigned_add_overflows(size, 1)) { + if (gentle) { + error(Data too large to fit into virtual memory space.); + return NULL; + } else + die(Data too large to fit into virtual memory space.); + } + ret = do_xmalloc(size + 1, gentle); + if (ret) + ((char*)ret)[size] = 0; return ret; } +void *xmallocz(size_t size) +{ + return do_xmallocz(size, 0); +} + +void *xmallocz_gentle(size_t size) +{ + return do_xmallocz(size, 1); +} + /* * xmemdupz() allocates (len + 1) bytes of memory, duplicates len bytes of * data to the allocated memory, zero terminates the allocated memory, @@ -96,7 +132,7 @@ void *xrealloc(void *ptr, size_t size) { void *ret; - memory_limit_check(size); + memory_limit_check(size, 0); ret = realloc(ptr, size); if (!ret !size) ret = realloc(ptr, 1); @@ -115,7 +151,7 @@ void *xcalloc(size_t nmemb, size_t size) { void *ret; - memory_limit_check(size * nmemb); + memory_limit_check(size * nmemb, 0); ret = calloc(nmemb, size); if (!ret (!nmemb || !size)) ret = calloc(1, 1); -- 1.9.1.346.ga2b5940 -- 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
[PATCH 4/4] diff: mark any file larger than core.bigfilethreshold binary
Too large files may lead to failure to allocate memory. If it happens here, it could impact quite a few commands that involve diff. Moreover, too large files are inefficient to compare anyway (and most likely non-text), so mark them binary and skip looking at their content. Noticed-by: Dale R. Worley wor...@alum.mit.edu Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- diff.c | 26 ++ diffcore.h | 1 + t/t1050-large.sh | 4 3 files changed, 23 insertions(+), 8 deletions(-) diff --git a/diff.c b/diff.c index 54281cb..0a2f865 100644 --- a/diff.c +++ b/diff.c @@ -2185,8 +2185,8 @@ int diff_filespec_is_binary(struct diff_filespec *one) one-is_binary = one-driver-binary; else { if (!one-data DIFF_FILE_VALID(one)) - diff_populate_filespec(one, 0); - if (one-data) + diff_populate_filespec(one, DIFF_POPULATE_IS_BINARY); + if (one-is_binary == -1 one-data) one-is_binary = buffer_is_binary(one-data, one-size); if (one-is_binary == -1) @@ -2721,6 +2721,11 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags) } if (size_only) return 0; + if ((flags DIFF_POPULATE_IS_BINARY) + s-size big_file_threshold s-is_binary == -1) { + s-is_binary = 1; + return 0; + } fd = open(s-path, O_RDONLY); if (fd 0) goto err_empty; @@ -2742,16 +2747,21 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags) } else { enum object_type type; - if (size_only) { + if (size_only || (flags DIFF_POPULATE_IS_BINARY)) { type = sha1_object_info(s-sha1, s-size); if (type 0) die(unable to read %s, sha1_to_hex(s-sha1)); - } else { - s-data = read_sha1_file(s-sha1, type, s-size); - if (!s-data) - die(unable to read %s, sha1_to_hex(s-sha1)); - s-should_free = 1; + if (size_only) + return 0; + if (s-size big_file_threshold s-is_binary == -1) { + s-is_binary = 1; + return 0; + } } + s-data = read_sha1_file(s-sha1, type, s-size); + if (!s-data) + die(unable to read %s, sha1_to_hex(s-sha1)); + s-should_free = 1; } return 0; } diff --git a/diffcore.h b/diffcore.h index a186d7c..e7760d9 100644 --- a/diffcore.h +++ b/diffcore.h @@ -56,6 +56,7 @@ extern void fill_filespec(struct diff_filespec *, const unsigned char *, int, unsigned short); #define DIFF_POPULATE_SIZE_ONLY 1 +#define DIFF_POPULATE_IS_BINARY 2 extern int diff_populate_filespec(struct diff_filespec *, unsigned int); extern void diff_free_filespec_data(struct diff_filespec *); extern void diff_free_filespec_blob(struct diff_filespec *); diff --git a/t/t1050-large.sh b/t/t1050-large.sh index 333909b..4d922e2 100755 --- a/t/t1050-large.sh +++ b/t/t1050-large.sh @@ -112,6 +112,10 @@ test_expect_success 'diff --raw' ' git diff --raw HEAD^ ' +test_expect_success 'diff --stat' ' + git diff --stat HEAD^ HEAD +' + test_expect_success 'hash-object' ' git hash-object large1 ' -- 1.9.1.346.ga2b5940 -- 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 3/4] diff.c: allow to pass more flags to diff_populate_filespec
Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- diff.c| 13 +++-- diffcore-rename.c | 6 -- diffcore.h| 3 ++- 3 files changed, 13 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index f72769a..54281cb 100644 --- a/diff.c +++ b/diff.c @@ -373,7 +373,7 @@ static unsigned long diff_filespec_size(struct diff_filespec *one) { if (!DIFF_FILE_VALID(one)) return 0; - diff_populate_filespec(one, 1); + diff_populate_filespec(one, DIFF_POPULATE_SIZE_ONLY); return one-size; } @@ -1907,11 +1907,11 @@ static void show_dirstat(struct diff_options *options) diff_free_filespec_data(p-one); diff_free_filespec_data(p-two); } else if (DIFF_FILE_VALID(p-one)) { - diff_populate_filespec(p-one, 1); + diff_populate_filespec(p-one, DIFF_POPULATE_SIZE_ONLY); copied = added = 0; diff_free_filespec_data(p-one); } else if (DIFF_FILE_VALID(p-two)) { - diff_populate_filespec(p-two, 1); + diff_populate_filespec(p-two, DIFF_POPULATE_SIZE_ONLY); copied = 0; added = p-two-size; diff_free_filespec_data(p-two); @@ -2664,8 +2664,9 @@ static int diff_populate_gitlink(struct diff_filespec *s, int size_only) * grab the data for the blob (or file) for our own in-core comparison. * diff_filespec has data and size fields for this purpose. */ -int diff_populate_filespec(struct diff_filespec *s, int size_only) +int diff_populate_filespec(struct diff_filespec *s, unsigned int flags) { + int size_only = flags DIFF_POPULATE_SIZE_ONLY; int err = 0; /* * demote FAIL to WARN to allow inspecting the situation @@ -4695,8 +4696,8 @@ static int diff_filespec_check_stat_unmatch(struct diff_filepair *p) !DIFF_FILE_VALID(p-two) || (p-one-sha1_valid p-two-sha1_valid) || (p-one-mode != p-two-mode) || - diff_populate_filespec(p-one, 1) || - diff_populate_filespec(p-two, 1) || + diff_populate_filespec(p-one, DIFF_POPULATE_SIZE_ONLY) || + diff_populate_filespec(p-two, DIFF_POPULATE_SIZE_ONLY) || (p-one-size != p-two-size) || !diff_filespec_is_identical(p-one, p-two)) /* (2) */ p-skip_stat_unmatch_result = 1; diff --git a/diffcore-rename.c b/diffcore-rename.c index 749a35d..8437917 100644 --- a/diffcore-rename.c +++ b/diffcore-rename.c @@ -147,9 +147,11 @@ static int estimate_similarity(struct diff_filespec *src, * is a possible size - we really should have a flag to * say whether the size is valid or not!) */ - if (!src-cnt_data diff_populate_filespec(src, 1)) + if (!src-cnt_data + diff_populate_filespec(src, DIFF_POPULATE_SIZE_ONLY)) return 0; - if (!dst-cnt_data diff_populate_filespec(dst, 1)) + if (!dst-cnt_data + diff_populate_filespec(dst, DIFF_POPULATE_SIZE_ONLY)) return 0; max_size = ((src-size dst-size) ? src-size : dst-size); diff --git a/diffcore.h b/diffcore.h index c876dac..a186d7c 100644 --- a/diffcore.h +++ b/diffcore.h @@ -55,7 +55,8 @@ extern void free_filespec(struct diff_filespec *); extern void fill_filespec(struct diff_filespec *, const unsigned char *, int, unsigned short); -extern int diff_populate_filespec(struct diff_filespec *, int); +#define DIFF_POPULATE_SIZE_ONLY 1 +extern int diff_populate_filespec(struct diff_filespec *, unsigned int); extern void diff_free_filespec_data(struct diff_filespec *); extern void diff_free_filespec_blob(struct diff_filespec *); extern int diff_filespec_is_binary(struct diff_filespec *); -- 1.9.1.346.ga2b5940 -- 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: [BUG REPORT] Git log pretty date
On Thu, May 29, 2014 at 7:24 PM, Rodrigo Fernandes rtfrodr...@gmail.com wrote: Duy, can you point me where is the date print from normal `git log` or `git show` so I can compare? It's the same function, show_date() in date.c. -- 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] tests: turn off git-daemon tests if FIFOs are not available
On Thu, May 29, 2014 at 01:36:14PM +0200, Stepan Kasal wrote: Signed-off-by: Stepan Kasal ka...@ucw.cz --- Hi, mingw does not have FIFOs, so it cannot run git-daemon tests. Thanks. I took a peek at the mkfifo call here. It is used to make sure the daemon has started before we run the tests which depend on it. I suspect we could do something with a regular pipe, but the code would be rather tricky. Simply skipping the daemon tests is a reasonable compromise until somebody feels like trying to make it work without a fifo. -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: git-multimail: migration: Config is not iterable
On Thu, May 29, 2014 at 7:22 AM, Azat Khuzhin a3at.m...@gmail.com wrote: Hi there, Using the latest version of git-multimail there is an issue with migration: $ ~azat/git-multimail/git-multimail/migrate-mailhook-config --overwrite Traceback (most recent call last): ... File /home/azat/git-multimail/git-multimail/migrate-mailhook-config, line 66, in _check_old_config_exists if name in old: TypeError: argument of type 'Config' is not iterable Thanks for the report. I'm not the strongest on python data model and its special methods but I believe I've got a fix: https://github.com/mhagger/git-multimail/pull/58 -- 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 v12 00/44] Use ref transactions for all ref updates
This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions Ronnie please review these remaining patches in this series. Sahlberg (44): refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: add an err argument to repack_without_refs refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: add an err argument to delete_ref_loose refs.c: make update_ref_write update a strbuf on failure update-ref.c: log transaction error from the update_ref refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status refs.c: change ref_transaction_create to do error checking and return status refs.c: ref_transaction_delete to check for error and return status tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction refs.c: free the transaction before returning when number of updates is 0 refs.c: ref_transaction_commit should not free the transaction fetch.c: clear errno before calling functions that might set it fetch.c: change s_update_ref to use a ref transaction fetch.c: use a single ref transaction for all ref updates receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make write_ref_sha1 static refs.c: make lock_ref_sha1 static refs.c: add transaction.status and track OPEN/CLOSED/ERROR refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction refs.c: pass the ref log message to _create/delete/update instead of _commit refs.c: pass NULL as *flags to read_ref_full refs.c: pack all refs before we start to rename a ref refs.c: move the check for valid refname to lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: add a new flag for transaction delete for refs we know are packed only refs.c: pass a skip list to name_conflict_fn refs.c: make rename_ref use a transaction refs.c: remove forward declaration of write_ref_sha1 branch.c | 30 ++-- builtin/commit.c | 24 ++- builtin/fetch.c| 29 +-- builtin/receive-pack.c | 21 +-- builtin/replace.c | 15 +- builtin/tag.c | 15 +- builtin/update-ref.c | 32 ++-- cache.h| 2 + fast-import.c | 42 +++-- lockfile.c | 21 ++- refs.c | 468 + refs.h | 54 +++--- sequencer.c| 24 ++- t/t3200-branch.sh | 2 +- walker.c | 51 +++--- 15 files changed, 491 insertions(+), 339 deletions(-) -- 2.0.0.rc3.510.g20c254b -- 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 v12 25/41] fast-import.c: use a ref transaction when dumping tags
Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- fast-import.c | 29 +++-- 1 file changed, 23 insertions(+), 6 deletions(-) diff --git a/fast-import.c b/fast-import.c index 4a7b196..587ef4a 100644 --- a/fast-import.c +++ b/fast-import.c @@ -1735,15 +1735,32 @@ static void dump_tags(void) { static const char *msg = fast-import; struct tag *t; - struct ref_lock *lock; - char ref_name[PATH_MAX]; + struct strbuf ref_name = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction; + transaction = ref_transaction_begin(err); + if (!transaction) { + failure |= error(%s, err.buf); + goto cleanup; + } for (t = first_tag; t; t = t-next_tag) { - sprintf(ref_name, tags/%s, t-name); - lock = lock_ref_sha1(ref_name, NULL); - if (!lock || write_ref_sha1(lock, t-sha1, msg) 0) - failure |= error(Unable to update %s, ref_name); + strbuf_reset(ref_name); + strbuf_addf(ref_name, refs/tags/%s, t-name); + + if (ref_transaction_update(transaction, ref_name.buf, t-sha1, + NULL, 0, 0, err)) { + failure |= error(%s, err.buf); + goto cleanup; + } } + if (ref_transaction_commit(transaction, msg, err)) + failure |= error(%s, err.buf); + + cleanup: + ref_transaction_free(transaction); + strbuf_release(ref_name); + strbuf_release(err); } static void dump_marks_helper(FILE *f, -- 2.0.0.rc3.474.g3833130 -- 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 v12 40/41] fetch.c: change s_update_ref to use a ref transaction
Change s_update_ref to use a ref transaction for the ref update. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/fetch.c | 21 + 1 file changed, 13 insertions(+), 8 deletions(-) diff --git a/builtin/fetch.c b/builtin/fetch.c index faa1233..b13e8f9 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -375,22 +375,27 @@ static int s_update_ref(const char *action, { char msg[1024]; char *rla = getenv(GIT_REFLOG_ACTION); - static struct ref_lock *lock; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; if (dry_run) return 0; if (!rla) rla = default_rla.buf; snprintf(msg, sizeof(msg), %s: %s, rla, action); - lock = lock_any_ref_for_update(ref-name, - check_old ? ref-old_sha1 : NULL, - 0, NULL); - if (!lock) - return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT : - STORE_REF_ERROR_OTHER; - if (write_ref_sha1(lock, ref-new_sha1, msg) 0) + + transaction = ref_transaction_begin(err); + if (!transaction || + ref_transaction_update(transaction, ref-name, ref-new_sha1, + ref-old_sha1, 0, check_old, msg, err) || + ref_transaction_commit(transaction, err)) { + ref_transaction_free(transaction); + error(%s, err.buf); + strbuf_release(err); return errno == ENOTDIR ? STORE_REF_ERROR_DF_CONFLICT : STORE_REF_ERROR_OTHER; + } + ref_transaction_free(transaction); return 0; } -- 2.0.0.rc3.474.g3833130 -- 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 v12 06/41] refs.c: add an err argument to repack_without_refs
Update repack_without_refs to take an err argument and update it if there is a failure. Pass the err variable from ref_transaction_commit to this function so that callers can print a meaningful error message if _commit fails due to a problem in repack_without_refs. Add a new function unable_to_lock_message that takes a strbuf argument and fills in the reason for the failure. In commit_packed_refs, make sure that we propagate any errno that commit_lock_file might have set back to our caller. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- cache.h| 2 ++ lockfile.c | 21 - refs.c | 25 +++-- 3 files changed, 33 insertions(+), 15 deletions(-) diff --git a/cache.h b/cache.h index 8c6cdc2..5858da8 100644 --- a/cache.h +++ b/cache.h @@ -559,6 +559,8 @@ struct lock_file { #define LOCK_DIE_ON_ERROR 1 #define LOCK_NODEREF 2 extern int unable_to_lock_error(const char *path, int err); +extern void unable_to_lock_message(const char *path, int err, + struct strbuf *buf); extern NORETURN void unable_to_lock_index_die(const char *path, int err); extern int hold_lock_file_for_update(struct lock_file *, const char *path, int); extern int hold_lock_file_for_append(struct lock_file *, const char *path, int); diff --git a/lockfile.c b/lockfile.c index 8fbcb6a..92e0397 100644 --- a/lockfile.c +++ b/lockfile.c @@ -157,33 +157,36 @@ static int lock_file(struct lock_file *lk, const char *path, int flags) return lk-fd; } -static char *unable_to_lock_message(const char *path, int err) +void unable_to_lock_message(const char *path, int err, struct strbuf *buf) { - struct strbuf buf = STRBUF_INIT; if (err == EEXIST) { - strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n + strbuf_addf(buf, Unable to create '%s.lock': %s.\n\n If no other git process is currently running, this probably means a\n git process crashed in this repository earlier. Make sure no other git\n process is running and remove the file manually to continue., absolute_path(path), strerror(err)); } else - strbuf_addf(buf, Unable to create '%s.lock': %s, + strbuf_addf(buf, Unable to create '%s.lock': %s, absolute_path(path), strerror(err)); - return strbuf_detach(buf, NULL); } int unable_to_lock_error(const char *path, int err) { - char *msg = unable_to_lock_message(path, err); - error(%s, msg); - free(msg); + struct strbuf buf = STRBUF_INIT; + + unable_to_lock_message(path, err, buf); + error(%s, buf.buf); + strbuf_release(buf); return -1; } NORETURN void unable_to_lock_index_die(const char *path, int err) { - die(%s, unable_to_lock_message(path, err)); + struct strbuf buf = STRBUF_INIT; + + unable_to_lock_message(path, err, buf); + die(%s, buf.buf); } int hold_lock_file_for_update(struct lock_file *lk, const char *path, int flags) diff --git a/refs.c b/refs.c index 25c3a93..6e28841 100644 --- a/refs.c +++ b/refs.c @@ -2208,6 +2208,7 @@ int commit_packed_refs(void) struct packed_ref_cache *packed_ref_cache = get_packed_ref_cache(ref_cache); int error = 0; + int save_errno = 0; if (!packed_ref_cache-lock) die(internal error: packed-refs not locked); @@ -2217,10 +2218,13 @@ int commit_packed_refs(void) do_for_each_entry_in_dir(get_packed_ref_dir(packed_ref_cache), 0, write_packed_entry_fn, packed_ref_cache-lock-fd); - if (commit_lock_file(packed_ref_cache-lock)) + if (commit_lock_file(packed_ref_cache-lock)) { + save_errno = errno; error = -1; + } packed_ref_cache-lock = NULL; release_packed_ref_cache(packed_ref_cache); + errno = save_errno; return error; } @@ -2427,12 +2431,12 @@ static int curate_packed_ref_fn(struct ref_entry *entry, void *cb_data) return 0; } -static int repack_without_refs(const char **refnames, int n) +static int repack_without_refs(const char **refnames, int n, struct strbuf *err) { struct ref_dir *packed; struct string_list refs_to_delete = STRING_LIST_INIT_DUP; struct string_list_item *ref_to_delete; - int i, removed = 0; + int i, ret, removed = 0; /* Look for a packed ref */ for (i = 0; i n; i++) @@ -2444,6 +2448,11 @@ static int repack_without_refs(const char **refnames, int n) return 0; /* no refname exists in packed refs */ if (lock_packed_refs(0)) { + if (err) { + unable_to_lock_message(git_path(packed-refs), errno, + err); +
[PATCH v12 08/41] refs.c: add an err argument to delete_ref_loose
Add an err argument to delete_loose_ref so that we can pass a descriptive error string back to the caller. Pass the err argument from transaction commit to this function so that transaction users will have a nice error string if the transaction failed due to delete_loose_ref. Add a new function unlink_or_err that we can call from delete_ref_loose. This function is similar to unlink_or_warn except that we can pass it an err argument. If err is non-NULL the function will populate err instead of printing a warning(). Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 35 +-- 1 file changed, 29 insertions(+), 6 deletions(-) diff --git a/refs.c b/refs.c index 9d079ec..d768a5e 100644 --- a/refs.c +++ b/refs.c @@ -2491,16 +2491,38 @@ static int repack_without_ref(const char *refname) return repack_without_refs(refname, 1, NULL); } -static int delete_ref_loose(struct ref_lock *lock, int flag) +static int add_err_if_unremovable(const char *op, const char *file, + struct strbuf *e, int rc) +{ + int err = errno; + if (rc 0 errno != ENOENT) { + strbuf_addf(e, unable to %s %s: %s, + op, file, strerror(errno)); + errno = err; + return -1; + } + return 0; +} + +static int unlink_or_err(const char *file, struct strbuf *err) +{ + if (err) + return add_err_if_unremovable(unlink, file, err, + unlink(file)); + else + return unlink_or_warn(file); +} + +static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err) { if (!(flag REF_ISPACKED) || flag REF_ISSYMREF) { /* loose */ - int err, i = strlen(lock-lk-filename) - 5; /* .lock */ + int res, i = strlen(lock-lk-filename) - 5; /* .lock */ lock-lk-filename[i] = 0; - err = unlink_or_warn(lock-lk-filename); + res = unlink_or_err(lock-lk-filename, err); lock-lk-filename[i] = '.'; - if (err errno != ENOENT) + if (res errno != ENOENT) return 1; } return 0; @@ -2514,7 +2536,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt) lock = lock_ref_sha1_basic(refname, sha1, delopt, flag); if (!lock) return 1; - ret |= delete_ref_loose(lock, flag); + ret |= delete_ref_loose(lock, flag, NULL); /* removing the loose one could have resurrected an earlier * packed one. Also, if it was not loose we need to repack @@ -3494,7 +3516,8 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (update-lock) { delnames[delnum++] = update-lock-ref_name; - ret |= delete_ref_loose(update-lock, update-type); + ret |= delete_ref_loose(update-lock, update-type, + err); } } -- 2.0.0.rc3.474.g3833130 -- 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 v12 33/41] refs.c: pass the ref log message to _create/delete/update instead of _commit
Change the reference transactions so that we pass the reflog message through to the create/delete/update function instead of the commit message. This allows for individual messages for each change in a multi ref transaction. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- branch.c | 4 ++-- builtin/commit.c | 4 ++-- builtin/fetch.c| 3 +-- builtin/receive-pack.c | 7 --- builtin/replace.c | 4 ++-- builtin/tag.c | 4 ++-- builtin/update-ref.c | 13 +++-- fast-import.c | 8 refs.c | 34 +- refs.h | 8 sequencer.c| 4 ++-- walker.c | 5 ++--- 12 files changed, 53 insertions(+), 45 deletions(-) diff --git a/branch.c b/branch.c index c1eae00..e0439af 100644 --- a/branch.c +++ b/branch.c @@ -301,8 +301,8 @@ void create_branch(const char *head, transaction = ref_transaction_begin(err); if (!transaction || ref_transaction_update(transaction, ref.buf, sha1, - null_sha1, 0, !forcing, err) || - ref_transaction_commit(transaction, msg, err)) + null_sha1, 0, !forcing, msg, err) || + ref_transaction_commit(transaction, err)) die(%s, err.buf); ref_transaction_free(transaction); } diff --git a/builtin/commit.c b/builtin/commit.c index 7db194f..608296c 100644 --- a/builtin/commit.c +++ b/builtin/commit.c @@ -1721,8 +1721,8 @@ int cmd_commit(int argc, const char **argv, const char *prefix) ref_transaction_update(transaction, HEAD, sha1, current_head ? current_head-object.sha1 : NULL, - 0, !!current_head, err) || - ref_transaction_commit(transaction, sb.buf, err)) { + 0, !!current_head, sb.buf, err) || + ref_transaction_commit(transaction, err)) { rollback_index_files(); die(%s, err.buf); } diff --git a/builtin/fetch.c b/builtin/fetch.c index 55f457c..faa1233 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -673,10 +673,9 @@ static int store_updated_refs(const char *raw_url, const char *remote_name, } } } - if (rc STORE_REF_ERROR_DF_CONFLICT) error(_(some local refs could not be updated; try running\n - 'git remote prune %s' to remove any old, conflicting + 'git remote prune %s' to remove any old, conflicting branches), remote_name); abort: diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index 13f4a63..5653fa2 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -586,10 +586,11 @@ static const char *update(struct command *cmd, struct shallow_info *si) transaction = ref_transaction_begin(err); if (!transaction || ref_transaction_update(transaction, namespaced_name, - new_sha1, old_sha1, 0, 1, err) || - ref_transaction_commit(transaction, push, err)) { - + new_sha1, old_sha1, 0, 1, push, + err) || + ref_transaction_commit(transaction, err)) { const char *str; + string_list_append(error_strings, err.buf); str = error_strings.items[error_strings.nr - 1].string; strbuf_release(err); diff --git a/builtin/replace.c b/builtin/replace.c index f24d814..09bfd40 100644 --- a/builtin/replace.c +++ b/builtin/replace.c @@ -160,8 +160,8 @@ static int replace_object_sha1(const char *object_ref, transaction = ref_transaction_begin(err); if (!transaction || ref_transaction_update(transaction, ref, repl, prev, - 0, !is_null_sha1(prev), err) || - ref_transaction_commit(transaction, NULL, err)) + 0, !is_null_sha1(prev), NULL, err) || + ref_transaction_commit(transaction, err)) die(%s, err.buf); ref_transaction_free(transaction); diff --git a/builtin/tag.c b/builtin/tag.c index c9bfc9a..74af63e 100644 --- a/builtin/tag.c +++ b/builtin/tag.c @@ -705,8 +705,8 @@ int cmd_tag(int argc, const char **argv, const char *prefix) transaction = ref_transaction_begin(err); if (!transaction || ref_transaction_update(transaction, ref.buf, object, prev, - 0, !is_null_sha1(prev), err) || - ref_transaction_commit(transaction, NULL, err)) +
[PATCH v12 31/41] refs.c: make prune_ref use a transaction to delete the ref
Change prune_ref to delete the ref using a ref transaction. To do this we also need to add a new flag REF_ISPRUNING that will tell the transaction that we do not want to delete this ref from the packed refs. This flag is private to refs.c and not exposed to external callers. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 28 +--- refs.h | 11 ++- 2 files changed, 31 insertions(+), 8 deletions(-) diff --git a/refs.c b/refs.c index 7cea694..60593d7 100644 --- a/refs.c +++ b/refs.c @@ -30,6 +30,12 @@ static inline int bad_ref_char(int ch) } /* + * Used as a flag to ref_transaction_delete when a loose ref is being + * pruned. + */ +#define REF_ISPRUNING 0x0100 + +/* * Try to read one refname component from the front of refname. Return * the length of the component found, or -1 if the component is not * legal. @@ -2328,17 +2334,24 @@ static void try_remove_empty_parents(char *name) /* make sure nobody touched the ref, and unlink */ static void prune_ref(struct ref_to_prune *r) { - struct ref_lock *lock; + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; if (check_refname_format(r-name + 5, 0)) return; - lock = lock_ref_sha1_basic(r-name, r-sha1, 0, NULL); - if (lock) { - unlink_or_warn(git_path(%s, r-name)); - unlock_ref(lock); - try_remove_empty_parents(r-name); + transaction = ref_transaction_begin(err); + if (!transaction || + ref_transaction_delete(transaction, r-name, r-sha1, + REF_ISPRUNING, 1, err) || + ref_transaction_commit(transaction, NULL, err)) { + ref_transaction_free(transaction); + error(%s, err.buf); + strbuf_release(err); + return; } + ref_transaction_free(transaction); + try_remove_empty_parents(r-name); } static void prune_refs(struct ref_to_prune *r) @@ -3536,9 +3549,10 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct ref_update *update = updates[i]; if (update-lock) { - delnames[delnum++] = update-lock-ref_name; ret |= delete_ref_loose(update-lock, update-type, err); + if (!(update-flags REF_ISPRUNING)) + delnames[delnum++] = update-lock-ref_name; } } diff --git a/refs.h b/refs.h index c38ee09..dee7c8f 100644 --- a/refs.h +++ b/refs.h @@ -171,8 +171,17 @@ extern int ref_exists(const char *); */ extern int peel_ref(const char *refname, unsigned char *sha1); -/** Locks any ref (for 'HEAD' type refs). */ +/* + * Flags controlling lock_any_ref_for_update(), ref_transaction_update(), + * ref_transaction_create(), etc. + * REF_NODEREF: act on the ref directly, instead of dereferencing + * symbolic references. + * + * Flags = 0x100 are reserved for internal use. + */ #define REF_NODEREF0x01 + +/** Locks any ref (for 'HEAD' type refs). */ extern struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p); -- 2.0.0.rc3.474.g3833130 -- 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 v12 16/41] refs.c: add transaction.status and track OPEN/CLOSED/ERROR
Track the status of a transaction in a new status field. Check the field for sanity, i.e. that status must be OPEN when _commit/_create/_delete or _update is called or else die(BUG:...) Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 36 +++- 1 file changed, 35 insertions(+), 1 deletion(-) diff --git a/refs.c b/refs.c index b5186cd..00d8fc9 100644 --- a/refs.c +++ b/refs.c @@ -3331,6 +3331,30 @@ struct ref_update { }; /* + * Transaction states. + * OPEN: The transaction is in a valid state and can accept new updates. + * An OPEN transaction can be committed. + * CLOSED: If an open transaction is successfully committed the state will + * change to CLOSED. No further changes can be made to a CLOSED + * transaction. + * CLOSED means that all updates have been successfully committed and + * the only thing that remains is to free the completed transaction. + * ERROR: The transaction has failed and is no longer committable. + * Eventhough the transaction can no longer be committed it is still + * possible to add additional updates to the transaction. The reason + * for this is to allow a caller to continue trying more updates + * so that a caller can report a list of ALL updates that would fail + * instead of just the first update that fails. + * An ERRORed transaction can not be committed and must be rolled + * back using transaction_free. + */ +enum ref_transaction_state { + REF_TRANSACTION_OPEN = 0, + REF_TRANSACTION_CLOSED = 1, + REF_TRANSACTION_ERROR = 2, +}; + +/* * Data structure for holding a reference transaction, which can * consist of checks and updates to multiple references, carried out * as atomically as possible. This structure is opaque to callers. @@ -3339,6 +3363,8 @@ struct ref_transaction { struct ref_update **updates; size_t alloc; size_t nr; + enum ref_transaction_state state; + int status; }; struct ref_transaction *ref_transaction_begin(struct strbuf *err) @@ -3476,8 +3502,13 @@ int ref_transaction_commit(struct ref_transaction *transaction, int n = transaction-nr; struct ref_update **updates = transaction-updates; - if (!n) + if (transaction-state != REF_TRANSACTION_OPEN) + return transaction-status; + + if (!n) { + transaction-state = REF_TRANSACTION_CLOSED; return 0; + } /* Allocate work space */ delnames = xmalloc(sizeof(*delnames) * n); @@ -3540,6 +3571,9 @@ int ref_transaction_commit(struct ref_transaction *transaction, clear_loose_ref_cache(ref_cache); cleanup: + transaction-state = ret ? REF_TRANSACTION_ERROR + : REF_TRANSACTION_CLOSED; + for (i = 0; i n; i++) if (updates[i]-lock) unlock_ref(updates[i]-lock); -- 2.0.0.rc3.474.g3833130 -- 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 v12 36/41] refs.c: move the check for valid refname to lock_ref_sha1_basic
Move the check for check_refname_format from lock_any_ref_for_update to lock_ref_sha1_basic. At some later stage we will get rid of lock_any_ref_for_update completely. This leaves lock_any_ref_for_updates as a no-op wrapper which could be removed. But this wrapper is also called from an external caller and we will soon make changes to the signature to lock_ref_sha1_basic that we do not want to expose to that caller. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-) diff --git a/refs.c b/refs.c index 5680028..df00993 100644 --- a/refs.c +++ b/refs.c @@ -2043,6 +2043,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, int missing = 0; int attempts_remaining = 3; + if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) + return NULL; + lock = xcalloc(1, sizeof(struct ref_lock)); lock-lock_fd = -1; @@ -2134,8 +2137,6 @@ struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) { - if (check_refname_format(refname, REFNAME_ALLOW_ONELEVEL)) - return NULL; return lock_ref_sha1_basic(refname, old_sha1, flags, type_p); } -- 2.0.0.rc3.474.g3833130 -- 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 v12 38/41] refs.c: pass a skip list to name_conflict_fn
Allow passing a list of refs to skip checking to name_conflict_fn. There are some conditions where we want to allow a temporary conflict and skip checking those refs. For example if we have a transaction that 1, guarantees that m is a packed refs and there is no loose ref for m 2, the transaction will delete m from the packed ref 3, the transaction will create conflicting m/m For this case we want to be able to lock and create m/m since we know that the conflict is only transient. I.e. the conflict will be automatically resolved by the transaction when it deletes m. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 36 ++-- 1 file changed, 26 insertions(+), 10 deletions(-) diff --git a/refs.c b/refs.c index edf73b7..ec8d642 100644 --- a/refs.c +++ b/refs.c @@ -793,11 +793,17 @@ struct name_conflict_cb { const char *refname; const char *oldrefname; const char *conflicting_refname; + const char **skip; + int skipnum; }; static int name_conflict_fn(struct ref_entry *entry, void *cb_data) { struct name_conflict_cb *data = (struct name_conflict_cb *)cb_data; + int i; + for (i = 0; i data-skipnum; i++) + if (!strcmp(entry-name, data-skip[i])) + return 0; if (data-oldrefname !strcmp(data-oldrefname, entry-name)) return 0; if (names_conflict(data-refname, entry-name)) { @@ -812,15 +818,19 @@ static int name_conflict_fn(struct ref_entry *entry, void *cb_data) * conflicting with the name of an existing reference in dir. If * oldrefname is non-NULL, ignore potential conflicts with oldrefname * (e.g., because oldrefname is scheduled for deletion in the same - * operation). + * operation). skip contains a list of refs we want to skip checking for + * conflicts with. */ static int is_refname_available(const char *refname, const char *oldrefname, - struct ref_dir *dir) + struct ref_dir *dir, + const char **skip, int skipnum) { struct name_conflict_cb data; data.refname = refname; data.oldrefname = oldrefname; data.conflicting_refname = NULL; + data.skip = skip; + data.skipnum = skipnum; sort_ref_dir(dir); if (do_for_each_entry_in_dir(dir, 0, name_conflict_fn, data)) { @@ -2032,7 +2042,8 @@ int dwim_log(const char *str, int len, unsigned char *sha1, char **log) static struct ref_lock *lock_ref_sha1_basic(const char *refname, const unsigned char *old_sha1, - int flags, int *type_p) + int flags, int *type_p, + const char **skip, int skipnum) { char *ref_file; const char *orig_refname = refname; @@ -2079,7 +2090,9 @@ static struct ref_lock *lock_ref_sha1_basic(const char *refname, * name is a proper prefix of our refname. */ if (missing -!is_refname_available(refname, NULL, get_packed_refs(ref_cache))) { +!is_refname_available(refname, NULL, + get_packed_refs(ref_cache), + skip, skipnum)) { last_errno = ENOTDIR; goto error_return; } @@ -2137,7 +2150,7 @@ struct ref_lock *lock_any_ref_for_update(const char *refname, const unsigned char *old_sha1, int flags, int *type_p) { - return lock_ref_sha1_basic(refname, old_sha1, flags, type_p); + return lock_ref_sha1_basic(refname, old_sha1, flags, type_p, NULL, 0); } /* @@ -2624,10 +2637,12 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms if (!symref) return error(refname %s not found, oldrefname); - if (!is_refname_available(newrefname, oldrefname, get_packed_refs(ref_cache))) + if (!is_refname_available(newrefname, oldrefname, + get_packed_refs(ref_cache), NULL, 0)) return 1; - if (!is_refname_available(newrefname, oldrefname, get_loose_refs(ref_cache))) + if (!is_refname_available(newrefname, oldrefname, + get_loose_refs(ref_cache), NULL, 0)) return 1; if (log rename(git_path(logs/%s, oldrefname), git_path(TMP_RENAMED_LOG))) @@ -2660,7 +2675,7 @@ int rename_ref(const char *oldrefname, const char *newrefname, const char *logms logmoved = log; - lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL); + lock = lock_ref_sha1_basic(newrefname, NULL, 0, NULL, NULL, 0); if (!lock) { error(unable to lock %s for update, newrefname); goto rollback; @@
[PATCH v12 41/41] refs.c: make write_ref_sha1 static
No external users call write_ref_sha1 any more so lets declare it static. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 6 +- refs.h | 3 --- 2 files changed, 5 insertions(+), 4 deletions(-) diff --git a/refs.c b/refs.c index 6414afc..aefa5bc 100644 --- a/refs.c +++ b/refs.c @@ -2618,6 +2618,9 @@ static int rename_tmp_log(const char *newrefname) return 0; } +static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, + const char *logmsg); + int rename_ref(const char *oldrefname, const char *newrefname, const char *logmsg) { unsigned char sha1[20], orig_sha1[20]; @@ -2848,7 +2851,8 @@ static int is_branch(const char *refname) return !strcmp(refname, HEAD) || starts_with(refname, refs/heads/); } -int write_ref_sha1(struct ref_lock *lock, +/** Writes sha1 into the ref specified by the lock. **/ +static int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *logmsg) { static char term = '\n'; diff --git a/refs.h b/refs.h index 88732a1..88020c5 100644 --- a/refs.h +++ b/refs.h @@ -195,9 +195,6 @@ extern int commit_ref(struct ref_lock *lock); /** Release any lock taken but not written. **/ extern void unlock_ref(struct ref_lock *lock); -/** Writes sha1 into the ref specified by the lock. **/ -extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg); - /** Setup reflog before using. **/ int log_ref_setup(const char *refname, char *logfile, int bufsize); -- 2.0.0.rc3.474.g3833130 -- 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 v12 24/41] receive-pack.c: use a reference transaction for updating the refs
Wrap all the ref updates inside a transaction. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- builtin/receive-pack.c | 31 ++- 1 file changed, 22 insertions(+), 9 deletions(-) diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index c323081..13f4a63 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -46,6 +46,7 @@ static void *head_name_to_free; static int sent_capabilities; static int shallow_update; static const char *alt_shallow_file; +static struct string_list error_strings = STRING_LIST_INIT_DUP; static enum deny_action parse_deny_action(const char *var, const char *value) { @@ -475,7 +476,6 @@ static const char *update(struct command *cmd, struct shallow_info *si) const char *namespaced_name; unsigned char *old_sha1 = cmd-old_sha1; unsigned char *new_sha1 = cmd-new_sha1; - struct ref_lock *lock; /* only refs/... are allowed */ if (!starts_with(name, refs/) || check_refname_format(name + 5, 0)) { @@ -576,19 +576,31 @@ static const char *update(struct command *cmd, struct shallow_info *si) return NULL; /* good */ } else { + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction; + if (shallow_update si-shallow_ref[cmd-index] update_shallow_ref(cmd, si)) return shallow error; - lock = lock_any_ref_for_update(namespaced_name, old_sha1, - 0, NULL); - if (!lock) { - rp_error(failed to lock %s, name); - return failed to lock; - } - if (write_ref_sha1(lock, new_sha1, push)) { - return failed to write; /* error() already called */ + transaction = ref_transaction_begin(err); + if (!transaction || + ref_transaction_update(transaction, namespaced_name, + new_sha1, old_sha1, 0, 1, err) || + ref_transaction_commit(transaction, push, err)) { + + const char *str; + string_list_append(error_strings, err.buf); + str = error_strings.items[error_strings.nr - 1].string; + strbuf_release(err); + + ref_transaction_free(transaction); + rp_error(%s, str); + return str; } + + ref_transaction_free(transaction); + strbuf_release(err); return NULL; /* good */ } } @@ -1215,5 +1227,6 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) packet_flush(1); sha1_array_clear(shallow); sha1_array_clear(ref); + string_list_clear(error_strings, 0); return 0; } -- 2.0.0.rc3.474.g3833130 -- 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 v12 39/41] refs.c: propagate any errno==ENOTDIR from _commit back to the callers
In _commit, ENOTDIR can happen in the call to lock_ref_sha1_basic, either when we lstat the new refname and it returns ENOTDIR or if the name checking function reports that the same type of conflict happened. In both cases it means that we can not create the new ref due to a name conflict. For these cases, save the errno value and abort and make sure that the caller can see errno==ENOTDIR. Also start defining specific return codes for _commit, assign -1 as a generic error and -2 as the error that refers to a name conflict. Callers can (and should) use that return value inspecting errno directly. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 23 --- refs.h | 4 2 files changed, 20 insertions(+), 7 deletions(-) diff --git a/refs.c b/refs.c index ec8d642..6414afc 100644 --- a/refs.c +++ b/refs.c @@ -3504,7 +3504,7 @@ static int ref_update_reject_duplicates(struct ref_update **updates, int n, int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err) { - int ret = 0, delnum = 0, i; + int ret = 0, delnum = 0, i, save_errno = 0; const char **delnames; int n = transaction-nr; struct ref_update **updates = transaction-updates; @@ -3522,9 +3522,10 @@ int ref_transaction_commit(struct ref_transaction *transaction, /* Copy, sort, and reject duplicate refs */ qsort(updates, n, sizeof(*updates), ref_update_compare); - ret = ref_update_reject_duplicates(updates, n, err); - if (ret) + if (ref_update_reject_duplicates(updates, n, err)) { + ret = -1; goto cleanup; + } /* Acquire all locks while verifying old values */ for (i = 0; i n; i++) { @@ -3538,10 +3539,12 @@ int ref_transaction_commit(struct ref_transaction *transaction, update-type, delnames, delnum); if (!update-lock) { + if (errno == ENOTDIR) + save_errno = errno; if (err) strbuf_addf(err, Cannot lock the ref '%s'., update-refname); - ret = 1; + ret = -1; goto cleanup; } } @@ -3559,6 +3562,7 @@ int ref_transaction_commit(struct ref_transaction *transaction, if (err) strbuf_addf(err, str, update-refname); + ret = -1; goto cleanup; } } @@ -3569,14 +3573,16 @@ int ref_transaction_commit(struct ref_transaction *transaction, struct ref_update *update = updates[i]; if (update-lock) { - ret |= delete_ref_loose(update-lock, update-type, - err); + if (delete_ref_loose(update-lock, update-type, err)) + ret = -1; + if (!(update-flags REF_ISPRUNING)) delnames[delnum++] = update-lock-ref_name; } } - ret |= repack_without_refs(delnames, delnum, err); + if (repack_without_refs(delnames, delnum, err)) + ret = -1; for (i = 0; i delnum; i++) unlink_or_warn(git_path(logs/%s, delnames[i])); clear_loose_ref_cache(ref_cache); @@ -3589,6 +3595,9 @@ cleanup: if (updates[i]-lock) unlock_ref(updates[i]-lock); free(delnames); + errno = save_errno; + if (save_errno == ENOTDIR) + ret = -2; return ret; } diff --git a/refs.h b/refs.h index 5c6c20f..88732a1 100644 --- a/refs.h +++ b/refs.h @@ -323,6 +323,10 @@ int ref_transaction_delete(struct ref_transaction *transaction, * Commit all of the changes that have been queued in transaction, as * atomically as possible. Return a nonzero value if there is a * problem. + * If the transaction is already in failed state this function will return + * an error. + * Function returns 0 on success, -1 for generic failures and -2 if the + * failure was due to a name collision (ENOTDIR). */ int ref_transaction_commit(struct ref_transaction *transaction, struct strbuf *err); -- 2.0.0.rc3.474.g3833130 -- 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 v12 26/41] walker.c: use ref transaction for ref updates
Switch to using ref transactions in walker_fetch(). As part of the refactoring to use ref transactions we also fix a potential memory leak where in the original code if write_ref_sha1() would fail we would end up returning from the function without free()ing the msg string. Note that this function is only called when fetching from a remote HTTP repository onto the local (most of the time single-user) repository which likely means that the type of collissions that the previous locking would protect against and cause the fetch to fail for to be even more rare. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- walker.c | 59 +++ 1 file changed, 35 insertions(+), 24 deletions(-) diff --git a/walker.c b/walker.c index 1dd86b8..60d9f9e 100644 --- a/walker.c +++ b/walker.c @@ -251,39 +251,36 @@ void walker_targets_free(int targets, char **target, const char **write_ref) int walker_fetch(struct walker *walker, int targets, char **target, const char **write_ref, const char *write_ref_log_details) { - struct ref_lock **lock = xcalloc(targets, sizeof(struct ref_lock *)); + struct strbuf ref_name = STRBUF_INIT; + struct strbuf err = STRBUF_INIT; + struct ref_transaction *transaction = NULL; unsigned char *sha1 = xmalloc(targets * 20); - char *msg; - int ret; + char *msg = NULL; int i; save_commit_buffer = 0; - for (i = 0; i targets; i++) { - if (!write_ref || !write_ref[i]) - continue; - - lock[i] = lock_ref_sha1(write_ref[i], NULL); - if (!lock[i]) { - error(Can't lock ref %s, write_ref[i]); - goto unlock_and_fail; + if (write_ref) { + transaction = ref_transaction_begin(err); + if (!transaction) { + error(%s, err.buf); + goto rollback_and_fail; } } - if (!walker-get_recover) for_each_ref(mark_complete, NULL); for (i = 0; i targets; i++) { if (interpret_target(walker, target[i], sha1[20 * i])) { error(Could not interpret response from server '%s' as something to pull, target[i]); - goto unlock_and_fail; + goto rollback_and_fail; } if (process(walker, lookup_unknown_object(sha1[20 * i]))) - goto unlock_and_fail; + goto rollback_and_fail; } if (loop(walker)) - goto unlock_and_fail; + goto rollback_and_fail; if (write_ref_log_details) { msg = xmalloc(strlen(write_ref_log_details) + 12); @@ -294,19 +291,33 @@ int walker_fetch(struct walker *walker, int targets, char **target, for (i = 0; i targets; i++) { if (!write_ref || !write_ref[i]) continue; - ret = write_ref_sha1(lock[i], sha1[20 * i], msg ? msg : fetch (unknown)); - lock[i] = NULL; - if (ret) - goto unlock_and_fail; + strbuf_reset(ref_name); + strbuf_addf(ref_name, refs/%s, write_ref[i]); + if (ref_transaction_update(transaction, ref_name.buf, + sha1[20 * i], NULL, 0, 0, + err)) { + error(%s, err.buf); + goto rollback_and_fail; + } + } + if (write_ref) { + if (ref_transaction_commit(transaction, + msg ? msg : fetch (unknown), + err)) { + error(%s, err.buf); + goto rollback_and_fail; + } + ref_transaction_free(transaction); } - free(msg); + free(msg); return 0; -unlock_and_fail: - for (i = 0; i targets; i++) - if (lock[i]) - unlock_ref(lock[i]); +rollback_and_fail: + ref_transaction_free(transaction); + free(msg); + strbuf_release(err); + strbuf_release(ref_name); return -1; } -- 2.0.0.rc3.474.g3833130 -- 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 v12 32/41] refs.c: make delete_ref use a transaction
Change delete_ref to use a ref transaction for the deletion. At the same time since we no longer have any callers of repack_without_ref we can now delete this function. Change delete_ref to return 0 on success and -1 on failure instead of the previous 0 on success either 1 or -1 on failure. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- refs.c | 36 ++-- 1 file changed, 14 insertions(+), 22 deletions(-) diff --git a/refs.c b/refs.c index 60593d7..e2c2249 100644 --- a/refs.c +++ b/refs.c @@ -2494,11 +2494,6 @@ static int repack_without_refs(const char **refnames, int n, struct strbuf *err) return ret; } -static int repack_without_ref(const char *refname) -{ - return repack_without_refs(refname, 1, NULL); -} - static int add_err_if_unremovable(const char *op, const char *file, struct strbuf *e, int rc) { @@ -2538,24 +2533,21 @@ static int delete_ref_loose(struct ref_lock *lock, int flag, struct strbuf *err) int delete_ref(const char *refname, const unsigned char *sha1, int delopt) { - struct ref_lock *lock; - int ret = 0, flag = 0; - - lock = lock_ref_sha1_basic(refname, sha1, delopt, flag); - if (!lock) - return 1; - ret |= delete_ref_loose(lock, flag, NULL); - - /* removing the loose one could have resurrected an earlier -* packed one. Also, if it was not loose we need to repack -* without it. -*/ - ret |= repack_without_ref(lock-ref_name); + struct ref_transaction *transaction; + struct strbuf err = STRBUF_INIT; - unlink_or_warn(git_path(logs/%s, lock-ref_name)); - clear_loose_ref_cache(ref_cache); - unlock_ref(lock); - return ret; + transaction = ref_transaction_begin(err); + if (!transaction || + ref_transaction_delete(transaction, refname, sha1, delopt, + sha1 !is_null_sha1(sha1), err) || + ref_transaction_commit(transaction, NULL, err)) { + error(%s, err.buf); + ref_transaction_free(transaction); + strbuf_release(err); + return -1; + } + ref_transaction_free(transaction); + return 0; } /* -- 2.0.0.rc3.474.g3833130 -- 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 v11 25/41] fast-import.c: use a ref transaction when dumping tags
On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder jrnie...@gmail.com wrote: Ronnie Sahlberg wrote: I rely on the fact that if the transaction has failed then it is safe to call ref_transaction_commit since it is guaranteed to return an error too. Yes, I am saying that behavior for ref_transaction_commit is weird. Usually when ref_transaction_commit is called I can do struct strbuf err = STRBUF_INIT; if (ref_transaction_commit(..., err)) die(%s, err.buf); and I know that since ref_transaction_commit has returned a nonzero result, err.buf is populated with a sensible message that will describe what went wrong. That's true even if there's a bug elsewhere in code I didn't write (e.g., someone forgot to check the return value from ref_transaction_update). But the guarantee you are describing removes that property. It creates a case where ref_transaction_commit can return nonzero without updating err. So I get the following message: fatal: I don't think that's a good outcome. In this case fatal: can not happen. This is no more subtle than most of the git core. I have changed this function to explicitly abort on _update failing but I think this is making the api too restrictive. Sure, if I am well acquainted with the API, I can make sure to use the same strbuf for all transaction API calls. But that would result in strange behavior, too: if multiple _update calls fail, then I get concatenated messages. Okay, I can make sure to do at most one failing _update, before calling _commit and printing the error. But at that point, what is the advantage over normal exception handling, where the error gets handled at the _update call site? 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 v12 00/44] Use ref transactions for all ref updates
These patches are the remaining patches that need review in this series. Please review them. On Thu, May 29, 2014 at 9:07 AM, Ronnie Sahlberg sahlb...@google.com wrote: This patch series can also be found at https://github.com/rsahlberg/git/tree/ref-transactions Ronnie please review these remaining patches in this series. Sahlberg (44): refs.c: constify the sha arguments for ref_transaction_create|delete|update refs.c: allow passing NULL to ref_transaction_free refs.c: add a strbuf argument to ref_transaction_commit for error logging refs.c: add an err argument to repack_without_refs refs.c: make ref_update_reject_duplicates take a strbuf argument for errors refs.c: add an err argument to delete_ref_loose refs.c: make update_ref_write update a strbuf on failure update-ref.c: log transaction error from the update_ref refs.c: remove the onerr argument to ref_transaction_commit refs.c: change ref_transaction_update() to do error checking and return status refs.c: change ref_transaction_create to do error checking and return status refs.c: ref_transaction_delete to check for error and return status tag.c: use ref transactions when doing updates replace.c: use the ref transaction functions for updates commit.c: use ref transactions for updates sequencer.c: use ref transactions for all ref updates fast-import.c: change update_branch to use ref transactions branch.c: use ref transaction for all ref updates refs.c: change update_ref to use a transaction refs.c: free the transaction before returning when number of updates is 0 refs.c: ref_transaction_commit should not free the transaction fetch.c: clear errno before calling functions that might set it fetch.c: change s_update_ref to use a ref transaction fetch.c: use a single ref transaction for all ref updates receive-pack.c: use a reference transaction for updating the refs fast-import.c: use a ref transaction when dumping tags walker.c: use ref transaction for ref updates refs.c: make write_ref_sha1 static refs.c: make lock_ref_sha1 static refs.c: add transaction.status and track OPEN/CLOSED/ERROR refs.c: remove the update_ref_lock function refs.c: remove the update_ref_write function refs.c: remove lock_ref_sha1 refs.c: make prune_ref use a transaction to delete the ref refs.c: make delete_ref use a transaction refs.c: pass the ref log message to _create/delete/update instead of _commit refs.c: pass NULL as *flags to read_ref_full refs.c: pack all refs before we start to rename a ref refs.c: move the check for valid refname to lock_ref_sha1_basic refs.c: call lock_ref_sha1_basic directly from commit refs.c: add a new flag for transaction delete for refs we know are packed only refs.c: pass a skip list to name_conflict_fn refs.c: make rename_ref use a transaction refs.c: remove forward declaration of write_ref_sha1 branch.c | 30 ++-- builtin/commit.c | 24 ++- builtin/fetch.c| 29 +-- builtin/receive-pack.c | 21 +-- builtin/replace.c | 15 +- builtin/tag.c | 15 +- builtin/update-ref.c | 32 ++-- cache.h| 2 + fast-import.c | 42 +++-- lockfile.c | 21 ++- refs.c | 468 + refs.h | 54 +++--- sequencer.c| 24 ++- t/t3200-branch.sh | 2 +- walker.c | 51 +++--- 15 files changed, 491 insertions(+), 339 deletions(-) -- 2.0.0.rc3.510.g20c254b -- 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: [BUG REPORT] Git log pretty date
On Thu, May 29, 2014 at 05:50:56PM +0700, Duy Nguyen wrote: On Thu, May 29, 2014 at 5:29 PM, Rodrigo Fernandes rtfrodr...@gmail.com wrote: I get an empty response on the date field, but since pretty has `%ad` it should follow the --date and return the date even if wrong. ... I tried to check the source code but have no idea where to start, maybe if you point me on some direction I can take a look for my self. pretty.c, format_commit_one() - format_person_part() - show_ident_date() - show_date(). The last one is in date.c show_ident_date() recently learned to always print Jan 1 1970 as a sentinel for dates that cannot be parsed (or for --date=raw, 0 +). But I do not think we hit that code path at all here. It looks like split_ident_line notices the bogus date and returns NULL for s.date_begin. Then format_person_part skips the placeholder, since it knows we have nothing worth showing. I think we probably want to do something like: diff --git a/pretty.c b/pretty.c index 3c43db5..7214a2c 100644 --- a/pretty.c +++ b/pretty.c @@ -732,14 +732,6 @@ static size_t format_person_part(struct strbuf *sb, char part, return placeholder_len; } - if (!s.date_begin) - goto skip; - - if (part == 't') { /* date, UNIX timestamp */ - strbuf_add(sb, s.date_begin, s.date_end - s.date_begin); - return placeholder_len; - } - switch (part) { case 'd': /* date */ strbuf_addstr(sb, show_ident_date(s, dmode)); @@ -753,6 +745,9 @@ static size_t format_person_part(struct strbuf *sb, char part, case 'i': /* date, ISO 8601 */ strbuf_addstr(sb, show_ident_date(s, DATE_ISO8601)); return placeholder_len; + case 't': /* date, UNIX timestamp */ + strbuf_addstr(sb, show_ident_date(s, DATE_RAW)); + return placeholder_len; } skip: to at least make --format date output consistent with the rest of git (and to make %at consistent with %ad and --date=raw). That still doesn't address Rodrigo's concern, though (we would print 0 +). For that, we would want on top: 1. Teach split_ident_line to return _something_ for his malformed case. 2. Teach show_ident_line to treat DATE_RAW to truly output raw content, even if it is malformed. I am not sure whether those are a good idea or not. The current code provides some level of sanitization, so that a parser of log output will not get malformed cruft. And DATE_RAW may mean show me what is in the raw commit, even if it is broken. Or it may just mean show me the date in unix timestamp format, because that is easy to parse. I'd argue that if somebody really wants the former, they should be using --pretty=raw, which will show the raw commit headers, without any parsing or fixup 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
Re: [PATCH] check_refname_component: Optimize
Duy Nguyen pclo...@gmail.com writes: On Thu, May 29, 2014 at 6:49 AM, David Turner dtur...@twopensource.com wrote: I assume that most of the time spent in check_refname_component() is while reading the packed-refs file, right? Yes. I wonder if we can get away without SSE code by saving stat info of the packed-refs version that we have verified. When we read pack-refs, if stat info matches, skip check_refname_component(). Assuming that pack-refs does not change often, of course. Can you elaborate a bit more? Regardless, I think I would prefer to see this patch done as at least a two step series, one that does only the look-up table thing, and then the other with arch-specific tweaks as a follow-up on top. -- 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 v11 25/41] fast-import.c: use a ref transaction when dumping tags
Ronnie Sahlberg wrote: On Wed, May 28, 2014 at 4:39 PM, Jonathan Nieder jrnie...@gmail.com wrote: Usually when ref_transaction_commit is called I can do struct strbuf err = STRBUF_INIT; if (ref_transaction_commit(..., err)) die(%s, err.buf); and I know that since ref_transaction_commit has returned a nonzero result, err.buf is populated with a sensible message that will describe what went wrong. [...] But the guarantee you are describing removes that property. It creates a case where ref_transaction_commit can return nonzero without updating err. So I get the following message: fatal: I don't think that's a good outcome. In this case fatal: can not happen. This is no more subtle than most of the git core. I have changed this function to explicitly abort on _update failing but I think this is making the api too restrictive. I don't want to push you toward making a change you think is wrong. I certainly don't own the codebase, and there are lots of other people (e.g., Michael, Junio, Jeff) to get advice from. So I guess I should try to address this. I'm not quite sure what you mean by too restrictive. a. Having API constraints that aren't enforced by the function makes using the API too fussy. I agree with that. That was something I liked about keeping track of the OPEN/CLOSED state of a transaction, which would let functions like _commit die() if someone is misusing the API so the problem gets detected early. b. Having to check the return value from _update() is too fussy. It certainly seems *possible* to have an API that doesn't require checking the return value, while still avoiding the usability problem I described in the quoted message above. For example: * _update() returns void and has no strbuf parameter * error handling happens by checking the error from _commit() That would score well on the scale described at http://ozlabs.org/~rusty/index.cgi/tech/2008-03-30.html An API where checking the return value is optional would be doable, too. For example: * _update() returns int and has a strbuf parameter * if the strbuf parameter is NULL, the caller is expected to wait for _commit() to check for errors, and a relevant message will be passed back then * if the strbuf parameter is non-NULL, then calling _commit() after an error is an API violation I don't understand the comment about no more subtle than most of git. Are you talking about the errno action at a distance you found in some functions? I thought we agreed that those were mistakes that accrue when people aim for a quick fix without thinking about maintainability and something git should have less of. 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] sideband.c: Get rid of ANSI sequences for non-terminal shell
On Wed, May 28, 2014 at 03:12:15AM +, Naumov, Michael (North Sydney) wrote: Some git tools such as GitExtensions for Windows use environment variable TERM=msys which causes the weird ANSI sequence shown for the messages returned from server-side hooks We add those ANSI sequences to help format sideband data on the user's terminal. However, GitExtensions is not using a terminal, and the ANSI sequences just confuses it. We can recognize this use by checking isatty(). See https://github.com/gitextensions/gitextensions/issues/1313 for more details Please wrap your commit messages (and emails) at something reasonable to fit on an 80-char terminal with indentation (I usually use 60, but anything = 72 is probably fine). Other than that, I think the patch looks fine. P.S. I gave up trying to send this letter from gmail, it eats my tab character P.S 2. Sorry, my tab character has been eaten again! This version looks OK to me (and applies via git-am). -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 v12 06/41] refs.c: add an err argument to repack_without_refs
Hi, Ronnie Sahlberg wrote: Update repack_without_refs to take an err argument and update it if there is a failure. Pass the err variable from ref_transaction_commit to this function so that callers can print a meaningful error message if _commit fails due to a problem in repack_without_refs. Add a new function unable_to_lock_message that takes a strbuf argument and fills in the reason for the failure. In commit_packed_refs, make sure that we propagate any errno that commit_lock_file might have set back to our caller. Signed-off-by: Ronnie Sahlberg sahlb...@google.com --- cache.h| 2 ++ lockfile.c | 21 - refs.c | 25 +++-- 3 files changed, 33 insertions(+), 15 deletions(-) I don't want to sound like a broken record, but this still has the same issues I described before at http://thread.gmane.org/gmane.comp.version-control.git/250197/focus=250309. The commit message or documentation or notes after the three dashes above could explain what I missed when making my suggestions. Otherwise I get no reality check as a reviewer, other reviewers get less insight into what's happening in the patch, people in the future looking into the patch don't understand its design as well, etc. As a general rule, that is a good practice anyway --- even when a reviewer was confused, what they got confused about can be an indication of where to make the code or design documentation (commit message) more clear, and when reposting a patch it can be a good opportunity to explain how the patch evolved. What would be wrong with the line of API documentation and the TODO comment for a known bug I suggested? If they are a bad idea, can you explain that so I can learn from it? Or if they were confusing, would you like a patch that explains what I mean? 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: .gitmodules containing SSH clone URLs should fall back to HTTPS when SSH key is not valid/existent
Jens Lehmann jens.lehm...@web.de writes: Am 29.05.2014 04:07, schrieb Jonathan Leonard: The title pretty much says it all. But you do not give much information about your special use case. Perhaps git grep insteadOf Documentation/ is all that is needed? -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.0.0
On Wed, May 28, 2014 at 06:17:25PM -0500, Felipe Contreras wrote: This is the last mail I sent to you, because you ignore them anyway, and remove them from the mailing list. [...] [2], a mail you conveniently removed from the tracked record. [...] You also conveniently removed this mail from the archives. I see you already noticed the changes in v2.0, but I wanted to address these points, because I consider silent censorship to be a serious accusation. I do not think Junio or anyone else has the technical ability to remove messages from the archive. There is not one archive, but rather several that get messages straight from vger.kernel.org and keep their own database (e.g., gmane, marc, spinics, nabble). E.g., here is the deleted message on nabble: http://git.661346.n2.nabble.com/PATCH-remote-helpers-point-at-their-upstream-repositories-td7610799i20.html#a7611324 Here it is on gmane: http://permalink.gmane.org/gmane.comp.version-control.git/249741 However, I had to pull that link from the NNTP interface. If you look at the non-threaded web interface for gmane here: http://blog.gmane.org/gmane.comp.version-control.git/day=20140520 and here: http://blog.gmane.org/gmane.comp.version-control.git/day=20140521 you will see that there is a huge gap in the list coverage from about midnight on the 20th until the 24th (but these messages are available via nntp). So this seems much more like a gmane bug than anything else. I've reported the bug to gmane.discuss (no link yet, as I'm waiting for the message to go through, but it is not a high traffic group, so it should be easy to find the thread once it is there). -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: [BUG REPORT] Git log pretty date
Jeff King p...@peff.net writes: ... to at least make --format date output consistent with the rest of git (and to make %at consistent with %ad and --date=raw). That still doesn't address Rodrigo's concern, though (we would print 0 +). For that, we would want on top: 1. Teach split_ident_line to return _something_ for his malformed case. 2. Teach show_ident_line to treat DATE_RAW to truly output raw content, even if it is malformed. I am not sure whether those are a good idea or not. The current code provides some level of sanitization, so that a parser of log output will not get malformed cruft. And DATE_RAW may mean show me what is in the raw commit, even if it is broken. Or it may just mean show me the date in unix timestamp format, because that is easy to parse. I'd argue that if somebody really wants the former, they should be using --pretty=raw, which will show the raw commit headers, without any parsing or fixup at all. I actually am not very much interested in deciding what to show for a broken timestamp. An empty string is just as good as any random cruft. I agree with you that it would be nice to have one escape hatch to let the users see what garbage is recorded, if only for diagnostic purposes, and DATE_RAW may be one good way to do so (but I'd rather recommend cat-file commit for real diagnostics). I would be more interested to see whatever broken tool that created such a commit gets fixed. Do we know where it came from? -- 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 10/10] t9904: new __git_ps1 tests for Zsh
Richard Hansen rhan...@bbn.com writes: These are the same tests as in t9903, but run in zsh instead of bash. Signed-off-by: Richard Hansen rhan...@bbn.com --- t/lib-zsh.sh | 30 ++ t/t9904-zsh-prompt.sh | 10 ++ 2 files changed, 40 insertions(+) create mode 100644 t/lib-zsh.sh create mode 100755 t/t9904-zsh-prompt.sh This doesn't appear to work in valgrind mode: $ ./t9904-zsh-prompt.sh --valgrind error: Test script did not set test_description. t9903 however works. I'm not sure how much of a difference it makes, but: I use bash as my shell and as /bin/sh, but I do have zsh installed. Can you look into it? -- Thomas Rast t...@thomasrast.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: Git chokes on large file
From: David Lang da...@lang.hm well, as others noted, the problem is actually caused by doing the diffs, and that is something that is a very common thing to do with source code. To some degree, my attitude comes from When I Was A Boy, when you got 16k for both your bytecode and your data, so you never kept more than one line of a file in memory unless you had to. Regardless of that, the problem with git fsck is not due to doing diffs, and git commit by default does diffs even if you don't ask for them, so the observed problems cannot be subsumed under the label you asked for a diff of a file that can't be diffed. And I would assume that files of several MB would be able to fit in memory (again, this was assumed to be for development, and compilers take a lot of ram to run, so having enough ram to hold any individual source file while the compiler is _not_ using ram doesn't seem likely to be a problem) At least the first versions of GCC only kept one function (and the global symbol table) in memory at once, so you could compile a source file that was larger than the available memory. In any case, if Git is no longer limited to handling source files, it needs to be updated so it can handle large files. Dale -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.0.0
Jeff King p...@peff.net writes: On Wed, May 28, 2014 at 06:17:25PM -0500, Felipe Contreras wrote: This is the last mail I sent to you, because you ignore them anyway, and remove them from the mailing list. [...] [2], a mail you conveniently removed from the tracked record. [...] You also conveniently removed this mail from the archives. I see you already noticed the changes in v2.0, but I wanted to address these points, because I consider silent censorship to be a serious accusation. I do not think Junio or anyone else has the technical ability to remove messages from the archive. You can post self-destructing messages by adding X-no-archive: yes if I am not mistaken. But that only concerns stuff you post yourself. There is not one archive, but rather several that get messages straight from vger.kernel.org and keep their own database (e.g., gmane, marc, spinics, nabble). Frankly, I find it weird that vger.kernel.org does not have an archive of its own. But yes, that makes it unlikely that silent censorship is much of a thing. A list moderator may put a particular sender on silent moderation, where postings will only appear once he has acknowledged them. However, that is a manual and burdensome process and requires an up-front decision to do so. Once a message made it to the list, the various archives will pick it up. -- David Kastrup -- 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
Reminder Notice
- Original Message - From: Jennifer H Gerald This Email is to notify you that your Email address have Won you an Award Sum of 1.500,000.00 (1.5 Million Euros) in an E-mail balloting program Held in Europe. Please contact the claim Officer with your winning info Ref Num(NL 030126)Batch Num(EU-0840113NL)Ticket Num(04,558,130) Name; Mr Nelson Paul,Email: agentnp...@aol.co.uk Sincerely yours, Jennifer Heldy Gerald, Public Relation Officer. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [ANNOUNCE] Git v2.0.0
On Thu, May 29, 2014 at 09:23:06PM +0200, David Kastrup wrote: I do not think Junio or anyone else has the technical ability to remove messages from the archive. You can post self-destructing messages by adding X-no-archive: yes if I am not mistaken. But that only concerns stuff you post yourself. Right, I was thinking specifically of deleting somebody else's message. The other obvious choke point is to convince vger to silently drop messages from somebody, but: 1. That is different than deleting already-posted messages. 2. vger is maintained by kernel.org folks, none of whom is a regular on the git list. So it would involve some collusion with those admins. That is pretty clearly not what happened here (the messages did go out to the list). Frankly, I find it weird that vger.kernel.org does not have an archive of its own. I don't think there is much need, as gmane is pretty featureful (and if you disagree, there are other competitors, or you can set up your own). I assume the current situation grew organically (somebody wanted an archive, so they set it up, and then vger saw no need to compete). -Peff PS My gmane bug report posted here: http://article.gmane.org/gmane.discuss/16175 -- 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: [BUG REPORT] Git log pretty date
On Thu, May 29, 2014 at 11:57:15AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... to at least make --format date output consistent with the rest of git (and to make %at consistent with %ad and --date=raw). That still doesn't address Rodrigo's concern, though (we would print 0 +). [...] I actually am not very much interested in deciding what to show for a broken timestamp. An empty string is just as good as any random cruft. I was thinking specifically of the first part I quoted above. We are not consistent between various methods of parsing/printing the date. That may fall into the if were doing it from scratch... category, though; it's possible that people currently using --format=%ad prefer and expect the empty string to denote a bogus value. I'm OK with leaving it. I agree with you that it would be nice to have one escape hatch to let the users see what garbage is recorded, if only for diagnostic purposes, and DATE_RAW may be one good way to do so (but I'd rather recommend cat-file commit for real diagnostics). Yeah, in case I wasn't clear, I don't actually like DATE_RAW as a way to do that. I'd prefer --pretty=raw or cat-file commit, which already work. I would be more interested to see whatever broken tool that created such a commit gets fixed. Do we know where it came from? I don't think it has been said yet in the thread. Rodrigo? -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: [BUG REPORT] Git log pretty date
Jeff, I have no idea what was the tool. The repo is not mine. I found the problem when I was doing some tests and the commit parsing was failing on that repo. Cumprimentos, Rodrigo Fernandes On Thu, May 29, 2014 at 8:49 PM, Jeff King p...@peff.net wrote: On Thu, May 29, 2014 at 11:57:15AM -0700, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... to at least make --format date output consistent with the rest of git (and to make %at consistent with %ad and --date=raw). That still doesn't address Rodrigo's concern, though (we would print 0 +). [...] I actually am not very much interested in deciding what to show for a broken timestamp. An empty string is just as good as any random cruft. I was thinking specifically of the first part I quoted above. We are not consistent between various methods of parsing/printing the date. That may fall into the if were doing it from scratch... category, though; it's possible that people currently using --format=%ad prefer and expect the empty string to denote a bogus value. I'm OK with leaving it. I agree with you that it would be nice to have one escape hatch to let the users see what garbage is recorded, if only for diagnostic purposes, and DATE_RAW may be one good way to do so (but I'd rather recommend cat-file commit for real diagnostics). Yeah, in case I wasn't clear, I don't actually like DATE_RAW as a way to do that. I'd prefer --pretty=raw or cat-file commit, which already work. I would be more interested to see whatever broken tool that created such a commit gets fixed. Do we know where it came from? I don't think it has been said yet in the thread. Rodrigo? -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] Improve function dir.c:trim_trailing_spaces()
On Wed, May 28, 2014 at 04:45:57PM -0700, Pasha Bolokhov wrote: Move backwards from the end of the string (more efficient for lines which do not have trailing spaces or have just a couple). The original code reads the string from left to right. In theory, that means we could get away with not calling strlen() at all, over a right-to-left that must start with a call to strlen(). That being said, I think we already have the length in the calling function, so you could probably avoid the strlen() altogether, which makes a right-to-left function more efficient. However, I doubt it makes that much of a difference in practice, so unless it's measurable, I would certainly go with the version that is more readable (and correct, of course). Slightly more rare occurrences of 'text \' with a backslash in between spaces are handled correctly. Can you add a test for this? Also, if you are refactoring this function, I think it makes sense to check that: foo\\ and foo\\\ match foo\ and foo\ , respectively (I think they do with your patch, but it is a tricky case that is not immediately obvious). -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] Improve function dir.c:trim_trailing_spaces()
On Thu, May 29, 2014 at 1:13 PM, Jeff King p...@peff.net wrote: On Wed, May 28, 2014 at 04:45:57PM -0700, Pasha Bolokhov wrote: Move backwards from the end of the string (more efficient for lines which do not have trailing spaces or have just a couple). The original code reads the string from left to right. In theory, that means we could get away with not calling strlen() at all, over a right-to-left that must start with a call to strlen(). That being said, I think we already have the length in the calling function, so you could probably avoid the strlen() altogether, which makes a right-to-left function more efficient. However, I doubt it makes that much of a difference in practice, so unless it's measurable, I would certainly go with the version that is more readable (and correct, of course). Sorry, just to recap, you would go with the existing version (which needs correction), or with the one that is being suggested? (I agree I can format the style a tiny bit better in the latter one) Tests should not be a big problem, although it's kind of clumsy to test an internal function which does not really give any output (you can only measure the outcome). Just again to stress, I have tested both implementation extensively and the suggested new implementation gives the correct answers for all your examples below and all others. If I show this with explicit t/ tests, will it suffice then? Basically what I suggest is -- either: improve the existing function such that it does correctly that text \case, and also does not use 'strlen' since it anyway moves left to right -- or: use the new suggested implementation (and just brush the formatting a bit), and perhaps borrow 'len' from the calling routine And add tests in any case. What is the preference? Slightly more rare occurrences of 'text \' with a backslash in between spaces are handled correctly. Can you add a test for this? Also, if you are refactoring this function, I think it makes sense to check that: foo\\ and foo\\\ match foo\ and foo\ , respectively (I think they do with your patch, but it is a tricky case that is not immediately obvious). -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 11/10] fixup! t9904: new __git_ps1 tests for Zsh
Signed-off-by: Richard Hansen rhan...@bbn.com --- On 2014-05-29 15:02, Thomas Rast wrote: Richard Hansen rhan...@bbn.com writes: These are the same tests as in t9903, but run in zsh instead of bash. Signed-off-by: Richard Hansen rhan...@bbn.com --- t/lib-zsh.sh | 30 ++ t/t9904-zsh-prompt.sh | 10 ++ 2 files changed, 40 insertions(+) create mode 100644 t/lib-zsh.sh create mode 100755 t/t9904-zsh-prompt.sh This doesn't appear to work in valgrind mode: $ ./t9904-zsh-prompt.sh --valgrind error: Test script did not set test_description. t9903 however works. I'm not sure how much of a difference it makes, but: I use bash as my shell and as /bin/sh, but I do have zsh installed. Can you look into it? *sigh* By default, Zsh munges $0 whenever a function is called or a file is sourced, with no (immediately obvious) way to get the original value of $0. This fixup causes that feature to be temporarily turned off so that test-lib.sh does the right thing when it execs $0. Thank you for finding this bug! -Richard t/lib-zsh.sh | 15 --- 1 file changed, 12 insertions(+), 3 deletions(-) diff --git a/t/lib-zsh.sh b/t/lib-zsh.sh index fa6fcd9..ab4bef2 100644 --- a/t/lib-zsh.sh +++ b/t/lib-zsh.sh @@ -2,17 +2,23 @@ # run under Zsh; primarily intended for tests of the git-prompt.sh # script. -if test -n $ZSH_VERSION test -z $POSIXLY_CORRECT; then +if test -n $ZSH_VERSION test -z $POSIXLY_CORRECT [[ ! -o FUNCTION_ARGZERO ]]; then true elif command -v zsh /dev/null 21; then unset POSIXLY_CORRECT - exec zsh $0 $@ + # Run Zsh with the FUNCTION_ARGZERO option disabled so that + # test-lib.sh sees the test script pathname when it examines + # $0 instead of ./lib-zsh.sh. (This works around a Zsh bug; + # 'emulate sh -c' should temporarily restore $0 to the POSIX + # specification for $0, but it doesn't.) + exec zsh +o FUNCTION_ARGZERO $0 $@ else echo '1..0 #SKIP skipping Zsh-specific tests; zsh not available' exit 0 fi -# ensure that we are in full-on Zsh mode +# ensure that we are in full-on Zsh mode. note: this re-enables the +# FUNCTION_ARGZERO option emulate -R zsh || exit 1 shellname=Zsh @@ -27,4 +33,7 @@ set_ps1_format_vars () { c_clear='%%f' } +# note: although the FUNCTION_ARGZERO option is currently enabled, sh +# emulation mode temporarily turns it off ($0 is left alone when +# sourcing test-lib.sh) emulate sh -c '. ./test-lib.sh' -- 2.0.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
[RFC PATCH] git log: support auto decorations
From: Linus Torvalds torva...@linux-foundation.org Date: Thu, 29 May 2014 15:19:40 -0700 Subject: [RFC PATCH] git log: support auto decorations This works kind of like --color=auto - add decorations for interactive use, but do not change defaults when scripting or when piping the output to anything but a terminal. You can use either [log] decorate=auto in the git config files, or the --decorate=auto command line option to choose this behavior. Signed-off-by: Linus Torvalds torva...@linux-foundation.org --- I actually like seeing decorations by default, but I do *not* think our current log.decorate options make sense, since they will change any random use of git log to have decorations. I much prefer the ui.color=auto behavior that we have for coloration. This is a trivial patch that tries to approximate that. It's marked with RFC because (a) that isatty(1) || pager_in_use() test is kind of hacky, maybe we would be better off sharing something with the auto-coloration? (b) I also think it would be nice to have the equivalent for --show-signature, but there we don't have any preexisting config file option. (c) maybe somebody would like a way to combine auto and full, although personally that doesn't seem to strike me as all that useful (would you really want to see the full refname when not scripting it) but the patch is certainly simple and seems to work. Comments? builtin/log.c | 2 ++ 1 file changed, 2 insertions(+) diff --git a/builtin/log.c b/builtin/log.c index 39e883635279..df6396c9c3d9 100644 --- a/builtin/log.c +++ b/builtin/log.c @@ -63,6 +63,8 @@ static int parse_decoration_style(const char *var, const char *value) return DECORATE_FULL_REFS; else if (!strcmp(value, short)) return DECORATE_SHORT_REFS; + else if (!strcmp(value, auto)) + return (isatty(1) || pager_in_use()) ? DECORATE_SHORT_REFS : 0; return -1; } -- 2.0.0.1.g5beb60c -- 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/5] fetch doc: update introductory part for clarity
- Branches is a more common way to say heads in these days. - Remote-tracking branches are used a lot more these days and it is worth mentioning that it is one of the primary side effects of the command to update them. - Avoid X. That means Y. If Y is easier to understand to readers, just say that upfront. - Use of explicit refspec to fetch tags does not have much to do with turning auto following on or off. It is a way to fetch tags that otherwise would not be fetched by auto-following. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/git-fetch.txt | 29 - 1 file changed, 16 insertions(+), 13 deletions(-) diff --git a/Documentation/git-fetch.txt b/Documentation/git-fetch.txt index 5809aa4..d5f5b54 100644 --- a/Documentation/git-fetch.txt +++ b/Documentation/git-fetch.txt @@ -17,20 +17,23 @@ SYNOPSIS DESCRIPTION --- -Fetches named heads or tags from one or more other repositories, -along with the objects necessary to complete them. - -The ref names and their object names of fetched refs are stored -in `.git/FETCH_HEAD`. This information is left for a later merge -operation done by 'git merge'. - -By default, tags are auto-followed. This means that when fetching -from a remote, any tags on the remote that point to objects that exist -in the local repository are fetched. The effect is to fetch tags that +Fetch branches and/or tags (collectively, refs) from one or more +other repositories, along with the objects necessary to complete the +histories of them. + +The names of refs that are fetched, together with the object names +they point at, are written to `.git/FETCH_HEAD`. This information +is used by a later merge operation done by 'git merge'. In addition, +the remote-tracking branches may be updated (see description on +refspec below for details). + +By default, any tag that points into the histories being fetched is +also fetched; the effect is to fetch tags that point at branches that you are interested in. This default behavior -can be changed by using the --tags or --no-tags options, by -configuring remote.name.tagopt, or by using a refspec that fetches -tags explicitly. +can be changed by using the --tags or --no-tags options or by +configuring remote.name.tagopt. By using a refspec that fetches tags +explicitly, you can fetch tags that do not point into branches you +are interested in as well. 'git fetch' can fetch from either a single named repository, or from several repositories at once if group is given and -- 2.0.0-479-g59ac8f9 -- 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 2/5] fetch doc: update note on '+' in front of the refspec
While it is not *wrong* per-se to say that pulling a rewound/rebased branch will lead to an unnecessary merge conflict, that is not what the leading + sign to allow non-fast-forward update of remote-tracking branch is at all. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/pull-fetch-param.txt | 18 +- 1 file changed, 9 insertions(+), 9 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 18cffc2..2a7e2b7 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -24,15 +24,15 @@ is updated even if it does not result in a fast-forward update. + [NOTE] -If the remote branch from which you want to pull is -modified in non-linear ways such as being rewound and -rebased frequently, then a pull will attempt a merge with -an older version of itself, likely conflict, and fail. -It is under these conditions that you would want to use -the `+` sign to indicate non-fast-forward updates will -be needed. There is currently no easy way to determine -or declare that a branch will be made available in a -repository with this behavior; the pulling user simply +When the remote branch you want to fetch is known to +be rewound and rebased regularly, it is expected that +the tip of it will not be descendant of the commit that +used to be at its tip the last time you fetched it and +stored in your remote-tracking branch. You would want +to use the `+` sign to indicate non-fast-forward updates +will be needed for such branches. There is no way to +determine or declare that a branch will be made available +in a repository with this behavior; the pulling user simply must know this is the expected usage pattern for a branch. + [NOTE] -- 2.0.0-479-g59ac8f9 -- 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 4/5] fetch doc: on pulling multiple refspecs
Replace desription of old-style Pull: lines in remotes/ configuration with modern remote.*.fetch variables. As this note applies only to git pull, enable it only in git-pull manual page. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/pull-fetch-param.txt | 12 1 file changed, 8 insertions(+), 4 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index e266c2d..ea4c5a6 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -34,22 +34,26 @@ will be needed for such branches. There is no way to determine or declare that a branch will be made available in a repository with this behavior; the pulling user simply must know this is the expected usage pattern for a branch. +ifdef::git-pull[] + [NOTE] There is a difference between listing multiple refspec directly on 'git pull' command line and having multiple -`Pull:` refspec lines for a repository and running +`remote.repository.fetch` entries in your configuration +for a repository and running 'git pull' command without any explicit refspec parameters. refspec listed explicitly on the command line are always merged into the current branch after fetching. In other words, if you list more than one remote refs, you would be making -an Octopus. While 'git pull' run without any explicit refspec -parameter takes default refspecs from `Pull:` lines, it +an Octopus merge. On the other hand, 'git pull' that is run +without any explicit refspec parameter takes default +refspecs from `remote.repository.fetch` configuration, it merges only the first refspec found into the current branch, -after fetching all the remote refs. This is because making an +after fetching all the remote refs specified. This is because making an Octopus from remote refs is rarely done, while keeping track of multiple remote heads in one-go by fetching more than one is often useful. +endif::git-pull[] + Some short-cut notations are also supported. + -- 2.0.0-479-g59ac8f9 -- 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 5/5] fetch doc: update refspec format description
The text made it sound as if the leading plus is the only thing that is optional, and forgot that lhs is the same as lhs:, i.e. fetch it and do not store anywhere. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/pull-fetch-param.txt | 1 + 1 file changed, 1 insertion(+) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index ea4c5a6..27cfd5c 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -15,6 +15,7 @@ endif::git-pull[] The format of a refspec parameter is an optional plus `+`, followed by the source ref src, followed by a colon `:`, followed by the destination ref dst. + The colon can be omitted when dst is empty. + The remote ref that matches src is fetched, and if dst is not empty string, the local -- 2.0.0-479-g59ac8f9 -- 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 0/5] Documentation updates for 'git fetch'
Noticed that this page has antiquated description, which may still be correct, that can use some modernization. There are a few more larger updates coming, but these are small enough to be reviewed separately and quickly, so I am sending them out early. Junio C Hamano (5): fetch doc: update introductory part for clarity fetch doc: update note on '+' in front of the refspec fetch doc: remove notes on outdated mixed layout fetch doc: on pulling multiple refspecs fetch doc: update refspec format description Documentation/git-fetch.txt| 29 ++--- Documentation/pull-fetch-param.txt | 44 -- 2 files changed, 34 insertions(+), 39 deletions(-) -- 2.0.0-479-g59ac8f9 -- 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 3/5] fetch doc: remove notes on outdated mixed layout
In old days before Git 1.5, it was customery for git fetch to use the same local branch namespace to keep track of the remote-tracking branches, and it was necessary to tell users not to check them out and commit on them. Since everybody uses the separate remote layout these days, there is no need to warn against the practice to check out the right-hand side of refspec and build on it---the RHS is typically not even a local branch. Incidentally, this also kills one mention of Pull: line of $GIT_DIR/remotes/* configuration, which is a lot less familiar to new people than the more modern remote.*.fetch configuration variable. Signed-off-by: Junio C Hamano gits...@pobox.com --- Documentation/pull-fetch-param.txt | 13 - 1 file changed, 13 deletions(-) diff --git a/Documentation/pull-fetch-param.txt b/Documentation/pull-fetch-param.txt index 2a7e2b7..e266c2d 100644 --- a/Documentation/pull-fetch-param.txt +++ b/Documentation/pull-fetch-param.txt @@ -36,19 +36,6 @@ in a repository with this behavior; the pulling user simply must know this is the expected usage pattern for a branch. + [NOTE] -You never do your own development on branches that appear -on the right hand side of a refspec colon on `Pull:` lines; -they are to be updated by 'git fetch'. If you intend to do -development derived from a remote branch `B`, have a `Pull:` -line to track it (i.e. `Pull: B:remote-B`), and have a separate -branch `my-B` to do your development on top of it. The latter -is created by `git branch my-B remote-B` (or its equivalent `git -checkout -b my-B remote-B`). Run `git fetch` to keep track of -the progress of the remote side, and when you see something new -on the remote branch, merge it into your development branch with -`git pull . remote-B`, while you are on `my-B` branch. -+ -[NOTE] There is a difference between listing multiple refspec directly on 'git pull' command line and having multiple `Pull:` refspec lines for a repository and running -- 2.0.0-479-g59ac8f9 -- 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: .gitmodules containing SSH clone URLs should fall back to HTTPS when SSH key is not valid/existent
But you do not give much information about your special use case. I assume you have submodule repositories for which some developers have a valid ssh key and others don't (maybe because they should only have read access via https)? Precisely. Specifically this is for a collection (17 or more) of GitHub-hosted projects which are maintained by only a couple of people (who have the ability to directly push via git:// or ssh://). Everyone else (including deployments and ordinary users) who clones the repo should be able to just grab the code via HTTPS and have it work. If that is the case you might want to look into access control tools like gitolite. We are using GitHub. Lack of this feature (or presence of this bug [depending on your perspective]) is a major PITA. But why is https special? Why not fall back to the git protocol? Or http? (And no: I'm not serious here ;-) HTTPS isn't special except in that it is the least privileged transport type (and thus should be the last resort). Whether to fallback to git:// from ssh:// or vice versa is inconsequential to this request. After the first failed clone of the submodule at via SSH the developer could also just do a git config submodule.name.url https://host/repo and override the URL from .gitmodules. Yes, this would work. But it would be a painful manual step which we would not want to force on ordinary users (and would not want to experience ourselves either). It should be noted that this is only really a problem as the other options GitHub gives us are also equally (or more) painful: a) - a unique deploy key per machine and project. (which at current would be 17 * 3 keys all manually maintained via clicking through a GUI [unless we wanted to automate via GitHub API (which is also a non-trivial amount of work)]). - or - b) - a fake 'team' with read-only access with a single fake GitHub account as member thereof. I imagine this feature would be convenient for non-GitHub scenarios as well though. --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] check_refname_component: Optimize
On Thu, May 29, 2014 at 11:36 PM, Junio C Hamano gits...@pobox.com wrote: Duy Nguyen pclo...@gmail.com writes: On Thu, May 29, 2014 at 6:49 AM, David Turner dtur...@twopensource.com wrote: I assume that most of the time spent in check_refname_component() is while reading the packed-refs file, right? Yes. I wonder if we can get away without SSE code by saving stat info of the packed-refs version that we have verified. When we read pack-refs, if stat info matches, skip check_refname_component(). Assuming that pack-refs does not change often, of course. Can you elaborate a bit more? The first time we read packed_refs, check_refname_format() is called in read_packed_refs()-create_ref_entry() as usual. If we find no problem, we store packed_refs stat() info in maybe packed_refs.stat. Next time we read packed_refs, if packed_refs.stat is there and indicates that packed_refs has not changed, we can make create_ref_entry() ignore check_refname_format() completely. That's even cheaper than SSE-enhanced check_refname_component() and easier to do. If packed_refs is updated, we do the whole check_refname_format dance again. -- 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
git 2.0.0 PROFILE=BUILD check-phase problems with ./t5561-http-backend.sh; GIT_TEST_HTTPD=false problems with t5537-fetch-shallow.sh
I observe test failures with git 2.0.0 which are attributable to the change to run network tests by default. I'm lumping them both together into one report because I'm lazy and I've blown too much time on this already. I've got Apache 2.2.24 on this box, and t5551-http-fetch-smart.sh fails in a peculiar fashion during the PROFILE=BUILD stage: --- exp 2014-05-29 22:42:50.221599297 + +++ act 2014-05-29 22:42:50.231598452 + @@ -12,10 +12,10 @@ GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 200 - GET /smart/repo.git/objects/41/57d6f47fc8b7cb455fbf6f18d6f47fed49a6a5 HTTP/1.1 200 GET /smart/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.pack HTTP/1.1 200 -GET /smart/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.idx HTTP/1.1 200 ### no git-daemon-export-ok ### +GET /smart/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.idx HTTP/1.1 200 GET /smart_noexport/repo.git/HEAD HTTP/1.1 404 - GET /smart_noexport/repo.git/info/refs HTTP/1.1 404 - GET /smart_noexport/repo.git/objects/info/packs HTTP/1.1 404 - @@ -34,10 +34,10 @@ GET /smart_noexport/repo.git/objects/info/http-alternates HTTP/1.1 200 - GET /smart_noexport/repo.git/objects/41/57d6f47fc8b7cb455fbf6f18d6f47fed49a6a5 HTTP/1.1 200 GET /smart_noexport/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.pack HTTP/1.1 200 -GET /smart_noexport/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.idx HTTP/1.1 200 ### getanyfile true ### +GET /smart_noexport/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.idx HTTP/1.1 200 GET /smart/repo.git/HEAD HTTP/1.1 200 GET /smart/repo.git/info/refs HTTP/1.1 200 GET /smart/repo.git/objects/info/packs HTTP/1.1 200 @@ -45,10 +45,10 @@ GET /smart/repo.git/objects/info/http-alternates HTTP/1.1 200 - GET /smart/repo.git/objects/41/57d6f47fc8b7cb455fbf6f18d6f47fed49a6a5 HTTP/1.1 200 GET /smart/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.pack HTTP/1.1 200 -GET /smart/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.idx HTTP/1.1 200 ### getanyfile false ### +GET /smart/repo.git/objects/pack/pack-2cb3186872e8768852fcb6b22cdf2182e12f7490.idx HTTP/1.1 200 GET /smart/repo.git/HEAD HTTP/1.1 403 - GET /smart/repo.git/info/refs HTTP/1.1 403 - GET /smart/repo.git/objects/info/packs HTTP/1.1 403 - not ok 14 - server request log matches test results # # sed -e # s/^.* \// # s/\// # s/ [1-9][0-9]*\$// # s/^GET /GET / #act $HTTPD_ROOT_PATH/access.log # test_cmp exp act # # failed 1 among 14 test(s) It appears that the Apache daemon is writing to the log slowly enough that its log lines only get there after the testsuite has written its separator, so a bunch of log lines appear to be attached to the wrong test, and the comparison fails. Curiously, I can't make this happen in a conventional 'make check', even though the only relevant components would seem to be bash and httpd: if anything, you'd expect the gcovvery to slow down git and thus make it *more* likely that any race between httpd log syncing and testsuite framework output to the same logfile would be hit... Attempting to work around this by building with GIT_TEST_HTTPD=false doesn't work either: *** t5537-fetch-shallow.sh *** ok 1 - setup ok 2 - setup shallow clone ok 3 - clone from shallow clone ok 4 - fetch from shallow clone ok 5 - fetch --depth from shallow clone ok 6 - fetch --unshallow from shallow clone ok 7 - fetch something upstream has but hidden by clients shallow boundaries ok 8 - fetch that requires changes in .git/shallow is filtered ok 9 - fetch --update-shallow error: Can't use skip_all after running some tests Makefile:43: recipe for target 't5537-fetch-shallow.sh' failed make[3]: *** [t5537-fetch-shallow.sh] Error 1 since this is trying to run the httpd halfway through the test, which will never work if it's skipping it. Moving the httpd sourcing to the top of the test isn't going to work either, because that would skip *everything*, when we want to skip only the httpd bits. Maybe splitting the httpd bits into a separate test is best here? I'm not sure. -- NULL (void) -- 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] check_refname_component: Optimize
On Fri, May 30, 2014 at 6:41 AM, Jeff King p...@peff.net wrote: On Fri, May 30, 2014 at 06:24:30AM +0700, Duy Nguyen wrote: I wonder if we can get away without SSE code by saving stat info of the packed-refs version that we have verified. When we read pack-refs, if stat info matches, skip check_refname_component(). Assuming that pack-refs does not change often, of course. Can you elaborate a bit more? The first time we read packed_refs, check_refname_format() is called in read_packed_refs()-create_ref_entry() as usual. If we find no problem, we store packed_refs stat() info in maybe packed_refs.stat. Next time we read packed_refs, if packed_refs.stat is there and indicates that packed_refs has not changed, we can make create_ref_entry() ignore check_refname_format() completely. I'm confused. Why would we re-open packed-refs at all if the stat information hasn't changed? No, not in the same process. In the next run. read_packed_refs is only called from get_packed_ref_cache, and we only do so if !refs-packed. And refs-packed is only NULL if we haven't read the file yet, or it is stat-dirty. If that is working as intended, then we should generally only open and read packed-refs once per invocation of git. -Peff -- 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: .gitmodules containing SSH clone URLs should fall back to HTTPS when SSH key is not valid/existent
On Thu, May 29, 2014 at 04:12:38PM -0700, Jonathan Leonard wrote: We are using GitHub. [...] But why is https special? Why not fall back to the git protocol? Or http? (And no: I'm not serious here ;-) HTTPS isn't special except in that it is the least privileged transport type (and thus should be the last resort). Whether to fallback to git:// from ssh:// or vice versa is inconsequential to this request. That's not quite true. git:// is the least privileged transport, as it always anonymous and read-only (there ways to allow insecure pushes over it, but GitHub does not enable them). Https is actually the most flexible protocol, in that the same URL works of the box for both logged-in and anonymous users (the latter assuming the repo is publicly available). The server only prompts for credentials if necessary. For that reason, it's a good choice for things like submodule URLs baked into .gitmodules files. The reasons not to are: 1. It isn't _quite_ as efficient or robust as the regular git protocol, though in practice it's generally not a big deal. 2. Pushers may prefer to authenticate with ssh keys (e.g., because they run ssh agent). I hope with modern credential helpers that logging in via http should not be a pain, either, though. After the first failed clone of the submodule at via SSH the developer could also just do a git config submodule.name.url https://host/repo and override the URL from .gitmodules. Yes, this would work. But it would be a painful manual step which we would not want to force on ordinary users (and would not want to experience ourselves either). Using insteadOf of in your global ~/.gitconfig would make this a one-liner per-user. So one option would be to reverse things. Put https URLs into the .gitmodules file, so most people have to do nothing, and then developers who really want to do git-over-ssh can do a one-time: git config --global url@github.com:.insteadOf https://github.com/ -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] check_refname_component: Optimize
On Fri, May 30, 2014 at 06:43:18AM +0700, Duy Nguyen wrote: The first time we read packed_refs, check_refname_format() is called in read_packed_refs()-create_ref_entry() as usual. If we find no problem, we store packed_refs stat() info in maybe packed_refs.stat. Next time we read packed_refs, if packed_refs.stat is there and indicates that packed_refs has not changed, we can make create_ref_entry() ignore check_refname_format() completely. I'm confused. Why would we re-open packed-refs at all if the stat information hasn't changed? No, not in the same process. In the next run. Ah, I thought packed_refs.stat was a struct member, but I guess you mean it as a filename. But then we're just trusting that the trust me flag on disk is correct. Why not just trust that packed-refs is correct in the first place? IOW, consider this progression of changes: 1. Check refname format when we read packed-refs (the current behavior). 2. Keep a separate file packed-refs.stat with stat information. If the packed-refs file matches that stat information, do not bother checking refname formats. 3. Put a flag in packed-refs that says trust me, I'm valid. Check the refnames when it is generated. 4. Realize that we already check the refnames when we write it out. Don't bother writing trust me, I'm valid; readers can assume that it is. What is the scenario that option (2) protects against that options (3) and (4) do not? I could guess something like the writer has a different idea of what a valid refname is than we do. But that applies as well to (2), but just as the reader who wrote packed-refs.stat has a different idea than we do. As a side note, while it is nice that we might make check_refname_format faster, I think if you _really_ want to make repos with a lot of refs faster, it would make more sense to introduce an on-disk format that does not need linear parsing (e.g., something we could mmap and binary search, or even something dbm-ish that could be updated without rewriting the whole file (deletions, for example, must rewrite the whole file, giving quadratic performance when deleting all refs one by one). -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
Bugreport: git push disobeys -c remote.xxx.url=...
Hello! I've tried to changing the URL of a remote temporarily because of network issues. I tried something like this: git -c remote.foo.url=http://gitserver.example/repo.git push foo bar Tracing shows that git push does not use the provided URL for the remote foo and instead uses the URL configured in the repository configuration as if the -c option was not present at all. This looks like a bug to me. My git identifies as version 1.8.1.2. Yours sincerely, Robert Clausecker -- () ascii ribbon campaign - for an 8-bit clean world /\ - against html email - against proprietary attachments -- 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: git 2.0.0 PROFILE=BUILD check-phase problems with ./t5561-http-backend.sh; GIT_TEST_HTTPD=false problems with t5537-fetch-shallow.sh
On Thu, May 29, 2014 at 11:44:37PM +0100, Nix wrote: I observe test failures with git 2.0.0 which are attributable to the change to run network tests by default. I'm lumping them both together into one report because I'm lazy and I've blown too much time on this already. Weird. I also see a strange failure on t5310 when building with PROFILE=BUILD. We get a segfault when reading jgit-produced bitmaps. Tracking it down, we're getting inexplicably bogus data from an mmap'd file (!). Compiling without PROFILE=BUILD, the test passes fine (even with valgrind). If I instrument it like this: diff --git a/ewah/ewah_io.c b/ewah/ewah_io.c index f7f700e..8cafacf 100644 --- a/ewah/ewah_io.c +++ b/ewah/ewah_io.c @@ -119,6 +119,7 @@ int ewah_read_mmap(struct ewah_bitmap *self, void *map, size_t len) ptr += sizeof(uint32_t); self-buffer_size = self-alloc_size = get_be32(ptr); + warning(got buffer_size of %lu from %lu, self-buffer_size, *(uint32_t *)ptr); ptr += sizeof(uint32_t); self-buffer = ewah_realloc(self-buffer, a regular test-run reads: warning: got buffer_size of 2 from 33554432 warning: got buffer_size of 2 from 33554432 warning: got buffer_size of 3 from 50331648 warning: got buffer_size of 1 from 16777216 warning: got buffer_size of 2 from 33554432 and a PROFILE=BUILD one reads: warning: got buffer_size of 2 from 33554432 warning: got buffer_size of 2 from 33554432 warning: got buffer_size of 3 from 50331648 warning: got buffer_size of 1 from 16777216 warning: got buffer_size of 131072 from 512 I'm willing to believe we're doing something weird that violates the C standard there, but I really can't see it. Anyway, that's a side note to your problem... It appears that the Apache daemon is writing to the log slowly enough that its log lines only get there after the testsuite has written its separator, so a bunch of log lines appear to be attached to the wrong test, and the comparison fails. Yeah, that looks to me like what is happening, too. If I put a 'sleep 1' into log_div, it passes. I would think apache would write the log before serving the file, but perhaps not. And like you, I would expect gcov to make things slower, not faster. Could there be something in the environment that I'm not sure what the best fix is. We could check the logfiles after each test instead of at the end, but that will just end up with the same race: we may check them before apache has written them. Attempting to work around this by building with GIT_TEST_HTTPD=false doesn't work either: *** t5537-fetch-shallow.sh *** ok 1 - setup ok 2 - setup shallow clone ok 3 - clone from shallow clone ok 4 - fetch from shallow clone ok 5 - fetch --depth from shallow clone ok 6 - fetch --unshallow from shallow clone ok 7 - fetch something upstream has but hidden by clients shallow boundaries ok 8 - fetch that requires changes in .git/shallow is filtered ok 9 - fetch --update-shallow error: Can't use skip_all after running some tests Makefile:43: recipe for target 't5537-fetch-shallow.sh' failed make[3]: *** [t5537-fetch-shallow.sh] Error 1 Hrm. This already came up, and we dealt with it in 0232852 (t5537: move http tests out to t5539, 2014-02-13). Which is in v2.0.0. But somehow the code is back in v2.0.0 Presumably this is the result of a mis-merge. I'll send a patch. -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] t5537: re-drop http tests
These were originally removed by 0232852 (t5537: move http tests out to t5539, 2014-02-13). However, they were accidentally re-added in 1ddb4d7 (Merge branch 'nd/upload-pack-shallow', 2014-03-21). This looks like an error in manual conflict resolution. Here's what happened: 1. v1.9.0 shipped with the http tests in t5537. 2. We realized that this caused problems, and built 0232852 on top to move the tests to their own file. This fix made it into v1.9.1. 3. We later had another fix in nd/upload-pack-shallow that also touched t5537. It was built directly on v1.9.0. When we merged nd/upload-pack-shallow to master, we got a conflict; it was built on a version with the http tests, but we had since removed them. The correct resolution was to drop the http tests and keep the new ones, but instead we kept everything. Signed-off-by: Jeff King p...@peff.net --- This is a regression in running make test in v2.0.0 for people who do not have apache installed, so it would be nice to hit maint for 2.0.1. I had a very hard time finding the merge to blame, because the combined-diff code is convinced this isn't an interesting hunk (because we kept no content from the other side, as there was none to keep!). I eventually tracked it down with git log --first-parent -m t/t5537*. And a final side note. If you retry the merge by: m=1ddb4d7 git checkout $m^1 git merge $m^2 you can see the resulting conflict is quite tricky to understand. You can tell from the blank ours side that _something_ was removed (otherwise there would not have been a conflict), but there is no way to tell what, or that the same something is part of the theirs side. Viewing it with diff3 conflict-style makes it much more obvious. t/t5537-fetch-shallow.sh | 28 1 file changed, 28 deletions(-) diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh index be951a4..a980574 100755 --- a/t/t5537-fetch-shallow.sh +++ b/t/t5537-fetch-shallow.sh @@ -173,33 +173,6 @@ EOF ) ' -if test -n $NO_CURL -o -z $GIT_TEST_HTTPD; then - say 'skipping remaining tests, git built without http support' - test_done -fi - -LIB_HTTPD_PORT=${LIB_HTTPD_PORT-'5537'} -. $TEST_DIRECTORY/lib-httpd.sh -start_httpd - -test_expect_success 'clone http repository' ' - git clone --bare --no-local shallow $HTTPD_DOCUMENT_ROOT_PATH/repo.git - git clone $HTTPD_URL/smart/repo.git clone - ( - cd clone - git fsck - git log --format=%s origin/master actual - cat EOF expect -7 -6 -5 -4 -3 -EOF - test_cmp expect actual - ) -' - test_expect_success POSIXPERM,SANITY 'shallow fetch from a read-only repo' ' cp -R .git read-only.git find read-only.git -print | xargs chmod -w @@ -213,5 +186,4 @@ EOF test_cmp expect actual ' -stop_httpd test_done -- 2.0.0.rc1.436.g03cb729 -- 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: Bugreport: git push disobeys -c remote.xxx.url=...
On Fri, May 30, 2014 at 02:03:56AM +0200, f...@fuz.su wrote: I've tried to changing the URL of a remote temporarily because of network issues. I tried something like this: git -c remote.foo.url=http://gitserver.example/repo.git push foo bar Tracing shows that git push does not use the provided URL for the remote foo and instead uses the URL configured in the repository configuration as if the -c option was not present at all. This looks like a bug to me. You are correct that this won't work, but the reason is a bit complicated. The remote.*.url config field is actually a multivar, meaning that you can specify it multiple times. In that case, git push will push to each configured URL in order in which they appear in the config. In the command above you are not overwriting remote.foo.url, but rather adding an extra value to it. So we first try to push to the remote defined in your actual config, and then to the one on the command-line. You can see this in action like: $ git init --bare /tmp/foo $ git init --bare /tmp/bar $ git remote add foo /tmp/foo $ git -c remote.foo.url=/tmp/bar push foo HEAD You should see pushes to both /tmp/foo and /tmp/bar. Config multivars like this are rather hard to work with, as there is no way to say reset the multivar to empty, _then_ add this new value. So there isn't a simple solution using remote.*.url from the command-line like this. Another way of doing what you want is to use url.*.insteadOf, like: git -c url./tmp/bar.insteadof=/tmp/foo push foo HEAD -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: [RFC PATCH] git log: support auto decorations
On Thu, May 29, 2014 at 03:31:58PM -0700, Linus Torvalds wrote: From: Linus Torvalds torva...@linux-foundation.org Date: Thu, 29 May 2014 15:19:40 -0700 Subject: [RFC PATCH] git log: support auto decorations I will spare you the usual lecture on having these lines in the message body. ;) I actually like seeing decorations by default, but I do *not* think our current log.decorate options make sense, since they will change any random use of git log to have decorations. I much prefer the ui.color=auto behavior that we have for coloration. This is a trivial patch that tries to approximate that. Yeah, I think this makes a lot of sense. I do use log.decorate=true, and it is usually not a big deal. However, I think I have run into annoyances once or twice when piping it. I'd probably use log.decorate=auto if we had it. It's marked with RFC because (a) that isatty(1) || pager_in_use() test is kind of hacky, maybe we would be better off sharing something with the auto-coloration? The magic for this is in color.c, want_color() and check_auto_color(). The color code checks pager_use_color when the pager is in use, but I do not think that makes any sense here. It also checks that $TERM is not dumb, but that also does not make sense here. So I think your check is fine. It would be nice to share with the color code, but I doubt it will end up any more readable, because of conditionally dealing with those two differences. (b) I also think it would be nice to have the equivalent for --show-signature, but there we don't have any preexisting config file option. Potentially yes, though there is a real performance impact for log --show-signature if you actually have a lot of signatures. Even on linux.git, a full git log is 15s with --show-signature, and 5s without. Maybe that is acceptable for interactive use (and certainly it is not a reason to make it an _option_, if somebody wants to turn it on). (c) maybe somebody would like a way to combine auto and full, although personally that doesn't seem to strike me as all that useful (would you really want to see the full refname when not scripting it) Yeah, full/short is really orthogonal to true/false/auto. If we were starting from scratch, I think putting full/short into log.decorateStyle would make more sense, but it is probably not worth changing now. I agree that full auto is probably not something useful, and we can live without 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: [PATCH] check_refname_component: Optimize
On Fri, May 30, 2014 at 7:07 AM, Jeff King p...@peff.net wrote: But then we're just trusting that the trust me flag on disk is correct. Why not just trust that packed-refs is correct in the first place? IOW, consider this progression of changes: 1. Check refname format when we read packed-refs (the current behavior). 2. Keep a separate file packed-refs.stat with stat information. If the packed-refs file matches that stat information, do not bother checking refname formats. 3. Put a flag in packed-refs that says trust me, I'm valid. Check the refnames when it is generated. 4. Realize that we already check the refnames when we write it out. Don't bother writing trust me, I'm valid; readers can assume that it is. What is the scenario that option (2) protects against that options (3) and (4) do not? I could guess something like the writer has a different idea of what a valid refname is than we do. But that applies as well to (2), but just as the reader who wrote packed-refs.stat has a different idea than we do. The reader and the writer have to agree on the same valid definition or it wouldn't work. I don't suppose this packed-refs.stat idea would spread out to other implementations than C git, so we're still good. If we could write a flag in packed-refs saying trust me and other implementations will strip it when they update packed-refs, then we're good too. As a side note, while it is nice that we might make check_refname_format faster, I think if you _really_ want to make repos with a lot of refs faster, it would make more sense to introduce an on-disk format that does not need linear parsing (e.g., something we could mmap and binary search, or even something dbm-ish that could be updated without rewriting the whole file (deletions, for example, must rewrite the whole file, giving quadratic performance when deleting all refs one by one). Yeah, I bring up the idea because I think Mike's multiple ref backends is the way to go (assuming that it won't take as long as pack v4 development). If we assume we'll go with that, then we can keep the workaround to minimum. -- 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: .gitmodules containing SSH clone URLs should fall back to HTTPS when SSH key is not valid/existent
On Fri, May 30, 2014 at 11:12 AM, Jonathan Leonard johana...@gmail.com wrote: But you do not give much information about your special use case. I assume you have submodule repositories for which some developers have a valid ssh key and others don't (maybe because they should only have read access via https)? Precisely. Specifically this is for a collection (17 or more) of GitHub-hosted projects which are maintained by only a couple of people (who have the ability to directly push via git:// or ssh://). Everyone else (including deployments and ordinary users) who clones the repo should be able to just grab the code via HTTPS and have it work. If that is the case you might want to look into access control tools like gitolite. We are using GitHub. Lack of this feature (or presence of this bug [depending on your perspective]) is a major PITA. But why is https special? Why not fall back to the git protocol? Or http? (And no: I'm not serious here ;-) HTTPS isn't special except in that it is the least privileged transport type (and thus should be the last resort). Whether to fallback to git:// from ssh:// or vice versa is inconsequential to this request. The problem is that a ssh:// url can't necessarily be transformed into a correct https:// or git:// with a simple sed 's/ssh/https/' chances are other parts of the URL will need changing. Which quickly becomes non-trivial. One solution that we use at work is to use relative paths (e.g. ../code/mod1.git) in .gitmodules (assuming the submodules are all part of the same project). That way if you clone the superproject over https:// all the submodules use that too. This works well for us using local mirrors across multiple sites _and_ different protocols. Another option would be to have a policy of storing the most permissive transport in .gitmodules which makes it easy for users and puts the special config requirements on the maintainers. Both of these are obviously solutions you need to convince the maintainer(s) of the superproject to implement. Perhaps what git could do is allow multiple urls for a submodule and at git submodule init time try them in order until the fetch is successful. Or provide a mechanism to map transports, arguably this is the url.foo.insteadOf mechanisim that has already been suggested. -- 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 PATCH] git log: support auto decorations
On Thu, May 29, 2014 at 6:58 PM, Jeff King p...@peff.net wrote: I will spare you the usual lecture on having these lines in the message body. ;) We do it for the kernel because they often get lost otherwise. Particularly the date/author. git doesn't tend to have the same kind of deep email forwarding models, and nobody uses quilt to develop git (I hope!) but it's a habit I like to encourage for the kernel, so I use it in general.. Yeah, I think this makes a lot of sense. I do use log.decorate=true, and it is usually not a big deal. However, I think I have run into annoyances once or twice when piping it. I'd probably use log.decorate=auto if we had it. I have various scripts to generate releases etc, using git log and piping it to random other stuff. I don't _think_ they care, but quite frankly, I'd rather not even have to think about it. Also, I actually find the un-colorized version of the decorations to be distracting. With color (not that I really like the default color scheme, but I'm too lazy to set it up to anything else), the colorization ends up making the decorations much easier to visually separate, so I like them there. It's marked with RFC because (a) that isatty(1) || pager_in_use() test is kind of hacky, maybe we would be better off sharing something with the auto-coloration? The magic for this is in color.c, want_color() and check_auto_color(). Oh, I know. That's where I stole it from. But the colorization actually has very different rules, and looks at TERM etc. So I only stole the actual non-color-specific parts of it, but I was wondering whether it might make sense to unify them. (b) I also think it would be nice to have the equivalent for --show-signature, but there we don't have any preexisting config file option. Potentially yes, though there is a real performance impact for log --show-signature if you actually have a lot of signatures. Even on linux.git, a full git log is 15s with --show-signature, and 5s without. Maybe that is acceptable for interactive use (and certainly it is not a reason to make it an _option_, if somebody wants to turn it on). Yes. This is an example of where the whole tty is fundamentally different from scripting happens. It really is about the whole user is looking at it vs scripting. There is no way in hell that --show-signature should be on by default for anybody sane. That said, part of it is just that show-signature is so suboptimal performance-wise, re-parsing the commit buffer for each commit when show_signature is set. That's just crazy, we've already parsed the commit text, we already *could* know if it has a signature or not, and skip it if it doesn't. That would require one of the flag bits in the object, though, or something, so it's probably not worth doing. Sure, performance might matter if you end up searching for something in the pager after you've done git log, but personally I think I'd never even notice.. Linus -- 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