* [PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
Add the `PATHSPEC_FROMROOT` flag to allow callers to instruct
'parse_pathspec()' that all provided pathspecs are relative to the root
of the repository. This allows a caller to prevent a path that may be
outside of the repository from erroring out during the pathspec struct
construction.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
pathspec.c | 2 +-
pathspec.h | 2 ++
2 files changed, 3 insertions(+), 1 deletion(-)
diff --git a/pathspec.c b/pathspec.c
index 7ababb315..ff622ba18 100644
--- a/pathspec.c
+++ b/pathspec.c
@@ -353,7 +353,7 @@ static void init_pathspec_item(struct pathspec_item *item, unsigned flags,
if (pathspec_prefix >= 0) {
match = xstrdup(copyfrom);
prefixlen = pathspec_prefix;
- } else if (magic & PATHSPEC_FROMTOP) {
+ } else if ((magic & PATHSPEC_FROMTOP) || (flags & PATHSPEC_FROMROOT)) {
match = xstrdup(copyfrom);
prefixlen = 0;
} else {
diff --git a/pathspec.h b/pathspec.h
index 49fd823dd..cad197436 100644
--- a/pathspec.h
+++ b/pathspec.h
@@ -66,6 +66,8 @@ struct pathspec {
* allowed, then it will automatically set for every pathspec.
*/
#define PATHSPEC_LITERAL_PATH (1<<8)
+/* For callers that know all paths are relative to the root of the repository */
+#define PATHSPEC_FROMROOT (1<<9)
extern void parse_pathspec(struct pathspec *pathspec,
unsigned magic_mask,
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 0/5] recursing submodules with relative pathspec (grep and ls-files)
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
It was discovered that when using the --recurse-submodules flag with `git grep`
and `git ls-files` and specifying a relative path when not at the root causes
the child processes spawned to error out with an error like:
fatal: ..: '..' is outside repository
While true that ".." is outside the scope of the submodule repository, it
probably doesn't make much sense to the user who gave that pathspec with
respect to the superproject. Since the child processes that are spawned to
handle the submodules have some context that they are executing underneath a
superproject (via the 'super_prefix'), they should be able to prevent dying
under this circumstance.
This series fixes this bug in both git grep and git ls-files as well as
correctly formatting the output from submodules to handle the relative paths
with "..".
One of the changes made to fix this was to add an additional flag for the
parse_pathspec() function in order to treat all paths provided as being from
the root of the repository. I hesitantly selected the name 'PATHSPEC_FROMROOT'
but I'm not fond of it since its too similar to the pathspec magic define
'PATHSPEC_FROMTOP'. So I'm open for naming suggestions.
Brandon Williams (5):
grep: illustrate bug when recursing with relative pathspec
pathspec: add PATHSPEC_FROMROOT flag
grep: fix bug when recuring with relative pathspec
ls-files: illustrate bug when recursing with relative pathspec
ls-files: fix bug when recuring with relative pathspec
builtin/grep.c | 8 ++++--
builtin/ls-files.c | 8 ++++--
pathspec.c | 2 +-
pathspec.h | 2 ++
t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++
t/t7814-grep-recurse-submodules.sh | 42 ++++++++++++++++++++++++++++
6 files changed, 107 insertions(+), 5 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* [PATCH 3/5] grep: fix bug when recuring with relative pathspec
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
Fix a bug which causes a child process for a submodule to error out when
a relative pathspec with a ".." is provided in the superproject.
While at it, correctly construct the super-prefix to be used in a
submodule when not at the root of the repository.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/grep.c | 8 ++++++--
t/t7814-grep-recurse-submodules.sh | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/grep.c b/builtin/grep.c
index 2c727ef49..65f3413d1 100644
--- a/builtin/grep.c
+++ b/builtin/grep.c
@@ -538,6 +538,7 @@ static int grep_submodule_launch(struct grep_opt *opt,
int status, i;
const char *end_of_base;
const char *name;
+ struct strbuf buf = STRBUF_INIT;
struct work_item *w = opt->output_priv;
end_of_base = strchr(gs->name, ':');
@@ -550,9 +551,11 @@ static int grep_submodule_launch(struct grep_opt *opt,
argv_array_push(&cp.env_array, GIT_DIR_ENVIRONMENT);
/* Add super prefix */
+ quote_path_relative(name, opt->prefix, &buf);
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
super_prefix ? super_prefix : "",
- name);
+ buf.buf);
+ strbuf_release(&buf);
argv_array_push(&cp.args, "grep");
/*
@@ -1199,7 +1202,8 @@ int cmd_grep(int argc, const char **argv, const char *prefix)
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
- (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0),
+ (opt.max_depth != -1 ? PATHSPEC_MAXDEPTH_VALID : 0) |
+ (super_prefix ? PATHSPEC_FROMROOT : 0),
prefix, argv + i);
pathspec.max_depth = opt.max_depth;
pathspec.recursive = 1;
diff --git a/t/t7814-grep-recurse-submodules.sh b/t/t7814-grep-recurse-submodules.sh
index 418ba68fe..e0932b2b7 100755
--- a/t/t7814-grep-recurse-submodules.sh
+++ b/t/t7814-grep-recurse-submodules.sh
@@ -227,7 +227,7 @@ test_expect_success 'grep history with moved submoules' '
test_cmp expect actual
'
-test_expect_failure 'grep using relative path' '
+test_expect_success 'grep using relative path' '
test_when_finished "rm -rf parent sub" &&
git init sub &&
echo "foobar" >sub/file &&
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 5/5] ls-files: fix bug when recuring with relative pathspec
From: Brandon Williams @ 2017-02-24 23:51 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
Fix a bug which causes a child process for a submodule to error out when a
relative pathspec with a ".." is provided in the superproject.
While at it, correctly construct the super-prefix to be used in a submodule
when not at the root of the repository.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
builtin/ls-files.c | 8 ++++++--
t/t3007-ls-files-recurse-submodules.sh | 2 +-
2 files changed, 7 insertions(+), 3 deletions(-)
diff --git a/builtin/ls-files.c b/builtin/ls-files.c
index 159229081..89533ab8e 100644
--- a/builtin/ls-files.c
+++ b/builtin/ls-files.c
@@ -194,12 +194,15 @@ static void compile_submodule_options(const struct dir_struct *dir, int show_tag
static void show_gitlink(const struct cache_entry *ce)
{
struct child_process cp = CHILD_PROCESS_INIT;
+ struct strbuf name = STRBUF_INIT;
int status;
int i;
+ quote_path_relative(ce->name, prefix, &name);
argv_array_pushf(&cp.args, "--super-prefix=%s%s/",
super_prefix ? super_prefix : "",
- ce->name);
+ name.buf);
+ strbuf_release(&name);
argv_array_push(&cp.args, "ls-files");
argv_array_push(&cp.args, "--recurse-submodules");
@@ -615,7 +618,8 @@ int cmd_ls_files(int argc, const char **argv, const char *cmd_prefix)
parse_pathspec(&pathspec, 0,
PATHSPEC_PREFER_CWD |
- PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP,
+ PATHSPEC_STRIP_SUBMODULE_SLASH_CHEAP |
+ (super_prefix ? PATHSPEC_FROMROOT : 0),
prefix, argv);
/*
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index d8ab10866..1522ed235 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -188,7 +188,7 @@ test_expect_success '--recurse-submodules and pathspecs' '
test_cmp expect actual
'
-test_expect_failure '--recurse-submodules and relative paths' '
+test_expect_success '--recurse-submodules and relative paths' '
# From top works
cat >expect <<-\EOF &&
.gitmodules
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 4/5] ls-files: illustrate bug when recursing with relative pathspec
From: Brandon Williams @ 2017-02-24 23:50 UTC (permalink / raw)
To: git; +Cc: Brandon Williams, sbeller, pclouds
In-Reply-To: <20170224235100.52627-1-bmwill@google.com>
When using the --recurse-submodules flag with a relative pathspec which
includes "..", an error is produced inside the child process spawned for a
submodule. When creating the pathspec struct in the child, the ".." is
interpreted to mean "go up a directory" which causes an error stating that the
path ".." is outside of the repository.
While it is true that ".." is outside the scope of the submodule, it is
confusing to a user who originally invoked the command where ".." was indeed
still inside the scope of the superproject. Since the child process luanched
for the submodule has some context that it is operating underneath a
superproject, this error could be avoided.
Signed-off-by: Brandon Williams <bmwill@google.com>
---
t/t3007-ls-files-recurse-submodules.sh | 50 ++++++++++++++++++++++++++++++++++
1 file changed, 50 insertions(+)
diff --git a/t/t3007-ls-files-recurse-submodules.sh b/t/t3007-ls-files-recurse-submodules.sh
index a5426171d..d8ab10866 100755
--- a/t/t3007-ls-files-recurse-submodules.sh
+++ b/t/t3007-ls-files-recurse-submodules.sh
@@ -188,6 +188,56 @@ test_expect_success '--recurse-submodules and pathspecs' '
test_cmp expect actual
'
+test_expect_failure '--recurse-submodules and relative paths' '
+ # From top works
+ cat >expect <<-\EOF &&
+ .gitmodules
+ a
+ b/b
+ h.txt
+ sib/file
+ sub/file
+ submodule/.gitmodules
+ submodule/c
+ submodule/f.TXT
+ submodule/g.txt
+ submodule/subsub/d
+ submodule/subsub/e.txt
+ EOF
+ git ls-files --recurse-submodules >actual &&
+ test_cmp expect actual &&
+
+ # Relative path to top errors out
+ cat >expect <<-\EOF &&
+ ../.gitmodules
+ ../a
+ b
+ ../h.txt
+ ../sib/file
+ ../sub/file
+ ../submodule/.gitmodules
+ ../submodule/c
+ ../submodule/f.TXT
+ ../submodule/g.txt
+ ../submodule/subsub/d
+ ../submodule/subsub/e.txt
+ EOF
+ git -C b ls-files --recurse-submodules -- .. >actual &&
+ test_cmp expect actual &&
+
+ # Relative path to submodule errors out
+ cat >expect <<-\EOF &&
+ ../submodule/.gitmodules
+ ../submodule/c
+ ../submodule/f.TXT
+ ../submodule/g.txt
+ ../submodule/subsub/d
+ ../submodule/subsub/e.txt
+ EOF
+ git -C b ls-files --recurse-submodules -- ../submodule >actual &&
+ test_cmp expect actual
+'
+
test_expect_success '--recurse-submodules does not support --error-unmatch' '
test_must_fail git ls-files --recurse-submodules --error-unmatch 2>actual &&
test_i18ngrep "does not support --error-unmatch" actual
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: SHA1 collisions found
From: Ian Jackson @ 2017-02-25 0:06 UTC (permalink / raw)
To: Junio C Hamano, Joey Hess, git
In-Reply-To: <22704.50445.435156.883001@chiark.greenend.org.uk>
Ian Jackson writes ("Re: SHA1 collisions found"):
> The idea of using the length is a neat trick, but it cannot support
> the dcurrent object name abbreviation approach unworkable.
Sorry, it's late here and my grammar seems to have disintegrated !
Ian.
^ permalink raw reply
* Re: SHA1 collisions found
From: ankostis @ 2017-02-25 0:31 UTC (permalink / raw)
To: Junio C Hamano, git
Cc: Stefan Beller, David Lang, Ian Jackson, Joey Hess, git
In-Reply-To: <xmqqh93j1g9n.fsf@gitster.mtv.corp.google.com>
On 24 February 2017 at 21:32, Junio C Hamano <gitster@pobox.com> wrote:
> ankostis <ankostis@gmail.com> writes:
>
>> Let's assume that git is retroffited to always support the "default"
>> SHA-3, but support additionally more hash-funcs.
>> If in the future SHA-3 also gets defeated, it would be highly unlikely
>> that the same math would also break e.g. Blake.
>> So certain high-profile repos might choose for extra security 2 or more hashes.
>
> I think you are conflating two unrelated things.
I believe the two distinct things you refer to below are these:
a. storing objects in filesystem and accessing them
by name (e.g. from cmdline), and
b. cross-referencing inside the objects (trees, tags, notes),
correct?
If not, then please ignore my answers, below.
> * How are these "2 or more hashes" actually used? Are you going to
> add three "parent " line to a commit with just one parent, each
> line storing the different hashes?
Yes, in all places where references are involved (tags, notes).
Based on what what the git-hackers have written so far, this might be doable.
To ensure integrity in the case of crypto-failures, all objects must
cross-reference each other with multiple hashes.
Of course this extra security would stop as soon as you reach "old"
history (unless you re-write it).
> How will such a commit object
> be named---does it have three names and do you plan to have three
> copies of .git/refs/heads/master somehow, each of which have
> SHA-1, SHA-3 and Blake, and let any one hash to identify the
> object?
Yes, based on Jason Cooper's idea, above, objects would be stored
under all names in the filesystem using hard links (although this
might not work nice on Windows).
> I suspect you are not going to do so; instead, you would use a
> very long string that is a concatenation of these three hashes as
> if it is an output from a single hash function that produces a
> long result.
>
> So I think the most natural way to do the "2 or more for extra
> security" is to allow us to use a very long hash. It does not
> help to allow an object to be referred to with any of these 2 or
> more hashes at the same time.
If hard-linking all names is doable, then most restrictions above are
gone, correct?
> * If employing 2 or more hashes by combining into one may enhance
> the security, that is wonderful. But we want to discourage
> people from inventing their own combinations left and right and
> end up fragmenting the world. If a project that begins with
> SHA-1 only naming is forked to two (or more) and each fork uses
> different hashes, merging them back will become harder than
> necessary unless you support all these hashes forks used.
Agree on discouraging people's inventions.
That is why I believe that some HASH (e.g. SHA-3) must be the blessed one.
All git >= 3.x.x must support at least this one (for naming and
cross-referencing between objects).
> Having said all that, the way to figure out the hash used in the way
> we spell the object name may not be the best place to discourage
> people from using random hashes of their choice. But I think we
> want to avoid doing something that would actively encourage
> fragmentation.
I guess the "blessed SHA-3 will discourage people using the other
names., untill the next crypto-crack.
^ permalink raw reply
* Re: [PATCH 2/5] pathspec: add PATHSPEC_FROMROOT flag
From: Stefan Beller @ 2017-02-25 0:31 UTC (permalink / raw)
To: Brandon Williams; +Cc: git@vger.kernel.org, Duy Nguyen
In-Reply-To: <20170224235100.52627-3-bmwill@google.com>
On Fri, Feb 24, 2017 at 3:50 PM, Brandon Williams <bmwill@google.com> wrote:
> Add the `PATHSPEC_FROMROOT` flag to allow callers to instruct
> 'parse_pathspec()' that all provided pathspecs are relative to the root
> of the repository. This allows a caller to prevent a path that may be
> outside of the repository from erroring out during the pathspec struct
> construction.
>
> +/* For callers that know all paths are relative to the root of the repository */
> +#define PATHSPEC_FROMROOT (1<<9)
What is the calling convention for these submodule pathspecs?
IIRC, we'd pass on the super-prefix and the literal pathspec,
e.g. when there is a submodule "sub" inside the superproject,
The invocation on the submodule would be
git FOO --super-prefix="sub" <arguments> -- sub/pathspec/inside/...
and then the submodule process would need to "subtract" the superprefix
from the pathspec argument to see its actual pathspec, e.g. in gerrit:
$ GIT_TRACE=1 git grep --recurse-submodules -i test -- \
plugins/cookbook-plugin/
...
trace: run_command: '--super-prefix=plugins/cookbook-plugin/' 'grep' \
'--recurse-submodules' '-i' '-etest' '--threads=4' '--'
'plugins/cookbook-plugin/'
..
but also:
...
trace: run_command: '--super-prefix=plugins/download-commands/' 'grep' \
'--recurse-submodules' '-i' '-etest' '--threads=4' '--'
'plugins/cookbook-plugin/'
...
So if I change into a directory:
$ cd plugins
plugins$ git grep --recurse-submodules -i test -- cookbook-plugin/
plugins$ #empty?
plugins$ git grep --recurse-submodules -i test -- plugins/cookbook-plugin/
...
Usual output, so the pathspecs are absolute path to the superprojects
root? Let's try relative path:
plugins$ git grep --recurse-submodules -i test -- ../plugins/cookbook-plugin/
fatal: ../plugins/cookbook-plugin/: '../plugins/cookbook-plugin/' is
outside repository
...
Running with GIT_TRACE=1:
trace: run_command: '--super-prefix=plugins/cookbook-plugin/' 'grep' \
'--recurse-submodules' '-i' '-etest' '--threads=4' '--'
'../plugins/cookbook-plugin/'
that seems like a mismatch of pathspec and superproject prefix, the prefix ought
to be different then? Maybe also including ../ because that is the
relative path from
cwd to the superporojects root and that is where we anchor all paths?
Easy to test that out:
plugins$ GIT_TRACE=1 git --super-prefix=../ grep --recurse-submodules \
-i test -- ../plugins/cookbook-plugin/
fatal: can't use --super-prefix from a subdirectory
ok, not as easy. :/
So another test with relative path:
(in git.git)
cd t/diff-lib
t/diff-lib$ git grep freedom
COPYING:freedom to share and change it. By contrast, the GNU General Public
...
So the path displayed is relative to the cwd (and the search results as well)
In the submodule case we would expect to have the super prefix
to be computed to be relative to the cwd?
Checking the tests, this is handled correctly with this patch series. :)
But nevertheless, I think I know why I dislike this approach now:
The super prefix is handled "too dumb" IMHO, see the case
plugins$ git grep test -- cookbook-plugin/
above, that doesn't correctly figure out the correct output.
Although this might be a separate bug, but it sounds like it
is the same underlying issue.
--
for the naming: How about PATHSPEC_FROMOUTSIDE
when going with the series as is here?
(the superprefix is not resolved, so the pathspecs given are
literally pathspecs that are outside this repo and we can ignore
them?
Thanks,
Stefan
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-25 0:39 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, Git Mailing List
In-Reply-To: <20170224233929.p2yckbc6ksyox5nu@sigill.intra.peff.net>
On Fri, Feb 24, 2017 at 3:39 PM, Jeff King <peff@peff.net> wrote:
>
> One thing I worry about in a mixed-hash setting is how often the two
> will be mixed.
Honestly, I think that a primary goal for a new hash implementation
absolutely needs to be to minimize mixing.
Not for security issues, but because of combinatorics. You want to
have a model that basically reads old data, but that very aggressively
approaches "new data only" in order to avoid the situation where you
have basically the exact same tree state, just _represented_
differently.
For example, what I would suggest the rules be is something like this:
- introduce new tag2/commit2/tree2/blob2 object type tags that imply
that they were hashed using the new hash
- an old type obviously can never contain a pointer to a new type (ie
you can't have a "tree" object that contains a tree2 object or a blob2
object.
- but also make the rule that a *new* type can never contain a
pointer to an old type, with the *very* specific exception that a
commit2 can have a parent that is of type "commit".
That way everything "converges" towards the new format: the only way
you can stay on the old format is if you only have old-format objects,
and once you have a new-format object all your objects are going to be
new format - except for the history.
Obviously, if somebody stays in old format, you might end up still
getting some object duplication when you continue to merge from him,
but that tree can never merge back without converting to new-format,
so it will be a temporary situation.
So you will end up with duplicate objects, and that's not good (think
of what it does to all our full-tree "diff" optimizations, for example
- you no longer get the "these sub-trees are identical" across a
format change), but realistically you'll have a very limited time of
that kind of duplication.
I'd furthermore suggest that from a UI standpoint, we'd
- convert to 64-character hex numbers (32-byte hashes)
- (as mentioned earlier) default to a 40-character abbreviation
- make the old 40-character SHA1's just show up within the same
address space (so they'd also be encoded as 32-byte hashes, just with
the last 12 bytes zero).
- you'd see in the "object->type" whether it's a new or old-style hash.
I suspect it shouldn't be too painful to do it that way.
Linus
^ permalink raw reply
* Re: [PATCH] mingw: use OpenSSL's SHA-1 routines
From: Linus Torvalds @ 2017-02-25 0:44 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Git Mailing List, Jeff Hostetler,
Junio C Hamano
In-Reply-To: <20170210050237.gajicliueuvk6s5d@sigill.intra.peff.net>
On Thu, Feb 9, 2017 at 9:02 PM, Jeff King <peff@peff.net> wrote:
>
> I think this is only half the story. A heavy-sha1 workload is faster,
> which is good. But one of the original reasons to prefer blk-sha1 (at
> least on Linux) is that resolving libcrypto.so symbols takes a
> non-trivial amount of time. I just timed it again, and it seems to be
> consistently 1ms slower to run "git rev-parse --git-dir" on my machine
> (from the top-level of a repo).
Yes. It's also a horrible plain to profile those things.
Avoiding openssl was a great thing, because it avoided a lot of crazy overhead.
I suspect that most of the openssl win comes from using the actual SHA
instructions on modern CPU's. Because last I looked, the hand-coded
assembly simply wasn't that much faster.
We could easily have some x86-specific library that just does "use SHA
instructions if you have them, use blk-sha1 otherwise".
Of course, if we end up using the collision checking SHA libraries
this is all moot anyway.
Linus
^ permalink raw reply
* Re: SHA1 collisions found
From: Linus Torvalds @ 2017-02-25 0:54 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFw6BLjPK-F0RGd9LT7X5xosKOXOxuhmKX65ZHn09r1xow@mail.gmail.com>
On Fri, Feb 24, 2017 at 4:39 PM, Linus Torvalds
<torvalds@linux-foundation.org> wrote:
>
> - you'd see in the "object->type" whether it's a new or old-style hash.
Actually, I take that back. I think it might be easier to keep
"object->type" as-is, and it would only show the current OBJ_xyz
fields. Then writing the SHA ends up deciding whether a OBJ_COMMIT
gets written as "commit" or "commit2".
With the reachability rules, you'd never have any ambiguity about which to use.
Linus
^ permalink raw reply
* Re: SHA1 collisions found
From: David Lang @ 2017-02-25 1:00 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, git
In-Reply-To: <20170224233929.p2yckbc6ksyox5nu@sigill.intra.peff.net>
On Fri, 24 Feb 2017, Jeff King wrote:
>
> So I'd much rather see strong rules like:
>
> 1. Once a repo has flag-day switched over to the new hash format[1],
> new references are _always_ done with the new hash. Even ones that
> point to pre-flag-day objects!
how do you define when a repo has "switched over" to the new format in a
distributed environment?
so you have one person working on a project that switches their version of git
to the new one that uses the new format.
But other people they interact with still use older versions of git
what happens when you have someone working on two different projects where one
has switched and the other hasn't?
what if they are forks of each other? (LEDE and OpenWRT, or just linux-kernel
and linux-kernel-stable)
> So you get a "commit-v2" object instead of a "commit", and it has a
> distinct hash identity from its "commit" counterpart. You can point
> to a classic "commit", but you do so by its new-hash.
>
> The flag-day switch would probably be a repo config flag based on
> repositoryformatversion (so old versions would just punt if they
> see it). Let's call this flag "newhash" for lack of a better term.
so how do you interact with someone who only expects the old commit instead of
the commit-v2?
David Lang
^ permalink raw reply
* Re: SHA1 collisions found
From: Stefan Beller @ 2017-02-25 1:15 UTC (permalink / raw)
To: David Lang
Cc: Jeff King, Junio C Hamano, Ian Jackson, Joey Hess,
git@vger.kernel.org
In-Reply-To: <nycvar.QRO.7.75.62.1702241656010.6590@qynat-yncgbc>
On Fri, Feb 24, 2017 at 5:00 PM, David Lang <david@lang.hm> wrote:
> On Fri, 24 Feb 2017, Jeff King wrote:
>
>>
>> So I'd much rather see strong rules like:
>>
>> 1. Once a repo has flag-day switched over to the new hash format[1],
>> new references are _always_ done with the new hash. Even ones that
>> point to pre-flag-day objects!
>
>
> how do you define when a repo has "switched over" to the new format in a
> distributed environment?
>
> so you have one person working on a project that switches their version of
> git to the new one that uses the new format.
>
> But other people they interact with still use older versions of git
>
> what happens when you have someone working on two different projects where
> one has switched and the other hasn't?
you get infected by the "new version requirement"
as soon as you pull? (GPL is cancer, anyone? ;)
If you are using an old version of git that doesn't understand the new version,
you're screwed.
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-25 1:16 UTC (permalink / raw)
To: Linus Torvalds; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, Git Mailing List
In-Reply-To: <CA+55aFw6BLjPK-F0RGd9LT7X5xosKOXOxuhmKX65ZHn09r1xow@mail.gmail.com>
On Fri, Feb 24, 2017 at 04:39:45PM -0800, Linus Torvalds wrote:
> For example, what I would suggest the rules be is something like this:
>
> - introduce new tag2/commit2/tree2/blob2 object type tags that imply
> that they were hashed using the new hash
>
> - an old type obviously can never contain a pointer to a new type (ie
> you can't have a "tree" object that contains a tree2 object or a blob2
> object.
>
> - but also make the rule that a *new* type can never contain a
> pointer to an old type, with the *very* specific exception that a
> commit2 can have a parent that is of type "commit".
Yeah, this is exactly what I had in mind. That way everybody in
"newhash" mode has no decisions to make. They follow the same rules and
it's as if sha1 never existed, except when you follow links in
historical objects.
> [in reply...]
> Actually, I take that back. I think it might be easier to keep
> "object->type" as-is, and it would only show the current OBJ_xyz
> fields. Then writing the SHA ends up deciding whether a OBJ_COMMIT
> gets written as "commit" or "commit2".
Yeah, I think there are some data structures with limited bits for the
"type" fields (e.g., the pack format). So sticking with OBJ_COMMIT might
be nice. For commits and tags, it would be nice to have an "I'm v2"
header at the start so there's no confusion about how they are meant to
be interpreted.
Trees are more difficult, as they don't have any such field. But a valid
tree does need to start with a mode, so sticking some non-numeric flag
at the front of the object would work (it breaks backwards
compatibility, but that's kind of the point).
I dunno. Maybe we do not need those markers at all, and could get by
purely on object-length, or annotating the headers in some way (like
"parent sha256:1234abcd").
It might just be nice if we could very easily identify objects as one
type or the other without having to parse them in detail.
> So you will end up with duplicate objects, and that's not good (think
> of what it does to all our full-tree "diff" optimizations, for example
> - you no longer get the "these sub-trees are identical" across a
> format change), but realistically you'll have a very limited time of
> that kind of duplication.
Yeah, cross-flag-day diffs will be more expensive. I think that's
something we have to live with. I was thinking originally that the
sha1->newhash mapping might solve that, but it only works at the blob
level. I.e., you can compare a sha1 and a newhash like:
if (!hashcmp(sha1_to_newhash(a), b))
without having to look at the contents. But it doesn't work recursively,
because the tree-pointing-to-newhash will have different content.
-Peff
^ permalink raw reply
* [PATCH 2/3] revision: exclude trees/blobs given commit
From: Jonathan Tan @ 2017-02-25 1:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, peartben, benpeart
In-Reply-To: <cover.1487984670.git.jonathantanmy@google.com>
When the --objects argument is given to rev-list, an argument of the
form "^$tree" can be given to exclude all blobs and trees reachable from
that tree, but an argument of the form "^$commit" only excludes that
commit, not any blob or tree reachable from it. Make "^$commit" behave
consistent to "^$tree".
Also, formalize this behavior in unit tests. (Some of the added tests
would already pass even before this commit, but are included
nevertheless for completeness.)
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
revision.c | 2 ++
t/t6000-rev-list-misc.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
2 files changed, 90 insertions(+)
diff --git a/revision.c b/revision.c
index 5e49d9e0e..e6a62da98 100644
--- a/revision.c
+++ b/revision.c
@@ -254,6 +254,8 @@ static struct commit *handle_commit(struct rev_info *revs,
die("unable to parse commit %s", name);
if (flags & UNINTERESTING) {
mark_parents_uninteresting(commit);
+ if (revs->tree_and_blob_objects)
+ mark_tree_uninteresting(commit->tree);
revs->limited = 1;
}
if (revs->show_source && !commit->util)
diff --git a/t/t6000-rev-list-misc.sh b/t/t6000-rev-list-misc.sh
index 969e4e9e5..c04a9582b 100755
--- a/t/t6000-rev-list-misc.sh
+++ b/t/t6000-rev-list-misc.sh
@@ -114,4 +114,92 @@ test_expect_success '--header shows a NUL after each commit' '
test_cmp expect actual
'
+test_expect_success 'setup tree-granularity and blob-granularity tests' '
+ echo 0 >file &&
+
+ mkdir subdir &&
+ echo 1 >subdir/file &&
+ git add file subdir/file &&
+ git commit -m one &&
+
+ echo 2 >subdir/file &&
+ git add subdir/file &&
+ git commit -m two &&
+
+ commit1=$(git rev-parse HEAD^1) &&
+ commit2=$(git rev-parse HEAD) &&
+ tree1=$(git rev-parse $commit1^{tree}) &&
+ tree2=$(git rev-parse $commit2^{tree}) &&
+ subtree1=$(git cat-file -p $tree1 | grep "subdir" | cut -c13-52) &&
+ subtree2=$(git cat-file -p $tree2 | grep "subdir" | cut -c13-52) &&
+ blob0=$(echo 0 | git hash-object --stdin) &&
+ blob1=$(echo 1 | git hash-object --stdin) &&
+ blob2=$(echo 2 | git hash-object --stdin)
+'
+
+test_expect_success 'include commit, exclude blob' '
+ git rev-list --objects $commit2 >out &&
+ grep "$blob1" out &&
+ grep "$blob2" out &&
+
+ git rev-list --objects $commit2 "^$blob2" >out &&
+ grep "$blob1" out &&
+ ! grep "$blob2" out
+'
+
+test_expect_success 'include commit, exclude tree (also excludes nested trees/blobs)' '
+ git rev-list --objects $commit2 "^$tree2" >out &&
+ grep "$tree1" out &&
+ grep "$subtree1" out &&
+ grep "$blob1" out &&
+ ! grep "$tree2" out &&
+ ! grep "$subtree2" out &&
+ ! grep "$blob2" out &&
+
+ git rev-list --objects $commit2 "^$subtree2" >out &&
+ grep "$tree1" out &&
+ grep "$subtree1" out &&
+ grep "$blob1" out &&
+ grep "$tree2" out &&
+ ! grep "$subtree2" out &&
+ ! grep "$blob2" out
+'
+
+test_expect_success 'include tree, exclude commit' '
+ git rev-list --objects "$tree1" "^$commit2" >out &&
+ ! grep "$blob0" out && # common to both
+ grep "$blob1" out && # only in tree
+ ! grep "$blob2" out # only in commit
+'
+
+test_expect_success 'include tree, exclude tree' '
+ git rev-list --objects "$tree1" "^$tree2" >out &&
+ ! grep "$blob0" out && # common to both
+ grep "$blob1" out && # only in tree1
+ ! grep "$blob2" out # only in tree2
+'
+
+test_expect_success 'include tree, exclude blob' '
+ git rev-list --objects "$tree1" "^$blob2" >out &&
+ grep "$blob0" out &&
+ grep "$blob1" out &&
+ ! grep "$blob2" out
+'
+
+test_expect_success 'include blob, exclude commit' '
+ git rev-list --objects "$blob2" "^$commit1" >out &&
+ grep "$blob2" out &&
+
+ git rev-list --objects "$blob2" "^$commit2" >out &&
+ ! grep "$blob2" out
+'
+
+test_expect_success 'include blob, exclude tree' '
+ git rev-list --objects "$blob2" "^$tree1" >out &&
+ grep "$blob2" out &&
+
+ git rev-list --objects "$blob2" "^$tree2" >out &&
+ ! grep "$blob2" out
+'
+
test_done
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 0/3] Test fetch-pack's ability to fetch arbitrary blobs
From: Jonathan Tan @ 2017-02-25 1:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, peartben, benpeart
As stated in a previous e-mail [1], I was trying to think a way to allow
Git to fetch arbitrary blobs from another Git server, and it turned out
that fetch-pack already can. However, there were some bugs with blob
reachability. This patch set fixes those bugs, and verifies (with tests)
that fetch-pack can fetch reachable blobs and cannot fetch unreachable
blobs.
These patches are (I think) worthwhile on their own, but may be of
special interest to people who need Git to tolerate missing objects in
the local repo (for example, the e-mail discussion "[RFC] Add support
for downloading blobs on demand" [2]) because a way for Git to download
missing objects natively is (I think) a prerequisite to that.
[1] <20170223230358.30050-1-jonathantanmy@google.com>
[2] <20170113155253.1644-1-benpeart@microsoft.com>
Jonathan Tan (3):
revision: unify {tree,blob}_objects in rev_info
revision: exclude trees/blobs given commit
upload-pack: compute blob reachability correctly
bisect.c | 2 +-
builtin/rev-list.c | 6 ++--
list-objects.c | 4 +--
pack-bitmap-write.c | 3 +-
pack-bitmap.c | 3 +-
reachable.c | 3 +-
revision.c | 18 +++++-----
revision.h | 3 +-
t/t5500-fetch-pack.sh | 30 +++++++++++++++++
t/t6000-rev-list-misc.sh | 88 ++++++++++++++++++++++++++++++++++++++++++++++++
upload-pack.c | 15 +++++++++
11 files changed, 151 insertions(+), 24 deletions(-)
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply
* [PATCH 1/3] revision: unify {tree,blob}_objects in rev_info
From: Jonathan Tan @ 2017-02-25 1:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, peartben, benpeart
In-Reply-To: <cover.1487984670.git.jonathantanmy@google.com>
Whenever tree_objects is set to 1 in revision.h's struct rev_info,
blob_objects is likewise set, and vice versa. Combine those two fields
into one.
Some of the existing code does not handle tree_objects being different
from blob_objects properly. For example, "handle_commit" in revision.c
recurses from an UNINTERESTING tree into its subtree if tree_objects ==
1, completely ignoring blob_objects; it probably should still recurse if
tree_objects == 0 and blob_objects == 1 (to mark the blobs), and should
behave differently depending on blob_objects (controlling the
instantiation and marking of blob objects). This commit resolves the
issue by forbidding tree_objects from being different to blob_objects.
It could be argued that in the future, Git might need to distinguish
tree_objects from blob_objects - in particular, a user might want
rev-list to print the trees but not the blobs. However, this results in
a minor performance savings at best in that objects no longer need to be
instantiated (causing memory allocations and hashtable insertions) - no
disk reads are being done for objects whether blob_objects is set or
not.
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
bisect.c | 2 +-
builtin/rev-list.c | 6 +++---
list-objects.c | 4 ++--
pack-bitmap-write.c | 3 +--
pack-bitmap.c | 3 +--
reachable.c | 3 +--
revision.c | 16 ++++++----------
revision.h | 3 +--
8 files changed, 16 insertions(+), 24 deletions(-)
diff --git a/bisect.c b/bisect.c
index 8e63c40d2..265b32905 100644
--- a/bisect.c
+++ b/bisect.c
@@ -634,7 +634,7 @@ static void bisect_common(struct rev_info *revs)
{
if (prepare_revision_walk(revs))
die("revision walk setup failed");
- if (revs->tree_objects)
+ if (revs->tree_and_blob_objects)
mark_edges_uninteresting(revs, NULL);
}
diff --git a/builtin/rev-list.c b/builtin/rev-list.c
index 0aa93d589..6c2651b31 100644
--- a/builtin/rev-list.c
+++ b/builtin/rev-list.c
@@ -345,7 +345,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
revs.commit_format = CMIT_FMT_RAW;
if ((!revs.commits &&
- (!(revs.tag_objects || revs.tree_objects || revs.blob_objects) &&
+ (!(revs.tag_objects || revs.tree_and_blob_objects) &&
!revs.pending.nr)) ||
revs.diff)
usage(rev_list_usage);
@@ -374,7 +374,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
return 0;
}
} else if (revs.max_count < 0 &&
- revs.tag_objects && revs.tree_objects && revs.blob_objects) {
+ revs.tag_objects && revs.tree_and_blob_objects) {
if (!prepare_bitmap_walk(&revs)) {
traverse_bitmap_commit_list(&show_object_fast);
return 0;
@@ -384,7 +384,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)
if (prepare_revision_walk(&revs))
die("revision walk setup failed");
- if (revs.tree_objects)
+ if (revs.tree_and_blob_objects)
mark_edges_uninteresting(&revs, show_edge);
if (bisect_list) {
diff --git a/list-objects.c b/list-objects.c
index f3ca6aafb..796957105 100644
--- a/list-objects.c
+++ b/list-objects.c
@@ -18,7 +18,7 @@ static void process_blob(struct rev_info *revs,
struct object *obj = &blob->object;
size_t pathlen;
- if (!revs->blob_objects)
+ if (!revs->tree_and_blob_objects)
return;
if (!obj)
die("bad blob object");
@@ -78,7 +78,7 @@ static void process_tree(struct rev_info *revs,
all_entries_interesting: entry_not_interesting;
int baselen = base->len;
- if (!revs->tree_objects)
+ if (!revs->tree_and_blob_objects)
return;
if (!obj)
die("bad tree object");
diff --git a/pack-bitmap-write.c b/pack-bitmap-write.c
index 970559601..5080e276b 100644
--- a/pack-bitmap-write.c
+++ b/pack-bitmap-write.c
@@ -258,8 +258,7 @@ void bitmap_writer_build(struct packing_data *to_pack)
init_revisions(&revs, NULL);
revs.tag_objects = 1;
- revs.tree_objects = 1;
- revs.blob_objects = 1;
+ revs.tree_and_blob_objects = 1;
revs.no_walk = 0;
revs.include_check = should_include;
diff --git a/pack-bitmap.c b/pack-bitmap.c
index 39bcc1684..445a24e0d 100644
--- a/pack-bitmap.c
+++ b/pack-bitmap.c
@@ -951,8 +951,7 @@ void test_bitmap_walk(struct rev_info *revs)
die("Commit %s doesn't have an indexed bitmap", oid_to_hex(&root->oid));
revs->tag_objects = 1;
- revs->tree_objects = 1;
- revs->blob_objects = 1;
+ revs->tree_and_blob_objects = 1;
result_popcnt = bitmap_popcount(result);
diff --git a/reachable.c b/reachable.c
index d0199cace..9a3beb75f 100644
--- a/reachable.c
+++ b/reachable.c
@@ -166,8 +166,7 @@ void mark_reachable_objects(struct rev_info *revs, int mark_reflog,
* in all object types, not just commits.
*/
revs->tag_objects = 1;
- revs->blob_objects = 1;
- revs->tree_objects = 1;
+ revs->tree_and_blob_objects = 1;
/* Add all refs from the index file */
add_index_objects_to_pending(revs, 0);
diff --git a/revision.c b/revision.c
index b37dbec37..5e49d9e0e 100644
--- a/revision.c
+++ b/revision.c
@@ -267,7 +267,7 @@ static struct commit *handle_commit(struct rev_info *revs,
*/
if (object->type == OBJ_TREE) {
struct tree *tree = (struct tree *)object;
- if (!revs->tree_objects)
+ if (!revs->tree_and_blob_objects)
return NULL;
if (flags & UNINTERESTING) {
mark_tree_contents_uninteresting(tree);
@@ -281,7 +281,7 @@ static struct commit *handle_commit(struct rev_info *revs,
* Blob object? You know the drill by now..
*/
if (object->type == OBJ_BLOB) {
- if (!revs->blob_objects)
+ if (!revs->tree_and_blob_objects)
return NULL;
if (flags & UNINTERESTING)
return NULL;
@@ -1817,23 +1817,19 @@ static int handle_revision_opt(struct rev_info *revs, int argc, const char **arg
revs->limited = 1;
} else if (!strcmp(arg, "--objects")) {
revs->tag_objects = 1;
- revs->tree_objects = 1;
- revs->blob_objects = 1;
+ revs->tree_and_blob_objects = 1;
} else if (!strcmp(arg, "--objects-edge")) {
revs->tag_objects = 1;
- revs->tree_objects = 1;
- revs->blob_objects = 1;
+ revs->tree_and_blob_objects = 1;
revs->edge_hint = 1;
} else if (!strcmp(arg, "--objects-edge-aggressive")) {
revs->tag_objects = 1;
- revs->tree_objects = 1;
- revs->blob_objects = 1;
+ revs->tree_and_blob_objects = 1;
revs->edge_hint = 1;
revs->edge_hint_aggressive = 1;
} else if (!strcmp(arg, "--verify-objects")) {
revs->tag_objects = 1;
- revs->tree_objects = 1;
- revs->blob_objects = 1;
+ revs->tree_and_blob_objects = 1;
revs->verify_objects = 1;
} else if (!strcmp(arg, "--unpacked")) {
revs->unpacked = 1;
diff --git a/revision.h b/revision.h
index 9fac1a607..43cce137f 100644
--- a/revision.h
+++ b/revision.h
@@ -89,8 +89,7 @@ struct rev_info {
simplify_merges:1,
simplify_by_decoration:1,
tag_objects:1,
- tree_objects:1,
- blob_objects:1,
+ tree_and_blob_objects:1,
verify_objects:1,
edge_hint:1,
edge_hint_aggressive:1,
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* [PATCH 3/3] upload-pack: compute blob reachability correctly
From: Jonathan Tan @ 2017-02-25 1:18 UTC (permalink / raw)
To: git; +Cc: Jonathan Tan, gitster, peff, peartben, benpeart
In-Reply-To: <cover.1487984670.git.jonathantanmy@google.com>
If allowreachablesha1inwant is set, upload-pack will provide a blob to a
user, provided its hash, regardless of whether the blob is reachable or
not. Teach upload-pack to compute reachability correctly by passing the
"--objects" argument when it invokes "rev-list" if necessary.
This commit only affects the case where blob/tree hashes are provided to
upload-pack; the more typical case of only commit/tag hashes being
provided is not affected. In the case where blob/tree hashes are
provided, the reachability check is now slower (since trees need to be
read) but correct. (The user may still set allowanysha1inwant instead of
allowreachablesha1inwant to opt-out of the reachability check.)
Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
---
t/t5500-fetch-pack.sh | 30 ++++++++++++++++++++++++++++++
upload-pack.c | 15 +++++++++++++++
2 files changed, 45 insertions(+)
diff --git a/t/t5500-fetch-pack.sh b/t/t5500-fetch-pack.sh
index 505e1b4a7..a4ae888ff 100755
--- a/t/t5500-fetch-pack.sh
+++ b/t/t5500-fetch-pack.sh
@@ -547,6 +547,36 @@ test_expect_success 'fetch-pack can fetch a raw sha1' '
git fetch-pack hidden $(git -C hidden rev-parse refs/hidden/one)
'
+test_expect_success 'setup for tests that fetch blobs by hash' '
+ git init blobserver &&
+ test_commit -C blobserver 1 &&
+ test_commit -C blobserver 2 &&
+ test_commit -C blobserver 3 &&
+ blob1=$(echo 1 | git hash-object --stdin) &&
+ blob2=$(echo 2 | git hash-object --stdin) &&
+ blob3=$(echo 3 | git hash-object --stdin) &&
+
+ unreachable=$(echo 4 | git -C blobserver hash-object -w --stdin) &&
+ git -C blobserver cat-file -e "$unreachable"
+'
+
+test_expect_success 'fetch-pack can fetch reachable blobs by hash' '
+ test_config -C blobserver uploadpack.allowreachablesha1inwant 1 &&
+
+ git init reachabletest &&
+ git -C reachabletest fetch-pack ../blobserver "$blob1" "$blob2" &&
+ git -C reachabletest cat-file -e "$blob1" &&
+ git -C reachabletest cat-file -e "$blob2" &&
+ test_must_fail git -C reachabletest cat-file -e "$blob3"
+'
+
+test_expect_success 'fetch-pack cannot fetch unreachable blobs' '
+ test_config -C blobserver uploadpack.allowreachablesha1inwant 1 &&
+
+ git init unreachabletest &&
+ test_must_fail git -C unreachabletest fetch-pack ../blobserver "$blob1" "$unreachable"
+'
+
check_prot_path () {
cat >expected <<-EOF &&
Diag: url=$1
diff --git a/upload-pack.c b/upload-pack.c
index 7597ba340..f05cc2b5e 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -471,6 +471,9 @@ static int do_reachable_revlist(struct child_process *cmd,
static const char *argv[] = {
"rev-list", "--stdin", NULL,
};
+ static const char *argv_with_objects[] = {
+ "rev-list", "--objects", "--stdin", NULL,
+ };
struct object *o;
char namebuf[42]; /* ^ + SHA-1 + LF */
int i;
@@ -488,6 +491,18 @@ static int do_reachable_revlist(struct child_process *cmd,
*/
sigchain_push(SIGPIPE, SIG_IGN);
+ /*
+ * If we are testing reachability of a tree or blob, rev-list needs to
+ * operate more granularly.
+ */
+ for (i = 0; i < src->nr; i++) {
+ o = src->objects[i].item;
+ if (o->type == OBJ_TREE || o->type == OBJ_BLOB) {
+ cmd->argv = argv_with_objects;
+ break;
+ }
+ }
+
if (start_command(cmd))
goto error;
--
2.11.0.483.g087da7b7c-goog
^ permalink raw reply related
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-25 1:21 UTC (permalink / raw)
To: David Lang; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, git
In-Reply-To: <nycvar.QRO.7.75.62.1702241656010.6590@qynat-yncgbc>
On Fri, Feb 24, 2017 at 05:00:55PM -0800, David Lang wrote:
> On Fri, 24 Feb 2017, Jeff King wrote:
>
> >
> > So I'd much rather see strong rules like:
> >
> > 1. Once a repo has flag-day switched over to the new hash format[1],
> > new references are _always_ done with the new hash. Even ones that
> > point to pre-flag-day objects!
>
> how do you define when a repo has "switched over" to the new format in a
> distributed environment?
You don't. It's a decision for each local repo, but the rules push
everybody towards upgrading (because you forbid them pulling from or
pushing to people who have upgraded).
So in practice, some centralized distribution point switches, and then
it floods out from there.
> so you have one person working on a project that switches their version of
> git to the new one that uses the new format.
That shouldn't happen when they switch. It should happen when they
decide to move their local clone to the new format. So let's assume they
upgrade _and_ decide to switch.
> But other people they interact with still use older versions of git
Those people get forced to upgrade if they want to continue interacting.
> what happens when you have someone working on two different projects where
> one has switched and the other hasn't?
See above. You only flip the flag on for one of the projects.
> what if they are forks of each other? (LEDE and OpenWRT, or just
> linux-kernel and linux-kernel-stable)
Once one flips, the other one needs to flip to, or can't interact with
them. I know that's harsh, and is likely to create headaches. But in the
long run, I think once everything has converged the resulting system is
less insane.
For that reason I _wouldn't_ recommend projects like the kernel flip the
flag immediately. Ideally we write the code and the new versions
permeate the community. Then somebody (per-project) decides that it's
time for the community to start switching.
> > The flag-day switch would probably be a repo config flag based on
> > repositoryformatversion (so old versions would just punt if they
> > see it). Let's call this flag "newhash" for lack of a better term.
>
> so how do you interact with someone who only expects the old commit instead
> of the commit-v2?
You ask them to upgrade.
-Peff
^ permalink raw reply
* [PATCH] submodule init: warn about falling back to a local path
From: Stefan Beller @ 2017-02-25 1:31 UTC (permalink / raw)
To: j6t; +Cc: git, philipoakley, gitster, sop, Stefan Beller
In-Reply-To: <CAGZ79kbzhHaV4-cVvqwodwXSBstRfH1FrOh=iYMvU6cqYUcUng@mail.gmail.com>
When a submodule is initialized, the config variable 'submodule.<name>.url'
is set depending on the value of the same variable in the .gitmodules
file. When the URL indicates to be relative, then the url is computed
relative to its default remote. The default remote cannot be determined
accurately in all cases, such that it falls back to 'origin'.
The 'origin' remote may not exist, though. In that case we give up looking
for a suitable remote and we'll just assume it to be a local relative path.
This can be confusing to users as there is a lot of guessing involved,
which is not obvious to the user.
So in the corner case of assuming a local autoritative truth, warn the
user to lessen the confusion.
This behavior was introduced in 4d6893200 (submodule add: allow relative
repository path even when no url is set, 2011-06-06), which shared the
code with submodule-init and then ported to C in 3604242f080a (submodule:
port init from shell to C, 2016-04-15).
In case of submodule-add, this behavior makes sense in some use cases[1],
however for submodule-init there does not seem to be an immediate obvious
use case to fall back to a local submodule. However there might be, so
warn instead of die here.
While adding the warning, also clarify the behavior of relative URLs in
the documentation.
[1] e.g. http://stackoverflow.com/questions/8721984/git-ignore-files-for-public-repository-but-not-for-private
"store a secret locally in a submodule, with no intention to publish it"
Reported-by: Shawn Pearce <spearce@spearce.org>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
> Perhaps s/ all submodules/,&/?
done
> I read this as "copying..., resolving relative to the default remote
> (if exists)."
reworded with shorter sentences:
Initialize the submodules recorded in the index (which were
added and committed elsewhere) by setting `submodule.$name.url`
in .git/config. It uses the same setting from .gitmodules as
a template. If the URL is relative, it will be resolved using
the default remote. If there is no default remote, the current
repository will be assumed to be upstream.
...
next paragraph
Documentation/git-submodule.txt | 38 ++++++++++++++++++++++++--------------
builtin/submodule--helper.c | 8 +++-----
2 files changed, 27 insertions(+), 19 deletions(-)
diff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt
index 8acc72ebb8..e05d0cddef 100644
--- a/Documentation/git-submodule.txt
+++ b/Documentation/git-submodule.txt
@@ -73,13 +73,17 @@ configuration entries unless `--name` is used to specify a logical name.
+
<repository> is the URL of the new submodule's origin repository.
This may be either an absolute URL, or (if it begins with ./
-or ../), the location relative to the superproject's origin
+or ../), the location relative to the superproject's default remote
repository (Please note that to specify a repository 'foo.git'
which is located right next to a superproject 'bar.git', you'll
have to use '../foo.git' instead of './foo.git' - as one might expect
when following the rules for relative URLs - because the evaluation
of relative URLs in Git is identical to that of relative directories).
-If the superproject doesn't have an origin configured
++
+The default remote is the remote of the remote tracking branch
+of the current branch. If no such remote tracking branch exists or
+the HEAD is detached, "origin" is assumed to be the default remote.
+If the superproject doesn't have a default remote configured
the superproject is its own authoritative upstream and the current
working directory is used instead.
+
@@ -118,18 +122,24 @@ too (and can also report changes to a submodule's work tree).
init [--] [<path>...]::
Initialize the submodules recorded in the index (which were
- added and committed elsewhere) by copying submodule
- names and urls from .gitmodules to .git/config.
- Optional <path> arguments limit which submodules will be initialized.
- It will also copy the value of `submodule.$name.update` into
- .git/config.
- The key used in .git/config is `submodule.$name.url`.
- This command does not alter existing information in .git/config.
- You can then customize the submodule clone URLs in .git/config
- for your local setup and proceed to `git submodule update`;
- you can also just use `git submodule update --init` without
- the explicit 'init' step if you do not intend to customize
- any submodule locations.
+ added and committed elsewhere) by setting `submodule.$name.url`
+ in .git/config. It uses the same setting from .gitmodules as
+ a template. If the URL is relative, it will be resolved using
+ the default remote. If there is no default remote, the current
+ repository will be assumed to be upstream.
++
+Optional <path> arguments limit which submodules will be initialized.
+If no path is specified, all submodules are initialized.
++
+When present, it will also copy the value of `submodule.$name.update`.
+This command does not alter existing information in .git/config.
+You can then customize the submodule clone URLs in .git/config
+for your local setup and proceed to `git submodule update`;
+you can also just use `git submodule update --init` without
+the explicit 'init' step if you do not intend to customize
+any submodule locations.
++
+See the add subcommand for the defintion of default remote.
deinit [-f|--force] (--all|[--] <path>...)::
Unregister the given submodules, i.e. remove the whole
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 899dc334e3..15a5430c00 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -356,12 +356,10 @@ static void init_submodule(const char *path, const char *prefix, int quiet)
strbuf_addf(&remotesb, "remote.%s.url", remote);
free(remote);
- if (git_config_get_string(remotesb.buf, &remoteurl))
- /*
- * The repository is its own
- * authoritative upstream
- */
+ if (git_config_get_string(remotesb.buf, &remoteurl)) {
+ warning(_("could not lookup configuration '%s'. Assuming this repository is its own authoritative upstream."), remotesb.buf);
remoteurl = xgetcwd();
+ }
relurl = relative_url(remoteurl, url, NULL);
strbuf_release(&remotesb);
free(remoteurl);
--
2.12.0.rc1.16.ge4278d41a0.dirty
^ permalink raw reply related
* Re: SHA1 collisions found
From: David Lang @ 2017-02-25 1:39 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, git
In-Reply-To: <20170225012100.ivfdlwspsqd7bkhf@sigill.intra.peff.net>
On Fri, 24 Feb 2017, Jeff King wrote:
>> what if they are forks of each other? (LEDE and OpenWRT, or just
>> linux-kernel and linux-kernel-stable)
>
> Once one flips, the other one needs to flip to, or can't interact with
> them. I know that's harsh, and is likely to create headaches. But in the
> long run, I think once everything has converged the resulting system is
> less insane.
>
> For that reason I _wouldn't_ recommend projects like the kernel flip the
> flag immediately. Ideally we write the code and the new versions
> permeate the community. Then somebody (per-project) decides that it's
> time for the community to start switching.
can you 'un-flip' the flag? or if you have someone who is a developer flip their
repo (because they heard that sha1 is unsafe, and they want to be safe), they
can't contribute to the kernel. We don't want to have them loose all their work,
so how can they convert their local repo back to somthing that's compatible?
how would submodules work if one module flips and another (or the parent)
doesn't?
OpenWRT/LEDE have their core repo, and they pull from many other (unrelated)
projects into that repo (and then have 'feeds', which is sort-of-like-submodules
to pull in other software that's maintained completely independently)
Microsoft has made lots of money with people being forced to upgrade Word
because one person got a new version and everyone else needed to upgrade to be
compatible. There's a LOT of pain during that process. Is that really the best
way to go?
David Lang
^ permalink raw reply
* Re: SHA1 collisions found
From: Jeff King @ 2017-02-25 1:47 UTC (permalink / raw)
To: David Lang; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, git
In-Reply-To: <nycvar.QRO.7.75.62.1702241733250.6590@qynat-yncgbc>
On Fri, Feb 24, 2017 at 05:39:43PM -0800, David Lang wrote:
> On Fri, 24 Feb 2017, Jeff King wrote:
>
> > > what if they are forks of each other? (LEDE and OpenWRT, or just
> > > linux-kernel and linux-kernel-stable)
> >
> > Once one flips, the other one needs to flip to, or can't interact with
> > them. I know that's harsh, and is likely to create headaches. But in the
> > long run, I think once everything has converged the resulting system is
> > less insane.
> >
> > For that reason I _wouldn't_ recommend projects like the kernel flip the
> > flag immediately. Ideally we write the code and the new versions
> > permeate the community. Then somebody (per-project) decides that it's
> > time for the community to start switching.
>
> can you 'un-flip' the flag? or if you have someone who is a developer flip
> their repo (because they heard that sha1 is unsafe, and they want to be
> safe), they can't contribute to the kernel. We don't want to have them loose
> all their work, so how can they convert their local repo back to somthing
> that's compatible?
I don't think it would be too hard to write an un-flipper (it's
basically just rewriting the newhash bit of history using sha1, and
converting your refs back to point at the sha1s).
> how would submodules work if one module flips and another (or the parent)
> doesn't?
That's a good question. It's possible that another exception should be
carved out for referring to a gitlink via sha1 (we _could_ say "no,
point to a newhash version of the submodule", but I think that creates a
lot of hardship for not much gain).
> OpenWRT/LEDE have their core repo, and they pull from many other (unrelated)
> projects into that repo (and then have 'feeds', which is
> sort-of-like-submodules to pull in other software that's maintained
> completely independently)
I think with submodules this should probably still work. If they are
pulling in with a subtree-ish strategy, then they'd convert the incoming
trees to the newhash format as part of that.
> Microsoft has made lots of money with people being forced to upgrade Word
> because one person got a new version and everyone else needed to upgrade to
> be compatible. There's a LOT of pain during that process. Is that really the
> best way to go?
I think there's going to be a lot of pain regardless. Any attempt to
mitigate that pain and work seamlessly across old and new versions of
git is going cause _ongoing_ pain as people quietly rewrite the same
content back and forth with different hashes. The viral-convergence
strategy is painful once (when you're forced to upgrade), but after that
just works.
If you want to work on a dual-hash strategy, be my guest. I can't
promise I'll be able to find horrific corner cases in it, but I
certainly can't even try to do so until there is a concrete proposal. :)
-Peff
^ permalink raw reply
* Re: SHA1 collisions found
From: David Lang @ 2017-02-25 1:56 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Ian Jackson, Joey Hess, git
In-Reply-To: <20170225014747.f36j2ctlszpebpsy@sigill.intra.peff.net>
On Fri, 24 Feb 2017, Jeff King wrote:
>> OpenWRT/LEDE have their core repo, and they pull from many other (unrelated)
>> projects into that repo (and then have 'feeds', which is
>> sort-of-like-submodules to pull in other software that's maintained
>> completely independently)
>
> I think with submodules this should probably still work. If they are
> pulling in with a subtree-ish strategy, then they'd convert the incoming
> trees to the newhash format as part of that.
as I understand things, they have two categories of things
1. Feeds, which are completely independent, separate maintainers
2. core, which gets pulled into one repo, I don't know if they use submodules in
the process. I know that what downstream users see is a single repo.
I understand and agree with the idea of trying to converge rapidly. I'm just
looking at cases where this may be hard (or where there may be holdouts for
whatever reason)
David Lang
^ permalink raw reply
* Re: SHA1 collisions found
From: Jacob Keller @ 2017-02-25 2:26 UTC (permalink / raw)
To: Jeff King
Cc: David Lang, Junio C Hamano, Ian Jackson, Joey Hess,
Git mailing list
In-Reply-To: <20170225012100.ivfdlwspsqd7bkhf@sigill.intra.peff.net>
On Fri, Feb 24, 2017 at 5:21 PM, Jeff King <peff@peff.net> wrote:
> On Fri, Feb 24, 2017 at 05:00:55PM -0800, David Lang wrote:
>
>> On Fri, 24 Feb 2017, Jeff King wrote:
>>
>> >
>> > So I'd much rather see strong rules like:
>> >
>> > 1. Once a repo has flag-day switched over to the new hash format[1],
>> > new references are _always_ done with the new hash. Even ones that
>> > point to pre-flag-day objects!
>>
>> how do you define when a repo has "switched over" to the new format in a
>> distributed environment?
>
> You don't. It's a decision for each local repo, but the rules push
> everybody towards upgrading (because you forbid them pulling from or
> pushing to people who have upgraded).
>
> So in practice, some centralized distribution point switches, and then
> it floods out from there.
This seems like the most reasonable strategy so far. I think that
trying to allow long term co-existence is a huge pain that discourages
switching, when we actually want to encourage everyone to switch
someone has switched.
I don't think it's sane to try and allow simultaneous use of both
hashes, since that creates a lot of headaches and discourages
transition somewhat.
Thanks,
Jake
^ permalink raw reply
* Re: SHA1 collisions found
From: Jacob Keller @ 2017-02-25 2:28 UTC (permalink / raw)
To: David Lang
Cc: Jeff King, Junio C Hamano, Ian Jackson, Joey Hess,
Git mailing list
In-Reply-To: <nycvar.QRO.7.75.62.1702241733250.6590@qynat-yncgbc>
On Fri, Feb 24, 2017 at 5:39 PM, David Lang <david@lang.hm> wrote:
> On Fri, 24 Feb 2017, Jeff King wrote:
>
>>> what if they are forks of each other? (LEDE and OpenWRT, or just
>>> linux-kernel and linux-kernel-stable)
>>
>>
>> Once one flips, the other one needs to flip to, or can't interact with
>> them. I know that's harsh, and is likely to create headaches. But in the
>> long run, I think once everything has converged the resulting system is
>> less insane.
>>
>> For that reason I _wouldn't_ recommend projects like the kernel flip the
>> flag immediately. Ideally we write the code and the new versions
>> permeate the community. Then somebody (per-project) decides that it's
>> time for the community to start switching.
>
>
> can you 'un-flip' the flag? or if you have someone who is a developer flip
> their repo (because they heard that sha1 is unsafe, and they want to be
> safe), they can't contribute to the kernel. We don't want to have them loose
> all their work, so how can they convert their local repo back to somthing
> that's compatible?
I'd think one of the first things we want is a way to flip *and*
unflip by re-writing history ala git-filter-branch style. (So if you
wanted, you could also flip all your old history).
One unrelated thought I had. When an old client sees the new stuff, it
will probably fail in a lot of weird ways. I wonder what we can do so
that if we in the future have to switch to an even newer hash, how can
we make it so that the old versions give a more clean error
experience? Ideally so that it lessens the pain of transition somewhat
in the future if/when it has to happen again?
Thanks,
Jake
^ 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