Re: [PATCH v5 02/14] trailer: process trailers from file and arguments

2014-02-10 Thread Junio C Hamano
Christian Couder chrisc...@tuxfamily.org writes:

 Many entries with the same key but distinct values can be configured
 using:

 if_exists = add_if_different
 if_missing = add

 Many entries with the same key but values that can be the same can be
 configured using:

 if_exists = add
 if_missing = add

While the above certainly can express the combinations, I am still
puzzled about the value of having under this condition (i.e.
if-exists/if-missing) and do this (i.e. add/add-if-different) as
two separate concepts.

If you do not have an existing entry with the same key, no new entry
can be the same as an existing one---therefore, the former allow
only distinct values for the same key can just say

  trailer.Fixes.action = add_if_different

or something, without any if_exists/if_missing.  In a similar way,
the latter duplicated values allowed for the same key can say

  trailer.Sob.action = add

You can throw into the mix other actions like add_if_missing, you
can also express only one value allowed for the same key---the
first one wins, replace to mean replace if there is one with the
same key, and replace_or_add to mean same as 'replace', but add
if there is no existing entries with the same key.  Will we lose
expressiveness by simplifying the semantics, getting rid of this
under this condition part and instead making do this part more
intelligent?

Even if we assume, for the sake of discussion, that it *is* a good
idea to separate under this condition part and do this part, I
do not think the above is the only or the best way to express
distinct values allowed for the same key.  How do we argue that
this from your example

  if_exists = add_if_different
  if_missing = add

is a better design than this, for example?

  if_key_value_exists = ignore ; exact matching key,val exists
  if_key_exists = add  ; otherwise
  if_missing = add

The latter splits remaining conditional logic from do this part
(no more add_if_different that causes add action to see if there
is the same key and act differently) and moves it to under this
condition part.

I am trying to illustrate that the line to split the under this
condition and do this looks arbitrary and fuzzy here, and because
of this arbitrary-ness and fuzziness, it is not very obvious to me
why it is a good idea to have under this condition as a separate
concept from do this settings.

What is the advantage of having such an extra axis?  Does it make it
easier to manage?  Does it allow us to express more?

I can see that the location where a new entry (or a duplicated one)
is added (i.e. do we prepend? do we append?) can be orthogonal to
any of the above; no need to bring up 'where' in the discussion.
--
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/4] docs/merge-strategies: remove hyphen from mis-merges

2014-02-10 Thread Junio C Hamano
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: [GIT PULL] l10n updates for 1.9.0 round 2

2014-02-10 Thread Junio C Hamano
Thansk.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH 3/4] docs/git-clone: clarify use of --no-hardlinks option

2014-02-10 Thread Junio C Hamano
Albert L. Lash, IV albert.l...@gmail.com writes:

 Current text claims optimization, implying the use of
 hardlinks, when this option ratchets down the level of
 efficiency. This change explains the difference made by
 using this option, namely copying instead of hardlinking,
 and why it may be useful.

 Signed-off-by: Albert L. Lash, IV ala...@bloomberg.net
 ---
  Documentation/git-clone.txt | 11 ---
  1 file changed, 4 insertions(+), 7 deletions(-)

 diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
 index bf3dac0..0363d00 100644
 --- a/Documentation/git-clone.txt
 +++ b/Documentation/git-clone.txt
 @@ -55,15 +55,12 @@ repository is specified as a URL, then this flag is 
 ignored (and we
  never use the local optimizations).  Specifying `--no-local` will
  override the default when `/path/to/repo` is given, using the regular
  Git transport instead.
 -+
 -To force copying instead of hardlinking (which may be desirable if you
 -are trying to make a back-up of your repository), but still avoid the
 -usual Git aware transport mechanism, `--no-hardlinks` can be used.
  
  --no-hardlinks::
 - Optimize the cloning process from a repository on a
 - local filesystem by copying files under `.git/objects`
 - directory.
 + Force the cloning process from a repository on a local
 + filesystem to copy the files under the `.git/objects`
 + directory instead of using hardlinks. This may be desirable
 + if you are trying to make a back-up of your repository.

Makes sense, and it is kind of embarrassing (I have to suspect that
this was originally the description of --hardlinks option).

[PATCH {1,2}/4] looked trivially correct, too.

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


Re: [PATCH 4/4] docs/git-blame: explain more clearly the example pickaxe use

2014-02-10 Thread Junio C Hamano
Albert L. Lash, IV albert.l...@gmail.com writes:

 We state that the following paragraph mentions the pickaxe
 interface, but the term pickaxe is not then used. This
 change clarifies that the example command uses the pickaxe
 interface and what it is searching for.

 Signed-off-by: Albert L. Lash, IV ala...@bloomberg.net
 ---
  Documentation/git-blame.txt | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/Documentation/git-blame.txt b/Documentation/git-blame.txt
 index 8e70a61..ddb88d0 100644
 --- a/Documentation/git-blame.txt
 +++ b/Documentation/git-blame.txt
 @@ -35,7 +35,8 @@ Apart from supporting file annotation, Git also supports 
 searching the
  development history for when a code snippet occurred in a change. This makes 
 it
  possible to track when a code snippet was added to a file, moved or copied
  between files, and eventually deleted or replaced. It works by searching for
 -a text string in the diff. A small example:
 +a text string in the diff. A small example of the pickaxe interface 
 +that searches for `blame_usage`:
  
  -
  $ git log --pretty=oneline -S'blame_usage'

Thanks.

I cannot shake this nagging feeling that this and the latter half of
the previous paragraph may not belong to this page, though.

Will queue.
--
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 00/11] More preparatory work for multiparent tree-walker

2014-02-10 Thread Junio C Hamano
Kirill Smelkov k...@mns.spb.ru writes:

 Here I'm preparing tree-diff.c to be ready for the new tree-walker, so that 
 the
 final change is tractable and looks good and non noisy. Some small speedups
 are gained along the way. The final bits are almost ready, but I don't want to
 release them in a hurry.

No worries.  We are not in a hurry to apply non-regression-fix
changes during a pre-release feature freeze period anyway.

This seems to somehow conflict with other topics and does not
cleanly apply on top of your other tree-diff topic, by the way.


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


Re: [PATCH v2 1/2] daemon: move daemonize() to libgit.a

2014-02-11 Thread Junio C Hamano
Duy Nguyen pclo...@gmail.com writes:

 On Tue, Feb 11, 2014 at 1:46 AM, Junio C Hamano gits...@pobox.com wrote:
 Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 diff --git a/setup.c b/setup.c
 index 6c3f85f..b09a412 100644
 --- a/setup.c
 +++ b/setup.c
 @@ -787,3 +787,27 @@ void sanitize_stdfds(void)
   if (fd  2)
   close(fd);
  }
 +
 +int daemonize(void)
 +{
 +#ifdef NO_POSIX_GOODIES
 + errno = -ENOSYS;

 Negated?

 Facepalm. I remember I wrote this somewhere but don't remember what
 topic :( Should I resend?

I've already amended it here.
--
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 02/14] trailer: process trailers from file and arguments

2014-02-11 Thread Junio C Hamano
Christian Couder christian.cou...@gmail.com writes:

 Even if we assume, for the sake of discussion, that it *is* a good
 idea to separate under this condition part and do this part, I
 do not think the above is the only or the best way to express
 distinct values allowed for the same key.  How do we argue that
 this from your example

   if_exists = add_if_different
   if_missing = add

 is a better design than this, for example?

   if_key_value_exists = ignore ; exact matching key,val exists
   if_key_exists = add  ; otherwise
   if_missing = add

 The question is what happens if we want to say if the same key,
 value pair exists but not near where we would add.

 With the solution I implemented, that is:
 ...
 With the solution you suggest, should we have:
 ...
   if_key_value_exists_not_neighbor = add ; exact matching key,val
 ...
 or:
   if_key_value_exists = ignore_if_neighbor ; exact matching key,val exists
 ...

 And what happens if we want to say if key exists and value matches
 regex, how do we express that then?
 Or what happens when we want to say if key exists and command succeeds?
 ...
 With what I suggest, we can still say:
 ...
 And then people might ask if they can also use something like this:
 ...
 and it will look like we are trying too much to do what should be done
 in only one script.

I think the above illustrates my point very clearly.

These numerous questions you have to ask are indications why
choosing this condition goes to the left hand side of the equal
sign (e.g. exists) and this condition goes to the right hand side
(e.g. do-this-if_neighbor) is not working well.  The user has to
remember which conditions go to the variable name and which
conditions go to the action part.

And the made-up alternative you listed above is not a solution I
suggest to begin with---it is a strawman if we assume such a
splitting were a good idea in the first place ;-).

What I was wondering if it is an better alternative was the below.

 The latter splits remaining conditional logic from do this part
 (no more add_if_different that causes add action to see if there
 is the same key and act differently) and moves it to under this
 condition part.
 ...
 I am trying to illustrate that the line to split the under this
 condition and do this looks arbitrary and fuzzy here, and because
 of this arbitrary-ness and fuzziness, it is not very obvious to me
 why it is a good idea to have under this condition as a separate
 concept from do this settings.

That is, not splitting the logic into two parts like you did,
i.e. if_X = do_Y_if_Z, which invites why is it not if_true =
do_Y_if_X_and_Z, if_X_and_Z = do_Y, if_Z = do_Y_if_X?

One possible way to avoid the confusion is to say do_Y_if_X_and_Z
without making the rule split into conditions and actions---I am
NOT saying that is _better_, there may be other solutions to avoid
this two-part logic in a cleaner way.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [RFH] hackday and GSoC topic suggestions

2014-02-11 Thread Junio C Hamano
Jeff King p...@peff.net writes:

  - Branch rename breaks local downstream branches
http://article.gmane.org/gmane.comp.version-control.git/241228

If you have a branch B that builds on A, if you are renaming A to C,
you may want B to automatically set to build on C in some cases, and
in other cases your renaming A is done explicitly in order to severe
the tie between A and B and setting the latter to build on C can be
a bad thing---after all, the user's intention may be to create a
branch A starting at some commit immediately after this rename so
that B will keep building on that updated A.

So I am not sure if this is a bug.

  - git stash doesn't use --index as default
http://article.gmane.org/gmane.comp.version-control.git/235892

I tend to think git stash was designed to work this way from day
one.

  - using git commit-tree with -F - adds trailing newlines
http://article.gmane.org/gmane.comp.version-control.git/236583

According to the initial commit that introduced this option, it
deliberately calls strbuf_complete_line() to make sure we do not
leave an incomplete line at the end.


Many of the other items in your bugs section seem to be worth
fixing.

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] bash completion: Add --recurse-submodules

2014-02-11 Thread Junio C Hamano
Keshav Kini keshav.k...@gmail.com writes:

 Sup Yut Sum ch3co...@gmail.com writes:

 Signed-off-by: Sup Yut Sum ch3co...@gmail.com
 ---
  contrib/completion/git-completion.bash | 19 ++-
  1 file changed, 18 insertions(+), 1 deletion(-)

 Aren't you missing a commit message?

The title itself is almost sufficient, I would think.  It may need
to mention that this is only for fetch, pull and push.  I'll
tentatively queue the following.

Stripping the leftmost constant string with ${var##constant} looks
somewhat strange (why wouldn't a single # work?), but that is not a
new problem this patch introduces, and can be cleaned up separately
if/when somebody wants to.

-- 8 --
From: Sup Yut Sum ch3co...@gmail.com
Date: Sun, 9 Feb 2014 22:35:31 +0800
Subject: [PATCH] completion: teach --recurse-submodules to fetch, pull and push

Signed-off-by: Sup Yut Sum ch3co...@gmail.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 contrib/completion/git-completion.bash | 19 ++-
 1 file changed, 18 insertions(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 8aaf214..c044a68 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1221,14 +1221,20 @@ _git_difftool ()
__git_complete_revlist_file
 }
 
+__git_fetch_recurse_submodules=yes on-demand no
+
 __git_fetch_options=
--quiet --verbose --append --upload-pack --force --keep --depth=
-   --tags --no-tags --all --prune --dry-run
+   --tags --no-tags --all --prune --dry-run --recurse-submodules=
 
 
 _git_fetch ()
 {
case $cur in
+   --recurse-submodules=*)
+   __gitcomp $__git_fetch_recurse_submodules  
${cur##--recurse-submodules=}
+   return
+   ;;
--*)
__gitcomp $__git_fetch_options
return
@@ -1577,6 +1583,10 @@ _git_pull ()
__git_complete_strategy  return
 
case $cur in
+   --recurse-submodules=*)
+   __gitcomp $__git_fetch_recurse_submodules  
${cur##--recurse-submodules=}
+   return
+   ;;
--*)
__gitcomp 
--rebase --no-rebase
@@ -1589,6 +1599,8 @@ _git_pull ()
__git_complete_remote_or_refspec
 }
 
+__git_push_recurse_submodules=check on-demand
+
 _git_push ()
 {
case $prev in
@@ -1601,10 +1613,15 @@ _git_push ()
__gitcomp_nl $(__git_remotes)  ${cur##--repo=}
return
;;
+   --recurse-submodules=*)
+   __gitcomp $__git_push_recurse_submodules  
${cur##--recurse-submodules=}
+   return
+   ;;
--*)
__gitcomp 
--all --mirror --tags --dry-run --force --verbose
--receive-pack= --repo= --set-upstream
+   --recurse-submodules=

return
;;
-- 
1.9.0-rc3-244-g3497008

--
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] Introduce experimental remote object access mode

2014-02-11 Thread Junio C Hamano
Shawn Pearce spea...@spearce.org writes:

 Why would you do this? Perhaps you need more time in your day
 to consume tea or coffee. Set GIT_RTT and enjoy a beverage.

So the conclusion is that it is not practical to do a lazy fetch if
it is done extremely naively at we want this object --- wait a bit
and we'll give you level?

