Re: [PATCH v6 4/4] config: add '--show-origin' option to print the origin of a config value

[Johannes Sixt]

Am 03.03.2016 um 08:38 schrieb Lars Schneider:

(1) If I have a Git core branch with a some changes that builds and
tests clean on Linux and OSX. How do I apply all the necessary Git for
Windows specific changes to this branch?


How do you do it when you make a patch on Linux and want to test it on 
OSX, or the other way around? It's the same with Windows, I would guess.


*I* would export the Linux directory for Windows using Samba and then 
fetch and push from the Windows side. I would *not* develop on Windows 
in the exported Samba directory directly. If Samba is too hairy, 
exchange git bundles on a USB stick.



(2) During my testing with Windows I noticed that the git config
paths look funny by adding ("\\" and "/"). I mentioned the problem in
the Gitfor Windows forum:
https://groups.google.com/forum/#!topic/git-for-windows/zTv60HhfnYk
Duy suggested a solution in that thread. Is this the default way
todeal with the paths? Would the list accept this solution?


IMHO, the solution is misguided. Either --show-origin is plumbing, then 
we need the quoting. Or it is porcelain, then the quoting can be removed 
(and it is not necessary to "prettify" the file names on Windows). I 
tend to categorize --show-origin as procelain.



(3) The tests on Windows seemed very slow to me. Are there tricks to
speed them up? Did you try a RAM disk? If yes, how do you do it?


Run on SSD (and be prepared to swap it out for a new one within a year 
or two) ;-)


Really, there doesn't seem to be much you can do. Run the tests 
--with-dashes to save a shell wrapper around git.exe.


Unfortunately, Windows does not have RAM disks built in. I would 
appreciate any hints in this direction as well.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "./t0001-init.sh --valgrind" is broken

[Johannes Sixt]

Am 03.03.2016 um 13:09 schrieb Duy Nguyen:

+the-other-Johannes who added valgrind support.

On Thu, Mar 3, 2016 at 1:55 PM, Johannes Sixt <j...@kdbg.org> wrote:

 8< 
Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind

When a test case is run without --valgrind, the wrap-for-bin.sh
helper script inserts the environment variable GIT_TEXTDOMAINDIR, but
when run with --valgrind, the variable is missing. A recently
introduced test case expects the presence of the variable, though, and
fails under --valgrind.


Yep.

It's interesting though that valgrind sets up some variables without
going through bin-wrappers. That's understandable because valgrind
support is added (in 4e1be63) 10 months before bin-wrappers (in
ea92519).  But it's probably better that we inject valgrind command
from inside bin-wrappers script, the same way we inject gdb, I think.


Rewrite the test case to strip conditially defined environment variables
from both expected and actual output.


Or we could set GIT_TEXTDOMAINDIR in the "if test -n $valgrind" code
in test-lib.sh, which makes the two more consistent. Also simpler
patch.


My fix (or something along its lines) is needed nevertheless. Just 
s/--valgrind/--with-dashes/g in the commit message if you want to fix 
the --valgrind case differently ;-)


I run tests on Windows --with-dashes in the hopes that it saves a fork 
and exec or two on every git invocation.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: "./t0001-init.sh --valgrind" is broken

[Johannes Sixt]

Am 03.03.2016 um 02:04 schrieb Duy Nguyen:
> On Thu, Mar 3, 2016 at 7:07 AM, Christian Couder
> <christian.cou...@gmail.com> wrote:
>> Hi,
>>
>> It looks like commit 57ea7123c86771f47f34e7d92d1822d8b429897a (git.c:
>> make sure we do not leak GIT_* to alias scripts, Dec 20 14:50:19 2015)
>> broke "./t0001-init.sh --valgrind".
> 
> Just wanted to confirm the problem. I will look at it later today.
> 

Here's a patch.

 8< 
Subject: [PATCH] t0001: fix GIT_* environment variable check under --valgrind

When a test case is run without --valgrind, the wrap-for-bin.sh
helper script inserts the environment variable GIT_TEXTDOMAINDIR, but
when run with --valgrind, the variable is missing. A recently
introduced test case expects the presence of the variable, though, and
fails under --valgrind.

Rewrite the test case to strip conditially defined environment variables
from both expected and actual output.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t0001-init.sh | 20 +---
 1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index 295aa59..a5b9e7a 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -88,19 +88,17 @@ test_expect_success 'plain nested in bare through aliased 
command' '
 '
 
 test_expect_success 'No extra GIT_* on alias scripts' '
-   (
-   env | sed -ne "/^GIT_/s/=.*//p" &&
-   echo GIT_PREFIX &&# setup.c
-   echo GIT_TEXTDOMAINDIR# wrapper-for-bin.sh
-   ) | sort | uniq >expected &&
-   cat <<-\EOF >script &&
-   #!/bin/sh
-   env | sed -ne "/^GIT_/s/=.*//p" | sort >actual
-   exit 0
+   write_script script <<-\EOF &&
+   env |
+   sed -n \
+   -e "/^GIT_PREFIX=/d" \
+   -e "/^GIT_TEXTDOMAINDIR=/d" \
+   -e "/^GIT_/s/=.*//p" |
+   sort
EOF
-   chmod 755 script &&
+   ./script >expected &&
git config alias.script \!./script &&
-   ( mkdir sub && cd sub && git script ) &&
+   ( mkdir sub && cd sub && git script >../actual ) &&
test_cmp expected actual
 '
 
-- 
2.7.0.118.g90056ae

--
To unsubscribe from this list: send the line "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 v6 4/4] config: add '--show-origin' option to print the origin of a config value

[Johannes Sixt]

Am 19.02.2016 um 10:16 schrieb larsxschnei...@gmail.com:

+test_expect_success '--show-origin with --list' '
+   cat >expect <<-EOF &&
+   file:$HOME/.gitconfig   user.global=true
+   file:$HOME/.gitconfig   user.override=global
+   file:$HOME/.gitconfig   
include.path=$INCLUDE_DIR/absolute.include


On Windows, this injects POSIX-style paths in the expected output, but 
git.exe produces mangled paths (with a drive letter). The pattern I use 
to fix this is:


file:$(pwd)/.gitconfig  user.override=global


+   file:$INCLUDE_DIR/absolute.include  user.absolute=include
+   file:.git/configuser.local=true
+   file:.git/configuser.override=local
+   file:.git/configinclude.path=../include/relative.include
+   file:.git/../include/relative.include   user.relative=include
+   cmdline:user.cmdline=true
+   EOF
+   git -c user.cmdline=true config --list --show-origin >output &&
+   test_cmp expect output
+'

...

+test_expect_success 'set up custom config file' '
+   CUSTOM_CONFIG_FILE="file\" (dq) and spaces.conf" &&
+   cat >"$CUSTOM_CONFIG_FILE" <<-\EOF
+   [user]
+   custom = true
+   EOF


This fails on Windows, because the shell cannot create a file containing 
a double-quote character.


IIUC, the test serves two purposes: (1) to test C-style quoting of the 
output and (2) non-standard configuration files. We'll have to separate 
that so that we can test at least (2) on Windows with "regular" file 
name. We cannot test (1) because the only case where quoting is used is 
when the file name contains a double-quote character.



+'
+
+test_expect_success '--show-origin escape special file name characters' '
+   cat >expect <<-\EOF &&
+   file:"file\" (dq) and spaces.conf"   user.custom=true
+   EOF
+   git config --file "$CUSTOM_CONFIG_FILE" --show-origin --list >output &&
+   test_cmp expect output
+'

...

+test_expect_success '--show-origin blob ref' '
+   cat >expect <<-\EOF &&
+   blob:"master:file\" (dq) and spaces.conf"
user.custom=true
+   EOF
+   git add "$CUSTOM_CONFIG_FILE" &&


Is this dual-purpose as well or just re-using the files established 
earlier in the test suite?



+   git commit -m "new config file" &&
+   git config --blob=master:"$CUSTOM_CONFIG_FILE" --show-origin --list >output 
&&
+   test_cmp expect output
+'
+
  test_done
--
2.5.1



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] submodule: port resolve_relative_url from shell to C

[Johannes Sixt]

Am 13.02.2016 um 00:39 schrieb Stefan Beller:
> @@ -289,4 +296,39 @@ test_git_path GIT_COMMON_DIR=bar config  
>  bar/config
>   test_git_path GIT_COMMON_DIR=bar packed-refs  bar/packed-refs
>   test_git_path GIT_COMMON_DIR=bar shallow  bar/shallow
>   
> +test_submodule_relative_url "(null)" "../foo/bar" "../sub/a/b/c" 
> "../foo/sub/a/b/c"
> +test_submodule_relative_url "../../../" "../foo/bar" "../sub/a/b/c" 
> "../../../../foo/sub/a/b/c"
> +test_submodule_relative_url "(null)" "../foo/bar" "../submodule" 
> "../foo/submodule"
> +test_submodule_relative_url "../" "../foo/bar" "../submodule" 
> "../../foo/submodule"
> +test_submodule_relative_url "(null)" "../foo/submodule" "../submodule" 
> "../foo/submodule"
> +test_submodule_relative_url "../" "../foo/submodule" "../submodule" 
> "../../foo/submodule"
> +test_submodule_relative_url "(null)" "../foo" "../submodule" "../submodule"
> +test_submodule_relative_url "../" "../foo" "../submodule" "../../submodule"
> +test_submodule_relative_url "(null)" "./foo/bar" "../submodule" 
> "foo/submodule"
> +test_submodule_relative_url "../" "./foo/bar" "../submodule" 
> "../foo/submodule"
> +test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
> +test_submodule_relative_url "../" "./foo" "../submodule" "../submodule"
> +test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" 
> "//somewhere else/subrepo"
> +test_submodule_relative_url "(null)" "/u//trash 
> directory.t7406-submodule-update/subsuper_update_r" "../subsubsuper_update_r" 
> "/u//trash directory.t7406-submodule-update/subsubsuper_update_r"
> +test_submodule_relative_url "(null)" "/u//trash 
> directory.t7406-submodule-update/super_update_r2" "../subsuper_update_r" 
> "/u//trash directory.t7406-submodule-update/subsuper_update_r"
> +test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm/." "../." 
> "/u/trash directory.t3600-rm/."
> +test_submodule_relative_url "(null)" "/u/trash directory.t3600-rm" "./." 
> "/u/trash directory.t3600-rm/."
> +test_submodule_relative_url "(null)" "/u/trash 
> directory.t7400-submodule-basic/addtest" "../repo" "/u/trash 
> directory.t7400-submodule-basic/repo"
> +test_submodule_relative_url "../" "/u/trash 
> directory.t7400-submodule-basic/addtest" "../repo" "/u/trash 
> directory.t7400-submodule-basic/repo"
> +test_submodule_relative_url "(null)" "/u/trash 
> directory.t7400-submodule-basic" "./å äö" "/u/trash 
> directory.t7400-submodule-basic/å äö"
> +test_submodule_relative_url "(null)" "/u/trash 
> directory.t7403-submodule-sync/." "../submodule" "/u/trash 
> directory.t7403-submodule-sync/submodule"
> +test_submodule_relative_url "(null)" "/u/trash 
> directory.t7407-submodule-foreach/submodule" "../submodule" "/u/trash 
> directory.t7407-submodule-foreach/submodule"
> +test_submodule_relative_url "(null)" "/u/trash 
> directory.t7409-submodule-detached-worktree/home2/../remote" "../bundle1" 
> "/u/trash directory.t7409-submodule-detached-worktree/home2/../bundle1"
> +test_submodule_relative_url "(null)" "/u/trash 
> directory.t7613-merge-submodule/submodule_update_repo" "./." "/u/trash 
> directory.t7613-merge-submodule/submodule_update_repo/."

The tests with absolute paths all fail on Windows. The reason is that
git.exe sees mangled paths and 'git submodule--helper
resolve-relative-url-test' produces mangled paths (that begins with a
drive letter), whereas the test script expects POSIX paths. The pattern
I currently use to fix this is

test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"

(In our test scripts, $PWD is a POSIX style path and $(pwd) is a
Windows style path).

With this change, the penultimate case above still fails because the
'home2/..' gets lost somewhere in the actual output, which I still have
to debug.

The two cases beginning with '/u//' cannot be tested on Windows.
Are they important? Are the doubled slashes intentional?

> +test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" 
> "file:///tmp/subrepo"
> +test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
> +test_submodule_relative_url "../" "foo/bar" "../submodule" "../foo/submodule"
> +test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
> +test_submodule_relative_url "../" "foo" "../submodule" "../submodule"
> +test_submodule_relative_url "(null)" "helper:://hostname/repo" "../subrepo" 
> "helper:://hostname/subrepo"
> +test_submodule_relative_url "(null)" "ssh://hostname/repo" "../subrepo" 
> "ssh://hostname/subrepo"
> +test_submodule_relative_url "(null)" "ssh://hostname:22/repo" "../subrepo" 
> "ssh://hostname:22/subrepo"
> +test_submodule_relative_url "(null)" "user@host:path/to/repo" "../subrepo" 
> "user@host:path/to/subrepo"
> +test_submodule_relative_url "(null)" "user@host:repo" "../subrepo" 
> "user@host:subrepo"
> +
>   test_done
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the 

Re: [PATCH v2] Mark win32's pthread_exit() as NORETURN

[Johannes Sixt]

Am 01.03.2016 um 15:13 schrieb Johannes Schindelin:

The pthread_exit() function is not expected to return. Ever. On Windows,
we call ExitThread() whose documentation claims: "This function does not
return a value.":

https://msdn.microsoft.com/en-us/library/windows/desktop/ms682659


This is misleading: MSDN marks all functions declared void as "does not 
return a value," for example, look at EnterCriticalSection:


https://msdn.microsoft.com/en-us/library/windows/desktop/ms682608

For this reason, I actually prefer your version 1 patch without the 
explanation.




Pointed out by Jeff King.

Signed-off-by: Johannes Schindelin 
---

Relative to v1, only the commit message changed (to clarify that
ExitThread() indeed never returns).

  compat/win32/pthread.h | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/win32/pthread.h b/compat/win32/pthread.h
index 20b35a2..148db60 100644
--- a/compat/win32/pthread.h
+++ b/compat/win32/pthread.h
@@ -78,7 +78,7 @@ extern int win32_pthread_join(pthread_t *thread, void 
**value_ptr);
  #define pthread_equal(t1, t2) ((t1).tid == (t2).tid)
  extern pthread_t pthread_self(void);

-static inline int pthread_exit(void *ret)
+static inline int NORETURN pthread_exit(void *ret)


I would have written it as

#ifdef __GNUC__
__attribute__((__noreturn__))
#endif
static inline int pthread_exit(void *ret) ...

but I can live with your version as long as it compiles.

Your solution is pragmatic: NORETURN is defined in git-compat-util.h, 
and by using it here, we depend on that pthread.h is included 
sufficiently late that the macro is available at this point. The 
instance in compat/nedmalloc/malloc.c.h is bracketed with #ifndef WIN32 
so that it is not compiled on Windows, all other instances are after 
git-compat-util.h or cache.h or in headers that are to be included only 
after git-compat-util.h or cache.h per convention. Looks like we are safe.



  {
ExitThread((DWORD)(intptr_t)ret);
  }



--
To unsubscribe from this list: send the line "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] run-command: do not pass child process data into callbacks

[Johannes Sixt]

Am 29.02.2016 um 22:57 schrieb Stefan Beller:
> The expected way to pass data into the callback is to pass them via
> the customizable callback pointer. The error reporting in
> default_{start_failure, task_finished} is not user friendly enough, that
> we want to encourage using the child data for such purposes.
> 
> Furthermore the struct child data is cleaned by the run-command API,
> before we access them in the callbacks, leading to use-after-free
> situations.

Thanks. The code changes match what I had prototyped. But please squash
in this documentation change:

diff --git a/run-command.h b/run-command.h
index c6a3e42..3d1e59e 100644
--- a/run-command.h
+++ b/run-command.h
@@ -191,9 +191,8 @@ typedef int (*task_finished_fn)(int result,
  * (both stdout and stderr) is routed to stderr in a manner that output
  * from different tasks does not interleave.
  *
- * If start_failure_fn or task_finished_fn are NULL, default handlers
- * will be used. The default handlers will print an error message on
- * error without issuing an emergency stop.
+ * start_failure_fn and task_finished_fn can be NULL to omit any
+ * special handling.
  */
 int run_processes_parallel(int n,
   get_next_task_fn,

--
To unsubscribe from this list: send the line "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: [PATCHv19 00/11] Expose submodule parallelism to the user

[Johannes Sixt]

Am 29.02.2016 um 22:19 schrieb Junio C Hamano:

Stefan Beller  writes:


Maybe we want to remove the struct child_process from the
function signature of the callbacks and callbacks need to rely on
the data provided solely thru the pointer as passed around for
callback purposes, which the user is free to use for any kind
of data.


I think that is the most sensible.


I also think that is the better approach. Dumping out the argv array is 
not the best end-user experience. It is just a debugging aid, and for 
that we have (or should extend if necessary) GIT_TRACE infrastructure. 
Moreover, a command that failed should have printed error messages, and 
it is not necessary to follow it up with another "A child process exited 
with code N" message.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv19 00/11] Expose submodule parallelism to the user

[Johannes Sixt]

Hi folks,

we have a major breakage in the parallel tasks infrastructure, and I'm
afraid it is already in master.

Instrument the code in sb/submodule-parallel-update like this and enjoy
the fireworks of './t7400-submodule-basic.sh -v -i -x --debug':

diff --git a/git-submodule.sh b/git-submodule.sh
index 0322282..482c7f6 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -690,8 +690,9 @@ cmd_update()
cmd_init "--" "$@" || return
fi
 
+   set -x
{
-   git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
+   valgrind git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
${wt_prefix:+--prefix "$wt_prefix"} \
${prefix:+--recursive-prefix "$prefix"} \
${update:+--update "$update"} \
diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh
index 5572327..717e491 100755
--- a/t/t7400-submodule-basic.sh
+++ b/t/t7400-submodule-basic.sh
@@ -337,6 +337,7 @@ test_expect_success 'update should fail when path is used 
by a file' '
 
echo "hello" >init &&
test_must_fail git submodule update &&
+   false &&
 
test_cmp expect init
 '

The culprit seems to be default_task_finished(), which accesses argv[]
of the struct child_process after finish_command has released it,
provided the child exited with an error, for example:

==3395== Invalid read of size 8
==3395==at 0x54F991: default_task_finished (run-command.c:932)
==3395==by 0x49158F: update_clone_task_finished (submodule--helper.c:421)
==3395==by 0x5504A2: pp_collect_finished (run-command.c:1122)
==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194)
==3395==by 0x4918EB: update_clone (submodule--helper.c:483)
==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527)
==3395==by 0x405CBE: run_builtin (git.c:353)
==3395==by 0x405EAA: handle_builtin (git.c:540)
==3395==by 0x405FCC: run_argv (git.c:594)
==3395==by 0x4061BF: main (git.c:701)
==3395==  Address 0x5e49370 is 0 bytes inside a block of size 192 free'd
==3395==at 0x4C2A37C: free (in 
/usr/lib64/valgrind/vgpreload_memcheck-amd64-linux.so)
==3395==by 0x4A26EE: argv_array_clear (argv-array.c:73)
==3395==by 0x54DFC4: child_process_clear (run-command.c:18)
==3395==by 0x54EFA7: finish_command (run-command.c:539)
==3395==by 0x550413: pp_collect_finished (run-command.c:1120)
==3395==by 0x5507C7: run_processes_parallel (run-command.c:1194)
==3395==by 0x4918EB: update_clone (submodule--helper.c:483)
==3395==by 0x4919D8: cmd_submodule__helper (submodule--helper.c:527)
==3395==by 0x405CBE: run_builtin (git.c:353)
==3395==by 0x405EAA: handle_builtin (git.c:540)
==3395==by 0x405FCC: run_argv (git.c:594)
==3395==by 0x4061BF: main (git.c:701)

I haven't thought about a solution, yet. Perhaps you have ideas.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] t5313: test bounds-checks of corrupted/malicious pack/idx files

[Johannes Sixt]

Am 25.02.2016 um 15:21 schrieb Jeff King:
> +munge () {
> + printf "$3" | dd of="$1" bs=1 conv=notrunc seek=$2
> +}

Instead of adding another call of dd, would it be an option to insert
the following patch at the front of this series and then use
test_overwrite_bytes?

---- 8< 
From: Johannes Sixt <j...@kdbg.org>
Subject: [PATCH] tests: overwrite bytes in files using a perl script instead of 
dd

The dd in my build environment on Windows crashes unpredictably. Work it
around by rewriting most instances with a helper function that uses perl
behind the scenes.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t1060-object-corruption.sh  |  2 +-
 t/t5300-pack-object.sh|  8 
 t/t5302-pack-index.sh |  5 +++--
 t/t5303-pack-corruption-resilience.sh |  2 +-
 t/test-lib-functions.sh   | 16 
 5 files changed, 25 insertions(+), 8 deletions(-)

diff --git a/t/t1060-object-corruption.sh b/t/t1060-object-corruption.sh
index 3f87051..e3c5de8 100755
--- a/t/t1060-object-corruption.sh
+++ b/t/t1060-object-corruption.sh
@@ -12,7 +12,7 @@ obj_to_file() {
 corrupt_byte() {
obj_file=$(obj_to_file "$1") &&
chmod +w "$obj_file" &&
-   printf '\0' | dd of="$obj_file" bs=1 seek="$2" conv=notrunc
+   printf '\0' | test_overwrite_bytes "$obj_file" "$2"
 }
 
 test_expect_success 'setup corrupt repo' '
diff --git a/t/t5300-pack-object.sh b/t/t5300-pack-object.sh
index fc2be63..f45a101 100755
--- a/t/t5300-pack-object.sh
+++ b/t/t5300-pack-object.sh
@@ -226,7 +226,7 @@ test_expect_success \
 test_expect_success \
 'verify-pack catches a corrupted pack signature' \
 'cat test-1-${packname_1}.pack >test-3.pack &&
- echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=2 &&
+ echo | test_overwrite_bytes test-3.pack 2 &&
  if git verify-pack test-3.idx
  then false
  else :;
@@ -235,7 +235,7 @@ test_expect_success \
 test_expect_success \
 'verify-pack catches a corrupted pack version' \
 'cat test-1-${packname_1}.pack >test-3.pack &&
- echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=7 &&
+ echo | test_overwrite_bytes test-3.pack 7 &&
  if git verify-pack test-3.idx
  then false
  else :;
@@ -244,7 +244,7 @@ test_expect_success \
 test_expect_success \
 'verify-pack catches a corrupted type/size of the 1st packed object data' \
 'cat test-1-${packname_1}.pack >test-3.pack &&
- echo | dd of=test-3.pack count=1 bs=1 conv=notrunc seek=12 &&
+ echo | test_overwrite_bytes test-3.pack 12 &&
  if git verify-pack test-3.idx
  then false
  else :;
@@ -255,7 +255,7 @@ test_expect_success \
 'l=$(wc -c <test-3.idx) &&
  l=$(expr $l - 20) &&
  cat test-1-${packname_1}.pack >test-3.pack &&
- printf "%20s" "" | dd of=test-3.idx count=20 bs=1 conv=notrunc seek=$l &&
+ printf "%20s" "" | test_overwrite_bytes test-3.idx $l &&
  if git verify-pack test-3.pack
  then false
  else :;
diff --git a/t/t5302-pack-index.sh b/t/t5302-pack-index.sh
index c2fc584..5a82f19 100755
--- a/t/t5302-pack-index.sh
+++ b/t/t5302-pack-index.sh
@@ -225,8 +225,9 @@ test_expect_success \
  obj=$(git hash-object file_001) &&
  nr=$(index_obj_nr ".git/objects/pack/pack-${pack1}.idx" $obj) &&
  chmod +w ".git/objects/pack/pack-${pack1}.idx" &&
- printf  | dd of=".git/objects/pack/pack-${pack1}.idx" conv=notrunc \
-bs=1 count=4 seek=$((8 + 256 * 4 + $(wc -l <obj-list) * 20 + $nr * 4)) 
&&
+ printf  |
+   test_overwrite_bytes ".git/objects/pack/pack-${pack1}.idx" \
+   $((8 + 256 * 4 + $(wc -l <obj-list) * 20 + $nr * 4)) &&
  ( while read obj
do git cat-file -p $obj >/dev/null || exit 1
done ;
+   open my $fh, "+<", $fname   or die "open $fname: $!\n";
+   seek($fh, $offset, 0)   or die "seek $fname: $!\n";
+   syswrite($fh, $bytes)   or die "write $fname: $!\n";
+   close $fh   or die "close $fname: $!\n";
+   ' "$@"
+}
+
 # The following mingw_* functions obey POSIX shell syntax, but are actually
 # bash scripts, and are meant to be used only with bash on Windows.
 
-- 
2.7.0.118.g90056ae

--
To unsubscribe from this list: send the line "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 v2] test-lib: limit the output of the yes utility

[Johannes Sixt]

From: Johannes Schindelin <johannes.schinde...@gmx.de>

On Windows, there is no SIGPIPE. A consequence of this is that the
upstream process of a pipe does not notice the death of the downstream
process until the pipe buffer is full and writing more data returns an
error. This behavior is the reason for an annoying delay during the
execution of t7610-mergetool.sh: There are a number of test cases where
'yes' is invoked upstream. Since the utility is basically an endless
loop it runs, on Windows, until the pipe buffer is full. This does take
a few seconds.

The test suite has its own implementation of 'yes'. Modify it to produce
only a limited amount of output that is sufficient for the test suite.
The amount chosen should be sufficiently high for any test case, assuming
that future test cases will not exaggerate their demands of input from
an upstream 'yes' invocation.

[j6t: commit message]

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
Am 02.02.2016 um 09:21 schrieb Johannes Schindelin:
> On Tue, 2 Feb 2016, Johannes Sixt wrote:
>> On Windows, there is no SIGPIPE.
> 
> True. But we do get some sort of write failure, no? Otherwise
> https://github.com/git/git/commit/2b86292ed would not work...

Of course. See the second sentence of the commit message.

> I agree with the idea, but I would like to have a less intrusive patch.
> Something like this should do the job as well, and is a little easier on
> the eyes IMHO:

I'm not 100% satisfied with your version because it stomps on
short-and-sweet shell variables $i and $y. But then the utility
only occurs upstream of a pipeline in a separate process, so that
does not matter.

Please allow me to pass authorship to you, since the patch text is
now all yours, and to forge your sign-off.

-- Hannes

 t/test-lib.sh | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bd4b02e..51e4a88 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -907,9 +907,11 @@ yes () {
y="$*"
fi
 
-   while echo "$y"
+   i=0
+   while test $i -lt 99
do
-   :
+   echo "$y"
+   i=$(($i+1))
done
 }
 
-- 
2.7.0.118.g90056ae

--
To unsubscribe from this list: send the line "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] test-lib: limit the output of the yes utility

[Johannes Sixt]

On Windows, there is no SIGPIPE. A consequence of this is that the
upstream process of a pipe does not notice the death of the downstream
process until the pipe buffer is full and writing more data returns an
error. This behavior is the reason for an annoying delay during the
execution of t7610-mergetool.sh: There are a number of test cases where
'yes' is invoked upstream. Since the utility is basically an endless
loop it runs, on Windows, until the pipe buffer is full. This does take
a few seconds.

The test suite has its own implementation of 'yes'. Modify it to produce
only a limited amount of output that is sufficient for the test suite.
The amount chosen should be sufficiently high for any test case, assuming
that future test cases will not exaggerate their demands of input from
an upstream 'yes' invocation.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 This does not fix an error, but only an unnecessary sink of CPU cycles
 and wasted wall clock time.

 t/test-lib.sh | 14 +++---
 1 file changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index bd4b02e..97e6491 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -902,15 +902,15 @@ fi
 yes () {
if test $# = 0
then
-   y=y
+   set -- y
else
-   y="$*"
+   set -- "$*"
fi
-
-   while echo "$y"
-   do
-   :
-   done
+   # we do not need an infinite supply of output for tests
+   set -- "$@" "$@" "$@" "$@"  # 4
+   set -- "$@" "$@" "$@" "$@"  # 16
+   set -- "$@" "$@" "$@" "$@"  # 64
+   printf "%s\n" "$@"
 }
 
 # Fix some commands on Windows
-- 
2.7.0.118.g90056ae

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [bug] Possible Windows 'git mv' bug

[Johannes Sixt]

Am 31.01.2016 um 15:03 schrieb Aaron Gray:

Hi,

I think I have found a possible difference in behaviour between
Windows git commandline distro and Linux git

basically If I do a :-

 git mv logger.h Logger.h

I get the following :-

 fatal: destination exists, source=lib/logger.h, destination=lib/Logger.h

It looks and smells like a bug to me !


Not really. When you attempt to overwrite an existing file with 'git 
mv', you get this error message on both Windows and Linux.


The difference is that logger.h and Logger.h are the same file on 
Windows, but they are not on Linux. Hence, when you attempt to overwrite 
Logger.h on Windows, you see the error because it exists already (as 
logger.h).


As a work-around, you can use -f.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] attempt connects in parallel for IPv6-capable builds

[Johannes Sixt]

Am 29.01.2016 um 02:41 schrieb Eric Wong:

Junio C Hamano  wrote:

Eric Wong  writes:


getaddrinfo() may return multiple addresses, not all of which
are equally performant.  In some cases, a user behind a non-IPv6
capable network may get an IPv6 address which stalls connect().
Instead of waiting synchronously for a connect() to timeout, use
non-blocking connect() in parallel and take the first successful
connection.

This may increase network traffic and server load slightly, but
makes the worst-case user experience more bearable when one
lacks permissions to edit /etc/gai.conf to favor IPv4 addresses.


Umm.  I am not sure what to think about this change--I generally do
not like a selfish "I'll try to use whatever resource given to me
to make my process go faster, screw the rest of the world" approach
and I cannot decide if this falls into that category.

I'll wait for opinions from others.


No problem, I can also make it cheaper for servers to handle
aborted connections in git-daemon:

standalone:

   1) use recv with MSG_PEEK or FIONREAD to determine if there's
  readable data in the socket before forking (and avoid
  forking for zero-bytes-written connections)

   2) use TCP_DEFER_ACCEPT in Linux and dataready filter in FreeBSD
  for standalone git-daemon to delay accept()

inetd:

   3) suppress die("The remote end hung up unexpectedly")
  if no bytes are read at all

At some point in the future, I would love to have git-daemon implement
something like IDLE in IMAP (to avoid having clients poll for updates).
Perhaps the standalone changes above would make sense there, too.


Before you submit a patch in that direction (or resubmit the patch under 
discussion here), could you please find someone to test your patch on 
Windows first? A lot of the infrastructure mentioned may not be 
available there or may not work as expected. (I admit that I'm just 
hand-waving, I haven't tested your patch.)


Thanks,
-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] mingw: avoid linking to the C library's isalpha()

[Johannes Sixt]

The implementation of mingw_skip_dos_drive_prefix() calls isalpha() via
has_dos_drive_prefix(). Since the definition occurs long before isalpha()
is defined in git-compat-util.h, my build environment reports:

CC alloc.o
In file included from git-compat-util.h:186,
 from cache.h:4,
 from alloc.c:12:
compat/mingw.h: In function 'mingw_skip_dos_drive_prefix':
compat/mingw.h:365: warning: implicit declaration of function 'isalpha'

Dscho does not see a similar warning in his build and suspects that
ctype.h is included somehow behind the scenes. This implies that his build
links to the C library's isalpha() and does not use git's isalpha().

To fix both the warning in my build and the inconsistency in Dscho's
build, move the function definition to mingw.c. Then it picks up git's
isalpha() because git-compat-util.h is included at the top of the file.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 compat/mingw.c | 7 +++
 compat/mingw.h | 7 +--
 2 files changed, 8 insertions(+), 6 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
return -1;
 }
 
+int mingw_skip_dos_drive_prefix(char **path)
+{
+   int ret = has_dos_drive_prefix(*path);
+   *path += ret;
+   return ret;
+}
+
 int mingw_offset_1st_component(const char *path)
 {
char *pos = (char *)path;
diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 
 #define has_dos_drive_prefix(path) \
(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-   int ret = has_dos_drive_prefix(*path);
-   *path += ret;
-   return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
 #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
-- 
2.7.0.118.g90056ae

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Sixt]

Am 24.01.2016 um 11:56 schrieb Johannes Schindelin:
> $ grep -w isalpha /mingw32/i686-w64-mingw32/include/*.h
> /mingw32/i686-w64-mingw32/include/ctype.h:  _CRTIMP int __cdecl isalpha(int 
> _C);
> /mingw32/i686-w64-mingw32/include/ctype.h:#define __iscsymf(_c) (isalpha(_c) 
> || ((_c)=='_'))
> 
> I guess that definition gets pulled in somehow.

Ok, then, Junio, kindly replace js/dirname-basename~4 with this patch.
(I hope I get the patch headers right for git-am.)

 8< 
From: Johannes Schindelin <johannes.schinde...@gmx.de>
Subject: [PATCH] Refactor skipping DOS drive prefixes

Junio noticed that there is an implicit assumption in pretty much
all the code calling has_dos_drive_prefix(): it forces all of its
callsites to hardcode the knowledge that the DOS drive prefix is
always two bytes long.

While this assumption is pretty safe, we can still make the code
more readable and less error-prone by introducing a function that
skips the DOS drive prefix safely.

While at it, we change the has_dos_drive_prefix() return value: it
now returns the number of bytes to be skipped if there is a DOS
drive prefix.

[j6t: moved definition of mingw_skip_dos_drive_prefix() out of line]

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 compat/basename.c |  4 +---
 compat/mingw.c| 21 -
 compat/mingw.h|  5 -
 git-compat-util.h |  8 
 path.c| 14 +-
 5 files changed, 30 insertions(+), 22 deletions(-)

diff --git a/compat/basename.c b/compat/basename.c
index d8f8a3c..9f00421 100644
--- a/compat/basename.c
+++ b/compat/basename.c
@@ -4,9 +4,7 @@
 char *gitbasename (char *path)
 {
const char *base;
-   /* Skip over the disk name in MSDOS pathnames. */
-   if (has_dos_drive_prefix(path))
-   path += 2;
+   skip_dos_drive_prefix();
for (base = path; *path; path++) {
if (is_dir_sep(*path))
base = path + 1;
diff --git a/compat/mingw.c b/compat/mingw.c
index f74da23..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,28 +1915,31 @@ pid_t waitpid(pid_t pid, int *status, int options)
return -1;
 }
 
+int mingw_skip_dos_drive_prefix(char **path)
+{
+   int ret = has_dos_drive_prefix(*path);
+   *path += ret;
+   return ret;
+}
+
 int mingw_offset_1st_component(const char *path)
 {
-   int offset = 0;
-   if (has_dos_drive_prefix(path))
-   offset = 2;
+   char *pos = (char *)path;
 
/* unc paths */
-   else if (is_dir_sep(path[0]) && is_dir_sep(path[1])) {
-
+   if (!skip_dos_drive_prefix() &&
+   is_dir_sep(pos[0]) && is_dir_sep(pos[1])) {
/* skip server name */
-   char *pos = strpbrk(path + 2, "\\/");
+   pos = strpbrk(pos + 2, "\\/");
if (!pos)
return 0; /* Error: malformed unc path */
 
do {
pos++;
} while (*pos && !is_dir_sep(*pos));
-
-   offset = pos - path;
}
 
-   return offset + is_dir_sep(path[offset]);
+   return pos + is_dir_sep(*pos) - path;
 }
 
 int xutftowcsn(wchar_t *wcs, const char *utfs, size_t wcslen, int utflen)
diff --git a/compat/mingw.h b/compat/mingw.h
index 738865c..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -358,7 +358,10 @@ HANDLE winansi_get_osfhandle(int fd);
  * git specific compatibility
  */
 
-#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
+#define has_dos_drive_prefix(path) \
+   (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
+int mingw_skip_dos_drive_prefix(char **path);
+#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
diff --git a/git-compat-util.h b/git-compat-util.h
index 0feeae2..38397d7 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -335,6 +335,14 @@ static inline int git_has_dos_drive_prefix(const char 
*path)
 #define has_dos_drive_prefix git_has_dos_drive_prefix
 #endif
 
+#ifndef skip_dos_drive_prefix
+static inline int git_skip_dos_drive_prefix(char **path)
+{
+   return 0;
+}
+#define skip_dos_drive_prefix git_skip_dos_drive_prefix
+#endif
+
 #ifndef is_dir_sep
 static inline int git_is_dir_sep(int c)
 {
diff --git a/path.c b/path.c
index 38f2ebd..747d6da 100644
--- a/path.c
+++ b/path.c
@@ -544,13 +544,10 @@ const char *relative_path(const char *in, const char 
*prefix,
else if (!prefix_len)
return in;
 
-   if (have_same_root(in, prefix)) {
+   if (have_same_root(in, prefix))
/* bypass dos_drive, for "c:" is identical to "C:" */
-   

Re: [PATCH 09/19] mingw: accomodate t0060-path-utils for MSYS2

[Johannes Sixt]

Am 24.01.2016 um 16:44 schrieb Johannes Schindelin:
> On Windows, there are no POSIX paths, only Windows ones (an absolute
> Windows path looks like "C:\Program Files\Git\ReleaseNotes.html", under
> most circumstances, forward slashes are also allowed and synonymous to
> backslashes).
> 
> So when a POSIX shell (such as MSYS2's Bash, which is used by Git for
> Windows to execute all those shell scripts that are part of Git) passes
> a POSIX path to test-path-utils.exe (which is not POSIX-aware), the path
> is translated into a Windows path. For example, /etc/profile becomes
> C:/Program Files/Git/etc/profile.
> 
> This path translation poses a problem when passing the root directory as
> parameter to test-path-utils.exe, as it is not well defined whether the
> translated root directory should end in a slash or not. MSys1 stripped
> the trailing slash, but MSYS2 does not.
> 
> To work with both behaviors, we simply test what the current system does
> in the beginning of t0060-path-utils.sh and then adjust the expected
> longest ancestor length accordingly.
> 
> Originally, the Git for Windows project patched MSYS2's runtime to
> accomodate Git's regression test, but we really should do it the other
> way round.
> 
> Thanks to Ray Donnelly for his patient help with this issue.
> 
> Signed-off-by: Johannes Schindelin 
> ---
>   t/t0060-path-utils.sh | 37 ++---
>   1 file changed, 22 insertions(+), 15 deletions(-)
> 
> diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
> index f0152a7..89d03e7 100755
> --- a/t/t0060-path-utils.sh
> +++ b/t/t0060-path-utils.sh
> @@ -7,6 +7,13 @@ test_description='Test various path utilities'
>   
>   . ./test-lib.sh
>   
> +# On Windows, the root directory "/" is translated into a Windows directory.
> +# As it is not well-defined whether that Windows directory should end in a
> +# slash or not, let's test for that and adjust our expected longest ancestor
> +# length accordingly.
> +root_offset=0
> +case "$(test-path-utils print_path /)" in ?*/) root_offset=-1;; esac
> +
>   norm_path() {
>   expected=$(test-path-utils print_path "$2")
>   test_expect_success $3 "normalize path: $1 => $2" \

In t0060-path-utils.sh, I currently find this:

# On Windows, we are using MSYS's bash, which mangles the paths.
# Absolute paths are anchored at the MSYS installation directory,
# which means that the path / accounts for this many characters:
rootoff=$(test-path-utils normalize_path_copy / | wc -c)
# Account for the trailing LF:
if test $rootoff = 2; then
rootoff=# we are on Unix
else
rootoff=$(($rootoff-1))
fi

ancestor() {
# We do some math with the expected ancestor length.
expected=$3
if test -n "$rootoff" && test "x$expected" != x-1; then
expected=$(($expected+$rootoff))
fi
test_expect_success "longest ancestor: $1 $2 => $expected" \
"actual=\$(test-path-utils longest_ancestor_length '$1' '$2') &&
 test \"\$actual\" = '$expected'"
}

Furthermore, since you are modifying only cases where the expected
value is not -1 and we already have a check for this case in the
helper function, wouldn't it be simpler to merge the offset that your
case needs with the one that we already have?

> @@ -112,30 +119,30 @@ norm_path /d1/.../d2 /d1/.../d2
>   norm_path /d1/..././../d2 /d1/d2
>   
>   ancestor / / -1
> -ancestor /foo / 0
> +ancestor /foo / $root_offset
>   ancestor /foo /fo -1
>   ancestor /foo /foo -1
>   ancestor /foo /bar -1
>   ancestor /foo /foo/bar -1
>   ancestor /foo /foo:/bar -1
> -ancestor /foo /:/foo:/bar 0
> -ancestor /foo /foo:/:/bar 0
> -ancestor /foo /:/bar:/foo 0
> -ancestor /foo/bar / 0
> +ancestor /foo /:/foo:/bar $root_offset
> +ancestor /foo /foo:/:/bar $root_offset
> +ancestor /foo /:/bar:/foo $root_offset
> +ancestor /foo/bar / $root_offset
>   ancestor /foo/bar /fo -1
> -ancestor /foo/bar /foo 4
> +ancestor /foo/bar /foo $((4+$root_offset))
>   ancestor /foo/bar /foo/ba -1
> -ancestor /foo/bar /:/fo 0
> -ancestor /foo/bar /foo:/foo/ba 4
> +ancestor /foo/bar /:/fo $root_offset
> +ancestor /foo/bar /foo:/foo/ba $((4+$root_offset))
>   ancestor /foo/bar /bar -1
>   ancestor /foo/bar /fo -1
> -ancestor /foo/bar /foo:/bar 4
> -ancestor /foo/bar /:/foo:/bar 4
> -ancestor /foo/bar /foo:/:/bar 4
> -ancestor /foo/bar /:/bar:/fo 0
> -ancestor /foo/bar /:/bar 0
> -ancestor /foo/bar /foo 4
> -ancestor /foo/bar /foo:/bar 4
> +ancestor /foo/bar /foo:/bar $((4+$root_offset))
> +ancestor /foo/bar /:/foo:/bar $((4+$root_offset))
> +ancestor /foo/bar /foo:/:/bar $((4+$root_offset))
> +ancestor /foo/bar /:/bar:/fo $root_offset
> +ancestor /foo/bar /:/bar $root_offset
> +ancestor /foo/bar /foo $((4+$root_offset))
> +ancestor /foo/bar /foo:/bar $((4+$root_offset))
>   ancestor /foo/bar /bar -1
>   
>   test_expect_success 'strip_path_suffix' '
> 

--
To unsubscribe from this list: send the line "unsubscribe git" in
the 

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Sixt]

Am 23.01.2016 um 09:25 schrieb Johannes Schindelin:

Hi Junio,

On Fri, 22 Jan 2016, Junio C Hamano wrote:


Johannes Sixt <j...@kdbg.org> writes:


I suggest to move the function definition out of line:

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
return -1;
  }

+int mingw_skip_dos_drive_prefix(char **path)
+{
+   int ret = has_dos_drive_prefix(*path);
+   *path += ret;
+   return ret;
+}
+
  int mingw_offset_1st_component(const char *path)
  {
char *pos = (char *)path;
diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);

  #define has_dos_drive_prefix(path) \
(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-   int ret = has_dos_drive_prefix(*path);
-   *path += ret;
-   return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
  #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
  #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
  static inline char *mingw_find_last_dir_sep(const char *path)


This sounds good to me.  Dscho?


Yep, sounds good to me, too.

Personally, I have no inclination to add compatibility with the
now-safely-obsolete MSys to my responsibilities, but if Hannes wants to do
it, who am I to stand in his way? Especially when the fix is as trivial as
here.


This is not a matter of compatibility. I am VERY curious why you do not 
see an error (or warning) without my proposed fixup. As I mentioned, 
isalpha() is defined much later than the definition of 
mingw_skip_dos_drive_prefix(). Where does your build get a declaration 
of isalpha() from?


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/4] Refactor skipping DOS drive prefixes

[Johannes Sixt]

Am 12.01.2016 um 08:57 schrieb Johannes Schindelin:
> diff --git a/compat/mingw.h b/compat/mingw.h
> index 57ca477..b3e5044 100644
> --- a/compat/mingw.h
> +++ b/compat/mingw.h
> @@ -361,7 +361,15 @@ HANDLE winansi_get_osfhandle(int fd);
>* git specific compatibility
>*/
>   
> -#define has_dos_drive_prefix(path) (isalpha(*(path)) && (path)[1] == ':')
> +#define has_dos_drive_prefix(path) \
> + (isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
> +static inline int mingw_skip_dos_drive_prefix(char **path)
> +{
> + int ret = has_dos_drive_prefix(*path);
> + *path += ret;
> + return ret;
> +}
> +#define skip_dos_drive_prefix mingw_skip_dos_drive_prefix

This triggers

CC alloc.o
In file included from git-compat-util.h:186,
 from cache.h:4,
 from alloc.c:12:
compat/mingw.h: In function 'mingw_skip_dos_drive_prefix':
compat/mingw.h:365: warning: implicit declaration of function 'isalpha'

when I build under the old MSYS environment. While I would understand
that the old MSYS environment is end-of-lifed and not worth your time
catering to, the error is still an indication of a problem.

Notice that mingw.h is #included in line 186 of git-compat-util.h,
isalpha is only (re-)defined much later in line 790. That would explain
the warning. What I do not understand is that you do not observe the
same warning in your MSYS2/MINGWxx environment. It would mean that
 is included somewhere.

At any rate, the resulting binary sometimes uses an isalpha
implementation other than the one provided in git-compat-util.h. The
result is most likely correct, but it is certainly not the intent,
is it?

I did not attempt to build with MSVC, but it is not unlikely that it
shows the same error.

I suggest to move the function definition out of line:

diff --git a/compat/mingw.c b/compat/mingw.c
index 10a51c0..0cebb61 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1915,6 +1915,13 @@ pid_t waitpid(pid_t pid, int *status, int options)
return -1;
 }
 
+int mingw_skip_dos_drive_prefix(char **path)
+{
+   int ret = has_dos_drive_prefix(*path);
+   *path += ret;
+   return ret;
+}
+
 int mingw_offset_1st_component(const char *path)
 {
char *pos = (char *)path;
diff --git a/compat/mingw.h b/compat/mingw.h
index 9b5db4e..2099b79 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -360,12 +360,7 @@ HANDLE winansi_get_osfhandle(int fd);
 
 #define has_dos_drive_prefix(path) \
(isalpha(*(path)) && (path)[1] == ':' ? 2 : 0)
-static inline int mingw_skip_dos_drive_prefix(char **path)
-{
-   int ret = has_dos_drive_prefix(*path);
-   *path += ret;
-   return ret;
-}
+int mingw_skip_dos_drive_prefix(char **path);
 #define skip_dos_drive_prefix mingw_skip_dos_drive_prefix
 #define is_dir_sep(c) ((c) == '/' || (c) == '\\')
 static inline char *mingw_find_last_dir_sep(const char *path)

--
To unsubscribe from this list: send the line "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: [PATCHv2 1/2] submodule: port resolve_relative_url from shell to C

[Johannes Sixt]

Am 20.01.2016 um 03:03 schrieb Stefan Beller:
> +static char *last_dir_separator(char *str)
> +{
> + char *p = str + strlen(str);
> + while (p-- > str)
> + if (is_dir_sep(*p))
> + return p;
> + return NULL;
> +}
> +

I just noticed that we already have this function. Please squash in the
following.

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1484b36..92d7d32 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -51,21 +51,12 @@ static int starts_with_dot_dot_slash(const char *str)
return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);
 }
 
-static char *last_dir_separator(char *str)
-{
-   char *p = str + strlen(str);
-   while (p-- > str)
-   if (is_dir_sep(*p))
-   return p;
-   return NULL;
-}
-
 /*
  * Returns 1 if it was the last chop before ':'.
  */
 static int chop_last_dir(char **remoteurl, int is_relative)
 {
-   char *rfind = last_dir_separator(*remoteurl);
+   char *rfind = find_last_dir_sep(*remoteurl);
if (rfind) {
*rfind = '\0';
return 0;

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

jc/rerere-multi (was: What's cooking in git.git (Jan 2016, #04; Wed, 20))

[Johannes Sixt]

Am 21.01.2016 um 00:33 schrieb Junio C Hamano:
> * jc/rerere-multi (2015-09-14) 7 commits
>   - rerere: do use multiple variants
>   - t4200: rerere a merge with two identical conflicts
>   - rerere: allow multiple variants to exist
>   - rerere: delay the recording of preimage
>   - rerere: handle leftover rr-cache/$ID directory and postimage files
>   - rerere: scan $GIT_DIR/rr-cache/$ID when instantiating a rerere_id
>   - rerere: split conflict ID further
> 
>   "git rerere" can encounter two or more files with the same conflict
>   signature that have to be resolved in different ways, but there was
>   no way to record these separate resolutions.
> 
>   Needs review.

I finally found some time to test and review this series. I have one
case where there are many identical conflicts (up to 15!) that rerere
was unable to resolve. But with this series applied, all of them are
now resolved automatically and correctly. That's a nice achievement!

Tested-by: Johannes Sixt <j...@kdbg.org>

I don't have the original submission anymore. So, I'm responding here.

Generally, the patches make sense.

Except for 510936082eb4 "handle leftover rr-cache/$ID directory and
postimage files": After the subsequent e2a6344cca47 "delay the
recording of preimage" is in place, nothing of what the former patch
changed (except test cases) remains, and the problem that the former
solved is still solved, and in addition the NEEDSWORK that the former
introduced is resolved by the latter. I think the two should be
squashed together.

e2a6344cca47 (rerere: delay the recording of preimage) needs this
fixup, I think:

diff --git a/rerere.c b/rerere.c
index c0482b8..33b1348 100644
--- a/rerere.c
+++ b/rerere.c
@@ -765,7 +765,7 @@ static void do_rerere_one_path(struct string_list_item 
*rr_item,
const char *path = rerere_path(id, "postimage");
if (unlink(path))
die_errno("cannot unlink stray '%s'", path);
-   id->collection->status &= ~RR_HAS_PREIMAGE;
+   id->collection->status &= ~RR_HAS_POSTIMAGE;
}
id->collection->status |= RR_HAS_PREIMAGE;
fprintf(stderr, "Recorded preimage for '%s'\n", path);

and perhaps this change:

diff --git a/rerere.c b/rerere.c
index fbdade8..df6beb9 100644
--- a/rerere.c
+++ b/rerere.c
@@ -1005,11 +1005,6 @@ static void unlink_rr_item(struct rerere_id *id)
unlink(rerere_path(id, "thisimage"));
unlink(rerere_path(id, "preimage"));
unlink(rerere_path(id, "postimage"));
-   /*
-* NEEDSWORK: what if this rmdir() fails?  Wouldn't we then
-* assume that we already have preimage recorded in
-* do_plain_rerere()?
-*/
rmdir(rerere_path(id, NULL));
 }
 

--
To unsubscribe from this list: send the line "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/10] t5100-mailinfo.sh: use the $( ... ) construct for command substitution

[Johannes Sixt]

Am 28.12.2015 um 02:59 schrieb Junio C Hamano:

Junio C Hamano <gits...@pobox.com> writes:


Very true.  Let's leave that kind of things as separate clean-ups
after these patches settle, as mixing manual and mechanical changes
in a single patch makes it harder to review.

Thanks.


So that I won't forget (I'll need to amend with your sign-off even
if this one is satisfactory to you).

-- >8 --
From: Johannes Sixt <j...@kdbg.org>
Date: Tue, 22 Dec 2015 19:35:16 +0100
Subject: [PATCH] t5100: no need to use 'echo' command substitutions for globbing

Instead of making the shell expand 00* and invoke 'echo' with it,
and then capturing its output as command substitution, just use
the result of expanding 00* directly.

Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
  t/t5100-mailinfo.sh | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh
index 4e52b3b..85b3df5 100755
--- a/t/t5100-mailinfo.sh
+++ b/t/t5100-mailinfo.sh
@@ -23,7 +23,7 @@ check_mailinfo () {
  }


-for mail in $(echo 00*)
+for mail in 00*
  do
test_expect_success "mailinfo $mail" '
check_mailinfo $mail "" &&
@@ -51,7 +51,7 @@ test_expect_success 'split box with rfc2047 samples' \
echo total is $last &&
test $(cat rfc2047/last) = 11'

-for mail in $(echo rfc2047/00*)
+for mail in rfc2047/00*
  do
test_expect_success "mailinfo $mail" '
git mailinfo -u $mail-msg $mail-patch <$mail >$mail-info &&



Signed-off-by: Johannes Sixt <j...@kdbg.org>

Thank you for making a proper patch!

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 0/3] nd/clear-gitenv-upon-use-of-alias

[Johannes Sixt]

Am 23.12.2015 um 10:37 schrieb Jeff King:

The second line comes from handle_alias itself. It calls
die_errno whenever run_command returns a negative value.
However, only -1 indicates a syscall error where errno has
something useful (note that it says the useless "success"
above). Instead, we treat negative returns from run_command
(except for -1) as a normal code to be passed to exit.

Signed-off-by: Jeff King 
---
  git.c | 2 +-
  run-command.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git.c b/git.c
index 6ed824c..34a18a3 100644
--- a/git.c
+++ b/git.c
@@ -252,7 +252,7 @@ static int handle_alias(int *argcp, const char ***argv)
alias_argv[argc] = NULL;

ret = run_command_v_opt(alias_argv, RUN_USING_SHELL);
-   if (ret >= 0)   /* normal exit */
+   if (ret != -1)  /* normal exit */


Why does this make a difference? We only ever return -1, zero, or a 
positive value from run_command/finish_command/wait_or_whine, as far as 
I can see.



exit(ret);

die_errno("While expanding alias '%s': '%s'",


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] t5100-mailinfo.sh: use the $( ... ) construct for command substitution

[Johannes Sixt]

Am 22.12.2015 um 16:27 schrieb Elia Pinto:

-for mail in `echo 00*`
+for mail in $(echo 00*)



-for mail in `echo rfc2047/00*`
+for mail in $(echo rfc2047/00*)


True, these are equvalence transformations. But a better way to get rid 
of the back-quotes is to write these lines as


for mail in echo 00*
for mail in echo rfc2047/00*

No?

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 07/10] t5100-mailinfo.sh: use the $( ... ) construct for command substitution

[Johannes Sixt]

Am 22.12.2015 um 19:35 schrieb Johannes Sixt:

Am 22.12.2015 um 16:27 schrieb Elia Pinto:

-for mail in `echo 00*`
+for mail in $(echo 00*)



-for mail in `echo rfc2047/00*`
+for mail in $(echo rfc2047/00*)


True, these are equvalence transformations. But a better way to get rid
of the back-quotes is to write these lines as

for mail in echo 00*
for mail in echo rfc2047/00*


Ahem... both of these lines without the 'echo', of course!



No?

-- Hannes



--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Odd rebase behavior

[Johannes Sixt]

Am 18.12.2015 um 19:05 schrieb John Keeping:

On Fri, Dec 18, 2015 at 11:43:16AM -0600, David A. Greene wrote:

John Keeping  writes:


It seems that the problem is introduces by --preserve-merges (and
-Xsubtree causes something interesting to happen as well).  I see the
following behaviour:


Thanks for narrowing this down!  Is it possible this is actually a
cherry-pick problem since --preserve-merges forces rebase to use
cherry-pick?


I'm pretty sure this a result of the code in git-rebase--interactive.sh
just below the comment "Watch for commits that have been dropped by
cherry-pick", which filters out certain commits.  However, I'm not at
all familiar with the --preserve-merges code in git-rebase so I could be
completely wrong.


git rebase -Xsubtree=files_subtree --onto files-master master

fatal: Could not parse object 
'b15c4133fc3146e1330c84159886f0f7a09fbf43^'
Unknown exit code (128) from command: git-merge-recursive
b15c4133fc3146e1330c84159886f0f7a09fbf43^ -- HEAD
b15c4133fc3146e1330c84159886f0f7a09fbf43


Ah, good!  I had seen this behavior as well but couldn't remember what I
did to trigger it.

I don't think I have the expertise to fix rebase and/or cherry-pick.
What's the process for adding these tests to the testbase and marking
them so the appropriate person can fix them?  I see a lot of TODO tests.
Should I mark these similarly and propose a patch to the testbase?


I think marking them with test_expect_failure (instead of
test_expect_success) is enough.



There are a few known breakages recorded in 
t3512-cherry-pick-submodule.sh. Perhaps the one observed here is already 
among them?


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] mingw: emulate write(2) that fails with a EPIPE

[Johannes Sixt]

Am 17.12.2015 um 18:08 schrieb Johannes Schindelin:

On Windows, when writing to a pipe fails, errno is always
EINVAL. However, Git expects it to be EPIPE.

According to the documentation, there are two cases in which write()
triggers EINVAL: the buffer is NULL, or the length is odd but the mode
is 16-bit Unicode (the broken pipe is not mentioned as possible cause).
Git never sets the file mode to anything but binary, therefore we know
that errno should actually be EPIPE if it is EINVAL and the buffer is
not NULL.

See https://msdn.microsoft.com/en-us/library/1570wh78.aspx for more
details.

This works around t5571.11 failing with v2.6.4 on Windows.

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
  compat/mingw.c | 17 +
  compat/mingw.h |  3 +++
  2 files changed, 20 insertions(+)

diff --git a/compat/mingw.c b/compat/mingw.c
index 90bdb1e..5edea29 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -394,6 +394,23 @@ int mingw_fflush(FILE *stream)
return ret;
  }

