[PATCH v5 1/3] merge-recursive: test rename threshold option

[Felipe Gonçalves Assis]

Commit 10ae7526bebb505ba01f76ec97d5f7b5e0e5 introduced this feature,
but did not include any tests. This commit fixes this.

Signed-off-by: Felipe Gonçalves Assis 
---

This commit is independent of the proposed feature, so it might be of interest
even if the rest of the patch is rejected.

 t/t3034-merge-recursive-rename-threshold.sh | 146 
 1 file changed, 146 insertions(+)
 create mode 100755 t/t3034-merge-recursive-rename-threshold.sh

diff --git a/t/t3034-merge-recursive-rename-threshold.sh 
b/t/t3034-merge-recursive-rename-threshold.sh
new file mode 100755
index 000..f0b3f44
--- /dev/null
+++ b/t/t3034-merge-recursive-rename-threshold.sh
@@ -0,0 +1,146 @@
+#!/bin/sh
+
+test_description='merge-recursive rename threshold option
+
+Test rename detection by examining rename/delete conflicts.
+
+Similarity index:
+R100 a-old a-new
+R075 b-old b-new
+R050 c-old c-new
+R025 d-old d-new
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   get_expected_stages () {
+   git checkout rename -- $1-new &&
+   git ls-files --stage $1-new > expected-stages-undetected-$1
+   sed "s/ 0   / 2 /
+   " < expected-stages-undetected-$1 > expected-stages-detected-$1
+   git read-tree -u --reset HEAD
+   } &&
+
+   rename_detected () {
+   git ls-files --stage $1-old $1-new > stages-actual-$1 &&
+   test_cmp expected-stages-detected-$1 stages-actual-$1
+   } &&
+
+   rename_undetected () {
+   git ls-files --stage $1-old $1-new > stages-actual-$1 &&
+   test_cmp expected-stages-undetected-$1 stages-actual-$1
+   } &&
+
+   check_common () {
+   git ls-files --stage > stages-actual &&
+   test $(wc -l < stages-actual) -eq 4
+   } &&
+
+   check_find_renames_25 () {
+   check_common &&
+   rename_detected a &&
+   rename_detected b &&
+   rename_detected c &&
+   rename_detected d
+   } &&
+
+   check_find_renames_50 () {
+   check_common
+   rename_detected a &&
+   rename_detected b &&
+   rename_detected c &&
+   rename_undetected d
+   } &&
+
+   check_find_renames_75 () {
+   check_common
+   rename_detected a &&
+   rename_detected b &&
+   rename_undetected c &&
+   rename_undetected d
+   } &&
+
+   check_find_renames_100 () {
+   check_common
+   rename_detected a &&
+   rename_undetected b &&
+   rename_undetected c &&
+   rename_undetected d
+   } &&
+
+   check_no_renames () {
+   check_common
+   rename_undetected a &&
+   rename_undetected b &&
+   rename_undetected c &&
+   rename_undetected d
+   } &&
+
+   cat <<-\EOF > a-old &&
+   aa1
+   aa2
+   aa3
+   aa4
+   EOF
+   sed s/aa/bb/ < a-old > b-old &&
+   sed s/aa/cc/ < a-old > c-old &&
+   sed s/aa/dd/ < a-old > d-old &&
+   git add [a-d]-old &&
+   test_tick &&
+   git commit -m base &&
+   git rm [a-d]-old &&
+   test_tick &&
+   git commit -m delete &&
+   git checkout -b rename HEAD^ &&
+   cp a-old a-new &&
+   sed 1,1s/./x/ < b-old > b-new &&
+   sed 1,2s/./x/ < c-old > c-new &&
+   sed 1,3s/./x/ < d-old > d-new &&
+   git add [a-d]-new &&
+   git rm [a-d]-old &&
+   test_tick &&
+   git commit -m rename &&
+   get_expected_stages a &&
+   get_expected_stages b &&
+   get_expected_stages c &&
+   get_expected_stages d
+'
+
+test_expect_success 'the default similarity index is 50%' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive HEAD^ -- HEAD master &&
+   check_find_renames_50
+'
+
+test_expect_success 'low rename threshold' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD 
master &&
+   check_find_renames_25
+'
+
+test_expect_success 'high rename threshold' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --rename-threshold=75 HEAD^ -- HEAD 
master &&
+   check_find_renames_75
+'
+
+test_expect_success 'exact renames only' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --rename-threshold=100% HEAD^ -- 
HEAD master &&
+   check_find_renames_100
+'
+
+test_expect_success 'rename threshold is truncated' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- 
HEAD master &&
+   check_find_renames_100
+'
+
+test_expect_success 'last wins in --rename-threshold= 
--rename-threshold=' '
+   git read-tree --reset 

[PATCH v5 2/3] merge-recursive: option to disable renames

[Felipe Gonçalves Assis]

The recursive strategy turns on rename detection by default. Add a
strategy option to disable rename detection even for exact renames.

Signed-off-by: Felipe Gonçalves Assis 
---

Added tests.

For consistent naming, I renamed and slightly edited the test of whitespace
options:
t/t3032-merge-recursive-options.sh -> t/t3032-merge-recursive-space-options.sh

 Documentation/merge-strategies.txt   |  6 ++
 merge-recursive.c|  7 +++
 merge-recursive.h|  1 +
 ...ons.sh => t3032-merge-recursive-space-options.sh} |  2 +-
 ...ld.sh => t3034-merge-recursive-rename-options.sh} | 20 +++-
 5 files changed, 34 insertions(+), 2 deletions(-)
 rename t/{t3032-merge-recursive-options.sh => 
t3032-merge-recursive-space-options.sh} (99%)
 rename t/{t3034-merge-recursive-rename-threshold.sh => 
t3034-merge-recursive-rename-options.sh} (83%)

diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 7bbd19b..1a5e197 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -81,8 +81,14 @@ no-renormalize;;
Disables the `renormalize` option.  This overrides the
`merge.renormalize` configuration variable.
 
+no-renames;;
+   Turn off rename detection.
+   See also linkgit:git-diff[1] `--no-renames`.
+
 rename-threshold=;;
Controls the similarity threshold used for rename detection.
+   Re-enables rename detection if disabled by a preceding
+   `no-renames`.
See also linkgit:git-diff[1] `-M`.
 
 subtree[=];;