I am wondering if we can do a bit better, like we want this object
--- wait a bit, ah that's a commit, so it is likely that you may
want the trees and blobs associated with it, too, if not right now
but in a near future, let me push a pack that holds them to you?


 So-not-signed-off-by: this author or anyone else
 ---

   :-)

  sha1_file.c | 14 ++
  1 file changed, 14 insertions(+)

 diff --git a/sha1_file.c b/sha1_file.c
 index 6e8c05d..9bdcbc3 100644
 --- a/sha1_file.c
 +++ b/sha1_file.c
 @@ -38,6 +38,7 @@ const unsigned char null_sha1[20];

  static const char *no_log_pack_access = no_log_pack_access;
  static const char *log_pack_access;
 +static useconds_t rtt;

  /*
   * This is meant to hold a *small* number of objects that you would
 @@ -436,9 +437,20 @@ void prepare_alt_odb(void)
   read_info_alternates(get_object_directory(), 0);
  }

 +static void apply_rtt()
 +{
 + if (!rtt) {
 + char *rtt_str = getenv(GIT_RTT);
 + rtt = rtt_str ? strtoul(rtt_str, NULL, 10) * 1000 : 1;
 + }
 + if (rtt  1)
 + usleep(rtt);
 +}
 +
  static int has_loose_object_local(const unsigned char *sha1)
  {
   char *name = sha1_file_name(sha1);
 + apply_rtt();
   return !access(name, F_OK);
  }

 @@ -1303,6 +1315,7 @@ void prepare_packed_git(void)

   if (prepare_packed_git_run_once)
   return;
 +
   prepare_packed_git_one(get_object_directory(), 1);
   prepare_alt_odb();
   for (alt = alt_odb_list; alt; alt = alt-next) {
 @@ -1439,6 +1452,7 @@ static int open_sha1_file(const unsigned char *sha1)
   struct alternate_object_database *alt;

   fd = git_open_noatime(name);
 + apply_rtt();
   if (fd = 0)
   return fd;
--
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] contrib/diff-highlight: multibyte characters diff

2014-02-11 Thread Junio C Hamano
Yoshihiro Sugi sugi1...@gmail.com writes:

 Signed-off-by: Yoshihiro Sugi sugi1...@gmail.com

 diff-highlight split each hunks and compare them as byte sequences.
 it causes problems when diff hunks include multibyte characters.
 This change enable to work on such cases by decoding inputs and encoding 
 output as utf8 string.
 ---
  contrib/diff-highlight/diff-highlight | 16 ++--
  1 file changed, 10 insertions(+), 6 deletions(-)

 diff --git a/contrib/diff-highlight/diff-highlight 
 b/contrib/diff-highlight/diff-highlight
 index c4404d4..49b4f53 100755
 --- a/contrib/diff-highlight/diff-highlight
 +++ b/contrib/diff-highlight/diff-highlight
 @@ -2,6 +2,7 @@
  
  use warnings FATAL = 'all';
  use strict;
 +use Encode qw(decode_utf8 encode_utf8);
  
  # Highlight by reversing foreground and background. You could do
  # other things like bold or underline if you prefer.
 @@ -15,8 +16,9 @@ my @added;
  my $in_hunk;
  
  while () {
 + $_ = decode_utf8($_);
   if (!$in_hunk) {
 - print;
 + print encode_utf8($_);
   $in_hunk = /^$COLOR*\@/;
   }
   elsif (/^$COLOR*-/) {
 @@ -30,7 +32,7 @@ while () {
   @removed = ();
   @added = ();
  
 - print;
 + print encode_utf8($_);
   $in_hunk = /^$COLOR*[\@ ]/;
   }
  
 @@ -58,7 +60,8 @@ sub show_hunk {
  
   # If one side is empty, then there is nothing to compare or highlight.
   if (!@$a || !@$b) {
 - print @$a, @$b;
 + print encode_utf8($_) for @$a;
 + print encode_utf8($_) for @$b;
   return;
   }
  
 @@ -67,17 +70,18 @@ sub show_hunk {
   # stupid, and only handle multi-line hunks that remove and add the same
   # number of lines.
   if (@$a != @$b) {
 - print @$a, @$b;
 + print encode_utf8($_) for @$a;
 + print encode_utf8($_) for @$b;
   return;
   }
  
   my @queue;
   for (my $i = 0; $i  @$a; $i++) {
   my ($rm, $add) = highlight_pair($a-[$i], $b-[$i]);
 - print $rm;
 + print encode_utf8($rm);
   push @queue, $add;
   }
 - print @queue;
 + print encode_utf8($_) for @queue;
  }
  
  sub highlight_pair {
--
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: [RFH] hackday and GSoC topic suggestions

2014-02-11 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Jeff King p...@peff.net writes:

  - Branch rename breaks local downstream branches
http://article.gmane.org/gmane.comp.version-control.git/241228

 If you have a branch B that builds on A, if you are renaming A to C,
 you may want B to automatically set to build on C in some cases, and
 in other cases your renaming A is done explicitly in order to severe
 the tie between A and B and setting the latter to build on C can be
 a bad thing---after all, the user's intention may be to create a
 branch A starting at some commit immediately after this rename so
 that B will keep building on that updated A.

 So I am not sure if this is a bug.

Having said that, the current behaviour of leaving B half-configured
to build on a missing branch is undesirable. If we were to change
this so that any branch B that used to build on branch A being
renamed to build on the branch under the new name C, the user may
have to do an extra --set-upstream-to A on B after recreating A if
this was done to save away the current state of A to C and then keep
building B on an updated A, so we may have to give _some_ clue what
we are doing behind their back when we rename, e.g.

$ git branch -m A C
warning: branch B is set to build on C now.

or something.
--
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: turn on network daemon tests by default

2014-02-11 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 I dug in the history to see if there was any reasoning given for the
 current off by default setting. It looks like Junio asked for it when
 the original http-push tests were added, and everything else just
 followed that. The reasoning there was basically they're heavyweight
 and we might not be able to run them, but hopefully having the option
 variable will make that OK.

Will queue, thanks.

I may have originally asked for it for one reason, thinking that one
reason would be enough while having another reason not to run them
as well.  But there would be countless silent others who have been
depending on that choice.

Those who run buildfarms may want to disable the networking test if
the buildfarms are not isolated well, for example.  They have to be
told somewhere that now they need to explicitly disable these tests
and how.

I am in favor of this change but just pointing out possible fallouts
might be larger than we think.

  t/lib-git-daemon.sh |  8 +---
  t/lib-httpd.sh  | 22 +++---
  t/test-lib-functions.sh | 44 
  3 files changed, 60 insertions(+), 14 deletions(-)

 diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
 index 1f22de2..e623bef 100644
 --- a/t/lib-git-daemon.sh
 +++ b/t/lib-git-daemon.sh
 @@ -16,9 +16,10 @@
  #stop_git_daemon
  #test_done
  
 -if test -z $GIT_TEST_GIT_DAEMON
 +GIT_TEST_GIT_DAEMON=$(test_tristate $GIT_TEST_GIT_DAEMON)
 +if test $GIT_TEST_GIT_DAEMON = false
  then
 - skip_all=git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to 
 enable)
 + skip_all=git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to 
 enable)
   test_done
  fi
  
 @@ -58,7 +59,8 @@ start_git_daemon() {
   kill $GIT_DAEMON_PID
   wait $GIT_DAEMON_PID
   trap 'die' EXIT
 - error git daemon failed to start
 + test_skip_or_die $GIT_TEST_GIT_DAEMON \
 + git daemon failed to start
   fi
  }
  
 diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
 index b43162e..bb900ca 100644
 --- a/t/lib-httpd.sh
 +++ b/t/lib-httpd.sh
 @@ -30,9 +30,10 @@
  # Copyright (c) 2008 Clemens Buchacher dri...@aon.at
  #
  
 -if test -z $GIT_TEST_HTTPD
 +GIT_TEST_HTTPD=$(test_tristate $GIT_TEST_HTTPD)
 +if test $GIT_TEST_HTTPD = false
  then
 - skip_all=Network testing disabled (define GIT_TEST_HTTPD to enable)
 + skip_all=Network testing disabled (unset GIT_TEST_HTTPD to enable)
   test_done
  fi
  
 @@ -76,8 +77,7 @@ GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export 
 GIT_VALGRIND_OPTIONS
  
  if ! test -x $LIB_HTTPD_PATH
  then
 - skip_all=skipping test, no web server found at '$LIB_HTTPD_PATH'
 - test_done
 + test_skip_or_die $GIT_TEST_HTTPD no web server found at 
 '$LIB_HTTPD_PATH'
  fi
  
  HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \
 @@ -89,19 +89,20 @@ then
   then
   if ! test $HTTPD_VERSION -ge 2
   then
 - skip_all=skipping test, at least Apache version 2 is 
 required
 - test_done
 + test_skip_or_die $GIT_TEST_HTTPD \
 + at least Apache version 2 is required
   fi
   if ! test -d $DEFAULT_HTTPD_MODULE_PATH
   then
 - skip_all=Apache module directory not found.  Skipping 
 tests.
 - test_done
 + test_skip_or_die $GIT_TEST_HTTPD \
 + Apache module directory not found
   fi
  
   LIB_HTTPD_MODULE_PATH=$DEFAULT_HTTPD_MODULE_PATH
   fi
  else
 - error Could not identify web server at '$LIB_HTTPD_PATH'
 + test_skip_or_die $GIT_TEST_HTTPD \
 + Could not identify web server at '$LIB_HTTPD_PATH'
  fi
  
  prepare_httpd() {
 @@ -155,9 +156,8 @@ start_httpd() {
   3 24
   if test $? -ne 0
   then
 - skip_all=skipping test, web server setup failed
   trap 'die' EXIT
 - test_done
 + test_skip_or_die $GIT_TEST_HTTPD web server setup failed
   fi
  }
  
 diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
 index aeae3ca..3cc09c0 100644
 --- a/t/test-lib-functions.sh
 +++ b/t/test-lib-functions.sh
 @@ -716,6 +716,50 @@ perl () {
   command $PERL_PATH $@
  }
  
 +# Normalize the value given in $1 to one of true, false, or auto. The
 +# result is written to stdout. E.g.:
 +#
 +# GIT_TEST_HTTPD=$(test_tristate $GIT_TEST_HTTPD)
 +#
 +test_tristate () {
 + case $1 in
 + |auto)
 + echo auto
 + ;;
 + *)
 + # Rely on git-config to handle all the variants of
 + # true/1/on/etc that we allow.
 + if ! git -c magic.hack=$1 config --bool magic.hack 2/dev/null
 + then
 + # If git-config failed, it was some non-bool value like
 +  

Re: [PATCH 00/11] More preparatory work for multiparent tree-walker

2014-02-11 Thread Junio C Hamano
Kirill Smelkov k...@mns.spb.ru writes:

 Sorry for the confusion. Could you please do the following:

 Patches should be applied over to ks/tree-diff-walk
 (74aa4a18). Before applying the patches, please cherry-pick

 c90483d9(tree-diff: no need to manually verify that there is no
  mode change for a path)

 ef4f0928(tree-diff: no need to pass match to
  skip_uninteresting())

 into that branch, and drop them from ks/combine-diff - we'll have one
 branch reworking the diff tree-walker, and the other taking advantage of
 it for combine-diff.

As long as that does not lose changes to tests and clean-ups, I'm
fine with that direction.  For example, I do not know if you want to
lose e3f62d12 (diffcore-order: export generic ordering interface,
2014-01-20), which is part of the combine-diff topic.




--
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: turn on network daemon tests by default

2014-02-11 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 On Tue, Feb 11, 2014 at 11:51:18AM -0800, Junio C Hamano wrote:

 Those who run buildfarms may want to disable the networking test if
 the buildfarms are not isolated well, for example.  They have to be
 told somewhere that now they need to explicitly disable these tests
 and how.

 I think they should be OK. The daemons run on the loopback interface, so
 there is hopefully not a security implication. If multiple buildfarms
 are sharing the same loopback space (e.g., running in separate
 directories on the same machine), the auto setting should degrade
 gracefully. One daemon will win the setup, and the tests will run, and
 on the other, they will be skipped.

 I am in favor of this change but just pointing out possible fallouts
 might be larger than we think.

 Agreed, but I think the only way to know the size of those fallouts is
 to try it and see who complains.  I would not normally be so cavalier
 with git itself, but I think for the test infrastructure, we have a
 small, tech-savvy audience that can help us iterate on it without too
 much pain.

Sure. One immediate complaint is that I would probably need to do
something like this in the build automation:

if testing a branch without this patch
then
: do nothing
else
GIT_TEST_GIT_DAEMON=false
fi

Arguably, it is the fault of the current/original code that treated
*any* non-empty value that is set in the environment variable as
true---if it paid attention to GIT_TEST_GIT_DAEMON=NoThanks, we
wouldn't have to have a workaround like this.

I wonder if GIT_TEST_X=$(test_tristate $GIT_TEST_X) pattern can be
made a bit more friendly, though.  For example, can we behave
differently depending on the reason why $GIT_TEST_X is empty?

 - People who have *not* been opting in to the expensive tests have
   not done anything special; GIT_TEST_X environment variable did
   not exist for them (i.e. unset), and we used to skip when
   $GIT_TEST_X is an empty string.

 - We want to encourage people who do not care to run these tests.
   If people do not do anything, their $GIT_TEST_X will continue to
   be an empty string without GIT_TEST_X variable in the
   environment.

If we let people who *do* want to opt out of the expensive tests by
explicitly setting $GIT_TEST_X to an empty string in the new scheme,
wouldn't the transition go a lot smoother?

The caller may have to pass the name of the variable:

GIT_TEST_DAEMON=$(test_tristate GIT_TEST_DAEMON)

and then the callee may become

test_tristate () {
variable=$1
if eval test x\\${$variable+isset}\ = xisset
then
# explicitly set
eval value=\$$variable
case $value in
)
echo false ;;
false | auto)
echo $value ;;
*)
echo true ;;
esac
else
echo auto
fi
}

so that

 - Any non-empty string other than the magic strings false and
   auto continue to mean please I want to test;

 - Setting the variable explicitly to an empty string will continue
   to mean no I do not want to test;

 - Leaving the variable unset will continue to mean I don't really
   care; just follow the default the project gives me.

--
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-note -C changes commit type?

2014-02-11 Thread Junio C Hamano
Johan Herland jo...@herland.net writes:

 There is currently no way the git notes commands will allow you to
 store the 3d7de37 commit object directly as a note. There is also
 (AFAICS) no easy workaround (git fast-import could've been a
 workaround if it did not already require the first N/notemodify
 argument to be a blob object). The best alternative, off the top of my
 head, would be to write your own program using the notes.h API to
 manipulate the notes tree directly (or - suboptimally - use other
 low-level Git operations to do the same).

Even worse. I do not think such a non-blob object in the notes tree
does not participate in the reachability at all, so you won't be
able to fetch refs/notes/whatever and expect to get a useful
result.  I do not think storing the raw bits of commit object as a
blob in the notes tree is useful behaviour, either.  The command
probably should refuse to get anything non-blob via that option.

Perhaps the notes entry should just note the object name of whatever
commit it wants to refer to in a *blob*?
--
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 00/11] More preparatory work for multiparent tree-walker

2014-02-12 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Kirill Smelkov k...@mns.spb.ru writes:

 Sorry for the confusion. Could you please do the following:

 Patches should be applied over to ks/tree-diff-walk
 (74aa4a18). Before applying the patches, please cherry-pick

 c90483d9(tree-diff: no need to manually verify that there is no
  mode change for a path)

 ef4f0928(tree-diff: no need to pass match to
  skip_uninteresting())

 into that branch, and drop them from ks/combine-diff - we'll have one
 branch reworking the diff tree-walker, and the other taking advantage of
 it for combine-diff.

 As long as that does not lose changes to tests and clean-ups, I'm
 fine with that direction.  For example, I do not know if you want to
 lose e3f62d12 (diffcore-order: export generic ordering interface,
 2014-01-20), which is part of the combine-diff topic.

Ahh, sorry, I misread the drop as salvage these two and drop the
rest.  The new series does apply cleanly on a commit in master..pu
that has both ks/tree-diff-walk and ks/combine-diff, and the latter
is not yet in 'next' so we are free to reorganize.

Let me flip the latter topic around, also queue these updates and
push the result out on 'pu'.

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 v2 2/2] gc: config option for running --auto in background

2014-02-12 Thread Junio C Hamano
Duy Nguyen pclo...@gmail.com writes:

 On Tue, Feb 11, 2014 at 2:11 AM, Junio C Hamano gits...@pobox.com wrote:
 On Mon, Feb 10, 2014 at 10:43 AM, Junio C Hamano gits...@pobox.com wrote:
 If --quiet is set, we should not be printing anyway. If not, I thinkg
 we could only print auto packing in background.. when we actually
 can do that, else just print the old message. It means an #ifdef
 NO_POSIX_GOODIES here again though..

 Didn't you change it not to die but return nosys or something?

 Ah, the problem is that it is too late to take back ... will do so in
 the background when you noticed that daemonize() did not succeed, so
 you would need a way to see if we can daemonize() before actually
 doing so if you want to give different messages.

 int can_daemonize(void) could be an answer that is nicer than
 NO_POSIX_GOODIES, but I am not sure if it is worth it.

 Or we could pass the quiet flag to daemonize() and let it print
 something in the #ifdef NO_POSIX_GOODIES part.

Hmph...  What would that something say?  I was asked to gc in the
background but I can't here is not suitable for daemonize() that is
not specific to gc.

The flow I had in mind was something along the lines of this

if (!quiet) {
if (detach_auto  can_daemonize())
say auto packing in the background;
else
say auto packing
}
if (detach_auto  can_daemonize())
daemonize();

If we had daemonize(noisy=1) and coded it this way:

if (!quiet)
say auto packing;
if (detach_auto)
daemonize(!quiet);

we could do something like:

daemonize(int noisy) {
if (noisy  !defined(NO_POSIX_GOODIES))
say , and doing so in the background;
... do the actual daemonizing ...
}

but that feels 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


Re: [PATCH] tests: turn on network daemon tests by default

2014-02-12 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 Agreed, but I think the only way to know the size of those fallouts is
 to try it and see who complains.  I would not normally be so cavalier
 with git itself, but I think for the test infrastructure, we have a
 small, tech-savvy audience that can help us iterate on it without too
 much pain.

There is another.

$ GIT_TEST_HTTPD=false sh t5537-fetch-shallow.sh 
ok 1 - setup
ok 2 - setup shallow clone
ok 3 - clone from shallow clone
ok 4 - fetch from shallow clone
ok 5 - fetch --depth from shallow clone
ok 6 - fetch --unshallow from shallow clone
ok 7 - fetch something upstream has but hidden by clients shallow boundaries
ok 8 - fetch that requires changes in .git/shallow is filtered
ok 9 - fetch --update-shallow
error: Can't use skip_all after running some tests

Under 'prove' test target, the way it exits causes:

*** prove ***
t5537-fetch-shallow.sh .. Dubious, test returned 1 (wstat 256, 0x100)
All 9 subtests passed

which leads to:

Test Summary Report
---
t5537-fetch-shallow.sh (Wstat: 256 Tests: 9 Failed: 0)
  Non-zero exit status: 1
  Parse errors: No plan found in TAP output


On the 'master' branch without these auto opt-in patches,

$ GIT_TEST_HTTPD= sh t5537-fetch-shallow.sh 
ok 1 - setup
ok 2 - setup shallow clone
ok 3 - clone from shallow clone
ok 4 - fetch from shallow clone
ok 5 - fetch --depth from shallow clone
ok 6 - fetch --unshallow from shallow clone
ok 7 - fetch something upstream has but hidden by clients shallow boundaries
ok 8 - fetch that requires changes in .git/shallow is filtered
ok 9 - fetch --update-shallow
skipping remaining tests, git built without http support
# passed all 9 test(s)
1..9

--
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] Make the global packed_git variable static to sha1_file.c.

2014-02-12 Thread Junio C Hamano
Stefan Zager sza...@google.com writes:

 If anyone has a recommendation for a less labor-intensive way to do
 this in emacs, I'd be very grateful.

This is not do this in emacs, but here is a possible approach.

You can ask git diff about what you changed, and actually apply
the change while fixing whitespace errors.  I.e.

git diff sha1_file.c | git apply --cached --whitespace=fix
git diff
git checkout sha1_file.c

The first step will add a cleaned-up version to your index.

The second diff (optional) is to see what whitespace errors are
introduced when going from that cleaned-up version to what you have
in the working tree.

With the last step you would update the working tree version to the
cleaned-up version from the index.

[alias]
wsadd = !sh -c 'git diff -- \$@\ | git apply --cached 
--whitespace=fix;\
git co -- ${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] blame.c: prepare_lines should not call xrealloc for every line

2014-02-12 Thread Junio C Hamano
David Kastrup d...@gnu.org writes:

 Making a single preparation run for counting the lines will avoid memory
 fragmentation.  Also, fix the allocated memory size which was wrong
 when sizeof(int *) != sizeof(int), and would have been too small
 for sizeof(int *)  sizeof(int), admittedly unlikely.

 Signed-off-by: David Kastrup d...@gnu.org
 ---

I think I took sizeof(int*)-sizeof(int) patch to the 'next' branch
already, which might have to conflict with this clean-up, but it
should be trivial to resolve.

Thanks for resending.  I was busy elsewhere (i.e. no feedback does
not mean silent rejection nor silent agreement at least from
me), and such a resend does help prevent patches fall thru cracks.

 diff --git a/builtin/blame.c b/builtin/blame.c
 index e44a6bb..1aefedf 100644
 --- a/builtin/blame.c
 +++ b/builtin/blame.c
 @@ -1772,25 +1772,41 @@ static int prepare_lines(struct scoreboard *sb)
  {
   const char *buf = sb-final_buf;
   unsigned long len = sb-final_buf_size;
 + const char *end = buf + len;
 + const char *p;
 + int *lineno;
 + int num = 0, incomplete = 0;
  
 + for (p = buf;;) {
 + p = memchr(p, '\n', end - p);
 + if (p) {
 + p++;
   num++;
 + continue;
   }
 + break;
   }
 +
 + if (len  end[-1] != '\n')
 + incomplete++; /* incomplete line at the end */
 +
 + sb-lineno = xmalloc(sizeof(*sb-lineno) * (num + incomplete + 1));
 + lineno = sb-lineno;
 +
 + *lineno++ = 0;
 + for (p = buf;;) {
 + p = memchr(p, '\n', end - p);
 + if (p) {
 + p++;
 + *lineno++ = p - buf;
 + continue;
 + }
 + break;
 + }
 +
 + if (incomplete)
 + *lineno++ = len;
 +
   sb-num_lines = num + incomplete;
   return sb-num_lines;
  }
--
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: Make the git codebase thread-safe

2014-02-12 Thread Junio C Hamano
Stefan Zager sza...@chromium.org writes:

 ...  I used the Very Sleepy profiler
 to see where all the time was spent on Windows: 55% of the time was
 spent in OpenFile, and 25% in CloseFile (both in win32).

This is somewhat interesting.

When we check things out, checkout_paths() has a list of paths to be
checked out, and iterates over them and call checkout_entry().

I wonder if you can:

 - introduce a version of checkout_entry() that takes file
   descriptors to write to;

 - have an asynchronous helper threads that pre-open the paths to be
   written out and feed ce, file descriptor to be written to a
   queue;

 - restructure that loop so that it reads the ce, file descriptor
   to be written from the queue, performs the actual writing out,
   and then feeds file descriptor to be closed to another queue; and

 - have another asynchronous helper threads that reads file
   descriptor to be closed from the queue and close them.

Calls to write (and preparation of data to be written) will then
remain single-threaded, but it sounds like that codepath is not the
bottleneck in your measurement, so

--
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 v2] Make the global packed_git variable static to sha1_file.c.

2014-02-12 Thread Junio C Hamano
I'll locally fix up these style issues before commenting on the patch.

Thanks.

ERROR: space required after that ',' (ctx:VxV)
#78: FILE: builtin/count-objects.c:111:
+   struct pack_data pd = {0,0,0};
^

ERROR: space required after that ',' (ctx:VxV)
#78: FILE: builtin/count-objects.c:111:
+   struct pack_data pd = {0,0,0};
  ^

ERROR: space required after that ',' (ctx:VxV)
#171: FILE: builtin/fsck.c:683:
+   struct verify_packs_data vpd = {0,0,0};
 ^

ERROR: space required after that ',' (ctx:VxV)
#171: FILE: builtin/fsck.c:683:
+   struct verify_packs_data vpd = {0,0,0};
   ^

ERROR: space required after that ',' (ctx:VxV)
#541: FILE: builtin/pack-redundant.c:591:
+   struct add_pack_data apd = {filename,0,NULL};
^

ERROR: space required after that ',' (ctx:VxV)
#541: FILE: builtin/pack-redundant.c:591:
+   struct add_pack_data apd = {filename,0,NULL};
  ^

ERROR: space required after that ',' (ctx:VxV)
#565: FILE: builtin/pack-redundant.c:604:
+   struct add_pack_data apd = {NULL,0,NULL};
^

ERROR: space required after that ',' (ctx:VxV)
#565: FILE: builtin/pack-redundant.c:604:
+   struct add_pack_data apd = {NULL,0,NULL};
  ^

ERROR: space prohibited after that '-' (ctx:WxW)
#726: FILE: pack-revindex.c:47:
+   num = - 1 - num;
  ^

ERROR: space required after that ',' (ctx:VxV)
#901: FILE: sha1_name.c:195:
+   struct disambiguate_data d = {len,bin_pfx,ds};
 ^

ERROR: space required after that ',' (ctx:VxV)
#901: FILE: sha1_name.c:195:
+   struct disambiguate_data d = {len,bin_pfx,ds};
 ^

total: 11 errors, 0 warnings, 781 lines checked
--
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 v2] Make the global packed_git variable static to sha1_file.c.

2014-02-12 Thread Junio C Hamano
sza...@chromium.org writes:

 From 0a59547f3e95ddecf7606c5f259ae6177c5a104f Mon Sep 17 00:00:00 2001

Please drop this line.

 From: Stefan Zager sza...@chromium.org

Please drop this line and instead have it in your e-mail header.

 Date: Mon, 10 Feb 2014 16:55:12 -0800

The date in your e-mail header, which is the first time general
public saw this particular version of the patch, is used by default
as the author time.  Unless there is a compelling reason to override
that with an in-body header, please drop this line.

 Subject: [PATCH] Make the global packed_git variable static to sha1_file.c.

Please drop this line and instead have it in your e-mail header.

 This patch is a pure refactor with no functional changes,...

 diff --git a/builtin/count-objects.c b/builtin/count-objects.c
 index a7f70cb..6554dfe 100644
 --- a/builtin/count-objects.c
 +++ b/builtin/count-objects.c
 @@ -83,14 +83,32 @@ static char const * const count_objects_usage[] = {
   NULL
  };
  
 +struct pack_data {
 + unsigned long packed;
 + off_t size_pack;
 + unsigned long num_pack;
 +};
 +
 +int pack_data_fn(struct packed_git *p, void *data)

Can't/shouldn't this be static?

Also I'd suggest s/pack_data_fn/collect_pack_data/ or something.
_fn may be a good suffix for typedef'ed typename used in a
callback function, but for a concrete function, it only adds noise
without giving us anything new.

Yes, I know there are a few existing violators, but we
shouldn't make the codebase worse, using their existence due
to past carelessness as an excuse.

 diff --git a/cache.h b/cache.h
 index dc040fb..542a9d9 100644
 --- a/cache.h
 +++ b/cache.h
 ...
 +/* The 'hint' argument is for the commonly-used 'last found pack' 
 optimization.
 + * It can be NULL.
 + */

/*
 * Please try to have opening slash-asterisk and closing
 * asterisk-slash in a multi-line comment block on their
 * own lines by themselves.
 */

 +extern void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git 
 *hint, void *data);
 +
 +extern size_t packed_git_count();
 +extern size_t packed_git_local_count();

extern size_t packed_git_count(void);
extern size_t packed_git_local_count(void);

 diff --git a/sha1_file.c b/sha1_file.c
 index 6e8c05d..aeeb7e6 100644
 --- a/sha1_file.c
 +++ b/sha1_file.c
 @@ -60,6 +60,7 @@ static struct cached_object empty_tree = {
   0
  };
  
 +static struct packed_git *packed_git;
  static struct packed_git *last_found_pack;
  
  static struct cached_object *find_cached_object(const unsigned char *sha1)
 @@ -468,7 +469,6 @@ static unsigned int pack_open_fds;
  static unsigned int pack_max_fds;
  static size_t peak_pack_mapped;
  static size_t pack_mapped;
 -struct packed_git *packed_git;

Hmm, any particular reason why only this variable and not others are
moved up?

 @@ -1091,6 +1091,37 @@ struct packed_git *add_packed_git(const char *path, 
 int path_len, int local)
   return p;
  }
  
 +void foreach_packed_git(packed_git_foreach_fn fn, struct packed_git *hint, 
 void *data)
 +{
 + struct packed_git *p;
 + if (hint  ((*fn)(hint, data)))
 + return;
 + for (p = packed_git; p; p = p-next)
 + if (p != hint  (*fn)(p, data))
 + return;

(mental note) In the new API, a non-zero return signals an early
return/break from the loop.

 +}
 +
 +size_t packed_git_count()

