git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Stefan Beller <sbeller@google.com>
Cc: "git\@vger.kernel.org" <git@vger.kernel.org>,
	Duy Nguyen <pclouds@gmail.com>,
	Jens Lehmann <Jens.Lehmann@web.de>,
	Jacob Keller <jacob.keller@gmail.com>,
	Eric Sunshine <sunshine@sunshineco.com>
Subject: Re: [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option
Date: Fri, 25 Mar 2016 15:39:43 -0700	[thread overview]
Message-ID: <xmqqmvpm18sw.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAGZ79kZotAZeKJxRsHczMp5BG7edu=GLj6mox08M6afkMu1UDg@mail.gmail.com> (Stefan Beller's message of "Fri, 25 Mar 2016 15:09:29 -0700")

Stefan Beller <sbeller@google.com> writes:

> The replacement works fine in all tests except for the recursive
> tests as then the chdir is an important detail. In the submodule
> there is no $wt_prefix (as it is the parents' wt_prefix we passed in),

So the real reason is that we may tweak $wt_prefix to refer to a
non-existing directory, that submodule--helper is buggy and does not
notice it, and that using "-C $wt_prefix" would catch it because it
first tries to chdir to it.

The calling script "git submodule" first sets $wt_prefix to the
original directory, so there is no way chdir'ing back there would
not work, but if we update it (e.g. by appending a path to a
submodule we want to work in), it may very well end up referring to
a directory that does not exist (e.g. it may not have been
"init"ed).  Is that what is going on?

If that is the case, it makes a lot more sense as an explanation.
The justification for the main change 4/5 in the log message,
i.e. "-C $wt_prefix" is more familiar, was an irrelevant subjective
statement that only said "we could change it to use -C" without
explaining why "--prefix $wt_prefix" was wrong, and that was why I
was unhappy about it.

> So here is the example from before:
>         repo/ # a superproject repository
>         repo/untracked/ # an untracked dir in repo/
>         repo/sub/ # a submodule
>         repo/sub/subsub # a submodule of a submodule
>
> When calling "git submodule <cmd> --recursive" from within
> repo/untracked/, the recursed instance will end up in
> repo/sub/ with the parents prefix ("untracked/")
>
> In case of git -C $wt_prefix we would try to chdir into
> repo/sub/untracked/ which doesn't exist and our journey ends here.

That sounds like an unrelated bug, though.  Whether -C or --prefix
is used, we shouldn't be using "repo/sub/untracked" after going to
"repo/sub".  If the <cmd> somehow wanted to refer to a relative path
(e.g. "file") in the original directory, it should be using
../untracked as the base (e.g. it would use "../untracked/file").

Of course by using -C, you might notice that repo/sub/untracked does
not exist, but that is not a proper error checking---what if the
submodule at repo/sub does have a directory with that name?  IOW,
the computation that gave repo/sub/untracked instead of ../untracked
is wrong and using -C would not make it right.

And if you clear the prefix you originally obtained in calling
script "git submodule", which is "untracked/", even if <cmd> somehow
wanted to refer to the "file" in that directory, the only clue to do
so is forever lost.  Again, this is unrelated to -C/--prefix, but
that is what the patch 2 in the original series (which was rolled
into patch 1 in the update) was about.

So I am not sure what the value of using -C is.  At least that
"example from before" does not serve as a good justification.

  reply	other threads:[~2016-03-25 22:39 UTC|newest]

Thread overview: 15+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2016-03-25 18:39 [PATCHv3 0/5] submodule helper: cleanup prefix passing Stefan Beller
2016-03-25 18:39 ` [PATCHv3 1/5] submodule: prepare recursive path from non root directory Stefan Beller
2016-03-25 19:21   ` Junio C Hamano
2016-03-25 18:39 ` [PATCHv3 2/5] submodule--helper list: lose the extra prefix option Stefan Beller
2016-03-25 18:39 ` [PATCHv3 3/5] submodule update: add test for recursive from non root dir Stefan Beller
2016-03-25 18:39 ` [PATCHv3 4/5] submodule sync: test syncing one submodule Stefan Beller
2016-03-25 18:39 ` [PATCHv3 5/5] submodule--helper clone: lose the extra prefix option Stefan Beller
2016-03-25 19:41   ` Junio C Hamano
2016-03-25 20:54     ` Junio C Hamano
2016-03-25 22:09       ` Stefan Beller
2016-03-25 22:39         ` Junio C Hamano [this message]
2016-03-25 23:02           ` Stefan Beller
2016-03-25 23:15             ` Junio C Hamano
2016-03-25 23:51               ` Stefan Beller
2016-03-28 16:56               ` Junio C Hamano

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

  List information: http://vger.kernel.org/majordomo-info.html

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqqmvpm18sw.fsf@gitster.mtv.corp.google.com \
    --to=gitster@pobox.com \
    --cc=Jens.Lehmann@web.de \
    --cc=git@vger.kernel.org \
    --cc=jacob.keller@gmail.com \
    --cc=pclouds@gmail.com \
    --cc=sbeller@google.com \
    --cc=sunshine@sunshineco.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

	https://80x24.org/mirrors/git.git

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).