diff --git a/merge-recursive.c b/merge-recursive.c
index 8eabde2..6dd0a11 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -482,6 +482,9 @@ static struct string_list *get_renames(struct merge_options 
*o,
struct diff_options opts;
 
renames = xcalloc(1, sizeof(struct string_list));
+   if (!o->detect_rename)
+   return renames;
+
diff_setup();
DIFF_OPT_SET(, RECURSIVE);
DIFF_OPT_CLR(, RENAME_EMPTY);
@@ -2039,6 +2042,7 @@ void init_merge_options(struct merge_options *o)
o->diff_rename_limit = -1;
o->merge_rename_limit = -1;
o->renormalize = 0;
+   o->detect_rename = 1;
merge_recursive_config(o);
if (getenv("GIT_MERGE_VERBOSITY"))
o->verbosity =
@@ -2088,9 +2092,12 @@ int parse_merge_opt(struct merge_options *o, const char 
*s)
o->renormalize = 1;
else if (!strcmp(s, "no-renormalize"))
o->renormalize = 0;
+   else if (!strcmp(s, "no-renames"))
+   o->detect_rename = 0;
else if (skip_prefix(s, "rename-threshold=", )) {
if ((o->rename_score = parse_rename_score()) == -1 || *arg 
!= 0)
return -1;
+   o->detect_rename = 1;
}
else
return -1;
diff --git a/merge-recursive.h b/merge-recursive.h
index 9e090a3..52f0201 100644
--- a/merge-recursive.h
+++ b/merge-recursive.h
@@ -17,6 +17,7 @@ struct merge_options {
unsigned renormalize : 1;
long xdl_opts;
int verbosity;
+   int detect_rename;
int diff_rename_limit;
int merge_rename_limit;
int rename_score;
diff --git a/t/t3032-merge-recursive-options.sh 
b/t/t3032-merge-recursive-space-options.sh
similarity index 99%
rename from t/t3032-merge-recursive-options.sh
rename to t/t3032-merge-recursive-space-options.sh
index 4029c9c..b56180e 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-space-options.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='merge-recursive options
+test_description='merge-recursive space options
 
 * [master] Clarify
  ! [remote] Remove cruft
diff --git a/t/t3034-merge-recursive-rename-threshold.sh 
b/t/t3034-merge-recursive-rename-options.sh
similarity index 83%
rename from t/t3034-merge-recursive-rename-threshold.sh
rename to t/t3034-merge-recursive-rename-options.sh
index f0b3f44..2f10fa7 100755
--- a/t/t3034-merge-recursive-rename-threshold.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -1,6 +1,6 @@
 #!/bin/sh
 
-test_description='merge-recursive rename threshold option
+test_description='merge-recursive rename options
 
 Test rename detection by examining rename/delete conflicts.
 
@@ -137,10 +137,28 @@ test_expect_success 'rename threshold is truncated' '
check_find_renames_100
 '
 
+test_expect_success 'disabled rename detection' '
+   git read-tree --reset -u HEAD &&
+   git merge-recursive --no-renames HEAD^ -- HEAD master &&
+   check_no_renames
+'
+
 test_expect_success 'last wins in --rename-threshold= 
--rename-threshold=' '
git read-tree --reset -u HEAD &&
test_must_fail git merge-recursive --rename-threshold=25 
--rename-threshold=75 HEAD^ -- HEAD master &&
check_find_renames_75
 '
 
+test_expect_success 

[PATCH v5 3/3] merge-recursive: more consistent interface

[Felipe Gonçalves Assis]

Add strategy option find-renames, following git-diff interface. This
makes the option rename-threshold redundant.

Signed-off-by: Felipe Gonçalves Assis 
---

Added tests and made --find-renames reset the similarity index to the default.

 Documentation/merge-strategies.txt| 10 ---
 merge-recursive.c |  7 -
 t/t3034-merge-recursive-rename-options.sh | 48 +++
 3 files changed, 54 insertions(+), 11 deletions(-)

diff --git a/Documentation/merge-strategies.txt 
b/Documentation/merge-strategies.txt
index 1a5e197..2eb92b9 100644
--- a/Documentation/merge-strategies.txt
+++ b/Documentation/merge-strategies.txt
@@ -85,11 +85,13 @@ no-renames;;
Turn off rename detection.
See also linkgit:git-diff[1] `--no-renames`.
 
+find-renames[=];;
+   Turn on rename detection, optionally setting the similarity
+   threshold.  This is the default.
+   See also linkgit:git-diff[1] `--find-renames`.
+
 rename-threshold=;;
-   Controls the similarity threshold used for rename detection.
-   Re-enables rename detection if disabled by a preceding
-   `no-renames`.
-   See also linkgit:git-diff[1] `-M`.
+   Deprecated synonym for `find-renames=`.
 
 subtree[=];;
This option is a more advanced form of 'subtree' strategy, where
diff --git a/merge-recursive.c b/merge-recursive.c
index 6dd0a11..63b8ba8 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -2094,7 +2094,12 @@ int parse_merge_opt(struct merge_options *o, const char 
*s)
o->renormalize = 0;
else if (!strcmp(s, "no-renames"))
o->detect_rename = 0;
-   else if (skip_prefix(s, "rename-threshold=", )) {
+   else if (!strcmp(s, "find-renames")) {
+   o->detect_rename = 1;
+   o->rename_score = 0;
+   }
+   else if (skip_prefix(s, "find-renames=", ) ||
+skip_prefix(s, "rename-threshold=", )) {
if ((o->rename_score = parse_rename_score()) == -1 || *arg 
!= 0)
return -1;
o->detect_rename = 1;
diff --git a/t/t3034-merge-recursive-rename-options.sh 
b/t/t3034-merge-recursive-rename-options.sh
index 2f10fa7..4e3fefd 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -115,25 +115,25 @@ test_expect_success 'the default similarity index is 50%' 
'
 
 test_expect_success 'low rename threshold' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=25 HEAD^ -- HEAD 
master &&
+   test_must_fail git merge-recursive --find-renames=25 HEAD^ -- HEAD 
master &&
check_find_renames_25
 '
 
 test_expect_success 'high rename threshold' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=75 HEAD^ -- HEAD 
master &&
+   test_must_fail git merge-recursive --find-renames=75 HEAD^ -- HEAD 
master &&
check_find_renames_75
 '
 
 test_expect_success 'exact renames only' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=100% HEAD^ -- 
HEAD master &&
+   test_must_fail git merge-recursive --find-renames=100% HEAD^ -- HEAD 
master &&
check_find_renames_100
 '
 
 test_expect_success 'rename threshold is truncated' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=200% HEAD^ -- 
HEAD master &&
+   test_must_fail git merge-recursive --find-renames=200% HEAD^ -- HEAD 
master &&
check_find_renames_100
 '
 
@@ -143,12 +143,36 @@ test_expect_success 'disabled rename detection' '
check_no_renames
 '
 
-test_expect_success 'last wins in --rename-threshold= 
--rename-threshold=' '
+test_expect_success 'last wins in --find-renames= --find-renames=' '
git read-tree --reset -u HEAD &&
-   test_must_fail git merge-recursive --rename-threshold=25 
--rename-threshold=75 HEAD^ -- HEAD master &&
+   test_must_fail git merge-recursive --find-renames=25 --find-renames=75 
HEAD^ -- HEAD master &&
check_find_renames_75
 '
 
+test_expect_success '--find-renames is equivalent to --find-renames=5' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --find-renames=25 --find-renames 
HEAD^ -- HEAD master &&
+   check_find_renames_50
+'
+
+test_expect_success 'last wins in --no-renames --find-renames' '
+   git read-tree --reset -u HEAD &&
+   test_must_fail git merge-recursive --no-renames --find-renames HEAD^ -- 
HEAD master &&
+   check_find_renames_50
+'
+
+test_expect_success 'last wins in --find-renames --no-renames' '
+   git read-tree --reset -u HEAD &&
+   git merge-recursive --find-renames --no-renames HEAD^ -- HEAD master &&
+   check_no_renames
+'
+
+test_expect_success 'rename-threshold= is a synonym for find-renames=' '

[PATCH v5 0/3] merge-recursive: option to disable renames

[Felipe Gonçalves Assis]

Add tests as suggested by Eric Sunshine. Also fixes an inconsitency in the last
part.

Since there were no tests for the --rename-threshold option, I added them in a
separate commit, which is useful in itself.

The other two commits contain the previous patches plus the relevant tests.

For the last part, I made --find-renames (without =) reset the similarity
index to the default, just as in git-diff.

Felipe Gonçalves Assis (3):
  merge-recursive: test rename threshold option
  merge-recursive: option to disable renames
  merge-recursive: more consistent interface

 Documentation/merge-strategies.txt |  12 +-
 merge-recursive.c  |  14 +-
 merge-recursive.h  |   1 +
 ...s.sh => t3032-merge-recursive-space-options.sh} |   2 +-
 t/t3034-merge-recursive-rename-options.sh  | 200 +
 5 files changed, 225 insertions(+), 4 deletions(-)
 rename t/{t3032-merge-recursive-options.sh => 
t3032-merge-recursive-space-options.sh} (99%)
 create mode 100755 t/t3034-merge-recursive-rename-options.sh

-- 
2.7.1.342.gf5bb636

--
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: interactive rebase results across shared histories

[Moritz Neeb]

Hi Seb,

On 02/20/2016 11:58 PM, Seb wrote:
> Hello,
> 
> I've recently learnt how to consolidate and clean up the master branch's
> commit history.  I've squashed/fixuped many commits thinking these would
> propagate to the children branches with whom it shares the earlier parts
> of the its history.  However, this is not the case; switching to the
> child branch still shows the non-rebased (dirty) commit history from
> master.  Am I misunderstanding something with this?

I am not sure what you meand by "child branch". If I understand
corretly, you have something like:

A---B---C topic
   /
  D---E---F---G master

then you merge the topic:

A---B---C topic
   / \
  D---E---F---G---H master

and then you do something like "git rebase -i E" to linearize history
and maybe squash some commits, to result in something like:

  D---E---F---G---AB'---C' master

Where AB' is a squashed commit containing the changes from A and B.

Now, your misunderstanding may be in the fact of "what happened to the
topic branch?". Because looking at the whole graph, you have something
like this:

  A---B---C topic
 /
D---E---F---G---AB'---C' master

where it is important to note, that the topic still points to C. Which
is totally correct, because you did not say anything about topic after
the merge. If you wanted to continue working on the topic branch, then
maybe a non-interactive rebasing, like described in the rebase manpage
would be something you might want to do before rebasing. E.g., from the
start doing "git rebase master topic" leads to:

 A'--B'--C' topic
/
   D---E---F---G master

and then you could squash your commits as you like with "git rebase -i G":

  AB'--C' topic
 /
D---E---F---G master

and maybe fast-forward merging master with "git merge master", then you
have both branches pointing to C':

D---E---F---G---AB'--C' topic,master

The same could've been reached in one step via "git rebase -i master topic".

Maybe, to get a better understanding, you could use visualization tool
like "tig" or "gitk" to observe what happens to your commits (hashes)
and branches (labels) and just play around with some of these operations.
--
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 v5 23/27] svn: learn ref-storage argument

[Eric Wong]

David Turner  wrote:
> +++ b/git-svn.perl

> + if (defined $_ref_storage) {
> + push @init_db, "--ref-storage=" . $_ref_storage;
> + }

Minor nit: git-svn uses tabs for indentation.
Otherwise, if we go this route, consider it:

Signed-off-by: Eric Wong 

Thanks.

I would favor Shawn's RefTree or similar to reuse existing
code + commands and avoid the external dependency, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PULL] svn pathnameencoding for git svn dcommit

[Eric Wong]

Kazutoshi Satoda  wrote:
> (Shouldn't the branch be based on maint, as these are bugfixes?)

I'm not sure if it being previously left-out/untested feature
would qualify for maint.  Maybe it does, but I suppose I'll let
Junio decide.

> Thank you. I tried it but got similar problem:



> I found how "\357\202\201\357\202\202" (U+F081 U+F082 in UTF-8) could
> come.
> https://cygwin.com/cygwin-ug-net/using-specialnames.html#pathnames-specialchars
> > Some characters are disallowed in filenames on Windows filesystems. ...
> ...
> > ... All of the above characters, except for the backslash, are converted
> > to special UNICODE characters in the range 0xf000 to 0xf0ff (the
> > "Private use area") when creating or accessing files.
> "U+F081 U+F082" seems the result of conversion from "0x8182" (neq in
> cp932) as treating each of 2 bytes as disallowed characters.
> 
> And I also noticed that LANG and LC_ALL is set to "C" in test-lib.sh.
> 
> Setting LC_ALL=C.UTF-8 in the test 11-12 made them pass on Cygwin.
> Same change made the previous version also pass. Please find the patch
> in the attached output of git format-patch.

Thanks.  However, I also wonder what happens on machines without
"C.UTF-8" support (are there still any?).

> Could you please test with this on non-Cygwin environment?

Works for me, at least.  I've squashed your changes into the two
patches already queued up.  I needed to split the
"export LC_ALL=C.UTF-8" statement into
"LC_ALL=C.UTF-8 && export LC_ALL" for portability.

> If it made no harm, please tell me what should I do to proceed this patch.
> Will you (Eric) please make further integration? Shall I make another
> series (v2) of patches?

I've pushed out a new branch with your LC_ALL changes squashed
in.  However I'm unsure if there's any new portability problems
with LC_ALL=C.UTF-8...

Junio or anyone else: thoughts?

The following changes since commit 0233b800c838ddda41db318ee396320b3c21a560:

  Merge branch 'maint' (2016-02-17 10:14:39 -0800)

are available in the git repository at:

  git://bogomips.org/git-svn.git ks/svn-pathnameencoding-3

for you to fetch changes up to 980c083276ba06a9400c5b1b2558c3626bcff969:

  git-svn: apply "svn.pathnameencoding" before URL encoding (2016-02-20 
23:30:16 +)


Kazutoshi Satoda (2):
  git-svn: enable "svn.pathnameencoding" on dcommit
  git-svn: apply "svn.pathnameencoding" before URL encoding

 perl/Git/SVN/Editor.pm   |  4 +++-
 t/t9115-git-svn-dcommit-funky-renames.sh | 39 ++--
 2 files changed, 40 insertions(+), 3 deletions(-)
--
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

interactive rebase results across shared histories

[Seb]

Hello,

I've recently learnt how to consolidate and clean up the master branch's
commit history.  I've squashed/fixuped many commits thinking these would
propagate to the children branches with whom it shares the earlier parts
of the its history.  However, this is not the case; switching to the
child branch still shows the non-rebased (dirty) commit history from
master.  Am I misunderstanding something with this?

Thanks,

-- 
Seb

--
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] tests: rename work-tree tests to *work-tree*

[Junio C Hamano]

Michael J Gruber  writes:

> "Work tree" or "working tree" is the name of a checked out tree,
> "worktree" the name of the command which manages several working trees.
> The naming of tests mixes these two, currently:
>
> $ls t/*worktree*
> ...
> Rename t1501, t1509 and t7409 to make it clear on first glance that they
> test work tree related behavior, rather than the worktree command.
>
> t2104, t7011 and t7012 are about the "skip-worktree" flag so that their
> name should remain unchanged.
>
> Signed-off-by: Michael J Gruber 
> ---
> Just some housekeeping. Not super necessary, but should make it easier to find
> the right test to amend, for example.

That is rather unfortunate.  Most of them predate the "worktree"
subcommand, I think, and having to rename them merely because a
subcommand with a confusing name appeared sound somewhat backwards.
--
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] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Junio C Hamano]

Dmitry Vilkov  writes:

> Hi guys! Any luck with fixing this issue?

I think Brian suggested an alternative approach, to which you earler
responded

>> That would be great! Definitely it will be much better solution than
>> patch I've proposed.

The patch has been queued as 121061f6 (http: add option to try
authentication without username, 2016-02-15); perhaps you can help
by trying it out in your installation before it hits a released
version of Git?  That way, if the patch does not fix your problem,
or it introduces an unexpected side effect, we would notice before
we include it in a future release.

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 v5 25/27] refs: add LMDB refs storage backend

[Junio C Hamano]

David Turner  writes:

> So just add this after every mkdir?
>
>   if (shared_repository)
>   adjust_shared_perm(db_path);

The function itself checks shared_repository configuration, so the
caller does not have to bother.  You should rather treat it as a
declaration and a documentation that says "this path in $GIT_DIR
should be writable".

You should check its exit value for errors, though.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 04/21] harden REALLOC_ARRAY and xcalloc against size_t overflow

[René Scharfe]

Am 19.02.2016 um 12:22 schrieb Jeff King:

REALLOC_ARRAY inherently involves a multiplication which can
overflow size_t, resulting in a much smaller buffer than we
think we've allocated. We can easily harden it by using
st_mult() to check for overflow.  Likewise, we can add
ALLOC_ARRAY to do the same thing for xmalloc calls.


Good idea!


xcalloc() should already be fine, because it takes the two
factors separately, assuming the system calloc actually
checks for overflow. However, before we even hit the system
calloc(), we do our memory_limit_check, which involves a
multiplication. Let's check for overflow ourselves so that
this limit cannot be bypassed.

Signed-off-by: Jeff King 
---
  git-compat-util.h | 3 ++-
  wrapper.c | 3 +++
  2 files changed, 5 insertions(+), 1 deletion(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 0c65033..55c073d 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -779,7 +779,8 @@ extern int odb_pack_keep(char *name, size_t namesz, const 
unsigned char *sha1);
  extern char *xgetcwd(void);
  extern FILE *fopen_for_writing(const char *path);

-#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), (alloc) * sizeof(*(x)))
+#define ALLOC_ARRAY(x, alloc) (x) = xmalloc(st_mult((alloc), sizeof(*(x
+#define REALLOC_ARRAY(x, alloc) (x) = xrealloc((x), st_mult((alloc), 
sizeof(*(x


st_mult(x, y) calls unsigned_mult_overflows(x, y), which divides by x. 
This division can be done at compile time if x is a constant.  This can 
be guaranteed for all users of the two macros above by reversing the 
arguments of st_mult(), so that sizeof comes first.  Probably not a big 
win, but why not do it if it's that easy?


Or perhaps a macro like this could help here and in other places which 
use st_mult with sizeof:


  #define SIZEOF_MULT(x, n) st_mult(sizeof(x), (n))

(I'd call it ARRAY_SIZE, but that name is already taken. :)

René

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

[Zakaria Itani]

Hello,

After downloading version 2.6.4 of git for mac, i faced a problem
launching it where a message showed up saying
git-2.6.4-intel-universal-mavericks.dmg couldn't be opened since image
is not recognized. My mac's current version is Yosemite 10.10.3

I'd like to know how could I fix this problem, thank you in advance.
Zak.
--
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] tests: rename work-tree tests to *work-tree*

[Michael J Gruber]

"Work tree" or "working tree" is the name of a checked out tree,
"worktree" the name of the command which manages several working trees.
The naming of tests mixes these two, currently:

$ls t/*worktree*
t/t1501-worktree.sh
t/t1509-root-worktree.sh
t/t2025-worktree-add.sh
t/t2026-worktree-prune.sh
t/t2027-worktree-list.sh
t/t2104-update-index-skip-worktree.sh
t/t3320-notes-merge-worktrees.sh
t/t7011-skip-worktree-reading.sh
t/t7012-skip-worktree-writing.sh
t/t7409-submodule-detached-worktree.sh

$grep -l "git worktree" t/*.sh
t/t0002-gitfile.sh
t/t1400-update-ref.sh
t/t2025-worktree-add.sh
t/t2026-worktree-prune.sh
t/t2027-worktree-list.sh
t/t3320-notes-merge-worktrees.sh
t/t7410-submodule-checkout-to.sh

Rename t1501, t1509 and t7409 to make it clear on first glance that they
test work tree related behavior, rather than the worktree command.

t2104, t7011 and t7012 are about the "skip-worktree" flag so that their
name should remain unchanged.

Signed-off-by: Michael J Gruber 
---
Just some housekeeping. Not super necessary, but should make it easier to find
the right test to amend, for example.

 t/{t1501-worktree.sh => t1501-work-tree.sh}   | 0
 t/{t1509-root-worktree.sh => t1509-root-work-tree.sh} | 0
 ...bmodule-detached-worktree.sh => t7409-submodule-detached-work-tree.sh} | 0
 3 files changed, 0 insertions(+), 0 deletions(-)
 rename t/{t1501-worktree.sh => t1501-work-tree.sh} (100%)
 rename t/{t1509-root-worktree.sh => t1509-root-work-tree.sh} (100%)
 rename t/{t7409-submodule-detached-worktree.sh => 
t7409-submodule-detached-work-tree.sh} (100%)

diff --git a/t/t1501-worktree.sh b/t/t1501-work-tree.sh
similarity index 100%
rename from t/t1501-worktree.sh
rename to t/t1501-work-tree.sh
diff --git a/t/t1509-root-worktree.sh b/t/t1509-root-work-tree.sh
similarity index 100%
rename from t/t1509-root-worktree.sh
rename to t/t1509-root-work-tree.sh
diff --git a/t/t7409-submodule-detached-worktree.sh 
b/t/t7409-submodule-detached-work-tree.sh
similarity index 100%
rename from t/t7409-submodule-detached-worktree.sh
rename to t/t7409-submodule-detached-work-tree.sh
-- 
2.7.1.428.g2de392b

--
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] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[brian m. carlson]

On Sat, Feb 20, 2016 at 05:35:19PM +0300, Dmitry Vilkov wrote:
> Maybe you could accept my patch, so users would use
> "credential.helper=store" to avoid using ":@" in remote URL? At least
> for now, while there is no good solution to this issue? It would be
> very helpful because now we have to have our own version of patched
> Git :(

I sent in a patch (and I believe I CC'd you) that adds an option
http.emptyAuth that can be used in this case.  It should make its way to
a future release.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: RSA v4 4096b: 88AC E9B2 9196 305B A994 7552 F1BA 225C 0223 B187


signature.asc
Description: PGP signature

Re: GSoC 2016: Microproject

[Mehul Jain]

Hello,

I faced the following problem when I tested master branch. Here I have
made no commits to master branch.

*** t5539-fetch-http-shallow.sh ***
ok 1 - setup shallow clone
not ok 2 - clone http repository
#
# git clone --bare --no-local shallow "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
# git clone $HTTPD_URL/smart/repo.git clone &&
# (
# cd clone &&
# git fsck &&
# git log --format=%s origin/master >actual &&
# cat  done" ../trace
# )
#
# failed 2 among 3 test(s)
1..3
make[1]: *** [t5539-fetch-http-shallow.sh] Error 1
make[1]: Leaving directory `/home/mj/git/t'
make: *** [test] Error 2

Is this test failure usual with linux based system or just happened with me.
I'm running Ubuntu 14.04.

thanks,
Mehul Jain

On Fri, Feb 19, 2016 at 11:20 PM, Stefan Beller  wrote:
> On Fri, Feb 19, 2016 at 9:39 AM, Mehul Jain  wrote:
>> On Fri, Feb 19, 2016 at 6:33 PM, Matthieu Moy
>>  wrote:
>>> Hi,
>>>
>>> This is a double-post. You've posted almost the same message under the
>>> title "GSoC 2016". Nothing serious, but attention to details is
>>> important if you want to give a good image of yourself.
>>
>> I'm sorry of this kind of behavior. It was due to a misunderstanding by my 
>> side.
>>
>>> I don't have all the code in mind, but I think it is. In this situation,
>>> you always end up with a variable telling Git what to do (I guess,
>>> autostash here), and this variable is set according to the configuration
>>> and the command-line.
>>>
>>> You should be careful about the precedence: command-line should override
>>> the configuration. And the default behavior should be used if neither
>>> the command-line nor the configuration specified otherwise.
>>
>> Thanks for the suggestion.
>> I've started the work on patch and did the change in the code which
>> were necessary for Microproject. I have run the test by using "make",
>> and there was no failure of any test.
>> I've a doubt regarding tests. Here as "git pull" will now understand
>> the argument that  "--[no-]autostash" means "rebase.autostash" should
>> be set false for current execution of command "git pull --rebase". So
>> do I have to write a test for this new option?
>>
>
> Yes, most likely t/t5521-pull-options.sh or t/t5520-pull.sh would be the right
> place as judging from the file name of the tests.
>
> Thanks,
> Stefan
>
>> Mehul Jain
>> --
>> 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
--
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] remote-curl: don't fall back to Basic auth if we haven't tried Negotiate

[Dmitry Vilkov]

Hi guys! Any luck with fixing this issue?

I would like to draw your attention to the fact that Git starting from
version 2.3.1 is unusable with servers that support GSS-Negotiation
(e.g. Microsoft TFS).

Sorry, english is not my native language and probably I was not clear
enough when described the problem at first time.

So, let me try again. Algorithm of fetching data through HTTP(S)
protocol is as follows:
1. Try to fetch data without any credentials.
2. If first step failed
  2.1. Disable GSS-Negotiation algorithm (this is the side effect
which is fixed by my patch).
  2.2. Read credentials from config files.
  2.3. If there is no credentials then ask user for it.
  2.4. Go to step one, but now try to fetch data with found/provided
credentials.

As you can see there is no other way to connect to server with
GSS-Negotiation auth than use URLs like
"https://:@my-gss-git-server.com; (because there is step zero where we
are parsing URL which can contain credentials that we can use in step
one).

Maybe you could accept my patch, so users would use
"credential.helper=store" to avoid using ":@" in remote URL? At least
for now, while there is no good solution to this issue? It would be
very helpful because now we have to have our own version of patched
Git :(

Thanks in advance.


2016-02-08 12:11 GMT+03:00 Dmitry Vilkov :
> 2016-02-06 0:52 GMT+03:00 Junio C Hamano :
>> "brian m. carlson"  writes:
>>
>>> On Fri, Feb 05, 2016 at 01:02:58PM -0800, Junio C Hamano wrote:
 Hmph, so documenting that :@
 as a supported way might be an ugly-looking solution to the original
 problem.  A less ugly-looking solution might be a boolean that can
 be set per URL (we already have urlmatch-config infrastructure to
 help us do so) to tell us to pass the empty credential to lubCurl,
 bypassing the step to ask the user for password that we do not use.

 The end-result of either of these solution would strictly be better
 than the patch we discussed in that the end user will not have to
 interact with the prompt at all, right?
>>>
>>> Yes, that's true.  I'll try to come up with a patch this weekend that
>>> implements that (maybe remote.forceAuth = true or somesuch).
>>
>> Thanks.
>>
>> I think the configuration should live inside http.* namespace, as
>> there are already things like http[.].sslCert and friends.
>>
>> I do not have a good suggestion on the name of the leaf-level
>> variable.  ForceAuth sounds as if you are forcing authentication
>> even when the other side does not require it, though.
>
> That would be great! Definitely it will be much better solution than
> patch I've proposed.
--
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.c: simplify striping extension of a file in handle_builtin()

[Alexander Kuleshov]

The handle_builtin() starts from striping of command extension if
STRIP_EXTENSION is enabled. Actually STRIP_EXTENSION does not used
anywhere else.

This patch introduces strip_extension() helper to strip STRIP_EXTENSION
extension from argv[0] with the strip_suffix() instead of manually
striping.

Signed-off-by: Alexander Kuleshov 
Helped-by: Jeff King 
---
 git-compat-util.h |  4 
 git.c | 26 +++---
 2 files changed, 15 insertions(+), 15 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 8f0e76b..b35251c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -333,10 +333,6 @@ extern char *gitdirname(char *);
 #define _PATH_DEFPATH "/usr/local/bin:/usr/bin:/bin"
 #endif
 
-#ifndef STRIP_EXTENSION
-#define STRIP_EXTENSION ""
-#endif
-
 #ifndef has_dos_drive_prefix
 static inline int git_has_dos_drive_prefix(const char *path)
 {
diff --git a/git.c b/git.c
index 087ad31..6cc0c07 100644
--- a/git.c
+++ b/git.c
@@ -509,21 +509,25 @@ int is_builtin(const char *s)
return !!get_builtin(s);
 }
 
+#ifdef STRIP_EXTENSION
+static void strip_extension(const char **argv)
+{
+   size_t len;
+
+   if (strip_suffix(argv[0], STRIP_EXTENSION, ))
+   argv[0] = xmemdupz(argv[0], len);
+}
+#else
+#define strip_extension(cmd)
+#endif
+
 static void handle_builtin(int argc, const char **argv)
 {
-   const char *cmd = argv[0];
-   int i;
-   static const char ext[] = STRIP_EXTENSION;
+   const char *cmd;
struct cmd_struct *builtin;
 
-   if (sizeof(ext) > 1) {
-   i = strlen(argv[0]) - strlen(ext);
-   if (i > 0 && !strcmp(argv[0] + i, ext)) {
-   char *argv0 = xstrdup(argv[0]);
-   argv[0] = cmd = argv0;
-   argv0[i] = '\0';
-   }
-   }
+   strip_extension(argv);
+   cmd = argv[0];
 
/* Turn "git cmd --help" into "git help cmd" */
if (argc > 1 && !strcmp(argv[1], "--help")) {
-- 
2.5.0

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

Re: [PATCH v5 25/27] refs: add LMDB refs storage backend

[Duy Nguyen]

On Fri, Feb 19, 2016 at 3:23 AM, David Turner  wrote:
> On Thu, 2016-02-18 at 15:50 +0700, Duy Nguyen wrote:
>
> [snip]
>
> Thanks; applied the above
>
>> This permission makes me wonder if we need adjust_shared_perm() here
>> and some other places.
>
> So just add this after every mkdir?
>
> if (shared_repository)
> adjust_shared_perm(db_path);
>

Another option is avoid mkdir entirely. Getting started page [1] says
if you do mdb_open_env(.., "refs.lmdb", MDB_NOSUBDIR,..) then it will
create two files refs.lmdb and refs.lmdb.lock. No need for the
directory "refs.lmdb" just to contain two files.

[1] http://symas.com/mdb/doc/starting.html
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-compat-util.h: move extension stripping from handle_builtin()

[Alexander Kuleshov]

Hello Jeff,

On 02-20-16, Jeff King wrote:
> On Sat, Feb 20, 2016 at 02:10:58PM +0600, Alexander Kuleshov wrote:
> 
> I'm not convinced that moving the functions inline into git-compat-util
> is actually cleaner. We've expanded the interface that is visible to the
> whole code base, warts at all.
> 
> One wart I see is that the caller cannot know whether the return value
> was newly allocated or not, and therefore cannot free it, creating a
> potential memory leak. Another is that the return value is not really
> necessary at all; we always munge argv[0].
> 
> Does any other part of the code actually care about this
> extension-stripping?

Nope, only this one.

> 
> Perhaps instead, could we do this:
> If we drop this default-to-empty value of STRIP_EXTENSION entirely, then
> we can do our #ifdef local to git.c, where it does not bother anybody
> else. Like:
> 
>   #ifdef STRIP_EXTENSION
>   static void strip_extension(const char **argv)
>   {
>   /* Do the thing */
>   }
>   #else
>   #define strip_extension(x)
>   #endif
> 
> (Note that I also simplified the return value).
> 
> In the case that we do have STRIP_EXTENSION, I don't think we need to
> handle the empty-string case. It would be a regression for somebody
> passing -DSTRIP_EXTENSION="", but I don't think that's worth worrying
> about. That macro is defined totally internally.
> 
> I suspect you could also use strip_suffix here. So something like:
> 
>   size_t len;
> 
>   if (strip_suffix(str, STRIP_EXTENSION, ))
>   argv[0] = xmemdupz(argv[0], len);
> 
> would probably work, but that's totally untested.

Good suggestion. I will try to do it and test.

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

Re: [PATCH] git-p4.py: Make submit working on bare repository

[Amadeusz Żołnowski]

Hi Junio,

To submit changes from master branch to Perforce, new commits should be
based on a remote branch p4/master:

(1)
  E'---F' master
 /
A---B---C---D p4/master

Commits from A to D map to Perforce changelists. These commits include
additional metadata in commit message which most important is changelist
number.

On submit git-p4 prepares changelists for commits E'-F' and submits
these to Perforce repository. After this operation it syncs back remote
branch p4/master. This is the common part for both bare and non-bare
repository.

(2)
  E'---F' master
 /
A---B---C---D---E---F p4/master

In non-bare repository git rebase is performed and it results in
following state:

(3)
A---B---C---D---E---F p4/master, master

In bare repository this operation cannot be performed, so it remains in
state (2).

With special care state (2) can be transformed to state (3) with manual
update of refs/heads/master with refs/remotes/p4/master.

I understand that implementing rebase for bare repository is unsafe and
it wouldn't be appreciated. Therefore we have to resort to such a
barbarity and git-p4 submit shouldn't attempt to perform a rebase
operation itself. Especially because it performs other operations before
and we end up in the intermediate state (2) anyway.

Is this explanation satisfactory? If yes, I'll include it in patch
description.


Kind regards,

-- 
Amadeusz Żołnowski


signature.asc
Description: PGP signature

Re: [PATCH] submodule: Fetch the direct sha1 first

[Jacob Keller]

On Fri, Feb 19, 2016 at 1:13 PM, Junio C Hamano  wrote:
>> Regarding performance, the first fetch should fail quite fast iff the fetch
>> fails and then continue with the normal fetch. In case the first fetch works
>> fine getting the exact sha1, the fetch should be faster than a default fetch
>> as potentially less data needs to be fetched.
>
> "The fetch should be faster" may not be making a good trade-off
> overall--people may have depended on the branches configured to be
> fetched to be fetched after this codepath is exercised, but now if
> the commit bound to the superproject tree happens to be complete,
> even though it is not anchored by any remote tracking ref (hence the
> next GC may clobber it), the fetch of other branches will not
> happen.
>
> My knee-jerk reaction is that the order of fallback is probably the
> other way around.  That is, try "git fetch" as before, check again
> if the commit bound to the superproject tree is now complete, and
> fallback to fetch that commit with an extra "git fetch".
>

FWIW, I think the order you suggest here is probably better. It would
be lower risk of breaking something since we'd only do something more
in this case if the current fetch fails.

I've definitely been bit by this before thinking that the sub module
would be able to be fetched just fine only to discover that it wasn't
able to locate the change.

Regards,
Jake
--
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: GSoC 2016: applications open, deadline = Fri, 19/2

[Johannes Schindelin]

Hi Junio,

On Fri, 19 Feb 2016, Junio C Hamano wrote:

> The "experimenting" would include mergy operations like "am -3" and
> "cherry-pick".  "After queuing a topic and trying it in isolation, an
> attempt to merge to the baseline results in quite a mess, and I give
> up"--there is nothing to salvage.
> 
> And obviously, "stash" is not useful in such a situation.

I think this is more a short-coming of "stash" than anything else.

Many a times did I wish I could simply quickly stash a failed merge and
then come back later. Or not. Just like stashed changes without conflicts
allow me to do already.

Ciao,
Dscho
--
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 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Sat, Feb 20, 2016 at 3:57 AM, Jeff King  wrote:
> On Sat, Feb 20, 2016 at 03:39:36AM -0500, Eric Sunshine wrote:
>> I also had made the strbuf_detach() analogy in my response but deleted
>> it before sending; I do think it's a reasonable API template to mirror
>> via new argv_array_detach().
>
> That would look like this, which I think is not too bad (on top of my
> series for now; I'd do the API function as a separate patch at the
> beginning and then use it immediately).

Looks reasonable.

> diff --git a/argv-array.c b/argv-array.c
> index eaed477..5d370fa 100644
> --- a/argv-array.c
> +++ b/argv-array.c
> @@ -74,3 +74,14 @@ void argv_array_clear(struct argv_array *array)
> }
> argv_array_init(array);
>  }
> +
> +const char **argv_array_detach(struct argv_array *array)
> +{
> +   if (array->argv == empty_argv)
> +   return xcalloc(1, sizeof(const char *));
> +   else {
> +   const char **ret = array->argv;
> +   argv_array_init(array);
> +   return ret;
> +   }
> +}
> diff --git a/argv-array.h b/argv-array.h
> index a2fa0aa..29056e4 100644
> --- a/argv-array.h
> +++ b/argv-array.h
> @@ -20,5 +20,6 @@ void argv_array_pushl(struct argv_array *, ...);
>  void argv_array_pushv(struct argv_array *, const char **);
>  void argv_array_pop(struct argv_array *);
>  void argv_array_clear(struct argv_array *);
> +const char **argv_array_detach(struct argv_array *);
>
>  #endif /* ARGV_ARRAY_H */
> diff --git a/line-log.c b/line-log.c
> index fa095b9..bbe31ed 100644
> --- a/line-log.c
> +++ b/line-log.c
> @@ -748,15 +748,17 @@ void line_log_init(struct rev_info *rev, const char 
> *prefix, struct string_list
>
> if (!rev->diffopt.detect_rename) {
> struct line_log_data *r;
> -   struct argv_array paths = ARGV_ARRAY_INIT;
> +   struct argv_array array = ARGV_ARRAY_INIT;
> +   const char **paths;
>
> for (r = range; r; r = r->next)
> -   argv_array_push(, r->path);
> +   argv_array_push(, r->path);
> +   paths = argv_array_detach();
> +
> parse_pathspec(>diffopt.pathspec, 0,
> -  PATHSPEC_PREFER_FULL, "", paths.argv);
> -   /* argv strings are now owned by pathspec */
> -   paths.argc = 0;
> -   argv_array_clear();
> +  PATHSPEC_PREFER_FULL, "", paths);
> +   /* strings are now owned by pathspec */
> +   free(paths);
> }
>  }
--
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 v5 25/27] refs: add LMDB refs storage backend

