Re: [PATCH] setup: suppress implicit "." work-tree for bare repos

[Junio C Hamano]

Jeff King  writes:

> diff --git a/cache.h b/cache.h
> index e493563..070169a 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int 
> mode)
>  #define GIT_DIR_ENVIRONMENT "GIT_DIR"
>  #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
>  #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
> +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
>  #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
>  #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
>  #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"

Not adding any user documentation is fine (you explained why in the
log message), but I would really prefer to have some in-code comment
to clarify its meaning.  Is it "Please do use implicit work tree"
boolean?  Is it "This is the path to the work tree we have already
figured out" string?  Is it something else?  What is it used for,
who sets it, what other codepath that will be invented in the future
need to be careful to set it (or unset it) and how does one who
writes that new codepath decides that he needs to do so (or
shouldn't)?

I would know *today* that it is a bool to affect us, after having
discovered that we are in bare and we have set GIT_DIR (so if the
end user already had GIT_DIR, we shouldn't set it ourselves), and
also our child processes, but I am not confident that I will
remember this thread 6 months down the road.
--
To unsubscribe from this list: send the line "unsubscribe 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] setup: suppress implicit "." work-tree for bare repos

[Johannes Sixt]

Am 3/8/2013 8:15, schrieb Jeff King:
> --- a/cache.h
> +++ b/cache.h
> @@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int 
> mode)
>  #define GIT_DIR_ENVIRONMENT "GIT_DIR"
>  #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
>  #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
> +#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
>  #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
>  #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
>  #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"

This new variable needs to be added to environment.c:local_repo_env, right?

-- 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] setup: suppress implicit "." work-tree for bare repos

[Jeff King]

If an explicit GIT_DIR is given without a working tree, we
implicitly assume that the current working directory should
be used as the working tree. E.g.,:

  GIT_DIR=/some/repo.git git status

would compare against the cwd.

Unfortunately, we fool this rule for sub-invocations of git
by setting GIT_DIR internally ourselves. For example:

  git init foo
  cd foo/.git
  git status ;# fails, as we expect
  git config alias.st status
  git status ;# does not fail, but should

What happens is that we run setup_git_directory when doing
alias lookup (since we need to see the config), set GIT_DIR
as a result, and then leave GIT_WORK_TREE blank (because we
do not have one). Then when we actually run the status
command, we do setup_git_directory again, which sees our
explicit GIT_DIR and uses the cwd as an implicit worktree.

It's tempting to argue that we should be suppressing that
second invocation of setup_git_directory, as it could use
the values we already found in memory. However, the problem
still exists for sub-processes (e.g., if "git status" were
an external command).

You can see another example with the "--bare" option, which
sets GIT_DIR explicitly. For example:

  git init foo
  cd foo/.git
  git status ;# fails
  git --bare status ;# does NOT fail

We need some way of telling sub-processes "even though
GIT_DIR is set, do not use cwd as an implicit working tree".
We could do it by putting a special token into
GIT_WORK_TREE, but the obvious choice (an empty string) has
some portability problems, and could potentially be
triggered accidentally by a user.

Instead, we add a new boolean variable, GIT_IMPLICIT_WORK_TREE,
which suppresses the use of cwd as a working tree when
GIT_DIR is set. We trigger the new variable when we know we
are in a bare setting.

The variable is left intentionally undocumented, as this is
an internal detail (for now, anyway). If somebody comes up
with a good alternate use for it, and once we are confident
we have shaken any bugs out of it, we can consider promoting
it further.

Signed-off-by: Jeff King 
---
So I think this just ends up being a cleaner and smaller change than
trying to support $GIT_BARE. I think $GIT_BARE could allow more
flexibility, but it's flexibility nobody is particularly asking for, and
there are lots of nasty corner cases around it. I'm pretty sure this is
doing the right thing.

Having written this, I'm still tempted to signal the same thing by
putting /dev/null into GIT_WORK_TREE (Junio's suggestion from the old
thread). This one works OK because we only check GIT_WORK_TREE_IMPLICIT
_after_ exhausting all of the other working tree options, so it is
always subordinate to a later setting of GIT_WORK_TREE. But it seems a
little cleaner for somebody setting GIT_WORK_TREE To clear this
"implicit" flag automatically.

At the same time, I would wonder how other git implementations would
react to GIT_WORK_TREE=/dev/null. Would they try to chdir() there and
barf, when they could happily exist without a working tree? Doing it
this way seems a bit safer from regressions (those other implementations
do not get the _benefit_ of this patch unless they support
GIT_WORK_TREE_IMPLICIT, of course, but at least we are not breaking
them).

 cache.h   |  1 +
 git.c |  1 +
 setup.c   |  8 
 t/t1510-repo-setup.sh | 19 +++
 4 files changed, 29 insertions(+)

diff --git a/cache.h b/cache.h
index e493563..070169a 100644
--- a/cache.h
+++ b/cache.h
@@ -344,6 +344,7 @@ static inline enum object_type object_type(unsigned int 
mode)
 #define GIT_DIR_ENVIRONMENT "GIT_DIR"
 #define GIT_NAMESPACE_ENVIRONMENT "GIT_NAMESPACE"
 #define GIT_WORK_TREE_ENVIRONMENT "GIT_WORK_TREE"
+#define GIT_IMPLICIT_WORK_TREE_ENVIRONMENT "GIT_IMPLICIT_WORK_TREE"
 #define DEFAULT_GIT_DIR_ENVIRONMENT ".git"
 #define DB_ENVIRONMENT "GIT_OBJECT_DIRECTORY"
 #define INDEX_ENVIRONMENT "GIT_INDEX_FILE"
diff --git a/git.c b/git.c
index b10c18b..24b7984 100644
--- a/git.c
+++ b/git.c
@@ -125,6 +125,7 @@ static int handle_options(const char ***argv, int *argc, 
int *envchanged)
static char git_dir[PATH_MAX+1];
is_bare_repository_cfg = 1;
setenv(GIT_DIR_ENVIRONMENT, getcwd(git_dir, 
sizeof(git_dir)), 0);
+   setenv(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, "0", 1);
if (envchanged)
*envchanged = 1;
} else if (!strcmp(cmd, "-c")) {
diff --git a/setup.c b/setup.c
index 1dee47e..6c87660 100644
--- a/setup.c
+++ b/setup.c
@@ -523,6 +523,12 @@ static const char *setup_explicit_git_dir(const char 
*gitdirenv,
set_git_work_tree(core_worktree);
}
}
+   else if (!git_env_bool(GIT_IMPLICIT_WORK_TREE_ENVIRONMENT, 1)) {
+   /* #16d */
+   set_git_dir(gitdirenv);
+   free(gitfile);
+   return NULL;
+   }

Re: inotify to minimize stat() calls

[Torsten Bögershausen]

On 08.03.13 01:04, Junio C Hamano wrote:
> Torsten Bögershausen  writes:
> 
>> diff --git a/builtin/commit.c b/builtin/commit.c
>> index d6dd3df..6a5ba11 100644
>> --- a/builtin/commit.c
>> +++ b/builtin/commit.c
>> @@ -1158,6 +1158,8 @@ int cmd_status(int argc, const char **argv, const char 
>> *prefix)
>>  unsigned char sha1[20];
>>  static struct option builtin_status_options[] = {
>>  OPT__VERBOSE(&verbose, N_("be verbose")),
>> +OPT_BOOLEAN('c', "changed-only", &s.check_changed_only,
>> +N_("Ignore untracked files. Check if files known to 
>> git are modified")),
> 
> Doesn't this make one wonder why a separate bit and implementation
> is necessary to say "I am not interested in untracked files" when
> "-uno" option is already there?
Thanks Junio,
this is good news.
I need to admit that I wasn't aware about "git status -uno".

Thinking about it, how many git users are aware of the speed penalty
when running git status to find out which (tracked) files they had changed?

Or to put it the other way, when a developer wants a quick overview
about the files she changed, then git status -uno may be a good and fast friend.

Does it make sence to stress put that someway in the documentation?

diff --git a/Documentation/git-status.txt b/Documentation/git-status.txt
index 9f1ef9a..360d813 100644
--- a/Documentation/git-status.txt
+++ b/Documentation/git-status.txt
@@ -51,13 +51,18 @@ default is 'normal', i.e. show untracked files and directori
 +
 The possible options are:
 +
-   - 'no' - Show no untracked files
+   - 'no' - Show no untracked files (this is fastest)
- 'normal' - Shows untracked files and directories
- 'all'- Also shows individual files in untracked directories.
 +
 The default can be changed using the status.showUntrackedFiles
 configuration variable documented in linkgit:git-config[1].
 
++
+Note: Searching for untracked files or directories may take some time.
+A fast way to get a status of files tracked by git is to use
+'git status -uno'
+












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

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

Re: [PATCH 2/3] git p4 test: should honor symlink in p4 client root

[Johannes Sixt]

Am 3/8/2013 0:19, schrieb Pete Wyckoff:
> +# When the p4 client Root is a symlink, make sure chdir() does not use
> +# getcwd() to convert it to a physical path.
> +test_expect_failure 'p4 client root symlink should stay symbolic' '
> + physical="$TRASH_DIRECTORY/physical" &&
> + symbolic="$TRASH_DIRECTORY/symbolic" &&
> + test_when_finished "rm -rf \"$physical\"" &&
> + test_when_finished "rm \"$symbolic\"" &&
> + mkdir -p "$physical" &&
> + ln -s "$physical" "$symbolic" &&

This test needs a SYMLINKS prerequisite to future-proof it, in case the
Windows port gains p4 support some time.

> + test_when_finished cleanup_git &&
> + (
> + P4CLIENT=client-sym &&
> + p4 client -i <<-EOF &&
> + Client: $P4CLIENT
> + Description: $P4CLIENT
> + Root: $symbolic
> + LineEnd: unix
> + View: //depot/... //$P4CLIENT/...
> + EOF
> + git p4 clone --dest="$git" //depot &&
> + cd "$git" &&
> + test_commit file2 &&
> + git config git-p4.skipSubmitEdit true &&
> + git p4 submit
> + )
> +'

-- 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: [BUG] bare repository detection does not work with aliases

[Jeff King]

On Thu, Mar 07, 2013 at 10:27:04PM -0800, Junio C Hamano wrote:

> The $GIT_BARE idea sounds very sensible to me.

Unfortunately, it is not quite as simple as that. I just wrote up the
patch, and it turns out that we are foiled by how core.bare is treated.
If it is true, the repo is definitely bare. If it is false, that is only
a hint for us.

So we cannot just look at is_bare_repository() after setup_git_directory
runs. Because we are not "definitely bare", only "maybe bare", it
returns false. We just happen not to have a work tree. We could do
something like:

  if (is_bare_repository_cfg || !work_tree)
  setenv("GIT_BARE", "1", 1);

which I think would work, but feels kind of wrong. We are bare in this
instance, but somebody setting GIT_WORK_TREE in a sub-process would
want to become unbare, presumably, but our variable would override them.

Just looking through all of the code paths, I am getting a little
nervous that I would not cover all the bases for such a $GIT_BARE to
work (e.g., doing GIT_BARE=0 would not do I would expect as a user,
because of the historical way we treat core.bare=false).

So rather than introduce something like $GIT_BARE which is going to
bring about all new kinds of corner cases, I think I'd rather just pass
along a $GIT_NO_IMPLICIT_WORK_TREE variable, which is much more direct
for solving this problem, and is less likely to end up having bugs of
its own.

-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: [BUG] bare repository detection does not work with aliases

[Junio C Hamano]

The $GIT_BARE idea sounds very sensible to me.



Jeff King  wrote:

>On Thu, Mar 07, 2013 at 05:47:45PM -0500, Mark Lodato wrote:
>
>> It seems that the fallback bare repository detection in the absence
>of
>> core.bare fails for aliases.
>
>This triggered some deja vu for me, so I went digging. And indeed, this
>has been a bug since at least 2008. This patch (which never got
>applied)
>fixed it:
>
>  http://thread.gmane.org/gmane.comp.version-control.git/72792
>
>The issue is that we treat:
>
>  GIT_DIR=/some/path git ...
>
>as if the current directory is the work tree, unless core.bare is
>explicitly set, or unless an explicit work tree is given (via
>GIT_WORK_TREE, "git --work-tree", or in the config).  This is handy,
>and
>backwards compatible.
>
>Inside setup_git_directory, when we find the directory we put it in
>$GIT_DIR for later reference by ourselves or sub-programs (since we are
>typically moving to the top of the working tree next, we need to record
>the original path, and can't rely on discovery finding the same path
>again). But we don't set $GIT_WORK_TREE. So if you don't have core.bare
>set, the above rule will kick in for sub-programs, or for aliases
>(which
>will call setup_git_directory again).
>
>The solution is that when we set $GIT_DIR like this, we need to also
>say
>"no, there is no working tree; we are bare". And that's what that patch
>does. It's 5 years old now, so not surprisingly, it does not apply
>cleanly. The moral equivalent in today's code base would be something
>like:
>
>diff --git a/environment.c b/environment.c
>index 89d6c70..8edaedd 100644
>--- a/environment.c
>+++ b/environment.c
>@@ -200,7 +200,8 @@ void set_git_work_tree(const char *new_work_tree)
>   return;
>   }
>   git_work_tree_initialized = 1;
>-  work_tree = xstrdup(real_path(new_work_tree));
>+  if (*new_work_tree)
>+  work_tree = xstrdup(real_path(new_work_tree));
> }
> 
> const char *get_git_work_tree(void)
>diff --git a/setup.c b/setup.c
>index e1cfa48..f0e1251 100644
>--- a/setup.c
>+++ b/setup.c
>@@ -544,7 +544,7 @@ static const char *setup_explicit_git_dir(const
>char *gitdirenv,
>   worktree = get_git_work_tree();
> 
>   /* both get_git_work_tree() and cwd are already normalized */
>-  if (!strcmp(cwd, worktree)) { /* cwd == worktree */
>+  if (!worktree || !strcmp(cwd, worktree)) { /* cwd == worktree */
>   set_git_dir(gitdirenv);
>   free(gitfile);
>   return NULL;
>@@ -636,6 +636,8 @@ static const char *setup_bare_git_dir(char *cwd,
>int offset, int len, int *nongi
>   }
>   else
>   set_git_dir(".");
>+
>+  setenv(GIT_WORK_TREE_ENVIRONMENT, "", 1);
>   return NULL;
> }
> 
>
>which passes your test. Unfortunately, this patch runs afoul of the
>same
>complaints that prevented the original from being acceptable (weirdness
>on Windows with empty environment variables).
>
>Having read the discussion again, I _think_ the more sane thing is to
>actually just have a new variable, $GIT_BARE, which overrides any
>core.bare config (just as $GIT_WORK_TREE override core.worktree). And
>then we set that explicitly when we are in a bare $GIT_DIR, propagating
>our auto-detection to sub-processes.
>
>-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

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

Re: [BUG] bare repository detection does not work with aliases

[Jeff King]

On Thu, Mar 07, 2013 at 05:47:45PM -0500, Mark Lodato wrote:

> It seems that the fallback bare repository detection in the absence of
> core.bare fails for aliases.

This triggered some deja vu for me, so I went digging. And indeed, this
has been a bug since at least 2008. This patch (which never got applied)
fixed it:

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

The issue is that we treat:

  GIT_DIR=/some/path git ...

as if the current directory is the work tree, unless core.bare is
explicitly set, or unless an explicit work tree is given (via
GIT_WORK_TREE, "git --work-tree", or in the config).  This is handy, and
backwards compatible.

Inside setup_git_directory, when we find the directory we put it in
$GIT_DIR for later reference by ourselves or sub-programs (since we are
typically moving to the top of the working tree next, we need to record
the original path, and can't rely on discovery finding the same path
again). But we don't set $GIT_WORK_TREE. So if you don't have core.bare
set, the above rule will kick in for sub-programs, or for aliases (which
will call setup_git_directory again).

The solution is that when we set $GIT_DIR like this, we need to also say
"no, there is no working tree; we are bare". And that's what that patch
does. It's 5 years old now, so not surprisingly, it does not apply
cleanly. The moral equivalent in today's code base would be something
like:

diff --git a/environment.c b/environment.c
index 89d6c70..8edaedd 100644
--- a/environment.c
+++ b/environment.c
@@ -200,7 +200,8 @@ void set_git_work_tree(const char *new_work_tree)
return;
}
git_work_tree_initialized = 1;
-   work_tree = xstrdup(real_path(new_work_tree));
+   if (*new_work_tree)
+   work_tree = xstrdup(real_path(new_work_tree));
 }
 
 const char *get_git_work_tree(void)
diff --git a/setup.c b/setup.c
index e1cfa48..f0e1251 100644
--- a/setup.c
+++ b/setup.c
@@ -544,7 +544,7 @@ static const char *setup_explicit_git_dir(const char 
*gitdirenv,
worktree = get_git_work_tree();
 
/* both get_git_work_tree() and cwd are already normalized */
-   if (!strcmp(cwd, worktree)) { /* cwd == worktree */
+   if (!worktree || !strcmp(cwd, worktree)) { /* cwd == worktree */
set_git_dir(gitdirenv);
free(gitfile);
return NULL;
@@ -636,6 +636,8 @@ static const char *setup_bare_git_dir(char *cwd, int 
offset, int len, int *nongi
}
else
set_git_dir(".");
+
+   setenv(GIT_WORK_TREE_ENVIRONMENT, "", 1);
return NULL;
 }
 

which passes your test. Unfortunately, this patch runs afoul of the same
complaints that prevented the original from being acceptable (weirdness
on Windows with empty environment variables).

Having read the discussion again, I _think_ the more sane thing is to
actually just have a new variable, $GIT_BARE, which overrides any
core.bare config (just as $GIT_WORK_TREE override core.worktree). And
then we set that explicitly when we are in a bare $GIT_DIR, propagating
our auto-detection to sub-processes.

-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] setup.c: Fix prefix_pathspec from looping pass end of string

[Andrew Wong]

On 03/07/13 20:51, Junio C Hamano wrote:
> But it is equally broken to behave as if there is nothing wrong in
> the incomplete magic ":(top" that is not closed, isn't it?
Ah, yea, I did notice that, but then I saw a few lines below:
if (*copyfrom == ')')
copyfrom++;
which is explicitly making the ")" optional. So I thought maybe that was
the original intention, and left it at that. Though the doc says to end
with ")", so I guess it should error out after all? If that's the case,
I can try to come up with a patch to error it out (through die() ?).
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string

[Junio C Hamano]

Andrew Wong  writes:

> On 3/7/13, Junio C Hamano  wrote:
>> This did not error out for me, though.
>>
>> $ cd t && git ls-files ":(top"
>
> No error message at all? Hm, maybe in your case, the byte after the
> end of string happens to be '\0' and the loop ended by chance?
>
> git doesn't crash for me, but it generates this error:
> $ git ls-files ":(top"
> fatal: Invalid pathspec magic 'LS_COLORS=' in ':(top'

What I meant was that I do not get any error _after_ applying your
patch.

It is broken to behave as if "LS_COLORS=..." (which is totally
unrelated string that happens to be laid out next in the memory) is
a part of the pathspec magic specification your ":(top" started.
Your patch makes the code stop doing that.

But it is equally broken to behave as if there is nothing wrong in
the incomplete magic ":(top" that is not closed, isn't it?

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

Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string

[Andrew Wong]

On 3/7/13, Junio C Hamano  wrote:
> This did not error out for me, though.
>
> $ cd t && git ls-files ":(top"

No error message at all? Hm, maybe in your case, the byte after the
end of string happens to be '\0' and the loop ended by chance?

git doesn't crash for me, but it generates this error:
$ git ls-files ":(top"
fatal: Invalid pathspec magic 'LS_COLORS=' in ':(top'

The loop runs for a second time after parsing "top", and copyfrom now
points to the byte after ":(top", which is coming from argv. And in my
distribution/platform, it looks like the envp, the third param of
main(), is packed right after the argv strings, because:
$ env | head -n 1
LS_COLORS=
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: inotify to minimize stat() calls

[Junio C Hamano]

Torsten Bögershausen  writes:

> diff --git a/builtin/commit.c b/builtin/commit.c
> index d6dd3df..6a5ba11 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1158,6 +1158,8 @@ int cmd_status(int argc, const char **argv, const char 
> *prefix)
>   unsigned char sha1[20];
>   static struct option builtin_status_options[] = {
>   OPT__VERBOSE(&verbose, N_("be verbose")),
> + OPT_BOOLEAN('c', "changed-only", &s.check_changed_only,
> + N_("Ignore untracked files. Check if files known to 
> git are modified")),

Doesn't this make one wonder why a separate bit and implementation
is necessary to say "I am not interested in untracked files" when
"-uno" option is already there?


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

Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string

[Junio C Hamano]

Andrew Wong  writes:

> On 3/7/13, Junio C Hamano  wrote:
>> The parser that goes past the end of the string may be a bug worth
>> fixing, but is this patch sufficient to diagnose such an input as an
>> error?
>
> Yea, the patch should fix the passing end of string too. The parser
> was going past end of string because the nextat is set to "copyfrom +
> len + 1" for the '\0' case too. Then "+ 1" causes the parser to go
> pass end of string. If we handle the '\0' case separately, then the
> parser ends properly, and shouldn't be able to go pass the end of
> string.

This did not error out for me, though.

$ cd t && git ls-files ":(top"

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

Re: [feature request] 2) Remove many tags at once and 1) Prune tags on old-branch-before-rebase

[Junio C Hamano]

Eric Chamberland  writes:

> 1) git tag --delete-tags-to-danglings-and-unnamed-banches
>
> This would be able to remove all tags that refers to commits which are
> on branches that are no more referenced by any branch name.  This is
> happening when you tag something, then "git rebase".  Your tag will
> still be there on the old-and-before-rebase branch and won't be
> "pruned" by any git command... (that I know of...)

Not interesting for at least two reasons.

Why are "tags" any special?  "git branch --delete-merged" may also
be of interest, and for that matter "git update-ref -d" to deal with
any ref in general would be equally valid if such an option were a
good idea.

What you want is a way to compute, given a set of tags (or refs in
general) and a set of branches (or another set of refs in general),
find the ones in the former that none of the latter can reach.  With
that, you can drive "git tag -d $(that way)".

In other words, the feature does not belong to "git tag" command.

> 2) git tag -d "TOKEN*"

Again, not interesting.  You already have:

git for-each-ref --format='%(refname:short)' refs/tags/TOKEN\* |
xargs -r git tag -d
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 3/3] git p4: avoid expanding client paths in chdir

