[PATCH 5/5] transport-helper: fix sync issue on crashes
From: Felipe Contreras When a remote helper crashes while pushing we should revert back to the state before the push, however, it's possible that `git fast-export` already finished its job, and therefore has exported the marks already. This creates a synchronization problem because from that moment on `git fast-{import,export}` will have marks that the remote helper is not aware of and all further commands fail (if those marks are referenced). The fix is to tell `git fast-export` to export to a temporary file, and only after the remote helper has finishes successfully, move to the final destination. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- t/t5801-remote-helpers.sh | 20 +++- transport-helper.c| 13 +++-- 2 files changed, 30 insertions(+), 3 deletions(-) diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh index 25fd2e7..aa3e223 100755 --- a/t/t5801-remote-helpers.sh +++ b/t/t5801-remote-helpers.sh @@ -220,6 +220,20 @@ test_expect_success 'push update refs failure' ' ) ' +clean_mark () { + cut -f 2 -d ' ' "$1" | + git cat-file --batch-check | + grep commit | + sort >$(basename "$1") +} + +cmp_marks () { + test_when_finished "rm -rf git.marks testgit.marks" && + clean_mark ".git/testgit/$1/git.marks" && + clean_mark ".git/testgit/$1/testgit.marks" && + test_cmp git.marks testgit.marks +} + test_expect_success 'proper failure checks for fetching' ' (GIT_REMOTE_TESTGIT_FAILURE=1 && export GIT_REMOTE_TESTGIT_FAILURE && @@ -232,7 +246,11 @@ test_expect_success 'proper failure checks for fetching' ' test_expect_success 'proper failure checks for pushing' ' (cd local && - test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all + git checkout -b crash master && + echo crash >>file && + git commit -a -m crash && + test_must_fail env GIT_REMOTE_TESTGIT_FAILURE=1 git push --all && + cmp_marks origin ) ' diff --git a/transport-helper.c b/transport-helper.c index c890db6..b468e4f 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -435,7 +435,7 @@ static int get_exporter(struct transport *transport, fastexport->argv[argc++] = data->signed_tags ? "--signed-tags=verbatim" : "--signed-tags=warn-strip"; if (data->export_marks) { - strbuf_addf(&tmp, "--export-marks=%s", data->export_marks); + strbuf_addf(&tmp, "--export-marks=%s.tmp", data->export_marks); fastexport->argv[argc++] = strbuf_detach(&tmp, NULL); } if (data->import_marks) { @@ -911,7 +911,16 @@ static int push_refs_with_export(struct transport *transport, if (finish_command(&exporter)) die("Error while running fast-export"); - return push_update_refs_status(data, remote_refs, flags); + if (push_update_refs_status(data, remote_refs, flags)) + return 1; + + if (data->export_marks) { + strbuf_addf(&buf, "%s.tmp", data->export_marks); + rename(buf.buf, data->export_marks); + strbuf_release(&buf); + } + + return 0; } static int push_refs(struct transport *transport, -- 1.9.2-459-g68773ac -- 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] transport-helper: propagate recvline() error pushing
From: Felipe Contreras It's cleaner, and will allow us to do something sensible on errors later. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 18 +++--- 1 file changed, 11 insertions(+), 7 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index f0d7fc7..e91bc9a 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -739,17 +739,22 @@ static int push_update_ref_status(struct strbuf *buf, return !(status == REF_STATUS_OK); } -static void push_update_refs_status(struct helper_data *data, +static int push_update_refs_status(struct helper_data *data, struct ref *remote_refs, int flags) { struct strbuf buf = STRBUF_INIT; struct ref *ref = remote_refs; + int ret = 0; + for (;;) { char *private; - if (recvline(data, &buf)) - exit(128); + if (recvline(data, &buf)) { + ret = 1; + break; + } + if (!buf.len) break; @@ -767,6 +772,7 @@ static void push_update_refs_status(struct helper_data *data, free(private); } strbuf_release(&buf); + return ret; } static int push_refs_with_push(struct transport *transport, @@ -847,8 +853,7 @@ static int push_refs_with_push(struct transport *transport, sendline(data, &buf); strbuf_release(&buf); - push_update_refs_status(data, remote_refs, flags); - return 0; + return push_update_refs_status(data, remote_refs, flags); } static int push_refs_with_export(struct transport *transport, @@ -906,8 +911,7 @@ static int push_refs_with_export(struct transport *transport, if (finish_command(&exporter)) die("Error while running fast-export"); - push_update_refs_status(data, remote_refs, flags); - return 0; + return push_update_refs_status(data, remote_refs, flags); } static int push_refs(struct transport *transport, -- 1.9.2-459-g68773ac -- 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] transport-helper: trivial cleanup
From: Felipe Contreras It's simpler to store the file names directly, and form the fast-export arguments only when needed, and re-use the same strbuf with a format. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 24 1 file changed, 12 insertions(+), 12 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index e91bc9a..c890db6 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -195,15 +195,9 @@ static struct child_process *get_helper(struct transport *transport) } else if (!strcmp(capname, "signed-tags")) { data->signed_tags = 1; } else if (starts_with(capname, "export-marks ")) { - struct strbuf arg = STRBUF_INIT; - strbuf_addstr(&arg, "--export-marks="); - strbuf_addstr(&arg, capname + strlen("export-marks ")); - data->export_marks = strbuf_detach(&arg, NULL); + data->export_marks = xstrdup(capname + strlen("export-marks ")); } else if (starts_with(capname, "import-marks")) { - struct strbuf arg = STRBUF_INIT; - strbuf_addstr(&arg, "--import-marks="); - strbuf_addstr(&arg, capname + strlen("import-marks ")); - data->import_marks = strbuf_detach(&arg, NULL); + data->import_marks = xstrdup(capname + strlen("import-marks ")); } else if (starts_with(capname, "no-private-update")) { data->no_private_update = 1; } else if (mandatory) { @@ -428,6 +422,8 @@ static int get_exporter(struct transport *transport, struct helper_data *data = transport->data; struct child_process *helper = get_helper(transport); int argc = 0, i; + struct strbuf tmp = STRBUF_INIT; + memset(fastexport, 0, sizeof(*fastexport)); /* we need to duplicate helper->in because we want to use it after @@ -438,10 +434,14 @@ static int get_exporter(struct transport *transport, fastexport->argv[argc++] = "--use-done-feature"; fastexport->argv[argc++] = data->signed_tags ? "--signed-tags=verbatim" : "--signed-tags=warn-strip"; - if (data->export_marks) - fastexport->argv[argc++] = data->export_marks; - if (data->import_marks) - fastexport->argv[argc++] = data->import_marks; + if (data->export_marks) { + strbuf_addf(&tmp, "--export-marks=%s", data->export_marks); + fastexport->argv[argc++] = strbuf_detach(&tmp, NULL); + } + if (data->import_marks) { + strbuf_addf(&tmp, "--import-marks=%s", data->import_marks); + fastexport->argv[argc++] = strbuf_detach(&tmp, NULL); + } for (i = 0; i < revlist_args->nr; i++) fastexport->argv[argc++] = revlist_args->items[i].string; -- 1.9.2-459-g68773ac -- 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] transport-helper: remove barely used xchgline()
From: Felipe Contreras It's only used once, we can just call the two functions inside directly. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 9 ++--- 1 file changed, 2 insertions(+), 7 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 86e1679..892107c 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -71,12 +71,6 @@ static int recvline(struct helper_data *helper, struct strbuf *buffer) return recvline_fh(helper->out, buffer, helper->name); } -static void xchgline(struct helper_data *helper, struct strbuf *buffer) -{ - sendline(helper, buffer); - recvline(helper, buffer); -} - static void write_constant(int fd, const char *str) { if (debug) @@ -307,7 +301,8 @@ static int set_helper_option(struct transport *transport, quote_c_style(value, &buf, NULL, 0); strbuf_addch(&buf, '\n'); - xchgline(data, &buf); + sendline(data, &buf); + recvline(data, &buf); if (!strcmp(buf.buf, "ok")) ret = 0; -- 1.9.2-459-g68773ac -- 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] remote-helpers: make recvline return an error
From: Felipe Contreras Instead of exiting directly, make it the duty of the caller to do so. Signed-off-by: Felipe Contreras Signed-off-by: Junio C Hamano --- transport-helper.c | 21 ++--- 1 file changed, 14 insertions(+), 7 deletions(-) diff --git a/transport-helper.c b/transport-helper.c index 892107c..f0d7fc7 100644 --- a/transport-helper.c +++ b/transport-helper.c @@ -58,7 +58,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, const char *name) if (strbuf_getline(buffer, helper, '\n') == EOF) { if (debug) fprintf(stderr, "Debug: Remote helper quit.\n"); - exit(128); + return 1; } if (debug) @@ -157,7 +157,8 @@ static struct child_process *get_helper(struct transport *transport) while (1) { const char *capname; int mandatory = 0; - recvline(data, &buf); + if (recvline(data, &buf)) + exit(128); if (!*buf.buf) break; @@ -302,7 +303,8 @@ static int set_helper_option(struct transport *transport, strbuf_addch(&buf, '\n'); sendline(data, &buf); - recvline(data, &buf); + if (recvline(data, &buf)) + exit(128); if (!strcmp(buf.buf, "ok")) ret = 0; @@ -374,7 +376,8 @@ static int fetch_with_fetch(struct transport *transport, sendline(data, &buf); while (1) { - recvline(data, &buf); + if (recvline(data, &buf)) + exit(128); if (starts_with(buf.buf, "lock ")) { const char *name = buf.buf + 5; @@ -558,7 +561,9 @@ static int process_connect_service(struct transport *transport, goto exit; sendline(data, &cmdbuf); - recvline_fh(input, &cmdbuf, name); + if (recvline_fh(input, &cmdbuf, name)) + exit(128); + if (!strcmp(cmdbuf.buf, "")) { data->no_disconnect_req = 1; if (debug) @@ -743,7 +748,8 @@ static void push_update_refs_status(struct helper_data *data, for (;;) { char *private; - recvline(data, &buf); + if (recvline(data, &buf)) + exit(128); if (!buf.len) break; @@ -969,7 +975,8 @@ static struct ref *get_refs_list(struct transport *transport, int for_push) while (1) { char *eov, *eon; - recvline(data, &buf); + if (recvline(data, &buf)) + exit(128); if (!*buf.buf) break; -- 1.9.2-459-g68773ac -- 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-rc0
Johan Herland writes: > On Fri, Apr 18, 2014 at 9:37 PM, Junio C Hamano wrote: >> An early preview release Git v2.0.0-rc0 is now available for >> testing at the usual places. > > This is supposed to have _all_ the v2.0 topics, correct? > > I'm unable to find the commit that actually _changes_ the default > prefix for "git svn" (as announced in Documentation/git-svn.txt and > the release notes for v1.8.5 and v1.9.0). Yes, I noticed that the topic has been in the release notes for a few cycles but the changes never came to my tree (perhaps review of the patch series never concluded?) at the beginning of this cycle, so dropped it from the release notes. > For reference, it was posted as patch 3/3 back in October: > http://thread.gmane.org/gmane.comp.version-control.git/232761/focus=235900 > > Very sorry for not discovering this earlier. Well, things happen X-<. I see the other two contained in the merge from Eric at 1668b7d78f81 (Merge git://git.bogomips.org/git-svn, 2013-10-16). Is the last one still viable? From my point of view, it would be best if Eric can take another look on it and throw me a pull request (and I have to remember to resurrect the entry in the release notes). Alternatively I could revert the "Warn about changing the default" for now and defer the topic to v2.1. Eric, what do you think? My preference is not to revert but at the same time I am hesitant to take a patch that was posted as RFC this late in the cycle without input from the area expert, 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
[L10N] Startup of Git 2.0.0 l10n round 1
Hi, Since Git v2.0.0-rc0 had already been released, it's time to start new round of git l10n. This time there are 45 new messages need to be translated since last update for v1.9.0: l10n: git.pot: v2.0.0 round 1 (45 new, 28 removed) Generate po/git.pot from v2.0.0-rc0 for git v2.0.0 l10n round 1. Signed-off-by: Jiang Xin You can get it from the usual place: https://github.com/git-l10n/git-po/ As how to update your XX.po and help to translate Git, please see "Updating a XX.po file" and other sections in “po/README" file. -- 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: [RFC/PATCHv3 3/3] Git 2.0: git svn: Set default --prefix='origin/' if --prefix is not given
Johan Herland wrote: > --- a/Documentation/git-svn.txt > +++ b/Documentation/git-svn.txt > @@ -86,14 +86,7 @@ COMMANDS > (refs/remotes/$remote/*). Setting a prefix is also useful > if you wish to track multiple projects that share a common > repository. > -+ > -NOTE: In Git v2.0, the default prefix will CHANGE from "" (no prefix) > -to "origin/". This is done to put SVN-tracking refs at > -"refs/remotes/origin/*" instead of "refs/remotes/*", and make them > -more compatible with how Git's own remote-tracking refs are organized > -(i.e. refs/remotes/$remote/*). You can enjoy the same benefits today, > -by using the --prefix option. > - > + By default, the prefix is set to 'origin/'. We should maintain a description of <=1.9 behavior in the manpage. Users on long-term-support systems are likely to continue using ancient git installations for some time (5-10 years, even?), but may come across the current documentation online. Otherwise the patch looks fine and I can push it up for Junio for 2.0-rc1. -- 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
How to recursively clean only those untracked files that are not ignored?
According to the help, without -x option git clean should let alone the ignored files, but it doesn't. [il@reallin test]$ cat .gitignore *.sar [il@reallin test]$ mkdir -p conf/sar && touch conf/sar/aaa.sar [il@reallin test]$ git status # On branch master nothing to commit, working directory clean [il@reallin test]$ git clean -df Removing conf/ conf/sar/aaa.sar is removed. I already asked this http://stackoverflow.com/questions/23148736/git-clean-removes-ignored-files-by-default Someone even replied that "git does exactly what documentation says". Well, maybe, but I have doubts that the combination '-df' (without '-x') is useful at all. If someone wanted to delete ignored files, he would use '-x' or '-X'. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH] git-svn.txt: Retain a description og pre-v2.0 default prefix
Add a description of <=1.9 behavior in the manpage. Users on long-term-support systems are likely to continue using ancient git installations for some time (5-10 years, even?), but may come across the current documentation online. Suggested-by: Eric Wong Signed-off-by: Johan Herland --- > We should maintain a description of <=1.9 behavior in the manpage. > Users on long-term-support systems are likely to continue using ancient > git installations for some time (5-10 years, even?), but may come across > the current documentation online. Feel free to add/squash this on top. > Otherwise the patch looks fine and I can push it up for Junio for > 2.0-rc1. Thanks! ...Johan Documentation/git-svn.txt | 5 + 1 file changed, 5 insertions(+) diff --git a/Documentation/git-svn.txt b/Documentation/git-svn.txt index 3a7dd80..5b3c38d 100644 --- a/Documentation/git-svn.txt +++ b/Documentation/git-svn.txt @@ -87,6 +87,11 @@ COMMANDS if you wish to track multiple projects that share a common repository. By default, the prefix is set to 'origin/'. ++ +NOTE: Before Git v2.0, the default prefix was "" (no prefix). This +meant that SVN-tracking refs were put at "refs/remotes/*", which is +incompatible with how Git's own remote-tracking refs are organized. + --ignore-paths=;; When passed to 'init' or 'clone' this regular expression will be preserved as a config key. See 'fetch' for a description -- 1.9.1.587.g6ba9303 -- 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: Please allow to use gpgsm to support X.509 certificates
Dear Git community last night, brian m. Carlson explained, that "Git wants a key that can be used by GnuPG" and therefore X.509 certificates are not supported. As you probably know, since 3 years gpg supports X.509 - unfortunately, gpg does not automatically detect X.509 certificates and we have to use gpgsm instead of gpg. The good thing: for identical functions, the command line arguments are identical :-) Therefore: please allow to configure git, so that it can use gpg or gpgsm. Or even better: if gpg fails, then please automatically try gpgsm :-) It works perfectly, I just replaced gpg.exe by gpgsm.exe: 1. Copied all missing *.dll and *.exe from c:\Program Files (x86)\GNU\GnuPG\ to c:\Program Files (x86)\Git\bin\ 2. renamed c:\Program Files (x86)\Git\bin\gpg.exe to c:\Program Files (x86)\Git\bin\gpg_.exe 3. renamed c:\Program Files (x86)\Git\bin\gpgsm.exe to c:\Program Files (x86)\Git\bin\gpg.exe 4. Imported the X.509 Certificate 5. signed a commit: $ git commit -S -m 'Test commit of foo' gpgsm: DBG: adding certificates at level -2 gpgsm: signature created [master dd5145a] Test commit of foo 1 file changed, 0 insertions(+), 0 deletions(-) create mode 100644 test 6. Tested the signature $ git log --show-signature commit dd5145aabac18f6a2fb2cd0d4a30b5064ef4c04a gpgsm: Signature made 2014-04-19 10:34:53 using certificate ID 0x12345678^M gpgsm: Good signature from "/CN=xxx/O=xxx/L=xxxl/ST=xxx/C=xx/EMail=x...@xxx.xx"^M Author: tom x...@xxx.xx Date: Sat Apr 19 12:34:53 2014 +0200 Test commit of foo commit b89934b6e3a86343be740f7a5a1fe446e572b5dd Author: tom x...@xxx.xx Date: Fri Apr 18 23:09:47 2014 +0200 Init Thanks a lot for this really great tool!! Kind regards, Tom On Fri, Apr 18, 2014 at 10:04:50PM +0200, Thomas Schittli wrote: > We already have trusted Certificates from a CA. Can we use them > instead of an additional PGP key? Git wants a key that can be used by GnuPG, and X.509 certificates can't be. It invokes the gpg binary that's in your path, so X.509 integration isn't possible unless gpg learns about it. > We already have: > - s/mime certificate > - web server ssl/tls certificate > - XMPP Jabber ssl/tls certificate > - Object Code Signing certificate > > Or if we have to use a new pgp key: can we sign it using any of our > certificates? Only in the sense that you can sign any arbitrary piece of text or data with your certificates. -- 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-- 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-svn.txt: Retain a description og pre-v2.0 default prefix
Johan Herland wrote: > Feel free to add/squash this on top. Thanks! Squashed and pushed. The following changes since commit cc291953df19aa4a97bee3590e708dc1fc557500: Git 2.0-rc0 (2014-04-18 11:21:43 -0700) are available in the git repository at: git://bogomips.org/git-svn.git master for you to fetch changes up to fe191fcaa58cb785c804465a0da9bcba9fd9e822: Git 2.0: git svn: Set default --prefix='origin/' if --prefix is not given (2014-04-19 11:30:13 +) Johan Herland (1): Git 2.0: git svn: Set default --prefix='origin/' if --prefix is not given Documentation/git-svn.txt| 20 ++--- git-svn.perl | 12 +- t/t9107-git-svn-migrate.sh | 54 t/t9114-git-svn-dcommit-merge.sh | 4 +- t/t9116-git-svn-log.sh | 46 ++-- t/t9117-git-svn-init-clone.sh| 16 +++ t/t9118-git-svn-funky-branch-names.sh| 20 - t/t9120-git-svn-clone-with-percent-escapes.sh| 14 +++--- t/t9125-git-svn-multi-glob-branch-names.sh | 6 +-- t/t9128-git-svn-cmd-branch.sh| 18 t/t9135-git-svn-moved-branch-empty-file.sh | 2 +- t/t9141-git-svn-multiple-branches.sh | 28 ++-- t/t9145-git-svn-master-branch.sh | 2 +- t/t9155-git-svn-fetch-deleted-tag.sh | 4 +- t/t9156-git-svn-fetch-deleted-tag-2.sh | 6 +-- t/t9161-git-svn-mergeinfo-push.sh| 22 +- t/t9163-git-svn-reset-clears-caches.sh | 4 +- t/t9165-git-svn-fetch-merge-branch-of-branch.sh | 2 +- t/t9166-git-svn-fetch-merge-branch-of-branch2.sh | 2 +- t/t9167-git-svn-cmd-branch-subproject.sh | 2 +- 20 files changed, 131 insertions(+), 153 deletions(-) -- 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] WinGit - native x86/x64 Git for Windows
> So the reason for this new package is that you need 64bit binaries? That's the most important bit. Plus, weird ssh transfer speeds [1] caused by ansient openssh bundled in msysgit. > I can see the need for a pure Windows solution (no msys tools at least for > runtime). Several Git scripts are written in perl, many in shell and a couple even in tcl/tk (oh, my). Until this is true, Git requires unix-like userland environment: all those sh, awk, coreutils, findutils and others. > But this sounds to me that the only thing you changed is the compiler and > 64bit? That would be true *if* msysgit was really msys + mingw-built-git. But in practice, msysgit is: 1) outdated msys that was patched in multiple ways without sending patches upstream 2) heavily patched git, again not upstreamed To be honest, msys isn't a great tool. After all, it's just outdated and heavily patched cygwin. There exists msys2 project (much less outdated and much less patched cygwin). So, msysgit is an (outdated patched)*2 cygwin + patched git. > What is the reason of using a closed source compiler? It happened to be already installed on my box. Switching to another one will require just minor tweaks to my build script. I don't have any strong reasons for using MSVC. > Sorry if I am a little bit skeptic, but I am wondering whether it does make > sense for you to join forces with msysgit instead of creating a fork? 1) It makes sense to purge msysgit and start over. See mingwGitDevEnv [2] (by msysgit developer). 2) I only used msys due to my unawareness of msys2 at the time of initial WinGit hacking. Due to massive Unicode-related msys troubles, ansient perl and svn, I plan to switch to msys2 soon. > there are no 64 bit binaries shipped with msysgit is that nobody needed them That's wrong. Google for 'windows x64 git' or 'msysgit x64'. People need it. There's even an issue [3] (stalled several years ago) in msysgit tracker. After all, I needed it. [1] https://github.com/msysgit/msysgit/issues/31 [2]: https://github.com/sschuberth/mingwGitDevEnv [3]: http://code.google.com/p/msysgit/issues/detail?id=396
[no subject]
T0 get your entitlement, write me via; siedm...@outlook.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: Git: Please allow to use gpgsm to support X.509 certificates
On Sat, Apr 19, 2014 at 11:03:07AM +, Schittli Thomas wrote: > last night, brian m. Carlson explained, that "Git wants a key that can > be used by GnuPG" and therefore X.509 certificates are not supported. > > As you probably know, since 3 years gpg supports X.509 - > unfortunately, gpg does not automatically detect X.509 certificates > and we have to use gpgsm instead of gpg. > The good thing: for identical functions, the command line arguments are > identical :-) > > Therefore: please allow to configure git, so that it can use gpg or gpgsm. > Or even better: if gpg fails, then please automatically try gpgsm :-) Have you tried `git config gpg.program gpgsm`? -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
Hi, On Sat, 19 Apr 2014, Heiko Voigt wrote: > On Thu, Apr 03, 2014 at 05:18:50PM +0400, ma...@slonopotamus.org wrote: > > I'm proud to announce WinGit: > > an attempt to bring Git powers to 64-bit Windows. > > So the reason for this new package is that you need 64bit binaries? > > > Relationship with msysgit > > = > > > > Unlike msysgit, WinGit is a pure-Windows binary build with MSVC. Marat, please do not add to the confusion. "msysGit" is the name of the *development environment* for developing Git for Windows. It also brings facilities to use MSVC instead of GCC. So do not compare WinGit to msysgit (that would be like comparing Git to GCC). Compare WinGit to Git for Windows (and clarify that you mean a different WinGit than the old name of Git for Windows). > > Like msysgit, WinGit also uses msys environment (sh/perl/etc) both > > during build-time and runtime. So it is not purely 64-bit, because MSys is not. > I can see the need for a pure Windows solution (no msys tools at least for > runtime). But this sounds to me that the only thing you changed is the > compiler and 64bit? The git binaries in msysgit are already pure Windows > binaries with no need of msys.dll. The only reason why so many other > tools are shipped with msysgit is to run scripted commands (e.g. like > gitk or rebase). > > What is the reason of using a closed source compiler? Why not use the > 64bit mingw that is already used to build the 64bit explorer extension > to package 64bit binaries along with the 32bit ones in the installer? > > Sorry if I am a little bit skeptic, but I am wondering whether it does > make sense for you to join forces with msysgit instead of creating a > fork? I think the main reason why there are no 64 bit binaries shipped > with msysgit is that nobody needed them and the need to ship both (at > least for some time). We do have a facility to build 64-bit binaries with msysGit. It is even dirt-easy: just run the two release scripts in /src/mingw-w64/, and then build Git with "make W64=1". The real reason why Git for Windows does not ship 64-bit binaries is that they did not pass the test suite last time I tried. And for the record: I would have welcome contributions to the Git for Windows project. I still will. After all, there is no reason for yet another fork. Ciao, Johannes -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
On Sat, Apr 19, 2014 at 05:24:33PM +0200, Johannes Schindelin wrote: > Marat, please do not add to the confusion. "msysGit" is the name of the > *development environment* for developing Git for Windows. This confusion comes from the fact that major part of msysGit is packaged with Git for Windows to be used at runtime. If you insist on msysGit-is-a-development-environment, you have to admit that msysGit is technically a fork of msys. My approach undoes this fork step and uses upstream runtime environment as-is, be it msys, msys2, Cygwin or even SUA [1]. I could even make it a noop and say "dear user, I don't care how, but please put sh/awk/find/etc on PATH to make Git work, like things normally happen in *nix world". Actually, even if Git was pure C, things like `git filter-branch` would be almost useless without coreutils & friends. > After all, there is no reason for yet another fork. If there wasn't, mingwGitDevEnv would not be started. I'd say I am doing a 'rebase' instead of 'fork' by using codebase of Git for Windows (upstream Git sources with Windows-specific patches) but replacing msysGit-provided runtime environment with another one. [1]: http://en.wikipedia.org/wiki/Windows_Services_for_UNIX -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
Am 19.04.2014 15:35, schrieb Marat Radchenko: >> So the reason for this new package is that you need 64bit >> binaries? > > That's the most important bit. Plus, weird ssh transfer speeds [1] > caused by ansient openssh bundled in msysgit. Yes the msysgit openssh is ancient and slow. Openssh in mingGitDevEnv has proper speeds, see [1]. > 1) outdated msys that was patched in multiple ways without sending patches > upstream > 2) heavily patched git, again not upstreamed You do realize how much work it is to send something upstream? And in the case of mingw patches have been sent upstream which are still either being reviewed or are just in a (presumably *very* long) queue. >> Sorry if I am a little bit skeptic, but I am wondering whether it >> does make sense for you to join forces with msysgit instead of >> creating a fork? > > 1) It makes sense to purge msysgit and start over. See mingwGitDevEnv > [2] (by msysgit developer). I would be happy to see you contributing to the mingGitDevEnv project. > 2) I only used msys due to my > unawareness of msys2 at the time of initial WinGit hacking. Due to > massive Unicode-related msys troubles, ansient perl and svn, I plan > to switch to msys2 soon. > >> there are no 64 bit binaries shipped with msysgit is that nobody >> needed them > > That's wrong. Google for 'windows x64 git' or 'msysgit x64'. People > need it. There's even an issue [3] (stalled several years ago) in > msysgit tracker. After all, I needed it. > > [1] https://github.com/msysgit/msysgit/issues/31 [2]: > https://github.com/sschuberth/mingwGitDevEnv [3]: > http://code.google.com/p/msysgit/issues/detail?id=396 Actually I would need a 64bit git for windows too. The problem here is that of course everyone likes to have it, but nobody so desperatley what he/she will start to make the test suite pass. And before the test suite passes I don't think it is a good idea to use it in production. So for my part I decided to first get mingwGitDevEnv going and then start thinking about a 64bit version. [1]: https://github.com/sschuberth/mingwGitDevEnv/pull/5#issuecomment-15128748 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [msysGit] Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
Hi Marat, On Sat, 19 Apr 2014, Marat Radchenko wrote: > But in practice, msysgit is: > 1) outdated msys that was patched in multiple ways without > sending patches upstream > 2) heavily patched git, again not upstreamed Again, this time explicitly: I wish you had done a little more research on the matter, and I wish you had participated in the msysGit community before going on to reinvent the wheel. I have nothing per se against your effort, of course, you can spend your time as you want, but please refrain from claiming things that are provably incorrect. Ciao, Johannes -- 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] l10n: de.po: translate 45 new messages
Translate 45 new messages came from git.pot update in 5e078fc (l10n: git.pot: v2.0.0 round 1 (45 new, 28 removed)). Signed-off-by: Ralf Thielow --- po/de.po | 123 +-- 1 file changed, 49 insertions(+), 74 deletions(-) diff --git a/po/de.po b/po/de.po index 59acf20..e3088db 100644 --- a/po/de.po +++ b/po/de.po @@ -436,13 +436,13 @@ msgstr[1] "vor %lu Jahren" #, c-format msgid "failed to read orderfile '%s'" msgstr "Fehler beim Lesen der Reihenfolgedatei '%s'." #: diffcore-rename.c:517 msgid "Performing inexact rename detection" -msgstr "" +msgstr "Führe Erkennung für ungenaue Umbenennung aus" #: diff.c:113 #, c-format msgid " Failed to parse dirstat cut-off percentage '%s'\n" msgstr "" " Fehler beim Parsen des abgeschnittenen \"dirstat\" Prozentsatzes '%s'\n" @@ -964,29 +964,32 @@ msgid "" "Perhaps you forgot to add either ':/' or '.' ?" msgstr "" ":(exclude) Muster, aber keine anderen Pfadspezifikationen angegeben.\n" "Vielleicht haben Sie vergessen entweder ':/' oder '.' hinzuzufügen?" #: progress.c:224 -#, fuzzy msgid "done" -msgstr "Fertig\n" +msgstr "Fertig" #: read-cache.c:1238 #, c-format msgid "" "index.version set, but the value is invalid.\n" "Using version %i" msgstr "" +"index.version gesetzt, aber Wert ungültig.\n" +"Verwende Version %i" #: read-cache.c:1248 #, c-format msgid "" "GIT_INDEX_VERSION set, but the value is invalid.\n" "Using version %i" msgstr "" +"GIT_INDEX_VERSION gesetzt, aber Wert ungültig.\n" +"Verwende Version %i" #: remote.c:758 #, c-format msgid "Cannot fetch both %s and %s to %s" msgstr "Kann 'fetch' nicht für sowohl %s als auch %s nach %s ausführen." @@ -1400,15 +1403,14 @@ msgstr "Konnte git-Verweis %s nicht erstellen" #: submodule.c:1132 #, c-format msgid "Could not set core.worktree in %s" msgstr "Konnte core.worktree in '%s' nicht setzen." #: unpack-trees.c:206 -#, fuzzy msgid "Checking out files" -msgstr "Prüfe Patch %s..." +msgstr "Checke Dateien aus" #: urlmatch.c:120 msgid "invalid URL scheme name or missing '://' suffix" msgstr "Ungültiges URL-Schema oder Suffix '://' fehlt" #: urlmatch.c:144 urlmatch.c:297 urlmatch.c:356 @@ -1554,55 +1556,47 @@ msgstr "von beiden hinzugefügt:" #: wt-status.c:264 msgid "both modified:" msgstr "von beiden geändert:" #: wt-status.c:266 -#, fuzzy, c-format +#, c-format msgid "bug: unhandled unmerged status %x" -msgstr "Fehler: unbehandelter Differenz-Status %c" +msgstr "Fehler: unbehandelter Unmerged-Status %x" #: wt-status.c:274 -#, fuzzy msgid "new file:" -msgstr "neue Datei" +msgstr "neue Datei:" #: wt-status.c:276 -#, fuzzy msgid "copied:" -msgstr "kopiert" +msgstr "kopiert:" #: wt-status.c:278 -#, fuzzy msgid "deleted:" -msgstr "gelöscht" +msgstr "gelöscht:" #: wt-status.c:280 -#, fuzzy msgid "modified:" -msgstr "geändert" +msgstr "geändert:" #: wt-status.c:282 -#, fuzzy msgid "renamed:" -msgstr "umbenannt" +msgstr "umbenannt:" #: wt-status.c:284 -#, fuzzy msgid "typechange:" -msgstr "Typänderung" +msgstr "Typänderung:" #: wt-status.c:286 -#, fuzzy msgid "unknown:" -msgstr "unbekannt" +msgstr "unbekannt:" #: wt-status.c:288 -#, fuzzy msgid "unmerged:" -msgstr "nicht zusammengeführt" +msgstr "nicht gemerged:" #: wt-status.c:370 msgid "new commits, " msgstr "neue Commits, " #: wt-status.c:372 @@ -3543,13 +3537,12 @@ msgstr "setzt HEAD zu benanntem Commit" #: builtin/checkout.c:1096 msgid "set upstream info for new branch" msgstr "setzt Informationen zum Upstream-Branch für den neuen Branch" #: builtin/checkout.c:1098 -#, fuzzy msgid "new-branch" msgstr "neuer Branch" #: builtin/checkout.c:1098 msgid "new unparented branch" msgstr "neuer Branch ohne Eltern-Commit" @@ -3791,22 +3784,20 @@ msgstr "löscht nur ignorierte Dateien" #: builtin/clean.c:903 msgid "-x and -X cannot be used together" msgstr "Die Optionen -x und -X können nicht gemeinsam verwendet werden." #: builtin/clean.c:907 -#, fuzzy msgid "" "clean.requireForce set to true and neither -i, -n, nor -f given; refusing to " "clean" msgstr "" "clean.requireForce auf \"true\" gesetzt und weder -i, -n noch -f gegeben; " "\"clean\" verweigert" #: builtin/clean.c:910 -#, fuzzy msgid "" "clean.requireForce defaults to true and neither -i, -n, nor -f given; " "refusing to clean" msgstr "" "clean.requireForce standardmäßig auf \"true\" gesetzt und weder -i, -n noch -" "f gegeben; \"clean\" verweigert" @@ -4419,13 +4410,12 @@ msgstr "" #: builtin/commit.c:1129 msgid "Clever... amending the last one with dirty index." msgstr "" "Klug... den letzten Commit mit einer geänderten Staging-Area nachbessern." #: builtin/commit.c:1131 -#, fuzzy msgid "Explicit paths specified without -i or -o; assuming --only paths..." msgstr "" "Explizite Pfade ohne -i oder -o angegeben; unter der Annahme von --only " "Pfaden..." #: builtin/commit.c:1143 builtin/tag.c:639 @@ -4594,1
AW: Git: Please allow to use gpgsm to support X.509 certificates
Hi John, > Have you tried `git config gpg.program gpgsm`? wau, thanks a lot for this hint!, it works :-) I think a "seamless" integration for all certificate-types would be better, but I try to motivate GnuPG to merge the function of gpgsm.exe into gpg.exe. This would give a great benefit for all applications using GnuPG. Thanks a lot, kind regards, Tom Von: git-ow...@vger.kernel.org [git-ow...@vger.kernel.org]" im Auftrag von "John Keeping [j...@keeping.me.uk] Gesendet: Samstag, 19. April 2014 17:19 An: Schittli Thomas Cc: git@vger.kernel.org Betreff: Re: Git: Please allow to use gpgsm to support X.509 certificates On Sat, Apr 19, 2014 at 11:03:07AM +, Schittli Thomas wrote: > last night, brian m. Carlson explained, that "Git wants a key that can > be used by GnuPG" and therefore X.509 certificates are not supported. > > As you probably know, since 3 years gpg supports X.509 - > unfortunately, gpg does not automatically detect X.509 certificates > and we have to use gpgsm instead of gpg. > The good thing: for identical functions, the command line arguments are > identical :-) > > Therefore: please allow to configure git, so that it can use gpg or gpgsm. > Or even better: if gpg fails, then please automatically try gpgsm :-) Have you tried `git config gpg.program gpgsm`? -- 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: Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
On Sat, Apr 19, 2014 at 05:35:07PM +0400, Marat Radchenko wrote: > > there are no 64 bit binaries shipped with msysgit is that nobody > > needed them > > That's wrong. Google for 'windows x64 git' or 'msysgit x64'. People > need it. There's even an issue [3] (stalled several years ago) in > msysgit tracker. After all, I needed it. Do not get me wrong. I was saying until now nobody "needed" it in a way that he/she would do something about it. Of course there are many people requesting it, but I just do not count those people anymore. You are clearly doing something about it and thats great, I like it. What I am trying to achieve here is that you join the msysgit community. There already is just a very small amount of developers on the windows side (compared to upstream). We should try to all work together. I think it will greatly add to confusion if we have another installer of Git for Windows. I think the msysgit community is quite open for changes like the ones you are trying to achieve. Regarding mingwGitDevEnv[2]: That is a project started by Sebastian who also contributes to msysgit (and Git for Windows). It eventually can (and probably should) be a successor of the current situation where we always manually patch, build and commit our binaries into a git repository. A proper package management system would greatly help here. But AFAIK its not ready for production use yet. I guess Sebastian would not mind contributions. Cheers Heiko > [1] https://github.com/msysgit/msysgit/issues/31 > [2]: https://github.com/sschuberth/mingwGitDevEnv > [3]: http://code.google.com/p/msysgit/issues/detail?id=396 -- 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 01/11] refs.c: constify the sha arguments for ref_transaction_create|delete|update
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > ref_transaction_create|delete|update has no need to modify the sha1 > arguments passed to it so it should use const unsigned char* instead > of unsigned char*. > > Some functions, such as fast_forward_to(), already have its old/new > sha1 arguments as consts. This function will at some point need to > use ref_transaction_update() in which case this change is required. Good, thanks. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 02/11] refs.c: change ref_transaction_update() to do error checking and return status
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > Update ref_transaction_update() do some basic error checking and return > true on error. Update all callers to check ref_transaction_update() for error. > > Signed-off-by: Ronnie Sahlberg > --- > builtin/update-ref.c | 11 +++ > refs.c | 9 +++-- > refs.h | 10 +- > 3 files changed, 19 insertions(+), 11 deletions(-) > > diff --git a/builtin/update-ref.c b/builtin/update-ref.c > index 405267f..12bfacc 100644 > --- a/builtin/update-ref.c > +++ b/builtin/update-ref.c > @@ -197,8 +197,10 @@ static const char *parse_cmd_update(struct strbuf > *input, const char *next) > if (*next != line_termination) > die("update %s: extra input: %s", refname, next); > > - ref_transaction_update(transaction, refname, new_sha1, old_sha1, > -update_flags, have_old); > + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, > +update_flags, have_old)) > + die("failed transaction update for %s", refname); > + > > update_flags = 0; > free(refname); > @@ -286,8 +288,9 @@ static const char *parse_cmd_verify(struct strbuf *input, > const char *next) > if (*next != line_termination) > die("verify %s: extra input: %s", refname, next); > > - ref_transaction_update(transaction, refname, new_sha1, old_sha1, > -update_flags, have_old); > + if (ref_transaction_update(transaction, refname, new_sha1, old_sha1, > +update_flags, have_old)) > + die("failed transaction update for %s", refname); > > update_flags = 0; > free(refname); > diff --git a/refs.c b/refs.c > index 138ab70..da1761d 100644 > --- a/refs.c > +++ b/refs.c > @@ -3327,19 +3327,24 @@ static struct ref_update *add_update(struct > ref_transaction *transaction, > return update; > } > > -void ref_transaction_update(struct ref_transaction *transaction, > +int ref_transaction_update(struct ref_transaction *transaction, > const char *refname, > const unsigned char *new_sha1, > const unsigned char *old_sha1, > int flags, int have_old) > { > - struct ref_update *update = add_update(transaction, refname); > + struct ref_update *update; > + > + if (have_old && !old_sha1) > + return error("have_old is true but old_sha1 is NULL"); The function documentation doesn't seem to allow old_sha1 == NULL. That means that calling it that way is a bug, not an error. Therefore, I think this should result in die("BUG: have_old is true but old_sha1 is NULL"); > > + update = add_update(transaction, refname); > hashcpy(update->new_sha1, new_sha1); > update->flags = flags; > update->have_old = have_old; > if (have_old) > hashcpy(update->old_sha1, old_sha1); > + return 0; > } > > void ref_transaction_create(struct ref_transaction *transaction, > diff --git a/refs.h b/refs.h > index 892c5b6..00e4f7b 100644 > --- a/refs.h > +++ b/refs.h > @@ -237,11 +237,11 @@ void ref_transaction_rollback(struct ref_transaction > *transaction); > * that the reference should have had before the update, or zeros if > * it must not have existed beforehand. > */ > -void ref_transaction_update(struct ref_transaction *transaction, > - const char *refname, > - const unsigned char *new_sha1, > - const unsigned char *old_sha1, > - int flags, int have_old); > +int ref_transaction_update(struct ref_transaction *transaction, > +const char *refname, > +const unsigned char *new_sha1, > +const unsigned char *old_sha1, > +int flags, int have_old); > > /* > * Add a reference creation to transaction. new_sha1 is the value > -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 03/11] refs.c: change ref_transaction_create to do error checking and return status
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > Do basic error checking in ref_transaction_create() and make it return > status. Update all callers to check the result of ref_transaction_create() > > Signed-off-by: Ronnie Sahlberg > --- > builtin/update-ref.c | 4 +++- > refs.c | 17 +++-- > refs.h | 8 > 3 files changed, 18 insertions(+), 11 deletions(-) > > diff --git a/builtin/update-ref.c b/builtin/update-ref.c > index 12bfacc..1b8a087 100644 > --- a/builtin/update-ref.c > +++ b/builtin/update-ref.c > @@ -226,7 +226,9 @@ static const char *parse_cmd_create(struct strbuf *input, > const char *next) > if (*next != line_termination) > die("create %s: extra input: %s", refname, next); > > - ref_transaction_create(transaction, refname, new_sha1, update_flags); > + if(ref_transaction_create(transaction, refname, new_sha1, > + update_flags)) Space between "if" and opening parenthesis. > + die("failed transaction create for %s", refname); > > update_flags = 0; > free(refname); > diff --git a/refs.c b/refs.c > index da1761d..c46249f 100644 > --- a/refs.c > +++ b/refs.c > @@ -3347,18 +3347,23 @@ int ref_transaction_update(struct ref_transaction > *transaction, > return 0; > } > > -void ref_transaction_create(struct ref_transaction *transaction, > - const char *refname, > - const unsigned char *new_sha1, > - int flags) > +int ref_transaction_create(struct ref_transaction *transaction, > +const char *refname, > +const unsigned char *new_sha1, > +int flags) > { > - struct ref_update *update = add_update(transaction, refname); > + struct ref_update *update; > + > + if (!new_sha1 || is_null_sha1(new_sha1)) > + return error("create ref with null new_sha1"); Please die("BUG: ..."). > + > + update = add_update(transaction, refname); > > - assert(!is_null_sha1(new_sha1)); > hashcpy(update->new_sha1, new_sha1); > hashclr(update->old_sha1); > update->flags = flags; > update->have_old = 1; > + return 0; > } > > void ref_transaction_delete(struct ref_transaction *transaction, > diff --git a/refs.h b/refs.h > index 00e4f7b..8799e15 100644 > --- a/refs.h > +++ b/refs.h > @@ -249,10 +249,10 @@ int ref_transaction_update(struct ref_transaction > *transaction, > * null SHA-1. It is verified that the reference does not exist > * already. > */ > -void ref_transaction_create(struct ref_transaction *transaction, > - const char *refname, > - const unsigned char *new_sha1, > - int flags); > +int ref_transaction_create(struct ref_transaction *transaction, > +const char *refname, > +const unsigned char *new_sha1, > +int flags); > > /* > * Add a reference deletion to transaction. If have_old is true, then > Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 04/11] refs.c: ref_transaction_delete to check for error and return status
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > Change ref_transaction_delete() to do basic error checking and return > status. Update all callers to check the return for ref_transaction_delete() > > Signed-off-by: Ronnie Sahlberg > --- > builtin/update-ref.c | 5 +++-- > refs.c | 15 ++- > refs.h | 8 > 3 files changed, 17 insertions(+), 11 deletions(-) > > diff --git a/builtin/update-ref.c b/builtin/update-ref.c > index 1b8a087..6ff8b86 100644 > --- a/builtin/update-ref.c > +++ b/builtin/update-ref.c > @@ -258,8 +258,9 @@ static const char *parse_cmd_delete(struct strbuf *input, > const char *next) > if (*next != line_termination) > die("delete %s: extra input: %s", refname, next); > > - ref_transaction_delete(transaction, refname, old_sha1, > -update_flags, have_old); > + if (ref_transaction_delete(transaction, refname, old_sha1, > +update_flags, have_old)) > + die("failed transaction delete for %s", refname); > > update_flags = 0; > free(refname); > diff --git a/refs.c b/refs.c > index c46249f..9cbcffa 100644 > --- a/refs.c > +++ b/refs.c > @@ -3366,19 +3366,24 @@ int ref_transaction_create(struct ref_transaction > *transaction, > return 0; > } > > -void ref_transaction_delete(struct ref_transaction *transaction, > - const char *refname, > - const unsigned char *old_sha1, > - int flags, int have_old) > +int ref_transaction_delete(struct ref_transaction *transaction, > +const char *refname, > +const unsigned char *old_sha1, > +int flags, int have_old) > { > - struct ref_update *update = add_update(transaction, refname); > + struct ref_update *update; > > + if (have_old && !old_sha1) > + return error("have_old is true but old_sha1 is NULL"); Ditto. > + > + update = add_update(transaction, refname); > update->flags = flags; > update->have_old = have_old; > if (have_old) { > assert(!is_null_sha1(old_sha1)); > hashcpy(update->old_sha1, old_sha1); > } > + return 0; > } > > int update_ref(const char *action, const char *refname, > diff --git a/refs.h b/refs.h > index 8799e15..7050da2 100644 > --- a/refs.h > +++ b/refs.h > @@ -259,10 +259,10 @@ int ref_transaction_create(struct ref_transaction > *transaction, > * old_sha1 holds the value that the reference should have had before > * the update (which must not be the null SHA-1). > */ > -void ref_transaction_delete(struct ref_transaction *transaction, > - const char *refname, > - const unsigned char *old_sha1, > - int flags, int have_old); > +int ref_transaction_delete(struct ref_transaction *transaction, > +const char *refname, > +const unsigned char *old_sha1, > +int flags, int have_old); > > /* > * Commit all of the changes that have been queued in transaction, as > Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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
[PATCH 0/6] run_external_diff cleanups
It's possible to overflow an array in run_external_diff and write a single NULL onto the stack. The first patch below fixes that. The rest are cleanups and modernizations I noticed while in the area. It's possible that patch 3 is also a bug fix, depending on your interpretation. [1/6]: run_external_diff: use an argv_array for the command line [2/6]: run_external_diff: use an argv_array for the environment [3/6]: run_external_diff: clean up error handling [4/6]: run_external_diff: drop fflush(NULL) [5/6]: run_external_diff: hoist common bits out of conditional [6/6]: run_external_diff: refactor cmdline setup logic -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 05/11] tag.c: use ref transactions when doing updates
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > Change tag.c to use ref transactions for all ref updates. > > Signed-off-by: Ronnie Sahlberg > --- > builtin/tag.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/builtin/tag.c b/builtin/tag.c > index 40356e3..dbeacc5 100644 > --- a/builtin/tag.c > +++ b/builtin/tag.c > @@ -488,7 +488,6 @@ int cmd_tag(int argc, const char **argv, const char > *prefix) > struct strbuf ref = STRBUF_INIT; > unsigned char object[20], prev[20]; > const char *object_ref, *tag; > - struct ref_lock *lock; > struct create_tag_options opt; > char *cleanup_arg = NULL; > int annotate = 0, force = 0, lines = -1; > @@ -496,6 +495,7 @@ int cmd_tag(int argc, const char **argv, const char > *prefix) > const char *msgfile = NULL, *keyid = NULL; > struct msg_arg msg = { 0, STRBUF_INIT }; > struct commit_list *with_commit = NULL; > + struct ref_transaction *transaction; > struct option options[] = { > OPT_CMDMODE('l', "list", &cmdmode, N_("list tag names"), 'l'), > { OPTION_INTEGER, 'n', NULL, &lines, N_("n"), > @@ -641,11 +641,14 @@ int cmd_tag(int argc, const char **argv, const char > *prefix) > if (annotate) > create_tag(object, tag, &buf, &opt, prev, object); > > - lock = lock_any_ref_for_update(ref.buf, prev, 0, NULL); > - if (!lock) > - die(_("%s: cannot lock the ref"), ref.buf); > - if (write_ref_sha1(lock, object, NULL) < 0) > - die(_("%s: cannot update the ref"), ref.buf); > + transaction = ref_transaction_begin(); > + if (!transaction) > + die(_("%s: cannot start transaction"), ref.buf); > + if (ref_transaction_update(transaction, ref.buf, object, prev, > +0, !is_null_sha1(prev))) > + die(_("%s: cannot update transaction"), ref.buf); > + if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR)) > + die(_("%s: cannot commit transaction"), ref.buf); I wonder whether these error messages are meaningful to the user. To the user, it's all just a failure to create the tag, right? Does it help to tell the user that the error was in the "start transaction", "update transaction", or "commit transaction" phase? In other words, maybe it would be just as good to do if (!transaction || ref_transaction_update(transaction, ref.buf, object, prev, 0, !is_null_sha1(prev)) || ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR)) die(_("%s: cannot update the ref"), ref.buf); But then I would also hope that somebody (probably ref_transaction_commit() now, but later maybe ref_transaction_update()) tells the user whether the problem was the inability to lock the reference, or the consistency check of old_sha1, or whatever. The next question is whether the generic error messages that the refs code generates is good enough, or whether the caller would rather learn about the reason for an error and generate its own, more specific error message. Please pay attention to this as you rewrite callers: by relying more on centralized code, are we losing out unacceptably on error message specificity? Oh and by the way, given that you pass UPDATE_REFS_DIE_ON_ERR, ref_transaction_commit() should never return a nonzero value, should it? > if (force && !is_null_sha1(prev) && hashcmp(prev, object)) > printf(_("Updated tag '%s' (was %s)\n"), tag, > find_unique_abbrev(prev, DEFAULT_ABBREV)); > > Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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 06/11] replace.c: use the ref transaction functions for updates
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > Update replace.c to use ref transactions for updates. > > Signed-off-by: Ronnie Sahlberg > --- > builtin/replace.c | 15 +-- > 1 file changed, 9 insertions(+), 6 deletions(-) > > diff --git a/builtin/replace.c b/builtin/replace.c > index b62420a..d8bd6ee 100644 > --- a/builtin/replace.c > +++ b/builtin/replace.c > @@ -129,7 +129,7 @@ static int replace_object(const char *object_ref, const > char *replace_ref, > unsigned char object[20], prev[20], repl[20]; > enum object_type obj_type, repl_type; > char ref[PATH_MAX]; > - struct ref_lock *lock; > + struct ref_transaction *transaction; > > if (get_sha1(object_ref, object)) > die("Failed to resolve '%s' as a valid ref.", object_ref); > @@ -157,11 +157,14 @@ static int replace_object(const char *object_ref, const > char *replace_ref, > else if (!force) > die("replace ref '%s' already exists", ref); > > - lock = lock_any_ref_for_update(ref, prev, 0, NULL); > - if (!lock) > - die("%s: cannot lock the ref", ref); > - if (write_ref_sha1(lock, repl, NULL) < 0) > - die("%s: cannot update the ref", ref); > + transaction = ref_transaction_begin(); > + if (!transaction) > + die(_("%s: cannot start transaction"), ref); > + if (ref_transaction_update(transaction, ref, repl, prev, > +0, !is_null_sha1(prev))) > + die(_("%s: cannot update transaction"), ref); > + if (ref_transaction_commit(transaction, NULL, UPDATE_REFS_DIE_ON_ERR)) > + die(_("%s: cannot commit transaction"), ref); The same comment that I made to 05/11 applies here too (and, I predict, to other patches yet to come). > > return 0; > } > Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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
[PATCH 1/6] run_external_diff: use an argv_array for the command line
We currently generate the command-line for the external command using a fixed-length array of size 10. But if there is a rename, we actually need 11 elements (10 items, plus a NULL), and end up writing a random NULL onto the stack. Rather than bump the limit, let's just an argv_array, which makes this sort of error impossible. Noticed-by: Max L Signed-off-by: Jeff King --- This was actually noticed by a GitHub user, who proposed bumping the array size to 11: https://github.com/git/git/pull/92 Even though this fix is a bigger change, I'd rather do it this way, as it is more obviously correct to a reader (and it solves the problem forever). I pulled the name/email from that commit, but please let me know if you'd prefer to be credited differently. diff.c | 32 1 file changed, 16 insertions(+), 16 deletions(-) diff --git a/diff.c b/diff.c index 539997f..b154284 100644 --- a/diff.c +++ b/diff.c @@ -16,6 +16,7 @@ #include "submodule.h" #include "ll-merge.h" #include "string-list.h" +#include "argv-array.h" #ifdef NO_FAST_WORKING_DIRECTORY #define FAST_WORKING_DIRECTORY 0 @@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm, int complete_rewrite, struct diff_options *o) { - const char *spawn_arg[10]; + struct argv_array argv = ARGV_ARRAY_INIT; int retval; - const char **arg = &spawn_arg[0]; struct diff_queue_struct *q = &diff_queued_diff; const char *env[3] = { NULL }; char env_counter[50]; @@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm, const char *othername = (other ? other : name); temp_one = prepare_temp_file(name, one); temp_two = prepare_temp_file(othername, two); - *arg++ = pgm; - *arg++ = name; - *arg++ = temp_one->name; - *arg++ = temp_one->hex; - *arg++ = temp_one->mode; - *arg++ = temp_two->name; - *arg++ = temp_two->hex; - *arg++ = temp_two->mode; + argv_array_push(&argv, pgm); + argv_array_push(&argv, name); + argv_array_push(&argv, temp_one->name); + argv_array_push(&argv, temp_one->hex); + argv_array_push(&argv, temp_one->mode); + argv_array_push(&argv, temp_two->name); + argv_array_push(&argv, temp_two->hex); + argv_array_push(&argv, temp_two->mode); if (other) { - *arg++ = other; - *arg++ = xfrm_msg; + argv_array_push(&argv, other); + argv_array_push(&argv, xfrm_msg); } } else { - *arg++ = pgm; - *arg++ = name; + argv_array_push(&argv, pgm); + argv_array_push(&argv, name); } - *arg = NULL; fflush(NULL); env[0] = env_counter; @@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm, env[1] = env_total; snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr); - retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, env); + retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env); remove_tempfile(); + argv_array_clear(&argv); if (retval) { fprintf(stderr, "external diff died, stopping at %s.\n", name); exit(1); -- 1.9.1.656.ge8a0637 -- 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/6] run_external_diff: use an argv_array for the environment
We currently use static buffers and a static array for formatting the environment passed to the external diff. There's nothing wrong in the code, but it is much easier to verify that it is correct if we use a dynamic argv_array. Signed-off-by: Jeff King --- diff.c | 14 +- 1 file changed, 5 insertions(+), 9 deletions(-) diff --git a/diff.c b/diff.c index b154284..760fc96 100644 --- a/diff.c +++ b/diff.c @@ -2904,11 +2904,9 @@ static void run_external_diff(const char *pgm, struct diff_options *o) { struct argv_array argv = ARGV_ARRAY_INIT; + struct argv_array env = ARGV_ARRAY_INIT; int retval; struct diff_queue_struct *q = &diff_queued_diff; - const char *env[3] = { NULL }; - char env_counter[50]; - char env_total[50]; if (one && two) { struct diff_tempfile *temp_one, *temp_two; @@ -2933,15 +2931,13 @@ static void run_external_diff(const char *pgm, } fflush(NULL); - env[0] = env_counter; - snprintf(env_counter, sizeof(env_counter), "GIT_DIFF_PATH_COUNTER=%d", -++o->diff_path_counter); - env[1] = env_total; - snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr); + argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); + argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr); - retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env); + retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv); remove_tempfile(); argv_array_clear(&argv); + argv_array_clear(&env); if (retval) { fprintf(stderr, "external diff died, stopping at %s.\n", name); exit(1); -- 1.9.1.656.ge8a0637 -- 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/6] run_external_diff: clean up error handling
When the external diff reports an error, we try to clean up and die. However, we can make this process a bit simpler: 1. We do not need to bother freeing memory, since we are about to exit. Nor do we need to clean up our tempfiles, since the atexit() handler will do it for us. So we can die as soon as we see the error. 3. We can just call die() rather than fprintf/exit. This does technically change our exit code, but the exit code of "1" is not meaningful here. In fact, it is probably wrong, since "1" from diff usually means "completed successfully, but there were differences". And while we're there, we can mark the error message for translation, and drop the full stop at the end to make it more like our other messages. Signed-off-by: Jeff King --- Note that we do have to update one test, which was expecting difftool to exit with 1 (and difftool just propagates diff's exit status in this case). I couldn't find any reasoning in the history for this exit(1), and it dates all the way back to May 2005. So I do not think it had any particular purpose, and for the reasons above, I do not think anyone would be sane to be relying on it. diff.c | 9 +++-- t/t7800-difftool.sh | 2 +- 2 files changed, 4 insertions(+), 7 deletions(-) diff --git a/diff.c b/diff.c index 760fc96..b517d01 100644 --- a/diff.c +++ b/diff.c @@ -2905,7 +2905,6 @@ static void run_external_diff(const char *pgm, { struct argv_array argv = ARGV_ARRAY_INIT; struct argv_array env = ARGV_ARRAY_INIT; - int retval; struct diff_queue_struct *q = &diff_queued_diff; if (one && two) { @@ -2934,14 +2933,12 @@ static void run_external_diff(const char *pgm, argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr); - retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv); + if (run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, env.argv)) + die(_("external diff died, stopping at %s"), name); + remove_tempfile(); argv_array_clear(&argv); argv_array_clear(&env); - if (retval) { - fprintf(stderr, "external diff died, stopping at %s.\n", name); - exit(1); - } } static int similarity_index(struct diff_filepair *p) diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh index 5a193c5..dc30a51 100755 --- a/t/t7800-difftool.sh +++ b/t/t7800-difftool.sh @@ -58,7 +58,7 @@ test_expect_success PERL 'custom tool commands override built-ins' ' test_expect_success PERL 'difftool ignores bad --tool values' ' : >expect && - test_expect_code 1 \ + test_must_fail \ git difftool --no-prompt --tool=bad-tool branch >actual && test_cmp expect actual ' -- 1.9.1.656.ge8a0637 -- 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/6] run_external_diff: drop fflush(NULL)
This fflush was added in d5535ec (Use run_command() to spawn external diff programs instead of fork/exec., 2007-10-19), because flushing buffers before forking is a good habit. But later, 7d0b18a (Add output flushing before fork(), 2008-08-04) added it to the generic run-command interface, meaning that our flush here is redundant. Signed-off-by: Jeff King --- diff.c | 1 - 1 file changed, 1 deletion(-) diff --git a/diff.c b/diff.c index b517d01..fdb7f6c 100644 --- a/diff.c +++ b/diff.c @@ -2928,7 +2928,6 @@ static void run_external_diff(const char *pgm, argv_array_push(&argv, pgm); argv_array_push(&argv, name); } - fflush(NULL); argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); argv_array_pushf(&env, "GIT_DIFF_PATH_TOTAL=%d", q->nr); -- 1.9.1.656.ge8a0637 -- 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/6] run_external_diff: hoist common bits out of conditional
Whether we have diff_filespecs to give to the diff command or not, we always are going to run the program and pass it the pathname. Let's pull that duplicated part out of the conditional to make it more obvious. Signed-off-by: Jeff King --- diff.c | 8 +++- 1 file changed, 3 insertions(+), 5 deletions(-) diff --git a/diff.c b/diff.c index fdb7f6c..173b657 100644 --- a/diff.c +++ b/diff.c @@ -2907,26 +2907,24 @@ static void run_external_diff(const char *pgm, struct argv_array env = ARGV_ARRAY_INIT; struct diff_queue_struct *q = &diff_queued_diff; + argv_array_push(&argv, pgm); + argv_array_push(&argv, name); + if (one && two) { struct diff_tempfile *temp_one, *temp_two; const char *othername = (other ? other : name); temp_one = prepare_temp_file(name, one); temp_two = prepare_temp_file(othername, two); - argv_array_push(&argv, pgm); - argv_array_push(&argv, name); argv_array_push(&argv, temp_one->name); argv_array_push(&argv, temp_one->hex); argv_array_push(&argv, temp_one->mode); argv_array_push(&argv, temp_two->name); argv_array_push(&argv, temp_two->hex); argv_array_push(&argv, temp_two->mode); if (other) { argv_array_push(&argv, other); argv_array_push(&argv, xfrm_msg); } - } else { - argv_array_push(&argv, pgm); - argv_array_push(&argv, name); } argv_array_pushf(&env, "GIT_DIFF_PATH_COUNTER=%d", ++o->diff_path_counter); -- 1.9.1.656.ge8a0637 -- 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 07/11] commit.c: use ref transactions for updates
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > Change commit.c to use ref transactions for all ref updates. > > Signed-off-by: Ronnie Sahlberg > --- > builtin/commit.c | 22 -- > 1 file changed, 12 insertions(+), 10 deletions(-) > > diff --git a/builtin/commit.c b/builtin/commit.c > index d9550c5..b8e4389 100644 > --- a/builtin/commit.c > +++ b/builtin/commit.c > @@ -1541,11 +1541,11 @@ int cmd_commit(int argc, const char **argv, const > char *prefix) > const char *index_file, *reflog_msg; > char *nl; > unsigned char sha1[20]; > - struct ref_lock *ref_lock; > struct commit_list *parents = NULL, **pptr = &parents; > struct stat statbuf; > struct commit *current_head = NULL; > struct commit_extra_header *extra = NULL; > + struct ref_transaction *transaction; > > if (argc == 2 && !strcmp(argv[1], "-h")) > usage_with_options(builtin_commit_usage, > builtin_commit_options); > @@ -1667,12 +1667,6 @@ int cmd_commit(int argc, const char **argv, const char > *prefix) > strbuf_release(&author_ident); > free_commit_extra_headers(extra); > > - ref_lock = lock_any_ref_for_update("HEAD", > -!current_head > -? NULL > -: current_head->object.sha1, > -0, NULL); The old version, above, contemplates that current_head might be NULL... > - > nl = strchr(sb.buf, '\n'); > if (nl) > strbuf_setlen(&sb, nl + 1 - sb.buf); > @@ -1681,14 +1675,22 @@ int cmd_commit(int argc, const char **argv, const > char *prefix) > strbuf_insert(&sb, 0, reflog_msg, strlen(reflog_msg)); > strbuf_insert(&sb, strlen(reflog_msg), ": ", 2); > > - if (!ref_lock) { > + transaction = ref_transaction_begin(); > + if (!transaction) { > rollback_index_files(); > - die(_("cannot lock HEAD ref")); > + die(_("HEAD: cannot start transaction")); > } > - if (write_ref_sha1(ref_lock, sha1, sb.buf) < 0) { > + if (ref_transaction_update(transaction, "HEAD", sha1, > +current_head->object.sha1, > +0, !!current_head)) { ...but here you dereference current_head without checking it first. It upsets me that the test suite didn't catch this NULL pointer dereference. Either 1. current_head cannot in fact be NULL, in which case the commit message should explain that fact and the code should be simplified or 2. the test suite is incomplete. If so, it would be great if you would add a test that exercises this branch of the code (and catches your error), and then fix the error. > rollback_index_files(); > die(_("cannot update HEAD ref")); > } > + if (ref_transaction_commit(transaction, sb.buf, > +UPDATE_REFS_QUIET_ON_ERR)) { > + rollback_index_files(); > + die(_("cannot commit HEAD ref")); > + } > > unlink(git_path("CHERRY_PICK_HEAD")); > unlink(git_path("REVERT_HEAD")); > Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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
[PATCH 6/6] run_external_diff: refactor cmdline setup logic
The current logic makes it hard to see what gets put onto the command line in which cases. Pulling out a helper function lets us see that we have two sets of file data, and the second set either uses the original name, or the "other" renamed/copy name. Signed-off-by: Jeff King --- The last patch and this one are getting a little bit into code churn, perhaps. I think the prior one is hands-down more readable. This one, I am on the fence on whether it is a true improvement or simply "this is how _I_ would have written it". I won't be offended if we drop it. diff.c | 26 +++--- 1 file changed, 15 insertions(+), 11 deletions(-) diff --git a/diff.c b/diff.c index 173b657..4b8bfc6 100644 --- a/diff.c +++ b/diff.c @@ -2888,6 +2888,16 @@ static struct diff_tempfile *prepare_temp_file(const char *name, return temp; } +static void add_external_diff_name(struct argv_array *argv, + const char *name, + struct diff_filespec *df) +{ + struct diff_tempfile *temp = prepare_temp_file(name, df); + argv_array_push(argv, temp->name); + argv_array_push(argv, temp->hex); + argv_array_push(argv, temp->mode); +} + /* An external diff command takes: * * diff-cmd name infile1 infile1-sha1 infile1-mode \ @@ -2911,17 +2921,11 @@ static void run_external_diff(const char *pgm, argv_array_push(&argv, name); if (one && two) { - struct diff_tempfile *temp_one, *temp_two; - const char *othername = (other ? other : name); - temp_one = prepare_temp_file(name, one); - temp_two = prepare_temp_file(othername, two); - argv_array_push(&argv, temp_one->name); - argv_array_push(&argv, temp_one->hex); - argv_array_push(&argv, temp_one->mode); - argv_array_push(&argv, temp_two->name); - argv_array_push(&argv, temp_two->hex); - argv_array_push(&argv, temp_two->mode); - if (other) { + add_external_diff_name(&argv, name, one); + if (!other) + add_external_diff_name(&argv, name, two); + else { + add_external_diff_name(&argv, other, two); argv_array_push(&argv, other); argv_array_push(&argv, xfrm_msg); } -- 1.9.1.656.ge8a0637 -- 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: Re: [msysGit] Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
On Sat, Apr 19, 2014 at 08:58:32PM +0400, Marat Radchenko wrote: > On Sat, Apr 19, 2014 at 05:24:33PM +0200, Johannes Schindelin wrote: > > Marat, please do not add to the confusion. "msysGit" is the name of the > > *development environment* for developing Git for Windows. > > This confusion comes from the fact that major part of msysGit is packaged > with Git for Windows to be used at runtime. Only the tools that are needed to run git (and some that the contributors like) are packaged in Git for Windows. For example there is no compiler or similar packaged. > If you insist on msysGit-is-a-development-environment, you have to admit > that msysGit is technically a fork of msys. Well it is a git repository that conveniently packages all the needed tools you need to build "Git for Windows" together. It is a little bit quick and dirty but it works. We have nothing against improving this situation. > My approach undoes this fork step and uses upstream runtime environment > as-is, be it msys, msys2, Cygwin or even SUA [1]. I could even make it a > noop and say "dear user, I don't care how, but please put sh/awk/find/etc > on PATH to make Git work, like things normally happen in *nix world". > > Actually, even if Git was pure C, things like `git filter-branch` would > be almost useless without coreutils & friends. > > > After all, there is no reason for yet another fork. > > If there wasn't, mingwGitDevEnv would not be started. I would not consider mingwGitDevEnv a fork. It is more msysgit next generation. But it needs more work to fully replace msysgit. > I'd say I am doing a 'rebase' instead of 'fork' by using codebase of > Git for Windows (upstream Git sources with Windows-specific patches) > but replacing msysGit-provided runtime environment with another one. The downside of doing this approach is that you regularly have to update your 'rebase' and fix problems. If you integrate your changes into msysgit itself you do not have to do that anymore. Well, if it is one of your changes that breaks something, it still would be nice if you do so ;-) > [1]: http://en.wikipedia.org/wiki/Windows_Services_for_UNIX Cheers Heiko P.S.: BTW, just in case: Being criticized in open-source is good. Even though it might not feel like that. It means people care about the stuff you do and think it is important enough it deserves a reply. They just want to help you improve it. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 11/11] walker.c: use ref transaction for ref updates
On 04/17/2014 09:46 PM, Ronnie Sahlberg wrote: > 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. I don't have time to review this last patch this evening, but one thing struck me. It seems like the old code went to extra effort to lock all the write_ref references early in the function, whereas your modified version doesn't lock them until later. Have you verified that you are not opening a possible race condition? If so, your commit message should justify that it isn't a problem. In other words, what does the code do between the old time of locking and the new time of locking and why doesn't it care whether the references are locked? Aside from my other comments, patches 01-10 in the series looked fine. Thanks! Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.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: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
On 19.04.2014 15:35, Marat Radchenko wrote: But in practice, msysgit is: 1) outdated msys that was patched in multiple ways without sending patches upstream It's not true that we are not sending patches upstream to MSYS, see [1]. It's just that most of them don't get included due to a lack of time from the MSYS maintainers, see e.g. [2]. 2) heavily patched git, again not upstreamed "Heavily" is relative, in fact it's not that much that I would give up hope on getting everything upstream. We once had put large efforts in bringing our stuff to upstream Git, just to over and over again being pulled into fussy discussions, costing way more time than developing the patches themselves. So at some time most of us just decided to spend their time more efficiently by bringing Git for Windows forward. To be honest, msys isn't a great tool. After all, it's just outdated and heavily patched cygwin. There exists msys2 project (much less outdated and much less patched cygwin). I agree that MSYS is not at all that great (anymore). It simply does not seem to be well maintained. But neither do I trust MSYS2 (yet), which looks to me like a spare time project by one or two guys, both newcomers not part of the original MSYS team. However, if MSYS2 turns out to be maintained better than MSYS in the future, I'm open to base mingwGitDevEnv on MSYS2. 1) It makes sense to purge msysgit and start over. See mingwGitDevEnv [2] (by msysgit developer). You would have been very welcomed to contribute 64-bit support to mingwGitDevEnv (which I'm the maintainer of). I saddens me that we blow out our energy on forks (without even getting in touch first) instead of pulling together. [1] http://sourceforge.net/p/mingw/bugs/search/?q=msysgit [2] http://sourceforge.net/p/mingw/bugs/1823/ -- 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: Re: [ANNOUNCE] WinGit - native x86/x64 Git for Windows
On Sat, Apr 19, 2014 at 8:42 PM, Heiko Voigt wrote: > But AFAIK its not ready for production use yet. I guess Sebastian would > not mind contributions. Not at all! -- 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
[PATCH] Documentation: git-gui: describe gui.displayuntracked
Signed-off-by: Max Kirillov --- Documentation for the option introduced in e632b3c0d3 Documentation/config.txt | 4 1 file changed, 4 insertions(+) diff --git a/Documentation/config.txt b/Documentation/config.txt index bbba728..41e31ce 100644 --- a/Documentation/config.txt +++ b/Documentation/config.txt @@ -1277,6 +1277,10 @@ gui.diffcontext:: Specifies how many context lines should be used in calls to diff made by the linkgit:git-gui[1]. The default is "5". +gui.displayuntracked:: + Determines if linkgit::git-gui[1] shows untracked files + in the file list. The default is "true". + gui.encoding:: Specifies the default encoding to use for displaying of file contents in linkgit:git-gui[1] and linkgit:gitk[1]. -- 1.8.4.rc3.902.g80a4b9e -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH 1/6] run_external_diff: use an argv_array for the command line
One more note: at this moment the problem is slightly deeper. This array is next passed to the execvp function, which now falls with EFAULT on two my machines (both faced this problem after upgrading to ubuntu 14.04, everything 'worked' fine before, looks like now execvp checks input more strictly). This leads to non-working 'git difftool'. 2014-04-19 23:17 GMT+04:00, Jeff King : > We currently generate the command-line for the external > command using a fixed-length array of size 10. But if there > is a rename, we actually need 11 elements (10 items, plus a > NULL), and end up writing a random NULL onto the stack. > > Rather than bump the limit, let's just an argv_array, which > makes this sort of error impossible. > > Noticed-by: Max L > Signed-off-by: Jeff King > --- > This was actually noticed by a GitHub user, who proposed bumping > the array size to 11: > > https://github.com/git/git/pull/92 > > Even though this fix is a bigger change, I'd rather do it this way, as > it is more obviously correct to a reader (and it solves the problem > forever). I pulled the name/email from that commit, but please let me > know if you'd prefer to be credited differently. > > diff.c | 32 > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/diff.c b/diff.c > index 539997f..b154284 100644 > --- a/diff.c > +++ b/diff.c > @@ -16,6 +16,7 @@ > #include "submodule.h" > #include "ll-merge.h" > #include "string-list.h" > +#include "argv-array.h" > > #ifdef NO_FAST_WORKING_DIRECTORY > #define FAST_WORKING_DIRECTORY 0 > @@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm, > int complete_rewrite, > struct diff_options *o) > { > - const char *spawn_arg[10]; > + struct argv_array argv = ARGV_ARRAY_INIT; > int retval; > - const char **arg = &spawn_arg[0]; > struct diff_queue_struct *q = &diff_queued_diff; > const char *env[3] = { NULL }; > char env_counter[50]; > @@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm, > const char *othername = (other ? other : name); > temp_one = prepare_temp_file(name, one); > temp_two = prepare_temp_file(othername, two); > - *arg++ = pgm; > - *arg++ = name; > - *arg++ = temp_one->name; > - *arg++ = temp_one->hex; > - *arg++ = temp_one->mode; > - *arg++ = temp_two->name; > - *arg++ = temp_two->hex; > - *arg++ = temp_two->mode; > + argv_array_push(&argv, pgm); > + argv_array_push(&argv, name); > + argv_array_push(&argv, temp_one->name); > + argv_array_push(&argv, temp_one->hex); > + argv_array_push(&argv, temp_one->mode); > + argv_array_push(&argv, temp_two->name); > + argv_array_push(&argv, temp_two->hex); > + argv_array_push(&argv, temp_two->mode); > if (other) { > - *arg++ = other; > - *arg++ = xfrm_msg; > + argv_array_push(&argv, other); > + argv_array_push(&argv, xfrm_msg); > } > } else { > - *arg++ = pgm; > - *arg++ = name; > + argv_array_push(&argv, pgm); > + argv_array_push(&argv, name); > } > - *arg = NULL; > fflush(NULL); > > env[0] = env_counter; > @@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm, > env[1] = env_total; > snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", q->nr); > > - retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, > env); > + retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, > env); > remove_tempfile(); > + argv_array_clear(&argv); > if (retval) { > fprintf(stderr, "external diff died, stopping at %s.\n", name); > exit(1); > -- > 1.9.1.656.ge8a0637 > > -- 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/6] run_external_diff: use an argv_array for the command line
On Sat, Apr 19, 2014 at 3:17 PM, Jeff King wrote: > We currently generate the command-line for the external > command using a fixed-length array of size 10. But if there > is a rename, we actually need 11 elements (10 items, plus a > NULL), and end up writing a random NULL onto the stack. > > Rather than bump the limit, let's just an argv_array, which s/just/just use/ > makes this sort of error impossible. > > Noticed-by: Max L > Signed-off-by: Jeff King > --- > This was actually noticed by a GitHub user, who proposed bumping > the array size to 11: > > https://github.com/git/git/pull/92 > > Even though this fix is a bigger change, I'd rather do it this way, as > it is more obviously correct to a reader (and it solves the problem > forever). I pulled the name/email from that commit, but please let me > know if you'd prefer to be credited differently. > > diff.c | 32 > 1 file changed, 16 insertions(+), 16 deletions(-) > > diff --git a/diff.c b/diff.c > index 539997f..b154284 100644 > --- a/diff.c > +++ b/diff.c > @@ -16,6 +16,7 @@ > #include "submodule.h" > #include "ll-merge.h" > #include "string-list.h" > +#include "argv-array.h" > > #ifdef NO_FAST_WORKING_DIRECTORY > #define FAST_WORKING_DIRECTORY 0 > @@ -2902,9 +2903,8 @@ static void run_external_diff(const char *pgm, > int complete_rewrite, > struct diff_options *o) > { > - const char *spawn_arg[10]; > + struct argv_array argv = ARGV_ARRAY_INIT; > int retval; > - const char **arg = &spawn_arg[0]; > struct diff_queue_struct *q = &diff_queued_diff; > const char *env[3] = { NULL }; > char env_counter[50]; > @@ -2915,23 +2915,22 @@ static void run_external_diff(const char *pgm, > const char *othername = (other ? other : name); > temp_one = prepare_temp_file(name, one); > temp_two = prepare_temp_file(othername, two); > - *arg++ = pgm; > - *arg++ = name; > - *arg++ = temp_one->name; > - *arg++ = temp_one->hex; > - *arg++ = temp_one->mode; > - *arg++ = temp_two->name; > - *arg++ = temp_two->hex; > - *arg++ = temp_two->mode; > + argv_array_push(&argv, pgm); > + argv_array_push(&argv, name); > + argv_array_push(&argv, temp_one->name); > + argv_array_push(&argv, temp_one->hex); > + argv_array_push(&argv, temp_one->mode); > + argv_array_push(&argv, temp_two->name); > + argv_array_push(&argv, temp_two->hex); > + argv_array_push(&argv, temp_two->mode); > if (other) { > - *arg++ = other; > - *arg++ = xfrm_msg; > + argv_array_push(&argv, other); > + argv_array_push(&argv, xfrm_msg); > } > } else { > - *arg++ = pgm; > - *arg++ = name; > + argv_array_push(&argv, pgm); > + argv_array_push(&argv, name); > } > - *arg = NULL; > fflush(NULL); > > env[0] = env_counter; > @@ -2940,8 +2939,9 @@ static void run_external_diff(const char *pgm, > env[1] = env_total; > snprintf(env_total, sizeof(env_total), "GIT_DIFF_PATH_TOTAL=%d", > q->nr); > > - retval = run_command_v_opt_cd_env(spawn_arg, RUN_USING_SHELL, NULL, > env); > + retval = run_command_v_opt_cd_env(argv.argv, RUN_USING_SHELL, NULL, > env); > remove_tempfile(); > + argv_array_clear(&argv); > if (retval) { > fprintf(stderr, "external diff died, stopping at %s.\n", > name); > exit(1); > -- > 1.9.1.656.ge8a0637 -- 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/6] run_external_diff: use an argv_array for the command line
On Sun, Apr 20, 2014 at 02:09:49AM +0400, Max L wrote: > One more note: at this moment the problem is slightly deeper. This > array is next passed to the execvp function, which now falls with > EFAULT on two my machines (both faced this problem after upgrading to > ubuntu 14.04, everything 'worked' fine before, looks like now execvp > checks input more strictly). This leads to non-working 'git difftool'. Interesting. We're overwriting whatever is after spawn_arg on the stack, so I'd expect the fork/exec to work, but the function to complain while popping the stack frame (though I couldn't get it to do so). I wonder if some kind of stack protection is kicking in, and the NULL doesn't get written or something. Either way, we should definitely address 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