Re: [PATCH] mergetools: Add tortoisegitmerge helper

[David Aguilar]

On Thu, Jan 24, 2013 at 11:21 PM, Junio C Hamano  wrote:
> David Aguilar  writes:
> This paragraph needs to be rewritten to unconfuse readers.  The
> original is barely intelligible, and it becomes unreadable as the
> set of tools subtracted by "minus" and added by "plus" grows.

 But I think this should not be part of this patch.
>>>
>>> I agree that it can be done (and it is better to be done) as a
>>> preparatory step.  The current text is barely readable, but with
>>> this patch there will be two "minus", and the result becomes
>>> unreadable at that point.
>>>
>>> It also could be done as a follow-up documentation readability fix.
>>
>> Another thought would be to minimize this section as much
>> as possible and point users to "git difftool --tool-help".
>
> We had a similar discussion here:
>
>   http://thread.gmane.org/gmane.comp.version-control.git/201913/focus=201976
>
> and Documentation/git-{diff,merge}tool.txt have stayed quiet since
> then.
>
> But Documentation/merge-config.txt tries to list everything that _could_
> be enabled, and I do not necessarily think having one single
> location that lists everything is such a bad idea.
>
> Is there a way for me to programatically tell what merge.tool and
> diff.tool could be enabled for a particular source checkout of Git
> regardless of what platform am I on (that is, even though I won't
> touch Windows, I want to see 'tortoise' appear in the output of such
> a procedure)?  We could generate a small text file from the Makefile
> in Documentation and include it when building the manual pages if
> such a procedure is available.

That's a good idea.
Here's one way... (typed into gmail, so probably broken)

LF='
'
mergetools=
difftools=
scriptlets="$(git --exec-path)"/mergetools

for script in "$scriptlets"/*
do
tool="$(basename "$script")"
if test "$tool" = "defaults"
then
continue
fi
. "$scriptlets"/defaults
can_diff && difftools="$difftools$tool$LF"
can_merge && mergetools="$mergetools$tool$LF"
done

I can follow up with a Documentation patch along these lines.
I'm would imagine it would be hooked up similarly to how the
command lists are constructed.

This should allow the tortoisemerge improvements to happen independently.
-- 
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: segmentation fault (nullpointer) with git log --submodule -p

[Jonathon Mah]

Just to note, the proposals so far don't prevent a "smart-ass" function from 
freeing the buffer when it's called underneath the use/release scope, as in:

with_commit_buffer(commit); {
fn1_needing_buffer(commit);
walk_rev_tree_or_something();
fn2_needing_buffer(commit);
} done_with_commit_buffer(commit);

walk_rev_tree_or_something() might need to read commits to do its thing, and it 
could still choose to free their buffers (as in rev-list.c finish_commit()). If 
those commits includes the one being "retained", the call to fn2 will still see 
NULL despite it being in a 'protected scope'.

Are the objections to using a reference count?



Jonathon Mah
m...@jonathonmah.com


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

Re: segmentation fault (nullpointer) with git log --submodule -p

[Junio C Hamano]

Jonathan Nieder  writes:

> Hi,
>
> Junio C Hamano wrote:
>
>> I've been toying with an idea along this line.
>
> Heh.  Just for fun, here's an uglier version:

Much nicer, though.

>
>   struct wcb_data {
>   int had_buffer;
>   int using_buffer;
>   };
>   #define WITH_COMMIT_BUFFER_DATA_INIT { 0, 0 }
>
>   extern void acquire_commit_buffer(struct commit *, struct wcb_data *);
>   extern void done_with_commit_buffer(struct commit *, struct wcb_data *);
>
>   /*
>* usage:
>*  struct wcb_data buf = WITH_COMMIT_BUFFER_INIT;
>*
>*  with_commit_buffer(commit, buf) {
>*  ...
>*  }
>*/
>   #define with_commit_buffer(commit, i) \
>   for (acquire_commit_buffer(commit, &i); \
>i.using_buffer; \
>done_with_commit_buffer(commit, &i))
>
>   void acquire_commit_buffer(struct commit *commit, struct wcb_data *i)
>   {
>   enum object_type type;
>   unsigned long size;
>
>   assert(!i->using_buffer);
>   i->using_buffer = 1;
>   i->had_buffer = !!commit->buffer;
>
>   if (i->had_buffer)
>   return;
>   commit->buffer = read_sha1_file(commit->object.sha1, &type, 
> &size);
>   if (!commit->buffer)
>   die("unable to read commit %s", 
> sha1_to_hex(commit->object.sha1));
>   }
>
>   void done_with_commit_buffer(struct commit *commit, struct wcb_data *i)
>   {
>   assert(i->using_buffer);
>   i->using_buffer = 0;
>
>   if (!i->had_buffer) {
>   free(commit->buffer);
>   commit->buffer = NULL;
>   }
>   }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mergetools: Add tortoisegitmerge helper

[Junio C Hamano]

David Aguilar  writes:

> Even though the old tortoisemerge and the new tortoisegitmerge
> have completely different syntax, could we still use the existence
> of one when deciding which syntax to use?
> ...
> ...and then later merge_cmd and diff_cmd
> can delegate to {diff,merge}_cmd_legacy() and
> {diff,merge}_cmd_gitmerge() functions to do the work.
>
> It's just a thought.  translate_merge_tool_path()
> is too low-level to do it, but it seems like we could
> get away with it by having some extra smarts in the
> scriptlet.

Sounds like a far better approach to me.  I'd like to at least see
an attempt be made to make that work first.

 This paragraph needs to be rewritten to unconfuse readers.  The
 original is barely intelligible, and it becomes unreadable as the
 set of tools subtracted by "minus" and added by "plus" grows.
>>>
>>> But I think this should not be part of this patch.
>>
>> I agree that it can be done (and it is better to be done) as a
>> preparatory step.  The current text is barely readable, but with
>> this patch there will be two "minus", and the result becomes
>> unreadable at that point.
>>
>> It also could be done as a follow-up documentation readability fix.
>
> Another thought would be to minimize this section as much
> as possible and point users to "git difftool --tool-help".

We had a similar discussion here:

  http://thread.gmane.org/gmane.comp.version-control.git/201913/focus=201976

and Documentation/git-{diff,merge}tool.txt have stayed quiet since
then.

But Documentation/merge-config.txt tries to list everything that _could_
be enabled, and I do not necessarily think having one single
location that lists everything is such a bad idea.

Is there a way for me to programatically tell what merge.tool and
diff.tool could be enabled for a particular source checkout of Git
regardless of what platform am I on (that is, even though I won't
touch Windows, I want to see 'tortoise' appear in the output of such
a procedure)?  We could generate a small text file from the Makefile
in Documentation and include it when building the manual pages if
such a procedure is available.

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

Any Identity tiffany bracelet Is certainly Associated Utilizing High quality

[moonrisePP]

Anytime people speaks about superior wineglass job, any identity Tiffany
without delay arrives at thought process. * tiffany uk
  * has got earned terrific attraction
meant for about seventy-five together with 20 years. It had become involving
some sort of North american specialist by way of the identity for Louis
Coziness Tiffany. This glassware was initially tastefully constructed
implementing a lot of different kinds of wineglass together with favrile,
opalescent, break, ripple, together with drapery.





*http://www.tiffanyandcosales.co.uk/*



--
View this message in context: 
http://git.661346.n2.nabble.com/Any-Identity-tiffany-bracelet-Is-certainly-Associated-Utilizing-High-quality-tp7576037.html
Sent from the git mailing list archive at Nabble.com.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

That Temptations In pandora jewelry charms

[shitit2013]

That picture, Avatar, contains earn very good worldwide recognition
throughout the world, additionally, the exquisite Pandora intercontinental
from this picture catches a persons vision of most many people. Pandora
develops into the latest ideas fairly recently, it is utilized for a number
of points, which includes, * pandora jewelry
  *. It will be well-accepted as a
result of many of us and even develops into quite possibly the most
contemporary expensive jewelry internationally.



*http://www.pandorajewelrysvip.com/   *



--
View this message in context: 
http://git.661346.n2.nabble.com/That-Temptations-In-pandora-jewelry-charms-tp7576036.html
Sent from the git mailing list archive at Nabble.com.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mergetools: Add tortoisegitmerge helper

[David Aguilar]

On Thu, Jan 24, 2013 at 2:15 PM, Junio C Hamano  wrote:
> Sven Strickroth  writes:
>
>> Am 24.01.2013 20:51 schrieb Junio C Hamano:
>>> Sven Strickroth  writes:
>>>
 - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
   (starting with 1.8.0) in order to make clear that this one has special
   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
   version.
>>>
>>> Wouldn't it make more sense in such a situation if your users can
>>> keep using the old "tortoisemerge" configured in their configuration
>>> and when the renamed one is found the mergetool automatically used
>>> it, rather than the way your patch is done?
>>
>> That was also my first idea, however, TortoiseMerge uses parameters as
>> follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
>> space from keys: '-base "$BASE"'. So both are incompatible (the first
>> approach has problems with spaces in filenames, the TortoiseGitMerge
>> approach fixes this).
>
> OK.  Please unconfuse future readers of "git log" by saying why such
> a unification does not work in the proposed log message.

Even though the old tortoisemerge and the new tortoisegitmerge
have completely different syntax, could we still use the existence
of one when deciding which syntax to use?

pseudo-code at the top of the scriptlet:

if test -z "$tortoisegitmerge"
then
if type tortoisegitmerge 2>&1 >/dev/null
then
tortoisegitmerge=true
else
tortoisegitmerge=false
fi
fi

...and then later merge_cmd and diff_cmd
can delegate to {diff,merge}_cmd_legacy() and
{diff,merge}_cmd_gitmerge() functions to do the work.

It's just a thought.  translate_merge_tool_path()
is too low-level to do it, but it seems like we could
get away with it by having some extra smarts in the
scriptlet.

 diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
 index 4314ad0..13cbe5b 100644
 --- a/Documentation/diff-config.txt
 +++ b/Documentation/diff-config.txt
 @@ -151,7 +151,7 @@ diff..cachetextconv::
  diff.tool::
 The diff tool to be used by linkgit:git-difftool[1].  This
 option overrides `merge.tool`, and has the same valid built-in
 -   values as `merge.tool` minus "tortoisemerge" and plus
 -   "kompare".  Any other value is treated as a custom diff tool,
 +   values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
 +   plus "kompare".  Any other value is treated as a custom diff tool,
 and there must be a corresponding `difftool..cmd`
 option.
>>>
>>> So in short, two tortoises and kompare are only valid as mergetool
>>> but cannot be used as difftool?  No, I am reading it wrong.
>>> merge.tool can be used for both, kompare can be used as difftool,
>>> and two tortoises can only be used as mergetool.
>>>
>>> This paragraph needs to be rewritten to unconfuse readers.  The
>>> original is barely intelligible, and it becomes unreadable as the
>>> set of tools subtracted by "minus" and added by "plus" grows.
>>
>> But I think this should not be part of this patch.
>
> I agree that it can be done (and it is better to be done) as a
> preparatory step.  The current text is barely readable, but with
> this patch there will be two "minus", and the result becomes
> unreadable at that point.
>
> It also could be done as a follow-up documentation readability fix.

Another thought would be to minimize this section as much
as possible and point users to "git difftool --tool-help".

We can then improve --tool-help (there are already preliminary
patches in-flight to do so) so that we do not have to maintain
this documentation in the future.

Likewise, if we are able to teach the scriptlet to choose
the best one (and not require a new scriptlet) then this
section could be left as-is for this patch.
-- 
David
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] .gitignore: ignore generated gitk-git/gitk-wish

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 .gitignore | 1 +
 1 file changed, 1 insertion(+)

diff --git a/.gitignore b/.gitignore
index aa258a6..63d4904 100644
--- a/.gitignore
+++ b/.gitignore
@@ -171,6 +171,7 @@
 /git-whatchanged
 /git-write-tree
 /git-core-*/?*
+/gitk-git/gitk-wish
 /gitweb/GITWEB-BUILD-OPTIONS
 /gitweb/gitweb.cgi
 /gitweb/static/gitweb.js
-- 
1.8.0.rc3.18.g0d9b108

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

These materials Could Normally Always be cheap links of london bracelets Based Via Flea

[lyndonhouse11]

All your family members can be * links of london sale
  * quite exclusive rear. While
getaways, 1st birthdays plus backlinks involving manchester substitute
gift-giving characteristics surface, you'll want to present the most
beneficial for you to all your family. Get your aspects further down in
addition to anyone abdominal muscles next occasion your current shopping
online. The web starts up way up entrances develop wished involving clothing
house gift items.



*http://www.cheaplinksoflondonshops.co.uk/ *



--
View this message in context: 
http://git.661346.n2.nabble.com/These-materials-Could-Normally-Always-be-cheap-links-of-london-bracelets-Based-Via-Flea-tp7576033.html
Sent from the git mailing list archive at Nabble.com.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: segmentation fault (nullpointer) with git log --submodule -p

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> I've been toying with an idea along this line.

Heh.  Just for fun, here's an uglier version:

struct wcb_data {
int had_buffer;
int using_buffer;
};
#define WITH_COMMIT_BUFFER_DATA_INIT { 0, 0 }

extern void acquire_commit_buffer(struct commit *, struct wcb_data *);
extern void done_with_commit_buffer(struct commit *, struct wcb_data *);

/*
 * usage:
 *  struct wcb_data buf = WITH_COMMIT_BUFFER_INIT;
 *
 *  with_commit_buffer(commit, buf) {
 *  ...
 *  }
 */
#define with_commit_buffer(commit, i) \
for (acquire_commit_buffer(commit, &i); \
 i.using_buffer; \
 done_with_commit_buffer(commit, &i))

void acquire_commit_buffer(struct commit *commit, struct wcb_data *i)
{
enum object_type type;
unsigned long size;

assert(!i->using_buffer);
i->using_buffer = 1;
i->had_buffer = !!commit->buffer;

if (i->had_buffer)
return;
commit->buffer = read_sha1_file(commit->object.sha1, &type, 
&size);
if (!commit->buffer)
die("unable to read commit %s", 
sha1_to_hex(commit->object.sha1));
}

void done_with_commit_buffer(struct commit *commit, struct wcb_data *i)
{
assert(i->using_buffer);
i->using_buffer = 0;

if (!i->had_buffer) {
free(commit->buffer);
commit->buffer = NULL;
}
}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"

[David Aguilar]

On Thu, Jan 24, 2013 at 11:55 AM, John Keeping  wrote:
> The "--tool-help" option to git-difftool currently displays incorrect
> output since it uses the names of the files in
> "$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
> git-mergetool--lib.
>
> Fix this by simply delegating the "--tool-help" argument to the
> show_tool_help function in git-mergetool--lib.

Very nice.

One thought I had was that the unified show_tool_help should
probably check TOOL_MODE=diff and skip over the
!can_diff entries.

The current output of "git difftool --tool-help" before your
patches has the problem that it will list tools such as
"tortoisemerge" as "valid but not available" because it
does not differentiate between missing and !can_diff.


> diff --git a/git-difftool.perl b/git-difftool.perl
> index edd0493..0a90de4 100755
> --- a/git-difftool.perl
> +++ b/git-difftool.perl
> @@ -59,57 +59,16 @@ sub find_worktree
> return $worktree;
>  }
>
> -sub filter_tool_scripts
> -{
> -   my ($tools) = @_;
> -   if (-d $_) {
> -   if ($_ ne ".") {
> -   # Ignore files in subdirectories
> -   $File::Find::prune = 1;
> -   }
> -   } else {
> -   if ((-f $_) && ($_ ne "defaults")) {
> -   push(@$tools, $_);
> -   }
> -   }
> -}
> -
>  sub print_tool_help
>  {
> -   my ($cmd, @found, @notfound, @tools);
> -   my $gitpath = Git::exec_path();
> -
> -   find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
> -
> -   foreach my $tool (@tools) {
> -   $cmd  = "TOOL_MODE=diff";
> -   $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
> -   $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
> -   $cmd .= " && can_diff >/dev/null 2>&1";
> -   if (system('sh', '-c', $cmd) == 0) {
> -   push(@found, $tool);
> -   } else {
> -   push(@notfound, $tool);
> -   }
> -   }

This is where we conflated can_diff with "is the tool available?".
The nice thing is that the old code did actually check can_diff,
but since it conflated these things it ended up putting them
in the @notfound list.  It should ignore those completely, IMO.

That could be a nice follow-up patch.  What do you think?
-- 
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/3] mergetool--lib: fix startup options for gvimdiff tool

[David Aguilar]

On Wed, Jan 23, 2013 at 11:16 PM, Alexey Shumkin  wrote:
> Options are taken from /mergetools/vim
>
> Signed-off-by: Alexey Shumkin 
> ---
>  git-gui/lib/mergetool.tcl | 8 +++-
>  1 file changed, 7 insertions(+), 1 deletion(-)

A better long-term solution might be to teach git gui to use "git difftool".

Would it be better to teach git-gui (and gitk) about mergetool/difftool?
That would allow us to possibly eliminate this duplication.

We did start towards that path when difftool learned the --extcmd
option (for use by gitk) but I have not followed through.

What do you think about trying that approach?


> diff --git a/git-gui/lib/mergetool.tcl b/git-gui/lib/mergetool.tcl
> index 3c8e73b..4fc1cab 100644
> --- a/git-gui/lib/mergetool.tcl
> +++ b/git-gui/lib/mergetool.tcl
> @@ -211,7 +211,13 @@ proc merge_resolve_tool2 {} {
> }
> }
> gvimdiff {
> -   set cmdline [list "$merge_tool_path" -f "$LOCAL" "$MERGED" 
> "$REMOTE"]
> +   if {$base_stage ne {}} {
> +   set cmdline [list "$merge_tool_path" -f -d -c "wincmd 
> J" \
> +   "$MERGED" "$LOCAL" "$BASE" "$REMOTE"]
> +   } else {
> +   set cmdline [list "$merge_tool_path" -f -d -c "wincmd 
> l" \
> +   "$LOCAL" "$MERGED" "$REMOTE"]
> +   }
> }
> kdiff3 {
> if {$base_stage ne {}} {
> --
> 1.8.1.1.10.g9255f3f
>
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majord...@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html



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

Re: [PATCH v4 0/3] Finishing touches to "push" advises

[Chris Rorvick]

On Thu, Jan 24, 2013 at 11:04 PM, Junio C Hamano  wrote:
> Would it be sufficient to do this?  I think "the tag already exists
> in the remote" is already clear that we are talking about the
> destination.

Good point.

> diff --git a/builtin/push.c b/builtin/push.c
> index a2b3fbe..78789be 100644
> --- a/builtin/push.c
> +++ b/builtin/push.c
> @@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
>"See the 'Note about fast-forwards' in 'git push --help' for 
> details.");
>
>  static const char message_advice_ref_already_exists[] =
> -   N_("Updates were rejected because the destination reference already 
> exists\n"
> +   N_("Updates were rejected because the tag already exists\n"
>"in the remote.");
>
>  static const char message_advice_ref_needs_force[] =

Looks like the new-line is now unnecessary, but that looks good to me.

Thanks,

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

Re: [PATCH v4 0/3] Finishing touches to "push" advises

[Junio C Hamano]

Chris Rorvick  writes:

> Had I written the the "already exists" advice in the context of these
> additional statuses I would have said "the destination *tag* reference
> already exists", or maybe even just "the destination *tag* already
> exists".

Yeah, now we do not use "already exists" for anything other than
refs/tags/, right?  Your rewording sounds like the right thing to
make it even clearer.

Thanks for bringing it up.  

Would it be sufficient to do this?  I think "the tag already exists
in the remote" is already clear that we are talking about the
destination.

diff --git a/builtin/push.c b/builtin/push.c
index a2b3fbe..78789be 100644
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -228,7 +228,7 @@ static const char message_advice_ref_fetch_first[] =
   "See the 'Note about fast-forwards' in 'git push --help' for 
details.");
 
 static const char message_advice_ref_already_exists[] =
-   N_("Updates were rejected because the destination reference already 
exists\n"
+   N_("Updates were rejected because the tag already exists\n"
   "in the remote.");
 
 static const char message_advice_ref_needs_force[] =
--
To unsubscribe from this list: send the line "unsubscribe 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] Update renamed file merge-file.h in Makefile

[Jiang Xin]

Oops,  I find it is already fixed in commit
a60521bc6099ce89d05ef2160d2e3c30a106fda7.

commit a60521bc6099ce89d05ef2160d2e3c30a106fda7
Author: Ramsay Jones 
Date:   Tue Jan 22 16:47:47 2013 +

Makefile: Replace merge-file.h with merge-blobs.h in LIB_H

Commit fa2364ec ("Which merge_file() function do you mean?", 06-12-2012)
renamed the files merge-file.[ch] to merge-blobs.[ch], but forgot to
rename the header file in the definition of the LIB_H macro.

Signed-off-by: Ramsay Jones 
Signed-off-by: Junio C Hamano 

(kick back to the list)

2013/1/25 Jiang Xin :
> Commit v1.8.1-rc0-3-gfa2364e renamed merge-file.h to merge-blobs.h, but
> forgot to update the reference of merge-file.h in Makefile. This would
> break the build of "po/git.pot", which depends on $(LOCALIZED_C), then
> fallback to the missing file "merge-file.h".
>
> Signed-off-by: Jiang Xin 
> ---
>  Makefile |2 +-
>  1 个文件被修改，插入 1 行(+)，删除 1 行(-)
>
> diff --git a/Makefile b/Makefile
> index 1b30d7b..a786d4c 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -649,7 +649,7 @@ LIB_H += list-objects.h
>  LIB_H += ll-merge.h
>  LIB_H += log-tree.h
>  LIB_H += mailmap.h
> -LIB_H += merge-file.h
> +LIB_H += merge-blobs.h
>  LIB_H += merge-recursive.h
>  LIB_H += mergesort.h
>  LIB_H += notes-cache.h
> --
> 1.8.1.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: What's cooking in git.git (Jan 2013, #08; Tue, 22)

[Chris Rorvick]

On Wed, Jan 23, 2013 at 3:12 PM, John Keeping  wrote:
> The existing script (git-cvsimport.perl) won't ever work with cvsps-3
> since features it relies on have been removed.

Not reporting the ancestry branch seems to be the big one.  Are there
others?  I had a version of the Perl script sort of working, but only
well enough to pass the t9600 and t9604 tests.

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

Re: [PATCH v3 01/10] wildmatch: fix "**" special case

[Junio C Hamano]

Duy Nguyen  writes:

> On Fri, Jan 25, 2013 at 1:22 AM, Junio C Hamano  wrote:
>> Duy Nguyen  writes:
>>
>>> I don't think we need to support two different sets of wildcards in
>>> the long run. I'm thinking of adding ":(glob)" with WM_PATHNAME.
>>> Pathspec without :(glob) still uses the current wildcards (i.e. no
>>> FNM_PATHNAME). At some point, like 2.0, we either switch the behavior
>>> of patterns-without-:(glob) to WM_PATHNAME, or just disable wildcards
>>> when :(glob) is not present.
>>
>> Yeah, I think that is sensible.
>>
>> I am meaning to merge your retire-fnmatch topic to 'master'.
>
> I thought you wanted it to stay in 'next' longer :-)

I only said "a bit longer than other topics", and the topic has been
cooking on 'next' for three weeks by now, which is a lot longer.  I
haven't been keeping exact tallies, but trivial ones graduate from
'next' to 'master' in 3 to 5 days, ones with medium complexity in 7
to 10 days on average, I think.

> That looks ok. You may want to mention that "**" syntax is enabled in
> .gitignore and .gitattributes as well (even without USE_WILDMATCH).

Yeah, I forgot about that behaviour, and it is a bit confusing
story.  You did that in 237ec6e (Support "**" wildcard in .gitignore
and .gitattributes, 2012-10-15).

c41244e (wildmatch: support "no FNM_PATHNAME" mode, 2013-01-01) that
is part of the retire-fnmatch topic, changes the behaviour of
wildmatch() with respect to /**/ magic drastically, but everything
cancels out in the end:

 - With the current master without nd/retire-fnmatch, wildmatch()
   always works in WM_PATHNAME mode wrt '/**/'; match_pathname()
   that is used for attr/ignore matching uses wildmatch() so a
   pattern "**/Makefile" matches "Makefile" at the top, affected by
   the "**/" magic;

 - After merging nd/retire-fnmatch, wildmatch() needs WM_PATHNAME
   passed in order to grok '/**/' magic, but match_pathname() is
   changed in the same commit to pass WM_PATHNAME, so the "**/"
   magic is retained for ignore/attr matching.

> We
> could even stop the behavior change in for-each-ref (FNM_PATHNAME in
> general) but I guess because it's a nice regression, nobody would
> complain.

That is not a "regression" (whose definition is "we inadvertently
changed the behaviour and fixed that once, but the breakage came
back").  It is a behaviour change that is backward incompatible.

I would agree that it is a nice behaviour change, though ;-)

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

Git 1.8.2 l10n round 1

[Jiang Xin]

Dear l10n team members,

New "git.pot" is generated from v1.8.1-476-gec3ae6e in the master branch.

l10n: Update git.pot (11 new, 7 removed messages)

Generate po/git.pot from v1.8.1-476-gec3ae6e, and there are 11 new and 7
removed messages.

Signed-off-by: Jiang Xin 

This update is for the l10n of upcoming git 1.8.2. You can get it from
the usual place:

https://github.com/git-l10n/git-po/

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

Re: [PATCH v4 0/3] Finishing touches to "push" advises

[Chris Rorvick]

On Wed, Jan 23, 2013 at 3:55 PM, Junio C Hamano  wrote:
> This builds on Chris Rorvick's earlier effort to forbid unforced
> updates to refs/tags/ hierarchy and giving sensible error and advise
> messages for that case (we are not rejecting such a push due to fast
> forwardness, and suggesting to fetch and integrate before pushing
> again does not make sense).

FWIW, these changes look good to me.  The logic in
set_ref_status_for_push() is easier to follow and the additional error
statuses (and associated advice) make things much clearer.

Had I written the the "already exists" advice in the context of these
additional statuses I would have said "the destination *tag* reference
already exists", or maybe even just "the destination *tag* already
exists".  It's probably fine the way it is, but I only avoided using
"tag" in the advice because I was abusing it.

Thanks,

Chris
--
To unsubscribe from this list: send the line "unsubscribe 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] t9902: protect test from stray build artifacts

[Jeff King]

On Thu, Jan 24, 2013 at 08:19:30PM -0800, Junio C Hamano wrote:

> > Ahh, ok, we show one element per line and just make sure "bundle"
> > is there, and we do not care what other buns appear in the output.
> >
> Not so quick, though.  The lower level "read from help -a" is only
> run once and its output kept in a two-level cache hierarchy; we need
> to reset both.

Ugh, I didn't even think about that.

I wonder if it would be simpler if the completion tests actually ran a
new bash for each test. That would be slower, but it somehow seems
cleaner.

> It starts to look a bit too intimately tied to the implementation of
> what is being tested for my taste, though.
> [...]
> +test_expect_success 'help -a read correctly by command list generator' '
> + __git_all_commands= &&
> + __git_porcelain_commands= &&
> + GIT_TESTING_COMMAND_COMPLETION= &&
> + run_completion "git bun" &&
> + grep "^bundle $" out
> +'

Agreed. I could take or leave it at this point. It's nice to check that
changes to "help -a" will not break it, but ultimately it feels a bit
too contrived to catch anything useful.

-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: segmentation fault (nullpointer) with git log --submodule -p

[Junio C Hamano]

Jeff King  writes:

> ... I know it gets the job done, but in my
> experience, macros which do not behave syntactically like functions are
> usually a good sign that you are doing something gross and
> unmaintainable.

Yeah, it is a bit too cute for my taste, too, even though it was fun
;-)
--
To unsubscribe from this list: send the line "unsubscribe 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] t9902: protect test from stray build artifacts

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> This looks good to me.
>>
>> The only thing I might add is a test just to double-check that "git help
>> -a" is parsed correctly. Like:
>>
>>   test_expect_success 'command completion works without test harness' '
>>GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>>grep "^bundle\$" out
>>   '
>>
>> (we know we are running bash here, so the one-shot variable is OK to be
>> used with a function).
>
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
>
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.
>
> Will squash in, then.

Not so quick, though.  The lower level "read from help -a" is only
run once and its output kept in a two-level cache hierarchy; we need
to reset both.

It starts to look a bit too intimately tied to the implementation of
what is being tested for my taste, though.

 t/t9902-completion.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index adc1372..bb6ee1a 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -286,4 +286,14 @@ test_expect_success 'send-email' '
test_completion "git send-email ma" "master "
 '
 
+# This is better to be at the end, as it resets the state by tweaking
+# the internals.
+test_expect_success 'help -a read correctly by command list generator' '
+   __git_all_commands= &&
+   __git_porcelain_commands= &&
+   GIT_TESTING_COMMAND_COMPLETION= &&
+   run_completion "git bun" &&
+   grep "^bundle $" out
+'
+
 test_done
-- 
1.8.1.1.525.gdace530

--
To unsubscribe from this list: send the line "unsubscribe 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] t9902: protect test from stray build artifacts

[Jeff King]

On Thu, Jan 24, 2013 at 08:11:07PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > This looks good to me.
> >
> > The only thing I might add is a test just to double-check that "git help
> > -a" is parsed correctly. Like:
> >
> >   test_expect_success 'command completion works without test harness' '
> >GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
> >grep "^bundle\$" out
> >   '
> >
> > (we know we are running bash here, so the one-shot variable is OK to be
> > used with a function).
> 
> I think you meant "^bundle $" there, but don't we have the same
> problem when there is an end-user subcommand "git bunny"?
> 
> Ahh, ok, we show one element per line and just make sure "bundle"
> is there, and we do not care what other buns appear in the output.

Exactly. At least that was the intent; I typed it straight into my MUA. :)

-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] t9902: protect test from stray build artifacts

[Junio C Hamano]

Jeff King  writes:

> This looks good to me.
>
> The only thing I might add is a test just to double-check that "git help
> -a" is parsed correctly. Like:
>
>   test_expect_success 'command completion works without test harness' '
>GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>grep "^bundle\$" out
>   '
>
> (we know we are running bash here, so the one-shot variable is OK to be
> used with a function).

I think you meant "^bundle $" there, but don't we have the same
problem when there is an end-user subcommand "git bunny"?

Ahh, ok, we show one element per line and just make sure "bundle"
is there, and we do not care what other buns appear in the output.

Will squash in, then.




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

Re: segmentation fault (nullpointer) with git log --submodule -p

[Jeff King]

On Thu, Jan 24, 2013 at 07:59:58PM -0800, Junio C Hamano wrote:

> Junio C Hamano  writes:
> 
> > Jeff King  writes:
> >
> >> ... (e.g., how should "log" know that a submodule diff might later want
> >> to see the same entry? Should we optimistically free and then make it
> >> easier for the later user to reliably ensure the buffer is primed? Or
> >> should we err on the side of keeping it in place?).
> >
> > My knee-jerk reaction is that we should consider that commit->buffer
> > belongs to the revision traversal machinery.  Any other uses bolted
> > on later can borrow it if buffer still exists (I do not think pretty
> > code rewrites the buffer contents in place in any way), or they can
> > ask read_sha1_file() to read it themselves and free when they are
> > done.
> 
> I've been toying with an idea along this line.
> 
>  commit.h| 16 
>  builtin/blame.c | 27 ---
>  commit.c| 20 
>  3 files changed, 44 insertions(+), 19 deletions(-)

I think we are on the same page as far as what needs to happen at the
call sites.

My suggested implementation had a separate buffer, but you are right
that we may need to actually set "commit->buffer" because sub-functions
expect to find it there (the alternative might be cleaning up the
sub-function interfaces). I haven't looked at the call-sites yet.

This:

> +extern int ensure_commit_buffer(struct commit *);
> +extern void discard_commit_buffer(struct commit *);
> +
> +#define with_commit_buffer(commit) \
> + do { \
> + int had_buffer_ = !!commit->buffer; \
> + if (!had_buffer_) \
> + ensure_commit_buffer(commit); \
> + do
> +
> +#define done_with_commit_buffer(commit) \
> + while (0); \
> + if (!had_buffer_) \
> + discard_commit_buffer(commit); \
> + } while (0)

is pretty nasty, though. I know it gets the job done, but in my
experience, macros which do not behave syntactically like functions are
usually a good sign that you are doing something gross and
unmaintainable.

I dunno.

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

Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from

[Duy Nguyen]

On Fri, Jan 25, 2013 at 10:58 AM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano  wrote:
  How about this way instead: we keep track of where objects come from
  so we can verify object source when we create or update something
  that contains SHA-1.
>>>
>>> The overall approach taken by this series may be worth considering, but
>>> I do not think the check implemented here is correct.
>>>
>>> An object may be found in an alternate odb but we may also have our
>>> own copy of the same object.  You need to prove that a suspicious
>>> object is visible to us *ONLY* through add_submodule_odb().
>>
>> The way alt odbs are linked (new odbs area always at the end), if we
>> have the same copy, their copy will never be read (we check out alt
>> odbs from head to tail). So those duplicate suspicious objects are
>> actually invisible to us.
>
> The way I read find_pack_entry() is that our heuristics to start
> our search by looking at the pack in which we found an object the
> last time will invalidate your assumption above.  Am I mistaken?

No, you are right. Loose objects are always searched from the start of
alt odb list. Packs are searched till the end then back to head again.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: segmentation fault (nullpointer) with git log --submodule -p

[Junio C Hamano]

Junio C Hamano  writes:

> Jeff King  writes:
>
>> ... (e.g., how should "log" know that a submodule diff might later want
>> to see the same entry? Should we optimistically free and then make it
>> easier for the later user to reliably ensure the buffer is primed? Or
>> should we err on the side of keeping it in place?).
>
> My knee-jerk reaction is that we should consider that commit->buffer
> belongs to the revision traversal machinery.  Any other uses bolted
> on later can borrow it if buffer still exists (I do not think pretty
> code rewrites the buffer contents in place in any way), or they can
> ask read_sha1_file() to read it themselves and free when they are
> done.

I've been toying with an idea along this line.

 commit.h| 16 
 builtin/blame.c | 27 ---
 commit.c| 20 
 3 files changed, 44 insertions(+), 19 deletions(-)

diff --git a/commit.h b/commit.h
index c16c8a7..b559535 100644
--- a/commit.h
+++ b/commit.h
@@ -226,4 +226,20 @@ extern void print_commit_list(struct commit_list *list,
  const char *format_cur,
  const char *format_last);
 
+extern int ensure_commit_buffer(struct commit *);
+extern void discard_commit_buffer(struct commit *);
+
+#define with_commit_buffer(commit) \
+   do { \
+   int had_buffer_ = !!commit->buffer; \
+   if (!had_buffer_) \
+   ensure_commit_buffer(commit); \
+   do
+
+#define done_with_commit_buffer(commit) \
+   while (0); \
+   if (!had_buffer_) \
+   discard_commit_buffer(commit); \
+   } while (0)
+
 #endif /* COMMIT_H */
diff --git a/builtin/blame.c b/builtin/blame.c
index b431ba3..8b2e4a5 100644
--- a/builtin/blame.c
+++ b/builtin/blame.c
@@ -1424,25 +1424,14 @@ static void get_commit_info(struct commit *commit,
 
commit_info_init(ret);
 
-   /*
-* We've operated without save_commit_buffer, so
-* we now need to populate them for output.
-*/
-   if (!commit->buffer) {
-   enum object_type type;
-   unsigned long size;
-   commit->buffer =
-   read_sha1_file(commit->object.sha1, &type, &size);
-   if (!commit->buffer)
-   die("Cannot read commit %s",
-   sha1_to_hex(commit->object.sha1));
-   }
-   encoding = get_log_output_encoding();
-   reencoded = logmsg_reencode(commit, encoding);
-   message   = reencoded ? reencoded : commit->buffer;
-   get_ac_line(message, "\nauthor ",
-   &ret->author, &ret->author_mail,
-   &ret->author_time, &ret->author_tz);
+   with_commit_buffer(commit) {
+   encoding = get_log_output_encoding();
+   reencoded = logmsg_reencode(commit, encoding);
+   message   = reencoded ? reencoded : commit->buffer;
+   get_ac_line(message, "\nauthor ",
+   &ret->author, &ret->author_mail,
+   &ret->author_time, &ret->author_tz);
+   } done_with_commit_buffer(commit);
 
if (!detailed) {
free(reencoded);
diff --git a/commit.c b/commit.c
index e8eb0ae..a627eea 100644
--- a/commit.c
+++ b/commit.c
@@ -1357,3 +1357,23 @@ void print_commit_list(struct commit_list *list,
printf(format, sha1_to_hex(list->item->object.sha1));
}
 }
+
+int ensure_commit_buffer(struct commit *commit)
+{
+   enum object_type type;
+   unsigned long size;
+
+   if (commit->buffer)
+   return 0;
+   commit->buffer = read_sha1_file(commit->object.sha1, &type, &size);
+   if (commit->buffer)
+   return -1;
+   else
+   return 0;
+}
+
+void discard_commit_buffer(struct commit *commit)
+{
+   free(commit->buffer);
+   commit->buffer = NULL;
+}
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from

[Junio C Hamano]

Duy Nguyen  writes:

> On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano  wrote:
>>>  How about this way instead: we keep track of where objects come from
>>>  so we can verify object source when we create or update something
>>>  that contains SHA-1.
>>
>> The overall approach taken by this series may be worth considering, but
>> I do not think the check implemented here is correct.
>>
>> An object may be found in an alternate odb but we may also have our
>> own copy of the same object.  You need to prove that a suspicious
>> object is visible to us *ONLY* through add_submodule_odb().
>
> The way alt odbs are linked (new odbs area always at the end), if we
> have the same copy, their copy will never be read (we check out alt
> odbs from head to tail). So those duplicate suspicious objects are
> actually invisible to us.

The way I read find_pack_entry() is that our heuristics to start
our search by looking at the pack in which we found an object the
last time will invalidate your assumption above.  Am I mistaken?
--
To unsubscribe from this list: send the line "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] Update renamed file merge-file.h in Makefile

[Jiang Xin]

Commit v1.8.1-rc0-3-gfa2364e renamed merge-file.h to merge-blobs.h, but
forgot to update the reference of merge-file.h in Makefile. This would
break the build of "po/git.pot", which depends on $(LOCALIZED_C), then
fallback to the missing file "merge-file.h".

Signed-off-by: Jiang Xin 
---
 Makefile |2 +-
 1 个文件被修改，插入 1 行(+)，删除 1 行(-)

diff --git a/Makefile b/Makefile
index 1b30d7b..a786d4c 100644
--- a/Makefile
+++ b/Makefile
@@ -649,7 +649,7 @@ LIB_H += list-objects.h
 LIB_H += ll-merge.h
 LIB_H += log-tree.h
 LIB_H += mailmap.h
-LIB_H += merge-file.h
+LIB_H += merge-blobs.h
 LIB_H += merge-recursive.h
 LIB_H += mergesort.h
 LIB_H += notes-cache.h
-- 
1.8.1.1

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

Re: [PATCH] t9902: protect test from stray build artifacts

[Jonathan Nieder]

Jeff King wrote:
> On Thu, Jan 24, 2013 at 03:07:42PM -0800, Junio C Hamano wrote[1]:

>> Instrument the completion script and give it a way for us to tell
>> what (subset of) subcommands we are going to ship.
[...]
> The only thing I might add is a test just to double-check that "git help
> -a" is parsed correctly. Like:
>
>   test_expect_success 'command completion works without test harness' '
>GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
>grep "^bundle\$" out
>   '

Yes.  Since there are no other 'git help -a' tests, I think we need
this.

Aside from that, the fix looks good to me.

Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/214167/focus=214469
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: segmentation fault (nullpointer) with git log --submodule -p

[Duy Nguyen]

On Fri, Jan 25, 2013 at 7:55 AM, Jeff King  wrote:
> On Thu, Jan 24, 2013 at 03:56:23PM -0800, Junio C Hamano wrote:
>
>> Jeff King  writes:
>>
>> > ... (e.g., how should "log" know that a submodule diff might later want
>> > to see the same entry? Should we optimistically free and then make it
>> > easier for the later user to reliably ensure the buffer is primed? Or
>> > should we err on the side of keeping it in place?).
>>
>> My knee-jerk reaction is that we should consider that commit->buffer
>> belongs to the revision traversal machinery.  Any other uses bolted
>> on later can borrow it if buffer still exists (I do not think pretty
>> code rewrites the buffer contents in place in any way), or they can
>> ask read_sha1_file() to read it themselves and free when they are
>> done.
>
> Yeah, that is probably the sanest way forward. It at least leaves
> ownership in one place, and everybody else is an opportunistic consumer.
> We do need to annotate each user site though with something like the
> "ensure" function I mentioned.
>
> If they are not owners, then the better pattern is probably something
> like:

You probably should rename "buffer" (to _buffer or something) and let
the compiler catches all the places commit->buffer illegally used.

>
>   /*
>* Get the commit buffer, either opportunistically using
>* the cached version attached to the commit object, or loading it
>* from disk if necessary.
>*/
>   const char *use_commit_buffer(struct commit *c)
>   {
>   enum object_type type;
>   unsigned long size;
>
>   if (c->buffer)
>   return c->buffer;
>   /*
>* XXX check type == OBJ_COMMIT?
>* XXX die() on NULL as a convenience to callers?
>*/
>   return read_sha1_file(c->object.sha1, &type, &size);
>   }
>
>   void unuse_commit_buffer(const char *buf, struct commit *c)
>   {
>   /*
>* If we used the cached copy attached to the commit,
>* we don't want to free it; it's not our responsibility.
>*/
>   if (buf == c->buffer)
>   return;
>
>   free((char *)buf);
>   }
>
> I suspect that putting a use/unuse pair inside format_commit_message
> would handle most cases.
>
> -Peff
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects

[Duy Nguyen]

On Fri, Jan 25, 2013 at 2:15 AM, Junio C Hamano  wrote:
> Nguyễn Thái Ngọc Duy   writes:
>
>> Signed-off-by: Nguyễn Thái Ngọc Duy 
>> ---
>>  read-cache.c | 20 
>>  1 file changed, 20 insertions(+)
>>
>> diff --git a/read-cache.c b/read-cache.c
>> index fda78bc..4579215 100644
>> --- a/read-cache.c
>> +++ b/read-cache.c
>> @@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, 
>> struct cache_entry *ce,
>> ce->name + common, ce_namelen(ce) - common);
>>   }
>>
>> + if (object_database_contaminated) {
>> + struct object_info oi;
>> + switch (ce->ce_mode) {
>> + case S_IFGITLINK:
>> + break;
>> + case S_IFDIR:
>> + if (sha1_object_info_extended(ce->sha1, &oi) != 
>> OBJ_TREE ||
>
> This case should never happen.  Do we store any tree object in the
> index in the first place?

No it was copy/paste mistake (from cache-tree.c, before I deleted it
and added check_sha1_file_for_external_source in 3/7)

>> + (oi.alt && oi.alt->external))
>> + die("cannot create index referring to an 
>> external tree %s",
>> + sha1_to_hex(ce->sha1));
>> + break;
>> + default:
>> + if (sha1_object_info_extended(ce->sha1, &oi) != 
>> OBJ_BLOB ||
>> + (oi.alt && oi.alt->external))
>> + die("cannot create index referring to an 
>> external blob %s",
>> + sha1_to_hex(ce->sha1));
>> + break;
>> + }
>> + }
>> +
>>   result = ce_write(c, fd, ondisk, size);
>>   free(ondisk);
>>   return result;
>
> I think the check you want to add is to the cache-tree code; before
> writing the new index out, if you suspect you might have called
> cache_tree_update() before, invalidate any hierarchy that is
> recorded to produce a tree object that refers to an object that is
> only available through external object store.

cache-tree is covered by check_sha1_file_for_external_source() when it
actually writes a tree (dry-run mode goes through unchecked). Even
when cache-tree is not involved, I do not want the index to point to
an non-existing SHA-1 ("git diff --cached" may fail next time, for
example).

> As to the logic to check if a object is something that we should
> refuse to create a new dependent, I think you should not butcher
> sha1_object_info_extended(), and instead add a new call that checks
> if a given SHA-1 yields an object if you ignore alternate object
> databases that are marked as "external", whose signature may
> resemble:
>
> int has_sha1_file_proper(const unsigned char *sha1);
>
> or something.

Agreed.

> This is a tangent, but in addition, you may want to add an even
> narrower variant that checks the same but ignoring _all_ alternate
> object databases, "external" or not:
>
> int has_sha1_file_local(const unsigned char *sha1);
>
> That may be useful if we want to later make the alternate safer to
> use; instead of letting the codepath to create an object ask
> has_sha1_file() to see if it already exists and refrain from writing
> a new copy, we switch to ask has_sha1_file_locally() and even if an
> alternate object database we borrow from has it, we keep our own
> copy in our repository.
>
> Not tested beyond syntax, but rebasing something like this patch on
> top of your "mark external sources" change may take us closer to
> that.

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

Re: [PATCH v3 01/10] wildmatch: fix "**" special case

[Duy Nguyen]

On Fri, Jan 25, 2013 at 1:22 AM, Junio C Hamano  wrote:
> Duy Nguyen  writes:
>
>> I don't think we need to support two different sets of wildcards in
>> the long run. I'm thinking of adding ":(glob)" with WM_PATHNAME.
>> Pathspec without :(glob) still uses the current wildcards (i.e. no
>> FNM_PATHNAME). At some point, like 2.0, we either switch the behavior
>> of patterns-without-:(glob) to WM_PATHNAME, or just disable wildcards
>> when :(glob) is not present.
>
> Yeah, I think that is sensible.
>
> I am meaning to merge your retire-fnmatch topic to 'master'.

I thought you wanted it to stay in 'next' longer :-)

