Single push for internal and public repo

[Jagan Teki]

Usually will do
$ git push  local_branch:destination_branch

repo - could be internal and public remote repo.

Any command or modified git push will do a push for internal and public repo's

Let me know If I'm unclear.

thanks!
-- 
Jagan.
--
To unsubscribe from this list: send the line "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 log --graph` with multiple roots is confusing

[John Keeping]

On Sun, Jun 29, 2014 at 11:40:40PM -0700, Gary Fixler wrote:
> I sometimes pull things in from unrelated repositories to rebase or
> cherry-pick items from a different line of development. I've done this
> to bring isolated features into a project in their own feature
> branches with their full development histories, and also to extract
> lines of development out to their own project, with their histories
> intact. These are usually not connected commits, but things I have to
> track down across time with `git log -S` and friends.
> 
> When I `git remote add otherrepo `, then view things with my
> aliased `git log --oneline --all --graph --decorate` alias, I'm
> usually immediately straining to figure out what's what, as the two
> trees stack onto each other with no separation. It would be nice if
> root commits used something other than *, and/or if they could be
> colored differently by default, or via some option to make them stand
> out as parent-less commits.
> 
> Is this feasible, or already possible?

Have you tried `git log --boundary`?
--
To unsubscribe from this list: send the line "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 format-patch doesn't add Content-type for UTF-8 diffs

[Paul Eggert]

I've been having trouble sending my Git-generated patches to the tz 
mailing list.  Patches containing UTF-8 text are garbled, e.g., if you 
visit  you'll 
see "Ãœrümqi" where the patch actually had "Ürümqi".


I've tracked this down to the fact that "git format-patch" isn't 
outputting a Content-Type: line in the outgoing email.  I thought it was 
supposed to do that; the man page implies that it does.


Here's how I can reproduce the bug with the git 1.9.3 that's shipped 
with Fedora 20.  Notice that the patch is missing the line 
"Content-Type: text/plain; charset=UTF-8" that the git-format-patch man 
page implies it should be generating, and this causes the ICANN email 
software to misinterpret the patch's character set encoding.


$ git init
Initialized empty Git repository in /home/eggert/junk/d/.git/
$ echo x >x
$ git add x
$ git commit -m'x'
[master (root-commit) 5d0e0ce] x
 1 file changed, 1 insertion(+)
 create mode 100644 x
$ echo '§' >x
$ git commit -am'added UTF-8'
[master 57f0669] added UTF-8
 1 file changed, 1 insertion(+), 1 deletion(-)
$ git format-patch -1
0001-added-UTF-8.patch
$ cat 0001-added-UTF-8.patch
From 57f066927a1d8e253715b7980460d81cb549b162 Mon Sep 17 00:00:00 2001
From: Paul Eggert 
Date: Mon, 30 Jun 2014 01:49:28 -0700
Subject: [PATCH] added UTF-8

---
 x | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/x b/x
index 587be6b..3038d22 100644
--- a/x
+++ b/x
@@ -1 +1 @@
-x
+§
--
1.9.3
--
To unsubscribe from this list: send the line "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 log --graph` with multiple roots is confusing

[Gary Fixler]

I just made a new test repo, added and fetched two unrelated repos,
and then did the log view (all/graph/decorate/oneline), and tacked on
--boundary, but saw no change. The root commits looked the same.

-g


On Mon, Jun 30, 2014 at 1:08 AM, John Keeping  wrote:
> On Sun, Jun 29, 2014 at 11:40:40PM -0700, Gary Fixler wrote:
>> I sometimes pull things in from unrelated repositories to rebase or
>> cherry-pick items from a different line of development. I've done this
>> to bring isolated features into a project in their own feature
>> branches with their full development histories, and also to extract
>> lines of development out to their own project, with their histories
>> intact. These are usually not connected commits, but things I have to
>> track down across time with `git log -S` and friends.
>>
>> When I `git remote add otherrepo `, then view things with my
>> aliased `git log --oneline --all --graph --decorate` alias, I'm
>> usually immediately straining to figure out what's what, as the two
>> trees stack onto each other with no separation. It would be nice if
>> root commits used something other than *, and/or if they could be
>> colored differently by default, or via some option to make them stand
>> out as parent-less commits.
>>
>> Is this feasible, or already possible?
>
> Have you tried `git log --boundary`?
--
To unsubscribe from this list: send the line "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 1/4] replace: add --graft option

[Christian Couder]

On Mon, Jun 30, 2014 at 8:37 AM, Junio C Hamano  wrote:
> Christian Couder  writes:
>
>> Now, after having read the recent thread about "git verify-commit", I 
>> understand
>> that you also want me to drop the signature of a tag that was merged, because
>> such signatures are added to the commit message.
>
> Huh?  I am not sure if I follow.  Perhaps we are talking about
> different things?

I think we are talking about the same thing but I might not have been clear.

> When you are preparing a replacement for an existing commit that
> merges a signed tag, there are two cases:
>
>  - The replacement commit still merges the same signed tag; or
>
>  - The replacement commit does not merge that signed tag (it may
>become a single-parent commit, or it may stay to be a merge but
>merges a different commit on the side branch).
>
> In the former, it would be sensible to keep the "mergetag" and
> propagate it to the replacement; it is a signature over the tip of
> the side branch being merged by the original (and the replacement)
> merge, and the replacement will not affect the validity of the
> signature at all.

Ok, this is what is done right now by the patch series.

> In the latter, we do want to drop the "mergetag"
> for the parent you are losing in the replacement, because by
> definition it will be irrelevant.

Yeah, it might be a good idea to drop the "mergetag", but note that
anyway such a commit probably has a title like "Merge tag ''" and
we won't automatically change this title and this title will be wrong
(because we are not merging anymore this tag).

So anyway in this case, --graft will do something that is not good. So
it might be better in this case to just error out and say that it
would be better to use --edit instead of --graft.
--
To unsubscribe from this list: send the line "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 v12 11/11] Documentation: add documentation for 'git interpret-trailers'

[Jakub Narębski]

Christian Couder wrote:


+
+
+* Configure a 'sign' trailer with a command to automatically add a
+  'Signed-off-by: ' with the author information only if there is no
+  'Signed-off-by: ' already, and show how it works:
++
+
+$ git config trailer.sign.key "Signed-off-by: "
+$ git config trailer.sign.ifmissing add
+$ git config trailer.sign.ifexists doNothing
+$ git config trailer.sign.command 'echo "$(git config user.name) <$(git config 
user.email)>"'
+$ git interpret-trailers < EOF


How to configure git-interpret-trailers command so that it follow
current rules for DCO:
* Signed-off-by: is always at bottom; we can have
  signoff+signoff+ack+signoff
* Signed-off-by: can repeat itself with the same author;
  this denotes steps in coming up with current version of the patch.
* but we shouldn't repeat the same signoff one after another

So we want to allow this:

  Signed-off-by: A U Thor 
  Signed-off-by: Joe R. Hacker 
  Acked-by: D E Veloper 
  Signed-off-by: C O Mitter 

but prevent this

  Signed-off-by: C O Mitter 
  Signed-off-by: C O Mitter 


IIRC
--
Jakub Narębski

--
To unsubscribe from this list: send the line "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: How to populate index/worktree when recursive merge merges multiple common ancestors?

[Christian Halstrick]

> They don't. The conflicts are preserved into the virtual ancestor. The
> user only sees the final conflicts during merging of A and B with
> virtual X3 as the common ancestor.

Ah, now I understand. When I merge X1 and X2 into the virtual X3
I should not stop if this is not doable without conflict resolution. I
should store in memory the X3 content, including all the conflict
markers. If I finally merge A and B I will specify a common base
content which may contain conflict markers. Right?
Are git config param's like merge.conflictstyle=diff3 are also
effective when creating the virtual X3 content? Couldn't that lead to
complicated conflict marker situations? In the area where you expect
common base content you again see conflict markers in diff3 style.
--
To unsubscribe from this list: send the line "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: Tackling Git Limitations with Singular Large Line-seperated Plaintext files

[Jakub Narębski]

Linus Torvalds wrote:

On Fri, Jun 27, 2014 at 10:48 AM, Junio C Hamano  wrote:


Even though the original question mentioned "delta discovery", I
think what was being asked is not "delta" in the Git sense (which
your answer is about) but is "can we diff two long sequences of text
(that happens to consist of only 4-letter alphabet but that is a
irrelevant detail) without holding both in-core in their entirety?",
which is a more relevant question/desire from the application point
of view.


.. even there, there's another issue. With enough memory, the diff
itself should be fairly reasonable to do, but we do not have any sane
*format* for diffing those kinds of things.

The regular textual diff is line-based, and is not amenable to
comparing two long lines. You'll just get a diff that says "the two
really long lines are different".

The binary diff option should work, but it is a horrible output
format, and not very helpful. It contains all the relevant data ("copy
this chunk from here to here"), but it's then shown in a binary
encoding that isn't really all that useful if you want to say "what
are the differences between these two chromosomes".


There is also --word-diff[=] word-based textual diff,
and I think one can abuse --word-diff-regex= for
character-based diff... or maybe not, as  specifies
word characters, not words or word separators.

--
Jakub Narębski

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

Re: [PATCH v2 1/2] sha1_file: replace PATH_MAX buffer wirh strbuf in, prepare_packed_git_one()

[Jeff King]

On Sun, Jun 29, 2014 at 07:43:17AM +0200, René Scharfe wrote:

> Instead of using strbuf to create a message string in case a path is
> too long for our fixed-size buffer, replace that buffer with a strbuf
> and thus get rid of the limitation.

Yay. Safer, and the end result is much more readable.

-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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 29.06.2014 13:01, schrieb Eric Sunshine:
> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra  wrote:
>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra  wrote:
 Use git_config_get_string instead of git_config to take advantage of
 the config hash-table api which provides a cleaner control flow.

 Signed-off-by: Tanay Abhra 
 ---
  notes-utils.c | 31 +++
  1 file changed, 15 insertions(+), 16 deletions(-)

 diff --git a/notes-utils.c b/notes-utils.c
 index a0b1d7b..fdc9912 100644
 --- a/notes-utils.c
 +++ b/notes-utils.c
 @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const 
 char *v)
 return NULL;
  }

 -static int notes_rewrite_config(const char *k, const char *v, void *cb)
 +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
  {
 -   struct notes_rewrite_cfg *c = cb;
 -   if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
 -   c->enabled = git_config_bool(k, v);
 -   return 0;
 -   } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
 +   struct strbuf key = STRBUF_INIT;
 +   const char *v;
 +   strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
 +
 +   if (!git_config_get_string(key.buf, &v))
 +   c->enabled = git_config_bool(key.buf, v);
 +
 +   if (!c->mode_from_env && 
 !git_config_get_string("notes.rewritemode", &v)) {
 if (!v)
 -   return config_error_nonbool(k);
 +   config_error_nonbool("notes.rewritemode");
>>>
>>> There's a behavior change here. In the original code, the callback
>>> function would return -1, which would cause the program to die() if
>>> the config.c:die_on_error flag was set. The new code merely emits an
>>> error.
>>
>> Is this change serious enough? Can I ignore it?

IMO its better to Fail Fast than continue with some invalid config (which
may lead to more severe errors such as data corruption / data loss).

> 
> I don't know. Even within this single function there is no consistency
> about whether such problems should die() or just emit a message and
> continue. For instance:
> 
> - if "notes.rewritemode" is bool, it die()s.
> 
> - if "notes.rewritemode" doesn't specify a recognized mode, it
> error()s but continues
> 

I think this would also die in git_parse_source():
...
if (get_value(fn, data, var) < 0)
  break;
  }
  if (cf->die_on_error)
die("bad config file line %d in %s", cf->linenr, cf->name);
...

(AFAICT, die_on_error is always true, except if invoked via 'git-config
--blob', which isn't used anywhere...)


This, however, raises another issue: switching to the config cache looses
file/line-precise error reporting for semantic errors. I don't know if
this feature is important enough to do something about it, though. A
message of the form "Key 'xyz' is bad" should usually enable a user to
locate the problematic file and line.

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

Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison

[Jeff King]

On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:

> Avoid overrunning the existing pack name (p->pack_name, a C string) in
> the case that the new path is longer by using strncmp instead of memcmp
> for comparing.  While at it, replace the magic constant 4 with a
> strlen call to document its meaning.

An unwritten assumption with this code is that we have "foo.idx" and we
want to make sure that we are matching "foo.pack" from the existing pack
list. Both before and after your patch, I think we would match
"foobar.pack". It's probably not a big deal, as we don't expect random
junk in the pack directory, but I wonder if it would be better to be
explicit, like:

  /*
   * like ends_with, but return the truncated size of str via
   * the "len" parameter.
   */
  int strip_suffix(const char *str, const char *suffix, size_t *len)
  {
size_t suflen = strlen(suffix);
*len = strlen(str);

if (len < suflen)
return 0;
else if (strcmp(str + len - suflen, suffix))
return 0;
else {
*len -= suflen;
return 1;
}
  }

  int idx_matches_pack(const char *idx, const char *pack)[
  {
size_t idx_len, pack_len;
if (!strip_suffix(idx, ".idx", &idx_len) ||
!strip_suffix(pack, ".pack", &pack_len))
return 0;
if (idx_len != pack_len)
return 0;
return !memcmp(idx, pack, idx_len);
  }

You'd perhaps want to split idx_matches_pack across the loop below (our
idx is actually a strbuf, so you can reuse path->len to avoid a strlen,
and you do not have to verify idx_len each time through the loop).

