Re: [PATCH v3 2/3] relative_path should honor DOS and UNC paths
2013/9/18 Junio C Hamano gits...@pobox.com: Jiang Xin worldhello@gmail.com writes: diff --git a/compat/mingw.h b/compat/mingw.h index bd0a88b..06e9f49 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -311,6 +311,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format #define has_dos_drive_prefix(path) (isalpha(*(path)) (path)[1] == ':') #define is_dir_sep(c) ((c) == '/' || (c) == '\\') +static inline int is_unc_path(const char *path) +{ + if (!is_dir_sep(*path) || !is_dir_sep(*(path+1)) || is_dir_sep(*(path+2))) + return 0; A UNC path must start with two slashes, but not three or more slashes. If path[1] == '\0', it would be !is_dir_sep() and we end up inspecting past the end of the string? The funciton is_unc_path will return false (0), if path is , /, //, ///three/slashes/, or /usr/local. So the problem is ? -- Jiang Xin -- 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] git-compat-util: Avoid strcasecmp() being inlined
On Tue, Sep 17, 2013 at 11:46 PM, Junio C Hamano gits...@pobox.com wrote: I don't think people on other platforms seeing the ugliness is really an issue. After all, the file is called git-*compat*-util.h; Well, judging from the way Linus reacted to the patch, I'd have to disagree. After all, that argument leads to the position that nothing is needed in compat/, no? My feeling is that Linus' reaction was more about that this work-around is even necessary (and MinGW is buggy) rather than applying it to git-compat-util.h and not elsewhere. One ugliness (lack of sane strcasecmp definition whose address can be taken) specific to mingw is worked around in compat/mingw.h, and another ugliness that some people may use compilers without include_next may need help from another configuration in the Makefile to tell it where the platform string.h resides. I am not sure why you see it as a problem. I just don't like that the ugliness is spreading out and requires a change to config.mak.uname now, too. Also, I regard the change to config.mak.uname by itself as ugly, mainly because you would have to set SYSTEM_STRING_H_HEADER to some path, but that path might differ from system to system, depending on where MinGW is installed on Windows. I do insist to avoid GCC-ism in C files,... To that I tend to agree. Unconditionally killing inlining for any mingw compilation in compat/mingw.h may be the simplest (albeit it may be less than optimal) solution. I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed, it involved the need to shuffle includes in git-compat-util.h around because winsock2.h already seems to include string.h, and I did not find a working include order. So I came up with the following, do you like that better? diff --git a/compat/string_no_inline.h b/compat/string_no_inline.h new file mode 100644 index 000..51eed52 --- /dev/null +++ b/compat/string_no_inline.h @@ -0,0 +1,25 @@ +#ifndef STRING_NO_INLINE_H +#define STRING_NO_INLINE_H + +#ifdef __MINGW32__ +#ifdef __NO_INLINE__ +#define __NO_INLINE_ALREADY_DEFINED +#else +#define __NO_INLINE__ /* do not inline strcasecmp() */ +#endif +#endif + +#include string.h +#ifdef HAVE_STRINGS_H +#include strings.h /* for strcasecmp() */ +#endif + +#ifdef __MINGW32__ +#ifdef __NO_INLINE_ALREADY_DEFINED +#undef __NO_INLINE_ALREADY_DEFINED +#else +#undef __NO_INLINE__ +#endif +#endif + +#endif diff --git a/git-compat-util.h b/git-compat-util.h index db564b7..348dd55 100644 --- a/git-compat-util.h +++ b/git-compat-util.h @@ -85,6 +85,8 @@ #define _NETBSD_SOURCE 1 #define _SGI_SOURCE 1 +#include compat/string_no_inline.h + #ifdef WIN32 /* Both MinGW and MSVC */ #ifndef _WIN32_WINNT #define _WIN32_WINNT 0x0502 @@ -101,10 +103,6 @@ #include stddef.h #include stdlib.h #include stdarg.h -#include string.h -#ifdef HAVE_STRINGS_H -#include strings.h /* for strcasecmp() */ -#endif #include errno.h #include limits.h #ifdef NEEDS_SYS_PARAM_H -- 1.8.3.mingw.1.dirty -- Sebastian Schuberth -- 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: Locking files / git
Thanks a lot for your answer. That's really good arguments that i was waiting for and that i have not get until now. My comprehension now : - it's not easy to maintain several versions of a binary file in parallel. So basically, it's not recommanded to have complex workflow for binary files. In case the project has a low number of binary files, it can be handle by simple communication, In case the project has a lot of binary files, a simple workflow with a centralized workflow is recommanded - git doesn't hate locks, it's just that it's not the layer to implement it because git is workflow independant. Locks depend on a centralized server which is directly linked to the workflow. I'm not trying to implement a such workflow. I'm just curious, reading a lot of things about git, and trying to understand what is sometimes called a limitation of git. A simple line in the documentation to say that locking should be handled in the upper layer (and it's done for example in gitolite) because it's dependant of the workflow could help some people looking about that point. Thanks a lot for git. 2013/9/17 Fredrik Gustafsson iv...@iveqy.com: On Tue, Sep 17, 2013 at 09:45:26PM +0200, Nicolas Adenis-Lamarre wrote: Ooops. It seems that each time somebody says these two words together, people hate him, and he is scorned by friends and family. For the moment, i want a first feedback, an intermediate between locking is bad and ok, but i would prefer in the negativ answer something with arguments (Take CVS as an example of what not to do; if in doubt, make the exact opposite decision. is one), and in the positiv answer, good remarks about problems with my implementation that could make it better. So working with locks and text-files is generally stupid to do with git since you don't use git merging capabilities. Working with binary files in git is stupid because git doesn't handle them very well because they the deltas can't be calculated very good. It seems to me that if you need to do locking one of the above scenarios is true for you and you should not use git at all. However, there's always the case when you've a mixed project with both binary and text-files. In that case I believe Jeff gave an excellent answer. But think twice, are you using git in a sane way? Even a small binary file will result in a huge git repo if it's updated often and the project has a long history. -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] git submodule update should give notice when run without init beforehand
On Tue, Sep 17, 2013 at 1:06 AM, Jens Lehmann jens.lehm...@web.de wrote: Thanks Jens for having a look! Am 15.09.2013 19:38, schrieb Tay Ray Chuan: When 'update' is run with no path in a repository with uninitialized submodules, the program terminates with no output, and zero status code. Be more helpful to users by mentioning this. [snip] it would be rather nasty to error out on every submodule update. Just to be sure we're on the right page, with this patch, the 'update' command still exits with status code zero (non-error), so this patch doesn't make it error out. After the 'autoinit' configuration (which lets upstream hint that certain submodules should be initialized on clone) has materialzed we might want to enable this error for these specific submodules. That's cool, I'm looking forward to this. Could you point me to somewhere detailing this? But in the meantime, on top of the advice.* config, how about having a submodule.name.ignoreUninit config to disable the message on a per-submodule basis? But in any case the error message should contain a hint on how you can get rid of the error in case you know what you are doing ;-). The message does mention that you can throw in an --init to fix the problem. This hint is similar to what git-submodule prints when a path is passed (see region at line 807). -- Cheers, Ray Chuan -- 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] git-compat-util: Avoid strcasecmp() being inlined
On Wed, Sep 18, 2013 at 5:43 AM, Sebastian Schuberth sschube...@gmail.com wrote: My feeling is that Linus' reaction was more about that this work-around is even necessary (and MinGW is buggy) rather than applying it to git-compat-util.h and not elsewhere. So I think it's an annoying MinGW bug, but the reason I dislike the no-inline approach is two-fold: - it's *way* too intimate with the bug. When you have a bug like this, the *last* thing you want to do is to make sweet sweet love to it, and get really involved with it. You want to say Eww, what a nasty little bug, I don't want to have anything to do with you. And quite frankly, delving into the details of exactly *what* MinGW does wrong, and defining magic __NO_INLINE__ macros, knowing that that is the particular incantation that hides the MinGW bug, that's being too intimate. That's simply a level of detail that *nobody* should ever have to know. The other patch (having just a wrapper function) doesn't have those kinds of intimacy issues. That patch just says MinGW is buggy and cannot do this function uninlined, so we wrap it. Notice the lack of detail, and lack of *interest* in the exact particular pattern of the bug. The other reason I'm not a fan of the __NO_INLINE__ approach is even more straightforward: - Why should we disable the inlining of everything in string.h (and possibly elsewhere too - who the hell knows what __NO_INLINE__ will do to other header files), when in 99% of all the cases we don't care, and in fact inlining may well be good and the right thing to do. So the __NO_INLINE__ games seem to be both too big of a hammer, and too non-specific, and at the same time it gets really intimate with MinGW in unhealthy ways. If you know something is diseased, you keep your distance, you don't try to embrace it. I tried to put the __NO_INLINE__ stuff in compat/mingw.h but failed, it involved the need to shuffle includes in git-compat-util.h around because winsock2.h already seems to include string.h, and I did not find a working include order. So I came up with the following, do you like that better? Ugh, so now that patch is fragile, so we have to complicate it even more. Really, just make a wrapper function. It doesn't even need to be conditional on MinGW. Just a single one-liner function, with a comment above it that says MinGW is broken and doesn't have an out-of-line copy of strcasecmp(), so we wrap it here. No unnecessary details about internal workings of a buggy MinGW header file. No complexity. No subtle issues with include file ordering. Just a straightforward workaround that is easy to explain. 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
[PATCH np/pack-v4] index-pack: tighten object type check based on pack version
In pack version 4, ref-delta technically could be used to compress any objects including commits and trees (both canonical and v4). But it does not make sense to do so. It can only lead to performance degradation. Catch those packers. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- I could now verify that pack-objects does not compress commits nor trees using ref-delta in v4. But perhaps these are a bit too strict? Maybe downgrade from die() to warning() and still accept the pack? builtin/index-pack.c | 16 1 file changed, 16 insertions(+) diff --git a/builtin/index-pack.c b/builtin/index-pack.c index fbf97f0..e4ecf69 100644 --- a/builtin/index-pack.c +++ b/builtin/index-pack.c @@ -788,6 +788,12 @@ static void *unpack_raw_entry(struct object_entry *obj, case OBJ_BLOB: case OBJ_TAG: break; + + /* +* OBJ_PV4_* are all greater than 7, which is the limit of +* field type in pack v2. So we do not really need 'if +* (!packv4) die(wrong type);' here. +*/ case OBJ_PV4_COMMIT: obj-real_type = OBJ_COMMIT; break; @@ -1288,6 +1294,16 @@ static void resolve_delta(struct object_entry *delta_obj, hash_sha1_file(result-data, result-size, typename(delta_obj-real_type), delta_obj-idx.sha1); check_against_sha1table(delta_obj-idx.sha1); + if (packv4 + delta_obj-type == OBJ_REF_DELTA + delta_obj-real_type == OBJ_TREE) + bad_object(delta_obj-idx.offset, + _(ref-delta on a tree is not supported in pack version 4)); + if (packv4 + delta_obj-type == OBJ_REF_DELTA + delta_obj-real_type == OBJ_COMMIT) + bad_object(delta_obj-idx.offset, + _(ref-delta on a commit is not supported in pack version 4)); sha1_object(result-data, NULL, result-size, delta_obj-real_type, delta_obj-idx.sha1); counter_lock(); -- 1.8.2.83.gc99314b -- 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] git clone -q ends with early EOF
Hello, I am trying to clone a repository and I am getting the following output: $ git clone -q git://kernel.ubuntu.com/ubuntu/linux.git fatal: The remote end hung up unexpectedly fatal: early EOF fatal: index-pack failed The fatal: lines usually appear a few minutes after running the clone. Of course, the clone does not finish successfully. Interestingly, when I drop the '-q' option from the git clone commandline, the clone finishes correctly. My git version is: $ git --version git version 1.8.4.rc3 Is this a known issue? Thank you! Best regards, Marek Vasut -- 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] fetch-pack.c: show correct command name that fails
When --shallow-file is added to the command line, it has to be before the subcommand name, the first argument won't be the command name any more. Stop assuming that and keep track of the command name explicitly. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- fetch-pack.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 6684348..b259c51 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -688,7 +688,7 @@ static int get_pack(struct fetch_pack_args *args, const char *argv[22]; char keep_arg[256]; char hdr_arg[256]; - const char **av; + const char **av, *cmd_name; int do_keep = args-keep_pack; struct child_process cmd; int ret; @@ -735,7 +735,7 @@ static int get_pack(struct fetch_pack_args *args, if (do_keep) { if (pack_lockfile) cmd.out = -1; - *av++ = index-pack; + *av++ = cmd_name = index-pack; *av++ = --stdin; if (!args-quiet !args-no_progress) *av++ = -v; @@ -752,7 +752,7 @@ static int get_pack(struct fetch_pack_args *args, *av++ = --check-self-contained-and-connected; } else { - *av++ = unpack-objects; + *av++ = cmd_name = unpack-objects; if (args-quiet || args-no_progress) *av++ = -q; args-check_self_contained_and_connected = 0; @@ -770,7 +770,7 @@ static int get_pack(struct fetch_pack_args *args, cmd.in = demux.out; cmd.git_cmd = 1; if (start_command(cmd)) - die(fetch-pack: unable to fork off %s, argv[0]); + die(fetch-pack: unable to fork off %s, cmd_name); if (do_keep pack_lockfile) { *pack_lockfile = index_pack_lockfile(cmd.out); close(cmd.out); @@ -782,7 +782,7 @@ static int get_pack(struct fetch_pack_args *args, args-check_self_contained_and_connected ret == 0; else - die(%s failed, argv[0]); + die(%s failed, cmd_name); if (use_sideband finish_async(demux)) die(error in sideband demultiplexer); return 0; -- 1.8.2.83.gc99314b -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/2] relative_path should honor dos_drive_prefix
On 2013-09-17 21.32, Johannes Sixt wrote: Am 17.09.2013 10:24, schrieb Jiang Xin: I have checked the behavior of UNC path on Windows (msysGit): * I can cd to a UNC path: cd //server1/share1/path * can cd to other share: cd ../../share2/path * and can cd to other server's share: cd ../../../server2/share/path That means relative_path(path1, path2) support UNC paths out of the box. We only need to check both path1 and path2 are UNC paths, or both not. Your tests are flawed. You issued the commands in bash, which (or rather MSYS) does everything for you that you need to make it work. But in reality it does not, because the system cannot apply .. to //server/share: $ git ls-remote //srv/public/../repos/his/setups.git fatal: '//srv/public/../repos/his/setups.git' does not appear to be a git repository fatal: Could not read from remote repository. Please make sure you have the correct access rights and the repository exists. even though the repository (and //srv/public, let me assure) exists: $ git ls-remote //srv/repos/his/setups.git bea489b0611a72c41f133343fdccbd3e2b9f80b5HEAD ... The situation does not change with your latest round (v3). Please let me suggest not to scratch where there is no itch. ;) Your round v2 was good enough. If you really want to check UNC paths, then you must compare two path components after the the double-slash, not just one. Furthermore, you should audit all code that references is_absolute_path(), relative_path(), normalize_path_copy(), and possibly a few others whether the functions or call sites need improvement. That's worth a separate patch. -- Hannes I tend to agree here. The V2 patch fixed a regression. This should be one commit on its own: Documentation/SubmittingPatches: (1) Make separate commits for logically separate changes. Fixing a bug is a good thing, thanks for working on this, The support for UNC paths is a new feature, and this deserves a seperate commit. /Torsten -- 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 np/pack-v4] index-pack: tighten object type check based on pack version
On Wed, 18 Sep 2013, Nguyễn Thái Ngọc Duy wrote: In pack version 4, ref-delta technically could be used to compress any objects including commits and trees (both canonical and v4). But it does not make sense to do so. It can only lead to performance degradation. Catch those packers. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- I could now verify that pack-objects does not compress commits nor trees using ref-delta in v4. But perhaps these are a bit too strict? Maybe downgrade from die() to warning() and still accept the pack? Even then... There is a difference between an invalid pack and a suboptimal one. I don't think we should complain when presented with suboptimal encoding if it is still valid and there is no problem actually processing the data correctly. You never know when this alternative encoding, even if suboptimal, might be handy. Robustness principle: Be conservative in what you send, be liberal in what you accept. Nicolas
git/path.c - order of accessing ~/.gitconfig
* commit 21cf32279120799a766d22416be7d82d9ecfbd04 | | Author: Huynh Khoi Nguyen Nguyen huynh-khoi-nguyen.ngu...@ensimag.imag.fr | Date: Fri Jun 22 11:03:23 2012 +0200 | |config: read (but not write) from $XDG_CONFIG_HOME/git/config file | |Teach git to read the gitconfig information from a new location, |$XDG_CONFIG_HOME/git/config; this allows the user to avoid |cluttering $HOME with many per-application configuration files. | |In the order of reading, this file comes between the global |configuration file (typically $HOME/.gitconfig) and the system wide |configuration file (typically /etc/gitconfig). However git/config.c (git_config_early) commit accesses xdg_config before user_config. So the comments and documentation are inconsistent with the code. [This looks like an intentional bug, I spotted it when commenting out the accesses to files under ~/.config. (protip: chmod 000 ~/.config helps identify and blacklist NWO apps and now git started complaining) ---Madhu -- 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 2/3] relative_path should honor DOS and UNC paths
On 2013-09-18 16.37, Torsten Bögershausen wrote: On 2013-09-17 18.12, Junio C Hamano wrote: Jiang Xin worldhello@gmail.com writes: diff --git a/compat/mingw.h b/compat/mingw.h index bd0a88b..06e9f49 100644 --- a/compat/mingw.h +++ b/compat/mingw.h @@ -311,6 +311,15 @@ int winansi_fprintf(FILE *stream, const char *format, ...) __attribute__((format #define has_dos_drive_prefix(path) (isalpha(*(path)) (path)[1] == ':') #define is_dir_sep(c) ((c) == '/' || (c) == '\\') +static inline int is_unc_path(const char *path) +{ + if (!is_dir_sep(*path) || !is_dir_sep(*(path+1)) || is_dir_sep(*(path+2))) + return 0; If path[1] == '\0', it would be !is_dir_sep() and we end up inspecting past the end of the string? Yes (If there was a previous mail, it was incomplete, sorry) I think we want to catch 2 (back)slashes followed by a letter http://msdn.microsoft.com/en-us/library/windows/desktop/aa365247%28v=vs.85%29.aspx #define is_unc_path(path) ((is_dir_sep(path)[0]) is_dir_sep((path)[1]) isalpha((path[2]))) Then we need #define is_relative_path(path) (((path)[0]) !is_dir_sep((path)[1])) And may be like this: static int have_same_root(const char *path1, const char *path2) { int is_abs1, is_abs2; is_abs1 = is_absolute_path(path1); is_abs2 = is_absolute_path(path2); if (is_abs1 is_abs2) { if (is_unc_path(path1) !is_relative_path(path2)) return 0; if (!is_relative_path(path1) is_unc_path(path2)) return 0; return tolower(path1[0]) == tolower(path2[0]); } else { return !is_abs1 !is_abs2; } } Could that work? -- 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/path.c - order of accessing ~/.gitconfig
Madhu enom...@meer.net writes: * commit 21cf32279120799a766d22416be7d82d9ecfbd04 | | Author: Huynh Khoi Nguyen Nguyen huynh-khoi-nguyen.ngu...@ensimag.imag.fr | Date: Fri Jun 22 11:03:23 2012 +0200 | |config: read (but not write) from $XDG_CONFIG_HOME/git/config file | |Teach git to read the gitconfig information from a new location, |$XDG_CONFIG_HOME/git/config; this allows the user to avoid |cluttering $HOME with many per-application configuration files. | |In the order of reading, this file comes between the global |configuration file (typically $HOME/.gitconfig) and the system wide |configuration file (typically /etc/gitconfig). However git/config.c (git_config_early) commit accesses xdg_config before user_config. So the comments and documentation are inconsistent with the code. It seems the commit message is wrong, indeed. But it's too late to fix it. OTOH, the documentation seems right to me (Documentation/git-config.txt). It says: $(prefix)/etc/gitconfig:: System-wide configuration file. $XDG_CONFIG_HOME/git/config:: Second user-specific configuration file. If $XDG_CONFIG_HOME is not set or empty, `$HOME/.config/git/config` will be used. Any single-valued variable set in this file will be overwritten by whatever is in `~/.gitconfig`. It is a good idea not to create this file if you sometimes use older versions of Git, as support for this file was added fairly recently. ~/.gitconfig:: User-specific configuration file. Also called global configuration file. $GIT_DIR/config:: Repository specific configuration file. -- Matthieu Moy http://www-verimag.imag.fr/~moy/ -- 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] git clone -q ends with early EOF
This sounds suspiciously like 05e95155 (upload-pack: send keepalive packets during pack computation, 2013-09-08) that is cooking in 'next'. If you are talking with other people's server side, you are SOL until the server end gets the patch. -- 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 2/3] relative_path should honor DOS and UNC paths
Jiang Xin worldhello@gmail.com writes: 2013/9/18 Junio C Hamano gits...@pobox.com: + if (!is_dir_sep(*path) || !is_dir_sep(*(path+1)) || is_dir_sep(*(path+2))) + return 0; If path[1] == '\0', it would be !is_dir_sep() and we end up inspecting past the end of the string? The funciton is_unc_path will return false (0), if path is , /, //, ///three/slashes/, or /usr/local. So the problem is ? If path[1] == '\0' (e.g. path=/), !is_dir_sep(path[1]) is true, not false (as I misread earlier), so we hit an early return and will not peek path[2]. So no problem. Sorry for the noise. But I agree with J6t and Torsten in near-by thread that the simpler one that does not worry about // should be done as a separate patch and //, if we decide to do it, should build on top. Thanks. -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
git clone silently aborts if stdout gets a broken pipe
One of our Perl scripts that does a git clone suddenly started to fail when I upgraded to git 1.8.4 from 1.8.3.1. The failing Perl code used a construct like this: Git::command_oneline('clone', $url, $path); There is no error raised, but the directory specified by $path is not created. If I look at the process using strace I can see the clone taking place, but then it seems to get a broken pipe since the code above only cares about the first line from stdout (and with the addition of Checking connectivity... git clone now outputs two lines to stdout). If I change the code to: my @foo = Git::command('clone', $url, $path); it works as expected. I have attached a simple Perl script that shows the problem. Run it as clone_test.pl git url. With git 1.8.4 it will fail for the first two test cases, whereas with older git versions it succeeds for all four test cases. I hope this is enough information for someone to look into this regression. Best regards, //Peter clone_test.pl Description: clone_test.pl
[PATCH] completion: improve untracked directory filtering for filename completion
Similar to Bash's default filename completion, our git-aware filename completion stops at directory boundaries, i.e. it doesn't offer the full 'path/to/file' at first, but only 'path/'. To achieve that the completion script runs 'git ls-files' with specific command line options to get the list of relevant paths under the current directory, and then processes each path to strip all but the base directory or filename (see __git_index_files()). To offer only modified and untracked files for 'git add' the completion script runs 'git ls-files --exclude-standard --others --modified'. This command lists all non-ignored files in untracked directories, which leads to a noticeable delay caused by the processing mentioned above if there are a lot of such files (__git_index_files() specifies '--exclude-standard' internally): $ mkdir untracked-dir $ for i in {1..1} ; do untracked-dir/$i ; done $ time __git_index_files --others --modified untracked-dir real 0m0.537s user 0m0.452s sys 0m0.160s Eliminate this delay by additionally passing the '--directory --no-empty-directory' options to 'git ls-files' to show only the directory name of non-empty untracked directories instead their whole content: $ time __git_index_files --others --modified --directory --no-empty-directory untracked-dir real 0m0.029s user 0m0.020s sys 0m0.004s Filename completion for 'git clean' suffers from the same delay, as it offers untracked files, too. The fix could be the same, but since it actually makes sense to 'git clean' empty directories, in this case we only pass the '--directory' option to 'git ls-files'. Reported-by: Isaac Levy il...@google.com Signed-off-by: SZEDER Gábor sze...@ira.uka.de --- contrib/completion/git-completion.bash | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash index e1b7313072..86f77345fd 100644 --- a/contrib/completion/git-completion.bash +++ b/contrib/completion/git-completion.bash @@ -901,7 +901,7 @@ _git_add () esac # XXX should we check for --update and --all options ? - __git_complete_index_file --others --modified + __git_complete_index_file --others --modified --directory --no-empty-directory } _git_archive () @@ -1063,7 +1063,7 @@ _git_clean () esac # XXX should we check for -x option ? - __git_complete_index_file --others + __git_complete_index_file --others --directory } _git_clone () -- 1.8.4.366.g16e4e67 -- 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 pack v4: next step, help required
I think the pack v4 format and compatibility code is pretty stable now, and probably as optimal as it can reasonably be. @Junio: might be a good idea to refresh your pu branch again. Now the biggest problem to solve is the actual tree deltification. I don't have the time to spend on this otherwise very interesting problem (IMHO at least) so I'm sending this request for help in the hope that more people would be keen to contribute their computing skills to solve this challenge. I'll make a quick description of the pv4 tree encoding first and explain the problem to solve afterwards. A pv4 tree is comprised of two types of records: immediate tree entry and tree sequence copy. 1) Immediate Tree Entry This is made of the following tuple: path component index,{SHA1 reference index The path component index refers to the path dictionary table where path strings and file modes are uniquely stored. The SHA1 index refers to the sorted SHA1 table as previously found in the pack index but which is now part of the pack file itself. So on average a single tree entry may take between 1 to 2 bytes for the path component index and 1 to 3 bytes for the SHA1 index. 2) Tree Sequence Copy This is used to literally copy a contiguous list of tree entries from another tree object. This goes as follows: tree entry where to start,number of entries to copy [,SHA1 index of the object to copy from] So instead of having arbitrary copying of data like in delta objects, we refer directly to tree entries in another object. The big advantage here is that the tree walker may directly jump to the copied object without having to do any delta patching and caching. The SHA1 index is optional if it refers to the same copied object as the previous tree sequence copy record in this tree object. And another possible optimization for the tree walker when enumerating objects is to skip the copy entry entirely if the object being copied from has already been enumerated. The size of this entry is more variable and is typically between 1 to 2 bytes for the start index, 1 to 2 bytes for the copy size, and 0 to 3 bytes for the SHA1 index. So... what to do with this? Currently the deltification is achieved using a pretty dumb heuristic which is to simply take each tree object in a pack v2 with their corresponding base delta object and perform a straight conversion into pack v4 tree format i.e. use copy records whenever possible as long as they represent a space saving over the corresponding immediate tree entries (which is normally the case). However this is rather sub-optimal for two reasons: 1) This doesn't benefit from the ability to use multiple base objects to copy from and is potentially missing on additional opportunities for copy sequences which are not possible in the selected base object from the pack v2 delta base selection. Pack v4 is already quite smaller than pack v2 yet it might possibly be smaller still. 2) This makes deep delta chains very inefficient at runtime when the pack is subsequently accessed. Let's consider this example to fully illustrate #2. Tree A: entry 0:foo.txt entry 1-3: copy from tree B: start=2 count=3 Tree B: entry 0-5: copy from tree C: start=2 count=5 entry 6:bar.txt Tree C: entry 0:file_a entry 1:file_b entry 2:file_c entry 3:file_D entry 4:file_E entry 5:file_F entry 6:file_G This is a good example of what typically happens when the above heuristic is applied. And it is not uncommon to see a long indirection chain of copy those 2 entries from that other object sometimes reaching 50 levels deep where the same 2 (or more) entries require up to 50 object hops before they can be obtained. Obviously here the encoding should be optimized to reduce the chaining effect simply by referring to the final object directly. Hence tree A could be encoded as follows: Tree A: entry 0:foo.txt entry 1-3: copy from tree C: start=4 count=3 The on-disk encoding is likely to be the same size but the runtime access is greatly optimized. Now... instead of trying to do reference simplification by factoring out the chain effect, I think a whole new approach to tree delta compression for pack v4 should be developed which would also address issue #1 above. For example, we may try to make each canonical tree objects into sequences of hashed tree entries in memory where each tree entry would be reduced down to a single CRC32 value (or even Adler32 for speed). Each tree object would then be represented by some kind of 32-bit character string. Then it is just a matter of applying substring matching algorithms to find the best copy sequences without creating reference cycles, weighted by the number
Re: [PATCH] fetch-pack.c: show correct command name that fails
On Wed, Sep 18, 2013 at 11:16:15AM -0700, Junio C Hamano wrote: I wondered if this is something we can have a test for easily. Unlike the --upload-pack option, there is no way for the receiving end to choose which index-pack (or unpack-objects) to run, but we may force a failure by temporarily futzing with PATH to make a dummy git-index-pack that always fails to be found or something silly like that. You can feed a bogus pack, which will cause either to fail reliably. I'm not sure if that counts as easy though. :) Munging PATH is probably simpler. fetch-pack.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) Patch looks good to me, too. -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] fetch-pack.c: show correct command name that fails
Nguyễn Thái Ngọc Duy pclo...@gmail.com writes: When --shallow-file is added to the command line, it has to be before the subcommand name, the first argument won't be the command name any more. Stop assuming that and keep track of the command name explicitly. Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com --- Thanks; this dates back to v1.8.3.2 but it is at the end of an error path and only for reporting, so it may not be worth merging it down to the 1.8.3.x maintenance track. I wondered if this is something we can have a test for easily. Unlike the --upload-pack option, there is no way for the receiving end to choose which index-pack (or unpack-objects) to run, but we may force a failure by temporarily futzing with PATH to make a dummy git-index-pack that always fails to be found or something silly like that. fetch-pack.c | 10 +- 1 file changed, 5 insertions(+), 5 deletions(-) diff --git a/fetch-pack.c b/fetch-pack.c index 6684348..b259c51 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -688,7 +688,7 @@ static int get_pack(struct fetch_pack_args *args, const char *argv[22]; char keep_arg[256]; char hdr_arg[256]; - const char **av; + const char **av, *cmd_name; int do_keep = args-keep_pack; struct child_process cmd; int ret; @@ -735,7 +735,7 @@ static int get_pack(struct fetch_pack_args *args, if (do_keep) { if (pack_lockfile) cmd.out = -1; - *av++ = index-pack; + *av++ = cmd_name = index-pack; *av++ = --stdin; if (!args-quiet !args-no_progress) *av++ = -v; @@ -752,7 +752,7 @@ static int get_pack(struct fetch_pack_args *args, *av++ = --check-self-contained-and-connected; } else { - *av++ = unpack-objects; + *av++ = cmd_name = unpack-objects; if (args-quiet || args-no_progress) *av++ = -q; args-check_self_contained_and_connected = 0; @@ -770,7 +770,7 @@ static int get_pack(struct fetch_pack_args *args, cmd.in = demux.out; cmd.git_cmd = 1; if (start_command(cmd)) - die(fetch-pack: unable to fork off %s, argv[0]); + die(fetch-pack: unable to fork off %s, cmd_name); if (do_keep pack_lockfile) { *pack_lockfile = index_pack_lockfile(cmd.out); close(cmd.out); @@ -782,7 +782,7 @@ static int get_pack(struct fetch_pack_args *args, args-check_self_contained_and_connected ret == 0; else - die(%s failed, argv[0]); + die(%s failed, cmd_name); if (use_sideband finish_async(demux)) die(error in sideband demultiplexer); return 0; -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [BUG] git clone -q ends with early EOF
On Wed, Sep 18, 2013 at 02:44:18PM +0200, Marek Vasut wrote: I am trying to clone a repository and I am getting the following output: $ git clone -q git://kernel.ubuntu.com/ubuntu/linux.git fatal: The remote end hung up unexpectedly fatal: early EOF fatal: index-pack failed The fatal: lines usually appear a few minutes after running the clone. Of course, the clone does not finish successfully. Interestingly, when I drop the '-q' option from the git clone commandline, the clone finishes correctly. As Junio mentioned, this does seem like the issue that 05e9515 (upload-pack: send keepalive packets during pack computation, 2013-09-08) tries to fix. The server is quiet for a long period while preparing the pack, and something in the network stack thinks the connection has timed out and kills it. One way you can check this is by running: time git clone -q git://kernel.ubuntu.com/ubuntu/linux.git If it's a network timeout, it will die consistently at some nice round number. In this case, I just ran that command twice, and it died both times at just a hair over 2 minutes. So that is probably what is going on. There's no proxy on my end, so presumably there is some front-end reverse proxying happening on the server end, and it has a 2-minute timeout. The keepalive patch is not in any released version yet, but we have been running it in production at GitHub for a few weeks. You may want to file a support request with the Ubuntu folks asking to pick up the patch, or to increase the timeouts on their proxies. -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/path.c - order of accessing ~/.gitconfig
[I posted this via gmane (m3mwnac9ob@leonis4.robolove.meer.net) but the posting hasn't appeared, so I'm sending this by email..] * Matthieu Moy vpq8uyuxjfe@anie.imag.fr : Wrote on Wed, 18 Sep 2013 17:01:57 +0200: | * commit 21cf32279120799a766d22416be7d82d9ecfbd04 | |In the order of reading, this file comes between the global | |configuration file (typically $HOME/.gitconfig) and the system wide | |configuration file (typically /etc/gitconfig). | | However git/config.c (git_config_early) commit accesses xdg_config | before user_config. So the comments and documentation are | inconsistent with the code. | | It seems the commit message is wrong, indeed. But it's too late to fix | it. OTOH, the documentation seems right to me | (Documentation/git-config.txt). It says: No, The commit message is also right. xdg_config indeed comes betweeen git_etc_gitconfig and user_config, as the commit says, I misunderstood what the commit was saying because it says this in the reverse order. Sorry, (nothing to see here) ---Madhu -- 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] build: add default configuration
Apologies for top post -- anybody have a recommendation for a better app then maildroid? Will this not conflict with folks that supply their own gitconfig? I like the idea. Docs? Also, should this not be done in the C side so that we don't waste time reading the config, and also prevent users from overriding these? -Original Message- From: Felipe Contreras felipe.contre...@gmail.com To: git@vger.kernel.org Cc: git-us...@googlegroups.com, Bráulio Bhavamitra brauli...@gmail.com, Felipe Contreras felipe.contre...@gmail.com Sent: Tue, 17 Sep 2013 8:23 AM Subject: [PATCH] build: add default configuration For now simply add a few common aliases. co = checkout ci = commit rb = rebase st = status Signed-off-by: Felipe Contreras felipe.contre...@gmail.com --- Makefile | 5 - gitconfig | 5 + 2 files changed, 9 insertions(+), 1 deletion(-) create mode 100644 gitconfig diff --git a/Makefile b/Makefile index 3588ca1..18081bf 100644 --- a/Makefile +++ b/Makefile @@ -1010,7 +1010,7 @@ ifndef sysconfdir ifeq ($(prefix),/usr) sysconfdir = /etc else -sysconfdir = etc +sysconfdir = $(prefix)/etc endif endif @@ -1586,6 +1586,7 @@ template_dir_SQ = $(subst ','\'',$(template_dir)) htmldir_relative_SQ = $(subst ','\'',$(htmldir_relative)) prefix_SQ = $(subst ','\'',$(prefix)) gitwebdir_SQ = $(subst ','\'',$(gitwebdir)) +sysconfdir_SQ = $(subst ','\'',$(sysconfdir)) SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH)) PERL_PATH_SQ = $(subst ','\'',$(PERL_PATH)) @@ -2340,6 +2341,8 @@ install: all $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' $(INSTALL) -m 644 mergetools/* '$(DESTDIR_SQ)$(mergetools_instdir_SQ)' + $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(sysconfdir_SQ)' + $(INSTALL) -m 644 gitconfig '$(DESTDIR_SQ)$(ETC_GITCONFIG_SQ)' ifndef NO_GETTEXT $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(localedir_SQ)' (cd po/build/locale $(TAR) cf - .) | \ diff --git a/gitconfig b/gitconfig new file mode 100644 index 000..c45d300 --- /dev/null +++ b/gitconfig @@ -0,0 +1,5 @@ +[alias] + co = checkout + ci = commit + rb = rebase + st = status -- 1.8.4-fc -- 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 -- 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 clone silently aborts if stdout gets a broken pipe
On Wed, Sep 18, 2013 at 06:52:13PM +0200, Peter Kjellerstedt wrote: The failing Perl code used a construct like this: Git::command_oneline('clone', $url, $path); There is no error raised, but the directory specified by $path is not created. If I look at the process using strace I can see the clone taking place, but then it seems to get a broken pipe since the code above only cares about the first line from stdout (and with the addition of Checking connectivity... git clone now outputs two lines to stdout). I think your perl script is somewhat questionable, as it is making assumptions about the output of git-clone, and you would do better to accept arbitrary-sized output (or better yet, leave stdout pointing to the user, so they can see the output, which is meant for them). That being said, the new messages should almost certainly go to stderr. -- 8 -- Subject: [PATCH] clone: write checking connectivity to stderr In commit 0781aa4 (clone: let the user know when check_everything_connected is run, 2013-05-03), we started giving the user a progress report during clone. However, since the actual work happens in a sub-process, we do not use the usual progress code that counts the objects, but rather just print a message ourselves. This message goes to stdout via printf, which is unlike other progress messages (both the eye candy within clone, and the checking connectivity progress in other commands). Let's send it to stderr for consistency. Signed-off-by: Jeff King p...@peff.net --- builtin/clone.c | 4 ++-- 1 file changed, 2 insertions(+), 2 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index ca3eb68..3c91844 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -551,12 +551,12 @@ static void update_remote_refs(const struct ref *refs, if (check_connectivity) { if (0 = option_verbosity) - printf(_(Checking connectivity... )); + fprintf(stderr, _(Checking connectivity... )); if (check_everything_connected_with_transport(iterate_ref_map, 0, rm, transport)) die(_(remote did not send all necessary objects)); if (0 = option_verbosity) - printf(_(done\n)); + fprintf(stderr, _(done\n)); } if (refs) { -- 1.8.4.rc4.16.g228394f -- 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] build: add default configuration
On Wed, Sep 18, 2013 at 1:13 PM, David Aguilar dav...@gmail.com wrote: Apologies for top post -- anybody have a recommendation for a better app then maildroid? Will this not conflict with folks that supply their own gitconfig? You mean people that provide their own ETC_GITCONFIG? If you mean distributions, their packaging would override /etc/gitconfig, if you mean people that have already a /etc/gitconfig, packaging systems usually save the old one so they can solve the conflict manually (e.g. /etc/gitconfig.pacsave). So no, it would not conflict. If you mean people that have ~/.gitconfig, then absolutely not, because that one takes precedence. Alternatively, we could have a higher level configuration file (e.g. /usr/share/git/config), but I think that's overkill. I like the idea. Docs? Also, should this not be done in the C side so that we don't waste time reading the config, and also prevent users from overriding these? But we want them to be easily readable, and possibly allow distributions to easily modify them. -- Felipe Contreras -- 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: Locking files / git
On Tuesday, September 17, 2013 09:45:26 PM Nicolas Adenis-Lamarre wrote: Ooops. It seems that each time somebody says these two words together, people hate him, and he is scorned by friends and family. See the thread intend-to-edit flag for one implementation idea: http://git.661346.n2.nabble.com/intend-to-edit-flag-td7591127.html -- 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 clone silently aborts if stdout gets a broken pipe
On Wed, Sep 18, 2013 at 02:45:51PM -0400, Jeff King wrote: That being said, the new messages should almost certainly go to stderr. -- 8 -- Subject: [PATCH] clone: write checking connectivity to stderr In commit 0781aa4 (clone: let the user know when check_everything_connected is run, 2013-05-03), we started giving the user a progress report during clone. However, since the actual work happens in a sub-process, we do not use the usual progress code that counts the objects, but rather just print a message ourselves. This message goes to stdout via printf, which is unlike other progress messages (both the eye candy within clone, and the checking connectivity progress in other commands). Let's send it to stderr for consistency. Hrm, this actually breaks t5701, which expects clone 2err to print nothing to stderr. What should happen here? The message is emulating the usual progress messages, which are silent when stderr is redirected. So we could actually use isatty() in the usual way to suppress them. On the other hand, the point of that suppression is that the regular progress code produces long output that is not meant to be seen sequentially (i.e., it is overwritten in the terminal with \r). But this message does not do so. So we can just tweak t5701 to be more careful about what it is looking for. Also, we should arguably give the Cloning into... message the same treatment. We have printed that to stdout for a very long time, so there is a slim chance that somebody actually tries to parse it. But I think they are wrong to do so; we already changed it once (in 28ba96a), and these days it is internationalized, anyway. In May of 2012 I posted this patch, but it got overlooked, and I forgot about it but carried it in my tree since then. Maybe we should apply it now (it fixes t5701; the checking connectivity patch can come on top, or even just be squashed in). -- 8 -- Subject: [PATCH] clone: send diagnostic messages to stderr Putting messages like Cloning into.. and done on stdout is un-Unix and uselessly clutters the stdout channel. Send them to stderr. We have to tweak two tests to accommodate this: 1. t5601 checks for doubled output due to forking, and doesn't actually care where the output goes; adjust it to check stderr. 2. t5702 is trying to test whether progress output was sent to stderr, but naively does so by checking whether stderr produced any output. Instead, have it look for %, a token found in progress output but not elsewhere (and which lets us avoid hard-coding the progress text in the test). Signed-off-by: Jeff King p...@peff.net --- builtin/clone.c | 6 +++--- t/t5601-clone.sh | 2 +- t/t5702-clone-options.sh | 4 ++-- 3 files changed, 6 insertions(+), 6 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 3c91844..8723a3a 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -379,7 +379,7 @@ static void clone_local(const char *src_repo, const char *dest_repo) } if (0 = option_verbosity) - printf(_(done.\n)); + fprintf(stderr, _(done.\n)); } static const char *junk_work_tree; @@ -849,9 +849,9 @@ int cmd_clone(int argc, const char **argv, const char *prefix) if (0 = option_verbosity) { if (option_bare) - printf(_(Cloning into bare repository '%s'...\n), dir); + fprintf(stderr, _(Cloning into bare repository '%s'...\n), dir); else - printf(_(Cloning into '%s'...\n), dir); + fprintf(stderr, _(Cloning into '%s'...\n), dir); } init_db(option_template, INIT_DB_QUIET); write_config(option_config); diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh index 0629149..b3b11e6 100755 --- a/t/t5601-clone.sh +++ b/t/t5601-clone.sh @@ -36,7 +36,7 @@ test_expect_success C_LOCALE_OUTPUT 'output from clone' ' test_expect_success C_LOCALE_OUTPUT 'output from clone' ' rm -fr dst - git clone -n file://$(pwd)/src dst output + git clone -n file://$(pwd)/src dst output 21 test $(grep Clon output | wc -l) = 1 ' diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh index 85cadfa..67e170e 100755 --- a/t/t5702-clone-options.sh +++ b/t/t5702-clone-options.sh @@ -22,14 +22,14 @@ test_expect_success 'redirected clone -v' ' test_expect_success 'redirected clone' ' git clone file://$(pwd)/parent clone-redirected out 2err - test_must_be_empty err + ! grep % err ' test_expect_success 'redirected clone -v' ' git clone --progress file://$(pwd)/parent clone-redirected-progress \ out 2err - test -s err + grep % err ' -- 1.8.4.rc4.16.g228394f -- 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
suspected bug(s) in git-cvsexportcommit.perl
Hello. I don't believe that git-cvsexportcommit.perl is working properly. First off, invocations of git-apply fail on my system. git apply works. (The aforementioned script uses the former.) Second, please have a look at this output (specific strings have been replaced with 'foo') hobbes@metalbaby:~/src/foo$ git cvsexportcommit -v fecc8b4bb3d91d204f4eb3ebd112f6cec6004819 Applying to CVS commit fecc8b4bb3d91d204f4eb3ebd112f6cec6004819 from parent 695a544fbdcf7e0614c35d1dab9a3eac0cc57b4c Checking if patch will apply cvs status: nothing known about `WebContent/WEB-INF/lib/commons-lang-2.6.jar' cvs status: nothing known about `WebContent/WEB-INF/lib/commons-configuration-1.9.jar' cvs status: nothing known about `JavaSource/foo/ConfigManager.java' cvs status: nothing known about `JavaSource/config.xml' Huh? Status 'Unknown' reported for unexpected file 'no file commons-lang-2.6.jar' Huh? Status 'Unknown' reported for unexpected file 'no file commons-configuration-1.9.jar' Huh? Status 'Unknown' reported for unexpected file 'no file ConfigManager.java' Huh? Status 'Unknown' reported for unexpected file 'no file config.xml' Applying stdin:7: trailing whitespace. ?xml version=1.0 encoding=UTF-8 ? stdin:8: trailing whitespace. config stdin:9: trailing whitespace. masterthread stdin:10: trailing whitespace. queuemappings stdin:11: trailing whitespace. mapping threadtype=foo service=foo action=foo_test / error: patch failed: JavaSource/foo/QueueMapping.java:67 error: JavaSource/foo/QueueMapping.java: patch does not apply Context reduced to (2/2) to apply fragment at 43 cannot patch at /usr/lib/git-core/git-cvsexportcommit line 333. hobbes@metalbaby:~/src/foo$ Even after I altered my copy of the script to invoke 'git apply' in two places, I still get the erroneous unknown lines. My suspicion is that it has something to do with the $basename stuff, but, who am I kidding? I don't grok perl. I haven't yet explored the error lines after Applying. So, what do I want? I want somebody to test git-cvsexportcommit.perl. If it passes tests, then I'd like help creating a test that it does fail. (Perhaps my workspace can help. Perhaps it is as simple as the presence of a new file in a subdirectory.) Cheers, thanks, --Dave -- To unsubscribe from this list: send the line unsubscribe git in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 3/3] git submodule update should give notice when run without init beforehand
Am 18.09.2013 12:12, schrieb Tay Ray Chuan: On Tue, Sep 17, 2013 at 1:06 AM, Jens Lehmann jens.lehm...@web.de wrote: Thanks Jens for having a look! Am 15.09.2013 19:38, schrieb Tay Ray Chuan: When 'update' is run with no path in a repository with uninitialized submodules, the program terminates with no output, and zero status code. Be more helpful to users by mentioning this. [snip] it would be rather nasty to error out on every submodule update. Just to be sure we're on the right page, with this patch, the 'update' command still exits with status code zero (non-error), so this patch doesn't make it error out. Ok, sorry for the confusion. But I still think we should not change the default to print these messages, but make it an opt-in. And the commit message (and maybe the documentation too) should talk about the use cases where this makes sense. After the 'autoinit' configuration (which lets upstream hint that certain submodules should be initialized on clone) has materialzed we might want to enable this error for these specific submodules. That's cool, I'm looking forward to this. Could you point me to somewhere detailing this? Not yet. I'm still wrestling with the autoupdate series, which is the logical step before autoinit. autoupdate enables to configure that initialized submodules are updated on checkout, reset, merge and all the other work tree manipulating commands. autoinit then also clones the repos of submodules into .git/modules and inits them in .git/config, so that autoupdate will automagically populate them. Unfortunately autoupdate is a rather largish series touching lots of commands and needing tons of tests ... But in the meantime, on top of the advice.* config, how about having a submodule.name.ignoreUninit config to disable the message on a per-submodule basis? I'd not add such an option unless users request it because it helps their not-so-terribly-specific use case. But in any case the error message should contain a hint on how you can get rid of the error in case you know what you are doing ;-). The message does mention that you can throw in an --init to fix the problem. This hint is similar to what git-submodule prints when a path is passed (see region at line 807). But for a lot of users that isn't a solution, as they never want to init it, they want to ignore it. And if you explicitly ask to update a special submodule, that message is ok. -- 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 clone silently aborts if stdout gets a broken pipe
Jeff King p...@peff.net writes: On Wed, Sep 18, 2013 at 02:45:51PM -0400, Jeff King wrote: That being said, the new messages should almost certainly go to stderr. -- 8 -- Subject: [PATCH] clone: write checking connectivity to stderr In commit 0781aa4 (clone: let the user know when check_everything_connected is run, 2013-05-03), we started giving the user a progress report during clone. However, since the actual work happens in a sub-process, we do not use the usual progress code that counts the objects, but rather just print a message ourselves. This message goes to stdout via printf, which is unlike other progress messages (both the eye candy within clone, and the checking connectivity progress in other commands). Let's send it to stderr for consistency. Hrm, this actually breaks t5701, which expects clone 2err to print nothing to stderr. Hmm, where in t5701? Ah, you meant t5702 and possibly t5601. What should happen here? The message is emulating the usual progress messages, which are silent when stderr is redirected. So we could actually use isatty() in the usual way to suppress them. On the other hand, the point of that suppression is that the regular progress code produces long output that is not meant to be seen sequentially (i.e., it is overwritten in the terminal with \r). But this message does not do so. So we can just tweak t5701 to be more careful about what it is looking for. I actually think it is long and not meant to be seen sequentially is a bad classifier; these new messages are also progress report in that it reports we are now in this phase. So if I were to vote, I would say we should apply the same progress-silencing criteria, preferrably by not checking isatty() again, but by recording the decision we have already made when squelching the progress during the transfer in order to make sure they stay consistent. Also, we should arguably give the Cloning into... message the same treatment. We have printed that to stdout for a very long time, so there is a slim chance that somebody actually tries to parse it. But I think they are wrong to do so; we already changed it once (in 28ba96a), and these days it is internationalized, anyway. Good thinking. Please make it so ;-) -- 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] git clone -q ends with early EOF
Jeff King p...@peff.net writes: The keepalive patch is not in any released version yet, but we have been running it in production at GitHub for a few weeks. That is good to hear; I'd feel safer to bump the scheduled graduation date to 'master' for the topic in that case. Like tomorrow ;-) -- 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] clone: treat checking connectivity like other progress
When stderr does not point to a tty, we typically suppress we are now in this phase progress reporting (e.g., we ask the server not to send us counting objects and the like). The new checking connectivity message is in the same vein, and should be suppressed. Since clone relies on the transport code to make the decision, we can simply sneak a peek at the progress field of the transport struct. That properly takes into account both the verbosity and progress options we were given, as well as the result of isatty(). Note that we do not set up that progress flag for a local clone, as we do not fetch using the transport at all. That's acceptable here, though, because we also do not perform a connectivity check in that case. Signed-off-by: Jeff King p...@peff.net --- Though the last paragraph explains why this is OK, it feels a bit fragile. I wonder if we should hoist the call to transport_set_verbosity outside the !is_local conditional. I do not think it would hurt anything. builtin/clone.c | 4 ++-- t/t5702-clone-options.sh | 3 ++- 2 files changed, 4 insertions(+), 3 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 8723a3a..7c62298 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -550,12 +550,12 @@ static void update_remote_refs(const struct ref *refs, const struct ref *rm = mapped_refs; if (check_connectivity) { - if (0 = option_verbosity) + if (transport-progress) fprintf(stderr, _(Checking connectivity... )); if (check_everything_connected_with_transport(iterate_ref_map, 0, rm, transport)) die(_(remote did not send all necessary objects)); - if (0 = option_verbosity) + if (transport-progress) fprintf(stderr, _(done\n)); } diff --git a/t/t5702-clone-options.sh b/t/t5702-clone-options.sh index d3dbdfe..9e24ec8 100755 --- a/t/t5702-clone-options.sh +++ b/t/t5702-clone-options.sh @@ -22,7 +22,8 @@ test_expect_success 'redirected clone does not show progress' ' test_expect_success 'redirected clone does not show progress' ' git clone file://$(pwd)/parent clone-redirected out 2err - ! grep % err + ! grep % err + test_i18ngrep ! Checking connectivity err ' -- 1.8.4.rc4.16.g228394f -- 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 clone silently aborts if stdout gets a broken pipe
On Wed, Sep 18, 2013 at 12:31:23PM -0700, Junio C Hamano wrote: Hrm, this actually breaks t5701, which expects clone 2err to print nothing to stderr. Hmm, where in t5701? Ah, you meant t5702 and possibly t5601. Yes, sorry, I meant t5702. I actually think it is long and not meant to be seen sequentially is a bad classifier; these new messages are also progress report in that it reports we are now in this phase. So if I were to vote, I would say we should apply the same progress-silencing criteria, preferrably by not checking isatty() again, but by recording the decision we have already made when squelching the progress during the transfer in order to make sure they stay consistent. Unfortunately that decision is made in the transport code, not by clone itself. We can cheat and peek at transport-progress after initializing the transport. That would require some refactoring, though; we print Cloning into before setting up the transport. And we do not even tell the transport about our progress options if we are doing a local clone. If we wanted to _just_ suppress Checking connectivity (and not Cloning into...), that's a bit easier. And I could see an argument that the former is the only one that falls into the progress report category. Also, we should arguably give the Cloning into... message the same treatment. We have printed that to stdout for a very long time, so there is a slim chance that somebody actually tries to parse it. But I think they are wrong to do so; we already changed it once (in 28ba96a), and these days it is internationalized, anyway. Good thinking. Please make it so ;-) OK. I've squashed the use stderr patches into one, and added a patch on top to correctly check the progress flag. [1/2]: clone: send diagnostic messages to stderr [2/2]: clone: treat checking connectivity like other progress -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 3/2] clone: always set transport options
On Wed, Sep 18, 2013 at 04:06:50PM -0400, Jeff King wrote: Note that we do not set up that progress flag for a local clone, as we do not fetch using the transport at all. That's acceptable here, though, because we also do not perform a connectivity check in that case. Signed-off-by: Jeff King p...@peff.net --- Though the last paragraph explains why this is OK, it feels a bit fragile. I wonder if we should hoist the call to transport_set_verbosity outside the !is_local conditional. I do not think it would hurt anything. Actually, I think the option-setting in clone is a little bit broken. Mostly it is just making fragile assumptions that happen to be true (e.g., that fetching the ref list will never care about the progress flag), but there are some options that should be respected in both cases. I think we should do this on top. -- 8 -- Subject: [PATCH] clone: always set transport options A clone will always create a transport struct, whether we are cloning locally or using an actual protocol. In the local case, we only use the transport to get the list of refs, and then transfer the objects out-of-band. However, there are many options that we do not bother setting up in the local case. For the most part, these are noops, because they only affect the object-fetching stage (e.g., the --depth option). However, some options do have a visible impact. For example, giving the path to upload-pack via -u does not currently work for a local clone, even though we need upload-pack to get the ref list. We can just drop the conditional entirely and set these options for both local and non-local clones. Rather than keep track of which options impact the object versus the ref fetching stage, we can simply let the noops be noops (and the cost of setting the options in the first place is not high). The one exception is that we also check that the transport provides both a get_refs_list and a fetch method. We will now be checking the former for both cases (which is good, since a transport that cannot fetch refs would not work for a local clone), and we tweak the conditional to check for a fetch only when we are non-local. Signed-off-by: Jeff King p...@peff.net --- The diff is rather unreadable, but using show -b reveals the actual changes. builtin/clone.c| 30 ++ t/t5701-clone-local.sh | 4 2 files changed, 18 insertions(+), 16 deletions(-) diff --git a/builtin/clone.c b/builtin/clone.c index 7c62298..7ac677d 100644 --- a/builtin/clone.c +++ b/builtin/clone.c @@ -884,27 +884,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix) remote = remote_get(option_origin); transport = transport_get(remote, remote-url[0]); - if (!is_local) { - if (!transport-get_refs_list || !transport-fetch) - die(_(Don't know how to clone %s), transport-url); + if (!transport-get_refs_list || (!is_local !transport-fetch)) + die(_(Don't know how to clone %s), transport-url); - transport_set_option(transport, TRANS_OPT_KEEP, yes); + transport_set_option(transport, TRANS_OPT_KEEP, yes); - if (option_depth) - transport_set_option(transport, TRANS_OPT_DEPTH, -option_depth); - if (option_single_branch) - transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, 1); + if (option_depth) + transport_set_option(transport, TRANS_OPT_DEPTH, +option_depth); + if (option_single_branch) + transport_set_option(transport, TRANS_OPT_FOLLOWTAGS, 1); - transport_set_verbosity(transport, option_verbosity, option_progress); + transport_set_verbosity(transport, option_verbosity, option_progress); - if (option_upload_pack) - transport_set_option(transport, TRANS_OPT_UPLOADPACK, -option_upload_pack); + if (option_upload_pack) + transport_set_option(transport, TRANS_OPT_UPLOADPACK, +option_upload_pack); - if (transport-smart_options !option_depth) - transport-smart_options-check_self_contained_and_connected = 1; - } + if (transport-smart_options !option_depth) + transport-smart_options-check_self_contained_and_connected = 1; refs = transport_get_remote_refs(transport); diff --git a/t/t5701-clone-local.sh b/t/t5701-clone-local.sh index 7ff6e0e..c490368 100755 --- a/t/t5701-clone-local.sh +++ b/t/t5701-clone-local.sh @@ -134,4 +134,8 @@ test_expect_success 'cloning a local path with --no-local does not hardlink' ' ! repo_is_hardlinked force-nonlocal ' +test_expect_success 'cloning locally respects -u for fetching refs' ' + test_must_fail git clone
Re: [BUG] git clone -q ends with early EOF
Hi, Jeff King p...@peff.net writes: The keepalive patch is not in any released version yet, but we have been running it in production at GitHub for a few weeks. That is good to hear; I'd feel safer to bump the scheduled graduation date to 'master' for the topic in that case. Like tomorrow ;-) Thanks for explaining guys, I will try to poke the canonical guys with reference to this thread. Best regards, Marek Vasut -- 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: Locking files / git
On 09/18/2013 01:15 AM, Nicolas Adenis-Lamarre wrote: Ooops. It seems that each time somebody says these two words together, people hate him, and he is scorned by friends and family. However, - gitolite implement it (but gitolite is not git). No. It pretends to implement it, for people who absolutely must have something and are willing to play by the rules. Quoting from the doc [1], When git is used in a truly distributed fashion, locking is impossible. I wrote it as a sort of bandaid, and that is all it is. Implement is too kind a word. regards sitaram -- 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] shortlog: ignore commits with missing authors
From: Jeff King p...@peff.net Most of git's traversals are robust against minor breakages in commit data. For example, git log will still output an entry for a commit that has a broken encoding or missing author, and will not abort the whole operation. Shortlog, on the other hand, will die as soon as it sees a commit without an author, meaning that a repository with a broken commit cannot get any shortlog output at all. Let's downgrade this fatal error to a warning, and continue the operation. We simply ignore the commit and do not count it in the total (since we do not have any author under which to file it). Alternatively, we could output some kind of empty record to collect these bogus commits. It is probably not worth it, though; we have already warned to stderr, so the user is aware that such bogosities exist, and any placeholder we came up with would either be syntactically invalid, or would potentially conflict with real data. Signed-off-by: Jeff King p...@peff.net --- The real-world breakage I saw that caused this was not a missing author line, but rather a commit that was in proper utf8, but whose encoding header said utf16. We can tweak the test to reproduce the exact case by making the sed line: sed /^\$/iencoding utf16 commit.tmp broken.tmp But I think that may be less portable, as systems with iconv that does not understand utf16 might leave the text intact. Simply removing the author line, as the test below does, has the same effect. builtin/shortlog.c | 6 -- t/t4201-shortlog.sh | 16 2 files changed, 20 insertions(+), 2 deletions(-) diff --git a/builtin/shortlog.c b/builtin/shortlog.c index ae73d17..c226f76 100644 --- a/builtin/shortlog.c +++ b/builtin/shortlog.c @@ -127,9 +127,11 @@ void shortlog_add_commit(struct shortlog *log, struct commit *commit) author = buffer + 7; buffer = eol; } - if (!author) - die(_(Missing author: %s), + if (!author) { + warning(_(Missing author: %s), sha1_to_hex(commit-object.sha1)); + return; + } if (log-user_format) { struct pretty_print_context ctx = {0}; ctx.fmt = CMIT_FMT_USERFORMAT; diff --git a/t/t4201-shortlog.sh b/t/t4201-shortlog.sh index 5493500..4286699 100755 --- a/t/t4201-shortlog.sh +++ b/t/t4201-shortlog.sh @@ -172,4 +172,20 @@ test_cmp expect out' git shortlog HEAD~2.. out test_cmp expect out' +test_expect_success 'shortlog ignores commits with missing authors' ' + git commit --allow-empty -m normal + git commit --allow-empty -m soon-to-be-broken + git cat-file commit HEAD commit.tmp + sed /^author/d commit.tmp broken.tmp + commit=$(git hash-object -w -t commit --stdin broken.tmp) + git update-ref HEAD $commit + cat expect -\EOF + A U Thor (1): + normal + + EOF + git shortlog HEAD~2.. actual + test_cmp expect actual +' + test_done -- 1.8.4.rc4.16.g228394f -- 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: Locking files / git
On 09/18/2013 03:42 PM, Nicolas Adenis-Lamarre wrote: Thanks a lot for your answer. That's really good arguments that i was waiting for and that i have not get until now. My comprehension now : - it's not easy to maintain several versions of a binary file in parallel. So basically, it's not recommanded to have complex workflow for binary files. In case the project has a low number of binary files, it can be handle by simple communication, Yes. Since you mentioned gitolite in your original post, I assume you read this caution also in the doc [1]: Of course, locking by itself is not quite enough. You may still get into merge situations if you make changes in branches. For best results you should actually keep all the binary files in their own branch, separate from the ones containing source code. The point is that locking and distribution don't go together at all. The **core** of distributed VCS is the old coding on an airplane story. What if someone locks a file after I am in the air, and I manage to get in a good 4 hours of solid work? CVCSs can also get into this situation, but to a lesser extent, I think. At least you won't be able to commit! [1]: http://gitolite.com/gitolite/locking.html In case the project has a lot of binary files, a simple workflow with a centralized workflow is recommanded - git doesn't hate locks, it's just that it's not the layer to implement it because git is workflow independant. Locks depend on a centralized server which is directly linked to the workflow. I'm not trying to implement a such workflow. I'm just curious, reading a lot of things about git, and trying to understand what is sometimes called a limitation of git. It's not a limitation of git. It's a fundamental conflict between the idea of distributed and what locking necessitates. A simple line in the documentation to say that locking should be handled in the upper layer (and it's done for example in gitolite) because it's dependant of the workflow could help some people looking about that point. For people who don't realise how important the D in DVCS is, and assume some sort of a central server will always exist, this simple line won't do. You'd have to explain all of that. And for people who do understand it, it's not necessary :-) Thanks a lot for git. 2013/9/17 Fredrik Gustafsson iv...@iveqy.com: On Tue, Sep 17, 2013 at 09:45:26PM +0200, Nicolas Adenis-Lamarre wrote: Ooops. It seems that each time somebody says these two words together, people hate him, and he is scorned by friends and family. For the moment, i want a first feedback, an intermediate between locking is bad and ok, but i would prefer in the negativ answer something with arguments (Take CVS as an example of what not to do; if in doubt, make the exact opposite decision. is one), and in the positiv answer, good remarks about problems with my implementation that could make it better. So working with locks and text-files is generally stupid to do with git since you don't use git merging capabilities. Working with binary files in git is stupid because git doesn't handle them very well because they the deltas can't be calculated very good. It seems to me that if you need to do locking one of the above scenarios is true for you and you should not use git at all. However, there's always the case when you've a mixed project with both binary and text-files. In that case I believe Jeff gave an excellent answer. But think twice, are you using git in a sane way? Even a small binary file will result in a huge git repo if it's updated often and the project has a long history. -- Med vänliga hälsningar Fredrik Gustafsson tel: 0733-608274 e-post: iv...@iveqy.com -- 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 -- 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 pack v4: next step, help required
On Thu, Sep 19, 2013 at 12:40 AM, Nicolas Pitre n...@fluxnic.net wrote: I think the pack v4 format and compatibility code is pretty stable now, and probably as optimal as it can reasonably be. @Junio: might be a good idea to refresh your pu branch again. Now the biggest problem to solve is the actual tree deltification. I don't have the time to spend on this otherwise very interesting problem (IMHO at least) so I'm sending this request for help in the hope that more people would be keen to contribute their computing skills to solve this challenge. Just so we're clear as I'm involved a bit in packv4 series. I plan to make the test suite fully pass, then add v4 support to git protocol. After that I may look into adding multi-base tree support to index-pack/unpack-objects, and only then either look into this challenge or continue to add v4-aware tree walker to git. In short, you you are intereted in Nico's challenge, go ahead, it will not overlap with my work, at least in the next one or two months. -- 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] build: add default configuration
On Wed, Sep 18, 2013 at 1:13 PM, David Aguilar dav...@gmail.com wrote: Will this not conflict with folks that supply their own gitconfig? You mean people that provide their own ETC_GITCONFIG? If you mean distributions, their packaging would override /etc/gitconfig, if you mean people that have already a /etc/gitconfig, packaging systems usually save the old one so they can solve the conflict manually (e.g. /etc/gitconfig.pacsave). So no, it would not conflict. Yuck. Yes, that one. I package my own /etc/gitconfig (as we have long advertised as the way to do it) and asking users to manually fix up thousands of machines is a bad idea. Yes, thousands. We're much past 30,000 cores at the moment. I like the idea. Docs? Also, should this not be done in the C side so that we don't waste time reading the config, and also prevent users from overriding these? But we want them to be easily readable, and possibly allow distributions to easily modify them. In that case I take it back -- I dont like that approach. We want consistency, not divergence. This encourages the former. -- David -- 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