[Duy Nguyen]

On Thu, Feb 18, 2016 at 12:17 PM, David Turner  wrote:
> LMDB has a few features that make it suitable for usage in git:
> ...

I'm reading lmdb documents and hitting  the caveat section [1]. Random thoughts

* "There is normally no pure read-only mode, since readers need write
access to locks and lock file.".

This will be a problem for server side that serves git:// protocol
only. Some of those servers disable write access to the entire
repository and git still works fine (but won't when lmdb is used).
Should we do something in this case? Just tell server admins to relax
file access, or use MDB_NOLOCK (and when? based on config var?)

*  " Use an MDB_env* in the process which opened it, without
fork()ing.". We do use fork on non-Windows in run-command.c, but it
should be followed by exec() with no ref access in between, so we're
almost good.

I notice atexit() is used in this for/exec code, which reminds me we
also use atexit() in many other places. I hope none of them access
refs, or we could be in trouble.

* "Do not have open an LMDB database twice in the same process at the
same time. Not even from a plain open() call - close()ing it breaks
flock() advisory locking"

I wonder what happens if we do open twice, will it error out or
silently ignore and move on? Because if it's the latter case, we need
some protection from the caller and I'm not sure if
lmdb-backend.c:init_env() has it, especially when it open submodule's
lmdb.

* "Avoid long-lived transactions"

OK we don't have a problem with this. But it makes me realize lmdb
transactions do not map with ref transactions. We don't open lmdb
transaction at ref_transaction_begin(), for example. Is it simply more
convenient to do transactions the current way, or is it impossible or
incorrect to attach lmdb transactions to ref_transaction_*()?

* "Avoid aborting a process with an active transaction. The
transaction becomes "long-lived" as above until a check for stale
readers is performed or the lockfile is reset, since the process may
not remove it from the lockfile."

Does it mean we should at atexit() and signal handler to release
currently active transaction?

* "Do not use LMDB databases on remote filesystems, even between
processes on the same host. This breaks flock() on some OSes, possibly
memory map sync, and certainly sync between programs on different
hosts."

OK can't do anything about it anyway, but maybe it should be mentioned
somewhere in Git documentation.

* "Opening a database can fail if another process is opening or
closing it at exactly the same time."

We have some retry logic in resolve_ref_1(). Do we need the same for
lmdb? Not very important though.

[1] http://symas.com/mdb/doc/
-- 
Duy
--
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 06/21] convert manual allocations to argv_array