+#undef write
+ssize_t mingw_write(int fd, const void *buf, size_t len)
+{
+   ssize_t result = write(fd, buf, len);
+
+   if (result < 0 && errno == EINVAL && buf) {
+   /* check if fd is a pipe */
+   HANDLE h = (HANDLE) _get_osfhandle(fd);
+   if (GetFileType(h) == FILE_TYPE_PIPE)
+   errno = EPIPE;
+   else
+   errno = EINVAL;
+   }
+
+   return result;
+}
+
  int mingw_access(const char *filename, int mode)
  {
wchar_t wfilename[MAX_PATH];


Looks good. I tested the patch, and it fixes the failure exposed by 
t5571.11.


Acked-by: Johannes Sixt <j...@kdbg.org>

Thanks!

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] submodule: Port resolve_relative_url from shell to C

[Johannes Sixt]

Am 17.12.2015 um 01:26 schrieb Stefan Beller:
> This reimplements the helper function `resolve_relative_url` in shell
> in C. This functionality is needed in C for introducing the groups
> feature later on. When using groups, the user should not need to run
> `git submodule init`, but it should be implicit at all appropriate places,
> which are all in C code. As the we would not just call out to `git
> submodule init`, but do a more fine grained structure there, we actually
> need all the init functionality in C before attempting the groups
> feature. To get the init functionality in C, rewriting the
> resolve_relative_url subfunction is a major step.
>
> This also improves the performance:
> (Best out of 3) time ./t7400-submodule-basic.sh
> Before:
> real  0m9.575s
> user  0m2.683s
> sys   0m6.773s
> After:
> real  0m9.293s
> user  0m2.691s
> sys   0m6.549s
>
> That's about 3%.

I appreciate this effort as it should help us on Windows. Although the
numbers (and my own timings) suggest that this is only a small step
forward. That's not surprising as the patch removes only two forks.

As to the implementation, find a patch below that removes the ifdefs
and a few other suggestions. It is a mechanical conversion without
understanding what relative_url() does. I have the gut feeling that the
two strbuf_addf towards the end of the function can be contracted and
the temporarily allocate copy in 'out' can be removed.

If there were a few examples in the comment above the function, it
would be much simpler to understand.

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index b925bed..8ec0975 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -11,6 +11,7 @@
 #include "run-command.h"
 #include "remote.h"
 #include "refs.h"
+#include "connect.h"
 
 static const char *get_default_remote(void)
 {
@@ -31,34 +32,23 @@ static const char *get_default_remote(void)
return xstrdup(dest);
 }
 
-static int has_same_dir_prefix(const char *str, const char **out)
+static int starts_with_dot_slash(const char *str)
 {
-#ifdef GIT_WINDOWS_NATIVE
-   return skip_prefix(str, "./", out)
-   || skip_prefix(str, ".\\", out);
-#else
-   return skip_prefix(str, "./", out);
-#endif
+   return str[0] == '.' && is_dir_sep(str[1]);
 }
 
-static int has_upper_dir_prefix(const char *str, const char **out)
+static int starts_with_dot_dot_slash(const char *str)
 {
-#ifdef GIT_WINDOWS_NATIVE
-   return skip_prefix(str, "../", out)
-   || skip_prefix(str, "..\\", out);
-#else
-   return skip_prefix(str, "../", out);
-#endif
+   return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);
 }
 
-static char *last_dir_separator(const char *str)
+static char *last_dir_separator(char *str)
 {
-#ifdef GIT_WINDOWS_NATIVE
-   return strrchr(str, "/")
-   || strrchr(str, "\\");
-#else
-   return strrchr(str, '/');
-#endif
+   char* p = str + strlen(str);
+   while (p-- != str)
+   if (is_dir_sep(*p))
+   return p;
+   return NULL;
 }
 
 /*
@@ -85,9 +75,10 @@ static const char *relative_url(const char *url, const char 
*up_path)
size_t len;
char *remoteurl = NULL;
char *sep = "/";
-   const char *out;
+   char *out;
struct strbuf sb = STRBUF_INIT;
const char *remote = get_default_remote();
+
strbuf_addf(, "remote.%s.url", remote);
 
if (git_config_get_string(sb.buf, ))
@@ -98,22 +89,23 @@ static const char *relative_url(const char *url, const char 
*up_path)
if (is_dir_sep(remoteurl[len]))
remoteurl[len] = '\0';
 
-   if (strchr(remoteurl, ':') || is_dir_sep(*remoteurl))
+   if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
-   else if (has_same_dir_prefix(remoteurl, ) ||
-   has_upper_dir_prefix(remoteurl, ))
+   else if (starts_with_dot_slash(remoteurl) ||
+   starts_with_dot_dot_slash(remoteurl))
is_relative = 1;
else {
is_relative = 1;
strbuf_reset();
strbuf_addf(, "./%s", remoteurl);
+   free(remoteurl);
remoteurl = strbuf_detach(, NULL);
}
 
while (url) {
-   if (has_upper_dir_prefix(url, )) {
+   if (starts_with_dot_dot_slash(url)) {
char *rfind;
-   url = out;
+   url += 3;
 
rfind = last_dir_separator(remoteurl);
if (rfind)
@@ -130,22 +122,23 @@ static const char *relative_url(const char *url, const 
char *up_path)
remoteurl = ".";
}
}
-   } else if (has_same_dir_prefix(url, ))
-   url = out;
-   

Re: [PATCHv2] submodule: Port resolve_relative_url from shell to C

[Johannes Sixt]

Am 17.12.2015 um 19:55 schrieb Johannes Sixt:

As to the implementation, find a patch below that removes the ifdefs
and a few other suggestions.


The word "offers" is missing in this last half-sentence ;)

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-gui: do not use obsolete `git merge $msg HEAD $branch'

[Johannes Sixt]

Use the modern `git merge' invocation pattern.

Since both `git merge' and `git fmt-merge-msg' obey the merge.log
configuration, instruct the former not to generate the log summary to
avoid that it appears twice in the commit message.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 lib/merge.tcl | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/lib/merge.tcl b/lib/merge.tcl
index 460d32f..f512456 100644
--- a/lib/merge.tcl
+++ b/lib/merge.tcl
@@ -115,8 +115,9 @@ method _start {} {
set cmd [list git]
lappend cmd merge
lappend cmd --strategy=recursive
+   lappend cmd --no-log
+   lappend cmd -m
lappend cmd [git fmt-merge-msg <[gitdir FETCH_HEAD]]
-   lappend cmd HEAD
lappend cmd $name
 
ui_status [mc "Merging %s and %s..." $current_branch $stitle]
-- 
2.6.2.337.ga235d84

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/8] xread_nonblock: add functionality to read from fds without blocking

[Johannes Sixt]

Am 15.12.2015 um 01:25 schrieb Stefan Beller:

I was actually thinking about using {without-x}read, just the plain system call.
Do we have any issues with that for wrapping purposes for Windows?


xread() limits the size being read to MAX_IO_SIZE, which is needed on 
some systems (I think that some Windows configurations did fall into 
this category).


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 0/8] Rerolling sb/submodule-parallel-fetch for the time after 2.7

[Johannes Sixt]

Am 14.12.2015 um 20:37 schrieb Stefan Beller:

I am sending out a new version for replacing sb/submodule-parallel-fetch for
the time after the 2.7 release.

The content are
  * all patches as in the branch sb/submodule-parallel-fetch
  * inlcuding the fixups as suggested by Hannes,
  * write a message to the debug log for better testing and debugging purposes
   (a patch cherry picked from the series which is supposed to build on top of 
this)

The patches themselves were rebased such that there are no fixup commits
any more, but we get things right the first time.

The commit message of "run-command: add an asynchronous parallel child 
processor"
has slightly been updated to mention the fact that we don't want to use 
waitpid(-1)
but rather use the assumption of child's stderr living as long as the child 
itself.


Thanks! I rebased a version of sb/submodule-parallel-fetch that includes 
my suggested improvements, and the result is identical to this series 
except for the trace output mentioned in the last bullet point.


With or without addressing my note about the commit message in 6/8:

Acked-by: Johannes Sixt <j...@kdbg.org>


Thanks,
Stefan


Jonathan Nieder (1):
   submodule.c: write "Fetching submodule " to stderr

Stefan Beller (7):
   xread: poll on non blocking fds
   xread_nonblock: add functionality to read from fds without blocking
   strbuf: add strbuf_read_once to read without blocking
   sigchain: add command to pop all common signals
   run-command: add an asynchronous parallel child processor
   fetch_populated_submodules: use new parallel job processing
   submodules: allow parallel fetching, add tests and documentation

  Documentation/fetch-options.txt |   7 +
  builtin/fetch.c |   6 +-
  builtin/pull.c  |   6 +
  git-compat-util.h   |   1 +
  run-command.c   | 335 
  run-command.h   |  80 ++
  sigchain.c  |   9 ++
  sigchain.h  |   1 +
  strbuf.c|  11 ++
  strbuf.h|   8 +
  submodule.c | 141 +++--
  submodule.h |   2 +-
  t/t0061-run-command.sh  |  53 +++
  t/t5526-fetch-submodules.sh |  71 ++---
  test-run-command.c  |  55 ++-
  wrapper.c   |  35 -
  16 files changed, 747 insertions(+), 74 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: [PATCH 6/8] run-command: add an asynchronous parallel child processor

[Johannes Sixt]

Am 14.12.2015 um 20:37 schrieb Stefan Beller:

This allows to run external commands in parallel with ordered output
on stderr.

If we run external commands in parallel we cannot pipe the output directly
to the our stdout/err as it would mix up. So each process's output will
flow through a pipe, which we buffer. One subprocess can be directly
piped to out stdout/err for a low latency feedback to the user.

Example:
Let's assume we have 5 submodules A,B,C,D,E and each fetch takes a
different amount of time as the different submodules vary in size, then
the output of fetches in sequential order might look like this:

  time -->
  output: |---A---| |-B-| |---C---| |-D-| |-E-|

When we schedule these submodules into maximal two parallel processes,
a schedule and sample output over time may look like this:

process 1: |---A---| |-D-| |-E-|

process 2: |-B-| |---C---|

output:|---A---|B|---C---|DE

So A will be perceived as it would run normally in the single child
version. As B has finished by the time A is done, we can dump its whole
progress buffer on stderr, such that it looks like it finished in no
time. Once that is done, C is determined to be the visible child and
its progress will be reported in real time.

So this way of output is really good for human consumption, as it only
changes the timing, not the actual output.

For machine consumption the output needs to be prepared in the tasks,
by either having a prefix per line or per block to indicate whose tasks
output is displayed, because the output order may not follow the
original sequential ordering:

  |A| |--B--| |-C-|

will be scheduled to be all parallel:

process 1: |A|
process 2: |--B--|
process 3: |-C-|
output:|A|CB

This happens because C finished before B did, so it will be queued for
output before B.

The detection when a child has finished executing is done the same way as
two fold. First we check regularly if the stderr pipe still exists in an
interleaved manner with other actions such as checking other children
for their liveliness or starting new children. Once a child closed their
stderr stream, we assume it is stopping very soon, such that we can use
the `finish_command` code borrowed from the single external process
execution interface.


I can't quite parse the first sentence in this paragraph. Perhaps 
something like this:


To detect when a child has finished executing, we check interleaved
with other actions (such as checking the liveliness of children or
starting new processes) whether the stderr pipe still exists. Once a
child closed its stderr stream, we assume it is terminating very soon,
and use finish_command() from the single external process execution
interface to collect the exit status.



By maintaining the strong assumption of stderr being open until the
very end of a child process, we can avoid other hassle such as an
implementation using `waitpid(-1)`, which is not implemented in Windows.

Signed-off-by: Stefan Beller 


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

sb/submodule-parallel-fetch, was: Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Johannes Sixt]

Am 11.12.2015 um 00:55 schrieb Stefan Beller:
> On Thu, Dec 10, 2015 at 3:51 PM, Junio C Hamano  wrote:
>> Stefan Beller  writes:
>>
 * sb/submodule-parallel-fetch (2015-11-24) 17 commits
 ...
>>>
>>> I assume you plan on merging this after 2.7 settled and then we can
>>> also get the above sb/submodule-parallel-update going again.
>>
>> Yeah, thanks for reminding me.  I think that would be a good plan
>> that gives us an opportunity to clean up this topic, some parts of
>> which are have "an early patch that was too hastily merged to 'next'
>> had to be tweaked by an 'oops' follow-up patch in the topic"
>> pattern, e.g. "make waitpid the secondary and closed pipe the
>> primary way to monitor children".
>>
>> Thanks.
> 
> This makes it sound as if you would drop it from next once 2.7 is out,
> expecting a complete reroll, which does the right thing from the beginning?
> That was not was I was expecting, but thanks for clarifying.

Also, rebuilding the topic such that it takes the direct route to its
current state would help bisectability on Windows.

Generally, I'm already quite satisfied with the state of the
infrastructure at the tip of the branch.

Nevertheless, I have a few niggles.

The primary one is that we are using a global variable of type
struct parallel_processes to keep track of the processes. Fortunately,
most functions already take a pointer (I gather you did anticipate an
object oriented use of the functions). The only exception is pp_init():
It returns a pointer to the global object, which is then passed around
to the other functions. This does not conform to our usual style,
however, where the initializer function takes a pointer to the object,
too.

After converting pp_init, we can have a nicely object oriented
collection of functions and get rid of the global object ... almost.

We still need a global variable for the signal handler. But since
signals and their handlers are a global resource, it is not that bad to
have a global variable that is dedicated to signal handling.

Another small nit is that I found it confusing that the closure
parameters are arranged differently in the callback functions. Granted,
in get_next_task one of them is an out parameter, but that is actually
an argument to move it to the end of the parameter list, IMHO.

On top of that I found an error or two in the documentation, and I have
a few suggestions for improvements.

All this is summarized in the patch below.

diff --git a/run-command.c b/run-command.c
index db4d916..f3addb9 100644
--- a/run-command.c
+++ b/run-command.c
@@ -864,7 +864,7 @@ enum child_state {
GIT_CP_WAIT_CLEANUP,
 };
 
-static struct parallel_processes {
+struct parallel_processes {
void *data;
 
int max_processes;
@@ -890,7 +890,7 @@ static struct parallel_processes {
 
int output_owner;
struct strbuf buffered_output; /* of finished children */
-} parallel_processes_struct;
+};
 
 static int default_start_failure(struct child_process *cp,
 struct strbuf *err,
@@ -933,23 +933,23 @@ static void kill_children(struct parallel_processes *pp, 
int signo)
kill(pp->children[i].process.pid, signo);
 }
 
