[PATCH 5/5] transport-helper: fix sync issue on crashes

[Junio C Hamano]

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

[Junio C Hamano]

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

[Junio C Hamano]

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()

[Junio C Hamano]

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

[Junio C Hamano]

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

[Junio C Hamano]

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

[Jiang Xin]

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

[Eric Wong]

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?

[Ilya Basin]

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

[Johan Herland]

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

[Schittli Thomas]

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

[Eric Wong]

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

[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.

> 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]

[Siegel, Suzan]

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

[John Keeping]

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

[Johannes Schindelin]

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

[Marat Radchenko]

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

[Thomas Braun]

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

[Johannes Schindelin]

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

[Ralf Thielow]

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

[Schittli Thomas]

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

[Heiko Voigt]

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

[Michael Haggerty]

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

[Michael Haggerty]

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

[Michael Haggerty]

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

[Michael Haggerty]

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

[Jeff King]

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

[Michael Haggerty]

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

[Michael Haggerty]

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

[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

[PATCH 2/6] run_external_diff: use an argv_array for the environment

[Jeff King]

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

[Jeff King]

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)

[Jeff King]

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

[Jeff King]

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

[Michael Haggerty]

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

[Jeff King]

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

[Heiko Voigt]

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

[Michael Haggerty]

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

[Sebastian Schuberth]

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

[Sebastian Schuberth]

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

[Max Kirillov]

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

[Max L]

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

[Eric Sunshine]

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

[Jeff King]

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