[PATCH] git-rebase.txt: update note about directory rename detection and am

[Johannes Sixt]

From: Elijah Newren 

In commit 6aba117d5cf7 ("am: avoid directory rename detection when
calling recursive merge machinery", 2018-08-29), the git-rebase manpage
probably should have also been updated to note the stronger
incompatibility between git-am and directory rename detection.  Update
it now.

Signed-off-by: Elijah Newren 
Signed-off-by: Johannes Sixt 
---
 Documentation/git-rebase.txt | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 41631df6e4..7bea21e8e3 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -569,8 +569,9 @@ it to keep commits that started empty.
 Directory rename detection
 ~~

-The merge and interactive backends work fine with
-directory rename detection.  The am backend sometimes does not.
+Directory rename heuristics are enabled in the merge and interactive
+backends.  Due to the lack of accurate tree information, directory
+rename detection is disabled in the am backend.

 include::merge-strategies.txt[]

--
2.19.1.1133.g2dd3d172d2

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 05.12.18 um 20:29 schrieb Frank Schäfer:


Am 02.12.18 um 22:22 schrieb Johannes Sixt:

Am 02.12.18 um 20:31 schrieb Frank Schäfer:

With other words:
"If CR comes immediately before a LF, do the following with *all* lines:
 after the CR if eol=lf but do not  after the CR if
eol=crlf."


Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but
the user wants to see them, then they are using the wrong pager or the
wrong pager settings.

AFAIU Junios explanation it's not the pagers fault.


Then Junio and I are in disagreement. IMO, Git does not have to be more 
clever than the pager when it comes to presentation of text.



As far as I am concerned, I do not have any of my files marked as
eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git
insert  between CR and LF would do the wrong thing for me.


But doing the same thing in added lines is doing the right thing for you ?


Yes, I think so. As long as I'm not telling Git that my files are CRLF 
when they actual are, then the CR before LF is a whitespace error. 
Nevertheless, I do *NOT* want Git to outwit my pager by inserting 
 between CR and LF all the time so that it is forced to treat the 
lone CR like a control character that is to be made visible.



Or are you suggesting to fix the behavior of added lines instead ?
In any case, inconsistent behavior is not what we want.


I'm suggesting that users who knowingly store CRLF files in the 
database, but do not want to see ^M in added lines have to use 
whitespace=cr-at-eol and that's all. I do not see inconsistency.


-- Hannes

Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences

[Johannes Sixt]

Am 05.12.18 um 16:37 schrieb Elijah Newren:

On Tue, Dec 4, 2018 at 11:40 PM Junio C Hamano  wrote:


Johannes Sixt  writes:


Please let me deposit my objection. This paragraph is not the right
place to explain what directory renme detection is and how it works
under the hood. "works fine" in the original text is the right phrase
here; if there is concern that this induces expectations that cannot
be met, throw in the word "heuristics".

Such as:
Directory rename heuristics work fine in the merge and interactive
backends. It does not in the am backend because...


OK, good enough, I guess.  Or just s/work fine/is enabled/.


So...

Directory rename heuristics are enabled in the merge and interactive
backends. They are not in the am backend because it operates on
"fake ancestors" that involve trees containing only the files modified
in the patch.  Due to the lack of accurate tree information, directory
rename detection is disabled for the am backend.


OK with me. And also if you drop sentence no.2 and keep just no.1 and no.3.

Thanks,
-- Hannes

Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences

[Johannes Sixt]

Am 05.12.18 um 07:20 schrieb Elijah Newren:

On Tue, Dec 4, 2018 at 7:54 PM Junio C Hamano  wrote:


Elijah Newren  writes:


Gah, when I was rebasing on your patch I adopted your sentence rewrite
but forgot to remove the "sometimes".  Thanks for catching; correction:




-- 8< --
Subject: [PATCH v2] git-rebase.txt: update note about directory rename
  detection and am

In commit 6aba117d5cf7 ("am: avoid directory rename detection when
calling recursive merge machinery", 2018-08-29), the git-rebase manpage
probably should have also been updated to note the stronger
incompatibility between git-am and directory rename detection.  Update
it now.

Signed-off-by: Elijah Newren 
---
  Documentation/git-rebase.txt | 8 ++--
  1 file changed, 6 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 41631df6e4..ef76cccf3f 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -569,8 +569,12 @@ it to keep commits that started empty.
  Directory rename detection
  ~~

-The merge and interactive backends work fine with
-directory rename detection.  The am backend sometimes does not.
+The merge and interactive backends work fine with directory rename


I am not sure "work fine" a fair and correct label, as rename is
always heuristic.

 The "directory rename detection" heuristic in "merge" and the
 "interactive" backends can take what happened to paths in the
 same directory into account when deciding if a disappeared path
 was "renamed" and to which other path.  The heuristic produces
 incorrect result when the information given is only about
 changed paths, which is why it is disabled when using the "am"
 backend.

perhaps.


The general idea sounds good.  Does adding a few more details help
with understanding, or is it more of an information overload?  I'm
thinking of something like:

  The "directory rename detection" heuristic in the "merge" and
  "interactive" backends can take what happened to paths in the
  same directory on the other side of history into account when
  deciding whether a new path in that directory should instead be
  moved elsewhere.  The heuristic produces incorrect results when
  the only information available is about files which were changed
  on the side of history being rebased, which is why directory
  rename detection is disabled when using the "am" backend.


Please let me deposit my objection. This paragraph is not the right 
place to explain what directory renme detection is and how it works 
under the hood. "works fine" in the original text is the right phrase 
here; if there is concern that this induces expectations that cannot be 
met, throw in the word "heuristics".


Such as:
   Directory rename heuristics work fine in the merge and interactive
   backends. It does not in the am backend because...

-- Hannes

Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences

[Johannes Sixt]

Am 04.12.18 um 22:24 schrieb Elijah Newren:

+  The am backend sometimes does not because it operates on
+"fake ancestors" that involve trees containing only the files modified
+in the patch.  Without accurate tree information, directory rename
+detection cannot safely operate and is thus turned off in the am
+backend.


Directory rename detection does not work sometimes. That is, it works 
most of the time. But how can that be the case when it is turned off?


-- Hannes

Re: [PATCH] rebase docs: fix incorrect format of the section Behavioral Differences

[Johannes Sixt]

Am 03.12.18 um 21:42 schrieb Martin Ågren:

On Mon, 3 Dec 2018 at 18:35, Johannes Sixt  wrote:

I actually did not test the result, because I don't have the
infrastructure.


I've tested with asciidoc and Asciidoctor, html and man-page. Looks
good.


Thank you so much!

-- Hannes

[PATCH] rebase docs: fix incorrect format of the section Behavioral Differences

[Johannes Sixt]

The text body of section Behavioral Differences is typeset as code,
but should be regular text. Remove the indentation to achieve that.

While here, prettify the language:

- use "the x backend" instead of "x-based rebase";
- use present tense instead of future tense;

and use subsections instead of a list.

Signed-off-by: Johannes Sixt 
---
Cf. https://git-scm.com/docs/git-rebase#_behavioral_differences

I actually did not test the result, because I don't have the
infrastructure.

The sentence "The am backend sometimes does not" is not very useful,
but is not my fault ;) It would be great if it could be made more
specific, but I do not know the details.

 Documentation/git-rebase.txt | 30 +-
 1 file changed, 17 insertions(+), 13 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index 80793bad8d..dff17b3178 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -550,24 +550,28 @@ Other incompatible flag pairs:
 BEHAVIORAL DIFFERENCES
 ---
 
- * empty commits:
+There are some subtle differences how the backends behave.
 
-am-based rebase will drop any "empty" commits, whether the
-commit started empty (had no changes relative to its parent to
-start with) or ended empty (all changes were already applied
-upstream in other commits).
+Empty commits
+~
+
+The am backend drops any "empty" commits, regardless of whether the
+commit started empty (had no changes relative to its parent to
+start with) or ended empty (all changes were already applied
+upstream in other commits).
 
-merge-based rebase does the same.
+The merge backend does the same.
 
-interactive-based rebase will by default drop commits that
-started empty and halt if it hits a commit that ended up empty.
-The `--keep-empty` option exists for interactive rebases to allow
-it to keep commits that started empty.
+The interactive backend drops commits by default that
+started empty and halts if it hits a commit that ended up empty.
+The `--keep-empty` option exists for the interactive backend to allow
+it to keep commits that started empty.
 
-  * directory rename detection:
+Directory rename detection
+~~
 
-merge-based and interactive-based rebases work fine with
-directory rename detection.  am-based rebases sometimes do not.
+The merge and interactive backends work fine with
+directory rename detection.  The am backend sometimes does not.
 
 include::merge-strategies.txt[]
 
-- 
2.19.1.1133.g2dd3d172d2

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 02.12.18 um 20:31 schrieb Frank Schäfer:

Am 29.11.18 um 03:11 schrieb Junio C Hamano:
[...]

This was misspoken a bit.  Let's revise it to

When producing a colored output (not limited to whitespace
error coloring of diff output) for contents that are not
marked as eol=crlf (and other historical means), insert
 before a CR that comes immediately before a LF.

You mean
  ...
   *after* a CR that comes immediately before a LF."


OK, AFAICS this produces the desired output in all cases if eol=lf.

Now what about the case eol=crlf ?
Keeping the current behavior of '-' lines is correct.
But shouldn't ^M now be suppressed in '+' lines for a consistent behavior ?


That can be achieved with whitespace=cr-at-eol.


With other words:
"If CR comes immediately before a LF, do the following with *all* lines:
 after the CR if eol=lf but do not  after the CR if eol=crlf."


Why? It is the pager's duty to highlight CR, IMO. If it doesn't, but the 
user wants to see them, then they are using the wrong pager or the wrong 
pager settings.


As far as I am concerned, I do not have any of my files marked as 
eol=crlf, but I do *not* want to see ^M in the pager. I.e., having git 
insert  between CR and LF would do the wrong thing for me.


-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 27.11.18 um 00:31 schrieb Junio C Hamano:

Johannes Sixt  writes:

Am 26.11.18 um 04:04 schrieb Junio C Hamano:
... this goes too far, IMO. It is the pager's task to decode control
characters.


It was tongue-in-cheek suggestion to split a CR into caret-em on our
end, but we'd get essentially the same visual effect if we added a
rule:

When producing a colored output (not limited to whitespace
error coloring of diff output), insert  before a CR
that comes immediately before a LF.

Then, what Frank saw in the troublesome output would become

 -something  CR  LF
 +something_new   CR  LF

and we'll let the existing pager+terminal magic turn that trailing
CR on the preimage line into caret-em, just like the trailing CR on
the postimage line is already shown as caret-em with the current
output.


I wouldn't want that to happen for all output (context lines, - lines, + 
lines): I really am not interested to see all the CRs in my CRLF files.



And a good thing is that I do not think that new rule is doing any
decode of control chars on our end.  We are just producing colored
output normally.


But we already have it, as Brian pointed out:

   git diff --ws-error-highlight=old,new

or by setting diff.wsErrorHighlight accordingly.

-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 26.11.18 um 04:04 schrieb Junio C Hamano:

It appears to me that what Frank sees is not "^M highlighted for
whitespace breakage appears only on postimage lines, while ^M is
shown but not highlighted on preimage lines".  My reading of the
above is "Not just without highlight, ^M is just *NOT* shown on the
preimage line".

That does not sound right.  I would understand it if both lines
showed ^M at the end, and only the one on the postimage line had it
highlighted as a trailing-whitespace.


I agree that this is a (small?) weakness. But...


When we are producing a colored output, we know we are *not* writing
for machines, so one way to do it would be to turn CR at the end of
the line into two letter "^" "M" sequence on our end, without relying
on the terminal and/or the pager.  I dunno.


... this goes too far, IMO. It is the pager's task to decode control 
characters.


-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 25.11.18 um 15:03 schrieb Frank Schäfer:

Am 24.11.18 um 23:07 schrieb Johannes Sixt:

I don't think that there is anything to fix. If you have a file with
CRLF in it, but you did not declare to Git that CRLF is the expected
end-of-line indicator, then the CR *is* trailing whitespace (because
the line ends at LF), and 'git diff' highlights it.


Sure, it's correct to highlight it.
But it doesn't highlight it in removed lines, just in added lines.
I can see no good reason why removed and added lines should be treated
differently.


But incorrect whitespace is never highlighted in removed lines, why 
should CR be an exception?



1) If CR+LF line termination is used in a file, changing the content of
a line (but not its termination) currently produces a diff like

-something
+something_new^M

which causes the user to think he has changed the line ending (added a
CR) although he didn't.


But this is not limited to CR at EOL:

-something
+something_new

will also show the incorrect whitespace highlighted only for the + line.


2) If someone/something unintentionally changes the line termination
from CR+LF to LF, it doesn't show up in the diff:

-something
+something


Same here for other cases, for example

-something
+something

will not have on obvious indicator that whitespace was corrected.

If you are worried about a change in EOL style, you should better listen 
to your other tools. Either it is important, or it is not. If it is, 
they will report it to you. If it isn't, why care?


-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 24.11.18 um 15:51 schrieb Frank Schäfer:

Am 23.11.18 um 22:47 schrieb Johannes Sixt:

Am 23.11.18 um 19:19 schrieb Frank Schäfer:

The CR marker ^M doesn't show up in '-' lines of diffs when the ending
of the removed line is CR+LF.
It shows up as expected in '-' lines when the ending of the removed line
is CR only.
It also always shows up as expected in '+' lines.


Is your repository configured to (1) highlight whitespace errors in
diff output and (2) to leave CRLF alone in text files?

I'm using the default configuration, so whitespace is set to
trailing-space, but cr-at-eol is not set.



If so, then it is just a side-effect of this combination, an illusion,
so to say: The CR in the CRLF combo is trailing whitespace. The 'git
diff' marks it by inserting an escape sequence to switch the color
before ^M and another escape sequence to reset to color after ^M. This
breaks the CRLF combination apart, so that the pager does not process
it as a combined CRLF sequence; it displays the lone CR as ^M.

Urghh... so that needs to be fixed.
Why does it work correctly with '+' lines ?


I don't think that there is anything to fix. If you have a file with 
CRLF in it, but you did not declare to Git that CRLF is the expected 
end-of-line indicator, then the CR *is* trailing whitespace (because the 
line ends at LF), and 'git diff' highlights it.


-- Hannes

Re: BUG: CR marker ^M doesn't show up in '-' lines of diffs when the ending of the removed line is CR+LF

[Johannes Sixt]

Am 23.11.18 um 19:19 schrieb Frank Schäfer:

The CR marker ^M doesn't show up in '-' lines of diffs when the ending
of the removed line is CR+LF.
It shows up as expected in '-' lines when the ending of the removed line
is CR only.
It also always shows up as expected in '+' lines.


Is your repository configured to (1) highlight whitespace errors in diff 
output and (2) to leave CRLF alone in text files?


If so, then it is just a side-effect of this combination, an illusion, 
so to say: The CR in the CRLF combo is trailing whitespace. The 'git 
diff' marks it by inserting an escape sequence to switch the color 
before ^M and another escape sequence to reset to color after ^M. This 
breaks the CRLF combination apart, so that the pager does not process it 
as a combined CRLF sequence; it displays the lone CR as ^M.


It is easy to achieve the opposite effect, i.e., that ^M is not 
displayed. For example with these lines in .git/info/attributes or 
.gitattributes:


*.cpp 
whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab

*.h whitespace=trailing-space,cr-at-eol,indent-with-non-tab,space-before-tab

Note the cr-at-eol. (There may be shorter versions to achieve a similar 
effect.)


-- Hannes

Re: [PATCH 1/1] mingw: replace an obsolete link with the superseding one

[Johannes Sixt]

Am 15.11.18 um 12:22 schrieb Johannes Schindelin via GitGitGadget:

From: Johannes Schindelin 

The MSDN documentation has been superseded by Microsoft Docs (which is
backed by a repository on GitHub containing many, many files in Markdown
format).

