Re: [PATCH] describe: add tests for unusual graphs

[Quinn Grier]

On 2016-12-09 17:12, Junio C Hamano wrote:
> Quinn Grier  writes:
> 
>> git describe may give incorrect results if there are backdated commits
>> or multiple roots. This commit adds two test_expect_failure tests that
>> demonstrate these problems.
> 
> I am not sure if this is a good patch to take.  test_expect_failure
> is to demonstrate an incorrect behaviour that we wish to correct
> later, but I do not think these demonstrate incorrect behaviours to
> begin with.
> 
> For example, the latter one seems to expect that by asking to
> describe D in this picture
> 
>> +#
>> +# A---B*--D master
>> +#/
>> +#   C* other
>> +#
> 
> you seem to expect the description is based on B.  
> 
> It is not at all clear why it is incorrect if the description were
> made based on C.  If D were described relative to A, ignoring B,
> then I understand why that result is incorrect and I would agree
> that describing D in terms of B is more correct.  But I do not think
> that is what the test is trying to demonstrate.
> 
> But it is hard to guess only from looking at the test and the
> proposed log message, because it does not say what makes you think
> the behaviour you saw was incorrect.
> 

I thought the behavior was incorrect because of the following paragraph
from the documentation for git describe:

  If multiple tags were found during the walk then the tag
  which has the fewest commits different from the input
  commit-ish will be selected and output. Here fewest commits
  different is defined as the number of commits which would be
  shown by git log tag..input will be the smallest number of
  commits possible.

[PATCH 1/2] mergetools/kompare: simplify can_merge() by using "false"

[David Aguilar]

Signed-off-by: David Aguilar 
---
This patch builds upon da/mergetool-trust-exit-code

 mergetools/kompare | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergetools/kompare b/mergetools/kompare
index e8c0bfa678..321022500b 100644
--- a/mergetools/kompare
+++ b/mergetools/kompare
@@ -1,5 +1,5 @@
 can_merge () {
-   return 1
+   false
 }
 
 diff_cmd () {
-- 
2.11.0.27.gdeff8c7

[PATCH 2/2] mergetools/tortoisemerge: simplify can_diff() by using "false"

[David Aguilar]

Signed-off-by: David Aguilar 
---
This patch builds upon da/mergetool-trust-exit-code

 mergetools/tortoisemerge | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/mergetools/tortoisemerge b/mergetools/tortoisemerge
index d7ab666a59..9067d8a4e5 100644
--- a/mergetools/tortoisemerge
+++ b/mergetools/tortoisemerge
@@ -1,5 +1,5 @@
 can_diff () {
-   return 1
+   false
 }
 
 merge_cmd () {
-- 
2.11.0.27.gdeff8c7

[PATCH] mergetools: fix xxdiff hotkeys

[David Aguilar]

xxdiff was using a mix of "Ctrl-" and "Ctrl+" hotkeys.
The dashed "-" form is not accepted by newer xxdiff versions.
Use the plus "+" form only.

Signed-off-by: David Aguilar 
---
This patch is based on top of da/mergetool-diff-order

 mergetools/xxdiff | 10 +-
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/mergetools/xxdiff b/mergetools/xxdiff
index e284811ff2..ce5b8e9f29 100644
--- a/mergetools/xxdiff
+++ b/mergetools/xxdiff
@@ -1,7 +1,7 @@
 diff_cmd () {
"$merge_tool_path" \
-R 'Accel.Search: "Ctrl+F"' \
-   -R 'Accel.SearchForward: "Ctrl-G"' \
+   -R 'Accel.SearchForward: "Ctrl+G"' \
"$LOCAL" "$REMOTE"
 }
 
@@ -9,15 +9,15 @@ merge_cmd () {
if $base_present
then
"$merge_tool_path" -X --show-merged-pane \
-   -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+   -R 'Accel.SaveAsMerged: "Ctrl+S"' \
-R 'Accel.Search: "Ctrl+F"' \
-   -R 'Accel.SearchForward: "Ctrl-G"' \
+   -R 'Accel.SearchForward: "Ctrl+G"' \
--merged-file "$MERGED" "$LOCAL" "$BASE" "$REMOTE"
else
"$merge_tool_path" -X $extra \
-   -R 'Accel.SaveAsMerged: "Ctrl-S"' \
+   -R 'Accel.SaveAsMerged: "Ctrl+S"' \
-R 'Accel.Search: "Ctrl+F"' \
-   -R 'Accel.SearchForward: "Ctrl-G"' \
+   -R 'Accel.SearchForward: "Ctrl+G"' \
--merged-file "$MERGED" "$LOCAL" "$REMOTE"
fi
 }
-- 
2.11.0.rc3.8.gaca8798.dirty

Re: [PATCH v2 12/16] pathspec: create parse_long_magic function

[Brandon Williams]

On 12/09, Stefan Beller wrote:
> On Fri, Dec 9, 2016 at 3:44 PM, Junio C Hamano  wrote:
> > Brandon Williams  writes:
> >
> >> Factor out the logic responsible for parsing long magic into its own
> >> function.  As well as hoist the prefix check logic outside of the inner
> >> loop as there isn't anything that needs to be done after matching
> >> "prefix:".
> >>
> >> Signed-off-by: Brandon Williams 
> >
> > These refactoring changes look like they are all going in the good
> > direction.  Stefan's :(attr:)path" changes however
> > have severe conflicts (e.g. the topic already does something similar
> > to this step and calls the factored-out function eat_long_magic()).
> >
> > My gut feeling is that we probably should ask Stefan's series to be
> > rebased on top of this series that cleans up pathspec implementation,
> > once it stabilizes.
> 
> Very much so.
> 
> Jonathan Nieder mentioned off list that he prefers to see that
> series rerolled without mutexes if possible. That is possible by
> creating the questions "struct attr_check" before preloading the
> index and then using the read only questions in the threaded code,
> to obtain answers fast; also no need for a mutex.
> 
> I did not look into that yet, though. So I think you could discard that
> series (again) until I find time to either redo the series or
> resend it with a proper explanation on why the approach above
> is not feasible.
> 
> >  We could probably go the other way around, but
> > logically it makes more sense to build "pathspec can also match
> > using attributes information" on top of a refactored codebase.
> >
> > Thoughts?
> 
> Please let the refactoring in in favor of the attr series.

Sounds good.  I only looked at your series briefly, but I'm hoping that
these refactoring changes I'm proposing make it relatively easy for you
to build on top of in the future.

-- 
Brandon Williams

Re: [PATCH 2/3] difftool: chdir as early as possible

[David Aguilar]

On Fri, Dec 09, 2016 at 03:02:09PM -0800, Junio C Hamano wrote:
> David Aguilar  writes:
> 
> > @@ -182,10 +188,6 @@ EOF
> > }
> > }
> >  
> > -   # Go to the root of the worktree so that the left index files
> > -   # are properly setup -- the index is toplevel-relative.
> > -   chdir($workdir);
> > -
> > # Setup temp directories
> > my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
> > my $ldir = "$tmpdir/left";
> 
> What codebase are you basing your work on?  I do not see these
> removed four lines in my tree, so it seems that the patch is fixing
> up some other fix I do not yet have.

Sorry about that.

I forgot to mention that this is based on the patches I recently
sent, which were in the "pu" branch.  The whats-cooking report
mentioned that they'll be merged to "next", so they might be
there already too.

The patch this was based upon:

difftool: fix dir-diff index creation when in a subdirectory
-- 
David

Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email

[Stefan Beller]

On Fri, Dec 9, 2016 at 3:52 PM, Junio C Hamano  wrote:
> Stefan Beller  writes:
>
>> So you are suggesting to
>> * have the check later in the game (e.g. just after asking
>>"Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>>   such as additional @to @cc are available.
>
> Yeah, probably before the loop starts asking that question for each
> message.  And hook does not necessarily need to cause the program to
> die.  The question can be reworded to "Your hook says no, but do you
> really want to send it?",

You could, but that would be inconsistent with the "*** SUBJECT ***"
treatment, which currently dies. That could also ask "do you really want
to send out an unfinished series" and continue if the user wants.

I assume we want to be consistent with the existing UI and just ask the
user to use force instead?

Re: [PATCH v2 12/16] pathspec: create parse_long_magic function

[Stefan Beller]

On Fri, Dec 9, 2016 at 3:44 PM, Junio C Hamano  wrote:
> Brandon Williams  writes:
>
>> Factor out the logic responsible for parsing long magic into its own
>> function.  As well as hoist the prefix check logic outside of the inner
>> loop as there isn't anything that needs to be done after matching
>> "prefix:".
>>
>> Signed-off-by: Brandon Williams 
>
> These refactoring changes look like they are all going in the good
> direction.  Stefan's :(attr:)path" changes however
> have severe conflicts (e.g. the topic already does something similar
> to this step and calls the factored-out function eat_long_magic()).
>
> My gut feeling is that we probably should ask Stefan's series to be
> rebased on top of this series that cleans up pathspec implementation,
> once it stabilizes.

Very much so.

Jonathan Nieder mentioned off list that he prefers to see that
series rerolled without mutexes if possible. That is possible by
creating the questions "struct attr_check" before preloading the
index and then using the read only questions in the threaded code,
to obtain answers fast; also no need for a mutex.

I did not look into that yet, though. So I think you could discard that
series (again) until I find time to either redo the series or
resend it with a proper explanation on why the approach above
is not feasible.

>  We could probably go the other way around, but
> logically it makes more sense to build "pathspec can also match
> using attributes information" on top of a refactored codebase.
>
> Thoughts?

Please let the refactoring in in favor of the attr series.

Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email

[Junio C Hamano]

Stefan Beller  writes:

> So you are suggesting to
> * have the check later in the game (e.g. just after asking
>"Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
>   such as additional @to @cc are available.

Yeah, probably before the loop starts asking that question for each
message.  And hook does not necessarily need to cause the program to
die.  The question can be reworded to "Your hook says no, but do you
really want to send it?",

Re: [PATCH v2 12/16] pathspec: create parse_long_magic function

[Junio C Hamano]

Brandon Williams  writes:

> Factor out the logic responsible for parsing long magic into its own
> function.  As well as hoist the prefix check logic outside of the inner
> loop as there isn't anything that needs to be done after matching
> "prefix:".
>
> Signed-off-by: Brandon Williams 

These refactoring changes look like they are all going in the good
direction.  Stefan's :(attr:)path" changes however
have severe conflicts (e.g. the topic already does something similar
to this step and calls the factored-out function eat_long_magic()).

My gut feeling is that we probably should ask Stefan's series to be
rebased on top of this series that cleans up pathspec implementation,
once it stabilizes.  We could probably go the other way around, but
logically it makes more sense to build "pathspec can also match
using attributes information" on top of a refactored codebase.

Thoughts?

Re: [PATCH 0/4] doc: fixes to gitcore-tutorial.txt

[Junio C Hamano]

Kristoffer Haugsbakk  writes:

> This series of patches attempts to fix some minor mistakes in
> gitcore-tutorial.txt that I found while reading it.  They are all
> concerned with grammar and things like accidentally omitted words.

Grammar is not my forte, so even though I'll queue them as-is
because I didn't spot anything wrong in them and thought they are
all improvements, I'd appreciate somebody else to lend an eye or two
over them.

Thanks.

Re: [PATCH] describe: add tests for unusual graphs

[Junio C Hamano]

Quinn Grier  writes:

> git describe may give incorrect results if there are backdated commits
> or multiple roots. This commit adds two test_expect_failure tests that
> demonstrate these problems.

I am not sure if this is a good patch to take.  test_expect_failure
is to demonstrate an incorrect behaviour that we wish to correct
later, but I do not think these demonstrate incorrect behaviours to
begin with.

For example, the latter one seems to expect that by asking to
describe D in this picture

> +#
> +# A---B*--D master
> +#/
> +#   C* other
> +#

you seem to expect the description is based on B.  

It is not at all clear why it is incorrect if the description were
made based on C.  If D were described relative to A, ignoring B,
then I understand why that result is incorrect and I would agree
that describing D in terms of B is more correct.  But I do not think
that is what the test is trying to demonstrate.

But it is hard to guess only from looking at the test and the
proposed log message, because it does not say what makes you think
the behaviour you saw was incorrect.

Re: [PATCHv6 4/7] worktree: get worktrees from submodules

[Stefan Beller]

On Fri, Dec 9, 2016 at 3:00 PM, Brandon Williams  wrote:
> On 12/08, Stefan Beller wrote:
>> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen  wrote:
>> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller  wrote:
>> >>
>> >> worktree = xcalloc(1, sizeof(*worktree));
>> >> worktree->path = strbuf_detach(_path, NULL);
>> >> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
>> >
>> > All the good stuff is outside context lines again.. Somewhere between
>> > here we call add_head_info() which calls resolve_ref_unsafe(), which
>> > always uses data from current repo, not the submodule we want it to
>> > look at.
>>
>> Unrelated side question: What would you think of "variable context line
>> configuration" ? e.g. you could configure it to include anything from
>> up that line
>> that is currently shown after the @@ which is the function signature line.
>>
>> As to the add_head_info/resolve_ref_unsafe what impact does that have?
>> It produces a wrong head info but AFAICT it will never die(), such that for 
>> the
>> purposes of this series (which only wants to know if a submodule uses the
>> worktree feature) it should be fine.
>>
>> It is highly misleading though for others to build upon this.
>> So maybe I'll only add the functionality internally in worktree.c
>> and document why the values are wrong, and only expose the
>> "int submodule_uses_worktrees(const char *path)" ?
>>
>> >> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
>> >> return list;
>> >
>> > Right before this line is mark_current_worktree(), which in turn calls
>> > get_git_dir() and not suitable for peeking into another repository the
>> > way submodule code does. get_worktree_git_dir() called within that
>> > function shares the same problem.
>>
>> It actually works correctly: "No submodule is the current worktree".
>>
>>
>> >
>> >>  }
>> >>
>> >> +struct worktree **get_worktrees(unsigned flags)
>> >> +{
>> >> +   return get_worktrees_internal(get_git_common_dir(), flags);
>> >> +}
>> >> +
>> >> +struct worktree **get_submodule_worktrees(const char *path, unsigned 
>> >> flags)
>> >> +{
>> >> +   char *submodule_gitdir;
>> >> +   struct strbuf sb = STRBUF_INIT;
>> >> +   struct worktree **ret;
>> >> +
>> >> +   submodule_gitdir = git_pathdup_submodule(path, "%s", "");
>> >> +   if (!submodule_gitdir)
>> >> +   return NULL;
>> >> +
>> >> +   /* the env would be set for the superproject */
>> >> +   get_common_dir_noenv(, submodule_gitdir);
>> >
>> > Technically we need to read submodule_gitdir/.config and see if we can
>> > understand core.repositoryformatversion, or find any unrecognized
>> > extensions. But the problem is not specific to this code. And fixing
>> > it is no small task. But perhaps we could call a dummy
>> > validate_submodule_gitdir() here? Then when we implement that function
>> > for real, we don't have to search the entire code base to see where to
>> > put it.
>> >
>> > Kinda off-topic though. Feel free to ignore the above comment.
>>
>> ok I'll add a TODO/emptyfunction for that.
>
> So is using resolve_gitdir not ok when trying to see if a submodule has
> a gitdir at a particular path?
>

Well it depends on the question that you are trying to answer:

Assume we introduce a new repository format in Git version 3.0.
By some means the submodule repository is converted to the new format,
but the superproject is not. (e.g. it is auto migrating repositories that have
a lot of large blobs/files)

Now for some reason you use the older git 2.X in the superproject.
(e.g. the code is on a shared network mount, or git3 has a bug, or ...)

Then the code above may tell that the submodule doesn't use worktrees
(as we cannot make any assumptions on the new crazy repository format),
but in fact it is, so in this case we technically we would need to:

1) read the config
2) check the repository format version (if larger than we know of,
   assume the worst or just die?)

As the function in this patch is used for safe guarding we may want to be
extra cautious, another function that is using resolve_gitdir may have other
assumptions on what is ok even for newer repository formats.

Re: [PATCH 2/3] difftool: chdir as early as possible

[Junio C Hamano]

David Aguilar  writes:

> @@ -182,10 +188,6 @@ EOF
>   }
>   }
>  
> - # Go to the root of the worktree so that the left index files
> - # are properly setup -- the index is toplevel-relative.
> - chdir($workdir);
> -
>   # Setup temp directories
>   my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
>   my $ldir = "$tmpdir/left";

What codebase are you basing your work on?  I do not see these
removed four lines in my tree, so it seems that the patch is fixing
up some other fix I do not yet have.

> @@ -235,10 +237,10 @@ EOF
>   symlink("$workdir/$file", "$rdir/$file") or
>   exit_cleanup($tmpdir, 1);
>   } else {
> - copy("$workdir/$file", "$rdir/$file") or
> + copy($file, "$rdir/$file") or
>   exit_cleanup($tmpdir, 1);
>  
> - my $mode = stat("$workdir/$file")->mode;
> + my $mode = stat($file)->mode;
>   chmod($mode, "$rdir/$file") or
>   exit_cleanup($tmpdir, 1);
>   }
> @@ -430,10 +432,10 @@ sub dir_diff
>   $error = 1;
>   } elsif (exists $tmp_modified{$file}) {
>   my $mode = stat("$b/$file")->mode;
> - copy("$b/$file", "$workdir/$file") or
> + copy("$b/$file", $file) or
>   exit_cleanup($tmpdir, 1);
>  
> - chmod($mode, "$workdir/$file") or
> + chmod($mode, $file) or
>   exit_cleanup($tmpdir, 1);
>   }
>   }

Re: [PATCHv6 4/7] worktree: get worktrees from submodules

[Brandon Williams]

