Re: [PATCH v7 0/1] refs.c: SSE4.2 optimizations for check_refname_component

[Johannes Sixt]

Am 6/10/2014 1:05, schrieb Junio C Hamano:
 Junio C Hamano gits...@pobox.com writes:
 
 David Turner dtur...@twopensource.com writes:

 Since Junio has picked up the first patch from previous versions of
 this series, I'm just going to send the second (SSE) one.  I decided
 not to s/NO_SSE42/!HAVE_SSE42/ because it looks like git mostly uses
 the former convention (for instance, that's what GIT_PARSE_WITH
 generates).

 Yeah but NO_FROTZ is used only when FROTZ is something everybody is
 expected to have (e.g. it's in posix, people ought to have it, but
 we do support those who don't), isn't it?  For a very arch specific
 stuff like sse42, I'd feel better to make it purely opt-in by
 forcing people to explicitly say HAVE_SSE42 to enable it.
 
 Just FYI: I am getting
 
 compat/cpuid.h:8:12: error: 'processor_supports_sse42' defined but
 not used [-Werror=unused-function]
 cc1: all warnings being treated as errors
 
 while building 'pu'; I'll have to rebuild 'pu' without this patch
 before I can push the day's result out.

And I get this when I compile on Windows with msysgit:

CC abspath.o
In file included from git-compat-util.h:694,
 from cache.h:4,
 from abspath.c:1:
compat/cpuid.h: In function 'processor_supports_sse42':
compat/cpuid.h:11: warning: implicit declaration of function '__cpuid'
abspath.c: At top level:
compat/cpuid.h:8: warning: 'processor_supports_sse42' defined but not used
abspath.c: In function 'processor_supports_sse42':
compat/cpuid.h:11: warning: 'eax' is used uninitialized in this function
compat/cpuid.h:11: warning: 'ebx' is used uninitialized in this function
compat/cpuid.h:11: warning: 'ecx' is used uninitialized in this function
compat/cpuid.h:11: warning: 'edx' is used uninitialized in this function

Perhaps our gcc is too old?

-- Hannes
--
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] t9001: avoid not portable '\n' with sed

[Torsten Bögershausen]

On 06/10/2014 07:55 AM, Junio C Hamano wrote:

Torsten Bögershausen tbo...@web.de writes:


t9001 used a '\n' in a sed expression to split one line into two lines,
but the usage of '\n' in the replacement string is not portable.

This looks peculiarly familiar; don't I already have it queued?

Yes, V2 is queued and in pu,and only the commit msg is changed
between V2 and V3.

I think that V3 explains the difference between POSIX sed and
gnu sed much better, and does reflect all the comments from
the list, which otherwise may be lost.
And I suspect that not only the sed under Mac OS X does not
handle this very '\n' different from gnu sed, or in other words,
more platforms may not have a gnu sed and may need the fix.

--
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 10/20] git-submodule.sh: avoid test cond -a/-o cond

[Johannes Sixt]

Am 6/10/2014 1:23, schrieb Junio C Hamano:
 Elia Pinto gitter.spi...@gmail.com writes:
 
 @@ -1059,13 +1059,17 @@ cmd_summary() {
  while read mod_src mod_dst sha1_src sha1_dst status sm_path
  do
  # Always show modules deleted or type-changed 
 (blob-module)
 -test $status = D -o $status = T  echo $sm_path  
 continue
 +case $status in
 +[DT])
 +printf '%s\n' $sm_path 
 +continue
 +esac
 
 I think this conversion is wrong and causes parse error.  The
 surrounding code cannot be seen in the context of thsi patch, but
 looks somewhat like this:
 
   modules=$( 
case $status in
[DT])
...
esac
 )
 
 Perhaps you would need to spell it with the extra opening
 parenthesis, like so:
 
   case string in
 ([DT])
   ...
   esac
 
 or something.

Do you just think that it causes parse errors or did you actually observe
them? Because I think that no parse error should occur.

-- Hannes
--
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 v7 0/1] refs.c: SSE4.2 optimizations for check_refname_component

[Junio C Hamano]

Johannes Sixt j.s...@viscovery.net writes:

 And I get this when I compile on Windows with msysgit:

 CC abspath.o
 In file included from git-compat-util.h:694,
  from cache.h:4,
  from abspath.c:1:
 compat/cpuid.h: In function 'processor_supports_sse42':
 compat/cpuid.h:11: warning: implicit declaration of function '__cpuid'
 abspath.c: At top level:
 compat/cpuid.h:8: warning: 'processor_supports_sse42' defined but not used
 abspath.c: In function 'processor_supports_sse42':
 compat/cpuid.h:11: warning: 'eax' is used uninitialized in this function
 compat/cpuid.h:11: warning: 'ebx' is used uninitialized in this function
 compat/cpuid.h:11: warning: 'ecx' is used uninitialized in this function
 compat/cpuid.h:11: warning: 'edx' is used uninitialized in this function

 Perhaps our gcc is too old?

Maybe.

In any case, it is a good indication that it probably is a good idea
to start with an optional USE_SSE42 (not !NO_SSE42 or HAVE_SSE42) so
that it is clear to anybody that those with SSE42 does not have to
use this compilation option.  Once the code proves itself, we can
consider turning it on by default when able, but it seems that it is
a bit too premature for that (not that the code itself is premature
in the original author's environment, but its portability has not
been quite ready for everybody yet, it seems).
--
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] send-email: do not insert third header

[Stepan Kasal]

On Mon, Jun 09, 2014 at 10:38:14PM -0700, Junio C Hamano wrote:
 two new options [..]
 would support the recent kernel submission convention better.

indeed, but they are not there and I do not volunteer to write them.

Instead, I edit the generated patches to add the necessary headers.
But if send-mail is used to mass-send these edited mails, it adds yet
another header.  As a quick solution, I fixed send-mail, so that it
respects the header added by hand.

Even if send-mail is in the future enhanced according to your
proposal, it is still possible that hand-crafted headers will be
needed for a special purpose, so my patch still may come handy.

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

Webmail E-mail frissítések@2014

[webmail technical support]

--
Kedves Email felhasználói;

Ön túllépte a tárolási határt 23.432 az e-postafiók beállítva a
WEB SERVICE / Administrator, és akkor problémái küldés
és a bejövő üzenetek, amíg meg újból érvényesíti az e-mail címét. A 
szükséges eljárások

nyújtottak be az alábbi a véleménye, ellenőrizze kattintva
az alábbi linkre és töltse ki az adatokat, hogy érvényesítse az e-mail 
címét.

Kérjük, kattintson ide

http://mailupdatews.jigsy.com/

Növelni az e-mail kvótát az e-mail.
Figyelem!
Ennek elmulasztása, ez azt eredményezi, korlátozott hozzáférést a 
postafiókba.

Ha nem frissíti fiókját számított három napon belül a frissítés
értesítést, akkor figyelembe kell zárni véglegesen.

Tisztelettel,
Rendszergazda ®
--
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 08/20] git-mergetool.sh: avoid test cond -a/-o cond

[David Aguilar]

On Fri, Jun 6, 2014 at 7:55 AM, Elia Pinto gitter.spi...@gmail.com wrote:
 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.

 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---

This looks good to me.  Thanks Elia,

Acked-by: David Aguilar dav...@gmail.com

  git-mergetool.sh |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)

 diff --git a/git-mergetool.sh b/git-mergetool.sh
 index d08dc92..9a046b7 100755
 --- a/git-mergetool.sh
 +++ b/git-mergetool.sh
 @@ -205,7 +205,7 @@ checkout_staged_file () {
 $(git checkout-index --temp --stage=$1 $2 2/dev/null) \
 : '\([^ ]*\)')

 -   if test $? -eq 0 -a -n $tmpfile
 +   if test $? -eq 0  test -n $tmpfile
 then
 mv -- $(git rev-parse --show-cdup)$tmpfile $3
 else
 @@ -256,7 +256,7 @@ merge_file () {
 checkout_staged_file 2 $MERGED $LOCAL
 checkout_staged_file 3 $MERGED $REMOTE

 -   if test -z $local_mode -o -z $remote_mode
 +   if test -z $local_mode || test -z $remote_mode
 then
 echo Deleted merge conflict for '$MERGED':
 describe_file $local_mode local $LOCAL
 --
 1.7.10.4

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

Re: [PATCH 08/15] provide helpers to access the commit buffer

[Eric Sunshine]

On Monday, June 9, 2014, Jeff King p...@peff.net wrote:
 Many sites look at commit-buffer to get more detailed
 information than what is in the parsed commit struct.
 However, we sometimes drop commit-buffer to save memory,
 in which case the caller would need to read the object
 afresh. Some callers do this (leading to duplicated code),
 and others do not (which opens the possibility of a segfault
 if somebody else frees the buffer).

 Let's provide a pair of helpers, get and unuse, that let
 callers easily get the buffer. They will use the cached
 buffer when possible, and otherwise load from disk using
 read_sha1_file.

 Note that we also need to add a get_cached variant which
 returns NULL when we do not have a cached buffer. At first
 glance this seems to defeat the purpose of get, which is
 to always provide a return value. However, some log code
 paths actually use the NULL-ness of commit-buffer as a
 boolean flag to decide whether to try to printing the

s/printing/print/ ... or some other variation of the phrase.

 commit. At least for now, we want to continue supporting
 that use.

 Signed-off-by: Jeff King p...@peff.net
--
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 10/15] use get_commit_buffer to avoid duplicate code

[Eric Sunshine]

On Monday, June 9, 2014, Jeff King p...@peff.net wrote:
 For both of these sites, we already do the fallback to
 read_sha1_file trick. But we can shorten the code by just
 using get_commit_buffer.

 Note that the error cases are slightly different when
 read_sha1_file fails. get_commit_buffer will die() if the
 object cannot be loaded, or is a non-commit.

 For get_sha1_oneline, this will almost certainly never
 happen, as we will have just called parse_object (and if it
 does, it's probably worth complaining about).

 For record_author_date, the new behavior is probably better;
 we notify the user of the error instead of silently ignoring
 it. And because it's used only for sorting by author-date,
 somebody examining a corrupt can fallback to the regular

Missing word here? examining a corrupt [...] can

 traversal order.

 Signed-off-by: Jeff King p...@peff.net
--
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 10/20] git-submodule.sh: avoid test cond -a/-o cond

[Johannes Sixt]

Am 6/10/2014 8:52, schrieb Johannes Sixt:
 Am 6/10/2014 1:23, schrieb Junio C Hamano:
 Elia Pinto gitter.spi...@gmail.com writes:

 @@ -1059,13 +1059,17 @@ cmd_summary() {
 while read mod_src mod_dst sha1_src sha1_dst status sm_path
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 -   test $status = D -o $status = T  echo $sm_path  
 continue
 +   case $status in
 +   [DT])
 +   printf '%s\n' $sm_path 
 +   continue
 +   esac

 I think this conversion is wrong and causes parse error.  The
 surrounding code cannot be seen in the context of thsi patch, but
 looks somewhat like this:

  modules=$( 
case $status in
[DT])
...
esac
 )

 Perhaps you would need to spell it with the extra opening
 parenthesis, like so:

  case string in
 ([DT])
  ...
  esac

 or something.
 
 Do you just think that it causes parse errors or did you actually observe
 them? Because I think that no parse error should occur.

(I should not talk, but test...) bash and zsh get it wrong, dash and ksh
get it right.
http://pubs.opengroup.org/onlinepubs/9699919799/utilities/V3_chap02.html#tag_18_03
item 5 does leave some leeway for interpretation. So it's better to adjust
as you suggest.

-- Hannes
-- 
Atomic objects are neither active nor radioactive. --
Programming Languages -- C++, Final Committee Draft (Doc.N3092)
--
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

Disk waste with packs and .keep files

[Matthieu Moy]

Hi,

To minimize useless on-disk changes, I have a script that periodically
creates .keep files for pack files greater than 10 Mb (so than tools
like unison and incremental backup remain efficient). From time to time,
I delete these .keep files and git gc each repo. This worked well for
years.

Since a few weeks however, Git started wasting my disk space: instead of
creating small .pack files next to the big .keep-ed pack files, it seems
to create redundant, big .pack files (i.e. I get N pack files of similar
size). git verify-pack confirms that, for example, the object
corresponding to the root commit is contained in each of the .pack file.

I don't have a reproducible way to get the situation so I didn't bisect,
but git log --grep .keep points me to this which seems related:

  commit ee34a2beadb94a9595f09af719e3c09b485ca797
  Author: Jeff King p...@peff.net
  Date:   Mon Mar 3 15:04:20 2014 -0500

repack: add `repack.packKeptObjects` config var

Now, my questions:

Is the behavior I observed actually the intended behavior? Should Git be
fixed, or should I fix my flow (I guess, stop using .keep files and
start using pack.packSizeLimit or so)?

Or is my message not clear enough and do I need to investigate a bit
more?

Thanks,

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
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/5] hashmap: add enum for hashmap free_entries option

[Heiko Voigt]

On Fri, Jun 06, 2014 at 07:52:03PM +0200, Karsten Blees wrote:
 Am 05.06.2014 08:06, schrieb Heiko Voigt:
  This allows a reader to immediately know which options can be used and
  what this parameter is about.
  
 [...]
  -void hashmap_free(struct hashmap *map, int free_entries)
  +void hashmap_free(struct hashmap *map, enum hashmap_free_options 
  free_entries)
 [...]
   
  +enum hashmap_free_options {
  +   HASHMAP_NO_FREE_ENTRIES = 0,
  +   HASHMAP_FREE_ENTRIES = 1,
  +};
 
 This was meant as a boolean parameter. Would it make sense to have
 
 enum boolean {
   false,
   true
 };
 
 or similar in some central place?

The intention of Jonathans critique here[1] was that you do not see what
this parameter does on the callsite. I.e.:

hashmap_free(map, 1);

compared to

hashmap_free(map, HASHMAP_FREE_ENTRIES);

A boolean basically transfers the same information and would not help
the reader here.

Cheers Heiko

[1] http://article.gmane.org/gmane.comp.version-control.git/243917
--
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/5] implement submodule config cache for lookup of submodule names

[Heiko Voigt]

