Re: [PATCH 00/18] Signed push

2014-08-20 Thread Nico Williams
No code == no substance might be a stretch, but definitely fair enough.

I thought the idea was clear enough, but I can flesh it out if
desired.  The particular advantage I saw in it is that it would reuse
the existing object infrastructure, and extend to branches the
first-class treatment that [signed] tags already get.  I.e.,
generality.  Other benefits include the ability to fetch and view a
remote branch's object and its history (which would represent branch
history in detail and with metadata that would otherwise not be
available).

I'm not interested in pushing something different when you already
have code that would achieve much of what I'd wanted.  At this point
my interest is in seeing if the architecture would be purer (in some
sense) by reusing existing infrastructure.

Nico
--
--
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 15/18] the beginning of the signed push

2014-08-20 Thread Bert Wesarg
On Wed, Aug 20, 2014 at 12:06 AM, Junio C Hamano gits...@pobox.com wrote:
 The basic flow based on this mechanism goes like this:

  1. You push out your work with git push -s.

  2. The sending side learns where the remote refs are as usual,
 together with what protocol extension the receiving end
 supports.  If the receiving end does not advertise the protocol
 extension push-cert, the sending side falls back to the normal
 push that is not signed.


Is this fallback silently? If so I think it would be better to abort
the push, if the receiver does not support this but the user requested
it.

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


Re: [PATCH v3] Allow the user to change the temporary file name for mergetool

2014-08-20 Thread Stefan Näwe
Am 19.08.2014 um 19:15 schrieb Robin Rosenberg:
 Using the original filename suffix for the temporary input files to
 the merge tool confuses IDEs like Eclipse. This patch introduces
 a configurtion option, mergetool.tmpsuffix, which get appended to
 the temporary file name. That way the user can choose to use a
 suffix like .tmp, which does not cause confusion.
 
 Signed-off-by: Robin Rosenberg robin.rosenb...@dewire.com
 ---
  Documentation/config.txt|  5 +
  Documentation/git-mergetool.txt |  7 +++
  git-mergetool.sh| 10 ++
  3 files changed, 18 insertions(+), 4 deletions(-)
 
 diff --git a/Documentation/config.txt b/Documentation/config.txt
 index c55c22a..0e15800 100644
 --- a/Documentation/config.txt
 +++ b/Documentation/config.txt
 @@ -1778,6 +1778,11 @@ notes.displayRef::
   several times.  A warning will be issued for refs that do not
   exist, but a glob that does not match any refs is silently
   ignored.
 +
 +mergetool.tmpsuffix::
 + A string to append the names of the temporary files mergetool
 + creates in the worktree as input to a custom merge tool. The
 + primary use is to avoid confusion in IDEs during merge.
  +
  This setting can be overridden with the `GIT_NOTES_DISPLAY_REF`
  environment variable, which must be a colon separated list of refs or
 diff --git a/Documentation/git-mergetool.txt b/Documentation/git-mergetool.txt
 index e846c2e..80a0526 100644
 --- a/Documentation/git-mergetool.txt
 +++ b/Documentation/git-mergetool.txt
 @@ -89,6 +89,13 @@ Setting the `mergetool.keepBackup` configuration variable 
 to `false`
  causes `git mergetool` to automatically remove the backup as files
  are successfully merged.
  
 +`git mergetool` may also create other temporary files for the
 +different versions involved in the merge. By default these files have
 +the same filename suffix as the file being merged. This may confuse
 +other tools in use during a long merge operation. The user can set
 +`mergetool.tmpsuffix` to be used as an extra suffix, which will be
 +appened to the temporary filename to lessen that problem.

Still:

s/appened/appended/

 +
  GIT
  ---
  Part of the linkgit:git[1] suite
 diff --git a/git-mergetool.sh b/git-mergetool.sh
 index 9a046b7..d7cc76c 100755
 --- a/git-mergetool.sh
 +++ b/git-mergetool.sh
 @@ -214,6 +214,8 @@ checkout_staged_file () {
  }
  
  merge_file () {
 + tmpsuffix=$(git config mergetool.tmpsuffix || true)
 +
   MERGED=$1
  
   f=$(git ls-files -u -- $MERGED)
 @@ -229,10 +231,10 @@ merge_file () {
   fi
  
   ext=$$$(expr $MERGED : '.*\(\.[^/]*\)$')
 - BACKUP=./$MERGED.BACKUP.$ext
 - LOCAL=./$MERGED.LOCAL.$ext
 - REMOTE=./$MERGED.REMOTE.$ext
 - BASE=./$MERGED.BASE.$ext
 + BACKUP=./$MERGED.BACKUP.$ext$tmpsuffix
 + LOCAL=./$MERGED.LOCAL.$ext$tmpsuffix
 + REMOTE=./$MERGED.REMOTE.$ext$tmpsuffix
 + BASE=./$MERGED.BASE.$ext$tmpsuffix
  
   base_mode=$(git ls-files -u -- $MERGED | awk '{if ($3==1) print $1;}')
   local_mode=$(git ls-files -u -- $MERGED | awk '{if ($3==2) print 
 $1;}')
 

Stefan
-- 

/dev/random says: It's Ensign Flintstone, Jim... He's Fred!
python -c print 
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')
 
GPG Key fingerprint = 2DF5 E01B 09C3 7501 BCA9  9666 829B 49C5 9221 27AF
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3] Allow the user to change the temporary file name for mergetool

2014-08-20 Thread Robin Rosenberg


- Ursprungligt meddelande -
 Från: Junio C Hamano gits...@pobox.com
 Till: Johannes Sixt j...@kdbg.org
 Kopia: Robin Rosenberg robin.rosenb...@dewire.com, git@vger.kernel.org
 Skickat: onsdag, 20 aug 2014 0:14:21
 Ämne: Re: [PATCH v3] Allow the user to change the temporary file name for 
 mergetool
 
 Johannes Sixt j...@kdbg.org writes:
 
  Am 19.08.2014 19:15, schrieb Robin Rosenberg:
  Using the original filename suffix for the temporary input files to
  the merge tool confuses IDEs like Eclipse. This patch introduces
  a configurtion option, mergetool.tmpsuffix, which get appended to
  the temporary file name. That way the user can choose to use a
  suffix like .tmp, which does not cause confusion.
 
  I have a merge tool that does syntax highlighting based on the file
  extension. Given this:
 
  +  BACKUP=./$MERGED.BACKUP.$ext$tmpsuffix
  +  LOCAL=./$MERGED.LOCAL.$ext$tmpsuffix
  +  REMOTE=./$MERGED.REMOTE.$ext$tmpsuffix
  +  BASE=./$MERGED.BASE.$ext$tmpsuffix
 
  I guess I lose syntax highlighting if I were to use mergetool.tmpsuffix;
  but then I don't use Eclipse. Could it be that this is really just a
  band-aid for Eclipse users, not IDEs in general as you are hinting in
  the Documentation of the new variable?
 
 The phrase IDEs like Eclipse in the proposed log message did not
 tell me (which I think is a good thing) if IDEs that need band-aid
 are majority or minority, but I agree that we should not hint that
 IDEs in general would benefit by setting this variable.  A warning
 on the syntax-aware editors may be necessary.

I'm not sure it's necessary since it is not a default. If you use the
setting you are probably well aware of why you use it, and the 
possible implications.

I have only had the problem with Eclipse, but I imagine any tool that
owns a directory and scans it for changes will find these temporary
files and do something unexpected based on the suffix. By setting the
suffix to something insert, like txt, tmp, dat or whatever you prevent
that tool from thinking too much.

In concrete terms, what happens is that Eclipse, in my case, find
temporary filenames with the suffix Foo.REMOTE.java and thinks that
is the one source file for Foo since it contains the source for Foo.

Sure you lose syntax highlighting, that's a trade-off. An alternative
solution would be to put these files somewhere else.

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


[BUG] rewriting history with filter-branch --commit-filter

2014-08-20 Thread Davide Fiorentino
Hi,

I was in the need to rewrite author name and email and commit date for a single 
commit and I guess I found a bug.
I run this git-filter script

$ git filter-branch --commit-filter ‘
if [ $GIT_COMMIT=9cfca27 ]; 
then GIT_AUTHOR_NAME=“Michelle”; 
GIT_AUTHOR_EMAIL=“miche...@email.com”; 
GIT_AUTHOR_DATE=2009-12-31T23:59:59”; 
git commit-tree $@“; 
else 
git commit-tree $@“;
fi' HEAD

and found that all history was rewritten as if “Michelle” not only commit 
9cfca27.

I also tried using full length commit id and a non-existing commit id: nothing 
changes.
In the following example to replicate the replicate the bug I’m using a 
non-existing commit id to make the effect more evident.


$ git --version
git version 2.0.1

$ mkdir project

$ cd project

$ git init

$ for i in 1 2 3; do echo foo  foo$i.txt; git add foo$i.txt; git commit -m 
foo$i; done
[master (root-commit) 89dec6e] foo1
 1 file changed, 1 insertion(+)
 create mode 100644 foo1.txt
[master 8a3c8e5] foo2
 1 file changed, 1 insertion(+)
 create mode 100644 foo2.txt
[master a3ca061] foo3
 1 file changed, 1 insertion(+)
 create mode 100644 foo3.txt

$ git log --graph --all --pretty=format:'%C(yellow)%h%C(cyan)%d%Creset %s 
%C(white)- %an, %ar%Creset'
* a3ca061 (HEAD, master) foo3 - David, 4 seconds ago
* 8a3c8e5 foo2 - David, 4 seconds ago
* 89dec6e foo1 - David, 4 seconds ago

$ git filter-branch --commit-filter '
if [ $GIT_COMMIT=308add7 ]; 
then GIT_AUTHOR_NAME=Michelle; 
GIT_AUTHOR_EMAIL=miche...@email.com; 
GIT_AUTHOR_DATE=2009-12-31T23:59:59; 
git commit-tree $@; 
else 
git commit-tree $@;
fi' HEAD

$ git log --graph --all --pretty=format:'%C(yellow)%h%C(cyan)%d%Creset %s 
%C(white)- %an, %ar%Creset'
* 8937dff (HEAD, master) foo3 - Michelle, 4 years, 8 months ago
* 30e494e foo2 - Michelle, 4 years, 8 months ago
* 2a2ba4f foo1 - Michelle, 4 years, 8 months ago
* a3ca061 (refs/original/refs/heads/master) foo3 - David, 8 seconds ago
* 8a3c8e5 foo2 - David, 8 seconds ago
* 89dec6e foo1 - David, 8 seconds ago

$ git log
commit 8937dff7e6a3f5545c2242e3fd5d33acbabe6df4
Author: Michelle miche...@email.com
Date:   Thu Dec 31 23:59:59 2009 +0100

foo3

commit 30e494ef27f16c5456e66214ea46b780581dfb48
Author: Michelle miche...@email.com
Date:   Thu Dec 31 23:59:59 2009 +0100

foo2

commit 2a2ba4fd6b9627e237a12b47570a3f020a202b55
Author: Michelle miche...@email.com
Date:   Thu Dec 31 23:59:59 2009 +0100

foo1


using env-filter, I managed to rewrite the history with this:

$ git filter-branch --env-filter '
 
name=$GIT_AUTHOR_NAME
email=$GIT_AUTHOR_EMAIL
 
if [ $GIT_COMMIT = 89dec6e4bc1fb3cff694ea83f5ed900dad43449e ]
then
name=Michelle
email=miche...@email.com
fi

export GIT_AUTHOR_NAME=“$name
export GIT_AUTHOR_EMAIL=“$email
'

Hope this helped.

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


Re: [BUG] rewriting history with filter-branch --commit-filter

2014-08-20 Thread Jeff King
On Wed, Aug 20, 2014 at 10:16:11AM +0200, Davide Fiorentino wrote:

 I was in the need to rewrite author name and email and commit date for a 
 single commit and I guess I found a bug.
 I run this git-filter script
 
 $ git filter-branch --commit-filter ‘
 if [ $GIT_COMMIT=9cfca27 ]; 
 then GIT_AUTHOR_NAME=“Michelle”; 
 GIT_AUTHOR_EMAIL=“miche...@email.com”; 
 GIT_AUTHOR_DATE=2009-12-31T23:59:59”; 
 git commit-tree $@“; 
 else 
 git commit-tree $@“;
 fi' HEAD
 
 and found that all history was rewritten as if “Michelle” not only commit 
 9cfca27.

The filter snippets you provide to filter-branch are shell script. The
`[` command (aka `test`) is just another shell command, and follows the
usual whitespace splitting rules. In your command:

  [ $GIT_COMMIT=9cfca27 ]

it sees only one single argument, all concatenated together. A single
argument given to `test` is the same as `test -n`: it tells you whether
the string is empty, so this conditional is always true. You wanted:

  [ $GIT_COMMIT = 9cfca27 ]

instead. The whitespace makes the = a separate argument, and the
command knows it's an operator. You should also use the full commit id,
as that is what will be in $GIT_COMMIT.

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


Re: Issuing warning when hook does not have execution permission

2014-08-20 Thread Chris Packham
On Wed, Aug 20, 2014 at 4:52 AM, Junio C Hamano gits...@pobox.com wrote:
 Jeff King p...@peff.net writes:

 On Tue, Aug 19, 2014 at 04:05:21PM +1000, Babak M wrote:

 I saw that if a hook file is present in .git/hooks and it does not
 have execution permissions it is silently ignored.

 I thought it might be worthwhile issuing a warning such as Warning:
 pre-commit hook exists but it cannot be executed due to insufficient
 permissions.

 Not sure if this has been discussed before. I searched the archive but
 didn't see anything.

 Thoughts, suggestions? Is there anything like that already?

 Once upon a time we shipped sample hooks with their execute bits turned
 off, and such a warning would have been very bad.

 These days we give them a .sample extension (because Windows installs
 had trouble with the execute bit :) ), so I think it should be OK in
 theory. Installing a new version of git on top of an old one with make
 install does not clean up old files. So somebody who has continuously
 upgraded their git via make install to the same directory would have
 the old-style sample files. Under your proposal, they would get a lot of
 warnings.

 However, that change came in v1.6.0, just over 6 years ago. We can
 probably discount that (and if it does happen, maybe it is time for that
 someone to clean up the other leftover cruft from past git installs).

 The above all sounds very sensible.

 We have another code path that looks for an executable, finds one
 with no execute permission and decides not to execute it, and I
 wonder if we should use the same criteria to decide to give (or not
 give) a warning as the one used in the other code path (i.e. looking
 up git-foo executable when the user runs git foo).
 --