+static struct parallel_processes *pp_for_signal;
+
 static void handle_children_on_signal(int signo)
 {
-   struct parallel_processes *pp = _processes_struct;
-
-   kill_children(pp, signo);
+   kill_children(pp_for_signal, signo);
sigchain_pop(signo);
raise(signo);
 }
 
-static struct parallel_processes *pp_init(int n,
- get_next_task_fn get_next_task,
- start_failure_fn start_failure,
- task_finished_fn task_finished,
- void *data)
+static void pp_init(struct parallel_processes *pp,
+   int n,
+   get_next_task_fn get_next_task,
+   start_failure_fn start_failure,
+   task_finished_fn task_finished,
+   void *data)
 {
int i;
-   struct parallel_processes *pp = _processes_struct;
 
if (n < 1)
n = online_cpus();
@@ -976,8 +976,9 @@ static struct parallel_processes *pp_init(int n,
pp->pfd[i].events = POLLIN | POLLHUP;
pp->pfd[i].fd = -1;
}
+
+   pp_for_signal = pp;
sigchain_push_common(handle_children_on_signal);
-   return pp;
 }
 
 static void pp_cleanup(struct parallel_processes *pp)
@@ -1019,10 +1020,10 @@ static int pp_start_one(struct parallel_processes *pp)
if (i == pp->max_processes)
die("BUG: bookkeeping is hard");
 
