* Re: Finding a tag that introduced a submodule change
From: Junio C Hamano @ 2017-03-03 18:04 UTC (permalink / raw)
To: Robert Dailey; +Cc: Git
In-Reply-To: <CAHd499CfJnPtLmi8qzr=_jrfCgMw85MOUv-wPKmAHFUyDFXhRA@mail.gmail.com>
Robert Dailey <rcdailey.lists@gmail.com> writes:
> Sometimes I run into a situation where I need to find out which
> release of the product a submodule change was introduced in. This is
> nontrivial, since there are no tags in the submodule itself.
Does your superproject rewind the commit in the submodule project as
it goes forward? That is, is this property guaranteed to hold by
your project's discipline:
Given any two commits C1 and C2 in the superproject, and the
commit in the submodule bound to C1's and C2's tree (call
them S1 and S2, respectively), if C1 is an ancestor of C2,
then S1 is the same as S2 or an ancestor of S2.
If so, I think you can do a bisection of the history in the
superproject. Pick an old commit in the superproject that binds an
old commit from the submodule that does not have the change and call
it "good". Similarly pick a new one in the superproject that binds
a newer commit from the submodule that does have the change, and
call it "bad". Then do
$ git bisect start $bad $good -- $path_to_submodule
which would suggest you to test commits that change what commit is
bound at the submodule's path.
When testing each of these commits, you would see if the commit
bound at the submodule's path has the change or not.
$ current=$(git rev-parse HEAD:$path_to_submodule)
would give you the object name of that commit, and then
$ git -C $path_to_submodule merge-base --is-ancestor $change $current
would tell you if the $change you are interested in is already
contained in that $current commit. Then you say "git bisect good"
if $current is too old to contain the $change, and "git bisect bad"
if $current is sufficiently new and contains the $change, to
continue.
If your superproject rewinds the commit in the submodule as it goes
forward, e.g. an older commit in the superproject used submodule
commit from day X, but somebody who made yesterday's commit in the
superproject realized that that submodule commit was broken and used
an older commit in the submodule from day (X-1), then you cannot
bisect. In such a case, I think you would essentially need to check
all superproject commits that changed the commit bound at the
submodule's path.
$ git rev-list $bad..$good -- $path_to_submodule
would give a list of such commits, and you would do the "merge-base"
check for all them to see which ones have and do not have the
$change (replace "HEAD" with the commit you are testing in the
computation that gives you $current).
^ permalink raw reply
* Re: [PATCH v7 0/3] Conditional config include
From: Junio C Hamano @ 2017-03-03 18:24 UTC (permalink / raw)
To: Jeff King
Cc: Nguyễn Thái Ngọc Duy, git, sschuberth,
Matthieu Moy, Philip Oakley, Ramsay Jones
In-Reply-To: <20170303063329.ji6do6eqjbpuwmxz@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> For those following on the mailing list, there is some discussion at:
>
> https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
>
> I think that is mostly focused around another failing in the
> error-handling of the config code, and that does not need to be
> addressed by this series (though of course I'd welcome any fixes).
Thanks. Without a message like this, the list may have never known
about the discussion taken elsewhere. I'd appreciate such a report
to appear on list the next time much earlier ;-)
When built with FREAD_READS_DIRECTORIES=Yes on Linux, the error in
the test can easily reproduce.
In early days of UNIX it was sometimes handy to be able to read the
bytes off of directory to "investigate", but we are not a filesystem
application, and I do not offhand see any reason why we should be
relying on being able to successfully fopen() a directory for
reading. A FILE * successfully opened that just returns EOF when
read is totally useless for any purpose anyway.
When the path to be opened from the end user (either from the
command line or in a configuration file) is a directory, it is
better to diagnose it as a user error, and if the path was computed
by our code, it may be a bug.
I am wondering if we should enable this on Linux, at least in
DEVELOPER builds but possibly even on the release builds, to catch
these problems more easily.
^ permalink raw reply
* Re: log -S/-G (aka pickaxe) searches binary files by default
From: Thomas Braun @ 2017-03-03 18:05 UTC (permalink / raw)
To: Junio C Hamano, Jeff King; +Cc: GIT Mailing-list
In-Reply-To: <xmqqzih29wdu.fsf@gitster.mtv.corp.google.com>
Am 03.03.2017 um 17:07 schrieb Junio C Hamano:
> Jeff King <peff@peff.net> writes:
>
>> On Thu, Mar 02, 2017 at 05:36:17PM -0800, Junio C Hamano wrote:
>> ...
>>>> Is that on purpose?
>>>
>>> No, it's a mere oversight (as I do not think I never even thought
>>> about special casing binary
>>> files from day one, it is unlikely that you would find _any_ old
>>> version of Git that behaves
>>> differently).
>>
>> The email focuses on "-G", and I think it is wrong to look in binary
>> files there, as "grep in diff" does not make sense for a binary file
>> that we would refuse to diff.
>
> Yeah, I agree.
>
>> But the subject also mentions "-S". I always assumed it was intentional
>> to look in binary files there, as it is searching for a pure byte
>> sequence. I would not mind an option to disable that, but I think the
>> default should remain on.
>
> As the feature was built to be one of the core ingredients necessary
> towards the 'ideal SCM' envisioned in
>
> <http://public-inbox.org/git/Pine.LNX.4.58.0504150753440.7211@ppc970.osdl.org>
>
> "-S" is about finding "a block of text". It was merely an oversight
> that we didn't add explicit code to ignore binary when we introduced
> the concept of "is this text? is it worth finding things in and
> diffing binary files?".
>
> I do agree that it may be too late and/or disruptive to change its
> behaviour now, as people may grew expectations different from the
> original motivation and design, though.
Thanks both for the encouraging answers.
I'll try to come up with patches in the next couple of weeks for the
following changes:
"log -G": disable looking in binaries
"log -S": add option to switch looking into binaries, defaults to true
Thomas
^ permalink raw reply
* [RFC PATCH 1/2] completion: add bash completion for 'git rev-list'
From: Denton Liu @ 2017-03-03 12:35 UTC (permalink / raw)
To: git
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
This patch isn't strictly necessary since 'git rev-list' isn't a porcelain
command. However, it might be nice to include in case users interactively call
'git rev-list' anyway.
---
contrib/completion/git-completion.bash | 30 ++++++++++++++++++++++++++++++
1 file changed, 30 insertions(+)
diff --git a/contrib/completion/git-completion.bash b/contrib/completion/git-completion.bash
index 41ee52991..412485369 100644
--- a/contrib/completion/git-completion.bash
+++ b/contrib/completion/git-completion.bash
@@ -2499,6 +2499,36 @@ _git_revert ()
__gitcomp_nl "$(__git_refs)"
}
+__git_rev_list_options="
+ --max-count= --skip= --max-age= --min-age= --sparse --merges --no-merges
+ --min-parents= --no-min-parents --max-parents= --no-max-parents --first-parent
+ --remove-empty --full-history --not --all --branches= --tags= --remotes=
+ --glob= --ignore-missing --stdin --quiet --topo-order --parents --timestamp
+ --left-right --left-only --right-only --cherry-mark --cherry-pick --encoding=
+ --author= --committer= --grep= --regexp-ignore-case --extended-regexp
+ --fixed-strings --date= --objects --objects-edge --objects-edge-aggressive
+ --unpacked --pretty --header --bisect --bisect-vars --bisect-all --merge
+ --reverse --walk-reflogs --no-walk --do-walk --count --use-bitmap-index
+"
+
+__git_complete_rev_list_command ()
+{
+ case "$cur" in
+ --*)
+ __gitcomp "$__git_rev_list_options"
+ return 0
+ ;;
+ esac
+ return 1
+}
+
+_git_rev_list ()
+{
+ __git_has_doubledash && return
+
+ __git_complete_rev_list_command || __gitcomp_nl "$(__git_refs)"
+}
+
_git_rm ()
{
case "$cur" in
--
2.12.0.1.g5415fdfc5.dirty
^ permalink raw reply related
* Re: [PATCH] t/perf: export variable used in other blocks
From: Junio C Hamano @ 2017-03-03 18:51 UTC (permalink / raw)
To: Jeff King; +Cc: Jonathan Tan, git
In-Reply-To: <20170303064512.khs2seru5onl54mh@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> Thanks, this is obviously the right thing to do, and the mistake is mine
> from ea97002fc (t/perf: time rev-list with UNINTERESTING commits,
> 2014-01-20). This is not the first time I've been confused by missing
> variables in t/perf scripts, since it behaves differently than the
> normal test suite. I wonder if we should turn on "set -a" during t/perf
> setup snippets. That's a bit of a blunt tool, but I suspect it would
> just be easier to work with.
I wonder if we can make t/perf to behave more similar to the normal
test suite to eliminate the need for this exporting, by the way.
t/perf/README does not say anything more than "for lack of better
options" throughout its history, which does not help very much.
^ permalink raw reply
* Re: [PATCH v1] Travis: also test on 32-bit Linux
From: Junio C Hamano @ 2017-03-03 18:43 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Lars Schneider, git
In-Reply-To: <alpine.DEB.2.20.1703030315580.3767@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> On Thu, 2 Mar 2017, Junio C Hamano wrote:
>
>> Another question is which v3 people mean in the discussion, when you
>> and Dscho work on improvements at the same time and each post the
>> "next" version marked as "v3", and they comment on one of them?
>
> But then, Lars & I communicate in a more direct way than the mailing list
> when discussing teeny tiny details such as: "could you have a look at my
> mail" or "how would you change .travis.yml to do XYZ".
>
> With that level of communication, there is almost no danger of two v3s.
Sure, that is true between two (or more) people who communicate
privately. The issue you raised on Lars's "v1" and your original
unversioned one can be seen either (1) as similar to having two v1's
or (2) having (an implicit) v0 and v1 that won't cause confusion.
As I said already, because the v0 and v1 (or v1 and v1) came from
two different people, it's not such a big deal if Lars named his
first one v1 or v2, just like it won't cause problem if his reroll
were done as v2 or v3.
I see he did v2 which you Acked in a different thread. Will replace
what's been on 'pu' and running with Travis the past few days with
it. Let's wait for one or more Travis cycles and then merge it to
'next'.
Thanks.
^ permalink raw reply
* Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts
From: Junio C Hamano @ 2017-03-03 19:06 UTC (permalink / raw)
To: Jeff King; +Cc: Sebastian Schuberth, git
In-Reply-To: <20170303112104.2n3hu23sajpn2x2z@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Mar 03, 2017 at 10:57:46AM +0000, Sebastian Schuberth wrote:
>
>> It does not make sense for these placeholder scripts to depend on Python
>> just because the real scripts do. At the example of Git for Windows, we
>> would not even be able to see those warnings as it does not ship with
>> Python. So just use plain shell scripts instead.
>
> Yeah, this seems like an obvious improvement. I think we got here
> because the originals issued a warning but kept working, and then it was
> slowly whittled down to remove the "working" part.
> ...
> OTOH, it is not really hurting much, so I do not mind keeping them
> around for another 3 years (or more) just to catch any stragglers.
Yup, let's queue it and remove it in a few years.
Thanks.
^ permalink raw reply
* Re: [PATCH v7 3/3] config: add conditional include
From: Junio C Hamano @ 2017-03-03 19:05 UTC (permalink / raw)
To: Jeff King
Cc: Nguyễn Thái Ngọc Duy, git, sschuberth,
Matthieu Moy, Philip Oakley, Ramsay Jones
In-Reply-To: <20170303063052.xkjoipuaohcwdl4n@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> That was how I had originally envisioned the namespace working when I
> introduced include.path long ago. And I think Duy's v1 used that, but
> the feedback was that it was not sufficiently obvious that the
> subsection was a conditional.
I am not sure about "obviousness", but I agree that we do not know
that "conditional include" would be the only thing we want for the
second level for "include.path" directive. "include-if.<cond>.path"
is better for that reason.
I presume that you could still do
[include "if:gitdir=$path"]
path = ...
i.e. design the second level to begin with a token that tells
readers what it means (and assign "if:" token for "conditional
include"), but I do not think it is worth it.
I also imagine that
[include]
condition = ...
path = ...
is easier to read and write by end-users, but it probably is not
feasible because it requires too invasive change to the current code
to teach it to grok such construct.
Between "include-if" and "includeIf", if people find the latter not
too ugly, I'd prefer to keep it the way Duy posted. Because of the
way "include.path" and "include-if.<cond>.path" work, we can declare
that they are not like ordinary configuration variable definition
at all but are higher-level directives and that may be a sufficient
justification to allow "-" in its name, though, if people find
"includeIf" too ugly a name to live.
^ permalink raw reply
* Re: [PATCH v7 0/3] Conditional config include
From: Junio C Hamano @ 2017-03-03 19:13 UTC (permalink / raw)
To: Duy Nguyen
Cc: Jeff King, git, sschuberth, Matthieu Moy, Philip Oakley,
Ramsay Jones
In-Reply-To: <20170303095252.GA12072@ash>
Duy Nguyen <pclouds@gmail.com> writes:
> The patch to fix it is this
>
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index f0cd2056ba..e833939320 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect includes' '
>
> test_expect_success 'missing include files are ignored' '
> cat >.gitconfig <<-\EOF &&
> - [include]path = foo
> + [include]path = non-existent
> [test]value = yes
> EOF
> echo yes >expect &&
>
> Junio could you squash this in?
Sure.
^ permalink raw reply
* Re: [PATCH] line-log.c: prevent crash during union of too many ranges
From: Junio C Hamano @ 2017-03-03 19:19 UTC (permalink / raw)
To: Allan Xavier; +Cc: git
In-Reply-To: <20170302172902.16850-1-allan.x.xavier@oracle.com>
Allan Xavier <allan.x.xavier@oracle.com> writes:
> The existing implementation of range_set_union does not correctly
> reallocate memory, leading to a heap overflow when it attempts to union
> more than 24 separate line ranges.
> ...
I'll add "Reviewed-by: Jeff King <peff@peff.net>" and queue, as I
know we already reviewed the change.
Thanks.
^ permalink raw reply
* [PATCH v2 5/5] Remove outdated info in difftool manpage
From: Denton Liu @ 2017-03-03 13:40 UTC (permalink / raw)
To: git; +Cc: davvid, Johannes.Schindelin
When difftool was rewritten in C, it removed the capability to read
fallback configs from mergetool. This changes the documentation to
reflect this.
Signed-off-by: Denton Liu <liu.denton@gmail.com>
---
Documentation/git-difftool.txt | 3 ---
1 file changed, 3 deletions(-)
diff --git a/Documentation/git-difftool.txt b/Documentation/git-difftool.txt
index 96c26e6aa..a00cb033e 100644
--- a/Documentation/git-difftool.txt
+++ b/Documentation/git-difftool.txt
@@ -105,9 +105,6 @@ See linkgit:git-diff[1] for the full list of supported options.
CONFIG VARIABLES
----------------
-'git difftool' falls back to 'git mergetool' config variables when the
-difftool equivalents have not been defined.
-
diff.tool::
The default diff tool to use.
--
2.12.0.5.gfbc750a84
^ permalink raw reply related
* Re: [PATCH 3/3] Remove outdated info in difftool manpage
From: Junio C Hamano @ 2017-03-03 19:29 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Denton Liu, git, davvid
In-Reply-To: <alpine.DEB.2.20.1703031645470.3767@virtualbox>
Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> Hi Denton (or should I address you as Liu?),
>
> On Fri, 3 Mar 2017, Denton Liu wrote:
>
>> When difftool was rewritten in C, it removed the capability to read
>> fallback configs from mergetool. This changes the documentation to
>> reflect this.
>
> Thanks for pointing that out. But that is probably an oversight on my
> part, not an intentional change...
So, ... in the meantime we'll hold off of this removal from the
documentation and wait for the "capability" to get restored?
^ permalink raw reply
* Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts
From: Junio C Hamano @ 2017-03-03 19:10 UTC (permalink / raw)
To: Marc Branchaud; +Cc: Sebastian Schuberth, git
In-Reply-To: <a9273d37-388a-8c27-1cf1-51e9cb28ef55@xiplink.com>
Marc Branchaud <marcnarc@xiplink.com> writes:
> On 2017-03-03 05:57 AM, Sebastian Schuberth wrote:
>> It does not make sense for these placeholder scripts to depend on Python
>> just because the real scripts do. At the example of Git for Windows, we
>> would not even be able to see those warnings as it does not ship with
>> Python. So just use plain shell scripts instead.
>
> Just a niggle: This change moves the warning message from stderr to stdout.
Right. Here is what I'll queue.
-- >8 --
From: Sebastian Schuberth <sschuberth@gmail.com>
Date: Fri, 3 Mar 2017 10:57:46 +0000
Subject: [PATCH] contrib: git-remote-{bzr,hg} placeholders don't need Python
It does not make sense for these placeholder scripts to depend on Python
just because the real scripts do. At the example of Git for Windows, we
would not even be able to see those warnings as it does not ship with
Python. So just use plain shell scripts instead.
Signed-off-by: Sebastian Schuberth <sschuberth@gmail.com>
Reviewed-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
contrib/remote-helpers/git-remote-bzr | 16 +++++++---------
contrib/remote-helpers/git-remote-hg | 16 +++++++---------
2 files changed, 14 insertions(+), 18 deletions(-)
diff --git a/contrib/remote-helpers/git-remote-bzr b/contrib/remote-helpers/git-remote-bzr
index 712a1377e2..ccc4aea362 100755
--- a/contrib/remote-helpers/git-remote-bzr
+++ b/contrib/remote-helpers/git-remote-bzr
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh
-import sys
-
-sys.stderr.write('WARNING: git-remote-bzr is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit https://github.com/felipec/git-remote-bzr\n')
-
-sys.stderr.write('''WARNING:
+cat >&2 <<'EOT'
+WARNING: git-remote-bzr is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-bzr
+WARNING:
WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING: $ wget -O $HOME/bin/git-remote-bzr \\
+WARNING: $ wget -O $HOME/bin/git-remote-bzr \
WARNING: https://raw.github.com/felipec/git-remote-bzr/master/git-remote-bzr
WARNING: $ chmod +x $HOME/bin/git-remote-bzr
-''')
+EOT
diff --git a/contrib/remote-helpers/git-remote-hg b/contrib/remote-helpers/git-remote-hg
index 4255ad6312..dfda44f311 100755
--- a/contrib/remote-helpers/git-remote-hg
+++ b/contrib/remote-helpers/git-remote-hg
@@ -1,13 +1,11 @@
-#!/usr/bin/env python
+#!/bin/sh
-import sys
-
-sys.stderr.write('WARNING: git-remote-hg is now maintained independently.\n')
-sys.stderr.write('WARNING: For more information visit https://github.com/felipec/git-remote-hg\n')
-
-sys.stderr.write('''WARNING:
+cat >&2 <<'EOT'
+WARNING: git-remote-hg is now maintained independently.
+WARNING: For more information visit https://github.com/felipec/git-remote-hg
+WARNING:
WARNING: You can pick a directory on your $PATH and download it, e.g.:
-WARNING: $ wget -O $HOME/bin/git-remote-hg \\
+WARNING: $ wget -O $HOME/bin/git-remote-hg \
WARNING: https://raw.github.com/felipec/git-remote-hg/master/git-remote-hg
WARNING: $ chmod +x $HOME/bin/git-remote-hg
-''')
+EOT
--
2.12.0-368-g85767a6c71
^ permalink raw reply related
* Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool
From: Junio C Hamano @ 2017-03-03 19:39 UTC (permalink / raw)
To: Denton Liu; +Cc: git, davvid, Johannes.Schindelin
In-Reply-To: <20170303134022.GA6258@arch-attack.localdomain>
Denton Liu <liu.denton@gmail.com> writes:
> In these tests, there are many situations where
> 'echo "" | git mergetool' is used. This replaces all of those
> occurrences with 'git mergetool -y' for simplicity and readability.
"-y where _possible_" is not necessarily a good thing to do in
tests. We do want to make sure that both "yes" from the input and
"-y" from the command line work. Changes to "-y" done only for the
tests that are not about accepting the interactive input from the
end-user is a very good idea, but "replaces all of those" makes me
worried.
> - ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool file1 file1 ) &&
> - ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> + git mergetool -y both >/dev/null 2>&1 &&
> + git mergetool -y file1 file1 &&
> + git mergetool -y file2 "spaced name" >/dev/null 2>&1 &&
> + git mergetool -y subdir/file3 >/dev/null 2>&1 &&
So these replace "the user interactively keeps typing <ENTER>" with
"-y", which sounds good. These are obviously more about what the
code actually does.
> - ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> + git mergetool file1 >/dev/null 2>&1 &&
> + git mergetool file2 >/dev/null 2>&1 &&
> + git mergetool "spaced name" >/dev/null 2>&1 &&
> + git mergetool both >/dev/null 2>&1 &&
> + git mergetool subdir/file3 >/dev/null 2>&1 &&
The reason for the lack of "-y" in the rewrite, in contrast to what
was done in the previous hunk we saw, is not quite obvious.
> @@ -177,7 +177,7 @@ test_expect_success 'mergetool in subdir' '
> (
> cd subdir &&
> test_expect_code 1 git merge master >/dev/null 2>&1 &&
> - ( yes "" | git mergetool file3 >/dev/null 2>&1 ) &&
> + git mergetool file3 >/dev/null 2>&1 &&
Likewise, and likewise for the remainder of the patch where the
updated command line does not say "-y".
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-03-03 11:04 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFwXaSAMF41Dz3u3nS+2S24umdUFv0+k+s18UyPoj+v31g@mail.gmail.com>
On Thu, Mar 02, 2017 at 11:55:45AM -0800, Linus Torvalds wrote:
> Anyway, I do have a suggestion for what the "object version" would be,
> but I'm not even going to mention it, because I want people to first
> think about the _concept_ and not the implementation.
>
> So: What do you think about the concept?
I think it very much depends on what's in the "object version". :)
IMHO, we are best to consider sha1 "broken" and not count on any of its
bytes for cryptographic integrity. I know that's not really the case,
but it just makes reasoning about the whole thing simpler. So at that
point, it's pretty obvious that the "object version" is really just "an
integrity hash".
And that takes us full circle to earlier proposals over the years to do
something like this in the commit header:
parent ...some sha1...
parent-sha256 ...some sha256...
and ditto in tag headers, and trees obviously need to be hackily
extended as you described to carry the extra hash. And then internally
we continue to happily use sha1s, except you can check the
sha256-validity of any reference if you feel like it.
This is functionally equivalent to "just start using sha-256, but keep a
mapping of old sha1s to sha-256s to handle old references". The
advantage is that it makes the code part of the transition simpler. The
disadvantage is that you're effectively carrying a piece of that
sha1->sha256 mapping around in _every_ object.
And that means the same bits of mapping data are repeated over and over.
Git's pretty good at de-duplicating on the surface. So yeah, every tree
entry is now 256 bits larger, but deltas mean that we usually only end
up storing each entry a handful of times. But we still pay the price to
walk over the bytes every time we apply a delta, zlib inflate, parse the
tree, etc. The runtime cost of the transition is carried forward
forever, even for repositories that are willing to rewrite history, or
are created after the flag day.
So I dunno. Maybe I am missing something really clever about your
proposal. Reading the rest of the thread, it sounds like you had a
thought that we could get by with a very tiny object version, but the
hash-adding thing nixed that. If I'm still missing the point, please try
to sketch it out a bit more concretely, and I'll come back with my
thinking cap on.
-Peff
^ permalink raw reply
* Re: [PATCH] Do not require Python for the git-remote-{bzr,hg} placeholder scripts
From: Sebastian Schuberth @ 2017-03-03 20:13 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Marc Branchaud, Git Mailing List
In-Reply-To: <xmqqtw7a89dg.fsf@gitster.mtv.corp.google.com>
On Fri, Mar 3, 2017 at 8:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> Just a niggle: This change moves the warning message from stderr to stdout.
>
> Right. Here is what I'll queue.
Indeed, thanks for the note, and also Junio for fixing while queuing.
--
Sebastian Schuberth
^ permalink raw reply
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Ramsay Jones @ 2017-03-03 20:21 UTC (permalink / raw)
To: Nguyễn Thái Ngọc Duy, git
Cc: Junio C Hamano, Jeff King, Johannes Schindelin
In-Reply-To: <20170303094252.11706-2-pclouds@gmail.com>
On 03/03/17 09:42, Nguyễn Thái Ngọc Duy wrote:
> If a directory is given as a config file by accident, we keep open it
> as a file. The behavior of fopen() in this case seems to be
> undefined.
>
> On Linux, we open a directory as a file ok, then get error (which we
> consider eof) on the first read. So the config parser sees this "file"
> as empty (i.e. valid config). All is well and we don't complain
> anything (but we should).
>
> The situation is slighly different on Windows. I think fopen() returns
> NULL. And we get a very unhelpful message:
>
> $ cat >abc <<EOF
> [include]
> path = /tmp/foo
> EOF
> $ mkdir /tmp/foo
> $ git config --includes --file=abc core.bare
> fatal: bad config line 3 in file abc
>
> Opening a directory is wrong in the first place. Avoid it. If caught,
> print something better. With this patch, we have
>
> $ git config --includes --file=abc core.bare
> error: '/tmp/foo' is not a file
> fatal: bad config line 3 in file abc
>
> It's not perfect (line should be 2 instead of 3). But it's definitely
> improving.
>
> The new test is only relevant on linux where we blindly open the
> directory and consider it an empty file. On Windows, the test should
> pass even without this patch.
> ---
> abspath.c | 7 +++++++
> cache.h | 1 +
> config.c | 9 +++++++++
> t/t1300-repo-config.sh | 5 +++++
> 4 files changed, 22 insertions(+)
>
> diff --git a/abspath.c b/abspath.c
> index 2f0c26e0e2..373cc3abb2 100644
> --- a/abspath.c
> +++ b/abspath.c
> @@ -11,6 +11,13 @@ int is_directory(const char *path)
> return (!stat(path, &st) && S_ISDIR(st.st_mode));
> }
>
> +int is_not_file(const char *path)
> +{
> + struct stat st;
> +
> + return !stat(path, &st) && !S_ISREG(st.st_mode);
> +}
> +
> /* removes the last path component from 'path' except if 'path' is root */
> static void strip_last_component(struct strbuf *path)
> {
> diff --git a/cache.h b/cache.h
> index 80b6372cf7..bdd1402ab9 100644
> --- a/cache.h
> +++ b/cache.h
> @@ -1149,6 +1149,7 @@ static inline int is_absolute_path(const char *path)
> return is_dir_sep(path[0]) || has_dos_drive_prefix(path);
> }
> int is_directory(const char *);
> +int is_not_file(const char *);
> char *strbuf_realpath(struct strbuf *resolved, const char *path,
> int die_on_error);
> const char *real_path(const char *path);
> diff --git a/config.c b/config.c
> index c6b874a7bf..c21c0ce433 100644
> --- a/config.c
> +++ b/config.c
> @@ -13,6 +13,7 @@
> #include "hashmap.h"
> #include "string-list.h"
> #include "utf8.h"
> +#include "dir.h"
Something is a bit odd here - nothing in this commit (that I can
see, anyway) would require the addition of this include. Also, this
include is already there in the 'pu' branch, brought in by your
conditional include changes. So, ...
ATB,
Ramsay Jones
^ permalink raw reply
* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
From: Devin J. Pohly @ 2017-03-03 20:30 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, Charles Bailey, git
In-Reply-To: <20170303075535.rxij4knd7d4ayfev@sigill.intra.peff.net>
On Fri, Mar 03, 2017 at 02:55:35AM -0500, Jeff King wrote:
> The only objectionable thing I noticed in the test additions is that
> the early ones should be marked test_expect_failure until the fix from
> 3/4 flips them to "success". Otherwise it breaks bisectability.
>
> -Peff
Good point. Will fix in a v2 set then.
--
<><
^ permalink raw reply
* Re: [PATCH 4/4] p7000: add test for filter-branch with --prune-empty
From: Devin J. Pohly @ 2017-03-03 20:34 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20170303075605.3pqqzmjcqdtv2bjd@sigill.intra.peff.net>
On Fri, Mar 03, 2017 at 02:56:05AM -0500, Jeff King wrote:
> On Thu, Feb 23, 2017 at 02:27:36AM -0600, Devin J. Pohly wrote:
>
> > +test_perf 'noop prune-empty' '
> > + git checkout --detach tip &&
> > + git filter-branch -f --prune-empty base..HEAD
> > +'
>
> I don't mind adding this, but of curiosity, does it show anything
> interesting?
>
> -Peff
Nothing surprising; the overhead for the change was minimal. I wasn't
sure what the practice is for adding unit/perf tests, so I erred on the
side of covering everything.
I will leave this as the last commit in the series so that it can be
dropped or merged as you see fit.
--
<><
^ permalink raw reply
* Re: [PATCH 3/4] filter-branch: fix --prune-empty on parentless commits
From: Junio C Hamano @ 2017-03-03 20:43 UTC (permalink / raw)
To: Devin J. Pohly, Jeff King; +Cc: Johannes Schindelin, Charles Bailey, git
In-Reply-To: <20170303075535.rxij4knd7d4ayfev@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> The change to filter-branch itself looks obviously correct. The only
> objectionable thing I noticed in the test additions is that the early
> ones should be marked test_expect_failure until the fix from 3/4 flips
> them to "success". Otherwise it breaks bisectability.
I'll squash in the necessary changes to flip between expect_success
and expect_failure in the appropriate places and re-queue on 'pu'.
Thanks.
commit 07cac4a5fdfeeb3c1c8385e222100d575a4460b0
Author: Junio C Hamano <gitster@pobox.com>
Date: Fri Mar 3 11:41:36 2017 -0800
fixup! t7003: ensure --prune-empty can prune root commit
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 2dfe462501..45372a1cba 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -362,7 +362,7 @@ test_expect_success 'prune empty works even without index/tree filters' '
test_cmp expect actual
'
-test_expect_success '--prune-empty is able to prune root commit' '
+test_expect_failure '--prune-empty is able to prune root commit' '
git rev-list branch-no-a >expect &&
git branch testing H &&
git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t" testing &&
commit 562ed048c681686426ca95e0e550378b48aa4852
Author: Junio C Hamano <gitster@pobox.com>
Date: Fri Mar 3 12:11:25 2017 -0800
fixup! t7003: ensure --prune-empty removes entire branch when applicable
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index a774a8e4b3..40526d1716 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -371,7 +371,7 @@ test_expect_failure '--prune-empty is able to prune root commit' '
test_cmp expect actual
'
-test_expect_success '--prune-empty is able to prune entire branch' '
+test_expect_failure '--prune-empty is able to prune entire branch' '
git branch prune-entire B &&
git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
test_path_is_missing .git/refs/heads/prune-entire &&
commit 520534c4035a13c54229dab0320e745d18635ef3
Author: Junio C Hamano <gitster@pobox.com>
Date: Fri Mar 3 12:39:58 2017 -0800
fixup! filter-branch: fix --prune-empty on parentless commits
diff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh
index 40526d1716..7cb60799be 100755
--- a/t/t7003-filter-branch.sh
+++ b/t/t7003-filter-branch.sh
@@ -362,7 +362,7 @@ test_expect_success 'prune empty works even without index/tree filters' '
test_cmp expect actual
'
-test_expect_failure '--prune-empty is able to prune root commit' '
+test_expect_success '--prune-empty is able to prune root commit' '
git rev-list branch-no-a >expect &&
git branch testing H &&
git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t" testing &&
@@ -371,7 +371,7 @@ test_expect_failure '--prune-empty is able to prune root commit' '
test_cmp expect actual
'
-test_expect_failure '--prune-empty is able to prune entire branch' '
+test_expect_success '--prune-empty is able to prune entire branch' '
git branch prune-entire B &&
git filter-branch -f --prune-empty --index-filter "git update-index --remove A.t B.t" prune-entire &&
test_path_is_missing .git/refs/heads/prune-entire &&
^ permalink raw reply related
* Re: [PATCH 1/2] config: check if config path is a file before parsing it
From: Junio C Hamano @ 2017-03-03 21:05 UTC (permalink / raw)
To: Jeff King; +Cc: Nguyễn Thái Ngọc Duy, git, Johannes Schindelin
In-Reply-To: <20170303095351.rgifjpfuvafx33jy@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> a. actually check ferror() after getting EOF and report the read
> error. That catches EISDIR, along with any other unexpected
> errors.
That is the most sensible, I would think (assuming that we really
get EISDIR instead of silent EOF).
> b. use an fopen wrapper that checks fstat(fileno(fh)) after the
> open, and turns fopen(some_dir) into an error.
That's already an option with FREAD_READS_DIRECTORIES, I think.
> 2. It doesn't address the root problem for git_config_from_file(),
> which is that it is quiet when fopen fails, even if the reason is
> something interesting besides ENOENT. The caller can't check errno
> because it doesn't know if fopen() failed, or if the config
> callback returned an error.
Perhaps like this one as a starting point, with FREAD_READS_DIRECTORIES?
config.c | 3 +++
1 file changed, 3 insertions(+)
diff --git a/config.c b/config.c
index 0dac0f4cb2..af8c01c8a3 100644
--- a/config.c
+++ b/config.c
@@ -1305,6 +1305,9 @@ int git_config_from_file(config_fn_t fn, const char *filename, void *data)
FILE *f;
f = fopen(filename, "r");
+ if (!f && errno != ENOENT)
+ die_errno("fopen('%s') failed", filename);
+
if (f) {
flockfile(f);
ret = do_config_from_file(fn, CONFIG_ORIGIN_FILE, filename, filename, f, data);
^ permalink raw reply related
* Re: [PATCH v7 0/3] Conditional config include
From: Junio C Hamano @ 2017-03-03 21:08 UTC (permalink / raw)
To: Duy Nguyen
Cc: Jeff King, git, sschuberth, Matthieu Moy, Philip Oakley,
Ramsay Jones
In-Reply-To: <20170303095252.GA12072@ash>
Duy Nguyen <pclouds@gmail.com> writes:
> On Fri, Mar 03, 2017 at 01:33:29AM -0500, Jeff King wrote:
>> For those following on the mailing list, there is some discussion at:
>>
>> https://github.com/git/git/commit/484f78e46d00c6d35f20058671a8c76bb924fb33
>>
>> I think that is mostly focused around another failing in the
>> error-handling of the config code, and that does not need to be
>> addressed by this series (though of course I'd welcome any fixes).
>>
>> But there's a test failure that probably does need to be dealt with
>> before this graduates to 'next'.
>
> The patch to fix it is this
>
> diff --git a/t/t1305-config-include.sh b/t/t1305-config-include.sh
> index f0cd2056ba..e833939320 100755
> --- a/t/t1305-config-include.sh
> +++ b/t/t1305-config-include.sh
> @@ -102,7 +102,7 @@ test_expect_success 'config modification does not affect includes' '
>
> test_expect_success 'missing include files are ignored' '
> cat >.gitconfig <<-\EOF &&
> - [include]path = foo
> + [include]path = non-existent
> [test]value = yes
> EOF
> echo yes >expect &&
>
> Junio could you squash this in?
>
I said yes, but I won't, as the series already has SQUASH??? for
other things, so I'll just queue this at the tip. I _might_ get
inclined to do the squashing and cleaning up myself later, but
please don't hold your breath.
^ permalink raw reply
* Re: git status --> Out of memory, realloc failed
From: Carsten Fuchs @ 2017-03-03 21:10 UTC (permalink / raw)
To: Duy Nguyen; +Cc: René Scharfe, Git Mailing List
In-Reply-To: <CACsJy8B3Qy1mra_GwhbYdk5LFrgYx=6NRqq=wvw2_wcWCO_yNw@mail.gmail.com>
Hi Duy,
Am 2017-03-02 um 10:31 schrieb Duy Nguyen:
> You could also try "git fast-export --anonymize" (read the doc first).
Eventually this was not needed, but thanks for letting me know about it!
I've not been aware of this feature beforehand, learned something.
Best regards,
Carsten
^ permalink raw reply
* Re: [PATCH v2 2/5] Use -y where possible in test t7610-mergetool
From: Denton Liu @ 2017-03-03 21:21 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, davvid, Johannes.Schindelin
In-Reply-To: <xmqqd1dy880d.fsf@gitster.mtv.corp.google.com>
On Fri, Mar 03, 2017 at 11:39:30AM -0800, Junio C Hamano wrote:
> Denton Liu <liu.denton@gmail.com> writes:
>
> > In these tests, there are many situations where
> > 'echo "" | git mergetool' is used. This replaces all of those
> > occurrences with 'git mergetool -y' for simplicity and readability.
>
> "-y where _possible_" is not necessarily a good thing to do in
> tests. We do want to make sure that both "yes" from the input and
> "-y" from the command line work. Changes to "-y" done only for the
> tests that are not about accepting the interactive input from the
> end-user is a very good idea, but "replaces all of those" makes me
> worried.
The 'custom mergetool' test case seems like a good place to introduce an
interactive test. Would the following patch to my patch work?
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index b6a419258..71624583c 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -135,8 +135,8 @@ test_expect_success 'custom mergetool' '
test_expect_code 1 git merge master >/dev/null 2>&1 &&
git mergetool -y both >/dev/null 2>&1 &&
git mergetool -y file1 file1 &&
- git mergetool -y file2 "spaced name" >/dev/null 2>&1 &&
- git mergetool -y subdir/file3 >/dev/null 2>&1 &&
+ ( yes "" | git mergetool file2 "spaced name" >/dev/null 2>&1 ) &&
+ ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
> > - ( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
> > - ( yes "" | git mergetool file2 >/dev/null 2>&1 ) &&
> > - ( yes "" | git mergetool "spaced name" >/dev/null 2>&1 ) &&
> > - ( yes "" | git mergetool both >/dev/null 2>&1 ) &&
> > - ( yes "" | git mergetool subdir/file3 >/dev/null 2>&1 ) &&
> > + git mergetool file1 >/dev/null 2>&1 &&
> > + git mergetool file2 >/dev/null 2>&1 &&
> > + git mergetool "spaced name" >/dev/null 2>&1 &&
> > + git mergetool both >/dev/null 2>&1 &&
> > + git mergetool subdir/file3 >/dev/null 2>&1 &&
>
> The reason for the lack of "-y" in the rewrite, in contrast to what
> was done in the previous hunk we saw, is not quite obvious.
>
Sorry, it was my mistake. I had forgotten to replace the '-y'. I have
corrected this for a future revision.
^ permalink raw reply related
* Re: [PATCH 3/3] Remove outdated info in difftool manpage
From: Denton Liu @ 2017-03-03 21:28 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, davvid
In-Reply-To: <alpine.DEB.2.20.1703031645470.3767@virtualbox>
On Fri, Mar 03, 2017 at 04:46:36PM +0100, Johannes Schindelin wrote:
> Hi Denton (or should I address you as Liu?),
Denton is fine, thanks.
>
> On Fri, 3 Mar 2017, Denton Liu wrote:
>
> > When difftool was rewritten in C, it removed the capability to read
> > fallback configs from mergetool. This changes the documentation to
> > reflect this.
>
> Thanks for pointing that out. But that is probably an oversight on my
> part, not an intentional change...
Do you expect to be submitting a patch for this soon? Or, if not, would
it be fine if I went ahead and did it?
>
> Ciao,
> Johannes
^ permalink raw reply
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