* Re: [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Junio C Hamano @ 2016-11-11 21:27 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: git
In-Reply-To: <20161111201958.2175-1-dennis@kaarsemaker.net>
Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> No tests or documentation updates yet, and I'm not sure whether
> --follow-symlinks in other modes than --no-index should be supported, ignored
> (as it is now) or cause an error, but I'm leaning towards the third option.
My knee-jerk reaction is:
* The --no-index mode should default to your --follow-symlinks
behaviour, without any option to turn it on or off.
* If normal "diff" that follows symlinks by default has an option
to disable it, then it is OK to also add --no-follow-symlinks
that is only valid in the --no-index mode, so that we can mimick
it better (I do not think this is the case, though).
* Other modes should not follow symbolic links ever, no need for a
new option.
In any case, I'd advise you not to reroll this too quickly and
frequently until the end of this cycle. During a feature freeze, I
won't take new topics in 'pu' as that would add more things I need
to worry about, and if you reroll in too quick succession, it will
become harder to identify the latest set and queue after the
release.
Thanks.
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Johannes Sixt @ 2016-11-11 21:22 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Lars Schneider, Johannes Schindelin, git
In-Reply-To: <xmqqr36h209y.fsf@gitster.mtv.corp.google.com>
Am 11.11.2016 um 22:07 schrieb Junio C Hamano:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> OK, then let's have
>>
>> filter_git () {
>> rm -f rot13-filter.log &&
>> git "$@"
>> ...
>>
>> and call that -rc1.
>
> That is, to queue this on top of ls/filter-process and merge it
> down.
>
> -- >8 --
> Subject: t0021: remove debugging cruft
>
> The redirection of the standard error stream to a temporary file is
> a leftover cruft during debugging. Remove it.
>
> Besides, it is reported by folks on the Windows that the test is
> flaky with this redirection; somebody gets confused and this
> merely-redirected-to file gets marked as delete-pending by git.exe
> and makes it finish with a non-zero exit status when "git checkout"
> finishes. Windows folks may want to figure that one out, but for
> the purpose of this test, it shouldn't become a show-stopper.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Nicely summarized! Thank you very much!
I've double-checked again that the redirection is really not used by the
tests. The name does occur in a .gitignore, but I guess that is not
important for the background filter process. It would be good if Lars
could confirm this.
> ---
> t/t0021-conversion.sh | 3 +--
> 1 file changed, 1 insertion(+), 2 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index cb72fa49de..5ce2535017 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -27,8 +27,7 @@ file_size () {
>
> filter_git () {
> rm -f rot13-filter.log &&
> - git "$@" 2>git-stderr.log &&
> - rm -f git-stderr.log
> + git "$@"
> }
>
> # Compare two files and ensure that `clean` and `smudge` respectively are
>
^ permalink raw reply
* Re: [PATCH v2] t6026: ensure that long-running script really is
From: Johannes Sixt @ 2016-11-11 21:15 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, Andreas Schwab
In-Reply-To: <xmqqmvh5207k.fsf@gitster.mtv.corp.google.com>
Am 11.11.2016 um 22:09 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Am 11.11.2016 um 21:48 schrieb Junio C Hamano:
>>> Johannes Sixt <j6t@kdbg.org> writes:
>>>
>>>> Good point. Here is an updated version.
>>>
>>> Unfortunately, I already took the version before this one and
>>> started my integration cycle today. I'll wiggle this in; it
>>> essentially is about adding a big comment to explain what is going
>>> on and then moving the when-finished down below it, right?
>>
>> Correct.
>
> Thanks. It has become like so:
>
> -- >8 --
> From: Johannes Sixt <j6t@kdbg.org>
> Date: Fri, 11 Nov 2016 21:24:44 +0100
> Subject: [PATCH] t6026: clarify the point of "kill $(cat sleep.pid)"
>
> We lengthened the time the leftover process sleeps in the previous
> commit to make sure it will be there while 'git merge' runs and
> finishes. It therefore needs to be killed before leaving the test.
> And it needs to be killed even when 'git merge' fails, so it has to
> be triggered via test_when_finished mechanism.
>
> Explain all that in a large comment, and move the use site of
> test_when_finished to immediately before 'git merge' invocation,
> where the process is spawned.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
Perfect, thank you very much.
> ---
> t/t6026-merge-attr.sh | 10 +++++++++-
> 1 file changed, 9 insertions(+), 1 deletion(-)
>
> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
> index 348d78b205..8f9b48a493 100755
> --- a/t/t6026-merge-attr.sh
> +++ b/t/t6026-merge-attr.sh
> @@ -187,12 +187,20 @@ test_expect_success 'custom merge does not lock index' '
> sleep 3600 &
> echo $! >sleep.pid
> EOF
> - test_when_finished "kill \$(cat sleep.pid)" &&
>
> test_write_lines >.gitattributes \
> "* merge=ours" "text merge=sleep-an-hour" &&
> test_config merge.ours.driver true &&
> test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
> +
> + # We are testing that the custom merge driver does not block
> + # index.lock on Windows due to an inherited file handle.
> + # To ensure that the backgrounded process ran sufficiently
> + # long (and has been started in the first place), we do not
> + # ignore the result of the kill command.
> + # By packaging the command in test_when_finished, we get both
> + # the correctness check and the clean-up.
> + test_when_finished "kill \$(cat sleep.pid)" &&
> git merge master
> '
>
>
^ permalink raw reply
* Re: [PATCH v2] t6026: ensure that long-running script really is
From: Junio C Hamano @ 2016-11-11 21:09 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Johannes Schindelin, git, Andreas Schwab
In-Reply-To: <ba2b3f08-87b1-c954-2020-e5a4bece7f83@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Am 11.11.2016 um 21:48 schrieb Junio C Hamano:
>> Johannes Sixt <j6t@kdbg.org> writes:
>>
>>> Good point. Here is an updated version.
>>
>> Unfortunately, I already took the version before this one and
>> started my integration cycle today. I'll wiggle this in; it
>> essentially is about adding a big comment to explain what is going
>> on and then moving the when-finished down below it, right?
>
> Correct.
Thanks. It has become like so:
-- >8 --
From: Johannes Sixt <j6t@kdbg.org>
Date: Fri, 11 Nov 2016 21:24:44 +0100
Subject: [PATCH] t6026: clarify the point of "kill $(cat sleep.pid)"
We lengthened the time the leftover process sleeps in the previous
commit to make sure it will be there while 'git merge' runs and
finishes. It therefore needs to be killed before leaving the test.
And it needs to be killed even when 'git merge' fails, so it has to
be triggered via test_when_finished mechanism.
Explain all that in a large comment, and move the use site of
test_when_finished to immediately before 'git merge' invocation,
where the process is spawned.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t6026-merge-attr.sh | 10 +++++++++-
1 file changed, 9 insertions(+), 1 deletion(-)
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 348d78b205..8f9b48a493 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -187,12 +187,20 @@ test_expect_success 'custom merge does not lock index' '
sleep 3600 &
echo $! >sleep.pid
EOF
- test_when_finished "kill \$(cat sleep.pid)" &&
test_write_lines >.gitattributes \
"* merge=ours" "text merge=sleep-an-hour" &&
test_config merge.ours.driver true &&
test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
+
+ # We are testing that the custom merge driver does not block
+ # index.lock on Windows due to an inherited file handle.
+ # To ensure that the backgrounded process ran sufficiently
+ # long (and has been started in the first place), we do not
+ # ignore the result of the kill command.
+ # By packaging the command in test_when_finished, we get both
+ # the correctness check and the clean-up.
+ test_when_finished "kill \$(cat sleep.pid)" &&
git merge master
'
--
2.11.0-rc0-149-gea84a1ce98
^ permalink raw reply related
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-11 21:07 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Lars Schneider, Johannes Schindelin, git
In-Reply-To: <xmqq1syh3fjx.fsf@gitster.mtv.corp.google.com>
Junio C Hamano <gitster@pobox.com> writes:
> OK, then let's have
>
> filter_git () {
> rm -f rot13-filter.log &&
> git "$@"
> ...
>
> and call that -rc1.
That is, to queue this on top of ls/filter-process and merge it
down.
-- >8 --
Subject: t0021: remove debugging cruft
The redirection of the standard error stream to a temporary file is
a leftover cruft during debugging. Remove it.
Besides, it is reported by folks on the Windows that the test is
flaky with this redirection; somebody gets confused and this
merely-redirected-to file gets marked as delete-pending by git.exe
and makes it finish with a non-zero exit status when "git checkout"
finishes. Windows folks may want to figure that one out, but for
the purpose of this test, it shouldn't become a show-stopper.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
t/t0021-conversion.sh | 3 +--
1 file changed, 1 insertion(+), 2 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index cb72fa49de..5ce2535017 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -27,8 +27,7 @@ file_size () {
filter_git () {
rm -f rot13-filter.log &&
- git "$@" 2>git-stderr.log &&
- rm -f git-stderr.log
+ git "$@"
}
# Compare two files and ensure that `clean` and `smudge` respectively are
^ permalink raw reply related
* Re: [PATCH v2] t6026: ensure that long-running script really is
From: Johannes Sixt @ 2016-11-11 20:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jeff King, Johannes Schindelin, git, Andreas Schwab
In-Reply-To: <xmqq60nt3fqc.fsf@gitster.mtv.corp.google.com>
Am 11.11.2016 um 21:48 schrieb Junio C Hamano:
> Johannes Sixt <j6t@kdbg.org> writes:
>
>> Good point. Here is an updated version.
>
> Unfortunately, I already took the version before this one and
> started my integration cycle today. I'll wiggle this in; it
> essentially is about adding a big comment to explain what is going
> on and then moving the when-finished down below it, right?
Correct.
Thank you very much!
-- Hannes
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-11 20:52 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Lars Schneider, Johannes Schindelin, git
In-Reply-To: <e0785e35-b43d-37ed-598d-b458daf3c355@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Good to know that I am not alone. This one fails consistently for
> me. I dug into it a bit today, but it drives me mad. Process Monitor
> reports that the redirected-to file (git-stderr.log) gets marked as
> "Delete pending" by git.exe, but I have absolutely no clue where this
> is coming from. Git.exe has no business fuzzing with the file; it just
> has to write() something into it, if at all. When 'git checkout' ends,
> the file is closed, and removed due to the "Delete pending" mark. At
> the same time, git.exe finishes with non-zero exit code. For whatever
> reason.
>
>> FWIW: This patch (which would be the right thing to do anyways) seems to fix
>> the flakyness but I can't be sure ... it needs to run longer...
>>
>> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
>> index 9ff5027..107766b 100755
>> --- a/t/t0021-conversion.sh
>> +++ b/t/t0021-conversion.sh
>> @@ -29,8 +29,7 @@ file_size () {
>>
>> filter_git () {
>> rm -f rot13-filter.log &&
>> - git "$@" 2>git-stderr.log &&
>> - rm -f git-stderr.log
>> + git "$@" 2>/dev/null
>
> If I remove the redirection altogether, the test passes for me
> consistently. The redirection isn't necessary anyway, it looks like a
> debugging left-over.
OK, then let's have
filter_git () {
rm -f rot13-filter.log &&
git "$@"
...
and call that -rc1; to Windows folks, this is sweeping a mysterious
failure around git.exe under the rug, but for the purpose of the
overall project, it is far lower priority to figure out the reason
why a merely-redirected-into file is considered to be delete-pending
on WIndows than have something the end user can start testing and
complaining about new features and regressions.
Thanks.
^ permalink raw reply
* Re: [PATCH v2] t6026: ensure that long-running script really is
From: Junio C Hamano @ 2016-11-11 20:48 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Jeff King, Johannes Schindelin, git, Andreas Schwab
In-Reply-To: <6a421222-a138-5647-4965-8ede24d904b2@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> Good point. Here is an updated version.
Unfortunately, I already took the version before this one and
started my integration cycle today. I'll wiggle this in; it
essentially is about adding a big comment to explain what is going
on and then moving the when-finished down below it, right?
Thanks.
> If the pid file is not created, the kill command receives no
> arguments. Here on Linux, it reports failure in this case.
> This is good. I could have said
>
> test_when_finished "kill \"\$(cat sleep.pid)\""
>
> but that is a bit too much quoting for my taste when it is
> not strictly necessary.
>
> t/t6026-merge-attr.sh | 18 +++++++++++++-----
> 1 file changed, 13 insertions(+), 5 deletions(-)
>
> diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
> index 7a6e33e673..8f9b48a493 100755
> --- a/t/t6026-merge-attr.sh
> +++ b/t/t6026-merge-attr.sh
> @@ -183,16 +183,24 @@ test_expect_success 'up-to-date merge without common ancestor' '
>
> test_expect_success 'custom merge does not lock index' '
> git reset --hard anchor &&
> - write_script sleep-one-second.sh <<-\EOF &&
> - sleep 1 &
> + write_script sleep-an-hour.sh <<-\EOF &&
> + sleep 3600 &
> echo $! >sleep.pid
> EOF
> - test_when_finished "kill \$(cat sleep.pid)" &&
>
> test_write_lines >.gitattributes \
> - "* merge=ours" "text merge=sleep-one-second" &&
> + "* merge=ours" "text merge=sleep-an-hour" &&
> test_config merge.ours.driver true &&
> - test_config merge.sleep-one-second.driver ./sleep-one-second.sh &&
> + test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
> +
> + # We are testing that the custom merge driver does not block
> + # index.lock on Windows due to an inherited file handle.
> + # To ensure that the backgrounded process ran sufficiently
> + # long (and has been started in the first place), we do not
> + # ignore the result of the kill command.
> + # By packaging the command in test_when_finished, we get both
> + # the correctness check and the clean-up.
> + test_when_finished "kill \$(cat sleep.pid)" &&
> git merge master
> '
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Johannes Sixt @ 2016-11-11 20:44 UTC (permalink / raw)
To: Lars Schneider; +Cc: Johannes Schindelin, Junio C Hamano, git
In-Reply-To: <C3840B91-58E2-4563-9597-5EE514D97CC1@gmail.com>
Am 11.11.2016 um 18:38 schrieb Lars Schneider:
>> On 11 Nov 2016, at 18:31, Lars Schneider <larsxschneider@gmail.com> wrote:
>>> On 11 Nov 2016, at 18:05, Lars Schneider <larsxschneider@gmail.com> wrote:
>>> @Dscho:
>>> There is still one remaining new issue with t0021 ... investigating!
>>
>> "17 - required process filter should be used only for "clean" operation only"
>> seems flaky.
>>
>> I don't have any more time today but I will look into it tomorrow.
Good to know that I am not alone. This one fails consistently for me. I
dug into it a bit today, but it drives me mad. Process Monitor reports
that the redirected-to file (git-stderr.log) gets marked as "Delete
pending" by git.exe, but I have absolutely no clue where this is coming
from. Git.exe has no business fuzzing with the file; it just has to
write() something into it, if at all. When 'git checkout' ends, the file
is closed, and removed due to the "Delete pending" mark. At the same
time, git.exe finishes with non-zero exit code. For whatever reason.
> FWIW: This patch (which would be the right thing to do anyways) seems to fix
> the flakyness but I can't be sure ... it needs to run longer...
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 9ff5027..107766b 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -29,8 +29,7 @@ file_size () {
>
> filter_git () {
> rm -f rot13-filter.log &&
> - git "$@" 2>git-stderr.log &&
> - rm -f git-stderr.log
> + git "$@" 2>/dev/null
If I remove the redirection altogether, the test passes for me
consistently. The redirection isn't necessary anyway, it looks like a
debugging left-over.
> }
>
> # Compare two files and ensure that `clean` and `smudge` respectively are
>
>
-- Hannes
^ permalink raw reply
* [PATCH v2] t6026: ensure that long-running script really is
From: Johannes Sixt @ 2016-11-11 20:24 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Johannes Schindelin, git, Andreas Schwab
In-Reply-To: <20161111084148.tgtsijn74z2pdylq@sigill.intra.peff.net>
When making sure that background tasks are cleaned up in 5babb5b
(t6026-merge-attr: clean up background process at end of test case,
2016-09-07), we considered to let the background task sleep longer, just
to be certain that it will still be running when we want to kill it
after the test.
Sadly, the assumption appears not to hold true that the test case passes
quickly enough to kill the background task within a second.
Simply increase it to an hour. No system can be possibly slow enough to
make above-mentioned assumption incorrect.
Reported by Andreas Schwab. In-code comments by J6t.
Signed-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 11.11.2016 um 09:41 schrieb Jeff King:
> But the other thing the "kill" is doing is make sure we clean up after
> ourselves, even if another part of the test fails.
Good point. Here is an updated version.
If the pid file is not created, the kill command receives no
arguments. Here on Linux, it reports failure in this case.
This is good. I could have said
test_when_finished "kill \"\$(cat sleep.pid)\""
but that is a bit too much quoting for my taste when it is
not strictly necessary.
t/t6026-merge-attr.sh | 18 +++++++++++++-----
1 file changed, 13 insertions(+), 5 deletions(-)
diff --git a/t/t6026-merge-attr.sh b/t/t6026-merge-attr.sh
index 7a6e33e673..8f9b48a493 100755
--- a/t/t6026-merge-attr.sh
+++ b/t/t6026-merge-attr.sh
@@ -183,16 +183,24 @@ test_expect_success 'up-to-date merge without common ancestor' '
test_expect_success 'custom merge does not lock index' '
git reset --hard anchor &&
- write_script sleep-one-second.sh <<-\EOF &&
- sleep 1 &
+ write_script sleep-an-hour.sh <<-\EOF &&
+ sleep 3600 &
echo $! >sleep.pid
EOF
- test_when_finished "kill \$(cat sleep.pid)" &&
test_write_lines >.gitattributes \
- "* merge=ours" "text merge=sleep-one-second" &&
+ "* merge=ours" "text merge=sleep-an-hour" &&
test_config merge.ours.driver true &&
- test_config merge.sleep-one-second.driver ./sleep-one-second.sh &&
+ test_config merge.sleep-an-hour.driver ./sleep-an-hour.sh &&
+
+ # We are testing that the custom merge driver does not block
+ # index.lock on Windows due to an inherited file handle.
+ # To ensure that the backgrounded process ran sufficiently
+ # long (and has been started in the first place), we do not
+ # ignore the result of the kill command.
+ # By packaging the command in test_when_finished, we get both
+ # the correctness check and the clean-up.
+ test_when_finished "kill \$(cat sleep.pid)" &&
git merge master
'
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* Re: [RFC] Add way to make Git credentials accessible from clean/smudge filter
From: Jeff King @ 2016-11-11 20:27 UTC (permalink / raw)
To: Dennis Kaarsemaker; +Cc: Lars Schneider, Matthieu Moy, git, me
In-Reply-To: <1478894572.12153.1.camel@kaarsemaker.net>
On Fri, Nov 11, 2016 at 09:02:52PM +0100, Dennis Kaarsemaker wrote:
> > > Are you sure about that? If I do:
> > >
> > > echo url=https://example.com/repo.git |
> > > git credential fill
> > >
> > > I get prompted for a username and password.
> >
> >
> > Hm.. either I don't understand you or I expressed myself unclear.
> >
> > Let's say a user runs:
> >
> > $ git clone https://myrepo.git
> >
> > If no credential helper is setup, then Git asks the user for credentials.
> > Afterwards Git starts downloading stuff. At some point Git will run my
> > smudge filter on some files and in my case the smudge filter needs the
> > Git credentials. AFAIK, the smudge filter has no way to get the credentials
> > from Git at this point - not even by invoking "git credential".
> > Is this correct?
>
> I think that's correct, but the same argument goes both ways: unless I
> use a credential helper, or explicitely give a filter application my
> credentials, I don't want a helper to be able to get to those
> credentials. I'd consider that a security bug.
Yeah, agreed. They are logically two separate operations, so I think it
is a feature that they do not implicitly share credentials.
I think the only place where we implicitly share credentials is when
serving an HTTP fetch or push requires multiple HTTP requests. And there
it seems pretty sane to do so.
-Peff
^ permalink raw reply
* [PATCH 1/2] diff --no-index: add option to follow symlinks
From: Dennis Kaarsemaker @ 2016-11-11 20:19 UTC (permalink / raw)
To: git; +Cc: Dennis Kaarsemaker
In-Reply-To: <20161111201958.2175-1-dennis@kaarsemaker.net>
Git's diff machinery does not follow symlinks, which makes sense as git
itself also does not, but stores the symlink destination.
In --no-index mode however, it is useful for diff to be able to follow
symlinks, matching the behaviour of ordinary diff.
Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
diff-no-index.c | 7 ++++---
diff.c | 10 ++++++++--
diff.h | 2 +-
3 files changed, 13 insertions(+), 6 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index f420786..15811c2 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -40,7 +40,7 @@ static int read_directory_contents(const char *path, struct string_list *list)
*/
static const char file_from_standard_input[] = "-";
-static int get_mode(const char *path, int *mode)
+static int get_mode(const char *path, int *mode, int follow_symlinks)
{
struct stat st;
@@ -52,7 +52,7 @@ static int get_mode(const char *path, int *mode)
#endif
else if (path == file_from_standard_input)
*mode = create_ce_mode(0666);
- else if (lstat(path, &st))
+ else if (follow_symlinks ? stat(path, &st) : lstat(path, &st))
return error("Could not access '%s'", path);
else
*mode = st.st_mode;
@@ -93,7 +93,8 @@ static int queue_diff(struct diff_options *o,
{
int mode1 = 0, mode2 = 0;
- if (get_mode(name1, &mode1) || get_mode(name2, &mode2))
+ if (get_mode(name1, &mode1, DIFF_OPT_TST(o, FOLLOW_SYMLINKS)) ||
+ get_mode(name2, &mode2, DIFF_OPT_TST(o, FOLLOW_SYMLINKS)))
return -1;
if (mode1 && mode2 && S_ISDIR(mode1) != S_ISDIR(mode2)) {
diff --git a/diff.c b/diff.c
index be11e4e..1eaf604 100644
--- a/diff.c
+++ b/diff.c
@@ -2815,7 +2815,7 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
s->size = xsize_t(st.st_size);
if (!s->size)
goto empty;
- if (S_ISLNK(st.st_mode)) {
+ if (S_ISLNK(s->mode)) {
struct strbuf sb = STRBUF_INIT;
if (strbuf_readlink(&sb, s->path, s->size))
@@ -2825,6 +2825,10 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
s->should_free = 1;
return 0;
}
+ if (S_ISLNK(st.st_mode)) {
+ stat(s->path, &st);
+ s->size = xsize_t(st.st_size);
+ }
if (size_only)
return 0;
if ((flags & CHECK_BINARY) &&
@@ -3884,7 +3888,9 @@ int diff_opt_parse(struct diff_options *options,
else if (!strcmp(arg, "--no-follow")) {
DIFF_OPT_CLR(options, FOLLOW_RENAMES);
DIFF_OPT_CLR(options, DEFAULT_FOLLOW_RENAMES);
- } else if (!strcmp(arg, "--color"))
+ } else if (!strcmp(arg, "--follow-symlinks"))
+ DIFF_OPT_SET(options, FOLLOW_SYMLINKS);
+ else if (!strcmp(arg, "--color"))
options->use_color = 1;
else if (skip_prefix(arg, "--color=", &arg)) {
int value = git_config_colorbool(NULL, arg);
diff --git a/diff.h b/diff.h
index 25ae60d..22b0c5a 100644
--- a/diff.h
+++ b/diff.h
@@ -69,7 +69,7 @@ typedef struct strbuf *(*diff_prefix_fn_t)(struct diff_options *opt, void *data)
#define DIFF_OPT_FIND_COPIES_HARDER (1 << 6)
#define DIFF_OPT_FOLLOW_RENAMES (1 << 7)
#define DIFF_OPT_RENAME_EMPTY (1 << 8)
-/* (1 << 9) unused */
+#define DIFF_OPT_FOLLOW_SYMLINKS (1 << 9)
#define DIFF_OPT_HAS_CHANGES (1 << 10)
#define DIFF_OPT_QUICK (1 << 11)
#define DIFF_OPT_NO_INDEX (1 << 12)
--
2.10.1-449-gab0f84c
^ permalink raw reply related
* [PATCH 2/2] diff --no-index: support reading from pipes
From: Dennis Kaarsemaker @ 2016-11-11 20:19 UTC (permalink / raw)
To: git; +Cc: Dennis Kaarsemaker
In-Reply-To: <20161111201958.2175-1-dennis@kaarsemaker.net>
diff <(command1) <(command2) provides useful output, let's make it
possible for git to do the same.
Signed-off-by: Dennis Kaarsemaker <dennis@kaarsemaker.net>
---
diff-no-index.c | 8 ++++++++
diff.c | 13 +++++++++++--
2 files changed, 19 insertions(+), 2 deletions(-)
diff --git a/diff-no-index.c b/diff-no-index.c
index 15811c2..487dbf5 100644
--- a/diff-no-index.c
+++ b/diff-no-index.c
@@ -83,6 +83,14 @@ static struct diff_filespec *noindex_filespec(const char *name, int mode)
name = "/dev/null";
s = alloc_filespec(name);
fill_filespec(s, null_sha1, 0, mode);
+ /*
+ * In --no-index mode, we support reading from pipes. canon_mode, called by
+ * fill_filespec, gets confused by this and thinks we now have subprojects.
+ * Detect this and tell the rest of the diff machinery to treat pipes as
+ * normal files.
+ */
+ if (S_ISGITLINK(s->mode))
+ s->mode = S_IFREG | ce_permissions(mode);
if (name == file_from_standard_input)
populate_from_stdin(s);
return s;
diff --git a/diff.c b/diff.c
index 1eaf604..c599efb 100644
--- a/diff.c
+++ b/diff.c
@@ -2839,9 +2839,18 @@ int diff_populate_filespec(struct diff_filespec *s, unsigned int flags)
fd = open(s->path, O_RDONLY);
if (fd < 0)
goto err_empty;
- s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+ if (!S_ISREG(st.st_mode)) {
+ struct strbuf sb = STRBUF_INIT;
+ strbuf_read(&sb, fd, 0);
+ s->size = sb.len;
+ s->data = strbuf_detach(&sb, NULL);
+ s->should_free = 1;
+ }
+ else {
+ s->data = xmmap(NULL, s->size, PROT_READ, MAP_PRIVATE, fd, 0);
+ s->should_munmap = 1;
+ }
close(fd);
- s->should_munmap = 1;
/*
* Convert from working tree format to canonical git format
--
2.10.1-449-gab0f84c
^ permalink raw reply related
* [RFC/PATCH 0/2] git diff <(command1) <(command2)
From: Dennis Kaarsemaker @ 2016-11-11 20:19 UTC (permalink / raw)
To: git; +Cc: Dennis Kaarsemaker
Today on #git, a user asked why git diff <(command1) <(command2) gave only some
gibberish about pipes as output. The answer is fairly simple: git diff gets as
arguments /dev/fd/62 and /dev/fd/63, which are symlinks. So git simply
readlink()s them and gets pipe:[123456] as destination of that link which it
then outputs.
Given that 'normal' diff provides arguably better output in this case (a diff
of the output of the two commands), I wanted to look at what it would take for
git to handle this. Surprisingly: not much. 1/2 adds support for
--follow-symlinks to git diff --no-index (and only the --no-index variant) and
2/2 adds support for reading from pipes.
No tests or documentation updates yet, and I'm not sure whether
--follow-symlinks in other modes than --no-index should be supported, ignored
(as it is now) or cause an error, but I'm leaning towards the third option.
Dennis Kaarsemaker (2):
diff --no-index: add option to follow symlinks
diff --no-index: support reading from pipes
diff-no-index.c | 15 ++++++++++++---
diff.c | 23 +++++++++++++++++++----
diff.h | 2 +-
3 files changed, 32 insertions(+), 8 deletions(-)
--
2.10.1-449-gab0f84c
^ permalink raw reply
* Re: [RFC] Add way to make Git credentials accessible from clean/smudge filter
From: Dennis Kaarsemaker @ 2016-11-11 20:02 UTC (permalink / raw)
To: Lars Schneider, Jeff King; +Cc: Matthieu Moy, git, me
In-Reply-To: <4060C484-B42E-42AB-BB42-4753373F0E7F@gmail.com>
On Fri, 2016-11-11 at 10:40 +0100, Lars Schneider wrote:
> On 11 Nov 2016, at 10:31, Jeff King <peff@peff.net> wrote:
>
> > On Fri, Nov 11, 2016 at 10:28:56AM +0100, Lars Schneider wrote:
> >
> > > > Yeah, that is the solution I was going to suggest. The credentials are
> > > > totally orthogonal to the filters, and I would rather not shove them
> > > > into the protocol. It's an extra process, but with the new multi-use
> > > > smudge filter, it's one per git invocation, not one per file.
> > >
> > >
> > > The trouble with "git credential" is that it works only if the credential
> > > helper is setup correctly. Although I assume that most people have setup this,
> > > I have also worked with a number of people who prefer to enter their passwords
> > > every time Git makes a network connection.
> >
> >
> > Are you sure about that? If I do:
> >
> > echo url=https://example.com/repo.git |
> > git credential fill
> >
> > I get prompted for a username and password.
>
>
> Hm.. either I don't understand you or I expressed myself unclear.
>
> Let's say a user runs:
>
> $ git clone https://myrepo.git
>
> If no credential helper is setup, then Git asks the user for credentials.
> Afterwards Git starts downloading stuff. At some point Git will run my
> smudge filter on some files and in my case the smudge filter needs the
> Git credentials. AFAIK, the smudge filter has no way to get the credentials
> from Git at this point - not even by invoking "git credential".
> Is this correct?
I think that's correct, but the same argument goes both ways: unless I
use a credential helper, or explicitely give a filter application my
credentials, I don't want a helper to be able to get to those
credentials. I'd consider that a security bug.
D.
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Junio C Hamano @ 2016-11-11 18:55 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Johannes Schindelin, git, Jeff King
In-Reply-To: <2b69d098-92ef-77b0-367a-516e9edbe257@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
> We have to use $PWD instead of $(pwd) because on Windows the latter
> would add a C: style path to bash's Unix-style $PATH variable, which
> becomes confused by the colon after the drive letter. ($PWD is a
> Unix-style path.)
>
> In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
> assembles a Unix-style path list with the colon as separators. It
> converts the value to a Windows-style path list with the semicolon as
> path separator when it forwards the variable to git.exe. The same
> confusion happens when bash's original value is contaminated with
> Windows style paths.
>
> Signed-off-by: Johannes Sixt <j6t@kdbg.org>
> ---
> Am 11.11.2016 um 18:11 schrieb Johannes Sixt:
>> Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
>>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>> ...
>
> When the MSYS program such as bash invokes a non-MSYS program, it
> translates the Unix-style paths in arguments and environment variables
> to Windows stlye. We only have to ensure that we inject only Unix-style
> paths in these places so as not to confuse the conversion algorithm.
> Most of the time, we do not have to worry.
>
> On the other hand, when we write a path to a file that git.exe consumes
> or receive a path from git.exe, i.e., when the path travels through
> stdout and stdin, no automatic translation happens (which is quite
> understandable), and we have do the translation explicitly. An example
> for such a case is when we write a .git/info/alternates file via the
> shell.
>
>> A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a
>> moment.
>
> Here it is. I had proposed the t0021 part earlier, but it fell through
> the cracks during the temporary maintainer change.
Thanks. Dscho, does this fix both of these issues to you?
> t/t0021-conversion.sh | 2 +-
> t/t5615-alternate-env.sh | 4 ++--
> 2 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
> index 9ff502773d..b93cd44546 100755
> --- a/t/t0021-conversion.sh
> +++ b/t/t0021-conversion.sh
> @@ -4,7 +4,7 @@ test_description='blob conversion via gitattributes'
>
> . ./test-lib.sh
>
> -TEST_ROOT="$(pwd)"
> +TEST_ROOT="$PWD"
> PATH=$TEST_ROOT:$PATH
>
> write_script <<\EOF "$TEST_ROOT/rot13.sh"
> diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
> index 22d9d8178b..eec4137ca5 100755
> --- a/t/t5615-alternate-env.sh
> +++ b/t/t5615-alternate-env.sh
> @@ -31,14 +31,14 @@ test_expect_success 'objects inaccessible without alternates' '
> '
>
> test_expect_success 'access alternate via absolute path' '
> - check_obj "$(pwd)/one.git/objects" <<-EOF
> + check_obj "$PWD/one.git/objects" <<-EOF
> $one blob
> $two missing
> EOF
> '
>
> test_expect_success 'access multiple alternates' '
> - check_obj "$(pwd)/one.git/objects:$(pwd)/two.git/objects" <<-EOF
> + check_obj "$PWD/one.git/objects:$PWD/two.git/objects" <<-EOF
> $one blob
> $two blob
> EOF
^ permalink raw reply
* Re: [git-for-windows] [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: Johannes Schindelin @ 2016-11-11 18:37 UTC (permalink / raw)
To: Lars Schneider; +Cc: git-for-windows, Git Mailing List, me
In-Reply-To: <F2B1E33E-3C31-45A3-8A5B-098A59CD399D@gmail.com>
Hi Lars,
On Fri, 11 Nov 2016, Lars Schneider wrote:
>
> On 10 Nov 2016, at 22:39, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>
> > Hi Lars,
> >
> > On Wed, 9 Nov 2016, Lars Schneider wrote:
> >
> >> On 05 Nov 2016, at 10:50, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
> >>
> >>> I finally got around to rebase the Windows-specific patches (which seem to
> >>> not make it upstream as fast as we get new ones) on top of upstream Git
> >>> v2.11.0-rc0, and to bundle installers, portable Git and MinGit [*1*]:
> >>>
> >>> https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.1
> >>
> >>
> >> I tested a new feature in 2.11 on Windows today and it failed. After some
> >> confusion I realized that the feature is not on your 2.11 branch.
> >
> > Oops. That must have been a major snafu on my side, very sorry for that.
> >
> > I just tagged v2.11.0-rc0.windows.2 in https://github.com/dscho/git and
> > will make a new prerelease tomorrow.
>
> No worries! I am looking forward to the new release!
>
> For some reason my own build of Git for Windows cannot use HTTPS properly.
> I need to debug this further at some point.
Here it is:
https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.2
Thanks for catching my snafu,
Dscho
^ permalink raw reply
* Re: [ANNOUNCE] Prerelease: Git for Windows v2.11.0-rc0
From: Johannes Schindelin @ 2016-11-11 18:33 UTC (permalink / raw)
To: stefan.naewe; +Cc: git-for-windows, git
In-Reply-To: <alpine.DEB.2.20.1611111750580.3746@virtualbox>
Hi Stefan,
On Fri, 11 Nov 2016, Johannes Schindelin wrote:
> Will keep you posted,
I published the prerelease:
https://github.com/git-for-windows/git/releases/tag/v2.11.0-rc0.windows.2
Thanks,
Dscho
^ permalink raw reply
* Re: [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Jeff King @ 2016-11-11 18:16 UTC (permalink / raw)
To: Johannes Sixt; +Cc: Junio C Hamano, Johannes Schindelin, git
In-Reply-To: <2b69d098-92ef-77b0-367a-516e9edbe257@kdbg.org>
On Fri, Nov 11, 2016 at 06:31:48PM +0100, Johannes Sixt wrote:
> We have to use $PWD instead of $(pwd) because on Windows the latter
> would add a C: style path to bash's Unix-style $PATH variable, which
> becomes confused by the colon after the drive letter. ($PWD is a
> Unix-style path.)
>
> In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
> assembles a Unix-style path list with the colon as separators. It
> converts the value to a Windows-style path list with the semicolon as
> path separator when it forwards the variable to git.exe. The same
> confusion happens when bash's original value is contaminated with
> Windows style paths.
So on reading your original I wondered why you did not need to use the
";", and that explains it. But wow, it's subtle.
I don't know what people typically write in the wild, and if it's worth
actually testing explicitly the ";" case. I'll leave that to Windows
people to debate.
-Peff
^ permalink raw reply
* Re: [PATCH] mingw: hot-fix t5615
From: Jeff King @ 2016-11-11 18:12 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: git, Junio C Hamano
In-Reply-To: <f4336a7fa3df8c0b426c3d01ada8d7d8f38e588a.1478881741.git.johannes.schindelin@gmx.de>
On Fri, Nov 11, 2016 at 05:29:33PM +0100, Johannes Schindelin wrote:
> That test made the incorrect assumption that the path separator character
> is always a colon. On Windows, it is a semicolon instead.
Oof, sorry about that. I remember being careful about the ";" while
doing the original alt-odb work, but forgot about it while making the
recent fix in 37a95862c.
The patch itself looks like obviously correct (or at least obviously in
the right direction, since it sounds like more is needed).
-Peff
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Junio C Hamano @ 2016-11-11 17:41 UTC (permalink / raw)
To: Lars Schneider; +Cc: Johannes Schindelin, git
In-Reply-To: <4FF1825D-F699-4532-9B53-A673DFF00D8B@gmail.com>
Lars Schneider <larsxschneider@gmail.com> writes:
> Part of the reason is that Hannes' squash got lost:
> http://public-inbox.org/git/d36d8b51-f2d7-a2f5-89ea-369f49556e10@kdbg.org/
>
> @Junio: Can you apply this (see discussion with Peff linked to email above).
Thanks for a quick and usable report followed by a quick fix.
^ permalink raw reply
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Lars Schneider @ 2016-11-11 17:38 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <89DB812E-EC29-4D12-97EA-6E8FF6F86FE5@gmail.com>
> On 11 Nov 2016, at 18:31, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>>
>> On 11 Nov 2016, at 18:05, Lars Schneider <larsxschneider@gmail.com> wrote:
>>
>>
>>> On 11 Nov 2016, at 17:13, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>>>
>>> Hi Junio,
>>>
>>> On Thu, 10 Nov 2016, Junio C Hamano wrote:
>>>
>>>> Junio C Hamano <gitster@pobox.com> writes:
>>>>
>>>>> I'll report back an updated schedule when able.
>>>>
>>>> I pushed some updates out on 'master' today.
>>>
>>> Which means that t0021 is now broken also on `master` when running in Git
>>> for Windows' SDK.
>>
>> Part of the reason is that Hannes' squash got lost:
>> http://public-inbox.org/git/d36d8b51-f2d7-a2f5-89ea-369f49556e10@kdbg.org/
>>
>> @Junio: Can you apply this (see discussion with Peff linked to email above).
>>
>> @Dscho:
>> There is still one remaining new issue with t0021 ... investigating!
>
> "17 - required process filter should be used only for "clean" operation only"
> seems flaky.
>
> I don't have any more time today but I will look into it tomorrow.
FWIW: This patch (which would be the right thing to do anyways) seems to fix
the flakyness but I can't be sure ... it needs to run longer...
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9ff5027..107766b 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -29,8 +29,7 @@ file_size () {
filter_git () {
rm -f rot13-filter.log &&
- git "$@" 2>git-stderr.log &&
- rm -f git-stderr.log
+ git "$@" 2>/dev/null
}
# Compare two files and ensure that `clean` and `smudge` respectively are
^ permalink raw reply related
* [PATCH] t0021, t5615: use $PWD instead of $(pwd) in PATH-like shell variables
From: Johannes Sixt @ 2016-11-11 17:31 UTC (permalink / raw)
To: Junio C Hamano, Johannes Schindelin; +Cc: git, Jeff King
In-Reply-To: <fa11def5-a878-a5c8-6e7b-627ea8cda7e2@kdbg.org>
We have to use $PWD instead of $(pwd) because on Windows the latter
would add a C: style path to bash's Unix-style $PATH variable, which
becomes confused by the colon after the drive letter. ($PWD is a
Unix-style path.)
In the case of GIT_ALTERNATE_OBJECT_DIRECTORIES, bash on Windows
assembles a Unix-style path list with the colon as separators. It
converts the value to a Windows-style path list with the semicolon as
path separator when it forwards the variable to git.exe. The same
confusion happens when bash's original value is contaminated with
Windows style paths.
Signed-off-by: Johannes Sixt <j6t@kdbg.org>
---
Am 11.11.2016 um 18:11 schrieb Johannes Sixt:
> Am 11.11.2016 um 18:06 schrieb Junio C Hamano:
>> Johannes Schindelin <johannes.schindelin@gmx.de> writes:
>>
>>> That test made the incorrect assumption that the path separator
>>> character
>>> is always a colon. On Windows, it is a semicolon instead.
>>
>> Documentation/git.txt says that GIT_ALTERNATE_OBJECT_DIRECTORIES is
>> separated with ";" on Windows fairly clearly, and we should have
>> caught that.
>>
>> For the upcoming release there is no need for any further tweak on
>> your fix I am responding to, but in the longer term we would want to
>> turn this to path_sep=";" (or ":") and define it in the global
>> t/test-lib.sh, as it is plausible that we may want to prepend or
>> append to $PATH in the tests and that also needs ";" on Windows, no?
When the MSYS program such as bash invokes a non-MSYS program, it
translates the Unix-style paths in arguments and environment variables
to Windows stlye. We only have to ensure that we inject only Unix-style
paths in these places so as not to confuse the conversion algorithm.
Most of the time, we do not have to worry.
On the other hand, when we write a path to a file that git.exe consumes
or receive a path from git.exe, i.e., when the path travels through
stdout and stdin, no automatic translation happens (which is quite
understandable), and we have do the translation explicitly. An example
for such a case is when we write a .git/info/alternates file via the
shell.
> A simpler fix is to use $PWD instead of $(pwd). I'll submit a patch in a
> moment.
Here it is. I had proposed the t0021 part earlier, but it fell through
the cracks during the temporary maintainer change.
t/t0021-conversion.sh | 2 +-
t/t5615-alternate-env.sh | 4 ++--
2 files changed, 3 insertions(+), 3 deletions(-)
diff --git a/t/t0021-conversion.sh b/t/t0021-conversion.sh
index 9ff502773d..b93cd44546 100755
--- a/t/t0021-conversion.sh
+++ b/t/t0021-conversion.sh
@@ -4,7 +4,7 @@ test_description='blob conversion via gitattributes'
. ./test-lib.sh
-TEST_ROOT="$(pwd)"
+TEST_ROOT="$PWD"
PATH=$TEST_ROOT:$PATH
write_script <<\EOF "$TEST_ROOT/rot13.sh"
diff --git a/t/t5615-alternate-env.sh b/t/t5615-alternate-env.sh
index 22d9d8178b..eec4137ca5 100755
--- a/t/t5615-alternate-env.sh
+++ b/t/t5615-alternate-env.sh
@@ -31,14 +31,14 @@ test_expect_success 'objects inaccessible without alternates' '
'
test_expect_success 'access alternate via absolute path' '
- check_obj "$(pwd)/one.git/objects" <<-EOF
+ check_obj "$PWD/one.git/objects" <<-EOF
$one blob
$two missing
EOF
'
test_expect_success 'access multiple alternates' '
- check_obj "$(pwd)/one.git/objects:$(pwd)/two.git/objects" <<-EOF
+ check_obj "$PWD/one.git/objects:$PWD/two.git/objects" <<-EOF
$one blob
$two blob
EOF
--
2.11.0.rc0.55.gd967357
^ permalink raw reply related
* Re: 2.11.0-rc1 will not be tagged for a few days
From: Lars Schneider @ 2016-11-11 17:31 UTC (permalink / raw)
To: Johannes Schindelin; +Cc: Junio C Hamano, git
In-Reply-To: <4FF1825D-F699-4532-9B53-A673DFF00D8B@gmail.com>
> On 11 Nov 2016, at 18:05, Lars Schneider <larsxschneider@gmail.com> wrote:
>
>
>> On 11 Nov 2016, at 17:13, Johannes Schindelin <johannes.schindelin@gmx.de> wrote:
>>
>> Hi Junio,
>>
>> On Thu, 10 Nov 2016, Junio C Hamano wrote:
>>
>>> Junio C Hamano <gitster@pobox.com> writes:
>>>
>>>> I'll report back an updated schedule when able.
>>>
>>> I pushed some updates out on 'master' today.
>>
>> Which means that t0021 is now broken also on `master` when running in Git
>> for Windows' SDK.
>
> Part of the reason is that Hannes' squash got lost:
> http://public-inbox.org/git/d36d8b51-f2d7-a2f5-89ea-369f49556e10@kdbg.org/
>
> @Junio: Can you apply this (see discussion with Peff linked to email above).
>
> @Dscho:
> There is still one remaining new issue with t0021 ... investigating!
"17 - required process filter should be used only for "clean" operation only"
seems flaky.
I don't have any more time today but I will look into it tomorrow.
- Lars
^ permalink raw reply
* [PATCH v2] upload-pack: Optionally allow fetching any sha1
From: David Turner @ 2016-11-11 17:23 UTC (permalink / raw)
To: git, spearce; +Cc: David Turner
It seems a little silly to do a reachabilty check in the case where we
trust the user to access absolutely everything in the repository.
Also, it's racy in a distributed system -- perhaps one server
advertises a ref, but another has since had a force-push to that ref,
and perhaps the two HTTP requests end up directed to these different
servers.
Signed-off-by: David Turner <dturner@twosigma.com>
---
Actually, I realized as forgot the doco. Sorry.
Documentation/config.txt | 5 +++++
Documentation/git-fetch-pack.txt | 6 +++---
t/t5551-http-fetch-smart.sh | 22 ++++++++++++++++++++++
upload-pack.c | 10 +++++++++-
4 files changed, 39 insertions(+), 4 deletions(-)
diff --git a/Documentation/config.txt b/Documentation/config.txt
index a0ab66a..b7f9991 100644
--- a/Documentation/config.txt
+++ b/Documentation/config.txt
@@ -2961,6 +2961,11 @@ uploadpack.allowReachableSHA1InWant::
calculating object reachability is computationally expensive.
Defaults to `false`.
+uploadpack.allowAnySHA1InWant::
+ Allow `upload-pack` to accept a fetch request that asks for any
+ object at all.
+ Defaults to `false`.
+
uploadpack.keepAlive::
When `upload-pack` has started `pack-objects`, there may be a
quiet period while `pack-objects` prepares the pack. Normally
diff --git a/Documentation/git-fetch-pack.txt b/Documentation/git-fetch-pack.txt
index d45f6ad..f7ebe36 100644
--- a/Documentation/git-fetch-pack.txt
+++ b/Documentation/git-fetch-pack.txt
@@ -119,9 +119,9 @@ be in a separate packet, and the list must end with a flush packet.
$GIT_DIR (e.g. "HEAD", "refs/heads/master"). When
unspecified, update from all heads the remote side has.
+
-If the remote has enabled the options `uploadpack.allowTipSHA1InWant` or
-`uploadpack.allowReachableSHA1InWant`, they may alternatively be 40-hex
-sha1s present on the remote.
+If the remote has enabled the options `uploadpack.allowTipSHA1InWant`,
+`uploadpack.allowReachableSHA1InWant`, or `uploadpack.allowAnySHA1InWant`,
+they may alternatively be 40-hex sha1s present on the remote.
SEE ALSO
--------
diff --git a/t/t5551-http-fetch-smart.sh b/t/t5551-http-fetch-smart.sh
index 43665ab..8d3db40 100755
--- a/t/t5551-http-fetch-smart.sh
+++ b/t/t5551-http-fetch-smart.sh
@@ -306,6 +306,28 @@ test_expect_success 'test allowreachablesha1inwant with unreachable' '
test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
'
+test_expect_success 'test allowanysha1inwant with unreachable' '
+ test_when_finished "rm -rf test_reachable.git; git reset --hard $(git rev-parse HEAD)" &&
+
+ #create unreachable sha
+ echo content >file2 &&
+ git add file2 &&
+ git commit -m two &&
+ git push public HEAD:refs/heads/doomed &&
+ git push public :refs/heads/doomed &&
+
+ server="$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
+ master_sha=$(git -C "$server" rev-parse refs/heads/master) &&
+ git -C "$server" config uploadpack.allowreachablesha1inwant 1 &&
+
+ git init --bare test_reachable.git &&
+ git -C test_reachable.git remote add origin "$HTTPD_URL/smart/repo.git" &&
+ test_must_fail git -C test_reachable.git fetch origin "$(git rev-parse HEAD)" &&
+
+ git -C "$server" config uploadpack.allowanysha1inwant 1 &&
+ git -C test_reachable.git fetch origin "$(git rev-parse HEAD)"
+'
+
test_expect_success EXPENSIVE 'http can handle enormous ref negotiation' '
(
cd "$HTTPD_DOCUMENT_ROOT_PATH/repo.git" &&
diff --git a/upload-pack.c b/upload-pack.c
index e0db8b4..7597ba3 100644
--- a/upload-pack.c
+++ b/upload-pack.c
@@ -46,6 +46,8 @@ static int no_progress, daemon_mode;
#define ALLOW_TIP_SHA1 01
/* Allow request of a sha1 if it is reachable from a ref (possibly hidden ref). */
#define ALLOW_REACHABLE_SHA1 02
+/* Allow request of any sha1. Implies ALLOW_TIP_SHA1 and ALLOW_REACHABLE_SHA1. */
+#define ALLOW_ANY_SHA1 07
static unsigned int allow_unadvertised_object_request;
static int shallow_nr;
static struct object_array have_obj;
@@ -825,7 +827,8 @@ static void receive_needs(void)
sha1_to_hex(sha1_buf));
if (!(o->flags & WANTED)) {
o->flags |= WANTED;
- if (!is_our_ref(o))
+ if (!((allow_unadvertised_object_request & ALLOW_ANY_SHA1) == ALLOW_ANY_SHA1
+ || is_our_ref(o)))
has_non_tip = 1;
add_object_array(o, NULL, &want_obj);
}
@@ -1008,6 +1011,11 @@ static int upload_pack_config(const char *var, const char *value, void *unused)
allow_unadvertised_object_request |= ALLOW_REACHABLE_SHA1;
else
allow_unadvertised_object_request &= ~ALLOW_REACHABLE_SHA1;
+ } else if (!strcmp("uploadpack.allowanysha1inwant", var)) {
+ if (git_config_bool(var, value))
+ allow_unadvertised_object_request |= ALLOW_ANY_SHA1;
+ else
+ allow_unadvertised_object_request &= ~ALLOW_ANY_SHA1;
} else if (!strcmp("uploadpack.keepalive", var)) {
keepalive = git_config_int(var, value);
if (!keepalive)
--
2.8.0.rc4.22.g8ae061a
^ permalink raw reply related
page: next (older) | prev (newer) | latest
- recent:[subjects (threaded)|topics (new)|topics (active)]
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox