* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Ramsay Jones @ 2016-12-05 11:21 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <20161205053258.jtnqq64gp5n7vtni@sigill.intra.peff.net>
On 05/12/16 05:32, Jeff King wrote:
> On Sun, Dec 04, 2016 at 08:45:59PM +0000, Ramsay Jones wrote:
>> 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.
Heh, that was the first version of the patch. However, I got to thinking
about why --abbrev=7 was there in the first place; the only reason I
could think of was to defeat local configuration to get a measure of
reproducibility.
Unfortunately, you can't get the 'auto' behaviour from --abbrev
(on the pu branch):
$ ./git describe --abbrev=-1
v2.11.0-286-g109e8
$ ./git describe --abbrev=0
v2.11.0
$ ./git describe
v2.11.0-286-g109e8a99d
$
I did think about using '-c core.abbrev=auto', but that would
depend on Junio's patch (nothing wrong with that, of course):
$ git version
git version 2.11.0
$ git -c core.abbrev=auto describe
fatal: bad numeric config value 'auto' for 'core.abbrev': invalid unit
$ ./git -c core.abbrev=auto describe
v2.11.0-286-g109e8a99d
$
What do you think?
ATB,
Ramsay Jones
^ permalink raw reply
* RE: git 2.11.0 error when pushing to remote located on a windows share
From: thomas.attwood @ 2016-12-05 11:05 UTC (permalink / raw)
To: tboegi, peff; +Cc: git
In-Reply-To: <20161204080914.GB2415@tb-raspi>
On Sun, 4 Dec 2016 08:09:14 +0000, Torsten Bögershausen wrote:
> There seems to be another issue, which may or may not being related:
> https://github.com/git-for-windows/git/issues/979
I think this is the same issue. I've posted my trace command output there as
It might be more appropriate:
https://github.com/git-for-windows/git/issues/979#issuecomment-264816175
^ permalink raw reply
* Re: [PATCH v1] git-p4: add config to retry p4 commands; retry 3 times by default
From: Luke Diamand @ 2016-12-05 11:02 UTC (permalink / raw)
To: Lars Schneider; +Cc: Git Users, Ori Rawlings, Lars Schneider
In-Reply-To: <20161204140311.26269-1-larsxschneider@gmail.com>
On 4 December 2016 at 14:03, <larsxschneider@gmail.com> wrote:
> From: Lars Schneider <lars.schneider@autodesk.com>
>
> 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.
Looks good to me, ack.
>
> [1] https://www.perforce.com/perforce/doc.current/manuals/cmdref/global.options.html
>
> Signed-off-by: Lars Schneider <larsxschneider@gmail.com>
> ---
>
> 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 <client>', 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
>
^ permalink raw reply
* Re: Error after calling git difftool -d with
From: Johannes Schindelin @ 2016-12-05 10:56 UTC (permalink / raw)
To: P. Duijst; +Cc: David Aguilar, git
In-Reply-To: <c0c8c333-adfa-ad58-f1ec-7239a3a16528@gmail.com>
Hi Peter,
On Mon, 5 Dec 2016, P. Duijst wrote:
> On 12/5/2016 06:15, David Aguilar wrote:
> > On Fri, Dec 02, 2016 at 05:05:06PM +0100, Johannes Schindelin wrote:
> > >
> > > 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.
>
> @Johannes: "git -c difftool.useBuiltin=true difftool -d" works OK :-), beyond
> compare is launching with the diff's displayed
Perfect.
In that case, I think it is not worth fixing the scripted tool but focus
on getting rid of it in favor of the builtin version.
It's not like it is the only problem with having difftool implemented
as a script...
Ciao,
Johannes
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Johannes Schindelin @ 2016-12-05 10:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, git, David Aguilar, Dennis Kaarsemaker
In-Reply-To: <xmqqlgvz6x87.fsf@gitster.mtv.corp.google.com>
Hi Junio,
On Thu, 1 Dec 2016, Junio C Hamano wrote:
> As to "I have to spawn config", I think it is sensible to start the
> cmd_difftool() wrapper without adding RUN_SETUP to the command table,
> then call git_config_get_bool() to check the configuration only from
> system and per-user files, and then finally either call into
> builtin_difftool() where setup_git_directory() is called, or spawn the
> scripted difftool, as Peff already said.
I just spent a considerable amount of time to figure out that I overlooked
your comment about "only from system and per-user files".
> Your "users opt-in while installing" is not about setting per-repository
> option.
Wow. That would make things really inconsistent.
I know that *I* would want to be able to switch that opt-in feature off
for repositories where I absolutely rely on some rock-solid difftool. And
as a user I would not only be shocked when it would not work as expected,
but I would be *positively* shocked to learn that this is intended, by
design.
Seriously, do you really think it is a good idea to have
git_config_get_value() *ignore* any value in .git/config?
Really?
It caught *me* by surprise, and it definitely makes for a very, very bad
user experience.
And this is much more fundamental than just difftool.useBuiltin.
I see for example that our pager.<cmd> setting is ignoring per-repository
settings. That is, if the user sets pager.<cmd> in ~/.gitconfig and then
tries to override this in a specific repository (which any user would only
do for very good reasons, I am sure), Git would happily ignore that
careful setup and create an angry user.
The only reason this did not blow up in our face yet is that users
apparently do not make use of this feature yet. Which does not make this
behavior (that "git_config_get_value()" happily ignores .git/config) less
broken.
We need to fix this.
Ciao,
Dscho
^ permalink raw reply
* Re: [PATCH 1/4] shallow.c: make paint_alloc slightly more robust
From: Duy Nguyen @ 2016-12-05 10:02 UTC (permalink / raw)
To: Jeff King; +Cc: Rasmus Villemoes, Git Mailing List
In-Reply-To: <20161203051454.vp772xtto5ddxe7g@sigill.intra.peff.net>
On Sat, Dec 3, 2016 at 12:14 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Dec 02, 2016 at 09:31:01PM +0100, Rasmus Villemoes wrote:
>
>> I have no idea if this is a real issue, but it's not obvious to me that
>> paint_alloc cannot be called with info->nr_bits greater than about
>> 4M (\approx 8*COMMIT_SLAB_SIZE). In that case the new slab would be too
>> small. So just round up the allocation to the maximum of
>> COMMIT_SLAB_SIZE and size.
>
> I had trouble understanding what the problem is from this description,
> but I think i figured it out from the code.
>
> Let me try to restate it to make sure I understand.
>
> The paint_alloc() may be asked to allocate a certain number of bits,
> which it does across a series of independently allocated slabs. Each
> slab holds a fixed size, but we only allocate a single slab. If the
> number we need to allocate is larger than fits in a single slab, then at
> the end we'll have under-allocated.
Each bit here represents a ref. This code walks the commit graph and
"paints" all commits reachable by the n-th ref with the n-th bit,
stored in the commit slab. But because the majority of commits will
have the same bitmap (e.g. when you exclude tag ABC and nothing else,
then all commits from ABC will have the same bitmap "1"), it's a waste
to allocate the same bitmap per commit (and it's also inefficient to
let malloc allocate 1 bit). I tried to reduce the memory usage: if the
a commit and its parent has the same bitmap, and the slab pointer of
the child commit points to the memory of the parent's, no extra
allocation is done. This manual memory management is pretty much like
alloc.c
The COMMIT_SLAB_SIZE here is really an arbitrary big number so that we
don't have to allocate often. It's basically allocating a new memory
pool. When we use all of that pool, we allocate a new one.. Yeah I
probably should define a new one instead of reusing COMMIT_SLAB_SIZE.
Tthe chances of under-allocation is super low, but still possible: you
need to send more than 4M "exclude" (or "shallow") requests to
upload-pack, to create a bitmap of over 512KiB. That's a lot of
traffic in git protocol.
> Your solution is to make the slab we allocate bigger. But that seems
> odd to me. Usually when we are using COMMIT_SLAB_SIZE, we are allocating
> a series of slabs that make up a virtual array, and we know that each
> slab has the same size. So if you need to find the k-th item, and each
> slab has length n, then you'd look at slab (k / n), and then at item (k
> % n) within that slab.
>
> In other words, I think the solution isn't to make the one slab bigger,
> but to allocate slabs until we have enough of them to meet the request.
If I still understand my code (it's been a long time since I wrote
this thing), then I think we just need to catch the problem and die().
Normal users should never ask the server to allocate this much.
--
Duy
^ permalink raw reply
* Re: [PATCH v1] git-p4: add config to retry p4 commands; retry 3 times by default
From: Lars Schneider @ 2016-12-05 9:09 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20161204140311.26269-1-larsxschneider@gmail.com>
> On 04 Dec 2016, at 15:03, larsxschneider@gmail.com wrote:
>
> From: Lars Schneider <lars.schneider@autodesk.com>
Hi Junio,
if you decide to queue this patch and/or the "git-p4: fix empty file
processing for large file system backend GitLFS", please use my
signed-off address. I accidentally messed up the author field in
both.
Thanks,
Lars
>
> 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 <larsxschneider@gmail.com>
^ permalink raw reply
* Re: Error after calling git difftool -d with
From: P. Duijst @ 2016-12-05 7:58 UTC (permalink / raw)
To: David Aguilar; +Cc: Johannes Schindelin, git
In-Reply-To: <20161205051510.itftw4hyzkv6nnxn@gmail.com>
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
^ permalink raw reply
* Re: Should reset_revision_walk clear more flags?
From: Jeff King @ 2016-12-05 7:36 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Mike Hommey, git
In-Reply-To: <xmqqmvga6dmb.fsf@gitster.mtv.corp.google.com>
On Sun, Dec 04, 2016 at 11:26:04PM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> 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
^ permalink raw reply
* Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository
From: Jeff King @ 2016-12-05 7:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jack Bates, git, Jack Bates
In-Reply-To: <xmqqr35m6dwt.fsf@gitster.mtv.corp.google.com>
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(&rev, 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
^ permalink raw reply
* Re: Should reset_revision_walk clear more flags?
From: Junio C Hamano @ 2016-12-05 7:26 UTC (permalink / raw)
To: Jeff King; +Cc: Mike Hommey, git
In-Reply-To: <20161205054013.taosbwjamxiwzocn@sigill.intra.peff.net>
Jeff King <peff@peff.net> 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.
^ permalink raw reply
* Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository
From: Junio C Hamano @ 2016-12-05 7:19 UTC (permalink / raw)
To: Jack Bates; +Cc: git, Jeff King, Jack Bates
In-Reply-To: <20161204194747.7100-1-jack@nottheoilrig.com>
Jack Bates <bk874k@nottheoilrig.com> 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(&rev, 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?
^ permalink raw reply
* Re: Git v2.11.0 breaks max depth nested alternates
From: Jeff King @ 2016-12-05 7:18 UTC (permalink / raw)
To: Philip Oakley; +Cc: Kyle J. McKay, Junio C Hamano, Git mailing list
In-Reply-To: <49DE271A91214D6DADBD4DE667A1659B@PhilipOakley>
On Sun, Dec 04, 2016 at 11:22:52AM -0000, 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
^ permalink raw reply
* Re: Git v2.11.0 breaks max depth nested alternates
From: Jeff King @ 2016-12-05 7:14 UTC (permalink / raw)
To: Kyle J. McKay; +Cc: Junio C Hamano, Git mailing list
In-Reply-To: <E3C2AF2A-FE07-4C94-B549-3BDAF9B3DB5D@gmail.com>
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
^ permalink raw reply
* Re: [PATCH v2] diff: handle --no-abbrev outside of repository
From: Jeff King @ 2016-12-05 6:58 UTC (permalink / raw)
To: Jack Bates; +Cc: git, Junio C Hamano, Jack Bates
In-Reply-To: <20161205061500.dinyc3juedkpw6o3@sigill.intra.peff.net>
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.yspqt34p3dp5g5fk@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
^ permalink raw reply
* Re: [PATCH] diff: fix up SHA-1 abbreviations outside of repository
From: Jeff King @ 2016-12-05 6:55 UTC (permalink / raw)
To: Jack Bates; +Cc: git, Jack Bates
In-Reply-To: <20161204194747.7100-1-jack@nottheoilrig.com>
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(&rev, 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
> +0000. (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
^ permalink raw reply
* Re: [PATCH v2] diff: handle --no-abbrev outside of repository
From: Jeff King @ 2016-12-05 6:15 UTC (permalink / raw)
To: Jack Bates; +Cc: git, Junio C Hamano, Jack Bates
In-Reply-To: <20161205060116.szy5ojetg3znu4w7@sigill.intra.peff.net>
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
^ permalink raw reply
* Re: [PATCH v2] diff: handle --no-abbrev outside of repository
From: Jeff King @ 2016-12-05 6:01 UTC (permalink / raw)
To: Jack Bates; +Cc: git, Junio C Hamano, Jack Bates
In-Reply-To: <20161202184840.2158-1-jack@nottheoilrig.com>
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
^ permalink raw reply
* Re: Should reset_revision_walk clear more flags?
From: Jeff King @ 2016-12-05 5:40 UTC (permalink / raw)
To: Mike Hommey; +Cc: git
In-Reply-To: <20161204230958.h3ilhueqqptv253u@glandium.org>
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
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Jeff King @ 2016-12-05 5:32 UTC (permalink / raw)
To: Ramsay Jones; +Cc: Junio C Hamano, GIT Mailing-list
In-Reply-To: <22e9dfa0-47fb-d6fd-caf4-c2d87f63f707@ramsayjones.plus.com>
On Sun, Dec 04, 2016 at 08:45:59PM +0000, Ramsay Jones wrote:
>
> Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
> ---
>
> 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
^ permalink raw reply
* Re: Error after calling git difftool -d with
From: David Aguilar @ 2016-12-05 5:15 UTC (permalink / raw)
To: P. Duijst; +Cc: Johannes Schindelin, git
In-Reply-To: <alpine.DEB.2.20.1612021704170.117539@virtualbox>
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
^ permalink raw reply
* Re: Where is Doc to configure Git + Apache + kerberos for Project level access in repo?
From: brian m. carlson @ 2016-12-05 2:31 UTC (permalink / raw)
To: Jeff King; +Cc: ken edward, git
In-Reply-To: <20161202222153.3j5i5rsacybwexg6@sigill.intra.peff.net>
[-- Attachment #1: Type: text/plain, Size: 1046 bytes --]
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
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 868 bytes --]
^ permalink raw reply
* Re: Should reset_revision_walk clear more flags?
From: Mike Hommey @ 2016-12-04 23:24 UTC (permalink / raw)
To: git
In-Reply-To: <20161204230958.h3ilhueqqptv253u@glandium.org>
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
^ permalink raw reply
* Should reset_revision_walk clear more flags?
From: Mike Hommey @ 2016-12-04 23:09 UTC (permalink / raw)
To: git
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
^ permalink raw reply
* Re: difftool -d not populating left correctly when not in git root
From: David Aguilar @ 2016-12-04 22:31 UTC (permalink / raw)
To: Frank Becker; +Cc: git, John Keeping, Johannes Schindelin, Junio C Hamano
In-Reply-To: <68f49f5e-4e71-fd52-cd6f-64e92face962@mooflu.com>
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 <file>..." to update what will be committed)
> (use "git checkout -- <file>..." 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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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_njdpyjwm000000gn/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 were
constructing it relative to the subdirectory, and thus it ends up empty
because we are in a subdirectory and the paths are incorrect.
Teach difftool to chdir to the toplevel of the repository before
preparing its temporary indexes. This ensures that all of the
toplevel-relative paths are valid.
Add a test case to exercise this use case.
Reported-by: Frank Becker <fb@mooflu.com>
Signed-off-by: David Aguilar <davvid@gmail.com>
---
git-difftool.perl | 4 ++++
t/t7800-difftool.sh | 7 +++++--
2 files changed, 9 insertions(+), 2 deletions(-)
diff --git a/git-difftool.perl b/git-difftool.perl
index a5790d03a..959822d5f 100755
--- a/git-difftool.perl
+++ b/git-difftool.perl
@@ -182,6 +182,10 @@ 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.XXXXX', CLEANUP => 0, TMPDIR => 1);
my $ldir = "$tmpdir/left";
diff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh
index 70a2de461..caab4b5ca 100755
--- a/t/t7800-difftool.sh
+++ b/t/t7800-difftool.sh
@@ -413,8 +413,11 @@ run_dir_diff_test 'difftool --dir-diff from subdirectory' '
(
cd sub &&
git difftool --dir-diff $symlinks --extcmd ls branch >output &&
- grep sub output &&
- grep file output
+ # "sub" must only exist in "right"
+ # "file" and "file2" must be listed in both "left" and "right"
+ test "1" = $(grep sub output | wc -l) &&
+ test "2" = $(grep file"$" output | wc -l) &&
+ test "2" = $(grep file2 output | wc -l)
)
'
--
2.11.0.dirty
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox