Re: Error after calling git difftool -d with

[P. Duijst]

On 12/5/2016 06:15, David Aguilar wrote:

On Fri, Dec 02, 2016 at 05:05:06PM +0100, Johannes Schindelin wrote:

Hi Peter,

On Fri, 2 Dec 2016, P. Duijst wrote:


Incase filenames are used with a quote ' or a bracket [  (and maybe some more
characters), git "diff" and "difftool -y" works fine, but git *difftool **-d*
gives the next error message:

peter@scm_ws_10 MINGW64 /d/Dev/test (master)
$ git diff
diff --git a/Test ''inch.txt b/Test ''inch.txt
index dbff793..41f3257 100644
--- a/Test ''inch.txt
+++ b/Test ''inch.txt
@@ -1 +1,3 @@
+
+ddd
  Test error in simple repository
warning: LF will be replaced by CRLF in Test ''inch.txt.
The file will have its original line endings in your working directory.

peter@scm_ws_10 MINGW64 /d/Dev/test (master)
*$ git difftool -d*
*fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
directory*
*hash-object /d/Dev/test//Test ''inch.txt: command returned error: 128*

peter@scm_ws_10 MINGW64 /d/Dev/test (master)
$


This issue is inside V2.10.x and V2.11.0.
V2.9.0 is working correctly...

You say v2.11.0, but did you also try the new, experimental builtin
difftool? You can test without reinstalling:

git -c difftool.useBuiltin=true difftool -d ...

FWIW, I verified that this problem does not manifest itself on
Linux, using the current scripted difftool.

Peter, what actual diff tool are you using?

Since these filenames work fine with "difftool -d" on Linux, it
suggests that this is either a tool-specific issue, or an issue
related to unix-to-windows path translation.

Hi all,

@Johannes: "git -c difftool.useBuiltin=true difftool -d" works OK :-), 
beyond compare is launching with the diff's displayed

@David: I am using Beyond Compare V4.1.9

Best regards,

Peter

Re: Should reset_revision_walk clear more flags?

[Jeff King]

On Sun, Dec 04, 2016 at 11:26:04PM -0800, Junio C Hamano wrote:

> Jeff King  writes:
> 
> > Which I think would include both of the flags you mentioned, along with
> > others like SYMMETRIC_LEFT, PATCHSAME, etc. Probably really everything
> > mentioned in revision.h, which should be summed up as ALL_REV_FLAGS.
> > Some callsites already seem to feed that to clear_commit_marks().
> >
> > I doubt you can go too wrong by clearing more flags. 
> 
> This and ...
> 
> > It's possible that
> > some caller is relying on a flag _not_ being cleared between two
> > traversals, but in that case it should probably be using
> > clear_commit_marks() or clear_object_flags() explicitly to make it clear
> > what it expects to be saved.
> 
> ... this are contradictory, no?

I don't think so. If you are calling reset_revision_walk(), then I'm not
sure you have any right to expect that particular flags are left intact.
You _should_ be using an option that lets you specify the full set of
flags, rather than making an implicit assumption about which flags are
left.

In other words, I'd much rather see:

  clear_object_flags(ALL_REV_FLAGS & (~TMP_MARK));

than:

  /* This leaves TMP_MARK intact! */
  reset_revision_walk();

if you need to leave TMP_MARK intact.

Which isn't to say every caller does it correctly already. My claim
above is only "should", not "does". :)

But given that many of them _do_ use the more specific flag-clearing
functions, I think most of the calls to reset_revision_walk() are
blanket "I'm done now, clean up after me" calls.

> A caller may use two sets of its own flags in addition to letting
> the traversal machinery use the basic ones, and it may want to keep
> one of its own two sets while clearing the other set.  It would
> clear_commit_marks() to clear the latter.  And then it would let
> that the next traversal to reset_revision_walk() clear the basic
> ones.
> 
> So if you make reset_revision_walk() clear "more flags", that would
> break such a caller that is using clear_commit_marks() to make it
> clear what its expectations are, no?

I'd argue that it should not be calling reset_revision_walk() at all if
it wants anything saved. It should mask out the flags it wants, as
above. But I do realize that reasoning is circular: it's OK for
reset_revision_walk() to reset everything because that's what it does,
or at least should do from its name. :)

-Peff

Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository

[Jeff King]

On Sun, Dec 04, 2016 at 11:19:46PM -0800, Junio C Hamano wrote:

> > -   if (no_index)
> > +   if (no_index) {
> > /* If this is a no-index diff, just run it and exit there. */
> > +   startup_info->have_repository = 0;
> > diff_no_index(, argc, argv);
> > +   }
> 
> This kind of change makes me nervous (partly because I am not seeing
> the whole code but only this part of the patch).
> 
> Some code may react to "have_repository" being zero and do the right
> thing (which I think is what you are using from your previous "we
> did one of the three cases" change here), but the codepath that led
> to "have_repository" being set to non-zero previously must have done
> a lot more than just flipping that field to non-zero, and setting
> zero to this field alone would not "undo" what it did.

I _think_ it's OK because the only substantive change would be the
chdir() to the top of the working tree. But that information is carried
through by revs->prefix, which we act on regardless of the value of
startup_info->have_repository when we call prefix_filename().

I agree that it may be an accident waiting to happen, though, as soon as
some buried sub-function needs to care about the distinction.

> I wonder if we're better off if we made sure that diff_no_index()
> works the same way regardless of the value of "have_repository"
> field?

If you mean adding a diffopt flag like "just abbreviate everything to
FALLBACK_DEFAULT_ABBREV even if we're in a repository", and then setting
that in diff_no_index(), I agree that is a lot cleaner.

I'm still not 100% convinced that it's actually the correct behavior,
but at least doing a more contained version wouldn't take away other
functionality like reading config.

-Peff

Re: Should reset_revision_walk clear more flags?

[Junio C Hamano]

Jeff King  writes:

> Which I think would include both of the flags you mentioned, along with
> others like SYMMETRIC_LEFT, PATCHSAME, etc. Probably really everything
> mentioned in revision.h, which should be summed up as ALL_REV_FLAGS.
> Some callsites already seem to feed that to clear_commit_marks().
>
> I doubt you can go too wrong by clearing more flags. 

This and ...

> It's possible that
> some caller is relying on a flag _not_ being cleared between two
> traversals, but in that case it should probably be using
> clear_commit_marks() or clear_object_flags() explicitly to make it clear
> what it expects to be saved.

... this are contradictory, no?  

A caller may use two sets of its own flags in addition to letting
the traversal machinery use the basic ones, and it may want to keep
one of its own two sets while clearing the other set.  It would
clear_commit_marks() to clear the latter.  And then it would let
that the next traversal to reset_revision_walk() clear the basic
ones.

So if you make reset_revision_walk() clear "more flags", that would
break such a caller that is using clear_commit_marks() to make it
clear what its expectations are, no?

I dunno.

Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository

[Junio C Hamano]

Jack Bates  writes:

> The three cases where "git diff" operates outside of a repository are 1)
> when we run it outside of a repository, 2) when one of the files we're
> comparing is outside of the repository we're in, and 3) the --no-index
> option. Commit 4f03666 ("diff: handle sha1 abbreviations outside of
> repository", 2016-10-20) only worked in the first case.
> ---
>  builtin/diff.c  |  4 +++-
>  t/t4063-diff-no-index-abbrev.sh | 50 
> +
>  2 files changed, 53 insertions(+), 1 deletion(-)
>  create mode 100755 t/t4063-diff-no-index-abbrev.sh
>
> diff --git a/builtin/diff.c b/builtin/diff.c
> index 7f91f6d..ec7c432 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -342,9 +342,11 @@ int cmd_diff(int argc, const char **argv, const char 
> *prefix)
>  "--no-index" : "[--no-index]");
>  
>   }
> - if (no_index)
> + if (no_index) {
>   /* If this is a no-index diff, just run it and exit there. */
> + startup_info->have_repository = 0;
>   diff_no_index(, argc, argv);
> + }

This kind of change makes me nervous (partly because I am not seeing
the whole code but only this part of the patch).

Some code may react to "have_repository" being zero and do the right
thing (which I think is what you are using from your previous "we
did one of the three cases" change here), but the codepath that led
to "have_repository" being set to non-zero previously must have done
a lot more than just flipping that field to non-zero, and setting
zero to this field alone would not "undo" what it did.

I wonder if we're better off if we made sure that diff_no_index()
works the same way regardless of the value of "have_repository"
field?

Re: Git v2.11.0 breaks max depth nested alternates

[Jeff King]

On Sun, Dec 04, 2016 at 11:22:52AM -, Philip Oakley wrote:

> > Ever since 722ff7f876 (receive-pack: quarantine objects until
> > pre-receive accepts, 2016-10-03, v2.11.0), Git has been quarantining
> > objects and packs received during an incoming push into a separate
> > objects directory and using the alternates mechanism to make them
> > available until they are either accepted and moved into the main
> > objects directory or rejected and discarded.
> 
> Is there a step here that after the accepted/rejected stage, it should then
> decrement the limit back to its original value. The problem description
> suggests that might be the case.

No. I thought that at first, too, but this increment happens in the
sub-process which is using the extra level of alternates for its entire
lifetime. So it "resets" it by exiting, and the parent process never
increments its internal value at all.

-Peff

Re: Git v2.11.0 breaks max depth nested alternates

[Jeff King]

On Sun, Dec 04, 2016 at 01:37:00AM -0800, Kyle J. McKay wrote:

> On Dec 3, 2016, at 20:55, Jeff King wrote:
> 
> > So I do think this is worth dealing with, but I'm also curious why
> > you're hitting the depth-5 limit. I'm guessing it has to do with hosting
> > a hierarchy of related repos. But is your system then always in danger
> > of busting the 5-limit if people create too deep a repository hierarchy?
> 
> No we check for the limit.  Anything at the limit gets broken by the
> quarantine change though.

OK. So the limit is an issue for your system, but one that you're able
to deal gracefully with (and the quarantine change makes that a lot
harder). I buy that line of reasoning.

> The patch is a step on that road.  It doesn't go that far but all it would
> take is connecting the introduced variable to a config item.  But you still
> need to bump it by 1 during quarantine operations.  Such support would even
> allow alternates to be disallowed (except during quarantine).  I wonder if
> there's an opportunity for further pack operation optimizations in such a
> case (you know there are no alternates because they're not allowed)?

I doubt it. We look at the list of alternates early on, and in most
cases there aren't any. So any optimization there can be done already at
that point.

The only optimization I know if in that area is 56dfeb626 (pack-objects:
compute local/ignore_pack_keep early, 2016-07-29), which works already.

> All true.  And I had similar thoughts.  Perhaps we should add your comments
> to the patch description?  There seems to be a trend towards having longer
> patch descriptions these days... ;)

Feel free to pick out anything that's useful and add it in verbatim or
rephrased, whichever is more convenient.

> You took the words right out of my mouth...   I guess I need to work on
> doing a better job of dumping my stream-of-thoughts that go into a patch
> into the emails to the list.

It's a lot easier when you're the reviewer, because you don't start
reading through the commit-message with a full understanding of the
problem yet. :)

> Most all of your comments could be dumped into the patch description as-is
> to pimp it out some.  I have no objection to that, even adding an
> "Additional-analysis-by:" (or similar) credit line too.  :)

Sure. I don't really need credit, or even just "reviewed-by" is fine.
Talking and generating a shared understanding of the problem is part of
the review process.

-Peff

Re: [PATCH v2] diff: handle --no-abbrev outside of repository

[Jeff King]

On Mon, Dec 05, 2016 at 01:15:00AM -0500, Jeff King wrote:

> On Mon, Dec 05, 2016 at 01:01:16AM -0500, Jeff King wrote:
> 
> >   Note that setting abbrev to "0" outside of a repository was broken
> >   recently by 4f03666ac (diff: handle sha1 abbreviations outside of
> >   repository, 2016-10-20). It adds a special out-of-repo code path for
> >   handling abbreviations which behaves differently than find_unique_abbrev()
> >   by truly giving a zero-length sha1, rather than taking "0" to mean "do
> >   not abbreviate".
> > 
> >   That bug was not triggerable until now, because there was no way to
> >   set the value to zero (using --abbrev=0 silently bumps it to the
> >   MINIMUM_ABBREV).
> 
> Actually, I take this last paragraph back. You _can_ trigger the bug
> with just:
> 
>   echo one >foo
>   echo two >bar
>   git diff --no-index --raw foo bar
> 
> which prints only "..." for each entry.
> 
> I didn't notice it before because without "--raw", we show the patch
> format. That uses the --full-index option, and does not respect --abbrev
> at all (which seems kind of bizarre, but has been that way forever).
> 
> So I think there _is_ a regression in v2.11, and the second half of your
> change fixes it.

Sorry for the sequence of emails, but as usual with "diff --no-index",
the deeper I dig the more confusion I find. :)

After digging into your related thread in:

  
http://public-inbox.org/git/20161205065523.yspqt34p3dp5g...@sigill.intra.peff.net/

I'm not convinced that "--no-index --raw" output isn't generally
nonsensical in the first place. So yes, there's a regression there (and
it's not just "oops, we didn't abbreviate correctly", but rather that
the output format is broken). But I'm not sure it's something people are
using. So it should be fixed on the 'maint' track, but I don't think
it's incredibly urgent.

-Peff

Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository

[Jeff King]

On Sun, Dec 04, 2016 at 12:47:47PM -0700, Jack Bates wrote:

> The three cases where "git diff" operates outside of a repository are 1)
> when we run it outside of a repository, 2) when one of the files we're
> comparing is outside of the repository we're in, and 3) the --no-index
> option. Commit 4f03666 ("diff: handle sha1 abbreviations outside of
> repository", 2016-10-20) only worked in the first case.

You didn't define "worked" here. From looking at your patch, it looks
like we look look in the object database for abbreviations in the
--no-index case, but you think we shouldn't.

I'm not sure I agree. The "--no-index" option asks git not to treat the
arguments as pathspecs, but instead as two filesystem files to diff.
But should it ignore the repository entirely? One use case is to just
ask for the diff of two files:

  git diff --no-index foo bar

which may or may not be tracked in the repository. For abbreviations, I
doubt that people would care much, but see below.

> diff --git a/builtin/diff.c b/builtin/diff.c
> index 7f91f6d..ec7c432 100644
> --- a/builtin/diff.c
> +++ b/builtin/diff.c
> @@ -342,9 +342,11 @@ int cmd_diff(int argc, const char **argv, const char 
> *prefix)
>  "--no-index" : "[--no-index]");
>  
>   }
> - if (no_index)
> + if (no_index) {
>   /* If this is a no-index diff, just run it and exit there. */
> + startup_info->have_repository = 0;
>   diff_no_index(, argc, argv);
> + }

This reset of have_repository would affect more than just abbreviations.
We may also look at the repository for attributes, config, etc.  For
instance, right now:

  echo "*.pdf diff=pdf" >.git/info/attributes
  git config diff.pdf.textconv pdftotext
  git diff --no-index --textconv foo.pdf bar.pdf

will show a text diff of the two files, but wouldn't after your patch.

(I actually think even needing to say --textconv is actually a bug;
normally the diff porcelain defaults to --textconv, but that setup is
skipped in the no-index case).

> +To check that we don'\''t, create an blob with a SHA-1 that starts with
> +. (Outside of a repository, SHA-1s are all zeros.) Then make an
> +abbreviation and check that Git doesn'\''t lengthen it.

That's not always true. If we actually diff the file contents, the sha1s
are correct (which you can see in the default --patch output). It's only
in the --raw case that the sha1 is all zeroes.

I'm not 100% sure that isn't a bug.  In a normal git diff, we can show
the sha1s without actually looking at the file content, because we get
them from either the containing tree or the index. In a --no-index diff,
we create the diff_filespec structs without a valid sha1. But we can't
get away from reading the files eventually. The magic happens in
diffcore_skip_stat_unmatch(), which actually does a series of checks,
culminating in a size-check and then a comparison of the contents.

