[PATCH] rewrite bulk-checkin.c:finish_bulk_checkin() using strbuf

[Faiz Kothari]

Signed-off-by: Faiz Kothari 

Notes:
I finally got what's happening, and why the errors were caused.
packname is supposed to contain the complete path to the .pack file.
Packs are stored as /path/to/.pack which I overlooked earlier.
After inspecting what is happening in pack-write.c:finish_tmp_packfile()
which indirectly modifies packname by appending the SHA1 and ".pack" to 
packname
This is happening in these code snippets:
char *end_of_name_prefix = strrchr(name_buffer, 0);

and later
sprintf(end_of_name_prefix, "%s.pack", sha1_to_hex(sha1));

name_buffer is packname.buf
Using const for the first argument of pack-write.c:finish_tmp_packfile()
doesnot raise any compile time warning or error and not any runtime errors,
since the packname.buf is on heap and has extra space to which more char 
can be written.
If this was not the case,
for e.g. passing a constant string and modifying it.
This will result in a segmentation fault.
---
 bulk-checkin.c |8 +---
 pack-write.c   |2 +-
 pack.h |2 +-
 3 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..bbdf1ec 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,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)
@@ -42,9 +42,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 state->offset);
close(fd);
}
+   strbuf_addf(&packname, "%s/pack/pack-", get_object_directory());
+   strbuf_grow(&packname, 40 + 5);
 
-   sprintf(packname, "%s/pack/pack-", get_object_directory());
-   finish_tmp_packfile(packname, state->pack_tmp_name,
+   finish_tmp_packfile(packname.buf, state->pack_tmp_name,
state->written, state->nr_written,
&state->pack_idx_opts, sha1);
for (i = 0; i < state->nr_written; i++)
@@ -53,6 +54,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 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/pack-write.c b/pack-write.c
index 605d01b..ac38867 100644
--- a/pack-write.c
+++ b/pack-write.c
@@ -336,7 +336,7 @@ struct sha1file *create_tmp_packfile(char **pack_tmp_name)
return sha1fd(fd, *pack_tmp_name);
 }
 