I actually find the existing behaviour useful. If I want to disable a
hook to I can just chmod -x .git/hook/... and I then chmod +x it when
I want to re-enable it. I guess I could live with an extra warning as
long as the command still succeeds.
--
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


Anyone know the innards of the gitk program?

2014-08-20 Thread John M. Dlugosz
I knew that gitk is written in a scripting language, so I had high hopes of making a 
little tweak to it.  But, alas, it's 12000 lines of languages I don't know!


The farthest I got was finding “text $cflist \” on line 2381 and the corroborating comment 
“# lower right” just above it.


What I want to accomplish is to highlight specific words in the Patch view; in particular 
certain path element names to indicate files I'm specifically interested in reviewing and 
those that I'm specifically not at all interested in.


Considering the way it works — Tk widgets — that ought to be easy.  I've used Perl/Tk in 
the past and can look up the parameters to different element types.  But I have no idea 
where to put such a change, or the general flow of the program in general.


Is anyone knowledgeable in the inner workings of this program willing to help 
me out?

I've tried a few git repository GUI programs (on Windows) and always end up gravitating 
back to Git Gui and gitk !


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


Re: [BUG] rewriting history with filter-branch --commit-filter

2014-08-20 Thread Davide Fiorentino
Damn! I haven’t seen that missing space.

Thank you Peff!

Davide--
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: Issuing warning when hook does not have execution permission

2014-08-20 Thread Jeff King
On Wed, Aug 20, 2014 at 08:55:52PM +1200, Chris Packham wrote:

 I actually find the existing behaviour useful. If I want to disable a
 hook to I can just chmod -x .git/hook/... and I then chmod +x it when
 I want to re-enable it. I guess I could live with an extra warning as
 long as the command still succeeds.

You could do the same thing mv $hook $hook.disabled but it involves
retraining your fingers. I kind of agree that the existing system of
respecting the executable bit is nice, though: it does what you told it
to do, and a misconfiguration is your problem, not the system's. It's
perhaps worth changing if people frequently get the executable-bit thing
wrong, but I don't know whether they do or not.

I kind of feel like we had a similar discussion around items in PATH,
but I don't remember how it resolved.

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


[PATCH 3/4] Added tests for the case of merged and unmerged entries for the same file

2014-08-20 Thread Jaime Soriano Pastor
Signed-off-by: Jaime Soriano Pastor jsorianopas...@gmail.com
---
 t/t9904-unmerged-file-with-merged-entry.sh | 86 ++
 1 file changed, 86 insertions(+)
 create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh

diff --git a/t/t9904-unmerged-file-with-merged-entry.sh 
b/t/t9904-unmerged-file-with-merged-entry.sh
new file mode 100755
index 000..945bc1c
--- /dev/null
+++ b/t/t9904-unmerged-file-with-merged-entry.sh
@@ -0,0 +1,86 @@
+#!/bin/sh
+
+test_description='Operations with unmerged files with merged entries'
+
+. ./test-lib.sh
+
+setup_repository() {
+   test_commit A conflict A
+   test_commit A conflict2 A2 branchbase
+   test_commit B conflict B
+   test_commit B conflict2 B2
+   git checkout branchbase -b branch1
+   test_commit C conflict C
+   test_commit C conflict2 C2
+   test_commit something otherfile otherfile
+}
+
+setup_stage_state() {
+   git checkout -f HEAD
+   {
+   git ls-files -s conflict conflict2
+   git merge master  /dev/null
+   git ls-files -s conflict conflict2
+   }  index
+   cat index | git update-index --index-info
+   rm index
+}
+
+test_expect_success 'setup - two branches with conflicting files' '
+   setup_repository 
+   setup_stage_state 
+   git ls-files -s conflict  output 
+   test_line_count = 4 output 
+   git ls-files -s conflict2  output 
+   test_line_count = 4 output 
+   rm output
+'
+
+test_expect_success 'git commit -a' '
+   setup_stage_state 
+   test_must_fail git commit -a
+'
+
+test_expect_success 'git add conflict' '
+   setup_stage_state 
+   test_must_fail git add conflict
+'
+
+test_expect_success 'git rm conflict' '
+   setup_stage_state 
+   test_must_fail git rm conflict
+'
+
+test_expect_success 'git add otherfile' '
+   setup_stage_state 
+   otherfile 
+   git add otherfile
+'
+
+test_expect_success 'git rm otherfile' '
+   setup_stage_state 
+   git rm otherfile
+'
+
+test_expect_success 'git add newfile' '
+   setup_stage_state 
+   newfile 
+   git add newfile
+'
+
+test_expect_success 'git merge branch' '
+   setup_stage_state 
+   test_must_fail git merge master
+'
+
+test_expect_success 'git reset --hard' '
+   setup_stage_state 
+   git reset --hard 
+   git show HEAD:conflict  expected 
+   cat conflict  current 
+   git show HEAD:conflict2  expected 
+   cat conflict2  current 
+   test_cmp expected current
+'
+
+test_done
-- 
2.0.4.4.gaf54b2b

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


[PATCH 0/4] Handling unmerged files with merged entries

2014-08-20 Thread Jaime Soriano Pastor
New approach for the case of finding unmerged files with merged entries
in the index.
After some discussion the solution tries to:
- Avoid the problems with infinite loops in this case.
- Provide better information to the user in the commands affected.
- Make sure there are ways to clean the index.
- Provide also a way to specifically recover each one of the files with
  this problem.

With these patches the behaviour of these commands (for this case) change:
- git reset is able to finish, cleaning the index, but warning out the
  information about the removed stages.
- git merge is able to finish, reporting that there is a merge in progress as
  usual, it also warns about the unmerged files with merged entries.
- git add fails when this case happens, telling the user to check the state
  with 'git ls-files -s', before, it did nothing. The same with git commit -a.
- git update-index --cacheinfo can be used to select an specific staged
  version to resolve the conflict, without the need of reseting the working
  copy. It did nothing before.