I suppose it _is_ faster than computing the actual sha1, because we can
sometimes show "modified" by only looking at the size, or the first few
bytes. But any time two files are identical, we pay the full cost. So if
you're comparing two hierarchies, say, like:

  git diff --raw one/ two/

it's probably not much more expensive to compare with the full sha1s,
because we're already reading the entire contents of every file that's
the same.

So I dunno. It kind of surprised me that anybody was using "--no-index
--raw" in the first place, so maybe there is some use case I'm missing.

-Peff

Re: [PATCH v2] diff: handle --no-abbrev outside of repository

[Jeff King]

On Mon, Dec 05, 2016 at 01:01:16AM -0500, Jeff King wrote:

>   Note that setting abbrev to "0" outside of a repository was broken
>   recently by 4f03666ac (diff: handle sha1 abbreviations outside of
>   repository, 2016-10-20). It adds a special out-of-repo code path for
>   handling abbreviations which behaves differently than find_unique_abbrev()
>   by truly giving a zero-length sha1, rather than taking "0" to mean "do
>   not abbreviate".
> 
>   That bug was not triggerable until now, because there was no way to
>   set the value to zero (using --abbrev=0 silently bumps it to the
>   MINIMUM_ABBREV).

Actually, I take this last paragraph back. You _can_ trigger the bug
with just:

  echo one >foo
  echo two >bar
  git diff --no-index --raw foo bar

which prints only "..." for each entry.

I didn't notice it before because without "--raw", we show the patch
format. That uses the --full-index option, and does not respect --abbrev
at all (which seems kind of bizarre, but has been that way forever).

So I think there _is_ a regression in v2.11, and the second half of your
change fixes it.

-Peff

Re: [PATCH v2] diff: handle --no-abbrev outside of repository

[Jeff King]

On Fri, Dec 02, 2016 at 11:48:40AM -0700, Jack Bates wrote:

> The "git diff --no-index" codepath didn't handle the --no-abbrev option.
> Also it didn't behave the same as find_unique_abbrev()
> in the case where abbrev == 0.
> find_unique_abbrev() returns the full, unabbreviated string in that
> case, but the "git diff --no-index" codepath returned an empty string.

If you've dug into what's wrong, I think it's often good to add some
notes in the commit message in case somebody has to revisit this later.

For example, I'd have written something like:

  The "git diff --no-index" codepath doesn't handle the --no-abbrev
  option, because it relies on diff_opt_parse(). Normally that function
  is called as part of handle_revision_opt(), which handles the abbrev
  options itself. Adding the option directly to diff_opt_parse() fixes
  this. We don't need to do the same for --abbrev, because it's already
  handled there.

  Note that setting abbrev to "0" outside of a repository was broken
  recently by 4f03666ac (diff: handle sha1 abbreviations outside of
  repository, 2016-10-20). It adds a special out-of-repo code path for
  handling abbreviations which behaves differently than find_unique_abbrev()
  by truly giving a zero-length sha1, rather than taking "0" to mean "do
  not abbreviate".

  That bug was not triggerable until now, because there was no way to
  set the value to zero (using --abbrev=0 silently bumps it to the
  MINIMUM_ABBREV).

>  t/t4013-diff-various.sh | 7 +++
>  t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir  | 3 +++
>  t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
>  t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
>  t/t4013/diff.diff_--raw_--abbrev=4_initial  | 6 ++
>  t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++
>  t/t4013/diff.diff_--raw_initial | 6 ++

I wondered if the tests without --no-index were redundant with earlier
ones, but I don't think so. --abbrev=4 is tested with diff-tree, but
--no-abbrev is not covered at all, AFAICT.

>  diff.c  | 6 +-

The actual code changes look good to me.

Thanks.

-Peff

Re: Should reset_revision_walk clear more flags?

[Jeff King]

On Mon, Dec 05, 2016 at 08:09:58AM +0900, Mike Hommey wrote:

> While trying to use the revision walking API twice in a row, I noticed
> that the second time for the same setup would not yield the same result.
> In my case, it turns out I was requesting boundaries, and
> reset_revision_walk() is not resetting CHILD_SHOWN and BOUNDARY, both
> required to be reset for the second revision walk to work.
> 
> So the question is, are consumers supposed to reset those flags on their
> own, or should reset_revision_walk clear them?

I would think that reset_revision_walk() should reset any flags set by
the revision-walking code (so anything set by calling init_revisions(),
and then either a call into list_objects() or repeated calls of
get_revision()).

Which I think would include both of the flags you mentioned, along with
others like SYMMETRIC_LEFT, PATCHSAME, etc. Probably really everything
mentioned in revision.h, which should be summed up as ALL_REV_FLAGS.
Some callsites already seem to feed that to clear_commit_marks().

I doubt you can go too wrong by clearing more flags. It's possible that
some caller is relying on a flag _not_ being cleared between two
traversals, but in that case it should probably be using
clear_commit_marks() or clear_object_flags() explicitly to make it clear
what it expects to be saved.

-Peff

Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

[Jeff King]

On Sun, Dec 04, 2016 at 08:45:59PM +, Ramsay Jones wrote:

> 
> Signed-off-by: Ramsay Jones 
> ---
> 
> Hi Junio,
> 
> I recently noticed that:
> 
> $ make >pout 2>&1
> $ ./git version
> git version 2.11.0.286.g109e8a9
> $ git describe
> v2.11.0-286-g109e8a99d
> $
> 
> ... for non-release builds, the commit part of the version
> string was still using an --abbrev=7.

It seems like this kind of discussion ought to go in the commit message.
:)

That said, I think the right patch may be to just drop --abbrev
entirely. Its use goes all the way back to 9b88fcef7 (Makefile: use
git-describe to mark the git version., 2005-12-27), where it was
--abbrev=4. That became --abbrev=7 in bf505158d (Git 1.7.10.1,
2012-05-01) without further comment.

I think at that point it was a noop, as 7 should have been the default.
And now we probably ought to drop it, so that we can use the
auto-scaling default.

-Peff

Re: Error after calling git difftool -d with

[David Aguilar]

On Fri, Dec 02, 2016 at 05:05:06PM +0100, Johannes Schindelin wrote:
> Hi Peter,
> 
> On Fri, 2 Dec 2016, P. Duijst wrote:
> 
> > Incase filenames are used with a quote ' or a bracket [  (and maybe some 
> > more
> > characters), git "diff" and "difftool -y" works fine, but git *difftool 
> > **-d*
> > gives the next error message:
> > 
> >peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> >$ git diff
> >diff --git a/Test ''inch.txt b/Test ''inch.txt
> >index dbff793..41f3257 100644
> >--- a/Test ''inch.txt
> >+++ b/Test ''inch.txt
> >@@ -1 +1,3 @@
> >+
> >+ddd
> >  Test error in simple repository
> >warning: LF will be replaced by CRLF in Test ''inch.txt.
> >The file will have its original line endings in your working directory.
> > 
> >peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> >*$ git difftool -d*
> >*fatal: Cannot open '/d/Dev/test//Test ''inch.txt': No such file or
> >directory*
> >*hash-object /d/Dev/test//Test ''inch.txt: command returned error: 128*
> > 
> >peter@scm_ws_10 MINGW64 /d/Dev/test (master)
> >$
> > 
> > 
> > This issue is inside V2.10.x and V2.11.0.
> > V2.9.0 is working correctly...
> 
> You say v2.11.0, but did you also try the new, experimental builtin
> difftool? You can test without reinstalling:
> 
>   git -c difftool.useBuiltin=true difftool -d ...

FWIW, I verified that this problem does not manifest itself on
Linux, using the current scripted difftool.

Peter, what actual diff tool are you using?

Since these filenames work fine with "difftool -d" on Linux, it
suggests that this is either a tool-specific issue, or an issue
related to unix-to-windows path translation.
-- 
David

Re: Where is Doc to configure Git + Apache + kerberos for Project level access in repo?

[brian m. carlson]