[Pete Wyckoff]

From: Miklós Fazekas 

The generic chdir() helper sets the PWD environment
variable, as that is what is used by p4 to know its
current working directory.  Normally the shell would
do this, but in git-p4, we must do it by hand.

However, when the path contains a symbolic link,
os.getcwd() will return the physical location.  If the
p4 client specification includes symlinks, setting PWD
to the physical location causes p4 to think it is not
inside the client workspace.  It complains, e.g.

Path /vol/bar/projects/foo/... is not under client root /p/foo

One workaround is to use AltRoots in the p4 client specification,
but it is cleaner to handle it directly in git-p4.

Other uses of chdir still require setting PWD to an
absolute path so p4 features like P4CONFIG work.  See
bf1d68f (git-p4: use absolute directory for PWD env
var, 2011-12-09).

[ pw: tweak patch and commit message ]

Thanks-to: John Keeping 
Signed-off-by: Pete Wyckoff 
---
 git-p4.py   | 29 ++---
 t/t9808-git-p4-chdir.sh |  2 +-
 2 files changed, 23 insertions(+), 8 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 647f110..7288c0b 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -79,12 +79,27 @@ def p4_build_cmd(cmd):
 real_cmd += cmd
 return real_cmd
 
-def chdir(dir):
-# P4 uses the PWD environment variable rather than getcwd(). Since we're
-# not using the shell, we have to set it ourselves.  This path could
-# be relative, so go there first, then figure out where we ended up.
-os.chdir(dir)
-os.environ['PWD'] = os.getcwd()
+def chdir(path, is_client_path=False):
+"""Do chdir to the given path, and set the PWD environment
+   variable for use by P4.  It does not look at getcwd() output.
+   Since we're not using the shell, it is necessary to set the
+   PWD environment variable explicitly.
+   
+   Normally, expand the path to force it to be absolute.  This
+   addresses the use of relative path names inside P4 settings,
+   e.g. P4CONFIG=.p4config.  P4 does not simply open the filename
+   as given; it looks for .p4config using PWD.
+
+   If is_client_path, the path was handed to us directly by p4,
+   and may be a symbolic link.  Do not call os.getcwd() in this
+   case, because it will cause p4 to think that PWD is not inside
+   the client path.
+   """
+
+os.chdir(path)
+if not is_client_path:
+path = os.getcwd()
+os.environ['PWD'] = path
 
 def die(msg):
 if verbose:
@@ -1624,7 +1639,7 @@ class P4Submit(Command, P4UserMap):
 new_client_dir = True
 os.makedirs(self.clientPath)
 
-chdir(self.clientPath)
+chdir(self.clientPath, is_client_path=True)
 if self.dry_run:
 print "Would synchronize p4 checkout in %s" % self.clientPath
 else:
diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index af8bd8a..09b2cc4 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -58,7 +58,7 @@ test_expect_success 'p4 client root would be relative due to 
clone --dest' '
 
 # When the p4 client Root is a symlink, make sure chdir() does not use
 # getcwd() to convert it to a physical path.
-test_expect_failure 'p4 client root symlink should stay symbolic' '
+test_expect_success 'p4 client root symlink should stay symbolic' '
physical="$TRASH_DIRECTORY/physical" &&
symbolic="$TRASH_DIRECTORY/symbolic" &&
test_when_finished "rm -rf \"$physical\"" &&
-- 
1.8.2.rc2.64.g8335025

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

[PATCH 2/3] git p4 test: should honor symlink in p4 client root

[Pete Wyckoff]

This test fails when the p4 client root includes
a symlink.  It complains:

Path /vol/bar/projects/foo/... is not under client root /p/foo

and dumps a traceback.

Signed-off-by: Pete Wyckoff 
---
 t/t9808-git-p4-chdir.sh | 27 +++
 1 file changed, 27 insertions(+)

diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index 55c5e36..af8bd8a 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -56,6 +56,33 @@ test_expect_success 'p4 client root would be relative due to 
clone --dest' '
)
 '
 
+# When the p4 client Root is a symlink, make sure chdir() does not use
+# getcwd() to convert it to a physical path.
+test_expect_failure 'p4 client root symlink should stay symbolic' '
+   physical="$TRASH_DIRECTORY/physical" &&
+   symbolic="$TRASH_DIRECTORY/symbolic" &&
+   test_when_finished "rm -rf \"$physical\"" &&
+   test_when_finished "rm \"$symbolic\"" &&
+   mkdir -p "$physical" &&
+   ln -s "$physical" "$symbolic" &&
+   test_when_finished cleanup_git &&
+   (
+   P4CLIENT=client-sym &&
+   p4 client -i <<-EOF &&
+   Client: $P4CLIENT
+   Description: $P4CLIENT
+   Root: $symbolic
+   LineEnd: unix
+   View: //depot/... //$P4CLIENT/...
+   EOF
+   git p4 clone --dest="$git" //depot &&
+   cd "$git" &&
+   test_commit file2 &&
+   git config git-p4.skipSubmitEdit true &&
+   git p4 submit
+   )
+'
+
 test_expect_success 'kill p4d' '
kill_p4d
 '
-- 
1.8.2.rc2.64.g8335025

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

[PATCH 1/3] git p4 test: make sure P4CONFIG relative path works

[Pete Wyckoff]

This adds a test for the fix in bf1d68f (git-p4: use absolute
directory for PWD env var, 2011-12-09).  It is necessary to
set PWD to an absolute path so that p4 can find files referenced
by non-absolute paths, like the value of the P4CONFIG environment
variable.

P4 does not open files directly; it builds a path by prepending
the contents of the PWD environment variable.

Signed-off-by: Pete Wyckoff 
---
 t/t9808-git-p4-chdir.sh | 14 ++
 1 file changed, 14 insertions(+)

diff --git a/t/t9808-git-p4-chdir.sh b/t/t9808-git-p4-chdir.sh
index dc92e60..55c5e36 100755
--- a/t/t9808-git-p4-chdir.sh
+++ b/t/t9808-git-p4-chdir.sh
@@ -42,6 +42,20 @@ test_expect_success 'P4CONFIG and relative dir clone' '
)
 '
 
+# Common setup using .p4config to set P4CLIENT and P4PORT breaks
+# if clone destination is relative.  Make sure that chdir() expands
+# the relative path in --dest to absolute.
+test_expect_success 'p4 client root would be relative due to clone --dest' '
+   test_when_finished cleanup_git &&
+   (
+   echo P4PORT=$P4PORT >git/.p4config &&
+   P4CONFIG=.p4config &&
+   export P4CONFIG &&
+   unset P4PORT &&
+   git p4 clone --dest="git" //depot
+   )
+'
+
 test_expect_success 'kill p4d' '
kill_p4d
 '
-- 
1.8.2.rc2.64.g8335025

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

[PATCH 0/3] fix git-p4 client root symlink problems

[Pete Wyckoff]

Miklós pointed out in

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

that when the p4 client root included a symlink, bad things
happen.  It is fixable, but inconvenient, to use an absolute path
in one's p4 client.  It's not too hard to be smarter about this
in git-p4.

Thanks to Miklós for the patch, and to John for the style
suggestions.  I wrote a couple of tests to make sure this part
doesn't break again.

This is maybe a bug introduced by bf1d68f (git-p4: use absolute
directory for PWD env var, 2011-12-09), but that's so long ago
that I don't think this is a candidate for maint.

-- Pete

Miklós Fazekas (1):
  git p4: avoid expanding client paths in chdir

Pete Wyckoff (2):
  git p4 test: make sure P4CONFIG relative path works
  git p4 test: should honor symlink in p4 client root

 git-p4.py   | 29 ++---
 t/t9808-git-p4-chdir.sh | 41 +
 2 files changed, 63 insertions(+), 7 deletions(-)

-- 
1.8.2.rc2.64.g8335025

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

[ANNOUNCE] Git v1.8.2-rc3

[Junio C Hamano]

A release candidate Git v1.8.2-rc3 is now available for testing
at the usual places.

The release tarballs are found at:

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

and their SHA-1 checksums are:

3fe30d85cea78a388d61ba79fe3a106fca41cfbe  git-1.8.2.rc3.tar.gz
4b378cf6129fa4c9355436b93a698dde2ed4ce7a  git-htmldocs-1.8.2.rc3.tar.gz
b18a1f2e70920b5028f1179cc4b362ad78a6f34c  git-manpages-1.8.2.rc3.tar.gz

Also the following public repositories all have a copy of the v1.8.2-rc3
tag and the master branch that the tag points at:

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

Git v1.8.2 Release Notes (draft)


Backward compatibility notes


In the next major release Git 2.0 (not *this* one), we will change the
behavior of the "git push" command.

When "git push [$there]" does not say what to push, we have used the
traditional "matching" semantics so far (all your branches were sent
to the remote as long as there already are branches of the same name
over there).  We will use the "simple" semantics that pushes the
current branch to the branch with the same name, only when the current
branch is set to integrate with that remote branch.  There is a user
preference configuration variable "push.default" to change this.

"git push $there tag v1.2.3" used to allow replacing a tag v1.2.3
that already exists in the repository $there, if the rewritten tag
you are pushing points at a commit that is a descendant of a commit
that the old tag v1.2.3 points at.  This was found to be error prone
and starting with this release, any attempt to update an existing
ref under refs/tags/ hierarchy will fail, without "--force".

When "git add -u" and "git add -A", that does not specify what paths
to add on the command line, is run from inside a subdirectory, the
scope of the operation has always been limited to the subdirectory.
Many users found this counter-intuitive, given that "git commit -a"
and other commands operate on the entire tree regardless of where you
are. In this release, these commands give warning in such a case and
encourage the user to say "git add -u/-A ." instead when restricting
the scope to the current directory.

At Git 2.0 (not *this* one), we plan to change these commands without
pathspec to operate on the entire tree.  Forming a habit to type "."
when you mean to limit the command to the current working directory
will protect you against the planned future change, and that is the
whole point of the new message (there will be no configuration
variable to squelch this warning---it goes against the "habit forming"
objective).


Updates since v1.8.1


