Re: [PATCH] checkout files in-place

[Clemens Buchacher]

+Cc: Orgad Shaneh

On Mon, Jun 11, 2018 at 08:35:41PM +, Edward Thomson wrote:
> On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
> > 
> > It is safe to do this on Linux file systems, even if open file handles
> > still exist, because unlink only removes the directory reference to the
> > file. On Windows, however, a file cannot be deleted until all handles to
> > it are closed. If a file cannot be deleted, its name cannot be reused.
> 
> I'm nervous about this proposed change, since it feels like it's
> addressing an issue that only exists in QT Creator.
> 
> You've accurately described the default semantics in Win32.  A file
> cannot be deleted until all handles to it are closed, unless it was
> opened with `FILE_SHARE_DELETE` as their sharing mode.  This is not the
> default sharing mode in either Win32 or .NET.
> 
> However, for your patch to have an effect, all processes with a handle
> open must have specified `FILE_SHARE_WRITE`.  This is rather uncommon,
> since it's also not included in the default Win32 or .NET sharing mode.
> This is because it's uncommon that you would want other processes to
> change the data underneath you in between ReadFile() calls.
> 
> So your patch will benefit people who have processes that have
> `FILE_SHARE_WRITE` set but not `FILE_SHARE_DELETE` set, which I think is
> generally an uncommon scenario to want to support.
> 
> Generally if you're willing to accept files changing underneath you,
> then you probably want to allow them to be deleted, too.  So this feels
> like something that's very specific to QT Creator.  Or are there other
> IDEs or development tools that use these open semantics that I'm not
> aware of?

I am also not aware of other IDEs which have this issue.

Orgad, you also mentioned FILE_SHARE_DELETE here [*1*]. Does the Qt
Creator issue persist despite this flag? You also just commented on
Github that "Regarding Qt Creator, the issue should be mostly solved by
now in 4.7". So a fix in Git is no longer needed?

[*1*] https://github.com/git-for-windows/git/pull/1666

[PATCH v2] checkout files in-place

[Clemens Buchacher]

When replacing files with new content during checkout, we do not write
to them in place. Instead we unlink and recreate the files in order to
let the system figure out ownership and permissions for the new file,
taking umask into account.

It is safe to do this on Linux file systems, even if open file handles
still exist, because unlink only removes the directory reference to the
file. On Windows, however, a file cannot be deleted until all handles to
it are closed. If a file cannot be deleted, its name cannot be reused.

This causes files to be deleted, but not checked out when switching
branches. This is frequently an issue with Qt Creator, which
continuously opens files in the work tree, as reported here:
https://github.com/git-for-windows/git/issues/1653

This change adds the core.checkoutInPlace option. If enabled, checkout
will open files for writing the new content in place. This fixes the
issue, but with this approach the system will not update file
permissions according to umask. Only essential updates of write and
executable permissions are performed.

The in-place checkout is therefore optional. It could be enabled by Git
installers on Windows, where umask is irrelevant.

Signed-off-by: Clemens Buchacher 
Reviewed-by: Orgad Shaneh 
Reviewed-by: "brian m. carlson" 
Reviewed-by: Duy Nguyen 
Reviewed-by: Ævar Arnfjörð Bjarmason 
---

Tested on Windows with Git-for-Windows and with Windows Subsystem for
Linux.

 Documentation/config.txt| 11 ++
 cache.h |  2 ++
 config.c|  5 +++
 entry.c | 18 --
 environment.c   |  1 +
 t/t2031-checkout-inplace.sh | 82 +
 6 files changed, 116 insertions(+), 3 deletions(-)
 create mode 100755 t/t2031-checkout-inplace.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index ab641bf..0860a81 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -912,6 +912,17 @@ core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
linkgit:git-read-tree[1] for more information.
 
+core.checkoutInPlace::
+   Check out file contents in place. By default Git checkout removes 
existing
+   work tree files before it replaces them with different content. If this
+   option is enabled, Git will overwrite the contents of existing files in
+   place. This is useful on Windows, where open file handles to a removed 
file
+   prevent creating new files at the same path.
+   Note that the current implementation of in-place checkout makes no 
effort
+   to update read/write permissions according to umask. Permissions are
+   however modified to enable write access and to update executable
+   permissions.
+
 core.abbrev::
Set the length object names are abbreviated to.  If
unspecified or set to "auto", an appropriate value is
diff --git a/cache.h b/cache.h
index 89a107a..5b8c4d6 100644
--- a/cache.h
+++ b/cache.h
@@ -815,6 +815,7 @@ extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_commit_graph;
 extern int core_apply_sparse_checkout;
+extern int checkout_inplace;
 extern int precomposed_unicode;
 extern int protect_hfs;
 extern int protect_ntfs;
@@ -1518,6 +1519,7 @@ struct checkout {
unsigned force:1,
 quiet:1,
 not_new:1,
+inplace:1,
 refresh_cache:1;
 };
 #define CHECKOUT_INIT { NULL, "" }
diff --git a/config.c b/config.c
index fbbf0f8..8b35ecd 100644
--- a/config.c
+++ b/config.c
@@ -1318,6 +1318,11 @@ static int git_default_core_config(const char *var, 
const char *value)
return 0;
}
 
+   if (!strcmp(var, "core.checkoutinplace")) {
+   checkout_inplace = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "core.precomposeunicode")) {
precomposed_unicode = git_config_bool(var, value);
return 0;
diff --git a/entry.c b/entry.c
index 2101201..a599fc1 100644
--- a/entry.c
+++ b/entry.c
@@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)
 
 static int create_file(const char *path, unsigned int mode)
 {
+   int flags;
+   if (checkout_inplace)
+   flags = O_WRONLY | O_CREAT | O_TRUNC;
+   else
+   flags = O_WRONLY | O_CREAT | O_EXCL;
mode = (mode & 0100) ? 0777 : 0666;
-   return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
+   return open(path, flags, mode);
 }
 
 static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
@@ -467,8 +472,15 @@ int checkout_entry(struct cache_entry *ce,
if (!state->force)
return error("%s is a directory", path.buf);
remove_subtree();

Re: [PATCH] checkout files in-place

[Clemens Buchacher]

On Mon, Jun 11, 2018 at 11:02:42AM -0700, Junio C Hamano wrote:
> 
> Aside from us not having to worry about emulating the umask, another
> reason why we prefer "create, complete the write, and then finally
> rename" over "overwrite and let it fail in the middle" is that the
> former makes sure we never leave the path in a bad state.

But the current checkout implementation does not do this. It writes
directly to the target location. The only difference to in-place
checkout is that files are unlinked before they are opened for writing.

Re: [PATCH] checkout files in-place

[Clemens Buchacher]

On Mon, Jun 11, 2018 at 02:04:11AM +, brian m. carlson wrote:
> On Sun, Jun 10, 2018 at 09:44:45PM +0200, Clemens Buchacher wrote:
> > +   file prevent creating new files at the same path. Note that Git will not
> > +   update read/write permissions according to umask.
> 
> I'm wondering if it's worth a mention that running out of disk space (or
> quota) will cause data to be truncated.

As far as I know we make no guarantees about the behavior when running
out of disk space. There could be other side effects, so I cannot safely
state anything here.

[PATCH] checkout files in-place

[Clemens Buchacher]

When replacing files with new content during checkout, we do not write
to them in-place. Instead we unlink and re-create the files in order to
let the system figure out ownership and permissions for the new file,
taking umask into account.

It is safe to do this on Linux file systems, even if open file handles
still exist, because unlink only removes the directory reference to the
file. On Windows, however, a file cannot be deleted until all handles to
it are closed. If a file cannot be deleted, its name cannot be reused.

This causes files to be deleted, but not checked out when switching
branches. This is frequently an issue with Qt Creator, which
continuously opens files in the work tree, as reported here:
https://github.com/git-for-windows/git/issues/1653

This change adds the core.checkout_inplace option. If enabled, checkout
will open files for writing the new content in-place. This fixes the
issue, but with this approach the system will not update file
permissions according to umask. Only essential updates of write and
executable permissions are performed.

The in-place checkout is therefore optional. It could be enabled by Git
installers on Windows, where umask is irrelevant.

Signed-off-by: Clemens Buchacher 
---

I wonder if Git should be responsible for updating ownership and file
permissions when modifying existing files during checkout. We could
otherwise remove the unlink completely. Maybe this could even improve
performance in some cases. It made no difference in a short test on
Windows.

Regression tests are running. This will take a while.

 Documentation/config.txt|  8 
 cache.h |  2 ++
 config.c|  5 +
 entry.c | 18 +++---
 environment.c   |  1 +
 t/t2031-checkout-inplace.sh | 41 +
 6 files changed, 72 insertions(+), 3 deletions(-)
 create mode 100755 t/t2031-checkout-inplace.sh

diff --git a/Documentation/config.txt b/Documentation/config.txt
index b6cb997164..17af0fe163 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -923,6 +923,14 @@ core.sparseCheckout::
Enable "sparse checkout" feature. See section "Sparse checkout" in
linkgit:git-read-tree[1] for more information.
 
+core.checkoutInplace::
+   Checkout file contents in-place. By default Git checkout removes 
existing
+   work tree files before it replaces them with different contents. If this
+   option is enabled Git will overwrite the contents of existing files
+   in-place. This is useful on systems where open file handles to a removed
+   file prevent creating new files at the same path. Note that Git will not
+   update read/write permissions according to umask.
+
 core.abbrev::
Set the length object names are abbreviated to.  If
unspecified or set to "auto", an appropriate value is
diff --git a/cache.h b/cache.h
index 2c640d4c31..c8fccd2a80 100644
--- a/cache.h
+++ b/cache.h
@@ -808,6 +808,7 @@ extern char *git_replace_ref_base;
 extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
+extern int checkout_inplace;
 extern int precomposed_unicode;
 extern int protect_hfs;
 extern int protect_ntfs;
@@ -1530,6 +1531,7 @@ struct checkout {
unsigned force:1,
 quiet:1,
 not_new:1,
+inplace:1,
 refresh_cache:1;
 };
 #define CHECKOUT_INIT { NULL, "" }
diff --git a/config.c b/config.c
index cd2b404b14..4ac2407057 100644
--- a/config.c
+++ b/config.c
@@ -1231,6 +1231,11 @@ static int git_default_core_config(const char *var, 
const char *value, void *cb)
return 0;
}
 
