Bug: Segfault when doing "git diff"

[Mathias L. Baumann]

Hello dear git devs,

I just stumbled upon a segfault when doing just "git diff" in my repo.

I managed to create a minimal repo setup where the bug is reproducable.

The problem seems to be a mix of having an untracked submodule and 
having set an alternates file for one submodule.


Attached you'll find my setup that will reproduce the problem. Simply 
run  'git diff' in bugtest1.


In case the attachment is refused, I also uploaded it here:

http://supraverse.net/bugdemo.tar.gz

cheers,

--Marenz


bugdemo.tar.gz
Description: GNU Zip compressed data

Re: [PATCH/RFC] receive-pack: allow for hiding refs outside the namespace

[Jeff King]

On Wed, Oct 28, 2015 at 08:00:45AM +0100, Lukas Fleischer wrote:

> My original question remains: Do we want to continue supporting things
> like transfer.hideRefs=.have (which currently magically hides all refs
> outside the current namespace)? For 100% backwards compatibility, we
> would have to. On the other hand, one could consider the current
> behavior a bug and one could argue that it is weird enough that probably
> nobody (apart from me) relies on it right now. If we decide to keep it
> anyway, I think it should be documented.

I don't think that hiding ".have" refs at that level is especially
useful. I do not use namespaces, but I do use alternates extensively,
and that is the original source of these ".have" refs. But filtering
them at the advertisement layer is very inefficient, as it is expensive
to get the list in the first place (we spawn ls-remote, which spawns
upload-pack in the alternate!). So we'd want to prevent that process
much earlier.

I have an unpublished patch to specially disable alternates
advertisement entirely (i.e., adding a new boolean config,
receive.advertiseAlternates). In my case, it is because the alternates
repositories have huge numbers of refs (sometimes ranging into the
gigabytes) and the performance hit on even loading that packed-refs file
is too large.

I suppose that behavior _could_ be triggered by ".have" appearing in the
hiderefs config, though (i.e., before accessing the alternate, check
ref_is_hidden(".have")). That seems a bit too subtle to me, though.

> Another patch I have in my patch queue adds support for a whitelist mode
> to hideRefs. There are several ways to implement that:
> 
> 1. Make transfer.hideRefs='' hide all refs (it currently does not). The
>user can then whitelist refs explicitly using negative patterns
>below that rule. This is how my current implementation works. Using
>the empty string seemed most natural since hideRefs matches prefixes
>and every string has the empty string as a prefix. If that seems too
>weird, we could probably special case something like
>transfer.hideRefs='*' instead.
> 
> 2. Detect whether hideRefs only contains negative patterns. Switch to
>whitelist mode ("hide by default") in that case.
> 
> 3. Add another option to switch between "hide by default" and "show by
>default".
> 
> I personally prefer the first option. Any other opinions?

I am just a bystander and would not use this myself, but I think the 1st
is the least ugly. I am not sure why ignoring "refs/" does not work,
though (it does not catch ".have", of course, but I think that is a
feature; there are a finite set of pseudo-refs, so you can ignore those,
too, if you want).

-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] git-p4: Handle p4 submit failure

[Etienne Girard]

Clean the workspace if p4_write_pipe raised SystemExit,
so that the user don't have to do it themselves.

Signed-off-by: GIRARD Etienne 
---
 git-p4.py | 71 +--
 1 file changed, 37 insertions(+), 34 deletions(-)

The p4 submit command may fail, for example if the changelist contains
a job that does not exist in the Jobs section. If this is the case,
p4_write_pipe will exit the script. This change makes it so that the
workspace is reverted before letting the script die.

diff --git a/git-p4.py b/git-p4.py
index 0093fa3..d535904 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1538,44 +1538,47 @@ class P4Submit(Command, P4UserMap):
 #
 # Let the user edit the change description, then submit it.
 #
-if self.edit_template(fileName):
-# read the edited message and submit
-ret = True
-tmpFile = open(fileName, "rb")
-message = tmpFile.read()
-tmpFile.close()
-if self.isWindows:
-message = message.replace("\r\n", "\n")
-submitTemplate = message[:message.index(separatorLine)]
-p4_write_pipe(['submit', '-i'], submitTemplate)
-
-if self.preserveUser:
-if p4User:
-# Get last changelist number. Cannot easily get it from
-# the submit command output as the output is
-# unmarshalled.
-changelist = self.lastP4Changelist()
-self.modifyChangelistUser(changelist, p4User)
-
-# The rename/copy happened by applying a patch that created a
-# new file.  This leaves it writable, which confuses p4.
-for f in pureRenameCopy:
-p4_sync(f, "-f")
+submitted = False

-else:
+try:
+if self.edit_template(fileName):
+# read the edited message and submit
+tmpFile = open(fileName, "rb")
+message = tmpFile.read()
+tmpFile.close()
+if self.isWindows:
+message = message.replace("\r\n", "\n")
+submitTemplate = message[:message.index(separatorLine)]
+p4_write_pipe(['submit', '-i'], submitTemplate)
+
+if self.preserveUser:
+if p4User:
+# Get last changelist number. Cannot easily get it from
+# the submit command output as the output is
+# unmarshalled.
+changelist = self.lastP4Changelist()
+self.modifyChangelistUser(changelist, p4User)
+
+# The rename/copy happened by applying a patch that created a
+# new file.  This leaves it writable, which confuses p4.
+for f in pureRenameCopy:
+p4_sync(f, "-f")
+submitted = True
+
+finally:
 # skip this patch
-ret = False
-print "Submission cancelled, undoing p4 changes."
-for f in editedFiles:
-p4_revert(f)
-for f in filesToAdd:
-p4_revert(f)
-os.remove(f)
-for f in filesToDelete:
-p4_revert(f)
+if not submitted:
+print "Submission cancelled, undoing p4 changes."
+for f in editedFiles:
+p4_revert(f)
+for f in filesToAdd:
+p4_revert(f)
+os.remove(f)
+for f in filesToDelete:
+p4_revert(f)

 os.remove(fileName)
-return ret
+return submitted

 # Export git tags as p4 labels. Create a p4 label and then tag
 # with that.
-- 
1.9.1
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Bug: Segfault when doing "git diff"

[Victor Leschuk]

On 10/28/2015 02:58 PM, Mathias L. Baumann wrote:

Hello dear git devs,

I just stumbled upon a segfault when doing just "git diff" in my repo.

I managed to create a minimal repo setup where the bug is reproducable.

The problem seems to be a mix of having an untracked submodule and 
having set an alternates file for one submodule.


Attached you'll find my setup that will reproduce the problem. Simply 
run  'git diff' in bugtest1.


In case the attachment is refused, I also uploaded it here:

http://supraverse.net/bugdemo.tar.gz

cheers,

--Marenz

Hello Marenz,

I have just tried to reproduce segfault with the provided archive:

[del@del-debian bugtest1 (master)]$ git diff
diff --git a/submodules/bugtest2 b/submodules/bugtest2
--- a/submodules/bugtest2
+++ b/submodules/bugtest2
@@ -1 +1 @@
-Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126
+Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126-dirty

No segfault occured. I am using

git version 2.6.2.308.g3b8f10c

Could you please specify which version of git you are using and also try 
to reproduce it with latest 2.6.2?


--
Victor
--
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: Segfault when doing "git diff"

[Mathias L. Baumann]

I was using the latest git version 2.6.2 already.
I suspect it is due to a .gitconfig. This is what is probably required:

➜  ~  cat .gitconfig
[diff]
submodule = log


On 28/10/15 13:24, Victor Leschuk wrote:



On 10/28/2015 02:58 PM, Mathias L. Baumann wrote:

Hello dear git devs,

I just stumbled upon a segfault when doing just "git diff" in my repo.

I managed to create a minimal repo setup where the bug is reproducable.

The problem seems to be a mix of having an untracked submodule and
having set an alternates file for one submodule.

Attached you'll find my setup that will reproduce the problem. Simply
run  'git diff' in bugtest1.

In case the attachment is refused, I also uploaded it here:

http://supraverse.net/bugdemo.tar.gz

cheers,

--Marenz

Hello Marenz,

I have just tried to reproduce segfault with the provided archive:

[del@del-debian bugtest1 (master)]$ git diff
diff --git a/submodules/bugtest2 b/submodules/bugtest2
--- a/submodules/bugtest2
+++ b/submodules/bugtest2
@@ -1 +1 @@
-Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126
+Subproject commit cd0b9ee2946d2df3626943347332a4d86f93b126-dirty

No segfault occured. I am using

git version 2.6.2.308.g3b8f10c

Could you please specify which version of git you are using and also try
to reproduce it with latest 2.6.2?

--
Victor

--
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 2/2] rebase-i: work around Windows CRLF line endings

[Johannes Schindelin]

Hi Junio,

On Tue, 27 Oct 2015, Junio C Hamano wrote:

> Johannes Schindelin  writes:
> 
> > My apologies: I forgot to spell out explicitly that this passes in Git
> > for Windows 2.x' SDK: It does.
> 
> Can you add that to the log message?  Your cover letter may also want to
> be updated for those who fish for the last version posted in the list
> archive.

Done,
Johannes
--
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 v4 1/2] Demonstrate rebase fails when the editor saves with CR/LF

[Johannes Schindelin]

Based on a bug report by Chad Boles.

Signed-off-by: Johannes Schindelin 
---
 t/t3404-rebase-interactive.sh | 12 
 1 file changed, 12 insertions(+)

diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 3de0b1d..5dfa16a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,4 +1261,16 @@ test_expect_success 'static check of bad SHA-1' '
test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
+test_expect_failure 'editor saves as CR/LF' '
+   git checkout -b with-crlf &&
+   write_script add-crs.sh <<-\EOF &&
+   sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
+   mv -f "$1".new "$1"
+   EOF
+   (
+   test_set_editor "$(pwd)/add-crs.sh" &&
+   git rebase -i HEAD^
+   )
+'
+
 test_done
-- 
2.1.4


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

[PATCH] add_submodule_odb: initialize alt_odb list earlier

[Jeff King]

On Wed, Oct 28, 2015 at 02:35:23PM +0100, Mathias L. Baumann wrote:

> I was using the latest git version 2.6.2 already.
> I suspect it is due to a .gitconfig. This is what is probably required:
> 
> ➜  ~  cat .gitconfig
> [diff]
> submodule = log

Yeah, I can reproduce it easily with that. Thanks for providing the
repository. It takes a rather convoluted set of conditions to trigger
the bug. :)

Here's the fix:

-- >8 --
Subject: add_submodule_odb: initialize alt_odb list earlier

The add_submodule_odb function tries to add a submodule's
object store as an "alternate". It needs the existing list
to be initialized (from the objects/info/alternates file)
for two reasons:

  1. We look for duplicates with the existing alternate
 stores, but obviously this doesn't work if we haven't
 loaded any yet.

  2. We link our new entry into the list by prepending it to
 alt_odb_list. But we do _not_ modify alt_odb_tail.
 This variable starts as NULL, and is a signal to the
 alt_odb code that the list has not yet been
 initialized.

 We then call read_info_alternates on the submodule (to
 recursively load its alternates), which will try to
 append to that tail, assuming it has been initialized.
 This causes us to segfault if it is NULL.

This rarely comes up in practice, because we will have
initialized the alt_odb any time we do an object lookup. So
you can trigger this only when:

  - you try to access a submodule (e.g., a diff with
diff.submodule=log)

  - the access happens before any other object has been
accessed (e.g., because the diff is between the working
tree and the index)

  - the submodule contains an alternates file (so we try to
add an entry to the NULL alt_odb_tail)

To fix this, we just need to call prepare_alt_odb at the
start of the function (and if we have already initialized,
it is a noop).

Note that we can remove the prepare_alt_odb call from the
end. It is guaranteed to be a noop, since we will have
called it earlier.

Signed-off-by: Jeff King 
---
 submodule.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/submodule.c b/submodule.c
index 5879cfb..88af54c 100644
--- a/submodule.c
+++ b/submodule.c
@@ -130,6 +130,7 @@ static int add_submodule_odb(const char *path)
goto done;
}
/* avoid adding it twice */
+   prepare_alt_odb();
for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
if (alt_odb->name - alt_odb->base == objects_directory.len &&
!strncmp(alt_odb->base, objects_directory.buf,
@@ -148,7 +149,6 @@ static int add_submodule_odb(const char *path)
 
/* add possible alternates from the submodule */
read_info_alternates(objects_directory.buf, 0);
-   prepare_alt_odb();
 done:
strbuf_release(_directory);
return ret;
-- 
2.6.2.572.g6ed22dd

--
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: Segfault when doing "git diff"

[Victor Leschuk]

On 10/28/2015 04:35 PM, Mathias L. Baumann wrote:

I was using the latest git version 2.6.2 already.
I suspect it is due to a .gitconfig. This is what is probably required:

➜  ~  cat .gitconfig
[diff]
submodule = log

Yep, that did the trick.

The segfault is from

sha1_file.c:

/* add the alternate entry */
 *alt_odb_tail = ent; /* <= alt_obd_tail is NULL here */
alt_odb_tail = &(ent->next);
ent->next = NULL;

Will try to take a closer look at it.

--
Victor
--
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 v4 0/2] Fix interactive rebase when the editor saves with CR/LF

[Johannes Schindelin]

Chad Boles reported that `git rebase -i` recently started producing
errors when the editor saves files with DOS line endings. The symptom
is:

Warning: the command isn't recognized in the following line:
 -

You can fix this with 'git rebase --edit-todo'.
Or you can abort the rebase with 'git rebase --abort'.

The real bummer is that simply calling `git rebase --continue` "fixes"
it.

Turns out that we now check whether a single Carriage Return is a valid
command. This new check was introduced recently (1db168ee9, ironically
named "rebase-i: loosen over-eager check_bad_cmd check").

The fix provided by Junio works around this issue by testing for an
explicit trailing carriage return and handles it like an empty line.

Unfortunately, this is the best we can do for now as there is
disagreement about a more general fix.

This iteration clarifies the comments in git-rebase--interactive,
updates the commit message to state that this has been tested with Git
for Windows, and replaces the description of the proposed fix with a
description of the actual work-around provided by Junio.


Johannes Schindelin (1):
  Demonstrate rebase fails when the editor saves with CR/LF

Junio C Hamano (1):
  rebase-i: work around Windows CRLF line endings

 git-rebase--interactive.sh| 12 
 t/t3404-rebase-interactive.sh | 12 
 2 files changed, 24 insertions(+)

Interdiff vs v3:

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index daadf2d..34cfe66 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,8 +77,7 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
-# Work around a Windows port of shell that does not strip
-# the newline at the end of a line correctly.
+# Work around Git for Windows' Bash that strips only LFs but no CRs.
 cr=$(printf "\015")
 
 strategy_args=
@@ -523,8 +522,8 @@ do_next () {
mark_action_done
;;
"$cr")
-   # Windows port of shell not stripping the newline
-   # at the end of an empty line correctly.
+   # Work around Carriage Returns not being stripped (e.g. with
+   # Git for Windows' Bash).
mark_action_done
;;
pick|p)
@@ -906,8 +905,8 @@ check_bad_cmd_and_sha () {
# Doesn't expect a SHA-1
;;
"$cr")
-   # Windows port of shell not stripping the newline
-   # at the end of an empty line correctly.
+   # Work around Carriage Returns not being stripped
+   # (e.g. with Git for Windows' Bash).
;;
pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
if ! check_commit_sha "${rest%%[]*}" "$lineno" 
"$1"

-- 
2.1.4

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

[PATCH v4 2/2] rebase-i: work around Windows CRLF line endings

[Johannes Schindelin]

From: Junio C Hamano 

Editors on Windows can and do save text files with CRLF line
endings, which is the convention on the platform.  We are seeing
reports that the "read" command in a port of bash to the environment
however does not strip the CRLF at the end, not adjusting for the
same convention on the platform.

This breaks the recently added sanity checks for the insn sheet fed
to "rebase -i"; instead of an empty line (hence nothing in $command),
the script was getting a lone CR in there.

Special case a lone CR and treat it the same way as an empty line to
work this around.

This patch passes the test with Git for Windows, where the issue was
seen first.

Signed-off-by: Junio C Hamano 
Signed-off-by: Johannes Schindelin 
---
 git-rebase--interactive.sh| 12 
 t/t3404-rebase-interactive.sh |  2 +-
 2 files changed, 13 insertions(+), 1 deletion(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d65c06e..34cfe66 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -77,6 +77,9 @@ amend="$state_dir"/amend
 rewritten_list="$state_dir"/rewritten-list
 rewritten_pending="$state_dir"/rewritten-pending
 
+# Work around Git for Windows' Bash that strips only LFs but no CRs.
+cr=$(printf "\015")
+
 strategy_args=
 if test -n "$do_merge"
 then
@@ -518,6 +521,11 @@ do_next () {
"$comment_char"*|''|noop|drop|d)
mark_action_done
;;
+   "$cr")
+   # Work around Carriage Returns not being stripped (e.g. with
+   # Git for Windows' Bash).
+   mark_action_done
+   ;;
pick|p)
comment_for_reflog pick
 
@@ -896,6 +904,10 @@ check_bad_cmd_and_sha () {
"$comment_char"*|''|noop|x|exec)
# Doesn't expect a SHA-1
;;
+   "$cr")
+   # Work around Carriage Returns not being stripped
+   # (e.g. with Git for Windows' Bash).
+   ;;
pick|p|drop|d|reword|r|edit|e|squash|s|fixup|f)
if ! check_commit_sha "${rest%%[]*}" "$lineno" 
"$1"
then
diff --git a/t/t3404-rebase-interactive.sh b/t/t3404-rebase-interactive.sh
index 5dfa16a..98eb49a 100755
--- a/t/t3404-rebase-interactive.sh
+++ b/t/t3404-rebase-interactive.sh
@@ -1261,7 +1261,7 @@ test_expect_success 'static check of bad SHA-1' '
test E = $(git cat-file commit HEAD | sed -ne \$p)
 '
 
-test_expect_failure 'editor saves as CR/LF' '
+test_expect_success 'editor saves as CR/LF' '
git checkout -b with-crlf &&
write_script add-crs.sh <<-\EOF &&
sed -e "s/\$/Q/" <"$1" | tr Q "\\015" >"$1".new &&
-- 
2.1.4
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] add_submodule_odb: initialize alt_odb list earlier

[Junio C Hamano]

Jeff King  writes:

> Yeah, I can reproduce it easily with that. Thanks for providing the
> repository. It takes a rather convoluted set of conditions to trigger
> the bug. :)
>
> Here's the fix:
>
> -- >8 --
> Subject: add_submodule_odb: initialize alt_odb list earlier
>
> The add_submodule_odb function tries to add a submodule's
> object store as an "alternate". It needs the existing list
> to be initialized (from the objects/info/alternates file)
> for two reasons:
>
>   1. We look for duplicates with the existing alternate
>  stores, but obviously this doesn't work if we haven't
>  loaded any yet.
>
>   2. We link our new entry into the list by prepending it to
>  alt_odb_list. But we do _not_ modify alt_odb_tail.
>  This variable starts as NULL, and is a signal to the
>  alt_odb code that the list has not yet been
>  initialized.
>
>  We then call read_info_alternates on the submodule (to
>  recursively load its alternates), which will try to
>  append to that tail, assuming it has been initialized.
>  This causes us to segfault if it is NULL.
>
> This rarely comes up in practice, because we will have
> initialized the alt_odb any time we do an object lookup. So
> you can trigger this only when:
>
>   - you try to access a submodule (e.g., a diff with
> diff.submodule=log)
>
>   - the access happens before any other object has been
> accessed (e.g., because the diff is between the working
> tree and the index)
>
>   - the submodule contains an alternates file (so we try to
> add an entry to the NULL alt_odb_tail)
>
> To fix this, we just need to call prepare_alt_odb at the
> start of the function (and if we have already initialized,
> it is a noop).
>
> Note that we can remove the prepare_alt_odb call from the
> end. It is guaranteed to be a noop, since we will have
> called it earlier.

Thanks for a quick and detailed diagnosis and a fix.

The removal is correct, but even without this fix, the order of
calls in the original should have screamed "bug" loudly at us, I
think.  We shouldn't be reading data from alternates file without
first preparing the place we read data into.

> Signed-off-by: Jeff King 
> ---
>  submodule.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/submodule.c b/submodule.c
> index 5879cfb..88af54c 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -130,6 +130,7 @@ static int add_submodule_odb(const char *path)
>   goto done;
>   }
>   /* avoid adding it twice */
> + prepare_alt_odb();
>   for (alt_odb = alt_odb_list; alt_odb; alt_odb = alt_odb->next)
>   if (alt_odb->name - alt_odb->base == objects_directory.len &&
>   !strncmp(alt_odb->base, objects_directory.buf,
> @@ -148,7 +149,6 @@ static int add_submodule_odb(const char *path)
>  
>   /* add possible alternates from the submodule */
>   read_info_alternates(objects_directory.buf, 0);
> - prepare_alt_odb();
>  done:
>   strbuf_release(_directory);
>   return ret;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: git repack command on larger pack file

[Junio C Hamano]

Jeff King  writes:

> Git tries to take some shortcuts when repacking: if two objects are in
> the same pack but not deltas, it will not consider making deltas out of
> them. The logic is we would already have tried that while making the
> original pack. But of course when you are doing weird things with the
> packing parameters, that is not always a good assumption.

Yup, that is 
http://thread.gmane.org/gmane.comp.version-control.git/16223/focus=16267

> [1] This is all theory, and I don't know how well git actually finds
> such deltas, but it is probably better to have a dense tree of
> deltas rather than long chains.  If you have a chain of N objects
> and would to add object N+1 to it, you are probably not much worse
> off to base it on object N-1, creating a "fork" at N.

Yes, your guess is perfectly correct here, and indeed we did an
extensive work along that line in 2006/2007.  For an example, see
http://thread.gmane.org/gmane.comp.version-control.git/51949/focus=52003

The histogram "verify-pack -v" produces was in fact done primarily
in order to make it easy to check the distribution of delta depth.

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: Why are submodules not automatically handled by default or at least configurable to do so?

[Nazri Ramliy]

On Tue, Oct 27, 2015 at 12:56 AM, Jens Lehmann  wrote:
> Which seems a bit error prone as you could forget to update the submodules
> and build incorrect rpms from them, or am I missing something?

For my case I'm not building the rpms directly after merging in the fixes
done in the octopus branch, so I don't care much about the state of the
submodules after the merge since I know that the octopus branch does
not modify any submodules. The rpms are built later on, possibly on
another machine where the submodules are updated w.r.t to each branch
in a release commit.

> I understand why you don't need to update the submodules every time, but
> would it hurt your workflow if they did (but don't get me wrong, that will
> always be configurable).

I'd say it depends - for the times when all I want to do is work on plain files
in the superproject (on an octopus branch for example) I don't want git to
automatically update the submodules everytime I switch branches - it would
hurt my productivity as there will be more disk activities due to files being
checked out unnecessarily for updating the submodules.

The submodule states are not committed that often into the superproject,
they are done normally when we're cutting a new release. During day-to-day
development each developer runs a script that pulls in the latest commits
for "hot" submodules.

git not updating the submodule state automatically is actually a convenient
for my particular use case here.

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

Re: git repack command on larger pack file

[Junio C Hamano]

Junio C Hamano  writes:

>> [1] This is all theory, and I don't know how well git actually finds
>> such deltas, but it is probably better to have a dense tree of
>> deltas rather than long chains.  If you have a chain of N objects
>> and would to add object N+1 to it, you are probably not much worse
>> off to base it on object N-1, creating a "fork" at N.
>
> Yes, your guess is perfectly correct here, and indeed we did an
> extensive work along that line in 2006/2007.  For an example, see
> http://thread.gmane.org/gmane.comp.version-control.git/51949/focus=52003

And here is another, which is probably one of the most important
thread on pack-objects, before the bitmap was introduced:

http://thread.gmane.org/gmane.comp.version-control.git/20056/focus=20134

--
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/RFC] receive-pack: allow for hiding refs outside the namespace

[Lukas Fleischer]

On Tue, 27 Oct 2015 at 19:18:26, Junio C Hamano wrote:
> [...]
> When I asked 'Is transfer.hiderefs insufficient?', I wasn't
> expecting it to be usable out of box.  It was a suggestion to build
> on top of it, instead of adding a parallel support for something
> specific to namespaces.
> 

Agreed, and I do have a couple of patches to improve hideRefs. I still
have some questions before submitting them, though. See below.

> For example, if the problem is that you cannot tell ref_is_hidden()
> what namespace the ref is from because it is called after running
> strip_namespace(), perhaps you can find a way to have the original
> "namespaced ref" specified on transfer.hiderefs and match them?
> Then in repository for project A, namespaced refs for project B can
> be excluded by specifying refs/namespaces/B/* on transfer.hiderefs.
> 
> Perhaps along the lines of this?
> [...]

My original question remains: Do we want to continue supporting things
like transfer.hideRefs=.have (which currently magically hides all refs
outside the current namespace)? For 100% backwards compatibility, we
would have to. On the other hand, one could consider the current
behavior a bug and one could argue that it is weird enough that probably
nobody (apart from me) relies on it right now. If we decide to keep it
anyway, I think it should be documented.

Another patch I have in my patch queue adds support for a whitelist mode
to hideRefs. There are several ways to implement that:

1. Make transfer.hideRefs='' hide all refs (it currently does not). The
   user can then whitelist refs explicitly using negative patterns
   below that rule. This is how my current implementation works. Using
   the empty string seemed most natural since hideRefs matches prefixes
   and every string has the empty string as a prefix. If that seems too
   weird, we could probably special case something like
   transfer.hideRefs='*' instead.

2. Detect whether hideRefs only contains negative patterns. Switch to
   whitelist mode ("hide by default") in that case.

3. Add another option to switch between "hide by default" and "show by
   default".

I personally prefer the first option. Any other opinions?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 1/2] http: allow selection of proxy authentication method

[Knut Franke]

CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
with a 307 redirect to an error page instead of a 407 listing supported
authentication methods. Therefore, allow the authentication method to be set
using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
variables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy
to http.proxy and remote..proxy).

The following values are supported:

* anyauth (default)
* basic
* digest
* negotiate
* ntlm

Signed-off-by: Knut Franke 
---
 Documentation/config.txt | 28 ++
 http.c   | 62 +---
 remote.c |  3 +++
 remote.h |  1 +
 4 files changed, 91 insertions(+), 3 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..f2644d1 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1597,6 +1597,29 @@ http.proxy::
`curl(1)`).  This can be overridden on a per-remote basis; see
remote..proxy
 
+http.proxyAuthmethod::
+   Set the method with which to authenticate against the HTTP proxy. This 
only
+takes effect if the configured proxy URI contains a user name part (i.e. is
+of the form 'user@host' or 'user@host:port'). This can be overridden on a
+per-remote basis; see `remote..proxyAuthmethod`. Both can be
+overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
+Possible values are:
++
+--
+* `anyauth` - Automatically pick a suitable authentication method. It is
+  assumed that the proxy answers an unauthenticated request with a 407
+  status code and one or more Proxy-authenticate headers with supported
+  authentication methods. This is the default.
+* `basic` - HTTP Basic authentication
+* `digest` - HTTP Digest authentication; this prevents the password from being
+  transmitted to the proxy in clear text
+* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option
+  of `curl(1)`)
+* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
+--
++
+
+
 http.cookieFile::
File containing previously stored cookie lines which should be used
in the Git http session, if they match the server. The file format
@@ -2390,6 +2413,11 @@ remote..proxy::
the proxy to use for that remote.  Set to the empty string to
disable proxying for that remote.
 
+remote..proxyAuthmethod::
+For remotes that require curl (http, https and ftp), the method to use for
+authenticating against the proxy in use (probably set in
+`remote..proxy`). See `http.proxyAuthmethod`.
+
 remote..fetch::
The default set of "refspec" for linkgit:git-fetch[1]. See
linkgit:git-fetch[1].
diff --git a/http.c b/http.c
index 7da76ed..4756bab 100644
--- a/http.c
+++ b/http.c
@@ -63,6 +63,22 @@ static long curl_low_speed_limit = -1;
 static long curl_low_speed_time = -1;
 static int curl_ftp_no_epsv;
 static const char *curl_http_proxy;
+static const char *http_proxy_authmethod = NULL;
+static struct {
+   const char *name;
+   long curlauth_param;
+} http_proxy_authmethods[] = {
+   { "basic", CURLAUTH_BASIC },
+   { "digest", CURLAUTH_DIGEST },
+   { "negotiate", CURLAUTH_GSSNEGOTIATE },
+   { "ntlm", CURLAUTH_NTLM },
+#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
+   { "anyauth", CURLAUTH_ANY },
+#endif
+   // CURLAUTH_DIGEST_IE has no corresponding command-line option in
+   // curl(1) and is not included in CURLAUTH_ANY, so we leave it out
+   // here, too
+};
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -257,6 +273,9 @@ static int http_options(const char *var, const char *value, 
void *cb)
if (!strcmp("http.proxy", var))
return git_config_string(_http_proxy, var, value);
 
+   if (!strcmp("http.proxyauthmethod", var))
+   return git_config_string(_proxy_authmethod, var, value);
+
if (!strcmp("http.cookiefile", var))
return git_config_string(_cookie_file, var, value);
if (!strcmp("http.savecookies", var)) {
@@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)
 #endif
 }
 
+static void copy_from_env(const char **var, const char *envname)
+{
+   const char *val = getenv(envname);
+   if (val)
+   *var = xstrdup(val);
+}
+
+static void init_curl_proxy_auth(CURL *result)
+{
+   copy_from_env(_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
+
+   if (http_proxy_authmethod) {
+   int i;
+   for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
+   if (!strcmp(http_proxy_authmethod, 
http_proxy_authmethods[i].name)) {
+   curl_easy_setopt(result, CURLOPT_PROXYAUTH,
+   

[PATCH 2/2] http: use credential API to handle proxy authentication

[Knut Franke]

Currently, the only way to pass proxy credentials to curl is by including them
in the proxy URL. Usually, this means they will end up on disk unencrypted, one
way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
proxy authentication often uses a domain user, credentials can be security
sensitive; therefore, a safer way of passing credentials is desirable.

If the configured proxy contains a username but not a password, query the
credential API for one. Also, make sure we approve/reject proxy credentials
properly.

For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
environment variables, which would otherwise be evaluated as a fallback by curl.
Without this, we would have different semantics for git configuration and
environment variables.

Signed-off-by: Knut Franke 
---
 http.c | 63 ++-
 http.h |  1 +
 2 files changed, 63 insertions(+), 1 deletion(-)

diff --git a/http.c b/http.c
index 4756bab..11bebe1 100644
--- a/http.c
+++ b/http.c
@@ -79,6 +79,7 @@ static struct {
// curl(1) and is not included in CURLAUTH_ANY, so we leave it out
// here, too
 };
+struct credential http_proxy_auth = CREDENTIAL_INIT;
 static const char *curl_cookie_file;
 static int curl_save_cookies;
 struct credential http_auth = CREDENTIAL_INIT;
@@ -176,6 +177,9 @@ static void finish_active_slot(struct active_request_slot 
*slot)
 #else
slot->results->auth_avail = 0;
 #endif
+
+   curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
+   >results->http_connectcode);
}
 
/* Run callback if appropriate */
@@ -333,6 +337,25 @@ static void copy_from_env(const char **var, const char 
*envname)
 
 static void init_curl_proxy_auth(CURL *result)
 {
+   if (http_proxy_auth.username) {
+   if (!http_proxy_auth.password) {
+   credential_fill(_proxy_auth);
+   }
+#if LIBCURL_VERSION_NUM >= 0x071301
+   curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
+   http_proxy_auth.username);
+   curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
+   http_proxy_auth.password);
+#else
+   struct strbuf up = STRBUF_INIT;
+   strbuf_reset();
+   strbuf_addstr_urlencode(, http_proxy_auth.username, 1);
+   strbuf_addch(, ':');
+   strbuf_addstr_urlencode(, http_proxy_auth.password, 1);
+   curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);
+#endif
+   }
+
copy_from_env(_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
 
if (http_proxy_authmethod) {
@@ -513,8 +536,36 @@ static CURL *get_curl_handle(void)
curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
 #endif
 
+   /*
+* curl also examines these variables as a fallback; but we need to 
query
+* them here in order to decide whether to prompt for missing password 
(cf.
+* init_curl_proxy_auth()).
+*/
+   if (!curl_http_proxy) {
+   if (!strcmp(http_auth.protocol, "https")) {
+   copy_from_env(_http_proxy, "HTTPS_PROXY");
+   copy_from_env(_http_proxy, "https_proxy");
+   } else {
+   copy_from_env(_http_proxy, "http_proxy");
+   }
+   if (!curl_http_proxy) {
+   copy_from_env(_http_proxy, "ALL_PROXY");
+   copy_from_env(_http_proxy, "all_proxy");
+   }
+   }
+
if (curl_http_proxy) {
-   curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
+   if (strstr(curl_http_proxy, "://"))
+   credential_from_url(_proxy_auth, curl_http_proxy);
+   else {
+   struct strbuf url = STRBUF_INIT;
+   strbuf_reset();
+   strbuf_addstr(, "http://;);
+   strbuf_addstr(, curl_http_proxy);
+   credential_from_url(_proxy_auth, url.buf);
+   }
+
+   curl_easy_setopt(result, CURLOPT_PROXY, http_proxy_auth.host);
}
init_curl_proxy_auth(result);
 
@@ -658,6 +709,12 @@ void http_cleanup(void)
curl_http_proxy = NULL;
}
 
+   if (http_proxy_auth.password) {
+   memset(http_proxy_auth.password, 0, 
strlen(http_proxy_auth.password));
+   free(http_proxy_auth.password);
+   http_proxy_auth.password = NULL;
+   }
+
if (http_proxy_authmethod) {
free((void *)http_proxy_authmethod);
http_proxy_authmethod = NULL;
@@ -991,6 +1048,8 @@ static int handle_curl_result(struct slot_results *results)
 
if (results->curl_result == CURLE_OK) {
credential_approve(_auth);
+   

[PATCH v2] http proxy authentication improvements

[Knut Franke]

Fixes in the second iteration:

* rename http.proxy-authmethod to http.proxyAuthmethod for consistency with
  other core git variables

* issue warning() instead of error() for unsupported authentication method, for
  consistency with http.sslVersion

* fix some code formatting / style issues

* fix memory management bug
-- 
Vorstandsvorsitzender/Chairman of the board of management:
Gerd-Lothar Leonhart
Vorstand/Board of Management:
Dr. Bernd Finkbeiner, Dr. Arno Steitz
Vorsitzender des Aufsichtsrats/
Chairman of the Supervisory Board:
Philippe Miltin
Sitz/Registered Office: Tuebingen
Registergericht/Registration Court: Stuttgart
Registernummer/Commercial Register No.: HRB 382196

--
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/RFC] receive-pack: allow for hiding refs outside the namespace

[Junio C Hamano]

Lukas Fleischer  writes:

> Another patch I have in my patch queue adds support for a whitelist mode
> to hideRefs. There are several ways to implement that:
>
> 1. Make transfer.hideRefs='' hide all refs (it currently does not). The

Hmph, that even sounds like a bug.  parse_hide_refs_config() does
not seem to reject ref[] whose length is zero, and ref_is_hidden()
would just check "starts_with(refname, match)" with an empty string
as "match", so I would naively have expected that to work already.

Ahh, there is "if refname[len] is at the end or slash boundary"
check after that.  You're right--you'd need to tweak that one for it
to work.

>user can then whitelist refs explicitly using negative patterns
>below that rule. This is how my current implementation works.

That sounds like a good way to go.

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: What's the ".git/gitdir" file?

[Junio C Hamano]

Mike Rappazzo  writes:

> On Tue, Oct 27, 2015 at 6:54 PM, Junio C Hamano  wrote:
>> Kyle Meyer  writes:
>>
>>> When a ".git" file points to another repo, a ".git/gitdir" file is
>>> created in that repo.
>>>
>>> For example, running
>>>
>>> $ mkdir repo-a repo-b
>>> $ cd repo-a
>>> $ git init
>>> $ cd ../repo-b
>>> $ echo "gitdir: ../repo-a/.git" > .git
>>> $ git status
>>>
>>> results in a file "repo-a/.git/gitdir" that contains
>>>
>>> $ cat repo-a/.git/gitdir
>>> .git
>>
>> Sounds like a bug in the recently added "worktree" stuff.  Perhaps
>> update_linked_gitdir() tweaked by 82fde87f (setup: update the right
>> file in multiple checkouts, 2015-08-25) is misbehaving?
>
> I noticed that as I was working on the worktree list command that my
> linked worktree gitdir files were being clobbered to '.git'.  I
> attributed it to my work, but now that you mention it, I think it has
> happened with the 2.6.1 release as well.

Thanks; I trust those who worked on the worktree feature in 2.6
timeframe would first take a look, OK?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Allow hideRefs to match refs outside the namespace

[Lukas Fleischer]

Right now, refs with a path outside the current namespace are replaced
by ".have" before passing them to show_ref() which in turn checks
whether the ref matches the hideRefs pattern. Move the check before the
path substitution in show_ref_cb() such that the hideRefs feature can be
used to hide specific refs outside the current namespace.

Signed-off-by: Lukas Fleischer 
---
The other show_ref() call sites are in show_one_alternate_sha1() and in
write_head_info(). The call site in show_one_alternate_sha1() is for
alternates and passes ".have". The other one is

show_ref("capabilities^{}", null_sha1);

and is not relevant to the hideRefs feature. Note that this kind of
breaks backwards compatibility since the "magic" hideRefs patterns
".have" and "capabilities^{}" no longer work, as explained in the
discussion.

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

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index bcb624b..4a5d0ae 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -195,9 +195,6 @@ static int receive_pack_config(const char *var, const char 
*value, void *cb)
 
 static void show_ref(const char *path, const unsigned char *sha1)
 {
-   if (ref_is_hidden(path))
-   return;
-
if (sent_capabilities) {
packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
} else {
@@ -221,6 +218,9 @@ static void show_ref(const char *path, const unsigned char 
*sha1)
 
 static int show_ref_cb(const char *path, const struct object_id *oid, int 
flag, void *unused)
 {
+   if (ref_is_hidden(path))
+   return 0;
+
path = strip_namespace(path);
/*
 * Advertise refs outside our current namespace as ".have"
-- 
2.6.2

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

Re: [PATCH] Allow hideRefs to match refs outside the namespace

[Junio C Hamano]

Lukas Fleischer  writes:

> Right now, refs with a path outside the current namespace are replaced
> by ".have" before passing them to show_ref() which in turn checks
> whether the ref matches the hideRefs pattern. Move the check before the
> path substitution in show_ref_cb() such that the hideRefs feature can be
> used to hide specific refs outside the current namespace.
>
> Signed-off-by: Lukas Fleischer 
> ---
> The other show_ref() call sites are in show_one_alternate_sha1() and in
> write_head_info(). The call site in show_one_alternate_sha1() is for
> alternates and passes ".have". The other one is
>
> show_ref("capabilities^{}", null_sha1);
>
> and is not relevant to the hideRefs feature. Note that this kind of
> breaks backwards compatibility since the "magic" hideRefs patterns
> ".have" and "capabilities^{}" no longer work, as explained in the
> discussion.

If somebody is using namespaces and has "refs/frotz/" in the
hiderefs configuration, we hide refs/frotz/ no matter which
namespace is being accessed.  With this change, with the removal the
check from show_ref(), wouldn't such a repository suddenly see a
behaviour change?

>  builtin/receive-pack.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
> index bcb624b..4a5d0ae 100644
> --- a/builtin/receive-pack.c
> +++ b/builtin/receive-pack.c
> @@ -195,9 +195,6 @@ static int receive_pack_config(const char *var, const 
> char *value, void *cb)
>  
>  static void show_ref(const char *path, const unsigned char *sha1)
>  {
> - if (ref_is_hidden(path))
> - return;
> -
>   if (sent_capabilities) {
>   packet_write(1, "%s %s\n", sha1_to_hex(sha1), path);
>   } else {
> @@ -221,6 +218,9 @@ static void show_ref(const char *path, const unsigned 
> char *sha1)
>  
>  static int show_ref_cb(const char *path, const struct object_id *oid, int 
> flag, void *unused)
>  {
> + if (ref_is_hidden(path))
> + return 0;
> +
>   path = strip_namespace(path);
>   /*
>* Advertise refs outside our current namespace as ".have"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Junio C Hamano]

Knut Franke  writes:

> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy
> to http.proxy and remote..proxy).
>
> The following values are supported:
>
> * anyauth (default)
> * basic
> * digest
> * negotiate
> * ntlm
>
> Signed-off-by: Knut Franke 
> ---

Thanks.

> +http.proxyAuthmethod::
> + Set the method with which to authenticate against the HTTP proxy. This 
> only
> +takes effect if the configured proxy URI contains a user name part (i.e. 
> is
> +of the form 'user@host' or 'user@host:port'). This can be overridden on a
> +per-remote basis; see `remote..proxyAuthmethod`. Both can be
> +overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
> +Possible values are:

I see inconsistent indentation here.  Indent with a tab, like you
did your first line, consistently up to this point, perhaps?

> @@ -2390,6 +2413,11 @@ remote..proxy::
>   the proxy to use for that remote.  Set to the empty string to
>   disable proxying for that remote.
>  
> +remote..proxyAuthmethod::
> +For remotes that require curl (http, https and ftp), the method to use 
> for
> +authenticating against the proxy in use (probably set in
> +`remote..proxy`). See `http.proxyAuthmethod`.
> +

Likewise (match the style of the surrounding paragraphs).

> diff --git a/http.c b/http.c
> index 7da76ed..4756bab 100644
> --- a/http.c
> +++ b/http.c
> @@ -63,6 +63,22 @@ static long curl_low_speed_limit = -1;
>  static long curl_low_speed_time = -1;
>  static int curl_ftp_no_epsv;
>  static const char *curl_http_proxy;
> +static const char *http_proxy_authmethod = NULL;
> +static struct {
> + const char *name;
> + long curlauth_param;
> +} http_proxy_authmethods[] = {

Perhaps call this "proxy_authmethod[]"?  We won't be talking about
any other kinds of proxy authentication method in http.c file, and a
long name like this makes the line unnecessarily long in a nested
loop like you added to init_curl_proxy_auth().

> + { "basic", CURLAUTH_BASIC },
> + { "digest", CURLAUTH_DIGEST },
> + { "negotiate", CURLAUTH_GSSNEGOTIATE },
> + { "ntlm", CURLAUTH_NTLM },
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> + { "anyauth", CURLAUTH_ANY },
> +#endif
> + // CURLAUTH_DIGEST_IE has no corresponding command-line option in
> + // curl(1) and is not included in CURLAUTH_ANY, so we leave it out
> + // here, too
> +};

Please do not use // C++ comments.

> @@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)
>  #endif
>  }
>  
> +static void copy_from_env(const char **var, const char *envname)
> +{
> + const char *val = getenv(envname);
> + if (val)
> + *var = xstrdup(val);
> +}
> +
> +static void init_curl_proxy_auth(CURL *result)
> +{
> + copy_from_env(_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");

Unless this helper is used regularly from many other places, is use
makes it harder to follow the flow of the logic, as it does not
offer clear and obvious abstraction, especially with the name
"copy_from_env()".  I was forced to look at the implementation to
see what happens when the environment variable does not exist to
make sure the right thing happens (i.e. http_proxy_authmethod is
unchanged).

> + if (http_proxy_authmethod) {
> + int i;
> + for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> + if (!strcmp(http_proxy_authmethod, 
> http_proxy_authmethods[i].name)) {
> + curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> + 
> http_proxy_authmethods[i].curlauth_param);
> + break;
> + }
> + }
> + if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> + warning("unsupported proxy authentication method %s: 
> using default",
> +   http_proxy_authmethod);
> + }
> + }
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> + else
> + curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}

This patch should take what 1c2dbf20 (http: support curl < 7.10.7,
2015-02-03) wanted to do into account.  Having the configuration
variable or the environment variable defined by itself, while
running a Git built with old cURL, shouldn't trigger any warning,
but the entire function should perhaps be ifdefed out or something?

Personally I find it a bit surprising that somebody still cares
about such an old version (7.10.7 is listed on 15 Aug 2003 in the
CHANGES file), but there apparently are happy 

Re: [PATCH v2] format-patch: introduce format.outputDirectory configuration

[Junio C Hamano]

Junio C Hamano  writes:

> Looks like there were mostly editorial niggles and no fundamental
> flaws in the design of the patch; it is somewhat a shame to make all
> the efforts go to waste.  Will we be seeing an update soon?

Second ping as I am going through the what's cooking reports and
trying to decide which topics listed in [Stalled] state need to be
discarded from my tree.

Not that my dropping a topic from 'pu' means very much (a dropped
topic can still be submitted and requeued after all), even if you
are no longer interested on the topic, hearing from you would help
others who may be interested in helping the topic to completion.

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

A call for Charity

[Mrs]

My Name is Mrs Jessica Peterside from United Kingdom who is suffering from 
cancer of the lungs I wish to use my fund to serve the poor in your country 
contact me via email ( mrsjessicapeters...@gmail.com )
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] http: allow selection of proxy authentication method

[Eric Sunshine]

In addition to Junio's review comments...

On Wednesday, October 28, 2015, Knut Franke
 wrote:
> CURLAUTH_ANY does not work with proxies which answer unauthenticated requests
> with a 307 redirect to an error page instead of a 407 listing supported
> authentication methods. Therefore, allow the authentication method to be set
> using the environment variable GIT_HTTP_PROXY_AUTHMETHOD or configuration
> variables http.proxyAuthmethod and remote..proxyAuthmethod (in analogy
> to http.proxy and remote..proxy).
>
> The following values are supported:
>
> * anyauth (default)
> * basic
> * digest
> * negotiate
> * ntlm
>
> Signed-off-by: Knut Franke 
> ---
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 391a0c3..f2644d1 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -1597,6 +1597,29 @@ http.proxy::
> `curl(1)`).  This can be overridden on a per-remote basis; see
> remote..proxy
>
> +http.proxyAuthmethod::

Should this be typed as "proxyAuthMethod"?

> +   Set the method with which to authenticate against the HTTP proxy. 
> This only
> +takes effect if the configured proxy URI contains a user name part (i.e. 
> is
> +of the form 'user@host' or 'user@host:port'). This can be overridden on a
> +per-remote basis; see `remote..proxyAuthmethod`. Both can be
> +overridden by the 'GIT_HTTP_PROXY_AUTHMETHOD' environment variable.
> +Possible values are:
> ++
> +--
> +* `anyauth` - Automatically pick a suitable authentication method. It is
> +  assumed that the proxy answers an unauthenticated request with a 407
> +  status code and one or more Proxy-authenticate headers with supported
> +  authentication methods. This is the default.
> +* `basic` - HTTP Basic authentication
> +* `digest` - HTTP Digest authentication; this prevents the password from 
> being
> +  transmitted to the proxy in clear text
> +* `negotiate` - GSS-Negotiate authentication (compare the --negotiate option
> +  of `curl(1)`)
> +* `ntlm` - NTLM authentication (compare the --ntlm option of `curl(1)`)
> +--

I think you can drop the unnecessary '--' lines here and above.

> ++
> +

No need for the extra unnecessary "+" line and empty line.

> +
>  http.cookieFile::
> File containing previously stored cookie lines which should be used
> in the Git http session, if they match the server. The file format
> diff --git a/http.c b/http.c
> index 7da76ed..4756bab 100644
> --- a/http.c
> +++ b/http.c
> @@ -305,6 +324,37 @@ static void init_curl_http_auth(CURL *result)
> +static void init_curl_proxy_auth(CURL *result)
> +{
> +   copy_from_env(_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
> +
> +   if (http_proxy_authmethod) {
> +   int i;
> +   for (i = 0; i < ARRAY_SIZE(http_proxy_authmethods); i++) {
> +   if (!strcmp(http_proxy_authmethod, 
> http_proxy_authmethods[i].name)) {
> +   curl_easy_setopt(result, CURLOPT_PROXYAUTH,
> +   
> http_proxy_authmethods[i].curlauth_param);
> +   break;
> +   }
> +   }
> +   if (i == ARRAY_SIZE(http_proxy_authmethods)) {
> +   warning("unsupported proxy authentication method %s: 
> using default",
> + http_proxy_authmethod);

Does the user know what "default" means here? Does it mean
CURLAUTH_ANY? If so, do you need to invoke curl_easy_setopt(...,
CURLAUTH_ANY) as you do below when http_proxy is NULL?

> +   }
> +   }
> +#ifdef LIBCURL_CAN_HANDLE_AUTH_ANY
> +   else
> +   curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> +#endif
> +}
> +
>  static int has_cert_password(void)
>  {
> if (ssl_cert == NULL || ssl_cert_password_required != 1)
> @@ -466,9 +516,7 @@ static CURL *get_curl_handle(void)
> if (curl_http_proxy) {
> curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> }
> -#if LIBCURL_VERSION_NUM >= 0x070a07
> -   curl_easy_setopt(result, CURLOPT_PROXYAUTH, CURLAUTH_ANY);
> -#endif
> +   init_curl_proxy_auth(result);
>
> set_curl_keepalive(result);
>
> diff --git a/remote.c b/remote.c
> index 1101f82..426c6d8 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -427,6 +427,9 @@ static int handle_config(const char *key, const char 
> *value, void *cb)
> } else if (!strcmp(subkey, ".proxy")) {
> return git_config_string((const char **)>http_proxy,
>  key, value);
> +   } else if (!strcmp(subkey, ".proxyAuthmethod")) {

In documentation, write "proxyAuthMethod", but in code use
"proxyauthmethod" string literal.

> +   return git_config_string((const char 
> **)>http_proxy_authmethod,
> +

Re: [PATCH v2 2/2] object name: introduce '^{/!-}' notation

[Junio C Hamano]

Will Palmer  writes:

> In summary: it looks like I'll be sending another one.

Has anything happened to this topic since then?  I am asking
primarily because I want to decide if I should discard
wp/sha1-name-negative-match topic from my tree [*1*].

I think what it attempts to do is a worthy thing, and it is shame to
see the initial implementation and review cycles we have spent so
far go to waste.


[Footnote]

*1* Not that my dropping a topic from 'pu' means very much; a
dropped topic can still be submitted and requeued after all.
--
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] add_submodule_odb: initialize alt_odb list earlier

[Jeff King]

On Wed, Oct 28, 2015 at 08:24:17AM -0700, Junio C Hamano wrote:

> > Note that we can remove the prepare_alt_odb call from the
> > end. It is guaranteed to be a noop, since we will have
> > called it earlier.
> 
> Thanks for a quick and detailed diagnosis and a fix.
> 
> The removal is correct, but even without this fix, the order of
> calls in the original should have screamed "bug" loudly at us, I
> think.  We shouldn't be reading data from alternates file without
> first preparing the place we read data into.

Yeah, I agree. I spent a long time trying to figure out if that
prepare_alt_odb was actually doing something useful (like if it was
needed to somehow "cement" the new alt into place).

But I don't think it was.

In the majority of cases, it was a noop (we had already prepared when we
looked up the first object). But for other cases...

  - if read_info_alternates actually did something, we segfaulted (i.e.,
this bug)

  - otherwise, we would prepare on _top_ of what we just added to the
list, which was probably buggy (I didn't dig far enough to see if
prepare_alt_odb() would overwrite what we just added to the list).

So some pretty dark corners of the code. :)

-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 0/3] bash-completion fixes for global git options handling

[Peter Wu]

Hi,

These patches improve bash-completion when global git options are present.
Consider this problem:

bash-4.3$ git -C ../linux staerror: invalid key: alias.../linux

This happens because the current script is unaware of the -C option. In general,
global options are not handled well (patch 001 fixes this).

Patch 2 makes the completions more aware of the --git-dir option.

Patch 3 builds on previous patches and makes completions aware of the -C option.

Kind regards,
Peter

Peter Wu (3):
  completion: ignore git options for subcommand completion
  completion: pass --git-dir to more commands
  completion: handle git -C option

 contrib/completion/git-completion.bash | 119 -
 1 file changed, 72 insertions(+), 47 deletions(-)

-- 
2.6.1

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

Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory

[Junio C Hamano]

Junio C Hamano  writes:

> Eric Sunshine  writes:
>
>>> -static void real_report_garbage(const char *desc, const char *path)
>>> +const char *bits_to_msg(unsigned seen_bits)
>>
>> If you don't expect other callers outside this file, then this should
>> be declared 'static'. If you do expect future external callers, then
>> this should be declared in a public header file (but renamed to be
>> more meaningful).
>
> I think this can be private to this file.  The sole point of moving
> this logic to this file is to make it private, after all ;-)  Thanks
> for sharp eyes.
>
> Together with the need for a description on "why", this probably
> deserves a test or two, probably at the end of t5304.
>
> Thanks.

Does somebody want to help tying the final loose ends on this topic?
It has been listed in the [Stalled] section for too long, I _think_
what it attempts to do is a worthy thing, and it is shame to see the
initial implementation and review cycles we have spent so far go to
waste.

If I find nothing else to do before any taker appears, I could
volunteer myself, but thought I should ask first.

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 v4 0/2] Fix interactive rebase when the editor saves with CR/LF

[Junio C Hamano]

Johannes Schindelin  writes:

> Turns out that we now check whether a single Carriage Return is a valid
> command. This new check was introduced recently (1db168ee9, ironically
> named "rebase-i: loosen over-eager check_bad_cmd check").

Will queue.

The root cause is not really "a new check added recently".  Earlier
the edited result was fed to stripspace and because "stripspace"
does what a dos-to-unix filter that turns CRLF into LF in addition
to cleaning up spaces and comments, we did not see the problematic
behaviour from "read" in this codepath.

Thanks.

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

[PATCH 3/3] completion: handle git -C option

[Peter Wu]

Avoid the "fatal: bad config file line 5 in config" message and properly
complete git commands having the "-C" option.

Besides the trivial command parsing, __gitdir is rewritten to apply any
directory changes requested via `git -C otherdir ...`.

Signed-off-by: Peter Wu 
---
 contrib/completion/git-completion.bash | 45 +++---
 1 file changed, 31 insertions(+), 14 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index fdf0f16..1646f61 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -34,24 +34,39 @@ case "$COMP_WORDBREAKS" in
 esac
 
 # __gitdir accepts 0 or 1 arguments (i.e., location)
-# returns location of .git repo
+# outputs location of .git repo if it exists, nothing otherwise.
 __gitdir ()
 {
-   if [ -z "${1-}" ]; then
-   if [ -n "${__git_dir-}" ]; then
-   echo "$__git_dir"
-   elif [ -n "${GIT_DIR-}" ]; then
-   test -d "${GIT_DIR-}" || return 1
-   echo "$GIT_DIR"
-   elif [ -d .git ]; then
-   echo .git
+   local gitdir=${1:-}
+
+   if [ -z "$gitdir" ]; then
+   # Try the first matching --git-dir or GIT_DIR, print nothing if 
these
+   # directories are invalid.
+   for gitdir in "${__git_dir-}" "${GIT_DIR-}"; do
+   [ -n "$gitdir" ] || continue
+   if [[ "$gitdir" != /* ]]; then
+   gitdir="${__git_cd:-.}/$gitdir"
+   fi
+   if [ -d "$gitdir" ]; then
+   echo "$gitdir"
+   fi
+   return
+   done
+
+   if [ -d "${__git_cd:-.}/.git" ]; then
+   echo "${__git_cd:-.}/.git"
else
-   git rev-parse --git-dir 2>/dev/null
+   git -C "$__git_cd" rev-parse --git-dir 2>/dev/null
fi
-   elif [ -d "$1/.git" ]; then
-   echo "$1/.git"
else
-   echo "$1"
+   if [[ "$gitdir" != /* ]]; then
+   gitdir="${__git_cd:-.}/$gitdir"
+   fi
+   if [ -d "$gitdir/.git" ]; then
+   echo "$gitdir/.git"
+   elif [ -d "$gitdir" ]; then
+   echo "$gitdir"
+   fi
fi
 }
 
@@ -2566,11 +2581,12 @@ _git_whatchanged ()
 
 __git_main ()
 {
-   local i c=1 command command_word_index __git_dir __git_options
+   local i c=1 command command_word_index __git_dir __git_options 
__git_cd=.
 
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
+   -C)  ((c++)) ; __git_cd="${words[c]}" ;;
--git-dir=*) __git_dir="${i#--git-dir=}" ;;
--git-dir)   ((c++)) ; __git_dir="${words[c]}" ;;
--bare)  __git_dir="." ;;
@@ -2583,6 +2599,7 @@ __git_main ()
done
 
__git_options=(
+   -C "$__git_cd"
--git-dir="$(__gitdir)"
)
 
-- 
2.6.1

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

[PATCH 2/3] completion: pass --git-dir to more commands

[Peter Wu]

The --git-dir option influences more commands, but was not applied
during completions. For example:

# previously empty because --git-dir was not passed to ls-remote
git --git-dir=git/.git config merge.o

Add --git-dir to more git commands (but not for repo-independent
commands such as git help) and add a new internal "__git_options"
array to store this option. In future patches, the -C option will also
be added.  (Alternatively, a new wrapper function can be added instead
of duplicating `${__git_options[@]}` all over the place, but let's keep
it simple for now.)

Add a variable and comments to __git_refs for clarity. (Note that
`--git-dir` needs to be kept there because it may not be the same as
the current repo, e.g. via `git fetch /tmp/repo `.)

Signed-off-by: Peter Wu 
---
 contrib/completion/git-completion.bash | 52 --
 1 file changed, 30 insertions(+), 22 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index bd9ef4c..fdf0f16 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -282,10 +282,10 @@ __gitcomp_file ()
 __git_ls_files_helper ()
 {
if [ "$2" == "--committable" ]; then
-   git -C "$1" diff-index --name-only --relative HEAD
+   git "${__git_options[@]}" -C "$1" diff-index --name-only 
--relative HEAD
else
# NOTE: $2 is not quoted in order to support multiple options
-   git -C "$1" ls-files --exclude-standard $2
+   git "${__git_options[@]}" -C "$1" ls-files --exclude-standard $2
fi 2>/dev/null
 }
 
@@ -315,7 +315,7 @@ __git_heads ()
 {
local dir="$(__gitdir)"
if [ -d "$dir" ]; then
-   git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
+   git "${__git_options[@]}" for-each-ref 
--format='%(refname:short)' \
refs/heads
return
fi
@@ -325,7 +325,7 @@ __git_tags ()
 {
local dir="$(__gitdir)"
if [ -d "$dir" ]; then
-   git --git-dir="$dir" for-each-ref --format='%(refname:short)' \
+   git "${__git_options[@]}" for-each-ref 
--format='%(refname:short)' \
refs/tags
return
fi
@@ -336,8 +336,9 @@ __git_tags ()
 # by checkout for tracking branches
 __git_refs ()
 {
-   local i hash dir="$(__gitdir "${1-}")" track="${2-}"
+   local i hash dir="$(__gitdir "${1-}")" track="${2-}" repo
local format refs
+   # Try refs from a local repository directory (e.g. "../linux")
if [ -d "$dir" ]; then
case "$cur" in
refs|refs/*)
@@ -353,14 +354,15 @@ __git_refs ()
refs="refs/tags refs/heads refs/remotes"
;;
esac
-   git --git-dir="$dir" for-each-ref --format="%($format)" \
-   $refs
+   git "${__git_options[@]}" --git-dir="$dir" \
+   for-each-ref --format="%($format)" $refs
if [ -n "$track" ]; then
# employ the heuristic used by git checkout
# Try to find a remote branch that matches the 
completion word
# but only output if the branch name is unique
local ref entry
-   git --git-dir="$dir" for-each-ref --shell 
--format="ref=%(refname:short)" \
+   git "${__git_options[@]}" --git-dir="$dir" \
+   for-each-ref --shell 
--format="ref=%(refname:short)" \
"refs/remotes/" | \
while read -r entry; do
eval "$entry"
@@ -372,9 +374,11 @@ __git_refs ()
fi
return
fi
+   # Try refs from a remote repository by name (e.g. "origin") or a URL
+   repo="${1-}"
case "$cur" in
refs|refs/*)
-   git ls-remote "$dir" "$cur*" 2>/dev/null | \
+   git "${__git_options[@]}" ls-remote "$repo" "$cur*" 2>/dev/null 
| \
while read -r hash i; do
case "$i" in
*^{}) ;;
@@ -384,8 +388,8 @@ __git_refs ()
;;
*)
echo "HEAD"
-   git for-each-ref --format="%(refname:short)" -- \
-   "refs/remotes/$dir/" 2>/dev/null | sed -e "s#^$dir/##"
+   git "${__git_options[@]}" for-each-ref 
--format="%(refname:short)" -- \
+   "refs/remotes/$repo/" 2>/dev/null | sed -e "s#^$repo/##"
;;
esac
 }
@@ -403,7 +407,7 @@ __git_refs2 ()
 __git_refs_remotes ()
 {
local i hash
-   git ls-remote "$1" 'refs/heads/*' 2>/dev/null | \
+   git "${__git_options[@]}" ls-remote "$1" 'refs/heads/*' 

[PATCH 1/3] completion: ignore git options for subcommand completion

[Peter Wu]

Do not assume that the first option after "git" is a subcommand. This
fixes completion such as:

# do not detect "push" as remote name
git --git-dir=git/.git push origin 
# do not overwrite "--git-dir=..." with the alias expansion
git --git-dir=git/.git gerrit-diff 

Signed-off-by: Peter Wu 
---
 contrib/completion/git-completion.bash | 26 +-
 1 file changed, 13 insertions(+), 13 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 482ca84..bd9ef4c 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,8 +531,8 @@ __git_complete_revlist ()
 
 __git_complete_remote_or_refspec ()
 {
-   local cur_="$cur" cmd="${words[1]}"
-   local i c=2 remote="" pfx="" lhs=1 no_complete_refspec=0
+   local cur_="$cur" cmd="${words[command_word_index]}"
+   local i c=$((command_word_index+1)) remote="" pfx="" lhs=1 
no_complete_refspec=0
if [ "$cmd" = "remote" ]; then
((c++))
fi
@@ -788,7 +788,7 @@ __git_aliased_command ()
 # __git_find_on_cmdline requires 1 argument
 __git_find_on_cmdline ()
 {
-   local word subcommand c=1
+   local word subcommand c=$command_word_index
while [ $c -lt $cword ]; do
word="${words[c]}"
for subcommand in $1; do
@@ -803,7 +803,7 @@ __git_find_on_cmdline ()
 
 __git_has_doubledash ()
 {
-   local c=1
+   local c=$command_word_index
while [ $c -lt $cword ]; do
if [ "--" = "${words[c]}" ]; then
return 0
@@ -826,8 +826,8 @@ __git_count_arguments ()
 {
local word i c=0
 
-   # Skip "git" (first argument)
-   for ((i=1; i < ${#words[@]}; i++)); do
+   # Skip "git" (first argument) and any options such as --git-dir.
+   for ((i=command_word_index; i < ${#words[@]}; i++)); do
word="${words[i]}"
 
case "$word" in
@@ -957,7 +957,7 @@ _git_bisect ()
 
 _git_branch ()
 {
-   local i c=1 only_local_ref="n" has_r="n"
+   local i c=$command_word_index only_local_ref="n" has_r="n"
 
while [ $c -lt $cword ]; do
i="${words[c]}"
@@ -992,7 +992,7 @@ _git_branch ()
 
 _git_bundle ()
 {
-   local cmd="${words[2]}"
+   local cmd="${words[command_word_index+1]}"
case "$cword" in
2)
__gitcomp "create list-heads verify unbundle"
@@ -1760,7 +1760,7 @@ _git_stage ()
 __git_config_get_set_variables ()
 {
local prevword word config_file= c=$cword
-   while [ $c -gt 1 ]; do
+   while [ $c -gt $command_word_index ]; do
word="${words[c]}"
case "$word" in
--system|--global|--local|--file=*)
@@ -2516,7 +2516,7 @@ _git_svn ()
 
 _git_tag ()
 {
-   local i c=1 f=0
+   local i c=$command_word_index f=0
while [ $c -lt $cword ]; do
i="${words[c]}"
case "$i" in
@@ -2562,7 +2562,7 @@ _git_whatchanged ()
 
 __git_main ()
 {
-   local i c=1 command __git_dir
+   local i c=1 command command_word_index __git_dir
 
while [ $c -lt $cword ]; do
i="${words[c]}"
@@ -2573,7 +2573,7 @@ __git_main ()
--help) command="help"; break ;;
-c|--work-tree|--namespace) ((c++)) ;;
-*) ;;
-   *) command="$i"; break ;;
+   *) command="$i"; command_word_index=$c; break ;;
esac
((c++))
done
@@ -2608,7 +2608,7 @@ __git_main ()
 
local expansion=$(__git_aliased_command "$command")
if [ -n "$expansion" ]; then
-   words[1]=$expansion
+   words[command_word_index]=$expansion
completion_func="_git_${expansion//-/_}"
declare -f $completion_func >/dev/null && $completion_func
fi
-- 
2.6.1

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

Re: [PATCH 2/2] http: use credential API to handle proxy authentication

[Eric Sunshine]

On Wednesday, October 28, 2015, Knut Franke
 wrote:
> Currently, the only way to pass proxy credentials to curl is by including them
> in the proxy URL. Usually, this means they will end up on disk unencrypted, 
> one
> way or another (by inclusion in ~/.gitconfig, shell profile or history). Since
> proxy authentication often uses a domain user, credentials can be security
> sensitive; therefore, a safer way of passing credentials is desirable.
>
> If the configured proxy contains a username but not a password, query the
> credential API for one. Also, make sure we approve/reject proxy credentials
> properly.
>
> For consistency reasons, add parsing of http_proxy/https_proxy/all_proxy
> environment variables, which would otherwise be evaluated as a fallback by 
> curl.
> Without this, we would have different semantics for git configuration and
> environment variables.
>
> Signed-off-by: Knut Franke 
> ---
> diff --git a/http.c b/http.c
> index 4756bab..11bebe1 100644
> --- a/http.c
> +++ b/http.c
> @@ -79,6 +79,7 @@ static struct {
> // curl(1) and is not included in CURLAUTH_ANY, so we leave it out
> // here, too
>  };
> +struct credential http_proxy_auth = CREDENTIAL_INIT;

s/^/static/

>  static const char *curl_cookie_file;
>  static int curl_save_cookies;
>  struct credential http_auth = CREDENTIAL_INIT;
> @@ -176,6 +177,9 @@ static void finish_active_slot(struct active_request_slot 
> *slot)
>  #else
> slot->results->auth_avail = 0;
>  #endif
> +
> +   curl_easy_getinfo(slot->curl, CURLINFO_HTTP_CONNECTCODE,
> +   >results->http_connectcode);
> }
>
> /* Run callback if appropriate */
> @@ -333,6 +337,25 @@ static void copy_from_env(const char **var, const char 
> *envname)
>
>  static void init_curl_proxy_auth(CURL *result)
>  {
> +   if (http_proxy_auth.username) {
> +   if (!http_proxy_auth.password) {
> +   credential_fill(_proxy_auth);
> +   }

Style: drop unnecessary braces

> +#if LIBCURL_VERSION_NUM >= 0x071301
> +   curl_easy_setopt(result, CURLOPT_PROXYUSERNAME,
> +   http_proxy_auth.username);
> +   curl_easy_setopt(result, CURLOPT_PROXYPASSWORD,
> +   http_proxy_auth.password);
> +#else
> +   struct strbuf up = STRBUF_INIT;

Minor: It took me a moment to figure out that "up" meant
user-password. I wonder if a simpler name such as 's' would suffice?

> +   strbuf_reset();

Unnecessary strbuf_reset().

> +   strbuf_addstr_urlencode(, http_proxy_auth.username, 1);
> +   strbuf_addch(, ':');
> +   strbuf_addstr_urlencode(, http_proxy_auth.password, 1);
> +   curl_easy_setopt(result, CURLOPT_PROXYUSERPWD, up.buf);

Leaking 'up'. Insert strbuf_release() here.

> +#endif
> +   }
> +
> copy_from_env(_proxy_authmethod, "GIT_HTTP_PROXY_AUTHMETHOD");
>
> if (http_proxy_authmethod) {
> @@ -513,8 +536,36 @@ static CURL *get_curl_handle(void)
> curl_easy_setopt(result, CURLOPT_USE_SSL, CURLUSESSL_TRY);
>  #endif
>
> +   /*
> +* curl also examines these variables as a fallback; but we need to 
> query
> +* them here in order to decide whether to prompt for missing 
> password (cf.
> +* init_curl_proxy_auth()).
> +*/
> +   if (!curl_http_proxy) {
> +   if (!strcmp(http_auth.protocol, "https")) {
> +   copy_from_env(_http_proxy, "HTTPS_PROXY");
> +   copy_from_env(_http_proxy, "https_proxy");
> +   } else {
> +   copy_from_env(_http_proxy, "http_proxy");

To the casual reader, it's not obvious why you check upper- and
lowercase versions of the other environment variables but not this
one.

> +   }
> +   if (!curl_http_proxy) {
> +   copy_from_env(_http_proxy, "ALL_PROXY");
> +   copy_from_env(_http_proxy, "all_proxy");
> +   }

If this sort of upper- and lowercase environment variable name
checking is indeed desirable, I wonder if it would make sense to fold
that functionality into the helper function.

> +   }
> +
> if (curl_http_proxy) {
> -   curl_easy_setopt(result, CURLOPT_PROXY, curl_http_proxy);
> +   if (strstr(curl_http_proxy, "://"))
> +   credential_from_url(_proxy_auth, 
> curl_http_proxy);
> +   else {
> +   struct strbuf url = STRBUF_INIT;
> +   strbuf_reset();

Unnecessary strbuf_reset().

> +   strbuf_addstr(, "http://;);
> +   strbuf_addstr(, curl_http_proxy);

strbuf_addf(, "http://%s;, curl_http_proxy) might be more straightforward.

> +   

Bug: dcommit fails when a file with '@' in the name is renamed

[Dimitar Dobrev]

When a file with '@' in the name is renamed - for example moved -
dcommit fails with:


Assertion failed: (svn_uri_is_canonical(child_uri, NULL)), function
uri_skip_ancestor, file subversion/libsvn_subr/dirent_uri.c, line 1520.

 error: git-svn died of signal 6


I have sent a fix at https://github.com/git/git/pull/184.

--
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] ref-filter: fallback on alphabetical comparison

[Karthik Nayak]

On Wed, Oct 28, 2015 at 1:20 AM, Junio C Hamano  wrote:
>> Hence, fallback to alphabetical comparison based on the refname
>> whenever the other criterion is equal. Fix the test in t3203 in this
>> regard.
>
> It is unclear what "in this regard" is.  Do you mean this (I am not
> suggesting you to spell these out in a very detailed way in the
> final log message; I am deliberately being detailed here to help me
> understand what you really mean)?
>
> A test in t3203 was expecting that branch-two sorts before HEAD,
> which happened to be how qsort(3) on Linux sorted the array, but
> (1) that outcome was not even guaranteed, and (2) once we start
> breaking ties with the refname, "HEAD" should sort before
> "branch-two" so the original expectation was inconsistent with
> the criterion we now use.
>

Exactly what you're saying, they happened to have the same objectsize.
Hence sorting them would put them together, but since we compare the
refname's the "HEAD" ref would come before "branch-two".

> Update it to match the new world order, which we can now depend
> on being stable.
>
> I am not sure about "HEAD" and "branch-two" in the above (it may be
> comparison between "HEAD" and "refs/heads/branch-two", for example).
>

It actually is, we consider "refs/heads/branch-two rather then the shortened
version of this. It makes sense to classify refs this way, even though this
was a side effect of this commit.

-- 
Regards,
Karthik Nayak
--
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: [PATCHv1 2/2] git-p4: work with a detached head

[Luke Diamand]

On 28/10/15 17:44, Junio C Hamano wrote:

Luke Diamand  writes:


On 9 September 2015 at 22:52, Junio C Hamano  wrote:

Luke Diamand  writes:
...
 def currentGitBranch():
 return read_pipe("git name-rev HEAD").split(" ")[1].strip()

Yuck.  I know it is not entirely the fault of this patch, but
shouldn't it be reading from

 $ git symbolic-ref HEAD

and catch the error "fatal: ref HEAD is not a symbolic ref" and use
it as a signal to tell that the HEAD is detached?


That sounds much nicer. I'll redo the patch accordingly.


No need to rush, but should I expect a reroll of this sometime, or
have things around this topic changed to make this topic no longer
necessary?  I am only asking so that I can decide to either keep or
drop ld/p4-detached-head topic that is listed in the [Stalled]
section for quite some time [*1*].


I was waiting for the other git-p4 changes to go through before starting 
this up again.


It definitely needs fixing - it was annoying me a lot today, as I kept 
on having to invent temporary branch names to needlessly keep git-p4 happy.


After getting to "for-p4-9", I'm now onto "x". I'll see if I can 
sort something out in the next few days.


Luke




Thanks.


[Footnote]

*1* Not that my dropping a topic from 'pu' means very much; a
 dropped topic can still be submitted and requeued after all.



--
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] difftool: avoid symlinks when reusing worktree files

[David Aguilar]

On Tue, Oct 27, 2015 at 03:24:49PM -0700, Junio C Hamano wrote:
> David Aguilar  writes:
> 
> > difftool's dir-diff should never reuse a symlink, regardless of
> > what it points to.  Tighten use_wt_file() so that it rejects all
> > symlinks.
> >
> > Helped-by: Junio C Hamano 
> > Signed-off-by: David Aguilar 
> > ---
> 
> Sorry.  I do recall saying "it is wrong to feed the contents of a
> file that a symlink points at to hash-object" but other than that,
> I completely lost track.
> 
> What purpose does this function play in its callchain?  What does
> its caller wants it to compute?  Is use of the entity in the working
> tree completely optional?  Would the caller happily produce correct
> result even if we changed this function to unconditionally return
> ($use=0, $wt_sha1='0'x40) regardless of the result of lstat(2) on
> "$workdir/$file"?
> 
> The conclusion of the thought process that starts from "it is wrong
> to feed the contents of a file that a symlink points at to
> hash-object" may not be "so let's return $use=0 for all symlinks",
> which is this patch. Depending on what its caller wants it to
> compute, the right conclusion may be "we need to call hash-object
> correctly by first running readlink and then feeding the result to
> it".
> 
> And if the answer is "the caller wants us to compute the hash for a
> symbolic link and say $use=1", then we would instead need to do
> an equivalent of
> 
>   wt_sha1=$(readlink "$workdir/$file" | hash-object --stdin)
> 
> I cannot quite tell which from the patch and explanation.
> 
> Perhaps an additional test or two would help illustrate what issues
> are being addressed better?
> 
> Thanks.

Right.  At first I thought I could revise the commit message to
make it clearer that we simply want to skip all symlinks, since
it never makes sense to reuse a worktree symlinks, but looking
at the tests and implementation makes me realize that it's not
that simple.

This is going to take a bit more time to get right.  John, I was
hoping you'd be able to take a look -- I'm playing catch-up too.
When it was first reported I let it sit for a while in hopes
that the original author would pickup the issue, but months
passed and I figured I'd take a stab at helping the user out.

Anyways, it'll take me a bit more time to understand the code
and work out a sensible solution.  My gut feeling is that we
should adjust the dir-diff feature so that it ignores all
symlinks.  That seems like a simple answer since we're deciding
to skip that chunk of complexity.

John, do you have any thoughts on how we can best handle this?
-- 
David
--
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/6] Facilitate debugging Git executables in tests with gdb

[Victor Leschuk]

  
+if test -n "$TEST_GDB_GIT"

+then
+   exec gdb -args "${GIT_EXEC_PATH}/@@PROG@@" "$@"
Maybe we could make $TEST_GDB_GIT not just a boolean flag? It would be 
useful to contain "gdb" executable name. It would allow to set path to 
GDB when it is not in $PATH, set different debuggers (for example, I 
usually use cgdb), or even set it to /path/to/gdb_wrapper.sh which could 
contain different gdb options and tunings.


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

Re: [PATCH 6/9] clone: allow an explicit argument for parallel submodule clones

[Stefan Beller]

On Tue, Oct 27, 2015 at 1:57 PM, Junio C Hamano  wrote:
>> + The number of submodules fetched at the same time.
>
> Do we want to say "Defaults to submodule.jobs" somewhere?

Yes. :)

> I am tempted to suggest that you should not pay attention to
> "submodule.jobs" in this command at all and just pass through
> "--jobs=$max_jobs" that was specified from the command line, as the
> spawned "submodule update --init --recursive" would handle
> "submodule.jobs" itself.

makes sense.

>
> Once you start allowing "clone.jobs" as a more specific version of
> "submodule.jobs", then reading max_jobs first from "clone.jobs" and
> then from the command line starts to make sense.  When neither is
> specified, you would spawn "submodule update --init --recursive"
> without any explicit "-j N" and let it honor its more generic
> "submodule.jobs" setting; otherwise, you would run it with "-j N" to
> override that more generic "submodule.jobs" setting with either the
> value the command line -j given to "clone" or specified by a more
> specific "clone.jobs".

I see. Though I do not plan adding clone.jobs in the near future.
--
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 5/9] submodule update: expose parallelism to the user

[Stefan Beller]

On Tue, Oct 27, 2015 at 1:59 PM, Junio C Hamano  wrote:
> And when 0 starts to meaning something special, we would need to
> describe that here (and/or submodule.jobs entry in config.txt).
> As I already said, I do not think "0 means num_cpus" is a useful
> default, and I would prefer if we reserved 0 to mean something more
> useful we would figure out later.

Ok I'll add that, too.

I am just debating with myself where the best place is.
In run-command.c in pp_init we have:

if (n < 1)
n = online_cpus();
pp->max_processes = n;

we would need to change only that one place to insert an

die("We haven't found the right default yet for 0");

However I think for most loads online_cpus makes sense as that
is ususally the bottleneck for local operations (if being excessive
memory may become an issue, but unlikely IMHO).
So instead I think it makes more sense to add it in the fetch/clone/update
to come up with a treatment for 0.

Maybe we want to make the explicit decision for the default value
for any user of the parallel processing, such that this code above
is misguided as it leads to bad defaults if reviewers are inattentive.

So having spelled out that, we may just want to bark in the pp_init
for having a number n < 1.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] clone: Allow an explicit argument for parallel submodule clones

[Sebastian Schuberth]

On 23.10.2015 20:44, Stefan Beller wrote:


[...] which may pick reasonable
defaults if you don't specify an explicit number.


IMO the above should also be mentioned ini the docs:


+-j::
+--jobs::
+   The number of submodules fetched at the same time.


Otherwise, from reading the docs, my immediate question would be "What's 
the default for n if not specified?"


--
Sebastian Schuberth

--
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: fatal: Unable to read current working directory: No error

[Jeff King]

On Tue, Oct 27, 2015 at 07:29:39PM -0400, Sean Krauth wrote:

> This seemed like about as good of an excuse as any to update Git. I
> was running v. 2.5.1-32-bit and so I downloaded v. 2.6.2-32-bit,
> installed it. And it ran, kinda. I no longer seem to have access to
> any of my old commits and when I try to "git init" or "git status" I
> get the above error, "fatal: Unable to read current working directory:
> No error". This error pops up for anything, even new files that never
> had a repository.

That message means that getcwd() is failing. If it were happening in one
place, I'd say to check if there is something funny with your directory
(e.g., bad permissions or something). But if it's happening anywhere, it
sounds like there is some fundamental incompatibility between the build
of Git and your system.

It sounds like you're on a Windows system, and the problem may be
system-specific. You might try asking at the specific Git for Windows
list:

  https://groups.google.com/forum/?hl=en#!forum/git-for-windows

though many of those people do frequent this list, too.

-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 13/17] cat-file: read batch stream with strbuf_gets()

[Junio C Hamano]

It is possible to prepare a text file with a DOS editor and feed it
as a batch command stream to the command.

Signed-off-by: Junio C Hamano 
---
 builtin/cat-file.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/cat-file.c b/builtin/cat-file.c
index c0fd8db..e79097d 100644
--- a/builtin/cat-file.c
+++ b/builtin/cat-file.c
@@ -401,7 +401,7 @@ static int batch_objects(struct batch_options *opt)
save_warning = warn_on_object_refname_ambiguity;
warn_on_object_refname_ambiguity = 0;
 
-   while (strbuf_getline(, stdin, '\n') != EOF) {
+   while (strbuf_gets(, stdin) != EOF) {
if (data.split_on_whitespace) {
/*
 * Split at first whitespace, tying off the beginning
-- 
2.6.2-423-g5314b62

--
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 03/17] update-index: read --index-info with strbuf_gets()

[Junio C Hamano]

Signed-off-by: Junio C Hamano 
---
 builtin/update-index.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index 7431938..dfc65a8 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -473,7 +473,9 @@ static void read_index_info(int line_termination)
struct strbuf buf = STRBUF_INIT;
struct strbuf uq = STRBUF_INIT;
 
-   while (strbuf_getline(, stdin, line_termination) != EOF) {
+   while ((line_termination
+   ? strbuf_gets(, stdin)
+   : strbuf_getline(, stdin, '\0')) != EOF) {
char *ptr, *tab;
char *path_name;
unsigned char sha1[20];
-- 
2.6.2-423-g5314b62

--
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 v1] git-p4: Add option to ignore empty commits

[Lars Schneider]

On 26 Oct 2015, at 21:40, Luke Diamand  wrote:

> On 24/10/15 19:08, Lars Schneider wrote:
>> 
>> On 21 Oct 2015, at 08:32, Luke Diamand  wrote:
>> 
>>> On 19/10/15 19:43, larsxschnei...@gmail.com wrote:
 From: Lars Schneider 
 
-- snip --
>> 
>>> Also, could you use python3 style print stmnts, print("whatever") ?
>> Sure. How do you prefer the formatting? Using "format" would be true Python 
>> 3 style I think:
>> print('Ignoring file outside of client spec: {}'.format(path))
> 
> Will that breaker older versions of python? There's a statement somewhere 
> about how far back we support.
The "format" method requires Python 2.6 according to the Python docs:
https://docs.python.org/2/library/functions.html#format

Luckily this is also the version we aim to support according to 
Documentation/CodingGuidelines

Thanks,
Lars

--
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] ref-filter: fallback on alphabetical comparison

[Junio C Hamano]

Karthik Nayak  writes:

> On Wed, Oct 28, 2015 at 1:20 AM, Junio C Hamano  wrote:
>>> Hence, fallback to alphabetical comparison based on the refname
>>> whenever the other criterion is equal. Fix the test in t3203 in this
>>> regard.
>>
>> It is unclear what "in this regard" is.  Do you mean this (I am not
>> suggesting you to spell these out in a very detailed way in the
>> final log message; I am deliberately being detailed here to help me
>> understand what you really mean)?
>>
>> A test in t3203 was expecting that branch-two sorts before HEAD,
>> which happened to be how qsort(3) on Linux sorted the array, but
>> (1) that outcome was not even guaranteed, and (2) once we start
>> breaking ties with the refname, "HEAD" should sort before
>> "branch-two" so the original expectation was inconsistent with
>> the criterion we now use.
>>
>
> Exactly what you're saying, they happened to have the same objectsize.
> Hence sorting them would put them together, but since we compare the
> refname's the "HEAD" ref would come before "branch-two".
>
>> Update it to match the new world order, which we can now depend
>> on being stable.
>>
>> I am not sure about "HEAD" and "branch-two" in the above (it may be
>> comparison between "HEAD" and "refs/heads/branch-two", for example).
>
> It actually is, we consider "refs/heads/branch-two rather then the shortened
> version of this. It makes sense to classify refs this way, even though this
> was a side effect of this commit.

Now these are enough bits of info, that can and needs to be
condenced into an updated log message to help future readers.

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

[PATCH 11/17] clone/sha1_file: read info/alternates with strbuf_gets()

[Junio C Hamano]

$GIT_OBJECT_DIRECTORY/info/alternates is a text file that can be
edited with a DOS editor.  We do not want to use the real path with
CR appeneded at the end.

Signed-off-by: Junio C Hamano 
---
 builtin/clone.c | 2 +-
 sha1_file.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 9eaecd9..3d2615c 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -339,7 +339,7 @@ static void copy_alternates(struct strbuf *src, struct 
strbuf *dst,
FILE *in = fopen(src->buf, "r");
struct strbuf line = STRBUF_INIT;
 
-   while (strbuf_getline(, in, '\n') != EOF) {
+   while (strbuf_gets(, in) != EOF) {
char *abs_path;
if (!line.len || line.buf[0] == '#')
continue;
diff --git a/sha1_file.c b/sha1_file.c
index 50896ff..957178c 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -395,7 +395,7 @@ void add_to_alternates_file(const char *reference)
struct strbuf line = STRBUF_INIT;
int found = 0;
 
-   while (strbuf_getline(, in, '\n') != EOF) {
+   while (strbuf_gets(, in) != EOF) {
if (!strcmp(reference, line.buf)) {
found = 1;
break;
-- 
2.6.2-423-g5314b62

--
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 07/17] revision: read --stdin with strbuf_gets()

[Junio C Hamano]

Reading with getwholeline() and manually stripping the terminating
'\n' would leave CR at the end of the line if the input comes from
a DOS editor.

Constrasting this with the previous few changes, one may realize
that the way "log" family of commands read the paths with --stdin
looks inconsistent and sloppy.  It does not allow us to C-quote a
textual input, and it does not accept NUL-terminated records.  These
are unfortunately way too late to fix X-<.

Signed-off-by: Junio C Hamano 
---
 revision.c | 9 ++---
 1 file changed, 2 insertions(+), 7 deletions(-)

diff --git a/revision.c b/revision.c
index 2236463..7d100d8 100644
--- a/revision.c
+++ b/revision.c
@@ -1641,10 +1641,7 @@ static void append_prune_data(struct cmdline_pathspec 
*prune, const char **av)
 static void read_pathspec_from_stdin(struct rev_info *revs, struct strbuf *sb,
 struct cmdline_pathspec *prune)
 {
-   while (strbuf_getwholeline(sb, stdin, '\n') != EOF) {
-   int len = sb->len;
-   if (len && sb->buf[len - 1] == '\n')
-   sb->buf[--len] = '\0';
+   while (strbuf_gets(sb, stdin) != EOF) {
ALLOC_GROW(prune->path, prune->nr + 1, prune->alloc);
prune->path[prune->nr++] = xstrdup(sb->buf);
}
@@ -1661,10 +1658,8 @@ static void read_revisions_from_stdin(struct rev_info 
*revs,
warn_on_object_refname_ambiguity = 0;
 
strbuf_init(, 1000);
-   while (strbuf_getwholeline(, stdin, '\n') != EOF) {
+   while (strbuf_gets(, stdin) != EOF) {
int len = sb.len;
-   if (len && sb.buf[len - 1] == '\n')
-   sb.buf[--len] = '\0';
if (!len)
break;
if (sb.buf[0] == '-') {
-- 
2.6.2-423-g5314b62

--
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 09/17] ident.c: read /etc/mailname with strbuf_gets()

[Junio C Hamano]

Just in case /etc/mailname file was edited with a DOS editor,
read it with strbuf_gets() so that a stray CR is not included
as the last character of the mail hostname.

We _might_ want to more aggressively discard whitespace characters
around the line with strbuf_trim(), but that is a bit outside the
scope of this series.

Signed-off-by: Junio C Hamano 
---
 ident.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/ident.c b/ident.c
index 5ff1aad..c377f2b 100644
--- a/ident.c
+++ b/ident.c
@@ -55,7 +55,7 @@ static int add_mailname_host(struct strbuf *buf)
strerror(errno));
return -1;
}
-   if (strbuf_getline(, mailname, '\n') == EOF) {
+   if (strbuf_gets(, mailname) == EOF) {
if (ferror(mailname))
warning("cannot read /etc/mailname: %s",
strerror(errno));
-- 
2.6.2-423-g5314b62

--
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 16/17] grep: read -f file with strbuf_gets()

[Junio C Hamano]

List of patterns file could come from a DOS editor.

This is iffy; you may actually be trying to find a line with ^M in
it on a system whose line ending is LF.  You can of course work it
around by having a line that has "^M^M^J", let the strbuf_gets() eat
the last "^M^J", leaving just the single "^M" as the pattern.

Signed-off-by: Junio C Hamano 
---
 builtin/grep.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/grep.c b/builtin/grep.c
index d04f440..ac27690 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -562,7 +562,7 @@ static int file_callback(const struct option *opt, const 
char *arg, int unset)
patterns = from_stdin ? stdin : fopen(arg, "r");
if (!patterns)
die_errno(_("cannot open '%s'"), arg);
-   while (strbuf_getline(, patterns, '\n') == 0) {
+   while (strbuf_gets(, patterns) == 0) {
/* ignore empty line like grep does */
if (sb.len == 0)
continue;
-- 
2.6.2-423-g5314b62

--
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 00/17] Peace with CRLF

[Junio C Hamano]

We have too many topics titled "War on something"; let's try to make
peace for a change.

This is a continuation to $gmane/275735, which is filed in the
archive under another mailing list:

  http://thread.gmane.org/gmane.comp.version-control.msysgit/21773

We read "text" files from filesystem (or from the standard input) in
various places in the code.  Some of them are written by us, but
others are often prepared by an editor driven by human user.  Even
though we write (and expect) lines in these text files to be
terminated with LF (not CRLF), the end-user's editor can be told to
use CRLF termination, and on some platforms that may be the default.

Because many codepaths (e.g. commit log message) first pass the
contents of such a file through stripspace(), and as a side effect
of discarding the whitespace at the end of each line, lines
terminated with CRLF are normalized to LF terminated lines (but we
do not lose a lone CR in the middle of a non-blank string), this is
not a problem in many codepaths.  But not all of the codepaths are
safe.

Typically, we use strbuf_getline() to read these "text" files, which
reads a single line up to the first LF into a buffer, discard that
LF at the end, and returns (an incomplete last line gets returned
as-is).  In theory, these places that expect to read "text", we
should be able to update the lotic to read a line up to the first
LF, discard that LF, together with a CR if one appears immediately
before that LF, without breaking anything.

I inspected all the callsites of this function to see if it is safe
to use such an updated logic at these callsites, and did not find
anything problematic.  I could update strbuf_getline() in place, but
just to be extra careful, this series instead introduces another
helper, strbuf_gets(), that is aware of this CRLF business, and
convert the ones that are safe to update as we verify.

At the end of this message, you will find my notes while inspecting
the current codebase as of 37023ba3 (Seventh batch for 2.7,
2015-10-26).

 * This series converts only the callers of strbuf_getline() in the
   category [A], i.e. the current code would misbehave when fed a
   file with CRLF-terminated lines and use the data with an unwanted
   CR appended at the end, and with the update the code should work
   as intended with such a file, without breaking the expected
   behaviour when working on a file with LF-terminated lines.

 * Callers that expect to read from our own output do not have to
   accomodate CRLF-terminated lines, but they can be updated to do
   so safely if we do not rely on the ability to express a payload
   that has CR at the end.  For example, the insn sheet "rebase -i"
   uses typically end with commit log summary that we ourselves do
   not read or use, so even if the end-user on a platform with LF
   lines deliberately does insert \r at the end of the line and
   strbuf_gets() removed that \r from the payload, no unexpected
   behaviour should happen.  They are categorized as [B] in the
   attached notes (and this series does not touch them).

 * Some callers just strbuf_trim() or otherwise have logic that
   tolerates whitespaces at the end of the line.  For them, it does
   not make any difference whether strbuf_getline() or strbuf_gets()
   is used to read the lines to be processed.  They are caregorized
   as [C] in the attached notes (and this series does not touch
   them).

 * I haven't found a caller that wants to only see LF-terminated
   lines (in other words, "ABC\r\n" must be treated as a line with 4
   bytes payload on it, A B C and CR), but the survey reserves
   category name [X] for this empty set ;-).

Double-checking my classification for callers in [B] and [C] and
making them all use strbuf_gets() would be a good microproject for
GSoC in coming years ;-) If all callers with strbuf_getline() that
uses '\n' as the terminator disappears at the end, that would be
ideal.

Junio C Hamano (17):
  strbuf: add strbuf_gets()
  check-attr, check-ignore, checkout-index: read paths with strbuf_gets()
  update-index: read --index-info with strbuf_gets()
  update-index: read list of paths with strbuf_gets() under --stdin
  mktree: read textual tree representation with strbuf_gets()
  hash-object: read --stdin-paths with strbuf_gets()
  revision: read --stdin with strbuf_gets()
  rev-parse: read parseopt spec with strbuf_gets()
  ident.c: read /etc/mailname with strbuf_gets()
  remote.c: read $GIT_DIR/remotes/* with strbuf_gets()
  clone/sha1_file: read info/alternates with strbuf_gets()
  transport-helper: read helper response with strbuf_gets()
  cat-file: read batch stream with strbuf_gets()
  column: read lines with strbuf_gets()
  send-pack: read list of refs with strbuf_gets()
  grep: read -f file with strbuf_gets()
  test-sha1-array: read command stream with strbuf_gets()

 builtin/am.c | 23 ---
 builtin/cat-file.c   |  2 +-
 builtin/check-attr.c |  4 +++-
 

[PATCH 01/17] strbuf: add strbuf_gets()

[Junio C Hamano]

Often we read "text" files that are supplied by the end user
(e.g. commit log message that was edited with $GIT_EDITOR upon 'git
commit -e'), and in some environments lines in a text file are
terminated with CRLF.  Existing strbuf_getline() knows to read a
single line and then strip the terminating byte from the result, but
it is handy to have a version that is more tailored for a "text"
input that takes both '\n' and '\r\n' as line terminator (aka
 in POSIX lingo) and returns the body of the line after
stripping .

Recently reimplemented "git am" already uses such a function
implemented privately; move it to strbuf.[ch] and make it available
for others.

Signed-off-by: Junio C Hamano 
---
 builtin/am.c | 23 ---
 strbuf.c | 16 ++--
 strbuf.h |  7 +++
 3 files changed, 25 insertions(+), 21 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 4e396c8..9376d5e 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -45,21 +45,6 @@ static int is_empty_file(const char *filename)
 }
 
 /**
- * Like strbuf_getline(), but treats both '\n' and "\r\n" as line terminators.
- */
-static int strbuf_getline_crlf(struct strbuf *sb, FILE *fp)
-{
-   if (strbuf_getwholeline(sb, fp, '\n'))
-   return EOF;
-   if (sb->buf[sb->len - 1] == '\n') {
-   strbuf_setlen(sb, sb->len - 1);
-   if (sb->len > 0 && sb->buf[sb->len - 1] == '\r')
-   strbuf_setlen(sb, sb->len - 1);
-   }
-   return 0;
-}
-
-/**
  * Returns the length of the first line of msg.
  */
 static int linelen(const char *msg)
@@ -627,7 +612,7 @@ static int is_mail(FILE *fp)
if (regcomp(, header_regex, REG_NOSUB | REG_EXTENDED))
die("invalid pattern: %s", header_regex);
 
-   while (!strbuf_getline_crlf(, fp)) {
+   while (!strbuf_gets(, fp)) {
if (!sb.len)
break; /* End of header */
 
@@ -674,7 +659,7 @@ static int detect_patch_format(const char **paths)
 
fp = xfopen(*paths, "r");
 
-   while (!strbuf_getline_crlf(, fp)) {
+   while (!strbuf_gets(, fp)) {
if (l1.len)
break;
}
@@ -695,9 +680,9 @@ static int detect_patch_format(const char **paths)
}
 
strbuf_reset();
-   strbuf_getline_crlf(, fp);
+   strbuf_gets(, fp);
strbuf_reset();
-   strbuf_getline_crlf(, fp);
+   strbuf_gets(, fp);
 
/*
 * If the second line is empty and the third is a From, Author or Date
diff --git a/strbuf.c b/strbuf.c
index d76f0ae..290fc74 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -505,8 +505,20 @@ int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 {
if (strbuf_getwholeline(sb, fp, term))
return EOF;
-   if (sb->buf[sb->len-1] == term)
-   strbuf_setlen(sb, sb->len-1);
+   if (sb->buf[sb->len - 1] == term)
+   strbuf_setlen(sb, sb->len - 1);
+   return 0;
+}
+
+int strbuf_gets(struct strbuf *sb, FILE *fp)
+{
+   if (strbuf_getwholeline(sb, fp, '\n'))
+   return EOF;
+   if (sb->buf[sb->len - 1] == '\n') {
+   strbuf_setlen(sb, sb->len - 1);
+   if (sb->len && sb->buf[sb->len - 1] == '\r')
+   strbuf_setlen(sb, sb->len - 1);
+   }
return 0;
 }
 
diff --git a/strbuf.h b/strbuf.h
index 7123fca..c22bae0 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -388,6 +388,13 @@ extern int strbuf_readlink(struct strbuf *sb, const char 
*path, size_t hint);
  */
 extern int strbuf_getline(struct strbuf *, FILE *, int);
 
+/*
+ * Similar to strbuf_getline(), but uses '\n' as the terminator,
+ * and additionally treats a '\r' that comes immediately before '\n'
+ * as part of the terminator.
+ */
+extern int strbuf_gets(struct strbuf *, FILE *);
+
 /**
  * Like `strbuf_getline`, but keeps the trailing terminator (if
  * any) in the buffer.
-- 
2.6.2-423-g5314b62

--
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 06/17] hash-object: read --stdin-paths with strbuf_gets()

[Junio C Hamano]

The list of paths could have been written with a DOS editor.

Signed-off-by: Junio C Hamano 
---
 builtin/hash-object.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/hash-object.c b/builtin/hash-object.c
index 43b098b..46d55e5 100644
--- a/builtin/hash-object.c
+++ b/builtin/hash-object.c
@@ -60,7 +60,7 @@ static void hash_stdin_paths(const char *type, int 
no_filters, unsigned flags,
 {
struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
 
-   while (strbuf_getline(, stdin, '\n') != EOF) {
+   while (strbuf_gets(, stdin) != EOF) {
if (buf.buf[0] == '"') {
strbuf_reset();
if (unquote_c_style(, buf.buf, NULL))
-- 
2.6.2-423-g5314b62

--
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 15/17] send-pack: read list of refs with strbuf_gets()

[Junio C Hamano]

Signed-off-by: Junio C Hamano 
---
 builtin/send-pack.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/send-pack.c b/builtin/send-pack.c
index f6e5d64..ba318c9 100644
--- a/builtin/send-pack.c
+++ b/builtin/send-pack.c
@@ -212,7 +212,7 @@ int cmd_send_pack(int argc, const char **argv, const char 
*prefix)
argv_array_push(_refspecs, buf);
} else {
struct strbuf line = STRBUF_INIT;
-   while (strbuf_getline(, stdin, '\n') != EOF)
+   while (strbuf_gets(, stdin) != EOF)
argv_array_push(_refspecs, line.buf);
strbuf_release();
}
-- 
2.6.2-423-g5314b62

--
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 08/17] rev-parse: read parseopt spec with strbuf_gets()

[Junio C Hamano]

"rev-parse --parseopt" specification is clearly text and we
should anticipate that we may be fed CRLF lines.

Signed-off-by: Junio C Hamano 
---
 builtin/rev-parse.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index 02d747d..5317389 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -386,7 +386,7 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
 
/* get the usage up to the first line with a -- on it */
for (;;) {
-   if (strbuf_getline(, stdin, '\n') == EOF)
+   if (strbuf_gets(, stdin) == EOF)
die("premature end of input");
ALLOC_GROW(usage, unb + 1, usz);
if (!strcmp("--", sb.buf)) {
@@ -399,7 +399,7 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
}
 
/* parse: (|,|)[*=?!]*? SP+  */
-   while (strbuf_getline(, stdin, '\n') != EOF) {
+   while (strbuf_gets(, stdin) != EOF) {
const char *s;
const char *help;
struct option *o;
-- 
2.6.2-423-g5314b62

--
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 04/17] update-index: read list of paths with strbuf_gets() under --stdin

[Junio C Hamano]

Signed-off-by: Junio C Hamano 
---
 builtin/update-index.c | 5 -
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/builtin/update-index.c b/builtin/update-index.c
index dfc65a8..004871b 100644
--- a/builtin/update-index.c
+++ b/builtin/update-index.c
@@ -1075,7 +1075,10 @@ int cmd_update_index(int argc, const char **argv, const 
char *prefix)
struct strbuf buf = STRBUF_INIT, nbuf = STRBUF_INIT;
 
setup_work_tree();
-   while (strbuf_getline(, stdin, line_termination) != EOF) {
+
+   while ((line_termination
+   ? strbuf_gets(, stdin)
+   : strbuf_getline(, stdin, '\0')) != EOF) {
char *p;
if (line_termination && buf.buf[0] == '"') {
strbuf_reset();
-- 
2.6.2-423-g5314b62

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

Re: [PATCH 1/2] prepare_packed_git(): refactor garbage reporting in pack directory

[Doug Kelly]

On Wed, Oct 28, 2015 at 12:48 PM, Junio C Hamano  wrote:
> Junio C Hamano  writes:
>
>> Eric Sunshine  writes:
>>
 -static void real_report_garbage(const char *desc, const char *path)
 +const char *bits_to_msg(unsigned seen_bits)
>>>
>>> If you don't expect other callers outside this file, then this should
>>> be declared 'static'. If you do expect future external callers, then
>>> this should be declared in a public header file (but renamed to be
>>> more meaningful).
>>
>> I think this can be private to this file.  The sole point of moving
>> this logic to this file is to make it private, after all ;-)  Thanks
>> for sharp eyes.
>>
>> Together with the need for a description on "why", this probably
>> deserves a test or two, probably at the end of t5304.
>>
>> Thanks.
>
> Does somebody want to help tying the final loose ends on this topic?
> It has been listed in the [Stalled] section for too long, I _think_
> what it attempts to do is a worthy thing, and it is shame to see the
> initial implementation and review cycles we have spent so far go to
> waste.
>
> If I find nothing else to do before any taker appears, I could
> volunteer myself, but thought I should ask first.
>
> Thanks.

I agree; I've been wanting to get back to it, but had some
higher-priority things at work for a while, so I've not had time.  I'd
be happy to get back into it, but if you get to it first, believe me,
I'm not going to be offended. :)

I'll see if I can't devote a little extra time to it this upcoming
week, though.  Hopefully it doesn't need too much additional polishing
to be ready.

P.S. Does a Googler want to tell the Inbox team that the inability to
send plain-text email is really annoying? :P
--
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] merge-file: clamp exit code to maximum 127

[Jeff King]

Git-merge-file is documented to return one of three exit
codes:

  - zero means the merge was successful

  - a negative number means an error occurred

  - a positive number indicates the number of conflicts

Unfortunately, this all gets stuffed into an 8-bit return
code. Which means that if you have 256 conflicts, this wraps
to zero, and the merge appears to succeed (and commits a
blob full of conflict-marker cruft!).

This patch clamps the return value to a maximum of 127,
which we should be able to safely represent everywhere. This
also leaves 128-255 for other values. Shells (and some parts
of git) will typically represent signal death as 128 plus
the signal number. And negative values are typically coerced
to an 8-bit unsigned value (so "return -1" ends up as 255).

Technically negative returns have the same problem (e.g.,
"-256" wraps back to 0), but this is not a problem in
practice, as the only negative value we use is "-1".

Signed-off-by: Jeff King 
---
This can be triggered when using the "resolve" strategy, though I found
it when comparing the results of git-merge-index with libgit2's merge
driver across a large number of GitHub merges (git claimed to
successfully merge but libgit2 correctly identified the conflict). The
real world case that triggered it had exactly 768 conflicts.

 Documentation/git-merge-file.txt |  3 ++-
 builtin/merge-file.c |  3 +++
 t/t7600-merge.sh | 33 +
 3 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-merge-file.txt b/Documentation/git-merge-file.txt
index d2fc12e..f856032 100644
--- a/Documentation/git-merge-file.txt
+++ b/Documentation/git-merge-file.txt
@@ -41,7 +41,8 @@ lines from ``, or lines from both respectively.  
The length of the
 conflict markers can be given with the `--marker-size` option.
 
 The exit value of this program is negative on error, and the number of
-conflicts otherwise. If the merge was clean, the exit value is 0.
+conflicts otherwise (truncated to 127 if there are more than that many
+conflicts). If the merge was clean, the exit value is 0.
 
 'git merge-file' is designed to be a minimal clone of RCS 'merge'; that is, it
 implements all of RCS 'merge''s functionality which is needed by
diff --git a/builtin/merge-file.c b/builtin/merge-file.c
index 50d0bc8..5544705 100644
--- a/builtin/merge-file.c
+++ b/builtin/merge-file.c
@@ -104,5 +104,8 @@ int cmd_merge_file(int argc, const char **argv, const char 
*prefix)
free(result.ptr);
}
 
+   if (ret > 127)
+   ret = 127;
+
return ret;
 }
diff --git a/t/t7600-merge.sh b/t/t7600-merge.sh
index 75c50ee..302e238 100755
--- a/t/t7600-merge.sh
+++ b/t/t7600-merge.sh
@@ -692,4 +692,37 @@ test_expect_success GPG 'merge --no-edit tag should skip 
editor' '
test_cmp actual expect
 '
 
+test_expect_success 'set up mod-256 conflict scenario' '
+   # 256 near-identical stanzas...
+   for i in $(test_seq 1 256); do
+   for j in 1 2 3 4 5; do
+   echo $i-$j
+   done
+   done >file &&
+   git add file &&
+   git commit -m base &&
+
+   # one side changes the first line of each to "master"
+   sed s/-1/-master/ tmp &&
+   mv tmp file &&
+   git commit -am master &&
+
+   # and the other to "side"; merging the two will
+   # yield 256 separate conflicts
+   git checkout -b side HEAD^ &&
+   sed s/-1/-side/ tmp &&
+   mv tmp file &&
+   git commit -am side
+'
+
+test_expect_success 'merge detects mod-256 conflicts (recursive)' '
+   git reset --hard &&
+   test_must_fail git merge -s recursive master
+'
+
+test_expect_success 'merge detects mod-256 conflicts (resolve)' '
+   git reset --hard &&
+   test_must_fail git merge -s resolve master
+'
+
 test_done
-- 
2.6.2.572.g6ed22dd
--
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 5/9] submodule update: expose parallelism to the user

[Junio C Hamano]

Stefan Beller  writes:

> On Tue, Oct 27, 2015 at 1:59 PM, Junio C Hamano  wrote:
>> And when 0 starts to meaning something special, we would need to
>> describe that here (and/or submodule.jobs entry in config.txt).
>> As I already said, I do not think "0 means num_cpus" is a useful
>> default, and I would prefer if we reserved 0 to mean something more
>> useful we would figure out later.
>
> Ok I'll add that, too.

Sorry, but I take it back.  We just can document that (1) "-j 0"
will give you some default, (2) we do not promise that the default
will be optimal for you from day one, (3) we reserve the right to
"improve" it over time, and (4) we promise that we won't make it an
insanely wrong value.  And let's keep "0 currently means num_cpu",
which may or may not be optimal but it cannot be an "insanely wrong"
value.
--
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 17/17] test-sha1-array: read command stream with strbuf_gets()

[Junio C Hamano]

The input to this command comes from a pipeline in t0064, whose
upstream has bunch of "echo"s.  It is not unreasonable to expect
that it may be fed CRLF lines on DOSsy systems.

Signed-off-by: Junio C Hamano 
---
 test-sha1-array.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/test-sha1-array.c b/test-sha1-array.c
index ddc491e..46ff240 100644
--- a/test-sha1-array.c
+++ b/test-sha1-array.c
@@ -11,7 +11,7 @@ int main(int argc, char **argv)
struct sha1_array array = SHA1_ARRAY_INIT;
struct strbuf line = STRBUF_INIT;
 
-   while (strbuf_getline(, stdin, '\n') != EOF) {
+   while (strbuf_gets(, stdin) != EOF) {
const char *arg;
unsigned char sha1[20];
 
-- 
2.6.2-423-g5314b62

--
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 02/17] check-attr, check-ignore, checkout-index: read paths with strbuf_gets()

[Junio C Hamano]

These commands read list of paths from their standard input under
the --stdin option (in order to avoid busting limit on the length of
the command line).

When they are using text input mode (i.e. line_termination is set to
'\n'), we should try to be more friendly to our DOSsy friends and
accept lines with CRLF endings.

It is tempting to lift this logic to strbuf_getline() and not
introduce a separate strbuf_gets(), but that can lead to silent
misconversion.

Signed-off-by: Junio C Hamano 
---
 builtin/check-attr.c | 4 +++-
 builtin/check-ignore.c   | 5 -
 builtin/checkout-index.c | 4 +++-
 3 files changed, 10 insertions(+), 3 deletions(-)

diff --git a/builtin/check-attr.c b/builtin/check-attr.c
index 265c9ba..72d4bb6 100644
--- a/builtin/check-attr.c
+++ b/builtin/check-attr.c
@@ -77,7 +77,9 @@ static void check_attr_stdin_paths(const char *prefix, int 
cnt,
 
strbuf_init(, 0);
strbuf_init(, 0);
-   while (strbuf_getline(, stdin, line_termination) != EOF) {
+   while ((line_termination
+   ? strbuf_gets(, stdin)
+   : strbuf_getline(, stdin, '\0')) != EOF) {
if (line_termination && buf.buf[0] == '"') {
strbuf_reset();
if (unquote_c_style(, buf.buf, NULL))
diff --git a/builtin/check-ignore.c b/builtin/check-ignore.c
index 43f3617..d36e9bf 100644
--- a/builtin/check-ignore.c
+++ b/builtin/check-ignore.c
@@ -122,7 +122,10 @@ static int check_ignore_stdin_paths(struct dir_struct 
*dir, const char *prefix)
 
strbuf_init(, 0);
strbuf_init(, 0);
-   while (strbuf_getline(, stdin, line_termination) != EOF) {
+
+   while ((line_termination
+   ? strbuf_gets(, stdin)
+   : strbuf_getline(, stdin, '\0')) != EOF) {
if (line_termination && buf.buf[0] == '"') {
strbuf_reset();
if (unquote_c_style(, buf.buf, NULL))
diff --git a/builtin/checkout-index.c b/builtin/checkout-index.c
index 8028c37..8b6be57 100644
--- a/builtin/checkout-index.c
+++ b/builtin/checkout-index.c
@@ -258,7 +258,9 @@ int cmd_checkout_index(int argc, const char **argv, const 
char *prefix)
if (all)
die("git checkout-index: don't mix '--all' and 
'--stdin'");
 
-   while (strbuf_getline(, stdin, line_termination) != EOF) {
+   while ((line_termination
+   ? strbuf_gets(, stdin)
+   : strbuf_getline(, stdin, '\0')) != EOF) {
char *p;
if (line_termination && buf.buf[0] == '"') {
strbuf_reset();
-- 
2.6.2-423-g5314b62

--
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 05/17] mktree: read textual tree representation with strbuf_gets()

[Junio C Hamano]

The input can come from a DOS editor.

Signed-off-by: Junio C Hamano 
---
 builtin/mktree.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/mktree.c b/builtin/mktree.c
index a964d6b..a55f067 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -157,7 +157,9 @@ int cmd_mktree(int ac, const char **av, const char *prefix)
 
while (!got_eof) {
while (1) {
-   if (strbuf_getline(, stdin, line_termination) == 
EOF) {
+   if ((line_termination
+? strbuf_gets(, stdin)
+: strbuf_getline(, stdin, '\0')) == EOF) {
got_eof = 1;
break;
}
-- 
2.6.2-423-g5314b62

--
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 12/17] transport-helper: read helper response with strbuf_gets()

[Junio C Hamano]

Our implementation of helpers never use CRLF line endings, and they
do not depend on the ability to place a CR as payload at the end of
the line, so this is essentially a no-op for in-tree users.  However,
this allows third-party implementation of helpers to give us their
line with CRLF line ending (they cannot expect us to feed CRLF to
them, though).

Signed-off-by: Junio C Hamano 
---
 transport-helper.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/transport-helper.c b/transport-helper.c
index 63d5427..e0e1c9c 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer, 
const char *name)
strbuf_reset(buffer);
if (debug)
fprintf(stderr, "Debug: Remote helper: Waiting...\n");
-   if (strbuf_getline(buffer, helper, '\n') == EOF) {
+   if (strbuf_gets(buffer, helper) == EOF) {
if (debug)
fprintf(stderr, "Debug: Remote helper quit.\n");
return 1;
-- 
2.6.2-423-g5314b62

--
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 10/17] remote.c: read $GIT_DIR/remotes/* with strbuf_gets()

[Junio C Hamano]

These files can be edited with a DOS editor, leaving CR at the end
of the line if read with strbuf_getline().

Signed-off-by: Junio C Hamano 
---
 remote.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/remote.c b/remote.c
index 1101f82..90eef22 100644
--- a/remote.c
+++ b/remote.c
@@ -256,7 +256,7 @@ static void read_remotes_file(struct remote *remote)
if (!f)
return;
remote->origin = REMOTE_REMOTES;
-   while (strbuf_getline(, f, '\n') != EOF) {
+   while (strbuf_gets(, f) != EOF) {
const char *v;
 
strbuf_rtrim();
-- 
2.6.2-423-g5314b62

--
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 14/17] column: read lines with strbuf_gets()

[Junio C Hamano]

Multiple lines read here are concatenated on a single line to form a
multi-column output line.  We do not want to have a CR at the end,
even if the input file consists of CRLF terminated lines.

Signed-off-by: Junio C Hamano 
---
 builtin/column.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/column.c b/builtin/column.c
index 449413c..e9fe928 100644
--- a/builtin/column.c
+++ b/builtin/column.c
@@ -51,7 +51,7 @@ int cmd_column(int argc, const char **argv, const char 
*prefix)
die(_("--command must be the first argument"));
}
finalize_colopts(, -1);
-   while (!strbuf_getline(, stdin, '\n'))
+   while (!strbuf_gets(, stdin))
string_list_append(, sb.buf);
 
print_columns(, colopts, );
-- 
2.6.2-423-g5314b62

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

git fsck failure on OS X with files >= 4 GiB

[Rafael Espíndola]

I first noticed this with "2.4.9 (Apple Git-60)", but it reproduces
with git built from 37023ba381b6d251d7140a997b39b566dbc63c42.

Create two files with just 0s:

-rw-r--r--  1 espindola  staff  4294967296 28 Oct 11:09 exactly-4gib
-rw-r--r--  1 espindola  staff  4294967295 28 Oct 11:09 one-less-than-4gib


and run

git init
git add one-less-than-4gib
git commit -m bar
git fsck
git add exactly-4gib
git commit -m bar
git fsck

The first fsck will run with no problems, but the second one fails:

error: packed cfdaf54c9ccfd8f5e4cee562f7d5f92df13d3106 from
.git/objects/pack/pack-ff08480fd7f767b6bd0aeb559f0f5dea2245b0b3.pack
is corrupt

Using the very same revision on freebsd doesn't cause any errors.

Cheers,
Rafael
--
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

[PATCHv2 8/8] clone: allow an explicit argument for parallel submodule clones

[Stefan Beller]

Just pass it along to "git submodule update", which may pick reasonable
defaults if you don't specify an explicit number.

Signed-off-by: Stefan Beller 
---
 Documentation/git-clone.txt |  6 +-
 builtin/clone.c | 23 +--
 t/t7406-submodule-update.sh | 15 +++
 3 files changed, 37 insertions(+), 7 deletions(-)

diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index f1f2a3f..01bd6b7 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -14,7 +14,7 @@ SYNOPSIS
  [-o ] [-b ] [-u ] [--reference ]
  [--dissociate] [--separate-git-dir ]
  [--depth ] [--[no-]single-branch]
- [--recursive | --recurse-submodules] [--] 
+ [--recursive | --recurse-submodules] [--jobs ] [--] 
  []
 
 DESCRIPTION
@@ -216,6 +216,10 @@ objects from the source repository into a pack in the 
cloned repository.
The result is Git repository can be separated from working
tree.
 
+-j ::
+--jobs ::
+   The number of submodules fetched at the same time.
+   Defaults to the `submodule.jobs` option.
 
 ::
The (possibly remote) repository to clone from.  See the
diff --git a/builtin/clone.c b/builtin/clone.c
index 9eaecd9..22b9924 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -50,6 +50,7 @@ static int option_progress = -1;
 static struct string_list option_config;
 static struct string_list option_reference;
 static int option_dissociate;
+static int max_jobs = -1;
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -72,6 +73,8 @@ static struct option builtin_clone_options[] = {
N_("initialize submodules in the clone")),
OPT_BOOL(0, "recurse-submodules", _recursive,
N_("initialize submodules in the clone")),
+   OPT_INTEGER('j', "jobs", _jobs,
+   N_("number of submodules cloned in parallel")),
OPT_STRING(0, "template", _template, N_("template-directory"),
   N_("directory from which templates will be used")),
OPT_STRING_LIST(0, "reference", _reference, N_("repo"),
@@ -95,10 +98,6 @@ static struct option builtin_clone_options[] = {
OPT_END()
 };
 
-static const char *argv_submodule[] = {
-   "submodule", "update", "--init", "--recursive", NULL
-};
-
 static const char *get_repo_path_1(struct strbuf *path, int *is_bundle)
 {
static char *suffix[] = { "/.git", "", ".git/.git", ".git" };
@@ -724,8 +723,20 @@ static int checkout(void)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive)
-   err = run_command_v_opt(argv_submodule, RUN_GIT_CMD);
+   if (!err && option_recursive) {
+   struct argv_array args = ARGV_ARRAY_INIT;
+   argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
+
+   if (max_jobs != -1) {
+   struct strbuf sb = STRBUF_INIT;
+   strbuf_addf(, "--jobs=%d", max_jobs);
+   argv_array_push(, sb.buf);
+   strbuf_release();
+   }
+
+   err = run_command_v_opt(args.argv, RUN_GIT_CMD);
+   argv_array_clear();
+   }
 
return err;
 }
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 05ea66f..ade0524 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -786,4 +786,19 @@ test_expect_success 'submodule update can be run in 
parallel' '
 grep "9 children" trace.out
)
 '
+
+test_expect_success 'git clone passes the parallel jobs config on to 
submodules' '
+   test_when_finished "rm -rf super4" &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 7 . 
super4 &&
+   grep "7 children" trace.out &&
+   rm -rf super4 &&
+   git config --global submodule.jobs 8 &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules . super4 &&
+   grep "8 children" trace.out &&
+   rm -rf super4 &&
+   GIT_TRACE=$(pwd)/trace.out git clone --recurse-submodules --jobs 9 . 
super4 &&
+   grep "9 children" trace.out &&
+   rm -rf super4
+'
+
 test_done
-- 
2.5.0.281.g4ed9cdb

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

[PATCHv2 2/8] submodule config: keep update strategy around

[Stefan Beller]

We need the submodule update strategies in a later patch.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 submodule-config.c | 11 +++
 submodule-config.h |  1 +
 2 files changed, 12 insertions(+)

diff --git a/submodule-config.c b/submodule-config.c
index afe0ea8..8b8c7d1 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -194,6 +194,7 @@ static struct submodule *lookup_or_create_by_name(struct 
submodule_cache *cache,
 
submodule->path = NULL;
submodule->url = NULL;
+   submodule->update = NULL;
submodule->fetch_recurse = RECURSE_SUBMODULES_NONE;
submodule->ignore = NULL;
 
@@ -311,6 +312,16 @@ static int parse_config(const char *var, const char 
*value, void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
+   } else if (!strcmp(item.buf, "update")) {
+   if (!value)
+   ret = config_error_nonbool(var);
+   else if (!me->overwrite && submodule->update != NULL)
+   warn_multiple_config(me->commit_sha1, submodule->name,
+"update");
+   else {
+   free((void *)submodule->update);
+   submodule->update = xstrdup(value);
+   }
}
 
strbuf_release();
diff --git a/submodule-config.h b/submodule-config.h
index 9061e4e..f9e2a29 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -14,6 +14,7 @@ struct submodule {
const char *url;
int fetch_recurse;
const char *ignore;
+   const char *update;
/* the sha1 blob id of the responsible .gitmodules file */
unsigned char gitmodules_sha1[20];
 };
-- 
2.5.0.281.g4ed9cdb

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

[PATCHv2 4/8] submodule-config: parse_config

[Stefan Beller]

This rewrites parse_config to distinguish between configs specific to
one submodule and configs which apply generically to all submodules.
We do not have generic submodule configs yet, but the next patch will
introduce "submodule.jobs".

Signed-off-by: Stefan Beller 

# Conflicts:
#   submodule-config.c

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 58 --
 1 file changed, 39 insertions(+), 19 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 4d0563c..1cea404 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -231,27 +231,23 @@ struct parse_config_parameter {
int overwrite;
 };
 
-static int parse_config(const char *var, const char *value, void *data)
+static int parse_generic_submodule_config(const char *var,
+ const char *key,
+ const char *value)
 {
-   struct parse_config_parameter *me = data;
-   struct submodule *submodule;
-   int subsection_len, ret = 0;
-   const char *subsection, *key;
-   char *name;
-
-   if (parse_config_key(var, "submodule", ,
-_len, ) < 0)
-   return 0;
-
-   if (!subsection_len)
-   return 0;
+   return 0;
+}
 
-   /* subsection is not null terminated */
-   name = xmemdupz(subsection, subsection_len);
-   submodule = lookup_or_create_by_name(me->cache,
-me->gitmodules_sha1,
-name);
-   free(name);
+static int parse_specific_submodule_config(struct parse_config_parameter *me,
+  const char *name,
+  const char *key,
+  const char *value,
+  const char *var)
+{
+   int ret = 0;
+   struct submodule *submodule = lookup_or_create_by_name(me->cache,
+  
me->gitmodules_sha1,
+  name);
 
if (!strcmp(key, "path")) {
if (!value)
@@ -318,6 +314,30 @@ static int parse_config(const char *var, const char 
*value, void *data)
return ret;
 }
 
+static int parse_config(const char *var, const char *value, void *data)
+{
+   struct parse_config_parameter *me = data;
+
+   int subsection_len;
+   const char *subsection, *key;
+   char *name;
+
+   if (parse_config_key(var, "submodule", ,
+_len, ) < 0)
+   return 0;
+
+   if (!subsection_len)
+   return parse_generic_submodule_config(var, key, value);
+   else {
+   int ret;
+   /* subsection is not null terminated */
+   name = xmemdupz(subsection, subsection_len);
+   ret = parse_specific_submodule_config(me, name, key, value, 
var);
+   free(name);
+   return ret;
+   }
+}
+
 static int gitmodule_sha1_from_commit(const unsigned char *commit_sha1,
  unsigned char *gitmodules_sha1)
 {
-- 
2.5.0.281.g4ed9cdb

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

[PATCHv2 7/8] submodule update: expose parallelism to the user

[Stefan Beller]

Expose possible parallelism either via the "--jobs" CLI parameter or
the "submodule.jobs" setting.

By having the variable initialized to -1, we make sure 0 can be passed
into the parallel processing machine, which will then pick as many parallel
workers as there are CPUs.

Signed-off-by: Stefan Beller 
---
 Documentation/git-submodule.txt |  7 ++-
 builtin/submodule--helper.c | 18 ++
 git-submodule.sh|  9 +
 t/t7406-submodule-update.sh | 12 
 4 files changed, 41 insertions(+), 5 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index f17687e..c70fafd 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -16,7 +16,7 @@ SYNOPSIS
 'git submodule' [--quiet] deinit [-f|--force] [--] ...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
  [-f|--force] [--rebase|--merge] [--reference ]
- [--depth ] [--recursive] [--] [...]
+ [--depth ] [--recursive] [--jobs ] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -374,6 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
+-j ::
+--jobs ::
+   This option is only valid for the update command.
+   Clone new submodules in parallel with as many jobs.
+   Defaults to the `submodule.jobs` option.
 
 ...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 1ec1b85..67dba1c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -431,6 +431,7 @@ static int update_clone_task_finished(int result,
 
 static int update_clone(int argc, const char **argv, const char *prefix)
 {
+   int max_jobs = -1;
struct string_list_item *item;
struct submodule_update_clone pp = SUBMODULE_UPDATE_CLONE_INIT;
 
@@ -451,6 +452,8 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
OPT_STRING(0, "depth", , "",
   N_("Create a shallow clone truncated to the "
  "specified number of revisions")),
+   OPT_INTEGER('j', "jobs", _jobs,
+   N_("parallel jobs")),
OPT__QUIET(, N_("do't print cloning progress")),
OPT_END()
};
@@ -472,10 +475,17 @@ static int update_clone(int argc, const char **argv, 
const char *prefix)
gitmodules_config();
/* Overlay the parsed .gitmodules file with .git/config */
git_config(git_submodule_config, NULL);
-   run_processes_parallel(1, update_clone_get_next_task,
- update_clone_start_failure,
- update_clone_task_finished,
- );
+
+   if (max_jobs < 0)
+   max_jobs = config_parallel_submodules();
+   if (max_jobs < 0)
+   max_jobs = 1;
+
+   run_processes_parallel(max_jobs,
+  update_clone_get_next_task,
+  update_clone_start_failure,
+  update_clone_task_finished,
+  );
 
if (pp.print_unmatched) {
printf("#unmatched\n");
diff --git a/git-submodule.sh b/git-submodule.sh
index 9f554fb..10c5af9 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -645,6 +645,14 @@ cmd_update()
--depth=*)
depth=$1
;;
+   -j|--jobs)
+   case "$2" in '') usage ;; esac
+   jobs="--jobs=$2"
+   shift
+   ;;
+   --jobs=*)
+   jobs=$1
+   ;;
--)
shift
break
@@ -670,6 +678,7 @@ cmd_update()
${update:+--update "$update"} \
${reference:+--reference "$reference"} \
${depth:+--depth "$depth"} \
+   ${jobs:+$jobs} \
"$@" | {
err=
while read mode sha1 stage just_cloned sm_path
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index dda3929..05ea66f 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -774,4 +774,16 @@ test_expect_success 'submodule update --recursive drops 
module name before recur
 test_i18ngrep "Submodule path .deeper/submodule/subsubmodule.: checked 
out" actual
)
 '
+
+test_expect_success 'submodule update can be run in parallel' '
+   (cd super2 &&
+

[PATCHv2 6/8] git submodule update: have a dedicated helper for cloning

[Stefan Beller]

This introduces a new helper function in git submodule--helper
which takes care of cloning all submodules, which we want to
parallelize eventually.

Some tests (such as empty URL, update_mode=none) are required in the
helper to make the decision for cloning. These checks have been
moved into the C function as well (no need to repeat them in the
shell script).

As we can only access the stderr channel from within the parallel
processing engine, we need to reroute the error message for
specified but initialized submodules to stderr. As it is an error
message, this should have gone to stderr in the first place, so it
is a bug fix along the way.

Signed-off-by: Stefan Beller 
Signed-off-by: Junio C Hamano 
---
 builtin/submodule--helper.c | 234 
 git-submodule.sh|  45 +++--
 t/t7400-submodule-basic.sh  |   4 +-
 3 files changed, 247 insertions(+), 36 deletions(-)

diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index f4c3eff..1ec1b85 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -255,6 +255,239 @@ static int module_clone(int argc, const char **argv, 
const char *prefix)
return 0;
 }
 
+static int git_submodule_config(const char *var, const char *value, void *cb)
+{
+   return parse_submodule_config_option(var, value);
+}
+
+struct submodule_update_clone {
+   int count;
+   int quiet;
+   int print_unmatched;
+   char *reference;
+   char *depth;
+   char *update;
+   const char *recursive_prefix;
+   const char *prefix;
+   struct module_list list;
+   struct string_list projectlines;
+   struct pathspec pathspec;
+};
+#define SUBMODULE_UPDATE_CLONE_INIT {0, 0, 0, NULL, NULL, NULL, NULL, NULL, 
MODULE_LIST_INIT, STRING_LIST_INIT_DUP}
+
+static void fill_clone_command(struct child_process *cp, int quiet,
+  const char *prefix, const char *path,
+  const char *name, const char *url,
+  const char *reference, const char *depth)
+{
+   cp->git_cmd = 1;
+   cp->no_stdin = 1;
+   cp->stdout_to_stderr = 1;
+   cp->err = -1;
+   argv_array_push(>args, "submodule--helper");
+   argv_array_push(>args, "clone");
+   if (quiet)
+   argv_array_push(>args, "--quiet");
+
+   if (prefix) {
+   argv_array_push(>args, "--prefix");
+   argv_array_push(>args, prefix);
+   }
+   argv_array_push(>args, "--path");
+   argv_array_push(>args, path);
+
+   argv_array_push(>args, "--name");
+   argv_array_push(>args, name);
+
+   argv_array_push(>args, "--url");
+   argv_array_push(>args, url);
+   if (reference)
+   argv_array_push(>args, reference);
+   if (depth)
+   argv_array_push(>args, depth);
+}
+
+static int update_clone_get_next_task(void **pp_task_cb,
+ struct child_process *cp,
+ struct strbuf *err,
+ void *pp_cb)
+{
+   struct submodule_update_clone *pp = pp_cb;
+
+   for (; pp->count < pp->list.nr; pp->count++) {
+   const struct submodule *sub = NULL;
+   const char *displaypath = NULL;
+   const struct cache_entry *ce = pp->list.entries[pp->count];
+   struct strbuf sb = STRBUF_INIT;
+   const char *update_module = NULL;
+   char *url = NULL;
+   int just_cloned = 0;
+
+   if (ce_stage(ce)) {
+   if (pp->recursive_prefix)
+   strbuf_addf(err, "Skipping unmerged submodule 
%s/%s\n",
+   pp->recursive_prefix, ce->name);
+   else
+   strbuf_addf(err, "Skipping unmerged submodule 
%s\n",
+   ce->name);
+   continue;
+   }
+
+   sub = submodule_from_path(null_sha1, ce->name);
+   if (!sub) {
+   strbuf_addf(err, "BUG: internal error managing 
submodules. "
+   "The cache could not locate '%s'", 
ce->name);
+   pp->print_unmatched = 1;
+   return 0;
+   }
+
+   if (pp->recursive_prefix)
+   displaypath = relative_path(pp->recursive_prefix, 
ce->name, );
+   else
+   displaypath = ce->name;
+
+   if (pp->update)
+   update_module = pp->update;
+   if (!update_module)
+   update_module = sub->update;
+   if (!update_module)
+   update_module = "checkout";
+   if (!strcmp(update_module, "none")) {
+  

git-fetch pulls already-pulled objects?

[Matt Glazar]

On a remote, I have two Git commit objects which point to the same tree
object (created with git commit-tree). If I fetch one of the commits, the
commit object (including the tree object) is fetched. If I then fetch the
other commit, the tree object (and its dependencies) is fetched *again* (I
think). I don't watch the tree object downloaded again, because it is
large (multi-gigabyte). Because the tree object exists locally, I think it
should not be downloaded.

Is this a bug in Git, or is this by design? How can I confirm that the
tree object (and dependencies) are downloaded twice? Is there are more
complicated git-fetch (or similar) command I can execute to not download
the already-downloaded tree objects? (I have the hash of the tree object
which would be potentially re-downloaded, if that helps.)

Sequence of commands to reproduce:

# Replace this with the URL to an empty Git repository.
remote=ssh://foo/bar.git

# Create some random data to exaggerate git-fetch times.
# If you have a slow remote, reduce 'count'.
mkdir minimal
cd minimal
dd if=/dev/urandom of=random bs=65536 count=4096

# Create our two commits (master and master2).
git init
git add random
git commit -m 'Random data (commit 1)'
git branch master2 \
  "$(echo 'Random data (commit 2)' \
| git commit-tree 'HEAD^{tree}')"

# Push our commits. Expected to take some time.
git remote add origin "${remote}"
git push origin \
  master:refs/heads/master \
  master2:refs/heads/master2

# Clone master. Expected to take some time.
cd ..
mkdir minimal-clone
git clone --single-branch --branch master "${remote}"

# Fetch master2. Should be nearly instant, but takes some
# time. Seems to be download everything again.
cd minimal-clone
git fetch origin master2

# Try again. git-fetch takes a while, but shouldn't.
rm -f .git/FETCH_HEAD
git gc --prune=all
git fetch origin master2

Info about my system:


Local (pusher):
OS: OS X 10.10.5
git: git version 2.0.1
ssh: OpenSSH_6.2p2, OSSLShim 0.9.8r 8 Dec 2011


Remote (server):
OS: Linux 4.0.9 (CentOS 6)
git: git version 2.4.6
sshd: OpenSSH_6.7p1-hpn14v5, OpenSSL 1.0.1e-fips 11 Feb 2013

[PATCHv2 1/8] run_processes_parallel: Add output to tracing messages

[Stefan Beller]

This commit serves 2 purposes. First this may help the user who
tries to diagnose intermixed process calls. Second this may be used
in a later patch for testing. As the output of a command should not
change visibly except for going faster, grepping for the trace output
seems like a viable testing strategy.

Signed-off-by: Stefan Beller 
---
 run-command.c | 4 
 1 file changed, 4 insertions(+)

diff --git a/run-command.c b/run-command.c
index 82cc238..49dec74 100644
--- a/run-command.c
+++ b/run-command.c
@@ -959,6 +959,9 @@ static struct parallel_processes *pp_init(int n,
n = online_cpus();
 
pp->max_processes = n;
+
+   trace_printf("run_processes_parallel: preparing to run up to %d 
children in parallel", n);
+
pp->data = data;
if (!get_next_task)
die("BUG: you need to specify a get_next_task function");
@@ -988,6 +991,7 @@ static void pp_cleanup(struct parallel_processes *pp)
 {
int i;
 
+   trace_printf("run_processes_parallel: parallel processing done");
for (i = 0; i < pp->max_processes; i++) {
strbuf_release(>children[i].err);
child_process_deinit(>children[i].process);
-- 
2.5.0.281.g4ed9cdb

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

[PATCHv2 5/8] fetching submodules: Respect `submodule.jobs` config option

[Stefan Beller]

This allows to configure fetching and updating in parallel
without having the command line option.

This moved the responsibility to determine how many parallel processes
to start from builtin/fetch to submodule.c as we need a way to communicate
"The user did not specify the number of parallel processes in the command
line options" in the builtin fetch. The submodule code takes care of
the precedence (CLI > config > default)

Signed-off-by: Stefan Beller 
---
 Documentation/config.txt|  7 +++
 builtin/fetch.c |  2 +-
 submodule-config.c  |  9 +
 submodule-config.h  |  2 ++
 submodule.c |  5 +
 t/t5526-fetch-submodules.sh | 14 ++
 6 files changed, 38 insertions(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 391a0c3..785721a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2643,6 +2643,13 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
 
+submodule.jobs::
+   This is used to determine how many submodules can be operated on in
+   parallel. Specifying a positive integer allows up to that number
+   of submodules being fetched in parallel. This is used in fetch
+   and clone operations only. A value of 0 will give some reasonable
+   default. The defaults may change with different versions of Git.
+
 tag.sort::
This variable controls the sort ordering of tags when displayed by
linkgit:git-tag[1]. Without the "--sort=" option provided, the
diff --git a/builtin/fetch.c b/builtin/fetch.c
index 9cc1c9d..60e6797 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -37,7 +37,7 @@ static int prune = -1; /* unspecified */
 static int all, append, dry_run, force, keep, multiple, update_head_ok, 
verbosity;
 static int progress = -1, recurse_submodules = RECURSE_SUBMODULES_DEFAULT;
 static int tags = TAGS_DEFAULT, unshallow, update_shallow;
-static int max_children = 1;
+static int max_children = -1;
 static const char *depth;
 static const char *upload_pack;
 static struct strbuf default_rla = STRBUF_INIT;
diff --git a/submodule-config.c b/submodule-config.c
index 1cea404..07bdcdf 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -32,6 +32,7 @@ enum lookup_type {
 
 static struct submodule_cache cache;
 static int is_cache_init;
+static int parallel_jobs = -1;
 
 static int config_path_cmp(const struct submodule_entry *a,
   const struct submodule_entry *b,
@@ -235,6 +236,9 @@ static int parse_generic_submodule_config(const char *var,
  const char *key,
  const char *value)
 {
+   if (!strcmp(key, "jobs")) {
+   parallel_jobs = strtol(value, NULL, 10);
+   }
return 0;
 }
 
@@ -483,3 +487,8 @@ void submodule_free(void)
cache_free();
is_cache_init = 0;
 }
+
+int config_parallel_submodules(void)
+{
+   return parallel_jobs;
+}
diff --git a/submodule-config.h b/submodule-config.h
index f9e2a29..d9bbf9a 100644
--- a/submodule-config.h
+++ b/submodule-config.h
@@ -27,4 +27,6 @@ const struct submodule *submodule_from_path(const unsigned 
char *commit_sha1,
const char *path);
 void submodule_free(void);
 
+int config_parallel_submodules(void);
+
 #endif /* SUBMODULE_CONFIG_H */
diff --git a/submodule.c b/submodule.c
index 0257ea3..188ba02 100644
--- a/submodule.c
+++ b/submodule.c
@@ -752,6 +752,11 @@ int fetch_populated_submodules(const struct argv_array 
*options,
argv_array_push(, "--recurse-submodules-default");
/* default value, "--submodule-prefix" and its value are added later */
 
+   if (max_parallel_jobs < 0)
+   max_parallel_jobs = config_parallel_submodules();
+   if (max_parallel_jobs < 0)
+   max_parallel_jobs = 1;
+
calculate_changed_submodule_paths();
run_processes_parallel(max_parallel_jobs,
   get_next_submodule,
diff --git a/t/t5526-fetch-submodules.sh b/t/t5526-fetch-submodules.sh
index 1b4ce69..5c3579c 100755
--- a/t/t5526-fetch-submodules.sh
+++ b/t/t5526-fetch-submodules.sh
@@ -470,4 +470,18 @@ test_expect_success "don't fetch submodule when newly 
recorded commits are alrea
test_i18ncmp expect.err actual.err
 '
 
+test_expect_success 'fetching submodules respects parallel settings' '
+   git config fetch.recurseSubmodules true &&
+   (
+   cd downstream &&
+   GIT_TRACE=$(pwd)/trace.out git fetch --jobs 7 &&
+   grep "7 children" trace.out &&
+   git config submodule.jobs 8 &&
+   GIT_TRACE=$(pwd)/trace.out git fetch &&
+   grep "8 children" trace.out &&
+   GIT_TRACE=$(pwd)/trace.out git fetch --jobs 9 &&
+   grep "9 children" trace.out
+   )

[PATCHv2 0/8] Expose the submodule parallelism to the user

[Stefan Beller]

This replaces origin/sb/submodule-parallel-update
(anchoring at 74367d8938, Merge branch 'sb/submodule-parallel-fetch'
into sb/submodule-parallel-update)

What does it do?
---
This series should finish the on going efforts of parallelizing
submodule network traffic. The patches contain tests for clone,
fetch and submodule update to use the actual parallelism both via
command line as well as a configured option. I decided to go with
"submodule.jobs" for all three for now.

What is new in v2?
---
* The patches got reordered slightly
* Documentation was adapted

Interdiff below

Stefan Beller (8):
  run_processes_parallel: Add output to tracing messages
  submodule config: keep update strategy around
  submodule config: remove name_and_item_from_var
  submodule-config: parse_config
  fetching submodules: Respect `submodule.jobs` config option
  git submodule update: have a dedicated helper for cloning
  submodule update: expose parallelism to the user
  clone: allow an explicit argument for parallel submodule clones

 Documentation/config.txt|   7 ++
 Documentation/git-clone.txt |   6 +-
 Documentation/git-submodule.txt |   7 +-
 builtin/clone.c |  23 +++-
 builtin/fetch.c |   2 +-
 builtin/submodule--helper.c | 244 
 git-submodule.sh|  54 -
 run-command.c   |   4 +
 submodule-config.c  |  98 ++--
 submodule-config.h  |   3 +
 submodule.c |   5 +
 t/t5526-fetch-submodules.sh |  14 +++
 t/t7400-submodule-basic.sh  |   4 +-
 t/t7406-submodule-update.sh |  27 +
 14 files changed, 418 insertions(+), 80 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 0de0138..785721a 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2643,12 +2643,12 @@ submodule..ignore::
"--ignore-submodules" option. The 'git submodule' commands are not
affected by this setting.
 
-submodule::jobs
+submodule.jobs::
This is used to determine how many submodules can be operated on in
parallel. Specifying a positive integer allows up to that number
-   of submodules being fetched in parallel. Specifying 0 the number
-   of cpus will be taken as the maximum number. Currently this is
-   used in fetch and clone operations only.
+   of submodules being fetched in parallel. This is used in fetch
+   and clone operations only. A value of 0 will give some reasonable
+   default. The defaults may change with different versions of Git.
 
 tag.sort::
This variable controls the sort ordering of tags when displayed by
diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
index affa52e..01bd6b7 100644
--- a/Documentation/git-clone.txt
+++ b/Documentation/git-clone.txt
@@ -216,9 +216,10 @@ objects from the source repository into a pack in the 
cloned repository.
The result is Git repository can be separated from working
tree.
 
--j::
---jobs::
+-j ::
+--jobs ::
The number of submodules fetched at the same time.
+   Defaults to the `submodule.jobs` option.
 
 ::
The (possibly remote) repository to clone from.  See the
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index f5429fa..c70fafd 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -374,10 +374,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
--j::
---jobs::
+-j ::
+--jobs ::
This option is only valid for the update command.
Clone new submodules in parallel with as many jobs.
+   Defaults to the `submodule.jobs` option.
 
 ...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/builtin/clone.c b/builtin/clone.c
index 5ac2d89..22b9924 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -727,10 +727,7 @@ static int checkout(void)
struct argv_array args = ARGV_ARRAY_INIT;
argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
 
-   if (max_jobs == -1)
-   if (git_config_get_int("submodule.jobs", _jobs))
-   max_jobs = 1;
-   if (max_jobs != 1) {
+   if (max_jobs != -1) {
struct strbuf sb = STRBUF_INIT;
strbuf_addf(, "--jobs=%d", max_jobs);
argv_array_push(, sb.buf);
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index c3d438a..67dba1c 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -476,9 +476,10 @@ static int update_clone(int argc, const char **argv, const 
char *prefix)
/* Overlay the parsed .gitmodules 

[PATCHv2 3/8] submodule config: remove name_and_item_from_var

[Stefan Beller]

By inlining `name_and_item_from_var` it is easy to add later options
which are not required to have a submodule name.

Signed-off-by: Stefan Beller 
---
 submodule-config.c | 46 +-
 1 file changed, 17 insertions(+), 29 deletions(-)

diff --git a/submodule-config.c b/submodule-config.c
index 8b8c7d1..4d0563c 100644
--- a/submodule-config.c
+++ b/submodule-config.c
@@ -161,22 +161,6 @@ static struct submodule *cache_lookup_name(struct 
submodule_cache *cache,
return NULL;
 }
 
-static int name_and_item_from_var(const char *var, struct strbuf *name,
- struct strbuf *item)
-{
-   const char *subsection, *key;
-   int subsection_len, parse;
-   parse = parse_config_key(var, "submodule", ,
-   _len, );
-   if (parse < 0 || !subsection)
-   return 0;
-
-   strbuf_add(name, subsection, subsection_len);
-   strbuf_addstr(item, key);
-
-   return 1;
-}
-
 static struct submodule *lookup_or_create_by_name(struct submodule_cache 
*cache,
const unsigned char *gitmodules_sha1, const char *name)
 {
@@ -251,18 +235,25 @@ static int parse_config(const char *var, const char 
*value, void *data)
 {
struct parse_config_parameter *me = data;
struct submodule *submodule;
-   struct strbuf name = STRBUF_INIT, item = STRBUF_INIT;
-   int ret = 0;
+   int subsection_len, ret = 0;
+   const char *subsection, *key;
+   char *name;
 
-   /* this also ensures that we only parse submodule entries */
-   if (!name_and_item_from_var(var, , ))
+   if (parse_config_key(var, "submodule", ,
+_len, ) < 0)
return 0;
 
+   if (!subsection_len)
+   return 0;
+
+   /* subsection is not null terminated */
+   name = xmemdupz(subsection, subsection_len);
submodule = lookup_or_create_by_name(me->cache,
 me->gitmodules_sha1,
-name.buf);
+name);
+   free(name);
 
-   if (!strcmp(item.buf, "path")) {
+   if (!strcmp(key, "path")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->path != NULL)
@@ -275,7 +266,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
submodule->path = xstrdup(value);
cache_put_path(me->cache, submodule);
}
-   } else if (!strcmp(item.buf, "fetchrecursesubmodules")) {
+   } else if (!strcmp(key, "fetchrecursesubmodules")) {
/* when parsing worktree configurations we can die early */
int die_on_error = is_null_sha1(me->gitmodules_sha1);
if (!me->overwrite &&
@@ -286,7 +277,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
submodule->fetch_recurse = parse_fetch_recurse(
var, value,
die_on_error);
-   } else if (!strcmp(item.buf, "ignore")) {
+   } else if (!strcmp(key, "ignore")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->ignore != NULL)
@@ -302,7 +293,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
free((void *) submodule->ignore);
submodule->ignore = xstrdup(value);
}
-   } else if (!strcmp(item.buf, "url")) {
+   } else if (!strcmp(key, "url")) {
if (!value) {
ret = config_error_nonbool(var);
} else if (!me->overwrite && submodule->url != NULL) {
@@ -312,7 +303,7 @@ static int parse_config(const char *var, const char *value, 
void *data)
free((void *) submodule->url);
submodule->url = xstrdup(value);
}
-   } else if (!strcmp(item.buf, "update")) {
+   } else if (!strcmp(key, "update")) {
if (!value)
ret = config_error_nonbool(var);
else if (!me->overwrite && submodule->update != NULL)
@@ -324,9 +315,6 @@ static int parse_config(const char *var, const char *value, 
void *data)
}
}
 
-   strbuf_release();
-   strbuf_release();
-
return ret;
 }
 
-- 
2.5.0.281.g4ed9cdb

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