On 12/08, Stefan Beller wrote:
> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen  wrote:
> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller  wrote:
> >>
> >> worktree = xcalloc(1, sizeof(*worktree));
> >> worktree->path = strbuf_detach(_path, NULL);
> >> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
> >
> > All the good stuff is outside context lines again.. Somewhere between
> > here we call add_head_info() which calls resolve_ref_unsafe(), which
> > always uses data from current repo, not the submodule we want it to
> > look at.
> 
> Unrelated side question: What would you think of "variable context line
> configuration" ? e.g. you could configure it to include anything from
> up that line
> that is currently shown after the @@ which is the function signature line.
> 
> As to the add_head_info/resolve_ref_unsafe what impact does that have?
> It produces a wrong head info but AFAICT it will never die(), such that for 
> the
> purposes of this series (which only wants to know if a submodule uses the
> worktree feature) it should be fine.
> 
> It is highly misleading though for others to build upon this.
> So maybe I'll only add the functionality internally in worktree.c
> and document why the values are wrong, and only expose the
> "int submodule_uses_worktrees(const char *path)" ?
> 
> >> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)
> >> return list;
> >
> > Right before this line is mark_current_worktree(), which in turn calls
> > get_git_dir() and not suitable for peeking into another repository the
> > way submodule code does. get_worktree_git_dir() called within that
> > function shares the same problem.
> 
> It actually works correctly: "No submodule is the current worktree".
> 
> 
> >
> >>  }
> >>
> >> +struct worktree **get_worktrees(unsigned flags)
> >> +{
> >> +   return get_worktrees_internal(get_git_common_dir(), flags);
> >> +}
> >> +
> >> +struct worktree **get_submodule_worktrees(const char *path, unsigned 
> >> flags)
> >> +{
> >> +   char *submodule_gitdir;
> >> +   struct strbuf sb = STRBUF_INIT;
> >> +   struct worktree **ret;
> >> +
> >> +   submodule_gitdir = git_pathdup_submodule(path, "%s", "");
> >> +   if (!submodule_gitdir)
> >> +   return NULL;
> >> +
> >> +   /* the env would be set for the superproject */
> >> +   get_common_dir_noenv(, submodule_gitdir);
> >
> > Technically we need to read submodule_gitdir/.config and see if we can
> > understand core.repositoryformatversion, or find any unrecognized
> > extensions. But the problem is not specific to this code. And fixing
> > it is no small task. But perhaps we could call a dummy
> > validate_submodule_gitdir() here? Then when we implement that function
> > for real, we don't have to search the entire code base to see where to
> > put it.
> >
> > Kinda off-topic though. Feel free to ignore the above comment.
> 
> ok I'll add a TODO/emptyfunction for that.

So is using resolve_gitdir not ok when trying to see if a submodule has
a gitdir at a particular path?

-- 
Brandon Williams

Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email

[Stefan Beller]

On Fri, Dec 9, 2016 at 2:36 PM, Junio C Hamano  wrote:
>
> I doubt that this is the best place to call this hook, because the
> called hook does not have access to information that may help it
> make a better decision.

As the commit message may elude, I chose this place as it would be
sufficient for checking for ChangeIds, missing signoffs, or even
rudimentary check for coding style and commit message line length.

>
> For example, because the hook gets one patchfile at a time, it does
> not have the entire picture (e.g. "are you sure you want 01/05,
> 02/05, 04/05 and 05/05 without 03/05?").  For another example, the
> hook does not have access to the decision git-send-email makes on
> various "parameters", which are computed based on the contents of
> the patchfiles and command line arguments at this point in the code.
> (e.g. @to, @cc, etc. are computed much later, so you cannot say "do
> not send anythnng outside corp by mistake" with this mechanism).
>

So you are suggesting to
* have the check later in the game (e.g. just after asking
   "Send this email? ([y]es|[n]o|[q]uit|[a]ll): " as then other information
  such as additional @to @cc are available.
* the hook should not just be called one file at a time, but rather
  we would give all file names via e.g. stdin. With the current code
  structure this contradicts the first point.

I wonder if we want to have multiple hooks for these different things
of either looking at the big picture or looking at each in detail.

For me currently I am only interested in the small picture thing.

Stefan

Re: [RFC PATCH] send-email: allow a custom hook to prevent sending email

[Junio C Hamano]

Stefan Beller  writes:

> This custom hook could be used to prevent sending out e.g. patches
> with change ids or other information that upstream doesn't like to see
> or is not supposed to see.
>
> Signed-off-by: Stefan Beller 
> ---
>
> My first perl contribution to Git. :)
>
> Marked as RFC to gauge general interest before writing tests and 
> documentation.
>
> Thanks,
> Stefan
>
>  git-send-email.perl | 9 +
>  1 file changed, 9 insertions(+)
>
> diff --git a/git-send-email.perl b/git-send-email.perl
> index da81be40cb..d3ebf666c3 100755
> --- a/git-send-email.perl
> +++ b/git-send-email.perl
> @@ -815,6 +815,15 @@ if (!$force) {
>   . "Pass --force if you really want to send.\n";
>   }
>   }
> + my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )
> + if( -x $hook[0] ) {
> + unless( system( @hook ) == 0 )
> + {
> + die "Refusing to send because the patch\n\t$f\n"
> + . "was refused by the send-email hook."
> + . "Pass --force if you really want to send.\n";
> + }
> + }
>  }

I doubt that this is the best place to call this hook, because the
called hook does not have access to information that may help it
make a better decision.  

For example, because the hook gets one patchfile at a time, it does
not have the entire picture (e.g. "are you sure you want 01/05,
02/05, 04/05 and 05/05 without 03/05?").  For another example, the
hook does not have access to the decision git-send-email makes on
various "parameters", which are computed based on the contents of
the patchfiles and command line arguments at this point in the code.
(e.g. @to, @cc, etc. are computed much later, so you cannot say "do
not send anythnng outside corp by mistake" with this mechanism).

Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines

[Junio C Hamano]

Johannes Schindelin  writes:

> Hi Vasco,
>
> On Fri, 9 Dec 2016, Vasco Almeida wrote:
>
>> A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
>> > The incremental update below looks sensible. We'd also want to
>> > protect this codepath from a misconfigured two-or-more byte sequence
>> > in core.commentchar, I would suspect, to be consistent.
>> 
>> Are the below changes alright for what you propose? It just checks if
>> the length of core.commentchar's value is 1, otherwise use '#' as the
>> comment_line_char.
>> As a note, when I set core.commentchar with "git config
>> core.commentChar 'batata'", I get the following error message when I
>> issue "git add -i":
>> 
>> error: core.commentChar should only be one character
>> fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6
>
> This is exactly the same issue I fixed for rebase -i recently.

Yes, but the patch we see here punts "core.commentChar is not a
single-byte single-letter--panic!" case differently.  I think you
did "just take the first one" in "rebase -i", which I think is more
in line with the rest of the system, and this addition to Git.pm
should do the same, I think.

Re: [REGRESSION 2.10.2] problematic "empty auth" changes

[brian m. carlson]

On Thu, Dec 08, 2016 at 04:12:32PM -0500, David Turner wrote:
> I know of no reason that shouldn't work.  Indeed, it's what we use do
> internally.  So far, nobody has reported problems.  That said, we have
> exactly three sets of git servers that most users talk to (two different
> internal; and occasionally github.com for external stuff).  So our
> coverage is not very broad.
> 
> If you're going to do it, tho, don't just do it for Windows users -- do
> it for everyone.  Plenty of Unix clients connect to Windows-based auth
> systems.

Let me echo this.  This would make Kerberos (and probably other forms of
SPNEGO) work out of the box, which would reduce a lot of confusion that
people have.

I can confirm enabling http.emptyAuth works properly with Kerberos,
including with fallback to Basic, so I see no reason why we shouldn't do
it.
-- 
brian m. carlson / brian with sandals: Houston, Texas, US
+1 832 623 2791 | https://www.crustytoothpaste.net/~bmc | My opinion only
OpenPGP: https://keybase.io/bk2204


signature.asc
Description: PGP signature

Re: [BUG] Colon in remote urls

[Johannes Sixt]

Am 09.12.2016 um 16:22 schrieb Jeff King:

+const char *parse_alt_odb_entry(const char *string, int sep,
+   struct strbuf *out)
+{
+   const char *p;
+   int literal = 0;
+
+   strbuf_reset(out);
+
+   for (p = string; *p; p++) {
+   if (literal) {
+   strbuf_addch(out, *p);
+   literal = 0;
+   } else {
+   if (*p == '\\')
+   literal = 1;


There are too many systems out there that use a backslash in path names. 
I don't think it is wise to use it also as the quoting character.



+   else if (*p == sep)
+   return p + 1;
+   else
+   strbuf_addch(out, *p);
+   }
+   }
+   return p;
+}


-- Hannes

[RFC PATCH] send-email: allow a custom hook to prevent sending email

[Stefan Beller]

This custom hook could be used to prevent sending out e.g. patches
with change ids or other information that upstream doesn't like to see
or is not supposed to see.

Signed-off-by: Stefan Beller 
---

My first perl contribution to Git. :)

Marked as RFC to gauge general interest before writing tests and documentation.

Thanks,
Stefan

 git-send-email.perl | 9 +
 1 file changed, 9 insertions(+)

diff --git a/git-send-email.perl b/git-send-email.perl
index da81be40cb..d3ebf666c3 100755
--- a/git-send-email.perl
+++ b/git-send-email.perl
@@ -815,6 +815,15 @@ if (!$force) {
. "Pass --force if you really want to send.\n";
}
}
+   my @hook = ( $ENV{GIT_DIR}.'hooks/send-email', $f )
+   if( -x $hook[0] ) {
+   unless( system( @hook ) == 0 )
+   {
+   die "Refusing to send because the patch\n\t$f\n"
+   . "was refused by the send-email hook."
+   . "Pass --force if you really want to send.\n";
+   }
+   }
 }
 
 if (defined $sender) {
-- 
2.11.0.rc2.49.ge1f3b0c.dirty

Mr. Mathiang Puk

[Mathiang Puk]

My Dear Friend,

How are you and your family? I hope you all are fine

I need your urgent assistance in transferring the sum of Eight Million United 
States Dollars ($8,000,000:00) into your account within 14 working banking days.

I don't want the money to go into our bank treasury as an abandoned fund. So 
this is the reason why I contacted you so that our bank will know you and 
release this 

money to you as the next of kin to the deceased  customer.

I am looking forward to your response.

My regards,
Mr. Mathiang Puk

Re: [PATCH 14/16] pathspec: create strip submodule slash helpers

[Brandon Williams]

On 12/09, Stefan Beller wrote:
> On Fri, Dec 9, 2016 at 11:18 AM, Brandon Williams  wrote:
> > Factor out the logic responsible for stripping the trailing slash on
> > pathspecs referencing submodules into its own function.
> >
> > Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4
> 
> I think we should come up with a solution to wipe out change ids
> before sending emails. ;)

Darn! Yeah maybe a hook or something :D

-- 
Brandon Williams

Re: [PATCH v2 1/4] real_path: resolve symlinks by hand

[Brandon Williams]

On 12/09, Johannes Sixt wrote:
> Am 09.12.2016 um 00:58 schrieb Brandon Williams:
> >The current implementation of real_path uses chdir() in order to resolve
> >symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
> >process as a whole and not just an individual thread.  Instead perform
> >the symlink resolution by hand so that the calls to chdir() can be
> >removed, making real_path one step closer to being reentrant.
> >
> >Signed-off-by: Brandon Williams 
> >---
> > abspath.c | 183 
> > +-
> > 1 file changed, 122 insertions(+), 61 deletions(-)
> >
> >diff --git a/abspath.c b/abspath.c
> >index 2825de8..92f2a29 100644
> >--- a/abspath.c
> >+++ b/abspath.c
> >@@ -11,8 +11,38 @@ int is_directory(const char *path)
> > return (!stat(path, ) && S_ISDIR(st.st_mode));
> > }
> >
> >+/* removes the last path component from 'path' except if 'path' is root */
> >+static void strip_last_component(struct strbuf *path)
> >+{
> >+if (path->len > offset_1st_component(path->buf)) {
> >+char *last_slash = find_last_dir_sep(path->buf);
> >+strbuf_setlen(path, last_slash - path->buf);
> >+}
> >+}
> 
> This implementation is not correct because when the input is "/foo",
> the result is "" when it should be "/". Also, can the input be a
> non-normalized path? When the input is "foo///bar", should the
> result be "foo" or would "foo//" be an acceptable result? I think it
> should be the former. find_last_dir_sep() returns the last of the
> three slashes, not the first one. Therefore, I've rewritten the
> function thusly:
> 
> static void strip_last_component(struct strbuf *path)
> {
>   size_t offset = offset_1st_component(path->buf);
>   size_t len = path->len;
>   while (offset < len && !is_dir_sep(path->buf[len - 1]))
>   len--;
>   while (offset < len && is_dir_sep(path->buf[len - 1]))
>   len--;
>   strbuf_setlen(path, len);
> }
> 

Thanks for that catch.  So your rewrite takes the offset of the 1st
component and ensures that we don't cut that part off.  It first strips
all non directory separators and then all directory separators.  This
way "/foobar" becomes "/foo" and as you pointed out "/foo" would
become "/".  The offset would also take care of windows drive letters
and the like.  Looks good.  Thanks!

> >+strbuf_addbuf(, );
> >+
> >+if (lstat(resolved.buf, )) {
> >+/* error out unless this was the last component */
> >+if (!(errno == ENOENT && !remaining.len)) {
> 
> Perhaps it was easier to _write_ the condition in this way, but I
> would have an easier time to _read_ it when it is
> 
>   if (errno != ENOENT || remaining.len) {
> 

Yes I did write it out weird, mostly because it made the most sense for
what I was trying to accomplish (add path components must exist, except
for the very last one).  I'm fine applying DeMorgan's since it looks a
little cleaner.

> >+
> >+if (is_absolute_path(symlink.buf)) {
> >+/* absolute symlink; set resolved to root */
> >+int offset = offset_1st_component(symlink.buf);
> >+strbuf_reset();
> >+strbuf_add(, symlink.buf, offset);
> >+strbuf_remove(, 0, offset);
> 
> Good. I would have expected some kind of "goto repeat;" because when
> we encounter a symlink to an absolute path, most, if not all, work
> done so far is obsoleted. But I haven't thought too deeply about
> this.

It made the most sense to just reuse the same looping condition that
I already had in place.  Resetting the resolved string to be the root
component of the absolute symlink made it easy to "throw away" all the
old work to allow us to start from scratch again.

-- 
Brandon Williams

Re: [PATCH v2 0/4] road to reentrant real_path

[Brandon Williams]

On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams  wrote:
> > diff --git a/setup.c b/setup.c
> > index fe572b8..0d9fdd0 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const 
> > char *gitdir)
> > if (!is_absolute_path(data.buf))
> > strbuf_addf(, "%s/", gitdir);
> > strbuf_addbuf(, );
> > -   strbuf_addstr(sb, real_path(path.buf));
> > +   strbuf_realpath(sb, path.buf, 1);
> 
> This is not the same because of this hunk in strbuf_realpath()

Then perhaps I shouldn't make this change (and just leave it as is)
since the way real_path_internal/strbuf_realpath is written requires
that the strbuf being used for the resolved path only contains the
resolved path (see the lstat(resolved->buf ) call).  Sidenote it
looks like strbuf_getcwd() also does a reset, though more subtlety,
since it just passes its buffer to getcwd().

> 
> > @@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, 
> > int die_on_error)
> > goto error_out;
> > }
> >
> > -   strbuf_reset();
> > +   strbuf_reset(resolved);
> >
> > if (is_absolute_path(path)) {
> 
> But if you you remove that, then all (old) callers of
> strbuf_realpath() must do a strbuf_reset() in advance if needed
> (probably just real_path does) which sounds reasonable to me. You're
> probably want to be careful about the strbuf_reset() at the end of the
> function too.
> 
> Other than that, I think this diff looks nice.
> -- 
> Duy

-- 
Brandon Williams

Re: [PATCH 14/16] pathspec: create strip submodule slash helpers

[Stefan Beller]

On Fri, Dec 9, 2016 at 11:18 AM, Brandon Williams  wrote:
> Factor out the logic responsible for stripping the trailing slash on
> pathspecs referencing submodules into its own function.
>
> Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4

I think we should come up with a solution to wipe out change ids
before sending emails. ;)

> Signed-off-by: Brandon Williams 
> ---
>  pathspec.c | 68 
> ++
>  1 file changed, 42 insertions(+), 26 deletions(-)
>
> diff --git a/pathspec.c b/pathspec.c
> index 84a57cf..4d9a6a0 100644
> --- a/pathspec.c
> +++ b/pathspec.c
> @@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, 
> int *prefix_len,
> return parse_short_magic(magic, elem);
>  }
>
> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
> +{
> +   if (item->len >= 1 && item->match[item->len - 1] == '/') {
> +   int i = cache_name_pos(item->match, item->len - 1);
> +
> +   if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
> +   item->len--;
> +   item->match[item->len] = '\0';
> +   }
> +   }
> +}
> +
> +static void strip_submodule_slash_expensive(struct pathspec_item *item)
> +{
> +   int i;
> +
> +   for (i = 0; i < active_nr; i++) {
> +   struct cache_entry *ce = active_cache[i];
> +   int ce_len = ce_namelen(ce);
> +
> +   if (!S_ISGITLINK(ce->ce_mode))
> +   continue;
> +
> +   if (item->len <= ce_len || item->match[ce_len] != '/' ||
> +   memcmp(ce->name, item->match, ce_len))
> +   continue;
> +
> +   if (item->len == ce_len + 1) {
> +   /* strip trailing slash */
> +   item->len--;
> +   item->match[item->len] = '\0';
> +   } else {
> +   die(_("Pathspec '%s' is in submodule '%.*s'"),
> +   item->original, ce_len, ce->name);
> +   }
> +   }
> +}
> +
>  /*
>   * Take an element of a pathspec and check for magic signatures.
>   * Append the result to the prefix. Return the magic bitmap.
> @@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item, unsigned flags,
> unsigned magic = 0, element_magic = 0;
> const char *copyfrom = elt;
> char *match;
> -   int i, pathspec_prefix = -1;
> +   int pathspec_prefix = -1;
>
> /* PATHSPEC_LITERAL_PATH ignores magic */
> if (flags & PATHSPEC_LITERAL_PATH) {
> @@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item 
> *item, unsigned flags,
> item->len = strlen(item->match);
> item->prefix = prefixlen;
>
> -   if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
> -   (item->len >= 1 && item->match[item->len - 1] == '/') &&
> -   (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
> -   S_ISGITLINK(active_cache[i]->ce_mode)) {
> -   item->len--;
> -   match[item->len] = '\0';
> -   }
> +   if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
> +   strip_submodule_slash_cheap(item);
>
> if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
> -   for (i = 0; i < active_nr; i++) {
> -   struct cache_entry *ce = active_cache[i];
> -   int ce_len = ce_namelen(ce);
> -
> -   if (!S_ISGITLINK(ce->ce_mode))
> -   continue;
> -
> -   if (item->len <= ce_len || match[ce_len] != '/' ||
> -   memcmp(ce->name, match, ce_len))
> -   continue;
> -   if (item->len == ce_len + 1) {
> -   /* strip trailing slash */
> -   item->len--;
> -   match[item->len] = '\0';
> -   } else
> -   die (_("Pathspec '%s' is in submodule 
> '%.*s'"),
> -elt, ce_len, ce->name);
> -   }
> +   strip_submodule_slash_expensive(item);
>
> if (magic & PATHSPEC_LITERAL)
> item->nowildcard_len = item->len;
> --
> 2.8.0.rc3.226.g39d4020
>

Re: [REGRESSION 2.10.2] problematic "empty auth" changes

[Junio C Hamano]

Johannes Schindelin  writes:

> It would be different, of course, if http.emptyAuth would *not* allow the
> user to type their credentials when accessing something like
> https://github.com/dscho/shhh-secret-repository, *only* trying the login
> credentials. But that is not the case, with http.emptyAuth=true, login
> credentials are attempted first, and when they fail, the user is still
> asked interactively for their credentials.
>
> All I can see is that this would be *an improvement*: corporate users
> trying to access a Git repository that requires their login credentials
> would now not even need to enter empty user name/password.

Yup, my thought process after seeing your first message to David
exactly mirrored the above two paragraphs.  It sounds like you two
have a good plan ;-)

Thanks.

> This alone would be already a good reason to change the default, IMHO.
>
> So here is my plan:
>
> - change the default of http.emptyAuth to true in the next Git for Windows
>   version
>
> - publish a prerelease for early adopters to test
>
> - contribute this patch here on the Git mailing list, in the hope that it
>   will make it into the next major version
>
> Ciao,
> Dscho

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

On 12/09/2016 08:24 PM, Stephan Beyer wrote:
> t3510 also shows another use-case for --quit: the title says it all:
> "cherry-pick --quit" to "cherry-pick --abort"

I should've read what I actually pasted.
I wanted to paste: '--quit keeps HEAD and conflicted index intact'

Sorry for making no sense ;)