On Sun, Jun 08, 2014 at 05:04:44AM -0400, Eric Sunshine wrote:
 On Thu, Jun 5, 2014 at 2:07 AM, Heiko Voigt hvo...@hvoigt.net wrote:
  This submodule configuration cache allows us to lazily read .gitmodules
  configurations by commit into a runtime cache which can then be used to
  easily lookup values from it. Currently only the values for path or name
  are stored but it can be extended for any value needed.
 
  [...]
 
  Signed-off-by: Heiko Voigt hvo...@hvoigt.net
  ---
  diff --git a/t/t7410-submodule-config.sh b/t/t7410-submodule-config.sh
  new file mode 100755
  index 000..ea453c5
  --- /dev/null
  +++ b/t/t7410-submodule-config.sh
  @@ -0,0 +1,73 @@
  +#!/bin/sh
  +#
  +# Copyright (c) 2014 Heiko Voigt
  +#
  +
  +test_description='Test submodules config cache infrastructure
  +
  +This test verifies that parsing .gitmodules configuration directly
  +from the database works.
  +'
  +
  +TEST_NO_CREATE_REPO=1
  +. ./test-lib.sh
  +
  +test_expect_success 'submodule config cache setup' '
  +   mkdir submodule 
  +   (cd submodule 
  +   git init
 
 Broken -chain.

Will fix. Thanks for catching.

Cheers Heiko
--
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 (Jun 2014, #02; Fri, 6)

[Johan Herland]

On Sat, Jun 7, 2014 at 12:42 AM, Junio C Hamano gits...@pobox.com wrote:
 * jh/submodule-tests (2014-04-17) 1 commit
  - t7410: 210 tests for various 'git submodule update' scenarios

  What's the status of this one?

More or less abandoned. It was an experiment to try to understand the
interaction of the many variables that affect the behavior of 'git
submodule update'. The experiment raised some questions which were
discussed in the ensuing thread [1]. There is AFAIK ongoing work by
Jens to develop a more substantial test framework for submodules [2].
That work will certainly overlap (and preferably supersede) my tests.
I have not found the time to look more at that effort.

[1]: http://thread.gmane.org/gmane.comp.version-control.git/246312
[2]: http://thread.gmane.org/gmane.comp.version-control.git/245048/focus=245046


...Johan

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

Re: [PATCH v1] config: Add hashtable for config parsing retrival

[Eric Sunshine]

On Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra tanay...@gmail.com wrote:
 Add a hash table to cache all key-value pairs read from config files
 (repo specific .git/config, user wide ~/.gitconfig and the global
 /etc/gitconfig). Add two external functions `git_config_get_string` and
 `git_config_get_string_multi` for querying in a non-callback manner from the
 hash table.

 Signed-off-by: Tanay Abhra tanay...@gmail.com
 ---
 diff --git a/Documentation/technical/api-config.txt 
 b/Documentation/technical/api-config.txt
 index 230b3a0..5b6e376 100644
 --- a/Documentation/technical/api-config.txt
 +++ b/Documentation/technical/api-config.txt
 @@ -77,6 +77,24 @@ To read a specific file in git-config format, use
  `git_config_from_file`. This takes the same callback and data parameters
  as `git_config`.

 +Querying For Specific Variables
 +---
 +
 +For programs wanting to query for specific variables in a non-callback
 +manner, the config API provides two functions `git_config_get_string`
 +and `git_config_get_string_multi`. They both take a single parameter,
 +
 +- a key string in canonical flat form for which the corresponding values
 +  will be retrieved and returned.

It's strange to have a bulleted list for a single item. It can be
stated more naturally as a full sentence, without the list.

More below.

 +They both read values from an internal cache generated previously from
 +reading the config files. `git_config_get_string` returns the value with
 +the highest priority(i.e. for the same variable value in the repo config
 +will be preferred over value in user wide config).
 +
 +`git_config_get_string_multi` returns a `string_list` containing all the
 +values for that particular key, sorted in order of increasing priority.
 +
  Value Parsing Helpers
  -

 diff --git a/config.c b/config.c
 index a30cb5c..6f6b04e 100644
 --- a/config.c
 +++ b/config.c
 @@ -9,6 +9,8 @@
  #include exec_cmd.h
  #include strbuf.h
  #include quote.h
 +#include hashmap.h
 +#include string-list.h

  struct config_source {
 struct config_source *prev;
 @@ -37,6 +39,120 @@ static struct config_source *cf;

  static int zlib_compression_seen;

 +struct config_cache_entry {
 +   struct hashmap_entry ent;
 +   char *key;
 +   struct string_list *value_list;

Same question as in my v1 and v2 reviews [1][2], and reiterated by
Matthieu [3]: Why is 'value_list' a pointer to a structure rather than
just a structure?

This is a genuine question. As a reviewer, I'm not sure how to
interpret your lack of response. I can't tell if you merely overlook
or forget the question multiple times; if you don't understand the
inquiry; or if you have a strong reason to prefer making this a
pointer. If you don't understand the question, it's okay to ask for
clarification. If there is a strong reason for this to be a pointer,
it should be explained. Without feedback from you, reviewers will
continue asking the same question(s) upon each submission.

 +};
 +
 +static int hashmap_is_init;
 +
 +static int config_cache_set_value(const char *key, const char *value);
 +
 +static int config_cache_entry_cmp_case(const struct config_cache_entry *e1,
 +const struct config_cache_entry *e2, const 
 void *unused)
 +{
 +   return strcmp(e1-key, e2-key);

As suggested by Peff [4], this comparison is now case-sensitive,
instead of case-insensitive as in the previous version. Rather than
changing the appended '_icase' to '_case', a more typical function
name would be simply config_cache_entry_cmp().

 +}
 +
 +static void config_cache_init(struct hashmap *config_cache)
 +{
 +   hashmap_init(config_cache, (hashmap_cmp_fn) 
 config_cache_entry_cmp_case, 0);

In his review [4], Peff suggested giving config_cache_entry_cmp_case()
the correct hashmap_cmp_fn signature so that this cast can be dropped.
It's not clear whether you disagree with his advice or overlooked or
forgot about it. If you disagree with his suggestion, it's okay to say
so and explain why you feel the way you do, but without feedback from
you, he or another reviewer is going to continue suggesting the same
change.

 +}
 +
 +static int config_cache_callback(const char *key, const char *value, void 
 *unused)
 +{
 +   config_cache_set_value(key, value);
 +   return 0;
 +}
 +
 +static struct hashmap *get_config_cache(void)
 +{
 +   static struct hashmap config_cache;
 +   if (!hashmap_is_init) {
 +   config_cache_init(config_cache);
 +   hashmap_is_init = 1;
 +   git_config(config_cache_callback, NULL);
 +   return config_cache;

Why do you 'return' here rather than just falling through to the
'return' outside the conditional?

 +   }
 +   return config_cache;
 +}
 +
 +static void config_cache_free(void)
 +{
 +   struct hashmap *config_cache;
 +   struct config_cache_entry *entry;
 +   struct hashmap_iter iter;
 +   

Re: [PATCH v1] config: Add hashtable for config parsing retrival

[Eric Sunshine]

One additional comment...

On Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra tanay...@gmail.com wrote:
 +static int config_cache_set_value(const char *key, const char *value)
 +{
 +   struct hashmap *config_cache;
 +   struct config_cache_entry *e;
 +
 +   config_cache = get_config_cache();
 +   e = config_cache_find_entry(key);
 +   if (!e) {
 +   e = xmalloc(sizeof(*e));
 +   hashmap_entry_init(e, strhash(key));
 +   e-key = xstrdup(key);
 +   e-value_list = xcalloc(sizeof(struct string_list), 1);

Order of xcalloc() arguments is incorrect [1].

[1]: 
http://git.661346.n2.nabble.com/PATCH-00-15-Rearrange-xcalloc-arguments-td7611675.html

 +   e-value_list-strdup_strings = 1;
 +   string_list_append(e-value_list, value);
 +   hashmap_add(config_cache, e);
 +   } else {
 +   string_list_append(e-value_list, value);
 +   }
 +   return 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 v1] config: Add hashtable for config parsing retrival

[Eric Sunshine]

On Mon, Jun 9, 2014 at 10:24 AM, Matthieu Moy
matthieu@grenoble-inp.fr wrote:
 Tanay Abhra tanay...@gmail.com writes:
 +struct config_cache_entry {
 + struct hashmap_entry ent;
 + char *key;
 + struct string_list *value_list;
 +};

 I guess this crossed Eric's remark about the fact that this is a
 pointer.

 +static void config_cache_free(void)

 I didn't look closely enough to make sure there were no memory leak
 remaining, but did you check with valgrind --leak-check that it is the
 case in practice?

It does leak the config_cache_entry::value_list member which was
xcalloc()'d in config_cache_set_value().  (I didn't mention it in my
reviews because I was expecting 'value_list' to be changed to a plain
structure rather than a pointer to a structure.)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] Fix git-p4 submit in non --prepare-p4-only mode

[Maxime Coste]

b4073bb387ef303c9ac3c044f46d6a8ae6e190f0 broke git p4 submit, here
is a proper fix, including proper handling for windows end of lines.

Signed-off-by: Maxime Coste frrr...@gmail.com
---
 git-p4.py | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 7bb0f73..ff132b2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1238,7 +1238,7 @@ class P4Submit(Command, P4UserMap):
 if response == 'n':
 return False
 
-def get_diff_description(self, editedFiles):
+def get_diff_description(self, editedFiles, filesToAdd):
 # diff
 if os.environ.has_key(P4DIFF):
 del(os.environ[P4DIFF])
@@ -1258,7 +1258,7 @@ class P4Submit(Command, P4UserMap):
 newdiff += + + line
 f.close()
 
-return diff + newdiff
+return (diff + newdiff).replace('\r\n', '\n')
 
 def applyCommit(self, id):
 Apply one commit, return True if it succeeded.
@@ -1422,10 +1422,10 @@ class P4Submit(Command, P4UserMap):
 separatorLine =  everything below this line is just the diff 
###\n
 if not self.prepare_p4_only:
 submitTemplate += separatorLine
-submitTemplate += self.get_diff_description(editedFiles)
+submitTemplate += self.get_diff_description(editedFiles, 
filesToAdd)
 
 (handle, fileName) = tempfile.mkstemp()
-tmpFile = os.fdopen(handle, w+)
+tmpFile = os.fdopen(handle, w+b)
 if self.isWindows:
 submitTemplate = submitTemplate.replace(\n, \r\n)
 tmpFile.write(submitTemplate)
@@ -1475,9 +1475,9 @@ class P4Submit(Command, P4UserMap):
 tmpFile = open(fileName, rb)
 message = tmpFile.read()
 tmpFile.close()
-submitTemplate = message[:message.index(separatorLine)]
 if self.isWindows:
-submitTemplate = submitTemplate.replace(\r\n, \n)
+message = message.replace(\r\n, \n)
+submitTemplate = message[:message.index(separatorLine)]
 p4_write_pipe(['submit', '-i'], submitTemplate)
 
 if self.preserveUser:
-- 
2.0.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

[PATCH] git-submodule.sh: avoid test cond -a/-o cond

[Elia Pinto]

The construct is error-prone; test being built-in in most modern
shells, the reason to avoid test cond  test cond spawning
one extra process by using a single test cond -a cond no
longer exists.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---

This is the fourth revision of this patch.

Change based on Junio suggestions 
http://www.spinics.net/lists/git/msg233569.html

 git-submodule.sh |   32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..d6a1dea 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -393,7 +393,7 @@ cmd_add()
sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
fi
 
-   if test -z $repo -o -z $sm_path; then
+   if test -z $repo || test -z $sm_path; then
usage
fi
 
@@ -450,7 +450,7 @@ Use -f if you really want to add it. 2
# perhaps the path exists and is already a git repo, else clone it
if test -e $sm_path
then
-   if test -d $sm_path/.git -o -f $sm_path/.git
+   if test -d $sm_path/.git || test -f $sm_path/.git
then
eval_gettextln Adding existing repo at '\$sm_path' to 
the index
else
@@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
continue
fi
 
-   if ! test -d $sm_path/.git -o -f $sm_path/.git
+   if ! test -d $sm_path/.git || test -f $sm_path/.git
then
module_clone $sm_path $name $url $reference 
$depth || exit
cloned_modules=$cloned_modules;$name
@@ -857,11 +857,11 @@ Maybe you want to use 'update --init'?)
die $(eval_gettext Unable to find current 
${remote_name}/${branch} revision in submodule path '\$sm_path')
fi
 
-   if test $subsha1 != $sha1 -o -n $force
+   if test $subsha1 != $sha1 || test -n $force
then
subforce=$force
# If we don't already have a -f flag and the submodule 
has never been checked out
-   if test -z $subsha1 -a -z $force
+   if test -z $subsha1  test -z $force
then
subforce=-f
fi
@@ -1031,7 +1031,7 @@ cmd_summary() {
then
head=$rev
test $# = 0 || shift
-   elif test -z $1 -o $1 = HEAD
+   elif test -z $1 || test $1 = HEAD
then
# before the first commit: compare with an empty tree
head=$(git hash-object -w -t tree --stdin /dev/null)
@@ -1056,13 +1056,17 @@ cmd_summary() {
while read mod_src mod_dst sha1_src sha1_dst status sm_path
do
# Always show modules deleted or type-changed 
(blob-module)
-   test $status = D -o $status = T  echo $sm_path  
continue
+   case $status in
+   ([DT])
+   printf '%s\n' $sm_path 
+   continue
+   esac
# Respect the ignore setting for --for-status.
if test -n $for_status
then
name=$(module_name $sm_path)
ignore_config=$(get_submodule_config $name 
ignore none)
-   test $status != A -a $ignore_config = all  
continue
+   test $status != A  test $ignore_config = all 
 continue
fi
# Also show added or modified modules which are checked 
out
GIT_DIR=$sm_path/.git git-rev-parse --git-dir 
/dev/null 21 
@@ -1122,7 +1126,7 @@ cmd_summary() {
*)
errmsg=
total_commits=$(
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
range=$sha1_src...$sha1_dst
elif test $mod_src = 16
@@ -1159,7 +1163,7 @@ cmd_summary() {
# i.e. deleted or changed to blob
test $mod_dst = 16  echo $errmsg
else
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
limit=
test $summary_limit -gt 0  
limit=-$summary_limit
@@ -1230,7 +1234,11 @@ cmd_status()
say U$sha1 $displaypath
continue
fi
-   if test -z $url || ! 

Re: [PATCH v1] config: Add hashtable for config parsing retrival

[Tanay Abhra]

Hi,

Thanks for the review, Eric. I have replied to your comments below,
I will try to reply early and more promptly now.

On 06/10/2014 04:27 AM, Eric Sunshine wrote:
 ---
 diff --git a/Documentation/technical/api-config.txt 
 b/Documentation/technical/api-config.txt
 index 230b3a0..5b6e376 100644
 --- a/Documentation/technical/api-config.txt
 +++ b/Documentation/technical/api-config.txt
 @@ -77,6 +77,24 @@ To read a specific file in git-config format, use
  `git_config_from_file`. This takes the same callback and data parameters
  as `git_config`.

 +Querying For Specific Variables
 +---
 +
 +For programs wanting to query for specific variables in a non-callback
 +manner, the config API provides two functions `git_config_get_string`
 +and `git_config_get_string_multi`. They both take a single parameter,
 +
 +- a key string in canonical flat form for which the corresponding values
 +  will be retrieved and returned.
 
 It's strange to have a bulleted list for a single item. It can be
 stated more naturally as a full sentence, without the list.

Point Noted.

 +They both read values from an internal cache generated previously from
 +reading the config files. `git_config_get_string` returns the value with
 +the highest priority(i.e. for the same variable value in the repo config
 +will be preferred over value in user wide config).
 +
 +`git_config_get_string_multi` returns a `string_list` containing all the
 +values for that particular key, sorted in order of increasing priority.
 +
  Value Parsing Helpers
  -

 diff --git a/config.c b/config.c
 index a30cb5c..6f6b04e 100644
 --- a/config.c
 +++ b/config.c
 @@ -9,6 +9,8 @@
  #include exec_cmd.h
  #include strbuf.h
  #include quote.h
 +#include hashmap.h
 +#include string-list.h

  struct config_source {
 struct config_source *prev;
 @@ -37,6 +39,120 @@ static struct config_source *cf;

  static int zlib_compression_seen;

 +struct config_cache_entry {
 +   struct hashmap_entry ent;
 +   char *key;
 +   struct string_list *value_list;
 
 Same question as in my v1 and v2 reviews [1][2], and reiterated by
 Matthieu [3]: Why is 'value_list' a pointer to a structure rather than
 just a structure?
 

Sorry for the lack of response on this part. I thought choosing a pointer to a
structure or just the structure itself was a stylistic choice. Since most of the
functions just pass the pointer to this struct, I thought it would be more 
natural to
use it. But I have changed my mind on this one. In the next iteration I will be 
using
a plane old struct.

 
 +};
 +
 +static int hashmap_is_init;
 +
 +static int config_cache_set_value(const char *key, const char *value);
 +
 +static int config_cache_entry_cmp_case(const struct config_cache_entry *e1,
 +const struct config_cache_entry *e2, const 
 void *unused)
 +{
 +   return strcmp(e1-key, e2-key);
 
 As suggested by Peff [4], this comparison is now case-sensitive,
 instead of case-insensitive as in the previous version. Rather than
 changing the appended '_icase' to '_case', a more typical function
 name would be simply config_cache_entry_cmp().

Noted.

 +}
 +
 +static void config_cache_init(struct hashmap *config_cache)
 +{
 +   hashmap_init(config_cache, (hashmap_cmp_fn) 
 config_cache_entry_cmp_case, 0);
 
 In his review [4], Peff suggested giving config_cache_entry_cmp_case()
 the correct hashmap_cmp_fn signature so that this cast can be dropped.
 It's not clear whether you disagree with his advice or overlooked or
 forgot about it. If you disagree with his suggestion, it's okay to say
 so and explain why you feel the way you do, but without feedback from
 you, he or another reviewer is going to continue suggesting the same
 change.

Now on this one, I checked the code thoroughly. Every single hashmap_init()
incantation in git code has a hashmap_cmp_fn cast. I don't think that it is
necessary to do so. Is it?

 +}
 +
 +static int config_cache_callback(const char *key, const char *value, void 
 *unused)
 +{
 +   config_cache_set_value(key, value);
 +   return 0;
 +}
 +
 +static struct hashmap *get_config_cache(void)
 +{
 +   static struct hashmap config_cache;
 +   if (!hashmap_is_init) {
 +   config_cache_init(config_cache);
 +   hashmap_is_init = 1;
 +   git_config(config_cache_callback, NULL);
 +   return config_cache;
 
 Why do you 'return' here rather than just falling through to the
 'return' outside the conditional?

Noted.

 +static struct config_cache_entry *config_cache_find_entry(const char *key)
 +{
 +   struct hashmap *config_cache;
 +   struct config_cache_entry k;
 +   char *normalized_key;
 +   int baselength = 0, ret;
 +   config_cache = get_config_cache();
 +   ret = git_config_parse_key(key, normalized_key, baselength);
 
 Since you neither care about nor ever reference 'baselength', you
 should just 

Re: [PATCH v1] config: Add hashtable for config parsing retrival

[Tanay Abhra]

On 06/10/2014 04:45 AM, Eric Sunshine wrote:
 One additional comment...
 
 On Mon, Jun 9, 2014 at 8:49 AM, Tanay Abhra tanay...@gmail.com wrote:
 +static int config_cache_set_value(const char *key, const char *value)
 +{
 +   struct hashmap *config_cache;
 +   struct config_cache_entry *e;
 +
 +   config_cache = get_config_cache();
 +   e = config_cache_find_entry(key);
 +   if (!e) {
 +   e = xmalloc(sizeof(*e));
 +   hashmap_entry_init(e, strhash(key));
 +   e-key = xstrdup(key);
 +   e-value_list = xcalloc(sizeof(struct string_list), 1);
 
 Order of xcalloc() arguments is incorrect [1].
 

Noted. Thanks for pointing it out.

 [1]: 
 http://git.661346.n2.nabble.com/PATCH-00-15-Rearrange-xcalloc-arguments-td7611675.html
 
 +   e-value_list-strdup_strings = 1;
 +   string_list_append(e-value_list, value);
 +   hashmap_add(config_cache, e);
 +   } else {
 +   string_list_append(e-value_list, value);
 +   }
 +   return 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

[PATCH v2 0/5] Speed up cache loading time

[Nguyễn Thái Ngọc Duy]

Compared to v1 [1], this is like a new series

 - git-read-cache--daemon is renamed to git-index-helper (easier to
   guess what it's for)
 - simplified locking mechanism on shared memory
 - UNIX signals are used for notification instead of UNIX sockets
 - Windows support (only tested with wine)

I think I'm getting closer to something that can finally have a chance
of merging. Still don't know how to write tests for this though.

[1] http://thread.gmane.org/gmane.comp.version-control.git/247268/focus=248760

Nguyễn Thái Ngọc Duy (5):
  read-cache: allow to keep mmap'd memory after reading
  index-helper: new daemon for caching index and related stuff
  index-helper: add Windows support
  daemonize(): set a flag before exiting the main process
  index-helper: add --detach

 .gitignore   |   1 +
 Documentation/git-index-helper.txt (new) |  26 
 Makefile |   7 ++
 builtin/gc.c |   2 +-
 cache.h  |   6 +-
 config.mak.uname |   3 +
 daemon.c |   2 +-
 git-compat-util.h|   1 +
 index-helper.c (new) | 198 +++
 read-cache.c |  97 +--
 setup.c  |   4 +-
 shm.c (new)  | 163 +
 shm.h (new)  |  23 
 13 files changed, 521 insertions(+), 12 deletions(-)
 create mode 100644 Documentation/git-index-helper.txt
 create mode 100644 index-helper.c
 create mode 100644 shm.c
 create mode 100644 shm.h

-- 
1.9.1.346.ga2b5940

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

[PATCH 2/5] index-helper: new daemon for caching index and related stuff

[Nguyễn Thái Ngọc Duy]

The shared memory's name folows the template git-something-SHA1
where SHA1 is the trailing SHA-1 of the index file. something is
index for caching index files. If such shared memory exists, it
contains the same index content as on disk. The content is already
validated by the daemon and git won't validate it again. Note that it
does not necessarily use the same format as the on-disk version. The
content could be in a format that can be parsed much faster, or even
reused without parsing).

Git can poke the daemon to tell it to refresh the index cache, or to
not exit for another some minutes via UNIX signals. It can't give any
real data directly to the daemon. Real data goes to disk first, then
the daemon reads and verifies it from there.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 .gitignore   |   1 +
 Documentation/git-index-helper.txt (new) |  24 +
 Makefile |   7 ++
 cache.h  |   1 +
 config.mak.uname |   1 +
 git-compat-util.h|   1 +
 index-helper.c (new) | 145 +++
 read-cache.c |  78 +++--
 shm.c (new)  |  67 ++
 shm.h (new)  |  23 +
 10 files changed, 341 insertions(+), 7 deletions(-)
 create mode 100644 Documentation/git-index-helper.txt
 create mode 100644 index-helper.c
 create mode 100644 shm.c
 create mode 100644 shm.h

diff --git a/.gitignore b/.gitignore
index 70992a4..5a829dd 100644
--- a/.gitignore
+++ b/.gitignore
@@ -71,6 +71,7 @@
 /git-http-fetch
 /git-http-push
 /git-imap-send
+/git-index-helper
 /git-index-pack
 /git-init
 /git-init-db
diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
new file mode 100644
index 000..d0b1365
--- /dev/null
+++ b/Documentation/git-index-helper.txt
@@ -0,0 +1,24 @@
+git-index-helper(1)
+=
+
+NAME
+
+git-index-helper - A simple cache server for speeding up index file access
+
+SYNOPSIS
+
+[verse]
+'git index-helper
+
+DESCRIPTION
+---
+Keep the index file in memory for faster access. This daemon is per
+repository. This daemon is only available on POSIX system with
+shared memory support (e.g. Linux)
+
+OPTIONS
+---
+
+GIT
+---
+Part of the linkgit:git[1] suite
diff --git a/Makefile b/Makefile
index f7058a6..d42f3cc 100644
--- a/Makefile
+++ b/Makefile
@@ -886,6 +886,7 @@ LIB_OBJS += sha1-lookup.o
 LIB_OBJS += sha1_file.o
 LIB_OBJS += sha1_name.o
 LIB_OBJS += shallow.o
+LIB_OBJS += shm.o
 LIB_OBJS += sideband.o
 LIB_OBJS += sigchain.o
 LIB_OBJS += split-index.o
@@ -1498,6 +1499,12 @@ ifdef HAVE_DEV_TTY
BASIC_CFLAGS += -DHAVE_DEV_TTY
 endif
 
+ifdef HAVE_SHM
+   BASIC_CFLAGS += -DHAVE_SHM
+   EXTLIBS += -lrt
+   PROGRAM_OBJS += index-helper.o
+endif
+
 ifdef DIR_HAS_BSD_GROUP_SEMANTICS
COMPAT_CFLAGS += -DDIR_HAS_BSD_GROUP_SEMANTICS
 endif
diff --git a/cache.h b/cache.h
index 6549e02..f05e062 100644
--- a/cache.h
+++ b/cache.h
@@ -483,6 +483,7 @@ extern int is_index_unborn(struct index_state *);
 extern int read_index_unmerged(struct index_state *);
 #define COMMIT_LOCK(1  0)
 #define CLOSE_LOCK (1  1)
+#define REFRESH_DAEMON (1  2)
 extern int write_locked_index(struct index_state *, struct lock_file *lock, 
unsigned flags);
 extern int discard_index(struct index_state *);
 extern int unmerged_index(const struct index_state *);
diff --git a/config.mak.uname b/config.mak.uname
index eee0fc2..8de61a4 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -39,6 +39,7 @@ ifeq ($(uname_S),Linux)
HAVE_PATHS_H = YesPlease
LIBC_CONTAINS_LIBINTL = YesPlease
HAVE_DEV_TTY = YesPlease
+   HAVE_SHM = YesPlease
 endif
 ifeq ($(uname_S),GNU/kFreeBSD)
NO_STRLCPY = YesPlease
diff --git a/git-compat-util.h b/git-compat-util.h
index f6d3a46..a6ebecc 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -356,6 +356,7 @@ static inline const char *skip_prefix(const char *str, 
const char *prefix)
 #define PROT_READ 1
 #define PROT_WRITE 2
 #define MAP_PRIVATE 1
+#define MAP_SHARED 2
 #endif
 
 #define mmap git_mmap
diff --git a/index-helper.c b/index-helper.c
new file mode 100644
index 000..e10d0c3
--- /dev/null
+++ b/index-helper.c
@@ -0,0 +1,145 @@
+#include cache.h
+#include parse-options.h
+#include sigchain.h
+#include split-index.h
+#include shm.h
+
+static unsigned char cached_sha1[20];
+static unsigned char cached_shared_sha1[20];
+static void *shm_index, *shm_sharedindex;
+static size_t size_index, size_sharedindex;
+static int daemonized;
+
+static void cleanup(void)
+{
+   if (daemonized)
+   return;
+   unlink(git_path(read-cache--daemon.pid));
+   if (shm_index) {
+   munmap(shm_index, size_index);
+   

[PATCH 1/5] read-cache: allow to keep mmap'd memory after reading

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 cache.h  |  3 +++
 read-cache.c | 13 -
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index c6b7770..6549e02 100644
--- a/cache.h
+++ b/cache.h
@@ -290,10 +290,13 @@ struct index_state {
struct split_index *split_index;
struct cache_time timestamp;
unsigned name_hash_initialized : 1,
+keep_mmap : 1,
 initialized : 1;
struct hashmap name_hash;
struct hashmap dir_hash;
unsigned char sha1[20];
+   void *mmap;
+   size_t mmap_size;
 };
 
 extern struct index_state the_index;
diff --git a/read-cache.c b/read-cache.c
index 342fe52..a5031f3 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1495,6 +1495,10 @@ int do_read_index(struct index_state *istate, const char 
*path, int must_exist)
mmap = xmmap(NULL, mmap_size, PROT_READ | PROT_WRITE, MAP_PRIVATE, fd, 
0);
if (mmap == MAP_FAILED)
die_errno(unable to map index file);
+   if (istate-keep_mmap) {
+   istate-mmap = mmap;
+   istate-mmap_size = mmap_size;
+   }
close(fd);
 
hdr = mmap;
@@ -1547,10 +1551,12 @@ int do_read_index(struct index_state *istate, const 
char *path, int must_exist)
src_offset += 8;
src_offset += extsize;
}
-   munmap(mmap, mmap_size);
+   if (!istate-keep_mmap)
+   munmap(mmap, mmap_size);
return istate-cache_nr;
 
 unmap:
+   istate-mmap = NULL;
munmap(mmap, mmap_size);
die(index file corrupt);
 }
@@ -1576,6 +1582,7 @@ int read_index_from(struct index_state *istate, const 
char *path)
discard_index(split_index-base);
else
split_index-base = xcalloc(1, sizeof(*split_index-base));
+   split_index-base-keep_mmap = istate-keep_mmap;
ret = do_read_index(split_index-base,
git_path(sharedindex.%s,
 sha1_to_hex(split_index-base_sha1)), 1);
@@ -1618,6 +1625,10 @@ int discard_index(struct index_state *istate)
free(istate-cache);
istate-cache = NULL;
istate-cache_alloc = 0;
+   if (istate-keep_mmap  istate-mmap) {
+   munmap(istate-mmap, istate-mmap_size);
+   istate-mmap = NULL;
+   }
discard_split_index(istate);
return 0;
 }
-- 
1.9.1.346.ga2b5940

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

[PATCH 5/5] index-helper: add --detach

[Nguyễn Thái Ngọc Duy]

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 Documentation/git-index-helper.txt | 4 +++-
 index-helper.c | 6 +-
 2 files changed, 8 insertions(+), 2 deletions(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index afeb334..303cda0 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -8,7 +8,7 @@ git-index-helper - A simple cache server for speeding up index 
file access
 SYNOPSIS
 
 [verse]
-'git index-helper
+'git index-helper [--detach]
 
 DESCRIPTION
 ---
@@ -18,6 +18,8 @@ shared memory support (e.g. Linux or Windows)
 
 OPTIONS
 ---
+--detach::
+   Detach from the shell.
 
 GIT
 ---
diff --git a/index-helper.c b/index-helper.c
index f92326a..9420591 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -159,8 +159,9 @@ int main(int argc, char **argv)
static struct lock_file lock;
struct strbuf sb = STRBUF_INIT;
const char *prefix;
-   int fd;
+   int fd, detach = 0;
struct option options[] = {
+   OPT_BOOL(0, detach, detach, detach the process),
OPT_END()
};
 
@@ -188,6 +189,9 @@ int main(int argc, char **argv)
sigchain_push(SIGQUIT, cleanup_on_signal);
sigchain_push(SIGPIPE, cleanup_on_signal);
 
+   if (detach  daemonize(daemonized))
+   die_errno(unable to detach);
+
loop(sb.buf, 600);
strbuf_release(sb);
return 0;
-- 
1.9.1.346.ga2b5940

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

[PATCH 4/5] daemonize(): set a flag before exiting the main process

[Nguyễn Thái Ngọc Duy]

This allows signal handlers and atexit functions to realize this
situation and not clean up.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 builtin/gc.c | 2 +-
 cache.h  | 2 +-
 daemon.c | 2 +-
 setup.c  | 4 +++-
 4 files changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/gc.c b/builtin/gc.c
index 85f5c2b..50275af 100644
--- a/builtin/gc.c
+++ b/builtin/gc.c
@@ -325,7 +325,7 @@ int cmd_gc(int argc, const char **argv, const char *prefix)
 * failure to daemonize is ok, we'll continue
 * in foreground
 */
-   daemonize();
+   daemonize(NULL);
} else
add_repack_all_option();
 
diff --git a/cache.h b/cache.h
index f05e062..6f4b863 100644
--- a/cache.h
+++ b/cache.h
@@ -450,7 +450,7 @@ extern int set_git_dir_init(const char *git_dir, const char 
*real_git_dir, int);
 extern int init_db(const char *template_dir, unsigned int flags);
 
 extern void sanitize_stdfds(void);
-extern int daemonize(void);
+extern int daemonize(int *);
 
 #define alloc_nr(x) (((x)+16)*3/2)
 
diff --git a/daemon.c b/daemon.c
index eba1255..2650504 100644
--- a/daemon.c
+++ b/daemon.c
@@ -1311,7 +1311,7 @@ int main(int argc, char **argv)
return execute();
 
if (detach) {
-   if (daemonize())
+   if (daemonize(NULL))
die(--detach not supported on this platform);
} else
sanitize_stdfds();
diff --git a/setup.c b/setup.c
index 613e3b3..e8e129a 100644
--- a/setup.c
+++ b/setup.c
@@ -842,7 +842,7 @@ void sanitize_stdfds(void)
close(fd);
 }
 
-int daemonize(void)
+int daemonize(int *daemonized)
 {
 #ifdef NO_POSIX_GOODIES
errno = ENOSYS;
@@ -854,6 +854,8 @@ int daemonize(void)
case -1:
die_errno(fork failed);
default:
+   if (daemonized)
+   *daemonized = 1;
exit(0);
}
if (setsid() == -1)
-- 
1.9.1.346.ga2b5940

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

[PATCH 3/5] index-helper: add Windows support

[Nguyễn Thái Ngọc Duy]

Windows supports shared memory, but a bit different that POSIX
shm. The most noticeable thing is there's no way to get the shared
memory's size from the reader. So the size is added near the end in
the shared memory, hidden away from shm users (storing it in headers
would cause more problems with munmap, storing it as a separate shm is
even worse).

PostMessage is used instead of UNIX signals for
notification. Lightweight (at least code-wise) on the client side.

Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
---
 Documentation/git-index-helper.txt |  2 +-
 config.mak.uname   |  2 +
 index-helper.c | 49 +++
 read-cache.c   |  6 +++
 shm.c  | 96 ++
 5 files changed, 154 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-index-helper.txt 
b/Documentation/git-index-helper.txt
index d0b1365..afeb334 100644
--- a/Documentation/git-index-helper.txt
+++ b/Documentation/git-index-helper.txt
@@ -14,7 +14,7 @@ DESCRIPTION
 ---
 Keep the index file in memory for faster access. This daemon is per
 repository. This daemon is only available on POSIX system with
-shared memory support (e.g. Linux)
+shared memory support (e.g. Linux or Windows)
 
 OPTIONS
 ---
diff --git a/config.mak.uname b/config.mak.uname
index 8de61a4..17d35e3 100644
--- a/config.mak.uname
+++ b/config.mak.uname
@@ -376,6 +376,7 @@ ifndef DEBUG
 else
BASIC_CFLAGS += -Zi -MTd
 endif
+   PROGRAM_OBJS += index-helper.o
X = .exe
 endif
 ifeq ($(uname_S),Interix)
@@ -529,6 +530,7 @@ ifneq (,$(wildcard ../THIS_IS_MSYSGIT))
 else
NO_CURL = YesPlease
 endif
+   PROGRAM_OBJS += index-helper.o
 endif
 ifeq ($(uname_S),QNX)
COMPAT_CFLAGS += -DSA_RESTART=0
diff --git a/index-helper.c b/index-helper.c
index e10d0c3..f92326a 100644
--- a/index-helper.c
+++ b/index-helper.c
@@ -95,6 +95,52 @@ static void loop(const char *pid_file, int idle_in_seconds)
utime(git_path(read-cache--daemon.pid), NULL);
 }
 
+#elif defined(GIT_WINDOWS_NATIVE)
+
+static void loop(const char *pid_file, int idle_in_seconds)
+{
+   HWND hwnd;
+   UINT_PTR timer = 0;
+   MSG msg;
+   HINSTANCE hinst = GetModuleHandle(NULL);
+   WNDCLASS wc;
+
+   /*
+* Emulate UNIX signals by sending WM_USER+x to a
+* window. Register window class and create a new window to
+* catch these messages.
+*/
+   memset(wc, 0, sizeof(wc));
+   wc.lpfnWndProc   = DefWindowProc;
+   wc.hInstance = hinst;
+   wc.lpszClassName = git-read-cache--daemon;
+   if (!RegisterClass(wc))
+   die_errno(could not register new window class);
+
+   hwnd = CreateWindow(git-read-cache--daemon, pid_file,
+   0, 0, 0, 1, 1, NULL, NULL, hinst, NULL);
+   if (!hwnd)
+   die_errno(could not register new window);
+
+   refresh(0);
+   while (1) {
+   timer = SetTimer(hwnd, timer, idle_in_seconds * 1000, NULL);
+   if (!timer)
+   die(no timer!);
+   if (!GetMessage(msg, hwnd, 0, 0) || msg.message == WM_TIMER)
+   break;
+   switch (msg.message) {
+   case WM_USER:
+   refresh(0);
+   break;
+   default:
+   /* just reset the timer */
+   break;
+   }
+   utime(git_path(read-cache--daemon.pid), NULL);
+   }
+}
+
 #else
 
 static void loop(const char *pid_file, int idle_in_seconds)
@@ -129,6 +175,9 @@ int main(int argc, char **argv)
   git_path(read-cache--daemon.pid),
   LOCK_DIE_ON_ERROR);
strbuf_addf(sb,
+#ifdef GIT_WINDOWS_NATIVE
+   HWND
+#endif
% PRIuMAX, (uintmax_t) getpid());
write_in_full(fd, sb.buf, sb.len);
commit_lock_file(lock);
diff --git a/read-cache.c b/read-cache.c
index f9df984..8001c29 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1483,6 +1483,12 @@ static void poke_daemon(struct stat *st, int 
refresh_cache)
pid_t pid = strtoul(sb.buf, end, 10);
if (end  !*end)
kill(pid, refresh_cache ? SIGHUP : SIGUSR1);
+#elif defined(GIT_WINDOWS_NATIVE)
+   if (starts_with(sb.buf, HWND)) {
+   HWND hwnd = FindWindow(git-read-cache--daemon, 
sb.buf);
+   PostMessage(hwnd, refresh_cache ? WM_USER : WM_USER + 1,
+   0, 0);
+   }
 #endif
}
close(fd);
diff --git a/shm.c b/shm.c
index 4ec1a00..04d8a35 100644
--- a/shm.c
+++ b/shm.c
@@ -52,6 +52,102 @@ void git_shm_unlink(const char *fmt, ...)
shm_unlink(path);
 }
 
+#elif defined(GIT_WINDOWS_NATIVE)
+
+#define 

Re: [PATCH] git-submodule.sh: avoid test cond -a/-o cond

[Junio C Hamano]

Elia Pinto gitter.spi...@gmail.com writes:

 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.

 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---

 This is the fourth revision of this patch.

H.

When applied on top of 'master', this seems to break 7406; fails the
same way with either bash 4.2-2ubuntu2.1 which identifes itself as
4.2.25 or dash 0.5.7-2ubuntu2.

-- 8 --

t7406-submodule-update.sh .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 14/43 subtests

Test Summary Report
---
t7406-submodule-update.sh (Wstat: 256 Tests: 43 Failed: 14)
  Failed tests:  4-6, 10-15, 18, 30-33

-- 8 --

Which shell did you test this patch with?
--
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-submodule.sh: avoid test cond -a/-o cond

[Torsten Bögershausen]

On 2014-06-10 14.28, Elia Pinto wrote:
[]
   # before the first commit: compare with an empty tree
   head=$(git hash-object -w -t tree --stdin /dev/null)
 @@ -1056,13 +1056,17 @@ cmd_summary() {
   while read mod_src mod_dst sha1_src sha1_dst status sm_path
   do
   # Always show modules deleted or type-changed 
 (blob-module)
 - test $status = D -o $status = T  echo $sm_path  
 continue
 + case $status in
 + ([DT])
Does this look strange? ^
Should it be
case $status in
D|T)


--
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-submodule.sh: avoid test cond -a/-o cond

[Junio C Hamano]

Elia Pinto gitter.spi...@gmail.com writes:

 @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
   continue
   fi
  
 - if ! test -d $sm_path/.git -o -f $sm_path/.git
 + if ! test -d $sm_path/.git || test -f $sm_path/.git

Which part of test conditions does that ! apply in the original,
and in the updated? 

I think the new test after || also needs negation, 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: Git reset --hard with staged changes

[Pierre-François CLEMENT]

2014-06-10 1:28 GMT+02:00 Junio C Hamano gits...@pobox.com:
 Pierre-François CLEMENT lik...@gmail.com writes:

 Hm, I didn't think of git apply --index... Makes sense for this
 special use, but I'm not sure about the other use cases.

 Try merging another branch that tracks a file your current branch
 does not know about and ending up with conflicts during that merge.
 Resetting the half-done result away must remove that new path from
 your working tree and the index.

Hm I see. Even though the documentation doesn't make it very clear
about what happens to such files, it turns out the scenario we
stumbled upon seems to be the special use case after all. Thanks for
shedding some light on this :) I wonder why does git-reset's hard mode
not always remove untracked files then?
--
Pierre-François CLEMENT
Application developer at Upcast Social
--
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-submodule.sh: avoid test cond -a/-o cond

[Johannes Sixt]

Am 6/10/2014 16:55, schrieb Junio C Hamano:
 Elia Pinto gitter.spi...@gmail.com writes:
 
 @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
  continue
  fi
  
 -if ! test -d $sm_path/.git -o -f $sm_path/.git
 +if ! test -d $sm_path/.git || test -f $sm_path/.git
 
 Which part of test conditions does that ! apply in the original,
 and in the updated? 
 
 I think the new test after || also needs negation, no?

Not just that; the || must be turned into  as well.

I noticed a similar construct later in the patch in a review of an earlier
iteration, but missed this one.

-- Hannes
--
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-submodule.sh: avoid test cond -a/-o cond

[Junio C Hamano]

Torsten Bögershausen tbo...@web.de writes:

 On 2014-06-10 14.28, Elia Pinto wrote:
 []
  # before the first commit: compare with an empty tree
  head=$(git hash-object -w -t tree --stdin /dev/null)
 @@ -1056,13 +1056,17 @@ cmd_summary() {
  while read mod_src mod_dst sha1_src sha1_dst status sm_path
  do
  # Always show modules deleted or type-changed 
 (blob-module)
 -test $status = D -o $status = T  echo $sm_path  
 continue
 +case $status in
 +([DT])
 Does this look strange? ^
 Should it be
 case $status in
 D|T)

Actually POSIX allows matching parentheses for case arm labels
(surprise!).

And some shells misparse

var=$( ... case arm) action ;; esac ... )

as if the ')' after the arm label closes the whole command
substitution.

Having said that, I'd prefer to see the following squashed into that
patch.

The first hunk is purely a bugfix.  It used to be 

if ! test -d $sm_path/.git -o -f $sm_path/.git

that is: unless $sm_path/.git is directory or file, do this.
And the rewrite broke that logic.

The second hunk is to avoid case that confuses without helping
readability that much.

I would also have preferred to see the echo to printf substitution
left out of this patch.  There are other places where $sm_path is
echoed and fixing only one of them in an otherwise unrelated patch
feels wrong---it should be a separate follow-up patch, I would
think.

 git-submodule.sh | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d6a1dea..27ca7d5 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
continue
fi
 
-   if ! test -d $sm_path/.git || test -f $sm_path/.git
+   if ! test -d $sm_path/.git  ! test -f $sm_path/.git
then
module_clone $sm_path $name $url $reference 
$depth || exit
cloned_modules=$cloned_modules;$name
@@ -1056,11 +1056,11 @@ cmd_summary() {
while read mod_src mod_dst sha1_src sha1_dst status sm_path
do
# Always show modules deleted or type-changed 
(blob-module)
-   case $status in
-   ([DT])
-   printf '%s\n' $sm_path 
+   if test $status = D || test $status = T
+   then
+   printf '%s\n' $sm_path
continue
-   esac
+   fi
# Respect the ignore setting for --for-status.
if test -n $for_status
then
--
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 reset --hard with staged changes

[David Kastrup]

Pierre-François CLEMENT lik...@gmail.com writes:

 2014-06-10 1:28 GMT+02:00 Junio C Hamano gits...@pobox.com:
 Pierre-François CLEMENT lik...@gmail.com writes:

 Hm, I didn't think of git apply --index... Makes sense for this
 special use, but I'm not sure about the other use cases.

 Try merging another branch that tracks a file your current branch
 does not know about and ending up with conflicts during that merge.
 Resetting the half-done result away must remove that new path from
 your working tree and the index.

 Hm I see. Even though the documentation doesn't make it very clear
 about what happens to such files, it turns out the scenario we
 stumbled upon seems to be the special use case after all. Thanks for
 shedding some light on this :) I wonder why does git-reset's hard mode
 not always remove untracked files then?

Because it never removes them?  Git only removes files once it tracks
them.  This includes the operation of removing _and_ untracking them,
like with git reset --hard.

The only command which explicitly messes with untracked files is
git-clean.

-- 
David Kastrup
--
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-submodule.sh: avoid test cond -a/-o cond

[Junio C Hamano]

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

 Torsten Bögershausen tbo...@web.de writes:

 On 2014-06-10 14.28, Elia Pinto wrote:
 []
 # before the first commit: compare with an empty tree
 head=$(git hash-object -w -t tree --stdin /dev/null)
 @@ -1056,13 +1056,17 @@ cmd_summary() {
 while read mod_src mod_dst sha1_src sha1_dst status sm_path
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 -   test $status = D -o $status = T  echo $sm_path  
 continue
 +   case $status in
 +   ([DT])
 Does this look strange? ^
 Should it be
 case $status in
 D|T)

 Actually POSIX allows matching parentheses for case arm labels
 (surprise!).

 And some shells misparse

   var=$( ... case arm) action ;; esac ... )

 as if the ')' after the arm label closes the whole command
 substitution.

 Having said that, I'd prefer to see the following squashed into that
 patch.
 ...
 I would also have preferred to see the echo to printf substitution
 left out of this patch.  There are other places where $sm_path is
 echoed and fixing only one of them in an otherwise unrelated patch
 feels wrong---it should be a separate follow-up patch, I would
 think.

... which may look like this (after removing s/echo/printf/ in that
hunk from this test -a/-o patch).

 git-submodule.sh | 16 
 1 file changed, 8 insertions(+), 8 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index d0d9b58..9245abf 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -235,7 +235,7 @@ module_name()
sed -n -e 's|^submodule\.\(.*\)\.path '$re'$|\1|p' )
test -z $name 
die $(eval_gettext No submodule mapping found in .gitmodules for path 
'\$sm_path')
-   echo $name
+   printf '%s\n' $name
 }
 
 #