[Jeff King]

On Sat, Feb 20, 2016 at 03:39:36AM -0500, Eric Sunshine wrote:

> I also had made the strbuf_detach() analogy in my response but deleted
> it before sending; I do think it's a reasonable API template to mirror
> via new argv_array_detach().

That would look like this, which I think is not too bad (on top of my
series for now; I'd do the API function as a separate patch at the
beginning and then use it immediately).

diff --git a/argv-array.c b/argv-array.c
index eaed477..5d370fa 100644
--- a/argv-array.c
+++ b/argv-array.c
@@ -74,3 +74,14 @@ void argv_array_clear(struct argv_array *array)
}
argv_array_init(array);
 }
+
+const char **argv_array_detach(struct argv_array *array)
+{
+   if (array->argv == empty_argv)
+   return xcalloc(1, sizeof(const char *));
+   else {
+   const char **ret = array->argv;
+   argv_array_init(array);
+   return ret;
+   }
+}
diff --git a/argv-array.h b/argv-array.h
index a2fa0aa..29056e4 100644
--- a/argv-array.h
+++ b/argv-array.h
@@ -20,5 +20,6 @@ void argv_array_pushl(struct argv_array *, ...);
 void argv_array_pushv(struct argv_array *, const char **);
 void argv_array_pop(struct argv_array *);
 void argv_array_clear(struct argv_array *);
