git@vger.kernel.org mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
* [PATCH] bisect: allow to run from subdirectories
@ 2021-06-20 21:38 Roland Hieber
  2021-06-21  0:35 ` Ævar Arnfjörð Bjarmason
  2021-06-21  3:43 ` Junio C Hamano
  0 siblings, 2 replies; 13+ messages in thread
From: Roland Hieber @ 2021-06-20 21:38 UTC (permalink / raw)
  To: git; +Cc: Roland Hieber, Vasco Almeida, Junio C Hamano

Currently, calling 'git bisect' from a directory other than the top
level of a repository only comes up with an error message:

    You need to run this command from the toplevel of the working tree.

After a glance through the bisect code, there seems to be nothing that
relies on the current working directory, and a few hours of bisect usage
also didn't turn up any problems. Set the appropriate flag for
git-sh-setup to remove the error message.

Signed-off-by: Roland Hieber <rhi@pengutronix.de>
---
 git-bisect.sh | 1 +
 1 file changed, 1 insertion(+)

diff --git a/git-bisect.sh b/git-bisect.sh
index 6a7afaea8da0..20ba0ee7c18a 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -32,6 +32,7 @@ git bisect run <cmd>...
 Please use "git help bisect" to get the full man page.'
 
 OPTIONS_SPEC=
+SUBDIRECTORY_OK=1
 . git-sh-setup
 
 _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
-- 
2.29.2


^ permalink raw reply related	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-20 21:38 [PATCH] bisect: allow to run from subdirectories Roland Hieber
@ 2021-06-21  0:35 ` Ævar Arnfjörð Bjarmason
  2021-06-21  2:00   ` brian m. carlson
  2021-06-21  3:43 ` Junio C Hamano
  1 sibling, 1 reply; 13+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-21  0:35 UTC (permalink / raw)
  To: Roland Hieber; +Cc: git, Vasco Almeida, Junio C Hamano


On Sun, Jun 20 2021, Roland Hieber wrote:

> Currently, calling 'git bisect' from a directory other than the top
> level of a repository only comes up with an error message:
>
>     You need to run this command from the toplevel of the working tree.
>
> After a glance through the bisect code, there seems to be nothing that
> relies on the current working directory, and a few hours of bisect usage
> also didn't turn up any problems. Set the appropriate flag for
> git-sh-setup to remove the error message.
>
> Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> ---
>  git-bisect.sh | 1 +
>  1 file changed, 1 insertion(+)
>
> diff --git a/git-bisect.sh b/git-bisect.sh
> index 6a7afaea8da0..20ba0ee7c18a 100755
> --- a/git-bisect.sh
> +++ b/git-bisect.sh
> @@ -32,6 +32,7 @@ git bisect run <cmd>...
>  Please use "git help bisect" to get the full man page.'
>  
>  OPTIONS_SPEC=
> +SUBDIRECTORY_OK=1
>  . git-sh-setup
>  
>  _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'

How does this affect out-of-tree scripts that will be run with "git
bisect run", is the cwd set to the root as they now might expect git to
check, or whatever subdirectory you ran the "run" from?

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21  0:35 ` Ævar Arnfjörð Bjarmason
@ 2021-06-21  2:00   ` brian m. carlson
  2021-06-21  2:10     ` Eric Sunshine
  0 siblings, 1 reply; 13+ messages in thread
From: brian m. carlson @ 2021-06-21  2:00 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Roland Hieber, git, Vasco Almeida, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 2128 bytes --]