UI, Workflows & Features

 * Initial ports to QNX and z/OS UNIX System Services have started.

 * Output from the tests is coloured using "green is okay, yellow is
   questionable, red is bad and blue is informative" scheme.

 * Mention of "GIT/Git/git" in the documentation have been updated to
   be more uniform and consistent.  The name of the system and the
   concept it embodies is "Git"; the command the users type is "git".
   All-caps "GIT" was merely a way to imitate "Git" typeset in small
   caps in our ASCII text only documentation and to be avoided.

 * The completion script (in contrib/completion) used to let the
   default completer to suggest pathnames, which gave too many
   irrelevant choices (e.g. "git add" would not want to add an
   unmodified path).  It learnt to use a more git-aware logic to
   enumerate only relevant ones.

 * In bare repositories, "git shortlog" and other commands now read
   mailmap files from the tip of the history, to help running these
   tools in server settings.

 * Color specifiers, e.g. "%C(blue)Hello%C(reset)", used in the
   "--format=" option of "git log" and friends can be disabled when
   the output is not sent to a terminal by prefixing them with
   "auto,", e.g. "%C(auto,blue)Hello%C(auto,reset)".

 * Scripts can ask Git that wildcard patterns in pathspecs they give do
   not have any significance, i.e. take them as literal strings.

 * The patterns in .gitignore and .gitattributes files can have **/,
   as a pattern that matches 0 or more levels of subdirectory.
   E.g. "foo/**/bar" matches "bar" in "foo" itself or in a
   subdirectory of "foo".

 * When giving arguments without "--" disambiguation, object names
   that come earlier on the command line must not be interpretable as
   pathspecs and pathspecs that come later on the command line must
   not be interpretable as object names.  This disambiguation rule has
   been tweaked so that ":/" (no other string before or after) is
   always interpreted as a pathspec; "git cmd -- :/" is no longer
   needed, you can just say "git cmd :/".

 * Various "hint" lines Git gives when it asks 

Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string

[Andrew Wong]

On 3/7/13, Junio C Hamano  wrote:
> The parser that goes past the end of the string may be a bug worth
> fixing, but is this patch sufficient to diagnose such an input as an
> error?

Yea, the patch should fix the passing end of string too. The parser
was going past end of string because the nextat is set to "copyfrom +
len + 1" for the '\0' case too. Then "+ 1" causes the parser to go
pass end of string. If we handle the '\0' case separately, then the
parser ends properly, and shouldn't be able to go pass the end of
string.

Hm, should I be paranoid and put an "else" clause to call die() as
well? In case there's a scenario where none of the 3 cases is true...

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

Re: inotify to minimize stat() calls

[Torsten Bögershausen]

On 11.02.13 03:56, Duy Nguyen wrote:
> On Mon, Feb 11, 2013 at 3:16 AM, Junio C Hamano  wrote:
>> The other "lstat()" experiment was a very interesting one, but this
>> is not yet an interesting experiment to see where in the "ignore"
>> codepath we are spending times.
>>
>> We know that we can tell wt_status_collect_untracked() not to bother
>> with the untracked or ignored files with !s->show_untracked_files
>> already, but I think the more interesting question is if we can show
>> the untracked files with less overhead.
>>
>> If we want to show untrackedd files, it is a given that we need to
>> read directories to see what paths there are on the filesystem. Is
>> the opendir/readdir cost dominating in the process? Are we spending
>> a lot of time sifting the result of opendir/readdir via the ignore
>> mechanism? Is reading the "ignore" files costing us much to prime
>> the ignore mechanism?
>>
>> If readdir cost is dominant, then that makes "cache gitignore" a
>> nonsense proposition, I think.  If you really want to "cache"
>> something, you need to have somebody (i.e. a daemon) who constantly
>> keeps an eye on the filesystem changes and can respond with the up
>> to date result directly to fill_directory().  I somehow doubt that
>> it is a direction we would want to go in, though.
> 
> Yeah, it did not cut out syscall cost, I also cut a lot of user-space
> processing (plus .gitignore content access). From the timings I posted
> earlier,
> 
>> unmodified  dir.c
>> real0m0.550s0m0.287s
>> user0m0.305s0m0.201s
>> sys 0m0.240s0m0.084s
> 
> sys time is reduced from 0.24s to 0.08s, so readdir+opendir definitely
> has something to do with it (and perhaps reading .gitignore). But it
> also reduces user time from 0.305 to 0.201s. I don't think avoiding
> readdir+openddir will bring us this gain. It's probably the cost of
> matching .gitignore. I'll try to replace opendir+readdir with a
> no-syscall version. At this point "untracked caching" sounds more
> feasible (and less complex) than ".gitignore cachine".
> 
Thanks for Duy for the measurements, and patches.
I took the freedom to convert the patched dir.c into a 
"runtime configurable" git status option.
I'm not sure if the following copy-and-paste work applies,
(it is based on Git 1.8.1.3), but the time spend for 
"git status --changed-only" is basically half the time of
"git status", similar to what Duy has measured.
I did a test both on a Linux box and Mac OS.

And the speedup is so impressive, that I am tempted to submit a patch simlar
to the following, what do we think about it?
/Torsten




-- >8 --

[PATCH] git status: add option changed-only
git status may be run faster if
- we only check if files are changed which are already known to git.
- we don't check if there are untracked files.

"git status --changed-only" (or the short form "git status -c")

will only check for changed files which are already known to git,
and which are in the index.

The call to read_directory_recursive() is skipped and untracked files
in the working tree are not reported.

Inspired-by: Duy Nguyen 
Signed-off-by: Torsten Bögershausen 
---
 builtin/commit.c | 2 ++
 dir.c| 5 +++--
 dir.h| 3 ++-
 wt-status.c  | 3 +++
 wt-status.h  | 1 +
 5 files changed, 11 insertions(+), 3 deletions(-)