+const char **argv_array_detach(struct argv_array *);
 
 #endif /* ARGV_ARRAY_H */
diff --git a/line-log.c b/line-log.c
index fa095b9..bbe31ed 100644
--- a/line-log.c
+++ b/line-log.c
@@ -748,15 +748,17 @@ void line_log_init(struct rev_info *rev, const char 
*prefix, struct string_list
 
if (!rev->diffopt.detect_rename) {
struct line_log_data *r;
-   struct argv_array paths = ARGV_ARRAY_INIT;
+   struct argv_array array = ARGV_ARRAY_INIT;
+   const char **paths;
 
for (r = range; r; r = r->next)
-   argv_array_push(, r->path);
+   argv_array_push(, r->path);
+   paths = argv_array_detach();
+
parse_pathspec(>diffopt.pathspec, 0,
-  PATHSPEC_PREFER_FULL, "", paths.argv);
-   /* argv strings are now owned by pathspec */
-   paths.argc = 0;
-   argv_array_clear();
+  PATHSPEC_PREFER_FULL, "", paths);
+   /* strings are now owned by pathspec */
+   free(paths);
}
 }
 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git-compat-util.h: move extension stripping from handle_builtin()

[Jeff King]

On Sat, Feb 20, 2016 at 02:10:58PM +0600, Alexander Kuleshov wrote:

> The handle_builtin() starts from striping of command extension if
> STRIP_EXTENSION is enabled. As this is an OS dependent, let's move
> to the git-compat-util.h as all similar functions to do handle_builtin()
> more cleaner.

I'm not convinced that moving the functions inline into git-compat-util
is actually cleaner. We've expanded the interface that is visible to the
whole code base, warts at all.

One wart I see is that the caller cannot know whether the return value
was newly allocated or not, and therefore cannot free it, creating a
potential memory leak. Another is that the return value is not really
necessary at all; we always munge argv[0].

Does any other part of the code actually care about this
extension-stripping?

Perhaps instead, could we do this:

>  git-compat-util.h | 18 ++
>  git.c | 13 +
>  2 files changed, 19 insertions(+), 12 deletions(-)
> 
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 658d03b..57f2fda 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -323,6 +323,24 @@ extern char *gitbasename(char *);
>  
>  #ifndef STRIP_EXTENSION
>  #define STRIP_EXTENSION ""
> +static inline const char *strip_extension(const char **argv)
> +{
> + return argv[0];
> +}
> +#else
> +static inline const char *strip_extension(const char **argv)
> +{
> + static const char ext[] = STRIP_EXTENSION;
> + int ext_len = strlen(argv[0]) - strlen(ext);
> +
> + if (ext_len > 0 && !strcmp(argv[0] + ext_len, ext)) {
> + char *argv0 = xstrdup(argv[0]);
> + argv[0] = argv0;
> + argv0[ext_len] = '\0';
> + }
> +
> + return argv[0];
> +}
>  #endif