On 2021-06-21 at 00:35:49, Ævar Arnfjörð Bjarmason wrote:
> 
> On Sun, Jun 20 2021, Roland Hieber wrote:
> 
> > Currently, calling 'git bisect' from a directory other than the top
> > level of a repository only comes up with an error message:
> >
> >     You need to run this command from the toplevel of the working tree.
> >
> > After a glance through the bisect code, there seems to be nothing that
> > relies on the current working directory, and a few hours of bisect usage
> > also didn't turn up any problems. Set the appropriate flag for
> > git-sh-setup to remove the error message.
> >
> > Signed-off-by: Roland Hieber <rhi@pengutronix.de>
> > ---
> >  git-bisect.sh | 1 +
> >  1 file changed, 1 insertion(+)
> >
> > diff --git a/git-bisect.sh b/git-bisect.sh
> > index 6a7afaea8da0..20ba0ee7c18a 100755
> > --- a/git-bisect.sh
> > +++ b/git-bisect.sh
> > @@ -32,6 +32,7 @@ git bisect run <cmd>...
> >  Please use "git help bisect" to get the full man page.'
> >  
> >  OPTIONS_SPEC=
> > +SUBDIRECTORY_OK=1
> >  . git-sh-setup
> >  
> >  _x40='[0-9a-f][0-9a-f][0-9a-f][0-9a-f][0-9a-f]'
> 
> How does this affect out-of-tree scripts that will be run with "git
> bisect run", is the cwd set to the root as they now might expect git to
> check, or whatever subdirectory you ran the "run" from?

I'm also interested in this, specifically as a patch to the
documentation and a corresponding test (and a commit message
justification), since folks will rely on whatever behavior we implement
and we won't want to break it.

We'd probably also want to add a test at least that the user can invoke
git bisect outside of the root of the repository, and maybe that it
performs correct results for at least one or two known cases when
invoked outside of the root.  And I'm also wondering if maybe there are
other cases that deserve a test along with this change.

As for the idea itself, I think it's a good one assuming everything
continues to work.  It will certainly be more convenient for a lot of
people.
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21  2:00   ` brian m. carlson
@ 2021-06-21  2:10     ` Eric Sunshine
  2021-06-21  9:33       ` Roland Hieber
  2021-06-22  0:09       ` brian m. carlson
  0 siblings, 2 replies; 13+ messages in thread
From: Eric Sunshine @ 2021-06-21  2:10 UTC (permalink / raw)
  To: brian m. carlson, Ævar Arnfjörð Bjarmason,
	Roland Hieber, Git List, Vasco Almeida, Junio C Hamano

On Sun, Jun 20, 2021 at 10:00 PM brian m. carlson
<sandals@crustytoothpaste.net> wrote:
> On 2021-06-21 at 00:35:49, Ævar Arnfjörð Bjarmason wrote:
> > On Sun, Jun 20 2021, Roland Hieber wrote:
> > > Currently, calling 'git bisect' from a directory other than the top
> > > level of a repository only comes up with an error message:
> > >
> > >     You need to run this command from the toplevel of the working tree.
> >
> > How does this affect out-of-tree scripts that will be run with "git
> > bisect run", is the cwd set to the root as they now might expect git to
> > check, or whatever subdirectory you ran the "run" from?
>
> As for the idea itself, I think it's a good one assuming everything
> continues to work.  It will certainly be more convenient for a lot of
> people.

There have been multiple patches sent to the project over the years
with the same purpose. One problem, I believe, which has never been
fully addressed is what happens when the subdirectory from which
git-bisect is run gets deleted as part of the bisection.

Here are a couple recent threads triggered by previous such patches
(but there are probably several more):

https://lore.kernel.org/git/pull.765.git.1603271344522.gitgitgadget@gmail.com/
https://lore.kernel.org/git/pull.736.git.git.1584868547682.gitgitgadget@gmail.com/

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-20 21:38 [PATCH] bisect: allow to run from subdirectories Roland Hieber
  2021-06-21  0:35 ` Ævar Arnfjörð Bjarmason
@ 2021-06-21  3:43 ` Junio C Hamano
  2021-06-23 23:40   ` Jeff King
  1 sibling, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2021-06-21  3:43 UTC (permalink / raw)
  To: Roland Hieber; +Cc: git, Vasco Almeida

Roland Hieber <rhi@pengutronix.de> writes:

> Currently, calling 'git bisect' from a directory other than the top
> level of a repository only comes up with an error message:
>
>     You need to run this command from the toplevel of the working tree.
>
> After a glance through the bisect code, there seems to be nothing that
> relies on the current working directory, and a few hours of bisect usage
> also didn't turn up any problems. Set the appropriate flag for
> git-sh-setup to remove the error message.

Try to find a history in which to run a bisect that

 (1) has a directory T in the recent part of the history, and

 (2) does not have that directory in the older part of the history.
     Better yet, if the older part of the history has T as a regular
     file, that would be ideal.  Even better, if that old regular
     file T was added, then removed, and then recreated, before it
     got turned into a directory.

Now, using the two commits you used to satisfy conditions (1) and
(2) as "bad" and "good", and using "bad - has T as a directory" and
"good - does not have T as a directory", as the bisection criterion,
try to find where "good" turns "bad"---in other words, find the
commit that either creates T as a directory or turns the regular
file T into a directory.

Perform that bisect from the subdirectory T.  Would that work?  I
suspect it wouldn't; when trying to check out an old version that
does not have T in the directory, either the checkout would fail
because it cannot remove T which has an active process (i.e. your
terminal session) in it, or your process sitting in an orphaned
directory (i.e. your ".." may still be the original top-level
directory that used to have T subdirectory, but "cd T" from the
top-level will not reach where you are).  All sorts of things can
go wrong and that is why we forbid it.  Just a single "cd" upfront
would save the user a lot of headache.

This does not depend on "do we have T as a directory?" being the
bisection criteria.  The important thing is that the current
directory would appear and disappear as the bisection process makes
you jump around in the history.

Hope this helps understanding the issue.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21  2:10     ` Eric Sunshine
@ 2021-06-21  9:33       ` Roland Hieber
  2021-06-21 12:45         ` Ævar Arnfjörð Bjarmason
  2021-06-22  0:09       ` brian m. carlson
  1 sibling, 1 reply; 13+ messages in thread
From: Roland Hieber @ 2021-06-21  9:33 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: brian m. carlson, Ævar Arnfjörð Bjarmason,
	Git List, Vasco Almeida, Junio C Hamano

On Sun, Jun 20, 2021 at 10:10:10PM -0400, Eric Sunshine wrote:
> On Sun, Jun 20, 2021 at 10:00 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > On 2021-06-21 at 00:35:49, Ævar Arnfjörð Bjarmason wrote:
> > > On Sun, Jun 20 2021, Roland Hieber wrote:
> > > > Currently, calling 'git bisect' from a directory other than the top
> > > > level of a repository only comes up with an error message:
> > > >
> > > >     You need to run this command from the toplevel of the working tree.
> > >
> > > How does this affect out-of-tree scripts that will be run with "git
> > > bisect run", is the cwd set to the root as they now might expect git to
> > > check, or whatever subdirectory you ran the "run" from?
> >
> > As for the idea itself, I think it's a good one assuming everything
> > continues to work.  It will certainly be more convenient for a lot of
> > people.
> 
> There have been multiple patches sent to the project over the years
> with the same purpose. One problem, I believe, which has never been
> fully addressed is what happens when the subdirectory from which
> git-bisect is run gets deleted as part of the bisection.
> 
> Here are a couple recent threads triggered by previous such patches
> (but there are probably several more):
> 
> https://lore.kernel.org/git/pull.765.git.1603271344522.gitgitgadget@gmail.com/
> https://lore.kernel.org/git/pull.736.git.git.1584868547682.gitgitgadget@gmail.com/

Ah, thanks for explaining the problem. Would a patch that adds a short
explanatory comment in git-bisect.sh on the matter help to prevent
people sending such patches?

 - Roland

-- 
Roland Hieber, Pengutronix e.K.          | r.hieber@pengutronix.de     |
Steuerwalder Str. 21                     | https://www.pengutronix.de/ |
31137 Hildesheim, Germany                | Phone: +49-5121-206917-0    |
Amtsgericht Hildesheim, HRA 2686         | Fax:   +49-5121-206917-5555 |

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21  9:33       ` Roland Hieber
@ 2021-06-21 12:45         ` Ævar Arnfjörð Bjarmason
  2021-06-21 20:02           ` Eric Sunshine
  2021-06-22 15:27           ` Ævar Arnfjörð Bjarmason
  0 siblings, 2 replies; 13+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-21 12:45 UTC (permalink / raw)
  To: Roland Hieber
  Cc: Eric Sunshine, brian m. carlson, Git List, Vasco Almeida,
	Junio C Hamano


On Mon, Jun 21 2021, Roland Hieber wrote:

> On Sun, Jun 20, 2021 at 10:10:10PM -0400, Eric Sunshine wrote:
>> On Sun, Jun 20, 2021 at 10:00 PM brian m. carlson
>> <sandals@crustytoothpaste.net> wrote:
>> > On 2021-06-21 at 00:35:49, Ævar Arnfjörð Bjarmason wrote:
>> > > On Sun, Jun 20 2021, Roland Hieber wrote:
>> > > > Currently, calling 'git bisect' from a directory other than the top
>> > > > level of a repository only comes up with an error message:
>> > > >
>> > > >     You need to run this command from the toplevel of the working tree.
>> > >
>> > > How does this affect out-of-tree scripts that will be run with "git
>> > > bisect run", is the cwd set to the root as they now might expect git to
>> > > check, or whatever subdirectory you ran the "run" from?
>> >
>> > As for the idea itself, I think it's a good one assuming everything
>> > continues to work.  It will certainly be more convenient for a lot of
>> > people.
>> 
>> There have been multiple patches sent to the project over the years
>> with the same purpose. One problem, I believe, which has never been
>> fully addressed is what happens when the subdirectory from which
>> git-bisect is run gets deleted as part of the bisection.
>> 
>> Here are a couple recent threads triggered by previous such patches
>> (but there are probably several more):
>> 
>> https://lore.kernel.org/git/pull.765.git.1603271344522.gitgitgadget@gmail.com/
>> https://lore.kernel.org/git/pull.736.git.git.1584868547682.gitgitgadget@gmail.com/
>
> Ah, thanks for explaining the problem. Would a patch that adds a short
> explanatory comment in git-bisect.sh on the matter help to prevent
> people sending such patches?

Having skimmed the linked discussions I don't think the consensus is
that this shouldn't exist, but that someone who wants it should do some
research on the relevant edge cases, come up with test cases for them,
discuss the trade-offs in a commit message etc.

I for one would welcome such a feature, it's often annoyed me, it should
just work like "rebase exec" in that a "run" script should cd to the
root, but (as discussed in the linked threads) I don't see why we'd
prevent it any more than several other commands that already have this
edge case, but don't explicitly prevent this.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21 12:45         ` Ævar Arnfjörð Bjarmason
@ 2021-06-21 20:02           ` Eric Sunshine
  2021-06-22 15:27           ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 13+ messages in thread
From: Eric Sunshine @ 2021-06-21 20:02 UTC (permalink / raw)
  To: Ævar Arnfjörð Bjarmason
  Cc: Roland Hieber, brian m. carlson, Git List, Vasco Almeida,
	Junio C Hamano

On Mon, Jun 21, 2021 at 8:50 AM Ævar Arnfjörð Bjarmason
<avarab@gmail.com> wrote:
> On Mon, Jun 21 2021, Roland Hieber wrote:
> > On Sun, Jun 20, 2021 at 10:10:10PM -0400, Eric Sunshine wrote:
> >> There have been multiple patches sent to the project over the years
> >> with the same purpose. One problem, I believe, which has never been
> >> fully addressed is what happens when the subdirectory from which
> >> git-bisect is run gets deleted as part of the bisection.
> >
> > Ah, thanks for explaining the problem. Would a patch that adds a short
> > explanatory comment in git-bisect.sh on the matter help to prevent
> > people sending such patches?
>
> Having skimmed the linked discussions I don't think the consensus is
> that this shouldn't exist, but that someone who wants it should do some
> research on the relevant edge cases, come up with test cases for them,
> discuss the trade-offs in a commit message etc.

Be that as it may, having a comment in the code explaining why it is
currently turned off and what needs to be accomplished before turning
it on might indeed be a good way to stem the flow of patches which
merely flip-the-switch without doing that extra research. Whether or
not Junio would accept such a patch documenting the current state of
affairs is a different question.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21  2:10     ` Eric Sunshine
  2021-06-21  9:33       ` Roland Hieber
@ 2021-06-22  0:09       ` brian m. carlson
  1 sibling, 0 replies; 13+ messages in thread
