| Message ID | 20190219083123.27686-1-nbelakovski@gmail.com (mailing list archive) |
|---|---|
| Headers | show |
| Series | [v8,1/3] ref-filter: add worktreepath atom | expand |
On Tue, Feb 19, 2019 at 05:31:20PM +0900, nbelakovski@gmail.com wrote: > From: Nickolai Belakovski <nbelakovski@gmail.com> > > I've made the various cosmetic changes that were suggested, as well as adding tests for 3/3 > > I don't have a particularly strong opinion on the subject of keeping the atom as "worktreepath" > or changing it to "worktree:path". We did feel earlier in this thread that if we went with > "worktree:path", then "worktree" is somewhat ambiguous, and that discussion led to deciding to > have "worktree" return the path,. After that I chose to name it "worktreepath" because I like to > make things explicit and intuitive. I am OK with it either way. We have used ":" for some variants (e.g., objectsize:disk). But we have also used long single names with related prefixes (e.g., objectname versus objecttype versus objectsize). Patch 1 looks good to me. Given that we're on v8 and most of the other comments are for patches 2 and 3, I think we might consider graduating it separately if the other two are not ready soon. It's independently useful, IMHO. I have a few comments on the others which I'll leave as replies there. -Peff
> > Patch 1 looks good to me. Given that we're on v8 and most of the other > comments are for patches 2 and 3, I think we might consider graduating > it separately if the other two are not ready soon. It's independently > useful, IMHO. Patch 2 was my main motivation, so it would be nice to get it in together with 1 :) Patch 3, like I said in the thread on that one I don't have strong feeling about it. It was an attempt to provide a connection between the new cyan output and its intent, as opposed to having the user guess, but I think anyone who's using a worktree will figure it out sooner or later, and anyone not using a worktree will be unaffected. I'm willing to keep going with comments on patch 2. I can't imagine it would take many more revisions as it's much more straightforward than patch 1, it's basically just modifying one line of branch.c. If we decide to drop patch 3 fine with me. I'll send in v9 with the latest changes for both 2 and 3 sometime tomorrow unless I hear otherwise. Thanks for the feedback.
From: Nickolai Belakovski <nbelakovski@gmail.com> I've made the various cosmetic changes that were suggested, as well as adding tests for 3/3 I don't have a particularly strong opinion on the subject of keeping the atom as "worktreepath" or changing it to "worktree:path". We did feel earlier in this thread that if we went with "worktree:path", then "worktree" is somewhat ambiguous, and that discussion led to deciding to have "worktree" return the path,. After that I chose to name it "worktreepath" because I like to make things explicit and intuitive. Travis CI results: https://travis-ci.org/nbelakovski/git/builds/494817576 Nickolai Belakovski (3): ref-filter: add worktreepath atom branch: update output to include worktree info branch: add worktree info on verbose output Documentation/git-branch.txt | 12 ++++-- Documentation/git-for-each-ref.txt | 5 +++ builtin/branch.c | 16 ++++++-- ref-filter.c | 78 ++++++++++++++++++++++++++++++++++++++ t/t3200-branch.sh | 8 ++-- t/t3203-branch-output.sh | 38 +++++++++++++++++++ t/t6302-for-each-ref-filter.sh | 11 ++++++ 7 files changed, 156 insertions(+), 12 deletions(-)