> What should the Release Notes say for the upcoming release?
>
> You can build with USE_WILDMATCH=YesPlease to use a replacement
> implementation of pattern matching logic used for pathname-like
> things, e.g.  refnames and paths in the repository.  The new
> implementation is not expected change the existing behaviour of
> Git at all, except for "git for-each-ref" where you can now say
> "refs/**/master" and match with both refs/heads/master and
> refs/remotes/origin/master.  In future versions of Git, we plan
> to use this new implementation in wider places (e.g. "git log
> '**/t*.sh'" may find commits that touch a shell script whose
> name begins with "t" at any level), but we are not there yet.
> By building with USE_WILDMATCH, using the resulting Git daily
> and reporting when you find breakages, you can help us get
> closer to that goal.

That looks ok. You may want to mention that "**" syntax is enabled in
.gitignore and .gitattributes as well (even without USE_WILDMATCH). We
could even stop the behavior change in for-each-ref (FNM_PATHNAME in
general) but I guess because it's a nice regression, nobody would
complain.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from

[Duy Nguyen]

On Fri, Jan 25, 2013 at 12:45 AM, Junio C Hamano  wrote:
>>  How about this way instead: we keep track of where objects come from
>>  so we can verify object source when we create or update something
>>  that contains SHA-1.
>
> The overall approach taken by this series may be worth considering, but
> I do not think the check implemented here is correct.
>
> An object may be found in an alternate odb but we may also have our
> own copy of the same object.  You need to prove that a suspicious
> object is visible to us *ONLY* through add_submodule_odb().

The way alt odbs are linked (new odbs area always at the end), if we
have the same copy, their copy will never be read (we check out alt
odbs from head to tail). So those duplicate suspicious objects are
actually invisible to us.

> Once you do add_submodule_odb() to contaminate our object pool, you
> make everything a suspicious object that needs to be checked; that
> is the worst part of the story.

And because we never really touch their alt copy, the returned alt
source is "ours", trusted and not checked. The check should only occur
on objects that we do not have.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] t9902: protect test from stray build artifacts

[Jeff King]

On Thu, Jan 24, 2013 at 03:07:42PM -0800, Junio C Hamano wrote:

> When you have random build artifacts in your build directory, left
> behind by running "make" while on another branch, the "git help -a"
> command run by __git_list_all_commands in the completion script that
> is being tested does not have a way to know that they are not part
> of the subcommands this build will ship.  Such extra subcommands may
> come from the user's $PATH.  They will interfere with the tests that
> expect a certain prefix to uniquely expand to a known completion.
> 
> Instrument the completion script and give it a way for us to tell
> what (subset of) subcommands we are going to ship.
> 
> Also add a test to "git --help " expansion.  It needs
> to show not just commands but some selected documentation pages.
> 
> Based on an idea by Jeff King.
> 
> Signed-off-by: Junio C Hamano 
> ---
>  contrib/completion/git-completion.bash | 11 ++-
>  t/t9902-completion.sh  | 25 -
>  2 files changed, 34 insertions(+), 2 deletions(-)

This looks good to me.

The only thing I might add is a test just to double-check that "git help
-a" is parsed correctly. Like:

  test_expect_success 'command completion works without test harness' '
   GIT_TESTING_COMMAND_COMPLETION= run_completion "git bun" &&
   grep "^bundle\$" out
  '

(we know we are running bash here, so the one-shot variable is OK to be
used with a function).

-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: [regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails

[Jeff King]

On Thu, Jan 24, 2013 at 03:21:46PM -0800, Jonathan Nieder wrote:

> This broke /etc/mailname handling.  Before:
> 
>   $ git var GIT_COMMITTER_IDENT
>   Jonathan Nieder  1359069165 -0800
> 
> After:
> 
>   $ git var GIT_COMMITTER_IDENT
>   Jonathan Nieder  1359069192 -0800

Ick. I wonder how that slipped through...I know I was testing with
/etc/mailname when developing the series, because I'm on a Debian
system. We do even check this code path in t7502 (if you have the
AUTOIDENT prereq), but of course we can't verify the actual value
automatically, because it could be anything. So I guess I just missed it
during my manual testing, and the automated testing is insufficient to
catch this particular breakage.

> > -   if (!fgets(buf, len, mailname)) {
> > +   if (strbuf_getline(buf, mailname, '\n') == EOF) {
> 
> This clears the strbuf.

Right. Definitely the problem.

> How about something like this as a quick fix?
> 
> Reported-by: Mihai Rusu 
> Signed-off-by: Jonathan Nieder 
> 
> diff --git a/ident.c b/ident.c
> index 73a06a1..cabd73f 100644
> --- a/ident.c
> +++ b/ident.c
> @@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct 
> strbuf *name)
>  static int add_mailname_host(struct strbuf *buf)
>  {
>   FILE *mailname;
> + struct strbuf mailnamebuf = STRBUF_INIT;
>  
>   mailname = fopen("/etc/mailname", "r");
>   if (!mailname) {
> @@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
>   strerror(errno));
>   return -1;
>   }
> - if (strbuf_getline(buf, mailname, '\n') == EOF) {
> + if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
>   if (ferror(mailname))
>   warning("cannot read /etc/mailname: %s",
>   strerror(errno));
> + strbuf_release(&mailnamebuf);
>   fclose(mailname);
>   return -1;
>   }
>   /* success! */
> + strbuf_addbuf(buf, &mailnamebuf);
> + strbuf_release(&mailnamebuf);
>   fclose(mailname);
>   return 0;
>  }

I think that is the only reasonable fix. Thanks for figuring it out.

We could expand the test in t7502 to check for "@" in the email, but it
feels weirdly specific to this bug. Either way,

Acked-by: Jeff King 

(with a proper commit message, of course).

-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: segmentation fault (nullpointer) with git log --submodule -p

[Jeff King]

On Thu, Jan 24, 2013 at 03:56:23PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > ... (e.g., how should "log" know that a submodule diff might later want
> > to see the same entry? Should we optimistically free and then make it
> > easier for the later user to reliably ensure the buffer is primed? Or
> > should we err on the side of keeping it in place?).
> 
> My knee-jerk reaction is that we should consider that commit->buffer
> belongs to the revision traversal machinery.  Any other uses bolted
> on later can borrow it if buffer still exists (I do not think pretty
> code rewrites the buffer contents in place in any way), or they can
> ask read_sha1_file() to read it themselves and free when they are
> done.

Yeah, that is probably the sanest way forward. It at least leaves
ownership in one place, and everybody else is an opportunistic consumer.
We do need to annotate each user site though with something like the
"ensure" function I mentioned.

If they are not owners, then the better pattern is probably something
like:

  /*
   * Get the commit buffer, either opportunistically using
   * the cached version attached to the commit object, or loading it
   * from disk if necessary.
   */
  const char *use_commit_buffer(struct commit *c)
  {
  enum object_type type;
  unsigned long size;

  if (c->buffer)
  return c->buffer;
  /*
   * XXX check type == OBJ_COMMIT?
   * XXX die() on NULL as a convenience to callers?
   */
  return read_sha1_file(c->object.sha1, &type, &size);
  }

  void unuse_commit_buffer(const char *buf, struct commit *c)
  {
  /*
   * If we used the cached copy attached to the commit,
   * we don't want to free it; it's not our responsibility.
   */
  if (buf == c->buffer)
  return;

  free((char *)buf);
  }

I suspect that putting a use/unuse pair inside format_commit_message
would handle most cases.

-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: segmentation fault (nullpointer) with git log --submodule -p

[Junio C Hamano]

Jeff King  writes:

> ... (e.g., how should "log" know that a submodule diff might later want
> to see the same entry? Should we optimistically free and then make it
> easier for the later user to reliably ensure the buffer is primed? Or
> should we err on the side of keeping it in place?).

My knee-jerk reaction is that we should consider that commit->buffer
belongs to the revision traversal machinery.  Any other uses bolted
on later can borrow it if buffer still exists (I do not think pretty
code rewrites the buffer contents in place in any way), or they can
ask read_sha1_file() to read it themselves and free when they are
done.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] parse_object: clear "parsed" when freeing buffers

[Jonathon Mah]

On 2013-01-23, at 23:07, Jeff King  wrote:

> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:
> 
>> Several areas of code would free buffers for object structs that
>> contained them ("struct tree" and "struct commit"), but without clearing
>> the "parsed" flag. parse_object would clear the flag for "struct tree",
>> but commits would remain in an invalid state (marked as parsed but with
>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
>> invalid objects stay around and can lead to bad behavior.
>> 
>> In particular, running pickaxe on all refs in a repository with a cached
>> textconv could segfault. If the textconv cache ref was evaluated first
>> by cmd_log_walk, a subsequent notes_cache_match_validity call would
>> dereference NULL.
> 
> Just to be sure I understand, what is going on is something like this?
> 
> Log reads all refs, including notes. After showing the notes commit,
> log frees the buffer to save memory.  Later, doing the diff on a
> commit causes us to use the notes_cache mechanism. That will look at
> the commit at the tip of the notes ref, which log has already
> processed. It will call parse_commit, but that will do nothing, as the
> parsed flag is already set, but the commit buffer remains NULL.
> 
> I wonder if this could be the source of the segfault here, too:
> 
> http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355

Indeed, that description matches what I see. The other segfault seems to be in 
the same place too.

The segfault hits with the following stack (optimization off):

0   git-log parse_commit_header + 39 (pretty.c:738)
1   git-log format_commit_one + 3102 (pretty.c:1138)
2   git-log format_commit_item + 177 (pretty.c:1203)
3   git-log strbuf_expand + 172 (strbuf.c:247)
4   git-log format_commit_message + 196 (pretty.c:1263)
5   git-log notes_cache_match_validity + 215 (notes-cache.c:22)
6   git-log notes_cache_init + 212 (notes-cache.c:41)
7   git-log userdiff_get_textconv + 176 (userdiff.c:301)
8   git-log get_textconv + 66 (diff.c:2233)
9   git-log has_changes + 53 (diffcore-pickaxe.c:205)
10  git-log pickaxe + 299 (diffcore-pickaxe.c:40)
11  git-log diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)
12  git-log diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)
13  git-log diffcore_std + 162 (diff.c:4673)
14  git-log log_tree_diff_flush + 43 (log-tree.c:721)
15  git-log log_tree_diff + 566 (log-tree.c:826)
16  git-log log_tree_commit + 63 (log-tree.c:847)
17  git-log cmd_log_walk + 172 (log.c:301)
18  git-log cmd_log + 186 (log.c:568)
19  git-log run_builtin + 475 (git.c:273)
20  git-log handle_internal_command + 199 (git.c:434)
21  git-log main + 149 (git.c:523)
22  libdyld.dylib   start + 1

commit->message was freed and nulled earlier in a call to cmd_log_walk. This 
test reproduces it reliably on my machine:

diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces 
correct results' '
test_cmp expect actual
'

+cat >expect 

Re: segmentation fault (nullpointer) with git log --submodule -p

[Jeff King]

On Thu, Jan 24, 2013 at 09:14:47PM +0700, Nguyen Thai Ngoc Duy wrote:

> >>> I did. My bisection told me this is the suspect:
> >>>
> >>> ccdc603 (parse_object: try internal cache before reading object db)
> >>
> >> diff --git a/object.c b/object.c
> >> index d8d09f9..6b06297 100644
> >> --- a/object.c
> >> +++ b/object.c
> >> @@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char 
> >> *sha1)
> >> enum object_type type;
> >> int eaten;
> >> const unsigned char *repl = lookup_replace_object(sha1);
> >> -   void *buffer = read_sha1_file(sha1, &type, &size);
> >> +   void *buffer;
> >> +   struct object *obj;
> >> +
> >> +   obj = lookup_object(sha1);
> >> +   if (obj && obj->parsed)
> >> +   return obj;
> >>
> >> Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?
> >> What if you change that "if" to
> >>
> >> if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit
> >> *)obj)->buffer))
> >>
> >
> > No more segfault!
> 
> Sweet. I have no idea how that fixes it. Maybe Jeff can give some
> explanation after he wakes up.

Ugh. I think I know why it fixes it. We free the commit's buffer as part
of the log traversal, but then later want to access it as part of the
diff. We presumably call parse_object somewhere in the middle to make
sure it is parsed.

Before ccdc603, a side effect of parse_object is that even for a parsed
object, we would fill in the buffer field of a commit or tree. See
parse_object_buffer:

} else if (type == OBJ_COMMIT) {
struct commit *commit = lookup_commit(sha1);
if (commit) {
if (parse_commit_buffer(commit, buffer, size))
return NULL;
if (!commit->buffer) {
commit->buffer = buffer;
eaten = 1;
}
obj = &commit->object;
}

When this patch was originally proposed, I wrote[1]:

  On Thu, Jan 05, 2012 at 01:55:22PM -0800, Junio C Hamano wrote:
  > > So I think it is safe short of somebody doing some horrible manual
  > > munging of a "struct object".
  >
  > Yeah, I was worried about codepaths like commit-pretty-printing
  > might be mucking with the contents of commit->buffer, perhaps
  > reencoding the text and then calling parse_object() to get the
  > unmodified original back, or something silly like that. But the
  > lookup_object() call at the beginning of the parse_object() already
  > prevents us from doing such a thing, so we should be OK, I would
  > think.

  [...]

  What saves you is that the parse_*_buffer functions all do nothing
  when the object.parsed flag is set, and the code I added makes sure
  that object.parsed is set in the object that lookup_object returns.

  So yeah, anytime you tweak the contents of commit->buffer but don't
  unset the "parsed" flag, you are asking for trouble.

Which is true, but obviously I missed that in addition to calling
parse_*_buffer, which will be a no-op, we _also_ set the buffer
independently. So parse_object was functioning in a belt-and-suspenders
for that case. And I think this is probably the same root cause as the
segfault which came up here:

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

So what to do?

We can revert ccdc603, but I do not think we need to. We can catch the
problematic cases with something like your patch, but still get the
optimization when the buffer really is already filled in. I think we'd
need to extend your patch to handle trees, too, to be totally correct.

But there are still some loose ends that I note:

  1. Making such a change would be parse_object erring on the side of
 providing the buffer. But it doesn't actually know if the buffer is
 desired or not. For instance, upload-pack benefited from this
 optimization, but does not need save_commit_buffer on at all. So
 commit->buffer is _always_ NULL there, and that's just fine; we
 really don't need to read the object.

 Now this may be a bad example, because due to my follow-on patches,
 we avoid calling parse_object at all in most cases, so I don't
 think it matters any longer to upload-pack. But I suspect there are
 other places with similar circumstances. Fundamentally parse_object
 doesn't know what the caller is interested in.

  2. This means that parse_commit and parse_object behave differently in
 this regard. The former will leave the buffer unfilled. Meaning we
 may still have issues with code paths that munge the buffer without
 resetting the parsed flag, independent of ccdc603 and fixing this.

To me, these highlight that our commit->buffer management is fragile and
is largely about guessing in various circumstances whether somebody will
later want the buffer. I'm not sure of the right solution, though. It
seems like something th

[regression] Re: [PATCHv2 10/15] drop length limitations on gecos-derived names and emails

[Jonathan Nieder]

Hi Jeff,

Jeff King wrote:

> When we pull the user's name from the GECOS field of the
> passwd file (or generate an email address based on their
> username and hostname), we put the result into a
> static buffer.
[...]
> The conversion is mostly mechanical: replace string copies
> with strbuf equivalents, and access the strbuf.buf directly.
> There are a few exceptions:

This broke /etc/mailname handling.  Before:

$ git var GIT_COMMITTER_IDENT
Jonathan Nieder  1359069165 -0800

After:

$ git var GIT_COMMITTER_IDENT
Jonathan Nieder  1359069192 -0800

The cause:

[...]
> --- a/ident.c
> +++ b/ident.c
> @@ -19,42 +18,27 @@ int user_ident_explicitly_given;
[...]
> -static int add_mailname_host(char *buf, size_t len)
> +static int add_mailname_host(struct strbuf *buf)
>  {
>   FILE *mailname;
>  
> @@ -65,7 +49,7 @@ static int add_mailname_host(char *buf, size_t len)
>   strerror(errno));
>   return -1;
>   }
> - if (!fgets(buf, len, mailname)) {
> + if (strbuf_getline(buf, mailname, '\n') == EOF) {

This clears the strbuf.

How about something like this as a quick fix?

Reported-by: Mihai Rusu 
Signed-off-by: Jonathan Nieder 

diff --git a/ident.c b/ident.c
index 73a06a1..cabd73f 100644
--- a/ident.c
+++ b/ident.c
@@ -41,6 +41,7 @@ static void copy_gecos(const struct passwd *w, struct strbuf 
*name)
 static int add_mailname_host(struct strbuf *buf)
 {
FILE *mailname;
+   struct strbuf mailnamebuf = STRBUF_INIT;
 
mailname = fopen("/etc/mailname", "r");
if (!mailname) {
@@ -49,14 +50,17 @@ static int add_mailname_host(struct strbuf *buf)
strerror(errno));
return -1;
}
-   if (strbuf_getline(buf, mailname, '\n') == EOF) {
+   if (strbuf_getline(&mailnamebuf, mailname, '\n') == EOF) {
if (ferror(mailname))
warning("cannot read /etc/mailname: %s",
strerror(errno));
+   strbuf_release(&mailnamebuf);
fclose(mailname);
return -1;
}
/* success! */
+   strbuf_addbuf(buf, &mailnamebuf);
+   strbuf_release(&mailnamebuf);
fclose(mailname);
return 0;
 }
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH] t9902: protect test from stray build artifacts

[Junio C Hamano]

When you have random build artifacts in your build directory, left
behind by running "make" while on another branch, the "git help -a"
command run by __git_list_all_commands in the completion script that
is being tested does not have a way to know that they are not part
of the subcommands this build will ship.  Such extra subcommands may
come from the user's $PATH.  They will interfere with the tests that
expect a certain prefix to uniquely expand to a known completion.

Instrument the completion script and give it a way for us to tell
what (subset of) subcommands we are going to ship.

Also add a test to "git --help " expansion.  It needs
to show not just commands but some selected documentation pages.

Based on an idea by Jeff King.

Signed-off-by: Junio C Hamano 
---
 contrib/completion/git-completion.bash | 11 ++-
 t/t9902-completion.sh  | 25 -
 2 files changed, 34 insertions(+), 2 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index a4c48e1..6139b50 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,10 +531,19 @@ __git_complete_strategy ()
return 1
 }
 
+__git_commands () {
+   if test -n "${GIT_TESTING_COMMAND_COMPLETION:-}"
+   then
+   printf "%s" "${GIT_TESTING_COMMAND_COMPLETION}"
+   else
+   git help -a|egrep '^  [a-zA-Z0-9]'
+   fi
+}
+
 __git_list_all_commands ()
 {
local i IFS=" "$'\n'
-   for i in $(git help -a|egrep '^  [a-zA-Z0-9]')
+   for i in $(__git_commands)
do
case $i in
*--*) : helper pattern;;
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..adc1372 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -13,6 +13,25 @@ complete ()
return 0
 }
 
+# Be careful when updating this list:
+#
+# (1) The build tree may have build artifact from different branch, or
+# the user's $PATH may have a random executable that may begin
+# with "git-check" that are not part of the subcommands this build
+# will ship, e.g.  "check-ignore".  The tests for completion for
+# subcommand names tests how "check" is expanded; we limit the
+# possible candidates to "checkout" and "check-attr" to make sure
+# "check-attr", which is known by the filter function as a
+# subcommand to be thrown out, while excluding other random files
+# that happen to begin with "check" to avoid letting them get in
+# the way.
+#
+# (2) A test makes sure that common subcommands are included in the
+# completion for "git ", and a plumbing is excluded.  "add",
+# "filter-branch" and "ls-files" are listed for this.
+
+GIT_TESTING_COMMAND_COMPLETION='add checkout check-attr filter-branch ls-files'
+
 . "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
 
 # We don't need this function to actually join words or do anything special.
@@ -196,7 +215,6 @@ test_expect_success 'general options plus command' '
test_completion "git --paginate check" "checkout " &&
test_completion "git --git-dir=foo check" "checkout " &&
test_completion "git --bare check" "checkout " &&
-   test_completion "git --help des" "describe " &&
test_completion "git --exec-path=foo check" "checkout " &&
test_completion "git --html-path check" "checkout " &&
test_completion "git --no-pager check" "checkout " &&
@@ -207,6 +225,11 @@ test_expect_success 'general options plus command' '
test_completion "git --no-replace-objects check" "checkout "
 '
 
+test_expect_success 'git --help completion' '
+   test_completion "git --help ad" "add " &&
+   test_completion "git --help core" "core-tutorial "
+'
+
 test_expect_success 'setup for ref completion' '