-void finish_tmp_packfile(char *name_buffer,
+void finish_tmp_packfile(const char *name_buffer,
 const char *pack_tmp_name,
 struct pack_idx_entry **written_list,
 uint32_t nr_written,
diff --git a/pack.h b/pack.h
index 12d9516..3b9e033 100644
--- a/pack.h
+++ b/pack.h
@@ -91,6 +91,6 @@ extern int encode_in_pack_object_header(enum object_type, 
uintmax_t, unsigned ch
 extern int read_pack_header(int fd, struct pack_header *);
 
 extern struct sha1file *create_tmp_packfile(char **pack_tmp_name);
-extern void finish_tmp_packfile(char *name_buffer, const char *pack_tmp_name, 
struct pack_idx_entry **written_list, uint32_t nr_written, struct 
pack_idx_option *pack_idx_opts, unsigned char sha1[]);
+extern void finish_tmp_packfile(const char *name_buffer, const char 
*pack_tmp_name, struct pack_idx_entry **written_list, uint32_t nr_written, 
struct pack_idx_option *pack_idx_opts, unsigned char sha1[]);
 
 #endif
-- 
1.7.9.5

--
To unsubscribe from this list: send the line "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] t3200-branch: test setting branch as own upstream

[Johannes Sixt]

Am 2/28/2014 8:14, schrieb Jeff King:
> I didn't think we bothered to make "sh -x" work robustly. I don't mind
> if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
> problem spots.
> 
> Hmm. Looks like it is only a problem if you are calling a shell function
> (since it is the shell function's trace output you are seeing). So this
> test would be OK as-is, but testing for an error, like:
> 
>   test_must_fail git branch -u foo foo 2>stderr
> 
> would not be, because we see the trace from test_must_fail. So some of
> the callsites found by my grep are actually probably fine.

Yeah, your assessment is correct: only shell function output is affected.

Some time (years?) ago, I used to run the tests on Windows with sh -x,
redirected to a log file to be able to trace intermittent failures. It was
distracting to find many false positives. Today, I don't do that anymore,
and it is not a big deal for me, just like for anybody else ;-)

Consider the topic settled.

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

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

[Jeff King]

On Fri, Feb 28, 2014 at 02:34:16PM +0700, Duy Nguyen wrote:

> > Yeah, I do this a lot, too.  The interface you propose makes sense to
> > me, though I'm not sure how much I would use it, as I often do not know
> > the specifier of the commit I want to change (was it "HEAD~3 or
> > HEAD~4?"). I guess using ":/" could make that easier.
> 
> In my case, I just copy/paste the commit ID from "git log -lp". I
> think :/ already works with rebase..

I think it should work. I just meant "I will have to get in the habit of
starting to use :/". :)

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

[Duy Nguyen]

On Fri, Feb 28, 2014 at 1:58 PM, Jeff King  wrote:
> On Thu, Feb 27, 2014 at 08:01:18PM +0700, Nguyễn Thái Ngọc Duy wrote:
>
>> I find myself often do "git rebase -i xxx" and replace one "pick" line
>> with "edit" to amend just one commit when I see something I don't like
>> in that commit. This happens often while cleaning up a series. This
>> automates the "replace" step so it sends me straight to that commit.
>
> Yeah, I do this a lot, too.  The interface you propose makes sense to
> me, though I'm not sure how much I would use it, as I often do not know
> the specifier of the commit I want to change (was it "HEAD~3 or
> HEAD~4?"). I guess using ":/" could make that easier.

In my case, I just copy/paste the commit ID from "git log -lp". I
think :/ already works with rebase..

>
> One comment on the option name:
>
>> +1,edit-one!generate todo list to edit this commit
>
> I'd expect "-$n" to mean "rebase the last $n commits" (as opposed to
> everything not in the upstream). That does not work currently, of
> course, but:
>
>   1. It has the potential to confuse people who read it, since it's
>  unlike what "-1" means in most of the rest of git.
>
>   2. It closes the door if we want to support "-$n" in the future.

I really like to do "git rebase -5" == "git rebase HEAD~5" but never
gotten around do make it so. "-1/--edit-one" was chosen without much
thought. Will change it to something else.
-- 
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 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

> Hmm. Looks like it is only a problem if you are calling a shell function
> (since it is the shell function's trace output you are seeing). So this
> test would be OK as-is

Indeed, this test passes when run locally, even using "sh -x".

I would be in favor of using test_i18ngrep, but it seems like this
test file overwhelmingly uses test_(i18n)cmp, even when inspecting
stderr output. Making double-sure that all tests pass when run with
"sh -x" seems like a larger endeavor.

Of course, I'd be happy to submit several patches if there's support
for such a change. But as Peff points out it will be a large diff.

- Brian Gesiak

On Fri, Feb 28, 2014 at 4:26 PM, Jeff King  wrote:
> On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote:
>
>> I didn't think we bothered to make "sh -x" work robustly. I don't mind
>> if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
>> problem spots.
>
> Just for fun:
>
>   cd t
>   make SHELL_PATH="sh -x" prove
>
> causes 326 test failures across 43 scripts. That's slightly misleading,
> because 200 of the failures are all in t0008, and updating one function
> would probably clear up all of them.
>
> -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 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 02:14:01AM -0500, Jeff King wrote:

> I didn't think we bothered to make "sh -x" work robustly. I don't mind
> if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
> problem spots.

Just for fun:

  cd t
  make SHELL_PATH="sh -x" prove

causes 326 test failures across 43 scripts. That's slightly misleading,
because 200 of the failures are all in t0008, and updating one function
would probably clear up all of them.

-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 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 07:55:25AM +0100, Johannes Sixt wrote:

> > This should use test_i18ncmp, as the string you are matching is
> > internationalized.
> 
> More generally, stderr output shouldn't be tested with test_cmp or
> test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that
> when you run the test with 'sh -x t3200* -v -i', the trace output is also
> in stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra
> text.

I didn't think we bothered to make "sh -x" work robustly. I don't mind
if we do, but "git grep -E 'test_(i18n)?cmp .*err" shows many potential
problem spots.

Hmm. Looks like it is only a problem if you are calling a shell function
(since it is the shell function's trace output you are seeing). So this
test would be OK as-is, but testing for an error, like:

  test_must_fail git branch -u foo foo 2>stderr

would not be, because we see the trace from test_must_fail. So some of
the callsites found by my grep are actually probably fine.

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

[Jeff King]

On Thu, Feb 27, 2014 at 08:01:18PM +0700, Nguyễn Thái Ngọc Duy wrote:

> I find myself often do "git rebase -i xxx" and replace one "pick" line
> with "edit" to amend just one commit when I see something I don't like
> in that commit. This happens often while cleaning up a series. This
> automates the "replace" step so it sends me straight to that commit.

Yeah, I do this a lot, too.  The interface you propose makes sense to
me, though I'm not sure how much I would use it, as I often do not know
the specifier of the commit I want to change (was it "HEAD~3 or
HEAD~4?"). I guess using ":/" could make that easier.

One comment on the option name:

> +1,edit-one!generate todo list to edit this commit

I'd expect "-$n" to mean "rebase the last $n commits" (as opposed to
everything not in the upstream). That does not work currently, of
course, but:

  1. It has the potential to confuse people who read it, since it's
 unlike what "-1" means in most of the rest of git.

  2. It closes the door if we want to support "-$n" in the future.

-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 1/2] t3200-branch: test setting branch as own upstream

[Johannes Sixt]

Am 2/28/2014 6:37, schrieb Jeff King:
> On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote:
> 
>> No test asserts that "git branch -u refs/heads/my-branch my-branch"
>> emits a warning. Add a test that does so.
> 
> For an operation like "git branch foo origin" where setting up the
> tracking is a side effect, a warning makes sense. But the sole purpose
> of the command above is to set the upstream, and we didn't do it; should
> this warning actually be upgraded to an error?
> 
>> +test_expect_success '--set-upstream-to shows warning if used to set branch 
>> as own upstream' '
>> +git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
>> +cat >expected <> +warning: Not setting branch my13 as its own upstream.
>> +EOF
>> +test_cmp expected actual
>> +'
> 
> This should use test_i18ncmp, as the string you are matching is
> internationalized.

More generally, stderr output shouldn't be tested with test_cmp or
test_i18ncmp at all, but with grep and test_i18ngrep. The reason is that
when you run the test with 'sh -x t3200* -v -i', the trace output is also
in stderr, and the test_cmp/test_i18ncmp fails due to the unexpected extra
text.

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

[PATCH] gitk: replace SHA1 entry field on keyboard paste

[Ilya Bobyr]

We already replace old SHA with the clipboard content for the mouse
paste event.  It seems reasonable to do the same when pasting from
keyboard.

Signed-off-by: Ilya Bobyr 
---
 gitk-git/gitk |1 +
 1 files changed, 1 insertions(+), 0 deletions(-)

diff --git a/gitk-git/gitk b/gitk-git/gitk
index 90764e8..2f58bcf 100755
--- a/gitk-git/gitk
+++ b/gitk-git/gitk
@@ -2585,6 +2585,7 @@ proc makewindow {} {
 bind $fstring  {dofind 1 1}
 bind $sha1entry  {gotocommit; break}
 bind $sha1entry <> clearsha1
+bind $sha1entry <> clearsha1
 bind $cflist <1> {sel_flist %W %x %y; break}
 bind $cflist  {sel_flist %W %x %y; break}
 bind $cflist  {treeclick %W %x %y}
-- 
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 v2 2/2] branch: use skip_prefix

[Brian Gesiak]

The install_branch_config function reimplemented the skip_prefix
function inline. Use skip_prefix function instead for brevity.

Reported-by: Michael Haggerty 
Signed-off-by: Brian Gesiak 
---
 branch.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..e163f3c 100644
--- a/branch.c
+++ b/branch.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
 #include "cache.h"
 #include "branch.h"
 #include "refs.h"
@@ -49,12 +50,11 @@ 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/");
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
 
-   if (remote_is_branch
+   if (shortname
&& !strcmp(local, shortname)
&& !origin) {
warning(_("Not setting branch %s as its own upstream."),
@@ -77,29 +77,29 @@ void install_branch_config(int flag, const char *local, 
const char *origin, cons
strbuf_release(&key);
 
if (flag & BRANCH_CONFIG_VERBOSE) {
-   if (remote_is_branch && origin)
+   if (shortname && origin)
printf_ln(rebasing ?
  _("Branch %s set up to track remote branch %s 
from %s by rebasing.") :
  _("Branch %s set up to track remote branch %s 
from %s."),
  local, shortname, origin);
-   else if (remote_is_branch && !origin)
+   else if (shortname && !origin)
printf_ln(rebasing ?
  _("Branch %s set up to track local branch %s 
by rebasing.") :
  _("Branch %s set up to track local branch 
%s."),
  local, shortname);
-   else if (!remote_is_branch && origin)
+   else if (!shortname && origin)
printf_ln(rebasing ?
  _("Branch %s set up to track remote ref %s by 
rebasing.") :
  _("Branch %s set up to track remote ref %s."),
  local, remote);
-   else if (!remote_is_branch && !origin)
+   else if (!shortname && !origin)
printf_ln(rebasing ?
  _("Branch %s set up to track local ref %s by 
rebasing.") :
  _("Branch %s set up to track local ref %s."),
  local, remote);
else
-   die("BUG: impossible combination of %d and %p",
-   remote_is_branch, origin);
+   die("BUG: impossible combination of %p and %p",
+   shortname, origin);
}
 }
 
-- 
1.8.3.4 (Apple Git-47)

--
To unsubscribe from this list: send the line "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

[Johannes Sixt]

Am 2/28/2014 0:38, schrieb Lee Hopkins:
>> If I understand the issue correctly, the problem is that packed-refs
>> are always case-sensitive, even if core.ignorecase=true. OTOH,

core.ignorecase is intended to affect filenames of the worktree, not
anything else, BTW.

>> checking / updating _unpacked_ refs on a case-insensitive file system
>> is naturally case-insensitive. So wouldn't it be a better workaround
>> to disallow packed refs (i.e. 'git config gc.packrefs false')?
> 
> You are correct, the issue boils down to mixing the usage of 
> packed-refs and loose refs on case insensitive file systems. So either 
> always using packed-refs or always using loose refs would take care of 
> the problem. Based Michael Haggerty's response, it seems that always 
> using loose refs would be a better workaround.

So, everybody on a case-insensitive file system should pay the price even
if they do not need the "feature"? No way.

If you are on a case-insensitive filesystem, or work on a cross-platform
project, ensure that you avoid ambiguous refs. Problem solved.

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

[PATCH v2 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

No test asserts that "git branch -u refs/heads/my-branch my-branch"
emits a warning. Add a test that does so.

Signed-off-by: Brian Gesiak 
---
 t/t3200-branch.sh | 8 
 1 file changed, 8 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fcdb867..6164126 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -507,6 +507,14 @@ EOF
test_cmp expected actual
 '
 
+test_expect_success '--set-upstream-to shows warning if used to set branch as 
own upstream' '
+   git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
+   cat >expected 

Re: [PATCH 1/2] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 03:17:28PM +0900, Brian Gesiak wrote:

> > For an operation like "git branch foo origin" where setting up the
> > tracking is a side effect, a warning makes sense. But the sole purpose
> > of the command above is to set the upstream, and we didn't do it; should
> > this warning actually be upgraded to an error?
> 
> I agree. I originally wrote the test using test_expect_failure--imagine my
> surprise when the exit status was 0, despite the fact that the upstream wasn't
> set!

For reference, you don't want test_expect_failure here; it is about "we
want this to succeed, but git is currently buggy and we know it, so mark
it as a failing test". You want test_must_fail to indicate a command
inside a test that must exit non-zero:

  test_expect_success 'pointing upstream to itself fails' '
  test_must_fail git branch -u ...
  '

I notice that the warning comes from install_branch_config, which gets
used both for "branch -u", but also in the "side effect" case I
mentioned above. Is it possible to trigger this as part of such a case?
I think maybe "git branch -f --track foo foo" would do it. If so, we
should perhaps include a test that it does not break if we upgrade the
"-u" case to an error.

> Patch is on the way, just waiting for the tests to complete. Thanks
> for pointing that out! Also, sorry if it's in the Makefile somewhere,
> but is there an easy way to run just a single test file in the t
> directory?

You can run "./t-sh" explicitly.

-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 1/2] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

> For an operation like "git branch foo origin" where setting up the
> tracking is a side effect, a warning makes sense. But the sole purpose
> of the command above is to set the upstream, and we didn't do it; should
> this warning actually be upgraded to an error?

I agree. I originally wrote the test using test_expect_failure--imagine my
surprise when the exit status was 0, despite the fact that the upstream wasn't
set!

> This should use test_i18ncmp, as the string you are matching is
> internationalized.

Patch is on the way, just waiting for the tests to complete. Thanks for pointing
that out! Also, sorry if it's in the Makefile somewhere, but is there
an easy way
to run just a single test file in the t directory?

- Brian Gesiak
--
To unsubscribe from this list: send the line "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] Rewrite git-compat-util.h:skip_prefix() as a loop

[Jeff King]

On Thu, Feb 27, 2014 at 09:33:45PM +0100, David Kastrup wrote:

> >> diff --git a/git-compat-util.h b/git-compat-util.h
> >> index cbd86c3..4daa6cf 100644
> >> --- a/git-compat-util.h
> >> +++ b/git-compat-util.h
> >> @@ -357,8 +357,8 @@ 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++ );
> >> +return *prefix == '\0' ? str : NULL;
> >
> > Documentation/CodingGuidelines?
> 
> Mostly relevant for tabification here, not helping much otherwise.  In
> particular, does not contain the advice "empty statements should appear
> on a line of their own" which would help with readability here.

Also whitespace in the "while", which I could not find mentioned in
CodingGuidelines either. Maybe:

-- >8 --
Subject: [PATCH] CodingGuidelines: mention C whitespace rules

We are fairly consistent about these, so most are covered by
"follow existing style", but it doesn't hurt to be explicit.

Signed-off-by: Jeff King 
---
 Documentation/CodingGuidelines | 11 +++
 1 file changed, 11 insertions(+)

diff --git a/Documentation/CodingGuidelines b/Documentation/CodingGuidelines
index ef67b53..ed432a8 100644
--- a/Documentation/CodingGuidelines
+++ b/Documentation/CodingGuidelines
@@ -126,6 +126,17 @@ For C programs:
"char * string".  This makes it easier to understand code
like "char *string, c;".
 
+ - Use whitespace around operators and keywords, but not inside
+   parentheses and not around functions. So:
+
+while (condition)
+   func(bar + 1);
+
+   and not:
+
+while( condition )
+   func (bar+1);
+
  - We avoid using braces unnecessarily.  I.e.
 
if (bla) {
-- 
1.8.5.2.500.g8060133

--
To unsubscribe from this list: send the line "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

[Jeff King]

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
--
To unsubscribe from this list: send the line "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] t3200-branch: test setting branch as own upstream

[Jeff King]

On Fri, Feb 28, 2014 at 12:04:18PM +0900, Brian Gesiak wrote:

> No test asserts that "git branch -u refs/heads/my-branch my-branch"
> emits a warning. Add a test that does so.

For an operation like "git branch foo origin" where setting up the
tracking is a side effect, a warning makes sense. But the sole purpose
of the command above is to set the upstream, and we didn't do it; should
this warning actually be upgraded to an error?

> +test_expect_success '--set-upstream-to shows warning if used to set branch 
> as own upstream' '
> + git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
> + cat >expected < +warning: Not setting branch my13 as its own upstream.
> +EOF
> + test_cmp expected actual
> +'

This should use test_i18ncmp, as the string you are matching is
internationalized.

-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] Problem in bulk-checkin.c:finish_bulk_checkin() Unable to fix

[Eric Sunshine]

On Thu, Feb 27, 2014 at 7:04 PM, Faiz Kothari  wrote:
> Signed-off-by: Faiz Kothari 
> ---
> Compiles without errors.
> Fails in test t/t1050-large.sh ,fails 12/15 tests. Dumps memory map and 
> backtrace.
> Somewhere its not able to free(): invalid pointer.
> Please somone pointout where I am doing it wrong.
> Help is really appreciated.
> Thanks.

Hint: Pay close attention to the second sentence of the microproject
you've undertaken:

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.

It's the clue to understanding the crash.

>  bulk-checkin.c |8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 118c625..c76cd6b 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -23,7 +23,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)
> @@ -42,9 +42,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
> *state)
>  state->offset);
> 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.buf, state->pack_tmp_name,
> state->written, state->nr_written,
> &state->pack_idx_opts, sha1);
> for (i = 0; i < state->nr_written; i++)
> @@ -53,6 +52,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
> *state)
>  clear_exit:
> free(state->written);
> memset(state, 0, sizeof(*state));
> +   strbuf_release(&packname);
>
> /* Make objects we just wrote available to ourselves */
> reprepare_packed_git();
> --
> 1.7.9.5
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: `git stash pop` UX Problem

[Brandon McCaig]

Stephan:

On Thu, Feb 27, 2014 at 9:57 PM, Stephen Leake
 wrote:
> You might be adding other files for other reasons. But if you add a file
> that does resolve a conflict caused by 'git stash pop', it is not
> guessing.

Staging a file doesn't tell git that you resolved a conflict. Git will
happily accept a blob full of conflict markers. Git doesn't know the
difference. Git expects the user to know what is right. The user has
the freedom to manipulate the index as they see fit, which means both
adding and removing from it anytime they wish.

> So "git add" and "git stash *" are lower level tools; to get the effect
> we are asking for, we should use a front-end (which is why I'm writing
> one for Emacs :).

You *can* use a front end, but I would argue that you shouldn't
necessarily. Most third-party front ends only serve to confuse users.
In general, they only cause problems and encourage ignorance.

Git is a very pure system. It doesn't impose too may rules on you. It
basically just gives you the tools that you need to work within the
system and gets out of your way. It is up to the user to learn how to
assemble those tools for good (and many front ends exist to help;
sometimes arguably too many as it is, such as git-pull(1) for
example).

 This isn't a case of the API being wrong. This is a case of PEBKAC,
IMO. Maybe the API can be a little bit more verbose in assisting the
user to understand what has happened and what sensible options there
are, but we should avoid catering to newbies too much. You should only
be a newbie for a short time. After that you should begin to learn the
API. Hand holding at that point would be noise (as it would for most
of us here, I imagine). The worst kind of "hand holding" is the kind
that imposes rules on you that aren't universal. Dropping the stash
after adding all changes to the index after a failed pop is not
universal. At most, git stash pop should give the user a bit more
guidance to understand what the situation is. At least, the user
should RTFM and learn to use the tools. And that might involve some
mistakes, but you learn from mistakes. At least Git does a good job of
making it easy to recover from most of your mistakes. The proposed
change to git-add takes away one of those safety nets.

Git isn't always the easiest thing to wrap your head around, but I
have found that once you have wrapped your head around it Git is the
easiest thing to get the job done the way you want. I consider the
learning curve a strength of Git. Which isn't to say that there isn't
room for improvement, but when proposing improvements we should try to
make sure that they actually make things universally better.

Regards,


-- 
Brandon McCaig  
Castopulence Software 
Blog 
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
--
To unsubscribe from this list: send the line "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] branch: use skip_prefix

[Brian Gesiak]

From: modocache 

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 
---
 branch.c | 18 +-
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..e163f3c 100644
--- a/branch.c
+++ b/branch.c
@@ -1,3 +1,4 @@
+#include "git-compat-util.h"
 #include "cache.h"
 #include "branch.h"
 #include "refs.h"
@@ -49,12 +50,11 @@ 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/");
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
 
-   if (remote_is_branch
+   if (shortname
&& !strcmp(local, shortname)
&& !origin) {
warning(_("Not setting branch %s as its own upstream."),
@@ -77,29 +77,29 @@ void install_branch_config(int flag, const char *local, 
const char *origin, cons
strbuf_release(&key);
 
if (flag & BRANCH_CONFIG_VERBOSE) {
-   if (remote_is_branch && origin)
+   if (shortname && origin)
printf_ln(rebasing ?
  _("Branch %s set up to track remote branch %s 
from %s by rebasing.") :
  _("Branch %s set up to track remote branch %s 
from %s."),
  local, shortname, origin);
-   else if (remote_is_branch && !origin)
+   else if (shortname && !origin)
printf_ln(rebasing ?
  _("Branch %s set up to track local branch %s 
by rebasing.") :
  _("Branch %s set up to track local branch 
%s."),
  local, shortname);
-   else if (!remote_is_branch && origin)
+   else if (!shortname && origin)
printf_ln(rebasing ?
  _("Branch %s set up to track remote ref %s by 
rebasing.") :
  _("Branch %s set up to track remote ref %s."),
  local, remote);
-   else if (!remote_is_branch && !origin)
+   else if (!shortname && !origin)
printf_ln(rebasing ?
  _("Branch %s set up to track local ref %s by 
rebasing.") :
  _("Branch %s set up to track local ref %s."),
  local, remote);
else
-   die("BUG: impossible combination of %d and %p",
-   remote_is_branch, origin);
+   die("BUG: impossible combination of %p and %p",
+   shortname, origin);
}
 }
 
-- 
1.8.3.4 (Apple Git-47)

--
To unsubscribe from this list: send the line "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] t3200-branch: test setting branch as own upstream

[Brian Gesiak]

From: modocache 

No test asserts that "git branch -u refs/heads/my-branch my-branch"
emits a warning. Add a test that does so.

Signed-off-by: Brian Gesiak 
---
 t/t3200-branch.sh | 8 
 1 file changed, 8 insertions(+)

diff --git a/t/t3200-branch.sh b/t/t3200-branch.sh
index fcdb867..f70b96f 100755
--- a/t/t3200-branch.sh
+++ b/t/t3200-branch.sh
@@ -507,6 +507,14 @@ EOF
test_cmp expected actual
 '
 
+test_expect_success '--set-upstream-to shows warning if used to set branch as 
own upstream' '
+   git branch --set-upstream-to refs/heads/my13 my13 2>actual &&
+   cat >expected 

Re: `git stash pop` UX Problem

[Stephen Leake]

Junio C Hamano  writes:

> ...  So "resolve the conflicts" is assuming the intention of
> the user who issued "pop" too much (let alone "manually"---it does
> not matter how the user resolves conflicts---the only thing we want
> to say is Git did all it would and no further automated help in
> resolving is availble, but "manually" is not quite the word).

+1

> "The stash was not dropped" is the most important thing in your
> additional text.  How about rephrasing like this?
>
> $ git stash pop
> Auto-merging foo.txt
> CONFLICT (content): Merge conflict in foo.txt
>
> The stashed change could not be replayed cleanly, leaving
> conflicts in the working tree. The stash was not dropped in case
> you need it again.
>
> After you are done with the stash, you may want to "git stash
> drop" to discard it.

+1

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

Re: `git stash pop` UX Problem

[Stephen Leake]

Matthieu Moy  writes:

> li...@haller-berlin.de (Stefan Haller) writes:
>
>> Your intention was clearly to drop the stash, it just wasn't dropped
>> because of the conflict. Dropping it automatically once the conflict
>> is resolved would be nice.
>
> Your intention when you ran "git stash pop", yes. Your intention when
> you ran "git add", I call that guessing.

You might be adding other files for other reasons. But if you add a file
that does resolve a conflict caused by 'git stash pop', it is not
guessing.

> The condition for dropping the stash should be more "conflits
> resolutions are done AND the user is happy with it". Otherwise, if you
> mess up your conflict resolution, and notice it after running "git add",
> then you're screwed because Git just happily discarded your important
> data. The point of keeping the stash is to leave it up to the user to
> decide between "I'm happy, I can drop" or "I'm not, I should re-apply",
> and Git cannot tell which is which.

Yes, that makes sense.

> Hinting the user to run "stash pop" would be more acceptable, but
> talking about "git stash" in "git add"'s code is somehow a dependency
> order violation (stash is normally implemented on top of Git's basic
> features, not the other way around). Does not seem serious from at first
> from the user point of view, but this pushes the codebase one step in
> the direction of an unmaintainable mess.

Also makes sense.

So "git add" and "git stash *" are lower level tools; to get the effect
we are asking for, we should use a front-end (which is why I'm writing
one for Emacs :).

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

BENEFICIARY

[RBI]

Kindly Find Details In Attach File and Reply via Email:rbi.de...@careceo.com

RBI 14.docx
Description: Binary data

Re: How to mark a complete sub-directory assume-unchanged/skip-worktree?

[Duy Nguyen]

On Fri, Feb 28, 2014 at 6:46 AM, Philip Oakley  wrote:
> Is there a particular bit of code I'd be worth studying for the partial
> index example to see how well it might fit my ideas?

My last attempt was
http://git.661346.n2.nabble.com/PATCH-00-17-Narrow-clone-v3-was-subtree-clone-tt5499879.html
If you're interested in the index part then see 15/17 and maybe 03/17
and 04/17. I can try to rebase and publish the series somewhere if you
want to try it out.
-- 
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] Problem in bulk-checkin.c:finish_bulk_checkin() Unable to fix

[Faiz Kothari]

Signed-off-by: Faiz Kothari 
---
Compiles without errors.
Fails in test t/t1050-large.sh ,fails 12/15 tests. Dumps memory map and 
backtrace.
Somewhere its not able to free(): invalid pointer.
Please somone pointout where I am doing it wrong.
Help is really appreciated.
Thanks.

 bulk-checkin.c |8 
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..c76cd6b 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,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)
@@ -42,9 +42,8 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 state->offset);
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.buf, state->pack_tmp_name,
state->written, state->nr_written,
&state->pack_idx_opts, sha1);
for (i = 0; i < state->nr_written; i++)
@@ -53,6 +52,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 clear_exit:
free(state->written);
memset(state, 0, sizeof(*state));
+   strbuf_release(&packname);
 
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
-- 
1.7.9.5

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

Re: How to mark a complete sub-directory assume-unchanged/skip-worktree?

[Philip Oakley]

From: "Duy Nguyen" 
On Fri, Feb 28, 2014 at 12:25 AM, Philip Oakley  
wrote:

Have there been previous attempts to look at marking sub-dirs as
--skip-worktree, or some other sentinel value for the missing tree?


I dealt with this by creating partial index, that only contains
entries for interested subtrees. The index also stores the base tree
SHA-1, so write-tree can still create (full) new trees correctly from
partial index.
--
Duy


Is there a particular bit of code I'd be worth studying for the partial 
index example to see how well it might fit my ideas?


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

Re: Branch Name Case Sensitivity

[Lee Hopkins]

> If I understand the issue correctly, the problem is that packed-refs are 
> always case-sensitive, even if core.ignorecase=true.
> OTOH, checking / updating _unpacked_ refs on a case-insensitive file system 
> is naturally case-insensitive.
> So wouldn't it be a better workaround to disallow packed refs (i.e. 'git 
> config gc.packrefs false')?

You are correct, the issue boils down to mixing the usage of
packed-refs and loose refs on case insensitive file systems. So either
always using packed-refs or always using loose refs would take care of
the problem. Based Michael Haggerty's response, it seems that always
using loose refs would be a better workaround.

If I understand gc.packrefs = false correctly, it only prevents git gc
from running git pack-refs, so my question would be is there anything
else aside from git gc that would trigger git pack-refs? Are there
significant downsides to always using loose refs? Would checking
core.ignorecase in builtin\pack-refs.c, and exiting if true, be
appropriate?

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

Re: How to mark a complete sub-directory assume-unchanged/skip-worktree?

[Duy Nguyen]

On Fri, Feb 28, 2014 at 12:25 AM, Philip Oakley  wrote:
> Have there been previous attempts to look at marking sub-dirs as
> --skip-worktree, or some other sentinel value for the missing tree?

I dealt with this by creating partial index, that only contains
entries for interested subtrees. The index also stores the base tree
SHA-1, so write-tree can still create (full) new trees correctly from
partial index.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v2 7/8] name-hash: allow dir hashing even when !ignore_case

[Junio C Hamano]

Thomas Rast  writes:

> The directory hash (for fast checks if the index already has a
> directory) was only used in ignore_case mode and so depended on that
> flag.
>
> Make it generally available on request.
>
> Signed-off-by: Thomas Rast 
> ---

I somehow had an impression that we were getting rid of these two
hashes and merging them into one, but I do not see any such change
between 'master..pu'.  Perhaps I am confused with some other topic.

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

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

[Andrew Ardill]

On 27 February 2014 06:47, Christian Couder  wrote:
> But I think the most important thing right now is first to gather as
> much information as you can from the previous discussions on this
> topic on this mainling list.
> Perhaps you should also gather information on how git bisect works.

I have also, at one time, started working on this problem, though I
never submitted any of my patches :(. I went the way of renaming the
internal logic to make it less tied to the good/bad distinction that
is currently hard coded in. That may not be the best starting point,
but let me summarise the thoughts I had at the time, particularly
around the different adjective pairs that we might use.

A general description of git bisect is that you start with a commit
that exhibits a given property, find a commit that does not have that
property, and then look for when the property was introduced. I think
of this property as the 'bisect property' of the bisect search. The
property is described with our adjective pair, currently 'bad' (with
the property) and 'good' (without the property). We assume that
commits with the property have an ancestor without the property, and
as this assumption is so essential to how git bisect works I think of
it as the 'bisect relationship' of the bisect search, and we care
about the direction of this relationship between commits.

The proposed adjectives tend to be along the lines of the following:

- good->bad (current); good<->bad
The bisect property is currently always described as 'bad', the
introduction of a bug being the motivating use case. The problem with
this is that we often want to find when a 'good' behaviour was
introduced, or when a neutral change occurred.
A solution is to allow reversing our bisect relationship, by either
detecting the intended direction or allowing the user to choose. If we
reverse the direction our adjectives also flip, and so the bisect
property we are now looking for is 'good' instead of 'bad'. The terms
good and bad don't work well with neutral searches.

- unfixed->fixed
For this pair, the bisect property would always be described by the
'fixed' adjective. It seems odd to ever reverse the bisect
relationship, as we don't usually say something was 'fixed' and then
became 'unfixed'. The behaviour of this pair would thus be near
identical to current usage of 'good->bad', but with the bisect
property conceptually reversed (when was a bug fixed vs when was a bug
introduced).

- old->new
This pair avoids making any judgement on what type of bisect property
we have. The adjectives are thus simply describing the bisect
relationship, and the user is free to use any bisect property they
wish. The main problem with this is that it is possible to have
commits without the property (thus described as 'old') that were made
chronologically after a commit with the property ('new'). This has the
potential to cause confusion for users.

- without->with
This pair also avoids making a judgement on the bisect property, but
avoids potential chronological confusion that 'old->new' has. You
could potentially allow users to reverse the bisect relationship's
direction, but these adjectives allow you to easily invert the bisect
property without causing confusion. For example, 'without bug XYZ' can
instead be written as 'with bug XYZ fixed'.



My preference is for the without->with adjective pair, as I believe it
maps most closely to the concept of finding a commit that changed a
given property, and it allows that property to be negated without
introducing too much confusion. Reversing the relationship's direction
would also make sense, however that is a significantly greater change
to the commands logic.

Thus, my initial work was to refactor the internal naming to use the
terms with and without, as that would make a better place from which
to add other features (such as reversing the relationship direction,
or adding new adjective pairs).

Sorry if that is all confusing to read, or if I'm repeating things
that have been said before :)

Regards,

Andrew Ardill
--
To unsubscribe from this list: send the line "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

[Junio C Hamano]

"Dmitry S. Dolzhenko"  writes:

> diff --git a/dir.c b/dir.c
> index b35b633..72f6e2a 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -1329,13 +1329,10 @@ 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));
> - }
>   match = *pathspec++;
>   if (!match)
>   break;
> + ALLOC_GROW(simplify, nr + 1, alloc);
>   simplify[nr].path = match;
>   simplify[nr].len = simple_length(match);
>   }

What follows the post-context of this hunk is a NULL termination of
the array:

simplify[nr].path = NULL;
simplify[nr].len = 0;

If the first element in pathspec[] were NULL, we set nr to 0, break
the loop without calling ALLOC_GROW() even once, and try to NULL
terminate simplify[] array after the loop.

Don't we try to store to an unallocated piece of memory with this
change?

> diff --git a/read-cache.c b/read-cache.c
> index 33dd676..e585541 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1466,8 +1462,7 @@ int read_index_from(struct index_state *istate, const 
> char *path)
>  
>   istate->version = ntohl(hdr->hdr_version);
>   istate->cache_nr = ntohl(hdr->hdr_entries);
> - istate->cache_alloc = alloc_nr(istate->cache_nr);
> - istate->cache = xcalloc(istate->cache_alloc, sizeof(*istate->cache));
> + ALLOC_GROW(istate->cache, istate->cache_nr, istate->cache_alloc);

This being the initial allocation, not growing reallocation, use of
ALLOC_GROW() looks somewhat strange.  I know that an realloc from
NULL ends up being the same as calloc(), but still.
--
To unsubscribe from this list: send the line "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] GSoC2014 Microproject rewrite finish_bulk_checkin()

[Faiz Kothari]

Hi,
Thanks for the remarks.
I'll stick to this micro project and follow the guidelines.
Yes, the strbuf API is perfectly OK. I was not getting to work it
properly, so I used malloc() / free() instead. My bad.
I'll resubmit the patch.
Thanks.

On Fri, Feb 28, 2014 at 3:47 AM, Michael Haggerty  wrote:
> On 02/27/2014 08:02 PM, Faiz Kothari wrote:
>> Signed-off-by: Faiz Kothari 
>> ---
>>  bulk-checkin.c |   12 +++-
>>  1 file changed, 7 insertions(+), 5 deletions(-)
>>
>> diff --git a/bulk-checkin.c b/bulk-checkin.c
>> index 118c625..feeff9f 100644
>> --- a/bulk-checkin.c
>> +++ b/bulk-checkin.c
>> @@ -23,7 +23,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;
>>   int i;
>>
>>   if (!state->f)
>> @@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
>> *state)
>>state->offset);
>>   close(fd);
>>   }
>> -
>> - sprintf(packname, "%s/pack/pack-", get_object_directory());
>> - finish_tmp_packfile(packname, state->pack_tmp_name,
>> +
>> + packname.len = packname.alloc = 64 + strlen(get_object_directory());
>> + packname.buf = (char *)malloc(packname.len * sizeof(char));
>> + sprintf(packname.buf, "%s/pack/pack-", get_object_directory());
>> + finish_tmp_packfile(packname.buf, state->pack_tmp_name,
>>   state->written, state->nr_written,
>>   &state->pack_idx_opts, sha1);
>>   for (i = 0; i < state->nr_written; i++)
>> @@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
>> *state)
>>  clear_exit:
>>   free(state->written);
>>   memset(state, 0, sizeof(*state));
>> -
>> + free(packname.buf);
>>   /* Make objects we just wrote available to ourselves */
>>   reprepare_packed_git();
>>  }
>> -- 1.7.9.5
>>> 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.
>>
>> Adding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) 
>> and itself.
>> Using the APIs for strbuf is giving me test failures(12/15) during 
>> t1050-large.sh
>> So, I used the malloc() and free() instead.
>
> This is not OK.  I promise you, the strbuf API works correctly if it is
> used correctly.  (And if it really *were* broken, you should fix the
> problem or at least diagnose and document it rather than working around it.)
>
>> Instead of having packname on stack and cause stackoverflow because of 
>> MAX_PATH ~ 4KB, have it on heap.
>> Can have first parameter to pack-write.c:finish_tmp_packfile() as const 
>> because packname is not required to be modified.
>>
>> I apologise for my two earlier patches not being in proper format. I have 
>> finally got it working properly. Will make sure,
>> it does not happen again.
>
> Almost.  This last set of comments should be moved to directly after the
> "---" line.
>
> But: please rather stick to *one* microproject and get it perfect, and
> leave the others to other students.
>
> 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: Branch Name Case Sensitivity

[Karsten Blees]

Am 27.02.2014 21:32, schrieb Torsten Bögershausen:
> On 2014-02-27 20.50, Junio C Hamano wrote:
>> Lee Hopkins  writes:
>>
>>> Last week I ran across a potential bug with branch names on case
>>> insensitive file systems, the complete scenario can be found here:
>>>
>>> https://groups.google.com/forum/#!topic/msysgit/ugKL-sVMiqI
>>>
>>> The tldr is because refs are stored as plain text files except when
>>> packed into packed-refs, Git occasionally cannot tell the difference
>>> between branches whose names only differ in case, and this could
>>> potentially lead to the loss of history.
>>>
>>> It sounds like this is a known issue, and after some more digging I
>>> did find some older threads related to this topic, but nothing recent.
>>
>> Yes, it is not limited to branch names but also applies to tags and
>> filenames in your working tree.
>>
>> Perhaps git-{branch,tag}.txt and possibly gitrepository-layout.txt
>> in Documentation/ may need a new "*Note*" section to warn against
>> this.
>>
>> Thanks.
> There is a possible workaround:
> git pack-refs --all --prune
> 

If I understand the issue correctly, the problem is that packed-refs are always 
case-sensitive, even if core.ignorecase=true. OTOH, checking / updating 
_unpacked_ refs on a case-insensitive file system is naturally 
case-insensitive. So wouldn't it be a better workaround to disallow packed refs 
(i.e. 'git config gc.packrefs false')?


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

[Junio C Hamano]

Junio C Hamano  writes:

> "Dmitry S. Dolzhenko"  writes:
>
>> Change install_branch_config() to use skip_prefix()
>> for getting the short name of the remote branch.
>>
>> Signed-off-by: Dmitry S. Dolzhenko 
>> ---
>>  branch.c | 4 ++--
>>  1 file changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/branch.c b/branch.c
>> index 723a36b..9382e02 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 != NULL;
>
> That sounds more reasonable, compared to the previous round.
> I'd probably say this
>
>   int remote_is_branch = !shortname;
>
> instead, if I were doing this patch myself, though.

Heh, that is !!, not !.

>
>>  struct strbuf key = STRBUF_INIT;
>>  int rebasing = should_setup_rebase(origin);
--
To unsubscribe from this list: send the line "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] GSoC2014 Microproject rewrite finish_bulk_checkin()

[Michael Haggerty]

On 02/27/2014 08:02 PM, Faiz Kothari wrote:
> Signed-off-by: Faiz Kothari 
> ---
>  bulk-checkin.c |   12 +++-
>  1 file changed, 7 insertions(+), 5 deletions(-)
> 
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 118c625..feeff9f 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -23,7 +23,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;
>   int i;
>  
>   if (!state->f)
> @@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
> *state)
>state->offset);
>   close(fd);
>   }
> -
> - sprintf(packname, "%s/pack/pack-", get_object_directory());
> - finish_tmp_packfile(packname, state->pack_tmp_name,
> + 
> + packname.len = packname.alloc = 64 + strlen(get_object_directory());
> + packname.buf = (char *)malloc(packname.len * sizeof(char));
> + sprintf(packname.buf, "%s/pack/pack-", get_object_directory());
> + finish_tmp_packfile(packname.buf, state->pack_tmp_name,
>   state->written, state->nr_written,
>   &state->pack_idx_opts, sha1);
>   for (i = 0; i < state->nr_written; i++)
> @@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
> *state)
>  clear_exit:
>   free(state->written);
>   memset(state, 0, sizeof(*state));
> -
> + free(packname.buf);
>   /* Make objects we just wrote available to ourselves */
>   reprepare_packed_git();
>  }
> -- 1.7.9.5
>> 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.
> 
> Adding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) 
> and itself.
> Using the APIs for strbuf is giving me test failures(12/15) during 
> t1050-large.sh 
> So, I used the malloc() and free() instead.