-   code = pp->get_next_task(>children[i].data,
->children[i].process,
+   code = pp->get_next_task(>children[i].process,
 

Re: sb/submodule-parallel-fetch, was: Re: What's cooking in git.git (Dec 2015, #03; Thu, 10)

[Johannes Sixt]

Am 11.12.2015 um 23:52 schrieb Stefan Beller:

If you don't mind I'll split your patch into chunks and
squash these where appropriate


I don't mind at all; please pick what you like.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule: Port resolve_relative_url from shell to C

[Johannes Sixt]

Am 10.12.2015 um 02:07 schrieb Stefan Beller:

This reimplements the helper function `resolve_relative_url` in shell
in C. This functionality is needed in C for introducing the groups
feature later on. When using groups, the user should not need to run
`git submodule init`, but it should be implicit at all appropriate places,
which are all in C code. As the we would not just call out to `git
submodule init`, but do a more fine grained structure there, we actually
need all the init functionality in C before attempting the groups
feature. To get the init functionality in C, rewriting the
resolve_relative_url subfunction is a major step.


I see lots of '/', but no is_dir_sep() in the C version. Did you 
consider that local URLs can use a backslash as path separator on 
Windows? In the shell version, this did not matter because bash converts 
the backslashes to forward slashes for us. But when rewritten in C, this 
does not happen.


Valid URLs are

  D:\foo\bar.git
  \\server\share\foo\bar
  ..\..\foo\bar

and all of them with some or all backslashes replaced by forward slashes.

See also connect.c:url_is_local_not_ssh, which ensures that the first 
example above is considered a local path with a drive letter, not a 
remote ssh path.




This also improves the performance:
(Best out of 3) time ./t7400-submodule-basic.sh
Before:
real0m9.575s
user0m2.683s
sys 0m6.773s
After:
real0m9.293s
user0m2.691s
sys 0m6.549s

Signed-off-by: Stefan Beller 
---

  This applies on origin/master, and I'd carry as its own feature branch
  as I am nowhere near done with the groups feature after reading Jens feedback.
  (It took me a while to identify this as a next best step.)

  Thanks,
  Stefan

  builtin/submodule--helper.c | 120 
  git-submodule.sh|  81 ++
  2 files changed, 124 insertions(+), 77 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..f48b5b5 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -9,6 +9,125 @@
  #include "submodule-config.h"
  #include "string-list.h"
  #include "run-command.h"
+#include "remote.h"
+#include "refs.h"
+
+static const char *get_default_remote(void)
+{
+   char *dest = NULL;
+   unsigned char sha1[20];
+   int flag;
+   struct strbuf sb = STRBUF_INIT;
+   const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, );
+
+   if (!refname)
+   die("No such ref: HEAD");
+
+   refname = shorten_unambiguous_ref(refname, 0);
+   strbuf_addf(, "branch.%s.remote", refname);
+   if (git_config_get_string(sb.buf, ))
+   return "origin";
+   else
+   return xstrdup(dest);
+}
+
+/*
+ * The function takes at most 2 arguments. The first argument is the
+ * URL that navigates to the submodule origin repo. When relative, this URL
+ * is relative to the superproject origin URL repo. The second up_path
+ * argument, if specified, is the relative path that navigates
+ * from the submodule working tree to the superproject working tree.
+ *
+ * The output of the function is the origin URL of the submodule.
+ *
+ * The output will either be an absolute URL or filesystem path (if the
+ * superproject origin URL is an absolute URL or filesystem path,
+ * respectively) or a relative file system path (if the superproject
+ * origin URL is a relative file system path).
+ *
+ * When the output is a relative file system path, the path is either
+ * relative to the submodule working tree, if up_path is specified, or to
+ * the superproject working tree otherwise.
+ */
+static const char *relative_url(const char *url, const char *up_path)
+{
+   int is_relative = 0;
+   size_t len;
+   char *remoteurl = NULL;
+   char *sep = "/";
+   const char *out;
+   struct strbuf sb = STRBUF_INIT;
+   const char *remote = get_default_remote();
+   strbuf_addf(, "remote.%s.url", remote);
+
+   if (git_config_get_string(sb.buf, ))
+   /* the repository is its own authoritative upstream */
+   remoteurl = xgetcwd();
+
+   if (strip_suffix(remoteurl, "/", ))
+   remoteurl[len] = '\0';
+
+   if (strchr(remoteurl, ':') || skip_prefix(remoteurl, "/", ))
+   is_relative = 0;
+   else if (skip_prefix(remoteurl, "./", ) ||
+   skip_prefix(remoteurl, "../", ))
+   is_relative = 1;
+   else {
+   is_relative = 1;
+   strbuf_reset();
+   strbuf_addf(, "./%s", remoteurl);
+   remoteurl = strbuf_detach(, NULL);
+   }
+
+   while (url) {
+   if (skip_prefix(url, "../", )) {
+   char *rfind;
+   url = out;
+
+   rfind = strrchr(remoteurl, '/');
+   if (rfind)
+   *rfind = 

Re: [PATCH v2] revision.c: fix possible null pointer access

[Johannes Sixt]

Am 07.12.2015 um 21:31 schrieb Junio C Hamano:

Stefan Naewe  writes:


mark_tree_uninteresting dereferences a tree pointer before checking
if the pointer is valid. Fix that by doing the check first.

Signed-off-by: Stefan Naewe 
---


I still have a problem with "dereferences", as "dereference" is
about computing an address and accessing memory based on the result,
and only the first half is happening here.  I can live with "The
function does a pointer arithmetic on 'tree' before it makes sure
that 'tree' is not NULL", but in any case, let's queue this as-is
for now and wait for a while to see if others can come up with a
more appropriate phrases.


Don't shoo away language lawyers, because this is a pure C language rule 
patch. If this were only about pointer arithmetic, a change would not be 
necessary. But it isn't. The patch corrects a case where the compiler 
can remove a NULL pointer check that we actually want to remain. The 
language rule that gives sufficient room for interpretation to the 
compiler is about dereferencing a pointer. It is irrelevant that an 
address of an object is taken after the dereference and then only 
pointer arithmetic remains---the dereference has already taken place, 
and that cannot occur for a NULL pointer in a valid program. So, the 
phrase "dereference" is precise and correct here.


-- Hannes



Thanks.


  revision.c | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index 0fbb684..8c569cc 100644
--- a/revision.c
+++ b/revision.c
@@ -135,10 +135,12 @@ static void mark_tree_contents_uninteresting(struct tree 
*tree)

  void mark_tree_uninteresting(struct tree *tree)
  {
-   struct object *obj = >object;
+   struct object *obj;

if (!tree)
return;
+
+   obj = >object;
if (obj->flags & UNINTERESTING)
return;
obj->flags |= UNINTERESTING;


--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] sh-setup: make require_clean_work_tree() work on orphan branches

[Johannes Sixt]

Am 24.11.2015 um 15:45 schrieb SZEDER Gábor:

git-sh-setup's require_clean_work_tree() always exits with error on an
orphan branch, even when the index and worktree are both clean.  The
reason is that require_clean_work_tree() starts off with verifying
HEAD, to make sure that it can safely pass HEAD to 'git diff-index'
later when it comes to checking the cleanness of the index, and errors
out finding the invalid HEAD of the orphan branch.

There are scripts requiring a clean worktree that should work on an
orphan branch as well, and those should be able to use this function
instead of duplicating its functionality and nice error reporting in a
way that handles orphan branches.

Fixing this is easy: the index should be compared to the empty tree
while on an orphan branch, and to HEAD otherwise.

However, just fixing require_clean_work_tree() this way is also
dangerous, because scripts must take care to work properly on orphan
branches.  Currently a script calling require_clean_work_tree() would
exit on a clean orphan branch, but with the simple fix it would
continue executing and who knows what the consequences might be if
the script is not prepared for orphan branches.

Let scripts tell git-sh-setup that they are capable of dealing with
orphan branches by setting the ORPHAN_OK variable, similar to how the
ability to run in a subdirectory must be signalled by setting
SUBDIRECTORY_OK.  Only if ORPHAN_OK is set while on an orphan branch
will require_clean_work_tree() go on and compare the index and
worktree to the empty tree, otherwise it will exit with error even
when the index and worktree are clean, i.e the default exit behavior
doesn't change.

Also provide an error message in the orphan branch/invalid HEAD case
that is consistent in style with the function's other error messages
instead of the error message coming straight from 'git rev-parse
--verify'.

Signed-off-by: SZEDER Gábor 
---
  Documentation/git-sh-setup.txt |  3 ++-
  git-sh-setup.sh| 16 ++--
  t/t2301-require-clean-work-tree.sh | 29 +
  3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-sh-setup.txt b/Documentation/git-sh-setup.txt
index 4f67c4cde6..bccaa2488f 100644
--- a/Documentation/git-sh-setup.txt
+++ b/Documentation/git-sh-setup.txt
@@ -25,7 +25,8 @@ Before sourcing it, your script should set up a few variables;
  `USAGE` (and `LONG_USAGE`, if any) is used to define message
  given by `usage()` shell function.  `SUBDIRECTORY_OK` can be set
  if the script can run from a subdirectory of the working tree
-(some commands do not).
+(some commands do not).  `ORPHAN_OK` can be set if the script can
+work on orphan branches.

  The scriptlet sets `GIT_DIR` and `GIT_OBJECT_DIRECTORY` shell
  variables, but does *not* export them to the environment.
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 4691fbcb64..f45b69386e 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -200,7 +200,19 @@ require_work_tree () {
  }

  require_clean_work_tree () {
-   git rev-parse --verify HEAD >/dev/null || exit 1
+   if git rev-parse --verify HEAD >/dev/null 2>/dev/null
+   then
+   compare_to=HEAD
+   else
+   if [ -z "$ORPHAN_OK" ]
+   then
+   echo >&2 "Cannot $1: Your current branch does not have any 
commits yet."
+   exit 1
+   else
+   # SHA1 of an empty tree
+   compare_to=4b825dc642cb6eb9a060e54bf8d69288fbee4904
+   fi
+   fi


It is worrysome that this now throws away any error message of 
rev-parse. A more conservative approach would be to test for -z 
"$ORPHAN_OK" first and entail new behavior only for the "$ORPHAN_OK" case.



git update-index -q --ignore-submodules --refresh
err=0

@@ -210,7 +222,7 @@ require_clean_work_tree () {
err=1
fi

-   if ! git diff-index --cached --quiet --ignore-submodules HEAD --
+   if ! git diff-index --cached --quiet --ignore-submodules $compare_to --
then
if [ $err = 0 ]
then
diff --git a/t/t2301-require-clean-work-tree.sh 
b/t/t2301-require-clean-work-tree.sh
index 1bb41b59a5..6d0957981e 100755
--- a/t/t2301-require-clean-work-tree.sh
+++ b/t/t2301-require-clean-work-tree.sh
@@ -60,4 +60,33 @@ test_expect_success 'error on dirty index and work tree 
while on orphan branch'
test_must_fail run_require_clean_work_tree
  '

+test_expect_success 'ORPHAN_OK - success on clean index and worktree while on 
orphan branch' '
+   test_when_finished "git checkout master" &&
+   git checkout --orphan orphan &&
+   git reset --hard &&
+   (
+   ORPHAN_OK=Yes &&
+   run_require_clean_work_tree
+   )
+'
+
+test_expect_success 'ORPHAN_OK - error on dirty index while on orphan branch' '
+   test_when_finished 

Re: [PATCH v2] blame: add support for --[no-]progress option

[Johannes Sixt]

Am 22.11.2015 um 17:02 schrieb Edmundo Carmona Antoranz:

Will also affect annotate

Signed-off-by: Edmundo Carmona Antoranz 
---
  Documentation/blame-options.txt |  7 +++
  Documentation/git-blame.txt |  9 -
  builtin/blame.c | 25 +++--
  3 files changed, 38 insertions(+), 3 deletions(-)

diff --git a/Documentation/blame-options.txt b/Documentation/blame-options.txt
index 760eab7..43f4f08 100644
--- a/Documentation/blame-options.txt
+++ b/Documentation/blame-options.txt
@@ -69,6 +69,13 @@ include::line-range-format.txt[]
iso format is used. For supported values, see the discussion
of the --date option at linkgit:git-log[1].

+--[no-]progress::
+   Progress status is reported on the standard error stream
+   by default when it is attached to a terminal. This flag
+   enables progress reporting even if not attached to a
+   terminal.
+
+
  -M||::
Detect moved or copied lines within a file. When a commit
moves or copies a block of lines (e.g. the original file
diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
index e6e947c..2e63397 100644
--- a/Documentation/git-blame.txt
+++ b/Documentation/git-blame.txt
@@ -10,7 +10,8 @@ SYNOPSIS
  [verse]
  'git blame' [-c] [-b] [-l] [--root] [-t] [-f] [-n] [-s] [-e] [-p] [-w] 
[--incremental]
[-L ] [-S ] [-M] [-C] [-C] [-C] [--since=]
-   [--abbrev=] [ | --contents  | --reverse ] [--] 

+   [--[no-]progress] [--abbrev=] [ | --contents  | --reverse 
]
+   [--] 


You add the option to to the synopsis of git-blame.txt, but not to 
git-annotate.txt.




  DESCRIPTION
  ---
@@ -88,6 +89,12 @@ include::blame-options.txt[]
abbreviated object name, use +1 digits. Note that 1 column
is used for a caret to mark the boundary commit.

+--[no-]progress::
+   Progress status is reported on the standard error stream
+   by default when it is attached to a terminal. This flag
+   enables progress reporting even if not attached to a
+   terminal.
+


Any particular reason you add this text twice? As can be seen on the 
hunk header, git-blame.txt includes blame-options.txt.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/2] test: factor out helper function test_must_contain

[Johannes Sixt]

Am 20.11.2015 um 21:50 schrieb René Scharfe:

Extract a helper function for searching for a pattern in a file and
printing the whole file if the pattern is not found.  It is useful
when starting tests with --verbose for debugging purposes.



+# Check if a file contains an expected pattern.
+test_must_contain () {
+   if grep "$1" "$2"
+   then
+   return 0
+   else
+   echo "didn't find /$1/ in '$2', it contains:"
+   cat "$2"
+   return 1
+   fi
+}


There is already test_i18n_grep. Should it be folded into this function? 
Wouldn't we also want to have a function test_must_not_contain?


IMHO, we should not increase the number of functions that give a bonus 
only when there is a test case failure. They do not scale well: There is 
a permanent mental burden on every reviewer to watch out that they are 
used in new tests. But without those functions, the burden is on the one 
person investigating a test case failure, who has to live without the 
debugging support.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv2] run-command: detect finished children by closed pipe rather than waitpid

[Johannes Sixt]

Am 20.11.2015 um 23:05 schrieb Jonathan Nieder:

Stefan Beller wrote:


Detect if a child stopped working by checking if their stderr pipe
was closed instead of checking their state with waitpid.
As waitpid is not fully working in Windows, this is an approach which
allows for better cross platform operation. (It's less code, too)


Can you say more about what is broken about waitpid on Windows?


waitpid(-1, ...) is not implemented on Windows.

Is it necessary to mention waitpid here at all? The most compelling 
reason to write the infrastructure in this new way is that it is much 
more in line with the usual "start_command, read-until-EOF, 
finish_command" sequence.



I ask because it's possible for a child to close stderr without
intending to be finished.  That might be okay here (though the commit
subject doesn't explain so, it is only affecting the workqueue
interface that runs commands in parallel and not the normal
run-command interface) but would need some documentation and could be
counterintuitive.


It could be spelled out more clearly. The children have both their stdin 
and stdout redirected to the same writable end. On the parent side, we 
have to deal only with "stderr" simply because our child_process 
facility has everything to redirect like ">&2" (the stdout_to_stderr 
flag), but nothing for "2>&1".


Yeah, it's still possible that the child closes both stdout and stderr 
long before it dies, but that would really be far, far outside the norm.



So if there's a simple way to get waitpid to work, that seems
preferrable.


It would be possible, but not simple, to make waitpid on Windows 
suitable for the original code, but that does not make the original code 
preferrable.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/7] modernize t9300: use test_must_be_empty

[Johannes Sixt]

Instead of comparing actual output to an empty file, use
test_must_be_empty. In addition to the better error message provided by
the helper, allocation of an empty file during the setup sequence can be
avoided.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t9300-fast-import.sh | 8 +++-
 1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index e9c7602..ceb3db3 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -47,8 +47,6 @@ file5_data='an inline file.
 file6_data='#!/bin/sh
 echo "$@"'
 
->empty
-
 ###
 ### series A
 ###
@@ -2320,12 +2318,12 @@ test_expect_success !MINGW 'R: in-stream cat-blob-fd 
not respected' '
cat-blob $blob
EOF
test_cmp expect actual.3 &&
-   test_cmp empty actual.1 &&
+   test_must_be_empty actual.1 &&
git fast-import 3>actual.3 >actual.1 <<-EOF &&
option cat-blob-fd=3
cat-blob $blob
EOF
-   test_cmp empty actual.3 &&
+   test_must_be_empty actual.3 &&
test_cmp expect actual.1
 '
 
@@ -2549,7 +2547,7 @@ EOF
 
 test_expect_success 'R: quiet option results in no stats being output' '
cat input | git fast-import 2> output &&
-   test_cmp empty output
+   test_must_be_empty output
 '
 
 test_expect_success 'R: feature done means terminating "done" is mandatory' '
-- 
2.6.2.337.ga235d84

--
To unsubscribe from this list: send the line "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/7] Modernize t9300-fast-import

[Johannes Sixt]

Some time ago, I had to dig into t9300-fast-import and found it quite
unhelpful that it does not follow our modern best-practices. This series
brings it up-to-date. I thought I submit it now while it is quiet in
the area.

The larger patches are best viewed using -w -color-words because the
regular patch text is ... overwhelming.

Improving shell coding style is outside the scope of this series. I mean
fixing eyesores such as 'cat >foo < bar', or minor
things such as quoting <<\EOF when the here-doc does not require
substitutions.

In case the large patches don't make it to the list, the series is also
available from

  https://github.com/j6t/git.git modernize-t9300

Johannes Sixt (7):
  modernize t9300: single-quote placement and indentation
  modernize t9300: use test_must_fail
  modernize t9300: use test_must_be_empty
  modernize t9300: wrap lines after &&
  modernize t9300: use test_when_finished for clean-up
  modernize t9300: mark here-doc words to ignore tab indentation
  modernize t9300: move test preparations into test_expect_success

 t/t9300-fast-import.sh | 3629 
 1 file changed, 1822 insertions(+), 1807 deletions(-)

-- 
2.6.2.337.ga235d84

--
To unsubscribe from this list: send the line "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/7] modernize t9300: single-quote placement and indentation

[Johannes Sixt]

Many test cases do not follow our modern style that places the
single-quotes that surround the shell code snippets before and after
the shell code. Make it so.

Many of the lines changed in this way are indented other than by a
single tab. Change them (and some additional lines) to be indented
with a tab.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t9300-fast-import.sh | 872 +
 1 file changed, 437 insertions(+), 435 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 9984c48..566f7bd 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -100,10 +100,10 @@ An annotated tag that annotates a blob.
 EOF
 
 INPUT_END
-test_expect_success \
-'A: create pack from stdin' \
-'git fast-import --export-marks=marks.out  
$GIT_COMMITTER_DATE
 
 initial
 EOF
-test_expect_success \
-   'A: verify commit' \
-   'git cat-file commit master | sed 1d >actual &&
-   test_cmp expect actual'
+test_expect_success 'A: verify commit' '
+   git cat-file commit master | sed 1d >actual &&
+   test_cmp expect actual
+'
 
 cat >expect <actual &&
-test_cmp expect actual'
+test_expect_success 'A: verify tree' '
+   git cat-file -p master^{tree} | sed "s/ [0-9a-f]*   / /" >actual &&
+   test_cmp expect actual
+'
 
 echo "$file2_data" >expect
-test_expect_success \
-   'A: verify file2' \
-   'git cat-file blob master:file2 >actual && test_cmp expect actual'
+test_expect_success 'A: verify file2' '
+   git cat-file blob master:file2 >actual && test_cmp expect actual
+'
 
 echo "$file3_data" >expect
-test_expect_success \
-   'A: verify file3' \
-   'git cat-file blob master:file3 >actual && test_cmp expect actual'
+test_expect_success 'A: verify file3' '
+   git cat-file blob master:file3 >actual && test_cmp expect actual
+'
 
 printf "$file4_data" >expect
-test_expect_success \
-   'A: verify file4' \
-   'git cat-file blob master:file4 >actual && test_cmp expect actual'
+test_expect_success 'A: verify file4' '
+   git cat-file blob master:file4 >actual && test_cmp expect actual
+'
 
 cat >expect <expect <actual &&
git cat-file tag tags/series-A-blob-3 >>actual &&
-   test_cmp expect actual'
+   test_cmp expect actual
+'
 
 test_tick
 cat >input <expect <actual
-test_expect_success \
-   'A: verify diff' \
-   'compare_diff_raw expect actual &&
-test `git rev-parse --verify master:file2` \
-   = `git rev-parse --verify verify--import-marks:copy-of-file2`'
+test_expect_success 'A: verify diff' '
+   compare_diff_raw expect actual &&
+   test `git rev-parse --verify master:file2` \
+   = `git rev-parse --verify verify--import-marks:copy-of-file2`
+'
 
 test_tick
 mt=$(git hash-object --stdin < /dev/null)
@@ -322,7 +322,8 @@ test_expect_success 'A: export marks with large values' '
cat input.blob input.commit | git fast-import 
--export-marks=marks.large &&
git ls-tree refs/heads/verify--dump-marks >tree.out &&
test_cmp tree.exp_s tree.out &&
-   test_cmp marks.exp marks.large'
+   test_cmp marks.exp marks.large
+'
 
 ###
 ### series B
@@ -342,7 +343,7 @@ M 755 0001 zero1
 
 INPUT_END
 test_expect_success 'B: fail on invalid blob sha1' '
-test_must_fail git fast-import /dev/null >/dev/null
@@ -482,19 +483,19 @@ M 755 $newf file2/newf
 D file3
 
 INPUT_END
-test_expect_success \
-'C: incremental import create pack from stdin' \
-'git fast-import expect < 
$GIT_COMMITTER_DATE
 
 second
 EOF
-test_expect_success \
-   'C: verify commit' \
-   'git cat-file commit branch | sed 1d >actual &&
-test_cmp expect actual'
+test_expect_success 'C: verify commit' '
+   git cat-file commit branch | sed 1d >actual &&
+   test_cmp expect actual
+'
 
 cat >expect <expect <actual
-test_expect_success \
-   'C: validate rename result' \
-   'compare_diff_raw expect actual'
+test_expect_success 'C: validate rename result' '
+   compare_diff_raw expect actual
+'
 
 ###
 ### series D
@@ -542,10 +543,10 @@ $file6_data
 EOF
 
 INPUT_END
-test_expect_success \
-'D: inline data in commit' \
-'git fast-import expect <actual
-test_expect_success \
-   'D: validate new files added' \
-   'compare_diff_raw expect actual'
+test_expect_success 'D: validate new files added' '
+   compare_diff_raw expect actual
+'
 
 echo "$file5_data" >expect
-test_expect_success \
-   'D: verify file5' \
-   'git cat-file blob branch:newdir/interesting >actual &&
-test_cmp expect 

[PATCH 2/7] modernize t9300: use test_must_fail

[Johannes Sixt]

One test case open-codes a test for an expected failure. Replace it by
test_must_fail.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t9300-fast-import.sh | 17 +++--
 1 file changed, 3 insertions(+), 14 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 566f7bd..e9c7602 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -630,20 +630,9 @@ from refs/heads/branch
 
 INPUT_END
 test_expect_success 'F: non-fast-forward update skips' '
-   if git fast-import http://vger.kernel.org/majordomo-info.html

[PATCH 6/7] modernize t9300: mark here-doc words to ignore tab indentation

[Johannes Sixt]

In the next commit, we will indent test case preparations. This will
require that here-documents ignore the tab indentation. Prepare for
this change by marking the here-doc words accordingly. This does not
have an effect now, but will remove some noise from the git diff -b
output of the next commit.

The change here is entirely automated with this perl command:

  perl -i -lpe 's/(cat.*<<) *((EOF|(EXPECT|INPUT)_END).*$)/$1-$2 &&/' 
t/t9300-fast-import.sh

i.e., inserts a dash between << and the EOF word (and removes blanks
that our style guide abhors) and appends the && that will become
necessary.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t9300-fast-import.sh | 200 -
 1 file changed, 100 insertions(+), 100 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index adabd68..7586f41 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -57,7 +57,7 @@ test_expect_success 'empty stream succeeds' '
git fast-import input <input <<-INPUT_END &&
 blob
 mark :2
 data <expect <expect <<-EOF &&
 author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 
@@ -118,7 +118,7 @@ test_expect_success 'A: verify commit' '
test_cmp expect actual
 '
 
-cat >expect <expect <<-EOF &&
 100644 blob file2
 100644 blob file3
 100755 blob file4
@@ -146,7 +146,7 @@ test_expect_success 'A: verify file4' '
test_cmp expect actual
 '
 
-cat >expect <expect <<-EOF &&
 object $(git rev-parse refs/heads/master)
 type commit
 tag series-A
@@ -158,7 +158,7 @@ test_expect_success 'A: verify tag/series-A' '
test_cmp expect actual
 '
 
-cat >expect <expect <<-EOF &&
 object $(git rev-parse refs/heads/master:file3)
 type blob
 tag series-A-blob
@@ -170,7 +170,7 @@ test_expect_success 'A: verify tag/series-A-blob' '
test_cmp expect actual
 '
 
-cat >expect <expect <<-EOF &&
 :2 `git rev-parse --verify master:file2`
 :3 `git rev-parse --verify master:file3`
 :4 `git rev-parse --verify master:file4`
@@ -190,7 +190,7 @@ test_expect_success 'A: verify marks import' '
 
 test_tick
 new_blob=$(echo testing | git hash-object --stdin)
-cat >input <input <<-INPUT_END &&
 tag series-A-blob-2
 from $(git rev-parse refs/heads/master:file3)
 data <expect <expect <<-EOF &&
 object $(git rev-parse refs/heads/master:file3)
 type blob
 tag series-A-blob-2
@@ -238,7 +238,7 @@ test_expect_success 'A: tag blob by sha1' '
 '
 
 test_tick
-cat >input <input <<-INPUT_END &&
 commit refs/heads/verify--import-marks
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 data <expect <expect <<-EOF &&
 :00 100755  
7123f7f44e39be127c5eb701e5968176ee9d78b1 A copy-of-file2
 EOF
 git diff-tree -M -r master verify--import-marks >actual
@@ -274,7 +274,7 @@ mt=$(git hash-object --stdin < /dev/null)
 : >marks.exp
 : >tree.exp
 
-cat >input.commit <input.commit <<-EOF &&
 commit refs/heads/verify--dump-marks
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 data <>input.blob <>input.blob <<-EOF &&
 blob
 mark :$l
 data 0
@@ -331,7 +331,7 @@ test_expect_success 'A: export marks with large values' '
 ###
 
 test_tick
-cat >input <input <<-INPUT_END &&
 commit refs/heads/branch
 mark :1
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
@@ -348,7 +348,7 @@ test_expect_success 'B: fail on invalid blob sha1' '
test_must_fail git fast-import input <input <<-INPUT_END &&
 commit TEMP_TAG
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
 data <input <input <<-INPUT_END &&
 commit refs/heads/empty-committer-1
 committer  <> $GIT_COMMITTER_DATE
 data <input <input <<-INPUT_END &&
 commit refs/heads/empty-committer-2
 committer <a...@b.com> $GIT_COMMITTER_DATE
 data <input <input <<-INPUT_END &&
 commit refs/heads/invalid-committer
 committer Name email> $GIT_COMMITTER_DATE
 data <input <input <<-INPUT_END &&
 commit refs/heads/invalid-committer
 committer Name <e $GIT_COMMITTER_DATE
 data <input <input <<-INPUT_END &&
 commit refs/heads/invalid-committer
 committer Name > $GIT_COMMITTER_DATE
 data <input <input <<-INPUT_END &&
 commit refs/heads/invalid-committer
 committer Name input <input <<-INPUT_END &&
 commit refs/heads/invalid-committer
 committer Name $GIT_COMMITTER_DATE
 data <input <

[PATCH 5/7] modernize t9300: use test_when_finished for clean-up

[Johannes Sixt]

A number of clean-ups of test cases are performed outside of
test_expect_success. Replace these cases by using test_when_finished.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t9300-fast-import.sh | 36 
 1 file changed, 16 insertions(+), 20 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index c36afdb..adabd68 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -344,9 +344,9 @@ M 755 0001 zero1
 
 INPUT_END
 test_expect_success 'B: fail on invalid blob sha1' '
+   test_when_finished "rm -f .git/objects/pack_* .git/objects/index_*" &&
test_must_fail git fast-import input </dev/null >/dev/null
-git prune 2>/dev/null >/dev/null
 
 cat >input </dev/null >/dev/null
-git prune 2>/dev/null >/dev/null
 
 cat >input </dev/null >/dev/null
-git prune 2>/dev/null >/dev/null
 
 cat >input <input <input <input <input <tmp &&
cat tmp | cut -f 2 >actual &&
@@ -921,8 +919,6 @@ test_expect_success 'L: nested tree copy does not corrupt 
deltas' '
git fsck `git rev-parse L2`
 '
 
-git update-ref -d refs/heads/L2
-
 ###
 ### series M
 ###
-- 
2.6.2.337.ga235d84

--
To unsubscribe from this list: send the line "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/7] modernize t9300: wrap lines after &

[Johannes Sixt]

It is customary to have each command in test snippets on its own line.
Fix those instances that do not follow this guideline.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t9300-fast-import.sh | 48 
 1 file changed, 32 insertions(+), 16 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index ceb3db3..c36afdb 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -130,17 +130,20 @@ test_expect_success 'A: verify tree' '
 
 echo "$file2_data" >expect
 test_expect_success 'A: verify file2' '
-   git cat-file blob master:file2 >actual && test_cmp expect actual
+   git cat-file blob master:file2 >actual &&
+   test_cmp expect actual
 '
 
 echo "$file3_data" >expect
 test_expect_success 'A: verify file3' '
-   git cat-file blob master:file3 >actual && test_cmp expect actual
+   git cat-file blob master:file3 >actual &&
+   test_cmp expect actual
 '
 
 printf "$file4_data" >expect
 test_expect_success 'A: verify file4' '
-   git cat-file blob master:file4 >actual && test_cmp expect actual
+   git cat-file blob master:file4 >actual &&
+   test_cmp expect actual
 '
 
 cat >expect <expect
 test_expect_success 'Q: verify first note for first commit' '
-   git cat-file blob refs/notes/foobar~2:$commit1 >actual && test_cmp 
expect actual
+   git cat-file blob refs/notes/foobar~2:$commit1 >actual &&
+   test_cmp expect actual
 '
 
 echo "$note2_data" >expect
 test_expect_success 'Q: verify first note for second commit' '
-   git cat-file blob refs/notes/foobar~2:$commit2 >actual && test_cmp 
expect actual
+   git cat-file blob refs/notes/foobar~2:$commit2 >actual &&
+   test_cmp expect actual
 '
 
 echo "$note3_data" >expect
 test_expect_success 'Q: verify first note for third commit' '
-   git cat-file blob refs/notes/foobar~2:$commit3 >actual && test_cmp 
expect actual
+   git cat-file blob refs/notes/foobar~2:$commit3 >actual &&
+   test_cmp expect actual
 '
 
 cat >expect <expect
 test_expect_success 'Q: verify second note for first commit' '
-   git cat-file blob refs/notes/foobar^:$commit1 >actual && test_cmp 
expect actual
+   git cat-file blob refs/notes/foobar^:$commit1 >actual &&
+   test_cmp expect actual
 '
 
 echo "$note2_data" >expect
 test_expect_success 'Q: verify first note for second commit' '
-   git cat-file blob refs/notes/foobar^:$commit2 >actual && test_cmp 
expect actual
+   git cat-file blob refs/notes/foobar^:$commit2 >actual &&
+   test_cmp expect actual
 '
 
 echo "$note3_data" >expect
 test_expect_success 'Q: verify first note for third commit' '
-   git cat-file blob refs/notes/foobar^:$commit3 >actual && test_cmp 
expect actual
+   git cat-file blob refs/notes/foobar^:$commit3 >actual &&
+   test_cmp expect actual
 '
 
 cat >expect <expect
 test_expect_success 'Q: verify third note for first commit' '
-   git cat-file blob refs/notes/foobar2:$commit1 >actual && test_cmp 
expect actual
+   git cat-file blob refs/notes/foobar2:$commit1 >actual &&
+   test_cmp expect actual
 '
 
 cat >expect <expect
 test_expect_success 'Q: verify second note for second commit' '
-   git cat-file blob refs/notes/foobar:$commit2 >actual && test_cmp expect 
actual
+   git cat-file blob refs/notes/foobar:$commit2 >actual &&
+   test_cmp expect actual
 '
 
 cat >input <http://vger.kernel.org/majordomo-info.html

[PATCH 7/7] modernize t9300: move test preparations into test_expect_success

[Johannes Sixt]

Our usual style these days is to execute everything inside
test_expect_success. Make it so.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t9300-fast-import.sh | 2580 
 1 file changed, 1297 insertions(+), 1283 deletions(-)

diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 7586f41..14a9384 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -51,54 +51,53 @@ echo "$@"'
 ### series A
 ###
 
-test_tick
-
 test_expect_success 'empty stream succeeds' '
git fast-import input <<-INPUT_END &&
-blob
-mark :2
-data < $GIT_COMMITTER_DATE
-data <input <<-INPUT_END &&
+   blob
+   mark :2
+   data < $GIT_COMMITTER_DATE
+   data <expect <<-EOF &&
-author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
-committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
-
-initial
-EOF
 test_expect_success 'A: verify commit' '
+   cat >expect <<-EOF &&
+   author $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+   committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+
+   initial
+   EOF
git cat-file commit master | sed 1d >actual &&
test_cmp expect actual
 '
 
-cat >expect <<-EOF &&
-100644 blob file2
-100644 blob file3
-100755 blob file4
-EOF
 test_expect_success 'A: verify tree' '
+   cat >expect <<-EOF &&
+   100644 blob file2
+   100644 blob file3
+   100755 blob file4
+   EOF
git cat-file -p master^{tree} | sed "s/ [0-9a-f]*   / /" >actual &&
test_cmp expect actual
 '
 
-echo "$file2_data" >expect
 test_expect_success 'A: verify file2' '
+   echo "$file2_data" >expect &&
git cat-file blob master:file2 >actual &&
test_cmp expect actual
 '
 
-echo "$file3_data" >expect
 test_expect_success 'A: verify file3' '
+   echo "$file3_data" >expect &&
git cat-file blob master:file3 >actual &&
test_cmp expect actual
 '
 
-printf "$file4_data" >expect
 test_expect_success 'A: verify file4' '
+   printf "$file4_data" >expect &&
git cat-file blob master:file4 >actual &&
test_cmp expect actual
 '
 
-cat >expect <<-EOF &&
-object $(git rev-parse refs/heads/master)
-type commit
-tag series-A
-
-An annotated tag without a tagger
-EOF
 test_expect_success 'A: verify tag/series-A' '
+   cat >expect <<-EOF &&
+   object $(git rev-parse refs/heads/master)
+   type commit
+   tag series-A
+
+   An annotated tag without a tagger
+   EOF
git cat-file tag tags/series-A >actual &&
test_cmp expect actual
 '
 
-cat >expect <<-EOF &&
-object $(git rev-parse refs/heads/master:file3)
-type blob
-tag series-A-blob
-
-An annotated tag that annotates a blob.
-EOF
 test_expect_success 'A: verify tag/series-A-blob' '
+   cat >expect <<-EOF &&
+   object $(git rev-parse refs/heads/master:file3)
+   type blob
+   tag series-A-blob
+
+   An annotated tag that annotates a blob.
+   EOF
git cat-file tag tags/series-A-blob >actual &&
test_cmp expect actual
 '
 
-cat >expect <<-EOF &&
-:2 `git rev-parse --verify master:file2`
-:3 `git rev-parse --verify master:file3`
-:4 `git rev-parse --verify master:file4`
-:5 `git rev-parse --verify master^0`
-EOF
 test_expect_success 'A: verify marks output' '
+   cat >expect <<-EOF &&
+   :2 `git rev-parse --verify master:file2`
+   :3 `git rev-parse --verify master:file3`
+   :4 `git rev-parse --verify master:file4`
+   :5 `git rev-parse --verify master^0`
+   EOF
test_cmp expect marks.out
 '
 
@@ -188,68 +187,69 @@ test_expect_success 'A: verify marks import' '
test_cmp expect marks.new
 '
 
-test_tick
-new_blob=$(echo testing | git hash-object --stdin)
-cat >input <<-INPUT_END &&
-tag series-A-blob-2
-from $(git rev-parse refs/heads/master:file3)
-data < 0 +
-data 0
-M 644 :6 new_blob
-#pretend we got sha1 from fast-import
-ls "new_blob"
-
-tag series-A-blob-3
-from $new_blob
-data <expect <<-EOF &&
-object $(git rev-parse refs/heads/master:file3)
-type blob
-tag series-A-blob-2
-
-Tag blob by sha1.
-object $new_blob
-type blob
-tag series-A-blob-3
-
-Tag new_blob.
-EOF
-
 test_expect_success 'A: tag blob by sha1' '
+   test_tick &&
+   new_blob=$(echo testing | git hash-object --stdin) &&
+   cat >input <<-INPUT_END &&
+   tag series-A-blob-2
+   from $(git rev-

Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid

[Johannes Sixt]

Am 11.11.2015 um 21:37 schrieb Stefan Beller:

including the list and all others this time.


 if (code < 0) {
 pp->shutdown = 1;
-   kill_children(pp, SIGTERM);
+   kill_children(pp, -code);



I'll see what this means for our kill emulation on Windows. Currently, we
handle only SIGTERM.


So currently we only pass in SIGTERM from the callers, and I certainly
only intend
to use that signal. I just thought special casing the SIGTERM signal
would do no good
in terms of design here.

So maybe that was not the right thought and we do have to special case
SIGTERM here?


I wonder why task_finish() callback gets to choose a signal. The point 
here is, IIUC, when one child dies, the others must be halted, too. 
SIGTERM seems to be the only sensible choice.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid

[Johannes Sixt]

Am 11.11.2015 um 21:53 schrieb Stefan Beller:

On Wed, Nov 11, 2015 at 12:48 PM, Johannes Sixt <j...@kdbg.org> wrote:

I wonder why task_finish() callback gets to choose a signal. The point here
is, IIUC, when one child dies, the others must be halted, too. SIGTERM seems
to be the only sensible choice.


SIGKILL would also do?

In case you know your children, you can also send a SIGUSR1 or SIGUSR2.
...
So I am not convinced SIGTERM is the only true choice here. And because I
have no idea which of the signals may be useful in the future, I decided to
go with all of them.


Fair enough.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] run-command: detect finished children by closed pipe rather than waitpid

[Johannes Sixt]

Am 07.11.2015 um 00:48 schrieb Stefan Beller:

Detect if a child stopped working by checking if their stderr pipe
was closed instead of checking their state with waitpid.
As waitpid is not fully working in Windows, this is an approach which
allows for better cross platform operation. (It's less code, too)

Previously we did not close the read pipe of finished children, which we
do now.

The old way missed some messages on an early abort. We just killed the
children and did not bother to look what was left over. With this approach
we'd send a signal to the children and wait for them to close the pipe to
have all the messages (including possible "killed by signal 15" messages).

To have the test suite passing as before, we allow for real graceful
abortion now. In case the user wishes to abort parallel execution
the user needs to provide either the signal used to kill all children
or the children are let run until they finish normally.

Signed-off-by: Stefan Beller 
---
  Hi,

  this applis on top of origin/sb/submodule-parallel-fetch,
  making Windows folks possibly even more happy. It makes the code easier
  to read and has less races on cleaning up a terminated child.

  It follows the idea of Johannes patch, instead of encoding information in .err
  I removed the in_use flag and added a state, currently having 3 states.

  Thanks,
  Stefan

  Johannes schrieb:
  > First let me say that I find it very questionable that the callbacks
  > receive a struct child_process.

  I tried to get rid of the child_process struct in the callbacks, but that's
  not as easy as one may think.


Fair enough. I see you removed .err, .no_stdin and .stdout_to_stderr 
from the callback. Good.



pp->nr_processes--;
-   pp->children[i].in_use = 0;
+   pp->children[i].state = FREE;
pp->pfd[i].fd = -1;
child_process_deinit(>children[i].process);


This cleanup is implied by finish_command and can be removed.


child_process_init(>children[i].process);



@@ -1195,12 +1175,12 @@ int run_processes_parallel(int n,
i < spawn_cap && !pp->shutdown &&
pp->nr_processes < pp->max_processes;
i++) {
-   int code = pp_start_one(pp);
+   code = pp_start_one(pp);
if (!code)
continue;
if (code < 0) {
pp->shutdown = 1;
-   kill_children(pp, SIGTERM);
+   kill_children(pp, -code);


I'll see what this means for our kill emulation on Windows. Currently, 
we handle only SIGTERM.



}
break;
}


Thanks you very much!

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Johannes Sixt]

Am 06.11.2015 um 20:00 schrieb Stefan Beller:

On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt <j...@kdbg.org> wrote:

Here is a prototype patch. Feel free to pick it up. It marks a process
whose EOF we have found by setting .err to -1. It's probably better to
extend the meaning of the in_use indicator for this purpose.


Thanks for the proposal, I'll take that and make in_use a tristate for now
(an enum consisting of FREE, WORKING, WAIT_CLEANUP)


I'd like to report that the prototype patch works on Windows. I tested 
it lightly using test-run-command with commands producing output from 
around 100 bytes to 10MB. So, I'm confident that this is the right approach.


Thank you for keeping the ball rolling!

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Johannes Sixt]

Am 05.11.2015 um 19:17 schrieb Stefan Beller:
> strbuf_read_once can also operate on blocking file descriptors if we are
> sure they are ready. The poll (2) command however makes sure this is the
> case.
> 
> Reading the manual for poll (2), there may be spurious returns indicating
> readiness but that is for network sockets only. Pipes should be unaffected.
> By having this patch, we rely on the correctness of poll to return
> only pipes ready to read.
> 
> This fixes compilation in Windows.

It certainly does (but I haven't tested, yet). But parallel processes
will not work because we do not have a sufficiently complete waitpid
emulation, yet. (waitpid(-1, ...) is not implemented.)

However, I think that the infrastructure can be simplified even further
to a level that we do not need additional emulation on Windows.

First let me say that I find it very questionable that the callbacks
receive a struct child_process. This is an implementation detail. It is
also an implementation detail that stderr of the children is read and
buffered, and that the child's stdout is redirected to stderr. It
should not be the task of the get_next_task callback to set the members
no_stdin, stdout_to_stderr, and err of struct child_process.

If you move that initialization to pp_start_one, you will notice rather
sooner than later that the readable end of the file descriptor is never
closed!

Which makes me think: Other users of start_command/finish_command work
such that they

1. request a pipe by setting .out = -1
2. start_command
3. read from .out until EOF
4. close .out
5. wait for the process with finish_command

But the parallel_process infrastructure does not follow this pattern.
It

1. requests a pipe by setting .err = -1
2. start_command
3. read from .err
4. wait for the process with waitpid

(and forgets to close .err). EOF is not in the picture (but that is
not essential).

I suggest to change this such that we read from the children until EOF,
mark them to be at their end of life, and then wait for them using
finish_command (assuming that a process that closes stdout and stderr
will die very soon if it is not already dead).

Here is a prototype patch. Feel free to pick it up. It marks a process
whose EOF we have found by setting .err to -1. It's probably better to
extend the meaning of the in_use indicator for this purpose. This seems
to work on Linux with test-run-command with sub-processes that produce
100k output each:

./test-run-command run-command-parallel 5 sh -c "printf \"%010d\n\" 999"

although error handling would require some polishing according to
t0061-run-command.

diff --git a/run-command.c b/run-command.c
index 51d078c..3e42299 100644
--- a/run-command.c
+++ b/run-command.c
@@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,
for (i = 0; i < n; i++) {
strbuf_init(>children[i].err, 0);
child_process_init(>children[i].process);
-   pp->pfd[i].events = POLLIN;
+   pp->pfd[i].events = POLLIN|POLLHUP;
pp->pfd[i].fd = -1;
}
sigchain_push_common(handle_children_on_signal);
@@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes 
*pp, int output_timeout)
/* Buffer output from all pipes. */
for (i = 0; i < pp->max_processes; i++) {
if (pp->children[i].in_use &&
-   pp->pfd[i].revents & POLLIN)
-   if (strbuf_read_once(>children[i].err,
-pp->children[i].process.err, 0) < 
0)
+   pp->pfd[i].revents & (POLLIN|POLLHUP)) {
+   int n = strbuf_read_once(>children[i].err,
+pp->children[i].process.err, 0);
+   if (n == 0) {
+   close(pp->children[i].process.err);
+   pp->children[i].process.err = -1;
+   } else if (n < 0) {
if (errno != EAGAIN)
die_errno("read");
+   }
+   }
}
 }
 
@@ -1082,59 +1088,20 @@ static void pp_output(struct parallel_processes *pp)
 static int pp_collect_finished(struct parallel_processes *pp)
 {
int i = 0;
-   pid_t pid;
-   int wait_status, code;
+   int code;
int n = pp->max_processes;
int result = 0;
 
while (pp->nr_processes > 0) {
-   pid = waitpid(-1, _status, WNOHANG);
-   if (pid == 0)
-   break;
-
-   if (pid < 0)
-   die_errno("wait");
-
for (i = 0; i < pp->max_processes; i++)
if (pp->children[i].in_use &&
-   pid == pp->children[i].process.pid)
+   pp->children[i].process.err == -1)
break;
+
if (i 

Re: [PATCH 1/2] run-command: Remove set_nonblocking

[Johannes Sixt]

Am 05.11.2015 um 23:20 schrieb Stefan Beller:

On Thu, Nov 5, 2015 at 12:27 PM, Johannes Sixt <j...@kdbg.org> wrote:


diff --git a/run-command.c b/run-command.c
index 51d078c..3e42299 100644
--- a/run-command.c
+++ b/run-command.c
@@ -977,7 +977,7 @@ static struct parallel_processes *pp_init(int n,
 for (i = 0; i < n; i++) {
 strbuf_init(>children[i].err, 0);
 child_process_init(>children[i].process);
-   pp->pfd[i].events = POLLIN;
+   pp->pfd[i].events = POLLIN|POLLHUP;
 pp->pfd[i].fd = -1;
 }
 sigchain_push_common(handle_children_on_signal);
@@ -1061,11 +1061,17 @@ static void pp_buffer_stderr(struct parallel_processes 
*pp, int output_timeout)
 /* Buffer output from all pipes. */
 for (i = 0; i < pp->max_processes; i++) {
 if (pp->children[i].in_use &&
-   pp->pfd[i].revents & POLLIN)
-   if (strbuf_read_once(>children[i].err,
-pp->children[i].process.err, 0) < 
0)
+   pp->pfd[i].revents & (POLLIN|POLLHUP)) {
+   int n = strbuf_read_once(>children[i].err,
+pp->children[i].process.err, 0);
+   if (n == 0) {
+   close(pp->children[i].process.err);
+   pp->children[i].process.err = -1;


So you set .err to -1 to signal the process has ended here...


-
 for (i = 0; i < pp->max_processes; i++)
 if (pp->children[i].in_use &&
-   pid == pp->children[i].process.pid)
+   pp->children[i].process.err == -1)
 break;


to make a decision here if we want to finish_command on it.


Correct.


+   code = finish_command(>children[i].process);



-   child_process_clear(>children[i].process);


but .err stays stays -1 here for the next iteration?
We would need to reset it to 0 again.


No. In the next round, we need -1 to request a pipe. get_next_task 
callback sets it to -1 as well (and I think it is wrong that the 
callback does it; pp_start_one should do that).



So .err is
   0 when the slot is not in use
  -1 when the child has finished awaiting termination
  >0 when the child is living a happy life.


But, as I said, .err is not the right place to mark dying processes (it 
was just the simplest way to demonstrate the concept in this patch). 
Better extend .in_use to a tri-state indicator.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCHv3 02/11] run-command: report failure for degraded output just once

[Johannes Sixt]

Am 04.11.2015 um 21:14 schrieb Stefan Beller:

On Wed, Nov 4, 2015 at 10:14 AM, Junio C Hamano  wrote:

Imagine that we are running two things A and B at the same time.  We
ask poll(2) and it says both A and B have some data ready to be
read, and we try to read from A.  strbuf_read_once() would try to
read up to 8K, relying on the fact that you earlier set the IO to be
nonblock.  It will get stuck reading from A without allowing output
from B to drain.  B's write may get stuck because we are not reading
from it, and would cause B to stop making progress.

What if the other sides of the connection from A and B are talking
with each other,


I am not sure if we want to allow this ever. How would that work with
jobs==1? How do we guarantee to have A and B running at the same time?


I think that a scenario where A and B are communicating is rather 
far-fetched. We are talking about parallelizing independent tasks. I 
would not worry.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Johannes Sixt]

Am 03.11.2015 um 19:18 schrieb Stefan Beller:

... ReadFileEx ... "overlapped" operation.


Let's not go there just yet.


  1. Make this an optional feature so that platforms can compile it
 out, if it is not already done.  My preference, even if we go
 that route, would be to see if we can find a way to preserve the
 overall code structure (e.g. instead of spawning multiple
 workers, which is why the code needs NONBLOCK to avoid getting
 stuck on reading from one while others are working, perhaps we
 can spawn only one and not do a nonblock read?).


Yeah that would be my understanding as well. If we don't come up with
a good solution for parallelism in Windows now, we'd need to make it at
least working in the jobs=1 case as well as it worked before.


That should be possible. I discovered today that we have this function:

static void set_nonblocking(int fd)
{
int flags = fcntl(fd, F_GETFL);
if (flags < 0)
warning("Could not get file status flags, "
"output will be degraded");
else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
warning("Could not set file status flags, "
"output will be degraded");
}

Notice that it is not a fatal condition if O_NONBLOCK cannot be 
established. (BTW, did you ever test this condition?) If we add two 
lines (which remove the stuff that does not work on Windows) like this:


static void set_nonblocking(int fd)
{
#ifndef GIT_WINDOWS_NATIVE
int flags = fcntl(fd, F_GETFL);
if (flags < 0)
warning("Could not get file status flags, "
"output will be degraded");
else if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
#endif
warning("Could not set file status flags, "
"output will be degraded");
}

we should get something that works, theoretically. We still need a more 
complete waitpid emulation, but that does not look like rocket science. 
I'll investigate further in this direction.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Johannes Sixt]

Am 03.11.2015 um 00:06 schrieb Stefan Beller:

On Mon, Nov 2, 2015 at 1:15 PM, Johannes Sixt <j...@kdbg.org> wrote:

run-command.c: In function 'set_nonblocking':
run-command.c:1011: error: 'F_GETFL' undeclared (first use in this function)
run-command.c:1011: error: (Each undeclared identifier is reported only once
run-command.c:1011: error: for each function it appears in.)
run-command.c:1015: error: 'F_SETFL' undeclared (first use in this function)
run-command.c:1015: error: 'O_NONBLOCK' undeclared (first use in this function)
make: *** [run-command.o] Error 1


Going by a quick search http://stackoverflow.com/a/22756664
I'd hope we only need to modify the set_nonblocking function using #ifdefs ?


Unfortunately, the solutions outlined in that post do not work for us. 
On Windows, the FIONBIO option is only available for sockets. 
"Overlapped IO" can be used only when the file descriptor is set in this 
mode right from the beginning (by open/CreateFile), and if it were so, 
it would have to be used in a totally different way than in Posix code.


My findings so far are negative. The only short-term and mid-term 
solution I see so far is to opt-out from the framework during build-time.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git.git as of tonight

[Johannes Sixt]

Am 02.11.2015 um 03:58 schrieb Junio C Hamano:
> * sb/submodule-parallel-fetch (2015-10-21) 14 commits
>(merged to 'next' on 2015-10-23 at 8f04bbd)
>   + run-command: fix missing output from late callbacks
>   + test-run-command: increase test coverage
>   + test-run-command: test for gracefully aborting
>   + run-command: initialize the shutdown flag
>   + run-command: clear leftover state from child_process structure
>   + run-command: fix early shutdown
>(merged to 'next' on 2015-10-15 at df63590)
>   + submodules: allow parallel fetching, add tests and documentation
>   + fetch_populated_submodules: use new parallel job processing
>   + run-command: add an asynchronous parallel child processor
>   + sigchain: add command to pop all common signals
>   + strbuf: add strbuf_read_once to read without blocking
>   + xread_nonblock: add functionality to read from fds without blocking
>   + xread: poll on non blocking fds
>   + submodule.c: write "Fetching submodule " to stderr
>   (this branch is used by rs/daemon-leak-fix and 
> sb/submodule-parallel-update.)
> 
>   Add a framework to spawn a group of processes in parallel, and use
>   it to run "git fetch --recurse-submodules" in parallel.
> 
>   Will merge to 'master'.

Please don't, yet. This series does not build on Windows:

run-command.c: In function 'set_nonblocking':
run-command.c:1011: error: 'F_GETFL' undeclared (first use in this function)
run-command.c:1011: error: (Each undeclared identifier is reported only once
run-command.c:1011: error: for each function it appears in.)
run-command.c:1015: error: 'F_SETFL' undeclared (first use in this function)
run-command.c:1015: error: 'O_NONBLOCK' undeclared (first use in this function)
make: *** [run-command.o] Error 1

I have to investigate whether we can have some sort of Posixy
non-blocking IO on Windows or whether we have to opt-out from this
parallel-process facility. Any help from Windows experts would be
appreciated.

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] ref-filter: fallback on alphabetical comparison

[Johannes Sixt]

Am 30.10.2015 um 09:45 schrieb Karthik Nayak:

In ref-filter.c the comparison of refs while sorting is handled by
cmp_ref_sorting() function. When sorting as per numerical values
(e.g. --sort=objectsize) there is no fallback comparison when both
refs hold the same value. This can cause unexpected results (i.e. the
order of listing refs with equal values cannot be pre-determined) as
pointed out by Johannes Sixt ($gmane/280117).

Hence, fallback to alphabetical comparison based on the refname
whenever the other criterion is equal.

A test in t3203 was expecting that branch-two sorts before HEAD, which
happened to be how qsort(3) on Linux sorted the array, but (1) that
outcome was not even guaranteed, and (2) once we start breaking ties
with the refname, "HEAD" should sort before "branch-two" so the
original expectation was inconsistent with the criterion we now use.

Update it to match the new world order, which we can now depend on
being stable.


Needless to say that the patch fixes the test failure on Windows. (I 
tested v2 of the patch.)


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git-gui is still using old-style git-merge invocation

[Johannes Sixt]

Thanks!

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

git-gui is still using old-style git-merge invocation

[Johannes Sixt]

Performing a merge with git gui presents the following message in the 
merge result window:


warning: old-style 'git merge  HEAD ' is deprecated.
Merge made by the 'recursive' strategy.
 a | 1 +
 1 file changed, 1 insertion(+)
 create mode 100644 a

But I am unable to find where the invocation happens. Can somebody help?

-- Hannes

PS: Reproducer:

git init
echo a > a
git add a
git commit -ma a
git checkout -b side @~
echo b > b
git add b
git commit -mb
git gui  # to merge: Ctrl-M, Enter, Enter
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

t3203: --sort=objectsize failure on Windows

[Johannes Sixt]

On Windows, I observe a failure of the test case 'git branch `--sort` 
option' introduced by aedcb7dc (branch.c: use 'ref-filter' APIs). The 
reason is that the resulting order generated by qsort is unspecified for 
entries that compare equal. In fact, the test case sorts 4 entries where 
there are only 2 different values.


To achieve a deterministic order, perhaps cmp_ref_sorting() should fall 
back to alphabetic comparison if the other criterion (when used) 
compares equal?


-- Hannes
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 2/2 jk/war-on-sprintf] compat/mingw.c: remove printf format warning

[Johannes Sixt]

5096d490 (convert trivial sprintf / strcpy calls to xsnprintf) converted
two sprintf calls. Now GCC warns that "format '%u' expects argument of
type 'unsigned int', but argument 4 has type 'long unsigned int'".
Instead of changing the format string, use a variable of type unsigned
in place of the typedef-ed type DWORD, which hides that it is actually an
unsigned long.

There is no correctness issue with the old code because unsigned long and
unsigned are always of the same size on Windows, even in 64-bit builds.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
I do not know why there are no warnings with the old code. Apparently, the
system provided sprintf declaration does not have format-printf
annotation.

 compat/mingw.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index a168800..90bdb1e 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -2131,7 +2131,7 @@ void mingw_startup()
 
 int uname(struct utsname *buf)
 {
-   DWORD v = GetVersion();
+   unsigned v = (unsigned)GetVersion();
memset(buf, 0, sizeof(*buf));
xsnprintf(buf->sysname, sizeof(buf->sysname), "Windows");
xsnprintf(buf->release, sizeof(buf->release),
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2 jk/war-on-sprintf] read_branches_file: plug a FILE* leak

[Johannes Sixt]

The earlier rewrite f28e3ab2 (read_branches_file: simplify string handling)
of read_branches_file() lost an fclose() call. Put it back.

As on Windows files that are open cannot be removed, the leak manifests in
a failure of 'git remote rename origin origin' when the remote's URL is
specified in .git/branches/origin, because by the time that the command
attempts to remove this file, it is still open.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 remote.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/remote.c b/remote.c
index 1101f82..fb16153 100644
--- a/remote.c
+++ b/remote.c
@@ -282,6 +282,7 @@ static void read_branches_file(struct remote *remote)
return;
 
strbuf_getline(, f, '\n');
+   fclose(f);
strbuf_trim();
if (!buf.len) {
strbuf_release();
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/34] libify mailinfo and call it directly from am

[Johannes Sixt]

Am 21.10.2015 um 17:51 schrieb Ramsay Jones:

On 20/10/15 22:24, Junio C Hamano wrote:

Junio C Hamano  writes:
some numbers on my desktop (Dell T3500 2.66GHz Xeon X5650 with 12GB,
running Ubuntu),


I suspect that I haven't tested exactly the same version as you, but I had
a quick look at testing this on Cygwin today. I have included a complete
transcript (below), so you can see what I did wrong! :-P



Between 'master' and the version with this series (on 'jch'),
applying this 34-patch series itself on top of 'master' using "git
am", best of 5 numbers for running:

 time git am mbox >/dev/null

are

   (master) (with the series)
 real0m0.648sreal0m0.537s
 user0m0.358suser0m0.338s
 sys 0m0.172ssys 0m0.154s



The corresponding times for me were:

 (master)   (with the series)
   real 0m9.760s  real  0m5.744s
   user 0m0.531s  user  0m0.656s
   sys  0m5.726s  sys   0m3.520s

So, yes, a noticeable improvement! :)


Same here, on Windows built with the old msysgit environment:

(master) (with the series)
real0m3.147s  real0m1.947s
user0m0.016s  user0m0.000s
sys 0m0.015s  sys 0m0.031s

Although I tested 'git am patches/*' where the patches/* are the result 
of 'git-format-patch v2.6.1..github/jc/mailinfo-lib'.


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [BUG?] applypatch-msg hook no-longer thinks stdin is a tty

[Johannes Sixt]

Am 03.10.2015 um 09:37 schrieb Chris Packham:

On Sat, Oct 3, 2015 at 6:43 AM, Junio C Hamano  wrote:

If you want to go interactive from the hook, you'd have to open and
interact with /dev/tty yourself in your hook anyway.


That may be what I have to do, although I have absolutely no idea how.


Something like this:

exec <>/dev/tty 1>&0 2>&0
printf "what now: "
read answer
echo the answer was: "$answer"

-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: More builtin git-am issues..

[Johannes Sixt]

Am 05.09.2015 um 02:54 schrieb Junio C Hamano:

Linus Torvalds  writes:


So I think that logic should basically be extended to saying

  - if any line in the last chunk has a "Signed-off-by:", set a flag.

  - at the end of the loop, if that flag wasn't set, return 0.


I am reluctant to special case S-o-b: too much, even though this is
about "am -s" and by definition S-o-b: is special, as that is what
we are adding after all.

How about a bit looser rule like this?

 A block of text at the end of the message, each and every
 line in which must match "^[^:]+:[  ]" (that is,
 a "keyword" that does not contain a whitespace nor a colon,
 followed by a colon and whitespace, and arbitrary value thru
 the end of line) is a signature block.


Why do we need a new rule? The old git-am had a logic that pleased 
everyone, and it must have been implemented somewhere. Shouldn't it be 
sufficient to just re-implement or re-use that logic?


-- Hannes

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] Mingw: verify both ends of the pipe () call

[Johannes Sixt]

Am 27.08.2015 um 23:50 schrieb Jonathan Nieder:

Johannes Schindelin wrote:


From: jfmc jfm...@gmail.com


This means the name shown by git shortlog would be jfmc instead of
Jose F. Morales.  Intended?


The code to open and test the second end of the pipe clearly imitates
the code for the first end. A little too closely, though... Let's fix
the obvious copy-edit bug.

Signed-off-by: Jose F. Morales jfm...@gmail.com
Signed-off-by: Johannes Schindelin johannes.schinde...@gmx.de
---
  compat/mingw.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)


Reviewed-by: Jonathan Nieder jrnie...@gmail.com

This is an old one --- more than 5 years old (since v1.7.0-rc0~86^2~4
Windows: simplify the pipe(2) implementation, 2010-01-15).  Thanks
for catching it.


Ouch! Thanks for cleaning up the mess I left behind.

-- Hannes



Regards,
Jonathan

(patch kept unsnipped for reference)


diff --git a/compat/mingw.c b/compat/mingw.c
index 496e6f8..f74da23 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -681,7 +681,7 @@ int pipe(int filedes[2])
return -1;
}
filedes[1] = _open_osfhandle((int)h[1], O_NOINHERIT);
-   if (filedes[0]  0) {
+   if (filedes[1]  0) {
close(filedes[0]);
CloseHandle(h[1]);
return -1;



--
To unsubscribe from this list: send the line 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 merge changes file mode from 644 to 755

[Johannes Sixt]

Am 14.08.2015 um 14:02 schrieb Dmitry Oksenchuk:

Hello,

I've noticed strange behavior of git merge on Windows with
core.filemode=false (set by default). Git changed mode of some files
from 644 to 755 for unknown reason. One of the files is stdafx.cpp,
it's absent in the common ancestor, it was added in the first branch
(master) with mode 644 and it's still absent in the second branch
(feature). So, git merges the file without conflicts but changes mode
from 644 to 755.


I do know the git-merge-recursive does not honor core.filemode. However, 
I am surprised to see a mode change from 644 to 755. I usually observe 
only mode changes from 755 to 644, but then the case where the file is 
not present on one branch is uncommon for me.



Why git merge changes mode from 644 to 755? Is it a known issue?


So, well, yes, it is known that with git merge mode changes do occur, 
but no, 644 to 755 is news to me.


The work-around is to unstage the file and stage it again, e.g. with 
git-gui and then to amend the merge commit.


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [msysGit] Re: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe

[Johannes Sixt]

Am 13.08.2015 um 09:30 schrieb Johannes Schindelin:

Hi Johannes,

On 2015-08-12 20:31, Johannes Sixt wrote:

Am 12.08.2015 um 13:58 schrieb Erik Faye-Lund:

On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:

FWIW Git for Windows has this patch (that I wanted to contribute
in  due time, what with being busy with all those tickets) to solve the
problem mentioned in your patch in a different way:

https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45


Yuck. On Windows, it's the extension of a file that dictates what kind
of file it is (and if it's executable or not), not the contents. If we
get a shell script written with the .exe-prefix, it's considered as
an invalid executable by the system. We should consider it the same
way, otherwise we're on the path to user-experience schizophrenia.

I'm not sure I consider this commit a step in the right direction.


I, too, think that it is a wrong decision to pessimize git for the
sake of a single test case.


Oh, you make it sound as if you believe that I had indeed weakened
Git  *just* for a single test case.


Whatever. Since I do not have the time to provide hard numbers that 
prove my claim that your patch removes an optimization (and, 
furthermore, I do not want to reply to your arguments that I consider 
mostly philosophical rather than pragmatic), I bow out. Until this 
solution or that one is in upstream, I can help myself.


Junio, please drop my patch. I do not have the nerves to support it.

-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] revisions --stdin: accept CRLF line terminators

[Johannes Sixt]

Am 12.08.2015 um 00:14 schrieb Junio C Hamano:

Now, I am wondering if it makes sense to do these two things:

  * Teach revision.c::read_revisions_from_stdin() to use
strbuf_getline() instead of strbuf_getwholeline().

  * Teach strbuf_getline() to remove CR at the end when stripping the
LF at the end, only if term parameter is set to LF.

Doing so would solve 1. and 2., but we obviously need to audit all
the other uses of strbuf_getline() to see if they can benefit (or if
some of them may be broken because they _always_ need LF terminated
lines, i.e. CRLF terminated input is illegal to them).


I can see what I can do with these. Don't hold your breath, though.


As to 3., I think it is OK.  The code structure of 4. is too ugly
and needs to be revamped to go one line at a time first before even
thinking about how to proceed, I would think.


Regarding update-ref --stdin (your 4.), I notice that the input format 
is very strict, so the solution is to allow an optional CR before the 
LF. I alread have a patch, but it skips all trailing space, which is 
probably too lenient. (I only needed the patch once for a debug 
sesssion, but there is no obvious breakage without the patch.)


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH jk/prune-mtime] prune: close directory earlier during loose-object directory traversal

[Johannes Sixt]

27e1e22d (prune: factor out loose-object directory traversal, 2014-10-16)
introduced a new function for_each_loose_file_in_objdir() with a helper
for_each_file_in_obj_subdir(). The latter calls callbacks for each file
found during a directory traversal and finally also a callback for the
directory itself.

git-prune uses the function to clean up the object directory. In
particular, in the directory callback it calls rmdir(). On Windows XP,
this rmdir call fails, because the directory is still open while the
callback is called. Close the directory before calling the callback.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 My Windows 8.1 machine does not require this fix for some unkonwn
 reason. But we still cater for Windows XP users, where this change
 is a real improvement.

 sha1_file.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 4d1b26f..5cecc68 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -3473,12 +3473,12 @@ static int for_each_file_in_obj_subdir(int subdir_nr,
break;
}
}
+   closedir(dir);
+
strbuf_setlen(path, baselen);
-
if (!r  subdir_cb)
r = subdir_cb(subdir_nr, path-buf, data);
 
-   closedir(dir);
return r;
 }
 
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line 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: [PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe

[Johannes Sixt]

Am 12.08.2015 um 13:58 schrieb Erik Faye-Lund:

On Wed, Aug 12, 2015 at 1:07 PM, Johannes Schindelin
johannes.schinde...@gmx.de wrote:

FWIW Git for Windows has this patch (that I wanted to contribute
in  due time, what with being busy with all those tickets) to solve the
problem mentioned in your patch in a different way:

https://github.com/git-for-windows/git/commit/2fff4b54a0d4e5c5e2e4638c9b0739d3c1ff1e45


Yuck. On Windows, it's the extension of a file that dictates what kind
of file it is (and if it's executable or not), not the contents. If we
get a shell script written with the .exe-prefix, it's considered as
an invalid executable by the system. We should consider it the same
way, otherwise we're on the path to user-experience schizophrenia.

I'm not sure I consider this commit a step in the right direction.


I, too, think that it is a wrong decision to pessimize git for the sake 
of a single test case.


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH bc/connect-plink] t5601-clone: remove broken and pointless check for plink.exe

[Johannes Sixt]

Invoking plink requires special treatment, and we have support and even
test cases for the commands 'plink' and 'tortoiseplink'. We also support
.exe variants for these two and there is a test for 'plink.exe'.

On Windows, however, where support for plink.exe would be relevant, the
test case fails because it is not possible to execute a file with a .exe
extension that is actually not a binary executable---it is a shell
script in our test. We have to disable the test case on Windows.

Considering, that 'plink.exe' is irrelevant on non-Windows, let's just
remove the test and assume that the code just works.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 t/t5601-clone.sh | 6 --
 1 file changed, 6 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index 9b34f3c..df69bf6 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -353,12 +353,6 @@ test_expect_success 'plink is treated specially (as 
putty)' '
expect_ssh -P 123 myhost src
 '
 
-test_expect_success 'plink.exe is treated specially (as putty)' '
-   copy_ssh_wrapper_as $TRASH_DIRECTORY/plink.exe 
-   git clone [myhost:123]:src ssh-bracket-clone-plink-1 
-   expect_ssh -P 123 myhost src
-'
-
 test_expect_success 'tortoiseplink is like putty, with extra arguments' '
copy_ssh_wrapper_as $TRASH_DIRECTORY/tortoiseplink 
git clone [myhost:123]:src ssh-bracket-clone-plink-2 
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line 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] revisions --stdin: accept CRLF line terminators

[Johannes Sixt]

On Windows, 'git rebase -i' with rebase.missingCommitsCheck set to
warn or error reports:

   Dropped commits (newer to older):
   'atal: bad revision '410dee56...

The error comes from the git rev-list --stdin invocation in
git-rebase--interactive.sh (function check_todo_list). It is caused by
CRs that end up in the file $todo.miss, because many tools of the MSYS
toolset force LF to CRLF conversion when regular files are written via
stdout.

To fix the error, permit CRLF line terminators when revisions and
pathspec are read using the --stdin option.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 This fixes a new failure in the test suite (t3404.8[67]) on Windows, but
 I got around to debug it only now.

 revision.c|  4 
 t/t6017-rev-list-stdin.sh | 16 
 2 files changed, 20 insertions(+)

diff --git a/revision.c b/revision.c
index cf60c5d..4efedeb 100644
--- a/revision.c
+++ b/revision.c
@@ -1641,6 +1641,8 @@ static void read_pathspec_from_stdin(struct rev_info 
*revs, struct strbuf *sb,
int len = sb-len;
if (len  sb-buf[len - 1] == '\n')
sb-buf[--len] = '\0';
+   if (len  sb-buf[len - 1] == '\r')
+   sb-buf[--len] = '\0';
ALLOC_GROW(prune-path, prune-nr + 1, prune-alloc);
prune-path[prune-nr++] = xstrdup(sb-buf);
}
@@ -1661,6 +1663,8 @@ static void read_revisions_from_stdin(struct rev_info 
*revs,
int len = sb.len;
if (len  sb.buf[len - 1] == '\n')
sb.buf[--len] = '\0';
+   if (len  sb.buf[len - 1] == '\r')
+   sb.buf[--len] = '\0';
if (!len)
break;
if (sb.buf[0] == '-') {
diff --git a/t/t6017-rev-list-stdin.sh b/t/t6017-rev-list-stdin.sh
index 667b375..34c43cf 100755
--- a/t/t6017-rev-list-stdin.sh
+++ b/t/t6017-rev-list-stdin.sh
@@ -75,4 +75,20 @@ test_expect_success 'not only --stdin' '
test_cmp expect actual
 '
 
+test_expect_success 'accept CRLF line terminators' '
+   cat expect -\EOF 
+   7
+
+   file-2
+   EOF
+   q_to_cr input -\EOF 
+   masterQ
+   ^master^Q
+   --Q
+   file-2Q
+   EOF
+   git log --pretty=tformat:%s --name-only --stdin input actual 
+   test_cmp expect actual
+'
+
 test_done
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line 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 ee/clean-remove-dirs] t7300-clean: require POSIXPERM for chmod 0 test

[Johannes Sixt]

A test case introduced by 91479b9c (t7300: add tests to document
behavior of clean and nested git) uses 'chmod 0' to verify that a
subdirectory that has an unreadable .git file is not removed. This can
work only when the system pays attention to the permissions set with
'chmod'. Therefore, set the POSIXPERM prerequisite on the test case.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 This fixes a new failure in the test suite (t3404.8[67]) on Windows, but
 I got around to debug it only now.

 t/t7300-clean.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
index 32e96da..27557d6 100755
--- a/t/t7300-clean.sh
+++ b/t/t7300-clean.sh
@@ -499,7 +499,7 @@ test_expect_success 'should not clean submodules' '
test_path_is_missing to_clean
 '
 
-test_expect_success 'should avoid cleaning possible submodules' '
+test_expect_success POSIXPERM 'should avoid cleaning possible submodules' '
rm -fr to_clean possible_sub1 
mkdir to_clean possible_sub1 
test_when_finished rm -rf possible_sub* 
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line 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 branch command is incompatible with bash

[Johannes Sixt]

Am 27.07.2015 um 23:49 schrieb Junio C Hamano:

Johannes Sixt j...@kdbg.org writes:


Try

   branchName=$(git rev-parse --abbrev-ref HEAD)


Hmm, interesting.

 $ git checkout --orphan notyet
 $ git rev-parse --abbrev-ref HEAD
 $ git symbolic-ref --short HEAD



Please don't scare newcomers with these corner cases ;-)

I see this:

$ git rev-parse --abbrev-ref HEAD
HEAD
fatal: ambiguous argument 'HEAD': unknown revision or path not in the 
working tree.

Use '--' to separate paths from revisions, like this:
'git command [revision...] -- [file...]'
$ git symbolic-ref --short HEAD
notyet

Are you trying to say that the result of 'rev-parse --abbrev-ref HEAD' 
is suboptimal and that of 'symbolic-ref --short HEAD' is OK?


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git branch command is incompatible with bash

[Johannes Sixt]

Am 28.07.2015 um 17:23 schrieb Junio C Hamano:

Johannes Sixt j...@kdbg.org writes:


Are you trying to say that the result of 'rev-parse --abbrev-ref HEAD'
is suboptimal and that of 'symbolic-ref --short HEAD' is OK?


My Interesting was primarily about that I wasn't aware of the
--abbrev-ref option.

Yes, I am sure some time ago I accepted a patch to add it, but I
simply do not see the point, especially because the --short option
to symbolic-ref feels much more superiour.  What branch am I on?
is about symbolic refs, rev-parse is about revisions.

I can see that symbolic-ref --short is much newer than the other
one, so addition of --abbrev-ref to rev-parse may have been a
mistake made while being desperate (i.e. not having a way to do so
with plumbing, we wanted some way to do so and chose poorly).


Heh. Originially, I was about to suggest

   git symbolic-ref -q --short HEAD || git rev-parse --abbrev HEAD

in order to handle the detached HEAD case by printing the abbreviated 
commit name, only to learn that this doesn't do what I expected. Looking 
at the man page of rev-parse, I discovered --abbrev-ref. And learned 
that I actually should have used --short instead of --abbrev in the 
above rev-parse command.


Confusing...

-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git branch command is incompatible with bash

[Johannes Sixt]

Am 27.07.2015 um 14:12 schrieb Anatol Rudolph:

When using the git branch command, git uses a '*' to denote the current
branch. Therefore, in bash this:

$ branchName=$(git branch -q)
$ echo $branchName

produces a directory listing, because the '*' is interpreded by the
shell.


Of course. You would write the last line as

  echo $branchName

These are shell fundamentals.


While an (unwieldly) workaround exists:

$ branchName=$(git symbolic-ref -q HEAD)
$ branchName=${branch##refs/heads/}


If you want to do that in a script, this is not a work-around, but it is 
how you should do it. But you may want to use option --short to save the 
second line.



it would still be nice, if there were a --current flag, that returned
only the current branch name, omitting the star:

$ branchName=$(git branch --current -q)
$ echo $branchName
master


Try

  branchName=$(git rev-parse --abbrev-ref HEAD)

-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Johannes Sixt]

Am 19.07.2015 um 09:37 schrieb Johannes Schindelin:

On 2015-07-19 08:54, Johannes Sixt wrote:

Am 18.07.2015 um 17:21 schrieb Ben Walton:

-   sed -e s/CHANGE_ME/change_me/ file file+ 
-   mv -f file+ file 
+   perl -pi -e s/CHANGE_ME/change_me/ file 


This is problematic. On Windows, perl -i fails when no backup file
extension is specified because perl attempts to replace a file that is
still open; that does not work on Windows.


Let's qualify this a bit better: it actually works with the SDK of
Git  for Windows 2.x.


Good to know!


I really wonder why the previous file+  mv -f file+ file dance
needs to be replaced?


The sed must be replaced because some versions on Solaris choke on the 
incomplete last line in the file.


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Johannes Sixt]

Am 18.07.2015 um 17:21 schrieb Ben Walton:

The space following the last / in a sed command caused Solaris'
xpg4/sed to fail, claiming the program was garbled and exit with
status 2:

% echo 'foo' | /usr/xpg4/bin/sed -e 's/foo/bar/ '
sed: command garbled: s/foo/bar/
% echo $?
2

Fix this by simply removing the unnecessary space.

Additionally, in 99094a7a, a trivial  breakage was fixed. This
exposed a problem with the test when run on Solaris with xpg4/sed that
had gone silently undetected since its introduction in
e4bd10b2. Solaris' sed executes the requested substitution but prints
a warning about the missing newline at the end of the file and exits
with status 2.

% echo CHANGE_ME | \
tr -d \\012 | /usr/xpg4/bin/sed -e 's/CHANGE_ME/change_me/'
sed: Missing newline at end of file standard input.
change_me
% echo $?
2

To work around this, use perl to execute the substitution instead. By
using inplace replacement, we can subsequently drop the mv command.

Signed-off-by: Ben Walton bdwal...@gmail.com
---
  t/t5601-clone.sh   |2 +-
  t/t9500-gitweb-standalone-no-errors.sh |3 +--
  2 files changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t5601-clone.sh b/t/t5601-clone.sh
index fa6be3c..2583f84 100755
--- a/t/t5601-clone.sh
+++ b/t/t5601-clone.sh
@@ -445,7 +445,7 @@ test_expect_success 'clone ssh://host.xz:22/~repo' '
  #IPv6
  for tuah in ::1 [::1] [::1]: user@::1 user@[::1] user@[::1]: [user@::1] 
[user@::1]:
  do
-   ehost=$(echo $tuah | sed -e s/1]:/1]/  | tr -d \133\135)
+   ehost=$(echo $tuah | sed -e s/1]:/1]/ | tr -d \133\135)


Can this not be rewritten as

ehost=$(echo $tuah | sed -e s/1]:/1]/ -e s/[][]//g)

But I admit that it looks like black magic without a comment...


test_expect_success clone ssh://$tuah/home/user/repo 
  test_clone_url ssh://$tuah/home/user/repo $ehost /home/user/repo

diff --git a/t/t9500-gitweb-standalone-no-errors.sh 
b/t/t9500-gitweb-standalone-no-errors.sh
index e94b2f1..eb264f9 100755
--- a/t/t9500-gitweb-standalone-no-errors.sh
+++ b/t/t9500-gitweb-standalone-no-errors.sh
@@ -290,8 +290,7 @@ test_expect_success 'setup incomplete lines' '
echo incomplete | tr -d \\012 file 
git commit -a -m Add incomplete line 
git tag incomplete_lines_add 
-   sed -e s/CHANGE_ME/change_me/ file file+ 
-   mv -f file+ file 
+   perl -pi -e s/CHANGE_ME/change_me/ file 


This is problematic. On Windows, perl -i fails when no backup file 
extension is specified because perl attempts to replace a file that is 
still open; that does not work on Windows. This should work, but I 
haven't tested, yet:


perl -pi.bak -e s/CHANGE_ME/change_me/ file 


git commit -a -m Incomplete context line 
git tag incomplete_lines_ctx 
echo Dominus regit me, file 



-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] Fix sed usage in tests to work around broken xpg4/sed on Solaris

[Johannes Sixt]

Am 19.07.2015 um 20:00 schrieb Ben Walton:

-   sed -e s/CHANGE_ME/change_me/ file file+ 
+   perl -pne s/CHANGE_ME/change_me/ file file+ 


Did you mean '-lpe' or better '-pe' here?

-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] gitk: Add a Copy commit summary command

[Johannes Sixt]

Am 16.07.2015 um 17:29 schrieb Beat Bolli:

When referring to earlier commits in commit messages or other text, one
of the established formats is

 abbrev-sha (summary, author-date)

Add a Copy commit summary command to the context menu that puts this
text for the currently selected commit on the clipboard. This makes it
easy for our users to create well-formatted commit references.

Signed-off-by: Beat Bolli dev+...@drbeat.li
Cc: Paul Mackerras pau...@samba.org
---
  gitk-git/gitk | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 9a2daf3..72a2756 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2617,6 +2617,7 @@ proc makewindow {} {
{mc Diff selected - this command {diffvssel 1}}
{mc Make patch command mkpatch}
{mc Create tag command mktag}
+   {mc Copy commit summary command copysummary}
{mc Write commit to file command writecommit}
{mc Create new branch command mkbranch}
{mc Cherry-pick this commit command cherrypick}
@@ -9341,6 +9342,19 @@ proc mktaggo {} {
  mktagcan
  }

+proc copysummary {} {
+global rowmenuid commitinfo
+
+set id [string range $rowmenuid 0 7]


You abbreviate the commit name to 7 characters. This is too short for 
certain repositories to remain unique. In my group, it is customary to 
abbreviate to 8 charaters. This reduces the usefulness for my use. If 
you don't want to make this a configuration I would suggest to aim for a 
longer commit name as it is simpler to delete excess letters after 
pasting than to add back the missing ones.


Except for this, I like the idea.


+set info $commitinfo($rowmenuid)
+set commit [lindex $info 0]
+set date [formatdate [lindex $info 2]]
+set summary $id (\$commit\, $date)
+
+clipboard clear
+clipboard append $summary
+}
+
  proc writecommit {} {
  global rowmenuid wrcomtop commitinfo wrcomcmd NS




-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] check_and_freshen_file: fix reversed success-check

[Johannes Sixt]

Am 08.07.2015 um 23:03 schrieb Johannes Sixt:

Am 08.07.2015 um 20:33 schrieb Jeff King:

...or maybe in the utime() step there is actually a bug, and we report
failure for no good reason. Ugh.


Ah! That code is less than a year old. When I began to adopt a workflow
requiring force-pushes lately, I wondered why I haven't seen these
failures earlier, because I did do force pushes in the past, but not
that frequently. I thought that I had just been lucky. But this would
explain it.


And, in fact, with this patch these particular failures are gone! Thank 
you so much!


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git force push fails after a rejected push (unpack failed)?

[Johannes Sixt]

Am 07.07.2015 um 21:49 schrieb Jeff King:

On Tue, Jul 07, 2015 at 09:31:25PM +0200, X H wrote:


For the moment, I'm the only one pushing to the remote, always with
the same user (second user is planned). I use git-for-windows which is
based on MSYS2. I have mounted the network share with noacl option so
permissions should be handled by the Windows share. I'm in a group
which has read/write access. I have not configured
core.sharedrepository, I don't think it is useful with noacl since
unix group are not used in this case. The permission for the folder
above the file with permission denied is rw, but this file is read
only so if git try to modify it it won't work.


Ah, so this is not a push to a server, but to a share mounted on the
local box?

That is leaving my realm of expertise. I'm not sure if it could be a
misconfiguration in your share setup, or that git is trying to do
something that would work on a Unix machine, but not on a Windows share.
You might want to ask on the msysgit list:

   https://groups.google.com/forum/#!forum/msysgit


Why does git try to write a file with the same name? If I amend a
commit isn't the sha modified?


Yes, but remember that git stores all of the objects for all of the
commits. So for some reason your push is perhaps trying to send an
object that the other side already has. Usually this does not happen
(the receiver says I already have these commits, do not bother sending
their objects), but it's possible that you have an object that is not
referenced by any commit, or a similar situation. It's hard to say
without looking at the repository.


After a non-fast-forward push fails, a subsequent forced push sends the 
same set of objects, which are already present at the server side, but 
are dangling objects.


Apparently, Git for Windows fails to replace the read-only files that 
live on the network file system.


I have observed this recently, as well. I haven't dug into the detailed 
failure mode, yet. In my case, I have a daily repack running on the 
server side (it's a Samba share on a Linux box), that garbage-collects 
the dangling objects. Usually, the next day the forced push is 
successful. I know this is not very helpful if you can't wait a day for 
the next push attempt...


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs

[Johannes Sixt]

Am 08.07.2015 um 02:55 schrieb David Turner:

Instead of directly writing to and reading from files in
$GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD
and REVERT_HEAD.

Signed-off-by: David Turner dtur...@twopensource.com
---

...

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 366f0bc..e2c5583 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -415,9 +415,9 @@ __git_ps1 ()
fi
elif [ -f $g/MERGE_HEAD ]; then
r=|MERGING
-   elif [ -f $g/CHERRY_PICK_HEAD ]; then
+   elif git rev-parse --quiet --verify CHERRY_PICK_HEAD 
/dev/null; then
r=|CHERRY-PICKING
-   elif [ -f $g/REVERT_HEAD ]; then
+   elif git rev-parse --quiet --verify REVERT_HEAD /dev/null; 
then
r=|REVERTING
elif [ -f $g/BISECT_LOG ]; then
r=|BISECTING


We are trying very hard not to spawn any new processes in __git_ps1(). 
So, I raise a moderate veto against this hunk.


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v7 2/8] cherry-pick: treat CHERRY_PICK_HEAD and REVERT_HEAD as refs

[Johannes Sixt]

Am 08.07.2015 um 21:16 schrieb David Turner:

On Wed, 2015-07-08 at 19:46 +0200, Johannes Sixt wrote:

Am 08.07.2015 um 02:55 schrieb David Turner:

Instead of directly writing to and reading from files in
$GIT_DIR, use ref API to interact with CHERRY_PICK_HEAD
and REVERT_HEAD.

Signed-off-by: David Turner dtur...@twopensource.com
---

...

diff --git a/contrib/completion/git-prompt.sh b/contrib/completion/git-prompt.sh
index 366f0bc..e2c5583 100644
--- a/contrib/completion/git-prompt.sh
+++ b/contrib/completion/git-prompt.sh
@@ -415,9 +415,9 @@ __git_ps1 ()
fi
elif [ -f $g/MERGE_HEAD ]; then
r=|MERGING
-   elif [ -f $g/CHERRY_PICK_HEAD ]; then
+   elif git rev-parse --quiet --verify CHERRY_PICK_HEAD 
/dev/null; then
r=|CHERRY-PICKING
-   elif [ -f $g/REVERT_HEAD ]; then
+   elif git rev-parse --quiet --verify REVERT_HEAD /dev/null; 
then
r=|REVERTING
elif [ -f $g/BISECT_LOG ]; then
r=|BISECTING


We are trying very hard not to spawn any new processes in __git_ps1().
So, I raise a moderate veto against this hunk.


Do you have an alternate suggestion about how to accomplish the same
thing? Here are my ideas:

We could special-case CHERRY_PICK_HEAD and REVERT_HEAD to be files
independent of the ref backend, but that tends to complicate the
backends.  I think this is a mistake.

We could reduce the number from two to one by providing a new
git-am-status command which outputs one of CHERRY-PICKING,
REVERTING, or  (or maybe it would also handle rebase and am).  We
could also generalize it to git-prompt-helper or something by moving
that entire bunch of if statements inside.  This would replace calls to
git describe.

But you probably have a better idea.


Isn't it mere coincidence that the content of these two files looks like 
a non-packed ref? Wouldn't it be better to consider the two akin to 
MERGE_HEAD (which is not a ref because it records more than just a 
commit name)?


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] check_and_freshen_file: fix reversed success-check

[Johannes Sixt]

Am 08.07.2015 um 20:33 schrieb Jeff King:

...or maybe in the utime() step there is actually a bug, and we report
failure for no good reason. Ugh.


Ah! That code is less than a year old. When I began to adopt a workflow 
requiring force-pushes lately, I wondered why I haven't seen these 
failures earlier, because I did do force pushes in the past, but not 
that frequently. I thought that I had just been lucky. But this would 
explain it.


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 12/12] t3901: test git-am encoding conversion

[Johannes Sixt]

Am 02.07.2015 um 20:16 schrieb Paul Tan:

Since d1c5f2a (Add git-am, applymbox replacement., 2005-10-07), git-am
supported the --utf8 and --no-utf8 options, and if set, would pass the
-u flag and the -k flag respectively.

git mailinfo -u will re-code the commit log message and authorship info
in the charset specified by i18n.commitencoding setting, while
git mailinfo -n will disable the re-coding.

Since d84029b (--utf8 is now default for 'git-am', 2007-01-08), --utf8
is set by default in git-am.

Add various encoding conversion tests to t3901 to test git-mailinfo's
encoding conversion. In addition, add a test for --no-utf8 to check that
no encoding conversion will occur if that option is set.

Cc: Junio C Hamano gits...@pobox.com
Signed-off-by: Paul Tan pyoka...@gmail.com
---
  t/t3901-i18n-patch.sh | 62 +++
  1 file changed, 62 insertions(+)

diff --git a/t/t3901-i18n-patch.sh b/t/t3901-i18n-patch.sh
index 75cf3ff..b49bdb7 100755
--- a/t/t3901-i18n-patch.sh
+++ b/t/t3901-i18n-patch.sh
@@ -251,4 +251,66 @@ test_expect_success 'rebase --merge (L/U)' '
check_encoding 2 8859
  '

+test_expect_success 'am (U/U)' '
+   # Apply UTF-8 patches with UTF-8 commitencoding
+   git config i18n.commitencoding UTF-8 
+   . $TEST_DIRECTORY/t3901-utf8.txt 
+
+   git reset --hard master 
+   git am out-u1 out-u2 
+
+   check_encoding 2
+'
+
+test_expect_success 'am (L/L)' '
+   # Apply ISO-8859-1 patches with ISO-8859-1 commitencoding
+   git config i18n.commitencoding ISO8859-1 
+   . $TEST_DIRECTORY/t3901-8859-1.txt 
+
+   git reset --hard master 
+   git am out-l1 out-l2 
+
+   check_encoding 2 8859
+'


This test case must be protected by !MINGW, just like the last case 
below and other cases that are already in the file. See 32f4cb6cee for 
details.



+
+test_expect_success 'am (U/L)' '
+   # Apply ISO-8859-1 patches with UTF-8 commitencoding
+   git config i18n.commitencoding UTF-8 
+   . $TEST_DIRECTORY/t3901-utf8.txt 
+   git reset --hard master 
+
+   # am specifies --utf8 by default.
+   git am out-l1 out-l2 
+
+   check_encoding 2
+'
+
+test_expect_success 'am --no-utf8 (U/L)' '
+   # Apply ISO-8859-1 patches with UTF-8 commitencoding
+   git config i18n.commitencoding UTF-8 
+   . $TEST_DIRECTORY/t3901-utf8.txt 
+
+   git reset --hard master 
+   git am --no-utf8 out-l1 out-l2 2err 
+
+   # commit-tree will warn that the commit message does not contain valid 
UTF-8
+   # as mailinfo did not convert it
+   grep did not conform err 
+
+   check_encoding 2
+'
+
+test_expect_success !MINGW 'am (L/U)' '
+   # Apply UTF-8 patches with ISO-8859-1 commitencoding
+   git config i18n.commitencoding ISO8859-1 
+   . $TEST_DIRECTORY/t3901-8859-1.txt 
+
+   git reset --hard master 
+   # mailinfo will re-code the commit message to the charset specified by
+   # i18n.commitencoding
+   git am out-u1 out-u2 
+
+   check_encoding 2 8859
+'
+
  test_done



-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git force push fails after a rejected push (unpack failed)?

[Johannes Sixt]

Am 08.07.2015 um 20:05 schrieb Jeff King:

We also don't write objects directly, of course; we write to a temporary
file and try to link them into place. It really sounds more like the
objects/d9 directory is where the permission problems are. But, hmm...


Not on Windows: A read-only file cannot be overwritten or removed, 
regardless of the permissions of the directory. We do treat this case 
mingw_rename, but I have a slight suspicion that this does not work 
sufficiently reliably on networked file systems.


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2] fetch-pack: optionally save packs to disk

[Johannes Sixt]

Am 11.06.2015 um 20:59 schrieb Augie Fackler:

When developing server software, it's often helpful to save a
potentially-bogus pack for later analysis. This makes that trivial,
instead of painful.


When you develop server software, shouldn't you test drive the server 
via the bare metal protocol anyway? That *is* painful, but unavoidable 
because you must harden the server against any garbage that a 
potentially malicous client could throw at it. Restricting yourself to a 
well-behaved client such as fetch-pack is only half the deal.


That said, I do think that fetch-pack could learn a mode that makes it 
easier to debug the normal behavior of a server (if such a mode is 
missing currently).


What is the problem with the current fetch-pack implementation? Does it 
remove a bogus packfile after download? Does it abort during download 
when it detects a broken packfile? Does --keep not do what you need?


Instead of your approach (which forks off tee to dump a copy of the 
packfile), would it not be simpler to add an option --debug-pack 
(probably not the best name) that skips the cleanup step when a broken 
packfile is detected and prints the name of the downloaded packfile?



diff --git a/fetch-pack.c b/fetch-pack.c
index a912935..fe6ba58 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -684,7 +684,7 @@ static int get_pack(struct fetch_pack_args *args,
const char *argv[22];
char keep_arg[256];
char hdr_arg[256];
-   const char **av, *cmd_name;
+   const char **av, *cmd_name, *savepath;
int do_keep = args-keep_pack;
struct child_process cmd = CHILD_PROCESS_INIT;
int ret;
@@ -708,9 +708,8 @@ static int get_pack(struct fetch_pack_args *args,
cmd.argv = argv;
av = argv;
*hdr_arg = 0;
+   struct pack_header header;
if (!args-keep_pack  unpack_limit) {
-   struct pack_header header;
-
if (read_pack_header(demux.out, header))
die(protocol error: bad pack header);
snprintf(hdr_arg, sizeof(hdr_arg),
@@ -762,7 +761,44 @@ static int get_pack(struct fetch_pack_args *args,
*av++ = --strict;
*av++ = NULL;

-   cmd.in = demux.out;
+   savepath = getenv(GIT_SAVE_FETCHED_PACK_TO);
+   if (savepath) {
+   struct child_process cmd2 = CHILD_PROCESS_INIT;
+   const char *argv2[22];
+   int pipefds[2];
+   int e;
+   const char **av2;
+   cmd2.argv = argv2;
+   av2 = argv2;
+   *av2++ = tee;
+   if (*hdr_arg) {
+   /* hdr_arg being nonempty means we already read the
+* pack header from demux, so we need to drop a pack
+* header in place for tee to append to, otherwise
+* we'll end up with a broken pack on disk.
+*/


/*
 * Write multi-line comments
 * like this (/* on its own line)
 */


+   int fp;
+   struct sha1file *s;
+   fp = open(savepath, O_CREAT | O_TRUNC | O_WRONLY, 0666);
+   s = sha1fd_throughput(fp, savepath, NULL);
+   sha1write(s, header, sizeof(header));
+   sha1flush(s);


Are you abusing sha1write() and sha1flush() to write a byte sequence to 
a file? Is write_in_full() not sufficient?



+   close(fp);
+   /* -a is supported by both GNU and BSD tee */
+   *av2++ = -a;
+   }
+   *av2++ = savepath;
+   *av2++ = NULL;
+   cmd2.in = demux.out;
+   e = pipe(pipefds);
+   if (e != 0)
+   die(couldn't make pipe to save pack);


start_command() can create the pipe for you. Just say cmd2.out = -1.


+   cmd2.out = pipefds[1];
+   cmd.in = pipefds[0];
+   if (start_command(cmd2))
+   die(couldn't start tee to save a pack);


When you call start_command(), you must also call finish_command(). 
start_command() prints an error message for you; you don't have to do 
that (the start_command() in the context below is a bad example).



+   } else
+   cmd.in = demux.out;
cmd.git_cmd = 1;
if (start_command(cmd))
die(fetch-pack: unable to fork off %s, cmd_name);
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 58207d8..bf4640d 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -82,11 +82,23 @@ test_expect_success 'fetch changes via http' '
test_cmp file clone/file
  '

+test_expect_success 'fetch changes via http and save pack' '
+   echo content file 
+   git commit -a -m two 

Re: [PATCH 03/14] lockfile: remove some redundant functions

[Johannes Sixt]

Am 10.06.2015 um 19:40 schrieb Junio C Hamano:

Michael Haggerty mhag...@alum.mit.edu writes:


Remove the following functions and rewrite their callers to use the
equivalent tempfile functions directly:

* fdopen_lock_file() - fdopen_tempfile()
* reopen_lock_file() - reopen_tempfile()
* close_lock_file() - close_tempfile()


Hmph,

My knee-jerk reaction was I thought lockfile abstraction was
fine---why do we expose the implementation detail of the lockfile,
which is now happen to be implemented on top of the tempfile API, to
the callers?


Just for the record, I had exactly the same reaction, and I find this 
transition against the spirit of a self-contained lockfile API.


-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] lockfile: wait using sleep_millisec() instead of select()

[Johannes Sixt]

Use the new function sleep_millisec() to delay execution for a short
time. This avoids the invocation of select() with just a timeout, but
no file descriptors. Such a use of select() is quit with EINVAL on
Windows, leading to no delay at all.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 lockfile.c | 10 +-
 1 file changed, 1 insertion(+), 9 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 3f5b699..fb78bda 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -157,14 +157,6 @@ static int lock_file(struct lock_file *lk, const char 
*path, int flags)
return lk-fd;
 }
 
-static int sleep_microseconds(long us)
-{
-   struct timeval tv;
-   tv.tv_sec = 0;
-   tv.tv_usec = us;
-   return select(0, NULL, NULL, NULL, tv);
-}
-
 /*
  * Constants defining the gaps between attempts to lock a file. The
  * first backoff period is approximately INITIAL_BACKOFF_MS
@@ -214,7 +206,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
backoff_ms = multiplier * INITIAL_BACKOFF_MS;
/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */
wait_ms = (750 + rand() % 500) * backoff_ms / 1000;
-   sleep_microseconds(wait_ms*1000);
+   sleep_millisec(wait_ms);
remaining_ms -= wait_ms;
 
/* Recursion: (n+1)^2 = n^2 + 2n + 1 */
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line 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/4] help.c: wrap wait-only poll() invocation in sleep_millisec()