echo content >file1 &&
echo more >file2 &&
--
To unsubscribe from this list: send the line "unsubscribe 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] t9902: Instruct git-completion.bash about a test mode

[Jean-Noël AVILA]

Le jeudi 24 janvier 2013 23:29:33, Junio C Hamano a écrit :
> "Jean-Noël AVILA"  writes:
> 
> > My rational was to be sure to put the environment variable out of the 
> > way once the script has been sourced. I can make two alternative 
> > definitions of __git_list_all_commands () depending on the presence of 
> > $AUTHORIZED_CMD_LIST if you are worried about performance.
> 
> Actually, it does not matter, as once __git_list_all_commands is
> run, its result is kept in a variable.
> 
> So Peff's
> 
>   if test -z "$FAKE_COMMAND_LIST"; then
>   __git_cmdlist() {
>   git help -a | egrep '^  [a-zA-Z0-9]'
>   }
>   else
>   __git_cmdlist() {
>   printf '%s' "$FAKE_COMMAND_LIST"
>   }
>   fi
> 
> would be just as simple if not simpler, does the same thing, and is
> sufficient, I think.
> 
> The t9902 test is only interested in making sure that the completion
> works, and we do not want "git help -a" that omits a subcommand from
> its output that is not built in your particular environment to get
> in the way, which will not be an issue with this approach.
> 


Ah. I totally missed the point. I though that the requested change was
to intersect the list needed for the test with the one provided by the 
standard completion.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v4 1/3] push: further clean up fields of "struct ref"

[Eric Sunshine]

On Wed, Jan 23, 2013 at 4:55 PM, Junio C Hamano  wrote:
> The "nonfastforward" and "update" fields are only used while
> deciding what value to assign to the "status" locally in a single
> function.  Remove them from the "struct ref".
>
> The "requires_force" field is not used to decide if the proposed
> update requires a --force option to succeed, or to record such a
> decision made elsewhere.  It is used by status reporting code that
> the particular update was "forced".  Rename it to "forced_udpate",

s/udpate/update/
--
To unsubscribe from this list: send the line "unsubscribe 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] t9902: Instruct git-completion.bash about a test mode

[Junio C Hamano]

"Jean-Noël AVILA"  writes:

> My rational was to be sure to put the environment variable out of the 
> way once the script has been sourced. I can make two alternative 
> definitions of __git_list_all_commands () depending on the presence of 
> $AUTHORIZED_CMD_LIST if you are worried about performance.

Actually, it does not matter, as once __git_list_all_commands is
run, its result is kept in a variable.

So Peff's

  if test -z "$FAKE_COMMAND_LIST"; then
  __git_cmdlist() {
  git help -a | egrep '^  [a-zA-Z0-9]'
  }
  else
  __git_cmdlist() {
  printf '%s' "$FAKE_COMMAND_LIST"
  }
  fi

would be just as simple if not simpler, does the same thing, and is
sufficient, I think.

The t9902 test is only interested in making sure that the completion
works, and we do not want "git help -a" that omits a subcommand from
its output that is not built in your particular environment to get
in the way, which will not be an issue with this approach.

--
To unsubscribe from this list: send the line "unsubscribe 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] mergetools: Add tortoisegitmerge helper

[Junio C Hamano]

Sven Strickroth  writes:

> Am 24.01.2013 20:51 schrieb Junio C Hamano:
>> Sven Strickroth  writes:
>> 
>>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>>>   (starting with 1.8.0) in order to make clear that this one has special
>>>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>>>   version.
>> 
>> Wouldn't it make more sense in such a situation if your users can
>> keep using the old "tortoisemerge" configured in their configuration
>> and when the renamed one is found the mergetool automatically used
>> it, rather than the way your patch is done?
>
> That was also my first idea, however, TortoiseMerge uses parameters as
> follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
> space from keys: '-base "$BASE"'. So both are incompatible (the first
> approach has problems with spaces in filenames, the TortoiseGitMerge
> approach fixes this).

OK.  Please unconfuse future readers of "git log" by saying why such
a unification does not work in the proposed log message.

>>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>>> index 4314ad0..13cbe5b 100644
>>> --- a/Documentation/diff-config.txt
>>> +++ b/Documentation/diff-config.txt
>>> @@ -151,7 +151,7 @@ diff..cachetextconv::
>>>  diff.tool::
>>> The diff tool to be used by linkgit:git-difftool[1].  This
>>> option overrides `merge.tool`, and has the same valid built-in
>>> -   values as `merge.tool` minus "tortoisemerge" and plus
>>> -   "kompare".  Any other value is treated as a custom diff tool,
>>> +   values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>>> +   plus "kompare".  Any other value is treated as a custom diff tool,
>>> and there must be a corresponding `difftool..cmd`
>>> option.
>> 
>> So in short, two tortoises and kompare are only valid as mergetool
>> but cannot be used as difftool?  No, I am reading it wrong.
>> merge.tool can be used for both, kompare can be used as difftool,
>> and two tortoises can only be used as mergetool.
>> 
>> This paragraph needs to be rewritten to unconfuse readers.  The
>> original is barely intelligible, and it becomes unreadable as the
>> set of tools subtracted by "minus" and added by "plus" grows.
>
> But I think this should not be part of this patch.

I agree that it can be done (and it is better to be done) as a
preparatory step.  The current text is barely readable, but with
this patch there will be two "minus", and the result becomes
unreadable at that point.

It also could be done as a follow-up documentation readability fix.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] mergetools: Add tortoisegitmerge helper

[Sven Strickroth]

Am 24.01.2013 20:51 schrieb Junio C Hamano:
> Sven Strickroth  writes:
> 
>> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>>   (starting with 1.8.0) in order to make clear that this one has special
>>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>>   version.
> 
> Wouldn't it make more sense in such a situation if your users can
> keep using the old "tortoisemerge" configured in their configuration
> and when the renamed one is found the mergetool automatically used
> it, rather than the way your patch is done?

That was also my first idea, however, TortoiseMerge uses parameters as
follows: '-base:"$BASE"'. TortoiseGitMerge uses values separated by
space from keys: '-base "$BASE"'. So both are incompatible (the first
approach has problems with spaces in filenames, the TortoiseGitMerge
approach fixes this).

>> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
>> index 4314ad0..13cbe5b 100644
>> --- a/Documentation/diff-config.txt
>> +++ b/Documentation/diff-config.txt
>> @@ -151,7 +151,7 @@ diff..cachetextconv::
>>  diff.tool::
>>  The diff tool to be used by linkgit:git-difftool[1].  This
>>  option overrides `merge.tool`, and has the same valid built-in
>> -values as `merge.tool` minus "tortoisemerge" and plus
>> -"kompare".  Any other value is treated as a custom diff tool,
>> +values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
>> +plus "kompare".  Any other value is treated as a custom diff tool,
>>  and there must be a corresponding `difftool..cmd`
>>  option.
> 
> So in short, two tortoises and kompare are only valid as mergetool
> but cannot be used as difftool?  No, I am reading it wrong.
> merge.tool can be used for both, kompare can be used as difftool,
> and two tortoises can only be used as mergetool.
> 
> This paragraph needs to be rewritten to unconfuse readers.  The
> original is barely intelligible, and it becomes unreadable as the
> set of tools subtracted by "minus" and added by "plus" grows.

But I think this should not be part of this patch.

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server
--
To unsubscribe from this list: send the line "unsubscribe 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] t9902: Instruct git-completion.bash about a test mode

[Jean-Noël AVILA]

Le jeudi 24 janvier 2013 22:46:13, Junio C Hamano a écrit :
> "Jean-Noël AVILA"  writes:
> 
> > In test mode, git completion should propose commands only if they
> > belong to the list of authorized commands.
> >
> > Signed-off-by: Jean-Noel Avila 
> > ---
> >
> > Better show some code than try to explain. Here is what I mean by
> > "filter the output git help -a". 
> 
> Why do you have to make an extra shell function call for each and
> every possible Git subcommand to slow down everybody's work when not
> in testing mode?
> 

My rational was to be sure to put the environment variable out of the 
way once the script has been sourced. I can make two alternative 
definitions of __git_list_all_commands () depending on the presence of 
$AUTHORIZED_CMD_LIST if you are worried about performance.

> Comparing it with Peff's suggestion, it is fairly clear which one we
> should pick, I think.
> 
> 
> 
> >  contrib/completion/git-completion.bash | 16 +++-
> >  t/t9902-completion.sh  |  4 ++--
> >  2 files changed, 17 insertions(+), 3 deletions(-)
> >
> > diff --git a/contrib/completion/git-completion.bash 
> > b/contrib/completion/git-completion.bash
> > index 14dd5e7..6490553 100644
> > --- a/contrib/completion/git-completion.bash
> > +++ b/contrib/completion/git-completion.bash
> > @@ -531,6 +531,20 @@ __git_complete_strategy ()
> > return 1
> >  }
> >  
> > +if test -z "$AUTHORIZED_CMD_LIST"; then
> > +   __git_cmdlist ()
> > +   { 
> > +   echo $1;
> > +   }
> > +else
> > +   __git_cmdlist ()
> > +   { 
> > +   if [[ " $AUTHORIZED_CMD_LIST " =~ " $1 " ]] ; then
> > +   echo $1
> > +   fi
> > +   }
> > +fi
> > +
> >  __git_list_all_commands ()
> >  {
> > local i IFS=" "$'\n'
> > @@ -538,7 +552,7 @@ __git_list_all_commands ()
> > do
> > case $i in
> > *--*) : helper pattern;;
> > -   *) echo $i;;
> > +   *) __git_cmdlist $i;;
> > esac
> > done
> >  }
> > diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> > index 3cd53f8..5e7d81e 100755
> > --- a/t/t9902-completion.sh
> > +++ b/t/t9902-completion.sh
> > @@ -12,8 +12,8 @@ complete ()
> > # do nothing
> > return 0
> >  }
> > -
> > -. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
> > +AUTHORIZED_CMD_LIST=" checkout show add filter-branch ls-files send-email 
> > describe"
> > +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" 
> >  
> >  # We don't need this function to actually join words or do anything 
> > special.
> >  # Also, it's cleaner to avoid touching bash's internal completion 
> > variables.
> --
> To unsubscribe from this list: send the line "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] t9902: Instruct git-completion.bash about a test mode

[Junio C Hamano]

"Jean-Noël AVILA"  writes:

> In test mode, git completion should propose commands only if they
> belong to the list of authorized commands.
>
> Signed-off-by: Jean-Noel Avila 
> ---
>
> Better show some code than try to explain. Here is what I mean by
> "filter the output git help -a". 

Why do you have to make an extra shell function call for each and
every possible Git subcommand to slow down everybody's work when not
in testing mode?

Comparing it with Peff's suggestion, it is fairly clear which one we
should pick, I think.



>  contrib/completion/git-completion.bash | 16 +++-
>  t/t9902-completion.sh  |  4 ++--
>  2 files changed, 17 insertions(+), 3 deletions(-)
>
> diff --git a/contrib/completion/git-completion.bash 
> b/contrib/completion/git-completion.bash
> index 14dd5e7..6490553 100644
> --- a/contrib/completion/git-completion.bash
> +++ b/contrib/completion/git-completion.bash
> @@ -531,6 +531,20 @@ __git_complete_strategy ()
>   return 1
>  }
>  
> +if test -z "$AUTHORIZED_CMD_LIST"; then
> + __git_cmdlist ()
> + { 
> + echo $1;
> + }
> +else
> + __git_cmdlist ()
> + { 
> + if [[ " $AUTHORIZED_CMD_LIST " =~ " $1 " ]] ; then
> + echo $1
> + fi
> + }
> +fi
> +
>  __git_list_all_commands ()
>  {
>   local i IFS=" "$'\n'
> @@ -538,7 +552,7 @@ __git_list_all_commands ()
>   do
>   case $i in
>   *--*) : helper pattern;;
> - *) echo $i;;
> + *) __git_cmdlist $i;;
>   esac
>   done
>  }
> diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
> index 3cd53f8..5e7d81e 100755
> --- a/t/t9902-completion.sh
> +++ b/t/t9902-completion.sh
> @@ -12,8 +12,8 @@ complete ()
>   # do nothing
>   return 0
>  }
> -
> -. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
> +AUTHORIZED_CMD_LIST=" checkout show add filter-branch ls-files send-email 
> describe"
> +. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" 
>  
>  # We don't need this function to actually join words or do anything special.
>  # Also, it's cleaner to avoid touching bash's internal completion variables.
--
To unsubscribe from this list: send the line "unsubscribe 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] mergetool: clean up temp files when aborted

[Junio C Hamano]

Phil Hord  writes:

> On Thu, Jan 24, 2013 at 2:54 PM, Junio C Hamano  wrote:
>> Phil Hord  writes:
>> ...
>>> These temporary files should not remain after the mergetool operation is
>>> completed.
>>
>> Aren't there cases where people "abort" so that they can have a
>> chance inspect them outside mergetool program?  If there are no such
>> cases, then I would agree with your claim "should not remain", but
>> the proposed log message does not explay why it is sure that there
>> are no such use cases.
>
> You may be right about other people's workflows which I forgot to
> consider.  I have noticed a lot of inconsistency in this tool wrt to
> mergetool.keepBackup and mergetool.keepTemporaries.  Perhaps this
> change should hinge on the latter.
>
> Would you agree with this rephrased statement (accompanied by matching logic)?
>
> When mergetool.keepTemporaries is false or not set, these temporary files
> should not remain when the mergetool operation is aborted.

That is perfectly sensible (and should apply equally to non-"abort"
cases, I think).

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] mergetool: clean up temp files when aborted

[Phil Hord]

On Thu, Jan 24, 2013 at 2:54 PM, Junio C Hamano  wrote:
> Phil Hord  writes:
>
>> When handling a symlink conflict or a deleted-file conflict, mergetool
>> stops to ask the user what to do. If the user chooses any option besides
>> "(a)bort", then the temporary files which mergetool created in
>> preparation for handling the conflict are removed.  But these temporary
>> files are not removed when the user chooses to abort the operation.
>>
>> $ git cherry-pick other/branch
>> error: could not apply 4e43581... Fix foo.c
>>
>> $ git status --short
>> DU foo.c
>>
>> $ git mergetool
>> Merging:
>> foo.c
>>
>> Deleted merge conflict for 'foo.c':
>>   {local}: deleted
>>   {remote}: modified file
>> Use (m)odified or (d)eleted file, or (a)bort? a
>> Continue merging other unresolved paths (y/n) ? n
>>
>> $ git status --short
>> DU foo.c
>> ?? foo.c.BACKUP.16929.c
>> ?? foo.c.BASE.16929.c
>> ?? foo.c.LOCAL.16929.c
>> ?? foo.c.REMOTE.16929.c
>>
>> These temporary files should not remain after the mergetool operation is
>> completed.
>
> Aren't there cases where people "abort" so that they can have a
> chance inspect them outside mergetool program?  If there are no such
> cases, then I would agree with your claim "should not remain", but
> the proposed log message does not explay why it is sure that there
> are no such use cases.

You may be right about other people's workflows which I forgot to
consider.  I have noticed a lot of inconsistency in this tool wrt to
mergetool.keepBackup and mergetool.keepTemporaries.  Perhaps this
change should hinge on the latter.

Would you agree with this rephrased statement (accompanied by matching logic)?

When mergetool.keepTemporaries is false or not set, these temporary files
should not remain when the mergetool operation is aborted.


Here is the wording from git help config:

  mergetool.keepBackup
   After performing a merge, the original file with conflict
markers can be saved as a file with a .orig extension. If this
variable is set to false then this file
   is not preserved. Defaults to true (i.e. keep the backup files).

   mergetool.keepTemporaries
   When invoking a custom merge tool, git uses a set of
temporary files to pass to the tool. If the tool returns an error and
this variable is set to true, then
   these temporary files will be preserved, otherwise they
will be removed after the tool has exited. Defaults to false.


I have another commit in the works to clean up consistency around the
keepBackup.  I'll re-roll this one at the same time.

Phil
--
To unsubscribe from this list: send the line "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] t9902: Instruct git-completion.bash about a test mode

[Jean-Noël AVILA]

In test mode, git completion should propose commands only if they
belong to the list of authorized commands.

Signed-off-by: Jean-Noel Avila 
---

Better show some code than try to explain. Here is what I mean by
"filter the output git help -a". 

 contrib/completion/git-completion.bash | 16 +++-
 t/t9902-completion.sh  |  4 ++--
 2 files changed, 17 insertions(+), 3 deletions(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 14dd5e7..6490553 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -531,6 +531,20 @@ __git_complete_strategy ()
return 1
 }
 
+if test -z "$AUTHORIZED_CMD_LIST"; then
+   __git_cmdlist ()
+   { 
+   echo $1;
+   }
+else
+   __git_cmdlist ()
+   { 
+   if [[ " $AUTHORIZED_CMD_LIST " =~ " $1 " ]] ; then
+   echo $1
+   fi
+   }
+fi
+
 __git_list_all_commands ()
 {
local i IFS=" "$'\n'
@@ -538,7 +552,7 @@ __git_list_all_commands ()
do
case $i in
*--*) : helper pattern;;
-   *) echo $i;;
+   *) __git_cmdlist $i;;
esac
done
 }
diff --git a/t/t9902-completion.sh b/t/t9902-completion.sh
index 3cd53f8..5e7d81e 100755
--- a/t/t9902-completion.sh
+++ b/t/t9902-completion.sh
@@ -12,8 +12,8 @@ complete ()
# do nothing
return 0
 }
-
-. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash"
+AUTHORIZED_CMD_LIST=" checkout show add filter-branch ls-files send-email 
describe"
+. "$GIT_BUILD_DIR/contrib/completion/git-completion.bash" 
 
 # We don't need this function to actually join words or do anything special.
 # Also, it's cleaner to avoid touching bash's internal completion variables.
-- 
1.8.1.1.271.g02f55e6

--
To unsubscribe from this list: send the line "unsubscribe 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] parse_object: clear "parsed" when freeing buffers

[Jonathon Mah]

On 2013-01-23, at 23:07, Jeff King  wrote:

> On Wed, Jan 23, 2013 at 01:25:04PM -0800, Jonathon Mah wrote:
> 
>> Several areas of code would free buffers for object structs that
>> contained them ("struct tree" and "struct commit"), but without clearing
>> the "parsed" flag. parse_object would clear the flag for "struct tree",
>> but commits would remain in an invalid state (marked as parsed but with
>> a NULL buffer). Because the objects are uniqued (ccdc6037fee), the
>> invalid objects stay around and can lead to bad behavior.
>> 
>> In particular, running pickaxe on all refs in a repository with a cached
>> textconv could segfault. If the textconv cache ref was evaluated first
>> by cmd_log_walk, a subsequent notes_cache_match_validity call would
>> dereference NULL.
> 
> Just to be sure I understand, what is going on is something like this?
> 
>  Log reads all refs, including notes. After showing the notes commit,
>  log frees the buffer to save memory.  Later, doing the diff on a
>  commit causes us to use the notes_cache mechanism. That will look at
>  the commit at the tip of the notes ref, which log has already
>  processed. It will call parse_commit, but that will do nothing, as the
>  parsed flag is already set, but the commit buffer remains NULL.
> 
> I wonder if this could be the source of the segfault here, too:
> 
>  http://thread.gmane.org/gmane.comp.version-control.git/214322/focus=214355

Indeed, that description matches what I see. The other segfault seems to be in 
the same place too.

The segfault hits with the following stack (optimization off):

0   git-log parse_commit_header + 39 (pretty.c:738)
1   git-log format_commit_one + 3102 (pretty.c:1138)
2   git-log format_commit_item + 177 (pretty.c:1203)
3   git-log strbuf_expand + 172 (strbuf.c:247)
4   git-log format_commit_message + 196 (pretty.c:1263)
5   git-log notes_cache_match_validity + 215 (notes-cache.c:22)
6   git-log notes_cache_init + 212 (notes-cache.c:41)
7   git-log userdiff_get_textconv + 176 (userdiff.c:301)
8   git-log get_textconv + 66 (diff.c:2233)
9   git-log has_changes + 53 (diffcore-pickaxe.c:205)
10  git-log pickaxe + 299 (diffcore-pickaxe.c:40)
11  git-log diffcore_pickaxe_count + 344 (diffcore-pickaxe.c:275)
12  git-log diffcore_pickaxe + 58 (diffcore-pickaxe.c:289)
13  git-log diffcore_std + 162 (diff.c:4673)
14  git-log log_tree_diff_flush + 43 (log-tree.c:721)
15  git-log log_tree_diff + 566 (log-tree.c:826)
16  git-log log_tree_commit + 63 (log-tree.c:847)
17  git-log cmd_log_walk + 172 (log.c:301)
18  git-log cmd_log + 186 (log.c:568)
19  git-log run_builtin + 475 (git.c:273)
20  git-log handle_internal_command + 199 (git.c:434)
21  git-log main + 149 (git.c:523)
22  libdyld.dylib   start + 1

commit->message was freed and nulled earlier in a call to cmd_log_walk. This 
test reproduces it reliably on my machine:

diff --git a/t/t4042-diff-textconv-caching.sh b/t/t4042-diff-textconv-caching.sh
index 91f8198..d7e26ca 100755
--- a/t/t4042-diff-textconv-caching.sh
+++ b/t/t4042-diff-textconv-caching.sh
@@ -106,4 +106,15 @@ test_expect_success 'switching diff driver produces 
correct results' '
test_cmp expect actual
 '
 
+cat >expect 

Re: [PATCH 0/4] Fix "git difftool --tool-help"

[Junio C Hamano]

Nice code reduction ;-)

Thanks, will queue.

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

Re: [PATCH v4 3/4] pretty: Add failing tests: user format ignores i18n.logOutputEncoding setting

[Junio C Hamano]

Alexey Shumkin  writes:

> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index c248509..4db43a4 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> ...
> @@ -112,12 +133,12 @@ commit $head2
>  commit $head1
>  EOF
>  
> -test_format raw-body %B <<'EOF'
> -commit 131a310eb913d107dd3c09a65d1651175898735d
> -changed foo
> +test_format failure raw-body %B < +commit $head2
> +$changed
>  
> -commit 86c75cfd708a0e5868dc876ed5b8bb66c80b4873
> -added foo
> +commit $head1
> +$added
>  
>  EOF

It may have been easier to follow if you did this "Don't hardcode"
as a separate preparatory patch, like your first two patches.

> @@ -135,16 +156,16 @@ commit $head1
>  foo
>  EOF
>  
> -cat >commit-msg <<'EOF'
> +iconv -f utf-8 -t cp1251 > commit-msg <  Test printing of complex bodies
>  
>  This commit message is much longer than the others,
> -and it will be encoded in iso8859-1. We should therefore
> -include an iso8859 character: ¡bueno!
> +and it will be encoded in cp1251. We should therefore
> +include an cp1251 character: так вот!
>  EOF
>  
>  test_expect_success 'setup complex body' '
> - git config i18n.commitencoding iso8859-1 &&
> + git config i18n.commitencoding cp1251 &&

What is going on here?

Is this an example that shows that i18n.commitencoding works
correctly with iso8859-1 but not with cp1251?  

> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index cf492f4..699c824 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> ...
> @@ -192,7 +214,7 @@ test_expect_success \
>   'changing files and redo the last commit should succeed' '
>   echo "3rd line 2nd file" >>secondfile &&
>   git commit -a -C ORIG_HEAD &&
> - check_changes 3d3b7be011a58ca0c179ae45d94e6c83c0b0cd0d &&
> + check_changes f06f78b8dd468c722952b77569dd0db212442c25 &&
>   test "$(git rev-parse ORIG_HEAD)" = \
>   $head5
>  '

This and remaining hunks to this script shows that it would be
helped by the same love you gave to other scripts with your first
two patches before you add the "non-unicode" tests, no?

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

Re: [PATCH] git-cvsimport.txt: cvsps-2 is deprecated

[Junio C Hamano]

John Keeping  writes:

> On Thu, Jan 24, 2013 at 12:04:11PM -0800, Junio C Hamano wrote:
>> John Keeping  writes:
>> 
>> > git-cvsimport relies on version 2 of cvsps and does not work with the
>> > new version 3.  Since cvsps 3.x does not currently work as well as
>> > version 2 for incremental import, document this fact.
>> >
>> > Specifically, there is no way to make new git-cvsimport that supports
>> > cvsps 3.x and have a seamless transition for existing users since cvsps
>> > 3.x needs a time from which to continue importing and git-cvsimport does
>> > not save the time of the last import or import into a specific namespace
>> > so there is no safe way to calculate the time of the last import.
>> 
>> Isn't the whole "and git-cvsimport does not save the time..."  part
>> something that can be fixed in the new cvsimport that reads the
>> output from cvsps3?
>
> Yes it can be fixed there (and I have patches to do that) - my argument
> here is that there cannot be a seamless upgrade for people who are
> currently using git-cvsimport incrementally.  If you don't have that
> file then how do you create it to reflect the current state of your
> repository?

I am not disagreeing with your patch text to warn the current users
that cvsimport will be made unusable if they lose cvsps2, and they
are better off using other tools when doing a whole-history import.

I was trying to make sure that my understanding of the current
situation and future possibilities matches yours.

Thanks, I think you clarified the situation better with your
response.


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

Re: [PATCH v4 3/4] pretty: Add failing tests: user format ignores i18n.logOutputEncoding setting

[Junio C Hamano]

Alexey Shumkin  writes:

> The following two commands are expected to give the same output to a terminal:
>
>   $ git log --oneline --no-color
>   $ git log --pretty=format:'%h %s'
>
> However, the former pays attention to i18n.logOutputEncoding
> configuration, while the latter does not when it format "%s".
> Log messages written in an encoding i18n.commitEncoding which differs
> from terminal encoding are shown corrupted with the latter even
> when i18n.logOutputEncoding and terminal encoding are the same.
>
> The same corruption is true for
>   $ git diff --submodule=log
> and
>   $ git rev-list --pretty=format:%s HEAD
> and
>   $ git reset --hard
>
> Signed-off-by: Alexey Shumkin 
> ---
>  t/t4041-diff-submodule-option.sh | 33 ---
>  t/t4205-log-pretty-formats.sh| 43 +++
>  t/t6006-rev-list-format.sh   | 90 
> ++--
>  t/t7102-reset.sh | 32 +++---
>  4 files changed, 138 insertions(+), 60 deletions(-)
>
> diff --git a/t/t4041-diff-submodule-option.sh 
> b/t/t4041-diff-submodule-option.sh
> index 32d4a60..e7d6363 100755
> --- a/t/t4041-diff-submodule-option.sh
> +++ b/t/t4041-diff-submodule-option.sh
> @@ -1,6 +1,7 @@
>  #!/bin/sh
>  #
>  # Copyright (c) 2009 Jens Lehmann, based on t7401 by Ping Yin
> +# Copyright (c) 2011 Alexey Shumkin (+ non-UTF-8 commit encoding tests)
>  #
>  
>  test_description='Support for verbose submodule differences in git diff
> @@ -10,6 +11,7 @@ This test tries to verify the sanity of the --submodule 
> option of git diff.
>  
>  . ./test-lib.sh
>  
> +added=$(printf 
> "\320\264\320\276\320\261\320\260\320\262\320\273\320\265\320\275")

Please have an in-code comment before this line to explain what this
variable is about, e.g.

# String "added" in Russian, encoded in UTF-8, used in
# sample commit log messages in add_file() function below.
added=$(printf "...")

>  add_file () {
>   (
>   cd "$1" &&
> @@ -19,7 +21,8 @@ add_file () {
>   echo "$name" >"$name" &&
>   git add "$name" &&
>   test_tick &&
> - git commit -m "Add $name" || exit
> + msg_added_cp1251=$(echo "Add $name ($added $name)" | 
> iconv -f utf-8 -t cp1251) &&
> + git -c 'i18n.commitEncoding=cp1251' commit -m 
> "$msg_added_cp1251"
>   done >/dev/null &&
>   git rev-parse --short --verify HEAD
>   )

Does this patch make the all tests in this script fail for people
without cp1251 locale installed?  We already have tests that depend
on 8859-1 and some other locales, and we'd be better off limiting
such dependency to the minimum.

Same comment applies to the changes to other test scripts.

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 v4 2/4] t7102 (reset): refactoring: don't hardcode SHA-1 in expected outputs

[Junio C Hamano]

Alexey Shumkin  writes:

> The expected SHA-1 digests are always available in variables.  Use
> them instead of hardcoding.
>
> Signed-off-by: Alexey Shumkin 
> ---

Looks good (" refactoring:" in the title may not want to be there,
though).

Thanks.

>  t/t7102-reset.sh | 41 +
>  1 file changed, 21 insertions(+), 20 deletions(-)
>
> diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
> index b096dc8..cf492f4 100755
> --- a/t/t7102-reset.sh
> +++ b/t/t7102-reset.sh
> @@ -28,7 +28,8 @@ test_expect_success 'creating initial files and commits' '
>  
>   echo "1st line 2nd file" >secondfile &&
>   echo "2nd line 2nd file" >>secondfile &&
> - git commit -a -m "modify 2nd file"
> + git commit -a -m "modify 2nd file" &&
> + head5=$(git rev-parse --verify HEAD)
>  '
>  # git log --pretty=oneline # to see those SHA1 involved
>  
> @@ -56,7 +57,7 @@ test_expect_success 'giving a non existing revision should 
> fail' '
>   test_must_fail git reset --mixed aa &&
>   test_must_fail git reset --soft aa &&
>   test_must_fail git reset --hard aa &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
>  '
>  
>  test_expect_success 'reset --soft with unmerged index should fail' '
> @@ -74,7 +75,7 @@ test_expect_success \
>   test_must_fail git reset --hard -- first &&
>   test_must_fail git reset --soft HEAD^ -- first &&
>   test_must_fail git reset --hard HEAD^ -- first &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
>  '
>  
>  test_expect_success 'giving unrecognized options should fail' '
> @@ -86,7 +87,7 @@ test_expect_success 'giving unrecognized options should 
> fail' '
>   test_must_fail git reset --soft -o &&
>   test_must_fail git reset --hard --other &&
>   test_must_fail git reset --hard -o &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
>  '
>  
>  test_expect_success \
> @@ -110,7 +111,7 @@ test_expect_success \
>  
>   git checkout master &&
>   git branch -D branch1 branch2 &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
>  '
>  
>  test_expect_success \
> @@ -133,27 +134,27 @@ test_expect_success \
>  
>   git checkout master &&
>   git branch -D branch3 branch4 &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
>  '
>  
>  test_expect_success \
>   'resetting to HEAD with no changes should succeed and do nothing' '
>   git reset --hard &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>   git reset --hard HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>   git reset --soft &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>   git reset --soft HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>   git reset --mixed &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>   git reset --mixed HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>   git reset &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
> + check_changes $head5 &&
>   git reset HEAD &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
>  '
>  
>  >.diff_expect
> @@ -176,7 +177,7 @@ test_expect_success '--soft reset only should show 
> changes in diff --cached' '
>   git reset --soft HEAD^ &&
>   check_changes d1a4bc3abce4829628ae2dcb0d60ef3d1a78b1c4 &&
>   test "$(git rev-parse ORIG_HEAD)" = \
> - 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + $head5
>  '
>  
>  >.diff_expect
> @@ -193,7 +194,7 @@ test_expect_success \
>   git commit -a -C ORIG_HEAD &&
>   check_changes 3d3b7be011a58ca0c179ae45d94e6c83c0b0cd0d &&
>   test "$(git rev-parse ORIG_HEAD)" = \
> - 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + $head5
>  '
>  
>  >.diff_expect
> @@ -303,7 +304,7 @@ test_expect_success 'redoing the last two commits should 
> succeed' '
>   echo "1st line 2nd file" >secondfile &&
>   echo "2nd line 2nd file" >>secondfile &&
>   git commit -a -m "modify 2nd file" &&
> - check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
> + check_changes $head5
>  '
>  
>  >.diff_expect
> @@ -341,15 +342,15 @@ EOF
>  test_expect_success \
>   '--hard reset to ORIG_HEAD should clear a fast-forward merge' '
>   git reset --hard HEAD^ &&
> - check_changes 3ec39

Re: [PATCH v4 1/4] t6006 (rev-list-format): don't hardcode SHA-1 in expected outputs

[Junio C Hamano]

Alexey Shumkin  writes:

> The expected SHA-1 digests are always available in variables.  Use
> them instead of hardcoding.
>
> Signed-off-by: Alexey Shumkin 
> ---
>  t/t6006-rev-list-format.sh | 130 
> +
>  1 file changed, 72 insertions(+), 58 deletions(-)
>
> diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
> index f94f0c4..c248509 100755
> --- a/t/t6006-rev-list-format.sh
> +++ b/t/t6006-rev-list-format.sh
> @@ -6,8 +6,19 @@ test_description='git rev-list --pretty=format test'
>  
>  test_tick
>  test_expect_success 'setup' '
> -touch foo && git add foo && git commit -m "added foo" &&
> -  echo changed >foo && git commit -a -m "changed foo"
> + touch foo &&

This is inherited from the original, but these days we try to avoid
touch, unless it is about setting a new file timestamp.  The
canonical (in the script we write) way to create an empty file is:

: >foo

with or without ": ", it does not matter that much.

> + git add foo &&
> + git commit -m "added foo" &&
> + head1=$(git rev-parse --verify HEAD) &&
> + head1_7=$(echo $head1 | cut -c1-7) &&

Why do we want "whatever_7" variables and use "cut -c1-7" to produce
them?  Is "7" something we care deeply about?

I think what we care a lot more than "7" that happens to be the
current default value is to make sure that, if we ever update the
default abbreviation length to a larger value, the abbreviation
shown with --format=%h is consistent with the abbreviation that is
given by rev-parse --short.

head1_short=$(git rev-parse --short $head1)

perhaps?

> + echo changed >foo &&
> + git commit -a -m "changed foo" &&
> + head2=$(git rev-parse --verify HEAD) &&
> + head2_7=$(echo $head2 | cut -c1-7) &&
> + head2_parent=$(git cat-file -p HEAD | grep parent | cut -f 2 -d" ") &&

Do not use "cat-file -p" that is for human consumption in scripts,
unless you are testing how the format for human consumption should
look like (we may later add more pretty-print to them), which is not
the case here.

Also be careful and pay attention to the end of the header; you do
not want to pick up a random "parent" string in the middle of a log
message.

head2_parent=$(git cat-file commit HEAD | sed -n -e "s/^parent //p" -e 
"/^$/q")

would be much better.

> + head2_parent_7=$(echo $head2_parent | cut -c1-7) &&
> + tree2=$(git cat-file -p HEAD | grep tree | cut -f 2 -d" ") &&

Likewise.

> + tree2_7=$(echo $tree2 | cut -c1-7)

Likewise.

> @@ -131,39 +142,42 @@ This commit message is much longer than the others,
>  and it will be encoded in iso8859-1. We should therefore
>  include an iso8859 character: ¡bueno!
>  EOF
> +
>  test_expect_success 'setup complex body' '
> -git config i18n.commitencoding iso8859-1 &&
> -  echo change2 >foo && git commit -a -F commit-msg
> + git config i18n.commitencoding iso8859-1 &&
> + echo change2 >foo && git commit -a -F commit-msg &&
> + head3=$(git rev-parse --verify HEAD) &&
> + head3_7=$(echo $head3 | cut -c1-7)
>  '

Likewise.

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] git-cvsimport.txt: cvsps-2 is deprecated

[John Keeping]

On Thu, Jan 24, 2013 at 12:04:11PM -0800, Junio C Hamano wrote:
> John Keeping  writes:
> 
> > git-cvsimport relies on version 2 of cvsps and does not work with the
> > new version 3.  Since cvsps 3.x does not currently work as well as
> > version 2 for incremental import, document this fact.
> >
> > Specifically, there is no way to make new git-cvsimport that supports
> > cvsps 3.x and have a seamless transition for existing users since cvsps
> > 3.x needs a time from which to continue importing and git-cvsimport does
> > not save the time of the last import or import into a specific namespace
> > so there is no safe way to calculate the time of the last import.
> 
> Isn't the whole "and git-cvsimport does not save the time..."  part
> something that can be fixed in the new cvsimport that reads the
> output from cvsps3?

Yes it can be fixed there (and I have patches to do that) - my argument
here is that there cannot be a seamless upgrade for people who are
currently using git-cvsimport incrementally.  If you don't have that
file then how do you create it to reflect the current state of your
repository?

> To me, it sounds more like
> 
> cvsps2 + cvsimport has an unfixable bugs and there have been an
> effort to rewrite cvsps2 from scratch.  The resulting cvsp3
> currently is unusable with cvsimport, especially when importing
> the history incrementally, and it isn't expected that it will
> ever be usable with cvsimport again.

cvsps3 isn't a re-write, it's cvsps2 with a lot of things ripped out and
a fast-export mode added.  And in fast-export mode it cannot inspect the
Git repository in the same way that git-cvsimport does.

> There are other tools that analyse the original history better
> and emits more correct output when used to convert the whole
> history, and hopefully cvsps3 + fast-import would become one of
> them.  Suggest users to use them instead of cvsimport when they
> are not doing an incremental import.

Yes.  The consensus seems to be that cvs2git is the most correct.

> By the way, do we want to make any recommendation to the distro
> folks which cvsps they should ship?  It appears that not shipping
> cvsps2 would be a major regression if cvsps3 does not plan to
> support incrementals, so shipping both might be the safest way for
> them to support their users with different needs.

I agree.  cvsps is only one binary and a man page so I don't think it
would be too hard to ship both.


John
--
To unsubscribe from this list: send the line "unsubscribe 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-cvsimport.txt: cvsps-2 is deprecated

[Junio C Hamano]

John Keeping  writes:

> git-cvsimport relies on version 2 of cvsps and does not work with the
> new version 3.  Since cvsps 3.x does not currently work as well as
> version 2 for incremental import, document this fact.
>
> Specifically, there is no way to make new git-cvsimport that supports
> cvsps 3.x and have a seamless transition for existing users since cvsps
> 3.x needs a time from which to continue importing and git-cvsimport does
> not save the time of the last import or import into a specific namespace
> so there is no safe way to calculate the time of the last import.

Isn't the whole "and git-cvsimport does not save the time..."  part
something that can be fixed in the new cvsimport that reads the
output from cvsps3?

To me, it sounds more like

cvsps2 + cvsimport has an unfixable bugs and there have been an
effort to rewrite cvsps2 from scratch.  The resulting cvsp3
currently is unusable with cvsimport, especially when importing
the history incrementally, and it isn't expected that it will
ever be usable with cvsimport again.

There are other tools that analyse the original history better
and emits more correct output when used to convert the whole
history, and hopefully cvsps3 + fast-import would become one of
them.  Suggest users to use them instead of cvsimport when they
are not doing an incremental import.

By the way, do we want to make any recommendation to the distro
folks which cvsps they should ship?  It appears that not shipping
cvsps2 would be a major regression if cvsps3 does not plan to
support incrementals, so shipping both might be the safest way for
them to support their users with different needs.

Thanks.  I think the patch text itself is good.

> Signed-off-by: John Keeping 
> ---
> On Wed, Jan 23, 2013 at 09:04:12PM -0800, Junio C Hamano wrote:
>> Care to roll a proper patch with a log message?  I'll discard the
>> topic for now and replace it with your documentation update.
>
> Here it is.
>
>  Documentation/git-cvsimport.txt | 6 ++
>  1 file changed, 6 insertions(+)
>
> diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
> index 9d5353e..f059ea9 100644
> --- a/Documentation/git-cvsimport.txt
> +++ b/Documentation/git-cvsimport.txt
> @@ -18,6 +18,12 @@ SYNOPSIS
>  
>  DESCRIPTION
>  ---
> +*WARNING:* `git cvsimport` uses cvsps version 2, which is considered
> +deprecated; it does not work with cvsps version 3 and later.  If you are
> +performing a one-shot import of a CVS repository consider using
> +link:http://cvs2svn.tigris.org/cvs2git.html[cvs2git] or
> +link:https://github.com/BartMassey/parsecvs[parsecvs].
> +
>  Imports a CVS repository into git. It will either create a new
>  repository, or incrementally import into an existing one.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

[PATCH 4/4] git-difftool: use git-mergetool--lib for "--tool-help"

[John Keeping]

The "--tool-help" option to git-difftool currently displays incorrect
output since it uses the names of the files in
"$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
git-mergetool--lib.

Fix this by simply delegating the "--tool-help" argument to the
show_tool_help function in git-mergetool--lib.

Signed-off-by: John Keeping 
---
 git-difftool.perl | 57 ---
 1 file changed, 8 insertions(+), 49 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index edd0493..0a90de4 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -59,57 +59,16 @@ sub find_worktree
return $worktree;
 }
 
-sub filter_tool_scripts
-{
-   my ($tools) = @_;
-   if (-d $_) {
-   if ($_ ne ".") {
-   # Ignore files in subdirectories
-   $File::Find::prune = 1;
-   }
-   } else {
-   if ((-f $_) && ($_ ne "defaults")) {
-   push(@$tools, $_);
-   }
-   }
-}
-
 sub print_tool_help
 {
-   my ($cmd, @found, @notfound, @tools);
-   my $gitpath = Git::exec_path();
-
-   find(sub { filter_tool_scripts(\@tools) }, "$gitpath/mergetools");
-
-   foreach my $tool (@tools) {
-   $cmd  = "TOOL_MODE=diff";
-   $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
-   $cmd .= " && get_merge_tool_path $tool >/dev/null 2>&1";
-   $cmd .= " && can_diff >/dev/null 2>&1";
-   if (system('sh', '-c', $cmd) == 0) {
-   push(@found, $tool);
-   } else {
-   push(@notfound, $tool);
-   }
-   }
-
-   print << 'EOF';
-'git difftool --tool=' may be set to one of the following:
-EOF
-   print "\t$_\n" for (sort(@found));
-
-   print << 'EOF';
-
-The following tools are valid, but not currently available:
-EOF
-   print "\t$_\n" for (sort(@notfound));
-
-   print << 'EOF';
-
-NOTE: Some of the tools listed above only work in a windowed
-environment. If run in a terminal-only session, they will fail.
-EOF
-   exit(0);
+   my $cmd = 'TOOL_MODE=diff';
+   $cmd .= ' && . "$(git --exec-path)/git-mergetool--lib"';
+   $cmd .= ' && show_tool_help';
+
+   # See the comment at the bottom of file_diff() for the reason behind
+   # using system() followed by exit() instead of exec().
+   my $rc = system('sh', '-c', $cmd);
+   exit($rc | ($rc >> 8));
 }
 
 sub exit_cleanup
-- 
1.8.1

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

[PATCH 3/4] git-mergetool: don't hardcode 'mergetool' in show_tool_help

[John Keeping]

When using show_tool_help from git-difftool we will want it to print
"git difftool" not "git mergetool" so use "git ${TOOL_MODE}tool".