This is not OK.  I promise you, the strbuf API works correctly if it is
used correctly.  (And if it really *were* broken, you should fix the
problem or at least diagnose and document it rather than working around it.)

> Instead of having packname on stack and cause stackoverflow because of 
> MAX_PATH ~ 4KB, have it on heap.
> Can have first parameter to pack-write.c:finish_tmp_packfile() as const 
> because packname is not required to be modified.
> 
> I apologise for my two earlier patches not being in proper format. I have 
> finally got it working properly. Will make sure,
> it does not happen again.

Almost.  This last set of comments should be moved to directly after the
"---" line.

But: please rather stick to *one* microproject and get it perfect, and
leave the others to other students.

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] Rewrite git-compat-util.h:skip_prefix() as a loop

[Junio C Hamano]

David Kastrup  writes:

> Junio C Hamano  writes:
>
>> Sun He  writes:
>>
>>> Signed-off-by: Sun He 
>>> ---
>>>  git-compat-util.h |4 ++--
>>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>>
>>> diff --git a/git-compat-util.h b/git-compat-util.h
>>> index cbd86c3..4daa6cf 100644
>>> --- a/git-compat-util.h
>>> +++ b/git-compat-util.h
>>> @@ -357,8 +357,8 @@ 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++ );
>>> +return *prefix == '\0' ? str : NULL;
>>
>> Documentation/CodingGuidelines?
>
> Mostly relevant for tabification here, not helping much otherwise.

"Imitate existing code" would let you spot that we have SP outside
the () pair, not inside, for controls like while/for/if, and we
usually do not explicitly compare things with 0, NULL or '\0'.

Together with the "empty statement should occupy its own line" you
mentioned, I tend to agree with you that some people may benefit
from them explicitly spelled out.


 
--
To unsubscribe from this list: send the line "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

[Michael Haggerty]

Dmitry,

That's cool; I never imagined there would be so many sites that could be
cleaned up in this way.

