Re: [PATCH v4] branch.c: change install_branch_config() to use skip_prefix()

[Eric Sunshine]

On Mon, Mar 3, 2014 at 1:36 AM, Guanglin Xu  wrote:
> to avoid a magic code of 11.
>
> Helped-by: Sun He 
> Helped-by: Eric Sunshine 
> Helped-by: Jacopo Notarstefano 
>
> Signed-off-by: Guanglin Xu 
> ---
>
> This is an implementation of the idea#2 of GSoC 2014 microproject.
>
>  branch.c | 8 ++--
>  1 file changed, 6 insertions(+), 2 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..4eec0ac 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -49,8 +49,12 @@ static int should_setup_rebase(const char *origin)
>
>  void install_branch_config(int flag, const char *local, const char *origin, 
> const char *remote)
>  {
> -   const char *shortname = remote + 11;
> -   int remote_is_branch = starts_with(remote, "refs/heads/");
> +   const char *shortname = skip_prefix(remote ,"refs/heads/");
> +   int remote_is_branch;
> +   if (NULL == shortname)
> +   remote_is_branch = 0;
> +   else
> +   remote_is_branch = 1;

A bit verbose. Perhaps just:

int remote_is_branch = !!shortname;

which you will find to be idiomatic in this project.

> struct strbuf key = STRBUF_INIT;
> int rebasing = should_setup_rebase(origin);
>
> --
> 1.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 00/11] Use ALLOC_GROW() instead of inline code

[Eric Sunshine]

On Mon, Mar 3, 2014 at 2:18 AM, Dmitry S. Dolzhenko
 wrote:
> Dmitry S. Dolzhenko (11):
>   builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
>   bundle.c: use ALLOC_GROW() in add_to_ref_list()
>   cache-tree.c: use ALLOC_GROW() in find_subtree()
>   commit.c: use ALLOC_GROW() in register_commit_graft()
>   diff.c: use ALLOC_GROW()
>   diffcore-rename.c: use ALLOC_GROW()
>   patch-ids.c: use ALLOC_GROW() in add_commit()
>   replace_object.c: use ALLOC_GROW() in register_replace_object()
>   reflog-walk.c: use ALLOC_GROW()
>   dir.c: use ALLOC_GROW() in create_simplify()
>   attr.c: use ALLOC_GROW() in handle_attr_line()
>
>  attr.c |  7 +--
>  builtin/pack-objects.c |  9 +++--
>  bundle.c   |  6 +-
>  cache-tree.c   |  6 +-
>  commit.c   |  8 ++--
>  diff.c | 12 ++--
>  diffcore-rename.c  | 12 ++--
>  dir.c  |  5 +
>  patch-ids.c|  5 +
>  reflog-walk.c  | 12 ++--
>  replace_object.c   |  8 ++--
>  11 files changed, 18 insertions(+), 72 deletions(-)
>
> --
> 1.8.5.3
>
> This version differs from previous only minor changes:
>   - update commit messages
>   - keep code lines within 80 columns

Place this commentary at the top of the cover letter since that's
where people look for it.

You want to ease the reviewer's job as much as possible, so it helps
to link to the previous submission, like this [1].

Likewise, you can help the reviewer by being more specific about how
you updated the commit messages (and perhaps by linking to the
relevant discussion points, like this [2][3]).

[1]: http://thread.gmane.org/gmane.comp.version-control.git/242857
[2]: http://article.gmane.org/gmane.comp.version-control.git/243004
[3]: http://article.gmane.org/gmane.comp.version-control.git/243049
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 12/27] *.sh: avoid hardcoding $GIT_DIR/hooks/...

[Eric Sunshine]

On Sat, Mar 1, 2014 at 7:12 AM, Nguyễn Thái Ngọc Duy  wrote:
> If $GIT_COMMON_DIR is set, it should be $GIT_COMMON_DIR/hooks/, not
> $GIT_DIR/hooks/. Just let rev-parse --git-path handle it.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  git-am.sh  | 22 +++---
>  git-rebase--interactive.sh |  6 +++---
>  git-rebase--merge.sh   |  6 ++
>  git-rebase.sh  |  4 ++--
>  templates/hooks--applypatch-msg.sample |  4 ++--
>  templates/hooks--pre-applypatch.sample |  4 ++--
>  6 files changed, 22 insertions(+), 24 deletions(-)
>
> diff --git a/git-am.sh b/git-am.sh
> index bbea430..dfa0618 100755
> --- a/git-am.sh
> +++ b/git-am.sh
> @@ -803,10 +803,10 @@ To restore the original branch and stop patching run 
> \"\$cmdline --abort\"."
> continue
> fi
>
> -   if test -x "$GIT_DIR"/hooks/applypatch-msg
> +   hook="`git rev-parse --git-path hooks/applypatch-msg`"

Did you want to use $(...) rather than `...`?

Same question for the remainder of the patch.