From: brian m. carlson @ 2021-06-22  0:09 UTC (permalink / raw)
  To: Eric Sunshine
  Cc: Ævar Arnfjörð Bjarmason, Roland Hieber, Git List,
	Vasco Almeida, Junio C Hamano

[-- Attachment #1: Type: text/plain, Size: 1817 bytes --]

On 2021-06-21 at 02:10:10, Eric Sunshine wrote:
> On Sun, Jun 20, 2021 at 10:00 PM brian m. carlson
> <sandals@crustytoothpaste.net> wrote:
> > On 2021-06-21 at 00:35:49, Ævar Arnfjörð Bjarmason wrote:
> > > On Sun, Jun 20 2021, Roland Hieber wrote:
> > > > Currently, calling 'git bisect' from a directory other than the top
> > > > level of a repository only comes up with an error message:
> > > >
> > > >     You need to run this command from the toplevel of the working tree.
> > >
> > > How does this affect out-of-tree scripts that will be run with "git
> > > bisect run", is the cwd set to the root as they now might expect git to
> > > check, or whatever subdirectory you ran the "run" from?
> >
> > As for the idea itself, I think it's a good one assuming everything
> > continues to work.  It will certainly be more convenient for a lot of
> > people.
> 
> There have been multiple patches sent to the project over the years
> with the same purpose. One problem, I believe, which has never been
> fully addressed is what happens when the subdirectory from which
> git-bisect is run gets deleted as part of the bisection.

And that's the thing I was missing.  This did seem a little too simple.

I think there are certainly cases where we know the directory isn't
changing; most of the situations in which I've bisected things in Git,
for example.  But we will, of course, need to specify the behavior when
that's not the case, and as Junio said, it probably will just fail in
unexpected ways, and that wouldn't be a helpful user experience.  At the
very least, we'd need to document the behavior, and ideally try to make
it work reasonably gracefully (e.g., by not removing the directory in
that case).
-- 
brian m. carlson (he/him or they/them)
Toronto, Ontario, CA

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 262 bytes --]

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21 12:45         ` Ævar Arnfjörð Bjarmason
  2021-06-21 20:02           ` Eric Sunshine