I think strip_suffix would have other uses, too. It's a more featureful
version of ends_with, as skip_prefix is to starts_with. E.g.,
builtin/remote.c:config_read_branches could use it to avoid some magic
numbers.

> diff --git a/sha1_file.c b/sha1_file.c
> index 394fa45..8adab14 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int 
> local)
>   if (has_extension(de->d_name, ".idx")) {
>   /* Don't reopen a pack we already have. */

If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.

-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 v2 2/2] sha1_file: use strncmp for string comparison

[Duy Nguyen]

On Mon, Jun 30, 2014 at 8:43 PM, Jeff King  wrote:
>> diff --git a/sha1_file.c b/sha1_file.c
>> index 394fa45..8adab14 100644
>> --- a/sha1_file.c
>> +++ b/sha1_file.c
>> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int 
>> local)
>>   if (has_extension(de->d_name, ".idx")) {
>>   /* Don't reopen a pack we already have. */
>
> If we don't follow my suggestion above, we still have this
> has_extension. This is a reimplementation of ends_with, isn't it? We can
> probably drop it and just use ends_with.

This calls for another patch if we just want to kill has_extension()
in favor of ends_with(). There are 12 call sites of it.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison

[Jeff King]

On Mon, Jun 30, 2014 at 08:59:53PM +0700, Duy Nguyen wrote:

> On Mon, Jun 30, 2014 at 8:43 PM, Jeff King  wrote:
> >> diff --git a/sha1_file.c b/sha1_file.c
> >> index 394fa45..8adab14 100644
> >> --- a/sha1_file.c
> >> +++ b/sha1_file.c
> >> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int 
> >> local)
> >>   if (has_extension(de->d_name, ".idx")) {
> >>   /* Don't reopen a pack we already have. */
> >
> > If we don't follow my suggestion above, we still have this
> > has_extension. This is a reimplementation of ends_with, isn't it? We can
> > probably drop it and just use ends_with.
> 
> This calls for another patch if we just want to kill has_extension()
> in favor of ends_with(). There are 12 call sites of it.

Yes. Some of those would want to become ends_with, and some would
actually want to become strip_suffix. I'm working up a series now.

-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: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Eric Sunshine]

On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees  wrote:
> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra  wrote:
>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
 On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra  wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra 
> ---
>  notes-utils.c | 31 +++
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/notes-utils.c b/notes-utils.c
> index a0b1d7b..fdc9912 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const 
> char *v)
> return NULL;
>  }
>
> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>  {
> -   struct notes_rewrite_cfg *c = cb;
> -   if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
> -   c->enabled = git_config_bool(k, v);
> -   return 0;
> -   } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
> +   struct strbuf key = STRBUF_INIT;
> +   const char *v;
> +   strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
> +
> +   if (!git_config_get_string(key.buf, &v))
> +   c->enabled = git_config_bool(key.buf, v);
> +
> +   if (!c->mode_from_env && 
> !git_config_get_string("notes.rewritemode", &v)) {
> if (!v)
> -   return config_error_nonbool(k);
> +   config_error_nonbool("notes.rewritemode");

 There's a behavior change here. In the original code, the callback
 function would return -1, which would cause the program to die() if
 the config.c:die_on_error flag was set. The new code merely emits an
 error.
>>>
>>> Is this change serious enough? Can I ignore it?
>
> IMO its better to Fail Fast than continue with some invalid config (which
> may lead to more severe errors such as data corruption / data loss).
>
>>
>> I don't know. Even within this single function there is no consistency
>> about whether such problems should die() or just emit a message and
>> continue. For instance:
>>
>> - if "notes.rewritemode" is bool, it die()s.
>>
>> - if "notes.rewritemode" doesn't specify a recognized mode, it
>> error()s but continues
>>
>
> I think this would also die in git_parse_source():
> ...
> if (get_value(fn, data, var) < 0)
>   break;
>   }
>   if (cf->die_on_error)
> die("bad config file line %d in %s", cf->linenr, cf->name);
> ...

One would expect so, but notes-utils.c:notes_rewrite_config() is
actually doing this:

if (!c->combine) {
error(_("Bad notes.rewriteMode value: '%s'"), v);
return 1;
}

Rather than returning the -1 result of error(), which would make
git_parse_source() die(), it's explicitly returning 1, which
get_parse_source() ignores.

> (AFAICT, die_on_error is always true, except if invoked via 'git-config
> --blob', which isn't used anywhere...)
>
> This, however, raises another issue: switching to the config cache looses
> file/line-precise error reporting for semantic errors. I don't know if
> this feature is important enough to do something about it, though. A
> message of the form "Key 'xyz' is bad" should usually enable a user to
> locate the problematic file and line.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/30/2014 7:04 PM, Karsten Blees wrote:
> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra  wrote:
>>> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
 On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra  wrote:
> Use git_config_get_string instead of git_config to take advantage of
> the config hash-table api which provides a cleaner control flow.
>
> Signed-off-by: Tanay Abhra 
> ---
>  notes-utils.c | 31 +++
>  1 file changed, 15 insertions(+), 16 deletions(-)
>
> diff --git a/notes-utils.c b/notes-utils.c
> index a0b1d7b..fdc9912 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const 
> char *v)
> return NULL;
>  }
>
> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>  {
> -   struct notes_rewrite_cfg *c = cb;
> -   if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
> -   c->enabled = git_config_bool(k, v);
> -   return 0;
> -   } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) {
> +   struct strbuf key = STRBUF_INIT;
> +   const char *v;
> +   strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
> +
> +   if (!git_config_get_string(key.buf, &v))
> +   c->enabled = git_config_bool(key.buf, v);
> +
> +   if (!c->mode_from_env && 
> !git_config_get_string("notes.rewritemode", &v)) {
> if (!v)
> -   return config_error_nonbool(k);
> +   config_error_nonbool("notes.rewritemode");

 There's a behavior change here. In the original code, the callback
 function would return -1, which would cause the program to die() if
 the config.c:die_on_error flag was set. The new code merely emits an
 error.
>>>
>>> Is this change serious enough? Can I ignore it?
> 
> IMO its better to Fail Fast than continue with some invalid config (which
> may lead to more severe errors such as data corruption / data loss).

Noted but, what I am trying to do with the rewrite is emit an error and
not set the value if the value found is a NULL. The only change is that
program will not crash in this case and warn the user not set a NULL value for
a non boolean key.
This won't lead to severe errors as the value will not be set if found value
is a NULL.

>>
>> I don't know. Even within this single function there is no consistency
>> about whether such problems should die() or just emit a message and
>> continue. For instance:
>>
>> - if "notes.rewritemode" is bool, it die()s.
>>
>> - if "notes.rewritemode" doesn't specify a recognized mode, it
>> error()s but continues
>>
> 
> I think this would also die in git_parse_source():
> ...
> if (get_value(fn, data, var) < 0)
>   break;
>   }
>   if (cf->die_on_error)
> die("bad config file line %d in %s", cf->linenr, cf->name);
> ...
> 
> (AFAICT, die_on_error is always true, except if invoked via 'git-config
> --blob', which isn't used anywhere...)
> 

Noted.

> This, however, raises another issue: switching to the config cache looses
> file/line-precise error reporting for semantic errors. I don't know if
> this feature is important enough to do something about it, though. A
> message of the form "Key 'xyz' is bad" should usually enable a user to
> locate the problematic file and line.
> 

Hmn, but during the config cache construction we parse key-value pairs through
git_config() which still warns users about semantic errors. This happened
yesterday only when I was writing tests for the new API.

Thanks for the review.

Cheers,
Tanay Abhra.
--
To unsubscribe from this list: send the line "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/PATCH] notes-util.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 30.06.2014 16:32, schrieb Eric Sunshine:
> On Mon, Jun 30, 2014 at 9:34 AM, Karsten Blees  
> wrote:
>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra  wrote:
 On 6/25/2014 1:24 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra  wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra 
>> ---
>>  notes-utils.c | 31 +++
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/notes-utils.c b/notes-utils.c
>> index a0b1d7b..fdc9912 100644
>> --- a/notes-utils.c
>> +++ b/notes-utils.c
>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const 
>> char *v)
>> return NULL;
>>  }
>>
>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>  {
>> -   struct notes_rewrite_cfg *c = cb;
>> -   if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>> -   c->enabled = git_config_bool(k, v);
>> -   return 0;
>> -   } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) 
>> {
>> +   struct strbuf key = STRBUF_INIT;
>> +   const char *v;
>> +   strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>> +
>> +   if (!git_config_get_string(key.buf, &v))
>> +   c->enabled = git_config_bool(key.buf, v);
>> +
>> +   if (!c->mode_from_env && 
>> !git_config_get_string("notes.rewritemode", &v)) {
>> if (!v)
>> -   return config_error_nonbool(k);
>> +   config_error_nonbool("notes.rewritemode");
>
> There's a behavior change here. In the original code, the callback
> function would return -1, which would cause the program to die() if
> the config.c:die_on_error flag was set. The new code merely emits an
> error.

 Is this change serious enough? Can I ignore it?
>>
>> IMO its better to Fail Fast than continue with some invalid config (which
>> may lead to more severe errors such as data corruption / data loss).
>>
>>>
>>> I don't know. Even within this single function there is no consistency
>>> about whether such problems should die() or just emit a message and
>>> continue. For instance:
>>>
>>> - if "notes.rewritemode" is bool, it die()s.
>>>
>>> - if "notes.rewritemode" doesn't specify a recognized mode, it
>>> error()s but continues
>>>
>>
>> I think this would also die in git_parse_source():
>> ...
>> if (get_value(fn, data, var) < 0)
>>   break;
>>   }
>>   if (cf->die_on_error)
>> die("bad config file line %d in %s", cf->linenr, cf->name);
>> ...
> 
> One would expect so, but notes-utils.c:notes_rewrite_config() is
> actually doing this:
> 
> if (!c->combine) {
> error(_("Bad notes.rewriteMode value: '%s'"), v);
> return 1;
> }
> 
> Rather than returning the -1 result of error(), which would make
> git_parse_source() die(), it's explicitly returning 1, which
> get_parse_source() ignores.
> 

Ahh...I missed the '< 0', sorry.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] git-merge-file: do not add LF at EOF while applying unrelated change

[Johannes Schindelin]

Hi Max,

On Sun, 29 Jun 2014, Max Kirillov wrote:

> diff --git a/xdiff/xmerge.c b/xdiff/xmerge.c
> index 9e13b25..625198e 100644
> --- a/xdiff/xmerge.c
> +++ b/xdiff/xmerge.c
> @@ -245,11 +245,11 @@ static int xdl_fill_merge_buffer(xdfenv_t *xe1, const 
> char *name1,
> dest ? dest + size : NULL);
>   /* Postimage from side #1 */
>   if (m->mode & 1)
> - size += xdl_recs_copy(xe1, m->i1, m->chg1, 1,
> + size += xdl_recs_copy(xe1, m->i1, m->chg1, 
> (m->mode & 2),
> dest ? dest + size : 
> NULL);
>   /* Postimage from side #2 */
>   if (m->mode & 2)
> - size += xdl_recs_copy(xe2, m->i2, m->chg2, 1,
> + size += xdl_recs_copy(xe2, m->i2, m->chg2, 0,
> dest ? dest + size : 
> NULL);
>   } else
>   continue;

Makes sense to me, especially with the nice explanation in the commit
message. I just wish the tests were a little easier to understand... It is
probably my fault because I insisted on using a text that has *nothing* to
do with Git. These days, I would probably have used better file names and
would have used file contents that reflect the purpose in the tests (i.e.
a line saying "This line ends with a carriage return" or some such).

Having said that, here is my ACK for the current revision of the patch
series (because I know how much work it would be to fix the issue I
described above, and it is an *entirely different* issue from the one you
fixed with this series, too).

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

GOOD DAY,

[KONG HUI]

My name is Kong Hui from Hong Kong, I want you to be my partner in a
business project.

If Interested Contact me back via my email address.


Thank you,
Mr. Kong Hui.

--
To unsubscribe from this list: send the line "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/PATCH] notes-util.c: replace git_config with git_config_get_string

[Karsten Blees]

Am 30.06.2014 16:39, schrieb Tanay Abhra:
> 
> On 6/30/2014 7:04 PM, Karsten Blees wrote:
>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
>>> On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra  wrote:
 On 6/25/2014 1:24 PM, Eric Sunshine wrote:
> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra  wrote:
>> Use git_config_get_string instead of git_config to take advantage of
>> the config hash-table api which provides a cleaner control flow.
>>
>> Signed-off-by: Tanay Abhra 
>> ---
>>  notes-utils.c | 31 +++
>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>
>> diff --git a/notes-utils.c b/notes-utils.c
>> index a0b1d7b..fdc9912 100644
>> --- a/notes-utils.c
>> +++ b/notes-utils.c
>> @@ -68,22 +68,23 @@ static combine_notes_fn parse_combine_notes_fn(const 
>> char *v)
>> return NULL;
>>  }
>>
>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>  {
>> -   struct notes_rewrite_cfg *c = cb;
>> -   if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>> -   c->enabled = git_config_bool(k, v);
>> -   return 0;
>> -   } else if (!c->mode_from_env && !strcmp(k, "notes.rewritemode")) 
>> {
>> +   struct strbuf key = STRBUF_INIT;
>> +   const char *v;
>> +   strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>> +
>> +   if (!git_config_get_string(key.buf, &v))
>> +   c->enabled = git_config_bool(key.buf, v);
>> +
>> +   if (!c->mode_from_env && 
>> !git_config_get_string("notes.rewritemode", &v)) {
>> if (!v)
>> -   return config_error_nonbool(k);
>> +   config_error_nonbool("notes.rewritemode");
>
> There's a behavior change here. In the original code, the callback
> function would return -1, which would cause the program to die() if
> the config.c:die_on_error flag was set. The new code merely emits an
> error.

 Is this change serious enough? Can I ignore it?
>>
>> IMO its better to Fail Fast than continue with some invalid config (which
>> may lead to more severe errors such as data corruption / data loss).
> 
> Noted but, what I am trying to do with the rewrite is emit an error and
> not set the value if the value found is a NULL.

If you don't set the value and continue, git will proceed with the variable's
default setting.

Which may not be too harmful in some cases, but if a user changes:

 gc.pruneexpire=4.weeks.ago

to

 gc.pruneexpire=4.monhts.ago

(note the typo), the next git-gc will warn the user and then happily throw
away data that the user intended to keep (default is 2.weeks.ago).

Thus I think git should die() if it encounters an invalid config setting.

> 
>> This, however, raises another issue: switching to the config cache looses
>> file/line-precise error reporting for semantic errors. I don't know if
>> this feature is important enough to do something about it, though. A
>> message of the form "Key 'xyz' is bad" should usually enable a user to
>> locate the problematic file and line.
>>
> 
> Hmn, but during the config cache construction we parse key-value pairs through
> git_config() which still warns users about semantic errors.

If I'm not mistaken you only detect _syntax_ errors when loading the file (i.e.
whether the config file is structurally correct).

The semantic value and correctness of a key (e.g. whether its a boolean or an
int or a string that denotes a known merge algorithm) is only checked when it is
accessed via git_config_get_. And at this point, : information
is already lost.

With the callback approach, both syntactic (structure) and semantic (meaning)
errors were checked at load time, resulting in

  die("bad config file line %d in %s", cf->linenr, cf->name);

if the callback returned -1.

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

Re: [RFC/PATCH] notes-util.c: replace git_config with git_config_get_string

[Tanay Abhra]

On 6/30/2014 9:26 PM, Karsten Blees wrote:
> Am 30.06.2014 16:39, schrieb Tanay Abhra:
>>
>> On 6/30/2014 7:04 PM, Karsten Blees wrote:
>>> Am 29.06.2014 13:01, schrieb Eric Sunshine:
 On Thu, Jun 26, 2014 at 4:19 AM, Tanay Abhra  wrote:
> On 6/25/2014 1:24 PM, Eric Sunshine wrote:
>> On Mon, Jun 23, 2014 at 6:41 AM, Tanay Abhra  wrote:
>>> Use git_config_get_string instead of git_config to take advantage of
>>> the config hash-table api which provides a cleaner control flow.
>>>
>>> Signed-off-by: Tanay Abhra 
>>> ---
>>>  notes-utils.c | 31 +++
>>>  1 file changed, 15 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/notes-utils.c b/notes-utils.c
>>> index a0b1d7b..fdc9912 100644
>>> --- a/notes-utils.c
>>> +++ b/notes-utils.c
>>> @@ -68,22 +68,23 @@ static combine_notes_fn 
>>> parse_combine_notes_fn(const char *v)
>>> return NULL;
>>>  }
>>>
>>> -static int notes_rewrite_config(const char *k, const char *v, void *cb)
>>> +static void notes_rewrite_config(struct notes_rewrite_cfg *c)
>>>  {
>>> -   struct notes_rewrite_cfg *c = cb;
>>> -   if (starts_with(k, "notes.rewrite.") && !strcmp(k+14, c->cmd)) {
>>> -   c->enabled = git_config_bool(k, v);
>>> -   return 0;
>>> -   } else if (!c->mode_from_env && !strcmp(k, 
>>> "notes.rewritemode")) {
>>> +   struct strbuf key = STRBUF_INIT;
>>> +   const char *v;
>>> +   strbuf_addf(&key, "notes.rewrite.%s", c->cmd);
>>> +
>>> +   if (!git_config_get_string(key.buf, &v))
>>> +   c->enabled = git_config_bool(key.buf, v);
>>> +
>>> +   if (!c->mode_from_env && 
>>> !git_config_get_string("notes.rewritemode", &v)) {
>>> if (!v)
>>> -   return config_error_nonbool(k);
>>> +   config_error_nonbool("notes.rewritemode");
>>
>> There's a behavior change here. In the original code, the callback
>> function would return -1, which would cause the program to die() if
>> the config.c:die_on_error flag was set. The new code merely emits an
>> error.
>
> Is this change serious enough? Can I ignore it?
>>>
>>> IMO its better to Fail Fast than continue with some invalid config (which
>>> may lead to more severe errors such as data corruption / data loss).
>>
>> Noted but, what I am trying to do with the rewrite is emit an error and
>> not set the value if the value found is a NULL.
> 
> If you don't set the value and continue, git will proceed with the variable's
> default setting.
> 
> Which may not be too harmful in some cases, but if a user changes:
> 
>  gc.pruneexpire=4.weeks.ago
> 
> to
> 
>  gc.pruneexpire=4.monhts.ago
> 
> (note the typo), the next git-gc will warn the user and then happily throw
> away data that the user intended to keep (default is 2.weeks.ago).
> 
> Thus I think git should die() if it encounters an invalid config setting.

Okay, point noted.

>>
>>> This, however, raises another issue: switching to the config cache looses
>>> file/line-precise error reporting for semantic errors. I don't know if
>>> this feature is important enough to do something about it, though. A
>>> message of the form "Key 'xyz' is bad" should usually enable a user to
>>> locate the problematic file and line.
>>>
>>
>> Hmn, but during the config cache construction we parse key-value pairs 
>> through
>> git_config() which still warns users about semantic errors.
> 
> If I'm not mistaken you only detect _syntax_ errors when loading the file 
> (i.e.
> whether the config file is structurally correct).
> 
> The semantic value and correctness of a key (e.g. whether its a boolean or an
> int or a string that denotes a known merge algorithm) is only checked when it 
> is
> accessed via git_config_get_. And at this point, : 
> information
> is already lost.
> 
> With the callback approach, both syntactic (structure) and semantic (meaning)
> errors were checked at load time, resulting in
> 
>   die("bad config file line %d in %s", cf->linenr, cf->name);
> 
> if the callback returned -1.
> 

Yup, you are right, we check only syntax error when loading the file for the 
cache.
I could save the : when the loading the file for future error
reporting. Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison

[René Scharfe]

Am 30.06.2014 15:43, schrieb Jeff King:
> On Sun, Jun 29, 2014 at 07:56:25AM +0200, René Scharfe wrote:
> 
>> Avoid overrunning the existing pack name (p->pack_name, a C string) in
>> the case that the new path is longer by using strncmp instead of memcmp
>> for comparing.  While at it, replace the magic constant 4 with a
>> strlen call to document its meaning.
> 
> An unwritten assumption with this code is that we have "foo.idx" and we
> want to make sure that we are matching "foo.pack" from the existing pack
> list. Both before and after your patch, I think we would match
> "foobar.pack".

Yes, indeed.

> It's probably not a big deal, as we don't expect random
> junk in the pack directory, but I wonder if it would be better to be
> explicit, like:



Here's a simpler approach:

-- >8 --
Subject: [PATCH v2 3/2] sha1_file: more precise packname matching

Consider the full length of the already loaded pack names when checking
for duplicates.

Signed-off-by: Rene Scharfe 
---
 sha1_file.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index b7ad6c1..a13fbbd 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1206,10 +1206,12 @@ static void prepare_packed_git_one(char *objdir, int 
local)
strbuf_addstr(&path, de->d_name);
 
if (has_extension(de->d_name, ".idx")) {
+   size_t len = path.len - strlen(".idx");
+
/* Don't reopen a pack we already have. */
for (p = packed_git; p; p = p->next) {
-   if (!strncmp(path.buf, p->pack_name,
-path.len - strlen(".idx")))
+   if (!strncmp(p->pack_name, path.buf, len) &&
+   !strcmp(p->pack_name + len, ".pack"))
break;
}
if (p == NULL &&
-- 
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

Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison

[René Scharfe]

Am 30.06.2014 16:22, schrieb Jeff King:

On Mon, Jun 30, 2014 at 08:59:53PM +0700, Duy Nguyen wrote:


On Mon, Jun 30, 2014 at 8:43 PM, Jeff King  wrote:

diff --git a/sha1_file.c b/sha1_file.c
index 394fa45..8adab14 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, int 
local)
   if (has_extension(de->d_name, ".idx")) {
   /* Don't reopen a pack we already have. */


If we don't follow my suggestion above, we still have this
has_extension. This is a reimplementation of ends_with, isn't it? We can
probably drop it and just use ends_with.


This calls for another patch if we just want to kill has_extension()
in favor of ends_with(). There are 12 call sites of it.


Yes. Some of those would want to become ends_with, and some would
actually want to become strip_suffix. I'm working up a series now.


NB: has_extension is almost the same as ends_with, but it also checks if 
the string is longer than just the extension:


ends_with("x", "x") => 1
has_extension("x", "x") => 0

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

Re: [PATCH v2 2/2] sha1_file: use strncmp for string comparison

[Jeff King]

On Mon, Jun 30, 2014 at 06:43:35PM +0200, René Scharfe wrote:

> Am 30.06.2014 16:22, schrieb Jeff King:
> >On Mon, Jun 30, 2014 at 08:59:53PM +0700, Duy Nguyen wrote:
> >
> >>On Mon, Jun 30, 2014 at 8:43 PM, Jeff King  wrote:
> diff --git a/sha1_file.c b/sha1_file.c
> index 394fa45..8adab14 100644
> --- a/sha1_file.c
> +++ b/sha1_file.c
> @@ -1207,7 +1207,8 @@ static void prepare_packed_git_one(char *objdir, 
> int local)
>    if (has_extension(de->d_name, ".idx")) {
>    /* Don't reopen a pack we already have. */
> >>>
> >>>If we don't follow my suggestion above, we still have this
> >>>has_extension. This is a reimplementation of ends_with, isn't it? We can
> >>>probably drop it and just use ends_with.
> >>
> >>This calls for another patch if we just want to kill has_extension()
> >>in favor of ends_with(). There are 12 call sites of it.
> >
> >Yes. Some of those would want to become ends_with, and some would
> >actually want to become strip_suffix. I'm working up a series now.
> 
> NB: has_extension is almost the same as ends_with, but it also checks if the
> string is longer than just the extension:
> 
>   ends_with("x", "x") => 1
>   has_extension("x", "x") => 0

Thanks, I didn't notice that. I don't think the distinction is
important in any callers, but I'll double check.

-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 v2 2/2] sha1_file: use strncmp for string comparison

[Jeff King]

On Mon, Jun 30, 2014 at 06:35:50PM +0200, René Scharfe wrote:

> > It's probably not a big deal, as we don't expect random
> > junk in the pack directory, but I wonder if it would be better to be
> > explicit, like:
> 
> 
> 
> Here's a simpler approach:

I agree that solves the problem. However, I'm about to post an
alternative series that also replaces has_extension with strip_suffix,
which I think ends up a bit nicer.

-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/9] sha1_file: replace PATH_MAX buffer with strbuf in prepare_packed_git_one()

[Jeff King]

From: René Scharfe 

Instead of using strbuf to create a message string in case a path is
too long for our fixed-size buffer, replace that buffer with a strbuf
and thus get rid of the limitation.

Helped-by: Duy Nguyen 
Signed-off-by: Rene Scharfe 
Signed-off-by: Jeff King 
---
 sha1_file.c | 42 --
 1 file changed, 16 insertions(+), 26 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 34d527f..394fa45 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1177,48 +1177,37 @@ static void report_pack_garbage(struct string_list 
*list)
 
 static void prepare_packed_git_one(char *objdir, int local)
 {
-   /* Ensure that this buffer is large enough so that we can
-  append "/pack/" without clobbering the stack even if
-  strlen(objdir) were PATH_MAX.  */
-   char path[PATH_MAX + 1 + 4 + 1 + 1];
-   int len;
+   struct strbuf path = STRBUF_INIT;
+   size_t dirnamelen;
DIR *dir;
struct dirent *de;
struct string_list garbage = STRING_LIST_INIT_DUP;
 
-   sprintf(path, "%s/pack", objdir);
-   len = strlen(path);
-   dir = opendir(path);
+   strbuf_addstr(&path, objdir);
+   strbuf_addstr(&path, "/pack");
+   dir = opendir(path.buf);
if (!dir) {
if (errno != ENOENT)
error("unable to open object pack directory: %s: %s",
- path, strerror(errno));
+ path.buf, strerror(errno));
+   strbuf_release(&path);
return;
}
-   path[len++] = '/';
+   strbuf_addch(&path, '/');
+   dirnamelen = path.len;
while ((de = readdir(dir)) != NULL) {
-   int namelen = strlen(de->d_name);
struct packed_git *p;
 
-   if (len + namelen + 1 > sizeof(path)) {
-   if (report_garbage) {
-   struct strbuf sb = STRBUF_INIT;
-   strbuf_addf(&sb, "%.*s/%s", len - 1, path, 
de->d_name);
-   report_garbage("path too long", sb.buf);
-   strbuf_release(&sb);
-   }
-   continue;
-   }
-
if (is_dot_or_dotdot(de->d_name))
continue;
 
-   strcpy(path + len, de->d_name);
+   strbuf_setlen(&path, dirnamelen);
+   strbuf_addstr(&path, de->d_name);
 
if (has_extension(de->d_name, ".idx")) {
/* Don't reopen a pack we already have. */
for (p = packed_git; p; p = p->next) {
-   if (!memcmp(path, p->pack_name, len + namelen - 
4))
+   if (!memcmp(path.buf, p->pack_name, path.len - 
4))
break;
}
if (p == NULL &&
@@ -1226,7 +1215,7 @@ static void prepare_packed_git_one(char *objdir, int 
local)
 * See if it really is a valid .idx file with
 * corresponding .pack file that we can map.
 */
-   (p = add_packed_git(path, len + namelen, local)) != 
NULL)
+   (p = add_packed_git(path.buf, path.len, local)) != 
NULL)
install_packed_git(p);
}
 
@@ -1237,13 +1226,14 @@ static void prepare_packed_git_one(char *objdir, int 
local)
has_extension(de->d_name, ".pack") ||
has_extension(de->d_name, ".bitmap") ||
has_extension(de->d_name, ".keep"))
-   string_list_append(&garbage, path);
+   string_list_append(&garbage, path.buf);
else
-   report_garbage("garbage found", path);
+   report_garbage("garbage found", path.buf);
}
closedir(dir);
report_pack_garbage(&garbage);
string_list_clear(&garbage, 0);
+   strbuf_release(&path);
 }
 
 static int sort_pack(const void *a_, const void *b_)
-- 
2.0.0.566.gfe3e6b2

--
To unsubscribe from this list: send the line "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/9] add strip_suffix as an alternative to ends_with

[Jeff King]

Here's a series to do for ends_with what the recent skip_prefix series
did for starts_with. Namely: drop some magic numbers and repeated string
literals, and hopefully make things more readable.

The first patch is René's patch 1/2, with the leak fix from Duy and typo
fixes in the commit message from me. The rest are new, and replace the
changes to prepare_packed_git_one done elsewhere in the thread.

  [1/9]: sha1_file: replace PATH_MAX buffer with strbuf in 
prepare_packed_git_one()
  [2/9]: add strip_suffix function
  [3/9]: implement ends_with via strip_suffix
  [4/9]: replace has_extension with ends_with
  [5/9]: use strip_suffix instead of ends_with in simple cases
  [6/9]: index-pack: use strip_suffix to avoid magic numbers
  [7/9]: strbuf: implement strbuf_strip_suffix
  [8/9]: verify-pack: use strbuf_strip_suffix
  [9/9]: prepare_packed_git_one: refactor duplicate-pack check

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

[PATCH 3/9] implement ends_with via strip_suffix

[Jeff King]

The ends_with function is essentially a simplified version
of strip_suffix, in which we throw away the stripped length.
Implementing it as an inline on top of strip_suffix has two
advantages:

  1. We save a bit of duplicated code.

  2. The suffix is typically a string literal, and we call
 strlen on it. By making the function inline, many
 compilers can replace the strlen call with a constant.

Signed-off-by: Jeff King 
---
 git-compat-util.h | 7 ++-
 strbuf.c  | 9 -
 2 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index d044c42..4cfde49 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -347,7 +347,6 @@ extern void set_error_routine(void (*routine)(const char 
*err, va_list params));
 extern void set_die_is_recursing_routine(int (*routine)(void));
 
 extern int starts_with(const char *str, const char *prefix);
-extern int ends_with(const char *str, const char *suffix);
 
 static inline const char *skip_prefix(const char *str, const char *prefix)
 {
@@ -385,6 +384,12 @@ static inline int strip_suffix(const char *str, const char 
*suffix, size_t *len)
return strip_suffix_mem(str, len, suffix);
 }
 
+static inline int ends_with(const char *str, const char *suffix)
+{
+   size_t len;
+   return strip_suffix(str, suffix, &len);
+}
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
 
 #ifndef PROT_READ
diff --git a/strbuf.c b/strbuf.c
index ac62982..99dbeba 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -11,15 +11,6 @@ int starts_with(const char *str, const char *prefix)
return 0;
 }
 