> +   if test -x "$hook"
> then
> -   "$GIT_DIR"/hooks/applypatch-msg "$dotest/final-commit" ||
> -   stop_here $this
> +   "$hook" "$dotest/final-commit" || stop_here $this
> fi
>
> if test -f "$dotest/final-commit"
> @@ -880,9 +880,10 @@ did you forget to use 'git add'?"
> stop_here_user_resolve $this
> fi
>
> -   if test -x "$GIT_DIR"/hooks/pre-applypatch
> +   hook="`git rev-parse --git-path hooks/pre-applypatch`"
> +   if test -x "$hook"
> then
> -   "$GIT_DIR"/hooks/pre-applypatch || stop_here $this
> +   "$hook" || stop_here $this
> fi
>
> tree=$(git write-tree) &&
> @@ -908,18 +909,17 @@ did you forget to use 'git add'?"
> echo "$(cat "$dotest/original-commit") $commit" >> 
> "$dotest/rewritten"
> fi
>
> -   if test -x "$GIT_DIR"/hooks/post-applypatch
> -   then
> -   "$GIT_DIR"/hooks/post-applypatch
> -   fi
> +   hook="`git rev-parse --git-path hooks/post-applypatch`"
> +   test -x "$hook" && "$hook"
>
> go_next
>  done
>
>  if test -s "$dotest"/rewritten; then
>  git notes copy --for-rewrite=rebase < "$dotest"/rewritten
> -if test -x "$GIT_DIR"/hooks/post-rewrite; then
> -   "$GIT_DIR"/hooks/post-rewrite rebase < "$dotest"/rewritten
> +hook="`git rev-parse --git-path hooks/post-rewrite`"
> +if test -x "$hook"; then
> +   "$hook" rebase < "$dotest"/rewritten
>  fi
>  fi
>
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 43c19e0..d741b04 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -632,9 +632,9 @@ do_next () {
> git notes copy --for-rewrite=rebase < "$rewritten_list" ||
> true # we don't care if this copying failed
> } &&
> -   if test -x "$GIT_DIR"/hooks/post-rewrite &&
> -   test -s "$rewritten_list"; then
> -   "$GIT_DIR"/hooks/post-rewrite rebase < "$rewritten_list"
> +   hook="`git rev-parse --git-path hooks/post-rewrite`"
> +   if test -x "$hook" && test -s "$rewritten_list"; then
> +   "$hook" rebase < "$rewritten_list"
> true # we don't care if this hook failed
> fi &&
> warn "Successfully rebased and updated $head_name."
> diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
> index e7d96de..68f5d09 100644
> --- a/git-rebase--merge.sh
> +++ b/git-rebase--merge.sh
> @@ -93,10 +93,8 @@ finish_rb_merge () {
> if test -s "$state_dir"/rewritten
> then
> git notes copy --for-rewrite=rebase <"$state_dir"/rewritten
> -   if test -x "$GIT_DIR"/hooks/post-rewrite
> -   then
> -   "$GIT_DIR"/hooks/post-rewrite rebase 
> <"$state_dir"/rewritten
> -   fi
> +   hook="`git rev-parse --git-path hooks/post-rewrite`"
> +   test -x "$hook" && "$hook" rebase <"$state_dir"/rewritten
> fi
> say All done.
>  }
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 8a3efa2..1cf8dba 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -195,9 +195,9 @@ run_specific_rebase () {
>
>  run_pre_rebase_hook () {
> if test -z "$ok_to_skip_pre_rebase" &&
> -  test -x "$GIT_DIR/hooks/pre-rebase"
> +  test -x "`git rev-parse --git-path hooks/pre-rebase`"
> then
> -   "$GIT_DIR/hooks/pre-rebase" ${1+"$@"} ||
> +   "`git rev-parse --git-path hooks/pre-rebase`" ${1+"$@"} ||
> die "$(gettext "The pre-rebase hook refused to rebase.")"
> fi
>  }
> diff --git a/templates/hooks--applypatch-msg.sample 
> b/templates/hooks--applypatch-msg.sample
> index 8b2a2fe..28b843b 100755
> --- a/templates/hooks--applypatch-msg.sample
> +++ b/tem

Re: [PATCH] commit.c: Replace starts_with() with skip_prefix()

[Eric Sunshine]

Thanks for the submission. Some commentary below to expose you to the
review process on this project...

On Mon, Mar 3, 2014 at 2:47 AM, Karthik Nayak  wrote:
> Replace with skip_prefix(), which uses the inbuilt function
> strcmp() to compare.

Explaining the purpose of the change is indeed important, however,
this description misses the mark. See below.

> Other Places were this can be implemented:
> commit.c : line 1117
> commit.c : line 1197

Bonus points if you actually follow through with this.

> Signed-off-by: Karthik Nayak 
> ---
>  commit.c | 3 ++-
>  1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..1e181cf 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -552,6 +552,7 @@ static void record_author_date(struct author_date_slab 
> *author_date,
> char *buffer = NULL;
> struct ident_split ident;
> char *date_end;
> +   char *flag_sp; /* stores return value of skip_prefix() */

It's not clear why this variable is needed. It's only assigned (below)
but never referenced. Also, the name conveys little or no meaning. If
you need a comment to explain the purpose of the variable, that's
probably a good indication that it's poorly named.

> unsigned long date;
>
> if (!commit->buffer) {
> @@ -566,7 +567,7 @@ static void record_author_date(struct author_date_slab 
> *author_date,
>  buf;
>  buf = line_end + 1) {
> line_end = strchrnul(buf, '\n');
> -   if (!starts_with(buf, "author ")) {
> +   if ((flag_sp = skip_prefix(buf, "author ")) == NULL) {

Unfortunately, this change makes the code more difficult to read.
Let's review the GSoC microproject:

Rewrite commit.c:record_author_date() to use skip_prefix().
Are there other places in this file where skip_prefix() would
be **more readable** than starts_with()?

Note the part I **highlighted**. This is a good clue that use of
skip_prefix() can be used to simplify the code. Examine
record_author_date() more carefully and see if you can identify how to
do so.

> if (!line_end[0] || line_end[1] == '\n')
> return; /* end of header */
> continue;
> --
> 1.9.0.138.g2de3478
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4] branch.c: change install_branch_config() to use skip_prefix()

[Guanglin Xu]

Hi Eric,

Yes, you're right. "!!" is comfortably concise and also idiomatic in
Git sources.

Thanks,

Guanglin

2014-03-03 16:12 GMT+08:00 Eric Sunshine :
> On Mon, Mar 3, 2014 at 1:36 AM, Guanglin Xu  wrote:
>> to avoid a magic code of 11.
>>
>> Helped-by: Sun He 
>> Helped-by: Eric Sunshine 
>> Helped-by: Jacopo Notarstefano 
>>
>> Signed-off-by: Guanglin Xu 
>> ---
>>
>> This is an implementation of the idea#2 of GSoC 2014 microproject.
>>
>>  branch.c | 8 ++--
>>  1 file changed, 6 insertions(+), 2 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 723a36b..4eec0ac 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -49,8 +49,12 @@ static int should_setup_rebase(const char *origin)
>>
>>  void install_branch_config(int flag, const char *local, const char *origin, 
>> const char *remote)
>>  {
>> -   const char *shortname = remote + 11;
>> -   int remote_is_branch = starts_with(remote, "refs/heads/");
>> +   const char *shortname = skip_prefix(remote ,"refs/heads/");
>> +   int remote_is_branch;
>> +   if (NULL == shortname)
>> +   remote_is_branch = 0;
>> +   else
>> +   remote_is_branch = 1;
>
> A bit verbose. Perhaps just:
>
> int remote_is_branch = !!shortname;
>
> which you will find to be idiomatic in this project.
>
>> struct strbuf key = STRBUF_INIT;
>> int rebasing = should_setup_rebase(origin);
>>
>> --
>> 1.9.0
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] commit.c:record_author_date() use skip_prefix() instead of starts_with()

[Michael Haggerty]

The format of this email is wrong.  The non-commit-message notes should
come between the "---" line (<- note, there are three minus signs here)
and the patch itself.

On 03/01/2014 08:48 PM, Tanay Abhra wrote:
> Signed-off-by: Tanay Abhra 
> ---
>  commit.c |2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
> 
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..c954ecb 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -566,7 +566,7 @@ static void record_author_date(struct author_date_slab 
> *author_date,
>buf;
>buf = line_end + 1) {
>   line_end = strchrnul(buf, '\n');
> - if (!starts_with(buf, "author ")) {
> + if (!skip_prefix(buf, "author ")) {

If this is the only change, there is not much point, is there?  How does
this help?  Perhaps there is some way to take advantage of the
difference between starts_with() and skip_prefix() to simplify the rest
of the function?

>   if (!line_end[0] || line_end[1] == '\n')
>   return; /* end of header */
>   continue;
> -- 
> 1.7.9.5
> 
> Hello,
> 
> This is my patch for the GSoC microproject #10:
> 
> Rewrite commit.c:record_author_date() to use skip_prefix(). 
> Are there other places in this file where skip_prefix() would be more 
> readable than starts_with()?
> 
> Since skip_prefix() and starts_with() implement the same functionality with 
> different
> return values, they can be interchanged easily.
> 
> Other usage of starts_with() in the same file can be found with
> 
> $ grep -n starts_with commit.c
> 
> 1116: else if (starts_with(line, gpg_sig_header) &&
> 1196: if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {

The rhetorical question that was part of this microproject was meant to
inspire you to actually *FIX* the other spots, at least if the change
makes sense.

> I have a query,should I tackle a bug from the mailing lists or research about 
> the proposal 
> and present a rough draft?

My suggestion is that you follow up on this microproject until it is
perfect before worrying too much about the next step.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v5] branch.c: change install_branch_config() to use skip_prefix()

[Guanglin Xu]

to avoid a magic code of 11.

Helped-by: Sun He 
Helped-by: Eric Sunshine 
Helped-by: Jacopo Notarstefano 

Signed-off-by: Guanglin Xu 
---

This is an implementation of the idea#2 of GSoC 2014 microproject.

 branch.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..fae7715 100644
--- a/branch.c
+++ b/branch.c
@@ -49,8 +49,8 @@ static int should_setup_rebase(const char *origin)
 
 void install_branch_config(int flag, const char *local, const char *origin, 
const char *remote)
 {
-   const char *shortname = remote + 11;
-   int remote_is_branch = starts_with(remote, "refs/heads/");
+   const char *shortname = skip_prefix(remote ,"refs/heads/");
+   int remote_is_branch = !!shortname;
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
 
-- 
1.9.0

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

Re: [PATCH 2/3] rebase: accept - as another way of saying HEAD~

[Michael Haggerty]

On 03/02/2014 04:55 PM, Matthieu Moy wrote:
> Eric Sunshine  writes:
> 
>> On Sun, Mar 2, 2014 at 3:53 AM, Eric Sunshine  
>> wrote:
>>> On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy  
>>> wrote:
 This is "rev-list style", where people can do "git rev-list -3" in
 addition to "git rev-list HEAD~3". A lot of commands are driven by the
 revision machinery and also accept this form. This addition to rebase
 is just for convenience.
>>>
>>> I'm seeing some pretty strange results with this. If I use -1, -2, or
>>> -3 then it rebases the expected commits, however, -4 gives me 9
>>> commits, and -5 rebases 35 commits. Am I misunderstanding how this
>>> works?
>>
>> Nevermind. I wasn't paying attention to the fact that I was attempting
>> to rebase merges.
> 
> Your remark is actually interesting. Most (all?) Git commands taking
> - as parameters act on n commits, regardless of merges.
> 
> So, this commit creates an inconsistency between e.g. "git log -3" (show
> last 3 commits) and "git rebase -3" (rebase up to HEAD~3, but there may
> be more commits in case there are merges).
> 
> I don't have a better proposal, but at least the inconsistancy should be
> documented (e.g. "note that this is different from what other commands
> like 'git log' do when used with a - option since ..." in the
> manpage).

This might be a reason that "-NUM" is a bad idea.

Or perhaps "-NUM" should fail with an error message if any of the last
NUM commits are merges.  In that restricted scenario (which probably
accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] finish_tmp_packfile():use strbuf for pathname construction

[Sun He]

The old version fixes a maximum length on the buffer, which could be a problem
if one is not certain of the length of get_object_directory().
Using strbuf can avoid the protential bug.

Helped-by: Michael Haggerty 
Helped-by: Eric Sunshine 
Signed-off-by: Sun He 
---

PATCH v3 adds the reason why we should apply this patch.
 Thanks to Micheal Haggerty.
PATCH v3 transposes the space and comma before the third argument as 
Eric Sunshine suggested to meet the style of existing code.
 Thanks to Eric Sunshine.
PATCH v3 fixes the order of Helped-by and Signed-off-by.

PATCH v2 follows the suggestions of Eric Sunshine to use strbuf_setlen() 
instead of strbuf_remove(), etc.
 Thanks to Eric Sunshine.

This patch has assumed that you have already fix the bug of
tmpname in builtin/pack-objects.c:write_pack_file() warning()


 builtin/pack-objects.c | 15 ++-
 bulk-checkin.c |  8 +---
 pack-write.c   | 18 ++
 pack.h |  2 +-
 4 files changed, 22 insertions(+), 21 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..099d6ed 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -803,7 +803,7 @@ static void write_pack_file(void)
 
if (!pack_to_stdout) {
struct stat st;
-   char tmpname[PATH_MAX];
+   struct strbuf tmpname = STRBUF_INIT;
 
/*
 * Packs are runtime accessed in their mtime
@@ -826,23 +826,19 @@ static void write_pack_file(void)
tmpname, strerror(errno));
}
 
-   /* Enough space for "-.pack"? */
-   if (sizeof(tmpname) <= strlen(base_name) + 50)
-   die("pack base name '%s' too long", base_name);
-   snprintf(tmpname, sizeof(tmpname), "%s-", base_name);
+   strbuf_addf(&tmpname, "%s-", base_name);
 
if (write_bitmap_index) {
bitmap_writer_set_checksum(sha1);
bitmap_writer_build_type_index(written_list, 
nr_written);
}
 
-   finish_tmp_packfile(tmpname, pack_tmp_name,
+   finish_tmp_packfile(&tmpname, pack_tmp_name,
written_list, nr_written,
&pack_idx_opts, sha1);
 
if (write_bitmap_index) {
-   char *end_of_name_prefix = strrchr(tmpname, 0);
-   sprintf(end_of_name_prefix, "%s.bitmap", 
sha1_to_hex(sha1));
+   strbuf_addf(&tmpname, "%s.bitmap", 
sha1_to_hex(sha1));
 
stop_progress(&progress_state);
 
@@ -851,10 +847,11 @@ static void write_pack_file(void)
bitmap_writer_select_commits(indexed_commits, 
indexed_commits_nr, -1);
bitmap_writer_build(&to_pack);
bitmap_writer_finish(written_list, nr_written,
-tmpname, 
write_bitmap_options);
+tmpname.buf, 
write_bitmap_options);
write_bitmap_index = 0;
}
 
+   strbuf_release(&tmpname);
free(pack_tmp_name);
puts(sha1_to_hex(sha1));
}
diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..98e651c 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -4,6 +4,7 @@
 #include "bulk-checkin.h"
 #include "csum-file.h"
 #include "pack.h"
+#include "strbuf.h"
 
 static int pack_compression_level = Z_DEFAULT_COMPRESSION;
 
@@ -23,7 +24,7 @@ static struct bulk_checkin_state {
 static void finish_bulk_checkin(struct bulk_checkin_state *state)
 {
unsigned char sha1[20];
-   char packname[PATH_MAX];
+   struct strbuf packname = STRBUF_INIT;
int i;
 
if (!state->f)
@@ -43,8 +44,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
close(fd);
}
 
-   sprintf(packname, "%s/pack/pack-", get_object_directory());
-   finish_tmp_packfile(packname, state->pack_tmp_name,
+   strbuf_addf(&packname, "%s/pack/pack-", get_object_directory());
+   finish_tmp_packfile(&packname, state->pack_tmp_name,
state->written, state->nr_written,
&state->pack_idx_opts, sha1);
for (i = 0; i < state->nr_written; i++)
@@ -54,6 +55,7 @@ clear_exit:
free(state->written);
memset(state, 0, sizeof(*state));
 
+   strbuf_release(&packname);
/* Make objects we just wrote available to ourselves */
reprepare_

Re: [PATCH v2] finish_tmp_packfile():use strbuf for pathname construction

[He Sun]

2014-03-03 15:41 GMT+08:00 Eric Sunshine :
> On Sat, Mar 1, 2014 at 9:29 PM, Sun He  wrote:
>> Signed-off-by: Sun He 
>> Helped-by: Eric Sunshine 
>> Helped-by: Michael Haggerty 
>> ---
>>
>> This patch has assumed that you have already fix the bug of
>> tmpname in builtin/pack-objects.c:write_pack_file() warning()
>>
>>
>>  builtin/pack-objects.c | 15 ++-
>>  bulk-checkin.c |  8 +---
>>  pack-write.c   | 18 ++
>>  pack.h |  2 +-
>>  4 files changed, 22 insertions(+), 21 deletions(-)
>>
>> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
>> index c733379..099d6ed 100644
>> --- a/builtin/pack-objects.c
>> +++ b/builtin/pack-objects.c
>> @@ -803,7 +803,7 @@ static void write_pack_file(void)
>>
>> if (!pack_to_stdout) {
>> struct stat st;
>> -   char tmpname[PATH_MAX];
>> +   struct strbuf tmpname = STRBUF_INIT;
>>
>> /*
>>  * Packs are runtime accessed in their mtime
>> @@ -826,23 +826,19 @@ static void write_pack_file(void)
>> tmpname, strerror(errno));
>> }
>>
>> -   /* Enough space for "-.pack"? */
>> -   if (sizeof(tmpname) <= strlen(base_name) + 50)
>> -   die("pack base name '%s' too long", 
>> base_name);
>> -   snprintf(tmpname, sizeof(tmpname), "%s-", base_name);
>> +   strbuf_addf(&tmpname, "%s-", base_name);
>>
>> if (write_bitmap_index) {
>> bitmap_writer_set_checksum(sha1);
>> bitmap_writer_build_type_index(written_list, 
>> nr_written);
>> }
>>
>> -   finish_tmp_packfile(tmpname, pack_tmp_name,
>> +   finish_tmp_packfile(&tmpname, pack_tmp_name,
>> written_list, nr_written,
>> &pack_idx_opts, sha1);
>>
>> if (write_bitmap_index) {
>> -   char *end_of_name_prefix = strrchr(tmpname, 
>> 0);
>> -   sprintf(end_of_name_prefix, "%s.bitmap", 
>> sha1_to_hex(sha1));
>> +   strbuf_addf(&tmpname, "%s.bitmap" 
>> ,sha1_to_hex(sha1));
>
> Transpose the space and comma before the third argument.
>
> Other than this, the patch looks reasonable.
>

OK, got it.
I have already fixed it.
Thank you very much

>> stop_progress(&progress_state);
>>
>> @@ -851,10 +847,11 @@ static void write_pack_file(void)
>> 
>> bitmap_writer_select_commits(indexed_commits, indexed_commits_nr, -1);
>> bitmap_writer_build(&to_pack);
>> bitmap_writer_finish(written_list, 
>> nr_written,
>> -tmpname, 
>> write_bitmap_options);
>> +tmpname.buf, 
>> write_bitmap_options);
>> write_bitmap_index = 0;
>> }
>>
>> +   strbuf_release(&tmpname);
>> free(pack_tmp_name);
>> puts(sha1_to_hex(sha1));
>> }
>> diff --git a/bulk-checkin.c b/bulk-checkin.c
>> index 118c625..98e651c 100644
>> --- a/bulk-checkin.c
>> +++ b/bulk-checkin.c
>> @@ -4,6 +4,7 @@
>>  #include "bulk-checkin.h"
>>  #include "csum-file.h"
>>  #include "pack.h"
>> +#include "strbuf.h"
>>
>>  static int pack_compression_level = Z_DEFAULT_COMPRESSION;
>>
>> @@ -23,7 +24,7 @@ static struct bulk_checkin_state {
>>  static void finish_bulk_checkin(struct bulk_checkin_state *state)
>>  {
>> unsigned char sha1[20];
>> -   char packname[PATH_MAX];
>> +   struct strbuf packname = STRBUF_INIT;
>> int i;
>>
>> if (!state->f)
>> @@ -43,8 +44,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
>> *state)
>> close(fd);
>> }
>>
>> -   sprintf(packname, "%s/pack/pack-", get_object_directory());
>> -   finish_tmp_packfile(packname, state->pack_tmp_name,
>> +   strbuf_addf(&packname, "%s/pack/pack-", get_object_directory());
>> +   finish_tmp_packfile(&packname, state->pack_tmp_name,
>> state->written, state->nr_written,
>> &state->pack_idx_opts, sha1);
>> for (i = 0; i < state->nr_written; i++)
>> @@ -54,6 +55,7 @@ clear_exit:
>> free(state->written);
>> memset(state, 0, sizeof(*state));
>>
>> +   strbuf_release(&packname);
>> /* Make objects we just wrote available to ourselves */
>> reprepare_packed_git();
>>  }
>> diff --git a/pa

Re: [PATCH 2/3] rebase: accept - as another way of saying HEAD~

[Matthieu Moy]

Michael Haggerty  writes:

> Or perhaps "-NUM" should fail with an error message if any of the last
> NUM commits are merges.  In that restricted scenario (which probably
> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".

Makes sense to me. So, -NUM would actually mean "rebase the last NUM
commits" (as well as being an alias for HEAD~NUM), but would fail when
it does not make sense (with an error message explaining the situation
and pointing the user to HEAD~N if this is what he wanted).

This would actually be a feature for me: I often want to rebase "recent
enough" history, and when my @{upstream} isn't well positionned, I
randomly type HEAD~N without remembering what N should be. When N is too
small, the rebase doesn't reach the interesting commit, and when N is
too big, it reaches a merge commit and I get a bunch of commits I'm not
allowed to edit in my todo-list. Then I have to abort the commit
manually. With -N failing on merge commits, the rebase would abort
itself automatically.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v3] Replace memcpy with hashcpy when dealing hash copy globally

[Sun He]

Replacing memcpy with hashcpy is more directly and elegant.

Leave ppc/sha1.c alone, as it is an isolated component.
Pull cache.h(actually ../cache.h) in just for one memcpy
there is not proper.

Helped-by: Michael Haggerty 
Helped-by: Duy Nguyen 
Signed-off-by: Sun He 
---

PATCH v3 delete the one-space indentation on each line of commit message
as is suggested by Eric Sunshine.
 Thanks to Eric Sunshine.

PATCH v2 leave ppc/sha1.c alone.

The general rule is if cache.h or git-compat-util.h is included,
it is the first #include, and system includes will be always in
git-compat-tuil.h.
via Duy Nguyen

The change in PATCH v1 is not proper because I placed cache.h
in the end.
And adding it to the head is not a good way to achieve the goal,
as is said above "---".
 Thanks to Duy Nguyen.

Find the potential places with memcpy by the bash command:
 $ find . | xargs grep "memcpy.*\(.*20.*\)"

ppc/sha1.c doesn't include cache.h and it cannot use hashcpy().
So just leave memcpy(in ppc/sha1.c) alone.

 bundle.c| 2 +-
 grep.c  | 2 +-
 pack-bitmap-write.c | 2 +-
 reflog-walk.c   | 4 ++--
 refs.c  | 2 +-
 5 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/bundle.c b/bundle.c
index e99065c..7809fbb 100644
--- a/bundle.c
+++ b/bundle.c
@@ -19,7 +19,7 @@ static void add_to_ref_list(const unsigned char *sha1, const 
char *name,
list->list = xrealloc(list->list,
list->alloc * sizeof(list->list[0]));
}
-   memcpy(list->list[list->nr].sha1, sha1, 20);
+   hashcpy(list->list[list->nr].sha1, sha1);
list->list[list->nr].name = xstrdup(name);
list->nr++;
 }
diff --git a/grep.c b/grep.c
index c668034..f5101f7 100644
--- a/grep.c
+++ b/grep.c
@@ -1650,7 +1650,7 @@ void grep_source_init(struct grep_source *gs, enum 
grep_source_type type,
break;
case GREP_SOURCE_SHA1:
gs->identifier = xmalloc(20);
-   memcpy(gs->identifier, identifier, 20);
+   hashcpy(gs->identifier, identifier);
break;
case GREP_SOURCE_BUF:
gs->identifier = NULL;
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 1218bef..5f1791a 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -530,7 +530,7 @@ void bitmap_writer_finish(struct pack_idx_entry **index,
header.version = htons(default_version);
header.options = htons(flags | options);
header.entry_count = htonl(writer.selected_nr);
-   memcpy(header.checksum, writer.pack_checksum, 20);
+   hashcpy(header.checksum, writer.pack_checksum);
 
sha1write(f, &header, sizeof(header));
dump_bitmap(f, writer.commits);
diff --git a/reflog-walk.c b/reflog-walk.c
index b2fbdb2..d490f7d 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -32,8 +32,8 @@ static int read_one_reflog(unsigned char *osha1, unsigned 
char *nsha1,
sizeof(struct reflog_info));
}
item = array->items + array->nr;
-   memcpy(item->osha1, osha1, 20);
-   memcpy(item->nsha1, nsha1, 20);
+   hashcpy(item->osha1, osha1);
+   hashcpy(item->nsha1, nsha1);
item->email = xstrdup(email);
item->timestamp = timestamp;
item->tz = tz;
diff --git a/refs.c b/refs.c
index 89228e2..f90b7ea 100644
--- a/refs.c
+++ b/refs.c
@@ -1222,7 +1222,7 @@ static int resolve_gitlink_packed_ref(struct ref_cache 
*refs,
if (ref == NULL)
return -1;
 
-   memcpy(sha1, ref->u.value.sha1, 20);
+   hashcpy(sha1, ref->u.value.sha1);
return 0;
 }
 
-- 
1.9.0.138.g2de3478.dirty

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

Re: Branch Name Case Sensitivity

[Karsten Blees]

Am 01.03.2014 07:54, schrieb Torsten Bögershausen:
> On 2014-03-01 03.42, Lee Hopkins wrote:
>> +
>> +if(ignore_case)
> Only looking at ignore_case here closes the door for people
> who have a branch "foo" and "Foo" at the same time.
> (Which means that they are carefully running git pack-refs)
> How about something like this:
>  +if (refs_ignore_case < 0)
>  +  refs_ignore_case = ignore_case;
>  +if (refs_ignore_case)

I don't think this distinction is necessary, either you have a case-insensitive 
file system or you don't. The case that the .git directory is case-sensitive 
and the worktree directory isn't (or the other way around) is probably so 
exotic that we can ignore it.

> (And then we need the diff further down on top of this.)
> (And of course Documentation/config.txt)
> The main motivation is that you can set refs.ignorecase == true on
> e.g. Linux, to prevent to have branches "Foo" and "foo" at the same time,
> which gives problems when pulling into e.g. Windows/Mac OS

If you want to prevent problems with Windows/Mac OS, you should set 
core.ignorecase = true. I don't see why we need yet another config setting for 
refs (and logs?).

--
To unsubscribe from this list: send the line "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] rebase: accept - as another way of saying HEAD~

[Duy Nguyen]

On Mon, Mar 3, 2014 at 4:37 PM, Matthieu Moy
 wrote:
> Michael Haggerty  writes:
>
>> Or perhaps "-NUM" should fail with an error message if any of the last
>> NUM commits are merges.  In that restricted scenario (which probably
>> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".
>
> Makes sense to me. So, -NUM would actually mean "rebase the last NUM
> commits" (as well as being an alias for HEAD~NUM), but would fail when
> it does not make sense (with an error message explaining the situation
> and pointing the user to HEAD~N if this is what he wanted).

Agreed, but..

> This would actually be a feature for me: I often want to rebase "recent
> enough" history, and when my @{upstream} isn't well positionned, I
> randomly type HEAD~N without remembering what N should be. When N is too
> small, the rebase doesn't reach the interesting commit, and when N is
> too big, it reaches a merge commit and I get a bunch of commits I'm not
> allowed to edit in my todo-list. Then I have to abort the commit
> manually. With -N failing on merge commits, the rebase would abort
> itself automatically.

would "git rebase -i --fork-point" be what you need instead? It's a
new thing, but may be what we actually should use, not this -NUM..
-- 
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 3/3] rebase: new convenient option to edit a single commit

[Michael Haggerty]

On 03/02/2014 10:09 AM, Eric Sunshine wrote:
> On Sun, Mar 2, 2014 at 4:04 AM, Eric Sunshine  wrote:
>> On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy  
>> wrote:
>>> "git rebase -e XYZ" is basically the same as
>>>
>>> EDITOR="sed -i '1s/pick XYZ/edit XYZ/' $@" \
>>> git rebase -i XYZ^
>>>
>>> In English, it prepares the todo list for you to edit only commit XYZ
>>> to save your time. The time saving is only significant when you edit a
>>> lot of commits separately.
>>
>> Should this accept multiple -e arguments? Based upon the above
>> justification, it sounds like it should, and I think that would be the
>> intuitive expectation (at least for me).
>>
>> The current implementation, however, is broken with multiple -e arguments. 
>> With:
>>
>> git rebase -i -e older -e newer
>>
>> 'newer' is ignored entirely. However, with:
>>
>> git rebase -i -e newer -e older
>>
>> it errors out when rewriting the todo list:
>>
>> "expected to find 'edit older' line but did not"
>>
>> An implementation supporting multiple -e arguments would need to
>> ensure that the todo list extends to the "oldest" rev specified by any
>> -e argument.
> 
> Of course, I'm misreading and abusing the intention of -e as if it is
> "-e ".

I think that your misreading is more consistent than the feature as
implemented.

git rebase -e OLDER

does not mean "do 'git rebase -i OLDER' and oh, by the way, also set up
commit OLDER to be edited".  It means "do 'git rebase -i OLDER^' ..."
(note: "OLDER^" and not "OLDER").  So it is confusing to think as "-e"
as a modifier on an otherwise normal "git rebase -i" invocation.
Rather, it seems to me that "-e" and "-i" should be mutually exclusive
(and consider it an implementation detail that the former is implemented
using the latter).

And if that is our point of view, then is perfectly logical to allow it
to be specified multiple times.  OTOH there is no reason that v1 has to
allow multiple "-e", as long as it properly rejects that usage.

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] rebase: accept - as another way of saying HEAD~

[Matthieu Moy]

Duy Nguyen  writes:

> On Mon, Mar 3, 2014 at 4:37 PM, Matthieu Moy
>
>> This would actually be a feature for me: I often want to rebase "recent
>> enough" history, and when my @{upstream} isn't well positionned, I
>> randomly type HEAD~N without remembering what N should be. When N is too
>> small, the rebase doesn't reach the interesting commit, and when N is
>> too big, it reaches a merge commit and I get a bunch of commits I'm not
>> allowed to edit in my todo-list. Then I have to abort the commit
>> manually. With -N failing on merge commits, the rebase would abort
>> itself automatically.
>
> would "git rebase -i --fork-point" be what you need instead?

I don't think so. My use case is when I did a manual merge, so
@{upstream} is helpless or even not positionned. There is for sure an
accurate command (remember the branch I just merged and put it on the
command-line), but my fingers prefer typing quick and dirty commands and
hope I won't get too many trial and error cycles ;-).

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/3] rebase: accept - as another way of saying HEAD~

[Jeff King]

On Mon, Mar 03, 2014 at 10:37:11AM +0100, Matthieu Moy wrote:

> Michael Haggerty  writes:
> 
> > Or perhaps "-NUM" should fail with an error message if any of the last
> > NUM commits are merges.  In that restricted scenario (which probably
> > accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".
> 
> Makes sense to me. So, -NUM would actually mean "rebase the last NUM
> commits" (as well as being an alias for HEAD~NUM), but would fail when
> it does not make sense (with an error message explaining the situation
> and pointing the user to HEAD~N if this is what he wanted).

Yeah, I agree with this. I think "-NUM" is only useful for linear
string-of-pearls history. With merges, it either means:

  - linearize history (a la git-log), go back NUM commits, and treat the
result as the upstream. This is useless, because it's difficult to
predict where you're going to end up. Using explicit "~" and "^"
relative-commit operators to specify the upstream is more flexible
if you really want to do this (but I don't know why you would).

  - do not use an UNINTERESTING upstream as the cutoff, but instead
try to rebase exactly NUM commits. In the face of non-linear
history, I'm not even sure what this would mean, or how we would
implement it.

Neither of those is useful, so I don't think erroring out on them is a
bad thing. And by erroring out (rather than documenting some weird and
useless behavior), we don't have to worry about backwards compatibility
if we later _do_ come up with a useful behavior (the best I can think of
is that "--first-parent -3" might have a use, but I think that should
already be covered, as the results of --first-parent are by-definition
linear).

> This would actually be a feature for me: I often want to rebase "recent
> enough" history, and when my @{upstream} isn't well positionned, I
> randomly type HEAD~N without remembering what N should be. When N is too
> small, the rebase doesn't reach the interesting commit, and when N is
> too big, it reaches a merge commit and I get a bunch of commits I'm not
> allowed to edit in my todo-list. Then I have to abort the commit
> manually. With -N failing on merge commits, the rebase would abort
> itself automatically.

I do the same thing.

-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 3/3] rebase: new convenient option to edit a single commit

[Duy Nguyen]

On Mon, Mar 3, 2014 at 5:10 PM, Michael Haggerty  wrote:
> On 03/02/2014 10:09 AM, Eric Sunshine wrote:
>> On Sun, Mar 2, 2014 at 4:04 AM, Eric Sunshine  
>> wrote:
>>> On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy  
>>> wrote:
 "git rebase -e XYZ" is basically the same as

 EDITOR="sed -i '1s/pick XYZ/edit XYZ/' $@" \
 git rebase -i XYZ^

 In English, it prepares the todo list for you to edit only commit XYZ
 to save your time. The time saving is only significant when you edit a
 lot of commits separately.
>>>
>>> Should this accept multiple -e arguments? Based upon the above
>>> justification, it sounds like it should, and I think that would be the
>>> intuitive expectation (at least for me).
>>>
>>> The current implementation, however, is broken with multiple -e arguments. 
>>> With:
>>>
>>> git rebase -i -e older -e newer
>>>
>>> 'newer' is ignored entirely. However, with:
>>>
>>> git rebase -i -e newer -e older
>>>
>>> it errors out when rewriting the todo list:
>>>
>>> "expected to find 'edit older' line but did not"
>>>
>>> An implementation supporting multiple -e arguments would need to
>>> ensure that the todo list extends to the "oldest" rev specified by any
>>> -e argument.
>>
>> Of course, I'm misreading and abusing the intention of -e as if it is
>> "-e ".
>
> I think that your misreading is more consistent than the feature as
> implemented.
>
> git rebase -e OLDER
>
> does not mean "do 'git rebase -i OLDER' and oh, by the way, also set up
> commit OLDER to be edited".  It means "do 'git rebase -i OLDER^' ..."
> (note: "OLDER^" and not "OLDER").  So it is confusing to think as "-e"
> as a modifier on an otherwise normal "git rebase -i" invocation.
> Rather, it seems to me that "-e" and "-i" should be mutually exclusive
> (and consider it an implementation detail that the former is implemented
> using the latter).
>
> And if that is our point of view, then is perfectly logical to allow it
> to be specified multiple times.

Logically, yes. Practically, no. If you have to put multiple -e and
some hashes in one line, wouldn't editing to-do list in your favorite
editor be faster?

> OTOH there is no reason that v1 has to
> allow multiple "-e", as long as it properly rejects that usage.
-- 
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

[PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Ilya Bobyr]

We used to show "(missing )" next to tests skipped because they are
specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" instead.
---
 t/test-lib.sh |   13 -
 1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..89a405b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,25 +446,28 @@ test_finish_ () {
 
 test_skip () {
to_skip=
+   skipped_reason=
if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
then
to_skip=t
+   skipped_reason="matched GIT_SKIP_TESTS"
fi
if test -z "$to_skip" && test -n "$test_prereq" &&
   ! test_have_prereq "$test_prereq"
then
to_skip=t
-   fi
-   case "$to_skip" in
-   t)
+
of_prereq=
if test "$missing_prereq" != "$test_prereq"
then
of_prereq=" of $test_prereq"
fi
-
+   skipped_reason="missing $missing_prereq${of_prereq}"
+   fi
+   case "$to_skip" in
+   t)
say_color skip >&3 "skipping test: $@"
-   say_color skip "ok $test_count # skip $1 (missing 
$missing_prereq${of_prereq})"
+   say_color skip "ok $test_count # skip $1 ($skipped_reason)"
: true
;;
*)
-- 
1.7.9

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

[PATCH 2/2] test-lib: GIT_TEST_ONLY to run only specific tests

[Ilya Bobyr]

This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when debugging.
---
 t/README  |   15 +++
 t/test-lib.sh |8 
 2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index caeeb9d..f939987 100644
--- a/t/README
+++ b/t/README
@@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip the whole
 test, or t[0-9]{4} followed by ".$number" to say which
 particular test to skip.
 
+Sometimes the opposite is desired - ability to execute only one or
+several tests.  Mostly while debugging tests.  For that you can say
+
+$ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
+
+or, similrary to GIT_SKIP_TESTS
+
+$ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
+
+In additiona to matching against "."
+GIT_TEST_ONLY is matched against just the test numbes.  This comes
+handy when you are running only one test:
+
+$ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
+
 Note that some tests in the existing test suite rely on previous
 test item, so you cannot arbitrarily disable one and expect the
 remainder of test to check what the test originally was intended
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 89a405b..12bf436 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -464,6 +464,14 @@ test_skip () {
fi
skipped_reason="missing $missing_prereq${of_prereq}"
fi
+   if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
+   ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
+   ! match_pattern_list $test_count $GIT_TEST_ONLY
+   then
+   to_skip=t
+   skipped_reason="not in GIT_TEST_ONLY"
+   fi
+
case "$to_skip" in
t)
say_color skip >&3 "skipping test: $@"
-- 
1.7.9

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

[PATCH] rev-parse --parseopt: option argument name hints

[Ilya Bobyr]

Built-in commands can specify names for option arguments, that are shown
when usage text is generated for the command.  sh based commands should
be able to do the same.

Option argument name hint is any text that comes after [*=?!] after the
argument name up to the first whitespace.  Underscores are replaced with
whitespace.  It is unlikely that an underscore would be useful in the
hint text.

Signed-off-by: Ilya Bobyr 
---
 Documentation/git-rev-parse.txt |   11 +--
 builtin/rev-parse.c |   17 -
 t/t1502-rev-parse-parseopt.sh   |   20 
 3 files changed, 45 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-rev-parse.txt b/Documentation/git-rev-parse.txt
index 0d2cdcd..4cb6e02 100644
--- a/Documentation/git-rev-parse.txt
+++ b/Documentation/git-rev-parse.txt
@@ -284,13 +284,13 @@ Input Format
 
 'git rev-parse --parseopt' input format is fully text based. It has two parts,
 separated by a line that contains only `--`. The lines before the separator
-(should be more than one) are used for the usage.
+(could be more than one) are used for the usage.
 The lines after the separator describe the options.
 
 Each line of options has this format:
 
 
-* SP+ help LF
+*? SP+ help LF
 
 
 ``::
@@ -313,6 +313,12 @@ Each line of options has this format:
 
* Use `!` to not make the corresponding negated long option available.
 
+``::
+   ``, if specified, is used as a name of the argument, if the
+   option takes an argument. `` is terminated by the first
+   whitespace. Angle braces are added automatically.  Underscore symbols
+   are replaced with spaces.
+
 The remainder of the line, after stripping the spaces, is used
 as the help associated to the option.
 
@@ -333,6 +339,7 @@ h,helpshow the help
 
 foo   some nifty option --foo
 bar=  some cool option --bar with an argument
+baz=arg   another cool option --baz with an argument named 
 
   An option group Header
 C?option C with an optional argument"
diff --git a/builtin/rev-parse.c b/builtin/rev-parse.c
index aaeb611..83a769e 100644
--- a/builtin/rev-parse.c
+++ b/builtin/rev-parse.c
@@ -395,9 +395,10 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
usage[unb++] = strbuf_detach(&sb, NULL);
}
 
-   /* parse: (|,|)[=?]? SP+  */
+   /* parse: (|,|)[*=?!]*? SP+  */
while (strbuf_getline(&sb, stdin, '\n') != EOF) {
const char *s;
+   const char *argh;
struct option *o;
 
if (!sb.len)
@@ -419,6 +420,20 @@ static int cmd_parseopt(int argc, const char **argv, const 
char *prefix)
o->value = &parsed;
o->flags = PARSE_OPT_NOARG;
o->callback = &parseopt_dump;
+
+   /* Possible argument name hint */
+   argh = s;
+   while (s > sb.buf && strchr("*=?!", s[-1]) == NULL)
+   --s;
+   if (s != sb.buf && s != argh) {
+   char *a;
+   o->argh = a = xmemdupz(s, argh - s);
+   while (a = strchr(a, '_'))
+   *a = ' ';
+   }
+   if (s == sb.buf)
+   s = argh;
+
while (s > sb.buf && strchr("*=?!", s[-1])) {
switch (*--s) {
case '=':
diff --git a/t/t1502-rev-parse-parseopt.sh b/t/t1502-rev-parse-parseopt.sh
index 83b1300..bf0db05 100755
--- a/t/t1502-rev-parse-parseopt.sh
+++ b/t/t1502-rev-parse-parseopt.sh
@@ -18,6 +18,17 @@ An option group Header
 -C[...]   option C with an optional argument
 -d, --data[=...]  short and long option with an optional argument
 
+Argument hints
+-b   short option required argument
+--bar2   long option required argument
+-e, --fuz 
+  short and long option required argument
+-s[]short option optional argument
+--long[=]   long option optional argument
+-g, --fluf[=]   short and long option optional argument
+--longest 
+  a very long argument hint
+
 Extras
 --extra1  line above used to cause a segfault but no longer 
does
 
@@ -39,6 +50,15 @@ b,baz a short and long option
 C?option C with an optional argument
 d,data?   short and long option with an optional argument
 
+ Argument hints
+b=arg short option required argument
+bar2=arg  long option required argument
+e,fuz=with_spaces  short and long option required argument
+s?someshort option optional argument
+long?data long option optional argument
+g,fluf?path short and long option optional argument
+longest=a_very_long_argument_hint  a very long argument hint
+
 Extras
 extra1line above used to cause a segfault but no longer does
 EOF
-- 
1.7.9

--
To unsubscr

Re: [PATCH 2/3] rebase: accept - as another way of saying HEAD~

[David Kastrup]

Duy Nguyen  writes:

> On Mon, Mar 3, 2014 at 4:37 PM, Matthieu Moy
>  wrote:
>> Michael Haggerty  writes:
>>
>>> Or perhaps "-NUM" should fail with an error message if any of the last
>>> NUM commits are merges.  In that restricted scenario (which probably
>>> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".
>>
>> Makes sense to me. So, -NUM would actually mean "rebase the last NUM
>> commits" (as well as being an alias for HEAD~NUM), but would fail when
>> it does not make sense (with an error message explaining the situation
>> and pointing the user to HEAD~N if this is what he wanted).
>
> Agreed, but..
>
>> This would actually be a feature for me: I often want to rebase "recent
>> enough" history, and when my @{upstream} isn't well positionned, I
>> randomly type HEAD~N without remembering what N should be. When N is too
>> small, the rebase doesn't reach the interesting commit, and when N is
>> too big, it reaches a merge commit and I get a bunch of commits I'm not
>> allowed to edit in my todo-list. Then I have to abort the commit
>> manually. With -N failing on merge commits, the rebase would abort
>> itself automatically.
>
> would "git rebase -i --fork-point" be what you need instead? It's a
> new thing, but may be what we actually should use, not this -NUM..

-0 might be a good mnemonic for --fork-point, though.

Of course, when using --preserve-merges explicitly it would appear that
-NUM should not error out.

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

Re: [PATCH 3/3] rebase: new convenient option to edit a single commit

[David Kastrup]

Duy Nguyen  writes:

> Logically, yes. Practically, no. If you have to put multiple -e and
> some hashes in one line, wouldn't editing to-do list in your favorite
> editor be faster?

An editor is the last resort when the card puncher is broken.

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

My advice for GSoC applicants

[Michael Haggerty]

Hi,

Based on my experience so far as a first-time Google Summer of Code
mentor, I just wrote a blog article containing some hopefully useful
advice for students applying to the program.  Please note that this is
my personal opinion only and doesn't necessarily reflect the views of
the Git/libgit2 projects as a whole.

My secret tip for GSoC success

http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

submodules: reuse .git/modules/... for multiple checkouts of same URL

[Stefan.Liebl]

Hi,

I have a git superproject with 3 submodules. The submodules are cloned from the 
same URL but use different branches. Git clones the repo three times and I have 
three entries in .git/modules. Is it possible to reuse the first clone for the 
next submodule clones?

Stefan
--
To unsubscribe from this list: send the line "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: submodules: reuse .git/modules/... for multiple checkouts of same URL

[Andrew Keller]

On Mar 3, 2014, at 7:24 AM,  wrote:

> I have a git superproject with 3 submodules. The submodules are cloned from 
> the same URL but use different branches. Git clones the repo three times and 
> I have three entries in .git/modules. Is it possible to reuse the first clone 
> for the next submodule clones?

Sort of - but I don't think you'll like the side effects.

If each of the submodules are checked out on a different branch, then HEAD is 
going to be different in each repository in .git/modules.  So, each of the 
repositories in .git/modules are unique.

You could share the objects database (see --shared or --reference) which would 
dramatically reduce the size of each repo on its own, but that comes with a 
side effect: because there is no communication between any of the repositories 
involved in sharing objects, git-gc will happily delete an object that may be 
unneeded locally, but another repository will suddenly think it's corrupt.

You could also customize the refspecs in each repository.  For example, if one 
of the submodules has only 'origin/contrib' checked out, then you could change 
the refspec to '+refs/heads/contrib:refs/remotes/origin/contrib', which will 
cause only the objects pertaining to that branch will be downloaded.  This has 
the most benefit when the commit graph is orphaned in some way.  However, this 
approach requires manual labor every time you initialize a submodule.

 - Andrew Keller

--
To unsubscribe from this list: send the line "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] Use ALLOC_GROW() instead of inline code

[He Sun]

2014-02-28 4:45 GMT+08:00 Dmitry S. Dolzhenko :
> Signed-off-by: Dmitry S. Dolzhenko 
> ---
>  attr.c |  7 +--
>  builtin/pack-objects.c |  7 +--
>  bundle.c   |  6 +-
>  cache-tree.c   |  6 +-
>  commit.c   |  8 ++--
>  diff.c | 12 ++--
>  diffcore-rename.c  | 12 ++--
>  dir.c  |  5 +
>  patch-ids.c|  5 +
>  read-cache.c   |  9 ++---
>  reflog-walk.c  | 13 +++--
>  replace_object.c   |  8 ++--
>  12 files changed, 19 insertions(+), 79 deletions(-)
>

I find another two files can be fixed this way.
- builtin/mktree.c
- sha1_file.c

I find it by searching the key word "alloc_nr" throughout the source code.
May be there are other places of this kind of code and you can find it
by other ways.

> diff --git a/attr.c b/attr.c
> index 8d13d70..734222d 100644
> --- a/attr.c
> +++ b/attr.c
> @@ -338,12 +338,7 @@ static void handle_attr_line(struct attr_stack *res,
> a = parse_attr_line(line, src, lineno, macro_ok);
> if (!a)
> return;
> -   if (res->alloc <= res->num_matches) {
> -   res->alloc = alloc_nr(res->num_matches);
> -   res->attrs = xrealloc(res->attrs,
> - sizeof(struct match_attr *) *
> - res->alloc);
> -   }
> +   ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);
> res->attrs[res->num_matches++] = a;
>  }
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 541667f..92cbce8 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -1156,12 +1156,7 @@ static int check_pbase_path(unsigned hash)
> if (0 <= pos)
> return 1;
> pos = -pos - 1;
> -   if (done_pbase_paths_alloc <= done_pbase_paths_num) {
> -   done_pbase_paths_alloc = alloc_nr(done_pbase_paths_alloc);
> -   done_pbase_paths = xrealloc(done_pbase_paths,
> -   done_pbase_paths_alloc *
> -   sizeof(unsigned));
> -   }
> +   ALLOC_GROW(done_pbase_paths, done_pbase_paths_num + 1, 
> done_pbase_paths_alloc);
> done_pbase_paths_num++;
> if (pos < done_pbase_paths_num)
> memmove(done_pbase_paths + pos + 1,
> diff --git a/bundle.c b/bundle.c
> index e99065c..1388a3e 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -14,11 +14,7 @@ static const char bundle_signature[] = "# v2 git bundle\n";
>  static void add_to_ref_list(const unsigned char *sha1, const char *name,
> struct ref_list *list)
>  {
> -   if (list->nr + 1 >= list->alloc) {
> -   list->alloc = alloc_nr(list->nr + 1);
> -   list->list = xrealloc(list->list,
> -   list->alloc * sizeof(list->list[0]));
> -   }
> +   ALLOC_GROW(list->list, list->nr + 1, list->alloc);
> memcpy(list->list[list->nr].sha1, sha1, 20);
> list->list[list->nr].name = xstrdup(name);
> list->nr++;
> diff --git a/cache-tree.c b/cache-tree.c
> index 0bbec43..30149d1 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -75,11 +75,7 @@ static struct cache_tree_sub *find_subtree(struct 
> cache_tree *it,
> return NULL;
>
> pos = -pos-1;
> -   if (it->subtree_alloc <= it->subtree_nr) {
> -   it->subtree_alloc = alloc_nr(it->subtree_alloc);
> -   it->down = xrealloc(it->down, it->subtree_alloc *
> -   sizeof(*it->down));
> -   }
> +   ALLOC_GROW(it->down, it->subtree_nr + 1, it->subtree_alloc);
> it->subtree_nr++;
>
> down = xmalloc(sizeof(*down) + pathlen + 1);
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..e004314 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -147,12 +147,8 @@ int register_commit_graft(struct commit_graft *graft, 
> int ignore_dups)
> return 1;
> }
> pos = -pos - 1;
> -   if (commit_graft_alloc <= ++commit_graft_nr) {
> -   commit_graft_alloc = alloc_nr(commit_graft_alloc);
> -   commit_graft = xrealloc(commit_graft,
> -   sizeof(*commit_graft) *
> -   commit_graft_alloc);
> -   }
> +   ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
> +   commit_graft_nr++;
> if (pos < commit_graft_nr)
> memmove(commit_graft + pos + 1,
> commit_graft + pos,
> diff --git a/diff.c b/diff.c
> index 8e4a6a9..f5f0fd1 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -1361,11 +1361,7 @@ static struct diffstat_file *diffstat_add(struct 
> diffstat_t *diffstat,
>  {
> struct diffstat_file *x;
> x = xcalloc(sizeof (*x), 1);
> -   if (diffstat->nr == diffstat->alloc) {

Re: Branch Name Case Sensitivity

[Lee Hopkins]

> I don't think this distinction is necessary, either you have a 
> case-insensitive file system or you don't. The case
> that the .git directory is case-sensitive and the worktree directory isn't 
> (or the other way around) is
> probably so exotic that we can ignore it.

I think Torsten's use case was for someone who is carefully curating
their loose and packed-refs, e.g. gc.packrefs = false. This could be
for backwards compatibility (existing ambiguous refs whose names
cannot be changed for some reason) or simply because they want to.

> If you want to prevent problems with Windows/Mac OS, you should set 
> core.ignorecase = true. I don't see why we need
> yet another config setting for refs (and logs?).

Since refs.ignorecase falls back to core.ignorecase, you could just
set core.ignorecase = true and feel safe when sharing with Windows/Mac
OS. I think having the distinction just makes Git more flexible, OTOH
I can see how having both refs.ignorecase and core.ignorecase could be
confusing and possibly redundant.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] contrib/subtree - unset prefix before proceeding

[Gilles Filippini]

Hi,

Any chance to have this patch considered?
Thanks in advance,

_g.

Gilles Filippini a écrit , Le 01/03/2014 17:33:
> This is to prevent unwanted prefix when such an environment variable
> exists. The case occurs for example during the Debian package build
> where the git-subtree test suite is called with 'prefix=/usr', which
> makes test 21 fail:
> not ok 21 - Check that prefix argument is required for split
> 
> Signed-off-by: Gilles Filippini 
> ---
>  contrib/subtree/git-subtree.sh | 1 +
>  1 file changed, 1 insertion(+)
> 
> diff --git a/contrib/subtree/git-subtree.sh b/contrib/subtree/git-subtree.sh
> index dc59a91..db925ca 100755
> --- a/contrib/subtree/git-subtree.sh
> +++ b/contrib/subtree/git-subtree.sh
> @@ -46,6 +46,7 @@ ignore_joins=
>  annotate=
>  squash=
>  message=
> +prefix=
>  
>  debug()
>  {
> 

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

[PATCH v2 2/2] i18n: assure command not corrupted by _() process

[Sandy Carter]

Separate message from command examples to avoid translation issues
such as a dash being omitted in a translation.

Signed-off-by: Sandy Carter 
---
 builtin/branch.c | 10 ++
 1 file changed, 6 insertions(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index b4d7716..b304da8 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -1022,11 +1022,13 @@ int cmd_branch(int argc, const char **argv, const char 
*prefix)
 */
if (argc == 1 && track == BRANCH_TRACK_OVERRIDE &&
!branch_existed && remote_tracking) {
-   fprintf(stderr, _("\nIf you wanted to make '%s' track 
'%s', do this:\n\n"), head, branch->name);
-   fprintf(stderr, _("git branch -d %s\n"), 
branch->name);
-   fprintf(stderr, _("git branch --set-upstream-to 
%s\n"), branch->name);
+   fprintf(stderr, "\n");
+   fprintf(stderr, _("If you wanted to make '%s' track 
'%s', do this:"), head, branch->name);
+   fprintf(stderr, "\n\n");
+   fprintf(stderr, "git branch -d %s\n", branch->name);
+   fprintf(stderr, "git branch --set-upstream-to 
%s\n", branch->name);
+   fprintf(stderr, "\n");
}
-
} else
usage_with_options(builtin_branch_usage, options);
 
-- 
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

[PATCH] submodule : Add --no-separate-git-dir option to add and update command.

[Henri GEIST]

This new option prevent git submodule  to clone the missing
submodules with the --separate-git-dir option.
Then the submodule will be regular repository and their gitdir will not
be placed in the superproject gitdir/modules directory.

Signed-off-by: Henri GEIST 
---
 Documentation/git-submodule.txt |   18 --
 git-submodule.sh|   22 --
 t/t7400-submodule-basic.sh  |   12 
 3 files changed, 48 insertions(+), 4 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 21cb59a..303a475 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -10,13 +10,14 @@ SYNOPSIS
 
 [verse]
 'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
- [--reference ] [--depth ] [--]  
[]
+ [--reference ] [--depth ] 
[--no-separate-git-dir]
+ [--]  []
 'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] [--] ...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
  [-f|--force] [--rebase|--merge|--checkout] [--reference 
]
- [--depth ] [--recursive] [--] [...]
+ [--depth ] [--recursive] [--no-separate-git-dir] [--] 
[...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -107,6 +108,10 @@ is the superproject and submodule repositories will be kept
 together in the same relative location, and only the
 superproject's URL needs to be provided: git-submodule will correctly
 locate the submodule using the relative URL in .gitmodules.
++
+If `--no-separate-git-dir` is specified, missing submodules will be cloned
+has normal git repository without the option `--separate-git-dir` pointing
+to the modules directory of the superproject gitdir.
 
 status::
Show the status of the submodules. This will print the SHA-1 of the
@@ -185,6 +190,10 @@ If the submodule is not yet initialized, and you just want 
to use the
 setting as stored in .gitmodules, you can automatically initialize the
 submodule with the `--init` option.
 +
+If `--no-separate-git-dir` is specified, missing submodules will be cloned
+has normal git repository without the option `--separate-git-dir` pointing
+to the modules directory of the superproject gitdir.
++
 If `--recursive` is specified, this command will recurse into the
 registered submodules, and update any nested submodules within.
 +
@@ -363,6 +372,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
options carefully.
clone with a history truncated to the specified number of revisions.
See linkgit:git-clone[1]
 
+--no-separate-git-dir::
+   This option is valid for add and update commands. Specify that missing
+   submodules should be clonned as self contain repository without a
+   separate gitdir placed in the modules directory of the superproject
+   gitdir.
 
 ...::
Paths to submodule(s). When specified this will restrict the command
diff --git a/git-submodule.sh b/git-submodule.sh
index a33f68d..36eaf31 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -5,11 +5,11 @@
 # Copyright (c) 2007 Lars Hjemli
 
 dashless=$(basename "$0" | sed -e 's/-/ /')
-USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
] [--]  []
+USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
] [--no-separate-git-dir] [--]  []
or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
or: $dashless [--quiet] init [--] [...]
or: $dashless [--quiet] deinit [-f|--force] [--] ...
-   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--rebase] [--reference ] [--merge] [--recursive] [--] 
[...]
+   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
[-f|--force] [--rebase] [--reference ] [--merge] [--recursive] 
[--no-separate-git-dir] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
@@ -36,6 +36,7 @@ update=
 prefix=
 custom_name=
 depth=
+noseparategitdir=
 
 # The function takes at most 2 arguments. The first argument is the
 # URL that navigates to the submodule origin repo. When relative, this URL
@@ -270,6 +271,17 @@ module_clone()
quiet=-q
fi
 
+
+   if test -n "$noseparategitdir"
+   then
+   (
+   clear_local_git_env
+   git clone $quiet ${depth:+"$depth"} -n 
${reference:+"$reference"} "$url" "$sm_path"
+   ) ||
+   die "$(eval_gettext "Clone of '\$url' into submodule path 
'\$sm_path' failed")"
+   return
+   fi
+
gitdir=
gi

[PATCH v2 1/2] i18n: proposed command missing leading dash

[Sandy Carter]

Add missing leading dash to proposed commands in french output when
using the command:
git branch --set-upstream remotename/branchname
and when upstream is gone

Signed-off-by: Sandy Carter 
---
 po/fr.po | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/po/fr.po b/po/fr.po
index e10263f..0b9d59e 100644
--- a/po/fr.po
+++ b/po/fr.po
@@ -1075,7 +1075,7 @@ msgstr "Votre branche est basée sur '%s', mais la branche 
amont a disparu.\n"
 
 #: remote.c:1875
 msgid "  (use \"git branch --unset-upstream\" to fixup)\n"
-msgstr "  (utilisez \"git branch -unset-upstream\" pour corriger)\n"
+msgstr "  (utilisez \"git branch --unset-upstream\" pour corriger)\n"
 
 #: remote.c:1878
 #, c-format
@@ -3266,7 +3266,7 @@ msgstr "git branch -d %s\n"
 #: builtin/branch.c:1027
 #, c-format
 msgid "git branch --set-upstream-to %s\n"
-msgstr "git branch -set-upstream-to %s\n"
+msgstr "git branch --set-upstream-to %s\n"
 
 #: builtin/bundle.c:47
 #, c-format
-- 
1.9.0

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

Re: [PATCH 1/2] test-lib: tests skipped by GIT_SKIP_TESTS say so

[Philip Oakley]

From: "Ilya Bobyr" 

We used to show "(missing )" next to tests skipped because they are
specified in GIT_SKIP_TESTS.  Use "(matched by GIT_SKIP_TESTS)" 
instead.


The message below forgets the "by". Otherwise looks sensible.


---
t/test-lib.sh |   13 -
1 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/t/test-lib.sh b/t/test-lib.sh
index 1531c24..89a405b 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -446,25 +446,28 @@ test_finish_ () {

test_skip () {
 to_skip=
+ skipped_reason=
 if match_pattern_list $this_test.$test_count $GIT_SKIP_TESTS
 then
 to_skip=t
+ skipped_reason="matched GIT_SKIP_TESTS"


s/matched GIT_SKIP_TESTS/matched by GIT_SKIP_TESTS/


 fi
 if test -z "$to_skip" && test -n "$test_prereq" &&
! test_have_prereq "$test_prereq"
 then
 to_skip=t
- fi
- case "$to_skip" in
- t)
+
 of_prereq=
 if test "$missing_prereq" != "$test_prereq"
 then
 of_prereq=" of $test_prereq"
 fi
-
+ skipped_reason="missing $missing_prereq${of_prereq}"
+ fi
+ case "$to_skip" in
+ t)
 say_color skip >&3 "skipping test: $@"
- say_color skip "ok $test_count # skip $1 (missing 
$missing_prereq${of_prereq})"

+ say_color skip "ok $test_count # skip $1 ($skipped_reason)"
 : true
 ;;
 *)
--
1.7.9


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

Re: [PATCH] commit.c: Replace starts_with() with skip_prefix()

[karthik nayak]

Hello Eric,
Thanks for Pointing out everything, i had a thorough look and fixed a
couple of things.
Here is an Updated Patch.
- Removed unnecessary code and variables.
- Replaced all instances of starts_with() with skip_prefix()

Replace starts_with() with skip_prefix() for better reading purposes.
Also to replace "buf + strlen(author )" by skip_prefix(), which is
saved in a new "const char" variable "buf_skipprefix".

Signed-off-by: Karthik Nayak 
---
 commit.c | 13 -
 1 file changed, 8 insertions(+), 5 deletions(-)

diff --git a/commit.c b/commit.c
index 6bf4fe0..e5dc2e2 100644
--- a/commit.c
+++ b/commit.c
@@ -552,6 +552,7 @@ static void record_author_date(struct
author_date_slab *author_date,
  char *buffer = NULL;
  struct ident_split ident;
  char *date_end;
+ const char *buf_skipprefix;
  unsigned long date;

  if (!commit->buffer) {
@@ -562,18 +563,20 @@ static void record_author_date(struct
author_date_slab *author_date,
  return;
  }

+ buf_skipprefix = skip_prefix(buf, "author ");
+
  for (buf = commit->buffer ? commit->buffer : buffer;
  buf;
  buf = line_end + 1) {
  line_end = strchrnul(buf, '\n');
- if (!starts_with(buf, "author ")) {
+ if (!buf_skipprefix) {
  if (!line_end[0] || line_end[1] == '\n')
  return; /* end of header */
  continue;
  }
  if (split_ident_line(&ident,
- buf + strlen("author "),
- line_end - (buf + strlen("author "))) ||
+ buf_skipprefix,
+ line_end - buf_skipprefix) ||
 !ident.date_begin || !ident.date_end)
  goto fail_exit; /* malformed "author" line */
  break;
@@ -1113,7 +1116,7 @@ int parse_signed_commit(const unsigned char *sha1,
  next = next ? next + 1 : tail;
  if (in_signature && line[0] == ' ')
  sig = line + 1;
- else if (starts_with(line, gpg_sig_header) &&
+ else if (skip_prefix(line, gpg_sig_header) &&
  line[gpg_sig_header_len] == ' ')
  sig = line + gpg_sig_header_len + 1;
  if (sig) {
@@ -1193,7 +1196,7 @@ static void parse_gpg_output(struct signature_check *sigc)
  for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
  const char *found, *next;

- if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
+ if (skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) {
  /* At the very beginning of the buffer */
  found = buf + strlen(sigcheck_gpg_status[i].check + 1);
  } else {
-- 
1.9.0.138.g2de3478
--
To unsubscribe from this list: send the line "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] test-lib: GIT_TEST_ONLY to run only specific tests

[Philip Oakley]

Minor nits.

From: "Ilya Bobyr" 
This is a counterpart to GIT_SKIP_TESTS.  Mostly useful when 
debugging.

---
t/README  |   15 +++
t/test-lib.sh |8 
2 files changed, 23 insertions(+), 0 deletions(-)

diff --git a/t/README b/t/README
index caeeb9d..f939987 100644
--- a/t/README
+++ b/t/README
@@ -187,6 +187,21 @@ and either can match the "t[0-9]{4}" part to skip 
the whole

test, or t[0-9]{4} followed by ".$number" to say which
particular test to skip.

+Sometimes the opposite is desired - ability to execute only one or
+several tests.  Mostly while debugging tests.  For that you can say
+
+$ GIT_TEST_ONLY=t9200.8 sh ./t9200-git-cvsexport-commit.sh
+
+or, similrary to GIT_SKIP_TESTS
+
+$ GIT_TEST_ONLY='t[0-4]??? t91?? t9200.8' make
+
+In additiona to matching against "."
+GIT_TEST_ONLY is matched against just the test .  This comes

s/numbes/numbers/


+handy when you are running only one test:

s/handy/in handy/


+
+$ GIT_TEST_ONLY='[0-8]' sh ./t9200-git-cvsexport-commit.sh
+
Note that some tests in the existing test suite rely on previous
test item, so you cannot arbitrarily disable one and expect the
remainder of test to check what the test originally was intended
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 89a405b..12bf436 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -464,6 +464,14 @@ test_skip () {
 fi
 skipped_reason="missing $missing_prereq${of_prereq}"
 fi
+ if test -z "$to_skip" && test -n "$GIT_TEST_ONLY" &&
+ ! match_pattern_list $this_test.$test_count $GIT_TEST_ONLY &&
+ ! match_pattern_list $test_count $GIT_TEST_ONLY
+ then
+ to_skip=t
+ skipped_reason="not in GIT_TEST_ONLY"
+ fi
+
 case "$to_skip" in
 t)
 say_color skip >&3 "skipping test: $@"
--
Philip 


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

[PATCH V2] commit.c: Use skip_prefix() instead of starts_with()

[Tanay Abhra]

In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of
starts_with() is more elegant and abstracts away the details.

Helped-by: Michael Haggerty 
Signed-off-by: Tanay Abhra 
---
Patch V2 Corrected email formatting ,reapplied the implementation according to 
suggestions.
Thanks to Michael Haggerty.

This is in respect to GSoC microproject #10.

In record_author_date(), extra and useless calls to strlen due to using 
starts_with()
were removed by using skip_prefix(). Extra variable "skip" was used as "buf" is 
used in 
for loop update check.

Other usages of starts_with() in the same file can be found with,

$ grep -n starts_with commit.c

1116:   else if (starts_with(line, gpg_sig_header) &&
1196:   if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {

The starts_with() in line 1116 was left as it is, as strlen values were pre 
generated as 
global variables.
The starts_with() in line 1196 was replaced as it abstracts way the skip_prefix 
part by
directly using the function.
Also skip_prefix() is inline when compared to starts_with().

 commit.c | 13 +++--
 1 file changed, 7 insertions(+), 6 deletions(-)

diff --git a/commit.c b/commit.c
index 6bf4fe0..668c703 100644
--- a/commit.c
+++ b/commit.c
@@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
 static void record_author_date(struct author_date_slab *author_date,
   struct commit *commit)
 {
-   const char *buf, *line_end;
+   const char *buf, *line_end, *skip;
char *buffer = NULL;
struct ident_split ident;
char *date_end;
@@ -566,14 +566,15 @@ static void record_author_date(struct author_date_slab 
*author_date,
 buf;
 buf = line_end + 1) {
line_end = strchrnul(buf, '\n');
-   if (!starts_with(buf, "author ")) {
+   if (!(skip = skip_prefix(buf, "author "))) {
if (!line_end[0] || line_end[1] == '\n')
return; /* end of header */
continue;
}
+   buf = skip;
if (split_ident_line(&ident,
-buf + strlen("author "),
-line_end - (buf + strlen("author "))) ||
+buf,
+line_end - buf) ||
!ident.date_begin || !ident.date_end)
goto fail_exit; /* malformed "author" line */
break;
@@ -1193,9 +1194,9 @@ static void parse_gpg_output(struct signature_check *sigc)
for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
const char *found, *next;
 
-   if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
+   if (found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) 
{
/* At the very beginning of the buffer */
-   found = buf + strlen(sigcheck_gpg_status[i].check + 1);
+   ;
} else {
found = strstr(buf, sigcheck_gpg_status[i].check);
if (!found)
-- 
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

[PATCH] cache_tree_find(): remove redundant check in while condition

[Michael Haggerty]

Signed-off-by: Michael Haggerty 
---
A trivial little cleanup.

 cache-tree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/cache-tree.c b/cache-tree.c
index 0bbec43..7f63c23 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -565,7 +565,7 @@ static struct cache_tree *cache_tree_find(struct cache_tree 
*it, const char *pat
return NULL;
it = sub->cache_tree;
if (slash)
-   while (*slash && *slash == '/')
+   while (*slash == '/')
slash++;
if (!slash || !*slash)
return it; /* prefix ended with slashes */
-- 
1.9.0

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

Re: [PATCH] cache_tree_find(): remove redundant check in while condition

[David Kastrup]

Michael Haggerty  writes:

> Signed-off-by: Michael Haggerty 
> ---
> A trivial little cleanup.
>
>  cache-tree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/cache-tree.c b/cache-tree.c
> index 0bbec43..7f63c23 100644
> --- a/cache-tree.c
> +++ b/cache-tree.c
> @@ -565,7 +565,7 @@ static struct cache_tree *cache_tree_find(struct 
> cache_tree *it, const char *pat
>   return NULL;
>   it = sub->cache_tree;
>   if (slash)
> - while (*slash && *slash == '/')
> + while (*slash == '/')
>   slash++;
>   if (!slash || !*slash)
>   return it; /* prefix ended with slashes */

That seems dragging around a NULL slash a lot.  How about not checking
for it multiple times?

if (!slash)
return it;
while (*slash == '/')
slash++;
if (!*slash)
return it; /* prefix ended with slashes */

As a bonus, the comment appears to be actually correct.  Attempting to
verify its correctness by seeing whether a non-NULL slash is guaranteed
to really end with slashes, we find the following code above for
defining slash:

slash = strchr(path, '/');
if (!slash)
slash = path + strlen(path);

So it is literally impossible for slash to ever be NULL and all the
checking is nonsensical.  In addition, "prefix ended with slashes" does
not seem overly convincing when this code path is reached whether or not
there is a slash at all (I am not sure about it: it depends on the
preceding find_subtree to some degree).

So perhaps all of that should just be

while (*slash == '/')
slash++;
if (!*slash)
return it;

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

Re: My advice for GSoC applicants

[Philip Oakley]

From: "Michael Haggerty" 
Cc: "Dmitry Dolzhenko" ; "Sun He"
; "Brian Gesiak" ; "Tanay
Abhra" ; "Kyriakos Georgiou"
; "Siddharth Goel"
; "Guanglin Xu" ;
"Karthik Nayak" ; "Alberto Corona"
; "Jacopo Notarstefano"



Hi,

Based on my experience so far as a first-time Google Summer of Code
mentor, I just wrote a blog article containing some hopefully useful
advice for students applying to the program.  Please note that this is
my personal opinion only and doesn't necessarily reflect the views of
the Git/libgit2 projects as a whole.

   My secret tip for GSoC success

http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html

Michael

--
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--


In particular I liked : " If the documentation is unclear, it is OK to
ask for a clarification, but then _fix the documentation_ so that your
mentor never has to answer the same question again."

So the rhetorical question(s) for students would be :-

- was the Git documentation useful - did you see the README?, did it
lead, easily, to the useful places [1]? How could the wording/layout be
improved for the first time reader?

- which points of clarification were most useful and are they in the
documentation? Where should they be included?

- which points needed repeating often, and why? Where was the
disconnect?

- what would a patch look like...

Philip Oakley

[1] README; INSTALL; Documentation/SubmittingPatches;
Documentation/CodingGuidelines;

--
To unsubscribe from this list: send the line "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] cache_tree_find(): remove redundant check in while condition

[Michael Haggerty]

On 03/03/2014 05:32 PM, David Kastrup wrote:
> [...]
> So perhaps all of that should just be
> 
>   while (*slash == '/')
>   slash++;
>   if (!*slash)
>   return it;
> 

I just fixed a little thing that caught my eye.  You OWNED it.  You are
absolutely right.

Will you prepare a patch or would you like me to do it?

Michael

-- 
Michael Haggerty
mhag...@alum.mit.edu
http://softwareswirl.blogspot.com/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: My advice for GSoC applicants

[Rick Umali]

On Mon, Mar 3, 2014 at 5:45 AM, Michael Haggerty  wrote:
> My secret tip for GSoC success
>
> http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html

I enjoyed reading that BLOG post. I daresay some of the points you
raise are pertinent for any new contributor to any open-source code
base.
--
To unsubscribe from this list: send the line "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] cache_tree_find(): remove redundant check in while condition

[David Kastrup]

Michael Haggerty  writes:

> On 03/03/2014 05:32 PM, David Kastrup wrote:
>> [...]
>> So perhaps all of that should just be
>> 
>>  while (*slash == '/')
>>  slash++;
>>  if (!*slash)
>>  return it;
>> 
>
> I just fixed a little thing that caught my eye.  You OWNED it.  You are
> absolutely right.
>
> Will you prepare a patch or would you like me to do it?

Feel free.  I really should be finishing some other patches instead...

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

Re: [PATCH] submodule : Add --no-separate-git-dir option to add and update command.

[Jens Lehmann]

Am 03.03.2014 14:47, schrieb Henri GEIST:
> This new option prevent git submodule  to clone the missing
> submodules with the --separate-git-dir option.
> Then the submodule will be regular repository and their gitdir will not
> be placed in the superproject gitdir/modules directory.

And what is your motivation for this? After all submodules containing
a .git directory are second class citizens (because they can never be
safely removed by regular git commands).

> Signed-off-by: Henri GEIST 
> ---
>  Documentation/git-submodule.txt |   18 --
>  git-submodule.sh|   22 --
>  t/t7400-submodule-basic.sh  |   12 
>  3 files changed, 48 insertions(+), 4 deletions(-)
> 
> diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
> index 21cb59a..303a475 100644
> --- a/Documentation/git-submodule.txt
> +++ b/Documentation/git-submodule.txt
> @@ -10,13 +10,14 @@ SYNOPSIS
>  
>  [verse]
>  'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
> -   [--reference ] [--depth ] [--]  
> []
> +   [--reference ] [--depth ] 
> [--no-separate-git-dir]
> +   [--]  []
>  'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
>  'git submodule' [--quiet] init [--] [...]
>  'git submodule' [--quiet] deinit [-f|--force] [--] ...
>  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
> [-f|--force] [--rebase|--merge|--checkout] [--reference 
> ]
> -   [--depth ] [--recursive] [--] [...]
> +   [--depth ] [--recursive] [--no-separate-git-dir] [--] 
> [...]
>  'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) 
> ]
> [commit] [--] [...]
>  'git submodule' [--quiet] foreach [--recursive] 
> @@ -107,6 +108,10 @@ is the superproject and submodule repositories will be 
> kept
>  together in the same relative location, and only the
>  superproject's URL needs to be provided: git-submodule will correctly
>  locate the submodule using the relative URL in .gitmodules.
> ++
> +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> +has normal git repository without the option `--separate-git-dir` pointing
> +to the modules directory of the superproject gitdir.
>  
>  status::
>   Show the status of the submodules. This will print the SHA-1 of the
> @@ -185,6 +190,10 @@ If the submodule is not yet initialized, and you just 
> want to use the
>  setting as stored in .gitmodules, you can automatically initialize the
>  submodule with the `--init` option.
>  +
> +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> +has normal git repository without the option `--separate-git-dir` pointing
> +to the modules directory of the superproject gitdir.
> ++
>  If `--recursive` is specified, this command will recurse into the
>  registered submodules, and update any nested submodules within.
>  +
> @@ -363,6 +372,11 @@ for linkgit:git-clone[1]'s `--reference` and `--shared` 
> options carefully.
>   clone with a history truncated to the specified number of revisions.
>   See linkgit:git-clone[1]
>  
> +--no-separate-git-dir::
> + This option is valid for add and update commands. Specify that missing
> + submodules should be clonned as self contain repository without a
> + separate gitdir placed in the modules directory of the superproject
> + gitdir.
>  
>  ...::
>   Paths to submodule(s). When specified this will restrict the command
> diff --git a/git-submodule.sh b/git-submodule.sh
> index a33f68d..36eaf31 100755
> --- a/git-submodule.sh
> +++ b/git-submodule.sh
> @@ -5,11 +5,11 @@
>  # Copyright (c) 2007 Lars Hjemli
>  
>  dashless=$(basename "$0" | sed -e 's/-/ /')
> -USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
> ] [--]  []
> +USAGE="[--quiet] add [-b ] [-f|--force] [--name ] [--reference 
> ] [--no-separate-git-dir] [--]  []
> or: $dashless [--quiet] status [--cached] [--recursive] [--] [...]
> or: $dashless [--quiet] init [--] [...]
> or: $dashless [--quiet] deinit [-f|--force] [--] ...
> -   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
> [-f|--force] [--rebase] [--reference ] [--merge] [--recursive] 
> [--] [...]
> +   or: $dashless [--quiet] update [--init] [--remote] [-N|--no-fetch] 
> [-f|--force] [--rebase] [--reference ] [--merge] [--recursive] 
> [--no-separate-git-dir] [--] [...]
> or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
> [commit] [--] [...]
> or: $dashless [--quiet] foreach [--recursive] 
> or: $dashless [--quiet] sync [--recursive] [--] [...]"
> @@ -36,6 +36,7 @@ update=
>  prefix=
>  custom_name=
>  depth=
> +noseparategitdir=
>  
>  # The function takes at most 2 arguments. The first argument is the
>  # URL that navigates to the submodule origin repo. When relative, this URL
> @@ -270,6 +271,17 @@ module_clone()
>   quiet=-q
> 

Re: [PATCH v3 01/25] path.c: make get_pathname() return strbuf instead of static buffer

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, Feb 20, 2014 at 6:26 AM, Junio C Hamano  wrote:
>> Is there a reason not to do just an equivalent of
>>
>> #define git_pathdup mkpathdup
>>
>> and be done with it?  Am I missing something?
>
> They have a subtle difference: mkpathdup() calls cleanup_path() while
> git_pathdup() does not. Without auditing all call sites, we can't be
> sure this difference is insignificant. So I keep both functions
> separate for now.

Thanks; that is a very good explanation why #define'ing two to be
the same would not be a workable solution to the ownership issue I
raised.

>  char *git_pathdup(const char *fmt, ...)
>  {
> - char path[PATH_MAX], *ret;
> + struct strbuf *path = get_pathname();
>   va_list args;
>   va_start(args, fmt);
> - ret = vsnpath(path, sizeof(path), fmt, args);
> + vsnpath(path, fmt, args);
>   va_end(args);
> - return xstrdup(ret);
> + return strbuf_detach(path, NULL);
>  }

This feels somewhat wrong.

This function is for callers who are willing to take ownership of
the path buffer and promise to free the returned buffer when they
are done, because you are returning strbuf_detach()'ed piece of
memory, giving the ownership away.

The whole point of using get_pathname() is to allow callers not to
care about allocation issues on the paths they scribble on during
their short-and-simple codepaths that do not have too many uses of
similar temporary path buffers.  Why borrow from that round-robin
pool (which may now cause some codepaths to overflow the number of
such active temporary path buffers---have they been all audited)?
--
To unsubscribe from this list: send the line "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: Branch Name Case Sensitivity

[Junio C Hamano]

Lee Hopkins  writes:

> I went ahead and took a stab at a solution. My solution is more
> aggressive than a warning, I actually prevent the creation of
> ambiguous refs. My changes are also in refs.c, which may not be
> appropriate, but it seemed like the natural place.
>
> I have never contributed to Git (in fact this is my first dive into
> the source) and my C is a bit rusty, so bear with me, this is just a
> suggestion:
>
> ---
>  refs.c |   31 ---
>  1 files changed, 24 insertions(+), 7 deletions(-)

Starting something like this from forbidding is likely to turn out
to be a very bad idea that can break existing repositories.

A new configuration

refs.caseInsensitive = {warn|error|allow}

that defaults to "warn" and the user can choose to set to "error" to
forbid, would be more palatable, I would say.

If the variable is not in 'core.' namespace, you should implement
this check at the Porcelain level, allowing lower-level tools like
update-ref as an escape hatch that let users bypass the restriction
to be used to correct breakages; it would mean an unconditional "if
!stricmp(), it is an error" in refs.c will not work well.

I think it might be OK to have

core.allowCaseInsentitiveRefs = {yes|no|warn}

which defaults to 'warn' (and 'yes' corresponds to 'allow', 'no'
corresponds to 'error', in the previous suggestion), instead. If we
wanted to prevent even lower-level tools like update-ref from
bypassing the check, that is.

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

Re: [PATCH v3 07/25] reflog: avoid constructing .lock path with git_path

[Junio C Hamano]

Duy Nguyen  writes:

> On Wed, Feb 26, 2014 at 5:44 AM, Junio C Hamano  wrote:
>> Nguyễn Thái Ngọc Duy   writes:
>>
>>> git_path() soon understands the path given to it. Some paths "abc" may
>>> become "def" while other "ghi" may become "ijk". We don't want
>>> git_path() to interfere with .lock path construction. Concatenate
>>> ".lock" after the path has been resolved by git_path() so if "abc"
>>> becomes "def", we'll have "def.lock", not "ijk".
>>
>> Hmph.  I am not sure if the above is readable (or if I am reading it
>> correctly).
>
> Definitely not now that I have had my break from the series and reread it.
>
>> If "abc" becomes "def", it would take deliberate work to make
>> "abc.lock" into "ijk", and it would be natural to expect that
>> "abc.lock" would become "def.lock" without any fancy trick, no?
>
> A better explanation may be, while many paths are not converted by
> git_path() ("abc" -> "abc"), some of them will be based on the given
> path ("def" -> "ghi"). Giving path def.lock to git_path() may confuse
> it and make it believe def.lock should not be converted because the
> signature is "def.lock" not "def". But we want the lock file to have
> the same base name with the locked file (e.g. "ghi.lock", not
> "def.lock"). So it's best to append ".lock" after git_path() has done
> its conversion.
>
> The alternative is teach git_path about ".lock", but I don't really
> want to put more logic down there.

I think your attempt to sound generic by using "abc", "def",
etc. backfired and made the description only obscure.  It would have
been immediately understandable if it were more like this:

Among pathnames in $GIT_DIR, e.g. "index" or "packed-refs",
we want to automatically and silently map some of them to
the $GIT_DIR of the repository we are borrowing from via
$GIT_COMMON_DIR mechanism.  When we formulate the pathname
for its lockfile, we want it to be in the same location as
its final destination.  "index" is not shared and needs to
remain in the borrowing repository, while "packed-refs" is
shared and needs to go to the borrowed repository.

git_path() could be taught about the ".lock" suffix and map
"index.lock" and "packed-refs.lock" the same way their
basenames are mapped, but instead the caller can help by
asking where the basename (e.g. "index") is mapped to
git_path() and then appending ".lock" after the mapping is
done.

Thanks for an explanation.  With that understanding, the patch makes
sense.


--
To unsubscribe from this list: send the line "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] repack: add `repack.honorpackkeep` config var

[Junio C Hamano]

Jeff King  writes:

> On Fri, Feb 28, 2014 at 10:45:39AM -0800, Junio C Hamano wrote:
>
>> > Exactly. The two features (bitmaps and .keep) are not compatible with
>> > each other, so you have to prioritize one. If you are using static .keep
>> > files, you might want them to continue being respected at the expense of
>> > using bitmaps for that repo. So I think you want a separate option from
>> > --write-bitmap-index to allow the appropriate flexibility.
>> 
>> What is "the appropriate flexibility", though?  If the user wants to
>> use bitmap, we would need to drop .keep, no?
>
> Or the flip side: if the user wants to use .keep, we should drop
> bitmaps. My point is that we do not know which way the user wants to
> go, so we should not tie the options together.

Hmph.  I think the short of your later explanation is "global config
may tell us to use bitmap, in which case we would need a way to
defeat that and have existing .keep honored, and it makes it easier
to do so if these two are kept separate, because you do not want to
run around and selectively disable bitmaps in these repositories.
We can instead do so with repack.packKeptObjects in the global
configuration." and I tend to agree with the reasoning.

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] repack: add `repack.honorpackkeep` config var

[Jeff King]

On Mon, Mar 03, 2014 at 10:13:47AM -0800, Junio C Hamano wrote:

> > Or the flip side: if the user wants to use .keep, we should drop
> > bitmaps. My point is that we do not know which way the user wants to
> > go, so we should not tie the options together.
> 
> Hmph.  I think the short of your later explanation is "global config
> may tell us to use bitmap, in which case we would need a way to
> defeat that and have existing .keep honored, and it makes it easier
> to do so if these two are kept separate, because you do not want to
> run around and selectively disable bitmaps in these repositories.
> We can instead do so with repack.packKeptObjects in the global
> configuration." and I tend to agree with the reasoning.

Yes. Do you need a re-roll from me? I think the last version I sent +
the squash to tie the default to bitmap-writing makes the most sense.

-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 00/11] Use ALLOC_GROW() instead of inline code

[Junio C Hamano]

Michael Haggerty  writes:

> On 02/28/2014 10:29 AM, Dmitry S. Dolzhenko wrote:
>> Thank you for your remarks. In this patch I tried to take them into account.
>> 
>> Dmitry S. Dolzhenko (11):
>>   builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()
>>   bundle.c: change add_to_ref_list() to use ALLOC_GROW()
>>   cache-tree.c: change find_subtree() to use ALLOC_GROW()
>>   commit.c: change register_commit_graft() to use ALLOC_GROW()
>>   diff.c: use ALLOC_GROW() instead of inline code
>>   diffcore-rename.c: use ALLOC_GROW() instead of inline code
>>   patch-ids.c: change add_commit() to use ALLOC_GROW()
>>   replace_object.c: change register_replace_object() to use ALLOC_GROW()
>>   reflog-walk.c: use ALLOC_GROW() instead of inline code
>>   dir.c: change create_simplify() to use ALLOC_GROW()
>>   attr.c: change handle_attr_line() to use ALLOC_GROW()
>> 
>>  attr.c |  7 +--
>>  builtin/pack-objects.c |  7 +--
>>  bundle.c   |  6 +-
>>  cache-tree.c   |  6 +-
>>  commit.c   |  8 ++--
>>  diff.c | 12 ++--
>>  diffcore-rename.c  | 12 ++--
>>  dir.c  |  5 +
>>  patch-ids.c|  5 +
>>  reflog-walk.c  | 13 +++--
>>  replace_object.c   |  8 ++--
>>  11 files changed, 17 insertions(+), 72 deletions(-)
>
> Everything looks fine to me.  Assuming the test suite ran 100%,
>
> Acked-by: Michael Haggerty 

Looked good (modulo titles, which I think we already discussed),
and queued on 'pu'.

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

Re: [PATCH v2 01/11] builtin/pack-objects.c: change check_pbase_path() to use ALLOC_GROW()

[Junio C Hamano]

Jeff King  writes:

> I realize that I just bikeshedded on subject lines for half a page, and
> part of me wants to go kill myself in shame. But I feel like I see the
> technique misapplied often enough that maybe some guidance is merited.

Thanks.  What I queued read like these:

$ git shortlog ..dd/use-alloc-grow

Dmitry S. Dolzhenko (11):
  builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
  bundle.c: use ALLOC_GROW() in add_to_ref_list()
  cache-tree.c: use ALLOC_GROW() in find_subtree()
  commit.c: use ALLOC_GROW() in register_commit_graft()
  diff.c: use ALLOC_GROW() instead of inline code
  diffcore-rename.c: use ALLOC_GROW() instead of inline code
  patch-ids.c: use ALLOC_GROW() in add_commit()
  replace_object.c: use ALLOC_GROW() in register_replace_object()
  reflog-walk.c: use ALLOC_GROW() instead of inline code
  dir.c: use ALLOC_GROW() in create_simplify()
  attr.c: use ALLOC_GROW() in handle_attr_line()

but I tend to agree with you that we can just stop at "use ALLOC_GROW"
after the filename.
--
To unsubscribe from this list: send the line "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] implemented strbuf_write_or_die()

[Junio C Hamano]

Faiz Kothari  writes:

> Signed-off-by: Faiz Kothari 
> ---
> Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places
> to substitute write_or_die(). I spotted other places where strbuf can be used
> in place of buf[MAX_PATH] but that would require a change in prototype of a 
> lot of functions and functions calling them and so on
> I'll look for more places where strbuf can be used safely.
>
> Thanks.
>
>  builtin/cat-file.c |2 +-
>  builtin/notes.c|4 ++--
>  builtin/receive-pack.c |2 +-
>  builtin/send-pack.c|2 +-
>  builtin/stripspace.c   |2 +-
>  builtin/tag.c  |2 +-
>  bundle.c   |2 +-
>  cache.h|1 +
>  credential-store.c |2 +-
>  fetch-pack.c   |2 +-
>  http-backend.c |2 +-
>  remote-curl.c  |8 +---
>  write_or_die.c |9 +
>  13 files changed, 26 insertions(+), 14 deletions(-)

It does not reduce the code, it does not make the resulting code
read any easier.

What is the benefit of this change?
--
To unsubscribe from this list: send the line "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] implemented strbuf_write_or_die()

[Junio C Hamano]

Eric Sunshine  writes:

> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
>>> diff --git a/remote-curl.c b/remote-curl.c
>>> index 10cb011..dee8716 100644
>>> --- a/remote-curl.c
>>> +++ b/remote-curl.c
>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
>>> discovery *heads)
>>> if (start_command(&client))
>>> exit(1);
>>> if (preamble)
>>> -   write_or_die(client.in, preamble->buf, preamble->len);
>>> +   strbuf_write_or_die(client.in, preamble);
>>> if (heads)
>>> write_or_die(client.in, heads->buf, heads->len);
>>
>> This should be changed. May be you can use Ctrl-F to search write_or_die().
>> Or if you are using vim, use "/ and n" to find all.
>
> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
> so Faiz correctly left this invocation alone.

That is a very good sign why this change is merely a code-churn and
not an improvement, isn't it?  We know (and any strbuf user should
know) that ->buf and ->len are the ways to learn the pointer and the
length the strbuf holds.  Why anybody thinks it is benefitial to
introduce another function that is _only_ for writing out strbuf and
cannot be used to write out a plain buffer is simply beyond me.

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

Re: An idea for "git bisect" and a GSoC enquiry

[Junio C Hamano]

Jacopo Notarstefano  writes:

> Here my proposal differs in that I have no way of knowing which label
> is good and which label is bad: I blindly accept two distinct labels
> and bisect with those. I gave an example of this behaviour above.

I think you fundamentally cannot use two labels that are merely
"distinct" and bisect correctly.  You need to know which ones
(i.e. good) are to be excluded and the other (i.e. bad) are to be
included when computing the "remaining to be tested" set of commits.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Rewriting git history during git-svn conversion

[Robert Dailey]

Hi,

I am converting my team's SVN server to GIT but they aren't ready to
transition yet. People are still working out of SVN, so I need to keep
the git-svn clone around to do 'git svn fetch' to continue to keep it
synchronized with SVN.

Once everyone is ready to switch, and after converting all of the
tag-branches to real GIT tags, I plan to push all branches & tags to
the new central git repository.

However, before pushing to the central GIT repo, I want to remove some
giant directories from the repository history. Specifically some third
party library directories. I have found a way to do this here:
http://stackoverflow.com/questions/10067848/remove-folder-and-its-contents-from-git-githubs-history

Is it safe to do this while still using git svn fetch? Will it
properly continue to convert SVN commits on top of my rewritten
history? If not, what changes can I make after I run the commands
linked by the URL above so that git svn continues to work normally?

Note that I plan to delete the third party libraries from SVN side and
then pull down the changes to git through git-svn, then at that point
clean it up (hopefully the git filter-branch command ignores the
commit that deleted the files and only hunts down the commits where
they were added or modified).

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

Re: [PATCH] commit.c:record_author_date() use skip_prefix() instead of starts_with()

[Junio C Hamano]

Michael Haggerty  writes:

>> -if (!starts_with(buf, "author ")) {
>> +if (!skip_prefix(buf, "author ")) {
>
> If this is the only change, there is not much point, is there?  How does
> this help?  Perhaps there is some way to take advantage of the
> difference between starts_with() and skip_prefix() to simplify the rest
> of the function?

I admit I lost track, but wasn't there a discussion to use
starts_with/ends_with when appropriate (namely, the caller is
absolutely not interested in what the remainder of the string is
after skipping the prefix), moving away from skip_prefix()?  Isn't
this change going in the wrong direction?
--
To unsubscribe from this list: send the line "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] write_pack_file: use correct variable in diagnostic

[Junio C Hamano]

Sun He  writes:

> 'pack_tmp_name' is the subject of the utime() check, so report it in the
> warning, not the uninitialized 'tmpname'
>
> Signed-off-by: Sun He 
> ---
>
>  Changing the subject and adding valid information as tutored by 
>  Eric Sunshine.
>  Thanks to him.
>
>  builtin/pack-objects.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index c733379..4922ce5 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -823,7 +823,7 @@ static void write_pack_file(void)
>   utb.modtime = --last_mtime;
>   if (utime(pack_tmp_name, &utb) < 0)
>   warning("failed utime() on %s: %s",
> - tmpname, strerror(errno));
> + pack_tmp_name, strerror(errno));
>   }
>  
>   /* Enough space for "-.pack"? */

Very nicely done.  Thanks.

And big Thanks to Eric guiding this patch through.

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

Fwd: [PATCH] implemented strbuf_write_or_die()

[Faiz Kothari]

On Tue, Mar 4, 2014 at 12:01 AM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
 diff --git a/remote-curl.c b/remote-curl.c
 index 10cb011..dee8716 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
 discovery *heads)
 if (start_command(&client))
 exit(1);
 if (preamble)
 -   write_or_die(client.in, preamble->buf, preamble->len);
 +   strbuf_write_or_die(client.in, preamble);
 if (heads)
 write_or_die(client.in, heads->buf, heads->len);
>>>
>>> This should be changed. May be you can use Ctrl-F to search write_or_die().
>>> Or if you are using vim, use "/ and n" to find all.
>>
>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
>> so Faiz correctly left this invocation alone.
>
> That is a very good sign why this change is merely a code-churn and
> not an improvement, isn't it?  We know (and any strbuf user should
> know) that ->buf and ->len are the ways to learn the pointer and the
> length the strbuf holds.  Why anybody thinks it is benefitial to
> introduce another function that is _only_ for writing out strbuf and
> cannot be used to write out a plain buffer is simply beyond me.
>
Hi,
Thanks for the feedback. Yes, I do realize, its kind of a code churn.
I didn't realize it until I looked at the sign you pointed out.
But it was a good exercise to go through the code as this is one of
the GSoC microprojects.
Sorry, it didn't turn out to be a beneficial one. My bad.

Thanks a lot again for the suggestions and feedback.

-Faiz
--
To unsubscribe from this list: send the line "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] branch.c: change install_branch_config() to use skip_prefix()

[Junio C Hamano]

Eric Sunshine  writes:

>> diff --git a/branch.c b/branch.c
>> index 723a36b..ca4e824 100644
>> --- a/branch.c
>> +++ b/branch.c
>> @@ -50,7 +50,7 @@ static int should_setup_rebase(const char *origin)
>>  void install_branch_config(int flag, const char *local, const char *origin, 
>> const char *remote)
>>  {
>> const char *shortname = remote + 11;
>> -   int remote_is_branch = starts_with(remote, "refs/heads/");
>> +   int remote_is_branch = (NULL != skip_prefix(remote ,"refs/heads/"));
>
> This actually makes the code more difficult to read and understand.
> There's a more fundamental reason to use skip_prefix() here. See if
> you can figure it out. (Hint: shortname)

I've already queued 0630aa49 (branch: use skip_prefix() in
install_branch_config(), 2014-02-28) on this topic, by the way.

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

Re: [PATCH] git-config: document interactive.singlekey requires Term::Readkey

[Junio C Hamano]

Simon Ruderich  writes:

> Most distributions don't require Term::Readkey as dependency,
> leaving the user to wonder why the setting doesn't work.
>
> Signed-off-by: Simon Ruderich 

Thanks, but is it true that interactive.singlekey "requries"
Term::ReadKey?

The relevant part of git-add--interactive reads like so:

if ($repo->config_bool("interactive.singlekey")) {
eval {
require Term::ReadKey;
Term::ReadKey->import;
$use_readkey = 1;
};
eval {
require Term::Cap;
my $termcap = Term::Cap->Tgetent;
foreach (values %$termcap) {
$term_escapes{$_} = 1 if /^\e/;
}
$use_termcap = 1;
};
}

The implementation of prompt_single_character sub wants to use
ReadKey, but can still let the user interact with the program by
falling back to a cooked input when it is not available, so perhaps
a better fix might be something like this:

if (!$use_readkey) {
print STDERR "missing Term::ReadKey, disabling 
interactive.singlekey\n";
}

inside the above if() that prepares $use_readkey?

You also misspelled the package name it seems ;-)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3] skip_prefix: rewrite so that prefix is scanned once

[Junio C Hamano]

Siddharth Goel  writes:

> Helped-by: Eric Sunshine 
> Signed-off-by: Siddharth Goel 
> ---
> Added a space after colon in the subject as compared to previous 
> patch [PATCH v2].
>
> [PATCH v2]: http://thread.gmane.org/gmane.comp.version-control.git/243150

Whenever you see "Change", "Rewrite", etc. in the subject of a patch
that touches existing code, think twice.  The subject line is a
scarce real estate to be wasted on a noiseword that carries no real
information, and we already know a patch that touches existing code
changes or rewrites things.

Subject: [PATCH v3] skip_prefix: scan prefix only once

perhaps?

>  git-compat-util.h | 7 +--
>  1 file changed, 5 insertions(+), 2 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index 614a5e9..550dce3 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char 
> *suffix);
>  
>  static inline const char *skip_prefix(const char *str, const char *prefix)
>  {
> - size_t len = strlen(prefix);
> - return strncmp(str, prefix, len) ? NULL : str + len;
> + while (*prefix != '\0' && *str == *prefix) {
> + str++;
> + prefix++;
> + }
> + return (*prefix == '\0' ? str : NULL);

Unlike another patch I saw the other day on the same topic, this
checks *prefix twice for the last round, even though I think this
one is probably slightly easier to read.  I dunno.

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

Re: [PATCH v3 00/11] Use ALLOC_GROW() instead of inline code

[Junio C Hamano]

Eric Sunshine  writes:

> On Mon, Mar 3, 2014 at 2:18 AM, Dmitry S. Dolzhenko
>  wrote:
>> Dmitry S. Dolzhenko (11):
>>   builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
>>   bundle.c: use ALLOC_GROW() in add_to_ref_list()
>>   cache-tree.c: use ALLOC_GROW() in find_subtree()
>>   commit.c: use ALLOC_GROW() in register_commit_graft()
>>   diff.c: use ALLOC_GROW()
>>   diffcore-rename.c: use ALLOC_GROW()
>>   patch-ids.c: use ALLOC_GROW() in add_commit()
>>   replace_object.c: use ALLOC_GROW() in register_replace_object()
>>   reflog-walk.c: use ALLOC_GROW()
>>   dir.c: use ALLOC_GROW() in create_simplify()
>>   attr.c: use ALLOC_GROW() in handle_attr_line()
>>
>>  attr.c |  7 +--
>>  builtin/pack-objects.c |  9 +++--
>>  bundle.c   |  6 +-
>>  cache-tree.c   |  6 +-
>>  commit.c   |  8 ++--
>>  diff.c | 12 ++--
>>  diffcore-rename.c  | 12 ++--
>>  dir.c  |  5 +
>>  patch-ids.c|  5 +
>>  reflog-walk.c  | 12 ++--
>>  replace_object.c   |  8 ++--
>>  11 files changed, 18 insertions(+), 72 deletions(-)
>>
>> --
>> 1.8.5.3
>>
>> This version differs from previous only minor changes:
>>   - update commit messages
>>   - keep code lines within 80 columns
>
> Place this commentary at the top of the cover letter since that's
> where people look for it.
>
> You want to ease the reviewer's job as much as possible, so it helps
> to link to the previous submission, like this [1].
>
> Likewise, you can help the reviewer by being more specific about how
> you updated the commit messages (and perhaps by linking to the
> relevant discussion points, like this [2][3]).
>
> [1]: http://thread.gmane.org/gmane.comp.version-control.git/242857
> [2]: http://article.gmane.org/gmane.comp.version-control.git/243004
> [3]: http://article.gmane.org/gmane.comp.version-control.git/243049

It would be helpful for people to also pay attention to what is
pushed out on 'pu' ;-)

--
To unsubscribe from this list: send the line "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] repack: add `repack.honorpackkeep` config var

[Shawn Pearce]

On Fri, Feb 28, 2014 at 10:05 PM, Jeff King  wrote:
> On Fri, Feb 28, 2014 at 10:09:08AM -0700, Nasser Grainawi wrote:
>
>> > Exactly. The two features (bitmaps and .keep) are not compatible with
>> > each other, so you have to prioritize one. If you are using static .keep
>> > files, you might want them to continue being respected at the expense of
>> > using bitmaps for that repo. So I think you want a separate option from
>> > --write-bitmap-index to allow the appropriate flexibility.
>>
>> Has anyone thought about how to make them compatible?
>
> Yes, but it's complicated and not likely to happen soon.
>
> Having .keep files means that you are not including some objects in the
> newly created pack. Each bit in a commit's bitmap corresponds to one
> object in the pack, and whether it is reachable from that commit. The
> bitmap is only useful if we can calculate the full reachability from it,
> and it has no way to specify objects outside of the pack.
>
> To fix this, you would need to change the on-disk format of the bitmaps
> to somehow reference objects outside of the pack. Either by having the
> bitmaps index a repo-global set of objects, or by permitting a list of
> "edge" objects that are referenced from the pack, but not included (and
> then when assembling the full reachable list, you would have to recurse
> across "edge" objects to find their reachable list in another pack,
> etc).
>
> So it's possible, but it would complicate the scheme quite a bit, and
> would not be backwards compatible with either JGit or C Git.

Colby Ranger always wanted to add this to the bitmap scheme. Construct
a partial pack bitmap on a partial pack of "recent" objects, with edge
pointers naming objects that are not in this pack but whose closures
need to be considered part of the bitmap. Its complicated in-memory
because you need to fuse together two or more bitmaps (the partial
pack one, and the larger historical kept pack) before running the
"want AND NOT have" computation.

Colby did not find time to work on this in JGit, so it just didn't get
implemented. But we did consider it, as the servers at Google we built
bitmap for use a multi-level pack scheme and don't want to rebuild
packs all of the time.

>> We're using Martin Fick's git-exproll script which makes heavy use of
>> keeps to reduce pack file churn. In addition to the on-disk benefits
>> we get there, the driving factor behind creating exproll was to
>> prevent Gerrit from having two large (30GB+) mostly duplicated pack
>> files open in memory at the same time. Repacking in JGit would help in
>> a single-master environment, but we'd be back to having this problem
>> once we go to a multi-master setup.
>>
>> Perhaps the solution here is actually something in JGit where it could
>> aggressively try to close references to pack files
>
> In C git we don't worry about this too much, because our programs tend
> to be short-lived, and references to the old pack will go away quickly.
> Plus it is all mmap'd, so as we simply stop accessing the pages of the
> old pack, they should eventually be dropped if there is memory pressure.
>
> I seem to recall that JGit does not mmap its packfiles. Does it pread?

JGit does not mmap because you can't munmap() until the Java GC gets
around to freeing the tiny little header object that contains the
memory address of the start of the mmap segment. This can take ages,
to the point where you run out of virtual address space in the process
and s**t starts to fail left and right inside of the JVM. The GC is
just unable to prioritize finding those tiny headers and getting them
out of the heap so the munmap can take place safely.

So yea, JGit does pread() for the blocks but it holds those in its own
buffer cache inside of the Java heap. Where a 4K disk block is a 4K
memory array that puts pressure on the GC to actually wake up and free
resources that are unused. What Nasser is talking about is JGit may
take a long time to realize one pack is unused and start kicking those
blocks out of its buffer cache. Those blocks are reference counted and
the file descriptor JGit preads from is held open so long as at least
one block is in the buffer cache. By keeping the file open we force
the filesystem to keep the inode alive a lot longer, which means the
disk needs a huge amount of free space to store the unlinked but still
open 30G pack files from prior GC generations.

> In that case, I'd expect unused bits from the duplicated packfile to get
> dropped from the disk cache over time. If it loads whole packfiles into
> memory, then yes, it should probably close more aggressively.

Its more than that, its the inode being kept alive by the open file
descriptor...
--
To unsubscribe from this list: send the line "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] submodule : Add --no-separate-git-dir option to add and update command.

[Junio C Hamano]

[CC'ing the submodule area experts.]

Henri GEIST  writes:

> This new option prevent git submodule  to clone the missing
> submodules with the --separate-git-dir option.
> Then the submodule will be regular repository and their gitdir will not
> be placed in the superproject gitdir/modules directory.
>
> Signed-off-by: Henri GEIST 
> ---

Thanks.

The above describes what the new option does, but does not explain
why the new option is a good idea in the first place.

Given that we used to directly clone into the superproject's working
tree like this patch does, realized that it was a very bad idea and
are trying to move to the direction of keeping it in modules/
subdirectory of the superproject's .git directory, there needs to be
a very good explanation to justify why this "going backwards" is
sometimes a desirable thing.


--
To unsubscribe from this list: send the line "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] commit.c: Use skip_prefix() instead of starts_with()

[Junio C Hamano]

Tanay Abhra  writes:

> In record_author_date() & parse_gpg_output() ,using skip_prefix() instead of
> starts_with() is more elegant and abstracts away the details.

Avoid subjective judgement like "more elegant" when justifying your
change; you are not your own judge.

The caller of starts_with() actually can use the string that follows
the expected prefix and that is the reason why using skip_prefix()
in these places is a good idea.  There is no need to be subjective
to justify that change.

I do not think there is any more abstracting away of the details in
this change.  The updated uses a different and more suitable
abstraction than the original.

> diff --git a/commit.c b/commit.c
> index 6bf4fe0..668c703 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -548,7 +548,7 @@ define_commit_slab(author_date_slab, unsigned long);
>  static void record_author_date(struct author_date_slab *author_date,
>  struct commit *commit)
>  {
> - const char *buf, *line_end;
> + const char *buf, *line_end, *skip;
>   char *buffer = NULL;
>   struct ident_split ident;
>   char *date_end;
> @@ -566,14 +566,15 @@ static void record_author_date(struct author_date_slab 
> *author_date,
>buf;
>buf = line_end + 1) {
>   line_end = strchrnul(buf, '\n');
> - if (!starts_with(buf, "author ")) {
> + if (!(skip = skip_prefix(buf, "author "))) {

We tend to avoid assignments in conditionals.

>   if (!line_end[0] || line_end[1] == '\n')
>   return; /* end of header */
>   continue;
>   }
> + buf = skip;
>   if (split_ident_line(&ident,
> -  buf + strlen("author "),
> -  line_end - (buf + strlen("author "))) ||
> +  buf,
> +  line_end - buf) ||
>   !ident.date_begin || !ident.date_end)
>   goto fail_exit; /* malformed "author" line */
>   break;

If you give a sensible name to what 'buf + strlen("author ")' is,
then the result becomes a lot more readable compared to the
original, and I think that is what this change is about.

And "skip" is not a good name for that.  'but + strlen("author ")'
is what split_ident_line() expects its input to be split; let's
tentatively call it "ident_line" and see what the call looks like:

split_ident_line(&ident, ident_line, line_end - ident_line))

And that is what we want to see here.  It is a bit more clear than
the original that we are splitting the ident information on the line,
ident_line (you could call it ident_begin) points at the beginning
and line_end points at the end of that ident information.

Use of skip_prefix(), which I am sure you took the name of the new
variable "skip" from, is merely an implementation detail of finding
where the ident begins.  A good rule of thumb to remember is to name
things after what they are, not how you obtain them, how they are
used or what they are used for/as.

> @@ -1193,9 +1194,9 @@ static void parse_gpg_output(struct signature_check 
> *sigc)
>   for (i = 0; i < ARRAY_SIZE(sigcheck_gpg_status); i++) {
>   const char *found, *next;
>  
> - if (starts_with(buf, sigcheck_gpg_status[i].check + 1)) {
> + if (found = skip_prefix(buf, sigcheck_gpg_status[i].check + 1)) 
> {
>   /* At the very beginning of the buffer */
> - found = buf + strlen(sigcheck_gpg_status[i].check + 1);
> + ;
>   } else {
>   found = strstr(buf, sigcheck_gpg_status[i].check);
>   if (!found)

This hunk looks good.  It can be a separate patch but they are both
minor changes so it is OK to have it in a single patch.
--
To unsubscribe from this list: send the line "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] implemented strbuf_write_or_die()

[Eric Sunshine]

On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>
>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
 diff --git a/remote-curl.c b/remote-curl.c
 index 10cb011..dee8716 100644
 --- a/remote-curl.c
 +++ b/remote-curl.c
 @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
 discovery *heads)
 if (start_command(&client))
 exit(1);
 if (preamble)
 -   write_or_die(client.in, preamble->buf, preamble->len);
 +   strbuf_write_or_die(client.in, preamble);
 if (heads)
 write_or_die(client.in, heads->buf, heads->len);
>>>
>>> This should be changed. May be you can use Ctrl-F to search write_or_die().
>>> Or if you are using vim, use "/ and n" to find all.
>>
>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
>> so Faiz correctly left this invocation alone.
>
> That is a very good sign why this change is merely a code-churn and
> not an improvement, isn't it?  We know (and any strbuf user should
> know) that ->buf and ->len are the ways to learn the pointer and the
> length the strbuf holds.  Why anybody thinks it is benefitial to
> introduce another function that is _only_ for writing out strbuf and
> cannot be used to write out a plain buffer is simply beyond me.

As a potential GSoC student and newcomer to the project, Faiz would
not have known that this would be considered unwanted churn when he
chose the task from the GSoC microproject page [1]. Perhaps it would
be a good idea to retire this item from the list?

On the other hand, it did expose Faiz to the iterative code review
process on this project and gave him a taste of what would be expected
of him as a GSoC student, so the microproject achieved that important
goal, and thus wasn't an utter failure.

[1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
--
To unsubscribe from this list: send the line "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] repack: add `repack.honorpackkeep` config var

[Junio C Hamano]

Jeff King  writes:

> On Mon, Mar 03, 2014 at 10:13:47AM -0800, Junio C Hamano wrote:
>
>> > Or the flip side: if the user wants to use .keep, we should drop
>> > bitmaps. My point is that we do not know which way the user wants to
>> > go, so we should not tie the options together.
>> 
>> Hmph.  I think the short of your later explanation is "global config
>> may tell us to use bitmap, in which case we would need a way to
>> defeat that and have existing .keep honored, and it makes it easier
>> to do so if these two are kept separate, because you do not want to
>> run around and selectively disable bitmaps in these repositories.
>> We can instead do so with repack.packKeptObjects in the global
>> configuration." and I tend to agree with the reasoning.
>
> Yes. Do you need a re-roll from me? I think the last version I sent +
> the squash to tie the default to bitmap-writing makes the most sense.

I have 9e20b390 (repack: add `repack.packKeptObjects` config var,
2014-02-26); I do not recall I've squashed anything into it, though.

Do you mean this one?

Here's the interdiff for doing the fallback:

---
diff --git a/Documentation/config.txt b/Documentation/config.txt
index 3a3d84f..a8ddc7f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2139,7 +2139,9 @@ repack.usedeltabaseoffset::
 repack.packKeptObjects::
If set to true, makes `git repack` act as if
`--pack-kept-objects` was passed. See linkgit:git-repack[1] for
-   details. Defaults to false.
+   details. Defaults to `false` normally, but `true` if a bitmap
+   index is being written (either via `--write-bitmap-index` or
+   `pack.writeBitmaps`).
 
 rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
diff --git a/builtin/repack.c b/builtin/repack.c
index 49947b2..6b0b62d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,7 +9,7 @@
 #include "argv-array.h"
 
 static int delta_base_offset = 1;
-static int pack_kept_objects;
+static int pack_kept_objects = -1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -190,6 +190,9 @@ int cmd_repack(int argc, const char **argv, const char 
*prefix)
argc = parse_options(argc, argv, prefix, builtin_repack_options,
git_repack_usage, 0);
 
+   if (pack_kept_objects < 0)
+   pack_kept_objects = write_bitmap;
+
packdir = mkpathdup("%s/pack", get_object_directory());
packtmp = mkpathdup("%s/.tmp-%d-pack", packdir, (int)getpid());
 
diff --git a/t/t7700-repack.sh b/t/t7700-repack.sh
index f8431a8..b1eed5c 100755
--- a/t/t7700-repack.sh
+++ b/t/t7700-repack.sh
@@ -21,7 +21,7 @@ test_expect_success 'objects in packs marked .keep are not 
repacked' '
objsha1=$(git verify-pack -v pack-$packsha1.idx | head -n 1 |
sed -e "s/^\([0-9a-f]\{40\}\).*/\1/") &&
mv pack-* .git/objects/pack/ &&
-   git repack -A -d -l &&
+   git repack --no-pack-kept-objects -A -d -l &&
git prune-packed &&
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
@@ -35,9 +35,9 @@ test_expect_success 'objects in packs marked .keep are not 
repacked' '
test -z "$found_duplicate_object"
 '
 
-test_expect_success '--pack-kept-objects duplicates objects' '
+test_expect_success 'writing bitmaps duplicates .keep objects' '
# build on $objsha1, $packsha1, and .keep state from previous
-   git repack -Adl --pack-kept-objects &&
+   git repack -Adl &&
test_when_finished "found_duplicate_object=" &&
for p in .git/objects/pack/*.idx; do
idx=$(basename $p)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] implemented strbuf_write_or_die()

[David Kastrup]

Eric Sunshine  writes:

> As a potential GSoC student and newcomer to the project, Faiz would
> not have known that this would be considered unwanted churn when he
> chose the task from the GSoC microproject page [1]. Perhaps it would
> be a good idea to retire this item from the list?
>
> On the other hand, it did expose Faiz to the iterative code review
> process on this project and gave him a taste of what would be expected
> of him as a GSoC student, so the microproject achieved that important
> goal, and thus wasn't an utter failure.

And the microproject has the fabulous property that we can use it over
and over again to have a newcomer try committing patches: the previously
reported problem that we were running out of microprojects will not
occur when every patch is eventually going to be rejected.

Joking aside, this is a motivational disaster.  It should be retired.

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

Re: [PATCH] commit.c:record_author_date() use skip_prefix() instead of starts_with()

[Eric Sunshine]

On Mon, Mar 3, 2014 at 1:40 PM, Junio C Hamano  wrote:
> Michael Haggerty  writes:
>
>>> -if (!starts_with(buf, "author ")) {
>>> +if (!skip_prefix(buf, "author ")) {
>>
>> If this is the only change, there is not much point, is there?  How does
>> this help?  Perhaps there is some way to take advantage of the
>> difference between starts_with() and skip_prefix() to simplify the rest
>> of the function?
>
> I admit I lost track, but wasn't there a discussion to use
> starts_with/ends_with when appropriate (namely, the caller is
> absolutely not interested in what the remainder of the string is
> after skipping the prefix), moving away from skip_prefix()?  Isn't
> this change going in the wrong direction?

Yes, it would be going in the wrong direction if this was all there
was to it, but the particular GSoC microproject [1] which inspired
this (incomplete) submission expects that the potential student will
dig deeper and discover how skip_prefix() can be used to achieve
greater simplification in record_author_date() and in other places in
the same file.

[1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
--
To unsubscribe from this list: send the line "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] branch: use skip_prefix

[Junio C Hamano]

Jeff King  writes:

> On Fri, Feb 28, 2014 at 12:04:19PM +0900, Brian Gesiak wrote:
>
>> From: modocache 
>
> Both your emailed patches have this, which is due to your author name
> not matching your sending identity. You probably want to set user.name,
> or if you already have (which it looks like you might have from your
> Signed-off-by), use "git commit --amend --reset-author" to update the
> author information.
>
>> The install_branch_config function reimplemented the skip_prefix
>> function inline. Use skip_prefix function instead for brevity.
>> 
>> Signed-off-by: Brian Gesiak 
>> Reported-by: Michael Haggerty 
>
> It's a minor thing, but usually these footer lines try to follow a
> chronological order. So the report would come before the signoff (and a
> further signoff from the maintainer would go after yours).
>
>> diff --git a/branch.c b/branch.c
>> index 723a36b..e163f3c 100644
>> --- a/branch.c
>> +++ b/branch.c
>> [...]
>
> The patch itself looks OK to me.
>
> -Peff

Thanks.  Queued and pushed out on 'pu' with fixups already.
--
To unsubscribe from this list: send the line "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] repack: add `repack.honorpackkeep` config var

[Jeff King]

On Mon, Mar 03, 2014 at 11:51:06AM -0800, Junio C Hamano wrote:

> > Yes. Do you need a re-roll from me? I think the last version I sent +
> > the squash to tie the default to bitmap-writing makes the most sense.
> 
> I have 9e20b390 (repack: add `repack.packKeptObjects` config var,
> 2014-02-26); I do not recall I've squashed anything into it, though.
> 
> Do you mean this one?
> 
> Here's the interdiff for doing the fallback:
> [...]

Yes. Though I just noticed that the commit message needs updating if
that is squashed in. Here is the whole patch, with that update.

And I am dropping Vicent as the author, since I think there are now
literally zero lines of his left. ;)

-- >8 --
Subject: [PATCH] repack: add `repack.packKeptObjects` config var

The git-repack command always passes `--honor-pack-keep`
to pack-objects. This has traditionally been a good thing,
as we do not want to duplicate those objects in a new pack,
and we are not going to delete the old pack.

However, when bitmaps are in use, it is important for a full
repack to include all reachable objects, even if they may be
duplicated in a .keep pack. Otherwise, we cannot generate
the bitmaps, as the on-disk format requires the set of
objects in the pack to be fully closed.

Even if the repository does not generally have .keep files,
a simultaneous push could cause a race condition in which a
.keep file exists at the moment of a repack. The repack may
try to include those objects in one of two situations:

  1. The pushed .keep pack contains objects that were
 already in the repository (e.g., blobs due to a revert of
 an old commit).

  2. Receive-pack updates the refs, making the objects
 reachable, but before it removes the .keep file, the
 repack runs.

In either case, we may prefer to duplicate some objects in
the new, full pack, and let the next repack (after the .keep
file is cleaned up) take care of removing them.

This patch introduces both a command-line and config option
to disable the `--honor-pack-keep` option.  By default, it
is triggered when pack.writeBitmaps (or `--write-bitmap-index`
is turned on), but specifying it explicitly can override the
behavior (e.g., in cases where you prefer .keep files to
bitmaps, but only when they are present).

Note that this option just disables the pack-objects
behavior. We still leave packs with a .keep in place, as we
do not necessarily know that we have duplicated all of their
objects.

Signed-off-by: Jeff King 
---
 Documentation/config.txt |  7 +++
 Documentation/git-repack.txt |  8 
 builtin/repack.c | 13 -
 t/t7700-repack.sh| 18 +-
 4 files changed, 44 insertions(+), 2 deletions(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index becbade..a8ddc7f 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2136,6 +2136,13 @@ repack.usedeltabaseoffset::
"false" and repack. Access from old Git versions over the
native protocol are unaffected by this option.
 
+repack.packKeptObjects::
+   If set to true, makes `git repack` act as if
+   `--pack-kept-objects` was passed. See linkgit:git-repack[1] for
+   details. Defaults to `false` normally, but `true` if a bitmap
+   index is being written (either via `--write-bitmap-index` or
+   `pack.writeBitmaps`).
+
 rerere.autoupdate::
When set to true, `git-rerere` updates the index with the
resulting contents after it cleanly resolves conflicts using
diff --git a/Documentation/git-repack.txt b/Documentation/git-repack.txt
index 002cfd5..4786a78 100644
--- a/Documentation/git-repack.txt
+++ b/Documentation/git-repack.txt
@@ -117,6 +117,14 @@ other objects in that pack they already have locally.
must be able to refer to all reachable objects. This option
overrides the setting of `pack.writebitmaps`.
 
+--pack-kept-objects::
+   Include objects in `.keep` files when repacking.  Note that we
+   still do not delete `.keep` packs after `pack-objects` finishes.
+   This means that we may duplicate objects, but this makes the
+   option safe to use when there are concurrent pushes or fetches.
+   This option is generally only useful if you are writing bitmaps
+   with `-b` or `pack.writebitmaps`, as it ensures that the
+   bitmapped packfile has the necessary objects.
 
 Configuration
 -
diff --git a/builtin/repack.c b/builtin/repack.c
index 49f5857..6b0b62d 100644
--- a/builtin/repack.c
+++ b/builtin/repack.c
@@ -9,6 +9,7 @@
 #include "argv-array.h"
 
 static int delta_base_offset = 1;
+static int pack_kept_objects = -1;
 static char *packdir, *packtmp;
 
 static const char *const git_repack_usage[] = {
@@ -22,6 +23,10 @@ static int repack_config(const char *var, const char *value, 
void *cb)
delta_base_offset = git_config_bool(var, value);
return 0;
}
+   if (!strcmp(var, "repack.packkeptob

Re: RFC GSoC idea: new "git config" features

[Junio C Hamano]

Jeff King  writes:

> Most callbacks would convert to a query system in a pretty
> straightforward way, but some that have side effects might be tricky.
> Converting them all may be too large for a GSoC project, but I think you
> could do it gradually:
>
>   1. Convert the parser to read into an in-memory representation, but
>  leave git_config() as a wrapper which iterates over it.
>
>   2. Add query functions like config_string_get() above.
>
>   3. Convert callbacks to query functions one by one.
>
>   4. Eventually drop git_config().
>
> A GSoC project could take us partway through (3).

I actually discarded the "read from these config files to preparsed
structure to memory, later to be consumed by repeated calls to the
git_config() callback functions, making the only difference from the
current scheme that the preparsed structure will be reset when there
is the new 'reset to the original' definition" as obvious and
uninteresting.

This is one of these times that I find myself blessed with capable
others that can go beyond, building on top of such an idea that I
may have discarded without thinking it through, around me ;-)

Yes, the new abstraction like config__get() that can live
alongside the existing "git_config() feeds callback chain
everything" and gradually replace the latter, would be a good way
forward.  Given that we read configuration multiple times anyway for
different purposes, even without the new abstraction, the end result
might perform better if we read the files once and reused in later
calls to git_config().

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 3/3] rebase: new convenient option to edit a single commit

[Eric Sunshine]

On Sat, Mar 1, 2014 at 9:53 PM, Nguyễn Thái Ngọc Duy  wrote:
> "git rebase -e XYZ" is basically the same as
>
> EDITOR="sed -i '1s/pick XYZ/edit XYZ/' $@" \
> git rebase -i XYZ^
>
> In English, it prepares the todo list for you to edit only commit XYZ
> to save your time. The time saving is only significant when you edit a
> lot of commits separately.

Is it correct to single out only "edit" for special treatment? If
allowing "edit" on the command-line, then shouldn't command-line
"reword" also be supported? I, for one, often need to reword a commit
message (or two or three); far more frequently than I need to edit a
commit.

(This is a genuine question about perceived favoritism of "edit", as
opposed to a request to further bloat the interface.)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] submodule : Add --no-separate-git-dir option to add and update command.

[Henri GEIST]

Le lundi 03 mars 2014 à 17:45 +, Jens Lehmann a écrit :
> Am 03.03.2014 14:47, schrieb Henri GEIST:
> > This new option prevent git submodule  to clone the missing
> > submodules with the --separate-git-dir option.
> > Then the submodule will be regular repository and their gitdir will not
> > be placed in the superproject gitdir/modules directory.
> 
> And what is your motivation for this? After all submodules containing
> a .git directory are second class citizens (because they can never be
> safely removed by regular git commands).
>

I recognize most people will prefer to have the .git directory separate.
And I do not intend to make this option the default.

My reasons are:

  - As it is not clearly stated in the doc that the gitdir is separate.
The first time I have copied one module to an USB key I had a big
surprise.

  - This will not change anything for people not using it.

  - I use an other patch which I plane to send later which enable multiple
level of superproject to add a gitlink to the same submodule.
And in this case the superproject containing the separate gitdir will be
arbitrary and depend on the processing order of the
'git submodule update --recursive' command.

  - I have written this for myself and have using it since 2012 and send it in
the hope it could be useful for someone else even if it is only a few
people. But if its not the case no problem I will keep using it for myself.


> > Signed-off-by: Henri GEIST 
> > ---
> >  Documentation/git-submodule.txt |   18 --
> >  git-submodule.sh|   22 --
> >  t/t7400-submodule-basic.sh  |   12 
> >  3 files changed, 48 insertions(+), 4 deletions(-)
> > 
> > diff --git a/Documentation/git-submodule.txt 
> > b/Documentation/git-submodule.txt
> > index 21cb59a..303a475 100644
> > --- a/Documentation/git-submodule.txt
> > +++ b/Documentation/git-submodule.txt
> > @@ -10,13 +10,14 @@ SYNOPSIS
> >  
> >  [verse]
> >  'git submodule' [--quiet] add [-b ] [-f|--force] [--name ]
> > - [--reference ] [--depth ] [--]  
> > []
> > + [--reference ] [--depth ] 
> > [--no-separate-git-dir]
> > + [--]  []
> >  'git submodule' [--quiet] status [--cached] [--recursive] [--] [...]
> >  'git submodule' [--quiet] init [--] [...]
> >  'git submodule' [--quiet] deinit [-f|--force] [--] ...
> >  'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
> >   [-f|--force] [--rebase|--merge|--checkout] [--reference 
> > ]
> > - [--depth ] [--recursive] [--] [...]
> > + [--depth ] [--recursive] [--no-separate-git-dir] [--] 
> > [...]
> >  'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) 
> > ]
> >   [commit] [--] [...]
> >  'git submodule' [--quiet] foreach [--recursive] 
> > @@ -107,6 +108,10 @@ is the superproject and submodule repositories will be 
> > kept
> >  together in the same relative location, and only the
> >  superproject's URL needs to be provided: git-submodule will correctly
> >  locate the submodule using the relative URL in .gitmodules.
> > ++
> > +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> > +has normal git repository without the option `--separate-git-dir` pointing
> > +to the modules directory of the superproject gitdir.
> >  
> >  status::
> > Show the status of the submodules. This will print the SHA-1 of the
> > @@ -185,6 +190,10 @@ If the submodule is not yet initialized, and you just 
> > want to use the
> >  setting as stored in .gitmodules, you can automatically initialize the
> >  submodule with the `--init` option.
> >  +
> > +If `--no-separate-git-dir` is specified, missing submodules will be cloned
> > +has normal git repository without the option `--separate-git-dir` pointing
> > +to the modules directory of the superproject gitdir.
> > ++
> >  If `--recursive` is specified, this command will recurse into the
> >  registered submodules, and update any nested submodules within.
> >  +
> > @@ -363,6 +372,11 @@ for linkgit:git-clone[1]'s `--reference` and 
> > `--shared` options carefully.
> > clone with a history truncated to the specified number of revisions.
> > See linkgit:git-clone[1]
> >  
> > +--no-separate-git-dir::
> > +   This option is valid for add and update commands. Specify that missing
> > +   submodules should be clonned as self contain repository without a
> > +   separate gitdir placed in the modules directory of the superproject
> > +   gitdir.
> >  
> >  ...::
> > Paths to submodule(s). When specified this will restrict the command
> > diff --git a/git-submodule.sh b/git-submodule.sh
> > index a33f68d..36eaf31 100755
> > --- a/git-submodule.sh
> > +++ b/git-submodule.sh
> > @@ -5,11 +5,11 @@
> >  # Copyright (c) 2007 Lars Hjemli
> >  
> >  dashless=$(basename "$0" | sed -e 's/-/ /')
> > -USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
> > [--reference ] [--]  []

Re: [PATCH] implemented strbuf_write_or_die()

[Junio C Hamano]

Eric Sunshine  writes:

> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano  wrote:
>> Eric Sunshine  writes:
>>
>>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun  wrote:
 2014-03-01 19:21 GMT+08:00 Faiz Kothari :
> diff --git a/remote-curl.c b/remote-curl.c
> index 10cb011..dee8716 100644
> --- a/remote-curl.c
> +++ b/remote-curl.c
> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct 
> discovery *heads)
> if (start_command(&client))
> exit(1);
> if (preamble)
> -   write_or_die(client.in, preamble->buf, preamble->len);
> +   strbuf_write_or_die(client.in, preamble);
> if (heads)
> write_or_die(client.in, heads->buf, heads->len);

 This should be changed. May be you can use Ctrl-F to search write_or_die().
 Or if you are using vim, use "/ and n" to find all.
>>>
>>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,
>>> so Faiz correctly left this invocation alone.
>>
>> That is a very good sign why this change is merely a code-churn and
>> not an improvement, isn't it?  We know (and any strbuf user should
>> know) that ->buf and ->len are the ways to learn the pointer and the
>> length the strbuf holds.  Why anybody thinks it is benefitial to
>> introduce another function that is _only_ for writing out strbuf and
>> cannot be used to write out a plain buffer is simply beyond me.
>
> As a potential GSoC student and newcomer to the project, Faiz would
> not have known that this would be considered unwanted churn when he
> chose the task from the GSoC microproject page [1]. Perhaps it would
> be a good idea to retire this item from the list?

I don't think I saw this on the microproject suggestion page when I
last looked at it, and assumed that this was on the student's own
initiative.

> On the other hand, it did expose Faiz to the iterative code review
> process on this project and gave him a taste of what would be expected
> of him as a GSoC student, so the microproject achieved that important
> goal, and thus wasn't an utter failure.
>
> [1]: 
> https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md

Surely.

I would have to say that this is not a good sample exercise to
suggest to new students and I'd encourage dropping it from the list.
You could argue that it is an effective way to cull people with bad
design taste to mix suggestions to make the codebase worse and see
who picks them, but I do not think it is very fair ;-)
--
To unsubscribe from this list: send the line "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] git-config: document interactive.singlekey requires Term::ReadKey

[Simon Ruderich]

Most distributions don't require Term::ReadKey as dependency, leaving
the user to wonder why the setting doesn't work.

Signed-off-by: Simon Ruderich 
---

On Mon, Mar 03, 2014 at 10:58:58AM -0800, Junio C Hamano wrote:
> Thanks, but is it true that interactive.singlekey "requries"
> Term::ReadKey?

Yes, it requires it. The code also works fine without
Term::ReadKey, but the feature "singlekey" requires this module.
I assumed a user enabling this option would also want to use the
feature, therefore "requires" is fine IMHO.

> The implementation of prompt_single_character sub wants to use
> ReadKey, but can still let the user interact with the program by
> falling back to a cooked input when it is not available, so perhaps
> a better fix might be something like this:
>
> if (!$use_readkey) {
>   print STDERR "missing Term::ReadKey, disabling 
> interactive.singlekey\n";
> }
>
> inside the above if() that prepares $use_readkey?

Good idea. Implemented in an additional patch.

I think the documentation should also be updated (this patch) to
make it clear to a reader of the man page, that an additional
module is required, without having him to try to use the option.

> You also misspelled the package name it seems ;-)

Oops, sorry. Fixed in this reroll.

Regards
Simon

 Documentation/config.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/config.txt b/Documentation/config.txt
index 5f4d793..406a582 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1633,7 +1633,7 @@ interactive.singlekey::
linkgit:git-add[1], linkgit:git-checkout[1], linkgit:git-commit[1],
linkgit:git-reset[1], and linkgit:git-stash[1]. Note that this
setting is silently ignored if portable keystroke input
-   is not available.
+   is not available; requires the Perl module Term::ReadKey.
 
 log.abbrevCommit::
If true, makes linkgit:git-log[1], linkgit:git-show[1], and
-- 
1.9.0.11.g9a08b42

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] git-add--interactive: warn if module for interactive.singlekey is missing

[Simon Ruderich]

Suggested-by: Junio C Hamano 
Signed-off-by: Simon Ruderich 
---
 git-add--interactive.perl | 3 +++
 1 file changed, 3 insertions(+)

diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 24bb1ab..d3bca12 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -58,6 +58,9 @@ if ($repo->config_bool("interactive.singlekey")) {
Term::ReadKey->import;
$use_readkey = 1;
};
+   if (!$use_readkey) {
+   print STDERR "missing Term::ReadKey, disabling 
interactive.singlekey\n";
+   }
eval {
require Term::Cap;
my $termcap = Term::Cap->Tgetent;
-- 
1.9.0.11.g9a08b42

-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Help needed: Tests failed While replacing char array with strbuf in bulk-checkin.c

[saikrishna.sripada]

Hi ,
I am trying do complete the microproject 4, inorder to apply to GSOC.
I have made the below changes:

https://gist.github.com/anhsirksai/9334565

Post my changes compilation is succes in the source directory.
But when I ran the tests[make in t/ directory] my tests are failing 
saying


"
 free(): invalid pointer: 0x3630376532353636 ***
=== Backtrace: =
/lib/x86_64-linux-gnu/libc.so.6(+0x7eb96)[0x2b5f3b540b96]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4fb829]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x47d425]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4064ad]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405a04]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x404cbd]
/lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x2b5f3b4e376d]
/home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405109]
"

Can some one please help me with the memory allacation and 
strbuf_release()


Thanks,
--sai krishna

--
To unsubscribe from this list: send the line "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] implemented strbuf_write_or_die()

[Eric Sunshine]

On Mon, Mar 3, 2014 at 3:35 PM, Junio C Hamano  wrote:
> Eric Sunshine  writes:
>> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano  wrote:
>>> Eric Sunshine  writes:
 It's not obvious from the patch fragment, but 'heads' is not a strbuf,
 so Faiz correctly left this invocation alone.
>>>
>>> That is a very good sign why this change is merely a code-churn and
>>> not an improvement, isn't it?  We know (and any strbuf user should
>>> know) that ->buf and ->len are the ways to learn the pointer and the
>>> length the strbuf holds.  Why anybody thinks it is benefitial to
>>> introduce another function that is _only_ for writing out strbuf and
>>> cannot be used to write out a plain buffer is simply beyond me.
>>
>> As a potential GSoC student and newcomer to the project, Faiz would
>> not have known that this would be considered unwanted churn when he
>> chose the task from the GSoC microproject page [1]. Perhaps it would
>> be a good idea to retire this item from the list?
>
> I don't think I saw this on the microproject suggestion page when I
> last looked at it, and assumed that this was on the student's own
> initiative.

I also had not seen it earlier on the microprojects page and had the
same reaction until I re-checked the page and found that it had been
added [1].

The microprojects page already instructs students to indicate that a
submission is for GSoC [2] (and many have followed the advice), but
perhaps we can avoid this sort of misunderstanding in the future by
making it more explicit: for instance, tell them to add [GSoC] to the
Subject:.

[1]: 
https://github.com/git/git.github.io/commit/f314120a2b5e831459673c612a3630ad953d9954
[2]: 
https://github.com/git/git.github.io/blame/master/SoC-2014-Microprojects.md#L83

>> On the other hand, it did expose Faiz to the iterative code review
>> process on this project and gave him a taste of what would be expected
>> of him as a GSoC student, so the microproject achieved that important
>> goal, and thus wasn't an utter failure.
>>
>> [1]: 
>> https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md
>
> Surely.
>
> I would have to say that this is not a good sample exercise to
> suggest to new students and I'd encourage dropping it from the list.
> You could argue that it is an effective way to cull people with bad
> design taste to mix suggestions to make the codebase worse and see
> who picks them, but I do not think it is very fair ;-)

Agreed. The item should be dropped from the list.
--
To unsubscribe from this list: send the line "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] rebase: accept - as another way of saying HEAD~

[Junio C Hamano]

Michael Haggerty  writes:

> This might be a reason that "-NUM" is a bad idea.
>
> Or perhaps "-NUM" should fail with an error message if any of the last
> NUM commits are merges.  In that restricted scenario (which probably
> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".

That sounds like one possible way out; the opposite would be to
enable mode that preserges merges when we see any.

If "rebase" had a "--first-parent" mode that simply replays only the
commits on the first parent chain, merging the same second and later
parents without looking at their history when dealing with merge
commits, and always used that mode unless "--flatten-history" was
given, the world might have been a better place.  We cannot go there
in a single step, but we could plan such a backward incompatible
migration if we wanted to.
--
To unsubscribe from this list: send the line "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] rebase: accept - as another way of saying HEAD~

[Junio C Hamano]

Matthieu Moy  writes:

> Michael Haggerty  writes:
>
>> Or perhaps "-NUM" should fail with an error message if any of the last
>> NUM commits are merges.  In that restricted scenario (which probably
>> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".
>
> Makes sense to me. So, -NUM would actually mean "rebase the last NUM
> commits" (as well as being an alias for HEAD~NUM), but would fail when
> it does not make sense (with an error message explaining the situation
> and pointing the user to HEAD~N if this is what he wanted).
>
> This would actually be a feature for me: I often want to rebase "recent
> enough" history, and when my @{upstream} isn't well positionned,...

Could you elaborate on this a bit?  What does "isn't well
positioned" mean?  Do you mean "the upstream has advanced but there
is no reason for my topic to build on that---I'd rather want to make
sure I can view 'diff @{1} HEAD' and understand what my changes
before the rebase was"?  That is, what you really want is

git rebase -i --onto $(git merge-base @{upstream} HEAD) @{upstream}

but that is too long to type?

If it is very common (and I suspect it is), we may want to support
such a short-hand---the above does not make any sense without '-i',
but I would say with '-i' you do not want to reBASE on an updated
base most of the time.  "git rebase -i @{upstream}...HEAD" or
something?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Help needed: Tests failed While replacing char array with strbuf in bulk-checkin.c

[Eric Sunshine]

On Mon, Mar 3, 2014 at 4:11 PM, saikrishna.sripada
 wrote:
> I am trying do complete the microproject 4, inorder to apply to GSOC.
> I have made the below changes:
>
> https://gist.github.com/anhsirksai/9334565
>
> Post my changes compilation is succes in the source directory.
> But when I ran the tests[make in t/ directory] my tests are failing saying
>
> "
>  free(): invalid pointer: 0x3630376532353636 ***
> === Backtrace: =
> /lib/x86_64-linux-gnu/libc.so.6(+0x7eb96)[0x2b5f3b540b96]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4fb829]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x47d425]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x4064ad]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405a04]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x404cbd]
> /lib/x86_64-linux-gnu/libc.so.6(__libc_start_main+0xed)[0x2b5f3b4e376d]
> /home/saikrishna/Desktop/libcloud-0.14.1/sai/git/git[0x405109]
> "
>
> Can some one please help me with the memory allacation and strbuf_release()

Read the microproject text carefully and _fully_. It provides the clue
you need to understand the problem.

Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for
handling packname, and explain why this is useful. Also check if
the first argument of pack-write.c:finish_tmp_packfile() can be
made const.

If, after making a closer examination of the mentioned functions, the
problem still eludes you, ask here again.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH v4 00/14] Use ALLOC_GROW() instead of inline code

[Dmitry S. Dolzhenko]

This version differs from previous [1] the following changes:
  - added three new commits with similar changes in "builtin/mktree.c",
"cache-tree.c" and "sha1_file.c".
  - updated commit messages: "use ALLOC_GROW() in function_name()" instead of
"change function_name() to use ALLOC_GROW()"
  - updated [PATCH v2 01/11] [2] to keep code lines within 80 columns in 
"builtin/pack-objects.c"

Duy Nguyen, Michael Haggerty, Junio C Hamano, Eric Sunshine, and He Sun, 
thanks you very much for your remarks and advices

[1] http://thread.gmane.org/gmane.comp.version-control.git/242919
[2] http://thread.gmane.org/gmane.comp.version-control.git/242920

Dmitry S. Dolzhenko (14):
  builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()
  bundle.c: use ALLOC_GROW() in add_to_ref_list()
  cache-tree.c: use ALLOC_GROW() in find_subtree()
  commit.c: use ALLOC_GROW() in register_commit_graft()
  diff.c: use ALLOC_GROW()
  diffcore-rename.c: use ALLOC_GROW()
  patch-ids.c: use ALLOC_GROW() in add_commit()
  replace_object.c: use ALLOC_GROW() in register_replace_object()
  reflog-walk.c: use ALLOC_GROW()
  dir.c: use ALLOC_GROW() in create_simplify()
  attr.c: use ALLOC_GROW() in handle_attr_line()
  builtin/mktree.c: use ALLOC_GROW() in append_to_tree()
  read-cache.c: use ALLOC_GROW() in add_index_entry()
  sha1_file.c: use ALLOC_GROW() in pretend_sha1_file()

 attr.c |  7 +--
 builtin/mktree.c   |  5 +
 builtin/pack-objects.c |  9 +++--
 bundle.c   |  6 +-
 cache-tree.c   |  6 +-
 commit.c   |  8 ++--
 diff.c | 12 ++--
 diffcore-rename.c  | 12 ++--
 dir.c  |  5 +
 patch-ids.c|  5 +
 read-cache.c   |  6 +-
 reflog-walk.c  | 12 ++--
 replace_object.c   |  8 ++--
 sha1_file.c|  7 +--
 14 files changed, 21 insertions(+), 87 deletions(-)

-- 
1.8.3.2

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

[PATCH v4 08/14] replace_object.c: use ALLOC_GROW() in register_replace_object()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 replace_object.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/replace_object.c b/replace_object.c
index cdcaf8c..843deef 100644
--- a/replace_object.c
+++ b/replace_object.c
@@ -36,12 +36,8 @@ static int register_replace_object(struct replace_object 
*replace,
return 1;
}
pos = -pos - 1;
-   if (replace_object_alloc <= ++replace_object_nr) {
-   replace_object_alloc = alloc_nr(replace_object_alloc);
-   replace_object = xrealloc(replace_object,
- sizeof(*replace_object) *
- replace_object_alloc);
-   }
+   ALLOC_GROW(replace_object, replace_object_nr + 1, replace_object_alloc);
+   replace_object_nr++;
if (pos < replace_object_nr)
memmove(replace_object + pos + 1,
replace_object + pos,
-- 
1.8.3.2

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

[PATCH v4 10/14] dir.c: use ALLOC_GROW() in create_simplify()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 dir.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/dir.c b/dir.c
index 98bb50f..4ae38e4 100644
--- a/dir.c
+++ b/dir.c
@@ -1341,10 +1341,7 @@ static struct path_simplify *create_simplify(const char 
**pathspec)
 
for (nr = 0 ; ; nr++) {
const char *match;
-   if (nr >= alloc) {
-   alloc = alloc_nr(alloc);
-   simplify = xrealloc(simplify, alloc * 
sizeof(*simplify));
-   }
+   ALLOC_GROW(simplify, nr + 1, alloc);
match = *pathspec++;
if (!match)
break;
-- 
1.8.3.2

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

[PATCH v4 03/14] cache-tree.c: use ALLOC_GROW() in find_subtree()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 cache-tree.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/cache-tree.c b/cache-tree.c
index 0bbec43..30149d1 100644
--- a/cache-tree.c
+++ b/cache-tree.c
@@ -75,11 +75,7 @@ static struct cache_tree_sub *find_subtree(struct cache_tree 
*it,
return NULL;
 
pos = -pos-1;
-   if (it->subtree_alloc <= it->subtree_nr) {
-   it->subtree_alloc = alloc_nr(it->subtree_alloc);
-   it->down = xrealloc(it->down, it->subtree_alloc *
-   sizeof(*it->down));
-   }
+   ALLOC_GROW(it->down, it->subtree_nr + 1, it->subtree_alloc);
it->subtree_nr++;
 
down = xmalloc(sizeof(*down) + pathlen + 1);
-- 
1.8.3.2

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

[PATCH v4 11/14] attr.c: use ALLOC_GROW() in handle_attr_line()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 attr.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/attr.c b/attr.c
index 8d13d70..734222d 100644
--- a/attr.c
+++ b/attr.c
@@ -338,12 +338,7 @@ static void handle_attr_line(struct attr_stack *res,
a = parse_attr_line(line, src, lineno, macro_ok);
if (!a)
return;
-   if (res->alloc <= res->num_matches) {
-   res->alloc = alloc_nr(res->num_matches);
-   res->attrs = xrealloc(res->attrs,
- sizeof(struct match_attr *) *
- res->alloc);
-   }
+   ALLOC_GROW(res->attrs, res->num_matches + 1, res->alloc);
res->attrs[res->num_matches++] = a;
 }
 
-- 
1.8.3.2

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

[PATCH v4 06/14] diffcore-rename.c: use ALLOC_GROW()

[Dmitry S. Dolzhenko]

Use ALLOC_GROW() instead inline code in
locate_rename_dst() and register_rename_src()

Signed-off-by: Dmitry S. Dolzhenko 
---
 diffcore-rename.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/diffcore-rename.c b/diffcore-rename.c
index 9b4f068..fbf3272 100644
--- a/diffcore-rename.c
+++ b/diffcore-rename.c
@@ -38,11 +38,7 @@ static struct diff_rename_dst *locate_rename_dst(struct 
diff_filespec *two,
if (!insert_ok)
return NULL;
/* insert to make it at "first" */
-   if (rename_dst_alloc <= rename_dst_nr) {
-   rename_dst_alloc = alloc_nr(rename_dst_alloc);
-   rename_dst = xrealloc(rename_dst,
- rename_dst_alloc * sizeof(*rename_dst));
-   }
+   ALLOC_GROW(rename_dst, rename_dst_nr + 1, rename_dst_alloc);
rename_dst_nr++;
if (first < rename_dst_nr)
memmove(rename_dst + first + 1, rename_dst + first,
@@ -82,11 +78,7 @@ static struct diff_rename_src *register_rename_src(struct 
diff_filepair *p)
}
 
/* insert to make it at "first" */
-   if (rename_src_alloc <= rename_src_nr) {
-   rename_src_alloc = alloc_nr(rename_src_alloc);
-   rename_src = xrealloc(rename_src,
- rename_src_alloc * sizeof(*rename_src));
-   }
+   ALLOC_GROW(rename_src, rename_src_nr + 1, rename_src_alloc);
rename_src_nr++;
if (first < rename_src_nr)
memmove(rename_src + first + 1, rename_src + first,
-- 
1.8.3.2

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

[PATCH v4 05/14] diff.c: use ALLOC_GROW()

[Dmitry S. Dolzhenko]

Use ALLOC_GROW() instead inline code in
diffstat_add() and diff_q()

Signed-off-by: Dmitry S. Dolzhenko 
---
 diff.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/diff.c b/diff.c
index e800666..aebdfda 100644
--- a/diff.c
+++ b/diff.c
@@ -1361,11 +1361,7 @@ static struct diffstat_file *diffstat_add(struct 
diffstat_t *diffstat,
 {
struct diffstat_file *x;
x = xcalloc(sizeof (*x), 1);
-   if (diffstat->nr == diffstat->alloc) {
-   diffstat->alloc = alloc_nr(diffstat->alloc);
-   diffstat->files = xrealloc(diffstat->files,
-   diffstat->alloc * sizeof(x));
-   }
+   ALLOC_GROW(diffstat->files, diffstat->nr + 1, diffstat->alloc);
diffstat->files[diffstat->nr++] = x;
if (name_b) {
x->from_name = xstrdup(name_a);
@@ -3965,11 +3961,7 @@ struct diff_queue_struct diff_queued_diff;
 
 void diff_q(struct diff_queue_struct *queue, struct diff_filepair *dp)
 {
-   if (queue->alloc <= queue->nr) {
-   queue->alloc = alloc_nr(queue->alloc);
-   queue->queue = xrealloc(queue->queue,
-   sizeof(dp) * queue->alloc);
-   }
+   ALLOC_GROW(queue->queue, queue->nr + 1, queue->alloc);
queue->queue[queue->nr++] = dp;
 }
 
-- 
1.8.3.2

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

[PATCH v4 02/14] bundle.c: use ALLOC_GROW() in add_to_ref_list()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 bundle.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index e99065c..1388a3e 100644
--- a/bundle.c
+++ b/bundle.c
@@ -14,11 +14,7 @@ static const char bundle_signature[] = "# v2 git bundle\n";
 static void add_to_ref_list(const unsigned char *sha1, const char *name,
struct ref_list *list)
 {
-   if (list->nr + 1 >= list->alloc) {
-   list->alloc = alloc_nr(list->nr + 1);
-   list->list = xrealloc(list->list,
-   list->alloc * sizeof(list->list[0]));
-   }
+   ALLOC_GROW(list->list, list->nr + 1, list->alloc);
memcpy(list->list[list->nr].sha1, sha1, 20);
list->list[list->nr].name = xstrdup(name);
list->nr++;
-- 
1.8.3.2

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

[PATCH v4 01/14] builtin/pack-objects.c: use ALLOC_GROW() in check_pbase_path()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 builtin/pack-objects.c | 9 +++--
 1 file changed, 3 insertions(+), 6 deletions(-)

diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index c733379..0ffad6f 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -1213,12 +1213,9 @@ static int check_pbase_path(unsigned hash)
if (0 <= pos)
return 1;
pos = -pos - 1;
-   if (done_pbase_paths_alloc <= done_pbase_paths_num) {
-   done_pbase_paths_alloc = alloc_nr(done_pbase_paths_alloc);
-   done_pbase_paths = xrealloc(done_pbase_paths,
-   done_pbase_paths_alloc *
-   sizeof(unsigned));
-   }
+   ALLOC_GROW(done_pbase_paths,
+  done_pbase_paths_num + 1,
+  done_pbase_paths_alloc);
done_pbase_paths_num++;
if (pos < done_pbase_paths_num)
memmove(done_pbase_paths + pos + 1,
-- 
1.8.3.2

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

[PATCH v4 14/14] sha1_file.c: use ALLOC_GROW() in pretend_sha1_file()

[Dmitry S. Dolzhenko]

Helped-by: He Sun 
Signed-off-by: Dmitry S. Dolzhenko 
---
 sha1_file.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 019628a..3cb17b8 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2624,12 +2624,7 @@ int pretend_sha1_file(void *buf, unsigned long len, enum 
object_type type,
hash_sha1_file(buf, len, typename(type), sha1);
if (has_sha1_file(sha1) || find_cached_object(sha1))
return 0;
-   if (cached_object_alloc <= cached_object_nr) {
-   cached_object_alloc = alloc_nr(cached_object_alloc);
-   cached_objects = xrealloc(cached_objects,
- sizeof(*cached_objects) *
- cached_object_alloc);
-   }
+   ALLOC_GROW(cached_objects, cached_object_nr + 1, cached_object_alloc);
co = &cached_objects[cached_object_nr++];
co->size = len;
co->type = type;
-- 
1.8.3.2

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

[PATCH v4 04/14] commit.c: use ALLOC_GROW() in register_commit_graft()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 commit.c | 8 ++--
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/commit.c b/commit.c
index 6bf4fe0..e004314 100644
--- a/commit.c
+++ b/commit.c
@@ -147,12 +147,8 @@ int register_commit_graft(struct commit_graft *graft, int 
ignore_dups)
return 1;
}
pos = -pos - 1;
-   if (commit_graft_alloc <= ++commit_graft_nr) {
-   commit_graft_alloc = alloc_nr(commit_graft_alloc);
-   commit_graft = xrealloc(commit_graft,
-   sizeof(*commit_graft) *
-   commit_graft_alloc);
-   }
+   ALLOC_GROW(commit_graft, commit_graft_nr + 1, commit_graft_alloc);
+   commit_graft_nr++;
if (pos < commit_graft_nr)
memmove(commit_graft + pos + 1,
commit_graft + pos,
-- 
1.8.3.2

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

[PATCH v4 13/14] read-cache.c: use ALLOC_GROW() in add_index_entry()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 read-cache.c | 6 +-
 1 file changed, 1 insertion(+), 5 deletions(-)

diff --git a/read-cache.c b/read-cache.c
index fb440b4..cbdf954 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -990,11 +990,7 @@ int add_index_entry(struct index_state *istate, struct 
cache_entry *ce, int opti
}
 
/* Make sure the array is big enough .. */
-   if (istate->cache_nr == istate->cache_alloc) {
-   istate->cache_alloc = alloc_nr(istate->cache_alloc);
-   istate->cache = xrealloc(istate->cache,
-   istate->cache_alloc * 
sizeof(*istate->cache));
-   }
+   ALLOC_GROW(istate->cache, istate->cache_nr + 1, istate->cache_alloc);
 
/* Add it in.. */
istate->cache_nr++;
-- 
1.8.3.2

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

[PATCH v4 07/14] patch-ids.c: use ALLOC_GROW() in add_commit()

[Dmitry S. Dolzhenko]

Signed-off-by: Dmitry S. Dolzhenko 
---
 patch-ids.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/patch-ids.c b/patch-ids.c
index bc8a28f..bf81b92 100644
--- a/patch-ids.c
+++ b/patch-ids.c
@@ -83,10 +83,7 @@ static struct patch_id *add_commit(struct commit *commit,
ent = &bucket->bucket[bucket->nr++];
hashcpy(ent->patch_id, sha1);
 
-   if (ids->alloc <= ids->nr) {
-   ids->alloc = alloc_nr(ids->nr);
-   ids->table = xrealloc(ids->table, sizeof(ent) * ids->alloc);
-   }
+   ALLOC_GROW(ids->table, ids->nr + 1, ids->alloc);
if (pos < ids->nr)
memmove(ids->table + pos + 1, ids->table + pos,
sizeof(ent) * (ids->nr - pos));
-- 
1.8.3.2

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

Re: [PATCH 2/3] rebase: accept - as another way of saying HEAD~

[Matthieu Moy]

Junio C Hamano  writes:

> Matthieu Moy  writes:
>
>> Michael Haggerty  writes:
>>
>>> Or perhaps "-NUM" should fail with an error message if any of the last
>>> NUM commits are merges.  In that restricted scenario (which probably
>>> accounts for 99% of rebases), "-NUM" is equivalent to "HEAD~NUM".
>>
>> Makes sense to me. So, -NUM would actually mean "rebase the last NUM
>> commits" (as well as being an alias for HEAD~NUM), but would fail when
>> it does not make sense (with an error message explaining the situation
>> and pointing the user to HEAD~N if this is what he wanted).
>>
>> This would actually be a feature for me: I often want to rebase "recent
>> enough" history, and when my @{upstream} isn't well positionned,...
>
> Could you elaborate on this a bit?  What does "isn't well
> positioned" mean?

The most common case is when @{upstream} is not positionned at all,
because I'm on a temporary branch on which I did not configure it. The
other case is when I did a manual merge of a branch other than upstream.

As I said in another message, there would probably be cleaner solutions,
but the trial and error one does not work that bad.

-- 
Matthieu Moy
http://www-verimag.imag.fr/~moy/
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: My advice for GSoC applicants

[Junio C Hamano]

Michael Haggerty  writes:

> Based on my experience so far as a first-time Google Summer of Code
> mentor, I just wrote a blog article containing some hopefully useful
> advice for students applying to the program.  Please note that this is
> my personal opinion only and doesn't necessarily reflect the views of
> the Git/libgit2 projects as a whole.
>
> My secret tip for GSoC success
>
> http://softwareswirl.blogspot.com/2014/03/my-secret-tip-for-gsoc-success.html

Thanks for writing this.

Also thanks for the MicroProject approach to introduce potential
students and the community.

Multiple students seem to be hitting the same microprojects (aka "we
are running out of micros"), which might be a bit unfortunate.  I
think the original plan might have been that for a student candidate
to pass, his-or-her patch must hit my tree and queued somewhere, but
with these duplicates I do not think it is fair to disqualify those
who interacted with reviewers well but solved an already solved
micro.

Even with the duplicates I think we are learning how well each
student respond to reviews (better ones even seem to pick up lessons
from reviews on others' threads that tackle micros different from
their own) and what his-or-her general cognitive ability is.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [GSoC14][RFC] Proposal Draft: Refactor tempfile handling

[Jeff King]

On Sun, Mar 02, 2014 at 06:04:39AM +0900, Brian Gesiak wrote:

> My name is Brian Gesiak. I'm a research student at the University of
> Tokyo, and I'm hoping to participate in this year's Google Summer of
> Code by contributing to Git. I'm a longtime user, first-time
> contributor--some of you may have noticed my "microproject"
> patches.[1][2]

Yes, we did notice them. Thanks, and welcome. :)

> The ideas page points out that while lock files are closed and
> unlinked[3] when the program exits[4], object pack files implement
> their own brand of temp file creation and deletion. This
> implementation doesn't share the same guarantees as lock files--it is
> possible that the program terminates before the temp file is
> unlinked.[5]
> 
> Lock file references are stored in a linked list. When the program
> exits, this list is traversed and each file is closed and unlinked. It
> seems to me that this mechanism is appropriate for temp files in
> general, not just lock files. Thus, my proposal would be to extract
> this logic into a separate module--tempfile.h, perhaps. Lock and
> object files would share the tempfile implementation.
> 
> That is, both object and lock temp files would be stored in a linked
> list, and all of these would be deleted at program exit.

Yes, I think this is definitely the right way to go. We should be able
to unify the tempfile handling for all of git. Once the logic is
extracted into a nice API, there are several other places that can use
it, too:

  - the external diff code creates tempfiles and uses its own cleanup
routines

  - the shallow_XX tempfiles (these are not cleaned right now,
though I sent a patch recently for them to do their own cleanup)

Those are just off the top of my head. There may be other spots, too.

It is worth thinking in your proposal about some of the things that the
API will want to handle. What are the mismatches in how lockfiles and
object files are handled? E.g., how do we finalize them into place?
How should the API be designed to minimize race conditions (e.g., if we
get a signal delivered while we are committing or cleaning up a file)?

> Please let me know if this seems like it would make for an interesting
> proposal, or if perhaps there is something I am overlooking. Any
> feedback at all would be appreciated. Thank you!

You definitely have a grasp of what the project is aiming for, and which
areas need to be touched.

-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 v4 09/14] reflog-walk.c: use ALLOC_GROW()

[Dmitry S. Dolzhenko]

Use ALLOC_GROW() instead inline code in
add_commit_info() and read_one_reflog()

Signed-off-by: Dmitry S. Dolzhenko 
---
 reflog-walk.c | 12 ++--
 1 file changed, 2 insertions(+), 10 deletions(-)

diff --git a/reflog-walk.c b/reflog-walk.c
index b2fbdb2..2899729 100644
--- a/reflog-walk.c
+++ b/reflog-walk.c
@@ -26,11 +26,7 @@ static int read_one_reflog(unsigned char *osha1, unsigned 
char *nsha1,
struct complete_reflogs *array = cb_data;
struct reflog_info *item;
 
-   if (array->nr >= array->alloc) {
-   array->alloc = alloc_nr(array->nr + 1);
-   array->items = xrealloc(array->items, array->alloc *
-   sizeof(struct reflog_info));
-   }
+   ALLOC_GROW(array->items, array->nr + 1, array->alloc);
item = array->items + array->nr;
memcpy(item->osha1, osha1, 20);
memcpy(item->nsha1, nsha1, 20);
@@ -114,11 +110,7 @@ static void add_commit_info(struct commit *commit, void 
*util,
struct commit_info_lifo *lifo)
 {
struct commit_info *info;
-   if (lifo->nr >= lifo->alloc) {
-   lifo->alloc = alloc_nr(lifo->nr + 1);
-   lifo->items = xrealloc(lifo->items,
-   lifo->alloc * sizeof(struct commit_info));
-   }
+   ALLOC_GROW(lifo->items, lifo->nr + 1, lifo->alloc);
info = lifo->items + lifo->nr;
info->commit = commit;
info->util = util;
-- 
1.8.3.2

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

[PATCH v4 12/14] builtin/mktree.c: use ALLOC_GROW() in append_to_tree()

[Dmitry S. Dolzhenko]

Helped-by: He Sun 
Signed-off-by: Dmitry S. Dolzhenko 
---
 builtin/mktree.c | 5 +
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/mktree.c b/builtin/mktree.c
index f92ba40..a964d6b 100644
--- a/builtin/mktree.c
+++ b/builtin/mktree.c
@@ -23,10 +23,7 @@ static void append_to_tree(unsigned mode, unsigned char 
*sha1, char *path)
if (strchr(path, '/'))
die("path %s contains slash", path);
 
-   if (alloc <= used) {
-   alloc = alloc_nr(used);
-   entries = xrealloc(entries, sizeof(*entries) * alloc);
-   }
+   ALLOC_GROW(entries, used + 1, alloc);
ent = entries[used++] = xmalloc(sizeof(**entries) + len + 1);
ent->mode = mode;
ent->len = len;
-- 
1.8.3.2

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

Re: [PATCH v3] skip_prefix: rewrite so that prefix is scanned once

[Junio C Hamano]

Junio C Hamano  writes:

> Siddharth Goel  writes:
>
>> Helped-by: Eric Sunshine 
>> Signed-off-by: Siddharth Goel 
>> ---
>> Added a space after colon in the subject as compared to previous 
>> patch [PATCH v2].
>>
>> [PATCH v2]: http://thread.gmane.org/gmane.comp.version-control.git/243150
>
> Whenever you see "Change", "Rewrite", etc. in the subject of a patch
> that touches existing code, think twice.  The subject line is a
> scarce real estate to be wasted on a noiseword that carries no real
> information, and we already know a patch that touches existing code
> changes or rewrites things.
>
> Subject: [PATCH v3] skip_prefix: scan prefix only once
>
> perhaps?
>
>>  git-compat-util.h | 7 +--
>>  1 file changed, 5 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index 614a5e9..550dce3 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -357,8 +357,11 @@ extern int suffixcmp(const char *str, const char 
>> *suffix);
>>  
>>  static inline const char *skip_prefix(const char *str, const char *prefix)
>>  {
>> -size_t len = strlen(prefix);
>> -return strncmp(str, prefix, len) ? NULL : str + len;
>> +while (*prefix != '\0' && *str == *prefix) {
>> +str++;
>> +prefix++;
>> +}
>> +return (*prefix == '\0' ? str : NULL);
>
> Unlike another patch I saw the other day on the same topic, this
> checks *prefix twice for the last round, even though I think this
> one is probably slightly easier to read.  I dunno.

That is, something like this instead.  After looking at it again, I
do not think it is less readable than the above.

 git-compat-util.h | 10 --
 1 file changed, 8 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index cbd86c3..68ffaef 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,14 @@ extern int suffixcmp(const char *str, const char *suffix);
 
 static inline const char *skip_prefix(const char *str, const char *prefix)
 {
-   size_t len = strlen(prefix);
-   return strncmp(str, prefix, len) ? NULL : str + len;
+   while (1) {
+   if (!*prefix)
+   return str;
+   if (*str != *prefix)
+   return NULL;
+   prefix++;
+   str++;
+   }
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html