All of lore.kernel.org
 help / color / mirror / 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

* 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.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.