[Johannes Sixt]

We want to use the new function elsewhere in a moment.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 cache.h   | 1 +
 help.c| 2 +-
 wrapper.c | 5 +
 3 files changed, 7 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index 54f108a..328cdb7 100644
--- a/cache.h
+++ b/cache.h
@@ -1680,5 +1680,6 @@ int stat_validity_check(struct stat_validity *sv, const 
char *path);
 void stat_validity_update(struct stat_validity *sv, int fd);
 
 int versioncmp(const char *s1, const char *s2);
+void sleep_millisec(int millisec);
 
 #endif /* CACHE_H */
diff --git a/help.c b/help.c
index 2072a87..de1279b 100644
--- a/help.c
+++ b/help.c
@@ -372,7 +372,7 @@ const char *help_unknown_cmd(const char *cmd)
if (autocorrect  0) {
fprintf_ln(stderr, _(in %0.1f seconds 
automatically...),
(float)autocorrect/10.0);
-   poll(NULL, 0, autocorrect * 100);
+   sleep_millisec(autocorrect * 100);
}
return assumed;
}
diff --git a/wrapper.c b/wrapper.c
index c1a663f..ff49807 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -595,3 +595,8 @@ int write_file(const char *path, int fatal, const char 
*fmt, ...)
}
return 0;
 }
