* Re: Bug: stash staged file move loses original file deletion
From: Jeff King @ 2016-12-06 15:25 UTC (permalink / raw)
To: Matthew Patey; +Cc: git
In-Reply-To: <CAFQpxx+PJ3FSoH9DFWyEw+ZLagji9Qou+aY9EB8A+=t+QX0o2A@mail.gmail.com>
On Tue, Dec 06, 2016 at 02:45:05PM +0000, Matthew Patey wrote:
> Thanks for the reply! I agree that it is weird that applying a stash with a
> move only puts the addition back in the index, and thanks for the tip on
> "index" to properly apply that. For this case I was focusing on the
> behavior of the second stash/apply, with the first stash/apply as an
> example of how to get into a weird state that triggers the odd behavior of
> the second.
Oh, heh, I totally missed that.
I agree that the second stash not saving the deletion seems like a bug.
Oddly, just stashing a deletion, like:
rm a
git stash
git stash apply
does store it. So it's not like we can't represent the deletion. Somehow
the presence of "b" in the index matters.
It looks like the culprit may be this part of create_stash():
git diff --name-only -z HEAD -- >"$TMP-stagenames"
That is using the "git diff" porcelain, which will respect renames, and
the --name-only bit mentions only "b".
If you run:
git -c diff.renames=false stash
then it works.
-Peff
^ permalink raw reply
* [PATCH] stash: disable renames when calling git-diff
From: Jeff King @ 2016-12-06 15:41 UTC (permalink / raw)
To: Matthew Patey; +Cc: git
In-Reply-To: <20161206152530.snccf7buiosst3e4@sigill.intra.peff.net>
On Tue, Dec 06, 2016 at 10:25:30AM -0500, Jeff King wrote:
> I agree that the second stash not saving the deletion seems like a bug.
> Oddly, just stashing a deletion, like:
>
> rm a
> git stash
> git stash apply
>
> does store it. So it's not like we can't represent the deletion. Somehow
> the presence of "b" in the index matters.
>
> It looks like the culprit may be this part of create_stash():
>
> git diff --name-only -z HEAD -- >"$TMP-stagenames"
>
> That is using the "git diff" porcelain, which will respect renames, and
> the --name-only bit mentions only "b".
>
> If you run:
>
> git -c diff.renames=false stash
>
> then it works.
And here's a patch to fix it.
-- >8 --
Subject: [PATCH] stash: disable renames when calling git-diff
When creating a stash, we need to look at the diff between
the working tree and HEAD. There's no plumbing command that
covers this case, so we do so by calling the git-diff
porcelain. This means we also inherit the usual list of
porcelain options, which can impact the output in confusing
ways.
There's at least one known problem: --name-only will not
mention the source side of a rename, meaning that we will
fail to stash a deletion in such a case.
The correct solution is to move to an appropriate plumbing
command. But since there isn't one available, in the short
term we can plug this particular problem by manually asking
git-diff not to respect renames.
Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
There may be other similar problems lurking depending on the various
config options you have set, but I don't think so. Most of the options
only affect --patch output.
I do find it interesting that --name-only does not mention the source
side of a rename. The longer forms like --name-status mention both
sides. Certainly --name-status does not have any way of saying "this is
the source side of a rename", but I'd think it would simply mention both
sides. Anyway, even if that were changed (which would fix this bug), I
think adding --no-renames is still a good thing to be doing here.
git-stash.sh | 2 +-
t/t3903-stash.sh | 9 +++++++++
2 files changed, 10 insertions(+), 1 deletion(-)
diff --git a/git-stash.sh b/git-stash.sh
index 4546abaae..40ab2710e 100755
--- a/git-stash.sh
+++ b/git-stash.sh
@@ -115,7 +115,7 @@ create_stash () {
git read-tree --index-output="$TMPindex" -m $i_tree &&
GIT_INDEX_FILE="$TMPindex" &&
export GIT_INDEX_FILE &&
- git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
+ git diff --name-only -z --no-renames HEAD -- >"$TMP-stagenames" &&
git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
git write-tree &&
rm -f "$TMPindex"
diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
index e1a6ccc00..2de3e18ce 100755
--- a/t/t3903-stash.sh
+++ b/t/t3903-stash.sh
@@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
test_cmp expect actual
'
+test_expect_success 'stash is not confused by partial renames' '
+ mv file renamed &&
+ git add renamed &&
+ git stash &&
+ git stash apply &&
+ test_path_is_file renamed &&
+ test_path_is_missing file
+'
+
test_done
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* Re: [PATCH] git-p4: add p4 shelf support
From: Vinicius Kursancew @ 2016-12-06 16:03 UTC (permalink / raw)
To: Luke Diamand; +Cc: Nuno Subtil, Git Users, Junio C Hamano, Lars Schneider
In-Reply-To: <CAE5ih79S+SFt-fsQ_2c4eXMankoXvoSE3zhxw39Y4XeQqQ9nMg@mail.gmail.com>
It seems if you do shelve and then later submit the p4 workspace is
left dirty (just like --prepare-p4-only would've done).
Vinicius Alexandre Kursancew <viniciusalexandre@gmail.com>
On Tue, Dec 6, 2016 at 8:36 AM, Luke Diamand <luke@diamand.org> wrote:
> On 6 December 2016 at 02:02, Nuno Subtil <subtil@gmail.com> wrote:
>> Extends the submit command to support shelving a commit instead of
>> submitting it to p4 (similar to --prepare-p4-only).
>
> Is this just the same as these two changes?
>
> http://www.spinics.net/lists/git/msg290755.html
> http://www.spinics.net/lists/git/msg291103.html
>
> Thanks,
> Luke
>
>>
>> Signed-off-by: Nuno Subtil <subtil@gmail.com>
>> ---
>> git-p4.py | 36 ++++++++++++++++++++++++++++++------
>> 1 file changed, 30 insertions(+), 6 deletions(-)
>>
>> diff --git a/git-p4.py b/git-p4.py
>> index fd5ca52..3c4be22 100755
>> --- a/git-p4.py
>> +++ b/git-p4.py
>> @@ -1286,6 +1286,8 @@ def __init__(self):
>> optparse.make_option("--export-labels", dest="exportLabels", action="store_true"),
>> optparse.make_option("--dry-run", "-n", dest="dry_run", action="store_true"),
>> optparse.make_option("--prepare-p4-only", dest="prepare_p4_only", action="store_true"),
>> + optparse.make_option("--shelve-only", dest="shelve_only", action="store_true", help="Create P4 shelf for first change that would be submitted (using a new CL)"),
>> + optparse.make_option("--shelve-cl", dest="shelve_cl", help="Replace shelf under existing CL number (previously shelved files will be deleted)"),
>> optparse.make_option("--conflict", dest="conflict_behavior",
>> choices=self.conflict_behavior_choices),
>> optparse.make_option("--branch", dest="branch"),
>> @@ -1297,6 +1299,8 @@ def __init__(self):
>> self.preserveUser = gitConfigBool("git-p4.preserveUser")
>> self.dry_run = False
>> self.prepare_p4_only = False
>> + self.shelve_only = False
>> + self.shelve_cl = None
>> self.conflict_behavior = None
>> self.isWindows = (platform.system() == "Windows")
>> self.exportLabels = False
>> @@ -1496,6 +1500,12 @@ def prepareSubmitTemplate(self):
>> else:
>> inFilesSection = False
>> else:
>> + if self.shelve_only and self.shelve_cl:
>> + if line.startswith("Change:"):
>> + line = "Change: %s\n" % self.shelve_cl
>> + if line.startswith("Status:"):
>> + line = "Status: pending\n"
>> +
>> if line.startswith("Files:"):
>> inFilesSection = True
>>
>> @@ -1785,7 +1795,11 @@ def applyCommit(self, id):
>> if self.isWindows:
>> message = message.replace("\r\n", "\n")
>> submitTemplate = message[:message.index(separatorLine)]
>> - p4_write_pipe(['submit', '-i'], submitTemplate)
>> +
>> + if self.shelve_only:
>> + p4_write_pipe(['shelve', '-i', '-r'], submitTemplate)
>> + else:
>> + p4_write_pipe(['submit', '-i'], submitTemplate)
>>
>> if self.preserveUser:
>> if p4User:
>> @@ -1799,12 +1813,17 @@ def applyCommit(self, id):
>> # new file. This leaves it writable, which confuses p4.
>> for f in pureRenameCopy:
>> p4_sync(f, "-f")
>> - submitted = True
>> +
>> + if not self.shelve_only:
>> + submitted = True
>>
>> finally:
>> # skip this patch
>> if not submitted:
>> - print "Submission cancelled, undoing p4 changes."
>> + if not self.shelve_only:
>> + print "Submission cancelled, undoing p4 changes."
>> + else:
>> + print "Change shelved, undoing p4 changes."
>> for f in editedFiles:
>> p4_revert(f)
>> for f in filesToAdd:
>> @@ -2034,9 +2053,13 @@ def run(self, args):
>> if ok:
>> applied.append(commit)
>> else:
>> - if self.prepare_p4_only and i < last:
>> - print "Processing only the first commit due to option" \
>> - " --prepare-p4-only"
>> + if (self.prepare_p4_only or self.shelve_only) and i < last:
>> + if self.prepare_p4_only:
>> + print "Processing only the first commit due to option" \
>> + " --prepare-p4-only"
>> + else:
>> + print "Processing only the first commit due to option" \
>> + " --shelve-only"
>> break
>> if i < last:
>> quit = False
>> @@ -3638,6 +3661,7 @@ def printUsage(commands):
>> "debug" : P4Debug,
>> "submit" : P4Submit,
>> "commit" : P4Submit,
>> + "shelve" : P4Submit,
>> "sync" : P4Sync,
>> "rebase" : P4Rebase,
>> "clone" : P4Clone,
>>
>> --
>> https://github.com/git/git/pull/309
^ permalink raw reply
* [PATCH v4] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 16:53 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jack Bates
In-Reply-To: <20161206010134.21856-1-jack@nottheoilrig.com>
There are two different places where the --no-abbrev option is parsed,
and two different places where SHA-1s are abbreviated. We normally parse
--no-abbrev with setup_revisions(), but in the no-index case, "git diff"
calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
--no-abbrev until now. (It did handle --abbrev, however.) We normally
abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
handle sha1 abbreviations outside of repository, 2016-10-20) recently
introduced a special case when you run "git diff" outside of a
repository.
setup_revisions() does also call diff_opt_parse(), but not for --abbrev
or --no-abbrev, which it handles itself. setup_revisions() sets
rev_info->abbrev, and later copies that to diff_options->abbrev. It
handles --no-abbrev by setting abbrev to zero. (This change doesn't
touch that.)
Setting abbrev to zero was broken in the outside-of-a-repository special
case, which until now resulted in a truly zero-length SHA-1, rather than
taking zero to mean do not abbreviate. The only way to trigger this bug,
however, was by running "git diff --raw" without either the --abbrev or
--no-abbrev options, because 1) without --raw it doesn't respect abbrev
(which is bizarre, but has been that way forever), 2) we silently clamp
--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
now.
The outside-of-a-repository case is one of three no-index cases. The
other two are when one of the files you're comparing is outside of the
repository you're in, and the --no-index option.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
diff.c | 6 +++++-
t/t4013-diff-various.sh | 7 +++++++
t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
t/t4013/diff.diff_--raw_initial | 6 ++++++
8 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
create mode 100644 t/t4013/diff.diff_--raw_initial
diff --git a/diff.c b/diff.c
index ec87283..84dba60 100644
--- a/diff.c
+++ b/diff.c
@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
- hex[abbrev] = '\0';
+ if (abbrev)
+ hex[abbrev] = '\0';
return hex;
}
}
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
options->file = stdout;
+ options->abbrev = DEFAULT_ABBREV;
options->line_termination = '\n';
options->break_opt = -1;
options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
offending, optarg);
return argcount;
}
+ else if (!strcmp(arg, "--no-abbrev"))
+ options->abbrev = 0;
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d7b71a0 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
diff --dirstat master~1 master~2
diff --dirstat initial rearrange
diff --dirstat-by-file initial rearrange
+# --abbrev and --no-abbrev outside of repository
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
EOF
test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M dir/sub
+:100644 100644 01e7... 10a8... M file0
+:000000 100644 0000... b1e6... A file1
+:100644 000000 01e7... 0000... D file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M dir/sub
+:100644 100644 01e79c3... 10a8a9f... M file0
+:000000 100644 0000000... b1e6722... A file1
+:100644 000000 01e79c3... 0000000... D file2
+$
--
2.10.2
^ permalink raw reply related
* [PATCH v4] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 16:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King, Jack Bates
In-Reply-To: <20161206010134.21856-1-jack@nottheoilrig.com>
There are two different places where the --no-abbrev option is parsed,
and two different places where SHA-1s are abbreviated. We normally parse
--no-abbrev with setup_revisions(), but in the no-index case, "git diff"
calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
--no-abbrev until now. (It did handle --abbrev, however.) We normally
abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
handle sha1 abbreviations outside of repository, 2016-10-20) recently
introduced a special case when you run "git diff" outside of a
repository.
setup_revisions() does also call diff_opt_parse(), but not for --abbrev
or --no-abbrev, which it handles itself. setup_revisions() sets
rev_info->abbrev, and later copies that to diff_options->abbrev. It
handles --no-abbrev by setting abbrev to zero. (This change doesn't
touch that.)
Setting abbrev to zero was broken in the outside-of-a-repository special
case, which until now resulted in a truly zero-length SHA-1, rather than
taking zero to mean do not abbreviate. The only way to trigger this bug,
however, was by running "git diff --raw" without either the --abbrev or
--no-abbrev options, because 1) without --raw it doesn't respect abbrev
(which is bizarre, but has been that way forever), 2) we silently clamp
--abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
now.
The outside-of-a-repository case is one of three no-index cases. The
other two are when one of the files you're comparing is outside of the
repository you're in, and the --no-index option.
Signed-off-by: Jack Bates <jack@nottheoilrig.com>
---
diff.c | 6 +++++-
t/t4013-diff-various.sh | 7 +++++++
t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
t/t4013/diff.diff_--raw_initial | 6 ++++++
8 files changed, 39 insertions(+), 1 deletion(-)
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
create mode 100644 t/t4013/diff.diff_--raw_initial
diff --git a/diff.c b/diff.c
index ec87283..84dba60 100644
--- a/diff.c
+++ b/diff.c
@@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
abbrev = FALLBACK_DEFAULT_ABBREV;
if (abbrev > GIT_SHA1_HEXSZ)
die("BUG: oid abbreviation out of range: %d", abbrev);
- hex[abbrev] = '\0';
+ if (abbrev)
+ hex[abbrev] = '\0';
return hex;
}
}
@@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
options->file = stdout;
+ options->abbrev = DEFAULT_ABBREV;
options->line_termination = '\n';
options->break_opt = -1;
options->rename_limit = -1;
@@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
offending, optarg);
return argcount;
}
+ else if (!strcmp(arg, "--no-abbrev"))
+ options->abbrev = 0;
else if (!strcmp(arg, "--abbrev"))
options->abbrev = DEFAULT_ABBREV;
else if (skip_prefix(arg, "--abbrev=", &arg)) {
diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
index 566817e..d09acfe 100755
--- a/t/t4013-diff-various.sh
+++ b/t/t4013-diff-various.sh
@@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
diff --dirstat master~1 master~2
diff --dirstat initial rearrange
diff --dirstat-by-file initial rearrange
+# No-index --abbrev and --no-abbrev
+diff --raw initial
+diff --raw --abbrev=4 initial
+diff --raw --no-abbrev initial
+diff --no-index --raw dir2 dir
+diff --no-index --raw --abbrev=4 dir2 dir
+diff --no-index --raw --no-abbrev dir2 dir
EOF
test_expect_success 'log -S requires an argument' '
diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
new file mode 100644
index 0000000..a71b38a
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --abbrev=4 dir2 dir
+:000000 100644 0000... 0000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
new file mode 100644
index 0000000..e0f0097
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw --no-abbrev dir2 dir
+:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
+$
diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
new file mode 100644
index 0000000..3cb4ee7
--- /dev/null
+++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
@@ -0,0 +1,3 @@
+$ git diff --no-index --raw dir2 dir
+:000000 100644 0000000... 0000000... A dir/sub
+$
diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
new file mode 100644
index 0000000..c3641db
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --abbrev=4 initial
+:100644 100644 35d2... 9929... M dir/sub
+:100644 100644 01e7... 10a8... M file0
+:000000 100644 0000... b1e6... A file1
+:100644 000000 01e7... 0000... D file2
+$
diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
new file mode 100644
index 0000000..c87a125
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
@@ -0,0 +1,6 @@
+$ git diff --raw --no-abbrev initial
+:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
+:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
+:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
+:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
+$
diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
new file mode 100644
index 0000000..a3e9780
--- /dev/null
+++ b/t/t4013/diff.diff_--raw_initial
@@ -0,0 +1,6 @@
+$ git diff --raw initial
+:100644 100644 35d242b... 992913c... M dir/sub
+:100644 100644 01e79c3... 10a8a9f... M file0
+:000000 100644 0000000... b1e6722... A file1
+:100644 000000 01e79c3... 0000000... D file2
+$
--
2.10.2
^ permalink raw reply related
* Re: [PATCH v4] diff: handle --no-abbrev in no-index case
From: Jack Bates @ 2016-12-06 17:00 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano, Jeff King
In-Reply-To: <20161206165614.22921-1-jack@nottheoilrig.com>
On 06/12/16 09:56 AM, Jack Bates wrote:
> There are two different places where the --no-abbrev option is parsed,
> and two different places where SHA-1s are abbreviated. We normally parse
> --no-abbrev with setup_revisions(), but in the no-index case, "git diff"
> calls diff_opt_parse() directly, and diff_opt_parse() didn't handle
> --no-abbrev until now. (It did handle --abbrev, however.) We normally
> abbreviate SHA-1s with find_unique_abbrev(), but commit 4f03666 ("diff:
> handle sha1 abbreviations outside of repository, 2016-10-20) recently
> introduced a special case when you run "git diff" outside of a
> repository.
>
> setup_revisions() does also call diff_opt_parse(), but not for --abbrev
> or --no-abbrev, which it handles itself. setup_revisions() sets
> rev_info->abbrev, and later copies that to diff_options->abbrev. It
> handles --no-abbrev by setting abbrev to zero. (This change doesn't
> touch that.)
>
> Setting abbrev to zero was broken in the outside-of-a-repository special
> case, which until now resulted in a truly zero-length SHA-1, rather than
> taking zero to mean do not abbreviate. The only way to trigger this bug,
> however, was by running "git diff --raw" without either the --abbrev or
> --no-abbrev options, because 1) without --raw it doesn't respect abbrev
> (which is bizarre, but has been that way forever), 2) we silently clamp
> --abbrev=0 to MINIMUM_ABBREV, and 3) --no-abbrev wasn't handled until
> now.
>
> The outside-of-a-repository case is one of three no-index cases. The
> other two are when one of the files you're comparing is outside of the
> repository you're in, and the --no-index option.
>
> Signed-off-by: Jack Bates <jack@nottheoilrig.com>
> ---
> diff.c | 6 +++++-
> t/t4013-diff-various.sh | 7 +++++++
> t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir | 3 +++
> t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir | 3 +++
> t/t4013/diff.diff_--no-index_--raw_dir2_dir | 3 +++
> t/t4013/diff.diff_--raw_--abbrev=4_initial | 6 ++++++
> t/t4013/diff.diff_--raw_--no-abbrev_initial | 6 ++++++
> t/t4013/diff.diff_--raw_initial | 6 ++++++
> 8 files changed, 39 insertions(+), 1 deletion(-)
> create mode 100644 t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> create mode 100644 t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> create mode 100644 t/t4013/diff.diff_--no-index_--raw_dir2_dir
> create mode 100644 t/t4013/diff.diff_--raw_--abbrev=4_initial
> create mode 100644 t/t4013/diff.diff_--raw_--no-abbrev_initial
> create mode 100644 t/t4013/diff.diff_--raw_initial
>
> diff --git a/diff.c b/diff.c
> index ec87283..84dba60 100644
> --- a/diff.c
> +++ b/diff.c
> @@ -3106,7 +3106,8 @@ static const char *diff_abbrev_oid(const struct object_id *oid, int abbrev)
> abbrev = FALLBACK_DEFAULT_ABBREV;
> if (abbrev > GIT_SHA1_HEXSZ)
> die("BUG: oid abbreviation out of range: %d", abbrev);
> - hex[abbrev] = '\0';
> + if (abbrev)
> + hex[abbrev] = '\0';
> return hex;
> }
> }
> @@ -3364,6 +3365,7 @@ void diff_setup(struct diff_options *options)
>
> options->file = stdout;
>
> + options->abbrev = DEFAULT_ABBREV;
> options->line_termination = '\n';
> options->break_opt = -1;
> options->rename_limit = -1;
> @@ -4024,6 +4026,8 @@ int diff_opt_parse(struct diff_options *options,
> offending, optarg);
> return argcount;
> }
> + else if (!strcmp(arg, "--no-abbrev"))
> + options->abbrev = 0;
> else if (!strcmp(arg, "--abbrev"))
> options->abbrev = DEFAULT_ABBREV;
> else if (skip_prefix(arg, "--abbrev=", &arg)) {
> diff --git a/t/t4013-diff-various.sh b/t/t4013-diff-various.sh
> index 566817e..d09acfe 100755
> --- a/t/t4013-diff-various.sh
> +++ b/t/t4013-diff-various.sh
> @@ -311,6 +311,13 @@ diff --line-prefix=abc master master^ side
> diff --dirstat master~1 master~2
> diff --dirstat initial rearrange
> diff --dirstat-by-file initial rearrange
> +# No-index --abbrev and --no-abbrev
I updated this comment to be consistent with the no-index vs.
outside-of-a-repository distinction in the commit message.
> +diff --raw initial
> +diff --raw --abbrev=4 initial
> +diff --raw --no-abbrev initial
> +diff --no-index --raw dir2 dir
> +diff --no-index --raw --abbrev=4 dir2 dir
> +diff --no-index --raw --no-abbrev dir2 dir
> EOF
>
> test_expect_success 'log -S requires an argument' '
> diff --git a/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> new file mode 100644
> index 0000000..a71b38a
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_--abbrev=4_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw --abbrev=4 dir2 dir
> +:000000 100644 0000... 0000... A dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> new file mode 100644
> index 0000000..e0f0097
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_--no-abbrev_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw --no-abbrev dir2 dir
> +:000000 100644 0000000000000000000000000000000000000000 0000000000000000000000000000000000000000 A dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--no-index_--raw_dir2_dir b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
> new file mode 100644
> index 0000000..3cb4ee7
> --- /dev/null
> +++ b/t/t4013/diff.diff_--no-index_--raw_dir2_dir
> @@ -0,0 +1,3 @@
> +$ git diff --no-index --raw dir2 dir
> +:000000 100644 0000000... 0000000... A dir/sub
> +$
> diff --git a/t/t4013/diff.diff_--raw_--abbrev=4_initial b/t/t4013/diff.diff_--raw_--abbrev=4_initial
> new file mode 100644
> index 0000000..c3641db
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_--abbrev=4_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw --abbrev=4 initial
> +:100644 100644 35d2... 9929... M dir/sub
> +:100644 100644 01e7... 10a8... M file0
> +:000000 100644 0000... b1e6... A file1
> +:100644 000000 01e7... 0000... D file2
> +$
> diff --git a/t/t4013/diff.diff_--raw_--no-abbrev_initial b/t/t4013/diff.diff_--raw_--no-abbrev_initial
> new file mode 100644
> index 0000000..c87a125
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_--no-abbrev_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw --no-abbrev initial
> +:100644 100644 35d242ba79ae89ac695e26b3d4c27a8e6f028f9e 992913c5aa0a5476d10c49ed0f21fc0c6d1aedf3 M dir/sub
> +:100644 100644 01e79c32a8c99c557f0757da7cb6d65b3414466d 10a8a9f3657f91a156b9f0184ed79a20adef9f7f M file0
> +:000000 100644 0000000000000000000000000000000000000000 b1e67221afe8461efd244b487afca22d46b95eb8 A file1
> +:100644 000000 01e79c32a8c99c557f0757da7cb6d65b3414466d 0000000000000000000000000000000000000000 D file2
> +$
> diff --git a/t/t4013/diff.diff_--raw_initial b/t/t4013/diff.diff_--raw_initial
> new file mode 100644
> index 0000000..a3e9780
> --- /dev/null
> +++ b/t/t4013/diff.diff_--raw_initial
> @@ -0,0 +1,6 @@
> +$ git diff --raw initial
> +:100644 100644 35d242b... 992913c... M dir/sub
> +:100644 100644 01e79c3... 10a8a9f... M file0
> +:000000 100644 0000000... b1e6722... A file1
> +:100644 000000 01e79c3... 0000000... D file2
> +$
>
^ permalink raw reply
* Re: Re: Feature request: Set git svn options in .git/config file
From: Juergen Kosel @ 2016-12-06 17:24 UTC (permalink / raw)
To: git
In-Reply-To: <20161205225424.GA29771@starla>
-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1
Hello,
Am 05.12.2016 um 23:54 schrieb Eric Wong:
> So, can you confirm that svn.addAuthorFrom and svn.useLogAuthor
> config keys work and can be documented?
yes, I can confirm, that adding this configuration keys works with git
2.1.4 work.
I have added the config keys as follows:
git config --add --bool svn.useLogAuthor true
git config --add --bool svn.addAuthorFrom true
>
> Even better would be for you to provide a patch to the
> documentation :) Otherwise, I can write one up sometime this week.
My English is not that well. So I prefer, if you update the
documentation :-)
Greetings
Juergen
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v2
Comment: Using GnuPG with Icedove - http://www.enigmail.net/
iD8DBQFYRvRhYZbcPghIM9IRAjtHAJ0QaqbUxcgoPXmidIg9WALXmg1JAQCfTHFj
wgPLKXK5Ny0bP/K9vpE9KzY=
=A+Yy
-----END PGP SIGNATURE-----
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Junio C Hamano @ 2016-12-06 17:53 UTC (permalink / raw)
To: Jeff King; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206135113.i7nlr45vg7uzgfcn@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> I don't know if that makes things any easier. I feel funny saying "no,
> no, mine preempts yours because it is more maint-worthy", but I think
> that order does make sense.
>
> I think it would be OK to put Brandon's on maint, too, though. It is a
> refactor of an existing security feature to make it more featureful, but
> the way it is implemented could not cause security regressions unless
> you use the new feature (IOW, we still respect the whitelist environment
> exactly as before).
I think I merged yours and then Brandon's on jch/pu branches in that
order, and the conflict resolution should look OK.
I however forked yours on v2.11.0-rc1, which would need to be
rebased to one of the earlier maintenance tracks, before we can
merge it to 'next'.
^ permalink raw reply
* Re: "git add -p ." raises an unexpected "warning: empty strings as pathspecs will be made invalid in upcoming releases. please use . instead if you meant to match all paths"
From: Brandon Williams @ 2016-12-06 18:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git, Emily Xie
In-Reply-To: <xmqqtwaobni5.fsf@gitster.mtv.corp.google.com>
On 11/30, Junio C Hamano wrote:
> Junio C Hamano <gitster@pobox.com> forgot to Cc: the author of the
> most relevant change to the issue, d426430e6e ("pathspec: warn on
> empty strings as pathspec", 2016-06-22).
>
> > Kevin Daudt <me@ikke.info> writes:
> >
> >> On Wed, Nov 30, 2016 at 12:31:49PM -0800, Peter Urda wrote:
> >>> After upgrading to version 2.11.0 I am getting a warning about empty
> >>> strings as pathspecs while using 'patch'
> >>>
> >>> - Ran 'git add -p .' from the root of my git repository.
> >>>
> >>> - I was able to normally stage my changes, but was presented with a
> >>> "warning: empty strings as pathspecs will be made invalid in upcoming
> >>> releases. please use . instead if you meant to match all paths"
> >>> message.
> >>>
> >>> - I expected no warning message since I included a "." with my original command.
> >>>
> >>> I believe that I should not be seeing this warning message as I
> >>> included the requested "." pathspec.
> >
> > Yes, this seems to be caused by pathspec.c::prefix_pathspec()
> > overwriting the original pathspec "." into "". The callchain
> > looks like this:
> >
> > builtin/add.c::interactive_add()
> > -> parse_pathspec()
> > passes argv[] that has "." to the caller,
> > receives pathspec whose pathspec->items[].original
> > is supposed to point at the unmolested original,
> > but prefix_pathspec() munges "." into ""
> > -> run_add_interactive()
> > which runs "git add--interactive" with
> > pathspec->items[].original as pathspecs
> >
> >
> > Perhaps this would work it around, but there should be a better way
> > to fix it (like, making sure that what we call "original" indeed
> > stays "original").
> >
> > builtin/add.c | 13 +++++++++++--
> > 1 file changed, 11 insertions(+), 2 deletions(-)
> >
> > diff --git a/builtin/add.c b/builtin/add.c
> > index e8fb80b36e..137097192d 100644
> > --- a/builtin/add.c
> > +++ b/builtin/add.c
> > @@ -167,9 +167,18 @@ int run_add_interactive(const char *revision, const char *patch_mode,
> > if (revision)
> > argv_array_push(&argv, revision);
> > argv_array_push(&argv, "--");
> > - for (i = 0; i < pathspec->nr; i++)
> > + for (i = 0; i < pathspec->nr; i++) {
> > /* pass original pathspec, to be re-parsed */
> > + if (!*pathspec->items[i].original) {
> > + /*
> > + * work around a misfeature in parse_pathspecs()
> > + * that munges "." into "".
> > + */
> > + argv_array_push(&argv, ".");
> > + continue;
> > + }
> > argv_array_push(&argv, pathspec->items[i].original);
> > + }
> >
> > status = run_command_v_opt(argv.argv, RUN_GIT_CMD);
> > argv_array_clear(&argv);
> > @@ -180,7 +189,7 @@ int interactive_add(int argc, const char **argv, const char *prefix, int patch)
> > {
> > struct pathspec pathspec;
> >
> > - parse_pathspec(&pathspec, 0,
> > + parse_pathspec(&pathspec, 0,
> > PATHSPEC_PREFER_FULL |
> > PATHSPEC_SYMLINK_LEADING_PATH |
> > PATHSPEC_PREFIX_ORIGIN,
I've been doing a bit of work trying to clean up the pathspec
initialization code and I believe this can be fixed without
having to add in this work around. The code which does the munging is
always trying to prefix the pathspec regardless if there is a prefix or
not. If instead its changed to only try and prefix the original if
there is indeed a prefix, then it should fix the munging.
I'll try to get the series I'm working on out in the next day.
--
Brandon Williams
^ permalink raw reply
* Re: [PATCH v7 4/4] transport: add from_user parameter to is_transport_allowed
From: Jeff King @ 2016-12-06 18:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <xmqq60mx2bbi.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 06, 2016 at 09:53:53AM -0800, Junio C Hamano wrote:
> Jeff King <peff@peff.net> writes:
>
> > I don't know if that makes things any easier. I feel funny saying "no,
> > no, mine preempts yours because it is more maint-worthy", but I think
> > that order does make sense.
> >
> > I think it would be OK to put Brandon's on maint, too, though. It is a
> > refactor of an existing security feature to make it more featureful, but
> > the way it is implemented could not cause security regressions unless
> > you use the new feature (IOW, we still respect the whitelist environment
> > exactly as before).
>
> I think I merged yours and then Brandon's on jch/pu branches in that
> order, and the conflict resolution should look OK.
>
> I however forked yours on v2.11.0-rc1, which would need to be
> rebased to one of the earlier maintenance tracks, before we can
> merge it to 'next'.
Yeah, I built it on top of master.
It does depend on some of the http-walker changes Eric made a few months
ago. In particular, 17966c0a6 (http: avoid disconnecting on 404s for
loose objects, 2016-07-11) added some checks against the HTTP status
code, and my series modifies the checks (mostly so that ">= 400" becomes
">= 300").
Rebasing on maint-2.9 means omitting those changes. That preserves the
security properties, but means that the error handling is worse when we
see an illegal redirect. That may be OK, though.
Since the resolution is to omit the changes entirely from my series,
merging up to v2.11 wouldn't produce any conflicts. We'd need to have a
separate set of patches adding those changes back in.
-Peff
^ permalink raw reply
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Junio C Hamano @ 2016-12-06 18:17 UTC (permalink / raw)
To: Jeff King; +Cc: Ramsay Jones, GIT Mailing-list
In-Reply-To: <20161206140259.lly76xkvsj7su3om@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Mon, Dec 05, 2016 at 10:01:19AM -0800, Junio C Hamano wrote:
>
>> > That said, I think the right patch may be to just drop --abbrev
>> > entirely.
>> > ...
>> > I think at that point it was a noop, as 7 should have been the default.
>> > And now we probably ought to drop it, so that we can use the
>> > auto-scaling default.
>>
>> Yeah, I agree.
>>
>> It does mean that snapshot binaries built out of the same commit in
>> the same repository before and after a repack have higher chances of
>> getting named differently, which may surprise people, but that
>> already is possible with a fixed length if the repacking involves
>> pruning (albeit with lower probabilities), and I do not think it is
>> a problem.
>
> I think that the number is already unstable, even with --abbrev, as it
> just specifies a minimum. So any operation that creates objects has a
> possibility of increasing the length. Or more likely, two people
> describing the same version may end up with different strings because
> they have different objects in their repositories (e.g., I used to
> carry's trast's git-notes archive of the mailing list which added quite
> a few objects).
>
> I agree that having it change over the course of a repack is slightly
> _more_ surprising than those cases, but ultimately the value just isn't
> stable.
Yup, that is what I meant to say with "that is already possible" and
we are on the same page. As all three of us seem to be happy with
just dropping --abbrev and letting describe do its default thing (as
configured by whoever is doing the build), let's do so.
-- >8 --
From: Ramsay Jones <ramsay@ramsayjones.plus.com>
Date: Sun, 4 Dec 2016 20:45:59 +0000
Subject: [PATCH] GIT-VERSION-GEN: do not force abbreviation length used by 'describe'
The default version name for a Git binary is computed by running
"git describe" on the commit the binary is made out of, basing on a
tag whose name matches "v[0-9]*", e.g. v2.11.0-rc2-2-g7f1dc9.
In the very early days, with 9b88fcef7d ("Makefile: use git-describe
to mark the git version.", 2005-12-27), we used "--abbrev=4" to get
absolute minimum number of abbreviated commit object name. This was
later changed to match the default minimum of 7 with bf505158d0
("Git 1.7.10.1", 2012-05-01).
These days, the "default minimum" scales automatically depending on
the size of the repository, and there is no point in specifying a
particular abbreviation length; all we wanted since Git 1.7.10.1
days was to get "something reasonable we would use by default".
Just drop "--abbrev=<number>" from the invocation of "git describe"
and let the command pick what it thinks is appropriate, taking the
end user's configuration and the repository contents into account.
Signed-off-by: Ramsay Jones <ramsay@ramsayjones.plus.com>
Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
GIT-VERSION-GEN | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/GIT-VERSION-GEN b/GIT-VERSION-GEN
index 556fbfc104..f95b04bb36 100755
--- a/GIT-VERSION-GEN
+++ b/GIT-VERSION-GEN
@@ -12,7 +12,7 @@ if test -f version
then
VN=$(cat version) || VN="$DEF_VER"
elif test -d ${GIT_DIR:-.git} -o -f .git &&
- VN=$(git describe --match "v[0-9]*" --abbrev=7 HEAD 2>/dev/null) &&
+ VN=$(git describe --match "v[0-9]*" HEAD 2>/dev/null) &&
case "$VN" in
*$LF*) (exit 1) ;;
v[0-9]*)
--
2.11.0-263-gd89495280e
^ permalink raw reply related
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Stefan Beller @ 2016-12-06 18:22 UTC (permalink / raw)
To: Jeff King
Cc: Johannes Schindelin, Junio C Hamano, git@vger.kernel.org,
David Aguilar, Dennis Kaarsemaker
In-Reply-To: <20161206150955.mvq4ocamaei52bap@sigill.intra.peff.net>
On Tue, Dec 6, 2016 at 7:09 AM, Jeff King <peff@peff.net> wrote:
>>
>> Maybe even go a step further and say that the config code needs a context
>> "object".
>
> If I were writing git from scratch, I'd consider making a "struct
> repository" object. I'm not sure how painful it would be to retro-fit it
> at this point.
Would it be possible to introduce "the repo" struct similar to "the index"
in cache.h?
From a submodule perspective I would very much welcome this
object oriented approach to repositories.
^ permalink raw reply
* [PATCH v2 1/6] http: simplify update_url_from_redirect
From: Jeff King @ 2016-12-06 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206182414.466uotqfufcimpqb@sigill.intra.peff.net>
This function looks for a common tail between what we asked
for and where we were redirected to, but it open-codes the
comparison. We can avoid some confusing subtractions by
using strip_suffix_mem().
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 10 ++++------
1 file changed, 4 insertions(+), 6 deletions(-)
diff --git a/http.c b/http.c
index d8e427b69..d4034a14b 100644
--- a/http.c
+++ b/http.c
@@ -1500,7 +1500,7 @@ static int update_url_from_redirect(struct strbuf *base,
const struct strbuf *got)
{
const char *tail;
- size_t tail_len;
+ size_t new_len;
if (!strcmp(asked, got->buf))
return 0;
@@ -1509,14 +1509,12 @@ static int update_url_from_redirect(struct strbuf *base,
die("BUG: update_url_from_redirect: %s is not a superset of %s",
asked, base->buf);
- tail_len = strlen(tail);
-
- if (got->len < tail_len ||
- strcmp(tail, got->buf + got->len - tail_len))
+ new_len = got->len;
+ if (!strip_suffix_mem(got->buf, &new_len, tail))
return 0; /* insane redirect scheme */
strbuf_reset(base);
- strbuf_add(base, got->buf, got->len - tail_len);
+ strbuf_add(base, got->buf, new_len);
return 1;
}
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* [PATCH v2 3/6] remote-curl: rename shadowed options variable
From: Jeff King @ 2016-12-06 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206182414.466uotqfufcimpqb@sigill.intra.peff.net>
The discover_refs() function has a local "options" variable
to hold the http_get_options we pass to http_get_strbuf().
But this shadows the global "struct options" that holds our
program-level options, which cannot be accessed from this
function.
Let's give the local one a more descriptive name so we can
tell the two apart.
Signed-off-by: Jeff King <peff@peff.net>
---
remote-curl.c | 18 +++++++++---------
1 file changed, 9 insertions(+), 9 deletions(-)
diff --git a/remote-curl.c b/remote-curl.c
index 6b83b7783..e3803daa3 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -254,7 +254,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
struct strbuf effective_url = STRBUF_INIT;
struct discovery *last = last_discovery;
int http_ret, maybe_smart = 0;
- struct http_get_options options;
+ struct http_get_options http_options;
if (last && !strcmp(service, last->service))
return last;
@@ -271,15 +271,15 @@ static struct discovery *discover_refs(const char *service, int for_push)
strbuf_addf(&refs_url, "service=%s", service);
}
- memset(&options, 0, sizeof(options));
- options.content_type = &type;
- options.charset = &charset;
- options.effective_url = &effective_url;
- options.base_url = &url;
- options.no_cache = 1;
- options.keep_error = 1;
+ memset(&http_options, 0, sizeof(http_options));
+ http_options.content_type = &type;
+ http_options.charset = &charset;
+ http_options.effective_url = &effective_url;
+ http_options.base_url = &url;
+ http_options.no_cache = 1;
+ http_options.keep_error = 1;
- http_ret = http_get_strbuf(refs_url.buf, &buffer, &options);
+ http_ret = http_get_strbuf(refs_url.buf, &buffer, &http_options);
switch (http_ret) {
case HTTP_OK:
break;
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* [PATCH v2 2/6] http: always update the base URL for redirects
From: Jeff King @ 2016-12-06 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206182414.466uotqfufcimpqb@sigill.intra.peff.net>
If a malicious server redirects the initial ref
advertisement, it may be able to leak sha1s from other,
unrelated servers that the client has access to. For
example, imagine that Alice is a git user, she has access to
a private repository on a server hosted by Bob, and Mallory
runs a malicious server and wants to find out about Bob's
private repository.
Mallory asks Alice to clone an unrelated repository from her
over HTTP. When Alice's client contacts Mallory's server for
the initial ref advertisement, the server issues an HTTP
redirect for Bob's server. Alice contacts Bob's server and
gets the ref advertisement for the private repository. If
there is anything to fetch, she then follows up by asking
the server for one or more sha1 objects. But who is the
server?
If it is still Mallory's server, then Alice will leak the
existence of those sha1s to her.
Since commit c93c92f30 (http: update base URLs when we see
redirects, 2013-09-28), the client usually rewrites the base
URL such that all further requests will go to Bob's server.
But this is done by textually matching the URL. If we were
originally looking for "http://mallory/repo.git/info/refs",
and we got pointed at "http://bob/other.git/info/refs", then
we know that the right root is "http://bob/other.git".
If the redirect appears to change more than just the root,
we punt and continue to use the original server. E.g.,
imagine the redirect adds a URL component that Bob's server
will ignore, like "http://bob/other.git/info/refs?dummy=1".
We can solve this by aborting in this case rather than
silently continuing to use Mallory's server. In addition to
protecting from sha1 leakage, it's arguably safer and more
sane to refuse a confusing redirect like that in general.
For example, part of the motivation in c93c92f30 is
avoiding accidentally sending credentials over clear http,
just to get a response that says "try again over https". So
even in a non-malicious case, we'd prefer to err on the side
of caution.
The downside is that it's possible this will break a
legitimate but complicated server-side redirection scheme.
The setup given in the newly added test does work, but it's
convoluted enough that we don't need to care about it. A
more plausible case would be a server which redirects a
request for "info/refs?service=git-upload-pack" to just
"info/refs" (because it does not do smart HTTP, and for some
reason really dislikes query parameters). Right now we
would transparently downgrade to dumb-http, but with this
patch, we'd complain (and the user would have to set
GIT_SMART_HTTP=0 to fetch).
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
http.c | 12 ++++++++----
t/lib-httpd/apache.conf | 8 ++++++++
t/t5551-http-fetch-smart.sh | 4 ++++
t/t5812-proto-disable-http.sh | 1 +
4 files changed, 21 insertions(+), 4 deletions(-)
diff --git a/http.c b/http.c
index d4034a14b..718d2109b 100644
--- a/http.c
+++ b/http.c
@@ -1491,9 +1491,9 @@ static int http_request(const char *url,
*
* Note that this assumes a sane redirect scheme. It's entirely possible
* in the example above to end up at a URL that does not even end in
- * "info/refs". In such a case we simply punt, as there is not much we can
- * do (and such a scheme is unlikely to represent a real git repository,
- * which means we are likely about to abort anyway).
+ * "info/refs". In such a case we die. There's not much we can do, such a
+ * scheme is unlikely to represent a real git repository, and failing to
+ * rewrite the base opens options for malicious redirects to do funny things.
*/
static int update_url_from_redirect(struct strbuf *base,
const char *asked,
@@ -1511,10 +1511,14 @@ static int update_url_from_redirect(struct strbuf *base,
new_len = got->len;
if (!strip_suffix_mem(got->buf, &new_len, tail))
- return 0; /* insane redirect scheme */
+ die(_("unable to update url base from redirection:\n"
+ " asked for: %s\n"
+ " redirect: %s"),
+ asked, got->buf);
strbuf_reset(base);
strbuf_add(base, got->buf, new_len);
+
return 1;
}
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 018a83a5a..9a355fb1c 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -132,6 +132,14 @@ RewriteRule ^/ftp-redir/(.*)$ ftp://localhost:1000/$1 [R=302]
RewriteRule ^/loop-redir/x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-x-(.*) /$1 [R=302]
RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
+# The first rule issues a client-side redirect to something
+# that _doesn't_ look like a git repo. The second rule is a
+# server-side rewrite, so that it turns out the odd-looking
+# thing _is_ a git repo. The "[PT]" tells Apache to match
+# the usual ScriptAlias rules for /smart.
+RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
+RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
+
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
# And as RewriteCond does not allow testing for non-matches, we match
# the desired case first (one has abra, two has cadabra), and let it
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 2f375eb94..d8826acde 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -110,6 +110,10 @@ test_expect_success 'redirects re-root further requests' '
git clone $HTTPD_URL/smart-redir-limited/repo.git repo-redir-limited
'
+test_expect_success 're-rooting dies on insane schemes' '
+ test_must_fail git clone $HTTPD_URL/insane-redir/repo.git insane
+'
+
test_expect_success 'clone from password-protected repository' '
echo two >expect &&
set_askpass user@host pass@host &&
diff --git a/t/t5812-proto-disable-http.sh b/t/t5812-proto-disable-http.sh
index 0d105d541..044cc152f 100755
--- a/t/t5812-proto-disable-http.sh
+++ b/t/t5812-proto-disable-http.sh
@@ -18,6 +18,7 @@ test_proto "smart http" http "$HTTPD_URL/smart/repo.git"
test_expect_success 'curl redirects respect whitelist' '
test_must_fail env GIT_ALLOW_PROTOCOL=http:https \
+ GIT_SMART_HTTP=0 \
git clone "$HTTPD_URL/ftp-redir/repo.git" 2>stderr &&
{
test_i18ngrep "ftp.*disabled" stderr ||
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* [PATCH v2 4/6] http: make redirects more obvious
From: Jeff King @ 2016-12-06 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206182414.466uotqfufcimpqb@sigill.intra.peff.net>
We instruct curl to always follow HTTP redirects. This is
convenient, but it creates opportunities for malicious
servers to create confusing situations. For instance,
imagine Alice is a git user with access to a private
repository on Bob's server. Mallory runs her own server and
wants to access objects from Bob's repository.
Mallory may try a few tricks that involve asking Alice to
clone from her, build on top, and then push the result:
1. Mallory may simply redirect all fetch requests to Bob's
server. Git will transparently follow those redirects
and fetch Bob's history, which Alice may believe she
got from Mallory. The subsequent push seems like it is
just feeding Mallory back her own objects, but is
actually leaking Bob's objects. There is nothing in
git's output to indicate that Bob's repository was
involved at all.
The downside (for Mallory) of this attack is that Alice
will have received Bob's entire repository, and is
likely to notice that when building on top of it.
2. If Mallory happens to know the sha1 of some object X in
Bob's repository, she can instead build her own history
that references that object. She then runs a dumb http
server, and Alice's client will fetch each object
individually. When it asks for X, Mallory redirects her
to Bob's server. The end result is that Alice obtains
objects from Bob, but they may be buried deep in
history. Alice is less likely to notice.
Both of these attacks are fairly hard to pull off. There's a
social component in getting Mallory to convince Alice to
work with her. Alice may be prompted for credentials in
accessing Bob's repository (but not always, if she is using
a credential helper that caches). Attack (1) requires a
certain amount of obliviousness on Alice's part while making
a new commit. Attack (2) requires that Mallory knows a sha1
in Bob's repository, that Bob's server supports dumb http,
and that the object in question is loose on Bob's server.
But we can probably make things a bit more obvious without
any loss of functionality. This patch does two things to
that end.
First, when we encounter a whole-repo redirect during the
initial ref discovery, we now inform the user on stderr,
making attack (1) much more obvious.
Second, the decision to follow redirects is now
configurable. The truly paranoid can set the new
http.followRedirects to false to avoid any redirection
entirely. But for a more practical default, we will disallow
redirects only after the initial ref discovery. This is
enough to thwart attacks similar to (2), while still
allowing the common use of redirects at the repository
level. Since c93c92f30 (http: update base URLs when we see
redirects, 2013-09-28) we re-root all further requests from
the redirect destination, which should generally mean that
no further redirection is necessary.
As an escape hatch, in case there really is a server that
needs to redirect individual requests, the user can set
http.followRedirects to "true" (and this can be done on a
per-server basis via http.*.followRedirects config).
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
Documentation/config.txt | 10 ++++++++++
http.c | 31 +++++++++++++++++++++++++++++--
http.h | 10 +++++++++-
remote-curl.c | 4 ++++
t/lib-httpd/apache.conf | 6 ++++++
t/t5550-http-fetch-dumb.sh | 23 +++++++++++++++++++++++
6 files changed, 81 insertions(+), 3 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index f4721a048..815333643 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -1833,6 +1833,16 @@ http.userAgent::
of common USER_AGENT strings (but not including those like git/1.7.1).
Can be overridden by the `GIT_HTTP_USER_AGENT` environment variable.
+http.followRedirects::
+ Whether git should follow HTTP redirects. If set to `true`, git
+ will transparently follow any redirect issued by a server it
+ encounters. If set to `false`, git will treat all redirects as
+ errors. If set to `initial`, git will follow redirects only for
+ the initial request to a remote, but not for subsequent
+ follow-up HTTP requests. Since git uses the redirected URL as
+ the base for the follow-up requests, this is generally
+ sufficient. The default is `initial`.
+
http.<url>.*::
Any of the http.* options above can be applied selectively to some URLs.
For a config key to match a URL, each element of the config key is
diff --git a/http.c b/http.c
index 718d2109b..b99ade5fa 100644
--- a/http.c
+++ b/http.c
@@ -98,6 +98,8 @@ static int http_proactive_auth;
static const char *user_agent;
static int curl_empty_auth;
+enum http_follow_config http_follow_config = HTTP_FOLLOW_INITIAL;
+
#if LIBCURL_VERSION_NUM >= 0x071700
/* Use CURLOPT_KEYPASSWD as is */
#elif LIBCURL_VERSION_NUM >= 0x070903
@@ -337,6 +339,16 @@ static int http_options(const char *var, const char *value, void *cb)
return 0;
}
+ if (!strcmp("http.followredirects", var)) {
+ if (value && !strcmp(value, "initial"))
+ http_follow_config = HTTP_FOLLOW_INITIAL;
+ else if (git_config_bool(var, value))
+ http_follow_config = HTTP_FOLLOW_ALWAYS;
+ else
+ http_follow_config = HTTP_FOLLOW_NONE;
+ return 0;
+ }
+
/* Fall back on the default ones */
return git_default_config(var, value, cb);
}
@@ -553,7 +565,6 @@ static CURL *get_curl_handle(void)
curl_low_speed_time);
}
- curl_easy_setopt(result, CURLOPT_FOLLOWLOCATION, 1);
curl_easy_setopt(result, CURLOPT_MAXREDIRS, 20);
#if LIBCURL_VERSION_NUM >= 0x071301
curl_easy_setopt(result, CURLOPT_POSTREDIR, CURL_REDIR_POST_ALL);
@@ -882,6 +893,16 @@ struct active_request_slot *get_active_slot(void)
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 1);
curl_easy_setopt(slot->curl, CURLOPT_RANGE, NULL);
+ /*
+ * Default following to off unless "ALWAYS" is configured; this gives
+ * callers a sane starting point, and they can tweak for individual
+ * HTTP_FOLLOW_* cases themselves.
+ */
+ if (http_follow_config == HTTP_FOLLOW_ALWAYS)
+ curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
+ else
+ curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 0);
+
#if LIBCURL_VERSION_NUM >= 0x070a08
curl_easy_setopt(slot->curl, CURLOPT_IPRESOLVE, git_curl_ipresolve);
#endif
@@ -1122,9 +1143,12 @@ static int handle_curl_result(struct slot_results *results)
* If we see a failing http code with CURLE_OK, we have turned off
* FAILONERROR (to keep the server's custom error response), and should
* translate the code into failure here.
+ *
+ * Likewise, if we see a redirect (30x code), that means we turned off
+ * redirect-following, and we should treat the result as an error.
*/
if (results->curl_result == CURLE_OK &&
- results->http_code >= 400) {
+ results->http_code >= 300) {
results->curl_result = CURLE_HTTP_RETURNED_ERROR;
/*
* Normally curl will already have put the "reason phrase"
@@ -1443,6 +1467,9 @@ static int http_request(const char *url,
strbuf_addstr(&buf, " no-cache");
if (options && options->keep_error)
curl_easy_setopt(slot->curl, CURLOPT_FAILONERROR, 0);
+ if (options && options->initial_request &&
+ http_follow_config == HTTP_FOLLOW_INITIAL)
+ curl_easy_setopt(slot->curl, CURLOPT_FOLLOWLOCATION, 1);
headers = curl_slist_append(headers, buf.buf);
diff --git a/http.h b/http.h
index 36f558bfb..31b4cc94b 100644
--- a/http.h
+++ b/http.h
@@ -116,6 +116,13 @@ extern struct credential http_auth;
extern char curl_errorstr[CURL_ERROR_SIZE];
+enum http_follow_config {
+ HTTP_FOLLOW_NONE,
+ HTTP_FOLLOW_ALWAYS,
+ HTTP_FOLLOW_INITIAL
+};
+extern enum http_follow_config http_follow_config;
+
static inline int missing__target(int code, int result)
{
return /* file:// URL -- do we ever use one??? */
@@ -139,7 +146,8 @@ extern char *get_remote_object_url(const char *url, const char *hex,
/* Options for http_get_*() */
struct http_get_options {
unsigned no_cache:1,
- keep_error:1;
+ keep_error:1,
+ initial_request:1;
/* If non-NULL, returns the content-type of the response. */
struct strbuf *content_type;
diff --git a/remote-curl.c b/remote-curl.c
index e3803daa3..05ae8dead 100644
--- a/remote-curl.c
+++ b/remote-curl.c
@@ -276,6 +276,7 @@ static struct discovery *discover_refs(const char *service, int for_push)
http_options.charset = &charset;
http_options.effective_url = &effective_url;
http_options.base_url = &url;
+ http_options.initial_request = 1;
http_options.no_cache = 1;
http_options.keep_error = 1;
@@ -294,6 +295,9 @@ static struct discovery *discover_refs(const char *service, int for_push)
die("unable to access '%s': %s", url.buf, curl_errorstr);
}
+ if (options.verbosity && !starts_with(refs_url.buf, url.buf))
+ warning(_("redirecting to %s"), url.buf);
+
last= xcalloc(1, sizeof(*last_discovery));
last->service = service;
last->buf_alloc = strbuf_detach(&buffer, &last->len);
diff --git a/t/lib-httpd/apache.conf b/t/lib-httpd/apache.conf
index 9a355fb1c..5b408d649 100644
--- a/t/lib-httpd/apache.conf
+++ b/t/lib-httpd/apache.conf
@@ -123,6 +123,7 @@ ScriptAlias /error/ error.sh/
</Files>
RewriteEngine on
+RewriteRule ^/dumb-redir/(.*)$ /dumb/$1 [R=301]
RewriteRule ^/smart-redir-perm/(.*)$ /smart/$1 [R=301]
RewriteRule ^/smart-redir-temp/(.*)$ /smart/$1 [R=302]
RewriteRule ^/smart-redir-auth/(.*)$ /auth/smart/$1 [R=301]
@@ -140,6 +141,11 @@ RewriteRule ^/loop-redir/(.*)$ /loop-redir/x-$1 [R=302]
RewriteRule ^/insane-redir/(.*)$ /intern-redir/$1/foo [R=301]
RewriteRule ^/intern-redir/(.*)/foo$ /smart/$1 [PT]
+# Serve info/refs internally without redirecting, but
+# issue a redirect for any object requests.
+RewriteRule ^/redir-objects/(.*/info/refs)$ /dumb/$1 [PT]
+RewriteRule ^/redir-objects/(.*/objects/.*)$ /dumb/$1 [R=301]
+
# Apache 2.2 does not understand <RequireAll>, so we use RewriteCond.
# And as RewriteCond does not allow testing for non-matches, we match
# the desired case first (one has abra, two has cadabra), and let it
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index 3484b6f0f..ad94ed7b1 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -299,5 +299,28 @@ test_expect_success 'git client does not send an empty Accept-Language' '
! grep "^Accept-Language:" stderr
'
+test_expect_success 'redirects can be forbidden/allowed' '
+ test_must_fail git -c http.followRedirects=false \
+ clone $HTTPD_URL/dumb-redir/repo.git dumb-redir &&
+ git -c http.followRedirects=true \
+ clone $HTTPD_URL/dumb-redir/repo.git dumb-redir 2>stderr
+'
+
+test_expect_success 'redirects are reported to stderr' '
+ # just look for a snippet of the redirected-to URL
+ test_i18ngrep /dumb/ stderr
+'
+
+test_expect_success 'non-initial redirects can be forbidden' '
+ test_must_fail git -c http.followRedirects=initial \
+ clone $HTTPD_URL/redir-objects/repo.git redir-objects &&
+ git -c http.followRedirects=true \
+ clone $HTTPD_URL/redir-objects/repo.git redir-objects
+'
+
+test_expect_success 'http.followRedirects defaults to "initial"' '
+ test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
+'
+
stop_httpd
test_done
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* [PATCH v2 5/6] http: treat http-alternates like redirects
From: Jeff King @ 2016-12-06 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206182414.466uotqfufcimpqb@sigill.intra.peff.net>
The previous commit made HTTP redirects more obvious and
tightened up the default behavior. However, there's another
way for a server to ask a git client to fetch arbitrary
content: by having an http-alternates file (or a regular
alternates file, which is used as a backup).
Similar to the HTTP redirect case, a malicious server can
claim to have refs pointing at object X, return a 404 when
the client asks for X, but point to some other URL via
http-alternates, which the client will transparently fetch.
The end result is that it looks from the user's perspective
like the objects came from the malicious server, as the
other URL is not mentioned at all.
Worse, because we feed the new URL to curl ourselves, the
usual protocol restrictions do not kick in (neither curl's
default of disallowing file://, nor the protocol
whitelisting in f4113cac0 (http: limit redirection to
protocol-whitelist, 2015-09-22).
Let's apply the same rules here as we do for HTTP redirects.
Namely:
- unless http.followRedirects is set to "always", we will
not follow remote redirects from http-alternates (or
alternates) at all
- set CURLOPT_PROTOCOLS alongside CURLOPT_REDIR_PROTOCOLS
restrict ourselves to a known-safe set and respect any
user-provided whitelist.
- mention alternate object stores on stderr so that the
user is aware another source of objects may be involved
The first item may prove to be too restrictive. The most
common use of alternates is to point to another path on the
same server. While it's possible for a single-server
redirect to be an attack, it takes a fairly obscure setup
(victim and evil repository on the same host, host speaks
dumb http, and evil repository has access to edit its own
http-alternates file).
So we could make the checks more specific, and only cover
cross-server redirects. But that means parsing the URLs
ourselves, rather than letting curl handle them. This patch
goes for the simpler approach. Given that they are only used
with dumb http, http-alternates are probably pretty rare.
And there's an escape hatch: the user can allow redirects on
a specific server by setting http.<url>.followRedirects to
"always".
Reported-by: Jann Horn <jannh@google.com>
Signed-off-by: Jeff King <peff@peff.net>
---
http-walker.c | 8 +++++---
http.c | 1 +
t/t5550-http-fetch-dumb.sh | 38 ++++++++++++++++++++++++++++++++++++++
3 files changed, 44 insertions(+), 3 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 2c721f0c3..4bff31c44 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -290,9 +290,8 @@ static void process_alternates_response(void *callback_data)
struct strbuf target = STRBUF_INIT;
strbuf_add(&target, base, serverlen);
strbuf_add(&target, data + i, posn - i - 7);
- if (walker->get_verbosely)
- fprintf(stderr, "Also look at %s\n",
- target.buf);
+ warning("adding alternate object store: %s",
+ target.buf);
newalt = xmalloc(sizeof(*newalt));
newalt->next = NULL;
newalt->base = strbuf_detach(&target, NULL);
@@ -318,6 +317,9 @@ static void fetch_alternates(struct walker *walker, const char *base)
struct alternates_request alt_req;
struct walker_data *cdata = walker->data;
+ if (http_follow_config != HTTP_FOLLOW_ALWAYS)
+ return;
+
/*
* If another request has already started fetching alternates,
* wait for them to arrive and return to processing this request's
diff --git a/http.c b/http.c
index b99ade5fa..a9778bfa4 100644
--- a/http.c
+++ b/http.c
@@ -581,6 +581,7 @@ static CURL *get_curl_handle(void)
if (is_transport_allowed("ftps"))
allowed_protocols |= CURLPROTO_FTPS;
curl_easy_setopt(result, CURLOPT_REDIR_PROTOCOLS, allowed_protocols);
+ curl_easy_setopt(result, CURLOPT_PROTOCOLS, allowed_protocols);
#else
if (transport_restrict_protocols())
warning("protocol restrictions not applied to curl redirects because\n"
diff --git a/t/t5550-http-fetch-dumb.sh b/t/t5550-http-fetch-dumb.sh
index ad94ed7b1..22011f0b6 100755
--- a/t/t5550-http-fetch-dumb.sh
+++ b/t/t5550-http-fetch-dumb.sh
@@ -322,5 +322,43 @@ test_expect_success 'http.followRedirects defaults to "initial"' '
test_must_fail git clone $HTTPD_URL/redir-objects/repo.git default
'
+# The goal is for a clone of the "evil" repository, which has no objects
+# itself, to cause the client to fetch objects from the "victim" repository.
+test_expect_success 'set up evil alternates scheme' '
+ victim=$HTTPD_DOCUMENT_ROOT_PATH/victim.git &&
+ git init --bare "$victim" &&
+ git -C "$victim" --work-tree=. commit --allow-empty -m secret &&
+ git -C "$victim" repack -ad &&
+ git -C "$victim" update-server-info &&
+ sha1=$(git -C "$victim" rev-parse HEAD) &&
+
+ evil=$HTTPD_DOCUMENT_ROOT_PATH/evil.git &&
+ git init --bare "$evil" &&
+ # do this by hand to avoid object existence check
+ printf "%s\\t%s\\n" $sha1 refs/heads/master >"$evil/info/refs"
+'
+
+# Here we'll just redirect via HTTP. In a real-world attack these would be on
+# different servers, but we should reject it either way.
+test_expect_success 'http-alternates is a non-initial redirect' '
+ echo "$HTTPD_URL/dumb/victim.git/objects" \
+ >"$evil/objects/info/http-alternates" &&
+ test_must_fail git -c http.followRedirects=initial \
+ clone $HTTPD_URL/dumb/evil.git evil-initial &&
+ git -c http.followRedirects=true \
+ clone $HTTPD_URL/dumb/evil.git evil-initial
+'
+
+# Curl supports a lot of protocols that we'd prefer not to allow
+# http-alternates to use, but it's hard to test whether curl has
+# accessed, say, the SMTP protocol, because we are not running an SMTP server.
+# But we can check that it does not allow access to file://, which would
+# otherwise allow this clone to complete.
+test_expect_success 'http-alternates cannot point at funny protocols' '
+ echo "file://$victim/objects" >"$evil/objects/info/http-alternates" &&
+ test_must_fail git -c http.followRedirects=true \
+ clone "$HTTPD_URL/dumb/evil.git" evil-file
+'
+
stop_httpd
test_done
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* [PATCH v2 6/6] http-walker: complain about non-404 loose object errors
From: Jeff King @ 2016-12-06 18:25 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206182414.466uotqfufcimpqb@sigill.intra.peff.net>
Since commit 17966c0a6 (http: avoid disconnecting on 404s
for loose objects, 2016-07-11), we turn off curl's
FAILONERROR option and instead manually deal with failing
HTTP codes.
However, the logic to do so only recognizes HTTP 404 as a
failure. This is probably the most common result, but if we
were to get another code, the curl result remains CURLE_OK,
and we treat it as success. We still end up detecting the
failure when we try to zlib-inflate the object (which will
fail), but instead of reporting the HTTP error, we just
claim that the object is corrupt.
Instead, let's catch anything in the 300's or above as an
error (300's are redirects which are not an error at the
HTTP level, but are an indication that we've explicitly
disabled redirects, so we should treat them as such; we
certainly don't have the resulting object content).
Note that we also fill in req->errorstr, which we didn't do
before. Without FAILONERROR, curl will not have filled this
in, and it will remain a blank string. This never mattered
for the 404 case, because in the logic below we hit the
"missing_target()" branch and print nothing. But for other
errors, we'd want to say _something_, if only to fill in the
blank slot in the error message.
Signed-off-by: Jeff King <peff@peff.net>
---
The second hunk here is new in v2; earlier it appeared in patch 3. But
arguably it goes better here anyway; I didn't even need to modify the
commit message to explain it.
http-walker.c | 7 +++++--
http.c | 2 +-
2 files changed, 6 insertions(+), 3 deletions(-)
diff --git a/http-walker.c b/http-walker.c
index 25a8b1ad4..c2f81cd6a 100644
--- a/http-walker.c
+++ b/http-walker.c
@@ -482,10 +482,13 @@ static int fetch_object(struct walker *walker, unsigned char *sha1)
* we turned off CURLOPT_FAILONERROR to avoid losing a
* persistent connection and got CURLE_OK.
*/
- if (req->http_code == 404 && req->curl_result == CURLE_OK &&
+ if (req->http_code >= 300 && req->curl_result == CURLE_OK &&
(starts_with(req->url, "http://") ||
- starts_with(req->url, "https://")))
+ starts_with(req->url, "https://"))) {
req->curl_result = CURLE_HTTP_RETURNED_ERROR;
+ xsnprintf(req->errorstr, sizeof(req->errorstr),
+ "HTTP request failed");
+ }
if (obj_req->state == ABORTED) {
ret = error("Request for %s aborted", hex);
diff --git a/http.c b/http.c
index ff4d88231..c25d1d540 100644
--- a/http.c
+++ b/http.c
@@ -2021,7 +2021,7 @@ static size_t fwrite_sha1_file(char *ptr, size_t eltsize, size_t nmemb,
if (c != CURLE_OK)
die("BUG: curl_easy_getinfo for HTTP code failed: %s",
curl_easy_strerror(c));
- if (slot->http_code >= 400)
+ if (slot->http_code >= 300)
return size;
}
--
2.11.0.191.gdb26c57
^ permalink raw reply related
* Re: [RFC PATCH] GIT-VERSION-GEN: set --abbrev=9 to match auto-scaling
From: Jeff King @ 2016-12-06 18:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Ramsay Jones, GIT Mailing-list
In-Reply-To: <xmqqwpfc2a7g.fsf@gitster.mtv.corp.google.com>
On Tue, Dec 06, 2016 at 10:17:55AM -0800, Junio C Hamano wrote:
> Yup, that is what I meant to say with "that is already possible" and
> we are on the same page. As all three of us seem to be happy with
> just dropping --abbrev and letting describe do its default thing (as
> configured by whoever is doing the build), let's do so.
>
> -- >8 --
> From: Ramsay Jones <ramsay@ramsayjones.plus.com>
> Date: Sun, 4 Dec 2016 20:45:59 +0000
> Subject: [PATCH] GIT-VERSION-GEN: do not force abbreviation length used by 'describe'
Thanks, this is a nice summary of the discussion, and the patch is
obviously correct.
-Peff
^ permalink raw reply
* [PATCH v2] jk/http-walker-limit-redirect rebased to maint-2.9
From: Jeff King @ 2016-12-06 18:24 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Brandon Williams, git, sbeller, bburky, jrnieder
In-Reply-To: <20161206181008.yaz2md3343pukaov@sigill.intra.peff.net>
On Tue, Dec 06, 2016 at 01:10:08PM -0500, Jeff King wrote:
> > I think I merged yours and then Brandon's on jch/pu branches in that
> > order, and the conflict resolution should look OK.
> >
> > I however forked yours on v2.11.0-rc1, which would need to be
> > rebased to one of the earlier maintenance tracks, before we can
> > merge it to 'next'.
>
> Yeah, I built it on top of master.
>
> It does depend on some of the http-walker changes Eric made a few months
> ago. In particular, 17966c0a6 (http: avoid disconnecting on 404s for
> loose objects, 2016-07-11) added some checks against the HTTP status
> code, and my series modifies the checks (mostly so that ">= 400" becomes
> ">= 300").
>
> Rebasing on maint-2.9 means omitting those changes. That preserves the
> security properties, but means that the error handling is worse when we
> see an illegal redirect. That may be OK, though.
>
> Since the resolution is to omit the changes entirely from my series,
> merging up to v2.11 wouldn't produce any conflicts. We'd need to have a
> separate set of patches adding those changes back in.
This actually turned out to be pretty easy. The final patch from my
original series is the one that tweaks the error-handling from
17966c0a6. The rest of them are _almost_ untouched, except that one of
the error-handling tweaks gets bumped to the final patch.
So here's a resend of the patches, rebased on your maint-2.9 branch.
Patches 1-5 should be applied directly there.
On maint-2.10, you can merge up maint-2.9, and then apply patch 6.
Hopefully that makes sense, but I've pushed branches
jk/maint-X-http-redirect to https://github.com/peff/git that show the
final configuration (and a diff of jk/maint-2.10-http-redirect shows the
outcome is identical to applying the original series on top of 2.10).
Merging up to 2.11 should be trivial.
[1/6]: http: simplify update_url_from_redirect
[2/6]: http: always update the base URL for redirects
[3/6]: remote-curl: rename shadowed options variable
[4/6]: http: make redirects more obvious
[5/6]: http: treat http-alternates like redirects
[6/6]: http-walker: complain about non-404 loose object errors
-Peff
^ permalink raw reply
* Re: [PATCH] clone,fetch: explain the shallow-clone option a little more clearly
From: Junio C Hamano @ 2016-12-06 18:29 UTC (permalink / raw)
To: Duy Nguyen; +Cc: Alex Henrie, Git Mailing List
In-Reply-To: <CACsJy8Cy+iWw4NhR7B_fZhBMMYt2QxdMeFsC=++LLnrTLNWfKw@mail.gmail.com>
Duy Nguyen <pclouds@gmail.com> writes:
> On Tue, Dec 6, 2016 at 1:28 AM, Junio C Hamano <gitster@pobox.com> wrote:
>> I however offhand do not think the feature can be used to make the
>> repository shallower
>
> I'm pretty sure it can,...
I wrote my message after a short local testing, but it is very
possible I botched it and reached a wrong conclusion above.
If we can use the command to make it shallower, then the phrase
"deepen" would probably be what we need to be fixing in this patch:
> OPT_STRING_LIST(0, "shallow-exclude", &option_not, N_("revision"),
> - N_("deepen history of shallow clone by excluding rev")),
> + N_("deepen history of shallow clone, excluding rev")),
Perhaps a shorter version of:
Adjust the depth of shallow clone so that commits that are
decendants of the named rev are made available, while commits
that are ancestors of the named rev are made beyond reach.
or something like that. Here is my (somewhat botched) attempt:
Adjust shallow clone's history to be cut at the rev
^ permalink raw reply
* Re: [PATCH v3 1/2] difftool: add a skeleton for the upcoming builtin
From: Jeff King @ 2016-12-06 18:35 UTC (permalink / raw)
To: Stefan Beller
Cc: Johannes Schindelin, Junio C Hamano, git@vger.kernel.org,
David Aguilar, Dennis Kaarsemaker
In-Reply-To: <CAGZ79kZoy2zSgSEc7kfAZ9tg9_uJxa+_FNFVO8UEDLVK6YDxVg@mail.gmail.com>
On Tue, Dec 06, 2016 at 10:22:21AM -0800, Stefan Beller wrote:
> >> Maybe even go a step further and say that the config code needs a context
> >> "object".
> >
> > If I were writing git from scratch, I'd consider making a "struct
> > repository" object. I'm not sure how painful it would be to retro-fit it
> > at this point.
>
> Would it be possible to introduce "the repo" struct similar to "the index"
> in cache.h?
>
> From a submodule perspective I would very much welcome this
> object oriented approach to repositories.
I think it may be more complicated, because there's some implicit global
state in "the repo", like where files are relative to our cwd. All of
those low-level functions would have to start caring about which repo
we're talking about so they can prefix the appropriate working tree
path, etc.
For some operations that would be fine, but there are things that would
subtly fail for submodules. I'm thinking we'd end up with some code
state like:
/* finding a repo does not modify global state; good */
struct repository *repo = repo_discover(".");
/* obvious repo-level operations like looking up refs can be done with
* a repository object; good */
repo_for_each_ref(repo, callback, NULL);
/*
* "enter" the repo so that we are at the top-level of the working
* tree, etc. After this you can actually look at the index without
* things breaking.
*/
repo_enter(repo);
That would be enough to implement a lot of submodule-level stuff, but it
would break pretty subtly as soon as you asked the submodule about its
working tree. The solution is to make everything that accesses the
working tree aware of the idea of a working tree root besides the cwd.
But that's a pretty invasive change.
-Peff
^ permalink raw reply
* Re: [PATCH v15 11/27] bisect--helper: `bisect_next_check` & bisect_voc shell function in C
From: Pranit Bauva @ 2016-12-06 18:39 UTC (permalink / raw)
To: Stephan Beyer; +Cc: Git List
In-Reply-To: <b78a4cbf-86ed-938e-1d41-6c48e0df981e@gmx.net>
Hey Stephan,
Sorry for the late replies. My end semester exams just got over.
On Fri, Nov 18, 2016 at 2:29 AM, Stephan Beyer <s-beyer@gmx.net> wrote:
>
> Hi Pranit,
>
> On 10/14/2016 04:14 PM, Pranit Bauva wrote:
> > Also reimplement `bisect_voc` shell function in C and call it from
> > `bisect_next_check` implementation in C.
>
> Please don't! ;D
>
> > +static char *bisect_voc(char *revision_type)
> > +{
> > + if (!strcmp(revision_type, "bad"))
> > + return "bad|new";
> > + if (!strcmp(revision_type, "good"))
> > + return "good|old";
> > +
> > + return NULL;
> > +}
>
> Why not simply use something like this:
>
> static const char *voc[] = {
> "bad|new",
> "good|old",
> };
>
> Then...
This probably seems a good idea.
> > +static int bisect_next_check(const struct bisect_terms *terms,
> > + const char *current_term)
> > +{
> > + int missing_good = 1, missing_bad = 1, retval = 0;
> > + char *bad_ref = xstrfmt("refs/bisect/%s", terms->term_bad);
> > + char *good_glob = xstrfmt("%s-*", terms->term_good);
> > + char *bad_syn, *good_syn;
>
> ...you don't need bad_syn and good_syn...
>
> > + bad_syn = xstrdup(bisect_voc("bad"));
> > + good_syn = xstrdup(bisect_voc("good"));
>
> ...and hence not these two lines...
>
> > + if (!is_empty_or_missing_file(git_path_bisect_start())) {
> > + error(_("You need to give me at least one %s and "
> > + "%s revision. You can use \"git bisect %s\" "
> > + "and \"git bisect %s\" for that. \n"),
> > + bad_syn, good_syn, bad_syn, good_syn);
>
> ...and write
> voc[0], voc[1], voc[0], voc[1]);
> instead...
>
> > + retval = -1;
> > + goto finish;
> > + }
> > + else {
> > + error(_("You need to start by \"git bisect start\". You "
> > + "then need to give me at least one %s and %s "
> > + "revision. You can use \"git bisect %s\" and "
> > + "\"git bisect %s\" for that.\n"),
> > + good_syn, bad_syn, bad_syn, good_syn);
>
> ...and here
> voc[1], voc[0], voc[0], voc[1]);
> ...
>
> > + retval = -1;
> > + goto finish;
> > + }
> > + goto finish;
> > +finish:
> > + if (!bad_ref)
> > + free(bad_ref);
> > + if (!good_glob)
> > + free(good_glob);
> > + if (!bad_syn)
> > + free(bad_syn);
> > + if (!good_syn)
> > + free(good_syn);
>
> ...and you can remove the 4 lines above.
>
> > + return retval;
> > +}
>
> Besides that, there are again some things that I've already mentioned
> and that can be applied here, too, for example, not capitalizing
> TERM_GOOD and TERM_BAD, the goto fail simplification, the terms memory leak.
Your suggestion simplifies things, I will use that.
Regards,
Pranit Bauva
^ permalink raw reply
* BUG: "cherry-pick A..B || git reset --hard OTHER"
From: Junio C Hamano @ 2016-12-06 18:58 UTC (permalink / raw)
To: git; +Cc: Christian Couder, SZEDER Gábor
I was burned a few times with this in the past few years, but it did
not irritate me often enough that I didn't write it down. But I
think this is serious enough that deserves attention from those who
were involved.
A short reproduction recipe, using objects from git.git that are
publicly available and stable, shows how bad it is.
$ git checkout v2.9.3^0
$ git cherry-pick 0582a34f52..a94bb68397
... see conflict, decide to give up backporting to
... such an old fork point.
... the git-prompt gives "|CHERRY-PICKING" correctly.
$ git reset --hard v2.10.2^0
... the git-prompt no longer says "|CHERRY-PICKING"
$ edit && git commit -m "prelim work for backporting"
[detached HEAD cc5a6a9219] prelim work for backporting
$ git cherry-pick 0582a34f52..a94bb68397
error: a cherry-pick or revert is already in progress
hint: try "git cherry-pick (--continue | --quit | --abort)"
fatal: cherry-pick failed
$ git cherry-pick --abort
... we come back to v2.9.3^0, losing the new commit!
The above shows two bugs.
(1) The third invocation of "cherry-pick" with "--abort" to get rid
of the state from the unfinished cherry-pick we did previously
is necessary, but the command does not notice that we resetted
to a new branch AND we even did some other work there. This
loses end-user's work.
"git cherry-pick --abort" should learn from "git am --abort"
that has an extra safety to deal with the above workflow. The
state from the unfinished "am" is removed, but the head is not
rewound to avoid losing end-user's work.
You can try by replacing two instances of
$ git cherry-pick 0582a34f52..a94bb68397
with
$ git format-patch --stdout 0582a34f52..a94bb68397 | git am
in the above sequence, and conclude with "git am--abort" to see
how much more pleasant and safe "git am --abort" is.
(2) The bug in "cherry-pick --abort" is made worse because the
git-completion script seems to be unaware of $GIT_DIR/sequencer
and stops saying "|CHERRY-PICKING" after the step to switch to
a different state with "git reset --hard v2.10.2^0". If the
prompt showed it after "git reset", the end user would have
thought twice before starting the "prelim work".
Thanks.
^ permalink raw reply
* Re: [PATCH] stash: disable renames when calling git-diff
From: Junio C Hamano @ 2016-12-06 19:20 UTC (permalink / raw)
To: Jeff King; +Cc: Matthew Patey, git
In-Reply-To: <20161206154120.yyuca35ugyuifpq6@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
>> If you run:
>>
>> git -c diff.renames=false stash
>>
>> then it works.
>
> And here's a patch to fix it.
Yuck. This obviously has easier to bite more people since we
enabled the renames by default. Thanks for a quick fix.
I wonder why we are using "git diff" here, not the plumbing,
though.
>
> -- >8 --
> Subject: [PATCH] stash: disable renames when calling git-diff
>
> When creating a stash, we need to look at the diff between
> the working tree and HEAD. There's no plumbing command that
> covers this case, so we do so by calling the git-diff
> porcelain. This means we also inherit the usual list of
> porcelain options, which can impact the output in confusing
> ways.
>
> There's at least one known problem: --name-only will not
> mention the source side of a rename, meaning that we will
> fail to stash a deletion in such a case.
>
> The correct solution is to move to an appropriate plumbing
> command. But since there isn't one available, in the short
> term we can plug this particular problem by manually asking
> git-diff not to respect renames.
>
> Reported-by: Matthew Patey <matthew.patey2167@gmail.com>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> There may be other similar problems lurking depending on the various
> config options you have set, but I don't think so. Most of the options
> only affect --patch output.
>
> I do find it interesting that --name-only does not mention the source
> side of a rename. The longer forms like --name-status mention both
> sides. Certainly --name-status does not have any way of saying "this is
> the source side of a rename", but I'd think it would simply mention both
> sides. Anyway, even if that were changed (which would fix this bug), I
> think adding --no-renames is still a good thing to be doing here.
>
> git-stash.sh | 2 +-
> t/t3903-stash.sh | 9 +++++++++
> 2 files changed, 10 insertions(+), 1 deletion(-)
>
> diff --git a/git-stash.sh b/git-stash.sh
> index 4546abaae..40ab2710e 100755
> --- a/git-stash.sh
> +++ b/git-stash.sh
> @@ -115,7 +115,7 @@ create_stash () {
> git read-tree --index-output="$TMPindex" -m $i_tree &&
> GIT_INDEX_FILE="$TMPindex" &&
> export GIT_INDEX_FILE &&
> - git diff --name-only -z HEAD -- >"$TMP-stagenames" &&
> + git diff --name-only -z --no-renames HEAD -- >"$TMP-stagenames" &&
> git update-index -z --add --remove --stdin <"$TMP-stagenames" &&
> git write-tree &&
> rm -f "$TMPindex"
> diff --git a/t/t3903-stash.sh b/t/t3903-stash.sh
> index e1a6ccc00..2de3e18ce 100755
> --- a/t/t3903-stash.sh
> +++ b/t/t3903-stash.sh
> @@ -766,4 +766,13 @@ test_expect_success 'stash list --cc shows combined diff' '
> test_cmp expect actual
> '
>
> +test_expect_success 'stash is not confused by partial renames' '
> + mv file renamed &&
> + git add renamed &&
> + git stash &&
> + git stash apply &&
> + test_path_is_file renamed &&
> + test_path_is_missing file
> +'
> +
> test_done
^ 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