+   if (!strcmp(var, "core.checkoutinplace")) {
+   checkout_inplace = git_config_bool(var, value);
+   return 0;
+   }
+
if (!strcmp(var, "core.precomposeunicode")) {
precomposed_unicode = git_config_bool(var, value);
return 0;
diff --git a/entry.c b/entry.c
index 31c00816dc..54c98870b9 100644
--- a/entry.c
+++ b/entry.c
@@ -78,8 +78,13 @@ static void remove_subtree(struct strbuf *path)
 
 static int create_file(const char *path, unsigned int mode)
 {
+   int flags;
+   if (checkout_inplace)
+   flags = O_WRONLY | O_CREAT | O_TRUNC;
+   else
+   flags = O_WRONLY | O_CREAT | O_EXCL;
mode = (mode & 0100) ? 0777 : 0666;
-   return open(path, O_WRONLY | O_CREAT | O_EXCL, mode);
+   return open(path, flags, mode);
 }
 
 static void *read_blob_entry(const struct cache_entry *ce, unsigned long *size)
@@ -470,8 +475,15 @@ int checkout_entry(struct cache_entry *ce,
if (!state->force)
return error("%s is a directory", path.buf);
remove_subtree()

[PATCH] completion: improve ls-files filter performance

[Clemens Buchacher]

>From the output of ls-files, we remove all but the leftmost path
component and then we eliminate duplicates. We do this in a while loop,
which is a performance bottleneck when the number of iterations is large
(e.g. for 6 files in linux.git).

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real0m11.876s
user0m4.685s
sys 0m6.808s

Replacing the loop with the cut command improves performance
significantly:

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real0m1.372s
user0m0.263s
sys 0m0.167s

The measurements were done with Msys2 bash, which is used by Git for
Windows.

When filtering the ls-files output we take care not to touch absolute
paths. This is redundant, because ls-files will never output absolute
paths. Remove the unnecessary operations.

The issue was reported here:
https://github.com/git-for-windows/git/issues/1533

Signed-off-by: Clemens Buchacher <dri...@gmx.net>
---

On Sun, Mar 18, 2018 at 02:26:18AM +0100, SZEDER Gábor wrote:
> 
> You didn't run the test suite, did you? ;)

My bad. I put the sort back in. Test t9902 is now pass. I did not run
the other tests. I think the completion script is not used there.

I also considered Junio's and Johannes' comments.

> I have a short patch series collecting dust somewhere for a long
> while, [...]
> Will try to dig up those patches.

Cool. Bash completion can certainly use more performance improvements.

 contrib/completion/git-completion.bash | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6da95b8..69a2d41 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -384,12 +384,7 @@ __git_index_files ()
local root="${2-.}" file
 
__git_ls_files_helper "$root" "$1" |
-   while read -r file; do
-   case "$file" in
-   ?*/*) echo "${file%%/*}" ;;
-   *) echo "$file" ;;
-   esac
-   done | sort | uniq
+   cut -f1 -d/ | sort | uniq
 }
 
 # Lists branches from the local repository.
-- 
2.7.4

[PATCH 1/2] completion: improve ls-files filter performance

[Clemens Buchacher]

>From the output of ls-files, we remove all but the leftmost path
component and then we eliminate duplicates. We do this in a while loop,
which is a performance bottleneck when the number of iterations is large
(e.g. for 6 files in linux.git).

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real0m11.876s
user0m4.685s
sys 0m6.808s

Using an equivalent sed script improves performance significantly:

$ COMP_WORDS=(git status -- ar) COMP_CWORD=3; time _git

real0m1.372s
user0m0.263s
sys 0m0.167s

The measurements were done with mingw64 bash, which is used by Git for
Windows.

Signed-off-by: Clemens Buchacher <dri...@gmx.net>
---
 contrib/completion/git-completion.bash | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 6da95b8..e3ddf27 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -384,12 +384,7 @@ __git_index_files ()
local root="${2-.}" file
 
__git_ls_files_helper "$root" "$1" |
-   while read -r file; do
-   case "$file" in
-   ?*/*) echo "${file%%/*}" ;;
-   *) echo "$file" ;;
-   esac
-   done | sort | uniq
+   sed -e '/^\//! s#/.*##' | sort | uniq
 }
 
 # Lists branches from the local repository.
-- 
2.7.4

[PATCH 2/2] completion: simplify ls-files filter

[Clemens Buchacher]

When filtering the ls-files output we take care not to touch absolute
paths. This is redundant, because ls-files will never output absolute
paths. Furthermore, sorting the output is also redundant, because the
output of ls-files is already sorted.

Remove the unnecessary operations.

Signed-off-by: Clemens Buchacher <dri...@gmx.net>
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index e3ddf27..394c3df 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -384,7 +384,7 @@ __git_index_files ()
local root="${2-.}" file
 
__git_ls_files_helper "$root" "$1" |
-   sed -e '/^\//! s#/.*##' | sort | uniq
+   cut -f1 -d/ | uniq
 }
 
 # Lists branches from the local repository.
-- 
2.7.4

[PATCH] git-gui: workaround ttk:style theme use

[Clemens Buchacher]

Tk 8.5.7, which is the latest version on Centos 6, does not support
getting the current theme with [ttk::style theme use]. Use the existing
workaround for this in all places.

Signed-off-by: Clemens Buchacher <dri...@gmx.net>
---
 git-gui/lib/themed.tcl | 15 ++-
 1 file changed, 10 insertions(+), 5 deletions(-)

diff --git a/git-gui/lib/themed.tcl b/git-gui/lib/themed.tcl
index 351a712..88b3119 100644
--- a/git-gui/lib/themed.tcl
+++ b/git-gui/lib/themed.tcl
@@ -1,6 +1,14 @@
 # Functions for supporting the use of themed Tk widgets in git-gui.
 # Copyright (C) 2009 Pat Thoyts <pattho...@users.sourceforge.net>
 