+
+void sleep_millisec(int millisec)
+{
+   poll(NULL, 0, millisec);
+}
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/4] lockfile: convert retry timeout computations to millisecond

[Johannes Sixt]

When the goal is to wait for some random amount of time up to one
second, it is not necessary to compute with microsecond precision.
This is a preparation to re-use sleep_millisec().

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 lockfile.c | 21 -
 1 file changed, 8 insertions(+), 13 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 738f202..3f5b699 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -184,7 +184,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
 {
int n = 1;
int multiplier = 1;
-   long remaining_us = 0;
+   long remaining_ms = 0;
static int random_initialized = 0;
 
if (timeout_ms == 0)
@@ -195,16 +195,11 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
random_initialized = 1;
}
 
-   if (timeout_ms  0) {
-   /* avoid overflow */
-   if (timeout_ms = LONG_MAX / 1000)
-   remaining_us = timeout_ms * 1000;
-   else
-   remaining_us = LONG_MAX;
-   }
+   if (timeout_ms  0)
+   remaining_ms = timeout_ms;
 
while (1) {
-   long backoff_ms, wait_us;
+   long backoff_ms, wait_ms;
int fd;
 
fd = lock_file(lk, path, flags);
@@ -213,14 +208,14 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
return fd; /* success */
else if (errno != EEXIST)
return -1; /* failure other than lock held */
-   else if (timeout_ms  0  remaining_us = 0)
+   else if (timeout_ms  0  remaining_ms = 0)
return -1; /* failure due to timeout */
 
backoff_ms = multiplier * INITIAL_BACKOFF_MS;
/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */
-   wait_us = (750 + rand() % 500) * backoff_ms;
-   sleep_microseconds(wait_us);
-   remaining_us -= wait_us;
+   wait_ms = (750 + rand() % 500) * backoff_ms / 1000;
+   sleep_microseconds(wait_ms*1000);
+   remaining_ms -= wait_ms;
 
/* Recursion: (n+1)^2 = n^2 + 2n + 1 */
multiplier += 2*n + 1;
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/4] lockfile: replace random() by rand()

[Johannes Sixt]

On Windows, we do not have functions srandom() and random(). Use srand()
and rand(). These functions produce random numbers of lesser quality,
but for the purpose (a retry time-out) they are still good enough.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
 This is the same version I posted earlier.

 lockfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 30e65e9..738f202 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -191,7 +191,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
return lock_file(lk, path, flags);
 
if (!random_initialized) {
-   srandom((unsigned int)getpid());
+   srand((unsigned int)getpid());
random_initialized = 1;
}
 
@@ -218,7 +218,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
 
backoff_ms = multiplier * INITIAL_BACKOFF_MS;
/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */
-   wait_us = (750 + random() % 500) * backoff_ms;
+   wait_us = (750 + rand() % 500) * backoff_ms;
sleep_microseconds(wait_us);
remaining_us -= wait_us;
 
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line 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/4] Fix file locking with retry and timeout on Windows

[Johannes Sixt]

The first patch is the same that I posted earlier. It fixes a build
failure on Windows on master due to missing random/srandom.

The remaining 3 patches replace the select() invocation that waits
for a short time period by the version with poll() that we already
use in help.c. This is necessary because a select() call where all
three sets of file descriptors are empty is not supported on Windows.

Johannes Sixt (4):
  lockfile: replace random() by rand()
  help.c: wrap wait-only poll() invocation in sleep_millisec()
  lockfile: convert retry timeout computations to millisecond
  lockfile: wait using sleep_millisec() instead of select()

 cache.h|  1 +
 help.c |  2 +-
 lockfile.c | 31 +--
 wrapper.c  |  5 +
 4 files changed, 16 insertions(+), 23 deletions(-)

-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line 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 mh/lockfile-retry] lockfile: replace random() by rand()

[Johannes Sixt]

Am 30.05.2015 um 19:12 schrieb Junio C Hamano:

Johannes Sixt j...@kdbg.org writes:


There you have it: Look the other way for a while, and people start
using exotic stuff... ;)


Is it exotic to have random/srandom?  Both are in POSIX and 4BSD;
admittedly rand/srand are written down in C89 and later, so they
might be more portable, but I recall the prevailing wisdom is to
favor random over rand for quality of randomness and portability, so
I am wondering if it may be a better approach to keep the code as-is
and do a compat/random.c based on either rand/srand (or use posix
sample implementation [*1*]).


For our purposes here, the linear congruence of rand() is certainly 
sufficient. At this time, compatibility functions for random/srandom 
would just mean a lot of work for little gain.





[Reference]

*1* http://pubs.opengroup.org/onlinepubs/9699919799/functions/rand.html





This is a build breakage of master on Windows. There are also a few
new test suite failures. On of them is in t1404#2, indicating that
a DF conflict takes a different error path. I haven't debugged, yet.
The lock file retry test fails, too. I'll report back as time permits.

  lockfile.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 5a93bc7..ee5cb01 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -191,7 +191,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
return lock_file(lk, path, flags);

if (!random_initialized) {
-   srandom((unsigned int)getpid());
+   srand((unsigned int)getpid());
random_initialized = 1;
}

@@ -218,7 +218,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,

backoff_ms = multiplier * INITIAL_BACKOFF_MS;
/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */
-   wait_us = (750 + random() % 500) * backoff_ms;
+   wait_us = (750 + rand() % 500) * backoff_ms;
sleep_microseconds(wait_us);
remaining_us -= wait_us;




-- Hannes

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH mh/lockfile-retry] lockfile: replace random() by rand()

[Johannes Sixt]

On Windows, we do not have functions srandom() and random(). Use srand()
and rand(). These functions produce random numbers of lesser quality,
but for the purpose (a retry time-out) they are still good enough.

Signed-off-by: Johannes Sixt j...@kdbg.org
---
There you have it: Look the other way for a while, and people start
using exotic stuff... ;)

This is a build breakage of master on Windows. There are also a few
new test suite failures. On of them is in t1404#2, indicating that
a DF conflict takes a different error path. I haven't debugged, yet.
The lock file retry test fails, too. I'll report back as time permits.

 lockfile.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/lockfile.c b/lockfile.c
index 5a93bc7..ee5cb01 100644
--- a/lockfile.c
+++ b/lockfile.c
@@ -191,7 +191,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
return lock_file(lk, path, flags);
 
if (!random_initialized) {
-   srandom((unsigned int)getpid());
+   srand((unsigned int)getpid());
random_initialized = 1;
}
 
@@ -218,7 +218,7 @@ static int lock_file_timeout(struct lock_file *lk, const 
char *path,
 
backoff_ms = multiplier * INITIAL_BACKOFF_MS;
/* back off for between 0.75*backoff_ms and 1.25*backoff_ms */
-   wait_us = (750 + random() % 500) * backoff_ms;
+   wait_us = (750 + rand() % 500) * backoff_ms;
sleep_microseconds(wait_us);
remaining_us -= wait_us;
 
-- 
2.3.2.245.gb5bf9d3

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html