@ 2021-06-22 15:27           ` Ævar Arnfjörð Bjarmason
  1 sibling, 0 replies; 13+ messages in thread
From: Ævar Arnfjörð Bjarmason @ 2021-06-22 15:27 UTC (permalink / raw)
  To: Roland Hieber
  Cc: Eric Sunshine, brian m. carlson, Git List, Vasco Almeida,
	Junio C Hamano


On Mon, Jun 21 2021, Ævar Arnfjörð Bjarmason wrote:

> On Mon, Jun 21 2021, Roland Hieber wrote:
>
>> On Sun, Jun 20, 2021 at 10:10:10PM -0400, Eric Sunshine wrote:
>>> On Sun, Jun 20, 2021 at 10:00 PM brian m. carlson
>>> <sandals@crustytoothpaste.net> wrote:
>>> > On 2021-06-21 at 00:35:49, Ævar Arnfjörð Bjarmason wrote:
>>> > > On Sun, Jun 20 2021, Roland Hieber wrote:
>>> > > > Currently, calling 'git bisect' from a directory other than the top
>>> > > > level of a repository only comes up with an error message:
>>> > > >
>>> > > >     You need to run this command from the toplevel of the working tree.
>>> > >
>>> > > How does this affect out-of-tree scripts that will be run with "git
>>> > > bisect run", is the cwd set to the root as they now might expect git to
>>> > > check, or whatever subdirectory you ran the "run" from?
>>> >
>>> > As for the idea itself, I think it's a good one assuming everything
>>> > continues to work.  It will certainly be more convenient for a lot of
>>> > people.
>>> 
>>> There have been multiple patches sent to the project over the years
>>> with the same purpose. One problem, I believe, which has never been
>>> fully addressed is what happens when the subdirectory from which
>>> git-bisect is run gets deleted as part of the bisection.
>>> 
>>> Here are a couple recent threads triggered by previous such patches
>>> (but there are probably several more):
>>> 
>>> https://lore.kernel.org/git/pull.765.git.1603271344522.gitgitgadget@gmail.com/
>>> https://lore.kernel.org/git/pull.736.git.git.1584868547682.gitgitgadget@gmail.com/
>>
>> Ah, thanks for explaining the problem. Would a patch that adds a short
>> explanatory comment in git-bisect.sh on the matter help to prevent
>> people sending such patches?
>
> Having skimmed the linked discussions I don't think the consensus is
> that this shouldn't exist, but that someone who wants it should do some
> research on the relevant edge cases, come up with test cases for them,
> discuss the trade-offs in a commit message etc.
>
> I for one would welcome such a feature, it's often annoyed me, it should
> just work like "rebase exec" in that a "run" script should cd to the
> root, but (as discussed in the linked threads) I don't see why we'd
> prevent it any more than several other commands that already have this
> edge case, but don't explicitly prevent this.

There's also a related issue: It's not just "git bisect start" etc. that
have this problem, but also "git bisect log". No matter what we do with
run etc.

I see no reason for why "log" should not locate the relevant log in the
.git directory, same for "view", also "good", "bad" etc. E.g. one might
start a bisect session in one terminal, and be cd'd into the 't/'
directory in another, and then couldn't run "good/bad" there.


^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-21  3:43 ` Junio C Hamano
@ 2021-06-23 23:40   ` Jeff King
  2021-06-29  1:22     ` Junio C Hamano
  0 siblings, 1 reply; 13+ messages in thread
From: Jeff King @ 2021-06-23 23:40 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Roland Hieber, git, Vasco Almeida

On Mon, Jun 21, 2021 at 12:43:32PM +0900, Junio C Hamano wrote:

> Roland Hieber <rhi@pengutronix.de> writes:
> 
> > Currently, calling 'git bisect' from a directory other than the top
> > level of a repository only comes up with an error message:
> >
> >     You need to run this command from the toplevel of the working tree.
> >
> > After a glance through the bisect code, there seems to be nothing that
> > relies on the current working directory, and a few hours of bisect usage
> > also didn't turn up any problems. Set the appropriate flag for
> > git-sh-setup to remove the error message.
> 
> Try to find a history in which to run a bisect that
> 
>  (1) has a directory T in the recent part of the history, and
> 
>  (2) does not have that directory in the older part of the history.
>      Better yet, if the older part of the history has T as a regular
>      file, that would be ideal.  Even better, if that old regular
>      file T was added, then removed, and then recreated, before it
>      got turned into a directory.
> 
> Now, using the two commits you used to satisfy conditions (1) and
> (2) as "bad" and "good", and using "bad - has T as a directory" and
> "good - does not have T as a directory", as the bisection criterion,
> try to find where "good" turns "bad"---in other words, find the
> commit that either creates T as a directory or turns the regular
> file T into a directory.
> 
> Perform that bisect from the subdirectory T.  Would that work?  I
> suspect it wouldn't; when trying to check out an old version that
> does not have T in the directory, either the checkout would fail
> because it cannot remove T which has an active process (i.e. your
> terminal session) in it, or your process sitting in an orphaned
> directory (i.e. your ".." may still be the original top-level
> directory that used to have T subdirectory, but "cd T" from the
> top-level will not reach where you are).  All sorts of things can
> go wrong and that is why we forbid it.  Just a single "cd" upfront
> would save the user a lot of headache.
> 
> This does not depend on "do we have T as a directory?" being the
> bisection criteria.  The important thing is that the current
> directory would appear and disappear as the bisection process makes
> you jump around in the history.

I think that is a good explanation. But I remain somewhat unconvinced
that it is that big a problem in practice.

There are already other cases where checkout may fail (e.g., an
untracked build artifact in one commit conflicting with a tracked file
in another). E.g., this sequence in git.git:

  # here we build git-remote-testgit from its .sh counterpart
  git checkout 5afb2ce4cd^
  make

  # now imagine our bisect jumps forward; this one drops it from
  # the .gitignore, so its now a precious untracked file
  git checkout 5afb2ce4cd
  make

  # now jump backwards. I'm not sure if this can actually happen in
  # a bisection of git.git, since we went forward then back. But
  # possibly you could hit it in parallel lines of history[1], and
  # certainly a similar history which didn't update ".gitignore" at the
  # same time could run into it.
  git checkout 709a957d9493^

That last command yields:

  error: The following untracked working tree files would be overwritten by checkout:
	git-remote-testgit
  Please move or remove them before you switch branches.
  Aborting

Which is annoying, but it's pretty clear that you need to remove the
file and then re-run your "bisect good" or "bisect bad" (and if we want
to make it more clear, then git-bisect could notice that git-checkout
failed and add extra advice).

And I think any directory typechange shenanigans would end up with a
similar message. It's nice to avoid that ahead of time, but it comes at
the cost of bugging the user to preemptively "cd" to the toplevel _every
time_, even if they are not going to bisect through history where the
directory goes away.

So while I don't disagree with avoiding the confusing case, it seems
like the safety check is overly cautious. (Of course an alternative is
that it could actually examine the history to make sure $PWD never goes
away; I wonder if that would be annoyingly costly or not).

The more interesting case, I think, is when T is simply removed.  If
there are untracked files (even ignored build artifacts) in T, then the
directory sticks around anyway. But if not, Git is able to checkout the
original commit and deletes the directory. And then you get:

  $ cd builtin
  $ git checkout 81b50f3ce40bfdd66e5d967bf82be001039a9a98^
  $ git status
  fatal: Unable to read current working directory: No such file or directory

That's potentially more confusing, as things subtly don't work. But as
shown by the command above, it's not the exclusive domain of bisect.
Perhaps a poorly written bisect-run script could get confused. But it
still doesn't seem to me like it justifies the tradeoff of "we must
never allow bisect from a subdirectory, on the off chance that we might
run into this case".

If we did want try helping the user in this case, we could also diagnose
it pretty easily by confirming that $PWD will be in the destination
commit before checking it out, and complaining otherwise (which turns it
into the earlier case). In fact, regular "checkout" could perhaps do
that, too, as a courtesy (with "-f" overriding).

There may be other corner cases of interest (I didn't think too hard
about weird symlink cases, for example). But overall I think people are
more likely to be annoyed by the "must be at the toplevel" safety valve
than they are by "checkout failed midway through my bisect", if only
because the former happens a lot more frequently.

-Peff

[1] I'm sorry not to have produced a real git-bisect example that causes
    "checkout" to bail. My subjective recollection is that I have run
    into this problem with git-remote-testgit before during real-world
    use, but I'm not 100% sure it was while bisecting, and not simply
    sight-seeing around history.

^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-23 23:40   ` Jeff King
@ 2021-06-29  1:22     ` Junio C Hamano
  2021-06-29  2:00       ` Jeff King
  0 siblings, 1 reply; 13+ messages in thread
From: Junio C Hamano @ 2021-06-29  1:22 UTC (permalink / raw)
  To: Jeff King; +Cc: Roland Hieber, git, Vasco Almeida

Jeff King <peff@peff.net> writes:

>> This does not depend on "do we have T as a directory?" being the
>> bisection criteria.  The important thing is that the current
>> directory would appear and disappear as the bisection process makes
>> you jump around in the history.
>
> I think that is a good explanation. But I remain somewhat unconvinced
> that it is that big a problem in practice.

It's just the difference in attitude, I would think.  Things like
"rebase" take a more liberal attitude and most of the time things
work out OK because removal of a directory is a rare event and
replacement of a directory with a non-directory is even rarer, but
when things break there is no provision to help users to know how it
broke by diagnosing why the revision cannot be checked out, or why
the directory D the user's shell session is sitting in is now
orphaned and different from the directory D the user thinks he is in
because it was removed (while the user's process is in there) and
then recreated under the same name, or any of the tricky things.

The ideal endgame would be to allow operating from subdirectory
*AND* have provisions for helping users when things go wrong because
the starting subdirectory goes away.  "bisect" works under the more
conservative philosophy (start strict and forbid operation that we
know we didn't spend any effort to avoid taking the user into
dangerous waters---we can allow it later once we make it safer but
not until then).

It would involve a bit of chicken-and-egg, I would guess.  If we
think improving Git is more important than avoiding even occasional
failures imposed on end-users, then the more liberal approach would
be easier to work with---we can allow the command to start from
subdirectory even if we do not do anything to help avoid problems,
let the users hit a snag and have them report it, which would give
us some concrete failure mode to work on.




^ permalink raw reply	[flat|nested] 13+ messages in thread

* Re: [PATCH] bisect: allow to run from subdirectories
  2021-06-29  1:22     ` Junio C Hamano