On Fri, Dec 02, 2016 at 05:21:53PM -0500, Jeff King wrote:
> On Fri, Dec 02, 2016 at 01:15:02PM -0500, ken edward wrote:
> 
> > Where is Doc to configure Git + Apache + kerberos for Project level
> > access in repo?
> 
> I don't know about Kerberos, but all of the documentation in git for
> configuring Apache is found in "git help http-backend".

If you want to use Kerberos for authentication, it's really as simple as
just using "AuthType Kerberos" instead of "AuthType Basic", plus
whatever Kerberos parameters you want.

This is what my configuration looks like, but obviously you'll want to
modify it a bit:

  AuthType Kerberos
  AuthName "Kerberos Login"
  KrbMethodNegotiate on
  KrbMethodK5Passwd off
  KrbAuthRealms CRUSTYTOOTHPASTE.NET
  Krb5Keytab /etc/krb5.apache.keytab

You may also want to set http.emptyAuth on the client side.
-- 
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: Should reset_revision_walk clear more flags?

[Mike Hommey]

On Mon, Dec 05, 2016 at 08:09:58AM +0900, Mike Hommey wrote:
> Hi,
> 
> While trying to use the revision walking API twice in a row, I noticed
> that the second time for the same setup would not yield the same result.
> In my case, it turns out I was requesting boundaries, and
> reset_revision_walk() is not resetting CHILD_SHOWN and BOUNDARY, both
> required to be reset for the second revision walk to work.

and TREESAME when using different pathspecs.

Mike

Should reset_revision_walk clear more flags?

[Mike Hommey]

Hi,

While trying to use the revision walking API twice in a row, I noticed
that the second time for the same setup would not yield the same result.
In my case, it turns out I was requesting boundaries, and
reset_revision_walk() is not resetting CHILD_SHOWN and BOUNDARY, both
required to be reset for the second revision walk to work.

So the question is, are consumers supposed to reset those flags on their
own, or should reset_revision_walk clear them?

Mike

Re: difftool -d not populating left correctly when not in git root

[David Aguilar]

On Fri, Dec 02, 2016 at 03:04:01PM -0800, Frank Becker wrote:
> Hi,
> 
> looks like this broke between 2.9.2 and 2.9.3
> 
> cat ~/.gitconfig
> [difftool "diff"]
> cmd = ls -l ${LOCAL}/* ${REMOTE}/*
> #cmd = diff -r ${LOCAL} ${REMOTE} | less
> 
> ~/stuff/gittest> ls -l *
> d1:
> total 8
> -rw-r--r--  1 frank  staff  16  2 Dec 14:30 test.txt
> 
> d2:
> total 8
> -rw-r--r--  1 frank  staff  18  2 Dec 14:30 anothertest.tst
> 
> 
> ~/stuff/gittest> git status
> On branch master
> Changes not staged for commit:
>   (use "git add ..." to update what will be committed)
>   (use "git checkout -- ..." to discard changes in working directory)
> 
> modified:   d1/test.txt
> modified:   d2/anothertest.tst
> 
> 
> ~/stuff/gittest> ~/stuff/git_tmp/bin/git --version
> git version 2.11.0
> 
> ~/stuff/gittest> ~/stuff/git_tmp/bin/git difftool -d -t diff
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.0oGRF/left/d1:
> total 8
> -rw-r--r--  1 frank  staff  6  2 Dec 14:52 test.txt
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.0oGRF/left/d2:
> total 8
> -rw-r--r--  1 frank  staff  7  2 Dec 14:52 anothertest.tst
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.0oGRF/right/d1:
> total 8
> lrwxr-xr-x  1 frank  staff  38  2 Dec 14:52 test.txt ->
> /Users/frank/stuff/gittest/d1/test.txt
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.0oGRF/right/d2:
> total 8
> lrwxr-xr-x  1 frank  staff  45  2 Dec 14:52 anothertest.tst ->
> /Users/frank/stuff/gittest/d2/anothertest.tst
> 
> 
> cd d2
> ~/stuff/gittest/d2> ~/stuff/git_tmp/bin/git difftool -d -t diff
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.eRXhB/left/d2:
> total 8
> -rw-r--r--  1 frank  staff  7  2 Dec 14:52 anothertest.tst
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.eRXhB/right/d1:
> total 8
> lrwxr-xr-x  1 frank  staff  38  2 Dec 14:52 test.txt ->
> /Users/frank/stuff/gittest/d1/test.txt
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.eRXhB/right/d2:
> total 8
> lrwxr-xr-x  1 frank  staff  45  2 Dec 14:52 anothertest.tst ->
> /Users/frank/stuff/gittest/d2/anothertest.tst
> 
> 
> Note that left does not contain d1
> 
> 
> 
> ~/stuff/gittest/d2> ~/stuff/git_tmp/bin/git --version
> git version 2.9.2
> ~/stuff/gittest/d2> ~/stuff/git_tmp/bin/git difftool -d -t diff
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.YxtVw/left/d1:
> total 8
> -rw-r--r--  1 frank  staff  6  2 Dec 15:02 test.txt
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.YxtVw/left/d2:
> total 8
> -rw-r--r--  1 frank  staff  7  2 Dec 15:02 anothertest.tst
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.YxtVw/right/d1:
> total 8
> lrwxr-xr-x  1 frank  staff  38  2 Dec 15:02 test.txt ->
> /Users/frank/stuff/gittest/d1/test.txt
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.YxtVw/right/d2:
> total 8
> lrwxr-xr-x  1 frank  staff  45  2 Dec 15:02 anothertest.tst ->
> /Users/frank/stuff/gittest/d2/anothertest.tst
> 
> 
> 
> ~/stuff/gittest/d2> ~/stuff/git_tmp/bin/git --version
> git version 2.9.3
> ~/stuff/gittest/d2> ~/stuff/git_tmp/bin/git difftool -d -t diff
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.TpJ5u/left/d2:
> total 8
> -rw-r--r--  1 frank  staff  7  2 Dec 15:01 anothertest.tst
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.TpJ5u/right/d1:
> total 8
> lrwxr-xr-x  1 frank  staff  38  2 Dec 15:01 test.txt ->
> /Users/frank/stuff/gittest/d1/test.txt
> 
> /var/folders/0j/3pk3pdsx7rzb9_njdpyjwm00gn/T/git-difftool.TpJ5u/right/d2:
> total 8
> lrwxr-xr-x  1 frank  staff  45  2 Dec 15:01 anothertest.tst ->
> /Users/frank/stuff/gittest/d2/anothertest.tst

This regression was not caught by our test suite.

This looks like it's an edge case not handled by:
9ec26e797781 "difftool: fix argument handling in subdirs"
The current "rewrite difftool in C" topic may need a
similar adjustment.

The problem:

When preparing the right-side of the diff we only construct the
parts that changed.  When constructing the left side we
construct a full index, but we were constructing it relative to
the subdirectory, and thus it ends up empty because we are in a
subdirectory and the paths are incorrect.

The fix seems simple -- when preparing the index files we need
to chdir to the toplevel to ensure that the index construction
steps find the correct toplevel-relative paths.

Thanks for the heads-up,
David

--- 8< ---
Date: Sun, 4 Dec 2016 14:27:17 -0800
Subject: [PATCH] difftool: properly handle being launched from a subdirectory

9ec26e797781239b36ebccb87c590e5778358007 corrected how path arguments
are handled in a subdirectory, but it introduced a regression in how
entries outside of the subdirectory are handled by the dir-diff.

When preparing the right-side of the diff we only construct the parts
that changed.

When constructing the left side we construct an index, but we 

[PATCH] clone,fetch: explain the shallow-clone option a little more clearly

[Alex Henrie]

"deepen by excluding" does not make sense because excluding a revision
does not deepen a repository; it makes the repository more shallow.

Signed-off-by: Alex Henrie 
---
 builtin/clone.c | 2 +-
 builtin/fetch.c | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/clone.c b/builtin/clone.c
index 6c76a6e..e3cb808 100644
--- a/builtin/clone.c
+++ b/builtin/clone.c
@@ -99,7 +99,7 @@ static struct option builtin_clone_options[] = {
OPT_STRING(0, "shallow-since", _since, N_("time"),
N_("create a shallow clone since a specific time")),
OPT_STRING_LIST(0, "shallow-exclude", _not, N_("revision"),
-   N_("deepen history of shallow clone by excluding rev")),
+   N_("deepen history of shallow clone, excluding rev")),
OPT_BOOL(0, "single-branch", _single_branch,
N_("clone only one branch, HEAD or --branch")),
OPT_BOOL(0, "shallow-submodules", _shallow_submodules,
diff --git a/builtin/fetch.c b/builtin/fetch.c
index b6a5597..fc74c84 100644
--- a/builtin/fetch.c
+++ b/builtin/fetch.c
@@ -122,7 +122,7 @@ static struct option builtin_fetch_options[] = {
OPT_STRING(0, "shallow-since", _since, N_("time"),
   N_("deepen history of shallow repository based on time")),
OPT_STRING_LIST(0, "shallow-exclude", _not, N_("revision"),
-   N_("deepen history of shallow clone by excluding rev")),
+   N_("deepen history of shallow clone, excluding rev")),
OPT_INTEGER(0, "deepen", _relative,
N_("deepen history of shallow clone")),
{ OPTION_SET_INT, 0, "unshallow", , NULL,
-- 
2.10.2

[PATCH] receive-pack: improve English grammar of denyCurrentBranch message

[Alex Henrie]

The article "the" is required here.

Signed-off-by: Alex Henrie 
---
 builtin/receive-pack.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
index e6b3879..6b97cbd 100644
--- a/builtin/receive-pack.c
+++ b/builtin/receive-pack.c
@@ -795,8 +795,8 @@ static char *refuse_unconfigured_deny_msg =
   "with what you pushed, and will require 'git reset --hard' to 
match\n"
   "the work tree to HEAD.\n"
   "\n"
-  "You can set 'receive.denyCurrentBranch' configuration variable to\n"
-  "'ignore' or 'warn' in the remote repository to allow pushing into\n"
+  "You can set the 'receive.denyCurrentBranch' configuration 
variable\n"
+  "to 'ignore' or 'warn' in the remote repository to allow pushing 
into\n"
   "its current branch; however, this is not recommended unless you\n"
   "arranged to update its work tree to match what you pushed in some\n"
   "other way.\n"
-- 
2.10.2

[PATCH] bisect: improve English grammar of not-ancestors message

[Alex Henrie]

Multiple revisions cannot be a single ancestor.

Signed-off-by: Alex Henrie 
---
 bisect.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/bisect.c b/bisect.c
index 21bc6da..8e63c40 100644
--- a/bisect.c
+++ b/bisect.c
@@ -747,7 +747,7 @@ static void handle_bad_merge_base(void)
exit(3);
}
 
-   fprintf(stderr, _("Some %s revs are not ancestor of the %s rev.\n"
+   fprintf(stderr, _("Some %s revs are not ancestors of the %s rev.\n"
"git bisect cannot work properly in this case.\n"
"Maybe you mistook %s and %s revs?\n"),
term_good, term_bad, term_good, term_bad);
-- 
2.10.2

[RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling

[Ramsay Jones]

Signed-off-by: Ramsay Jones 
---

Hi Junio,

I recently noticed that:

$ make >pout 2>&1
$ ./git version
git version 2.11.0.286.g109e8a9
$ git describe
v2.11.0-286-g109e8a99d
$

... for non-release builds, the commit part of the version
string was still using an --abbrev=7.

I don't know that it actually matters too much (since it will
show as many as necessary, thus the RFC), but it caused me to
look twice. ;-)

ATB,
Ramsay Jones

 GIT-VERSION-GEN | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 520d6e66e..05601f753 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -12,7 +12,7 @@ if test -f version
 then
VN=$(cat version) || VN="$DEF_VER"
 elif test -d ${GIT_DIR:-.git} -o -f .git &&
-   VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) &&
+   VN=$(git describe --match "v[0-9]*" --abbrev=9 HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
v[0-9]*)
-- 
2.11.0

Re: [BUG] git gui can't commit multiple files

[David Aguilar]

On Sun, Dec 04, 2016 at 05:36:46PM +0100, Timon wrote:
> This is a regression in git 2.11.0 (version 2.10.2 is fine).
> 
> In git-gui I select multiple files in the Unstaged Changes (using
> shift+click) and press ctrl+t to stage them. Then only one files gets
> staged instead of all of the selected files.
> The same happens when unstaging files.
> 
> Git-cola also exhibits the same behavior. Although there I could stage
> multiple files if I used a popup menu instead of the keyboard shortcut
> (I'm guessing it goes through a different code path?).

Can you elaborate a bit?

I just tested git-cola with Git 2.11 and it worked fine for me.
I selected several files and used the Ctrl+s hotkey to stage the
selected files.  They all got staged.

If you have a test repo, or reproduction recipe, I'd be curious
to try it out.
-- 
David

[PATCH] diff: fix up SHA-1 abbreviations outside of repository

[Jack Bates]

The three cases where "git diff" operates outside of a repository are 1)
when we run it outside of a repository, 2) when one of the files we're
comparing is outside of the repository we're in, and 3) the --no-index
option. Commit 4f03666 ("diff: handle sha1 abbreviations outside of
repository", 2016-10-20) only worked in the first case.
---
 builtin/diff.c  |  4 +++-
 t/t4063-diff-no-index-abbrev.sh | 50 +
 2 files changed, 53 insertions(+), 1 deletion(-)
 create mode 100755 t/t4063-diff-no-index-abbrev.sh

diff --git a/builtin/diff.c b/builtin/diff.c
index 7f91f6d..ec7c432 100644
--- a/builtin/diff.c
+++ b/builtin/diff.c
@@ -342,9 +342,11 @@ int cmd_diff(int argc, const char **argv, const char 
*prefix)
   "--no-index" : "[--no-index]");
 
}
-   if (no_index)
+   if (no_index) {
/* If this is a no-index diff, just run it and exit there. */
+   startup_info->have_repository = 0;
diff_no_index(, argc, argv);
+   }
 