If we drop this default-to-empty value of STRIP_EXTENSION entirely, then
we can do our #ifdef local to git.c, where it does not bother anybody
else. Like:

  #ifdef STRIP_EXTENSION
  static void strip_extension(const char **argv)
  {
/* Do the thing */
  }
  #else
  #define strip_extension(x)
  #endif

(Note that I also simplified the return value).

In the case that we do have STRIP_EXTENSION, I don't think we need to
handle the empty-string case. It would be a regression for somebody
passing -DSTRIP_EXTENSION="", but I don't think that's worth worrying
about. That macro is defined totally internally.

I suspect you could also use strip_suffix here. So something like:

  size_t len;

  if (strip_suffix(str, STRIP_EXTENSION, ))
argv[0] = xmemdupz(argv[0], len);

would probably work, but that's totally untested.

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

Re: [PATCH 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Sat, Feb 20, 2016 at 3:34 AM, Jeff King  wrote:
> On Sat, Feb 20, 2016 at 03:29:29AM -0500, Eric Sunshine wrote:
>> On Sat, Feb 20, 2016 at 3:10 AM, Jeff King  wrote:
>> > On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
>> >> > +   /* argv strings are now owned by pathspec */
>> >> > +   paths.argc = 0;
>> >> > +   argv_array_clear();
>> >>
>> >> This overly intimate knowledge of the internal implementation of
>> >> argv_array_clear() is rather ugly.
>> >
>> > Yep, I agree. Suggestions?
>> > [...]
>> >
>> > I guess we can make an argv_array_detach_strings() function. Or maybe
>> > even just argv_array_detach() would be less gross, and then this
>> > function could manually free the array but not the strings themselves.
>>
>> [...]
>> I wonder if a simple "dup'ing" string_list would be more suitable for
>> this case. You'd have to append the NULL item manually with
>> string_list_append_nodup(), and string_list_clear() would then be the
>> correct way to dispose of the list without intimate knowledge of its
>> implementation and no need for an API extension.
>
> A string_list doesn't just store pointers; it's a struct with a util
> field. So you can't pass it to things expecting a "const char **".

Yep, I knew that but wasn't thinking straight.

> I think argv_array_detach() is the least-bad thing here. It matches
> strbuf_detach() to say "you now own the storage" (as opposed to just
> peeking at argv.argv, which we should do only in a read-only way).

I also had made the strbuf_detach() analogy in my response but deleted
it before sending; I do think it's a reasonable API template to mirror
via new argv_array_detach().
--
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 06/21] convert manual allocations to argv_array

[Jeff King]

On Sat, Feb 20, 2016 at 03:29:29AM -0500, Eric Sunshine wrote:

> On Sat, Feb 20, 2016 at 3:10 AM, Jeff King  wrote:
> > On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
> >> > +   /* argv strings are now owned by pathspec */
> >> > +   paths.argc = 0;
> >> > +   argv_array_clear();
> >>
> >> This overly intimate knowledge of the internal implementation of
> >> argv_array_clear() is rather ugly.
> >
> > Yep, I agree. Suggestions?
> >
> > We can just leak the array of "char *". This function is called only
> > once per program invocation, and that's unlikely to change.
> >
> > I guess we can make an argv_array_detach_strings() function. Or maybe
> > even just argv_array_detach() would be less gross, and then this
> > function could manually free the array but not the strings themselves.
> 
> The latter is what I was thinking, and I agree that
> argv_array_detach() would be less gross than
> argv_array_detach_strings(), however, it also feels a bit wrong since
> this sort of ownership transfer is kind of out of scope for
> argv_array.
> 
> I wonder if a simple "dup'ing" string_list would be more suitable for
> this case. You'd have to append the NULL item manually with
> string_list_append_nodup(), and string_list_clear() would then be the
> correct way to dispose of the list without intimate knowledge of its
> implementation and no need for an API extension.