size_t packed_git_count(void)

 +{
 + size_t res = 0;
 + struct packed_git *p;
 +
 + for (p = packed_git; p; p = p-next)
 + ++res;

When pre- or post- increment does not make any difference (i.e. you
do not use its value), please stick to post-increment.  pre-increment
looks unusual in our codebase and becomes distracting while reading
it through.

Same comments for packed-git-local-count.

 diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
 index 541667f..bc3074b 100644
 --- a/builtin/pack-objects.c
 +++ b/builtin/pack-objects.c
 @@ -900,14 +900,45 @@ static int no_try_delta(const char *path)
   return 0;
  }
  
 +struct find_pack_data {
 + const unsigned char *sha1;
 + off_t offset;
 + struct packed_git *found_pack;
 + int exclude;
 + int found_non_local_pack;
 + int found_pack_keep;
 +};
 +
 +static int find_pack_fn(struct packed_git *p, void *data)
 +{
 + struct find_pack_data *fpd = (struct find_pack_data *) data;
 + off_t offset = find_pack_entry_one(fpd-sha1, p);
 + if (offset) {
 + if (!fpd-found_pack) {
 + if (!is_pack_valid(p)) {
 + warning(packfile %s cannot be accessed, 
 p-pack_name);
 + return 0;
 + }
 + fpd-offset = offset;
 + fpd-found_pack = p;
 + }
 + if (fpd-exclude)
 + return 1;
 + if (!p-pack_local)
 + fpd-found_non_local_pack = 1;
 + else if 

What's cooking in git.git (Feb 2014, #04; Wed, 12)

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

As a workaround to make life easier for third-party tools, the
upcoming major release will be called Git 1.9.0 (not Git 1.9).
The first maintenance release for it will be Git 1.9.1, and the
major release after Git 1.9.0 will either be Git 2.0.0 or Git
1.10.0.

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

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

--
[New Topics]

* al/docs (2014-02-11) 4 commits
 - docs/git-blame: explain more clearly the example pickaxe use
 - docs/git-clone: clarify use of --no-hardlinks option
 - docs/git-remote: capitalize first word of initial blurb
 - docs/merge-strategies: remove hyphen from mis-merges

 A handful of documentation updates, all trivially harmless.

 Will merge to 'next'.


* jk/test-ports (2014-02-10) 2 commits
 - tests: auto-set git-daemon port
 - tests: auto-set LIB_HTTPD_PORT from test name
 (this branch is tangled with nd/http-fetch-shallow-fix.)

 Avoid having to assign port number to be used in tests manually.

 Will merge to 'next'.


* nd/daemonize-gc (2014-02-10) 2 commits
 - gc: config option for running --auto in background
 - daemon: move daemonize() to libgit.a

 Allow running gc --auto in the background.

 Will merge to 'next'.


* nd/gitignore-trailing-whitespace (2014-02-10) 2 commits
 - dir: ignore trailing spaces in exclude patterns
 - dir: warn about trailing spaces in exclude patterns

 Warn and then ignore trailing whitespaces in .gitignore files,
 unless they are quoted for fnmatch(3), e.g. path\ .


* nd/log-show-linear-break (2014-02-10) 1 commit
 - log: add --show-linear-break to help see non-linear history


* ss/completion-rec-sub-fetch-push (2014-02-11) 1 commit
 - completion: teach --recurse-submodules to fetch, pull and push


* ks/tree-diff-more (2014-02-12) 16 commits
 - tree-diff: reuse base str(buf) memory on sub-tree recursion
 - tree-diff: no need to call full diff_tree_sha1 from show_path()
 - tree-diff: rework diff_tree interface to be sha1 based
 - tree-diff: remove special-case diff-emitting code for empty-tree cases
 - tree-diff: simplify tree_entry_pathcmp
 - tree-diff: show_path prototype is not needed anymore
 - tree-diff: rename compare_tree_entry - tree_entry_pathcmp
 - tree-diff: move all action-taking code out of compare_tree_entry()
 - tree-diff: don't assume compare_tree_entry() returns -1,0,1
 - FIXUP!
 - tree-diff: consolidate code for emitting diffs and recursion in one place
 - tree-diff: show_tree() is not needed
 - tree-diff: no need to pass match to skip_uninteresting()
 - tree-diff: no need to manually verify that there is no mode change for a path
 - combine-diff: move changed-paths scanning logic into its own function
 - combine-diff: move show_log_first logic/action out of paths scanning
 (this branch uses ks/combine-diff and ks/tree-diff-walk.)


* jh/note-trees-record-blobs (2014-02-12) 1 commit
 - notes: Disallow reusing non-blob as a note object


* jk/run-network-tests-by-default (2014-02-12) 1 commit
 - tests: turn on network daemon tests by default

 Teach make test to run networking tests when possible by default.

 Needs a bit more work.
 e.g. $gmane/242013

--
[Stalled]

* po/everyday-doc (2014-01-27) 1 commit
 - Make 'git help everyday' work

 This may make the said command to emit something, but the source is
 not meant to be formatted into a manual pages to begin with, and
 also its contents are a bit stale.  It may be a good first step in
 the right direction, but needs more work to at least get the
 mark-up right before public consumption.

 Will hold.


* jk/branch-at-publish-rebased (2014-01-17) 5 commits
 - t1507 (rev-parse-upstream): fix typo in test title
 - implement @{publish} shorthand
 - branch_get: provide per-branch pushremote pointers
 - branch_get: return early on error
 - sha1_name: refactor upstream_mark

 Give an easier access to the tracking branches from other side in
 a triangular workflow by introducing B@{publish} that works in a
 similar way to how B@{upstream} does.

 Meant to be used as a basis for whatever Ram wants to build on.

 Will hold.


* rb/merge-prepare-commit-msg-hook (2014-01-10) 4 commits
 - merge: drop unused arg from abort_commit method signature
 - merge: make prepare_to_commit responsible for write_merge_state
 - t7505: ensure cleanup after hook blocks merge
 - t7505: add missing 

 Expose more merge states (e.g. $GIT_DIR/MERGE_MODE) to hooks that
 run during git merge.  The log message stresses too much on one
 hook, prepare-commit-msg, but it would equally apply to other hooks
 like post-merge, I think.

 Waiting for a reroll.


* jl/submodule-recursive-checkout (2013-12-26) 5 commits
 - Teach checkout to recursively checkout 

Re: [PATCH] tests: turn on network daemon tests by default

2014-02-12 Thread Junio C Hamano
Jeff King p...@peff.net writes:

   test_normalize_tristate GIT_TEST_DAEMON

Heh, great minds think alike.  This is what I am playing with,
without committing (because I do like your ask config if this is a
kind of various boolean 'false' representations, which I haven't
managed to add to it).


 t/lib-git-daemon.sh |  2 +-
 t/lib-httpd.sh  |  2 +-
 t/test-lib-functions.sh | 45 +++--
 3 files changed, 29 insertions(+), 20 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 36106de..615bf5d 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -16,7 +16,7 @@
 #  stop_git_daemon
 #  test_done
 
-GIT_TEST_GIT_DAEMON=$(test_tristate $GIT_TEST_GIT_DAEMON)
+test_tristate GIT_TEST_GIT_DAEMON
 if test $GIT_TEST_GIT_DAEMON = false
 then
skip_all=git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to 
enable)
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index 583fb14..f9c2e22 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -30,7 +30,7 @@
 # Copyright (c) 2008 Clemens Buchacher dri...@aon.at
 #
 
-GIT_TEST_HTTPD=$(test_tristate $GIT_TEST_HTTPD)
+test_tristate GIT_TEST_HTTPD
 if test $GIT_TEST_HTTPD = false
 then
skip_all=Network testing disabled (unset GIT_TEST_HTTPD to enable)
diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index 3cc09c0..21c5214 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -716,27 +716,36 @@ perl () {
command $PERL_PATH $@
 }
 
-# Normalize the value given in $1 to one of true, false, or auto. The
-# result is written to stdout. E.g.:
+# Given a variable $1, normalize the value of it to one of true,
+# false, or auto and store the result to it.
 #
-# GIT_TEST_HTTPD=$(test_tristate $GIT_TEST_HTTPD)
+# test_tristate GIT_TEST_HTTPD
 #
+# A variable set to an empty string is set to 'false'.
+# A variable set to 'false' or 'auto' keeps its value.
+# Anything else is set to 'true'.
+# An unset variable defaults to 'auto'.
+#
+# The last rule is to allow people to set the variable to an empty
+# string and export it to decline testing the particular feature
+# for versions both before and after this change.  We used to treat
+# both unset and empty variable as a signal for do not test and
+# took any non-empty string as please test.
+
 test_tristate () {
-   case $1 in
-   |auto)
-   echo auto
-   ;;
-   *)
-   # Rely on git-config to handle all the variants of
-   # true/1/on/etc that we allow.
-   if ! git -c magic.hack=$1 config --bool magic.hack 2/dev/null
-   then
-   # If git-config failed, it was some non-bool value like
-   # YesPlease. Default this to true for historical
-   # compatibility.
-   echo true
-   fi
-   esac
+   if eval test x\\${$1+isset}\ = xisset
+   then
+   # explicitly set
+   eval 
+   case \\$$1\ in
+   '') $1=false ;;
+   false | auto) ;;
+   *)  $1=true ;;
+   esac
+   
+   else
+   eval $1=auto
+   fi
 }
 
 # Exit the test suite, either by skipping all remaining tests or by
--
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: Make the git codebase thread-safe

2014-02-12 Thread Junio C Hamano
On Wed, Feb 12, 2014 at 12:27 PM, Stefan Zager sza...@chromium.org wrote:
 On Wed, Feb 12, 2014 at 12:06 PM, Junio C Hamano gits...@pobox.com wrote:
 Stefan Zager sza...@chromium.org writes:

 Calls to write (and preparation of data to be written) will then
 remain single-threaded, but it sounds like that codepath is not the
 bottleneck in your measurement, so

 Yes, I considered that as well.  At a minimum, that would still
 require attr.c to implement thread locking, since attribute files must
 be parsed to look for stream filters.  I have already done that work.

I would have imagined that use of the attribute system belongs to write and
preparation of data to be written category, i.e. the single threaded
part of the
kludge I outlined.

 But I'm not sure it's the best long-term approach to add convoluted
 custom threading solutions to each git operation as it appears on the
 performance radar.

Yeah, it depends on how clean and non-intrusive an abstraction we can make.
The kludge I outlined is certainly not very pretty.
--
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 02/11] tree-diff: consolidate code for emitting diffs and recursion in one place

2014-02-13 Thread Junio C Hamano
Kirill Smelkov k...@mns.spb.ru writes:

 +static void show_path(struct strbuf *base, struct diff_options *opt,
 +   struct tree_desc *t1, struct tree_desc *t2)
  {
   unsigned mode;
   const char *path;
 - const unsigned char *sha1 = tree_entry_extract(desc, path, mode);
 - int pathlen = tree_entry_len(desc-entry);
 + const unsigned char *sha1;
 + int pathlen;
   int old_baselen = base-len;
 + int isdir, recurse = 0, emitthis = 1;
 +
 + /* at least something has to be valid */
 + assert(t1 || t2);
 +
 + if (t2) {
 + /* path present in resulting tree */
 + sha1 = tree_entry_extract(t2, path, mode);
 + pathlen = tree_entry_len(t2-entry);
 + isdir = S_ISDIR(mode);
 + }
 + else {
 + /* a path was removed - take path from parent. Also take
 +  * mode from parent, to decide on recursion.
 +  */
 + tree_entry_extract(t1, path, mode);
 + pathlen = tree_entry_len(t1-entry);
 +
 + isdir = S_ISDIR(mode);
 + sha1 = NULL;
 + mode = 0;
 + }
 +
 + if (DIFF_OPT_TST(opt, RECURSIVE)  isdir) {
 + recurse = 1;
 + emitthis = DIFF_OPT_TST(opt, TREE_IN_RECURSIVE);
 + }
  
   strbuf_add(base, path, pathlen);
 - if (DIFF_OPT_TST(opt, RECURSIVE)  S_ISDIR(mode)) {
 - if (DIFF_OPT_TST(opt, TREE_IN_RECURSIVE))
 - opt-add_remove(opt, *prefix, mode, sha1, 1, base-buf, 
 0);
  
 + if (emitthis)
 + emit_diff(opt, base, t1, t2);
 +
 + if (recurse) {
   strbuf_addch(base, '/');
 - diff_tree_sha1(*prefix == '-' ? sha1 : NULL,
 -*prefix == '+' ? sha1 : NULL, base-buf, opt);
 - } else
 - opt-add_remove(opt, prefix[0], mode, sha1, 1, base-buf, 0);
 + diff_tree_sha1(t1 ? t1-entry.sha1 : NULL,
 +t2 ? t2-entry.sha1 : NULL, base-buf, opt);
 + }


After this step, sha1 is assigned but never gets used.  Please
double-check the fix-up I queued in the series before merging it to
'pu'.

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] tests: turn on network daemon tests by default

2014-02-13 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Jeff King p...@peff.net writes:

   test_normalize_tristate GIT_TEST_DAEMON

 Heh, great minds think alike.  This is what I am playing with,
 without committing (because I do like your ask config if this is a
 kind of various boolean 'false' representations, which I haven't
 managed to add to it).

And this is with the ask config helper.

Two tangents.

 - We may want to do something similar in cvsserver and git-gui to
   make them more robust.

   $ git grep -e true --and -e 1 --and -e yes

 - Do we want to do something similar to GIT_TEST_CREDENTIAL_HELPER?

-- 8 --
From: Jeff King p...@peff.net
Date: Mon, 10 Feb 2014 16:29:37 -0500
Subject: [PATCH] tests: turn on network daemon tests by default

We do not run the httpd nor git-daemon tests by default, as
they are rather heavyweight and require network access
(albeit over localhost). However, it would be nice if more
pepole ran them, for two reasons:

  1. We would get more test coverage on more systems.

  2. The point of the test suite is to find regressions. It
 is very easy to change some of the underlying code and
 break the httpd code without realizing you are even
 affecting it. Running the httpd tests helps find these
 problems sooner (ideally before the patches even hit
 the list).

We still want to leave an out, though, for people who really do
not want to run them. For that reason, the GIT_TEST_HTTPD and
GIT_TEST_GIT_DAEMON variables are now tri-state booleans
(true/false/auto), so you can say GIT_TEST_HTTPD=false to turn the
tests back off.  To support those who want a stable single way to
disable these tests across versions of Git before and after this
change, an empty string explicitly set to these variables is also
taken as false, so the behaviour changes only for those who:

  a. did not express any preference by leaving these variables
 unset.  They did not test these features before, but now they
 do; or

  b. did express that they want to test these features by setting
 GIT_TEST_FEATURE=false (or any equivalent other ways to tell
 false to Git, e.g. 0), which has been a valid but funny way
 to say that they do want to test the feature only because we
 used to interpret any non-empty string to mean yes please
 test.  They no longer test that feature.

In addition, we are forgiving of common setup failures (e.g., you do
not have apache installed, or have an old version) when the
tri-state is auto (or empty), but report an error when it is
true. This makes auto a sane default, as we should not cause
failures on setups where the tests cannot run. But it allows people
who use true to catch regressions in their system (e.g., they
uninstalled apache, but were expecting their automated test runs to
test git-httpd, and would want to be notified).

Signed-off-by: Jeff King p...@peff.net
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 t/lib-git-daemon.sh |  8 ---
 t/lib-httpd.sh  | 22 +--
 t/test-lib-functions.sh | 58 +
 3 files changed, 74 insertions(+), 14 deletions(-)

diff --git a/t/lib-git-daemon.sh b/t/lib-git-daemon.sh
index 394b06b..615bf5d 100644
--- a/t/lib-git-daemon.sh
+++ b/t/lib-git-daemon.sh
@@ -16,9 +16,10 @@
 #  stop_git_daemon
 #  test_done
 
-if test -z $GIT_TEST_GIT_DAEMON
+test_tristate GIT_TEST_GIT_DAEMON
+if test $GIT_TEST_GIT_DAEMON = false
 then
-   skip_all=git-daemon testing disabled (define GIT_TEST_GIT_DAEMON to 
enable)
+   skip_all=git-daemon testing disabled (unset GIT_TEST_GIT_DAEMON to 
enable)
test_done
 fi
 
@@ -58,7 +59,8 @@ start_git_daemon() {
kill $GIT_DAEMON_PID
wait $GIT_DAEMON_PID
trap 'die' EXIT
-   error git daemon failed to start
+   test_skip_or_die $GIT_TEST_GIT_DAEMON \
+   git daemon failed to start
fi
 }
 
diff --git a/t/lib-httpd.sh b/t/lib-httpd.sh
index bfdff2a..f9c2e22 100644
--- a/t/lib-httpd.sh
+++ b/t/lib-httpd.sh
@@ -30,9 +30,10 @@
 # Copyright (c) 2008 Clemens Buchacher dri...@aon.at
 #
 
-if test -z $GIT_TEST_HTTPD
+test_tristate GIT_TEST_HTTPD
+if test $GIT_TEST_HTTPD = false
 then
-   skip_all=Network testing disabled (define GIT_TEST_HTTPD to enable)
+   skip_all=Network testing disabled (unset GIT_TEST_HTTPD to enable)
test_done
 fi
 
@@ -76,8 +77,7 @@ GIT_VALGRIND_OPTIONS=$GIT_VALGRIND_OPTIONS; export 
GIT_VALGRIND_OPTIONS
 
 if ! test -x $LIB_HTTPD_PATH
 then
