git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Stefan Beller <sbeller@google.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jacob Keller <jacob.e.keller@intel.com>,
	"git@vger.kernel.org" <git@vger.kernel.org>,
	Brandon Williams <bmwill@google.com>,
	Jacob Keller <jacob.keller@gmail.com>
Subject: Re: [PATCH v2 2/2] ls-files: fix path used when recursing into submodules
Date: Tue, 18 Apr 2017 11:41:01 -0700	[thread overview]
Message-ID: <CAGZ79kY9aEPp6BgWxp85ssNhdr=0AjMAh5tHJkMo6c+JhLjg-Q@mail.gmail.com> (raw)
In-Reply-To: <xmqqk26ih3qf.fsf@gitster.mtv.corp.google.com>

On Mon, Apr 17, 2017 at 7:03 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jacob Keller <jacob.e.keller@intel.com> writes:
>
>> From: Jacob Keller <jacob.keller@gmail.com>
>>
>> Don't assume that the current working directory is the root of the
>> repository. Correctly generate the path for the recursing child
>> processes by building it from the work_tree() root instead. Otherwise if
>> we run ls-files using --git-dir or --work-tree it will not work
>> correctly as it attempts to change directory into a potentially invalid
>> location. Best case, it doesn't exist and we produce an error. Worst
>> case we cd into the wrong location and unknown behavior occurs.
>>
>> Add a new test which highlights this possibility.
>>
>> Signed-off-by: Jacob Keller <jacob.keller@gmail.com>
>> ---
>> I'm not sure that I'm convinced by this method of solving the problem as
>> I suspect it has some corner cases (what about when run inside a
>> subdirectory? It seems to work for me but I'm not sure...) Additionally,
>> it felt weird that there's no helper function for creating a toplevel
>> relative path.
>
> Is this a similar issue as discussed in a nearby thread e.g.
>
>   <CACsJy8CLBY22j3EjR4PW3n+K6PWUzb-HCgxTVeCGpwtApZF-6g@mail.gmail.com>

yes, it's the same broader issue.

>
> I do think it is a bug that sometimes we do not go to the root of
> the working tree when we know the repository is not a bare one.

ok, that would have been my understanding as well.

That nearby thread however highlights how it may be a user breaking if
we just fix the paths. The patch here doesn't cd to the root of the repo,
but constructs the correct path, which is correct. It may be considered
clumsy if we decide that cd'ing to the root of the repo is the correct bug fix
for the underlying issue.

Thanks,
Stefan

  parent reply	other threads:[~2017-04-18 18:41 UTC|newest]

Thread overview: 33+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2017-04-12  0:39 [PATCH] ls-files: properly prepare submodule environment Jacob Keller
2017-04-12 16:58 ` Brandon Williams
2017-04-12 22:45   ` Jacob Keller
2017-04-13 17:12     ` [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules Jacob Keller
2017-04-13 17:12       ` [PATCH v2 2/2] ls-files: fix path used when recursing into submodules Jacob Keller
2017-04-13 18:10         ` Brandon Williams
2017-04-13 18:15         ` Stefan Beller
2017-04-13 18:34           ` Jacob Keller
2017-04-18  2:03         ` Junio C Hamano
2017-04-18  7:42           ` Jacob Keller
2017-04-18 18:41           ` Stefan Beller [this message]
2017-04-13 18:03       ` [PATCH v2 1/2] ls-files: fix recurse-submodules with nested submodules Brandon Williams
2017-04-13 18:31         ` Jacob Keller
2017-04-13 18:35           ` Stefan Beller
2017-04-13 18:36             ` Brandon Williams
2017-04-13 18:57       ` [PATCH 3/2] ls-files: only recurse on active submodules Brandon Williams
2017-04-13 19:05         ` Stefan Beller
2017-04-13 19:12           ` Jacob Keller
2017-04-13 19:25             ` Stefan Beller
2017-04-13 19:30               ` Jacob Keller
2017-04-14 16:33               ` Jacob Keller
2017-04-14 17:02                 ` Stefan Beller
2017-04-14 23:49                   ` Jacob Keller
2017-04-19  1:03         ` Junio C Hamano
2017-05-12 16:21           ` paul
2017-05-12 17:26             ` Brandon Williams
2017-05-12 18:12               ` Paul Jolly
2017-05-12 18:19                 ` Brandon Williams
2017-08-01 18:16                   ` Paul Jolly
     [not found]                   ` <CACoUkn7i76dEsQa3eoN+7WR8QmsD1pWsRQ0dvhkxzFN0sxTmRQ@mail.gmail.com>
2017-08-01 18:18                     ` Brandon Williams
2017-08-01 18:19                       ` Paul Jolly
2017-08-01 18:20                         ` Brandon Williams
2017-08-01 18:41                           ` 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='CAGZ79kY9aEPp6BgWxp85ssNhdr=0AjMAh5tHJkMo6c+JhLjg-Q@mail.gmail.com' \
    --to=sbeller@google.com \
    --cc=bmwill@google.com \
    --cc=git@vger.kernel.org \
    --cc=gitster@pobox.com \
    --cc=jacob.e.keller@intel.com \
    --cc=jacob.keller@gmail.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).