Tests added for these cases. Rest of the tests remain unchanged and pass too.

Jaime Soriano Pastor (4):
  read_index_unmerged doesn't loop forever if merged stage exists for
unmerged file
  Error out when adding a file with merged and unmerged entries
  Added tests for the case of merged and unmerged entries for the same
file
  git update-index --cacheinfo can be used to select a stage when there
are merged and unmerged entries

 builtin/update-index.c |  1 +
 cache.h|  1 +
 read-cache.c   | 36 ++--
 t/t9904-unmerged-file-with-merged-entry.sh | 94 ++
 4 files changed, 127 insertions(+), 5 deletions(-)
 create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh

-- 
2.0.4.4.gaf54b2b

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


[PATCH 1/4] read_index_unmerged doesn't loop forever if merged stage exists for unmerged file

2014-08-20 Thread Jaime Soriano Pastor
Signed-off-by: Jaime Soriano Pastor jsorianopas...@gmail.com
---
 read-cache.c | 21 +++--
 1 file changed, 19 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 7f5645e..c932b83 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1933,6 +1933,7 @@ int read_index_unmerged(struct index_state *istate)
 {
int i;
int unmerged = 0;
+   struct cache_entry *merged_ce = NULL;
 
read_index(istate);
for (i = 0; i  istate-cache_nr; i++) {
@@ -1940,9 +1941,26 @@ int read_index_unmerged(struct index_state *istate)
struct cache_entry *new_ce;
int size, len;
 
-   if (!ce_stage(ce))
+   if (!ce_stage(ce)) {
+   merged_ce = ce;
continue;
+   }
unmerged = 1;
+   if (merged_ce  ce_same_name(merged_ce, ce)) {
+   warning(Unexpected stages for merged file '%s':,
+   merged_ce-name);
+   i--;
+   while (i  istate-cache_nr 
+  ce_same_name(merged_ce, istate-cache[i])) {
+   ce = istate-cache[i++];
+   warning(%06o %s %d,
+   ce-ce_mode, sha1_to_hex(ce-sha1), 
ce_stage(ce));
+   }
+   i--;
+   merged_ce-ce_flags = create_ce_flags(0) | 
CE_CONFLICTED;
+   merged_ce = NULL;
+   continue;
+   }
len = ce_namelen(ce);
size = cache_entry_size(len);
new_ce = xcalloc(1, size);
@@ -1953,7 +1971,6 @@ int read_index_unmerged(struct index_state *istate)
if (add_index_entry(istate, new_ce, 0))
return error(%s: cannot drop to stage #0,
 new_ce-name);
-   i = index_name_pos(istate, new_ce-name, len);
}
return unmerged;
 }
-- 
2.0.4.4.gaf54b2b

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


[PATCH 2/4] Error out when adding a file with merged and unmerged entries