> With this additional information, I'd vote to keep --quit/--forget and
> just make it consistent.

Now!

~Stephan

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Stephan Beyer]

Hi Junio,

On 12/09/2016 07:07 PM, Junio C Hamano wrote:
> Duy Nguyen  writes:
>> Having the same operation with different names only increases git
>> reputation of bad/inconsistent UI. Either forget is renamed to quit,
>> or vice versa. I prefer forget, but the decision is yours and the
>> community's. So I'm sending two patches to rename in either direction.
>> You can pick one.
> 
> I actually was advocating to remove both by making --abort saner.
> With an updated --abort that behaves saner, is "rebase --forget"
> still necessary?

A quick change in t3407 of the "rebase --forget" test to use "rebase
--abort" failed.  That's because it checks the use-case of
forgetting/aborting without changing the HEAD.  So --abort makes a
rollback, --forget just keeps the current head.  I am not sure if that
tested use-case is a real use-case though.

A quick change in the pristine_detach function in t3510 and t3511 from
"cherry-pick --quit" to "cherry-pick --abort" works when one ignores the
return value of "cherry-pick --abort". The "--quit" is used here to
ensure a clean cherry-pick state, and --quit always succeeds, even if no
cherry-pick is in progress.  That may be a real use-case somehow that
could also be used for "rebase --forget"

t3510 also shows another use-case for --quit: the title says it all:
"cherry-pick --quit" to "cherry-pick --abort"

With this additional information, I'd vote to keep --quit/--forget and
just make it consistent.

~Stephan

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Brandon Williams]

On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams  wrote:
> > On 12/08, Duy Nguyen wrote:
> >> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams  wrote:
> >> > On 12/07, Duy Nguyen wrote:
> >> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  
> >> >> wrote:
> >> >> > Convert 'create_simplify()' to use the pathspec struct interface from
> >> >> > using the '_raw' entry in the pathspec.
> >> >>
> >> >> It would be even better to kill this create_simplify() and let
> >> >> simplify_away() handle struct pathspec directly.
> >> >>
> >> >> There is a bug in this code, that might have been found if we
> >> >> simpify_away() handled pathspec directly: the memcmp() in
> >> >> simplify_away() will not play well with :(icase) magic. My bad. If
> >> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
> >> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
> >> >> ignore exclude patterns there too (although not excluding is not a
> >> >> bug).
> >> >
> >> > So are you implying that the simplify struct needs to be killed?  That
> >> > way the pathspec struct itself is being passed around instead?
> >>
> >> Yes. simplify struct was a thing when pathspec was an array of char *.
> >> At this point I think it can retire (when we have time to retire it)
> >
> > Alright, then for now I can leave this change as is and have a follow up
> > series that kills the simplify struct.
> 
> Do let me know if you decide to drop it, so I can put it back in my backlog.

K will do

-- 
Brandon Williams

[PATCH 14/16] pathspec: create strip submodule slash helpers

[Brandon Williams]

Factor out the logic responsible for stripping the trailing slash on
pathspecs referencing submodules into its own function.

Change-Id: Icad62647c04b4195309def0e3db416203d14f9e4
Signed-off-by: Brandon Williams 
---
 pathspec.c | 68 ++
 1 file changed, 42 insertions(+), 26 deletions(-)

diff --git a/pathspec.c b/pathspec.c
index 84a57cf..4d9a6a0 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -258,6 +258,44 @@ static const char *parse_element_magic(unsigned *magic, 
int *prefix_len,
return parse_short_magic(magic, elem);
 }
 
+static void strip_submodule_slash_cheap(struct pathspec_item *item)
+{
+   if (item->len >= 1 && item->match[item->len - 1] == '/') {
+   int i = cache_name_pos(item->match, item->len - 1);
+
+   if (i >= 0 && S_ISGITLINK(active_cache[i]->ce_mode)) {
+   item->len--;
+   item->match[item->len] = '\0';
+   }
+   }
+}
+
+static void strip_submodule_slash_expensive(struct pathspec_item *item)
+{
+   int i;
+
+   for (i = 0; i < active_nr; i++) {
+   struct cache_entry *ce = active_cache[i];
+   int ce_len = ce_namelen(ce);
+
+   if (!S_ISGITLINK(ce->ce_mode))
+   continue;
+
+   if (item->len <= ce_len || item->match[ce_len] != '/' ||
+   memcmp(ce->name, item->match, ce_len))
+   continue;
+
+   if (item->len == ce_len + 1) {
+   /* strip trailing slash */
+   item->len--;
+   item->match[item->len] = '\0';
+   } else {
+   die(_("Pathspec '%s' is in submodule '%.*s'"),
+   item->original, ce_len, ce->name);
+   }
+   }
+}
+
 /*
  * Take an element of a pathspec and check for magic signatures.
  * Append the result to the prefix. Return the magic bitmap.
@@ -278,7 +316,7 @@ static unsigned prefix_pathspec(struct pathspec_item *item, 
unsigned flags,
unsigned magic = 0, element_magic = 0;
const char *copyfrom = elt;
char *match;
-   int i, pathspec_prefix = -1;
+   int pathspec_prefix = -1;
 
/* PATHSPEC_LITERAL_PATH ignores magic */
if (flags & PATHSPEC_LITERAL_PATH) {
@@ -329,33 +367,11 @@ static unsigned prefix_pathspec(struct pathspec_item 
*item, unsigned flags,
item->len = strlen(item->match);
item->prefix = prefixlen;
 
-   if ((flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP) &&
-   (item->len >= 1 && item->match[item->len - 1] == '/') &&
-   (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
-   S_ISGITLINK(active_cache[i]->ce_mode)) {
-   item->len--;
-   match[item->len] = '\0';
-   }
+   if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP)
+   strip_submodule_slash_cheap(item);
 
if (flags & PATHSPEC_STRIP_SUBMODULE_SLASH_EXPENSIVE)
-   for (i = 0; i < active_nr; i++) {
-   struct cache_entry *ce = active_cache[i];
-   int ce_len = ce_namelen(ce);
-
-   if (!S_ISGITLINK(ce->ce_mode))
-   continue;
-
-   if (item->len <= ce_len || match[ce_len] != '/' ||
-   memcmp(ce->name, match, ce_len))
-   continue;
-   if (item->len == ce_len + 1) {
-   /* strip trailing slash */
-   item->len--;
-   match[item->len] = '\0';
-   } else
-   die (_("Pathspec '%s' is in submodule '%.*s'"),
-elt, ce_len, ce->name);
-   }
+   strip_submodule_slash_expensive(item);
 
if (magic & PATHSPEC_LITERAL)
item->nowildcard_len = item->len;
-- 
2.8.0.rc3.226.g39d4020

Re: Any interest in 'git merge --continue' as a command

[Junio C Hamano]

Jeff King  writes:

>> They knew about git rebase --continue (and git am and git cherry-pick)
>> but they were unsure how to "continue" a merge (it didn't help that
>> the advice saying to use 'git commit' was scrolling off the top of the
>> terminal). I know that using 'git commit' has been the standard way to
>> complete a merge but given other commands have a --continue should
>> merge have it as well?
>
> It seems like that would be in line with 35d2fffdb (Provide 'git merge
> --abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
> goal was providing consistency with other multi-command operations.
>
> I assume it would _just_ run a vanilla "git commit", and not try to do
> any trickery with updating the index (which could be disastrous).

If we were to have "merge --continue", I agree that it would be the
logical implementation.

There is nothing to "continue" in a stopped merge where Git asked
for help from the user, and because of that, I view the final "git
commit" as "concluding the merge", not "continuing".  "continue"
makes quite a lot of sense with rebase and cherry-pick A..B that
stopped; it concludes the current step and let it continue to
process the remainder.  So from that point of view, it somewhat
feels strange to call it "merge --continue", but it probably is just
me.

Re: [BUG] Colon in remote urls

[Junio C Hamano]

Jeff King  writes:

> (One other option is to just declare that the quarantine feature doesn't
> work with colons in the pathname, but stop turning it on by default. I'm
> not sure I like that, though).

I think we long time ago in 2005 have declared that a colon in a
directory name would not work for Git repositories because of things
like GIT_CEILING_DIRECTORIES, GIT_ALTERNATE_OBJECT_DIRECTORIES; so I
do not think we terribly mind that direction.

> Here's a rough idea of what the quoting solution could look like. It
> should make your case work, but I'm not sure what we want to do about
> backwards-compatibility, if anything.

Yes, obviously it robs from those with backslash in their pathnames
to please those with colons; we've never catered to the latter, so I
am not sure if the trade-off is worth it.

I can see how adding a new environment variable could work, though.
A naive way would be to add GIT_ALT_OBJ_DIRS that uses your quoting
when splitting its elements, give it precedence over the existing
one (or allow to use both and take union as the set of alternate
object stores) and make sure that the codepaths we use internally
uses the new variable.  But if the quarantine codepath will stay to
be the only user of this mechanism (and I strongly suspect that will
be the case), the new environment could just be pointing at a single
directory, i.e. GIT_OBJECT_QUARANTINE_DIRECTORY, whose value is
added without splitting to the list of alternate object stores, and
the quarantine codepath can export that instead.

Re: [PATCH v2 00/16] pathspec cleanup

[Brandon Williams]

On 12/08, Junio C Hamano wrote:
> Will queue, but with fixes on issues spotted by my pre-acceptance
> mechanical filter squashed in, to fix style issues in the
> destination of code movements.

Is this pre-acceptance filter you use something that I could run
locally?

-- 
Brandon Williams

[PATCH v2 4/5] Make sequencer abort safer

[Stephan Beyer]

In contrast to "git am --abort", a sequencer abort did not check
whether the current HEAD is the one that is expected. This can
lead to loss of work (when not spotted and resolved using reflog
before the garbage collector chimes in).

This behavior is now changed by mimicking "git am --abort":
the abortion is done but HEAD is not changed when the current HEAD
is not the expected HEAD.

A new file "sequencer/current" is added to save the expected HEAD.

The new behavior is only active when --abort is invoked on multiple
picks. The problem does not occur for the single-pick case because
it is handled differently.

Signed-off-by: Stephan Beyer 
---
 sequencer.c | 48 +
 t/t3510-cherry-pick-sequence.sh |  2 +-
 2 files changed, 49 insertions(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index 30b10ba14..35c158471 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,6 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
 static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
+static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
 
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -310,6 +311,20 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
 }
 
+static void update_abort_safety_file(void)
+{
+   struct object_id head;
+
+   /* Do nothing on a single-pick */
+   if (!file_exists(git_path_seq_dir()))
+   return;
+
+   if (!get_oid("HEAD", ))
+   write_file(git_path_abort_safety_file(), "%s", 
oid_to_hex());
+   else
+   write_file(git_path_abort_safety_file(), "%s", "");
+}
+
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
int unborn, struct replay_opts *opts)
 {
@@ -339,6 +354,7 @@ static int fast_forward_to(const unsigned char *to, const 
unsigned char *from,
strbuf_release();
strbuf_release();
ref_transaction_free(transaction);
+   update_abort_safety_file();
return 0;
 }
 
@@ -813,6 +829,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
 
 leave:
free_message(commit, );
+   update_abort_safety_file();
 
return res;
 }
@@ -1132,6 +1149,30 @@ static int save_head(const char *head)
return 0;
 }
 
+static int rollback_is_safe(void)
+{
+   struct strbuf sb = STRBUF_INIT;
+   struct object_id expected_head, actual_head;
+
+   if (strbuf_read_file(, git_path_abort_safety_file(), 0) >= 0) {
+   strbuf_trim();
+   if (get_oid_hex(sb.buf, _head)) {
+   strbuf_release();
+   die(_("could not parse %s"), 
git_path_abort_safety_file());
+   }
+   strbuf_release();
+   }
+   else if (errno == ENOENT)
+   oidclr(_head);
+   else
+   die_errno(_("could not read '%s'"), 
git_path_abort_safety_file());
+
+   if (get_oid("HEAD", _head))
+   oidclr(_head);
+
+   return !oidcmp(_head, _head);
+}
+
 static int reset_for_rollback(const unsigned char *sha1)
 {
const char *argv[4];/* reset --merge  + NULL */
@@ -1189,6 +1230,12 @@ int sequencer_rollback(struct replay_opts *opts)
error(_("cannot abort from a branch yet to be born"));
goto fail;
}
+
+   if (!rollback_is_safe()) {
+   /* Do not error, just do not rollback */
+   warning(_("You seem to have moved HEAD. "
+ "Not rewinding, check your HEAD!"));
+   } else
if (reset_for_rollback(sha1))
goto fail;
strbuf_release();
@@ -1393,6 +1440,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
+   update_abort_safety_file();
res = pick_commits(_list, opts);
todo_list_release(_list);
return res;
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index efcd4fc48..372307c21 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,7 +147,7 @@ test_expect_success '--abort to cancel single cherry-pick' '
git diff-index --exit-code HEAD
 '
 
-test_expect_failure '--abort does not unsafely change HEAD' '
+test_expect_success '--abort does not unsafely change HEAD' '
pristine_detach initial &&
test_must_fail git cherry-pick picked anotherpick &&
git reset --hard base &&
-- 
2.11.0.27.g74d6bea

[PATCH v2 5/5] sequencer: Remove useless get_dir() function

[Stephan Beyer]

This function is used only once, for the removal of the
directory. It is not used for the creation of the directory
nor anywhere else.

Signed-off-by: Stephan Beyer 
---
 sequencer.c | 7 +--
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 35c158471..aba096a0a 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -47,11 +47,6 @@ static inline int is_rebase_i(const struct replay_opts *opts)
return 0;
 }
 
-static const char *get_dir(const struct replay_opts *opts)
-{
-   return git_path_seq_dir();
-}
-
 static const char *get_todo_path(const struct replay_opts *opts)
 {
return git_path_todo_file();
@@ -160,7 +155,7 @@ int sequencer_remove_state(struct replay_opts *opts)
free(opts->xopts[i]);
free(opts->xopts);
 
-   strbuf_addf(, "%s", get_dir(opts));
+   strbuf_addf(, "%s", git_path_seq_dir());
remove_dir_recursively(, 0);
strbuf_release();
 
-- 
2.11.0.27.g74d6bea

[PATCH v2 3/5] Add test that cherry-pick --abort does not unsafely change HEAD

[Stephan Beyer]

The test expects failure because it is a current breakage
reported by Junio C Hamano.

Signed-off-by: Stephan Beyer 
---
 t/t3510-cherry-pick-sequence.sh | 10 ++
 1 file changed, 10 insertions(+)

diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89dbd..efcd4fc48 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -147,6 +147,16 @@ test_expect_success '--abort to cancel single cherry-pick' 
'
git diff-index --exit-code HEAD
 '
 
+test_expect_failure '--abort does not unsafely change HEAD' '
+   pristine_detach initial &&
+   test_must_fail git cherry-pick picked anotherpick &&
+   git reset --hard base &&
+   test_must_fail git cherry-pick picked anotherpick &&
+   git cherry-pick --abort 2>actual &&
+   test_i18ngrep "You seem to have moved HEAD" actual &&
+   test_cmp_rev base HEAD
+'
+
 test_expect_success 'cherry-pick --abort to cancel multiple revert' '
pristine_detach anotherpick &&
test_expect_code 1 git revert base..picked &&
-- 
2.11.0.27.g74d6bea

[PATCH v2 1/5] am: Fix filename in safe_to_abort() error message

[Stephan Beyer]

Signed-off-by: Stephan Beyer 
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 6981f42ce..7cf40e6f2 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2124,7 +2124,7 @@ static int safe_to_abort(const struct am_state *state)
 
if (read_state_file(, state, "abort-safety", 1) > 0) {
if (get_oid_hex(sb.buf, _safety))
-   die(_("could not parse %s"), am_path(state, 
"abort_safety"));
+   die(_("could not parse %s"), am_path(state, 
"abort-safety"));
} else
oidclr(_safety);
 
-- 
2.11.0.27.g74d6bea

[PATCH v2 2/5] am: Change safe_to_abort()'s not rewinding error into a warning

[Stephan Beyer]

The error message tells the user that something went terribly wrong
and the --abort could not be performed. But the --abort is performed,
only without rewinding. By simply changing the error into a warning,
we indicate the user that she must not try something like
"git am --abort --force", instead she just has to check the HEAD.

Signed-off-by: Stephan Beyer 
---
 builtin/am.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/am.c b/builtin/am.c
index 7cf40e6f2..826f18ba1 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -2134,7 +2134,7 @@ static int safe_to_abort(const struct am_state *state)
if (!oidcmp(, _safety))
return 1;
 
-   error(_("You seem to have moved HEAD since the last 'am' failure.\n"
+   warning(_("You seem to have moved HEAD since the last 'am' failure.\n"
"Not rewinding to ORIG_HEAD"));
 
return 0;
-- 
2.11.0.27.g74d6bea

Re: [PATCHv7 4/6] worktree: have a function to check if worktrees are in use

[Stefan Beller]

On Fri, Dec 9, 2016 at 4:00 AM, Duy Nguyen  wrote:

> int submodule_uses_worktrees(const char *path)
> {
> struct strbuf path = STRBUF_INIT;
> DIR *dir;
> struct dirent *d;
> int ret = 0;
>
> strbuf_addf(, "%s/worktrees", path);
> dir = opendir(path.buf);
> strbuf_release();
>
> if (!dir)
> return 0;

The submodule may be one of the linked worktrees, which would be
caught if we use the code as I sent it out?

If this is one of the linked worktrees, we'd rather check if a file
"commondir" or "gitdir" exists?

I ask that because I would not know how to relocate such a linked
worktree gitdir?

Re: git add -p with new file

[Ariel]

On Fri, 9 Dec 2016, Jeff King wrote:


On Tue, Dec 06, 2016 at 08:18:59PM -0500, Ariel wrote:



If you do git add -p new_file it says:



No changes.



Which is a rather confusing message. I would expect it to show me the
content of the file in patch form, in the normal way that -p works, let me
edit it, etc.



What should:

[git add directory with two new files]

do?


It should do the exact same thing that git add without -p does: Add the 
directory and both files, but because of the -p also show the diff on the 
screen and ask.


It's contrary to the rest of git-add for specifying pathspecs to 
actually make things _more_ inclusive rather than less.


Is it? Because git add without -p is happy to add new files.


Historically "add -p" has been more like "add -u" in updating tracked
files.


But it doesn't have to be that way. You could make add -p identical to add 
without options, except the -p prompts to review diffs first.



We have "-A" for "update everything _and_ new files". It doesn't
seem unreasonable to me to have a variant of "-p" that is similar.


That seems unnecessarily complex because -p asks about each file, so you 
will never find new files added without realizing it.



I don't think that variant should just be "add -N everything, and then
run add -p". As Duy points out, the patch for a new file is just one big
block. But the decision of "do I want to start tracking this untracked
file" is potentially an interesting one.


What makes sense to me is a two part question: First ask 'Add new path', 
and then if yes, ask to stage the hunk (where the hunk is the entire 
file).


This makes -p useful on new files, without hurting prior expectations of 
how it works.


-Ariel

Re: [PATCH 4/5] Make sequencer abort safer

[Junio C Hamano]

Stephan Beyer  writes:

> However:
>
>> -static void update_curr_file()
>> +static void update_current_file(void)
>
> This function name could lead to the impression that there is some
> current file (defined by a global state or whatever) that is updated.
>
> So I'd rather rename the *file* to one of
>
>  * sequencer/abort-safety (consistent to am, describes its purpose)
>  * sequencer/safety (shorter, still describes the purpose)
>  * sequencer/current-head (describes what it contains)
>  * sequencer/last (a four-letter word, not totally unambiguous though)

OK, so here is a patch that needs to be squashed further on top of
4/5.  I just picked the first one on your list ;-)

Thanks.

 sequencer.c | 20 ++--
 1 file changed, 10 insertions(+), 10 deletions(-)

diff --git a/sequencer.c b/sequencer.c
index 874aaa4cd4..3ac4cb8d3b 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -27,7 +27,7 @@ GIT_PATH_FUNC(git_path_seq_dir, "sequencer")
 static GIT_PATH_FUNC(git_path_todo_file, "sequencer/todo")
 static GIT_PATH_FUNC(git_path_opts_file, "sequencer/opts")
 static GIT_PATH_FUNC(git_path_head_file, "sequencer/head")
-static GIT_PATH_FUNC(git_path_current_file, "sequencer/current")
+static GIT_PATH_FUNC(git_path_abort_safety_file, "sequencer/abort-safety")
 
 /*
  * A script to set the GIT_AUTHOR_NAME, GIT_AUTHOR_EMAIL, and
@@ -306,7 +306,7 @@ static int error_dirty_index(struct replay_opts *opts)
return -1;
 }
 
-static void update_current_file(void)
+static void update_abort_safety_file(void)
 {
struct object_id head;
 
@@ -315,9 +315,9 @@ static void update_current_file(void)
return;
 
if (!get_oid("HEAD", ))
-   write_file(git_path_current_file(), "%s", oid_to_hex());
+   write_file(git_path_abort_safety_file(), "%s", 
oid_to_hex());
else
-   write_file(git_path_current_file(), "%s", "");
+   write_file(git_path_abort_safety_file(), "%s", "");
 }
 
 static int fast_forward_to(const unsigned char *to, const unsigned char *from,
@@ -349,7 +349,7 @@ static int fast_forward_to(const unsigned char *to, const 
unsigned char *from,
strbuf_release();
strbuf_release();
ref_transaction_free(transaction);
-   update_current_file();
+   update_abort_safety_file();
return 0;
 }
 
@@ -824,7 +824,7 @@ static int do_pick_commit(enum todo_command command, struct 
commit *commit,
 
 leave:
free_message(commit, );
-   update_current_file();
+   update_abort_safety_file();
 
return res;
 }
@@ -1149,18 +1149,18 @@ static int rollback_is_safe(void)
struct strbuf sb = STRBUF_INIT;
struct object_id expected_head, actual_head;
 
-   if (strbuf_read_file(, git_path_current_file(), 0) >= 0) {
+   if (strbuf_read_file(, git_path_abort_safety_file(), 0) >= 0) {
strbuf_trim();
if (get_oid_hex(sb.buf, _head)) {
strbuf_release();
-   die(_("could not parse %s"), git_path_current_file());
+   die(_("could not parse %s"), 
git_path_abort_safety_file());
}
strbuf_release();
}
else if (errno == ENOENT)
oidclr(_head);
else
-   die_errno(_("could not read '%s'"), git_path_current_file());
+   die_errno(_("could not read '%s'"), 
git_path_abort_safety_file());
 
if (get_oid("HEAD", _head))
oidclr(_head);
@@ -1436,7 +1436,7 @@ int sequencer_pick_revisions(struct replay_opts *opts)
return -1;
if (save_opts(opts))
return -1;
-   update_current_file();
+   update_abort_safety_file();
res = pick_commits(_list, opts);
todo_list_release(_list);
return res;

Re: git add -p with new file

[Ariel]

On Wed, 7 Dec 2016, Duy Nguyen wrote:


On Wed, Dec 7, 2016 at 8:18 AM, Ariel  wrote:



If you do git add -p new_file it says:



No changes.



Which is a rather confusing message. I would expect it to show me the
content of the file in patch form, in the normal way that -p works, let me
edit it, etc.



We could improve it a bit, suggesting the user to do git add -N. But
is there a point of using -p on a new file?


I got into the habit of always using -p as a way of checking my diffs 
before committing, so I ran it out of habit on a new file as well and got 
that confusing message.


It's even worse if you run it on multiple files, some changed, some added 
- the added ones are ignored completely, without any message at all.



It will be one big chunk, you can't split anything.


That's fine, that's what I would expect.


Perhaps maybe you want to use 'e' to edit what's added?


I might, but mainly it would show me what it was adding.

-Ariel

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Junio C Hamano]

Duy Nguyen  writes:

> On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano  wrote:
>> Stephan Beyer  writes:
>>
>>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>>> different wording for the same thing makes things unintuitive.
>>
>> It is not too late to STOP "--forget" from getting added to "rebase"
>> and give it a better name.
>
> Having the same operation with different names only increases git
> reputation of bad/inconsistent UI. Either forget is renamed to quit,
> or vice versa. I prefer forget, but the decision is yours and the
> community's. So I'm sending two patches to rename in either direction.
> You can pick one.

I actually was advocating to remove both by making --abort saner.
With an updated --abort that behaves saner, is "rebase --forget"
still necessary?

Re: Resend: Gitk: memory consumption improvements

[Stefan Beller]

On Fri, Dec 9, 2016 at 3:51 AM, Markus Hitter  wrote:
>
> It's a month now since I sent three patches to this list for reducing memory 
> consumption of Gitk considerably:
>
> https://public-inbox.org/git/de7cd593-0c10-4e93-1681-7e123504f...@jump-ing.de/
> https://public-inbox.org/git/e09a5309-351d-d246-d272-f527f50ad...@jump-ing.de/
> https://public-inbox.org/git/8e1c5923-d2a6-bc77-97ab-3f154b41d...@jump-ing.de/
> https://public-inbox.org/git/2cb7f76f-0004-a5b6-79f1-9bb4f979c...@jump-ing.de/
>
> Everybody cheered, but apparently nobody picked these patches up and applied 
> them to either the Git or Gitk repository. They don't appear in the regular 
> "what's cooking" post either.

The "What's cooking" email is done by Junio (the Git maintainer)
describing the development of Git, whereas gitk is maintained by Paul
if I understand correctly.

If you look at the Git history, the changes to gitk-git/ always come in via
a merge/pull from Paul.

>
> Anything missing? I'd like to put a 'done' checkmark behind this.
>

I'd love to see those patches in use here (via a regular upstream update).

So I guess the way to go for you is to keep bugging Paul for an ack?

Thanks,
Stefan

Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config

[Jeff King]

On Fri, Dec 09, 2016 at 06:28:10PM +0100, Johannes Schindelin wrote:

> > Great. Thanks for taking a stab at this.
> 
> Well, I figured that I can go through you to get this integrated into
> git.git.

I am not sure what you mean here, but it _sounds_ like you are
continuing to be negative about the chances of fixes going into git.git
here. I really don't think that negativity is merited, but even if it
is, making snide comments does not help and mostly just makes the
conversation less pleasant.

> > I don't think it would be bad to use a global for "we do not want a
> > repo".
> 
> I would think it would be bad, as the entire reason for this patch series
> is that we have global state that gets messed up too early (I am speaking
> from the point of view of somebody who patched Git locally so that it does
> read config variables *before* launching builtins).

I think those are two different things. One is global state that is
munged as a side effect of setup_git_directory(). The other is global
state that the process sets to say "this is an invariant" so it does not
have to deal with passing it through a huge call chain.

> > After all, it's just modifying the _existing_ global for "are we in a
> > repo".
> 
> No it does not.

Perhaps I wasn't clear on my "it" here. I mean that we will continue to
have startup_info->have_repository as a global (and if not that, then
certainly the environment variable GIT_DIR and the cwd are process
globals). I'm proposing a global flag to modify how to interpret those
globals. I don't think that really makes anything worse.

> The read_early_config() function specifically does not leave any traces in
> the global namespace. It calls the git_config_with_options() function
> without touching the_config_set.

I'm not talking about read_early_config() modifying the global state.
I'm talking about main() modifying it so that lower-level functions like
read_early_config() can make use of it.

> I would look more favorably on this idea if we were to teach
> the_config_set to record a little bit more about the state from which it
> was constructed, and to auto-flush-and-re-read when it detects that, say,
> git_dir was changed in the meantime.

I considered that when fixing some bugs in git-init a few months ago,
but I think the cure becomes worse than the problem. Automatic cache
invalidation can have some tricky corner cases, and there really
_aren't_ that many places where we need to do it. Just dropping
git_config_clear() in those places is ugly, but practical.

> > And I do not see that global going away anytime soon.
> 
> And that is really, really sad.

I think we differ on that.

> > As an alternative, I also think it would be OK to just always have the
> > pager config read from local repo, even for init/clone.
> 
> For the purpose of this current discussion, I am utterly uninterested in
> the pager config. What I want to use the early config for is substantially
> different and more relevant: I need to configure some command to run
> before, and after, every single Git command here. And this configuration
> needs to be per-repository. And no, I do not want to hardcode anything.

I do not see how that changes things. If there is the concept of a
"before the command is run" phase during which we would read from the
current-directory config even for git-init, it seems to me that applies
logically to pagers, aliases, _and_ whatever pre-command magic you are
interested in adding.

> > In other words, to loosen the idea that git-init can _never_ look in the
> > current git-dir, and declare that there is a stage before the command is
> > initiated, and during which git may read local-repo config. Aliases
> > would fall into this, too, so:
> > 
> >   git config --local alias.foo init
> >   git foo /some/other/dir
> > 
> > would work (as it must, because we cannot know that "foo" is "init"
> > until we read the config!).
> 
> True.
> 
> But is this a good excuse to just shrug our shoulders and let git-init
> (which we do know very well) fall into the same trap?

I'm not sure I agree it is a trap. There is a consistent and reasonable
mental model where it is the natural thing. I understand that's not the
one you prefer, but it seems like a practical one to me.

> > We already have a config-caching system. If we went with a global
> > "config_discover_refs",
> 
> Why "_refs"?

Er, sorry, slip of the tongue. I think I meant config_discover_git, and
for some reason managed to repeat it over and over. Hopefully you
figured out what I meant.

> > then I think the sequence for something like git-init would become:
> > 
> >   1. When git.c starts, config_discover_refs is set to "true". Pager and
> >  alias lookup may look in .git/config if it's available, even if
> >  they go through the configset cache.
> > 
> >   2. As soon as git-init starts, it disables config_discover_refs, and
> >  it flushes the config cache. Any configset lookups will now examine
> >  the reduced 

Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines

[Johannes Schindelin]

Hi Vasco,

On Fri, 9 Dec 2016, Vasco Almeida wrote:

> A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
> > The incremental update below looks sensible.  We'd also want to
> > protect this codepath from a misconfigured two-or-more byte sequence
> > in core.commentchar, I would suspect, to be consistent.
> 
> Are the below changes alright for what you propose? It just checks if
> the length of core.commentchar's value is 1, otherwise use '#' as the
> comment_line_char.
> As a note, when I set core.commentchar with "git config
> core.commentChar 'batata'", I get the following error message when I
> issue "git add -i":
> 
> error: core.commentChar should only be one character
> fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6

This is exactly the same issue I fixed for rebase -i recently.

Good eyes,
Dscho

Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config

[Johannes Schindelin]

Hi Peff,

On Thu, 8 Dec 2016, Jeff King wrote:

> On Thu, Dec 08, 2016 at 04:35:56PM +0100, Johannes Schindelin wrote:
> 
> > The idea here is to discover the .git/ directory gently (i.e. without
> > changing the current working directory), and to use it to read the
> > .git/config file early, before we actually called setup_git_directory()
> > (if we ever do that).
> 
> Great. Thanks for taking a stab at this.

Well, I figured that I can go through you to get this integrated into
git.git.

> > Notes:
> > 
> > - I find the diff pretty ugly: I wish there was a more elegant way to
> >   *disable* discovery of .git/ *just* for `init` and `clone`. I
> >   considered a function `about_to_create_git_dir()` that is called in a
> >   hard-coded manner *only* for `init` and `clone`, but that would
> >   introduce another magic side effect, when all I want is to reduce those.
> 
> It looks like a lot of that ugliness comes from passing around the "are
> we OK to peek at git-dir config" flag through the various pager-related
> calls.

Correct.

> I don't think it would be bad to use a global for "we do not want a
> repo".

I would think it would be bad, as the entire reason for this patch series
is that we have global state that gets messed up too early (I am speaking
from the point of view of somebody who patched Git locally so that it does
read config variables *before* launching builtins).

> After all, it's just modifying the _existing_ global for "are we in a
> repo".

No it does not.

The read_early_config() function specifically does not leave any traces in
the global namespace. It calls the git_config_with_options() function
without touching the_config_set.

Of course we could introduce a new config set, just for early use. It
would share all the downsides with the current config set.

I would look more favorably on this idea if we were to teach
the_config_set to record a little bit more about the state from which it
was constructed, and to auto-flush-and-re-read when it detects that, say,
git_dir was changed in the meantime.

> And I do not see that global going away anytime soon.

And that is really, really sad.

> Sometimes it's good to make incremental steps towards an end goal, but
> in this case it seems like we just pay the cost of the step without any
> real plan for reaping the benefit in the long term.

Fine. Let's roll with this for now, then.

I just hope that we do not repeat the "slam the door shut to a better
solution" mistake that led to this situation, as the chdir() way did.

> As an alternative, I also think it would be OK to just always have the
> pager config read from local repo, even for init/clone.

For the purpose of this current discussion, I am utterly uninterested in
the pager config. What I want to use the early config for is substantially
different and more relevant: I need to configure some command to run
before, and after, every single Git command here. And this configuration
needs to be per-repository. And no, I do not want to hardcode anything.

So now the cat is out of the bag, I have ulterior motives.

These motives serve a greater good, though: designing read_early_config()
around a very specific use case will always fall short of a well-designed
read_early_config() that could then be used for any use case that
requires, well, reading the config early.

> In other words, to loosen the idea that git-init can _never_ look in the
> current git-dir, and declare that there is a stage before the command is
> initiated, and during which git may read local-repo config. Aliases
> would fall into this, too, so:
> 
>   git config --local alias.foo init
>   git foo /some/other/dir
> 
> would work (as it must, because we cannot know that "foo" is "init"
> until we read the config!).

True.

But is this a good excuse to just shrug our shoulders and let git-init
(which we do know very well) fall into the same trap?

> > - For the moment, I do not handle dashed invocations of `init` and
> >   `clone` correctly. The real problem is the continued existence of
> >   the dashed git-init and git-clone, of course.
> 
> I assume you mean setting the CREATES_GIT_DIR flag here? I think it
> would apply to the dashed invocations, too. They strip off the "git-"
> and end up in handle_builtin() just like "git clone" does. I didn't test
> it, though.

You are correct. I misread the code to expect it to spawn another instance
of git.exe.

> > - There is still duplicated code. For the sake of this RFC, I did not
> >   address that yet.
> 
> Yeah, I did not read your discover function very carefully. Because I
> think the one thing we really don't want to do here is introduce a
> separate lookup process that is not shared by setup_git_directory(). The
> only sane path, I think, is to refactor setup_git_directory() to build
> on top of discover_git_directory(), which implies that the latter
> handles all of the cases.

This could maybe happen later, but for now I do not concern myself with
building the 

Re: [PATCH v6 01/16] Git.pm: add subroutines for commenting lines

[Vasco Almeida]

A Ter, 22-11-2016 às 09:42 -0800, Junio C Hamano escreveu:
> The incremental update below looks sensible.  We'd also want to
> protect this codepath from a misconfigured two-or-more byte sequence
> in core.commentchar, I would suspect, to be consistent.

Are the below changes alright for what you propose? It just checks if
the length of core.commentchar's value is 1, otherwise use '#' as the
comment_line_char.
As a note, when I set core.commentchar with "git config
core.commentChar 'batata'", I get the following error message when I
issue "git add -i":

error: core.commentChar should only be one character
fatal: bad config variable 'core.commentchar' in file '.git/config' at line 6

-- >8 --
diff --git a/git-add--interactive.perl b/git-add--interactive.perl
index 3a6d846..4e0ab5a 100755
--- a/git-add--interactive.perl
+++ b/git-add--interactive.perl
@@ -1072,7 +1072,7 @@ sub edit_hunk_manually {
    print $fh @$oldtext;
    my $is_reverse = $patch_mode_flavour{IS_REVERSE};
    my ($remove_plus, $remove_minus) = $is_reverse ? ('-', '+') : ('+', 
'-');
-   my $comment_line_char = Git::config("core.commentchar") || '#';
+   my $comment_line_char = Git::get_comment_line_char;
    print $fh Git::comment_lines sprintf(__ <

Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config

[Johannes Schindelin]

Hi Duy,

On Fri, 9 Dec 2016, Duy Nguyen wrote:

> On Thu, Dec 8, 2016 at 10:35 PM, Johannes Schindelin
>  wrote:
> > Hopefully these patches will lead to something that we can integrate,
> > and that eventually will make Git's startup sequence much less
> > surprising.
> 
> What did it surprise you with?

I mentioned a couple of WTFs elsewhere:

- that it changes the working directory (personally, I am not surprised,
  just because I was exposed to the Git source code for 10+ years,
  however, I know developers who do not share that experience and find that
  feature... interesting)

- that there are multiple code paths to do essentially the same thing, but
  in an incompatible manner: setup_git_directory() and setup_git_env()
  (and possibly more)

- that some environment variables are set, and need to be unset (or even
  possibly reset to no-longer-known values) to allow subprocesses to
  access different repositories

- that git_path() can be used before setup_git_directory() without
  failure, but wreaks havoc with subsequent git_config() calls.

- that the setup_git_directory() cascade is completely oblivious of
  whatever config has already been read (possibly requiring clearing)

- that setup_git_directory() *returns* the prefix, even stores it in
  startup_info, but a subsequent call to setup_git_directory() returns
  NULL

- that as a consequence, builtins that require the prefix to work if there
  is any, but do not necessarily require a repository to begin with, think
  `git diff`, *must not* be marked with RUN_SETUP_GENTLY

- that check_repository_format() sets have_repository=1

- that setup_git_directory() is a oneliner, calling
  setup_git_directory_gently(NULL), when it would be more logical to
  simply make setup_git_directory() accept the "nongit_ok" parameter

- that setup_git_directory_gently() is not at all gentle when the
  parameter is NULL

- that resolve_gitdir() does not, in fact, resolve any git directory, but
  only tests a specified path whether it refers to a .git/ directory or to
  a .git file

- that resolve_gitdir() actually tests for a .git *file*

- that resolve_gitdir() is not used in setup_git_directory_gently_1()

- that resolve_gitdir() tries the order directory,file and
  setup_git_directory_gently_1() tries the opposite order

- that the handling of the ceiling directories is hardcoded into the
  setup_git_directory_gently_1() function

- that a ceiling directory of /home/hello/world does not prevent Git from
  looking at /home/hello/world/.git/

- that canonicalize_ceiling_entry()' relationship to ceiling directories
  is rather coincidental when the name suggests that it is very specific

- that canonicalize_ceiling_entry() does not, in fact, canonicalize
  non-absolute entries

Need I go on? I could, you know...

> I can see that I disrespect the ceiling directory setting, perhaps
> that's that.

No, I actually see a lot of good reasons for the ceiling directories.

Ciao,
Dscho

Git 2.11.0 on OS X El Capitan 10.11.6

[Kyle Flesness]

Hello! Thanks for taking the time to read this bug report, I am operating a mac 
book pro with OS X El Capitan 10.11.6 and attempting to download git 2.10.1, 
the installation appears to finalize with no problems but Git is not at the 
download destination from the designated path. Will 2.11.0 be compatible with 
OS X El Capitan 10.11.6? Any other solutions you might recommend?

Thank you for your time and hard work!!

Cheers,

Kyle

Re: [REGRESSION 2.10.2] problematic "empty auth" changes

[Johannes Schindelin]

Hi David,

On Thu, 8 Dec 2016, David Turner wrote:

> On Thu, 2016-12-08 at 15:47 +0100, Johannes Schindelin wrote:
> 
> > I got a couple of bug reports that claim that 2.10.2 regressed on
> > using network credentials. That is, users regularly hit Enter twice
> > when being asked for user name and password while fetching via
> > https://, and cURL automatically used to fall back to using the login
> > credentials (i.e.  authenticating via the Domain controller).
> > 
> > Turns out those claims are correct: hitting Enter twice (or using URLs
> > with empty user name/password such as https://:tfs:8080/) work in
> > 2.10.1 and yield "Authentication failed" in 2.10.2.
> > 
> > I tracked this down to 5275c3081c (http: http.emptyauth should allow
> > empty (not just NULL) usernames, 2016-10-04) which all of a sudden
> > disallowed empty user names (and now only handles things correctly
> > when http.emptyAuth is set to true specifically).
> > 
> > This smells like a real bad regression to me, certainly given the time
> > I had to spend to figure this out (starting from not exactly helpful
> > bug reports, due to being very specific to their setups being
> > private).
> > 
> > I am *really* tempted to change the default of http.emptyAuth to true,
> > *at least* for Windows (where it is quite common to use your login
> > credentials to authenticate to corporate servers).
> > 
> > Before I do anything rash, though: Do you see any downside to that?
> 
> I know of no reason that shouldn't work.  Indeed, it's what we use do
> internally.  So far, nobody has reported problems.

Good.

> That said, we have exactly three sets of git servers that most users
> talk to (two different internal; and occasionally github.com for
> external stuff).  So our coverage is not very broad.

Okay. I think I will extend that coverage rather boldly, then ;-)

> If you're going to do it, tho, don't just do it for Windows users -- do
> it for everyone.  Plenty of Unix clients connect to Windows-based auth
> systems.

Makes sense.

> That said, I could imagine that there are cases where it would cause
> failures for a different set of users.

Let's see. At the moment, my main concern is that Git for Windows is
broken for corporate users (i.e. users who rely on the implicit
login authentication provided through their domain accounts). I cannot
imagine that defaulting http.emptyAuth=true could cause any worse
breakage.

It would be different, of course, if http.emptyAuth would *not* allow the
user to type their credentials when accessing something like
https://github.com/dscho/shhh-secret-repository, *only* trying the login
credentials. But that is not the case, with http.emptyAuth=true, login
credentials are attempted first, and when they fail, the user is still
asked interactively for their credentials.

All I can see is that this would be *an improvement*: corporate users
trying to access a Git repository that requires their login credentials
would now not even need to enter empty user name/password.

This alone would be already a good reason to change the default, IMHO.

So here is my plan:

- change the default of http.emptyAuth to true in the next Git for Windows
  version

- publish a prerelease for early adopters to test

- contribute this patch here on the Git mailing list, in the hope that it
  will make it into the next major version

Ciao,
Dscho

[PATCH 3/4] doc: make the intent of sentence clearer

[Kristoffer Haugsbakk]

By adding the word "just", which might have been accidentally omitted.

Adding the word "just" makes it clear that the point is to *not* do an
octopus merge simply because you *can* do it.  In other words, you
should have a reason for doing it beyond simply having two (seemingly)
independent commits that you need to merge into another branch, since
it's not always the best approach.

The previous sentence made it look more like it was trying to say that
you shouldn't do an octopus merge *because* you can do an octopus merge.
Although this interpretation doesn't make sense and the rest of the
paragraph makes the intended meaning clear, this adjustment should make
the intent of the sentence more immediately clear to the reader.

Signed-off-by: Kristoffer Haugsbakk 
---
 Documentation/gitcore-tutorial.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitcore-tutorial.txt 
b/Documentation/gitcore-tutorial.txt
index 72ed90ca3..72ca9c1ef 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1635,7 +1635,7 @@ $ git show-branch
 ++* [master~2] Pretty-print messages.
 
 
-Note that you should not do Octopus because you can.  An octopus
+Note that you should not do Octopus just because you can.  An octopus
 is a valid thing to do and often makes it easier to view the
 commit history if you are merging more than two independent
 changes at the same time.  However, if you have merge conflicts
-- 
2.11.0

[PATCH 4/4] doc: omit needless "for"

[Kristoffer Haugsbakk]

What was intended was perhaps "... plumbing does for you" ("you" added), but
simply omitting the word "for" is more terse and gets the intended point across
just as well, if not more so.

I originally went with the approach of writing "for you", but Junio C
Hamano suggested this approach instead.

Signed-off-by: Kristoffer Haugsbakk 
---

Notes (kristoffers):
The original patch was sent to the mailing list on 2016-11-04, and Junio
replied with his suggested correction on 2016-11-10; see the cover
letter.

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

diff --git a/Documentation/gitcore-tutorial.txt 
b/Documentation/gitcore-tutorial.txt
index 72ca9c1ef..22309cfb4 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -25,7 +25,7 @@ you want to understand Git's internals.
 The core Git is often called "plumbing", with the prettier user
 interfaces on top of it called "porcelain". You may not want to use the
 plumbing directly very often, but it can be good to know what the
-plumbing does for when the porcelain isn't flushing.
+plumbing does when the porcelain isn't flushing.
 
 Back when this document was originally written, many porcelain
 commands were shell scripts. For simplicity, it still uses them as
-- 
2.11.0

[PATCH 0/4] doc: fixes to gitcore-tutorial.txt

[Kristoffer Haugsbakk]

This series of patches attempts to fix some minor mistakes in
gitcore-tutorial.txt that I found while reading it.  They are all
concerned with grammar and things like accidentally omitted words.

I previously sent a single patch on 2016-11-04 ("[PATCH] doc: fill in
omitted word").  The patch "doc: omit needless "for"" is a follow-up to
that, taking into consideration the feedback from Junio C Hamano and
Jeff King.

Kristoffer Haugsbakk (4):
  doc: add articles (grammar)
  doc: add verb in front of command to run
  doc: make the intent of sentence clearer
  doc: omit needless "for"

 Documentation/gitcore-tutorial.txt | 12 ++--
 1 file changed, 6 insertions(+), 6 deletions(-)

-- 
2.11.0

[PATCH 1/4] doc: add articles (grammar)

[Kristoffer Haugsbakk]

Add definite and indefinite articles in three places where they were
missing.

- Use "the" in front of a directory name
- Use "the" in front of "style of cooperation"
- Use an indefinite article in front of "CVS background"

Signed-off-by: Kristoffer Haugsbakk 
---
 Documentation/gitcore-tutorial.txt | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/Documentation/gitcore-tutorial.txt 
b/Documentation/gitcore-tutorial.txt
index 4546fa0d7..6c434aff3 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1368,7 +1368,7 @@ $ git repack
 will do it for you. If you followed the tutorial examples, you
 would have accumulated about 17 objects in `.git/objects/??/`
 directories by now. 'git repack' tells you how many objects it
-packed, and stores the packed file in `.git/objects/pack`
+packed, and stores the packed file in the `.git/objects/pack`
 directory.
 
 [NOTE]
@@ -1543,9 +1543,9 @@ like this:
 Working with Others, Shared Repository Style
 
 
-If you are coming from CVS background, the style of cooperation
+If you are coming from a CVS background, the style of cooperation
 suggested in the previous section may be new to you. You do not
-have to worry. Git supports "shared public repository" style of
+have to worry. Git supports the "shared public repository" style of
 cooperation you are probably more familiar with as well.
 
 See linkgit:gitcvs-migration[7] for the details.
-- 
2.11.0

[PATCH 2/4] doc: add verb in front of command to run

[Kristoffer Haugsbakk]

Instead of using the command 'git clone' as a verb, use "run" as the
verb indicating the action of executing the command 'git clone'.

Signed-off-by: Kristoffer Haugsbakk 
---
 Documentation/gitcore-tutorial.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/gitcore-tutorial.txt 
b/Documentation/gitcore-tutorial.txt
index 6c434aff3..72ed90ca3 100644
--- a/Documentation/gitcore-tutorial.txt
+++ b/Documentation/gitcore-tutorial.txt
@@ -1478,7 +1478,7 @@ You can repack this private repository whenever you feel 
like.
 A recommended work cycle for a "subsystem maintainer" who works
 on that project and has an own "public repository" goes like this:
 
-1. Prepare your work repository, by 'git clone' the public
+1. Prepare your work repository, by running 'git clone' on the public
repository of the "project lead". The URL used for the
initial cloning is stored in the remote.origin.url
configuration variable.
-- 
2.11.0

Re: [BUG] Colon in remote urls

[Jeff King]

On Fri, Dec 09, 2016 at 03:02:15PM +0100, Klaus Ethgen wrote:

> I have some repositories where I have a colon in the (local) url for a
> remote. That was no problem until now but with 2.11.0, I see the
> following problem:
>~> git push
>Counting objects: 11, done.
>Delta compression using up to 8 threads.
>Compressing objects: 100% (10/10), done.
>Writing objects: 100% (11/11), 1.26 KiB | 0 bytes/s, done.
>Total 11 (delta 7), reused 0 (delta 0)
>remote: error: object directory /home/xxx does not exist; check 
> .git/objects/info/alternates.
>remote: error: object directory yyy.git/objects does not exist; check 
> .git/objects/info/alternates.
>remote: fatal: unresolved deltas left after unpacking

Hrm. The problem v2.11's new object-quarantine system. The receiving
side of a push will write into a new temporary object directory, and
refer to the original with the GIT_ALTERNATE_OBJECT_DIRECTORIES
environment variable. But that variable splits its entries on ":", and
has no way of representing a path that includes a ":".

So I think the solution would probably be to introduce some kind of
quoting mechanism to that variable, so that it can pass through
arbitrary paths. Or alternatively use a separate variable, but that does
nothing to help other cases where somebody wants to use xxx:yyy.git as
an alternate.

(One other option is to just declare that the quarantine feature doesn't
work with colons in the pathname, but stop turning it on by default. I'm
not sure I like that, though).

Here's a rough idea of what the quoting solution could look like. It
should make your case work, but I'm not sure what we want to do about
backwards-compatibility, if anything.

---
 sha1_file.c  | 41 ++---
 tmp-objdir.c | 18 --
 2 files changed, 46 insertions(+), 13 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 9c86d1924..a0b341b5a 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -329,13 +329,35 @@ static int link_alt_odb_entry(const char *entry, const 
char *relative_base,
return 0;
 }
 
+const char *parse_alt_odb_entry(const char *string, int sep,
+   struct strbuf *out)
+{
+   const char *p;
+   int literal = 0;
+
+   strbuf_reset(out);
+
+   for (p = string; *p; p++) {
+   if (literal) {
+   strbuf_addch(out, *p);
+   literal = 0;
+   } else {
+   if (*p == '\\')
+   literal = 1;
+   else if (*p == sep)
+   return p + 1;
+   else
+   strbuf_addch(out, *p);
+   }
+   }
+   return p;
+}
+
 static void link_alt_odb_entries(const char *alt, int len, int sep,
 const char *relative_base, int depth)
 {
-   struct string_list entries = STRING_LIST_INIT_NODUP;
-   char *alt_copy;
-   int i;
struct strbuf objdirbuf = STRBUF_INIT;
+   struct strbuf entry = STRBUF_INIT;
 
if (depth > 5) {
error("%s: ignoring alternate object stores, nesting too deep.",
@@ -348,16 +370,13 @@ static void link_alt_odb_entries(const char *alt, int 
len, int sep,
die("unable to normalize object directory: %s",
objdirbuf.buf);
 
-   alt_copy = xmemdupz(alt, len);
-   string_list_split_in_place(, alt_copy, sep, -1);
-   for (i = 0; i < entries.nr; i++) {
-   const char *entry = entries.items[i].string;
-   if (entry[0] == '\0' || entry[0] == '#')
+   while (*alt) {
+   alt = parse_alt_odb_entry(alt, sep, );
+   if (!entry.len || entry.buf[0] == '#')
continue;
-   link_alt_odb_entry(entry, relative_base, depth, objdirbuf.buf);
+   link_alt_odb_entry(entry.buf, relative_base, depth, 
objdirbuf.buf);
}
-   string_list_clear(, 0);
-   free(alt_copy);
+   strbuf_release();
strbuf_release();
 }
 
diff --git a/tmp-objdir.c b/tmp-objdir.c
index 64435f23a..900e15af1 100644
--- a/tmp-objdir.c
+++ b/tmp-objdir.c
@@ -70,6 +70,17 @@ static void remove_tmp_objdir_on_signal(int signo)
raise(signo);
 }
 
+static char *backslash_quote(const char *s, int delim)
+{
+   struct strbuf buf = STRBUF_INIT;
+   while (*s) {
+   if (*s == '\\' || *s == delim)
+   strbuf_addch(, '\\');
+   strbuf_addch(, *s++);
+   }
+   return strbuf_detach(, NULL);
+}
+
 /*
  * These env_* functions are for setting up the child environment; the
  * "replace" variant overrides the value of any existing variable with that
@@ -79,12 +90,15 @@ static void remove_tmp_objdir_on_signal(int signo)
  */
 static void env_append(struct argv_array *env, const char *key, const char 
*val)
 {
+   char *quoted = 

[BUG] Colon in remote urls

[Klaus Ethgen]

-BEGIN PGP SIGNED MESSAGE-
Hash: SHA512

Hello,

I have some repositories where I have a colon in the (local) url for a
remote. That was no problem until now but with 2.11.0, I see the
following problem:
   ~> git push
   Counting objects: 11, done.
   Delta compression using up to 8 threads.
   Compressing objects: 100% (10/10), done.
   Writing objects: 100% (11/11), 1.26 KiB | 0 bytes/s, done.
   Total 11 (delta 7), reused 0 (delta 0)
   remote: error: object directory /home/xxx does not exist; check 
.git/objects/info/alternates.
   remote: error: object directory yyy.git/objects does not exist; check 
.git/objects/info/alternates.
   remote: fatal: unresolved deltas left after unpacking
   error: unpack failed: unpack-objects abnormal exit
   To /home/xxx:yyy.git
! [remote rejected] master -> master (unpacker error)
   error: failed to push some refs to '/home/xxx:yyy.git'

Prepending the path with "file://" does not help.

It seems that the new git version splits the url at ":" which ends in
two incorrect paths.

Pull seems tho work well currently.

Regards
   Klaus

Ps. I am not subscribet to the mailing list, so please keep me in Cc.
- -- 
Klaus Ethgen   http://www.ethgen.ch/
pub  4096R/4E20AF1C 2011-05-16Klaus Ethgen 
Fingerprint: 85D4 CA42 952C 949B 1753  62B3 79D0 B06F 4E20 AF1C
-BEGIN PGP SIGNATURE-
Comment: Charset: ISO-8859-1

iQGzBAEBCgAdFiEEMWF28vh4/UMJJLQEpnwKsYAZ9qwFAlhKuV4ACgkQpnwKsYAZ
9qz81Qv6AsgZHlaEHEybERAvGikjZgUvjyC7dzQ2zCmc8iv0eb8kGLiBtVtInsWr
eo/CiMSdX2emoCD5LQq/sxcRIgIoWpF8m2NEXiBMl94d6YLOpvWV1yC1kQ8qK6bt
Pyeo9/LofAnpLcQRn1am8tFwrcCMLZxSM7cxMxjtP6i+RU0MHc/rS1HqhdzptpsH
jvB0x41X7LNoeRLqG5n8lVlyHP1PiGyP0Dl4Aa9rFBHn+hydiJEmLEwdn9w4wgIz
vo2DZmqGm0r4vTaTP1gQRPxoW6fsBZ1uUWKRMjd947R1eaELpyROj4SFt0bWNqwW
cx9izYUuTg+xSe1KTaSgPlmZn1817DlG2yJ/YduKH8v61ZJ2r6B1awO2tz32g7cA
QdjsnzAyz6eVLrrsJ5OrJPRF2Fl7gM22jo9gs3BJrHeJdC9dU6kVIAR3eryoFvUg
fG/Vl2zvfbMRQAaUDGMyxNk5TGVFB6ANw0KS/NczRvmbPA9kukBz012+8ZY8MHzD
aEvmk/yz
=tDwn
-END PGP SIGNATURE-

Re: [PATCH v2 3/4] real_path: create real_pathdup

[Johannes Sixt]

Am 09.12.2016 um 00:58 schrieb Brandon Williams:

+char *real_pathdup(const char *path)
+{
+   struct strbuf realpath = STRBUF_INIT;
+   char *retval = NULL;
+
+   if(strbuf_realpath(, path, 0))


Style nit: blank after if is missing.

-- Hannes

Re: [PATCH v2 1/4] real_path: resolve symlinks by hand

[Johannes Sixt]

Am 09.12.2016 um 00:58 schrieb Brandon Williams:

The current implementation of real_path uses chdir() in order to resolve
symlinks.  Unfortunately this isn't thread-safe as chdir() affects a
process as a whole and not just an individual thread.  Instead perform
the symlink resolution by hand so that the calls to chdir() can be
removed, making real_path one step closer to being reentrant.

Signed-off-by: Brandon Williams 
---
 abspath.c | 183 +-
 1 file changed, 122 insertions(+), 61 deletions(-)

diff --git a/abspath.c b/abspath.c
index 2825de8..92f2a29 100644
--- a/abspath.c
+++ b/abspath.c
@@ -11,8 +11,38 @@ int is_directory(const char *path)
return (!stat(path, ) && S_ISDIR(st.st_mode));
 }

+/* removes the last path component from 'path' except if 'path' is root */
+static void strip_last_component(struct strbuf *path)
+{
+   if (path->len > offset_1st_component(path->buf)) {
+   char *last_slash = find_last_dir_sep(path->buf);
+   strbuf_setlen(path, last_slash - path->buf);
+   }
+}


This implementation is not correct because when the input is "/foo", the 
result is "" when it should be "/". Also, can the input be a 
non-normalized path? When the input is "foo///bar", should the result be 
"foo" or would "foo//" be an acceptable result? I think it should be the 
former. find_last_dir_sep() returns the last of the three slashes, not 
the first one. Therefore, I've rewritten the function thusly:


static void strip_last_component(struct strbuf *path)
{
size_t offset = offset_1st_component(path->buf);
size_t len = path->len;
while (offset < len && !is_dir_sep(path->buf[len - 1]))
len--;
while (offset < len && is_dir_sep(path->buf[len - 1]))
len--;
strbuf_setlen(path, len);
}


+
+/* get (and remove) the next component in 'remaining' and place it in 'next' */
+static void get_next_component(struct strbuf *next, struct strbuf *remaining)
+{
+   char *start = NULL;
+   char *end = NULL;
+
+   strbuf_reset(next);
+
+   /* look for the next component */
+   /* Skip sequences of multiple path-separators */
+   for (start = remaining->buf; is_dir_sep(*start); start++)
+   ; /* nothing */
+   /* Find end of the path component */
+   for (end = start; *end && !is_dir_sep(*end); end++)
+   ; /* nothing */
+
+   strbuf_add(next, start, end - start);
+   /* remove the component from 'remaining' */
+   strbuf_remove(remaining, 0, end - remaining->buf);
+}


Mental note: This function strips leading slashes and the first path 
component; post-condition: remaining is empty or has leading slashes.



+
 /* We allow "recursive" symbolic links. Only within reason, though. */
-#define MAXDEPTH 5
+#define MAXSYMLINKS 5

 /*
  * Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +51,6 @@ int is_directory(const char *path)
  * absolute_path().)  The return value is a pointer to a static
  * buffer.
  *
- * The input and all intermediate paths must be shorter than MAX_PATH.
  * The directory part of path (i.e., everything up to the last
  * dir_sep) must denote a valid, existing directory, but the last
  * component need not exist.  If die_on_error is set, then die with an
@@ -33,22 +62,16 @@ int is_directory(const char *path)
  */
 static const char *real_path_internal(const char *path, int die_on_error)
 {
-   static struct strbuf sb = STRBUF_INIT;
+   static struct strbuf resolved = STRBUF_INIT;
+   struct strbuf remaining = STRBUF_INIT;
+   struct strbuf next = STRBUF_INIT;
+   struct strbuf symlink = STRBUF_INIT;
char *retval = NULL;
-
-   /*
-* If we have to temporarily chdir(), store the original CWD
-* here so that we can chdir() back to it at the end of the
-* function:
-*/
-   struct strbuf cwd = STRBUF_INIT;
-
-   int depth = MAXDEPTH;
-   char *last_elem = NULL;
+   int num_symlinks = 0;
struct stat st;

/* We've already done it */
-   if (path == sb.buf)
+   if (path == resolved.buf)
return path;

if (!*path) {
@@ -58,74 +81,112 @@ static const char *real_path_internal(const char *path, 
int die_on_error)
goto error_out;
}

-   strbuf_reset();
-   strbuf_addstr(, path);
-
-   while (depth--) {
-   if (!is_directory(sb.buf)) {
-   char *last_slash = find_last_dir_sep(sb.buf);
-   if (last_slash) {
-   last_elem = xstrdup(last_slash + 1);
-   strbuf_setlen(, last_slash - sb.buf + 1);
-   } else {
-   last_elem = xmemdupz(sb.buf, sb.len);
-   strbuf_reset();
-   }
+ 

Assalamu`Alaikum.

[mohammad ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($10.6 Million us dollars) to transfer into 
your account,

I will send you more details about this deal and the procedures to follow when 
I receive a positive response from you, 

Have a great day,
Dr mohammad ouattara.

Re: git add -p with new file

[Jeff King]

On Tue, Dec 06, 2016 at 08:18:59PM -0500, Ariel wrote:

> If you do git add -p new_file it says:
> 
> No changes.
> 
> Which is a rather confusing message. I would expect it to show me the
> content of the file in patch form, in the normal way that -p works, let me
> edit it, etc.
> 
> (Note: I am aware I can do -N first, but when I specifically enter the name
> of a new file I feel it should figure out what I mean.)

Keep in mind that the argument to "git add -p" is not a filename, but a
"pathspec" that can match many files. What should:

  git init
  mkdir subdir
  for i in one two; do echo $i >subdir/$i; done
  git add -p subdir

do?  Or for that matter, "git add -p ."? It's contrary to the rest of
git-add for specifying pathspecs to actually make things _more_
inclusive rather than less.

So I don't think triggering the change of behavior based on the presence
of a pathspec makes sense.

Historically "add -p" has been more like "add -u" in updating tracked
files. We have "-A" for "update everything _and_ new files". It doesn't
seem unreasonable to me to have a variant of "-p" that is similar.

I don't think that variant should just be "add -N everything, and then
run add -p". As Duy points out, the patch for a new file is just one big
block. But the decision of "do I want to start tracking this untracked
file" is potentially an interesting one.

-Peff

Re: [PATCH v8 18/19] branch: use ref-filter printing APIs

[Jeff King]

On Wed, Dec 07, 2016 at 09:06:26PM +0530, Karthik Nayak wrote:

> +const char *quote_literal_for_format(const char *s)
>  {
> + struct strbuf buf = STRBUF_INIT;
>  
> + strbuf_reset();
> + while (*s) {
> + const char *ep = strchrnul(s, '%');
> + if (s < ep)
> + strbuf_add(, s, ep - s);
> + if (*ep == '%') {
> + strbuf_addstr(, "%%");
> + s = ep + 1;
> + } else {
> + s = ep;
> + }
>   }
> + return buf.buf;
>  }

You should use strbuf_detach() to return the buffer from a strbuf.
Otherwise it is undefined whether the pointer is allocated or not (and
whether it needs to be freed).

In this case, if "s" is empty, buf.buf would point to a string literal,
but otherwise to allocated memory. strbuf_detach() normalizes that.

But...

> + branch_get_color(BRANCH_COLOR_REMOTE), maxwidth, 
> quote_literal_for_format(remote_prefix),

This caller never stores the return value, and it ends up leaking. So I
wonder if you wanted "static struct strbuf" in the first place (and that
would explain the strbuf_reset() in your function).

-Peff

Re: Bug: git-sh-setup giving no such file or directory

[Paul Boyle]

> Hmm. Did you run "make install"? Or are you trying to run git directly
> out of the build directory?
>
> If the latter, that has been unsupported for a while, though it mostly
> works. The "right" way is to either set up GIT_EXEC_PATH as appropriate,
> or to just .../git/bin-wrappers into your $PATH.

This was the source of it. Root cause, a stupid user ;) I'd a PATH
setup to the build directory. Changing the path to bin-wrappers fixed
it up.

Thanks!

On Fri, Dec 9, 2016 at 1:45 PM, Jeff King  wrote:
> On Fri, Dec 09, 2016 at 12:00:36PM +, Paul Boyle wrote:
>
>> There appears to be an issue with the latest master.
>>
>> "git submodule init" is producing the following error:
>>
>> /home/paul.boyle/bin/git/git-sh-setup: line 46:
>> /home/paul.boyle/libexec/git-core/git-sh-i18n: No such file or
>> directory
>
> Hmm. Did you run "make install"? Or are you trying to run git directly
> out of the build directory?
>
> If the latter, that has been unsupported for a while, though it mostly
> works. The "right" way is to either set up GIT_EXEC_PATH as appropriate,
> or to just .../git/bin-wrappers into your $PATH.
>
>> Broken sha: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
>>
>> Checking out an older version works fine.
>>
>> git checkout 'master@{2016-11-01 18:30:00}'
>>
>> Sha: 3cdd5d19178a54d2e51b5098d43b57571241d0ab
>>
>> This can be reproduced simply by:
>>
>> make clean ; make ; git submodule init
>>
>>
>> I didn't track it down further than to a commit sometime in the last month.
>
> You could probably find the exact commit with git-bisect. However, I'd
> be surprised if it is anything but 1073094f3 (git-sh-setup: be explicit
> where to dot-source git-sh-i18n from., 2016-10-29). Before that commit,
> we found git-sh-i18n in the $PATH, which would work if you were adding
> git's build directory to your $PATH (but not work for people who
> actually did an install).
>
> -Peff

Re: Bug: git-sh-setup giving no such file or directory

[Jeff King]

On Fri, Dec 09, 2016 at 12:00:36PM +, Paul Boyle wrote:

> There appears to be an issue with the latest master.
> 
> "git submodule init" is producing the following error:
> 
> /home/paul.boyle/bin/git/git-sh-setup: line 46:
> /home/paul.boyle/libexec/git-core/git-sh-i18n: No such file or
> directory

Hmm. Did you run "make install"? Or are you trying to run git directly
out of the build directory?

If the latter, that has been unsupported for a while, though it mostly
works. The "right" way is to either set up GIT_EXEC_PATH as appropriate,
or to just .../git/bin-wrappers into your $PATH.

> Broken sha: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2
> 
> Checking out an older version works fine.
> 
> git checkout 'master@{2016-11-01 18:30:00}'
> 
> Sha: 3cdd5d19178a54d2e51b5098d43b57571241d0ab
> 
> This can be reproduced simply by:
> 
> make clean ; make ; git submodule init
> 
> 
> I didn't track it down further than to a commit sometime in the last month.

You could probably find the exact commit with git-bisect. However, I'd
be surprised if it is anything but 1073094f3 (git-sh-setup: be explicit
where to dot-source git-sh-i18n from., 2016-10-29). Before that commit,
we found git-sh-i18n in the $PATH, which would work if you were adding
git's build directory to your $PATH (but not work for people who
actually did an install).

-Peff

[PATCH] describe: add tests for unusual graphs

[Quinn Grier]

git describe may give incorrect results if there are backdated commits
or multiple roots. This commit adds two test_expect_failure tests that
demonstrate these problems.

Signed-off-by: Quinn Grier 
---
 t/t6120-describe.sh | 48 
 1 file changed, 48 insertions(+)

diff --git a/t/t6120-describe.sh b/t/t6120-describe.sh
index 85f2694..ca82837 100755
--- a/t/t6120-describe.sh
+++ b/t/t6120-describe.sh
@@ -206,4 +206,52 @@ test_expect_success 'describe --contains with the exact 
tags' '
test_cmp expect actual
 '
 
+#
+# A---B*--D master
+#  \ /
+#   .---C topic
+#
+
+test_expect_failure 'backdated commit' '(
+   test_tick &&
+   b=$GIT_COMMITTER_DATE && test_tick &&
+   test_create_repo backdated-commit &&
+   cd backdated-commit &&
+   git commit --allow-empty -m A && test_tick &&
+   GIT_COMMITTER_DATE=$b git commit --allow-empty -m B && test_tick &&
+   git checkout -b topic :/A &&
+   git commit --allow-empty -m C && test_tick &&
+   git checkout master &&
+   git merge -m D topic && test_tick &&
+   git tag -m B B :/B && test_tick &&
+   git describe :/D >tmp &&
+   sed s/-g.\*// tmp >actual &&
+   echo B-2 >expected &&
+   test_cmp expected actual
+)'
+
+#
+# A---B*--D master
+#/
+#   C* other
+#
+
+test_expect_failure 'multiple roots' '(
+   test_tick &&
+   test_create_repo multiple-roots &&
+   cd multiple-roots &&
+   git commit --allow-empty -m A && test_tick &&
+   git commit --allow-empty -m B && test_tick &&
+   git checkout --orphan other &&
+   git commit --allow-empty -m C && test_tick &&
+   git checkout master &&
+   git merge --allow-unrelated-histories -m D other && test_tick &&
+   git tag -m B B :/B && test_tick &&
+   git tag -m C C :/C && test_tick &&
+   git describe :/D >tmp &&
+   sed s/-g.\*// tmp >actual &&
+   echo B-2 >expected &&
+   test_cmp expected actual
+)'
+
 test_done
-- 
2.8.3

Re: [PATCH 02/17] dir: convert create_simplify to use the pathspec struct interface

[Duy Nguyen]

On Fri, Dec 9, 2016 at 1:19 AM, Brandon Williams  wrote:
> On 12/08, Duy Nguyen wrote:
>> On Thu, Dec 8, 2016 at 7:03 AM, Brandon Williams  wrote:
>> > On 12/07, Duy Nguyen wrote:
>> >> On Wed, Dec 7, 2016 at 4:51 AM, Brandon Williams  
>> >> wrote:
>> >> > Convert 'create_simplify()' to use the pathspec struct interface from
>> >> > using the '_raw' entry in the pathspec.
>> >>
>> >> It would be even better to kill this create_simplify() and let
>> >> simplify_away() handle struct pathspec directly.
>> >>
>> >> There is a bug in this code, that might have been found if we
>> >> simpify_away() handled pathspec directly: the memcmp() in
>> >> simplify_away() will not play well with :(icase) magic. My bad. If
>> >> :(icase) is used, the easiest/safe way is simplify nothing. Later on
>> >> maybe we can teach simplify_away() to do strncasecmp instead. We could
>> >> ignore exclude patterns there too (although not excluding is not a
>> >> bug).
>> >
>> > So are you implying that the simplify struct needs to be killed?  That
>> > way the pathspec struct itself is being passed around instead?
>>
>> Yes. simplify struct was a thing when pathspec was an array of char *.
>> At this point I think it can retire (when we have time to retire it)
>
> Alright, then for now I can leave this change as is and have a follow up
> series that kills the simplify struct.

Do let me know if you decide to drop it, so I can put it back in my backlog.
-- 
Duy

Re: [PATCHv6 4/7] worktree: get worktrees from submodules

[Duy Nguyen]

On Fri, Dec 9, 2016 at 1:55 AM, Stefan Beller  wrote:
> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen  wrote:
>> On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller  wrote:
>>>
>>> worktree = xcalloc(1, sizeof(*worktree));
>>> worktree->path = strbuf_detach(_path, NULL);
>>> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)
>>
>> All the good stuff is outside context lines again.. Somewhere between
>> here we call add_head_info() which calls resolve_ref_unsafe(), which
>> always uses data from current repo, not the submodule we want it to
>> look at.
>
> Unrelated side question: What would you think of "variable context line
> configuration" ? e.g. you could configure it to include anything from
> up that line
> that is currently shown after the @@ which is the function signature line.

Hmm.. no idea. I once dreamt of writing "Diff-Options: -U10" in the
commit message and let git-log and everybody use that option
automatically, though. I guess it's unrelated to your question :D

> As to the add_head_info/resolve_ref_unsafe what impact does that have?
> It produces a wrong head info but AFAICT it will never die(), such that for 
> the
> purposes of this series (which only wants to know if a submodule uses the
> worktree feature) it should be fine.
>
> It is highly misleading though for others to build upon this.
> So maybe I'll only add the functionality internally in worktree.c
> and document why the values are wrong, and only expose the
> "int submodule_uses_worktrees(const char *path)" ?

Yeah for submodule use then it should be ok. But people may start
using it for something else, not realizing that it does not work as
expected.
-- 
Duy

Assalamu`Alaikum.

[mohammad ouattara]

Dear Sir/Madam.

Assalamu`Alaikum.

I am Dr mohammad ouattara, I have  ($10.6 Million us dollars) to transfer into 
your account,

I will send you more details about this deal and the procedures to follow when 
I receive a positive response from you, 

Have a great day,
Dr mohammad ouattara.

Re: [PATCH/RFC 0/7] Pie-in-the-sky attempt to fix the early config

[Duy Nguyen]

On Thu, Dec 8, 2016 at 10:35 PM, Johannes Schindelin
 wrote:
> Hopefully these patches will lead to something that we can integrate,
> and that eventually will make Git's startup sequence much less
> surprising.

What did it surprise you with? Just curious. I can see that I
disrespect the ceiling directory setting, perhaps that's that.
-- 
Duy

Re: Feature request: read git config from parent directory

[Duy Nguyen]

On Fri, Dec 9, 2016 at 2:49 AM, Dominique Dumont  wrote:
> Hello
>
> I use the same machine for work and open-source contribution. In both cases, I
> deal with a lot of repositories. Depending on whether I commit for work or
> open-source activities, I must use a different mail address. I used to setup
> work address for each work repo in git local config, but this is no longer
> practical.

Sounds like the same problem I have (and the reason I came up with
conditional include [1]). Would that work for you (check out the
example part in that patch)?

It's not supported yet. I'll need to address a few comments in the
future reroll.

[1] http://public-inbox.org/git/20160712164216.24072-1-pclo...@gmail.com/

> Since I use different directories for work and open-source, would it be
> possible for git to read irs config also from parent directories ? I.e. setup
> git to read config from ./.git/config then ../.gitconfig, ../../gitconfig 
> until
> global ~/.gitconfig.
-- 
Duy

Re: [PATCH v2 0/4] road to reentrant real_path

[Duy Nguyen]

On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams  wrote:
> diff --git a/setup.c b/setup.c
> index fe572b8..0d9fdd0 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const char 
> *gitdir)
> if (!is_absolute_path(data.buf))
> strbuf_addf(, "%s/", gitdir);
> strbuf_addbuf(, );
> -   strbuf_addstr(sb, real_path(path.buf));
> +   strbuf_realpath(sb, path.buf, 1);

This is not the same because of this hunk in strbuf_realpath()

> @@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, 
> int die_on_error)
> goto error_out;
> }
>
> -   strbuf_reset();
> +   strbuf_reset(resolved);
>
> if (is_absolute_path(path)) {

But if you you remove that, then all (old) callers of
strbuf_realpath() must do a strbuf_reset() in advance if needed
(probably just real_path does) which sounds reasonable to me. You're
probably want to be careful about the strbuf_reset() at the end of the
function too.

Other than that, I think this diff looks nice.
-- 
Duy

Re: [PATCH] doc: fill in omitted word

[Kristoffer Haugsbakk]

I agree.  Just writing "... what the plumbing does ..." is clearer and
less redundant.

I'll probably be sending a patch series that includes your proposed fix
sometime soon.

-- 
Kristoffer Haugsbakk

Re: [PATCH v2 14/16] pathspec: create strip submodule slash helpers

[Duy Nguyen]

On Fri, Dec 9, 2016 at 7:28 AM, Junio C Hamano  wrote:
> Brandon Williams  writes:
>
>> +static void strip_submodule_slash_cheap(struct pathspec_item *item)
>> +{
>> + int i;
>> +
>> + if ((item->len >= 1 && item->match[item->len - 1] == '/') &&
>> + (i = cache_name_pos(item->match, item->len - 1)) >= 0 &&
>> + S_ISGITLINK(active_cache[i]->ce_mode)) {
>> + item->len--;
>> + item->match[item->len] = '\0';
>> + }
>> +}
>
> I know that this is merely a moved code, but while I was reading
> this, it triggered "Do not make an assignment inside if condition"
> check.

Yeah with a function of its own, it's probably better to separate that
assignment.

> But more importantly, is the code even correct?  If the path
> for the submodule is unmerged, we would get a negative i that points
> at the conflicting entry; don't we want to do something about it, at
> least when we have a submodule entry at stage #2 (i.e. ours)?

In my defense I was simply moving (again!) the code from
strip_trailing_slash_from_submodules() in b69bb3f:builtin/ls-files.c.
Could be an improvement point for submodule people, I guess.
-- 
Duy

Re: [PATCHv7 4/6] worktree: have a function to check if worktrees are in use

[Duy Nguyen]

On Thu, Dec 08, 2016 at 01:03:27PM -0800, Stefan Beller wrote:
> +/*
> + * NEEDSWORK: The values in the returned worktrees are broken, e.g.
> + * the refs or path resolution is influenced by the current repository.
> + */
> +static struct worktree **get_submodule_worktrees(const char *path, unsigned 
> flags)

I'm ok with this (at least we know the function is half-broken). But I
wonder if we could go simpler, with something like this. It's more
efficient as well. And we can replace its implementation with
get_worktrees() or something when we are able to.

As long as this function stays in worktree.c I think it's ok for it to
peek into "worktrees" directory directly. That's what all other
functions in this file do after all.

int submodule_uses_worktrees(const char *path)
{
struct strbuf path = STRBUF_INIT;
DIR *dir;
struct dirent *d;
int ret = 0;

strbuf_addf(, "%s/worktrees", path);
dir = opendir(path.buf);
strbuf_release();

if (!dir)
return 0;

while ((d = readdir(dir)) != NULL) {
if (is_dot_or_dotdot(d->d_name))
continue;

ret = 1;
break;
}
closedir(dir);
return ret;
}
--
Duy

Bug: git-sh-setup giving no such file or directory

[Paul Boyle]

Hi

There appears to be an issue with the latest master.

"git submodule init" is producing the following error:

/home/paul.boyle/bin/git/git-sh-setup: line 46:
/home/paul.boyle/libexec/git-core/git-sh-i18n: No such file or
directory

Broken sha: 8d7a455ed52e2a96debc080dfc011b6bb00db5d2

Checking out an older version works fine.

git checkout 'master@{2016-11-01 18:30:00}'

Sha: 3cdd5d19178a54d2e51b5098d43b57571241d0ab

This can be reproduced simply by:

make clean ; make ; git submodule init


I didn't track it down further than to a commit sometime in the last month.

Details of machine that this is happening on:

[paul.boyle@gonzo git]$ cat /etc/redhat-release

Scientific Linux release 6.8 (Carbon)


[paul.boyle@gonzo git]$ env

SSH_AGENT_PID=29474

HOSTNAME=gonzo

TERM=xterm

SHELL=/bin/bash

HISTSIZE=1000

SSH_CLIENT=

QTDIR=/usr/lib64/qt-3.3

QTINC=/usr/lib64/qt-3.3/include

SSH_TTY=/dev/pts/135

USER=paul.boyle

LS_COLORS=rs=0:di=01;34:ln=01;36:mh=00:pi=40;33:so=01;35:do=01;35:bd=40;33;01:cd=40;33;01:or=40;31;01:mi=01;05;37;41:su=37;41:sg=30;43:ca=30;41:tw=30;42:ow=34;42:st=37;44:ex=01;32:*.tar=01;31:*.tgz=01;31:*.arj=01;31:*.taz=01;31:*.lzh=01;31:*.lzma=01;31:*.tlz=01;31:*.txz=01;31:*.zip=01;31:*.z=01;31:*.Z=01;31:*.dz=01;31:*.gz=01;31:*.lz=01;31:*.xz=01;31:*.bz2=01;31:*.tbz=01;31:*.tbz2=01;31:*.bz=01;31:*.tz=01;31:*.deb=01;31:*.rpm=01;31:*.jar=01;31:*.rar=01;31:*.ace=01;31:*.zoo=01;31:*.cpio=01;31:*.7z=01;31:*.rz=01;31:*.jpg=01;35:*.jpeg=01;35:*.gif=01;35:*.bmp=01;35:*.pbm=01;35:*.pgm=01;35:*.ppm=01;35:*.tga=01;35:*.xbm=01;35:*.xpm=01;35:*.tif=01;35:*.tiff=01;35:*.png=01;35:*.svg=01;35:*.svgz=01;35:*.mng=01;35:*.pcx=01;35:*.mov=01;35:*.mpg=01;35:*.mpeg=01;35:*.m2v=01;35:*.mkv=01;35:*.ogm=01;35:*.mp4=01;35:*.m4v=01;35:*.mp4v=01;35:*.vob=01;35:*.qt=01;35:*.nuv=01;35:*.wmv=01;35:*.asf=01;35:*.rm=01;35:*.rmvb=01;35:*.flc=01;35:*.avi=01;35:*.fli=01;35:*.flv=01;35:*.gl=01;35:*.dl=01;35:*.xcf=01;35:*.xwd=01;35:*.yuv=01;35:*.cgm=01;35:*.emf=01;35:*.axv=01;35:*.anx=01;35:*.ogv=01;35:*.ogx=01;35:*.aac=01;36:*.au=01;36:*.flac=01;36:*.mid=01;36:*.midi=01;36:*.mka=01;36:*.mp3=01;36:*.mpc=01;36:*.ogg=01;36:*.ra=01;36:*.wav=01;36:*.axa=01;36:*.oga=01;36:*.spx=01;36:*.xspf=01;36:

SSH_AUTH_SOCK=/tmp/ssh-oXONs29470/agent.29470

MAIL=/var/spool/mail/paul.boyle

PATH=/home/paul.boyle/bin/git:/home/paul.boyle/bin/tig:/home/paul.boyle/bin:/usr/lib64/qt-3.3/bin:/usr/local/bin:/bin:/usr/bin:/usr/local/sbin:/usr/sbin:/sbin:/home/paul.boyle/bin

PWD=/home/paul.boyle/bin/git

LANG=en_IE.UTF-8

HISTCONTROL=ignoredups

SHLVL=1

HOME=/home/paul.boyle

LOGNAME=paul.boyle

QTLIB=/usr/lib64/qt-3.3/lib

CVS_RSH=ssh

SSH_CONNECTION=

LESSOPEN=||/usr/bin/lesspipe.sh %s

G_BROKEN_FILENAMES=1

OLDPWD=/home/paul.boyle/

_=/bin/env

Resend: Gitk: memory consumption improvements

[Markus Hitter]

It's a month now since I sent three patches to this list for reducing memory 
consumption of Gitk considerably:

https://public-inbox.org/git/de7cd593-0c10-4e93-1681-7e123504f...@jump-ing.de/
https://public-inbox.org/git/e09a5309-351d-d246-d272-f527f50ad...@jump-ing.de/
https://public-inbox.org/git/8e1c5923-d2a6-bc77-97ab-3f154b41d...@jump-ing.de/
https://public-inbox.org/git/2cb7f76f-0004-a5b6-79f1-9bb4f979c...@jump-ing.de/

Everybody cheered, but apparently nobody picked these patches up and applied 
them to either the Git or Gitk repository. They don't appear in the regular 
"what's cooking" post either.

Anything missing? I'd like to put a 'done' checkmark behind this.


Markus

-- 
- - - - - - - - - - - - - - - - - - -
Dipl. Ing. (FH) Markus Hitter
http://www.jump-ing.de/

[PATCH] revert, cherry-pick: rename --quit to be consistent with rebase

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

The old --quit remains supported, just hidden away.

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-cherry-pick.txt  |  2 +-
 Documentation/git-revert.txt   |  2 +-
 Documentation/sequencer.txt|  2 +-
 builtin/revert.c   |  7 +--
 contrib/completion/git-completion.bash |  4 ++--
 sequencer.c|  2 +-
 t/t3510-cherry-pick-sequence.sh| 14 +++---
 t/t3511-cherry-pick-x.sh   |  2 +-
 8 files changed, 19 insertions(+), 16 deletions(-)

diff --git a/Documentation/git-cherry-pick.txt 
b/Documentation/git-cherry-pick.txt
index 6154e57..de878ff 100644
--- a/Documentation/git-cherry-pick.txt
+++ b/Documentation/git-cherry-pick.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 'git cherry-pick' [--edit] [-n] [-m parent-number] [-s] [-x] [--ff]
  [-S[]] ...
 'git cherry-pick' --continue
-'git cherry-pick' --quit
+'git cherry-pick' --forget
 'git cherry-pick' --abort
 
 DESCRIPTION
diff --git a/Documentation/git-revert.txt b/Documentation/git-revert.txt
index 573616a..c21a43b 100644
--- a/Documentation/git-revert.txt
+++ b/Documentation/git-revert.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 'git revert' [--[no-]edit] [-n] [-m parent-number] [-s] [-S[]] 
...
 'git revert' --continue
-'git revert' --quit
+'git revert' --forget
 'git revert' --abort
 
 DESCRIPTION
diff --git a/Documentation/sequencer.txt b/Documentation/sequencer.txt
index 5747f44..ddfaad6 100644
--- a/Documentation/sequencer.txt
+++ b/Documentation/sequencer.txt
@@ -3,7 +3,7 @@
'.git/sequencer'.  Can be used to continue after resolving
conflicts in a failed cherry-pick or revert.
 
---quit::
+--forget::
Forget about the current operation in progress.  Can be used
to clear the sequencer state after a failed cherry-pick or
revert.
diff --git a/builtin/revert.c b/builtin/revert.c
index 56a2c36..663eaf7 100644
--- a/builtin/revert.c
+++ b/builtin/revert.c
@@ -77,7 +77,10 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
const char *me = action_name(opts);
int cmd = 0;
struct option options[] = {
-   OPT_CMDMODE(0, "quit", , N_("end revert or cherry-pick 
sequence"), 'q'),
+   OPT_CMDMODE(0, "forget", , N_("end revert or cherry-pick 
sequence"), 'q'),
+   { OPTION_CMDMODE, 0, "quit", , NULL,
+ N_("end revert or cherry-pick sequence"),
+ PARSE_OPT_NOARG|PARSE_OPT_NONEG|PARSE_OPT_HIDDEN, NULL, 'q' },
OPT_CMDMODE(0, "continue", , N_("resume revert or 
cherry-pick sequence"), 'c'),
OPT_CMDMODE(0, "abort", , N_("cancel revert or cherry-pick 
sequence"), 'a'),
OPT_BOOL('n', "no-commit", >no_commit, N_("don't 
automatically commit")),
@@ -134,7 +137,7 @@ static void parse_args(int argc, const char **argv, struct 
replay_opts *opts)
if (opts->subcommand != REPLAY_NONE) {
char *this_operation;
if (opts->subcommand == REPLAY_REMOVE_STATE)
-   this_operation = "--quit";
+   this_operation = "--forget";
else if (opts->subcommand == REPLAY_CONTINUE)
this_operation = "--continue";
else {
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 8159f28..d5c74e7 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1047,7 +1047,7 @@ _git_cherry_pick ()
 {
local dir="$(__gitdir)"
if [ -f "$dir"/CHERRY_PICK_HEAD ]; then
-   __gitcomp "--continue --quit --abort"
+   __gitcomp "--continue --forget --abort"
return
fi
case "$cur" in
@@ -2303,7 +2303,7 @@ _git_revert ()
 {
local dir="$(__gitdir)"
if [ -f "$dir"/REVERT_HEAD ]; then
-   __gitcomp "--continue --quit --abort"
+   __gitcomp "--continue --forget --abort"
return
fi
case "$cur" in
diff --git a/sequencer.c b/sequencer.c
index e66f2fe..12d10d0 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -812,7 +812,7 @@ static int create_seq_dir(void)
 {
if (file_exists(git_path_seq_dir())) {
error(_("a cherry-pick or revert is already in progress"));
-   advise(_("try \"git cherry-pick (--continue | --quit | 
--abort)\""));
+   advise(_("try \"git cherry-pick (--continue | --forget | 
--abort)\""));
return -1;
}
else if (mkdir(git_path_seq_dir(), 0777) < 0)
diff --git a/t/t3510-cherry-pick-sequence.sh b/t/t3510-cherry-pick-sequence.sh
index 7b7a89d..d84fafa 100755
--- a/t/t3510-cherry-pick-sequence.sh
+++ b/t/t3510-cherry-pick-sequence.sh
@@ -18,7 +18,7 @@ test_description='Test cherry-pick continuation features
 _r10='\1\1\1\1\1\1\1\1\1\1'
 
 pristine_detach () 

[PATCH] rebase: rename --forget to be consistent with sequencer

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

Signed-off-by: Nguyễn Thái Ngọc Duy 
---
 Documentation/git-rebase.txt   | 4 ++--
 contrib/completion/git-completion.bash | 4 ++--
 git-rebase.sh  | 6 +++---
 t/t3407-rebase-abort.sh| 8 
 4 files changed, 11 insertions(+), 11 deletions(-)

diff --git a/Documentation/git-rebase.txt b/Documentation/git-rebase.txt
index e01d78e..f892458 100644
--- a/Documentation/git-rebase.txt
+++ b/Documentation/git-rebase.txt
@@ -12,7 +12,7 @@ SYNOPSIS
[ []]
 'git rebase' [-i | --interactive] [options] [--exec ] [--onto ]
--root []
-'git rebase' --continue | --skip | --abort | --forget | --edit-todo
+'git rebase' --continue | --skip | --abort | --quit | --edit-todo
 
 DESCRIPTION
 ---
@@ -252,7 +252,7 @@ leave out at most one of A and B, in which case it defaults 
to HEAD.
will be reset to where it was when the rebase operation was
started.
 
---forget::
+--quit::
Abort the rebase operation but HEAD is not reset back to the
original branch. The index and working tree are also left
unchanged as a result.
diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 8159f28..2c134f8 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1670,10 +1670,10 @@ _git_rebase ()
 {
local dir="$(__gitdir)"
if [ -f "$dir"/rebase-merge/interactive ]; then
-   __gitcomp "--continue --skip --abort --forget --edit-todo"
+   __gitcomp "--continue --skip --abort --quit --edit-todo"
return
elif [ -d "$dir"/rebase-apply ] || [ -d "$dir"/rebase-merge ]; then
-   __gitcomp "--continue --skip --abort --forget"
+   __gitcomp "--continue --skip --abort --quit"
return
fi
__git_complete_strategy && return
diff --git a/git-rebase.sh b/git-rebase.sh
index f0de633..c62b178 100755
--- a/git-rebase.sh
+++ b/git-rebase.sh
@@ -43,7 +43,7 @@ continue!  continue
 abort! abort and check out the original branch
 skip!  skip current patch and continue
 edit-todo! edit the todo list during an interactive rebase
-forget!abort but keep HEAD where it is
+quit!  abort but keep HEAD where it is
 "
 . git-sh-setup
 . git-sh-i18n
@@ -240,7 +240,7 @@ do
--verify)
ok_to_skip_pre_rebase=
;;
-   --continue|--skip|--abort|--forget|--edit-todo)
+   --continue|--skip|--abort|--quit|--edit-todo)
test $total_argc -eq 2 || usage
action=${1##--}
;;
@@ -403,7 +403,7 @@ abort)
finish_rebase
exit
;;
-forget)
+quit)
exec rm -rf "$state_dir"
;;
 edit-todo)
diff --git a/t/t3407-rebase-abort.sh b/t/t3407-rebase-abort.sh
index 6bc5e71..910f218 100755
--- a/t/t3407-rebase-abort.sh
+++ b/t/t3407-rebase-abort.sh
@@ -99,26 +99,26 @@ testrebase() {
 testrebase "" .git/rebase-apply
 testrebase " --merge" .git/rebase-merge
 
-test_expect_success 'rebase --forget' '
+test_expect_success 'rebase --quit' '
cd "$work_dir" &&
# Clean up the state from the previous one
git reset --hard pre-rebase &&
test_must_fail git rebase master &&
test_path_is_dir .git/rebase-apply &&
head_before=$(git rev-parse HEAD) &&
-   git rebase --forget &&
+   git rebase --quit &&
test $(git rev-parse HEAD) = $head_before &&
test ! -d .git/rebase-apply
 '
 
-test_expect_success 'rebase --merge --forget' '
+test_expect_success 'rebase --merge --quit' '
cd "$work_dir" &&
# Clean up the state from the previous one
git reset --hard pre-rebase &&
test_must_fail git rebase --merge master &&
test_path_is_dir .git/rebase-merge &&
head_before=$(git rev-parse HEAD) &&
-   git rebase --forget &&
+   git rebase --quit &&
test $(git rev-parse HEAD) = $head_before &&
test ! -d .git/rebase-merge
 '
-- 
2.8.2.524.g6ff3d78

Re: BUG: "cherry-pick A..B || git reset --hard OTHER"

[Duy Nguyen]

On Thu, Dec 8, 2016 at 3:04 AM, Junio C Hamano  wrote:
> Stephan Beyer  writes:
>
>> [1] By the way: git cherry-pick --quit, git rebase --forget ...
>> different wording for the same thing makes things unintuitive.
>
> It is not too late to STOP "--forget" from getting added to "rebase"
> and give it a better name.

Having the same operation with different names only increases git
reputation of bad/inconsistent UI. Either forget is renamed to quit,
or vice versa. I prefer forget, but the decision is yours and the
community's. So I'm sending two patches to rename in either direction.
You can pick one.
-- 
Duy

Re: Any interest in 'git merge --continue' as a command

[Jacob Keller]

On December 9, 2016 1:11:27 AM PST, Jeff King  wrote:
>On Fri, Dec 09, 2016 at 08:57:58PM +1300, Chris Packham wrote:
>
>> I hit this at $dayjob recently.
>> 
>> A developer had got themselves into a confused state when needing to
>> resolve a merge conflict.
>> 
>> They knew about git rebase --continue (and git am and git
>cherry-pick)
>> but they were unsure how to "continue" a merge (it didn't help that
>> the advice saying to use 'git commit' was scrolling off the top of
>the
>> terminal). I know that using 'git commit' has been the standard way
>to
>> complete a merge but given other commands have a --continue should
>> merge have it as well?
>
>It seems like that would be in line with 35d2fffdb (Provide 'git merge
>--abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
>goal was providing consistency with other multi-command operations.
>
>I assume it would _just_ run a vanilla "git commit", and not try to do
>any trickery with updating the index (which could be disastrous).
>
>-Peff

This makes sense to me.

Thanks,
Jake

Re: [REGRESSION 2.10.2] problematic "empty auth" changes

[David Turner]

On Thu, 2016-12-08 at 15:47 +0100, Johannes Schindelin wrote:
> Hi Dave,
> 
> I got a couple of bug reports that claim that 2.10.2 regressed on using
> network credentials. That is, users regularly hit Enter twice when being
> asked for user name and password while fetching via https://, and cURL
> automatically used to fall back to using the login credentials (i.e.
> authenticating via the Domain controller).
> 
> Turns out those claims are correct: hitting Enter twice (or using URLs
> with empty user name/password such as https://:tfs:8080/) work in 2.10.1
> and yield "Authentication failed" in 2.10.2.
> 
> I tracked this down to 5275c3081c (http: http.emptyauth should allow empty
> (not just NULL) usernames, 2016-10-04) which all of a sudden disallowed
> empty user names (and now only handles things correctly when
> http.emptyAuth is set to true specifically).
> 
> This smells like a real bad regression to me, certainly given the time I
> had to spend to figure this out (starting from not exactly helpful bug
> reports, due to being very specific to their setups being private).
> 
> I am *really* tempted to change the default of http.emptyAuth to true, *at
> least* for Windows (where it is quite common to use your login credentials
> to authenticate to corporate servers).
> 
> Before I do anything rash, though: Do you see any downside to that?

I know of no reason that shouldn't work.  Indeed, it's what we use do
internally.  So far, nobody has reported problems.  That said, we have
exactly three sets of git servers that most users talk to (two different
internal; and occasionally github.com for external stuff).  So our
coverage is not very broad.

If you're going to do it, tho, don't just do it for Windows users -- do
it for everyone.  Plenty of Unix clients connect to Windows-based auth
systems.

That said, I could imagine that there are cases where it would cause
failures for a different set of users.  I don't know of any off the top
of my head, but this is not my area of expertise.

We could move closer to the old behavior with something like:

if (!http_auth.username || !*http_auth.username) {
if (curl_empty_auth)
curl_easy_setopt(result, CURLOPT_USERPWD, ":");
if (!http_auth.username)
return;
}

This is very ad-hoc, in that I have not examined exactly when
http_auth.username would be null vs empty; it merely attempts to get as
close as possible to the old behavior.

[Side note: I was curious if 26a7b23429 would have been a better fix for
our problem.  It appears that the answer is no; using that patch but
minus my 5275c3081c does not work for us.]

Re: Any interest in 'git merge --continue' as a command

[Jeff King]

On Fri, Dec 09, 2016 at 08:57:58PM +1300, Chris Packham wrote:

> I hit this at $dayjob recently.
> 
> A developer had got themselves into a confused state when needing to
> resolve a merge conflict.
> 
> They knew about git rebase --continue (and git am and git cherry-pick)
> but they were unsure how to "continue" a merge (it didn't help that
> the advice saying to use 'git commit' was scrolling off the top of the
> terminal). I know that using 'git commit' has been the standard way to
> complete a merge but given other commands have a --continue should
> merge have it as well?

It seems like that would be in line with 35d2fffdb (Provide 'git merge
--abort' as a synonym to 'git reset --merge', 2010-11-09), whose stated
goal was providing consistency with other multi-command operations.

I assume it would _just_ run a vanilla "git commit", and not try to do
any trickery with updating the index (which could be disastrous).

-Peff

[PATCH 3/3] difftool: rename variables for consistency

[David Aguilar]

Always call the list of files @files.
Always call the worktree $worktree.

Signed-off-by: David Aguilar 
---
 git-difftool.perl | 28 ++--
 1 file changed, 14 insertions(+), 14 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 99b03949bf..4e4f5d8138 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -100,7 +100,7 @@ sub changed_files
 
 sub setup_dir_diff
 {
-   my ($workdir, $symlinks) = @_;
+   my ($worktree, $symlinks) = @_;
my @gitargs = ('diff', '--raw', '--no-abbrev', '-z', @ARGV);
my $diffrtn = Git::command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
@@ -109,7 +109,7 @@ sub setup_dir_diff
# changed files.  The paths returned by diff --raw are relative to the
# top-level of the repository, but we defer changing directories so
# that @ARGV can perform pathspec limiting in the current directory.
-   chdir($workdir);
+   chdir($worktree);
 
# Build index info for left and right sides of the diff
my $submodule_mode = '16';
@@ -121,7 +121,7 @@ sub setup_dir_diff
my $wtindex = '';
my %submodule;
my %symlink;
-   my @working_tree = ();
+   my @files = ();
my %working_tree_dups = ();
my @rawdiff = split('\0', $diffrtn);
 
@@ -173,14 +173,14 @@ EOF
}
 
if ($rmode ne $null_mode) {
-   # Avoid duplicate working_tree entries
+   # Avoid duplicate entries
if ($working_tree_dups{$dst_path}++) {
next;
}
my ($use, $wt_sha1) =
use_wt_file($dst_path, $rsha1);
if ($use) {
-   push @working_tree, $dst_path;
+   push @files, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
} else {
$rindex .= "$rmode $rsha1\t$dst_path\0";
@@ -227,14 +227,14 @@ EOF
 
# Changes in the working tree need special treatment since they are
# not part of the index.
-   for my $file (@working_tree) {
+   for my $file (@files) {
my $dir = dirname($file);
unless (-d "$rdir/$dir") {
mkpath("$rdir/$dir") or
exit_cleanup($tmpdir, 1);
}
if ($symlinks) {
-   symlink("$workdir/$file", "$rdir/$file") or
+   symlink("$worktree/$file", "$rdir/$file") or
exit_cleanup($tmpdir, 1);
} else {
copy($file, "$rdir/$file") or
@@ -278,7 +278,7 @@ EOF
exit_cleanup($tmpdir, 1) if not $ok;
}
 
-   return ($ldir, $rdir, $tmpdir, @working_tree);
+   return ($ldir, $rdir, $tmpdir, @files);
 }
 
 sub write_to_file
@@ -388,9 +388,9 @@ sub dir_diff
my $error = 0;
my $repo = Git->repository();
my $repo_path = $repo->repo_path();
-   my $workdir = $repo->wc_path();
-   $workdir =~ s|/$||; # Avoid double slashes in symlink targets
-   my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);
+   my $worktree = $repo->wc_path();
+   $worktree =~ s|/$||; # Avoid double slashes in symlink targets
+   my ($a, $b, $tmpdir, @files) = setup_dir_diff($worktree, $symlinks);
 
if (defined($extcmd)) {
$rc = system($extcmd, $a, $b);
@@ -411,13 +411,13 @@ sub dir_diff
my %tmp_modified;
my $indices_loaded = 0;
 
-   for my $file (@worktree) {
+   for my $file (@files) {
next if $symlinks && -l "$b/$file";
next if ! -f "$b/$file";
 
if (!$indices_loaded) {
%wt_modified = changed_files(
-   $repo_path, "$tmpdir/wtindex", $workdir);
+   $repo_path, "$tmpdir/wtindex", $worktree);
%tmp_modified = changed_files(
$repo_path, "$tmpdir/wtindex", $b);
$indices_loaded = 1;
@@ -425,7 +425,7 @@ sub dir_diff
 
if (exists $wt_modified{$file} and exists $tmp_modified{$file}) 
{
my $errmsg = "warning: Both files modified: ";
-   $errmsg .= "'$workdir/$file' and '$b/$file'.\n";
+   $errmsg .= "'$worktree/$file' and '$b/$file'.\n";
$errmsg .= "warning: Working tree file has been 
left.\n";
$errmsg .= "warning:\n";
warn $errmsg;
-- 
2.11.0.26.gb65c994

[PATCH 2/3] difftool: chdir as early as possible

[David Aguilar]

Make difftool chdir to the top-level of the repository as soon as it can
so that we can simplify how paths are handled.  Replace construction of
absolute paths via string concatenation with relative paths wherever
possible.  The bulk of the code no longer needs to use absolute paths.

Signed-off-by: David Aguilar 
---
 git-difftool.perl | 26 ++
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 17c336321f..99b03949bf 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -59,14 +59,14 @@ sub exit_cleanup
 
 sub use_wt_file
 {
-   my ($workdir, $file, $sha1) = @_;
+   my ($file, $sha1) = @_;
my $null_sha1 = '0' x 40;
 
-   if (-l "$workdir/$file" || ! -e _) {
+   if (-l $file || ! -e _) {
return (0, $null_sha1);
}
 
-   my $wt_sha1 = Git::command_oneline('hash-object', "$workdir/$file");
+   my $wt_sha1 = Git::command_oneline('hash-object', $file);
my $use = ($sha1 eq $null_sha1) || ($sha1 eq $wt_sha1);
return ($use, $wt_sha1);
 }
@@ -105,6 +105,12 @@ sub setup_dir_diff
my $diffrtn = Git::command_oneline(@gitargs);
exit(0) unless defined($diffrtn);
 
+   # Go to the root of the worktree now that we've captured the list of
+   # changed files.  The paths returned by diff --raw are relative to the
+   # top-level of the repository, but we defer changing directories so
+   # that @ARGV can perform pathspec limiting in the current directory.
+   chdir($workdir);
+
# Build index info for left and right sides of the diff
my $submodule_mode = '16';
my $symlink_mode = '12';
@@ -172,7 +178,7 @@ EOF
next;
}
my ($use, $wt_sha1) =
-   use_wt_file($workdir, $dst_path, $rsha1);
+   use_wt_file($dst_path, $rsha1);
if ($use) {
push @working_tree, $dst_path;
$wtindex .= "$rmode $wt_sha1\t$dst_path\0";
@@ -182,10 +188,6 @@ EOF
}
}
 
-   # Go to the root of the worktree so that the left index files
-   # are properly setup -- the index is toplevel-relative.
-   chdir($workdir);
-
# Setup temp directories
my $tmpdir = tempdir('git-difftool.X', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
@@ -235,10 +237,10 @@ EOF
symlink("$workdir/$file", "$rdir/$file") or
exit_cleanup($tmpdir, 1);
} else {
-   copy("$workdir/$file", "$rdir/$file") or
+   copy($file, "$rdir/$file") or
exit_cleanup($tmpdir, 1);
 
-   my $mode = stat("$workdir/$file")->mode;
+   my $mode = stat($file)->mode;
chmod($mode, "$rdir/$file") or
exit_cleanup($tmpdir, 1);
}
@@ -430,10 +432,10 @@ sub dir_diff
$error = 1;
} elsif (exists $tmp_modified{$file}) {
my $mode = stat("$b/$file")->mode;
-   copy("$b/$file", "$workdir/$file") or
+   copy("$b/$file", $file) or
exit_cleanup($tmpdir, 1);
 
-   chmod($mode, "$workdir/$file") or
+   chmod($mode, $file) or
exit_cleanup($tmpdir, 1);
}
}
-- 
2.11.0.26.gb65c994

[PATCH 1/3] difftool: sanitize $workdir as early as possible

[David Aguilar]

The double-slash fixup on the $workdir variable was being
performed just-in-time to avoid double-slashes in symlink
targets, but the rest of the code was silently using paths with
embedded "//" in them.

A recent user-reported error message contained double-slashes.
Eliminate the issue by sanitizing inputs as soon as they arrive.

Signed-off-by: David Aguilar 
---
 git-difftool.perl | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/git-difftool.perl b/git-difftool.perl
index 959822d5f3..17c336321f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -224,9 +224,7 @@ EOF
delete($ENV{GIT_INDEX_FILE});
 
# Changes in the working tree need special treatment since they are
-   # not part of the index. Remove any trailing slash from $workdir
-   # before starting to avoid double slashes in symlink targets.
-   $workdir =~ s|/$||;
+   # not part of the index.
for my $file (@working_tree) {
my $dir = dirname($file);
unless (-d "$rdir/$dir") {
@@ -389,6 +387,7 @@ sub dir_diff
my $repo = Git->repository();
my $repo_path = $repo->repo_path();
my $workdir = $repo->wc_path();
+   $workdir =~ s|/$||; # Avoid double slashes in symlink targets
my ($a, $b, $tmpdir, @worktree) = setup_dir_diff($workdir, $symlinks);
 
if (defined($extcmd)) {
-- 
2.11.0.26.gb65c994

Re: git bash error

[Konstantin Khomoutov]

On Fri, 9 Dec 2016 11:38:55 +0530
"Karamjeet Singh"  wrote:

> Dear git support,
> My app is crashing whenever i launch the git bash tool. I am
> attaching the error log file from the event viewer. Can you please
> let me know what the issue is with it.
> https://www.dropbox.com/sh/mhkmjn8bmh3x1oh/AABUKmhnn-HW2Kv5UVxdckN6a?dl=0

Hi!

Your report misses lots of information required to even approach it:

- What Git version are you using (the fact is: in each next version of
  a software package some bugs get fixed and others might creep in;
  so knowing an exact version is paramount).

- What OS? Version, flavor, architecture (32/64 bit).

- What software package (i.e. where did you get your Git install from)?

>From the term "git bash", I gather you're talking about Git for Windows.
If so, that project has its own bug tracker on Github [1] -- because
it's still a project sort-of separate from the "vanilla" Git due to
the fact it maintains a set of changes not yet in the Git proper, and
they do packaging work, too.

Please use that issue tracker in two steps:

1) Search it for your issue.  Say, remove the "is:open" modifier from
   the search box in the tracker's web interface, put there the words
   "git", "bash" and "crash" and search.  I'm sure you'll get a hefty
   amount of reports.  Please see whether your issue is already
   reported.

2) If yes, and if (and only if) you have additional details about it,
   please summarise them in a comment.  Please try to write that in
   plain English (plain bad non-native English is okay :-)); try not to
   post links to pictures or videos.  They aren't indexed by search
   engines and require the maintainers to switch their context when
   reading your report/comment.  On some platforms (say, w/o proper
   full-blown web browser) they can even be plain hard to even see.

3) If no, write your report there -- by filling the offerred template.

Thanks.

1. https://github.com/git-for-windows/git/issues/