* git commit file completion recently broke @ 2017-12-06 23:53 Jacob Keller 2017-12-07 0:01 ` Jacob Keller 0 siblings, 1 reply; 13+ messages in thread From: Jacob Keller @ 2017-12-06 23:53 UTC (permalink / raw) To: Git mailing list Hi, I'm still investigating, but thought I'd send an email. I recently updated to jch branch, and found that completion for git commit does not work as expected. If I have a git repository with a modified file in a subdirectiory, then git commit <TAB> produces the name of the subdirectory instead of the file names. This occurs regardless of where I run the git commit command. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-06 23:53 git commit file completion recently broke Jacob Keller @ 2017-12-07 0:01 ` Jacob Keller 2017-12-07 0:08 ` Jacob Keller 2017-12-07 0:22 ` Jeff King 0 siblings, 2 replies; 13+ messages in thread From: Jacob Keller @ 2017-12-07 0:01 UTC (permalink / raw) To: Git mailing list On Wed, Dec 6, 2017 at 3:53 PM, Jacob Keller <jacob.keller@gmail.com> wrote: > Hi, > > I'm still investigating, but thought I'd send an email. I recently > updated to jch branch, and found that completion for git commit does > not work as expected. > > If I have a git repository with a modified file in a subdirectiory, > then git commit <TAB> produces the name of the subdirectory instead of > the file names. > > This occurs regardless of where I run the git commit command. > > Thanks, > Jake I think I narrowed this down to "git diff-index --name-only --relative HEAD" producing a list of files *not* relative to the current directory. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 0:01 ` Jacob Keller @ 2017-12-07 0:08 ` Jacob Keller 2017-12-07 0:22 ` Jeff King 1 sibling, 0 replies; 13+ messages in thread From: Jacob Keller @ 2017-12-07 0:08 UTC (permalink / raw) To: Git mailing list On Wed, Dec 6, 2017 at 4:01 PM, Jacob Keller <jacob.keller@gmail.com> wrote: > On Wed, Dec 6, 2017 at 3:53 PM, Jacob Keller <jacob.keller@gmail.com> wrote: >> Hi, >> >> I'm still investigating, but thought I'd send an email. I recently >> updated to jch branch, and found that completion for git commit does >> not work as expected. >> >> If I have a git repository with a modified file in a subdirectiory, >> then git commit <TAB> produces the name of the subdirectory instead of >> the file names. >> >> This occurs regardless of where I run the git commit command. >> >> Thanks, >> Jake > > I think I narrowed this down to "git diff-index --name-only --relative > HEAD" producing a list of files *not* relative to the current > directory. > > Thanks, > Jake I've started a git bisect to see if i can find the source of the problem. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 0:01 ` Jacob Keller 2017-12-07 0:08 ` Jacob Keller @ 2017-12-07 0:22 ` Jeff King 2017-12-07 0:24 ` Jeff King 1 sibling, 1 reply; 13+ messages in thread From: Jeff King @ 2017-12-07 0:22 UTC (permalink / raw) To: Jacob Keller; +Cc: Christian Couder, Git mailing list On Wed, Dec 06, 2017 at 04:01:51PM -0800, Jacob Keller wrote: > I think I narrowed this down to "git diff-index --name-only --relative > HEAD" producing a list of files *not* relative to the current > directory. Hmm, my guess would have been something funny in the setup code forgetting our original prefix. But nope, it looks like the culprit is f7923a5ece (diff: use skip_to_optional_val(), 2017-12-04), which switched over parsing of "--relative". -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 0:22 ` Jeff King @ 2017-12-07 0:24 ` Jeff King 2017-12-07 0:38 ` Jacob Keller 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2017-12-07 0:24 UTC (permalink / raw) To: Jacob Keller; +Cc: Christian Couder, Git mailing list On Wed, Dec 06, 2017 at 07:22:35PM -0500, Jeff King wrote: > On Wed, Dec 06, 2017 at 04:01:51PM -0800, Jacob Keller wrote: > > > I think I narrowed this down to "git diff-index --name-only --relative > > HEAD" producing a list of files *not* relative to the current > > directory. > > Hmm, my guess would have been something funny in the setup code > forgetting our original prefix. > > But nope, it looks like the culprit is f7923a5ece (diff: use > skip_to_optional_val(), 2017-12-04), which switched over parsing of > "--relative". Oh, actually, I guess I was half-right. It feeds &options->prefix as the "default", meaning that we overwrite it with the empty string. I don't think "--relative" works for the semantics of skip_to_optional_value, since it needs: --relative=foo: set prefix to "foo" --relative: leave prefix untouched -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 0:24 ` Jeff King @ 2017-12-07 0:38 ` Jacob Keller 2017-12-07 0:56 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Jacob Keller @ 2017-12-07 0:38 UTC (permalink / raw) To: Jeff King; +Cc: Christian Couder, Git mailing list On Wed, Dec 6, 2017 at 4:24 PM, Jeff King <peff@peff.net> wrote: > On Wed, Dec 06, 2017 at 07:22:35PM -0500, Jeff King wrote: > >> On Wed, Dec 06, 2017 at 04:01:51PM -0800, Jacob Keller wrote: >> >> > I think I narrowed this down to "git diff-index --name-only --relative >> > HEAD" producing a list of files *not* relative to the current >> > directory. >> >> Hmm, my guess would have been something funny in the setup code >> forgetting our original prefix. >> >> But nope, it looks like the culprit is f7923a5ece (diff: use >> skip_to_optional_val(), 2017-12-04), which switched over parsing of >> "--relative". > > Oh, actually, I guess I was half-right. It feeds &options->prefix as the > "default", meaning that we overwrite it with the empty string. I don't > think "--relative" works for the semantics of skip_to_optional_value, > since it needs: > > --relative=foo: set prefix to "foo" > > --relative: leave prefix untouched > > -Peff Yep, and apparently our test suite completely lacked any tests of --relative on its own. I've sent a patch to add some tests. I don't know the exact best way to fix this, I guess we could just revert it the changes to relative... but maybe we could add or modify the semantics of skip_to_optional_val()?? What if it was changed so that it left the value alone if no value was provided? This would require callers to pre-set the value they want as default, but that would solve relative's problem. I'll look into that. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 0:38 ` Jacob Keller @ 2017-12-07 0:56 ` Jeff King 2017-12-07 1:04 ` Jacob Keller 2017-12-07 8:14 ` Christian Couder 0 siblings, 2 replies; 13+ messages in thread From: Jeff King @ 2017-12-07 0:56 UTC (permalink / raw) To: Jacob Keller; +Cc: Christian Couder, Git mailing list On Wed, Dec 06, 2017 at 04:38:29PM -0800, Jacob Keller wrote: > >> But nope, it looks like the culprit is f7923a5ece (diff: use > >> skip_to_optional_val(), 2017-12-04), which switched over parsing of > >> "--relative". > > > > Oh, actually, I guess I was half-right. It feeds &options->prefix as the > > "default", meaning that we overwrite it with the empty string. I don't > > think "--relative" works for the semantics of skip_to_optional_value, > > since it needs: > > > > --relative=foo: set prefix to "foo" > > > > --relative: leave prefix untouched > > > > -Peff > > Yep, and apparently our test suite completely lacked any tests of > --relative on its own. > > I've sent a patch to add some tests. Great. I was also saddened by our lack of tests. > I don't know the exact best way to fix this, I guess we could just > revert it the changes to relative... but maybe we could add or modify > the semantics of skip_to_optional_val()?? What if it was changed so > that it left the value alone if no value was provided? This would > require callers to pre-set the value they want as default, but that > would solve relative's problem. I think that would work for this case. But just looking at others from the same series, I think they'd get pretty awkward. For instance we now have: else if (!strcmp(arg, "--color)) options->use_color = 1; else if (skip_prefix(arg, "--color=", &arg)) /* parse "arg" as colorbool */ which became: else if (skip_to_optional_val_default(arg, "--color", &arg, "always")) /* parse "arg" as colorbool */ How would that look with the "leave it alone instead of assigning a default" semantics? It gets pretty clumsy, because you have to pre-assign "always" to some pointer. But then we can't reuse "arg", so we end up with something more like: const char *color_val = "always"; ... else if (skip_to_optional_val(arg, "--color", &color_val)) But we need one such "color_val" for every option we test for, and we have to set all of them up before any matches (because we don't know which one we'll actually match). Yuck. I think we'd do better to just assign NULL when there's "=", so we can tell the difference between "--relative", "--relative=", and "--relative=foo" (all of which are distinct). I think that's possible with the current scheme by doing: else if (skip_to_optional_val_default(arg, "--relative", &arg, NULL)) { options->flags.relative_name = 1; if (arg) options->prefix = arg; } IOW, the problem isn't in the design of the skip function, but just how it was used in this particular case. I do think it may make sense for the "short" one to use NULL, like: skip_to_optional_val(arg, "--relative, &arg) but maybe some other callers would be more inconvenienced (they may have to current NULL back into the empty string if they want to string "--foo" the same as "--foo="). -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 0:56 ` Jeff King @ 2017-12-07 1:04 ` Jacob Keller 2017-12-07 1:08 ` Jeff King 2017-12-07 8:14 ` Christian Couder 1 sibling, 1 reply; 13+ messages in thread From: Jacob Keller @ 2017-12-07 1:04 UTC (permalink / raw) To: Jeff King; +Cc: Christian Couder, Git mailing list On Wed, Dec 6, 2017 at 4:56 PM, Jeff King <peff@peff.net> wrote: > On Wed, Dec 06, 2017 at 04:38:29PM -0800, Jacob Keller wrote: > >> >> But nope, it looks like the culprit is f7923a5ece (diff: use >> >> skip_to_optional_val(), 2017-12-04), which switched over parsing of >> >> "--relative". >> > >> > Oh, actually, I guess I was half-right. It feeds &options->prefix as the >> > "default", meaning that we overwrite it with the empty string. I don't >> > think "--relative" works for the semantics of skip_to_optional_value, >> > since it needs: >> > >> > --relative=foo: set prefix to "foo" >> > >> > --relative: leave prefix untouched >> > >> > -Peff >> >> Yep, and apparently our test suite completely lacked any tests of >> --relative on its own. >> >> I've sent a patch to add some tests. > > Great. I was also saddened by our lack of tests. > >> I don't know the exact best way to fix this, I guess we could just >> revert it the changes to relative... but maybe we could add or modify >> the semantics of skip_to_optional_val()?? What if it was changed so >> that it left the value alone if no value was provided? This would >> require callers to pre-set the value they want as default, but that >> would solve relative's problem. > > I think that would work for this case. But just looking at others from > the same series, I think they'd get pretty awkward. For instance we now > have: > That obviously won't work for any case which sues skip_to_optional_val_default() (since these provide a default value to give in case none is provided. > else if (!strcmp(arg, "--color)) > options->use_color = 1; > else if (skip_prefix(arg, "--color=", &arg)) > /* parse "arg" as colorbool */ > > which became: > > else if (skip_to_optional_val_default(arg, "--color", &arg, "always")) > /* parse "arg" as colorbool */ > > How would that look with the "leave it alone instead of assigning a > default" semantics? It gets pretty clumsy, because you have to > pre-assign "always" to some pointer. But then we can't reuse "arg", so > we end up with something more like: > > const char *color_val = "always"; > ... > else if (skip_to_optional_val(arg, "--color", &color_val)) > It obviously wouldn't. The only sensible solution is to have "skip_to_optional_val_something()" which does this new behavior. Or, change skip_to_optional_val() behave this new way, but skip_to_optional_val_default() behave in the current way. > But we need one such "color_val" for every option we test for, and we > have to set all of them up before any matches (because we don't know > which one we'll actually match). Yuck. > > I think we'd do better to just assign NULL when there's "=", so we can > tell the difference between "--relative", "--relative=", and > "--relative=foo" (all of which are distinct). > > I think that's possible with the current scheme by doing: > > else if (skip_to_optional_val_default(arg, "--relative", &arg, NULL)) { > options->flags.relative_name = 1; > if (arg) > options->prefix = arg; > } > > IOW, the problem isn't in the design of the skip function, but just how > it was used in this particular case. I do think it may make sense for > the "short" one to use NULL, like: > > skip_to_optional_val(arg, "--relative, &arg) > > but maybe some other callers would be more inconvenienced (they may have > to current NULL back into the empty string if they want to string > "--foo" the same as "--foo="). > > -Peff What you outlined above is probably the best we can do. We could even add some extra helper which does that for us if we want. I sent a patch that merely reverts the change to --relative and adds a test for now though. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 1:04 ` Jacob Keller @ 2017-12-07 1:08 ` Jeff King 0 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2017-12-07 1:08 UTC (permalink / raw) To: Jacob Keller; +Cc: Christian Couder, Git mailing list On Wed, Dec 06, 2017 at 05:04:02PM -0800, Jacob Keller wrote: > What you outlined above is probably the best we can do. We could even > add some extra helper which does that for us if we want. > > I sent a patch that merely reverts the change to --relative and adds a > test for now though. Thanks. I'm fine with reverting --relative to its original form, or having it do the NULL thing I mentioned. Christian can decide, since it's his series. I do think we may want to take the test improvement as a separate patch. -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 0:56 ` Jeff King 2017-12-07 1:04 ` Jacob Keller @ 2017-12-07 8:14 ` Christian Couder 2017-12-07 8:19 ` Jeff King 2017-12-07 15:24 ` Junio C Hamano 1 sibling, 2 replies; 13+ messages in thread From: Christian Couder @ 2017-12-07 8:14 UTC (permalink / raw) To: Jeff King; +Cc: Jacob Keller, Git mailing list On Thu, Dec 7, 2017 at 1:56 AM, Jeff King <peff@peff.net> wrote: > I think we'd do better to just assign NULL when there's "=", so we can > tell the difference between "--relative", "--relative=", and > "--relative=foo" (all of which are distinct). > > I think that's possible with the current scheme by doing: > > else if (skip_to_optional_val_default(arg, "--relative", &arg, NULL)) { > options->flags.relative_name = 1; > if (arg) > options->prefix = arg; > } Yeah, that is a possible fix. > IOW, the problem isn't in the design of the skip function, but just how > it was used in this particular case. I agree. > I do think it may make sense for > the "short" one to use NULL, like: > > skip_to_optional_val(arg, "--relative, &arg) > > but maybe some other callers would be more inconvenienced (they may have > to current NULL back into the empty string if they want to string > "--foo" the same as "--foo="). I discussed that with Junio and yeah there are many callers that want "--foo" to be the same as "--foo=". By the way I wonder if "--relative=" makes any sense, and if we should emit a warning or just die in this case. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 8:14 ` Christian Couder @ 2017-12-07 8:19 ` Jeff King 2017-12-07 15:24 ` Junio C Hamano 1 sibling, 0 replies; 13+ messages in thread From: Jeff King @ 2017-12-07 8:19 UTC (permalink / raw) To: Christian Couder; +Cc: Jacob Keller, Git mailing list On Thu, Dec 07, 2017 at 09:14:31AM +0100, Christian Couder wrote: > > I do think it may make sense for > > the "short" one to use NULL, like: > > > > skip_to_optional_val(arg, "--relative, &arg) > > > > but maybe some other callers would be more inconvenienced (they may have > > to current NULL back into the empty string if they want to string > > "--foo" the same as "--foo="). Oof, I lost all ability to type in that last sentence. :) It looks like you deciphered my meaning, though. > I discussed that with Junio and yeah there are many callers that want > "--foo" to be the same as "--foo=". Yeah, if this is the only case, then just calling the "_default" variant with NULL for this instance makes sense to me. > By the way I wonder if "--relative=" makes any sense, and if we should > emit a warning or just die in this case. I also wondered about that. It does function as "relative to the root of the tree". But of course if you want that you can just omit "--relative". Still, it could be a minor convenience for somebody who is filling in "--relative" in a script. That's reaching, I think, but I don't see any particular reason to _forbid_ it (especially since it has worked for many years). -Peff ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 8:14 ` Christian Couder 2017-12-07 8:19 ` Jeff King @ 2017-12-07 15:24 ` Junio C Hamano 2017-12-07 18:57 ` Jacob Keller 1 sibling, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2017-12-07 15:24 UTC (permalink / raw) To: Christian Couder; +Cc: Jeff King, Jacob Keller, Git mailing list Christian Couder <christian.couder@gmail.com> writes: >> I do think it may make sense for >> the "short" one to use NULL, like: >> >> skip_to_optional_val(arg, "--relative, &arg) >> >> but maybe some other callers would be more inconvenienced (they may have >> to current NULL back into the empty string if they want to string >> "--foo" the same as "--foo="). > > I discussed that with Junio and yeah there are many callers that want > "--foo" to be the same as "--foo=". Yup, the original thread has details and me saying that assuming all of them want --foo and --foo= the same is questionable. The likely fix would be to use the _default variant with NULL, which was added exactly for cases like this. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: git commit file completion recently broke 2017-12-07 15:24 ` Junio C Hamano @ 2017-12-07 18:57 ` Jacob Keller 0 siblings, 0 replies; 13+ messages in thread From: Jacob Keller @ 2017-12-07 18:57 UTC (permalink / raw) To: Junio C Hamano; +Cc: Christian Couder, Jeff King, Git mailing list On Thu, Dec 7, 2017 at 7:24 AM, Junio C Hamano <gitster@pobox.com> wrote: > Christian Couder <christian.couder@gmail.com> writes: > >>> I do think it may make sense for >>> the "short" one to use NULL, like: >>> >>> skip_to_optional_val(arg, "--relative, &arg) >>> >>> but maybe some other callers would be more inconvenienced (they may have >>> to current NULL back into the empty string if they want to string >>> "--foo" the same as "--foo="). >> >> I discussed that with Junio and yeah there are many callers that want >> "--foo" to be the same as "--foo=". > > Yup, the original thread has details and me saying that assuming all > of them want --foo and --foo= the same is questionable. The likely > fix would be to use the _default variant with NULL, which was added > exactly for cases like this. > Slightly more complex. You have to use the _default variant, pass in arg instead of options->prefix, and then make sure arg was set before overwriting options->prefix. If you just use _default with NULL, it will not quite fix the problem. Thanks, Jake ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2017-12-07 18:57 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2017-12-06 23:53 git commit file completion recently broke Jacob Keller 2017-12-07 0:01 ` Jacob Keller 2017-12-07 0:08 ` Jacob Keller 2017-12-07 0:22 ` Jeff King 2017-12-07 0:24 ` Jeff King 2017-12-07 0:38 ` Jacob Keller 2017-12-07 0:56 ` Jeff King 2017-12-07 1:04 ` Jacob Keller 2017-12-07 1:08 ` Jeff King 2017-12-07 8:14 ` Christian Couder 2017-12-07 8:19 ` Jeff King 2017-12-07 15:24 ` Junio C Hamano 2017-12-07 18:57 ` Jacob Keller
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).