2014-08-20 Thread Jaime Soriano Pastor
Signed-off-by: Jaime Soriano Pastor jsorianopas...@gmail.com
---
 read-cache.c | 14 +++---
 1 file changed, 11 insertions(+), 3 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index c932b83..d549d0b 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -935,6 +935,7 @@ static int add_index_entry_with_check(struct index_state 
*istate, struct cache_e
int ok_to_replace = option  ADD_CACHE_OK_TO_REPLACE;
int skip_df_check = option  ADD_CACHE_SKIP_DFCHECK;
int new_only = option  ADD_CACHE_NEW_ONLY;
+   int replaced = 0;
 
cache_tree_invalidate_path(istate-cache_tree, ce-name);
pos = index_name_stage_pos(istate, ce-name, ce_namelen(ce), 
ce_stage(ce));
@@ -943,9 +944,10 @@ static int add_index_entry_with_check(struct index_state 
*istate, struct cache_e
if (pos = 0) {
if (!new_only)
replace_index_entry(istate, pos, ce);
-   return 0;
-   }
-   pos = -pos-1;
+   replaced = 1;
+   pos++;
+   } else
+   pos = -pos-1;
 
/*
 * Inserting a merged entry (stage 0) into the index
@@ -953,12 +955,18 @@ static int add_index_entry_with_check(struct index_state 
*istate, struct cache_e
 */
if (pos  istate-cache_nr  ce_stage(ce) == 0) {
while (ce_same_name(istate-cache[pos], ce)) {
+   if (replaced)
+   die(Merged and unmerged entries found for 
+   '%s', check 'git ls-files -s \%s\',
+   ce-name, ce-name);
ok_to_add = 1;
if (!remove_index_entry_at(istate, pos))
break;
}
}
 
+   if (replaced)
+   return 0;
if (!ok_to_add)
return -1;
if (!verify_path(ce-name))
-- 
2.0.4.4.gaf54b2b

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


[PATCH 4/4] git update-index --cacheinfo can be used to select a stage when there are merged and unmerged entries

2014-08-20 Thread Jaime Soriano Pastor
Signed-off-by: Jaime Soriano Pastor jsorianopas...@gmail.com
---
 builtin/update-index.c |  1 +
 cache.h|  1 +
 read-cache.c   |  3 ++-
 t/t9904-unmerged-file-with-merged-entry.sh | 14 +++---
 4 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index ebea285..509fae7 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -239,6 +239,7 @@ static int add_cacheinfo(unsigned int mode, const unsigned 
char *sha1,
ce-ce_flags |= CE_VALID;
option = allow_add ? ADD_CACHE_OK_TO_ADD : 0;
option |= allow_replace ? ADD_CACHE_OK_TO_REPLACE : 0;
+   option |= ADD_CACHE_OK_TO_REPLACE_MERGED;
if (add_cache_entry(ce, option))
return error(%s: cannot add to the index - missing --add 
option?,
 path);
diff --git a/cache.h b/cache.h
index c708062..ecac41c 100644
--- a/cache.h
+++ b/cache.h
@@ -474,6 +474,7 @@ extern int index_name_pos(const struct index_state *, const 
char *name, int name
 #define ADD_CACHE_SKIP_DFCHECK 4   /* Ok to skip DF conflict checks */
 #define ADD_CACHE_JUST_APPEND 8/* Append only; 
tree.c::read_tree() */
 #define ADD_CACHE_NEW_ONLY 16  /* Do not replace existing ones */
+#define ADD_CACHE_OK_TO_REPLACE_MERGED 32 /* Ok to replace even if a merged 
entry exists */
 extern int add_index_entry(struct index_state *, struct cache_entry *ce, int 
option);
 extern void rename_index_entry_at(struct index_state *, int pos, const char 
*new_name);
 extern int remove_index_entry_at(struct index_state *, int pos);
diff --git a/read-cache.c b/read-cache.c
index d549d0b..c4ddefe 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -933,6 +933,7 @@ static int add_index_entry_with_check(struct index_state 
*istate, struct cache_e
int pos;
int ok_to_add = option  ADD_CACHE_OK_TO_ADD;
int ok_to_replace = option  ADD_CACHE_OK_TO_REPLACE;
+   int ok_to_replace_merged = option  ADD_CACHE_OK_TO_REPLACE_MERGED;
int skip_df_check = option  ADD_CACHE_SKIP_DFCHECK;
int new_only = option  ADD_CACHE_NEW_ONLY;
int replaced = 0;
@@ -955,7 +956,7 @@ static int add_index_entry_with_check(struct index_state 
*istate, struct cache_e
 */
if (pos  istate-cache_nr  ce_stage(ce) == 0) {
while (ce_same_name(istate-cache[pos], ce)) {
-   if (replaced)
+   if (replaced  !ok_to_replace_merged)
die(Merged and unmerged entries found for 
'%s', check 'git ls-files -s \%s\',
ce-name, ce-name);
diff --git a/t/t9904-unmerged-file-with-merged-entry.sh 
b/t/t9904-unmerged-file-with-merged-entry.sh
index 945bc1c..9138821 100755
--- a/t/t9904-unmerged-file-with-merged-entry.sh
+++ b/t/t9904-unmerged-file-with-merged-entry.sh
@@ -28,11 +28,11 @@ setup_stage_state() {
 
 test_expect_success 'setup - two branches with conflicting files' '
setup_repository 
-   setup_stage_state 
+   git merge master
git ls-files -s conflict  output 
-   test_line_count = 4 output 
+   test_line_count = 3 output 
git ls-files -s conflict2  output 
-   test_line_count = 4 output 
+   test_line_count = 3 output 
rm output
 '
 
@@ -83,4 +83,12 @@ test_expect_success 'git reset --hard' '
test_cmp expected current
 '
 
+test_expect_success 'git update-index --cacheinfo to select a stage to use' '
+   setup_stage_state 
+   git cat-file blob :1:conflict  conflict 
+   git update-index --cacheinfo 100644,`git hash-object conflict`,conflict
+   git ls-files -s conflict  output 
+   test_line_count = 1 output
+'
+
 test_done
-- 
2.0.4.4.gaf54b2b

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


[PATCH 1/1] pretty: note that %cd respects the --date= option

2014-08-20 Thread Thomas Braun
Signed-off-by: Thomas Braun thomas.br...@virtuell-zuhause.de
---

Today I found out that both %cd and %ad pretty print format
specifications honour the --date option as shown in:

$ git log --abbrev=8 --date=short --pretty=%h (%s, %cd) -n1
5bdb1c4e (Merge pull request #245 from
kblees/kb/master/fix-libsvn-address-conflict, 2014-08-16)
$ git log --abbrev=8 --date=short --pretty=%h (%s, %ad) -n1
5bdb1c4e (Merge pull request #245 from
kblees/kb/master/fix-libsvn-address-conflict, 2014-08-16)

But the documentation did not mention that.

 Documentation/pretty-formats.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/pretty-formats.txt
b/Documentation/pretty-formats.txt
index 85d6353..eac7909 100644
--- a/Documentation/pretty-formats.txt
+++ b/Documentation/pretty-formats.txt
@@ -122,7 +122,7 @@ The placeholders are:
 - '%ce': committer email
 - '%cE': committer email (respecting .mailmap, see
   linkgit:git-shortlog[1] or linkgit:git-blame[1])
-- '%cd': committer date
+- '%cd': committer date (format respects --date= option)
 - '%cD': committer date, RFC2822 style
 - '%cr': committer date, relative
 - '%ct': committer date, UNIX timestamp
-- 
2.1.0.msysgit.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: Re: Re: Relative submodule URLs

2014-08-20 Thread Robert Dailey
On Tue, Aug 19, 2014 at 3:57 PM, Heiko Voigt hvo...@hvoigt.net wrote:
 I would actually error out when specified in already cloned state.
 Because otherwise the user might expect the remote to be updated.

 Since we are currently busy implementing recursive fetch and checkout I have
 added that to our ideas list[1] so we do not forget about it.

 In the meantime you can either use the branch.name.remote
 configuration to define a remote to use or just use 'origin'.

 Cheers Heiko

 [1] 
 https://github.com/jlehmann/git-submod-enhancements/wiki#add-with-remote--switch-to-submodule-update

Thanks Heiko.

I would offer to help implement this for you, if you find it to be a
good idea, but I've never done git development before and based on
what I've seen it seems like you need to know at least 2-3 languages
to contribute: bash, perl, C++. I know C++  Python but I don't know
perl or bash scripting language.

What would it take to help you guys out? It's easy to complain  file
bugs but as a developer I feel like I should offer more, if it suits
you.

Let me know I'm happy to help with anything. Thanks again!!
--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Michael Haggerty
On 07/15/2014 10:58 PM, Ronnie Sahlberg wrote:
 On Tue, Jul 15, 2014 at 12:34 PM, Ronnie Sahlberg sahlb...@google.com wrote:
 On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 Michael Haggerty wrote:

 So...I like the idea of enforcing refname checks at the lowest level
 possible, but I think that the change you propose is too abrupt.  I
 think it needs either more careful analysis showing that it won't hurt
 anybody, or some kind of tooling or non-strict mode that people can use
 to fix their repositories.

 The recovery code has been broken for a while, so I don't see harm
 in this change.

 How to take care of the recovery use case is another question.  FWIW I
 also would prefer if git update-ref -d or git branch -D could be
 used to delete corrupt refs instead of having to use fsck (since a
 fsck run can take a while), but that's a question for a later series.

 In an ideal world, the low-level functions would allow *reading* and
 *deleting* poorly named refs (even without any special flag) but not
 creating them.  Is that doable?

 That should be doable. Ill add these repairs as 3-4 new patches at the
 end of the current patch series.

 The main complication I can see is
 iteration: would iteration skip poorly named refs and warn, or would
 something more complicated be needed?

I think we can get away with not including broken refnames when
iterating.  After all, the main goal of tolerating them is to let them
be deleted, right?  Then as long as iteration is not needed to implement
the command git update-ref -d, it seems to me that it is OK if
iterating over a reference with a broken name causes a die().

 Right now,  git branch  will error and abort right away when it
 finds the first bad ref. I haven't checked the exact spot it does this
 yet but I suspect it is when building the ref-cache.
 I will solve the cases for create and delete for now.


 What/how to handle iterables will require more thought.
 Right now, since these refs will be filtered out and never end up in
 ref-cache, either from loose refs or from packed refs
 it does mean that anyone that uses an iterator is guaranteed to only
 get refs with valid names passed back to them.
 We would need to audit all code that uses iterators and make sure it
 can handle the case where the callback is suddenly
 invoked with a bad refname.


 Thanks,
 Jonathan
 
 The following seems to do the trick to allow deleting a bad ref. We
 would need something for the iterator too.
 Since this touches the same areas that my ~100 other ref transaction
 patches that are queued up do, I
 would like to wait applying this patch until once the next few series
 are finished and merged.
 (to avoid having to do a lot of rebases and fix legio of merge
 conflicts that this would introduce in my branches).
 
 
 Treat this as an approximation on what the fix to repair git branch -D
 will look like once the time comes.

I'm a little worried that abandoning *all* refname checks could open us
up to somehow trying to delete a reference with a name like
../../../../etc/passwd.  Either such names have to be prohibited
somehow, or we have to be very sure that they can only come from trusted
sources.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Ronnie Sahlberg
On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu wrote:
 On 07/15/2014 10:58 PM, Ronnie Sahlberg wrote:
 On Tue, Jul 15, 2014 at 12:34 PM, Ronnie Sahlberg sahlb...@google.com 
 wrote:
 On Tue, Jul 15, 2014 at 11:04 AM, Jonathan Nieder jrnie...@gmail.com 
 wrote:
 Michael Haggerty wrote:

 So...I like the idea of enforcing refname checks at the lowest level
 possible, but I think that the change you propose is too abrupt.  I
 think it needs either more careful analysis showing that it won't hurt
 anybody, or some kind of tooling or non-strict mode that people can use
 to fix their repositories.

 The recovery code has been broken for a while, so I don't see harm
 in this change.

 How to take care of the recovery use case is another question.  FWIW I
 also would prefer if git update-ref -d or git branch -D could be
 used to delete corrupt refs instead of having to use fsck (since a
 fsck run can take a while), but that's a question for a later series.

 In an ideal world, the low-level functions would allow *reading* and
 *deleting* poorly named refs (even without any special flag) but not
 creating them.  Is that doable?

 That should be doable. Ill add these repairs as 3-4 new patches at the
 end of the current patch series.

 The main complication I can see is
 iteration: would iteration skip poorly named refs and warn, or would
 something more complicated be needed?

 I think we can get away with not including broken refnames when
 iterating.  After all, the main goal of tolerating them is to let them
 be deleted, right?  Then as long as iteration is not needed to implement
 the command git update-ref -d, it seems to me that it is OK if
 iterating over a reference with a broken name causes a die().

 Right now,  git branch  will error and abort right away when it
 finds the first bad ref. I haven't checked the exact spot it does this
 yet but I suspect it is when building the ref-cache.
 I will solve the cases for create and delete for now.


 What/how to handle iterables will require more thought.
 Right now, since these refs will be filtered out and never end up in
 ref-cache, either from loose refs or from packed refs
 it does mean that anyone that uses an iterator is guaranteed to only
 get refs with valid names passed back to them.
 We would need to audit all code that uses iterators and make sure it
 can handle the case where the callback is suddenly
 invoked with a bad refname.


 Thanks,
 Jonathan

 The following seems to do the trick to allow deleting a bad ref. We
 would need something for the iterator too.
 Since this touches the same areas that my ~100 other ref transaction
 patches that are queued up do, I
 would like to wait applying this patch until once the next few series
 are finished and merged.
 (to avoid having to do a lot of rebases and fix legio of merge
 conflicts that this would introduce in my branches).


 Treat this as an approximation on what the fix to repair git branch -D
 will look like once the time comes.

 I'm a little worried that abandoning *all* refname checks could open us
 up to somehow trying to delete a reference with a name like
 ../../../../etc/passwd.  Either such names have to be prohibited
 somehow, or we have to be very sure that they can only come from trusted
 sources.


I only set this flag from builtin/branch.c so it should only be used
when a user runs 'git branch -D' from the command line.
All other places where we delete branches we should still be checking
the rename for badness.

That said, unless the rules for good refname changes in the future,
which is unlikely, is should be exceptionally rare that a user ends up
with a bad refname in the first place.
Perhaps my example I gave was bad since if you manually create bad
refs using echo  .git/refs/heads/...  then you should probably know
how to fix it too and thus maybe we do not need this patch in the
first place.

Do you want me to delete this patch and resend this part of the series
? Or is the 'only works for branch -D from the commandline' sufficient
?
I have no strong feelings either way so I will just follow what you decide.


regards
ronnie sahlberg
--
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 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread David Turner
On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
 Reusing the GPG signature check helpers we already have, verify
 the signature in receive-pack and give the results to the hooks
 via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
 
 Policy decisions, such as accepting or rejecting a good signature by
 a key that is not fully trusted, is left to the hook and kept
 outside of the core.

If I understand correctly, the hook does not have enough information to
make this decision, because it is missing the date from the signature.
This might allow an old signed push to be replayed, moving the head of a
branch to an older state (say, one lacking the latest security updates).
I have not proven this, and it is entirely possible that I am wrong, but
I think it would be worth either documenting why this is not possible,
or fixing it if it is possible.

--
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 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread Junio C Hamano
On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com wrote:
 On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
 Reusing the GPG signature check helpers we already have, verify
 the signature in receive-pack and give the results to the hooks
 via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.

 Policy decisions, such as accepting or rejecting a good signature by
 a key that is not fully trusted, is left to the hook and kept
 outside of the core.

 If I understand correctly, the hook does not have enough information to
 make this decision, because it is missing the date from the signature.

The full certificate is available to the hook so anything we can do the hook
has enough information to do ;-)  But of course we should try to make it
easier for the hook to validate the request.

I am not opposed to extract the timestamp from pushed-by header in the cert
and export it in another environment before calling the hook, but I am not sure
it is worth it, as that is already a single liner text information.

 This might allow an old signed push to be replayed, moving the head of a
 branch to an older state (say, one lacking the latest security updates).

... with old-sha1 recorded in the certificate?
--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Jonathan Nieder
Hi,

Ronnie Sahlberg wrote:
 On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu 
 wrote:

 I'm a little worried that abandoning *all* refname checks could open us
 up to somehow trying to delete a reference with a name like
 ../../../../etc/passwd.  Either such names have to be prohibited
 somehow, or we have to be very sure that they can only come from trusted
 sources.

 I only set this flag from builtin/branch.c so it should only be used
 when a user runs 'git branch -D' from the command line.
 All other places where we delete branches we should still be checking
 the rename for badness.

Right, this should be safe for 'git branch -D' and 'git update-ref -d'.

But if we wanted to open it up in the future for 'git push --delete',
too, then it would be a way to break out of the repository on hosts where
people use git-shell instead of relying on filesystem permissions.  And
that wouldn't be good.

I think elsewhere git has some checks for does this pathname fall in
this directory.  Could that be reused here, too, to make sure the
resolved path is under the resolved $GIT_DIR/refs directory?

Alternatively, when a ref being deleted doesn't meet the
'check-ref-format' checks, would it make sense to check that it is one
of the refs you can get by iteration?

Jonathan
--
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: Issuing warning when hook does not have execution permission

2014-08-20 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 On Wed, Aug 20, 2014 at 08:55:52PM +1200, Chris Packham wrote:

 I actually find the existing behaviour useful. If I want to disable a
 hook to I can just chmod -x .git/hook/... and I then chmod +x it when
 I want to re-enable it. I guess I could live with an extra warning as
 long as the command still succeeds.

 You could do the same thing mv $hook $hook.disabled but it involves
 retraining your fingers. I kind of agree that the existing system of
 respecting the executable bit is nice, though: it does what you told it
 to do, and a misconfiguration is your problem, not the system's. It's
 perhaps worth changing if people frequently get the executable-bit thing
 wrong, but I don't know whether they do or not.

 I kind of feel like we had a similar discussion around items in PATH,
 but I don't remember how it resolved.

I don't either, but IIRC the primary tricky point was what happens
when a component of $PATH list is inaccessible, making us unable to
even know if an executable we are looking for exists there or not,
which is slightly a different issue.

And I also kind of agree that the existing system is nice.  It may
sound like a good idea to warn when there is even a slight chance of
misconfiguration on the user's side, but for this particular one, it
has been a designed-in behaviour for a long time, and it may be
unwise to change it.

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


Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Ronnie Sahlberg
On Wed, Aug 20, 2014 at 10:49 AM, Jonathan Nieder jrnie...@gmail.com wrote:
 Hi,

 Ronnie Sahlberg wrote:
 On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu 
 wrote:

 I'm a little worried that abandoning *all* refname checks could open us
 up to somehow trying to delete a reference with a name like
 ../../../../etc/passwd.  Either such names have to be prohibited
 somehow, or we have to be very sure that they can only come from trusted
 sources.

 I only set this flag from builtin/branch.c so it should only be used
 when a user runs 'git branch -D' from the command line.
 All other places where we delete branches we should still be checking
 the rename for badness.

 Right, this should be safe for 'git branch -D' and 'git update-ref -d'.

 But if we wanted to open it up in the future for 'git push --delete',
 too, then it would be a way to break out of the repository on hosts where
 people use git-shell instead of relying on filesystem permissions.  And
 that wouldn't be good.

 I think elsewhere git has some checks for does this pathname fall in
 this directory.  Could that be reused here, too, to make sure the
 resolved path is under the resolved $GIT_DIR/refs directory?

 Alternatively, when a ref being deleted doesn't meet the
 'check-ref-format' checks, would it make sense to check that it is one
 of the refs you can get by iteration?

Good idea! I will add such a check using the iterator.
That means that we can git branch -D anything that shows up in the
iterator regardless if the ref is badly named or unresolvable which
sounds like fairly sane semantics.

Thanks!

Ronnie
--
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 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread David Turner
On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
 On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com 
 wrote:
  On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
  Reusing the GPG signature check helpers we already have, verify
  the signature in receive-pack and give the results to the hooks
  via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
 
  Policy decisions, such as accepting or rejecting a good signature by
  a key that is not fully trusted, is left to the hook and kept
  outside of the core.
 
  If I understand correctly, the hook does not have enough information to
  make this decision, because it is missing the date from the signature.
 
 The full certificate is available to the hook so anything we can do the hook
 has enough information to do ;-)  But of course we should try to make it
 easier for the hook to validate the request.

Excellent, then motivated hooks can do the right thing.

  This might allow an old signed push to be replayed, moving the head of a
  branch to an older state (say, one lacking the latest security updates).
 
 ... with old-sha1 recorded in the certificate?

That does prevent most replays, but it does not prevent resurrection of
a deleted branch by a replay of its initial creation (nor an undo of a
force-push to rollback).  So I think we still need timestamps, but
parsing them out of the cert is not terrible.

--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Michael Haggerty
On 08/20/2014 06:28 PM, Ronnie Sahlberg wrote:
 On Wed, Aug 20, 2014 at 7:52 AM, Michael Haggerty mhag...@alum.mit.edu 
 wrote:
 I'm a little worried that abandoning *all* refname checks could open us
 up to somehow trying to delete a reference with a name like
 ../../../../etc/passwd.  Either such names have to be prohibited
 somehow, or we have to be very sure that they can only come from trusted
 sources.
 
 I only set this flag from builtin/branch.c so it should only be used
 when a user runs 'git branch -D' from the command line.
 All other places where we delete branches we should still be checking
 the rename for badness.
 
 That said, unless the rules for good refname changes in the future,
 which is unlikely, is should be exceptionally rare that a user ends up
 with a bad refname in the first place.
 Perhaps my example I gave was bad since if you manually create bad
 refs using echo  .git/refs/heads/...  then you should probably know
 how to fix it too and thus maybe we do not need this patch in the
 first place.
 
 Do you want me to delete this patch and resend this part of the series
 ? Or is the 'only works for branch -D from the commandline' sufficient
 ?
 I have no strong feelings either way so I will just follow what you decide.

I think that if you run the refname through normalize_path_copy_len()
and that function returns (1) without an error, (2) without modifying
its argument, and (3) the result does not begin with a
has_dos_drive_prefix() or is_dir_sep(), then we should be safe against
directory traversal attacks.  I suggest doing this kind of check even if
not doing the full check_refname_format() check.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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] imap-send.c: imap_folder - imap_server_conf.folder

2014-08-20 Thread Junio C Hamano
Bernhard Reiter ock...@raz.or.at writes:

 Rename the imap_folder variable to folder and make it a member
 of struct imap_server_conf.

 Signed-off-by: Bernhard Reiter ock...@raz.or.at
 ---
 As discussed in
 http://www.mail-archive.com/git@vger.kernel.org/msg57019.html

 Bernhard

  imap-send.c | 10 +-
  1 file changed, 5 insertions(+), 5 deletions(-)

 diff --git a/imap-send.c b/imap-send.c
 index fb01a9c..05a02b5 100644
 --- a/imap-send.c
 +++ b/imap-send.c
 @@ -69,6 +69,7 @@ struct imap_server_conf {
   char *tunnel;
   char *host;
   int port;
 + char *folder;
   char *user;
   char *pass;
   int use_ssl;
 @@ -82,6 +83,7 @@ static struct imap_server_conf server = {
   NULL,   /* tunnel */
   NULL,   /* host */
   0,  /* port */
 + NULL,   /* folder */
   NULL,   /* user */
   NULL,   /* pass */
   0,  /* use_ssl */
 @@ -1323,8 +1325,6 @@ static int split_msg(struct strbuf *all_msgs,
 struct strbuf *msg, int *ofs)
   return 1;
  }
  -static char *imap_folder;
 -
  static int git_imap_config(const char *key, const char *val, void *cb)
  {
   if (!skip_prefix(key, imap., key))

The patch is corrupt; even though it claims to be text/plain, it
smells like some sort of text/flawed, but it is even worse.  Even
the line counts on @@ lines do not match what is in the patch text.

I wiggled it in so there is no need to resend, but please double
check your outgoing mail toolchain (sending the patch first to
yourself in exactly the same way as you would later send it to the
list and checking what comes out would be one good way to check).

Thanks.

 @@ -1339,7 +1339,7 @@ static int git_imap_config(const char *key, const
 char *val, void *cb)
   return config_error_nonbool(key);
   if (!strcmp(folder, key)) {
 - imap_folder = xstrdup(val);
 + server.folder = xstrdup(val);
   } else if (!strcmp(host, key)) {
   if (starts_with(val, imap:))
   val += 5;
 @@ -1387,7 +1387,7 @@ int main(int argc, char **argv)
   if (!server.port)
   server.port = server.use_ssl ? 993 : 143;
  -if (!imap_folder) {
 + if (!server.folder) {
   fprintf(stderr, no imap store specified\n);
   return 1;
   }
 @@ -1424,7 +1424,7 @@ int main(int argc, char **argv)
   }
   fprintf(stderr, sending %d message%s\n, total, (total != 1) ? s :
 );
 - ctx-name = imap_folder;
 + ctx-name = server.folder;
   while (1) {
   unsigned percent = n * 100 / total;
  -- 2.1.0.3.g63c96dd
--
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 16/18] receive-pack: GPG-validate push certificates

2014-08-20 Thread Junio C Hamano
David Turner dtur...@twopensource.com writes:

 On Wed, 2014-08-20 at 10:29 -0700, Junio C Hamano wrote:
 On Wed, Aug 20, 2014 at 9:56 AM, David Turner dtur...@twopensource.com 
 wrote:
  On Tue, 2014-08-19 at 15:06 -0700, Junio C Hamano wrote:
  Reusing the GPG signature check helpers we already have, verify
  the signature in receive-pack and give the results to the hooks
  via GIT_PUSH_CERT_{SIGNER,KEY,STATUS} environment variables.
 
  Policy decisions, such as accepting or rejecting a good signature by
  a key that is not fully trusted, is left to the hook and kept
  outside of the core.
 
  If I understand correctly, the hook does not have enough information to
  make this decision, because it is missing the date from the signature.
 
 The full certificate is available to the hook so anything we can do the hook
 has enough information to do ;-)  But of course we should try to make it
 easier for the hook to validate the request.

 Excellent, then motivated hooks can do the right thing.

  This might allow an old signed push to be replayed, moving the head of a
  branch to an older state (say, one lacking the latest security updates).
 
 ... with old-sha1 recorded in the certificate?

 That does prevent most replays, but it does not prevent resurrection of
 a deleted branch by a replay of its initial creation (nor an undo of a
 force-push to rollback).  So I think we still need timestamps, but
 parsing them out of the cert is not terrible.

As I aleady mentioned elsewhere, a more problematic thing about the
push certificate as presented in 15/18 is that it does not say
anything about where the push is going.  If you can capture a trial
push to some random test repository I do with my signed push
certificate, you could replay it to my public repository hosted at
a more official site (say, k.org in the far distant future where it
does not rely on ssh authentication to protect their services but
uses the GPG signature on the push certificate to make sure it is I
who is pushing).

We can add a new pushed-to repository URL header line to the
certificate, next to pushed-by ident time, and have the
receiving end verify that it matches to prevent such a replay.  I
wonder if we can further extend it to avoid replays to the same
repository.

Instead of pushed-to, we can tweak the capability advertisement
sent from the server upon initial contact to advertise not just
push-cert, but push-cert=nonce, add a new push-nonce nonce
header to the certificate and then have the receiving end make sure
they are the same.  That way, the receiver can make sure it is not
being fed a certificate used when a different push was done to it or
somebody else and by doing so we do not even need pushed-to
repository URL header, perhaps?

I am still fuzzy how robust such a scheme be against MITM, though.
One way I can think of to attack the nonce-only scheme would be to
create a you can push anything here service, convince me to push
garbage there, and when I try to push to it, it can turn around and
act as a client to some high-value site the attacker does not even
control, grab the nonce, relay it back to me and advertise the
same nonce, have me sign the certificate to push garbage, and
relay that push session to the high-value target.

I am not sure if that is a valid threat model we would care about,
but with pushed-to repository URL the high-value target site can
notice that I am pushing garbage to the joker site and reject the
certificate.

--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Junio C Hamano
Michael Haggerty mhag...@alum.mit.edu writes:

 I think we can get away with not including broken refnames when
 iterating.  After all, the main goal of tolerating them is to let them
 be deleted, right?

Or read from a ref whose name has retroactively made invalid, in
order to create a similar but validly named ref to serve as its
replacement?  So at least we need to give the users some way of
reading from them, in addition to deleting them, no?
--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Michael Haggerty
On 08/20/2014 09:45 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 I think we can get away with not including broken refnames when
 iterating.  After all, the main goal of tolerating them is to let them
 be deleted, right?
 
 Or read from a ref whose name has retroactively made invalid, in
 order to create a similar but validly named ref to serve as its
 replacement?  So at least we need to give the users some way of
 reading from them, in addition to deleting them, no?

The die() error message would presumably include the name of the invalid
reference.

If we change the rules for reference names and a bunch of names become
invalid, then it would admittedly be cumbersome to run git N times to
find the N invalid names.  But if we change the rules, then *at that
time* we could always build in iteration over broken reference names.

It's not that I have something against building it iteration over broken
reference names now, as long as it is clearly segregated from normal
iteration to prevent illegal names from getting loose in the code.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu

--
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] intersect_paths: respect mode in git's tree-sort

2014-08-20 Thread Junio C Hamano
Thanks.

Will queue on top of v2.0.0-rc0~146^2 (tests: add checking that
combine-diff emits only correct paths, 2014-02-03) so that we can
merge the fix down to 2.0 maintenance track later.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/4] Added tests for the case of merged and unmerged entries for the same file

2014-08-20 Thread Junio C Hamano
Jaime Soriano Pastor jsorianopas...@gmail.com writes:

 Signed-off-by: Jaime Soriano Pastor jsorianopas...@gmail.com
 ---
  t/t9904-unmerged-file-with-merged-entry.sh | 86 
 ++

Isn't this number already used for another test?  A test on the
index probably belongs to t2XXX or t3XXX family.

  1 file changed, 86 insertions(+)
  create mode 100755 t/t9904-unmerged-file-with-merged-entry.sh

 diff --git a/t/t9904-unmerged-file-with-merged-entry.sh 
 b/t/t9904-unmerged-file-with-merged-entry.sh
 new file mode 100755
 index 000..945bc1c
 --- /dev/null
 +++ b/t/t9904-unmerged-file-with-merged-entry.sh
 @@ -0,0 +1,86 @@
 +#!/bin/sh
 +
 +test_description='Operations with unmerged files with merged entries'
 +
 +. ./test-lib.sh
 +
 +setup_repository() {
 + test_commit A conflict A
 + test_commit A conflict2 A2 branchbase
 + test_commit B conflict B
 + test_commit B conflict2 B2
 + git checkout branchbase -b branch1
 + test_commit C conflict C
 + test_commit C conflict2 C2
 + test_commit something otherfile otherfile
 +}

No error is checked here?

 +setup_stage_state() {
 + git checkout -f HEAD
 + {
 + git ls-files -s conflict conflict2
 + git merge master  /dev/null
 + git ls-files -s conflict conflict2
 + }  index

No error is checked here?

Style: no SP between redirection operator and its target, i.e.

git merge master /dev/null
{ ... } index

 + cat index | git update-index --index-info

Do not cat a single file into a pipeline, i.e.

git update-index --index-info index


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


Re: [PATCH 4/4] git update-index --cacheinfo can be used to select a stage when there are merged and unmerged entries

2014-08-20 Thread Junio C Hamano
Jaime Soriano Pastor jsorianopas...@gmail.com writes:

 Subject: Re: [PATCH 4/4] git update-index --cacheinfo can be used to select
  a stage when there are merged and unmerged entries

Hmph, what does it even mean?  Shared with your [1/4] is that it is
unclear if you are stating an existing problem to be fixed or
describing the desired end result.

Also update-index --cacheinfo is not about selecting but is
about stuffing an entry to the index, so can be used to select
is doubly puzzling...

   ...
 +test_expect_success 'git update-index --cacheinfo to select a stage to use' '
 + setup_stage_state 
 + git cat-file blob :1:conflict  conflict 

Style: no SP between redirection and its target.

 + git update-index --cacheinfo 100644,`git hash-object conflict`,conflict

Style: we prefer $() over ``

 + git ls-files -s conflict  output 
 + test_line_count = 1 output

Is we have only one line the only thing we care about?  Don't we
want to check which stage the entry is at?

 +'
 +
  test_done
--
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 v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Junio C Hamano
Michael Haggerty mhag...@alum.mit.edu writes:

 On 08/20/2014 09:45 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:
 
 I think we can get away with not including broken refnames when
 iterating.  After all, the main goal of tolerating them is to let them
 be deleted, right?
 
 Or read from a ref whose name has retroactively made invalid, in
 order to create a similar but validly named ref to serve as its
 replacement?  So at least we need to give the users some way of
 reading from them, in addition to deleting them, no?

 The die() error message would presumably include the name of the invalid
 reference.

 If we change the rules for reference names and a bunch of names become
 invalid, then it would admittedly be cumbersome to run git N times to
 find the N invalid names.  But if we change the rules, then *at that
 time* we could always build in iteration over broken reference names.

 It's not that I have something against building it iteration over broken
 reference names now, as long as it is clearly segregated from normal
 iteration to prevent illegal names from getting loose in the code.

Oh, I don't think it is that important for iterator to show broken
ones.  If refs/heads/foo/$brokenname exists but is skipped from any
and all iterations, nobody will get hurt until the end user wonders
where foo/$brokenname went, at which time rev-parse can be used to
grab the value from it before update-ref -d can be used to remove it.

If refs/heads/foo/$brokenname, which is skipped from iterations,
prevents a valid ref refs/heads/foo from getting created, that would
give a bit of surprise to the user who long forgot foo/$brokenname
existed, but the recovery procedure is exactly the same as the case
where he has a branch foo/$koshername and wants to create foo; first
'branch -D' the D/F-conflicting one and then create foo.

So the primary things I care about are when the user has a string
that is the name of an existing misnamed ref, the value of the ref
can be obtained, and the ref can be removed.

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


Re: [PATCH v20 43/48] refs.c: move the check for valid refname to lock_ref_sha1_basic

2014-08-20 Thread Ronnie Sahlberg
On Wed, Aug 20, 2014 at 1:11 PM, Michael Haggerty mhag...@alum.mit.edu wrote:
 On 08/20/2014 09:45 PM, Junio C Hamano wrote:
 Michael Haggerty mhag...@alum.mit.edu writes:

 I think we can get away with not including broken refnames when
 iterating.  After all, the main goal of tolerating them is to let them
 be deleted, right?

 Or read from a ref whose name has retroactively made invalid, in
 order to create a similar but validly named ref to serve as its
 replacement?  So at least we need to give the users some way of
 reading from them, in addition to deleting them, no?

 The die() error message would presumably include the name of the invalid
 reference.

 If we change the rules for reference names and a bunch of names become
 invalid, then it would admittedly be cumbersome to run git N times to
 find the N invalid names.  But if we change the rules, then *at that
 time* we could always build in iteration over broken reference names.

 It's not that I have something against building it iteration over broken
 reference names now, as long as it is clearly segregated from normal
 iteration to prevent illegal names from getting loose in the code.

We already have iterators that show also bad refs.
For example, git branch uses
DO_FOR_EACH_INCLUDE_BROKEN
which is a flag to include also broken refs that can not be resolved
which allows them to be listed :



$ echo this is not a valid sha1 .git/refs/heads/broken
$ git branch
  broken
  foo
* master
$ git branch -D broken
error: branch 'broken' not found.

(allowing -D to remove it is in a different patch further down my series)


Since we already display broken/unresolvable refs, I think the most
consistent path is to also allow showing the refs broken/illegal-names
too in the list. (when DO_FOR_EACH_INCLUDE_BROKEN is specified)
Of course, an end user could fix this by deleting the file but since
it is easy to add the special casing to 'git branch -D' to handle this
case I think this would be more userfriendly since then the user can
use git branch -D regardless of the reason why the ref is broken.


Ronnie
--
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 v13 11/11] Documentation: add documentation for 'git interpret-trailers'

2014-08-20 Thread Marc Branchaud
On 14-08-16 12:06 PM, Christian Couder wrote:
 While at it add git-interpret-trailers to command-list.txt.
 
 Signed-off-by: Christian Couder chrisc...@tuxfamily.org
 Signed-off-by: Junio C Hamano gits...@pobox.com
 ---
  Documentation/git-interpret-trailers.txt | 308 
 +++
  command-list.txt |   1 +
  2 files changed, 309 insertions(+)
  create mode 100644 Documentation/git-interpret-trailers.txt
 
 diff --git a/Documentation/git-interpret-trailers.txt 
 b/Documentation/git-interpret-trailers.txt
 new file mode 100644
 index 000..cf5b194
 --- /dev/null
 +++ b/Documentation/git-interpret-trailers.txt
 @@ -0,0 +1,308 @@
 +git-interpret-trailers(1)
 +=
 +
 +NAME
 +
 +git-interpret-trailers - help add stuctured information into commit messages
 +
 +SYNOPSIS
 +
 +[verse]
 +'git interpret-trailers' [--trim-empty] [(--trailer 
 token[(=|:)value])...] [file...]
 +
 +DESCRIPTION
 +---
 +Help adding 'trailers' lines, that look similar to RFC 822 e-mail
 +headers, at the end of the otherwise free-form part of a commit
 +message.
 +
 +This command reads some patches or commit messages from either the
 +file arguments or the standard input if no file is specified. Then
 +this command applies the arguments passed using the `--trailer`
 +option, if any, to the commit message part of each input file. The
 +result is emitted on the standard output.
 +
 +Some configuration variables control the way the `--trailer` arguments
 +are applied to each commit message and the way any existing trailer in
 +the commit message is changed. They also make it possible to
 +automatically add some trailers.
 +
 +By default, a 'token=value' or 'token:value' argument given
 +using `--trailer` will be appended after the existing trailers only if
 +the last trailer has a different (token, value) pair (or if there
 +is no existing trailer). The token and value parts will be trimmed
 +to remove starting and trailing whitespace, and the resulting trimmed
 +token and value will appear in the message like this:
 +
 +
 +token: value
 +
 +
 +This means that the trimmed token and value will be separated by
 +`': '` (one colon followed by one space).
 +
 +By default the new trailer will appear at the end of all the existing
 +trailers. If there is no existing trailer, the new trailer will appear
 +after the commit message part of the ouput, and, if there is no line
 +with only spaces at the end of the commit message part, one blank line
 +will be added before the new trailer.
 +
 +The trailers are recognized in the input message using the following
 +rules:
 +
 +* by default only lines that contains a ':' (colon) are considered

s/contains/contain/

 +  trailers,
 +
 +* the trailer lines must all be next to each other,
 +
 +* after them it's only possible to have some lines that contain only
 +  spaces, and then a patch; the patch part is recognized using the
 +  fact that its first line starts with '---' (three minus signs),

Is that starts with or consists solely of?

 +
 +* before them there must be at least one line with only spaces.

I had little bit of trouble parsing those three points, and it seems like a
lot of text to describe something simple.  How about a single paragraph:

Existing trailers are extracted from the input message by looking for a group
of one or more lines that contain a colon (by default), where the group is
preceded by one or more empty (or whitespace-only) lines.  The group must
either be at the end of the message or be the last non-whitespace lines
before a line that starts with '---' (three minus signs).


Also, will a trailer be recognized if there is whitespace before and/or after
the separator?  Can there be whitespace before the token?  In the token?  (I
don't feel strongly about the answers to these questions, they just came to
mind as I read the documentation.)

 +
 +Note that 'trailers' do not follow and are not intended to follow many
 +rules for RFC 822 headers. For example they do not follow the line
 +folding rules, the encoding rules and probably many other rules.
 +
 +OPTIONS
 +---
 +--trim-empty::
 + If the value part of any trailer contains only whitespace,
 + the whole trailer will be removed from the resulting message.
 + This apply to existing trailers as well as new trailers.
 +
 +--trailer token[(=|:)value]::
 + Specify a (token, value) pair that should be applied as a
 + trailer to the input messages. See the description of this
 + command.
 +
 +CONFIGURATION VARIABLES
 +---
 +
 +trailer.separators::
 + This option tells which characters are recognized as trailer
 + separators. By default only ':' is recognized as a trailer
 + separator, except that '=' is always accepted on the command
 + line for compatibility with other git commands.
 ++
 

Re: [PATCH 0/4] Handling unmerged files with merged entries

2014-08-20 Thread Junio C Hamano
Jaime Soriano Pastor jsorianopas...@gmail.com writes:

 New approach for the case of finding unmerged files with merged entries
 in the index.
 After some discussion the solution tries to:
 - Avoid the problems with infinite loops in this case.
 - Provide better information to the user in the commands affected.
 - Make sure there are ways to clean the index.
 - Provide also a way to specifically recover each one of the files with
   this problem.

 With these patches the behaviour of these commands (for this case) change:
 - git reset is able to finish, cleaning the index, but warning out the
   information about the removed stages.
 - git merge is able to finish, reporting that there is a merge in progress as
   usual, it also warns about the unmerged files with merged entries.
 - git add fails when this case happens, telling the user to check the state
   with 'git ls-files -s', before, it did nothing. The same with git commit -a.
 - git update-index --cacheinfo can be used to select an specific staged
   version to resolve the conflict, without the need of reseting the working
   copy. It did nothing before.

 Tests added for these cases. Rest of the tests remain unchanged and pass too.

Thanks.

After looking at what you did in 1/4, I started to wonder if we can
solve this in add_index_entry_with_check() in a less intrusive way.
When we call the function with a stage #0 entry, we are telling the
index that any entry in higher stage for the same path must
disappear.  Since the current implementation of the function assumes
that the index is not corrupt in this particular way to have both
merged and unmerged entries for the same path, it fails to remove
the higher stage entries.  If we fix the function, wouldn't it make
your 1/4 unnecessary?  Read-only operations such as ls-files -s
would not call add_index_entry() so diagnostic tools would not be
affected even with such a fix.

... which may look something like the one attached at the end.

But then it made me wonder even more.

There are other ways a piece of software can leave a corrupt index
for us to read from.  Your fix, or the simpler one I suggested for
that matter, would still assume that the index entries are in the
sorted order, and a corrupt index that does not sort its entries
correctly will cause us to behave in an undefined way.  At some
point we should draw a line and say Your index is hopelessly
corrupt., send it back to whatever broken software that originally
wrote such a mess and have the user use that software to fix the
corrupt index up before talking to us.

For that, we need to catch an index whose entries are not sorted and
error out, perhaps when read_index_from() iterates over the mmapped
index entries.  We can even draw that hopelessly corrupt line
above the breakage you are addressing and add a check to make sure
no path has both merged and unmerged entries to the same check to
make it error out.

I suspect that such a detect and error out may be sufficient and
also may be more robust than the approach that assumes that a
breakage is only to have both merged and unmerged entries for the
same path, the entries are still correctly sorted.


 read-cache.c | 11 +--
 1 file changed, 9 insertions(+), 2 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index 7f5645e..56006a3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -943,9 +943,16 @@ static int add_index_entry_with_check(struct index_state 
*istate, struct cache_e
if (pos = 0) {
if (!new_only)
replace_index_entry(istate, pos, ce);
-   return 0;
+   /*
+* ... but protect ourselves from a corrupt index
+* that has an unmerged entry for the same path.
+*/
+   if (istate-cache_nr = pos + 1 ||
+   !ce_same_name(ce, istate-cache[pos + 1]))
+   return 0;
+   } else {
+   pos = -pos-1;
}
-   pos = -pos-1;
 
/*
 * Inserting a merged entry (stage 0) into the index





[Footnote]


--
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: Transaction patch series overview

2014-08-20 Thread Jonathan Nieder
Hi,

Ronnie Sahlberg wrote:

 List, please see here an overview and ordering of the ref transaction
 patch series.

Thanks much for this.

[...]
 rs/ref-transaction-0
[...]
 Has been merged into next.

This is even part of master now, so if people have review comments
then they can make them most easily by submitting new patches on top.

 ref-transaction-1 (2014-07-16) 20 commits
 -
 Second batch of ref transactions

  - refs.c: make delete_ref use a transaction
  - refs.c: make prune_ref use a transaction to delete the ref
  - refs.c: remove lock_ref_sha1
  - refs.c: remove the update_ref_write function
  - refs.c: remove the update_ref_lock function
  - refs.c: make lock_ref_sha1 static
  - walker.c: use ref transaction for ref updates
  - fast-import.c: use a ref transaction when dumping tags
  - receive-pack.c: use a reference transaction for updating the refs
  - refs.c: change update_ref to use a transaction
  - branch.c: use ref transaction for all ref updates
  - fast-import.c: change update_branch to use ref transactions
  - sequencer.c: use ref transactions for all ref updates
  - commit.c: use ref transactions for updates
  - replace.c: use the ref transaction functions for updates
  - tag.c: use ref transactions when doing updates
  - refs.c: add transaction.status and track OPEN/CLOSED/ERROR
  - refs.c: make ref_transaction_begin take an err argument
  - refs.c: update ref_transaction_delete to check for error and return status
  - refs.c: change ref_transaction_create to do error checking and return 
 status
  (this branch is used by rs/ref-transaction, rs/ref-transaction-multi,
 rs/ref-transaction-reflog and rs/ref-transaction-rename.)

  The second batch of the transactional ref update series.

 Has been merged into pu

Last reviewed at
http://thread.gmane.org/gmane.comp.version-control.git/252226, if
I am using gmane search correctly.

Are there any pending questions for this part?

I've having trouble keeping track of how patches change, which patches
have been reviewed and which haven't, unaddressed comments, and so on,
so as an experiment I've pushed this part of the series to the Gerrit
server at

 https://code-review.googlesource.com/#/q/topic:ref-transaction-1

It's running a copy of gerrit code review
(https://code.google.com/p/gerrit).

Maybe this can be useful as a way of keeping track of the state of
long and long-lived series like this one.  It works something like
this:

Preparation
---
Get a password from https://code-review.googlesource.com/new-password
and put it in netrc.

Install the hook from
https://code-review.googlesource.com/tools/hooks/commit-msg to
.git/hooks/commit-msg.  This automatically populates a Change-Id
line in the commit message if none is present so gerrit can
tell when you are uploading a new version of an existing patch.

(The commit-msg hook can be annoying when not working with gerrit
code review.  To turn it off, use 'git config gerrit.createChangeId false'.)

Uploading patches
-
Write a patch series against 'maint' or 'master' as usual, then push:

git push https://code.googlesource.com/git \
HEAD:refs/for/master; # or refs/for/maint

There can be a parameter '%topic=name' after the refs/for/branch
(e.g., refs/for/master%topic=ref-transaction-1) if the series should
be labelled with a topic name, or that can be set in the web UI or
using the HTTP API after the fact.

Commenting on patches
-
Visit https://code-review.googlesource.com.  Patches can be found
under All - Open.  Patches you're involved with somehow are
at My - Changes.

To prepare inline comments, choose a file, highlight the text to comment
on or click a line number, and comment.

To publish comments, go back up to the change overview screen (using
the up arrow button or by pressing u), click Reply, and say
something.

'?' brings up keyboard shortcuts.

Downloading patches
---
In the change overview screen, there's a 'Download' menu in the
top-right corner with commands to download the patch.

Revising patches

After modifying a patch locally using the usual tools such as rebase
--interactive, push again:

git push https://code.googlesource.com/git \
HEAD:refs/for/master; # or refs/for/maint

When a patch seems polished
---
Get rid of the Change-Id lines --- they shouldn't be needed any
more.  Send the patches or a request to pull from a public git
repository, as usual.  A link (in the top-left corner where it says
'Change 1000', the 1000 is a link to the change) can be helpful
for letting people on-list see how the patch got into the form it's
in today.

Maybe it can be useful.

Thanks,
Jonathan
--
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


What's cooking in git.git (Aug 2014, #03; Wed, 20)

2014-08-20 Thread Junio C Hamano
Here are the topics that have been cooking.  Commits prefixed with
'-' are only in 'pu' (proposed updates) while commits prefixed with
'+' are in 'next'.

A few fixes for regressions have been posted since 2.1 was tagged,
which I'd like to address before moving any topic to 'master',
rewinding 'next', or doing anything for the 2.2 cycle.  Hopefully by
the end of this week 'next' will be rewound and rebuilt and then we
can start taking patches for new topics in earnest from that point.

You can find the changes described here in the integration branches
of the repositories listed at

http://git-blame.blogspot.com/p/git-public-repositories.html

--
[New Topics]

* br/http-init-fix (2014-08-18) 2 commits
 - http: style fixes for curl_multi_init error check
 - http.c: die if curl_*_init fails

 Needs S-o-b from peff for the topmost one.


* br/imap-send-simplify-tunnel-child-process (2014-08-20) 2 commits
 - imap-send.c: imap_folder - imap_server_conf.folder
 - git-imap-send: simplify tunnel construction

 Will merge to 'next'.


* jc/config-mak-document-darwin-vs-macosx (2014-08-15) 1 commit
 - config.mak.uname: add hint on uname_R for MacOS X
 (this branch uses km/no-apple-common-crypto-on-darwin-8-and-below.)

 Will merge to 'next'.


* jk/fix-profile-feedback-build (2014-08-19) 1 commit
 - Makefile: make perf tests optional for profile build

 Fix profile-feedback build broken in 2.1 for tarball releases.

 Will merge to 'next'.


* jk/pack-shallow-always-without-bitmap (2014-08-12) 1 commit
 - pack-objects: turn off bitmaps when we see --shallow lines

 Reachability bitmaps do not work with shallow operations.

 Will merge to 'next'.


* jn/header-dependencies (2014-08-10) 1 commit
 - Update hard-coded header dependencies

 Needs further discussions on the list.


* jn/unpack-trees-checkout-m-carry-deletion (2014-08-13) 4 commits
 - SQUASH???
 - checkout -m: attempt merge when deletion of path was staged
 - unpack-trees: use 'cuddled' style for if-else cascade
 - unpack-trees: simplify 'all other failures' case

 Needs tests; perhaps squashing in the topmost SQUASH??? is enough.


* km/no-apple-common-crypto-on-darwin-8-and-below (2014-08-15) 1 commit
 - config.mak.uname: set NO_APPLE_COMMON_CRYPTO on older systems
 (this branch is used by jc/config-mak-document-darwin-vs-macosx.)

 Build automation for older versions of MacOS X.

 Will merge to 'next'.


* nd/large-blobs (2014-08-18) 5 commits
 - diff: shortcut for diff'ing two binary SHA-1 objects
 - diff --stat: mark any file larger than core.bigfilethreshold binary
 - diff.c: allow to pass more flags to diff_populate_filespec
 - sha1_file.c: do not die failing to malloc in unpack_compressed_entry
 - wrapper.c: introduce gentle xmallocz that does not die()

 Will merge to 'next'.


* nd/mv-code-cleaning (2014-08-11) 9 commits
 - SQUASH???
 - mv: no SP between function name and the first opening parenthese
 - mv: combine two if(s)
 - mv: unindent one level for directory move code
 - mv: move index search code out
 - mv: remove an if that's always true
 - mv: split submodule move preparation code out
 - mv: flatten error handling code block
 - mv: mark strings for translations

 Needs rerolling to account for the topmost SQUASH??? simplification.


* nd/strbuf-utf8-replace (2014-08-11) 1 commit
 - utf8.c: fix strbuf_utf8_replace() consuming data beyond input string

 Will merge to 'next'.


* rr/mergetool-temporary-filename-tweak (2014-08-19) 1 commit
 - Allow the user to change the temporary file name for mergetool

 Needs rerolling (new paragraph in doc seems to be in a wrong place)


* rs/clean-menu-item-defn (2014-08-18) 1 commit
 - clean: use f(void) instead of f() to declare a pointer to a function without 
arguments

 Will merge to 'next'.


* rs/inline-compat-path-macros (2014-08-18) 1 commit
 - turn path macros into inline function

 Will merge to 'next'.


* rs/refresh-beyond-symlink (2014-08-10) 1 commit
 - read-cache: check for leading symlinks when refreshing index

 git add x where x that used to be a directory has become a
 symbolic link to a directory misbehaved.

 Will merge to 'next'.


* sb/blame-msg-i18n (2014-08-12) 1 commit
 - builtin/blame.c: add translation to warning about failed revision walk

 Will merge to 'next'.


* sb/plug-leaks (2014-08-10) 2 commits
 - clone.c: don't leak memory in cmd_clone
 - remote.c: don't leak the base branch name in format_tracking_info

 Will merge to 'next'.


* sb/prepare-revision-walk-error-check (2014-08-12) 1 commit
 - prepare_revision_walk(): check for return value in all places

 Will merge to 'next'.


* sb/unpack-trees-dead-code-removal (2014-08-12) 2 commits
 - SQUASH???
 - unpack-tree.c: remove dead code

 Will discard (jn/unpack-trees-checkout-m-carry-deletion should do the same).


* so/rebase-doc (2014-08-12) 1 commit
 - Documentation/git-rebase.txt: -f forces a rebase that would otherwise be a 
no-op

 May need description on what 

Re: [PATCH 15/18] the beginning of the signed push

2014-08-20 Thread Junio C Hamano
Bert Wesarg bert.wes...@googlemail.com writes:

 On Wed, Aug 20, 2014 at 12:06 AM, Junio C Hamano gits...@pobox.com wrote:
 The basic flow based on this mechanism goes like this:

  1. You push out your work with git push -s.

  2. The sending side learns where the remote refs are as usual,
 together with what protocol extension the receiving end
 supports.  If the receiving end does not advertise the protocol
 extension push-cert, the sending side falls back to the normal
 push that is not signed.


 Is this fallback silently? If so I think it would be better to abort
 the push, if the receiver does not support this but the user requested
 it.

Let me change it in the reroll.  I however am not quite sure if
warning is insufficient, because there is nothing, other than
rerunning the command without --signed, that the user could do
when it happens.
--
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 v13 11/11] Documentation: add documentation for 'git interpret-trailers'

2014-08-20 Thread Christian Couder
On Thu, Aug 21, 2014 at 12:05 AM, Marc Branchaud marcn...@xiplink.com wrote:
 On 14-08-16 12:06 PM, Christian Couder wrote:

 +The trailers are recognized in the input message using the following
 +rules:
 +
 +* by default only lines that contains a ':' (colon) are considered

 s/contains/contain/

Ok.

 +  trailers,
 +
 +* the trailer lines must all be next to each other,
 +
 +* after them it's only possible to have some lines that contain only
 +  spaces, and then a patch; the patch part is recognized using the
 +  fact that its first line starts with '---' (three minus signs),

 Is that starts with or consists solely of?

It is starts with. (The starts_with() function is used.)

 +
 +* before them there must be at least one line with only spaces.

 I had little bit of trouble parsing those three points, and it seems like a
 lot of text to describe something simple.  How about a single paragraph:

 Existing trailers are extracted from the input message by looking for a group
 of one or more lines that contain a colon (by default), where the group is
 preceded by one or more empty (or whitespace-only) lines.  The group must
 either be at the end of the message or be the last non-whitespace lines
 before a line that starts with '---' (three minus signs).

Ok, I will use something like that, thanks.

 Also, will a trailer be recognized if there is whitespace before and/or after
 the separator?

Yes.

 Can there be whitespace before the token?

Yes.

  In the token?

Yes.

 (I don't feel strongly about the answers to these questions, they just came to
 mind as I read the documentation.)

Ok, I will add something.

 +Note that 'trailers' do not follow and are not intended to follow many
 +rules for RFC 822 headers. For example they do not follow the line
 +folding rules, the encoding rules and probably many other rules.
 +
 +OPTIONS
 +---
 +--trim-empty::
 + If the value part of any trailer contains only whitespace,
 + the whole trailer will be removed from the resulting message.
 + This apply to existing trailers as well as new trailers.
 +
 +--trailer token[(=|:)value]::
 + Specify a (token, value) pair that should be applied as a
 + trailer to the input messages. See the description of this
 + command.
 +
 +CONFIGURATION VARIABLES
 +---
 +
 +trailer.separators::
 + This option tells which characters are recognized as trailer
 + separators. By default only ':' is recognized as a trailer
 + separator, except that '=' is always accepted on the command
 + line for compatibility with other git commands.
 ++
 +The first character given by this option will be the default character
 +used when another separator is not specified in the config for this
 +trailer.
 ++
 +For example, if the value for this option is %=$, then only lines
 +using the format 'tokensepvalue' with sep containing '%', '='
 +or '$' and then spaces will be considered trailers. And '%' will be
 +the default separator used, so by default trailers will appear like:
 +'token% value' (one percent sign and one space will appear between
 +the token and the value).
 +
 +trailer.where::
 + This option tells where a new trailer will be added.
 ++
 +This can be `end`, which is the default, `start`, `after` or `before`.
 ++
 +If it is `end`, then each new trailer will appear at the end of the
 +existing trailers.
 ++
 +If it is `start`, then each new trailer will appear at the start,
 +instead of the end, of the existing trailers.
 ++
 +If it is `after`, then each new trailer will appear just after the
 +last trailer with the same token.
 ++
 +If it is `before`, then each new trailer will appear just before the
 +last trailer with the same token.

 It seems to me that it would be more sensible to make the new trailer appear
 before the *first* trailer with the same token.

Yeah, it is a copy-paste mistake that I will fix.

Thanks for the careful review,
Christian.
--
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