Signed-off-by: John Keeping 
---
 git-mergetool--lib.sh | 6 --
 1 file changed, 4 insertions(+), 2 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 1748315..4c1e129 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -188,12 +188,14 @@ show_tool_help () {
unavailable="$unavailable$i$LF"
fi
done
+
+   cmd_name=${TOOL_MODE}tool
if test -n "$available"
then
-   echo "'git mergetool --tool=' may be set to one of the 
following:"
+   echo "'git $cmd_name --tool=' may be set to one of the 
following:"
echo "$available" | sort | sed -e 's/^/ /'
else
-   echo "No suitable tool for 'git mergetool --tool=' found."
+   echo "No suitable tool for 'git $cmd_name --tool=' found."
fi
if test -n "$unavailable"
then
-- 
1.8.1

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

[PATCH 2/4] git-mergetool: remove redundant assignment

[John Keeping]

TOOL_MODE is set at the top of git-mergetool.sh so there is no need to
set it again in show_tool_help.  Removing this lets us re-use
show_tool_help in git-difftool.

Signed-off-by: John Keeping 
---
 git-mergetool--lib.sh | 1 -
 1 file changed, 1 deletion(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index 89a857f..1748315 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -175,7 +175,6 @@ list_merge_tool_candidates () {
 }
 
 show_tool_help () {
-   TOOL_MODE=merge
list_merge_tool_candidates
unavailable= available= LF='
 '
-- 
1.8.1

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

[PATCH 1/4] git-mergetool: move show_tool_help to mergetool--lib

[John Keeping]

This is the first step in unifying "git difftool --tool-help" and
"git mergetool --tool-help".

Signed-off-by: John Keeping 
---
 git-mergetool--lib.sh | 37 +
 git-mergetool.sh  | 37 -
 2 files changed, 37 insertions(+), 37 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index f013a03..89a857f 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -174,6 +174,43 @@ list_merge_tool_candidates () {
esac
 }
 
+show_tool_help () {
+   TOOL_MODE=merge
+   list_merge_tool_candidates
+   unavailable= available= LF='
+'
+   for i in $tools
+   do
+   merge_tool_path=$(translate_merge_tool_path "$i")
+   if type "$merge_tool_path" >/dev/null 2>&1
+   then
+   available="$available$i$LF"
+   else
+   unavailable="$unavailable$i$LF"
+   fi
+   done
+   if test -n "$available"
+   then
+   echo "'git mergetool --tool=' may be set to one of the 
following:"
+   echo "$available" | sort | sed -e 's/^/ /'
+   else
+   echo "No suitable tool for 'git mergetool --tool=' found."
+   fi
+   if test -n "$unavailable"
+   then
+   echo
+   echo 'The following tools are valid, but not currently 
available:'
+   echo "$unavailable" | sort | sed -e 's/^/   /'
+   fi
+   if test -n "$unavailable$available"
+   then
+   echo
+   echo "Some of the tools listed above only work in a windowed"
+   echo "environment. If run in a terminal-only session, they will 
fail."
+   fi
+   exit 0
+}
+
 guess_merge_tool () {
list_merge_tool_candidates
echo >&2 "merge tool candidates: $tools"
diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..c0ee9aa 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -315,43 +315,6 @@ merge_file () {
return 0
 }
 
-show_tool_help () {
-   TOOL_MODE=merge
-   list_merge_tool_candidates
-   unavailable= available= LF='
-'
-   for i in $tools
-   do
-   merge_tool_path=$(translate_merge_tool_path "$i")
-   if type "$merge_tool_path" >/dev/null 2>&1
-   then
-   available="$available$i$LF"
-   else
-   unavailable="$unavailable$i$LF"
-   fi
-   done
-   if test -n "$available"
-   then
-   echo "'git mergetool --tool=' may be set to one of the 
following:"
-   echo "$available" | sort | sed -e 's/^/ /'
-   else
-   echo "No suitable tool for 'git mergetool --tool=' found."
-   fi
-   if test -n "$unavailable"
-   then
-   echo
-   echo 'The following tools are valid, but not currently 
available:'
-   echo "$unavailable" | sort | sed -e 's/^/   /'
-   fi
-   if test -n "$unavailable$available"
-   then
-   echo
-   echo "Some of the tools listed above only work in a windowed"
-   echo "environment. If run in a terminal-only session, they will 
fail."
-   fi
-   exit 0
-}
-
 prompt=$(git config --bool mergetool.prompt || echo true)
 
 while test $# != 0
-- 
1.8.1

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

[PATCH 0/4] Fix "git difftool --tool-help"

[John Keeping]

The "--tool-help" option to git-difftool currently displays incorrect
output since it uses the names of the files in
"$GIT_EXEC_PATH/mergetools/" rather than the list of command names in
git-mergetool--lib.

This series fixes this by changing it to simply delegate to a function
in git-mergetool--lib.

The first three patches are just refactorings to move the show_tool_help
function from git-mergetool to git-mergetool--lib and make it usable by
git-difftool.  The final patch switches git-difftool to use this
function.


John Keeping (4):
  git-mergetool: move show_tool_help to mergetool--lib
  git-mergetool: remove redundant assignment
  git-mergetool: don't hardcode 'mergetool' in show_tool_help
  git-difftool: use git-mergetool--lib for "--tool-help"

 git-difftool.perl | 57 ---
 git-mergetool--lib.sh | 38 ++
 git-mergetool.sh  | 37 -
 3 files changed, 46 insertions(+), 86 deletions(-)

-- 
1.8.1

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

Re: [PATCH] mergetool: clean up temp files when aborted

[Junio C Hamano]

Phil Hord  writes:

> When handling a symlink conflict or a deleted-file conflict, mergetool
> stops to ask the user what to do. If the user chooses any option besides
> "(a)bort", then the temporary files which mergetool created in
> preparation for handling the conflict are removed.  But these temporary
> files are not removed when the user chooses to abort the operation.
>
> $ git cherry-pick other/branch
> error: could not apply 4e43581... Fix foo.c
>
> $ git status --short
> DU foo.c
>
> $ git mergetool
> Merging:
> foo.c
>
> Deleted merge conflict for 'foo.c':
>   {local}: deleted
>   {remote}: modified file
> Use (m)odified or (d)eleted file, or (a)bort? a
> Continue merging other unresolved paths (y/n) ? n
>
> $ git status --short
> DU foo.c
> ?? foo.c.BACKUP.16929.c
> ?? foo.c.BASE.16929.c
> ?? foo.c.LOCAL.16929.c
> ?? foo.c.REMOTE.16929.c
>
> These temporary files should not remain after the mergetool operation is
> completed.

Aren't there cases where people "abort" so that they can have a
chance inspect them outside mergetool program?  If there are no such
cases, then I would agree with your claim "should not remain", but
the proposed log message does not explay why it is sure that there
are no such use cases.

>
> Remove the temporary files by calling the cleanup_temp_files when the
> user chooses to abort the mergetool operation.
>
> It looks like 'cleanup_temp_files' without the --save-backups option is
> the correct thing to do, and this is how this commit is implemented. But
> some other paths do use --save-backups resulting in a foo.c.orig file
> being left behind.  That seems to be a different bug, though.
>
> Signed-off-by: Phil Hord 
> ---
>  git-mergetool.sh | 2 ++
>  1 file changed, 2 insertions(+)
>
> diff --git a/git-mergetool.sh b/git-mergetool.sh
> index c50e18a..bb93b70 100755
> --- a/git-mergetool.sh
> +++ b/git-mergetool.sh
> @@ -90,6 +90,7 @@ resolve_symlink_merge () {
>   return 0
>   ;;
>   [aA]*)
> + cleanup_temp_files
>   return 1
>   ;;
>   esac
> @@ -118,6 +119,7 @@ resolve_deleted_merge () {
>   return 0
>   ;;
>   [aA]*)
> + cleanup_temp_files
>   return 1
>   ;;
>   esac
--
To unsubscribe from this list: send the line "unsubscribe 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] mergetools: Add tortoisegitmerge helper

[Junio C Hamano]

Sven Strickroth  writes:

> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>   (starting with 1.8.0) in order to make clear that this one has special
>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>   version.

Wouldn't it make more sense in such a situation if your users can
keep using the old "tortoisemerge" configured in their configuration
and when the renamed one is found the mergetool automatically used
it, rather than the way your patch is done?  It seems that you are
forcing all the users to reconfigure or retrain their fingers.  Is
that the best we can do?  Is it too cumbersome to autodetect the
presense of tortoisegitmerge and redirect a request for tortoisemerge
to it, perhaps using translate_merge_tool_path (cf. mergetools/bc3)?

Assuming that people that have both variants will always want
mergetool to use tortoisegitmerge, that is.  If there are some
features missing from or extra bugs in tortoisegitmerge that makes
some people favor tortoisemerge, then giving two choices like your
patch does may make more sense.  I only know the difference between
the two from your four-line description above, but it does not look
like it is the case.

> diff --git a/Documentation/diff-config.txt b/Documentation/diff-config.txt
> index 4314ad0..13cbe5b 100644
> --- a/Documentation/diff-config.txt
> +++ b/Documentation/diff-config.txt
> @@ -151,7 +151,7 @@ diff..cachetextconv::
>  diff.tool::
>   The diff tool to be used by linkgit:git-difftool[1].  This
>   option overrides `merge.tool`, and has the same valid built-in
> - values as `merge.tool` minus "tortoisemerge" and plus
> - "kompare".  Any other value is treated as a custom diff tool,
> + values as `merge.tool` minus "tortoisemerge"/"tortoisegitmerge" and
> + plus "kompare".  Any other value is treated as a custom diff tool,
>   and there must be a corresponding `difftool..cmd`
>   option.

So in short, two tortoises and kompare are only valid as mergetool
but cannot be used as difftool?  No, I am reading it wrong.
merge.tool can be used for both, kompare can be used as difftool,
and two tortoises can only be used as mergetool.

This paragraph needs to be rewritten to unconfuse readers.  The
original is barely intelligible, and it becomes unreadable as the
set of tools subtracted by "minus" and added by "plus" grows.

--
To unsubscribe from this list: send the line "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-cvsimport.txt: cvsps-2 is deprecated

[John Keeping]

git-cvsimport relies on version 2 of cvsps and does not work with the
new version 3.  Since cvsps 3.x does not currently work as well as
version 2 for incremental import, document this fact.

Specifically, there is no way to make new git-cvsimport that supports
cvsps 3.x and have a seamless transition for existing users since cvsps
3.x needs a time from which to continue importing and git-cvsimport does
not save the time of the last import or import into a specific namespace
so there is no safe way to calculate the time of the last import.

Signed-off-by: John Keeping 
---
On Wed, Jan 23, 2013 at 09:04:12PM -0800, Junio C Hamano wrote:
> Care to roll a proper patch with a log message?  I'll discard the
> topic for now and replace it with your documentation update.

Here it is.

 Documentation/git-cvsimport.txt | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/git-cvsimport.txt b/Documentation/git-cvsimport.txt
index 9d5353e..f059ea9 100644
--- a/Documentation/git-cvsimport.txt
+++ b/Documentation/git-cvsimport.txt
@@ -18,6 +18,12 @@ SYNOPSIS
 
 DESCRIPTION
 ---
+*WARNING:* `git cvsimport` uses cvsps version 2, which is considered
+deprecated; it does not work with cvsps version 3 and later.  If you are
+performing a one-shot import of a CVS repository consider using
+link:http://cvs2svn.tigris.org/cvs2git.html[cvs2git] or
+link:https://github.com/BartMassey/parsecvs[parsecvs].
+
 Imports a CVS repository into git. It will either create a new
 repository, or incrementally import into an existing one.
 
-- 
1.8.1

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

Re: [PATCH 6/7] read-cache: refuse to create index referring to external objects

[Junio C Hamano]

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

> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  read-cache.c | 20 
>  1 file changed, 20 insertions(+)
>
> diff --git a/read-cache.c b/read-cache.c
> index fda78bc..4579215 100644
> --- a/read-cache.c
> +++ b/read-cache.c
> @@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, 
> struct cache_entry *ce,
> ce->name + common, ce_namelen(ce) - common);
>   }
>  
> + if (object_database_contaminated) {
> + struct object_info oi;
> + switch (ce->ce_mode) {
> + case S_IFGITLINK:
> + break;
> + case S_IFDIR:
> + if (sha1_object_info_extended(ce->sha1, &oi) != 
> OBJ_TREE ||

This case should never happen.  Do we store any tree object in the
index in the first place?

> + (oi.alt && oi.alt->external))
> + die("cannot create index referring to an 
> external tree %s",
> + sha1_to_hex(ce->sha1));
> + break;
> + default:
> + if (sha1_object_info_extended(ce->sha1, &oi) != 
> OBJ_BLOB ||
> + (oi.alt && oi.alt->external))
> + die("cannot create index referring to an 
> external blob %s",
> + sha1_to_hex(ce->sha1));
> + break;
> + }
> + }
> +
>   result = ce_write(c, fd, ondisk, size);
>   free(ondisk);
>   return result;

I think the check you want to add is to the cache-tree code; before
writing the new index out, if you suspect you might have called
cache_tree_update() before, invalidate any hierarchy that is
recorded to produce a tree object that refers to an object that is
only available through external object store.

As to the logic to check if a object is something that we should
refuse to create a new dependent, I think you should not butcher
sha1_object_info_extended(), and instead add a new call that checks
if a given SHA-1 yields an object if you ignore alternate object
databases that are marked as "external", whose signature may
resemble:

int has_sha1_file_proper(const unsigned char *sha1);

or something.

This is a tangent, but in addition, you may want to add an even
narrower variant that checks the same but ignoring _all_ alternate
object databases, "external" or not:

int has_sha1_file_local(const unsigned char *sha1);

That may be useful if we want to later make the alternate safer to
use; instead of letting the codepath to create an object ask
has_sha1_file() to see if it already exists and refrain from writing
a new copy, we switch to ask has_sha1_file_locally() and even if an
alternate object database we borrow from has it, we keep our own
copy in our repository.

Not tested beyond syntax, but rebasing something like this patch on
top of your "mark external sources" change may take us closer to
that.

 cache.h |  2 ++
 sha1_file.c | 60 +++-
 2 files changed, 57 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 1f96f65..8d651b6 100644
--- a/cache.h
+++ b/cache.h
@@ -766,6 +766,8 @@ extern int move_temp_to_file(const char *tmpfile, const 
char *filename);
 
 extern int has_sha1_pack(const unsigned char *sha1);
 extern int has_sha1_file(const unsigned char *sha1);
+extern int has_sha1_file_proper(const unsigned char *sha1);
+extern int has_sha1_file_local(const unsigned char *sha1);
 extern int has_loose_object_nonlocal(const unsigned char *sha1);
 
 extern int has_pack_index(const unsigned char *sha1);
diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..1a3192a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -420,11 +420,18 @@ static int has_loose_object_local(const unsigned char 
*sha1)
return !access(name, F_OK);
 }
 
-int has_loose_object_nonlocal(const unsigned char *sha1)
+enum odb_origin {
+   odb_default = 0, odb_proper, odb_local
+};
+
+static int has_loose_object_nonlocal_limited(const unsigned char *sha1,
+enum odb_origin origin)
 {
struct alternate_object_database *alt;
prepare_alt_odb();
for (alt = alt_odb_list; alt; alt = alt->next) {
+   if (origin == odb_proper && 0 /* alt->external */)
+   continue;
fill_sha1_path(alt->name, sha1);
if (!access(alt->base, F_OK))
return 1;
@@ -432,6 +439,11 @@ int has_loose_object_nonlocal(const unsigned char *sha1)
return 0;
 }
 
+int has_loose_object_nonlocal(const unsigned char *sha1)
+{
+   return has_loose_object_nonlocal_limited(sha1, odb_default);
+}
+
 static int has_loose_object(const unsigned char *sha1)
 {
return has_loose_object_local(sha1) ||
@@ -2062,12 +2074,28 @@ int is_pack_valid(struct packed_git 

Re: [PATCH] send-email: Honor multi-part email messages

[Junio C Hamano]

Shumkin Alexey  writes:

> I've meant you did not reply to my last message in this thread
> http://thread.gmane.org/gmane.comp.version-control.git/181791

Please repost the patch (with possible updates) for review when
attempting to resurrect an ancient topic, instead of forcing people
to hunt for an ancient message.

I think your patch is wrong.  What happens when we see a Subject:
line with a non-ascii on it that causes an early return of the loop
before your new code has a chance to see Content-Type: header?

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

Re: [PATCH] send-email: Honor multi-part email messages

[Shumkin Alexey]

I've meant you did not reply to my last message in this thread
http://thread.gmane.org/gmane.comp.version-control.git/181791

2013/1/24 Junio C Hamano 
>
> Alexey Shumkin  writes:
>
> > Bump
>
> Bump what???
--
To unsubscribe from this list: send the line "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] mergetool: clean up temp files when aborted

[Phil Hord]

When handling a symlink conflict or a deleted-file conflict, mergetool
stops to ask the user what to do. If the user chooses any option besides
"(a)bort", then the temporary files which mergetool created in
preparation for handling the conflict are removed.  But these temporary
files are not removed when the user chooses to abort the operation.

$ git cherry-pick other/branch
error: could not apply 4e43581... Fix foo.c

$ git status --short
DU foo.c

$ git mergetool
Merging:
foo.c

Deleted merge conflict for 'foo.c':
  {local}: deleted
  {remote}: modified file
Use (m)odified or (d)eleted file, or (a)bort? a
Continue merging other unresolved paths (y/n) ? n

$ git status --short
DU foo.c
?? foo.c.BACKUP.16929.c
?? foo.c.BASE.16929.c
?? foo.c.LOCAL.16929.c
?? foo.c.REMOTE.16929.c

These temporary files should not remain after the mergetool operation is
completed.

Remove the temporary files by calling the cleanup_temp_files when the
user chooses to abort the mergetool operation.

It looks like 'cleanup_temp_files' without the --save-backups option is
the correct thing to do, and this is how this commit is implemented. But
some other paths do use --save-backups resulting in a foo.c.orig file
being left behind.  That seems to be a different bug, though.

Signed-off-by: Phil Hord 
---
 git-mergetool.sh | 2 ++
 1 file changed, 2 insertions(+)

diff --git a/git-mergetool.sh b/git-mergetool.sh
index c50e18a..bb93b70 100755
--- a/git-mergetool.sh
+++ b/git-mergetool.sh
@@ -90,6 +90,7 @@ resolve_symlink_merge () {
return 0
;;
[aA]*)
+   cleanup_temp_files
return 1
;;
esac
@@ -118,6 +119,7 @@ resolve_deleted_merge () {
return 0
;;
[aA]*)
+   cleanup_temp_files
return 1
;;
esac
-- 
1.8.1.1.ga649ac9.dirty

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

Re: [PATCH v3 01/10] wildmatch: fix "**" special case

[Junio C Hamano]

Duy Nguyen  writes:

> I don't think we need to support two different sets of wildcards in
> the long run. I'm thinking of adding ":(glob)" with WM_PATHNAME.
> Pathspec without :(glob) still uses the current wildcards (i.e. no
> FNM_PATHNAME). At some point, like 2.0, we either switch the behavior
> of patterns-without-:(glob) to WM_PATHNAME, or just disable wildcards
> when :(glob) is not present.

Yeah, I think that is sensible.

I am meaning to merge your retire-fnmatch topic to 'master'.  What
should the Release Notes say for the upcoming release?  

You can build with USE_WILDMATCH=YesPlease to use a replacement
implementation of pattern matching logic used for pathname-like
things, e.g.  refnames and paths in the repository.  The new
implementation is not expected change the existing behaviour of
Git at all, except for "git for-each-ref" where you can now say
"refs/**/master" and match with both refs/heads/master and
refs/remotes/origin/master.  In future versions of Git, we plan
to use this new implementation in wider places (e.g. "git log
'**/t*.sh'" may find commits that touch a shell script whose
name begins with "t" at any level), but we are not there yet.
By building with USE_WILDMATCH, using the resulting Git daily
and reporting when you find breakages, you can help us get
closer to that goal.

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

Re: [PATCH] send-email: Honor multi-part email messages

[Junio C Hamano]

Alexey Shumkin  writes:

> Bump

Bump what???
--
To unsubscribe from this list: send the line "unsubscribe 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/7] sha1_file: keep track of where an SHA-1 object comes from

[Junio C Hamano]

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

> We currently know if an object is loose or packed. We do not know if
> it's from the repo's object database, or via alternates
> mechanism. With this patch, sha1_object_info_extended() can tell if an
> object comes from alternates source (and which one).
>
> Signed-off-by: Nguyễn Thái Ngọc Duy 
> ---
>  How about this way instead: we keep track of where objects come from
>  so we can verify object source when we create or update something
>  that contains SHA-1.

The overall approach taken by this series may be worth considering, but
I do not think the check implemented here is correct.

An object may be found in an alternate odb but we may also have our
own copy of the same object.  You need to prove that a suspicious
object is visible to us *ONLY* through add_submodule_odb().

Once you do add_submodule_odb() to contaminate our object pool, you
make everything a suspicious object that needs to be checked; that
is the worst part of the story.

If I understand the test case that triggers the issue correctly, it
makes a merge in the top-level superproject, and while doing so,
tries to three-way merge gitlink changes, so that it can detect
fast-forward situation inside the submodule.  It should just be done
with running "git merge-base A B" there; after all, we are in the
tree-wide merge of the top-level superproject, which is already a
heavy-weight operation. I do not see a reason to make it more brittle
than necessary.

If we rip out add_submodule_odb() we do not have to ever worry about
this kind of object database breakages that are hard to track down.
It is an optimization to allow the code become more brittle, violate
the project boundary, and break the object database faster as a
result.  That is not a good optimization at all.

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

Re: [PATCH 1/2] upload-pack: avoid parsing objects during ref advertisement

[Junio C Hamano]

Jeff King  writes:

> And yeah, this should use lookup_unknown_object to extend the
> optimization to mark_our_ref (and avoid removing it for the
> ref-advertisement case, of course).

Thanks for sanity checking.  Here is what is queued at the bottom of
the hide-refs topic.

-- >8 --
Date: Fri, 18 Jan 2013 15:48:49 -0800
Subject: [PATCH] upload-pack: share more code

We mark the objects pointed at our refs with "OUR_REF" flag in two
functions (mark_our_ref() and send_ref()), but we can just use the
former as a helper for the latter.

Update the way mark_our_ref() prepares in-core object to use
lookup_unknown_object() to delay reading the actual object data,
just like we did in 435c833 (upload-pack: use peel_ref for ref
advertisements, 2012-10-04).

Signed-off-by: Junio C Hamano 
---
 upload-pack.c | 31 ++-
 1 file changed, 14 insertions(+), 17 deletions(-)

diff --git a/upload-pack.c b/upload-pack.c
index 95d8313..3dd220d 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -722,15 +722,28 @@ static void receive_needs(void)
free(shallows.objects);
 }
 
+static int mark_our_ref(const char *refname, const unsigned char *sha1, int 
flag, void *cb_data)
+{
+   struct object *o = lookup_unknown_object(sha1);
+   if (!o)
+   die("git upload-pack: cannot find object %s:", 
sha1_to_hex(sha1));
+   if (!(o->flags & OUR_REF)) {
+   o->flags |= OUR_REF;
+   nr_our_refs++;
+   }
+   return 0;
+}
+
 static int send_ref(const char *refname, const unsigned char *sha1, int flag, 
void *cb_data)
 {
static const char *capabilities = "multi_ack thin-pack side-band"
" side-band-64k ofs-delta shallow no-progress"
" include-tag multi_ack_detailed";
-   struct object *o = lookup_unknown_object(sha1);
const char *refname_nons = strip_namespace(refname);
unsigned char peeled[20];
 
+   mark_our_ref(refname, sha1, flag, cb_data);
+
if (capabilities)
packet_write(1, "%s %s%c%s%s agent=%s\n",
 sha1_to_hex(sha1), refname_nons,
@@ -740,27 +753,11 @@ static int send_ref(const char *refname, const unsigned 
char *sha1, int flag, vo
else
packet_write(1, "%s %s\n", sha1_to_hex(sha1), refname_nons);
capabilities = NULL;
-   if (!(o->flags & OUR_REF)) {
-   o->flags |= OUR_REF;
-   nr_our_refs++;
-   }
if (!peel_ref(refname, peeled))
packet_write(1, "%s %s^{}\n", sha1_to_hex(peeled), 
refname_nons);
return 0;
 }
 
-static int mark_our_ref(const char *refname, const unsigned char *sha1, int 
flag, void *cb_data)
-{
-   struct object *o = parse_object(sha1);
-   if (!o)
-   die("git upload-pack: cannot find object %s:", 
sha1_to_hex(sha1));
-   if (!(o->flags & OUR_REF)) {
-   o->flags |= OUR_REF;
-   nr_our_refs++;
-   }
-   return 0;
-}
-
 static void upload_pack(void)
 {
if (advertise_refs || !stateless_rpc) {
-- 
1.8.1.1.525.gdace530



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

Re: [PATCH v4 3/3] push: introduce REJECT_FETCH_FIRST and REJECT_NEEDS_FORCE

[Junio C Hamano]

Jeff King  writes:

> I wonder if we can reword it to explain more about why we do not have
> the object, without getting too inaccurate. Something like:
>
>   Updates were rejected because the remote contains objects that you do
>   not have locally. This is usually caused by another repository pushing
>   to the same ref. You may want to first merge the remote changes (e.g.,
>   'git pull') before pushing again.
>
> I was also tempted to s/objects/work/, which is more vague, but is less
> jargon-y for new users who do not know how git works.

After all this is "hint", and there is a value in being more
approachable at the cost of being less accurate, over being
impenetrable to achieve perfect correctness.

> Also, how should this interact with the checkout-then-pull-then-push
> advice? We make a distinction for the non-fastforward case between HEAD
> and other refs. Should we be making the same distinction here?

Possibly, but I am not among the people who cared most about the
distinction there; with the default behaviour switching to 'simple',
that distinction will start mattering even less, I suspect.
--
To unsubscribe from this list: send the line "unsubscribe 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] parse_object: clear "parsed" when freeing buffers

[Junio C Hamano]

Jeff King  writes:

> The ensure_commit_buffer function could look something like:
>
>   int ensure_commit_buffer(struct commit *item)
>   {
>   enum object_type type;
>   unsigned long size;
>
>   if (!item)
>   return -1;
>   if (!item->object.parsed)
>   return parse_commit(item);
>   if (item->buffer)
>   return 0;
>
>   item->buffer = read_sha1_file(item->object.sha1, &type, &size);
>   if (!item->buffer)
>   return error("Could not read %s",
>sha1_to_hex(item->object.sha1);
>   return 0;
>   }

The more important issue is when to release them.

Only the subcommands that know that what they are doing do not need
to access commit->buffer and they operate solely on parsed data are
suppposed to flip the save_commit_buffer bit off to reclaim the
memory early, so new callers that violate that assumption may want
to call ensure_*, but if they did so when somebody cares about the
memory pressure enough to turn the bit off need to free the buffer
themselves after they use it.  Otherwise the resulting program may
still be "correct" but we will end up keeping buffer that nobody
will use, just in case it is asked again.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: segmentation fault (nullpointer) with git log --submodule -p

[Duy Nguyen]

On Thu, Jan 24, 2013 at 9:06 PM, Stefan Näwe
 wrote:
> Am Donnerstag, 24. Januar 2013 14:40:47 schrieb Duy Nguyen:
>> On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe
>>  wrote:
 Does it fail with older versions of git? If so, can you bisect?
>>>
>>> I did. My bisection told me this is the suspect:
>>>
>>> ccdc603 (parse_object: try internal cache before reading object db)
>>
>> diff --git a/object.c b/object.c
>> index d8d09f9..6b06297 100644
>> --- a/object.c
>> +++ b/object.c
>> @@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)
>> enum object_type type;
>> int eaten;
>> const unsigned char *repl = lookup_replace_object(sha1);
>> -   void *buffer = read_sha1_file(sha1, &type, &size);
>> +   void *buffer;
>> +   struct object *obj;
>> +
>> +   obj = lookup_object(sha1);
>> +   if (obj && obj->parsed)
>> +   return obj;
>>
>> Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?
>> What if you change that "if" to
>>
>> if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit
>> *)obj)->buffer))
>>
>
> No more segfault!

Sweet. I have no idea how that fixes it. Maybe Jeff can give some
explanation after he wakes up.
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: segmentation fault (nullpointer) with git log --submodule -p

[Stefan Näwe]

Am Donnerstag, 24. Januar 2013 14:40:47 schrieb Duy Nguyen:
> On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe
>  wrote:
>>> Does it fail with older versions of git? If so, can you bisect?
>>
>> I did. My bisection told me this is the suspect:
>>
>> ccdc603 (parse_object: try internal cache before reading object db)
>
> diff --git a/object.c b/object.c
> index d8d09f9..6b06297 100644
> --- a/object.c
> +++ b/object.c
> @@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)
> enum object_type type;
> int eaten;
> const unsigned char *repl = lookup_replace_object(sha1);
> -   void *buffer = read_sha1_file(sha1, &type, &size);
> +   void *buffer;
> +   struct object *obj;
> +
> +   obj = lookup_object(sha1);
> +   if (obj && obj->parsed)
> +   return obj;
>
> Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?
> What if you change that "if" to
>
> if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit
> *)obj)->buffer))
>

