On Wed, Jan 13, 2016 at 2:03 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Stefan Beller <sbeller@xxxxxxxxxx> writes:
>
>> Later on we want to deprecate the `git submodule init` command and make
>> it implicit in other submodule commands.
>
> I doubt there is a concensus for "deprecate" part to warrant the use
> of "we want" here. I tend to think that the latter half of the
> sentence is uncontroversial, i.e. it is a good idea to make other
> "submodule" subcommands internally call it when it makes sense, and
> also make knobs available to other commands like "clone" and
> possibly "checkout" so that the users do not have to do the
> "submodule init" as a separate step, though.
Maybe I need to rethink my strategy here and deliver a patch series
which includes a complete port of `submodule init`, and maybe even
options in checkout (and clone) to run `submodule init`. That way the
immediate benefit would be clear on why the series is a good idea.
The current wording is mostly arguing to Jens, how to do the submodule
groups thing later on, but skipping the immediate steps.
>
>> As I was porting the functionality I noticed some odds with the inputs.
>
> I can parse but cannot quite grok. You found some strange things in
> the input? Whose input, that comes from where given by whom?
>
>> To fully understand the situation I added some logging to the function
>> temporarily to capture all calls to the function throughout the test
>> suite. Duplicates have been removed and all unique testing inputs have
>> been recorded into t0060.
>
> I can also parse this, but it is unclear what you did to the
> temporary debugging help at the end. If you left it, then that is
> no longer a temporary but is part of the final product. It is also
> unclear what "Duplicates" you are talking about here.
So in v1 somebody complained it's not clear what kind of input you'd get into
the relative_url(up_path, remoteurl, url) function. I did not know either, as it
was a straight port, passing the test suite. So I wanted to add tests.
To come up with reasonable tests I added a section to the code similar as this:
{
FILE *f = fopen("/tmp/testcases", "a");
fprintf(f, "%s|%s|%s|%s\n", up_path, remoteurl, url, result);
fclose(f);
}
Then I run the whole test suite with the relative_url instrumented.
This gave me a file "/tmp/testcases" containing 500 lines with valid
in and output for the `relative_url` function.
However I run these 500 lines through sort|uniq to get about 90 lines
of unduplicated tests.
but in these 90 lines there were still syntactic duplicates where one
line may look like the other line just with
s/trash directory.tXXXX/trash directory.tYYYY/
so I removed these lines manually, too.
And that's how I came up with the set of tests.
The logging function to "/tmp/testcases" was temporary and is not part
of the final product, but by mentioning that, some issues may be clear
to the reader, such as:
* why there are tests with /u/trash directory-t7400.../...
* the tests are as exhaustive as the test suite before.
* there are no tests to test failure though, only test for good tests
>
> Do you mean that you found some of the existing tests were odd, and
> after examination with help from a temporary hack which does not
> remain in this patch, you determined that some tests were duplicated,
> which you removed, while adding new ones?
Yes, this.
>
>> builtin/submodule--helper.c | 189 ++++++++++++++++++++++++++++++++++++++++++++
>> git-submodule.sh | 81 +------------------
>> t/t0060-path-utils.sh | 42 ++++++++++
>> 3 files changed, 235 insertions(+), 77 deletions(-)
>>
>> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
>> index f4c3eff..3e58b5d 100644
>> --- a/builtin/submodule--helper.c
>> +++ b/builtin/submodule--helper.c
>> @@ -9,6 +9,193 @@
>> #include "submodule-config.h"
>> #include "string-list.h"
>> #include "run-command.h"
>> +#include "remote.h"
>> +#include "refs.h"
>> +#include "connect.h"
>> +
>> +static char *get_default_remote(void)
>> +{
>> + char *dest = NULL, *ret;
>> + unsigned char sha1[20];
>> + int flag;
>> + struct strbuf sb = STRBUF_INIT;
>> + const char *refname = resolve_ref_unsafe("HEAD", 0, sha1, &flag);
>> +
>> + if (!refname)
>> + die("No such ref: HEAD");
>> +
>> + refname = shorten_unambiguous_ref(refname, 0);
>> + strbuf_addf(&sb, "branch.%s.remote", refname);
>
> Is it correct to use shorten_unambiguous_ref() here like this? The
> function is meant to be used when you want heads/frotz because you
> have both refs/heads/frotz and refs/tags/frotz at the same time. I
> think you want to say branch.frotz.remote even in such a case. IOW,
> shouldn't it be skip_prefix() with refs/heads/, together with die()
> if the prefix is something else?
Right.
>
>> + if (git_config_get_string(sb.buf, &dest))
>> + ret = xstrdup("origin");
>> + else
>> + ret = xstrdup(dest);
>> +
>> + strbuf_release(&sb);
>> + return ret;
>> +}
>> +
>> +static int starts_with_dot_slash(const char *str)
>> +{
>> + return str[0] == '.' && is_dir_sep(str[1]);
>> +}
>> +
>> +static int starts_with_dot_dot_slash(const char *str)
>> +{
>> + return str[0] == '.' && str[1] == '.' && is_dir_sep(str[2]);
>> +}
>> +
>> +static char *last_dir_separator(char *str)
>> +{
>> + char* p = str + strlen(str);
>
> Asterisk sticks to the variable, not the type.
ok
>
>> + while (p-- != str)
>
> It is preferable to use '>' not '!=' here, because you know p
> approaches str from the larger side, for readability.
Also known as the limes operator (p--> str, "p goes to str")
just kidding :)
>
>> + if (is_dir_sep(*p))
>> + return p;
>> + return NULL;
>> +}
>
> (a useless comment) This is one of the rare places where I wish
> there were a version of strcspn() that scans from the right.
>
>> +static char *relative_url(const char *remote_url,
>> + const char *url,
>> + const char *up_path)
>> +{
>> + int is_relative = 0;
>> + int colonsep = 0;
>> + char *out;
>> + char *remoteurl = xstrdup(remote_url);
>> + struct strbuf sb = STRBUF_INIT;
>> + size_t len;
>> +
>> + len = strlen(remoteurl);
>
> Nothing wrong here, but it looked somewhat inconsistent to see this
> assignment, while remoteurl is done as an initialization [*1*]
ok, noted.
>
>
> [Footnote]
>
> *1* as a personal preference, I tend to prefer seeing only simple
> operations in initialization and heavyweight ones as a separate
> assignment to an otherwise uninitialized variable, and strlen() is
> lighter-weight than xstrdup() in my dictionary.
>
>
>
>> + if (is_dir_sep(remoteurl[len]))
>> + remoteurl[len] = '\0';
>> +
>> + if (!url_is_local_not_ssh(remoteurl) || is_absolute_path(remoteurl))
>> + is_relative = 0;
>> + else {
>> + is_relative = 1;
>> +
>> + /* Prepend a './' to ensure all relative remoteurls start
>> + * with './' or '../'. */
>
> Adjust the style, and perhaps remove the final full-stop to make the
> last string literal easier to see? I.e.
>
> /*
> * Prepend a './' to ensure all relative remoteurls
> * start with './' or '../'
> */
>
> would be easier to see what it is said.
ok
>
>> + if (!starts_with_dot_slash(remoteurl) &&
>> + !starts_with_dot_dot_slash(remoteurl)) {
>> + strbuf_reset(&sb);
>> + strbuf_addf(&sb, "./%s", remoteurl);
>> + free(remoteurl);
>> + remoteurl = strbuf_detach(&sb, NULL);
>> + }
>> + }
>> + /* When the url starts with '../', remove that and the
>> + * last directory in remoteurl. */
>
> Style.
ok
>
>> + while (url) {
>> + if (starts_with_dot_dot_slash(url)) {
>> + char *rfind;
>> + url += 3;
>> +
>> + rfind = last_dir_separator(remoteurl);
>> + if (rfind)
>> + *rfind = '\0';
>> + else {
>> + rfind = strrchr(remoteurl, ':');
>> + if (rfind) {
>> + *rfind = '\0';
>> + colonsep = 1;
>> + } else {
>> + if (is_relative || !strcmp(".", remoteurl))
>> + die(_("cannot strip one component off url '%s'"), remoteurl);
>> + else
>> + remoteurl = xstrdup(".");
>> + }
>> + }
>
> It is somewhat hard to see how this avoids stripping one (or both)
> slashes just after "http:" in remoteurl="http://site/path/";, leaving
> just "http:/" (or "http:").
it would leave just 'http:/' if url were to be ../../some/where/else,
such that the constructed url below would be http://some/where/else.
>
> This codepath has overly deep nesting levels. Is this the simplest
> we can do?
it's a direct translation from shell. I could imagine the inside of
if (starts_with_dot_dot_slash(url)) {
...
}
may go to its own function, such that it becomes:
while (url) {
if (starts_with_dot_dot_slash(url)) {
adjust_remoteurl_and_url(&url, &remoteurl)
else if (starts_with_dot_slash(url))
url += 2;
else
break;
}
with a proper name for adjust_remoteurl_and_url of course.
>
> The final else { if .. else } can be made into else if .. else to
> dedent the overlong die() by one level, but I am wondering if the
> deep nesting is just a symptom of logic being unnecessarily complex.
I don't think it's unnecessary complex, but results from a direct
shell->C translation.
>
>> + } else if (starts_with_dot_slash(url)) {
>> + url += 2;
>> + } else
>> + break;
>> + }
>> + strbuf_reset(&sb);
>> + strbuf_addf(&sb, "%s%s%s", remoteurl, colonsep ? ":" : "/", url);
>> +
>> + if (starts_with_dot_slash(sb.buf))
>> + out = xstrdup(sb.buf + 2);
>> + else
>> + out = xstrdup(sb.buf);
>> + strbuf_reset(&sb);
>> +
>> + free(remoteurl);
>> + if (!up_path || !is_relative)
>> + return out;
>> +
>> + strbuf_addf(&sb, "%s%s", up_path, out);
>> + free(out);
>> + return strbuf_detach(&sb, NULL);
>> +}
>
> Thanks.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html