Re: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Johannes Sixt]

Am 15.01.2018 um 03:37 schrieb Randall S. Becker:

On January 14, 2018 4:33 PM, I wrote:

The exotic error code coming back from perl is 162. I can muck with it, if
there was a value more useful to git.
*  553  } else if (WIFEXITED(status)) {
*  554  code = WEXITSTATUS(status);
(eInspect 3,887):p code
$4 = 162

The perl code uses die to terminate with a non-specific non-zero error code.
What it seems is that there is an assumed value that the git tests depend on,
but perl.org describes the following:

"die raises an exception. Inside an eval the error message is stuffed into $@
and the eval is terminated with the undefined value. If the exception is
outside of all enclosing evals, then the uncaught exception prints LIST to
STDERR and exits with a non-zero value. If you need to exit the process with
a specific exit code, see exit."


I take "die exits with non-zero" as a piece of information for the 
*users* so that they can write "if perl foo.pl; then something; fi" in 
shell scripts. I do *not* interpret it as leeway for implementers of 
perl to choose any random value as exit code. Choosing 162 just to be 
funky would be short-sighted. [I'm saying all this without knowing how 
perl specifies 'die' beyond the paragraph you cited. Perhaps there's 
more about 'die' that justifies exit code 162.] I'd say that the perl 
port is broken.




So it seems that we might need to either not use die or change the tests not
to care about the exit code for specific tests involving perl. Suggestions?


Sadly, while changing the funky 162 completion code to 255 works
fine for t9001, it causes a bunch of other tests to fail (parts of
1308 and most of 1404). I can't tall whether this is test suite or
code related but I'm caught in the middle. Going to the platform
developers may eventually get the fix for t9001 (fixing perl), but
otherwise, I'm not sure why changing 162 to 255 causes 1308 and 1404
to blow so badly. In any event, I'm putting this away for a few days
due to $DAYJOB.


Why do you not choose 1? He, on my Linux box perl -e die exits with 255. 
So...


-- Hannes

Re: [BUG] test_must_fail: does not correctly detect failures - Was Git 2.16.0-rc2 Test Summary on NonStop

[Johannes Sixt]

Am 14.01.2018 um 17:50 schrieb Randall S. Becker:

Follow-up: This looks like the completion code from perl on NonStop is not
the same as expected by git in the case of failures. I need to debug this to
get more details to the team. We have had completion issues before relating
to interpretation problems between perl, bash, and git in this platform, so
I'm assuming this to be likely again but need to track down the specifics.
Can anyone point me to where the detection is within git or the execv?


Perhaps you are looking for wait_or_whine() in run-command.c? But this 
function cannot do anything if perl alread exits with an exotic code (> 
128 even though no signal was received).


-- Hannes

Re: [PATCH] run-command.c: print env vars when GIT_TRACE is set

[Johannes Sixt]

Am 11.01.2018 um 11:07 schrieb Jeff King:

The output for a single command is pretty shell-like due to the quoting:

   $ GIT_TRACE=1 ./git upload-pack . >/dev/null
   [...]run_command: 'git-upload-pack' '.'

You could copy and paste that to a shell if you wanted.  And with
environment variables, that remains so:

   $ GIT_TRACE=1 ./git ls-remote https://github.com/git/git >/dev/null
   [...]run_command: 'GIT_DIR=.git' 'git-remote-https' 'https://[...]'


Not quite, though. For variable assignments to be recognized as such, 
the name and equal sign must not be quoted:


  GIT_DIR='.git' 'git-remote-https' 'https://[...]'

-- Hannes

Re: [PATCH] t3900: add some more quotes

[Johannes Sixt]

Am 10.01.2018 um 10:58 schrieb Beat Bolli:

In 89a70b80 ("t0302 & t3900: add forgotten quotes", 2018-01-03), quotes
were added to protect against spaces in $HOME. In the test_when_finished
hander, two files are deleted which must be quoted individually.

Signed-off-by: Beat Bolli 
---
  t/t3900-i18n-commit.sh | 8 
  1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t3900-i18n-commit.sh b/t/t3900-i18n-commit.sh
index 9e4e694d9..dc00db87b 100755
--- a/t/t3900-i18n-commit.sh
+++ b/t/t3900-i18n-commit.sh
@@ -40,7 +40,7 @@ test_expect_success 'UTF-16 refused because of NULs' '
  '
  
  test_expect_success 'UTF-8 invalid characters refused' '

-   test_when_finished "rm -f \"$HOME/stderr $HOME/invalid\"" &&
+   test_when_finished "rm -f \"$HOME/stderr\" \"$HOME/invalid\"" &&


Should that not better be

test_when_finished "rm -f \"\$HOME/stderr\" \"\$HOME/invalid\""

i.e., delay the expansion of $HOME to protect against double-quotes in 
the path?


-- Hannes

Re: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Johannes Sixt]

Am 10.01.2018 um 01:12 schrieb Randall S. Becker:
> On January 9, 2018 6:01 PM, Johannes Sixt wrote:
> I'm encountering strange warnings, while looking into the details of what 
> test t0001 fails in spots. These include:
> #24 warning: templates not found x00...[lots of 0 
> deleted...]
> which exceeds 2K, but that's just content, right, and not causing an apparent 
> breakage.

This warning occurs also on Linux and Windows. I think it is by design
and not something to be fixed.

> 
> # 34. Admittedly it was shorter than 2K, but there is something weird in this 
> path that I can't find, causing a failure out of fts_read from gnulib.
> Initialized empty Git repository in /home/ituglib/randall/git/t/trash 
> directory.t0001-init/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/123456789abcdef/newdir/.git/
> rm: fts_read failed: No such file or directory
> 
> This error is coming from some of shell utilities (in this case rm)
> used in the test rather than git code itself.

So, the problem is not in the git executable. This does not warrant a
change in the build process, yet.

>  While well within the
> supported path length of the operating system/platform (1K), there is
> an acknowledged issue that is causing breakage when paths get large
> enough (even only this large, unfortunately). We're at 221 breaks out
> of 12982-ish, which is good, but have to otherwise visually check
> each breakage until the fts_read problem is resolved - I know what
> the issue is, but I don't have the auth to resolve it, so waiting on
> HPE platform development for that. Of course, manually patching that
> many breaks is equally unwieldy, so I'm willing to tolerate not
> having this patch applied at this time.

Let me propose a different workaround. In my build on Windows, I inject
a few blind spots in the test suite using GIT_SKIP_TESTS for cases where
I do not have time to find a fix. It looks like this:

diff --git a/t/Makefile b/t/Makefile
index 96317a35f4..fd8b18c3c0 100644
--- a/t/Makefile
+++ b/t/Makefile
@@ -103,3 +103,19 @@ perf:
$(MAKE) -C perf/ all
 
 .PHONY: pre-clean $(T) aggregate-results clean valgrind perf
+
+# see 
https://public-inbox.org/git/alpine.DEB.2.21.1.1710260008270.37495@virtualbox/
+# the suggested solution is for MSYS2; don't have time to fix this for MSYS
+GIT_SKIP_TESTS += t0021.1[5-9] t0021.2[0-6]
+# special file names
+GIT_SKIP_TESTS += t1300.14[02]
+# GIT_SSH_COMMAND with args forwarded incompletely via git clone to 
test_fake_ssh
+GIT_SKIP_TESTS += t5601.5[01]
+# unknown failure in shallow submodule test
+GIT_SKIP_TESTS += t7406.46
+# mktemp missing?
+GIT_SKIP_TESTS += t7610.22
+export GIT_SKIP_TESTS
+
+NO_SVN_TESTS=SkipThem
+export NO_SVN_TESTS
-- 

Build the list of test cases that do not pass, until the test suite runs
through. Then start fixing the cases.

It is not foolproof, but very effective in keeping the focus on new
cases. You have to run tests with 'make' so that the variable is picked
up. Also, when somebody adds new tests in front of the mentioned cases,
the numbers must be adjusted.

-- Hannes

Re: [git-for-windows] Re: [ANNOUNCE] Git v2.16.0-rc1

[Johannes Sixt]

Am 10.01.2018 um 18:37 schrieb Johannes Schindelin:

On Tue, 9 Jan 2018, Junio C Hamano wrote:

Johannes Schindelin  writes:


diff --git a/t/t0021/rot13-filter.pl b/t/t0021/rot13-filter.pl
index f1678851de9..470107248eb 100644
--- a/t/t0021/rot13-filter.pl
+++ b/t/t0021/rot13-filter.pl
@@ -31,7 +31,22 @@
  #
  
  use 5.008;

-use lib (split(/:/, $ENV{GITPERLLIB}));
+sub gitperllib {
+...
+   if ($ENV{GITPERLLIB} =~ /;/) {
+   return split(/;/, $ENV{GITPERLLIB});
+   }
+   return split(/:/, $ENV{GITPERLLIB});
+}


This cannot be the whole story for a few reasons.

  - In t/test-lib.sh we see this:


GITPERLLIB="$GIT_BUILD_DIR"/perl/blib/lib:"$GIT_BUILD_DIR"/perl/blib/arch/auto/Git
export GITPERLLIB

If this part wants to split with ';', then the joining needs to
be done with ';' to match, no?


No.

It is a lot more complicated than that. As you know, on Linux there is
this implicit assumption that path lists are colon-separated. As a
consequence, Cygwin does the same (because it would be too hard to port
all those Linux/Unix projects to stop assuming colon-separated path lists,
right?)

This is what Cygwin's Bash does (and hence the MSYS2 Bash used by Git for
Windows, too).

Then the MSYS2 Bash calls git.exe, which is *not* an MSYS2 program, hence
the MSYS2 runtime knows that it has to convert the path lists to Windows
paths separated by semicolons.

The next thing happening in our case is that the Perl script is called
from git.exe. Now, the MSYS2 runtime (implicitly spun up by the MSYS2 Perl
interpreter) does *not* convert those path lists back to Unix-like paths
separated by colons.


But this is a bug in MSYS2, isn't it? The MSYS2 runtime should detect 
that it was not invoked by some other MSYS2 process. The MSYS2 startup 
sequence should assume in this case that the environment is 
Windows-style and convert to POSIX before it calls into perl's main().



And that's why the Unix shell script can happily construct the
colon-separated list, and the Perl script will *still* receive the
semicolon-separated version of it.


  - In addition to t0021, there are similar split with colon in 0202,
9000 and 9700, yet I am getting the feeling that you observed the
issue only in0021, to which I do not think of a good explanation
why.


Here is the good explanation: t0021 relies on a Perl package that is not
yet installed. t0202 relies on Git::I18N, of which there is a version
installed in Git for Windows' SDK. (I do not bother to slow down the test
runs by the Subversion tests, I always skip all of them, that's why t9*
does not matter to me.)


t0202 and the t9* cases are different because perl is invoked by bash 
directly (AFAICS), without a non-MSYS2 process between them. There is no 
difference when the path conversion is omitted in this case by design or 
due to a bug.


-- Hannes

Re: [PATCH] Prototype PATH_MAX length detection in tests, demonstrated in t0001-init.sh

[Johannes Sixt]

Am 09.01.2018 um 19:12 schrieb Randall S. Becker:

This patch create a configuration variable PATH_MAX that
corresponds with the value in limits.h. The value of PATH_MAX,
if supplied, is added to BASIC_CFLAGS and will validate with
limits.h. PATH_MAX is also added to GIT-BUILD-OPTIONS and is
available in the git test suite.

This patch also creates a test_expected_success_cond, taking a
single function as first argument. In the t0001-init.sh case,
subtest 34 this function is test_path_max_is_sane, although any
0/1 returning function can be used. The prototype allows the long base
path test to be skipped if PATH_MAX is less than 2048 bytes.


OK, but...


diff --git a/t/t0001-init.sh b/t/t0001-init.sh
index c4814d2..58dad87 100755
--- a/t/t0001-init.sh
+++ b/t/t0001-init.sh
@@ -315,7 +315,7 @@ test_expect_success 'init with separate gitdir' '
test_path_is_dir realgitdir/refs
  '

-test_expect_success 'init in long base path' '
+test_expect_success_cond 'test_path_max_is_sane' 'init in long base path' '
# exceed initial buffer size of strbuf_getcwd()
component=123456789abcdef &&
test_when_finished "chmod 0700 $component; rm -rf $component" &&


... why would you want to skip this test? If I'm reading the test case 
correctly, it requires only a path length of 127 plus whatever your 
build directory is plus a score for the trash directory. That should 
pose a problem only if your system is even more crippled than Windows 
with its PATH_MAX of 260.



+test_path_max_is_sane() {
+   if test -z "$PATH_MAX"
+   then
+   retval=1
+   elif test $PATH_MAX -ge 2048
+   then
+   retval=1
+   else
+   retval=0
+   fi
+   return "$retval"
+}


This can probably be reduced to

test_path_max_is_sane () {
test "${PATH_MAX:-4000}" -ge 2048
}

(Style note: we have a blank before the () pair in shell scripts.)

-- Hannes

Re: [PATCH] oidset: don't return value from oidset_init

[Johannes Sixt]

Am 09.01.2018 um 00:26 schrieb Junio C Hamano:

Thomas Gummerer  writes:


c3a9ad3117 ("oidset: add iterator methods to oidset", 2017-11-21)
introduced a 'oidset_init()' function in oidset.h, which has void as
return type, but returns an expression.
...
diff --git a/oidset.h b/oidset.h
index 783abceccd..40ec5f87fe 100644
--- a/oidset.h
+++ b/oidset.h
@@ -27,7 +27,7 @@ struct oidset {
  
  static inline void oidset_init(struct oidset *set, size_t initial_size)

  {
-   return oidmap_init(>map, initial_size);
+   oidmap_init(>map, initial_size);
  }


Makes sense.  Perhaps "inline" hids this from error-checking
compilers, I wonder?


outmap_init returns void itself. It is a modern language feature, I 
guess, that this void "value" can be forwarded in a return statement.


-- Hannes

Re: [PATCH v5 00/34] Add directory rename detection to git

[Johannes Sixt]

Am 03.01.2018 um 22:02 schrieb Elijah Newren:

On Wed, Jan 3, 2018 at 2:57 AM, Johannes Sixt <j...@kdbg.org> wrote:


I tested the series on Windows recently. It requires the patch below.
I don't know whether this is indicating some portability issues of grep
(^ being used in the middle of a RE instead of at the very beginning) or
just a quirk in my setup.


Thanks for testing it out.  What version of Windows were you running
on?  With cygwin or without?  I tested previously on cygwin (I think
on Windows Server 2012??) and got all the tests passing there,
eventually[1].  I'm not sure I can find access to any other Windows
systems, but I'd be happy to take a look if I can.

[1] 
https://public-inbox.org/git/cabpp-bej6-mry0ocz1wwetrtg_iehkzodcuon_puukvywau...@mail.gmail.com/


I have an ancient MinGW setup, where I build "vanilla" Git (not exactly 
vanilla, but also not with the many patches that Git for Windows carries).



The need to backslash escape a caret for a literal match when it
appears in the middle of the string makes sense.  Thanks for sending
along the patch.  Would you prefer I squashed it into the series
(still sitting in 'pu'), or keep your patch separate?  I'm fine with
either, I'm just unsure the protocol here.


Please squash into the relevant commits so that the series is bisectable 
if the need arises.



But it still does not pass the test suite because the system does not
like file names such as y/c~HEAD:

++ grep 'Refusing to lose dirty file at z/c' out
Refusing to lose dirty file at z/c
++ grep -q stuff x/b y/a y/c y/c~HEAD z/c
grep: y/c: Invalid request code
error: last command exited with $?=2
not ok 94 - 11d-check: Avoid losing not-uptodate with rename + D/F conflict


This is exceptionally odd.  The actual line from the testsuite was
   grep -q stuff */*

which suggests your shell is both doing the pathname expansion and
treating the resulting filename not as a string but as something to be
interpreted that happens to have some kind of special
characters/commands, and then choking on the result.  Super weird.  I
could probably work around this by just running
   grep -q stuff z/c

I think I had the asterisks in there because I was thinking in terms
of directory rename detection potentially moving the file, but that's
probably just overkill.  Does the test pass for you with that change?


I can test on Monday at the earliest. If it's that easy to fix my 
failures, I'd appreciate to go this route. But otherwise, I can deal 
with the situation, so we don't need to complicate things just to please 
my exotic setup.



(If so, there are also two similar tests that I'd need to make similar
changes to.)

However, although that might fix this particular case, it suggests
some fragility of the tests and filenames for whatever system you
happen to be using.  merge-recursive.c's unique_path has created
filenames with tilde's in them for many years, it may just be that I'm
the first to use the resulting file in combination with grep to ensure
the contents are as we expect.  There may be other issues lurking
(even if not yet appearing in the testsuite) for your system when
dealing with merge conflicts.


I can't recall having seen issues around tildas in file names, either. 
It may be a new situation. I'll investigate.


-- Hannes

Re: [PATCH v5 00/34] Add directory rename detection to git

[Johannes Sixt]

Am 03.01.2018 um 01:02 schrieb Elijah Newren:
> On Wed, Dec 27, 2017 at 8:13 PM, Elijah Newren <new...@gmail.com> wrote:
>> This patchset introduces directory rename detection to merge-recursive.  See
>>https://public-inbox.org/git/20171110190550.27059-1-new...@gmail.com/
>> for the first series (including design considerations, etc.), and follow-up
>> series can be found at
>>https://public-inbox.org/git/20171120220209.15111-1-new...@gmail.com/
>>https://public-inbox.org/git/20171121080059.32304-1-new...@gmail.com/
>>https://public-inbox.org/git/20171129014237.32570-1-new...@gmail.com/
>>
>> Changes since v4:
>>* Squashed Junio's GETTEXT_POISON fixes into the appropriate commits
> 
> As per Jonathan's request[1], shamelessly re-sending Stefan's request
> for further review.  :-)
> 
> Quoting Stefan:
> 
> "I have reviewed the first three patches (which could form an
> independent series)
> that it would warrant a Reviewed-By: Stefan Beller <sbel...@google.com>
> 
> While I reviewed the earlier versions of the later patches, I would
> prefer if there is another reviewer for these as it seems like a bigger
> contribution at a core functionality.
> 
> I cc'd some people who were active in some form of rename detection
> work earlier; could you review this series, please?"
> 
> My note: Stefan also looked through the testcases pretty closely and
> even suggested additional tests, which would account for another 11
> patches or so, but extra eyes on any part of the series always
> welcome.

I tested the series on Windows recently. It requires the patch below.
I don't know whether this is indicating some portability issues of grep
(^ being used in the middle of a RE instead of at the very beginning) or
just a quirk in my setup.

But it still does not pass the test suite because the system does not
like file names such as y/c~HEAD:

++ grep 'Refusing to lose dirty file at z/c' out
Refusing to lose dirty file at z/c
++ grep -q stuff x/b y/a y/c y/c~HEAD z/c
grep: y/c: Invalid request code
error: last command exited with $?=2
not ok 94 - 11d-check: Avoid losing not-uptodate with rename + D/F conflict

I haven't debugged this any further, yet.

---- 8< ----
From: Johannes Sixt <j...@kdbg.org>
Date: Fri, 22 Dec 2017 09:33:13 +0100
Subject: [PATCH] fixup directory rename tests

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t6043-merge-rename-directories.sh | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/t/t6043-merge-rename-directories.sh 
b/t/t6043-merge-rename-directories.sh
index f0af66b8a9..b8cd428341 100755
--- a/t/t6043-merge-rename-directories.sh
+++ b/t/t6043-merge-rename-directories.sh
@@ -2940,8 +2940,8 @@ test_expect_success '10b-check: Overwrite untracked with 
dir rename + delete' '
echo contents >y/e &&
 
test_must_fail git merge -s recursive B^0 >out 2>err &&
-   test_i18ngrep "CONFLICT (rename/delete).*Version B^0 of y/d 
left in tree at y/d~B^0" out &&
-   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
writing to y/e~B^0 instead" out &&
+   test_i18ngrep "CONFLICT (rename/delete).*Version B\^0 of y/d 
left in tree at y/d~B\^0" out &&
+   test_i18ngrep "Error: Refusing to lose untracked file at y/e; 
writing to y/e~B\^0 instead" out &&
 
test 3 -eq $(git ls-files -s | wc -l) &&
test 2 -eq $(git ls-files -u | wc -l) &&
@@ -3010,7 +3010,7 @@ test_expect_success '10c-check: Overwrite untracked with 
dir rename/rename(1to2)
 
test_must_fail git merge -s recursive B^0 >out 2>err &&
test_i18ngrep "CONFLICT (rename/rename)" out &&
-   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
as y/c~B^0 instead" out &&
+   test_i18ngrep "Refusing to lose untracked file at y/c; adding 
as y/c~B\^0 instead" out &&
 
test 6 -eq $(git ls-files -s | wc -l) &&
test 3 -eq $(git ls-files -u | wc -l) &&
-- 
2.14.2.808.g3bc32f2729

Re: [PATCH 4/4] branch: add '--show-description' option

[Johannes Sixt]

Am 01.01.2018 um 23:54 schrieb SZEDER Gábor:

'git branch' has an option to edit a branch's description, but lacks
the option to show that description.

Therefore, add a new '--show-description' option to do just that, as a
more user-friendly alternative to 'git config --get
branch.$branchname.description':

   - it's shorter to type (both in the number of characters and the
 number of TABs if using completion),
   - works on the current branch without explicitly naming it,
   - hides the implementation detail that branch descriptions are
 stored in the config file, and
   - errors out with a proper error message when the given branch
 doesn't exist (but exits quietly with an error code when the
 branch does exit but has no description, just like the 'git config'
 query does).



+test_expect_success '--show-description with no description errors quietly' '
+   git config --unset branch.master.description &&
+   test_must_fail git branch --show-description >actual 2>actual.err &&
+   test_must_be_empty actual &&
+   test_must_be_empty actual.err
+'


Checking the exact contents of stderr typically fails when tests are run 
under -x. Perhaps


test_i18ngrep ! "fatal: " actual.err &&"
test_i18ngrep ! "error: " actual.err &&
test_i18ngrep ! "warning: " actual.err

Which makes me wonder: Why would --show-description have to error out 
silently? This is not 'git config' after all.


-- Hannes

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Johannes Sixt]

Am 24.12.2017 um 15:54 schrieb Jeff King:

On Sat, Nov 18, 2017 at 10:01:45AM +0100, Johannes Sixt wrote:


Yeah, I have mixed feelings on that. I think it does make the control
flow less clear. At the same time, what I found was that handlers like
die/ignore/warn were the thing that gave the most reduction in
complexity in the callers.


Would you not consider switching over to C++? With exceptions, you get the
error context without cluttering the API. (Did I mention that
librarification would become a breeze? Do not die in library routines: not a
problem anymore, just catch the exception. die_on_error parameters? Not
needed anymore. Not to mention that resource leaks would be much, MUCH
simpler to treat.)


I threw this email on my todo pile since I was traveling when it came,
but I think it deserves a response (albeit quite late).

It's been a long while since I've done any serious C++, but I did really
like the RAII pattern coupled with exceptions. That said, I think it's
dangerous to do it half-way, and especially to retrofit an existing code
base. It introduces a whole new control-flow pattern that is invisible
to the existing code, so you're going to get leaks and variables in
unexpected states whenever you see an exception.

I also suspect there'd be a fair bit of in converting the existing code
to something that actually compiles as C++.


I think I mentioned that I had a version that passed the test suite. 
It's not pure C++ as it required -fpermissive due to the many implicit 
void*-to-pointer-to-object conversions (which are disallowed in C++). 
And, yes, a fair bit of conversion was required on top of that. ;)



So if we were starting the project from scratch and thinking about using
C++ with RAII and exceptions, sure, that's something I'd entertain[1]
(and maybe even Linus has softened on his opinion of C++ these days ;) ).
But at this point, it doesn't seem like the tradeoff for switching is
there.


Fair enough. I do agree that the tradeoff is not there, in particular, 
when the major players are more fluent in C than in modern C++.


There is just my usual rant: Why do we have look for resource leaks 
during review when we could have leak-free code by design? (But Dscho 
scored a point[*] some time ago: "For every fool-proof system invented, 
somebody invents a better fool.")


[*] 
https://public-inbox.org/git/alpine.DEB.2.20.1704281334060.3480@virtualbox/

Re: [PATCH v2 7/7] wildmatch test: create & test files on disk in addition to in-memory

[Johannes Sixt]

Am 25.12.2017 um 01:28 schrieb Ævar Arnfjörð Bjarmason:

+create_test_file() {
+   file=$1
+
+   case $file in
+   # `touch .` will succeed but obviously not do what we intend
+   # here.
+   ".")
+   return 1
+   ;;
+   # We cannot create a file with an empty filename.
+   "")
+   return 1
+   ;;
+   # The tests that are testing that e.g. foo//bar is matched by
+   # foo/*/bar can't be tested on filesystems since there's no
+   # way we're getting a double slash.
+   *//*)
+   return 1
+   ;;
+   # When testing the difference between foo/bar and foo/bar/ we
+   # can't test the latter.
+   */)
+   return 1
+   ;;
+   esac


Nice!


+
+   # Turn foo/bar/baz into foo/bar to create foo/bar as a
+   # directory structure.
+   dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')


dirs=${file%/*}

should do the same without forking processes, no?

-- Hannes

Re: [PATCH 6/6] wildmatch test: create & test files on disk in addition to in-memory

[Johannes Sixt]

Am 24.12.2017 um 12:06 schrieb Ævar Arnfjörð Bjarmason:

On Sun, Dec 24 2017, Johannes Sixt jotted:

Am 23.12.2017 um 22:30 schrieb Ævar Arnfjörð Bjarmason:

+   printf '%s' '$text' >expect &&


There are no single-quotes in any $text instances, right?


There's not, but maybe we should be more careful here and use here-docs.


Unless it is essential to test the single-quote case, we need not 
complicate the code. I just wanted to rise awareness. If a problematic 
test case is introduced, it will be noticed soon enough. It's not that 
we deal with unknown input here.


-- Hannes

Re: [PATCH 6/6] wildmatch test: create & test files on disk in addition to in-memory

[Johannes Sixt]

Am 23.12.2017 um 22:30 schrieb Ævar Arnfjörð Bjarmason:
> Signed-off-by: Ævar Arnfjörð Bjarmason 
> ---
>   a[]b |   0
>   t/t3070-wildmatch.sh | 336 
> ---
>   2 files changed, 319 insertions(+), 17 deletions(-)
>   create mode 100644 a[]b
> 
> diff --git a/a[]b b/a[]b
> new file mode 100644
> index 00..e69de29bb2

A big no-no! This file can't be created on Windows!

> diff --git a/t/t3070-wildmatch.sh b/t/t3070-wildmatch.sh
> index 47b479e423..d423bb01f3 100755
> --- a/t/t3070-wildmatch.sh
> +++ b/t/t3070-wildmatch.sh
> @@ -4,31 +4,146 @@ test_description='wildmatch tests'
>   
>   . ./test-lib.sh
>   
> +create_test_file() {
> + file=$1
> +
> + # `touch .` will succeed but obviously not do what we intend
> + # here.
> + test "$file" = "." && return 1
> + # We cannot create a file with an empty filename.
> + test "$file" = "" && return 1
> + # The tests that are testing that e.g. foo//bar is matched by
> + # foo/*/bar can't be tested on filesystems since there's no
> + # way we're getting a double slash.
> + echo "$file" | grep -q -F '//' && return 1
> + # When testing the difference between foo/bar and foo/bar/ we
> + # can't test the latter.
> + echo "$file" | grep -q -E '/$' && return 1
> +
> + dirs=$(echo "$file" | sed -r 's!/[^/]+$!!')

Booh! Booh! So many fork()s! ;)

case "$file" in
*//*)
# The tests that are testing that e.g. foo//bar is matched by
# foo/*/bar can't be tested on filesystems since there's no
# way we're getting a double slash.
return 1;;
*/)
# When testing the difference between foo/bar and foo/bar/ we
# can't test the latter.
return 1;;
esac

dirs=${file%/*}

> +
> + # We touch "./$file" instead of "$file" because even an
> + # escaped "touch -- -" means something different.
> + if test "$file" != "$dirs"
> + then
> + mkdir -p -- "$dirs" 2>/dev/null &&
> + touch -- "./$file" 2>/dev/null &&
> + return 0
> + else
> + touch -- "./$file" 2>/dev/null &&
> + return 0
> + fi
> + return 1
> +}
> +
>   wildtest() {
> - match_w_glob=$1
> - match_w_globi=$2
> - match_w_pathmatch=$3
> - match_w_pathmatchi=$4
> - text=$5
> - pattern=$6
> + if test "$#" = 6
> + then
> + # When test-wildmatch and git ls-files produce the same
> + # result.
> + match_w_glob=$1
> + match_f_w_glob=$match_w_glob
> + match_w_globi=$2
> + match_f_w_globi=$match_w_globi
> + match_w_pathmatch=$3
> + match_f_w_pathmatch=$match_w_pathmatch
> + match_w_pathmatchi=$4
> + match_f_w_pathmatchi=$match_w_pathmatchi
> + text=$5
> + pattern=$6
> + elif test "$#" = 10
> + then
> + match_w_glob=$1
> + match_w_globi=$2
> + match_w_pathmatch=$3
> + match_w_pathmatchi=$4
> + match_f_w_glob=$5
> + match_f_w_globi=$6
> + match_f_w_pathmatch=$7
> + match_f_w_pathmatchi=$8
> + text=$9
> + pattern=$10
> + fi
>   
> + # $1: Case sensitive glob match: test-wildmatch
>   if test "$match_w_glob" = 1
>   then
> - test_expect_success "wildmatch: match '$text' '$pattern'" "
> + test_expect_success "wildmatch: match '$text' '$pattern'" "
>   test-wildmatch wildmatch '$text' '$pattern'
>   "
>   elif test "$match_w_glob" = 0
>   then
> - test_expect_success "wildmatch:  no match '$text' '$pattern'" "
> + test_expect_success "wildmatch: no match '$text' '$pattern'" "
>   ! test-wildmatch wildmatch '$text' '$pattern'
>   "
>   else
>   test_expect_success "PANIC: Test framework error. Unknown 
> matches value $match_w_glob" 'false'

I think you can write this as 'say ...; exit 1'. See t*.

>   fi
>   
> + # $1: Case sensitive glob match: ls-files
> + if test "$match_f_w_glob" = 'E'
> + then
> + if create_test_file "$text"
> + then
> + test_expect_success "wildmatch(ls): match dies on 
> '$pattern' '$text'" "
> + test_when_finished \"
> + rm -rf -- * &&

Can we be a bit more careful with this rm -rf, please?
There is only one similarly loose case in t/t7003-filter-branch.sh,
and it is outside test_when_finished, i.e., it is well under control;
this instance here inside test_when_finished is not.

> + git reset
> + \" &&
> + 

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 18.12.2017 um 11:13 schrieb Torsten Bögershausen:

Just to confirm my missing knowledge here:
Does this mean, that git-gui and gitk can decode/reencode
the content of a file/blob, when the .gitattributes say so ?


No. I think they parse the output of git-diff et.al., split it per file, 
and then consult .gitattributes to interpret the byte sequence according 
to the encoding.


-- Hannes

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 12.12.2017 um 01:59 schrieb Junio C Hamano:

Stepping back a bit, what does this thing do you are introducing?
And what does the other thing do that J6t is using, that would get
confused with this new one?

What does the other one do?  "Declare that the contents of this path
is in this encoding"?  As opposed to the new one, which tells Git to
"run iconv from and to this encoding when checking out and checking
in"?

If so, any phrase that depends heavily on the word "encode" would
not help differenciating the two uses.  The phrase needs to be
something that contrasts the new one, which actively modifies things
(what is on the filesystem is not what is stored in the object
store), with the old one, which does not (passed as a declaration to
a viewer what encoding the contents already use and does not change
anything).


Well explained!


...  perhaps "smudge-encoding" would work (we declare that the
result of smudge operations are left in this encoding, so the
opposite operation "clean" will do the reverse---and we say this
without explicitly saying that the other end of the conversion is
always UTF-8)?  Or "checkout-encoding" (the same explanation; we do
not say the opposite operation "checkin/add" will do the reverse).


I would favor "checkout-encoding" over "smudge-encoding" only because 
"checkout" is better known than "smudge", I would think. I do not have 
better suggestions.


-- Hannes

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 12.12.2017 um 00:42 schrieb Lars Schneider:

BTW: I am curios, can you share what encoding you use?
My main use case is UTF-16 and I was surprised that I haven't
found a single public repo on github.com with "encoding=utf-16"


Shift-JIS and CP1252. These are used for Windows resource files (*.rc).

-- Hannes

Re: [PATCH v1] convert: add support for 'encoding' attribute

[Johannes Sixt]

Am 11.12.2017 um 16:50 schrieb lars.schnei...@autodesk.com:

From: Lars Schneider 

Git and its tools (e.g. git diff) expect all text files in UTF-8
encoding. Git will happily accept content in all other encodings, too,
but it might not be able to process the text (e.g. viewing diffs or
changing line endings).

Add an attribute to tell Git what encoding the user has defined for a
given file. If the content is added to the index, then Git converts the
content to a canonical UTF-8 representation. On checkout Git will
reverse the conversion.

Reviewed-by: Patrick Lühne 
Signed-off-by: Lars Schneider 
---

Hi,

here is a WIP patch to add text encoding support for files encoded with
something other than UTF-8 [RFC].

The 'encoding' attribute is already used to view blobs in gitk. That
could be a problem as the content is stored in Git with the defined
encoding. This patch would interpret the content as UTF-8 encoded and


This will be a major drawback for me because my code base stores text 
files that are not UTF-8 encoded. And I do use the existing 'encoding' 
attribute to view the text in git-gui and gitk. Repurposing this 
attribute name is not an option, IMO.


it would try to reencode it to the defined encoding on checkout > Plus, many repos define the attribute very broad (e.g. "* 

encoding=cp1251").

These folks would see errors like these with my patch:
 error: failed to encode 'foo.bar' from utf-8 to cp1251


-- Hannes

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Johannes Sixt]

I am sorry for not responding in detail. I think we've reached a mutual 
understanding of our workflows.


Though, from the ideas you tossed around most recently, you seem to want 
to make git-commit into a kitchen-sink for everything. I have my doubts 
that this will be a welcome change. Just because new commits are created 
does not mean that the feature must live in git-commit.


-- Hannes

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Johannes Sixt]

Am 05.12.2017 um 22:35 schrieb Junio C Hamano:
> Dan Jacques  writes:
> 
>> Thanks for checking! The patch that you quoted above looks like it's from
>> this "v4" thread; however, the patch that you are diffing against in your
>> latest reply seems like it is from an earlier version.
>>
>> I believe that the $(pathsep) changes in your proposed patch are already
>> present in v4,...
> 
> You're of course right.  The patches I had in my tree are outdated.
> 
> Will replace, even though I won't be merging them to 'pu' while we
> wait for Ævar's perl build procedure update to stabilize.

The updated series works for me now. Nevertheless, I suggest to squash
in the following change to protect against IFS and globbing characters in
$INSTLIBDIR.

diff --git a/Makefile b/Makefile
index 7ac4458f11..08c78a1a63 100644
--- a/Makefile
+++ b/Makefile
@@ -2072,7 +2072,7 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES 
perl/perl.mak Makefile
INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
-   -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+   -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
-e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
-e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
$< >$@+ && \

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Johannes Sixt]

Am 01.12.2017 um 18:25 schrieb Johannes Sixt:
> Am 01.12.2017 um 18:13 schrieb Johannes Schindelin:
>> Hi Hannes,
>>
>> On Fri, 1 Dec 2017, Johannes Sixt wrote:
>>
>>> Am 29.11.2017 um 16:56 schrieb Dan Jacques:
>>>> @@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
>>>>     echo "$$FLAGS" >$@; \
>>>>     fi
>>>>    +GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES 
>>>> perl/perl.mak
>>>> Makefile
>>>> +    $(QUIET_GEN)$(RM) $@ && \
>>>> +    INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory
>>>> instlibdir` && \
>>>> +    INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
>>>> +    
>>>> INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" &&
>>>> \
>>>> +    sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
>>>
>>> This doesn't work, unfortunately. When $(pathsep) is ';', we get an 
>>> incomplete
>>> sed expression because ';' is also a command separator in the sed 
>>> language.
>>
>> Funny, I tried this also with ';' as pathsep, and it worked in the Git 
>> for
>> Windows SDK...
> 
> My ancient sed vs. your modern sed, perhaps? I can check this on Monday.

I don't know what I tested last week; most likely not the version of the
patch I quoted above.

Today's version, with the tip at 5d7f59c391ce, is definitely bogus
with its quoting. It needs the patch below, otherwise an unquoted
semicolon may be expanded from $(pathsep). This would terminate the sed
command, of course.


diff --git a/Makefile b/Makefile
index 484dc44ade..a658c8169a 100644
--- a/Makefile
+++ b/Makefile
@@ -2071,10 +2071,10 @@ GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) 
GIT-PERL-DEFINES perl/perl.mak
INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory 
instlibdir` && \
INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
-   sed -e 's=@@PATHSEP@@='$(pathsep)'=g' \
-   -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
-   -e 's=@@GITEXECDIR@@='$(gitexecdir_relative_SQ)'=g' \
-   -e 's=@@PERLLIBDIR@@='$(perllibdir_relative_SQ)'=g' \
+   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \
+   -e 's=@@INSTLIBDIR@@='"$$INSTLIBDIR"'=g' \
+   -e 's=@@GITEXECDIR@@=$(gitexecdir_relative_SQ)=g' \
+   -e 's=@@PERLLIBDIR@@=$(perllibdir_relative_SQ)=g' \
$< >$@
 
 .PHONY: gitweb
-- 
2.14.2.808.g3bc32f2729

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Johannes Sixt]

Am 01.12.2017 um 18:13 schrieb Johannes Schindelin:

Hi Hannes,

On Fri, 1 Dec 2017, Johannes Sixt wrote:


Am 29.11.2017 um 16:56 schrieb Dan Jacques:

@@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
echo "$$FLAGS" >$@; \
fi
   +GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak
Makefile
+   $(QUIET_GEN)$(RM) $@ && \
+   INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory
instlibdir` && \
+   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" &&
\
+   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \


This doesn't work, unfortunately. When $(pathsep) is ';', we get an incomplete
sed expression because ';' is also a command separator in the sed language.


Funny, I tried this also with ';' as pathsep, and it worked in the Git for
Windows SDK...


My ancient sed vs. your modern sed, perhaps? I can check this on Monday.

-- Hannes

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Johannes Sixt]

Am 30.11.2017 um 00:10 schrieb Igor Djordjevic:

On 29/11/2017 20:11, Johannes Sixt wrote:

With git-post, I make a fixup commit commit on the integration
branch, then `git post B && git merge B`:

  ...A...C  <- topics A, C
  \   \
---o---o---o---o---f---F<- integration
  /   /   /
  ...B...D   /  <- topic D
  \ /
   f'--'<- topic B

The merge F does not introduce any changes on the integration branch,
so I do not need it, but it helps keep topic B off radar when I ask
`git branch --no-merged` later.


But you`re not committing (posting) on your HEAD`s (direct) parent in
the first place (topic B), so `commit --onto-parent` isn`t right tool
for the job... yet :)


Ohh..kay.


To work with `--onto-parent` and be able to commit on top of any of
the topic branches, you would need a situation like this instead:

  (1)  ...C  <- topic C
  |
 ...A |  <- topic A
 \|
   ...o I<- integration
 /|
 ...B |  <- topic B
  |
   ...D  <- topic D


This is a very, VERY exotic workflow, I would say. How would you 
construct commit I when three or more topics have conflicts? 
merge-octopus does not support this use-case.



With `commit --onto-parent` you would skip `git post B && git merge
B` steps, where "fixup commit" would be done with `--onto-parent B`,
So you end up in situation like this:

  (2)  ...C  <- topic C
  |
 ...A |  <- topic A
 \|
   ...o I'   <- integration
 /|
 ...B---f |  <- topic B
  |
   ...D  <- topic D

State of index and working tree files in your F and my I' commit is
exactly the same (I' = I + f), where in my case (2) history looks
like "f" was part of topic B from the start, before integration
merge happened.

BUT, all this said, I understand that your starting position, where
not all topic branches are merged at the same time (possibly to keep
conflict resolution sane), is probably more often to be encountered
in real-life work... and as existing `--onto-parent` machinery should
be able to support it already, I`m looking forward to make it happen :)

Once there, starting from your initial position:


...A...C<- topics A, C
\   \ E
  ---o---o---o---o I<- integration <- HEAD
/   /
...B...D<- topics B, D


... and doing something like `git commit --onto B --merge` would yield:
  
  (3) ...A...C<- topics A, C

  \   \ E
---o---o---o---o I'   <- integration
  /   /|
  ...B...D |  <- topic D
  \|
   f---'  <- topic B

... where (I' = I + f) is still true.


I am not used to this picture. I would not think that it is totally 
unacceptable, but it still has a hmm-factor.



If that`s preferred in some
cases, it could even look like this instead:

  (4) ...A...C <- topics A, C
  \   \ E  I
---o---o---o---o---F   <- integration
  /   /   /
  ...B...D   / <- topic D
  \ /
   f---'   <- topic B

... where F(4) = I'(3), so similar situation, just that we don`t
discard I but post F on top of it.


This is very acceptable.

Nevertheless, IMO, it is not the task of git-commit to re-compute a 
merge commit. It would be OK that it commits changes on top of a branch 
that is not checked out. Perhaps it would even be OK to remove the 
change from the current workspace (index and worktree), because it will 
return in the form of a merge later, but producing that merge is 
certainly not the task of git-commit.


-- Hannes

Re: [PATCH v4 1/4] Makefile: generate Perl header from template file

[Johannes Sixt]

Am 29.11.2017 um 16:56 schrieb Dan Jacques:

@@ -1989,6 +1986,15 @@ GIT-PERL-DEFINES: FORCE
echo "$$FLAGS" >$@; \
fi
  
+GIT-PERL-HEADER: $(PERL_HEADER_TEMPLATE) GIT-PERL-DEFINES perl/perl.mak Makefile

+   $(QUIET_GEN)$(RM) $@ && \
+   INSTLIBDIR=`MAKEFLAGS= $(MAKE) -C perl -s --no-print-directory instlibdir` 
&& \
+   INSTLIBDIR_EXTRA='$(PERLLIB_EXTRA_SQ)' && \
+   INSTLIBDIR="$$INSTLIBDIR$${INSTLIBDIR_EXTRA:+:$$INSTLIBDIR_EXTRA}" && \
+   sed -e 's=@@PATHSEP@@=$(pathsep)=g' \


This doesn't work, unfortunately. When $(pathsep) is ';', we get an 
incomplete sed expression because ';' is also a command separator in the 
sed language.



+   -e 's=@@INSTLIBDIR@@='$$INSTLIBDIR'=g' \
+   $< >$@+ && \
+   mv $@+ $@
  
  .PHONY: gitweb

  gitweb:


-- Hannes

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Johannes Sixt]

Am 28.11.2017 um 02:15 schrieb Igor Djordjevic:

On 27/11/2017 22:54, Johannes Sixt wrote:


I my opinion, putting the focus on integration merge commits and the
desire to automate the re-merge step brings in a LOT of complexity in
the implementation for a very specific use-case that does not
necessarily help other cases.


It might seem more complex than it is, until you examine the guts to
see how it really works :)

Basic concept is pretty simple, as I actually don`t automate
anything, at least not in terms of what would manual user steps look
like - for example, there`s no real re-merge step in terms of
actually redoing the merge, I just reuse what was already there in a
very clean way, I would think (supported by my current, humble
knowledge, still).

The only merge that could possibly ever happen is upon committing
desired subset of changes onto parent, and that shouldn`t be too
complex by definition, otherwise that commit doesn`t really belong
there in the first place, if it can`t be meaningfully applied where
we want it (for now, at least).

That said, the whole operation of "posting on parent and re-merging
everything", the way it looks like from the outside, could end just
with a simple diff-apply-commit-commit internally, no merges at all.
Only if simple `git apply` fails, we try some trivial merging - and
all that inside separate (parent) index only, not touching original
HEAD index nor working tree, staying pristine for the whole process,
untouched.

Once done, you should be in the very same situation you started from,
nothing changed, just having your history tweaked a bit to tell a
different story on how you got there (now including a commit you
posted on your HEAD`s parent).


Ok, then please explain, how this process should work in my workflow and 
with the `commit --onto-parent` feature that you have in mind. I have an 
integration branch (which is a throw-away type, so you can mangle it in 
any way you want); it is a series of merges:


 ...A...C  <- topics A, C
 \   \
   ---o---o---o---o<- integration
 /   /
 ...B...D  <- topics B, D

Now I find a bug in topic B. Assume that the merges of C and D have 
textual conflicts with the integration branch (but not with B) and/or 
may be evil. What should I do?


With git-post, I make a fixup commit commit on the integration branch, 
then `git post B && git merge B`:


 ...A...C  <- topics A, C
 \   \
   ---o---o---o---o---f---F<- integration
 /   /   /
 ...B...D   /  <- topic D
 \ /
  f'--'<- topic B

The merge F does not introduce any changes on the integration branch, so 
I do not need it, but it helps keep topic B off radar when I ask `git 
branch --no-merged` later.



Don`t let "usual/preferred/recommended" Git workflow distract you too
much - one of the reasons I made this is because it also allows _kind
of_ "vanilla Git" patch queue, where you can quickly work on top of
the merge head, pushing commits onto parents below, being tips of
your "queues", putting you up to speed without a need to ever switch
a branch (hypothetically), until satisfied with what you have, where
you can slow down and polish each branch separately, as usual.

Like working on multiple branches at the same time, in the manner
similar to what `git add --patch` allows in regards to working on
multiple commits at the same time. This just takes it on yet another
level... hopefully :)


'kay, I'm not eagerly waiting for this particular next level (I prefer 
to keep things plain and simple), but I would never say this were a 
broken workflow. ;)



In your scenario above, it would certainly not be too bad if you
forgo the automatic merge and have the user issue a merge command
manually. The resulting history could look like this:

(3) o---o---A---X(topicA)
/ \   \
   /   M1--M2 (test, HEAD)
  /   /||
  ---o---o---M---' || (master)
  \   \   / |
   \   o-B /  (topicB)
\ /
 o---o---C(topicC)

I.e., commit --onto-parent A produced commit X, but M2 was then a
regular manual merge. (Of course, I am assuming that the merge
commits are dispensible, and only the resulting tree is of
interest.)


I see - and what you`re asking for is what I already envisioned and
hoped to get some more feedback about, here`s excerpt from
[SCRIPT/RFC 3/3] git-commit--onto-parent.sh[1] (I guess you didn`t
have time to checked that one yet?):


I did have a brief look, but I stopped when I saw

# Remove entry from HEAD reflog, not to pollute it with
# uninteresting in-between steps we take, leaking implementation
# details to end user.

It's a clear sign for me that's something wrong. It is not just reflogs 
that c

Re: [SCRIPT/RFC 0/3] git-commit --onto-parent (three-way merge, no working tree file changes)

[Johannes Sixt]

Am 26.11.2017 um 23:35 schrieb Igor Djordjevic:

Approach discussed here could have a few more useful applications,
but one seems to be standing out the most - in case where multiple
topic branches are temporarily merged for integration testing, it
could be very useful to be able to post "hotfix" commits to merged
branches directly, _without actually switching to them_ (and thus
without touching working tree files), and still keeping everything
merged, in one go.

Example starting point is "master" branch with 3 topic branches (A,
B, C), to be (throwaway) merged for integration testing inside
temporary "test" branch:

(1)o---o---A (topicA)
   /
  /
 /
 ---o---o---M (master, test, HEAD)
 \   \
  \   o---B (topicB)
   \
o---o---C (topicC)


This is what we end up with once "master" and topic branches are
merged in merge commit M1 inside temporary "test" branch for further
integration testing:

(2)o---o---A (topicA)
   / \
  /   M1 (test, HEAD)
 /   /||
 ---o---o---M---/ || (master)
 \   \   / |
  \   o---B-/  | (topicB)
   \   |
o---o---C--/ (topicC)


Upon discovery of a fix needed inside "topicA", hotfix changes X
should be committed to "topicA" branch and re-merged inside merge
commit M2 on temporary integration "test" branch (previous temporary
merge commit M1 is thrown away as uninteresting):

(3)o---o---A---X (topicA)
   / \
  /   M2 (test, HEAD)
 /   /||
 ---o---o---M---/ || (master)
 \   \   / |
  \   o---B-/  | (topicB)
   \  /
o---o---C/ (topicC)


I my opinion, putting the focus on integration merge commits and the 
desire to automate the re-merge step brings in a LOT of complexity in 
the implementation for a very specific use-case that does not 
necessarily help other cases.


For example, in my daily work, I have encountered situations where, 
while working on one topic, I made a hot-fix for a different topic. 
There is no demand for a merge step in this scenario.


In your scenario above, it would certainly not be too bad if you forgo 
the automatic merge and have the user issue a merge command manually. 
The resulting history could look like this:


(3) o---o---A---X(topicA)
   / \   \
  /   M1--M2 (test, HEAD)
 /   /||
 ---o---o---M---' || (master)
 \   \   / |
  \   o-B /  (topicB)
   \ /
o---o---C(topicC)

I.e., commit --onto-parent A produced commit X, but M2 was then a 
regular manual merge. (Of course, I am assuming that the merge commits 
are dispensible, and only the resulting tree is of interest.)


Moreover, you seem to assume that an integration branch is an octopus 
merge, that can be re-created easily. I would say that this a very, very 
exceptional situation.




At this point, I spent five minutes thinking of how I would use commit 
--onto-parent if I did not have git-post.


While on the integration branch, I typically make separate commits for 
each fix, mostly because the bugs are discovered and fixed not 
simultaneously, but over time. So, I have a small number of commits that 
I distribute later using my git-post script. But that does not have to 
be so. I think I could work with a git commit --onto-parent feature as 
long as it does not attempt to make a merge commit for me. (I would hate 
that.)


Sometimes, however I have two bug fixes in the worktree, ready to be 
committed. Then the ability to pass pathspec to git commit is useful. 
Does your implementation support this use case (partially staged 
worktree changes)?


Thanks,
-- Hannes

Re: [PATCH 2/2] Reduce performance cost of the trace if trace category is disabled

[Johannes Sixt]

Am 19.11.2017 um 01:42 schrieb gennady.kup...@gmail.com:

+#define trace_printf_key(key, ...) \
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_printf_key_fl(TRACE_CONTEXT, __LINE__, key,   \
+   __VA_ARGS__);   \
+   } while(0)
+
+#define trace_printf(...) trace_printf_key(_default_key, __VA_ARGS__);
+
+#define trace_argv_printf(argv, ...)   \
+   do {\
+   if (trace_pass_fl(_default_key))  
\
+  trace_argv_printf_fl(TRACE_CONTEXT, __LINE__,\
+   argv, __VA_ARGS__); \
+   } while(0)
+
+#define trace_strbuf(key, data)
\
+   do {\
+   if (trace_pass_fl(key)) \
+   trace_strbuf_fl(TRACE_CONTEXT, __LINE__, key, data);\
+   } while(0)
+
+#define trace_performance(nanos, ...)  \
+   do {\
+   if (trace_pass_fl(key)) \


The token "key" here looks suspicious. Did you mean _perf_key?


+   trace_performance_fl(TRACE_CONTEXT, __LINE__, nanos,\
+__VA_ARGS__);  \
+   } while(0)
+
+#define trace_performance_since(start, ...)\
+   do {\
+   if (trace_pass_fl(_perf_key)) \
+   trace_performance_fl(TRACE_CONTEXT, __LINE__,   \
+getnanotime() - (start),   \
+__VA_ARGS__);  \
+   } while(0)
  
  /* backend functions, use non-*fl macros instead */

  __attribute__((format (printf, 4, 5)))


-- Hannes

Re: Improved error handling (Was: [PATCH 1/2] sequencer: factor out rewrite_file())

[Johannes Sixt]

Am 17.11.2017 um 23:33 schrieb Jeff King:

On Mon, Nov 06, 2017 at 05:13:15PM +0100, Simon Ruderich wrote:

On Sat, Nov 04, 2017 at 10:07:00PM -0400, Jeff King wrote:

Yes, I think what you've written here (and below) is quite close to the
error_context patches I linked elsewhere in the thread. In other
words, I think it's a sane approach.


In contrast to error_context I'd like to keep all exiting
behavior (die, ignore, etc.) in the hand of the caller and not
use any callbacks as that makes the control flow much harder to
follow.


Yeah, I have mixed feelings on that. I think it does make the control
flow less clear. At the same time, what I found was that handlers like
die/ignore/warn were the thing that gave the most reduction in
complexity in the callers.


Would you not consider switching over to C++? With exceptions, you get 
the error context without cluttering the API. (Did I mention that 
librarification would become a breeze? Do not die in library routines: 
not a problem anymore, just catch the exception. die_on_error 
parameters? Not needed anymore. Not to mention that resource leaks would 
be much, MUCH simpler to treat.)


-- Hannes

Re: Consequences of CRLF in index?

[Johannes Sixt]

Thank you for the clarification, it's much appreciated.

-- Hannes

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 26.10.2017 um 13:01 schrieb Lars Schneider:

On 26 Oct 2017, at 09:09, Johannes Sixt <j...@kdbg.org> wrote:
Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:

I envy you for the blessing of such a clean C++ source that you do not
have any, say, Unix shell script in it. Try this, and weep:
$ printf 'echo \\\r\n\t123\r\n' >a1
$ sh a1
a1: 2: a1: 123: not found


I was bitten by that, too. For this reason, I ensure that shell scripts and 
Makefiles begin their life on Linux. Fortunately, modern editors on Windows, 
includ^Wand vi, do not force CRLF line breaks, and such files can be edited on 
Windows, too.


Wouldn't this kind of .gitattributes setup solve the problem?


But there is no problem. Don't have CRs in shell scripts and be done 
with it.




* -text
*.sh   text eol=lf


Why would that be necessary? I cannot have CRLF in shell scripts etc., 
not even on Windows. (And in addition I do not mind CR in C++ source 
code.) All I want is: Git, please, by all means, do not mess with my 
files, ever. Isn't the simplest way to achieve that to set *nothing* at 
all? Not even core.autocrlf?


-- Hannes

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 25.10.2017 um 14:19 schrieb Johannes Schindelin:

I envy you for the blessing of such a clean C++ source that you do not
have any, say, Unix shell script in it. Try this, and weep:

$ printf 'echo \\\r\n\t123\r\n' >a1

$ sh a1

a1: 2: a1: 123: not found


I was bitten by that, too. For this reason, I ensure that shell scripts 
and Makefiles begin their life on Linux. Fortunately, modern editors on 
Windows, includ^Wand vi, do not force CRLF line breaks, and such files 
can be edited on Windows, too.


Of course, I do not set core.autocrlf anywhere to avoid any changes 
behind my back.



For the same reason (Unix shell not handling CR/LF gracefull), I went
through that painful work that finally landed as 00ddc9d13ca (Fix build
with core.autocrlf=true, 2017-05-09).


That's much appreciated!

-- Hannes

Re: Consequences of CRLF in index?

[Johannes Sixt]

Am 24.10.2017 um 19:48 schrieb Lars Schneider:

I've migrated a large repo (110k+ files) with a lot of history (177k commits)
and a lot of users (200+) to Git. Unfortunately, all text files in the index
of the repo have CRLF line endings. In general this seems not to be a problem
as the project is developed exclusively on Windows.

However, I wonder if there are any "hidden consequences" of this setup?


I've been working on a project with CRLF in every source file for a 
decade now. It's C++ source, and it isn't even Windows-only: when 
checked out on Linux, there are CRs in the files, with no bad 
consequences so far. GCC is happy with them.


-- Hannes

Re: [PATCH v2 1/1] completion: add remaining flags to checkout

[Johannes Sixt]

Am 24.10.2017 um 15:19 schrieb Thomas Braun:

In the commits 1fc458d9 (builtin/checkout: add --recurse-submodules
switch, 2017-03-14), 08d595dc (checkout: add --ignore-skip-worktree-bits
in sparse checkout mode, 2013-04-13) and 32669671 (checkout: introduce
--detach synonym for "git checkout foo^{commit}", 2011-02-08) checkout
gained new flags but the completion was not updated, although these flags
are useful completions. Add them.

The flags --force and --ignore-other-worktrees are not added as they are
potentially dangerous.

The flags --progress and --no-progress are only useful for scripting and are
therefore also not included.

Signed-off-by: Thomas Braun 
---
  contrib/completion/git-completion.bash | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d934417475..eb6ade6974 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1250,7 +1250,8 @@ _git_checkout ()
--*)
__gitcomp "
--quiet --ours --theirs --track --no-track --merge
-   --conflict= --orphan --patch
+   --conflict= --orphan --patch --detach 
--ignore-skip-worktree-bits
+   --recurse-submodules --no-recurse-submodules
"
;;
*)