A string_list doesn't just store pointers; it's a struct with a util
field. So you can't pass it to things expecting a "const char **".

I think argv_array_detach() is the least-bad thing here. It matches
strbuf_detach() to say "you now own the storage" (as opposed to just
peeking at argv.argv, which we should do only in a read-only way).

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

Re: [PATCH 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Sat, Feb 20, 2016 at 3:10 AM, Jeff King  wrote:
> On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:
>> > +   /* argv strings are now owned by pathspec */
>> > +   paths.argc = 0;
>> > +   argv_array_clear();
>>
>> This overly intimate knowledge of the internal implementation of
>> argv_array_clear() is rather ugly.
>
> Yep, I agree. Suggestions?
>
> We can just leak the array of "char *". This function is called only
> once per program invocation, and that's unlikely to change.
>
> I guess we can make an argv_array_detach_strings() function. Or maybe
> even just argv_array_detach() would be less gross, and then this
> function could manually free the array but not the strings themselves.

The latter is what I was thinking, and I agree that
argv_array_detach() would be less gross than
argv_array_detach_strings(), however, it also feels a bit wrong since
this sort of ownership transfer is kind of out of scope for
argv_array.

I wonder if a simple "dup'ing" string_list would be more suitable for
this case. You'd have to append the NULL item manually with
string_list_append_nodup(), and string_list_clear() would then be the
correct way to dispose of the list without intimate knowledge of its
implementation and no need for an API extension.
--
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-compat-util.h: move extension stripping from handle_builtin()

[Alexander Kuleshov]

The handle_builtin() starts from striping of command extension if
STRIP_EXTENSION is enabled. As this is an OS dependent, let's move
to the git-compat-util.h as all similar functions to do handle_builtin()
more cleaner.
---
 git-compat-util.h | 18 ++
 git.c | 13 +
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index 658d03b..57f2fda 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -323,6 +323,24 @@ extern char *gitbasename(char *);
 
 #ifndef STRIP_EXTENSION
 #define STRIP_EXTENSION ""
+static inline const char *strip_extension(const char **argv)
+{
+   return argv[0];
+}
+#else
+static inline const char *strip_extension(const char **argv)
+{
+   static const char ext[] = STRIP_EXTENSION;
+   int ext_len = strlen(argv[0]) - strlen(ext);
+
+   if (ext_len > 0 && !strcmp(argv[0] + ext_len, ext)) {
+   char *argv0 = xstrdup(argv[0]);
+   argv[0] = argv0;
+   argv0[ext_len] = '\0';
+   }
+
+   return argv[0];
+}
 #endif
 
 #ifndef has_dos_drive_prefix
diff --git a/git.c b/git.c
index 8751ef0..a4d2a46 100644
--- a/git.c
+++ b/git.c
@@ -506,19 +506,8 @@ int is_builtin(const char *s)
 
 static void handle_builtin(int argc, const char **argv)
 {
-   const char *cmd = argv[0];
-   int i;
-   static const char ext[] = STRIP_EXTENSION;
struct cmd_struct *builtin;
-
-   if (sizeof(ext) > 1) {
-   i = strlen(argv[0]) - strlen(ext);
-   if (i > 0 && !strcmp(argv[0] + i, ext)) {
-   char *argv0 = xstrdup(argv[0]);
-   argv[0] = cmd = argv0;
-   argv0[i] = '\0';
-   }
-   }
+   const char *cmd = strip_extension(argv);
 
/* Turn "git cmd --help" into "git help cmd" */
if (argc > 1 && !strcmp(argv[1], "--help")) {
-- 
2.4.4.764.gf6c74eb.dirty

--
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 06/21] convert manual allocations to argv_array

[Jeff King]

On Sat, Feb 20, 2016 at 03:07:00AM -0500, Eric Sunshine wrote:

> > diff --git a/line-log.c b/line-log.c
> > @@ -746,23 +747,16 @@ void line_log_init(struct rev_info *rev, const char 
> > *prefix, struct string_list
> > if (!rev->diffopt.detect_rename) {
> > -   int i, count = 0;
> > -   struct line_log_data *r = range;
> > -   const char **paths;
> > -   while (r) {
> > -   count++;
> > -   r = r->next;
> > -   }
> > -   paths = xmalloc((count+1)*sizeof(char *));
> > -   r = range;
> > -   for (i = 0; i < count; i++) {
> > -   paths[i] = xstrdup(r->path);
> > -   r = r->next;
> > -   }
> > -   paths[count] = NULL;
> > +   struct line_log_data *r;
> > +   struct argv_array paths = ARGV_ARRAY_INIT;
> > +
> > +   for (r = range; r; r = r->next)
> > +   argv_array_push(, r->path);
> > parse_pathspec(>diffopt.pathspec, 0,
> > -  PATHSPEC_PREFER_FULL, "", paths);
> > -   free(paths);
> > +  PATHSPEC_PREFER_FULL, "", paths.argv);
> > +   /* argv strings are now owned by pathspec */
> > +   paths.argc = 0;
> > +   argv_array_clear();
> 
> This overly intimate knowledge of the internal implementation of
> argv_array_clear() is rather ugly.

Yep, I agree. Suggestions?

We can just leak the array of "char *". This function is called only
once per program invocation, and that's unlikely to change.

I guess we can make an argv_array_detach_strings() function. Or maybe
even just argv_array_detach() would be less gross, and then this
function could manually free the array but not the strings themselves.

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

Re: [PATCH 06/21] convert manual allocations to argv_array

[Eric Sunshine]

On Fri, Feb 19, 2016 at 6:23 AM, Jeff King  wrote:
> There are many manual argv allocations that predate the
> argv_array API. Switching to that API brings a few
> advantages:
>
>   1. We no longer have to manually compute the correct final
>  array size (so it's one less thing we can screw up).
>
>   2. In many cases we had to make a separate pass to count,
>  then allocate, then fill in the array. Now we can do it
>  in one pass, making the code shorter and easier to
>  follow.
>
>   3. argv_array handles memory ownership for us, making it
>  more obvious when things should be free()d and and when
>  not.
>
> Most of these cases are pretty straightforward. In some, we
> switch from "run_command_v" to "run_command" which lets us
> directly use the argv_array embedded in "struct
> child_process".
>
> Signed-off-by: Jeff King 
> ---
> diff --git a/line-log.c b/line-log.c
> @@ -746,23 +747,16 @@ void line_log_init(struct rev_info *rev, const char 
> *prefix, struct string_list
> if (!rev->diffopt.detect_rename) {
> -   int i, count = 0;
> -   struct line_log_data *r = range;
> -   const char **paths;
> -   while (r) {
> -   count++;
> -   r = r->next;
> -   }
> -   paths = xmalloc((count+1)*sizeof(char *));
> -   r = range;
> -   for (i = 0; i < count; i++) {
> -   paths[i] = xstrdup(r->path);
> -   r = r->next;
> -   }
> -   paths[count] = NULL;
> +   struct line_log_data *r;
> +   struct argv_array paths = ARGV_ARRAY_INIT;
> +
> +   for (r = range; r; r = r->next)
> +   argv_array_push(, r->path);
> parse_pathspec(>diffopt.pathspec, 0,
> -  PATHSPEC_PREFER_FULL, "", paths);
> -   free(paths);
> +  PATHSPEC_PREFER_FULL, "", paths.argv);
> +   /* argv strings are now owned by pathspec */
> +   paths.argc = 0;
> +   argv_array_clear();

This overly intimate knowledge of the internal implementation of
argv_array_clear() is rather ugly.

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