Re: [PATCH/WIP 0/9] for-each-ref format improvements
Nguyễn Thái Ngọc Duy writes: > This series introduces: > > - %(current), which either shows "*" if the ref is pointed by HEAD >or a space. Junio called it %(headness). I don't like that. >I don't like %(current) either but we have to start somewhere. >Name suggestion? %(marker)?? "marker" will paint us into a corner we cannot get out of when we want to mark refs with different criteria later. At least "current" tells you what kind of marker we are talking about and is 1000 times better. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 0/9] for-each-ref format improvements
Nguyễn Thái Ngọc Duy writes: > Originally I wanted to introduce --pretty with git-log's pretty syntax > to for-each-ref, deprecating --format. If you are going to unify the two mechanisms, I think the "--format" option of "for-each-ref" needs to become a superset of what the "--pretty" option of "log/rev-list" can express and shared between the two. The former has to handle anything that can be at the tip of a ref, including commits, so it needs to be able to do anything the latter can. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 0/9] for-each-ref format improvements
Duy Nguyen wrote: > I don't think you can easily borrow parsing code from pretty-formats > (but I may be wrong). Anyway new stuff with new syntax would look > alien in for-each-ref format lines. Either we bring --pretty to > for-each-ref, leaving all for-each-ref atoms behind in --format, or we > should follow %(..) convention if we add new stuff to --format. Why so extremist? pretty-formats has %(...), %C(...) as well as %..., so why shouldn't we? Our format is undocumented, and I doubt anyone even uses it; we're not breaking anyone's expectations. I'm just saying that our format can be a little reminiscent of pretty-formats, nothing more. There's no need to borrow parsing code: we can do it ourselves, I think. There is no need to go to the other extreme and throw out the existing --format and start out with a --pretty from scratch either: the current code isn't so bad that we can't build on top of it. Sure, we can eventually deprecate --format and move to --pretty for consistency (but that's a long-term goal). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 0/9] for-each-ref format improvements
On Sun, May 19, 2013 at 7:08 PM, Ramkumar Ramachandra wrote: >> "branch -vv" shows [upstream: ahead x, behind y]. We need a syntax to >> cover that too. > > Can't we construct that using [%(upstream:short): %(upstream:diff)]? > It's nothing fundamental. If there is no upstream, [] should not be shown. We don't have conditional specifiers so [] should be produced by one of the specifiers. But yes it's nothing fundamental. >> pretty and for-each-ref format seem to be on the opposite: one is >> terse, one verbose. Unless you are going to introduce a lot of new >> specifiers (and in the worst case, bring all pretty specifiers over, >> unify underlying code), I think we should stick with %(xx) convention. > > We can stick to using the existing %(...) atoms: there's no need to go > as far as %an versus %aN. The atoms cannot be consistent with > pretty-formats anyway, because pretty-formats has completely different > atoms. For the _new_ stuff like color and alignment, we can be > consistent with pretty-formats, no? I don't think you can easily borrow parsing code from pretty-formats (but I may be wrong). Anyway new stuff with new syntax would look alien in for-each-ref format lines. Either we bring --pretty to for-each-ref, leaving all for-each-ref atoms behind in --format, or we should follow %(..) convention if we add new stuff to --format. > >>> Why should we deviate from the pretty case? What is different here? >> >> Laziness plays a big factor :) So again, you want to take over? ;) > > It's just a matter of modifying the parsing/printing layer, instead of > introducing new atoms in the current parser. Doesn't $gmane/224692 > demonstrate that the former can, in fact, be easier? Yes it looks so. > >> it uses builtin/branch.c:branch_use_color. Eventually >> fill_tracking_info() should be moved to for-each-ref.c and pass >> branch_use_color in as an argument. But for now, I just leave it >> broken. > > Auto-color can come later: it's not that urgent. What's more > important is that we give users the flexibility to set their own > colors now. > > Can you give me a pull URL to your series, so we can start > collaborating? Mine's at gh:artagnon/git (branch: hot-branch). https://github.com/pclouds/git/tree/for-each-ref-pretty I merged 07/09 and 08/09 together as they should be one. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 0/9] for-each-ref format improvements
Duy Nguyen wrote: > Hmm.. I missed that mail (or I wouldn't have worked on this already). > Do you want to take over? Oh, we can collaborate on one beautiful series :) > "branch -vv" shows [upstream: ahead x, behind y]. We need a syntax to > cover that too. Can't we construct that using [%(upstream:short): %(upstream:diff)]? It's nothing fundamental. > pretty and for-each-ref format seem to be on the opposite: one is > terse, one verbose. Unless you are going to introduce a lot of new > specifiers (and in the worst case, bring all pretty specifiers over, > unify underlying code), I think we should stick with %(xx) convention. We can stick to using the existing %(...) atoms: there's no need to go as far as %an versus %aN. The atoms cannot be consistent with pretty-formats anyway, because pretty-formats has completely different atoms. For the _new_ stuff like color and alignment, we can be consistent with pretty-formats, no? >> Why should we deviate from the pretty case? What is different here? > > Laziness plays a big factor :) So again, you want to take over? ;) It's just a matter of modifying the parsing/printing layer, instead of introducing new atoms in the current parser. Doesn't $gmane/224692 demonstrate that the former can, in fact, be easier? > it uses builtin/branch.c:branch_use_color. Eventually > fill_tracking_info() should be moved to for-each-ref.c and pass > branch_use_color in as an argument. But for now, I just leave it > broken. Auto-color can come later: it's not that urgent. What's more important is that we give users the flexibility to set their own colors now. Can you give me a pull URL to your series, so we can start collaborating? Mine's at gh:artagnon/git (branch: hot-branch). -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 0/9] for-each-ref format improvements
On Sun, May 19, 2013 at 6:11 PM, Ramkumar Ramachandra wrote: > Nguyễn Thái Ngọc Duy wrote: >> The purpose of this series is to make "for-each-ref --format" powerful >> enough to display what "branch -v" and "branch -vv" do so that we >> could get rid of those display code and use for-each-ref code instead. > > Damn, you beat me to it. I just introduced color, and was working on > alignment. See $gmane/224692. Hmm.. I missed that mail (or I wouldn't have worked on this already). Do you want to take over? >> - %(tracking[:upstream]) gives us the exact output that branch -v[v] >>does. %(upstream) does not include []. We can't change its >>semantics. > > There's already an atom called "upstream", and "upstream:short" works. > Why not introduce "upstream:diff" for "[ahead x, behind y]" and > "upstream:shortdiff" for "<>" (like in the prompt)? "branch -vv" shows [upstream: ahead x, behind y]. We need a syntax to cover that too. >> - %(color:...) is pretty much the same as %C family in pretty code. >>I haven't added code for %(color:foo) == %C(foo) yet. There's a >>potential ambiguity here: %C(red) == %Cred or %C(red)?? > > I'd vote for dropping %C altogether and just go with %C(). > Why do we need %(color:) at all? pretty and for-each-ref format seem to be on the opposite: one is terse, one verbose. Unless you are going to introduce a lot of new specifiers (and in the worst case, bring all pretty specifiers over, unify underlying code), I think we should stick with %(xx) convention. >> - %(...:aligned) to do left aligning. I'm not entirely sure about >>this. We might be able to share code with %>, %< and %>< from >>pretty.c. But we need improvements there too because in >>for-each-ref case, we could calculate column width but %< would >>require the user to specify the width. > > Yeah, I think we should go with the %> and %< you introduced in > pretty.c. Yes, I want to be able to specify width. I still think we should follow %(...), e.g. %(left:N), %(right:N) as equivalent of %< and %>... >>Do people expect fancy layout with for-each-ref (and branch)? If so >>we might need to have %(align) or something instead of the simple >>left alignment case in %(...:aligned) > > Why should we deviate from the pretty case? What is different here? Laziness plays a big factor :) So again, you want to take over? ;) >> - We may need an equivalent of the space following % in pretty >>format. If the specifier produces something, then prepend a space, >>otherwise produce nothing. Do it like %C( tracking) vs >>%C(tracking)?? > > Yeah, sounds good. > >> You can try this after applying the series, which should give you the >> about close to 'branch -v'. %(tracking) coloring does not work though. > > Why doesn't %(tracking) coloring work? it uses builtin/branch.c:branch_use_color. Eventually fill_tracking_info() should be moved to for-each-ref.c and pass branch_use_color in as an argument. But for now, I just leave it broken. -- Duy -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 0/9] for-each-ref format improvements
On Sun, May 19, 2013 at 6:11 AM, Ramkumar Ramachandra wrote: > Yes, I think this is the direction we should be taking. Poorly > thought-out stuff like -v and -vv should be deprecated. Of course not. They are useful and user-friendly. The only question is what should be the format by default. > Also, I think it'll help to have a --pretty="format:" > equivalent to --format="" so that we can introduce pretty > names like oneline, short, medium, full. We can eventually deprecate > --format for consistency. I don't see the point of --pretty. I think there should be shortcuts, something like --hash for the SHA1s only, and --names for the refs only, or something like that. -- Felipe Contreras -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
Re: [PATCH/WIP 0/9] for-each-ref format improvements
Nguyễn Thái Ngọc Duy wrote: > The purpose of this series is to make "for-each-ref --format" powerful > enough to display what "branch -v" and "branch -vv" do so that we > could get rid of those display code and use for-each-ref code instead. Damn, you beat me to it. I just introduced color, and was working on alignment. See $gmane/224692. Yes, I think this is the direction we should be taking. Poorly thought-out stuff like -v and -vv should be deprecated. > This series introduces: > > - %(current), which either shows "*" if the ref is pointed by HEAD >or a space. Junio called it %(headness). I don't like that. >I don't like %(current) either but we have to start somewhere. >Name suggestion? %(marker)?? How about %(HEAD)? > - %(tracking[:upstream]) gives us the exact output that branch -v[v] >does. %(upstream) does not include []. We can't change its >semantics. There's already an atom called "upstream", and "upstream:short" works. Why not introduce "upstream:diff" for "[ahead x, behind y]" and "upstream:shortdiff" for "<>" (like in the prompt)? > - %(color:...) is pretty much the same as %C family in pretty code. >I haven't added code for %(color:foo) == %C(foo) yet. There's a >potential ambiguity here: %C(red) == %Cred or %C(red)?? I'd vote for dropping %C altogether and just go with %C(). Why do we need %(color:) at all? > - %(...:aligned) to do left aligning. I'm not entirely sure about >this. We might be able to share code with %>, %< and %>< from >pretty.c. But we need improvements there too because in >for-each-ref case, we could calculate column width but %< would >require the user to specify the width. Yeah, I think we should go with the %> and %< you introduced in pretty.c. Yes, I want to be able to specify width. >Do people expect fancy layout with for-each-ref (and branch)? If so >we might need to have %(align) or something instead of the simple >left alignment case in %(...:aligned) Why should we deviate from the pretty case? What is different here? > - We may need an equivalent of the space following % in pretty >format. If the specifier produces something, then prepend a space, >otherwise produce nothing. Do it like %C( tracking) vs >%C(tracking)?? Yeah, sounds good. > You can try this after applying the series, which should give you the > about close to 'branch -v'. %(tracking) coloring does not work though. Why doesn't %(tracking) coloring work? > Nguyễn Thái Ngọc Duy (9): I'll have a look at this. Also, I think it'll help to have a --pretty="format:" equivalent to --format="" so that we can introduce pretty names like oneline, short, medium, full. We can eventually deprecate --format for consistency. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
[PATCH/WIP 0/9] for-each-ref format improvements
The purpose of this series is to make "for-each-ref --format" powerful enough to display what "branch -v" and "branch -vv" do so that we could get rid of those display code and use for-each-ref code instead. The benefits are clear: share more code, branch can also borrow --sort and --format from for-each-ref, which should satisty users who are not happy with what -v and -vv provides. This version has not gotten there yet. I just want to post quick and dirty changes and try to address some design issues. Originally I wanted to introduce --pretty with git-log's pretty syntax to for-each-ref, deprecating --format. But because --format has to be there forever, we cannot remove its code, and the code change to make pretty code ready for displaying refs may not be small. So I went with enhancing --format syntax instead. This series introduces: - %(current), which either shows "*" if the ref is pointed by HEAD or a space. Junio called it %(headness). I don't like that. I don't like %(current) either but we have to start somewhere. Name suggestion? %(marker)?? - %(tracking[:upstream]) gives us the exact output that branch -v[v] does. %(upstream) does not include []. We can't change its semantics. - %(color:...) is pretty much the same as %C family in pretty code. I haven't added code for %(color:foo) == %C(foo) yet. There's a potential ambiguity here: %C(red) == %Cred or %C(red)?? - %(...:aligned) to do left aligning. I'm not entirely sure about this. We might be able to share code with %>, %< and %>< from pretty.c. But we need improvements there too because in for-each-ref case, we could calculate column width but %< would require the user to specify the width. Do people expect fancy layout with for-each-ref (and branch)? If so we might need to have %(align) or something instead of the simple left alignment case in %(...:aligned) - We may need an equivalent of the space following % in pretty format. If the specifier produces something, then prepend a space, otherwise produce nothing. Do it like %C( tracking) vs %C(tracking)?? You can try this after applying the series, which should give you the about close to 'branch -v'. %(tracking) coloring does not work though. git for-each-ref --format='%(current) %(color:auto)%(refname:short:aligned)%(color:reset) %%(objectname:short) %(tracking) %(subject)' 'refs/heads/*' Nguyễn Thái Ngọc Duy (9): quote.c: make sq_quote_print a slight wrapper of sq_quote_buf for-each-ref: convert to use *_quote_buf instead of _quote_print for-each-ref: avoid printing each element directly to stdout for-each-ref: add %(current) for current branch marker for-each-ref: add %(tracking[:upstream]) for tracking info for-each-ref: add %(color:...) for-each-ref: prepoplulate all atoms before show_ref() for-each-ref: merge show_ref into show_refs for-each-ref: support %(...:aligned) for left alignment branch.h | 11 builtin/branch.c | 16 ++ builtin/for-each-ref.c | 146 ++--- quote.c| 61 + quote.h| 6 +- 5 files changed, 170 insertions(+), 70 deletions(-) -- 1.8.2.83.gc99314b -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majord...@vger.kernel.org More majordomo info at http://vger.kernel.org/majordomo-info.html