Looks good to me. Thanks,
-- Hannes

Re: [PATCH/RFC] git-post: the opposite of git-cherry-pick

[Johannes Sixt]

Am 17.10.2017 um 01:01 schrieb Rafael Ascensao:

This is worth discussing, though not my preference. The picture to "pick
cherries" has become quite common, and now that we use it for the name of
the command, "cherry-pick", the direction of flow is quite obvious and
strongly implied: from somewhere else to me (and not to somebody else).


What if we borrow '--onto' from rebase and make it cherry-pick --onto
?


I actually like this. Although I would miss the convenience that the 
source defaults to HEAD. Unless we make a special case for --onto, that 
is, of course.


-- Hannes

Re: Does Git build things during 'make install"?

[Johannes Sixt]

Am 16.10.2017 um 10:23 schrieb Junio C Hamano:
> Johannes Sixt <j...@kdbg.org> writes:
> 
>> Yes, running "sudo make install" is a nightmare. sudo clears the path,
>> and the git command is not found by the make invoked with root
>> permissions. This changes the version string that gets compiled into
>> the executable, which finally triggers a complete rebuild under
>> root. Sad...
> 
> In the meantime, would it help to intall as yourself under DESTDIR
> set to where you can write into, and then limit the potential
> damange done while pretending to be a privileged user to "cp -R" (or
> "tar cf" in $DESTDIR piped to "tar xf" in /)?
> 
> It appears that some dependencies are screwed up around "perl"
> related things, which may want to get fixed.  I agree that "make &&
> make install" that runs two 'make' under the same environment and
> user shouldn't (re)build anything during the latter 'make', but we
> somehow seem to do so.

We do so only, if 'make install' does not run in the same environment
and if there is no version file.

I use the patch below. It works for me, but I could imagine that it
suffers from the original problem if there is no git in PATH and there
is no version file, i.e., when the source is not a release tarball.

- 8< -
Subject: [PATCH] version-gen: Use just built git if no other git is in PATH

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 GIT-VERSION-GEN | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 0e88e23653..b610aa3249 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -3,6 +3,9 @@
 GVF=GIT-VERSION-FILE
 DEF_VER=v2.15.0-rc1
 
+# use git that was just compiled if there is no git elsewhere in PATH
+PATH=$PATH:.
+
 LF='
 '
 
-- 
2.14.2.808.g3bc32f2729

Re: Does Git build things during 'make install"?

[Johannes Sixt]

Am 16.10.2017 um 07:05 schrieb Jeffrey Walton:

My script to build Git dies during cleanup. Cleanup removes the
downloaded tarball and the unpacked directory:

** Cleanup **

rm: cannot remove 'git-2.14.2/perl/blib/lib/.exists': Permission denied
rm: cannot remove 'git-2.14.2/perl/blib/lib/Git/SVN/Fetcher.pm':
Permission denied
rm: cannot remove 'git-2.14.2/perl/blib/lib/Git/SVN/Utils.pm': Permission denied
rm: cannot remove 'git-2.14.2/perl/blib/lib/Git/SVN/Ra.pm': Permission denied
rm: cannot remove 'git-2.14.2/perl/blib/lib/Git/SVN/GlobSpec.pm':
Permission denied
...

When I look at the permissions:

$ ls -Al git-2.14.2/perl/blib/lib/.exists
-rw-r--r--   1 root root   0 Oct 16 00:43
git-2.14.2/perl/blib/lib/.exists

The only place in my script that does anything with privileges is
'make install' because it runs with sudo.

Is Git building things in the install recipe? If so, then I don't
believe that's supposed to happen. According to the GNU coding
standards, Git should not be doing that. Cf;
https://www.gnu.org/prep/standards/html_node/Standard-Targets.html.


Yes, running "sudo make install" is a nightmare. sudo clears the path, 
and the git command is not found by the make invoked with root 
permissions. This changes the version string that gets compiled into the 
executable, which finally triggers a complete rebuild under root. Sad...


-- Hannes

Re: [PATCH/RFC] git-post: the opposite of git-cherry-pick

[Johannes Sixt]

Am 13.10.2017 um 12:51 schrieb Ævar Arnfjörð Bjarmason:

On Thu, Oct 05 2017, Johannes Sixt jotted:

Am 05.10.2017 um 21:33 schrieb Stefan Beller:

* Is it a good design choice to have a different command, just because the
target branch is [not] checked out?


I would not want to make it a sub-mode of cherry-pick, if that is what
you mean, because "cherry picking" is about getting something, not
giving something away.


It occurs to me that a better long-term UI design might be to make
git-{cherry-pick,pick) some sort of submodes for git-commit.


I don't quite agree. To commit an index state that is derived from a 
worktree state is such a common and important operation that it deserves 
to be its own command. Adding different modi operandi would make it 
confusing.



Right now git-commit picks the current index for committing, but "use a
patch as the source instead" could be a submode.

Right now it commits that change on top of your checked out commit, but
"other non-checked-out target commit" could be a mode instead,
i.e. exposing more of the power of the underlying git-commit-tree.


This is worth discussing, though not my preference. The picture to "pick 
cherries" has become quite common, and now that we use it for the name 
of the command, "cherry-pick", the direction of flow is quite obvious 
and strongly implied: from somewhere else to me (and not to somebody else).



[Not entirely serious]. Well if cherry-picking is taking a thing and
eating it here, maybe git-cherry-puke takes an already digested thing
and "throws" it elsewhere ?:)

It's a silly name but it's somewhat symmetric :)


One of the decisions to be made is whether to begin the new command with 
"git-cherry-" or not, because it introduces a new abiguity for command 
line completion.


-- Hannes

Re: [PATCH v1 1/1] completion: add remaining flags to checkout

[Johannes Sixt]

Am 12.10.2017 um 18:50 schrieb Johannes Sixt:

Am 12.10.2017 um 14:20 schrieb Thomas Braun:

+    --force --ignore-skip-worktree-bits --ignore-other-worktrees


Destructive and dangerous options are typically not offered by command 
completion. You should omit all three in the line above, IMO.


Ah, no, only --force and --ignore-other-worktrees are dangerous, 
--ignore-skip-worktree-bits is not.


-- Hannes

Re: [PATCH v1 1/1] completion: add remaining flags to checkout

[Johannes Sixt]

Am 12.10.2017 um 14:20 schrieb Thomas Braun:

In the commits 1d0fa898 (checkout: add --ignore-other-wortrees,
2015-01-03), 1fc458d9 (builtin/checkout: add --recurse-submodules switch,
2017-03-14), 870ebdb9 (checkout: add --progress option, 2015-11-01),
08d595dc (checkout: add --ignore-skip-worktree-bits in sparse checkout
mode, 2013-04-13), 1d0fa898 (checkout: add --ignore-other-wortrees,
2015-01-03), 32669671 (checkout: introduce --detach synonym for "git
checkout foo^{commit}", 2011-02-08) and db941099 (checkout -f: allow
ignoring unmerged paths when checking out of the index, 2008-08-30)
checkout gained new flags but the completion was not updated, although
these flags are useful completions. Add them.

Signed-off-by: Thomas Braun 
---
  contrib/completion/git-completion.bash | 4 +++-
  1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index d934417475..393d4ae230 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1250,7 +1250,9 @@ _git_checkout ()
--*)
__gitcomp "
--quiet --ours --theirs --track --no-track --merge
-   --conflict= --orphan --patch
+   --conflict= --orphan --patch --detach --progress 
--no-progress
+   --force --ignore-skip-worktree-bits 
--ignore-other-worktrees


Destructive and dangerous options are typically not offered by command 
completion. You should omit all three in the line above, IMO.


Furthermore, --progress and --no-progress are not useful in daily work 
on the command line, I think. By offering them, --p would not 
complete to --patch anymore, you would need --pa. You should omit 
them, too.



+   --recurse-submodules --no-recurse-submodules
"
;;
*)



-- Hannes

Re: [PATCH/RFC] git-post: the opposite of git-cherry-pick

[Johannes Sixt]

Am 05.10.2017 um 21:33 schrieb Stefan Beller:

On Thu, Oct 5, 2017 at 12:13 PM, Johannes Sixt <j...@kdbg.org> wrote:

+git-post(1)
+===
+
+NAME
+
+git-post - Apply a commit on top of a branch that is not checked out


Contrasted to:
   git-cherry-pick - Apply the changes introduced by some existing commits
Some questions on top of my head:
* Do we want to emphasize the cherry-pick to say checked out?


Maybe. Maybe "cherry picking" is sufficiently clear that it is not needed.


* Is it a good design choice to have a different command, just because the
   target branch is [not] checked out?


I would not want to make it a sub-mode of cherry-pick, if that is what 
you mean, because "cherry picking" is about getting something, not 
giving something away.



* Naming: I just searched for synonyms "opposite of cherry-picking" and
   came up with cherry-put, cherry-post, target-commit


With command line completion, we have to type 'git cherr-' 
currently. If we add another command that begins with 'cherry-', another 
 is needed. Therefore, I do not want to use a name starting with 
'cherry-'.



* What was the rationale for naming it "post" (it sounds very generic to me)?


Yes, it is generic, but I did not find a better word that means "give 
away" a commit. Perhaps "tack"?



* Does it play well with worktrees? (i.e. if a worktree has a branch checked
   out, we cannot post there, or can we?)


Good point. It should be forbidden, but there are no safety checks, 
currently.



+EXAMPLES
+
+
+Assume, while working on a topic, you find and fix an unrelated bug.
+Now:
+
+
+$ git commit   <1>
+$ git post master  <2>
+$ git show | git apply -R && git reset HEAD^   <3>
+
+
+<1> create a commit with the fix on the current branch
+<2> copy the fix onto the branch where it ought to be
+<3> revert current topic branch to the unfixed state;
+can also be done with `git reset --keep HEAD^` if there are no
+unstaged changes in files that are modified by the fix
+
+Oftentimes, switching branches triggers a rebuild of a code base.
+With the sequence above the branch switch can be avoided.
+That said, it is good practice to test the bug fix on the
+destination branch eventually.


This is a cool and motivating example.


Thanks. Another use case is when you receive a patch to be applied on a 
different branch while you are in the middle of some work. If it can be 
applied on the current branch, then you can post it to the destination, 
rewind, and continue with your work.



+BUGS
+
+
+The change can be applied on `dest-branch` only if there is
+no textual conflict.


This is not a bug per se, it could be signaled via a return code that
the posting was unsuccessful.


Oh, it does so return an error. But there are some cases that one 
expects that work, but where git-apply is not capable enough.


-- Hannes

[PATCH/RFC] git-post: the opposite of git-cherry-pick

[Johannes Sixt]

Add a new command that can be used to copy an arbitrary commit
to a branch that is not checked out.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 I have been using this command since years, but got around to
 write a man page and tests only now. I hope the man page makes
 sense. I don't have the tool chain to build it, though, so
 any hints for improvement are welcome.

 .gitignore|  1 +
 Documentation/git-cherry-pick.txt |  3 +-
 Documentation/git-post.txt| 57 +
 Makefile  |  1 +
 command-list.txt  |  1 +
 git-post.sh   | 60 ++
 t/t3514-post.sh   | 77 +++
 7 files changed, 199 insertions(+), 1 deletion(-)
 create mode 100644 Documentation/git-post.txt
 create mode 100755 git-post.sh
 create mode 100755 t/t3514-post.sh

diff --git a/.gitignore b/.gitignore
index 833ef3b0b7..a16263249a 100644
--- a/.gitignore
+++ b/.gitignore
@@ -106,6 +106,7 @@
 /git-pack-refs
 /git-parse-remote
 /git-patch-id
+/git-post
 /git-prune
 /git-prune-packed
 /git-pull
diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index d35d771fc8..6b34f4d994 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -226,7 +226,8 @@ context lines.
 
 SEE ALSO
 
-linkgit:git-revert[1]
+linkgit:git-revert[1],
+linkgit:git-post[1]
 
 GIT
 ---
diff --git a/Documentation/git-post.txt b/Documentation/git-post.txt
new file mode 100644
index 00..e835e62be3
--- /dev/null
+++ b/Documentation/git-post.txt
@@ -0,0 +1,57 @@
+git-post(1)
+===
+
+NAME
+
+git-post - Apply a commit on top of a branch that is not checked out
+
+SYNOPSIS
+
+[verse]
+'git post' dest-branch [source-rev]
+
+DESCRIPTION
+---
+
+Applies the changes made by 'source-rev' (or, if not given, `HEAD`)
+on top of the branch 'dest-branch' and records a new commit.
+'dest-branch' is advanced to point to the new commit.
+The operation that this command performs can be regarded as
+the opposite of cherry-picking.
+
+EXAMPLES
+
+
+Assume, while working on a topic, you find and fix an unrelated bug.
+Now:
+
+
+$ git commit   <1>
+$ git post master  <2>
+$ git show | git apply -R && git reset HEAD^   <3>
+
+
+<1> create a commit with the fix on the current branch
+<2> copy the fix onto the branch where it ought to be
+<3> revert current topic branch to the unfixed state;
+can also be done with `git reset --keep HEAD^` if there are no
+unstaged changes in files that are modified by the fix
+
+Oftentimes, switching branches triggers a rebuild of a code base.
+With the sequence above the branch switch can be avoided.
+That said, it is good practice to test the bug fix on the
+destination branch eventually.
+
+BUGS
+
+
+The change can be applied on `dest-branch` only if there is
+no textual conflict.
+
+SEE ALSO
+
+linkgit:git-cherry-pick[1].
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index b143e4eea3..1753bf176f 100644
--- a/Makefile
+++ b/Makefile
@@ -551,6 +551,7 @@ SCRIPT_SH += git-merge-octopus.sh
 SCRIPT_SH += git-merge-one-file.sh
 SCRIPT_SH += git-merge-resolve.sh
 SCRIPT_SH += git-mergetool.sh
+SCRIPT_SH += git-post.sh
 SCRIPT_SH += git-quiltimport.sh
 SCRIPT_SH += git-rebase.sh
 SCRIPT_SH += git-remote-testgit.sh
diff --git a/command-list.txt b/command-list.txt
index a1fad28fd8..bc95b424d0 100644
--- a/command-list.txt
+++ b/command-list.txt
@@ -98,6 +98,7 @@ git-pack-redundant  plumbinginterrogators
 git-pack-refs   ancillarymanipulators
 git-parse-remotesynchelpers
 git-patch-idpurehelpers
+git-postmainporcelain
 git-prune   ancillarymanipulators
 git-prune-packedplumbingmanipulators
 git-pullmainporcelain   remote
diff --git a/git-post.sh b/git-post.sh
new file mode 100755
index 00..6627d69f73
--- /dev/null
+++ b/git-post.sh
@@ -0,0 +1,60 @@
+#!/bin/sh
+# Copyright (c) 2017 Johannes Sixt
+
+SUBDIRECTORY_OK=Yes
+OPTIONS_SPEC="\
+git post dest-branch [source-rev]
+--
+"
+. git-sh-setup
+
+while test $# != 0
+do
+   case "$1" in
+   --) shift; break;;
+   -*) usage;;
+   *)  break;;
+   esac
+   shift
+done
+
+dest=$(git rev-parse --verify --symbolic-full-name "$1") || exit
+if test -z "$dest"
+then
+   die "$(gettext "Destination must be a branch tip")"
+fi
+
+shift
+case $# in
+0) set -- HEAD;;
+1) : good;;
+*) usage;;
+esac
+
+# apply change to a temporar

Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Johannes Sixt]

Am 04.10.2017 um 06:59 schrieb Junio C Hamano:

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


Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:

diff --git a/sub-process.c b/sub-process.c
index 6dde5062be..4680af8193 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
   {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
+   const char **argv = xmalloc(2 * sizeof(char *));
+   argv[0] = cmd;
+   argv[1] = NULL;
entry->cmd = cmd;
process = >process;



Perhaps this should become

argv_array_push(>args, cmd);

so that there is no new memory leak?


Sounds like a good idea (if I am not grossly mistaken as to what is
being suggested).

Here is what I am planning to queue.

-- >8 --
From: Johannes Sixt <j...@kdbg.org>
Date: Tue, 3 Oct 2017 22:24:57 +0200
Subject: [PATCH] sub-process: use child_process.args instead of 
child_process.argv

Currently the argv is only allocated on the stack, and then assigned to
process->argv.  When the start_subprocess function goes out of scope,
the local argv variable is eliminated from the stack, but the pointer is
still kept around in process->argv.

Much later when we try to access the same process->argv in
finish_command, this leads us to access a memory location that no longer
contains what we want.  As argv0 is only used for printing errors, this
is not easily noticed in normal git operations.  However when running
t0021-conversion.sh through valgrind, valgrind rightfully complains:

==21024== Invalid read of size 8
==21024==at 0x2ACF64: finish_command (run-command.c:869)
==21024==by 0x2D6B18: subprocess_exit_handler (sub-process.c:72)
==21024==by 0x2AB41E: cleanup_children (run-command.c:45)
==21024==by 0x2AB526: cleanup_children_on_exit (run-command.c:81)
==21024==by 0x54AD487: __run_exit_handlers (in /usr/lib/libc-2.26.so)
==21024==by 0x54AD4D9: exit (in /usr/lib/libc-2.26.so)
==21024==by 0x11A9EF: handle_builtin (git.c:550)
==21024==by 0x11ABCC: run_argv (git.c:602)
==21024==by 0x11AD8E: cmd_main (git.c:679)
==21024==by 0x1BF125: main (common-main.c:43)
==21024==  Address 0x1ffeffec00 is on thread 1's stack
==21024==  1504 bytes below stack pointer
==21024==

These days, the child_process structure has its own args array, and
the standard way to set up its argv[] is to use that one, instead of
assigning to process->argv to point at an array that is outside.
Use that facility automatically fixes this issue.

Reported-by: Thomas Gummerer <t.gumme...@gmail.com>
Signed-off-by: Johannes Sixt <j...@kdbg.org>
Signed-off-by: Junio C Hamano <gits...@pobox.com>
---
  sub-process.c | 3 +--
  1 file changed, 1 insertion(+), 2 deletions(-)

diff --git a/sub-process.c b/sub-process.c
index fcc4832c14..648b3a3943 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -74,13 +74,12 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
  {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
  
  	entry->cmd = cmd;

process = >process;
  
  	child_process_init(process);

-   process->argv = argv;
+   argv_array_push(>args, cmd);
process->use_shell = 1;
process->in = -1;
process->out = -1;



Thank you very much! That looks good. Just to be on the safe side:

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

-- Hannes

Re: [PATCH 3/3] sub-process: allocate argv on the heap

[Johannes Sixt]

Am 03.10.2017 um 21:57 schrieb Thomas Gummerer:

diff --git a/sub-process.c b/sub-process.c
index 6dde5062be..4680af8193 100644
--- a/sub-process.c
+++ b/sub-process.c
@@ -77,7 +77,9 @@ int subprocess_start(struct hashmap *hashmap, struct 
subprocess_entry *entry, co
  {
int err;
struct child_process *process;
-   const char *argv[] = { cmd, NULL };
+   const char **argv = xmalloc(2 * sizeof(char *));
+   argv[0] = cmd;
+   argv[1] = NULL;
  
  	entry->cmd = cmd;

process = >process;



Perhaps this should become

argv_array_push(>args, cmd);

so that there is no new memory leak?

-- Hannes

Re: what is git's position on "classic" mac -only end of lines?

[Johannes Sixt]

Am 01.10.2017 um 21:29 schrieb Bryan Turner:

On Sun, Oct 1, 2017 at 10:52 AM, Robert P. J. Day  wrote:


   sorry for more pedantic nitpickery, but i'm trying to write a
section on how to properly process mixtures of EOLs in git, and when i
read "man git-config", everything seems to refer to Mac OS X and macOS
(and linux, of course) using  for EOL, with very little mention of
what one does if faced with "classic" mac EOL of just .


  No command in Git that I'm aware of considers a standalone  to be
a line ending. A file containing only s is treated as a single
line by every Git command I've used. I'm not sure whether that
behavior is configurable. For files with standalone s mixed with
other line endings ( or , either or both), the  and
 endings are both considered line endings while the standalone
s are not.


That's true, AFAIK. In addition, when Git auto-detects whether a file is 
binary or text, then a file with a bare CR is treated as binary:


https://github.com/git/git/blob/master/convert.c#L91

That basically amounts to: "it [is] considered not important enough to 
deal with" ;)


-- Hannes

Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules

[Johannes Sixt]

Am 26.09.2017 um 20:54 schrieb Stefan Beller:

+test_expect_success 'submodule update - command in .gitmodules is ignored' '
+   test_when_finished "git -C super reset --hard HEAD^" &&
+
+   git -C super config -f .gitmodules submodule.submodule.update "!false" 
&&
+   git -C super commit -a -m "add command to .gitmodules file" &&
+   git -C super/submodule reset --hard $submodulesha1^ &&
+   git -C super submodule update submodule &&
+   test_path_is_missing bad


This test for a missing file is certainly a remnant from the previous 
iteration, isn't it?


-- Hannes

Re: [PATCH] t7406: submodule..update command must not be run from .gitmodules

[Johannes Sixt]

Am 26.09.2017 um 00:50 schrieb Stefan Beller:

submodule..update can be assigned an arbitrary command via setting
it to "!command". When this command is found in the regular config, Git
ought to just run that command instead of other update mechanisms.

However if that command is just found in the .gitmodules file, it is
potentially untrusted, which is why we do not run it.  Add a test
confirming the behavior.

Suggested-by: Jonathan Nieder 
Signed-off-by: Stefan Beller 
---

  updated to use the super robust script.
  Thanks Jonathan,
  
  Stefan


  t/t7406-submodule-update.sh | 14 ++
  1 file changed, 14 insertions(+)

diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 034914a14f..d718cb00e7 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -406,6 +406,20 @@ test_expect_success 'submodule update - command in 
.git/config' '
)
  '
  
+test_expect_success 'submodule update - command in .gitmodules is ignored' '

+   test_when_finished "git -C super reset --hard HEAD^" &&
+
+   write_script must_not_run.sh <<-EOF &&
+   >$TEST_DIRECTORY/bad
+   EOF


I am pretty confident that this does not test what you intend to test. 
Notice that $TEST_DIRECTORY is expanded when the script is written. But 
that path contains a blank, and we have something like this in the test 
script:


#!/bin/sh
>/the/build/directory/t/trash directory.t7406/bad

If you inject the bug against which this test protects into 
git-submodule, you should find a file "trash" in your t directory, and 
the file "bad" still absent. Not to mention that the script fails 
because it cannot run "directory.t7406/bad".


To fix that, you should use and exported variable and access that from 
the test script, for example:


write_script must_not_run.sh <<-\EOF &&
>"$TEST_DIRECTORY"/bad
EOF
...
(
export TEST_DIRECTORY &&
git -C super submodule update submodule
) &&
test_path_is_missing bad


+
+   git -C super config -f .gitmodules submodule.submodule.update 
"!$TEST_DIRECTORY/must_not_run.sh" &&
+   git -C super commit -a -m "add command to .gitmodules file" &&
+   git -C super/submodule reset --hard $submodulesha1^ &&
+   git -C super submodule update submodule &&
+   test_path_is_missing bad
+'
+
  cat << EOF >expect
  Execution of 'false $submodulesha1' failed in submodule path 'submodule'
  EOF



-- Hannes

Re: [PATCH] git: add --no-optional-locks option

[Johannes Sixt]

Am 21.09.2017 um 06:32 schrieb Jeff King:

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 6e3a6767e5..8dd3ae05ae 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -159,6 +159,10 @@ foo.bar= ...`) sets `foo.bar` to the empty string which ` 
git config
Add "icase" magic to all pathspec. This is equivalent to setting
the `GIT_ICASE_PATHSPECS` environment variable to `1`.
  
+--no-optional-locks::

+   Do not perform optional operations that require locks. This is
+   equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.
+
  GIT COMMANDS
  
  
@@ -697,6 +701,15 @@ of clones and fetches.

which feed potentially-untrusted URLS to git commands.  See
linkgit:git-config[1] for more details.
  
+`GIT_OPTIONAL_LOCKS`::

+   If set to `0`, Git will avoid performing any operations which
+   require taking a lock and which are not required to complete the
+   requested operation. For example, this will prevent `git status`
+   from refreshing the index as a side effect. This is useful for
+   processes running in the background which do not want to cause
+   lock contention with other operations on the repository.
+   Defaults to `1`.


I don't think we should pass this environment variable to remote 
repositories. It should be listed in local_repo_env[] in environment.c.


-- Hannes

Re: [Patch size_t V3 11/19] Use size_t for config parsing

[Johannes Sixt]

Am 16.08.2017 um 22:16 schrieb Martin Koegler:
> +int git_parse_size_t(const char *value, size_t *ret)
> +{
> + uintmax_t tmp;
> + if (!git_parse_unsigned(value, , 
> maximum_signed_value_of_type(size_t)))
> + return 0;
> + *ret = tmp;
> + return 1;
> +}
> +

I think this requires the following on top:

diff --git a/config.c b/config.c
index 81d46602f9..b3075aa1c4 100644
--- a/config.c
+++ b/config.c
@@ -866,7 +866,7 @@ static int git_parse_ssize_t(const char *value, ssize_t 
*ret)
 int git_parse_size_t(const char *value, size_t *ret)
 {
uintmax_t tmp;
-   if (!git_parse_unsigned(value, , 
maximum_signed_value_of_type(size_t)))
+   if (!git_parse_unsigned(value, , 
maximum_unsigned_value_of_type(size_t)))
return 0;
*ret = tmp;
return 1;

Re: How to force a push to succeed?

[Johannes Sixt]

Am 23.08.2017 um 00:26 schrieb Jeffrey Walton:

You know, I look at how fucked up yet another simple workflow is, and
all I can do is wonder. It is absolutely amazing. Its like the project
goes out of its way to make simple tasks difficult.


No, no. So simple your task might look, in the end you have LOST DATA! 
It is a design goal of Git to make it difficult to lose data.


-- Hannes

Re: [RFC 0/3] imap-send curl tunnelling support

[Johannes Sixt]

Am 21.08.2017 um 09:27 schrieb Nicolas Morey-Chaisemartin:

(Sent a reply from my phone while out of town but couldn't find it so here it 
is again)

It's available on my github:
https://github.com/nmorey/git/tree/dev/curl-tunnel

The series had been stlighly changed since the patch were posted, mostly to add 
the proper ifdefs to handle older curl versions.


This does not build for me on Windows due to a missing socketpair() 
function. But I am working in an old environment, so I do not know 
whether this statement has much value.


-- Hannes

mk-dontmerge/size-t-on-next test failure

[Johannes Sixt]

I observe the test failure below in t0040-parse-options.sh. It bisects
to 1a7909b25eb4ab3071ce4290115618e2582eadaa "Convert pack-objects to
size_t". It looks like git_parse_size_t() needs a fix. This is on
Windows, 32 bit. size_t, int and long are all 32 bits wide.

expecting success:
check magnitude: 1073741824 -m 1g

ok 18 - OPT_MAGNITUDE() giga

expecting success:
check magnitude: 3221225472 -m 3g

error: switch `m' expects a non-negative integer value with an optional
k/m/g suffix
usage: test-parse-options 

--yes get a boolean
-D, --no-doubtbegins with 'no-'
-B, --no-fear be brave
-b, --boolean increment by one
-4, --or4 bitwise-or boolean with ...0100
--neg-or4 same as --no-or4

-i, --integer  get a integer
-j get a integer, too
-m, --magnitudeget a magnitude
--set23   set integer to 23
-t  get timestamp of 
-L, --length get length of 
-F, --file  set file to 

String options
-s, --string 
  get a string
--string2get another string
--st  get another string (pervert ordering)
-o   get another string
--list   add str to list

Magic arguments
--quuxmeans --quux
-NUM  set integer to NUM
+ same as -b
--ambiguous   positive ambiguity
--no-ambiguousnegative ambiguity

Standard options
--abbrev[=]use  digits to display SHA-1s
-v, --verbose be verbose
-n, --dry-run dry run
-q, --quiet   be quiet
--expect  expected output in the variable dump

not ok 19 - OPT_MAGNITUDE() 3giga
#
#   check magnitude: 3221225472 -m 3g
#

Re: Git *accepts* a branch name, it can't identity in the future?

[Johannes Sixt]

Am 20.08.2017 um 09:51 schrieb Kaartic Sivaraam:

I made a small assumption in the script which turned out to be false. I
thought the unicode prefixes I used corresponded to only two bytes.
This lead to the issue. The unicode character '✓' corresponds to three
characters and as a result instead of removing it, my script replaced
it with the unknown character '�'. So, the branch named '✓doc-fix'
became 'done/�doc-fix'. Here's the issue. I couldn't use

 $ git branch -m done/�doc-fix done/dic-fix

to rename the branch. Nor could I refer to it in anyway. Git simply
says,

 error: pathspec 'done/�doc-fix' did not match any file(s) known to git.

It's not a big issue as I haven't lost anything out of it. The branches
have been merged into 'master'.

I just wanted to know why git accepted a branch name which it can't
identify later?

If it had rejected that name in the first place it would have been
better. In case you would like to know how I got that weird name,
here's a way to get that

 $ echo '✓doc-fix' | cut -c3-100



See, these two are different:

$ echo '✓doc-fix' | cut -c3-100 | od -t x1
000 93 64 6f 63 2d 66 69 78 0a
011
$ echo '�doc-fix' | od -t x1
000 64 6f bd 64 6f 63 2d 66 69 78 0a
013

It is not Git's fault that your terminal converts an invalid UTF-8 
sequence (that your script produces) to �. Nor is it when you paste that 
character onto the command line, that it is passed as a (correct) UTF-8 
character.


Perhaps this helps (untested):

$ git branch -m done/$(printf '\x93')doc-fix done/dic-fix

In Git's database, branch names are just sequences of bytes. It is 
outside the scope to verify that all input is encoded correctly.


-- Hannes

Re: [PATCH 2/5] pack-objects: take lock before accessing `remaining`

[Johannes Sixt]

Am 15.08.2017 um 14:53 schrieb Martin Ågren:

When checking the conditional of "while (me->remaining)", we did not
hold the lock. Calling find_deltas would still be safe, since it checks
"remaining" (after taking the lock) and is able to handle all values. In
fact, this could (currently) not trigger any bug: a bug could happen if
`remaining` transitioning from zero to non-zero races with the evaluation
of the while-condition, but these are always separated by the
data_ready-mechanism.

Make sure we have the lock when we read `remaining`. This does mean we
release it just so that find_deltas can take it immediately again. We
could tweak the contract so that the lock should be taken before calling
find_deltas, but let's defer that until someone can actually show that
"unlock+lock" has a measurable negative impact.

Signed-off-by: Martin Ågren 
---
I don't think this corrects any real error. The benefits of this patch
would be "future-proofs things slightly" and "silences tsan, so that
other errors don't drown in noise". Feel free to tell me those benefits
are negligible and that this change actually hurts.

  builtin/pack-objects.c | 6 ++
  1 file changed, 6 insertions(+)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c753e9237..bd391e97a 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2170,7 +2170,10 @@ static void *threaded_find_deltas(void *arg)
  {
struct thread_params *me = arg;
  
+	progress_lock();

while (me->remaining) {
+   progress_unlock();
+
find_deltas(me->list, >remaining,
me->window, me->depth, me->processed);
  
@@ -2192,7 +2195,10 @@ static void *threaded_find_deltas(void *arg)

pthread_cond_wait(>cond, >mutex);
me->data_ready = 0;
pthread_mutex_unlock(>mutex);
+
+   progress_lock();
}
+   progress_unlock();
/* leave ->working 1 so that this doesn't get more work assigned */
return NULL;
  }



It is correct that this access of me->remaining requires a lock. It 
could be solved in the way you did. I tried to do it differently, but 
all cleaner solutions that I can think of are overkill...


-- Hannes

Re: [PATCH] t1002: stop using sum(1)

[Johannes Sixt]

Am 15.08.2017 um 02:46 schrieb Jonathan Nieder:

Hi,

René Scharfe wrote:


sum(1) is a command for calculating checksums of the contents of files.
It was part of early editions of Unix ("Research Unix", 1972/1973, [1]).
cksum(1) appeared in 4.4BSD (1993) as a replacement [2], and became part
of POSIX.1-2008 [3].  OpenBSD 5.6 (2014) removed sum(1).

We only use sum(1) in t1002 to check for changes in three files.  On
MinGW we use md5sum(1) instead.  We could switch to the standard command
cksum(1) for all platforms; MinGW comes with GNU coreutils now, which
provides sum(1), cksum(1) and md5sum(1).  Use our standard method for
checking for file changes instead: test_cmp.

It's more convenient because it shows differences nicely, it's faster on
MinGW because we have a special implementation there based only on
shell-internal commands, it's simpler as it allows us to avoid stripping
out unnecessary entries from the checksum file using grep(1), and it's
more consistent with the rest of the test suite.

We already compare changed files with their expected new contents using
diff(1), so we don't need to check with "test_must_fail test_cmp" if
they differ from their original state.  A later patch could convert the
direct diff(1) calls to test_cmp as well.

With all sum(1) calls gone, remove the MinGW-specific implementation
from test-lib.sh as well.

[1] http://minnie.tuhs.org/cgi-bin/utree.pl?file=V3/man/man1/sum.1
[2] 
http://minnie.tuhs.org/cgi-bin/utree.pl?file=4.4BSD/usr/share/man/cat1/cksum.0
[3] http://pubs.opengroup.org/onlinepubs/9699919799/utilities/cksum.html
---
  t/t1002-read-tree-m-u-2way.sh | 67 ++-
  t/test-lib.sh |  3 --
  2 files changed, 35 insertions(+), 35 deletions(-)


Nicely analyzed.  May we forge your sign-off?

[...]

--- a/t/t1002-read-tree-m-u-2way.sh
+++ b/t/t1002-read-tree-m-u-2way.sh

[...]

@@ -132,8 +138,8 @@ test_expect_success \
   git ls-files --stage >7.out &&
   test_cmp M.out 7.out &&
   check_cache_at frotz dirty &&
- sum bozbar frotz nitfol >actual7.sum &&
- if cmp M.sum actual7.sum; then false; else :; fi &&
+ test_cmp bozbar.M bozbar &&
+ test_cmp nitfol.M nitfol &&


This one is strange.  What is that '! cmp' trying to check for?
Does the replacement capture the same thing?

E.g., does it need a '! test_cmp frotz.M frotz &&' line?


In the context that you stripped a 'diff frotz frotz1' occurs, i.e., a 
check for the expected content of the file whose content changes. 
Together with the new test_cmp lines, the new text is a stricter check 
than we have in the old text.


The patch looks good.

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

-- Hannes

Re: [PATCH] tree-walk: convert fill_tree_descriptor() to object_id

[Johannes Sixt]

Am 12.08.2017 um 09:14 schrieb René Scharfe:

-   Initialize a `tree_desc` and decode its first entry given the sha1 of
-   a tree. Returns the `buffer` member if the sha1 is a valid tree
-   identifier and NULL otherwise.
+   Initialize a `tree_desc` and decode its first entry given the
+   object ID of a tree. Returns the `buffer` member if the sha1 is
+   a valid tree identifier and NULL otherwise.


There is another mention of "sha1" in the rewritten text. Intended or an 
oversight?



-#define ENTRY_SHA1(e) (((e)->mode && S_ISDIR((e)->mode)) ? (e)->oid->hash : 
NULL)
-   buf0 = fill_tree_descriptor(t+0, ENTRY_SHA1(n + 0));
-   buf1 = fill_tree_descriptor(t+1, ENTRY_SHA1(n + 1));
-   buf2 = fill_tree_descriptor(t+2, ENTRY_SHA1(n + 2));
+#define ENTRY_OID(e) (((e)->mode && S_ISDIR((e)->mode)) ? (e)->oid : NULL)
+   buf0 = fill_tree_descriptor(t + 0, ENTRY_OID(n + 0));
+   buf1 = fill_tree_descriptor(t + 1, ENTRY_OID(n + 1));
+   buf2 = fill_tree_descriptor(t + 2, ENTRY_OID(n + 2));
  #undef ENTRY_SHA1


This whould be #undef ENTRY_OID.

-- Hannes

Re: [git-for-windows] [ANNOUNCE] Git for Windows 2.14.0

[Johannes Sixt]

Am 07.08.2017 um 12:02 schrieb Johannes Schindelin:

On Sun, 6 Aug 2017, Johannes Sixt wrote:

Am 06.08.2017 um 01:00 schrieb Johannes Schindelin:

* Comes with [BusyBox v1.28.0pre.15857.9480dca7c](https://github.com/
  git-for-windows/busybox-w32/commit/9480dca7c].


What is the implication of this addition? I guess it is not just for the
fun of it. Does it mean that all POSIX command line tools invoked by Git
including a POSIX shell are now routed through busybox instead of the
MSYS2 variant?


As I wrote a little later:

* Git for Windows releases now also include an experimental [BusyBox-based
   
MinGit](https://github.com/git-for-windows/git/wiki/MinGit#experimental-busybox-based-mingit).


Thanks for the clue. It's an interesting concept. I would be interested 
in replacing my old MSYS environment by BusyBox. At best, it would be 
just a matter of replacing sh.exe.


-- Hannes

Re: [git-for-windows] [ANNOUNCE] Git for Windows 2.14.0

[Johannes Sixt]

Am 06.08.2017 um 01:00 schrieb Johannes Schindelin:

Dear Git users,

It is my pleasure to announce that Git for Windows 2.14.0 is available from:

https://git-for-windows.github.io/

Changes since Git for Windows v2.13.3 (July 13th 2017)


Thank you so much! One question, though:


New Features
...
   * Comes with [BusyBox v1.28.0pre.15857.9480dca7c](https://github.com/
 git-for-windows/busybox-w32/commit/9480dca7c].


What is the implication of this addition? I guess it is not just for the 
fun of it. Does it mean that all POSIX command line tools invoked by Git 
including a POSIX shell are now routed through busybox instead of the 
MSYS2 variant?


-- Hannes

Re: reftable: new ref storage format

[Johannes Sixt]

Am 16.07.2017 um 12:03 schrieb Jeff King:

On Sun, Jul 16, 2017 at 10:07:57AM +0200, Johannes Sixt wrote:


Am 14.07.2017 um 22:08 schrieb Jeff King:

The implementation on this doesn't seem overly complex. My main concerns
are what we're asking from the filesystem in terms of atomicity, and
what possible races there are.


One of the failure modes is that on Windows a file cannot be deleted while
it is open in any process. It can happen that a compacting updater wants to
remove a reftable file that is still open in a reader.


Good point. I think the explicit pointers I mentioned are an improvement
there, because a compacting updater _can_ leave the file in place if the
delete fails (and later, another compaction can clean up cruft that was
left).


Yes, I think so, too. The pointers make things so much simpler.


I assume that's more or less how pack deletion works on Windows.


Correct.

-- Hannes

Re: reftable: new ref storage format

[Johannes Sixt]

Am 14.07.2017 um 22:08 schrieb Jeff King:

The implementation on this doesn't seem overly complex. My main concerns
are what we're asking from the filesystem in terms of atomicity, and
what possible races there are.


One of the failure modes is that on Windows a file cannot be deleted 
while it is open in any process. It can happen that a compacting updater 
wants to remove a reftable file that is still open in a reader.


-- Hannes

Re: [FEATURE] git-commit option to prepend filename to commit message

[Johannes Sixt]

Am 15.07.2017 um 16:19 schrieb John J Foerch:

The feature would be a command line option for git commit that would
automatically prepend the ": " to the commit message.

Write a prepare-commit-msg hook:

https://www.kernel.org/pub/software/scm/git/docs/githooks.html#_prepare_commit_msg

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 12.07.2017 um 21:12 schrieb Ævar Arnfjörð Bjarmason:

I think in the context of this desire Johannes Sixt's "Actually, I'm
serious [about let's compile with c++]"[1] should be given some more
consideration.


Thank you for your support.


I've just compiled Git with it and it passes all tests. I think the
endeavor is worthwhile in itself as C++ source-level compatibility for
git.git is clearly easy to achieve, and would effectively give us access
to more compilers (albeit in different modes, but they may discover
novel bugs that also apply to the C mode code).


Please keep in mind that my patches only show that it can be done. 
Nevertheless, the result is far, far away from valid C++ code. It can be 
compiled by GCC (thanks to its -fpermissive flag) and happens to produce 
something that works. But that does not mean that other compilers would 
grok it.


Source-level compatibility is only necessary as a stop gap in the 
transition to C++. If the transition is not going to happen, I doubt 
that there is any value. It is simply too much code churn for too little 
gain. The real gains are in the features of C++(11,14).



But why do it? Aside from the "more compilers" argument, we may find
that it's going to be much easier to use some C99 features we want by
having C++ source-level compatibility, and on compilers like that may
not support those features in C use the C++ mode that may support those.


I would be happy to learn about a C99 feature where C++ mode of some 
compiler would help.


The only C99 feature mentioned so far was designated initializers. 
Unfortunately, that actually widens the gap between C and C++, not 
lessens it. (C++ does not have designated initializers, and they are not 
on the agenda.)



If not C++ support would be interesting for other reasons. Johannes
Sixt: It would be very nice to get those patches on-list.


I don't think it's worth to swamp the list with the patches at this 
time. Interested parties can find them here:


https://github.com/j6t/git.git c-plus-plus

I may continue the work, slowly and as long as I find it funny. It's 
just mental exercise, unless the Git community copies the GCC Steering 
Committee's mindeset with regard to C++ in the code base 
(https://gcc.gnu.org/ml/gcc/2010-05/msg00705.html).


-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 12.07.2017 um 03:26 schrieb brian m. carlson:

On Tue, Jul 11, 2017 at 07:24:07AM +0200, Johannes Sixt wrote:

I can revive the patches if there is interest.


I'd be interested in at least a pointer to them if you have one.  I
think it might allow us to take advantage of desirable features that are
in the intersection of C99 and C++.


See here: https://github.com/j6t/git.git  c-plus-plus

I do not think that there is a lot of intersection, though.

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 11.07.2017 um 02:05 schrieb brian m. carlson:

On Mon, Jul 10, 2017 at 09:57:40PM +0200, Johannes Sixt wrote:

It's a pity, though, that you do not suggest something even more useful,
such as C++14.


I have tried compiling Git with a C++ compiler, so that I could test
whether that was a viable alternative for MSVC in case its C++ mode
supported features its C mode did not.  Let's just say that the
compilation aborted very quickly and I gave up after a few minutes.


It's 3 cleanup patches and one hacky patch with this size:

 80 files changed, 899 insertions(+), 807 deletions(-)

to compile with C++. It passed the test suite last time I tried. Getting 
rid of the remaining 1000+ -fpermissive warnings is a different matter, 
though.


I can revive the patches if there is interest.

-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 10.07.2017 um 22:38 schrieb Junio C Hamano:

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


It's a pity, though, that you do not suggest something even more
useful, such as C++14.


I cannot tell if this part is tongue-in-cheek (especially the "++"),
so I will ignore it to avoid wasting time.


Actually, I'm serious.


Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT



-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }


While this may serve as a test balloon, changing STRBUF_INIT, or any
of those _INIT macros, is actually the least interesting. The
interesting instances are initializations for which we do *not* have a
macro.


I am not sure what negative impact you think the macro-ness would
have to the validity of the result from this test balloon.  An old
compiler that does not understand designated initializer syntax
would fail to compile both the same way, no?

struct strbuf buf0 = STRBUF_INIT;
struct strbuf buf1 = { .alloc = 0, .len = 0, .buf = strbuf_slopbuf };


I said it is uninteresting, not that there is a negative impact. There 
is simply nothing gained for strbuf users: They would use STRBUF_INIT 
before and after the change and would not benefit from designated 
initializers.


This change may serve well as a test balloon, but not as an example of 
the sort of changes that we would want to see later (of the kind "change 
FOO_INIT macro to use designated initializers"; they are just code churn).


-- Hannes

Re: [PATCH] strbuf: use designated initializers in STRBUF_INIT

[Johannes Sixt]

Am 10.07.2017 um 09:03 schrieb Jeff King:

On Sun, Jul 09, 2017 at 10:05:49AM -0700, Junio C Hamano wrote:


René Scharfe  writes:


I wonder when we can begin to target C99 in git's source, though. :)


Let's get the ball rolling...


Good to know that you do not resist moving to a more modern build 
environment.



by starting to use some of the useful
features like designated initializers,


It's a pity, though, that you do not suggest something even more useful, 
such as C++14.



Subject: [PATCH] strbuf: use designated initializers in STRBUF_INIT



-#define STRBUF_INIT  { 0, 0, strbuf_slopbuf }
+#define STRBUF_INIT  { .alloc = 0, .len = 0, .buf = strbuf_slopbuf }


While this may serve as a test balloon, changing STRBUF_INIT, or any of 
those _INIT macros, is actually the least interesting. The interesting 
instances are initializations for which we do *not* have a macro.


-- Hannes

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Johannes Sixt]

Am 16.06.2017 um 20:43 schrieb Johannes Sixt:

Am 16.06.2017 um 15:49 schrieb Johannes Schindelin:

On Thu, 15 Jun 2017, Junio C Hamano wrote:

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 325ec75353..801bce25da 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -45,7 +45,7 @@ create_expected_success_am() {
  }
  create_expected_success_interactive() {
-cr=$'\r' &&
+cr=$(echo . | tr '.' '\015') &&
  cat >expected <<-EOF
  $(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
  HEAD is now at $(git rev-parse --short feature-branch) third 
commit


This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
would emit) is interpreted correctly as a line break on Windows, meaning
that cr is now *empty*. Not what we want.

What I did is to replace the `cat` by `q_to_cr` (we have that lovely
function, might just as well use it), replace `${cr}` by `Q` and skip the
cr variable altogether.


You beat me to it. I came up with the identical q_to_cr changes, but 
haven't dug the remaining failure regarding the swapped output lines. 
You seem to have nailed it. Will test your proposed changes tomorrow.


As expected, the patches fix the observed test failures for me, too, if 
that's still relevant.


-- Hannes

Re: [PATCH v2 0/3] Add regression tests for rectent rebase -i fixes

[Johannes Sixt]

Am 16.06.2017 um 15:49 schrieb Johannes Schindelin:

On Thu, 15 Jun 2017, Junio C Hamano wrote:

diff --git a/t/t3420-rebase-autostash.sh b/t/t3420-rebase-autostash.sh
index 325ec75353..801bce25da 100755
--- a/t/t3420-rebase-autostash.sh
+++ b/t/t3420-rebase-autostash.sh
@@ -45,7 +45,7 @@ create_expected_success_am() {
  }
  
  create_expected_success_interactive() {

-   cr=$'\r' &&
+   cr=$(echo . | tr '.' '\015') &&
cat >expected <<-EOF
$(grep "^Created autostash: [0-9a-f][0-9a-f]*\$" actual)
HEAD is now at $(git rev-parse --short feature-branch) third commit


This is still incorrect, as the \r\n (which $(echo . | tr \.\ '\015')
would emit) is interpreted correctly as a line break on Windows, meaning
that cr is now *empty*. Not what we want.

What I did is to replace the `cat` by `q_to_cr` (we have that lovely
function, might just as well use it), replace `${cr}` by `Q` and skip the
cr variable altogether.


You beat me to it. I came up with the identical q_to_cr changes, but 
haven't dug the remaining failure regarding the swapped output lines. 
You seem to have nailed it. Will test your proposed changes tomorrow.


-- Hannes

Re: What's cooking in git.git (Jun 2017, #01; Thu, 1)

[Johannes Sixt]

Am 01.06.2017 um 09:44 schrieb Junio C Hamano:

* nd/fopen-errors (2017-05-30) 14 commits
  - mingw_fopen: report ENOENT for invalid file names
  - SQUASH??? use test_i18ngrep and add it at the end
  - mingw: verify that paths are not mistaken for remote nicknames
  - log: fix memory leak in open_next_file()
  - rerere.c: move error_errno() closer to the source system call
  - print errno when reporting a system call error
  - wrapper.c: make warn_on_inaccessible() static
  - wrapper.c: add and use fopen_or_warn()
  - wrapper.c: add and use warn_on_fopen_errors()
  - config.mak.uname: set FREAD_READS_DIRECTORIES for Darwin, too
  - config.mak.uname: set FREAD_READS_DIRECTORIES for Linux and FreeBSD
  - clone: use xfopen() instead of fopen()
  - use xfopen() in more places
  - git_fopen: fix a sparse 'not declared' warning

  We often try to open a file for reading whose existence is
  optional, and silently ignore errors from open/fopen; report such
  errors if they are not due to missing files.

  Waiting for an Ack to the SQUASH fix or a reroll of the tip commits.


ACK!

See also 
https://public-inbox.org/git/2899d715-a416-1852-4399-28af0a3e9...@kdbg.org/


-- Hannes

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 30.05.2017 um 06:46 schrieb Junio C Hamano:

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


Doesn't this need test_i18ngrep?:


Good catch! It would be this one in warn_on_inaccessible:


  wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);


But actually, I'm more worried about the unholy mix of
one-test-first-then-skip_all-later that occurs in this test script (I
do not mean the skip_all that is visible in the context, there are
others later). I think there was some buzz recently that prove only
understands a summary line that reads "1..0", but here we would see
"1..1". What to do? Reorganize the test script? Dscho, any ideas?


For now I've queued this on top of 1/2, so that suggestions are not
lost, and then tweaked 2/2 (as context for the patch to the test
changes).

Either an ack or a reroll is appreciated (I do not think we'd
terribly mind if this test were added to another script, or if this
test were skipped when UNC path cannot be determined even though it
does not need that prereq.  Also UNC_PATH can become prereq that is
tested by individual test in this script and the new test can be
added without requiring that prereq).

Thanks.

  t/t5580-clone-push-unc.sh | 12 ++--
  1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..944730cddc 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,12 +8,6 @@ if ! test_have_prereq MINGW; then
test_done
  fi
  
-test_expect_failure 'remote nick cannot contain backslashes' '

-   BACKSLASHED="$(pwd | tr / )" &&
-   git ls-remote "$BACKSLASHED" >out 2>err &&
-   ! grep "unable to access" err
-'
-
  UNCPATH="$(pwd)"
  case "$UNCPATH" in
  [A-Z]:*)
@@ -51,4 +45,10 @@ test_expect_success push '
test "$rev" = "$(git rev-parse --verify refs/heads/to-push)"
  '
  
+test_expect_failure 'remote nick cannot contain backslashes' '

+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   test_i18ngrep ! "unable to access" err
+'
+
  test_done



ACK!

-- Hannes

Re: Error with Templates: Could not find templates on cloning but on creating

[Johannes Sixt]

Am 29.05.2017 um 13:20 schrieb Mathias Artus:

Hi,
Today i've tried to set up a template directory. I added in the system wide 
gitconfig the following lines:

[init]
templatedir = "//OurServer/SomeDirectory/GitTemplate"

Where //Ourserver is a Network Path.
With this line i can create a new Repository and the template gets copied. But 
when i clone a repo the following error shows up and the template doesn't get 
copied:
templates not found /OurServer/SomeDirectory/GitTemplate

I Recognized that one slash was missing. Hence i added one:
[init]
templatedir = "///OurServer/SomeDirectory/GitTemplate"

Fine, cloning works after that, but creating a new repository then shows up a 
Warning:
templates not found ///OurServer/SomeDirectory/GitTemplate

Is that a known bug or is it my Failure?

I use git 2.13 on windows 7


I cannot reproduce. I'm on Windows 8.1, but I wouldn't expect that to 
make a difference. Are you using Cygwin's git by any chance?


-- Hannes

Re: [PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 29.05.2017 um 22:40 schrieb Ævar Arnfjörð Bjarmason:

On Mon, May 29, 2017 at 10:25 PM, Johannes Sixt <j...@kdbg.org> wrote:

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b195f71ea9..fd719a209e 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -1,13 +1,19 @@
  #!/bin/sh

-test_description='various UNC path tests (Windows-only)'
+test_description='various Windows-only path tests'
  . ./test-lib.sh

  if ! test_have_prereq MINGW; then
-   skip_all='skipping UNC path tests, requires Windows'
+   skip_all='skipping Windows-only path tests'
 test_done
  fi

+test_expect_failure 'remote nick cannot contain backslashes' '
+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   ! grep "unable to access" err
+'


Doesn't this need test_i18ngrep?:


Good catch! It would be this one in warn_on_inaccessible:


 wrapper.c:581:  warning_errno(_("unable to access '%s'"), path);


But actually, I'm more worried about the unholy mix of 
one-test-first-then-skip_all-later that occurs in this test script (I do 
not mean the skip_all that is visible in the context, there are others 
later). I think there was some buzz recently that prove only understands 
a summary line that reads "1..0", but here we would see "1..1". What to 
do? Reorganize the test script? Dscho, any ideas?


-- Hannes

[PATCH 2/2] mingw_fopen: report ENOENT for invalid file names

[Johannes Sixt]

On Windows, certain characters are prohibited in file names, most
prominently the colon. When fopen() is called with such an invalid file
name, the underlying Windows API actually reports a particular error,
but since there is no suitable errno value, this error is translated
to EINVAL. Detect the case and report ENOENT instead.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 compat/mingw.c| 2 ++
 t/t5580-clone-push-unc.sh | 2 +-
 2 files changed, 3 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 62109cc4e6..ce6fe8f46b 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -423,6 +423,8 @@ FILE *mingw_fopen (const char *filename, const char 
*otype)

return NULL;
}
file = _wfopen(wfilename, wotype);
+   if (!file && GetLastError() == ERROR_INVALID_NAME)
+   errno = ENOENT;
if (file && hide && set_hidden_flag(wfilename, 1))
warning("could not mark '%s' as hidden.", filename);
return file;
diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index fd719a209e..93ce99ba3c 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -8,7 +8,7 @@ if ! test_have_prereq MINGW; then
test_done
 fi

-test_expect_failure 'remote nick cannot contain backslashes' '
+test_expect_success 'remote nick cannot contain backslashes' '
BACKSLASHED="$(pwd | tr / )" &&
git ls-remote "$BACKSLASHED" >out 2>err &&
! grep "unable to access" err
--
2.13.0.55.g17b7d13330

[PATCH 1/2] mingw: verify that paths are not mistaken for remote nicknames

[Johannes Sixt]

Am 26.05.2017 um 05:35 schrieb Junio C Hamano:
> When asked to open/fopen a path, e.g. "a/b:/c", which does not exist
> on the filesystem, Windows (correctly) fails to open it but sets
> EINVAL to errno because the pathname has characters that cannot be
> stored in its filesystem.
> 
> As this is an expected failure, teach is_missing_file_error() helper
> about this case.
> 
> This is RFC, as there may be a case where we get EINVAL from
> open/fopen for reasons other than "the filesystem does not like this
> pathname" that may be worth reporting to the user, and this change
> is sweeping such an error under the rug.
> 
> Signed-off-by: Junio C Hamano <gits...@pobox.com>
> ---
>   wrapper.c | 4 
>   1 file changed, 4 insertions(+)
> 
> diff --git a/wrapper.c b/wrapper.c
> index f1c87ec7ea..74aa3b7803 100644
> --- a/wrapper.c
> +++ b/wrapper.c
> @@ -434,6 +434,10 @@ static void warn_on_inaccessible(const char *path)
>* see if the errno indicates a missing file that we can safely ignore.
>*/
>   static int is_missing_file_error(int errno_) {
> +#ifdef GIT_WINDOWS_NATIVE
> + if (errno_ == EINVAL)
> + return 1;
> +#endif
>   return (errno_ == ENOENT || errno_ == ENOTDIR);
>   }

I would prefer to catch the case in the compatibility layer. Here is
a two patch series that would replace your 12/13 and 13/13.

 8< 
From: Johannes Schindelin <johannes.schinde...@gmx.de>
Subject: mingw: verify that paths are not mistaken for remote nicknames

This added test case simply verifies that users will not be bothered
with bogus complaints à la

warning: unable to access '.git/remotes/D:\repo': Invalid argument

when fetching from a Windows path (in this case, D:\repo).

[j6t: mark the new test as test_expect_failure]

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t5580-clone-push-unc.sh | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b195f71ea9..fd719a209e 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -1,13 +1,19 @@
 #!/bin/sh
 
-test_description='various UNC path tests (Windows-only)'
+test_description='various Windows-only path tests'
 . ./test-lib.sh
 
 if ! test_have_prereq MINGW; then
-   skip_all='skipping UNC path tests, requires Windows'
+   skip_all='skipping Windows-only path tests'
test_done
 fi
 
+test_expect_failure 'remote nick cannot contain backslashes' '
+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   ! grep "unable to access" err
+'
+
 UNCPATH="$(pwd)"
 case "$UNCPATH" in
 [A-Z]:*)
-- 
2.13.0.55.g17b7d13330

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Johannes Sixt]

Am 27.05.2017 um 16:06 schrieb Ramsay Jones:

To be more explicit, last Sunday I hacked into t7407 to show an
example failure on cygwin (see patch below), but it passes on both
Linux (expected) and cygwin! :( Perhaps you can see what I'm doing
wrong?


As long as the git.exe you are using here is Cygwin's, Windows 
conventions do not apply. I think, the environment is transfered across 
the exec boundaries using Cygwin's own tools, and Windows's 
case-insensitive environment does not enter the game at all. But that's 
just a shot in the dark.




ATB,
Ramsay Jones

-- >8 --
Date: Sun, 21 May 2017 16:23:58 +0100
Subject: [PATCH] submodule: foreach $path munging on cygwin

---
  t/t7407-submodule-foreach.sh | 19 +++
  1 file changed, 19 insertions(+)

diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf42..c2d66bab7 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -68,17 +68,36 @@ Entering 'sub3'
  $pwd/clone-foo3-sub3-$sub3sha1
  EOF
  
+cat >expect-func <
+Entering 'sub1'
+running from TRASH
+path is <>
+Entering 'sub3'
+running from TRASH
+path is <>
+EOF
+
  test_expect_success 'test basic "submodule foreach" usage' '
+   PATH="$PWD:$PATH" &&
+   write_script foreach-func <<-\EOF &&
+   echo "running from TRASH"
+   echo "path is <<$1>>"
+   EOF
git clone super clone &&
(
cd clone &&
git submodule update --init -- sub1 sub3 &&
git submodule foreach "echo \$toplevel-\$name-\$path-\$sha1" > 
../actual &&
+   git submodule foreach "foreach-func \$path" > ../actual-func1 &&
+   git submodule foreach "export path; foreach-func \$path" > 
../actual-func2 &&
git config foo.bar zar &&
git submodule foreach "git config --file \"\$toplevel/.git/config\" 
foo.bar"
) &&
+   test_i18ncmp expect-func actual-func1 &&
+   test_i18ncmp expect-func actual-func2 &&
test_i18ncmp expect actual
  '
+test_done
  
  cat >expect <
  Entering '../sub1'

Re: [GSoC][PATCH v5 3/3] submodule: port subcommand foreach from shell to C

[Johannes Sixt]

Am 26.05.2017 um 17:17 schrieb Prathamesh Chavan:

+   argv_array_pushf(_array, "path=%s", list_item->name);


Not good! On Windows, environment variables are case insensitive. The 
environment variable "path" has a very special purpose, although it is 
generally spelled "PATH" (actually "Path" on Windows).


Lowercase "path" may have worked as long as it was only used in a shell 
script (and perhaps only by lucky coincidence), but this I can pretty 
much guarantee to fail. (I haven't tested it, though.)


The correct fix can only be to rename this variable here and in shell 
scripts that need the value that is set here.


-- Hannes

[PATCH v3 2/2] Windows: do not treat a path with backslashes as a remote's nick name

[Johannes Sixt]

On Windows, the remote repository name in, e.g., `git fetch foo\bar`
is clearly not a nickname for a configured remote repository. However,
the function valid_remote_nick() does not account for backslashes.
Use is_dir_sep() to check for both slashes and backslashes on Windows.

This was discovered while playing with Duy's patches that warn after
fopen() failures. The functions that read the branches and remotes
files are protected by a valid_remote_nick() check. Without this
change, a Windows style absolute path is incorrectly regarded as
nickname and is concatenated to a prefix and used with fopen(). This
triggers warnings because a colon in a path name is not allowed:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
>From C:\Temp\gittest
 * branchHEAD   -> FETCH_HEAD

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
Am 24.05.2017 um 07:45 schrieb Johannes Sixt:
> Am 24.05.2017 um 00:08 schrieb Junio C Hamano:
>> So in short:
>>
>>   (1) Hannes's patches are good, but they solve a problem that is
>>   different from what their log messages say; the log message
>>   needs to be updated;

I do not resend patch 1/2 as it is unchanged in all regards. This 2/2
changes the justification; patch text is unchanged.

 remote.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index ad6c5424ed..1949882c10 100644
--- a/remote.c
+++ b/remote.c
@@ -645,7 +645,12 @@ static int valid_remote_nick(const char *name)
 {
if (!name[0] || is_dot_or_dotdot(name))
return 0;
-   return !strchr(name, '/'); /* no slash */
+
+   /* remote nicknames cannot contain slashes */
+   while (*name)
+   if (is_dir_sep(*name++))
+   return 0;
+   return 1;
 }
 
 const char *remote_for_branch(struct branch *branch, int *explicit)
-- 
2.13.0.55.g17b7d13330

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 24.05.2017 um 00:08 schrieb Junio C Hamano:

So in short:

  (1) Hannes's patches are good, but they solve a problem that is
  different from what their log messages say; the log message
  needs to be updated;

  (2) In nd/fopen-errors topic, we need a new patch that deals with
  EINVAL on Windows.


Will reroll the patches. Good analysis, BTW. (It was a late discovery of 
mine that nd/fopen-errors is actually the source of the warnings.)


-- Hannes

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 23.05.2017 um 12:53 schrieb Johannes Schindelin:

Hi Hannes (& Junio, see below),

On Mon, 22 May 2017, Johannes Sixt wrote:


Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:

On Sat, 20 May 2017, Johannes Sixt wrote:

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid
warning: argument
  From C:\Temp\gittest
   * branchHEAD   -> FETCH_HEAD

The fix is in the second patch; the first patch is a
preparation that allows to write the fix in my preferred style.


Thank you!

Maybe you want to accompany these patches with a simple test case that
uses e.g. ls-remote on $(pwd | tr / )?


Actually, no, I don't want to. It would have to be protected by MINGW, and I
don't want to burden us (and here I mean Windows folks) with a check for a
cosmetic deficiency. (Shell scripts, slow forks, yadda, yadda...)


Actually, yes, I want to.

Yes, it would have to be protected by MINGW, and yes, it would put a
burden on us, but also yes: it would put an even higher burden on me to
check this manually before releasing Git for Windows, or even worse: this
regression would be the kind of bug that triggers many bug reports,
addressing which would take a lot more time than writing this test case
and executing it as part of our test suite.


Fair enough. The patch looks good. I'll be able to test no earlier than 
Monday, though.




So here goes:

-- snipsnap --
From: Johannes Schindelin <johannes.schinde...@gmx.de>
Date: Tue, 23 May 2017 12:42:13 +0200
Subject: [PATCH] mingw: verify that paths are not mistaken for remote nicknames

This added test case simply verifies that users will not be bothered
with bogus complaints à la

warning: unable to access '.git/remotes/D:\repo': Invalid argument

when fetching from a Windows path (in this case, D:\repo).

Signed-off-by: Johannes Schindelin <johannes.schinde...@gmx.de>
---
  t/t5580-clone-push-unc.sh | 10 --
  1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/t/t5580-clone-push-unc.sh b/t/t5580-clone-push-unc.sh
index b195f71ea98..93ce99ba3c6 100755
--- a/t/t5580-clone-push-unc.sh
+++ b/t/t5580-clone-push-unc.sh
@@ -1,13 +1,19 @@
  #!/bin/sh
  
-test_description='various UNC path tests (Windows-only)'

+test_description='various Windows-only path tests'
  . ./test-lib.sh
  
  if ! test_have_prereq MINGW; then

-   skip_all='skipping UNC path tests, requires Windows'
+   skip_all='skipping Windows-only path tests'
test_done
  fi
  
+test_expect_success 'remote nick cannot contain backslashes' '

+   BACKSLASHED="$(pwd | tr / )" &&
+   git ls-remote "$BACKSLASHED" >out 2>err &&
+   ! grep "unable to access" err
+'
+
  UNCPATH="$(pwd)"
  case "$UNCPATH" in
  [A-Z]:*)

Re: [PATCH v3] ref-filter: trim end whitespace in subject

[Johannes Sixt]

Am 22.05.2017 um 21:53 schrieb Jeff King:

On Mon, May 22, 2017 at 09:47:59PM +0200, Johannes Sixt wrote:


Am 22.05.2017 um 19:10 schrieb DOAN Tran Cong Danh:

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 5778c0afe..fa4441868 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -13,7 +13,8 @@ test_expect_success 'make commits' '
   test_expect_success 'make branches' '
git branch branch-one &&
-   git branch branch-two HEAD^
+   git branch branch-two $(printf "%s\r\n" one " " line3_long line4 |


I didn't notice earlier that there is a blank between the dq here.


+git commit-tree HEAD:)
   '
   test_expect_success 'make remote branches' '



This updated test shows nothing, I am afraid: If I apply only this change
without the rest of the patch, then all test in t3203 still pass. And I do
not see how the code change could make any difference at all. What am I
missing?


And I didn't look carefully enough at t3203. Some tests do check branch 
-v output.




It does for me here on Linux; I wonder if the CRs are being eaten by the
shell expansion.


And I tested on Linux, too, but on the wrong branch. On a branch closer 
to master I see a failure as well. Sorry for the noise.


There are no CRs on the command line, BTW, only on stdin of commit-tree.

-- Hannes

Re: [PATCH v3] ref-filter: trim end whitespace in subject

[Johannes Sixt]

Am 22.05.2017 um 19:10 schrieb DOAN Tran Cong Danh:

diff --git a/t/t3203-branch-output.sh b/t/t3203-branch-output.sh
index 5778c0afe..fa4441868 100755
--- a/t/t3203-branch-output.sh
+++ b/t/t3203-branch-output.sh
@@ -13,7 +13,8 @@ test_expect_success 'make commits' '
  
  test_expect_success 'make branches' '

git branch branch-one &&
-   git branch branch-two HEAD^
+   git branch branch-two $(printf "%s\r\n" one " " line3_long line4 |
+git commit-tree HEAD:)
  '
  
  test_expect_success 'make remote branches' '




This updated test shows nothing, I am afraid: If I apply only this 
change without the rest of the patch, then all test in t3203 still pass. 
And I do not see how the code change could make any difference at all. 
What am I missing?


-- Hannes

[PATCH v2 2/2] Windows: do not treat a path with backslashes as a remote's nick name

[Johannes Sixt]

Fetching from a remote using a native Windows path produces these warnings:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
>From C:\Temp\gittest
 * branchHEAD   -> FETCH_HEAD

The functions that read the branches and remotes files are protected by
a valid_remote_nick() check. Its implementation does not take Windows
paths into account, so that the caller simply concatenates the paths,
leading to the error seen above.

Use is_dir_sep() to check for both slashes and backslashes on Windows.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 This v2 writes your suggested comment in front of the loop (not just
 the 'if' because then I feel I would have to add braces).

 remote.c | 7 ++-
 1 file changed, 6 insertions(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index ad6c5424ed..1949882c10 100644
--- a/remote.c
+++ b/remote.c
@@ -645,7 +645,12 @@ static int valid_remote_nick(const char *name)
 {
if (!name[0] || is_dot_or_dotdot(name))
return 0;
-   return !strchr(name, '/'); /* no slash */
+
+   /* remote nicknames cannot contain slashes */
+   while (*name)
+   if (is_dir_sep(*name++))
+   return 0;
+   return 1;
 }
 
 const char *remote_for_branch(struct branch *branch, int *explicit)
-- 
2.13.0.55.g17b7d13330

[PATCH v2 1/2] mingw.h: permit arguments with side effects for is_dir_sep

[Johannes Sixt]

Taking git-compat-util.h's cue (which uses an inline function to back
is_dir_sep()), let's use an inline function to back also the Windows
version of is_dir_sep(). This avoids problems when calling the function
with arguments that do more than just provide a single character, e.g.
incrementing a pointer. Example:

is_dir_sep(*p++)

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 This v2 takes your commit message body because I like it better
 than mine. I did not change the subject because your suggestion
 sounded like the exact opposite of what this patch wants to achieve
 on first reading. Patch text is unchanged.

 compat/mingw.h | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index 034fff9479..d2168c1e5e 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -395,7 +395,11 @@ HANDLE winansi_get_osfhandle(int fd);
(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 int mingw_is_dir_sep(int c)
+{
+   return c == '/' || c == '\\';
+}
+#define is_dir_sep mingw_is_dir_sep
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
char *ret = NULL;
-- 
2.13.0.55.g17b7d13330

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 22.05.2017 um 13:59 schrieb Johannes Schindelin:

On Sat, 20 May 2017, Johannes Sixt wrote:

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
 From C:\Temp\gittest
  * branchHEAD   -> FETCH_HEAD

The fix is in the second patch; the first patch is a
preparation that allows to write the fix in my preferred style.


Thank you!

Maybe you want to accompany these patches with a simple test case that
uses e.g. ls-remote on $(pwd | tr / )?


Actually, no, I don't want to. It would have to be protected by MINGW, 
and I don't want to burden us (and here I mean Windows folks) with a 
check for a cosmetic deficiency. (Shell scripts, slow forks, yadda, 
yadda...)


-- Hannes

Re: [PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

Am 22.05.2017 um 16:01 schrieb Johannes Schindelin:

On Mon, 22 May 2017, stefan.na...@atlas-elektronik.com wrote:

Am 20.05.2017 um 08:28 schrieb Johannes Sixt:

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
 From C:\Temp\gittest
  * branchHEAD   -> FETCH_HEAD


What is the exact precondition to get such a warning ?

Just wondering, because I'm not able to reproduce that warning
(with Git4win version 2.13.0.windows.1).


I had tested this also, and came to the conclusion that only Hannes'
MSys-based custom version is affected that is much closer to git.git's
`master` than to Git for Windows' fork.


In this case, the warning occurs because I build with nd/fopen-errors.

Which explains why I observed it only recently even though I fetch from 
Windows paths regularly.


-- Hannes

Re: [PATCH] mingw: simplify PATH handling

[Johannes Sixt]

Am 20.05.2017 um 17:29 schrieb René Scharfe:

-static char *path_lookup(const char *cmd, char **path, int exe_only)
+static char *path_lookup(const char *cmd, int exe_only)
  {
+   const char *path;
char *prog = NULL;
int len = strlen(cmd);
int isexe = len >= 4 && !strcasecmp(cmd+len-4, ".exe");
  
  	if (strchr(cmd, '/') || strchr(cmd, '\\'))

-   prog = xstrdup(cmd);
+   return xstrdup(cmd);
  
-	while (!prog && *path)

-   prog = lookup_prog(*path++, cmd, isexe, exe_only);
+   path = mingw_getenv("PATH");
+   if (!path)
+   return NULL;
+
+   for (; !prog && *path; path++) {
+   const char *sep = strchrnul(path, ';');
+   if (sep == path)
+   continue;
+   prog = lookup_prog(path, sep - path, cmd, isexe, exe_only);
+   path = sep;
+   }


The loop termination does not work here. When the final PATH component 
is investigated, sep points to the NUL. This pointer is assigned to 
path, which is incremented and now points one past NUL. Then the loop 
condition (*path) accesses the char behind NUL.


  
  	return prog;

  }
@@ -1569,13 +1527,10 @@ static pid_t mingw_spawnve_fd(const char *cmd, const 
char **argv, char **deltaen
}
  
  	if (getenv("GIT_STRACE_COMMANDS")) {

-   char **path = get_path_split();
-   char *p = path_lookup("strace.exe", path, 1);
+   char *p = path_lookup("strace.exe", 1);
if (!p) {
-   free_path_split(path);
return error("strace not found!");
}
-   free_path_split(path);
if (xutftowcs_path(wcmd, p) < 0) {
free(p);
return -1;


Upstream does not have this hunk.

Otherwise, good catch!

-- Hannes

Re: [Bug] git branch -v has problems with carriage returns

[Johannes Sixt]

Am 19.05.2017 um 23:55 schrieb Atousa Duprat:

I have tried to repro this issue but git goes out of its way to store
the commit messages using unix end-of-line format.
I think that git itself cannot create a repo exhibiting this problem.


Here is a recipe to reproduce the error:

  git init
  git commit --allow-empty -m initial
  git branch crlf $(printf '%s\r\n' subject '' line3_long line4 |
   git commit-tree HEAD:)

The reason for the "bug" is obviously that a line having CR in addition 
to LF is not "an empty line". Consequently, the second line is not 
treated as a separator between subject and body, whereupon Git 
concatenates all lines into one large subject line. This strips the LFs 
but leaves the CRs in tact, which, when printed on a terminal move the 
cursor to the beginning of the line, so that text after the CRs 
overwrites what is already in the terminal.


This is just to give you a head start. I'm not going to look into this.

-- Hannes


If I do `git branch -v` with such a subject line somehow the third and
second line get combined before the hash. Example:

$ git branch -v
See merge request ! temp space 84e18d22fd Merge branch
'feature-XXX' into 'develop'
#

Before git v2.13.0 `git branch -v` worked completely normal.

[PATCH 0/2] Fix warnings on access of a remote with Windows paths

[Johannes Sixt]

This small series fixes these warnings on Windows:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
>From C:\Temp\gittest
 * branchHEAD   -> FETCH_HEAD

The fix is in the second patch; the first patch is a
preparation that allows to write the fix in my preferred style.

Johannes Sixt (2):
  mingw.h: permit arguments with side effects for is_dir_sep
  Windows: do not treat a path with backslashes as a remote's nick name

 compat/mingw.h | 6 +-
 remote.c   | 5 -
 2 files changed, 9 insertions(+), 2 deletions(-)

-- 
2.13.0.55.g17b7d13330

[PATCH 2/2] Windows: do not treat a path with backslashes as a remote's nick name

[Johannes Sixt]

Fetching from a remote using a native Windows path produces these warnings:

C:\Temp\gittest>git fetch C:\Temp\gittest
warning: unable to access '.git/remotes/C:\Temp\gittest': Invalid argument
warning: unable to access '.git/branches/C:\Temp\gittest': Invalid argument
>From C:\Temp\gittest
 * branchHEAD   -> FETCH_HEAD

The functions that read the branches and remotes files are protected by
a valid_remote_nick() check. Its implementation does not take Windows
paths into account, so that the caller simply concatenates the paths,
leading to the error seen above.

Use is_dir_sep() to check for both slashes and backslashes on Windows.

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

diff --git a/remote.c b/remote.c
index 9584af366d..9501357c06 100644
--- a/remote.c
+++ b/remote.c
@@ -649,7 +649,10 @@ static int valid_remote_nick(const char *name)
 {
if (!name[0] || is_dot_or_dotdot(name))
return 0;
-   return !strchr(name, '/'); /* no slash */
+   while (*name)
+   if (is_dir_sep(*name++))/* no slash */
+   return 0;
+   return 1;
 }
 
 const char *remote_for_branch(struct branch *branch, int *explicit)
-- 
2.13.0.55.g17b7d13330

[PATCH 1/2] mingw.h: permit arguments with side effects for is_dir_sep

[Johannes Sixt]

The implementation of is_dir_sep in git-compat-util.h uses an inline
function. Use one also for the implementation in compat/mingw.h to support
non-trivial argument expressions.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 compat/mingw.h | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/compat/mingw.h b/compat/mingw.h
index cdc112526a..5e8447019b 100644
--- a/compat/mingw.h
+++ b/compat/mingw.h
@@ -398,7 +398,11 @@ HANDLE winansi_get_osfhandle(int fd);
(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 int mingw_is_dir_sep(int c)
+{
+   return c == '/' || c == '\\';
+}
+#define is_dir_sep mingw_is_dir_sep
 static inline char *mingw_find_last_dir_sep(const char *path)
 {
char *ret = NULL;
-- 
2.13.0.55.g17b7d13330

Re: [PATCH 00/23] Prepare to separate out a packed_ref_store

[Johannes Sixt]

Am 17.05.2017 um 14:05 schrieb Michael Haggerty:

This patch series is the next leg on a journey towards reading
`packed-refs` using `mmap()`, the most interesting aspect of which is
that we will often be able to avoid having to read the whole
`packed-refs` file if we only need a subset of references.


Which features of mmap() are you going to use? We have only MAP_PRIVATE 
on Windows. We do emulate PROT_WRITE (without PROT_READ), but I don't 
think that code is exercised anywhere. See also compat/win32mmap.c.


-- Hannes

Re: t5400 failure on Windows

[Johannes Sixt]

Am 17.05.2017 um 07:44 schrieb Jeff King:
> I wonder if there's a way we could convince the trace for the two
> programs to go to separate locations. We don't care about receive-pack's
> trace at all. So maybe:

This works. Below it is with a commit message. I'm unsure about the
sign-off procedure, though.

>> * refs/files-backend.c: There are uses in functions open_or_create_logfile()
>> and log_ref_setup(). Sounds like it is in reflogs. Sounds like this is about
>> reflogs. If there are concurrent accesses, there is a danger that a reflog
>> is not recorded correctly. Then reflogs may be missing and objects may be
>> pruned earlier than expected. That's something to worry about, but I would
>> not count it as mission critical.

Of course, the reflog can also be corrupted, but:

> We should always hold the matching ref lock already when modifying a
> reflog. I seem to recall there was a case that missed this (I think
> maybe modifying the HEAD reflog without holding HEAD.lock), but it was
> fixed in the last few versions.

we should be fairly safe then.

 8< 
From: Jeff King <p...@peff.net>
Subject: [PATCH jk/alternate-ref-optim] t5400: avoid concurrent writes into a 
trace file

One test in t5400 examines the packet exchange between git-push and
git-receive-pack. The latter inherits the GIT_TRACE_PACKET environment
variable, so that both processes dump trace data into the same file
concurrently. This should not be a problem because the trace file is
opened with O_APPEND.

On Windows, however, O_APPEND is not atomic as it should be: it is
emulated as lseek(SEEK_END) followed by write(). For this reason, the
test is unreliable: it can happen that one process overwrites a line
that was just written by the other process. As a consequence, the test
sometimes does not find one or another line that is expected (and it is
also successful occasionally).

The test case is actually only interested in the output of git-push.
To ensure that only git-push writes to the trace file, override the
receive-pack command such that it does not even open the trace file.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 t/t5400-send-pack.sh | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/t/t5400-send-pack.sh b/t/t5400-send-pack.sh
index 3331e0f534..d375d7110d 100755
--- a/t/t5400-send-pack.sh
+++ b/t/t5400-send-pack.sh
@@ -288,7 +288,10 @@ test_expect_success 'receive-pack de-dupes .have lines' '
$shared .have
EOF
 
-   GIT_TRACE_PACKET=$(pwd)/trace git push fork HEAD:foo &&
+   GIT_TRACE_PACKET=$(pwd)/trace \
+   git push \
+   --receive-pack="unset GIT_TRACE_PACKET; git-receive-pack" \
+   fork HEAD:foo &&
extract_ref_advertisement refs &&
test_cmp expect refs
 '
-- 
2.13.0.55.g17b7d13330

Re: [PATCH v1 2/5] Teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

[Johannes Sixt]

Am 17.05.2017 um 16:26 schrieb Ben Peart:

On 5/16/2017 3:13 PM, Johannes Sixt wrote:

Am 16.05.2017 um 19:17 schrieb Ben Peart:

OK, now I'm confused as to the best path for adding a get_be64.  This
one is trivial:

#define get_be64(p)ntohll(*(uint64_t *)(p))


I cringe when I see a cast like this. Unless you can guarantee that p is
char* (bare or signed or unsigned), you fall pray to strict aliasing
violations, aka undefined behavior. And I'm not even mentioning correct
alignment, yet.


Note, this macro is only used where the CPU architecture is OK with 
unaligned memory access.


I'm not worried about the unaligned memory access: It either works, or 
we get a SIGBUS. The undefined behavior is more worrisome because the 
code may work or not, and we can never be sure which it is.


-- Hannes

Re: [PATCH v1 2/5] Teach git to optionally utilize a file system monitor to speed up detecting new or changed files.

[Johannes Sixt]

Am 16.05.2017 um 19:17 schrieb Ben Peart:
OK, now I'm confused as to the best path for adding a get_be64.  This 
one is trivial:


#define get_be64(p)ntohll(*(uint64_t *)(p))


I cringe when I see a cast like this. Unless you can guarantee that p is 
char* (bare or signed or unsigned), you fall pray to strict aliasing 
violations, aka undefined behavior. And I'm not even mentioning correct 
alignment, yet.


-- Hannes

Re: t5400 failure on Windows

[Johannes Sixt]

Am 16.05.2017 um 00:24 schrieb Jeff King:

   4. There is something racy and unportable about both programs writing
  to the same trace file. It's opened with O_APPEND, which means that
  each write should atomically position the pointer at the end of the
  file. Is it possible there's a problem with that in the way
  O_APPEND works on Windows?

  If that was the case, though, I'd generally expect a sheared write
  or a partial overwrite. The two origin/HEAD lines from the two
  programs are the exact same length, but I'd find it more likely for
  the overwrite to happen with one of the follow-on lines.


Good guesswork! O_APPEND is not atomic on Windows, in particular, it is 
emulated as lseek(SEEK_END) followed by write().


I ran the test a few more times, and it fails in different ways 
(different missing lines) and also succeeds in a minority of the cases.


Windows is capable of writing atomically in the way O_APPEND requires 
(keyword: FILE_APPEND_DATA), but I do not see a way to wedge this into 
the emulation layer. For the time being, I think I have to skip the test 
case.


The question remains how endangered our uses of O_APPEND are when the 
POSIX semantics are not obeyed precisely:


* trace.c: It is about debugging. Not critical.

* sequencer.c: It writes the "done" file. No concurrent accesses are 
expected: Not critial.


* refs/files-backend.c: There are uses in functions 
open_or_create_logfile() and log_ref_setup(). Sounds like it is in 
reflogs. Sounds like this is about reflogs. If there are concurrent 
accesses, there is a danger that a reflog is not recorded correctly. 
Then reflogs may be missing and objects may be pruned earlier than 
expected. That's something to worry about, but I would not count it as 
mission critical.


-- Hannes

t5400 failure on Windows

[Johannes Sixt]

I observe the following failure on Windws with origin/next, in
t5400-send-pack.sh

+++ diff -u expect refs
--- expect  Mon May 15 06:54:59 2017
+++ refsMon May 15 06:54:59 2017
@@ -1,4 +1,3 @@
 5285e1710002a06a379056b0d21357a999f3c642 refs/heads/master
-5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/HEAD
 5285e1710002a06a379056b0d21357a999f3c642 refs/remotes/origin/master
 53d9066ca10f2e103b33caf3a16a723553c33b00 .have
error: last command exited with $?=1
not ok 16 - receive-pack de-dupes .have lines


The trace file looks like this:

 trace 
packet: receive-pack> 5285e1710002a06a379056b0d21357a999f3c642 
refs/heads/master\0report-status delete-refs side-band-64k quiet atomic 
ofs-delta agent=git/2.13.0.497.g5af12421b0
packet: receive-pack> 5285e1710002a06a379056b0d21357a999f3c642 
refs/remotes/origin/HEAD
packet: push< 5285e1710002a06a379056b0d21357a999f3c642 
refs/heads/master\0report-status delete-refs side-band-64k quiet atomic 
ofs-delta agent=git/2.13.0.497.g5af12421b0
packet: receive-pack> 5285e1710002a06a379056b0d21357a999f3c642 
refs/remotes/origin/master
packet: push< 5285e1710002a06a379056b0d21357a999f3c642 
refs/remotes/origin/master
packet: receive-pack> 53d9066ca10f2e103b33caf3a16a723553c33b00 .have
packet: push< 53d9066ca10f2e103b33caf3a16a723553c33b00 .have
packet: receive-pack> 
packet: push< 
packet: push>  
b1a1c97e94b6388c108e195d28a3e89f00c81698 refs/heads/foo\0 report-status 
agent=git/2.13.0.497.g5af12421b0
packet: push> 
packet: receive-pack<  
b1a1c97e94b6388c108e195d28a3e89f00c81698 refs/heads/foo\0 report-status 
agent=git/2.13.0.497.g5af12421b0
packet: receive-pack< 
packet: receive-pack> unpack ok
packet: receive-pack> ok refs/heads/foo
packet: receive-pack> 
packet: push< unpack ok
packet: push< ok refs/heads/foo
packet: push< 
 end of trace 

Where should I start looking? On a Linux box, the test case does
produce an additional line

packet: push< 5285e1710002a06a379056b0d21357a999f3c642 
refs/remotes/origin/HEAD

in the trace file. I also do not see that any tests would be skipped
on Windows. (But I forgot to check whether refs/remotes/origin/HEAD
is present in any of the repositories.)

-- Hannes

Re: [PATCH v4 1/3] usability: don't ask questions if no reply is required

[Johannes Sixt]

Am 13.05.2017 um 00:36 schrieb Junio C Hamano:

Thanks, all three patches look good.  Will queue.

Let's merge them to 'next' soonish and eventually down to 'master'
and 'maint'.


The patches change translated strings. You should probably wait for an 
update of their translations before you release a maintenance version 
with these changes.


-- Hannes

Re: [PATCH] C style: use standard style for "TRANSLATORS" comments

[Johannes Sixt]

Am 11.05.2017 um 23:20 schrieb Ævar Arnfjörð Bjarmason:

diff --git a/builtin/notes.c b/builtin/notes.c
index 7b891471c4..fb856e53b6 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -340,8 +340,10 @@ static struct notes_tree *init_notes_check(const char 
*subcommand,
  
  	ref = (flags & NOTES_INIT_WRITABLE) ? t->update_ref : t->ref;

if (!starts_with(ref, "refs/notes/"))
-   /* TRANSLATORS: the first %s will be replaced by a
-  git notes command: 'add', 'merge', 'remove', etc.*/
+   /*
+* TRANSLATORS: the first %s will be replaced by a git
+* notes command: 'add', 'merge', 'remove', etc.
+*/


Rewrapping lines is generally frowned upon because it makes it difficult 
to see whether something was changed. Keeping the line wrapping will 
also reduce the noise in the next .pot commit, I think (not sure if that 
is a worthwhile goal, though).



I hate it when J. Random Developer insists in a particular line length 
and when they have their editor do the wrapping, logical entities are 
suddenly split into two lines: it is "git notes", one logical thing; not 
two words "git" "notes" that happen to occur in sequence.



-- Hannes

Re: [PATCH 8/8] pathspec: convert parse_pathspec to take an index

[Johannes Sixt]

Am 11.05.2017 um 03:48 schrieb Junio C Hamano:

But perhaps you are right---it may be wrong for the contents of the
current index (or any index) to affect how a pathspec element is
parsed in the first place.  If the current code (before this series)
uses the_index only for error checking, we may want to separate that
out of the parse_pathspec() callchain, so that it does not even look
at any index (not just the_index).  And that may be a better change
overall.


Just a reminder: if core.ignoreCase is set, the variant of a path in the 
index takes precedence over the variant found in the working tree. 
Hence, pathspec must be matched against the index that corresponds to 
the working tree. I guess that's the_index.


-- Hannes

Re: [PATCH] fixup! use perl instead of sed

[Johannes Sixt]

Am 09.05.2017 um 19:00 schrieb Ævar Arnfjörð Bjarmason:

Finally, you can just use -i like you did with sed, no need for the tempfile:


Nope. Some implementations of perl attempt to remove the file that it 
has just opened. That doesn't work on Windows. You have to supply a 
backup file name as in `perl -i.bak ...` :-(




 $ echo hibar >push
 $ perl -pi -e 's/([^ ])bar/$1baz/' push
 $ cat push
 hibaz


-- Hannes