@@ -305,10 +305,10 @@ module_clone()
b=${b%/}
 
# Turn each leading */ component into ../
-   rel=$(echo $b | sed -e 's|[^/][^/]*|..|g')
-   echo gitdir: $rel/$a $sm_path/.git
+   rel=$(printf '%s\n' $b | sed -e 's|[^/][^/]*|..|g')
+   printf '%s\n' gitdir: $rel/$a $sm_path/.git
 
-   rel=$(echo $a | sed -e 's|[^/][^/]*|..|g')
+   rel=$(printf '%s\n' $a | sed -e 's|[^/][^/]*|..|g')
(clear_local_git_env; cd $sm_path  GIT_WORK_TREE=. git config 
core.worktree $rel/$b)
 }
 
@@ -389,7 +389,7 @@ cmd_add()
sm_path=$2
 
if test -z $sm_path; then
-   sm_path=$(echo $repo |
+   sm_path=$(printf '%s\n' $repo |
sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
fi
 
@@ -1058,7 +1058,7 @@ cmd_summary() {
# Always show modules deleted or type-changed 
(blob-module)
if test $status = D || test $status = T
then
-   echo $sm_path
+   printf '%s\n' $sm_path
continue
fi
# Respect the ignore setting for --for-status.
@@ -1070,7 +1070,7 @@ cmd_summary() {
fi
# Also show added or modified modules which are checked 
out
GIT_DIR=$sm_path/.git git-rev-parse --git-dir 
/dev/null 21 
-   echo $sm_path
+   printf '%s\n' $sm_path
done
)
 
@@ -1311,7 +1311,7 @@ cmd_sync()
./*|../*)
# rewrite foo/bar as ../.. to find path from
# submodule work tree to superproject work tree
-   up_path=$(echo $sm_path | sed s/[^/][^/]*/../g) 
+   up_path=$(printf '%s\n' $sm_path | sed 
s/[^/][^/]*/../g) 
# guarantee a trailing /
up_path=${up_path%/}/ 
# path from submodule work tree to submodule origin repo


--
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-submodule.sh: avoid test cond -a/-o cond

[Junio C Hamano]

Johannes Sixt j.s...@viscovery.net writes:

 Am 6/10/2014 16:55, schrieb Junio C Hamano:
 Elia Pinto gitter.spi...@gmail.com writes:
 
 @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
 continue
 fi
  
 -   if ! test -d $sm_path/.git -o -f $sm_path/.git
 +   if ! test -d $sm_path/.git || test -f $sm_path/.git
 
 Which part of test conditions does that ! apply in the original,
 and in the updated? 
 
 I think the new test after || also needs negation, no?

 Not just that; the || must be turned into  as well.

I think our mails crossed ;-)
--
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] t9001: avoid not portable '\n' with sed

[Junio C Hamano]

Torsten Bögershausen tbo...@web.de writes:

 I think that V3 explains the difference between POSIX sed and
 gnu sed much better, and does reflect all the comments from
 the list, which otherwise may be lost.

Too late for that as the patch is already in 'next' X-.


--
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 12/15] use get_commit_buffer everywhere

[Junio C Hamano]

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

 I agree it's not right, though. I think the original is questionable,
 too. It takes a pointer into the middle of partial_commit-buffer and
 attaches it to a strbuf. That's wrong because:

   1. It's pointing into the middle of an allocated buffer, not the
  beginning.

   2. We do not own partial_commit-buffer in the first place.

 So any call to strbuf_detach on the result would be disastrous.

You are right.  Where did this original crap come from X-...

 ... and it
 doesn't cause a bug in practice because the only use of the strbuf is to
 pass it as a const to create_notes_commit.

 I feel like the most elegant solution is for create_notes_commit to take
 a buf/len pair rather than a strbuf, but it unfortunately is just
 feeding that to commit_tree. Adjusting that code path would affect quite
 a few other spots.

 The other obvious option is actually populating the strbuf, but it feels
 ugly to have to make a copy just to satisfy the function interface.

 Maybe a cast and a warning comment are the least evil thing, as below? I
 dunno, it feels pretty wrong.

Yeah, it does feel wrong wrong wrong.  Perhaps this big comment
would serve as a marker for a low-hanging fruit for somebody else to
fix it, e.g. by using strbuf-add to make a copy, which would be the
easiest and safest workaround?

 diff --git a/notes-merge.c b/notes-merge.c
 index 94a1a8a..1f3b309 100644
 --- a/notes-merge.c
 +++ b/notes-merge.c
 @@ -671,7 +671,7 @@ int notes_merge_commit(struct notes_merge_options *o,
   DIR *dir;
   struct dirent *e;
   struct strbuf path = STRBUF_INIT;
 - char *msg = strstr(partial_commit-buffer, \n\n);
 + const char *msg = strstr(partial_commit-buffer, \n\n);
   struct strbuf sb_msg = STRBUF_INIT;
   int baselen;
  
 @@ -719,7 +719,15 @@ int notes_merge_commit(struct notes_merge_options *o,
   strbuf_setlen(path, baselen);
   }
  
 - strbuf_attach(sb_msg, msg, strlen(msg), strlen(msg) + 1);
 + /*
 +  * This is a bit tricky. We should not be attaching msg, which
 +  * is not owned by us and is not even the start of a heap buffer, to a
 +  * strbuf. But the create_notes_commit interface really wants
 +  * a strbuf, even though it will only ever use it as a buf/len pair and
 +  * never modify it. So this is tentatively safe as long as nobody ever
 +  * modifies, detaches, or releases the strbuf.
 +  */
 + strbuf_attach(sb_msg, (char *)msg, strlen(msg), strlen(msg) + 1);
   create_notes_commit(partial_tree, partial_commit-parents, sb_msg,
   result_sha1);
   if (o-verbosity = 4)

 I'm still confused and disturbed that my gcc is not noticing this
 obvious const violation. Hmm, shutting off ccache seems to make it work.
 Doubly disturbing.

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

Re: Git reset --hard with staged changes

[Pierre-François CLEMENT]

2014-06-10 17:27 GMT+02:00 David Kastrup d...@gnu.org:
 Pierre-François CLEMENT lik...@gmail.com writes:

 2014-06-10 1:28 GMT+02:00 Junio C Hamano gits...@pobox.com:
 Pierre-François CLEMENT lik...@gmail.com writes:

 Hm, I didn't think of git apply --index... Makes sense for this
 special use, but I'm not sure about the other use cases.

 Try merging another branch that tracks a file your current branch
 does not know about and ending up with conflicts during that merge.
 Resetting the half-done result away must remove that new path from
 your working tree and the index.

 Hm I see. Even though the documentation doesn't make it very clear
 about what happens to such files, it turns out the scenario we
 stumbled upon seems to be the special use case after all. Thanks for
 shedding some light on this :) I wonder why does git-reset's hard mode
 not always remove untracked files then?

 Because it never removes them?  Git only removes files once it tracks
 them.  This includes the operation of removing _and_ untracking them,
 like with git reset --hard.

 The only command which explicitly messes with untracked files is
 git-clean.

 --
 David Kastrup

Yeah sorry, I just noticed the emails on the definition of what are
(un)tracked files
(http://thread.gmane.org/gmane.comp.version-control.git/251071/focus=251151),
as I didn't get them in my inbox for some reason. So staged files
which aren't in HEAD are also considered tracked -- which explains it
all. Someone told me that too on the Git for human beings Google
Group, but I couldn't find a definition that backs this in the man
pages (maybe the git-glossary would be a good place for it?), and the
one from the Git-Scm book only confused me in thinking the opposite.
Thanks for the clarification

--
Pierre-François CLEMENT
Application developer at Upcast Social
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-submodule.sh: avoid test cond -a/-o cond

[Elia Pinto]

The construct is error-prone; test being built-in in most modern
shells, the reason to avoid test cond  test cond spawning
one extra process by using a single test cond -a cond no
longer exists.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
This is the fifth revision.

Change based on Junio bugfix and better rewrite of the case condition
http://permalink.gmane.org/gmane.comp.version-control.git/251198

I dropped also the echo - printf replacement for doing
it in another patch.

Pass all the t/*submodule* tests. Finally ! :=)

Thank you all very much and sorry for the mess.

 git-submodule.sh |   32 
 1 file changed, 20 insertions(+), 12 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..e128a4a 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -393,7 +393,7 @@ cmd_add()
sed -e 's|/$||' -e 's|:*/*\.git$||' -e 's|.*[/:]||g')
fi
 
-   if test -z $repo -o -z $sm_path; then
+   if test -z $repo || test -z $sm_path; then
usage
fi
 
@@ -450,7 +450,7 @@ Use -f if you really want to add it. 2
# perhaps the path exists and is already a git repo, else clone it
if test -e $sm_path
then
-   if test -d $sm_path/.git -o -f $sm_path/.git
+   if test -d $sm_path/.git || test -f $sm_path/.git
then
eval_gettextln Adding existing repo at '\$sm_path' to 
the index
else
@@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
continue
fi
 
-   if ! test -d $sm_path/.git -o -f $sm_path/.git
+   if ! test -d $sm_path/.git  test -f $sm_path/.git
then
module_clone $sm_path $name $url $reference 
$depth || exit
cloned_modules=$cloned_modules;$name
@@ -857,11 +857,11 @@ Maybe you want to use 'update --init'?)
die $(eval_gettext Unable to find current 
${remote_name}/${branch} revision in submodule path '\$sm_path')
fi
 
-   if test $subsha1 != $sha1 -o -n $force
+   if test $subsha1 != $sha1 || test -n $force
then
subforce=$force
# If we don't already have a -f flag and the submodule 
has never been checked out
-   if test -z $subsha1 -a -z $force
+   if test -z $subsha1  test -z $force
then
subforce=-f
fi
@@ -1031,7 +1031,7 @@ cmd_summary() {
then
head=$rev
test $# = 0 || shift
-   elif test -z $1 -o $1 = HEAD
+   elif test -z $1 || test $1 = HEAD
then
# before the first commit: compare with an empty tree
head=$(git hash-object -w -t tree --stdin /dev/null)
@@ -1056,13 +1056,17 @@ cmd_summary() {
while read mod_src mod_dst sha1_src sha1_dst status sm_path
do
# Always show modules deleted or type-changed 
(blob-module)
-   test $status = D -o $status = T  echo $sm_path  
continue
+   if test $status = D || test $status = T
+then
+   echo $sm_path 
+   continue
+   fi
# Respect the ignore setting for --for-status.
if test -n $for_status
then
name=$(module_name $sm_path)
ignore_config=$(get_submodule_config $name 
ignore none)
-   test $status != A -a $ignore_config = all  
continue
+   test $status != A  test $ignore_config = all 
 continue
fi
# Also show added or modified modules which are checked 
out
GIT_DIR=$sm_path/.git git-rev-parse --git-dir 
/dev/null 21 
@@ -1122,7 +1126,7 @@ cmd_summary() {
*)
errmsg=
total_commits=$(
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
range=$sha1_src...$sha1_dst
elif test $mod_src = 16
@@ -1159,7 +1163,7 @@ cmd_summary() {
# i.e. deleted or changed to blob
test $mod_dst = 16  echo $errmsg
else
-   if test $mod_src = 16 -a $mod_dst = 16
+   if test $mod_src = 16  test $mod_dst = 16
then
limit=

Re: [PATCH] git-submodule.sh: avoid test cond -a/-o cond

[Junio C Hamano]

Elia Pinto gitter.spi...@gmail.com writes:

 @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
   continue
   fi
  
 - if ! test -d $sm_path/.git -o -f $sm_path/.git
 + if ! test -d $sm_path/.git  test -f $sm_path/.git

H.  Is the above correct?

$ if ! false  true; then echo true; else echo false; fi
true

In other words, ! cmd1  cmd2 parses not as ! (cmd1  cmd2)
but as (! cmd1)  cmd2.

It almost makes me wonder if the code may become simpler if we did
it the way in the attached.  That is, if $sm_path/.git is there
(whether as an embedded repository, or a file pointing to a
repository elsewhere), then grab its HEAD, otherwise $sm_path is a
submodule that hasn't been run 'submodule init' on, so run the whole
nine yards starting from module_clone.

 git-submodule.sh | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..018f1bb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -832,15 +832,15 @@ Maybe you want to use 'update --init'?)
continue
fi
 
-   if ! test -d $sm_path/.git -o -f $sm_path/.git
+   if test -e $sm_path/.git
then
-   module_clone $sm_path $name $url $reference 
$depth || exit
-   cloned_modules=$cloned_modules;$name
-   subsha1=
-   else
subsha1=$(clear_local_git_env; cd $sm_path 
git rev-parse --verify HEAD) ||
die $(eval_gettext Unable to find current revision in 
submodule path '\$displaypath')
+   else
+   module_clone $sm_path $name $url $reference 
$depth || exit
+   cloned_modules=$cloned_modules;$name
+   subsha1=
fi
 
if test -n $remote
--
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-submodule.sh: avoid test cond -a/-o cond

[Junio C Hamano]

Elia Pinto gitter.spi...@gmail.com writes:

 @@ -832,7 +832,7 @@ Maybe you want to use 'update --init'?)
   continue
   fi
  
 - if ! test -d $sm_path/.git -o -f $sm_path/.git
 + if ! test -d $sm_path/.git  test -f $sm_path/.git

H.  Is the above correct?

$ if ! false  true; then echo true; else echo false; fi
true

In other words, ! cmd1  cmd2 parses not as ! (cmd1  cmd2)
but as (! cmd1)  cmd2.

It almost makes me wonder if the code may become simpler if we did
it the way in the attached.  That is, if $sm_path/.git is there
(whether as an embedded repository, or a file pointing to a
repository elsewhere), then grab its HEAD, otherwise $sm_path is a
submodule that hasn't been run 'submodule init' on, so run the whole
nine yards starting from module_clone.

Such a rewrite is not within the scope of this series, so 

if ! test -d $sm_path/.git  ! test -f $sm_path/.git

may be the closest to the original intent, I would think.

 git-submodule.sh | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/git-submodule.sh b/git-submodule.sh
index e146b83..018f1bb 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -832,15 +832,15 @@ Maybe you want to use 'update --init'?)
continue
fi
 
-   if ! test -d $sm_path/.git -o -f $sm_path/.git
+   if test -e $sm_path/.git
then
-   module_clone $sm_path $name $url $reference 
$depth || exit
-   cloned_modules=$cloned_modules;$name
-   subsha1=
-   else
subsha1=$(clear_local_git_env; cd $sm_path 
git rev-parse --verify HEAD) ||
die $(eval_gettext Unable to find current revision in 
submodule path '\$displaypath')
+   else
+   module_clone $sm_path $name $url $reference 
$depth || exit
+   cloned_modules=$cloned_modules;$name
+   subsha1=
fi
 
if test -n $remote
--
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: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Junio C Hamano]

