* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Junio C Hamano @ 2016-10-22 16:25 UTC (permalink / raw)
To: Jeff King; +Cc: git
In-Reply-To: <20161022050426.t2fifjqrldc6mebc@sigill.intra.peff.net>
Jeff King <peff@peff.net> writes:
> On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:
>
>> And this is the final one.
>>
>> -- >8 --
>> From: Junio C Hamano <gitster@pobox.com>
>> Date: Fri, 21 Oct 2016 21:33:06 -0700
>> Subject: [PATCH] transport: compute summary-width dynamically
>>
>> Now all that is left to do is to actually iterate over the refs
>> and measure the display width needed to show their abbreviation.
>
> I think we crossed emails. :) This is obviously correct, if we don't
> mind paying the find_unique_abbrev cost twice for each sha1.
Indeed we did. I do not think the cost matters that much in the
codepath to produce the final summary output.
> This is a minor style nit, but I think it's better to avoid mixing
> unrelated bits between the initialization, condition, and iteration bits
> of a for loop.
Yeah, you're right.
^ permalink raw reply
* Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer
From: Junio C Hamano @ 2016-10-22 16:19 UTC (permalink / raw)
To: Christian Couder
Cc: Jonathan Tan, Stefan Beller, git@vger.kernel.org, Ramsay Jones
In-Reply-To: <CAP8UFD0XvNx6Brio9muDhOj5+dr=HRbhK_Lmr+k4LUPKrAVC9Q@mail.gmail.com>
Christian Couder <christian.couder@gmail.com> writes:
> On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
>>
>> If I were guiding a topic that introduce this feature from scratch
>> today, I would probably suggest a pattern based approach, e.g. a
>> built-in "[-A-Za-z0-9]+:" [*1*] may be the default prefix that is
>> used to recognize the beginning of a trailer, and a user or a
>> project that wants "Bug #538" would be allowed to add an additional
>> pattern, e.g. "Bug *#", that recognises a custom trailer line that
>> is used by the project.
>
> When we designed the separator mechanism, we had the following discussions:
>
> https://public-inbox.org/git/xmqqa9a1d6xn.fsf@gitster.dls.corp.google.com/
> https://public-inbox.org/git/xmqqmwcuzyqx.fsf@gitster.dls.corp.google.com/
>
> They made me think that you were against too much flexibility, so I
> removed functionality that allowed to put separators into the ".key"
> config options, and now you are saying that we botched the thing and
> that you would like more flexibility of this kind back.
Correct. Pay attention to the fact that I said _we_ botched.
If an initial design made by a topic author is crappy, that may be
author's botch. Once a topic goes through a review cycle by getting
reviewed, rerolled, re-reviewed, ... to the point that those
involved accept the result, and we later realize that it was not
good, the botch no longer is author's alone. If it is shipped as
part of a release, then it is not just the authors and the reviewers
but everybody. We collectively stopped at a place that was not
ideal and share the blame ;-).
> Anyway I think it is still possible to add back such kind of
> functionality in a backward compatible way for example by adding
> ".extendedKey" config options.
Yup, or with trailer.keyPattern that is multi-values, or with any
number of alternatives.
^ permalink raw reply
* Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer
From: Christian Couder @ 2016-10-22 13:07 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Tan, Stefan Beller, git@vger.kernel.org, Ramsay Jones
In-Reply-To: <xmqqeg3aeeqe.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 2:18 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> That is true - I think we can take the allowed separators as an
>> argument (meaning that we can have different behavior for file parsing
>> and command line parsing), and since we already have that string, we
>> can use strcspn. I'll try this out in the next reroll.
>
> Sounds good. Thanks.
>
>
> The following is a tangent that I think this topic should ignore,
> but we may want to revisit it sometime later.
>
> I think the design of the "separator" mechanism is one of the things
> we botched in the current system. If I recall correctly, this was
> introduced to allow people write "Bug# 538" in the trailer section
> and get it recognised as a valid trailer.
>
> When I say that this was a botched design, I do not mean to say that
> we should have instead forced projects to adopt "Bug: 538" format.
> The design is botched because the users' wish to allow "Bug# 538" or
> "Bug #538" by setting separators to ":#" from the built-in ":" does
> not mean that they would want "Signed-off-by# me <my@addre.ss>" to
> be accepted.
>
> If I were guiding a topic that introduce this feature from scratch
> today, I would probably suggest a pattern based approach, e.g. a
> built-in "[-A-Za-z0-9]+:" [*1*] may be the default prefix that is
> used to recognize the beginning of a trailer, and a user or a
> project that wants "Bug #538" would be allowed to add an additional
> pattern, e.g. "Bug *#", that recognises a custom trailer line that
> is used by the project.
When we designed the separator mechanism, we had the following discussions:
https://public-inbox.org/git/xmqqa9a1d6xn.fsf@gitster.dls.corp.google.com/
https://public-inbox.org/git/xmqqmwcuzyqx.fsf@gitster.dls.corp.google.com/
They made me think that you were against too much flexibility, so I
removed functionality that allowed to put separators into the ".key"
config options, and now you are saying that we botched the thing and
that you would like more flexibility of this kind back.
Anyway I think it is still possible to add back such kind of
functionality in a backward compatible way for example by adding
".extendedKey" config options.
^ permalink raw reply
* Re: [PATCH v4 5/8] trailer: clarify failure modes in parse_trailer
From: Christian Couder @ 2016-10-22 9:29 UTC (permalink / raw)
To: Junio C Hamano
Cc: Jonathan Tan, Stefan Beller, git@vger.kernel.org, Ramsay Jones
In-Reply-To: <xmqqr37aej26.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 12:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Tan <jonathantanmy@google.com> writes:
>
>> If we do that, there is also the necessity of creating a string that
>> combines the separators and '=' (I guess '\n' is not necessary now,
>> since all the lines are null terminated). I'm OK either way.
>>
>> (We could cache that string, although I would think that if we did
>> that, we might as well write the loop manually, like in this patch.)
>
> I wonder if there is a legit reason to look for '=' in the first
> place. "Signed-off-by= Jonathan Tan <jt@my.home>" does not look
> like a valid trailer line to me.
>
> Isn't that a remnant of lazy coding in the original that tried to
> share a single parser for contents and command line options or
> something?
I think the relevant discussion was this one:
https://public-inbox.org/git/20140915.080429.1739849931027469667.chriscool@tuxfamily.org/
^ permalink raw reply
* Re: tools for easily "uncommitting" parts of a patch I just commited?
From: Lukas Fleischer @ 2016-10-22 9:19 UTC (permalink / raw)
To: git; +Cc: Jeff King, Git mailing list
In-Reply-To: <CA+P7+xq25LcdmtzmBNChiGhGratcdp7m0EOsQuEh68=gJQ9HNQ@mail.gmail.com>
On Thu, 20 Oct 2016 at 19:27:58, Jacob Keller wrote:
> [...]
> I still think we're misunderstanding. I want git commit to complain
> *only* under the following circumstance:
>
> I run "git add -p" and put a partial change into the index in <file>.
> There are still other parts which were not added to the index yet.
> Thus, the index version of the file and the actual file differ.
>
> Then, I (accidentally) run "git commit <file>"
> [...]
This reminded me of something that bothered me for a while. It's not
100% on-topic but still quite related so I thought I'd bring it up.
When working on a feature, I usually try to make atomic changes from the
beginning and use `git commit -a` to commit them one after another. This
works fine most of the time. Sometimes I notice only after making some
changes that it might be better to split the working tree changes into
several commits.
In that case, I git-add the relevant hunks and then, unfortunately, I
often run `git commit -a` instead of `git commit` (muscle memory bites
me), so I need to do all the splitting work again.
It's not much of an issue but would it be worthwhile to add an optional
feature (configurable) that warns you when using --all with staged
changes (which are not new files)? Are there others having the same
issue? Do you think this should be implemented as part of an alias
instead?
Regards,
Lukas
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Junio C Hamano @ 2016-10-22 7:33 UTC (permalink / raw)
To: Johannes Sixt
Cc: Stefan Beller, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <c6c5ce05-3511-a992-e079-316f0ce90ecd@kdbg.org>
Johannes Sixt <j6t@kdbg.org> writes:
>> The logic to construct the relative urls is not smart enough to
>> detect that the ending /. is referring to the directory itself
>> but rather treats it like any other relative path, i.e.
>>
>> path/to/dir/. + ../relative/path/to/submodule
>>
>> would result in
>>
>> path/to/dir/relative/path/to/submodule
>>
>> and not omit the "dir" as you may expect.
>>
>> As in a later patch we'll normalize the remote url before the
>> computation of relative urls takes place, we need to first get our
>> test suite in a shape with normalized urls only, which is why we should
>> refrain from cloning from '.'
>
> But you are removing a valid use case from the tests. Aren't you
> sweeping something under the rug with this patch?
I share the same reaction.
If the primary problem being solved is that the combination of a
relative URL ../sub and the base URL for the superproject which is
set to /path/to/dir/. (due to "clone .") were incorrectly resolved
as /path/to/dir/sub (because the buggy relative path logic did not
know that removing "/." at the end does not take you to one level
up), and a topic that fixes the bug would make that relative URL
../sub to be resolved as /path/to/sub, of course. Otherwise, the
topic did not fix the bug.
Now if a test that wanted to have a clone of the superproject by
"clone .", which results in the base URL of /path/to/dir/., actually
wants to refer in its .gitmodules to /path/to/dir/sub (which after
all was where the submodule the test created with or without the
bugfix), I would think the right adjustment for the test that used
to rely on the buggy behaviour would be to stop using ../sub and
instead use ./sub as the relative URL, no? After all, the bug forced
the original test writer to write ../sub but the submodule in this
case actually is at ./sub relative to its superproject, and that is
what the original test writer would have written if the bug weren't
there in the first place, no?
Another thing I do not quite understand is why this step comes
before the fix. If the "clone ." is adjusted to avoid triggering
the buggy behaviour, i.e. making the base URL to /path/to/dir
(instead of /path/to/dir/.), wouldn't the relative URL ../sub that
was written to work around the bug that hasn't been fixed yet in
this step need to be adjusted anyway? It would end up referring to
/path/to/sub and not /path/to/dir/sub until the fix is put in place.
Is the removal of remote.origin.url a wrong workaround for that
breakage, I wonder... I do not understand that change at all, and I
do not think it was explained in the log message.
If we really wanted to update the test before fixing the bug, I
would understand if this step changed ../sub (or whatever relative
URL that has extra ../ only because the base URL has extra /. at the
end to compensate for the buggy resolution) to ./sub in the tests
and marked them to expect failure, and then a later step that fixes
the bug turns them to expect success without make any other change.
^ permalink raw reply
* Re: [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Johannes Sixt @ 2016-10-22 7:09 UTC (permalink / raw)
To: Stefan Beller; +Cc: gitster, Johannes.Schindelin, git, venv21, dennis, jrnieder
In-Reply-To: <20161021235939.20792-3-sbeller@google.com>
Am 22.10.2016 um 01:59 schrieb Stefan Beller:
> When adding a submodule via "git submodule add <relative url>",
> the relative url applies to the superprojects remote. When the
> superproject was cloned via "git clone . super", the remote url
> is ending with '/.'.
>
> The logic to construct the relative urls is not smart enough to
> detect that the ending /. is referring to the directory itself
> but rather treats it like any other relative path, i.e.
>
> path/to/dir/. + ../relative/path/to/submodule
>
> would result in
>
> path/to/dir/relative/path/to/submodule
>
> and not omit the "dir" as you may expect.
>
> As in a later patch we'll normalize the remote url before the
> computation of relative urls takes place, we need to first get our
> test suite in a shape with normalized urls only, which is why we should
> refrain from cloning from '.'
But you are removing a valid use case from the tests. Aren't you
sweeping something under the rug with this patch?
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> t/t7064-wtstatus-pv2.sh | 9 ++++++---
> t/t7403-submodule-sync.sh | 3 ++-
> t/t7406-submodule-update.sh | 6 ++++--
> t/t7407-submodule-foreach.sh | 3 ++-
> t/t7506-status-submodule.sh | 3 ++-
> 5 files changed, 16 insertions(+), 8 deletions(-)
>
> diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
> index 3012a4d..95514bb 100755
> --- a/t/t7064-wtstatus-pv2.sh
> +++ b/t/t7064-wtstatus-pv2.sh
> @@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
> test_expect_success 'verify upstream fields in branch header' '
> git checkout master &&
> test_when_finished "rm -rf sub_repo" &&
> - git clone . sub_repo &&
> + git clone "$(pwd)" sub_repo &&
> + git -C sub_repo config --unset remote.origin.url &&
Why is it necessary to remove this configuration? Is it because when it
is present, the submodule does not construct its own reference? And if
so, should it not be sufficient to only remove the configuration
(without changing 'git clone' command), but move this patch after the
patch that fixes the /. treatment?
> (
> ## Confirm local master tracks remote master.
> cd sub_repo &&
...
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Philip Oakley @ 2016-10-22 5:45 UTC (permalink / raw)
To: Junio C Hamano; +Cc: GitList, Jeff King, Johannes Schindelin
In-Reply-To: <xmqq1sz9b9ex.fsf@gitster.mtv.corp.google.com>
From: "Junio C Hamano" <gitster@pobox.com>
> "Philip Oakley" <philipoakley@iee.org> writes:
>
>> It appears that acciidoctor sees the art as being a separated
>> mono-spaced block, with border/background as locally
>> appropriate. While the asciidoc looks to simply change to mono-spaced
>> text.
>
> FWIW, I changed my mind and your patch is now queued on 'next'.
>
> Thanks.
>
Many thanks
Philip
^ permalink raw reply
* Re: [PATCH] daemon: detect and reject too-long paths
From: Jeff King @ 2016-10-22 5:26 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
In-Reply-To: <20161022045938.h3xa3yapzlg427vy@sigill.intra.peff.net>
On Sat, Oct 22, 2016 at 12:59:38AM -0400, Jeff King wrote:
> When we are checking the path via path_ok(), we use some
> fixed PATH_MAX buffers. We write into them via snprintf(),
> so there's no possibility of overflow, but it does mean we
> may silently truncate the path, leading to potentially
> confusing errors when the partial path does not exist.
>
> We're better off to reject the path explicitly.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> Another option would be to switch to strbufs here. That potentially
> introduces cases where a client can convince us to just keep allocating
> memory, but I don't think so in practice; the paths and interpolated
> data items all have to come in 64K pkt-lines, which places a hard
> limit. This is a much more minimal change, though, and I don't hear
> anybody complaining about the inability to use large paths.
For reference, the switch to dynamic memory looks something like this.
We don't even need strbufs, and we can get rid of the static variables
entirely (they weren't about buffer reuse, but just about extending the
lifetime past the return value).
Though we do have to add some free()s to avoid leaking error cases, this
looks simpler to me (the return value _is_ leaked in the success case,
because the caller doesn't know if we returned the original value or a
newly allocated one. In practice it doesn't matter because we call this
function once per process; compare to the 8K of BSS being wasted in the
original).
diff --git a/daemon.c b/daemon.c
index 425aad0507..4575ce5 100644
--- a/daemon.c
+++ b/daemon.c
@@ -158,8 +158,7 @@ static size_t expand_path(struct strbuf *sb, const char *placeholder, void *ctx)
static const char *path_ok(const char *directory, struct hostinfo *hi)
{
- static char rpath[PATH_MAX];
- static char interp_path[PATH_MAX];
+ char *to_free = NULL;
const char *path;
const char *dir;
@@ -187,9 +186,9 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
namlen = slash - dir;
restlen -= namlen;
loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
- snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
- namlen, dir, user_path, restlen, slash);
- dir = rpath;
+ dir = to_free = xstrfmt("%.*s/%s%.*s",
+ namlen, dir, user_path,
+ restlen, slash);
}
}
else if (interpolated_path && hi->saw_extended_args) {
@@ -207,11 +206,8 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
strbuf_expand(&expanded_path, interpolated_path,
expand_path, &context);
- strlcpy(interp_path, expanded_path.buf, PATH_MAX);
- strbuf_release(&expanded_path);
- loginfo("Interpolated dir '%s'", interp_path);
-
- dir = interp_path;
+ dir = to_free = strbuf_detach(&expanded_path, NULL);
+ loginfo("Interpolated dir '%s'", dir);
}
else if (base_path) {
if (*dir != '/') {
@@ -219,8 +215,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
logerror("'%s': Non-absolute path denied (base-path active)", dir);
return NULL;
}
- snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
- dir = rpath;
+ dir = to_free = xstrfmt("%s%s", base_path, dir);
}
path = enter_repo(dir, strict_paths);
@@ -229,12 +224,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
* if we fail and base_path_relaxed is enabled, try without
* prefixing the base path
*/
+ free(to_free);
+ to_free = NULL;
dir = directory;
path = enter_repo(dir, strict_paths);
}
if (!path) {
logerror("'%s' does not appear to be a git repository", dir);
+ free(to_free);
return NULL;
}
@@ -265,6 +263,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
}
logerror("'%s': not in whitelist", path);
+ free(to_free);
return NULL; /* Fallthrough. Deny by default */
}
^ permalink raw reply related
* Re: [PATCH 4/3] test-lib: bail out when "-v" used under "prove"
From: Jacob Keller @ 2016-10-22 5:25 UTC (permalink / raw)
To: Jeff King; +Cc: Junio C Hamano, Stefan Beller, Lars Schneider, git
In-Reply-To: <20161022044506.vba6g2q25yxa2air@sigill.intra.peff.net>
On Fri, Oct 21, 2016 at 9:45 PM, Jeff King <peff@peff.net> wrote:
> I thought I'd just knock this out in 5 minutes before I forgot about it.
> But as with so many things, getting it right proved slightly harder than
> I thought.
Always seems to be that way, doesn't it?
> But I did learn about TAP's "Bail out!" directive. And
> apparently you can pass it back arbitrary YAML (!). And the "--verbose"
> output really is violating the spec, and they claim that Test::Harness
> will eventually be tightened to complain (though that was in 2007, and
> it still hasn't happened, so...).
>
> Anyway. Here is the patch I came up with (on top of the others).
>
Nice.
> -- >8 --
> Subject: test-lib: bail out when "-v" used under "prove"
>
> When there is a TAP harness consuming the output of our test
> scripts, the "--verbose" breaks the output by mingling
> test command output with TAP. Because the TAP::Harness
> module used by "prove" is fairly lenient, this _usually_
> works, but it violates the spec, and things get very
> confusing if the commands happen to output a line that looks
> like TAP (e.g., the word "ok" on its own line).
>
> Let's detect this situation and complain. Just calling
> error() isn't great, though; prove will tell us that the
> script failed, but the message doesn't make it through to
> the user. Instead, we can use the special TAP signal "Bail
> out!". This not only shows the message to the user, but
> instructs the harness to stop running the tests entirely.
> This is exactly what we want here, as the problem is in the
> command-line options, and every test script would produce
> the same error.
>
> The result looks like this (the first "Bailout called" line
> is in red if prove uses color on your terminal):
>
> $ make GIT_TEST_OPTS='--verbose --tee'
> rm -f -r 'test-results'
> *** prove ***
> Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
> FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
> Makefile:39: recipe for target 'prove' failed
> make: *** [prove] Error 255
>
Nice that makes sense.
> Signed-off-by: Jeff King <peff@peff.net>
> ---
> t/test-lib.sh | 10 ++++++++++
> 1 file changed, 10 insertions(+)
>
> diff --git a/t/test-lib.sh b/t/test-lib.sh
> index 85946ec40d..b859db61ac 100644
> --- a/t/test-lib.sh
> +++ b/t/test-lib.sh
> @@ -321,6 +321,16 @@ say () {
> say_color info "$*"
> }
>
> +if test -n "$HARNESS_ACTIVE"
> +then
> + if test "$verbose" = t || test -n "$verbose_only"
> + then
> + printf 'Bail out! %s\n' \
> + 'verbose mode forbidden under TAP harness; try --verbose-log'
> + exit 1
> + fi
> +fi
> +
Not too much code, so that's good. I like it.
Thanks,
Jake
> test "${test_description}" != "" ||
> error "Test script did not set test_description."
>
> --
> 2.10.1.776.ge0e381e
>
^ permalink raw reply
* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Jeff King @ 2016-10-22 5:04 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <xmqqoa2d9eum.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 09:39:45PM -0700, Junio C Hamano wrote:
> And this is the final one.
>
> -- >8 --
> From: Junio C Hamano <gitster@pobox.com>
> Date: Fri, 21 Oct 2016 21:33:06 -0700
> Subject: [PATCH] transport: compute summary-width dynamically
>
> Now all that is left to do is to actually iterate over the refs
> and measure the display width needed to show their abbreviation.
I think we crossed emails. :) This is obviously correct, if we don't
mind paying the find_unique_abbrev cost twice for each sha1.
> int transport_summary_width(const struct ref *refs)
> {
> - return (2 * FALLBACK_DEFAULT_ABBREV + 3);
> + int maxw;
> +
> + for (maxw = -1; refs; refs = refs->next) {
> + maxw = measure_abbrev(&refs->old_oid, maxw);
> + maxw = measure_abbrev(&refs->new_oid, maxw);
> + }
This is a minor style nit, but I think it's better to avoid mixing
unrelated bits between the initialization, condition, and iteration bits
of a for loop. IOW:
int maxw = -1;
for (; refs; refs = refs->next) {
...
}
makes the for-loop _just_ about iteration, and it is more clear that the
manipulation of "maxw" is a side effect of the loop that is valid after
it finishes.
Though in this particular case, the loop and the whole function are
short enough that I don't care that much. Just raising a philosophical
point. :)
-Peff
^ permalink raw reply
* [PATCH] daemon: detect and reject too-long paths
From: Jeff King @ 2016-10-22 4:59 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
When we are checking the path via path_ok(), we use some
fixed PATH_MAX buffers. We write into them via snprintf(),
so there's no possibility of overflow, but it does mean we
may silently truncate the path, leading to potentially
confusing errors when the partial path does not exist.
We're better off to reject the path explicitly.
Signed-off-by: Jeff King <peff@peff.net>
---
Another option would be to switch to strbufs here. That potentially
introduces cases where a client can convince us to just keep allocating
memory, but I don't think so in practice; the paths and interpolated
data items all have to come in 64K pkt-lines, which places a hard
limit. This is a much more minimal change, though, and I don't hear
anybody complaining about the inability to use large paths.
daemon.c | 25 +++++++++++++++++++++----
1 file changed, 21 insertions(+), 4 deletions(-)
diff --git a/daemon.c b/daemon.c
index 425aad0507..ff0fa583b0 100644
--- a/daemon.c
+++ b/daemon.c
@@ -160,6 +160,7 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
{
static char rpath[PATH_MAX];
static char interp_path[PATH_MAX];
+ size_t rlen;
const char *path;
const char *dir;
@@ -187,8 +188,12 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
namlen = slash - dir;
restlen -= namlen;
loginfo("userpath <%s>, request <%s>, namlen %d, restlen %d, slash <%s>", user_path, dir, namlen, restlen, slash);
- snprintf(rpath, PATH_MAX, "%.*s/%s%.*s",
- namlen, dir, user_path, restlen, slash);
+ rlen = snprintf(rpath, sizeof(rpath), "%.*s/%s%.*s",
+ namlen, dir, user_path, restlen, slash);
+ if (rlen >= sizeof(rpath)) {
+ logerror("user-path too large: %s", rpath);
+ return NULL;
+ }
dir = rpath;
}
}
@@ -207,7 +212,15 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
strbuf_expand(&expanded_path, interpolated_path,
expand_path, &context);
- strlcpy(interp_path, expanded_path.buf, PATH_MAX);
+
+ rlen = strlcpy(interp_path, expanded_path.buf,
+ sizeof(interp_path));
+ if (rlen >= sizeof(interp_path)) {
+ logerror("interpolated path too large: %s",
+ interp_path);
+ return NULL;
+ }
+
strbuf_release(&expanded_path);
loginfo("Interpolated dir '%s'", interp_path);
@@ -219,7 +232,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
logerror("'%s': Non-absolute path denied (base-path active)", dir);
return NULL;
}
- snprintf(rpath, PATH_MAX, "%s%s", base_path, dir);
+ rlen = snprintf(rpath, sizeof(rpath), "%s%s", base_path, dir);
+ if (rlen >= sizeof(rpath)) {
+ logerror("base-path too large: %s", rpath);
+ return NULL;
+ }
dir = rpath;
}
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* [PATCH 4/3] test-lib: bail out when "-v" used under "prove"
From: Jeff King @ 2016-10-22 4:45 UTC (permalink / raw)
To: Jacob Keller; +Cc: Junio C Hamano, Stefan Beller, Lars Schneider, git
In-Reply-To: <CA+P7+xp8TDL59tQgCpmMDJ-BspA1CF6YsnuGMXas1bx_X0qJoA@mail.gmail.com>
On Fri, Oct 21, 2016 at 09:15:28AM -0700, Jacob Keller wrote:
> >> > For $HARNESS_ACTIVE with _just_ "--verbose", I don't think it would be a
> >> > good idea to activate it. We should either silently ignore --verbose
> >> > then, or complain and die.
> >>
> >> We should probably do that to make sure people realize what might
> >> happen. I read your series and it has a good explanation of the
> >> possible alternatives. I like the approach you chose.
> >
> > Thanks. Do you want to make a patch on top of my series?
>
> I am not sure I will find time to do it today, so it wouldn't be for a
> few more days.
I thought I'd just knock this out in 5 minutes before I forgot about it.
But as with so many things, getting it right proved slightly harder than
I thought. But I did learn about TAP's "Bail out!" directive. And
apparently you can pass it back arbitrary YAML (!). And the "--verbose"
output really is violating the spec, and they claim that Test::Harness
will eventually be tightened to complain (though that was in 2007, and
it still hasn't happened, so...).
Anyway. Here is the patch I came up with (on top of the others).
-- >8 --
Subject: test-lib: bail out when "-v" used under "prove"
When there is a TAP harness consuming the output of our test
scripts, the "--verbose" breaks the output by mingling
test command output with TAP. Because the TAP::Harness
module used by "prove" is fairly lenient, this _usually_
works, but it violates the spec, and things get very
confusing if the commands happen to output a line that looks
like TAP (e.g., the word "ok" on its own line).
Let's detect this situation and complain. Just calling
error() isn't great, though; prove will tell us that the
script failed, but the message doesn't make it through to
the user. Instead, we can use the special TAP signal "Bail
out!". This not only shows the message to the user, but
instructs the harness to stop running the tests entirely.
This is exactly what we want here, as the problem is in the
command-line options, and every test script would produce
the same error.
The result looks like this (the first "Bailout called" line
is in red if prove uses color on your terminal):
$ make GIT_TEST_OPTS='--verbose --tee'
rm -f -r 'test-results'
*** prove ***
Bailout called. Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
FAILED--Further testing stopped: verbose mode forbidden under TAP harness; try --verbose-log
Makefile:39: recipe for target 'prove' failed
make: *** [prove] Error 255
Signed-off-by: Jeff King <peff@peff.net>
---
t/test-lib.sh | 10 ++++++++++
1 file changed, 10 insertions(+)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 85946ec40d..b859db61ac 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -321,6 +321,16 @@ say () {
say_color info "$*"
}
+if test -n "$HARNESS_ACTIVE"
+then
+ if test "$verbose" = t || test -n "$verbose_only"
+ then
+ printf 'Bail out! %s\n' \
+ 'verbose mode forbidden under TAP harness; try --verbose-log'
+ exit 1
+ fi
+fi
+
test "${test_description}" != "" ||
error "Test script did not set test_description."
--
2.10.1.776.ge0e381e
^ permalink raw reply related
* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Junio C Hamano @ 2016-10-22 4:39 UTC (permalink / raw)
To: git
In-Reply-To: <20161021223927.26364-4-gitster@pobox.com>
Junio C Hamano <gitster@pobox.com> writes:
> Now we have identified three callchains that have a set of refs that
> they want to show their <old, new> object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter. This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.
And this is the final one.
-- >8 --
From: Junio C Hamano <gitster@pobox.com>
Date: Fri, 21 Oct 2016 21:33:06 -0700
Subject: [PATCH] transport: compute summary-width dynamically
Now all that is left to do is to actually iterate over the refs
and measure the display width needed to show their abbreviation.
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
transport.c | 18 +++++++++++++++++-
1 file changed, 17 insertions(+), 1 deletion(-)
diff --git a/transport.c b/transport.c
index d4b8bf5f25..f1f95cf7c7 100644
--- a/transport.c
+++ b/transport.c
@@ -429,9 +429,25 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
return 1;
}
+static int measure_abbrev(const struct object_id *oid, int sofar)
+{
+ char hex[GIT_SHA1_HEXSZ + 1];
+ int w = find_unique_abbrev_r(hex, oid->hash, DEFAULT_ABBREV);
+
+ return (w < sofar) ? sofar : w;
+}
+
int transport_summary_width(const struct ref *refs)
{
- return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+ int maxw;
+
+ for (maxw = -1; refs; refs = refs->next) {
+ maxw = measure_abbrev(&refs->old_oid, maxw);
+ maxw = measure_abbrev(&refs->new_oid, maxw);
+ }
+ if (maxw < 0)
+ maxw = FALLBACK_DEFAULT_ABBREV;
+ return (2 * maxw + 3);
}
void transport_print_push_status(const char *dest, struct ref *refs,
--
2.10.1-723-g2384e83bc3
^ permalink raw reply related
* Re: [PATCH 3/3] transport: allow summary-width to be computed dynamically
From: Jeff King @ 2016-10-22 4:10 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
In-Reply-To: <20161021223927.26364-4-gitster@pobox.com>
On Fri, Oct 21, 2016 at 03:39:27PM -0700, Junio C Hamano wrote:
> Now we have identified three callchains that have a set of refs that
> they want to show their <old, new> object names in an aligned output,
> we can replace their reference to the constant TRANSPORT_SUMMARY_WIDTH
> with a helper function call to transport_summary_width() that takes
> the set of ref as a parameter. This step does not yet iterate over
> the refs and compute, which is left as an exercise to the readers.
The final step could be something like this:
diff --git a/transport.c b/transport.c
index 4dac713063..c1eaa4a 100644
--- a/transport.c
+++ b/transport.c
@@ -443,7 +443,21 @@ static int print_one_push_status(struct ref *ref, const char *dest, int count,
int transport_summary_width(const struct ref *refs)
{
- return (2 * FALLBACK_DEFAULT_ABBREV + 3);
+ int max_abbrev;
+
+ /*
+ * Computing the complete set of abbreviated sha1s is expensive just to
+ * find their lengths, but we can at least find our real dynamic
+ * minimum by picking an arbitrary sha1.
+ */
+ if (refs)
+ max_abbrev = strlen(find_unique_abbrev(refs->old_oid.hash,
+ DEFAULT_ABBREV));
+ else
+ max_abbrev = FALLBACK_DEFAULT_ABBREV;
+
+ /* 2 abbreviated sha1s, plus "..." in between */
+ return (2 * max_abbrev + 3);
}
void transport_print_push_status(const char *dest, struct ref *refs,
which produces reasonable results for me. But if we really wanted the
true value, I think we'd want to compute and store the abbreviated
sha1s, and then the refactoring done by your series probably isn't the
right direction.
I think we'd instead want to replace "struct strbuf *display" passed
down to update_local_ref() with something more like:
struct ref_status_table {
struct ref_status_item {
char old_hash[GIT_SHA1_HEX + 1];
char new_hash[GIT_SHA1_HEX + 1];
char *remote_ref;
char *local_ref;
char *summary;
char code;
} *items;
size_t alloc, nr;
};
and then format_display() would just add to the list (including calling
find_unique_abbrev()), and then at the end we'd call a function to show
them all.
That would also get rid of prepare_format_display(), as we could easily
walk over the prepared list before printing any of them (as opposed to
what that function does now, which is to walk over the ref map; that
requires that it know which refs are going to be printed).
-Peff
^ permalink raw reply related
* Re: [PATCH] daemon, path.c: fix a bug with ~ in repo paths
From: Jeff King @ 2016-10-22 3:26 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Duy Nguyen, Luke Shumaker, Git Mailing List
In-Reply-To: <xmqqvawlblxi.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 11:23:53AM -0700, Junio C Hamano wrote:
> A request to "git://<site>/<string>", depending on <string>, results
> in "directory" given to path_ok() in a bit different forms. Namely,
> connect.c::parse_connect_url() gives
>
> URL directory
> git://site/nouser.git /nouser.git
> git://site/~nouser.git ~nouser.git
>
> by special casing "~user" syntax (in other words, a pathname that
> begins with a tilde _is_ special cased, and tilde is not considered
> a normal character that can be in a pathname). Note the lack of
> leading slash in the second one.
>
> Because that is how the shipped clients work, the daemon needs a bit
> of adjustment, because interpolation and base-path prefix codepaths
> wants to accept only paths that begin with a slash, and relies on
> the slash when interpolating it in the template or concatenating it
> to the base (e.g. roughly "sprintf(buf, "%s%s", base_path, dir)").
>
> I came up with the following as a less invasive patch. There no
> longer is the ase where "user-path not allowed" error is given,
> as treating tilde as just a normal pathname character even when it
> appears at the beginning is the whole point of this change.
Thanks for explaining this. It is rather gross, but I think your
less-invasive patch is the best we could do given the client behavior.
And it's more what I would have expected based on the original problem
description.
> As I said already, I am not sure if this is a good change, though.
I also am on the fence. I'm not sure I understand a compelling reason to
have a non-interpolated "~" in the repo path name. Sure, it's a
limitation, but why does anybody care?
> @@ -170,11 +171,11 @@ static const char *path_ok(const char *directory, struct hostinfo *hi)
> return NULL;
> }
>
> + if (!user_path && *dir == '~') {
> + snprintf(tilde_path, PATH_MAX, "/%s", dir);
> + dir = tilde_path;
> + }
I know you are following existing convention in this function to use an
unchecked snprintf(), but it makes me wonder what kind of mischief a
client could get up to by silently truncating via snprintf. This
function is, after all, supposed to be checking the quality of the
incoming path.
xsnprintf() is probably too blunt a hammer, but:
if (snprintf(rpath, PATH_MAX, ...) >= PATH_MAX) {
logerror("path too long");
return NULL;
}
in various places may be appropriate.
-Peff
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Jeff King @ 2016-10-22 1:09 UTC (permalink / raw)
To: Philip Oakley; +Cc: Junio C Hamano, GitList, Johannes Schindelin
In-Reply-To: <D861234B3E78496DBA70EE63B2BCDB96@PhilipOakley>
On Fri, Oct 21, 2016 at 10:26:29PM +0100, Philip Oakley wrote:
> > updating the source they work on. Otherwise, the broken "doc-tool
> > stack" will keep producing broken output next time a source that
> > respects "tab is to skip to the next multiple of 8" rule is fed to
> > it, no?
>
> By avoiding tabs *within the art* we would also be tolerant of those who may
> not have a set their tab spacing to 8 when viewing the raw text.
>
> It's particularly the criss-cross diagram that needs fixed one way or
> another (for the doc/doctor differences).
I think the new asciidoctor correctly handles tabs within the art. The
earlier diagrams begin each line with a tab (to mark the pre-formatted
block), and then only some of the lines have additional tabs, and expect
those tabs to expand to 8 characters to line up with the other bits
(which is what caused a problem with earlier asciidoctor).
What is funny about that criss-cross diagram is that it actually chooses
different markers on each line to start the art: sometimes tabs and
sometimes spaces. And that seems to confuse even recent versions of
asciidoctor.
It may be that asciidoctor is wrong here, but I have to admit we are
venturing well into "what happens to work with asciidoc" territory, and
the right solution is just fixing the diagram (i.e., your patch).
-Peff
^ permalink raw reply
* Re: [PATCH v5 0/8] allow non-trailers and multiple-line trailers
From: Stefan Beller @ 2016-10-22 0:06 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Jonathan Tan, Git Mailing List
In-Reply-To: <CAPc5daX-bdSBAxy60zG2ZuGbrjGwRcsvHFktCqKw_o2QuuWTEg@mail.gmail.com>
On Fri, Oct 21, 2016 at 4:59 PM, Junio C Hamano <gitster@pobox.com> wrote:
> On Fri, Oct 21, 2016 at 10:54 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
>> I've updated patch 5/8 to use strcspn and to pass in the list of
>> separators, meaning that we no longer accept '=' in file input (and also
>> updated its commit message accordingly).
>
> Thanks for a pleasant read. Queued.
>
> Hopefully this is ready for 'next' now.
I also just read through and was about to say the same.
^ permalink raw reply
* [PATCH 2/3] submodule tests: replace cloning from . by "$(pwd)"
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
In-Reply-To: <20161021235939.20792-1-sbeller@google.com>
When adding a submodule via "git submodule add <relative url>",
the relative url applies to the superprojects remote. When the
superproject was cloned via "git clone . super", the remote url
is ending with '/.'.
The logic to construct the relative urls is not smart enough to
detect that the ending /. is referring to the directory itself
but rather treats it like any other relative path, i.e.
path/to/dir/. + ../relative/path/to/submodule
would result in
path/to/dir/relative/path/to/submodule
and not omit the "dir" as you may expect.
As in a later patch we'll normalize the remote url before the
computation of relative urls takes place, we need to first get our
test suite in a shape with normalized urls only, which is why we should
refrain from cloning from '.'
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7064-wtstatus-pv2.sh | 9 ++++++---
t/t7403-submodule-sync.sh | 3 ++-
t/t7406-submodule-update.sh | 6 ++++--
t/t7407-submodule-foreach.sh | 3 ++-
t/t7506-status-submodule.sh | 3 ++-
5 files changed, 16 insertions(+), 8 deletions(-)
diff --git a/t/t7064-wtstatus-pv2.sh b/t/t7064-wtstatus-pv2.sh
index 3012a4d..95514bb 100755
--- a/t/t7064-wtstatus-pv2.sh
+++ b/t/t7064-wtstatus-pv2.sh
@@ -330,7 +330,8 @@ test_expect_success 'verify UU (edit-edit) conflict' '
test_expect_success 'verify upstream fields in branch header' '
git checkout master &&
test_when_finished "rm -rf sub_repo" &&
- git clone . sub_repo &&
+ git clone "$(pwd)" sub_repo &&
+ git -C sub_repo config --unset remote.origin.url &&
(
## Confirm local master tracks remote master.
cd sub_repo &&
@@ -392,8 +393,10 @@ test_expect_success 'verify upstream fields in branch header' '
test_expect_success 'create and add submodule, submodule appears clean (A. S...)' '
git checkout master &&
- git clone . sub_repo &&
- git clone . super_repo &&
+ git clone "$(pwd)" sub_repo &&
+ git -C sub_repo config --unset remote.origin.url &&
+ git clone "$(pwd)" super_repo &&
+ git -C super_repo config --unset remote.origin.url &&
( cd super_repo &&
git submodule add ../sub_repo sub1 &&
diff --git a/t/t7403-submodule-sync.sh b/t/t7403-submodule-sync.sh
index 0726799..6d85391 100755
--- a/t/t7403-submodule-sync.sh
+++ b/t/t7403-submodule-sync.sh
@@ -15,7 +15,8 @@ test_expect_success setup '
git add file &&
test_tick &&
git commit -m upstream &&
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
git clone super submodule &&
(
cd submodule &&
diff --git a/t/t7406-submodule-update.sh b/t/t7406-submodule-update.sh
index 64f322c..9430c2a 100755
--- a/t/t7406-submodule-update.sh
+++ b/t/t7406-submodule-update.sh
@@ -26,7 +26,8 @@ test_expect_success 'setup a submodule tree' '
git add file &&
test_tick &&
git commit -m upstream &&
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
git clone super submodule &&
git clone super rebasing &&
git clone super merging &&
@@ -64,7 +65,8 @@ test_expect_success 'setup a submodule tree' '
test_tick &&
git commit -m "none"
) &&
- git clone . recursivesuper &&
+ git clone "$(pwd)" recursivesuper &&
+ git -C recursivesuper config --unset remote.origin.url &&
( cd recursivesuper
git submodule add ../super super
)
diff --git a/t/t7407-submodule-foreach.sh b/t/t7407-submodule-foreach.sh
index 6ba5daf..257e817 100755
--- a/t/t7407-submodule-foreach.sh
+++ b/t/t7407-submodule-foreach.sh
@@ -17,7 +17,8 @@ test_expect_success 'setup a submodule tree' '
git add file &&
test_tick &&
git commit -m upstream &&
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
git clone super submodule &&
(
cd super &&
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index 74cb6b8..62a99bc 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -197,7 +197,8 @@ A sub1
EOF
test_expect_success 'status with merge conflict in .gitmodules' '
- git clone . super &&
+ git clone "$(pwd)" super &&
+ git -C super config --unset remote.origin.url &&
test_create_repo_with_commit sub1 &&
test_tick &&
test_create_repo_with_commit sub2 &&
--
2.10.1.507.g2a9098a
^ permalink raw reply related
* [PATCH 3/3] submodule--helper: normalize funny urls
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
In-Reply-To: <20161021235939.20792-1-sbeller@google.com>
The remote URL for the submodule can be specified relative
to the URL of the superproject in .gitmodules. A top-level
git://site.xz/toplevel.git can specify in its .gitmodules
[submodule "sub"]
url = ../submodule.git
path = sub
to say that git://site.xz/submodule.git is where the
submodule bound at its "sub/" is found.
However, when the toplevel is cloned like this:
git clone git://site.xz/toplevel.git/. top
i.e. the toplevel specifies its URL with trailing "/.", the
code set the URL to git://site.xz/toplevel.git/submodule.git
for the submodule, which is nonsense. This was because the
logic failed to treat trailing "/." any differently from
trailing "/<anything-without-slash>" when resolving a
relative URL "../<something>" off of it. Stripping "/." at
the end does *not* take you one level up, even though
stripping "/<anything-without-slash>" does!
Some users may rely on this by always cloning with '/.' and having
an additional '../' in the relative path for the submodule, and this
patch breaks them. So why introduce this patch?
The fix in c12922024 (submodule: ignore trailing slash on superproject
URL, 2016-10-10) and prior discussion revealed, that Git and Git
for Windows treat URLs differently, as currently Git for Windows
strips off a trailing dot from paths when calling a Git binary
unlike when running a shell. Which means Git for Windows is already
doing the right thing for the case mentioned above, but it would fail
our current tests, that test for the broken behavior and it would
confuse users working across platforms. So we'd rather fix it
in Git to ignore any of these trailing no ops in the path properly.
We never produce the URLs with a trailing '/.' in Git ourselves,
they come to us, because the user used it as the URL for cloning
a superproject. Normalize these paths.
The test 3600 needs a slight adaption as well, and was not covered by
the previous patch, because the setup of the submodule in this test
is different than the "clone . super" pattern that was fixed in the
previous patch.
The url configured for the submodule path submod is "./.", which
gets normalized with this patch, such that the nested submodule
'subsubmod' doesn't resolve its path properly via '../'.
In later tests we do not need the url any more as testing of
removal of the config including url happens in early tests, so the
easieast fix for that test is to just make the submodule its own
authoritative source of truth by removing the remote url.
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Stefan Beller <sbeller@google.com>
---
builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++------------
t/t0060-path-utils.sh | 11 +++++++----
t/t3600-rm.sh | 1 +
3 files changed, 44 insertions(+), 16 deletions(-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index 4beeda5..21e2e2a 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -76,6 +76,29 @@ static int chop_last_dir(char **remoteurl, int is_relative)
return 0;
}
+static void strip_url_ending(char *url, size_t *len_)
+{
+ size_t len = len_ ? *len_ : strlen(url);
+
+ for (;;) {
+ if (len > 1 && is_dir_sep(url[len - 2]) && url[len - 1] == '.') {
+ url[len-2] = '\0';
+ len -= 2;
+ continue;
+ }
+ if (len > 0 && is_dir_sep(url[len-1])) {
+ url[len-1] = '\0';
+ len--;
+ continue;
+ }
+
+ break;
+ }
+
+ if (len_)
+ *len_ = len;
+}
+
/*
* The `url` argument is the URL that navigates to the submodule origin
* repo. When relative, this URL is relative to the superproject origin
@@ -93,14 +116,16 @@ static int chop_last_dir(char **remoteurl, int is_relative)
* the superproject working tree otherwise.
*
* NEEDSWORK: This works incorrectly on the domain and protocol part.
- * remote_url url outcome expectation
- * http://a.com/b ../c http://a.com/c as is
- * http://a.com/b/ ../c http://a.com/c same as previous line, but
- * ignore trailing slash in url
- * http://a.com/b ../../c http://c error out
- * http://a.com/b ../../../c http:/c error out
- * http://a.com/b ../../../../c http:c error out
- * http://a.com/b ../../../../../c .:c error out
+ * remote_url url outcome expectation
+ * http://a.com/b ../c http://a.com/c as is
+ * http://a.com/b/ ../c http://a.com/c same as previous line, but
+ * ignore trailing '/' in url
+ * http://a.com/b/. ../c http://a.com/c same as previous line, but
+ * ignore trailing '/.' in url
+ * http://a.com/b ../../c http://c error out
+ * http://a.com/b ../../../c http:/c error out
+ * http://a.com/b ../../../../c http:c error out
+ * http://a.com/b ../../../../../c .:c error out
* NEEDSWORK: Given how chop_last_dir() works, this function is broken
* when a local part has a colon in its path component, too.
*/
@@ -115,8 +140,7 @@ static char *relative_url(const char *remote_url,
struct strbuf sb = STRBUF_INIT;
size_t len = strlen(remoteurl);
- if (is_dir_sep(remoteurl[len-1]))
- remoteurl[len-1] = '\0';
+ strip_url_ending(remoteurl, &len);
if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
is_relative = 0;
@@ -149,10 +173,10 @@ static char *relative_url(const char *remote_url,
}
strbuf_reset(&sb);
strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
- if (ends_with(url, "/"))
- strbuf_setlen(&sb, sb.len - 1);
free(remoteurl);
+ strip_url_ending(sb.buf, &sb.len);
+
if (starts_with_dot_slash(sb.buf))
out = xstrdup(sb.buf + 2);
else
diff --git a/t/t0060-path-utils.sh b/t/t0060-path-utils.sh
index 25b48e5..e154e5f 100755
--- a/t/t0060-path-utils.sh
+++ b/t/t0060-path-utils.sh
@@ -329,14 +329,17 @@ test_submodule_relative_url "(null)" "./foo" "../submodule" "submodule"
test_submodule_relative_url "(null)" "//somewhere else/repo" "../subrepo" "//somewhere else/subrepo"
test_submodule_relative_url "(null)" "$PWD/subsuper_update_r" "../subsubsuper_update_r" "$(pwd)/subsubsuper_update_r"
test_submodule_relative_url "(null)" "$PWD/super_update_r2" "../subsuper_update_r" "$(pwd)/subsuper_update_r"
-test_submodule_relative_url "(null)" "$PWD/." "../." "$(pwd)/."
-test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)/."
+test_submodule_relative_url "(null)" "$PWD/sub/." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD/sub/.////././/./." "../." "$(pwd)"
+test_submodule_relative_url "(null)" "$PWD" "./." "$(pwd)"
test_submodule_relative_url "(null)" "$PWD/addtest" "../repo" "$(pwd)/repo"
test_submodule_relative_url "(null)" "$PWD" "./å äö" "$(pwd)/å äö"
-test_submodule_relative_url "(null)" "$PWD/." "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub" "../submodule" "$(pwd)/submodule"
+test_submodule_relative_url "(null)" "$PWD/sub/." "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/submodule" "../submodule" "$(pwd)/submodule"
test_submodule_relative_url "(null)" "$PWD/home2/../remote" "../bundle1" "$(pwd)/home2/../bundle1"
-test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo/."
+test_submodule_relative_url "(null)" "$PWD/submodule_update_repo" "./." "$(pwd)/submodule_update_repo"
test_submodule_relative_url "(null)" "file:///tmp/repo" "../subrepo" "file:///tmp/subrepo"
test_submodule_relative_url "(null)" "foo/bar" "../submodule" "foo/submodule"
test_submodule_relative_url "(null)" "foo" "../submodule" "submodule"
diff --git a/t/t3600-rm.sh b/t/t3600-rm.sh
index d046d98..7839ccd 100755
--- a/t/t3600-rm.sh
+++ b/t/t3600-rm.sh
@@ -615,6 +615,7 @@ test_expect_success 'setup subsubmodule' '
git reset --hard &&
git submodule update &&
(cd submod &&
+ git config --unset remote.origin.url &&
git update-index --add --cacheinfo 160000 $(git rev-parse HEAD) subsubmod &&
git config -f .gitmodules submodule.sub.url ../. &&
git config -f .gitmodules submodule.sub.path subsubmod &&
--
2.10.1.507.g2a9098a
^ permalink raw reply related
* [PATCH 1/3] t7506: fix diff order arguments in test_cmp
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
In-Reply-To: <20161021235939.20792-1-sbeller@google.com>
Fix the argument order for test_cmp. When given the expected
result first the diff shows the actual output with '+' and the
expectation with '-', which is the convention for our tests.
Signed-off-by: Stefan Beller <sbeller@google.com>
---
t/t7506-status-submodule.sh | 4 ++--
1 file changed, 2 insertions(+), 2 deletions(-)
diff --git a/t/t7506-status-submodule.sh b/t/t7506-status-submodule.sh
index d31b34d..74cb6b8 100755
--- a/t/t7506-status-submodule.sh
+++ b/t/t7506-status-submodule.sh
@@ -260,7 +260,7 @@ test_expect_success 'diff with merge conflict in .gitmodules' '
cd super &&
git diff >../diff_actual 2>&1
) &&
- test_cmp diff_actual diff_expect
+ test_cmp diff_expect diff_actual
'
test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
@@ -268,7 +268,7 @@ test_expect_success 'diff --submodule with merge conflict in .gitmodules' '
cd super &&
git diff --submodule >../diff_submodule_actual 2>&1
) &&
- test_cmp diff_submodule_actual diff_submodule_expect
+ test_cmp diff_submodule_expect diff_submodule_actual
'
test_done
--
2.10.1.507.g2a9098a
^ permalink raw reply related
* Re: [PATCH v5 0/8] allow non-trailers and multiple-line trailers
From: Junio C Hamano @ 2016-10-21 23:59 UTC (permalink / raw)
To: Jonathan Tan; +Cc: Git Mailing List, Stefan Beller
In-Reply-To: <cover.1477072247.git.jonathantanmy@google.com>
On Fri, Oct 21, 2016 at 10:54 AM, Jonathan Tan <jonathantanmy@google.com> wrote:
> I've updated patch 5/8 to use strcspn and to pass in the list of
> separators, meaning that we no longer accept '=' in file input (and also
> updated its commit message accordingly).
Thanks for a pleasant read. Queued.
Hopefully this is ready for 'next' now.
^ permalink raw reply
* [PATCH 0/3] Fix submodule url issues
From: Stefan Beller @ 2016-10-21 23:59 UTC (permalink / raw)
To: gitster, j6t, Johannes.Schindelin
Cc: git, venv21, dennis, jrnieder, Stefan Beller
previous patch: http://public-inbox.org/git/20161018210623.32696-1-sbeller@google.com/
First fix our test suite in patch 2,
then patch 3 is a resend of the previous patch to normalize the configured
remote.
Thanks,
Stefan
Stefan Beller (3):
t7506: fix diff order arguments in test_cmp
submodule tests: replace cloning from . by "$(pwd)"
submodule--helper: normalize funny urls
builtin/submodule--helper.c | 48 +++++++++++++++++++++++++++++++++-----------
t/t0060-path-utils.sh | 11 ++++++----
t/t3600-rm.sh | 1 +
t/t7064-wtstatus-pv2.sh | 9 ++++++---
t/t7403-submodule-sync.sh | 3 ++-
t/t7406-submodule-update.sh | 6 ++++--
t/t7407-submodule-foreach.sh | 3 ++-
t/t7506-status-submodule.sh | 7 ++++---
8 files changed, 62 insertions(+), 26 deletions(-)
--
2.10.1.507.g2a9098a
^ permalink raw reply
* Re: generating combined diff without an existing merge commit
From: Jacob Keller @ 2016-10-21 22:54 UTC (permalink / raw)
To: Junio C Hamano; +Cc: Git mailing list
In-Reply-To: <xmqq60olb9zq.fsf@gitster.mtv.corp.google.com>
On Fri, Oct 21, 2016 at 3:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.keller@gmail.com> writes:
>
>> Turns out that somehow I must have messed up my command because "git
>> diff <treeish> <treeish> <treeish>" does indeed do exactly what I
>> want.
>
> Thanks. I was wondering why it didn't work for you, as the feature
> was done exactly to support that use case ;-)
I'm not really sure. It kept spitting out usage information to me, but
then I tried again with a fresh example and it worked as expected.
Thanks,
Jake
^ permalink raw reply
* Re: [PATCH] doc: fix merge-base ASCII art tab spacing
From: Junio C Hamano @ 2016-10-21 22:54 UTC (permalink / raw)
To: Philip Oakley; +Cc: GitList, Jeff King, Johannes Schindelin
In-Reply-To: <D861234B3E78496DBA70EE63B2BCDB96@PhilipOakley>
"Philip Oakley" <philipoakley@iee.org> writes:
> It appears that acciidoctor sees the art as being a separated
> mono-spaced block, with border/background as locally
> appropriate. While the asciidoc looks to simply change to mono-spaced
> text.
FWIW, I changed my mind and your patch is now queued on 'next'.
Thanks.
^ 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