@ 2021-06-29  2:00       ` Jeff King
  0 siblings, 0 replies; 13+ messages in thread
From: Jeff King @ 2021-06-29  2:00 UTC (permalink / raw)
  To: Junio C Hamano; +Cc: Roland Hieber, git, Vasco Almeida

On Mon, Jun 28, 2021 at 06:22:37PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> >> This does not depend on "do we have T as a directory?" being the
> >> bisection criteria.  The important thing is that the current
> >> directory would appear and disappear as the bisection process makes
> >> you jump around in the history.
> >
> > I think that is a good explanation. But I remain somewhat unconvinced
> > that it is that big a problem in practice.
> 
> It's just the difference in attitude, I would think.  Things like
> "rebase" take a more liberal attitude and most of the time things
> work out OK because removal of a directory is a rare event and
> replacement of a directory with a non-directory is even rarer, but
> when things break there is no provision to help users to know how it
> broke by diagnosing why the revision cannot be checked out, or why
> the directory D the user's shell session is sitting in is now
> orphaned and different from the directory D the user thinks he is in
> because it was removed (while the user's process is in there) and
> then recreated under the same name, or any of the tricky things.
> 
> The ideal endgame would be to allow operating from subdirectory
> *AND* have provisions for helping users when things go wrong because
> the starting subdirectory goes away.  "bisect" works under the more
> conservative philosophy (start strict and forbid operation that we
> know we didn't spend any effort to avoid taking the user into
> dangerous waters---we can allow it later once we make it safer but
> not until then).

Yes, I agree with this second paragraph. Just trying to create a
constructive path forward, I think I'd be comfortable with a patch
series that:

  - confirmed that bisect's behavior when checkout fails produces a
    reasonable error message that the user can act on (either from
    checkout itself, or perhaps extra advice from bisect when the
    checkout fails)

  - detected the case when the prefix we started from goes away as part
    of the checkout, and turned that into an error (rather than
    orphaning the user's cwd and leading to confusing results). This
    _might_ even be something that regular "git checkout" would benefit
    from, too. And I think should not be too expensive to implement (at
    least not after an admittedly moderate amount of thinking on it).

  - only then turn on SUBDIRECTORY_OK.

-Peff

^ permalink raw reply	[flat|nested] 13+ messages in thread

end of thread, other threads:[~2021-06-29  2:00 UTC | newest]

Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2021-06-20 21:38 [PATCH] bisect: allow to run from subdirectories Roland Hieber
2021-06-21  0:35 ` Ævar Arnfjörð Bjarmason
2021-06-21  2:00   ` brian m. carlson
2021-06-21  2:10     ` Eric Sunshine
2021-06-21  9:33       ` Roland Hieber
2021-06-21 12:45         ` Ævar Arnfjörð Bjarmason
2021-06-21 20:02           ` Eric Sunshine
2021-06-22 15:27           ` Ævar Arnfjörð Bjarmason
2021-06-22  0:09       ` brian m. carlson
2021-06-21  3:43 ` Junio C Hamano
2021-06-23 23:40   ` Jeff King
2021-06-29  1:22     ` Junio C Hamano
2021-06-29  2:00       ` Jeff King

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).