/* Otherwise, we are doing the usual "git" diff */
rev.diffopt.skip_stat_unmatch = !!diff_auto_refresh_index;
diff --git a/t/t4063-diff-no-index-abbrev.sh b/t/t4063-diff-no-index-abbrev.sh
new file mode 100755
index 000..d1d6302
--- /dev/null
+++ b/t/t4063-diff-no-index-abbrev.sh
@@ -0,0 +1,50 @@
+#!/bin/sh
+
+test_description='don'\'' peek into .git when operating outside of a repository
+
+When abbreviating SHA-1s, if another object in the repository has the
+same abbreviation, we normally lengthen the abbreviation until it'\''s
+unique. Commit 4f03666 ("diff: handle sha1 abbreviations outside of
+repository", 2016-10-20) addressed the case of abbreviating SHA-1s
+outside the context of a repository. In that case we shouldn'\''t peek
+into a .git directory to make an abbreviation unique.
+
+To check that we don'\''t, create an blob with a SHA-1 that starts with
+. (Outside of a repository, SHA-1s are all zeros.) Then make an
+abbreviation and check that Git doesn'\''t lengthen it.
+
+The three cases where "git diff" operates outside of a repository are
+1) when we run it outside of a repository, 2) when one of the files
+we'\''re comparing is outside of the repository we'\''re in,
+and 3) the --no-index option.
+'
+
+. ./test-lib.sh
+
+test_expect_success setup '
+   echo 1 >a &&
+   echo 2 >b &&
+   git init repo &&
+   (
+   cd repo &&
+
+   # Create a blob
+   # 2e907f44c3881822c473d8842405cfd96362
+   echo 119132 >collision &&
+   git add collision
+   )
+'
+
+cat >expect 

Re: git reset --hard should not irretrievably destroy new files

[Junio C Hamano]

Julian de Bhal  writes:

> The behaviour that would make the most sense to me (personally) would be
> for a hard reset to unstage new files,...

I think _sometimes_ that may be useful.  I haven't thought things
through yet to arrive the final decision, but one thing that must be
kept in mind by anybody who wants to move this topic forward is that
a path that does not exist in the HEAD commit MUST be removed from
the index and the working tree upon "git reset --hard" when the path
resulted from a mergy operation.  I.e. in this sequence:

$ git merge other-branch ;# or git cherry-pick one-commit
... try to resolve conflicts, make a mess, decide
... to try it again from scratch
$ git reset --hard
$ git merge other-branch ;# or git cherry-pick one-commit

the "reset --hard" step MUST remove new paths that existed only on
the other-branch (or in one-commit), which by definition would have
auto-resolved cleanly, from the index and the working tree.  There
are other commands (e.g. "git am -3", "git apply -3", "git rebase")
that are "mergy" and their intermediate states must be handled the
same way.

If a very simple to explain and understand rule can be used to tell
if a new path (i.e. a path that exists in the index and in the
working tree, but is not in the HEAD commit) is what was created
manually by the end user without any other copy (i.e. "create
newfile && edit newfile && git add newfile") and is not a result of
a mergy operation being abandoned, then I think it is OK to allow
"reset --hard" to leave the working tree files untracked, but if the
rule becomes anything complex to understand for the users, I think
it would make the behaviour of "reset --hard" hard to explain,
understand AND anticipate---and at that point, we would be better
off keeping the "You said 'hard', we clean 'hard' to match HEAD"
behaviour of "reset --hard" and EDUCATE users not to say 'hard' too
casually. There may be a room for new option that unconditionally
leave the new working tree files untracked so that users can choose
between the two, if we end up going that route.

[BUG] git gui can't commit multiple files

[Timon]

This is a regression in git 2.11.0 (version 2.10.2 is fine).

In git-gui I select multiple files in the Unstaged Changes (using
shift+click) and press ctrl+t to stage them. Then only one files gets
staged instead of all of the selected files.
The same happens when unstaging files.

Git-cola also exhibits the same behavior. Although there I could stage
multiple files if I used a popup menu instead of the keyboard shortcut
(I'm guessing it goes through a different code path?).

Note that I tested by reverting back to 2.10.2 and verified that
everything works, so I'm quite certain that this is a regression in
git.

[PATCH v1] git-p4: fix empty file processing for large file system backend GitLFS

[larsxschneider]

From: Lars Schneider 

If git-p4 tried to store an empty file in GitLFS then it crashed while
parsing the pointer file:

  oid = re.search(r'^oid \w+:(\w+)', pointerFile, re.MULTILINE).group(1)
  AttributeError: 'NoneType' object has no attribute 'group'

This happens because GitLFS does not create a pointer file for an empty
file. Teach git-p4 this behavior to fix the problem and add a test case.

Signed-off-by: Lars Schneider 
---

Notes:
Base Commit: 454cb6b (v2.11.0)
Diff on Web: 
https://github.com/git/git/compare/454cb6b...larsxschneider:b717fde
Checkout:git fetch https://github.com/larsxschneider/git 
git-p4/empty-files-v1 && git checkout b717fde

 git-p4.py | 29 +
 t/t9824-git-p4-git-lfs.sh |  2 ++
 2 files changed, 19 insertions(+), 12 deletions(-)

diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..ccfb68105f 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -1005,18 +1005,20 @@ class LargeFileSystem(object):
steps."""
 if self.exceedsLargeFileThreshold(relPath, contents) or 
self.hasLargeFileExtension(relPath):
 contentTempFile = self.generateTempFile(contents)
-(git_mode, contents, localLargeFile) = 
self.generatePointer(contentTempFile)
-
-# Move temp file to final location in large file system
-largeFileDir = os.path.dirname(localLargeFile)
-if not os.path.isdir(largeFileDir):
-os.makedirs(largeFileDir)
-shutil.move(contentTempFile, localLargeFile)
-self.addLargeFile(relPath)
-if gitConfigBool('git-p4.largeFilePush'):
-self.pushFile(localLargeFile)
-if verbose:
-sys.stderr.write("%s moved to large file system (%s)\n" % 
(relPath, localLargeFile))
+(pointer_git_mode, contents, localLargeFile) = 
self.generatePointer(contentTempFile)
+if pointer_git_mode:
+git_mode = pointer_git_mode
+if localLargeFile:
+# Move temp file to final location in large file system
+largeFileDir = os.path.dirname(localLargeFile)
+if not os.path.isdir(largeFileDir):
+os.makedirs(largeFileDir)
+shutil.move(contentTempFile, localLargeFile)
+self.addLargeFile(relPath)
+if gitConfigBool('git-p4.largeFilePush'):
+self.pushFile(localLargeFile)
+if verbose:
+sys.stderr.write("%s moved to large file system (%s)\n" % 
(relPath, localLargeFile))
 return (git_mode, contents)
 
 class MockLFS(LargeFileSystem):
@@ -1056,6 +1058,9 @@ class GitLFS(LargeFileSystem):
the actual content. Return also the new location of the actual
content.
"""
+if os.path.getsize(contentFile) == 0:
+return (None, '', None)
+
 pointerProcess = subprocess.Popen(
 ['git', 'lfs', 'pointer', '--file=' + contentFile],
 stdout=subprocess.PIPE
diff --git a/t/t9824-git-p4-git-lfs.sh b/t/t9824-git-p4-git-lfs.sh
index 110a7e7924..734b8db4cb 100755
--- a/t/t9824-git-p4-git-lfs.sh
+++ b/t/t9824-git-p4-git-lfs.sh
@@ -42,6 +42,8 @@ test_expect_success 'Create repo with binary files' '
(
cd "$cli" &&
 
+   >file0.dat &&
+   p4 add file0.dat &&
echo "content 1 txt 23 bytes" >file1.txt &&
p4 add file1.txt &&
echo "content 2-3 bin 25 bytes" >file2.dat &&
-- 
2.11.0

[PATCH] Completion: Add support for --submodule=diff

[peterjclaw]

From: Peter Law 

Teach git-completion.bash about the 'diff' option to 'git diff
--submodule=', which was added in Git 2.11.

Signed-off-by: Peter Law 
---
 contrib/completion/git-completion.bash | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/contrib/completion/git-completion.bash 
b/contrib/completion/git-completion.bash
index 21016bf8d..ab11e7371 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -1206,7 +1206,7 @@ _git_describe ()
 
 __git_diff_algorithms="myers minimal patience histogram"
 
-__git_diff_submodule_formats="log short"
+__git_diff_submodule_formats="diff log short"
 
 __git_diff_common_options="--stat --numstat --shortstat --summary
--patch-with-stat --name-only --name-status --color
-- 
2.11.0

[PATCH v1] git-p4: add config to retry p4 commands; retry 3 times by default

[larsxschneider]

From: Lars Schneider 

P4 commands can fail due to random network issues. P4 users can counter
these issues by using a retry flag supported by all p4 commands [1].

Add an integer Git config value `git-p4.retries` to define the number of
retries for all p4 invocations. If the config is not defined then set
the default retry count to 3.

[1] 
https://www.perforce.com/perforce/doc.current/manuals/cmdref/global.options.html

Signed-off-by: Lars Schneider 
---

Notes:
Base Commit: 454cb6b (v2.11.0)
Diff on Web: 
https://github.com/git/git/compare/454cb6b...larsxschneider:654c727
Checkout:git fetch https://github.com/larsxschneider/git 
git-p4/retries-v1 && git checkout 654c727

 Documentation/git-p4.txt | 4 
 git-p4.py| 5 +
 2 files changed, 9 insertions(+)

diff --git a/Documentation/git-p4.txt b/Documentation/git-p4.txt
index c83aaf39c3..656587248c 100644
--- a/Documentation/git-p4.txt
+++ b/Documentation/git-p4.txt
@@ -467,6 +467,10 @@ git-p4.client::
Client specified as an option to all p4 commands, with
'-c ', including the client spec.
 
+git-p4.retries::
+   Specifies the number of times to retry a p4 command (notably,
+   'p4 sync') if the network times out. The default value is 3.
+
 Clone and sync variables
 
 git-p4.syncFromOrigin::
diff --git a/git-p4.py b/git-p4.py
index fd5ca52462..2422178210 100755
--- a/git-p4.py
+++ b/git-p4.py
@@ -78,6 +78,11 @@ def p4_build_cmd(cmd):
 if len(client) > 0:
 real_cmd += ["-c", client]
 
+retries = gitConfigInt("git-p4.retries")
+if retries is None:
+# Perform 3 retries by default
+retries = 3
+real_cmd += ["-r", str(retries)]
 
 if isinstance(cmd,basestring):
 real_cmd = ' '.join(real_cmd) + ' ' + cmd
-- 
2.11.0

[PATCH v1] travis-ci: update P4 to 16.2 and GitLFS to 1.5.2 in Linux build

[larsxschneider]

From: Lars Schneider 

Update Travis-CI dependencies to the latest available versions in
Linux build.

Signed-off-by: Lars Schneider 
---

Notes:
Base Commit: 454cb6b (v2.11.0)
Diff on Web: https://git.io/test_ls_travisci_dep-update-v1.diff
Checkout:git fetch https://github.com/larsxschneider/git 
travisci/dep-update-v1 && git checkout 421365c

 .travis.yml | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/.travis.yml b/.travis.yml
index 0b2ea5c3e2..3843967a69 100644
--- a/.travis.yml
+++ b/.travis.yml
@@ -27,8 +27,8 @@ env:
 # The Linux build installs the defined dependency versions below.
 # The OS X build installs the latest available versions. Keep that
 # in mind when you encounter a broken OS X build!
-- LINUX_P4_VERSION="16.1"
-- LINUX_GIT_LFS_VERSION="1.2.0"
+- LINUX_P4_VERSION="16.2"
+- LINUX_GIT_LFS_VERSION="1.5.2"
 - DEFAULT_TEST_TARGET=prove
 - GIT_PROVE_OPTS="--timer --jobs 3 --state=failed,slow,save"
 - GIT_TEST_OPTS="--verbose-log"
-- 
2.11.0

[PATCH v1] t0021: minor filter process test cleanup

[larsxschneider]

From: Lars Schneider 

Remove superfluous .gitignore pattern and invalid '.' in `git commit`
calls.

Signed-off-by: Lars Schneider 
---

Notes:
Base Commit: 454cb6b (v2.11.0)
Diff on Web: https://git.io/test_ls_filter-process_cleanup-test-v1.diff
Checkout:git fetch https://github.com/larsxschneider/git 
filter-process/cleanup-test-v1 && git checkout c052a3d

 t/t0021-conversion.sh | 5 ++---
 1 file changed, 2 insertions(+), 3 deletions(-)

diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 4ea534e9fa..34891c4b1a 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -350,10 +350,9 @@ test_expect_success PERL 'required process filter should 
filter data' '
cd repo &&
git init &&
 
-   echo "git-stderr.log" >.gitignore &&
echo "*.r filter=protocol" >.gitattributes &&
git add . &&
-   git commit . -m "test commit 1" &&
+   git commit -m "test commit 1" &&
git branch empty-branch &&
 
cp "$TEST_ROOT/test.o" test.r &&
@@ -378,7 +377,7 @@ test_expect_success PERL 'required process filter should 
filter data' '
EOF
test_cmp_count expected.log rot13-filter.log &&
 
-   filter_git commit . -m "test commit 2" &&
+   filter_git commit -m "test commit 2" &&
cat >expected.log <<-EOF &&
START
init handshake complete
-- 
2.11.0

Re: Git v2.11.0 breaks max depth nested alternates

[Philip Oakley]

From: "Kyle J. McKay" 
Sent: Sunday, December 04, 2016 12:24 AM

The recent addition of pre-receive quarantining breaks nested
alternates that are already at the maximum alternates nesting depth.

In the file sha1_file.c in the function link_alt_odb_entries we have
this:

> if (depth > 5) {
> error("%s: ignoring alternate object stores, nesting too deep.",
> relative_base);
> return;
> }

When the incoming quarantine takes place the current objects directory
is demoted to an alternate thereby increasing its depth (and any
alternates it references) by one and causing any object store that was
previously at the maximum nesting depth to be ignored courtesy of the
above hard-coded maximum depth.

If the incoming push happens to need access to some of those objects
to perhaps "--fix-thin" its pack it will crash and burn.

Originally I was not going to include a patch to fix this, but simply
suggest that the expeditious fix is to just allow one additional
alternates nesting depth level during quarantine operations.

However, it was so simple, I have included the patch below :)

I have verified that where a push with Git v2.10.2 succeeds and a push
with Git v2.11.0 to the same repository fails because of this problem
that the below patch does indeed correct the issue and allow the push
to succeed.

Cheers,

Kyle

-- 8< --
Subject: [PATCH] receive-pack: increase max alternates depth during 
quarantine


Ever since 722ff7f876 (receive-pack: quarantine objects until
pre-receive accepts, 2016-10-03, v2.11.0), Git has been quarantining
objects and packs received during an incoming push into a separate
objects directory and using the alternates mechanism to make them
available until they are either accepted and moved into the main
objects directory or rejected and discarded.


Is there a step here that after the accepted/rejected stage, it should then 
decrement the limit back to its original value. The problem description 
suggests that might be the case.

--
Philip



Unfortunately this has the side effect of increasing the alternates
nesting depth level by one for all pre-existing alternates.

If a repository is already at the maximum alternates nesting depth,
then this quarantining operation can temporarily push it over making
the incoming push fail.

To prevent the failure we simply increase the allowed alternates
nesting depth by one whenever a quarantine operation is in effect.

Signed-off-by: Kyle J. McKay 
---

Notes:
   Some alternates nesting depth background:

   If base/fork0/fork1/fork2/fork3/fork4/fork5 represents
   seven git repositories where base.git has no alternates,
   fork0.git has base.git as an alternate, fork1.git has
   fork0.git as an alternate and so on where fork5.git has
   only fork4.git as an alternate, then fork5.git is at
   the maximum allowed depth of 5.  git fsck --strict --full
   works without complaint on fork5.git.

   However, in base/fork0/fork1/fork2/fork3/fork4/fork5/fork6,
   an fsck --strict --full of fork6.git will generate complaints
   and any objects/packs present in base.git will be ignored.

cache.h   | 1 +
common-main.c | 3 +++
environment.c | 1 +
sha1_file.c   | 2 +-
4 files changed, 6 insertions(+), 1 deletion(-)

diff --git a/cache.h b/cache.h
index a50a61a1..25c17c29 100644
--- a/cache.h
+++ b/cache.h
@@ -676,6 +676,7 @@ extern size_t packed_git_limit;
extern size_t delta_base_cache_limit;
extern unsigned long big_file_threshold;
extern unsigned long pack_size_limit_cfg;
+extern int alt_odb_max_depth;

/*
 * Accessors for the core.sharedrepository config which lazy-load the 
value

diff --git a/common-main.c b/common-main.c
index c654f955..9f747491 100644
--- a/common-main.c
+++ b/common-main.c
@@ -37,5 +37,8 @@ int main(int argc, const char **argv)

 restore_sigpipe_to_default();

+ if (getenv(GIT_QUARANTINE_ENVIRONMENT))
+ alt_odb_max_depth++;
+
 return cmd_main(argc, argv);
}
diff --git a/environment.c b/environment.c
index 0935ec69..32e11f70 100644
--- a/environment.c
+++ b/environment.c
@@ -64,6 +64,7 @@ int merge_log_config = -1;
int precomposed_unicode = -1; /* see probe_utf8_pathname_composition() */
unsigned long pack_size_limit_cfg;
enum hide_dotfiles_type hide_dotfiles = HIDE_DOTFILES_DOTGITONLY;
+int alt_odb_max_depth = 5;

#ifndef PROTECT_HFS_DEFAULT
#define PROTECT_HFS_DEFAULT 0
diff --git a/sha1_file.c b/sha1_file.c
index 9c86d192..15b8432e 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -337,7 +337,7 @@ static void link_alt_odb_entries(const char *alt, int 
len, int sep,

 int i;
 struct strbuf objdirbuf = STRBUF_INIT;

- if (depth > 5) {
+ if (depth > alt_odb_max_depth) {
 error("%s: ignoring alternate object stores, nesting too deep.",
 relative_base);
 return;
---

Re: git reset --hard should not irretrievably destroy new files

[Christian Couder]

On Sun, Dec 4, 2016 at 1:57 AM, Julian de Bhal  wrote:
> On Sat, Dec 3, 2016 at 6:11 PM, Christian Couder
>  wrote:
>> On Sat, Dec 3, 2016 at 6:04 AM, Julian de Bhal  
>> wrote:
>>> but I'd be nearly as happy if a
>>> commit was added to the reflog when the reset happens (I can probably make
>>> that happen with some configuration now that I've been bitten).
>>
>> Not sure if this has been proposed. Perhaps it would be simpler to
>> just output the sha1, and maybe the filenames too, of the blobs, that
>> are no more referenced from the trees, somewhere (in a bloblog?).
>
> Yeah, after doing a bit more reading around the issue, this seems like
> a smaller part of destroying local changes with a hard reset, and I'm
> one of the lucky ones where it is recoverable.

Yeah, but not everyone knows it is recoverable and using fsck to
recover is not nice and easy for the user.
So having a bloblog for example in .git/logs/blobs/, like the reflogs
we already have, but for blobs, could help even if (first) it's just
about writing the filenames and sha1s related to the blobs we stop
referencing.

> Has anyone discussed having `git reset --hard` create objects for the
> current state of anything it's about to destroy, specifically so they
> end up in the --lost-found?

Well, when we start talking about creating new objects, then someone
usually says that it is what "git stash" is about. So the discussion
then often turns to how can we make people more aware of "git stash",
or incite them to create an alias or a shell function that does a "git
stash" before "git reset --hard ...", or teach them to use "git reset
--keep ..." when it does what they want and is safer...

> I think this is what you're suggesting, only without checking for
> references, so that tree & blob objects exist that make any hard reset
> reversible.

I suggest we start with just logging blobs that we have already
created (when they have been "git add"ed) but that we are
dereferencing.
If we can agree on that, it will already help and not be very costly
performance wise. After that we could then start thinking about
creating blobs for all the content we discard, which could be done
only in a beginner mode (at least at first) to make sure it has no
performance impact if people rely on "git reset --hard" being fast.

Re: Git v2.11.0 breaks max depth nested alternates

[Kyle J. McKay]

On Dec 3, 2016, at 20:55, Jeff King wrote:


So I do think this is worth dealing with, but I'm also curious why
you're hitting the depth-5 limit. I'm guessing it has to do with  
hosting

a hierarchy of related repos. But is your system then always in danger
of busting the 5-limit if people create too deep a repository  
hierarchy?


No we check for the limit.  Anything at the limit gets broken by the  
quarantine change though.


Specifically, I'm wondering if it would be sufficient to just bump  
it to

6. Or 100.


Well, if we left the current limit in place, but as you say:


Of course any static bump runs into the funny case where a repo
_usually_ works, but fails when pushed to. Which is kind of nasty and
unintuitive. And your patch fixes that,


Yes.  That's not nice, hence the patch.  Without the fix, pushing  
might work sometimes until you actually need to access cut-off objects  
at pre-receive time.  So you might be able to push sometimes and  
sometimes it breaks.



and we can leave the idea of
bumping the static depth number as an orthogonal issue (that  
personally,

I do not care about much about either way).


The patch is a step on that road.  It doesn't go that far but all it  
would take is connecting the introduced variable to a config item.   
But you still need to bump it by 1 during quarantine operations.  Such  
support would even allow alternates to be disallowed (except during  
quarantine).  I wonder if there's an opportunity for further pack  
operation optimizations in such a case (you know there are no  
alternates because they're not allowed)?



diff --git a/common-main.c b/common-main.c
index c654f955..9f747491 100644
--- a/common-main.c
+++ b/common-main.c
@@ -37,5 +37,8 @@ int main(int argc, const char **argv)

restore_sigpipe_to_default();

+   if (getenv(GIT_QUARANTINE_ENVIRONMENT))
+   alt_odb_max_depth++;
+
return cmd_main(argc, argv);


After reading your problem description, my initial thought was to
increment the counter when we allocate the tmp-objdir, and decrement
when it is destroyed. Because the parent receive-pack process adds  
it to

its alternates, too. But:

 1. Receive-pack doesn't care; it adds the tmp-objdir as an alternate,
rather than adding it as its main object dir and bumping down the
main one.

 2. There would have to be some way of communicating to sub-processes
that they should bump their max-depth by one.


All true.  And I had similar thoughts.  Perhaps we should add your  
comments to the patch description?  There seems to be a trend towards  
having longer patch descriptions these days... ;)



You've basically used the quarantine-path variable as the
inter-process flag for (2). Which feels a little funny, because its
value is unrelated to the alt-odb setup. But it is a reliable  
signal, so

there's a certain elegance. It's probably the best option, given that
the alternative is a specific variable to say "hey, bump your
max-alt-odb-depth by one". That's pretty ugly, too. :)


You took the words right out of my mouth...   I guess I need to work  
on doing a better job of dumping my stream-of-thoughts that go into a  
patch into the emails to the list.


Most all of your comments could be dumped into the patch description  
as-is to pimp it out some.  I have no objection to that, even adding  
an "Additional-analysis-by:" (or similar) credit line too.  :)


--Kyle

Re: git 2.11.0 error when pushing to remote located on a windows share

[Torsten Bögershausen]

On Fri, Dec 02, 2016 at 05:37:50PM -0500, Jeff King wrote:
> On Fri, Dec 02, 2016 at 06:02:16PM +, thomas.attw...@stfc.ac.uk wrote:
> 
> > After updating git from 2.10.0 to 2.11.0 when trying to push any
> > changes to a repo located in a windows share, the following error
> > occurs:
> > 
> > $ git push origin test
> > Counting objects: 2, done.
> > Delta compression using up to 8 threads.
> > Compressing objects: 100% (2/2), done.
> > Writing objects: 100% (2/2), 284 bytes | 0 bytes/s, done.
> > Total 2 (delta 1), reused 1 (delta 0)
> > remote: error: object directory /path/to/dir/objects does not exist; check 
> > .git/objects/info/alternates.
> > remote: fatal: unresolved deltas left after unpacking
> > error: unpack failed: unpack-objects abnormal exit
> > To //path/to/dir
> >  ! [remote rejected] test -> test (unpacker error)
> > error: failed to push some refs to '//path/to/dir'
> 
> Hmm. This is probably related to the quarantine-push change in v2.11;
> the receiving end will write the objects into a temporary directory but
> point to the original via GIT_ALTERNATE_OBJECT_DIRECTORIES. That pointer
> isn't working for some reason, so the receiver can't resolve the deltas
> it needs.
> 
> As you noted, the extra "/" is missing in the error message, and that
> sounds like a plausible cause for what you're seeing. I'm not sure where
> the slash is getting dropped, though. The value in the environment comes
> from calling absolute_path(get_object_directory()), so I suspect the
> real problem is not in the quarantine code, but it's just triggering a
> latent bug elsewhere (either in absolute_path(), or in the code which
> generates the objdir path).
> 
> > No error occurs if pushing to the same repo (a direct copy into a local 
> > directory) using 2.11.0.
> > 
> > $ git push local_test test
> > Counting objects: 2, done.
> > Delta compression using up to 8 threads.
> > Compressing objects: 100% (2/2), done.
> > Writing objects: 100% (2/2), 284 bytes | 0 bytes/s, done.
> > Total 2 (delta 1), reused 1 (delta 0)
> > To C:/path/to/dir
> >  * [new branch]  test -> test
> 
> The fact that it works using the non-UNC path reinforces my feeling that
> something is normalizing the absolute path incorrectly.
> 
> > Using `git fsck --full` in both 2.11.0 and 2.10.0, it doesn't reveal any 
> > additional problems.
> 
> Yeah, I don't think there is anything wrong with your repo. It's just a
> path-building issue internal to the receiving process.
> 

There seems to be another issue, which may or may not being related:
https://github.com/git-for-windows/git/issues/979

This is pure speculation:
Could it be that a '/' is lost because of a change in the underlying
Msys2 between 2.10 and 2.11 ?

Dscho, (or anybody else) any ideas?