Re: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
On 05/14/2013 04:24 PM, Johan Herland wrote: On Mon, May 13, 2013 at 10:34 PM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Johan Herland jo...@herland.net writes: Obviously, I named it '%1' since it expands into the _first_ component of the (slash-separated) shorthand. OK, I can buy something like %* refs/%* refs/heads/%* ... refs/remotes/%*/HEAD refs/remotes/%1/%2 refs/peers/%1/heads/%2 that is, for a pattern that has %*, we feed the end-user string as a whole, and for a pattern that has %1 thru %N, we find an appropriate way to chop the end-user string into N pieces (e.g. nick/name would be split into %1 = nick, %2 = name, while foo/bar/baz might have two possibilities, %1, %2 = foo, bar/baz or foo/bar, baz). The earlier ones on the above list can even be written with their %* substituted with %1 if we go that route. Just to make sure. Please do not let two possibilities stop you. As I said in the nearby thread, I do not necessarily insist that we must try all N possibilities. We find an appropriate way could be just we always chop at the first slash, and %1 is what comes before it, %2 is what comes after it. That will close the possibility for us to use %1 thru %N (N is limited to 2), but it still is We have %1 and we have %2, both fall into the same 'path components, numbered from left to right' category, and justifies the use of %1 (one, not el). So still no objection to %1 from me. I think I like refs/peers/%1/heads/%* better than refs/peers/%1/heads/%2, since the latter sort of makes me wonder whether the 3rd, 4th, etc. components would be discarded. That said, having %* mean the rest of the shorthand means that we must adjust the expansion of %* for every preceding %N, which prevents us from simplifying the code by using strbuf_expand_dict_cb() with a static dictionary [1]. I am not sure why we would want refs/remotes/%1/%2 instead of refs/remote/%*. Maybe I've been staring at this for too long, but I find the latter shorter and more descriptive and the %1/%2 notation needlessly cumbersome, especially if it's also supposed to match foo/bar/baz refs/remotes/%1/%2 (or refs/remotes/%1/%*) might be a nice way to imply that the rule should only be attempted if the input has at least two components, whereas something like refs/heads/%* would be applied even for inputs with no slashes. Michael -- Michael Haggerty mhag...@alum.mit.edu http://softwareswirl.blogspot.com/ -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty mhag...@alum.mit.edu wrote: On 05/14/2013 04:24 PM, Johan Herland wrote: I am not sure why we would want refs/remotes/%1/%2 instead of refs/remote/%*. Maybe I've been staring at this for too long, but I find the latter shorter and more descriptive and the %1/%2 notation needlessly cumbersome, especially if it's also supposed to match foo/bar/baz refs/remotes/%1/%2 (or refs/remotes/%1/%*) might be a nice way to imply that the rule should only be attempted if the input has at least two components, whereas something like refs/heads/%* would be applied even for inputs with no slashes. /me likes, at least for refs/remotes/%1/%*. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
On Wed, May 15, 2013 at 9:39 AM, Johan Herland jo...@herland.net wrote: On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty mhag...@alum.mit.edu wrote: refs/remotes/%1/%2 (or refs/remotes/%1/%*) might be a nice way to imply that the rule should only be attempted if the input has at least two components, whereas something like refs/heads/%* would be applied even for inputs with no slashes. /me likes, at least for refs/remotes/%1/%*. Unfortunately, using refs/remotes/%1/%* instead of refs/remotes/%* breaks a number of git-svn tests which puts refs directly within refs/remotes/, and then does things like git reset --hard trunk (expecting trunk - refs/remotes/trunk, which the refs/remotes/%1/%* doesn't match). I don't know if putting refs directly within refs/remotes/ is something that git-svn does by default (which would prevent us from changing refs/remotes/%* to refs/remotes/%1/%*), or whether it is specific to the tests (in which case we should fix the tests). Also, there might be too many other users of refs directly within refs/remotes/ that expect foo_without_slash to expand to refs/remotes/foo_without_slash, which would prevent us from doing this change. ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
Johan Herland jo...@herland.net writes: On Wed, May 15, 2013 at 9:39 AM, Johan Herland jo...@herland.net wrote: On Wed, May 15, 2013 at 8:45 AM, Michael Haggerty mhag...@alum.mit.edu wrote: refs/remotes/%1/%2 (or refs/remotes/%1/%*) might be a nice way to imply that the rule should only be attempted if the input has at least two components, whereas something like refs/heads/%* would be applied even for inputs with no slashes. /me likes, at least for refs/remotes/%1/%*. Unfortunately, using refs/remotes/%1/%* instead of refs/remotes/%* breaks a number of git-svn tests which puts refs directly within refs/remotes/, and then does things like git reset --hard trunk (expecting trunk - refs/remotes/trunk, which the refs/remotes/%1/%* doesn't match). Oh, I never thought refs/remotes/%1/%* was a suggestion for a serious improvement, but was merely a thought experiment if all the remotes were at least two level names, we could express it like this to stress that fact. We already allow 'origin' to refer to refs/remotes/origin/HEAD, so it is clear refs/remotes/%1/%* alone will not be able to replace what we have; we need refs/remotes/%* and refs/remotes/%*/HEAD anyway. -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
Johan Herland jo...@herland.net wrote: Unfortunately, using refs/remotes/%1/%* instead of refs/remotes/%* breaks a number of git-svn tests which puts refs directly within refs/remotes/, and then does things like git reset --hard trunk (expecting trunk - refs/remotes/trunk, which the refs/remotes/%1/%* doesn't match). I don't know if putting refs directly within refs/remotes/ is something that git-svn does by default (which would prevent us from changing refs/remotes/%* to refs/remotes/%1/%*), or whether it is specific to the tests (in which case we should fix the tests). Yes, git-svn uses refs/remotes/%* by default. This was a design mistake on my part. I think it's too late to change, now, as existing users will encounter breakage and/or out-of-date documentation (which is even more confusing, as git-svn is often the first introduction to git for SVN users). -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
On Mon, May 13, 2013 at 10:34 PM, Junio C Hamano gits...@pobox.com wrote: Junio C Hamano gits...@pobox.com writes: Johan Herland jo...@herland.net writes: Obviously, I named it '%1' since it expands into the _first_ component of the (slash-separated) shorthand. OK, I can buy something like %* refs/%* refs/heads/%* ... refs/remotes/%*/HEAD refs/remotes/%1/%2 refs/peers/%1/heads/%2 that is, for a pattern that has %*, we feed the end-user string as a whole, and for a pattern that has %1 thru %N, we find an appropriate way to chop the end-user string into N pieces (e.g. nick/name would be split into %1 = nick, %2 = name, while foo/bar/baz might have two possibilities, %1, %2 = foo, bar/baz or foo/bar, baz). The earlier ones on the above list can even be written with their %* substituted with %1 if we go that route. Just to make sure. Please do not let two possibilities stop you. As I said in the nearby thread, I do not necessarily insist that we must try all N possibilities. We find an appropriate way could be just we always chop at the first slash, and %1 is what comes before it, %2 is what comes after it. That will close the possibility for us to use %1 thru %N (N is limited to 2), but it still is We have %1 and we have %2, both fall into the same 'path components, numbered from left to right' category, and justifies the use of %1 (one, not el). So still no objection to %1 from me. I think I like refs/peers/%1/heads/%* better than refs/peers/%1/heads/%2, since the latter sort of makes me wonder whether the 3rd, 4th, etc. components would be discarded. That said, having %* mean the rest of the shorthand means that we must adjust the expansion of %* for every preceding %N, which prevents us from simplifying the code by using strbuf_expand_dict_cb() with a static dictionary [1]. I am not sure why we would want refs/remotes/%1/%2 instead of refs/remote/%*. Maybe I've been staring at this for too long, but I find the latter shorter and more descriptive and the %1/%2 notation needlessly cumbersome, especially if it's also supposed to match foo/bar/baz When it comes to multi-level remote names, I guess I could drop the patch that disallows them, and still just have %1 only map to the first component (i.e. foo/bar/baz would always be interpreted as %1 = foo, and never %1 = foo/bar). This would mean that the foo/bar/baz shorthand notation would simply not work against remote-tracking branch baz from remote foo/bar, but we might say that's ok, because (a) multi-level remote names are so rare, and (b) the simple workaround of explicitly saying refs/peers/foo/bar/heads/baz would always be available in any case. ...Johan [1]: Maybe we could use '%N+' to mean everything starting from component #N? Then we could construct the following dictionary the shorthand foo/bar/baz: * - foo/bar/baz 1 - foo 1+ - foo/bar/baz 2 - bar 2+ - bar/baz etc. But I really think this is overkill. Avoiding having to write our own expansion helper is not _that_ important. -- Johan Herland, jo...@herland.net www.herland.net -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
Johan Herland jo...@herland.net writes: I think I like refs/peers/%1/heads/%* better than refs/peers/%1/heads/%2, since the latter sort of makes me wonder whether the 3rd, 4th, etc. components would be discarded. Makes sense. I am not sure why we would want refs/remotes/%1/%2 instead of refs/remote/%*. The former way makes it easier to see what refs/peers/%1/heads/%2 means, I think, but otherwise aren't they equivalent? I do not see a strong reason to favor one over the other. remote-tracking branch baz from remote foo/bar, but we might say that's ok, because (a) multi-level remote names are so rare, and (b) the simple workaround of explicitly saying refs/peers/foo/bar/heads/baz would always be available in any case. Sounds sensible. And if you limit things that way, %1 again starts to make sense, as you are representing the first path component with it. -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
On Mon, May 13, 2013 at 6:56 AM, Junio C Hamano gits...@pobox.com wrote: Johan Herland jo...@herland.net writes: The refname_expand() function no longer uses mkpath()/mksnpath() to perform the pattern expansion. Instead, it uses strbuf_expand(), which removes the need for using fixed-length buffers from the code. It is a brilliant idea to use strbuf_expand() for this. I like it. I notice that you later introduce %1 (that is 'one', not 'el'), but unless you are planning to introduce %2 and %3 that semantically fall into a similar category as %1, I would rather see a different letter used that is mnemonic to what the placeholder _means_. The choice of the letter is arbitrary and may not look like it matters that much, because it is not exposed to the end user. But by switching from the sprintf() semantics that shows things given to it in the order they were given, without knowing what they mean, and introducing a strbuf_expand() machinery tailored for refnames (and refnames only), the new code assigns meanings to each part of the refname, and we can afford to be more descriptive. The choice of '%*' is justifiable, it is the closest to the '*' we traditionally used to replace only one thing, but '%1' does not look the best placeholder to use, at least to me. Obviously, I named it '%1' since it expands into the _first_ component of the (slash-separated) shorthand. There is no further parsing or verification that it actually corresponds to a remote (and as far as I currently understand, we do not want to do such verification), so I thought it better not to make such assumptions in the placeholder name. That said, I could go with '%r' for remote, although we have plenty of other concepts in Git that use 'r' as the initial letter. I could maybe use '%remote' instead? Also, about the '%*': When used alone, it means the entire shorthand, but when preceded with a '%1' it subtly changes meaning into 'the remainder of the shorthand after extracting the first component'. I believe the two interpretations are compatible and unambiguous, but if we want to be very explicit about what's happening, we could use something like '%all' and '%the_rest' for the two cases, respectively? ...Johan -- Johan Herland, jo...@herland.net www.herland.net -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
Junio C Hamano gits...@pobox.com writes: Johan Herland jo...@herland.net writes: Obviously, I named it '%1' since it expands into the _first_ component of the (slash-separated) shorthand. OK, I can buy something like %* refs/%* refs/heads/%* ... refs/remotes/%*/HEAD refs/remotes/%1/%2 refs/peers/%1/heads/%2 that is, for a pattern that has %*, we feed the end-user string as a whole, and for a pattern that has %1 thru %N, we find an appropriate way to chop the end-user string into N pieces (e.g. nick/name would be split into %1 = nick, %2 = name, while foo/bar/baz might have two possibilities, %1, %2 = foo, bar/baz or foo/bar, baz). The earlier ones on the above list can even be written with their %* substituted with %1 if we go that route. Just to make sure. Please do not let two possibilities stop you. As I said in the nearby thread, I do not necessarily insist that we must try all N possibilities. We find an appropriate way could be just we always chop at the first slash, and %1 is what comes before it, %2 is what comes after it. That will close the possibility for us to use %1 thru %N (N is limited to 2), but it still is We have %1 and we have %2, both fall into the same 'path components, numbered from left to right' category, and justifies the use of %1 (one, not el). So still no objection to %1 from me. And that makes perfect sense, and is exactly the kind of you plan to have %2 and %3 that falls into the same category as %1 I was asking you about in the message. So, no more objection to %1 from me, if that is the direction you are taking us. -- 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: [PATCHv2 03/10] refs.c: Refactor code for mapping between shorthand names and full refnames
Johan Herland jo...@herland.net writes: The refname_expand() function no longer uses mkpath()/mksnpath() to perform the pattern expansion. Instead, it uses strbuf_expand(), which removes the need for using fixed-length buffers from the code. It is a brilliant idea to use strbuf_expand() for this. I like it. I notice that you later introduce %1 (that is 'one', not 'el'), but unless you are planning to introduce %2 and %3 that semantically fall into a similar category as %1, I would rather see a different letter used that is mnemonic to what the placeholder _means_. The choice of the letter is arbitrary and may not look like it matters that much, because it is not exposed to the end user. But by switching from the sprintf() semantics that shows things given to it in the order they were given, without knowing what they mean, and introducing a strbuf_expand() machinery tailored for refnames (and refnames only), the new code assigns meanings to each part of the refname, and we can afford to be more descriptive. The choice of '%*' is justifiable, it is the closest to the '*' we traditionally used to replace only one thing, but '%1' does not look the best placeholder to use, at least to me. -- 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