Signed-off-by: Johannes Schindelin 
---
  compat/mingw.c | 4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index d2f4fabb44..9e42b0ee26 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -1028,8 +1028,8 @@ char *mingw_getcwd(char *pointer, int len)
  }
  
  /*

- * See http://msdn2.microsoft.com/en-us/library/17w5ykft(vs.71).aspx
- * (Parsing C++ Command-Line Arguments)
+ * See "Parsing C++ Command-Line Arguments" at Microsoft's Docs:
+ * https://docs.microsoft.com/en-us/cpp/cpp/parsing-cpp-command-line-arguments
   */
  static const char *quote_arg(const char *arg)
  {



Thank you! That's much better than the original obfuscated page name.

-- Hannes

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

[Johannes Sixt]

Am 07.11.18 um 21:41 schrieb Jeff King:

On Wed, Nov 07, 2018 at 07:52:28PM +0100, Johannes Sixt wrote:

Do I understand correctly, that you use a leading slash as an indicator to
construct a path relative to system_path(). How about a "reserved" user
name? For example,

   [http] sslcert = ~system_path/what/ever

although a more unique name, perhaps with some punctuation, may be
desirable.


It's syntactically a bit further afield, but something like:

   [http]
   sslcert = $RUNTIME_PREFIX/what/ever

would make sense to me, and is a bit less subtle than the fake user. I
don't know if that would confuse people into thinking that we
interpolate arbitrary environment variables, though.


The expansion of a fake user name would have to go in 
expand_user_path(), a fake variable name would have to be expanded 
elsewhere. Now, Dscho mentions in the cover letter that his patch has 
already seen some field testing ("has been 'in production' for a good 
while"). So, we have gained some confidence that the point where the 
substitution happens, in expand_user_path(), is suitable. Therefore, I 
have slight preference for a fake user.


-- Hannes

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

[Johannes Sixt]

Am 07.11.18 um 12:23 schrieb Johannes Schindelin:

On Tue, 6 Nov 2018, Johannes Sixt wrote:


Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget:
Even if a path looks like a POSIX paths, i.e. it starts with a directory
separator, but not with drive-letter-colon, it still has a particular meaning,
namely (as far as I know) that the path is anchored at the root of the drive
of the current working directory.

If a user specifies such a path on Windows, and it must undergo
expand_user_path(), then that is a user error, or the user knows what they are
doing.

I think it is wrong to interpret such a path as relative to some random other
directory, like this patch seems to do.


Okay, now we know everything you find wrong with the current patch. Do you
have any suggestion how to make it right? I.e. what would you suggest as a
way to specify in a gitconfig in a portable Git where the certificate
bundle is?


Ah, so your actual problem is quite a different one!

Do I understand correctly, that you use a leading slash as an indicator 
to construct a path relative to system_path(). How about a "reserved" 
user name? For example,


  [http] sslcert = ~system_path/what/ever

although a more unique name, perhaps with some punctuation, may be 
desirable.


-- Hannes

Re: [PATCH 1/1] Windows: force-recompile git.res for differing architectures

[Johannes Sixt]

Am 07.11.18 um 02:32 schrieb Junio C Hamano:

Johannes Sixt  writes:

On Linux, when I recompile for a different architecture, CFLAGS would
change, so I would have thought that GIT-CFLAGS were the natural
choice for a dependency. Don't they change in this case on Windows,
too?


Depending on GIT-CFLAGS would have a better chance of being safe, I
guess, even though it can at times be overly safe, than GIT-PREFIX,
I suspect.  As a single user target in Makefile, which is only used
by Dscho, who intends to stick to /mingw(32|64)/ convention til the
end of time, I think the posted patch is OK, though.


I think that it's not only Dscho who uses the target (my build 
environment, for example, is different from Dscho's and compiles 
git.res, too). But since the patch helps him most and doesn't hurt 
others, it is good to go. No objection from my side.


-- Hannes

Re: [PATCH 1/1] Windows: force-recompile git.res for differing architectures

[Johannes Sixt]

Am 06.11.18 um 15:55 schrieb Johannes Schindelin via GitGitGadget:

From: Johannes Schindelin 

When git.rc is compiled into git.res, the result is actually dependent
on the architecture. That is, you cannot simply link a 32-bit git.res
into a 64-bit git.exe.

Therefore, to allow 32-bit and 64-bit builds in the same directory, we
let git.res depend on GIT-PREFIX so that it gets recompiled when
compiling for a different architecture (this works because the exec path
changes based on the architecture: /mingw32/libexec/git-core for 32-bit
and /mingw64/libexec/git-core for 64-bit).


On Linux, when I recompile for a different architecture, CFLAGS would 
change, so I would have thought that GIT-CFLAGS were the natural choice 
for a dependency. Don't they change in this case on Windows, too?




Signed-off-by: Johannes Schindelin 
---
  Makefile | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index bbfbb4292d..8375736c32 100644
--- a/Makefile
+++ b/Makefile
@@ -2110,7 +2110,7 @@ $(SCRIPT_LIB) : % : %.sh GIT-SCRIPT-DEFINES
$(QUIET_GEN)$(cmd_munge_script) && \
mv $@+ $@
  
-git.res: git.rc GIT-VERSION-FILE

+git.res: git.rc GIT-VERSION-FILE GIT-PREFIX
$(QUIET_RC)$(RC) \
  $(join -DMAJOR= -DMINOR= -DMICRO= -DPATCHLEVEL=, $(wordlist 1, 4, \
$(shell echo $(GIT_VERSION) 0 0 0 0 | tr '.a-zA-Z-' ' '))) \

Re: [PATCH 1/1] mingw: handle absolute paths in expand_user_path()

[Johannes Sixt]

Am 06.11.18 um 15:53 schrieb Johannes Schindelin via GitGitGadget:

From: Johannes Schindelin 

On Windows, an absolute POSIX path needs to be turned into a Windows
one.


If I were picky, I would say that in a pure Windows application there 
cannot be POSIX paths to begin with.


Even if a path looks like a POSIX paths, i.e. it starts with a directory 
separator, but not with drive-letter-colon, it still has a particular 
meaning, namely (as far as I know) that the path is anchored at the root 
of the drive of the current working directory.


If a user specifies such a path on Windows, and it must undergo 
expand_user_path(), then that is a user error, or the user knows what 
they are doing.


I think it is wrong to interpret such a path as relative to some random 
other directory, like this patch seems to do.




Signed-off-by: Johannes Schindelin 
---
  path.c | 5 +
  1 file changed, 5 insertions(+)

diff --git a/path.c b/path.c
index 34f0f98349..a72abf0e1f 100644
--- a/path.c
+++ b/path.c
@@ -11,6 +11,7 @@
  #include "path.h"
  #include "packfile.h"
  #include "object-store.h"
+#include "exec-cmd.h"
  
  static int get_st_mode_bits(const char *path, int *mode)

  {
@@ -709,6 +710,10 @@ char *expand_user_path(const char *path, int real_home)
  
  	if (path == NULL)

goto return_null;
+#ifdef __MINGW32__
+   if (path[0] == '/')
+   return system_path(path + 1);
+#endif
if (path[0] == '~') {
const char *first_slash = strchrnul(path, '/');
const char *username = path + 1;

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt]

Am 05.11.18 um 08:01 schrieb Johannes Sixt:

Am 05.11.18 um 00:26 schrieb Junio C Hamano:

OK, thanks.  It seems that the relative silence after this message
is a sign that the resulting patch after squashing is what everybody
is happey with?


I'm not 100% happy.
I see the patch is already in next. Never mind. The patch text is fine, 
I just wanted to modify the commit message a bit.


Thanks,
-- Hannes

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt]

Am 05.11.18 um 00:26 schrieb Junio C Hamano:

OK, thanks.  It seems that the relative silence after this message
is a sign that the resulting patch after squashing is what everybody
is happey with?


I'm not 100% happy. I'll resend a squashed patch, but it has to wait as 
I have to catch a train now.


Appologies for the sub-optimal submission process.

-- Hannes

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt]

Am 03.11.18 um 09:14 schrieb Carlo Arenas:

On Fri, Nov 2, 2018 at 9:44 AM Johannes Sixt  wrote:


+  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);


nitpick: cast to DWORD instead of int


No; timeout is of type int; after an explicit type cast we don't want to 
have another implicit conversion.


-- Hannes

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt]

Am 02.11.18 um 15:47 schrieb Steve Hoelzer:
> On Thu, Nov 1, 2018 at 5:22 AM Johannes Sixt  wrote:
>>
>> Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:
>>> @@ -614,7 +618,7 @@ restart:
>>>
>>>  if (!rc && orig_timeout && timeout != INFTIM)
>>>{
>>> -  elapsed = GetTickCount() - start;
>>> +  elapsed = (DWORD)(GetTickCount64() - start);
>>
>> AFAICS, this subtraction in the old code is the correct way to take
>> account of wrap-arounds in the tick count. The new code truncates the 64
>> bit difference to 32 bits; the result is exactly identical to a
>> difference computed from truncated 32 bit values, which is what we had
>> in the old code.
>>
>> IOW, there is no change in behavior. The statement "avoid wrap-around
>> issues" in the subject line is not correct. The patch's only effect is
>> that it removes Warning C28159.
>>
>> What is really needed is that all quantities in the calculations are
>> promoted to ULONGLONG. Unless, of course, we agree that a timeout of
>> more than 49 days cannot happen ;)
> 
> Yep, correct on all counts. I'm in favor of changing the commit message to
> only say that this patch removes Warning C28159.

How about this fixup instead?

 8< 
squash! poll: use GetTickCount64() to avoid wrap-around issues

The value of timeout starts as an int value, and for this reason it
cannot overflow unsigned long long aka ULONGLONG. The unsigned version
of this initial value is available in orig_timeout. The difference
(orig_timeout - elapsed) cannot wrap around because it is protected by
a conditional (as can be seen in the patch text). Hence, the ULONGLONG
difference can only have values that are smaller than the initial
timeout value and truncation to int cannot overflow.

Signed-off-by: Johannes Sixt 
---
 compat/poll/poll.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index 4abbfcb6a4..4459408c7d 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -452,7 +452,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
   static HANDLE hEvent;
   WSANETWORKEVENTS ev;
   HANDLE h, handle_array[FD_SETSIZE + 2];
-  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
+  DWORD ret, wait_timeout, nhandles, orig_timeout = 0;
   ULONGLONG start = 0;
   fd_set rfds, wfds, xfds;
   BOOL poll_again;
@@ -618,8 +618,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
 
   if (!rc && orig_timeout && timeout != INFTIM)
 {
-  elapsed = (DWORD)(GetTickCount64() - start);
-  timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
+  ULONGLONG elapsed = GetTickCount64() - start;
+  timeout = elapsed >= orig_timeout ? 0 : (int)(orig_timeout - elapsed);
 }
 
   if (!rc && timeout)
-- 
2.19.1.406.g1aa3f475f3

Re: [PATCH 3/3] tests: optionally skip `git rebase -p` tests

[Johannes Sixt]

Am 01.11.18 um 07:12 schrieb Junio C Hamano:

"Johannes Schindelin via GitGitGadget" 
writes:


The `--preserve-merges` mode of the `rebase` command is slated to be
deprecated soon, ...


Is everybody on board on this statement?  I vaguely recall that some
people wanted to have something different from what rebase-merges
does (e.g. wrt first-parent history), and extending perserve-merges
might be one way to do so.


Maybe you are referring to my proposals from a long time ago. My 
first-parent hack did not work very well, and I have changed my 
workflow. --preserve-merges is certainly not a feature that *I* would 
like to keep.


The important question is whether there are too many users of 
preserve-merges who would be hurt when it is removed. It is irrelevant 
whether preserve-merges is a good base for extensions because it can 
easily be resurrected if the need arises.



I do not mind at all if the way forward were to extend rebase-merges
for any future features.  To me, it is preferrable having to deal
with just one codepath than two written in different languages.

I just want to make sure we know everybody is on board the plan that
we will eventually remove preserve-merges, tell those who want to
use it to switch to rebase-merges, and we will extend rebase-merges
when they raise issues with it saying that they cannot do something
preserve-merges would have served them well with rebase-merges.


-- Hannes

Re: [PATCH 1/1] poll: use GetTickCount64() to avoid wrap-around issues

[Johannes Sixt]

Am 31.10.18 um 22:11 schrieb Steve Hoelzer via GitGitGadget:

From: Steve Hoelzer 

 From Visual Studio 2015 Code Analysis: Warning C28159 Consider using
'GetTickCount64' instead of 'GetTickCount'.

Reason: GetTickCount() overflows roughly every 49 days. Code that does
not take that into account can loop indefinitely. GetTickCount64()
operates on 64 bit values and does not have that problem.

Note: this patch has been carried in Git for Windows for almost two
years, but with a fallback for Windows XP, as the GetTickCount64()
function is only available on Windows Vista and later. However, in the
meantime we require Vista or later, hence we can drop that fallback.

Signed-off-by: Steve Hoelzer 
Signed-off-by: Johannes Schindelin 
---
  compat/poll/poll.c | 10 +++---
  1 file changed, 7 insertions(+), 3 deletions(-)

diff --git a/compat/poll/poll.c b/compat/poll/poll.c
index ad5dcde439..4abbfcb6a4 100644
--- a/compat/poll/poll.c
+++ b/compat/poll/poll.c
@@ -18,6 +18,9 @@
 You should have received a copy of the GNU General Public License along
 with this program; if not, see .  */
  
+/* To bump the minimum Windows version to Windows Vista */

+#include "git-compat-util.h"
+
  /* Tell gcc not to warn about the (nfd < 0) tests, below.  */
  #if (__GNUC__ == 4 && 3 <= __GNUC_MINOR__) || 4 < __GNUC__
  # pragma GCC diagnostic ignored "-Wtype-limits"
@@ -449,7 +452,8 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
static HANDLE hEvent;
WSANETWORKEVENTS ev;
HANDLE h, handle_array[FD_SETSIZE + 2];
-  DWORD ret, wait_timeout, nhandles, start = 0, elapsed, orig_timeout = 0;
+  DWORD ret, wait_timeout, nhandles, elapsed, orig_timeout = 0;
+  ULONGLONG start = 0;
fd_set rfds, wfds, xfds;
BOOL poll_again;
MSG msg;
@@ -465,7 +469,7 @@ poll (struct pollfd *pfd, nfds_t nfd, int timeout)
if (timeout != INFTIM)
  {
orig_timeout = timeout;
-  start = GetTickCount();
+  start = GetTickCount64();
  }
  
if (!hEvent)

@@ -614,7 +618,7 @@ restart:
  
if (!rc && orig_timeout && timeout != INFTIM)

  {
-  elapsed = GetTickCount() - start;
+  elapsed = (DWORD)(GetTickCount64() - start);


AFAICS, this subtraction in the old code is the correct way to take 
account of wrap-arounds in the tick count. The new code truncates the 64 
bit difference to 32 bits; the result is exactly identical to a 
difference computed from truncated 32 bit values, which is what we had 
in the old code.


IOW, there is no change in behavior. The statement "avoid wrap-around 
issues" in the subject line is not correct. The patch's only effect is 
that it removes Warning C28159.


What is really needed is that all quantities in the calculations are 
promoted to ULONGLONG. Unless, of course, we agree that a timeout of 
more than 49 days cannot happen ;)



timeout = elapsed >= orig_timeout ? 0 : orig_timeout - elapsed;
  }
  


-- Hannes

[PATCH v2 sg/test-rebase-editor-fix] t3404-rebase-interactive: test abbreviated commands

[Johannes Sixt]

Make sure that each short command is tested at least once. To
not exacerbate the runtime of the test script, do not add new
tests, but modify existing ones according to these criteria:

- The test does not have a prerequisite.
- The 'git rebase' command is not guarded by test_must_fail.

The pick commands are optional in the FAKE_LINES variable, but
when used, they do end up in the insn sheet. Test them, too.

Signed-off-by: Johannes Sixt 
---
 v2:
 - updated commit message
 - patch text is unchanged

 t/lib-rebase.sh   |  4 ++--
 t/t3404-rebase-interactive.sh | 10 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 2ca9fb69d6..0c93d00bdd 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -47,9 +47,9 @@ set_fake_editor () {
action=pick
for line in $FAKE_LINES; do
case $line in
-   pick|squash|fixup|edit|reword|drop)
+   pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
action="$line";;
-   exec*)
+   exec_*|x_*)
echo "$line" | sed 's/_/ /g' >> "$1";;
"#")
echo '# comment' >> "$1";;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 352a52e59d..d36ee4f807 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -114,7 +114,7 @@ test_expect_success 'rebase -i with exec allows git 
commands in subdirs' '
git checkout master &&
mkdir subdir && (cd subdir &&
set_fake_editor &&
-   FAKE_LINES="1 exec_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
+   FAKE_LINES="1 x_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
git rebase -i HEAD^
)
 '
@@ -499,7 +499,7 @@ test_expect_success 'squash works as expected' '
git checkout -b squash-works no-conflict-branch &&
one=$(git rev-parse HEAD~3) &&
set_fake_editor &&
-   FAKE_LINES="1 squash 3 2" EXPECT_HEADER_COUNT=2 \
+   FAKE_LINES="1 s 3 2" EXPECT_HEADER_COUNT=2 \
git rebase -i HEAD~3 &&
test $one = $(git rev-parse HEAD~2)
 '
@@ -732,7 +732,7 @@ test_expect_success 'reword' '
git show HEAD^ | grep "D changed" &&
FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" git rebase 
-i A &&
git show HEAD~3 | grep "B changed" &&
-   FAKE_LINES="1 reword 2 3 4" FAKE_COMMIT_MESSAGE="C changed" git rebase 
-i A &&
+   FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" git 
rebase -i A &&
git show HEAD~2 | grep "C changed"
 '
 
@@ -758,7 +758,7 @@ test_expect_success 'rebase -i can copy notes over a fixup' 
'
git reset --hard n3 &&
git notes add -m"an earlier note" n2 &&
set_fake_editor &&
-   GIT_NOTES_REWRITE_MODE=concatenate FAKE_LINES="1 fixup 2" git rebase -i 
n1 &&
+   GIT_NOTES_REWRITE_MODE=concatenate FAKE_LINES="1 f 2" git rebase -i n1 
&&
git notes show > output &&
test_cmp expect output
 '
@@ -1208,7 +1208,7 @@ rebase_setup_and_clean () {
 test_expect_success 'drop' '
rebase_setup_and_clean drop-test &&
set_fake_editor &&
-   FAKE_LINES="1 drop 2 3 drop 4 5" git rebase -i --root &&
+   FAKE_LINES="1 drop 2 3 d 4 5" git rebase -i --root &&
test E = $(git cat-file commit HEAD | sed -ne \$p) &&
test C = $(git cat-file commit HEAD^ | sed -ne \$p) &&
test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
-- 
2.19.1.406.g1aa3f475f3

Re: [PATCH sg/test-rebase-editor-fix] t3404-rebase-interactive: test abbreviated commands

[Johannes Sixt]

Am 25.10.18 um 22:54 schrieb Johannes Sixt:

Test each short command at least once. The commands changed here
are chosen such that

- tests do not have a prerequisite,
- the 'git rebase' command is not guarded by test_must_fail.

The pick commands are optional noise words in the FAKE_LINES
variable. Test them, too.


Actually, this sentence should better be:

The pick commands are optional in the FAKE_LINES variable, but
when used, they do end up in the insn sheet. Test them, too.



Signed-off-by: Johannes Sixt 

...

@@ -732,7 +732,7 @@ test_expect_success 'reword' '
git show HEAD^ | grep "D changed" &&
FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" git rebase -i A 
&&
git show HEAD~3 | grep "B changed" &&
-   FAKE_LINES="1 reword 2 3 4" FAKE_COMMIT_MESSAGE="C changed" git rebase -i A 
&&
+   FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" git rebase -i A 
&&
git show HEAD~2 | grep "C changed"
  '

[PATCH sg/test-rebase-editor-fix] t3404-rebase-interactive: test abbreviated commands

[Johannes Sixt]

Test each short command at least once. The commands changed here
are chosen such that

- tests do not have a prerequisite,
- the 'git rebase' command is not guarded by test_must_fail.

The pick commands are optional noise words in the FAKE_LINES
variable. Test them, too.

Signed-off-by: Johannes Sixt 
---
 This patch must be placed on top of sg/test-rebase-editor-fix.
 It has a textual conflict with my sequencer 'b' fix from some
 minutes ago, but the resolution should be obvious:

 -  exec*|break|b)
  - exec_*|x_*)
 ++ exec_*|x_*|break|b)

 t/lib-rebase.sh   |  4 ++--
 t/t3404-rebase-interactive.sh | 10 +-
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 2ca9fb69d6..0c93d00bdd 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -47,9 +47,9 @@ set_fake_editor () {
action=pick
for line in $FAKE_LINES; do
case $line in
-   pick|squash|fixup|edit|reword|drop)
+   pick|p|squash|s|fixup|f|edit|e|reword|r|drop|d)
action="$line";;
-   exec*)
+   exec_*|x_*)
echo "$line" | sed 's/_/ /g' >> "$1";;
"#")
echo '# comment' >> "$1";;
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 352a52e59d..d36ee4f807 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -114,7 +114,7 @@ test_expect_success 'rebase -i with exec allows git 
commands in subdirs' '
git checkout master &&
mkdir subdir && (cd subdir &&
set_fake_editor &&
-   FAKE_LINES="1 exec_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
+   FAKE_LINES="1 x_cd_subdir_&&_git_rev-parse_--is-inside-work-tree" \
git rebase -i HEAD^
)
 '
@@ -499,7 +499,7 @@ test_expect_success 'squash works as expected' '
git checkout -b squash-works no-conflict-branch &&
one=$(git rev-parse HEAD~3) &&
set_fake_editor &&
-   FAKE_LINES="1 squash 3 2" EXPECT_HEADER_COUNT=2 \
+   FAKE_LINES="1 s 3 2" EXPECT_HEADER_COUNT=2 \
git rebase -i HEAD~3 &&
test $one = $(git rev-parse HEAD~2)
 '
@@ -732,7 +732,7 @@ test_expect_success 'reword' '
git show HEAD^ | grep "D changed" &&
FAKE_LINES="reword 1 2 3 4" FAKE_COMMIT_MESSAGE="B changed" git rebase 
-i A &&
git show HEAD~3 | grep "B changed" &&
-   FAKE_LINES="1 reword 2 3 4" FAKE_COMMIT_MESSAGE="C changed" git rebase 
-i A &&
+   FAKE_LINES="1 r 2 pick 3 p 4" FAKE_COMMIT_MESSAGE="C changed" git 
rebase -i A &&
git show HEAD~2 | grep "C changed"
 '
 
@@ -758,7 +758,7 @@ test_expect_success 'rebase -i can copy notes over a fixup' 
'
git reset --hard n3 &&
git notes add -m"an earlier note" n2 &&
set_fake_editor &&
-   GIT_NOTES_REWRITE_MODE=concatenate FAKE_LINES="1 fixup 2" git rebase -i 
n1 &&
+   GIT_NOTES_REWRITE_MODE=concatenate FAKE_LINES="1 f 2" git rebase -i n1 
&&
git notes show > output &&
test_cmp expect output
 '
@@ -1208,7 +1208,7 @@ rebase_setup_and_clean () {
 test_expect_success 'drop' '
rebase_setup_and_clean drop-test &&
set_fake_editor &&
-   FAKE_LINES="1 drop 2 3 drop 4 5" git rebase -i --root &&
+   FAKE_LINES="1 drop 2 3 d 4 5" git rebase -i --root &&
test E = $(git cat-file commit HEAD | sed -ne \$p) &&
test C = $(git cat-file commit HEAD^ | sed -ne \$p) &&
test A = $(git cat-file commit HEAD^^ | sed -ne \$p)
-- 
2.19.1.406.g1aa3f475f3

[PATCH 3/2] rebase -i: recognize short commands without arguments

[Johannes Sixt]

The sequencer instruction 'b', short for 'break', is rejected:

  error: invalid line 2: b

The reason is that the parser expects all short commands to have
an argument. Permit short commands without arguments.

Signed-off-by: Johannes Sixt 
---
 I'll send a another patch in a moment that tests all short
 sequencer commands, but it is independent from this topic.

 sequencer.c| 3 ++-
 t/lib-rebase.sh| 2 +-
 t/t3418-rebase-continue.sh | 4 +++-
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index ee3961ec63..3107f59ea7 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1954,7 +1954,8 @@ static int parse_insn_line(struct todo_item *item, const 
char *bol, char *eol)
if (skip_prefix(bol, todo_command_info[i].str, )) {
item->command = i;
break;
-   } else if (bol[1] == ' ' && *bol == todo_command_info[i].c) {
+   } else if ((bol + 1 == eol || bol[1] == ' ') &&
+  *bol == todo_command_info[i].c) {
bol++;
item->command = i;
break;
diff --git a/t/lib-rebase.sh b/t/lib-rebase.sh
index 584604ee63..86572438ec 100644
--- a/t/lib-rebase.sh
+++ b/t/lib-rebase.sh
@@ -49,7 +49,7 @@ set_fake_editor () {
case $line in
squash|fixup|edit|reword|drop)
action="$line";;
-   exec*|break)
+   exec*|break|b)
echo "$line" | sed 's/_/ /g' >> "$1";;
"#")
echo '# comment' >> "$1";;
diff --git a/t/t3418-rebase-continue.sh b/t/t3418-rebase-continue.sh
index 185a491089..b282505aac 100755
--- a/t/t3418-rebase-continue.sh
+++ b/t/t3418-rebase-continue.sh
@@ -243,7 +243,9 @@ unset GIT_SEQUENCE_EDITOR
 
 test_expect_success 'the todo command "break" works' '
rm -f execed &&
-   FAKE_LINES="break exec_>execed" git rebase -i HEAD &&
+   FAKE_LINES="break b exec_>execed" git rebase -i HEAD &&
+   test_path_is_missing execed &&
+   git rebase --continue &&
test_path_is_missing execed &&
git rebase --continue &&
test_path_is_file execed
-- 
2.19.1.406.g1aa3f475f3

Re: What's cooking in git.git (Oct 2018, #04; Fri, 19)

[Johannes Sixt]

Am 19.10.18 um 08:02 schrieb Junio C Hamano:

* js/diff-notice-has-drive-prefix (2018-10-19) 1 commit
  - diff: don't attempt to strip prefix from absolute Windows paths

  "git diff /a/b/c /a/d/f" noticed these are full paths with shared
  leading prefix "/a", but failed to notice a similar fact about "git
  diff D:/a/b/c D:/a/d/f", which has been corrected.


This patch isn't about a misdetected leading prefix, but about
incorrectly truncated absolute paths. How about:

  Under certain circumstances, "git diff D:/a/b/c D:/a/b/d" on
  Windows would strip initial parts from the paths because they
  were not recognized as absolute, which has been corrected.


  Want tests.


I've sent v2 with a test.

-- Hannes

[PATCH v2] diff: don't attempt to strip prefix from absolute Windows paths

[Johannes Sixt]

git diff can be invoked with absolute paths. Typically, this triggers
the --no-index case. Then the absolute paths remain in the file names
that are printed in the output.

There is one peculiarity, though: When the command is invoked from a
a sub-directory in a repository, then it is attempted to strip the
sub-directory from the beginning of relative paths. Yet, to detect a
relative path the code just checks for an initial forward slash.
This mistakes a Windows style path like "D:/base" as a relative path
and the output looks like this, for example:

  D:\dir\test\one>git -P diff --numstat D:\dir\base D:\dir\diff
  1   1   ir/{base => diff}/1.txt

where the correct output should be

  D:\dir\test\one>git -P diff --numstat D:\dir\base D:\dir\diff
  1   1   D:/dir/{base => diff}/1.txt

If the sub-directory where 'git diff' is invoked is sufficiently deep
that the prefix becomes longer than the path to be printed, then the
subsequent code accesses the path out of bounds.

Use is_absolute_path() to detect Windows style absolute paths.

One might wonder whether the check for a directory separator that
is visible in the patch context should be changed from == '/' to
is_dir_sep() or not. It turns out not to be necessary. That code
only ever investigates paths that have undergone pathspec
normalization, after which there are only forward slashes even on
Windows.

Signed-off-by: Johannes Sixt 
---
v2:
- added a test that demonstrates the problem on Windows
- changed the example in the commit message to clarify that this is
  about truncated paths, not about failure to detect common prefix

 diff.c   |  4 ++--
 t/t4053-diff-no-index.sh | 10 ++
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index f0c7557b40..d18eb198f2 100644
--- a/diff.c
+++ b/diff.c
@@ -4267,12 +4267,12 @@ static void diff_fill_oid_info(struct diff_filespec 
*one)
 static void strip_prefix(int prefix_length, const char **namep, const char 
**otherp)
 {
/* Strip the prefix but do not molest /dev/null and absolute paths */
-   if (*namep && **namep != '/') {
+   if (*namep && !is_absolute_path(*namep)) {
*namep += prefix_length;
if (**namep == '/')
++*namep;
}
-   if (*otherp && **otherp != '/') {
+   if (*otherp && !is_absolute_path(*otherp)) {
*otherp += prefix_length;
if (**otherp == '/')
++*otherp;
diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 453e6c35eb..6e0dd6f9e5 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -127,4 +127,14 @@ test_expect_success 'diff --no-index from repo subdir 
respects config (implicit)
test_cmp expect actual.head
 '
 
+test_expect_success 'diff --no-index from repo subdir with absolute paths' '
+   cat <<-EOF >expect &&
+   1   1   $(pwd)/non/git/{a => b}
+   EOF
+   test_expect_code 1 \
+   git -C repo/sub diff --numstat \
+   "$(pwd)/non/git/a" "$(pwd)/non/git/b" >actual &&
+   test_cmp expect actual
+'
+
 test_done
-- 
2.19.1.406.g1aa3f475f3

Re: [PATCH] diff: don't attempt to strip prefix from absolute Windows paths

[Johannes Sixt]

Am 18.10.18 um 20:49 schrieb Stefan Beller:
> On Thu, Oct 18, 2018 at 11:38 AM Johannes Sixt  wrote:
> 
>> There is one peculiarity, though: [...]
> 
> The explanation makes sense, and the code looks good.
> Do we want to have a test for this niche case?
> 

Good point. That would be the following. But give me a day or two to
cross-check on Windows and whether it really catches the breakage.

diff --git a/t/t4053-diff-no-index.sh b/t/t4053-diff-no-index.sh
index 453e6c35eb..6e0dd6f9e5 100755
--- a/t/t4053-diff-no-index.sh
+++ b/t/t4053-diff-no-index.sh
@@ -127,4 +127,14 @@ test_expect_success 'diff --no-index from repo subdir 
respects config (implicit)
test_cmp expect actual.head
 '
 
+test_expect_success 'diff --no-index from repo subdir with absolute paths' '
+   cat <<-EOF >expect &&
+   1   1   $(pwd)/non/git/{a => b}
+   EOF
+   test_expect_code 1 \
+   git -C repo/sub diff --numstat \
+   "$(pwd)/non/git/a" "$(pwd)/non/git/b" >actual &&
+   test_cmp expect actual
+'
+
 test_done

[PATCH] diff: don't attempt to strip prefix from absolute Windows paths

[Johannes Sixt]

git diff can be invoked with absolute paths. Typically, this triggers
the --no-index case. Then the absolute paths remain in the file names
that are printed in the output.

There is one peculiarity, though: When the command is invoked from a
a sub-directory in a repository, then it is attempted to strip the
sub-directory from the beginning of relative paths. Yet, to detect a
relative path the code just checks for an initial forward slash.
This mistakes a Windows style path like D:/base as a relative path
and the output looks like this, for example:

  D:\test\one>git -P diff --numstat D:\base D:\diff
  1   1   {ase => iff}/1.txt

where the correct output should be

  D:\test\one>git -P diff --numstat D:\base D:\diff
  1   1   D:/{base => diff}/1.txt

If the sub-directory where 'git diff' is invoked is sufficiently deep
that the prefix becomes longer than the path to be printed, then the
subsequent code even accesses the paths out of bounds!

Use is_absolute_path() to detect Windows style absolute paths.

One might wonder whether the check for a directory separator that
is visible in the patch context should be changed from == '/' to
is_dir_sep() or not. It turns out not to be necessary. That code
only ever investigates paths that have undergone pathspec
normalization, after which there are only forward slashes even on
Windows.

Signed-off-by: Johannes Sixt 
---
 diff.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/diff.c b/diff.c
index f0c7557b40..d18eb198f2 100644
--- a/diff.c
+++ b/diff.c
@@ -4267,12 +4267,12 @@ static void diff_fill_oid_info(struct diff_filespec 
*one)
 static void strip_prefix(int prefix_length, const char **namep, const char 
**otherp)
 {
/* Strip the prefix but do not molest /dev/null and absolute paths */
-   if (*namep && **namep != '/') {
+   if (*namep && !is_absolute_path(*namep)) {
*namep += prefix_length;
if (**namep == '/')
++*namep;
}
-   if (*otherp && **otherp != '/') {
+   if (*otherp && !is_absolute_path(*otherp)) {
*otherp += prefix_length;
if (**otherp == '/')
++*otherp;
-- 
2.19.1.406.g1aa3f475f3

Re: [PATCH] zlib.c: use size_t for size

[Johannes Sixt]

Am 13.10.18 um 04:46 schrieb Jeff King:

But no, right before that we have this line:

   offset -= win->offset;

So offset is in fact no longer its original meaning of "offset into the
packfile", but is now an offset of the specific request into the window
we found.

So I think it's correct, but it sure confused me. I wonder if another
variable might help, like:

   size_t offset_in_window;
   ...

   /*
* We know this difference will fit in a size_t, because our mmap
* window by * definition can be no larger than a size_t.
*/
   offset_in_window = xsize_t(offset - win->offset);
   if (left)
*left = win->len - offset_in_window;
   return win->base + offset_in_window;

I dunno. Maybe it is overkill.


Thank you for your analysis. No, I don't think that such a new variable 
would be overkill. It is important to make such knowledge of value 
magnitudes explicit precisely because it reduces confusion and helps 
reviewers of the code verify correctness.


-- Hannes

Re: [PATCH v3 4/7] revision.c: begin refactoring --topo-order logic

[Johannes Sixt]

Am 12.10.18 um 08:33 schrieb Junio C Hamano:

"Derrick Stolee via GitGitGadget"  writes:

+struct topo_walk_info {};
+
+static void init_topo_walk(struct rev_info *revs)
+{
+   struct topo_walk_info *info;
+   revs->topo_walk_info = xmalloc(sizeof(struct topo_walk_info));
+   info = revs->topo_walk_info;
+   memset(info, 0, sizeof(struct topo_walk_info));


There is no member in the struct at this point.  Are we sure this is
safe?  Just being curious.


sizeof cannot return 0. sizeof(struct topo_walk_info) will be 1 here.

-- Hannes

Re: What's cooking in git.git (Oct 2018, #01; Wed, 10)

[Johannes Sixt]

Am 10.10.18 um 07:43 schrieb Junio C Hamano:

We haven't seen much complaints and breakages reported against the
two big "rewrite in C" topics around "rebase"; perhaps it is a good
time to merge them to 'next' soonish to cook them for a few weeks
before moving them to 'master'?


Please let me express my sincerest gratitude to Alban, Joel, 
Paul-Sebastian, Pratik, and Dscho. It is such a pleasure to work with 
the builtin rebase and stash commands on Windows now. I am using them 
since a month or two, and they work extremely well for me.


Thank you all for your hard work!

-- Hannes

Re: What's so special about objects/17/ ?

[Johannes Sixt]

Am 07.10.18 um 21:06 schrieb Ævar Arnfjörð Bjarmason:

Picking any one number is explained in the comment. I'm asking why 17 in
particular not for correctness reasons but as a bit of historical lore,
and because my ulterior is to improve the GC docs.

The number in that comic is 4 (and no datestamp on when it was
published). Are you saying Junio's patch is somehow a reference to that
xkcd in particular, or that it's just a funny reference in this context?


No lore, AFAIR. It's just a random number, determined by a fair dice 
roll or something ;)


-- Hannes

Re: What's so special about objects/17/ ?

[Johannes Sixt]

Am 07.10.18 um 20:28 schrieb Ævar Arnfjörð Bjarmason:

In 2007 Junio wrote
(https://public-inbox.org/git/7vr6lcj2zi@gitster.siamese.dyndns.org/):

 +static int need_to_gc(void)
 +{
 +  /*
 +   * Quickly check if a "gc" is needed, by estimating how
 +   * many loose objects there are.  Because SHA-1 is evenly
 +   * distributed, we can check only one and get a reasonable
 +   * estimate.
 +   */



1. We still have this check of objects/17/ in builtin/gc.c today. Why
objects/17/ and not e.g. objects/00/ to go with other 000* magic such
as the  SHA-1?  Statistically
it doesn't matter, but 17 seems like an odd thing to pick at random
out of 00..ff, does it have any significance?


The reason is explained in the comment. And, BTW, you do know about this 
one: https://xkcd.com/221/ don't you? (TLDR: the title is "Random Number")



2. It seems overly paranoid to be checking that the files in
   .git/objects/17/ look like a SHA-1. If we have stuff not generated by
   git in .git/objects/??/ we probably have bigger problems than
   prematurely triggering auto gc, can this just be removed as
   redundant. Was this some check e.g. expecting that this would need to
   deal with tempfiles in these directories that we created at the time
   (but no longer do?)?


It's not about that there are SHA-1s in there, it's about how many there 
are.


-- Hannes

Re: What's cooking in git.git (Sep 2018, #04; Thu, 20)

[Johannes Sixt]

Am 21.09.18 um 07:22 schrieb Junio C Hamano:
> The tip of 'next' hasn't been rewound yet.  The three GSoC "rewrite
> in C" topics are still unclassified in this "What's cooking" report,
> but I am hoping that we can have them in 'next' sooner rather than
> later.  I got an impression that Dscho wanted a chance for the final
> clean-up on some of them, so I am not doing anything hasty yet at
> this moment, though.

While playing around with those topics in my own build on Windows, I
noticed a small glitch in your merge commits.

When I compile 59085279e6, which is today's jch~11, I see

CC builtin/rebase.o
builtin/rebase.c: In function 'can_fast_forward':
builtin/rebase.c:443:2: warning: implicit declaration of function 
'get_merge_bases' [-Wimplicit-function-declaration]
  merge_bases = get_merge_bases(onto, head);
  ^
builtin/rebase.c:443:14: warning: assignment makes pointer from integer without 
a cast [enabled by default]
  merge_bases = get_merge_bases(onto, head);
  ^

I notice that you fixed it in the next merge, jch~10 aka d311e29abe,
by adding

#include "commit-reach.h"

in builtin/rebase.c; this line is obviously required one merge
commit earlier, jch~11.

-- Hannes

Re: [Question] Signature calculation ignoring parts of binary files

[Johannes Sixt]

Am 12.09.18 um 21:16 schrieb Randall S. Becker:

I feel really bad asking this, and I should know the answer, and yet.

I have a binary file that needs to go into a repo intact (unchanged). I also
have a program that interprets the contents, like a textconv, that can
output the relevant portions of the file in whatever format I like - used
for diff typically, dumps in 1K chunks by file section. What I'm looking for
is to have the SHA1 signature calculated with just the relevant portions of
the file so that two actually different files will be considered the same by
git during a commit or status. In real terms, I'm trying to ignore the
Creator metadata of a JPG because it is mutable and irrelevant to my repo
contents.

I'm sorry to ask, but I thought this was in .gitattributes but I can't
confirm the SHA1 behaviour.


You are looking for a clean filter. See the 'filter' attribute in 
gitattributes(5). Your clean filter program or script should strip the 
unwanted metadata or set it to a constant known-good value.


(You shouldn't need a smudge filter.)

-- Hannes

Re: [PATCH v2 2/2] mingw: fix mingw_open_append to work with named pipes

[Johannes Sixt]

Am 10.09.18 um 19:05 schrieb Jeff Hostetler via GitGitGadget:

diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..f87376b26a 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,6 +341,19 @@ int mingw_mkdir(const char *path, int mode)
return ret;
  }
  
+/*

+ * Calling CreateFile() using FILE_APPEND_DATA and without FILE_WRITE_DATA
+ * is documented in [1] as opening a writable file handle in append mode.
+ * (It is believed that) this is atomic since it is maintained by the
+ * kernel unlike the O_APPEND flag which is racily maintained by the CRT.
+ *
+ * [1] 
https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants
+ *
+ * This trick does not appear to work for named pipes.  Instead it creates
+ * a named pipe client handle that cannot be written to.  Callers should
+ * just use the regular _wopen() for them.  (And since client handle gets
+ * bound to a unique server handle, it isn't really an issue.)
+ */
  static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
  {
HANDLE handle;
@@ -360,10 +373,12 @@ static int mingw_open_append(wchar_t const *wfilename, 
int oflags, ...)
NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
if (handle == INVALID_HANDLE_VALUE)
return errno = err_win_to_posix(GetLastError()), -1;
+
/*
 * No O_APPEND here, because the CRT uses it only to reset the
-* file pointer to EOF on write(); but that is not necessary
-* for a file created with FILE_APPEND_DATA.
+* file pointer to EOF before each write(); but that is not
+* necessary (and may lead to races) for a file created with
+* FILE_APPEND_DATA.
 */
fd = _open_osfhandle((intptr_t)handle, O_BINARY);
if (fd < 0)
@@ -371,6 +386,23 @@ static int mingw_open_append(wchar_t const *wfilename, int 
oflags, ...)
return fd;
  }
  
+#define IS_SBS(ch) (((ch) == '/') || ((ch) == '\\'))

+/*
+ * Does the pathname map to the local named pipe filesystem?
+ * That is, does it have a "//./pipe/" prefix?
+ */
+static int mingw_is_local_named_pipe_path(const char *filename)
+{
+   return (IS_SBS(filename[0]) &&
+   IS_SBS(filename[1]) &&
+   filename[2] == '.'  &&
+   IS_SBS(filename[3]) &&
+   !strncasecmp(filename+4, "pipe", 4) &&
+   IS_SBS(filename[8]) &&
+   filename[9]);
+}
+#undef IS_SBS
+
  int mingw_open (const char *filename, int oflags, ...)
  {
typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
@@ -387,7 +419,7 @@ int mingw_open (const char *filename, int oflags, ...)
if (filename && !strcmp(filename, "/dev/null"))
filename = "nul";
  
-	if (oflags & O_APPEND)

+   if ((oflags & O_APPEND) && !mingw_is_local_named_pipe_path(filename))
open_fn = mingw_open_append;
else
open_fn = _wopen;


This looks reasonable.

I wonder which part of the code uses local named pipes. Is it downstream 
in Git for Windows or one of the topics in flight?


-- Hannes

Re: [PATCH] Revert "Merge branch 'sb/submodule-core-worktree'" (was Re: Old submodules broken in 2.19rc1 and 2.19rc2)

[Johannes Sixt]

Am 08.09.2018 um 04:04 schrieb Junio C Hamano:
> Jonathan Nieder  writes:
> 
>> It is late in the release cycle, so revert the whole 3-patch series.
>> We can try again later for 2.20.
>>
>> Reported-by: Allan Sandfeld Jensen 
>> Helped-by: Stefan Beller 
>> Signed-off-by: Jonathan Nieder 
>> ---
>> Stefan Beller wrote:
>>> Jonathan Nieder wrote:
>>
 I think we
 should revert e98317508c0 in "master" (for 2.19) and keep making use
 of that 'second try' in "next" (for 2.20).
>>>
>>> Actually I'd rather revert the whole topic leading up to
>>> 7e25437d35a (Merge branch 'sb/submodule-core-worktree', 2018-07-18)
>>> as the last patch in there doesn't work well without e98317508c0 IIRC.
>>>
>>> And having only the first patch would bring an inconsistent state as
>>> then different commands behave differently w.r.t. setting core.worktree.
>>
>> Like this (generated using "git revert -m1)?
> 
> OK.  Thanks for taking care of it.

Please don't forget to remove the corresponding release notes entry.

diff --git a/Documentation/RelNotes/2.19.0.txt 
b/Documentation/RelNotes/2.19.0.txt
index bcbfbc2041..834454ffb9 100644
--- a/Documentation/RelNotes/2.19.0.txt
+++ b/Documentation/RelNotes/2.19.0.txt
@@ -296,12 +296,6 @@ Fixes since v2.18
to the submodule was changed in the range of commits in the
superproject, sometimes showing "(null)".  This has been corrected.
 
- * "git submodule" did not correctly adjust core.worktree setting that
-   indicates whether/where a submodule repository has its associated
-   working tree across various state transitions, which has been
-   corrected.
-   (merge 984cd77ddb sb/submodule-core-worktree later to maint).
-
  * Bugfix for "rebase -i" corner case regression.
(merge a9279c6785 pw/rebase-i-keep-reword-after-conflict later to maint).
 

Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Johannes Sixt]

Am 08.09.2018 um 11:26 schrieb Johannes Sixt:

Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:

diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..ef03bbe5d2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const 
*wfilename, int oflags, ...)

   * FILE_SHARE_WRITE is required to permit child processes
   * to append to the file.
   */
-    handle = CreateFileW(wfilename, FILE_APPEND_DATA,
+    handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA,
  FILE_SHARE_WRITE | FILE_SHARE_READ,
  NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
  if (handle == INVALID_HANDLE_VALUE)



I did not go with this version because the documentation 
https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants 
says:


FILE_APPEND_DATA: For a file object, the right to append data to the 
file. (For local files, write operations will not overwrite existing 
data if this flag is specified without FILE_WRITE_DATA.) [...]


which could be interpreted as: Only if FILE_WRITE_DATA is not set, we 
have the guarantee that existing data in local files is not overwritten, 
i.e., new data is appended atomically.


Is this interpretation too narrow and we do get atomicity even when 
FILE_WRITE_DATA is set?


Here is are some comments on stackoverflow which let me think that 
FILE_APPEND_DATA with FILE_WRITE_DATA is no longer atomic:


https://stackoverflow.com/questions/20093571/difference-between-file-write-data-and-file-append-data#comment29995346_20108249

-- Hannes

Re: [PATCH 2/2] mingw: fix mingw_open_append to work with named pipes

[Johannes Sixt]

Am 07.09.2018 um 20:19 schrieb Jeff Hostetler via GitGitGadget:

From: Jeff Hostetler 

Signed-off-by: Jeff Hostetler 
---
  compat/mingw.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 858ca14a57..ef03bbe5d2 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -355,7 +355,7 @@ static int mingw_open_append(wchar_t const *wfilename, int 
oflags, ...)
 * FILE_SHARE_WRITE is required to permit child processes
 * to append to the file.
 */
-   handle = CreateFileW(wfilename, FILE_APPEND_DATA,
+   handle = CreateFileW(wfilename, FILE_WRITE_DATA | FILE_APPEND_DATA,
FILE_SHARE_WRITE | FILE_SHARE_READ,
NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
if (handle == INVALID_HANDLE_VALUE)



I did not go with this version because the documentation 
https://docs.microsoft.com/en-us/windows/desktop/fileio/file-access-rights-constants 
says:


FILE_APPEND_DATA: For a file object, the right to append data to the 
file. (For local files, write operations will not overwrite existing 
data if this flag is specified without FILE_WRITE_DATA.) [...]


which could be interpreted as: Only if FILE_WRITE_DATA is not set, we 
have the guarantee that existing data in local files is not overwritten, 
i.e., new data is appended atomically.


Is this interpretation too narrow and we do get atomicity even when 
FILE_WRITE_DATA is set?


-- Hannes

Re: [PATCH] config: fix commit-graph related config docs

[Johannes Sixt]

Am 22.08.2018 um 15:16 schrieb Derrick Stolee:

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 1c42364988..f846543414 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -917,7 +917,11 @@ core.notesRef::
  This setting defaults to "refs/notes/commits", and it can be overridden by
  the `GIT_NOTES_REF` environment variable.  See linkgit:git-notes[1].
  
-gc.commitGraph::

+core.commitGraph::
+   Enable git commit graph feature. Allows reading from the
+   commit-graph file.


Please do not assume that everybody knows what "the commit-graph file" 
is and that they know that they want it or do not want it. I do not 
know. You should mention here when to set this configuration variable to 
which values and what the default value is.



+
+gc.writeCommitGraph::
If true, then gc will rewrite the commit-graph file when
linkgit:git-gc[1] is run. When using linkgit:git-gc[1]
'--auto' the commit-graph will be updated if housekeeping is


-- Hannes

Re: Do not raise conflict when a code in a patch was already added

[Johannes Sixt]

Am 20.08.2018 um 19:40 schrieb Phillip Wood:

On 20/08/2018 11:22, Konstantin Kharlamov wrote:

It's spectacular, that content of one of inserted conflict markers is
empty, so all you have to do is to remove the markers, and use `git add`
on the file, and then `git rebase --continue`

Its a lot of unncessary actions, git could just figure that the code it
sees in the patch is already there, being a part of another commit.


If there are conflict markers where one side is empty it means that some
lines from the merge base (which for a rebase is the parent of the
commit being picked) have been deleted on one side and modified on the
other. Git cannot know if you want to use the deleted version or the
modified version.


There's another possibility (and I think it is what happens actually in 
Konstantin's case): When one side added lines 1 2 and the other side 
added 1 2 3, then the actual conflict is << 1 2 == 1 2 3 >>, but our 
merge code is able to move the identical part out of the conflicted 
section: 1 2 << == 3 >>. But this is just a courtesy for the user; the 
real conflict is the original one. Without this optimization, the work 
to resolve the conflict would be slightly more arduous.


-- Hannes

Re: [PATCH] mingw: enable atomic O_APPEND

[Johannes Sixt]

Am 14.08.2018 um 00:37 schrieb Jeff King:

And then you can do something like:

   for size in 4097 8193 16385 32769 65537 131073 262145 524289 1048577; do
 >out ;# clean up from last run
 echo "Trying $size..."
 timeout 5 ./write $size out
 if ! ./check $size 

I used your programs with necessary adjustments (as fork() is not 
available), and did similar tests with concurrent processes. With packet 
sizes 1025, 4093, 7531 (just to include some odd number), and 8193 I did 
not observe any overlapping or short writes.


I'm now very confident that we are on the safe side for our purposes.

-- Hannes

Re: [PATCH] mingw: enable atomic O_APPEND

[Johannes Sixt]

Am 13.08.2018 um 22:20 schrieb Junio C Hamano:

Johannes Sixt  writes:


The Windows CRT implements O_APPEND "manually": on write() calls, the
file pointer is set to EOF before the data is written. Clearly, this is
not atomic. And in fact, this is the root cause of failures observed in
t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where
different processes write to the same trace file simultanously; it also
occurred in t5400-send-pack.sh, but there it was worked around in
71406ed4d6 ("t5400: avoid concurrent writes into a trace file",
2017-05-18).

Fortunately, Windows does support atomic O_APPEND semantics using the
file access mode FILE_APPEND_DATA. Provide an implementation that does.

This implementation is minimal in such a way that it only implements
the open modes that are actually used in the Git code base. Emulation
for other modes can be added as necessary later. To become aware of
the necessity early, the unusal error ENOSYS is reported if an
unsupported mode is encountered.

Diagnosed-by: Johannes Schindelin 
Helped-by: Jeff Hostetler 
Signed-off-by: Johannes Sixt 
---
  compat/mingw.c | 41 +++--
  1 file changed, 39 insertions(+), 2 deletions(-)


Nice.

I wonder how much more expensive using this implementation is
compared with the original "race susceptible" open(), when raciness
is known not to be an issue (e.g. there is higher level lock that
protects the appending).


Certainly, the former way that uses two syscalls 
(SetFilePointer+WriteFile) is more costly than this new way with just 
one syscall (WriteFile). Of course, I don't know how atomic append would 
be implemented in the kernel, but I can't think of a reason why it 
should be slow on Windows, but fast on POSIX.


(But I can't provide numbers to back up my gut feeling...)

(And I also assume that you are not worried about the performance of 
open() itself.)



...[define race_safe_append_open]... and replace
the call to open(... O_APPEND ...) in trace.c::get_trace_fd() with a
call to that wrapper.  That way, other codepaths that use O_APPEND
(namely, reflog and todo-list writers) can avoid the additional
cost, if any.

Some may find it beneficial from code readability POV because that
approach marks the codepath that needs to have non-racy fd more
explicitly.


O_APPEND is POSIX and means race-free append. If you mark some call 
sites with O_APPEND, then that must be the ones that need race-free 
append. Hence, you would have to go the other route: Mark those call 
sites that do _not_ need race-free append with some custom 
function/macro. (Or mark both with different helpers and avoid writing 
down O_APPEND.)


-- Hannes

[PATCH] mingw: enable atomic O_APPEND

[Johannes Sixt]

The Windows CRT implements O_APPEND "manually": on write() calls, the
file pointer is set to EOF before the data is written. Clearly, this is
not atomic. And in fact, this is the root cause of failures observed in
t5552-skipping-fetch-negotiator.sh and t5503-tagfollow.sh, where
different processes write to the same trace file simultanously; it also
occurred in t5400-send-pack.sh, but there it was worked around in
71406ed4d6 ("t5400: avoid concurrent writes into a trace file",
2017-05-18).

Fortunately, Windows does support atomic O_APPEND semantics using the
file access mode FILE_APPEND_DATA. Provide an implementation that does.

This implementation is minimal in such a way that it only implements
the open modes that are actually used in the Git code base. Emulation
for other modes can be added as necessary later. To become aware of
the necessity early, the unusal error ENOSYS is reported if an
unsupported mode is encountered.

Diagnosed-by: Johannes Schindelin 
Helped-by: Jeff Hostetler 
Signed-off-by: Johannes Sixt 
---
 compat/mingw.c | 41 +++--
 1 file changed, 39 insertions(+), 2 deletions(-)

diff --git a/compat/mingw.c b/compat/mingw.c
index 6ded1c859f..858ca14a57 100644
--- a/compat/mingw.c
+++ b/compat/mingw.c
@@ -341,12 +341,44 @@ int mingw_mkdir(const char *path, int mode)
return ret;
 }
 
+static int mingw_open_append(wchar_t const *wfilename, int oflags, ...)
+{
+   HANDLE handle;
+   int fd;
+   DWORD create = (oflags & O_CREAT) ? OPEN_ALWAYS : OPEN_EXISTING;
+
+   /* only these flags are supported */
+   if ((oflags & ~O_CREAT) != (O_WRONLY | O_APPEND))
+   return errno = ENOSYS, -1;
+
+   /*
+* FILE_SHARE_WRITE is required to permit child processes
+* to append to the file.
+*/
+   handle = CreateFileW(wfilename, FILE_APPEND_DATA,
+   FILE_SHARE_WRITE | FILE_SHARE_READ,
+   NULL, create, FILE_ATTRIBUTE_NORMAL, NULL);
+   if (handle == INVALID_HANDLE_VALUE)
+   return errno = err_win_to_posix(GetLastError()), -1;
+   /*
+* No O_APPEND here, because the CRT uses it only to reset the
+* file pointer to EOF on write(); but that is not necessary
+* for a file created with FILE_APPEND_DATA.
+*/
+   fd = _open_osfhandle((intptr_t)handle, O_BINARY);
+   if (fd < 0)
+   CloseHandle(handle);
+   return fd;
+}
+
 int mingw_open (const char *filename, int oflags, ...)
 {
+   typedef int (*open_fn_t)(wchar_t const *wfilename, int oflags, ...);
va_list args;
unsigned mode;
int fd;
wchar_t wfilename[MAX_PATH];
+   open_fn_t open_fn;
 
va_start(args, oflags);
mode = va_arg(args, int);
@@ -355,9 +387,14 @@ int mingw_open (const char *filename, int oflags, ...)
if (filename && !strcmp(filename, "/dev/null"))
filename = "nul";
 
+   if (oflags & O_APPEND)
+   open_fn = mingw_open_append;
+   else
+   open_fn = _wopen;
+
if (xutftowcs_path(wfilename, filename) < 0)
return -1;
-   fd = _wopen(wfilename, oflags, mode);
+   fd = open_fn(wfilename, oflags, mode);
 
if (fd < 0 && (oflags & O_ACCMODE) != O_RDONLY && errno == EACCES) {
DWORD attrs = GetFileAttributesW(wfilename);
@@ -375,7 +412,7 @@ int mingw_open (const char *filename, int oflags, ...)
 * CREATE_ALWAYS flag of CreateFile()).
 */
if (fd < 0 && errno == EACCES)
-   fd = _wopen(wfilename, oflags & ~O_CREAT, mode);
+   fd = open_fn(wfilename, oflags & ~O_CREAT, mode);
if (fd >= 0 && set_hidden_flag(wfilename, 1))
warning("could not mark '%s' as hidden.", filename);
}
-- 
2.18.0.rc0.112.g5f42c6ebd3

Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE

[Johannes Sixt]

Am 10.08.2018 um 18:51 schrieb Jeff Hostetler:



On 8/10/2018 12:15 PM, Johannes Sixt wrote:

Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:
I reported a couple of times that t5552 is not passing reliably. It 
has now

reached next, and will no doubt infect master soon.

Turns out that it is not a Windows-specific issue, even if it occurs 
a lot

more often on Windows than elsewhere.

The culprit is that two processes try simultaneously to write to the 
same

file specified via GIT_TRACE_PACKET, and it is not well defined how that
should work, even on Linux.


Thanks for digging down to the root cause. As has been said, the 
behavior of O_APPEND is well-defined under POSIX, but last time I 
looked for equivalent feature on Windows, I did not find any.


Last time was when I worked around the same failure in 
t5503-tagfollow.sh in my private builds:


https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742 



It is basically the same as Peff suggests: log only one side of the 
fetch.


As this buglet looks like a recurring theme, and a proper fix is 
preferable over repeated work-arounds. To me it looks like we need 
some sort of locking on Windows. Unless your friends at Microsoft have 
an ace in their sleeves that lets us have atomic O_APPEND the POSIX 
way...


The official Windows CRT for open() does document an O_APPEND, but after
looking at the distributed source, I'm not sure I trust it.


I looked at the CRT code, too, and no, we can't trust it.


I have not looked at the MSYS version of the CRT yet so I cannot comment
on it.

I did find that the following code does what we want.  Notice the use of
FILE_APPEND_DATA in place of the usual GENERIC_READ.  (And yes, the docs
are very vague here.)


As far as I understand, FILE_APPEND_DATA is just a permission flag (I'm 
not sure why that would be necessary), but at least the documentation 
states that it is still necessary for the caller to set the file pointer.


But I haven't tried your code below, yet. Should it append the line of 
text on each invocation? And if so, is it an atomic operation?



Before we try a locking solution, perhaps we should tweak mingw_open()
to use something like this to get a proper HANDLE directly and then use
_open_osfhandle() to get a "fd" for it.

Jeff

-

#include 

int main()
{
 HANDLE h = CreateFileW(L"foo.txt",
     FILE_APPEND_DATA,
     FILE_SHARE_READ | FILE_SHARE_WRITE | FILE_SHARE_DELETE,
     NULL, OPEN_ALWAYS, FILE_ATTRIBUTE_NORMAL, NULL);

 const char * buf = "this is a test\n";
 DWORD len = strlen(buf);
 DWORD nbw;

 WriteFile(h, buf, len, , NULL);
 CloseHandle(h);

 return 0;
}

Re: [PATCH 0/4] t5552: fix flakiness by introducing proper locking for GIT_TRACE

[Johannes Sixt]

Am 09.08.2018 um 19:35 schrieb Johannes Schindelin via GitGitGadget:

I reported a couple of times that t5552 is not passing reliably. It has now
reached next, and will no doubt infect master soon.

Turns out that it is not a Windows-specific issue, even if it occurs a lot
more often on Windows than elsewhere.

The culprit is that two processes try simultaneously to write to the same
file specified via GIT_TRACE_PACKET, and it is not well defined how that
should work, even on Linux.


Thanks for digging down to the root cause. As has been said, the 
behavior of O_APPEND is well-defined under POSIX, but last time I looked 
for equivalent feature on Windows, I did not find any.


Last time was when I worked around the same failure in 
t5503-tagfollow.sh in my private builds:


https://github.com/j6t/git/commit/9a447a6844b50b43746d9765b3ac809e2793d742

It is basically the same as Peff suggests: log only one side of the fetch.

As this buglet looks like a recurring theme, and a proper fix is 
preferable over repeated work-arounds. To me it looks like we need some 
sort of locking on Windows. Unless your friends at Microsoft have an ace 
in their sleeves that lets us have atomic O_APPEND the POSIX way...


-- Hannes

Re: [PATCH v2] t/test-lib: make `test_dir_is_empty` more robust

[Johannes Sixt]

Am 05.08.2018 um 06:20 schrieb William Chargin:

While the `test_dir_is_empty` function appears correct in most normal
use cases, it can fail when filenames contain newlines. This patch
changes the implementation to use `ls -A`, which is specified by POSIX.
The output should be empty exactly if the directory is empty.

The newly added unit test fails before this change and passes after it.

Signed-off-by: William Chargin 
---

I originally wrote this patch for the standalone Sharness library, but
that library advises that such patches be sent to the Git mailing list
first.

  t/t-basic.sh| 29 +
  t/test-lib-functions.sh |  2 +-
  2 files changed, 30 insertions(+), 1 deletion(-)

diff --git a/t/t-basic.sh b/t/t-basic.sh
index 34859fe4a..3885b26f9 100755
--- a/t/t-basic.sh
+++ b/t/t-basic.sh
@@ -821,6 +821,35 @@ test_expect_success 'tests clean up even on failures' "
EOF
  "
  
+test_expect_success 'test_dir_is_empty behaves even in pathological cases' "

+   run_sub_test_lib_test \
+   dir-empty 'behavior of test_dir_is_empty' <<-\\EOF &&
+   test_expect_success 'should pass with actually empty directory' '
+   mkdir empty_dir &&
+   test_dir_is_empty empty_dir
+   '
+   test_expect_success 'should fail with a normal filename' '
+   mkdir nonempty_dir &&
+   touch nonempty_dir/some_file &&
+   test_must_fail test_dir_is_empty nonempty_dir
+   '
+   test_expect_success 'should fail with dot-newline-dot filename' '
+   mkdir pathological_dir &&
+   printf \"pathological_dir/.n.0\" | xargs -0 touch &&
+   test_must_fail test_dir_is_empty pathological_dir
+   '
+   test_done
+   EOF
+   check_sub_test_lib_test dir-empty <<-\\EOF
+   > ok 1 - should pass with actually empty directory
+   > ok 2 - should fail with a normal filename
+   > ok 3 - should fail with dot-newline-dot filename
+   > # passed all 3 test(s)
+   > 1..3
+   EOF
+"
+
+
  
  # Basics of the basics
  
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh

index 2b2181dca..f7ff28ef6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -568,7 +568,7 @@ test_path_is_dir () {
  # Check if the directory exists and is empty as expected, barf otherwise.
  test_dir_is_empty () {
test_path_is_dir "$1" &&
-   if test -n "$(ls -a1 "$1" | egrep -v '^\.\.?$')"
+   if test "$(ls -A1 "$1" | wc -c)" != 0
then
echo "Directory '$1' is not empty, it contains:"
ls -la "$1"



Did you accidentally resend the same patch in this v2? I can't spot the 
difference to v1.


-- Hannes

First test of t5552 fails on Windows

[Johannes Sixt]

I'm testing origin/next on Windows with a few other topics on top.

The first test fails like this. Do you see what is wrong?
Where should I start looking? Is it perhaps that upload-pack
is responding too soon so that fetch does not send 'have c1'?

 this is the console output
...(truncated)...
++ git -C client config fetch.negotiationalgorithm skipping
+++ pwd
+++ builtin pwd -W
+++ pwd
+++ builtin pwd -W
++ GIT_TRACE_PACKET='D:/Src/mingw-git/t/trash 
directory.t5552-skipping-fetch-negotiator/trace'
++ git -C client fetch 'D:/Src/mingw-git/t/trash 
directory.t5552-skipping-fetch-negotiator/server'
warning: no common commits
remote: Enumerating objects: 3, done.
remote: Counting objects: 100% (3/3), done.
remote: Total 3 (delta 0), reused 0 (delta 0)
Unpacking objects: 100% (3/3), done.
>From D:/Src/mingw-git/t/trash directory.t5552-skipping-fetch-negotiator/server
 * branchHEAD   -> FETCH_HEAD
++ have_sent c7 c5 c2 c1
++ test 4 -ne 0
+++ git -C client rev-parse c7
++ grep 'fetch> have 9b13844ba1d52a28bb9487107b41cce9916b74c9' trace
packet:fetch> have 9b13844ba1d52a28bb9487107b41cce9916b74c9
++ test 0 -ne 0
++ shift
++ test 3 -ne 0
+++ git -C client rev-parse c5
++ grep 'fetch> have 0abab022ac7e07f16265106cf36faf7cb5d87ab3' trace
packet:fetch> have 0abab022ac7e07f16265106cf36faf7cb5d87ab3
++ test 0 -ne 0
++ shift
++ test 2 -ne 0
+++ git -C client rev-parse c2
++ grep 'fetch> have 0d7e994c092abbb0a21e7d243114efa5ba452b8c' trace
packet:fetch> have 0d7e994c092abbb0a21e7d243114efa5ba452b8c
++ test 0 -ne 0
++ shift
++ test 1 -ne 0
+++ git -C client rev-parse c1
++ grep 'fetch> have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9' trace
++ test 1 -ne 0
+++ git -C client rev-parse c1
++ echo 'No have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9 (c1)'
No have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9 (c1)
++ return 1
error: last command exited with $?=1
not ok 1 - commits with no parents are sent regardless of skip distance
#
#   git init server &&
#   test_commit -C server to_fetch &&
#
#   git init client &&
#   for i in $(seq 7)
#   do
#   test_commit -C client c$i
#   done &&
#
#   # We send: "c7" (skip 1) "c5" (skip 2) "c2" (skip 4). After 
that, since
#   # "c1" has no parent, it is still sent as "have" even though it 
would
#   # normally be skipped.
#   test_config -C client fetch.negotiationalgorithm skipping &&
#   GIT_TRACE_PACKET="$(pwd)/trace" git -C client fetch 
"$(pwd)/server" &&
#   have_sent c7 c5 c2 c1 &&
#   have_not_sent c6 c4 c3
#

 trace file

packet:  upload-pack> 92dc17da106e837e66a07e4815c474bf4fe99dce HEAD\0multi_ack 
thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not 
deepen-relative no-progress include-tag multi_ack_detailed 
symref=HEAD:refs/heads/master agent=git/2.18.0.721.g75e0872d82
packet:fetch< 92dc17da106e837e66a07e4815c474bf4fe99dce HEAD\0multi_ack 
thin-pack side-band side-band-64k ofs-delta shallow deepen-since deepen-not 
deepen-relative no-progress include-tag multi_ack_detailed 
symref=HEAD:refs/heads/master agent=git/2.18.0.721.g75e0872d82
packet:  upload-pack> 92dc17da106e837e66a07e4815c474bf4fe99dce refs/heads/master
packet:  upload-pack> 92dc17da106e837e66a07e4815c474bf4fe99dce 
refs/tags/to_fetch
packet:fetch< 92dc17da106e837e66a07e4815c474bf4fe99dce refs/heads/master
packet:  upload-pack> 
7da106e837e66a07e4815c474bf4fe99dce refs/tags/to_fetch
packet:fetch< 
packet:fetch> want 92dc17da106e837e66a07e4815c474bf4fe99dce 
multi_ack_detailed side-band-64k thin-pack ofs-delta deepen-since deepen-not 
agent=git/2.18.0.721.g75e0872d82
packet:fetch> 
packet:fetch> have 9b13844ba1d52a28bb9487107b41cce9916b74c9
packet:fetch> have 0abab022ac7e07f16265106cf36faf7cb5d87ab3
packet:fetch> have 0d7e994c092abbb0a21e7d243114efa5ba452b8c
packet:  upload-pack< want 92dc17da106e837e66a07e4815c474bf4fe99dce 
multi_ack_detailed side-band-64k thin-pack ofs-delta deepen-since deepen-not 
agent=git/2.18.0.721.g75e0872d82
packet:fetch> done
packet:  upload-pack< 
packet:  upload-pack< have 9b13844ba1d52a28bb9487107b41cce9916b74c9
packet:  upload-pack< have 0abab022ac7e07f16265106cf36faf7cb5d87ab3
packet:  upload-pack< have 0d7e994c092abbb0a21e7d243114efa5ba452b8c
packet:  upload-pack< have 9e22a6c1b441ee1bcd54b8da801261ba8b15eac9
packet:  upload-pack< done
packet:  upload-pack> NAK
packet:fetch< NAK
packet: sideband< \2Enumerating objects: 3, done.Counting objects:  33% 
(1/3)   \15Counting objects:  66% (2/3)   \15Counting objects: 100% (3/3)   
\15Co
packet: sideband< \2unting objects: 100% (3/3), done.Total 3 (delta 0), 
reused 0 (delta 0)
packet: sideband< PACK ...
packet:  upload-pack> 
packet: sideband< 

Re: [PATCH v2] pack-objects: fix performance issues on packing large deltas

[Johannes Sixt]

Am 22.07.2018 um 10:04 schrieb Nguyễn Thái Ngọc Duy:

+   if (size < pack->oe_delta_size_limit) {
+   e->delta_size_ = size;
+   e->delta_size_valid = 1;
+   } else {
+   packing_data_lock(pack);
+   if (!pack->delta_size)
+   ALLOC_ARRAY(pack->delta_size, pack->nr_alloc);
+   packing_data_unlock(pack);
+
+   pack->delta_size[e - pack->objects] = size;


My first thought was that this is wrong (falling prey to the same 
mistake as the double-checked locking pattern). But after thinking twice 
over it again, I think that this unprotected access of pack->delta_size 
is thread-safe.


Of course, I'm assuming that different threads never assign to the same 
array index.



+   e->delta_size_valid = 0;
+   }
  }


-- Hannes

Re: [PATCH 7/9] vscode: use 8-space tabs, no trailing ws, etc for Git's source code

[Johannes Sixt]

Am 23.07.2018 um 15:52 schrieb Johannes Schindelin via GitGitGadget:

From: Johannes Schindelin 

This adds a couple settings for the .c/.h files so that it is easier to
conform to Git's conventions while editing the source code.

Signed-off-by: Johannes Schindelin 
---
  contrib/vscode/init.sh | 8 
  1 file changed, 8 insertions(+)

diff --git a/contrib/vscode/init.sh b/contrib/vscode/init.sh
index face115e8..29f2a729d 100755
--- a/contrib/vscode/init.sh
+++ b/contrib/vscode/init.sh
@@ -21,6 +21,14 @@ cat >.vscode/settings.json.new <<\EOF ||
  "editor.wordWrap": "wordWrapColumn",
  "editor.wordWrapColumn": 72
  },
+"[c]": {
+"editor.detectIndentation": false,
+"editor.insertSpaces": false,
+"editor.tabSize": 8,
+"editor.wordWrap": "wordWrapColumn",
+"editor.wordWrapColumn": 80,
+"files.trimTrailingWhitespace": true
+},


I am a VS Code user, but I haven't used these settings before.

With these settings, does the editor break lines while I am typing? Or 
does it just insert a visual cue that tells where I should insert a line 
break? If the former, it would basically make the editor unusable for my 
taste. I want to have total control over the code I write. The 80 column 
limit is just a recommendation, not a hard requirement.



  "files.associations": {
  "*.h": "c",
  "*.c": "c"



-- Hannes

Re: [PATCH 0/3] rebase -r: support octopus merges

[Johannes Sixt]

Am 12.07.2018 um 18:26 schrieb Junio C Hamano:

Johannes Schindelin  writes:

A much more meaningful measure would be: how many octopus merge commits
have been pushed to GitHub in the past two weeks. I don't think I have the
technical means to answer that question, though.


It does not mean that misusing a feature is a good thing and should
be encouraged if many misguided people do so.


Just recently I had to rebuild the version of git-gui that comes with 
Git 2.18.0 before it was released:


https://github.com/j6t/git-gui-ng/commit/f07ae1d7f07b036d78a3d4706e6cb4102e623fb3

I think that an octopus merge is the right tool for the task. Am I 
misguided?


-- Hannes

Re: [PATCH v2 1/2] t3418: add testcase showing problems with rebase -i and strategy options

[Johannes Sixt]

Am 27.06.2018 um 19:27 schrieb Elijah Newren:

On Wed, Jun 27, 2018 at 9:54 AM, Junio C Hamano  wrote:

Having said that, it would be simpler for at least the latter to
write it using a single-shot environment assignment, perhaps?  I.e.

 PATH=./test-bin:$PATH git rebase --continue &&

without running in a subshell?


Seems reasonable.  Since these tests were essentially copies of other
tests within the same file, just for rebase -i instead of -m, should I
also add another patch to the series fixing up the rebase -m testcase
to also replace the subshell with a single-shot environment
assignment?


Pitfalls ahead!

PATH=... git rebase ...

is OK, but

PATH=... test_must_fail git rebase ...

is not; the latter requires the subshell, otherwise the modified PATH 
variable survives the command because test_must_fail is a shell 
function. Yes, it's silly, but that's how it is.


-- Hannes

Re: [PATCH 29/29] t/test-lib: teach --chain-lint to detect broken &&-chains in subshells

[Johannes Sixt]

Am 27.06.2018 um 04:15 schrieb Elijah Newren:

On Tue, Jun 26, 2018 at 2:01 PM, Jeff King  wrote:

On Tue, Jun 26, 2018 at 04:46:18PM -0400, Eric Sunshine wrote:



Some of these dangers can be de-thoothed during the linting phase by
defining do-nothing shell functions:

 cp () { :; }
 mv () { :; }
 ln () { :; }

That, at least, makes the scariest case ("rm") much less so.


Now that's an interesting idea. We can't catch every dangerous action
(notably ">" would be hard to override), but it should be pretty cheap
to cover some obvious ones.

-Peff


Crazy idea: maybe we could defang it a little more thoroughly with
something like the following (apologies in advance if gmail whitespace
damages this):

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 28315706be..7fda08a90a 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -675,7 +675,7 @@ test_run_ () {
 trace=
 # 117 is magic because it is unlikely to match the exit
 # code of other programs
-   if test "OK-117" != "$(test_eval_ "(exit 117) &&
$1${LF}${LF}echo OK-\$?" 3>&1)"
+   if test "OK-117" != "$(test_eval_ "cd() { return 0; }
&& PATH=/dev/null && export PATH && (exit 117) && $1${LF}${LF}echo
OK-\$?" 3>&1)"
 then
 error "bug in the test script: broken &&-chain
or run-away HERE-DOC: $1"
 fi


I'd define all these functions as { return 1; } because we want to stop 
any && chain as early as possible (and with an exit code that is not the 
sentinel value).


-- Hannes

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Johannes Sixt]

Am 26.06.2018 um 20:14 schrieb Eric Sunshine:

On Tue, Jun 26, 2018 at 2:06 PM Johannes Sixt  wrote:

Hence, these lines should actually be

 p4 help client &&
 ! p4 help nosuchcommand


Thanks for the comment; you're right, of course. I'll certainly make
this change if I have to re-roll for some other reason, but do you
feel that this itself is worth a re-roll?


Not worth a re-roll IMO.

-- Hannes

Re: [PATCH 17/29] t: use test_must_fail() instead of checking exit code manually

[Johannes Sixt]

Am 26.06.2018 um 11:21 schrieb Eric Sunshine:

On Tue, Jun 26, 2018 at 4:58 AM Elijah Newren  wrote:

On Tue, Jun 26, 2018 at 12:29 AM, Eric Sunshine  wrote:

+   p4 help client &&
+   test_must_fail p4 help nosuchcommand


same question?


Same answer. Not shown in this patch, but just above the context lines
you will find this comment in the file:

 # We rely on this behavior to detect for p4 move availability.

which means that the test is really interested in being able to
reliably detect if a sub-command is or is not available. So, despite
the (somewhat) misleading test title, this test doesn't care about the
exact error code but rather cares only that "p4 help nosuchcommand"
errors out, period. Hence, test_must_fail() again agrees with the
spirit of the test.


test_must_fail ensures that only "proper" failures are diagnosed as 
expected; failures due to signals such as SEGV are not expected failures.


In the test suite we expect all programs that are not our "git" to work 
correctly; in particular, that they do not crash on anything that we ask 
them to operate on. Under this assumption, the protection given by 
test_must_fail is not needed.


Hence, these lines should actually be

p4 help client &&
! p4 help nosuchcommand

-- Hannes

Re: Unexpected ignorecase=false behavior on Windows

[Johannes Sixt]

Am 22.06.2018 um 14:04 schrieb Marc Strapetz:

On Windows, when creating following repository:

$ git init
$ echo "1" > file.txt
$ git add .
$ git commit -m "initial import"
$ ren file.txt File.txt
$ git config core.ignorecase false


This is a user error. core.ignorecase is *not* an instruction as in 
"hey, Git, do not ignore the case of file names". It is better regarded 
as an internal value, with which Git remembers how it should treat the 
names of files that it receives when it traverses the directories on the 
disk.


Git could probe the file system capabilities each time it runs. But that 
would be wasteful. Hence, this probe happens only once when the 
repository is initialized, and the result is recorded in this 
configuration value. You should not change it.



Status results are:

$ git status --porcelain
?? File.txt

As on Unix, I would expect to see:

$ git status --porcelain
  D file.txt
?? File.txt

Is the Windows behavior intended? I'm asking, because e.g. JGit will 
report missing files, too, and I'm wondering what would be the correct 
way to resolve this discrepancy? (JGit does not have 
"ignorecase=true"-support at all, btw).


Then I don't think there is a way to remove the discrepancy.

-- Hannes

Re: want option to git-rebase

[Johannes Sixt]

Am 19.06.2018 um 03:06 schrieb Jonathan Nieder:

Ian Jackson wrote[1]:

git-rebase leaves entries like this in the reflog:

   c15f4d5391 HEAD@{33}: rebase: checkout 
c15f4d5391ff07a718431aca68a73e672fe8870e

It would be nice if there were an option to control this message.
Particularly, when another tool invokes git-rebase, the other tool may
specify an interesting --onto, and there is no way to record any
information about that --onto commit.

git-rebase already has a -m option, so I suggest
   --reason=

It doesn't matter much exactly how the provided string is used.
Any of the following would be good IMO:
   
   rebase start: 


 From git(1):

  GIT_REFLOG_ACTION
When a ref is updated, reflog entries are created to keep
track of the reason why the ref was updated (which is
typically the name of the high-level command that updated the
ref), in addition to the old and new values of the ref. A
scripted Porcelain command can use set_reflog_action helper
function in git-sh-setup to set its name to this variable when
it is invoked as the top level command by the end user, to be
recorded in the body of the reflog.

"git rebase" sets this itself, so it doesn't solve your problem.


If it does so unconditionally, then that is a bug. If a script wants to 
set GIT_REFLOG_ACTION, but finds that it is already set, then it must 
not change the value. set_reflog_action in git-sh-setup does the right 
thing.


So, if there is another script or application around git-rebase, then it 
should just set GIT_REFLOG_ACTION (if it is not already set) and export 
the environment variable to git-rebase.


-- Hannes

Re: git-gui ignores core.hooksPath

[Johannes Sixt]

Am 11.06.2018 um 23:58 schrieb Stefan Beller:

On Mon, Jun 4, 2018 at 10:48 PM Bert Wesarg  wrote:

the last time this topic came up, Stefan (in Cc) offered to volunteer.
Stefan, is this offer still open? I would support this.


After I made this offer, I started looking at the code base more and trying
to add a feature just to discover I do not qualify as fluent in tcl/tk.
Also I have some issues managing my time, so I retract that offer.
Though I'd still review the code in this area.


Maybe it's of interest to somebody: My current pet fun project is this 
one: https://github.com/j6t/git-gui-ng -- rewrite to C++ using Tk as GUI 
toolkit.


I use git gui (the original) daily, but no, I don't volunteer to keep it 
going. Tcl is just too exotic.


-- Hannes

Re: [RFC PATCH v1] telemetry design overview (part 1)

[Johannes Sixt]

Am 09.06.2018 um 22:43 schrieb Johannes Schindelin:

On Sat, 9 Jun 2018, Johannes Sixt wrote:

When you want usage data, ask your users for feedback. Look over their
shoulders. But do not ask the software itself to gather usage data. It will be
abused.

Do not offer open source software that has a "call-home" method built-in.

If you want to peek into the workplaces of YOUR users, then monkey-patch
survaillance into YOUR version of Git. But please do not burden the rest of
us.


We already offer hooks. You can do anything with those hooks. Even, if you
do not pay close attention, to transfer all your bitcoin to a specific
account.

I agree with Peff: this is something you as a user need to be aware of,
and need to make sure you configure your Git just like you want. As long
as this is a purely opt-in feature, it is useful and helpful.


The problem with this feature is not so much that it enables someone to 
do bad things, but that it is specifically targeted at recording *how 
users use Git*.



We do need it in-house, for the many thousands of Git users we try to
support with a relatively small team of Git developers.


Then please build your private version and sell it to your thousands of 
Git users. "in-house" == Microsoft? Small team of Git developers? So, 
instead of shelling out the bucks for this extra burden, they put the 
burden on the Community?


-- Hannes

Re: [RFC PATCH v1] telemetry design overview (part 1)

[Johannes Sixt]

Am 09.06.2018 um 08:51 schrieb Jeff King:

I actually think this could be useful for normal users, too. We have
GIT_TRACE for dumping debug output, and we sometimes ask users to turn
it on to help diagnose a problem (not to mention that we use it
ourselves).


The BIG difference is in "we ask the users". Asking for a trace is a 
one-shot thing; this telemetry thing is obviously intended for long-term 
survaillance.



AFAICT this telemetry data is the same thing, but for performance. When
somebody says "why does this command take so long", wouldn't it be nice
for us to be able to tell them to flip a switch that will collect data
on which operations are taking a long time?


Why do we need long-term survaillance to answer this question and why 
can it not be made a mode of GIT_TRACE?


-- Hannes

Re: [RFC PATCH v1] telemetry design overview (part 1)

[Johannes Sixt]

Am 09.06.2018 um 00:20 schrieb Ævar Arnfjörð Bjarmason:


On Fri, Jun 08 2018, Johannes Sixt wrote:


Am 08.06.2018 um 18:00 schrieb Thomas Braun:

I for my part would much rather prefer that to be a compile time
option so that I don't need to check on every git update on windows
if  this is now enabled or not.


This exactly my concern, too! A compile-time option may make it a good
deal less worrisome.


Can you elaborate on how someone who can maintain inject malicious code
into your git package + config would be thwarted by this being some
compile-time option, wouldn't they just compile it in?


Of course they can. But would we, the Git community do that?

From the design document:

> The goal of the telemetry feature is to be able to gather usage data
> across a group of production users to identify real-world performance
> problems in production.  Additionally, it might help identify common
> user errors and guide future user training.

The goal to gather usage data may be valid for a small subset of Git 
installations. But it is wrong to put this into the software itself, in 
particular when the implementations includes scary things like loading 
unspecified dynamic libraries:


> If the config setting "telemetry.plugin" contains the pathname to a
> shared library, the library will be dynamically loaded during start up
> and events will be sent to it using the plugin API.

When you want usage data, ask your users for feedback. Look over their 
shoulders. But do not ask the software itself to gather usage data. It 
will be abused.


Do not offer open source software that has a "call-home" method built-in.

If you want to peek into the workplaces of YOUR users, then monkey-patch 
survaillance into YOUR version of Git. But please do not burden the rest 
of us.


-- Hannes

Re: GDPR compliance best practices?

[Johannes Sixt]

Am 08.06.2018 um 04:53 schrieb Theodore Y. Ts'o:

And of course, that's the other thing you seem to fundamentally not
understand about how git works.  Every developer in the world working
on that open source project has their own copy.


Everyone here understands how Git works, of course.

"*shrug* but that's how Git works" does *NOT* override the GDPR.

-- Hannes

Re: [RFC PATCH v1] telemetry design overview (part 1)

[Johannes Sixt]

Am 08.06.2018 um 18:00 schrieb Thomas Braun:

I for my part would much rather prefer that to be a compile time
option so that I don't need to check on every git update on windows
if  this is now enabled or not.


This exactly my concern, too! A compile-time option may make it a good 
deal less worrisome.


-- Hannes

Re: [RFC PATCH v1] telemetry design overview (part 1)

[Johannes Sixt]

Am 07.06.2018 um 16:53 schrieb g...@jeffhostetler.com:

From: Jeff Hostetler 

I've been working to add code to Git to optionally collect telemetry data.
The goal is to be able to collect performance data from Git commands and
allow it to be aggregated over a user community to find "slow commands".


Seriously? "add code to collect telemetry data" said by somebody whose 
email address ends with @microsoft.com is very irritating. I really 
don't want to have yet another switch that I must check after every 
update that it is still off.


-- Hannes

Re: how exactly can git config section names contain periods?

[Johannes Sixt]

Am 03.06.2018 um 11:53 schrieb Robert P. J. Day:

if i wanted to do something this admittedly awkward, would using
periods give me some benefit related to, i don't know, regex matching,
as compared to using a different character? or am i just way
overthinking this? is anyone out there actually taking advantage of
multi-level subsections?


There is nothing wrong with having all of these

git config foo.a.b.c.key value
git config foo.a.b.c value
git config foo.a.b.key value

in a single configuration file, I think. There is nothing special there. 
They're three different settings in two different (sub-)sections.


-- Hannes

Re: is there a reason pre-commit.sample uses "git diff-index"?

[Johannes Sixt]

Am 31.05.2018 um 19:27 schrieb Robert P. J. Day:

On Thu, 31 May 2018, Duy Nguyen wrote:

git diff-index is "plumbing", designed for writing scripts. "git
diff" on the other hand is for users and its behavior may change
even if it breaks backward compatibility.


   ah, this was a philosophical underpinning i was unaware of. i see
occasional explanations of git porcelain versus plumbing, but i don't
recall anyone simply stating that the plumbing is meant to have a
long-term stability that is not guaranteed for the porcelain.


So, there you have it. ;) Plumbing commands offer long-term stability. 
That is not just philosophical, but practically relevant.



   in any event, this does mean that, stability issues aside, "git
diff" would apparently have worked just fine for that hook.


It may have worked just fine. You should still not use it.

Didn't you say that you are teaching git and hooks? Then you should 
teach the right thing, and the right thing is to use plumbing for scripts.


-- Hannes

Re: [PATCH 5/5] merge-recursive: simplify handle_change_delete

[Johannes Sixt]

Am 19.05.2018 um 04:07 schrieb Elijah Newren:

There is really no need for four branches of nearly identical messages
when we can store the differences into small variables before printing.


Oh, there is a reason for the repeated message text: translations! 
Please do not play sentence Lego with translated strings. The original 
code is preferable.



It does require a few allocations this way, but makes the code much
easier to parse for human readers.

Signed-off-by: Elijah Newren 
---
  merge-recursive.c | 36 +++-
  1 file changed, 11 insertions(+), 25 deletions(-)

diff --git a/merge-recursive.c b/merge-recursive.c
index 273ee79afa..3bd727995b 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1290,31 +1290,17 @@ static int handle_change_delete(struct merge_options *o,
if (!ret)
ret = update_file(o, 0, o_oid, o_mode, update_path);
} else {
-   if (!alt_path) {
-   if (!old_path) {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s in %s. Version %s of %s left in 
tree."),
-  change, path, delete_branch, change_past,
-  change_branch, change_branch, path);
-   } else {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s to %s in %s. Version %s of %s left in 
tree."),
-  change, old_path, delete_branch, 
change_past, path,
-  change_branch, change_branch, path);
-   }
-   } else {
-   if (!old_path) {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s in %s. Version %s of %s left in tree 
at %s."),
-  change, path, delete_branch, change_past,
-  change_branch, change_branch, path, 
alt_path);
-   } else {
-   output(o, 1, _("CONFLICT (%s/delete): %s deleted in 
%s "
-  "and %s to %s in %s. Version %s of %s left in 
tree at %s."),
-  change, old_path, delete_branch, 
change_past, path,
-  change_branch, change_branch, path, 
alt_path);
-   }
-   }
+   const char *deleted_path = old_path ? old_path : path;
+   char *supp1 = xstrfmt(old_path ? " to %s" : "", path);
+   char *supp2 = xstrfmt(alt_path ? " at %s" : "", alt_path);
+
+   output(o, 1, _("CONFLICT (%s/delete): %s deleted in %s "
+  "and %s%s in %s. Version %s of %s left in tree%s."),
+  change, deleted_path, delete_branch, change_past,
+  supp1, change_branch, change_branch, path, supp2);
+   free(supp1);
+   free(supp2);
+
/*
 * No need to call update_file() on path when change_branch ==
 * o->branch1 && !alt_path, since that would needlessly touch

Re: Git log range reverse bug

[Johannes Sixt]

Am 16.05.2018 um 20:19 schrieb Mehdi Zeinali:

Git Version: Version: 2.14.2

When reversing a range in git log, it does not start from the expected commit:


--reverse does not change the meaning A..B to B..A or something. For a 
particular A..B specification, the set of commits selected when 
--reverse is given remains the same. Only the order in which they are 
listed is reversed.


-- Hannes

Re: [PATCH v2 17/28] t4014: abstract away SHA-1-specific constants

[Johannes Sixt]

Am 13.05.2018 um 04:24 schrieb brian m. carlson:

Adjust the test so that it computes values for blobs instead of using
hard-coded hashes.

Signed-off-by: brian m. carlson 
---
  t/t4014-format-patch.sh | 7 ---
  1 file changed, 4 insertions(+), 3 deletions(-)

diff --git a/t/t4014-format-patch.sh b/t/t4014-format-patch.sh
index dac3f349a3..42b3e11207 100755
--- a/t/t4014-format-patch.sh
+++ b/t/t4014-format-patch.sh
@@ -578,7 +578,9 @@ test_expect_success 'excessive subject' '
  
  	rm -rf patches/ &&

git checkout side &&
+   before=$(git rev-parse --short $(git hash-object file)) &&
for i in 5 6 1 2 3 A 4 B C 7 8 9 10 D E F; do echo "$i"; done >>file &&
+   after=$(git rev-parse --short $(git hash-object file)) &&


It would be better to avoid process expansion in command arguments, 
because the shell does not diagnose failures. This is preferable:


before=$(git hash-object file) &&
before=$(git rev-parse --short $before) &&

-- Hannes

[PATCH v2] git: add -P as a short option for --no-pager

[Johannes Sixt]

It is possible to configure 'less', the pager, to use an alternate
screen to show the content, for example, by setting LESS=RS in the
environment. When it is closed in this configuration, it switches
back to the original screen, and all content is gone.

It is not uncommon to request that the output remains visible in
the terminal. For this, the option --no-pager can be used. But
it is a bit cumbersome to type, even when command completion is
available. Provide a short option, -P, to make the option easier
accessible.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 Given the positive feedback, I resurrect the patch.

 Changes since v1:
 - Use -P instead of -N
 - Commit message changed as proposed by Kaartic

 Documentation/git.txt | 3 ++-
 git.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..c662f41c1d 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git' [--version] [--help] [-C ] [-c =]
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
-[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+[-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
 [--git-dir=] [--work-tree=] [--namespace=]
 [--super-prefix=]
  []
@@ -103,6 +103,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
configuration options (see the "Configuration Mechanism" section
below).
 
+-P::
 --no-pager::
Do not pipe Git output into a pager.
 
diff --git a/git.c b/git.c
index ceaa58ef40..71d013424e 100644
--- a/git.c
+++ b/git.c
@@ -7,7 +7,7 @@
 const char git_usage_string[] =
N_("git [--version] [--help] [-C ] [-c =]\n"
   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [-p | --paginate | -P | --no-pager] 
[--no-replace-objects] [--bare]\n"
   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
   "[]");
 
@@ -81,7 +81,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
exit(0);
} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
use_pager = 1;
-   } else if (!strcmp(cmd, "--no-pager")) {
+   } else if (!strcmp(cmd, "-P") || !strcmp(cmd, "--no-pager")) {
use_pager = 0;
if (envchanged)
*envchanged = 1;
-- 
2.17.0.69.g0c1d01d9b6

Re: js/no-pager-shorthand [was: What's cooking in git.git (Apr 2018, #04; Mon, 30)]

[Johannes Sixt]

Am 01.05.2018 um 13:57 schrieb Johannes Schindelin:

Hi Hannes,

On Mon, 30 Apr 2018, Johannes Sixt wrote:


Am 30.04.2018 um 05:25 schrieb Junio C Hamano:

* js/no-pager-shorthand (2018-04-25) 1 commit
   - git: add -N as a short option for --no-pager

   "git --no-pager cmd" did not have short-and-sweet single letter
   option. Now it does.

   Will merge to 'next'.


I consider your argument that -N is only an abbreviation for an unspecific
"no" a valid one. So, I would like to be sure that we are not painting us into
the wrong corner by squatting -N for --no-pager.

I find -P is not that bad after all.


To me, `-P` would suggest the positive action --pager rather than the
negative --no-pager.

I wonder whether `-!p` would be better/feasible?


We do not have this pattern, yet, nor do I know it from some other 
utility. I do not want to set a precedent.



Your use case is quite the corner case, I hope you realize that, as it
seems that everybody else is fine with having -FRX as default options for
`less`... And with copy/pasting from the `less` output. So introducing a
sweet short option for --no-pager, for the benefit of maybe even only one
user, seems quite... unusual.

Granted, you cannot simply introduce an alias for `git --no-pager`. But
maybe that is what we should do? Maybe we should start supporting aliases
without specifying commands, opening the door for things like `git -c
ui.color=false`, too.

Then you could add `alias.n=--no-pager` and call `git n show HEAD`, and
the -N and -P short options could still wait for a widely-popular option
to require a short name.


But then I can just as well have alias gitn='git --no-pager' in my 
.bashrc. Maybe I should do that.


Given the ambivalence (or inconclusiveness), I retract this patch 
without offering a replacement for the time being.


Thanks for a bit of sanity,
-- Hannes

Re: [PATCH 2/6] t1406: prepare for the refs code to fail with BUG()

[Johannes Sixt]

Am 30.04.2018 um 00:17 schrieb Johannes Schindelin:

t1406 specifically verifies that certain code paths fail with a BUG: ...
message.

In the upcoming commit, we will convert that message to be generated via
BUG() instead of die("BUG: ..."), which implies SIGABRT instead of a
regular exit code.

Signed-off-by: Johannes Schindelin 
---
  t/t1406-submodule-ref-store.sh | 15 +--
  1 file changed, 9 insertions(+), 6 deletions(-)

diff --git a/t/t1406-submodule-ref-store.sh b/t/t1406-submodule-ref-store.sh
index e093782cc37..0ea3457cae3 100755
--- a/t/t1406-submodule-ref-store.sh
+++ b/t/t1406-submodule-ref-store.sh
@@ -16,7 +16,7 @@ test_expect_success 'setup' '
  '
  
  test_expect_success 'pack_refs() not allowed' '

-   test_must_fail $RUN pack-refs 3
+   test_must_fail ok=sigabrt $RUN pack-refs 3
  '
  
  test_expect_success 'peel_ref(new-tag)' '

@@ -27,15 +27,18 @@ test_expect_success 'peel_ref(new-tag)' '
  '
  
  test_expect_success 'create_symref() not allowed' '

-   test_must_fail $RUN create-symref FOO refs/heads/master nothing
+   test_must_fail ok=sigabrt \
+   $RUN create-symref FOO refs/heads/master nothing
  '
  
  test_expect_success 'delete_refs() not allowed' '

-   test_must_fail $RUN delete-refs 0 nothing FOO refs/tags/new-tag
+   test_must_fail ok=sigabrt \
+   $RUN delete-refs 0 nothing FOO refs/tags/new-tag
  '
  
  test_expect_success 'rename_refs() not allowed' '

-   test_must_fail $RUN rename-ref refs/heads/master refs/heads/new-master
+   test_must_fail ok=sigabrt \
+   $RUN rename-ref refs/heads/master refs/heads/new-master
  '
  
  test_expect_success 'for_each_ref(refs/heads/)' '

@@ -91,11 +94,11 @@ test_expect_success 'reflog_exists(HEAD)' '
  '
  
  test_expect_success 'delete_reflog() not allowed' '

-   test_must_fail $RUN delete-reflog HEAD
+   test_must_fail ok=sigabrt $RUN delete-reflog HEAD
  '
  
  test_expect_success 'create-reflog() not allowed' '

-   test_must_fail $RUN create-reflog HEAD 1
+   test_must_fail ok=sigabrt $RUN create-reflog HEAD 1
  '


I can't quite follow the rationale for this change. A 'BUG' error exit 
must never be reached, otherwise it is a bug in the program by 
definition. It cannot be OK that SIGABRT is a valid result from Git.


If SIGABRT occurs as a result of BUG(), and we know that this happens 
for certain cases, it means we have an unfixed bug. Should then not run 
these cases under test_expect_failure instead of test_expect_success to 
identify them as known bugs?


Confused.

-- Hannes

Re: [PATCH v1] test-drop-caches: simplify delay loading of NtSetSystemInformation

[Johannes Sixt]

Am 30.04.2018 um 16:26 schrieb Ben Peart:

@@ -82,8 +83,6 @@ static int cmd_dropcaches(void)
  {
HANDLE hProcess = GetCurrentProcess();
HANDLE hToken;
-   HMODULE ntdll;
-   DWORD(WINAPI *NtSetSystemInformation)(INT, PVOID, ULONG);
SYSTEM_MEMORY_LIST_COMMAND command;
int status;
  
@@ -95,14 +94,9 @@ static int cmd_dropcaches(void)
  
  	CloseHandle(hToken);
  
-	ntdll = LoadLibrary("ntdll.dll");

-   if (!ntdll)
-   return error("Can't load ntdll.dll, wrong Windows version?");
-
-   NtSetSystemInformation =
-   (DWORD(WINAPI *)(INT, PVOID, ULONG))GetProcAddress(ntdll, 
"NtSetSystemInformation");
-   if (!NtSetSystemInformation)
-   return error("Can't get function addresses, wrong Windows 
version?");
+   DECLARE_PROC_ADDR(ntdll.dll, DWORD, NtSetSystemInformation, INT, PVOID, 
ULONG);


This is a declaration-after-statement. Even though this is not in 
generic code, it is all too easy to trigger a warning from GCC if a 
corresponding warning is turned on.



+   if (!INIT_PROC_ADDR(NtSetSystemInformation))
+   return error("Could not find NtSetSystemInformation() 
function");
  
  	command = MemoryPurgeStandbyList;

status = NtSetSystemInformation(
@@ -115,8 +109,6 @@ static int cmd_dropcaches(void)
else if (status != STATUS_SUCCESS)
error("Unable to execute the memory list command %d", status);
  
-	FreeLibrary(ntdll);

-
return status;
  }


-- Hannes

js/no-pager-shorthand [was: What's cooking in git.git (Apr 2018, #04; Mon, 30)]

[Johannes Sixt]

Am 30.04.2018 um 05:25 schrieb Junio C Hamano:

* js/no-pager-shorthand (2018-04-25) 1 commit
  - git: add -N as a short option for --no-pager

  "git --no-pager cmd" did not have short-and-sweet single letter
  option. Now it does.

  Will merge to 'next'.


I consider your argument that -N is only an abbreviation for an 
unspecific "no" a valid one. So, I would like to be sure that we are not 
painting us into the wrong corner by squatting -N for --no-pager.


I find -P is not that bad after all.

-- Hannes

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 11:21 schrieb Phillip Wood:

On 24/04/18 17:59, Johannes Sixt wrote:


In modern setups, less, the pager, uses alternate screen to show
the content. When it is closed, it switches back to the original
screen, and all content is gone.


Are you setting LESS explicitly in the environment?

 From the git config man page:
When the LESS environment variable is unset, Git sets it to FRX (if LESS 
environment variable is set, Git does not change it at all).


 From the less man page the X option:
Disables  sending the termcap initialization and deinitialization 
strings to the terminal.  This is sometimes desirable if the 
deinitialization string does something unnecessary, like clearing the 
screen.


So with the default setup the output should remain on the screen.


Right. But I have LESS=RS because I do *not* want it to remain on screen 
by default.


-- Hannes

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 09:41 schrieb Johannes Schindelin:

Hi Hannes,

On Wed, 25 Apr 2018, Johannes Sixt wrote:


Am 25.04.2018 um 02:05 schrieb Junio C Hamano:

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

It is not uncommon to request that the output remains visible in
the terminal.


I ran `git log` and then hit `q`, and the latest screen contents were
still visible in all of these scenarios:

- in a Linux VM via SSH client

- in Git Bash on Windows

- in Git CMD on Windows

Granted, this is only the latest screen, but that is usually good enough
here.

Is anything different happening in your setups?


I have LESS=RS in my environment, because I do want that the alternate 
screen is used by the pager. Nevertheless, occasionally I want git's 
output to remain visible.


-- Hannes

Re: [PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

Am 25.04.2018 um 02:05 schrieb Junio C Hamano:

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

It is not uncommon to request that the output remains visible in
the terminal. For this, the option --no-pager can be used. But
it is a bit cumbersome to type, even when command completion is
available. Provide a short option, -N, to make the option easier
accessible.

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


Heh, I used to append "|cat", which is four keystrokes that is a bit
shorter than " --no-pager", but that is only acceptable when you do
not care about colored output ;-)

I am not absolutely certain about the choice of a single letter. I
already checked we do not use "git -N cmd" for anything else right
now, so I am certain about the availability, but I am not sure if
capital 'N' is the best choice, when the other side is lowercase 'p'
(and more importantly, the other side 'p' has mneomonic value for
'pagination', but 'N' merely stands for 'no' and could be negating
anything, not related to pagination). But I agree that a short-hand
would be welcome.


I considered -P, but thought that it would better be reserved for 
something related to "paths". We have --{html,man,info}-path, which 
would be served better with -[HMI]. That leaves --exec-path, which we 
would probably abbreviate as -x or -X. So, -P is perhaps not that bad 
after all.


We could also choose +p, for which there is some precedent in other 
POSIX tools to mean negated -p, but not in git, I think. 
Implementationwise, it is not that trivial, either, because the section 
handling options is guarded by a check that the word begins with a dash.


Perhaps --no-pager means also "unpaginated" (-u, -U), "linear" (-l, -L), 
"streamed" (-s, -S). Other ideas, anyone?





diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..17b50b0dc6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
  [verse]
  'git' [--version] [--help] [-C ] [-c =]
  [--exec-path[=]] [--html-path] [--man-path] [--info-path]
-[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+[-p|--paginate|-N|--no-pager] [--no-replace-objects] [--bare]
  [--git-dir=] [--work-tree=] [--namespace=]
  [--super-prefix=]
   []

[PATCH] git: add -N as a short option for --no-pager

[Johannes Sixt]

In modern setups, less, the pager, uses alternate screen to show
the content. When it is closed, it switches back to the original
screen, and all content is gone.

It is not uncommon to request that the output remains visible in
the terminal. For this, the option --no-pager can be used. But
it is a bit cumbersome to type, even when command completion is
available. Provide a short option, -N, to make the option easier
accessible.

Signed-off-by: Johannes Sixt <j...@kdbg.org>
---
 Documentation/git.txt | 3 ++-
 git.c | 4 ++--
 2 files changed, 4 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 4767860e72..17b50b0dc6 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git' [--version] [--help] [-C ] [-c =]
 [--exec-path[=]] [--html-path] [--man-path] [--info-path]
-[-p|--paginate|--no-pager] [--no-replace-objects] [--bare]
+[-p|--paginate|-N|--no-pager] [--no-replace-objects] [--bare]
 [--git-dir=] [--work-tree=] [--namespace=]
 [--super-prefix=]
  []
@@ -103,6 +103,7 @@ foo.bar= ...`) sets `foo.bar` to the empty string which 
`git config
configuration options (see the "Configuration Mechanism" section
below).
 
+-N::
 --no-pager::
Do not pipe Git output into a pager.
 
diff --git a/git.c b/git.c
index ceaa58ef40..9e2d78f442 100644
--- a/git.c
+++ b/git.c
@@ -7,7 +7,7 @@
 const char git_usage_string[] =
N_("git [--version] [--help] [-C ] [-c =]\n"
   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [-p | --paginate | -N | --no-pager] 
[--no-replace-objects] [--bare]\n"
   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
   "[]");
 
@@ -81,7 +81,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
exit(0);
} else if (!strcmp(cmd, "-p") || !strcmp(cmd, "--paginate")) {
use_pager = 1;
-   } else if (!strcmp(cmd, "--no-pager")) {
+   } else if (!strcmp(cmd, "-N") || !strcmp(cmd, "--no-pager")) {
use_pager = 0;
if (envchanged)
*envchanged = 1;
-- 
2.17.0.69.g0c1d01d9b6

squash! Merge branch 'ps/test-chmtime-get'

[Johannes Sixt]

Junio,

you may want to squash this into your merge commit of branch
ps/test-chmtime-get (today it is fa57c0871fc9)

-- Hannes

diff --git a/t/helper/test-chmtime.c b/t/helper/test-chmtime.c
index daeddc1cbc..aa22af48c2 100644
--- a/t/helper/test-chmtime.c
+++ b/t/helper/test-chmtime.c
@@ -25,7 +25,7 @@
  *
  * To print only the mtime use --get:
  *
- * test-chmtime --get file
+ * test-tool chmtime --get file
  *
  * To set the mtime to current time:
  *
@@ -33,7 +33,7 @@
  *
  * To set the file mtime offset to +1 and print the new value:
  *
- * test-chmtime --get +1 file
+ * test-tool chmtime --get +1 file
  *
  */
 #include "test-tool.h"

Re: [RFC PATCH] ident: don't cache default date

[Johannes Sixt]

Am 18.04.2018 um 19:47 schrieb Phillip Wood:
> On 18/04/18 12:27, Ævar Arnfjörð Bjarmason wrote:
>> On Wed, Apr 18 2018, Phillip Wood wrote:
>>> From: Phillip Wood <phillip.w...@dunelm.org.uk>
>>> as it is created by running an separate instance of 'git commit'.  If
>>> the reworded commit is follow by further picks, those later commits
>>> will have an earlier committer date than the reworded one. This is
>>> caused by git caching the default date used when GIT_COMMITTER_DATE is
>>> not set. Fix this by not caching the date.
>>>
>>> Users expect commits to have the same author and committer dates when
>>> the don't explicitly set them. As the date is now updated each time
>>> git_author_info() or git_committer_info() is run it is possible to end
>>> up with different author and committer dates. Fix this for
>>> 'commit-tree', 'notes' and 'merge' by using a single date in
>>> commit_tree_extended() and passing it explicitly to the new functions
>>> git_author_info_with_date() and git_committer_info_with_date() when
>>> neither the author date nor the committer date are explicitly
>>> set. 'commit' always passes the author date to commit_tree_extended()
>>> and relied on the date caching to have the same committer and author
>>> dates when neither was specified. Fix this by setting
>>> GIT_COMMITTER_DATE to be the same as the author date passed to
>>> commit_tree_extended().
>>>
>>> Signed-off-by: Phillip Wood <phillip.w...@dunelm.org.uk>
>>> Reported-by: Johannes Sixt <j...@kdbg.org>
>>> ---
>>>
>>> I'm slightly nervous that setting GIT_COMMITTER_DATE in
>>> builtin/commit.c will break someone's hook script. Maybe it would be
>>> better to add a committer parameter to commit_tree() and
>>> commit_tree_extended().

While I like the basic theme of your patch, I think we should fix this
case in a much simpler way, namely, use the infrastructure that was
introduced for git-am.

I've shamelessly lifted the commit message from your patch.

 8< 
Subject: [PATCH] sequencer: reset the committer date before commits

Now that the sequencer commits without forking when the commit message
isn't edited all the commits that are picked have the same committer
date. If a commit is reworded it's committer date will be a later time
as it is created by running an separate instance of 'git commit'.  If
the reworded commit is follow by further picks, those later commits
will have an earlier committer date than the reworded one. This is
caused by git caching the default date used when GIT_COMMITTER_DATE is
not set. Reset the cached date before a commit is generated
in-process.

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

diff --git a/sequencer.c b/sequencer.c
index f9d1001dee..f0bac903a0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -1148,6 +1148,8 @@ static int try_to_commit(struct strbuf *msg, const char 
*author,
goto out;
}
 
+   reset_ident_date();
+
if (commit_tree_extended(msg->buf, msg->len, , parents,
 oid, author, opts->gpg_sign, extra)) {
res = error(_("failed to write commit object"));
-- 
2.17.0.69.g0c1d01d9b6

Re: Bug: rebase -i creates committer time inversions on 'reword'

[Johannes Sixt]

Am 15.04.2018 um 23:35 schrieb Junio C Hamano:

Ah, do you mean we have an internal sequence like this, when "rebase
--continue" wants to conclude an edit/reword?


Yes, it's only 'reword' that is affected, because then subsequent picks 
are processed by the original process.



  - we figure out the committer ident, which grabs a timestamp and
cache it;

  - we spawn "commit" to conclude the stopped step, letting it record
its beginning time (which is a bit older than the above) or its
ending time (which is much older due to human typing speed);


Younger in both cases, of course. According to my tests, we seem to pick 
the beginning time, because the first 'reword'ed commit typically has 
the same timestamp as the preceding picks. Later 'reword'ed commits have 
noticably younger timestamps.



  - subsequent "picks" are made in the same process, and share the
timestamp we grabbed in the first step, which is older than the
second one.

I guess we'd want a mechanism to tell ident.c layer "discard the
cached one, as we are no longer in the same automated sequence", and
use that whenever we spawn an editor (or otherwise go interactive).


Frankly, I think that this caching is overengineered (or prematurly 
optimized). If the design requires that different callers of datestamp() 
must see the same time, then the design is broken. In a fixed design, 
there would be a single call of datestamp() in advance, and then the 
timestamp, which then obviously is a very important piece of data, would 
be passed along as required.


-- Hannes

Bug: rebase -i creates committer time inversions on 'reword'

[Johannes Sixt]

I just noticed that all commits in a 70-commit branch have the same
committer timestamp. This is very unusual on Windows, where rebase -i of
such a long branch takes more than one second (but not more than 3 or
so thanks to the builtin nature of the command!).

And, in fact, if you mark some commits with 'reword' to delay the quick
processing of the patches, then the reworded commits have later time
stamps, but subsequent not reworded commits receive the earlier time
stamp. This is clearly not intended.

Perhaps something like this below is needed.

diff --git a/ident.c b/ident.c
index 327abe557f..2c6bff7b9d 100644
--- a/ident.c
+++ b/ident.c
@@ -178,8 +178,8 @@ const char *ident_default_email(void)
 
 static const char *ident_default_date(void)
 {
-   if (!git_default_date.len)
-   datestamp(_default_date);
+   strbuf_reset(_default_date);
+   datestamp(_default_date);
return git_default_date.buf;
 }
 

Re: Windows > git.exe > the result of "git branch" does not always highlight the current branch

[Johannes Sixt]

Am 09.04.2018 um 21:26 schrieb Hari Lubovac:

It appears to be just a reporting issue. Probably not a big deal, but
I thought I should report this, if it hasn't been noticed: when a
branch is switched to by being named with non-original
character-casing, then it's not clear which branch is current.

Example:

C:\repo>git branch
* bar
   foo

C:\repo>git checkout Bar
Switched to branch 'Bar'

C:\repo>git branch
   bar
   foo


The bug is not that the branch is not marked, but that you are permitted 
to check out a branch that does not exist. This is a side-effect of the 
fact that branch names are sometimes stored using file names, and, as we 
know, file names are case-insensitive on Windows. I don't know of any 
efforts to fix that (I assume that it is not just a simple fix). In the 
meantime, I can only recommend: if it hurts, don't do it.


If you call `git gc` before the checkout command, I would expect that 
you would not be able to check out branch 'Bar', because branches are 
represented unambiguously after 'gc' (not as file names).


-- Hannes

Re: js/runtime-prefix-windows, was Re: What's cooking in git.git (Mar 2018, #06; Fri, 30)

[Johannes Sixt]

Am 03.04.2018 um 15:12 schrieb Johannes Schindelin:

On Fri, 30 Mar 2018, Junio C Hamano wrote:


* js/runtime-prefix-windows (2018-03-27) 2 commits
  - mingw/msvc: use the new-style RUNTIME_PREFIX helper
  - exec_cmd: provide a new-style RUNTIME_PREFIX helper for Windows
  (this branch uses dj/runtime-prefix.)

  The Windows port was the first that allowed Git to be installed
  anywhere by having its components refer to each other with relative
  pathnames.  The recent dj/runtime-prefix topic extends the idea to
  other platforms, and its approach has been adopted back in the
  Windows port.

  Is this, together with the dj/runtime-prefix topic, ready for
  'next'?


As far as I am concerned: yes!


Works in my environment, too. No objections.

-- Hannes

Re: Need help debugging issue in git

[Johannes Sixt]

Am 02.04.2018 um 02:36 schrieb Robert Dailey:

I'm struggling with a bug that I found introduced in git v2.13.2. The
bug was not reproducible in v2.13.1. The issue is that using arguments
like "@{-1}" to aliases causes those curly braces to be removed, so
once the command is executed after alias processing the argument looks
like "@-1". This breaks any aliases you have that wrap `git log` and
such. I originally opened the bug on the Git for Windows project
(since I use Git mostly on Windows):

https://github.com/git-for-windows/git/issues/1220

...

Here is the alias being used for a test:

[alias]
 lgtest = !git log --oneline \"$@\"

And here is the command I invoke for the test:

$ git lgtest @{-1}

I should get logs for the previously-checked-out branch.

When `prepare_shell_cmd()` is called in run-command.c, it gets expanded like so:

+ [0] "sh" const char *
+ [1] "-c" const char *
+ [2] "git log --oneline \"$@\" \"$@\"" const char *
+ [3] "git log --oneline \"$@\"" const char *
+ [4] "@{-1}" const char *

With my modifications (again, patch inline below) I get this:

+ [0] "sh" const char *
+ [1] "-c" const char *
+ [2] "git log --oneline \"$@\"" const char *
+ [3] "@{-1}" const char *

The second version looks much better.


But this is wrong. Try this on the command line:

  sh -c 'echo "$@"' a b c

Notice how this prints only 'b c', not 'a b c'. The reason is that the 
argument 'a' is treated like a "script" name, i.e. what you get for 
"$0", and 'b' and 'c' as the actual arguments to the "script".


That is, you must fill in some dummy "script" name at slot [3], and 
run_command chooses to put the alias text there.



I think the constant nesting of
commands inside each other that the first version does is somehow
causing curly braces to be removed. I don't understand enough about
shell processing to know why it would do this.


Some shells expand the curly braces. They must get lost somewhere by one 
of the two shell invocations that happen on the way.


BTW, you don't happen to have a file named '@-1' in your directory, most 
likely by accident?


-- Hannes

Re: [PATCH 3/3] Makefile: optionally symlink libexec/git-core binaries to bin/git

[Johannes Sixt]

Am 13.03.2018 um 21:39 schrieb Ævar Arnfjörð Bjarmason:

Add a INSTALL_SYMLINKS option which if enabled, changes the default
hardlink installation method to one where the relevant binaries in
libexec/git-core are symlinked back to ../../bin, instead of being
hardlinked.

This new option also overrides the behavior of the existing
NO_*_HARDLINKS variables which in some cases would produce symlinks
within to libexec/, e.g. "git-add" symlinked to "git" which would be
copy of the "git" found in bin/, now "git-add" in libexec/ is always
going to be symlinked to the "git" found in the bin/ directory.


It is important to leave the default at hard-linking the binaries, 
because on Windows symbolic links are second class citizens (they 
require special privileges and there is a distinction between link 
targets being files or directories). Hard links work well.




This option is being added because:

  1) I think it makes what we're doing a lot more obvious. E.g. I'd
 never noticed that the libexec binaries were really just hardlinks
 since e.g. ls(1) won't show that in any obvious way. You need to
 start stat(1)-ing things and look at the inodes to see what's
 going on.

  2) Some tools have very crappy support for hardlinks, e.g. the Git
 shipped with GitLab is much bigger than it should be because
 they're using a chef module that doesn't know about hardlinks, see
 https://github.com/chef/omnibus/issues/827

 I've also ran into other related issues that I think are explained


s/ran/run/


 by this, e.g. compiling git with debugging and rpm refusing to
 install a ~200MB git package with 2GB left on the FS, I think that
 was because it doesn't consider hardlinks, just the sum of the
 byte size of everything in the package.

As for the implementation, the "../../bin" noted above will vary given
some given some values of "../.." and "bin" depending on the depth of


s/given some//


the gitexecdir relative to the destdir, and the "bindir" target,
e.g. setting "bindir=/tmp/git/binaries gitexecdir=foo/bar/baz" will do
the right thing and produce this result:

 $ file /tmp/git/foo/bar/baz/git-add
 /tmp/git/foo/bar/baz/git-add: symbolic link to ../../../binaries/git

Signed-off-by: Ævar Arnfjörð Bjarmason 
---
  Makefile | 46 +++---
  1 file changed, 31 insertions(+), 15 deletions(-)

diff --git a/Makefile b/Makefile
index ee0b6c8940..ac7616422d 100644
--- a/Makefile
+++ b/Makefile
@@ -329,6 +329,13 @@ all::
  # when hardlinking a file to another name and unlinking the original file 
right
  # away (some NTFS drivers seem to zero the contents in that scenario).
  #
+# Define INSTALL_SYMLINKS if you prefer to have everything that can be
+# symlinked between bin/ and libexec/ to use relative symlinks between
+# the two. This option overrides NO_CROSS_DIRECTORY_HARDLINKS and


s/ between the two//


+# NO_INSTALL_HARDLINKS which will also use symlinking by indirection
+# within the same directory in some cases, INSTALL_SYMLINKS will
+# always symlink to the final target directly.


"the final target"? Do you mean "the git executable installed in 
$bindir" or something like this?



+#
  # Define NO_CROSS_DIRECTORY_HARDLINKS if you plan to distribute the installed
  # programs as a tar, where bin/ and libexec/ might be on different file 
systems.
  #
@@ -2594,35 +2601,44 @@ endif
  
  	bindir=$$(cd '$(DESTDIR_SQ)$(bindir_SQ)' && pwd) && \

execdir=$$(cd '$(DESTDIR_SQ)$(gitexec_instdir_SQ)' && pwd) && \
+   destdir_from_execdir_SQ=$$(echo '$(gitexecdir_relative_SQ)' | sed -e 
's|[^/][^/]*|..|g') && \
{ test "$$bindir/" = "$$execdir/" || \
  for p in git$X $(filter $(install_bindir_programs),$(ALL_PROGRAMS)); 
do \
$(RM) "$$execdir/$$p" && \
-   test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" 
&& \
-   ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
-   cp "$$bindir/$$p" "$$execdir/$$p" || exit; \
+   test -n "$(INSTALL_SYMLINKS)" && \
+   ln -s "$$destdir_from_execdir_SQ/$(bindir_relative_SQ)/$$p" 
"$$execdir/$$p" || \
+   { test -z "$(NO_INSTALL_HARDLINKS)$(NO_CROSS_DIRECTORY_HARDLINKS)" 
&& \
+ ln "$$bindir/$$p" "$$execdir/$$p" 2>/dev/null || \
+ cp "$$bindir/$$p" "$$execdir/$$p" || exit; } \


I think that it is unnecessary to place the later options in {} brackets 
because && and || have equal precedence in shell scripts. That is:


want symlinks? &&
make symlinks ||
want hard links? &&
make hard links ||
make copies ||
exit

Of course, it means that when symlinking fails, it falls back to hard 
links (if permitted) or copies, whichever works. But that also happens 
with your version.


(Ditto in the rest of the hunk, which I don't repeat here.)

-- Hannes

Re: [PATCH 2/2] worktree: add: change to new worktree directory before running hook

[Johannes Sixt]

Am 12.02.2018 um 04:15 schrieb Eric Sunshine:

--- a/t/t2025-worktree-add.sh
+++ b/t/t2025-worktree-add.sh
@@ -454,20 +454,29 @@ post_checkout_hook () {
test_when_finished "rm -f .git/hooks/post-checkout" &&
mkdir -p .git/hooks &&
write_script .git/hooks/post-checkout <<-\EOF
-   echo $* >hook.actual
+   {
+   echo $*
+   git rev-parse --show-toplevel
+   } >../hook.actual
EOF
  }
  
  test_expect_success '"add" invokes post-checkout hook (branch)' '

post_checkout_hook &&
-   printf "%s %s 1\n" $_z40 $(git rev-parse HEAD) >hook.expect &&
+   {
+   echo $_z40 $(git rev-parse HEAD) 1 &&
+   echo $(pwd)/gumby


$(pwd) is here and in the other tests correct. $PWD would be wrong on 
Windows. Thanks for being considerate.



+   } >hook.expect &&
git worktree add gumby &&
test_cmp hook.expect hook.actual


-- Hannes

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Sixt]

Am 09.02.2018 um 07:11 schrieb Sergey Organov:

Johannes Schindelin  writes:

Let me explain the scenario which comes up plenty of times in my work with
Git for Windows. We have a thicket of some 70 branches on top of git.git's
latest release. These branches often include fixup! and squash! commits
and even more complicated constructs that rebase cannot handle at all at
the moment, such as reorder-before! and reorder-after! (for commits that
really need to go into a different branch).


I sympathize, but a solution that breaks even in simple cases can't be
used reliably to solve more complex problems, sorry. Being so deep
into your problems, I think you maybe just aren't seeing forest for the
trees [1].


Hold your horses! Dscho has a point here. --preserve-merges 
--first-parent works only as long as you don't tamper with the side 
branches. If you make changes in the side branches during the same 
rebase operation, this --first-parent mode would undo that change. (And, 
yes, its result would be called an "evil merge", and that scary name 
_should_ frighten you!)


-- Hannes

Re: [PATCH] t0050: remove the unused $test_case variable

[Johannes Sixt]

Am 07.02.2018 um 09:07 schrieb Ævar Arnfjörð Bjarmason:


On Wed, Feb 07 2018, Johannes Sixt jotted:


Am 07.02.2018 um 00:13 schrieb Ævar Arnfjörð Bjarmason:

The $test_case variable hasn't been used since
decd3c0c28 ("t0050-*.sh: mark the rename (case change) test as
passing", 2014-11-28) when its last user went away.

Let's remove the "say" as well, since it's obvious from subsequent
output that we're testing on a case sensitive filesystem.


Am I misunderstanding the message? I think it reports properties of
the test environment. And the tests do run on case-insensitive
filesystems. IMO, the message should be kept.


It's obvious from subsequent output whether the FS is case sensitive or
not, so I thought it was redundant to keep this report at the top since
we didn't have the variable setting anymore.


There are test cases that do different things depending on whether the 
CASE_INSENSITIVE_FS prerequisite is set. I think it was the intent to 
report whether it is set and not whether one or the other value of the 
(now unused) variable is used somewhere.


BTW, the message texts do not show which variant is taken (these are 
without your patch):


On Windows:

t>sh t0050-filesystem.sh
will test on a case insensitive filesystem
will test on a filesystem lacking symbolic links
ok 1 - detection of case insensitive filesystem during repo init
ok 2 - detection of filesystem w/o symlink support during repo init
ok 3 - setup case tests
ok 4 - rename (case change)
ok 5 - merge (case change)
not ok 6 - add (with different case) # TODO known breakage
ok 7 - setup unicode normalization tests
ok 8 - rename (silent unicode normalization)
ok 9 - merge (silent unicode normalization)
# still have 1 known breakage(s)
# passed all remaining 8 test(s)
1..9

On Linux:

t@master:1002> ./t0050-filesystem.sh
ok 1 - detection of case insensitive filesystem during repo init
ok 2 - detection of filesystem w/o symlink support during repo init
ok 3 - setup case tests
ok 4 - rename (case change)
ok 5 - merge (case change)
ok 6 # skip add (with different case) (missing CASE_INSENSITIVE_FS)
ok 7 - setup unicode normalization tests
ok 8 - rename (silent unicode normalization)
ok 9 - merge (silent unicode normalization)
# passed all 9 test(s)
1..9

I'd even argue that there should be messages on Linux, too.

-- Hannes

Re: [PATCH 5/8] rebase: introduce the --recreate-merges option

[Johannes Sixt]

Am 07.02.2018 um 07:16 schrieb Sergey Organov:

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

[...]


+--recreate-merges::
+   Recreate merge commits instead of flattening the history by replaying
+   merges. Merge conflict resolutions or manual amendments to merge
+   commits are not preserved.


I wonder why you guys still hold on replaying "merge-the-operation"
instead of replaying "merge-the-result"? The latter, the merge commit
itself, no matter how exactly it was created in the first place, is the
most valuable thing git keeps about the merge, and you silently drop it
entirely! OTOH, git keeps almost no information about
"merge-the-operation", so it's virtually impossible to reliably replay
the operation automatically, and yet you try to.


Very well put. I share your concerns.

-- Hannes


IMHO that was severe mistake in the original --preserve-merges, and you
bring with you to this new --recreate-merges... It's sad. Even more sad
as solution is already known for years:

 bc00341838a8faddcd101da9e746902994eef38a
 Author: Johannes Sixt <j...@kdbg.org>
 Date:   Sun Jun 16 15:50:42 2013 +0200
 
 rebase -p --first-parent: redo merge by cherry-picking first-parent change


and it works like a charm.

-- Sergey

Re: [PATCH] t0050: remove the unused $test_case variable

[Johannes Sixt]

Am 07.02.2018 um 00:13 schrieb Ævar Arnfjörð Bjarmason:

The $test_case variable hasn't been used since
decd3c0c28 ("t0050-*.sh: mark the rename (case change) test as
passing", 2014-11-28) when its last user went away.

Let's remove the "say" as well, since it's obvious from subsequent
output that we're testing on a case sensitive filesystem.


Am I misunderstanding the message? I think it reports properties of the 
test environment. And the tests do run on case-insensitive filesystems. 
IMO, the message should be kept.




Signed-off-by: Ævar Arnfjörð Bjarmason 
---
  t/t0050-filesystem.sh | 8 
  1 file changed, 8 deletions(-)

diff --git a/t/t0050-filesystem.sh b/t/t0050-filesystem.sh
index b29d749bb7..606ffddd92 100755
--- a/t/t0050-filesystem.sh
+++ b/t/t0050-filesystem.sh
@@ -7,14 +7,6 @@ test_description='Various filesystem issues'
  auml=$(printf '\303\244')
  aumlcdiar=$(printf '\141\314\210')
  
-if test_have_prereq CASE_INSENSITIVE_FS

-then
-   say "will test on a case insensitive filesystem"
-   test_case=test_expect_failure
-else
-   test_case=test_expect_success
-fi
-
  if test_have_prereq UTF8_NFD_TO_NFC
  then
say "will test on a unicode corrupting filesystem"

Re: [PATCH v7 17/31] merge-recursive: add a new hashmap for storing directory renames

[Johannes Sixt]

Am 03.02.2018 um 22:34 schrieb Elijah Newren:
>  If anyone can find an
> example of a real world open source repository (linux, webkit, git,
> etc.) with a merge where n is greater than about 10, I'll be
> surprised.

git rev-list --parents --merges master |
 grep " .* .* .* .* .* .* .* .* .* .* "

returns quite a few hits in the Linux repository. Most notable is
fa623d1b0222adbe8f822e53c08003b9679a410c; spoiler: it has 30 parents.

-- Hannes

Re: Git on Windows maps creation time onto changed time

[Johannes Sixt]

Am 02.02.2018 um 00:10 schrieb Keith Goldfarb:

Dear git,

While tracking down a problem with a filesystem shared by Windows and Ubuntu, I 
came across the following code in compat/mingw.c (ming_fstat(), also in 
do_lstat()):

if (GetFileInformationByHandle(fh, )) {
buf->st_ino = 0;
buf->st_gid = 0;
buf->st_uid = 0;
buf->st_nlink = 1;
buf->st_mode = file_attr_to_st_mode(fdata.dwFileAttributes);
buf->st_size = fdata.nFileSizeLow |
(((off_t)fdata.nFileSizeHigh)<<32);
buf->st_dev = buf->st_rdev = 0; /* not used by Git */
buf->st_atime = filetime_to_time_t(&(fdata.ftLastAccessTime));
buf->st_mtime = filetime_to_time_t(&(fdata.ftLastWriteTime));
buf->st_ctime = filetime_to_time_t(&(fdata.ftCreationTime));
return 0;
}

The assignment of buf->st_ctime doesn’t seem right to me. I
understand there’s no good choice here, but I think a better choice
would be to  duplicate the definition used for st_mtime.


The purpose of these values is to allow to notice a change on the file 
system without going through the actual file data. Duplicating st_mtime 
would be pointless.



Background: When I do a git status on Windows and then later on
Ubuntu (or the other order), it is extremely slow, as the entire tree
is being traversed. I tracked it down to this difference in
definition of c_time. Yes, I know about the core.trustctime variable,
but my problem aside  this seems like an unwise choice.


Don't do that then. Use core.trustctime.

-- Hannes

Re: [PATCH 00/37] removal of some c++ keywords

[Johannes Sixt]

Am 29.01.2018 um 23:36 schrieb Brandon Williams:

A while back there was some discussion of getting our codebase into a state
where we could use a c++ compiler if we wanted to (for various reason like
leveraging c++ only analysis tools, etc.).  Johannes Sixt had a very large
patch that achieved this but it wasn't in a state where it could be upstreamed.
I took that idea and did some removals of c++ keywords (new, template, try,
this, etc) but broke it up into several (well maybe more than several) patches.
I don't believe I've captured all of them in this series but this is at least
moving a step closer to being able to compile using a c++ compiler.


Cool! The patches all look reasonable. Some keywords remain: 'delete', 
'private', 'thread_local', 'not', 'xor', 'explicit', 'export'.



I don't know if this is something the community still wants to move towards,
but if this is something people are still interested in, and this series is
wanted, then we can keep doing these sort of conversions in chunks slowly.


I've rebased my patches on top of this series. They are available from

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

-- Hannes

Re: [PATCH 1/1] Mark messages for translations

[Johannes Sixt]

Am 15.01.2018 um 06:44 schrieb Alexander Shopov:

@@ -5,11 +5,11 @@
  #include "run-command.h"
  
  const char git_usage_string[] =

-   "git [--version] [--help] [-C ] [-c name=value]\n"
-   "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
-   "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
-   "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
-   "[]";
+   N_("git [--version] [--help] [-C ] [-c name=value]\n"
+  "   [--exec-path[=]] [--html-path] [--man-path] 
[--info-path]\n"
+  "   [-p | --paginate | --no-pager] [--no-replace-objects] 
[--bare]\n"
+  "   [--git-dir=] [--work-tree=] 
[--namespace=]\n"
+  "[]");
  
  const char git_more_info_string[] =

N_("'git help -a' and 'git help -g' list available subcommands and 
some\n"
@@ -92,7 +92,7 @@ static int handle_options(const char ***argv, int *argc, int 
*envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "--git-dir")) {
if (*argc < 2) {
-   fprintf(stderr, "No directory given for 
--git-dir.\n" );
+   fprintf(stderr, _("No directory given for 
--git-dir.\n" ));
usage(git_usage_string);


It is not obvious to me where git_usage_string is looked up in the 
message catalog. Should this not be


usage(_(git_usage_string));

(here and in later instances)? It is used that way in builtin/help.c, 
for example.



@@ -385,14 +385,14 @@ void setup_work_tree(void)
return;
  
  	if (work_tree_config_is_bogus)

-   die("unable to set up work tree using invalid config");
+   die(_("unable to set up work tree using invalid config"));
  
  	work_tree = get_git_work_tree();

git_dir = get_git_dir();
if (!is_absolute_path(git_dir))
git_dir = real_path(get_git_dir());
if (!work_tree || chdir(work_tree))
-   die("This operation must be run in a work tree");
+   die(_("This operation must be run in a work tree"));


We have settled with lower-case letters at the beginning of error 
messages. (See Documentation/CodingGuidelines, "Error Messages".) You 
could fix that while you are touching die, die_errno, etc, messages.



@@ -677,12 +677,12 @@ static const char *setup_explicit_git_dir(const char 
*gitdirenv,
else {
char *core_worktree;
if (chdir(gitdirenv))
-   die_errno("Could not chdir to '%s'", gitdirenv);
+   die_errno(_("Cannot chdir to '%s'"), gitdirenv);


I notice you change past tense to present tense in some cases. IMO, this 
makes the messages more consistent. Good.


I'm not a friend of geeky abbreviations like "chdir" or "cwd" in 
user-visible messages, and I would have taken the opportunity to change 
the messages accordingly. This is really only my personal taste, though, 
and it's possible that I'm alone in this camp.



if (chdir(git_work_tree_cfg))
-   die_errno("Could not chdir to '%s'", 
git_work_tree_cfg);
+   die_errno(_("Cannot chdir to '%s'"), 
git_work_tree_cfg);
core_worktree = xgetcwd();
if (chdir(cwd->buf))
-   die_errno("Could not come back to cwd");
+   die_errno(_("Cannot come back to cwd");

...

@@ -1207,7 +1207,7 @@ void sanitize_stdfds(void)
while (fd != -1 && fd < 2)
fd = dup(fd);
if (fd == -1)
-   die_errno("open /dev/null or dup failed");
+   die_errno(_("open /dev/null or dup failed"));
if (fd > 2)
close(fd);
  }
@@ -1222,12 +1222,12 @@ int daemonize(void)
case 0:
break;
case -1:
-   die_errno("fork failed");
+   die_errno(_("fork failed"));
default:
exit(0);
}
if (setsid() == -1)
-   die_errno("setsid failed");
+   die_errno(_("setsid failed"));


Here is a certain class of errors: They should occur only rarely 
(actually, is that true?) Then it is useful to have the function name in 
the message. Which rises the question: why translate them at all? It's 
possible that translators turn the message into unusable gibberish just 
to please their language. All of this is only IMHO; I don't use 
translated Git.


-- Hannes