-int ends_with(const char *str, const char *suffix)
-{
-   int len = strlen(str), suflen = strlen(suffix);
-   if (len < suflen)
-   return 0;
-   else
-   return !strcmp(str + len - suflen, suffix);
-}
-
 /*
  * Used as the default ->buf value, so that people can always assume
  * buf is non NULL and ->buf is NUL terminated even for a freshly
-- 
2.0.0.566.gfe3e6b2

--
To unsubscribe from this list: send the line "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/9] replace has_extension with ends_with

[Jeff King]

These two are almost the same function, with the exception
that has_extension only matches if there is content before
the suffix. So ends_with(".exe", ".exe") is true, but
has_extension would not be.

This distinction does not matter to any of the callers,
though, and we can just replace uses of has_extension with
ends_with. We prefer the "ends_with" name because it is more
generic, and there is nothing about the function that
requires it to be used for file extensions.

Signed-off-by: Jeff King 
---
 builtin/index-pack.c  |  4 ++--
 builtin/verify-pack.c |  4 ++--
 git-compat-util.h |  7 ---
 help.c|  2 +-
 refs.c|  4 ++--
 sha1_file.c   | 10 +-
 6 files changed, 12 insertions(+), 19 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 18f57de..46376b6 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1603,7 +1603,7 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
die(_("--fix-thin cannot be used without --stdin"));
if (!index_name && pack_name) {
int len = strlen(pack_name);
-   if (!has_extension(pack_name, ".pack"))
+   if (!ends_with(pack_name, ".pack"))
die(_("packfile name '%s' does not end with '.pack'"),
pack_name);
index_name_buf = xmalloc(len);
@@ -1613,7 +1613,7 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
}
if (keep_msg && !keep_name && pack_name) {
int len = strlen(pack_name);
-   if (!has_extension(pack_name, ".pack"))
+   if (!ends_with(pack_name, ".pack"))
die(_("packfile name '%s' does not end with '.pack'"),
pack_name);
keep_name_buf = xmalloc(len);
diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c
index 66cd2df..2fd29ce 100644
--- a/builtin/verify-pack.c
+++ b/builtin/verify-pack.c
@@ -27,9 +27,9 @@ static int verify_one_pack(const char *path, unsigned int 
flags)
 * normalize these forms to "foo.pack" for "index-pack --verify".
 */
strbuf_addstr(&arg, path);
-   if (has_extension(arg.buf, ".idx"))
+   if (ends_with(arg.buf, ".idx"))
strbuf_splice(&arg, arg.len - 3, 3, "pack", 4);
-   else if (!has_extension(arg.buf, ".pack"))
+   else if (!ends_with(arg.buf, ".pack"))
strbuf_add(&arg, ".pack", 5);
argv[2] = arg.buf;
 
diff --git a/git-compat-util.h b/git-compat-util.h
index 4cfde49..8cf79ae 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -594,13 +594,6 @@ static inline size_t xsize_t(off_t len)
return (size_t)len;
 }
 
-static inline int has_extension(const char *filename, const char *ext)
-{
-   size_t len = strlen(filename);
-   size_t extlen = strlen(ext);
-   return len > extlen && !memcmp(filename + len - extlen, ext, extlen);
-}
-
 /* in ctype.c, for kwset users */
 extern const char tolower_trans_tbl[256];
 
diff --git a/help.c b/help.c
index b266b09..372728f 100644
--- a/help.c
+++ b/help.c
@@ -156,7 +156,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,
continue;
 
entlen = strlen(de->d_name) - prefix_len;
-   if (has_extension(de->d_name, ".exe"))
+   if (ends_with(de->d_name, ".exe"))
entlen -= 4;
 
add_cmdname(cmds, de->d_name + prefix_len, entlen);
diff --git a/refs.c b/refs.c
index dc45774..e31adfd 100644
--- a/refs.c
+++ b/refs.c
@@ -1162,7 +1162,7 @@ static void read_loose_refs(const char *dirname, struct 
ref_dir *dir)
 