+proc ttk_get_current_theme {} {
+   # Handle either current Tk or older versions of 8.5
+   if {[catch {set theme [ttk::style theme use]}]} {
+   set theme  $::ttk::currentTheme
+   }
+   return $theme
+}
+
 proc InitTheme {} {
# Create a color label style (bg can be overridden by widget option)
ttk::style layout Color.TLabel {
@@ -28,10 +36,7 @@ proc InitTheme {} {
}
}
 
-   # Handle either current Tk or older versions of 8.5
-   if {[catch {set theme [ttk::style theme use]}]} {
-   set theme  $::ttk::currentTheme
-   }
+   set theme [ttk_get_current_theme]
 
if {[lsearch -exact {default alt classic clam} $theme] != -1} {
# Simple override of standard ttk::entry to change the field
@@ -248,7 +253,7 @@ proc tspinbox {w args} {
 proc ttext {w args} {
global use_ttk
if {$use_ttk} {
-   switch -- [ttk::style theme use] {
+   switch -- [ttk_get_current_theme] {
"vista" - "xpnative" {
lappend args -highlightthickness 0 -borderwidth 0
}
-- 
2.7.4

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

On Mon, Feb 01, 2016 at 10:17:24AM -0800, Junio C Hamano wrote:
> 
> Your proposal is to redefine "is the working tree dirty?"; it would
> check if "git checkout -f" would change what is in the working tree.

I like this definition. Sounds obviously right.

> > Regarding performance impact: We only need to do this extra check if the
> > usual check convert_to_git(work tree) against index state fails, and
> > conversion is in effect.
> 
> How would you detect the failure, though?  Having contents in the
> index that contradicts the attributes and eol settings affects the
> cleanliness both ways.  Running the working tree contents via to-git
> conversion and hashing may match the blob in the index, declaring
> that the index entry is "clean", but passing the blob to to-worktree
> conversion may produce result different from what is in the
> worktree, which is "falsely clean".

True. But this is what we do today, and I thought at first that we have
to keep this behavior. The following enables eol conversion on git add,
but not on checkout:

 printf 'line 1\r\n' >dos.txt
 echo '* text' >.gitattributes
 git add dos.txt
 git commit

After git add the worktree is considered clean, even though dos.txt
still has CRLF line endings, and rm dos.txt && git checkout dos.txt
re-creates dos.txt with LF line endings. If we change the definition as
proposed above, then the worktree would be dirty even though we just
did git add and git commit.

So I concluded that we have to treat the worktree clean if either git
add -u does not change the index state, _or_ git checkout -f does not
change the worktree state.

But doing only the git checkout -f check makes much more sense. Maybe we
can handle the above situation better by doing an implicit
git checkout -f  after git commit. After all, I would
expect git commit to give me exactly the same state that I get later
when I do git checkout  for the same commit.

> > On the other hand, a user who simply follows an upstream repository by
> > doing git pull all the time, and who does not make changes to their
> > configuration, can still run into this issue, because upstream could
> > change .gitattributes. This part we could actually detect by hashing the
> > attributes for each index entry, and if that changes we re-evaluate the
> > file state.
> 
> If this has to bloat each index entry, I do not think solving the
> problem is worth that cost of that overhead.  I'd rather just say
> "if you have inconsistent data, here is a workaround using 'reset'
> and then 'reset --hard'" and be done with it.

Works for me.

> > This is also an issue only if a smudge filter is in place. The eol
> > conversion which only acts in the convert_to_git direction is not
> > affected.
> 
> IIRC, autocrlf=true would strip CR at the end of line in to-git
> conversion, and would add CR in to-worktree conversion.  So some eol
> conversion may only act in to-git, but some others do affect both,
> and without needing you to touch attributes.

I was somehow under the impression that autocrlf=true is discouraged,
and setting the text attribute to true is the new recommended way to
configure eol conversion. But I see that the Git for Windows installer
still offers autocrlf=true as the default option, so clearly we need to
support it well.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

On Thu, Jan 28, 2016 at 01:32:30PM -0800, Junio C Hamano wrote:
> Clemens Buchacher <dri...@aon.at> writes:
> 
> > If we do this, then git diff should show the diff between
> > convert_to_worktree(index state) and the worktree state.
> 
> And that unfortunately is a very good reason why this approach
> should not be taken.

Ok, then let's take a step back. I do not actually care if git diff and
friends say the worktree is clean or not. But I know that I did not make
any modifications to the worktree, because I just did git reset --hard.
And now I want to use commands like cherry-pick and checkout without
failure. But they can fail, because they essentially use git diff to
check if there are worktree changes, and if so refuse to overwrite them.

So, if the check "Am I allowed to modify the worktree file?", would go
the extra mile to also check if the worktree is clean in the sense that
convert_to_worktree(index state) matches the worktree. If this is the
case, then it is safe to modify the file because it is the committed
state, and can be recovered.

Regarding performance impact: We only need to do this extra check if the
usual check convert_to_git(work tree) against index state fails, and
conversion is in effect.

> Besides, I do not think the above approach really solves the issue,
> either.  After "git reset --hard" to have the contents in the index
> dumped to the working tree, if your core.autocrlf is flipped,

Indeed, if the user configuration changes, then we cannot always detect
this (at least if the filter is an external program, and the behavior of
that changes). But the user is in control of that, and we can document
this limitation.

On the other hand, a user who simply follows an upstream repository by
doing git pull all the time, and who does not make changes to their
configuration, can still run into this issue, because upstream could
change .gitattributes. This part we could actually detect by hashing the
attributes for each index entry, and if that changes we re-evaluate the
file state.

This is also an issue only if a smudge filter is in place. The eol
conversion which only acts in the convert_to_git direction is not
affected.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] optionally disable gitattributes

[Clemens Buchacher]

On Wed, Jan 27, 2016 at 04:04:39PM +0100, Torsten Bögershausen wrote:
>
> It feels like a workaround for something that could be fixable, or is already 
> ongoing.
> Before going into more details,
> could you tell us which attributes you are typically using (when having this 
> problems) ?
> Is it
> * text=auto
> or
> *.sh text 
> or something else?

My concrete use case is the text attribute, as in your example: "*.sh
text". But I think of the patch as a more general solution for cases
where we want to work with the files as they are committed, without
having to deal with not normalized files or other conversions due to
gitattributes.

Please note that you may also want to read my reply to the other thread
that Junio mentioned: [PATCH] travis-ci: run previously failed tests
first, then slowest to fastest.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

On Wed, Jan 27, 2016 at 12:49:31PM -0800, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > I wonder what would break if we ask this question instead:
> >
> > We do not know if the working tree file and the indexed data
> > match.  Let's see if "git checkout" of that path would leave the
> > same data as what currently is in the working tree file.

If we do this, then git diff should show the diff between
convert_to_worktree(index state) and the worktree state. That would be
nice, because the diff would actually show what we have in the worktree.
It keeps confusing me that with eol conversion enabled, git diff does
not actually show me the worktree state.

However, even if the file is clean in that direction, there could be a
mismatch between convert_to_git(worktree state) and the index state.
This will happen for example in t0025.4, where we have a CRLF file in
the index and the worktree, but convert_to_git converts it to a file
with LF line endings. Still, I do not see a problem if we also provide a
command like git add --fix-index, which will force normalization of all
files.

> > If we did this, "reset --hard HEAD" followed by "diff HEAD" will by
> > definition always report "is clean" as long as nobody changes files
> > in the working tree, even with the inconsistent data in the index.

Yes, this is a more elegant and a more complete solution to the problem
which prompted me to submit the GIT_ATTRIBUTES_DISABLED patch.

> > This still requires that convert_to_working_tree(), i.e. your smudge
> > filter, is deterministic, though, but I think that is a sensible
> > assumption for sane people, even for those with inconsistent data in
> > the index.

Deterministic, yes. But not unchanging. When a smudge filter is added,
or modified, or if the filter program changes, we still have to remove
the index before we can trust git diff again. The only way to avoid this
would be to somehow detect if the conversion itself changes. One could
hash the attributes, but changes to the filter configuration or the
filter itself are hard to detect. So I think we have to live with this.

> [...] Doing the other check will have to
> inflate the blob data and apply the convert_to_working_tree()
> processing, and also read the whole thing from the filesystem and
> compare, which is more work at runtime.

If we assume that the smudge filter is deterministic, then we could also
hash the output of convert_to_working_tree, and store the hash in the
index. With this optimization, the comparision would be less work,
because we do not have to apply a filter again, whereas currently we
have to apply convert_to_git.

> IOW, I am saying that the "add --fix-index" lunchbreak patch I sent
> earlier in the thread that has to hold the data in-core while
> processing is not a production quality patch ;-)

Ok. The existing implementation in renormalize_buffer (convert.c) works
for me, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] optionally disable gitattributes

[Clemens Buchacher]

If committed files are not normalized, adding gitattributes has the
side effect that such files are shown as modified, even though they
were not actually modified by the user, and the work tree matches
the committed file. This is because with gitattributes, the file is
modified on the fly when git reads it from disk, before it compares
with the index contents.

This is desirable in most situations, because it makes the user
aware that files should be normalized. However, it can become an
issue for automation. Since Git considers the work tree to be
dirty, some operations such as git rebase or git cherry-pick refuse
to operate. Those commands offer no flag to force overwrite work
tree changes. The only options are to commit the changes, or to
remove gitattributes, but that changes the repository state, which
may be undesirable in a scripted context.

Introduce an environment variable GIT_ATTRIBUTES_DISABLED, which if
set makes Git ignore any gitattributes.

Signed-off-by: Clemens Buchacher <dri...@aon.at>
---
 Documentation/git.txt   |  4 
 Documentation/gitattributes.txt |  6 ++
 attr.c  |  3 +++
 t/t0003-attributes.sh   | 43 +
 4 files changed, 56 insertions(+)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index bff6302..00f4e3b 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -1132,6 +1132,10 @@ of clones and fetches.
  - any external helpers are named by their protocol (e.g., use
`hg` to allow the `git-remote-hg` helper)
 
+'GIT_ATTRIBUTES_DISABLED'::
+   If set, attributes are disabled for all paths. See
+   linkgit:gitattributes[1] for more details on attributes.
+
 
 Discussion[[Discussion]]
 
diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
index e3b1de8..f6a2b1d 100644
--- a/Documentation/gitattributes.txt
+++ b/Documentation/gitattributes.txt
@@ -996,6 +996,12 @@ frotz  unspecified
 
 
 
+ENVIRONMENT
+---
+
+GIT_ATTRIBUTES_DISABLED::
+   If set, attributes are disabled for all paths.
+
 SEE ALSO
 
 linkgit:git-check-attr[1].
diff --git a/attr.c b/attr.c
index 086c08d..0fa2f1a 100644
--- a/attr.c
+++ b/attr.c
@@ -547,6 +547,9 @@ static void prepare_attr_stack(const char *path, int dirlen)
int len;
const char *cp;
 
+   if (getenv("GIT_ATTRIBUTES_DISABLED"))
+   return;
+
/*
 * At the bottom of the attribute stack is the built-in
 * set of attribute definitions, followed by the contents
diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
index f0fbb42..26e6766 100755
--- a/t/t0003-attributes.sh
+++ b/t/t0003-attributes.sh
@@ -13,6 +13,14 @@ attr_check () {
test_line_count = 0 err
 }
 
+attr_check_disabled () {
+   (
+   GIT_ATTRIBUTES_DISABLED=t
+   export GIT_ATTRIBUTES_DISABLED
+   attr_check "$@" unspecified
+   )
+}
+
 test_expect_success 'setup' '
mkdir -p a/b/d a/c b &&
(
@@ -84,6 +92,41 @@ test_expect_success 'attribute test' '
attr_check a/b/d/yes unspecified
 '
 
+test_expect_success 'gitattributes disabled' '
+   attr_check_disabled f &&
+   attr_check_disabled a/f &&
+   attr_check_disabled a/c/f &&
+   attr_check_disabled a/g &&
+   attr_check_disabled a/b/g &&
+   attr_check_disabled b/g &&
+   attr_check_disabled a/b/h &&
+   attr_check_disabled a/b/d/g &&
+   attr_check_disabled onoff &&
+   attr_check_disabled offon &&
+   attr_check_disabled no &&
+   attr_check_disabled a/b/d/no &&
+   attr_check_disabled a/b/d/yes
+'
+
+test_expect_success 'no changes if gitattributes disabled' '
+   mkdir clean &&
+   git init clean &&
+   (
+   cd clean &&
+   printf "foo\r\n" >dos.txt &&
+   git add dos.txt &&
+   test_tick &&
+   git commit -q -m dos.txt &&
+   echo "*.txt text eol=lf" >.gitattributes &&
+   git add .gitattributes &&
+   test_tick &&
+   git commit -q -m .gitattributes &&
+   rm -f .git/index &&
+   git reset &&
+   GIT_ATTRIBUTES_DISABLED=t git diff --exit-code
+   )
+'
+
 test_expect_success 'attribute matching is case sensitive when 
core.ignorecase=0' '
 
test_must_fail attr_check F f "-c core.ignorecase=0" &&
-- 
2.7.0

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

Re: [PATCH] travis-ci: run previously failed tests first, then slowest to fastest

[Clemens Buchacher]

I think Junio pointed me to this thread from "[PATCH] optionally disable
gitattributes". Since I am not sure I am following everything correctly
in this thread, allow me to recapitulate what I understood so far.

Firstly, I think the racy'ness of t0025 is understood. It is due to the
is_racy_timestamp check in read-cache.c's ie_match_stat. But for the
moment I would like to put this aside, because the issue can be
reproduced reliably with this change to t0025:

diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
index c164b46..e30e9b3 100755
--- a/t/t0025-crlf-auto.sh
+++ b/t/t0025-crlf-auto.sh
@@ -55,8 +55,11 @@ test_expect_success 'crlf=true causes a CRLF file to be 
normalized' '
 test_expect_success 'text=true causes a CRLF file to be normalized' '
 
rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
-   echo "CRLFonly text" > .gitattributes &&
git read-tree --reset -u HEAD &&
+   sleep 1 &&
+   rm .git/index &&
+   git reset &&
+   echo "CRLFonly text" > .gitattributes &&
 
# Note, "normalized" means that git will normalize it if added
has_cr CRLFonly &&

I intentionally wait for one second and then I remove and re-read the
index. Now the timestamps of CRLFonly and .git/index are different, so
we avoid the is_racy_timestamp check. From now on Git will not read the
contents of CRLFonly from disk again until either the index entry or the
mtime of CRLFonly changes (maybe we also check the size, I am not sure).

Now we add .gitattributes. This does not change the index entry, nor
does it change the mtime of CRLFonly. Therefore the subsequent git diff
turns out empty, and the test fails.

I believe this behavior is expected. In gitattributes(5) we therefore
recommend using rm .git/index and git reset to "force Git to rescan the
working directory." The test should be fixed accordingly, something
like:

diff --git a/t/t0025-crlf-auto.sh b/t/t0025-crlf-auto.sh
index c164b46..2917591 100755
--- a/t/t0025-crlf-auto.sh
+++ b/t/t0025-crlf-auto.sh
@@ -57,6 +57,8 @@ test_expect_success 'text=true causes a CRLF file to be 
normalized' '
rm -f .gitattributes tmp LFonly CRLFonly LFwithNUL &&
echo "CRLFonly text" > .gitattributes &&
git read-tree --reset -u HEAD &&
+   rm .git/index &&
+   git reset &&
 
# Note, "normalized" means that git will normalize it if added
has_cr CRLFonly &&



On Mon, Jan 25, 2016 at 01:52:18PM -0800, Junio C Hamano wrote:
> Junio C Hamano  writes:
> 
> > I do not think there is a canned command to help dealing with these
> > broken paths right now.

I think (rm .git/index && git reset) works well enough in most cases,
but not all:

> We could go even fancier and attempt the round-trip twice or more.
> It is possible that the in-index representation will not converge
> when you use a misconfigured pair of clean/smudge filters

This can also happen with eol conversion, for example if you have files
with CRCRLF line endings. The eol conversion will remove only one CR.
Two conversions would be needed to achieve a normalized format. But
iterating (rm .git/index && git reset) does not help. Since we do not
touch the file on disk, after the first round, we have CRCRLF on disk
and CRLF in the index. During the second round, Git reads CRCRLF from
disk again, converts it to CRLF, which matches the index. Even
git reset --hard will not checkout the CRLF version to the worktree.

A possible solution is to iterate (rm -r * && git checkout -- . && git
add -u) until the work tree is clean. Quite ugly.

A command like git add --fix-index would make this conversion less
painful.  It should be ok if the user has to run it several times in
corner cases like CRCRLF, but it would be nice to issue a warning if the
index is still not normalized after running git add --fix-index.

Regarding the name of the option, maybe git add --renormalize-index
would be more consistent, since we also have the related merge option
"renormalize", which is very similar. In fact possibly you can share
some code with it.

Your patch looks good to me otherwise.


Coming back to "[PATCH] optionally disable gitattributes": The topics
are related, because they both deal with the situation where the work
tree has files which are not normalized according to gitattributes. But
my patch is more about saying: ok, I know I may have files which need to
be normalized, but I want to ignore this issue for now. Please disable
gitattributes for now, because I want to work with the files as they are
committed. Conversely, the discussion here is about how to reliably
detect and fix files which are not normalized.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

Hi Junio,

I believe we have finalized the discussion on this patch. Please apply

On Fri, Nov 13, 2015 at 06:23:20PM -0500, Jeff King wrote:
> 
> > +test_expect_success 'filling pipe buffer does not cause failure' '
> > +   git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
> > +   test_cmp expected actual
> > +'
> 
> It actually _does_ read all of the input, but I guess is making sure we
> call write() in a loop. I don't know if this is even worth keeping.
> 
> Can you think of a good reason that it is checking something
> interesting?

No, I also cannot think of a good reason to keep it. Here is the patch
with the test above removed.

Best regards,
Clemens

--o<--
Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input stream)
the pre-receive and post-receive hooks ignore SIGPIPE. Do the same for the
remaining hooks pre-push and post-rewrite, which read from standard input. The
same arguments for ignoring SIGPIPE apply.

Include test by Jeff King which checks that SIGPIPE does not cause
pre-push hook failure. With the use of git update-ref --stdin it is fast
enough to be enabled by default.

Signed-off-by: Clemens Buchacher <clemens.buchac...@intel.com>
---
 builtin/commit.c |  3 +++
 t/t5571-pre-push-hook.sh | 33 +++--
 transport.c  | 11 +--
 3 files changed, 27 insertions(+), 20 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dca09e2..f2a8b78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
 #include "sequencer.h"
 #include "notes-utils.h"
 #include "mailmap.h"
+#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
@@ -1537,8 +1538,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
return code;
n = snprintf(buf, sizeof(buf), "%s %s\n",
 sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   sigchain_push(SIGPIPE, SIG_IGN);
write_in_full(proc.in, buf, n);
close(proc.in);
+   sigchain_pop(SIGPIPE);
return finish_command();
 }
 
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..ba975bb 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,20 @@ test_expect_success 'push to URL' '
diff expected actual
 '
 
-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-   printf 'parent1\nrepo1\n' >expected
-   nr=1000
-   while test $nr -lt 2000
-   do
-   nr=$(( $nr + 1 ))
-   git branch b/$nr $COMMIT3
-   echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" 
>>expected
-   done
-
-   test_expect_success 'push many refs' '
-   git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-   diff expected actual
-   '
-fi
+test_expect_success 'set up many-ref tests' '
+   {
+   nr=1000
+   while test $nr -lt 2000
+   do
+   nr=$(( $nr + 1 ))
+   echo "create refs/heads/b/$nr $COMMIT3"
+   done
+   } | git update-ref --stdin
+'
+
+test_expect_success 'sigpipe does not cause pre-push hook failure' '
+   echo "exit 0" | write_script "$HOOK" &&
+   git push parent1 "refs/heads/b/*:refs/heads/b/*"
+'
 
 test_done
diff --git a/transport.c b/transport.c
index 23b2ed6..e34ab92 100644
--- a/transport.c
+++ b/transport.c
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "sigchain.h"
 
 /* rsync support */
 
@@ -1127,6 +1128,8 @@ static int run_pre_push_hook(struct transport *transport,
return -1;
}
 
+   sigchain_push(SIGPIPE, SIG_IGN);
+
strbuf_init(, 256);
 
for (r = remote_refs; r; r = r->next) {
@@ -1140,8 +1143,10 @@ static int run_pre_push_hook(struct transport *transport,
 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 r->name, sha1_to_hex(r->old_sha1));
 
-   if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
-   ret = -1;
+   if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+   /* We do not mind if a hook does not read all refs. */
+   if (errno != EPIPE)
+   ret = -1;
break;
}
}
@@ -1152,6 +1157,8 @@ static int run_pre_push_hook(struct transport *transport,
if (!ret)
ret = x;
 
+   sigchain_pop(SIGPIPE);
+
x = finish_command();
if (!ret)
ret = x;
-- 
1.9.4

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

Re: [PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

On Fri, Nov 13, 2015 at 01:17:29AM -0500, Jeff King wrote:
> 
> The test below reliably fails without your patch and passes with it, and
> seems to run reasonably quickly for me:

Thank you. I confirm the same behavior on my system. Below I have added
your change to the patch.

-->o--
Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input stream)
the pre-receive and post-receive hooks ignore SIGPIPE. Do the same for the
remaining hooks pre-push and post-rewrite, which read from standard input. The
same arguments for ignoring SIGPIPE apply.

Performance improvements which allow us to enable the test by
default by Jeff King.

Signed-off-by: Clemens Buchacher <clemens.buchac...@intel.com>
---
 builtin/commit.c |  3 +++
 t/t5571-pre-push-hook.sh | 41 +++--
 transport.c  | 11 +--
 3 files changed, 35 insertions(+), 20 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dca09e2..f2a8b78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
 #include "sequencer.h"
 #include "notes-utils.h"
 #include "mailmap.h"
+#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
@@ -1537,8 +1538,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
return code;
n = snprintf(buf, sizeof(buf), "%s %s\n",
 sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   sigchain_push(SIGPIPE, SIG_IGN);
write_in_full(proc.in, buf, n);
close(proc.in);
+   sigchain_pop(SIGPIPE);
return finish_command();
 }
 
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..61df2f9 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,28 @@ test_expect_success 'push to URL' '
diff expected actual
 '
 
-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-   printf 'parent1\nrepo1\n' >expected
-   nr=1000
-   while test $nr -lt 2000
-   do
-   nr=$(( $nr + 1 ))
-   git branch b/$nr $COMMIT3
-   echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" 
>>expected
-   done
-
-   test_expect_success 'push many refs' '
-   git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-   diff expected actual
-   '
-fi
+test_expect_success 'set up many-ref tests' '
+   {
+   echo >&3 parent1 &&
+   echo >&3 repo1 &&
+   nr=1000
+   while test $nr -lt 2000
+   do
+   nr=$(( $nr + 1 ))
+   echo "create refs/heads/b/$nr $COMMIT3"
+   echo >&3 "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr 
$_z40"
+   done
+   } 3>expected | git update-ref --stdin
+'
+
+test_expect_success 'filling pipe buffer does not cause failure' '
+   git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
+   test_cmp expected actual
+'
+
+test_expect_success 'sigpipe does not cause pre-push hook failure' '
+   echo "exit 0" | write_script "$HOOK" &&
+   git push parent1 "refs/heads/b/*:refs/heads/c/*"
+'
 
 test_done
diff --git a/transport.c b/transport.c
index 23b2ed6..e34ab92 100644
--- a/transport.c
+++ b/transport.c
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "sigchain.h"
 
 /* rsync support */
 
@@ -1127,6 +1128,8 @@ static int run_pre_push_hook(struct transport *transport,
return -1;
}
 
+   sigchain_push(SIGPIPE, SIG_IGN);
+
strbuf_init(, 256);
 
for (r = remote_refs; r; r = r->next) {
@@ -1140,8 +1143,10 @@ static int run_pre_push_hook(struct transport *transport,
 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 r->name, sha1_to_hex(r->old_sha1));
 
-   if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
-   ret = -1;
+   if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+   /* We do not mind if a hook does not read all refs. */
+   if (errno != EPIPE)
+   ret = -1;
break;
}
}
@@ -1152,6 +1157,8 @@ static int run_pre_push_hook(struct transport *transport,
if (!ret)
ret = x;
 
+   sigchain_pop(SIGPIPE);
+
x = finish_command();
if (!ret)
ret = x;
-- 
1.9.4

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

Re: [PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

On Wed, Nov 11, 2015 at 03:39:20PM +0100, Clemens Buchacher wrote:
> + if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
> + /* We do not mind if a hook does not read all refs. */
> + if (errno != EPIPE)
> + ret = -1;

I can reproduce the pipe error reliably with the test below. I did not
include it in the patch since I am in doubt if we should add an optional
sleep to the code.

-->o--
diff --git a/t/t5571-pre-push-hook.sh b/t/t5571-pre-push-hook.sh
index 6f9916a..8cfe59a 100755
--- a/t/t5571-pre-push-hook.sh
+++ b/t/t5571-pre-push-hook.sh
@@ -109,23 +109,13 @@ test_expect_success 'push to URL' '
diff expected actual
 '

-# Test that filling pipe buffers doesn't cause failure
-# Too slow to leave enabled for general use
-if false
-then
-   printf 'parent1\nrepo1\n' >expected
-   nr=1000
-   while test $nr -lt 2000
-   do
-   nr=$(( $nr + 1 ))
-   git branch b/$nr $COMMIT3
-   echo "refs/heads/b/$nr $COMMIT3 refs/heads/b/$nr $_z40" 
>>expected
-   done
-
-   test_expect_success 'push many refs' '
-   git push parent1 "refs/heads/b/*:refs/heads/b/*" &&
-   diff expected actual
-   '
-fi
+write_script "$HOOK" <<\EOF
+exit 0
+EOF
+
+test_expect_success 'hook does not consume input' '
+git branch noinput &&
+GIT_TEST_SIGPIPE=t git push parent1 noinput
+'

 test_done
diff --git a/transport.c b/transport.c
index 23b2ed6..d83ef1c 100644
--- a/transport.c
+++ b/transport.c
@@ -1129,6 +1129,8 @@ static int run_pre_push_hook(struct transport *transport,

strbuf_init(, 256);

+   if (getenv("GIT_TEST_SIGPIPE"))
+   sleep_millisec(10);
for (r = remote_refs; r; r = r->next) {
if (!r->peer_ref) continue;
if (r->status == REF_STATUS_REJECT_NONFASTFORWARD) continue;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] allow hooks to ignore their standard input stream

[Clemens Buchacher]

Since ec7dbd145 (receive-pack: allow hooks to ignore its standard input
stream) the pre-receive and post-receive hooks ignore SIGPIPE. Do the
same for the remaining hooks pre-push and post-rewrite, which read from
standard input. The same arguments for ignoring SIGPIPE apply.

Signed-off-by: Clemens Buchacher <clemens.buchac...@intel.com>
---
 builtin/commit.c |  3 +++
 transport.c  | 11 +--
 2 files changed, 12 insertions(+), 2 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index dca09e2..f2a8b78 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -32,6 +32,7 @@
 #include "sequencer.h"
 #include "notes-utils.h"
 #include "mailmap.h"
+#include "sigchain.h"
 
 static const char * const builtin_commit_usage[] = {
N_("git commit [] [--] ..."),
@@ -1537,8 +1538,10 @@ static int run_rewrite_hook(const unsigned char *oldsha1,
return code;
n = snprintf(buf, sizeof(buf), "%s %s\n",
 sha1_to_hex(oldsha1), sha1_to_hex(newsha1));
+   sigchain_push(SIGPIPE, SIG_IGN);
write_in_full(proc.in, buf, n);
close(proc.in);
+   sigchain_pop(SIGPIPE);
return finish_command();
 }
 
diff --git a/transport.c b/transport.c
index 23b2ed6..e34ab92 100644
--- a/transport.c
+++ b/transport.c
@@ -15,6 +15,7 @@
 #include "submodule.h"
 #include "string-list.h"
 #include "sha1-array.h"
+#include "sigchain.h"
 
 /* rsync support */
 
@@ -1127,6 +1128,8 @@ static int run_pre_push_hook(struct transport *transport,
return -1;
}
 
+   sigchain_push(SIGPIPE, SIG_IGN);
+
strbuf_init(, 256);
 
for (r = remote_refs; r; r = r->next) {
@@ -1140,8 +1143,10 @@ static int run_pre_push_hook(struct transport *transport,
 r->peer_ref->name, sha1_to_hex(r->new_sha1),
 r->name, sha1_to_hex(r->old_sha1));
 
-   if (write_in_full(proc.in, buf.buf, buf.len) != buf.len) {
-   ret = -1;
+   if (write_in_full(proc.in, buf.buf, buf.len) < 0) {
+   /* We do not mind if a hook does not read all refs. */
+   if (errno != EPIPE)
+   ret = -1;
break;
}
}
@@ -1152,6 +1157,8 @@ static int run_pre_push_hook(struct transport *transport,
if (!ret)
ret = x;
 
+   sigchain_pop(SIGPIPE);
+
x = finish_command();
if (!ret)
ret = x;
-- 
1.9.4

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

Re: [PATCH] git_open_noatime: return with errno=0 on success

[Clemens Buchacher]

On Wed, Aug 05, 2015 at 10:59:09AM +0200, Linus Torvalds wrote:
 On Tue, Aug 4, 2015 at 11:03 PM, Junio C Hamano gits...@pobox.com wrote:
 
  I would agree it is a good idea to clear it after seeing the first
  open fail due to lack of O_NOATIME before trying open for the second
  time, iow, more like this?

Looks good to me.

 So I don't think this is _wrong_ per se, but I think the deeper issue
 is that somebody cares about 'errno' here in the first place.
 
 A stale 'errno' generally shouldn't matter, because we either
 
  (a) return success (and nobody should look at errno)
 
 or
 
  (b) return an error later, without setting errno for that _later_ error.
 
 and I think either of those two situations are the real bug, and this
 clear stale errno is just a workaround.

I agree. But I do not see how to get there easily.

We are trying to read an object. We first try to read from a pack. We
may encounter broken pack files, missing index files, unreadable files,
but those errors are not necessarily fatal since we may still be able to
read the object from the next pack file or from a sha1 file.

If finally we do not find the object anywhere, in
read_sha1_file_extended we try our best to die with an appropriate error
message, for example by looking at errno, and otherwise we just return
NULL. Most callers seem to die explicitly or they dereference the null
pointer.

I think we should instead output error messages closer to the source,
like for example in map_sha1_file, but continue anyway. In particular we
should immediately report failures due to EPERM or unexpected ENOENT. In
the end we may return NULL without another message, but at least the
user should have some hints about what went wrong along the way.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git_open_noatime: return with errno=0 on success

[Clemens Buchacher]

In read_sha1_file_extended we die if read_object fails with a fatal
error. We detect a fatal error if errno is non-zero and is not
ENOENT. If the object could not be read because it does not exist,
this is not considered a fatal error and we want to return NULL.

Somewhere down the line, read_object calls git_open_noatime to open
a pack index file, for example. We first try open with O_NOATIME.
If O_NOATIME fails with EPERM, we retry without O_NOATIME. When the
second open succeeds, errno is however still set to EPERM from the
first attempt. When we finally determine that the object does not
exist, read_object returns NULL and read_sha1_file_extended dies
with a fatal error:

fatal: failed to read object sha1: Operation not permitted

Fix this by resetting errno to zero before we call open again.

Cc: Linus Torvalds torva...@linux-foundation.org
Signed-off-by: Clemens Buchacher clemens.buchac...@intel.com
---

This is a re-submission without changes except for a typo fix in the
comments (thanks Eric). The original submission received no other
comments, but I think it is a clear improvement and I hope it was just
missed the first time.

Best regards,
Clemens

 sha1_file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sha1_file.c b/sha1_file.c
index 77cd81d..62b7ad6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1453,6 +1453,7 @@ int git_open_noatime(const char *name)
static int sha1_file_open_flag = O_NOATIME;
 
for (;;) {
+   errno = 0;
int fd = open(name, O_RDONLY | sha1_file_open_flag);
if (fd = 0)
return fd;
-- 
1.9.4

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

[PATCH] git_open_noatime: return with errno=0 on success

[Clemens Buchacher]

In read_sha1_file_extended we die if read_object fails with a fatal
error. We detect a fatal error if errno is non-zero and is not
ENOENT. If the object could not be read because it does not exist,
this is not considered a fatal error and we want to return NULL.

Somewhere down the line, read_object calls git_open_noatime to open
a pack index file, for example. We first try open with O_NOATIME.
If O_NOATIME fails with EPERM, we retry without O_NOATIME. When the
second open succeeds, errno is however still set to EPERM from the
first attemt. When we finally determine that the object does not
exist, read_object returns NULL and read_sha1_file_extended dies
with a fatal error:

fatal: failed to read object sha1: Operation not permitted

Fix this by resetting errno to zero before we call open again.

Cc: Linus Torvalds torva...@linux-foundation.org
Signed-off-by: Clemens Buchacher clemens.buchac...@intel.com
Helped-by: Martin Schröder martin.h.schroe...@intel.com
---
 sha1_file.c | 1 +
 1 file changed, 1 insertion(+)

diff --git a/sha1_file.c b/sha1_file.c
index 77cd81d..62b7ad6 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1453,6 +1453,7 @@ int git_open_noatime(const char *name)
static int sha1_file_open_flag = O_NOATIME;
 
for (;;) {
+   errno = 0;
int fd = open(name, O_RDONLY | sha1_file_open_flag);
if (fd = 0)
return fd;
-- 
1.9.4

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

Re: [PATCH] rebase: return non-zero error code if format-patch fails

[Clemens Buchacher]

On Fri, Jul 03, 2015 at 10:52:32AM -0700, Junio C Hamano wrote:
 
  Cc: Andrew Wong andrew.k...@gmail.com
  Signed-off-by: Clemens Buchacher clemens.buchac...@intel.com
  Reviewed-by: Jorge Nunes jorge.nu...@intel.com
 
 Where was this review made?  I may have missed a recent discussion,
 and that is why I am asking, because Reviewed-by: lines that cannot
 be validated by going back to the list archive does not add much
 value.

Jorge helped me by reviewing the patch before I submitted it to the
list. My intention is to give credit for his contribution, and to
involve him in any discussion regarding the patch. Maybe it makes more
sense to say Helped-by:? Please feel free to change as you see fit. I
will follow your recommendation in the future.

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

[PATCH] rebase: return non-zero error code if format-patch fails

[Clemens Buchacher]

Since e481af06 (rebase: Handle cases where format-patch fails) we
notice if format-patch fails and return immediately from
git-rebase--am. We save the return value with ret=$?, but then we
return $?, which is usually zero in this case.

Fix this by returning $ret instead.

Cc: Andrew Wong andrew.k...@gmail.com
Signed-off-by: Clemens Buchacher clemens.buchac...@intel.com
Reviewed-by: Jorge Nunes jorge.nu...@intel.com
---
 git-rebase--am.sh | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index f923732..9ae898b 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -78,7 +78,7 @@ else
 
As a result, git cannot rebase them.
EOF
-   return $?
+   return $ret
fi
 
git am $git_am_opt --rebasing --resolvemsg=$resolvemsg \
-- 
1.9.4

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

[PATCH] filter-branch: handle deletion of annotated tags

[Clemens Buchacher]

If filter-branch removes a commit which an annotated tag points to,
and that tag is in the list of refs to be rewritten, we die with an
error like this:

error: Ref refs/tags/a1t is at 699cce2774f79a0830d8c5f631deca12d4b1ee8c but 
expected ba247450492030b03e3d2a9d5fef7ef67519483e
Could not delete refs/tags/a1t

In order to update refs, we first peel the ref until we find a
commit sha1. We then pass the commit sha1 to update-ref as the
oldvalue parameter. Please consider the following scenarios:

 a) The ref points to a commit object directly. In this case,
update-ref will find that the current value of the ref still
matches oldvalue, and succeeds. This check is redundant, since
we only just queried the current value.

 b) The ref points to a tag object. In this case, update-ref will
error out, since the commit sha1 cannot match the current value
of the ref. If the commit has been removed, or rewritten into
multiple commits, we simply die. If the commit has been
rewritten, we output a warning message saying that to rewrite
tags one should use --tag-name-filter, and then we continue. If
--tag-name-filter is active, the tag will later be rewritten.

There seems to be no added value in passing the oldvalue
parameter. So remove it.

This fixes deletion of tag objects. We also do not die any more if
a tag object points to a commit which has been rewritten into
multiple commits. However, we probably will die later in the
--tag-name-filter code, because it does not seem to handle this
case.

This is a minimalist fix which leaves the following issues open:

 o In the absence of --tag-name-filter, we rewrite lightweight tags, but
   not annotated tags, which is not intuitive. We do output a warning,
   though:

   $ git filter-branch --msg-filter cat  echo hi -- --all
   [...]
   WARNING: You said to rewrite tagged commits, but not the corresponding tag.
   WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.

 o Annotated tags are backed up as lightweight tags.

 o Annotated tags are backed up even in the absence of
   --tag-name-filter. But in this case backup is not needed because
   they are not rewritten.

These issues could be solved by moving the tag rewriting logic from
tag-name-filter to the regular ref updating code, and
tag-name-filter should deal only with renaming tags. However, this
would change behavior. Currently, the following command would
rewrite tags:

git filter-branch --msg-filter cat  echo hi \
--tag-name-filter cat -- --branches

With the suggested behavior, tags would be rewritten only if we
include them in the rev-list options:

git filter-branch --msg-filter=cat  echo hi -- --all

I am not sure if we can afford to change behavior like that.

Cc: Thomas Rast tr...@student.ethz.ch
Cc: Johannes Schindelin johannes.schinde...@gmx.de
Signed-off-by: Clemens Buchacher clemens.buchac...@intel.com
Reviewed-by: Jorge Nunes jorge.nu...@intel.com
---
 git-filter-branch.sh | 20 +---
 t/t7003-filter-branch.sh | 21 +
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..7ca1d99 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -399,21 +399,19 @@ do
case $rewritten in
'')
echo Ref '$ref' was deleted
-   git update-ref -m filter-branch: delete -d $ref $sha1 ||
+   git update-ref -m filter-branch: delete -d $ref ||
die Could not delete $ref
;;
$_x40)
echo Ref '$ref' was rewritten
-   if ! git update-ref -m filter-branch: rewrite \
-   $ref $rewritten $sha1 2/dev/null; 
then
-   if test $(git cat-file -t $ref) = tag; then
-   if test -z $filter_tag_name; then
-   warn WARNING: You said to rewrite 
tagged commits, but not the corresponding tag.
-   warn WARNING: Perhaps use 
'--tag-name-filter cat' to rewrite the tag.
-   fi
-   else
-   die Could not rewrite $ref
+   if test $(git cat-file -t $ref) = tag; then
+   if test -z $filter_tag_name; then
+   warn WARNING: You said to rewrite tagged 
commits, but not the corresponding tag.
+   warn WARNING: Perhaps use '--tag-name-filter 
cat' to rewrite the tag.
fi
+   else
+   git update-ref -m filter-branch: rewrite $ref 
$rewritten ||
+   die Could not rewrite $ref
fi
;;
*)
@@ -423,7 +421,7 @@ do
warn WARNING: Ref '$ref' points to the first one now.
rewritten=$(echo $rewritten | head -n

Re: [PATCH] filter-branch: handle deletion of annotated tags

[Clemens Buchacher]

If filter-branch removes a commit which an annotated tag points to,
and that tag is in the list of refs to be rewritten, we die with an
error like this:

error: Ref refs/tags/a1t is at 699cce2774f79a0830d8c5f631deca12d4b1ee8c but 
expected ba247450492030b03e3d2a9d5fef7ef67519483e
Could not delete refs/tags/a1t

In order to update refs, we first peel the ref until we find a
commit sha1. We then pass the commit sha1 to update-ref as the
oldvalue parameter. Please consider the following scenarios:

 a) The ref points to a commit object directly. In this case,
update-ref will find that the current value of the ref still
matches oldvalue, and succeeds. This check is redundant, since
we only just queried the current value.

 b) The ref points to a tag object. In this case, update-ref will
error out, since the commit sha1 cannot match the current value
of the ref. If the commit has been removed, or rewritten into
multiple commits, we simply die. If the commit has been
rewritten, we output a warning message saying that to rewrite
tags one should use --tag-name-filter, and then we continue. If
--tag-name-filter is active, the tag will later be rewritten.

There seems to be no added value in passing the oldvalue
parameter. So remove it.

This fixes deletion of tag objects. We also do not die any more if
a tag object points to a commit which has been rewritten into
multiple commits. However, we probably will die later in the
--tag-name-filter code, because it does not seem to handle this
case.

This is a minimalist fix which leaves the following issues open:

 o In the absence of --tag-name-filter, we rewrite lightweight tags, but
   not annotated tags, which is not intuitive. We do output a warning,
   though:

   $ git filter-branch --msg-filter cat  echo hi -- --all
   [...]
   WARNING: You said to rewrite tagged commits, but not the corresponding tag.
   WARNING: Perhaps use '--tag-name-filter cat' to rewrite the tag.

 o Annotated tags are backed up as lightweight tags.

 o Annotated tags are backed up even in the absence of
   --tag-name-filter. But in this case backup is not needed because
   they are not rewritten.

These issues could be solved by moving the tag rewriting logic from
tag-name-filter to the regular ref updating code, and
tag-name-filter should deal only with renaming tags. However, this
would change behavior. Currently, the following command would
rewrite tags:

git filter-branch --msg-filter cat  echo hi \
--tag-name-filter cat -- --branches

With the suggested behavior, tags would be rewritten only if we
include them in the rev-list options:

git filter-branch --msg-filter=cat  echo hi -- --all

I am not sure if we can afford to change behavior like that.

Cc: Thomas Rast t...@thomasrast.ch
Cc: Johannes Schindelin johannes.schinde...@gmx.de
Signed-off-by: Clemens Buchacher clemens.buchac...@intel.com
---

Re-send with Thomas' email address fixed.

 git-filter-branch.sh | 20 +---
 t/t7003-filter-branch.sh | 21 +
 2 files changed, 30 insertions(+), 11 deletions(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 5b3f63d..7ca1d99 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -399,21 +399,19 @@ do
case $rewritten in
'')
echo Ref '$ref' was deleted
-   git update-ref -m filter-branch: delete -d $ref $sha1 ||
+   git update-ref -m filter-branch: delete -d $ref ||
die Could not delete $ref
;;
$_x40)
echo Ref '$ref' was rewritten
-   if ! git update-ref -m filter-branch: rewrite \
-   $ref $rewritten $sha1 2/dev/null; 
then
-   if test $(git cat-file -t $ref) = tag; then
-   if test -z $filter_tag_name; then
-   warn WARNING: You said to rewrite 
tagged commits, but not the corresponding tag.
-   warn WARNING: Perhaps use 
'--tag-name-filter cat' to rewrite the tag.
-   fi
-   else
-   die Could not rewrite $ref
+   if test $(git cat-file -t $ref) = tag; then
+   if test -z $filter_tag_name; then
+   warn WARNING: You said to rewrite tagged 
commits, but not the corresponding tag.
+   warn WARNING: Perhaps use '--tag-name-filter 
cat' to rewrite the tag.
fi
+   else
+   git update-ref -m filter-branch: rewrite $ref 
$rewritten ||
+   die Could not rewrite $ref
fi
;;
*)
@@ -423,7 +421,7 @@ do
warn WARNING: Ref '$ref' points to the first one now.
rewritten=$(echo $rewritten | head -n 1

Re: Missing capabilities in Documentation/technical/protocol-capbilities.txt

[Clemens Buchacher]

On Mon, Jul 15, 2013 at 07:25:19PM +0700, Duy Nguyen wrote:

 I noticed that quiet and agent capabilities were missing in
 protocol-capabilitities.txt. I have a rough idea what they do, but I
 think it's best to be documented by the authors. Maybe you have some
 time to make a patch?

Hi Duy,

I am sorry to disappoint, but if I had time to work on Git, I'd rather
be writing code. I have some great ideas if you are interested. :-P

Besides, I barely even remember that it was me who implemented the
quiet capability. In order to write documentation for it, I would have
to research the implementation as much as anyone.

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

Re: [PATCH] fix segfault with git log -c --follow

[Clemens Buchacher]

On Tue, May 28, 2013 at 10:22:17AM -0700, Junio C Hamano wrote:
 Clemens Buchacher dri...@aon.at writes:
 
  In diff_tree_combined we make a copy of diffopts. In
  try_to_follow_renames, called via diff_tree_sha1, we free and
  re-initialize diffopts-pathspec-items. Since we did not make a deep
  copy of diffopts in diff_tree_combined, the original diffopts does not
  get the update. By the time we return from diff_tree_combined,
  rev-diffopt-pathspec-items points to an invalid memory address. We
  get a segfault next time we try to access that pathspec.
 
 I am not quite sure if I follow.  Do you mean
 
   diff_tree_combined()
 - makes a shallow copy of rev-diffopt
 - calls diff_tree_sha1()
   diff_tree_sha1()
   - tries to follow rename and clobbers diffopt

Right.

 - tries to use the shallow copy of original rev-diffopt
   that no longer is valid, which is a problem

diff_tree_combined does not try to use it right away. It does return,
but rev-diffopt is now invalid and the next time we do any kind of diff
with it, we have a problem.

 I wonder, just like we force recursive and disable external on the
 copy before we use it to call diff_tree_sha1(), if we should disable
 follow-renames on it.  --follow is an option that is given to the
 history traversal part and it should not play any role in getting the
 pairwise diff with all parents diff_tree_combined() does.

Can't parse that last sentence.

In any case, I don't think disabling diff_tree_sha1 is a solution. The
bug is in diff_tree_sha1 and its subfunctions, because they manipulate a
data structures such that it becomes corrupt. And they do so in an
obfuscated and clearly unintentional manner. So we should not blame the
user for calling diff_tree_sha1 in such a way that it causes corruption.

 Besides,
 
  - --follow hack lets us keep track of only one path; and

Ok. Good to know it is considered a hack. The code is quite strange
indeed.

  - -c and --cc make sense only when dealing with a merge commit
and the path in the child may have come from different path in
parents,

Sorry, I don't get it.

 so I am not sure if allowing combination of --follow -c/--cc makes
 much sense in the first place.

My use-case is came up with this history:

1. Code gets added to file A.
2. File A gets renamed to B in a different branch.
3. The branches get merged, and code from (1) is removed in the merge.

Later I wonder why code from (1) is gone from B even though I felt
certain it had been added before. I also remember that B was renamed at
some point. So I do git log -p --follow B, and it nicely shows that diff
where the code was added, but no diff where the code is removed.

The reason is of course, that the code was removed in the merge and that
diff is not shown. And -c is usually what I do to enable showing diffs
in merge commits.

And if the pairwise diff can also deal with file renames, I think it
absolutely does make sense to show also a three-way diff.

I can't tell far away the code is from supporting anything like that.

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

[PATCH] fix segfault with git log -c --follow

[Clemens Buchacher]

In diff_tree_combined we make a copy of diffopts. In
try_to_follow_renames, called via diff_tree_sha1, we free and
re-initialize diffopts-pathspec-items. Since we did not make a deep
copy of diffopts in diff_tree_combined, the original diffopts does not
get the update. By the time we return from diff_tree_combined,
rev-diffopt-pathspec-items points to an invalid memory address. We
get a segfault next time we try to access that pathspec.

Instead, along with the copy of diffopts, make a copy pathspec-items as
well.

We would also have to make a copy of pathspec-raw to keep it consistent
with pathspec-items, but nobody seems to rely on that.

Signed-off-by: Clemens Buchacher dri...@aon.at
---

I wonder why I get a segfault from this so reliably, since it's not
actually a null-pointer dereference. Maybe this is gcc 4.8 doing
something different from previous versions?

Also, I have absolutely no confidence in my understanding of this code.
This is the first solution that came to mind, and could be totally
wrong. I just figured a patch is better than no patch.

Clemens

 combine-diff.c |  3 +++
 t/t4202-log.sh | 14 ++
 2 files changed, 17 insertions(+)

diff --git a/combine-diff.c b/combine-diff.c
index 77d7872..8825604 100644
--- a/combine-diff.c
+++ b/combine-diff.c
@@ -1302,6 +1302,7 @@ void diff_tree_combined(const unsigned char *sha1,
int i, num_paths, needsep, show_log_first, num_parent = parents-nr;
 
diffopts = *opt;
+   diff_tree_setup_paths(diffopts.pathspec.raw, diffopts);
diffopts.output_format = DIFF_FORMAT_NO_OUTPUT;
DIFF_OPT_SET(diffopts, RECURSIVE);
DIFF_OPT_CLR(diffopts, ALLOW_EXTERNAL);
@@ -1372,6 +1373,8 @@ void diff_tree_combined(const unsigned char *sha1,
paths = paths-next;
free(tmp);
}
+
+   diff_tree_release_paths(diffopts);
 }
 
 void diff_tree_combined_merge(const struct commit *commit, int dense,
diff --git a/t/t4202-log.sh b/t/t4202-log.sh
index 9243a97..cb03d28 100755
--- a/t/t4202-log.sh
+++ b/t/t4202-log.sh
@@ -530,6 +530,20 @@ test_expect_success 'show added path under --follow -M' '
)
 '
 
+test_expect_success 'git log -c --follow' '
+   test_create_repo follow-c 
+   (
+   cd follow-c 
+   test_commit initial file original 
+   git rm file 
+   test_commit rename file2 original 
+   git reset --hard initial 
+   test_commit modify file foo 
+   git merge -m merge rename 
+   git log -c --follow file2
+   )
+'
+
 cat expect \EOF
 *   commit COMMIT_OBJECT_NAME
 |\  Merge: MERGE_PARENTS
-- 
1.8.2.3

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