No more segfault!

> Also you did not encode commits in any specific encoding,

We're using Git for Windows and some commits contain 'umlauts' (äöü).
But those characters should be encoded in UTF-8, shouldn't they?
But the 'git log...' only crashes on a Debian/Linux machine.

> nor set i18n.logOutputEncoding?

It's not set.

(only i18n.filesEncoding is set to utf-8 on my machine)

Oh, and it's not crashing if I do:

git log -p --submodule |cat

Stefan
--

/dev/random says: Dumb luck beats sound planning every time. Trust me.
python -c "print 
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: segmentation fault (nullpointer) with git log --submodule -p

[Duy Nguyen]

On Thu, Jan 24, 2013 at 7:11 PM, Stefan Näwe
 wrote:
>> Does it fail with older versions of git? If so, can you bisect?
>
> I did. My bisection told me this is the suspect:
>
> ccdc603 (parse_object: try internal cache before reading object db)

diff --git a/object.c b/object.c
index d8d09f9..6b06297 100644
--- a/object.c
+++ b/object.c
@@ -191,10 +191,15 @@ struct object *parse_object(const unsigned char *sha1)
enum object_type type;
int eaten;
const unsigned char *repl = lookup_replace_object(sha1);
-   void *buffer = read_sha1_file(sha1, &type, &size);
+   void *buffer;
+   struct object *obj;
+
+   obj = lookup_object(sha1);
+   if (obj && obj->parsed)
+   return obj;

Any chance obj->parsed is 1 but ((struct commit*)obj)->buffer is NULL?
What if you change that "if" to

if (obj && obj->parsed && (obj->type != OBJ_COMMIT || ((struct commit
*)obj)->buffer))

??

Also you did not encode commits in any specific encoding, nor set
i18n.logOutputEncoding?
-- 
Duy
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: segmentation fault (nullpointer) with git log --submodule -p

[Stefan Näwe]

Am 23.01.2013 21:02, schrieb Jeff King:
> On Wed, Jan 23, 2013 at 03:38:16PM +0100, Armin wrote:
> 
>> Hello dear git people.
>>
>> I experience a reproducible segmentation fault on one of my
>> repositories when doing a "git log --submodule -p", tested with newest
>> version on Arch Linux (git version 1.8.1.1) and built fresh (git
>> version 1.8.1.1.347.g9591fcc), tried on 2 seperate systems:
>>
>>
>> Program terminated with signal 11, Segmentation fault.
>> #0  0x004b51e5 in parse_commit_header (context=0x769b6980) at 
>> pretty.c:752
>> 752 for (i = 0; msg[i]; i++) {
>> [...]
>> (gdb) l
>> 747 static void parse_commit_header(struct format_commit_context *context)
>> 748 {
>> 749 const char *msg = context->message;
>> 750 int i;
>> 751 
>> 752 for (i = 0; msg[i]; i++) {
>> 753 int eol;
>> 754 for (eol = i; msg[eol] && msg[eol] != '\n'; eol++)
>> 755 ; /* do nothing */
>> 756 
>> (gdb) p msg
>> $7 = 
>> (gdb) p context->message
>> $8 = 0x0
> 
> Yeah, that should definitely not be NULL. I can't reproduce here with a
> few simple examples, though.
> 
> Does it fail with older versions of git? If so, can you bisect?

I did. My bisection told me this is the suspect:

ccdc603 (parse_object: try internal cache before reading object db)

My git-fu is not good enough to analyze that...

> Is it possible for you to make your repo available?

Unfortunately not. It crashes with one particular repos (using submodules)
that I can't make available but not with another which is available
at https://github.com/snaewe/super.git

HTH

Stefan
-- 

/dev/random says: There must be more to life than compile-and-go.
python -c "print 
'73746566616e2e6e616577654061746c61732d656c656b74726f6e696b2e636f6d'.decode('hex')"
--
To unsubscribe from this list: send the line "unsubscribe 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] mergetools: Add tortoisegitmerge helper

[Sven Strickroth]

Am 21.01.2013 09:26 schrieb Sven Strickroth:
> - The TortoiseGit team renamed TortoiseMerge.exe to TortoiseGitMerge.exe
>   (starting with 1.8.0) in order to make clear that this one has special
>   support for git and prevent confusion with the TortoiseSVN TortoiseMerge
>   version.
> - The tortoisemerge mergetool does not work with filenames which have
>   a space in it. Fixing this required changes in git and also in
>   TortoiseGitMerge; see https://github.com/msysgit/msysgit/issues/57.
> 
> The new tortoisegitmerge helper was added so that people can still use
> TortoiseMerge from TortoiseSVN (and older TortoiseGit versions).

Any comments for this patch?

-- 
Best regards,
 Sven Strickroth
 PGP key id F5A9D4C4 @ any key-server
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] send-email: Honor multi-part email messages

[Alexey Shumkin]

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

[PULL (updated)] git-svn updates for master

[Eric Wong]

I've squashed the change I mentioned in
http://mid.gmane.org/20130124101417.ga22...@dcvr.yhbt.net
to Barry's commit, and added Jonathan's Reviewed-by to my cleanup
commit.

The following changes since commit ec3ae6ec46ed48383ae40643990f169b65a563cc:

  Merge git://ozlabs.org/~paulus/gitk (2013-01-23 08:35:03 -0800)

are available in the git repository at:


  git://bogomips.org/git-svn for-junio

for you to fetch changes up to bc93ceb7c5ec959ad275f575a3c49ff491337a8b:

  git-svn: Simplify calculation of GIT_DIR (2013-01-24 10:21:23 +)

(My old master is unchanged.)


Barry Wardell (1):
  git-svn: Simplify calculation of GIT_DIR

Eric Wong (1):
  git-svn: cleanup sprintf usage for uppercasing hex

 git-svn.perl | 37 ++---
 perl/Git/SVN.pm  |  4 ++--
 perl/Git/SVN/Editor.pm   |  2 +-
 t/t9100-git-svn-basic.sh |  8 
 4 files changed, 25 insertions(+), 26 deletions(-)
-- 
Eric Wong
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH v3 0/2] Make git-svn work with gitdir links

[Eric Wong]

Junio C Hamano  wrote:
> Eric Wong  writes:
> 
> > diff --git a/git-svn.perl b/git-svn.perl
> > index c232798..e5bd292 100755
> > --- a/git-svn.perl
> > +++ b/git-svn.perl
> > @@ -332,11 +332,13 @@ if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
> > $ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
> > } "Unable to find .git directory\n";
> > my $cdup = undef;
> > +   my $git_dir = delete $ENV{GIT_DIR};
> > git_cmd_try {
> > $cdup = command_oneline(qw/rev-parse --show-cdup/);
> > chomp $cdup if ($cdup);
> > $cdup = "." unless ($cdup && length $cdup);
> > -   } "Already at toplevel, but $ENV{GIT_DIR} not found\n";
> > +   } "Already at toplevel, but $git_dir not found\n";
> > +   $ENV{GIT_DIR} = $git_dir;
> > chdir $cdup or die "Unable to chdir up to '$cdup'\n";
> > $_repository = Git->repository(Repository => $ENV{GIT_DIR});
> >  }
> 
> This does not look quite right, though.
> 
> Can't the user have his own $GIT_DIR when this command is invoked?
> The first command_oneline() runs rev-parse with that environment and
> get the user specified value of GIT_DIR in $ENV{GIT_DIR}, but by
> doing a "delete" before running --show-cdup, you are not honoring
> that GIT_DIR (and GIT_WORK_TREE if exists) the user gave you.  You
> already used that GIT_DIR when you asked rev-parse --git-dir to find
> what the GIT_DIR value should be, so you would be operating with
> values of $git_dir and $cdup that you discovered in an inconsistent
> way, no?
> 
> Shouldn't it be more like this instead?
> 
>   my ($git_dir, $cdup) = undef;
> try {
>   $git_dir = command_oneline(qw(rev-parse --git-dir));
>   } "Unable to ...";
> try {
>   $cdup = command_oneline(qw(rev-parse --show-cdup));
>   ... tweak $cdup ...
>   } "Unable to ...";
>   if (defined $git_dir) { $ENV{GIT_DIR} = $git_dir; }
>   chdir $cdup;

Thanks, I'll squash the following and push a new branch.  I don't
believe the (defined $git_dir) check is necessary since we already
checked for errors with git_cmd_try.

diff --git a/git-svn.perl b/git-svn.perl
index e5bd292..b46795f 100755
--- a/git-svn.perl
+++ b/git-svn.perl
@@ -324,19 +324,18 @@ for (my $i = 0; $i < @ARGV; $i++) {
}
 };
 
 # make sure we're always running at the top-level working directory
 if ($cmd && $cmd =~ /(?:clone|init|multi-init)$/) {
$ENV{GIT_DIR} ||= ".git";
 } else {
+   my ($git_dir, $cdup);
git_cmd_try {
-   $ENV{GIT_DIR} = command_oneline([qw/rev-parse --git-dir/]);
+   $git_dir = command_oneline([qw/rev-parse --git-dir/]);
} "Unable to find .git directory\n";
-   my $cdup = undef;
-   my $git_dir = delete $ENV{GIT_DIR};
git_cmd_try {
$cdup = command_oneline(qw/rev-parse --show-cdup/);
chomp $cdup if ($cdup);
$cdup = "." unless ($cdup && length $cdup);
} "Already at toplevel, but $git_dir not found\n";
$ENV{GIT_DIR} = $git_dir;
chdir $cdup or die "Unable to chdir up to '$cdup'\n";

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

[PATCH v4 3/4] pretty: Add failing tests: user format ignores i18n.logOutputEncoding setting

[Alexey Shumkin]

The following two commands are expected to give the same output to a terminal:

$ git log --oneline --no-color
$ git log --pretty=format:'%h %s'

However, the former pays attention to i18n.logOutputEncoding
configuration, while the latter does not when it format "%s".
Log messages written in an encoding i18n.commitEncoding which differs
from terminal encoding are shown corrupted with the latter even
when i18n.logOutputEncoding and terminal encoding are the same.

The same corruption is true for
$ git diff --submodule=log
and
$ git rev-list --pretty=format:%s HEAD
and
$ git reset --hard

Signed-off-by: Alexey Shumkin 
---
 t/t4041-diff-submodule-option.sh | 33 ---
 t/t4205-log-pretty-formats.sh| 43 +++
 t/t6006-rev-list-format.sh   | 90 ++--
 t/t7102-reset.sh | 32 +++---
 4 files changed, 138 insertions(+), 60 deletions(-)

diff --git a/t/t4041-diff-submodule-option.sh b/t/t4041-diff-submodule-option.sh
index 32d4a60..e7d6363 100755
--- a/t/t4041-diff-submodule-option.sh
+++ b/t/t4041-diff-submodule-option.sh
@@ -1,6 +1,7 @@
 #!/bin/sh
 #
 # Copyright (c) 2009 Jens Lehmann, based on t7401 by Ping Yin
+# Copyright (c) 2011 Alexey Shumkin (+ non-UTF-8 commit encoding tests)
 #
 
 test_description='Support for verbose submodule differences in git diff
@@ -10,6 +11,7 @@ This test tries to verify the sanity of the --submodule 
option of git diff.
 
 . ./test-lib.sh
 
+added=$(printf 
"\320\264\320\276\320\261\320\260\320\262\320\273\320\265\320\275")
 add_file () {
(
cd "$1" &&
@@ -19,7 +21,8 @@ add_file () {
echo "$name" >"$name" &&
git add "$name" &&
test_tick &&
-   git commit -m "Add $name" || exit
+   msg_added_cp1251=$(echo "Add $name ($added $name)" | 
iconv -f utf-8 -t cp1251) &&
+   git -c 'i18n.commitEncoding=cp1251' commit -m 
"$msg_added_cp1251"
done >/dev/null &&
git rev-parse --short --verify HEAD
)
@@ -89,29 +92,29 @@ test_expect_success 'diff.submodule does not affect 
plumbing' '
 commit_file sm1 &&
 head2=$(add_file sm1 foo3)
 
-test_expect_success 'modified submodule(forward)' '
+test_expect_failure 'modified submodule(forward)' '
git diff-index -p --submodule=log HEAD >actual &&
cat >expected <<-EOF &&
Submodule sm1 $head1..$head2:
- > Add foo3
+ > Add foo3 ($added foo3)
EOF
test_cmp expected actual
 '
 
-test_expect_success 'modified submodule(forward)' '
+test_expect_failure 'modified submodule(forward)' '
git diff --submodule=log >actual &&
cat >expected <<-EOF &&
Submodule sm1 $head1..$head2:
- > Add foo3
+ > Add foo3 ($added foo3)
EOF
test_cmp expected actual
 '
 
-test_expect_success 'modified submodule(forward) --submodule' '
+test_expect_failure 'modified submodule(forward) --submodule' '
git diff --submodule >actual &&
cat >expected <<-EOF &&
Submodule sm1 $head1..$head2:
- > Add foo3
+ > Add foo3 ($added foo3)
EOF
test_cmp expected actual
 '
@@ -138,25 +141,25 @@ head3=$(
git rev-parse --short --verify HEAD
 )
 
-test_expect_success 'modified submodule(backward)' '
+test_expect_failure 'modified submodule(backward)' '
git diff-index -p --submodule=log HEAD >actual &&
cat >expected <<-EOF &&
Submodule sm1 $head2..$head3 (rewind):
- < Add foo3
- < Add foo2
+ < Add foo3 ($added foo3)
+ < Add foo2 ($added foo2)
EOF
test_cmp expected actual
 '
 
 head4=$(add_file sm1 foo4 foo5)
-test_expect_success 'modified submodule(backward and forward)' '
+test_expect_failure 'modified submodule(backward and forward)' '
git diff-index -p --submodule=log HEAD >actual &&
cat >expected <<-EOF &&
Submodule sm1 $head2...$head4:
- > Add foo5
- > Add foo4
- < Add foo3
- < Add foo2
+ > Add foo5 ($added foo5)
+ > Add foo4 ($added foo4)
+ < Add foo3 ($added foo3)
+ < Add foo2 ($added foo2)
EOF
test_cmp expected actual
 '
diff --git a/t/t4205-log-pretty-formats.sh b/t/t4205-log-pretty-formats.sh
index 98a43d4..76ffa0d 100755
--- a/t/t4205-log-pretty-formats.sh
+++ b/t/t4205-log-pretty-formats.sh
@@ -1,20 +1,31 @@
 #!/bin/sh
 #
 # Copyright (c) 2010, Will Palmer
+# Copyright (c) 2011, Alexey Shumkin (+ non-UTF-8 commit encoding tests)
 #
 
 test_description='Test pretty formats'
 . ./test-lib.sh
 
+commit_msg() {
+   msg=$(printf "initial \320\272\320\276\320\274\320\274\320\270\321\202")
+   if test -n "$1"; then
+   msg=$(echo $msg | iconv -f utf-8 -t $1)
+   fi
+   echo $msg
+}
+
 test_expect_success 'set up ba

[PATCH v4 2/4] t7102 (reset): refactoring: don't hardcode SHA-1 in expected outputs

[Alexey Shumkin]

The expected SHA-1 digests are always available in variables.  Use
them instead of hardcoding.

Signed-off-by: Alexey Shumkin 
---
 t/t7102-reset.sh | 41 +
 1 file changed, 21 insertions(+), 20 deletions(-)

diff --git a/t/t7102-reset.sh b/t/t7102-reset.sh
index b096dc8..cf492f4 100755
--- a/t/t7102-reset.sh
+++ b/t/t7102-reset.sh
@@ -28,7 +28,8 @@ test_expect_success 'creating initial files and commits' '
 
echo "1st line 2nd file" >secondfile &&
echo "2nd line 2nd file" >>secondfile &&
-   git commit -a -m "modify 2nd file"
+   git commit -a -m "modify 2nd file" &&
+   head5=$(git rev-parse --verify HEAD)
 '
 # git log --pretty=oneline # to see those SHA1 involved
 
@@ -56,7 +57,7 @@ test_expect_success 'giving a non existing revision should 
fail' '
test_must_fail git reset --mixed aa &&
test_must_fail git reset --soft aa &&
test_must_fail git reset --hard aa &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   check_changes $head5
 '
 
 test_expect_success 'reset --soft with unmerged index should fail' '
@@ -74,7 +75,7 @@ test_expect_success \
test_must_fail git reset --hard -- first &&
test_must_fail git reset --soft HEAD^ -- first &&
test_must_fail git reset --hard HEAD^ -- first &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   check_changes $head5
 '
 
 test_expect_success 'giving unrecognized options should fail' '
@@ -86,7 +87,7 @@ test_expect_success 'giving unrecognized options should fail' 
'
test_must_fail git reset --soft -o &&
test_must_fail git reset --hard --other &&
test_must_fail git reset --hard -o &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   check_changes $head5
 '
 
 test_expect_success \
@@ -110,7 +111,7 @@ test_expect_success \
 
git checkout master &&
git branch -D branch1 branch2 &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   check_changes $head5
 '
 
 test_expect_success \
@@ -133,27 +134,27 @@ test_expect_success \
 
git checkout master &&
git branch -D branch3 branch4 &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   check_changes $head5
 '
 
 test_expect_success \
'resetting to HEAD with no changes should succeed and do nothing' '
git reset --hard &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
git reset --hard HEAD &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
git reset --soft &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
git reset --soft HEAD &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
git reset --mixed &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
git reset --mixed HEAD &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
git reset &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
git reset HEAD &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   check_changes $head5
 '
 
 >.diff_expect
@@ -176,7 +177,7 @@ test_expect_success '--soft reset only should show changes 
in diff --cached' '
git reset --soft HEAD^ &&
check_changes d1a4bc3abce4829628ae2dcb0d60ef3d1a78b1c4 &&
test "$(git rev-parse ORIG_HEAD)" = \
-   3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   $head5
 '
 
 >.diff_expect
@@ -193,7 +194,7 @@ test_expect_success \
git commit -a -C ORIG_HEAD &&
check_changes 3d3b7be011a58ca0c179ae45d94e6c83c0b0cd0d &&
test "$(git rev-parse ORIG_HEAD)" = \
-   3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   $head5
 '
 
 >.diff_expect
@@ -303,7 +304,7 @@ test_expect_success 'redoing the last two commits should 
succeed' '
echo "1st line 2nd file" >secondfile &&
echo "2nd line 2nd file" >>secondfile &&
git commit -a -m "modify 2nd file" &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc
+   check_changes $head5
 '
 
 >.diff_expect
@@ -341,15 +342,15 @@ EOF
 test_expect_success \
'--hard reset to ORIG_HEAD should clear a fast-forward merge' '
git reset --hard HEAD^ &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $head5 &&
 
git pull . branch1 &&
git reset --hard ORIG_HEAD &&
-   check_changes 3ec39651e7f44ea531a5de18a9fa791c0fd370fc &&
+   check_changes $

[PATCH v4 1/4] t6006 (rev-list-format): don't hardcode SHA-1 in expected outputs

[Alexey Shumkin]

The expected SHA-1 digests are always available in variables.  Use
them instead of hardcoding.

Signed-off-by: Alexey Shumkin 
---
 t/t6006-rev-list-format.sh | 130 +
 1 file changed, 72 insertions(+), 58 deletions(-)

diff --git a/t/t6006-rev-list-format.sh b/t/t6006-rev-list-format.sh
index f94f0c4..c248509 100755
--- a/t/t6006-rev-list-format.sh
+++ b/t/t6006-rev-list-format.sh
@@ -6,8 +6,19 @@ test_description='git rev-list --pretty=format test'
 
 test_tick
 test_expect_success 'setup' '
-touch foo && git add foo && git commit -m "added foo" &&
-  echo changed >foo && git commit -a -m "changed foo"
+   touch foo &&
+   git add foo &&
+   git commit -m "added foo" &&
+   head1=$(git rev-parse --verify HEAD) &&
+   head1_7=$(echo $head1 | cut -c1-7) &&
+   echo changed >foo &&
+   git commit -a -m "changed foo" &&
+   head2=$(git rev-parse --verify HEAD) &&
+   head2_7=$(echo $head2 | cut -c1-7) &&
+   head2_parent=$(git cat-file -p HEAD | grep parent | cut -f 2 -d" ") &&
+   head2_parent_7=$(echo $head2_parent | cut -c1-7) &&
+   tree2=$(git cat-file -p HEAD | grep tree | cut -f 2 -d" ") &&
+   tree2_7=$(echo $tree2 | cut -c1-7)
 '
 
 # usage: test_format name format_string foo && git commit -a -F commit-msg
+   git config i18n.commitencoding iso8859-1 &&
+   echo change2 >foo && git commit -a -F commit-msg &&
+   head3=$(git rev-parse --verify HEAD) &&
+   head3_7=$(echo $head3 | cut -c1-7)
 '
 
-test_format complex-encoding %e <<'EOF'
-commit f58db70b055c5718631e5c61528b28b12090cdea
+test_format complex-encoding %e expect commit $head3 &&
echo >>expect fooQbar &&
git rev-list -1 --format=foo%x00bar HEAD >actual.nul &&
nul_to_q actual &&
-- 
1.8.1.1.10.g9255f3f

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

[PATCH v4 0/4] Reroll patches against v1.8.1.1

[Alexey Shumkin]

Wf! 
Reroll my patches again after 1.5 years ;)
They must be applied against v1.8.1.1
Sorry for my laziness ;)

Alexey Shumkin (4):
  t6006 (rev-list-format): don't hardcode SHA-1 in expected outputs
  t7102 (reset): refactoring: don't hardcode SHA-1 in expected outputs
  pretty: Add failing tests: user format ignores i18n.logOutputEncoding
setting
  pretty: user format ignores i18n.logOutputEncoding setting

 builtin/reset.c  |   8 +-
 builtin/rev-list.c   |   1 +
 builtin/shortlog.c   |   1 +
 log-tree.c   |   1 +
 submodule.c  |   3 +
 t/t4041-diff-submodule-option.sh |  23 +++--
 t/t4205-log-pretty-formats.sh|  37 ++--
 t/t6006-rev-list-format.sh   | 200 +++
 t/t7102-reset.sh |  71 +-
 9 files changed, 224 insertions(+), 121 deletions(-)

-- 
1.8.1.1.10.g9255f3f

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

[PATCH 7/7] refs: do not update ref(log) referring to external objects

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 refs.c | 24 
 1 file changed, 24 insertions(+)

diff --git a/refs.c b/refs.c
index 541fec2..86003af 100644
--- a/refs.c
+++ b/refs.c
@@ -2014,6 +2014,18 @@ static int log_ref_write(const char *refname, const 
unsigned char *old_sha1,
char *logrec;
const char *committer;
 
+   if (object_database_contaminated) {
+   struct object_info oi;
+   if (sha1_object_info_extended(old_sha1, &oi) == -1 ||
+   (oi.alt && oi.alt->external))
+   return error("cannot update reflog pointing to an 
external SHA-1 %s",
+sha1_to_hex(old_sha1));
+   if (sha1_object_info_extended(new_sha1, &oi) == -1 ||
+   (oi.alt && oi.alt->external))
+   return error("cannot update reflog pointing to an 
external SHA-1 %s",
+sha1_to_hex(new_sha1));
+   }
+
if (log_all_ref_updates < 0)
log_all_ref_updates = !is_bare_repository();
 
@@ -2054,6 +2066,18 @@ int write_ref_sha1(struct ref_lock *lock,
 
if (!lock)
return -1;
+
+   if (object_database_contaminated) {
+   struct object_info oi;
+   if (sha1_object_info_extended(sha1, &oi) == -1 ||
+   (oi.alt && oi.alt->external)) {
+   error("cannot update ref pointing to an external SHA-1 
%s",
+ sha1_to_hex(sha1));
+   unlock_ref(lock);
+   return -1;
+   }
+   }
+
if (!lock->force_write && !hashcmp(lock->old_sha1, sha1)) {
unlock_ref(lock);
return 0;
-- 
1.8.0.rc3.18.g0d9b108

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

[PATCH 6/7] read-cache: refuse to create index referring to external objects

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 read-cache.c | 20 
 1 file changed, 20 insertions(+)

diff --git a/read-cache.c b/read-cache.c
index fda78bc..4579215 100644
--- a/read-cache.c
+++ b/read-cache.c
@@ -1720,6 +1720,26 @@ static int ce_write_entry(git_SHA_CTX *c, int fd, struct 
cache_entry *ce,
  ce->name + common, ce_namelen(ce) - common);
}
 
+   if (object_database_contaminated) {
+   struct object_info oi;
+   switch (ce->ce_mode) {
+   case S_IFGITLINK:
+   break;
+   case S_IFDIR:
+   if (sha1_object_info_extended(ce->sha1, &oi) != 
OBJ_TREE ||
+   (oi.alt && oi.alt->external))
+   die("cannot create index referring to an 
external tree %s",
+   sha1_to_hex(ce->sha1));
+   break;
+   default:
+   if (sha1_object_info_extended(ce->sha1, &oi) != 
OBJ_BLOB ||
+   (oi.alt && oi.alt->external))
+   die("cannot create index referring to an 
external blob %s",
+   sha1_to_hex(ce->sha1));
+   break;
+   }
+   }
+
result = ce_write(c, fd, ondisk, size);
free(ondisk);
return result;
-- 
1.8.0.rc3.18.g0d9b108

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

[PATCH 5/7] sha1_file: refuse to write tags referring to external objects

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 sha1_file.c | 6 ++
 1 file changed, 6 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index ec3a040..01681e5 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2666,6 +2666,12 @@ static void check_sha1_file_for_external_source(const 
char *buf,
sha1_to_hex(entry.sha1));
break;
}
+   } else if (!strcmp(type, "tag")) {
+   if (get_sha1_hex(buf + 7, sha1) < 0 ||
+   sha1_object_info_extended(sha1, &oi) != OBJ_TREE ||
+   (oi.alt && oi.alt->external))
+   die("cannot create a tag with external tree %s",
+   sha1_to_hex(sha1));
}
 }
 
-- 
1.8.0.rc3.18.g0d9b108

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

[PATCH 4/7] sha1_file: refuse to write trees referring to external objects

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 sha1_file.c | 23 +++
 1 file changed, 23 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index 5948dcb..ec3a040 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2643,6 +2643,29 @@ static void check_sha1_file_for_external_source(const 
char *buf,
sha1_to_hex(sha1));
buf += 48; /* "parent " + hex sha1 + "\n" */
}
+   } else if (!strcmp(type, "tree")) {
+   struct tree_desc desc;
+   struct name_entry entry;
+
+   init_tree_desc(&desc, buf, len);
+   while (tree_entry(&desc, &entry))
+   switch (entry.mode) {
+   case S_IFGITLINK:
+   /* we already know we probably don't have this 
SHA-1 */
+   break;
+   case S_IFDIR:
+   if (sha1_object_info_extended(entry.sha1, &oi) 
!= OBJ_TREE ||
+   (oi.alt && oi.alt->external))
+   die("cannot create a tree with external 
tree %s",
+   sha1_to_hex(entry.sha1));
+   break;
+   default:
+   if (sha1_object_info_extended(entry.sha1, &oi) 
!= OBJ_BLOB ||
+   (oi.alt && oi.alt->external))
+   die("cannot create a tree with external 
blob %s",
+   sha1_to_hex(entry.sha1));
+   break;
+   }
}
 }
 
-- 
1.8.0.rc3.18.g0d9b108

--
To unsubscribe from this list: send the line "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/7] sha1_file: refuse to write commits referring to external objects

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 sha1_file.c | 34 ++
 1 file changed, 34 insertions(+)

diff --git a/sha1_file.c b/sha1_file.c
index af71122..5948dcb 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -2615,12 +2615,46 @@ static int write_loose_object(const unsigned char 
*sha1, char *hdr, int hdrlen,
return move_temp_to_file(tmp_file, filename);
 }
 
+static void check_sha1_file_for_external_source(const char *buf,
+   unsigned long len,
+   const char *type)
+{
+   struct object_info oi;
+   unsigned char sha1[20];
+
+   /*
+* It is assumed that the object is well formatted. Otherwise
+* die() is waiting..
+*/
+   if (!strcmp(type, "commit")) {
+   const char *tail = buf + len;
+   if (get_sha1_hex(buf + 5, sha1) < 0 ||
+   sha1_object_info_extended(sha1, &oi) != OBJ_TREE ||
+   (oi.alt && oi.alt->external))
+   die("cannot create a commit with external tree %s",
+   sha1_to_hex(sha1));
+   buf += 46; /* "tree " + "hex sha1" + "\n" */
+
+   while (buf + 48 < tail && !memcmp(buf, "parent ", 7)) {
+   if (get_sha1_hex(buf + 7, sha1) < 0 ||
+   sha1_object_info_extended(sha1, &oi) != OBJ_COMMIT 
||
+   (oi.alt && oi.alt->external))
+   die("cannot create a commit with external 
parent %s",
+   sha1_to_hex(sha1));
+   buf += 48; /* "parent " + hex sha1 + "\n" */
+   }
+   }
+}
+
 int write_sha1_file(const void *buf, unsigned long len, const char *type, 
unsigned char *returnsha1)
 {
unsigned char sha1[20];
char hdr[32];
int hdrlen;
 
+   if (object_database_contaminated)
+   check_sha1_file_for_external_source(buf, len, type);
+
/* Normally if we have it in the pack then we do not bother writing
 * it out into .git/objects/??/?{38} file.
 */
-- 
1.8.0.rc3.18.g0d9b108

--
To unsubscribe from this list: send the line "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/7] sha1_file: separate alt object db from own repos and submodules's

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

A submodule's object database may be imported to in-core object pool
for a quick peek without paying the price of running a separate git
command. These databases are marked in for stricter checks later to
avoid accidentially refering to a submodule's SHA-1 from main repo
(except in gitlinks).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 cache.h   |  4 +++-
 environment.c |  2 ++
 sha1_file.c   | 38 +-
 submodule.c   |  2 +-
 4 files changed, 35 insertions(+), 11 deletions(-)

diff --git a/cache.h b/cache.h
index 92854ab..b8d5826 100644
--- a/cache.h
+++ b/cache.h
@@ -561,6 +561,7 @@ extern int fsync_object_files;
 extern int core_preload_index;
 extern int core_apply_sparse_checkout;
 extern int precomposed_unicode;
+extern int object_database_contaminated;
 
 enum branch_track {
BRANCH_TRACK_UNSPECIFIED = -1,
@@ -957,11 +958,12 @@ extern void remove_scheduled_dirs(void);
 
 extern struct alternate_object_database {
struct alternate_object_database *next;
+   int external;
char *name;
char base[FLEX_ARRAY]; /* more */
 } *alt_odb_list;
 extern void prepare_alt_odb(void);
-extern void read_info_alternates(const char * relative_base, int depth);
+extern void read_external_info_alternates(const char * relative_base, int 
depth);
 extern void add_to_alternates_file(const char *reference);
 typedef int alt_odb_fn(struct alternate_object_database *, void *);
 extern void foreach_alt_odb(alt_odb_fn, void*);
diff --git a/environment.c b/environment.c
index 85edd7f..3c90d95 100644
--- a/environment.c
+++ b/environment.c
@@ -65,6 +65,8 @@ unsigned long pack_size_limit_cfg;
 /* Parallel index stat data preload? */
 int core_preload_index = 0;
 
+int object_database_contaminated;
+
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
 char *git_work_tree_cfg;
 static char *work_tree;
diff --git a/sha1_file.c b/sha1_file.c
index afc7355..af71122 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -58,6 +58,9 @@ static struct cached_object empty_tree = {
 
 static struct packed_git *last_found_pack;
 
+static void read_info_alternates(const char * relative_base,
+int depth, int external);
+
 static struct cached_object *find_cached_object(const unsigned char *sha1)
 {
int i;
@@ -247,7 +250,10 @@ static int git_open_noatime(const char *name);
  * SHA1, an extra slash for the first level indirection, and the
  * terminating NUL.
  */
-static int link_alt_odb_entry(const char *entry, const char *relative_base, 
int depth)
+static int link_alt_odb_entry(const char *entry,
+ const char *relative_base,
+ int depth,
+ int external)
 {
const char *objdir = get_object_directory();
struct alternate_object_database *ent;
@@ -277,6 +283,7 @@ static int link_alt_odb_entry(const char *entry, const char 
*relative_base, int
memcpy(ent->base, pathbuf.buf, pfxlen);
strbuf_release(&pathbuf);
 
+   ent->external = external;
ent->name = ent->base + pfxlen + 1;
ent->base[pfxlen + 3] = '/';
ent->base[pfxlen] = ent->base[entlen-1] = 0;
@@ -310,15 +317,19 @@ static int link_alt_odb_entry(const char *entry, const 
char *relative_base, int
ent->next = NULL;
 
/* recursively add alternates */
-   read_info_alternates(ent->base, depth + 1);
+   read_info_alternates(ent->base, depth + 1, 0);
 
ent->base[pfxlen] = '/';
 
+   if (external)
+   object_database_contaminated = 1;
+
return 0;
 }
 
 static void link_alt_odb_entries(const char *alt, int len, int sep,
-const char *relative_base, int depth)
+const char *relative_base,
+int depth, int external)
 {
struct string_list entries = STRING_LIST_INIT_NODUP;
char *alt_copy;
@@ -340,14 +351,16 @@ static void link_alt_odb_entries(const char *alt, int 
len, int sep,
error("%s: ignoring relative alternate object store %s",
relative_base, entry);
} else {
-   link_alt_odb_entry(entry, relative_base, depth);
+   link_alt_odb_entry(entry, relative_base,
+  depth, external);
}
}
string_list_clear(&entries, 0);
free(alt_copy);
 }
 
-void read_info_alternates(const char * relative_base, int depth)
+static void read_info_alternates(const char * relative_base,
+int depth, int external)
 {
char *map;
size_t mapsz;
@@ -371,11 +384,18 @@ void read_info_alternates(const char * relative_base, int 
depth)
map = xmmap(NULL, mapsz, PROT_READ, MAP_PRIVATE, fd, 0);
close(fd);
 
-   link_alt_odb_entries(map, mapsz, '\n', relative

[PATCH 1/7] sha1_file: keep track of where an SHA-1 object comes from

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

We currently know if an object is loose or packed. We do not know if
it's from the repo's object database, or via alternates
mechanism. With this patch, sha1_object_info_extended() can tell if an
object comes from alternates source (and which one).

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 How about this way instead: we keep track of where objects come from
 so we can verify object source when we create or update something
 that contains SHA-1.

 1/7 and 2/7 prepare for tracking object source. The rest verifies
 that new commits, trees, tags, indexes, refs or reflogs do not refer
 to an external source.

 This adds some cost when add_submodule_odb() is used. I did not
 measure, but I guess the added cost is much smaller compared to
 forking, especially on Windows. No breakages detected by the test
 suite, which is really good (or my code is really broken).

 builtin/index-pack.c |  2 +-
 cache.h  |  4 +++-
 fast-import.c|  2 +-
 sha1_file.c  | 66 
 4 files changed, 51 insertions(+), 23 deletions(-)

diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 43d364b..a7de3f8 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1393,7 +1393,7 @@ static void read_v2_anomalous_offsets(struct packed_git 
*p,
 
 static void read_idx_option(struct pack_idx_option *opts, const char 
*pack_name)
 {
-   struct packed_git *p = add_packed_git(pack_name, strlen(pack_name), 1);
+   struct packed_git *p = add_packed_git(pack_name, strlen(pack_name), 1, 
NULL);
 
if (!p)
die(_("Cannot open existing pack file '%s'"), pack_name);
diff --git a/cache.h b/cache.h
index c257953..92854ab 100644
--- a/cache.h
+++ b/cache.h
@@ -978,6 +978,7 @@ struct pack_window {
 extern struct packed_git {
struct packed_git *next;
struct pack_window *windows;
+   struct alternate_object_database *alt;
off_t pack_size;
const void *index_data;
size_t index_size;
@@ -1066,7 +1067,7 @@ extern void close_pack_windows(struct packed_git *);
 extern void unuse_pack(struct pack_window **);
 extern void free_pack_by_name(const char *);
 extern void clear_delta_base_cache(void);
-extern struct packed_git *add_packed_git(const char *, int, int);
+extern struct packed_git *add_packed_git(const char *, int, int, struct 
alternate_object_database *);
 extern const unsigned char *nth_packed_object_sha1(struct packed_git *, 
uint32_t);
 extern off_t nth_packed_object_offset(const struct packed_git *, uint32_t);
 extern off_t find_pack_entry_one(const unsigned char *, struct packed_git *);
@@ -1102,6 +1103,7 @@ struct object_info {
unsigned int is_delta;
} packed;
} u;
+   struct alternate_object_database *alt;
 };
 extern int sha1_object_info_extended(const unsigned char *, struct object_info 
*);
 
diff --git a/fast-import.c b/fast-import.c
index c2a814e..4bf732e 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -964,7 +964,7 @@ static void end_packfile(void)
idx_name = keep_pack(create_index());
 
/* Register the packfile with core git's machinery. */
-   new_p = add_packed_git(idx_name, strlen(idx_name), 1);
+   new_p = add_packed_git(idx_name, strlen(idx_name), 1, NULL);
if (!new_p)
die("core git rejected index %s", idx_name);
all_packs[pack_id] = new_p;
diff --git a/sha1_file.c b/sha1_file.c
index 40b2329..afc7355 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -933,7 +933,8 @@ static void try_to_free_pack_memory(size_t size)
release_pack_memory(size, -1);
 }
 
-struct packed_git *add_packed_git(const char *path, int path_len, int local)
+struct packed_git *add_packed_git(const char *path, int path_len, int local,
+ struct alternate_object_database *alt)
 {
static int have_set_try_to_free_routine;
struct stat st;
@@ -973,6 +974,7 @@ struct packed_git *add_packed_git(const char *path, int 
path_len, int local)
p->mtime = st.st_mtime;
if (path_len < 40 || get_sha1_hex(path + path_len - 40, p->sha1))
hashclr(p->sha1);
+   p->alt = alt;
return p;
 }
 
@@ -1000,7 +1002,8 @@ void install_packed_git(struct packed_git *pack)
packed_git = pack;
 }
 
-static void prepare_packed_git_one(char *objdir, int local)
+static void prepare_packed_git_one(char *objdir, int local,
+  struct alternate_object_database *alt)
 {
/* Ensure that this buffer is large enough so that we can
   append "/pack/" without clobbering the stack even if
@@ -1041,7 +1044,7 @@ static void prepare_packed_git_one(char *objdir, int 
local)
/* See if it really is a valid .idx file with corresponding
 * .pack file that we can map.
 */
-   p = add_packed_git(pat