if (de->d_name[0] == '.')
continue;
-   if (has_extension(de->d_name, ".lock"))
+   if (ends_with(de->d_name, ".lock"))
continue;
strbuf_addstr(&refname, de->d_name);
refdir = *refs->name
@@ -3233,7 +3233,7 @@ static int do_for_each_reflog(struct strbuf *name, 
each_ref_fn fn, void *cb_data
 
if (de->d_name[0] == '.')
continue;
-   if (has_extension(de->d_name, ".lock"))
+   if (ends_with(de->d_name, ".lock"))
continue;
strbuf_addstr(name, de->d_name);
if (stat(git_path("logs/%s", name->buf), &st) < 0) {
diff --git a/sha1_file.c b/sha1_file.c
index 394fa45..93b794f 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1204,7 +1204,7 @@ static void prepare_packed_git_one(char *objdir, int 
local)
strbuf_setlen(&path, dirnamelen);
strbuf_addstr(&path, de->d_name);
 
-   if (has_extension(de->d_name, ".idx")) {
+   if (ends_with(de->d_name, ".idx")) {
/* Don't reopen a pack we already have. */
for (p =

[PATCH 2/9] add strip_suffix function

[Jeff King]

Many callers of ends_with want to not only find out whether
a string has a suffix, but want to also strip it off. Doing
that separately has two minor problems:

  1. We often run over the string twice (once to find
 the suffix, and then once more to find its length to
 subtract the suffix length).

  2. We have to specify the suffix length again, which means
 either a magic number, or repeating ourselves with
 strlen("suffix").

Just as we have skip_prefix to avoid these cases with
starts_with, we can add a strip_suffix to avoid them with
ends_with.

Note that we add two forms of strip_suffix here: one that
takes a string, with the resulting length as an
out-parameter; and one that takes a pointer/length pair, and
reuses the length as an out-parameter. The latter is more
efficient when the caller already has the length (e.g., when
using strbufs), but it can be easy to confuse the two, as
they take the same number and types of parameters.

For that reason, the "mem" form puts its length parameter
next to the buffer (since they are a pair), and the string
form puts it at the end (since it is an out-parameter). The
compiler can notice when you get the order wrong, which
should help prevent writing one when you meant the other.

Signed-off-by: Jeff King 
---
I hope the word "strip" is OK, as it does not actually NUL-terminate
(doing so would make it unusable for many cases). Between the comment
below and the "const" in the parameter, I think it should be pretty
clear that it does not touch the string. And I could not think of a
better word.

 git-compat-util.h | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/git-compat-util.h b/git-compat-util.h
index b6f03b3..d044c42 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -358,6 +358,33 @@ static inline const char *skip_prefix(const char *str, 
const char *prefix)
return NULL;
 }
 
+/*
+ * If buf ends with suffix, return 1 and subtract the length of the suffix
+ * from *len. Otherwise, return 0 and leave *len untouched.
+ */
+static inline int strip_suffix_mem(const char *buf, size_t *len,
+  const char *suffix)
+{
+   size_t suflen = strlen(suffix);
+   if (*len < suflen || memcmp(buf + (*len - suflen), suffix, suflen))
+   return 0;
+   *len -= suflen;
+   return 1;
+}
+
+/*
+ * If str ends with suffix, return 1 and set *len to the size of the string
+ * without the suffix. Otherwise, return 0 and set *len to the size of the
+ * string.
+ *
+ * Note that we do _not_ NUL-terminate str to the new length.
+ */
+static inline int strip_suffix(const char *str, const char *suffix, size_t 
*len)
+{
+   *len = strlen(str);
+   return strip_suffix_mem(str, len, suffix);
+}
+
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
 
 #ifndef PROT_READ
-- 
2.0.0.566.gfe3e6b2

--
To unsubscribe from this list: send the line "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/9] use strip_suffix instead of ends_with in simple cases

[Jeff King]

When stripping a suffix like:

  if (ends_with(str, "foo"))
buf = xmemdupz(str, strlen(str) - 3);

we can instead use strip_suffix to avoid the constant 3,
which must match the literal "foo" (we sometimes use
strlen("foo") instead, but that means we are repeating
ourselves). The example above becomes:

  if (strip_suffix(str, "foo", &len))
buf = xmemdupz(str, len);

This also saves a strlen(), since we calculate the string
length when detecting the suffix.

Note that in some cases we also switch from xstrndup to
xmemdupz, which saves a further strlen call.

Signed-off-by: Jeff King 
---
 builtin/remote.c | 13 +++--
 builtin/repack.c |  5 ++---
 connected.c  |  6 +++---
 help.c   |  5 ++---
 4 files changed, 14 insertions(+), 15 deletions(-)

diff --git a/builtin/remote.c b/builtin/remote.c
index c9102e8..7c2999e 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -265,16 +265,17 @@ static int config_read_branches(const char *key, const 
char *value, void *cb)
struct string_list_item *item;
struct branch_info *info;
enum { REMOTE, MERGE, REBASE } type;
+   size_t key_len;
 
key += 7;
-   if (ends_with(key, ".remote")) {
-   name = xstrndup(key, strlen(key) - 7);
+   if (strip_suffix(key, ".remote", &key_len)) {
+   name = xmemdupz(key, key_len);
type = REMOTE;
-   } else if (ends_with(key, ".merge")) {
-   name = xstrndup(key, strlen(key) - 6);
+   } else if (strip_suffix(key, ".merge", &key_len)) {
+   name = xmemdupz(key, key_len);
type = MERGE;
-   } else if (ends_with(key, ".rebase")) {
-   name = xstrndup(key, strlen(key) - 7);
+   } else if (strip_suffix(key, ".rebase", &key_len)) {
+   name = xmemdupz(key, key_len);
type = REBASE;
} else
return 0;
diff --git a/builtin/repack.c b/builtin/repack.c
index ff2216a..a77e743 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -83,16 +83,15 @@ static void get_non_kept_pack_filenames(struct string_list 
*fname_list)
DIR *dir;
struct dirent *e;
char *fname;
-   size_t len;
 
if (!(dir = opendir(packdir)))
return;
 
while ((e = readdir(dir)) != NULL) {
-   if (!ends_with(e->d_name, ".pack"))
+   size_t len;
+   if (!strip_suffix(e->d_name, ".pack", &len))
continue;
 
-   len = strlen(e->d_name) - strlen(".pack");
fname = xmemdupz(e->d_name, len);
 
if (!file_exists(mkpath("%s/%s.keep", packdir, fname)))
diff --git a/connected.c b/connected.c
index be0253e..dae9c99 100644
--- a/connected.c
+++ b/connected.c
@@ -31,6 +31,7 @@ static int check_everything_connected_real(sha1_iterate_fn fn,
unsigned char sha1[20];
int err = 0, ac = 0;
struct packed_git *new_pack = NULL;
+   size_t base_len;
 
if (fn(cb_data, sha1))
return err;
@@ -38,10 +39,9 @@ static int check_everything_connected_real(sha1_iterate_fn 
fn,
if (transport && transport->smart_options &&
transport->smart_options->self_contained_and_connected &&
transport->pack_lockfile &&
-   ends_with(transport->pack_lockfile, ".keep")) {
+   strip_suffix(transport->pack_lockfile, ".keep", &base_len)) {
struct strbuf idx_file = STRBUF_INIT;
-   strbuf_addstr(&idx_file, transport->pack_lockfile);
-   strbuf_setlen(&idx_file, idx_file.len - 5); /* ".keep" */
+   strbuf_add(&idx_file, transport->pack_lockfile, base_len);
strbuf_addstr(&idx_file, ".idx");
new_pack = add_packed_git(idx_file.buf, idx_file.len, 1);
strbuf_release(&idx_file);
diff --git a/help.c b/help.c
index 372728f..97567c4 100644
--- a/help.c
+++ b/help.c
@@ -145,7 +145,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,
len = buf.len;
 
while ((de = readdir(dir)) != NULL) {
-   int entlen;
+   size_t entlen;
 
if (!starts_with(de->d_name, prefix))
continue;
@@ -156,8 +156,7 @@ static void list_commands_in_dir(struct cmdnames *cmds,
continue;
 
entlen = strlen(de->d_name) - prefix_len;
-   if (ends_with(de->d_name, ".exe"))
-   entlen -= 4;
+   strip_suffix(de->d_name, ".exe", &entlen);
 
add_cmdname(cmds, de->d_name + prefix_len, entlen);
}
-- 
2.0.0.566.gfe3e6b2

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

[PATCH 6/9] index-pack: use strip_suffix to avoid magic numbers

[Jeff King]

We also switch to using strbufs, which lets us avoid the
potentially dangerous combination of a manual malloc
followed by a strcpy.

Signed-off-by: Jeff King 
---
 builtin/index-pack.c | 29 ++---
 1 file changed, 14 insertions(+), 15 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 46376b6..d4b77fd 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1505,7 +1505,8 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
const char *curr_index;
const char *index_name = NULL, *pack_name = NULL;
const char *keep_name = NULL, *keep_msg = NULL;
-   char *index_name_buf = NULL, *keep_name_buf = NULL;
+   struct strbuf index_name_buf = STRBUF_INIT,
+ keep_name_buf = STRBUF_INIT;
struct pack_idx_entry **idx_objects;
struct pack_idx_option opts;
unsigned char pack_sha1[20];
@@ -1602,24 +1603,22 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
if (fix_thin_pack && !from_stdin)
die(_("--fix-thin cannot be used without --stdin"));
if (!index_name && pack_name) {
-   int len = strlen(pack_name);
-   if (!ends_with(pack_name, ".pack"))
+   size_t len;
+   if (!strip_suffix(pack_name, ".pack", &len))
die(_("packfile name '%s' does not end with '.pack'"),
pack_name);
-   index_name_buf = xmalloc(len);
-   memcpy(index_name_buf, pack_name, len - 5);
-   strcpy(index_name_buf + len - 5, ".idx");
-   index_name = index_name_buf;
+   strbuf_add(&index_name_buf, pack_name, len);
+   strbuf_addstr(&index_name_buf, ".idx");
+   index_name = index_name_buf.buf;
}
if (keep_msg && !keep_name && pack_name) {
-   int len = strlen(pack_name);
-   if (!ends_with(pack_name, ".pack"))
+   size_t len;
+   if (!strip_suffix(pack_name, ".pack", &len))
die(_("packfile name '%s' does not end with '.pack'"),
pack_name);
-   keep_name_buf = xmalloc(len);
-   memcpy(keep_name_buf, pack_name, len - 5);
-   strcpy(keep_name_buf + len - 5, ".keep");
-   keep_name = keep_name_buf;
+   strbuf_add(&keep_name_buf, pack_name, len);
+   strbuf_addstr(&keep_name_buf, ".idx");
+   keep_name = keep_name_buf.buf;
}
if (verify) {
if (!index_name)
@@ -1667,8 +1666,8 @@ int cmd_index_pack(int argc, const char **argv, const 
char *prefix)
else
close(input_fd);
free(objects);
-   free(index_name_buf);
-   free(keep_name_buf);
+   strbuf_release(&index_name_buf);
+   strbuf_release(&keep_name_buf);
if (pack_name == NULL)
free((void *) curr_pack);
if (index_name == NULL)
-- 
2.0.0.566.gfe3e6b2

--
To unsubscribe from this list: send the line "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 7/9] strbuf: implement strbuf_strip_suffix

[Jeff King]

You can almost get away with just calling "strip_suffix_mem"
on a strbuf's buf and len fields. But we also need to move
the NUL-terminator to satisfy strbuf's invariants. Let's
provide a convenience wrapper that handles this.

Signed-off-by: Jeff King 
---
I called strbuf_setlen here because it seemed sensible to use that as an
opaque building block, but we are violating the invariant here for a
moment by setting sb->len for a moment. I think that is fine, as this is
a strbuf function, and can violate the invariant for a moment if it
wants to.

But if we care, we can keep a separate "size_t len" variable, and
strbuf_setlen to that.

Or we can make it less opaque, and replace the setlen with
"sb->buf[sb->len] = 0".

 strbuf.h | 9 +
 1 file changed, 9 insertions(+)

diff --git a/strbuf.h b/strbuf.h
index e9ad03e..ec11742 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -49,6 +49,15 @@ extern int strbuf_reencode(struct strbuf *sb, const char 
*from, const char *to);
 extern void strbuf_tolower(struct strbuf *sb);
 extern int strbuf_cmp(const struct strbuf *, const struct strbuf *);
 
+static inline int strbuf_strip_suffix(struct strbuf *sb, const char *suffix)
+{
+   if (strip_suffix_mem(sb->buf, &sb->len, suffix)) {
+   strbuf_setlen(sb, sb->len);
+   return 1;
+   } else
+   return 0;
+}
+
 /*
  * Split str (of length slen) at the specified terminator character.
  * Return a null-terminated array of pointers to strbuf objects
-- 
2.0.0.566.gfe3e6b2

--
To unsubscribe from this list: send the line "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 8/9] verify-pack: use strbuf_strip_suffix

[Jeff King]

In this code, we try to convert both "foo.idx" and "foo"
into "foo.pack". By stripping the suffix, we can avoid a
confusing use of strbuf_splice, and make it clear that both
cases are adding ".pack" to the end.

Signed-off-by: Jeff King 
---
 builtin/verify-pack.c | 7 +++
 1 file changed, 3 insertions(+), 4 deletions(-)

diff --git a/builtin/verify-pack.c b/builtin/verify-pack.c
index 2fd29ce..972579f 100644
--- a/builtin/verify-pack.c
+++ b/builtin/verify-pack.c
@@ -27,10 +27,9 @@ static int verify_one_pack(const char *path, unsigned int 
flags)
 * normalize these forms to "foo.pack" for "index-pack --verify".
 */
strbuf_addstr(&arg, path);
-   if (ends_with(arg.buf, ".idx"))
-   strbuf_splice(&arg, arg.len - 3, 3, "pack", 4);
-   else if (!ends_with(arg.buf, ".pack"))
-   strbuf_add(&arg, ".pack", 5);
+   if (strbuf_strip_suffix(&arg, ".idx") ||
+   !ends_with(arg.buf, ".pack"))
+   strbuf_addstr(&arg, ".pack");
argv[2] = arg.buf;
 
memset(&index_pack, 0, sizeof(index_pack));
-- 
2.0.0.566.gfe3e6b2

--
To unsubscribe from this list: send the line "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 9/9] prepare_packed_git_one: refactor duplicate-pack check

[Jeff King]

When we are reloading the list of packs, we check whether a
particular pack has been loaded. This is slightly tricky,
because we load packs based on the presence of their ".idx"
files, but record the name of the matching ".pack" file.
Therefore we want to compare their bases.

The existing code stripped off ".idx" from a file we found,
then compared that whole base length to strings containing
the ".pack" version. This meant we could end up comparing
bytes past what the ".pack" string contained, if the ".idx"
file name was much longer.

In practice, it worked OK because memcmp would end up seeing
a difference in the two strings and would return early
before hitting the full length. However, memcmp may
sometimes read extra bytes past a difference (e.g., because
it is comparing 64-bit words), or is even free to compare in
reverse order.

Furthermore, our memcmp made no guarantees that we matched
the whole pack name, up to ".pack". So "foo.idx" would match
"foo-bar.pack", which is wrong (but does not typically
happen, because our pack names have a fixed size).

We can fix both issues, avoid magic numbers, and document
that we expect to compare against a string with ".pack" by
using strip_suffix.

Signed-off-by: Jeff King 
---
This is a teeny bit less efficient than it could be, because we are
verifying our assumption at run-time that each pack name ends in
".pack". I'd venture to say if we cared about efficiency here, the low
hanging fruit would be to avoid the O(n^2) loop to find duplicate pack
names in the first place.

 sha1_file.c | 9 +++--
 1 file changed, 7 insertions(+), 2 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 93b794f..129a4c5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1197,6 +1197,7 @@ static void prepare_packed_git_one(char *objdir, int 
local)
dirnamelen = path.len;
while ((de = readdir(dir)) != NULL) {
struct packed_git *p;
+   size_t base_len;
 
if (is_dot_or_dotdot(de->d_name))
continue;
@@ -1204,10 +1205,14 @@ static void prepare_packed_git_one(char *objdir, int 
local)
strbuf_setlen(&path, dirnamelen);
strbuf_addstr(&path, de->d_name);
 
-   if (ends_with(de->d_name, ".idx")) {
+   base_len = path.len;
+   if (strip_suffix_mem(path.buf, &base_len, ".idx")) {
/* Don't reopen a pack we already have. */
for (p = packed_git; p; p = p->next) {
-   if (!memcmp(path.buf, p->pack_name, path.len - 
4))
+   size_t len;
+   if (strip_suffix(p->pack_name, ".pack", &len) &&
+   len == base_len &&
+   !memcmp(p->pack_name, path.buf, len))
break;
}
if (p == NULL &&
-- 
2.0.0.566.gfe3e6b2
--
To unsubscribe from this list: send the line "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/8] add functions for memory-efficient bitmaps

[Jeff King]

On Sun, Jun 29, 2014 at 03:41:37AM -0400, Eric Sunshine wrote:

> > +static inline void bitset_set(unsigned char *bits, int n)
> > +{
> > +   bits[n / CHAR_BIT] |= 1 << (n % CHAR_BIT);
> > +}
> 
> Is it intentional or an oversight that there is no way to clear a bit
> in the set?

Intentional in the sense that I had no need for it in my series, and I
didn't think about it. I doubt many callers would want it, since commit
traversals tend to propagate bits through the graph, and then clean them
up all at once. And the right way to clean up slabbed data like this is
to just clear the slab.

Of course somebody may use the code for something besides commit
traversals. But I'd rather avoid adding dead code on the off chance that
somebody uses it later (and then gets to find out whether it even works
or not!).

-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 format-patch doesn't add Content-type for UTF-8 diffs

[Jeff King]

On Mon, Jun 30, 2014 at 02:03:25AM -0700, Paul Eggert wrote:

> I've been having trouble sending my Git-generated patches to the tz mailing
> list.  Patches containing UTF-8 text are garbled, e.g., if you visit
>  you'll see
> "Ãœrümqi" where the patch actually had "Ürümqi".
> 
> I've tracked this down to the fact that "git format-patch" isn't outputting
> a Content-Type: line in the outgoing email.  I thought it was supposed to do
> that; the man page implies that it does.

format-patch will add a content-type header if the commit message
contains non-ascii characters, and is marked as an alternate encoding
(usually this is utf8, but you can use i18n.commitEncoding to store them
in a different format).

However, it doesn't look at the filenames or diff contents at all. If it
were to do so, it would have to guess at the correct encoding, since git
doesn't know anything about the encoding of filenames or contents.
Worse, you could actually have several different encodings, across
multiple files in the same diff.

Typically, the next stage in the pipeline is to give the output to
send-email, or to a MUA. Send-email will detect high-bit characters in
this case and ask you which encoding you want. Many MUAs will do some
kind of auto-detection and fill in the content-type (e.g., I know that
mutt handles this correctly).

How do you send the mails after they come out of format-patch?

> Here's how I can reproduce the bug with the git 1.9.3 that's shipped with
> Fedora 20.  Notice that the patch is missing the line "Content-Type:
> text/plain; charset=UTF-8" that the git-format-patch man page implies it
> should be generating, and this causes the ICANN email software to
> misinterpret the patch's character set encoding.
> [...]

Thanks for the reproduction recipe. While I think we have to accept that
some hard cases (e.g., multiple encodings in a single diff) can't be
handled cleanly, it would be really nice if this all-utf8 case worked
out of the box. And perhaps the complex cases could use binary diffs
when we see multiple encodings.

One tricky thing about the implementation is that we stream the output
from format-patch, and write the content-type header (if any) before we
start opening the blobs for diff.

I wonder if it would be enough to do:

  1. Always add a content-type header, even if the commit is utf-8 and
 contains only ascii characters. This _shouldn't_ hurt anything,
 though I suppose it would if you have latin1 (for example) commit
 messages and did not correctly set the encoding header in your
 commits.

  2. When producing diff header lines do not respect core.quotepath if
 the filename does is not valid for the encoding we claimed earlier.

  3. When producing lines of textual diff, use a binary diff if the
 contents are not valid for the encoding we claimed earlier.

That would make the utf-8 case "just work", and would prevent us from
ever sending malformed contents (i.e., mismatched encodings in the
commit message and diff contents). However, it is not perfect:

  1. Right now if you send a diff for a latin1 file but do not use any
 non-ascii characters in your commit message (and do not set
 i18n.commitEncoding, so it is "utf8"), you get no claimed encoding
 in the email. If your receiving end is OK with that, everything
 works, and you get to see text diffs.

 So my scheme would be a slight regression there. But it is somewhat
 of an accident waiting to happen. If you ever use a utf-8 character
 in your commit message, that particular email will be marked as
 utf-8, and your diff will be broken.

  2. We can only check "is it valid?" for the encoding. That works well
 with utf-8, which has rules. But for something like latin1 versus
 another "use a code page for the high-bit bytes" type of encoding,
 we cannot really tell the difference. However, I do not think we
 are making anything _worse_ there. You'd already get mojibake in
 such a case.

-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

GOOD DAY,

[KONG HUI]

My name is Kong Hui from Hong Kong, I want you to be my partner in a
business project.

If Interested Contact me back via my email address.


Thank you,
Mr. Kong Hui.

--
To unsubscribe from this list: send the line "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/PATCH] notes-util.c: replace git_config with git_config_get_string

[Junio C Hamano]

Karsten Blees  writes:

> Which may not be too harmful in some cases, but if a user changes:
>
>  gc.pruneexpire=4.weeks.ago
>
> to
>
>  gc.pruneexpire=4.monhts.ago
>
> (note the typo), the next git-gc will warn the user and then happily throw
> away data that the user intended to keep (default is 2.weeks.ago).
>
> Thus I think git should die() if it encounters an invalid config setting.

Yes but not at the parsing time.  Only when we are about to _use_
the value for pruneexpire as a time duration we should die of the
error.  Diagnosing an error early is a separate matter, but if the
operation does not care about the typo we shouldn't die.
--
To unsubscribe from this list: send the line "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 log --show-signature breaks --graph formatting

[Jason Pyeron]

It looks like the gpg output is not parsed to prefix the correct number of
spaces. What interests me the most is the folowing line is also wrong.

$ git log  --graph
*   commit 258e0a237cb69aaa587b0a4fb528bb0316b1b776
|\  Merge: 727c355 1ca13ed
| | Author: Jason Pyeron 
| | Date:   Mon Jun 30 13:22:29 2014 -0400
| |
| | Merge of 1ca13ed2271d60ba93d40bcc8db17ced8545f172 branch - rebranding
| |
| * commit 1ca13ed2271d60ba93d40bcc8db17ced8545f172
| | Author: Stephen R Guglielmo 
| | Date:   Mon Jun 23 09:45:27 2014 -0400
| |
| | Minor URL updates
| |
| * commit d8587c43cfdb12343524ce117a4a59726c438925
| | Author: Stephen R Guglielmo 
| | Date:   Tue Jun 17 12:58:23 2014 -0400
| |
| | One final 'foundation' reference. Also fix a string length for the
rescue disk.
| |
| * commit 49ecdfd874029ea287d2755f7e0d5188ce49e81a
| | Author: Stephen R Guglielmo 
| | Date:   Tue Jun 17 12:44:36 2014 -0400


$ git log --show-signature --graph
*   commit 258e0a237cb69aaa587b0a4fb528bb0316b1b776
|\  gpg: Signature made Mon, Jun 30, 2014 13:22:33 EDT using RSA key ID DA0848AD
gpg: Good signature from "Jason Pyeron "
Merge: 727c355 1ca13ed
| | Author: Jason Pyeron 
| | Date:   Mon Jun 30 13:22:29 2014 -0400
| |
| | Merge of 1ca13ed2271d60ba93d40bcc8db17ced8545f172 branch - rebranding
| |
| * commit 1ca13ed2271d60ba93d40bcc8db17ced8545f172
| | gpg: Signature made Mon, Jun 23, 2014  9:45:47 EDT using RSA key ID DD3766FF
gpg: Good signature from "Stephen Robert Guglielmo "
gpg: aka "Stephen Robert Guglielmo "
gpg: WARNING: This key is not certified with a trusted signature!
gpg:  There is no indication that the signature belongs to the owner.
Primary key fingerprint: 0D54 BE6A B832 8701 AA94  9036 2D15 C7B0 DD37 66FF
Author: Stephen R Guglielmo 
| | Date:   Mon Jun 23 09:45:27 2014 -0400
| |
| | Minor URL updates
| |
| * commit d8587c43cfdb12343524ce117a4a59726c438925
| | gpg: Signature made Tue, Jun 17, 2014 12:58:30 EDT using RSA key ID DD3766FF
gpg: Good signature from "Stephen Robert Guglielmo "
gpg: aka "Stephen Robert Guglielmo "
gpg: WARNING: This key is not certified with a trusted signature!
gpg:  There is no indication that the signature belongs to the owner.
Primary key fingerprint: 0D54 BE6A B832 8701 AA94  9036 2D15 C7B0 DD37 66FF
Author: Stephen R Guglielmo 
| | Date:   Tue Jun 17 12:58:23 2014 -0400
| |
| | One final 'foundation' reference. Also fix a string length for the
rescue disk.
| |
| * commit 49ecdfd874029ea287d2755f7e0d5188ce49e81a
| | gpg: Signature made Tue, Jun 17, 2014 12:44:43 EDT using RSA key ID DD3766FF
gpg: Good signature from "Stephen Robert Guglielmo "
gpg: aka "Stephen Robert Guglielmo "
gpg: WARNING: This key is not certified with a trusted signature!
gpg:  There is no indication that the signature belongs to the owner.
Primary key fingerprint: 0D54 BE6A B832 8701 AA94  9036 2D15 C7B0 DD37 66FF
Author: Stephen R Guglielmo 
| | Date:   Tue Jun 17 12:44:36 2014 -0400
| |
| | Should be done!

--
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
-   -
- Jason Pyeron  PD Inc. http://www.pdinc.us -
- Principal Consultant  10 West 24th Street #100-
- +1 (443) 269-1555 x333Baltimore, Maryland 21218   -
-   -
-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-=-
This message is copyright PD Inc, subject to license 20080407P00.


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

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[Junio C Hamano]

David Turner  writes:

> During the commit process, the cache-tree is updated. We need to write
> this updated cache-tree so that it's ready for subsequent commands.
>
> Add test code which demonstrates that git commit now writes the cache
> tree.  Also demonstrate that  cache-tree invalidation is correct.
>
> Signed-off-by: David Turner 
> ---
>  builtin/commit.c  |  6 ++
>  t/t0090-cache-tree.sh | 50 +++---
>  2 files changed, 53 insertions(+), 3 deletions(-)
>
> diff --git a/builtin/commit.c b/builtin/commit.c
> index 9cfef6c..6814e87 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -607,6 +607,8 @@ static int prepare_to_commit(const char *index_file, 
> const char *prefix,
>   const char *hook_arg2 = NULL;
>   int clean_message_contents = (cleanup_mode != CLEANUP_NONE);
>   int old_display_comment_prefix;
> + static struct lock_file index_lock;
> + int index_fd;
>  
>   /* This checks and barfs if author is badly specified */
>   determine_author_info(author_ident);
> @@ -872,6 +874,10 @@ static int prepare_to_commit(const char *index_file, 
> const char *prefix,
>   error(_("Error building trees"));
>   return 0;
>   }
> + /* After updating the cache-tree, rewrite the index */
> + index_fd = hold_locked_index(&index_lock, 0);
> + if (index_fd >= 0 && write_index(&the_index, index_fd) >= 0)
> + commit_locked_index(&index_lock);

Is this run unconditionally even when we are making a partial commit
out of a temporary index file (which will be discarded after we
create this commit)?

I have a feeling that a better place to populate the cache-tree may
be prepare-index which knows the distinction between various modes
of commit, but I haven't looked at the code path for a while...
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[Junio C Hamano]

Duy Nguyen  writes:

> I wonder if we should do this in !opts->force code path only. In the
> opts->force code path we could use prime_cache_tree() (like
> read-tree), which is supposedly faster...

Nobody sane should be constantly running "checkout -f", so even if
priming from existing tree objects were faster, it would be adding
complexity to the code to optimize for a wrong case.

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

Re: [PATCH 2/2] t0027: combinations of core.autocrlf, core.eol and text

[Junio C Hamano]

Torsten Bögershausen  writes:

> Historically there are 3 different parameters controlling how line endings
> are handled by Git:
> - core.autocrlf
> - core.eol
> - the "text" attribute in .gitattributes
>
> There are different types of content:
> - (1) Files with only LF
> - (2) Files with only CRLF
> - (3) Files with mixed LF and CRLF
> - (4) Files with LF and/or CRLF with CR not followed by LF
> - (5) Files which are binary (e.g. have NUL bytes)
>
> Recently the question came up, how files with mixed EOLs are handled by Git
> (and libgit2) when they are checked out and core.autocrlf=true.

I have a slight suspicion that it may be better to leave behaviour
on some nonsensical combinations (e.g. 3 and 4) as "unspecified",
which would mean we shouldn't be able to pick between
test_expect_success and test_expect_failure for some of them.

> +test_description='CRLF conversion all combinations'
> +
> +check_files_in_ws()
> +{

(minor) Please have SP between "ws" and "()".


> +(
> + type od >/dev/null &&
> + printf "line1Q\r\nline2\r\nline3" | q_to_nul >CRLF_nul &&
> + cat >expect <<-EOF &&
> + 000 l i n e 1 \0 \r \n l i n e 2 \r \n l
> + 020 i n e 3
> + 024
> +EOF
> + od -c CRLF_nul | sed -e "s/[][   ]*/ /g" -e "s/ *$//" >actual
> + test_cmp expect actual &&
> + rm expect actual
> +) || {
> + skip_all="od not found or od -c not usable"
> + exit 0
> + test_done
> +}

I am not sure how cast-in-stone portable "od -c" output is across
platforms in practice.  You can use tr to convert NUL, CR and LF to
some distinctive and otherwise unused character, just like you use
q_to_nul to map NUL to "Q" already.  And that would allow you to
forget about such portability issues.


> +test_expect_success 'setup master' '
> + echo >.gitattributes &&
> + git checkout -b master &&
> + git add .gitattributes &&
> + git commit -m "add .gitattributes" "" &&
> + printf "line1\nline2\nline3" >LF &&
> + printf "line1\r\nline2\r\nline3" >CRLF &&
> + printf "line1\r\nline2\nline3"   >CRLF_mix_LF &&
> + printf "line1\nline2\rline3" >LF_mix_CR &&
> + printf "line1\r\nline2\rline3"   >CRLF_mix_CR &&
> + printf "line1Q\nline2\nline3" | q_to_nul >LF_nul

I do not see why you would want files that end with incomplete lines
for these.  Please have the line-end for the last line in them.
--
To unsubscribe from this list: send the line "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 format-patch doesn't add Content-type for UTF-8 diffs

[Paul Eggert]

Jeff King wrote:

How do you send the mails after they come out of format-patch?


I run a shell command like this (on Solaris 10):

/usr/lib/sendmail -ONoRecipientAction=add-to t...@iana.org < 
0001-whatever.patch


(The "NoRecipientAction" option pacifies the IANA MTA.)

This is an old machine not under my control, with an old 'git' installed 
that I don't use and don't particularly want to worry about porting to. 
 I generate the patch file on a different machine with git 1.9.3, and 
scp it into the email-sending machine.


I suppose that I could work around the problem with this shell command:

(grep -q '^Mime-Version: ' 0001-whatever.patch ||
   printf '%s\n' \
 'MIME-Version: 1.0' \
 'Content-Type: text/plain; charset=UTF-8' \
 'Content-Transfer-Encoding: 8bit'
 cat 0001-whatever.patch) |
/usr/lib/sendmail -ONoRecipientAction=add-to t...@iana.org

but that's less convenient.


I wonder if it would be enough to do:

  1. Always add a content-type header, even if the commit is utf-8 and
 contains only ascii characters.


That would help for my case, yes.  We use only UTF-8, and to me it 
feelds weird that patches are mailed properly if the commit log contains 
non-ASCII characters, but don't work if the commit log is ASCII and the 
diff contains non-ASCII.

--
To unsubscribe from this list: send the line "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 filter-branch to eliminate duplicate mapped parents

[Charles Bailey]

From: Charles Bailey 

When multiple parents of a merge commit get mapped to the same commit,
filter-branch used to pass all instances of the parent commit to the
parent and commit filters and to "git commit-tree" or
"git_commit_non_empty_tree".

This can often happen when extracting a small project from a large
repository; merges can join history with no commits on any branch which
affect the paths being retained. Once the intermediate commits have been
filtered out, all the immediate parents of the merge commit can end up
being mapped to the same commit - either the original merge-base or an
ancestor of it.

"git commit-tree" would display an error but write the commit with the
normalized parents in any case. "git_commit_non_empty_tree" would fail
to notice that the commit being made was in fact a non-merge commit and
would retain it even if a further pass with --prune-empty would discard
the commit as empty.

This change ensure that duplicate parents are pruned before the parent
filter and ensures that --prune-empty is idempotent, removing all
empty non-merge commits in a singe pass.

Signed-off-by: Charles Bailey 
---

I worked on this after discovering that --prune-empty often left some
apparently empty commits that I was wasn't expecting it to leave and
that running filter-branch --prune-empty in a loop would often do many
passes where it was still pruning empty former merge commits.

The test is a simple example of such a case. A non-ff merge of a commit
that only changes a file that is to be pruned gets squashed into an
empty non-merge commit that should be pruned.

 git-filter-branch.sh |  8 +++-
 t/t7003-filter-branch.sh | 11 +++
 2 files changed, 18 insertions(+), 1 deletion(-)

diff --git a/git-filter-branch.sh b/git-filter-branch.sh
index 86d6994..c5b82a8 100755
--- a/git-filter-branch.sh
+++ b/git-filter-branch.sh
@@ -332,7 +332,13 @@ while read commit parents; do
parentstr=
for parent in $parents; do
for reparent in $(map "$parent"); do
-   parentstr="$parentstr -p $reparent"
+   case "$parentstr" in
+   *" -p $reparent"*)
+   ;;
+   *)
+   parentstr="$parentstr -p $reparent"
+   ;;
+   esac
done
done
if [ "$filter_parent" ]; then
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 9496736..3741f51 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -308,6 +308,17 @@ test_expect_success 'Prune empty commits' '
test_cmp expect actual
 '
 
+test_expect_success 'Prune empty collapsed merges' '
+   test_config merge.ff false &&
+   git rev-list HEAD > expect &&
+   test_commit to_remove_2 &&
+   git reset --hard HEAD^ &&
+   test_merge non-ff to_remove_2 &&
+   git filter-branch -f --index-filter "git update-index --remove 
to_remove_2.t" --prune-empty HEAD &&
+   git rev-list HEAD > actual &&
+   test_cmp expect actual
+'
+
 test_expect_success '--remap-to-ancestor with filename filters' '
git checkout master &&
git reset --hard A &&
-- 
1.9.0

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

Re: How to populate index/worktree when recursive merge merges multiple common ancestors?

[Shawn Pearce]

On Mon, Jun 30, 2014 at 5:48 AM, Christian Halstrick
 wrote:
>> They don't. The conflicts are preserved into the virtual ancestor. The
>> user only sees the final conflicts during merging of A and B with
>> virtual X3 as the common ancestor.
>
> Ah, now I understand. When I merge X1 and X2 into the virtual X3
> I should not stop if this is not doable without conflict resolution. I
> should store in memory the X3 content, including all the conflict
> markers. If I finally merge A and B I will specify a common base
> content which may contain conflict markers. Right?

Yes.

If X3 content is large, it could flow to loose objects on disk. These
will be unreachable and cleaned up automatically in a future garbage
collection.

> Are git config param's like merge.conflictstyle=diff3 are also
> effective when creating the virtual X3 content? Couldn't that lead to
> complicated conflict marker situations? In the area where you expect
> common base content you again see conflict markers in diff3 style.

Yes, but I think this is the correct behavior. The machine can't
reconcile the two branches any better than this, so now a human has to
step in and fix all of the conflicts.

IIRC, this is uncommon. Usually you use A's common content as A and B
do not differ relative to X3 in the regions where X3 has a conflict,
so those conflicts aren't considered relevant when A and B merge.
--
To unsubscribe from this list: send the line "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/3] cache-tree: Create/update cache-tree on checkout

[David Turner]

When git checkout checks out a branch, create or update the
cache-tree so that subsequent operations are faster.

Signed-off-by: David Turner 
---
 builtin/checkout.c|  8 
 cache-tree.c  |  5 +++--
 t/t0090-cache-tree.sh | 15 ++-
 3 files changed, 25 insertions(+), 3 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 07cf555..a023a86 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -553,6 +553,14 @@ static int merge_working_tree(const struct checkout_opts 
*opts,
}
}
 
+   if (!active_cache_tree)
+   active_cache_tree = cache_tree();
+
+   if (!cache_tree_fully_valid(active_cache_tree))
+   cache_tree_update(active_cache_tree,
+ (const struct cache_entry * const 
*)active_cache,
+ active_nr, 0);
+
if (write_cache(newfd, active_cache, active_nr) ||
commit_locked_index(lock_file))
die(_("unable to write new index file"));
diff --git a/cache-tree.c b/cache-tree.c
index 7fa524a..28d2356 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -612,9 +612,10 @@ int write_cache_as_tree(unsigned char *sha1, int flags, 
const char *prefix)
cache_tree_find(active_cache_tree, prefix);
if (!subtree)
return WRITE_TREE_PREFIX_ERROR;
-   hashcpy(sha1, subtree->sha1);
+   if (sha1)
+   hashcpy(sha1, subtree->sha1);
}
-   else
+   else if (sha1)
hashcpy(sha1, active_cache_tree->sha1);
 
if (0 <= newfd)
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 6c33e28..7c60675 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives 
cache-tree' '
test_shallow_cache_tree
 '
 
-test_expect_failure 'checkout gives cache-tree' '
+test_expect_success 'checkout gives cache-tree' '
+   git tag current
git checkout HEAD^ &&
test_shallow_cache_tree
 '
 
+test_expect_success 'checkout -b gives cache-tree' '
+   git checkout current &&
+   git checkout -b prev HEAD^ &&
+   test_shallow_cache_tree
+'
+
+test_expect_success 'checkout -B gives cache-tree' '
+   git checkout current &&
+   git checkout -B prev HEAD^ &&
+   test_shallow_cache_tree
+'
+
 test_done
-- 
2.0.0.390.gcb682f8

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

[PATCH 2/3] test-dump-cache-tree: Improve output format and exit code

[David Turner]

Make test-dump-cache-tree more useful for testing.  Do not treat known
invalid trees as errors (and do not produce non-zero exit code),
because we can fall back to the non-cache-tree codepath.

Signed-off-by: David Turner 
---
 t/t0090-cache-tree.sh  | 28 +---
 test-dump-cache-tree.c | 24 
 2 files changed, 37 insertions(+), 15 deletions(-)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 7c60675..5383258 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -21,10 +21,13 @@ test_shallow_cache_tree () {
cmp_cache_tree expect
 }
 
+# Test that the cache-tree for a given directory is invalid.
+# If no directory is given, check that the root is invalid
 test_invalid_cache_tree () {
-   echo "invalid   (0 subtrees)" >expect &&
-   printf "SHA #(ref)  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) 
>>expect &&
-   cmp_cache_tree expect
+   test-dump-cache-tree >actual &&
+   sed -e "s/$_x40/SHA/" -e "s/[0-9]* subtrees//g" filtered &&
+   expect=$(printf "invalid  $1 ()\n") &&
+   fgrep "$expect" filtered
 }
 
 test_no_cache_tree () {
@@ -49,6 +52,25 @@ test_expect_success 'git-add invalidates cache-tree' '
test_invalid_cache_tree
 '
 
+test_expect_success 'git-add in subdir invalidates cache-tree' '
+   test_when_finished "git reset --hard; git read-tree HEAD" &&
+   mkdir dirx &&
+   echo "I changed this file" > dirx/foo &&
+   git add dirx/foo &&
+   test_invalid_cache_tree
+'
+
+test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' 
'
+   git tag no-children
+   test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
+   mkdir dir1 dir2 &&
+   test_commit dir1/a &&
+   test_commit dir2/b &&
+   echo "I changed this file" > dir1/a &&
+   git add dir1/a &&
+   test_invalid_cache_tree dir1/
+'
+
 test_expect_success 'update-index invalidates cache-tree' '
test_when_finished "git reset --hard; git read-tree HEAD" &&
echo "I changed this file" > foo &&
diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
index 47eab97..ad42ca1 100644
--- a/test-dump-cache-tree.c
+++ b/test-dump-cache-tree.c
@@ -6,12 +6,12 @@
 static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
 {
if (it->entry_count < 0)
-   printf("%-40s %s%s (%d subtrees)\n",
-  "invalid", x, pfx, it->subtree_nr);
+   printf("%-40s %s (%d subtrees)%s\n",
+  "invalid", pfx, it->subtree_nr, x);
else
-   printf("%s %s%s (%d entries, %d subtrees)\n",
-  sha1_to_hex(it->sha1), x, pfx,
-  it->entry_count, it->subtree_nr);
+   printf("%s %s (%d entries, %d subtrees)%s\n",
+  sha1_to_hex(it->sha1), pfx,
+  it->entry_count, it->subtree_nr, x);
 }
 
 static int dump_cache_tree(struct cache_tree *it,
@@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
/* missing in either */
return 0;
 
-   if (it->entry_count < 0) {
+   if (it->entry_count < 0)
+   /* invalid */
dump_one(it, pfx, "");
-   dump_one(ref, pfx, "#(ref) ");
-   if (it->subtree_nr != ref->subtree_nr)
-   errs = 1;
-   }
else {
-   dump_one(it, pfx, "");
if (hashcmp(it->sha1, ref->sha1) ||
ref->entry_count != it->entry_count ||
ref->subtree_nr != it->subtree_nr) {
-   dump_one(ref, pfx, "#(ref) ");
+   /* claims to be valid but is lying */
+   dump_one(ref, pfx, " #(error)");
errs = 1;
+   } else {
+   /* is actually valid */
+   dump_one(it, pfx, "");
}
}
 
-- 
2.0.0.390.gcb682f8

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

[PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[David Turner]

During the commit process, the cache-tree is updated. We need to write
this updated cache-tree so that it's ready for subsequent commands.

Add test code which demonstrates that git commit now writes the cache
tree.  Also demonstrate that cache-tree invalidation is correct.

Signed-off-by: David Turner 
---
 builtin/commit.c  | 15 ++---
 t/t0090-cache-tree.sh | 61 ---
 2 files changed, 65 insertions(+), 11 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index 9cfef6c..dbd4f4b 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -342,6 +342,8 @@ static char *prepare_index(int argc, const char **argv, 
const char *prefix,
 
discard_cache();
read_cache_from(index_lock.filename);
+   if (update_main_cache_tree(WRITE_TREE_SILENT) >= 0)
+   write_cache(fd, active_cache, active_nr);
 
commit_style = COMMIT_NORMAL;
return index_lock.filename;
@@ -383,14 +385,10 @@ static char *prepare_index(int argc, const char **argv, 
const char *prefix,
if (!only && !pathspec.nr) {
fd = hold_locked_index(&index_lock, 1);
refresh_cache_or_die(refresh_flags);
-   if (active_cache_changed) {
-   update_main_cache_tree(WRITE_TREE_SILENT);
-   if (write_cache(fd, active_cache, active_nr) ||
-   commit_locked_index(&index_lock))
-   die(_("unable to write new_index file"));
-   } else {
-   rollback_lock_file(&index_lock);
-   }
+   update_main_cache_tree(WRITE_TREE_SILENT);
+   if (write_cache(fd, active_cache, active_nr) ||
+   commit_locked_index(&index_lock))
+   die(_("unable to write new_index file"));
commit_style = COMMIT_AS_IS;
return get_index_file();
}
@@ -435,6 +433,7 @@ static char *prepare_index(int argc, const char **argv, 
const char *prefix,
fd = hold_locked_index(&index_lock, 1);
add_remove_files(&partial);
refresh_cache(REFRESH_QUIET);
+   update_main_cache_tree(WRITE_TREE_SILENT);
if (write_cache(fd, active_cache, active_nr) ||
close_lock_file(&index_lock))
die(_("unable to write new_index file"));
diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 5383258..d50acb8 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -16,8 +16,31 @@ cmp_cache_tree () {
 # We don't bother with actually checking the SHA1:
 # test-dump-cache-tree already verifies that all existing data is
 # correct.
+generate_expected_cache_tree () {
+   if [ -n "$1" ]
+   then
+   local dir="$1/"
+   else
+   local dir="$1"
+   fi
+   # ls-files might have foo/bar, foo/bar/baz, and foo/bar/quux
+   # We want to count only foo because it's the only direct child
+   local subtrees=$(git ls-files|egrep '/' |cut -d / -f 1|uniq) &&
+   local subtree_count=$(echo "$subtrees"|grep -c .) &&
+   local files_count=$(git ls-files|grep -v /|wc -l) &&
+   local entries=$(expr "$subtree_count" + "$files_count") &&
+   printf "SHA $dir (%d entries, %d subtrees)\n" $entries $subtree_count &&
+   local subtree &&
+   for subtree in $subtrees
+   do
+   cd "$subtree"
+   generate_expected_cache_tree "$dir$subtree" || return 1
+   cd ..
+   done
+}
+
 test_shallow_cache_tree () {
-   printf "SHA  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) >expect 
&&
+   generate_expected_cache_tree > expect &&
cmp_cache_tree expect
 }
 
@@ -35,7 +58,7 @@ test_no_cache_tree () {
cmp_cache_tree expect
 }
 
-test_expect_failure 'initial commit has cache-tree' '
+test_expect_success 'initial commit has cache-tree' '
test_commit foo &&
test_shallow_cache_tree
 '
@@ -60,15 +83,28 @@ test_expect_success 'git-add in subdir invalidates 
cache-tree' '
test_invalid_cache_tree
 '
 
+cat >before <<\EOF
+SHA  (3 entries, 2 subtrees)
+SHA dir1/ (1 entries, 0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
+cat >expect <<\EOF
+invalid   (2 subtrees)
+invalid  dir1/ (0 subtrees)
+SHA dir2/ (1 entries, 0 subtrees)
+EOF
+
 test_expect_success 'git-add in subdir does not invalidate sibling cache-tree' 
'
git tag no-children
test_when_finished "git reset --hard no-children; git read-tree HEAD" &&
mkdir dir1 dir2 &&
test_commit dir1/a &&
test_commit dir2/b &&
+   cmp_cache_tree before &&
echo "I changed this file" > dir1/a &&
git add dir1/a &&
-   test_invalid_cache_tree dir1/
+   cmp_cache_tree expect
 '
 
 test_expect_success 'update-index invalidates cache-tree' '
@

Re: [PATCH 0/9] add strip_suffix as an alternative to ends_with

[Junio C Hamano]

On Mon, Jun 30, 2014 at 9:55 AM, Jeff King  wrote:
> Here's a series to do for ends_with what the recent skip_prefix series
> did for starts_with. Namely: drop some magic numbers and repeated string
> literals, and hopefully make things more readable.
>
> The first patch is René's patch 1/2, with the leak fix from Duy and typo
> fixes in the commit message from me. The rest are new, and replace the
> changes to prepare_packed_git_one done elsewhere in the thread.
>
>   [1/9]: sha1_file: replace PATH_MAX buffer with strbuf in 
> prepare_packed_git_one()
>   [2/9]: add strip_suffix function
>   [3/9]: implement ends_with via strip_suffix
>   [4/9]: replace has_extension with ends_with
>   [5/9]: use strip_suffix instead of ends_with in simple cases
>   [6/9]: index-pack: use strip_suffix to avoid magic numbers
>   [7/9]: strbuf: implement strbuf_strip_suffix
>   [8/9]: verify-pack: use strbuf_strip_suffix
>   [9/9]: prepare_packed_git_one: refactor duplicate-pack check

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

Re: [PATCH 1/3] cache-tree: Create/update cache-tree on checkout

[Torsten Bögershausen]

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 6c33e28..7c60675 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -85,9 +85,22 @@ test_expect_success 'reset --hard without index gives 
cache-tree' '
test_shallow_cache_tree
 '
 
-test_expect_failure 'checkout gives cache-tree' '

+test_expect_success 'checkout gives cache-tree' '
+   git tag current
git checkout HEAD^ &&
test_shallow_cache_tree

The && chainis broken here.
Does the test now pass, because "git tag" is added ?
In this case: does it may make sense the keep the old one as it is
an  add a new test case  like this ?

+test_expect_success 'tag and checkout gives cache-tree' '

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

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[Torsten Bögershausen]

[]

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 5383258..d50acb8 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -16,8 +16,31 @@ cmp_cache_tree () {
 # We don't bother with actually checking the SHA1:
 # test-dump-cache-tree already verifies that all existing data is
 # correct.
+generate_expected_cache_tree () {
+   if [ -n "$1" ]
+   then
+   local dir="$1/"
+   else
+   local dir="$1"
+   fi

I think the Git test cases prefer "test" over "[]":

if test -n "$1"
then
local dir="$1/"
else
local dir="$1"
fi

And should there be a && after the "fi" ?
And as test -n tests for a non-zero string,
could we write like this (and drop the local ?)?

if test -n "$1"
then
dir="$1/"
else
dir=""
fi



--
To unsubscribe from this list: send the line "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 format-patch doesn't add Content-type for UTF-8 diffs

[Torsten Bögershausen]

I wonder if it would be enough to do:



 1. Always add a content-type header, even if the commit is utf-8 and
contains only ascii characters. This _shouldn't_ hurt anything,
though I suppose it would if you have latin1 (for example) commit
messages and did not correctly set the encoding header in your
commits.


Does it make sense to call this function (from utf8.c)

int is_utf8(const char *text)

and either add the content-type header for utf-8 (or not)


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

Re: [PATCH 2/3] test-dump-cache-tree: Improve output format and exit code

[Eric Sunshine]

On Mon, Jun 30, 2014 at 8:13 PM, David Turner  wrote:
> Make test-dump-cache-tree more useful for testing.  Do not treat known
> invalid trees as errors (and do not produce non-zero exit code),
> because we can fall back to the non-cache-tree codepath.
>
> Signed-off-by: David Turner 
> ---
>  t/t0090-cache-tree.sh  | 28 +---
>  test-dump-cache-tree.c | 24 
>  2 files changed, 37 insertions(+), 15 deletions(-)
>
> diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
> index 7c60675..5383258 100755
> --- a/t/t0090-cache-tree.sh
> +++ b/t/t0090-cache-tree.sh
> @@ -21,10 +21,13 @@ test_shallow_cache_tree () {
> cmp_cache_tree expect
>  }
>
> +# Test that the cache-tree for a given directory is invalid.
> +# If no directory is given, check that the root is invalid
>  test_invalid_cache_tree () {
> -   echo "invalid   (0 subtrees)" >expect 
> &&
> -   printf "SHA #(ref)  (%d entries, 0 subtrees)\n" $(git ls-files|wc -l) 
> >>expect &&
> -   cmp_cache_tree expect
> +   test-dump-cache-tree >actual &&
> +   sed -e "s/$_x40/SHA/" -e "s/[0-9]* subtrees//g" filtered &&
> +   expect=$(printf "invalid  $1 ()\n") &&
> +   fgrep "$expect" filtered
>  }
>
>  test_no_cache_tree () {
> @@ -49,6 +52,25 @@ test_expect_success 'git-add invalidates cache-tree' '
> test_invalid_cache_tree
>  '
>
> +test_expect_success 'git-add in subdir invalidates cache-tree' '
> +   test_when_finished "git reset --hard; git read-tree HEAD" &&
> +   mkdir dirx &&
> +   echo "I changed this file" > dirx/foo &&

Style: drop whitespace after >

> +   git add dirx/foo &&
> +   test_invalid_cache_tree
> +'
> +
> +test_expect_success 'git-add in subdir does not invalidate sibling 
> cache-tree' '
> +   git tag no-children

Broken &&-chain.

> +   test_when_finished "git reset --hard no-children; git read-tree HEAD" 
> &&
> +   mkdir dir1 dir2 &&
> +   test_commit dir1/a &&
> +   test_commit dir2/b &&
> +   echo "I changed this file" > dir1/a &&

Style: drop whitespace after >

> +   git add dir1/a &&
> +   test_invalid_cache_tree dir1/
> +'
> +
>  test_expect_success 'update-index invalidates cache-tree' '
> test_when_finished "git reset --hard; git read-tree HEAD" &&
> echo "I changed this file" > foo &&
> diff --git a/test-dump-cache-tree.c b/test-dump-cache-tree.c
> index 47eab97..ad42ca1 100644
> --- a/test-dump-cache-tree.c
> +++ b/test-dump-cache-tree.c
> @@ -6,12 +6,12 @@
>  static void dump_one(struct cache_tree *it, const char *pfx, const char *x)
>  {
> if (it->entry_count < 0)
> -   printf("%-40s %s%s (%d subtrees)\n",
> -  "invalid", x, pfx, it->subtree_nr);
> +   printf("%-40s %s (%d subtrees)%s\n",
> +  "invalid", pfx, it->subtree_nr, x);
> else
> -   printf("%s %s%s (%d entries, %d subtrees)\n",
> -  sha1_to_hex(it->sha1), x, pfx,
> -  it->entry_count, it->subtree_nr);
> +   printf("%s %s (%d entries, %d subtrees)%s\n",
> +  sha1_to_hex(it->sha1), pfx,
> +  it->entry_count, it->subtree_nr, x);
>  }
>
>  static int dump_cache_tree(struct cache_tree *it,
> @@ -25,19 +25,19 @@ static int dump_cache_tree(struct cache_tree *it,
> /* missing in either */
> return 0;
>
> -   if (it->entry_count < 0) {
> +   if (it->entry_count < 0)
> +   /* invalid */
> dump_one(it, pfx, "");
> -   dump_one(ref, pfx, "#(ref) ");
> -   if (it->subtree_nr != ref->subtree_nr)
> -   errs = 1;
> -   }
> else {
> -   dump_one(it, pfx, "");
> if (hashcmp(it->sha1, ref->sha1) ||
> ref->entry_count != it->entry_count ||
> ref->subtree_nr != it->subtree_nr) {
> -   dump_one(ref, pfx, "#(ref) ");
> +   /* claims to be valid but is lying */
> +   dump_one(ref, pfx, " #(error)");
> errs = 1;
> +   } else {
> +   /* is actually valid */
> +   dump_one(it, pfx, "");
> }
> }
>
> --
> 2.0.0.390.gcb682f8
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 3/3] cache-tree: Write index with updated cache-tree after commit

[Johannes Sixt]

Am 7/1/2014 6:26, schrieb Torsten Bögershausen:
> And as test -n tests for a non-zero string,
> could we write like this (and drop the local ?)?
> 
> if test -n "$1"
> then
> dir="$1/"
> else
> dir=""
> fi

These six lines can be written as

dir="$1${1:+/}" &&

and 'local' must be dropped because is not universally supported, yet.

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