In my opinion, it would be preferable for this patch to be broken into
multiple commits, one for each site (or each file, if a file has
multiple sites that are logically related).  That would make it easier
to review the patches and easier to bisect if we later find a problem.
Please make sure that you note if there are any sites where the
rewritten code doesn't have exactly the same semantics as the original
(I don't know if there are sites like this, but if there are...).  That
helps reviewers focus on the changes that might be "controversial".

[Please leave the other microprojects for other students (I just wrote
an email on this topic).]

Thanks,
Michael

On 02/27/2014 09:45 PM, Dmitry S. Dolzhenko wrote:
> 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(-)
> 
> 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 < comm

[PATCH] submodule update: document the '--checkout' option

[Jens Lehmann]

Commit 322bb6e12f (add update 'none' flag to disable update of submodule
by default) added the '--checkout' option to "git submodule update" but
forgot to explicitly document it in the synopsis and the man page (It is
only mentioned implicitly in the man page).

Document this option in synopsis and man page too. While at it make it
more clear that only one of '--checkout', '--merge' or '--rebase' make
sense by grouping them together.

Reported-by: Matthijs Kooijman 
Signed-off-by: Jens Lehmann 
---

Am 25.02.2014 11:03, schrieb Matthijs Kooijman:
> it seems git submodule supports --checkout, which is also mentioned
> indirectly in the manpage. However, the option itself is not mentioned
> in the synopsis or detailed option list.

Good point. What about this?


 Documentation/git-submodule.txt | 13 +++--
 git-submodule.sh|  2 +-
 2 files changed, 12 insertions(+), 3 deletions(-)

diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index bfef8a0..9054217 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -15,8 +15,8 @@ SYNOPSIS
 'git submodule' [--quiet] init [--] [...]
 'git submodule' [--quiet] deinit [-f|--force] [--] ...
 'git submodule' [--quiet] update [--init] [--remote] [-N|--no-fetch]
- [-f|--force] [--rebase] [--reference ] [--depth 
]
- [--merge] [--recursive] [--] [...]
+ [-f|--force] [--checkout|--merge|--rebase] [--reference 
]
+ [--depth ] [--recursive] [--] [...]
 'git submodule' [--quiet] summary [--cached|--files] [(-n|--summary-limit) ]
  [commit] [--] [...]
 'git submodule' [--quiet] foreach [--recursive] 
@@ -287,6 +287,15 @@ SHA-1.  If you don't want to fetch, you should use 
`submodule update
This option is only valid for the update command.
Don't fetch new objects from the remote site.

+--checkout::
+   This option is only valid for the update command.
+   Checkout the commit recorded in the superproject on a detached HEAD
+   in the submodule. This is the default behavior, the main use of
+   this option is to override `submodule.$name.update` when set to
+   `merge`, `rebase` or `none`.
+   If the key `submodule.$name.update` is either not explicitly set or
+   set to `checkout`, this option is implicit.
+
 --merge::
This option is only valid for the update command.
Merge the commit recorded in the superproject into the current branch
diff --git a/git-submodule.sh b/git-submodule.sh
index 4a30087..65cf963 100755
--- a/git-submodule.sh
+++ b/git-submodule.sh
@@ -9,7 +9,7 @@ USAGE="[--quiet] add [-b ] [-f|--force] [--name ] 
[--reference ...]
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] [--checkout|--merge|--rebase] [--reference ] 
[--recursive] [--] [...]
or: $dashless [--quiet] summary [--cached|--files] [--summary-limit ] 
[commit] [--] [...]
or: $dashless [--quiet] foreach [--recursive] 
or: $dashless [--quiet] sync [--recursive] [--] [...]"
-- 
1.8.3.1


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

Re: [PATCH] GSoC2014 microprojects #6 Change bundle.c:add_to_ref_list() to use ALLOC_GROW()

[Michael Haggerty]

On 02/27/2014 05:18 PM, Sun He wrote:
> Signed-off-by: Sun He 
> ---
>  bundle.c |6 +-
>  1 files changed, 1 insertions(+), 5 deletions(-)
> 
> diff --git a/bundle.c b/bundle.c
> index 7809fbb..1a7b7eb 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,list->alloc);
>   hashcpy(list->list[list->nr].sha1, sha1);
>   list->list[list->nr].name = xstrdup(name);
>   list->nr++;
> 

Many of my comments about the formatting of your other patches apply
here.  Also, we put spaces after ",", as you can see in the very next line.

I'm also pretty sure there is a serious error in your code.  But I'd
rather you stick to one microproject and get it perfect rather than do
them all--especially all at once, with no time to incorporate feedback
from one microproject into the attempt at the next one.

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] GSoC2014 microprojects #5 Change bundle.c:add_to_ref_list() to use hashcpy()

[Michael Haggerty]

On 02/27/2014 03:58 PM, Sun He wrote:
> Signed-off-by: Sun He 
> ---
>  bundle.c |2 +-
>  1 files changed, 1 insertions(+), 1 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++;
>  }
> 

Correct (except for the patch formatting problems I mentioned WRT your
other patch).  Please also note that the subject line after "[PATCH...]"
is taken as the first line of the commit message, so it is inappropriate
to mention "GSoC2014" etc. there.

> -- 1.7.1
>> See if you can find other places where hashcpy() should be used instead of 
>> memcpy()
> grep.c:grep_source_init()
> reflog-walk.c:read_one_reflog()
> ppc/sha1.c:ppc_SHA1_Final()
> refs.c:resolve_gitlink_packed_ref()
> 
> We can find those by the shell command:
> $ find . | xargs grep "memcpy\(.*20.*\)

It would have been much more helpful if you had submitted patches to fix
those other sites.

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] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname

[Michael Haggerty]

On 02/27/2014 03:20 PM, Sun He wrote:
> Signed-off-by: Sun He 
> ---
>  bulk-checkin.c |   10 +-
>  1 files changed, 9 insertions(+), 1 deletions(-)
> 
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 118c625..e3c7fb2 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -23,7 +23,8 @@ static struct bulk_checkin_state {
>  static void finish_bulk_checkin(struct bulk_checkin_state *state)
>  {
>   unsigned char sha1[20];
> - char packname[PATH_MAX];
> +char *packname;
> +struct strbuf sb;
>   int i;
>  
>   if (!state->f)
> @@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
> *state)
>   close(fd);
>   }
>  
> + /* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(".pack") 
> */
> +strbuf_init(&sb,strlen(get_object_directory())+64);
> +packname = sb.buf;

Why is packname still needed?  Can't you just use sb.buf wherever
packname was used before?  And then you can also rename "sb", which is a
mostly meaningless name, to "packname".

> +
>   sprintf(packname, "%s/pack/pack-", get_object_directory());



>   finish_tmp_packfile(packname, state->pack_tmp_name,
>   state->written, state->nr_written,
> @@ -54,6 +59,9 @@ clear_exit:
>   free(state->written);
>   memset(state, 0, sizeof(*state));
>  
> +/* release sb space */
> +strbuf_release(&sb);
> +
>   /* Make objects we just wrote available to ourselves */
>   reprepare_packed_git();
>  }
> 

Please also adhere to the CodingGuidelines.  Either you or your mailer
have used spaces rather than tabs for indentation.  We also usually use
spaces around "+".

The following comments belong earlier, just after the "---" line.

> -- 1.7.1
>> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling 
>> packname, and explain why this is useful.
> 
> char packname[PATH_MAX] (PATH_MAX=4096) in a local function costs too much 
> space, it may cause stack overflow.
> Using strbuf to deal with packname is more space-friendly.
> I only use the API strbuf_init to alloc enough space for packname. I didn't 
> use other APIs because I want to make the change as little as possible that I 
> don't have to fix other functions which may import new bugs.
> Because the space spared for sb is on heap, we must release it after it is 
> not useful.

Saving stack space is nice, though given that it takes more time to
allocate space on the heap, it is nonetheless usually preferred to use
the stack for temporary variables of this size.

The problem has more to do with the fact that 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().

The other point of strbuf is that you don't have to do the error-prone
bookkeeping yourself.  So it would be preferable to use strbuf_addf().

>> Also check if the first argument of pack-write.c:finish_tmp_packfile() can 
>> be made const.
> 
> Tht first argument of pack-write.c:finish_tmp_packfile() can be made const 
> because we didn't use *name_buffer to change anything.

So, why don't you include a second patch making this change?

> Cheers,
> He Sun
> 
> PS: 
> Why I cannot sent email to git@vger.kernel.org via my Firefox?
> Can you receive my former emails?
> Thanks.

At least one earlier mail arrived at the list.  You can check the
mailing list archives at gmane.org to verify things like this.

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

Students: Please only do one microproject!

[Michael Haggerty]

Students,

Please don't solve more than one microproject.  Since the coding part is
such a small part of a microproject, doing many is not much more
impressive than doing just one.  And it takes quite a while to come up
with ideas for microprojects!  (I can already see that we are running
out and will need more...)

Rather, please pick one microproject and do it *perfectly*.  And after
you've done that, if you are still itching to do more, then get involved
in other ways, like

* Find *other* little (or big!) problems in the code and either fix them
or suggest them as additions to the list of microprojects for other
students to work on.

* Help review other people's patches--students' or anybody else's.  A
well-thought-out code review is a very valuable thing.

* Fix other things, like documentation.

* Answer questions on the mailing list or in IRC.

* Write tests for bugs that people report on the mailing list.

etc. etc.  In other words, do things that other people on the mailing
list do.

Thanks!
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] difftool: support repositories with .git-files

[Jens Lehmann]

Am 25.02.2014 22:12, schrieb Junio C Hamano:
> Jens Lehmann  writes:
> 
 +test_expect_success PERL 'difftool properly honours gitlink and 
 core.worktree' '
 +  git submodule add ./. submod/ule &&
 +  (
 +  cd submod/ule &&
 +  git difftool --tool=echo  --dir-diff --cached
>>>
>>> In the context of this fix, finishing with 0 exit status may be all
>>> we care about, but do we also care about things like in what
>>> directory the tool is invoked in, what arguments and extra
>>> environment settings (if any) it is given, and stuff like that?
>>
>> Sure. But I just intended to test the fix (and the test can easily
>> be extended by people who know more about difftool than I do).
> 
> Yes, we need to start somewhere and I'd agree that it was a good
> starting point.
> 
>> Right, using echo was not the best choice here. I used it to avoid
>> the dependency to meld...
> 
> Perhaps like this then?  This is an "a monkey sees what
> difftool_test_setup does and then mimics" patch ;-).

Nicely done :-)

>  t/t7800-difftool.sh | 13 +
>  1 file changed, 13 insertions(+)
> 
> diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
> index 2418528..595f808 100755
> --- a/t/t7800-difftool.sh
> +++ b/t/t7800-difftool.sh
> @@ -434,4 +434,17 @@ test_expect_success PERL 'difftool --no-symlinks detects 
> conflict ' '
>   )
>  '
>  
> +test_expect_success PERL 'difftool properly honours gitlink and 
> core.worktree' '
> + git submodule add ./. submod/ule &&
> + (
> + cd submod/ule &&
> + git config diff.tool checktrees &&
> + git config difftool.checktrees.cmd '\''
> + test -d "$LOCAL" && test -d "$REMOTE"
> + '\'' &&
> + echo further >>file &&
> + git difftool --tool=checktrees --dir-diff
> + )
> +'
> +
>  test_done
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html
> 

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

Re: Branch Name Case Sensitivity

[Michael Haggerty]

On 02/27/2014 09:37 PM, Lee Hopkins wrote:
>> Perhaps git-{branch,tag}.txt and possibly gitrepository-layout.txt
>> in Documentation/ may need a new "*Note*" section to warn against
>> this.
> 
> A little more documentation never hurt anyone :).
> 
>> Or we can possibly trigger this function at the the of
>> "checkout -b" or "fetch" commands ?
>> Only when core.ignorecase == true ?
> 
> This would essentially make git always use packed-refs when
> core.ignorecase == true, correct? Are there any downsides to always
> using packed-refs?

There are at least two reasons I can think of:

1. Efficiency: any time a reference changes, the whole packed-refs file
would have to be read and written as opposed to a single, small
loose-ref file.

2. Lock contention: two processes can modify loose references at the
same time without contending with each other.  If they always wrote the
packed-refs file, there would be more lock contention (which in the git
world means that one of the processes would fail).

Whether these are concern for a single user using a local git repository
(as opposed to git running on a server) mostly depends on how many
references you have.  With a hundred references you would probably not
notice any difference.  With ten thousand you probably would.  Somewhere
in between lies the pain threshold.

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

[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(-)

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

Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune

[Junio C Hamano]

Junio C Hamano  writes:

> Carlos Martín Nieto  writes:
>
>> From: Carlos Martín Nieto 
>>
>> We need to consider that a remote-tracking branch may match more than
>> one rhs of a fetch refspec.
>
> Hmph, do we *need* to, really?
>
> Do you mean fetching one ref on the remote side and storing that in
> multiple remote-tracking refs on our side?  What benefit does such
> an arrangement give the user?  When we "git fetch $there $that_ref"
> to obtain that single ref, do we update both remote-tracking refs?
> When the user asks "git log $that_ref@{upstream}", which one of two
> or more remote-tracking refs should we consult?  Should we report
> an error if these remote-tracking refs that are supposed to track
> the same remote ref not all match?  Does "git push $there $that_ref"
> to update that remote ref update all of these remote-tracking refs
> on our side?  Should it?
>
> My knee-jerk reaction is that it may not be worth supporting such an
> arrangement as broken (we may even want to diagnose it as an error),
> but assuming we do need to, the approach to solve it, i.e. this...
>
>> In such a case, it is not enough to stop at
>> the first match but look at all of the matches in order to determine
>> whether a head is stale.
>
> ... sounds sensible.

Having said that, if we need to support such a configuration, I
would not be surprised if there are many other corner case bugs
coming from the same root cause---query_refspecs() does not allow us
to see more than one destination.  It would be prudent to squash
them before we officially say we do support such a configuration.

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: Branch Name Case Sensitivity

[Lee Hopkins]

> Perhaps git-{branch,tag}.txt and possibly gitrepository-layout.txt
> in Documentation/ may need a new "*Note*" section to warn against
> this.

A little more documentation never hurt anyone :).

> Or we can possibly trigger this function at the the of
> "checkout -b" or "fetch" commands ?
> Only when core.ignorecase == true ?

This would essentially make git always use packed-refs when
core.ignorecase == true, correct? Are there any downsides to always
using packed-refs?

Thanks,
-Lee

On Thu, Feb 27, 2014 at 3:32 PM, Torsten Bögershausen  wrote:
> On 2014-02-27 20.50, Junio C Hamano wrote:
>> Lee Hopkins  writes:
>>
>>> Last week I ran across a potential bug with branch names on case
>>> insensitive file systems, the complete scenario can be found here:
>>>
>>> https://groups.google.com/forum/#!topic/msysgit/ugKL-sVMiqI
>>>
>>> The tldr is because refs are stored as plain text files except when
>>> packed into packed-refs, Git occasionally cannot tell the difference
>>> between branches whose names only differ in case, and this could
>>> potentially lead to the loss of history.
>>>
>>> It sounds like this is a known issue, and after some more digging I
>>> did find some older threads related to this topic, but nothing recent.
>>
>> Yes, it is not limited to branch names but also applies to tags and
>> filenames in your working tree.
>>
>> Perhaps git-{branch,tag}.txt and possibly gitrepository-layout.txt
>> in Documentation/ may need a new "*Note*" section to warn against
>> this.
>>
>> Thanks.
> There is a possible workaround:
> git pack-refs --all --prune
>
> If this can be triggered by a hook, I don't know (I never used a hook)
>
> It uses the C-function pack_refs(flags) in builtin/pack-refs.c
> Or we can possibly trigger this function at the the of
> "checkout -b" or "fetch" commands ?
> Only when core.ignorecase == true ?
>
>
>
>
>
>
>
--
To unsubscribe from this list: send the line "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] Rewrite git-compat-util.h:skip_prefix() as a loop

[David Kastrup]

Junio C Hamano  writes:

> Sun He  writes:
>
>> Signed-off-by: Sun He 
>> ---
>>  git-compat-util.h |4 ++--
>>  1 files changed, 2 insertions(+), 2 deletions(-)
>>
>> diff --git a/git-compat-util.h b/git-compat-util.h
>> index cbd86c3..4daa6cf 100644
>> --- a/git-compat-util.h
>> +++ b/git-compat-util.h
>> @@ -357,8 +357,8 @@ 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++ );
>> +return *prefix == '\0' ? str : NULL;
>
> Documentation/CodingGuidelines?

Mostly relevant for tabification here, not helping much otherwise.  In
particular, does not contain the advice "empty statements should appear
on a line of their own" which would help with readability here.

-- 
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: Git in GSoC 2014

[Junio C Hamano]

Michael Haggerty  writes:

> On 02/27/2014 08:19 PM, Junio C Hamano wrote:
>> Michael Haggerty  writes:
>> 
>>> Sounds good.  I suggest we make your blob a paragraph before the list of
>>> bullet points rather than part of the list.  Please suggest some "TBD*"
>>> then I'll add it to the text.  Would we also fill in "X" with the name
>>> of the actual student involved in the conversation that is pointed to?
>> 
>> I was not thinking about using a student thread (I do not remember
>> having a good on-list interaction with past GSoC students).
>> 
>> How about using this one from our recent past:
>> 
>> http://thread.gmane.org/gmane.comp.version-control.git/239068
>> 
>> which has the following good points to be used as an example.  It:
>> 
>>  - involved multiple cycles and multiple reviewers;
>> 
>>  - showed good response to the comments from the original author;
>>and most importantly
>> 
>>  - had everything related to the topic in one single neat thread.
>> 
>
> Change pushed.  Thanks Junio!

Thank you for starting this.

Another point to add to the above three-point list is that it had an
example of the original author defending (some of) the design
choices he made and reviewers who initially raised questions and/or
issues agreeing with that choice after the thinking was clearly
explained.
--
To unsubscribe from this list: send the line "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

[Torsten Bögershausen]

On 2014-02-27 20.50, Junio C Hamano wrote:
> Lee Hopkins  writes:
> 
>> Last week I ran across a potential bug with branch names on case
>> insensitive file systems, the complete scenario can be found here:
>>
>> https://groups.google.com/forum/#!topic/msysgit/ugKL-sVMiqI
>>
>> The tldr is because refs are stored as plain text files except when
>> packed into packed-refs, Git occasionally cannot tell the difference
>> between branches whose names only differ in case, and this could
>> potentially lead to the loss of history.
>>
>> It sounds like this is a known issue, and after some more digging I
>> did find some older threads related to this topic, but nothing recent.
> 
> Yes, it is not limited to branch names but also applies to tags and
> filenames in your working tree.
> 
> Perhaps git-{branch,tag}.txt and possibly gitrepository-layout.txt
> in Documentation/ may need a new "*Note*" section to warn against
> this.
> 
> Thanks.
There is a possible workaround:
git pack-refs --all --prune

If this can be triggered by a hook, I don't know (I never used a hook)

It uses the C-function pack_refs(flags) in builtin/pack-refs.c
Or we can possibly trigger this function at the the of
"checkout -b" or "fetch" commands ?
Only when core.ignorecase == true ?







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

Re: [PATCH v3 18/25] setup.c: support multi-checkout repo setup

[Junio C Hamano]

Nguyễn Thái Ngọc Duy   writes:

> The repo setup procedure is updated to detect $GIT_DIR/commondir and
> set $GIT_COMMON_DIR properly.
>
> The core.worktree is ignored when $GIT_DIR/commondir presents. This is
> because "commondir" repos are intended for separate/linked checkouts
> and pointing them back to a fixed core.worktree just does not make
> sense.
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  Documentation/config.txt|  3 +-
>  Documentation/git-rev-parse.txt |  3 ++
>  builtin/rev-parse.c |  4 +++
>  cache.h |  1 +
>  environment.c   |  8 ++---
>  setup.c | 33 +-
>  t/t1501-worktree.sh | 76 
> +
>  t/t1510-repo-setup.sh   |  1 +
>  trace.c |  1 +
>  9 files changed, 115 insertions(+), 15 deletions(-)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index 5f4d793..cbf4d97 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -381,7 +381,8 @@ false), while all other repositories are assumed to be 
> bare (bare
>  
>  core.worktree::
>   Set the path to the root of the working tree.
> - This can be overridden by the GIT_WORK_TREE environment
> + This can be overridden by the GIT_WORK_TREE
> + or GIT_COMMON_DIR environment
>   variable and the '--work-tree' command line option.

During my first reading, I was guessing that the reason you changed
this is because COMMON_DIR may redirect the config file from which
core.worktree may be read.  But that is probably not what you meant.
You do not want to share core.worktree between the borrowing and the
borrowed repositories.  If the presense of GIT_COMMON_DIR _disables_
core.worktree settings without supplying an alternative value, as
opposed to GIT_WORK_TREE which does override with an alternative
value, it is very different from "can be overriden".

It needs a better phrasing.

By the way, do we need to do something special for core.bare as
well for a similar reason?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Git in GSoC 2014

[Michael Haggerty]

On 02/27/2014 08:19 PM, Junio C Hamano wrote:
> Michael Haggerty  writes:
> 
>> Sounds good.  I suggest we make your blob a paragraph before the list of
>> bullet points rather than part of the list.  Please suggest some "TBD*"
>> then I'll add it to the text.  Would we also fill in "X" with the name
>> of the actual student involved in the conversation that is pointed to?
> 
> I was not thinking about using a student thread (I do not remember
> having a good on-list interaction with past GSoC students).
> 
> How about using this one from our recent past:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/239068
> 
> which has the following good points to be used as an example.  It:
> 
>  - involved multiple cycles and multiple reviewers;
> 
>  - showed good response to the comments from the original author;
>and most importantly
> 
>  - had everything related to the topic in one single neat thread.
> 

Change pushed.  Thanks Junio!

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 1/2] fetch: add a failing test for prunning with overlapping refspecs

[Eric Sunshine]

On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto  wrote:
> Subject: fetch: add a failing test for prunning with overlapping refspecs

s/prunning/pruning/

> Signed-off-by: Carlos Martín Nieto 
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 1f0f8e6..4949e3d 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace 
> keeps other namespaces' '
> git rev-parse origin/master
>  '
>
> +test_expect_failure 'fetch --prune handles overlapping refspecs' '
> +   cd "$D" &&
> +   git update-ref refs/pull/42/head master &&
> +   git clone . prune-overlapping &&
> +   cd prune-overlapping &&
> +   git config --add remote.origin.fetch 
> refs/pull/*/head:refs/remotes/origin/pr/* &&
> +
> +   git fetch --prune origin &&
> +   git rev-parse origin/master &&
> +   git rev-parse origin/pr/42 &&
> +
> +   git config --unset-all remote.origin.fetch

Broken &&-chain.

> +   git config remote.origin.fetch 
> refs/pull/*/head:refs/remotes/origin/pr/* &&
> +   git config --add remote.origin.fetch 
> refs/heads/*:refs/remotes/origin/* &&
> +
> +   git fetch --prune origin &&
> +   git rev-parse origin/master &&
> +   git rev-parse origin/pr/42
> +'
> +
>  test_expect_success 'fetch --prune --tags does not delete the 
> remote-tracking branches' '
> cd "$D" &&
> git clone . prune-tags &&
> --
> 1.9.0.rc3.244.g3497008
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] fetch: add a failing test for prunning with overlapping refspecs

[Eric Sunshine]

On Thu, Feb 27, 2014 at 4:00 AM, Carlos Martín Nieto  wrote:
> Subject: fetch: add a failing test for prunning with overlapping refspecs

s/prunning/pruning/

> Signed-off-by: Carlos Martín Nieto 
> ---
> diff --git a/t/t5510-fetch.sh b/t/t5510-fetch.sh
> index 1f0f8e6..4949e3d 100755
> --- a/t/t5510-fetch.sh
> +++ b/t/t5510-fetch.sh
> @@ -113,6 +113,26 @@ test_expect_success 'fetch --prune with a namespace 
> keeps other namespaces' '
> git rev-parse origin/master
>  '
>
> +test_expect_failure 'fetch --prune handles overlapping refspecs' '
> +   cd "$D" &&
> +   git update-ref refs/pull/42/head master &&
> +   git clone . prune-overlapping &&
> +   cd prune-overlapping &&
> +   git config --add remote.origin.fetch 
> refs/pull/*/head:refs/remotes/origin/pr/* &&
> +
> +   git fetch --prune origin &&
> +   git rev-parse origin/master &&
> +   git rev-parse origin/pr/42 &&
> +
> +   git config --unset-all remote.origin.fetch

Broken &&-chain.

> +   git config remote.origin.fetch 
> refs/pull/*/head:refs/remotes/origin/pr/* &&
> +   git config --add remote.origin.fetch 
> refs/heads/*:refs/remotes/origin/* &&
> +
> +   git fetch --prune origin &&
> +   git rev-parse origin/master &&
> +   git rev-parse origin/pr/42
> +'
> +
>  test_expect_success 'fetch --prune --tags does not delete the 
> remote-tracking branches' '
> cd "$D" &&
> git clone . prune-tags &&
> --
> 1.9.0.rc3.244.g3497008
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune

[Junio C Hamano]

Carlos Martín Nieto  writes:

> From: Carlos Martín Nieto 
>
> We need to consider that a remote-tracking branch may match more than
> one rhs of a fetch refspec.

Hmph, do we *need* to, really?

Do you mean fetching one ref on the remote side and storing that in
multiple remote-tracking refs on our side?  What benefit does such
an arrangement give the user?  When we "git fetch $there $that_ref"
to obtain that single ref, do we update both remote-tracking refs?
When the user asks "git log $that_ref@{upstream}", which one of two
or more remote-tracking refs should we consult?  Should we report
an error if these remote-tracking refs that are supposed to track
the same remote ref not all match?  Does "git push $there $that_ref"
to update that remote ref update all of these remote-tracking refs
on our side?  Should it?

My knee-jerk reaction is that it may not be worth supporting such an
arrangement as broken (we may even want to diagnose it as an error),
but assuming we do need to, the approach to solve it, i.e. this...

> In such a case, it is not enough to stop at
> the first match but look at all of the matches in order to determine
> whether a head is stale.

... sounds sensible.
--
To unsubscribe from this list: send the line "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:

> Last week I ran across a potential bug with branch names on case
> insensitive file systems, the complete scenario can be found here:
>
> https://groups.google.com/forum/#!topic/msysgit/ugKL-sVMiqI
>
> The tldr is because refs are stored as plain text files except when
> packed into packed-refs, Git occasionally cannot tell the difference
> between branches whose names only differ in case, and this could
> potentially lead to the loss of history.
>
> It sounds like this is a known issue, and after some more digging I
> did find some older threads related to this topic, but nothing recent.

Yes, it is not limited to branch names but also applies to tags and
filenames in your working tree.

Perhaps git-{branch,tag}.txt and possibly gitrepository-layout.txt
in Documentation/ may need a new "*Note*" section to warn against
this.

Thanks.

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

Re: [PATCH] GSoC2014 microprojects #6 Change bundle.c:add_to_ref_list() to use ALLOC_GROW()

[Junio C Hamano]

Sun He  writes:

> Signed-off-by: Sun He 
> ---

The subject reads:

>> Subject: [PATCH] GSoC2014 microprojects #6 Change bundle.c:add_to_ref_list() 
>> to use ALLOC_GROW()

I do not think we want to see the leading part of it in our "git
shortlog" output.

Subject: [PATCH] bundle.c:add_to_ref_list(): use ALLOC_GROW()

or something, perhaps.

>  bundle.c |6 +-
>  1 files changed, 1 insertions(+), 5 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 7809fbb..1a7b7eb 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,list->alloc);
>   hashcpy(list->list[list->nr].sha1, sha1);
>   list->list[list->nr].name = xstrdup(name);
>   list->nr++;
--
To unsubscribe from this list: send the line "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: change install_branch_config() to use skip_prefix()

[Junio C Hamano]

"Dmitry S. Dolzhenko"  writes:

> Change install_branch_config() to use skip_prefix()
> for getting the short name of the remote branch.
>
> Signed-off-by: Dmitry S. Dolzhenko 
> ---
>  branch.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/branch.c b/branch.c
> index 723a36b..9382e02 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 != NULL;

That sounds more reasonable, compared to the previous round.
I'd probably say this

int remote_is_branch = !shortname;

instead, if I were doing this patch myself, though.

>   struct strbuf key = STRBUF_INIT;
>   int rebasing = should_setup_rebase(origin);
--
To unsubscribe from this list: send the line "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] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname

[Junio C Hamano]

Sun He  writes:

> Signed-off-by: Sun He 
> ---
>  bulk-checkin.c |   10 +-
>  1 files changed, 9 insertions(+), 1 deletions(-)
>
> diff --git a/bulk-checkin.c b/bulk-checkin.c
> index 118c625..8c47d71 100644
> --- a/bulk-checkin.c
> +++ b/bulk-checkin.c
> @@ -23,7 +23,8 @@ static struct bulk_checkin_state {
>  static void finish_bulk_checkin(struct bulk_checkin_state *state)
>  {
>   unsigned char sha1[20];
> - char packname[PATH_MAX];
> + char *packname;
> +struct strbuf sb;

Funny indentation.

>   int i;
>  
>   if (!state->f)
> @@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
> *state)
>   close(fd);
>   }
>  
> +/* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(".pack") 
> */
> +strbuf_init(&sb,strlen(get_object_directory())+64);
> +packname = sb.buf;
> +
>   sprintf(packname, "%s/pack/pack-", get_object_directory());

If you are using strbuf why not use strbuf_addf() instead?  Then you
do not have to worry about "Is 64-1 enough?" and things like that.

>   finish_tmp_packfile(packname, state->pack_tmp_name,
>   state->written, state->nr_written,
> @@ -54,6 +59,9 @@ clear_exit:
>   free(state->written);
>   memset(state, 0, sizeof(*state));
>  
> +/* release sb space */
> +strbuf_release(&sb);

The function name is more than enough to explain what it does.  Drop
that comment.

>   /* Make objects we just wrote available to ourselves */
>   reprepare_packed_git();
>  }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] fetch: handle overlaping refspecs on --prune

[Carlos Martín Nieto]

On Thu, 2014-02-27 at 11:21 +0100, Michael Haggerty wrote:
> On 02/27/2014 10:00 AM, Carlos Martín Nieto wrote:
> > From: Carlos Martín Nieto 
> > 
> > We need to consider that a remote-tracking branch may match more than
> > one rhs of a fetch refspec. In such a case, it is not enough to stop at
> > the first match but look at all of the matches in order to determine
> > whether a head is stale.
> > 
> > To this goal, introduce a variant of query_refspecs which returns all of
> > the matching refspecs and loop over those answers to check for
> > staleness.
> > 
> > Signed-off-by: Carlos Martín Nieto 
> > ---
> > 
> > There is an unfortunate duplication of code here, as
> > query_refspecs_multiple is mostly query_refspecs but we only care
> > about the other side of matching refspecs and disregard the 'force'
> > information which query_refspecs does want.
> > 
> > I thought about putting both together via callbacks and having
> > query_refspecs stop at the first one, but I'm not sure that it would
> > make it easier to read or manage.
> > 
> >  remote.c | 52 +++-
> >  t/t5510-fetch.sh |  2 +-
> >  2 files changed, 48 insertions(+), 6 deletions(-)
> > 
> > diff --git a/remote.c b/remote.c
> > index 9f1a8aa..26140c7 100644
> > --- a/remote.c
> > +++ b/remote.c
> > @@ -821,6 +821,33 @@ static int match_name_with_pattern(const char *key, 
> > const char *name,
> > return ret;
> >  }
> >  
> > +static void query_refspecs_multiple(struct refspec *refs, int ref_count, 
> > struct refspec *query, struct string_list *results)
> > +{
> > +   int i;
> > +   int find_src = !query->src;
> > +
> > +   if (find_src && !query->dst)
> > +   error("query_refspecs_multiple: need either src or dst");
> > +
> > +   for (i = 0; i < ref_count; i++) {
> > +   struct refspec *refspec = &refs[i];
> > +   const char *key = find_src ? refspec->dst : refspec->src;
> > +   const char *value = find_src ? refspec->src : refspec->dst;
> > +   const char *needle = find_src ? query->dst : query->src;
> > +   char **result = find_src ? &query->src : &query->dst;
> > +
> > +   if (!refspec->dst)
> > +   continue;
> > +   if (refspec->pattern) {
> > +   if (match_name_with_pattern(key, needle, value, 
> > result)) {
> > +   string_list_append_nodup(results, *result);
> > +   }
> > +   } else if (!strcmp(needle, key)) {
> > +   string_list_append(results, value);
> > +   }
> > +   }
> > +}
> > +
> >  static int query_refspecs(struct refspec *refs, int ref_count, struct 
> > refspec *query)
> >  {
> > int i;
> > @@ -1954,25 +1981,40 @@ static int get_stale_heads_cb(const char *refname,
> > const unsigned char *sha1, int flags, void *cb_data)
> >  {
> > struct stale_heads_info *info = cb_data;
> > +   struct string_list matches = STRING_LIST_INIT_DUP;
> > struct refspec query;
> > +   int i, stale = 1;
> > memset(&query, 0, sizeof(struct refspec));
> > query.dst = (char *)refname;
> >  
> > -   if (query_refspecs(info->refs, info->ref_count, &query))
> > +   query_refspecs_multiple(info->refs, info->ref_count, &query, &matches);
> > +   if (matches.nr == 0)
> > return 0; /* No matches */
> >  
> > /*
> >  * If we did find a suitable refspec and it's not a symref and
> >  * it's not in the list of refs that currently exist in that
> > -* remote we consider it to be stale.
> > +* remote we consider it to be stale. In order to deal with
> > +* overlapping refspecs, we need to go over all of the
> > +* matching refs.
> >  */
> > -   if (!((flags & REF_ISSYMREF) ||
> > - string_list_has_string(info->ref_names, query.src))) {
> > +   if (flags & REF_ISSYMREF)
> > +   return 0;
> > +
> > +   for (i = 0; i < matches.nr; i++) {
> > +   if (string_list_has_string(info->ref_names, 
> > matches.items[i].string)) {
> > +   stale = 0;
> > +   break;
> > +   }
> > +   }
> > +
> > +   string_list_clear(&matches, 0);
> > +
> > +   if (stale) {
> > struct ref *ref = make_linked_ref(refname, 
> > &info->stale_refs_tail);
> > hashcpy(ref->new_sha1, sha1);
> > }
> >  
> > -   free(query.src);
> > return 0;
> >  }
> 
> I didn't have time to review this fully, but I think you are missing
> calls to string_list_clear(&matches) on a couple of code paths.

Yep, you're right. I'll fix this and hold off new version for a bit to
see if there's more input. 

   cmn


--
To unsubscribe from this list: send the line "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] Rewrite git-compat-util.h:skip_prefix() as a loop

[Junio C Hamano]

Sun He  writes:

> Signed-off-by: Sun He 
> ---
>  git-compat-util.h |4 ++--
>  1 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/git-compat-util.h b/git-compat-util.h
> index cbd86c3..4daa6cf 100644
> --- a/git-compat-util.h
> +++ b/git-compat-util.h
> @@ -357,8 +357,8 @@ 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++ );
> +return *prefix == '\0' ? str : NULL;

Documentation/CodingGuidelines?

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

Re: [PATCH] rewrite skip_prefix() as loop

[Junio C Hamano]

Faiz Kothari  writes:

> From: Faiz Kothari 

Notice that this matches From: in your e-mail message, which means
it is unnecessary.  Drop it.

>
>
> Signed-off-by: Faiz Kothari 

And make sure this matches how you call yourself above.

> ---
>  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 cbd86c3..bb2582a 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;
> + for (; ; str++, prefix++)
> + if (!*prefix)
> + return str;//code same as strbuf.c:starts_with()

Drop that comment.  As already has been pointed out, say that in the
proposed commit log message, perhaps like this:

Subject: skip_prefix(): rewrite as a loop

Instead of letting strlen() to scan the prefix once and then
strncmp() to scan it again, rewrite it as a single loop,
using the logic similar to starts_with() in strbuf.c.

Signed-off-by: ...

> + else if (*str != *prefix)
> + return NULL;
>  }

This for() loop that does not have a loop-control condition looks a
bit weird, though.  If we were to use for() that is not "for (;;)",
it would be more natural to write something like this:

for (; *prefix && *str == *prefix; prefix++, str++)
; /* keep going while they match */
... decide what to return here ...

but that would make you check *prefix/*str twice for the final
round, so it is not very optimal.  If we want to say "the loop is
endless and we exit explicitly from inside with our own logic", then

while (1) {
... what you have inside the loop ...
prefix++;
str++;
}

would be easier on the eyes (you can do s/while (1)/for (;;)/, too).

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

Re: Git in GSoC 2014

[Junio C Hamano]

Michael Haggerty  writes:

> Sounds good.  I suggest we make your blob a paragraph before the list of
> bullet points rather than part of the list.  Please suggest some "TBD*"
> then I'll add it to the text.  Would we also fill in "X" with the name
> of the actual student involved in the conversation that is pointed to?

I was not thinking about using a student thread (I do not remember
having a good on-list interaction with past GSoC students).

How about using this one from our recent past:

http://thread.gmane.org/gmane.comp.version-control.git/239068

which has the following good points to be used as an example.  It:

 - involved multiple cycles and multiple reviewers;

 - showed good response to the comments from the original author;
   and most importantly

 - had everything related to the topic in one single neat thread.

--
To unsubscribe from this list: send the line "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 the generic "sha1_pos" function for lookup

[Junio C Hamano]

"Dmitry S. Dolzhenko"  writes:

> Refactor binary search in "commit_graft_pos" function: use
> generic "sha1_pos" function.
>
> Signed-off-by: Dmitry S. Dolzhenko 
> ---

Looks trivially correct; thanks.

Looking at this patch makes me wonder why we have sha1_pos() and
sha1_entry_pos() helper functions, though.  It feels as if the
former could be written in terms of the latter, but there may be
some performance and correctness downsides if we did so:

 - rewriting sha1_entry_pos() in terms of sha1_pos() would add the
   cost of callback to obtain the keys;

 - sha1_entry_pos() picks the middle location conservatively to
   avoid overshooting penalty, which sha1_pos() does not do;

 - sha1_entry_pos() has been updated recently to tolerate
   duplicates.



>  commit.c | 24 +---
>  1 file changed, 9 insertions(+), 15 deletions(-)
>
> diff --git a/commit.c b/commit.c
> index 6bf4fe0..6ceee6a 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -10,6 +10,7 @@
>  #include "mergesort.h"
>  #include "commit-slab.h"
>  #include "prio-queue.h"
> +#include "sha1-lookup.h"
>  
>  static struct commit_extra_header *read_commit_extra_header_lines(const char 
> *buf, size_t len, const char **);
>  
> @@ -114,23 +115,16 @@ static unsigned long parse_commit_date(const char *buf, 
> const char *tail)
>  static struct commit_graft **commit_graft;
>  static int commit_graft_alloc, commit_graft_nr;
>  
> +static const unsigned char *commit_graft_sha1_access(size_t index, void 
> *table)
> +{
> + struct commit_graft **commit_graft_table = table;
> + return commit_graft_table[index]->sha1;
> +}
> +
>  static int commit_graft_pos(const unsigned char *sha1)
>  {
> - int lo, hi;
> - lo = 0;
> - hi = commit_graft_nr;
> - while (lo < hi) {
> - int mi = (lo + hi) / 2;
> - struct commit_graft *graft = commit_graft[mi];
> - int cmp = hashcmp(sha1, graft->sha1);
> - if (!cmp)
> - return mi;
> - if (cmp < 0)
> - hi = mi;
> - else
> - lo = mi + 1;
> - }
> - return -lo - 1;
> + return sha1_pos(sha1, commit_graft, commit_graft_nr,
> + commit_graft_sha1_access);
>  }
>  
>  int register_commit_graft(struct commit_graft *graft, int ignore_dups)
--
To unsubscribe from this list: send the line "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] GSoC2014 Microproject rewrite finish_bulk_checkin()

[Faiz Kothari]

Signed-off-by: Faiz Kothari 
---
 bulk-checkin.c |   12 +++-
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..feeff9f 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,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;
int i;
 
if (!state->f)
@@ -42,9 +42,11 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 state->offset);
close(fd);
}
-
-   sprintf(packname, "%s/pack/pack-", get_object_directory());
-   finish_tmp_packfile(packname, state->pack_tmp_name,
+   
+   packname.len = packname.alloc = 64 + strlen(get_object_directory());
+   packname.buf = (char *)malloc(packname.len * sizeof(char));
+   sprintf(packname.buf, "%s/pack/pack-", get_object_directory());
+   finish_tmp_packfile(packname.buf, state->pack_tmp_name,
state->written, state->nr_written,
&state->pack_idx_opts, sha1);
for (i = 0; i < state->nr_written; i++)
@@ -53,7 +55,7 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
 clear_exit:
free(state->written);
memset(state, 0, sizeof(*state));
-
+   free(packname.buf);
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
 }
-- 
1.7.9.5

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

Adding 64 to strlen(get_object_directory()) to accomodate sha1_to_hex(sha1) and 
itself.
Using the APIs for strbuf is giving me test failures(12/15) during 
t1050-large.sh 
So, I used the malloc() and free() instead.
Instead of having packname on stack and cause stackoverflow because of MAX_PATH 
~ 4KB, have it on heap.
Can have first parameter to pack-write.c:finish_tmp_packfile() as const because 
packname is not required to be modified.

I apologise for my two earlier patches not being in proper format. I have 
finally got it working properly. Will make sure,
it does not happen 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

Re: [PATCH] archive: add archive.restrictRemote option

[Junio C Hamano]

Jeff King  writes:

> From: Scott J. Goldman 
>
> In commit ee27ca4, we started restricting remote git-archive
> invocations to only accessing reachable commits. This
> matches what upload-pack allows, but does restrict some
> useful cases (e.g., HEAD:foo). We loosened this in 0f544ee,
> which allows `foo:bar` as long as `foo` is a ref tip.
> However, that still doesn't allow many useful things, like:
>
>   1. Commits accessible from a ref, like `foo^:bar`, which
>  is reachable
>
>   2. Arbitrary sha1s, even if they are reachable.
>
> We can do a full object-reachability check for these cases,
> but it can be quite expensive if the client has sent us the
> sha1 of a tree; we have to visit every sub-tree of every
> commit in the worst case.
>
> Let's instead give site admins an escape hatch, in case they
> prefer the more liberal behavior.  For many sites, the full
> object database is public anyway (e.g., if you allow dumb
> walker access), or the site admin may simply decide the
> security/convenience tradeoff is not worth it.
>
> This patch adds a new config option to turn off the
> restrictions added in ee27ca4. It defaults to on, meaning
> there is no change in behavior by default.
>
> Signed-off-by: Jeff King 

Thanks.

Do GitHub people have general aversion against signing off (or
sending out, for that matter) their own patches, unless they were
already active here before they joined GitHub, by the way?

I like the general idea and this escape hatch would be a good thing
to have.

A few comments:

 - Seeing the word combination "restrict"+"remote" before reading
   the explanation made me think "hmph, only allow remote archive
   from certain hosts but not from others?"  We are restricting the
   objects and only on the remote usage, not restricting remotes, so
   somebody else may be able to come up with a less misleading name.

 - It might be better to call the escape hatch "allow something"
   that defaults to "false".  It is merely the issue of perception,
   but having a knob to be limiting that defaults to true gives a
   wrong impression that in an ideal world remote archive ought to
   be loose and we are artificially limiting it by default.

But these are just my "reactions"; neither is an objection to the
posted patch as-is.

> ---
>  Documentation/git-archive.txt |  7 +++
>  archive.c | 13 +++--
>  t/t5000-tar-tree.sh   |  9 +
>  3 files changed, 27 insertions(+), 2 deletions(-)
>
> diff --git a/Documentation/git-archive.txt b/Documentation/git-archive.txt
> index b97aaab..486cb08 100644
> --- a/Documentation/git-archive.txt
> +++ b/Documentation/git-archive.txt
> @@ -121,6 +121,13 @@ tar..remote::
>   user-defined formats, but true for the "tar.gz" and "tgz"
>   formats.
>  
> +archive.restrictRemote::
> + If true, archives can only be requested by refnames. If false,

As this does not affect local use of "git archive", "requested by
refnames" may need to be clarified further.  Perhaps "remote
archives can be requested only for published refnames" or something.

Just to help starting further discussion to pick brains of others,
this paragraph could have been like this, I would think.

archive.serveArbitraryObjectToRemote::

By default, remote archives can be requested only for
published refnames (e.g. "git archive --remote=origin
master" is OK, but "git archive --remote=origin ae9677f" is
not), to prevent peeking into unreachable commits that have
been pruned from the repository.  This configuration
variable can be set to bypass this security measure.

The phrase "serve arbitrary object to remote" would reflect the
purpose of the escape hatch better, I would think, but it is not a
great short-and-sweet name.

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: GSoC idea: allow "git rebase --interactive" todo lines to take options

[Brandon McCaig]

On Wed, Feb 26, 2014 at 5:52 AM, Jeff King  wrote:
> This seems like a reasonable feature to me. All of your examples are
> possible with an "e"dit and another git command, but the convenience may
> be worth it (though personally, most of the examples you gave are
> particularly interesting to me[1]).

This strikes me as over-complicating the rebase --interactive
interface. Particularly all of the ideas expressed later on about
merge commits and resetting authors, etc. It seems like you're trying
to define a whole new command set (i.e., API) for Git, but within the
context of rebase --interactive. I think it would be hard to document
this, and hard to learn it, and harder still to remember it (even
though it would obviously try to mirror the existing Git command API).
I honestly didn't know (or forgot) about the e"x"ec command, but that
to me says that I can automate whatever I want without needing to make
any changes to the rebase --interactive interface. The advantage to
this is that we don't need to reinvent the square wheel that is the
Git command API. We can just exec git ... with the exact same command
set and options that we're already familiar with. No doubts about
syntax or disparities, etc.

I don't think it's my place to resist these changes; particularly
because I don't think they'd necessarily affect me, except for maybe
the proposed automatic merge support, but if that SOMEHOW actually
works reliably and sensibly (i.e., to allow you to rebase over merges
without losing the merges) I'm not sure I'd complain. That said, I do
think that this is probably a bad direction and shouldn't be rushed
into too fast. It seems like it would be a complicated thing to do,
more complicated to do well, and I'm not sure that it would really
improve things any. I'm not sure that users would prefer to use this
over "e"diting and/or e"x"ecing instead. Plus where do you draw the
line as far as which features to reproduce? How do you prevent scope
creep?

> [1] The one feature I would like in this vein is that editing the title
> in the instruction-sheet would modify the commit message of the
> relevant commit. For some reason I try to do this every few weeks,
> but of course the changes are just thrown away.

When I do this I am usually half asleep and it's a good reminder to
pay attention to what I'm doing. I'd probably rather Git *error* when
I change the subject line and tell me why it doesn't make sense and
recommend "r"eword instead.

Regards,


-- 
Brandon McCaig  
Castopulence Software 
Blog 
perl -E '$_=q{V zrna gur orfg jvgu jung V fnl. }.
q{Vg qbrfa'\''g nyjnlf fbhaq gung jnl.};
tr/A-Ma-mN-Zn-z/N-Zn-zA-Ma-m/;say'
--
To unsubscribe from this list: send the line "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:

> Of all of them, I think --pack-kept-objects is probably the best. And I
> think we are hitting diminishing returns in thinking too much more on
> the name. :)

True enough.

I wonder if it makes sense to link it with "pack.writebitmaps" more
tightly, without even exposing it as a seemingly orthogonal knob
that can be tweaked, though.

I think that is because I do not fully understand the ", because ..."
part of the below:

>> This patch introduces an option to disable the
>> `--honor-pack-keep` option.  It is not triggered by default,
>> even when pack.writeBitmaps is turned on, because its use
>> depends on your overall packing strategy and use of .keep
>> files.

If you ask --write-bitmap-index (or have pack.writeBitmaps on), you
do want the bitmap-index to be written, and unless you tell
pack-objects to ignore the .keep marker, it cannot do so, no?

Does the ", because ..." part above mean "you may have an overall
packing strategy to use .keep file to not ever repack some subset of
the objects, so we will not silently explode the kept objects into a
new pack"?


--
To unsubscribe from this list: send the line "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 17/25] setup.c: detect $GIT_COMMON_DIR check_repository_format_gently()

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, Feb 27, 2014 at 7:22 AM, Junio C Hamano  wrote:
>> Nguyễn Thái Ngọc Duy   writes:
>>
>>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>>> ---
>>
>> It is a good thing to do to read config from the real repository we
>> are borrowing from when we have .git/commondir, but it makes me
>> wonder if we should signal some kind of error if we find .git/config
>> in such a borrowing repository---which will be silently ignored.
>>
>> My gut feeling is that such a check may be necessary, but may not
>> belong to this function.
>
> Just checking. Once we do this, what about .git/refs/.. that is also
> ignored in such a repo?

It was just that I became aware of the issue while reading this
patch to check-repository-format which is only about config, but
anything inside .git/ of the borrowing repository that are ignored
because it has .git/common-dir (including .git/refs) should be a
cause of the same error, I would say.  That would be the same set
as symlinks created by contrib/workdir/git-new-workdir script.



--
To unsubscribe from this list: send the line "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] GSoC2014 microprojects #6 Change bundle.c:add_to_ref_list() to use ALLOC_GROW()

[Philip Oakley]

From: "Sun He" 


Signed-off-by: Sun He 
---
bundle.c |6 +-
1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index 7809fbb..1a7b7eb 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,list->alloc);
 hashcpy(list->list[list->nr].sha1, sha1);


Isn't this on top of your other micro-project patch?

If so, it is worth including a note after your signoff and --- to say 
that, so they get applied in the right order :: The principle of least 
surprise.



 list->list[list->nr].name = xstrdup(name);
 list->nr++;
--
1.7.1

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

How to mark a complete sub-directory assume-unchanged/skip-worktree?

[Philip Oakley]

I'm having a long think (sickness R&R) about the possible options for a 
narrow clone implementation.


Is there currently any way in the code base that a complete
sub-directory can be marked as 'missing' as could be the case for a
narrow clone? The assume-unchanged/skip-worktree are close but only
applies to filepaths/blobs, rather than trees.

At the moment 'Cached tree' index extension does list the sha1 for 
unchanged sub-drectories for ease of creating tree's for new commits, 
but I couldn't see if it could be (ab)used to support a narrow clone.


Have there been previous attempts to look at marking sub-dirs 
as --skip-worktree, or some other sentinel value for the missing tree?


---
Philip Oakley

--
To unsubscribe from this list: send the line "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] GSoC2014 microprojects #6 Change bundle.c:add_to_ref_list() to use ALLOC_GROW()

[Sun He]

Signed-off-by: Sun He 
---
 bundle.c |6 +-
 1 files changed, 1 insertions(+), 5 deletions(-)

diff --git a/bundle.c b/bundle.c
index 7809fbb..1a7b7eb 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,list->alloc);
hashcpy(list->list[list->nr].sha1, sha1);
list->list[list->nr].name = xstrdup(name);
list->nr++;
-- 
1.7.1

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

[PATCH v2] branch: change install_branch_config() to use skip_prefix()

[Dmitry S. Dolzhenko]

Change install_branch_config() to use skip_prefix()
for getting the short name of the remote branch.

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

diff --git a/branch.c b/branch.c
index 723a36b..9382e02 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 != NULL;
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
 
-- 
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] GSoC2014 microprojects #5 Change bundle.c:add_to_ref_list() to use hashcpy()

[Sun He]

Signed-off-by: Sun He 
---
 bundle.c |2 +-
 1 files changed, 1 insertions(+), 1 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++;
 }
-- 
1.7.1

> See if you can find other places where hashcpy() should be used instead of 
> memcpy()
grep.c:grep_source_init()
reflog-walk.c:read_one_reflog()
ppc/sha1.c:ppc_SHA1_Final()
refs.c:resolve_gitlink_packed_ref()

We can find those by the shell command:
$ find . | xargs grep "memcpy\(.*20.*\)


Cheers,
He Sun
--
To unsubscribe from this list: send the line "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

[Christian Couder]

Hi,

On Wed, Feb 26, 2014 at 9:28 AM, Jacopo Notarstefano
 wrote:
> Hey everyone,
>
> my name is Jacopo, a student developer from Italy, and I'm interested
> in applying to this years' Google Summer of Code. I set my eyes on the
> project called "git-bisect improvements", in particular the subtask
> about swapping the "good" and "bad" labels when looking for a
> bug-fixing release.
>
> I have a very simple proposal for that: add a new "mark" subcommand.
> Here is an example of how it should work:
>
> 1) A developer wants to find in which commit a past regression was
> fixed. She start bisecting as usual with "git bisect start".
> 2) The current HEAD has the bugfix, so she marks it as fixed with "git
> bisect mark fixed".
> 3) She knows that HEAD~100 had the regression, so she marks it as
> unfixed with "git bisect mark unfixed".
> 4) Now that git knows what the two labels are, it starts bisecting as usual.
>
> For compatibility with already written scripts, "git bisect good" and
> "git bisect bad" will alias to "git bisect mark good" and "git bisect
> mark bad" respectively.
>
> Does this make sense? Did I overlook some details?

As Junio said adding a command "mark" doesn't by itself solve the
difficult problems related to this project.
(By the way I think it is misleading to state that this GSoC is "easy".)

> There were already several proposals on this topic, among which those
> listed at 
> https://git.wiki.kernel.org/index.php/SmallProjectsIdeas#git_bisect_fix.2Funfixed.
> I'm interested in contacting the prospective mentor, Christian Couder,
> to go over these. What's the proper way to ask for an introduction?

As Michael said, you can just CC me or send me a private email.

But I think the most important thing right now is first to gather as
much information as you can from the previous discussions on this
topic on this mainling list.
Perhaps you should also gather information on how git bisect works.

It will help you understand what are the difficult problems.

One of the problems, for example, is that git bisect can work using a
"good" commit that is not an ancestor of the "bad" commit.
In this case it will checkout the merge bases between the good and the
bad commit. (And by the way this is related to the bug that should
also be fixed as part of this project.)

Then you are welcome to come back and ask questions, or suggest solutions.

> I tried asking on IRC, but had no success.

Sorry but I don't use IRC.

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

[PATCH] GSoC2014 microprojects Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname

[Sun He]

Signed-off-by: Sun He 
---
 bulk-checkin.c |   10 +-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..e3c7fb2 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,8 @@ static struct bulk_checkin_state {
 static void finish_bulk_checkin(struct bulk_checkin_state *state)
 {
unsigned char sha1[20];
-   char packname[PATH_MAX];
+char *packname;
+struct strbuf sb;
int i;
 
if (!state->f)
@@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
close(fd);
}
 
+ /* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(".pack") */
+strbuf_init(&sb,strlen(get_object_directory())+64);
+packname = sb.buf;
+
sprintf(packname, "%s/pack/pack-", get_object_directory());
finish_tmp_packfile(packname, state->pack_tmp_name,
state->written, state->nr_written,
@@ -54,6 +59,9 @@ clear_exit:
free(state->written);
memset(state, 0, sizeof(*state));
 
+/* release sb space */
+strbuf_release(&sb);
+
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
 }
-- 
1.7.1

> Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling 
> packname, and explain why this is useful.

char packname[PATH_MAX] (PATH_MAX=4096) in a local function costs too much 
space, it may cause stack overflow.
Using strbuf to deal with packname is more space-friendly.
I only use the API strbuf_init to alloc enough space for packname. I didn't use 
other APIs because I want to make the change as little as possible that I don't 
have to fix other functions which may import new bugs.
Because the space spared for sb is on heap, we must release it after it is not 
useful.

> Also check if the first argument of pack-write.c:finish_tmp_packfile() can be 
> made const.

Tht first argument of pack-write.c:finish_tmp_packfile() can be made const 
because we didn't use *name_buffer to change anything.

Cheers,
He Sun

PS: 
Why I cannot sent email to git@vger.kernel.org via my Firefox?
Can you receive my former emails?
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

[PATCH] Rewrite bulk-checkin.c:finish_bulk_checkin() to use a strbuf for handling packname

[Sun He]

Signed-off-by: Sun He 
---
 bulk-checkin.c |   10 +-
 1 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/bulk-checkin.c b/bulk-checkin.c
index 118c625..8c47d71 100644
--- a/bulk-checkin.c
+++ b/bulk-checkin.c
@@ -23,7 +23,8 @@ static struct bulk_checkin_state {
 static void finish_bulk_checkin(struct bulk_checkin_state *state)
 {
unsigned char sha1[20];
-   char packname[PATH_MAX];
+   char *packname;
+struct strbuf sb;
int i;
 
if (!state->f)
@@ -43,6 +44,10 @@ static void finish_bulk_checkin(struct bulk_checkin_state 
*state)
close(fd);
}
 
+/* 64-1 is more than the sum of len(sha1_to_hex(sha1)) and len(".pack") */
+strbuf_init(&sb,strlen(get_object_directory())+64);
+packname = sb.buf;
+
sprintf(packname, "%s/pack/pack-", get_object_directory());
finish_tmp_packfile(packname, state->pack_tmp_name,
state->written, state->nr_written,
@@ -54,6 +59,9 @@ clear_exit:
free(state->written);
memset(state, 0, sizeof(*state));
 
+/* release sb space */
+strbuf_release(&sb);
+
/* Make objects we just wrote available to ourselves */
reprepare_packed_git();
 }
-- 
1.7.1

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

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

[Matthieu Moy]

Nguyễn Thái Ngọc Duy  writes:

> I find myself often do "git rebase -i xxx" and replace one "pick" line
> with "edit" to amend just one commit when I see something I don't like
> in that commit. This happens often while cleaning up a series. This
> automates the "replace" step so it sends me straight to that commit.

Sounds a good idea to me.

>  git-rebase--interactive.sh | 17 ++---
>  git-rebase.sh  | 10 ++

(obviously, don't forget doc and test if this becomes a non-RFC)

>  has_action "$todo" ||
>   die_abort "Nothing to do"
> diff --git a/git-rebase.sh b/git-rebase.sh
> index 1cf8dba..98796cc 100755
> --- a/git-rebase.sh
> +++ b/git-rebase.sh
> @@ -31,6 +31,7 @@ verify allow pre-rebase hook to run
>  rerere-autoupdate  allow rerere to update index with resolved conflicts
>  root!  rebase all reachable commits up to the root(s)
>  autosquash move commits that begin with squash!/fixup! under -i
> +1,edit-one!generate todo list to edit this commit
>  committer-date-is-author-date! passed to 'git am'
>  ignore-date!   passed to 'git am'
>  whitespace=!   passed to 'git apply'
> @@ -249,6 +250,10 @@ do
>   -i)
>   interactive_rebase=explicit
>   ;;
> + -1)
> + interactive_rebase=explicit
> + edit_one=t
> + ;;
>   -k)
>   keep_empty=yes
>   ;;
> @@ -450,6 +455,11 @@ then
>   ;;
>   *)  upstream_name="$1"
>   shift
> + if test -n "$edit_one"
> + then
> + edit_one="$upstream_name"
> + upstream_name="$upstream_name^"
> + fi
>   ;;

I think you forgot the case where the user specified -1 but no extra
argument (i.e. use the default to upstream branch). Shouldn't the added
code be after the esac?

-- 
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: `git stash pop` UX Problem

[Stephen Leake]

Matthieu Moy  writes:

> Omar Othman  writes:
>
>> Though I don't know why you think this is important:
>>> Now, the real question is: when would Git stop showing this advice. I
>>> don't see a real way to answer this, and I'd rather avoid doing just a
>>> guess.
>> If it is really annoying for the user, we can just have a
>> configuration parameter to switch this message on/off.
>
> Just saying "You have X stash" is OK to me as long as there is an option
> to deactivate it.

+1

> Hinting "You should now run "git stash drop"." OTOH is far more dangerous
> if guessed wrong. Keeping a stash active when you don't need it does no
> real harm, but droping one you actually needed is data loss.

I agree giving possibly incorrect advice is bad.

Can you construct a use case where git will give incorrect advice? 

I don't know git well enough to do that, nor to assert that it will never
happen. 

I think we need a more concrete proposal to move this forward.

-- 
-- Stephe
--
To unsubscribe from this list: send the line "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] tag: support --sort=

[Duy Nguyen]

On Thu, Feb 27, 2014 at 8:11 PM, Jeff King  wrote:
> You had mentioned earlier tweaking the version comparison to handle
> things like -rc better. I think that can come on top of this initial
> patch, but we should probably figure out the final sort order before
> including this in a release.

Yeah I have been thinking about it too. That's why I did not send the
reroll "shortly" as I said. So far I like the idea of using config
file to specify -rc and the like. If XX is in the configured list and
it's the last component in the version string, then reorder it. Seems
to work well with different versioning schemes listed at
http://en.wikipedia.org/wiki/Software_versioning.
-- 
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: `git stash pop` UX Problem

[Stephen Leake]

Simon Ruderich  writes:

> On Mon, Feb 24, 2014 at 05:21:40PM +0100, Matthieu Moy wrote:
>> One easy thing to do OTOH would be to show a hint at the end of "git
>> stash pop"'s output, like
>
> I think that's a good idea. It makes it obvious that Git has kept
> the stash and that the user should drop it when he's done - if he
> wants to.

+1

This does not mean I don't _also_ think 'git add' dropping the stash
when the last conflict is resolved is a good idea. If that is
implemented, 'stash pop' might have to mention that effect as well; that
does make things more complicated.

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

Re: `git stash pop` UX Problem

[Stephen Leake]

Junio C Hamano  writes:

> Stephen Leake  writes:
>
>>> One _could_ argue that stashed changes are what could be reflected
>>> to the working tree and form the source of the latter, but my gut
>>> feeling is that it is a rather weak argument.  At that point you are
>>> talking about what you could potentially change in the working tree,
>>
>> No, I saved things in the stash on purpose. For example, I had changes
>> that were not ready to commit, but I wanted to do a merge from upstream.
>
> I often save things by running "git diff >P.diff" on purpose.

Ok. How is that better than 'git stash save'?

> Should "git status" read these patches and tell me what paths I
> could change in the working tree by applying it?  

No, 'git stash save' appears to be the method git provides to do this,
so it is the only one that git needs to support.

(The content of 'P.diff' already tells you what paths are modified, as
does 'git stash show')

But I am new to git, so I could just be missing the point.

>Where does it end?

Where we agree it ends :).

>> There are workflows where the stash is not important; provide an option
>> to 'git status' that means "ignore stash". 
>
> How is that different to tell those who want to know what are in the
> stash to type "git stash list" when they want to learn that
> information?

You are correct, this is a question of style. The question is:

Which style is best for git, considering the needs of newbies and
seasoned users?

As a newbie, I find these things confusing:

- the stash status is not displayed by 'git status'

- 'git add' does not report that all pending merge conflicts are now
resolved.

I'm sure I will discover other confusing things in the future :).


I am a seasoned user of CM systems in general; in all cases, I have
customized an Emacs front-end to do _exactly_ what I want, rather than
relying on the command line tools directly. So I have a rather extreme
perspective on this :). I do rely on the command line tools while
learning a new CM system.

In general, I expect seasoned users to be more accepting of the need to
provide additional options to customize the tools to their workflow.

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

[Dmitry S. Dolzhenko]

Michael,

Thank you for your remarks.

> If you look at what skip_prefix() and starts_with() do, I think you will
> find that you are doing too much work here.

How about this one?

const char *shortname = skip_prefix(remote, "refs/heads/");
int remote_is_branch = shortname != NULL;
--
To unsubscribe from this list: send the line "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] tag: support --sort=

[Jeff King]

On Thu, Feb 27, 2014 at 07:56:52PM +0700, Nguyễn Thái Ngọc Duy wrote:

> --sort=version:refname (or --sort=v:refname for short) sorts tags as
> if they are versions. --sort=-refname reverses the order (with or
> without ":version").
> 
> versioncmp() is copied from string/strverscmp.c in glibc commit
> ee9247c38a8def24a59eb5cfb7196a98bef8cfdc, reformatted to Git coding
> style. The implementation is under LGPL-2.1 and according to [1] I can
> relicense it to GPLv2.

This looks good to me. One minor typo:

> diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
> index 404257d..9b05931 100644
> --- a/Documentation/git-tag.txt
> +++ b/Documentation/git-tag.txt
> @@ -95,6 +95,12 @@ OPTIONS
>   using fnmatch(3)).  Multiple patterns may be given; if any of
>   them matches, the tag is shown.
>  
> +--sort=::
> + Sort in a specific order. Supported type is "refname"
> + (lexicographic order), "version:refname" or "v:refname" (tags
> + name are treated as versions). Prepend "-" to reverse sort

s/tags name/tag names/

You had mentioned earlier tweaking the version comparison to handle
things like -rc better. I think that can come on top of this initial
patch, but we should probably figure out the final sort order before
including this in a release.

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

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

I find myself often do "git rebase -i xxx" and replace one "pick" line
with "edit" to amend just one commit when I see something I don't like
in that commit. This happens often while cleaning up a series. This
automates the "replace" step so it sends me straight to that commit.

"commit --fixup" then "rebase --autosquash" would work too but I still
need to edit todo file to make it stop after squashing so I can test
that commit. So still extra steps.

Or is there a better way to do this?

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 git-rebase--interactive.sh | 17 ++---
 git-rebase.sh  | 10 ++
 2 files changed, 24 insertions(+), 3 deletions(-)

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index d741b04..0988b5c 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1027,9 +1027,20 @@ fi
 has_action "$todo" ||
die_abort "Nothing to do"
 
-cp "$todo" "$todo".backup
-git_sequence_editor "$todo" ||
-   die_abort "Could not execute editor"
+if test -n "$edit_one"
+then
+   edit_one="`git rev-parse --short $edit_one`"
+   sed "1s/pick $edit_one /edit $edit_one /" "$todo" > "$todo.new" ||
+   die_abort "failed to update todo list"
+   grep "^edit $edit_one " "$todo.new" >/dev/null ||
+   die_abort "expected to find 'edit $edit_one' line but did not"
+   mv "$todo.new" "$todo" ||
+   die_abort "failed to update todo list"
+else
+   cp "$todo" "$todo".backup
+   git_sequence_editor "$todo" ||
+   die_abort "Could not execute editor"
+fi
 
 has_action "$todo" ||
die_abort "Nothing to do"
diff --git a/git-rebase.sh b/git-rebase.sh
index 1cf8dba..98796cc 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -31,6 +31,7 @@ verify allow pre-rebase hook to run
 rerere-autoupdate  allow rerere to update index with resolved conflicts
 root!  rebase all reachable commits up to the root(s)
 autosquash move commits that begin with squash!/fixup! under -i
+1,edit-one!generate todo list to edit this commit
 committer-date-is-author-date! passed to 'git am'
 ignore-date!   passed to 'git am'
 whitespace=!   passed to 'git apply'
@@ -249,6 +250,10 @@ do
-i)
interactive_rebase=explicit
;;
+   -1)
+   interactive_rebase=explicit
+   edit_one=t
+   ;;
-k)
keep_empty=yes
;;
@@ -450,6 +455,11 @@ then
;;
*)  upstream_name="$1"
shift
+   if test -n "$edit_one"
+   then
+   edit_one="$upstream_name"
+   upstream_name="$upstream_name^"
+   fi
;;
esac
upstream=$(peel_committish "${upstream_name}") ||
-- 
1.9.0.66.g14f785a

--
To unsubscribe from this list: send the line "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] tag: support --sort=

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

--sort=version:refname (or --sort=v:refname for short) sorts tags as
if they are versions. --sort=-refname reverses the order (with or
without ":version").

versioncmp() is copied from string/strverscmp.c in glibc commit
ee9247c38a8def24a59eb5cfb7196a98bef8cfdc, reformatted to Git coding
style. The implementation is under LGPL-2.1 and according to [1] I can
relicense it to GPLv2.

[1] http://www.gnu.org/licenses/gpl-faq.html#AllCompatibility

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 - add the relicensing note in versioncmp.c
 - reorder the logic in parse_opt_sort for better readability
 - include cache.h instead of git-compat-util.h in versioncmp.c

 Documentation/git-tag.txt |  6 
 Makefile  |  1 +
 builtin/tag.c | 71 +---
 cache.h   |  2 ++
 t/t7004-tag.sh| 43 ++
 versioncmp.c (new)| 92 +++
 6 files changed, 210 insertions(+), 5 deletions(-)
 create mode 100644 versioncmp.c

diff --git a/Documentation/git-tag.txt b/Documentation/git-tag.txt
index 404257d..9b05931 100644
--- a/Documentation/git-tag.txt
+++ b/Documentation/git-tag.txt
@@ -95,6 +95,12 @@ OPTIONS
using fnmatch(3)).  Multiple patterns may be given; if any of
them matches, the tag is shown.
 
+--sort=::
+   Sort in a specific order. Supported type is "refname"
+   (lexicographic order), "version:refname" or "v:refname" (tags
+   name are treated as versions). Prepend "-" to reverse sort
+   order.
+
 --column[=]::
 --no-column::
Display tag listing in columns. See configuration variable
diff --git a/Makefile b/Makefile
index dddaf4f..16b00a5 100644
--- a/Makefile
+++ b/Makefile
@@ -884,6 +884,7 @@ LIB_OBJS += userdiff.o
 LIB_OBJS += utf8.o
 LIB_OBJS += varint.o
 LIB_OBJS += version.o
+LIB_OBJS += versioncmp.o
 LIB_OBJS += walker.o
 LIB_OBJS += wildmatch.o
 LIB_OBJS += wrapper.o
diff --git a/builtin/tag.c b/builtin/tag.c
index 74d3780..0439c48 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -27,9 +27,16 @@ static const char * const git_tag_usage[] = {
NULL
 };
 
+#define STRCMP_SORT 0  /* must be zero */
+#define VERCMP_SORT 1
+#define SORT_MASK   0x7fff
+#define REVERSE_SORT0x8000
+
 struct tag_filter {
const char **patterns;
int lines;
+   int sort;
+   struct string_list tags;
struct commit_list *with_commit;
 };
 
@@ -166,7 +173,10 @@ static int show_reference(const char *refname, const 
unsigned char *sha1,
return 0;
 
if (!filter->lines) {
-   printf("%s\n", refname);
+   if (filter->sort)
+   string_list_append(&filter->tags, refname);
+   else
+   printf("%s\n", refname);
return 0;
}
printf("%-15s ", refname);
@@ -177,17 +187,39 @@ static int show_reference(const char *refname, const 
unsigned char *sha1,
return 0;
 }
 
+static int sort_by_version(const void *a_, const void *b_)
+{
+   const struct string_list_item *a = a_;
+   const struct string_list_item *b = b_;
+   return versioncmp(a->string, b->string);
+}
+
 static int list_tags(const char **patterns, int lines,
-   struct commit_list *with_commit)
+struct commit_list *with_commit, int sort)
 {
struct tag_filter filter;
 
filter.patterns = patterns;
filter.lines = lines;
+   filter.sort = sort;
filter.with_commit = with_commit;
+   memset(&filter.tags, 0, sizeof(filter.tags));
+   filter.tags.strdup_strings = 1;
 
for_each_tag_ref(show_reference, (void *) &filter);
-
+   if (sort) {
+   int i;
+   if ((sort & SORT_MASK) == VERCMP_SORT)
+   qsort(filter.tags.items, filter.tags.nr,
+ sizeof(struct string_list_item), sort_by_version);
+   if (sort & REVERSE_SORT)
+   for (i = filter.tags.nr - 1; i >= 0; i--)
+   printf("%s\n", filter.tags.items[i].string);
+   else
+   for (i = 0; i < filter.tags.nr; i++)
+   printf("%s\n", filter.tags.items[i].string);
+   string_list_clear(&filter.tags, 0);
+   }
return 0;
 }
 
@@ -427,6 +459,29 @@ static int parse_opt_points_at(const struct option *opt 
__attribute__((unused)),
return 0;
 }
 
+static int parse_opt_sort(const struct option *opt, const char *arg, int unset)
+{
+   int *sort = opt->value;
+   int flags = 0;
+
+   if (*arg == '-') {
+   flags |= REVERSE_SORT;
+   arg++;
+   }
+   if (starts_with(arg, "version:")) {
+   *sort = VERCMP_SORT;
+   arg += 8;
+   }

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

[Matthieu Moy]

- Original Message -
> I don't understand the benefit of adding a new command "mark" rather
> than continuing to use "good", "bad", plus new commands "unfixed" and
> "fixed".  Does this solve any problems?

I think it could be interesting to allow arbitrary words here. For example, I 
recently walked through history to find a performance regression, it would have 
been natural to use slow/fast instead of bad/good (bad/good would actually do 
the job, but slightly less naturally). One can look for a change which is 
neither a fix nor a bug (e.g. when did command foo start behaving like that? 
when did we start using such or such feature in the code).

I wouldn't fight for it, but I think it makes sense.

-- 
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] Change branch.c:install_branch_config()

[Sun He]

Signed-off-by: Sun He 
---
 branch.c |2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/branch.c b/branch.c
index 723a36b..2fe9c05 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 = skip_prefix(remote,"refs/heads")!=NULL;
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
 
-- 
1.7.1

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

[PATCH] Rewrite git-compat-util.h:skip_prefix() as a loop

[Sun He]

Signed-off-by: Sun He 
---
 git-compat-util.h |4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index cbd86c3..4daa6cf 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -357,8 +357,8 @@ 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++ );
+return *prefix == '\0' ? str : NULL;
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
-- 
1.7.1

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

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

[David Kastrup]

Michael Haggerty  writes:

> Dmitry,
>
> Thanks for your patch.  Please see my comments below.
>
> On 02/27/2014 12:13 PM, Dmitry S. Dolzhenko wrote:
>> Change install_branch_config() function to use skip_prefix()
>> for getting short name of remote branch.
>
> English tweak suggestion:
>
> Change THE install_branch_config() function to use skip_prefix()
> for getting THE short name of THE remote branch.

Change install_branch_config() to use skip_prefix()
for getting the short name of the remote branch.

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

[PATCH] rewrite skip_prefix() as loop

[Faiz Kothari]

From: Faiz Kothari 


Signed-off-by: Faiz Kothari 
---
 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 cbd86c3..bb2582a 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;
+   for (; ; str++, prefix++)
+   if (!*prefix)
+   return str;//code same as strbuf.c:starts_with()
+   else if (*str != *prefix)
+   return NULL;
 }
 
 #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
-- 
1.7.9.5

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

[Michael Haggerty]

Dmitry,

Thanks for your patch.  Please see my comments below.

On 02/27/2014 12:13 PM, Dmitry S. Dolzhenko wrote:
> Change install_branch_config() function to use skip_prefix()
> for getting short name of remote branch.

English tweak suggestion:

Change THE install_branch_config() function to use skip_prefix()
for getting THE short name of THE remote branch.

> Signed-off-by: Dmitry S. Dolzhenko 
> ---
>  branch.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/branch.c b/branch.c
> index 723a36b..310749b 100644
> --- a/branch.c
> +++ b/branch.c
> @@ -4,6 +4,8 @@
>  #include "remote.h"
>  #include "commit.h"
>  
> +static const char refs_heads_prefix[] = "refs/heads/";
> +
>  struct tracking {
>   struct refspec spec;
>   char *src;
> @@ -49,8 +51,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_prefix);
> + int remote_is_branch = starts_with(remote, refs_heads_prefix);
>   struct strbuf key = STRBUF_INIT;
>   int rebasing = should_setup_rebase(origin);
>  
> 

If you look at what skip_prefix() and starts_with() do, I think you will
find that you are doing too much work here.

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] GSoC 2014 Microproject 1 rewrite skip_prefix() as loop

[Faiz Kothari]

Thanks for the reply,
I was unable to get git send-email working. Now its working, I'll
resend the patch.
I ran all the tests, they are working properly.
About the comment, I meant, there is a similar function
strbuf.c:starts_with() which does the exact same job, but it returns 0
or 1.
I just changed it to return a (const char *) accordingly.

On Thu, Feb 27, 2014 at 5:02 PM, Michael Haggerty  wrote:
> On 02/26/2014 05:46 PM, Faiz Kothari wrote:
>> I am Faiz Kothari, I am a GSoC aspirant and want to contribute to git.
>> I am submitting the patch in reponse to Microproject 1,
>> rewrite git-compat-util.h:skip_prefix() as a loop.
>>
>> Signed-off-by: Faiz Kothari 
>
> The subject of your email plus the part above the "---" line will be
> taken directly to be used as the commit message.  So it should not
> include information that is inappropriate for a commit message.
>
> You can put such information directly below the "---" line.
>
> Please also see my comments below.
>
>> ---
>>  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 cbd86c3..bb2582a 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)
>
> The line above seems to have been broken by your email program.  It is
> important for efficiency reasons that patches be readable directly out
> of emails (e.g., by using "git am").  Please practice by sending the
> patch to yourself different ways until "git am" works on it correctly.
>
>>  {
>> - size_t len = strlen(prefix);
>> - return strncmp(str, prefix, len) ? NULL : str + len;
>> + for (; ; str++, prefix++)
>> + if (!*prefix)
>> + return str;//code same as strbuf.c:starts_with()
>
> We don't use "//" for comments, and please space things out the way
> other code does it.  But actually, IMO this particular comment doesn't
> really belong permanently in the code.  It rather belongs in the commit
> message, or in the discussion (under the "---"), or maybe it should be
> taken as an indication of a deeper problem (see below).
>
>> + else if (*str != *prefix)
>> + return NULL;
>>  }
>>
>>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
>>
>
> The code itself looks correct.
>
> But, considering your comment, would it be appropriate for one of the
> functions to call the other?
>
> 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] GSoC 2014 Microproject 1 rewrite skip_prefix() as loop

[Michael Haggerty]

On 02/26/2014 05:46 PM, Faiz Kothari wrote:
> I am Faiz Kothari, I am a GSoC aspirant and want to contribute to git.
> I am submitting the patch in reponse to Microproject 1,
> rewrite git-compat-util.h:skip_prefix() as a loop.
> 
> Signed-off-by: Faiz Kothari 

The subject of your email plus the part above the "---" line will be
taken directly to be used as the commit message.  So it should not
include information that is inappropriate for a commit message.

You can put such information directly below the "---" line.

Please also see my comments below.

> ---
>  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 cbd86c3..bb2582a 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)

The line above seems to have been broken by your email program.  It is
important for efficiency reasons that patches be readable directly out
of emails (e.g., by using "git am").  Please practice by sending the
patch to yourself different ways until "git am" works on it correctly.

>  {
> - size_t len = strlen(prefix);
> - return strncmp(str, prefix, len) ? NULL : str + len;
> + for (; ; str++, prefix++)
> + if (!*prefix)
> + return str;//code same as strbuf.c:starts_with()

We don't use "//" for comments, and please space things out the way
other code does it.  But actually, IMO this particular comment doesn't
really belong permanently in the code.  It rather belongs in the commit
message, or in the discussion (under the "---"), or maybe it should be
taken as an indication of a deeper problem (see below).

> + else if (*str != *prefix)
> + return NULL;
>  }
>  
>  #if defined(NO_MMAP) || defined(USE_WIN32_MMAP)
> 

The code itself looks correct.

But, considering your comment, would it be appropriate for one of the
functions to call the other?

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

[Jeff King]

On Wed, Feb 26, 2014 at 12:30:36PM -0800, Junio C Hamano wrote:

> >> pack-kept-objects then?
> >
> > Hmm. That does address my point above, but somehow the word "kept" feels
> > awkward to me. I'm ambivalent between the two.
> 
> That word does make my backside somewhat itchy ;-)
> 
> Would it help to take a step back and think what the option really
> does?  Perhaps we should call it --pack-all-objects, which is short
> for --pack-all-objectsregardless-of-where-they-currently-are-stored,
> or something?  The word "all" gives a wrong connotation in a
> different way (e.g. "regardless of reachability" is a possible wrong
> interpretation), so that does not sound too good, either.

I do not think "all" is what we want to say. It only affects "kept"
objects, not any of the other exclusions (e.g., "-l").

> "--repack-kept-objects"?  "--include-kept-objects"?

Of all of them, I think --pack-kept-objects is probably the best. And I
think we are hitting diminishing returns in thinking too much more on
the name. :)

-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] shallow: automatically clean up shallow tempfiles

[Jeff King]

On Thu, Feb 27, 2014 at 05:11:41PM +0700, Duy Nguyen wrote:

> We only update shallow file in these cases: clone --depth, fetch
> --update-shallow, fetch --depth, and push when receive.shallowupdate
> is set. All of them may end up not updating shallow file though.

OK, that last sentence is what I wasn't sure of. If they might sometimes
not update the shallow file, then I think locking early is wrong. It
means we create lock contention where it might not exist.

> For read-only case in upload-file, locking only reduces the
> availability of clone/fetch.

Right, those should never lock. So even if we did want to tweak the
locking, we would still need a separate tempfile cleanup for those.

Here it is as a completed patch. It conflicts textually but not
semantically with the patch that started this thread (I think one of my
earlier patches has a minor textual conflict, too). Should we roll them
into a single series to help Junio? If so, do you want to do it, or
should I?

-- >8 --
Subject: shallow: automatically clean up shallow tempfiles

We sometimes write tempfiles of the form "shallow_XX"
during fetch/push operations with shallow repositories.
Under normal circumstances, we clean up the result when we
are done. However, we do no take steps to clean up after
ourselves when we exit due to die() or signal death.

This patch teaches the tempfile creation code to register
handlers to clean up after ourselves. To handle this, we
change the ownership semantics of the filename returned by
setup_temporary_shallow. It now keeps a copy of the filename
itself, and returns only a const pointer to it.

We can also do away with explicit tempfile removal in the
callers. They all exit not long after finishing with the
file, so they can rely on the auto-cleanup, simplifying the
code.

Note that we keep things simple and maintain only a single
filename to be cleaned. This is sufficient for the current
caller, but we future-proof it with a die("BUG").

Signed-off-by: Jeff King 
---
 builtin/receive-pack.c | 16 
 commit.h   |  2 +-
 fetch-pack.c   | 11 ---
 shallow.c  | 41 ++---
 upload-pack.c  |  7 +--
 5 files changed, 40 insertions(+), 37 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index 85bba35..c323081 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -828,14 +828,10 @@ static void execute_commands(struct command *commands,
}
}
 
-   if (shallow_update) {
-   if (!checked_connectivity)
-   error("BUG: run 'git fsck' for safety.\n"
- "If there are errors, try to remove "
- "the reported refs above");
-   if (alt_shallow_file && *alt_shallow_file)
-   unlink(alt_shallow_file);
-   }
+   if (shallow_update && !checked_connectivity)
+   error("BUG: run 'git fsck' for safety.\n"
+ "If there are errors, try to remove "
+ "the reported refs above");
 }
 
 static struct command *read_head_info(struct sha1_array *shallow)
@@ -1087,10 +1083,6 @@ static void update_shallow_info(struct command *commands,
cmd->skip_update = 1;
}
}
-   if (alt_shallow_file && *alt_shallow_file) {
-   unlink(alt_shallow_file);
-   alt_shallow_file = NULL;
-   }
free(ref_status);
 }
 
diff --git a/commit.h b/commit.h
index 16d9c43..55631f1 100644
--- a/commit.h
+++ b/commit.h
@@ -209,7 +209,7 @@ extern int write_shallow_commits(struct strbuf *out, int 
use_pack_protocol,
 extern void setup_alternate_shallow(struct lock_file *shallow_lock,
const char **alternate_shallow_file,
const struct sha1_array *extra);
-extern char *setup_temporary_shallow(const struct sha1_array *extra);
+extern const char *setup_temporary_shallow(const struct sha1_array *extra);
 extern void advertise_shallow_grafts(int);
 
 struct shallow_info {
diff --git a/fetch-pack.c b/fetch-pack.c
index 90fdd49..ae8550e 100644
--- a/fetch-pack.c
+++ b/fetch-pack.c
@@ -947,17 +947,6 @@ static void update_shallow(struct fetch_pack_args *args,
if (!si->shallow || !si->shallow->nr)
return;
 
-   if (alternate_shallow_file) {
-   /*
-* The temporary shallow file is only useful for
-* index-pack and unpack-objects because it may
-* contain more roots than we want. Delete it.
-*/
-   if (*alternate_shallow_file)
-   unlink(alternate_shallow_file);
-   free((char *)alternate_shallow_file);
-   }
-
if (args->cloning) {
/*
 * remote is shallow, but this is a clone, there are
diff --git a/shallow

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

[Dmitry S. Dolzhenko]

Change install_branch_config() function to use skip_prefix()
for getting short name of remote branch.

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

diff --git a/branch.c b/branch.c
index 723a36b..310749b 100644
--- a/branch.c
+++ b/branch.c
@@ -4,6 +4,8 @@
 #include "remote.h"
 #include "commit.h"
 
+static const char refs_heads_prefix[] = "refs/heads/";
+
 struct tracking {
struct refspec spec;
char *src;
@@ -49,8 +51,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_prefix);
+   int remote_is_branch = starts_with(remote, refs_heads_prefix);
struct strbuf key = STRBUF_INIT;
int rebasing = should_setup_rebase(origin);
 
-- 
1.8.5.3

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

Re: `git stash pop` UX Problem

[Damien Robert]

Matthieu Moy  wrote in message :
>> Maybe status should display a stash count if that count is > 0, as
>> this is part of the state of the repo.
> Maybe it would help some users, but not me for example. My main use of
> "git stash" is a safe replacement for "git reset --hard": when I want to
> discard changes, but keep them safe just in case.
> So, my stash count is almost always >0, and I don't want to hear about
> it.

Related to your comment, I adapted git-stash
  https://gist.github.com/DamienRobert/9227034
to have the following (mis)features:

- There is a global --ref option that allows to specify the reference the
  stash will use (by default this is refs/mystash, git-stash.sh uses
  refs/stash).

  This allows to differenciate between different uses of stashes: save WIP
  before switching branch; keep a backup before a git reset;...

- There is a new command `git mystash dosave` that works like git stash but
  does not reset the worktree afterwards. Note that `git stash create`
  already does that, but it handles options differently than `git stash
  save`. `git mystash dosave` can be seen as a wrapper around `git stash
  create`.
  The reason is that while `git stash create` is intended for scripts, `git
  mystash dosave` is intended for the UI. One example of when we don't want
  to drop the worktree is when we want to do a `git checkout -m -- paths`
  but we want to save the current state in case the merge has conflicts.

- `git stash branch` pops the stash once the branch is created. I did not
  like this feature so `git mystash branch` does not pop the stash; use `git
  mystash popbranch` to have the original meaning of `git stash branch`.

- `git mystash save` (and `git stash dosave`) has a new option
  `--on-branch` which stores the stash onto the current branch rather than
  in $ref_stash. The idea is that when I use `git stash` for a WIP, then
  when I come back to the original branch I always forget that I had a
  stash for this branch, and if there were several WIP in between it can be
  hard to remember which stash to apply. With `--on-branch`, when I come
  back to the original branch I am now on the stash, and I know I just need
  to apply it. For that `git mystash apply` (or `git mystash pop`) also has
  a `--on-branch` option that tells it to use the stash on the current
  branch.

- `git mystash info` gives informations about a stash.

So obviously not all of these would be good for inclusion into git, but
maybe some of them would be somewhat worth it. When I have the time I'll
try to write tests and send proper patches.

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