diff --git a/builtin/commit.c b/builtin/commit.c
index d6dd3df..6a5ba11 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1158,6 +1158,8 @@ int cmd_status(int argc, const char **argv, const char 
*prefix)
unsigned char sha1[20];
static struct option builtin_status_options[] = {
OPT__VERBOSE(&verbose, N_("be verbose")),
+   OPT_BOOLEAN('c', "changed-only", &s.check_changed_only,
+   N_("Ignore untracked files. Check if files known to 
git are modified")),
OPT_SET_INT('s', "short", &status_format,
N_("show status concisely"), STATUS_FORMAT_SHORT),
OPT_BOOLEAN('b', "branch", &s.show_branch,
diff --git a/dir.c b/dir.c
index a473ca2..555b652 100644
--- a/dir.c
+++ b/dir.c
@@ -1274,8 +1274,9 @@ int read_directory(struct dir_struct *dir, const char 
*path, int len, const char
return dir->nr;
 
simplify = create_simplify(pathspec);
-   if (!len || treat_leading_path(dir, path, len, simplify))
-   read_directory_recursive(dir, path, len, 0, simplify);
+   if ((!(dir->flags & DIR_CHECK_CHANGED_ONLY)) &&
+   (!len || treat_leading_path(dir, path, len, simplify))) 
o
+   read_directory_recursive(dir, path, len, 0, simplify);
free_simplify(simplify);
qsort(dir->entries, dir->nr, sizeof(struct dir_entry *), cmp_name);
qsort(dir->ignored, dir->ignored_nr, sizeof(struct dir_entry *), 
cmp_name);
diff --git a/dir.h b/dir.h
index f5c89e3..1a915a7 100644
--- a/dir.h
+++ b/dir.h
@@ -41,7 +41,8 @@ struct dir_str

Re: What I want rebase to do

[Dale R. Worley]

> From: Thomas Rast 
> 
> wor...@alum.mit.edu (Dale R. Worley) writes:
> [...snip...]
> 
> Isn't that just a very long-winded way of restating what Junio said
> earlier:
> 
> > > It was suggested to make it apply the first-parent diff and record
> > > the result, I think.  If that were an acceptable approach (I didn't
> > > think about it through myself, though), that would automatically
> > > cover the evil-merge case as well.

Well, I believe what I said was a fleshed-out way of saying what I
*think* Junio said, but...

> You can fake that with something like
> 
> git rev-list --first-parent --reverse RANGE_TO_REBASE |
> while read rev; do
> if git rev-parse $rev^2 >/dev/null 2>&1; then
> git cherry-pick -n -m1 $rev
> git rev-parse $rev^2 >.git/MERGE_HEAD
> git commit -C$rev
> else
> git cherry-pick $rev
> fi
> done

This code doesn't do that.  I don't want something that rebases a
single thread of the current branch, I want something that rebases
*all* the commits between the head commit and the merge base.  Which
is what is illustrated in my message.

> [1]  If you don't get the sarcasm: that would amount to reinventing
> large parts of git-rebase.

Yes, that is the point of the exercise.

I've done a proof-of-concept implementation of what I want to see,
calling it git-rebase--merge-safe.  But I'm new here and likely that
is a pretty crude solution.  I suspect that a real implementation
could be done by inserting this logic into the framework of
git-filter-tree.  Following is git-rebase--merge-safe, and the script
I use to test it (and explore rebase problems).

Dale
--
git-rebase--merge-safe

#!/bin/bash

. git-sh-setup

prec=4

set -ex

# Ensure the work tree is clean.
require_clean_work_tree "rebase" "Please commit or stash them."

onto_name=$1
onto=$(git rev-parse --verify "${onto_name}^0") ||
die "Does not point to a valid commit: $1"

head_name=$( git symbolic-ref HEAD )
orig_head=$(git rev-parse --verify $head_name) ||
exit 1

echo onto=$onto
echo head_name=$head_name
echo orig_head=$orig_head

# Get the merge base, which is the root of the branch that we are rebasing.
# (For now, ignore the question of whether there is more than one merge base.)
mb=$(git merge-base "$onto" "$orig_head")
echo mb=$mb

# Get the list of commits to rebase, which is everything between $mb and
# $orig_head.
# Note that $mb is not included.
revisions=`git rev-list --reverse --ancestry-path $mb..$orig_head`
echo revisions=$revisions

# Set up the list mapping the commits on the original branch to the commits
# on the branch we are creating.
# Its format is ",old-hash1/new-hash1,old-hash2/new-hash2,...,".
# The initial value maps $mb to $onto.
map=",$mb/$onto,"

# Export these so git commit can see them.
export GIT_AUTHOR_NAME GIT_AUTHOR_EMAIL GIT_AUTHOR_DATE

# Process each commit in forward topological order.
for cmt in $revisions
do
# Examine the commit to extract information we will need to reconstruct it.
# First parent of the commit that has a mapping, i.e., is part of the
# branch (and has thus been rebuilt already.
first_mapped_parent=
# The new commit that was made of $first_mapped_parent.
first_mapped_parent_mapped=
# List of -p options naming the parent commits, or their new commits if they
# are in the branch.
parents=
   # Dissect the old commit's data.
# Output the commit data into FD 3.
exec 3< <( git cat-file commit $cmt )

while read keyword rest <&3
do
case $keyword in
tree)
# Ignored
;;
parent)
# See if the parent is mapped, i.e., is in the
# original branch.
if [[ "$map" == *,$rest/* ]]
then
# This parent has been mapped.  Get the new commit.
parent_mapped=${map#*,$rest/}
parent_mapped=${parent_mapped%%,*}
if test -z "$first_mapped_parent"
then
first_mapped_parent=$rest
first_mapped_parent_mapped=$parent_mapped
fi
else
# This parent has not been mapped.
parent_mapped=$rest
fi
# $parent_mapped is a parent of the new commit.
parents="$parents -p $parent_mapped"
;;
author)
# Extract the information about the author.
GIT_AUTHOR_NAME="${rest%% <*}"
GIT_AUTHOR_EMAIL="${rest##* <}"
GIT_AUTHOR_EMAIL="${GIT_AUTHOR_EMAIL%%> *}"
GIT_AUTHOR_DATE="${rest##*> }"
;;
committer)
# Ignored:  The new commit will have this use's name
# as committer.
;;
'')
 

[feature request] 2) Remove many tags at once and 1) Prune tags on old-branch-before-rebase

[Eric Chamberland]

Hi,


Short story:

we are now using *annotated* tags in a way that we would need to manage 
(remove) them easily. It would be usefull to have one of the folowing in 
"git tag":


1) git tag --delete-tags-to-danglings-and-unnamed-banches

This would be able to remove all tags that refers to commits which are 
on branches that are no more referenced by any branch name.  This is 
happening when you tag something, then "git rebase".  Your tag will 
still be there on the old-and-before-rebase branch and won't be "pruned" 
by any git command... (that I know of...)


Then you will end up to delete all of them "by hand"...

to do so you would like to have:

2) git tag -d "TOKEN*"

This would be able to delete all tags referred by the name.
Ok ok, I can do this like this:

rm .git/refs/tags/TOKEN*

but why have the git users "play" into the .git...?





Long story:


We started using annotated tags to hold information about the code 
"status", ie the results of our regression tests are stored in annotated 
tags each time you do a "make test" in the distribution.  We can 
retrieve the information by "git show" which we aliased to parse the 
output, extract the ".html" that we stored in the tag message  (about 
67kb) and then display the "make test" results as a web page in a 
browser... ;-)


We also "resume" the information on the number of "(P)assed" and 
"(F)ailed" tests in the tag name to quickly view the overall status of 
the code in the local clone.  For example, we end up with tags name like 
these:


SQA_ericc_ad76kj78_P_155_F_0

as a result of: SQA_${USER}_${SHA}_P_${PASSED}_F_${FAILED}

However, the number of tags increases as you do many "make test" to 
validate your developments and see the progressions you do.


Moreover, when you "git fetch" from colleagues, you retrieve their 
annotated tags too, which is nice, since you can view the results of the 
regression tests they have done, which can contain useful information to 
share...


BUT, when your colleagues delete their old tags because they rebased, 
you don't have the possibility to "git fetch --prune-tags", so you are 
left with all those "old" tags hanging all around and would like to 
easily remove them...


Because you know that no more branches are associated with the tip 
commit of all those "old-before-rebased-branches", you would like 
something like feature request #1 to automagically do the job you do by 
hand in feature request #2... ;-)


Ok, we shall not rebase but merge, but this is another long story... ;-)

Hope all this might be useful to other, so if other people start using 
tags like this, more will hope to have these features... ;-)




thanks,

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

Re: [gitweb] Removed reference to git.kernel.org

[Junio C Hamano]

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

Re: [PATCH] setup.c: Fix prefix_pathspec from looping pass end of string

[Junio C Hamano]

Andrew Wong  writes:

> The previous code was assuming length ends at either `)` or `,`, and was
> not handling the case where strcspn returns length due to end of string.
> So specifying ":(top" as pathspec will cause the loop to go pass the end
> of string.

Thanks.

The parser that goes past the end of the string may be a bug worth
fixing, but is this patch sufficient to diagnose such an input as an
error?




> Signed-off-by: Andrew Wong 
> ---
>  setup.c | 6 --
>  1 file changed, 4 insertions(+), 2 deletions(-)
>
> diff --git a/setup.c b/setup.c
> index 1dee47e..f4c4e73 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -207,9 +207,11 @@ static const char *prefix_pathspec(const char *prefix, 
> int prefixlen, const char
>*copyfrom && *copyfrom != ')';
>copyfrom = nextat) {
>   size_t len = strcspn(copyfrom, ",)");
> - if (copyfrom[len] == ')')
> + if (copyfrom[len] == '\0')
>   nextat = copyfrom + len;
> - else
> + else if (copyfrom[len] == ')')
> + nextat = copyfrom + len;
> + else if (copyfrom[len] == ',')
>   nextat = copyfrom + len + 1;
>   if (!len)
>   continue;
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 2/2] bundle: Add colons to list headings in "verify"

[Junio C Hamano]

Lukas Fleischer  writes:

> These slightly improve the reading flow by making it obvious that a list
> follows.
>
> Signed-off-by: Lukas Fleischer 

Perhaps.

The earlier message says "contains X ref(s)" while the later one
says "requires this/these X ref(s)".  Do we want to make them
consistent, too?

> ---
>  bundle.c | 8 
>  1 file changed, 4 insertions(+), 4 deletions(-)
>
> diff --git a/bundle.c b/bundle.c
> index 65db53b..8cd8b4f 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -183,8 +183,8 @@ int verify_bundle(struct bundle_header *header, int 
> verbose)
>   struct ref_list *r;
>  
>   r = &header->references;
> - printf_ln(Q_("The bundle contains %d ref",
> -  "The bundle contains %d refs",
> + printf_ln(Q_("The bundle contains %d ref:",
> +  "The bundle contains %d refs:",
>r->nr),
> r->nr);
>   list_refs(r, 0, NULL);
> @@ -192,8 +192,8 @@ int verify_bundle(struct bundle_header *header, int 
> verbose)
>   if (!r->nr) {
>   printf_ln(_("The bundle records a complete history."));
>   } else {
> - printf_ln(Q_("The bundle requires this ref",
> -  "The bundle requires these %d refs",
> + printf_ln(Q_("The bundle requires this ref:",
> +  "The bundle requires these %d refs:",
>r->nr),
> r->nr);
>   list_refs(r, 0, 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: Please pull l10n updates for 1.8.2 round 4

[Junio C Hamano]

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

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

[Junio C Hamano]

David Aguilar  writes:

> I would prefer to treat this as a bugfix rather than introducing
> a new set of configuration knobs if possible.  It really does
> seem like a correction.
> 
> Users that want the traditional behavior can get that by
> configuring a custom mergetool.p4merge.cmd, so we're not
> completely losing the ability to get at the old behavior.
>
> Users that want to see a reverse diff with difftool can
> already say "--reverse", so there's even less reason to
> have it there (though I know we're talking about mergetool only).
> ...
> I would much rather prefer to have the default/mainstream
> behavior be the best out-of-the-box sans configuration.
>
> The reasoning behind swapping them for p4merge makes sense
> for p4merge only.  I don't think we're quite ready to declare
> that all the merge tools need to be swapped or that we need a
> mechanism for swapping the order.

Thanks for an injection of sanity.

--
To unsubscribe from this list: send the line "unsubscribe 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] p4merge: swap LOCAL and REMOTE for mergetool

[David Aguilar]

On Thu, Mar 7, 2013 at 11:10 AM, Junio C Hamano  wrote:
> Kevin Bracey  writes:
>
>> On 07/03/2013 09:23, Junio C Hamano wrote:
>>> If p4merge GUI labels one side clearly as "theirs" and the other
>>> "ours", and the way we feed the inputs to it makes the side that is
>>> actually "ours" appear in p4merge GUI labelled as "theirs", then I
>>> do not think backward compatibility argument does not hold water. It
>>> is just correcting a longstanding 3-4 year old bug in a tool that
>>> nobody noticed.
>>
>> It's not quite that clear-cut. Some years ago, and before p4merge was
>> added as a Git mergetool, P4Merge was changed so its main GUI text
>> says "left" and "right" instead of "theirs" and "ours" when invoked
>> manually.
>>
>> But it appears that's as far as they went. It doesn't seem any of its
>> asymmetric diff display logic was changed; it works better with ours
>> on the right, and the built-in help all remains written on the
>> theirs/ours basis. And even little details like the icons imply it (a
>> square for the base, a downward-pointing triangle for their incoming
>> stuff, and a circle for the version we hold).
>
> So in short, a user of p4merge can see that left side is intended as
> "theirs", even though recent p4merge sometimes calls it "left".  And
> your description on the coloring (green vs blue) makes it clear that
> "left" and "theirs" are still intended to be synonyms.
>
> If that is the case I would think you can still argue such a change
> as "correcting a 3-4-year old bug".

I would prefer to treat this as a bugfix rather than introducing
a new set of configuration knobs if possible.  It really does
seem like a correction.

Users that want the traditional behavior can get that by
configuring a custom mergetool.p4merge.cmd, so we're not
completely losing the ability to get at the old behavior.

Users that want to see a reverse diff with difftool can
already say "--reverse", so there's even less reason to
have it there (though I know we're talking about mergetool only).


>> Would it be going too far to also have "xxxtool.reverse" to choose the
>> global default?
>
> It would be a natural thing to do.  I left it out because I thought
> it would go without saying, given that precedences already exist,
> e.g. mergetool.keepBackup etc.

Medium NACK.  If we can do without configuration all the better.

I would much rather prefer to have the default/mainstream
behavior be the best out-of-the-box sans configuration.

The reasoning behind swapping them for p4merge makes sense
for p4merge only.  I don't think we're quite ready to declare
that all the merge tools need to be swapped or that we need a
mechanism for swapping the order.

>> My only reservation is that I assume it would be implemented by
>> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
>> $LOCAL="a.REMOTE.1234.c".
>
> Doesn't the UI show the actual temporary filename?  When merging my
> version of hello.c with your version, showing them as hello.LOCAL.c
> and hello.REMOTE.c is an integral part of the UI experience, I
> think, even if the GUI tool does not give its own labels (and
> behaviour differences as you mentioned for p4merge) to mark which
> side is theirs and which side is ours.  The temporary file that
> holds their version should still be named with REMOTE, even when the
> mergetool.reverse option is in effect.
>
> As to the name of the variable, I do not care too deeply about it
> myself, but I think keeping the current LOCAL and REMOTE would help
> people following the code, especially given the option is called
> "reverse", meaning that there is an internal convention that the
> order is "LOCAL and then REMOTE".
>
> One thing to watch out for is from which temporary file we take the
> merged results.  You can present the two sides swapped, but if the
> tool always writes the results out by updating the second file, the
> caller needs to be prepared to read from the one that gets changed.
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

[Junio C Hamano]

Kevin Bracey  writes:

> On 07/03/2013 09:23, Junio C Hamano wrote:
>> If p4merge GUI labels one side clearly as "theirs" and the other
>> "ours", and the way we feed the inputs to it makes the side that is
>> actually "ours" appear in p4merge GUI labelled as "theirs", then I
>> do not think backward compatibility argument does not hold water. It
>> is just correcting a longstanding 3-4 year old bug in a tool that
>> nobody noticed.
>
> It's not quite that clear-cut. Some years ago, and before p4merge was
> added as a Git mergetool, P4Merge was changed so its main GUI text
> says "left" and "right" instead of "theirs" and "ours" when invoked
> manually.
>
> But it appears that's as far as they went. It doesn't seem any of its
> asymmetric diff display logic was changed; it works better with ours
> on the right, and the built-in help all remains written on the
> theirs/ours basis. And even little details like the icons imply it (a
> square for the base, a downward-pointing triangle for their incoming
> stuff, and a circle for the version we hold).

So in short, a user of p4merge can see that left side is intended as
"theirs", even though recent p4merge sometimes calls it "left".  And
your description on the coloring (green vs blue) makes it clear that
"left" and "theirs" are still intended to be synonyms.

If that is the case I would think you can still argue such a change
as "correcting a 3-4-year old bug".

> Would it be going too far to also have "xxxtool.reverse" to choose the
> global default?

It would be a natural thing to do.  I left it out because I thought
it would go without saying, given that precedences already exist,
e.g. mergetool.keepBackup etc.

> My only reservation is that I assume it would be implemented by
> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
> $LOCAL="a.REMOTE.1234.c".

Doesn't the UI show the actual temporary filename?  When merging my
version of hello.c with your version, showing them as hello.LOCAL.c
and hello.REMOTE.c is an integral part of the UI experience, I
think, even if the GUI tool does not give its own labels (and
behaviour differences as you mentioned for p4merge) to mark which
side is theirs and which side is ours.  The temporary file that
holds their version should still be named with REMOTE, even when the
mergetool.reverse option is in effect.

As to the name of the variable, I do not care too deeply about it
myself, but I think keeping the current LOCAL and REMOTE would help
people following the code, especially given the option is called
"reverse", meaning that there is an internal convention that the
order is "LOCAL and then REMOTE".

One thing to watch out for is from which temporary file we take the
merged results.  You can present the two sides swapped, but if the
tool always writes the results out by updating the second file, the
caller needs to be prepared to read from the one that gets changed.


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

Re: Merging submodules - best merge-base

[Heiko Voigt]

On Thu, Mar 07, 2013 at 10:49:09AM +0100, Daniel Bratell wrote:
> Den 2013-03-06 19:12:05 skrev Heiko Voigt :
> 
> >On Mon, Feb 25, 2013 at 05:44:05PM +0100, Daniel Bratell wrote:
> >>A submodule change can be merged, but only if the merge is a
> >>"fast-forward" which I think is a fair demand, but currently it
> >>checks if
> >>it's a fast-forward from a commit that might not be very interesting
> >>anymore.
> >>
> >>If two branches A and B split at a point when they used submodule commit
> >>S1 (based on S), and both then switched to S2 (also based on S)
> >>and B then
> >>switched to S21, then it's today not possible to merge B into A, despite
> >>S21 being a descendant of S2 and you get a conflict and this warning:
> >>
> >>warning: Failed to merge submodule S (commits don't follow merge-base)
> >>
> >>(attempt at ASCII gfx:
> >>
> >>Submodule tree:
> >>
> >>S  S1
> >>   \
> >>\ - S2 -- S21
> >>
> >>Main tree:
> >>
> >>A' (uses S1) --- A (uses S2)
> >>   \
> >>\ --- B' (uses S2) -- B (uses S21)
> >>
> >>
> >>I would like it to end up as:
> >>
> >>A' (uses S1) --- A (uses S2)  A+ (uses S21)
> >>   \ /
> >>\ --- B' (uses S2) -- B (uses S21)- /
> >>
> >>And that should be legal since S21 is a descendant of S2.
> >
> >So to summarize what you are requesting: You want a submodule merge be
> >two way in the view of the superproject and calculate the merge base
> >in the submodule from the two commits that are going to be merged?
> >
> >It currently sounds logical but I have to think about it further and
> >whether that might break other use cases.
> 
> Maybe both could be legal even. The current code can't be all wrong,
> and this case also seems to be straightforward.

Ok I have thought about it further and I did not come up with a simple
(and stable) enough strategy that would allow your use case to merge
cleanly without user interaction.

The problem is that your are actually doing a rewind from base to both
tips. The fact that a rewind is there makes git suspicious and we simply
give up. IMO, thats the right thing to do in such a situation.

What should a merge strategy do? It infers from two changes what the
final intention might be. For submodules we can do that when the changes
on both sides point forward. Since thats the typical progress of
development. If not there is some reason for it we do not know about. So
the merge gives up.

Please see this post about why we need to forbid rewinds from the
initial design discussion:

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

I am not saying that the current behavior is perfect but I think a merge
containing a rewind needs user support. We could give the user a hint
and say: "Hey I gave up but the two sides are contained in each other
and this is the commit containing both". Then the user can choose to use
that suggested solution. We already do the same for the merge commit
search.

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

Re: feature suggestion: optimize common parts for checkout --conflict=diff3

[Jeff King]

On Thu, Mar 07, 2013 at 10:40:46AM -0800, Junio C Hamano wrote:

> Where we differ is if such information loss is a good thing to have.
>
> We could say "both sides added, identically" is auto-resolved when
> you use the zealous option, and do so regardless of how the merge
> conflicts are presented.  Then it becomes perfectly fine to eject
> "A" and "E" out of the conflicted block and merge them to be part of
> pre/post contexts.  The same goes for reducing "" to "C".  As
> long as we clearly present the users what the option does and what
> its implications are, it is not bad to have such an option, I think.

Exactly. I do think it has real-world uses (see the example script I
posted yesterday), but it would never replace diff3. I'm going to try it
out for a bit. As I mentioned yesterday, I see those sorts of
cherry-pick-with-something-on-top conflicts when I am rebasing onto or
merging my topics into what you have picked up from the same topic on
the list.

I think the code in Uwe's patch looked fine, but it definitely needs a
documentation change to explain the new mode and its caveats. I'd also
be happy with a different name, if you think it implies that it is too
related to zdiff3, but I cannot think of anything better at the moment.

> > The wrong thing to me is the arbitrary choice about how to distribute
> > the preimage lines.
> 
> Yeah, but that is not "diff3 -m" vs "zealous-diff3" issue, is it?
> If you value the original and want to show it somewhere, you cannot
> avoid making the choice whether you are zealous or not if you split
> such a hunk.

Right, but I meant that we would never split a hunk like that with
diff3, because we would not do any hunk refinement at all.  Splitting a
hunk with "merge" is OK, because the "where does the preimage go"
problem does not exist there. zdiff3 is the only problematic case,
because it would be the only one that (potentially) splits and cares
about how the preimage maps to each hunk. But we can deal with that if
and when we ever do such splitting.

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

Re: [PATCH 4/4] teach config parsing to read from strbuf

[Ramsay Jones]

Heiko Voigt wrote:
> This can be used to read configuration values directly from gits
> database.
> 
> Signed-off-by: Heiko Voigt 
> ---
>  .gitignore |  1 +
>  Makefile   |  1 +
>  cache.h|  1 +
>  config.c   | 47 +++
>  t/t1300-repo-config.sh |  4 
>  test-config.c  | 41 +
>  6 files changed, 95 insertions(+)
>  create mode 100644 test-config.c
> 

[...]

> diff --git a/config.c b/config.c
> index 19aa205..492873a 100644
> --- a/config.c
> +++ b/config.c
> @@ -46,6 +46,37 @@ static long config_file_ftell(struct config *conf)
>   return ftell(f);
>  }
>  
> +struct config_strbuf {
> + struct strbuf *strbuf;
> + int pos;
> +};
> +
> +static int config_strbuf_fgetc(struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> +
> + if (str->pos < str->strbuf->len)
> + return str->strbuf->buf[str->pos++];
> +
> + return EOF;
> +}
> +
> +static int config_strbuf_ungetc(int c, struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> +
> + if (str->pos > 0)
> + return str->strbuf->buf[--str->pos];
> +
> + return EOF;
> +}
> +
> +static long config_strbuf_ftell(struct config *conf)
> +{
> + struct config_strbuf *str = conf->data;
> + return str->pos;
> +}
> +
>  #define MAX_INCLUDE_DEPTH 10
>  static const char include_depth_advice[] =
>  "exceeded maximum include depth (%d) while including\n"
> @@ -961,6 +992,22 @@ int git_config_from_file(config_fn_t fn, const char 
> *filename, void *data)
>   return ret;
>  }
>  
> +int git_config_from_strbuf(config_fn_t fn, struct strbuf *strbuf, void *data)
> +{
> + struct config top;
> + struct config_strbuf str;
> +
> + str.strbuf = strbuf;
> + str.pos = 0;
> +
> + top.data = &str;

You will definitely want to initialise 'top.name' here, rather
than let it take whatever value happens to be at that position
on the stack. In your editor, search for 'cf->name' and contemplate
each such occurrence.

> + top.fgetc = config_strbuf_fgetc;
> + top.ungetc = config_strbuf_ungetc;
> + top.ftell = config_strbuf_ftell;
> +
> + return do_config_from(&top, fn, data);
> +}
> +
>  const char *git_etc_gitconfig(void)
>  {
>   static const char *system_wide;
> diff --git a/t/t1300-repo-config.sh b/t/t1300-repo-config.sh
> index 3c96fda..3304bcd 100755
> --- a/t/t1300-repo-config.sh
> +++ b/t/t1300-repo-config.sh
> @@ -1087,4 +1087,8 @@ test_expect_success 'barf on incomplete string' '
>   grep " line 3 " error
>  '
>  
> +test_expect_success 'reading config from strbuf' '
> + test-config strbuf
> +'
> +
>  test_done
> diff --git a/test-config.c b/test-config.c
> new file mode 100644
> index 000..7a4103c
> --- /dev/null
> +++ b/test-config.c
> @@ -0,0 +1,41 @@
> +#include "cache.h"
> +
> +static const char *config_string = "[some]\n"
> + "   value = content\n";
> +
> +static int config_strbuf(const char *var, const char *value, void *data)
> +{
> + int *success = data;
> + if (!strcmp(var, "some.value") && !strcmp(value, "content"))
> + *success = 0;
> +
> + printf("var: %s, value: %s\n", var, value);
> +
> + return 1;
> +}
> +
> +static void die_usage(int argc, char **argv)
> +{
> + fprintf(stderr, "Usage: %s strbuf\n", argv[0]);
> + exit(1);
> +}
> +
> +int main(int argc, char **argv)
> +{
> + if (argc < 2)
> + die_usage(argc, argv);
> +
> + if (!strcmp(argv[1], "strbuf")) {
> + int success = 1;
> + struct strbuf buf = STRBUF_INIT;
> +
> + strbuf_addstr(&buf, config_string);
> + git_config_from_strbuf(config_strbuf, &buf, &success);
> +
> + return success;
> + }
> +
> + die_usage(argc, argv);
> +
> + return 1;
> +}
> 

Does the 'include' facility work from a strbuf? Should it?

Are you happy with the error handling/reporting?

Do the above additions to the test-suite give you confidence
that the code works as you intend?

ATB,
Ramsay Jones


--
To unsubscribe from this list: send the line "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] line-log: Fix sparse warnings

[Ramsay Jones]

Sparse issues the following warnings:

line-log.c:17:6: warning: symbol 'range_set_grow' was not declared. Should 
it be static?
line-log.c:25:6: warning: symbol 'range_set_init' was not declared. Should 
it be static?
line-log.c:33:6: warning: symbol 'range_set_release' was not declared. 
Should it be static?
line-log.c:41:6: warning: symbol 'range_set_copy' was not declared. Should 
it be static?
line-log.c:47:6: warning: symbol 'range_set_move' was not declared. Should 
it be static?
line-log.c:58:6: warning: symbol 'range_set_append' was not declared. 
Should it be static?
line-log.c:299:46: warning: Using plain integer as NULL pointer
line-log.c:444:12: warning: symbol 'parse_loc' was not declared. Should it 
be static?
line-log.c:681:49: warning: Using plain integer as NULL pointer
line-log.c:684:58: warning: Using plain integer as NULL pointer
builtin/log.c:120:57: warning: Using plain integer as NULL pointer
builtin/log.c:120:60: warning: Using plain integer as NULL pointer

In order to suppress the "... was not declared" warnings, we simply
add the static modifier to the declarations of those symbols, since
they do not need more than file scope.

In order to suppress the "NULL pointer" warnings, we simply replace
the use of the integer constant zero as a representation of the null
pointer with the NULL symbol.

Signed-off-by: Ramsay Jones 
---

Hi Thomas,

If you need to re-roll the patches in your 'tr/line-log' branch, could
you please squash these changes in. [Note that this patch applies to the
tip of the pu branch ;-) ]

Also, I noticed some things in passing, viz:

   971  static void load_tree_desc(struct tree_desc *desc, void **tree,
   972  const unsigned char *sha1)
   973  {
   974  unsigned long size;
   975  *tree = read_object_with_reference(sha1, tree_type, &size, 
NULL);
   976  if (!tree)
   977  die("Unable to read tree (%s)", sha1_to_hex(sha1));
   978  init_tree_desc(desc, *tree, size);
   979  }

The die() on line 977 will never trigger, since (given that !tree
holds) we will already have dumped core on line 975! ;-)

  1401  int line_log_filter(struct rev_info *rev)
  1402  {
  1403  struct commit *commit;
  1404  struct commit_list *list = rev->commits;
  1405  struct commit_list *out = NULL, *cur = NULL;
  1406
  1407  list = rev->commits;
  1408  while (list) {

Note that the assignment on line 1407 is redundant and can be
removed; list has been initialized to the same value in it's
declaration on line 1404.

HTH

ATB,
Ramsay Jones

 builtin/log.c |  2 +-
 line-log.c| 20 ++--
 2 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/builtin/log.c b/builtin/log.c
index c5d2313..a551d8d 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -117,7 +117,7 @@ static void cmd_log_init_finish(int argc, const char 
**argv, const char *prefix,
 {
struct userformat_want w;
int quiet = 0, source = 0, mailmap = 0;
-   static struct line_opt_callback_data line_cb = {0, 0, 
STRING_LIST_INIT_DUP};
+   static struct line_opt_callback_data line_cb = {NULL, NULL, 
STRING_LIST_INIT_DUP};
 
const struct option builtin_log_options[] = {
OPT_BOOLEAN(0, "quiet", &quiet, N_("suppress diff output")),
diff --git a/line-log.c b/line-log.c
index a74bbaf..4d01798 100644
--- a/line-log.c
+++ b/line-log.c
@@ -14,7 +14,7 @@
 #include "userdiff.h"
 #include "line-log.h"
 
-void range_set_grow (struct range_set *rs, size_t extra)
+static void range_set_grow (struct range_set *rs, size_t extra)
 {
ALLOC_GROW(rs->ranges, rs->nr + extra, rs->alloc);
 }
@@ -22,7 +22,7 @@ void range_set_grow (struct range_set *rs, size_t extra)
 /* Either initialization would be fine */
 #define RANGE_SET_INIT {0}
 
-void range_set_init (struct range_set *rs, size_t prealloc)
+static void range_set_init (struct range_set *rs, size_t prealloc)
 {
rs->alloc = rs->nr = 0;
rs->ranges = NULL;
@@ -30,7 +30,7 @@ void range_set_init (struct range_set *rs, size_t prealloc)
range_set_grow(rs, prealloc);
 }
 
-void range_set_release (struct range_set *rs)
+static void range_set_release (struct range_set *rs)
 {
free(rs->ranges);
rs->alloc = rs->nr = 0;
@@ -38,13 +38,13 @@ void range_set_release (struct range_set *rs)
 }
 
 /* dst must be uninitialized! */
-void range_set_copy (struct range_set *dst, struct range_set *src)
+static void range_set_copy (struct range_set *dst, struct range_set *src)
 {
range_set_init(dst, src->nr);
memcpy(dst->ranges, src->ranges, src->nr*sizeof(struct range_set));
dst->nr = src->nr;
 }
-void range_set_move (struct range_set *dst, struct range_set *src)
+static void range_set_move (struct range_set *dst, struct range_set *src)
 {
range_set_release(dst);
dst->ranges = src->ranges;
@@ -55,7 +55,7 

Re: feature suggestion: optimize common parts for checkout --conflict=diff3

[Junio C Hamano]

Jeff King  writes:

> I'm not sure I agree. In this output (which does the zealous
> simplification, the splitting, and arbitrarily assigns deleted preimage
> to the first of the split hunks):
>
>   1234ACE789
>
> I do not see the promotion of C to "already resolved, you cannot tell if
> it was really in the preimage or not" as any more or less misleading or
> wrong than that of A or E.  It is no more misleading than what the
> merge-marker case would do, which would be:
>
>   1234ACE789

That is exactly my point and I think we are in complete agreement.
While the intended difference between RCS merge and diff3 -m is for
the latter not to lose information on the original, zealous-diff3
chooses to lose information in "both sides added, identically" case.

Where we differ is if such information loss is a good thing to have.

We could say "both sides added, identically" is auto-resolved when
you use the zealous option, and do so regardless of how the merge
conflicts are presented.  Then it becomes perfectly fine to eject
"A" and "E" out of the conflicted block and merge them to be part of
pre/post contexts.  The same goes for reducing "" to "C".  As
long as we clearly present the users what the option does and what
its implications are, it is not bad to have such an option, I think.

> The wrong thing to me is the arbitrary choice about how to distribute
> the preimage lines.

Yeah, but that is not "diff3 -m" vs "zealous-diff3" issue, is it?
If you value the original and want to show it somewhere, you cannot
avoid making the choice whether you are zealous or not if you split
such a hunk.

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

[PATCH] Fix revision walk for commits with the same dates

[Kacper Kornet]

git rev-list A^! --not B provides wrong answer if all commits in the
range A..B had the same commit times and there are more then 8 of them.
This commits fixes the logic in still_interesting function to prevent
this error.

Signed-off-by: Kacper Kornet 
---
 revision.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/revision.c b/revision.c
index ef60205..cf620c6 100644
--- a/revision.c
+++ b/revision.c
@@ -709,7 +709,7 @@ static int still_interesting(struct commit_list *src, 
unsigned long date, int sl
 * Does the destination list contain entries with a date
 * before the source list? Definitely _not_ done.
 */
-   if (date < src->item->date)
+   if (date <= src->item->date)
return SLOP;
 
/*
-- 
1.8.2.rc2
--
To unsubscribe from this list: send the line "unsubscribe 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] p4merge: swap LOCAL and REMOTE for mergetool

[Kevin Bracey]

On 07/03/2013 09:23, Junio C Hamano wrote:
If p4merge GUI labels one side clearly as "theirs" and the other 
"ours", and the way we feed the inputs to it makes the side that is 
actually "ours" appear in p4merge GUI labelled as "theirs", then I do 
not think backward compatibility argument does not hold water. It is 
just correcting a longstanding 3-4 year old bug in a tool that nobody 
noticed.


It's not quite that clear-cut. Some years ago, and before p4merge was 
added as a Git mergetool, P4Merge was changed so its main GUI text says 
"left" and "right" instead of "theirs" and "ours" when invoked manually.


But it appears that's as far as they went. It doesn't seem any of its 
asymmetric diff display logic was changed; it works better with ours on 
the right, and the built-in help all remains written on the theirs/ours 
basis. And even little details like the icons imply it (a square for the 
base, a downward-pointing triangle for their incoming stuff, and a 
circle for the version we hold).



For people who are very used to the way p4merge shows the merged
contents by theirs-base-yours order in side-by-side view, I do not
think it is unreasonable to introduce the "mergetool.$name.reverse"
configuration variable and teach the mergetool frontend to pay
attention to it.  That will allow them to see their merge in reverse
order even when they are using a backend other than p4merge.

With such a mechanism in place, by default, we can just declare that
mergetool.p4merge.reverse is "true" when unset, while making
mergetool.$name.reverse for all the other tools default to "false".
People who are already used to the way our p4merge integration works
can set mergetool.p4merge.reverse to "false" explicitly to retain
the historical behaviour that you are declaring "buggy" with such a
change.


I like this idea as a user - having made this change to p4merge, it does 
throw me when I decide to attempt a particularly tricky merge with bc3 
instead, and get the other order. The user config options you suggest 
sound good to me.


For completion on this idea, I'd suggest difftool.xxx.reverse, to allow 
the orientation for 0- and 1-revision diffs to be chosen - allow the 
implied working tree version to be on the left or right. That would 
allow "ours-theirs" order, which some would view as being more 
consistent with the "ours-base-theirs" default for mergetool.


Would it be going too far to also have "xxxtool.reverse" to choose the 
global default? Then the choice hierarchy would be "xxxtool.xxx.reverse 
if set" > "optional inbuilt tool preference" > "xxxtool.reverse if set" 
> "false". So the user could request a global swap, except that they'd 
have to explicitly override any tools that have a preferred orientation.


My only reservation is that I assume it would be implemented by swapping 
what's passed in $LOCAL and $REMOTE. Which seems a bit icky: 
$LOCAL="a.REMOTE.1234.c". On the other hand, $LOCAL and $REMOTE are 
already not very meaningful names for difftool... Maybe we should change 
to using $LEFT and $RIGHT, acknowledging the existing difftool 
situation, and that the user can now swap merges too.


I'd be happy to prepare a fuller patch on this sort of basis.

Kevin

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

Re: feature suggestion: optimize common parts for checkout --conflict=diff3

[Junio C Hamano]

Junio C Hamano  writes:

> You could make it "1234789", and that is
> technically correct (what there were in the shared original for the
> conflicted part is 5 and then 6), but the representation pretends
> that it knows more than there actually is information, which may be
> somewhat misleading.  All these three are equally plausible split of
> the original "56":
>
>   1234789
>   1234789
>   1234789
>
> and picking one over others would be a mere heuristic.  All three
> are technically correct representations and it is just the matter of
> which one is the easiest to understand.  So, this is the kind of
> "misleading but not incorrect".

I forgot to say that youu could even do something silly like:

1234789

;-)

> In all these cases, the middle part would look like this:
>
>   <<< ours
>   C
>   ||| base
>   ===
>   C
>   >>> theirs
>
> in order to honor the explicit "I want to view all three versions to
> examine the situation" aka "--conflict=diff3" option.  We cannot
> reduce it to just "C".  That will make it "not just misleading but
> is actively wrong".

I also forgot to say that the issue is the same to reduce

1234789

to

1234789

which is unconditionally correct and then for all x reduce  to
x, yielding

1234ACE789

which your zealous-diff3 would do.  So squashing that  in the
middle would be consistent if you take the zealous-diff3 route.

But again, that is discarding the information of the original, which
the user explicitly asked from "diff3 -m", i.e. show all three to
examine the situation. If the user wants to operate _without_ the
original, the user would have asked for "RCS merge" style output, so
I am still not sure if that is a sensible mode of operation for diff3
to begin with.




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

Re: feature suggestion: optimize common parts for checkout --conflict=diff3

[Jeff King]

On Thu, Mar 07, 2013 at 09:26:05AM -0800, Junio C Hamano wrote:

> Without thinking about it too deeply,...
> 
> I think the "RCS merge" _could_ show it as "1234ACE789"
> without losing any information (as it is already discarding what was
> in the original in the part that is affected by the conflict,
> i.e. "56 was there").

Right, I think that is sane, though we do not do that at this point.

> Let's think aloud how "diff3 -m" _should_ split this. The most
> straight-forward representation would be "1234789",
> that is, where "56" was originally there, one side made it to
> "ABCDE" and the other "AXCYE".

Yes, that is what diff3 would do now (because it does not do any hunk
refinement at all), and should continue doing.

> You could make it "1234789", and that is
> technically correct (what there were in the shared original for the
> conflicted part is 5 and then 6), but the representation pretends
> that it knows more than there actually is information, which may be
> somewhat misleading.  All these three are equally plausible split of
> the original "56":
> 
>   1234789
>   1234789
>   1234789
> 
> and picking one over others would be a mere heuristic.  All three
> are technically correct representations and it is just the matter of
> which one is the easiest to understand.  So, this is the kind of
> "misleading but not incorrect".

Yes, I agree it is a heuristic about which part of a split hunk to place
deleted preimage lines in. Conceptually, I'm OK with that; the point of
zdiff3 is to try to make the conflict easier to read by eliminating
possibly uninteresting parts. It doesn't have to be right all the time;
it just has to be useful most of the time. But it's not clear how true
that would be in real life.

I think this is somewhat a moot point, though. We do not do this
splitting now. If we later learn to do it, there is nothing to say that
zdiff3 would have to adopt it also; it could stop at a lower
zealous-level than the regular merge markers. I think I'd want to
experiment with it and see some real-world examples before making a
decision on that.

> In all these cases, the middle part would look like this:
> 
>   <<< ours
> C
> ||| base
> ===
>   C
> >>> theirs
> 
> in order to honor the explicit "I want to view all three versions to
> examine the situation" aka "--conflict=diff3" option.  We cannot
> reduce it to just "C".  That will make it "not just misleading but
> is actively wrong".

I'm not sure I agree. In this output (which does the zealous
simplification, the splitting, and arbitrarily assigns deleted preimage
to the first of the split hunks):

  1234ACE789

I do not see the promotion of C to "already resolved, you cannot tell if
it was really in the preimage or not" as any more or less misleading or
wrong than that of A or E.  It is no more misleading than what the
merge-marker case would do, which would be:

  1234ACE789

The wrong thing to me is the arbitrary choice about how to distribute
the preimage lines. In this example, it is not a big deal for the
heuristic to be wrong; you can see both of the hunks. But if C is long,
and you do not even see D=Y while resolving B=X, seeing the preimage
there may become nonsensical.

But again, we don't do this splitting now. So I don't think it's
something that should make or break a decision to have zdiff3. Without
the splitting, I can see it being quite useful. I'm going to carry the
patch in my tree for a while and try using it in practice for a while.

-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] add: Clarify documentation of -A and -u

[Junio C Hamano]

Greg Price  writes:

> On Wed, Mar 06, 2013 at 09:10:57AM -0800, Junio C Hamano wrote:
>> I do not know if mentioning what -A does in the description for -u
>> (and vice versa) makes it easier to understand or more confusing
>> (not rhetorical: I suspect some may find it easier and others not).
>> 
>> But "and the default behaviour will..." here _is_ confusing.  After
>> reading this patch three times, I still cannot tell what "default"
>> you are trying to describe.  Is it "-u" without arguments?  Is it
>> "add" without "-u" nor "-A"?  Is it something else???
>
> I meant the behavior without -A or -u.  This could be fixed directly
> by s/the default behavior will/with neither -A nor -u we/.

When we have bulletted list that enumerates options and describes
what each option does and how each option affects the behaviour, I'd
prefer to see us *not* talking about what happens when you do *not*
give that option, unless it makes it hard to understand that option
without such an extra description.  The overall description of what
the command does without the options should go to the top level.

> Here's a crack at making those distinctions clear.  I've also tried to
> make the descriptions as parallel as possible, as what they're saying
> is very similar.
>
> -u::
> --update::
>   Update the index just where it already has an entry matching
>   .

Good; this was the short phrasing I was looking for but couldn't
come up with myself without repeating "the index" over and over.

> Then follow both with the "If no " paragraph.  I just
> noticed that the paragraph actually needs a small modification to fit
> '-A', too.  New patch below.
>
> Greg
>
> From: Greg Price 
> Date: Thu, 7 Mar 2013 02:08:21 -0800
> Subject: [PATCH] add: Clarify documentation of -A and -u

(for future reference) Drop the three lines and have "-- >8 --" here.

[patch kept unsnipped for others']

> The documentation of '-A' and '-u' is very confusing for someone who
> doesn't already know what they do.  Describe them with fewer words and
> clearer parallelism to each other and to the behavior of plain 'add'.
>
> Also mention the default  for '-A' as well as '-u', because
> it applies to both.
>
> Signed-off-by: Greg Price 
> ---
>  Documentation/git-add.txt | 22 --
>  1 file changed, 12 insertions(+), 10 deletions(-)
>
> diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
> index 388a225..b0944e5 100644
> --- a/Documentation/git-add.txt
> +++ b/Documentation/git-add.txt
> @@ -100,12 +100,9 @@ apply to the index. See EDITING PATCHES below.
>  
>  -u::
>  --update::
> - Only match  against already tracked files in
> - the index rather than the working tree. That means that it
> - will never stage new files, but that it will stage modified
> - new contents of tracked files and that it will remove files
> - from the index if the corresponding files in the working tree
> - have been removed.
> + Update the index just where it already has an entry matching
> + .  This removes as well as modifies index entries to
> + match the working tree, but adds no new files.
>  +
>  If no  is given, the current version of Git defaults to
>  "."; in other words, update all tracked files in the current directory
> @@ -114,10 +111,15 @@ of Git, hence the form without  should not be 
> used.
>  
>  -A::
>  --all::
> - Like `-u`, but match  against files in the
> - working tree in addition to the index. That means that it
> - will find new files as well as staging modified content and
> - removing files that are no longer in the working tree.
> + Update the index not only where the working tree has a file
> + matching  but also where the index already has an
> + entry.  This adds, modifies, and removes index entries to
> + match the working tree.
> ++
> +If no  is given, the current version of Git defaults to
> +"."; in other words, update all files in the current directory
> +and its subdirectories. This default will change in a future version
> +of Git, hence the form without  should not be used.
>  
>  -N::
>  --intent-to-add::
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What I want rebase to do

[Junio C Hamano]

Thomas Rast  writes:

> I still think that the _right_ solution is first redoing the merge on
> its original parents and then seeing how the actual merge differs from
> that.

I think that is what was suggested in

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

> Perhaps a new option to git-rebase could trigger the above behavior for
> merges, who knows.  (It could be called --first-parent.)

Yeah, I think that is what the old thread concluded to be the way to
move forward:

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

I'll throw it in to the "leftover bits".

http://git-blame.blogspot.com/2013/02/more-leftover-bits.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: feature suggestion: optimize common parts for checkout --conflict=diff3

[Junio C Hamano]

Jeff King  writes:

> I was also curious whether it would the diff3/zealous combination would
> trigger any weird corner cases. In particular, I wanted to know how the
> example you gave in that commit of:
>
>   postimage#1: 1234ABCDE789
>   |/
>   |   /
>   preimage:123456789
>   |   \
>   |\
>   postimage#2: 1234AXCYE789
>
> would react with diff3 (this is not the original example, but one with
> an extra "C" in the middle of postimage#2, which could in theory be
> presented as split hunks). However, it seems that we do not do such hunk
> splitting at all, neither for diff3 nor for the "merge" representation.

Without thinking about it too deeply,...

I think the "RCS merge" _could_ show it as "1234ACE789"
without losing any information (as it is already discarding what was
in the original in the part that is affected by the conflict,
i.e. "56 was there").

Let's think aloud how "diff3 -m" _should_ split this. The most
straight-forward representation would be "1234789",
that is, where "56" was originally there, one side made it to
"ABCDE" and the other "AXCYE".

You could make it "1234789", and that is
technically correct (what there were in the shared original for the
conflicted part is 5 and then 6), but the representation pretends
that it knows more than there actually is information, which may be
somewhat misleading.  All these three are equally plausible split of
the original "56":

1234789
1234789
1234789

and picking one over others would be a mere heuristic.  All three
are technically correct representations and it is just the matter of
which one is the easiest to understand.  So, this is the kind of
"misleading but not incorrect".

In all these cases, the middle part would look like this:

<<< ours
C
||| base
===
C
>>> theirs

in order to honor the explicit "I want to view all three versions to
examine the situation" aka "--conflict=diff3" option.  We cannot
reduce it to just "C".  That will make it "not just misleading but
is actively wrong".
--
To unsubscribe from this list: send the line "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] setup.c: Fix prefix_pathspec from looping pass end of string

[Andrew Wong]

The previous code was assuming length ends at either `)` or `,`, and was
not handling the case where strcspn returns length due to end of string.
So specifying ":(top" as pathspec will cause the loop to go pass the end
of string.

Signed-off-by: Andrew Wong 
---
 setup.c | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/setup.c b/setup.c
index 1dee47e..f4c4e73 100644
--- a/setup.c
+++ b/setup.c
@@ -207,9 +207,11 @@ static const char *prefix_pathspec(const char *prefix, int 
prefixlen, const char
 *copyfrom && *copyfrom != ')';
 copyfrom = nextat) {
size_t len = strcspn(copyfrom, ",)");
-   if (copyfrom[len] == ')')
+   if (copyfrom[len] == '\0')
nextat = copyfrom + len;
-   else
+   else if (copyfrom[len] == ')')
+   nextat = copyfrom + len;
+   else if (copyfrom[len] == ',')
nextat = copyfrom + len + 1;
if (!len)
continue;
-- 
1.8.2.rc0.22.gb3600c3

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

Re: Questions/investigations on git-subtree and tags

[Jeremy Rosen]

> 
> I think I tried adding the ^{} syntax, but I don't think it works on
> remote repos. Or I couldn't get the right syntax.
> 

indeed, it doesn't work on fetch, but it could be used somewhere between the 
fetch and the commit-tree to move from the ref to the associated commit




> 
> Latest patch:
> 
>   http://thread.gmane.org/gmane.comp.version-control.git/217257
> 

oh, that patch, yes I found it while looking around it is a step in the right 
direction but it doesn't help in my case since i'm using a valid remote ref 
that can be fetched

(on a side note you could use git ls-remote to check for the remote ref and 
avoid a fetch in case of an incorrect ref, but that's just a detail)



I just tested with it and here is what happens

git subtree add --squash -P br2 git://git.buildroot.net/buildroot 2013.02 => 
works ok, br2 is created

however the message of the squash commit is 


Squashed 'br2/' content from commit f1d2c19

git-subtree-dir: br2
git-subtree-split: f1d2c19091e1c2ef803ec3267fe71cf6ce7dd948


which is not correct :

git ls-remote git://git.buildroot.net/buildroot 2013.02
f1d2c19091e1c2ef803ec3267fe71cf6ce7dd948refs/tags/2013.02

git ls-remote git://git.buildroot.net/buildroot 2013.02^{}
15ace1a845c9e7fc65b648bbaf4dd14e03d938fdrefs/tags/2013.02^{}


as you can see git subtee thinks it splited from the tag SHA instead of the 
tag's commit SHA

this is incorrect because the tag isn't here, and at split time git subtree 
won't be able to find the correct ancestor. We just need to make sure we use 
the tag's commit instead
of the tag



changing
revs=FETCH_HEAD
to
revs=FETCH_HEAD^{}
in cmd_add_repository

seems to fix it, both for remote branch pull and remote tag pull


we still have a bug lurking around it's the case where the user does the fetch 
himself then use subtree add with a tag SHA. but let's discuss problems one at 
a time :)




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

Re: Questions/investigations on git-subtree and tags

[Paul Campbell]

On Thu, Mar 7, 2013 at 3:15 PM, Jeremy Rosen  wrote:
>> >
>> > Ok, I can understand that you don't want to import tags for
>> > namespace reason, but in that case shouldn't
>> > git subtree add refuse to create a subtree when the tag isn't a
>> > commit
>>
>> It shouldn't and tries not to, but is limited in it's ability to
>> identify if a refspec points to a commit or not in the remote repo.
>>
>
> ok, i've studied a little more
>
> * the target for "git subtree add   can only be a remote branch 
> or tag, since we git fetch
> can only target remote refs.
> * in case of a branch, git subtree forgets the branch and only use the commit 
> linked to the branch. for
> tags, the fetch part is ok, it's the merge part that fail. adding ^{} at the 
> right place would probably fix that

I think I tried adding the ^{} syntax, but I don't think it works on
remote repos. Or I couldn't get the right syntax.

>>
>> I've posted a patch (which is pending a lot of other changes to
>> git-subtree that I'm corralling) that tries to prevent some obvious
>> errors in the refspec. But letting the git fetch used by git-subtree
>> add and git-subtree pull catch the error and report it may be the
>> best
>> option.
>>
>
> that's interesting... do you have a link ?

Latest patch:

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

Prior patch with comments from Junio on what was probably going on
with the old tests:

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

>>
>> I've never really tried using --squash, I don't see that it adds any
>> value for me.
>>
>
> my project has a git subtree for a linux kernel and another subtree for 
> buildroot,
>
> a default .git is about 1.5G, squashing it reduces it to 200M so it's worth 
> it for me :)

If disk space is the issue, or bandwidth for initial cloning, then
sure, but I thought Git was efficient enough that a large repo
wouldn't give much of a performance hit.  Unless you use git-subtree
split or push, they are slow.

If git-subtree split could be optimised then --squash wouldn't be
needed as much. It does take an age compared to other Git operations.

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

Re: Questions/investigations on git-subtree and tags

[Jeremy Rosen]

> >
> > Ok, I can understand that you don't want to import tags for
> > namespace reason, but in that case shouldn't
> > git subtree add refuse to create a subtree when the tag isn't a
> > commit
> 
> It shouldn't and tries not to, but is limited in it's ability to
> identify if a refspec points to a commit or not in the remote repo.
> 

ok, i've studied a little more

* the target for "git subtree add   can only be a remote branch 
or tag, since we git fetch 
can only target remote refs.
* in case of a branch, git subtree forgets the branch and only use the commit 
linked to the branch. for 
tags, the fetch part is ok, it's the merge part that fail. adding ^{} at the 
right place would probably fix that


> 
> I've posted a patch (which is pending a lot of other changes to
> git-subtree that I'm corralling) that tries to prevent some obvious
> errors in the refspec. But letting the git fetch used by git-subtree
> add and git-subtree pull catch the error and report it may be the
> best
> option.
> 

that's interesting... do you have a link ? 


> 
> I've never really tried using --squash, I don't see that it adds any
> value for me.
> 

my project has a git subtree for a linux kernel and another subtree for 
buildroot, 

a default .git is about 1.5G, squashing it reduces it to 200M so it's worth it 
for me :)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Questions/investigations on git-subtree and tags

[Paul Campbell]

On Thu, Mar 7, 2013 at 12:50 PM, Jeremy Rosen  wrote:
>> >>
>> >> Git subtree ignores tags from the remote repo.
>> >>
>> >
>> > is that a design decision or a case of "not implemented yet"
>>
>> I'm not sure. If you imported all the tags from all your subtrees
>> repos, you could easily end up with duplicate tags from different
>> repos. They could be namespaced, but there is no concept of namespace
>> in git-subtree. That even assumes that you can tag a subtree (I've
>> not
>> tried).
>>
>
> Ok, I can understand that you don't want to import tags for namespace reason, 
> but in that case shouldn't
> git subtree add refuse to create a subtree when the tag isn't a commit

It shouldn't and tries not to, but is limited in it's ability to
identify if a refspec points to a commit or not in the remote repo.

> or if it allows it, what would be the gracefull way to handle that ?

I've posted a patch (which is pending a lot of other changes to
git-subtree that I'm corralling) that tries to prevent some obvious
errors in the refspec. But letting the git fetch used by git-subtree
add and git-subtree pull catch the error and report it may be the best
option.

> i'm quite new to git's internals, so I don't really know if/what the right 
> approch would be.
>
> note that all those problems seems to disapear when squash is not used

I've never really tried using --squash, I don't see that it adds any
value for me.

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

Re: Questions/investigations on git-subtree and tags

[Jeremy Rosen]

> >>
> >> Git subtree ignores tags from the remote repo.
> >>
> >
> > is that a design decision or a case of "not implemented yet"
> 
> I'm not sure. If you imported all the tags from all your subtrees
> repos, you could easily end up with duplicate tags from different
> repos. They could be namespaced, but there is no concept of namespace
> in git-subtree. That even assumes that you can tag a subtree (I've
> not
> tried).
> 

Ok, I can understand that you don't want to import tags for namespace reason, 
but in that case shouldn't 
git subtree add refuse to create a subtree when the tag isn't a commit

or if it allows it, what would be the gracefull way to handle that ?

i'm quite new to git's internals, so I don't really know if/what the right 
approch would be.

note that all those problems seems to disapear when squash is not used
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: Questions/investigations on git-subtree and tags

[Paul Campbell]

On Thu, Mar 7, 2013 at 11:05 AM, Jeremy Rosen  wrote:
>>
>> Hi Jérémy,
>>
>> Git subtree ignores tags from the remote repo.
>>
>
> is that a design decision or a case of "not implemented yet"

I'm not sure. If you imported all the tags from all your subtrees
repos, you could easily end up with duplicate tags from different
repos. They could be namespaced, but there is no concept of namespace
in git-subtree. That even assumes that you can tag a subtree (I've not
tried).

>> To follow a project in a subdirectory I would use git-subtree add
>> selecting a branch, not a tag, from the other repo. Then use
>> git-subtree pull to keep yourself updated.
>>
>
>
> well... yes, but releases are marked by tags, not branches so what I really 
> want is a tag.
>
> I still use git so I have the possibility to update and can traceback what 
> happened later
>
>> e.g.
>>
>> To add:
>>
>> git subtree add --prefix=$subdir $repo $branch
>>
>> Then to update:
>>
>> git subtree pull --prefix=$subdir $repo $branch
>>
>
>
> ok, that probably works with branches (didn't test)
>
>> If you make any changes on the branch and wanted to push them back
>> you
>> could do that with:
>>
>> git subtree pull --prefix=$subdir $repo2 $branch2
>>
>> $repo2 and $branch2 would be different from $repo and $branch if you
>> wanted to push to your own fork before submitting a pull request.
>>
>
> shouldn't there be a subtree split somewhere ? IIUC pull is only merge from 
> the remote to my local repo,
> not the other way round

Oops, that should have been git subtree push, which uses git subtree
split internally.

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

[PATCH] git svn: ignore partial svn:mergeinfo

[Jan Pešta]

Currently this is cosmetic change - the merges are ignored, becuase the methods 
(lookup_svn_merge, find_rev_before, find_rev_after) are failing on comparing 
text with number.

See http://www.open.collab.net/community/subversion/articles/merge-info.html
Extract:
The range r30430:30435 that was added to 1.5.x in this merge has a '*' suffix 
for 1.5.x\www.
This '*' is the marker for a non-inheritable mergeinfo range.
The '*' means that only the path on which the mergeinfo is explicitly set has 
had this range merged into it.

Signed-off-by: Jan Pesta 
---
 perl/Git/SVN.pm | 5 +
 1 file changed, 5 insertions(+)

diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
index 0ebc68a..74d49bb 100644
--- a/perl/Git/SVN.pm
+++ b/perl/Git/SVN.pm
@@ -1493,6 +1493,11 @@ sub lookup_svn_merge {
my @merged_commit_ranges;
# find the tip
for my $range ( @ranges ) {
+   if ($range =~ /[*]$/) {
+   warn "W:Ignoring partial merge in svn:mergeinfo "
+   ."dirprop: $source:$range\n";
+   next;
+   }
my ($bottom, $top) = split "-", $range;
$top ||= $bottom;
my $bottom_commit = $gs->find_rev_after( $bottom, 1, $top );
-- 
1.8.1.msysgit.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: Questions/investigations on git-subtree and tags

[Jeremy Rosen]

> 
> Hi Jérémy,
> 
> Git subtree ignores tags from the remote repo.
> 

is that a design decision or a case of "not implemented yet"

> To follow a project in a subdirectory I would use git-subtree add
> selecting a branch, not a tag, from the other repo. Then use
> git-subtree pull to keep yourself updated.
> 


well... yes, but releases are marked by tags, not branches so what I really 
want is a tag.

I still use git so I have the possibility to update and can traceback what 
happened later

> e.g.
> 
> To add:
> 
> git subtree add --prefix=$subdir $repo $branch
> 
> Then to update:
> 
> git subtree pull --prefix=$subdir $repo $branch
> 


ok, that probably works with branches (didn't test)

> If you make any changes on the branch and wanted to push them back
> you
> could do that with:
> 
> git subtree pull --prefix=$subdir $repo2 $branch2
> 
> $repo2 and $branch2 would be different from $repo and $branch if you
> wanted to push to your own fork before submitting a pull request.
> 

shouldn't there be a subtree split somewhere ? IIUC pull is only merge from the 
remote to my local repo,
not the other way round


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

Re: Questions/investigations on git-subtree and tags

[Paul Campbell]

On Thu, Mar 7, 2013 at 10:25 AM, Jeremy Rosen  wrote:
> Hello everybody
>
>
> I am trying to use git-subtree to follow a subproject but I have a couple of 
> problems and I am not sure if I am doing something wrong
>
> Basically I am trying to use a tag on the subproject as my "base" for the 
> subproject but subtree doesn't seem to handle that properly
>
>
> my first attempt was to simply do
>
> "git subtree add --squash git://git.buildroot.net/buildroot 2013.02"
>
> but subtree refused, telling me that the SHA of the tag is not a valid commit.
> Ok it makes sense, though I think this is a very valid use-case...
>
> so I tried
>
> "git subtree add git://git.buildroot.net/buildroot 2013.02^{commit}"
>
> which was refused because git fetch can't parse the ^{commit} marker.
> Again it makes sense, but I really want to start from that tag.
>
>
> so I tried
>
> "git fetch git://git.buildroot.net/buildroot 2013.02"
> "git subtree add --squash FETCH_HEAD"
>
> which worked. Ok at that point I am slightly abusing git subtree, but it 
> seems a valid usage
>
> except that this last attempt causes serious problems when trying to split 
> out the tree again
>
> the call to "git commit-tree" within "git subtree split" complains that the 
> SHA of the parent
> is not a valid commit SHA. Which is true, it's the SHA of the tag.
>
>
> At this point I am not sure if I am abusing subtree, if I have a legitimate 
> but unimplemented use-case and how to
> fix/implement it.
>
> the squash-commit message only contains the SHA of the tag, should it contain 
> the SHA of the commit ?
>
> "subtree split" can only handle commit SHA, should it somehow follow tag SHA 
> too ? how ?
>
> this is probably a trivial fix for someone with good knowledge of git-subtree 
> but i'm not there yet, so any hint would be welcomed
>
> Regards
>
> Jérémy Rosen
>

Hi Jérémy,

Git subtree ignores tags from the remote repo.

To follow a project in a subdirectory I would use git-subtree add
selecting a branch, not a tag, from the other repo. Then use
git-subtree pull to keep yourself updated.

e.g.

To add:

git subtree add --prefix=$subdir $repo $branch

Then to update:

git subtree pull --prefix=$subdir $repo $branch

If you make any changes on the branch and wanted to push them back you
could do that with:

git subtree pull --prefix=$subdir $repo2 $branch2

$repo2 and $branch2 would be different from $repo and $branch if you
wanted to push to your own fork before submitting a pull request.

-- 
Paul [W] Campbell
--
To unsubscribe from this list: send the line "unsubscribe 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] format-patch: RFC 2047 says multi-octet character may not be split

[Kirill Smelkov]

Junio,

On Wed, Mar 06, 2013 at 09:47:53AM -0800, Junio C Hamano wrote:
> Kirill Smelkov  writes:
> 
> > Intro
> > -
> 
> Drop this.  We know the beginning part is "intro" already ;-)

:)


> > Subject:  föö bar
> >
> > encoding
> >
> > Subject: =?UTF-8?q?=20f=C3=B6=C3=B6?=
> >  =?UTF-8?q?=20bar?=
> >
> > is correct, and
> >
> > Subject: =?UTF-8?q?=20f=C3=B6=C3?=  <-- NOTE ö is broken here
> >  =?UTF-8?q?=B6=20bar?=
> >
> > is not, because "ö" character UTF-8 encoding C3 B6 is split here across
> > adjacent encoded words.
> 
> The above is an important part to keep in the log message.
> Everything above that I snipped can be left out for brevity.
> 
> > As it is now, format-patch does not respect "multi-octet charactes may
> > not be split" rule, and so sending patches with non-english subject has
> > issues:
> >
> > The problematic case shows in mail readers as " fö?? bar".
> 
> But the log message lacks crucial bits of information before you
> start talking about your solution.  Where does it go wrong?  What
> did the earlier attempt bafc478..41dd00bad miss?  This can be fixed
> trivially by replacing the above (and the "solution" section),
> perhaps like this:
> 
> Even though an earlier attempt (bafc478..41dd00bad) cleaned
> up RFC 2047 encoding, pretty.c::add_rfc2047() still decides
> where to split the output line by going through the input
> one byte at a time, and potentially splits a character in
> the middle.  A subject line may end up showing like this:
> 
>  The problematic case shows in mail readers as " fö?? bar".
> 
> Instead, make the loop grab one _character_ at a time and
> determine its output length to see where to break the output
> line.  Note that this version only knows about UTF-8, but the
> logic to grab one character is abstracted out in mbs_chrlen()
> function to make it possible to extend it to other encodings.

I agree my description was messy and thanks for reworking and clarifying
it - your version is much better.

I'll use its slight variation for the updated patch.


> > +   while (len) {
> > +   /*
> > +* RFC 2047, section 5 (3):
> > +*
> > +* Each 'encoded-word' MUST represent an integral number of
> > +* characters.  A multi-octet character may not be split across
> > +* adjacent 'encoded- word's.
> > +*/
> > +   const unsigned char *p = (const unsigned char *)line;
> > +   int chrlen = mbs_chrlen(&line, &len, encoding);
> > +   int is_special = (chrlen > 1) || is_rfc2047_special(*p, type);
> >  
> > /*
> >  * According to RFC 2047, we could encode the special character
> > @@ -367,16 +376,18 @@ static void add_rfc2047(struct strbuf *sb, const char 
> > *line, int len,
> >  * causes ' ' to be encoded as '=20', avoiding this problem.
> >  */
> >  
> > +   if (line_len + 2 + (is_special ? 3*chrlen : 1) > 
> > max_encoded_length) {
> 
> Always have SP around binary operators such as '*' (multiplication).

ok, but note that's just a matter of style, and if one is used to code
formulas, _not_ having SP is more convenient sometimes.


> I would actually suggest adding an extra variable "encoded_len" and
> do something like this:
> 
>   /* "=%02X" times num_char, or the byte itself */
>   encoded_len = is_special ? 3 * num_char : 1;
> if (max_encoded_length < line_len + 2 + encoded_len) {
>   /* It will not fit---break the line */
>   ...

Right. Actually if we add encoded_len, adding encoded_fmt is tempting

const char *encoded_fmt = is_special ? "=%02X": "%c";

and then encoding part simplifies to just unconditional

for (i = 0; i < chrlen; i++)
strbuf_addf(sb, encoded_fmt, p[i]);
line_len += encoded_len;


> You may also want to say what the hardcoded "2" is about in the
> comment there.

ok.


> > diff --git a/utf8.c b/utf8.c
> > index 8f6e84b..7911b58 100644
> > --- a/utf8.c
> > +++ b/utf8.c
> > @@ -531,3 +531,42 @@ char *reencode_string(const char *in, const char 
> > *out_encoding, const char *in_e
> > return out;
> >  }
> >  #endif
> > +
> > +/*
> > + * Returns first character length in bytes for multi-byte `text` according 
> > to
> > + * `encoding`.
> > + *
> > + * - The `text` pointer is updated to point at the next character.
> > + * - When `remainder_p` is not NULL, on entry `*remainder_p` is how much 
> > bytes
> > + *   we can consume from text, and on exit `*remainder_p` is reduced by 
> > returned
> > + *   character length. Otherwise `text` is treated as limited by NUL.
> > + */
> > +int mbs_chrlen(const char **text, size_t *remainder_p, const char 
> > *encoding)
> > +{
> > +   int chrlen;
> > +   const char *p = *text;
> > +   size_t r = (remainder_p ? *remainder_p : INT_MAX);
> 
> Ugly, and more importantly I suspect this is wrong

Re: [PATCH] In partial SVN merges, the ranges contains additional character "*"

[Eric Wong]

(adding Sam to the Cc:, I rely on him for SVN merge knowledge)

Jan Pešta  wrote:
> See http://www.open.collab.net/community/subversion/articles/merge-info.html
> Extract:
> The range r30430:30435 that was added to 1.5.x in this merge has a '*'
> suffix for 1.5.x\www.
> This '*' is the marker for a non-inheritable mergeinfo range.
> The '*' means that only the path on which the mergeinfo is explicitly set
> has had this range merged into it.

Jan: can you write a better commit message to explain what your
patch fixes/changes, and why we do it?

Something like:

  Subject: [PATCH] git svn: ignore partial svn:mergeinfo

  

See Documentation/SubmittingPatches for hints.  Thanks!

> Signed-off-by: Jan Pesta 
> ---
>  perl/Git/SVN.pm | 5 +
>  1 file changed, 5 insertions(+)
> 
> diff --git a/perl/Git/SVN.pm b/perl/Git/SVN.pm
> index 0ebc68a..74d49bb 100644
> --- a/perl/Git/SVN.pm
> +++ b/perl/Git/SVN.pm
> @@ -1493,6 +1493,11 @@ sub lookup_svn_merge {
>   my @merged_commit_ranges;
>   # find the tip
>   for my $range ( @ranges ) {
> + if ($range =~ /[*]$/) {
> + warn "W:Ignoring partial merge in svn:mergeinfo "
> + ."dirprop: $source:$range\n";
> + next;
> + }
>   my ($bottom, $top) = split "-", $range;
>   $top ||= $bottom;
>   my $bottom_commit = $gs->find_rev_after( $bottom, 1, $top );
> -- 
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Questions/investigations on git-subtree and tags

[Jeremy Rosen]

Hello everybody


I am trying to use git-subtree to follow a subproject but I have a couple of 
problems and I am not sure if I am doing something wrong 

Basically I am trying to use a tag on the subproject as my "base" for the 
subproject but subtree doesn't seem to handle that properly


my first attempt was to simply do 

"git subtree add --squash git://git.buildroot.net/buildroot 2013.02" 

but subtree refused, telling me that the SHA of the tag is not a valid commit. 
Ok it makes sense, though I think this is a very valid use-case...

so I tried

"git subtree add git://git.buildroot.net/buildroot 2013.02^{commit}" 

which was refused because git fetch can't parse the ^{commit} marker.
Again it makes sense, but I really want to start from that tag.


so I tried

"git fetch git://git.buildroot.net/buildroot 2013.02"
"git subtree add --squash FETCH_HEAD"

which worked. Ok at that point I am slightly abusing git subtree, but it seems 
a valid usage

except that this last attempt causes serious problems when trying to split out 
the tree again

the call to "git commit-tree" within "git subtree split" complains that the SHA 
of the parent
is not a valid commit SHA. Which is true, it's the SHA of the tag.


At this point I am not sure if I am abusing subtree, if I have a legitimate but 
unimplemented use-case and how to 
fix/implement it.

the squash-commit message only contains the SHA of the tag, should it contain 
the SHA of the commit ?

"subtree split" can only handle commit SHA, should it somehow follow tag SHA 
too ? how ?

this is probably a trivial fix for someone with good knowledge of git-subtree 
but i'm not there yet, so any hint would be welcomed

Regards

Jérémy Rosen

fight key loggers : write some perl using vim
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

rebase: strange failures to apply patc 3-way

[Max Horn]

Recently I have observed very strange failures in "git rebase" that cause it to 
fail to work automatically in situations where it should trivially be able to 
do so.

In case it matter, here's my setup: git 1.8.2.rc2.4.g7799588 (i.e. git.git 
master branch) on Mac OS X. The repos clone is on a HFS+ partition, not on a 
network volume. No gitattributes are being used.  Regarding the origin of the 
repos (I hope it doesn't matter, but just in case): The repository in question 
used to be a CVS repository; it was decided to switch to Mercurial (not my 
decision ;-) and I performed the conversion; I first converted it to git using 
cvs2git (from the cvs2svn suite), then performed some history cleanup, and 
finally used "hg convert" to convert it to git. Recently, I have been accessing 
the repository from git via the gitifyhg tool 
. 

Anyway, several strange things just happened (and I had similar experiences in 
the past days and weeks, but that was using an older git, and I thought I was 
just doing something wrong).

Specifically, I wanted to rebase a branch with some experimental commits. The 
strange things started with this:

$ git rebase MY-NEW-BASE
First, rewinding head to replay your work on top of it...
Applying: SOME COMMIT
Applying: SOME OTHER COMMIT
...
Applying: COMMIT A
Applying: COMMIT B
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: Your local changes to the following files would be overwritten by merge:
some/source.file
Please, commit your changes or stash them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0014 COMMIT B
The copy of the patch that failed is found in:
   /path/to/my/repo/.git/rebase-apply/patch

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


Now, what is strange about this is that the failed patch actually applies 
cleanly:

$ patch -p1 < /path/to/my/repo/.git/rebase-apply/patch
patching file some/source.file
$

And there is no subtle merge issue here, either: That patch is the only one to 
have touched the surrounding code since 1999! There is no source of conflict 
there!

Anyway. The tale gets stranger, as I was trying to do this again (no changes 
were made to the repos in between, this is a straight continuation from above):

$ git rebase --abort
$ git rebase MY-NEW-BASE
First, rewinding head to replay your work on top of it...
Applying: SOME COMMIT
Applying: SOME OTHER COMMIT
...
Applying: COMMIT A
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: Your local changes to the following files would be overwritten by merge:
some/othersource.file
some/yetanother.file
Please, commit your changes or stash them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0013 COMMIT A
The copy of the patch that failed is found in:
   /path/to/my/repo/.git/rebase-apply/patch

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



So suddenly it fails to apply the commit A, the one before the previously 
failing commit. Huh? But again, the failing patch applies cleanly (and after 
all, rebase was able to apply it in my previous attempt).  And again, the patch 
actually applies cleanly. So one more try:


$ git rebase --abort
$ git rebase MY-NEW-BASE
First, rewinding head to replay your work on top of it...
Applying: SOME COMMIT
Applying: SOME OTHER COMMIT
...
Applying: COMMIT A
Using index info to reconstruct a base tree...
Falling back to patching base and 3-way merge...
error: Your local changes to the following files would be overwritten by merge:
some/othersource.file
Please, commit your changes or stash them before you can merge.
Aborting
Failed to merge in the changes.
Patch failed at 0013 COMMIT A
The copy of the patch that failed is found in:
   /path/to/my/repo/.git/rebase-apply/patch

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


Again it fails in commit A -- but this time, it only thinks one file is 
problematic. HUH? Again, the patch actually applies cleanly:

$ patch -p1 < /path/to/my/repo/.git/rebase-apply/patch
patching file some/othersource.file
patching file some/yetanother.file


At this point, things stabilized, and when I now abort and reattempt the merge, 
it always fails in the same way. This time trying to apply a change to a code 
comment that was last changed in 1997 (though for one hunk, a few lines before 
the changed lines, there is a line that w

Re: [PATCH] add: Clarify documentation of -A and -u

[Greg Price]

On Wed, Mar 06, 2013 at 09:10:57AM -0800, Junio C Hamano wrote:
> I do not know if mentioning what -A does in the description for -u
> (and vice versa) makes it easier to understand or more confusing
> (not rhetorical: I suspect some may find it easier and others not).
> 
> But "and the default behaviour will..." here _is_ confusing.  After
> reading this patch three times, I still cannot tell what "default"
> you are trying to describe.  Is it "-u" without arguments?  Is it
> "add" without "-u" nor "-A"?  Is it something else???

I meant the behavior without -A or -u.  This could be fixed directly
by s/the default behavior will/with neither -A nor -u we/.  But we can
also keep revising -- there's definitely more room to make this
clearer!  I suggest new versions at the bottom of this message.

I do think that it's important that the reader be able to see clearly
what the option does as a contrast with what the command does without
the option.  Normally in a man page this is how the option is
described in the first place -- for example, the next entry in this
very man page says "Record *only* the fact that the path will be added
later", rather than "Record the fact that ..."  These two descriptions
suffer from not doing that, so that it's not clear which parts of the
behavior they describe are just what 'add' does with no options and
which parts are actually due to the option.


> Whenever you see an incomprehensible technobabble followed by "That
> means", "In other words", etc., you (not limited to Greg-you but
> figuratively "everybody") should see if it makes it easier to read
> to remove everything up to that "That means" [...]

Seems like a reasonable heuristic.


>   -u::
> --update::
>   Update modified and/or removed paths in the index
>   that match  with the current state of the
>   working tree files.  No new path is added because
>   this considers only the paths that are already in
>   the index.
> 
>   -A::
> --all::
>   Update the index to record the current state of the
>   working tree files that match .  Note that
>   new paths will be added to the index, in addition to
>   modified and/or removed paths.

These are good -- I especially like that they're shorter.  I do think
they're still likely to be confusing.  The lead sentences are hard to
tell apart from each other or one's mental model of what 'add' alone
does, though the contrasts that follow them help.  I also think the
lead sentence for '--all' isn't really correct -- we update the index
not only for the working tree files that match , but also
where there is no working tree file, only an index entry.  (So the
sentence actually describes what 'add' with neither option does.)

Maybe it's worth taking a step back.  The overall taxonomy is
 * 'add' alone considers matching filenames in the working tree
 * 'add -u' considers matching filenames in the index
 * 'add -A' considers matching filenames in both the index and the
   working tree
and in each case we make the index match the working tree on those
files.  Or, put another way,
 * 'add' alone modifies and adds files
 * 'add -u' modifies and removes files
 * 'add -A' modifies, adds, and removes files

Here's a crack at making those distinctions clear.  I've also tried to
make the descriptions as parallel as possible, as what they're saying
is very similar.

-u::
--update::
Update the index just where it already has an entry matching
.  This removes as well as modifies index entries to
match the working tree, but adds no new files.

-A::
--all::
Update the index not only where the working tree has a file
matching  but also where the index already has an
entry.  This adds, modifies, and removes index entries to
match the working tree.

These are the shortest in the discussion so far, and I think they're
also the clearest.

Then follow both with the "If no " paragraph.  I just
noticed that the paragraph actually needs a small modification to fit
'-A', too.  New patch below.

Greg


From: Greg Price 
Date: Thu, 7 Mar 2013 02:08:21 -0800
Subject: [PATCH] add: Clarify documentation of -A and -u

The documentation of '-A' and '-u' is very confusing for someone who
doesn't already know what they do.  Describe them with fewer words and
clearer parallelism to each other and to the behavior of plain 'add'.

Also mention the default  for '-A' as well as '-u', because
it applies to both.

Signed-off-by: Greg Price 
---
 Documentation/git-add.txt | 22 --
 1 file changed, 12 insertions(+), 10 deletions(-)

diff --git a/Documentation/git-add.txt b/Documentation/git-add.txt
index 388a225..b0944e5 100644
--- a/Documentation/git-add.txt
+++ b/Documentation/git-add.txt
@@ -100,12 +100,9 @@ apply to the index. See EDITING PATCHES below.
 
 -u::
 --update::
-   Only match  against already track

Re: What I want rebase to do

[Johannes Sixt]

Am 3/7/2013 9:48, schrieb Thomas Rast:
> wor...@alum.mit.edu (Dale R. Worley) writes:
> [...snip...]
> 
> Isn't that just a very long-winded way of restating what Junio said
> earlier:
> 
>>> It was suggested to make it apply the first-parent diff and record
>>> the result, I think.  If that were an acceptable approach (I didn't
>>> think about it through myself, though), that would automatically
>>> cover the evil-merge case as well.
> 
> You can fake that with something like
> 
> git rev-list --first-parent --reverse RANGE_TO_REBASE |
> while read rev; do
> if git rev-parse $rev^2 >/dev/null 2>&1; then
> git cherry-pick -n -m1 $rev
> git rev-parse $rev^2 >.git/MERGE_HEAD
> git commit -C$rev
> else
> git cherry-pick $rev
> fi
> done
> 
> Only tested very lightly.  Dealing with octopii, conflicts and actually
> preserving the commit's attributes is left as an exercise to the
> reader[1].

I proposed this long ago, but by modifying preserve-merges rather than
with a new option (--first-parent):
http://thread.gmane.org/gmane.comp.version-control.git/198125

It works very well. I'm using it frequently in the field.

-- 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 2/2] p4merge: create a virtual base if none available

[Kevin Bracey]

On 07/03/2013 04:23, David Aguilar wrote:

On Wed, Mar 6, 2013 at 12:32 PM, Kevin Bracey  wrote:

+make_virtual_base() {
+   # Copied from git-merge-one-file.sh.

I think the reasoning behind these patches is good.

How do we feel about this duplication?

Bad.

Should we make a common function in the git-sh-setup.sh,
or is it okay to have a slightly modified version of this
function in two places?
I'd prefer to have a common function, I just didn't know if there was 
somewhere appropriate to place it, available from both files. And I'm 
going to have to learn a bit more sh to get it right.

Also, the "@@DIFF@@" string may not work here.
This is a template string that is replaced by the Makefile.


It does work in git-mergetool--lib.sh, but not in mergetools/p4merge.


We prefer $(command) instead of `command`.
These should be adjusted.

Can the same thing be accomplished using "git diff --no-index"
so that we do not need a dependency on an external "diff" command here?
Do these comments still apply if it's a common function in 
git-sh-setup.sh that git-one-merge-file.sh will use? I'm wary of 
layering violations.


Kevin


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

Re: Merging submodules - best merge-base

[Daniel Bratell]

Den 2013-03-06 19:12:05 skrev Heiko Voigt :


On Mon, Feb 25, 2013 at 05:44:05PM +0100, Daniel Bratell wrote:

I can phrase this in two ways and I'll start with the short way:

Why does a merge of a git submodule use as merge-base the commit that  
was
active in the merge-base of the parent repo, rather than the merge-base  
of

the two commits that are being merged?

The long question is:

A submodule change can be merged, but only if the merge is a
"fast-forward" which I think is a fair demand, but currently it checks  
if

it's a fast-forward from a commit that might not be very interesting
anymore.

If two branches A and B split at a point when they used submodule commit
S1 (based on S), and both then switched to S2 (also based on S) and B  
then

switched to S21, then it's today not possible to merge B into A, despite
S21 being a descendant of S2 and you get a conflict and this warning:

warning: Failed to merge submodule S (commits don't follow merge-base)

(attempt at ASCII gfx:

Submodule tree:

S  S1
   \
\ - S2 -- S21

Main tree:

A' (uses S1) --- A (uses S2)
   \
\ --- B' (uses S2) -- B (uses S21)


I would like it to end up as:

A' (uses S1) --- A (uses S2)  A+ (uses S21)
   \ /
\ --- B' (uses S2) -- B (uses S21)- /

And that should be legal since S21 is a descendant of S2.


So to summarize what you are requesting: You want a submodule merge be
two way in the view of the superproject and calculate the merge base
in the submodule from the two commits that are going to be merged?

It currently sounds logical but I have to think about it further and
whether that might break other use cases.


Maybe both could be legal even. The current code can't be all wrong, and  
this case also seems to be straightforward.


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

Re: [PATCH] git p4: chdir resolves symlinks only for relative paths

[John Keeping]

On Thu, Mar 07, 2013 at 09:36:06AM +0100, Miklós Fazekas wrote:
> Sorry for the late turnaround here is an improved version. Now chdir
> has an optional argument client_path, if it's true then we don't do
> os.getcwd. I think that my first patch is also valid too - when the
> path is absolute no need for getcwd no matter what is the context,
> when it's relative we have to use os.getcwd() no matter of the
> context.
> 
> ---
> If p4 client is configured to /p/foo which is a symlink:
> /p/foo -> /vol/barvol/projects/foo.  Then resolving the
> symlink will confuse p4:
> "Path /vol/barvol/projects/foo/... is not under client root
> /p/foo". While AltRoots in p4 client specification can be
> used as a workaround on p4 side, git-p4 should not resolve
> symlinks in client paths.
> chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve
> relative paths, but as a sideeffect it resolves symlinks
> too. Now for client paths we don't call os.getcwd().
> 
> Signed-off-by: Miklós Fazekas 
> ---
>  git-p4.py |   11 ---
>  1 file changed, 8 insertions(+), 3 deletions(-)
> 
> diff --git a/git-p4.py b/git-p4.py
> index 0682e61..2bd8cc2 100755
> --- a/git-p4.py
> +++ b/git-p4.py
> @@ -68,12 +68,17 @@ def p4_build_cmd(cmd):
>  real_cmd += cmd
>  return real_cmd
> 
> -def chdir(dir):
> +def chdir(dir,client_path=False):

Style (space after comma):

def chdir(dir, client_path=False):

>  # P4 uses the PWD environment variable rather than getcwd(). Since we're
>  # not using the shell, we have to set it ourselves.  This path could
>  # be relative, so go there first, then figure out where we ended up.
> +# os.getcwd() will resolve symlinks, so we should avoid it for
> +# client_paths.
>  os.chdir(dir)
> -os.environ['PWD'] = os.getcwd()
> +if client_path:
> +os.environ['PWD'] = dir
> +else:
> +   os.environ['PWD'] = os.getcwd()

Indentation seems to have gone a bit wrong here...

> 
>  def die(msg):
>  if verbose:
> @@ -1554,7 +1559,7 @@ class P4Submit(Command, P4UserMap):
>  new_client_dir = True
>  os.makedirs(self.clientPath)
> 
> -chdir(self.clientPath)
> +chdir(self.clientPath,client_path=True)

Again, there should be a space after the comma here.

>  if self.dry_run:
>  print "Would synchronize p4 checkout in %s" % self.clientPath
>  else:
> -- 
> 1.7.10.2 (Apple Git-33)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: What I want rebase to do

[Thomas Rast]

wor...@alum.mit.edu (Dale R. Worley) writes:
[...snip...]

Isn't that just a very long-winded way of restating what Junio said
earlier:

> > It was suggested to make it apply the first-parent diff and record
> > the result, I think.  If that were an acceptable approach (I didn't
> > think about it through myself, though), that would automatically
> > cover the evil-merge case as well.

You can fake that with something like

git rev-list --first-parent --reverse RANGE_TO_REBASE |
while read rev; do
if git rev-parse $rev^2 >/dev/null 2>&1; then
git cherry-pick -n -m1 $rev
git rev-parse $rev^2 >.git/MERGE_HEAD
git commit -C$rev
else
git cherry-pick $rev
fi
done

Only tested very lightly.  Dealing with octopii, conflicts and actually
preserving the commit's attributes is left as an exercise to the
reader[1].

I still think that the _right_ solution is first redoing the merge on
its original parents and then seeing how the actual merge differs from
that.  --preserve-merges has bigger issues though, like Junio said.

Perhaps a new option to git-rebase could trigger the above behavior for
merges, who knows.  (It could be called --first-parent.)


[1]  If you don't get the sarcasm: that would amount to reinventing
large parts of git-rebase.

-- 
Thomas Rast
trast@{inf,student}.ethz.ch
--
To unsubscribe from this list: send the line "unsubscribe 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-scm.com/book/ru -- incorrect "next" link containing a question mark

[Konstantin Khomoutov]

On Thu, 7 Mar 2013 00:01:31 -0800 (PST)
Aleksey Rozhkov  wrote:

> The page http://git-scm.com/book/ru/
> Введение-Первоначальная-настройка-Git contains incorrect link "next"
> Now this link to the page 
> http://git-scm.com/book/ru/Введение-Как-получить-помощь? , but this
> page does not exist

I would say "?" is just interpreted as a request part of the URL.

Indeed, the page source contains:

prev | next

so the question mark is really included verbatim.

> So, correct link is 
> http://git-scm.com/book/ru/Введение-Как-получить-помощь%3F

Good point, thanks.  I Cc'ed the main Git list and made the message
title a bit more clear.

To the Pro Git book maintainer: is it possible to somehow fix the HTML
generation script to URL-encode any special characters if they're to
appear in an URL?
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] git p4: chdir resolves symlinks only for relative paths

[Miklós Fazekas]

Sorry for the late turnaround here is an improved version. Now chdir
has an optional argument client_path, if it's true then we don't do
os.getcwd. I think that my first patch is also valid too - when the
path is absolute no need for getcwd no matter what is the context,
when it's relative we have to use os.getcwd() no matter of the
context.

---
If p4 client is configured to /p/foo which is a symlink:
/p/foo -> /vol/barvol/projects/foo.  Then resolving the
symlink will confuse p4:
"Path /vol/barvol/projects/foo/... is not under client root
/p/foo". While AltRoots in p4 client specification can be
used as a workaround on p4 side, git-p4 should not resolve
symlinks in client paths.
chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve
relative paths, but as a sideeffect it resolves symlinks
too. Now for client paths we don't call os.getcwd().

Signed-off-by: Miklós Fazekas 
---
 git-p4.py |   11 ---
 1 file changed, 8 insertions(+), 3 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index 0682e61..2bd8cc2 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -68,12 +68,17 @@ def p4_build_cmd(cmd):
 real_cmd += cmd
 return real_cmd

-def chdir(dir):
+def chdir(dir,client_path=False):
 # P4 uses the PWD environment variable rather than getcwd(). Since we're
 # not using the shell, we have to set it ourselves.  This path could
 # be relative, so go there first, then figure out where we ended up.
+# os.getcwd() will resolve symlinks, so we should avoid it for
+# client_paths.
 os.chdir(dir)
-os.environ['PWD'] = os.getcwd()
+if client_path:
+os.environ['PWD'] = dir
+else:
+   os.environ['PWD'] = os.getcwd()

 def die(msg):
 if verbose:
@@ -1554,7 +1559,7 @@ class P4Submit(Command, P4UserMap):
 new_client_dir = True
 os.makedirs(self.clientPath)

-chdir(self.clientPath)
+chdir(self.clientPath,client_path=True)
 if self.dry_run:
 print "Would synchronize p4 checkout in %s" % self.clientPath
 else:
-- 
1.7.10.2 (Apple Git-33)


On Mon, Feb 4, 2013 at 12:08 AM, Pete Wyckoff  wrote:
> mfaze...@szemafor.com wrote on Tue, 29 Jan 2013 09:37 +0100:
>> If a p4 client is configured to /p/foo which is a symlink
>> to /vol/bar/projects/foo, then resolving symlink, which
>> is done by git-p4's chdir will confuse p4: "Path
>> /vol/bar/projects/foo/... is not under client root /p/foo"
>> While AltRoots in p4 client specification can be used as a
>> workaround on p4 side, git-p4 should not resolve symlinks
>> in client paths.
>> chdir(dir) uses os.getcwd() after os.chdir(dir) to resolve
>> relative paths, but as a side effect it resolves symlinks
>> too. Now it checks if the dir is relative before resolving.
>>
>> Signed-off-by: Miklós Fazekas 
>> ---
>>  git-p4.py |5 -
>>  1 file changed, 4 insertions(+), 1 deletion(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index 2da5649..5d74649 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -64,7 +64,10 @@ def chdir(dir):
>>  # not using the shell, we have to set it ourselves.  This path could
>>  # be relative, so go there first, then figure out where we ended up.
>>  os.chdir(dir)
>> -os.environ['PWD'] = os.getcwd()
>> +if os.path.isabs(dir):
>> +os.environ['PWD'] = dir
>> +else:
>> +os.environ['PWD'] = os.getcwd()
>>
>>  def die(msg):
>>  if verbose:
>
> Thanks, this is indeed a bug and I have reproduced it with a test
> case.  Your patch works, but I think it would be better to
> separate the callers of chdir():  those that know they are
> cd-ing to a path from a p4 client, and everybody else.  The former
> should not use os.getcwd(), as you show.
>
> I'll whip something up soon, unless you beat me to it.
>
> -- Pete
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: feature suggestion: optimize common parts for checkout --conflict=diff3

[Jeff King]

On Wed, Mar 06, 2013 at 01:32:28PM -0800, Junio C Hamano wrote:

> > We show "both sides added, either identically or differently" as
> > noteworthy events, but the patched code pushes "both sides added
> > identically" case outside the conflicting hunk, as if what was added
> > relative to the common ancestor version (in Uwe's case, is it 1-14
> > that is common, or just 10-14?) is not worth looking at when
> > considering what the right resolution is.  If it is not worth
> > looking at what was in the original for the conflicting part, why
> > would we be even using diff3 mode in the first place?
> 
> I vaguely recall we did this "clip to eager" as an explicit bugfix
> at 83133740d9c8 (xmerge.c: "diff3 -m" style clips merge reduction
> level to EAGER or less, 2008-08-29).  The list archive around that
> time may give us more contexts.

Thanks for the pointer. The relevant threads are:

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

and

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

There is not much discussion beyond what ended up in 8313374; both Linus
and Dscho question whether level and output format are orthogonal, but
seem to accept the explanation you give in the commit message.

Having read that commit and the surrounding thread, I think I stand by
my argument that "zdiff3" is a useful tool to have, as long as the user
understands what the hunks mean. It should never replace diff3, but I
think it makes sense as a separate format.

I was also curious whether it would the diff3/zealous combination would
trigger any weird corner cases. In particular, I wanted to know how the
example you gave in that commit of:

  postimage#1: 1234ABCDE789
  |/
  |   /
  preimage:123456789
  |   \
  |\
  postimage#2: 1234AXCYE789

would react with diff3 (this is not the original example, but one with
an extra "C" in the middle of postimage#2, which could in theory be
presented as split hunks). However, it seems that we do not do such hunk
splitting at all, neither for diff3 nor for the "merge" representation.

-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