Fabian Ruch baf...@gmail.com writes:

 On 05/27/2014 08:42 PM, Junio C Hamano wrote:
 Fabian Ruch baf...@gmail.com writes:
 [..]

 In order to signal the three possible situations (not only success and
 failure to complete) after a pick through porcelain commands such as
 `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
 chosen in line with the other situations in which the sequencer
 encounters an error.
 
 Hmph... do we still pass negative to exit() anywhere in our codebase?

 No, but I thought `cmd_cherry_pick` would just forward the `return -1` from 
 the
 sequencer to the shell. I had another look and found that `cmd_cherry_pick`
 calls `die` instead. Now, the commit inserts 128 as exit status in
 `fast_forward_to`.

 Would it be appropriate to mention the choice of exit status in the coding
 guidelines? I didn't know that the int-argument to exit(3) gets truncated and
 that this is why it is a general rule to only use values in the range from 0 
 to
 255 with exit(3).

I personally do not think of a reason why it is necessary to mention
how the argument to exit(3) is expected to be used by the system, but
if it is common not to know it, I guess it would not hurt to have a
single paragraph with at most two lines.

In any case, I agree that exiting with 1 that signals failed with
conflict can be confusing to the caller.  Can we have a test to
demonstrate when this fix matters?

Thanks.

 -- 8 --
 Subject: sequencer: Signal failed ff as an aborted, not a conflicted merge

 `do_pick_commit` handles three situations if it is not fast-forwarding.
 In order for `do_pick_commit` to identify the situation, it examines the
 return value of the selected merge command.

 1. return value 0 stands for a clean merge
 2. 1 is passed in case of a failed merge due to conflict
 3. any other return value means that the merge did not even start

 So far, the sequencer returns 1 in case of a failed fast-forward, which
 would mean failed merge due to conflict. However, a fast-forward
 either starts and succeeds or does not start at all. In particular, it
 cannot fail in between like a merge with a dirty index due to conflicts.

 In order to signal the three possible situations (not only success and
 failure to complete) after a pick through porcelain commands such as
 `cherry-pick`, exit with a return value that is neither 0 nor 1. 128 was
 chosen in line with the other situations in which the sequencer
 encounters an error. In such situations, the sequencer returns a
 negative value and `cherry-pick` translates this into a call to `die`.
 `die` then terminates the process with exit status 128.

 Signed-off-by: Fabian Ruch baf...@gmail.com
 ---
  sequencer.c | 2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

 diff --git a/sequencer.c b/sequencer.c
 index 90cac7b..225afcb 100644
 --- a/sequencer.c
 +++ b/sequencer.c
 @@ -278,7 +278,7 @@ static int fast_forward_to(const unsigned char *to, const 
 unsigned char *from,
  
   read_cache();
   if (checkout_fast_forward(from, to, 1))
 - exit(1); /* the callee should have complained already */
 + exit(128); /* the callee should have complained already */
   ref_lock = lock_any_ref_for_update(HEAD, unborn ? null_sha1 : from,
  0, NULL);
   strbuf_addf(sb, %s: fast-forward, action_name(opts));
--
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 03/20] contrib/examples/git-commit.sh: avoid test cond -a/-o cond

[David Aguilar]

On Fri, Jun 06, 2014 at 07:55:46AM -0700, Elia Pinto wrote:
 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.
 
 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
  contrib/examples/git-commit.sh |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/contrib/examples/git-commit.sh b/contrib/examples/git-commit.sh
 index 5cafe2e..934505b 100755
 --- a/contrib/examples/git-commit.sh
 +++ b/contrib/examples/git-commit.sh
 @@ -51,7 +51,7 @@ run_status () {
   export GIT_INDEX_FILE
   fi
  
 - if test $status_only = t -o $use_status_color = t; then
 + if test $status_only = t || test $use_status_color = t; then
   color=
   else
   color=--nocolor

It might be worth moving the then to the next line so that it's
consistent with the preferred sh style and with the rest of the script.

If we do that then there's one less line that would need to be touched
by a future style-fix patch.

 @@ -296,7 +296,7 @@ t,,,[1-9]*)
   die No paths with -i does not make sense. ;;
  esac
  
 -if test ! -z $templatefile -a -z $log_given
 +if test ! -z $templatefile  test -z $log_given
  then
   if test ! -f $templatefile
   then
 -- 
 1.7.10.4

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

Re: [PATCH 05/20] contrib/examples/git-repack.sh: avoid test cond -a/-o cond

[David Aguilar]

On Fri, Jun 06, 2014 at 07:55:48AM -0700, Elia Pinto wrote:
 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.
 
 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
  contrib/examples/git-repack.sh |4 ++--
  1 file changed, 2 insertions(+), 2 deletions(-)
 
 diff --git a/contrib/examples/git-repack.sh b/contrib/examples/git-repack.sh
 index f312405..96e3fed 100755
 --- a/contrib/examples/git-repack.sh
 +++ b/contrib/examples/git-repack.sh
 @@ -76,8 +76,8 @@ case ,$all_into_one, in
   existing=$existing $e
   fi
   done
 - if test -n $existing -a -n $unpack_unreachable -a \
 - -n $remove_redundant
 + if test -n $existing  test -n $unpack_unreachable  \
 + test -n $remove_redundant
   then

I do not think the traling \ is needed anymore; the  takes care of it.
-- 
David
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 09/20] git-rebase--interactive.sh: avoid test cond -a/-o cond

[David Aguilar]

On Fri, Jun 06, 2014 at 07:55:52AM -0700, Elia Pinto wrote:
 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.
 
 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
  git-rebase--interactive.sh |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
 index 6ec9d3c..797571f 100644
 --- a/git-rebase--interactive.sh
 +++ b/git-rebase--interactive.sh
 @@ -1013,7 +1013,7 @@ then
   git rev-list $revisions |
   while read rev
   do
 - if test -f $rewritten/$rev -a $(sane_grep $rev 
 $state_dir/not-cherry-picks) = 
 + if test -f $rewritten/$rev  test $(sane_grep $rev 
 $state_dir/not-cherry-picks) = 
   then

This line is getting pretty long.
I wonder whether it should be wrapped at the  to keep it shorter?

Also, it looks like the last half of the expression can be simplified to,

test -z $(sane_grep $rev $state_dir/not-cherry-picks)

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

Re: [PATCH 2/5] index-helper: new daemon for caching index and related stuff

[David Turner]

On Tue, 2014-06-10 at 20:24 +0700, Nguyễn Thái Ngọc Duy wrote:

 + loop(sb.buf, 600);
...
 + if (st-st_mtime + 600  time(NULL))

s/600/INDEX_HELPER_TIMEOUT/ or something.

 + return;   /* don't try to read from stale .pid file */
 +
 + fd = open(git_path(read-cache--daemon.pid), O_RDONLY);

should be index-helper.pid (actually, there are a few other instances of
this as well); maybe make it a constant.


--
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 t0001: test git init when run via an alias

[Junio C Hamano]

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

 Commit 4ad8332 (t0001: test git init when run via an alias -
 2010-11-26) noted breakages when running init via alias. The problem
 is for alias to be used, $GIT_DIR must be searched, but 'init' and
 'clone' are not happy with that. So we start a new process like an
 external command, with clean environment in this case. Env variables
 that are set by command line (e.g. git --git-dir=.. ) are kept.

 This should also fix autocorrecting a command typo to init because
 it's the same problem: aliases are read, then init is unhappy with
 $GIT_DIR already set up because of that.

 Reminded-by: David Turner dtur...@twopensource.com
 Signed-off-by: Nguyễn Thái Ngọc Duy pclo...@gmail.com
 ---
  git.c   | 52 

This goes far deeper than Fix t0001, doesn't it?

  t/t0001-init.sh |  4 ++--
  2 files changed, 50 insertions(+), 6 deletions(-)

 diff --git a/git.c b/git.c
 index 7780572..d1e49da 100644
 --- a/git.c
 +++ b/git.c
 @@ -484,6 +521,10 @@ static void handle_builtin(int argc, const char **argv)
   struct cmd_struct *p = commands+i;
   if (strcmp(p-cmd, cmd))
   continue;
 + if (saved_environment  (p-option  NO_SETUP)) {
 + restore_env();
 + break;
 + }
   exit(run_builtin(p, argc, argv));
   }
  }
 @@ -539,7 +580,10 @@ static int run_argv(int *argcp, const char ***argv)
* of overriding git log with git show by having
* alias.log = show
*/
 - if (done_alias || !handle_alias(argcp, argv))
 + if (done_alias)
 + break;
 + save_env();
 + if (!handle_alias(argcp, argv))
   break;
   done_alias = 1;
   }

So the save-env kicks in only after we tried the builtins and the
externals and didn't find any, and before expanding the alias (which
has to look at the config, which means we need to do discovery and
may contaminate the environment and the globals), and then when we
retry with the expanded alias, we restore when we know the command
will misbehave if we didn't do so?

That does not sound so bad.  Even though I wonder if that save and
then restore sequence logically belongs around handle_alias(), you
would not have sufficient clue to let you cheat by not restoring the
environment for commands that you happen to know that they do not
care, so that may be a reasonable optimization.

Is it too brittle a solution to force people to mark problematic
subcommands with NO_SETUP, though?  What kind of change to a
subcommand that currently does not have to be marked with NO_SETUP
would make it necessary to mark it with NO_SETUP?

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 15/20] t/t5000-tar-tree.sh: avoid test cond -a/-o cond

[David Aguilar]

On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:
 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.
 
 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
  t/t5000-tar-tree.sh |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
 index 74fc5a8..ad6fa0d 100755
 --- a/t/t5000-tar-tree.sh
 +++ b/t/t5000-tar-tree.sh
 @@ -72,7 +72,7 @@ check_tar() {
   for header in *.paxheader
   do
   data=${header%.paxheader}.data 
 - if test -h $data -o -e $data
 + if test -h $data || test -e $data
   then

This looks okay, but it raises a question for the original author
(René, I think that's you so I've added you to the To: line).

Should that be test -f instead of test -e?

This is a very minor note and should not block this patch.
-- 
David
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Phil Hord]

On 06/10/2014 01:56 PM, Junio C Hamano wrote:
 Fabian Ruch baf...@gmail.com writes:

 On 05/27/2014 08:42 PM, Junio C Hamano wrote:
 Fabian Ruch baf...@gmail.com writes:
 [..]

 In order to signal the three possible situations (not only success and
 failure to complete) after a pick through porcelain commands such as
 `cherry-pick`, exit with a return value that is neither 0 nor 1. -1 was
 chosen in line with the other situations in which the sequencer
 encounters an error.
 Hmph... do we still pass negative to exit() anywhere in our codebase?
 No, but I thought `cmd_cherry_pick` would just forward the `return -1` from 
 the
 sequencer to the shell. I had another look and found that `cmd_cherry_pick`
 calls `die` instead. Now, the commit inserts 128 as exit status in
 `fast_forward_to`.

 Would it be appropriate to mention the choice of exit status in the coding
 guidelines? I didn't know that the int-argument to exit(3) gets truncated and
 that this is why it is a general rule to only use values in the range from 0 
 to
 255 with exit(3).
 I personally do not think of a reason why it is necessary to mention
 how the argument to exit(3) is expected to be used by the system, but
 if it is common not to know it, I guess it would not hurt to have a
 single paragraph with at most two lines.

 In any case, I agree that exiting with 1 that signals failed with
 conflict can be confusing to the caller.  Can we have a test to
 demonstrate when this fix matters?

I think you are asking for a test and not for clarification.  But a test
was provided in 3/3 in this series.  Was it not related directly enough?

For clarification, this tri-state return value matters when the caller
is planning to do some cleanup and needs to handle the fallout
correctly.  Maybe changing this return value is not the correct way
forward, though.  It might be better if the caller could examine the
result after-the-fact instead.  This would require some reliable state
functions which I recall were somewhat scattered last time I looked. 
Also I cannot think of a reliable test for the previous cherry-pick
failed during pre-condition checks and I'm not sure anything should
exist to track this in .git outside of the return value for this function. 

--
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: Disk waste with packs and .keep files

[Jeff King]

On Tue, Jun 10, 2014 at 10:21:03AM +0200, Matthieu Moy wrote:

 Since a few weeks however, Git started wasting my disk space: instead of
 creating small .pack files next to the big .keep-ed pack files, it seems
 to create redundant, big .pack files (i.e. I get N pack files of similar
 size). git verify-pack confirms that, for example, the object
 corresponding to the root commit is contained in each of the .pack file.

 I don't have a reproducible way to get the situation so I didn't bisect,
 but git log --grep .keep points me to this which seems related:
 
   commit ee34a2beadb94a9595f09af719e3c09b485ca797
   Author: Jeff King p...@peff.net
   Date:   Mon Mar 3 15:04:20 2014 -0500
 
 repack: add `repack.packKeptObjects` config var

Eek. Does anybody have a brown paper bag I can borrow?

-- 8 --
Subject: repack: do not accidentally pack kept objects by default

Commit ee34a2b (repack: add `repack.packKeptObjects` config
var, 2014-03-03) added a flag which could duplicate kept
objects, but did not mean to turn it on by default. Instead,
the option is tied by default to the decision to write
bitmaps, like:

  if (pack_kept_objects  0)
  pack_kept_objects = write_bitmap;

after which we expect pack_kept_objects to be a boolean 0 or
1.  However, that assignment neglects that write_bitmap is
_also_ a tri-state with -1 as the default, and with
neither option given, we accidentally turn the option on.

This patch is the minimal fix to restore the desired
behavior for the default state. However, the real fix will
be more involved.

The decision to turn on bitmaps via config is actually made
in pack-objects itself (which is why we need write_bitmap as
a tri-state here; we only pass the override option if the
user gave us a command-line option). To tie the options
together correctly, we need to either pass the don't know
tristate down to pack-objects (which would also read
repack.packKeptObjects), or pull the reading of
pack.writebitmaps up to the repack level.

Signed-off-by: Jeff King p...@peff.net
---
I think the latter makes the most sense, and it was a mistake to read
the option in pack-objects in the first place. We _never_ want to
write bitmaps when packing to stdout, or even when doing a non-complete
repack. We had to teach pack-objects special logic to turn bitmaps off
in that case, but the right solution instead is that pack-objects should
always respect the --write-bitmap-index flag on the command line, and
the callers should drive that decision (and really only repack -[aA]
would want to use it). And then the fix here will just come out
naturally from that.

I'll work up a series, but we may want to fast-track this patch for
maint. It's a fairly big regression in v2.0. We didn't notice because
it's only an optimization issue, not a correctness one, and I guess not
that many people use .keep packs.

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

diff --git a/builtin/repack.c b/builtin/repack.c
index 6b0b62d..17bc8da 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
git_repack_usage, 0);
 
if (pack_kept_objects  0)
-   pack_kept_objects = write_bitmap;
+   pack_kept_objects = write_bitmap  0;
 
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
-- 
2.0.0.729.g520999f

--
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 t0001: test git init when run via an alias

[Junio C Hamano]

I'd squash this in, though.

 git.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/git.c b/git.c
index 6bb2043..9bfa8fb 100644
--- a/git.c
+++ b/git.c
@@ -36,7 +36,8 @@ static void save_env(void)
if (saved_environment)
return;
saved_environment = 1;
-   getcwd(orig_cwd, sizeof(orig_cwd));
+   if (getcwd(orig_cwd, sizeof(orig_cwd)))
+   die_errno(cannot getcwd);
for (i = 0; i  ARRAY_SIZE(env_names); i++) {
orig_env[i] = getenv(env_names[i]);
if (orig_env[i])
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

git rebase --skip stuck in a loop

[Phillip Susi]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA1

I'm in the middle of a long rebase and have had no trouble with git rebase 
--skip several times, but now it has become stuck:

psusi@devserv:~/parted.git$ git rebase --skip
Auto-merging libparted/arch/linux.c
CONFLICT (content): Merge conflict in libparted/arch/linux.c

When you have resolved this problem, run git rebase --continue.
If you prefer to skip this patch, run git rebase --skip instead.
To check out the original branch and stop rebasing, run git rebase --abort.

psusi@devserv:~/parted.git$ cat .git/rebase-merge/msgnum
17

Each time I try to skip, it just keeps trying to reapply this one patch.  Any 
ideas?

git version 1.9.1

-BEGIN PGP SIGNATURE-
Version: GnuPG v2.0.17 (MingW32)
Comment: Using GnuPG with Thunderbird - http://www.enigmail.net/

iQEcBAEBAgAGBQJTl1UcAAoJEI5FoCIzSKrwwHIH/38Cm19zg+37zgckLiy/3GhN
3Gmil5kX+3KkIHCxlPz3Ti3xCA5baM7tDzFdDIKcx8N/i8oALgWeAWf1Euy9Ww1K
3etIAMKzO463kmV7UbgSbLz5DpYWSaGo9WiYAC7xklhQV94w1Ainx5Lo4kRv1Wfj
R9TpQgViFnW2gNJv1zw0qHLXk1/h88LlAQsBaaY6I4f/DOLhAte7rGinkTgZtjmo
G/9PUMudQcehG65ITPlNLtoFsM8UHadNMLwJts/B7Yq23XNyRF50IaT8c1A/irSU
mfYqdHCho3D4kq+k0u+t0Z0bj6pfvo4b0khLafrYLrWGHC5K+Z3lE63ysJ/Mdj8=
=LZ9q
-END PGP SIGNATURE-
--
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 t0001: test git init when run via an alias

[Junio C Hamano]

Oops; obviously the check must be for NULL-ness, i.e.

 if (!getcwd(...))
   die_errno(...);


On Tue, Jun 10, 2014 at 11:53 AM, Junio C Hamano gits...@pobox.com wrote:
 I'd squash this in, though.

  git.c | 3 ++-
  1 file changed, 2 insertions(+), 1 deletion(-)

 diff --git a/git.c b/git.c
 index 6bb2043..9bfa8fb 100644
 --- a/git.c
 +++ b/git.c
 @@ -36,7 +36,8 @@ static void save_env(void)
 if (saved_environment)
 return;
 saved_environment = 1;
 -   getcwd(orig_cwd, sizeof(orig_cwd));
 +   if (getcwd(orig_cwd, sizeof(orig_cwd)))
 +   die_errno(cannot getcwd);
 for (i = 0; i  ARRAY_SIZE(env_names); i++) {
 orig_env[i] = getenv(env_names[i]);
 if (orig_env[i])
--
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: [RFC 1/3] sequencer: Signal failed ff as an aborted, not a conflicted merge

[Junio C Hamano]

Phil Hord ho...@cisco.com writes:

 In any case, I agree that exiting with 1 that signals failed with
 conflict can be confusing to the caller.  Can we have a test to
 demonstrate when this fix matters?

 I think you are asking for a test and not for clarification.  But a test
 was provided in 3/3 in this series.  Was it not related directly enough?

X-  Somehow I missed the 3 in 1/3 above and did not look beyond
this first patch.

 For clarification, this tri-state return value matters when the caller
 is planning to do some cleanup and needs to handle the fallout
 correctly.  Maybe changing this return value is not the correct way
 forward, though.  It might be better if the caller could examine the
 result after-the-fact instead.

I am not sure about that.  For merge strategies exit with 1 iff you
left the conflict in the index is the contract between git merge
frontend and the strategies backend; if a similar contract is needed
between the sequencer and its users, it is good to follow the same
pattern for consistency.  The resulting index and/or the working
tree may or may not match the contents recorded in the HEAD commit
but without the backend telling the caller, the caller cannot tell
if the difference was from before the operation or created by the
operation.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid test cond -a/-o cond

[David Aguilar]

On Tue, Jun 10, 2014 at 11:49:15AM -0700, David Aguilar wrote:
 On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:
  The construct is error-prone; test being built-in in most modern
  shells, the reason to avoid test cond  test cond spawning
  one extra process by using a single test cond -a cond no
  longer exists.
  
  Signed-off-by: Elia Pinto gitter.spi...@gmail.com
  ---
   t/t5000-tar-tree.sh |2 +-
   1 file changed, 1 insertion(+), 1 deletion(-)
  
  diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
  index 74fc5a8..ad6fa0d 100755
  --- a/t/t5000-tar-tree.sh
  +++ b/t/t5000-tar-tree.sh
  @@ -72,7 +72,7 @@ check_tar() {
  for header in *.paxheader
  do
  data=${header%.paxheader}.data 
  -   if test -h $data -o -e $data
  +   if test -h $data || test -e $data
  then
 
 This looks okay, but it raises a question for the original author
 (René, I think that's you so I've added you to the To: line).

Just following up -- I got a bounce from René's email address.

 
 Should that be test -f instead of test -e?

It does seem like this should be test -f nonetheless.
Sorry for the noise.

 This is a very minor note and should not block this patch.
-- 
David
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 00/11] Zsh prompt tests

[Torsten Bögershausen]

On 2014-06-04 23.01, Richard Hansen wrote:
[]
I haven't digged too deep, but this is what I get on pu:

./t9904-zsh-prompt.sh 
./lib-zsh.sh:emulate:42: too many arguments
./lib-zsh.sh:emulate:52: too many arguments
/lib-prompt-tests.sh:.:6: no such file or directory: /lib-prompt-tests.sh
##

 zsh --version
zsh 4.3.9 (i386-apple-darwin10.0)

I'm not a zsh expert, but I can offer to do more tests/debugging if needed 

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

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid test cond -a/-o cond

[David Aguilar]

[Resent using René's correct email address this time, sorry for the noise]

On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:
 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.
 
 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
  t/t5000-tar-tree.sh |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)
 
 diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
 index 74fc5a8..ad6fa0d 100755
 --- a/t/t5000-tar-tree.sh
 +++ b/t/t5000-tar-tree.sh
 @@ -72,7 +72,7 @@ check_tar() {
   for header in *.paxheader
   do
   data=${header%.paxheader}.data 
 - if test -h $data -o -e $data
 + if test -h $data || test -e $data
   then

This looks okay, but it raises a question for the original author
(René, I think that's you so I've added you to the To: line).

Should that be test -f instead of test -e?

This is a very minor note and should not block this patch.
It's probably a change that's better made in a follow-up patch.

   path=$(get_pax_header $header path) 
   if test -n $path
-- 
David
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 0/6] fix repack.packKeptObjects regression in v2.0

[Jeff King]

Here's a cleaned up version of the fix I posted earlier, with related
fixes on top.

  [1/6]: repack: do not accidentally pack kept objects by default

This is the most critical one for 'maint', as it fixes the
regression for people who are not using bitmaps at all.

  [2/6]: repack: respect pack.writebitmaps

We probably want to apply this one to maint, too, as the first one
breaks people depending on repack.packKeptObjects being impacted by
pack.writebitmaps (it never worked properly, but the breakage fixed
by the first patch covered it up).

  [3/6]: repack: s/write_bitmap/s/ in code
  [4/6]: pack-objects: stop respecting pack.writebitmaps
  [5/6]: repack: simplify handling of --write-bitmap-index
  [6/6]: repack: introduce repack.writeBitmaps config option

These ones can go the normal route, but see the regression comments in
4/6.

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

[PATCH 1/6] repack: do not accidentally pack kept objects by default

[Jeff King]

Commit ee34a2b (repack: add `repack.packKeptObjects` config
var, 2014-03-03) added a flag which could duplicate kept
objects, but did not mean to turn it on by default. Instead,
the option is tied by default to the decision to write
bitmaps, like:

  if (pack_kept_objects  0)
  pack_kept_objects = write_bitmap;

after which we expect pack_kept_objects to be a boolean 0 or
1.  However, that assignment neglects that write_bitmap is
_also_ a tri-state with -1 as the default, and with
neither option given, we accidentally turn the option on.

This patch is the minimal fix to restore the desired
behavior for the default state. Further patches will fix the
more complicated cases.

Note the update to t7700. It failed to turn on bitmaps,
meaning we were actually confirming the wrong behavior!

Signed-off-by: Jeff King p...@peff.net
---
 builtin/repack.c  | 2 +-
 t/t7700-repack.sh | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 6b0b62d..17bc8da 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -191,7 +191,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
git_repack_usage, 0);
 
if (pack_kept_objects  0)
-   pack_kept_objects = write_bitmap;
+   pack_kept_objects = write_bitmap  0;
 
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 284018e..f054434 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -37,7 +37,7 @@ test_expect_success 'objects in packs marked .keep are not 
repacked' '
 
 test_expect_success 'writing bitmaps can duplicate .keep objects' '
# build on $objsha1, $packsha1, and .keep state from previous
-   git repack -Adl 
+   git repack -Adbl 
test_when_finished found_duplicate_object= 
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
-- 
2.0.0.729.g520999f

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

[PATCH 2/6] repack: respect pack.writebitmaps

[Jeff King]

The config option to turn on bitmaps is read all the way
down in the plumbing of pack-objects. This makes it hard for
other options in the porcelain of repack to make decisions
based on the bitmap setting. For example,
repack.packKeptObjects tries to kick in by default only when
bitmaps are turned on. But it can't do so reliably because
it doesn't yet know whether we are using bitmaps.

This patch teaches repack to respect pack.writebitmaps. It
means we pass a redundant command-line flag to pack-objects,
but that's OK; it shouldn't affect the outcome.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/repack.c  |  6 +-
 t/t7700-repack.sh | 18 +-
 2 files changed, 22 insertions(+), 2 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 17bc8da..fab9989 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -10,6 +10,7 @@
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
+static int write_bitmap = -1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -27,6 +28,10 @@ static int repack_config(const char *var, const char *value, 
void *cb)
pack_kept_objects = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, pack.writebitmaps)) {
+   write_bitmap = git_config_bool(var, value);
+   return 0;
+   }
return git_default_config(var, value, cb);
 }
 
@@ -149,7 +154,6 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
int no_update_server_info = 0;
int quiet = 0;
int local = 0;
-   int write_bitmap = -1;
 
struct option builtin_repack_options[] = {
OPT_BIT('a', NULL, pack_everything,
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f054434..61e6ed3 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -35,7 +35,7 @@ test_expect_success 'objects in packs marked .keep are not 
repacked' '
test -z $found_duplicate_object
 '
 
-test_expect_success 'writing bitmaps can duplicate .keep objects' '
+test_expect_success 'writing bitmaps via command-line can duplicate .keep 
objects' '
# build on $objsha1, $packsha1, and .keep state from previous
git repack -Adbl 
test_when_finished found_duplicate_object= 
@@ -51,6 +51,22 @@ test_expect_success 'writing bitmaps can duplicate .keep 
objects' '
test $found_duplicate_object = 1
 '
 
+test_expect_success 'writing bitmaps via config can duplicate .keep objects' '
+   # build on $objsha1, $packsha1, and .keep state from previous
+   git -c pack.writebitmaps=true repack -Adl 
+   test_when_finished found_duplicate_object= 
+   for p in .git/objects/pack/*.idx; do
+   idx=$(basename $p)
+   test pack-$packsha1.idx = $idx  continue
+   if git verify-pack -v $p | egrep ^$objsha1; then
+   found_duplicate_object=1
+   echo DUPLICATE OBJECT FOUND
+   break
+   fi
+   done 
+   test $found_duplicate_object = 1
+'
+
 test_expect_success 'loose objects in alternate ODB are not repacked' '
mkdir alt_objects 
echo `pwd`/alt_objects  .git/objects/info/alternates 
-- 
2.0.0.729.g520999f

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

[PATCH 3/6] repack: s/write_bitmap/s/ in code

[Jeff King]

The config name is writeBitmaps, so the internal variable
missing the plural is unnecessarily confusing to write.

Signed-off-by: Jeff King p...@peff.net
---
Obviously not critical, but after getting it wrong several times while
working on the code, I was inspired to write this.

 builtin/repack.c | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index fab9989..36c1cf9 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -10,7 +10,7 @@
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
-static int write_bitmap = -1;
+static int write_bitmaps = -1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -29,7 +29,7 @@ static int repack_config(const char *var, const char *value, 
void *cb)
return 0;
}
if (!strcmp(var, pack.writebitmaps)) {
-   write_bitmap = git_config_bool(var, value);
+   write_bitmaps = git_config_bool(var, value);
return 0;
}
return git_default_config(var, value, cb);
@@ -172,7 +172,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
OPT__QUIET(quiet, N_(be quiet)),
OPT_BOOL('l', local, local,
N_(pass --local to git-pack-objects)),
-   OPT_BOOL('b', write-bitmap-index, write_bitmap,
+   OPT_BOOL('b', write-bitmap-index, write_bitmaps,
N_(write bitmap index)),
OPT_STRING(0, unpack-unreachable, unpack_unreachable, 
N_(approxidate),
N_(with -A, do not loosen objects older than 
this)),
@@ -195,7 +195,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
git_repack_usage, 0);
 
if (pack_kept_objects  0)
-   pack_kept_objects = write_bitmap  0;
+   pack_kept_objects = write_bitmaps  0;
 
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
@@ -221,9 +221,9 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
argv_array_pushf(cmd_args, --no-reuse-delta);
if (no_reuse_object)
argv_array_pushf(cmd_args, --no-reuse-object);
-   if (write_bitmap = 0)
+   if (write_bitmaps = 0)
argv_array_pushf(cmd_args, --%swrite-bitmap-index,
-write_bitmap ?  : no-);
+write_bitmaps ?  : no-);
 
if (pack_everything  ALL_INTO_ONE) {
get_non_kept_pack_filenames(existing_packs);
-- 
2.0.0.729.g520999f

--
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 v14 06/40] refs.c: add an err argument to repack_without_refs

[Jonathan Nieder]

Ronnie Sahlberg wrote:

 Update repack_without_refs to take an err argument and update it if there
 is a failure. Pass the err variable from ref_transaction_commit to this
 function so that callers can print a meaningful error message if _commit
 fails due to a problem in repack_without_refs.

 Add a new function unable_to_lock_message that takes a strbuf argument and
 fills in the reason for the failure.

 In commit_packed_refs, make sure that we propagate any errno that
 commit_lock_file might have set back to our caller.

 Make sure both commit_packed_refs and lock_file has errno set to a meaningful
 value on error. Update a whole bunch of other places to keep errno from
 beeing clobbered.

This patch is doing several (all nice) things.  Are they connected?
Would splitting some of them out into separate patches make sense or
would it be too much fuss to be worth the trouble?

[...]
 --- a/cache.h
 +++ b/cache.h
 @@ -559,6 +559,8 @@ struct lock_file {
  #define LOCK_DIE_ON_ERROR 1
  #define LOCK_NODEREF 2
  extern int unable_to_lock_error(const char *path, int err);
 +extern void unable_to_lock_message(const char *path, int err,
 +struct strbuf *buf);

Introducing a new unable_to_lock_message helper, which has nicer
semantics than unable_to_lock_error and cleans up lockfile.c a little.

[...]
 --- a/lockfile.c
 +++ b/lockfile.c
 @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
   return p;
  }
  
 -
 +/* Make sure errno contains a meaningful value on error */
  static int lock_file(struct lock_file *lk, const char *path, int flags)

Making errno when returning from lock_file() meaningful, which should
fix

 * an existing almost-bug in lock_ref_sha1_basic where it assumes
   errno==ENOENT is meaningful and could waste some work on retries

 * an existing bug in repack_without_refs where it prints
   strerror(errno) and picks advice based on errno, despite errno
   potentially being zero and potentially having been clobbered by
   that point

To make sure we don't lose these fixes, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:

 * hold_lock_file_for_update (declared in cache.h)
 * lock_packed_refs (declared in refs.h)

[...]
 --- a/refs.c
 +++ b/refs.c
 @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, 
 unsigned char *sha1, int rea

Making errno when returning from resolve_ref_unsafe() meaningful,
which should fix

 * a bug in lock_ref_sha1_basic, where it assumes EISDIR
   means it failed due to a directory being in the way

To make sure we don't lose this fix, it would be helpful to change
Michael's understated comment (why wasn't it marked with NEEDSWORK,
btw?)

 * errno is sometimes set on errors, but not always.

to describe the new guarantee.

[...]
 @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock 
 *lock,

Making errno when returning from verify_lock() meaningful, which
should almost but not completely fix

 * a bug in git fetch's s_update_ref, which trusts the result of an
   errno == ENOTDIR check to detect D/F conflicts

To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for the relevant
functions:

 * lock_any_ref_for_update (declared in refs.h), after handling the
   check_refname_format case
 * lock_ref_sha1_basic

ENOTDIR makes sense as a sign that a file was in the way of a
directory we wanted to create.  Should git fetch also look for
ENOTEMPTY or EEXIST to catch cases where a directory was in the way
of a file to be created?

 @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)

Making errno when returning from remove_empty_directories() more
obviously meaningful, which should provide some peace of mind for
people auditing lock_ref_sha1_basic.

[...]
 @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)
[...]
  int commit_packed_refs(void)

Making errno when returning from commit_packed_refs() meaningful,
which should fix

 * a bug in git clone where it prints strerror(errno) based on
   errno, despite errno possibly being zero and potentially having
   been clobbered by that point
 * the same kind of bug in git pack-refs

and prepares for repack_without_refs() to get a meaningful
error message when commit_packed_refs() fails without falling into
the same bug.

To make sure we don't lose this fix, it would be helpful to also
mention that errno is set in the API documentation for
commit_packed_refs in refs.h.

[...]
 @@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry 
 *entry, void *cb_data)
   return 0;
  }
  
 -static int repack_without_refs(const char **refnames, int n)
 +static int repack_without_refs(const char **refnames, int n, struct strbuf 
 *err)

Adding an 'err' argument to repack_without_refs and writing to it on
error.

[...]
 @@ -2723,9 +2755,12 @@ int log_ref_setup(const char 

[PATCH 5/6] repack: simplify handling of --write-bitmap-index

[Jeff King]

We previously needed to pass --no-write-bitmap-index
explicitly to pack-objects to override its reading of
pack.writebitmaps from the config. Now that it no longer
does so, we can assume that bitmaps are off by default, and
only turn them on when necessary. This also lets us avoid a
confusing tri-state flag for write_bitmaps.

Signed-off-by: Jeff King p...@peff.net
---
Obviously needs to be dropped if we don't take the previous patch.

 builtin/repack.c | 9 -
 1 file changed, 4 insertions(+), 5 deletions(-)

diff --git a/builtin/repack.c b/builtin/repack.c
index 36c1cf9..0e119b7 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -10,7 +10,7 @@
 
 static int delta_base_offset = 1;
 static int pack_kept_objects = -1;
-static int write_bitmaps = -1;
+static int write_bitmaps;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -195,7 +195,7 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
git_repack_usage, 0);
 
if (pack_kept_objects  0)
-   pack_kept_objects = write_bitmaps  0;
+   pack_kept_objects = write_bitmaps;
 
packdir = mkpathdup(%s/pack, get_object_directory());
packtmp = mkpathdup(%s/.tmp-%d-pack, packdir, (int)getpid());
@@ -221,9 +221,8 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
argv_array_pushf(cmd_args, --no-reuse-delta);
if (no_reuse_object)
argv_array_pushf(cmd_args, --no-reuse-object);
-   if (write_bitmaps = 0)
-   argv_array_pushf(cmd_args, --%swrite-bitmap-index,
-write_bitmaps ?  : no-);
+   if (write_bitmaps)
+   argv_array_push(cmd_args, --write-bitmap-index);
 
if (pack_everything  ALL_INTO_ONE) {
get_non_kept_pack_filenames(existing_packs);
-- 
2.0.0.729.g520999f

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

[PATCH 4/6] pack-objects: stop respecting pack.writebitmaps

[Jeff King]

The handling of the pack.writebitmaps config option
originally happened in pack-objects, which is quite
low-level. It would make more sense for drivers of
pack-objects to read the config, and then manipulate
pack-objects with command-line options.

Recently, repack learned to do so, making the low-level read
of pack.writebitmaps redundant here. Other callers, like
upload-pack, would not generally want to write bitmaps
anyway.

This could be considered a regression for somebody who is
driving pack-objects themselves outside of repack and
expects the config option to be used. However, such users
seem rather unlikely given how new the bitmap code is (and
the fact that they would basically be reimplementing repack
in the first place).

Note that we do not do anything with pack.writeBitmapHashCache
here. That option is not about do we write bimaps, but
rather when we are writing bitmaps, how do we do it?. You
would want that to kick in anytime you decide to write them,
similar to how pack.indexVersion is used.

Signed-off-by: Jeff King p...@peff.net
---
I'm not sure what we want to do with this. It _is_ a possible regression
as explained above, but I really do find it improbable that anyone will
care. Even at GitHub, where we use a custom script instead of running
`git gc`, we hook into the repack code, and not directly into
pack-objects.

One option is obviously to drop it as not worth it (you don't see the
benefit here, but it enables the cleanups in the rest of the series).

Another option is noting that the regression is worth dealing with,
adding a deprecation notice, and phasing it out eventually. I tend to
think it's not worth the trouble here.

Another option is to track it to graduate to master during the next
cycle. I.e., decide that the possible regression isn't a big deal.

The final option is to track it for maint, along with the earlier
patches.  The argument for that is:

  1. It's not a regression worth caring about (i.e., if it's not worth
 caring about for master, it's probably not worth caring about for
 maint, either).

  2. It shortens the window in which the old behavior is in a release,
 making it less likely for somebody to try depending on it.

 builtin/pack-objects.c | 4 
 1 file changed, 4 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index de36c60..238b502 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -2214,10 +2214,6 @@ static int git_pack_config(const char *k, const char *v, 
void *cb)
cache_max_small_delta_size = git_config_int(k, v);
return 0;
}
-   if (!strcmp(k, pack.writebitmaps)) {
-   write_bitmap_index = git_config_bool(k, v);
-   return 0;
-   }
if (!strcmp(k, pack.writebitmaphashcache)) {
if (git_config_bool(k, v))
write_bitmap_options |= BITMAP_OPT_HASH_CACHE;
-- 
2.0.0.729.g520999f

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

[PATCH 6/6] repack: introduce repack.writeBitmaps config option

[Jeff King]

We currently have pack.writeBitmaps, which originally
operated at the pack-objects level. This should really have
been a repack.* option from day one. Let's give it the more
sensible name, but keep the old version as a deprecated
synonym.

Signed-off-by: Jeff King p...@peff.net
---
We can still do this if we don't take 4/6, but the this is a synonym
in the documentation is not quite true, then. We'd probably want to
explain the difference.

 Documentation/config.txt | 17 ++---
 builtin/repack.c |  3 ++-
 t/perf/p5310-pack-bitmaps.sh |  3 +++
 t/t5310-pack-bitmaps.sh  |  2 +-
 t/t7700-repack.sh|  2 +-
 5 files changed, 17 insertions(+), 10 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index cd2d651..8f0f800 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1901,12 +1901,7 @@ pack.useBitmaps::
you are debugging pack bitmaps.
 
 pack.writebitmaps::
-   When true, git will write a bitmap index when packing all
-   objects to disk (e.g., when `git repack -a` is run).  This
-   index can speed up the counting objects phase of subsequent
-   packs created for clones and fetches, at the cost of some disk
-   space and extra time spent on the initial repack.  Defaults to
-   false.
+   This is a deprecated synonym for `repack.writeBitmaps`.
 
 pack.writeBitmapHashCache::
When true, git will include a hash cache section in the bitmap
@@ -2183,7 +2178,15 @@ repack.packKeptObjects::
`--pack-kept-objects` was passed. See linkgit:git-repack[1] for
details. Defaults to `false` normally, but `true` if a bitmap
index is being written (either via `--write-bitmap-index` or
-   `pack.writeBitmaps`).
+   `repack.writeBitmaps`).
+
+repack.writeBitmaps::
+   When true, git will write a bitmap index when packing all
+   objects to disk (e.g., when `git repack -a` is run).  This
+   index can speed up the counting objects phase of subsequent
+   packs created for clones and fetches, at the cost of some disk
+   space and extra time spent on the initial repack.  Defaults to
+   false.
 
 rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
diff --git a/builtin/repack.c b/builtin/repack.c
index 0e119b7..ff2216a 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -28,7 +28,8 @@ static int repack_config(const char *var, const char *value, 
void *cb)
pack_kept_objects = git_config_bool(var, value);
return 0;
}
-   if (!strcmp(var, pack.writebitmaps)) {
+   if (!strcmp(var, repack.writebitmaps) ||
+   !strcmp(var, pack.writebitmaps)) {
write_bitmaps = git_config_bool(var, value);
return 0;
}
diff --git a/t/perf/p5310-pack-bitmaps.sh b/t/perf/p5310-pack-bitmaps.sh
index 685d46f..f8ed857 100755
--- a/t/perf/p5310-pack-bitmaps.sh
+++ b/t/perf/p5310-pack-bitmaps.sh
@@ -8,6 +8,9 @@ test_perf_large_repo
 # note that we do everything through config,
 # since we want to be able to compare bitmap-aware
 # git versus non-bitmap git
+#
+# We intentionally use the deprecated pack.writebitmaps
+# config so that we can test against older versions of git.
 test_expect_success 'setup bitmap config' '
git config pack.writebitmaps true 
git config pack.writebitmaphashcache true
diff --git a/t/t5310-pack-bitmaps.sh b/t/t5310-pack-bitmaps.sh
index f4f02ba..0580258 100755
--- a/t/t5310-pack-bitmaps.sh
+++ b/t/t5310-pack-bitmaps.sh
@@ -18,7 +18,7 @@ test_expect_success 'setup repo with moderate-sized history' '
git checkout master 
blob=$(echo tagged-blob | git hash-object -w --stdin) 
git tag tagged-blob $blob 
-   git config pack.writebitmaps true 
+   git config repack.writebitmaps true 
git config pack.writebitmaphashcache true
 '
 
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index 61e6ed3..82d39ad 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -53,7 +53,7 @@ test_expect_success 'writing bitmaps via command-line can 
duplicate .keep object
 
 test_expect_success 'writing bitmaps via config can duplicate .keep objects' '
# build on $objsha1, $packsha1, and .keep state from previous
-   git -c pack.writebitmaps=true repack -Adl 
+   git -c repack.writebitmaps=true repack -Adl 
test_when_finished found_duplicate_object= 
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
-- 
2.0.0.729.g520999f
--
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 00/11] Zsh prompt tests

[Richard Hansen]

On 2014-06-10 16:06, Torsten Bögershausen wrote:
 On 2014-06-04 23.01, Richard Hansen wrote:
 []
 I haven't digged too deep, but this is what I get on pu:
 
 ./t9904-zsh-prompt.sh 
 ./lib-zsh.sh:emulate:42: too many arguments
 ./lib-zsh.sh:emulate:52: too many arguments
 /lib-prompt-tests.sh:.:6: no such file or directory: /lib-prompt-tests.sh
 ##

Thank you for trying these patches and reporting your findings!

 
  zsh --version
 zsh 4.3.9 (i386-apple-darwin10.0)

zsh 4.3.9 is over 5 years old (2008-11-03).  Is that young enough that
we should still try to support it?

 
 I'm not a zsh expert, but I can offer to do more tests/debugging if needed 

I'm not a zsh expert either (I just got a crash course over the past
couple of weeks), but if I were to guess I'd say that the 'emulate -c'
option is new to zsh 5.0 (2012-07-21).  When I next have time I'll try
testing with a bunch of different versions of zsh.

Thanks,
Richard
--
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: Disk waste with packs and .keep files

[Jeff King]

On Tue, Jun 10, 2014 at 02:53:21PM -0400, Jeff King wrote:

 This patch is the minimal fix to restore the desired
 behavior for the default state. However, the real fix will
 be more involved.

This patch actually breaks t7700, but because the test is wrong. Double
yikes. All fixed in my series, which should be along in a moment.

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

Re: [PATCH 15/15] commit: record buffer length in cache

[Jeff King]

On Tue, Jun 10, 2014 at 01:27:13AM -0400, Jeff King wrote:

  I find the above strange. I would have done something like:
  
  -   set_commit_buffer(commit, strbuf_detach(msg, NULL));
  +   size_t size;
  +   char *buf = strbuf_detach(msg, size);
  +   set_commit_buffer(commit, buf, size);
 
 It is a little strange. You can't do:
 
   set_commit_buffer(commit, strbuf_detach(msg, NULL), msg.len);
 
 because the second argument resets msg.len as a side effect. Doing it
 your way is longer, but perhaps is a bit more canonical. In general, one
 should call strbuf_detach to ensure that the buffer is allocated (and
 does not point to slopbuf). That's guaranteed here, because we just put
 contents into the buffer, but it's probably more hygienic to use the
 more verbose form.

I was trying to avoid cluttering up the function with the extra variable
definitions. I ended up with the change below, which I think is clear,
correct, and pushes the complexity outside of the function.

diff --git a/builtin/blame.c b/builtin/blame.c
index cde19eb..53f43ab 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2266,6 +2266,18 @@ static void append_merge_parents(struct commit_list 
**tail)
 }
 
 /*
+ * This isn't as simple as passing sb-buf and sb-len, because we
+ * want to transfer ownership of the buffer to the commit (so we
+ * must use detach).
+ */
+static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf *sb)
+{
+   size_t len;
+   void *buf = strbuf_detach(sb, len);
+   set_commit_buffer(c, buf, len);
+}
+
+/*
  * Prepare a dummy commit that represents the work tree (or staged) item.
  * Note that annotating work tree item never works in the reverse.
  */
@@ -2313,7 +2325,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
ident, ident, path,
(!contents_from ? path :
 (!strcmp(contents_from, -) ? standard input : 
contents_from)));
-   set_commit_buffer(commit, strbuf_detach(msg, NULL));
+   set_commit_buffer_from_strbuf(commit, msg);
 
if (!contents_from || strcmp(-, contents_from)) {
struct stat st;


--
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-submodule.sh: avoid test cond -a/-o cond

[Eric Sunshine]

On Tue, Jun 10, 2014 at 12:43 PM, Elia Pinto gitter.spi...@gmail.com wrote:
 The construct is error-prone; test being built-in in most modern
 shells, the reason to avoid test cond  test cond spawning
 one extra process by using a single test cond -a cond no
 longer exists.

 Signed-off-by: Elia Pinto gitter.spi...@gmail.com
 ---
 This is the fifth revision.

 Change based on Junio bugfix and better rewrite of the case condition
 http://permalink.gmane.org/gmane.comp.version-control.git/251198

 I dropped also the echo - printf replacement for doing
 it in another patch.

 Pass all the t/*submodule* tests. Finally ! :=)

 Thank you all very much and sorry for the mess.

  git-submodule.sh |   32 
  1 file changed, 20 insertions(+), 12 deletions(-)

 diff --git a/git-submodule.sh b/git-submodule.sh
 index e146b83..e128a4a 100755
 --- a/git-submodule.sh
 +++ b/git-submodule.sh
 while read mod_src mod_dst sha1_src sha1_dst status sm_path
 do
 # Always show modules deleted or type-changed 
 (blob-module)
 -   test $status = D -o $status = T  echo $sm_path  
 continue
 +   if test $status = D || test $status = T
 +then
 +   echo $sm_path 

Unnecessary .

 +   continue
 +   fi
 # Respect the ignore setting for --for-status.
 if test -n $for_status
 then
 name=$(module_name $sm_path)
 ignore_config=$(get_submodule_config $name 
 ignore none)
 -   test $status != A -a $ignore_config = all  
 continue
 +   test $status != A  test $ignore_config = 
 all  continue
 fi
 # Also show added or modified modules which are 
 checked out
 GIT_DIR=$sm_path/.git git-rev-parse --git-dir 
 /dev/null 21 
--
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 10/15] use get_commit_buffer to avoid duplicate code

[Jeff King]

On Tue, Jun 10, 2014 at 04:01:29AM -0400, Eric Sunshine wrote:

  For record_author_date, the new behavior is probably better;
  we notify the user of the error instead of silently ignoring
  it. And because it's used only for sorting by author-date,
  somebody examining a corrupt can fallback to the regular
 
 Missing word here? examining a corrupt [...] can

It was repo. Thanks, fixed this and your other comments for my
re-roll (I'm still looking into the best solution for the strbuf_attach
issue Junio mentioned).

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

Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps

[Junio C Hamano]

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

 I'm not sure what we want to do with this. It _is_ a possible regression
 as explained above, but I really do find it improbable that anyone will
 care. Even at GitHub, where we use a custom script instead of running
 `git gc`, we hook into the repack code, and not directly into
 pack-objects.

 One option is obviously to drop it as not worth it (you don't see the
 benefit here, but it enables the cleanups in the rest of the series).

 Another option is noting that the regression is worth dealing with,
 adding a deprecation notice, and phasing it out eventually. I tend to
 think it's not worth the trouble here.

 Another option is to track it to graduate to master during the next
 cycle. I.e., decide that the possible regression isn't a big deal.

My gut feeling is that the last one is sufficient.  These low level
subcommands that are designed to be used by scripts (aka plumbing)
shouldn't have configuration options in the first place, and users
shouldn't depend on them even if they were added by design mistake.

 The final option is to track it for maint, along with the earlier
 patches.  The argument for that is:

   1. It's not a regression worth caring about (i.e., if it's not worth
  caring about for master, it's probably not worth caring about for
  maint, either).

   2. It shortens the window in which the old behavior is in a release,
  making it less likely for somebody to try depending on it.

Yeah, probably.  But I am not sure if that is even needed.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v1] config: Add hashtable for config parsing retrival

[Eric Sunshine]

On Tue, Jun 10, 2014 at 8:35 AM, Tanay Abhra tanay...@gmail.com wrote:
 Thanks for the review, Eric. I have replied to your comments below,
 I will try to reply early and more promptly now.

Thanks for responding. More below.

 On 06/10/2014 04:27 AM, Eric Sunshine wrote:
 ---
 diff --git a/config.c b/config.c
 index a30cb5c..6f6b04e 100644
 --- a/config.c
 +++ b/config.c
 @@ -9,6 +9,8 @@
  #include exec_cmd.h
  #include strbuf.h
  #include quote.h
 +#include hashmap.h
 +#include string-list.h

  struct config_source {
 struct config_source *prev;
 @@ -37,6 +39,120 @@ static struct config_source *cf;

  static int zlib_compression_seen;

 +struct config_cache_entry {
 +   struct hashmap_entry ent;
 +   char *key;
 +   struct string_list *value_list;

 Same question as in my v1 and v2 reviews [1][2], and reiterated by
 Matthieu [3]: Why is 'value_list' a pointer to a structure rather than
 just a structure?

 Sorry for the lack of response on this part. I thought choosing a pointer to a
 structure or just the structure itself was a stylistic choice.

Each point or question raised in a review is meaningful to the
reviewer and deserves some sort of response or consideration, even if
you don't agree with the reviewer's opinion. It's okay to have
stylistic or other preferences (and to argue for them), but unless you
voice them, the reviewer is likely to conclude that his review effort
is wasted.

In this case, it's not a stylistic consideration which prompted the
question, but practical concern. (More on that below.)

 Since most of the
 functions just pass the pointer to this struct, I thought it would be more 
 natural to
 use it. But I have changed my mind on this one. In the next iteration I will 
 be using
 a plane old struct.

There are practical reasons for choosing a structure rather than a
pointer to a structure. Problems with a pointer include:

- Expense of an extra (unnecessary) heap allocation. You allocate both
config_cache_entry and string_list on the heap for each entry being
inserted into the hash, rather than allocating only a
config_cache_entry.

- Potential additional heap fragmentation.

- Increased possibility of leaking memory. The current implementation
of config_cache_free() drives this point home: although it correctly
clears the string_list, it leaks the heap-allocated string_list
itself.

- Potentially misleading readers of the code into thinking that there
is an important reason for the string_list to be allocated separately
from its containing object (for instance, is the string list shared?
or does the string list need to exist beyond the lifetime of the
config_cache_entry?).

 +}
 +
 +static void config_cache_init(struct hashmap *config_cache)
 +{
 +   hashmap_init(config_cache, (hashmap_cmp_fn) 
 config_cache_entry_cmp_case, 0);

 In his review [4], Peff suggested giving config_cache_entry_cmp_case()
 the correct hashmap_cmp_fn signature so that this cast can be dropped.
 It's not clear whether you disagree with his advice or overlooked or
 forgot about it. If you disagree with his suggestion, it's okay to say
 so and explain why you feel the way you do, but without feedback from
 you, he or another reviewer is going to continue suggesting the same
 change.

 Now on this one, I checked the code thoroughly. Every single hashmap_init()
 incantation in git code has a hashmap_cmp_fn cast.

That's a good argument for explaining to Peff why you did it this way,
but unless you voice this, he will likely feel that you simply ignored
his review comment.

 I don't think that it is necessary to do so. Is it?

There are good reasons for avoiding the cast. Jonathan gives one such
reason here [1]. In that case, he let it slide for the reason you
mention: existing hashmap clients use the cast. (However, the
implication is that it would be nice for someone -- not necessarily
you -- to submit a patch series eliminating the casts.)

Whether Peff will be swayed by the same argument, only he can answer.
(If he doesn't answer, taking [1] into consideration, you'd probably
be on safe ground to continue using the cast.)

[1]: http://thread.gmane.org/gmane.comp.version-control.git/243818/focus=243917
(see his comments about hashmap_init()).

 +   if (ret)
 +   return NULL;
 +
 +   hashmap_entry_init(k, strhash(normalized_key));
 +   k.key = (char*) key;

 This looks fishy. You're hashing based upon 'normalized_key' but then
 comparing against the unnormalized 'key'. Peff's suggestion [4] was to
 store the normalized key in the hash, which means that you should use
 'normalized_key' for both hashing and comparing. (See additional
 commentary about this issue below in config_cache_set_value().)

 Ouch, this I had corrected in a future commit. But forgot to include in
 this patch.

[Leaving unsnipped since it has relevance below.]

 Style: (char *)key

 Noted. In function arguments the code uses (char*) key. I copied it from 
 there.
 :)

Indeed, there's an 

Re: [PATCH 12/15] use get_commit_buffer everywhere

[Jeff King]

On Tue, Jun 10, 2014 at 09:06:35AM -0700, Junio C Hamano wrote:

  So any call to strbuf_detach on the result would be disastrous.
 
 You are right.  Where did this original crap come from X-...

I do not know if that face means you actually looked at the history, but
in case you did not...

It was added by Duy's 13f8b72 (Convert commit_tree() to take strbuf as
message, 2011-12-15). However that was v2 of his patch. If you read the
original thread, you can see that v1 passed a separate pointer/length
pair, and was only changed after a reviewer-who-shall-not-be-named asked
him to change it. ;)

Of course there were many people participating in the review, and none
of us noticed it. I think it is simply a subtle bug.

  I feel like the most elegant solution is for create_notes_commit to take
  a buf/len pair rather than a strbuf, but it unfortunately is just
  feeding that to commit_tree. Adjusting that code path would affect quite
  a few other spots.
 
  The other obvious option is actually populating the strbuf, but it feels
  ugly to have to make a copy just to satisfy the function interface.
 
  Maybe a cast and a warning comment are the least evil thing, as below? I
  dunno, it feels pretty wrong.
 
 Yeah, it does feel wrong wrong wrong.  Perhaps this big comment
 would serve as a marker for a low-hanging fruit for somebody else to
 fix it, e.g. by using strbuf-add to make a copy, which would be the
 easiest and safest workaround?

I really think commit_tree is the culprit here. It doesn't actually want
a strbuf at all, but takes one to make passing the pointer/len pair
simpler. Fixing it turned out to be not _too_ disruptive, and it showed
that there is another dubious use of strbuf_attach from a different
caller.

I'll post my re-rolled series with those fixes in a moment.

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

Re: [PATCH 4/6] pack-objects: stop respecting pack.writebitmaps

[Jeff King]

On Tue, Jun 10, 2014 at 02:07:37PM -0700, Junio C Hamano wrote:

  Another option is to track it to graduate to master during the next
  cycle. I.e., decide that the possible regression isn't a big deal.
 
 My gut feeling is that the last one is sufficient.  These low level
 subcommands that are designed to be used by scripts (aka plumbing)
 shouldn't have configuration options in the first place, and users
 shouldn't depend on them even if they were added by design mistake.

That is my gut, too (and why I posted the patch). I was mostly just
trying to make sure it was not me being lazy (it is easy to be so when
you are the one writing the patches, rather than the one reviewing
them or acting as maintainer). :)

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

Re: [PATCH 15/15] commit: record buffer length in cache

[Christian Couder]

From: Jeff King p...@peff.net
Subject: Re: [PATCH 15/15] commit: record buffer length in cache
Date: Tue, 10 Jun 2014 16:33:49 -0400

 On Tue, Jun 10, 2014 at 01:27:13AM -0400, Jeff King wrote:
 
  I find the above strange. I would have done something like:
  
  -  set_commit_buffer(commit, strbuf_detach(msg, NULL));
  +  size_t size;
  +  char *buf = strbuf_detach(msg, size);
  +  set_commit_buffer(commit, buf, size);
 
 It is a little strange. You can't do:
 
   set_commit_buffer(commit, strbuf_detach(msg, NULL), msg.len);
 
 because the second argument resets msg.len as a side effect. Doing it
 your way is longer, but perhaps is a bit more canonical. In general, one
 should call strbuf_detach to ensure that the buffer is allocated (and
 does not point to slopbuf). That's guaranteed here, because we just put
 contents into the buffer, but it's probably more hygienic to use the
 more verbose form.
 
 I was trying to avoid cluttering up the function with the extra variable
 definitions. I ended up with the change below, which I think is clear,
 correct, and pushes the complexity outside of the function.

Yeah, great!
 
 diff --git a/builtin/blame.c b/builtin/blame.c
 index cde19eb..53f43ab 100644
 --- a/builtin/blame.c
 +++ b/builtin/blame.c
 @@ -2266,6 +2266,18 @@ static void append_merge_parents(struct commit_list 
 **tail)
  }
  
  /*
 + * This isn't as simple as passing sb-buf and sb-len, because we
 + * want to transfer ownership of the buffer to the commit (so we
 + * must use detach).
 + */
 +static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf 
 *sb)
 +{
 + size_t len;
 + void *buf = strbuf_detach(sb, len);
 + set_commit_buffer(c, buf, len);
 +}
 +
 +/*
   * Prepare a dummy commit that represents the work tree (or staged) item.
   * Note that annotating work tree item never works in the reverse.
   */
 @@ -2313,7 +2325,7 @@ static struct commit *fake_working_tree_commit(struct 
 diff_options *opt,
   ident, ident, path,
   (!contents_from ? path :
(!strcmp(contents_from, -) ? standard input : 
 contents_from)));
 - set_commit_buffer(commit, strbuf_detach(msg, NULL));
 + set_commit_buffer_from_strbuf(commit, msg);
  
   if (!contents_from || strcmp(-, contents_from)) {
   struct stat st;

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

Re: [PATCH 02/15] commit: push commit_index update into alloc_commit_node

[Junio C Hamano]

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

 This will make the alloc_report output a little uglier (it will say
 raw_commit), but I don't think anyone cares. And I wanted to make sure
 there wasn't an easy way to accidentally call the wrong alloc function
 that doesn't handle the index.

Thanks; I like this change.


  alloc.c  | 12 ++--
  commit.c |  2 --
  2 files changed, 10 insertions(+), 4 deletions(-)

 diff --git a/alloc.c b/alloc.c
 index 38ff7e7..eb22a45 100644
 --- a/alloc.c
 +++ b/alloc.c
 @@ -47,10 +47,18 @@ union any_object {
  
  DEFINE_ALLOCATOR(blob, struct blob)
  DEFINE_ALLOCATOR(tree, struct tree)
 -DEFINE_ALLOCATOR(commit, struct commit)
 +DEFINE_ALLOCATOR(raw_commit, struct commit)
  DEFINE_ALLOCATOR(tag, struct tag)
  DEFINE_ALLOCATOR(object, union any_object)
  
 +void *alloc_commit_node(void)
 +{
 + static int commit_count;
 + struct commit *c = alloc_raw_commit_node();
 + c-index = commit_count++;
 + return c;
 +}
 +
  static void report(const char *name, unsigned int count, size_t size)
  {
   fprintf(stderr, %10s: %8u (%PRIuMAX kB)\n,
 @@ -64,7 +72,7 @@ void alloc_report(void)
  {
   REPORT(blob, struct blob);
   REPORT(tree, struct tree);
 - REPORT(commit, struct commit);
 + REPORT(raw_commit, struct commit);
   REPORT(tag, struct tag);
   REPORT(object, union any_object);
  }
 diff --git a/commit.c b/commit.c
 index f479331..21957ee 100644
 --- a/commit.c
 +++ b/commit.c
 @@ -17,7 +17,6 @@ static struct commit_extra_header 
 *read_commit_extra_header_lines(const char *bu
  int save_commit_buffer = 1;
  
  const char *commit_type = commit;
 -static int commit_count;
  
  static struct commit *check_commit(struct object *obj,
  const unsigned char *sha1,
 @@ -64,7 +63,6 @@ struct commit *lookup_commit(const unsigned char *sha1)
   struct object *obj = lookup_object(sha1);
   if (!obj) {
   struct commit *c = alloc_commit_node();
 - c-index = commit_count++;
   return create_object(sha1, OBJ_COMMIT, c);
   }
   if (!obj-type)
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v2 0/17] store length of commit-buffer

[Jeff King]

Here's a re-roll of the commit-slab series. It fixes the issues pointed
out by Eric and Christian (thanks, both).

It adds two new patches at the beginning to clean up the dangerous
strbufs that we discussed elsewhere. And as a result, silences the
warning from the old 12/15. I even compiled with a working copy of gcc
to confirm. :)

  [01/17]: commit_tree: take a pointer/len pair rather than a const strbuf
  [02/17]: replace dangerous uses of strbuf_attach
  [03/17]: alloc: include any-object allocations in alloc_report
  [04/17]: commit: push commit_index update into alloc_commit_node
  [05/17]: do not create struct commit with xcalloc
  [06/17]: logmsg_reencode: return const buffer
  [07/17]: sequencer: use logmsg_reencode in get_message
  [08/17]: provide a helper to free commit buffer
  [09/17]: provide a helper to set the commit buffer
  [10/17]: provide helpers to access the commit buffer
  [11/17]: use get_cached_commit_buffer where appropriate
  [12/17]: use get_commit_buffer to avoid duplicate code
  [13/17]: convert logmsg_reencode to get_commit_buffer
  [14/17]: use get_commit_buffer everywhere
  [15/17]: commit-slab: provide a static initializer
  [16/17]: commit: convert commit-buffer to a slab
  [17/17]: commit: record buffer length in cache

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

Re: [PATCH 03/15] do not create struct commit with xcalloc

[Junio C Hamano]

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

 In both blame and merge-recursive, we sometimes create a
 fake commit struct for convenience (e.g., to represent the
 HEAD state as if we would commit it). By allocating
 ourselves rather than using alloc_commit_node, we do not
 properly set the index field of the commit. This can
 produce subtle bugs if we then use commit-slab on the
 resulting commit, as we will share the 0 index with
 another commit.

The change I see here is all good, but I wonder if it is too late to
start the real index from 1 and catch anything that has 0 index with
a BUG(do not hand-allocate a commit).

 We can fix this by using alloc_commit_node() to allocate.
 Note that we cannot free the result, as it is part of our
 commit allocator. However, both cases were already leaking
 the allocated commit anyway, so there's nothing to fix up.

 Signed-off-by: Jeff King p...@peff.net
 ---
  builtin/blame.c   | 2 +-
  merge-recursive.c | 2 +-
  2 files changed, 2 insertions(+), 2 deletions(-)

 diff --git a/builtin/blame.c b/builtin/blame.c
 index a52a279..d6056a5 100644
 --- a/builtin/blame.c
 +++ b/builtin/blame.c
 @@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct 
 diff_options *opt,
   struct strbuf msg = STRBUF_INIT;
  
   time(now);
 - commit = xcalloc(1, sizeof(*commit));
 + commit = alloc_commit_node();
   commit-object.parsed = 1;
   commit-date = now;
   commit-object.type = OBJ_COMMIT;
 diff --git a/merge-recursive.c b/merge-recursive.c
 index cab16fa..2b37d42 100644
 --- a/merge-recursive.c
 +++ b/merge-recursive.c
 @@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, 
 struct tree *two,
  
  static struct commit *make_virtual_commit(struct tree *tree, const char 
 *comment)
  {
 - struct commit *commit = xcalloc(1, sizeof(struct commit));
 + struct commit *commit = alloc_commit_node();
   struct merge_remote_desc *desc = xmalloc(sizeof(*desc));
  
   desc-name = comment;
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 01/17] commit_tree: take a pointer/len pair rather than a const strbuf

[Jeff King]

While strbufs are pretty common throughout our code, it is
more flexible for functions to take a pointer/len pair than
a strbuf. It's easy to turn a strbuf into such a pair (by
dereferencing its members), but less easy to go the other
way (you can strbuf_attach, but that has implications about
memory ownership).

This patch teaches commit_tree (and its associated callers
and sub-functions) to take such a pair for the commit
message rather than a strbuf.  This makes passing the buffer
around slightly more verbose, but means we can get rid of
some dangerous strbuf_attach calls in the next patch.

Signed-off-by: Jeff King p...@peff.net
---
Sadly, the diffstat is not +x/-x. Rewrapping the longer lines
gave us a few extra lines.

 builtin/commit-tree.c |  4 ++--
 builtin/commit.c  |  4 ++--
 builtin/merge.c   |  8 
 commit.c  | 12 +++-
 commit.h  |  6 --
 notes-cache.c |  2 +-
 notes-merge.c |  6 --
 notes-utils.c |  7 ---
 notes-utils.h |  2 +-
 9 files changed, 29 insertions(+), 22 deletions(-)

diff --git a/builtin/commit-tree.c b/builtin/commit-tree.c
index 987a4c3..8a66c74 100644
--- a/builtin/commit-tree.c
+++ b/builtin/commit-tree.c
@@ -123,8 +123,8 @@ int cmd_commit_tree(int argc, const char **argv, const char 
*prefix)
die_errno(git commit-tree: failed to read);
}
 
-   if (commit_tree(buffer, tree_sha1, parents, commit_sha1,
-   NULL, sign_commit)) {
+   if (commit_tree(buffer.buf, buffer.len, tree_sha1, parents,
+   commit_sha1, NULL, sign_commit)) {
strbuf_release(buffer);
return 1;
}
diff --git a/builtin/commit.c b/builtin/commit.c
index 99c2044..ef300f1 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1731,8 +1731,8 @@ int cmd_commit(int argc, const char **argv, const char 
*prefix)
append_merge_tag_headers(parents, tail);
}
 
-   if (commit_tree_extended(sb, active_cache_tree-sha1, parents, sha1,
-author_ident.buf, sign_commit, extra)) {
+   if (commit_tree_extended(sb.buf, sb.len, active_cache_tree-sha1,
+parents, sha1, author_ident.buf, sign_commit, extra)) {
rollback_index_files();
die(_(failed to write commit object));
}
diff --git a/builtin/merge.c b/builtin/merge.c
index 428ca24..b49c310 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -852,8 +852,8 @@ static int merge_trivial(struct commit *head, struct 
commit_list *remoteheads)
parent-next-item = remoteheads-item;
parent-next-next = NULL;
prepare_to_commit(remoteheads);
-   if (commit_tree(merge_msg, result_tree, parent, result_commit, NULL,
-   sign_commit))
+   if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parent,
+   result_commit, NULL, sign_commit))
die(_(failed to write commit object));
finish(head, remoteheads, result_commit, In-index merge);
drop_save();
@@ -877,8 +877,8 @@ static int finish_automerge(struct commit *head,
commit_list_insert(head, parents);
strbuf_addch(merge_msg, '\n');
prepare_to_commit(remoteheads);
-   if (commit_tree(merge_msg, result_tree, parents, result_commit,
-   NULL, sign_commit))
+   if (commit_tree(merge_msg.buf, merge_msg.len, result_tree, parents,
+   result_commit, NULL, sign_commit))
die(_(failed to write commit object));
strbuf_addf(buf, Merge made by the '%s' strategy., wt_strategy);
finish(head, remoteheads, result_commit, buf.buf);
diff --git a/commit.c b/commit.c
index f479331..bd3d5af 100644
--- a/commit.c
+++ b/commit.c
@@ -1344,7 +1344,8 @@ void free_commit_extra_headers(struct commit_extra_header 
*extra)
}
 }
 
-int commit_tree(const struct strbuf *msg, const unsigned char *tree,
+int commit_tree(const char *msg, size_t msg_len,
+   const unsigned char *tree,
struct commit_list *parents, unsigned char *ret,
const char *author, const char *sign_commit)
 {
@@ -1352,7 +1353,7 @@ int commit_tree(const struct strbuf *msg, const unsigned 
char *tree,
int result;
 
append_merge_tag_headers(parents, tail);
-   result = commit_tree_extended(msg, tree, parents, ret,
+   result = commit_tree_extended(msg, msg_len, tree, parents, ret,
  author, sign_commit, extra);
free_commit_extra_headers(extra);
return result;
@@ -1473,7 +1474,8 @@ static const char commit_utf8_warn[] =
 You may want to amend it after fixing the message, or set the config\n
 variable i18n.commitencoding to the encoding your project uses.\n;
 
-int commit_tree_extended(const struct strbuf *msg, const unsigned char *tree,
+int 

[PATCH 02/17] replace dangerous uses of strbuf_attach

[Jeff King]

It is not a good idea to strbuf_attach an arbitrary pointer
just because a function you are calling wants a strbuf.
Attaching implies a transfer of memory ownership; if anyone
were to modify or release the resulting strbuf, we would
free() the pointer, leading to possible problems:

  1. Other users of the original pointer might access freed
 memory.

  2. The pointer might not be the start of a malloc'd
 area, so calling free() on it in the first place would
 be wrong.

In the two cases modified here, we are fortunate that nobody
touches the strbuf once it is attached, but it is an
accident waiting to happen.  Since the previous commit,
commit_tree and friends take a pointer/buf pair, so we can
just do away with the strbufs entirely.

Signed-off-by: Jeff King p...@peff.net
---
I also manually checked the other dozen or so calls to strbuf_attach,
and they all look good.

 notes-cache.c | 6 ++
 notes-merge.c | 5 +
 2 files changed, 3 insertions(+), 8 deletions(-)

diff --git a/notes-cache.c b/notes-cache.c
index 4ad0799..c4e9bb7 100644
--- a/notes-cache.c
+++ b/notes-cache.c
@@ -48,7 +48,6 @@ int notes_cache_write(struct notes_cache *c)
 {
unsigned char tree_sha1[20];
unsigned char commit_sha1[20];
-   struct strbuf msg = STRBUF_INIT;
 
if (!c || !c-tree.initialized || !c-tree.ref || !*c-tree.ref)
return -1;
@@ -57,9 +56,8 @@ int notes_cache_write(struct notes_cache *c)
 
if (write_notes_tree(c-tree, tree_sha1))
return -1;
-   strbuf_attach(msg, c-validity,
- strlen(c-validity), strlen(c-validity) + 1);
-   if (commit_tree(msg.buf, msg.len, tree_sha1, NULL, commit_sha1, NULL, 
NULL)  0)
+   if (commit_tree(c-validity, strlen(c-validity), tree_sha1, NULL,
+   commit_sha1, NULL, NULL)  0)
return -1;
if (update_ref(update notes cache, c-tree.ref, commit_sha1, NULL,
   0, UPDATE_REFS_QUIET_ON_ERR)  0)
diff --git a/notes-merge.c b/notes-merge.c
index 9d94210..697cec3 100644
--- a/notes-merge.c
+++ b/notes-merge.c
@@ -673,7 +673,6 @@ int notes_merge_commit(struct notes_merge_options *o,
struct dirent *e;
struct strbuf path = STRBUF_INIT;
char *msg = strstr(partial_commit-buffer, \n\n);
-   struct strbuf sb_msg = STRBUF_INIT;
int baselen;
 
strbuf_addstr(path, git_path(NOTES_MERGE_WORKTREE));
@@ -720,10 +719,8 @@ int notes_merge_commit(struct notes_merge_options *o,
strbuf_setlen(path, baselen);
}
 
-   strbuf_attach(sb_msg, msg, strlen(msg), strlen(msg) + 1);
create_notes_commit(partial_tree, partial_commit-parents,
-   sb_msg.buf, sb_msg.len,
-   result_sha1);
+   msg, strlen(msg), result_sha1);
if (o-verbosity = 4)
printf(Finalized notes merge commit: %s\n,
sha1_to_hex(result_sha1));
-- 
2.0.0.729.g520999f

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

[PATCH 03/17] alloc: include any-object allocations in alloc_report

[Jeff King]

When 2c1cbec (Use proper object allocators for unknown
object nodes too, 2007-04-16), added a special any_object
allocator, it never taught alloc_report to report on it. To
do so we need to add an extra type argument to the REPORT
macro, as that commit did for DEFINE_ALLOCATOR.

Signed-off-by: Jeff King p...@peff.net
---
 alloc.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/alloc.c b/alloc.c
index f3ee745..38ff7e7 100644
--- a/alloc.c
+++ b/alloc.c
@@ -57,13 +57,14 @@ static void report(const char *name, unsigned int count, 
size_t size)
name, count, (uintmax_t) size);
 }
 
-#define REPORT(name)   \
-report(#name, name##_allocs, name##_allocs * sizeof(struct name)  10)
+#define REPORT(name, type) \
+report(#name, name##_allocs, name##_allocs * sizeof(type)  10)
 
 void alloc_report(void)
 {
-   REPORT(blob);
-   REPORT(tree);
-   REPORT(commit);
-   REPORT(tag);
+   REPORT(blob, struct blob);
+   REPORT(tree, struct tree);
+   REPORT(commit, struct commit);
+   REPORT(tag, struct tag);
+   REPORT(object, union any_object);
 }
-- 
2.0.0.729.g520999f

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

[PATCH 05/17] do not create struct commit with xcalloc

[Jeff King]

In both blame and merge-recursive, we sometimes create a
fake commit struct for convenience (e.g., to represent the
HEAD state as if we would commit it). By allocating
ourselves rather than using alloc_commit_node, we do not
properly set the index field of the commit. This can
produce subtle bugs if we then use commit-slab on the
resulting commit, as we will share the 0 index with
another commit.

We can fix this by using alloc_commit_node() to allocate.
Note that we cannot free the result, as it is part of our
commit allocator. However, both cases were already leaking
the allocated commit anyway, so there's nothing to fix up.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/blame.c   | 2 +-
 merge-recursive.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index a52a279..d6056a5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2286,7 +2286,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
struct strbuf msg = STRBUF_INIT;
 
time(now);
-   commit = xcalloc(1, sizeof(*commit));
+   commit = alloc_commit_node();
commit-object.parsed = 1;
commit-date = now;
commit-object.type = OBJ_COMMIT;
diff --git a/merge-recursive.c b/merge-recursive.c
index cab16fa..2b37d42 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -40,7 +40,7 @@ static struct tree *shift_tree_object(struct tree *one, 
struct tree *two,
 
 static struct commit *make_virtual_commit(struct tree *tree, const char 
*comment)
 {
-   struct commit *commit = xcalloc(1, sizeof(struct commit));
+   struct commit *commit = alloc_commit_node();
struct merge_remote_desc *desc = xmalloc(sizeof(*desc));
 
desc-name = comment;
-- 
2.0.0.729.g520999f

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

[PATCH 04/17] commit: push commit_index update into alloc_commit_node

[Jeff King]

Whenever we create a commit object via lookup_commit, we
give it a unique index to be used with the commit-slab API.
The theory is that any struct commit we create would
follow this code path, so any such struct would get an
index. However, callers could use alloc_commit_node()
directly (and get multiple commits with index 0).

Let's push the indexing into alloc_commit_node so that it's
hard for callers to get it wrong.

Signed-off-by: Jeff King p...@peff.net
---
 alloc.c  | 12 ++--
 commit.c |  2 --
 2 files changed, 10 insertions(+), 4 deletions(-)

diff --git a/alloc.c b/alloc.c
index 38ff7e7..eb22a45 100644
--- a/alloc.c
+++ b/alloc.c
@@ -47,10 +47,18 @@ union any_object {
 
 DEFINE_ALLOCATOR(blob, struct blob)
 DEFINE_ALLOCATOR(tree, struct tree)
-DEFINE_ALLOCATOR(commit, struct commit)
+DEFINE_ALLOCATOR(raw_commit, struct commit)
 DEFINE_ALLOCATOR(tag, struct tag)
 DEFINE_ALLOCATOR(object, union any_object)
 
+void *alloc_commit_node(void)
+{
+   static int commit_count;
+   struct commit *c = alloc_raw_commit_node();
+   c-index = commit_count++;
+   return c;
+}
+
 static void report(const char *name, unsigned int count, size_t size)
 {
fprintf(stderr, %10s: %8u (%PRIuMAX kB)\n,
@@ -64,7 +72,7 @@ void alloc_report(void)
 {
REPORT(blob, struct blob);
REPORT(tree, struct tree);
-   REPORT(commit, struct commit);
+   REPORT(raw_commit, struct commit);
REPORT(tag, struct tag);
REPORT(object, union any_object);
 }
diff --git a/commit.c b/commit.c
index bd3d5af..fbdc480 100644
--- a/commit.c
+++ b/commit.c
@@ -17,7 +17,6 @@ static struct commit_extra_header 
*read_commit_extra_header_lines(const char *bu
 int save_commit_buffer = 1;
 
 const char *commit_type = commit;
-static int commit_count;
 
 static struct commit *check_commit(struct object *obj,
   const unsigned char *sha1,
@@ -64,7 +63,6 @@ struct commit *lookup_commit(const unsigned char *sha1)
struct object *obj = lookup_object(sha1);
if (!obj) {
struct commit *c = alloc_commit_node();
-   c-index = commit_count++;
return create_object(sha1, OBJ_COMMIT, c);
}
if (!obj-type)
-- 
2.0.0.729.g520999f

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

[PATCH 07/17] sequencer: use logmsg_reencode in get_message

[Jeff King]

This simplifies the code, as logmsg_reencode handles the
reencoding for us in a single call. It also means we learn
logmsg_reencode's trick of pulling the buffer from disk when
commit-buffer is NULL (we currently just silently return!).
It is doubtful this matters in practice, though, as
sequencer operations would not generally turn off
save_commit_buffer.

Note that we may be fixing a bug here. The existing code
does:

  if (same_encoding(to, from))
  reencode_string(buf, to, from);

That probably should have been !same_encoding.

Signed-off-by: Jeff King p...@peff.net
---
 sequencer.c | 45 +
 1 file changed, 5 insertions(+), 40 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 0a80c58..3fcab4d 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -116,39 +116,23 @@ static const char *action_name(const struct replay_opts 
*opts)
return opts-action == REPLAY_REVERT ? revert : cherry-pick;
 }
 
-static char *get_encoding(const char *message);
-
 struct commit_message {
char *parent_label;
const char *label;
const char *subject;
-   char *reencoded_message;
const char *message;
 };
 
 static int get_message(struct commit *commit, struct commit_message *out)
 {
-   const char *encoding;
const char *abbrev, *subject;
int abbrev_len, subject_len;
char *q;
 
-   if (!commit-buffer)
-   return -1;
-   encoding = get_encoding(commit-buffer);
-   if (!encoding)
-   encoding = UTF-8;
if (!git_commit_encoding)
git_commit_encoding = UTF-8;
 
-   out-reencoded_message = NULL;
-   out-message = commit-buffer;
-   if (same_encoding(encoding, git_commit_encoding))
-   out-reencoded_message = reencode_string(commit-buffer,
-   git_commit_encoding, encoding);
-   if (out-reencoded_message)
-   out-message = out-reencoded_message;
-
+   out-message = logmsg_reencode(commit, NULL, git_commit_encoding);
abbrev = find_unique_abbrev(commit-object.sha1, DEFAULT_ABBREV);
abbrev_len = strlen(abbrev);
 
@@ -167,29 +151,10 @@ static int get_message(struct commit *commit, struct 
commit_message *out)
return 0;
 }
 
-static void free_message(struct commit_message *msg)
+static void free_message(struct commit *commit, struct commit_message *msg)
 {
free(msg-parent_label);
-   free(msg-reencoded_message);
-}
-
-static char *get_encoding(const char *message)
-{
-   const char *p = message, *eol;
-
-   while (*p  *p != '\n') {
-   for (eol = p + 1; *eol  *eol != '\n'; eol++)
-   ; /* do nothing */
-   if (starts_with(p, encoding )) {
-   char *result = xmalloc(eol - 8 - p);
-   strlcpy(result, p + 9, eol - 8 - p);
-   return result;
-   }
-   p = eol;
-   if (*p == '\n')
-   p++;
-   }
-   return NULL;
+   logmsg_free(msg-message, commit);
 }
 
 static void write_cherry_pick_head(struct commit *commit, const char 
*pseudoref)
@@ -489,7 +454,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
unsigned char head[20];
struct commit *base, *next, *parent;
const char *base_label, *next_label;
-   struct commit_message msg = { NULL, NULL, NULL, NULL, NULL };
+   struct commit_message msg = { NULL, NULL, NULL, NULL };
char *defmsg = NULL;
struct strbuf msgbuf = STRBUF_INIT;
int res, unborn = 0, allow;
@@ -654,7 +619,7 @@ static int do_pick_commit(struct commit *commit, struct 
replay_opts *opts)
res = run_git_commit(defmsg, opts, allow);
 
 leave:
-   free_message(msg);
+   free_message(commit, msg);
free(defmsg);
 
return res;
-- 
2.0.0.729.g520999f

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

[PATCH 06/17] logmsg_reencode: return const buffer

[Jeff King]

The return value from logmsg_reencode may be either a newly
allocated buffer or a pointer to the existing commit-buffer.
We would not want the caller to accidentally free() or
modify the latter, so let's mark it as const.  We can cast
away the constness in logmsg_free, but only once we have
determined that it is a free-able buffer.

Signed-off-by: Jeff King p...@peff.net
---
Fixed comment style issue from v1.

 builtin/blame.c |  2 +-
 builtin/reset.c |  2 +-
 commit.h|  8 
 pretty.c| 14 +++---
 revision.c  | 13 ++---
 5 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index d6056a5..6ce3c6d 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1655,7 +1655,7 @@ static void get_commit_info(struct commit *commit,
 {
int len;
const char *subject, *encoding;
-   char *message;
+   const char *message;
 
commit_info_init(ret);
 
diff --git a/builtin/reset.c b/builtin/reset.c
index f368266..7ebee07 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -93,7 +93,7 @@ static int reset_index(const unsigned char *sha1, int 
reset_type, int quiet)
 static void print_new_head_line(struct commit *commit)
 {
const char *hex, *body;
-   char *msg;
+   const char *msg;
 
hex = find_unique_abbrev(commit-object.sha1, DEFAULT_ABBREV);
printf(_(HEAD is now at %s), hex);
diff --git a/commit.h b/commit.h
index 1cb55ae..4df48cb 100644
--- a/commit.h
+++ b/commit.h
@@ -115,10 +115,10 @@ struct userformat_want {
 
 extern int has_non_ascii(const char *text);
 struct rev_info; /* in revision.h, it circularly uses enum cmit_fmt */
-extern char *logmsg_reencode(const struct commit *commit,
-char **commit_encoding,
-const char *output_encoding);
-extern void logmsg_free(char *msg, const struct commit *commit);
+extern const char *logmsg_reencode(const struct commit *commit,
+  char **commit_encoding,
+  const char *output_encoding);
+extern void logmsg_free(const char *msg, const struct commit *commit);
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
  const char *line_separator);
diff --git a/pretty.c b/pretty.c
index e1e2cad..d152de2 100644
--- a/pretty.c
+++ b/pretty.c
@@ -606,9 +606,9 @@ static char *replace_encoding_header(char *buf, const char 
*encoding)
return strbuf_detach(tmp, NULL);
 }
 
-char *logmsg_reencode(const struct commit *commit,
- char **commit_encoding,
- const char *output_encoding)
+const char *logmsg_reencode(const struct commit *commit,
+   char **commit_encoding,
+   const char *output_encoding)
 {
static const char *utf8 = UTF-8;
const char *use_encoding;
@@ -687,10 +687,10 @@ char *logmsg_reencode(const struct commit *commit,
return out ? out : msg;
 }
 
-void logmsg_free(char *msg, const struct commit *commit)
+void logmsg_free(const char *msg, const struct commit *commit)
 {
if (msg != commit-buffer)
-   free(msg);
+   free((void *)msg);
 }
 
 static int mailmap_name(const char **email, size_t *email_len,
@@ -796,7 +796,7 @@ struct format_commit_context {
struct signature_check signature_check;
enum flush_type flush_type;
enum trunc_type truncate;
-   char *message;
+   const char *message;
char *commit_encoding;
size_t width, indent1, indent2;
int auto_color;
@@ -1700,7 +1700,7 @@ void pretty_print_commit(struct pretty_print_context *pp,
unsigned long beginning_of_body;
int indent = 4;
const char *msg;
-   char *reencoded;
+   const char *reencoded;
const char *encoding;
int need_8bit_cte = pp-need_8bit_cte;
 
diff --git a/revision.c b/revision.c
index 71e2337..be151ef 100644
--- a/revision.c
+++ b/revision.c
@@ -2788,7 +2788,7 @@ static int commit_match(struct commit *commit, struct 
rev_info *opt)
 {
int retval;
const char *encoding;
-   char *message;
+   const char *message;
struct strbuf buf = STRBUF_INIT;
 
if (!opt-grep_filter.pattern_list  !opt-grep_filter.header_list)
@@ -2830,12 +2830,19 @@ static int commit_match(struct commit *commit, struct 
rev_info *opt)
format_display_notes(commit-object.sha1, buf, encoding, 1);
}
 
-   /* Find either in the original commit message, or in the temporary */
+   /*
+* Find either in the original commit message, or in the temporary.
+* Note that we cast away the constness of message here. It is
+* const because it may come from the cached commit buffer. That's OK,
+* because we know that it is modifiable heap 

Re: [PATCH 05/15] sequencer: use logmsg_reencode in get_message

[Junio C Hamano]

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

 Note that we may be fixing a bug here. The existing code
 does:

   if (same_encoding(to, from))
 reencode_string(buf, to, from);

 That probably should have been !same_encoding.

 Signed-off-by: Jeff King p...@peff.net
 ---
 I didn't actually test for the bug, so it's possible that I'm missing
 something clever...

Thanks for spotting.  There is nothing clever going on.

0e18bcd5 (reencode_string(): introduce and use same_encoding(),
2012-10-18) has this misconversion.

diff --git a/sequencer.c b/sequencer.c
index bd62680..f2f5b13 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -58,7 +58,7 @@ static int get_message(struct commit *commit, struct 
commit_message *out)
 
out-reencoded_message = NULL;
out-message = commit-buffer;
-   if (strcmp(encoding, git_commit_encoding))
+   if (same_encoding(encoding, git_commit_encoding))
out-reencoded_message = reencode_string(commit-buffer,
git_commit_encoding, encoding);
if (out-reencoded_message)

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

[PATCH 11/17] use get_cached_commit_buffer where appropriate

[Jeff King]

Some call sites check commit-buffer to see whether we have
a cached buffer, and if so, do some work with it. In the
long run we may want to switch these code paths to make
their decision on a different boolean flag (because checking
the cache may get a little more expensive in the future).
But for now, we can easily support them by converting the
calls to use get_cached_commit_buffer.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/rev-list.c | 2 +-
 log-tree.c | 2 +-
 object.c   | 2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index e012ebe..3fcbf21 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -106,7 +106,7 @@ static void show_commit(struct commit *commit, void *data)
else
putchar('\n');
 
-   if (revs-verbose_header  commit-buffer) {
+   if (revs-verbose_header  get_cached_commit_buffer(commit)) {
struct strbuf buf = STRBUF_INIT;
struct pretty_print_context ctx = {0};
ctx.abbrev = revs-abbrev;
diff --git a/log-tree.c b/log-tree.c
index cf2f86c..e9ef8ab 100644
--- a/log-tree.c
+++ b/log-tree.c
@@ -588,7 +588,7 @@ void show_log(struct rev_info *opt)
show_mergetag(opt, commit);
}
 
-   if (!commit-buffer)
+   if (!get_cached_commit_buffer(commit))
return;
 
if (opt-show_notes) {
diff --git a/object.c b/object.c
index 44ca657..67b6e35 100644
--- a/object.c
+++ b/object.c
@@ -197,7 +197,7 @@ struct object *parse_object_buffer(const unsigned char 
*sha1, enum object_type t
if (commit) {
if (parse_commit_buffer(commit, buffer, size))
return NULL;
-   if (!commit-buffer) {
+   if (!get_cached_commit_buffer(commit)) {
set_commit_buffer(commit, buffer);
*eaten_p = 1;
}
-- 
2.0.0.729.g520999f

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

[PATCH 10/17] provide helpers to access the commit buffer

[Jeff King]

Many sites look at commit-buffer to get more detailed
information than what is in the parsed commit struct.
However, we sometimes drop commit-buffer to save memory,
in which case the caller would need to read the object
afresh. Some callers do this (leading to duplicated code),
and others do not (which opens the possibility of a segfault
if somebody else frees the buffer).

Let's provide a pair of helpers, get and unuse, that let
callers easily get the buffer. They will use the cached
buffer when possible, and otherwise load from disk using
read_sha1_file.

Note that we also need to add a get_cached variant which
returns NULL when we do not have a cached buffer. At first
glance this seems to defeat the purpose of get, which is
to always provide a return value. However, some log code
paths actually use the NULL-ness of commit-buffer as a
boolean flag to decide whether to try printing the
commit. At least for now, we want to continue supporting
that use.

Signed-off-by: Jeff King p...@peff.net
---
Typofix in commit message since v1.

 commit.c | 28 
 commit.h | 21 +
 2 files changed, 49 insertions(+)

diff --git a/commit.c b/commit.c
index fc8b4e2..b6b0e0d 100644
--- a/commit.c
+++ b/commit.c
@@ -250,6 +250,34 @@ void set_commit_buffer(struct commit *commit, void *buffer)
commit-buffer = buffer;
 }
 
+const void *get_cached_commit_buffer(const struct commit *commit)
+{
+   return commit-buffer;
+}
+
+const void *get_commit_buffer(const struct commit *commit)
+{
+   const void *ret = get_cached_commit_buffer(commit);
+   if (!ret) {
+   enum object_type type;
+   unsigned long size;
+   ret = read_sha1_file(commit-object.sha1, type, size);
+   if (!ret)
+   die(cannot read commit object %s,
+   sha1_to_hex(commit-object.sha1));
+   if (type != OBJ_COMMIT)
+   die(expected commit for %s, got %s,
+   sha1_to_hex(commit-object.sha1), typename(type));
+   }
+   return ret;
+}
+
+void unuse_commit_buffer(const struct commit *commit, const void *buffer)
+{
+   if (commit-buffer != buffer)
+   free((void *)buffer);
+}
+
 void free_commit_buffer(struct commit *commit)
 {
free(commit-buffer);
diff --git a/commit.h b/commit.h
index cc89128..259c0ae 100644
--- a/commit.h
+++ b/commit.h
@@ -58,6 +58,27 @@ void parse_commit_or_die(struct commit *item);
 void set_commit_buffer(struct commit *, void *buffer);
 
 /*
+ * Get any cached object buffer associated with the commit. Returns NULL
+ * if none. The resulting memory should not be freed.
+ */
+const void *get_cached_commit_buffer(const struct commit *);
+
+/*
+ * Get the commit's object contents, either from cache or by reading the object
+ * from disk. The resulting memory should not be modified, and must be given
+ * to unuse_commit_buffer when the caller is done.
+ */
+const void *get_commit_buffer(const struct commit *);
+
+/*
+ * Tell the commit subsytem that we are done with a particular commit buffer.
+ * The commit and buffer should be the input and return value, respectively,
+ * from an earlier call to get_commit_buffer.  The buffer may or may not be
+ * freed by this call; callers should not access the memory afterwards.
+ */
+void unuse_commit_buffer(const struct commit *, const void *buffer);
+
+/*
  * Free any cached object buffer associated with the commit.
  */
 void free_commit_buffer(struct commit *);
-- 
2.0.0.729.g520999f

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

[PATCH 08/17] provide a helper to free commit buffer

[Jeff King]

This converts two lines into one at each caller. But more
importantly, it abstracts the concept of freeing the buffer,
which will make it easier to change later.

Note that we also need to provide a detach mechanism for
the weird case in fsck which passes the buffer back to be
freed.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/fsck.c   |  3 +--
 builtin/index-pack.c |  2 +-
 builtin/log.c|  6 ++
 builtin/rev-list.c   |  3 +--
 commit.c | 13 +
 commit.h | 11 +++
 6 files changed, 29 insertions(+), 9 deletions(-)

diff --git a/builtin/fsck.c b/builtin/fsck.c
index fc150c8..8aadca1 100644
--- a/builtin/fsck.c
+++ b/builtin/fsck.c
@@ -310,8 +310,7 @@ static int fsck_obj(struct object *obj)
if (obj-type == OBJ_COMMIT) {
struct commit *commit = (struct commit *) obj;
 
-   free(commit-buffer);
-   commit-buffer = NULL;
+   free_commit_buffer(commit);
 
if (!commit-parents  show_root)
printf(root %s\n, sha1_to_hex(commit-object.sha1));
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 18f57de..995df39 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct 
object_entry *obj_entry,
}
if (obj-type == OBJ_COMMIT) {
struct commit *commit = (struct commit *) obj;
-   commit-buffer = NULL;
+   detach_commit_buffer(commit);
}
obj-flags |= FLAG_CHECKED;
}
diff --git a/builtin/log.c b/builtin/log.c
index 39e8836..226f8f2 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -349,8 +349,7 @@ static int cmd_log_walk(struct rev_info *rev)
rev-max_count++;
if (!rev-reflog_info) {
/* we allow cycles in reflog ancestry */
-   free(commit-buffer);
-   commit-buffer = NULL;
+   free_commit_buffer(commit);
}
free_commit_list(commit-parents);
commit-parents = NULL;
@@ -1508,8 +1507,7 @@ int cmd_format_patch(int argc, const char **argv, const 
char *prefix)
reopen_stdout(rev.numbered_files ? NULL : commit, NULL, 
rev, quiet))
die(_(Failed to create output files));
shown = log_tree_commit(rev, commit);
-   free(commit-buffer);
-   commit-buffer = NULL;
+   free_commit_buffer(commit);
 
/* We put one extra blank line between formatted
 * patches and this flag is used by log-tree code
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 9f92905..e012ebe 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -173,8 +173,7 @@ static void finish_commit(struct commit *commit, void *data)
free_commit_list(commit-parents);
commit-parents = NULL;
}
-   free(commit-buffer);
-   commit-buffer = NULL;
+   free_commit_buffer(commit);
 }
 
 static void finish_object(struct object *obj,
diff --git a/commit.c b/commit.c
index fbdc480..11a05c1 100644
--- a/commit.c
+++ b/commit.c
@@ -245,6 +245,19 @@ int unregister_shallow(const unsigned char *sha1)
return 0;
 }
 
+void free_commit_buffer(struct commit *commit)
+{
+   free(commit-buffer);
+   commit-buffer = NULL;
+}
+
+const void *detach_commit_buffer(struct commit *commit)
+{
+   void *ret = commit-buffer;
+   commit-buffer = NULL;
+   return ret;
+}
+
 int parse_commit_buffer(struct commit *item, const void *buffer, unsigned long 
size)
 {
const char *tail = buffer;
diff --git a/commit.h b/commit.h
index 4df48cb..d72ed43 100644
--- a/commit.h
+++ b/commit.h
@@ -51,6 +51,17 @@ int parse_commit_buffer(struct commit *item, const void 
*buffer, unsigned long s
 int parse_commit(struct commit *item);
 void parse_commit_or_die(struct commit *item);
 
+/*
+ * Free any cached object buffer associated with the commit.
+ */
+void free_commit_buffer(struct commit *);
+
+/*
+ * Disassociate any cached object buffer from the commit, but do not free it.
+ * The buffer (or NULL, if none) is returned.
+ */
+const void *detach_commit_buffer(struct commit *);
+
 /* Find beginning and length of commit subject. */
 int find_commit_subject(const char *commit_buffer, const char **subject);
 
-- 
2.0.0.729.g520999f

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

[PATCH 09/17] provide a helper to set the commit buffer

[Jeff King]

Right now this is just a one-liner, but abstracting it will
make it easier to change later.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/blame.c | 2 +-
 commit.c| 7 ++-
 commit.h| 6 ++
 object.c| 2 +-
 4 files changed, 14 insertions(+), 3 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 6ce3c6d..0af3a18 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2313,7 +2313,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
ident, ident, path,
(!contents_from ? path :
 (!strcmp(contents_from, -) ? standard input : 
contents_from)));
-   commit-buffer = strbuf_detach(msg, NULL);
+   set_commit_buffer(commit, strbuf_detach(msg, NULL));
 
if (!contents_from || strcmp(-, contents_from)) {
struct stat st;
diff --git a/commit.c b/commit.c
index 11a05c1..fc8b4e2 100644
--- a/commit.c
+++ b/commit.c
@@ -245,6 +245,11 @@ int unregister_shallow(const unsigned char *sha1)
return 0;
 }
 
+void set_commit_buffer(struct commit *commit, void *buffer)
+{
+   commit-buffer = buffer;
+}
+
 void free_commit_buffer(struct commit *commit)
 {
free(commit-buffer);
@@ -335,7 +340,7 @@ int parse_commit(struct commit *item)
}
ret = parse_commit_buffer(item, buffer, size);
if (save_commit_buffer  !ret) {
-   item-buffer = buffer;
+   set_commit_buffer(item, buffer);
return 0;
}
free(buffer);
diff --git a/commit.h b/commit.h
index d72ed43..cc89128 100644
--- a/commit.h
+++ b/commit.h
@@ -52,6 +52,12 @@ int parse_commit(struct commit *item);
 void parse_commit_or_die(struct commit *item);
 
 /*
+ * Associate an object buffer with the commit. The ownership of the
+ * memory is handed over to the commit, and must be free()-able.
+ */
+void set_commit_buffer(struct commit *, void *buffer);
+
+/*
  * Free any cached object buffer associated with the commit.
  */
 void free_commit_buffer(struct commit *);
diff --git a/object.c b/object.c
index 57a0890..44ca657 100644
--- a/object.c
+++ b/object.c
@@ -198,7 +198,7 @@ struct object *parse_object_buffer(const unsigned char 
*sha1, enum object_type t
if (parse_commit_buffer(commit, buffer, size))
return NULL;
if (!commit-buffer) {
-   commit-buffer = buffer;
+   set_commit_buffer(commit, buffer);
*eaten_p = 1;
}
obj = commit-object;
-- 
2.0.0.729.g520999f

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

[PATCH 12/17] use get_commit_buffer to avoid duplicate code

[Jeff King]

For both of these sites, we already do the fallback to
read_sha1_file trick. But we can shorten the code by just
using get_commit_buffer.

Note that the error cases are slightly different when
read_sha1_file fails. get_commit_buffer will die() if the
object cannot be loaded, or is a non-commit.

For get_sha1_oneline, this will almost certainly never
happen, as we will have just called parse_object (and if it
does, it's probably worth complaining about).

For record_author_date, the new behavior is probably better;
we notify the user of the error instead of silently ignoring
it. And because it's used only for sorting by author-date,
somebody examining a corrupt repo can fallback to the
regular traversal order.

Signed-off-by: Jeff King p...@peff.net
---
Typofix in commit message since v1.

 commit.c| 16 +++-
 sha1_name.c | 18 --
 2 files changed, 7 insertions(+), 27 deletions(-)

diff --git a/commit.c b/commit.c
index b6b0e0d..1903dde 100644
--- a/commit.c
+++ b/commit.c
@@ -583,22 +583,12 @@ static void record_author_date(struct author_date_slab 
*author_date,
   struct commit *commit)
 {
const char *buf, *line_end, *ident_line;
-   char *buffer = NULL;
+   const char *buffer = get_commit_buffer(commit);
struct ident_split ident;
char *date_end;
unsigned long date;
 
-   if (!commit-buffer) {
-   unsigned long size;
-   enum object_type type;
-   buffer = read_sha1_file(commit-object.sha1, type, size);
-   if (!buffer)
-   return;
-   }
-
-   for (buf = commit-buffer ? commit-buffer : buffer;
-buf;
-buf = line_end + 1) {
+   for (buf = buffer; buf; buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
ident_line = skip_prefix(buf, author );
if (!ident_line) {
@@ -619,7 +609,7 @@ static void record_author_date(struct author_date_slab 
*author_date,
*(author_date_slab_at(author_date, commit)) = date;
 
 fail_exit:
-   free(buffer);
+   unuse_commit_buffer(commit, buffer);
 }
 
 static int compare_commits_by_author_date(const void *a_, const void *b_,
diff --git a/sha1_name.c b/sha1_name.c
index 2b6322f..0a65d23 100644
--- a/sha1_name.c
+++ b/sha1_name.c
@@ -862,27 +862,17 @@ static int get_sha1_oneline(const char *prefix, unsigned 
char *sha1,
commit_list_insert(l-item, backup);
}
while (list) {
-   char *p, *to_free = NULL;
+   const char *p, *buf;
struct commit *commit;
-   enum object_type type;
-   unsigned long size;
int matches;
 
commit = pop_most_recent_commit(list, ONELINE_SEEN);
if (!parse_object(commit-object.sha1))
continue;
-   if (commit-buffer)
-   p = commit-buffer;
-   else {
-   p = read_sha1_file(commit-object.sha1, type, size);
-   if (!p)
-   continue;
-   to_free = p;
-   }
-
-   p = strstr(p, \n\n);
+   buf = get_commit_buffer(commit);
+   p = strstr(buf, \n\n);
matches = p  !regexec(regex, p + 2, 0, NULL, 0);
-   free(to_free);
+   unuse_commit_buffer(commit, buf);
 
if (matches) {
hashcpy(sha1, commit-object.sha1);
-- 
2.0.0.729.g520999f

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

[PATCH 13/17] convert logmsg_reencode to get_commit_buffer

[Jeff King]

Like the callsites in the previous commit, logmsg_reencode
already falls back to read_sha1_file when necessary.
However, I split its conversion out into its own commit
because it's a bit more complex.

We return either:

  1. The original commit-buffer

  2. A newly allocated buffer from read_sha1_file

  3. A reencoded buffer (based on either 1 or 2 above).

while trying to do as few extra reads/allocations as
possible. Callers currently free the result with
logmsg_free, but we can simplify this by pointing them
straight to unuse_commit_buffer. This is a slight layering
violation, in that we may be passing a buffer from (3).
However, since the end result is to free() anything except
(1), which is unlikely to change, and because this makes the
interface much simpler, it's a reasonable bending of the
rules.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/blame.c |  4 ++--
 builtin/reset.c |  2 +-
 commit.h|  1 -
 pretty.c| 40 +++-
 revision.c  |  2 +-
 sequencer.c |  2 +-
 6 files changed, 16 insertions(+), 35 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index 0af3a18..cde19eb 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1666,7 +1666,7 @@ static void get_commit_info(struct commit *commit,
ret-author_time, ret-author_tz);
 
if (!detailed) {
-   logmsg_free(message, commit);
+   unuse_commit_buffer(commit, message);
return;
}
 
@@ -1680,7 +1680,7 @@ static void get_commit_info(struct commit *commit,
else
strbuf_addf(ret-summary, (%s), 
sha1_to_hex(commit-object.sha1));
 
-   logmsg_free(message, commit);
+   unuse_commit_buffer(commit, message);
 }
 
 /*
diff --git a/builtin/reset.c b/builtin/reset.c
index 7ebee07..850d532 100644
--- a/builtin/reset.c
+++ b/builtin/reset.c
@@ -109,7 +109,7 @@ static void print_new_head_line(struct commit *commit)
}
else
printf(\n);
-   logmsg_free(msg, commit);
+   unuse_commit_buffer(commit, msg);
 }
 
 static void update_index_from_diff(struct diff_queue_struct *q,
diff --git a/commit.h b/commit.h
index 259c0ae..5ce5ce7 100644
--- a/commit.h
+++ b/commit.h
@@ -156,7 +156,6 @@ struct rev_info; /* in revision.h, it circularly uses enum 
cmit_fmt */
 extern const char *logmsg_reencode(const struct commit *commit,
   char **commit_encoding,
   const char *output_encoding);
-extern void logmsg_free(const char *msg, const struct commit *commit);
 extern void get_commit_format(const char *arg, struct rev_info *);
 extern const char *format_subject(struct strbuf *sb, const char *msg,
  const char *line_separator);
diff --git a/pretty.c b/pretty.c
index d152de2..8fd60cd 100644
--- a/pretty.c
+++ b/pretty.c
@@ -613,22 +613,9 @@ const char *logmsg_reencode(const struct commit *commit,
static const char *utf8 = UTF-8;
const char *use_encoding;
char *encoding;
-   char *msg = commit-buffer;
+   const char *msg = get_commit_buffer(commit);
char *out;
 
-   if (!msg) {
-   enum object_type type;
-   unsigned long size;
-
-   msg = read_sha1_file(commit-object.sha1, type, size);
-   if (!msg)
-   die(Cannot read commit object %s,
-   sha1_to_hex(commit-object.sha1));
-   if (type != OBJ_COMMIT)
-   die(Expected commit for '%s', got %s,
-   sha1_to_hex(commit-object.sha1), typename(type));
-   }
-
if (!output_encoding || !*output_encoding) {
if (commit_encoding)
*commit_encoding =
@@ -652,12 +639,13 @@ const char *logmsg_reencode(const struct commit *commit,
 * Otherwise, we still want to munge the encoding header in the
 * result, which will be done by modifying the buffer. If we
 * are using a fresh copy, we can reuse it. But if we are using
-* the cached copy from commit-buffer, we need to duplicate it
-* to avoid munging commit-buffer.
+* the cached copy from get_commit_buffer, we need to duplicate 
it
+* to avoid munging the cached copy.
 */
-   out = msg;
-   if (out == commit-buffer)
-   out = xstrdup(out);
+   if (msg == get_cached_commit_buffer(commit))
+   out = xstrdup(msg);
+   else
+   out = (char *)msg;
}
else {
/*
@@ -667,8 +655,8 @@ const char *logmsg_reencode(const struct commit *commit,
 * copy, we can free it.
 */
out = reencode_string(msg, output_encoding, use_encoding);
-   if 

[PATCH 14/17] use get_commit_buffer everywhere

[Jeff King]

Each of these sites assumes that commit-buffer is valid.
Since they would segfault if this was not the case, they are
likely to be correct in practice. However, we can
future-proof them by using get_commit_buffer.

And as a side effect, we abstract away the final bare uses
of commit-buffer.

Signed-off-by: Jeff King p...@peff.net
---
 builtin/fast-export.c   |  5 -
 builtin/fmt-merge-msg.c |  5 -
 builtin/log.c   |  7 +--
 fsck.c  | 13 +++--
 merge-recursive.c   |  4 +++-
 notes-merge.c   |  4 +++-
 sequencer.c |  4 +++-
 7 files changed, 33 insertions(+), 9 deletions(-)

diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index b8d8a3a..7ee5e08 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -279,6 +279,7 @@ static const char *find_encoding(const char *begin, const 
char *end)
 static void handle_commit(struct commit *commit, struct rev_info *rev)
 {
int saved_output_format = rev-diffopt.output_format;
+   const char *commit_buffer;
const char *author, *author_end, *committer, *committer_end;
const char *encoding, *message;
char *reencoded = NULL;
@@ -288,7 +289,8 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev)
rev-diffopt.output_format = DIFF_FORMAT_CALLBACK;
 
parse_commit_or_die(commit);
-   author = strstr(commit-buffer, \nauthor );
+   commit_buffer = get_commit_buffer(commit);
+   author = strstr(commit_buffer, \nauthor );
if (!author)
die (Could not find author in commit %s,
 sha1_to_hex(commit-object.sha1));
@@ -335,6 +337,7 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev)
  ? strlen(message) : 0),
   reencoded ? reencoded : message ? message : );
free(reencoded);
+   unuse_commit_buffer(commit, commit_buffer);
 
for (i = 0, p = commit-parents; p; p = p-next) {
int mark = get_object_mark(p-item-object);
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 3906eda..01f6d59 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -230,12 +230,14 @@ static void add_branch_desc(struct strbuf *out, const 
char *name)
 static void record_person(int which, struct string_list *people,
  struct commit *commit)
 {
+   const char *buffer;
char *name_buf, *name, *name_end;
struct string_list_item *elem;
const char *field;
 
field = (which == 'a') ? \nauthor  : \ncommitter ;
-   name = strstr(commit-buffer, field);
+   buffer = get_commit_buffer(commit);
+   name = strstr(buffer, field);
if (!name)
return;
name += strlen(field);
@@ -247,6 +249,7 @@ static void record_person(int which, struct string_list 
*people,
if (name_end  name)
return;
name_buf = xmemdupz(name, name_end - name + 1);
+   unuse_commit_buffer(commit, buffer);
 
elem = string_list_lookup(people, name_buf);
if (!elem) {
diff --git a/builtin/log.c b/builtin/log.c
index 226f8f2..2c74260 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -918,9 +918,12 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
log_write_email_headers(rev, head, pp.subject, pp.after_subject,
need_8bit_cte);
 
-   for (i = 0; !need_8bit_cte  i  nr; i++)
-   if (has_non_ascii(list[i]-buffer))
+   for (i = 0; !need_8bit_cte  i  nr; i++) {
+   const char *buf = get_commit_buffer(list[i]);
+   if (has_non_ascii(buf))
need_8bit_cte = 1;
+   unuse_commit_buffer(list[i], buf);
+   }
 
if (!branch_name)
branch_name = find_branch_name(rev);
diff --git a/fsck.c b/fsck.c
index abed62b..8223780 100644
--- a/fsck.c
+++ b/fsck.c
@@ -276,9 +276,10 @@ static int fsck_ident(const char **ident, struct object 
*obj, fsck_error error_f
return 0;
 }
 
-static int fsck_commit(struct commit *commit, fsck_error error_func)
+static int fsck_commit_buffer(struct commit *commit, const char *buffer,
+ fsck_error error_func)
 {
-   const char *buffer = commit-buffer, *tmp;
+   const char *tmp;
unsigned char tree_sha1[20], sha1[20];
struct commit_graft *graft;
int parents = 0;
@@ -336,6 +337,14 @@ static int fsck_commit(struct commit *commit, fsck_error 
error_func)
return 0;
 }
 
+static int fsck_commit(struct commit *commit, fsck_error error_func)
+{
+   const char *buffer = get_commit_buffer(commit);
+   int ret = fsck_commit_buffer(commit, buffer, error_func);
+   unuse_commit_buffer(commit, buffer);
+   return ret;
+}
+
 static int fsck_tag(struct tag *tag, fsck_error error_func)
 {
struct object *tagged = tag-tagged;
diff --git 

[PATCH 16/17] commit: convert commit-buffer to a slab

[Jeff King]

This will make it easier to manage the buffer cache
independently of the struct commit objects. It also
shrinks struct commit by one pointer, which may be
helpful.

Unfortunately it does not reduce the max memory size of
something like rev-list, because rev-list uses
get_cached_commit_buffer() to decide not to show each
commit's output (and due to the design of slab_at, accessing
the slab requires us to extend it, allocating exactly the
same number of buffer pointers we dropped from the commit
structs).

Signed-off-by: Jeff King p...@peff.net
---
 commit.c | 20 +---
 commit.h |  1 -
 2 files changed, 13 insertions(+), 8 deletions(-)

diff --git a/commit.c b/commit.c
index 1903dde..e289c78 100644
--- a/commit.c
+++ b/commit.c
@@ -245,14 +245,17 @@ int unregister_shallow(const unsigned char *sha1)
return 0;
 }
 
+define_commit_slab(buffer_slab, void *);
+static struct buffer_slab buffer_slab = COMMIT_SLAB_INIT(1, buffer_slab);
+
 void set_commit_buffer(struct commit *commit, void *buffer)
 {
-   commit-buffer = buffer;
+   *buffer_slab_at(buffer_slab, commit) = buffer;
 }
 
 const void *get_cached_commit_buffer(const struct commit *commit)
 {
-   return commit-buffer;
+   return *buffer_slab_at(buffer_slab, commit);
 }
 
 const void *get_commit_buffer(const struct commit *commit)
@@ -274,20 +277,23 @@ const void *get_commit_buffer(const struct commit *commit)
 
 void unuse_commit_buffer(const struct commit *commit, const void *buffer)
 {
-   if (commit-buffer != buffer)
+   void *cached = *buffer_slab_at(buffer_slab, commit);
+   if (cached != buffer)
free((void *)buffer);
 }
 
 void free_commit_buffer(struct commit *commit)
 {
-   free(commit-buffer);
-   commit-buffer = NULL;
+   void **b = buffer_slab_at(buffer_slab, commit);
+   free(*b);
+   *b = NULL;
 }
 
 const void *detach_commit_buffer(struct commit *commit)
 {
-   void *ret = commit-buffer;
-   commit-buffer = NULL;
+   void **b = buffer_slab_at(buffer_slab, commit);
+   void *ret = *b;
+   *b = NULL;
return ret;
 }
 
diff --git a/commit.h b/commit.h
index 5ce5ce7..e1c2569 100644
--- a/commit.h
+++ b/commit.h
@@ -20,7 +20,6 @@ struct commit {
unsigned long date;
struct commit_list *parents;
struct tree *tree;
-   char *buffer;
 };
 
 extern int save_commit_buffer;
-- 
2.0.0.729.g520999f

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

[PATCH 15/17] commit-slab: provide a static initializer

[Jeff King]

Callers currently must use init_foo_slab() at runtime before
accessing a slab. For global slabs, it's much nicer if we
can initialize them in BSS, so that each user does not have
to add code to check-and-initialize.

Signed-off-by: Jeff King p...@peff.net
---
There was no comment on this one in v1. I'd be curious if anyone has
comments on what I wrote in:

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

 commit-slab.h | 12 
 1 file changed, 12 insertions(+)

diff --git a/commit-slab.h b/commit-slab.h
index cc114b5..375c9c7 100644
--- a/commit-slab.h
+++ b/commit-slab.h
@@ -117,4 +117,16 @@ static int stat_ ##slabname## realloc
  * catch because GCC silently parses it by default.
  */
 
+/*
+ * Statically initialize a commit slab named var. Note that this
+ * evaluates stride multiple times! Example:
+ *
+ *   struct indegree indegrees = COMMIT_SLAB_INIT(1, indegrees);
+ *
+ */
+#define COMMIT_SLAB_INIT(stride, var) { \
+   COMMIT_SLAB_SIZE / sizeof(**((var).slab)) / (stride), \
+   (stride), 0, NULL \
+}
+
 #endif /* COMMIT_SLAB_H */
-- 
2.0.0.729.g520999f

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

[PATCH 17/17] commit: record buffer length in cache

[Jeff King]

Most callsites which use the commit buffer try to use the
cached version attached to the commit, rather than
re-reading from disk. Unfortunately, that interface provides
only a pointer to the NUL-terminated buffer, with no
indication of the original length.

For the most part, this doesn't matter. People do not put
NULs in their commit messages, and the log code is happy to
treat it all as a NUL-terminated string. However, some code
paths do care. For example, when checking signatures, we
want to be very careful that we verify all the bytes to
avoid malicious trickery.

This patch just adds an optional size out-pointer to
get_commit_buffer and friends. The existing callers all pass
NULL (there did not seem to be any obvious sites where we
could avoid an immediate strlen() call, though perhaps with
some further refactoring we could).

Signed-off-by: Jeff King p...@peff.net
---
Since v1, fixes dubious use of strbuf as pointed out by Christian.

 builtin/blame.c | 14 -
 builtin/fast-export.c   |  2 +-
 builtin/fmt-merge-msg.c |  2 +-
 builtin/index-pack.c|  2 +-
 builtin/log.c   |  2 +-
 builtin/rev-list.c  |  2 +-
 commit.c| 54 -
 commit.h|  8 
 fsck.c  |  2 +-
 log-tree.c  |  2 +-
 merge-recursive.c   |  2 +-
 notes-merge.c   |  2 +-
 object.c|  4 ++--
 pretty.c|  4 ++--
 sequencer.c |  2 +-
 sha1_name.c |  2 +-
 16 files changed, 68 insertions(+), 38 deletions(-)

diff --git a/builtin/blame.c b/builtin/blame.c
index cde19eb..53f43ab 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -2266,6 +2266,18 @@ static void append_merge_parents(struct commit_list 
**tail)
 }
 
 /*
+ * This isn't as simple as passing sb-buf and sb-len, because we
+ * want to transfer ownership of the buffer to the commit (so we
+ * must use detach).
+ */
+static void set_commit_buffer_from_strbuf(struct commit *c, struct strbuf *sb)
+{
+   size_t len;
+   void *buf = strbuf_detach(sb, len);
+   set_commit_buffer(c, buf, len);
+}
+
+/*
  * Prepare a dummy commit that represents the work tree (or staged) item.
  * Note that annotating work tree item never works in the reverse.
  */
@@ -2313,7 +2325,7 @@ static struct commit *fake_working_tree_commit(struct 
diff_options *opt,
ident, ident, path,
(!contents_from ? path :
 (!strcmp(contents_from, -) ? standard input : 
contents_from)));
-   set_commit_buffer(commit, strbuf_detach(msg, NULL));
+   set_commit_buffer_from_strbuf(commit, msg);
 
if (!contents_from || strcmp(-, contents_from)) {
struct stat st;
diff --git a/builtin/fast-export.c b/builtin/fast-export.c
index 7ee5e08..05d161f 100644
--- a/builtin/fast-export.c
+++ b/builtin/fast-export.c
@@ -289,7 +289,7 @@ static void handle_commit(struct commit *commit, struct 
rev_info *rev)
rev-diffopt.output_format = DIFF_FORMAT_CALLBACK;
 
parse_commit_or_die(commit);
-   commit_buffer = get_commit_buffer(commit);
+   commit_buffer = get_commit_buffer(commit, NULL);
author = strstr(commit_buffer, \nauthor );
if (!author)
die (Could not find author in commit %s,
diff --git a/builtin/fmt-merge-msg.c b/builtin/fmt-merge-msg.c
index 01f6d59..ef8b254 100644
--- a/builtin/fmt-merge-msg.c
+++ b/builtin/fmt-merge-msg.c
@@ -236,7 +236,7 @@ static void record_person(int which, struct string_list 
*people,
const char *field;
 
field = (which == 'a') ? \nauthor  : \ncommitter ;
-   buffer = get_commit_buffer(commit);
+   buffer = get_commit_buffer(commit, NULL);
name = strstr(buffer, field);
if (!name)
return;
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 995df39..25e3e9b 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -786,7 +786,7 @@ static void sha1_object(const void *data, struct 
object_entry *obj_entry,
}
if (obj-type == OBJ_COMMIT) {
struct commit *commit = (struct commit *) obj;
-   detach_commit_buffer(commit);
+   detach_commit_buffer(commit, NULL);
}
obj-flags |= FLAG_CHECKED;
}
diff --git a/builtin/log.c b/builtin/log.c
index 2c74260..c599eac 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -919,7 +919,7 @@ static void make_cover_letter(struct rev_info *rev, int 
use_stdout,
need_8bit_cte);
 
for (i = 0; !need_8bit_cte  i  nr; i++) {
-   const char *buf = get_commit_buffer(list[i]);
+   const char *buf = get_commit_buffer(list[i], NULL);
if (has_non_ascii(buf))
need_8bit_cte 

Re: [PATCH v14 06/40] refs.c: add an err argument to repack_without_refs

[Ronnie Sahlberg]

Thanks.

On Tue, Jun 10, 2014 at 1:10 PM, Jonathan Nieder jrnie...@gmail.com wrote:
 Ronnie Sahlberg wrote:

 Update repack_without_refs to take an err argument and update it if there
 is a failure. Pass the err variable from ref_transaction_commit to this
 function so that callers can print a meaningful error message if _commit
 fails due to a problem in repack_without_refs.

 Add a new function unable_to_lock_message that takes a strbuf argument and
 fills in the reason for the failure.

 In commit_packed_refs, make sure that we propagate any errno that
 commit_lock_file might have set back to our caller.

 Make sure both commit_packed_refs and lock_file has errno set to a meaningful
 value on error. Update a whole bunch of other places to keep errno from
 beeing clobbered.

 This patch is doing several (all nice) things.  Are they connected?
 Would splitting some of them out into separate patches make sense or
 would it be too much fuss to be worth the trouble?

 [...]
 --- a/cache.h
 +++ b/cache.h
 @@ -559,6 +559,8 @@ struct lock_file {
  #define LOCK_DIE_ON_ERROR 1
  #define LOCK_NODEREF 2
  extern int unable_to_lock_error(const char *path, int err);
 +extern void unable_to_lock_message(const char *path, int err,
 +struct strbuf *buf);

 Introducing a new unable_to_lock_message helper, which has nicer
 semantics than unable_to_lock_error and cleans up lockfile.c a little.


Done.

 [...]
 --- a/lockfile.c
 +++ b/lockfile.c
 @@ -121,7 +121,7 @@ static char *resolve_symlink(char *p, size_t s)
   return p;
  }

 -
 +/* Make sure errno contains a meaningful value on error */
  static int lock_file(struct lock_file *lk, const char *path, int flags)

 Making errno when returning from lock_file() meaningful, which should
 fix

  * an existing almost-bug in lock_ref_sha1_basic where it assumes
errno==ENOENT is meaningful and could waste some work on retries

  * an existing bug in repack_without_refs where it prints
strerror(errno) and picks advice based on errno, despite errno
potentially being zero and potentially having been clobbered by
that point

 To make sure we don't lose these fixes, it would be helpful to also
 mention that errno is set in the API documentation for the relevant
 functions:

  * hold_lock_file_for_update (declared in cache.h)
  * lock_packed_refs (declared in refs.h)

Done.


 [...]
 --- a/refs.c
 +++ b/refs.c
 @@ -1333,8 +1333,10 @@ const char *resolve_ref_unsafe(const char *refname, 
 unsigned char *sha1, int rea

 Making errno when returning from resolve_ref_unsafe() meaningful,
 which should fix

  * a bug in lock_ref_sha1_basic, where it assumes EISDIR
means it failed due to a directory being in the way

 To make sure we don't lose this fix, it would be helpful to change
 Michael's understated comment (why wasn't it marked with NEEDSWORK,
 btw?)

  * errno is sometimes set on errors, but not always.

 to describe the new guarantee.

Done.


 [...]
 @@ -1908,14 +1918,17 @@ static struct ref_lock *verify_lock(struct ref_lock 
 *lock,

 Making errno when returning from verify_lock() meaningful, which
 should almost but not completely fix

  * a bug in git fetch's s_update_ref, which trusts the result of an
errno == ENOTDIR check to detect D/F conflicts

 To make sure we don't lose this fix, it would be helpful to also
 mention that errno is set in the API documentation for the relevant
 functions:

  * lock_any_ref_for_update (declared in refs.h), after handling the
check_refname_format case
  * lock_ref_sha1_basic

 ENOTDIR makes sense as a sign that a file was in the way of a
 directory we wanted to create.  Should git fetch also look for
 ENOTEMPTY or EEXIST to catch cases where a directory was in the way
 of a file to be created?


Done.

 @@ -1928,13 +1941,15 @@ static int remove_empty_directories(const char *file)

 Making errno when returning from remove_empty_directories() more
 obviously meaningful, which should provide some peace of mind for
 people auditing lock_ref_sha1_basic.


Done.

 [...]
 @@ -2203,11 +2218,16 @@ int lock_packed_refs(int flags)
 [...]
  int commit_packed_refs(void)

 Making errno when returning from commit_packed_refs() meaningful,
 which should fix

  * a bug in git clone where it prints strerror(errno) based on
errno, despite errno possibly being zero and potentially having
been clobbered by that point
  * the same kind of bug in git pack-refs

 and prepares for repack_without_refs() to get a meaningful
 error message when commit_packed_refs() fails without falling into
 the same bug.

 To make sure we don't lose this fix, it would be helpful to also
 mention that errno is set in the API documentation for
 commit_packed_refs in refs.h.


Done.

 [...]
 @@ -2427,12 +2450,12 @@ static int curate_packed_ref_fn(struct ref_entry 
 *entry, void *cb_data)
   return 0;
  }

 -static int repack_without_refs(const char **refnames, int n)
 +static int 

Re: [PATCH v2 0/17] store length of commit-buffer

[Jeff King]

On Tue, Jun 10, 2014 at 05:35:09PM -0400, Jeff King wrote:

 Here's a re-roll of the commit-slab series. It fixes the issues pointed
 out by Eric and Christian (thanks, both).

Side note: I marked this as v2, but forgot to do so in each individual
patch (I write my cover letters first, and then issue format-patch as a
separate step, and I sometimes forget -v2 there). How big an
inconvenience is this?

When I was acting maintainer, it didn't bother me, as I picked the
patches up via threading. But if it is annoying, I can try to teach my
scripts to do it automatically, so I stop forgetting.

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

Re: [PATCH 15/20] t/t5000-tar-tree.sh: avoid test cond -a/-o cond

[René Scharfe]

Am 10.06.2014 22:08, schrieb David Aguilar:

[Resent using René's correct email address this time, sorry for the noise]

On Fri, Jun 06, 2014 at 07:55:58AM -0700, Elia Pinto wrote:

The construct is error-prone; test being built-in in most modern
shells, the reason to avoid test cond  test cond spawning
one extra process by using a single test cond -a cond no
longer exists.

Signed-off-by: Elia Pinto gitter.spi...@gmail.com
---
  t/t5000-tar-tree.sh |2 +-
  1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/t/t5000-tar-tree.sh b/t/t5000-tar-tree.sh
index 74fc5a8..ad6fa0d 100755
--- a/t/t5000-tar-tree.sh
+++ b/t/t5000-tar-tree.sh
@@ -72,7 +72,7 @@ check_tar() {
for header in *.paxheader
do
data=${header%.paxheader}.data 
-   if test -h $data -o -e $data
+   if test -h $data || test -e $data
then


This looks okay, but it raises a question for the original author
(René, I think that's you so I've added you to the To: line).

Should that be test -f instead of test -e?


With -f instead of -e the function would ignore pax path headers for 
directories and special files.  The latter is not relevant for git at 
all and we don't currently have a test for long directory names, but why 
restrict the code to handle only regular files?


A better change would be adding tests for symlinks and directories with 
long names.




This is a very minor note and should not block this patch.
It's probably a change that's better made in a follow-up patch.


path=$(get_pax_header $header path) 
if test -n $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