-   skip_all=skipping test, no web server found at '$LIB_HTTPD_PATH'
-   test_done
+   test_skip_or_die $GIT_TEST_HTTPD no web server found at 
'$LIB_HTTPD_PATH'
 fi
 
 HTTPD_VERSION=`$LIB_HTTPD_PATH -v | \
@@ -89,19 +89,20 @@ then
then
if ! test $HTTPD_VERSION -ge 2
then
-   skip_all=skipping test, at least Apache version 2 is 
required

Re: [PATCH 1/2] tree-diff: rework diff_tree() to generate diffs for multiparent cases as well

2014-02-13 Thread Junio C Hamano
Kirill Smelkov k...@mns.spb.ru writes:

 + /* until we go to it next round, .next holds how many bytes we
 +  * allocated (for faster realloc - we don't need copying old 
 data).
 +  */
 + p-next = (struct combine_diff_path *)alloclen;

I am getting this here:

tree-diff.c: In function '__path_appendnew':
tree-diff.c:140:13: error: cast to pointer from integer of different size 
[-Werror=int-to-pointer-cast]
--
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] combine-diff: speed it up, by using multiparent diff tree-walker directly

2014-02-13 Thread Junio C Hamano
Kirill Smelkov k...@mns.spb.ru writes:

 + if (need_generic_pathscan) {
 + /* NOTE generic case also handles --stat, as it computes
 +  * diff(sha1,parent_i) for all i to do the job, specifically
 +  * for parent0.
 +  */
 + paths = find_paths_generic(sha1, parents, diffopts);
 + }
 + else {
 + paths = find_paths_multitree(sha1, parents, diffopts);
 +
 + /* show stat against the first parent even
 +  * when doing combined diff.
 +  */
 + int stat_opt = (opt-output_format 
 + (DIFF_FORMAT_NUMSTAT|DIFF_FORMAT_DIFFSTAT));

/*
 * We see decl-after-stmt here.
 * Also please have slash-asterisk and asterisk-slash
 * at the beginning and the end of a multi-line comment
 * block on their own line.
 */
--
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] for-each-ref: add option to omit newlines

2014-02-13 Thread Junio C Hamano
Øystein Walle oys...@gmail.com writes:

 On to the patch itself: I contemplated putting '\n' in the default format and
 removing it if -n was given, which would get rid of the need to pass an exta
 argument to show_ref(). But that means we would need to *insert it* when a
 format is given and -n is not...

I would rather see us go in the direction to add -z output option,
which is what everybody else that produces NUL terminated entries in
our suite of subcommands does.

IOW, something along with this line (untested).

 builtin/for-each-ref.c | 6 +-
 1 file changed, 5 insertions(+), 1 deletion(-)

diff --git a/builtin/for-each-ref.c b/builtin/for-each-ref.c
index 51798b4..2c8cac8 100644
--- a/builtin/for-each-ref.c
+++ b/builtin/for-each-ref.c
@@ -94,6 +94,7 @@ static const char **used_atom;
 static cmp_type *used_atom_type;
 static int used_atom_cnt, need_tagged, need_symref;
 static int need_color_reset_at_eol;
+static int line_termination = '\n';
 
 /*
  * Used to parse format string and sort specifiers
@@ -1023,7 +1024,7 @@ static void show_ref(struct refinfo *info, const char 
*format, int quote_style)
resetv.s = color;
print_value(resetv, quote_style);
}
-   putchar('\n');
+   putchar(line_termination);
 }
 
 static struct ref_sort *default_sort(void)
@@ -1088,6 +1089,9 @@ int cmd_for_each_ref(int argc, const char **argv, const 
char *prefix)
OPT_STRING(  0 , format, format, N_(format), N_(format to 
use for the output)),
OPT_CALLBACK(0 , sort, sort_tail, N_(key),
N_(field name to sort on), opt_parse_sort),
+   OPT_SET_INT('z', NULL, line_termination,
+   N_(Use NUL instead of LF to end each output 
records),
+   '\0'),
OPT_END(),
};
 
--
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] t5537: move http tests out to t5539

2014-02-13 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 start_httpd is supposed to be at the beginning of the test file, not
 the middle of it. The test_seq line in no shallow lines.. test is
 updated to compensate missing refs that are there in t5537, but not in
 the new t5539.

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
  On Thu, Feb 13, 2014 at 8:22 AM, Duy Nguyen pclo...@gmail.com wrote:
   On Thu, Feb 13, 2014 at 5:12 AM, Jeff King p...@peff.net wrote:
   lib-httpd was never designed to be included from anywhere except the
   beginning of the file. But that wouldn't be right for t5537, because it
   wants to run some of the tests, even if apache setup fails. The right
   way to do it is probably to have lib-httpd do all of its work in a lazy
   prereq. I don't know how clunky that will end up, though; it might be
   simpler to just move the shallow http test into one of the http-fetch
   scripts.
  
   I'll move it out later.
  
  Here it is, on top of nd/http-fetch-shallow-fix because the new test
  in t5537 is picky and a simple merge resolution wouldn't do it.

Will queue; thanks.


  t/t5537-fetch-shallow.sh   | 57 ---
  t/t5539-fetch-http-shallow.sh (new +x) | 82 
 ++
  2 files changed, 82 insertions(+), 57 deletions(-)

 diff --git a/t/t5537-fetch-shallow.sh b/t/t5537-fetch-shallow.sh
 index 098f220..3ae9092 100755
 --- a/t/t5537-fetch-shallow.sh
 +++ b/t/t5537-fetch-shallow.sh
 @@ -173,61 +173,4 @@ EOF
   )
  '
  
 -if test -n $NO_CURL -o -z $GIT_TEST_HTTPD; then
 - say 'skipping remaining tests, git built without http support'
 - test_done
 -fi
 -
 -. $TEST_DIRECTORY/lib-httpd.sh
 -start_httpd
 -
 -test_expect_success '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 EOF expect 
 -7
 -6
 -5
 -4
 -3
 -EOF
 - test_cmp expect actual
 - )
 -'
 -
 -# This test is tricky. We need large enough haves that fetch-pack
 -# will put pkt-flush in between. Then we need a have the server
 -# does not have, it'll send ACK %s ready
 -test_expect_success 'no shallow lines after receiving ACK ready' '
 - (
 - cd shallow 
 - for i in $(test_seq 10)
 - do
 - git checkout --orphan unrelated$i 
 - test_commit unrelated$i 
 - git push -q $HTTPD_DOCUMENT_ROOT_PATH/repo.git \
 - refs/heads/unrelated$i:refs/heads/unrelated$i 
 - git push -q ../clone/.git \
 - refs/heads/unrelated$i:refs/heads/unrelated$i ||
 - exit 1
 - done 
 - git checkout master 
 - test_commit new 
 - git push  $HTTPD_DOCUMENT_ROOT_PATH/repo.git master
 - ) 
 - (
 - cd clone 
 - git checkout --orphan newnew 
 - test_commit new-too 
 - GIT_TRACE_PACKET=$TRASH_DIRECTORY/trace git fetch --depth=2 
 - grep fetch-pack ACK .* ready ../trace 
 - ! grep fetch-pack done ../trace
 - )
 -'
 -
 -stop_httpd
  test_done
 diff --git a/t/t5539-fetch-http-shallow.sh b/t/t5539-fetch-http-shallow.sh
 new file mode 100755
 index 000..94553e1
 --- /dev/null
 +++ b/t/t5539-fetch-http-shallow.sh
 @@ -0,0 +1,82 @@
 +#!/bin/sh
 +
 +test_description='fetch/clone from a shallow clone over http'
 +
 +. ./test-lib.sh
 +
 +if test -n $NO_CURL; then
 + skip_all='skipping test, git built without http support'
 + test_done
 +fi
 +
 +. $TEST_DIRECTORY/lib-httpd.sh
 +start_httpd
 +
 +commit() {
 + echo $1 tracked 
 + git add tracked 
 + git commit -m $1
 +}
 +
 +test_expect_success 'setup shallow clone' '
 + commit 1 
 + commit 2 
 + commit 3 
 + commit 4 
 + commit 5 
 + commit 6 
 + commit 7 
 + git clone --no-local --depth=5 .git shallow 
 + git config --global transfer.fsckObjects true
 +'
 +
 +test_expect_success '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 EOF expect 
 +7
 +6
 +5
 +4
 +3
 +EOF
 + test_cmp expect actual
 + )
 +'
 +
 +# This test is tricky. We need large enough haves that fetch-pack
 +# will put pkt-flush in between. Then we need a have the server
 +# does not have, it'll send ACK %s ready
 +test_expect_success 'no shallow lines after receiving ACK ready' '
 + (
 + cd shallow 
 + for i in $(test_seq 15)
 + do
 + git checkout --orphan unrelated$i 
 + test_commit unrelated$i 
 +  

Re: [PATCH] release notes: typo fixes

2014-02-13 Thread Junio C Hamano
Michael J Gruber g...@drmicha.warpmail.net writes:

 Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net
 ---
 Just a few things I spotted while trying to keep myself informed :)

Thanks.  Very much appreciated.
--
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


[ANNOUNCE] Git v1.8.5.5

2014-02-13 Thread Junio C Hamano
The latest maintenance release Git v1.8.5.5 is now available at
the usual places.  Hopefully this will be the last update to the
1.8.5.x series.

The release tarballs are found at:

http://code.google.com/p/git-core/downloads/list

and their SHA-1 checksums are:

7bb4ea883b1f8f6f7f927035f85e8e27b57e0194  git-1.8.5.5.tar.gz
39dd7979c8757d2dc4bc3aaa82741ba93557d566  git-htmldocs-1.8.5.5.tar.gz
a4a2aef1440d4751f37c65359da57c9bd51a7beb  git-manpages-1.8.5.5.tar.gz

The following public repositories all have a copy of the v1.8.5.5
tag and the maint branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://code.google.com/p/git-core/
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

Also, http://www.kernel.org/pub/software/scm/git/ has copies of the
release tarballs.

Git v1.8.5.5 Release Notes
==

Fixes since v1.8.5.4


 * The pathspec matching code, while comparing two trees (e.g. git
   diff A B -- path1 path2) was too aggressive and failed to match
   some paths when multiple pathspecs were involved.

 * git repack --max-pack-size=8g stopped being parsed correctly when
   the command was reimplemented in C.

 * A recent update to git send-email broke platforms where
   /etc/ssl/certs/ directory exists but cannot be used as SSL_ca_path
   (e.g. Fedora rawhide).

 * A handful of bugs around interpreting $branch@{upstream} notation
   and its lookalike, when $branch part has interesting characters,
   e.g. @, and :, have been fixed.

 * git clone would fail to clone from a repository that has a ref
   directly under refs/, e.g. refs/stash, because different
   validation paths do different things on such a refname.  Loosen the
   client side's validation to allow such a ref.

 * git log --left-right A...B lost the leftness of commits
   reachable from A when A is a tag as a side effect of a recent
   bugfix.  This is a regression in 1.8.4.x series.

 * git merge-base --octopus used to leave cleaning up suboptimal
   result to the caller, but now it does the clean-up itself.

 * git mv A B/, when B does not exist as a directory, should error
   out, but it didn't.

Also contains typofixes, documentation updates and trivial code clean-ups.



Changes since v1.8.5.4 are as follows:

Andy Spencer (1):
  tree_entry_interesting: match against all pathspecs

Jeff King (9):
  fetch-pack: do not filter out one-level refs
  interpret_branch_name: factor out upstream handling
  interpret_branch_name: rename cp variable to at
  interpret_branch_name: always respect namelen parameter
  interpret_branch_name: avoid @{upstream} past colon
  interpret_branch_name: find all possible @-marks
  repack: fix typo in max-pack-size option
  repack: make parsed string options const-correct
  repack: propagate pack-objects options as strings

Junio C Hamano (5):
  merge-base: separate --independent codepath into its own helper
  merge-base --octopus: reduce the result from get_octopus_merge_bases()
  revision: mark contents of an uninteresting tree uninteresting
  revision: propagate flag bits from tags to pointees
  Git 1.8.5.5

Ruben Kerkhof (1):
  send-email: /etc/ssl/certs/ directory may not be usable as ca_path

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

2014-02-13 Thread Junio C Hamano
Thomas Rast t...@thomasrast.ch writes:

 Downside: not listing code merged as a goal may not make the project
 as shiny, neither for Git nor for the student.

I'd actually view that as an upside. This sounds like a good first
step for a feasibility study that is really necessary.

I wonder why the handling of storage corruption and replacement
could be left broken, though. Is that because libgit2 has known
breakages in these areas, or is there some other reason?
--
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: turn on network daemon tests by default

2014-02-14 Thread Junio C Hamano
Jeff King p...@peff.net writes:

  - We may want to do something similar in cvsserver and git-gui to
make them more robust.
 
$ git grep -e true --and -e 1 --and -e yes

 I assume the something here is to respect bool options more
 consistently?

Yeah, mostly by employing your 'git -c magic.var=X config --bool'
trick and check only for 'false' and 'true', instead of keeping a
hard-coded logic like the lines that hit the above query do.

 I have no problem with that, but nor do I care too much
 about those programs (that is partially laziness, but also partially
 that I do not want to deal with introducing a regression).

True, too ;-)

  - Do we want to do something similar to GIT_TEST_CREDENTIAL_HELPER?

 No, it is not a boolean. It is a bit of a hack, but it is meant to be
 used like:

   GIT_TEST_CREDENTIAL_HELPER=foo ./t0303-*

 to test some random git-credential-foo you have in your PATH. There is
 nothing to run by default there.

Ah, OK.  I was only grepping for test -z .*GIT_TEST_.
 tri-state is auto (or empty), but report an error when it is

 You probably want to drop this or empty or change it to or unset,

Thanks, I totally missed that.
--
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] notes: Disallow reusing non-blob as a note object

2014-02-14 Thread Junio C Hamano
Eric Sunshine sunsh...@sunshineco.com writes:

 +   if (type != OBJ_BLOB) {
 +   free(buf);
 +   die(_(Cannot read note data from non-blob object '%s'.), 
 arg);

 The way this diagnostic is worded, it sound as if the 'read' failed
 rather than that the user specified an incorrect object type. Perhaps
 Object is not a blob '%s' or Expected blob but '%s' has type '%s'
 or something along those lines?

Yeah, sounds good.  You also need to say what expects a blob, too.
Perhaps something like this?

 builtin/notes.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/builtin/notes.c b/builtin/notes.c
index c11d6e6..a16bc00 100644
--- a/builtin/notes.c
+++ b/builtin/notes.c
@@ -272,8 +272,10 @@ static int parse_reuse_arg(const struct option *opt, const 
char *arg, int unset)
die(_(Failed to read object '%s'.), arg);
}
if (type != OBJ_BLOB) {
+   struct msg_arg *msg = opt-value;
free(buf);
-   die(_(Cannot read note data from non-blob object '%s'.), arg);
+   die(_(The -%c option takes a blob, which '%s' is not.,
+ msg-use_editor ? 'c' : 'C', arg));
}
strbuf_add((msg-buf), buf, len);
free(buf);
--
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] for-each-ref: add option to omit newlines

2014-02-14 Thread Junio C Hamano
Øystein Walle oys...@gmail.com writes:

 However, when specifying a format string it's just a matter of ending
 the format string in '%00' and you're good to go. But then you get the
 null byte *and* a newline. And with your proposal there would be no way
 of saying you want neither.

I very well understand that.  All other commands that support -z
to give you NUL terminated output do not consider that a downside.
Why should for-each-ref be special?
--
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: error: src refspec refs/heads/master matches more than one.

2014-02-14 Thread Junio C Hamano
Duy Nguyen pclo...@gmail.com writes:

 On Fri, Feb 14, 2014 at 7:45 PM, Andreas Schwab sch...@linux-m68k.org wrote:
 Josef Wolf j...@raven.inka.de writes:

 Notice the refs/heads _within_ refs/heads!

 Now I wonder how I managed to get into this situation and what's the best 
 way
 to recover?

 Probably you did something like git branch refs/heads/master.  You can
 remove it again with git branch -d refs/heads/master.

 As a porcelain, git branch should prevent (or at least warn) users
 from creating such refs, I think.

warn, possibly, but I do not see a reason to *prevent*.

 A. You are not allowed to call your branch with a string that begins with
'refs/heads/'.
 B. Why?
 A. Because it will confuse you.
 B. I know what I am doing.
 A. ???


--
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] tree-diff: rework diff_tree() to generate diffs for multiparent cases as well

2014-02-14 Thread Junio C Hamano
Kirill Smelkov k...@mns.spb.ru writes:

  8 
 From: Kirill Smelkov k...@mns.spb.ru
 Subject: [PATCH v2 1/2] tree-diff: rework diff_tree() to generate diffs for
  multiparent cases as well
 MIME-Version: 1.0
 Content-Type: text/plain; charset=UTF-8
 Content-Transfer-Encoding: 8bit

The last three do not belong here.  Only From, Date and Subject are
relevant and taken as in-body headers.

For that matter, as the From and Subject above are exactly the same
as what you have on the e-mail header, you do not want any of them.

I'll strip them out here, so no need to resend.  Thanks.

 Previously diff_tree(), which is now named __diff_tree_sha1(), was

That name with two leading underscores is a rather unfortunate,
especially for a function that is not a file scope static.  No way
to rename this to something more sensible?

 That impedance mismatch *hurts* *performance* *badly* for generating
 combined diffs - in c839f1bd (combine-diff: optimize combine_diff_path

Please avoid referring to a commit that is not in 'master' by its
object name.  It can be reworked later and get a different name.

 That slowness comes from the fact that currently, while generating
 combined diff, a lot of time is spent computing diff(commit,commit^2)
 just to only then intersect that huge diff to almost small set of files
 from diff(commit,commit^1).

Good observation.

|a|   |b|a  b   -  a ∉ B   -   D(A,B) +=  +aa↓
|-|   |-|a  b   -  b ∉ A   -   D(A,B) +=  -bb↓
| |   | |a = b   -  investigate δ(a,b)a↓ b↓

In both the n-parallel and diff-tree, when an entry 'a' is a
tree, I take this D(A,B) += +a to mean (recursively) adding all
the paths within 'a' to the result as addition.  Sounds sensible.

 D(A,B)

 is by definition the same as combined diff

 D(A,[B]),

 so if we could rework the code for common case and make it be not slower
 for nparent=1 case, usual diff(t1,t2) generation will not be slower, and
 multiparent diff tree-walker would greatly benefit generating
 combine-diff.

OK.

 What we do is as follows:

 1) diff tree-walker __diff_tree_sha1() is internally reworked to be
a paths generator (new name diff_tree_paths()), with each generated path
being `struct combine_diff_path` with info for path, new sha1,mode and for
every parent which sha1,mode it was in it.

 2) From that info, we can still generate usual diff queue with
struct diff_filepairs, via exporting generated
combine_diff_path, if we know we run for nparent=1 case.
(see emit_diff() which is now named emit_diff_p0only())

s/p0/first_parent_/; perhaps?

 3) In order for diff_can_quit_early(), which checks

DIFF_OPT_TST(opt, HAS_CHANGES))

to work, that exporting have to be happening not in bulk, but
incrementally, one diff path at a time.

Good thinking.

 Some notes(*):

 1) For loops,

  i = 0; do { ... } while (++i  nparent);

 is used instead of

  for (i = 0; i  nparent; ++i)
  ...

 because for the former case, the compiler have to emit additional
 prologue code which checks for i = nparent case before entering the
 loop.

 As we require nparent must be 0, that additional overhead
 conflicts with the runs not slower for nparent=1 case than before
 goal.

Unfortunate.  I'd rather see us stick to more readable and familiar
form for maintainability if this were not measurable.

 2) alloca(), for small arrays, is used for the same reason - if we change
 it to xmalloc()/free() the timings get worse

Do you see any use of it outside compat/?

I thought we specifically avoid alloca() for portability.  Also we
do not use variable-length-arrays on the stack either, I think.

 3) For every parent tree, we need to keep a tag, whether entry from that
 parent equals to entry from minimal parent. For performance reasons I'm
 keeping that tag in entry's mode field in unused bit - see S_IFXMIN_NEQ.

Unfortunate, but I do not see another place to keep this
information offhand (nor implement this approach without keeping
that piece of information).

 P.S. and combined diff is not some exotic/for-play-only stuff - for

No need to convince us about that ;-)

 example for a program I write to represent Git archives as readonly
 filesystem, there is initial scan with

 `git log --reverse --raw --no-abbrev --no-renames -c`

 to extract log of what was created/changed when, as a result building a
 map

 {}  sha1-  in which commit (and date) a content was added

 that `-c` means also show combined diff for merges, and without them, if
 a merge is non-trivial (merges changes from two parents with both having
 separate changes to a file), or an evil one, the map will not be full,
 i.e. some valid sha1 would be absent from it.

 That case was my initial motivation for combined diffs speedup.

I wonder if this machinery can be reused for log -m as well (or
perhaps you do that already?).  After all, by performing a single
parallel scan, you are gathering all the necessary 

Re: [PATCH] config: git_config_from_file(): handle - filename as stdin

2014-02-14 Thread Junio C Hamano
Kirill A. Shutemov kir...@shutemov.name writes:

 The patch extends git config --file interface to allow read config from
 stdin.

Thanks.  The external interface proposed by this change that behaves
the way your new test expects is a good addition to the system.  I
would describe it as:

  Subject: config: teach git config --file - to read from the standard input

I however think the patch implements it at the level that is too low
in the callchain.  It will affect a lot more than the dash given to
git config --file -.  Fortunately, it does not make it possible
for users to make this mistake

[include]
path = -

and scratch their heads, wondering why git config is not answering
until they hit ^D.  But that is _only_ because we check if a file
whose name is - actually exists in the current directory before
falling into this codepath (and usually no such file exists).  If
such a funnily-named file does exist, we read from that file, not
the standard input.  So that include codepath happens to be safe,
but who knows what dragons lie in other codepaths that call this
function.

I recall that an earlier implementation of git diff --no-index
that made - read one side to be compared from the standard input
had exactly the same issue of comparing filename with -, which we
had to fix with code reorganization recently.  I'd prefer to see
this update to git config --file - done the right way from the
start.

 diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
 index 967359344dab..f1a63075e34f 100755
 --- a/t/t1300-repo-config.sh
 +++ b/t/t1300-repo-config.sh
 @@ -484,6 +484,10 @@ test_expect_success 'alternative GIT_CONFIG (--file)' '
   test_cmp expect output
  '
  
 +test_expect_success 'alternative GIT_CONFIG (--file=-)' '
 + git config --file - -l  other-config  output 

Please leave no SP between redirection operator and its file, i.e.

git config --file - --list other-config output

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: error: src refspec refs/heads/master matches more than one.

2014-02-14 Thread Junio C Hamano
David Kastrup d...@gnu.org writes:

 Junio C Hamano gits...@pobox.com writes:
 ...
  A. Because it will confuse you.
  B. I know what I am doing.
  A. ???

 A. But maybe Git will no longer know what you are doing.  Its standard
 way of resolving references will mean that once a branch
 refs/heads/wibble exists, referring to a branch wibble will become extra
 hard.  For example, stuff like

 push origin HEAD:refs/heads/wibble

 will maybe create or update a new branch wibble, or maybe it will just
 push to the existing branch refs/heads/wibble.

I suspect that is a bad example (do refs on _our_ side influence how
RHS of the colon that names refs on the other side is interpreted),
but I think I know what you are trying to say.  But Git never knows
what you are doing---it just does what you tell it to, so it comes
back to It will confuse you to the point that you would not be able
to guess what Git will do matches your expectation.  Whenever I
tell them that, a counterpoint B. makes is still I know what I am
doing., which is stubborn, rather obvious, and unfortunate.

How much of the namespace are we willing to carve out if we were to
go this route anyway?  Things we use, i.e. refs/heads and refs/tags,
would be no-brainers, but do we also forbid refs/review, which we
ourselves do not use but some people may have in their repositories?
Anything that begins with refs/?  Anything that begins with refs/
and has more than two slashes in it?

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


Re: What's cooking in git.git (Feb 2014, #04; Wed, 12)

2014-02-14 Thread Junio C Hamano
brian m. carlson sand...@crustytoothpaste.net writes:

  Changes to some scripted Porcelains use unsafe variable
  substitutions and still need to be tightened.
 
  Will merge to 'next'.

 Junio, did you want a reroll with that fixed commit message, or will you
 fix it up yourself?

I haven't merged them yet---if there are need to update any one of
them, please reroll a replacement set.

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: diff weirdness (bug?)

2014-02-14 Thread Junio C Hamano
Dario Bertini berda...@gmail.com writes:

 git clone g...@github.com:ansible/ansible.git
 git revert 3616dffb68badb2b8d56

 manually solve the conflict (you can look at the commit here:
 https://github.com/ansible/ansible/commit/3616dffb68badb2b8d56ef34391d7aae8de79cd6
 )

 git diff will output:

 dario@macbook ~/P/ansible (devel*+|REVERTING) git diff
 diff --cc lib/ansible/constants.py
 index c055ccf,6eac602..000
 --- a/lib/ansible/constants.py
 +++ b/lib/ansible/constants.py
 @@@ -84,16 -61,8 +84,12 @@@ active_user   = pwd.getpwuid(os.geteuid

   # Needed so the RPM can call setup.py and have modules land in the
   # correct location. See #1277 for discussion
 - if getattr(sys, real_prefix, None):
 - # in a virtualenv
 - DIST_MODULE_PATH = os.path.join(sys.prefix, 'share/ansible/')
 - else:
 - DIST_MODULE_PATH = '/usr/share/ansible/'
 + DIST_MODULE_PATH = os.path.join(sys.prefix, 'share/ansible/')

  +# check all of these extensions when looking for yaml files for things
 like
  +# group variables
  +YAML_FILENAME_EXTENSIONS = [ , .yml, .yaml ]
  +
   # sections in config file
   DEFAULTS='defaults'



 now, it weirdly/incorrectly says that we added the YAML-related lines

This is a combined diff, and yaml-related lines are added relative
to your _other_ branch you are merging (notice these + are indented
by one place).  Relative to what you had at the tip of your branch
before you started this operation that ended up conflicted, the
half-merged result removes if/else that sets DIST_MODULE_PATH and
replaces it with a single line (their +/- are on the first column,
signifying that these are differences relative to the first parent,
i.e. your state before you started the operation).

 if we remove these 3 lines, we'll get this diff:

With that understanding, I think the output after removing these
three lines is perfectlyh understandable and correct.  You are
looking at the three lines that used to exist in the version you
started from, that were missing from the other side.  If you remoe
them, it will show as removal from _your_ version (notice these -
that shows what _you_ did manually are on the first column, saying
that that is relative to _your_ version).
--
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 cooking in git.git (Feb 2014, #04; Wed, 12)

2014-02-14 Thread Junio C Hamano
Andrew Eikum aei...@codeweavers.com writes:

 On Wed, Feb 12, 2014 at 01:59:41PM -0800, Junio C Hamano wrote:
 As a workaround to make life easier for third-party tools, the
 upcoming major release will be called Git 1.9.0 (not Git 1.9).
 The first maintenance release for it will be Git 1.9.1, and the
 major release after Git 1.9.0 will either be Git 2.0.0 or Git
 1.10.0.
 

 Apologies if this ground has been tread before, but has there been a
 version numbering discussion? A quick google didn't seem to turn
 anything up.

 This seems to be an opportune time to drop the useless first digit.
 Explicitly, the major release numbers would be: 1.8, 1.9, then 2.0,
 3.0, 4.0, etc, with the 2nd digit would take the meaning of the
 current 3rd digit and so on.

Considered, and discarded.

cf. http://article.gmane.org/gmane.comp.version-control.git/241498

When you see a version number vX.Y.0 next time, think of it as just
play vX.Y without the third digit, and you will be fine.  People's
script cannot learn the think of it as ... part overnight, and
that is why we have the .0 there.
--
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] config: git_config_from_file(): handle - filename as stdin

2014-02-14 Thread Junio C Hamano
Kirill A. Shutemov kir...@shutemov.name writes:

 On Fri, Feb 14, 2014 at 10:27:11AM -0800, Junio C Hamano wrote:
 ...
 I recall that an earlier implementation of git diff --no-index
 that made - read one side to be compared from the standard input
 had exactly the same issue of comparing filename with -, which we
 had to fix with code reorganization recently.  I'd prefer to see
 this update to git config --file - done the right way from the
 start.

 Okay, reworked version is below. It's slightly more invasive then the
 original.

Thanks.

It looks more invasive mostly because you renamed check_blob_write()
to check_write().  While I think that rename is a right thing to do
(it used to be if we are attempting to write to blob, signal an
error, but we could have called it check_write(), meaning we are
attempting to write; error out if that should not be allowed), it
would have been much easier to review and comment if this were done
as a separate clean-up preparatory patch.  It wouldn't have had to
touch this many lines, I would think.

And clean-up existing code as a separate step, without changing the
behaviour, before starting to work on a new feature is actively
encouraged around here.

 From 199e6a995bb5228578e66189ef586421a4d8d9ba Mon Sep 17 00:00:00 2001
 From: Kirill A. Shutemov kir...@shutemov.name
 Date: Fri, 14 Feb 2014 21:59:39 +0200
 Subject: [PATCH] config: teach git config --file - to read from the standard
  input

I do not see a need for these four lines in your e-mail.  All the
necessary information is already in your e-mail header.  Please drop
them, perhaps except for the Subject: in-body header.

If you are going to have a discussion and then your proposed patch,
please do it this way (without the 8-space indentation I added for
illustration) using the -- 8 -- scissors line:

...
Okay, reworked version is below.

-- 8 --
Subject: config: teach git config --file - to read from the standard 
input

Extend git config --file interface to allow reading from
the standard input

Editing or setting value is an error.

Signed-off-by: ...
---
 diffstat and patch here

The in-body header Subject:  is there to let you retitle the
commit.

 Editing stdin or setting value in stdin is an error.

Good thinking.  Even comes with tests to cover these cases.  Good.

 diff --git a/builtin/config.c b/builtin/config.c
 index 92ebf23f0a9a..625f914c44a0 100644
 --- a/builtin/config.c
 +++ b/builtin/config.c
 @@ -21,6 +21,7 @@ static char key_delim = ' ';
  static char term = '\n';
  
  static int use_global_config, use_system_config, use_local_config;
 +static int use_stdin;
  static const char *given_config_file;
  static const char *given_config_blob;

If we are changing the function signature of git_config_with_options()
anyway, would it be even a better idea to introduce something like:

struct config_source {
int use_stdin;
const char *config_file;
const char *config_blob;
};

static struct config_source given_config_source;

and have one _fewer_ parameter to the function, instead of adding
one extra parameter?

 diff --git a/config.c b/config.c
 index d969a5aefc2b..53dd39f0b9ef 100644
 --- a/config.c
 +++ b/config.c
 @@ -1030,24 +1030,34 @@ static int do_config_from(struct config_source *top, 
 config_fn_t fn, void *data)
   return ret;
  }
  
 -int git_config_from_file(config_fn_t fn, const char *filename, void *data)
 +static int do_config_from_file(config_fn_t fn, const char *filename, FILE *f,
 +void *data)
  {
 - int ret;
 - FILE *f = fopen(filename, r);
 + struct config_source top;
  
 - ret = -1;
 - if (f) {
 - struct config_source top;
 + top.u.file = f;
 + top.name = filename;
 + top.die_on_error = 1;
 + top.do_fgetc = config_file_fgetc;
 + top.do_ungetc = config_file_ungetc;
 + top.do_ftell = config_file_ftell;
 +
 + return do_config_from(top, fn, data);
 +}
  
 - top.u.file = f;
 - top.name = filename;
 - top.die_on_error = 1;
 - top.do_fgetc = config_file_fgetc;
 - top.do_ungetc = config_file_ungetc;
 - top.do_ftell = config_file_ftell;
 +static int git_config_from_stdin(config_fn_t fn, void *data)
 +{
 + return do_config_from_file(fn, stdin, stdin, data);
 +}

So the above callchain will set top.name to the string stdin.
How would that interact with the code in handle_path_include() that
makes sure that relative config includes are only allowed in file
with known location?

Other than that, I didn't spot any issue in this round looks.  It
seems to be almost there.

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 cooking in git.git (Feb 2014, #04; Wed, 12)

2014-02-14 Thread Junio C Hamano
Andrew Eikum aei...@codeweavers.com writes:

 My worry is having 2. hang around for another decade or longer. I'd
 rather see X.0.0 denote a major feature release (currently represented
 as 1.X.0), with X.Y.0 for minor enhancements and X.Y.Z for bugfix.

We need three categories: (1) potentially incompatible, (2) feature,
(3) fixes-only.  We have been doing two levels of features by having
both second and third numbers and we are flattening by removing the
second one.

 It seems reasonable to expect fewer backwards incompatible changes in
 the future as Git has become more mature. This reduces the utility of
 reserving X.0.0 for major backwards incompatible changes, especially
 considering it's already been eight years for the first increment.

We are not done yet, far from it.  If we can stay at 2.X longer,
that is a very good thing.

If we followed your numbering scheme, you rob from the users a way
to learn about a rare event, a potentially backward-incompatible
change.  How would you tell your users when the version gap really
matters?  After hearing You need to plan carefully when you update
to version 47 and then updating to version 47 (or the user may skip
that version), the user will learn about a new version 48 and does
not hear such a you need to be careful.  What should he think?  No
news is a good news?  He should refrain from updating because the
last one was a big one?  What if the last time he updated was to
version 43, stayed at that version for a long time without paying
much attention (as Git grows more and more mature), and now we have
version 50 after having a large compatibility gap at version 47 he
did not pay much attention because he was skipping?

The rarer the important event is, the more necessary that the
importance is communicated clearly.

--
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 02/14] trailer: process trailers from file and arguments

2014-02-14 Thread Junio C Hamano
Christian Couder chrisc...@tuxfamily.org writes:

 For example some people might want:

 if_exists = overwrite
 if_missing = add

 while others might want:

 if_exists = overwrite
 if_missing = do_nothing

 and I don't see how we can say that with just:

 action = do_Y_if_X_and_Z

Yes, but then we go back to my original question: why exists and
missing are so special, and why there aren't two kinds of exists
(i.e. there exists an entry with the same key, value vs there
exists an entry with the same key).  I would have understood your
this is not too hard to understand for users if you had three
(i.e. missing, in addition to these two flavours of exists), but
with only two, I do not see how it is useful in a hypothetical
situation like above.

For example, how would you express something like this only with
if-exists vs if-missing?

if_exists_exactly = ignore
if_exists_with_different_value = append
if_missng = prepend_to_the_beginning
--
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


[ANNOUNCE] Git v1.9.0

2014-02-14 Thread Junio C Hamano
The latest feature release Git v1.9.0 is now available at the
usual places.

The release tarballs are found at:

http://code.google.com/p/git-core/downloads/list

and their SHA-1 checksums are:

e60667fc16e5a5f1cde46616b0458cc802707743  git-1.9.0.tar.gz
65eb3f411f4699695c7081a7c716cabb9ce23d75  git-htmldocs-1.9.0.tar.gz
cff590c92b4d1c8a143c078473140b653cc5d56a  git-manpages-1.9.0.tar.gz

The following public repositories all have a copy of the v1.9.0
tag and the master branch that the tag points at:

  url = https://kernel.googlesource.com/pub/scm/git/git
  url = git://repo.or.cz/alt-git.git
  url = https://code.google.com/p/git-core/
  url = git://git.sourceforge.jp/gitroot/git-core/git.git
  url = git://git-core.git.sourceforge.net/gitroot/git-core/git-core
  url = https://github.com/gitster/git

Also, http://www.kernel.org/pub/software/scm/git/ has copies of the
release tarballs.

Git v1.9.0 Release Notes


Backward compatibility notes


git submodule foreach $cmd $args used to treat $cmd $args the same
way ssh did, concatenating them into a single string and letting the
shell unquote. Careless users who forget to sufficiently quote $args
get their argument split at $IFS whitespaces by the shell, and got
unexpected results due to this. Starting from this release, the
command line is passed directly to the shell, if it has an argument.

Read-only support for experimental loose-object format, in which users
could optionally choose to write their loose objects for a short
while between v1.4.3 and v1.5.3 era, has been dropped.

The meanings of the --tags option to git fetch has changed; the
command fetches tags _in addition to_ what is fetched by the same
command line without the option.

The way git push $there $what interprets the $what part given on the
command line, when it does not have a colon that explicitly tells us
what ref at the $there repository is to be updated, has been enhanced.

A handful of ancient commands that have long been deprecated are
finally gone (repo-config, tar-tree, lost-found, and peek-remote).


Backward compatibility notes (for Git 2.0.0)


When git push [$there] does not say what to push, we have used the
traditional matching semantics so far (all your branches were sent
to the remote as long as there already are branches of the same name
over there).  In Git 2.0, the default will change to the simple
semantics, which pushes:

 - only the current branch to the branch with the same name, and only
   when the current branch is set to integrate with that remote
   branch, if you are pushing to the same remote as you fetch from; or

 - only the current branch to the branch with the same name, if you
   are pushing to a remote that is not where you usually fetch from.

Use the user preference configuration variable push.default to
change this.  If you are an old-timer who is used to the matching
semantics, you can set the variable to matching to keep the
traditional behaviour.  If you want to live in the future early, you
can set it to simple today without waiting for Git 2.0.

When git add -u (and git add -A) is run inside a subdirectory and
does not specify which paths to add on the command line, it
will operate on the entire tree in Git 2.0 for consistency
with git commit -a and other commands.  There will be no
mechanism to make plain git add -u behave like git add -u ..
Current users of git add -u (without a pathspec) should start
training their fingers to explicitly say git add -u .
before Git 2.0 comes.  A warning is issued when these commands are
run without a pathspec and when you have local changes outside the
current directory, because the behaviour in Git 2.0 will be different
from today's version in such a situation.

In Git 2.0, git add path will behave as git add -A path, so
that git add dir/ will notice paths you removed from the directory
and record the removal.  Versions before Git 2.0, including this
release, will keep ignoring removals, but the users who rely on this
behaviour are encouraged to start using git add --ignore-removal path
now before 2.0 is released.

The default prefix for git svn will change in Git 2.0.  For a long
time, git svn created its remote-tracking branches directly under
refs/remotes, but it will place them under refs/remotes/origin/ unless
it is told otherwise with its --prefix option.


Updates since v1.8.5


Foreign interfaces, subsystems and ports.

 * The HTTP transport, when talking GSS-Negotiate, uses 100
   Continue response to avoid having to rewind and resend a large
   payload, which may not be always doable.

 * Various bugfixes to remote-bzr and remote-hg (in contrib/).

 * The build procedure is aware of MirBSD now.

 * Various git p4, git svn and gitk updates.


UI, Workflows  Features

 * Fetching from a shallowly-cloned repository used to be forbidden,
   primarily because the codepaths involved were 

What's cooking in git.git (Feb 2014, #05; Fri, 14)

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

The tip of 'master' is v1.9.0.  The first maintenance release for it
will be Git 1.9.1, and the major release after Git 1.9.0 will
either be Git 2.0.0 or Git 1.10.0.

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

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

--
[New Topics]

* ks/tree-diff-nway (2014-02-14) 2 commits
 - combine-diff: speed it up, by using multiparent diff
 - tree-diff: rework diff_tree() to generate diffs for multiparent cases as well
 (this branch uses ks/combine-diff, ks/tree-diff-more and ks/tree-diff-walk.)

 Instead of running N pair-wise diff-trees when inspecting a
 N-parent merge, find the set of paths that were touched by walking
 N+1 trees in parallel.  These set of paths can then be turned into
 N pair-wise diff-tree results to be processed through rename
 detections and such.  And N=2 case nicely degenerates to the usual
 2-way diff-tree, which is very nice.

 Promising, but unfortunately the implementation seems a bit too
 unportable for such a core part of the system.

--
[Stalled]

* po/everyday-doc (2014-01-27) 1 commit
 - Make 'git help everyday' work

 This may make the said command to emit something, but the source is
 not meant to be formatted into a manual pages to begin with, and
 also its contents are a bit stale.  It may be a good first step in
 the right direction, but needs more work to at least get the
 mark-up right before public consumption.

 Will hold.


* jk/branch-at-publish-rebased (2014-01-17) 5 commits
 - t1507 (rev-parse-upstream): fix typo in test title
 - implement @{publish} shorthand
 - branch_get: provide per-branch pushremote pointers
 - branch_get: return early on error
 - sha1_name: refactor upstream_mark

 Give an easier access to the tracking branches from other side in
 a triangular workflow by introducing B@{publish} that works in a
 similar way to how B@{upstream} does.

 Meant to be used as a basis for whatever Ram wants to build on.

 Will hold.


* rb/merge-prepare-commit-msg-hook (2014-01-10) 4 commits
 - merge: drop unused arg from abort_commit method signature
 - merge: make prepare_to_commit responsible for write_merge_state
 - t7505: ensure cleanup after hook blocks merge
 - t7505: add missing 

 Expose more merge states (e.g. $GIT_DIR/MERGE_MODE) to hooks that
 run during git merge.  The log message stresses too much on one
 hook, prepare-commit-msg, but it would equally apply to other hooks
 like post-merge, I think.

 Waiting for a reroll.


* jl/submodule-recursive-checkout (2013-12-26) 5 commits
 - Teach checkout to recursively checkout submodules
 - submodule: teach unpack_trees() to update submodules
 - submodule: teach unpack_trees() to repopulate submodules
 - submodule: teach unpack_trees() to remove submodule contents
 - submodule: prepare for recursive checkout of submodules

 Expecting a reroll.


* jc/graph-post-root-gap (2013-12-30) 3 commits
 - WIP: document what we want at the end
 - graph: remove unused code a bit
 - graph: stuff the current commit into graph-columns[]

 This was primarily a RFH ($gmane/239580).


* fc/transport-helper-fixes (2013-12-09) 6 commits
 - remote-bzr: support the new 'force' option
 - test-hg.sh: tests are now expected to pass
 - transport-helper: check for 'forced update' message
 - transport-helper: add 'force' to 'export' helpers
 - transport-helper: don't update refs in dry-run
 - transport-helper: mismerge fix

 Updates transport-helper, fast-import and fast-export to allow the
 ref mapping and ref deletion in a way similar to the natively
 supported transports.

 Reported to break t5541, and has been stalled for a while without
 fixes.

 Will discard.


* fc/completion (2013-12-09) 1 commit
 - completion: fix completion of certain aliases

 SZEDER Gábor noticed that this breaks git -c var=val alias and
 also suggested a better description of the change.

 Has been stalled for a while without much comments from anybody
 interested.

 Will discard.


* mo/subtree-split-updates (2013-12-10) 3 commits
 - subtree: add --edit option
 - subtree: allow --squash and --message with push
 - subtree: support split --rejoin --squash

 Has been stalled for a while without much comments from anybody
 interested.

 Will discard.


* hv/submodule-ignore-fix (2013-12-06) 4 commits
 - disable complete ignorance of submodules for index - HEAD diff
 - always show committed submodules in summary after commit
 - teach add -f option for ignored submodules
 - fix 'git add' to skip submodules configured as ignored

 Teach git add to be consistent with git status when changes to
 submodules are set to be ignored, to avoid surprises after checking
 with git status to see there isn't any 

Re: [PATCH] for-each-ref: add option to omit newlines

2014-02-14 Thread Junio C Hamano
Øystein Walle oys...@gmail.com writes:

 Maybe it's all subjective... I'm okay with just leaving things as they
 are.

Lack of -z in for-each-ref can be called an inconsistency that
already exists you may want to fix in any case.

As an extension to that, I would not be fundamentally against a new
option, e.g. --terminiator=7, that causes us to use putchar(7)
instead of putchar('\n') or putchar('\0') to terminate each records.
At that point, -z becomes a synonym for --terminator=0.

And --terminator='' might even be a natural extension to that
option to cause us not to call any putchar() there.

If we were to do that, we should do them for all commands that let
you use -z, not just for-each-ref, for consistency reasons, I
would think.




--
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 02/14] trailer: process trailers from file and arguments

2014-02-14 Thread Junio C Hamano
 For example, how would you express something like this only with
 if-exists vs if-missing?

   if_exists_exactly = ignore
 if_exists_with_different_value = append
 if_missng = prepend_to_the_beginning

 First, previously in the discussion you said that you didn't want us
 to talk about the where = (after | before) part, because you could
 see that it was orthogonal to the other stuff, but now it looks like
 you want again to put that on the table.

Oh, then replace both append and prepend with append (it was a mistake).
Can you express that without having two kinds of if-exists?

 But it could be possible with only if-exists vs
 if-missing like this:

 if_exists = append_if_different
 if_missing = prepend
 ...
 because we can still easily express things like:

 if_exists = append_if_different_neighbor

The proliferation of these random if_X on the action part _is_ exactly
what I find the proposal confusing.
--
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 02/14] trailer: process trailers from file and arguments

2014-02-14 Thread Junio C Hamano
Christian Couder chrisc...@tuxfamily.org writes:

 but we also want to say:

 action = do_Y_if_X_and_Z AND do_U_if_V

 For example some people might want:

 if_exists = overwrite
 if_missing = add

 while others might want:

 if_exists = overwrite
 if_missing = do_nothing

 and I don't see how we can say that with just:

 action = do_Y_if_X_and_Z

That is a very relevant illustration that makes me realize why I
found your if-exists/if-missing = do-Y-if-Z somewhat distasteful.

Your

 if_exists = add_if_different

says if the same key is there, add it if the value is different,
but it also implicitly says donothing if the value is the same.

That is, you are saying with the above

 if_exists = add_if_different AND ignore_if_same

So you already have to support more than one actions depending on
the condition, unless you want to limit the actions for all the
cases other than X to be only ignore when you invent your next
do_Y_if_X, X being different in this case, but you support (and
need to support) different-neighbour and other random collections
of conditions, I think.  Which is essentially the same as saying
that you need this:

 action = do_Y_if_X_and_Z AND do_U_if_V

Again, unless all the U's are limited to ignore, that is.

--
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 02/14] trailer: process trailers from file and arguments

2014-02-14 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 That is, you are saying with the above

  if_exists = add_if_different AND ignore_if_same

 So you already have to support more than one actions depending on
 the condition, ...
 of conditions, I think.  Which is essentially the same as saying
 that you need this:

 action = do_Y_if_X_and_Z AND do_U_if_V

 Again, unless all the U's are limited to ignore, that is.

Oh by the way, don't get me wrong.  I am not saying that the last
one is necessarily better or worse.  I am only saying that the
semantics proposed seems to be hard to explain and we need to do
find a way to do better.

If you have these existing ones:

Sob: A
Sob: B
Sob: C
Sob: D

and you have Sob: B at hand, Sob.if-missing would not fire
(because if-exists/if-missing is only about keys) ans
Sob.if-exists will.  What happens is now up to the action part
(i.e. what follows if_exists =, e.g. add_if_different).

The conditional part of add_if_different action is explainable as
a conditon on the value (as opposed to condition on keys, which is
the left-hand-side).  But what does a condition with neighbour in
its name really mean?  It is not a condition about the value, but
also is a condition about the order of the existing records.

What is the right mental model the end-user needs to form when
understanding these?  Conditions on keys go on the left, and any
other random conditions can come as a modifier after action
e.g. add_if_same_value_is_not_at_the_end?
--
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: Make the git codebase thread-safe

2014-02-18 Thread Junio C Hamano
Duy Nguyen pclo...@gmail.com writes:

 On Sat, Feb 15, 2014 at 8:15 AM, Zachary Turner ztur...@chromium.org wrote:
 ...
 2) Use TLS as you suggest and have one fd per pack thread.  Probably
 the most complicated code change (at least for me, being a first-time
 contributor)

 It's not so complicated. I suggested a patch [1] before (surprise!).
 ...
 [1] http://article.gmane.org/gmane.comp.version-control.git/196042

That message is at the tail end of the discussion. I wonder why
nothing came out of it back then.

While I do not see anything glaringly wrong with the change from a
quick glance over it, it would be nice to hear how well it performs
on their platform from Windows folks.

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: error: src refspec refs/heads/master matches more than one.

2014-02-18 Thread Junio C Hamano
Duy Nguyen pclo...@gmail.com writes:

 Prevent is a strong word. I meant we only do it if they force
 it. Something like this..

 -- 8 --
 diff --git a/branch.c b/branch.c
 index 723a36b..3f0540f 100644
 --- a/branch.c
 +++ b/branch.c
 @@ -251,6 +251,11 @@ void create_branch(const char *head,
   forcing = 1;
   }
  
 + if (!force  dwim_ref(name, strlen(name), sha1, real_ref))
 + die(_(creating ref refs/heads/%s makes %s ambiguous.\n
 +   Use -f to create it anyway.),
 + name, name);

Does this check still allow you to create a branch refs/heads/next
and then later create a branch next?  The latter will introduce an
ambiguity without any prevention, even though the prevention would
trigger if the order in which these two branches are created is
swapped--- the end result has ambiguity but the safety covers only
one avenue to the confusing situation.

And the only way I can think of to avoid that kind of confusion is
to forbid creation of a subset of possible names by reserving a set
of known (but arbitrary) prefixes---which I am not sure is a good
way to go.  At least not yet.

So...
--
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: diff weirdness (bug?)

2014-02-18 Thread Junio C Hamano
Dario Bertini berda...@gmail.com writes:

 On 02/14/2014 09:03 PM, Junio C Hamano wrote:
 This is a combined diff, and yaml-related lines are added relative
 to your _other_ branch you are merging (notice these + are indented
 by one place).  Relative to what you had at the tip of your branch
 before you started this operation that ended up conflicted, the
 half-merged result removes if/else that sets DIST_MODULE_PATH and
 replaces it with a single line (their +/- are on the first column,
 signifying that these are differences relative to the first parent,
 i.e. your state before you started the operation).
 
 if we remove these 3 lines, we'll get this diff:
 
 With that understanding, I think the output after removing these
 three lines is perfectlyh understandable and correct.  You are
 looking at the three lines that used to exist in the version you
 started from, that were missing from the other side.  If you remoe
 them, it will show as removal from _your_ version (notice these -
 that shows what _you_ did manually are on the first column, saying
 that that is relative to _your_ version).
 

 Thank you, I was completely unaware of combined diffs. Still: I can't
 see how this would explain the empty diff when deleting 4 lines instead
 of 3.

 Also: in the diff output I get 2 hashes, but these are not the hashes of
 the commits, but the contents of the files apparently. One should be
 HEAD (but if I run sha1sum over the file the hash doesn't match), but

A blob object name (or for that matter, names of any type of object)
is not the same as the hash over its contents alone.

See combined diff format section of git diff --help if you are
interested in reading what the output format is telling you.

 the other can't be the commit which I reverted: the diff is too small...
 or at least this is what I understand

 By the way, in the man of git diff there's the briefly mentioned '-m'
 flag. If anyone else reading this mail in the archives is confused by
 the combined diff output, just use git diff -m HEAD... I'll probably
 add this in my git aliases

If you are primarily interested in what a merge (or other
mergy-operation like revert) did to your working tree state,
relative to the state it operated on, git diff HEAD is most likely
what you want.
--
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] config: teach git config --file - to read from the standard input

2014-02-18 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 +} else {
 +if (cf-name)
 +return error(bad config file line %d in %s,
 +cf-linenr, cf-name);
 +else
 +return error(bad config file line %d, cf-linenr);
 +}

 I think I preferred the earlier version where you had stdin in the
 name field, and this hunk could just go away. I know you switched it to
 NULL here to avoid making bogus relative filenames in includes.

Exactly the same comment here.  I really like the way how this
series becomes more polished every time we see it.

 But that would be pretty straightforward to fix by splitting the name
 field into an additional path field. It looks like git config --blob
 has the same problem (it will try relative lookups in the filesystem,
 even though that is nonsensical from the blob). So we could fix the
 existing bug at the same time. :)

 Perhaps this could go at the start of your series?

Sounds like the right thing to do.

Thanks.

 -- 8 --
 Subject: config: disallow relative include paths from blobs

 When we see a relative config include like:

   [include]
   path = foo

 we make it relative to the containing directory of the file
 that contains the snippet. This makes no sense for config
 read from a blob, as it is not on the filesystem.  Something
 like HEAD:some/path could have a relative path within the
 tree, but:

   1. It would not be part of include.path, which explicitly
  refers to the filesystem.

   2. It would need different parsing rules anyway to
  determine that it is a tree path.

 The current code just uses the name field, which is wrong.
 Let's split that into name and path fields, use the
 latter for relative includes, and fill in only the former
 for blobs.

 Signed-off-by: Jeff King p...@peff.net
 ---
 I don't think we considered includes at all when adding --blob. I cannot
 think of any good reason to have even an absolute filesystem include
 from a blob. And I wonder if it is possible to cause security mischief
 by convincing git to look at /etc/passwd or similar (it would not parse,
 of course, but you might be able to glean information from the error
 messages or something).

 It's probably OK, though, because you would generally not read real
 config from an untrusted source (there are many bad things they could
 set), and other code which uses the config reader explicitly does not
 turn on includes.

  config.c  | 10 ++
  t/t1305-config-include.sh | 16 
  2 files changed, 22 insertions(+), 4 deletions(-)

 diff --git a/config.c b/config.c
 index d969a5a..b295310 100644
 --- a/config.c
 +++ b/config.c
 @@ -21,6 +21,7 @@ struct config_source {
   } buf;
   } u;
   const char *name;
 + const char *path;
   int die_on_error;
   int linenr;
   int eof;
 @@ -97,12 +98,12 @@ static int handle_path_include(const char *path, struct 
 config_include_data *inc
   if (!is_absolute_path(path)) {
   char *slash;
  
 - if (!cf || !cf-name)
 + if (!cf || !cf-path)
   return error(relative config includes must come from 
 files);
  
 - slash = find_last_dir_sep(cf-name);
 + slash = find_last_dir_sep(cf-path);
   if (slash)
 - strbuf_add(buf, cf-name, slash - cf-name + 1);
 + strbuf_add(buf, cf-path, slash - cf-path + 1);
   strbuf_addstr(buf, path);
   path = buf.buf;
   }
 @@ -1040,7 +1041,7 @@ int git_config_from_file(config_fn_t fn, const char 
 *filename, void *data)
   struct config_source top;
  
   top.u.file = f;
 - top.name = filename;
 + top.name = top.path = filename;
   top.die_on_error = 1;
   top.do_fgetc = config_file_fgetc;
   top.do_ungetc = config_file_ungetc;
 @@ -1062,6 +1063,7 @@ int git_config_from_buf(config_fn_t fn, const char 
 *name, const char *buf,
   top.u.buf.len = len;
   top.u.buf.pos = 0;
   top.name = name;
 + top.path = NULL;
   top.die_on_error = 0;
   top.do_fgetc = config_buf_fgetc;
   top.do_ungetc = config_buf_ungetc;
 diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
 index a707076..6edd38b 100755
 --- a/t/t1305-config-include.sh
 +++ b/t/t1305-config-include.sh
 @@ -122,6 +122,22 @@ test_expect_success 'relative includes from command line 
 fail' '
   test_must_fail git -c include.path=one config test.one
  '
  
 +test_expect_success 'absolute includes from blobs work' '
 + echo [test]one = 1 one 
 + echo [include]path=$(pwd)/one blob 
 + blob=$(git hash-object -w blob) 
 + echo 1 expect 
 + git config --blob=$blob test.one actual 
 + test_cmp expect actual
 +'
 +
 +test_expect_success 'relative includes from blobs fail' '
 + echo [test]one = 1 one 
 + 

Re: [PATCH 0/4] Good bye fnmatch

2014-02-18 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 Long story short, we wanted globbing wildcard ** so I ripped
 wildmatch library from rsync to do it. And it opened a possibility
 to replace fnmatch completely, which would provide consistent behavior
 across platforms (native fnmatch behaves differently on many corner
 cases), and some performance gains. I started fnmatch replacement with
 4917e1e (Makefile: promote wildmatch to be the default fnmatch
 implementation - 2013-05-30). This is the final step.

Nice.


 Nguyễn Thái Ngọc Duy (4):
   Use wildmatch() directly without fnmatch() wrapper
   Revert test-wildmatch: add perf command to compare wildmatch and fnmatch
   Stop using fnmatch (either native or compat)
   Actually remove compat fnmatch source code

  Makefile|  22 --
  builtin/apply.c |   2 +-
  builtin/branch.c|   2 +-
  builtin/describe.c  |   2 +-
  builtin/for-each-ref.c  |   2 +-
  builtin/ls-remote.c |   2 +-
  builtin/name-rev.c  |   2 +-
  builtin/reflog.c|   2 +-
  builtin/replace.c   |   2 +-
  builtin/show-branch.c   |   2 +-
  builtin/tag.c   |   2 +-
  compat/fnmatch/fnmatch.c (gone) | 494 
 
  compat/fnmatch/fnmatch.h (gone) |  84 ---
  config.mak.uname|  10 -
  configure.ac|  28 ---
  diffcore-order.c|   2 +-
  dir.c   |  11 +-
  git-compat-util.h   |  12 -
  refs.c  |   2 +-
  revision.c  |   2 +-
  t/t3070-wildmatch.sh|  13 --
  test-wildmatch.c|  79 ---
  22 files changed, 20 insertions(+), 759 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


Re: [PATCH/RESEND] attr: allow pattern escape using backslashes

2014-02-18 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 Patterns in .gitattributes are separated by whitespaces, which makes
 it impossible to specify exact spaces in the pattern. '?' can be used
 as a workaround, but it matches other characters too. This patch makes
 a space following a backslash part of the pattern, not a pattern
 separator.

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
  Last discussion is [1] although the thread went off topic, so no
  actual discussion.

The only people who could get hurt with this patch are those who do
have a path that ends with a backslash (e.g. 'hello\') and have been
matching it with a pattern that literally match with it, but even
then they should have been quoting it at the end of the pattern
(e.g. as 'hello\\'), so the new parsing rule would not be confused,
I would think.

So, I like it.  Thanks.

  
  [1] http://thread.gmane.org/gmane.comp.version-control.git/212631

  Documentation/gitattributes.txt | 6 +++---
  attr.c  | 8 +++-
  t/t0003-attributes.sh   | 5 +
  3 files changed, 15 insertions(+), 4 deletions(-)

 diff --git a/Documentation/gitattributes.txt b/Documentation/gitattributes.txt
 index 643c1ba..5d4d386 100644
 --- a/Documentation/gitattributes.txt
 +++ b/Documentation/gitattributes.txt
 @@ -20,9 +20,9 @@ Each line in `gitattributes` file is of form:
  
   pattern attr1 attr2 ...
  
 -That is, a pattern followed by an attributes list,
 -separated by whitespaces.  When the pattern matches the
 -path in question, the attributes listed on the line are given to
 +That is, a pattern followed by an attributes list, separated by
 +whitespaces that are not quoted by a backslash. When the pattern matches
 +the path in question, the attributes listed on the line are given to
  the path.
  
  Each attribute can be in one of these states for a given path:
 diff --git a/attr.c b/attr.c
 index 8d13d70..699716d 100644
 --- a/attr.c
 +++ b/attr.c
 @@ -209,7 +209,13 @@ static struct match_attr *parse_attr_line(const char 
 *line, const char *src,
   if (!*cp || *cp == '#')
   return NULL;
   name = cp;
 - namelen = strcspn(name, blank);
 + namelen = 0;
 + while (name[namelen] != '\0'  !strchr(blank, name[namelen])) {
 + if (name[namelen] == '\\'  name[namelen + 1] != '\0')
 + namelen += 2;
 + else
 + namelen++;
 + }
   if (strlen(ATTRIBUTE_MACRO_PREFIX)  namelen 
   starts_with(name, ATTRIBUTE_MACRO_PREFIX)) {
   if (!macro_ok) {
 diff --git a/t/t0003-attributes.sh b/t/t0003-attributes.sh
 index b9d7947..2f16805 100755
 --- a/t/t0003-attributes.sh
 +++ b/t/t0003-attributes.sh
 @@ -23,6 +23,7 @@ test_expect_success 'setup' '
   echo offon -test test
   echo no notest
   echo A/e/F test=A/e/F
 + echo A\\ b test=space
   ) .gitattributes 
   (
   echo g test=a/g 
 @@ -195,6 +196,10 @@ test_expect_success 'root subdir attribute test' '
   attr_check subdir/a/i unspecified
  '
  
 +test_expect_success 'quoting in pattern' '
 + attr_check A b space
 +'
 +
  test_expect_success 'negative patterns' '
   echo !f test=bar .gitattributes 
   git check-attr test -- '''!f''' 2errors 
--
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: error: src refspec refs/heads/master matches more than one.

2014-02-18 Thread Junio C Hamano
John Keeping j...@keeping.me.uk writes:

 There's already the arbitrary set of prefixes in
 refs.c::prettify_refname() and refs.c::ref_rev_parse_rules().  I can see
 how a user might think that since git log refs/heads/name is
 equivalent to git log master then git branch refs/heads/name should
 be equivalent to git branch name.

Not quite, I am afraid.  Branch names used for git branch name
and git checkout name are like the Lvalue of an assignment, as
opposed to extended SHA-1 expressions to express any commit
(e.g. 'master^0', 'refs/heads/master', or 'master') that correspond
to the Rvalues used in an expression.  Because git checkout can
take a branch name or an arbitrary commit object name, there needs a
way for the users to disambiguate.

Saying that git checkout refs/heads/name must be equivalent to
git checkout name is like arguing that assignment value+0 = x
should be valid because value+0 is a valid value.

For the first parameter to git branch, there is no ambiguity---it
must be the name of a branch and cannot be an arbitrary commit
object name, so special casing git branch refs/heads/master to
mean git branch master may not be too bad.  But then we need to
either start rejecting git branch refs/tags/v1.0 or keep allowing
it to create a ref refs/heads/refs/tags/v1.0 to denote the branch of
that exact name given---neither of which is more consistent, so...
--
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/5] implement @{publish} shorthand

2014-02-18 Thread Junio C Hamano
Jeff King p...@peff.net writes:

 In that sense, publish is not the best word, either, as it describes
 only the first two, but not the third case (and those are just examples;
 there may be other setups beyond that, even).

 Perhaps @{push} would be the most direct word.

Hmph, then the other one would be @{pull}.

Which does not sound too bad, IMHO.
--
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] Provide a 'git help user-manual' route to the docbook

2014-02-18 Thread Junio C Hamano
Philip Oakley philipoak...@iee.org writes:

 diff --git a/Documentation/gituser-manual.txt 
 b/Documentation/gituser-manual.txt
 new file mode 100644
 index 000..9fd4744
 --- /dev/null
 +++ b/Documentation/gituser-manual.txt
 @@ -0,0 +1,34 @@
 +gituser-manual(7)
 +=
 +
 +NAME
 +
 +gituser-manual - a link to the user-manual docbook
 +
 +
 +SYNOPSIS
 +
 +[verse]
 +'git help user-manual'
 +
 +link:user-manual.html[Git User's Manual]

Is it just me, or is typing

$ git help user-manual

and not seeing the manual itself, but only a link you have to click
to get there a worthwhile addition?

I would not mind having a clickable link in the output from

$ git help git

or something that does already have other useful information, though.

 +
 +DESCRIPTION
 +---
 +Git is a fast, scalable, distributed revision control system with an
 +unusually rich command set that provides both high-level operations
 +and full access to internals.
 +
 +The link:user-manual.html[Git User's Manual] provides an
 +in-depth introduction to Git.
 +
 +SEE ALSO
 +
 +linkgit:gittutorial[7],
 +linkgit:giteveryday[7],
 +linkgit:gitcli[7],
 +linkgit:gitworkflows[7]
 +
 +GIT
 +---
 +Part of the linkgit:git[1] suite
 diff --git a/builtin/help.c b/builtin/help.c
 index 1fdefeb..be7c39d 100644
 --- a/builtin/help.c
 +++ b/builtin/help.c
 @@ -427,6 +427,7 @@ static struct {
   { modules, N_(Defining submodule properties) },
   { revisions, N_(Specifying revisions and ranges for Git) },
   { tutorial, N_(A tutorial introduction to Git (for version 1.5.1 or 
 newer)) },
 + { user-manual, N_(A link to the user-manual docbook) },
   { workflows, N_(An overview of recommended workflows with Git) },
  };
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH] Fix documentation AsciiDoc links for external urls

2014-02-18 Thread Junio C Hamano
Roberto Tyley roberto.ty...@gmail.com writes:

 Turns out that putting 'link:' before the 'http' is actually superfluous
 in AsciiDoc, as there's already a predefined macro to handle it.

 http, https, [etc] URLs are rendered using predefined inline macros.
 http://www.methods.co.nz/asciidoc/userguide.html#_urls

 Hypertext links to files on the local file system are specified
 using the link inline macro.
 http://www.methods.co.nz/asciidoc/userguide.html#_linking_to_local_documents

 Despite being superfluous, the reference implementation of AsciiDoc
 tolerates the extra 'link:' and silently removes it, giving a functioning
 link in the generated HTML. However, AsciiDoctor (the Ruby implementation
 of AsciiDoc used to render the http://git-scm.com/ site) does /not/ have
 this behaviour, and so generates broken links, as can be seen here:

 http://git-scm.com/docs/git-cvsimport (links to cvs2git  parsecvs)
 http://git-scm.com/docs/git-filter-branch (link to The BFG)

 It's worth noting that after this change, the html generated by 'make html'
 in the git project is identical, and all links still work.
 ---

Sign-off?

The overall reasoning sounds good, and the patch also looks sensible.

Thanks.


  Documentation/git-cvsimport.txt   | 4 ++--
  Documentation/git-filter-branch.txt   | 4 ++--
  Documentation/gitcore-tutorial.txt| 2 +-
  Documentation/gitcvs-migration.txt| 2 +-
  Documentation/gitweb.txt  | 2 +-
  Documentation/technical/http-protocol.txt | 4 ++--
  6 files changed, 9 insertions(+), 9 deletions(-)

 diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
 index 2df9953..260f39f 100644
 --- a/Documentation/git-cvsimport.txt
 +++ b/Documentation/git-cvsimport.txt
 @@ -21,8 +21,8 @@ DESCRIPTION
  *WARNING:* `git cvsimport` uses cvsps version 2, which is considered
  deprecated; it does not work with cvsps version 3 and later.  If you are
  performing a one-shot import of a CVS repository consider using
 -link:http://cvs2svn.tigris.org/cvs2git.html[cvs2git] or
 -link:https://github.com/BartMassey/parsecvs[parsecvs].
 +http://cvs2svn.tigris.org/cvs2git.html[cvs2git] or
 +https://github.com/BartMassey/parsecvs[parsecvs].
  
  Imports a CVS repository into Git. It will either create a new
  repository, or incrementally import into an existing one.
 diff --git a/Documentation/git-filter-branch.txt 
 b/Documentation/git-filter-branch.txt
 index 2eba627..09535f2 100644
 --- a/Documentation/git-filter-branch.txt
 +++ b/Documentation/git-filter-branch.txt
 @@ -436,7 +436,7 @@ git-filter-branch allows you to make complex 
 shell-scripted rewrites
  of your Git history, but you probably don't need this flexibility if
  you're simply _removing unwanted data_ like large files or passwords.
  For those operations you may want to consider
 -link:http://rtyley.github.io/bfg-repo-cleaner/[The BFG Repo-Cleaner],
 +http://rtyley.github.io/bfg-repo-cleaner/[The BFG Repo-Cleaner],
  a JVM-based alternative to git-filter-branch, typically at least
  10-50x faster for those use-cases, and with quite different
  characteristics:
 @@ -455,7 +455,7 @@ characteristics:
_is_ possible to write filters that include their own parallellism,
in the scripts executed against each commit.
  
 -* The link:http://rtyley.github.io/bfg-repo-cleaner/#examples[command 
 options]
 +* The http://rtyley.github.io/bfg-repo-cleaner/#examples[command options]
are much more restrictive than git-filter branch, and dedicated just
to the tasks of removing unwanted data- e.g:
`--strip-blobs-bigger-than 1M`.
 diff --git a/Documentation/gitcore-tutorial.txt 
 b/Documentation/gitcore-tutorial.txt
 index 058a352..d2d7c21 100644
 --- a/Documentation/gitcore-tutorial.txt
 +++ b/Documentation/gitcore-tutorial.txt
 @@ -1443,7 +1443,7 @@ Although Git is a truly distributed system, it is often
  convenient to organize your project with an informal hierarchy
  of developers. Linux kernel development is run this way. There
  is a nice illustration (page 17, Merges to Mainline) in
 -link:http://www.xenotime.net/linux/mentor/linux-mentoring-2006.pdf[Randy 
 Dunlap's presentation].
 +http://www.xenotime.net/linux/mentor/linux-mentoring-2006.pdf[Randy Dunlap's 
 presentation].
  
  It should be stressed that this hierarchy is purely *informal*.
  There is nothing fundamental in Git that enforces the chain of
 diff --git a/Documentation/gitcvs-migration.txt 
 b/Documentation/gitcvs-migration.txt
 index 5ea94cb..5f4e890 100644
 --- a/Documentation/gitcvs-migration.txt
 +++ b/Documentation/gitcvs-migration.txt
 @@ -117,7 +117,7 @@ Importing a CVS archive
  ---
  
  First, install version 2.1 or higher of cvsps from
 -link:http://www.cobite.com/cvsps/[http://www.cobite.com/cvsps/] and make
 +http://www.cobite.com/cvsps/[http://www.cobite.com/cvsps/] and make
  sure it is in your path.  Then cd to a checked out CVS working directory
  of the project you are 

Re: [PATCH] rev-parse: fix --resolve-git-dir argument handling

2014-02-18 Thread Junio C Hamano
John Keeping j...@keeping.me.uk writes:

 There are two problems here:

 1) If no argument is provided, then the command segfaults
 2) The argument is not consumed, so there will be excess output

 Fix both of these in one go by restructuring the handler for this
 option.

 Reported-by: Daniel Hahler genml+git-2...@thequod.de
 Signed-off-by: John Keeping j...@keeping.me.uk
 ---

Looks sensible; thanks.

  builtin/rev-parse.c | 7 +--
  1 file changed, 5 insertions(+), 2 deletions(-)

 diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
 index aaeb611..645cc4a 100644
 --- a/builtin/rev-parse.c
 +++ b/builtin/rev-parse.c
 @@ -738,9 +738,12 @@ int cmd_rev_parse(int argc, const char **argv, const 
 char *prefix)
   continue;
   }
   if (!strcmp(arg, --resolve-git-dir)) {
 - const char *gitdir = resolve_gitdir(argv[i+1]);
 + const char *gitdir;
 + if (++i = argc)
 + die(--resolve-git-dir requires an 
 argument);
 + gitdir = resolve_gitdir(argv[i]);
   if (!gitdir)
 - die(not a gitdir '%s', argv[i+1]);
 + die(not a gitdir '%s', argv[i]);
   puts(gitdir);
   continue;
   }
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: git gc --aggressive led to about 40 times slower git log --raw

2014-02-18 Thread Junio C Hamano
Jonathan Nieder jrnie...@gmail.com writes:

 David Kastrup wrote:
 Duy Nguyen pclo...@gmail.com writes:

 Likely because --aggressive passes --depth=250 to pack-objects. Long
 delta chains could reduce pack size and increase I/O as well as zlib
 processing signficantly.
 [...]
 Compression should reduce rather than increase the total amount of
 reads.

 --depth=250 means to allow chains of To get this object, first
 inflate this object, then apply this delta of length 250.

 That's absurdly long, and doesn't even help compression much in
 practice (many short chains referring to the same objects tends to
 work fine).  We probably shouldn't make --aggressive do that.
 Something like --depth=10 would make more sense.

Yes, my thinking indeed.

I didn't know --agressive was so aggressive myself, as I personally
never use it. git repack -a -d -f --depth=32 window=4000 is what I
often use, but I suspect most people would not be patient enough for
that 4k window.

Let's do something like this first and then later make --depth
configurable just like --width, perhaps?  For aggressive, I think
the default width (hardcoded to 250 but configurable) is a bit too
narrow.

 builtin/gc.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 6be6c8d..0d010f0 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -204,7 +204,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 
if (aggressive) {
argv_array_push(repack, -f);
-   argv_array_push(repack, --depth=250);
+   argv_array_push(repack, --depth=20);
if (aggressive_window  0)
argv_array_pushf(repack, --window=%d, 
aggressive_window);
}
--
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] diff: do not reuse_worktree_file for submodules

2014-02-18 Thread Junio C Hamano
Thomas Rast t...@thomasrast.ch writes:

 The GIT_EXTERNAL_DIFF calling code attempts to reuse existing worktree
 files for the worktree side of diffs, for performance reasons.
 However, that code also tries to do the same with submodules.  This
 results in calls to $GIT_EXTERNAL_DIFF where the old-file is a file of
 the form Submodule commit $sha1, but the new-file is a directory in
 the worktree.

 Fix it by never reusing a worktree file in the submodule case.

 Reported-by: Grégory Pakosz gregory.pak...@gmail.com
 Signed-off-by: Thomas Rast t...@thomasrast.ch
 ---
  diff.c   |  5 +++--
  t/t4020-diff-external.sh | 30 +-
  2 files changed, 32 insertions(+), 3 deletions(-)

 diff --git a/diff.c b/diff.c
 index 7c59bfe..e9a8874 100644
 --- a/diff.c
 +++ b/diff.c
 @@ -2845,8 +2845,9 @@ static struct diff_tempfile *prepare_temp_file(const 
 char *name,
   remove_tempfile_installed = 1;
   }
  
 - if (!one-sha1_valid ||
 - reuse_worktree_file(name, one-sha1, 1)) {
 + if (!S_ISGITLINK(one-mode) 
 + (!one-sha1_valid ||
 +  reuse_worktree_file(name, one-sha1, 1))) {

I agree with the goal/end result, but I have to wonder if the
reuse_worktree_file() be the helper function that ought to
encapsulate such a logic?

Instead of feeding it an object name and a path, if we passed a
diff_filespec to the helper, it would have access to the mode as
well.  It would result in a more intrusive change, so I'd prefer to
see your patch applied first and then build such a refactor on top,
perhaps like the attached.

 diff.c | 18 ++
 1 file changed, 10 insertions(+), 8 deletions(-)

diff --git a/diff.c b/diff.c
index a96992a..74eec80 100644
--- a/diff.c
+++ b/diff.c
@@ -2582,11 +2582,13 @@ void fill_filespec(struct diff_filespec *spec, const 
unsigned char *sha1,
  * the work tree has that object contents, return true, so that
  * prepare_temp_file() does not have to inflate and extract.
  */
-static int reuse_worktree_file(const char *name, const unsigned char *sha1, 
int want_file)
+static int reuse_worktree_file(const struct diff_filespec *spec, int want_file)
 {
const struct cache_entry *ce;
struct stat st;
int pos, len;
+   const char *name = spec-path;
+   const unsigned char *sha1 = spec-sha1;
 
/*
 * We do not read the cache ourselves here, because the
@@ -2698,7 +2700,7 @@ int diff_populate_filespec(struct diff_filespec *s, int 
size_only)
return diff_populate_gitlink(s, size_only);
 
if (!s-sha1_valid ||
-   reuse_worktree_file(s-path, s-sha1, 0)) {
+   reuse_worktree_file(s, 0)) {
struct strbuf buf = STRBUF_INIT;
struct stat st;
int fd;
@@ -2844,17 +2846,17 @@ static struct diff_tempfile *prepare_temp_file(const 
char *name,
 
if (!S_ISGITLINK(one-mode) 
(!one-sha1_valid ||
-reuse_worktree_file(name, one-sha1, 1))) {
+reuse_worktree_file(one, 1))) {
struct stat st;
-   if (lstat(name, st)  0) {
+   if (lstat(one-path, st)  0) {
if (errno == ENOENT)
goto not_a_valid_file;
-   die_errno(stat(%s), name);
+   die_errno(stat(%s), one-path);
}
if (S_ISLNK(st.st_mode)) {
struct strbuf sb = STRBUF_INIT;
-   if (strbuf_readlink(sb, name, st.st_size)  0)
-   die_errno(readlink(%s), name);
+   if (strbuf_readlink(sb, one-path, st.st_size)  0)
+   die_errno(readlink(%s), one-path);
prep_temp_blob(name, temp, sb.buf, sb.len,
   (one-sha1_valid ?
one-sha1 : null_sha1),
@@ -2864,7 +2866,7 @@ static struct diff_tempfile *prepare_temp_file(const char 
*name,
}
else {
/* we can borrow from the file in the work tree */
-   temp-name = name;
+   temp-name = one-path;
if (!one-sha1_valid)
strcpy(temp-hex, sha1_to_hex(null_sha1));
else
--
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] tree-diff: rework diff_tree() to generate diffs for multiparent cases as well

2014-02-18 Thread Junio C Hamano
Kirill Smelkov k...@navytux.spb.ru writes:

  2) alloca(), for small arrays, is used for the same reason - if we change
  it to xmalloc()/free() the timings get worse
 
 Do you see any use of it outside compat/?
 
 I thought we specifically avoid alloca() for portability.  Also we
 do not use variable-length-arrays on the stack either, I think.

 No, no usage outside compat/ and I knew alloca and VLAs are not used in
 Git codebase for portability, and I understand alloca will be
 criticized, but wanted to start the discussion rolling.

 I've actually started without alloca, and used xmalloc/free for
 [nparent] vectors, but the impact was measurable, so it just had to be
 changed to something more optimal.

 For me, personally, alloca is ok, but I understand there could be
 portability issues (by the way, what compiler/system Git cares about
 does not have working alloca?). Thats why I propose we do the following

 1. at configure time, determine, do we have working alloca, and define

 #define HAVE_ALLOCA

if yes.

 2. in code

 #ifdef HAVE_ALLOCA
 # define xalloca(size)  (alloca(size))
 # define xalloca_free(p)do {} while(0)
 #else
 # define xalloca(size)  (xmalloc(size))
 # define xalloca_free(p)(free(p))
 #endif

and use it like

func() {
p = xalloca(size);
...

xalloca_free(p);
}

 This way, for systems, where alloca is available, we'll have optimal
 on-stack allocations with fast executions. On the other hand, on
 systems, where alloca is not available, this gracefully fallbacks to
 xmalloc/free.

 Please tell me what you think.

I guess the above is clean enough, and we cannot do better than that,
if we want to use alloca() on platforms where we can.
--
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] Rename read_replace_refs to check_replace_refs

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

 The semantics of this flag was changed in commit

 ecef23 inline lookup_replace_object() calls

 but wasn't renamed at the time to minimize code churn.  Rename it now,
 and add a comment explaining its use.

 Signed-off-by: Michael Haggerty mhag...@alum.mit.edu
 ---
 This change doesn't conflict with anything in pu; perhaps we can
 squeeze it in now?

I think it is a good time to do this kind of clean-up once the
post-release dusts settle.

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: error: src refspec refs/heads/master matches more than one.

2014-02-18 Thread Junio C Hamano
David Kastrup d...@gnu.org writes:

 Junio C Hamano gits...@pobox.com writes:

 Duy Nguyen pclo...@gmail.com writes:

 +   if (!force  dwim_ref(name, strlen(name), sha1, real_ref))
 +   die(_(creating ref refs/heads/%s makes %s ambiguous.\n
 + Use -f to create it anyway.),
 +   name, name);

 Does this check still allow you to create a branch refs/heads/next
 and then later create a branch next?  The latter will introduce an
 ambiguity without any prevention, even though the prevention would
 trigger if the order in which these two branches are created is
 swapped--- the end result has ambiguity but the safety covers only one
 avenue to the confusing situation.

 And the only way I can think of to avoid that kind of confusion is
 to forbid creation of a subset of possible names by reserving a set
 of known (but arbitrary) prefixes---which I am not sure is a good
 way to go.  At least not yet.

 Just for the record: after seeing the respective arguments, I consider
 it the sanest way.

 It's conceivable to give a configuration option for augmenting the set
 of reserved prefixes.  That would allow to adapt the arbitrariness to
 match the policies or ref name choices of a particular project while
 keeping the set of references addressed by the standard git commands in
 check automagically.

I am inclined to say that we should start by just giving a warning
whenever git branch, git checkout -b, etc. tries to create a
branch whose name begins with refs/ and other potentially
ambiguous ones that match ref_rev_parse_rules[].  I personally do
not think people name their branch with a name that begins with
refs/ on purpose; I am not sure about other ones, like heads or
tags. Personally I think it also is unlikely to want to have these
words immediately followed by a slash in the branch name, so it may
not even be necessary to give them any way to turn off the warning,
which in turn would mean we can promote that warning to an die()
that can be overridable with --force, but by going that first
warn and see if people are annoyed route, we would hopefully find
out soon enough that there may be some people who do want to name
their branches in a funny way ;-)

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


Re: [PATCH] Git release notes man page

2014-02-18 Thread Junio C Hamano
Philip Oakley philipoak...@iee.org writes:

 A few days too late for the 1.9.0 release cycle :(

 This responds to Stefan Nwe's request for a 'git help' command that would
 access the release notes. ($gmane/240595 17 Jan 2014).

 I've used the full name release-notes for the help guide rather than
 Stefan's original 'git help relnotes'.

 The release-notes man page lists just the notes for the current release.
 The combined notes for all releases is nearing 15k lines.

RelNotes are incremental and only useful for those who know what the
immediately previous release contained, but for most people who get
their Git from distros, I have this impression that the versions of
Git they get skip versions, and seeing the notable changes since the
previous source release will not give them wrong information---they
may have this warm fuzzy feeling that they know what is going on,
but they are missing information on all the accumulated changes that
were added in earlier versions their distro skipped---these changes
are still in the version they are running.  I do not understand why
it is even a good idea to show release notes from the command line
git interface.
--
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] config: teach git config --file - to read from the standard input

2014-02-18 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 I think I preferred the earlier version where you had stdin in the
 name field, and this hunk could just go away. I know you switched it to
 NULL here to avoid making bogus relative filenames in includes.

 Exactly the same comment here.  I really like the way how this
 series becomes more polished every time we see it.

 But that would be pretty straightforward to fix by splitting the name
 field into an additional path field. It looks like git config --blob
 has the same problem (it will try relative lookups in the filesystem,
 even though that is nonsensical from the blob). So we could fix the
 existing bug at the same time. :)

 Perhaps this could go at the start of your series?

 Sounds like the right thing to do.

 Thanks.

And [PATCH 3/3] rebased on the others may look like this.

 builtin/config.c  | 11 +++
 cache.h   |  1 +
 config.c  | 42 --
 t/t1300-repo-config.sh| 17 +++--
 t/t1305-config-include.sh | 16 +++-
 5 files changed, 70 insertions(+), 17 deletions(-)

diff --git a/builtin/config.c b/builtin/config.c
index de41ba5..5677c94 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -360,6 +360,9 @@ static int get_colorbool(int print)
 
 static void check_write(void)
 {
+   if (given_config_source.use_stdin)
+   die(writing to stdin is not supported);
+
if (given_config_source.blob)
die(writing config blobs is not supported);
 }
@@ -472,6 +475,12 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
usage_with_options(builtin_config_usage, 
builtin_config_options);
}
 
+   if (given_config_source.file 
+   !strcmp(given_config_source.file, -)) {
+   given_config_source.file = NULL;
+   given_config_source.use_stdin = 1;
+   }
+
if (use_global_config) {
char *user_config = NULL;
char *xdg_config = NULL;
@@ -558,6 +567,8 @@ int cmd_config(int argc, const char **argv, const char 
*prefix)
check_argc(argc, 0, 0);
if (!given_config_source.file  nongit)
die(not in a git directory);
+   if (given_config_source.use_stdin)
+   die(editing stdin is not supported);
if (given_config_source.blob)
die(editing blobs is not supported);
git_config(git_default_config, NULL);
diff --git a/cache.h b/cache.h
index 9d94bd6..4db19b5 100644
--- a/cache.h
+++ b/cache.h
@@ -1147,6 +1147,7 @@ extern int update_server_info(int);
 #define CONFIG_GENERIC_ERROR 7
 
 struct git_config_source {
+   unsigned int use_stdin:1;
const char *file;
const char *blob;
 };
diff --git a/config.c b/config.c
index a21b0dd..9dd0bdb 100644
--- a/config.c
+++ b/config.c
@@ -1031,24 +1031,36 @@ static int do_config_from(struct config_source *top, 
config_fn_t fn, void *data)
return ret;
 }
 
-int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+static int do_config_from_file(config_fn_t fn,
+  const char *filename, FILE *f,
+  const char *label, void *data)
 {
-   int ret;
-   FILE *f = fopen(filename, r);
+   struct config_source top;
 
-   ret = -1;
-   if (f) {
-   struct config_source top;
+   top.u.file = f;
+   top.name = label;
+   top.path = filename;
+   top.die_on_error = 1;
+   top.do_fgetc = config_file_fgetc;
+   top.do_ungetc = config_file_ungetc;
+   top.do_ftell = config_file_ftell;
+
+   return do_config_from(top, fn, data);
+}
 
-   top.u.file = f;
-   top.name = top.path = filename;
-   top.die_on_error = 1;
-   top.do_fgetc = config_file_fgetc;
-   top.do_ungetc = config_file_ungetc;
-   top.do_ftell = config_file_ftell;
+static int git_config_from_stdin(config_fn_t fn, void *data)
+{
+   return do_config_from_file(fn, NULL, stdin, stdin, data);
+}
 
-   ret = do_config_from(top, fn, data);
+int git_config_from_file(config_fn_t fn, const char *filename, void *data)
+{
+   int ret = -1;
+   FILE *f;
 
+   f = fopen(filename, r);
+   if (f) {
+   ret = do_config_from_file(fn, filename, f, filename, data);
fclose(f);
}
return ret;
@@ -1190,7 +1202,9 @@ int git_config_with_options(config_fn_t fn, void *data,
 * If we have a specific filename, use it. Otherwise, follow the
 * regular lookup sequence.
 */
-   if (config_source  config_source-file)
+   if (config_source  config_source-use_stdin)
+   return git_config_from_stdin(fn, data);
+   else if (config_source  config_source-file

Re: [PATCH] Git release notes man page

2014-02-18 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Philip Oakley philipoak...@iee.org writes:

 A few days too late for the 1.9.0 release cycle :(

 This responds to Stefan Nwe's request for a 'git help' command that would
 access the release notes. ($gmane/240595 17 Jan 2014).

 I've used the full name release-notes for the help guide rather than
 Stefan's original 'git help relnotes'.

 The release-notes man page lists just the notes for the current release.
 The combined notes for all releases is nearing 15k lines.

 RelNotes are incremental and only useful for those who know what the
 immediately previous release contained, but for most people who get
 their Git from distros, I have this impression that the versions of
 Git they get skip versions, and seeing the notable changes since the
 previous source release will not give them wrong information---they

Ehh,, s/will not give them/will give them/; obviously...

 may have this warm fuzzy feeling that they know what is going on,
 but they are missing information on all the accumulated changes that
 were added in earlier versions their distro skipped---these changes
 are still in the version they are running.  I do not understand why
 it is even a good idea to show release notes from the command line
 git interface.
--
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-contacts: do not fail parsing of good diffs

2014-02-18 Thread Junio C Hamano
lar...@gullik.org (Lars Gullik Bjønnes) writes:

 If a line in a patch starts with ---  it will be deemed
 malformed unless it also contains the proper diff header
 format. This situation can happen with a valid patch if
 it has a line starting with --  and that line is removed.

 This patch just removes the check in git-contacts.

 Signed-off-by: Lars Gullik Bjønnes lar...@gullik.org
 ---

If the script wanted to be more correct, it should be paying
attention to the $len it already parses out of the hunk headers to
make sure it does not mistake removal of a line that begins with --
 as the beginning of a patch to a different path, but as the
original does not seem to aim to be so careful anyway, this change
should be OK, I would say.

The patch was whitespace damaged, by the way.  It was easy to hand
tweak so there is no need to resend this particular patch, but if
you are planning to send more patches, please check your MUA and
tell it not to.

Thanks.

  contrib/contacts/git-contacts | 2 --
  1 file changed, 2 deletions(-)

 diff --git a/contrib/contacts/git-contacts b/contrib/contacts/git-contacts
 index 428cc1a..dbe2abf 100755
 --- a/contrib/contacts/git-contacts
 +++ b/contrib/contacts/git-contacts
 @@ -96,8 +96,6 @@ sub scan_patches {
 next unless $id;
 if (m{^--- (?:a/(.+)|/dev/null)$}) {
 $source = $1;
 -   } elsif (/^--- /) {
 -   die Cannot parse hunk source: $_\n;
 } elsif (/^@@ -(\d+)(?:,(\d+))?/  $source) {
 my $len = defined($2) ? $2 : 1;
 push @{$sources-{$source}{$id}}, [$1, $len] if $len;
 -- 
 1.9.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 5/5] streaming: simplify attaching a filter

2014-02-18 Thread Junio C Hamano
John Keeping j...@keeping.me.uk writes:

 We are guaranteed that 'nst' is non-null because it is allocated with
 xmalloc(), and in fact we rely on this three lines later by
 unconditionally dereferencing it.

The intent of the original code is for attach_stream_filter() to
detect an error condition and return NULL, in which case it closes
the istream it allocated and signal error to the caller, I think,
and falling thru to use st-anything and return st when that happens
is *not* a guarantee that a-s-f will not detect an error ever, but
rather is a bug in the error codepath.


 Signed-off-by: John Keeping j...@keeping.me.uk
 ---
  streaming.c | 5 +
  1 file changed, 1 insertion(+), 4 deletions(-)

 diff --git a/streaming.c b/streaming.c
 index d7c9f32..8a7135d 100644
 --- a/streaming.c
 +++ b/streaming.c
 @@ -151,10 +151,7 @@ struct git_istream *open_istream(const unsigned char 
 *sha1,
   }
   if (filter) {
   /* Add  !is_null_stream_filter(filter) for performance */
 - struct git_istream *nst = attach_stream_filter(st, filter);
 - if (!nst)
 - close_istream(st);
 - st = nst;
 + st = attach_stream_filter(st, filter);
   }
  
   *size = st-size;
--
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/5] streaming: simplify attaching a filter

2014-02-18 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 John Keeping j...@keeping.me.uk writes:

 We are guaranteed that 'nst' is non-null because it is allocated with
 xmalloc(), and in fact we rely on this three lines later by
 unconditionally dereferencing it.

 The intent of the original code is for attach_stream_filter() to
 detect an error condition and return NULL, in which case it closes
 the istream it allocated and signal error to the caller, I think,
 and falling thru to use st-anything and return st when that happens
 is *not* a guarantee that a-s-f will not detect an error ever, but
 rather is a bug in the error codepath.

In other words, something like this instead.

-- 8 --
Subject: [PATCH] open_istream(): do not dereference NULL in the error case

When stream-filter cannot be attached, it is expected to return NULL,
and we should close the stream we opened and signal an error by
returning NULL ourselves from this function.

However, we attempted to dereference that NULL pointer between the
point we detected the error and returned from the function.

Brought-to-attention-by: John Keeping j...@keeping.me.uk
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 streaming.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

diff --git a/streaming.c b/streaming.c
index d7c9f32..2ff036a 100644
--- a/streaming.c
+++ b/streaming.c
@@ -152,8 +152,10 @@ struct git_istream *open_istream(const unsigned char *sha1,
if (filter) {
/* Add  !is_null_stream_filter(filter) for performance */
struct git_istream *nst = attach_stream_filter(st, filter);
-   if (!nst)
+   if (!nst) {
close_istream(st);
+   return NULL;
+   }
st = nst;
}
 
-- 
1.9.0-279-gdc9e3eb

--
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 gc --aggressive led to about 40 times slower git log --raw

2014-02-18 Thread Junio C Hamano
Duy Nguyen pclo...@gmail.com writes:

 Lower depth than default (50) does not sound aggressive to me, at
 least from disk space utilization. I agree it should be configurable
 though.

Do you mean you want to keep --aggressive to mean too aggressive
in resulting size, to the point that it is not useful to anybody?

Shallow and wide will give us, with a large window, the most
aggressively efficient packfiles that are useful, and we would
rather want to fix it to be usable, I would think.

--
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] rev-parse: fix --resolve-git-dir argument handling

2014-02-18 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 John Keeping j...@keeping.me.uk writes:

 There are two problems here:

 1) If no argument is provided, then the command segfaults
 2) The argument is not consumed, so there will be excess output

 Fix both of these in one go by restructuring the handler for this
 option.

 Reported-by: Daniel Hahler genml+git-2...@thequod.de
 Signed-off-by: John Keeping j...@keeping.me.uk
 ---

 Looks sensible; thanks.

Ehh, I spoke too fast. Don't we already have this queued as a43219f2
(rev-parse: check i before using argv[i] against argc, 2014-01-28)?

--
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/3] introduce GIT_INDEX_VERSION environment variable

2014-02-18 Thread Junio C Hamano
Thomas Gummerer t.gumme...@gmail.com writes:

 diff --git a/Documentation/git.txt b/Documentation/git.txt
 index aec3726..bc9eeea 100644
 --- a/Documentation/git.txt
 +++ b/Documentation/git.txt
 @@ -712,6 +712,11 @@ Git so take care if using Cogito etc.
   index file. If not specified, the default of `$GIT_DIR/index`
   is used.
  
 +'GIT_INDEX_VERSION'::
 + This environment variable allows the specification of an index
 + version for new repositories.  It won't affect existing index
 + files.  By default index file version 3 is used.
 +

This is half-correct, isn't it?  In-code variable may say version 3
but we demote it to version 2 unless we absolutely need to use the
version 3 ugliness.

 diff --git a/read-cache.c b/read-cache.c
 index 3f735f3..3993e12 100644
 --- a/read-cache.c
 +++ b/read-cache.c
 @@ -1223,6 +1223,22 @@ static struct cache_entry *refresh_cache_entry(struct 
 cache_entry *ce, int reall
  
  #define INDEX_FORMAT_DEFAULT 3
  
 +static unsigned int get_index_format_default()
 +{
 + char *envversion = getenv(GIT_INDEX_VERSION);
 + if (!envversion) {
 + return INDEX_FORMAT_DEFAULT;
 + } else {
 + unsigned int version = strtol(envversion, NULL, 10);

If we are using strtol() to parse it carefully, we should make sure
it parses to the end by giving a non-NULL second argument and
checking where the parsing stopped.

 diff --git a/t/t1600-index.sh b/t/t1600-index.sh
 new file mode 100755
 index 000..37fd84d
 --- /dev/null
 +++ b/t/t1600-index.sh
 @@ -0,0 +1,24 @@
 +#!/bin/sh
 +
 +test_description='index file specific tests'
 +
 +. ./test-lib.sh
 +
 +test_expect_success 'setup' '
 + echo 1 a
 +'
 +
 +test_expect_success 'out of bounds GIT_INDEX_VERSION issues warning' '
 + (
 + GIT_INDEX_VERSION=1 
 + export GIT_INDEX_VERSION 
 + git add a 21 | sed s/[0-9]// actual.err 
 + sed -e s/ Z$/ / -\EOF expect.err 
 + warning: GIT_INDEX_VERSION set, but the value is 
 invalid.
 + Using version Z
 + EOF
 + test_i18ncmp expect.err actual.err
 + )
 +'

If we already have an index in version N when this test starts, what
should happen?

Will queue on 'pu', with some suggested fixups.

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 2/3] test-lib: allow setting the index format version

2014-02-18 Thread Junio C Hamano
Thomas Gummerer t.gumme...@gmail.com writes:

 Allow adding a TEST_GIT_INDEX_VERSION variable to config.mak to set the
 index version with which the test suite should be run.
 ...
 diff --git a/Makefile b/Makefile
 index 287e6f8..c98d28f 100644
 --- a/Makefile
 +++ b/Makefile
 @@ -342,6 +342,10 @@ all::
  # Define DEFAULT_HELP_FORMAT to man, info or html
  # (defaults to man) if you want to have a different default when
  # git help is called without a parameter specifying the format.
 +#
 +# Define TEST_GIT_INDEX_FORMAT to 2, 3 or 4 to run the test suite

s/_FORMAT/_VERSION/, I would think.

Will queue on 'pu' with a fix-up on top.

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] Git release notes man page

2014-02-19 Thread Junio C Hamano
Philip Oakley philipoak...@iee.org writes:

 RelNotes are incremental and only useful for those who know what the
 immediately previous release contained, but for most people who get
 their Git from distros, I have this impression that the versions of
 Git they get skip versions, and seeing the notable changes since the
 previous source release will not give them wrong information---they
 may have this warm fuzzy feeling that they know what is going on,
 but they are missing information on all the accumulated changes that
 were added in earlier versions their distro skipped---these changes
 are still in the version they are running.

 That's a reasonable argument.

I am not making an argument in order to reject the notion of
making release-notes available, though. I am only raising concerns,
pointing out why showing a single release notes as if that were the
only one that matter is misleading.

I am not opposed to the idea of making release notes available to
those who do not install from the source, by the way. Being able to
grep the release notes through may help people who are writing
scripts using Git to learn when a feature they want to use appeared
to make sure that they do not depend on something their users may
not have yet. But for that kind of users, it would be more helpful
to point them at the file location they can find the plain text
version of release notes, instead of giving them a bunch of web
links they need to read through page by page.

 I did look at trying to get the
 stalenotes to work as an alternative, that is extract the stalenotes
 section from the git.txt, and create a release notes man page from
 that.

I am not sure if stale-notes section meshes well with this; the
primary purpose of it was to point at the whole documentation set,
not just release-notes, for versions previously released, so those
who came to a website that hosts them can pick the version, possibly
a stale one, that they are using and read the manual pages for that
version, without seeing newer features that are not available to
them.  I do not think it is very useful in the context of your You
received this single version of software, and you can access its
documentation off-line feature---you cannot reasonably expect that
such a software release to contain all the past documentation sets,
but even if you could do so, that is not how normal people use the
installed documentation, i.e. to learn about older releases that
they no longer 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: git gc --aggressive led to about 40 times slower git log --raw

2014-02-19 Thread Junio C Hamano
Philippe Vaucher philippe.vauc...@gmail.com writes:

 fwiw this is the thread that added --depth=250

 http://thread.gmane.org/gmane.comp.gcc.devel/94565/focus=94626

 This post is quite interesting:
 http://article.gmane.org/gmane.comp.gcc.devel/94637

Yes, it most clearly says that --depth=250 was *not* a
recommendation, with technical background to explain why such a long
delta chain is a bad idea.
--
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] gitweb: Avoid overflowing page body frame with large images

2014-02-19 Thread Junio C Hamano
Andrew Keller and...@kellerfarm.com writes:

 When displaying a blob in gitweb, if it's an image, specify constraints for
 maximum display width and height to prevent the image from overflowing the
 frame of the enclosing page_body div.

 This change assumes that it is more desirable to see the whole image without
 scrolling (new behavior) than it is to see every pixel without zooming
 (previous behavior).

 Signed-off-by: Andrew Keller and...@kellerfarm.com
 ---

 This is an updated copy of this patch.

 Could I request a thumbs up, thumbs down, or thumbs sideways from those who 
 develop gitweb?

I do not develop gitweb, but the change looks reasonable to me.

  gitweb/gitweb.perl   |2 +-
  gitweb/static/gitweb.css |5 +
  2 files changed, 6 insertions(+), 1 deletion(-)

 diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
 index 3bc0f0b..79057b7 100755
 --- a/gitweb/gitweb.perl
 +++ b/gitweb/gitweb.perl
 @@ -7094,7 +7094,7 @@ sub git_blob {
   git_print_page_path($file_name, blob, $hash_base);
   print div class=\page_body\\n;
   if ($mimetype =~ m!^image/!) {
 - print qq!img type=!.esc_attr($mimetype).qq!!;
 + print qq!img class=blob type=!.esc_attr($mimetype).qq!!;
   if ($file_name) {
   print qq! alt=!.esc_attr($file_name).qq! 
 title=!.esc_attr($file_name).qq!!;
   }
 diff --git a/gitweb/static/gitweb.css b/gitweb/static/gitweb.css
 index 3b4d833..3212601 100644
 --- a/gitweb/static/gitweb.css
 +++ b/gitweb/static/gitweb.css
 @@ -32,6 +32,11 @@ img.avatar {
   vertical-align: middle;
  }
  
 +img.blob {
 + max-height: 100%;
 + max-width: 100%;
 +}
 +
  a.list img.avatar {
   border-style: none;
  }
--
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 v2 1/3] revert.c: Allow to specify -x via git-config

2014-02-19 Thread Junio C Hamano
Guido Günther a...@sigxcpu.org writes:

 Without this when maintaining stable branches it's easy to forget to use
 -x to track where a patch was cherry-picked from.
 ---
  Documentation/config.txt  |  4 
  Documentation/git-cherry-pick.txt |  8 
  builtin/revert.c  | 14 +-
  3 files changed, 25 insertions(+), 1 deletion(-)

Hmph.  Does this round address the issues raised in the previous
discussion in any way?

How does it affect people's existing scripts that use cherry-pick
and rely on it not doing the unwanted -x thing if such a
configuration variable is introduced as the first step in the
series, without even giving them to override the configured default
from the command line?

For that matter, I do not think a new override option from the
command line is a great solution, as that approach forces people's
existing script to be adjusted.

I personally found the way Jonathan explained why git backport
alias is the best solution (not just a usable workaround) very
compelling, especially his point (3):

 (3) The caller explicitly specifies their intent by running git
 backport.  It doesn't affect unrelated uses of cherry-pick on
 other branches.

I do not even mind throwing something like this:

#!/bin/sh
# git backport - cherry-pick with -x always on.
exec git cherry-pick -x $@

in contrib/ somewhere, which feels like a far more appropriate
solution to your easy to forget problem, at least to me.
--
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: release-notes could be clearer on git-fetch changes

2014-02-19 Thread Junio C Hamano
Jan Engelhardt jeng...@inai.de writes:

 The release notes for 1.9.0 read:

 * The --tags option to git fetch no longer tells the command to
   fetch _only_ the tags. It instead fetches tags _in addition to_
   what are fetched by the same command line without the option.

 I think the release notes should also say -- like it was done
 extensively for git add -- how to get back the old
 behavior (perhaps through now-different commands).

Perhaps, but the release note is not a place to repeat what the
documentation already teaches---it primarily is to enumerate the
changed areas, to highlight the things users may want to look up in the
documentation, to give them a starting point.

You would do something like this, I would think:

git fetch $there 'refs/tags/*:refs/tags/*'

--
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 14/25] setup.c: convert is_git_directory() to use strbuf

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---

(Only minor nits first during this round of review)

 diff --git a/strbuf.h b/strbuf.h
 index 73e80ce..aec9fdb 100644
 --- a/strbuf.h
 +++ b/strbuf.h
 @@ -116,6 +116,10 @@ extern void strbuf_add(struct strbuf *, const void *, 
 size_t);
  static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
   strbuf_add(sb, s, strlen(s));
  }
 +static inline void strbuf_addstr_at(struct strbuf *sb, size_t len, const 
 char *s) {

Please have the opening { on its own line.

Surrounding existing functions are all offenders, but that is not an
excuse to make it worse (cleaning them up will need to be done in a
separate patch).

 + strbuf_setlen(sb, len);
 + strbuf_add(sb, s, strlen(s));

I am not sure addstr_at() gives us a good abstraction, or at least
the name conveys what it does well not to confuse readers.

At first after only seeing its name, I would have expected that it
would splice the given string into an existing strbuf at the
location, not chopping the existing strbuf at the location and
appending.

 +}
  static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf 
 *sb2) {
   strbuf_grow(sb, sb2-len);
   strbuf_add(sb, sb2-buf, sb2-len);
--
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 18/25] setup.c: support multi-checkout repo setup

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

(Only nitpicks during this round of review).

 diff --git a/t/t1501-worktree.sh b/t/t1501-worktree.sh
 index 8f36aa9..d8bdaf4 100755
 --- a/t/t1501-worktree.sh
 +++ b/t/t1501-worktree.sh
 @@ -346,4 +346,80 @@ test_expect_success 'relative $GIT_WORK_TREE and git 
 subprocesses' '
   test_cmp expected actual
  '
  
 +test_expect_success 'Multi-worktree setup' '
 + mkdir work 
 + mkdir -p repo.git/repos/foo 
 + cp repo.git/HEAD repo.git/index repo.git/repos/foo 
 + unset GIT_DIR GIT_CONFIG GIT_WORK_TREE

Are these known to be set always when we get to this point?
Otherwise please use sane_unset.
--
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 23/25] checkout: detach if the branch is already checked out elsewhere

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 The normal rule is anything outside refs/heads/ is detached. This
 strictens the rule a bit more: if the branch is checked out (either in
 $GIT_COMMON_DIR/HEAD or any $GIT_DIR/repos/.../HEAD) then it's
 detached as well.

 A hint is given so the user knows where to go and do something there
 if they still want to checkout undetached here.

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com

(Only nitpicks during this round of review).

 diff --git a/t/t2025-checkout-to.sh b/t/t2025-checkout-to.sh
 index 76eae4a..f6a5c47 100755
 --- a/t/t2025-checkout-to.sh
 +++ b/t/t2025-checkout-to.sh
 @@ -13,13 +13,14 @@ test_expect_success 'checkout --to not updating paths' '
  '
  
  test_expect_success 'checkout --to a new worktree' '
 + git rev-parse HEAD expect 
   git checkout --to here master 
   (
   cd here 
   test_cmp ../init.t init.t 
 - git symbolic-ref HEAD actual 
 - echo refs/heads/master expect 
 - test_cmp expect actual 
 + ! git symbolic-ref HEAD 

test_must_fail?
--
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 25/25] gc: support prune --repos

2014-02-19 Thread Junio C Hamano
(Only nitpicks during this round of review).

 @@ -274,6 +284,7 @@ int cmd_gc(int argc, const char **argv, const char 
 *prefix)
   argv_array_pushl(reflog, reflog, expire, --all, NULL);
   argv_array_pushl(repack, repack, -d, -l, NULL);
   argv_array_pushl(prune, prune, --expire, NULL );
 + argv_array_pushl(prune_repos, prune, --repos, --expire, NULL );

No SP before closing ).
--
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 01/25] path.c: make get_pathname() return strbuf instead of static buffer

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

(Only nitpicks during this round of review).

 -static char *get_pathname(void)
 +static struct strbuf *get_pathname()

static struct strbuf *get_pathname(void)
--
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 24/25] prune: strategies for linked checkouts

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

(Only nitpicks during this round of review).

 + if (get_device_or_die(path) != get_device_or_die(get_git_dir())) {
 + strbuf_reset(sb);
 + strbuf_addf(sb, %s/locked, sb_repo.buf);
 + write_file(sb.buf, 1, located on a different file system\n);
 + keep_locked = 1;
 + } else {
 + strbuf_reset(sb);
 + strbuf_addf(sb, %s/link, sb_repo.buf);
 + link(sb_git.buf, sb.buf); /* it's ok to fail */

If so, perhaps tell that to the code by saying something like

(void) link(...);

?

But why is it OK to fail in the first place?  If we couldn't link,
don't you want to fall back to the lock codepath?  After all, the
are we on the same device? check is to cope with the case where
we cannot link and that alternate codepath is supposed to be
prepared to handle the ah, we cannot link case correctly, no?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


Re: [PATCH v3 24/25] prune: strategies for linked checkouts

2014-02-19 Thread Junio C Hamano
Junio C Hamano gits...@pobox.com writes:

 Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 (Only nitpicks during this round of review).

 +if (get_device_or_die(path) != get_device_or_die(get_git_dir())) {
 +strbuf_reset(sb);
 +strbuf_addf(sb, %s/locked, sb_repo.buf);
 +write_file(sb.buf, 1, located on a different file system\n);
 +keep_locked = 1;
 +} else {
 +strbuf_reset(sb);
 +strbuf_addf(sb, %s/link, sb_repo.buf);
 +link(sb_git.buf, sb.buf); /* it's ok to fail */

 If so, perhaps tell that to the code by saying something like

   (void) link(...);

 ?

 But why is it OK to fail in the first place?  If we couldn't link,
 don't you want to fall back to the lock codepath?  After all, the
 are we on the same device? check is to cope with the case where
 we cannot link and that alternate codepath is supposed to be
 prepared to handle the ah, we cannot link case correctly, no?

In other words, couldn't that part more like this?  That is, we
optimisiticly try the link(2) first and if it fails for whatever
reason fall back to whatever magic the lock codepath implements.

+   strbuf_reset(sb);
+   strbuf_addf(sb, %s/link, sb_repo.buf);
+   if (link(sb_git.buf, sb.buf)  0) {
+   strbuf_reset(sb);
+   strbuf_addf(sb, %s/locked, sb_repo.buf);
+   write_file(sb.buf, 1, located on a different file system\n);
+   keep_locked = 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 v3 00/25] Support multiple checkouts

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 In short you can attach multiple worktrees to the same git repository
 with git checkout --to somewhere. This is basically what
 git-new-workdir is for.

This is exciting.

I'll be pushing this out on 'pu' (with trivial fix-ups squashed in
and/or queued on-top) for people to play with.

I'm still slowly chewing thru these patches, though. Haven't had a
chance to read them carefully yet.

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


What's cooking in git.git (Feb 2014, #06; Wed, 19)

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

The tip of 'next' hasn't been rewound, and none of the topics that
have been cooking there has graduated, yet.  Hopefully that can
start happening to open the new development cycle later this week,
at which time I'll be also updating tinyurl.com/gitCal and also
annotate the topics listed in this file with the course of action
(i.e. if you run Meta/cook -w, many are listed as Undecided right
now, which I need to fix).

There are many stalled topics; I think some of them do deserve to be
discarded as marked, but others do solve (or at least attempt to)
real issues and it would be nice to see people help unblock them
(one reason for blockage would be that they introduce regressions).

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

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

--
[New Topics]

* bc/blame-crlf-test (2014-02-18) 1 commit
 - blame: add a failing test for a CRLF issue.


* jk/http-no-curl-easy (2014-02-18) 1 commit
 - http: never use curl_easy_perform


* jk/janitorial-fixes (2014-02-18) 5 commits
 - open_istream(): do not dereference NULL in the error case
 - builtin/mv: don't use memory after free
 - utf8: use correct type for values in interval table
 - utf8: fix iconv error detection
 - notes-utils: handle boolean notes.rewritemode correctly


* ks/config-file-stdin (2014-02-18) 4 commits
 - config: teach git config --file - to read from the standard input
 - config: change git_config_with_options() interface
 - builtin/config.c: rename check_blob_write() - check_write()
 - config: disallow relative include paths from blobs


* lb/contrib-contacts-looser-diff-parsing (2014-02-18) 1 commit
 - git-contacts: do not fail parsing of good diffs


* mh/replace-refs-variable-rename (2014-02-18) 1 commit
 - Rename read_replace_refs to check_replace_refs


* nd/commit-editor-cleanup (2014-02-18) 3 commits
 - commit: add --cleanup=scissors
 - wt-status.c: move cut-line print code out to wt_status_add_cut_line
 - wt-status.c: make cut_line[] const to shrink .data section a bit


* nd/no-more-fnmatch (2014-02-18) 4 commits
 - Actually remove compat fnmatch source code
 - Stop using fnmatch (either native or compat)
 - Revert test-wildmatch: add perf command to compare wildmatch and fnmatch
 - Use wildmatch() directly without fnmatch() wrapper


* nd/reset-setup-worktree (2014-02-18) 1 commit
 - reset: optionally setup worktree and refresh index on --mixed


* po/git-help-user-manual (2014-02-18) 1 commit
 - Provide a 'git help user-manual' route to the docbook


* rt/links-for-asciidoctor (2014-02-18) 1 commit
 - Fix documentation AsciiDoc links for external urls


* tg/index-v4-format (2014-02-18) 5 commits
 - read-cache: add index.version config variable
 - FIXUP
 - test-lib: allow setting the index format version
 - FIXUP
 - introduce GIT_INDEX_VERSION environment variable


* tr/diff-submodule-no-reuse-worktree (2014-02-18) 2 commits
 - diff: refactor reuse_worktree_file()
 - diff: do not reuse_worktree_file for submodules


* nd/multiple-work-trees (2014-02-19) 26 commits
 - FIXUP???
 - gc: support prune --repos
 - prune: strategies for linked checkouts
 - checkout: detach if the branch is already checked out elsewhere
 - checkout: clean up half-prepared directories in --to mode
 - checkout: support checking out into a new working directory
 - use new wrapper write_file() for simple file writing
 - wrapper.c: wrapper to open a file, fprintf then close
 - setup.c: support multi-checkout repo setup
 - setup.c: detect $GIT_COMMON_DIR check_repository_format_gently()
 - setup.c: convert check_repository_format_gently to use strbuf
 - setup.c: detect $GIT_COMMON_DIR in is_git_directory()
 - setup.c: convert is_git_directory() to use strbuf
 - git-stash: avoid hardcoding $GIT_DIR/logs/
 - *.sh: avoid hardcoding $GIT_DIR/hooks/...
 - git-sh-setup.sh: use rev-parse --git-path to get $GIT_DIR/objects
 - Add new environment variable $GIT_COMMON_DIR
 - commit: use SEQ_DIR instead of hardcoding sequencer
 - fast-import: use git_path() for accessing .git dir instead of get_git_dir()
 - reflog: avoid constructing .lock path with git_path
 - *.sh: respect $GIT_INDEX_FILE
 - Make git_path() aware of file relocation in $GIT_DIR
 - path.c: group git_path(), git_pathdup() and strbuf_git_path() together
 - path.c: rename vsnpath() to do_git_path()
 - Convert git_snpath() to strbuf_git_path()
 - path.c: make get_pathname() return strbuf instead of static buffer

--
[Stalled]

* po/everyday-doc (2014-01-27) 1 commit
 - Make 'git help everyday' work

 This may make the said command to emit something, but the source is
 not meant to be formatted into a manual pages to begin with, and
 also its contents 

Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 We've been avoiding PATH_MAX whenever possible. This patch makes
 get_pathname() return a strbuf and updates the callers to take
 advantage of this. The code is simplified as we no longer need to
 worry about buffer overflow.

 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
  path.c | 119 
 +++--
  1 file changed, 50 insertions(+), 69 deletions(-)

Nice.

  char *git_pathdup(const char *fmt, ...)
  {
 - char path[PATH_MAX], *ret;
 + struct strbuf *path = get_pathname();
   va_list args;
   va_start(args, fmt);
 - ret = vsnpath(path, sizeof(path), fmt, args);
 + vsnpath(path, fmt, args);
   va_end(args);
 - return xstrdup(ret);
 + return strbuf_detach(path, NULL);
  }

This feels somewhat wrong.

This function is for callers who are willing to take ownership of
the path buffer and promise to free the returned buffer when they
are done, because you are returning strbuf_detach()'ed piece of
memory, giving the ownership away.

The whole point of using get_pathname() is to allow callers not to
care about allocation issues on the paths they scribble on during
their short-and-simple codepaths that do not have too many uses of
similar temporary path buffers.  Why borrow from that round-robin
pool (which may now cause some codepaths to overflow the number of
such active temporary path buffers---have they been all audited)?

Is there a reason not to do just an equivalent of

#define git_pathdup mkpathdup

and be done with it?  Am I missing something?

  char *mkpathdup(const char *fmt, ...)
  {
 - char *path;
   struct strbuf sb = STRBUF_INIT;
   va_list args;
 -
   va_start(args, fmt);
   strbuf_vaddf(sb, fmt, args);
   va_end(args);
 - path = xstrdup(cleanup_path(sb.buf));
 -
 - strbuf_release(sb);
 - return path;
 + strbuf_cleanup_path(sb);
 + return strbuf_detach(sb, NULL);
  }
  
  char *mkpath(const char *fmt, ...)
  {
   va_list args;
 - unsigned len;
 - char *pathname = get_pathname();
 -
 + struct strbuf *pathname = get_pathname();
   va_start(args, fmt);
 - len = vsnprintf(pathname, PATH_MAX, fmt, args);
 + strbuf_vaddf(pathname, fmt, args);
   va_end(args);
 - if (len = PATH_MAX)
 - return bad_path;
 - return cleanup_path(pathname);
 + return cleanup_path(pathname-buf);
  }

On the other hand, this one does seem correct.

  char *git_path(const char *fmt, ...)
  {
 - char *pathname = get_pathname();
 + struct strbuf *pathname = get_pathname();
   va_list args;
 - char *ret;
 -
   va_start(args, fmt);
 - ret = vsnpath(pathname, PATH_MAX, fmt, args);
 + vsnpath(pathname, fmt, args);
   va_end(args);
 - return ret;
 + return pathname-buf;
  }

So does this.

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 v3 02/25] Convert git_snpath() to strbuf_git_path()

2014-02-19 Thread Junio C Hamano
Nguyễn Thái Ngọc Duy  pclo...@gmail.com writes:

 @@ -651,14 +653,10 @@ static void update_refs_for_switch(const struct 
 checkout_opts *opts,
   new-name);
   }
   }
 - if (old-path  old-name) {
 - char log_file[PATH_MAX], ref_file[PATH_MAX];
 -
 - git_snpath(log_file, sizeof(log_file), logs/%s, 
 old-path);
 - git_snpath(ref_file, sizeof(ref_file), %s, old-path);
 - if (!file_exists(ref_file)  file_exists(log_file))
 - remove_path(log_file);
 - }
 + if (old-path  old-name 
 + !file_exists(git_path(%s, old-path)) 
 +  file_exists(git_path(logs/%s, old-path)))
 + remove_path(git_path(logs/%s, old-path));

Hmph.  Is this conversion safe?

This adds three uses of the round-robin path buffer; if a caller of
this function used two or more path buffers obtained from
get_pathname() and expected their contents to remain stable across
the call to this, it will silently break.

--
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 02/25] Convert git_snpath() to strbuf_git_path()

2014-02-19 Thread Junio C Hamano
Duy Nguyen pclo...@gmail.com writes:

 - }
 + if (old-path  old-name 
 + !file_exists(git_path(%s, old-path)) 
 +  file_exists(git_path(logs/%s, old-path)))
 + remove_path(git_path(logs/%s, old-path));

 Hmph.  Is this conversion safe?

 This adds three uses of the round-robin path buffer; if a caller of
 this function used two or more path buffers obtained from
 get_pathname() and expected their contents to remain stable across
 the call to this, it will silently break.

 three round-robin buffers but not all required at the same time, once
 the first file_exists() returns the first round-robin buffer could be
 free, and remove_path() calls git_path again, not reusing the result
 from the second file_exists, so the second buffer is free to go too.

I know these three callers to git_path() will not step on each
other's toes.  But that is not the question I asked.
--
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: error: src refspec refs/heads/master matches more than one.

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

 I wonder whether we could give a way to specify a reference in an
 unambiguous, canonical fashion like I expected, for example by using a
 leading slash: /refs/heads/mybranch.  This could be a way for the user
 to ask for DWIMming to be turned off without having to resort to
 plumbing commands like update-ref.  This wouldn't necessarily solve the
 problem, but it would at least lead the new user to type

 git branch /refs/heads/mybranch

 instead of the ambiguous command above, which Git could either accept or
 reject in good conscience rather than having to speculate about what the
 user *really* meant.  I think that supporting absolute reference names
 like this would also be useful for scripts, which otherwise probably
 often have subtle failure modes if the user has defined reference names
 that are ambiguous, modulo DWIM, with the reference that the script
 intended.

I do agree that things start to become confusing to the end users
when we tell refnames and object names apart and behave differently,
e.g. git checkout master vs git checkout master^0 (this example
uses a disambiguation syntax that is related to but different from
what you brought up).

For the name in git branch name [commit] (but not commit),
I do not see much value in allowing the users to say refs/heads/
in the first place---all the local branch refs are to be created in
refs/heads/ anyway and git branch /refs/tags/bar (if we were to
allow your notation to name an absolute ref) will have to be checked
and signaled as an error.

Even though there is no reason to forbid a ref to be named in such a
way at the lowest machinery level (read: at the sha1_name.c layer)
[*1*], I would say it would be better to at least warn users when
they create such a ref with Porcelain commands like branch,
checkout -b, etc., or even outright forbid.

In other contexts, however, it _might_ make sense, but I am somewhat
skeptical.  For example, if you have a branch 'foo' (whose ref being
refs/heads/foo) and a branch 'refs/heads/foo' (whose ref being
refs/heads/refs/heads/foo) at the same time, you need some way to
clarify that you mean the former, and one way to do so may be

git branch newfoo /refs/heads/foo

and removing the latter would be

git branch -D /refs/heads/refs/heads/foo

perhaps.  But this starts to sound like a workaround to a problem
that the user ended up having such a strangely named branch in the
first place, not a useful feature.

[Footnote]

*1* The way refs are used and the specific meanings given to some of
the hierarchies under refs/ by the core-git Porcelain is not
fundamental to the data model of Git.  Git the SCM by convention
uses refs/heads/ for branches, and it is perfectly fine for Git
the SCM to enforce its own policy like that to its end users and
forbid creating and using any ref outside that hierarchy as a
local branch (e.g. checking it out), but I'd prefer it if we can
keep the lower level a general filesystem to build SCM on top
layer as separate from such policy decision as possible.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html


<    1   2   3   4   5   6   7   8   9   10   >